Repository navigation
fix(hooks): honor permission rules for already-rtk-prefixed commands - #3195
albatrossflyon-coder wants to merge 1 commit into
Conversation
Fixes rtk-ai#3152. Claude Code sometimes issues the already-rewritten form of a command directly (e.g. `rtk grep foo` instead of `grep foo`) once it has seen it once in an earlier turn. Permission rules like `Bash(grep *)` are written against the underlying tool, but neither RTK's own matcher nor the host's native one recognized `rtk grep foo` as a match for that pattern — RTK's hook fell back to Defer (nothing to rewrite), and the host's native check independently failed the same way, so both allow and deny rules silently stopped applying once a command arrived pre-rewritten. - permissions.rs: strip a leading `rtk ` from each compound-command segment before matching against deny/ask/allow patterns, so rules match regardless of which form the command arrives in. - hook_cmd.rs (decide_from_verdict): an Allow verdict with nothing left to rewrite is no longer silently discarded as Defer — it's asserted directly, since the corrected matching above means it's a real Allow. - hook_cmd.rs (process_claude_payload): a Deny verdict on an already-rtk segment is now asserted explicitly instead of silently deferring to the host's native check — deferring only works when the host evaluates text that still matches the user's original pattern, which isn't true once the segment carries the `rtk ` prefix. This closes a real bypass: a deny rule could previously be sidestepped just by the command arriving pre-rewritten. The original (non-rtk) command path is untouched — same defer-to-native behavior as before. Verified live against the reporter's exact settings.local.json + rtk-prefixed grep/ls repro, the already-rtk deny case, and a compound already-rtk `||` chain. Added regression tests in both files; full suite (2498), clippy, and fmt all clean.
…ady-rtk-prefixed commands)
…tream rtk-ai#3195) Fork-authored amendments closing review findings on the adopted commit: - permissions: match each pattern against BOTH the raw and the rtk-stripped segment. Stripping alone silently disabled rules written against the rtk form itself (a Bash(rtk:*) deny stopped firing) — a fail-unsafe regression. - permissions: strip repeatedly and across any whitespace, sharing one is_rtk_prefixed predicate with the hook gate, so 'rtk rtk rm' and 'rtk<TAB>rm' cannot evade a deny. - hook: gate the new assert-Allow-without-rewrite branch on the command actually being already-rtk; unrewritable plain commands keep deferring to the host's stricter native matcher. - hook: Copilot IDE renderer stays silent on an AllowRewrite carrying the command unchanged — its deny-with-suggestion contract would otherwise tell the agent to re-run the identical command, deny-looping forever. - tests: regression coverage for each of the above plus end-to-end PayloadAction::Deny / skip:deny_rule rendering via an injectable process_claude_payload_impl (mirrors run_gemini_inner_impl). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-2 review fixes on the rtk-ai#3195 adoption: - The assert-Allow-without-rewrite gate quantified over ANY rtk segment, so a compound mixing an rtk segment with raw text (rtk grep x && shutdown now) asserted allow for a raw segment the host's stricter native matcher should have evaluated (RTK treats a bare rule as a prefix match; the host requires ':*'). The allow gate now requires EVERY non-empty segment to be rtk-prefixed; the deny gate keeps the any-segment quantifier deliberately (over-asserting deny is fail-safe) and the asymmetry is documented against future unification. - normalize_for_matching strips only ASCII IFS whitespace after 'rtk': unicode whitespace glues into one word the shell cannot execute, so stripping across it normalized text the shell never runs. - tests: mixed-compound defer ('&&', newline, raw-first orderings), all-rtk compound allow, NBSP non-normalization, and gemini/droid renderer pins for the AllowRewrite(cmd-unchanged) shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-3 review fixes on the rtk-ai#3195 adoption: - rtk proxy <cmd> executes <cmd> raw, and rtk err/test/summary execute it filtered — so 'rtk proxy rm -rf /' evaded a Bash(rm:*) deny while a Bash(rtk:*) allowlist actively allow-asserted it. normalize_for_matching now strips one wrapper verb after each rtk token (exhaustive set: proxy, err, test, summary; 'rtk check' never executes its argument). - An Ask verdict on an already-rtk command was silently deferred, letting a host-native Bash(rtk:*) allowlist auto-run a command the user marked ask. The Claude path now asserts permissionDecision 'ask' (through the existing bypassPermissions-gated Ask rendering); renderers without an ask channel keep their pre-existing behavior. - tests: wrapper deny for all four verbs (plus repeated prefix), normalize contract for wrapper shapes, ask assertion, and fail-safe pins for env-prefixed, quoted, and command-substitution shapes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ions Round-4 review fixes on the rtk-ai#3195 adoption: - COMMAND_WRAPPERS missed 'run' — the one subcommand that shells out its argument via sh -c — so 'rtk run rm -rf /' evaded a Bash(rm:*) deny while Bash(rtk:*) allow-asserted it. 'run' is now stripped like proxy, and the exhaustiveness doc is corrected ('check' lives under rtk hook, not top-level). - Adding the token is not sufficient: wrapper arguments are arbitrary text ('rtk run -c "rm -rf /"', quoted args in any wrapper) that token-literal matching cannot decompose. New total rule: wrapped invocations are matched best-effort for deny/ask (over-firing is fail-safe) but are excluded from the hook's assert gates entirely (is_wrapped_invocation) — RTK never speaks for text it cannot attest. - The unattestable-construct branch pre-empted the already-rtk ask assertion: 'rtk git push > f' silently deferred into a host-native rtk:* allowlist. Already-rtk unattestables now assert ask (forced prompt); plain ones keep the silent defer. - test_claude_already_rtk_passthrough migrated to injected rules — since rtk-ai#3152 its outcome depends on permission rules, and reading the real on-disk settings made it environment-dependent. - tests: run-wrapper deny, never-assert sweep across wrapper shapes, unattestable ask assertion, is_wrapped_invocation contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round-5 (approve-level) minors on the rtk-ai#3195 adoption: - Ask assert now uses the deny-side any-segment quantifier and includes wrapped invocations: asserting ask can only produce a prompt, so over-firing is fail-safe, and the narrow gate left the PR's own bypass scenario (host-native rtk:* allowlist) open for mixed/wrapped compounds. - Vibe and Droid renderers stay silent when the decision carries the command unchanged — the unattestable already-rtk arm is reachable there with zero rules and would announce a rewrite that didn't happen. - Docs scoped: wrapped invocations are excluded from the allow assert specifically; quantifier note covers all three gates. - tests: mixed/wrapped ask assertion, vibe/droid unchanged-command silence, end-to-end Claude allow assert, CRLF normalize pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
KuSh
left a comment
There was a problem hiding this comment.
Round 1 — CHANGES REQUESTED
Claim: deny/ask/allow rules written for a tool apply when the agent sends it already prefixed (rtk grep …, rtk rm …).
Scope: accept if narrowed to "deny and ask rules apply to the command behind a model-authored rtk prefix, on every hook entry point; allow matching unchanged". That is #3970 (instance 1), so the PR should say Fixes #3970 instead of Fixes #3152: the #3152 reporter confirmed their agent sends plain grep, and their compound case (grep … || echo, grep … | head) is #1347's mechanism, which this PR does not touch. The allow half (auto-allowing rtk grep when Bash(grep *) is allowed) is out: rtk only asserts a decision on commands it rewrites. #4188 fixes the same instance; it is being closed in favour of this PR, and its wrapper unwrapping is carried in below with credit to @Ha1baraA11.
Ran: develop e542dca vs this head (and #4188) × 37 commands across 7 permission profiles × rtk hook claude and rtk rewrite; invariants: under the hook, every decision. /code-review high over the diff, 10 hypotheses, each checked by running it. Savings: n/a (no filter output).
Pins: 6 decisions, 6 mutated, 2 survived (the explicit deny output).
Matrix: executed this round; pin check: executed this round.
CI @6eb4e6339: only license/cla has run; the branch conflicts with develop (hook_cmd.rs, permissions.rs), so CI waits for the rebase.
Blocks merge (3)
-
src/hooks/hook_cmd.rs:153,src/hooks/permissions.rs:83— explicit allow where develop deferred. Strippingrtkon the allow side, plus the newNone if Allow => AllowRewrite(cmd)arm, makes the hook assertallowfor commands the host would prompt for or deny:- allow
Bash(./deploy.sh staging),./deploy.sh staging --prod(rtk does not rewrite it): develop no output →allow - allow
Bash(test:*),rtk test rm -rf /tmp/x: develop no output →allow - deny
Bash(rtk proxy:*)+ allowBash(*),rtk proxy rm -rf /tmp/x: hookallow, andrtk rewritegoes from exit 2 to 0. The strip replaces the segment instead of adding a reading, so a rule written inrtkform stops matching. Claude Code 2.1.302 still applies its own deny over the hook's allow (checked with a live session), but the hosts that readrtk rewrite's exit code get exit 0, auto-allow.
The fix: drop the allow half entirely. Deny/ask see the stripped form as an extra reading next to the segment as written. Develop already does exactly this for env assignments and wrappers inregistry::matching_readings, which runs for deny and ask only.
- allow
-
src/hooks/permissions.rs:386— thertkprefix still hides the command from deny/ask in most of its forms (one literalstrip_prefix("rtk ")). With allowBash(rtk:*), every one of these still runs with no prompt:- deny
Bash(rm:*):rtk bypass rm …,rtk proxy rm …,rtk run -c 'rm …',rtk rm …(two spaces),rtk -v rm … - ask
Bash(git push:*):rtk git push origin main. The hook asserts deny but not ask.
One site closes the whole class: a peel step for
rtk(its global flags,bypass/proxy, andrun/run -c) inmatching_readings. The outcome then belongs insrc/hooks/decision.rs, so thatrtk rewrite,rtk hook checkand every host judgertk rm …alike. @Ha1baraA11's unwrap ofproxyandrun -cin #4188 is the starting point for the peel.What
Bash(rtk:*)keeps granting: rtk's own commands (rtk gain,rtk discover, …) and its filters (rtk git status,rtk grep …). What it does not grant: anything an explicit deny or ask rule names for the inner command, which always applies; and a command rtk only passes through (rtk rm,rtk curl) or runs on request (rtk bypass/proxy/run), which needs an allow rule of its own, the same as without the prefix.
On an identity command the hook therefore assertsdenyfor an inner deny, andaskfor an inner ask, or for a pass-through or runner command that no allow rule covers. Each outcome needs a test through the payload path. - deny
-
src/hooks/hook_cmd.rs:388— unpinned behaviour (the deny output for an already-rtk command). Two mutations each leave all hook tests passing:if false && contains_already_rtk_segment(cmd), and deleting thePayloadAction::Denyarm ofrun_claude_inner. The tests check the verdict, never what the hook prints. A test must feedrtk rm -rf /tmp/xwith an injected denyBash(rm:*)(not the developer's settings) through the payload path and assertpermissionDecision: "deny". Both mutations must turn it red.
Checked and correct — no need to re-verify
- Bare
rtk rm -rf /tmp/xunder denyBash(rm:*)now emitsdeny, andrtk rewriteexits 2. It holds with or withoutBash(rtk:*)allowed, and in a compound (git status && rtk rm …). - Plain (unprefixed) commands are unchanged on every row.
Next
Rebase onto develop, drop the allow half, move the rtk peel into matching_readings (deny/ask only, as an added reading) and the outcome into decision.rs, add the deny/ask payload tests, and retarget the PR to Fixes #3970.
Threads: 3 open (waiting on the author).
| // deferring to the host's native check, which won't recognize the | ||
| // rewritten form either and would prompt for something the user | ||
| // already allowlisted. | ||
| None if verdict == PermissionVerdict::Allow => HookDecision::AllowRewrite(cmd.to_string()), |
There was a problem hiding this comment.
Blocking (1 in the review body). This arm asserts allow for every command rtk does not rewrite, not only rtk … ones. Combined with the allow-side strip, it now allows commands the host would prompt for. Under an exact rule Bash(./deploy.sh staging), ./deploy.sh staging --prod went from no hook output to allow, and so did rtk test rm -rf /tmp/x under Bash(test:*). A hook allow skips Claude Code's prompt (checked with a live 2.1.302 session), so both run unprompted. Please drop this arm together with the allow-side normalization.
| /// Normalizing here — right before matching — keeps the deny/ask/allow | ||
| /// checks meaningful regardless of which form the command arrives in. | ||
| fn normalize_for_matching(segment: &str) -> &str { | ||
| segment.strip_prefix("rtk ").unwrap_or(segment) |
There was a problem hiding this comment.
Blocking (2 in the review body). One literal strip_prefix("rtk ") misses most spellings. With allow Bash(rtk:*) and deny Bash(rm:*), all of these still run: rtk bypass rm …, rtk proxy rm …, rtk run -c 'rm …', rtk rm … (two spaces), rtk -v rm …. With ask Bash(git push:*), so does rtk git push origin main. It also replaces the segment rather than adding a reading, so a rule written as Bash(rtk proxy:*) stops matching. On develop, registry::matching_readings peels prefixes for deny and ask as extra readings. An rtk peel step there (global flags, bypass/proxy, run/run -c) closes the whole list.
|
|
||
| let (rewritten, allow) = match decide_hook_action(cmd, permissions::Host::Claude) { | ||
| HookDecision::Deny => { | ||
| if contains_already_rtk_segment(cmd) { |
There was a problem hiding this comment.
Blocking (3 in the review body). Nothing pins this output. With if false && contains_already_rtk_segment(cmd) here, or with the PayloadAction::Deny arm removed from run_claude_inner, every hook test still passes. A test should feed rtk rm -rf /tmp/x with an injected deny Bash(rm:*) through the payload path and assert permissionDecision: "deny". Both edits must turn it red.
Fixes #3152.
Root cause
Claude Code sometimes issues the already-rewritten form of a command directly (e.g.
rtk grep fooinstead ofgrep foo) once it has seen the rewrite once in an earlier turn. Permission rules likeBash(grep *)are written against the underlying tool, but neither RTK's own matcher nor the host's native one recognizedrtk grep fooas a match for that pattern — RTK's hook fell back toDefer(nothing left to rewrite), and the host's native check independently failed to match for the same reason, so both allow and deny rules silently stopped applying once a command arrived pre-rewritten.Traced with the reporter's exact repro (
.claude/settings.local.jsonwithBash(ls *)/Bash(grep *)inallow): feeding the original form throughrtk hook claudecorrectly emittedpermissionDecision: allow, but feeding the already-rewrittenrtk grep …form emitted nothing, leaving Claude Code to prompt.Fix
permissions.rs: strip a leadingrtkfrom each compound-command segment before matching against deny/ask/allow patterns, so rules match regardless of which form the command arrives in.hook_cmd.rs(decide_from_verdict): an Allow verdict with nothing left to rewrite is no longer silently discarded asDefer— it's asserted directly, since the corrected matching above means it's a real Allow.hook_cmd.rs(process_claude_payload): a Deny verdict on an already-rtk segment is now asserted explicitly (permissionDecision: "deny") instead of silently deferring to the host's native check — deferring only works when the host evaluates text that still matches the user's original pattern, which isn't true once the segment carries thertkprefix. This closes a real bypass: a deny rule could previously be sidestepped just by the command arriving pre-rewritten. The original (non-rtk) command path is untouched — same defer-to-native behavior as before, so this doesn't change behavior for the common case.Verification
settings.local.json+ bothrtk grepandrtk ls— now correctly emitpermissionDecision: allow.rtk rm -rf /tmp/dangeragainst aBash(rm:*)deny rule now correctly emitspermissionDecision: deny(previously emitted nothing, deferring to a native check that would've missed it).||question from the issue thread — both segments of an already-rtkrtk grep … || rtk echo …chain are checked independently and correctly allowed when both match.contains_already_rtk_segmenthelper).cargo clippy -- -D warningsandcargo fmt --checkboth clean.