diff --git a/changelog.d/205-child-network-env.fixed.md b/changelog.d/205-child-network-env.fixed.md new file mode 100644 index 000000000..76e8c49f4 --- /dev/null +++ b/changelog.d/205-child-network-env.fixed.md @@ -0,0 +1 @@ +- Stdio MCPL children inherit `REQUESTS_CA_BUNDLE`, `CURL_CA_BUNDLE`, and uppercase/lowercase `HTTP_PROXY`, `HTTPS_PROXY`, and `NO_PROXY` so Python/curl CA trust and proxy-based egress survive the child environment allowlist (#191). Declared server `env` still overrides host values, including differently cased keys and empty values on Windows; POSIX names stay case-sensitive. Duplicate Windows spellings within one source use Node's first lexicographic spelling before declared values override host values. Proxy URLs can carry credentials; the allowlist limits accidental inheritance rather than isolating a hostile child. diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 2fc8a523e..044249cc4 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -104,10 +104,12 @@ export abstract class McplTransport extends EventEmitter { /** * 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 + * network proxies, 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. + * `inheritEnv: true` to get the whole host env as before. Proxy URLs may include + * credentials. This limits accidental inheritance, not access by a hostile + * same-user child process; it is not a process isolation boundary. */ export const CHILD_ENV_ALLOWLIST = [ 'PATH', 'HOME', 'USER', 'LOGNAME', 'SHELL', 'TERM', 'LANG', 'TZ', @@ -115,24 +117,54 @@ export const CHILD_ENV_ALLOWLIST = [ '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', + 'REQUESTS_CA_BUNDLE', 'CURL_CA_BUNDLE', + 'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', + 'http_proxy', 'https_proxy', 'no_proxy', '__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. */ + * server's declared `env` on top. Windows names merge case-insensitively; + * POSIX names remain case-sensitive. `inheritEnv: true` restores full inheritance. */ export function buildChildEnv( config: Pick, hostEnv: NodeJS.ProcessEnv = process.env, + platform: NodeJS.Platform = process.platform, ): 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; + const win = platform === 'win32'; + const env: NodeJS.ProcessEnv = config.inheritEnv ? { ...hostEnv } : {}; + if (!config.inheritEnv) { + for (const [key, value] of Object.entries(hostEnv)) { + if (value === undefined) continue; + // Windows commonly enumerates Path/SystemRoot rather than PATH/SYSTEMROOT. + const probe = win ? key.toUpperCase() : key; + if (CHILD_ENV_ALLOWLIST.includes(probe) || probe.startsWith('LC_')) env[key] = value; + } + } + if (!win) return { ...env, ...config.env }; + + // Windows spawn folds environment names case-insensitively and may choose + // an earlier host spelling over a differently cased declared override. + // Select Node's first lexicographic spelling within each source, then layer + // declared values (including '') over host values. Source precedence and + // duplicate-name resolution are separate decisions. + const merged: NodeJS.ProcessEnv = {}; + const spellings = new Map(); + for (const source of [env, config.env ?? {}]) { + const seen = new Set(); + for (const key of Object.keys(source).sort()) { + const canonical = key.toUpperCase(); + if (seen.has(canonical)) continue; + seen.add(canonical); + const previous = spellings.get(canonical); + if (previous !== undefined && previous !== key) delete merged[previous]; + merged[key] = source[key]; + spellings.set(canonical, key); + } } - return { ...env, ...config.env }; + return merged; } export class StdioTransport extends McplTransport { diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index c080af76d..a5732e1d6 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -229,7 +229,9 @@ export interface McplServerConfig { /** * 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. + * LANG, LC_*, TMPDIR, TLS CA bundles, and HTTP(S) proxy settings — see + * CHILD_ENV_ALLOWLIST) plus exactly these. Proxy URLs can contain credentials. + * This limits accidental inheritance; it does not isolate a hostile child. */ env?: Record; diff --git a/test/mcpl-child-env.test.ts b/test/mcpl-child-env.test.ts index 9458a5af7..6773837a5 100644 --- a/test/mcpl-child-env.test.ts +++ b/test/mcpl-child-env.test.ts @@ -30,6 +30,39 @@ test('drops host secrets, keeps operating vars and LC_*', () => { }); }); +test('keeps CA bundles and uppercase/lowercase proxy configuration', () => { + const networkEnv = { + REQUESTS_CA_BUNDLE: '/etc/ssl/certs/internal.pem', + CURL_CA_BUNDLE: '/etc/ssl/certs/curl.pem', + HTTP_PROXY: 'http://user:password@proxy.example:8080', + HTTPS_PROXY: 'http://proxy.example:8443', + NO_PROXY: 'localhost,127.0.0.1,.internal', + http_proxy: 'http://lowercase.example:8080', + https_proxy: 'http://lowercase.example:8443', + no_proxy: 'localhost,.lowercase.internal', + }; + const env = buildChildEnv({}, { ...HOST_ENV, ...networkEnv }, 'linux'); + for (const [key, value] of Object.entries(networkEnv)) { + assert.equal(env[key], value, key); + } + assert.equal(env.OPENAI_API_KEY, undefined); + assert.equal(env.DISCORD_TOKEN, undefined); + assert.equal(env.ANTHROPIC_AUTH_TOKEN, undefined); +}); + +test('declared network settings override the host and empty values are preserved', () => { + const env = buildChildEnv( + { env: { REQUESTS_CA_BUNDLE: '/server/ca.pem', HTTPS_PROXY: '', NO_PROXY: '*' } }, + { REQUESTS_CA_BUNDLE: '/host/ca.pem', HTTPS_PROXY: 'http://proxy.example', NO_PROXY: '', http_proxy: '', CURL_CA_BUNDLE: undefined }, + ); + assert.deepEqual(env, { + REQUESTS_CA_BUNDLE: '/server/ca.pem', + HTTPS_PROXY: '', + NO_PROXY: '*', + http_proxy: '', + }); +}); + test('declared env is passed through and wins over allowlisted host vars', () => { const env = buildChildEnv( { env: { DISCORD_TOKEN: 'declared-token', PATH: '/opt/bin' } }, @@ -50,7 +83,11 @@ test('inheritEnv: true passes the whole host env, declared env on top', () => { test('a real spawned stdio child sees only allowlist + declared env', async () => { const secretKey = 'MCPL_ENV_TEST_HOST_SECRET'; + const previousCaBundle = process.env.REQUESTS_CA_BUNDLE; + const previousProxy = process.env.HTTPS_PROXY; process.env[secretKey] = 'must-not-leak'; + process.env.REQUESTS_CA_BUNDLE = '/host/internal-ca.pem'; + process.env.HTTPS_PROXY = 'http://proxy.example:8443'; try { const transport = StdioTransport.spawn({ id: 'env-probe', @@ -66,9 +103,75 @@ test('a real spawned stdio child sees only allowlist + declared env', async () = const childEnv = JSON.parse(line) as Record; assert.equal(childEnv[secretKey], undefined); assert.equal(childEnv.DECLARED_VAR, 'declared-value'); + assert.equal(childEnv.REQUESTS_CA_BUNDLE, '/host/internal-ca.pem'); + assert.equal(childEnv.HTTPS_PROXY, 'http://proxy.example:8443'); 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]; + if (previousCaBundle === undefined) delete process.env.REQUESTS_CA_BUNDLE; + else process.env.REQUESTS_CA_BUNDLE = previousCaBundle; + if (previousProxy === undefined) delete process.env.HTTPS_PROXY; + else process.env.HTTPS_PROXY = previousProxy; + } +}); + +for (const inheritEnv of [false, true]) { + test(`win32: declared proxy/CA overrides win across case variants (inheritEnv=${inheritEnv})`, () => { + for (const name of ['HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', 'REQUESTS_CA_BUNDLE', 'CURL_CA_BUNDLE']) { + for (const [hostKey, declaredKey] of [[name, name.toLowerCase()], [name.toLowerCase(), name]]) { + const env = buildChildEnv( + { inheritEnv, env: { [declaredKey!]: '' } }, + { [hostKey!]: 'http://user:password@host-proxy.example', OPENAI_API_KEY: 'host-secret' }, + 'win32', + ); + assert.deepEqual( + Object.keys(env).filter((key) => key.toUpperCase() === name), + [declaredKey], + 'only the declared spelling may reach Windows spawn', + ); + assert.equal(env[declaredKey!], '', name + ' must remain explicitly disabled'); + assert.equal(env.OPENAI_API_KEY, inheritEnv ? 'host-secret' : undefined); + } + } + }); +} + +test('win32: operating allowlist matches native mixed-case spellings', () => { + const env = buildChildEnv({}, { + Path: 'C:\\Windows', SystemRoot: 'C:\\Windows', ComSpec: 'C:\\Windows\\System32\\cmd.exe', + Https_Proxy: 'http://proxy.example', Requests_CA_Bundle: 'C:\\internal.pem', + lc_ctype: 'UTF-8', OpenAI_Api_Key: 'host-secret', + }, 'win32'); + assert.deepEqual(env, { + Path: 'C:\\Windows', SystemRoot: 'C:\\Windows', ComSpec: 'C:\\Windows\\System32\\cmd.exe', + Https_Proxy: 'http://proxy.example', Requests_CA_Bundle: 'C:\\internal.pem', lc_ctype: 'UTF-8', + }); +}); + +test('win32: sorted-first spelling wins within each source regardless of insertion order', () => { + for (const reverse of [false, true]) { + const hostEntries = [['https_proxy', 'http://lower-host.example'], ['HTTPS_PROXY', 'http://upper-host.example']]; + const declaredEntries = [['https_proxy', 'http://lower-declared.example'], ['HTTPS_PROXY', '']]; + const host = Object.fromEntries(reverse ? hostEntries.reverse() : hostEntries); + const declared = Object.fromEntries(reverse ? declaredEntries.reverse() : declaredEntries); + for (const inheritEnv of [false, true]) { + assert.deepEqual(buildChildEnv({ inheritEnv }, host, 'win32'), { HTTPS_PROXY: 'http://upper-host.example' }); + assert.deepEqual(buildChildEnv({ inheritEnv, env: declared }, host, 'win32'), { HTTPS_PROXY: '' }); + assert.deepEqual(buildChildEnv({ inheritEnv, env: { https_proxy: '' } }, host, 'win32'), { https_proxy: '' }, + 'declared source still outranks every host spelling'); + } + } +}); + +test('posix: differently cased host and declared keys remain distinct', () => { + for (const inheritEnv of [false, true]) { + const env = buildChildEnv({ inheritEnv, env: { https_proxy: '' } }, { + HTTPS_PROXY: 'http://host.example', Path: '/mixed-case', PATH: '/usr/bin', + }, 'linux'); + assert.equal(env.HTTPS_PROXY, 'http://host.example'); + assert.equal(env.https_proxy, ''); + assert.equal(env.PATH, '/usr/bin'); + assert.equal(env.Path, inheritEnv ? '/mixed-case' : undefined); } });