feat: author ${VAR} references, gated on whether the client expands them (#76)
CI / Lint (ruff) (pull_request) Successful in 9s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 11s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 10s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 12s
CI / Catalog signature (pull_request) Successful in 7s
CI / Tests (py3.12 / windows-latest) (pull_request) Has been cancelled
CI / Lint (ruff) (pull_request) Successful in 9s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 11s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 10s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 12s
CI / Catalog signature (pull_request) Successful in 7s
CI / Tests (py3.12 / windows-latest) (pull_request) Has been cancelled
The blocker on this issue was whether BCC or the client does the expanding.
Answer, from Anthropic's docs: Claude Code expands ${VAR} and
${VAR:-default} itself, in command, args, env, url and headers, for both
project .mcp.json and user-scope ~/.claude.json. Claude Desktop has no
documented support.
So this is a per-client capability, not a global one, and BCC does NOT
expand on write: resolving a reference into the file would put the secret
back on disk -- the whole thing the user is avoiding -- and would defeat a
feature the client already implements correctly. BCC authors, validates and
warns; expand_env_refs exists to preview what the client will do.
Semantics mirror the documented ones exactly, including the unusual bit:
an unset variable with no default is left as literal ${VAR} text rather
than blanked, because that is what Claude Code passes through.
Gating uses the existing profile_targets_claude_desktop(), so a config that
is correct under Claude Code and broken under Desktop is reported against
whichever profile is actually loaded. The two warnings are worded
differently on purpose -- 'this client will never expand these' is a
different problem from 'this variable looks unset here'.
Two existing behaviours were backwards for this feature and are fixed:
- Secret masking hid placeholders. is_secret_key('API_KEY') is true, so
${API_KEY} rendered as dots -- making a reference indistinguishable from
a stored credential, which is the one distinction that makes the feature
worth adopting. should_mask_value() now skips references, in the table
delegate, _redact_server_data and redact_args alike.
- args_secret_warning fired on placeholders. Moving a token into ${VAR} is
the recommended fix for that warning; continuing to warn punished the
fix. It now skips references while still flagging a real secret that
follows one.
Real secrets are still masked everywhere they were before -- asserted, not
assumed.
Refs #76
This commit is contained in:
@@ -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) == []
|
||||
|
||||
Reference in New Issue
Block a user