fix(mcp): stop handing stdio servers every secret in executor's environment - #1595
Merged
RhysSullivan merged 5 commits intoAug 28, 2026
Merged
Conversation
…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.
This was referenced Aug 15, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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,USERon POSIX — and it even skips function-shaped values, calling them a security risk in its own comment. The SDK then spawns with: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
npxMCP server goes from "this server can see the API key I gave it" to "this server holds the key that decrypts every other credential, plusEXECUTOR_AUTH_TOKENandDATABASE_URL".Worth noting where the leak sat. With no env configured we passed
undefinedand the SDK's safe-list applied normally. The leak was on theconfig.envbranch — 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.
packages/plugins/mcpsuite: 130 passed, 29 skipped, 16 files.createStdioTransportagainst a realnodechild that dumpsprocess.envto a file.PATHandHOMEstill do, so the fix cannot strand a server that needs to find its own interpreter.{ ...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.oxlinton both changed files: 0 warnings, 0 errors.