diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index 8604a79961..0769c7363d 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -20,7 +20,10 @@ import { import type { FunctionCall } from '@google/genai'; import { SafetyCheckDecision } from '../safety/protocol.js'; import type { CheckerRunner } from '../safety/checker-runner.js'; -import { initializeShellParsers } from '../utils/shell-utils.js'; +import { + initializeShellParsers, + parseCommandDetails, +} from '../utils/shell-utils.js'; import { buildArgsPatterns } from './utils.js'; import { NoopSandboxManager, @@ -477,6 +480,77 @@ describe('PolicyEngine', () => { const { decision } = await engine.check({ name: 'test-tool' }, undefined); expect(decision).toBe(PolicyDecision.DENY); }); + + it('should fail closed in YOLO mode when shell parsing fails for restricted rule', async () => { + const originalMock = vi + .mocked(parseCommandDetails) + .getMockImplementation(); + vi.mocked(parseCommandDetails).mockImplementationOnce( + (command: string) => { + if (command === 'echo bypass') { + return { details: [], hasError: true }; + } + return originalMock!(command); + }, + ); + + const rules: PolicyRule[] = [ + { + toolName: 'run_shell_command', + decision: PolicyDecision.ALLOW, + argsPattern: /"command":"echo/, + }, + ]; + + engine = new PolicyEngine({ + rules, + approvalMode: ApprovalMode.YOLO, + }); + + const { decision } = await engine.check( + { name: 'run_shell_command', args: { command: 'echo bypass' } }, + undefined, + ); + + expect(decision).toBe(PolicyDecision.DENY); + }); + + it('should fail closed in YOLO mode when shell parsing has errors for restricted rule', async () => { + const originalMock = vi + .mocked(parseCommandDetails) + .getMockImplementation(); + vi.mocked(parseCommandDetails).mockImplementationOnce( + (command: string) => { + if (command === 'echo bypass') { + return { + details: [{ name: 'echo', text: 'echo bypass', startIndex: 0 }], + hasError: true, + }; + } + return originalMock!(command); + }, + ); + + const rules: PolicyRule[] = [ + { + toolName: 'run_shell_command', + decision: PolicyDecision.ALLOW, + argsPattern: /"command":"echo/, + }, + ]; + + engine = new PolicyEngine({ + rules, + approvalMode: ApprovalMode.YOLO, + }); + + const { decision } = await engine.check( + { name: 'run_shell_command', args: { command: 'echo bypass' } }, + undefined, + ); + + expect(decision).toBe(PolicyDecision.DENY); + }); }); describe('addRule', () => { diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index e0e3c61215..01f6b75fa6 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -364,14 +364,22 @@ export class PolicyEngine { const parsed = parseCommandDetails(command); const subCommands = parsed?.details ?? []; - if (subCommands.length === 0) { + // Handle parser failures or syntax errors + if (subCommands.length === 0 || parsed?.hasError) { // If the matched rule says DENY, we should respect it immediately even if parsing fails. if (ruleDecision === PolicyDecision.DENY) { return { decision: PolicyDecision.DENY, rule }; } - // In YOLO mode, we should proceed anyway even if we can't parse the command. if (this.approvalMode === ApprovalMode.YOLO) { + // Block execution if arguments cannot be validated + if (rule?.argsPattern) { + debugLogger.debug( + `[PolicyEngine.check] Parsing failed for restricted rule, forcing DENY: ${command}`, + ); + return { decision: PolicyDecision.DENY, rule }; + } + // Allow if no argument restrictions apply return { decision: PolicyDecision.ALLOW, rule,