fix(mcp): pass MCP stdio server args as separate array elements to pr… (#1222)

* 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

@@ -17,4 +17,5 @@ package-lock.json
 coverage/
 agent.log
 plan/
-temp_reference/
+.tmp
+temp_reference/

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'"])
+})

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.