fix(core): replace hardcoded non-interactive ASK_USER denial with explicit policy rules (#23668)

This commit is contained in:
ruomeng
2026-03-26 14:35:12 -04:00
committed by GitHub
parent aa4d9316a9
commit c888da5f73
13 changed files with 207 additions and 66 deletions
+17 -6
View File
@@ -143,12 +143,17 @@ vi.mock('@google/gemini-cli-core', async () => {
respectGeminiIgnore: true, respectGeminiIgnore: true,
customIgnoreFilePaths: [], customIgnoreFilePaths: [],
}, },
createPolicyEngineConfig: vi.fn(async () => ({ createPolicyEngineConfig: vi.fn(
rules: [], async (_settings, approvalMode, _workspacePoliciesDir, interactive) => ({
checkers: [], rules: [],
defaultDecision: ServerConfig.PolicyDecision.ASK_USER, checkers: [],
approvalMode: ServerConfig.ApprovalMode.DEFAULT, defaultDecision: interactive
})), ? ServerConfig.PolicyDecision.ASK_USER
: ServerConfig.PolicyDecision.DENY,
approvalMode: approvalMode ?? ServerConfig.ApprovalMode.DEFAULT,
nonInteractive: !interactive,
}),
),
getAdminErrorMessage: vi.fn( getAdminErrorMessage: vi.fn(
(_feature) => (_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`, `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(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -3481,6 +3488,8 @@ describe('Policy Engine Integration in loadCliConfig', () => {
}), }),
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -3504,6 +3513,8 @@ describe('Policy Engine Integration in loadCliConfig', () => {
], ],
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
}); });
+1 -1
View File
@@ -792,8 +792,8 @@ export async function loadCliConfig(
effectiveSettings, effectiveSettings,
approvalMode, approvalMode,
workspacePoliciesDir, workspacePoliciesDir,
interactive,
); );
policyEngineConfig.nonInteractive = !interactive;
const defaultModel = PREVIEW_GEMINI_MODEL_AUTO; const defaultModel = PREVIEW_GEMINI_MODEL_AUTO;
const specifiedModel = const specifiedModel =
@@ -605,12 +605,12 @@ describe('Policy Engine Integration Tests', () => {
it('should verify non-interactive mode transformation', async () => { it('should verify non-interactive mode transformation', async () => {
const settings: Settings = {}; const settings: Settings = {};
const config = await createPolicyEngineConfig( const engineConfig = await createPolicyEngineConfig(
settings, settings,
ApprovalMode.DEFAULT, ApprovalMode.DEFAULT,
undefined,
false,
); );
// Enable non-interactive mode
const engineConfig = { ...config, nonInteractive: true };
const engine = new PolicyEngine(engineConfig); const engine = new PolicyEngine(engineConfig);
// ASK_USER should become DENY in non-interactive mode // ASK_USER should become DENY in non-interactive mode
+7 -1
View File
@@ -53,6 +53,7 @@ export async function createPolicyEngineConfig(
settings: Settings, settings: Settings,
approvalMode: ApprovalMode, approvalMode: ApprovalMode,
workspacePoliciesDir?: string, workspacePoliciesDir?: string,
interactive: boolean = true,
): Promise<PolicyEngineConfig> { ): Promise<PolicyEngineConfig> {
// Explicitly construct PolicySettings from Settings to ensure type safety // Explicitly construct PolicySettings from Settings to ensure type safety
// and avoid accidental leakage of other settings properties. // and avoid accidental leakage of other settings properties.
@@ -68,7 +69,12 @@ export async function createPolicyEngineConfig(
settings.admin?.secureModeEnabled, settings.admin?.secureModeEnabled,
}; };
return createCorePolicyEngineConfig(policySettings, approvalMode); return createCorePolicyEngineConfig(
policySettings,
approvalMode,
undefined,
interactive,
);
} }
export function createPolicyUpdater( export function createPolicyUpdater(
@@ -88,6 +88,8 @@ describe('Workspace-Level Policy CLI Integration', () => {
), ),
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -107,6 +109,8 @@ describe('Workspace-Level Policy CLI Integration', () => {
workspacePoliciesDir: undefined, workspacePoliciesDir: undefined,
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -131,6 +135,8 @@ describe('Workspace-Level Policy CLI Integration', () => {
workspacePoliciesDir: undefined, workspacePoliciesDir: undefined,
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -163,6 +169,8 @@ describe('Workspace-Level Policy CLI Integration', () => {
), ),
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -201,6 +209,8 @@ describe('Workspace-Level Policy CLI Integration', () => {
), ),
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -237,6 +247,8 @@ describe('Workspace-Level Policy CLI Integration', () => {
), ),
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
}); });
@@ -278,6 +290,8 @@ describe('Workspace-Level Policy CLI Integration', () => {
workspacePoliciesDir: undefined, workspacePoliciesDir: undefined,
}), }),
expect.anything(), expect.anything(),
undefined,
expect.anything(),
); );
} finally { } finally {
// Restore for other tests // Restore for other tests
+5 -1
View File
@@ -285,6 +285,7 @@ export async function createPolicyEngineConfig(
settings: PolicySettings, settings: PolicySettings,
approvalMode: ApprovalMode, approvalMode: ApprovalMode,
defaultPoliciesDir?: string, defaultPoliciesDir?: string,
interactive: boolean = true,
): Promise<PolicyEngineConfig> { ): Promise<PolicyEngineConfig> {
const systemPoliciesDir = path.resolve(Storage.getSystemPoliciesDir()); const systemPoliciesDir = path.resolve(Storage.getSystemPoliciesDir());
const userPoliciesDir = path.resolve(Storage.getUserPoliciesDir()); const userPoliciesDir = path.resolve(Storage.getUserPoliciesDir());
@@ -524,7 +525,10 @@ export async function createPolicyEngineConfig(
return { return {
rules, rules,
checkers, checkers,
defaultDecision: PolicyDecision.ASK_USER, defaultDecision: interactive
? PolicyDecision.ASK_USER
: PolicyDecision.DENY,
nonInteractive: !interactive,
approvalMode, approvalMode,
disableAlwaysAllow: settings.disableAlwaysAllow, disableAlwaysAllow: settings.disableAlwaysAllow,
}; };
@@ -6,3 +6,10 @@
toolName = "discovered_tool_*" toolName = "discovered_tool_*"
decision = "ask_user" decision = "ask_user"
priority = 10 priority = 10
interactive = true
[[rule]]
toolName = "discovered_tool_*"
decision = "deny"
priority = 10
interactive = false
@@ -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
@@ -86,6 +86,16 @@ toolAnnotations = { readOnlyHint = true }
decision = "ask_user" decision = "ask_user"
priority = 70 priority = 70
modes = ["plan"] modes = ["plan"]
interactive = true
[[rule]]
toolName = "*"
mcpName = "*"
toolAnnotations = { readOnlyHint = true }
decision = "deny"
priority = 70
modes = ["plan"]
interactive = false
[[rule]] [[rule]]
toolName = [ toolName = [
@@ -108,6 +118,14 @@ toolName = ["ask_user", "save_memory"]
decision = "ask_user" decision = "ask_user"
priority = 70 priority = 70
modes = ["plan"] 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) # 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. # We split this into two rules to avoid ReDoS checker issues with nested optional segments.
@@ -31,6 +31,7 @@
toolName = "replace" toolName = "replace"
decision = "ask_user" decision = "ask_user"
priority = 10 priority = 10
interactive = true
[[rule]] [[rule]]
toolName = "replace" toolName = "replace"
@@ -47,21 +48,25 @@ required_context = ["environment"]
toolName = "save_memory" toolName = "save_memory"
decision = "ask_user" decision = "ask_user"
priority = 10 priority = 10
interactive = true
[[rule]] [[rule]]
toolName = "run_shell_command" toolName = "run_shell_command"
decision = "ask_user" decision = "ask_user"
priority = 10 priority = 10
interactive = true
[[rule]] [[rule]]
toolName = "write_file" toolName = "write_file"
decision = "ask_user" decision = "ask_user"
priority = 10 priority = 10
interactive = true
[[rule]] [[rule]]
toolName = "activate_skill" toolName = "activate_skill"
decision = "ask_user" decision = "ask_user"
priority = 10 priority = 10
interactive = true
[[rule]] [[rule]]
toolName = "write_file" toolName = "write_file"
@@ -84,3 +89,19 @@ modes = ["autoEdit"]
toolName = "web_fetch" toolName = "web_fetch"
decision = "ask_user" decision = "ask_user"
priority = 10 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
+1 -1
View File
@@ -30,12 +30,12 @@
# Ask-user tool always requires user interaction, even in YOLO mode. # Ask-user tool always requires user interaction, even in YOLO mode.
# This ensures the model can gather user preferences/decisions when needed. # 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]] [[rule]]
toolName = "ask_user" toolName = "ask_user"
decision = "ask_user" decision = "ask_user"
priority = 999 priority = 999
modes = ["yolo"] modes = ["yolo"]
interactive = true
# Plan mode transitions are blocked in YOLO mode to maintain state consistency # Plan mode transitions are blocked in YOLO mode to maintain state consistency
# and because planning currently requires human interaction (plan approval), # and because planning currently requires human interaction (plan approval),
+92 -33
View File
@@ -293,8 +293,22 @@ describe('PolicyEngine', () => {
const config: PolicyEngineConfig = { const config: PolicyEngineConfig = {
nonInteractive: true, nonInteractive: true,
rules: [ 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: 'allowed-tool', decision: PolicyDecision.ALLOW },
{
toolName: 'ask_user',
decision: PolicyDecision.DENY,
interactive: false,
},
], ],
}; };
@@ -1258,6 +1272,51 @@ describe('PolicyEngine', () => {
).toBe(PolicyDecision.ALLOW); ).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 () => { it('should NOT downgrade ALLOW to ASK_USER for quoted redirection chars', async () => {
const rules: PolicyRule[] = [ const rules: PolicyRule[] = [
{ {
@@ -1423,21 +1482,25 @@ describe('PolicyEngine', () => {
expect(result.decision).toBe(PolicyDecision.DENY); 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 = { const config: PolicyEngineConfig = {
nonInteractive: true, nonInteractive: true,
rules: [ rules: [
{ {
toolName: 'run_shell_command', toolName: 'run_shell_command',
decision: PolicyDecision.ALLOW, decision: PolicyDecision.ALLOW,
interactive: true,
},
{
toolName: 'run_shell_command',
decision: PolicyDecision.DENY,
interactive: false,
}, },
], ],
}; };
engine = new PolicyEngine(config); engine = new PolicyEngine(config);
// Redirected command should be DENIED in non-interactive mode
// (Normally ASK_USER, but ASK_USER -> DENY in non-interactive)
expect( expect(
( (
await engine.check( await engine.check(
@@ -2215,34 +2278,6 @@ describe('PolicyEngine', () => {
const result = await engine.check({ name: 'tool' }, undefined); const result = await engine.check({ name: 'tool' }, undefined);
expect(result.decision).toBe(PolicyDecision.ASK_USER); 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', () => { describe('getExcludedTools', () => {
@@ -2345,18 +2380,42 @@ describe('PolicyEngine', () => {
expected: [], 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: [ rules: [
{ {
toolName: 'tool1', toolName: 'tool1',
decision: PolicyDecision.ASK_USER, decision: PolicyDecision.ASK_USER,
modes: [ApprovalMode.DEFAULT], modes: [ApprovalMode.DEFAULT],
interactive: true,
},
{
toolName: 'tool1',
decision: PolicyDecision.DENY,
modes: [ApprovalMode.DEFAULT],
interactive: false,
}, },
], ],
nonInteractive: true, nonInteractive: true,
allToolNames: ['tool1'], allToolNames: ['tool1'],
expected: ['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', name: 'should ignore rules with argsPattern',
rules: [ rules: [
+14 -20
View File
@@ -244,8 +244,10 @@ export class PolicyEngine {
} }
} }
this.defaultDecision = config.defaultDecision ?? PolicyDecision.ASK_USER;
this.nonInteractive = config.nonInteractive ?? false; this.nonInteractive = config.nonInteractive ?? false;
this.defaultDecision =
config.defaultDecision ??
(this.nonInteractive ? PolicyDecision.DENY : PolicyDecision.ASK_USER);
this.disableAlwaysAllow = config.disableAlwaysAllow ?? false; this.disableAlwaysAllow = config.disableAlwaysAllow ?? false;
this.checkerRunner = checkerRunner; this.checkerRunner = checkerRunner;
this.approvalMode = config.approvalMode ?? ApprovalMode.DEFAULT; this.approvalMode = config.approvalMode ?? ApprovalMode.DEFAULT;
@@ -340,7 +342,7 @@ export class PolicyEngine {
): Promise<CheckResult> { ): Promise<CheckResult> {
if (!command) { if (!command) {
return { return {
decision: this.applyNonInteractiveMode(ruleDecision), decision: ruleDecision,
rule, rule,
}; };
} }
@@ -363,13 +365,13 @@ export class PolicyEngine {
} }
debugLogger.debug( 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. // We return the rule that matched so the evaluation loop terminates.
return { return {
decision: this.applyNonInteractiveMode(PolicyDecision.ASK_USER), decision: this.defaultDecision,
rule, rule,
}; };
} }
@@ -466,7 +468,7 @@ export class PolicyEngine {
} }
return { return {
decision: this.applyNonInteractiveMode(aggregateDecision), decision: aggregateDecision,
// If we stayed at ALLOW, we return the original rule (if any). // If we stayed at ALLOW, we return the original rule (if any).
// If we downgraded, we return the responsible rule (or undefined if implicit). // If we downgraded, we return the responsible rule (or undefined if implicit).
rule: aggregateDecision === ruleDecision ? rule : responsibleRule, rule: aggregateDecision === ruleDecision ? rule : responsibleRule,
@@ -474,7 +476,7 @@ export class PolicyEngine {
} }
return { return {
decision: this.applyNonInteractiveMode(ruleDecision), decision: ruleDecision,
rule, rule,
}; };
} }
@@ -597,7 +599,7 @@ export class PolicyEngine {
break; break;
} }
} else { } else {
decision = this.applyNonInteractiveMode(rule.decision); decision = rule.decision;
matchedRule = rule; matchedRule = rule;
break; break;
} }
@@ -641,7 +643,7 @@ export class PolicyEngine {
decision = shellResult.decision; decision = shellResult.decision;
matchedRule = shellResult.rule; matchedRule = shellResult.rule;
} else { } else {
decision = this.applyNonInteractiveMode(this.defaultDecision); decision = this.defaultDecision;
} }
} }
@@ -697,7 +699,7 @@ export class PolicyEngine {
} }
return { return {
decision: this.applyNonInteractiveMode(decision), decision,
rule: matchedRule, rule: matchedRule,
}; };
} }
@@ -866,7 +868,7 @@ export class PolicyEngine {
continue; continue;
} else { } else {
// Unconditional rule for this tool // Unconditional rule for this tool
const decision = this.applyNonInteractiveMode(rule.decision); const decision = rule.decision;
staticallyExcluded = decision === PolicyDecision.DENY; staticallyExcluded = decision === PolicyDecision.DENY;
matchFound = true; matchFound = true;
break; break;
@@ -876,7 +878,7 @@ export class PolicyEngine {
if (!matchFound) { if (!matchFound) {
// Fallback to default decision if no rule matches // Fallback to default decision if no rule matches
const defaultDec = this.applyNonInteractiveMode(this.defaultDecision); const defaultDec = this.defaultDecision;
if (defaultDec === PolicyDecision.DENY) { if (defaultDec === PolicyDecision.DENY) {
staticallyExcluded = true; staticallyExcluded = true;
} }
@@ -889,12 +891,4 @@ export class PolicyEngine {
return excludedTools; 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;
}
} }