fix(core): scope tools.core wildcard deny to built-in tools#28365
fix(core): scope tools.core wildcard deny to built-in tools#28365vedhakoushik wants to merge 3 commits into
Conversation
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.<name>.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 google-gemini#28361 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Bellam vedha kousik <bellam.vedhakoushik@gmail.com>
|
📊 PR Size: size/L
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where configuring the core tools allowlist (tools.core) incorrectly disabled Model Context Protocol (MCP) tools. It introduces a builtinOnly flag on policy rules to ensure wildcard rules meant for built-in tools do not affect MCP tools, updating the policy engine, configuration logic, documentation, and tests. The review feedback identifies a potential edge case in ruleMatches where serverName might be undefined for MCP tools during static analysis (e.g., in getExcludedTools), and suggests checking the tool name prefix to prevent accidental exclusion of MCP tools.
| if ('builtinOnly' in rule && rule.builtinOnly && serverName !== undefined) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
In ruleMatches, the check to skip MCP tools for builtinOnly rules relies solely on serverName !== undefined. However, in some contexts (such as PolicyEngine.getExcludedTools() when toolMetadata is missing or incomplete), serverName may be undefined even for MCP tools. Since all MCP tools are prefixed with mcp_ (and thus satisfy isMcpToolName), we should also check isMcpToolName(toolCall.name) to ensure that builtinOnly rules are reliably scoped to built-in tools and do not accidentally match and deny MCP tools.
if (
'builtinOnly' in rule &&
rule.builtinOnly &&
(serverName !== undefined || (toolCall.name && isMcpToolName(toolCall.name)))
) {
return false;
}References
- Tool security policies are applied based on tool.name, not tool.kind.
There was a problem hiding this comment.
Good catch — confirmed and fixed in 36c4982. Traced it to a real path: PolicyEngine.getExcludedTools()'s toolMetadata param is optional, and even in production tool-registry.ts's buildToolMetadata() only attaches _serverName when tool instanceof DiscoveredMCPTool — any other MCP tool wrapper (or a caller that omits toolMetadata) would silently lose the protection.
builtinOnly now checks serverName !== undefined || isMcpToolName(toolCall.name), using the already-imported isMcpToolName() helper as the structural fallback that doesn't depend on caller-supplied metadata. Added regression tests for both the direct check() path and getExcludedTools() with no metadata provided.
gemini-cli
Bot
added
priority/p1
Addresses Gemini Code Assist review on google-gemini#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 <noreply@anthropic.com> Signed-off-by: Bellam vedha kousik <bellam.vedhakoushik@gmail.com>
Signed-off-by: Bellam vedha kousik <bellam.vedhakoushik@gmail.com>
|
Superseded by #28388 — same fix, clean re-submission without the Co-Authored-By trailer that was blocking the CLA check. |
1 participant
Summary
Fixes a bug where setting
tools.coreto any value — including[]— silently disabled every MCP tool, regardless of trust settings, because a wildcard DENY rule had no way to exclude MCP tools from matching it.Details
builtinOnlyfield toPolicyRule(types.ts). When set, the rule never matches MCP tool calls, regardless of itstoolNamepattern.ruleMatches()now short-circuitsbuiltinOnlyrules for any tool call with aserverName(i.e. an MCP tool).config.tsnow setsbuiltinOnly: true, sotools.corematches its documented scope ("Restrict the set of built-in tools") without also silently excluding MCP tools frommcpServers.<name>.trust,mcp.allowed, andmcp.autoAllowInHeadless.PolicyEngine.getExcludedTools()(which drives which tool declarations reach the model) shares the sameruleMatches()used for call-time checks, this one change fixes both the declaration-level exclusion and the call-time deny.tools.coreindocs/reference/configuration.mdto state explicitly that it does not affect MCP tools.Related Issues
Fixes #28361
How to Validate
settings.json:{ "mcpServers": { "github": { "command": "github-mcp-server", "args": ["stdio"], "trust": true } }, "tools": { "core": [] } }gemini --approval-mode yolo -p "Call any tool from the github MCP server"Tool "<name>" not found. After this fix: the MCP tool is discoverable and callable.npm test -w @google/gemini-cli-core -- src/policy/config.test.ts src/policy/policy-engine.test.tsPre-Merge Checklist
🤖 Authored with Claude Code. First-time contributor here — happy to adjust anything (e.g. naming of the
builtinOnlyfield) to match maintainer preference.