diff --git a/bcc.py b/bcc.py index a7770e0..e496e02 100644 --- a/bcc.py +++ b/bcc.py @@ -268,7 +268,7 @@ class _SecretMaskDelegate(QStyledItemDelegate): if self.revealed or not option.text: return key_item = self._table.item(index.row(), 0) - if key_item and core.is_secret_key(key_item.text()): + if key_item and core.should_mask_value(key_item.text(), option.text): option.text = core.MASK @@ -2622,6 +2622,12 @@ class MainWindow(QMainWindow): self.save_btn.setEnabled(False) return False lint_warnings = core.lint_servers(self.servers) + # ${VAR} references are only meaningful if the target client expands + # them -- Claude Desktop doesn't, so the same config is fine in one + # profile and broken in another (#76). Report against the loaded one. + for entry in self.servers: + for warning in core.env_ref_warnings(entry.data, self.current_profile): + 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 63b0dc8..ff104f9 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -893,13 +893,28 @@ def backup_label(backup_path: Path | str) -> str: return f"{ts[:4]}-{ts[4:6]}-{ts[6:8]} {ts[9:11]}:{ts[11:13]}:{ts[13:]}" +def should_mask_value(key: str, value) -> bool: + """Whether an env/header value should be masked for display. + + A ${VAR} reference is NOT a secret -- it's a pointer to one, and it's the + thing we want users to adopt. Masking it to dots would make a reference + indistinguishable from a stored credential, hiding exactly the distinction + that makes the feature worth using (#76). + """ + if not is_secret_key(key): + return False + return not is_env_ref(value) if isinstance(value, str) else True + + def _redact_server_data(data: dict) -> dict: """Return a copy of a server definition with secrets masked for display.""" out = dict(data) if "args" in out: out["args"] = redact_args(list(out["args"] or [])) if "env" in out: - out["env"] = {k: (MASK if is_secret_key(k) else v) for k, v in (out["env"] or {}).items()} + out["env"] = { + k: (MASK if should_mask_value(k, v) else v) for k, v in (out["env"] or {}).items() + } return out @@ -1450,6 +1465,155 @@ MASK = "••••••••" _EMBEDDED_CRED_RE = re.compile(r"://[^:@/\s]+:[^:@/\s]+@") +# --------------------------------------------------------------------------- # +# Environment-variable references (issue #76) +# +# Claude Code expands ${VAR} and ${VAR:-default} itself, in command, args, env, +# url and headers. So BCC does NOT expand these on write -- resolving them into +# the file would put the secret back on disk, which is the whole thing the user +# is avoiding, and would defeat a feature the client already implements. BCC +# authors, validates and warns. +# +# Claude Desktop has no documented support, so the same text there is passed to +# the server literally. That makes this a per-client capability, not a global +# one -- see client_expands_env_refs(). +# --------------------------------------------------------------------------- # +# ${NAME} or ${NAME:-default}. Names follow the shell convention (letter or +# underscore first) so a bare "${}" or "${1}" isn't mistaken for a reference. +_ENV_REF_RE = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}") + +# The five fields Claude Code documents as expansion sites. +ENV_REF_FIELDS = ("command", "args", "env", "url", "headers") + + +class EnvRef(NamedTuple): + """One ${VAR} / ${VAR:-default} occurrence found in a server definition.""" + + name: str + default: str | None + field: str # which of ENV_REF_FIELDS it was found in + + @property + def has_default(self) -> bool: + return self.default is not None + + +def find_env_refs(text: str, field: str = "") -> list[EnvRef]: + """Every ${VAR} / ${VAR:-default} reference in a single string.""" + if not isinstance(text, str): + return [] + return [EnvRef(m.group(1), m.group(2), field) for m in _ENV_REF_RE.finditer(text)] + + +def is_env_ref(value: str) -> bool: + """True when the value contains at least one ${VAR} reference. + + Used to keep placeholders OUT of secret masking: `${API_KEY}` under a + secret-looking key is a reference, not a secret, and masking it to dots + would hide the one distinction the user needs to see. + """ + return bool(find_env_refs(value)) + + +def server_env_refs(data: dict) -> list[EnvRef]: + """Every env reference in a server definition, tagged with its field. + + Only inspects the fields Claude Code actually expands; a ${VAR} written + into some other key is not a reference and shouldn't be reported as one. + """ + out: list[EnvRef] = [] + if not isinstance(data, dict): + return out + for field in ENV_REF_FIELDS: + value = data.get(field) + if isinstance(value, str): + out.extend(find_env_refs(value, field)) + elif isinstance(value, list): + for item in value: + out.extend(find_env_refs(item, field)) + elif isinstance(value, dict): + for v in value.values(): + out.extend(find_env_refs(v, field)) + return out + + +def expand_env_refs(text: str, environ: dict | None = None) -> str: + """Expand ${VAR} / ${VAR:-default} the way Claude Code documents it. + + Provided for previewing what the client will do -- BCC never writes the + expanded form back to the config. Unset with no default is left as the + literal ${VAR} text, matching Claude Code: the config still loads and the + unexpanded text is passed through. + """ + if not isinstance(text, str): + return text + env = os.environ if environ is None else environ + + def repl(m: re.Match) -> str: + name, default = m.group(1), m.group(2) + if name in env: + return env[name] + return default if default is not None else m.group(0) + + return _ENV_REF_RE.sub(repl, text) + + +def unresolved_env_refs(data: dict, environ: dict | None = None) -> list[EnvRef]: + """References that would not resolve: variable unset AND no default. + + Best-effort by nature -- BCC's environment isn't necessarily the client's, + so this warns rather than blocks, and the warning text says so. + """ + env = os.environ if environ is None else environ + return [r for r in server_env_refs(data) if not r.has_default and r.name not in env] + + +def client_expands_env_refs(profile: Profile) -> bool: + """Whether the client behind `profile` expands ${VAR} itself. + + Claude Code does, in command/args/env/url/headers, for both project + `.mcp.json` and user-scope `~/.claude.json`. Claude Desktop has no + documented support, so a reference there reaches the server as literal + text -- which surfaces as a confusing auth failure rather than an obvious + config error, hence the warning. + """ + return not profile_targets_claude_desktop(profile) + + +def env_ref_warnings( + data: dict, profile: Profile | None = None, environ: dict | None = None +) -> list[str]: + """Advisory warnings about env references in one server definition. + + Two distinct problems, deliberately worded differently: + - the target client won't expand them at all (Claude Desktop) + - the client will expand them, but a variable looks unset here + """ + refs = server_env_refs(data) + if not refs: + return [] + + if profile is not None and not client_expands_env_refs(profile): + names = ", ".join(sorted({f"${{{r.name}}}" for r in refs})) + return [ + f"{names} will NOT be expanded by Claude Desktop -- it has no " + f"documented support for variable references, so the server " + f"receives the literal text. Use a real value here, or move this " + f"server to a Claude Code config." + ] + + missing = unresolved_env_refs(data, environ) + if not missing: + return [] + names = ", ".join(sorted({r.name for r in missing})) + return [ + f"{names} is not set in this environment and has no ':-default'. " + f"Claude Code will pass the reference through unexpanded. " + f"(Checked against BCC's environment, which may differ from the " + f"client's.)" + ] + + def is_secret_key(name: str) -> bool: """Does this env-var / header / flag name look like it holds a secret?""" return bool(_SECRET_KEY_RE.search(name or "")) @@ -1466,17 +1630,22 @@ def redact_args(args: list[str]) -> list[str]: --api-key=abc123 -> --api-key=•••••••• (inline flag=value) ghp_abc123 -> •••••••• (well-known token prefix) Everything else passes through untouched. + + ${VAR} references are left visible: they name a secret rather than being + one, and hiding them would obscure the difference between "this config + leaks a token" and "this config points at one" (#76). """ out: list[str] = [] mask_next = False for a in args: s = str(a) if mask_next: - out.append(MASK) mask_next = False + out.append(s if is_env_ref(s) else MASK) continue if s.startswith("-") and "=" in s and is_secret_key(s.split("=", 1)[0]): - out.append(s.split("=", 1)[0] + "=" + MASK) + flag, value = s.split("=", 1) + out.append(f"{flag}={value}" if is_env_ref(value) else f"{flag}={MASK}") continue if s.startswith("-") and is_secret_key(s): out.append(s) @@ -1503,6 +1672,11 @@ def args_secret_warning(data: dict) -> str | None: args = [str(a) for a in (data.get("args") or [])] mask_next = False for a in args: + # A ${VAR} reference is the recommended fix for this very warning -- + # continuing to warn after the user adopts it punishes the fix (#76). + if is_env_ref(a): + mask_next = False + continue if mask_next: mask_next = False if not a.startswith("-"): diff --git a/tests/test_core.py b/tests/test_core.py index 4ccea2b..fc0b047 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2680,3 +2680,138 @@ def test_update_notice_renders_both_versions_the_same_way(): assert "1.3.0" in n["text"] and "1.2.0" in n["text"] # the machine-readable field keeps the real tag assert n["version"] == "v1.3.0" + + +# --------------------------------------------------------------------------- # +# #76 -- ${VAR} references. Semantics mirror Claude Code's documented +# behaviour: ${VAR} and ${VAR:-default}, expanded in command/args/env/url/ +# headers, and an unset variable with no default left as literal text. +# --------------------------------------------------------------------------- # +def test_find_env_refs_plain_and_defaulted(): + refs = c.find_env_refs("${A} and ${B:-fallback}") + assert [(r.name, r.default) for r in refs] == [("A", None), ("B", "fallback")] + + +@pytest.mark.parametrize("text", ["${}", "${1BAD}", "$NOTBRACED", "{NOPE}", "plain", "$${X"]) +def test_find_env_refs_ignores_non_references(text): + assert c.find_env_refs(text) == [] + + +def test_find_env_refs_allows_an_empty_default(): + """`${VAR:-}` is a documented way to say 'blank if unset'.""" + refs = c.find_env_refs("${A:-}") + assert refs[0].default == "" + assert refs[0].has_default is True + + +def test_server_env_refs_covers_all_five_documented_fields(): + data = { + "command": "${BIN}", + "args": ["--x", "${ARG}"], + "env": {"K": "${ENVV}"}, + "url": "${URL}/mcp", + "headers": {"Authorization": "Bearer ${HDR}"}, + } + found = {(r.name, r.field) for r in c.server_env_refs(data)} + assert found == { + ("BIN", "command"), + ("ARG", "args"), + ("ENVV", "env"), + ("URL", "url"), + ("HDR", "headers"), + } + + +def test_server_env_refs_ignores_unexpanded_fields(): + """Claude Code expands five fields; a ${VAR} elsewhere isn't a reference.""" + assert c.server_env_refs({"description": "${NOPE}", "timeout": "${ALSO_NO}"}) == [] + + +def test_expand_env_refs_matches_documented_semantics(): + env = {"SET": "value"} + assert c.expand_env_refs("${SET}", env) == "value" + assert c.expand_env_refs("${MISSING:-dflt}", env) == "dflt" + assert c.expand_env_refs("${SET:-dflt}", env) == "value" + # unset with no default: left as literal text, exactly as Claude Code does + assert c.expand_env_refs("${MISSING}", env) == "${MISSING}" + + +def test_expand_env_refs_handles_several_in_one_string(): + assert c.expand_env_refs("${A}/${B:-two}/${C}", {"A": "one"}) == "one/two/${C}" + + +def test_unresolved_env_refs_only_flags_unset_without_default(): + data = {"env": {"A": "${SET}", "B": "${UNSET}", "C": "${OTHER:-has_default}"}} + assert [r.name for r in c.unresolved_env_refs(data, {"SET": "x"})] == ["UNSET"] + + +# --- the two interactions that were backwards for this feature ------------ +def test_placeholder_under_a_secret_key_is_not_masked(): + """A ${VAR} names a secret rather than being one. Masking it would make a + reference indistinguishable from a stored credential.""" + assert c.should_mask_value("API_KEY", "${API_KEY}") is False + assert c.should_mask_value("API_KEY", "ghp_realsecret") is True + assert c.should_mask_value("NOT_SECRET", "${API_KEY}") is False + + +def test_redacted_display_keeps_placeholders_but_masks_real_secrets(): + out = c._redact_server_data({"env": {"API_KEY": "${API_KEY}", "TOKEN": "ghp_real"}}) + assert out["env"]["API_KEY"] == "${API_KEY}" + assert out["env"]["TOKEN"] == c.MASK + + +def test_redact_args_keeps_placeholders_visible(): + assert c.redact_args(["--token", "${GH_TOKEN}"]) == ["--token", "${GH_TOKEN}"] + assert c.redact_args(["--api-key=${K}"]) == ["--api-key=${K}"] + # real secrets still masked + assert c.redact_args(["--token", "ghp_real"]) == ["--token", c.MASK] + assert c.redact_args(["--api-key=sk-real"]) == [f"--api-key={c.MASK}"] + + +def test_args_secret_warning_is_silenced_by_a_placeholder(): + """Moving a token into ${VAR} is the recommended fix for this warning -- + still warning afterwards would punish the fix.""" + assert c.args_secret_warning({"args": ["--token", "ghp_real"]}) is not None + assert c.args_secret_warning({"args": ["--token", "${GH_TOKEN}"]}) is None + + +def test_args_secret_warning_still_fires_on_the_arg_after_a_placeholder(): + """A placeholder must clear the pending-flag state, not blanket-suppress.""" + assert c.args_secret_warning({"args": ["${SAFE}", "--token", "ghp_real"]}) is not None + + +# --- per-client gating ---------------------------------------------------- +def _profile(path): + return c.Profile(label="p", path=Path(path), config_exists=True) + + +def test_claude_code_profiles_expand_references(): + assert c.client_expands_env_refs(_profile(Path.home() / ".claude.json")) is True + assert c.client_expands_env_refs(_profile("/repo/.mcp.json")) is True + + +def test_claude_desktop_profile_does_not_expand_references(): + desktop = _profile(c.app_support_base() / "Claude" / c.CONFIG_FILENAME) + assert c.client_expands_env_refs(desktop) is False + + +def test_desktop_profile_warns_that_references_are_literal(): + data = {"env": {"API_KEY": "${API_KEY}"}} + desktop = _profile(c.app_support_base() / "Claude" / c.CONFIG_FILENAME) + warnings = c.env_ref_warnings(data, desktop, {"API_KEY": "set"}) + assert len(warnings) == 1 + assert "NOT be expanded" in warnings[0] + assert "${API_KEY}" in warnings[0] + + +def test_claude_code_profile_warns_only_about_unset_variables(): + code = _profile(Path.home() / ".claude.json") + data = {"env": {"A": "${UNSET_ONE}"}} + assert c.env_ref_warnings(data, code, {}) != [] + assert c.env_ref_warnings(data, code, {"UNSET_ONE": "x"}) == [] + # a default means it always resolves + assert c.env_ref_warnings({"env": {"A": "${X:-d}"}}, code, {}) == [] + + +def test_no_references_means_no_warnings(): + assert c.env_ref_warnings({"command": "npx", "args": ["-y", "pkg"]}, None) == []