From 8c51c252112c5f8b7cfcbd7930de7748ff74d1ac Mon Sep 17 00:00:00 2001 From: Cowork Supervisor Date: Wed, 12 Aug 2026 02:59:49 -0400 Subject: [PATCH] feat(#91): sidecar config detection + precedence + verified paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ssh-mcp v2 reads a TOML sidecar and only falls back to CLI args when that file is ABSENT — so BCC's managed --host/--user args can be completely inert while the real config lives in a file BCC never looks at. This adds read-only truth-telling for that (no sidecar writing). Core (pure, fully injectable platform/environ/home/exists/read for testing): - sidecar_path(spec): VERIFIED per-platform TOML location from the package source, NOT the README (macOS → ~/Library/Application Support/ssh-mcp, Windows → %APPDATA%, else → ${XDG_CONFIG_HOME:-~/.config}). sidecar_doc_path() is the README path. - sidecar_status(): resolves exists / has_managed_args / args_inert / wrong_path. - sidecar_warnings(): mirrors removed_flag_warnings' shape. Reports: * precedence — "these arguments are inert; the server reads " * wrong-path — a TOML at the README path the server never actually reads * #11 credential scoping — an unprefixed SSH_MCP_PASSWORD shared across 2+ profiles in a multi-profile sidecar (count_toml_profiles is a documented 3.10-safe heuristic; unscoped_credential_warning gates on it). GUI: a read-only advisory label in the stdio editor (mirrors the removed-flag label; no fix button — editing the sidecar is a separate deliberate action). Uses the real platform/env/filesystem so it reflects this machine. pytest green, ruff + format clean. Closes #91. Part of epic #94. Co-Authored-By: Claude Opus 4.8 --- bcc.py | 19 ++++ bcc_core.py | 242 +++++++++++++++++++++++++++++++++++++++++++++ tests/test_core.py | 124 +++++++++++++++++++++++ 3 files changed, 385 insertions(+) diff --git a/bcc.py b/bcc.py index afbf20b..f4d1151 100644 --- a/bcc.py +++ b/bcc.py @@ -899,6 +899,15 @@ class ServerEditor(QFrame): rf_row.addWidget(self.removed_flag_warn, 1) rf_row.addWidget(self.removed_flag_fix_btn) v.addLayout(rf_row) + # Shown when a server reads a config sidecar (ssh-mcp's TOML): the args + # may be inert, or a file may sit at the README path the server never + # reads. Read-only advisory (#91) — no auto-fix; editing the sidecar is + # a separate, deliberate action. + self.sidecar_warn = QLabel("") + self.sidecar_warn.setStyleSheet(f"color: {WARN};") + self.sidecar_warn.setWordWrap(True) + self.sidecar_warn.hide() + v.addWidget(self.sidecar_warn) v.addWidget(self._lbl("Environment variables")) self.env = KeyValueTable( "Variable", "Value", on_change=self._emit, before_change=self._before_change @@ -1000,6 +1009,7 @@ class ServerEditor(QFrame): self.secret_warn.hide() self.removed_flag_warn.hide() self.removed_flag_fix_btn.hide() + self.sidecar_warn.hide() self._loading = False return self.setEnabled(True) @@ -1089,6 +1099,7 @@ class ServerEditor(QFrame): self.secret_warn.hide() self.removed_flag_warn.hide() self.removed_flag_fix_btn.hide() + self.sidecar_warn.hide() return _, notes = core.split_suspicious_args(self._current_arg_lines()) if notes: @@ -1121,6 +1132,14 @@ class ServerEditor(QFrame): else: self.removed_flag_warn.hide() self.removed_flag_fix_btn.hide() + # Sidecar precedence / wrong-path / credential-scoping (#91). Uses the + # real platform + environment + filesystem so it reflects this machine. + sc_warnings = core.sidecar_warnings(stdio) + if sc_warnings: + self.sidecar_warn.setText("⚠ " + "\n\n".join(sc_warnings)) + self.sidecar_warn.show() + else: + self.sidecar_warn.hide() def _fix_args(self): if self._before_change: diff --git a/bcc_core.py b/bcc_core.py index 23804ca..c303b8b 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -2535,6 +2535,248 @@ def drift_warnings(data: dict) -> list[str]: return out +# --------------------------------------------------------------------------- # +# Sidecar config detection + precedence (issue #91, epic #94) +# +# Some MCP servers read a config *sidecar* (ssh-mcp v2 reads a TOML file) and +# only fall back to CLI args when that file is ABSENT. So BCC's carefully-managed +# --host/--user args can be completely inert while the real config lives in a file +# BCC never looks at. Two traps this surfaces (read-only — no sidecar writing): +# +# 1. Precedence: when the sidecar exists, the args are inert. Tell the user +# where the file the server actually reads is. +# 2. Verified vs documented paths: ssh-mcp's README says ~/.config/ssh-mcp/…, +# but the code resolves the OS-native app-data dir on macOS/Windows. A user +# following the README writes a file the server never reads, with no error. +# +# Plus the #11 credential-scoping trap: an unprefixed SSH_MCP_PASSWORD is offered +# to *every* profile in a multi-profile TOML — detect that and suggest scoping. +# +# All resolution is injectable (platform / environ / home / exists / read) so the +# logic is unit-testable with fixtures and never touches the real filesystem in CI. +# --------------------------------------------------------------------------- # +# ${VAR} and ${VAR:-default} expansion inside a sidecar path template. +_SIDECAR_VAR_RE = re.compile(r"\$\{([A-Z_][A-Z0-9_]*)(?::-([^}]*))?\}") + +# The connection-defining args a sidecar would override (rendering them inert). +_SIDECAR_MANAGED_FLAGS = ("--host", "--user", "--port", "--identity", "--key") + + +def _sidecar_platform_key(platform: str) -> str: + """Map a sys.platform-style string to a ServerSpec.sidecar_paths key.""" + if platform == "darwin": + return "darwin" + if platform.startswith("win"): + return "win32" + return "posix" + + +def _expand_sidecar_template(tmpl: str, environ: dict, home: Path) -> str: + """Expand ${VAR}/${VAR:-default} and a leading ~ in a path template. + + Injectable so tests drive it with a fixture environ + home rather than the + real process environment. An unset ${VAR} with no default expands to "". + """ + + def sub(m: re.Match) -> str: + var, default = m.group(1), m.group(2) + val = environ.get(var) + if val: + return val + return default if default is not None else "" + + s = _SIDECAR_VAR_RE.sub(sub, tmpl) + # A leading ~ (either literal, or introduced by a ${XDG:-~/.config} default). + if s.startswith("~"): + s = str(home) + s[1:] + return s + + +def sidecar_path( + spec: ServerSpec | None, + *, + platform: str | None = None, + environ: dict | None = None, + home: str | os.PathLike | None = None, +) -> Path | None: + """The VERIFIED sidecar config path for `spec` on `platform`, or None. + + None when the package has no sidecar, or the platform has no entry. Uses the + package-source-derived paths in ServerSpec.sidecar_paths — NOT the README, + which is wrong on macOS/Windows. All inputs are injectable for testing. + """ + if spec is None or not spec.sidecar_paths: + return None + platform = platform if platform is not None else sys.platform + environ = environ if environ is not None else dict(os.environ) + home = Path(home) if home is not None else Path.home() + tmpl = spec.sidecar_paths.get(_sidecar_platform_key(platform)) + if not tmpl: + return None + return Path(_expand_sidecar_template(tmpl, environ, home)) + + +def sidecar_doc_path( + spec: ServerSpec | None, + *, + home: str | os.PathLike | None = None, +) -> Path | None: + """The path the package's README documents (but may not actually read).""" + if spec is None or not spec.sidecar_doc_path: + return None + home = Path(home) if home is not None else Path.home() + return Path(_expand_sidecar_template(spec.sidecar_doc_path, {}, home)) + + +def _has_managed_connection_args(data: dict) -> bool: + """True if the server carries connection args a sidecar would make inert.""" + return any(str(a).split("=", 1)[0] in _SIDECAR_MANAGED_FLAGS for a in data.get("args") or []) + + +def sidecar_status( + data: dict, + *, + platform: str | None = None, + environ: dict | None = None, + home: str | os.PathLike | None = None, + exists=None, +) -> dict | None: + """Resolve sidecar presence + precedence for one server. Pure/injectable. + + Returns None when the server isn't a package with a sidecar. Otherwise a + dict: ``{package, path, doc_path, exists, doc_exists, has_managed_args, + args_inert, wrong_path}``. ``exists`` is a callable ``Path -> bool`` so tests + inject a virtual filesystem; it defaults to a real ``Path.is_file`` check. + """ + spec = resolve_server_spec(data) + if spec is None: + return None + path = sidecar_path(spec, platform=platform, environ=environ, home=home) + if path is None: + return None + doc = sidecar_doc_path(spec, home=home) + exists = exists if exists is not None else (lambda p: Path(p).is_file()) + file_exists = bool(exists(path)) + # Only count the doc path as a separate "wrong place" when it's genuinely a + # different location from the real one (on Linux they coincide). + doc_exists = bool(doc is not None and doc != path and exists(doc)) + has_managed = _has_managed_connection_args(data) + return { + "package": spec.package, + "path": path, + "doc_path": doc, + "exists": file_exists, + "doc_exists": doc_exists, + "has_managed_args": has_managed, + # Precedence: the sidecar wins, so managed args are inert only when it exists. + "args_inert": file_exists and has_managed, + # A file sits where the README says but not where the server actually reads. + "wrong_path": doc_exists and not file_exists, + } + + +def count_toml_profiles(text: str) -> int: + """Best-effort count of profile sections in a TOML sidecar. + + ssh-mcp's multi-profile mode defines several named tables; an unprefixed + credential is shared across all of them (the #11 trap). Without a TOML parser + on the 3.10 baseline, this counts top-level ``[table]`` and ``[[array]]`` + headers (ignoring comments and dotted sub-keys) as a conservative proxy for + "how many profiles are defined". Heuristic — see #91 notes; the exact ssh-mcp + schema should be confirmed before this drives anything destructive. + """ + seen: set[str] = set() + count = 0 + for raw in (text or "").splitlines(): + line = raw.strip() + if not line or line.startswith("#"): + continue + m = re.match(r"\[\[?\s*([^\]]+?)\s*\]\]?", line) + if not m: + continue + # Top-level section name (first dotted component), deduped so repeated + # sub-tables of one profile don't inflate the count. + top = m.group(1).split(".", 1)[0].strip().strip("'\"") + if top and top not in seen: + seen.add(top) + count += 1 + return count + + +def unscoped_credential_warning(data: dict, profile_count: int | None = None) -> str | None: + """Advisory when a bare credential is shared across multiple TOML profiles (#11). + + ssh-mcp offers an unprefixed ``SSH_MCP_PASSWORD`` to every profile, so a + single stored password is silently handed to every host defined in a + multi-profile sidecar. Fires only when a bare credential is set AND the + sidecar defines 2+ profiles. ``profile_count`` is injected by the aggregator + (which reads the file); None means "unknown", so nothing is claimed. + """ + spec = resolve_server_spec(data) + if spec is None or profile_count is None or profile_count < 2: + return None + env = data.get("env") or {} + bare = [k for k in ("SSH_MCP_PASSWORD", "SSH_MCP_SUDO_PASSWORD") if k in env] + if not bare: + return None + names = ", ".join(sorted(bare)) + return ( + f"{spec.package}: {names} is unprefixed, so it is offered to every one of " + f"the {profile_count} profiles in the sidecar. Scope the credential " + f"per-profile so one host's password isn't shared with the others." + ) + + +def sidecar_warnings( + data: dict, + *, + platform: str | None = None, + environ: dict | None = None, + home: str | os.PathLike | None = None, + exists=None, + read_text=None, +) -> list[str]: + """Human-readable sidecar advisories for a server. Empty list == nothing to flag. + + Mirrors ``removed_flag_warnings``' shape so the GUI wiring is identical. Read-only: + reports precedence (args inert), the wrong-path trap, and the #11 unscoped-credential + trap. ``read_text`` is an injectable ``Path -> str`` for the sidecar body (used only + for the profile count); it defaults to a safe real read that degrades to "". + """ + status = sidecar_status(data, platform=platform, environ=environ, home=home, exists=exists) + if status is None: + return [] + out: list[str] = [] + if status["args_inert"]: + out.append( + f"{status['package']}: these arguments are currently inert — this server " + f"reads its config from {status['path']}, which already exists, and only " + f"falls back to CLI args when that file is absent. Edit the file instead." + ) + if status["wrong_path"]: + out.append( + f"{status['package']}: a config file exists at {status['doc_path']} (the " + f"path the README documents) but this server actually reads " + f"{status['path']} — the file you wrote is never loaded. Move it there." + ) + # #11: read the sidecar (if present) to count profiles for the credential check. + if status["exists"]: + if read_text is None: + + def read_text(p): + try: + return Path(p).read_text(encoding="utf-8", errors="replace") + except OSError: + return "" + + cred = unscoped_credential_warning( + data, profile_count=count_toml_profiles(read_text(status["path"])) + ) + if cred: + out.append(cred) + return out + + # --------------------------------------------------------------------------- # # Validation # --------------------------------------------------------------------------- # diff --git a/tests/test_core.py b/tests/test_core.py index 31d9b5f..3e0a9a9 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -3297,6 +3297,130 @@ def test_drift_warning_quiet_for_other_values_and_unknown_pkg(): assert c.drift_warnings({"command": "npx", "args": ["other", "--maxChars=none"]}) == [] +# --------------------------------------------------------------------------- # +# Sidecar config detection + precedence (issue #91) +# --------------------------------------------------------------------------- # +_SSH = {"command": "npx", "args": ["-y", "ssh-mcp", "--host=h", "--user=u"]} +_HOME = "/Users/tester" + + +def test_sidecar_path_verified_per_platform(): + spec = c.SERVER_SPECS["ssh-mcp"] + mac = c.sidecar_path(spec, platform="darwin", environ={}, home=_HOME) + assert str(mac) == "/Users/tester/Library/Application Support/ssh-mcp/config.toml" + # Windows resolves under %APPDATA%, not ~/.config. + win = c.sidecar_path( + spec, platform="win32", environ={"APPDATA": "C:/Users/t/AppData/Roaming"}, home=_HOME + ) + assert "ssh-mcp/config.toml" in str(win) + assert "AppData/Roaming" in str(win) + # POSIX honours XDG_CONFIG_HOME, else ~/.config. + xdg = c.sidecar_path(spec, platform="linux", environ={"XDG_CONFIG_HOME": "/cfg"}, home=_HOME) + assert str(xdg) == "/cfg/ssh-mcp/config.toml" + default = c.sidecar_path(spec, platform="linux", environ={}, home=_HOME) + assert str(default) == "/Users/tester/.config/ssh-mcp/config.toml" + + +def test_sidecar_path_none_for_non_sidecar_package(): + assert c.sidecar_path(None) is None + assert c.sidecar_path(c.ServerSpec(package="nope")) is None + + +def test_sidecar_args_inert_when_file_exists(): + real = c.sidecar_path(c.SERVER_SPECS["ssh-mcp"], platform="darwin", environ={}, home=_HOME) + st = c.sidecar_status( + _SSH, platform="darwin", environ={}, home=_HOME, exists=lambda p: p == real + ) + assert st["exists"] is True + assert st["has_managed_args"] is True + assert st["args_inert"] is True + assert st["wrong_path"] is False + warns = c.sidecar_warnings( + _SSH, platform="darwin", environ={}, home=_HOME, exists=lambda p: p == real + ) + assert warns and "inert" in warns[0] + assert str(real) in warns[0] + + +def test_sidecar_args_live_when_file_absent(): + st = c.sidecar_status(_SSH, platform="darwin", environ={}, home=_HOME, exists=lambda p: False) + assert st["exists"] is False + assert st["args_inert"] is False + assert ( + c.sidecar_warnings(_SSH, platform="darwin", environ={}, home=_HOME, exists=lambda p: False) + == [] + ) + + +def test_sidecar_wrong_path_flag_on_macos(): + # A TOML written at the README's ~/.config path is never read on macOS. + doc = c.sidecar_doc_path(c.SERVER_SPECS["ssh-mcp"], home=_HOME) + assert str(doc) == "/Users/tester/.config/ssh-mcp/config.toml" + st = c.sidecar_status( + _SSH, platform="darwin", environ={}, home=_HOME, exists=lambda p: p == doc + ) + assert st["wrong_path"] is True + assert st["args_inert"] is False # the real file doesn't exist, so args still apply + warns = c.sidecar_warnings( + _SSH, platform="darwin", environ={}, home=_HOME, exists=lambda p: p == doc + ) + assert warns and "never loaded" in warns[0] + + +def test_sidecar_no_wrong_path_on_linux_where_paths_coincide(): + # On Linux the real path and the doc path are the same, so a file there is + # correctly loaded — no wrong-path warning. + real = c.sidecar_path(c.SERVER_SPECS["ssh-mcp"], platform="linux", environ={}, home=_HOME) + st = c.sidecar_status( + _SSH, platform="linux", environ={}, home=_HOME, exists=lambda p: p == real + ) + assert st["wrong_path"] is False + assert st["args_inert"] is True + + +def test_count_toml_profiles(): + assert c.count_toml_profiles("") == 0 + assert c.count_toml_profiles("[server]\nhost='h'\n") == 1 + text = "# comment\n[[hosts]]\nname='a'\n[[hosts]]\nname='b'\n[settings]\nx=1\n" + # two [[hosts]] share a top-level name -> one profile group; [settings] -> another. + assert c.count_toml_profiles(text) == 2 + multi = "[prod]\nhost='p'\n[prod.auth]\nkey='k'\n[staging]\nhost='s'\n" + assert c.count_toml_profiles(multi) == 2 + + +def test_unscoped_credential_warning_11(): + data = dict(_SSH, env={"SSH_MCP_PASSWORD": "shared"}) + # Single profile: no sharing concern. + assert c.unscoped_credential_warning(data, profile_count=1) is None + # Unknown count: claim nothing. + assert c.unscoped_credential_warning(data, profile_count=None) is None + # Multiple profiles + bare credential: warn. + w = c.unscoped_credential_warning(data, profile_count=3) + assert w and "every one of the 3 profiles" in w + # No bare credential: nothing to warn. + assert c.unscoped_credential_warning(_SSH, profile_count=3) is None + + +def test_sidecar_warnings_includes_credential_scoping(): + real = c.sidecar_path(c.SERVER_SPECS["ssh-mcp"], platform="darwin", environ={}, home=_HOME) + data = dict(_SSH, env={"SSH_MCP_PASSWORD": "shared"}) + toml = "[prod]\nhost='p'\n[staging]\nhost='s'\n" + warns = c.sidecar_warnings( + data, + platform="darwin", + environ={}, + home=_HOME, + exists=lambda p: p == real, + read_text=lambda p: toml, + ) + assert any("profiles" in w for w in warns) + + +def test_sidecar_status_none_for_unknown_package(): + assert c.sidecar_status({"command": "npx", "args": ["other"]}) is None + assert c.sidecar_warnings({"command": "npx", "args": ["other"]}) == [] + + # --------------------------------------------------------------------------- # # Move to environment variable (issue #83) # --------------------------------------------------------------------------- #