fix(core): trust dialog discloses the hook shape that never runs (#27901)#27915
fix(core): trust dialog discloses the hook shape that never runs (#27901)#27915magudeshhmw wants to merge 2 commits into
Conversation
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 addresses a critical discrepancy between the workspace-trust dialog's disclosure logic and the actual hook execution engine. Previously, the dialog was reading flat hook definitions that the engine ignored, while failing to report the nested definitions that were actually being executed. By updating the discovery service to mirror the engine's parsing logic and hardening the trust warning display to include raw commands, this change ensures users have accurate visibility into the actions they are authorizing. 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
|
|
📊 PR Size: size/M
|
|
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. |
There was a problem hiding this comment.
Code Review
This pull request improves security by ensuring that the actual commands executed by project hooks are always disclosed to the user, preventing malicious hooks from hiding behind benign names. It updates TrustedHooksManager to display both the hook name and command, and modifies FolderTrustDiscoveryService to parse the canonical nested hook structure that the execution engine actually runs. The review feedback correctly suggests treating workspace-level configurations as untrusted by default and preventing security-sensitive settings like hooks from being loaded unless trust is explicitly granted.
e5fd9d6 to
b136441
Compare
The dialog displayed one set of hook commands but the agent executed a different set. User approves A, B runs instead. Fixed so both display and execution read from the same source. Closes google-gemini#27901
534d199 to
17526fa
Compare
|
Thanks for validating my findings! |
What breaks
The workspace-trust dialog shows the inverse of the hooks that actually run. A project can ship a
SessionStarthook in the canonical nested shape; it executes arbitrary shell on a single Trust folder click while the dialog never displays the command. Closes #27901.Root cause
The disclosure parser and the execution engine read two mutually exclusive hook shapes:
FolderTrustDiscoveryService.discoverSettingsread acommandoff the outerHookDefinition(the flat shorthand{ type, command }).hookRegistry.processHookDefinitiononly runs commands nested insidedefinition.hooks[], and returns early for any definition without that array (the!Array.isArray(definition.hooks)guard).trustedHooks.getUntrustedHooksskips the flat shape too.{ type, command }{ hooks: [{ type, command }] }So disclosure is anti-correlated with execution: the shape that runs is the shape that is never shown.
Fix
FolderTrustDiscoveryServicenow descends intodefinition.hooks[]and reads the innerHookConfig.command, mirroring exactly what the engine executes. Flat definitions the engine discards are no longer falsely disclosed.Secondary hardening (same theme):
trustedHooks.getUntrustedHooks— which builds the project-hooks warning — now surfaces the command asname (command)instead of only the friendlyname, which could otherwise mask the command behind a benign label.Tests
SessionStarthook's command is now disclosed bydiscover().nameis present.trustedHooksexpectations updated to the security-correct format.All of
FolderTrustDiscoveryService,trustedHooks, andhookRegistrysuites pass (42 tests).tscandeslintclean.Scope / not included
This PR fixes the disclosure↔execution inversion (defect A in #27901). The issue also notes (B) the per-hook trust store auto-filling itself and (C) the warning being future-tense and emitted after the hook already ran — those are behavioral/UX changes best handled separately and are intentionally out of scope here.
Risk
Low. The change is confined to read-only discovery + warning text; it widens what the trust dialog reports to match what already executes. No execution-path behavior changes.
🤖 Generated with Claude Code