From 6c13bf7fa8247c0a193d336cc3a2a7db92b2c5dd Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Sat, 15 Aug 2026 16:16:56 +0200 Subject: [PATCH] fix(mcp): stop handing stdio servers every secret in executor's environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../plugins/mcp/src/sdk/stdio-connector.ts | 13 ++- .../mcp/src/sdk/stdio-env-isolation.test.ts | 101 ++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 packages/plugins/mcp/src/sdk/stdio-env-isolation.test.ts diff --git a/packages/plugins/mcp/src/sdk/stdio-connector.ts b/packages/plugins/mcp/src/sdk/stdio-connector.ts index 99a0f72e37..3a431e6ed6 100644 --- a/packages/plugins/mcp/src/sdk/stdio-connector.ts +++ b/packages/plugins/mcp/src/sdk/stdio-connector.ts @@ -26,6 +26,17 @@ export const createStdioTransport = (config: StdioTransportConfig) => new StdioClientTransport({ command: config.command, args: config.args ? [...config.args] : undefined, - env: config.env ? ({ ...process.env, ...config.env } as Record) : undefined, + // Pass only what the integration declared. The SDK already merges this + // over `getDefaultEnvironment()`, a sudo-style safe-list (HOME, LOGNAME, + // PATH, SHELL, TERM, USER) that deliberately excludes everything else and + // skips function-shaped values as a security risk. + // + // Spreading `process.env` here did not add to that safe-list, it defeated + // it: the child received every variable this process holds, which for a + // server that spawns one includes `EXECUTOR_SECRET_KEY` (the key that + // decrypts the secret store), `EXECUTOR_AUTH_TOKEN`, `DATABASE_URL` and + // whatever else the operator exported. A stdio server needing one of + // those declares it in the integration's `env` like any other value. + env: config.env, cwd: config.cwd, }); diff --git a/packages/plugins/mcp/src/sdk/stdio-env-isolation.test.ts b/packages/plugins/mcp/src/sdk/stdio-env-isolation.test.ts new file mode 100644 index 0000000000..0c254e6aa1 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/stdio-env-isolation.test.ts @@ -0,0 +1,101 @@ +// What environment does a stdio MCP server actually receive? +// +// This spawns a real subprocess through the real transport and reads back the +// environment that process was handed. Asserting on the arguments we pass to +// the SDK would not answer the question — the SDK merges its own safe-list +// underneath ours, so the only honest answer comes from the child itself. +// +// The child is a plain node script rather than an MCP server: it is spawned by +// the same code path either way, and speaking the protocol would add nothing +// to what is being measured. It never completes a handshake, so the transport +// is closed once the file has been written. + +import { mkdtempSync, readFileSync, rmSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "@effect/vitest"; + +import { createStdioTransport } from "./stdio-connector"; + +/** A variable only this test sets, standing in for a real one like + * `EXECUTOR_SECRET_KEY`. Using a fake keeps the test honest on a machine + * where the real one happens not to be set. */ +const HOST_ONLY_SECRET = "EXECUTOR_TEST_HOST_ONLY_SECRET"; +const HOST_ONLY_VALUE = "host-secret-that-must-not-reach-a-child"; + +const dirs: string[] = []; + +afterEach(() => { + delete process.env[HOST_ONLY_SECRET]; + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +/** Spawn a child through the transport and return the environment it saw. */ +const envSeenByChild = async ( + declared: Record | undefined, +): Promise> => { + const dir = mkdtempSync(join(tmpdir(), "executor-stdio-env-")); + dirs.push(dir); + const out = join(dir, "env.json"); + + const transport = createStdioTransport({ + command: process.execPath, + args: [ + "-e", + "require('node:fs').writeFileSync(process.argv[1], JSON.stringify(process.env))", + out, + ], + env: declared, + }); + + await transport.start(); + // The child writes and exits; poll briefly rather than assuming timing. + for (let i = 0; i < 100 && !existsSync(out); i++) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + await transport.close(); + + // oxlint-disable-next-line executor/no-json-parse -- boundary: reading back the raw env dump this test's own child process just wrote; the value is only key-checked, never decoded into domain types + return JSON.parse(readFileSync(out, "utf8")) as Record; +}; + +describe("environment handed to a stdio MCP subprocess", () => { + it("does not leak a host secret to a server that declares its own env", async () => { + // The declared-env branch is the one that matters: it is the branch a + // credential-bearing integration takes, and it was the leaking one. + process.env[HOST_ONLY_SECRET] = HOST_ONLY_VALUE; + + const childEnv = await envSeenByChild({ DECLARED_TOKEN: "declared-value" }); + + expect(childEnv[HOST_ONLY_SECRET]).toBeUndefined(); + // ...and the thing the integration actually asked for still arrives. + expect(childEnv.DECLARED_TOKEN).toBe("declared-value"); + }); + + it("does not leak a host secret to a server that declares no env", async () => { + process.env[HOST_ONLY_SECRET] = HOST_ONLY_VALUE; + + const childEnv = await envSeenByChild(undefined); + + expect(childEnv[HOST_ONLY_SECRET]).toBeUndefined(); + }); + + it("still provides the SDK's safe-list, so servers keep working", async () => { + // The fix must not strand servers that legitimately need PATH to find + // their own interpreter. The SDK's list is what supplies it. + const childEnv = await envSeenByChild({ DECLARED_TOKEN: "declared-value" }); + + expect(childEnv.PATH).toBeDefined(); + expect(childEnv.HOME).toBeDefined(); + }); + + it("POSITIVE CONTROL: the child does report a variable when it is passed one", async () => { + // Proves the measurement works. Without this, a child that failed to + // write, or wrote an empty object, would satisfy every assertion above. + process.env[HOST_ONLY_SECRET] = HOST_ONLY_VALUE; + + const childEnv = await envSeenByChild({ [HOST_ONLY_SECRET]: HOST_ONLY_VALUE }); + + expect(childEnv[HOST_ONLY_SECRET]).toBe(HOST_ONLY_VALUE); + }); +});