feat(security): add disableAlwaysAllow setting to disable auto-approvals (#21941)

This commit is contained in:
Gal Zahavi
2026-03-13 16:02:09 -07:00
committed by GitHub
parent b0d151bd65
commit b49fc8122d
20 changed files with 352 additions and 63 deletions
+2 -13
View File
@@ -16,6 +16,7 @@ import {
type PolicyRule,
type PolicySettings,
type SafetyCheckerRule,
ALWAYS_ALLOW_PRIORITY_OFFSET,
} from './types.js';
import type { PolicyEngine } from './policy-engine.js';
import { loadPoliciesFromToml, type PolicyFileError } from './toml-loader.js';
@@ -66,19 +67,6 @@ export const WORKSPACE_POLICY_TIER = 3;
export const USER_POLICY_TIER = 4;
export const ADMIN_POLICY_TIER = 5;
/**
* The fractional priority of "Always allow" rules (e.g., 950/1000).
* Higher fraction within a tier wins.
*/
export const ALWAYS_ALLOW_PRIORITY_FRACTION = 950;
/**
* The fractional priority offset for "Always allow" rules (e.g., 0.95).
* This ensures consistency between in-memory rules and persisted rules.
*/
export const ALWAYS_ALLOW_PRIORITY_OFFSET =
ALWAYS_ALLOW_PRIORITY_FRACTION / 1000;
// Specific priority offsets and derived priorities for dynamic/settings rules.
export const MCP_EXCLUDED_PRIORITY = USER_POLICY_TIER + 0.9;
@@ -535,6 +523,7 @@ export async function createPolicyEngineConfig(
checkers,
defaultDecision: PolicyDecision.ASK_USER,
approvalMode,
disableAlwaysAllow: settings.disableAlwaysAllow,
};
}
@@ -14,6 +14,7 @@ import {
InProcessCheckerType,
ApprovalMode,
PRIORITY_SUBAGENT_TOOL,
ALWAYS_ALLOW_PRIORITY_FRACTION,
} from './types.js';
import type { FunctionCall } from '@google/genai';
import { SafetyCheckDecision } from '../safety/protocol.js';
@@ -3229,4 +3230,116 @@ describe('PolicyEngine', () => {
expect(hookCheckers[1].priority).toBe(5);
});
});
describe('disableAlwaysAllow', () => {
it('should ignore "Always Allow" rules when disableAlwaysAllow is true', async () => {
const alwaysAllowRule: PolicyRule = {
toolName: 'test-tool',
decision: PolicyDecision.ALLOW,
priority: 3 + ALWAYS_ALLOW_PRIORITY_FRACTION / 1000, // 3.95
source: 'Dynamic (Confirmed)',
};
const engine = new PolicyEngine({
rules: [alwaysAllowRule],
disableAlwaysAllow: true,
defaultDecision: PolicyDecision.ASK_USER,
});
const result = await engine.check(
{ name: 'test-tool', args: {} },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ASK_USER);
});
it('should respect "Always Allow" rules when disableAlwaysAllow is false', async () => {
const alwaysAllowRule: PolicyRule = {
toolName: 'test-tool',
decision: PolicyDecision.ALLOW,
priority: 3 + ALWAYS_ALLOW_PRIORITY_FRACTION / 1000, // 3.95
source: 'Dynamic (Confirmed)',
};
const engine = new PolicyEngine({
rules: [alwaysAllowRule],
disableAlwaysAllow: false,
defaultDecision: PolicyDecision.ASK_USER,
});
const result = await engine.check(
{ name: 'test-tool', args: {} },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
it('should NOT ignore other rules when disableAlwaysAllow is true', async () => {
const normalRule: PolicyRule = {
toolName: 'test-tool',
decision: PolicyDecision.ALLOW,
priority: 1.5, // Not a .950 fraction
source: 'Normal Rule',
};
const engine = new PolicyEngine({
rules: [normalRule],
disableAlwaysAllow: true,
defaultDecision: PolicyDecision.ASK_USER,
});
const result = await engine.check(
{ name: 'test-tool', args: {} },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
});
describe('getExcludedTools with disableAlwaysAllow', () => {
it('should exclude tool if an Always Allow rule says ALLOW but disableAlwaysAllow is true (falling back to DENY)', async () => {
// To prove the ALWAYS_ALLOW rule is ignored, we set the default decision to DENY.
// If the rule was honored, the decision would be ALLOW (tool not excluded).
// Since it's ignored, it falls back to the default DENY (tool is excluded).
// In the real app, it usually falls back to ASK_USER, but ASK_USER also doesn't
// exclude the tool, so we use DENY here purely to make the test observable.
const alwaysAllowRule: PolicyRule = {
toolName: 'test-tool',
decision: PolicyDecision.ALLOW,
priority: 3 + ALWAYS_ALLOW_PRIORITY_FRACTION / 1000,
};
const engine = new PolicyEngine({
rules: [alwaysAllowRule],
disableAlwaysAllow: true,
defaultDecision: PolicyDecision.DENY,
});
const excluded = engine.getExcludedTools(
undefined,
new Set(['test-tool']),
);
expect(excluded.has('test-tool')).toBe(true);
});
it('should NOT exclude tool if ALWAYS_ALLOW is enabled and rule says ALLOW', async () => {
const alwaysAllowRule: PolicyRule = {
toolName: 'test-tool',
decision: PolicyDecision.ALLOW,
priority: 3 + ALWAYS_ALLOW_PRIORITY_FRACTION / 1000,
};
const engine = new PolicyEngine({
rules: [alwaysAllowRule],
disableAlwaysAllow: false,
defaultDecision: PolicyDecision.DENY,
});
const excluded = engine.getExcludedTools(
undefined,
new Set(['test-tool']),
);
expect(excluded.has('test-tool')).toBe(false);
});
});
});
+18
View File
@@ -13,6 +13,7 @@ import {
type HookCheckerRule,
ApprovalMode,
type CheckResult,
ALWAYS_ALLOW_PRIORITY_FRACTION,
} from './types.js';
import { stableStringify } from './stable-stringify.js';
import { debugLogger } from '../utils/debugLogger.js';
@@ -154,6 +155,7 @@ export class PolicyEngine {
private hookCheckers: HookCheckerRule[];
private readonly defaultDecision: PolicyDecision;
private readonly nonInteractive: boolean;
private readonly disableAlwaysAllow: boolean;
private readonly checkerRunner?: CheckerRunner;
private approvalMode: ApprovalMode;
@@ -169,6 +171,7 @@ export class PolicyEngine {
);
this.defaultDecision = config.defaultDecision ?? PolicyDecision.ASK_USER;
this.nonInteractive = config.nonInteractive ?? false;
this.disableAlwaysAllow = config.disableAlwaysAllow ?? false;
this.checkerRunner = checkerRunner;
this.approvalMode = config.approvalMode ?? ApprovalMode.DEFAULT;
}
@@ -187,6 +190,13 @@ export class PolicyEngine {
return this.approvalMode;
}
private isAlwaysAllowRule(rule: PolicyRule): boolean {
return (
rule.priority !== undefined &&
Math.round((rule.priority % 1) * 1000) === ALWAYS_ALLOW_PRIORITY_FRACTION
);
}
private shouldDowngradeForRedirection(
command: string,
allowRedirection?: boolean,
@@ -422,6 +432,10 @@ export class PolicyEngine {
}
for (const rule of this.rules) {
if (this.disableAlwaysAllow && this.isAlwaysAllowRule(rule)) {
continue;
}
const match = toolCallsToTry.some((tc) =>
ruleMatches(
rule,
@@ -684,6 +698,10 @@ export class PolicyEngine {
// Evaluate rules in priority order (they are already sorted in constructor)
for (const rule of this.rules) {
if (this.disableAlwaysAllow && this.isAlwaysAllowRule(rule)) {
continue;
}
// Create a copy of the rule without argsPattern to see if it targets the tool
// regardless of the runtime arguments it might receive.
const ruleWithoutArgs: PolicyRule = { ...rule, argsPattern: undefined };
+19
View File
@@ -285,6 +285,11 @@ export interface PolicyEngineConfig {
*/
nonInteractive?: boolean;
/**
* Whether to ignore "Always Allow" rules.
*/
disableAlwaysAllow?: boolean;
/**
* Whether to allow hooks to execute.
* When false, all hooks are denied.
@@ -314,6 +319,7 @@ export interface PolicySettings {
// Admin provided policies that will supplement the ADMIN level policies
adminPolicyPaths?: string[];
workspacePoliciesDir?: string;
disableAlwaysAllow?: boolean;
}
export interface CheckResult {
@@ -326,3 +332,16 @@ export interface CheckResult {
* Effective priority matching Tier 1 (Default) read-only tools.
*/
export const PRIORITY_SUBAGENT_TOOL = 1.05;
/**
* The fractional priority of "Always allow" rules (e.g., 950/1000).
* Higher fraction within a tier wins.
*/
export const ALWAYS_ALLOW_PRIORITY_FRACTION = 950;
/**
* The fractional priority offset for "Always allow" rules (e.g., 0.95).
* This ensures consistency between in-memory rules and persisted rules.
*/
export const ALWAYS_ALLOW_PRIORITY_OFFSET =
ALWAYS_ALLOW_PRIORITY_FRACTION / 1000;