diff --git a/changelog.d/fix-mcpl-child-env-scrub.breaking.md b/changelog.d/fix-mcpl-child-env-scrub.breaking.md new file mode 100644 index 00000000..41b109f8 --- /dev/null +++ b/changelog.d/fix-mcpl-child-env-scrub.breaking.md @@ -0,0 +1,7 @@ +- **Operators of stdio MCPL servers:** children no longer inherit the whole + host environment. They get a small allowlist (`PATH`, `HOME`, locale/`LC_*`, + `TMPDIR`, TLS roots, …; see `CHILD_ENV_ALLOWLIST`) plus the server's own + `env`, so one server's credentials (provider API keys, other bots' tokens) + are no longer readable by another. A server that relied on an inherited + variable must declare it in its `env` (recipes can `${VAR}`-substitute), or + set `inheritEnv: true` to restore full inheritance. diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 356dd5e0..2fc8a523 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -101,6 +101,40 @@ export abstract class McplTransport extends EventEmitter { // stdio // --------------------------------------------------------------------------- +/** + * Host variables a stdio child gets by default: just enough for an ordinary + * process to run (find binaries, a home dir, temp dir, locale, TLS roots, + * a display for GUI helpers). Everything else in the host env — notably API + * keys and bot tokens — is NOT passed on; a server that needs a variable must + * declare it in its `env` (recipes can `${VAR}`-substitute from .env), or set + * `inheritEnv: true` to get the whole host env as before. + */ +export const CHILD_ENV_ALLOWLIST = [ + 'PATH', 'HOME', 'USER', 'LOGNAME', 'SHELL', 'TERM', 'LANG', 'TZ', + 'TMPDIR', 'TMP', 'TEMP', + 'DISPLAY', 'WAYLAND_DISPLAY', + 'XDG_RUNTIME_DIR', 'XDG_CONFIG_HOME', 'XDG_CACHE_HOME', 'XDG_DATA_HOME', + 'SSL_CERT_FILE', 'SSL_CERT_DIR', 'NODE_EXTRA_CA_CERTS', + '__CF_USER_TEXT_ENCODING', + // Windows equivalents, harmless elsewhere. + 'SYSTEMROOT', 'WINDIR', 'COMSPEC', 'PATHEXT', 'APPDATA', 'LOCALAPPDATA', 'USERPROFILE', +]; + +/** Environment for a stdio child: allowlisted host vars (+ LC_*), then the + * server's declared `env` on top. `inheritEnv: true` restores full inheritance. */ +export function buildChildEnv( + config: Pick, + hostEnv: NodeJS.ProcessEnv = process.env, +): NodeJS.ProcessEnv { + if (config.inheritEnv) return { ...hostEnv, ...config.env }; + const env: NodeJS.ProcessEnv = {}; + for (const [key, value] of Object.entries(hostEnv)) { + if (value === undefined) continue; + if (CHILD_ENV_ALLOWLIST.includes(key) || key.startsWith('LC_')) env[key] = value; + } + return { ...env, ...config.env }; +} + export class StdioTransport extends McplTransport { readonly kind = 'stdio' as const; @@ -139,7 +173,7 @@ export class StdioTransport extends McplTransport { } const child = spawn(config.command, config.args ?? [], { stdio: ['pipe', 'pipe', 'pipe'], - env: { ...process.env, ...config.env }, + env: buildChildEnv(config), }); const rl = createInterface({ input: child.stdout! }); return new StdioTransport(child, rl); diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index 30cc3273..f0aec725 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -209,9 +209,20 @@ export interface McplServerConfig { /** Arguments for the command */ args?: string[]; - /** Environment variables for the child process */ + /** + * Environment variables for the child process. Stdio children do NOT inherit + * the host environment wholesale: they get a small allowlist (PATH, HOME, + * LANG, LC_*, TMPDIR, ... — see CHILD_ENV_ALLOWLIST) plus exactly these. + */ env?: Record; + /** + * Escape hatch: pass the host's entire environment (including secrets) to + * the stdio child, as older versions did. Only for servers that genuinely + * need it. Default false. + */ + inheritEnv?: boolean; + /** * WebSocket URL for the network transport (`ws://` or `wss://`). Mutually * exclusive with `command`. When set (or `transport: 'websocket'`), the host diff --git a/test/mcpl-child-env.test.ts b/test/mcpl-child-env.test.ts new file mode 100644 index 00000000..9458a5af --- /dev/null +++ b/test/mcpl-child-env.test.ts @@ -0,0 +1,74 @@ +/** + * Stdio MCPL children must not inherit the host's secrets. They get a small + * operating allowlist (PATH, HOME, LC_*, ...) plus their declared `env`; + * `inheritEnv: true` restores full inheritance. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { buildChildEnv, StdioTransport } from '../src/mcpl/transport.js'; + +const HOST_ENV = { + PATH: '/usr/bin:/bin', + HOME: '/home/resident', + LANG: 'en_US.UTF-8', + LC_CTYPE: 'UTF-8', + TMPDIR: '/tmp/x', + ANTHROPIC_AUTH_TOKEN: 'host-anthropic-secret', + DISCORD_TOKEN: 'host-discord-secret', + OPENAI_API_KEY: 'host-openai-secret', +}; + +test('drops host secrets, keeps operating vars and LC_*', () => { + const env = buildChildEnv({}, HOST_ENV); + assert.deepEqual(env, { + PATH: '/usr/bin:/bin', + HOME: '/home/resident', + LANG: 'en_US.UTF-8', + LC_CTYPE: 'UTF-8', + TMPDIR: '/tmp/x', + }); +}); + +test('declared env is passed through and wins over allowlisted host vars', () => { + const env = buildChildEnv( + { env: { DISCORD_TOKEN: 'declared-token', PATH: '/opt/bin' } }, + HOST_ENV, + ); + assert.equal(env.DISCORD_TOKEN, 'declared-token'); + assert.equal(env.PATH, '/opt/bin'); + assert.equal(env.HOME, '/home/resident'); + assert.equal(env.ANTHROPIC_AUTH_TOKEN, undefined); + assert.equal(env.OPENAI_API_KEY, undefined); +}); + +test('inheritEnv: true passes the whole host env, declared env on top', () => { + const env = buildChildEnv({ inheritEnv: true, env: { EXTRA: '1' } }, HOST_ENV); + assert.equal(env.ANTHROPIC_AUTH_TOKEN, 'host-anthropic-secret'); + assert.equal(env.EXTRA, '1'); +}); + +test('a real spawned stdio child sees only allowlist + declared env', async () => { + const secretKey = 'MCPL_ENV_TEST_HOST_SECRET'; + process.env[secretKey] = 'must-not-leak'; + try { + const transport = StdioTransport.spawn({ + id: 'env-probe', + command: process.execPath, + args: ['-e', 'console.log(JSON.stringify(process.env))'], + env: { DECLARED_VAR: 'declared-value' }, + }); + const line = await new Promise((resolve, reject) => { + transport.once('line', resolve); + transport.once('error', reject); + }); + await transport.close(); + const childEnv = JSON.parse(line) as Record; + assert.equal(childEnv[secretKey], undefined); + assert.equal(childEnv.DECLARED_VAR, 'declared-value'); + if (process.env.PATH) assert.equal(childEnv.PATH, process.env.PATH); + if (process.env.HOME) assert.equal(childEnv.HOME, process.env.HOME); + } finally { + delete process.env[secretKey]; + } +});