diff --git a/docs/cli/plan-mode.md b/docs/cli/plan-mode.md index 5299bb3463..2163e4fcd1 100644 --- a/docs/cli/plan-mode.md +++ b/docs/cli/plan-mode.md @@ -200,6 +200,7 @@ your specific environment. ```toml [[rule]] +toolName = "*" mcpName = "*" toolAnnotations = { readOnlyHint = true } decision = "allow" diff --git a/docs/reference/policy-engine.md b/docs/reference/policy-engine.md index 1b9575475a..c9fc482ea7 100644 --- a/docs/reference/policy-engine.md +++ b/docs/reference/policy-engine.md @@ -413,6 +413,7 @@ registered MCP server. This is useful for setting category-wide defaults. ```toml # Ask user for any tool call from any MCP server [[rule]] +toolName = "*" mcpName = "*" decision = "ask_user" priority = 10 diff --git a/packages/a2a-server/src/config/config.ts b/packages/a2a-server/src/config/config.ts index 97243c88d8..1fe55258fc 100644 --- a/packages/a2a-server/src/config/config.ts +++ b/packages/a2a-server/src/config/config.ts @@ -87,6 +87,7 @@ export async function loadConfig( approvalMode === ApprovalMode.YOLO ? [ { + toolName: '*', decision: PolicyDecision.ALLOW, priority: PRIORITY_YOLO_ALLOW_ALL, modes: [ApprovalMode.YOLO], diff --git a/packages/cli/src/config/policy-engine.integration.test.ts b/packages/cli/src/config/policy-engine.integration.test.ts index 2e74a28201..3b2a34ca69 100644 --- a/packages/cli/src/config/policy-engine.integration.test.ts +++ b/packages/cli/src/config/policy-engine.integration.test.ts @@ -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, diff --git a/packages/cli/src/test-utils/AppRig.tsx b/packages/cli/src/test-utils/AppRig.tsx index a735677631..9475861950 100644 --- a/packages/cli/src/test-utils/AppRig.tsx +++ b/packages/cli/src/test-utils/AppRig.tsx @@ -166,7 +166,7 @@ export class AppRig { private sessionId: string; private pendingConfirmations = new Map(); - private breakpointTools = new Set(); + private breakpointTools = new Set(); 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); } diff --git a/packages/core/src/policy/config.test.ts b/packages/core/src/policy/config.test.ts index c4204e3c6c..7e39fe41dd 100644 --- a/packages/core/src/policy/config.test.ts +++ b/packages/core/src/policy/config.test.ts @@ -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) => diff --git a/packages/core/src/policy/policies/plan.toml b/packages/core/src/policy/policies/plan.toml index b9efd50db7..b6ddef72ef 100644 --- a/packages/core/src/policy/policies/plan.toml +++ b/packages/core/src/policy/policies/plan.toml @@ -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" diff --git a/packages/core/src/policy/policies/yolo.toml b/packages/core/src/policy/policies/yolo.toml index 0516484acd..5e2a194d2e 100644 --- a/packages/core/src/policy/policies/yolo.toml +++ b/packages/core/src/policy/policies/yolo.toml @@ -49,6 +49,7 @@ interactive = true # Allow everything else in YOLO mode [[rule]] +toolName = "*" decision = "allow" priority = 998 modes = ["yolo"] diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index 4e53418907..eb39d6ed8d 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -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, diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index cb114b7c7f..c35c9c5d4f 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -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; diff --git a/packages/core/src/policy/toml-loader.test.ts b/packages/core/src/policy/toml-loader.test.ts index 224450f2a2..6835e200b4 100644 --- a/packages/core/src/policy/toml-loader.test.ts +++ b/packages/core/src/policy/toml-loader.test.ts @@ -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); }); diff --git a/packages/core/src/policy/toml-loader.ts b/packages/core/src/policy/toml-loader.ts index 7f52dacc9f..977e8a399a 100644 --- a/packages/core/src/policy/toml-loader.ts +++ b/packages/core/src/policy/toml-loader.ts @@ -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 = 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 = 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; }>, diff --git a/packages/core/src/policy/types.ts b/packages/core/src/policy/types.ts index 5cd668ef4e..494956c364 100644 --- a/packages/core/src/policy/types.ts +++ b/packages/core/src/policy/types.ts @@ -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. diff --git a/packages/core/src/scheduler/policy.test.ts b/packages/core/src/scheduler/policy.test.ts index abcfc422cd..84e77d0166 100644 --- a/packages/core/src/scheduler/policy.test.ts +++ b/packages/core/src/scheduler/policy.test.ts @@ -760,6 +760,7 @@ describe('policy.ts', () => { (mockConfig as unknown as { config: Config }).config = mockConfig; const rule = { + toolName: '*', decision: PolicyDecision.DENY, denyMessage: 'Custom Deny', }; diff --git a/packages/core/src/scheduler/scheduler.test.ts b/packages/core/src/scheduler/scheduler.test.ts index 3ad99c397b..a72ed45852 100644 --- a/packages/core/src/scheduler/scheduler.test.ts +++ b/packages/core/src/scheduler/scheduler.test.ts @@ -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); diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index 58b7b6c8e2..fdd8bb7008 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -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; + }>; }; } diff --git a/packages/core/src/tools/mcp-tool.test.ts b/packages/core/src/tools/mcp-tool.test.ts index 4bb76e2e98..ac43adbc8c 100644 --- a/packages/core/src/tools/mcp-tool.test.ts +++ b/packages/core/src/tools/mcp-tool.test.ts @@ -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_*'); }); diff --git a/packages/core/src/tools/mcp-tool.ts b/packages/core/src/tools/mcp-tool.ts index 195a78ec61..42b8ae7cea 100644 --- a/packages/core/src/tools/mcp-tool.ts +++ b/packages/core/src/tools/mcp-tool.ts @@ -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}`;