fix(permission): prefer bash command prefix for approval grants#3815
Conversation
|
Other generic tools (search, web_fetch, grep, etc.) already returned the bare tool name in the old code and are unchanged. File mutation tools now use the same "Edit" grant for persistent rules that they already used for session grants — no new behavior, just alignment. |
esengine
left a comment
There was a problem hiding this comment.
Thanks — the root-cause analysis is exactly right, and preferring a safe bash prefix is the behaviour I want for #3759. The danger/shell-syntax guard in BashCommandPrefix keeps the widening safe, and the prefix-match tests are good. A few things to sort before I merge:
1. Finish removing the scope concept — right now it's only half-gone. The description says the unused scope parameter was removed and all callers updated, but it was only dropped from SessionGrantRuleForScope / RememberRuleForScope. approvalReply.scope, the scope parameter on Controller.ApproveWithScope, and the ApprovalScopeExact / ApprovalScopePrefix constants are still written but no longer read — requestApproval no longer consumes r.scope. Either fully remove the scope plumbing or keep it honoured; leaving it inert is dead code in a security-sensitive path.
2. The TUI still offers "exact" vs "prefix" keys that now produce the same rule. In chat_tui.go the session/persist handlers still pass ApprovalScopeExact vs ApprovalScopePrefix, but since the rule builder ignores scope, choosing the exact option silently stores the wider prefix grant. If we're committing to prefix-by-default (I'm fine with that), collapse those into a single "allow for this session" / "always allow" pair so the UI matches what's actually stored.
3. Confirm the persistent file-edit widening is intended. rememberRule("edit_file", "src/app.go") now returns Edit instead of Edit(src/app.go), so "Always allow (save to config)" on one edit persists a tool-wide Edit grant — every file edit in every future session auto-approves. That matches the in-session grant, but persisting a blanket edit-allow is a bigger step than the session one. I'm inclined to accept it, but please call it out in the description and add a test asserting the persisted rule is tool-wide, so it's a deliberate choice rather than a side effect.
Core direction is right. Get the scope cleanup done and the Edit widening documented + tested, and I'll run CI and merge.
|
Thanks for the detailed review and the careful security analysis. I’ll address all three points before the merge:
I'll push the cleanup, UI changes, and the additional test shortly. Once CI passes, feel free to merge. Thanks again for the thorough review. |
Address reviewer feedback on esengine#3815: 1. Remove the now-inert scope parameter from approvalReply, the defunct ApproveWithScope method, and the ApprovalScopeExact / ApprovalScopePrefix constants. All callers (desktop, serve, ACP dispatch, TUI) updated to use the single Approve method. 2. Collapse the TUI approval handler into a single set of options (allow once / session / persist) since prefix and exact scopes now produce identical rules. No more hasBashPrefix branching. 3. Add TestPersistedEditRuleIsToolWide asserting that a persisted "always allow" on one edit_file call creates a tool-wide Edit grant (no path restriction) covering all mutation tools on all files. This documents the deliberate choice. All existing tests pass. Net -59 lines across 11 files.
e234b64 to
b65fbd0
Compare
Address reviewer feedback on esengine#3815: 1. Remove the now-inert scope parameter from approvalReply, the defunct ApproveWithScope method, and the ApprovalScopeExact / ApprovalScopePrefix constants. All callers (desktop, serve, ACP dispatch, TUI) updated to use the single Approve method. 2. Collapse the TUI approval handler and banner into a single set of options (allow once / session / persist / deny) since prefix and exact scopes now produce identical rules. Preserved all deny key bindings (4/n/5/Esc). 3. Add TestPersistedEditRuleIsToolWide asserting that a persisted "always allow" on one edit_file call creates a tool-wide Edit grant (no path restriction) covering all mutation tools on all files. This documents the deliberate choice. All existing tests pass.
6c58c2e to
b65fbd0
Compare
Fixes esengine#3759 — the approval system remembered rules with full command arguments (e.g. "Bash(grep -r specificTerm .)"), causing every slightly different invocation to re-prompt within the same session. Root cause: SessionGrantRuleForScope and RememberRuleForScope only applied BashCommandPrefix when scope==ApprovalScopePrefix, but the default "allow this session" path used ApprovalScopeExact — so the prefix was never used. Every command was stored literally. Fix: - Both functions now always prefer BashCommandPrefix for bash when a safe prefix can be extracted, regardless of the scope parameter. Safe prefix means: >=2 words, no shell metacharacters, not dangerous. When no prefix is available (single word, shell syntax, dangerous command), the exact command is still stored as the fallback. - File mutation tools remain tool-wide ("Edit") — matching existing session-grant behaviour. - The now-unused scope parameter is removed from both function signatures; all callers updated. Concrete example: Before: "grep -r TODO ." approves -> stored "Bash(grep -r TODO .)" "grep -r FIXME ." -> prompts again (different subject) After: "grep -r TODO ." approves -> stored "Bash(grep -r:*)" "grep -r FIXME ." -> auto-allowed (matches prefix) "git log" -> still prompts (different command family)
…s backend + frontend Address reviewer feedback on esengine#3815: 1. Remove the inert scope parameter from approvalReply, Controller.ApproveWithScope, ApprovalScopeExact/ApprovalScopePrefix constants, and all callers. 2. Collapse TUI approval handler into single session/persist pair (no more hasBashPrefix branching). Restored numeric key "4" for deny. 3. Add TestPersistedEditRuleIsToolWide for the deliberate tool-wide Edit grant. 4. Clean up desktop Go (ApproveWithScope/ApproveTabWithScope), TypeScript bridge interface+mock, serve.go Scope field, and React ApprovalModal "prefix" arg. 5. Restore nil guard in bindApprove.
b65fbd0 to
a0b4b77
Compare
|
All three points addressed:
Full regression passes, three-way merge against upstream main-v2 is clean. @esengine |
esengine
left a comment
There was a problem hiding this comment.
All three points addressed exactly as promised — the scope plumbing is fully gone end to end (controller, ACP dispatch/service, desktop bridge + dev mock, serve HTTP API), the TUI and modal now present a single session/persist pair that matches what actually gets stored, and TestPersistedEditRuleIsToolWide locks in the tool-wide Edit grant as a deliberate choice with both positive and negative matches. Nice, thorough cleanup.
Merging once CI is green.
Reviewer feedback
Edit persistence scope (point 3): approving one
edit_filecall with "Always allow (save to config)" saves the rule asEdit— tool-wide, no path restriction. This covers all six file-mutation tools on any file across all future sessions. Deny rules still take precedence. This is intentional and matches the existing session-grant behaviour.Fixes #3759 — the approval system remembered rules with full command arguments (e.g. "Bash(grep -r specificTerm .)"), causing every slightly different invocation to re-prompt within the same session.
Root cause: SessionGrantRuleForScope and RememberRuleForScope only applied BashCommandPrefix when scope==ApprovalScopePrefix, but the default "allow this session" path used ApprovalScopeExact — so the prefix was never used. Every command was stored literally.
Fix:
Concrete example:
Before: "grep -r TODO ." approves -> stored "Bash(grep -r TODO .)" "grep -r FIXME ." -> prompts again (different subject) After: "grep -r TODO ." approves -> stored "Bash(grep -r:*)" "grep -r FIXME ." -> auto-allowed (matches prefix) "git log" -> still prompts (different command family)