diff --git a/tests/tools/test_connectors_mcp_launch.py b/tests/tools/test_connectors_mcp_launch.py index b88db6f1cd..b1cd52b819 100644 --- a/tests/tools/test_connectors_mcp_launch.py +++ b/tests/tools/test_connectors_mcp_launch.py @@ -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 + "®ion=eu#frag" assert launch_target(url, None, None)["target"] == "https://mcp.example.com/v1/Ab3dEf9hIj12/sse?api_key=***®ion=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", ["", "my file.js"])["target"] - assert line == "'bash\\x1b]0;pwned\\x07' '' 'my file.js'" + assert line == "$'bash\\x1b]0;pwned\\x07' '' '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"), diff --git a/tools/connectors/mcp_launch.py b/tools/connectors/mcp_launch.py index 953fcd3589..4af4b6d640 100644 --- a/tools/connectors/mcp_launch.py +++ b/tools/connectors/mcp_launch.py @@ -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]: