From 4365653a6553679d20ec906f0f8da8377b5b5e2f Mon Sep 17 00:00:00 2001 From: sean-park-funda Date: Fri, 25 Sep 2026 16:42:51 +0900 Subject: [PATCH] fix(client): honour explicit Path casing in stdio env on Windows Windows environment variables are case-insensitive, but an object spread is not: the PATH produced by getDefaultEnvironment and a caller's Path survived the merge as two separate keys. Node's child_process resolves that duplicate by taking the first in sorted order, so the inherited PATH reached the child and the configured one was silently dropped. Path is the casing Windows itself uses when listing process.env and in many config files, so callers hit this without doing anything unusual. Drop an inherited default when the caller supplied the same variable under any casing, on win32 only. Elsewhere the two names are genuinely different variables and both are kept. Fixes #2859 Co-Authored-By: Claude Opus 5 --- .../stdio-windows-env-case-precedence.md | 5 ++ packages/client/src/client/stdio.ts | 43 +++++++++++++-- packages/client/test/client/stdio.test.ts | 54 ++++++++++++++++++- 3 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 .changeset/stdio-windows-env-case-precedence.md diff --git a/.changeset/stdio-windows-env-case-precedence.md b/.changeset/stdio-windows-env-case-precedence.md new file mode 100644 index 0000000000..d76d2359e9 --- /dev/null +++ b/.changeset/stdio-windows-env-case-precedence.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Honour an explicit `Path` (or any non-`PATH` casing) passed in `StdioServerParameters.env` on Windows. Windows environment variables are case-insensitive but an object spread is not, so the inherited `PATH` and the caller's `Path` both survived the merge; Node's `child_process` then resolved the duplicate in favour of the inherited value and the configured one never reached the server process. diff --git a/packages/client/src/client/stdio.ts b/packages/client/src/client/stdio.ts index 6dbddb667c..03af7a13df 100644 --- a/packages/client/src/client/stdio.ts +++ b/packages/client/src/client/stdio.ts @@ -76,6 +76,44 @@ export const DEFAULT_INHERITED_ENV_VARS = : /* list inspired by the default env inheritance of sudo */ ['HOME', 'LOGNAME', 'PATH', 'SHELL', 'TERM', 'USER']; +/** + * Merges the inherited default environment with the caller-supplied one. + * + * Windows environment variables are case-insensitive, but an object spread is not: + * the `PATH` produced by {@linkcode getDefaultEnvironment} and a caller's `Path` + * survive as two separate keys. Node's `child_process` then resolves the duplicate + * by taking the first in sorted order, so the inherited `PATH` reaches the child and + * the caller's value is silently dropped. `Path` is the casing Windows itself uses, + * so callers hit this without doing anything unusual. + * + * On Windows an inherited default is therefore dropped when the caller supplied the + * same variable under any casing. Elsewhere the two names are genuinely different + * variables and both are kept. + */ +export function mergeDefaultEnvironment( + defaultEnv: Record, + env: Record | undefined +): Record { + if (env === undefined) { + return { ...defaultEnv }; + } + + if (process.platform !== 'win32') { + return { ...defaultEnv, ...env }; + } + + const overridden = new Set(Object.keys(env).map(key => key.toUpperCase())); + const merged: Record = {}; + + for (const [key, value] of Object.entries(defaultEnv)) { + if (!overridden.has(key.toUpperCase())) { + merged[key] = value; + } + } + + return { ...merged, ...env }; +} + /** * Returns a default environment object including only environment variables deemed safe to inherit. */ @@ -135,10 +173,7 @@ export class StdioClientTransport implements Transport { return new Promise((resolve, reject) => { this._process = spawn(this._serverParams.command, this._serverParams.args ?? [], { // merge default env with server env because mcp server needs some env vars - env: { - ...getDefaultEnvironment(), - ...this._serverParams.env - }, + env: mergeDefaultEnvironment(getDefaultEnvironment(), this._serverParams.env), stdio: ['pipe', 'pipe', this._serverParams.stderr ?? 'inherit'], shell: false, windowsHide: process.platform === 'win32', diff --git a/packages/client/test/client/stdio.test.ts b/packages/client/test/client/stdio.test.ts index fe6d9258ce..9cd0984621 100644 --- a/packages/client/test/client/stdio.test.ts +++ b/packages/client/test/client/stdio.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from 'node:os'; import type { JSONRPCMessage } from '@modelcontextprotocol/core-internal'; import type { StdioServerParameters } from '../../src/client/stdio'; -import { DEFAULT_INHERITED_ENV_VARS, StdioClientTransport } from '../../src/client/stdio'; +import { DEFAULT_INHERITED_ENV_VARS, mergeDefaultEnvironment, StdioClientTransport } from '../../src/client/stdio'; // Configure default server parameters based on OS // Uses 'more' command for Windows and 'tee' command for Unix/Linux @@ -181,3 +181,55 @@ test('DEFAULT_INHERITED_ENV_VARS matches the host platform', () => { expect(DEFAULT_INHERITED_ENV_VARS).toEqual(['HOME', 'LOGNAME', 'PATH', 'SHELL', 'TERM', 'USER']); } }); + +describe('mergeDefaultEnvironment', () => { + const originalPlatform = process.platform; + + const setPlatform = (value: NodeJS.Platform) => { + Object.defineProperty(process, 'platform', { value }); + }; + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: originalPlatform }); + }); + + test('returns a copy of the defaults when no environment is given', () => { + setPlatform('win32'); + const defaults = { PATH: 'C:\\inherited' }; + + const merged = mergeDefaultEnvironment(defaults, undefined); + + expect(merged).toEqual(defaults); + expect(merged).not.toBe(defaults); + }); + + test('on Windows, an explicit Path replaces the inherited PATH', () => { + setPlatform('win32'); + + const merged = mergeDefaultEnvironment({ PATH: 'C:\\inherited', SYSTEMROOT: 'C:\\Windows' }, { Path: 'C:\\explicit-only' }); + + // Both keys surviving is the bug: Node resolves the duplicate by sorted + // order, so the inherited PATH would win and the caller's Path is dropped. + expect(Object.keys(merged).filter(key => key.toUpperCase() === 'PATH')).toEqual(['Path']); + expect(merged.Path).toBe('C:\\explicit-only'); + expect(merged.SYSTEMROOT).toBe('C:\\Windows'); + }); + + test('on Windows, an exact-case override still wins and defaults are otherwise kept', () => { + setPlatform('win32'); + + const merged = mergeDefaultEnvironment({ PATH: 'C:\\inherited', TEMP: 'C:\\Temp' }, { PATH: 'C:\\explicit', EXTRA: '1' }); + + expect(merged).toEqual({ PATH: 'C:\\explicit', TEMP: 'C:\\Temp', EXTRA: '1' }); + }); + + test('off Windows, names differing only in case stay separate variables', () => { + setPlatform('linux'); + + const merged = mergeDefaultEnvironment({ PATH: '/inherited' }, { Path: '/explicit' }); + + // POSIX environment variables are case-sensitive, so these are two + // different variables and dropping either would be wrong. + expect(merged).toEqual({ PATH: '/inherited', Path: '/explicit' }); + }); +});