feat(core): implement native support for blocking hook execution via policy files#20430
feat(core): implement native support for blocking hook execution via policy files#20430h30s wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @h30s, 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 security and control mechanisms within the system by introducing explicit policy enforcement for hooks. Instead of relying on an indirect message bus route for policy checks, a dedicated 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
priority/p3
![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 native support for blocking hook execution via policy files, enhancing security and control. While the implementation in HookEventHandler and PolicyEngine is generally robust, correctly handling denied hooks, updating UI state, and following a 'fail-secure' principle, a critical logic flaw was identified. Specifically, the checkHook method in PolicyEngine incorrectly matches rules with a commandPattern to hooks lacking a command property (e.g., runtime hooks), which could lead to unintended authorization decisions. Furthermore, a high-severity issue exists in toml-loader.ts where the argsPattern for hook rules is inconsistent with tool rules, potentially causing user confusion.
| if (rule.commandPattern && hookConfig.command) { | ||
| if (!rule.commandPattern.test(hookConfig.command)) { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
The checkHook method contains a logic flaw in how it matches rules with a commandPattern. If a rule specifies a commandPattern, but the hook being checked does not have a command property (which is the case for RuntimeHook types), the matching logic skips the pattern check and incorrectly considers the rule a match. This happens because the condition if (rule.commandPattern && hookConfig.command) evaluates to false when hookConfig.command is missing, bypassing the continue statement that would otherwise signal a non-match.
This flaw can lead to unintended authorization decisions. For example, a rule intended to allow only specific shell commands (e.g., [[hook_rule]] commandPrefix = "git" decision = "allow") would unintentionally allow ALL runtime hooks for the same event, as they lack a command property. Conversely, a restrictive rule could block more than intended.
To fix this, ensure that if a rule specifies a commandPattern, it only matches hooks that have a command property and where that property matches the pattern.
| if (rule.commandPattern && hookConfig.command) { | |
| if (!rule.commandPattern.test(hookConfig.command)) { | |
| continue; | |
| } | |
| } | |
| if (rule.commandPattern) { | |
| if (!hookConfig.command || !rule.commandPattern.test(hookConfig.command)) { | |
| continue; | |
| } | |
| } |
| commandPrefix: z.union([z.string(), z.array(z.string())]).optional(), | ||
| commandRegex: z.string().optional(), | ||
| argsPattern: z.string().optional(), |
There was a problem hiding this comment.
The use of commandRegex and argsPattern here is confusing and inconsistent with how argsPattern is used for tool rules ([[rule]]). For tool rules, argsPattern matches against the JSON-stringified arguments. For hook rules, both commandRegex and argsPattern are being used to match against the hook's command string, as the checkHook method in PolicyEngine does not have access to hook arguments.
This creates an inconsistent and misleading API for users writing policy files. They might expect argsPattern to match against the hook's input payload, which is not the case.
To improve clarity and consistency, I recommend consolidating these fields. A good approach would be to:
- Deprecate
commandRegexandargsPatternfor[[hook_rule]]. - Introduce a single, clearly named field like
commandPatternfor regex matching against the hook's command string.
This would make the policy file schema for hooks less ambiguous.
1 participant
Summary
Implemented native support for blocking the execution of hooks using policy files (
.toml). Previously, hooks were unintentionally subjected to policy checks by routing their execution through the message bus asrun_shell_commandtools. This PR introduces explicit[[hook_rule]]configurations in policy files to enforce whether a hook is allowed or denied without relying on the message bus.Details
HookRuletoPolicyEngineConfig.HookRuleSchemaintoml-loader.tsto parse[[hook_rule]]items from.tomlfiles.checkHooktoPolicyEngineprioritizing matching rules foreventName,hookName, andcommandPattern.HookEventHandler'sexecuteHooksto filter out denied hooks and appropriately log decisions without invokinghookRunner.ASK_USERdecisions uniformly downgraded toDENYdue to headless hook invocation.Related Issues
Fixes: #17406
How to Validate
[[hook_rule]]in~/.gemini/policies/project.tomlwithdecision = "deny"foreventName = "BeforeTool".npm run test -- -w @google/gemini-cli-core.Pre-Merge Checklist