Skip to content

hooks: permission gate should read the rewrite's own wrapper scan (follow-up to #4530) #4545

Open

Description

@aeppling

Summary

#4530 closed the #4126 regression (a denied command inside a shell wrapper the rewrite reaches now returns Defer) with the smallest change that is never worse than master. Three things were left on purpose. All rows below were verified on release binaries of master 0.51.0, develop 311ebcbf and #4530 with allow Bash(*) + deny Bash(git push:*) (rtk rewrite exit: 0 allow · 1 passthrough · 2 deny · 3 ask).

1. A rewritten prefixed script is auto-allowed on the outer rule

Command (allow Bash(env:*)) master develop #4530
env bash -c 'git status' → env bash -c 'rtk git status' 1 0 0
env bash -c 'git -C . push' → env bash -c 'rtk git -C . push' 1 0 0
env bash -c 'git reset --hard' with ask git reset:* 1 0 0

The host's outcome is unchanged (its own env:* rule allows the raw line), but RTK answers "allow" for a script it rewrote and only partly understood: the inner match reads past the prefixes the rewrite peels, not past a program's own options, and ask rules inside a script are not checked at all. At the segment start develop already refuses to auto-allow a wrapper (bash -c 'git status' → 3); behind a prefix it does not.

Proposed fix. Have the rewrite walk record what it did — the wrapper scripts it parsed, and whether it changed one — and let the gate read that instead of re-deriving positions. This is #3828's WrapperScan (07e17bab, @SindriFr), lost when #4126 superseded it. With it: Ask iff RTK actually rewrote inside a wrapper, deny and ask rules checked on exactly the scripts the rewrite saw, and the gate and the rewrite cannot drift by construction. Acceptance: #4530's test_shell_wrapper_positions_match_where_the_rewrite_reaches and test_wrapper_reached_by_the_rewrite_is_gated_for_deny_rules, plus a differential fuzz vs master and develop (prefixes inside scripts, \⏎ continuations, redirects glued to assignments, 40k-wrapper timing — the oracle used for #4530: equal to develop except Defer on a denied inner script, no new block or auto-allow vs master).

Plain prefixed denies (timeout 5 git push, env git push) are #4284's scope; it needs a rebase over #4530 (check_command_with_rules body moved to check_command_with_rules_and_prefixes, inner-script tests expect Defer).

2. Plain command with a redirect glued to an assignment

FOO=a>/dev/null git push → 0 on master, develop and #4530: split_for_permissions cuts the segment at the redirect (the gate sees FOO=a), while the rewriter's env peel swallows FOO=a>/dev/null and rewrites rtk git push. #4530 fixed this for wrappers only (lexer::split_for_wrappers); the rule gate's own split still cuts.

3. Continuation join changes semantics

collapse_line_continuations joins \⏎ with a space; bash removes it. sh -c "rm\⏎-rf x" is rewritten to sh -c "rm -rf x", a different command. Pre-existing on develop.

Decision recorded

#4530 returns Defer (no rewrite, no decision) rather than Deny on an inner-script match, so RTK never blocks where the raw command would run. Only Copilot JetBrains rendered #4126's Deny as a block; Gemini and Antigravity load no deny rules from RTK's side. Keep, or revisit together with the scan.

Refs: #4126, #3828, #4284, #4530, #4399.

Activity

  1. rtk-wshm-sync-bot commented on Oct 13, 2026

    @rtk-wshm-sync-bot

    wshm · Automated triage by AI

    🔍 Automated Triage

    🐛 Category bug
    🟠 Priority high
    🎯 Confidence 82%

    Summary

    Follow-up to #4530: permission gate still over-trusts rewritten wrapper scripts (auto-allow on outer rule, redirect/assignment split mismatch, continuation-join semantics change); proposes reinstating #3828's WrapperScan so the gate reads the rewrite's own scan.

    📁 Relevant files
    • src/hook/permission_gate.rs
    • src/rewrite/wrapper_scan.rs
    • src/lexer.rs

    Suggested Actions

    1. Decide whether to resurrect fix(rewrite): rewrite inner commands of quoted sh/bash/zsh -c wrappers #3828's WrapperScan (07e17ba) as proposed, vs. a narrower patch
    2. Coordinate with fix(permissions): match deny and ask rules past assignments and wrappers (#4281) #4284 — rebase plain-prefixed-deny work over fix(hook): defer on a denied command inside a prefixed shell wrapper #4530's check_command_with_rules_and_prefixes split
    3. Fix lexer::split_for_permissions to match split_for_wrappers' redirect/assignment handling (item 2)
    4. Re-examine collapse_line_continuations semantics change on \⏎ (item 3) — pre-existing on develop

    Triaged automatically by wshm · This is an automated analysis, not a human review.

  2. akushonkamen commented on Oct 13, 2026

    @akushonkamen

    Taking this one. Confirmed on develop @ ffa5651:

    • Gap 1: the permission gate never sees what rewrite parsed inside wrapper scripts — env bash -c "git push" returns 0 (allow) under deny Bash(git push:*) because the inner match only skips the prefix the rewrite peeled.
    • Gap 2: src/discover/lexer.rs:684 (split_for_permissions) still truncates segments at redirects, so FOO=a>/dev/null git push gates on FOO=a — fix(hook): defer on a denied command inside a prefixed shell wrapper #4530 fixed this only for the wrapper path (split_for_wrappers, src/discover/lexer.rs:688).
    • Gap 3: src/discover/registry.rs:736 (collapse_line_continuations) joins \⏎ with a space where bash deletes it, changing the rewritten command.

    Plan: fix gap 2 by aligning the gate's split with split_for_wrappers' redirect handling, fix gap 3 to bash's deletion semantics, and wire the rewrite's wrapper scan into the gate decision for gap 1. Will keep #4530's Defer-not-Deny semantics unless review says otherwise.

    Submitted with AI assistance (Claude, Anthropic) as part of an open-source contribution exercise.

  3. akushonkamen commented on Oct 13, 2026

    @akushonkamen

    PR: #4554 — gate reads the rewrite's own wrapper scan (Ask when it changed a script behind a prefix), assignment-glued redirects no longer truncate the segment, continuation joins match bash's deletion. One note recorded there: the issue's row-3 baseline on ffa5651 is 1, not 0 (git reset --hard is never rewritten), and the exit code stays 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions