From 25e14e21bb4f13546f34e7ee9a6d1a5089eaa403 Mon Sep 17 00:00:00 2001 From: Bellam vedha kousik Date: Mon, 13 Jul 2026 13:11:44 +0530 Subject: [PATCH] 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, regardless of its toolName pattern — including the wildcard '*'. All MCP tools are structurally guaranteed to carry the mcp_ prefix (MCP_TOOL_PREFIX / isMcpToolName()), independent of any metadata being correctly populated, so builtinOnly checks serverName OR isMcpToolName(toolCall.name) to stay reliable even when metadata is missing or incomplete. 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, and end-to-end tests through createPolicyEngineConfig + PolicyEngine reproducing the issue's minimal repro. Also clarified the tools.core docs to state explicitly that it does not affect MCP tools. Fixes #28361 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 | 130 ++++++++++++++++++ packages/core/src/policy/policy-engine.ts | 19 +++ packages/core/src/policy/types.ts | 10 ++ 6 files changed, 258 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..6d350063f6c 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -858,6 +858,99 @@ 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 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 }], + }); + + 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 +2893,43 @@ 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'], + }, + { + // 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 a3b9aa09927..9e4dd078251 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -117,6 +117,25 @@ 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. + // 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; + } + // 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.