fix(core): properly support allowRedirect in policy engine (#23579)

This commit is contained in:
Tommaso Sciortino
2026-03-23 20:32:50 +00:00
committed by GitHub
parent 42a673a52c
commit 37857ab956
15 changed files with 168 additions and 17 deletions
+10 -2
View File
@@ -301,7 +301,7 @@ priority = 10
# (Optional) A custom message to display when a tool call is denied by this # (Optional) A custom message to display when a tool call is denied by this
# rule. This message is returned to the model and user, # rule. This message is returned to the model and user,
# useful for explaining *why* it was denied. # 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. # (Optional) An array of approval modes where this rule is active.
modes = ["autoEdit"] modes = ["autoEdit"]
@@ -310,6 +310,14 @@ modes = ["autoEdit"]
# non-interactive (false) environments. # non-interactive (false) environments.
# If omitted, the rule applies to both. # If omitted, the rule applies to both.
interactive = true 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) ### Using arrays (lists)
@@ -394,7 +402,7 @@ server.
mcpName = "untrusted-server" mcpName = "untrusted-server"
decision = "deny" decision = "deny"
priority = 500 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** **3. Targeting all MCP servers**
@@ -16,7 +16,7 @@ toolName = "grep_search"
argsPattern = "(\.env|id_rsa|passwd)" argsPattern = "(\.env|id_rsa|passwd)"
decision = "deny" decision = "deny"
priority = 200 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: Apply path validation to all write operations.
[[safety_checker]] [[safety_checker]]
@@ -136,6 +136,7 @@ export interface UpdatePolicy {
argsPattern?: string; argsPattern?: string;
commandPrefix?: string | string[]; commandPrefix?: string | string[];
mcpName?: string; mcpName?: string;
allowRedirection?: boolean;
} }
export interface ToolPolicyRejection { export interface ToolPolicyRejection {
+7
View File
@@ -537,6 +537,7 @@ interface TomlRule {
priority?: number; priority?: number;
commandPrefix?: string | string[]; commandPrefix?: string | string[];
argsPattern?: string; argsPattern?: string;
allowRedirection?: boolean;
// Index signature to satisfy Record type if needed for toml.stringify // Index signature to satisfy Record type if needed for toml.stringify
[key: string]: unknown; [key: string]: unknown;
} }
@@ -581,6 +582,7 @@ export function createPolicyUpdater(
argsPattern: new RegExp(pattern), argsPattern: new RegExp(pattern),
mcpName: message.mcpName, mcpName: message.mcpName,
source: 'Dynamic (Confirmed)', source: 'Dynamic (Confirmed)',
allowRedirection: message.allowRedirection,
}); });
} }
} }
@@ -617,6 +619,7 @@ export function createPolicyUpdater(
argsPattern, argsPattern,
mcpName: message.mcpName, mcpName: message.mcpName,
source: 'Dynamic (Confirmed)', source: 'Dynamic (Confirmed)',
allowRedirection: message.allowRedirection,
}); });
} }
@@ -681,6 +684,10 @@ export function createPolicyUpdater(
newRule.argsPattern = message.argsPattern; newRule.argsPattern = message.argsPattern;
} }
if (message.allowRedirection !== undefined) {
newRule.allowRedirection = message.allowRedirection;
}
// Add to rules // Add to rules
existingData.rule.push(newRule); existingData.rule.push(newRule);
@@ -71,6 +71,26 @@ describe('createPolicyUpdater', () => {
expect(content).toContain(`priority = ${expectedPriority}`); 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 () => { it('should not persist policy when persist flag is false or undefined', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage); createPolicyUpdater(policyEngine, messageBus, mockStorage);
@@ -7,4 +7,4 @@ toolName = ["read_file", "write_file", "replace", "list_directory", "glob", "gre
decision = "allow" decision = "allow"
priority = 100 priority = 100
argsPattern = "(^|.*/)\\.gemini/.*" 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."
+4 -4
View File
@@ -46,7 +46,7 @@ toolName = "enter_plan_mode"
decision = "deny" decision = "deny"
priority = 70 priority = 70
modes = ["plan"] modes = ["plan"]
deny_message = "You are already in Plan Mode." denyMessage = "You are already in Plan Mode."
[[rule]] [[rule]]
toolName = "exit_plan_mode" toolName = "exit_plan_mode"
@@ -65,7 +65,7 @@ interactive = false
toolName = "exit_plan_mode" toolName = "exit_plan_mode"
decision = "deny" decision = "deny"
priority = 50 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. # 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" decision = "deny"
priority = 60 priority = 60
modes = ["plan"] 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. # Explicitly Allow Read-Only Tools in Plan mode.
@@ -121,4 +121,4 @@ toolName = ["write_file", "replace"]
decision = "deny" decision = "deny"
priority = 65 priority = 65
modes = ["plan"] 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."
+1 -1
View File
@@ -52,4 +52,4 @@ interactive = true
decision = "allow" decision = "allow"
priority = 998 priority = 998
modes = ["yolo"] modes = ["yolo"]
allow_redirection = true allowRedirection = true
@@ -26,6 +26,7 @@ vi.mock('../config/storage.js');
vi.mock('../utils/shell-utils.js', () => ({ vi.mock('../utils/shell-utils.js', () => ({
getCommandRoots: vi.fn(), getCommandRoots: vi.fn(),
stripShellWrapper: vi.fn(), stripShellWrapper: vi.fn(),
hasRedirection: vi.fn(),
})); }));
interface ParsedPolicy { interface ParsedPolicy {
rule?: Array<{ 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 () => { it('should persist multiple rules correctly to TOML', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage); createPolicyUpdater(policyEngine, messageBus, mockStorage);
vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' }); vi.mocked(fs.readFile).mockRejectedValue({ code: 'ENOENT' });
@@ -238,6 +258,7 @@ describe('ShellToolInvocation Policy Update', () => {
vi.mocked(shellUtils.stripShellWrapper).mockImplementation( vi.mocked(shellUtils.stripShellWrapper).mockImplementation(
(c: string) => c, (c: string) => c,
); );
vi.mocked(shellUtils.hasRedirection).mockReturnValue(false);
}); });
it('should extract multiple root commands for chained commands', () => { it('should extract multiple root commands for chained commands', () => {
@@ -279,4 +300,26 @@ describe('ShellToolInvocation Policy Update', () => {
expect(options!.commandPrefix).toEqual(['ls']); expect(options!.commandPrefix).toEqual(['ls']);
expect(shellUtils.getCommandRoots).toHaveBeenCalledWith('ls -la /tmp'); 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',
);
});
}); });
+29 -1
View File
@@ -263,6 +263,20 @@ allow_redirection = true
expect(result.errors).toHaveLength(0); 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 () => { it('should parse deny_message property', async () => {
const result = await runLoadPoliciesFromToml(` const result = await runLoadPoliciesFromToml(`
[[rule]] [[rule]]
@@ -273,7 +287,21 @@ deny_message = "Deletion is permanent"
`); `);
expect(result.rules).toHaveLength(1); 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].decision).toBe(PolicyDecision.DENY);
expect(result.rules[0].denyMessage).toBe('Deletion is permanent'); expect(result.rules[0].denyMessage).toBe('Deletion is permanent');
expect(getErrors(result)).toHaveLength(0); expect(getErrors(result)).toHaveLength(0);
+7 -4
View File
@@ -63,8 +63,10 @@ const PolicyRuleSchema = z.object({
modes: z.array(z.nativeEnum(ApprovalMode)).optional(), modes: z.array(z.nativeEnum(ApprovalMode)).optional(),
interactive: z.boolean().optional(), interactive: z.boolean().optional(),
toolAnnotations: z.record(z.any()).optional(), toolAnnotations: z.record(z.any()).optional(),
allow_redirection: z.boolean().optional(), allowRedirection: z.boolean().optional(),
deny_message: z.string().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, modes: rule.modes,
interactive: rule.interactive, interactive: rule.interactive,
toolAnnotations: rule.toolAnnotations, toolAnnotations: rule.toolAnnotations,
allowRedirection: rule.allow_redirection, allowRedirection:
rule.allowRedirection ?? rule.allow_redirection,
source: `${tierName.charAt(0).toUpperCase() + tierName.slice(1)}: ${file}`, source: `${tierName.charAt(0).toUpperCase() + tierName.slice(1)}: ${file}`,
denyMessage: rule.deny_message, denyMessage: rule.denyMessage ?? rule.deny_message,
}; };
// Compile regex pattern // Compile regex pattern
+4 -2
View File
@@ -100,10 +100,12 @@ export class ShellToolInvocation extends BaseToolInvocation<
) { ) {
const command = stripShellWrapper(this.params.command); const command = stripShellWrapper(this.params.command);
const rootCommands = [...new Set(getCommandRoots(command))]; const rootCommands = [...new Set(getCommandRoots(command))];
const allowRedirection = hasRedirection(command) ? true : undefined;
if (rootCommands.length > 0) { if (rootCommands.length > 0) {
return { commandPrefix: rootCommands }; return { commandPrefix: rootCommands, allowRedirection };
} }
return { commandPrefix: this.params.command }; return { commandPrefix: this.params.command, allowRedirection };
} }
return undefined; return undefined;
} }
+1
View File
@@ -138,6 +138,7 @@ export interface PolicyUpdateOptions {
commandPrefix?: string | string[]; commandPrefix?: string | string[];
mcpName?: string; mcpName?: string;
toolName?: string; toolName?: string;
allowRedirection?: boolean;
} }
/** /**
@@ -19,6 +19,7 @@ import {
getShellConfiguration, getShellConfiguration,
initializeShellParsers, initializeShellParsers,
parseCommandDetails, parseCommandDetails,
splitCommands,
stripShellWrapper, stripShellWrapper,
hasRedirection, hasRedirection,
resolveExecutable, 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', () => { describe('stripShellWrapper', () => {
it('should strip sh -c with quotes', () => { it('should strip sh -c with quotes', () => {
expect(stripShellWrapper('sh -c "ls -l"')).toEqual('ls -l'); expect(stripShellWrapper('sh -c "ls -l"')).toEqual('ls -l');
+4 -1
View File
@@ -663,7 +663,10 @@ export function splitCommands(command: string): string[] {
return []; 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);
} }
/** /**