diff --git a/src/services/mcp/__tests__/channelPermissions.test.ts b/src/services/mcp/__tests__/channelPermissions.test.ts index dc19af315..86813bef0 100644 --- a/src/services/mcp/__tests__/channelPermissions.test.ts +++ b/src/services/mcp/__tests__/channelPermissions.test.ts @@ -1,7 +1,12 @@ import { mock, describe, expect, test } from "bun:test"; mock.module("src/utils/slowOperations.js", () => ({ + clone: structuredClone, + cloneDeep: structuredClone, + jsonParse: JSON.parse, jsonStringify: (v: unknown) => JSON.stringify(v), + slowLogging: () => ({ [Symbol.dispose]: () => {} }), + writeFileSync_DEPRECATED: () => {}, })); mock.module("src/services/analytics/growthbook.js", () => ({ getFeatureValue_CACHED_MAY_BE_STALE: () => false, diff --git a/src/services/mcp/__tests__/headersHelper.test.ts b/src/services/mcp/__tests__/headersHelper.test.ts new file mode 100644 index 000000000..7b49704d3 --- /dev/null +++ b/src/services/mcp/__tests__/headersHelper.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, mock, test } from 'bun:test' +import type { McpHTTPServerConfig } from '../types.js' + +mock.module('src/utils/config.js', () => ({ + checkHasTrustDialogAccepted: () => true, +})) +mock.module('src/utils/debug.js', () => ({ + logAntError: () => {}, +})) +mock.module('src/utils/log.js', () => ({ + logError: () => {}, + logMCPDebug: () => {}, + logMCPError: () => {}, +})) +mock.module('src/services/analytics/index.js', () => ({ + logEvent: () => {}, +})) +mock.module('src/utils/slowOperations.js', () => ({ + clone: structuredClone, + cloneDeep: structuredClone, + jsonParse: JSON.parse, + jsonStringify: JSON.stringify, + slowLogging: () => ({ [Symbol.dispose]: () => {} }), + writeFileSync_DEPRECATED: () => {}, +})) + +const { getMcpHeadersFromHelper } = await import('../headersHelper.js') + +function makeConfig(headersHelper: string): McpHTTPServerConfig { + return { + type: 'http', + url: 'https://example.com/mcp', + headersHelper, + } +} + +function validHelperCommand(value: string): string { + const script = + 'console.log(JSON.stringify({ Authorization: process.argv[1] }))' + return [process.execPath, '-e', JSON.stringify(script), JSON.stringify(value)].join( + ' ', + ) +} + +describe('getMcpHeadersFromHelper', () => { + test('executes helper commands with quoted arguments without a shell', async () => { + const headers = await getMcpHeadersFromHelper( + 'test-server', + makeConfig(validHelperCommand('Bearer token with spaces')), + ) + + expect(headers).toEqual({ Authorization: 'Bearer token with spaces' }) + }) + + test('rejects shell operators in helper commands', async () => { + const headers = await getMcpHeadersFromHelper( + 'test-server', + makeConfig(`${validHelperCommand('safe')} ; true`), + ) + + expect(headers).toBeNull() + }) +}) diff --git a/src/services/mcp/__tests__/officialRegistry.test.ts b/src/services/mcp/__tests__/officialRegistry.test.ts index ffb4b94c9..11577e176 100644 --- a/src/services/mcp/__tests__/officialRegistry.test.ts +++ b/src/services/mcp/__tests__/officialRegistry.test.ts @@ -4,6 +4,7 @@ mock.module("axios", () => ({ default: { get: async () => ({ data: { servers: [] } }) }, })); mock.module("src/utils/debug.js", () => ({ + logAntError: () => {}, logForDebugging: () => {}, })); mock.module("src/utils/errors.js", () => ({ diff --git a/src/services/mcp/headersHelper.ts b/src/services/mcp/headersHelper.ts index 3ae0e3d95..e918bc01a 100644 --- a/src/services/mcp/headersHelper.ts +++ b/src/services/mcp/headersHelper.ts @@ -1,3 +1,4 @@ +import { parse as shellParse } from 'shell-quote' import { getIsNonInteractiveSession } from '../../bootstrap/state.js' import { checkHasTrustDialogAccepted } from '../../utils/config.js' import { logAntError } from '../../utils/debug.js' @@ -58,8 +59,22 @@ export async function getMcpHeadersFromHelper( try { logMCPDebug(serverName, 'Executing headersHelper to get dynamic headers') - const execResult = await execFileNoThrowWithCwd(config.headersHelper, [], { - shell: true, + + // Parse the headersHelper command string into tokens. + // Reject any input containing shell operators to prevent injection. + const tokens = shellParse(config.headersHelper) + if (!tokens.every((t: unknown) => typeof t === 'string')) { + throw new Error( + `headersHelper must be an executable path with arguments only, no shell operators allowed`, + ) + } + const commandTokens = tokens as string[] + if (commandTokens.length === 0) { + throw new Error('headersHelper command is empty') + } + const [cmd, ...args] = commandTokens + + const execResult = await execFileNoThrowWithCwd(cmd, args, { timeout: 10000, // Pass server context so one helper script can serve multiple MCP servers // (git credential-helper style). See deshaw/anthropic-issues#28. diff --git a/src/utils/execFileNoThrow.ts b/src/utils/execFileNoThrow.ts index d6bf018ed..e2d99c7a6 100644 --- a/src/utils/execFileNoThrow.ts +++ b/src/utils/execFileNoThrow.ts @@ -50,7 +50,6 @@ type ExecFileWithCwdOptions = { maxBuffer?: number cwd?: string env?: NodeJS.ProcessEnv - shell?: boolean | string | undefined stdin?: 'ignore' | 'inherit' | 'pipe' input?: string } @@ -96,7 +95,6 @@ export function execFileNoThrowWithCwd( cwd: finalCwd, env: finalEnv, maxBuffer, - shell, stdin: finalStdin, input: finalInput, }: ExecFileWithCwdOptions = { @@ -113,7 +111,6 @@ export function execFileNoThrowWithCwd( timeout: finalTimeout, cwd: finalCwd, env: finalEnv, - shell, stdin: finalStdin, input: finalInput, reject: false, // Don't throw on non-zero exit codes