From 0bc2fa4effc8cf17f069b14951ff017d4e9fc674 Mon Sep 17 00:00:00 2001 From: Greg Land Date: Sun, 12 Jul 2026 09:34:50 -0400 Subject: [PATCH] fix: isolate stdio MCP child env from OpenWiki credentials Stdio MCP subprocesses were spawned with the full process.env, so any MCP server command (which comes from user-editable connector config) inherited every provider API key and OAuth refresh token. Build the child env from an allow-list of safe base variables (PATH, HOME, TMPDIR, locale/terminal, Windows equivalents) plus only the credentials the transport explicitly declares via transport.env. The full process.env is never forwarded. Route all three stdio spawn sites through the new buildChildEnv helper. Add test/mcp-client.test.ts pinning the invariant: secrets don't leak by key or value, allow-listed base vars pass through, declared refs resolve to the mapped target var, and invalid/unresolvable refs throw. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/connectors/mcp-client.ts | 60 ++++++++++++++++++++++++------- test/mcp-client.test.ts | 70 ++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 12 deletions(-) create mode 100644 test/mcp-client.test.ts diff --git a/src/connectors/mcp-client.ts b/src/connectors/mcp-client.ts index fdc3c73e..c72d752b 100644 --- a/src/connectors/mcp-client.ts +++ b/src/connectors/mcp-client.ts @@ -170,10 +170,7 @@ async function executeStdioMcp( } const child = spawn(transport.command, transport.args ?? [], { - env: { - ...process.env, - ...resolveChildEnv(transport.env ?? {}), - }, + env: buildChildEnv(transport.env ?? {}), shell: false, stdio: ["pipe", "pipe", "pipe"], }); @@ -220,10 +217,7 @@ async function executeStdioMcpTool( } const child = spawn(transport.command, transport.args ?? [], { - env: { - ...process.env, - ...resolveChildEnv(transport.env ?? {}), - }, + env: buildChildEnv(transport.env ?? {}), shell: false, stdio: ["pipe", "pipe", "pipe"], }); @@ -256,10 +250,7 @@ async function listStdioMcpTools( } const child = spawn(transport.command, transport.args ?? [], { - env: { - ...process.env, - ...resolveChildEnv(transport.env ?? {}), - }, + env: buildChildEnv(transport.env ?? {}), shell: false, stdio: ["pipe", "pipe", "pipe"], }); @@ -734,6 +725,51 @@ function resolveChildEnv( ); } +// Base environment variables an MCP subprocess may legitimately need to run +// (locating binaries, resolving the home dir, temp paths, terminal behavior). +// Deliberately excludes OpenWiki credentials so a spawned MCP server command +// cannot read the user's API keys and OAuth refresh tokens out of process.env. +const CHILD_ENV_ALLOWLIST = [ + "PATH", + "HOME", + "HOMEPATH", + "HOMEDRIVE", + "USERPROFILE", + "TMPDIR", + "TEMP", + "TMP", + "LANG", + "LC_ALL", + "LC_CTYPE", + "TERM", + "TZ", + "SystemRoot", + "SYSTEMROOT", + "ComSpec", + "PATHEXT", +] as const; + +// Builds the environment for a spawned stdio MCP subprocess: only an +// allow-listed set of safe base variables plus the credentials the transport +// explicitly declares via `transport.env`. The full process.env (which holds +// every provider API key and OAuth token) is never forwarded. +// Exported for tests to pin this security invariant. +export function buildChildEnv( + envRefs: Record, +): Record { + const base: Record = {}; + for (const key of CHILD_ENV_ALLOWLIST) { + const value = process.env[key]; + if (typeof value === "string") { + base[key] = value; + } + } + return { + ...base, + ...resolveChildEnv(envRefs), + }; +} + async function resolveHeaders( headers: Record, ): Promise> { diff --git a/test/mcp-client.test.ts b/test/mcp-client.test.ts new file mode 100644 index 00000000..44cc55e5 --- /dev/null +++ b/test/mcp-client.test.ts @@ -0,0 +1,70 @@ +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { buildChildEnv } from "../src/connectors/mcp-client.ts"; + +describe("buildChildEnv", () => { + const SECRET_KEYS = [ + "ANTHROPIC_API_KEY", + "OPENAI_API_KEY", + "TAVILY_API_KEY", + "SLACK_CLIENT_SECRET", + "GMAIL_ACCESS_TOKEN", + "X_REFRESH_TOKEN", + ]; + const saved: Record = {}; + + beforeEach(() => { + for (const key of [...SECRET_KEYS, "PATH", "MCP_SERVER_TOKEN"]) { + saved[key] = process.env[key]; + } + for (const key of SECRET_KEYS) { + process.env[key] = `secret-value-for-${key}`; + } + process.env.PATH = "/usr/bin:/bin"; + process.env.MCP_SERVER_TOKEN = "declared-token-123"; + }); + + afterEach(() => { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + }); + + test("does not forward OpenWiki credentials to the child env", () => { + const childEnv = buildChildEnv({}); + for (const key of SECRET_KEYS) { + expect(childEnv).not.toHaveProperty(key); + } + // A random full-process.env secret must never leak by value either. + expect(Object.values(childEnv)).not.toContain( + "secret-value-for-ANTHROPIC_API_KEY", + ); + }); + + test("passes through allow-listed base variables like PATH", () => { + const childEnv = buildChildEnv({}); + expect(childEnv.PATH).toBe("/usr/bin:/bin"); + }); + + test("resolves only the credentials the transport explicitly declares", () => { + const childEnv = buildChildEnv({ MCP_TOKEN: "${MCP_SERVER_TOKEN}" }); + expect(childEnv.MCP_TOKEN).toBe("declared-token-123"); + // The source var name itself is not exposed, only the mapped target var. + expect(childEnv).not.toHaveProperty("MCP_SERVER_TOKEN"); + }); + + test("throws for an unresolvable declared reference", () => { + expect(() => buildChildEnv({ MCP_TOKEN: "${DOES_NOT_EXIST}" })).toThrow( + /DOES_NOT_EXIST is required/u, + ); + }); + + test("rejects invalid child env key names", () => { + expect(() => buildChildEnv({ "bad-key": "${PATH}" })).toThrow( + /Invalid env var reference/u, + ); + }); +});