From ed469e492b4190ce8983b9dd1c5fb5b9f1b140d0 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Fri, 24 Apr 2026 12:26:59 -0700 Subject: [PATCH] fix(core): fail closed in YOLO mode when shell parsing fails for restricted rules (#25935) --- .../core/src/policy/policy-engine.test.ts | 76 ++++++++++++++++++- packages/core/src/policy/policy-engine.ts | 12 ++- 2 files changed, 85 insertions(+), 3 deletions(-) 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,