clone/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-06-01 19:03:42 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat(core): improve shell redirection transparency and security (#16486)

Browse Source
This commit is contained in:
N. Taylor Mullen
2026-01-19 20:07:28 -08:00
committed by GitHub
parent dfa1f4a0ed
commit 049e3ef1dc
16 changed files with 497 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
packages/core/src/policy/config.test.ts
View File

@@ -98,7 +98,7 @@ describe('createPolicyEngineConfig', () => {
expect(config.rules).toEqual([]);
vi.doUnmock('node:fs/promises');
});
}, 30000);
it('should allow tools in tools.allowed', async () => {
const { createPolicyEngineConfig } = await import('./config.js');

117
packages/core/src/policy/policy-engine.test.ts
View File

@@ -1279,6 +1279,123 @@ describe('PolicyEngine', () => {
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
it('should require confirmation for a compound command with redirection even if individual commands are allowed', async () => {
const rules: PolicyRule[] = [
{
toolName: 'run_shell_command',
argsPattern: /"command":"mkdir\b/,
decision: PolicyDecision.ALLOW,
priority: 20,
},
{
toolName: 'run_shell_command',
argsPattern: /"command":"echo\b/,
decision: PolicyDecision.ALLOW,
priority: 20,
},
];
engine = new PolicyEngine({ rules });
// The full command has redirection, even if the individual split commands do not.
// splitCommands will return ['mkdir -p "bar"', 'echo "hello"']
// The redirection '> bar/test.md' is stripped by splitCommands.
const result = await engine.check(
{
name: 'run_shell_command',
args: { command: 'mkdir -p "bar" && echo "hello" > bar/test.md' },
},
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should report redirection when a sub-command specifically has redirection', async () => {
const rules: PolicyRule[] = [
{
toolName: 'run_shell_command',
argsPattern: /"command":"mkdir\b/,
decision: PolicyDecision.ALLOW,
priority: 20,
},
{
toolName: 'run_shell_command',
argsPattern: /"command":"echo\b/,
decision: PolicyDecision.ALLOW,
priority: 20,
},
];
engine = new PolicyEngine({ rules });
// In this case, we mock splitCommands to keep the redirection in the sub-command
vi.mocked(initializeShellParsers).mockResolvedValue(undefined);
const { splitCommands } = await import('../utils/shell-utils.js');
vi.mocked(splitCommands).mockReturnValueOnce([
'mkdir bar',
'echo hello > bar/test.md',
]);
const result = await engine.check(
{
name: 'run_shell_command',
args: { command: 'mkdir bar && echo hello > bar/test.md' },
},
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should allow redirected shell commands in AUTO_EDIT mode if individual commands are allowed', async () => {
const rules: PolicyRule[] = [
{
toolName: 'run_shell_command',
argsPattern: /"command":"echo\b/,
decision: PolicyDecision.ALLOW,
priority: 20,
},
];
engine = new PolicyEngine({ rules });
engine.setApprovalMode(ApprovalMode.AUTO_EDIT);
const result = await engine.check(
{
name: 'run_shell_command',
args: { command: 'echo "hello" > test.txt' },
},
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
it('should allow compound commands with safe operators (&&, ||) if individual commands are allowed', async () => {
const rules: PolicyRule[] = [
{
toolName: 'run_shell_command',
argsPattern: /"command":"echo\b/,
decision: PolicyDecision.ALLOW,
priority: 20,
},
];
engine = new PolicyEngine({ rules });
// "echo hello && echo world" should be allowed since both parts are ALLOW and no redirection is present.
const result = await engine.check(
{
name: 'run_shell_command',
args: { command: 'echo hello && echo world' },
},
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
});
describe('safety checker integration', () => {

76
packages/core/src/policy/policy-engine.ts
View File

@@ -14,6 +14,7 @@ import {
type HookExecutionContext,
getHookSource,
ApprovalMode,
type CheckResult,
} from './types.js';
import { stableStringify } from './stable-stringify.js';
import { debugLogger } from '../utils/debugLogger.js';
@@ -141,6 +142,18 @@ export class PolicyEngine {
return this.approvalMode;
}
private shouldDowngradeForRedirection(
command: string,
allowRedirection?: boolean,
): boolean {
return (
!allowRedirection &&
hasRedirection(command) &&
this.approvalMode !== ApprovalMode.AUTO_EDIT &&
this.approvalMode !== ApprovalMode.YOLO
);
}
/**
* Check if a shell command is allowed.
*/
@@ -152,7 +165,7 @@ export class PolicyEngine {
dir_path: string | undefined,
allowRedirection?: boolean,
rule?: PolicyRule,
): Promise<{ decision: PolicyDecision; rule?: PolicyRule }> {
): Promise<CheckResult> {
if (!command) {
return {
decision: this.applyNonInteractiveMode(ruleDecision),
@@ -190,11 +203,20 @@ export class PolicyEngine {
let aggregateDecision = PolicyDecision.ALLOW;
let responsibleRule: PolicyRule | undefined;
// Check for redirection on the full command string
if (this.shouldDowngradeForRedirection(command, allowRedirection)) {
debugLogger.debug(
`[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${command}`,
);
aggregateDecision = PolicyDecision.ASK_USER;
responsibleRule = undefined; // Inherent policy
}
for (const rawSubCmd of subCommands) {
const subCmd = rawSubCmd.trim();
// Prevent infinite recursion for the root command
if (subCmd === command) {
if (!allowRedirection && hasRedirection(subCmd)) {
if (this.shouldDowngradeForRedirection(subCmd, allowRedirection)) {
debugLogger.debug(
`[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`,
);
@@ -224,7 +246,7 @@ export class PolicyEngine {
// subResult.decision is already filtered through applyNonInteractiveMode by this.check()
const subDecision = subResult.decision;
// If any part is DENIED, the whole command is DENIED
// If any part is DENIED, the whole command is DENY
if (subDecision === PolicyDecision.DENY) {
return {
decision: PolicyDecision.DENY,
@@ -243,8 +265,7 @@ export class PolicyEngine {
// Check for redirection in allowed sub-commands
if (
subDecision === PolicyDecision.ALLOW &&
!allowRedirection &&
hasRedirection(subCmd)
this.shouldDowngradeForRedirection(subCmd, allowRedirection)
) {
debugLogger.debug(
`[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`,
@@ -255,6 +276,7 @@ export class PolicyEngine {
}
}
}
return {
decision: this.applyNonInteractiveMode(aggregateDecision),
// If we stayed at ALLOW, we return the original rule (if any).
@@ -276,10 +298,7 @@ export class PolicyEngine {
async check(
toolCall: FunctionCall,
serverName: string | undefined,
): Promise<{
decision: PolicyDecision;
rule?: PolicyRule;
}> {
): Promise<CheckResult> {
let stringifiedArgs: string | undefined;
// Compute stringified args once before the loop
if (
@@ -299,7 +318,9 @@ export class PolicyEngine {
let command: string | undefined;
let shellDirPath: string | undefined;
if (toolCall.name && SHELL_TOOL_NAMES.includes(toolCall.name)) {
const toolName = toolCall.name;
if (toolName && SHELL_TOOL_NAMES.includes(toolName)) {
isShellCommand = true;
const args = toolCall.args as { command?: string; dir_path?: string };
command = args?.command;
@@ -330,9 +351,9 @@ export class PolicyEngine {
`[PolicyEngine.check] MATCHED rule: toolName=${rule.toolName}, decision=${rule.decision}, priority=${rule.priority}, argsPattern=${rule.argsPattern?.source || 'none'}`,
);
if (isShellCommand) {
if (isShellCommand && toolName) {
const shellResult = await this.checkShellCommand(
toolCall.name!,
toolName,
command,
rule.decision,
serverName,
@@ -345,11 +366,6 @@ export class PolicyEngine {
matchedRule = shellResult.rule;
break;
}
// If no rule returned (e.g. downgraded to default ASK_USER due to redirection),
// we might still want to blame the matched rule?
// No, test says we should return undefined rule if implicit.
matchedRule = shellResult.rule;
break;
} else {
decision = this.applyNonInteractiveMode(rule.decision);
matchedRule = rule;
@@ -358,31 +374,27 @@ export class PolicyEngine {
}
}
if (!decision) {
// No matching rule found, use default decision
// Default if no rule matched
if (decision === undefined) {
debugLogger.debug(
`[PolicyEngine.check] NO MATCH - using default decision: ${this.defaultDecision}`,
);
decision = this.applyNonInteractiveMode(this.defaultDecision);
// If it's a shell command and we fell back to default, we MUST still verify subcommands!
// This is critical for security: "git commit && git push" where "git push" is DENY but "git commit" has no rule.
if (isShellCommand && decision !== PolicyDecision.DENY) {
if (toolName && SHELL_TOOL_NAMES.includes(toolName)) {
const shellResult = await this.checkShellCommand(
toolCall.name!,
toolName,
command,
decision, // default decision
this.defaultDecision,
serverName,
shellDirPath,
false, // no rule, so no allowRedirection
undefined, // no rule
);
decision = shellResult.decision;
matchedRule = shellResult.rule;
} else {
decision = this.applyNonInteractiveMode(this.defaultDecision);
}
}
// If decision is not DENY, run safety checkers
// Safety checks
if (decision !== PolicyDecision.DENY && this.checkerRunner) {
for (const checkerRule of this.checkers) {
if (
@@ -402,10 +414,9 @@ export class PolicyEngine {
toolCall,
checkerRule.checker,
);
if (result.decision === SafetyCheckDecision.DENY) {
debugLogger.debug(
`[PolicyEngine.check] Safety checker denied: ${result.reason}`,
`[PolicyEngine.check] Safety checker '${checkerRule.checker.name}' denied execution: ${result.reason}`,
);
return {
decision: PolicyDecision.DENY,
@@ -419,7 +430,8 @@ export class PolicyEngine {
}
} catch (error) {
debugLogger.debug(
`[PolicyEngine.check] Safety checker failed: ${error}`,
`[PolicyEngine.check] Safety checker '${checkerRule.checker.name}' threw an error:`,
error,
);
return {
decision: PolicyDecision.DENY,

5
packages/core/src/policy/types.ts
View File

@@ -265,3 +265,8 @@ export interface PolicySettings {
};
mcpServers?: Record<string, { trust?: boolean }>;
}
export interface CheckResult {
decision: PolicyDecision;
rule?: PolicyRule;
}