From 9472365e626db82b51135d40e2ece0ee47cc9a70 Mon Sep 17 00:00:00 2001 From: Bellam vedha kousik Date: Sun, 12 Jul 2026 16:45:10 +0530 Subject: [PATCH 1/3] fix(core): scope tools.core wildcard deny to built-in tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting settings.tools.core to any value, including [], added a wildcard '*' DENY rule with no way to exclude MCP tools from matching it. That DENY (priority 4.24) outranked every MCP-specific allow mechanism — mcpServers..trust (4.2), mcp.allowed, and mcp.autoAllowInHeadless (4.1) — so all MCP tools were silently denied regardless of trust settings. Because PolicyEngine.getExcludedTools() (which drives which tools are removed from the model's declarations) reuses the same rule matching as call-time checks, the tools weren't just denied at call time — their declarations never reached the model, so it blind-guessed tool names until exhausting maxSessionTurns. This also contradicted tools.core's documented scope: "Restrict the set of built-in tools." Add an opt-in `builtinOnly` field to PolicyRule. When set, the rule never matches MCP tools (serverName !== undefined), regardless of its toolName pattern — including the wildcard '*'. Set builtinOnly: true on the Core Tools Allowlist Enforcement rule in config.ts, so it stays scoped to built-in tools as documented while leaving MCP tools governed by their own trust/allow mechanisms. Because getExcludedTools() and the real-time check() share the same ruleMatches() function, this one change fixes both the declaration-level exclusion and the call-time deny. Added regression tests: builtinOnly matching in ruleMatches() and getExcludedTools(), a priority-ordering test reproducing the exact issue scenario (MCP trust ALLOW at 4.2 now wins over the builtinOnly DENY at 4.24), and end-to-end tests through createPolicyEngineConfig + PolicyEngine reproducing the issue's minimal repro (tools.core: [] plus a trusted MCP server). Also clarified the tools.core docs to state explicitly that it does not affect MCP tools. Fixes #28361 Co-Authored-By: Claude Sonnet 5 Signed-off-by: Bellam vedha kousik --- docs/reference/configuration.md | 6 +- packages/core/src/policy/config.test.ts | 88 +++++++++++++++++++ packages/core/src/policy/config.ts | 6 ++ .../core/src/policy/policy-engine.test.ts | 87 ++++++++++++++++++ packages/core/src/policy/policy-engine.ts | 8 ++ packages/core/src/policy/types.ts | 10 +++ 6 files changed, 204 insertions(+), 1 deletion(-) diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 293d99313c7..35173774948 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -1779,7 +1779,11 @@ their corresponding top-level category object in your `settings.json` file. - **Description:** Restrict the set of built-in tools with an allowlist. Match semantics mirror tools.allowed; see the built-in tools documentation for - available names. + available names. This setting only restricts built-in tools — it does not + affect MCP tools, which are governed separately by + `mcpServers..trust`, `mcp.allowed`, and `mcp.autoAllowInHeadless`. + Setting `tools.core` to `[]` disables all built-in tools but leaves MCP + tools unaffected. - **Default:** `undefined` - **Requires restart:** Yes diff --git a/packages/core/src/policy/config.test.ts b/packages/core/src/policy/config.test.ts index 01ddbc030c0..0fa8bd19528 100644 --- a/packages/core/src/policy/config.test.ts +++ b/packages/core/src/policy/config.test.ts @@ -21,6 +21,7 @@ import { clearEmittedPolicyWarnings, getPolicyDirectories, } from './config.js'; +import { PolicyEngine } from './policy-engine.js'; import { Storage } from '../config/storage.js'; import * as tomlLoader from './toml-loader.js'; import { coreEvents } from '../utils/events.js'; @@ -454,6 +455,93 @@ describe('createPolicyEngineConfig', () => { expect(excludedRule?.priority).toBe(4.9); // MCP excluded server }); + describe('Core Tools Allowlist (tools.core)', () => { + // Regression tests for a bug where any settings.tools.core value (even + // []) added a wildcard '*' DENY that silently excluded every MCP tool + // from the tool registry, outranking mcpServers..trust and + // mcp.allowed/autoAllowInHeadless — contradicting tools.core's documented + // scope of "restrict the set of built-in tools". + + it('should mark the Core Tools Allowlist Enforcement rule as builtinOnly', async () => { + const config = await createPolicyEngineConfig( + { tools: { core: [] } }, + ApprovalMode.DEFAULT, + MOCK_DEFAULT_DIR, + ); + + const enforcementRule = config.rules?.find( + (r) => r.source === 'Settings (Core Tools Allowlist Enforcement)', + ); + expect(enforcementRule).toBeDefined(); + expect(enforcementRule?.toolName).toBe('*'); + expect(enforcementRule?.decision).toBe(PolicyDecision.DENY); + expect(enforcementRule?.builtinOnly).toBe(true); + }); + + it('should still deny built-in tools not in the tools.core allowlist', async () => { + const config = await createPolicyEngineConfig( + { tools: { core: ['read_file'] } }, + ApprovalMode.DEFAULT, + MOCK_DEFAULT_DIR, + ); + const engine = new PolicyEngine(config); + + expect( + (await engine.check({ name: 'read_file' }, undefined)).decision, + ).toBe(PolicyDecision.ALLOW); + expect( + (await engine.check({ name: 'run_shell_command' }, undefined)).decision, + ).toBe(PolicyDecision.DENY); + }); + + it('should not deny a trusted MCP server tool when tools.core is set to []', async () => { + // Reproduces the issue's minimal repro: tools.core: [] plus a trusted + // MCP server should leave that server's tools callable. + const config = await createPolicyEngineConfig( + { + tools: { core: [] }, + mcpServers: { github: { trust: true } }, + }, + ApprovalMode.DEFAULT, + MOCK_DEFAULT_DIR, + ); + const engine = new PolicyEngine(config); + + expect( + (await engine.check({ name: 'mcp_github_get_pull_request' }, 'github')) + .decision, + ).toBe(PolicyDecision.ALLOW); + + // Built-in tools remain restricted to the (empty) core allowlist. + expect( + (await engine.check({ name: 'read_file' }, undefined)).decision, + ).toBe(PolicyDecision.DENY); + }); + + it('should not statically exclude a trusted MCP tool from getExcludedTools when tools.core is set', async () => { + // This is the deeper root cause: getExcludedTools() drives which tools + // are removed from the model's declarations entirely (not just denied + // at call time). A trusted MCP tool must not be excluded here either. + const config = await createPolicyEngineConfig( + { + tools: { core: [] }, + mcpServers: { github: { trust: true } }, + }, + ApprovalMode.DEFAULT, + MOCK_DEFAULT_DIR, + ); + const engine = new PolicyEngine(config); + + const excluded = engine.getExcludedTools( + new Map([['mcp_github_get_pull_request', { _serverName: 'github' }]]), + new Set(['read_file', 'mcp_github_get_pull_request']), + ); + + expect(excluded.has('mcp_github_get_pull_request')).toBe(false); + expect(excluded.has('read_file')).toBe(true); + }); + }); + it('should allow all tools in YOLO mode', async () => { const config = await createPolicyEngineConfig({}, ApprovalMode.YOLO); const rule = config.rules?.find( diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index ccee6d37159..58c89d3fa00 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -552,8 +552,14 @@ export async function createPolicyEngineConfig( // If core tools are restricted, we should add a default DENY rule for everything else // at a slightly lower priority than the explicit allows. + // Scoped to built-in tools only (builtinOnly): `tools.core` restricts the + // set of built-in tools, as documented. It must not also exclude MCP + // tools, which are governed by their own trust/allow mechanisms + // (mcpServers..trust, mcp.allowed, mcp.autoAllowInHeadless) — + // otherwise this wildcard DENY silently outranks all of them. rules.push({ toolName: '*', + builtinOnly: true, decision: PolicyDecision.DENY, priority: CORE_TOOLS_FLAG_PRIORITY - 0.01, source: 'Settings (Core Tools Allowlist Enforcement)', diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index b4d9f6b21d2..a8b4c224dcf 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -858,6 +858,75 @@ describe('PolicyEngine', () => { }); }); + describe('builtinOnly rules', () => { + it('should not match MCP tools even with a wildcard toolName', async () => { + engine = new PolicyEngine({ + rules: [ + { + toolName: '*', + builtinOnly: true, + decision: PolicyDecision.DENY, + priority: 10, + }, + ], + defaultDecision: PolicyDecision.ASK_USER, + }); + + // Built-in tool: the builtinOnly wildcard DENY applies. + expect( + (await engine.check({ name: 'read_file' }, undefined)).decision, + ).toBe(PolicyDecision.DENY); + + // MCP tool: builtinOnly excludes it from the rule, so it falls through + // to the engine's default decision instead of being denied. + expect( + (await engine.check({ name: 'mcp_server_tool' }, 'server')).decision, + ).toBe(PolicyDecision.ASK_USER); + }); + + it('should not affect rules without builtinOnly set', async () => { + engine = new PolicyEngine({ + rules: [{ toolName: '*', decision: PolicyDecision.DENY, priority: 10 }], + }); + + expect( + (await engine.check({ name: 'mcp_server_tool' }, 'server')).decision, + ).toBe(PolicyDecision.DENY); + }); + + // Regression test for the `tools.core` bug (settings.tools.core caused a + // builtinOnly-less wildcard DENY that outranked mcpServers..trust, + // silently disabling every MCP tool). Reproduces the priority ordering + // documented in the issue: the Core Tools Allowlist Enforcement DENY + // (4.24) sits above the MCP trust ALLOW (4.2), so without builtinOnly the + // MCP tool would be denied despite being trusted. + it('lets a higher-priority MCP trust ALLOW win over a lower-priority builtinOnly wildcard DENY', async () => { + engine = new PolicyEngine({ + rules: [ + { + toolName: 'mcp_*', + decision: PolicyDecision.ALLOW, + priority: 4.2, // TRUSTED_MCP_SERVER_PRIORITY + source: 'Settings (MCP Trusted)', + }, + { + toolName: '*', + builtinOnly: true, + decision: PolicyDecision.DENY, + priority: 4.24, // CORE_TOOLS_FLAG_PRIORITY - 0.01 + source: 'Settings (Core Tools Allowlist Enforcement)', + }, + ], + defaultDecision: PolicyDecision.ASK_USER, + }); + + expect( + (await engine.check({ name: 'mcp_github_get_pull_request' }, 'github')) + .decision, + ).toBe(PolicyDecision.ALLOW); + }); + }); + describe('complex scenarios', () => { it('should handle multiple matching rules with different priorities', async () => { const rules: PolicyRule[] = [ @@ -2800,6 +2869,24 @@ describe('PolicyEngine', () => { ]), expected: ['mcp_server_search'], }, + { + // Regression test for the `tools.core` bug: a builtinOnly wildcard + // DENY (as config.ts sets on the Core Tools Allowlist Enforcement + // rule) must exclude built-in tools but leave MCP tool declarations + // alone, so MCP tools stay governed by their own trust/allow rules. + name: 'should exclude only built-in tools for a builtinOnly wildcard * in getExcludedTools', + rules: [ + { + toolName: '*', + builtinOnly: true, + decision: PolicyDecision.DENY, + priority: 10, + }, + ], + allToolNames: ['toolA', 'toolB', 'mcp_server_toolC'], + metadata: new Map([['mcp_server_toolC', { _serverName: 'server' }]]), + expected: ['toolA', 'toolB'], + }, ]; it.each(testCases)( diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index a3b9aa09927..123e601f4fd 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -117,6 +117,14 @@ function ruleMatches( } } + // A builtinOnly rule never matches MCP tools, regardless of its toolName + // pattern. This lets a blanket '*' rule (e.g. a default-deny paired with + // an allowlist) stay scoped to built-in tools without also excluding MCP + // tools, which are governed separately by their own trust/allow rules. + if ('builtinOnly' in rule && rule.builtinOnly && serverName !== undefined) { + return false; + } + // Check tool name if specified if (rule.toolName !== undefined) { // Support wildcard patterns: "mcp_serverName_*" matches "mcp_serverName_anyTool" diff --git a/packages/core/src/policy/types.ts b/packages/core/src/policy/types.ts index 5af37549034..dda5e7a0a3e 100644 --- a/packages/core/src/policy/types.ts +++ b/packages/core/src/policy/types.ts @@ -136,6 +136,16 @@ export interface PolicyRule { */ mcpName?: string; + /** + * If true, this rule only matches built-in tools and never matches MCP + * tools, even when `toolName` is the wildcard `'*'`. Use this to scope a + * blanket rule (e.g. a default-deny for an allowlist) to built-in tools + * without also excluding MCP tools, which should remain governed by their + * own trust/allow mechanisms (`mcpServers..trust`, `mcp.allowed`, + * `mcp.autoAllowInHeadless`). + */ + builtinOnly?: boolean; + /** * Pattern to match against tool arguments. * Can be used for more fine-grained control. From 36c49820fb5bf48b3fe8355b6efe55c2f7e7b255 Mon Sep 17 00:00:00 2001 From: Bellam vedha kousik Date: Sun, 12 Jul 2026 17:11:57 +0530 Subject: [PATCH 2/3] fix(core): fall back to mcp_ name prefix when serverName is undefined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Gemini Code Assist review on #28365: the builtinOnly check relied solely on serverName !== undefined, but serverName comes from caller-supplied tool metadata (PolicyEngine.getExcludedTools()'s toolMetadata param) and can be undefined even for a genuine MCP tool call — e.g. toolMetadata is omitted, or a tool has no entry in the map because it wasn't recognized as DiscoveredMCPTool upstream (tool-registry.ts's buildToolMetadata()). In that case a builtinOnly wildcard DENY would still match the MCP tool and reintroduce the original bug for that code path. All MCP tools are structurally guaranteed to carry the mcp_ prefix (MCP_TOOL_PREFIX / isMcpToolName(), already imported and used elsewhere in this file), independent of any metadata being correctly populated. builtinOnly now checks serverName OR isMcpToolName(toolCall.name), so it stays reliable even when metadata is missing or incomplete. Added regression tests for both the direct check() path and the getExcludedTools() path with no metadata provided. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Bellam vedha kousik --- .../core/src/policy/policy-engine.test.ts | 43 +++++++++++++++++++ packages/core/src/policy/policy-engine.ts | 13 +++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index a8b4c224dcf..6d350063f6c 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -884,6 +884,30 @@ describe('PolicyEngine', () => { ).toBe(PolicyDecision.ASK_USER); }); + it('should fall back to the mcp_ name prefix when serverName is undefined', async () => { + // serverName is derived from caller-supplied tool metadata and can be + // undefined even for a genuine MCP tool call (missing/incomplete + // metadata, a tool not recognized as DiscoveredMCPTool, etc). The + // mcp_ name prefix is a structural guarantee that doesn't depend on + // that metadata, so builtinOnly must also honor it directly. + engine = new PolicyEngine({ + rules: [ + { + toolName: '*', + builtinOnly: true, + decision: PolicyDecision.DENY, + priority: 10, + }, + ], + defaultDecision: PolicyDecision.ASK_USER, + }); + + // No serverName passed, but the tool name is still mcp_-prefixed. + expect( + (await engine.check({ name: 'mcp_server_tool' }, undefined)).decision, + ).toBe(PolicyDecision.ASK_USER); + }); + it('should not affect rules without builtinOnly set', async () => { engine = new PolicyEngine({ rules: [{ toolName: '*', decision: PolicyDecision.DENY, priority: 10 }], @@ -2887,6 +2911,25 @@ describe('PolicyEngine', () => { metadata: new Map([['mcp_server_toolC', { _serverName: 'server' }]]), expected: ['toolA', 'toolB'], }, + { + // Same as above, but with no metadata provided at all (as happens + // when toolMetadata is omitted, or a tool has no entry in the map, + // e.g. it isn't recognized as DiscoveredMCPTool upstream). serverName + // resolves to undefined for every tool in this case, so builtinOnly + // must fall back to the mcp_ name prefix to still protect the MCP + // tool from the wildcard DENY. + name: 'should exclude only built-in tools for a builtinOnly wildcard * in getExcludedTools even without metadata', + rules: [ + { + toolName: '*', + builtinOnly: true, + decision: PolicyDecision.DENY, + priority: 10, + }, + ], + allToolNames: ['toolA', 'toolB', 'mcp_server_toolC'], + expected: ['toolA', 'toolB'], + }, ]; it.each(testCases)( diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index 123e601f4fd..9e4dd078251 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -121,7 +121,18 @@ function ruleMatches( // pattern. This lets a blanket '*' rule (e.g. a default-deny paired with // an allowlist) stay scoped to built-in tools without also excluding MCP // tools, which are governed separately by their own trust/allow rules. - if ('builtinOnly' in rule && rule.builtinOnly && serverName !== undefined) { + // Check both serverName (from caller-supplied metadata) and the tool + // name's own mcp_ prefix: serverName can be undefined even for a real + // MCP tool when metadata is missing or incomplete (e.g. a tool not + // recognized as DiscoveredMCPTool, or a caller that omits toolMetadata), + // so name-prefix is the structural fallback that doesn't depend on any + // external metadata being correctly populated. + if ( + 'builtinOnly' in rule && + rule.builtinOnly && + (serverName !== undefined || + (toolCall.name !== undefined && isMcpToolName(toolCall.name))) + ) { return false; } From 620a497b2abcbbe2d9138b4eae84953b7589d0d7 Mon Sep 17 00:00:00 2001 From: Bellam vedha kousik Date: Sun, 12 Jul 2026 17:19:10 +0530 Subject: [PATCH 3/3] retrigger cla check Signed-off-by: Bellam vedha kousik