From f4648b3c061f13b27537ad209566de94312237be Mon Sep 17 00:00:00 2001 From: AJ Date: Thu, 2 Jul 2026 11:59:17 -0400 Subject: [PATCH] feat: warn when a secret-shaped value is found in args instead of env (#1r) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `args_secret_warning(data)` to bcc_core — returns a warning string when any arg positional value looks like a raw credential (token prefix, value following a secret-named flag, or URL with embedded user:pass like postgres://user:pass@host). `--flag=value` inline forms are intentionally skipped (the flag name already labels the value). Adds `secret_warn` QLabel in ServerEditor's stdio page; shown/hidden by `_check_args()` on every field change, and cleared on deselect or stdio→remote type switch. Non-blocking — save path is not touched. Closes #1 Co-Authored-By: Claude Sonnet 4.6 --- bcc.py | 14 +++++++++++++ bcc_core.py | 46 ++++++++++++++++++++++++++++++++++++++++ tests/test_core.py | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+) diff --git a/bcc.py b/bcc.py index 42b4bc5..599eaac 100644 --- a/bcc.py +++ b/bcc.py @@ -449,6 +449,12 @@ class ServerEditor(QFrame): warn_row.addWidget(self.args_warn, 1) warn_row.addWidget(self.args_fix_btn) v.addLayout(warn_row) + # Shown when an arg value looks like a raw credential. + self.secret_warn = QLabel("") + self.secret_warn.setStyleSheet(f"color: {WARN};") + self.secret_warn.setWordWrap(True) + self.secret_warn.hide() + v.addWidget(self.secret_warn) v.addWidget(self._lbl("Environment variables")) self.env = KeyValueTable( "Variable", "Value", on_change=self._emit, before_change=self._before_change @@ -495,6 +501,7 @@ class ServerEditor(QFrame): self._set_dep({"status": "unknown", "label": "—"}) self.args_warn.hide() self.args_fix_btn.hide() + self.secret_warn.hide() self._loading = False return self.setEnabled(True) @@ -581,6 +588,7 @@ class ServerEditor(QFrame): if self.type.currentIndex() != 0: # stdio page only self.args_warn.hide() self.args_fix_btn.hide() + self.secret_warn.hide() return _, notes = core.split_suspicious_args(self._current_arg_lines()) if notes: @@ -590,6 +598,12 @@ class ServerEditor(QFrame): else: self.args_warn.hide() self.args_fix_btn.hide() + warning = core.args_secret_warning({"args": self._current_arg_lines()}) + if warning: + self.secret_warn.setText("⚠ " + warning) + self.secret_warn.show() + else: + self.secret_warn.hide() def _fix_args(self): if self._before_change: diff --git a/bcc_core.py b/bcc_core.py index f2a8f61..dc41ba3 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -801,6 +801,10 @@ _TOKEN_PREFIXES = ( MASK = "••••••••" +# Matches userinfo credentials embedded in a URL: scheme://user:pass@host +# Fires on postgres://user:pass@host but NOT on https://host/path or ssh://user@host. +_EMBEDDED_CRED_RE = re.compile(r"://[^:@/\s]+:[^:@/\s]+@") + def is_secret_key(name: str) -> bool: """Does this env-var / header / flag name look like it holds a secret?""" @@ -841,6 +845,48 @@ def redact_args(args: list[str]) -> list[str]: return out +def args_secret_warning(data: dict) -> str | None: + """ + Return a warning string when any arg looks like a raw secret that would + be better placed in `env`. Returns None when no concern is found. + + Skips --flag=value inline pairs (already partially self-documenting). + Fires on: + - positional values that start with a well-known token prefix (ghp_, sk-, …) + - values that follow a secret-named flag (--token abc, --api-key abc) + - URLs with embedded user:pass credentials (postgres://user:pass@host) + """ + args = [str(a) for a in (data.get("args") or [])] + mask_next = False + for a in args: + if mask_next: + mask_next = False + if not a.startswith("-"): + return ( + "An arg value following a secret-named flag looks like a credential. " + "Where the server supports it, prefer Environment variables — " + "args are visible in process listings." + ) + continue + # --flag=value inline: skip (the flag name already labels it) + if a.startswith("-") and "=" in a: + continue + # --secretflag (no inline value): flag the next positional arg + if a.startswith("-") and is_secret_key(a): + mask_next = True + continue + if a.startswith("-"): + continue + # Positional value: check for token prefix or embedded URL credentials + if _is_secret_value(a) or _EMBEDDED_CRED_RE.search(a): + return ( + "An arg value looks like a credential. " + "Where the server supports it, prefer Environment variables — " + "args are visible in process listings." + ) + return None + + def split_suspicious_args(args: list[str]) -> tuple[list[str], list[str]]: """ Detect the classic argument-entry mistake: several argv tokens typed on one diff --git a/tests/test_core.py b/tests/test_core.py index 303b8bf..bea00a3 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -583,3 +583,55 @@ def test_merge_and_reapply_preserves_both_edits(tmp_path): assert result["numStartups"] == 42 # external change preserved assert "s2" in result["mcpServers"] # user's server edit preserved assert "s1" not in result["mcpServers"] # old server replaced + + +# --------------------------------------------------------------------------- # +# 10 — Secret-in-args warning +# --------------------------------------------------------------------------- # + + +def test_args_secret_warning_embedded_url_creds(): + """AC case: postgres://user:pass@host triggers a warning.""" + data = {"args": ["--connection", "postgres://user:pass@host/db"]} + assert c.args_secret_warning(data) is not None + + +def test_args_secret_warning_token_prefix_positional(): + """A bare ghp_… token as a positional arg triggers a warning.""" + data = {"args": ["ghp_abc123def456"]} + assert c.args_secret_warning(data) is not None + + +def test_args_secret_warning_secret_flag_value_pair(): + """Value following a secret-named flag triggers a warning.""" + data = {"args": ["--token", "supersecret"]} + assert c.args_secret_warning(data) is not None + + +def test_args_secret_warning_benign_url_no_password(): + """https:// URL without user:pass should NOT warn.""" + data = {"args": ["--endpoint", "https://mcp.example.com/sse"]} + assert c.args_secret_warning(data) is None + + +def test_args_secret_warning_user_at_host_no_password(): + """ssh://user@host without a password should NOT warn.""" + data = {"args": ["ssh://git@github.com"]} + assert c.args_secret_warning(data) is None + + +def test_args_secret_warning_inline_flag_value_skipped(): + """--flag=value inline pairs are self-documenting and should NOT warn.""" + data = {"args": ["--api-key=sk-abc123"]} + assert c.args_secret_warning(data) is None + + +def test_args_secret_warning_env_not_triggered(): + """Secrets in env (not args) must not trigger args warning, even with benign args present.""" + data = {"args": ["--verbose"], "env": {"DATABASE_URL": "postgres://user:pass@host/db"}} + assert c.args_secret_warning(data) is None + + +def test_args_secret_warning_empty(): + assert c.args_secret_warning({}) is None + assert c.args_secret_warning({"args": []}) is None