diff --git a/docs/reference/policy-engine.md b/docs/reference/policy-engine.md index 456c8a9dc8..1b9575475a 100644 --- a/docs/reference/policy-engine.md +++ b/docs/reference/policy-engine.md @@ -301,7 +301,7 @@ priority = 10 # (Optional) A custom message to display when a tool call is denied by this # rule. This message is returned to the model and user, # useful for explaining *why* it was denied. -deny_message = "Deletion is permanent" +denyMessage = "Deletion is permanent" # (Optional) An array of approval modes where this rule is active. modes = ["autoEdit"] @@ -310,6 +310,14 @@ modes = ["autoEdit"] # non-interactive (false) environments. # If omitted, the rule applies to both. interactive = true + +# (Optional) If true, lets shell commands use redirection operators +# (>, >>, <, <<, <<<). By default, the policy engine asks for confirmation +# when redirection is detected, even if a rule matches the command. +# This permission is granular; it only applies to the specific rule it's +# defined in. In chained commands (e.g., cmd1 > file && cmd2), each +# individual command rule must permit redirection if it's used. +allowRedirection = true ``` ### Using arrays (lists) @@ -394,7 +402,7 @@ server. mcpName = "untrusted-server" decision = "deny" priority = 500 -deny_message = "This server is not trusted by the admin." +denyMessage = "This server is not trusted by the admin." ``` **3. Targeting all MCP servers** diff --git a/packages/cli/src/commands/extensions/examples/policies/policies/policies.toml b/packages/cli/src/commands/extensions/examples/policies/policies/policies.toml index d89d5e5737..225627c59b 100644 --- a/packages/cli/src/commands/extensions/examples/policies/policies/policies.toml +++ b/packages/cli/src/commands/extensions/examples/policies/policies/policies.toml @@ -16,7 +16,7 @@ toolName = "grep_search" argsPattern = "(\.env|id_rsa|passwd)" decision = "deny" priority = 200 -deny_message = "Access to sensitive credentials or system files is restricted by the policy-example extension." +denyMessage = "Access to sensitive credentials or system files is restricted by the policy-example extension." # Safety Checker: Apply path validation to all write operations. [[safety_checker]] diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index ceb1c96296..70e2d31f6b 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -136,6 +136,7 @@ export interface UpdatePolicy { argsPattern?: string; commandPrefix?: string | string[]; mcpName?: string; + allowRedirection?: boolean; } export interface ToolPolicyRejection { diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index c54e7f1667..f6107bf460 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -537,6 +537,7 @@ interface TomlRule { priority?: number; commandPrefix?: string | string[]; argsPattern?: string; + allowRedirection?: boolean; // Index signature to satisfy Record type if needed for toml.stringify [key: string]: unknown; } @@ -581,6 +582,7 @@ export function createPolicyUpdater( argsPattern: new RegExp(pattern), mcpName: message.mcpName, source: 'Dynamic (Confirmed)', + allowRedirection: message.allowRedirection, }); } } @@ -617,6 +619,7 @@ export function createPolicyUpdater( argsPattern, mcpName: message.mcpName, source: 'Dynamic (Confirmed)', + allowRedirection: message.allowRedirection, }); } @@ -681,6 +684,10 @@ export function createPolicyUpdater( newRule.argsPattern = message.argsPattern; } + if (message.allowRedirection !== undefined) { + newRule.allowRedirection = message.allowRedirection; + } + // Add to rules existingData.rule.push(newRule); diff --git a/packages/core/src/policy/persistence.test.ts b/packages/core/src/policy/persistence.test.ts index da39160020..d4781fb4be 100644 --- a/packages/core/src/policy/persistence.test.ts +++ b/packages/core/src/policy/persistence.test.ts @@ -71,6 +71,26 @@ describe('createPolicyUpdater', () => { expect(content).toContain(`priority = ${expectedPriority}`); }); + it('should include allowRedirection when persisting policy', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + const policyFile = '/mock/user/.gemini/policies/auto-saved.toml'; + vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'test_tool', + persist: true, + allowRedirection: true, + }); + + await vi.advanceTimersByTimeAsync(100); + + const content = memfs.readFileSync(policyFile, 'utf-8') as string; + expect(content).toContain('toolName = "test_tool"'); + expect(content).toContain('allowRedirection = true'); + }); + it('should not persist policy when persist flag is false or undefined', async () => { createPolicyUpdater(policyEngine, messageBus, mockStorage); diff --git a/packages/core/src/policy/policies/memory-manager.toml b/packages/core/src/policy/policies/memory-manager.toml index 2055fcdf3a..b1b1b4ddd9 100644 --- a/packages/core/src/policy/policies/memory-manager.toml +++ b/packages/core/src/policy/policies/memory-manager.toml @@ -7,4 +7,4 @@ toolName = ["read_file", "write_file", "replace", "list_directory", "glob", "gre decision = "allow" priority = 100 argsPattern = "(^|.*/)\\.gemini/.*" -deny_message = "Memory Manager is only allowed to access the .gemini folder." +denyMessage = "Memory Manager is only allowed to access the .gemini folder." diff --git a/packages/core/src/policy/policies/plan.toml b/packages/core/src/policy/policies/plan.toml index 5a7ee6e59f..b9efd50db7 100644 --- a/packages/core/src/policy/policies/plan.toml +++ b/packages/core/src/policy/policies/plan.toml @@ -46,7 +46,7 @@ toolName = "enter_plan_mode" decision = "deny" priority = 70 modes = ["plan"] -deny_message = "You are already in Plan Mode." +denyMessage = "You are already in Plan Mode." [[rule]] toolName = "exit_plan_mode" @@ -65,7 +65,7 @@ interactive = false toolName = "exit_plan_mode" decision = "deny" priority = 50 -deny_message = "You are not currently in Plan Mode. Use enter_plan_mode first to design a plan." +denyMessage = "You are not currently in Plan Mode. Use enter_plan_mode first to design a plan." # Catch-All: Deny everything by default in Plan mode. @@ -74,7 +74,7 @@ deny_message = "You are not currently in Plan Mode. Use enter_plan_mode first to decision = "deny" priority = 60 modes = ["plan"] -deny_message = "You are in Plan Mode with access to read-only tools. Execution of scripts (including those from skills) is blocked." +denyMessage = "You are in Plan Mode with access to read-only tools. Execution of scripts (including those from skills) is blocked." # Explicitly Allow Read-Only Tools in Plan mode. @@ -121,4 +121,4 @@ toolName = ["write_file", "replace"] decision = "deny" priority = 65 modes = ["plan"] -deny_message = "You are in Plan Mode and cannot modify source code. You may ONLY use write_file or replace to save plans to the designated plans directory as .md files." +denyMessage = "You are in Plan Mode and cannot modify source code. You may ONLY use write_file or replace to save plans to the designated plans directory as .md files." diff --git a/packages/core/src/policy/policies/yolo.toml b/packages/core/src/policy/policies/yolo.toml index 230b4c2670..0516484acd 100644 --- a/packages/core/src/policy/policies/yolo.toml +++ b/packages/core/src/policy/policies/yolo.toml @@ -52,4 +52,4 @@ interactive = true decision = "allow" priority = 998 modes = ["yolo"] -allow_redirection = true +allowRedirection = true diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 3bf3579bbc..5ee9d65df4 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -26,6 +26,7 @@ vi.mock('../config/storage.js'); vi.mock('../utils/shell-utils.js', () => ({ getCommandRoots: vi.fn(), stripShellWrapper: vi.fn(), + hasRedirection: vi.fn(), })); interface ParsedPolicy { rule?: Array<{ @@ -177,6 +178,25 @@ describe('createPolicyUpdater', () => { ); }); + it('should pass allowRedirection to policyEngine.addRule', async () => { + createPolicyUpdater(policyEngine, messageBus, mockStorage); + + await messageBus.publish({ + type: MessageBusType.UPDATE_POLICY, + toolName: 'run_shell_command', + commandPrefix: 'ls', + persist: false, + allowRedirection: true, + }); + + expect(policyEngine.addRule).toHaveBeenCalledWith( + expect.objectContaining({ + toolName: 'run_shell_command', + allowRedirection: true, + }), + ); + }); + it('should persist multiple rules correctly to TOML', async () => { createPolicyUpdater(policyEngine, messageBus, mockStorage); vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' }); @@ -238,6 +258,7 @@ describe('ShellToolInvocation Policy Update', () => { vi.mocked(shellUtils.stripShellWrapper).mockImplementation( (c: string) => c, ); + vi.mocked(shellUtils.hasRedirection).mockReturnValue(false); }); it('should extract multiple root commands for chained commands', () => { @@ -279,4 +300,26 @@ describe('ShellToolInvocation Policy Update', () => { expect(options!.commandPrefix).toEqual(['ls']); expect(shellUtils.getCommandRoots).toHaveBeenCalledWith('ls -la /tmp'); }); + + it('should include allowRedirection if command has redirection', () => { + vi.mocked(shellUtils.getCommandRoots).mockReturnValue(['echo']); + vi.mocked(shellUtils.hasRedirection).mockReturnValue(true); + + const invocation = new ShellToolInvocation( + mockConfig, + { command: 'echo "hello" > file.txt' }, + mockMessageBus, + 'run_shell_command', + 'Shell', + ); + + const options = ( + invocation as unknown as TestableShellToolInvocation + ).getPolicyUpdateOptions(ToolConfirmationOutcome.ProceedAlways); + 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/toml-loader.test.ts b/packages/core/src/policy/toml-loader.test.ts index 959f09ba80..224450f2a2 100644 --- a/packages/core/src/policy/toml-loader.test.ts +++ b/packages/core/src/policy/toml-loader.test.ts @@ -263,6 +263,20 @@ allow_redirection = true expect(result.errors).toHaveLength(0); }); + it('should parse and transform allowRedirection property (camelCase)', async () => { + const result = await runLoadPoliciesFromToml(` +[[rule]] +toolName = "run_shell_command" +commandPrefix = "echo" +decision = "allow" +priority = 100 +allowRedirection = true +`); + + expect(result.rules).toHaveLength(1); + expect(result.rules[0].allowRedirection).toBe(true); + expect(result.errors).toHaveLength(0); + }); it('should parse deny_message property', async () => { const result = await runLoadPoliciesFromToml(` [[rule]] @@ -273,7 +287,21 @@ deny_message = "Deletion is permanent" `); expect(result.rules).toHaveLength(1); - expect(result.rules[0].toolName).toBe('rm'); + expect(result.rules[0].decision).toBe(PolicyDecision.DENY); + expect(result.rules[0].denyMessage).toBe('Deletion is permanent'); + expect(getErrors(result)).toHaveLength(0); + }); + + it('should parse denyMessage property (camelCase)', async () => { + const result = await runLoadPoliciesFromToml(` +[[rule]] +toolName = "rm" +decision = "deny" +priority = 100 +denyMessage = "Deletion is permanent" +`); + + expect(result.rules).toHaveLength(1); expect(result.rules[0].decision).toBe(PolicyDecision.DENY); expect(result.rules[0].denyMessage).toBe('Deletion is permanent'); expect(getErrors(result)).toHaveLength(0); diff --git a/packages/core/src/policy/toml-loader.ts b/packages/core/src/policy/toml-loader.ts index f5210954f7..7f52dacc9f 100644 --- a/packages/core/src/policy/toml-loader.ts +++ b/packages/core/src/policy/toml-loader.ts @@ -63,8 +63,10 @@ const PolicyRuleSchema = z.object({ modes: z.array(z.nativeEnum(ApprovalMode)).optional(), interactive: z.boolean().optional(), toolAnnotations: z.record(z.any()).optional(), - allow_redirection: z.boolean().optional(), - deny_message: z.string().optional(), + allowRedirection: z.boolean().optional(), + allow_redirection: z.boolean().optional(), // deprecated snake_case for backward compatibility + denyMessage: z.string().optional(), + deny_message: z.string().optional(), // deprecated snake_case for backward compatibility }); /** @@ -478,9 +480,10 @@ export async function loadPoliciesFromToml( modes: rule.modes, interactive: rule.interactive, toolAnnotations: rule.toolAnnotations, - allowRedirection: rule.allow_redirection, + allowRedirection: + rule.allowRedirection ?? rule.allow_redirection, source: `${tierName.charAt(0).toUpperCase() + tierName.slice(1)}: ${file}`, - denyMessage: rule.deny_message, + denyMessage: rule.denyMessage ?? rule.deny_message, }; // Compile regex pattern diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 8917d281bd..5ae3948559 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -100,10 +100,12 @@ export class ShellToolInvocation extends BaseToolInvocation< ) { const command = stripShellWrapper(this.params.command); const rootCommands = [...new Set(getCommandRoots(command))]; + const allowRedirection = hasRedirection(command) ? true : undefined; + if (rootCommands.length > 0) { - return { commandPrefix: rootCommands }; + return { commandPrefix: rootCommands, allowRedirection }; } - return { commandPrefix: this.params.command }; + return { commandPrefix: this.params.command, allowRedirection }; } return undefined; } diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 8b7d7223bd..38f484fba3 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -138,6 +138,7 @@ export interface PolicyUpdateOptions { commandPrefix?: string | string[]; mcpName?: string; toolName?: string; + allowRedirection?: boolean; } /** diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 933ca84817..2370aa25c4 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -19,6 +19,7 @@ import { getShellConfiguration, initializeShellParsers, parseCommandDetails, + splitCommands, stripShellWrapper, hasRedirection, resolveExecutable, @@ -304,6 +305,40 @@ describeWindowsOnly('PowerShell integration', () => { }); }); +describe('splitCommands', () => { + it('should split chained commands', () => { + expect(splitCommands('ls -l && git status')).toEqual([ + 'ls -l', + 'git status', + ]); + }); + + it('should filter out redirection tokens but keep command parts', () => { + // Standard redirection + expect(splitCommands('echo "hello" > file.txt')).toEqual(['echo "hello"']); + expect(splitCommands('printf "test" >> log.txt')).toEqual([ + 'printf "test"', + ]); + expect(splitCommands('cat < input.txt')).toEqual(['cat']); + + // Heredoc/Herestring + expect(splitCommands('cat << EOF\nhello\nEOF')).toEqual(['cat']); + // Note: The Tree-sitter bash parser includes the herestring in the main + // command node's text, unlike standard redirections which are siblings. + expect(splitCommands('grep "foo" <<< "foobar"')).toEqual([ + 'grep "foo" <<< "foobar"', + ]); + }); + + it('should extract nested commands from process substitution while filtering the redirection operator', () => { + // This is the key security test: we want cat to be checked, but not the > >(...) wrapper part + const parts = splitCommands('echo "foo" > >(cat)'); + expect(parts).toContain('echo "foo"'); + expect(parts).toContain('cat'); + expect(parts.some((p) => p.includes('>'))).toBe(false); + }); +}); + describe('stripShellWrapper', () => { it('should strip sh -c with quotes', () => { expect(stripShellWrapper('sh -c "ls -l"')).toEqual('ls -l'); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index d2b28a348c..14fce36a34 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -663,7 +663,10 @@ export function splitCommands(command: string): string[] { return []; } - return parsed.details.map((detail) => detail.text).filter(Boolean); + return parsed.details + .filter((detail) => !REDIRECTION_NAMES.has(detail.name)) + .map((detail) => detail.text) + .filter(Boolean); } /**