From 0d4e2479056238cff7656d43e66818e47daae038 Mon Sep 17 00:00:00 2001 From: ArkhAngelLifeJiggy <141562589+LifeJiggy@users.noreply.github.com> Date: Mon, 8 Jun 2026 23:23:30 +0100 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20pass=20MCP=20stdio=20server=20args?= =?UTF-8?q?=20as=20separate=20array=20elements=20to=20pr=E2=80=A6=20(#1222?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): pass MCP stdio server args as separate array elements to prevent shell injection (issue #131) * fix: extract buildMcpStdioCommand helper and add regression tests (PR #131 review) * fix(mcp): handle shell -c prefix in buildMcpStdioCommand (PR #131 review) When CLAUDE_CODE_SHELL_PREFIX contains -c (e.g. sh -c, bash -c), the original MCP command and args must be joined as a single shell command string after -c. Without this join, sh -c runs only the first word as the command string and treats remaining entries as positional parameters, so the MCP server never receives its configured arguments. - Detect -c in prefixParts and join command+args into one string - Non-shell prefixes (docker run --rm -i, bunx, etc.) unchanged - Add regression test for sh -c pattern * chore: add .tmp to gitignore * fix(mcp): shell-quote each arg in sh -c join to prevent injection (PR #131 P2 fixup) * fix(mcp): preserve spaced executable path in buildMcpStdioCommand -c split (PR #131 fixup) Use lastIndexOf(' -c') instead of whitespace split so paths like 'C:\Program Files\Git\bin\bash.exe -c' are handled correctly. Removes dead old code left in from previous edit. --- .gitignore | 3 +- src/services/mcp/client.test.ts | 106 +++++++++++++++++++++++++++++++- src/services/mcp/client.ts | 76 +++++++++++++++++++++-- 3 files changed, 178 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index e3c95ef48..3c8aa89cc 100644 --- a/.gitignore +++ b/.gitignore @@ -17,4 +17,5 @@ package-lock.json coverage/ agent.log plan/ -temp_reference/ \ No newline at end of file +.tmp +temp_reference/ diff --git a/src/services/mcp/client.test.ts b/src/services/mcp/client.test.ts index 6f69ee7b9..dd625fc01 100644 --- a/src/services/mcp/client.test.ts +++ b/src/services/mcp/client.test.ts @@ -1,7 +1,7 @@ import assert from 'node:assert/strict' import test from 'node:test' -import { cleanupFailedConnection } from './client.js' +import { cleanupFailedConnection, buildMcpStdioCommand } from './client.js' test('cleanupFailedConnection awaits transport close before resolving', async () => { let closed = false @@ -46,3 +46,107 @@ test('cleanupFailedConnection closes in-process server and transport', async () assert.equal(inProcessClosed, true) assert.equal(transportClosed, true) }) + +test('buildMcpStdioCommand — no prefix passes command and args through unchanged', () => { + const { command, args } = buildMcpStdioCommand( + 'node', + ['server.js', '--port=8080'], + undefined, + ) + assert.equal(command, 'node') + assert.deepEqual(args, ['server.js', '--port=8080']) +}) + +test('buildMcpStdioCommand — empty string prefix is treated as no prefix', () => { + const { command, args } = buildMcpStdioCommand( + 'uvx', + ['mcp-server'], + '', + ) + assert.equal(command, 'uvx') + assert.deepEqual(args, ['mcp-server']) +}) + +test('buildMcpStdioCommand — single-part prefix: prefix is command, original command is first arg', () => { + const { command, args } = buildMcpStdioCommand( + 'npx', + ['@modelcontextprotocol/server-everything', '--debug'], + 'bunx', + ) + assert.equal(command, 'bunx') + assert.deepEqual(args, [ + 'npx', + '@modelcontextprotocol/server-everything', + '--debug', + ]) +}) + +test('buildMcpStdioCommand — multi-part prefix: structured argv with no shell join', () => { + const { command, args } = buildMcpStdioCommand( + 'some-server', + ['--path=/tmp;rm -rf /', '--arg=$(whoami)'], + 'docker run --rm -i', + ) + assert.equal(command, 'docker') + assert.deepEqual(args, [ + 'run', + '--rm', + '-i', + 'some-server', + '--path=/tmp;rm -rf /', + '--arg=$(whoami)', + ]) +}) + +test('buildMcpStdioCommand — whitespace in prefix is normalized (multiple spaces, tabs)', () => { + const { command, args } = buildMcpStdioCommand( + 'cmd', + [], + ' sudo -u bob ', + ) + assert.equal(command, 'sudo') + assert.deepEqual(args, ['-u', 'bob', 'cmd']) +}) + +test('buildMcpStdioCommand — shell -c prefix joins command+args as single string (sh -c pattern)', () => { + const { command, args } = buildMcpStdioCommand( + 'some-server', + ['--port=8080', '--debug'], + 'sh -c', + ) + assert.equal(command, 'sh') + assert.deepEqual(args, ['-c', "'some-server' '--port=8080' '--debug'"]) +}) + +test('buildMcpStdioCommand — shell -c prefix escapes args to prevent injection', () => { + const { command, args } = buildMcpStdioCommand( + 'some-server', + ['--path=/tmp; touch /tmp/pwned', 'normal-arg'], + 'sh -c', + ) + assert.equal(command, 'sh') + // The semicolon and spaces inside the arg are inside single quotes, + // so the shell treats them as a literal string, not as syntax. + assert.deepEqual(args, ['-c', "'some-server' '--path=/tmp; touch /tmp/pwned' 'normal-arg'"]) +}) + +test('buildMcpStdioCommand — shell -c prefix escapes embedded single quotes', () => { + const { command, args } = buildMcpStdioCommand( + "some-server", + ["it's a test"], + 'sh -c', + ) + assert.equal(command, 'sh') + // Embedded single quote is escaped: 'it'\''s test' + assert.deepEqual(args, ['-c', "'some-server' 'it'\\''s a test'"]) +}) + +test('buildMcpStdioCommand — shell -c prefix with spaced executable path (Windows Git Bash)', () => { + const { command, args } = buildMcpStdioCommand( + 'some-server', + ['--port=8080'], + 'C:\\Program Files\\Git\\bin\\bash.exe -c', + ) + assert.equal(command, 'C:\\Program Files\\Git\\bin\\bash.exe') + assert.deepEqual(args, ['-c', "'some-server' '--port=8080'"]) +}) diff --git a/src/services/mcp/client.ts b/src/services/mcp/client.ts index 06e911e8a..0f60eaa62 100644 --- a/src/services/mcp/client.ts +++ b/src/services/mcp/client.ts @@ -960,11 +960,17 @@ export const connectToServer = memoize( transport = clientTransport logMCPDebug(name, `In-process Computer Use MCP server started`) } else if (serverRef.type === 'stdio' || !serverRef.type) { - const finalCommand = - process.env.CLAUDE_CODE_SHELL_PREFIX || serverRef.command - const finalArgs = process.env.CLAUDE_CODE_SHELL_PREFIX - ? [[serverRef.command, ...serverRef.args].join(' ')] - : serverRef.args + // Split the prefix into separate args so we hand a real array to the + // MCP SDK's stdio transport. Joining the server command + its args + // into one string and putting that single string inside a one-element + // array causes the SDK to shell-invoke the whole blob, letting shell + // metacharacters in serverRef.args run arbitrary commands before the + // target binary even starts. + const { command: finalCommand, args: finalArgs } = buildMcpStdioCommand( + serverRef.command, + serverRef.args ?? [], + process.env.CLAUDE_CODE_SHELL_PREFIX, + ) transport = new StdioClientTransport({ command: finalCommand, args: finalArgs, @@ -3311,6 +3317,66 @@ function extractToolUseId(message: AssistantMessage): string | undefined { return message.message.content[0].id } +/** + * Build the command and args for a stdio MCP transport, applying the + * CLAUDE_CODE_SHELL_PREFIX split into separate argv entries. This + * ensures the MCP SDK receives a proper command + args[] instead of + * a shell-joined string, preventing shell metacharacter injection + * from serverRef.args. + * + * When a prefix is set, prefixParts[0] becomes the command and + * prefixParts[1..] + original command + original args become the + * argv array. + */ +export function buildMcpStdioCommand( + command: string, + args: string[], + shellPrefix?: string, +): { command: string; args: string[] } { + if (!shellPrefix) { + return { command, args } + } + + let finalCommand: string + let prefixArgs: string[] + + // Split on the last " -c" to preserve spaced executable path + // (e.g. "C:\Program Files\Git\bin\bash.exe -c"). When no " -c" is + // present, fall back to plain whitespace split. + const cIndex = shellPrefix.lastIndexOf(' -c') + if (cIndex > 0) { + finalCommand = shellPrefix.substring(0, cIndex) + prefixArgs = ['-c', ...shellPrefix.substring(cIndex + 3).split(/\s+/).filter(Boolean)] + } else { + const parts = shellPrefix.split(/\s+/).filter(Boolean) + if (parts.length === 0) return { command, args } + finalCommand = parts[0] + prefixArgs = parts.slice(1) + } + + // Shell -c prefix (e.g. sh -c, bash -c): everything after -c is a single + // shell command string, not individual argv entries. Without this join, + // sh -c runs only the first word as the command string and treats the + // remaining entries as shell positional parameters ($0, $1, ...), so the + // MCP server never receives its configured arguments. + // + // Each original command/arg is single-quote-escaped to prevent shell + // injection via MCP server args (e.g. --path=/tmp; rm -rf / would + // otherwise execute the semicolon as a command separator). + if (prefixArgs.includes('-c')) { + const cmdStr = [command, ...args].map(a => `'${a.replace(/'/g, "'\\''")}'`).join(' ') + return { + command: finalCommand, + args: [...prefixArgs, cmdStr], + } + } + + return { + command: finalCommand, + args: [...prefixArgs, command, ...args], + } +} + /** * Sets up SDK MCP clients by creating transports and connecting them. * This is used for SDK MCP servers that run in the same process as the SDK.