fix(security): prevent shell injection in headersHelper (#36)
- Parse headersHelper command with shell-quote to reject operators - Call execFileNoThrowWithCwd(cmd, args) without shell: true - Remove shell option from ExecFileWithCwdOptions type entirely - Add headersHelper.test.ts with injection rejection test - Fix existing MCP test mocks for compatibility Test: MCP tests 91/0, full suite 3058 pass (0 new failures)
This commit is contained in:
parent
f8c3354c75
commit
368dd99d01
|
|
@ -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,
|
||||
|
|
|
|||
63
src/services/mcp/__tests__/headersHelper.test.ts
Normal file
63
src/services/mcp/__tests__/headersHelper.test.ts
Normal file
|
|
@ -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()
|
||||
})
|
||||
})
|
||||
|
|
@ -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", () => ({
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user