diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index bb65fbdab7..fae05f2aa6 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -147,7 +147,7 @@ export interface UpdatePolicy { persist?: boolean; persistScope?: 'workspace' | 'user'; argsPattern?: string; - commandPrefix?: string | string[]; + commandPrefix?: string | string[] | string[][]; mcpName?: string; allowRedirection?: boolean; } diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index 38106e7261..7617fd4452 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -539,7 +539,7 @@ interface TomlRule { mcpName?: string; decision?: string; priority?: number; - commandPrefix?: string | string[]; + commandPrefix?: string | string[] | string[][]; argsPattern?: string; allowRedirection?: boolean; // Index signature to satisfy Record type if needed for toml.stringify @@ -561,7 +561,12 @@ export function createPolicyUpdater( if (message.commandPrefix) { // Convert commandPrefix(es) to argsPatterns for in-memory rules - const patterns = buildArgsPatterns(undefined, message.commandPrefix); + const patterns = buildArgsPatterns( + undefined, + message.commandPrefix, + undefined, + message.allowRedirection, + ); const tier = message.persistScope === 'user' ? USER_POLICY_TIER diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 5ee9d65df4..de6552ffa8 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -25,8 +25,10 @@ vi.mock('node:fs/promises'); vi.mock('../config/storage.js'); vi.mock('../utils/shell-utils.js', () => ({ getCommandRoots: vi.fn(), + getCommandSegments: vi.fn(), stripShellWrapper: vi.fn(), hasRedirection: vi.fn(), + isNakedSensitiveCommand: vi.fn(), })); interface ParsedPolicy { rule?: Array<{ @@ -82,7 +84,7 @@ describe('createPolicyUpdater', () => { priority: ALWAYS_ALLOW_PRIORITY, mcpName: 'test-mcp', argsPattern: new RegExp( - escapeRegex('"command":"echo') + '(?:[\\s"]|\\\\")', + `\\x00${escapeRegex('"command":"')}echo\\b(?:(?:[^"&|;\\n\\r<>]|\\\\"))*${escapeRegex('"')}\\x00`, ), }), ); @@ -93,7 +95,7 @@ describe('createPolicyUpdater', () => { priority: ALWAYS_ALLOW_PRIORITY, mcpName: 'test-mcp', argsPattern: new RegExp( - escapeRegex('"command":"ls') + '(?:[\\s"]|\\\\")', + `\\x00${escapeRegex('"command":"')}ls\\b(?:(?:[^"&|;\\n\\r<>]|\\\\"))*${escapeRegex('"')}\\x00`, ), }), ); @@ -172,7 +174,7 @@ describe('createPolicyUpdater', () => { toolName: 'run_shell_command', priority: ALWAYS_ALLOW_PRIORITY, argsPattern: new RegExp( - escapeRegex('"command":"git') + '(?:[\\s"]|\\\\")', + `\\x00${escapeRegex('"command":"git')}\\b(?:(?:[^"&|;\n\r<>]|\\\\"))*${escapeRegex('"')}\\x00`, ), }), ); @@ -259,10 +261,14 @@ describe('ShellToolInvocation Policy Update', () => { (c: string) => c, ); vi.mocked(shellUtils.hasRedirection).mockReturnValue(false); + vi.mocked(shellUtils.isNakedSensitiveCommand).mockReturnValue(false); }); - it('should extract multiple root commands for chained commands', () => { - vi.mocked(shellUtils.getCommandRoots).mockReturnValue(['git', 'npm']); + it('should extract multiple command segments for chained commands', () => { + vi.mocked(shellUtils.getCommandSegments).mockReturnValue([ + ['git'], + ['npm'], + ]); const invocation = new ShellToolInvocation( mockConfig, @@ -276,14 +282,14 @@ describe('ShellToolInvocation Policy Update', () => { const options = ( invocation as unknown as TestableShellToolInvocation ).getPolicyUpdateOptions(ToolConfirmationOutcome.ProceedAlways); - expect(options!.commandPrefix).toEqual(['git', 'npm']); - expect(shellUtils.getCommandRoots).toHaveBeenCalledWith( + expect(options!.commandPrefix).toEqual([['git'], ['npm']]); + expect(shellUtils.getCommandSegments).toHaveBeenCalledWith( 'git status && npm test', ); }); - it('should extract a single root command', () => { - vi.mocked(shellUtils.getCommandRoots).mockReturnValue(['ls']); + it('should extract a single command segment', () => { + vi.mocked(shellUtils.getCommandSegments).mockReturnValue([['ls']]); const invocation = new ShellToolInvocation( mockConfig, @@ -297,12 +303,12 @@ describe('ShellToolInvocation Policy Update', () => { const options = ( invocation as unknown as TestableShellToolInvocation ).getPolicyUpdateOptions(ToolConfirmationOutcome.ProceedAlways); - expect(options!.commandPrefix).toEqual(['ls']); - expect(shellUtils.getCommandRoots).toHaveBeenCalledWith('ls -la /tmp'); + expect(options!.commandPrefix).toEqual([['ls']]); + expect(shellUtils.getCommandSegments).toHaveBeenCalledWith('ls -la /tmp'); }); it('should include allowRedirection if command has redirection', () => { - vi.mocked(shellUtils.getCommandRoots).mockReturnValue(['echo']); + vi.mocked(shellUtils.getCommandSegments).mockReturnValue([['echo']]); vi.mocked(shellUtils.hasRedirection).mockReturnValue(true); const invocation = new ShellToolInvocation( @@ -316,7 +322,7 @@ describe('ShellToolInvocation Policy Update', () => { const options = ( invocation as unknown as TestableShellToolInvocation ).getPolicyUpdateOptions(ToolConfirmationOutcome.ProceedAlways); - expect(options!.commandPrefix).toEqual(['echo']); + expect(options!.commandPrefix).toEqual([['echo']]); expect(options!.allowRedirection).toBe(true); expect(shellUtils.hasRedirection).toHaveBeenCalledWith( 'echo "hello" > file.txt', diff --git a/packages/core/src/policy/shell-safety.test.ts b/packages/core/src/policy/shell-safety.test.ts index 340264485e..11220b614c 100644 --- a/packages/core/src/policy/shell-safety.test.ts +++ b/packages/core/src/policy/shell-safety.test.ts @@ -150,6 +150,64 @@ describe('Shell Safety Policy', () => { expect(result.decision).toBe(PolicyDecision.ASK_USER); }); + describe('Sequence-based matching (e.g. ["git", "log"])', () => { + let sequencePolicyEngine: PolicyEngine; + + beforeEach(() => { + const argsPatterns = buildArgsPatterns( + undefined, + [['git', 'log']], + undefined, + ); + sequencePolicyEngine = new PolicyEngine({ + rules: [ + { + toolName: 'run_shell_command', + argsPattern: new RegExp(argsPatterns[0]!), + decision: PolicyDecision.ALLOW, + priority: 10, + }, + ], + defaultDecision: PolicyDecision.ASK_USER, + }); + }); + + it('SHOULD allow "git log" and variations with flags', async () => { + const commands = [ + 'git log', + 'git log --oneline', + 'git --no-pager log', + 'git -c color.ui=always log', + ]; + + for (const command of commands) { + const result = await sequencePolicyEngine.check( + { name: 'run_shell_command', args: { command } }, + undefined, + ); + expect(result.decision).toBe(PolicyDecision.ALLOW); + } + }); + + it('SHOULD NOT allow "git push" or unrelated git commands', async () => { + const toolCall: FunctionCall = { + name: 'run_shell_command', + args: { command: 'git push' }, + }; + const result = await sequencePolicyEngine.check(toolCall, undefined); + expect(result.decision).toBe(PolicyDecision.ASK_USER); + }); + + it('SHOULD NOT allow compound commands if any part is disallowed', async () => { + const toolCall: FunctionCall = { + name: 'run_shell_command', + args: { command: 'git log && rm -rf /' }, + }; + const result = await sequencePolicyEngine.check(toolCall, undefined); + expect(result.decision).toBe(PolicyDecision.ASK_USER); + }); + }); + it('SHOULD NOT allow "git log; rm -rf /" (semicolon separator)', async () => { const toolCall: FunctionCall = { name: 'run_shell_command', @@ -327,6 +385,7 @@ describe('Shell Safety Policy', () => { undefined, 'git log', undefined, + true, ); const policyWithRedirection = new PolicyEngine({ rules: [ diff --git a/packages/core/src/policy/toml-loader.test.ts b/packages/core/src/policy/toml-loader.test.ts index 6835e200b4..77029cdf41 100644 --- a/packages/core/src/policy/toml-loader.test.ts +++ b/packages/core/src/policy/toml-loader.test.ts @@ -10,6 +10,7 @@ import { ApprovalMode, PRIORITY_SUBAGENT_TOOL, } from './types.js'; +import { stableStringify } from './stable-stringify.js'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import * as os from 'node:os'; @@ -94,11 +95,15 @@ priority = 100 expect(result.rules[0].toolName).toBe('run_shell_command'); expect(result.rules[1].toolName).toBe('run_shell_command'); expect( - result.rules[0].argsPattern?.test('{"command":"git status"}'), + result.rules[0].argsPattern?.test( + stableStringify({ command: 'git status' }), + ), + ).toBe(true); + expect( + result.rules[1].argsPattern?.test( + stableStringify({ command: 'git log' }), + ), ).toBe(true); - expect(result.rules[1].argsPattern?.test('{"command":"git log"}')).toBe( - true, - ); expect(result.errors).toHaveLength(0); }); @@ -160,13 +165,19 @@ priority = 100 expect(result.rules).toHaveLength(1); expect( - result.rules[0].argsPattern?.test('{"command":"git status"}'), + result.rules[0].argsPattern?.test( + stableStringify({ command: 'git status' }), + ), ).toBe(true); expect( - result.rules[0].argsPattern?.test('{"command":"git log --all"}'), + result.rules[0].argsPattern?.test( + stableStringify({ command: 'git log --all' }), + ), ).toBe(true); expect( - result.rules[0].argsPattern?.test('{"command":"git branch"}'), + result.rules[0].argsPattern?.test( + stableStringify({ command: 'git branch' }), + ), ).toBe(false); expect(result.errors).toHaveLength(0); }); @@ -184,7 +195,9 @@ priority = 100 // The generated pattern is "command":"^git status // This will NOT match '{"command":"git status"}' because of the '{"' at the start. expect( - result.rules[0].argsPattern?.test('{"command":"git status"}'), + result.rules[0].argsPattern?.test( + stableStringify({ command: 'git status' }), + ), ).toBe(false); expect(result.errors).toHaveLength(0); }); @@ -421,10 +434,14 @@ priority = 100 expect(result.rules).toHaveLength(1); // The regex should have escaped the * and . expect( - result.rules[0].argsPattern?.test('{"command":"git log file.txt"}'), + result.rules[0].argsPattern?.test( + stableStringify({ command: 'git log file.txt' }), + ), ).toBe(false); expect( - result.rules[0].argsPattern?.test('{"command":"git log *.txt"}'), + result.rules[0].argsPattern?.test( + stableStringify({ command: 'git log *.txt' }), + ), ).toBe(true); expect(result.errors).toHaveLength(0); }); diff --git a/packages/core/src/policy/toml-loader.ts b/packages/core/src/policy/toml-loader.ts index 977e8a399a..3325cae785 100644 --- a/packages/core/src/policy/toml-loader.ts +++ b/packages/core/src/policy/toml-loader.ts @@ -41,7 +41,9 @@ const PolicyRuleSchema = z.object({ subagent: z.string().optional(), mcpName: z.string().optional(), argsPattern: z.string().optional(), - commandPrefix: z.union([z.string(), z.array(z.string())]).optional(), + commandPrefix: z + .union([z.string(), z.array(z.union([z.string(), z.array(z.string())]))]) + .optional(), commandRegex: z.string().optional(), decision: z.nativeEnum(PolicyDecision), // Priority must be in range [0, 999] to prevent tier overflow. @@ -76,7 +78,9 @@ const SafetyCheckerRuleSchema = z.object({ toolName: z.union([z.string(), z.array(z.string())]), mcpName: z.string().optional(), argsPattern: z.string().optional(), - commandPrefix: z.union([z.string(), z.array(z.string())]).optional(), + commandPrefix: z + .union([z.string(), z.array(z.union([z.string(), z.array(z.string())]))]) + .optional(), commandRegex: z.string().optional(), priority: z.number().int().default(0), modes: z.array(z.nativeEnum(ApprovalMode)).optional(), @@ -458,6 +462,7 @@ export async function loadPoliciesFromToml( rule.argsPattern, rule.commandPrefix, rule.commandRegex, + rule.allowRedirection ?? rule.allow_redirection, ); // For each argsPattern, expand toolName arrays diff --git a/packages/core/src/policy/utils.test.ts b/packages/core/src/policy/utils.test.ts index db6225827a..c543717d35 100644 --- a/packages/core/src/policy/utils.test.ts +++ b/packages/core/src/policy/utils.test.ts @@ -6,6 +6,7 @@ import { expect, describe, it } from 'vitest'; import { escapeRegex, buildArgsPatterns, isSafeRegExp } from './utils.js'; +import { stableStringify } from './stable-stringify.js'; describe('policy/utils', () => { describe('escapeRegex', () => { @@ -69,44 +70,51 @@ describe('policy/utils', () => { it('should build pattern from a single commandPrefix', () => { const result = buildArgsPatterns(undefined, 'ls', undefined); - expect(result).toEqual(['\\"command\\":\\"ls(?:[\\s"]|\\\\")']); + expect(result).toEqual([ + '\\x00\\"command\\":\\"ls\\b(?:(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', + ]); }); it('should build patterns from an array of commandPrefixes', () => { const result = buildArgsPatterns(undefined, ['echo', 'ls'], undefined); expect(result).toEqual([ - '\\"command\\":\\"echo(?:[\\s"]|\\\\")', - '\\"command\\":\\"ls(?:[\\s"]|\\\\")', + '\\x00\\"command\\":\\"echo\\b(?:(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', + '\\x00\\"command\\":\\"ls\\b(?:(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', ]); }); it('should build pattern from commandRegex', () => { const result = buildArgsPatterns(undefined, undefined, 'rm -rf .*'); - expect(result).toEqual(['"command":"rm -rf .*']); + expect(result).toEqual([ + '\\x00\\"command\\":\\"rm -rf .*(?:(?!&|;\n\r<>)(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', + ]); }); it('should prioritize commandPrefix over commandRegex and argsPattern', () => { const result = buildArgsPatterns('raw', 'prefix', 'regex'); - expect(result).toEqual(['\\"command\\":\\"prefix(?:[\\s"]|\\\\")']); + expect(result).toEqual([ + '\\x00\\"command\\":\\"prefix\\b(?:(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', + ]); }); it('should prioritize commandRegex over argsPattern if no commandPrefix', () => { const result = buildArgsPatterns('raw', undefined, 'regex'); - expect(result).toEqual(['"command":"regex']); + expect(result).toEqual([ + '\\x00\\"command\\":\\"regex(?:(?!&|;\n\r<>)(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', + ]); }); it('should escape characters in commandPrefix', () => { const result = buildArgsPatterns(undefined, 'git checkout -b', undefined); expect(result).toEqual([ - '\\"command\\":\\"git\\ checkout\\ \\-b(?:[\\s"]|\\\\")', + '\\x00\\"command\\":\\"git\\ checkout\\ \\-b\\b(?:(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', ]); }); it('should correctly escape quotes in commandPrefix', () => { const result = buildArgsPatterns(undefined, 'git "fix"', undefined); expect(result).toEqual([ - // eslint-disable-next-line no-useless-escape - '\\\"command\\\":\\\"git\\ \\\\\\\"fix\\\\\\\"(?:[\\s\"]|\\\\\")', + '\\x00\\"command\\":\\"git\\ \\\\\\"fix\\\\\\"(?:(?:[^"&|;\n\r<>]|\\\\"))*\\"\\x00', ]); }); @@ -121,9 +129,8 @@ describe('policy/utils', () => { const patterns = buildArgsPatterns(undefined, prefix, undefined); const regex = new RegExp(patterns[0]!); - // Mimic JSON stringified args - // echo "foo" -> {"command":"echo \"foo\""} - const validJsonArgs = '{"command":"echo \\"foo\\""}'; + // Mimic JSON stringified args with null byte boundaries + const validJsonArgs = stableStringify({ command: 'echo "foo"' }); expect(regex.test(validJsonArgs)).toBe(true); }); @@ -134,16 +141,76 @@ describe('policy/utils', () => { const regex = new RegExp(patterns[0]!); // echo\foo -> {"command":"echo\\foo"} - // In regex matching: "echo " is followed by "\" which is NOT in [\s"] and is not \" - const attackJsonArgs = '{"command":"echo\\\\foo"}'; + const attackJsonArgs = stableStringify({ command: 'echo\\foo' }); expect(regex.test(attackJsonArgs)).toBe(false); // Also validation for "git " matching "git\status" const gitPatterns = buildArgsPatterns(undefined, 'git ', undefined); const gitRegex = new RegExp(gitPatterns[0]!); // git\status -> {"command":"git\\status"} - const gitAttack = '{"command":"git\\\\status"}'; + const gitAttack = stableStringify({ command: 'git\\status' }); expect(gitAttack).not.toMatch(gitRegex); }); + + it('should NOT match chained commands using shell operators (security check)', () => { + // Testing that we block "git log && rm -rf /" when the prefix is "git log" + const prefix = 'git log'; + const patterns = buildArgsPatterns(undefined, prefix, undefined); + const regex = new RegExp(patterns[0]!); + + const attackJsonArgs = stableStringify({ + command: 'git log && rm -rf /', + }); + expect(regex.test(attackJsonArgs)).toBe(false); + + const semicolonAttack = stableStringify({ + command: 'git log; rm -rf /', + }); + expect(regex.test(semicolonAttack)).toBe(false); + }); + + describe('sequence matching', () => { + it('should build pattern from a sequence of tokens', () => { + const result = buildArgsPatterns( + undefined, + [['git', 'log']], + undefined, + ); + const regex = new RegExp(result[0]!); + + expect(regex.test(stableStringify({ command: 'git log' }))).toBe(true); + expect( + regex.test(stableStringify({ command: 'git log --oneline' })), + ).toBe(true); + expect( + regex.test(stableStringify({ command: 'git --no-pager log' })), + ).toBe(true); + expect(regex.test(stableStringify({ command: 'git push' }))).toBe( + false, + ); + }); + + it('should allow flags and options between tokens', () => { + const result = buildArgsPatterns( + undefined, + [['python3', 'main.py']], + undefined, + ); + const regex = new RegExp(result[0]!); + + expect( + regex.test(stableStringify({ command: 'python3 main.py' })), + ).toBe(true); + expect( + regex.test(stableStringify({ command: 'python3 main.py --help' })), + ).toBe(true); + expect( + regex.test(stableStringify({ command: 'python3 -u main.py' })), + ).toBe(true); + expect( + regex.test(stableStringify({ command: 'python3 other.py' })), + ).toBe(false); + }); + }); }); }); diff --git a/packages/core/src/policy/utils.ts b/packages/core/src/policy/utils.ts index 3c7bd4d16b..a9d81eb690 100644 --- a/packages/core/src/policy/utils.ts +++ b/packages/core/src/policy/utils.ts @@ -49,21 +49,57 @@ export function isSafeRegExp(pattern: string): boolean { * the internal argsPattern representation used by the PolicyEngine. * * @param argsPattern An optional raw regex string for arguments. - * @param commandPrefix An optional command prefix (or list of prefixes) to allow. + * @param commandPrefix An optional command prefix (or list of prefixes/sequences) to allow. * @param commandRegex An optional command regex string to allow. + * @param allowRedirection Whether to allow redirection operators in the command. * @returns An array of string patterns (or undefined) for the PolicyEngine. */ export function buildArgsPatterns( argsPattern?: string, - commandPrefix?: string | string[], + commandPrefix?: string | Array, commandRegex?: string, + allowRedirection?: boolean, ): Array { + const shellSeparators = '&|;\n\r'; + const forbiddenChars = allowRedirection + ? shellSeparators + : `${shellSeparators}<>`; + + const forbiddenCharClass = `(?:[^"${forbiddenChars}]|\\\\")`; + if (commandPrefix) { const prefixes = Array.isArray(commandPrefix) ? commandPrefix : [commandPrefix]; return prefixes.map((prefix) => { + if (Array.isArray(prefix)) { + // Handle sequence matching (e.g., ['git', 'log']) + // We want to match these tokens in order, allowing flags in between. + if (prefix.length === 0) return undefined; + + // Escape each token and ensure word boundaries + const escapedTokens = prefix.map((token) => escapeRegex(token)); + + // Start with matching \0"command":" + let pattern = `\\x00${escapeRegex('"command":"')}${escapedTokens[0]}`; + + // For subsequent tokens, allow flags in between + for (let i = 1; i < escapedTokens.length; i++) { + // Allow whitespace or flags (starting with -) in between tokens. + // [^"]*? is replaced by forbiddenCharClass to stay within the command string value + // and avoid separators. + const gap = `(?:(?:\\s+|-(?:${forbiddenCharClass})*?)\\s*)`; + pattern += `${gap}*${escapedTokens[i]}`; + } + + // After the last token, we also don't allow forbidden characters until the end of the command string. + // We match until the closing quote of the command value followed by \0. + // We add a word boundary if the last token ends in a word character. + const suffix = /\w$/.test(prefix[prefix.length - 1]) ? '\\b' : ''; + return `${pattern}${suffix}(?:${forbiddenCharClass})*${escapeRegex('"')}\\x00`; + } + // JSON.stringify safely encodes the prefix in quotes. // We remove ONLY the trailing quote to match it as an open prefix string. const encodedPrefix = JSON.stringify(prefix); @@ -72,18 +108,22 @@ export function buildArgsPatterns( encodedPrefix.length - 1, ); - // Escape the exact JSON literal segment we expect to see - const matchSegment = escapeRegex(`"command":${openQuotePrefix}`); + // Escape the exact JSON literal segment we expect to see, including \0 boundary. + const matchSegment = `\\x00${escapeRegex( + `"command":${openQuotePrefix}`, + )}`; - // We allow [\s], ["], or the specific sequence [\"] (for escaped quotes - // in JSON). We do NOT allow generic [\\], which would match "git\status" - // -> "gitstatus". - return `${matchSegment}(?:[\\s"]|\\\\")`; + // We allow any characters except forbidden shell separators until the closing quote and \0. + // We add a word boundary if the prefix ends in a word character. + const suffix = /\w$/.test(prefix) ? '\\b' : ''; + return `${matchSegment}${suffix}(?:${forbiddenCharClass})*${escapeRegex('"')}\\x00`; }); } if (commandRegex) { - return [`"command":"${commandRegex}`]; + return [ + `\\x00${escapeRegex('"command":"')}${commandRegex}(?:(?!${forbiddenChars})${forbiddenCharClass})*${escapeRegex('"')}\\x00`, + ]; } return [argsPattern]; @@ -102,10 +142,10 @@ export function buildParamArgsPattern( value: unknown, ): string { const encodedValue = JSON.stringify(value); - // We wrap the JSON string in escapeRegex and prepend/append \\0 to explicitly + // We wrap the JSON string in escapeRegex and prepend/append \\x00 to explicitly // match top-level JSON properties generated by stableStringify, preventing // argument injection bypass attacks. - return `\\\\0${escapeRegex(`"${paramName}":${encodedValue}`)}\\\\0`; + return `\\x00${escapeRegex(`"${paramName}":${encodedValue}`)}\\x00`; } /** diff --git a/packages/core/src/sandbox/utils/commandSafety.ts b/packages/core/src/sandbox/utils/commandSafety.ts index 180d0748d2..d000f112c6 100644 --- a/packages/core/src/sandbox/utils/commandSafety.ts +++ b/packages/core/src/sandbox/utils/commandSafety.ts @@ -77,8 +77,28 @@ export function isKnownSafeCommand(args: string[]): boolean { // Normalize zsh to bash const normalizedArgs = args.map((a) => (a === 'zsh' ? 'bash' : a)); + const shellMetaCharacters = [ + '&', + '|', + ';', + '<', + '>', + '(', + ')', + '$', + '`', + '\\\\', + '\\n', + '\\r', + ]; + const hasShellMeta = (str: string) => + shellMetaCharacters.some((meta) => str.includes(meta)); + if (isSafeToCallWithExec(normalizedArgs)) { - return true; + // Ensure no arguments contain shell metacharacters that could lead to injection + // when this is called from a shell context (like PolicyEngine). + // We allow the root command itself (normalizedArgs[0]) but check all arguments. + return !normalizedArgs.slice(1).some(hasShellMeta); } // Support `bash -lc "..."` diff --git a/packages/core/src/scheduler/policy.test.ts b/packages/core/src/scheduler/policy.test.ts index 44a3feaa34..d2fd098e94 100644 --- a/packages/core/src/scheduler/policy.test.ts +++ b/packages/core/src/scheduler/policy.test.ts @@ -696,7 +696,7 @@ describe('policy.ts', () => { expect.objectContaining({ toolName: 'write_file', argsPattern: - '\\\\0' + escapeRegex('"file_path":"src/foo.ts"') + '\\\\0', + '\\x00' + escapeRegex('"file_path":"src/foo.ts"') + '\\x00', }), ); }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 63a9b1dc83..c2c163da72 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -37,11 +37,14 @@ import { formatBytes } from '../utils/formatters.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; import { getCommandRoots, + getCommandSegments, initializeShellParsers, stripShellWrapper, parseCommandDetails, hasRedirection, + isNakedSensitiveCommand, } from '../utils/shell-utils.js'; +import { buildParamArgsPattern } from '../policy/utils.js'; import { SHELL_TOOL_NAME } from './tool-names.js'; import { PARAM_ADDITIONAL_PERMISSIONS } from './definitions/base-declarations.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; @@ -142,13 +145,23 @@ export class ShellToolInvocation extends BaseToolInvocation< outcome === ToolConfirmationOutcome.ProceedAlways ) { const command = stripShellWrapper(this.params.command); - const rootCommands = [...new Set(getCommandRoots(command))]; + const segments = getCommandSegments(command); const allowRedirection = hasRedirection(command) ? true : undefined; - if (rootCommands.length > 0) { - return { commandPrefix: rootCommands, allowRedirection }; + // Filter out "naked" sensitive commands to prevent over-broad matching + const safeSegments = segments.filter( + (seg) => !isNakedSensitiveCommand(seg), + ); + + if (safeSegments.length > 0) { + return { commandPrefix: safeSegments, allowRedirection }; } - return { commandPrefix: this.params.command, allowRedirection }; + // If the command is naked and sensitive, we fall back to the raw command as an exact argsPattern + // instead of a broad prefix rule. + return { + argsPattern: buildParamArgsPattern('command', this.params.command), + allowRedirection, + }; } return undefined; } diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index e89ef1b9e6..e7b258ea8f 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -149,7 +149,7 @@ export function isBackgroundExecutionData( */ export interface PolicyUpdateOptions { argsPattern?: string; - commandPrefix?: string | string[]; + commandPrefix?: string | string[] | string[][]; mcpName?: string; toolName?: string; allowRedirection?: boolean; diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index e2a240a0b0..5b80cd0152 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -751,6 +751,131 @@ export function getCommandRoots(command: string): string[] { .filter(Boolean); } +/** + * Binaries that are considered sensitive and should not be allowed to be saved as "naked" rules. + */ +export const SENSITIVE_BINARIES = new Set([ + 'git', + 'node', + 'npm', + 'npx', + 'yarn', + 'pnpm', + 'python', + 'python3', + 'pip', + 'pip3', + 'bash', + 'sh', + 'zsh', + 'sudo', + 'docker', + 'kubectl', + 'aws', + 'gcloud', + 'az', + 'chmod', + 'chown', + 'rm', + 'mv', + 'cp', + 'find', + 'grep', + 'sed', + 'awk', + 'curl', + 'wget', + 'ssh', + 'scp', + 'rsync', +]); + +/** + * Checks if a command sequence represents a "naked" sensitive command. + * A command is naked if it only contains the binary name without any subcommands or scripts. + * + * @param sequence - The command sequence (e.g., ['git', 'log']). + * @returns true if the command is a naked sensitive command. + */ +export function isNakedSensitiveCommand(sequence: string[]): boolean { + if (sequence.length === 0) return false; + const binary = sequence[0]; + // A command is "naked" if it has only one token (the binary itself) + // and that binary is in the sensitive list. + return sequence.length === 1 && SENSITIVE_BINARIES.has(binary); +} + +/** + * Extracts sequences of positional arguments for each command in a potentially chained shell command. + * Flags (starting with -) are ignored, and subcommands/scripts are captured. + * + * @param command - The shell command string to parse. + * @returns An array of sequences, where each sequence is string[]. + * @example "git --no-pager log" -> [["git", "log"]] + * @example "git log && ls -la" -> [["git", "log"], ["ls"]] + */ +export function getCommandSegments(command: string): string[][] { + if (!command) { + return []; + } + + const bashResult = parseBashCommandDetails(command); + if (!bashResult || bashResult.hasError) { + // Fallback to simple root extraction if tree-sitter fails or is not bash + const roots = getCommandRoots(command); + return roots.map((r) => [r]); + } + + const tree = parseCommandTree(command); + if (!tree) { + const roots = getCommandRoots(command); + return roots.map((r) => [r]); + } + + const segments: string[][] = []; + + function walk(node: Node) { + if (node.type === 'command') { + const sequence: string[] = []; + const nameNode = node.childForFieldName('name'); + if (nameNode) { + sequence.push(normalizeCommandName(nameNode.text)); + } + + for (let i = 0; i < node.childCount; i++) { + const child = node.child(i); + if (!child || child.id === nameNode?.id) continue; + + // We only care about words/strings that are arguments + if ( + child.type === 'word' || + child.type === 'string' || + child.type === 'raw_string' + ) { + const text = child.text; + if (!text.startsWith('-')) { + sequence.push(normalizeCommandName(text)); + } + } + // Limit sequence length to command + first positional argument + // to avoid over-specifying (e.g., git commit "message" -> git commit) + if (sequence.length >= 2) break; + } + if (sequence.length > 0) { + segments.push(sequence); + } + } + + for (let i = 0; i < node.childCount; i++) { + const child = node.child(i); + if (child) walk(child); + } + } + + walk(tree.rootNode); + return segments; +} + export function stripShellWrapper(command: string): string { const pattern = /^\s*(?:(?:(?:\S+\/)?(?:sh|bash|zsh))\s+-c|cmd\.exe\s+\/c|powershell(?:\.exe)?\s+(?:-NoProfile\s+)?-Command|pwsh(?:\.exe)?\s+(?:-NoProfile\s+)?-Command)\s+/i;