Repository navigation
fix(hook): defer on a denied command inside a prefixed shell wrapper - #4530
Conversation
|
Adrien EPPLING seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📊 Automated PR Analysis
SummaryFixes a permission-check regression where a deny rule inside a shell wrapper (e.g. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
pszymkowiak
left a comment
pszymkowiak
left a comment
There was a problem hiding this comment.
LGTM. Verified locally: fmt/clippy clean, cargo test --all green, and rtk rewrite matches the table (prefixed/sudo/xargs/nested wrappers with an inner deny all defer; plain git push still exits 2).
Non-blocking nits:
PermissionVerdict::Deferhas no doc comment, andcheck_command's precedence doc doesn't mention it.- A one-line test that
decide_with_params(…, Defer)yieldsHookDecision::Deferwould pin the new gate. - Side effect worth noting in the description: since wrappers are detected at any arg position,
find . -exec sh -c '…' \;withallow find:*now gets Ask instead of Allow (stricter, fine by me).
CLA still pending on the commit email.
aeppling
changed the title
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Review — request changes
The fix covers the env bash -c 'git push' regression it targets. However, deleting the is_shell_wrapper_candidate fallback opens a new auto-allow, and the new rewrite-detection check is quadratic.
Every finding below was reproduced against debug builds of this head (e4a200f) and develop (311ebcb).
- Settings:
allow Bash(*),deny Bash(git push:*) rtk rewriteexit codes: 0 allow · 1 no rewrite · 2 deny · 3 ask
Blocking
1. A line continuation hides the wrapper from the gate, so a rewritten script is auto-allowed (regression)
| Command | this PR | develop |
|---|---|---|
bash -c \⏎'git push' |
0, bash -c 'rtk git push' |
3 |
bash -c 'git status'\⏎ && ls |
0 | 3 |
The rewriter runs collapse_line_continuations (registry.rs:796) before it parses wrappers. The gate segments the raw cmd (permissions.rs:118). parse_shell_wrapper therefore fails on the gate's text, and the segment is skipped. Before this PR, is_shell_wrapper_candidate turned this case into Ask.
Suggested fix: collapse continuations on cmd at the top of check_command_with_rewrite_params, before segmenting.
2. The rewritten-script check is quadratic
permissions.rs:229-231 runs rewritten.matches(quoted).count() and cmd.matches(quoted).count() for each wrapper. Timings for bash -c 'git status' repeated, joined by ; :
| wrappers | develop | this PR |
|---|---|---|
| 1 000 | 0.16 s | 0.46 s |
| 2 000 | 0.19 s | 1.29 s |
| 4 000 | 0.40 s | 4.33 s |
The hook stdin payload has no argv limit, so a MiB-scale command is much worse. The "linear" timing in the description was measured on inputs that don't exercise this path (100k plain args, a single large script).
Suggested fix: per wrapper position, compare against the rewriter's own rewrite_shell_wrapper result instead of counting substrings. That is O(n), and it also removes the reliance on text matching, which breaks silently if the rewriter ever re-quotes or re-escapes.
Should fix
3. A redirect glued to an env assignment hides the wrapper
FOO=a>/dev/null bash -c 'git push'is rewritten toFOO=a>/dev/null bash -c 'rtk git push'with exit 0.FOO=a&>/dev/null timeout 5 bash -c 'git push'behaves the same.
develop also returns 0 here, so this is not a regression. It is, however, the class of bug this PR sets out to close. ENV_PREFIX (registry.rs:69, [^\s]*) swallows >/dev/null into the value, so the rewrite strips the prefix and reaches the wrapper. split_for_permissions instead cuts the segment at the redirect token, so the gate never sees the wrapper.
4. The drift test can't catch 1 or 3
test_shell_wrapper_positions_match_where_the_rewrite_reaches calls shell_wrapper_positions on pre-split strings. It never goes through split_for_permissions, never normalizes the command, and never checks the verdict.
Suggested: assert "rewrite reaches a wrapper ⇒ check_command_with_rewrite_params ≠ Allow" for these rows:
bash -c \⏎'git status'bash \⏎-c 'git status'timeout 5 \⏎bash -c …true &&\⏎bash -c …FOO=a>/dev/null bash -c …FOO=a&>/dev/null timeout 5 bash -c …- 10 vs 11 stacked
execprefixes - a case with non-empty exclude patterns
5. Exit-code docs are stale
An inner-script deny now exits 1, not 2. These places still describe exit 2 as "a deny rule matched (checked before anything else)" or omit Defer:
src/main.rs:987(rtk rewrite --help)- the
runtable atsrc/hooks/rewrite_cmd.rs:70 - the gate-order doc at
decision.rs:122-139
Non-blocking
- Prefixed denied command inside a script →
Ask, notDefer.env bash -c 'git status && timeout 5 git push'returns 3 (a prompt, not an allow). The inner deny match is literal, the same as the outer loop. - Prefixed denied command outside a wrapper is auto-allowed.
timeout 5 git push,env git pushandFOO=1 git pushreturn 0 on this PR and on develop. Out of scope here, but it is the same family. test_defer_verdict_maps_to_passthrough_exit_codedoesn't exerciserun. It only checks the test-localexpected_exit_code, so deleting theDefer => exit(1)arm would not fail it. A subprocess test viacommon::rtk_command()asserting exit 1 and empty stdout would cover it.- No end-to-end coverage of
check_command_for_agentwith configuredtransparent_prefixes, and no case where an ask rule matches an inner command. - Spurious
Defer, safe direction.ls && ( bash -c '…' )and a middle pipeline stage (echo a | bash -c '…' | cat) defer, which drops thelsrewrite.
Checked and OK
- The prefix walk matches
rewrite_segment_innerbranch by branch: depth bound,RTK_DISABLED=, routableuv run, usertransparent_prefixes. Deferis handled by every consumer. All host hook formats pass the command through unrewritten, and the discover coverage estimate excludes it.quoted_scriptoffsets always land on char boundaries.- Deny →
Deferhas no effect on the in-tree delegates: hermes treats exits 1 and 2 identically, and pi rewrites only on 0 and 3. - Gate checks pass: fmt, clippy (0 warnings),
cargo doc -Dwarnings, and 4,075 lib/bin tests.guard_integration_test::git_log_malformed_digit_run…andcopilot_selfheal_test::unwritable_hooks_dir…fail in the same way on develop; the causes are environment-specific (git error wording, running as uid 0).
Generated by Claude Code
aeppling
changed the title
3de213d to
ae46f1d
Compare
|
Squashed to one commit ( @KuSh — against your review of
Fuzz on this content: 548,505 runs vs master and develop, oracle "equal to develop except @pszymkowiak — the approach changed since your approval of |
pszymkowiak
left a comment
pszymkowiak
left a comment
There was a problem hiding this comment.
Re-looked at ae46f1db as asked.
Locally: fmt and clippy clean, cargo test --all 4391 green, and KuSh's five points all check out on my side (continuation, quadratic, glued redirect, drift test, exit-code docs). Every prefixed-wrapper row from the table reproduces, including env -i, env -u X, timeout --signal=KILL 5, timeout -k 2 5, nohup … &, ( … ), trailing >/dev/null, 2>&1 | cat, $'…', and a nested bash -c 'bash -c "git push"'. The fix for the #4126 regression is right and the shell_wrapper_positions drift test is a good guard.
Two points inline before I re-approve, both reproduced on this head with allow Bash(*) + deny Bash(git push:*) in project settings and an empty HOME:
- A prefix inside the script still gets a rewritten, prefixed wrapper auto-allowed (
env bash -c 'exec git push'→ exit 0). Not a regression, same family as the one this PR closes. - Deny → Defer removes develop's hard block on hosts where RTK renders the decision itself.
CLA is still unsigned on the commit email.
| continue; | ||
| }; | ||
| if depth == 0 { | ||
| contains_shell_wrapper = true; |
There was a problem hiding this comment.
A prefix inside the script defeats the inner deny check, and because the wrapper is at depth 1 nothing caps the verdict at Ask
Not a regression (develop: 0, master: 1), but it is the same family as env bash -c 'git push'. Repro on ae46f1db, allow Bash(*) + deny Bash(git push:*) in .claude/settings.json, empty HOME:
$ rtk rewrite "git push" # exit 2 — deny rule is loaded
$ rtk rewrite "env bash -c 'git push'" # exit 1 — this PR ✔
$ rtk rewrite "env bash -c 'exec git push'"
env bash -c 'exec rtk git push' # exit 0
$ echo '{"tool_name":"Bash","tool_input":{"command":"env bash -c '"'"'exec git push'"'"'"}}' | rtk hook claude
{"hookSpecificOutput":{…,"updatedInput":{"command":"env bash -c 'exec rtk git push'"},"permissionDecision":"allow"}}Same with command git push, env git push, timeout 5 git push and git -C . push inside the script.
Why: the inner match is literal, so exec git push does not match git push:*. That part is #4284's scope, agreed. But the wrapper sits at depth 1, so this depth == 0 guard skips develop's "a wrapper is never auto-allowed, at most Ask" rule, the outer env matches allow *, and the rewritten script is auto-allowed. Unprefixed bash -c 'exec git push' gets 3 (Ask) from the segment-start rule — the prefix is what removes the guard.
Suggestion: set contains_shell_wrapper = true whenever parse_shell_wrapper succeeds, at any depth (keep the depth == 0 guard on the is_shell_wrapper_candidate fallback above so grep -rn bash -c src stays untouched). Cost: env bash -c 'git status' becomes Ask instead of Allow — the verdict develop already gives bash -c 'git status', so it is consistent, and env zsh -ec 'git status' && ls would prompt the way bash -c 'git status' && ls already does (the v2 trade-off KuSh pushed back on; I think the segment-start rule has already accepted that cost). The two *_keeps_the_outer_verdict tests would flip to Ask. Benefit: RTK never answers "allow" on a wrapper script it did not fully understand.
If you'd rather keep that for the #3828 follow-up, please at least list env bash -c 'exec git push' → exit 0 explicitly under "Not changed", so the release notes are honest about it.
| }) | ||
| }); | ||
| if inner_denied { | ||
| return Some(PermissionVerdict::Defer); |
There was a problem hiding this comment.
Deny → Defer downgrades develop's hard block on hosts where RTK renders the decision
bash -c 'git push' with the same rules: develop exits 2 (Deny), this PR exits 1 (Defer). The description frames that as master's behaviour, but master is the weaker baseline; develop had moved forward and the 68 Deny→Defer rows in your fuzz are exactly this.
Impact per host, from hook_cmd.rs on this head:
- Claude Code: none.
process_claude_payloadturns Deny intoPayloadAction::Skip,run_claude_innerprints nothing for anything but a rewrite, so Deny and Defer both render as no output and Claude applies its own rules (I checked:rtk hook claudeprints nothing forgit pushand forbash -c 'git push'). - Gemini: Deny renders
{"decision":"deny",…}, Defer rendersask_user. Antigravity: Deny renders"decision":"deny", Defer renders{"decision":"allow"}. Those hosts do not parse-cscripts, so their owndeny git push:*does not matchbash -c 'git push'— with this PR the block is simply gone for them. Trae collapses Deny and Defer intoNone, so it is unaffected; I did not check every remaining host.
The inner match is literal (git push, rm), so false positives are rare; a literal deny match inside a script RTK fully parsed looks like exactly the case where RTK should keep saying Deny. Could you either keep Deny here — at least at depth 0, which is develop's tested behaviour — or say why a confirmed inner deny should hand the decision back to a host that cannot see it? If the concern is #4126's Deny on a wrapper the rewrite would not touch anyway, develop already does that today and nobody has reported it.
There was a problem hiding this comment.
Correction on my side: the Gemini and Antigravity part of this comment was wrong. I read how each hook renders a Deny and did not check which rules each host loads. load_rules_for gives Gemini its own allow/confirm rules (load_gemini_rules) and Antigravity, Codex, Trae and Vibe no RTK-side rules at all; none of them reads .claude/settings.json. So a Claude deny rule never produced a Deny for those hosts, on develop or anywhere else, and there was no block to lose there.
The only host where develop actually blocked bash -c 'git push' is Copilot IDE, which decides with Host::Claude rules and renders Deny as permissionDecision: deny. That block came in with #4126, which is not on master (v0.51.0), so it was never released. With that, Defer being "never worse than master on any host" holds, and I withdraw the request to keep Deny here for the release. Whether a literal inner deny match should become a hard Deny at depth 0 is a product call for the maintainers; your one-line offer stands if they want it.
#4126 made the rewrite descend through prefixes (env, assignments, timeout, nice, nohup, time, exec, command, uv run, configured transparent_prefixes) to a quoted shell wrapper, but the permission gate still looked for the wrapper at the segment start only. A denied inner command rode the outer allow: `env bash -c 'git push'` under `allow env:*` + `deny git push:*` was rewritten and auto-allowed. The gate now checks deny rules against the wrapper's script at every position the rewrite reaches (`registry::shell_wrapper_positions`, the rewrite's own prefix walk), on the text the rewrite reads: continuations joined, segments not cut at a redirect (`lexer::split_for_wrappers`). Inside the script the same walk is applied before matching, so a prefix there (`exec git push`, `timeout 5 git push`, `FOO=1 git push`) reaches the rule too. A match returns the new `PermissionVerdict::Defer`: no rewrite, no decision, the host sees the command as typed — master's behaviour, instead of #4126's Deny. Nothing else changes: a prefixed wrapper otherwise keeps the outer command's verdict, and a wrapper at the segment start keeps develop's rule. Fixes the release blocker reported on #4399. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ae46f1d to
4d37aa8
Compare
|
@pszymkowiak thanks — both points reproduced on release binaries of master, develop and this head; 1. Prefix inside the script — closed. The inner deny match now applies the same prefix walk the gate already uses (
I kept the narrow form rather than "any parsed wrapper → Ask at every depth": that would prompt on every parseable prefixed script, rewritten or not ( 2. Deny → Defer. Per host,
Gemini and Antigravity never receive a Gate on CLA: the commit email is not attached to the author's GitHub account; being handled on our side. |
pszymkowiak
left a comment
pszymkowiak
left a comment
There was a problem hiding this comment.
Re-verified on 4d37aa8b (still one commit, on the tip of develop).
Gate: fmt and clippy clean, cargo test --all 4392 green, CI green on the three OS.
Point 1 — closed. Every inner-prefix row defers on my side, rtk hook claude prints nothing for them:
env bash -c 'exec git push' exit 1 (was 0)
env bash -c 'command git push' exit 1
env bash -c 'env git push' exit 1
env bash -c 'timeout 5 git push' exit 1
env bash -c 'FOO=1 git push' exit 1
env bash -c 'nice -n 5 git push' exit 1
env bash -c 'time git push' exit 1
env bash -c 'nohup git push' exit 1
env bash -c 'cd x && exec git push' exit 1
env bash -c 'exec exec git push' exit 1
env bash -c 'bash -c "exec git push"' exit 1
env bash -c 'direnv exec . git push' exit 1 (configured transparent_prefixes, inside and outside the script)
env bash -c 'git -C . push' exit 0 → listed under "Not changed", fine
env bash -c 'exec git status' exit 0 → rewritten, as expected
My 18 non-regression rows from the previous pass (redirects, pipes, env -i, timeout --signal, $'…', grep -rn bash -c src, git commit -m "bash -c …", …) give the same verdicts as on ae46f1db.
Point 2 — you were right and I was wrong about Gemini and Antigravity; see my reply in the thread. Defer is indeed never worse than master on any host. Deny at depth 0 is a product call for the maintainers, not a blocker for the release.
Timing, debug builds, same machine, 3 runs each, develop vs this head: 4000 bash -c 'git status' wrappers 0.43–0.48 s vs 0.45–0.63 s; 4000 env bash -c 'git status' wrappers 0.41–0.49 s vs 0.52–0.60 s (≈ +15 %, linear, on an unrealistic payload); one wrapper with 5000 inner segments plus a git push 0.40 s exit 0 on develop vs 0.08 s exit 1 here — develop rewrote and auto-allowed the whole script, this head stops at the deny.
Merge is still blocked on the CLA (license/cla: pending).
|
Fuzz figures for |
4 participants
Summary
env bash -c '…'), but the permission gate still looked for the wrapper at the segment start only. A denied inner command rode the outer allow:env bash -c 'git push'withallow env:*+deny git push:*was rewritten and auto-allowed (master: no rewrite, host decides). Same fortimeout 5,nice,nohup,time,exec,command,uv runand a configuredtransparent_prefixesentry.registry::shell_wrapper_positions, the rewrite's own prefix walk). A deny match returnsDefer: no rewrite, no decision, the host sees the command exactly as typed — master's behaviour, instead of feat(rewrite): rewrite inner commands of a quoted fish -c wrapper #4126'sDeny.Ask) is kept as is.registry::collapse_line_continuations), sobash -c 'git \⏎push'andbash \⏎-c 'git push'reach the deny rule likebash -c 'git push'does. The second form was auto-allowed on develop.lexer::split_for_wrappers). The rule gate's own split cuts a segment at its first redirect, soFOO=a>/dev/null bash -c 'git push'showed it onlyFOO=awhile the rewrite, whose env peel swallowsFOO=a>/dev/null, rewrote the script. Auto-allowed on develop, closed here. (Rule matching itself is unchanged; the plainFOO=a>/dev/null git pushis the same on master and develop.)env bash -c 'exec git push','command git push','env git push','timeout 5 git push','FOO=1 git push'defer. Auto-allowed on develop (the inner match was literal), closed here.allow Bash(*),deny git push:*env bash -c 'git push'timeout 5 bash -c 'git push' && lsdirenv exec . bash -c 'git push'(prefix configured)bash \⏎-c 'git push'bash -c 'git \⏎push'FOO=a>/dev/null bash -c 'git push'FOO=a&>/dev/null timeout 5 bash -c 'git push'env bash -c 'exec git push'env bash -c 'timeout 5 git push'bash -c 'exec git push'env zsh -c 'git status' && lsenv zsh -ec 'git status' && lsbash -c 'git status' && lsbash -c 'git push'grep -rn bash -c src(exit: 0 allow, 1 no rewrite, 2 deny, 3 ask)
Review history
grep -rn bash -c src), dropped rewrites, quadratic. Replaced.Ask: prompted on wrappers whose script RTK did not change (env zsh -ec … && ls). Replaced.split_for_wrappers; 4 (drift test) is now a verdict-level test over his rows (test_wrapper_reached_by_the_rewrite_is_gated_for_deny_rules); 5 (stale exit-code docs inmain.rs,rewrite_cmd.rs,decision.rs) updated.Related
07e17bab) with aWrapperScanrecorded by the rewrite walk; feat(rewrite): rewrite inner commands of a quoted fish -c wrapper #4126 carried an earlier version without it. Recording what the rewrite did is still the right design to also refuse auto-allowing a rewritten prefixed script (develop auto-allows it today, as does this PR, equal to master's outcome); that is the v1 follow-up, not needed to unblock the release.matching_readingsmatches deny/ask rules past prefixes on plain commands; it does not gate wrappers). Whichever lands second rebasescheck_command_with_rules(body incheck_command_with_rules_and_prefixes); fix(permissions): match deny and ask rules past assignments and wrappers (#4281) #4284's inner-script tests must expectDefer.Not changed (same as master, for v1 — tracked in #4545)
find . -exec bash -c '…' \;,ssh host bash -c '…',xargs sh -c '…'.env bash -c 'git -C . push'→ exit 0, as on develop. The host does not read pastgit -Ceither; fix(permissions): match deny and ask rules past assignments and wrappers (#4281) #4284 draws the same line.Test plan
cargo fmt --all && cargo clippy --all-targets && cargo test --all && RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps— 4390 tests pass, 0 warningsuv runand configureddirenv exec .; nine non-wrapper commands; four untouched-wrapper rows; five continuation rows; glued-redirect rows incl.ls && FOO=a>/dev/null bash -c '…'; trailing and leading redirects on a wrapper;cat >file bash -c '…'stays untouched;find -exec; suffix-- sh -c; deny word in argument text; 10 vs 11 stackedexec)ls0.27 s / 0.26 s; 100k args 0.05 s / 0.05 s; 1 MiB wrapper hook payload 0.18 s / 0.14 srtk rewrite+ 8 hook formats). Generator: prefixed wrappers,-lc/-ec/-x -c/-c --forms, prefixes inside the script (exec,command,env,timeout, assignments,git -C), shell names as arguments, quoted shell paths, suffix-- sh -c, stacked prefixes, configuredtransparent_prefixes, pipelines, groups, random\⏎continuations, redirects glued to assignments, every probe from the five reviews. Oracle: the PR must equal develop on every format except a Defer on a denied inner script or develop's Deny; any new block or auto-allow vs master and develop, any rewrite-text difference, any crash or invalid hook JSON fails. 0 violations, 0 flagged rows. Every change is a Defer on a denied inner script (1,645), develop's Deny→Defer (70), or a joined continuation (25). Timing suite: identical to develop within noise (40,000 untouched wrappers 0.42 s vs 0.41 s; 1 MiB wrapper hook 0.25 s vs 0.17 s). Earlier heads: 548,505 and 547,155 runs, 0 regressions each.🤖 Generated with Claude Code