Skip to content

fix(mcp): stop handing stdio servers every secret in executor's environment - #1595

Merged
RhysSullivan merged 5 commits into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-stdio-env-inheritance
Aug 28, 2026
Merged

fix(mcp): stop handing stdio servers every secret in executor's environment#1595
RhysSullivan merged 5 commits into
UsefulSoftwareCo:mainfrom
GeiserX:fix/mcp-stdio-env-inheritance

Conversation

@GeiserX

@GeiserX GeiserX commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

A stdio MCP server is handed every environment variable Executor holds. Including EXECUTOR_SECRET_KEY, the key that decrypts the secret store.

The MCP SDK already guards against this. getDefaultEnvironment() is a sudo-style safe-list — HOME, LOGNAME, PATH, SHELL, TERM, USER on POSIX — and it even skips function-shaped values, calling them a security risk in its own comment. The SDK then spawns with:

env: { ...getDefaultEnvironment(), ...serverParams.env }

So whatever we pass is merged on top of the safe-list. Passing { ...process.env, ...config.env } therefore didn't add to it — it overwrote it with the whole environment.

What that means in practice: adding one third-party npx MCP server goes from "this server can see the API key I gave it" to "this server holds the key that decrypts every other credential, plus EXECUTOR_AUTH_TOKEN and DATABASE_URL".

Worth noting where the leak sat. With no env configured we passed undefined and the SDK's safe-list applied normally. The leak was on the config.env branch — the branch a credential-bearing integration takes. The safe path was the one nobody was using.

The fix

Pass only what the integration declared, and let the SDK apply its own base. A server that genuinely needs a variable declares it in the integration's env, which is the mechanism that already exists for exactly that.

How I tested it

I spawned a real subprocess and read back the environment it actually received. Asserting on the arguments we hand the SDK would not have answered the question, since the SDK merges its own list underneath ours — the only honest answer comes from the child.

  • Full packages/plugins/mcp suite: 130 passed, 29 skipped, 16 files.
  • Four new tests drive createStdioTransport against a real node child that dumps process.env to a file.
  • One asserts a host-only secret does not arrive while the declared variable does.
  • One asserts PATH and HOME still do, so the fix cannot strand a server that needs to find its own interpreter.
  • One is a positive control that passes the secret in deliberately and asserts the child reports it — without that, a child which failed to write anything would satisfy every other assertion.
  • Mutation check: I put the old { ...process.env, ... } expression back and re-ran. Exactly one test failed — the declared-env one. That is the same branch the analysis pointed at, measured rather than argued.
  • oxlint on both changed files: 0 warnings, 0 errors.

…onment

The MCP SDK ships a sudo-style safe-list for exactly this — HOME, LOGNAME,
PATH, SHELL, TERM, USER — and merges whatever env you pass on top of it.
Spreading process.env into that did not extend the safe-list, it defeated it.

So any stdio MCP server received every variable this process holds:
EXECUTOR_SECRET_KEY, which decrypts the whole secret store, plus
EXECUTOR_AUTH_TOKEN, DATABASE_URL and anything else the operator exported.
Adding one third-party server went from "it sees its own API key" to "it holds
the key to everyone else's".

The leak was on the declared-env branch only. With no env configured the SDK's
safe-list already applied, so the branch a credential-bearing integration takes
was the unsafe one.

Pass only what the integration declared and let the SDK apply its own base.
The env fix drops process.env from the spawn, which also drops the
host's proxy and TLS trust configuration. No source config declares
those and a server behind a corporate proxy or an intercepting CA
cannot reach anything without them, so pass a fixed allowlist beneath
the declared env: HTTP_PROXY, HTTPS_PROXY, NO_PROXY (both spellings),
NODE_EXTRA_CA_CERTS, SSL_CERT_FILE, SSL_CERT_DIR. Same list and same
reasoning as the pass-through in apps/cli/src/service.ts. The declared
env still wins on a key collision.

Tests cover the allowlist in both directions and the child now reports
only the keys under test instead of dumping the whole environment to a
temp file. Adds the changeset.
Windows environment keys are case-insensitive, but a JavaScript spread is
not. An inherited HTTP_PROXY beside a declared http_proxy, or the SDK's
PATH beside a declared Path, both reached the child, which then read
whichever spelling Windows resolved first rather than the declared one.

Merge by case-insensitive key identity on win32: the declared env still
wins and the losing spelling is dropped. A key the SDK's own safe-list
also sets is emitted with the SDK's spelling, so it replaces that entry
instead of aliasing it.

Also correct the README, which still described stdio children as
inheriting process.env.
@RhysSullivan
RhysSullivan merged commit 8324e1e into UsefulSoftwareCo:main Aug 28, 2026
40 checks passed
This was referenced Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants