fix(mcp): consent card never hides inline code and masks secrets with metacharacters

Three defects an adversarial review found in the previous masking round:

1. Interpreter latch: any later arg whose basename matched the (over-broad) interpreter pattern
   (ssh, publish, a --mode r value) made every following -c/-e/-p count as inline code, so the
   named secret after it was shown. Interpreter status is now argv[0] only (or the effective
   command resolved past wrappers), with a precise interpreter name list; a secret behind
   docker run --name ssh -e GITHUB_TOKEN=x, node server.js -e API_KEY=x, etc. masks again.
2. The "plain value only" rule showed real passwords containing shell metacharacters
   (--password=Pa$$w0rd!, -e DB_PASSWORD=x$y, ?token=a(b), Authorization: Bearer ab{c}).
   Named-secret values (flags, -e/--env, headers, URL userinfo/query) are masked whatever they
   hold; the plain-value check stays only on a bare KEY=VALUE, where it tells an interpreter's
   inline code from an env var.
3. Readback: a control/invisible arg rendered unquoted, so shlex read it wrong and a real newline
   collided with the literal text \n. Such args are now bash $'...' words (escapes doubled), so a
   real newline is distinct from \n and no hidden code point is shown raw.

Pre-existing, left per the review: a userinfo password holding a raw / # ? (an invalid URL) is
shown, because the authority splits on the first /.
This commit is contained in:
Teknium committed 2026-11-04 21:44:24 -08:00
1 parent 6858e9a386
commit b63eb8bf5a
2 files changed
+170 -60

No files matched your search

+61 -12
View File
@@ -131,7 +131,11 @@ def test_what_runs_is_never_masked(command, args):
script, URL path or non-secret-named flag value is masked."""
line = launch_target(None, command, args)["target"]
assert "***" not in line
assert shlex.split(line.replace("\\n", "\n")) == [command, *args]
assert "\n" not in line # a real newline in an arg is a visible escape, never a raw break
# Args with control characters are bash ``$'…'`` words (not shlex-parseable); the rest read back
# as exactly the argv that runs.
if all(ch.isprintable() or ch == " " for a in args for ch in a):
assert shlex.split(line) == [command, *args]
def test_a_positional_or_short_flag_secret_is_shown_as_the_user_approves_it():
@@ -139,6 +143,40 @@ def test_a_positional_or_short_flag_secret_is_shown_as_the_user_approves_it():
assert launch_target(None, "mysql", ["-u", "root", "-p" + S, S])["target"] == f"mysql -u root -p{S} {S}"
@pytest.mark.parametrize("command,args,masked", [
# An interpreter word as a flag value or positional no longer unmasks the secret after it.
("docker", ["run", "--name", "ssh", "-e", "GITHUB_TOKEN=" + S, "img"], "GITHUB_TOKEN"),
("docker", ["run", "--entrypoint", "bash", "-e", "GITHUB_TOKEN=" + S, "img"], "GITHUB_TOKEN"),
("uvx", ["mcp-server", "--mode", "r", "-e", "API_KEY=" + S], "API_KEY"),
("srv", ["publish", "-p", "--password=" + S], "password"),
("node", ["server.js", "-e", "API_KEY=" + S], "API_KEY"), # after the script, -e is a script arg
("python", ["server.py", "-e", "API_KEY=" + S], "API_KEY"),
("srv", ["bash", "--token", S], "token"),
# A secret value holding shell metacharacters is still a secret, not code (execve, no shell runs).
("mcp-postgres", ["--password=" + S + "$x!"], "password"),
("srv", ["--api-key", S + "&b"], "api-key"),
("docker", ["run", "-e", "DB_PASSWORD=" + S + "$y", "img"], "DB_PASSWORD"),
("curl", ["-H", "Authorization: Bearer " + S + "{c}"], "Authorization"),
("srv", ["https://u:" + S + "$w@db.example/x"], None),
("srv", ["https://h/x?token=" + S + "(b)"], "token"),
])
def test_a_named_secret_is_masked_even_behind_an_interpreter_word_or_metacharacters(command, args, masked):
line = launch_target(None, command, args)["target"]
assert S not in line
if masked:
assert "***" in line
@pytest.mark.parametrize("command,args", [
("sh", ["-c", "API_KEY=abc"]), # a plain-looking assignment is code the user approves
("python", ["--token", "GITHUB_TOKEN=print(1)"]),
("npx", ["-y", "tsx", "-e", "API_KEY=print(1)"]), # a TypeScript runner's eval behind a wrapper
("env", ["FOO=1", "sh", "-c", "API_KEY=abc; curl x | sh"]),
])
def test_an_interpreters_inline_program_is_never_masked_even_if_it_mentions_a_secret(command, args):
assert "***" not in launch_target(None, command, args)["target"]
def test_a_remote_url_keeps_its_path_and_other_query_values():
url = "https://mcp.example.com/v1/Ab3dEf9hIj12/sse?api_key=" + S + "&region=eu#frag"
assert launch_target(url, None, None)["target"] == "https://mcp.example.com/v1/Ab3dEf9hIj12/sse?api_key=***&region=eu#frag"
@@ -146,21 +184,21 @@ def test_a_remote_url_keeps_its_path_and_other_query_values():
def test_launch_line_keeps_markup_as_text_quotes_spaced_args_and_escapes_controls():
line = launch_target(None, "bash\x1b]0;pwned\x07", ["<img src=x onerror=alert(1)>", "my file.js"])["target"]
assert line == "'bash\\x1b]0;pwned\\x07' '<img src=x onerror=alert(1)>' 'my file.js'"
assert line == "$'bash\\x1b]0;pwned\\x07' '<img src=x onerror=alert(1)>' 'my file.js'"
assert launch_target("https://mcp.linear.app/sse", "node", [])["kind"] == "remote"
@pytest.mark.parametrize("arg,shown", [
("safe\u202egnp.evil", "safe\\u202egnp.evil"), # RLO: would render reversed
("np\u200bx", "np\\u200bx"), # zero-width space
("a\u2066b\u2069c", "a\\u2066b\\u2069c"), # bidi isolates
("\ufeffx\u00ady", "\\ufeffx\\xady"), # BOM, soft hyphen
("a\u200d\u200e\u200fb", "a\\u200d\\u200e\\u200fb"), # ZWJ, LRM, RLM
("line\nbreak", "'line\\nbreak'"), # whitespace inside one arg: quoted
("pk\u0336g", "pk\\u0336g"), # combining stroke: would draw a struck k
("a\u20ddb", "a\\u20ddb"), # enclosing mark (Me)
("pk\ufffcg", "pk\\ufffcg"), # object replacement character
("caf\u00e9", "caf\u00e9"), # a precomposed letter stays as written
("safe\u202egnp.evil", "$'safe\\u202egnp.evil'"), # RLO: would render reversed
("np\u200bx", "$'np\\u200bx'"), # zero-width space
("a\u2066b\u2069c", "$'a\\u2066b\\u2069c'"), # bidi isolates
("\ufeffx\u00ady", "$'\\ufeffx\\xady'"), # BOM, soft hyphen
("a\u200d\u200e\u200fb", "$'a\\u200d\\u200e\\u200fb'"), # ZWJ, LRM, RLM
("line\nbreak", "$'line\\nbreak'"), # a real newline inside one arg
("pk\u0336g", "$'pk\\u0336g'"), # combining stroke: would draw a struck k
("a\u20ddb", "$'a\\u20ddb'"), # enclosing mark (Me)
("pk\ufffcg", "$'pk\\ufffcg'"), # object replacement character
("caf\u00e9", "caf\u00e9"), # a precomposed letter stays as written
])
def test_launch_line_shows_invisible_and_reordering_characters_as_escapes(arg, shown):
line = launch_target(None, "node", [arg])["target"]
@@ -168,6 +206,17 @@ def test_launch_line_shows_invisible_and_reordering_characters_as_escapes(arg, s
assert all(unicodedata.category(c) != "Cf" for c in line)
@pytest.mark.parametrize("real,literal", [
("a\nb", "a\\nb"), ("x\ty", "x\\ty"), ("a b\u200b", "a b\\u200b"),
])
def test_a_real_control_character_never_reads_as_its_literal_escape(real, literal):
"""The card must show whether ``sh -c`` code holds a real newline (a separator) or the text ``\\n``."""
a = launch_target(None, "srv", [real])["target"]
b = launch_target(None, "srv", [literal])["target"]
assert a != b
assert "\n" not in a and "\t" not in a # the real control is a visible escape, not a raw character
@pytest.mark.parametrize("url,command,args,line", [
(None, "npx", ["-y", "@modelcontextprotocol/server-filesystem", "/home/me/projects", "./data"],
"npx -y @modelcontextprotocol/server-filesystem /home/me/projects ./data"),
+109 -48
View File
@@ -5,16 +5,19 @@ Install reads the catalog manifest's transport; enable and authorize read the co
The card exists so the user consents to what will run, so hiding execution is worse than showing a
secret. The line is never cut, and a value is masked only when its own name says it is secret: a long
flag such as ``--api-key=…`` / ``--token …`` / ``--token:…``, a ``GITHUB_TOKEN=…`` assignment, an
``Authorization:`` / ``Cookie:`` / API-key header arg, a URL userinfo password, a secret-named URL
query value (``?token=``, ``?key=``). Positional args, short flags (``-p``, ``-k``, ``-t``), packages,
images, scripts and URL paths are always shown, even when one of them is a key: the user approves
that exact command. Only a plain value is masked: one holding a line break or a shell metacharacter
(``;`` ``|`` ``&`` ``$`` backtick, brackets, quotes) is shown, and the code given to an interpreter's
inline-program flag (``sh -c``, ``python -c``, ``node -e``, ``perl -e``) is never masked, so a secret
name inside code cannot hide the code. An argument is quoted when it would not read back as exactly
that one argument (whitespace, a quote, backslash or shell metacharacter, an empty argument), so
``shlex.split`` of the line is the argv shown. Invisible and composing code points render as escapes.
flag such as ``--api-key=…`` / ``--token …`` / ``--token:…``, a ``GITHUB_TOKEN=…`` assignment, a
``docker``-style ``-e KEY=…`` / ``--env KEY=…``, an ``Authorization:`` / ``Cookie:`` / API-key header
arg, a URL userinfo password, a secret-named URL query value (``?token=``, ``?key=``). A named secret's
value is masked whatever it holds (shell metacharacters included) — execve argv never runs a shell.
Positional args, short flags (``-p``, ``-k``, ``-t``), packages, images, scripts and URL paths are
always shown, even when one of them is a key: the user approves that exact command. The program given
to an interpreter's inline-program flag (``sh -c``, ``python -c``, ``node -e``, ``perl -e``), found
past any wrapper (``env``, ``npx -y node``, ``sudo -u x bash``), is never masked, and a bare
``KEY=VALUE`` holding a line break or shell metacharacter is shown too, so a secret name inside code
cannot hide the code. An argument is quoted when it would not read back as exactly that one argument
(whitespace, a quote, backslash or shell metacharacter, an empty argument), so ``shlex.split`` of the
line is the argv shown; an argument with control or invisible code points is a bash ``$'…'`` word whose
escapes keep a real newline distinct from the literal text ``\n``.
"""
from __future__ import annotations
@@ -40,16 +43,27 @@ _NOT_A_VALUE_RE = re.compile(
_CAMEL_RE = re.compile(r"(?<=[a-z0-9])(?=[A-Z])")
# ``KEY=VALUE`` (an ``env`` prefix, a ``-e`` value): the key's last identifier carries the name.
_ASSIGN_RE = re.compile(r"^([^\s=]*?)([A-Za-z_][A-Za-z0-9_]*)=(.*)$", re.DOTALL)
# A value that can only be data. Anything that can run or split (a line break, ``;``, ``|``, ``&``,
# ``$(…)``, a backtick, brackets, quotes) means the arg may be code, so it is shown, not masked.
# A bare ``KEY=VALUE`` positional is ambiguous: a real env assignment, or code an interpreter runs
# (``sh -c 'TOKEN=x; curl …'``). Only when the value can only be data (no line break, ``;``, ``|``,
# ``&``, ``$(…)``, a backtick, brackets, quotes) is it masked; a value that can run or split is shown.
# The value of a recognised secret mechanism (a named flag, ``-e``/``--env``, a header, URL userinfo
# or query) is always masked instead, whatever it holds — that is never an interpreter's code.
_PLAIN_VALUE_RE = re.compile(r"[^\s;|&$`<>(){}'\"\\]+")
# A header value may hold spaces and ``;`` (``Cookie: a=1; b=2``) but never a line break or code.
_PLAIN_HEADER_VALUE_RE = re.compile(r"[^\x00-\x1f\x7f|&$`<>(){}'\"\\]+")
# An interpreter word (command or arg, ``/usr/bin/python3.12``, ``bash.exe``) and the flags whose next
# arg (or attached remainder) is the program it runs.
# A header value is masked whole; only a line break or control (header injection) keeps it visible.
_PLAIN_HEADER_VALUE_RE = re.compile(r"[^\x00-\x1f\x7f]+")
# A precise interpreter word (command or arg, ``/usr/bin/python3.12``, ``bash.exe``). Names are
# listed in full, never ``[a-z]*sh`` (that caught ``ssh`` and ``publish`` and hid the secret after).
_INTERPRETER_RE = re.compile(
r"(?:[a-z]*sh|python[\d.]*|pypy[\d.]*|node(?:js)?|deno|bun|ruby|perl[\d.]*|php[\d.]*|lua(?:jit)?|"
r"pwsh|powershell|osascript|tclsh|r|rscript)(?:\.exe)?", re.IGNORECASE)
r"(?:sh|bash|zsh|dash|ksh|mksh|ash|fish|csh|tcsh|rc|python[\d.]*|pypy[\d.]*|node(?:js)?|deno|bun|"
r"tsx|ts-node|ruby|perl[\d.]*|php[\d.]*|lua(?:jit)?|pwsh|powershell|osascript|tclsh|r|rscript|elixir)(?:\.exe)?",
re.IGNORECASE)
# Commands that run a following command; the real command (and whether it is an interpreter) is found
# past the wrapper's options, so ``-e`` after ``npx -y node`` is node's eval, not a docker env var.
_WRAPPERS = frozenset({
"env", "sudo", "doas", "nohup", "setsid", "nice", "ionice", "time", "timeout", "stdbuf",
"command", "exec", "busybox", "chroot", "xargs", "uv", "uvx", "npx", "pnpm", "bunx", "pipx",
"poetry", "yarn",
})
_INLINE_CODE_FLAG_RE = re.compile(r"(-[ceEpr]|--eval|--print|--?command)(=?)(.*)", re.IGNORECASE | re.DOTALL)
_SECRET_ENV_RE = re.compile(r"TOKEN|SECRET|PASSW(?:OR)?D|API_?KEY|AUTH(?!OR(?!IZ))|CREDENTIAL|PRIVATE_?KEY",
re.IGNORECASE)
@@ -69,28 +83,33 @@ def _is_secret_name(name: str) -> bool:
return bool(_SECRET_NAME_RE.search(norm)) and not _NOT_A_VALUE_RE.search(norm)
def _mask_arg(arg: str) -> tuple[str, bool]:
"""``(shown, mask_next)`` for one argv entry."""
def _mask_arg(arg: str, interpreter: bool = False) -> tuple[str, bool]:
"""``(shown, mask_next)`` for one argv entry. ``interpreter`` is True while the command word (argv[0])
is an interpreter and its own options are still being read: a named-secret flag then shows its value
(it may be the script or its code) instead of masking it, but an attached ``--flag=`` secret is still
masked, since that value is never the program."""
if (header := _SECRET_HEADER_ARG_RE.match(arg)) and _PLAIN_HEADER_VALUE_RE.fullmatch(header.group(2)):
return f"{header.group(1)}***", False
if arg.startswith("--"):
flag = _LONG_FLAG_RE.match(arg)
if flag and _is_secret_name(flag.group(1)):
if not flag.group(2):
return arg, True
return (f"{flag.group(0)}***" if _PLAIN_VALUE_RE.fullmatch(arg[flag.end():]) else arg), False
return arg, not interpreter
return f"{flag.group(0)}***", False # ``--api-key=VALUE``: the value is never the program
# ``--env=GITHUB_TOKEN=…``: the assignment's own key names the secret.
if flag and flag.group(2):
return f"{flag.group(0)}{_mask_assignment(arg[flag.end():])}", False
return f"{flag.group(0)}{_mask_assignment(arg[flag.end():], force=True)}", False
return arg, False
# ``KEY=VALUE`` alone, or attached to a short flag (``-eGITHUB_TOKEN=…``, java ``-Dapi.token=…``).
return _mask_assignment(arg), False
def _mask_assignment(text: str) -> str:
"""A ``KEY=VALUE`` whose key names a secret and whose value is plain data, with the value masked."""
def _mask_assignment(text: str, force: bool = False) -> str:
"""A ``KEY=VALUE`` whose key names a secret, with the value masked. ``force`` masks it whatever the
value holds (the value of ``-e``/``--env``/``--flag=``); otherwise only a plain data value is masked,
so a bare ``KEY=VALUE`` that is really an interpreter's inline code is shown."""
assign = _ASSIGN_RE.match(text)
if assign and _SECRET_ENV_RE.search(assign.group(2)) and _PLAIN_VALUE_RE.fullmatch(assign.group(3)):
if assign and _SECRET_ENV_RE.search(assign.group(2)) and (force or _PLAIN_VALUE_RE.fullmatch(assign.group(3))):
return f"{assign.group(1)}{assign.group(2)}=***"
return text
@@ -99,30 +118,51 @@ def _is_interpreter(word: str) -> bool:
return bool(_INTERPRETER_RE.fullmatch(re.split(r"[\\/]", word)[-1]))
def _effective_is_interpreter(command: str, args: list[str]) -> bool:
"""Whether the command that actually runs is an interpreter, looking past wrapper words. ``env …
sh``, ``npx -y node`` and ``sudo -u x bash`` all resolve to an interpreter, so a later ``-e`` is its
eval flag, not a ``docker``-style env var worth masking."""
if _is_interpreter(command):
return True
if re.split(r"[\\/]", command)[-1] not in _WRAPPERS:
return False
return any(_is_interpreter(a) for a in args if not a.startswith("-") and "=" not in a)
def _mask_flag_values(command: str, args: list[str]) -> list[str]:
out: list[str] = []
mask_next = code_next = False
interpreter = _is_interpreter(command)
interp = _is_interpreter(command) # argv[0] is an interpreter; its own flags may carry inline code
allow_env_force = not _effective_is_interpreter(command, args)
code_next = mask_next = env_next = False
for arg in args:
# The program an interpreter runs inline (``sh -c 'GITHUB_TOKEN=x; curl …'``) is what the user
# approves: shown whole, whatever names it mentions.
if code_next:
out.append(arg)
mask_next = code_next = False
code_next = interp = False
continue
if interpreter and (inline := _INLINE_CODE_FLAG_RE.fullmatch(arg)):
if mask_next:
mask_next = False
if not arg.startswith("-"): # a following flag is the next flag, not the secret's value
out.append("***")
continue
if env_next:
out.append(_mask_assignment(arg, force=True))
env_next = False
continue
if interp and (inline := _INLINE_CODE_FLAG_RE.fullmatch(arg)):
out.append(arg)
code_next = not (inline.group(2) or inline.group(3))
mask_next = False
interp = code_next # attached code (``-ccode``/``--eval=code``) ends the option region now
continue
# Any flag right after a secret flag is the next flag, not its value (``--token -c x`` does
# not parse as a token): masking it would hide what runs.
if mask_next and not arg.startswith("-"):
out.append("***" if _PLAIN_VALUE_RE.fullmatch(arg) else arg)
mask_next = False
if interp and not arg.startswith("-"):
interp = False # the script positional: its own args mask normally from here
# ``docker``-style ``-e KEY=VALUE`` / ``--env KEY=VALUE`` (only when no interpreter runs).
if allow_env_force and arg in ("-e", "--env"):
out.append(arg)
env_next = True
continue
shown, mask_next = _mask_arg(arg)
interpreter = interpreter or _is_interpreter(arg)
shown, mask_next = _mask_arg(arg, interpreter=interp)
out.append(shown)
return out
@@ -136,11 +176,6 @@ def _mask_query_pair(pair: str) -> str:
return f"{key}=***" if eq and (key.lower() == "key" or _is_secret_name(key)) else pair
def _mask_query_value(pair: str) -> str:
masked = _mask_query_pair(pair)
return masked if masked == pair or _PLAIN_VALUE_RE.fullmatch(pair.partition("=")[2]) else pair
def _mask_url(match: re.Match[str]) -> str:
"""A userinfo password and secret-named query values become ``***``. Host, path, other query
values and the fragment stay: the path of a ``deno run https://…`` URL is the code that runs."""
@@ -150,11 +185,11 @@ def _mask_url(match: re.Match[str]) -> str:
rest, qmark, query = rest.partition("?")
authority, slash, path = rest.partition("/")
userinfo, at, host = authority.rpartition("@")
if at and ":" in userinfo and _PLAIN_VALUE_RE.fullmatch(userinfo.partition(":")[2]):
if at and ":" in userinfo: # a userinfo password is always secret, whatever it holds
authority = f"{userinfo.partition(':')[0]}:***@{host}"
out = f"{head}{sep}{authority}{slash}{path}"
if qmark:
out += "?" + "&".join(_mask_query_value(pair) for pair in query.split("&"))
out += "?" + "&".join(_mask_query_pair(pair) for pair in query.split("&"))
return out + hash_ + fragment
@@ -188,14 +223,40 @@ def scrub(text: str) -> str:
# Characters that change how a shell or ``shlex.split`` reads a word: such a word, or an empty one,
# is quoted, so ``python -c 'X=print(1)'`` reads as the one argument it is.
_NEEDS_QUOTE_RE = re.compile(r"[\s'\"\\;|&$`<>(){}]")
# Common controls get a readable name; every other hidden code point gets its ``\\x``/``\\u`` escape.
_CONTROL_NAMES = {"\n": "\\n", "\t": "\\t", "\r": "\\r"}
def _is_hidden(ch: str) -> bool:
"""A control, format (bidi override/isolate, zero-width, BOM, soft hyphen), non-space separator or
combining code point: shown as an escape so the line reads as the argv runs, never a hidden char."""
return not ch.isprintable() or unicodedata.category(ch) in ("Mn", "Me", "Mc") or ch == "\ufffc"
def _escape_cp(ch: str) -> str:
if ch in _CONTROL_NAMES:
return _CONTROL_NAMES[ch]
o = ord(ch)
return f"\\x{o:02x}" if o <= 0xff else (f"\\u{o:04x}" if o <= 0xffff else f"\\U{o:08x}")
def _ansi_c_quote(text: str) -> str:
"""An arg holding hidden code points as a bash ``$'…'`` word: backslashes are doubled and each hidden
code point is a visible escape, so a real newline (``$'a\\nb'``) never reads as the literal text
``a\\n b`` (``'a\\nb'``) — the card must show which one the shell runs."""
body = "".join("\\\\" if c == "\\" else (_escape_cp(c) if _is_hidden(c) else c) for c in text)
return "$'" + body.replace("'", "\\'") + "'"
def _word(arg: str) -> str:
"""One argv entry as one shell word: ``''`` for an empty arg, ``'review server.py'`` with both
spaces, so the line never reads as a different argv. Escapes are applied after quoting, so an
invisible character still shows as ``\\u…`` and a literal backslash arg is the quoted one."""
spaces, so the line never reads as a different argv. An arg with control or invisible code points is a
``$'…'`` word (bash reads it back); a plain arg is ``shlex.quote``-d, so ``shlex.split`` of the line
is the argv shown."""
shown = _URL_RE.sub(_mask_url, arg)
return _visible(shlex.quote(shown) if not shown or _NEEDS_QUOTE_RE.search(shown) else shown)
if any(_is_hidden(c) for c in shown):
return _ansi_c_quote(shown)
return shlex.quote(shown) if not shown or _NEEDS_QUOTE_RE.search(shown) else shown
def _env_keys(env: Any) -> list[str]: