mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-12 21:03:05 -07:00
feat(core): add support for custom deny messages in policy rules (#17427)
This commit is contained in:
@@ -210,6 +210,10 @@ decision = "ask_user"
|
|||||||
# The priority of the rule, from 0 to 999.
|
# The priority of the rule, from 0 to 999.
|
||||||
priority = 10
|
priority = 10
|
||||||
|
|
||||||
|
# (Optional) A custom message to display when a tool call is denied by this rule.
|
||||||
|
# This message is returned to the model and user, useful for explaining *why* it was denied.
|
||||||
|
deny_message = "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"]
|
||||||
```
|
```
|
||||||
@@ -282,6 +286,7 @@ only the `mcpName`.
|
|||||||
mcpName = "untrusted-server"
|
mcpName = "untrusted-server"
|
||||||
decision = "deny"
|
decision = "deny"
|
||||||
priority = 500
|
priority = 500
|
||||||
|
deny_message = "This server is not trusted by the admin."
|
||||||
```
|
```
|
||||||
|
|
||||||
## Default policies
|
## Default policies
|
||||||
|
|||||||
@@ -173,6 +173,22 @@ allow_redirection = true
|
|||||||
expect(result.errors).toHaveLength(0);
|
expect(result.errors).toHaveLength(0);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should parse deny_message property', async () => {
|
||||||
|
const result = await runLoadPoliciesFromToml(`
|
||||||
|
[[rule]]
|
||||||
|
toolName = "rm"
|
||||||
|
decision = "deny"
|
||||||
|
priority = 100
|
||||||
|
deny_message = "Deletion is permanent"
|
||||||
|
`);
|
||||||
|
|
||||||
|
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(result.errors).toHaveLength(0);
|
||||||
|
});
|
||||||
|
|
||||||
it('should support modes property for Tier 2 and Tier 3 policies', async () => {
|
it('should support modes property for Tier 2 and Tier 3 policies', async () => {
|
||||||
await fs.writeFile(
|
await fs.writeFile(
|
||||||
path.join(tempDir, 'tier2.toml'),
|
path.join(tempDir, 'tier2.toml'),
|
||||||
|
|||||||
@@ -46,6 +46,7 @@ const PolicyRuleSchema = z.object({
|
|||||||
}),
|
}),
|
||||||
modes: z.array(z.nativeEnum(ApprovalMode)).optional(),
|
modes: z.array(z.nativeEnum(ApprovalMode)).optional(),
|
||||||
allow_redirection: z.boolean().optional(),
|
allow_redirection: z.boolean().optional(),
|
||||||
|
deny_message: z.string().optional(),
|
||||||
});
|
});
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -347,6 +348,7 @@ export async function loadPoliciesFromToml(
|
|||||||
modes: rule.modes,
|
modes: rule.modes,
|
||||||
allowRedirection: rule.allow_redirection,
|
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,
|
||||||
};
|
};
|
||||||
|
|
||||||
// Compile regex pattern
|
// Compile regex pattern
|
||||||
|
|||||||
@@ -142,6 +142,12 @@ export interface PolicyRule {
|
|||||||
* e.g. "my-policies.toml", "Settings (MCP Trusted)", etc.
|
* e.g. "my-policies.toml", "Settings (MCP Trusted)", etc.
|
||||||
*/
|
*/
|
||||||
source?: string;
|
source?: string;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Optional message to display when this rule results in a DENY decision.
|
||||||
|
* This message will be returned to the model/user.
|
||||||
|
*/
|
||||||
|
denyMessage?: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface SafetyCheckerRule {
|
export interface SafetyCheckerRule {
|
||||||
|
|||||||
@@ -36,8 +36,8 @@ describe('policy.ts', () => {
|
|||||||
tool: { name: 'test-tool' },
|
tool: { name: 'test-tool' },
|
||||||
} as ValidatingToolCall;
|
} as ValidatingToolCall;
|
||||||
|
|
||||||
const decision = await checkPolicy(toolCall, mockConfig);
|
const result = await checkPolicy(toolCall, mockConfig);
|
||||||
expect(decision).toBe(PolicyDecision.ALLOW);
|
expect(result.decision).toBe(PolicyDecision.ALLOW);
|
||||||
expect(mockPolicyEngine.check).toHaveBeenCalledWith(
|
expect(mockPolicyEngine.check).toHaveBeenCalledWith(
|
||||||
{ name: 'test-tool', args: {} },
|
{ name: 'test-tool', args: {} },
|
||||||
undefined,
|
undefined,
|
||||||
@@ -102,8 +102,8 @@ describe('policy.ts', () => {
|
|||||||
tool: { name: 'test-tool' },
|
tool: { name: 'test-tool' },
|
||||||
} as ValidatingToolCall;
|
} as ValidatingToolCall;
|
||||||
|
|
||||||
const decision = await checkPolicy(toolCall, mockConfig);
|
const result = await checkPolicy(toolCall, mockConfig);
|
||||||
expect(decision).toBe(PolicyDecision.DENY);
|
expect(result.decision).toBe(PolicyDecision.DENY);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should return ASK_USER without throwing in interactive mode', async () => {
|
it('should return ASK_USER without throwing in interactive mode', async () => {
|
||||||
@@ -121,8 +121,8 @@ describe('policy.ts', () => {
|
|||||||
tool: { name: 'test-tool' },
|
tool: { name: 'test-tool' },
|
||||||
} as ValidatingToolCall;
|
} as ValidatingToolCall;
|
||||||
|
|
||||||
const decision = await checkPolicy(toolCall, mockConfig);
|
const result = await checkPolicy(toolCall, mockConfig);
|
||||||
expect(decision).toBe(PolicyDecision.ASK_USER);
|
expect(result.decision).toBe(PolicyDecision.ASK_USER);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -4,7 +4,11 @@
|
|||||||
* SPDX-License-Identifier: Apache-2.0
|
* SPDX-License-Identifier: Apache-2.0
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { ApprovalMode, PolicyDecision } from '../policy/types.js';
|
import {
|
||||||
|
ApprovalMode,
|
||||||
|
PolicyDecision,
|
||||||
|
type CheckResult,
|
||||||
|
} from '../policy/types.js';
|
||||||
import type { Config } from '../config/config.js';
|
import type { Config } from '../config/config.js';
|
||||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||||
import {
|
import {
|
||||||
@@ -28,19 +32,25 @@ import type { ValidatingToolCall } from './types.js';
|
|||||||
export async function checkPolicy(
|
export async function checkPolicy(
|
||||||
toolCall: ValidatingToolCall,
|
toolCall: ValidatingToolCall,
|
||||||
config: Config,
|
config: Config,
|
||||||
): Promise<PolicyDecision> {
|
): Promise<CheckResult> {
|
||||||
const serverName =
|
const serverName =
|
||||||
toolCall.tool instanceof DiscoveredMCPTool
|
toolCall.tool instanceof DiscoveredMCPTool
|
||||||
? toolCall.tool.serverName
|
? toolCall.tool.serverName
|
||||||
: undefined;
|
: undefined;
|
||||||
|
|
||||||
const { decision } = await config
|
const result = await config
|
||||||
.getPolicyEngine()
|
.getPolicyEngine()
|
||||||
.check(
|
.check(
|
||||||
{ name: toolCall.request.name, args: toolCall.request.args },
|
{ name: toolCall.request.name, args: toolCall.request.args },
|
||||||
serverName,
|
serverName,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
const { decision } = result;
|
||||||
|
|
||||||
|
/*
|
||||||
|
* Return the full check result including the rule that matched.
|
||||||
|
* This is necessary to access metadata like custom deny messages.
|
||||||
|
*/
|
||||||
if (decision === PolicyDecision.ASK_USER) {
|
if (decision === PolicyDecision.ASK_USER) {
|
||||||
if (!config.isInteractive()) {
|
if (!config.isInteractive()) {
|
||||||
throw new Error(
|
throw new Error(
|
||||||
@@ -51,7 +61,7 @@ export async function checkPolicy(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return decision;
|
return { decision, rule: result.rule };
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -194,6 +194,10 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
|
|
||||||
vi.mocked(resolveConfirmation).mockReset();
|
vi.mocked(resolveConfirmation).mockReset();
|
||||||
vi.mocked(checkPolicy).mockReset();
|
vi.mocked(checkPolicy).mockReset();
|
||||||
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.ALLOW,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
vi.mocked(updatePolicy).mockReset();
|
vi.mocked(updatePolicy).mockReset();
|
||||||
|
|
||||||
mockExecutor = {
|
mockExecutor = {
|
||||||
@@ -663,7 +667,10 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should update state to error with POLICY_VIOLATION if Policy returns DENY', async () => {
|
it('should update state to error with POLICY_VIOLATION if Policy returns DENY', async () => {
|
||||||
vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.DENY);
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.DENY,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
await scheduler.schedule(req1, signal);
|
await scheduler.schedule(req1, signal);
|
||||||
|
|
||||||
@@ -678,6 +685,36 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
expect(mockExecutor.execute).not.toHaveBeenCalled();
|
expect(mockExecutor.execute).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should include denyMessage in error response if present', async () => {
|
||||||
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.DENY,
|
||||||
|
rule: {
|
||||||
|
decision: PolicyDecision.DENY,
|
||||||
|
denyMessage: 'Custom denial reason',
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
await scheduler.schedule(req1, signal);
|
||||||
|
|
||||||
|
expect(mockStateManager.updateStatus).toHaveBeenCalledWith(
|
||||||
|
'call-1',
|
||||||
|
'error',
|
||||||
|
expect.objectContaining({
|
||||||
|
errorType: ToolErrorType.POLICY_VIOLATION,
|
||||||
|
responseParts: expect.arrayContaining([
|
||||||
|
expect.objectContaining({
|
||||||
|
functionResponse: expect.objectContaining({
|
||||||
|
response: {
|
||||||
|
error:
|
||||||
|
'Tool execution denied by policy. Custom denial reason',
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
]),
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it('should handle errors from checkPolicy (e.g. non-interactive ASK_USER)', async () => {
|
it('should handle errors from checkPolicy (e.g. non-interactive ASK_USER)', async () => {
|
||||||
const error = new Error('Not interactive');
|
const error = new Error('Not interactive');
|
||||||
vi.mocked(checkPolicy).mockRejectedValue(error);
|
vi.mocked(checkPolicy).mockRejectedValue(error);
|
||||||
@@ -701,7 +738,10 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should bypass confirmation and ProceedOnce if Policy returns ALLOW (YOLO/AllowedTools)', async () => {
|
it('should bypass confirmation and ProceedOnce if Policy returns ALLOW (YOLO/AllowedTools)', async () => {
|
||||||
vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ALLOW);
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.ALLOW,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
// Provide a mock execute to finish the loop
|
// Provide a mock execute to finish the loop
|
||||||
mockExecutor.execute.mockResolvedValue({
|
mockExecutor.execute.mockResolvedValue({
|
||||||
@@ -754,8 +794,14 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
|
|
||||||
// First call requires confirmation, second is auto-approved (simulating policy update)
|
// First call requires confirmation, second is auto-approved (simulating policy update)
|
||||||
vi.mocked(checkPolicy)
|
vi.mocked(checkPolicy)
|
||||||
.mockResolvedValueOnce(PolicyDecision.ASK_USER)
|
.mockResolvedValueOnce({
|
||||||
.mockResolvedValueOnce(PolicyDecision.ALLOW);
|
decision: PolicyDecision.ASK_USER,
|
||||||
|
rule: undefined,
|
||||||
|
})
|
||||||
|
.mockResolvedValueOnce({
|
||||||
|
decision: PolicyDecision.ALLOW,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
vi.mocked(resolveConfirmation).mockResolvedValue({
|
vi.mocked(resolveConfirmation).mockResolvedValue({
|
||||||
outcome: ToolConfirmationOutcome.ProceedAlways,
|
outcome: ToolConfirmationOutcome.ProceedAlways,
|
||||||
@@ -777,7 +823,10 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should call resolveConfirmation and updatePolicy when ASK_USER', async () => {
|
it('should call resolveConfirmation and updatePolicy when ASK_USER', async () => {
|
||||||
vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER);
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.ASK_USER,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
const resolution = {
|
const resolution = {
|
||||||
outcome: ToolConfirmationOutcome.ProceedAlways,
|
outcome: ToolConfirmationOutcome.ProceedAlways,
|
||||||
@@ -820,7 +869,10 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should cancel and NOT execute if resolveConfirmation returns Cancel', async () => {
|
it('should cancel and NOT execute if resolveConfirmation returns Cancel', async () => {
|
||||||
vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER);
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.ASK_USER,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
const resolution = {
|
const resolution = {
|
||||||
outcome: ToolConfirmationOutcome.Cancel,
|
outcome: ToolConfirmationOutcome.Cancel,
|
||||||
@@ -842,7 +894,10 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should mark as cancelled (not errored) when abort happens during confirmation error', async () => {
|
it('should mark as cancelled (not errored) when abort happens during confirmation error', async () => {
|
||||||
vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER);
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.ASK_USER,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
// Simulate shouldConfirmExecute logic throwing while aborted
|
// Simulate shouldConfirmExecute logic throwing while aborted
|
||||||
vi.mocked(resolveConfirmation).mockImplementation(async () => {
|
vi.mocked(resolveConfirmation).mockImplementation(async () => {
|
||||||
@@ -865,7 +920,10 @@ describe('Scheduler (Orchestrator)', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should preserve confirmation details (e.g. diff) in cancelled state', async () => {
|
it('should preserve confirmation details (e.g. diff) in cancelled state', async () => {
|
||||||
vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER);
|
vi.mocked(checkPolicy).mockResolvedValue({
|
||||||
|
decision: PolicyDecision.ASK_USER,
|
||||||
|
rule: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
const confirmDetails = {
|
const confirmDetails = {
|
||||||
type: 'edit' as const,
|
type: 'edit' as const,
|
||||||
|
|||||||
@@ -404,15 +404,16 @@ export class Scheduler {
|
|||||||
const callId = toolCall.request.callId;
|
const callId = toolCall.request.callId;
|
||||||
|
|
||||||
// Policy & Security
|
// Policy & Security
|
||||||
const decision = await checkPolicy(toolCall, this.config);
|
const { decision, rule } = await checkPolicy(toolCall, this.config);
|
||||||
|
|
||||||
if (decision === PolicyDecision.DENY) {
|
if (decision === PolicyDecision.DENY) {
|
||||||
|
const denyMessage = rule?.denyMessage ? ` ${rule.denyMessage}` : '';
|
||||||
this.state.updateStatus(
|
this.state.updateStatus(
|
||||||
callId,
|
callId,
|
||||||
'error',
|
'error',
|
||||||
createErrorResponse(
|
createErrorResponse(
|
||||||
toolCall.request,
|
toolCall.request,
|
||||||
new Error('Tool execution denied by policy.'),
|
new Error(`Tool execution denied by policy.${denyMessage}`),
|
||||||
ToolErrorType.POLICY_VIOLATION,
|
ToolErrorType.POLICY_VIOLATION,
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
|
|||||||
Reference in New Issue
Block a user