diff --git a/packages/a2a-server/src/utils/testing_utils.ts b/packages/a2a-server/src/utils/testing_utils.ts index d472b4f995..87c7315f82 100644 --- a/packages/a2a-server/src/utils/testing_utils.ts +++ b/packages/a2a-server/src/utils/testing_utils.ts @@ -16,6 +16,7 @@ import { DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD, GeminiClient, HookSystem, + PolicyDecision, } from '@google/gemini-cli-core'; import { createMockMessageBus } from '@google/gemini-cli-core/src/test-utils/mock-message-bus.js'; import type { Config, Storage } from '@google/gemini-cli-core'; @@ -77,6 +78,17 @@ export function createMockConfig( mockConfig.getGeminiClient = vi .fn() .mockReturnValue(new GeminiClient(mockConfig)); + + mockConfig.getPolicyEngine = vi.fn().mockReturnValue({ + check: async () => { + const mode = mockConfig.getApprovalMode(); + if (mode === ApprovalMode.YOLO) { + return { decision: PolicyDecision.ALLOW }; + } + return { decision: PolicyDecision.ASK_USER }; + }, + }); + return mockConfig; } diff --git a/packages/cli/src/services/prompt-processors/shellProcessor.test.ts b/packages/cli/src/services/prompt-processors/shellProcessor.test.ts index 2c93ecf8c0..0f6fb562a8 100644 --- a/packages/cli/src/services/prompt-processors/shellProcessor.test.ts +++ b/packages/cli/src/services/prompt-processors/shellProcessor.test.ts @@ -9,7 +9,11 @@ import { ConfirmationRequiredError, ShellProcessor } from './shellProcessor.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import type { CommandContext } from '../../ui/commands/types.js'; import type { Config } from '@google/gemini-cli-core'; -import { ApprovalMode, getShellConfiguration } from '@google/gemini-cli-core'; +import { + ApprovalMode, + getShellConfiguration, + PolicyDecision, +} from '@google/gemini-cli-core'; import { quote } from 'shell-quote'; import { createPartFromText } from '@google/genai'; import type { PromptPipelineContent } from './types.js'; @@ -60,15 +64,23 @@ const SUCCESS_RESULT = { describe('ShellProcessor', () => { let context: CommandContext; let mockConfig: Partial; + let mockPolicyEngineCheck: Mock; beforeEach(() => { vi.clearAllMocks(); + mockPolicyEngineCheck = vi.fn().mockResolvedValue({ + decision: PolicyDecision.ALLOW, + }); + mockConfig = { getTargetDir: vi.fn().mockReturnValue('/test/dir'), getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT), getEnableInteractiveShell: vi.fn().mockReturnValue(false), getShellExecutionConfig: vi.fn().mockReturnValue({}), + getPolicyEngine: vi.fn().mockReturnValue({ + check: mockPolicyEngineCheck, + }), }; context = createMockCommandContext({ @@ -124,9 +136,8 @@ describe('ShellProcessor', () => { const prompt: PromptPipelineContent = createPromptPipelineContent( 'The current status is: !{git status}', ); - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: true, - disallowedCommands: [], + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ALLOW, }); mockShellExecute.mockReturnValue({ result: Promise.resolve({ ...SUCCESS_RESULT, output: 'On branch main' }), @@ -134,10 +145,12 @@ describe('ShellProcessor', () => { const result = await processor.process(prompt, context); - expect(mockCheckCommandPermissions).toHaveBeenCalledWith( - 'git status', - expect.any(Object), - context.session.sessionShellAllowlist, + expect(mockPolicyEngineCheck).toHaveBeenCalledWith( + { + name: 'run_shell_command', + args: { command: 'git status' }, + }, + undefined, ); expect(mockShellExecute).toHaveBeenCalledWith( 'git status', @@ -155,9 +168,8 @@ describe('ShellProcessor', () => { const prompt: PromptPipelineContent = createPromptPipelineContent( '!{git status} in !{pwd}', ); - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: true, - disallowedCommands: [], + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ALLOW, }); mockShellExecute @@ -173,7 +185,7 @@ describe('ShellProcessor', () => { const result = await processor.process(prompt, context); - expect(mockCheckCommandPermissions).toHaveBeenCalledTimes(2); + expect(mockPolicyEngineCheck).toHaveBeenCalledTimes(2); expect(mockShellExecute).toHaveBeenCalledTimes(2); expect(result).toEqual([{ text: 'On branch main in /usr/home' }]); }); @@ -183,9 +195,8 @@ describe('ShellProcessor', () => { const prompt: PromptPipelineContent = createPromptPipelineContent( 'Do something dangerous: !{rm -rf /}', ); - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: false, - disallowedCommands: ['rm -rf /'], + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ASK_USER, }); await expect(processor.process(prompt, context)).rejects.toThrow( @@ -198,11 +209,11 @@ describe('ShellProcessor', () => { const prompt: PromptPipelineContent = createPromptPipelineContent( 'Do something dangerous: !{rm -rf /}', ); - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: false, - disallowedCommands: ['rm -rf /'], + // In YOLO mode, PolicyEngine returns ALLOW + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ALLOW, }); - // Override the approval mode for this test + // Override the approval mode for this test (though PolicyEngine mock handles the decision) (mockConfig.getApprovalMode as Mock).mockReturnValue(ApprovalMode.YOLO); mockShellExecute.mockReturnValue({ result: Promise.resolve({ ...SUCCESS_RESULT, output: 'deleted' }), @@ -227,17 +238,14 @@ describe('ShellProcessor', () => { const prompt: PromptPipelineContent = createPromptPipelineContent( 'Do something forbidden: !{reboot}', ); - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: false, - disallowedCommands: ['reboot'], - isHardDenial: true, // This is the key difference - blockReason: 'System commands are blocked', + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.DENY, }); // Set approval mode to YOLO (mockConfig.getApprovalMode as Mock).mockReturnValue(ApprovalMode.YOLO); await expect(processor.process(prompt, context)).rejects.toThrow( - /Blocked command: "reboot". Reason: System commands are blocked/, + /Blocked command: "reboot". Reason: Blocked by policy/, ); // Ensure it never tried to execute @@ -249,9 +257,8 @@ describe('ShellProcessor', () => { const prompt: PromptPipelineContent = createPromptPipelineContent( 'Do something dangerous: !{rm -rf /}', ); - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: false, - disallowedCommands: ['rm -rf /'], + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ASK_USER, }); try { @@ -273,14 +280,12 @@ describe('ShellProcessor', () => { const prompt: PromptPipelineContent = createPromptPipelineContent( '!{cmd1} and !{cmd2}', ); - mockCheckCommandPermissions.mockImplementation((cmd) => { - if (cmd === 'cmd1') { - return { allAllowed: false, disallowedCommands: ['cmd1'] }; + mockPolicyEngineCheck.mockImplementation(async (toolCall) => { + const cmd = toolCall.args.command; + if (cmd === 'cmd1' || cmd === 'cmd2') { + return { decision: PolicyDecision.ASK_USER }; } - if (cmd === 'cmd2') { - return { allAllowed: false, disallowedCommands: ['cmd2'] }; - } - return { allAllowed: true, disallowedCommands: [] }; + return { decision: PolicyDecision.ALLOW }; }); try { @@ -301,11 +306,12 @@ describe('ShellProcessor', () => { 'First: !{echo "hello"}, Second: !{rm -rf /}', ); - mockCheckCommandPermissions.mockImplementation((cmd) => { + mockPolicyEngineCheck.mockImplementation(async (toolCall) => { + const cmd = toolCall.args.command; if (cmd.includes('rm')) { - return { allAllowed: false, disallowedCommands: [cmd] }; + return { decision: PolicyDecision.ASK_USER }; } - return { allAllowed: true, disallowedCommands: [] }; + return { decision: PolicyDecision.ALLOW }; }); await expect(processor.process(prompt, context)).rejects.toThrow( @@ -322,10 +328,13 @@ describe('ShellProcessor', () => { 'Allowed: !{ls -l}, Disallowed: !{rm -rf /}', ); - mockCheckCommandPermissions.mockImplementation((cmd) => ({ - allAllowed: !cmd.includes('rm'), - disallowedCommands: cmd.includes('rm') ? [cmd] : [], - })); + mockPolicyEngineCheck.mockImplementation(async (toolCall) => { + const cmd = toolCall.args.command; + if (cmd.includes('rm')) { + return { decision: PolicyDecision.ASK_USER }; + } + return { decision: PolicyDecision.ALLOW }; + }); try { await processor.process(prompt, context); @@ -344,13 +353,12 @@ describe('ShellProcessor', () => { 'Run !{cmd1} and !{cmd2}', ); - // Add commands to the session allowlist + // Add commands to the session allowlist (conceptually, in this test we just mock the engine allowing them) context.session.sessionShellAllowlist = new Set(['cmd1', 'cmd2']); // checkCommandPermissions should now pass for these - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: true, - disallowedCommands: [], + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ALLOW, }); mockShellExecute @@ -363,20 +371,58 @@ describe('ShellProcessor', () => { const result = await processor.process(prompt, context); - expect(mockCheckCommandPermissions).toHaveBeenCalledWith( - 'cmd1', - expect.any(Object), - context.session.sessionShellAllowlist, - ); - expect(mockCheckCommandPermissions).toHaveBeenCalledWith( - 'cmd2', - expect.any(Object), - context.session.sessionShellAllowlist, - ); + expect(mockPolicyEngineCheck).not.toHaveBeenCalled(); expect(mockShellExecute).toHaveBeenCalledTimes(2); expect(result).toEqual([{ text: 'Run output1 and output2' }]); }); + it('should support the full confirmation flow (Ask -> Approve -> Retry)', async () => { + // 1. Initial State: Command NOT allowed + const processor = new ShellProcessor('test-command'); + const prompt: PromptPipelineContent = + createPromptPipelineContent('!{echo "once"}'); + + // Policy Engine says ASK_USER + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ASK_USER, + }); + + // 2. First Attempt: processing should fail with ConfirmationRequiredError + try { + await processor.process(prompt, context); + expect.fail('Should have thrown ConfirmationRequiredError'); + } catch (e) { + expect(e).toBeInstanceOf(ConfirmationRequiredError); + expect(mockPolicyEngineCheck).toHaveBeenCalledTimes(1); + } + + // 3. User Approves: Add to session allowlist (simulating UI action) + context.session.sessionShellAllowlist.add('echo "once"'); + + // 4. Retry: calling process() again with the same context + // Reset mocks to ensure we track new calls cleanly + mockPolicyEngineCheck.mockClear(); + + // Mock successful execution + mockShellExecute.mockReturnValue({ + result: Promise.resolve({ ...SUCCESS_RESULT, output: 'once' }), + }); + + const result = await processor.process(prompt, context); + + // 5. Verify Success AND Policy Engine Bypass + expect(mockPolicyEngineCheck).not.toHaveBeenCalled(); + expect(mockShellExecute).toHaveBeenCalledWith( + 'echo "once"', + expect.any(String), + expect.any(Function), + expect.any(Object), + false, + expect.any(Object), + ); + expect(result).toEqual([{ text: 'once' }]); + }); + it('should trim whitespace from the command inside the injection before interpolation', async () => { const processor = new ShellProcessor('test-command'); const prompt: PromptPipelineContent = createPromptPipelineContent( @@ -389,9 +435,8 @@ describe('ShellProcessor', () => { const expectedCommand = `ls ${expectedEscapedArgs} -l`; - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: true, - disallowedCommands: [], + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ALLOW, }); mockShellExecute.mockReturnValue({ result: Promise.resolve({ ...SUCCESS_RESULT, output: 'total 0' }), @@ -399,10 +444,9 @@ describe('ShellProcessor', () => { await processor.process(prompt, context); - expect(mockCheckCommandPermissions).toHaveBeenCalledWith( - expectedCommand, - expect.any(Object), - context.session.sessionShellAllowlist, + expect(mockPolicyEngineCheck).toHaveBeenCalledWith( + { name: 'run_shell_command', args: { command: expectedCommand } }, + undefined, ); expect(mockShellExecute).toHaveBeenCalledWith( expectedCommand, @@ -421,7 +465,7 @@ describe('ShellProcessor', () => { const result = await processor.process(prompt, context); - expect(mockCheckCommandPermissions).not.toHaveBeenCalled(); + expect(mockPolicyEngineCheck).not.toHaveBeenCalled(); expect(mockShellExecute).not.toHaveBeenCalled(); // It replaces !{} with an empty string. @@ -615,20 +659,20 @@ describe('ShellProcessor', () => { const expectedEscapedArgs = getExpectedEscapedArgForPlatform(rawArgs); const expectedResolvedCommand = `rm ${expectedEscapedArgs}`; - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: false, - disallowedCommands: [expectedResolvedCommand], - isHardDenial: false, + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.ASK_USER, }); await expect(processor.process(prompt, context)).rejects.toThrow( ConfirmationRequiredError, ); - expect(mockCheckCommandPermissions).toHaveBeenCalledWith( - expectedResolvedCommand, - expect.any(Object), - context.session.sessionShellAllowlist, + expect(mockPolicyEngineCheck).toHaveBeenCalledWith( + { + name: 'run_shell_command', + args: { command: expectedResolvedCommand }, + }, + undefined, ); }); @@ -638,15 +682,12 @@ describe('ShellProcessor', () => { createPromptPipelineContent('!{rm {{args}}}'); const expectedEscapedArgs = getExpectedEscapedArgForPlatform(rawArgs); const expectedResolvedCommand = `rm ${expectedEscapedArgs}`; - mockCheckCommandPermissions.mockReturnValue({ - allAllowed: false, - disallowedCommands: [expectedResolvedCommand], - isHardDenial: true, - blockReason: 'It is forbidden.', + mockPolicyEngineCheck.mockResolvedValue({ + decision: PolicyDecision.DENY, }); await expect(processor.process(prompt, context)).rejects.toThrow( - `Blocked command: "${expectedResolvedCommand}". Reason: It is forbidden.`, + `Blocked command: "${expectedResolvedCommand}". Reason: Blocked by policy.`, ); }); }); diff --git a/packages/cli/src/services/prompt-processors/shellProcessor.ts b/packages/cli/src/services/prompt-processors/shellProcessor.ts index 350421c1c5..4c8369f664 100644 --- a/packages/cli/src/services/prompt-processors/shellProcessor.ts +++ b/packages/cli/src/services/prompt-processors/shellProcessor.ts @@ -5,12 +5,11 @@ */ import { - ApprovalMode, - checkCommandPermissions, escapeShellArg, getShellConfiguration, ShellExecutionService, flatMapTextParts, + PolicyDecision, } from '@google/gemini-cli-core'; import type { CommandContext } from '../../ui/commands/types.js'; @@ -81,7 +80,6 @@ export class ShellProcessor implements IPromptProcessor { `Security configuration not loaded. Cannot verify shell command permissions for '${this.commandName}'. Aborting.`, ); } - const { sessionShellAllowlist } = context.session; const injections = extractInjections( prompt, @@ -121,21 +119,25 @@ export class ShellProcessor implements IPromptProcessor { if (!command) continue; + if (context.session.sessionShellAllowlist?.has(command)) { + continue; + } + // Security check on the final, escaped command string. - const { allAllowed, disallowedCommands, blockReason, isHardDenial } = - checkCommandPermissions(command, config, sessionShellAllowlist); + const { decision } = await config.getPolicyEngine().check( + { + name: 'run_shell_command', + args: { command }, + }, + undefined, + ); - if (!allAllowed) { - if (isHardDenial) { - throw new Error( - `${this.commandName} cannot be run. Blocked command: "${command}". Reason: ${blockReason || 'Blocked by configuration.'}`, - ); - } - - // If not a hard denial, respect YOLO mode and auto-approve. - if (config.getApprovalMode() !== ApprovalMode.YOLO) { - disallowedCommands.forEach((uc) => commandsToConfirm.add(uc)); - } + if (decision === PolicyDecision.DENY) { + throw new Error( + `${this.commandName} cannot be run. Blocked command: "${command}". Reason: Blocked by policy.`, + ); + } else if (decision === PolicyDecision.ASK_USER) { + commandsToConfirm.add(command); } } diff --git a/packages/cli/src/ui/hooks/useToolScheduler.test.ts b/packages/cli/src/ui/hooks/useToolScheduler.test.ts index 7015e6afea..1ffaa61cc7 100644 --- a/packages/cli/src/ui/hooks/useToolScheduler.test.ts +++ b/packages/cli/src/ui/hooks/useToolScheduler.test.ts @@ -33,6 +33,7 @@ import { ApprovalMode, HookSystem, PREVIEW_GEMINI_MODEL, + PolicyDecision, } from '@google/gemini-cli-core'; import { MockTool } from '@google/gemini-cli-core/src/test-utils/mock-tool.js'; import { createMockMessageBus } from '@google/gemini-cli-core/src/test-utils/mock-message-bus.js'; @@ -80,13 +81,21 @@ const mockConfig = { getGeminiClient: () => null, // No client needed for these tests getShellExecutionConfig: () => ({ terminalWidth: 80, terminalHeight: 24 }), getMessageBus: () => null, - getPolicyEngine: () => null, isInteractive: () => false, getExperiments: () => {}, getEnableHooks: () => false, } as unknown as Config; mockConfig.getMessageBus = vi.fn().mockReturnValue(createMockMessageBus()); mockConfig.getHookSystem = vi.fn().mockReturnValue(new HookSystem(mockConfig)); +mockConfig.getPolicyEngine = vi.fn().mockReturnValue({ + check: async () => { + const mode = mockConfig.getApprovalMode(); + if (mode === ApprovalMode.YOLO) { + return { decision: PolicyDecision.ALLOW }; + } + return { decision: PolicyDecision.ASK_USER }; + }, +}); function createMockConfigOverride(overrides: Partial = {}): Config { return { ...mockConfig, ...overrides } as Config; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 5859de2133..10c06950b8 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -84,7 +84,7 @@ import { FileExclusions } from '../utils/ignorePatterns.js'; import type { EventEmitter } from 'node:events'; import { MessageBus } from '../confirmation-bus/message-bus.js'; import { PolicyEngine } from '../policy/policy-engine.js'; -import type { PolicyEngineConfig } from '../policy/types.js'; +import { ApprovalMode, type PolicyEngineConfig } from '../policy/types.js'; import { HookSystem } from '../hooks/index.js'; import type { UserTierId } from '../code_assist/types.js'; import type { RetrieveUserQuotaResponse } from '../code_assist/types.js'; @@ -101,8 +101,6 @@ import { debugLogger } from '../utils/debugLogger.js'; import { SkillManager, type SkillDefinition } from '../skills/skillManager.js'; import { startupProfiler } from '../telemetry/startupProfiler.js'; -import { ApprovalMode } from '../policy/types.js'; - export interface AccessibilitySettings { disableLoadingPhrases?: boolean; screenReader?: boolean; diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index ba4e22506b..22ef939a62 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -19,7 +19,6 @@ import type { ToolResult, Config, ToolRegistry, - AnyToolInvocation, MessageBus, } from '../index.js'; import { @@ -31,6 +30,7 @@ import { Kind, ApprovalMode, HookSystem, + PolicyDecision, } from '../index.js'; import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; import { @@ -39,8 +39,8 @@ import { MOCK_TOOL_SHOULD_CONFIRM_EXECUTE, } from '../test-utils/mock-tool.js'; import * as modifiableToolModule from '../tools/modifiable-tool.js'; -import { isShellInvocationAllowlisted } from '../utils/shell-permissions.js'; import { DEFAULT_GEMINI_MODEL } from '../config/models.js'; +import type { PolicyEngine } from '../policy/policy-engine.js'; vi.mock('fs/promises', () => ({ writeFile: vi.fn(), @@ -274,11 +274,35 @@ function createMockConfig(overrides: Partial = {}): Config { getGeminiClient: () => null, getMessageBus: () => createMockMessageBus(), getEnableHooks: () => false, - getPolicyEngine: () => null, getExperiments: () => {}, } as unknown as Config; - return { ...baseConfig, ...overrides } as Config; + const finalConfig = { ...baseConfig, ...overrides } as Config; + + // Patch the policy engine to use the final config if not overridden + if (!overrides.getPolicyEngine) { + finalConfig.getPolicyEngine = () => + ({ + check: async (toolCall: { name: string; args: object }) => { + // Mock simple policy logic for tests + const mode = finalConfig.getApprovalMode(); + if (mode === ApprovalMode.YOLO) { + return { decision: PolicyDecision.ALLOW }; + } + const allowed = finalConfig.getAllowedTools(); + if ( + allowed && + (allowed.includes(toolCall.name) || + allowed.some((p) => toolCall.name.startsWith(p))) + ) { + return { decision: PolicyDecision.ALLOW }; + } + return { decision: PolicyDecision.ASK_USER }; + }, + }) as unknown as PolicyEngine; + } + + return finalConfig; } describe('CoreToolScheduler', () => { @@ -570,7 +594,7 @@ describe('CoreToolScheduler', () => { const mockConfig = createMockConfig({ getToolRegistry: () => mockToolRegistry, - isInteractive: () => false, + isInteractive: () => true, }); const scheduler = new CoreToolScheduler({ @@ -1192,15 +1216,6 @@ describe('CoreToolScheduler request queueing', () => { }); it('should require approval for a chained shell command even when prefix is allowlisted', async () => { - expect( - isShellInvocationAllowlisted( - { - params: { command: 'git status && rm -rf /tmp/should-not-run' }, - } as unknown as AnyToolInvocation, - ['run_shell_command(git)'], - ), - ).toBe(false); - const executeFn = vi.fn().mockResolvedValue({ llmContent: 'Shell command executed', returnDisplay: 'Shell command executed', @@ -1249,6 +1264,10 @@ describe('CoreToolScheduler request queueing', () => { }), getToolRegistry: () => toolRegistry, getHookSystem: () => undefined, + getPolicyEngine: () => + ({ + check: async () => ({ decision: PolicyDecision.ASK_USER }), + }) as unknown as PolicyEngine, }); const scheduler = new CoreToolScheduler({ diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 20afc07b2c..69a2e03475 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -14,7 +14,7 @@ import { } from '../tools/tools.js'; import type { EditorType } from '../utils/editor.js'; import type { Config } from '../config/config.js'; -import { ApprovalMode } from '../policy/types.js'; +import { PolicyDecision } from '../policy/types.js'; import { logToolCall } from '../telemetry/loggers.js'; import { ToolErrorType } from '../tools/tool-error.js'; import { ToolCallEvent } from '../telemetry/types.js'; @@ -25,12 +25,7 @@ import { modifyWithEditor, } from '../tools/modifiable-tool.js'; import * as Diff from 'diff'; -import { SHELL_TOOL_NAMES } from '../utils/shell-utils.js'; -import { - doesToolInvocationMatch, - getToolSuggestion, -} from '../utils/tool-utils.js'; -import { isShellInvocationAllowlisted } from '../utils/shell-permissions.js'; +import { getToolSuggestion } from '../utils/tool-utils.js'; import type { ToolConfirmationRequest } from '../confirmation-bus/types.js'; import { MessageBusType } from '../confirmation-bus/types.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; @@ -592,17 +587,46 @@ export class CoreToolScheduler { return; } - const confirmationDetails = - await invocation.shouldConfirmExecute(signal); + // Policy Check using PolicyEngine + // We must reconstruct the FunctionCall format expected by PolicyEngine + const toolCallForPolicy = { + name: toolCall.request.name, + args: toolCall.request.args, + }; + const { decision } = await this.config + .getPolicyEngine() + .check(toolCallForPolicy, undefined); // Server name undefined for local tools - if (!confirmationDetails) { + if (decision === PolicyDecision.DENY) { + const errorMessage = `Tool execution denied by policy.`; + this.setStatusInternal( + reqInfo.callId, + 'error', + signal, + createErrorResponse( + reqInfo, + new Error(errorMessage), + ToolErrorType.POLICY_VIOLATION, + ), + ); + await this.checkAndNotifyCompletion(signal); + return; + } + + if (decision === PolicyDecision.ALLOW) { this.setToolCallOutcome( reqInfo.callId, ToolConfirmationOutcome.ProceedAlways, ); this.setStatusInternal(reqInfo.callId, 'scheduled', signal); } else { - if (this.isAutoApproved(toolCall)) { + // PolicyDecision.ASK_USER + + // We need confirmation details to show to the user + const confirmationDetails = + await invocation.shouldConfirmExecute(signal); + + if (!confirmationDetails) { this.setToolCallOutcome( reqInfo.callId, ToolConfirmationOutcome.ProceedAlways, @@ -616,6 +640,7 @@ export class CoreToolScheduler { }" requires user confirmation, which is not supported in non-interactive mode.`, ); } + // Fire Notification hook before showing confirmation to user const messageBus = this.config.getMessageBus(); const hooksEnabled = this.config.getEnableHooks(); @@ -1014,20 +1039,4 @@ export class CoreToolScheduler { }; }); } - - private isAutoApproved(toolCall: ValidatingToolCall): boolean { - if (this.config.getApprovalMode() === ApprovalMode.YOLO) { - return true; - } - - const allowedTools = this.config.getAllowedTools() || []; - const { tool, invocation } = toolCall; - const toolName = typeof tool === 'string' ? tool : tool.name; - - if (SHELL_TOOL_NAMES.includes(toolName)) { - return isShellInvocationAllowlisted(invocation, allowedTools); - } - - return doesToolInvocationMatch(tool, invocation, allowedTools); - } } diff --git a/packages/core/src/core/nonInteractiveToolExecutor.test.ts b/packages/core/src/core/nonInteractiveToolExecutor.test.ts index a903bfdfa1..7753923d88 100644 --- a/packages/core/src/core/nonInteractiveToolExecutor.test.ts +++ b/packages/core/src/core/nonInteractiveToolExecutor.test.ts @@ -20,6 +20,7 @@ import { ApprovalMode, HookSystem, PREVIEW_GEMINI_MODEL, + PolicyDecision, } from '../index.js'; import type { Part } from '@google/genai'; import { MockTool } from '../test-utils/mock-tool.js'; @@ -65,7 +66,9 @@ describe('executeToolCall', () => { getActiveModel: () => PREVIEW_GEMINI_MODEL, getGeminiClient: () => null, // No client needed for these tests getMessageBus: () => null, - getPolicyEngine: () => null, + getPolicyEngine: () => ({ + check: async () => ({ decision: PolicyDecision.ALLOW }), + }), isInteractive: () => false, getExperiments: () => {}, getEnableHooks: () => false, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2f11c4ae71..a15ee2951d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -67,7 +67,8 @@ export * from './utils/googleQuotaErrors.js'; export * from './utils/fileUtils.js'; export * from './utils/retry.js'; export * from './utils/shell-utils.js'; -export * from './utils/shell-permissions.js'; +export { PolicyDecision, ApprovalMode } from './policy/types.js'; +export * from './utils/tool-utils.js'; export * from './utils/terminalSerializer.js'; export * from './utils/systemEncoding.js'; export * from './utils/textUtils.js'; diff --git a/packages/core/src/policy/persistence.test.ts b/packages/core/src/policy/persistence.test.ts index 479ae8707c..22f00ac9a8 100644 --- a/packages/core/src/policy/persistence.test.ts +++ b/packages/core/src/policy/persistence.test.ts @@ -127,7 +127,7 @@ describe('createPolicyUpdater', () => { expect(addedRule).toBeDefined(); expect(addedRule?.priority).toBe(2.95); expect(addedRule?.argsPattern).toEqual( - new RegExp(`"command":"git\\ status(?:[\\s"]|$)`), + new RegExp(`"command":"git\\ status(?:[\\s"]|\\\\")`), ); // Verify file written diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index e5add3748a..aa6b7ac887 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -72,14 +72,14 @@ describe('createPolicyUpdater', () => { 1, expect.objectContaining({ toolName: 'run_shell_command', - argsPattern: new RegExp('"command":"echo(?:[\\s"]|$)'), + argsPattern: new RegExp('"command":"echo(?:[\\s"]|\\\\")'), }), ); expect(policyEngine.addRule).toHaveBeenNthCalledWith( 2, expect.objectContaining({ toolName: 'run_shell_command', - argsPattern: new RegExp('"command":"ls(?:[\\s"]|$)'), + argsPattern: new RegExp('"command":"ls(?:[\\s"]|\\\\")'), }), ); }); @@ -98,7 +98,7 @@ describe('createPolicyUpdater', () => { expect(policyEngine.addRule).toHaveBeenCalledWith( expect.objectContaining({ toolName: 'run_shell_command', - argsPattern: new RegExp('"command":"git(?:[\\s"]|$)'), + argsPattern: new RegExp('"command":"git(?:[\\s"]|\\\\")'), }), ); }); diff --git a/packages/core/src/policy/utils.test.ts b/packages/core/src/policy/utils.test.ts index 991cd28eed..dfbb8b298c 100644 --- a/packages/core/src/policy/utils.test.ts +++ b/packages/core/src/policy/utils.test.ts @@ -31,14 +31,14 @@ 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(['"command":"ls(?:[\\s"]|\\\\")']); }); it('should build patterns from an array of commandPrefixes', () => { const result = buildArgsPatterns(undefined, ['ls', 'cd'], undefined); expect(result).toEqual([ - '"command":"ls(?:[\\s"]|$)', - '"command":"cd(?:[\\s"]|$)', + '"command":"ls(?:[\\s"]|\\\\")', + '"command":"cd(?:[\\s"]|\\\\")', ]); }); @@ -49,7 +49,7 @@ describe('policy/utils', () => { it('should prioritize commandPrefix over commandRegex and argsPattern', () => { const result = buildArgsPatterns('raw', 'prefix', 'regex'); - expect(result).toEqual(['"command":"prefix(?:[\\s"]|$)']); + expect(result).toEqual(['"command":"prefix(?:[\\s"]|\\\\")']); }); it('should prioritize commandRegex over argsPattern if no commandPrefix', () => { @@ -59,13 +59,15 @@ describe('policy/utils', () => { it('should escape characters in commandPrefix', () => { const result = buildArgsPatterns(undefined, 'git checkout -b', undefined); - expect(result).toEqual(['"command":"git\\ checkout\\ \\-b(?:[\\s"]|$)']); + expect(result).toEqual([ + '"command":"git\\ checkout\\ \\-b(?:[\\s"]|\\\\")', + ]); }); it('should correctly escape quotes in commandPrefix', () => { const result = buildArgsPatterns(undefined, 'git "fix"', undefined); expect(result).toEqual([ - '"command":"git\\ \\\\\\"fix\\\\\\"(?:[\\s"]|$)', + '"command":"git\\ \\\\\\"fix\\\\\\"(?:[\\s"]|\\\\")', ]); }); @@ -73,5 +75,36 @@ describe('policy/utils', () => { const result = buildArgsPatterns(undefined, undefined, undefined); expect(result).toEqual([undefined]); }); + + it('should match prefixes followed by JSON escaped quotes', () => { + // Testing the security fix logic: allowing "echo \"foo\"" + const prefix = 'echo '; + 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\\""}'; + expect(regex.test(validJsonArgs)).toBe(true); + }); + + it('should NOT match prefixes followed by raw backslashes (security check)', () => { + // Testing that we blocked the hole: "echo\foo" + const prefix = 'echo '; + const patterns = buildArgsPatterns(undefined, prefix, undefined); + 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"}'; + 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"}'; + expect(gitRegex.test(gitAttack)).toBe(false); + }); }); }); diff --git a/packages/core/src/policy/utils.ts b/packages/core/src/policy/utils.ts index 0052e90035..b891a8fda1 100644 --- a/packages/core/src/policy/utils.ts +++ b/packages/core/src/policy/utils.ts @@ -38,7 +38,10 @@ export function buildArgsPatterns( // always followed by a space or a closing quote. return prefixes.map((prefix) => { const jsonPrefix = JSON.stringify(prefix).slice(1, -1); - return `"command":"${escapeRegex(jsonPrefix)}(?:[\\s"]|$)`; + // We allow [\s], ["], or the specific sequence [\"] (for escaped quotes + // in JSON). We do NOT allow generic [\\], which would match "git\status" + // -> "gitstatus". + return `"command":"${escapeRegex(jsonPrefix)}(?:[\\s"]|\\\\")`; }); } diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index 4102f1a490..f29470b780 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -12,6 +12,10 @@ * - Fatal: System-level issues that prevent continued execution (e.g., disk full, critical I/O errors) */ export enum ToolErrorType { + POLICY_VIOLATION = 'policy_violation', + /** + * General tool execution failure (e.g. file system error, API error). + */ // General Errors INVALID_TOOL_PARAMS = 'invalid_tool_params', UNKNOWN = 'unknown', diff --git a/packages/core/src/utils/shell-permissions.test.ts b/packages/core/src/utils/shell-permissions.test.ts deleted file mode 100644 index a80afdbd7a..0000000000 --- a/packages/core/src/utils/shell-permissions.test.ts +++ /dev/null @@ -1,551 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { - expect, - describe, - it, - beforeEach, - beforeAll, - vi, - afterEach, -} from 'vitest'; -import { initializeShellParsers } from './shell-utils.js'; -import { - checkCommandPermissions, - isCommandAllowed, - isShellInvocationAllowlisted, -} from './shell-permissions.js'; -import type { Config } from '../config/config.js'; -import type { AnyToolInvocation } from '../index.js'; - -const mockPlatform = vi.hoisted(() => vi.fn()); -const mockHomedir = vi.hoisted(() => vi.fn()); -vi.mock('os', () => ({ - default: { - platform: mockPlatform, - homedir: mockHomedir, - }, - platform: mockPlatform, - homedir: mockHomedir, -})); - -const mockSpawnSync = vi.hoisted(() => vi.fn()); -vi.mock('node:child_process', async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - spawnSync: mockSpawnSync, - }; -}); - -const mockQuote = vi.hoisted(() => vi.fn()); -vi.mock('shell-quote', () => ({ - quote: mockQuote, -})); - -let config: Config; -const isWindowsRuntime = process.platform === 'win32'; -const describeWindowsOnly = isWindowsRuntime ? describe : describe.skip; - -beforeAll(async () => { - mockPlatform.mockReturnValue('linux'); - await initializeShellParsers(); -}); - -beforeEach(() => { - mockPlatform.mockReturnValue('linux'); - mockQuote.mockImplementation((args: string[]) => - args.map((arg) => `'${arg}'`).join(' '), - ); - config = { - getCoreTools: () => [], - getExcludeTools: () => new Set([]), - getAllowedTools: () => [], - getApprovalMode: () => 'strict', - isInteractive: () => false, - } as unknown as Config; -}); - -afterEach(() => { - vi.clearAllMocks(); -}); - -describe('isCommandAllowed', () => { - it('should allow a command if no restrictions are provided', () => { - const result = isCommandAllowed('goodCommand --safe', config); - expect(result.allowed).toBe(true); - }); - - it('should allow a command if it is in the global allowlist', () => { - config.getCoreTools = () => ['ShellTool(goodCommand)']; - const result = isCommandAllowed('goodCommand --safe', config); - expect(result.allowed).toBe(true); - }); - - it('should block a command if it is not in a strict global allowlist', () => { - config.getCoreTools = () => ['ShellTool(goodCommand --safe)']; - const result = isCommandAllowed('badCommand --danger', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "badCommand --danger"`, - ); - }); - - it('should block a command if it is in the blocked list', () => { - config.getExcludeTools = () => new Set(['ShellTool(badCommand --danger)']); - const result = isCommandAllowed('badCommand --danger', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command 'badCommand --danger' is blocked by configuration`, - ); - }); - - it('should prioritize the blocklist over the allowlist', () => { - config.getCoreTools = () => ['ShellTool(badCommand --danger)']; - config.getExcludeTools = () => new Set(['ShellTool(badCommand --danger)']); - const result = isCommandAllowed('badCommand --danger', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command 'badCommand --danger' is blocked by configuration`, - ); - }); - - it('should allow any command when a wildcard is in coreTools', () => { - config.getCoreTools = () => ['ShellTool']; - const result = isCommandAllowed('any random command', config); - expect(result.allowed).toBe(true); - }); - - it('should block any command when a wildcard is in excludeTools', () => { - config.getExcludeTools = () => new Set(['run_shell_command']); - const result = isCommandAllowed('any random command', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - 'Shell tool is globally disabled in configuration', - ); - }); - - it('should block a command on the blocklist even with a wildcard allow', () => { - config.getCoreTools = () => ['ShellTool']; - config.getExcludeTools = () => new Set(['ShellTool(badCommand --danger)']); - const result = isCommandAllowed('badCommand --danger', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command 'badCommand --danger' is blocked by configuration`, - ); - }); - - it('should allow a chained command if all parts are on the global allowlist', () => { - config.getCoreTools = () => [ - 'run_shell_command(echo)', - 'run_shell_command(goodCommand)', - ]; - const result = isCommandAllowed( - 'echo "hello" && goodCommand --safe', - config, - ); - expect(result.allowed).toBe(true); - }); - - it('should block a chained command if any part is blocked', () => { - config.getExcludeTools = () => new Set(['run_shell_command(badCommand)']); - const result = isCommandAllowed( - 'echo "hello" && badCommand --danger', - config, - ); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command 'badCommand --danger' is blocked by configuration`, - ); - }); - - it('should block a command that redefines an allowed function to run an unlisted command', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed( - 'echo () (curl google.com) ; echo Hello World', - config, - ); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block a multi-line function body that runs an unlisted command', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed( - `echo () { - curl google.com -} ; echo ok`, - config, - ); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block a function keyword declaration that runs an unlisted command', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed( - 'function echo { curl google.com; } ; echo hi', - config, - ); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block command substitution that invokes an unlisted command', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed('echo $(curl google.com)', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block pipelines that invoke an unlisted command', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed('echo hi | curl google.com', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block background jobs that invoke an unlisted command', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed('echo hi & curl google.com', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block command substitution inside a here-document when the inner command is unlisted', () => { - config.getCoreTools = () => [ - 'run_shell_command(echo)', - 'run_shell_command(cat)', - ]; - const result = isCommandAllowed( - `cat < { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed('echo `curl google.com`', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block process substitution using <() when the inner command is unlisted', () => { - config.getCoreTools = () => [ - 'run_shell_command(diff)', - 'run_shell_command(echo)', - ]; - const result = isCommandAllowed( - 'diff <(curl google.com) <(echo safe)', - config, - ); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block process substitution using >() when the inner command is unlisted', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = isCommandAllowed('echo "data" > >(curl google.com)', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - `Command(s) not in the allowed commands list. Disallowed commands: "curl google.com"`, - ); - }); - - it('should block commands containing prompt transformations', () => { - const result = isCommandAllowed( - 'echo "${var1=aa\\140 env| ls -l\\140}${var1@P}"', - config, - ); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - 'Command rejected because it could not be parsed safely', - ); - }); - - it('should block simple prompt transformation expansions', () => { - const result = isCommandAllowed('echo ${foo@P}', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - 'Command rejected because it could not be parsed safely', - ); - }); - - describe('command substitution', () => { - it('should allow command substitution using `$(...)`', () => { - const result = isCommandAllowed('echo $(goodCommand --safe)', config); - expect(result.allowed).toBe(true); - expect(result.reason).toBeUndefined(); - }); - - it('should allow command substitution using `<(...)`', () => { - const result = isCommandAllowed('diff <(ls) <(ls -a)', config); - expect(result.allowed).toBe(true); - expect(result.reason).toBeUndefined(); - }); - - it('should allow command substitution using `>(...)`', () => { - const result = isCommandAllowed( - 'echo "Log message" > >(tee log.txt)', - config, - ); - expect(result.allowed).toBe(true); - expect(result.reason).toBeUndefined(); - }); - - it('should allow command substitution using backticks', () => { - const result = isCommandAllowed('echo `goodCommand --safe`', config); - expect(result.allowed).toBe(true); - expect(result.reason).toBeUndefined(); - }); - - it('should allow substitution-like patterns inside single quotes', () => { - config.getCoreTools = () => ['ShellTool(echo)']; - const result = isCommandAllowed("echo '$(pwd)'", config); - expect(result.allowed).toBe(true); - }); - - it('should block a command when parsing fails', () => { - const result = isCommandAllowed('ls &&', config); - expect(result.allowed).toBe(false); - expect(result.reason).toBe( - 'Command rejected because it could not be parsed safely', - ); - }); - }); -}); - -describe('checkCommandPermissions', () => { - describe('in "Default Allow" mode (no sessionAllowlist)', () => { - it('should return a detailed success object for an allowed command', () => { - const result = checkCommandPermissions('goodCommand --safe', config); - expect(result).toEqual({ - allAllowed: true, - disallowedCommands: [], - }); - }); - - it('should block commands that cannot be parsed safely', () => { - const result = checkCommandPermissions('ls &&', config); - expect(result).toEqual({ - allAllowed: false, - disallowedCommands: ['ls &&'], - blockReason: 'Command rejected because it could not be parsed safely', - isHardDenial: true, - }); - }); - - it('should return a detailed failure object for a blocked command', () => { - config.getExcludeTools = () => new Set(['ShellTool(badCommand)']); - const result = checkCommandPermissions('badCommand --danger', config); - expect(result).toEqual({ - allAllowed: false, - disallowedCommands: ['badCommand --danger'], - blockReason: `Command 'badCommand --danger' is blocked by configuration`, - isHardDenial: true, - }); - }); - - it('should return a detailed failure object for a command not on a strict allowlist', () => { - config.getCoreTools = () => ['ShellTool(goodCommand)']; - const result = checkCommandPermissions( - 'git status && goodCommand', - config, - ); - expect(result).toEqual({ - allAllowed: false, - disallowedCommands: ['git status'], - blockReason: `Command(s) not in the allowed commands list. Disallowed commands: "git status"`, - isHardDenial: false, - }); - }); - }); - - describe('in "Default Deny" mode (with sessionAllowlist)', () => { - it('should allow a command on the sessionAllowlist', () => { - const result = checkCommandPermissions( - 'goodCommand --safe', - config, - new Set(['goodCommand --safe']), - ); - expect(result.allAllowed).toBe(true); - }); - - it('should block a command not on the sessionAllowlist or global allowlist', () => { - const result = checkCommandPermissions( - 'badCommand --danger', - config, - new Set(['goodCommand --safe']), - ); - expect(result.allAllowed).toBe(false); - expect(result.blockReason).toContain( - 'not on the global or session allowlist', - ); - expect(result.disallowedCommands).toEqual(['badCommand --danger']); - }); - - it('should allow a command on the global allowlist even if not on the session allowlist', () => { - config.getCoreTools = () => ['ShellTool(git status)']; - const result = checkCommandPermissions( - 'git status', - config, - new Set(['goodCommand --safe']), - ); - expect(result.allAllowed).toBe(true); - }); - - it('should allow a chained command if parts are on different allowlists', () => { - config.getCoreTools = () => ['ShellTool(git status)']; - const result = checkCommandPermissions( - 'git status && git commit', - config, - new Set(['git commit']), - ); - expect(result.allAllowed).toBe(true); - }); - - it('should block a command on the sessionAllowlist if it is also globally blocked', () => { - config.getExcludeTools = () => new Set(['run_shell_command(badCommand)']); - const result = checkCommandPermissions( - 'badCommand --danger', - config, - new Set(['badCommand --danger']), - ); - expect(result.allAllowed).toBe(false); - expect(result.blockReason).toContain('is blocked by configuration'); - }); - - it('should block a chained command if one part is not on any allowlist', () => { - config.getCoreTools = () => ['run_shell_command(echo)']; - const result = checkCommandPermissions( - 'echo "hello" && badCommand --danger', - config, - new Set(['echo']), - ); - expect(result.allAllowed).toBe(false); - expect(result.disallowedCommands).toEqual(['badCommand --danger']); - }); - }); -}); - -describeWindowsOnly('PowerShell integration', () => { - const originalComSpec = process.env['ComSpec']; - - beforeEach(() => { - mockPlatform.mockReturnValue('win32'); - const systemRoot = process.env['SystemRoot'] || 'C:\\Windows'; - process.env['ComSpec'] = - `${systemRoot}\\System32\\WindowsPowerShell\\v1.0\\powershell.exe`; - }); - - afterEach(() => { - if (originalComSpec === undefined) { - delete process.env['ComSpec']; - } else { - process.env['ComSpec'] = originalComSpec; - } - }); - - it('should block commands when PowerShell parser reports errors', () => { - // Mock spawnSync to avoid the overhead of spawning a real PowerShell process, - // which can lead to timeouts in CI environments even on Windows. - mockSpawnSync.mockReturnValue({ - status: 0, - stdout: JSON.stringify({ success: false }), - }); - - const { allowed, reason } = isCommandAllowed('Get-ChildItem |', config); - expect(allowed).toBe(false); - expect(reason).toBe( - 'Command rejected because it could not be parsed safely', - ); - }); - - it('should allow valid commands through PowerShell parser', () => { - // Mock spawnSync to avoid the overhead of spawning a real PowerShell process, - // which can lead to timeouts in CI environments even on Windows. - mockSpawnSync.mockReturnValue({ - status: 0, - stdout: JSON.stringify({ - success: true, - commands: [{ name: 'Get-ChildItem', text: 'Get-ChildItem' }], - }), - }); - - const { allowed } = isCommandAllowed('Get-ChildItem', config); - expect(allowed).toBe(true); - }); -}); - -describe('isShellInvocationAllowlisted', () => { - function createInvocation(command: string): AnyToolInvocation { - return { params: { command } } as unknown as AnyToolInvocation; - } - - it('should return false when any chained command segment is not allowlisted', () => { - const invocation = createInvocation( - 'git status && rm -rf /tmp/should-not-run', - ); - expect( - isShellInvocationAllowlisted(invocation, ['run_shell_command(git)']), - ).toBe(false); - }); - - it('should return true when every segment is explicitly allowlisted', () => { - const invocation = createInvocation( - 'git status && rm -rf /tmp/should-run && git diff', - ); - expect( - isShellInvocationAllowlisted(invocation, [ - 'run_shell_command(git)', - 'run_shell_command(rm -rf)', - ]), - ).toBe(true); - }); - - it('should return true when the allowlist contains a wildcard shell entry', () => { - const invocation = createInvocation('git status && rm -rf /tmp/should-run'); - expect( - isShellInvocationAllowlisted(invocation, ['run_shell_command']), - ).toBe(true); - }); - - it('should treat piped commands as separate segments that must be allowlisted', () => { - const invocation = createInvocation('git status | tail -n 1'); - expect( - isShellInvocationAllowlisted(invocation, ['run_shell_command(git)']), - ).toBe(false); - expect( - isShellInvocationAllowlisted(invocation, [ - 'run_shell_command(git)', - 'run_shell_command(tail)', - ]), - ).toBe(true); - }); -}); diff --git a/packages/core/src/utils/shell-permissions.ts b/packages/core/src/utils/shell-permissions.ts deleted file mode 100644 index 29f28b5410..0000000000 --- a/packages/core/src/utils/shell-permissions.ts +++ /dev/null @@ -1,270 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import type { AnyToolInvocation } from '../index.js'; -import type { Config } from '../config/config.js'; -import { doesToolInvocationMatch } from './tool-utils.js'; -import { - parseCommandDetails, - SHELL_TOOL_NAMES, - type ParsedCommandDetail, -} from './shell-utils.js'; - -/** - * Checks a shell command against security policies and allowlists. - * - * This function operates in one of two modes depending on the presence of - * the `sessionAllowlist` parameter: - * - * 1. **"Default Deny" Mode (sessionAllowlist is provided):** This is the - * strictest mode, used for user-defined scripts like custom commands. - * A command is only permitted if it is found on the global `coreTools` - * allowlist OR the provided `sessionAllowlist`. It must not be on the - * global `excludeTools` blocklist. - * - * 2. **"Default Allow" Mode (sessionAllowlist is NOT provided):** This mode - * is used for direct tool invocations (e.g., by the model). If a strict - * global `coreTools` allowlist exists, commands must be on it. Otherwise, - * any command is permitted as long as it is not on the `excludeTools` - * blocklist. - * - * @param command The shell command string to validate. - * @param config The application configuration. - * @param sessionAllowlist A session-level list of approved commands. Its - * presence activates "Default Deny" mode. - * @returns An object detailing which commands are not allowed. - */ -export function checkCommandPermissions( - command: string, - config: Config, - sessionAllowlist?: Set, -): { - allAllowed: boolean; - disallowedCommands: string[]; - blockReason?: string; - isHardDenial?: boolean; -} { - const parseResult = parseCommandDetails(command); - if (!parseResult || parseResult.hasError) { - return { - allAllowed: false, - disallowedCommands: [command], - blockReason: 'Command rejected because it could not be parsed safely', - isHardDenial: true, - }; - } - - const normalize = (cmd: string): string => cmd.trim().replace(/\s+/g, ' '); - const commandsToValidate = parseResult.details - .map((detail: ParsedCommandDetail) => normalize(detail.text)) - .filter(Boolean); - const invocation: AnyToolInvocation & { params: { command: string } } = { - params: { command: '' }, - } as AnyToolInvocation & { params: { command: string } }; - - // 1. Blocklist Check (Highest Priority) - const excludeTools = config.getExcludeTools() || new Set([]); - const isWildcardBlocked = SHELL_TOOL_NAMES.some((name) => - excludeTools.has(name), - ); - - if (isWildcardBlocked) { - return { - allAllowed: false, - disallowedCommands: commandsToValidate, - blockReason: 'Shell tool is globally disabled in configuration', - isHardDenial: true, - }; - } - - for (const cmd of commandsToValidate) { - invocation.params['command'] = cmd; - if ( - doesToolInvocationMatch('run_shell_command', invocation, [ - ...excludeTools, - ]) - ) { - return { - allAllowed: false, - disallowedCommands: [cmd], - blockReason: `Command '${cmd}' is blocked by configuration`, - isHardDenial: true, - }; - } - } - - const coreTools = config.getCoreTools() || []; - const isWildcardAllowed = SHELL_TOOL_NAMES.some((name) => - coreTools.includes(name), - ); - - // If there's a global wildcard, all commands are allowed at this point - // because they have already passed the blocklist check. - if (isWildcardAllowed) { - return { allAllowed: true, disallowedCommands: [] }; - } - - const disallowedCommands: string[] = []; - - if (sessionAllowlist) { - // "DEFAULT DENY" MODE: A session allowlist is provided. - // All commands must be in either the session or global allowlist. - const normalizedSessionAllowlist = new Set( - [...sessionAllowlist].flatMap((cmd) => - SHELL_TOOL_NAMES.map((name) => `${name}(${cmd})`), - ), - ); - - for (const cmd of commandsToValidate) { - invocation.params['command'] = cmd; - const isSessionAllowed = doesToolInvocationMatch( - 'run_shell_command', - invocation, - [...normalizedSessionAllowlist], - ); - if (isSessionAllowed) continue; - - const isGloballyAllowed = doesToolInvocationMatch( - 'run_shell_command', - invocation, - coreTools, - ); - if (isGloballyAllowed) continue; - - disallowedCommands.push(cmd); - } - - if (disallowedCommands.length > 0) { - return { - allAllowed: false, - disallowedCommands, - blockReason: `Command(s) not on the global or session allowlist. Disallowed commands: ${disallowedCommands - .map((c) => JSON.stringify(c)) - .join(', ')}`, - isHardDenial: false, // This is a soft denial; confirmation is possible. - }; - } - } else { - // "DEFAULT ALLOW" MODE: No session allowlist. - const hasSpecificAllowedCommands = - coreTools.filter((tool) => - SHELL_TOOL_NAMES.some((name) => tool.startsWith(`${name}(`)), - ).length > 0; - - if (hasSpecificAllowedCommands) { - for (const cmd of commandsToValidate) { - invocation.params['command'] = cmd; - const isGloballyAllowed = doesToolInvocationMatch( - 'run_shell_command', - invocation, - coreTools, - ); - if (!isGloballyAllowed) { - disallowedCommands.push(cmd); - } - } - if (disallowedCommands.length > 0) { - return { - allAllowed: false, - disallowedCommands, - blockReason: `Command(s) not in the allowed commands list. Disallowed commands: ${disallowedCommands - .map((c) => JSON.stringify(c)) - .join(', ')}`, - isHardDenial: false, - }; - } - } - // If no specific global allowlist exists, and it passed the blocklist, - // the command is allowed by default. - } - - // If all checks for the current mode pass, the command is allowed. - return { allAllowed: true, disallowedCommands: [] }; -} - -export function isCommandAllowed( - command: string, - config: Config, -): { allowed: boolean; reason?: string } { - // By not providing a sessionAllowlist, we invoke "default allow" behavior. - const { allAllowed, blockReason } = checkCommandPermissions(command, config); - if (allAllowed) { - return { allowed: true }; - } - return { allowed: false, reason: blockReason }; -} - -/** - * Determines whether a shell invocation should be auto-approved based on an allowlist. - * - * This reuses the same parsing logic as command-permission enforcement so that - * chained commands must be individually covered by the allowlist. - * - * @param invocation The shell tool invocation being evaluated. - * @param allowedPatterns The configured allowlist patterns (e.g. `run_shell_command(git)`). - * @returns True if every parsed command segment is allowed by the patterns; false otherwise. - */ -export function isShellInvocationAllowlisted( - invocation: AnyToolInvocation, - allowedPatterns: string[], -): boolean { - if (!allowedPatterns.length) { - return false; - } - - const hasShellWildcard = allowedPatterns.some((pattern) => - SHELL_TOOL_NAMES.includes(pattern), - ); - const hasShellSpecificPattern = allowedPatterns.some((pattern) => - SHELL_TOOL_NAMES.some((name) => pattern.startsWith(`${name}(`)), - ); - - if (!hasShellWildcard && !hasShellSpecificPattern) { - return false; - } - - if (hasShellWildcard) { - return true; - } - - if ( - !('params' in invocation) || - typeof invocation.params !== 'object' || - invocation.params === null || - !('command' in invocation.params) - ) { - return false; - } - - const commandValue = (invocation.params as { command?: unknown }).command; - if (typeof commandValue !== 'string' || !commandValue.trim()) { - return false; - } - - const command = commandValue.trim(); - - const parseResult = parseCommandDetails(command); - if (!parseResult || parseResult.hasError) { - return false; - } - - const normalize = (cmd: string): string => cmd.trim().replace(/\s+/g, ' '); - const commandsToValidate = parseResult.details - .map((detail: ParsedCommandDetail) => normalize(detail.text)) - .filter(Boolean); - - if (commandsToValidate.length === 0) { - return false; - } - - return commandsToValidate.every((commandSegment: string) => - doesToolInvocationMatch( - SHELL_TOOL_NAMES[0], - { params: { command: commandSegment } } as AnyToolInvocation, - allowedPatterns, - ), - ); -}