mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-04-20 18:14:29 -07:00
fix(core)!: Force policy config to specify toolName (#23330)
This commit is contained in:
@@ -87,6 +87,7 @@ export async function loadConfig(
|
||||
approvalMode === ApprovalMode.YOLO
|
||||
? [
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: PRIORITY_YOLO_ALLOW_ALL,
|
||||
modes: [ApprovalMode.YOLO],
|
||||
|
||||
@@ -381,6 +381,7 @@ describe('Policy Engine Integration Tests', () => {
|
||||
// Add a manual rule with annotations to the config
|
||||
config.rules = config.rules || [];
|
||||
config.rules.push({
|
||||
toolName: '*',
|
||||
toolAnnotations: { readOnlyHint: true },
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: 10,
|
||||
|
||||
@@ -166,7 +166,7 @@ export class AppRig {
|
||||
private sessionId: string;
|
||||
|
||||
private pendingConfirmations = new Map<string, PendingConfirmation>();
|
||||
private breakpointTools = new Set<string | undefined>();
|
||||
private breakpointTools = new Set<string>();
|
||||
private lastAwaitedConfirmation: PendingConfirmation | undefined;
|
||||
|
||||
/**
|
||||
@@ -436,11 +436,7 @@ export class AppRig {
|
||||
MockShellExecutionService.setMockCommands(commands);
|
||||
}
|
||||
|
||||
setToolPolicy(
|
||||
toolName: string | undefined,
|
||||
decision: PolicyDecision,
|
||||
priority = 10,
|
||||
) {
|
||||
setToolPolicy(toolName: string, decision: PolicyDecision, priority = 10) {
|
||||
if (!this.config) throw new Error('AppRig not initialized');
|
||||
this.config.getPolicyEngine().addRule({
|
||||
toolName,
|
||||
@@ -450,27 +446,20 @@ export class AppRig {
|
||||
});
|
||||
}
|
||||
|
||||
setBreakpoint(toolName: string | string[] | undefined) {
|
||||
setBreakpoint(toolName: string | string[]) {
|
||||
if (Array.isArray(toolName)) {
|
||||
for (const name of toolName) {
|
||||
this.setBreakpoint(name);
|
||||
}
|
||||
} else {
|
||||
// Use undefined toolName to create a global rule if '*' is provided
|
||||
const actualToolName = toolName === '*' ? undefined : toolName;
|
||||
this.setToolPolicy(actualToolName, PolicyDecision.ASK_USER, 100);
|
||||
this.setToolPolicy(toolName, PolicyDecision.ASK_USER, 100);
|
||||
this.breakpointTools.add(toolName);
|
||||
}
|
||||
}
|
||||
|
||||
removeToolPolicy(toolName?: string, source = 'AppRig Override') {
|
||||
removeToolPolicy(toolName: string, source = 'AppRig Override') {
|
||||
if (!this.config) throw new Error('AppRig not initialized');
|
||||
// Map '*' back to undefined for policy removal
|
||||
const actualToolName = toolName === '*' ? undefined : toolName;
|
||||
this.config
|
||||
.getPolicyEngine()
|
||||
|
||||
.removeRulesForTool(actualToolName as string, source);
|
||||
this.config.getPolicyEngine().removeRulesForTool(toolName, source);
|
||||
this.breakpointTools.delete(toolName);
|
||||
}
|
||||
|
||||
|
||||
@@ -314,7 +314,7 @@ describe('createPolicyEngineConfig', () => {
|
||||
it('should allow all tools in YOLO mode', async () => {
|
||||
const config = await createPolicyEngineConfig({}, ApprovalMode.YOLO);
|
||||
const rule = config.rules?.find(
|
||||
(r) => r.decision === PolicyDecision.ALLOW && !r.toolName,
|
||||
(r) => r.decision === PolicyDecision.ALLOW && r.toolName === '*',
|
||||
);
|
||||
expect(rule).toBeDefined();
|
||||
expect(rule?.priority).toBeCloseTo(1.998, 5);
|
||||
@@ -513,7 +513,7 @@ describe('createPolicyEngineConfig', () => {
|
||||
);
|
||||
|
||||
const wildcardRule = config.rules?.find(
|
||||
(r) => !r.toolName && r.decision === PolicyDecision.ALLOW,
|
||||
(r) => r.toolName === '*' && r.decision === PolicyDecision.ALLOW,
|
||||
);
|
||||
const writeToolRules = config.rules?.filter(
|
||||
(r) =>
|
||||
|
||||
@@ -71,6 +71,7 @@ denyMessage = "You are not currently in Plan Mode. Use enter_plan_mode first to
|
||||
# Catch-All: Deny everything by default in Plan mode.
|
||||
|
||||
[[rule]]
|
||||
toolName = "*"
|
||||
decision = "deny"
|
||||
priority = 60
|
||||
modes = ["plan"]
|
||||
@@ -79,6 +80,7 @@ denyMessage = "You are in Plan Mode with access to read-only tools. Execution of
|
||||
# Explicitly Allow Read-Only Tools in Plan mode.
|
||||
|
||||
[[rule]]
|
||||
toolName = "*"
|
||||
mcpName = "*"
|
||||
toolAnnotations = { readOnlyHint = true }
|
||||
decision = "ask_user"
|
||||
|
||||
@@ -49,6 +49,7 @@ interactive = true
|
||||
|
||||
# Allow everything else in YOLO mode
|
||||
[[rule]]
|
||||
toolName = "*"
|
||||
decision = "allow"
|
||||
priority = 998
|
||||
modes = ["yolo"]
|
||||
|
||||
@@ -267,7 +267,7 @@ describe('PolicyEngine', () => {
|
||||
|
||||
it('should apply wildcard rules (no toolName)', async () => {
|
||||
const rules: PolicyRule[] = [
|
||||
{ decision: PolicyDecision.DENY }, // Applies to all tools
|
||||
{ toolName: '*', decision: PolicyDecision.DENY }, // Applies to all tools
|
||||
{ toolName: 'safe-tool', decision: PolicyDecision.ALLOW, priority: 10 },
|
||||
];
|
||||
|
||||
@@ -692,7 +692,7 @@ describe('PolicyEngine', () => {
|
||||
describe('complex scenarios', () => {
|
||||
it('should handle multiple matching rules with different priorities', async () => {
|
||||
const rules: PolicyRule[] = [
|
||||
{ decision: PolicyDecision.DENY, priority: 0 }, // Default deny all
|
||||
{ toolName: '*', decision: PolicyDecision.DENY, priority: 0 }, // Default deny all
|
||||
{ toolName: 'shell', decision: PolicyDecision.ASK_USER, priority: 5 },
|
||||
{
|
||||
toolName: 'shell',
|
||||
@@ -1617,6 +1617,7 @@ describe('PolicyEngine', () => {
|
||||
|
||||
const fixedRules: PolicyRule[] = [
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 1.06,
|
||||
modes: [ApprovalMode.PLAN],
|
||||
@@ -1647,6 +1648,7 @@ describe('PolicyEngine', () => {
|
||||
const { splitCommands } = await import('../utils/shell-utils.js');
|
||||
const rules: PolicyRule[] = [
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: 999,
|
||||
modes: [ApprovalMode.YOLO],
|
||||
@@ -1685,6 +1687,7 @@ describe('PolicyEngine', () => {
|
||||
priority: 2000, // Very high priority DENY (e.g. Admin)
|
||||
},
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: 999,
|
||||
modes: [ApprovalMode.YOLO],
|
||||
@@ -1978,10 +1981,12 @@ describe('PolicyEngine', () => {
|
||||
describe('addChecker', () => {
|
||||
it('should add a new checker and maintain priority order', () => {
|
||||
const checker1: SafetyCheckerRule = {
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'checker1' },
|
||||
priority: 5,
|
||||
};
|
||||
const checker2: SafetyCheckerRule = {
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'checker2' },
|
||||
priority: 10,
|
||||
};
|
||||
@@ -2034,6 +2039,39 @@ describe('PolicyEngine', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('should match global wildcard (*) for checkers', async () => {
|
||||
const rules: PolicyRule[] = [
|
||||
{ toolName: '*', decision: PolicyDecision.ALLOW },
|
||||
];
|
||||
const globalChecker: SafetyCheckerRule = {
|
||||
checker: { type: 'external', name: 'global' },
|
||||
toolName: '*',
|
||||
};
|
||||
|
||||
engine = new PolicyEngine(
|
||||
{ rules, checkers: [globalChecker] },
|
||||
mockCheckerRunner,
|
||||
);
|
||||
|
||||
vi.mocked(mockCheckerRunner.runChecker).mockResolvedValue({
|
||||
decision: SafetyCheckDecision.ALLOW,
|
||||
});
|
||||
|
||||
await engine.check({ name: 'any_tool' }, undefined);
|
||||
expect(mockCheckerRunner.runChecker).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
expect.objectContaining({ name: 'global' }),
|
||||
);
|
||||
|
||||
vi.mocked(mockCheckerRunner.runChecker).mockClear();
|
||||
|
||||
await engine.check({ name: 'mcp_server_tool' }, 'server');
|
||||
expect(mockCheckerRunner.runChecker).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
expect.objectContaining({ name: 'global' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('should support wildcard patterns for checkers', async () => {
|
||||
const rules: PolicyRule[] = [
|
||||
{
|
||||
@@ -2070,6 +2108,7 @@ describe('PolicyEngine', () => {
|
||||
];
|
||||
const checkers: SafetyCheckerRule[] = [
|
||||
{
|
||||
toolName: '*',
|
||||
checker: {
|
||||
type: 'in-process',
|
||||
name: InProcessCheckerType.ALLOWED_PATH,
|
||||
@@ -2095,6 +2134,7 @@ describe('PolicyEngine', () => {
|
||||
];
|
||||
const checkers: SafetyCheckerRule[] = [
|
||||
{
|
||||
toolName: '*',
|
||||
checker: {
|
||||
type: 'in-process',
|
||||
name: InProcessCheckerType.ALLOWED_PATH,
|
||||
@@ -2119,6 +2159,7 @@ describe('PolicyEngine', () => {
|
||||
];
|
||||
const checkers: SafetyCheckerRule[] = [
|
||||
{
|
||||
toolName: '*',
|
||||
checker: {
|
||||
type: 'in-process',
|
||||
name: InProcessCheckerType.ALLOWED_PATH,
|
||||
@@ -2143,6 +2184,7 @@ describe('PolicyEngine', () => {
|
||||
];
|
||||
const checkers: SafetyCheckerRule[] = [
|
||||
{
|
||||
toolName: '*',
|
||||
checker: {
|
||||
type: 'in-process',
|
||||
name: InProcessCheckerType.ALLOWED_PATH,
|
||||
@@ -2320,6 +2362,7 @@ describe('PolicyEngine', () => {
|
||||
name: 'should respect wildcard ALLOW rules (e.g. YOLO mode)',
|
||||
rules: [
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: 999,
|
||||
modes: [ApprovalMode.YOLO],
|
||||
@@ -2396,6 +2439,7 @@ describe('PolicyEngine', () => {
|
||||
},
|
||||
{
|
||||
// Simulates the global deny in Plan Mode
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 60,
|
||||
modes: [ApprovalMode.PLAN],
|
||||
@@ -2506,6 +2550,7 @@ describe('PolicyEngine', () => {
|
||||
engine = new PolicyEngine({
|
||||
rules: [
|
||||
{
|
||||
toolName: '*',
|
||||
toolAnnotations: { destructiveHint: true },
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 10,
|
||||
@@ -2523,6 +2568,7 @@ describe('PolicyEngine', () => {
|
||||
engine = new PolicyEngine({
|
||||
rules: [
|
||||
{
|
||||
toolName: '*',
|
||||
toolAnnotations: { destructiveHint: true },
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 10,
|
||||
@@ -2544,6 +2590,7 @@ describe('PolicyEngine', () => {
|
||||
engine = new PolicyEngine({
|
||||
rules: [
|
||||
{
|
||||
toolName: '*',
|
||||
toolAnnotations: { destructiveHint: true },
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 10,
|
||||
@@ -2615,6 +2662,7 @@ describe('PolicyEngine', () => {
|
||||
priority: 70,
|
||||
},
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 60,
|
||||
},
|
||||
@@ -2661,6 +2709,7 @@ describe('PolicyEngine', () => {
|
||||
priority: 70,
|
||||
},
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 60,
|
||||
},
|
||||
@@ -2701,6 +2750,7 @@ describe('PolicyEngine', () => {
|
||||
priority: 70,
|
||||
},
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 60,
|
||||
},
|
||||
@@ -2782,6 +2832,7 @@ describe('PolicyEngine', () => {
|
||||
modes: [ApprovalMode.PLAN],
|
||||
},
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 60,
|
||||
modes: [ApprovalMode.PLAN],
|
||||
@@ -2857,6 +2908,7 @@ describe('PolicyEngine', () => {
|
||||
modes: [ApprovalMode.YOLO],
|
||||
},
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: PRIORITY_YOLO_ALLOW_ALL,
|
||||
modes: [ApprovalMode.YOLO],
|
||||
@@ -2884,6 +2936,7 @@ describe('PolicyEngine', () => {
|
||||
modes: [ApprovalMode.YOLO],
|
||||
},
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: PRIORITY_YOLO_ALLOW_ALL,
|
||||
modes: [ApprovalMode.YOLO],
|
||||
@@ -2907,6 +2960,7 @@ describe('PolicyEngine', () => {
|
||||
it('should allow activate_skill but deny shell commands in Plan Mode', async () => {
|
||||
const rules: PolicyRule[] = [
|
||||
{
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
priority: 60,
|
||||
modes: [ApprovalMode.PLAN],
|
||||
@@ -3110,14 +3164,17 @@ describe('PolicyEngine', () => {
|
||||
describe('removeCheckersByTier', () => {
|
||||
it('should remove checkers matching a specific tier', () => {
|
||||
engine.addChecker({
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'c1' },
|
||||
priority: 1.1,
|
||||
});
|
||||
engine.addChecker({
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'c2' },
|
||||
priority: 1.9,
|
||||
});
|
||||
engine.addChecker({
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'c3' },
|
||||
priority: 2.5,
|
||||
});
|
||||
@@ -3135,14 +3192,17 @@ describe('PolicyEngine', () => {
|
||||
describe('removeCheckersBySource', () => {
|
||||
it('should remove checkers matching a specific source', () => {
|
||||
engine.addChecker({
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'c1' },
|
||||
source: 'sourceA',
|
||||
});
|
||||
engine.addChecker({
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'c2' },
|
||||
source: 'sourceB',
|
||||
});
|
||||
engine.addChecker({
|
||||
toolName: '*',
|
||||
checker: { type: 'external', name: 'c3' },
|
||||
source: 'sourceA',
|
||||
});
|
||||
@@ -3161,6 +3221,7 @@ describe('PolicyEngine', () => {
|
||||
engine = new PolicyEngine({
|
||||
rules: [
|
||||
{
|
||||
toolName: '*',
|
||||
toolAnnotations: { readOnlyHint: true },
|
||||
decision: PolicyDecision.ALLOW,
|
||||
priority: 10,
|
||||
|
||||
@@ -88,14 +88,14 @@ function ruleMatches(
|
||||
}
|
||||
|
||||
// Check subagent if specified (only for PolicyRule, SafetyCheckerRule doesn't have it)
|
||||
if ('subagent' in rule && rule.subagent) {
|
||||
if ('subagent' in rule && rule.subagent !== undefined) {
|
||||
if (rule.subagent !== subagent) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
// Strictly enforce mcpName identity if the rule dictates it
|
||||
if (rule.mcpName) {
|
||||
if (rule.mcpName !== undefined) {
|
||||
if (rule.mcpName === '*') {
|
||||
// Rule requires it to be ANY MCP tool
|
||||
if (serverName === undefined) return false;
|
||||
@@ -106,7 +106,7 @@ function ruleMatches(
|
||||
}
|
||||
|
||||
// Check tool name if specified
|
||||
if (rule.toolName) {
|
||||
if (rule.toolName !== undefined) {
|
||||
// Support wildcard patterns: "mcp_serverName_*" matches "mcp_serverName_anyTool"
|
||||
if (rule.toolName === '*') {
|
||||
// Match all tools
|
||||
@@ -203,6 +203,40 @@ export class PolicyEngine {
|
||||
this.hookCheckers = (config.hookCheckers ?? []).sort(
|
||||
(a, b) => (b.priority ?? 0) - (a.priority ?? 0),
|
||||
);
|
||||
|
||||
// Validate rules
|
||||
for (const rule of this.rules) {
|
||||
if (rule.toolName === undefined || rule.toolName === '') {
|
||||
throw new Error(
|
||||
`Invalid policy rule: toolName is required. Use '*' for all tools. Rule source: ${rule.source || 'unknown'}`,
|
||||
);
|
||||
}
|
||||
if (rule.mcpName === '') {
|
||||
throw new Error(
|
||||
`Invalid policy rule: mcpName is required if specified (cannot be empty). Rule source: ${rule.source || 'unknown'}`,
|
||||
);
|
||||
}
|
||||
if (rule.subagent === '') {
|
||||
throw new Error(
|
||||
`Invalid policy rule: subagent is required if specified (cannot be empty). Rule source: ${rule.source || 'unknown'}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Validate checkers
|
||||
for (const checker of this.checkers) {
|
||||
if (checker.toolName === undefined || checker.toolName === '') {
|
||||
throw new Error(
|
||||
`Invalid safety checker rule: toolName is required. Use '*' for all tools. Checker source: ${checker.source || 'unknown'}`,
|
||||
);
|
||||
}
|
||||
if (checker.mcpName === '') {
|
||||
throw new Error(
|
||||
`Invalid safety checker rule: mcpName is required if specified (cannot be empty). Checker source: ${checker.source || 'unknown'}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
this.defaultDecision = config.defaultDecision ?? PolicyDecision.ASK_USER;
|
||||
this.nonInteractive = config.nonInteractive ?? false;
|
||||
this.disableAlwaysAllow = config.disableAlwaysAllow ?? false;
|
||||
|
||||
@@ -123,6 +123,7 @@ priority = 70
|
||||
it('should transform mcpName = "*" to wildcard toolName', async () => {
|
||||
const result = await runLoadPoliciesFromToml(`
|
||||
[[rule]]
|
||||
toolName = "*"
|
||||
mcpName = "*"
|
||||
decision = "ask_user"
|
||||
priority = 10
|
||||
@@ -476,6 +477,21 @@ name = "allowed-path"
|
||||
});
|
||||
|
||||
describe('Negative Tests', () => {
|
||||
it('should return a schema_validation error if toolName is missing in safety_checker', async () => {
|
||||
const result = await runLoadPoliciesFromToml(`
|
||||
[[safety_checker]]
|
||||
priority = 100
|
||||
[safety_checker.checker]
|
||||
type = "in-process"
|
||||
name = "allowed-path"
|
||||
`);
|
||||
expect(result.errors).toHaveLength(1);
|
||||
const error = result.errors[0];
|
||||
expect(error.errorType).toBe('schema_validation');
|
||||
expect(error.details).toContain('toolName');
|
||||
expect(error.details).toContain('Invalid input');
|
||||
});
|
||||
|
||||
it('should return a schema_validation error if priority is missing', async () => {
|
||||
const result = await runLoadPoliciesFromToml(`
|
||||
[[rule]]
|
||||
@@ -571,6 +587,19 @@ priority = 100
|
||||
expect(error.details).toContain('decision');
|
||||
});
|
||||
|
||||
it('should return a schema_validation error if toolName is missing', async () => {
|
||||
const result = await runLoadPoliciesFromToml(`
|
||||
[[rule]]
|
||||
decision = "allow"
|
||||
priority = 100
|
||||
`);
|
||||
expect(result.errors).toHaveLength(1);
|
||||
const error = result.errors[0];
|
||||
expect(error.errorType).toBe('schema_validation');
|
||||
expect(error.details).toContain('toolName');
|
||||
expect(error.details).toContain('Invalid input');
|
||||
});
|
||||
|
||||
it('should return a schema_validation error if toolName is not a string or array', async () => {
|
||||
const result = await runLoadPoliciesFromToml(`
|
||||
[[rule]]
|
||||
@@ -795,9 +824,10 @@ priority = 100
|
||||
expect(result.rules).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('should not warn for catch-all rules (no toolName)', async () => {
|
||||
it('should not warn for catch-all rules (toolName = "*")', async () => {
|
||||
const result = await runLoadPoliciesFromToml(`
|
||||
[[rule]]
|
||||
toolName = "*"
|
||||
decision = "deny"
|
||||
priority = 100
|
||||
`);
|
||||
@@ -855,6 +885,7 @@ priority = 100
|
||||
'Should have loaded a rule with toolAnnotations',
|
||||
).toBeDefined();
|
||||
expect(annotationRule!.toolName).toBe('mcp_*');
|
||||
expect(annotationRule!.mcpName).toBe('*');
|
||||
expect(annotationRule!.toolAnnotations).toEqual({
|
||||
readOnlyHint: true,
|
||||
});
|
||||
@@ -866,7 +897,7 @@ priority = 100
|
||||
const denyRule = result.rules.find(
|
||||
(r) =>
|
||||
r.decision === PolicyDecision.DENY &&
|
||||
r.toolName === undefined &&
|
||||
r.toolName === '*' &&
|
||||
r.denyMessage?.includes('Plan Mode'),
|
||||
);
|
||||
expect(
|
||||
@@ -1089,13 +1120,12 @@ priority = 100
|
||||
expect(warnings).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('should skip rules without toolName', () => {
|
||||
it('should skip wildcard rules (matching all tools)', () => {
|
||||
const warnings = validateMcpPolicyToolNames(
|
||||
'my-server',
|
||||
['tool1'],
|
||||
[{ toolName: undefined }],
|
||||
[{ toolName: '*', mcpName: 'my-server' }],
|
||||
);
|
||||
|
||||
expect(warnings).toHaveLength(0);
|
||||
});
|
||||
|
||||
|
||||
@@ -37,7 +37,7 @@ const MAX_TYPO_DISTANCE = 3;
|
||||
* Schema for a single policy rule in the TOML file (before transformation).
|
||||
*/
|
||||
const PolicyRuleSchema = z.object({
|
||||
toolName: z.union([z.string(), z.array(z.string())]).optional(),
|
||||
toolName: z.union([z.string(), z.array(z.string())]),
|
||||
subagent: z.string().optional(),
|
||||
mcpName: z.string().optional(),
|
||||
argsPattern: z.string().optional(),
|
||||
@@ -73,7 +73,7 @@ const PolicyRuleSchema = z.object({
|
||||
* Schema for a single safety checker rule in the TOML file.
|
||||
*/
|
||||
const SafetyCheckerRuleSchema = z.object({
|
||||
toolName: z.union([z.string(), z.array(z.string())]).optional(),
|
||||
toolName: z.union([z.string(), z.array(z.string())]),
|
||||
mcpName: z.string().optional(),
|
||||
argsPattern: z.string().optional(),
|
||||
commandPrefix: z.union([z.string(), z.array(z.string())]).optional(),
|
||||
@@ -411,14 +411,28 @@ export async function loadPoliciesFromToml(
|
||||
// Validate tool names in rules
|
||||
for (let i = 0; i < tomlRules.length; i++) {
|
||||
const rule = tomlRules[i];
|
||||
|
||||
const toolNamesRaw: string[] = Array.isArray(rule.toolName)
|
||||
? rule.toolName
|
||||
: [rule.toolName];
|
||||
|
||||
if (toolNamesRaw.some((name) => name === '')) {
|
||||
errors.push({
|
||||
filePath,
|
||||
fileName: file,
|
||||
tier: tierName,
|
||||
ruleIndex: i,
|
||||
errorType: 'rule_validation',
|
||||
message: 'Invalid policy rule: toolName cannot be empty string',
|
||||
details: `Rule #${i + 1} contains an empty toolName string. Use "*" to match all tools.`,
|
||||
});
|
||||
continue;
|
||||
}
|
||||
|
||||
// We no longer skip MCP-scoped rules because we need to specifically
|
||||
// warn users if they use deprecated "__" syntax for MCP tool names
|
||||
|
||||
const toolNames: string[] = rule.toolName
|
||||
? Array.isArray(rule.toolName)
|
||||
? rule.toolName
|
||||
: [rule.toolName]
|
||||
: [];
|
||||
const toolNames: string[] = toolNamesRaw;
|
||||
|
||||
for (const name of toolNames) {
|
||||
const warning = validateToolName(name, i);
|
||||
@@ -448,15 +462,13 @@ export async function loadPoliciesFromToml(
|
||||
|
||||
// For each argsPattern, expand toolName arrays
|
||||
return argsPatterns.flatMap((argsPattern) => {
|
||||
const toolNames: Array<string | undefined> = rule.toolName
|
||||
? Array.isArray(rule.toolName)
|
||||
? rule.toolName
|
||||
: [rule.toolName]
|
||||
: [undefined];
|
||||
const toolNames: string[] = Array.isArray(rule.toolName)
|
||||
? rule.toolName
|
||||
: [rule.toolName];
|
||||
|
||||
// Create a policy rule for each tool name
|
||||
return toolNames.map((toolName) => {
|
||||
let effectiveToolName: string | undefined = toolName;
|
||||
let effectiveToolName: string = toolName;
|
||||
const mcpName = rule.mcpName;
|
||||
|
||||
if (mcpName) {
|
||||
@@ -535,13 +547,28 @@ export async function loadPoliciesFromToml(
|
||||
const tomlCheckerRules = validationResult.data.safety_checker ?? [];
|
||||
for (let i = 0; i < tomlCheckerRules.length; i++) {
|
||||
const checker = tomlCheckerRules[i];
|
||||
|
||||
const checkerToolNamesRaw: string[] = Array.isArray(checker.toolName)
|
||||
? checker.toolName
|
||||
: [checker.toolName];
|
||||
|
||||
if (checkerToolNamesRaw.some((name) => name === '')) {
|
||||
errors.push({
|
||||
filePath,
|
||||
fileName: file,
|
||||
tier: tierName,
|
||||
ruleIndex: i,
|
||||
errorType: 'rule_validation',
|
||||
message:
|
||||
'Invalid safety checker rule: toolName cannot be empty string',
|
||||
details: `Checker #${i + 1} contains an empty toolName string. Use "*" to match all tools.`,
|
||||
});
|
||||
continue;
|
||||
}
|
||||
|
||||
if (checker.mcpName) continue;
|
||||
|
||||
const checkerToolNames: string[] = checker.toolName
|
||||
? Array.isArray(checker.toolName)
|
||||
? checker.toolName
|
||||
: [checker.toolName]
|
||||
: [];
|
||||
const checkerToolNames: string[] = checkerToolNamesRaw;
|
||||
|
||||
for (const name of checkerToolNames) {
|
||||
const warning = validateToolName(name, i);
|
||||
@@ -572,15 +599,13 @@ export async function loadPoliciesFromToml(
|
||||
);
|
||||
|
||||
return argsPatterns.flatMap((argsPattern) => {
|
||||
const toolNames: Array<string | undefined> = checker.toolName
|
||||
? Array.isArray(checker.toolName)
|
||||
? checker.toolName
|
||||
: [checker.toolName]
|
||||
: [undefined];
|
||||
const toolNames: string[] = Array.isArray(checker.toolName)
|
||||
? checker.toolName
|
||||
: [checker.toolName];
|
||||
|
||||
return toolNames.map((toolName) => {
|
||||
let effectiveToolName: string | undefined;
|
||||
if (checker.mcpName && toolName) {
|
||||
let effectiveToolName: string;
|
||||
if (checker.mcpName && toolName !== '*') {
|
||||
effectiveToolName = `${MCP_TOOL_PREFIX}${checker.mcpName}_${toolName}`;
|
||||
} else if (checker.mcpName) {
|
||||
effectiveToolName = `${MCP_TOOL_PREFIX}${checker.mcpName}_*`;
|
||||
@@ -675,7 +700,7 @@ export function validateMcpPolicyToolNames(
|
||||
serverName: string,
|
||||
discoveredToolNames: string[],
|
||||
policyRules: ReadonlyArray<{
|
||||
toolName?: string;
|
||||
toolName: string;
|
||||
mcpName?: string;
|
||||
source?: string;
|
||||
}>,
|
||||
|
||||
@@ -106,9 +106,9 @@ export interface PolicyRule {
|
||||
|
||||
/**
|
||||
* The name of the tool this rule applies to.
|
||||
* If undefined, the rule applies to all tools.
|
||||
* Use '*' to match all tools.
|
||||
*/
|
||||
toolName?: string;
|
||||
toolName: string;
|
||||
|
||||
/**
|
||||
* The name of the subagent this rule applies to.
|
||||
@@ -182,9 +182,9 @@ export interface PolicyRule {
|
||||
export interface SafetyCheckerRule {
|
||||
/**
|
||||
* The name of the tool this rule applies to.
|
||||
* If undefined, the rule applies to all tools.
|
||||
* Use '*' to match all tools.
|
||||
*/
|
||||
toolName?: string;
|
||||
toolName: string;
|
||||
|
||||
/**
|
||||
* Identifies the MCP server this rule applies to.
|
||||
|
||||
@@ -760,6 +760,7 @@ describe('policy.ts', () => {
|
||||
|
||||
(mockConfig as unknown as { config: Config }).config = mockConfig;
|
||||
const rule = {
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
denyMessage: 'Custom Deny',
|
||||
};
|
||||
|
||||
@@ -642,6 +642,7 @@ describe('Scheduler (Orchestrator)', () => {
|
||||
vi.mocked(checkPolicy).mockResolvedValue({
|
||||
decision: PolicyDecision.DENY,
|
||||
rule: {
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
denyMessage: 'Custom denial reason',
|
||||
},
|
||||
@@ -693,7 +694,7 @@ describe('Scheduler (Orchestrator)', () => {
|
||||
it('should return POLICY_VIOLATION error type when denied in Plan Mode', async () => {
|
||||
vi.mocked(checkPolicy).mockResolvedValue({
|
||||
decision: PolicyDecision.DENY,
|
||||
rule: { decision: PolicyDecision.DENY },
|
||||
rule: { toolName: '*', decision: PolicyDecision.DENY },
|
||||
});
|
||||
|
||||
mockConfig.getApprovalMode.mockReturnValue(ApprovalMode.PLAN);
|
||||
@@ -722,7 +723,11 @@ describe('Scheduler (Orchestrator)', () => {
|
||||
const customMessage = 'Custom Plan Mode Deny';
|
||||
vi.mocked(checkPolicy).mockResolvedValue({
|
||||
decision: PolicyDecision.DENY,
|
||||
rule: { decision: PolicyDecision.DENY, denyMessage: customMessage },
|
||||
rule: {
|
||||
toolName: '*',
|
||||
decision: PolicyDecision.DENY,
|
||||
denyMessage: customMessage,
|
||||
},
|
||||
});
|
||||
|
||||
mockConfig.getApprovalMode.mockReturnValue(ApprovalMode.PLAN);
|
||||
|
||||
@@ -1755,7 +1755,11 @@ export interface McpContext {
|
||||
setUserInteractedWithMcp?(): void;
|
||||
isTrustedFolder(): boolean;
|
||||
getPolicyEngine?(): {
|
||||
getRules(): ReadonlyArray<{ toolName?: string; source?: string }>;
|
||||
getRules(): ReadonlyArray<{
|
||||
toolName: string;
|
||||
mcpName?: string;
|
||||
source?: string;
|
||||
}>;
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -99,6 +99,10 @@ describe('formatMcpToolName', () => {
|
||||
expect(formatMcpToolName('github', '*')).toBe('mcp_github_*');
|
||||
});
|
||||
|
||||
it('should handle both server and tool wildcards', () => {
|
||||
expect(formatMcpToolName('*', '*')).toBe('mcp_*');
|
||||
});
|
||||
|
||||
it('should handle undefined toolName as a tool-level wildcard', () => {
|
||||
expect(formatMcpToolName('github')).toBe('mcp_github_*');
|
||||
});
|
||||
|
||||
@@ -80,11 +80,11 @@ export function formatMcpToolName(
|
||||
serverName: string,
|
||||
toolName?: string,
|
||||
): string {
|
||||
if (serverName === '*' && !toolName) {
|
||||
if (serverName === '*' && (toolName === undefined || toolName === '*')) {
|
||||
return `${MCP_TOOL_PREFIX}*`;
|
||||
} else if (serverName === '*') {
|
||||
return `${MCP_TOOL_PREFIX}*_${toolName}`;
|
||||
} else if (!toolName) {
|
||||
} else if (toolName === undefined || toolName === '*') {
|
||||
return `${MCP_TOOL_PREFIX}${serverName}_*`;
|
||||
} else {
|
||||
return `${MCP_TOOL_PREFIX}${serverName}_${toolName}`;
|
||||
|
||||
Reference in New Issue
Block a user