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, + ); + }); +});