From c888da5f737332bfbf04a0eef2ee2f008b1efff3 Mon Sep 17 00:00:00 2001 From: ruomeng Date: Thu, 26 Mar 2026 14:35:12 -0400 Subject: [PATCH] fix(core): replace hardcoded non-interactive ASK_USER denial with explicit policy rules (#23668) --- packages/cli/src/config/config.test.ts | 23 +++- packages/cli/src/config/config.ts | 2 +- .../config/policy-engine.integration.test.ts | 6 +- packages/cli/src/config/policy.ts | 8 +- .../src/config/workspace-policy-cli.test.ts | 14 ++ packages/core/src/policy/config.ts | 6 +- .../core/src/policy/policies/discovered.toml | 7 + .../src/policy/policies/non-interactive.toml | 7 + packages/core/src/policy/policies/plan.toml | 18 +++ packages/core/src/policy/policies/write.toml | 21 +++ packages/core/src/policy/policies/yolo.toml | 2 +- .../core/src/policy/policy-engine.test.ts | 125 +++++++++++++----- packages/core/src/policy/policy-engine.ts | 34 ++--- 13 files changed, 207 insertions(+), 66 deletions(-) create mode 100644 packages/core/src/policy/policies/non-interactive.toml diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index f312ddde4f..0d9fb8a9a0 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -143,12 +143,17 @@ vi.mock('@google/gemini-cli-core', async () => { respectGeminiIgnore: true, customIgnoreFilePaths: [], }, - createPolicyEngineConfig: vi.fn(async () => ({ - rules: [], - checkers: [], - defaultDecision: ServerConfig.PolicyDecision.ASK_USER, - approvalMode: ServerConfig.ApprovalMode.DEFAULT, - })), + createPolicyEngineConfig: vi.fn( + async (_settings, approvalMode, _workspacePoliciesDir, interactive) => ({ + rules: [], + checkers: [], + defaultDecision: interactive + ? ServerConfig.PolicyDecision.ASK_USER + : ServerConfig.PolicyDecision.DENY, + approvalMode: approvalMode ?? ServerConfig.ApprovalMode.DEFAULT, + nonInteractive: !interactive, + }), + ), getAdminErrorMessage: vi.fn( (_feature) => `YOLO mode is disabled by your administrator. To enable it, please request an update to the settings at: https://goo.gle/manage-gemini-cli`, @@ -3460,6 +3465,8 @@ describe('Policy Engine Integration in loadCliConfig', () => { }), }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -3481,6 +3488,8 @@ describe('Policy Engine Integration in loadCliConfig', () => { }), }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -3504,6 +3513,8 @@ describe('Policy Engine Integration in loadCliConfig', () => { ], }), expect.anything(), + undefined, + expect.anything(), ); }); }); diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index fa6d16fc72..af8c1ae0ac 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -792,8 +792,8 @@ export async function loadCliConfig( effectiveSettings, approvalMode, workspacePoliciesDir, + interactive, ); - policyEngineConfig.nonInteractive = !interactive; const defaultModel = PREVIEW_GEMINI_MODEL_AUTO; const specifiedModel = diff --git a/packages/cli/src/config/policy-engine.integration.test.ts b/packages/cli/src/config/policy-engine.integration.test.ts index 3b2a34ca69..edc06bfbf0 100644 --- a/packages/cli/src/config/policy-engine.integration.test.ts +++ b/packages/cli/src/config/policy-engine.integration.test.ts @@ -605,12 +605,12 @@ describe('Policy Engine Integration Tests', () => { it('should verify non-interactive mode transformation', async () => { const settings: Settings = {}; - const config = await createPolicyEngineConfig( + const engineConfig = await createPolicyEngineConfig( settings, ApprovalMode.DEFAULT, + undefined, + false, ); - // Enable non-interactive mode - const engineConfig = { ...config, nonInteractive: true }; const engine = new PolicyEngine(engineConfig); // ASK_USER should become DENY in non-interactive mode diff --git a/packages/cli/src/config/policy.ts b/packages/cli/src/config/policy.ts index 9837c2c355..317d2e848d 100644 --- a/packages/cli/src/config/policy.ts +++ b/packages/cli/src/config/policy.ts @@ -53,6 +53,7 @@ export async function createPolicyEngineConfig( settings: Settings, approvalMode: ApprovalMode, workspacePoliciesDir?: string, + interactive: boolean = true, ): Promise { // Explicitly construct PolicySettings from Settings to ensure type safety // and avoid accidental leakage of other settings properties. @@ -68,7 +69,12 @@ export async function createPolicyEngineConfig( settings.admin?.secureModeEnabled, }; - return createCorePolicyEngineConfig(policySettings, approvalMode); + return createCorePolicyEngineConfig( + policySettings, + approvalMode, + undefined, + interactive, + ); } export function createPolicyUpdater( diff --git a/packages/cli/src/config/workspace-policy-cli.test.ts b/packages/cli/src/config/workspace-policy-cli.test.ts index d0d98a5a31..bd9bcd0105 100644 --- a/packages/cli/src/config/workspace-policy-cli.test.ts +++ b/packages/cli/src/config/workspace-policy-cli.test.ts @@ -88,6 +88,8 @@ describe('Workspace-Level Policy CLI Integration', () => { ), }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -107,6 +109,8 @@ describe('Workspace-Level Policy CLI Integration', () => { workspacePoliciesDir: undefined, }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -131,6 +135,8 @@ describe('Workspace-Level Policy CLI Integration', () => { workspacePoliciesDir: undefined, }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -163,6 +169,8 @@ describe('Workspace-Level Policy CLI Integration', () => { ), }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -201,6 +209,8 @@ describe('Workspace-Level Policy CLI Integration', () => { ), }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -237,6 +247,8 @@ describe('Workspace-Level Policy CLI Integration', () => { ), }), expect.anything(), + undefined, + expect.anything(), ); }); @@ -278,6 +290,8 @@ describe('Workspace-Level Policy CLI Integration', () => { workspacePoliciesDir: undefined, }), expect.anything(), + undefined, + expect.anything(), ); } finally { // Restore for other tests diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index f6107bf460..38106e7261 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -285,6 +285,7 @@ export async function createPolicyEngineConfig( settings: PolicySettings, approvalMode: ApprovalMode, defaultPoliciesDir?: string, + interactive: boolean = true, ): Promise { const systemPoliciesDir = path.resolve(Storage.getSystemPoliciesDir()); const userPoliciesDir = path.resolve(Storage.getUserPoliciesDir()); @@ -524,7 +525,10 @@ export async function createPolicyEngineConfig( return { rules, checkers, - defaultDecision: PolicyDecision.ASK_USER, + defaultDecision: interactive + ? PolicyDecision.ASK_USER + : PolicyDecision.DENY, + nonInteractive: !interactive, approvalMode, disableAlwaysAllow: settings.disableAlwaysAllow, }; diff --git a/packages/core/src/policy/policies/discovered.toml b/packages/core/src/policy/policies/discovered.toml index b343a1807f..41ebe8124e 100644 --- a/packages/core/src/policy/policies/discovered.toml +++ b/packages/core/src/policy/policies/discovered.toml @@ -6,3 +6,10 @@ toolName = "discovered_tool_*" decision = "ask_user" priority = 10 +interactive = true + +[[rule]] +toolName = "discovered_tool_*" +decision = "deny" +priority = 10 +interactive = false diff --git a/packages/core/src/policy/policies/non-interactive.toml b/packages/core/src/policy/policies/non-interactive.toml new file mode 100644 index 0000000000..04c41f6eb1 --- /dev/null +++ b/packages/core/src/policy/policies/non-interactive.toml @@ -0,0 +1,7 @@ +# Policy for non-interactive mode. +# ASK_USER is strictly forbidden here. +[[rule]] +toolName = "ask_user" +decision = "deny" +priority = 999 +interactive = false diff --git a/packages/core/src/policy/policies/plan.toml b/packages/core/src/policy/policies/plan.toml index 7627010662..b144f3c679 100644 --- a/packages/core/src/policy/policies/plan.toml +++ b/packages/core/src/policy/policies/plan.toml @@ -86,6 +86,16 @@ toolAnnotations = { readOnlyHint = true } decision = "ask_user" priority = 70 modes = ["plan"] +interactive = true + +[[rule]] +toolName = "*" +mcpName = "*" +toolAnnotations = { readOnlyHint = true } +decision = "deny" +priority = 70 +modes = ["plan"] +interactive = false [[rule]] toolName = [ @@ -108,6 +118,14 @@ toolName = ["ask_user", "save_memory"] decision = "ask_user" priority = 70 modes = ["plan"] +interactive = true + +[[rule]] +toolName = ["ask_user", "save_memory"] +decision = "deny" +priority = 70 +modes = ["plan"] +interactive = false # Allow write_file and replace for .md files in the plans directory (cross-platform) # We split this into two rules to avoid ReDoS checker issues with nested optional segments. diff --git a/packages/core/src/policy/policies/write.toml b/packages/core/src/policy/policies/write.toml index 527ac6f059..55ffd8c54f 100644 --- a/packages/core/src/policy/policies/write.toml +++ b/packages/core/src/policy/policies/write.toml @@ -31,6 +31,7 @@ toolName = "replace" decision = "ask_user" priority = 10 +interactive = true [[rule]] toolName = "replace" @@ -47,21 +48,25 @@ required_context = ["environment"] toolName = "save_memory" decision = "ask_user" priority = 10 +interactive = true [[rule]] toolName = "run_shell_command" decision = "ask_user" priority = 10 +interactive = true [[rule]] toolName = "write_file" decision = "ask_user" priority = 10 +interactive = true [[rule]] toolName = "activate_skill" decision = "ask_user" priority = 10 +interactive = true [[rule]] toolName = "write_file" @@ -84,3 +89,19 @@ modes = ["autoEdit"] toolName = "web_fetch" decision = "ask_user" priority = 10 +interactive = true + +# Headless Denial Rule (Priority 10) +# Ensures that tools that normally default to ASK_USER are denied in non-interactive mode. +[[rule]] +toolName = [ + "replace", + "save_memory", + "run_shell_command", + "write_file", + "activate_skill", + "web_fetch" +] +decision = "deny" +priority = 10 +interactive = false diff --git a/packages/core/src/policy/policies/yolo.toml b/packages/core/src/policy/policies/yolo.toml index 5e2a194d2e..b6a8fdea91 100644 --- a/packages/core/src/policy/policies/yolo.toml +++ b/packages/core/src/policy/policies/yolo.toml @@ -30,12 +30,12 @@ # Ask-user tool always requires user interaction, even in YOLO mode. # This ensures the model can gather user preferences/decisions when needed. -# Note: In non-interactive mode, this decision is converted to DENY by the policy engine. [[rule]] toolName = "ask_user" decision = "ask_user" priority = 999 modes = ["yolo"] +interactive = true # Plan mode transitions are blocked in YOLO mode to maintain state consistency # and because planning currently requires human interaction (plan approval), diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index 137ca76aa1..95f754bc02 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -293,8 +293,22 @@ describe('PolicyEngine', () => { const config: PolicyEngineConfig = { nonInteractive: true, rules: [ - { toolName: 'interactive-tool', decision: PolicyDecision.ASK_USER }, + { + toolName: 'interactive-tool', + decision: PolicyDecision.ASK_USER, + interactive: true, + }, + { + toolName: 'interactive-tool', + decision: PolicyDecision.DENY, + interactive: false, + }, { toolName: 'allowed-tool', decision: PolicyDecision.ALLOW }, + { + toolName: 'ask_user', + decision: PolicyDecision.DENY, + interactive: false, + }, ], }; @@ -1258,6 +1272,51 @@ describe('PolicyEngine', () => { ).toBe(PolicyDecision.ALLOW); }); + it('should NOT automatically DENY redirected shell commands in non-interactive mode if rules permit it', async () => { + const toolName = 'run_shell_command'; + const command = 'ls > out.txt'; + + const rules: PolicyRule[] = [ + { + toolName, + decision: PolicyDecision.ALLOW, + allowRedirection: true, + }, + ]; + + engine = new PolicyEngine({ rules, nonInteractive: true }); + + expect( + (await engine.check({ name: toolName, args: { command } }, undefined)) + .decision, + ).toBe(PolicyDecision.ALLOW); + }); + + it('should respect DENY rules for redirected shell commands in non-interactive mode', async () => { + const toolName = 'run_shell_command'; + const command = 'ls > out.txt'; + + const rules: PolicyRule[] = [ + { + toolName, + decision: PolicyDecision.ASK_USER, + interactive: true, + }, + { + toolName, + decision: PolicyDecision.DENY, + interactive: false, + }, + ]; + + engine = new PolicyEngine({ rules, nonInteractive: true }); + + expect( + (await engine.check({ name: toolName, args: { command } }, undefined)) + .decision, + ).toBe(PolicyDecision.DENY); + }); + it('should NOT downgrade ALLOW to ASK_USER for quoted redirection chars', async () => { const rules: PolicyRule[] = [ { @@ -1423,21 +1482,25 @@ describe('PolicyEngine', () => { expect(result.decision).toBe(PolicyDecision.DENY); }); - it('should DENY redirected shell commands in non-interactive mode', async () => { + it('should respect explicit DENY rules for redirected shell commands in non-interactive mode', async () => { const config: PolicyEngineConfig = { nonInteractive: true, rules: [ { toolName: 'run_shell_command', decision: PolicyDecision.ALLOW, + interactive: true, + }, + { + toolName: 'run_shell_command', + decision: PolicyDecision.DENY, + interactive: false, }, ], }; engine = new PolicyEngine(config); - // Redirected command should be DENIED in non-interactive mode - // (Normally ASK_USER, but ASK_USER -> DENY in non-interactive) expect( ( await engine.check( @@ -2215,34 +2278,6 @@ describe('PolicyEngine', () => { const result = await engine.check({ name: 'tool' }, undefined); expect(result.decision).toBe(PolicyDecision.ASK_USER); }); - - it('should DENY if checker returns ASK_USER in non-interactive mode', async () => { - const rules: PolicyRule[] = [ - { toolName: 'tool', decision: PolicyDecision.ALLOW }, - ]; - const checkers: SafetyCheckerRule[] = [ - { - toolName: '*', - checker: { - type: 'in-process', - name: InProcessCheckerType.ALLOWED_PATH, - }, - }, - ]; - - engine = new PolicyEngine( - { rules, checkers, nonInteractive: true }, - mockCheckerRunner, - ); - - vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({ - decision: SafetyCheckDecision.ASK_USER, - reason: 'Suspicious path', - }); - - const result = await engine.check({ name: 'tool' }, undefined); - expect(result.decision).toBe(PolicyDecision.DENY); - }); }); describe('getExcludedTools', () => { @@ -2345,18 +2380,42 @@ describe('PolicyEngine', () => { expected: [], }, { - name: 'should NOT include ASK_USER tools even in non-interactive mode', + name: 'should include tools in exclusion list only if explicitly denied in non-interactive mode', rules: [ { toolName: 'tool1', decision: PolicyDecision.ASK_USER, modes: [ApprovalMode.DEFAULT], + interactive: true, + }, + { + toolName: 'tool1', + decision: PolicyDecision.DENY, + modes: [ApprovalMode.DEFAULT], + interactive: false, }, ], nonInteractive: true, allToolNames: ['tool1'], expected: ['tool1'], }, + { + name: 'should specifically exclude ask_user tool in non-interactive mode', + rules: [ + { + toolName: 'ask_user', + decision: PolicyDecision.DENY, + interactive: false, + }, + { + toolName: 'read_file', + decision: PolicyDecision.ALLOW, + }, + ], + nonInteractive: true, + allToolNames: ['ask_user', 'read_file'], + expected: ['ask_user'], + }, { name: 'should ignore rules with argsPattern', rules: [ diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index 18ab20bb14..c901116eb7 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -244,8 +244,10 @@ export class PolicyEngine { } } - this.defaultDecision = config.defaultDecision ?? PolicyDecision.ASK_USER; this.nonInteractive = config.nonInteractive ?? false; + this.defaultDecision = + config.defaultDecision ?? + (this.nonInteractive ? PolicyDecision.DENY : PolicyDecision.ASK_USER); this.disableAlwaysAllow = config.disableAlwaysAllow ?? false; this.checkerRunner = checkerRunner; this.approvalMode = config.approvalMode ?? ApprovalMode.DEFAULT; @@ -340,7 +342,7 @@ export class PolicyEngine { ): Promise { if (!command) { return { - decision: this.applyNonInteractiveMode(ruleDecision), + decision: ruleDecision, rule, }; } @@ -363,13 +365,13 @@ export class PolicyEngine { } debugLogger.debug( - `[PolicyEngine.check] Command parsing failed for: ${command}. Falling back to ASK_USER.`, + `[PolicyEngine.check] Command parsing failed for: ${command}. Falling back to ${this.defaultDecision}.`, ); - // Parsing logic failed, we can't trust it. Force ASK_USER (or DENY). + // Parsing logic failed, we can't trust it. Use default decision ASK_USER (or DENY in non-interactive). // We return the rule that matched so the evaluation loop terminates. return { - decision: this.applyNonInteractiveMode(PolicyDecision.ASK_USER), + decision: this.defaultDecision, rule, }; } @@ -466,7 +468,7 @@ export class PolicyEngine { } return { - decision: this.applyNonInteractiveMode(aggregateDecision), + decision: aggregateDecision, // If we stayed at ALLOW, we return the original rule (if any). // If we downgraded, we return the responsible rule (or undefined if implicit). rule: aggregateDecision === ruleDecision ? rule : responsibleRule, @@ -474,7 +476,7 @@ export class PolicyEngine { } return { - decision: this.applyNonInteractiveMode(ruleDecision), + decision: ruleDecision, rule, }; } @@ -597,7 +599,7 @@ export class PolicyEngine { break; } } else { - decision = this.applyNonInteractiveMode(rule.decision); + decision = rule.decision; matchedRule = rule; break; } @@ -641,7 +643,7 @@ export class PolicyEngine { decision = shellResult.decision; matchedRule = shellResult.rule; } else { - decision = this.applyNonInteractiveMode(this.defaultDecision); + decision = this.defaultDecision; } } @@ -697,7 +699,7 @@ export class PolicyEngine { } return { - decision: this.applyNonInteractiveMode(decision), + decision, rule: matchedRule, }; } @@ -866,7 +868,7 @@ export class PolicyEngine { continue; } else { // Unconditional rule for this tool - const decision = this.applyNonInteractiveMode(rule.decision); + const decision = rule.decision; staticallyExcluded = decision === PolicyDecision.DENY; matchFound = true; break; @@ -876,7 +878,7 @@ export class PolicyEngine { if (!matchFound) { // Fallback to default decision if no rule matches - const defaultDec = this.applyNonInteractiveMode(this.defaultDecision); + const defaultDec = this.defaultDecision; if (defaultDec === PolicyDecision.DENY) { staticallyExcluded = true; } @@ -889,12 +891,4 @@ export class PolicyEngine { return excludedTools; } - - private applyNonInteractiveMode(decision: PolicyDecision): PolicyDecision { - // In non-interactive mode, ASK_USER becomes DENY - if (this.nonInteractive && decision === PolicyDecision.ASK_USER) { - return PolicyDecision.DENY; - } - return decision; - } }