From 7368dcdbffc8190e565fbb88a892b47b5cbf6cfa Mon Sep 17 00:00:00 2001 From: the_og Date: Tue, 11 Aug 2026 15:58:17 +0000 Subject: [PATCH] feat(#88): detect & migrate removed CLI flags into env (ssh-mcp v2) ssh-mcp v2 removed --password from the command line and reads SSH_MCP_PASSWORD instead, so an old config crashes on startup. Add a data-driven FLAG_ENV_MIGRATIONS registry plus detect_migratable_package, migrate_removed_flags and removed_flag_warnings in bcc_core, and a 'Fix: move to environment variables' one-click action + warning in the stdio server editor, with a matching main-window lint line. The literal value lands in env{} (the only form Claude Desktop honours). 14 tests. --- bcc.py | 49 +++++++++++ bcc_core.py | 199 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_core.py | 147 +++++++++++++++++++++++++++++++++ 3 files changed, 395 insertions(+) diff --git a/bcc.py b/bcc.py index dc224f7..f7071ce 100644 --- a/bcc.py +++ b/bcc.py @@ -602,6 +602,19 @@ class ServerEditor(QFrame): self.secret_warn.setWordWrap(True) self.secret_warn.hide() v.addWidget(self.secret_warn) + # Shown when args carry a CLI flag a package removed in a major version + # (e.g. ssh-mcp v2's --password), with a one-click move into env. + self.removed_flag_warn = QLabel("") + self.removed_flag_warn.setStyleSheet(f"color: {WARN};") + self.removed_flag_warn.setWordWrap(True) + self.removed_flag_warn.hide() + self.removed_flag_fix_btn = QPushButton("Fix: move to environment variables") + self.removed_flag_fix_btn.clicked.connect(self._fix_removed_flags) + self.removed_flag_fix_btn.hide() + rf_row = QHBoxLayout() + rf_row.addWidget(self.removed_flag_warn, 1) + rf_row.addWidget(self.removed_flag_fix_btn) + v.addLayout(rf_row) v.addWidget(self._lbl("Environment variables")) self.env = KeyValueTable( "Variable", "Value", on_change=self._emit, before_change=self._before_change @@ -649,6 +662,8 @@ class ServerEditor(QFrame): self.args_warn.hide() self.args_fix_btn.hide() self.secret_warn.hide() + self.removed_flag_warn.hide() + self.removed_flag_fix_btn.hide() self._loading = False return self.setEnabled(True) @@ -736,6 +751,8 @@ class ServerEditor(QFrame): self.args_warn.hide() self.args_fix_btn.hide() self.secret_warn.hide() + self.removed_flag_warn.hide() + self.removed_flag_fix_btn.hide() return _, notes = core.split_suspicious_args(self._current_arg_lines()) if notes: @@ -751,6 +768,23 @@ class ServerEditor(QFrame): self.secret_warn.show() else: self.secret_warn.hide() + # Flags a package removed across a major version (e.g. ssh-mcp v2's + # --password). Needs command + env too, so build from the live form. + stdio = { + "command": self.command.text().strip(), + "args": self._current_arg_lines(), + "env": self.env.dump(), + } + rf_warnings = core.removed_flag_warnings(stdio) + if rf_warnings: + self.removed_flag_warn.setText("⚠ " + "\n".join(rf_warnings)) + self.removed_flag_warn.show() + # Only offer the button when something is actually auto-migratable. + migrated, _ = core.migrate_removed_flags(stdio) + self.removed_flag_fix_btn.setVisible(migrated is not stdio) + else: + self.removed_flag_warn.hide() + self.removed_flag_fix_btn.hide() def _fix_args(self): if self._before_change: @@ -758,6 +792,19 @@ class ServerEditor(QFrame): fixed, _ = core.split_suspicious_args(self._current_arg_lines()) self.args.setPlainText("\n".join(fixed)) # triggers _emit -> recheck + def _fix_removed_flags(self): + if self._before_change: + self._before_change() + stdio = { + "command": self.command.text().strip(), + "args": self._current_arg_lines(), + "env": self.env.dump(), + } + migrated, _ = core.migrate_removed_flags(stdio) + # Reload env first, then args; setting args text triggers _emit -> recheck. + self.env.load(migrated.get("env", {})) + self.args.setPlainText("\n".join(migrated.get("args", []))) + # --- dependency ------------------------------------------------------ # def refresh_dependency(self, auto_open=False): if not self.isEnabled(): @@ -2633,6 +2680,8 @@ class MainWindow(QMainWindow): for entry in self.servers: for warning in core.env_ref_warnings(entry.data, self.current_profile): lint_warnings.append(f"'{entry.name}': {warning}") + for warning in core.removed_flag_warnings(entry.data): + lint_warnings.append(f"'{entry.name}': {warning}") if lint_warnings: self.validation_lbl.setText(f"⚠ {lint_warnings[0]}") self.validation_lbl.setStyleSheet(f"color: {WARN};") diff --git a/bcc_core.py b/bcc_core.py index d709b28..bc7466f 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -1964,6 +1964,205 @@ def _looks_like_multiple_args(a: str) -> bool: return len(toks) > 1 and any(t.startswith("-") for t in toks) +# --------------------------------------------------------------------------- # +# Removed-flag → env migration +# +# Some MCP servers moved credential CLI flags into environment variables across +# a major version (secrets on the command line are visible to any local user in +# process listings). A config written for the old version then fails hard on the +# new one — e.g. ssh-mcp v2 exits with "These flags were removed in v2: +# --password". This registry lets BCC recognise that shape, warn about it, and +# offer a one-click migration that lifts the value out of `args` into `env`. +# +# Per package: +# "env" removed flag -> env var that now supplies it. BCC auto-migrates +# these: the flag (and its value) leave `args`, the value lands in +# `env` under the mapped name. +# "removed" flags that no longer exist but have no confirmed 1:1 env +# replacement (e.g. a boolean, or an env name not documented here). +# BCC only warns; the user resolves these by hand. +# +# Extending it: add a package key and its flag maps. Keep "env" limited to +# mappings whose env-var name is verified, so the auto-fix stays trustworthy. +# --------------------------------------------------------------------------- # +FLAG_ENV_MIGRATIONS: dict[str, dict[str, dict[str, str]]] = { + "ssh-mcp": { + "env": { + "--password": "SSH_MCP_PASSWORD", + }, + "removed": { + "--sudoPassword": ( + "removed in ssh-mcp v2 — supply it via an environment variable " + "instead (see the ssh-mcp README's 'Migrating from v1' section)" + ), + "--suPassword": ( + "removed in ssh-mcp v2 — supply it via an environment variable " + "instead (see the ssh-mcp README's 'Migrating from v1' section)" + ), + "--disableSudo": ( + "removed in ssh-mcp v2 — sudo is now governed by the server " + "config/policy rather than a CLI flag" + ), + }, + }, +} + + +def _normalize_pkg_token(token: str) -> str: + """Reduce an argv token to a bare npm package name for registry lookup. + + Strips any leading path (``/usr/local/bin/ssh-mcp`` -> ``ssh-mcp``) and a + trailing ``@version`` (``ssh-mcp@2.0.1`` -> ``ssh-mcp``), while preserving a + leading scope (``@acme/ssh-mcp`` stays intact). + """ + t = str(token or "").strip() + if not t or t.startswith("-"): + return "" + # Drop a path prefix but keep an npm scope (leading '@' with no earlier '/'). + if "/" in t and not t.startswith("@"): + t = t.rsplit("/", 1)[-1] + # Strip a trailing @version. For scoped names, only the version '@' counts: + # split on the LAST '@' when it isn't the scope's leading one. + at = t.rfind("@") + if at > 0: # >0 so a scope's leading '@' at index 0 is untouched + t = t[:at] + return t + + +def detect_migratable_package(data: dict) -> str | None: + """Return the FLAG_ENV_MIGRATIONS key this stdio server runs, or None. + + Scans the command and every arg (so ``npx -y ssh-mcp`` and a direct + ``command: ssh-mcp`` both resolve), matching on the normalised package name. + """ + if not isinstance(data, dict): + return None + tokens = [data.get("command", "")] + tokens.extend(data.get("args") or []) + for tok in tokens: + name = _normalize_pkg_token(tok) + if name in FLAG_ENV_MIGRATIONS: + return name + return None + + +def migrate_removed_flags(data: dict) -> tuple[dict, list[str]]: + """Move known removed credential flags out of `args` and into `env`. + + Returns ``(new_data, notes)``. When there is nothing to migrate the original + dict is returned unchanged with an empty notes list, so callers can cheaply + treat an empty notes list as "no change". + + Only flags in the package's ``env`` map are rewritten. Both spellings are + handled: ``--password secret`` (value on the next token) and + ``--password=secret`` (inline). An existing ``env`` value for the target + name is never overwritten — the redundant flag is dropped and noted instead. + A ``${VAR}`` reference is migrated verbatim (env is the right home for it). + Flags listed under ``removed`` are left untouched here; see + ``removed_flag_warnings`` for those. + """ + pkg = detect_migratable_package(data) + if pkg is None: + return data, [] + env_map = FLAG_ENV_MIGRATIONS[pkg]["env"] + args = [str(a) for a in (data.get("args") or [])] + if not args: + return data, [] + + new_args: list[str] = [] + new_env: dict = dict(data.get("env") or {}) + notes: list[str] = [] + i = 0 + n = len(args) + changed = False + while i < n: + a = args[i] + # Inline form: --flag=value + if a.startswith("-") and "=" in a and a.split("=", 1)[0] in env_map: + flag, value = a.split("=", 1) + env_name = env_map[flag] + changed = True + if env_name in new_env and new_env[env_name] != value: + notes.append(f"{flag} dropped from args ({env_name} is already set in env)") + else: + new_env[env_name] = value + notes.append(f"moved {flag} into env as {env_name}") + i += 1 + continue + # Separate form: --flag value + if a in env_map: + flag = a + env_name = env_map[flag] + has_value = i + 1 < n and not args[i + 1].startswith("-") + if not has_value: + # No value to move (unexpected for a credential flag). Drop the + # bare flag so the server can start, and say so. + changed = True + notes.append(f"removed {flag} from args (no value found to move)") + i += 1 + continue + value = args[i + 1] + changed = True + if env_name in new_env and new_env[env_name] != value: + notes.append(f"{flag} dropped from args ({env_name} is already set in env)") + else: + new_env[env_name] = value + notes.append(f"moved {flag} into env as {env_name}") + i += 2 + continue + new_args.append(a) + i += 1 + + if not changed: + return data, [] + + new_data = dict(data) + if new_args: + new_data["args"] = new_args + else: + new_data.pop("args", None) + if new_env: + new_data["env"] = new_env + return new_data, notes + + +def removed_flag_warnings(data: dict) -> list[str]: + """Advisory lines for every removed-in-a-major-version flag still present. + + Covers both the auto-migratable flags (reported as a one-click fix being + available) and the manual-only ones. Empty list == nothing to flag. + """ + pkg = detect_migratable_package(data) + if pkg is None: + return [] + spec = FLAG_ENV_MIGRATIONS[pkg] + env_map = spec["env"] + removed_map = spec.get("removed", {}) + present = set() + for a in data.get("args") or []: + s = str(a) + present.add(s.split("=", 1)[0] if s.startswith("-") and "=" in s else s) + + out: list[str] = [] + migratable = [f for f in env_map if f in present] + if migratable: + flags = ", ".join(sorted(migratable)) + out.append( + f"{pkg}: {flags} was removed from the command line — its value should " + f"live in an environment variable. Use “Fix” to move it into env." + ) + for flag in sorted(removed_map): + if flag in present: + out.append(f"{pkg}: {flag} {removed_map[flag]}") + return out + + +def removed_flag_warning(data: dict) -> str | None: + """First removed-flag advisory for a single-line UI label, or None.""" + warnings = removed_flag_warnings(data) + return warnings[0] if warnings else None + + # --------------------------------------------------------------------------- # # Validation # --------------------------------------------------------------------------- # diff --git a/tests/test_core.py b/tests/test_core.py index fd36e80..4ed5661 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -3059,3 +3059,150 @@ def test_discover_project_configs_skips_non_object_and_garbage(tmp_path): assert str(array / ".mcp.json") not in paths assert str(garbage / ".mcp.json") not in paths assert str(missing / ".mcp.json") not in paths + + +# --------------------------------------------------------------------------- # +# Removed-flag -> env migration (ssh-mcp v2 and the general mechanism) +# --------------------------------------------------------------------------- # +def test_detect_migratable_package_via_npx_args(): + data = {"command": "npx", "args": ["-y", "ssh-mcp", "--host=h", "--password=p"]} + assert c.detect_migratable_package(data) == "ssh-mcp" + + +def test_detect_migratable_package_direct_command_and_path_and_version(): + assert c.detect_migratable_package({"command": "ssh-mcp", "args": []}) == "ssh-mcp" + assert ( + c.detect_migratable_package({"command": "/usr/local/bin/ssh-mcp", "args": []}) == "ssh-mcp" + ) + assert ( + c.detect_migratable_package({"command": "npx", "args": ["-y", "ssh-mcp@2.0.1"]}) + == "ssh-mcp" + ) + + +def test_detect_migratable_package_unknown_returns_none(): + assert c.detect_migratable_package({"command": "npx", "args": ["some-other"]}) is None + assert c.detect_migratable_package({}) is None + assert c.detect_migratable_package({"url": "https://x"}) is None + + +def test_migrate_removed_flags_inline_form(): + data = { + "command": "npx", + "args": ["-y", "ssh-mcp", "--", "--host=1.2.3.4", "--password=hunter2"], + } + new, notes = c.migrate_removed_flags(data) + assert new["args"] == ["-y", "ssh-mcp", "--", "--host=1.2.3.4"] + assert new["env"] == {"SSH_MCP_PASSWORD": "hunter2"} + assert any("SSH_MCP_PASSWORD" in n for n in notes) + # Original untouched (pure function). + assert "env" not in data + + +def test_migrate_removed_flags_separate_form(): + data = { + "command": "npx", + "args": ["-y", "ssh-mcp", "--user", "root", "--password", "s3cret"], + } + new, _ = c.migrate_removed_flags(data) + assert new["args"] == ["-y", "ssh-mcp", "--user", "root"] + assert new["env"] == {"SSH_MCP_PASSWORD": "s3cret"} + + +def test_migrate_removed_flags_preserves_existing_env_and_merges(): + data = { + "command": "npx", + "args": ["ssh-mcp", "--password=p"], + "env": {"OTHER": "keep"}, + } + new, _ = c.migrate_removed_flags(data) + assert new["env"] == {"OTHER": "keep", "SSH_MCP_PASSWORD": "p"} + + +def test_migrate_removed_flags_does_not_clobber_existing_secret(): + data = { + "command": "npx", + "args": ["ssh-mcp", "--password=fromargs"], + "env": {"SSH_MCP_PASSWORD": "fromenv"}, + } + new, notes = c.migrate_removed_flags(data) + # env value wins; the redundant flag is still stripped so v2 can start. + assert new["env"] == {"SSH_MCP_PASSWORD": "fromenv"} + assert "--password=fromargs" not in new["args"] + assert any("already set" in n for n in notes) + + +def test_migrate_removed_flags_moves_env_ref_verbatim(): + data = {"command": "npx", "args": ["ssh-mcp", "--password", "${MY_PW}"]} + new, _ = c.migrate_removed_flags(data) + assert new["env"] == {"SSH_MCP_PASSWORD": "${MY_PW}"} + assert "${MY_PW}" not in new["args"] + + +def test_migrate_removed_flags_noop_returns_same_object(): + data = {"command": "npx", "args": ["ssh-mcp", "--host=h", "--user=u"]} + new, notes = c.migrate_removed_flags(data) + assert new is data + assert notes == [] + + +def test_migrate_removed_flags_unknown_package_noop(): + data = {"command": "npx", "args": ["mystery", "--password=p"]} + new, notes = c.migrate_removed_flags(data) + assert new is data and notes == [] + + +def test_migrate_removed_flags_only_args_left_drops_args_key(): + data = {"command": "ssh-mcp", "args": ["--password=p"]} + new, _ = c.migrate_removed_flags(data) + assert "args" not in new + assert new["env"] == {"SSH_MCP_PASSWORD": "p"} + + +def test_removed_flag_warnings_covers_migratable_and_manual(): + data = { + "command": "npx", + "args": ["ssh-mcp", "--password=p", "--sudoPassword=s", "--disableSudo"], + } + warnings = c.removed_flag_warnings(data) + text = " ".join(warnings) + assert "--password" in text + assert "sudoPassword" in text + assert "disableSudo" in text + # migrate only touches the confirmed --password mapping + new, _ = c.migrate_removed_flags(data) + assert new["env"] == {"SSH_MCP_PASSWORD": "p"} + assert "--sudoPassword=s" in new["args"] + assert "--disableSudo" in new["args"] + + +def test_removed_flag_warnings_empty_when_clean(): + assert c.removed_flag_warnings({"command": "ssh-mcp", "args": ["--host=h"]}) == [] + assert c.removed_flag_warning({"command": "ssh-mcp", "args": ["--host=h"]}) is None + + +def test_ssh_membermatters_end_to_end(): + # The exact shape from AJ's failing server. + data = { + "command": "npx", + "args": [ + "-y", + "ssh-mcp", + "--", + "--host=member.example.io", + "--user=deploy", + "--port=22", + "--password=topsecret", + ], + } + new, notes = c.migrate_removed_flags(data) + assert new["args"] == [ + "-y", + "ssh-mcp", + "--", + "--host=member.example.io", + "--user=deploy", + "--port=22", + ] + assert new["env"] == {"SSH_MCP_PASSWORD": "topsecret"} + assert notes