Merge pull request 'feat: secret-in-args warning badge (#1r)' (#16) from feat/1r-secret-args-badge into main
feat: secret-in-args warning badge (#1r) — Closes #1
This commit was merged in pull request #16.
This commit is contained in:
@@ -449,6 +449,12 @@ class ServerEditor(QFrame):
|
|||||||
warn_row.addWidget(self.args_warn, 1)
|
warn_row.addWidget(self.args_warn, 1)
|
||||||
warn_row.addWidget(self.args_fix_btn)
|
warn_row.addWidget(self.args_fix_btn)
|
||||||
v.addLayout(warn_row)
|
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"))
|
v.addWidget(self._lbl("Environment variables"))
|
||||||
self.env = KeyValueTable(
|
self.env = KeyValueTable(
|
||||||
"Variable", "Value", on_change=self._emit, before_change=self._before_change
|
"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._set_dep({"status": "unknown", "label": "—"})
|
||||||
self.args_warn.hide()
|
self.args_warn.hide()
|
||||||
self.args_fix_btn.hide()
|
self.args_fix_btn.hide()
|
||||||
|
self.secret_warn.hide()
|
||||||
self._loading = False
|
self._loading = False
|
||||||
return
|
return
|
||||||
self.setEnabled(True)
|
self.setEnabled(True)
|
||||||
@@ -581,6 +588,7 @@ class ServerEditor(QFrame):
|
|||||||
if self.type.currentIndex() != 0: # stdio page only
|
if self.type.currentIndex() != 0: # stdio page only
|
||||||
self.args_warn.hide()
|
self.args_warn.hide()
|
||||||
self.args_fix_btn.hide()
|
self.args_fix_btn.hide()
|
||||||
|
self.secret_warn.hide()
|
||||||
return
|
return
|
||||||
_, notes = core.split_suspicious_args(self._current_arg_lines())
|
_, notes = core.split_suspicious_args(self._current_arg_lines())
|
||||||
if notes:
|
if notes:
|
||||||
@@ -590,6 +598,12 @@ class ServerEditor(QFrame):
|
|||||||
else:
|
else:
|
||||||
self.args_warn.hide()
|
self.args_warn.hide()
|
||||||
self.args_fix_btn.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):
|
def _fix_args(self):
|
||||||
if self._before_change:
|
if self._before_change:
|
||||||
|
|||||||
+46
@@ -801,6 +801,10 @@ _TOKEN_PREFIXES = (
|
|||||||
|
|
||||||
MASK = "••••••••"
|
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:
|
def is_secret_key(name: str) -> bool:
|
||||||
"""Does this env-var / header / flag name look like it holds a secret?"""
|
"""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
|
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]]:
|
def split_suspicious_args(args: list[str]) -> tuple[list[str], list[str]]:
|
||||||
"""
|
"""
|
||||||
Detect the classic argument-entry mistake: several argv tokens typed on one
|
Detect the classic argument-entry mistake: several argv tokens typed on one
|
||||||
|
|||||||
@@ -583,3 +583,55 @@ def test_merge_and_reapply_preserves_both_edits(tmp_path):
|
|||||||
assert result["numStartups"] == 42 # external change preserved
|
assert result["numStartups"] == 42 # external change preserved
|
||||||
assert "s2" in result["mcpServers"] # user's server edit preserved
|
assert "s2" in result["mcpServers"] # user's server edit preserved
|
||||||
assert "s1" not in result["mcpServers"] # old server replaced
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user