feat(core): support tool annotations and global wildcards in policies - #19671
feat(core): support tool annotations and global wildcards in policies#19671Sikandar1310291 wants to merge 1 commit into
Conversation
Summary of ChangesHello @Sikandar1310291, 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 significantly enhances the policy engine's flexibility and configurability by introducing support for global wildcards and tool annotations. These additions allow for more dynamic and expressive policy definitions, particularly for managing tool access across multiple Model Context Protocol (MCP) servers and for applying rules based on intrinsic tool properties. The change also streamlines policy management by migrating previously hardcoded logic into declarative TOML configuration. Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
gemini-cli
Bot
added
the
status/need-issue
![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 introduces valuable enhancements to the policy engine by adding support for global MCP wildcards and tool annotations, refactoring hardcoded logic into configurable policies. However, critical security vulnerabilities have been identified in the implementation of tool annotation matching and wildcard policy enforcement. These issues include incomplete and inconsistent propagation of tool annotations (where only readOnlyHint is handled and MessageBus is not updated), and a flaw in server-specific wildcard matching that could lead to unintended tool executions. These need to be addressed to ensure the reliability and consistency of the access control system.
| if (prefix === '*') { | ||
| if (serverName === undefined) { | ||
| return false; | ||
| } | ||
| } else if (serverName !== undefined && serverName !== prefix) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The current logic for handling server-specific wildcards (e.g., my-server__*) is not strict enough. If a non-MCP tool is named with a __ separator (e.g., my-server__foo), it can incorrectly match a policy intended for an MCP tool from my-server. This happens because when serverName is undefined, the else if condition is not met, and the check falls through.
This could lead to unintended tool executions if a permissive policy is set for a specific MCP server. The check should be simplified to ensure that for any non-global wildcard, the serverName must be defined and must match the rule's prefix.
| if (prefix === '*') { | |
| if (serverName === undefined) { | |
| return false; | |
| } | |
| } else if (serverName !== undefined && serverName !== prefix) { | |
| return false; | |
| } | |
| if (prefix === '*') { | |
| // For a global MCP wildcard, this rule should only match if it's an MCP call. | |
| if (serverName === undefined) { | |
| return false; | |
| } | |
| } else if (serverName !== prefix) { | |
| // For a specific server wildcard (e.g., "my-server__*"), the serverName | |
| // must be defined and must match the prefix. This prevents a non-MCP | |
| // tool from matching a rule intended for a specific MCP server. | |
| return false; | |
| } |
| const toolAnnotations: Record<string, unknown> = {}; | ||
| if (toolCall.tool.isReadOnly) { | ||
| toolAnnotations['readOnlyHint'] = true; | ||
| } | ||
|
|
||
| const { decision, rule } = await this.config | ||
| .getPolicyEngine() | ||
| .check(toolCallForPolicy, serverName); | ||
| .check(toolCallForPolicy, serverName, toolAnnotations); |
There was a problem hiding this comment.
Incomplete Tool Annotation Propagation
The implementation of tool annotation matching is incomplete. Here, the toolAnnotations object is manually populated with only the readOnlyHint annotation. Any other annotations provided by MCP servers or defined in policy rules are ignored. This means rules intended to restrict tools based on other semantic annotations (e.g., a rule to deny tools with a "dangerous" annotation) will never match, as those annotations are never passed to the policy engine.
Remediation:
- Update the
AnyDeclarativeToolinterface andDiscoveredMCPToolto store and expose all available tool annotations. - Update this scheduler to collect and propagate all annotations from the tool instance to the
PolicyEngine.
| async check( | ||
| toolCall: FunctionCall, | ||
| serverName: string | undefined, | ||
| toolAnnotations?: Record<string, unknown>, | ||
| ): Promise<CheckResult> { |
There was a problem hiding this comment.
Inconsistent Policy Enforcement via MessageBus
The check method now requires toolAnnotations for correct policy matching, but existing callers like the MessageBus (in packages/core/src/confirmation-bus/message-bus.ts) have not been updated to provide this context. Consequently, any policy rules relying on annotations will fail to match during checks initiated via the MessageBus (e.g., when tools check their own permissions). This leads to inconsistent security decisions where a tool might be allowed by the scheduler but denied when the tool itself checks its permissions.
Remediation:
- Update the
MessageBusprotocol and theToolConfirmationRequestmessage to includetoolAnnotations. - Ensure all callers of
PolicyEngine.checkprovide the full set of tool annotations.
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
3 participants
Description
This PR enhances the Policy Engine to support:
mcpName = "*"in rules to target all tools from any MCP server.readOnlyHint.mcp-client.tshas been migrated toplan.tomlusing the new annotation matching mechanism.Changes
PolicyRuleandSafetyCheckerRuletypes.PolicyEnginematching logic for wildcards and annotations.CoreToolSchedulerto propagate tool annotations.mcpName = "*"only targets MCP tools.plan.toml.Verification
policy-engine-annotations.test.ts.