Skip to content

fix(security): delimit a quoted $( body from a fresh quote context - #20851

Open
Maqix wants to merge 1 commit into
kirodotdev:mainfrom
Maqix:fix/heredoc-substitution-extract
Open

Maqix wants to merge 1 commit into
kirodotdev:mainfrom
Maqix:fix/heredoc-substitution-extract

Conversation

@Maqix

@Maqix Maqix commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Problem / Motivation

Goal: a $(...) inside a double-quoted word is extracted for nested scanning at the span bash actually runs, so a feature push after git commit -m "$(date)" is allowed and a push hidden inside or after the body is refused.

_substitution_bodies (the nested-payload extractor, now in shell_normalizer_span.py) proved each $( / <( / >( span by walking _matching_close_paren from the start of the whole command, so the enclosing word's quote state leaked into the body. The fresh-context walk fixes the simple "$(date)" case, but on its own it still mis-delimits a body whose own interior re-establishes quoting. On main @ 574cb18:

is_denied('git commit -m "$(date)" && git push origin feat')          # refused: unverifiable (over-deny)
is_denied('echo "$(echo "$(echo ")")" ; bash -c \'git push origin main\')"')  # None  (ALLOWED — bypass)
is_denied('echo "$(echo "${x:-")"}" ; bash -c \'git push origin main\')"')     # None  (ALLOWED — bypass)
is_denied('echo "$(cat <<\'E\'\n)\nE\n ; bash -c \'git push origin main\')"')   # mis-delimited

Three interior constructs each open their own quoting/parsing context, so a ) or " inside them is data, not the close of the enclosing $(:

  • a nested $( inside the double-quoted word — the inner " flips the walk's state, the span closes at the first ) after it, and the bash -c '<push main>' behind it is never scanned (the Opus-review BLOCK);
  • a ${...} default value carrying a ) — "${x:-")"}";
  • a heredoc body line holding a ) — $(cat <<'E'\n)\nE\n ... ).

bash runs the push for all three (verified in a subprocess with a fake git on PATH that records its args).

This revision (base origin/main b9ff59ab5e, head fefe107439)

One commit, rebased onto origin/main @ b9ff59ab5e (main #23292 also edits the rm floor: the conflicts in rm_floor.py, its security.md paragraph and _PACKAGE_LINE_BUDGET keep both sides -- main's backtick-body and glued-redirection reads and its _names_rm_word floor-error path stay, beside this change's span reads; the budget is re-pinned to the measured 33,554). Readings reviewers flagged, each confirmed to let a command bash runs slip the gate, are closed. None moves a push, kill or /tmp verdict beyond what each fix requires.

  1. A run-command after a heredoc body inside a bare $(...) was never scanned. A $(...) left unproven by a heredoc still closes, and a process substitution (or a nested $(...) / backtick) bash runs can sit after the body inside it: echo $(cat <<E + newline + E + newline + cat <(bash -c '<publish main>')) runs the <(...), but the suppressed over-reads scanned that text without emitting the opener as its own payload, so the publish — and its self-restart and self-kill twins — was no source and the floor never ran. The span's proven-close body is emitted NON-suppressed now, bounded to the one span (the walk still jumps past it), so the payload walk reaches the nested opener while a run of heredoc spans stays linear.

  2. An apostrophe in a double-quoted assignment prefix desynced the fail-closed home operand. _in_assignment_value opened a single quote on a ' even inside a double-quoted word, so MSG="it's" rm -rf "$(…)" ~ read the unproven span as an assignment VALUE and dropped the synthetic home operand, allowing the $HOME wipe. Double quotes are tracked now, so the rm is seen as a command and its home wipe refused; main refuses it too.

  3. An ANSI-C / locale heredoc delimiter was read as the literal $E. <<$'E' (and <<$"E") has delimiter E after quote removal, so the body terminates and the span closes; reading it as $E left the span unproven to end of text and over-refused a harmless rm -rf "$(cat <<$'E' + newline + /tmp/x + newline + E + newline + )" /tmp/y cleanup. _heredoc_delimiter decodes the quote now, so the /tmp twin returns to main's verdict while a home operand after the same, now-proven span is still refused.

  4. A run-command inside a span that never closes was never scanned. An arithmetic << before a newline (echo $(echo $((1<<2)) + newline + cat <(bash -c '<push>')) ; true) is read as a heredoc with no terminator line, so the span runs to the end of the text and every reading of it was a suppressed over-read; the <(...) bash runs after the newline was no payload source. When an opener sits in such a remainder it is now scanned once, flat, as the capped never-close case already is, and the walk ends there. Push sweeps vs 7cd2aa735e: 0 changes (6,688 + 2,268 + 20,328); 0 protected pushes allowed; rm and kill corpora unchanged.

  5. A later command after an rm-floor span that never closes was never segmented. In the rm floor, a double-quoted $( span the reader cannot close before the text ends (an arithmetic << before a newline, or a heredoc with no terminator) made _segments resume at the end of the command, so echo "$(echo $((1<<2)) + newline + )" ; rm -r -f $HOME (and && / | / newline, ~, ${HOME}, /, an x= or rm -f lead) was never read: bash closes the span and runs the rm. Such a span now refuses the whole command through the floor's existing unread-command refusal; a capped span closed at a guess keeps segmenting after the guess. Over 90 probe rows head was allowing 75 that main refuses (60 are real home or / wipes in bash; the || rows never reach rm); all 90 are refused now, including 15 /tmp twins, since the span cannot be read. 4,795-command rm corpus: 0 home/root rows head allows that main refuses; 27 rows move allowed→denied vs 94a81f2fce, all allowed on main too (a deep x="$(…${x:-(}…)" rm -fr … chain): 24 are real home or / wipes, 3 are /tmp over-refusals. Push sweeps vs 94a81f2fce: 0 changes (6,688 + 2,268 + 20,328), 0 protected pushes allowed; kill corpus identical to main; every earlier probe identical to the prior head. Gates: 1,894 passed, plus the 2 markdown_it host-import failures.

  6. A self-kill after a span that never closes was never walked. The raw self-kill walk ended at a span the heredoc rule never closes (k=kill; x=$(echo $((1<<2)) + newline + ); $k $(printf ')' ; pgrep -f kirocrew)), so the later decoyed $k $(...) sibling bash runs was never reached; main refuses it. Both the $( and <(/>( branches now resume at the close a heredoc-agnostic walk finds (bash reads that << as a shift), so the sibling is walked; the nginx twin stays allowed.

  7. A heredoc terminator glued to the close (EOF)") was read as a body line. bash ends the body there inside $(...) (measured on bash 5.2, with a warning), but the reader ran the span to the end, so the rm floor refused an ordinary git commit -m "$(cat <<'EOF' + newline + fix: confirm the terminal format + newline + EOF)" (it holds a literal rm in confirm/format). A line that starts with the delimiter followed by ) now ends the last pending body; 'EOF', EOF, "EOF" and <<- forms return to main's verdict, while ; rm -rf ~ after them and a ~ operand after the span stay refused. EOFX) (not the delimiter) is still read as a body line and refused, the fail-closed side.
    Verification of 6–7 vs 631c965cc2 and main d2417a48c1: c115 probe (36 rows) identical to main except the EOFX) row; every earlier probe identical to the prior head; push sweeps 0 changes (6,688 + 2,268 + 20,328), 0 protected pushes allowed; rm corpus (4,795) 0 changes; kill corpus (546) identical to main; span timing still linear (0.82 s at 5.8 KB). Gates: 1,896 passed, plus the 2 markdown_it host-import failures.

  8. A quote on a heredoc body line after a span that never closes hid a later opener. The flat pass of item 4 read quotes from a neutral state with no heredoc model, so a real heredoc after the arithmetic line whose body holds an unbalanced quote (it's, $'x\'y) opened a quote over the rest of the text, and a later <(...) / >(...) self restart or self kill was no payload source while bash runs it (bolichen97, reviews on 94a81f2fce through ef9fb000ed). The never-close remainder is now also scanned quote-blind (_capped_tail_entries(quote_blind=True)): every opener, backtick and ) is a boundary, so no body quote can swallow an opener. Its runs stay disjoint, so the walk stays linear.
    Verification of 8 vs ef9fb000ed and main 7d63942c4c: c118 probe (1,920 rows: 3 arithmetic spellings x 5 body quotes/parens x 4 openers x 4 outers x 2 orderings x restart / stop / pkill / nginx twin) has 0 rows this head allows that main refuses (48 before, 24 of them run the restart or kill in real bash with stubs), and every twin keeps main's verdict; every earlier probe identical to the prior head; push sweeps 0 changes (6,688 + 2,268 + 20,328), 0 protected pushes allowed; rm corpus (4,795) 0 changes; kill corpus (546) identical to main; span timing linear (0.82 s at 5.8 KB), the arithmetic run unchanged from the prior head. Gates: the gate test set passes as on the prior head plus the new test, with the same 2 markdown_it host-import failures.

  9. A double-quoted heredoc delimiter lost a backslash bash keeps. Inside "..." (and $"...") bash drops a backslash only before $, a backtick, ", \ or a newline, so <<"E\q" ends at a line E\q (measured on bash 5.2). _heredoc_delimiter dropped every backslash and read it as Eq, so a body line Eq)" closed the span early and the quote state after it hid the home operand: rm -rf "$(cat <<"E\q" + newline + " + newline + Eq)" + newline + E\q + newline + )" ~ was allowed while bash hands rm the home directory (GPT 6.1, review on 3a84d0827c). Both quote forms now unescape with bash's double-quote rules (_dq_unescape).

  10. An arithmetic << that closes the span LATE inside a quote hid the operand after the real ). In rm -r -f "$(echo $((1<<X)) + newline + )" ~ ; : ' + newline + X + newline + )' the <<X inside $(( was recorded as a pending heredoc X, the body lines up to the later X line were skipped as data, and the span closed unproven at the ) inside the trailing single quote (< len(body), so not the run-to-end case item 5 already refuses). The ~ after the real ) was skipped as heredoc data and never segmented, so bash's rm -r -f 1 ~ slipped (Opus review on f671e86f5c). The rm floor now refuses the whole command when an unproven span has a << after an arithmetic opener ((( or $[), the same way item 5 refuses a span that runs to the end: bash reads a shift there, so the walk's close can sit anywhere past the real ), however much text comes between. Any << counts, not just the first, so a real heredoc placed first ("$(cat <<E⏎E⏎echo $((1<<X))⏎)" ~) does not hide the later shift. A genuine heredoc span ($(cat <<EOF ...)) has no such opener and keeps its own reading. The capped-span assignment check also steps over a leading redirection (and its separate target word), which bash applies before the command, so 2>/dev/null x="$(<capped>)" rm -r -f ~ keeps its rm (both Opus findings on 3418e15a0d). c124 probes (home / root / $HOME / spaced / inner-assign / && / | / newline / assignment-prefix / echo-prefix operands, the same rows padded past 400 chars, plus genuine-heredoc and /tmp twins): 0 home/root rows head allows that main refuses; every row that moves allowed->denied vs f671e86f5c is a real home or / wipe, or a /tmp twin of this rare arithmetic shape (the disclosed trade-off, as for item 5); genuine-heredoc /tmp twins stay allowed. Every earlier probe (c102/c103/c104/c106/c109/c111/c113/c114/c115/c116/c119/c120x) is 0 flips vs f671e86f5c; the 4,795-command rm corpus has 0 home/root leaks and 0 flips vs the prior head; push sweeps (6,688 + 2,268 + 20,328) 0 flips, 0 protected; kill corpus (546) identical to main; span timing linear. Gates: 1,914 passed (only the 2 markdown_it host-import failures); black / isort / flake8 / mypy clean.

  11. A shift quoted in heredoc data was refused, and a shift could still hide a self kill. The item 10 check searched the span's whole text, heredoc body included, so a commit or PR message quoting $((1<<2)) was refused whenever the command named rm (Opus and Security Scope on 05b0cb9d0f; main allows those). _close_paren_walk now reports (arith_hits) each SOURCE << it reads as a heredoc after an arithmetic opener -- $((, ((, $[, an array subscript a[ or =( [ (bash reads a[1<<X ]=1 as arithmetic, bash-measured; that subscript shape hid ~ on 05b0cb9d0f, main refuses it) -- and a << in a skipped body reports nothing. For a reported span the rm floor also takes the heredoc-free reading: the same close means no doubt; a heredoc reading that runs to the end uses the heredoc-free ) (bash shifts, or the never-ending heredoc is a syntax error that runs nothing, bash-measured) and reads the span's own text with no heredocs; any other mismatch refuses the whole command, as item 10 did. So BYTES="$(⏎echo $((4 << 20))⏎)"⏎rm -rf ./dist and rm -rf "$(echo $((1<<2))⏎)" /tmp/x keep main's verdict while the home and root rows stay refused. The self-kill raw walk had the same blind spot (k=kill; x=$(echo $((1<<X))⏎); $k $(printf ')' ; pgrep -f <name>) ; : '⏎X⏎)', and the <( and ${y:-$(...)} forms): when an arithmetic opener comes before a << it now also walks with no heredocs and both readings are searched, and _matching_close_brace hands heredocs to its nested walks.
    Verification of 11 vs 05b0cb9d0f and main 4d5d86af80: c126 probe (self kill x3 + nginx twins, heredoc-data commit / PR bodies, the 3 Scope rows, bolichen97's 3 rows, 24 kept home/root rows) matches main except the <<'' empty-delimiter /tmp row (still refused, left for a follow-up) and x="$(echo $((1<<2))⏎rm -rf $HOME⏎)" (refused here, allowed on main; bash wipes home); array-subscript rows: home refused as on main, /tmp twins refused (the item 10 trade-off). Every earlier probe moves only /tmp twins denied->allowed, each to main's verdict, plus c120x's c=${0:0:0}rm; $c -rf ~ after the span, which main and both heads allow with no span at all (an rm-floor gap that predates this PR). rm corpus (4,795) 0 changes; push sweeps (6,688 + 2,268 + 20,328) 0 changes, 0 protected pushes allowed; kill corpus (546) identical to main; span timing linear (0.83 s at 5.8 KB). Gates: 1,918 passed, plus the 2 markdown_it host-import failures; the new tests fail on 05b0cb9d0f.

  12. A capped span as a leading redirection target hid the rm after it. The capped-span home-operand fail-closed (reading a capped $(...) output as ~) was applied whenever the span was not a NAME= assignment value, so a leading redirection target got it too: in >"$(<capped>)" rm -fr ~ the synthetic ~ landed in the redirection target, _rm_wipe_targets stepped the > redirection over it, took ~ as the program, and skipped the segment, so the real rm -fr ~ after it was never read and bash ran a home wipe (Opus review on 536d7e80d9, rm_floor.py:431). Position is now decided by _rm_operand_position (replacing the not-an-assignment check): the synthetic ~ is appended only when the open word holding the $( is an OPERAND after a completed command word, ordering the simple command as bash does (assignment prefixes, then redirections with their glued or separate targets, then the command word, then operands). So a redirection operator, its target, and the assignment-prefix run all get a plain placeholder and the real rm is seen.

  13. A $(...) heredoc delimiter was read as $, closing the span early. bash performs no expansion on a heredoc delimiter word but does not treat a $(...) or backtick in it as word-ending metacharacters -- the whole construct is the delimiter (bash 5.2: <<$(echo E) ends the body at a line $(echo E), not $, while a bare <<a(b ends the word at the (). _heredoc_delimiter broke at the (, left the delimiter as $, and read the )" <target> operator-line tail as body, closing the enclosing span early and hiding the rm target after it: rm -fr "$(cat <<$(echo E)⏎$(echo E)⏎)" ~ ; ... wiped home while the floor allowed it (Opus review on 536d7e80d9, shell_normalizer_span.py:1190). Reading the construct to its real end costs a balanced walk at EACH <<, so echo $((x<<$((x<<... (a shift run, every << seen by the parser) would go quadratic; so a delimiter that opens a $( substitution or a backtick now leaves the span unread and the floor refuses it, fail-closed. A plain ${NAME} delimiter has no word-ending char and is still read whole, unchanged.
    Verification of 12-13 vs 536d7e80d9 and main 9d2df153d2: a 113-row c128 probe (F1: > / > / 2> / < / x=1 > / >...x=1 / ; -led redirection targets holding a 33- and 40-deep capped span, x ~ / / / $HOME / /tmp x rm -fr / rm -rf / rm --recursive --force; F2: $(echo E) / ${E} / backtick / x$(e) delimiters x the same operands) has 0 home/root rows head allows that main refuses; vs 536d7e80d9, 45 rows flip allowed->denied (42 F1 redirection-target home/root wipes + 3 F2 $(echo E) home/root wipes), each a real $HOME or / wipe in bash with a stub rm, and the one $(echo E) /tmp twin is over-refused (the disclosed fail-closed cost of the unread delimiter, same family as the backtick / x$(e) /tmp twins already refused). The capped span as a plain rm OPERAND still fails closed (unchanged). Every earlier probe (c102..c126_sub2) is 0 flips vs 536d7e80d9; the 4,795-command rm corpus has 0 home/root leaks and 0 flips vs the prior head; push sweeps (6,688 + 2,268) 0 flips, 0 protected; kill corpus (546) identical to main; span timing linear and the arithmetic-shift liveness test passes (the quadratic the balanced-walk reading would have caused is avoided). Gates: 1,643 security / push / argv-floor / denied-rule tests pass, plus 69 facade-mirror tests; black / isort / flake8 / mypy clean; one commit over origin/main. The new tests test_a_capped_span_as_a_redirection_target_cannot_mask_a_home_rm and test_a_substitution_heredoc_delimiter_cannot_mask_a_home_rm fail on 536d7e80d9 and pass here.

Verification of 9 vs 3a84d0827c and main ea89e96911: c119 probe (GPT's row and variants) — the 6 home and / rows the prior head allowed are refused, as on main, and every row matches main; c119 sweep (288 rows: 12 escaped characters x both quote forms x kept/dropped early line x ~ / / / /tmp x 2 outer forms, each run in real bash with a stub rm) — 0 rows this head allows that wipe the home directory or / (20 on the prior head, all refused by main), 0 rows this head refuses that main allows and bash runs harmlessly; every earlier probe identical to the prior head; c118 rows (1,920) 0 rows this head allows that main refuses; push sweeps 0 changes (6,688 + 2,268 + 20,328), 0 protected pushes allowed; rm corpus (4,795) 0 changes; kill corpus (546) identical to main; span timing linear (0.82 s at 5.8 KB). Gates: 1,848 passed, plus the 2 markdown_it host-import failures; one commit.

Verification against main @ 41708c96ab and the prior head f5bf048215 (every allowed/denied command confirmed in real bash with stub binaries that delete/push/kill nothing):

  • c111 probe: the (a), (a') and (b) home/self-protection rows are refused; the feature-push and nginx twins and the (c) /tmp rows stay at main's verdict. The (b) /tmp arithmetic twin is refused as the documented fail-closed consequence of unmodelled arithmetic ($(( 1<<2 )) reads as a heredoc), not a new class.
  • rm sweeps: c110_strict over c104 / c106 / c109 / c103 — 48 / 90 / 12 / 9 rows refuse that main allows, every one a real $HOME or / wipe under a stub rm, 0 harmless over-refusals. 4,795-command rm corpus: 0 home/root rows head allows that main refuses; 0 verdict changes vs f5bf048215 (head and f5bf048215 both deny 3,200).
  • Push corpus (20,328): 0 rows head allows that main refuses; 0 protected pushes allowed (head allows 3,715). Six rows flip allowed→denied vs f5bf048215, each a heredoc-obfuscated push the (a) fix now reaches, refused fail-closed — a tightening, never a loosening. Kill corpus (546): identical to main. Nested / heredoc / backtick push probes: 0 loosening flips.
  • Liveness: a run of nested heredoc spans still grows ~2× per doubling (c104v_time n = 50 / 100 / 200: 0.19 / 0.38 / 0.75 s).
  • _PACKAGE_LINE_BUDGET re-pinned to the measured cat src/kiro_crew/security/*.py | wc -l = 33,525 (+2: the argv_floor facade manifest now names the two new forwarded helpers, _ARITH_CONTEXT_RE and _arith_shift_possible, which test_facade_mirror_guards.py requires). Gates green: 9,058 security, push, argv-floor and denied-rule tests pass, plus all 69 facade-mirror tests; black / flake8 clean.

The tests added for this revision are, in TestAMisreadSpanFailsClosed, test_a_run_command_after_a_heredoc_body_in_a_bare_span_is_denied (a push / self-restart through <(...) / $(...) / backtick after a heredoc body, refused; feature-push and nginx twins allowed), test_an_apostrophe_in_a_quoted_assignment_prefix_keeps_the_home_rm (the MSG="it's" home rows refused), and test_an_ansi_c_heredoc_delimiter_is_decoded (the $'E' / $"E" /tmp cleanup allowed, a home operand after the same span refused); plus, in the self-kill suite, test_a_kill_in_a_run_command_after_a_heredoc_body_is_a_self_kill (and its non-protected-target twin) covering the same span reading on the kill floor. For this reading, test_an_arithmetic_shift_closing_late_in_a_quote_keeps_the_home_rm asserts Opus's command and the spaced / inner-assignment / && / | / newline / assignment-prefix / echo-prefix home and root variants are refused, a genuine-heredoc operand after a real close is refused, and the /tmp twin is allowed. For item 11, test_a_shift_quoted_in_a_heredoc_body_is_data, test_a_shift_whose_heredoc_reading_never_ends_uses_the_shift_close, test_an_array_subscript_shift_cannot_hide_a_home_rm and test_a_self_kill_after_a_shift_that_closes_late_is_denied.

Prior revision (the heredoc body read structurally as data, base origin/main b2b87b0f1a, head 76e9cc8a4a)

One commit, still based on origin/main @ b2b87b0f1a (a test rebase onto the current origin/main is clean, so this is not rebased). This revision ends the per-character patching of heredoc body lines by reading the body structurally as data. Once a << operator's delimiter is parsed, every line of its body is skipped — no quote, opener, backtick, paren or depth state — until the terminator line (<<- strips leading tabs; <<'E' / <<"E" / <<\E unquote the delimiter), and the walk then resumes normal reading. The span is left unproven (the push floor still over-reads the body), but its close index is the real ), so the rm floor reads the operand after it.

Two body-line classes the prior per-character rounds still mis-sized, each closed by reading the whole body as data:

  • a ) inside a nested quoted substitution closed the inner span early, and an unclosed ${ / $( / $(( / ( / backtick later on the same body line then ran one unproven span to the end of the text, hiding the home operand after the real close;
  • an ANSI-C $'a\'b on a body line was read as closing on its line while the walk kept it open.

_heredoc_delimiter (delimiter parsing) and _skip_heredoc_bodies (line skipping) replace the _quote_closes_on_line helper and the per-character body-quote / body-backtick / body-opener / bare-( branches. A span the reader cannot prove that also runs to the end of its text — an unparsable delimiter, or a heredoc with no terminator line — now fails closed on a home/root operand in the rm floor, beside the capped span it already covered.

Verification against main @ 41708c96ab and the prior head f5bf048215:

  • c109 probe (232 rows, the two classes above × <<E / <<'E' × direct / nested / post-terminator × home/root//tmp): 0 home/root rows head allows that main refuses; head refuses 12 direct ANSI-C home/root rows main allows, each a real wipe (bash hands $HOME or / to a stub rm); all /tmp twins match main.
  • The earlier heredoc probes c104 (home-operand opener family) and c103 (nested-opener family): head refuses 48 and 9 home/root rows main allows, each a real wipe; every /tmp twin matches main; 0 rows head allows that main refuses. c102: identical to main.
  • 4,795-command rm corpus: 0 home/root rows head allows that main refuses; 0 verdict changes vs f5bf048215.
  • Kill corpus (546 commands): identical to main. Push corpus (20,328 commands): 0 flips vs f5bf048215; 0 protected pushes allowed (head and main each allow 3,721 corpus rows, 0 protected among either).
  • Liveness: a run of nested heredoc spans grows ~2× per doubling (c104v_time n = 50 / 100 / 200: 0.19 / 0.37 / 0.73 s).
  • _PACKAGE_LINE_BUDGET re-pinned to the measured cat src/kiro_crew/security/*.py | wc -l = 32,969. Gates green (pytest: the only 2 failures are the markdown_it host-import gap, 1,884 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit over origin/main.

The test added for this revision is TestAMisreadSpanFailsClosed::test_the_heredoc_body_is_read_structurally_to_its_terminator (both c109 classes × <<E / <<'E' × direct / nested / post-terminator × home/root, every row refused, each /tmp twin allowed; plus a terminator-tracking row — a <<-E with a tab-indented terminator and two heredocs pending on one line — refused on home/root and allowed on its /tmp twin).

Prior revision (the heredoc-body quote family and a linear nested-span walk, base origin/main b2b87b0, head f5bf048)

Two changes on top of the prior pushed head 3032ab76a0, rebased onto origin/main @ b2b87b0f1a (one commit). The rebase conflicts were _PACKAGE_LINE_BUDGET and test/test_security.py, which main split (#23119): the five assertion updates this change makes there now land in test/test_security_shell_grammar.py, where those tests moved. None of main's new commits touch the security files this change edits.

  1. A quote on a heredoc BODY line is data. A ' / " / ANSI-C $' on a body line is printed stdin data, not a live quote, but the span walk let an unpaired one open a quote whose state ran past the span's real ), so the home operand after the span never reached the rm floor (rm -rf "$(cat <<E + newline + it's + newline + E + newline + )" $HOME). The walk now reads a body-line quote that does not close on its own line as data, like the backtick and opener branches, so the span closes at its real ) and the operand after it is read. A body-line quote that DOES close on its own line keeps the paired reading, so its interior stays quoted and a body ")" cannot close the span early and leave the line's closing quote over the operand behind it.
  2. The nested-payload walk no longer re-extracts a span-walk over-read. Its text is already scanned whole and its real nested openers are emitted by the structural walk, so re-walking each near-full remainder of a run of nested heredoc spans was an O(n²) that bought no finding. The walk is linear again.

Verification against main @ b2b87b0f1a and the prior head 3032ab76a0:

  • rm probes (the earlier 24-, 58- and 120-row heredoc probes): head matches main on every row. Heredoc-body quote probe (288 rows): 0 home/root rows head allows that main refuses; head refuses 90 home/root rows main allows, each a real wipe; all 72 /tmp twins match main.
  • 4,795-command rm corpus: 0 home/root rows head allows that main refuses; 0 verdict changes vs 3032ab76a0.
  • Kill corpus (546 commands): identical to main. Nested sweep (2,268): 0 flips. Heredoc and backtick push probes: 0 flips.
  • Push: 24 of 6,688 probe rows and 6 of 20,328 corpus rows move denied→allowed vs 3032ab76a0. Every one is a FEATURE-branch push (git push [-u] origin feat through bash -c / sh -c / eval) behind a here-string-wrapped heredoc span, which real bash runs as that feature push and nothing else. 0 protected pushes allowed (head allows 3,721 corpus rows, main 3,715; 0 protected among either).
  • Liveness: a run of nested heredoc spans grows ~2× per doubling (n = 50 / 100 / 200: 0.19 / 0.37 / 0.73 s); the previous head took 47 s at n = 200.
  • _PACKAGE_LINE_BUDGET re-pinned to the measured cat src/kiro_crew/security/*.py | wc -l = 32,927. Gates green (pytest: the only 2 failures are the markdown_it host-import gap, 1,883 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit over origin/main.

The tests added for this revision are TestAMisreadSpanFailsClosed::test_the_heredoc_body_quote_family_cannot_mask_a_home_rm (unpaired body quotes × <<E / <<'E' / <<"E" × direct / nested / post-terminator × home/root, every row refused; the direct and nested /tmp twins stay allowed) and test_a_body_quote_closed_on_its_line_keeps_its_interior_quoted (paired body quotes around a ), plus the earlier heredoc-body rows, every home/root row refused; the /tmp twins stay allowed). A liveness row pins the near-linear growth of the nested-span run.

Prior revision (the heredoc-body rm opener family, base origin/main 64576241e0, head 3032ab76a0)

The heredoc-body rm family reviewers flagged on aca7ad2404 is closed, rebased onto origin/main @ 64576241e0 (one commit; the only rebase conflict was _PACKAGE_LINE_BUDGET, and none of main's new commits touch the files this change edits). A backtick, a $( / <( / >( opener and a bare ( on a heredoc BODY line are stdin data the here-document prints, not live units. Two readings still treated them as units:

  • A body-line backtick paired with a later backtick (; echo '') through _backtick_closer, so the walk skipped past the span's real )` and the home operand after it never reached the rm floor. A body-line backtick is now data at every nest level, and the span is left unproven.
  • A body-line two-char opener stepped over only its first char, so its ( counted toward depth. Nested one level inside a quoted "$(echo ...)", that extra depth ate the enclosing span's real ) (a bare ( body line did the same). One level or more deep, the whole opener and a bare ( are now data with no depth change, and the span is left unproven. At the top level the direct rows keep main's verdict.

Verification against the prior head aca7ad2404 and main:

  • Heredoc-body probe (120 rows: backtick / ( / $( / <( / >( bodies × <<E / <<'E' × direct / nested / later-backtick × home / root / /tmp): head matches main on every row; aca7ad2404 allowed 30 rows main refuses. The earlier rm probes (24 + 58 rows) also match main on every row.
  • 4,795-command rm corpus: 0 home/root rows head allows that main refuses; 0 verdict changes vs aca7ad2404.
  • Push verdicts: 0 changes vs aca7ad2404 across 29,284 commands (6,688 + 2,268 probe/nested sweeps and the 20,328-command corpus), plus the heredoc and backtick push probes.
  • Kill corpus (546 commands): identical to main.
  • _PACKAGE_LINE_BUDGET re-pinned to the measured cat src/kiro_crew/security/*.py | wc -l = 32,811. Gates green (pytest -n 4: the only 2 failures are the markdown_it host-import gap, 1,880 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit over origin/main.

The test added for this revision is TestAMisreadSpanFailsClosed::test_the_whole_heredoc_body_opener_family_cannot_mask_a_home_rm (every home/root row refused, as on main; every /tmp twin stays allowed).

Prior revision (the last heredoc-body rm hole, base origin/main 8e80404a00, head aca7ad2404)

One more reviewer-flagged home-directory rm bypass is closed on top of the prior pushed head f1d2e350b4, rebased onto origin/main @ 8e80404a00 (one commit; the only rebase conflict was _PACKAGE_LINE_BUDGET, and none of main's new commits touch the files this change edits):

A nested opener on a heredoc BODY line could leave the operand after the span unread. A ${ / ${x / ${x:- (or $( / <( / >() on a heredoc body line is stdin DATA the here-document prints, not a live nested unit, but the span walk still skipped it as a unit there. For a ${ with no } on the body line that skip ran to the end of the command, so the walk never reached the enclosing span's real ) and the rm segment never saw the home operand after it — rm -rf "$(cat <<E + newline + ${ + newline + E + newline + )" ~ wiped $HOME while the floor allowed it, where main refuses it. Once a heredoc is pending and its newline has passed (the walk is reading body lines), the walk now steps over just the opener's first char and leaves the span unproven — the same fail-closed, one-char step the unterminated-backtick branch already uses — instead of unit-skipping. The span then resumes at the real close and the ~/$HOME/${HOME} after it reaches the rm floor (or fails closed on a home operand). The /tmp twin of the same shape stays allowed, as on main. (Found while hunting the family of such shapes a reviewer flagged; this is one member, not a claim that it is the exact input shared privately.)

Verification for this revision (every allowed command run in real bash with a stub rm that deletes nothing):

  • Heredoc-body family corpus (4,000 commands = span openers × body-line opener shapes × rm/home/root//tmp spellings): head is byte-identical to main (0 head-allow/main-deny, 0 /tmp-twin over-deny); vs the prior head f1d2e350b4, 288 home rows flip allowed→denied and 0 denied→allowed. The prior c102 home-rm corpus (1,920) and the parent's 4,795-command rm corpus are unchanged: head still matches main with 0 home head-allow/main-deny (the 14 /tmp divergences are main over-refusing capped spans, 0 home).
  • Real bash (stub rm): the body-line ${/${x/${x:- shapes the prior head allowed are confirmed live $HOME// wipes and are all refused now; their /tmp twins stay allowed on both. (The $(/<(/>( body-line shapes are allowed on BOTH head and main — a shared pre-existing rm-floor seatbelt limit, not a head-allow/main-deny regression; the fix scopes to the head-allow/main-deny ${ family.)
  • Push + kill sweeps unchanged (the change is rm-and-span-local): probe 6,688, nested 2,268, c97 20,328, self-kill 546 — 0 flips vs f1d2e350b4 in either direction; 0 protected pushes allowed in real bash.
  • Liveness: the new body-line shape stays linear — a ${-per-body-line run near 20 KB grows exactly ~2× per doubling (2.3 s at ~9.6 KB, 4.7 s at ~19 KB), no quadratic.
  • _PACKAGE_LINE_BUDGET re-pinned to the measured cat src/kiro_crew/security/*.py | wc -l = 32,775. Gates green (pytest -n 4: the only 2 failures are the markdown_it host-import gap, 1,879 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit over origin/main.

The test added for this revision is TestAMisreadSpanFailsClosed::test_a_heredoc_body_opener_cannot_mask_a_home_rm (span openers "$(cat <<E / <<-E / <<'E' × body openers ${ / ${x / ${x:- × home/root operands, each ALLOWED on f1d2e350b4, refused here and on main; the /tmp twins stay allowed).

Prior revision (two home-directory rm holes, base origin/main 5ea67b3cb4, head f1d2e350b4)

Two reviewer-confirmed home-directory rm bypasses are closed on top of the prior pushed head 8b6cfaabcc, rebased onto origin/main @ 5ea67b3cb4 (one commit; the only rebase conflict was _PACKAGE_LINE_BUDGET, and none of main's new commits touch the files this change edits):

  1. The capped-span assignment-value exemption was start-anchored. The rm floor's capped-span fail-closed reads a capped $(...) output as a home operand (~) so rm -rf "$(<capped>)" ~ fails closed, but it must NOT do that when the span is a NAME= assignment VALUE — there the synthetic ~ lands between the assignment and a real rm after it, so x="$(<capped>)" rm -rf $HOME was read as the command ~ with rm as its argument and the home wipe was missed. The prior head decided "assignment value" with a start-anchored regex that matched only when NAME=" began the segment, so anything before it — a separator-cut leading space (true; x="…"), an earlier completed assignment (a="p q" x="…"), a partial value (x="a$(…") — slipped past it and the synthetic ~ masked the rm anyway. "Assignment value" is now decided from word structure (_in_assignment_value), quote-aware: the $( is in an assignment value exactly when the current open word is a NAME= / NAME+= word and every completed word before it was too. All the prefixed shapes are read as the assignment values they are, so the real rm is seen; main refuses them too. += stays at main's verdict (its env-strip leaves x+= in place).

  2. A heredoc body inside a double-quoted span could leave the operand after it unread. A $(...) whose heredoc body held an unterminated backtick left the span unproven and made the backtick scan swallow the rest of the command, so the walk never reached the span's real ) and the enclosing rm segment never saw the home operand after it — a rm -rf "$(…)" ~ shape wiped $HOME while the floor allowed it, where main refuses it. The span walk's unterminated-backtick branch now steps over just that one backtick and keeps scanning (marking the span unproven) instead of jumping to the end, so the span resumes at the real close and the ~/$HOME after it reaches the rm floor, or fails closed on a home operand. The /tmp twin of the same shape stays allowed, as on main. (Found while hunting the family of such shapes a reviewer flagged; this is one member, not a claim that it is the exact input shared privately.)

Verification for this revision (every allowed command run in real bash with a stub rm that deletes nothing):

  • Home-rm family corpus (1,920 commands = assignment-prefix shapes × heredoc-body shapes × rm/home//tmp spellings): head is byte-identical to main (0 head-allow/main-deny, 0 tmp-twin over-deny); vs the prior head 8b6cfaabcc, 270 home rows flip allowed→denied and 0 denied→allowed. The parent's independent 4,795-command rm corpus: of the 14 head allows that main refuses, all are /tmp operands (main over-refusing capped spans), 0 are home.
  • Real bash (stub rm): the 14 separator/prefix shapes the prior head allowed are confirmed live $HOME wipes and are all refused now; the confirmed backtick heredoc shape is refused now (matching main) and its /tmp twin stays allowed.
  • Push + kill sweeps unchanged (rm-and-span-local change): probe 6,688, nested 2,268, c97 20,328, self-kill 546 — 0 flips vs 8b6cfaabcc in either direction.
  • Liveness: the backtick-heredoc and assignment shapes stay linear (~2× per doubling), head timing equal to the prior head (the backtick unit-skip pairs each backtick with the next, so only the final unpaired one changes path — no quadratic).
  • _PACKAGE_LINE_BUDGET re-pinned to the measured cat src/kiro_crew/security/*.py | wc -l = 32,736. Gates green (pytest -n 4: the only 2 failures are the markdown_it host-import gap, 1,878 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit over origin/main.
Prior revision (maintainer round, base 398772ffdd, head 8b6cfaabcc)

Three changes on top of the prior pushed head 8939c641f2, rebased onto origin/main @ 398772ffdd (one commit; the only rebase conflict was _PACKAGE_LINE_BUDGET, and none of main's new commits touch the files this change edits):

  1. Opus 5.5 BLOCK — a capped span that closes EARLY. A data ) read past _MAX_SPAN_NESTING (${x:-)}) can close a capped span one ) too soon, so the span's own ; bash -c '<push>' before bash's real close sat past the guessed close, reached by no opener, and was never a payload source: echo "$( + $(echo …33-deep… ${x:-)} …33… + ; bash -c '<push main>')" was ALLOWED while bash pushes main. The capped branch already scanned past a LATE close; it now also scans the text past an EARLY (guessed) close once per walk (_capped_tail_entries(..., reopen=True)), every ) there a boundary that closes nothing however early the guess was, so the launcher is read and a run of capped spans stays linear.

  2. Maintainer decision — drop the $(( / (( arithmetic handling and fail closed. Arithmetic is no longer modelled: $(( and (( are read as plain parens, so a << shift inside them counts as a heredoc like any other <<. A shift followed by a newline before the span closes therefore leaves the span unproven and the whole remainder is scanned — the fail-closed direction. The (( 1<<2 ))-and-a-newline cases are now expected refusals, documented and tested as such (TestAnArithmeticShiftIsAnExpectedRefusal) rather than as cases to keep allowing. Removing the arithmetic seed also deletes the per-opener adjacency scan and the dual-reading second pass it carried, so _close_paren_walk is a single linear pass with no can_arith parameter.

  3. A home-directory rm bypass (found while looking for cruisercohen's privately shared row; not yet confirmed to be the same input). The rm floor's capped-span fail-closed reads a capped $(...) output as a home operand (~) so rm -rf "$(<capped>)" ~ fails closed. That synthetic ~ was inserted even when the capped span was a VAR="..." assignment VALUE, where the span feeds a variable, not an rm: in x="$(<capped>)" rm -rf $HOME the ~ landed between the assignment and the real rm, so _rm_wipe_targets read ~ as the command and rm -rf $HOME as its arguments and missed the home wipe — ALLOWED since at least 629d0e4292, while main refuses it. The synthetic operand is now added only in operand/command position (_ASSIGNMENT_VALUE_OPENER_RE recognises a leading [VAR=val …] NAME=" so the assignment-value case is excluded), so the real rm is seen; main refuses it too. The capped-span-as-operand case (rm -rf "$(<capped>)" ~) still fails closed.

Verification for this revision (every allowed command run in real bash with a stub git, and the rm rows with a stub rm that deletes nothing / bash -n for the /bin/rm shapes):

  • Push sweeps vs 8939c641f2: probe 5,104, nested 2,268, c97 20,328, early-close 6,720 — all before→head flips are allowed→denied (0 denied→allowed), and 0 of the commands head allows pushes a protected branch in real bash.
  • Home-rm corpus (41,104 commands = 8 rm spellings × 7 home operands × many decoy spans × positions): of the 358 head allows that main refuses, 0 actually wipe ~/$HOME in real bash — 320 run but target no home dir, and the other 38 (/bin/rm forms behind an unterminated backtick) are all bash -n syntax errors, so rm never runs. The 82 VAR="$(<capped>)" rm -rf $HOME shapes that the prior head allowed are now refused.
  • Self-kill corpus unchanged by this revision (the self-kill raw-walk fix from the prior round is retained).
  • Liveness: the opus early-close shape near 20 KB stays linear (60→21→40 ms at N=350/700/1400, ~2× per doubling); all existing liveness rows pass.
  • _PACKAGE_LINE_BUDGET is re-pinned to the measured cat src/kiro_crew/security/*.py | wc -l = 32,674 (down from 32,747 at rebase, because dropping the arithmetic seed is a net reduction). Gates green (pytest -n 4: the only 2 failures are the markdown_it host-import gap, 1,877 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit over origin/main.

Why it matters

On main, a push to a protected branch hidden inside a double-quoted $(...) passes the push filter and bash runs it, so an agent steered by injected text can push main with a command the gate allows. The same mis-delimiting also refuses ordinary commands: a feature push after git commit -m "$(date)" is denied with an "unverifiable" reason, and the agent has to reword and retry.

Not a goal

  • The rest of push gate: a feature push after a heredoc commit message is refused #20845: the heredoc commit-message shape (git commit -m "$(cat <<'EOF' ...)" followed by a push) stays refused. Allowing it needs fix(security): keep a quoted newline inside its word on the push path #15438's per-line reading relaxed for heredoc bodies, which is a separate change.
  • Any parser other than _matching_close_paren / _substitution_bodies (both in shell_normalizer_span.py); their own quote handling keeps today's behaviour. Two floors call these two and so DO get the new span: the rm floor (rm_floor._segments) and the kill self-name check (argv_floor_self._bare_kill_raw_bodies and the _substitution_bodies scan in argv_floor_self.py), as does the mint-verb check. Tests pin each one (see Tests).

What changed (motivation → approach → change)

The paren matcher skips each nested construct as a unit, so its inner quotes and parens stay sealed inside it and the enclosing span closes at its own matching ):

  • a nested $( / <( / >( recurses through the walk and resumes the outer quote state across the jump;
  • a ${...} runs to its closing } via a new _matching_close_brace (quote-aware; a nested ${ is skipped as a unit, and a bare { is data, as bash reads it: ${x:-a{b}c} closes at the first }). A ${/$( opener opens only on a lone $ — an even run of literal dollars before it leaves one free, but $$ is the PID and the {/( after it is data, so rm -rf "$(echo $${)" ~ closes at the ) and keeps its ~. The walk carries the length of the current literal $ run, so that check costs O(1) per character (a back-walk over a long $$$… run was quadratic);
  • a <(/>( process substitution opens only unquoted (state 0), so the double-quoted <( in rm -rf "$(echo ${x:-"<("})" ~ is text and the span still closes at its ) with the ~ intact;
  • a backtick ` substitution closes on the next unescaped backtick via _backtick_closer — a backslash escapes it but a single quote does NOT, since bash ends a backtick only on an unescaped backtick (rm -rf "$(echo "`echo '`")" ~ kept its ~ operand only once the skip stopped using the quote-aware matcher that ran to end-of-text).

A heredoc is not parsed (no tag or body reading). A << only marks a heredoc pending, and a newline reached while one is pending leaves the span unproven, so the whole remainder is scanned:

  • a heredoc a nested substitution leaves open ($(cat <<E) closing before its newline) is passed up, so the enclosing span's next newline is unproven too;
  • the ) before that newline still proves the span, so rm -rf "$(cat <<E)" ~ keeps its ~ operand for the rm floor;
  • the index of an unproven span is where the walk closes when it reads any body lines as commands, so a caller that keeps reading the enclosing text (the rm floor) resumes there;
  • a here-string <<< is not a heredoc (its later < characters open nothing). Arithmetic is not modelled (maintainer decision): $(( and (( are read as plain parens, so a << shift inside them counts as a heredoc like any other <<, and a shift followed by a newline before the span closes leaves the span unproven, so the whole remainder is scanned — the fail-closed direction. echo "$((cat <<E\n)\nE\n) ; bash -c '<push main>')" is denied, and the (( 1<<2 ))-on-its-own-line shapes are expected refusals. Any other << counts as a heredoc too; over-reading is the safe side.

Nesting past _MAX_SPAN_NESTING (32) is read without unit skipping and reported unproven, so a deep ${a:-${a:-…}} chain cannot end the walk with a RecursionError.

For every caller, an unproven span is also scanned up to its last ) and in the outer quoting context. Otherwise a payload glued to the text after the real close, such as bash -c 'git push')", reads as git push). All three callers (push payload walk, mint-verb check, kill self-name check) deny if any body matches, so the extra readings can only add findings. These extra readings defend a push glued after a real close that a heredoc (or a genuine unbalance) left unproven. A span unproven only because the nesting passed _MAX_SPAN_NESTING has no glued tail of its own — its structure is all substitutions the payload walk descends anyway — so its last-) and outer-context readings are skipped (adding them re-queued near-duplicate bodies and made the extractor super-linear, so a 400-byte echo $(echo $(… took 69 s before and is now within ~1.2× of main). But a capped span can still hold a heredoc, whose body lines and the command glued after them sit past a newline with the same ) tail, so it is scanned once from its first newline to its last ) — enough to reach a bash -c '<push>' after the heredoc without re-walking the deep prefix. A capped span that CLOSES also CONTINUES the walk past its close, so a real $(...) opener AFTER it — echo $(echo …33-deep… x )…33… ; echo "$(echo "$(bash -c '<push main>')")" — is emitted as its own shallow entry and scanned. Before, the capped branch stopped at the span and the opener after it slipped through (the Opus BLOCK fixed here); re-extracting the whole deep remainder instead would re-walk the nest at every level (the O(n²) an opener run cannot afford).

A capped span that NEVER closes hid a later push the same way. Past the cap the deep interior is read without fresh contexts, so a ) that is only DATA there — a ${x:-(} default value's paren — is counted as a close and consumes one of the span's own ), leaving the span open past where bash balances it. bash runs the opener AFTER it as a sibling, but the gate's over-long span swallowed it: the whole-remainder body is a value-suppressed suffix, so echo $(echo …33-deep… ${x:-(} )…33… ; echo "$(bash -c '<push main>')" was ALLOWED while bash pushed main (the Opus-review BLOCK on this revision). The never-close branch now scans the remainder once, flat and quote-aware (_capped_tail_entries), emitting each later substitution segment — a $(...), a backtick or a launcher script — as its own shallow entry, so the hidden push is read without force-extracting the remainder or stepping back into it opener by opener (either re-walks the deep interior at every level — the O(n²) an opener run cannot afford). A ) that closes a span this scan opened is honoured even inside double quotes (where the lexer reports it inactive), so the segment ends at the real close and a feature push behind the span stays allowed rather than over-refused.

The same data ) can make a capped span close LATE rather than never: at the ) of an unquoted PROCESS substitution bash runs as a sibling after its own close, as in echo $(echo …33-deep… ${x:-(} )…33… ; cat <(bash -c '<push main>'). The span then read as closed, so the walk skipped past it and the <( body was never a payload source; the $(bash spelling of the same tail was refused only because a $(bash token reads as a launcher, which <(bash and x=$(bash do not (bolichen97's BLOCK on d42c8f4: cat <(bash -c|sh -c|eval '<push main>') and echo x > >(bash -c '<push main>'), after ; or &&, were ALLOWED while main refuses them and bash pushes). A late-closing capped span is now scanned once for its substitution segments with _capped_tail_entries, bounded at the close, as the never-close span is, and the walk still continues from the close, so the per-opener whole-remainder slice stays gone and a run stays linear. Rows in TestAMisreadSpanFailsClosed::test_a_capped_span_closing_late_cannot_hide_a_process_substitution (20 deny rows, each allowed on d42c8f4; the benign and feature-push process subs behind the span stay allowed).

A capped span nested inside an OUTER body hid the outer body's own launcher: in echo "$( $(…33-deep… : )…33… ; bash -c '<push main>')" the outer span is capped too, and its segment scan started at depth 0, so it emitted only the text inside the NESTED openers (:). The outer body's own ; bash -c '…' was never a payload source, and at the root the double-quoted word is one token (Opus and bolichen97 BLOCK on 67038bf; unquoted, the same shape was already refused). _capped_tail_entries now starts inside the capped span (depth 1, a segment open at its start), in both capped branches, so the span's own top-level text is a segment too, in the same single linear pass. Rows in TestAMisreadSpanFailsClosed::test_a_capped_span_inside_an_outer_body_cannot_hide_its_launcher (24 deny rows: echo "$( / x="$( × ; / && × bash -c / sh -c / eval × main / mainline, each allowed on 67038bf; the feature-push and echo done twins stay allowed).

The payload walk suppressed re-extraction of any substitution body whose value was a suffix of its source. A proven body can equal the end of its source — echo $(x <(bash -c 'git p""ush origin main') \') 'x <(bash -c 'git p""ush origin main') \' — so it was queued without extracting its <(, and the later copy was then dropped as already seen (Opus and bolichen97 BLOCK on 67038bf; the plain push spelling too). Suppression is now by entry: _substitution_body_entries flags only its over-reads (an unproven span's full remainder, glued tails and later span bodies, and a capped span's heredoc tail — the readings whose later openers the same walk emits as their own entries) as suppressible, still only while a suffix of the source. A proven body, a capped segment and a data or unproven-backtick remainder are always re-extracted, and a payload first queued suppressed is queued again when it recurs as a body that must be extracted. _value_suppressed_suffix_bodies is removed. Rows in TestAMisreadSpanFailsClosed::test_a_proven_body_equal_to_a_source_suffix_is_re_extracted (24 deny rows: git push / git p""ush × <( / >( × three launchers × main / mainline, each allowed on 67038bf; the feature-push and echo done twins stay allowed). Flagging only the full remainder, as first proposed, is not enough for liveness: the glued tails of a periodic heredoc run are suffixes too, and re-extracting them made echo "$(cat <<E\n) "×N quadratic again (no verdict within 10 minutes at the 20 KB cap), so they keep the suppressibility they had.

A run of sequential unproven heredoc spans — echo "$(cat <<E\n) " repeated N times — had the same whole-remainder glued-tail over-read re-queued at every opener, and the payload walk recursed into each near-full remainder, which begins with the next opener and re-queues another near-full remainder: O(n²) in the command's length, and at the 20 KB scan cap it stalled the gate for minutes — an injection-reachable DoS (head before this commit: 6.0 s at N=100, 23.6 s at N=200; ~3.9× per doubling). That over-read is now emitted once, at the first unproven opener of a body, and the scan continues structurally past each real close, so every later opener is reached in the same linear pass contributing only its own short span body. The payload walk no longer re-extracts substitution bodies from the glued-tail over-read — that body is a proper suffix of its source, whose nested subs are exactly those later openers already emitted, so re-extracting them re-explodes the walk for no new coverage; it is still scanned (and still mined for -c/eval payloads), so a push hidden anywhere behind the heredoc is still refused. The work is now linear (0.35 s at N=100, 0.60 s at N=200 — faster than main), with no verdict change (see the sweep below).

Openers are matched byte by byte, so one may sit inside a single quote, where bash reads it as data. The single-quote state at each opener is read from the text AROUND the spans, advancing the quote machine from the previous span's end to the opener with the state that span left — never THROUGH a span, because bash parses each span in a fresh context, so a ' inside one ("$(echo "'")") quotes nothing after its ). A flat walk through the span carried that ' out and marked a later REAL opener single-quoted, hiding its push (echo "$(echo "'")" "$(git p""ush origin main)"). A single-quoted opener is stepped past so the REAL opener after it still gets its own reading — in echo '$(' "$(echo ')' ; bash -c 'git push origin main')" the quoted $('s span would otherwise swallow the real "$( after it. A proven span is skipped PAST (its inner openers are reached when the payload walk recurses into its body); an unproven span emits its glued-tail over-read once and the scan then continues structurally past its real close, so every later opener is reached in one pass. No opener is re-walked from scratch — the per-opener rescan was the super-linear cost. The quote machine stepped that way knows no # comment or heredoc body, where a ' is data too (# don't on an earlier line would leave a later real opener single-quoted). So where the text holds a comment or a <<, a single-quoted reading is not trusted to skip an opener: the scan takes that opener's whole remainder as the body and stops, as an unproven span does.

Self-protection kill floor — the one other consumer that acted on an unproven span. argv_floor_self._bare_kill_raw_bodies re-derives a bare kill's argv window from the raw text, quote-awarely, to catch a decoyed target name the token walk's de-quoted paren counter drops (kill $(printf ')' ; pgrep -f <self>)). The heredoc change above made _matching_close_paren mark a span holding a << unproven, and this raw walk treated unproven as "ends here": it recorded the remainder and set i = n, so a heredoc span earlier in the argv stopped the walk before it reached the later top-level $k $(printf ')' ; pgrep -f <self>) — the decoyed-alias clause only this raw walk catches. bash runs that pgrep (confirmed in real bash with stub kill/pgrep), so k=kill; x=$(cat <<E\nE\n); $k $(printf ')' ; pgrep -f <self>) and its <( and "$( twins were ALLOWED on the prior pushed head 9ad63e8129 while main refuses them (Opus 5.5 BLOCK, bolichen97 CHANGES_REQUESTED). A heredoc span still CLOSES at a ) before its body line, so both the $( and the <(/>( branches now resume the walk past that close (i = off + 2 + rel) instead of ending it, and the later sibling substitution is scanned; only a span that TRULY never balances (rel == len(remainder)) lands i at n and ends the walk, since bash cannot execute past an unterminated substitution either. The whole-remainder over-read that names a target glued directly after the close is still recorded, but emitted once per walk — re-queuing it at every opener of a heredoc run was O(n²) at the 20 KB cap, and the resumed walk reaches the later openers structurally — so a step-count row over the heredoc-decoy kill shape near 20 KB stays ~2.0× per doubling. Per bolichen97's rule, every other _matching_close_paren / _close_paren_walk proven consumer was audited for the same unproven→stop shape (shell_normalizer_span._substitution_bodies, rm_floor, argv_floor_git's process-sub boundary, deny_gate): none regressed — the span extractor already continues structurally past an unproven close, the rm floor resumes in the enclosing text, and the git boundary fails closed to an ambiguity sentinel.

With the span correct, the "$(date)" over-deny and the bypasses above — the quoted nested $(, the ${...} default, the heredoc body line, the $((-seeded heredoc, the quote-aware backtick skip, the quoted <( and $$ read as openers, a ' carried out of an earlier span, a capped span blind to its heredoc, a ' in a comment or heredoc body, a bare (( that is two subshells, a capped-but-closing span whose later openers were never reached, a capped span that never closes hiding a later opener's push, a single-quoted backtick read as a substitution opener, and a heredoc span stopping the bare-kill raw window before its decoyed target name — are fixed by the delimiting corrections, and the extractor stays near-linear on adversarial opener runs.

Intended narrowing: git commit -m "$(date)" && git push origin feat is now allowed (a false refusal removed). Protected targets stay denied in the same shape: main, a bare push, $b, {feat,main} and $'main'.

First-principles cleanup: all three callers of _matching_close_paren pass the slice after the opener at neutral state, so its open_end parameter and the off < open_end skip are removed.

File split: this change would take shell_normalizer_walk.py past the 1,600-line split point in AGENTS.md, so the span matcher (_MAX_SPAN_NESTING, _matching_close_brace, _close_paren_walk, _matching_close_paren, _matching_close_backtick, _backtick_closer) and _substitution_bodies move to a new sibling, shell_normalizer_span.py (794 lines, including the new _capped_tail_entries never-close tail scan). It reads the lexer helpers from shell_normalizer_walk.py, now 1,181 lines, which imports nothing back. shell_normalizer and the kiro_crew.security name map still serve all three names callers use, so every import path holds.

Backwards compatibility

Compatible: every command newly refused versus main embeds a protected push (main/mainline/bare) in its text, and no clean command is newly blocked (sweep under Manual verification); the newly allowed commands are feature pushes after a quoted $(...).

Removes nothing: no rule, tag, export or reader is removed; the only signature change is the private _matching_close_paren losing its unused open_end parameter (all three callers updated). _substitution_bodies keeps its signature but can now return more than one body per opener; four one-body unpacks in test_security.py read the first body (body, *_ = ...), which is the same reading as before.

Tests

All in test/test_push_branch_gate_parser.py (main moved this half of test_push_branch_gate.py there).

TestANestedConstructDoesNotHideAPushBehindItsOwnQuoting:

  • test_the_opus_command_is_denied: the first review-BLOCK command;
  • test_a_push_hidden_behind_any_nested_construct_is_denied: carrier (nested $(, ${...}, backtick, quoted ")"/')') × depth 1–3 × launcher × target, plus a bare git push;
  • test_a_heredoc_body_holding_the_close_paren_is_denied, test_a_feature_push_after_a_quoted_substitution_stays_allowed;
  • test_a_heredoc_left_pending_by_a_nested_substitution_is_denied: the maintainer review's command with $(, $('...'), <( and ${x:-$(...)} carriers, plus a bare push;
  • test_a_heredoc_tag_in_any_quoting_leaves_the_span_unproven: $'E', $'\x45', "E\"F" and \E tags, protected and bare pushes;
  • test_a_body_line_that_differs_only_in_case_does_not_end_the_body.

TestALessThanPairThatIsNotAHeredocKeepsTheSpan (span assertions): a here-string <<< keeps a proven span, and a real heredoc after <<< leaves it unproven. TestAnArithmeticShiftIsAnExpectedRefusal pins the maintainer decision: a multi-line (( 1<<2 )) / $(( 1<<2\n)) shift now leaves the span UNPROVEN (arithmetic not modelled), push text after it is refused, and a shift closed on its own line with no body line before the close still proves the span.

TestAMisreadSpanFailsClosed (the third review round): $[1<<x] hiding a bash -c push, rm -rf "$(cat <<E)" ~ (and the quoted and multi-line forms), and a 1,000-deep ${a:- chain in front of a push and in an rm -rf target. All 3 fail on the head before that round, 86f817f (two allows, one RecursionError), and pass with the fix. The fourth round adds test_a_single_quoted_opener_cannot_swallow_the_real_one and test_a_bare_brace_in_a_default_is_data (rm -rf "$(echo ${x:-{})" ~ plus the _matching_close_brace span), both failing on eb1bdbc, and test_a_heredoc_span_still_reaches_the_mint_and_kill_checks. The maintainer round adds two, both failing on 85155a3 (the prior pushed head) and passing with the fix: test_an_arithmetic_seed_cannot_hide_a_heredoc_push (echo "$((cat <<E\n)\nE\n) ; bash -c '<push main>')", the $((-seeded heredoc) and test_a_quoted_backtick_skip_keeps_the_rm_target (rm -rf "$(echo "`echo '`")" ~, the quote-aware backtick skip dropping the ~). The seventh round adds four, each failing on the prior pushed head 12ea71f and passing with the fix: test_a_quoted_process_sub_in_a_default_is_data (rm -rf "$(echo ${x:-"<("})" ~), test_a_pid_dollar_opens_no_expansion (rm -rf "$(echo $${)" ~, plus a _paired_dollar table), test_a_quote_inside_an_earlier_span_quotes_nothing_after_it (echo "$(echo "'")" "$(git p""ush origin main)") and test_a_capped_span_still_scans_past_its_heredoc (a 32-deep $(echo prefix then a heredoc and a bash -c push). The eighth round adds test_an_arithmetic_seed_that_closes_a_subshell_first_is_dropped (echo "$((a);(cat <<E\n))\nE\nbash -c '<push main>'))"), failing on 629d0e4 and passing with the fix; the $$ table now checks the same pairing through _matching_close_paren. The ninth round adds test_a_quote_in_a_comment_or_heredoc_cannot_hide_an_opener: a ' in a # comment line, in a comment after a command, in a heredoc body and after a here-string, each in front of echo "$(bash -c '<push main>')" # '. All four fail on 3ffd044 and pass with the fix. The tenth round adds test_a_bare_double_paren_without_an_adjacent_close_is_two_subshells: four spellings of ((cat <<E\n)\nE\n) ) in front of a bash -c push, failing on cc4a08e and passing with the fix, plus ((1<<2)) keeping its shift and its proven close. The eleventh round adds two, each failing on 677cbca and passing with the fix: test_a_double_quoted_process_sub_opener_is_data (echo "<(echo '" "$(bash -c '<push main>')" "')": bash prints the "<(echo '" text and runs the push, so a <(/>( opens a span only unquoted, and a quoted one is stepped past like a single-quoted opener) and test_an_rm_target_after_a_capped_span_fails_closed (an 18-deep "$(echo ${x:- chain in rm -rf "$(echo …)" ~: past the nesting cap the span's close is a guess, so the rm floor reads that span's output as a home operand; ${...} now reports the cap too). The twelfth round (this commit) adds two to TestAMisreadSpanFailsClosed, each ALLOWED on the prior pushed head d0b2c5c and denied with the fix: test_a_capped_span_remainder_is_re_extracted_for_a_nested_push (a 33-deep $(echo span then echo "$(echo "$(bash -c '<push main>')")" — the real opener after the capped-but-balanced span was never scanned because the capped branch stopped there) and test_a_single_quoted_backtick_is_literal_not_an_opener (echo '' "$(bash -c '')" '' plus its nested, ANSI-C $''and process-sub variants — a backtick in single-quote state is literal, not a substitution opener, so its bogus span no longer carries the'over the real$(after it). This revision addstest_a_capped_span_that_never_closes_cannot_hide_a_later_pushto the same class: the capped${x:-(} miscount prefix × three separators × three launchers (bash -c/sh -c/eval) × two protected targets (main, mainline) × four hidden carriers ($(...)/backtick substitution, each bare and inside "…"). Every row is ALLOWED on the prior pushed head a722… and denied with the fix, and each runs a protected git pushin real bash (stub git). The same test pins that a direct$(git push origin main)behind the span stays denied (raw floor), that a benign non-push carrier behind the span is NOT refused, and that a feature push behind it stays allowed. This revision addstest_a_quoted_or_escaped_paren_cannot_end_the_capped_tail_earlyto the same class: the capped$(…)×33prefix × two decoys (a double-quoted)dataecho ")"and an escaped)) × two openers (echo "$(, x="$() × three launchers (bash -c/sh -c/eval) × two protected targets (main, mainline). Every row is ALLOWED on the prior pushed head 588749dand denied with the fix, and each runs a protectedgit pushin real bash (stub git). It also pins that a feature push and a benign tail behind the same decoy stay allowed, and that a genuine double-quoted)closing an inner double-quoted$(` is still a boundary, so a push after that real close stays refused.

This revision adds, to TestAMisreadSpanFailsClosed, test_a_capped_span_closing_early_cannot_hide_its_launcher (a ${x:-)} / ${x:-))} / `echo )` data close × three nest openers × three outer openers × three separators × two protected targets × three launchers — each ALLOWED on 8939c641f2 and denied here; most push in real bash with a stub git, the rest over-refused) and test_a_capped_span_as_an_assignment_value_cannot_mask_a_home_rm (the x="$(<capped>)" rm -rf $HOME / ${HOME} / ~ family across a deep $(echo nest and an 18-deep ${x:- brace nest × several rm spellings and home spellings — each ALLOWED on 8939c641f2, refused here and on main; the capped-span-as-operand rm -rf "$(<capped>)" ~ still fails closed, and a capped assignment value with no rm after it stays allowed). TestAnArithmeticShiftIsAnExpectedRefusal carries the arithmetic expected-refusals.

This revision also adds TestSelfKillHeredocSpanDoesNotStopTheRawWalk to test/test_denied_commands_security_kill.py for the self-kill raw-walk regression: test_heredoc_decoy_kill_is_still_a_self_kill (7 rows — the three issue spellings k=kill; x=$(cat <<E…); $k $(printf ')' ; pgrep -f <self>), the <( and "$( twins, a backtick-alias decoy, a literal-kill heredoc decoy, a heredoc run, and a heredoc whose close has a ) glued after it so the target rides the whole-remainder over-read) and test_heredoc_decoy_twins_stay_allowed (4 rows — the same shapes with a non-protected nginx target, and a heredoc sibling owned by a different command, all still allowed). The four alias-decoy rows FAIL on the prior pushed head 9ad63e8129 (ALLOWED) and pass with the fix; main refuses all 11.

test/test_security_gate_liveness.py gains eleven structural rows (GPT 6.1 asked that the timed rows not depend on scheduler timing). Each counts the characters the shell quote machine steps through inside is_denied (in both shell_normalizer_walk and shell_normalizer_span, which read it) — the walk's actual work, which a loaded CI worker cannot move — and asserts doubling the opener run grows that work at most 4.5× (both this build and main are quadratic, ~3–4× per doubling; each regression the review found grew 6×+). The shapes: balanced $( openers, unbalanced "$( openers, alternating single-/double-quoted openers (the quote-state case), a deeply nested $(echo …))) run, the three $(( shapes from the fifth review round ($((×N, $((x+×N 1 ))×N, $((echo ×N) and the two from the sixth ($((x<<×N, $((cat <<E ×N). The two sixth-round shapes grow ~6× per doubling on 82c1c47 (the pre-fix seed) and within the bound here. The tenth row, test_a_run_of_sequential_heredoc_spans_is_linear (echo "$(cat <<E\n) "×N), pins the O(n²) remainder-chain fixed here with a tighter 2.3× bound that holds only because the work is now linear (~2× per doubling): it FAILS on the head before this commit (2.5× growth) and passes with the fix. The eleventh row, test_a_run_of_capped_substitution_spans_is_linear (echo + N copies of a depth-33 $(echo … x ) nest joined by ;), pins the capped-span run fixed on this revision under the same 2.3× bound: the dropped whole-remainder slice and per-opener find/rfind made it ~2.69× per doubling on the head before this commit (a722…, ~17 s at the 20 KB cap), and it is now ~2.0× (linear, ~1 s).

The existing TestSubstitutionInsideDoubleQuotesIsExtractedFromAFreshContext and the _matching_close_paren unit assertions use the sliced signature, and the exact-body lists in it and in TestNestedPayloadExtractionSpansTheProvenBoundary now include the inner opener's body and the last-) reading.

Rebased on main @ a247d03 (no conflict; none of main's new commits touch security; every is_denied verdict is byte-identical to the pre-rebase head fe3b23e079, 0 flips across 5,104 push-gate and 2,268 nested commands). The comment above the suffix-sub check in _shell_payload_walk is shortened so shell_normalizer.py stays at the 1,600-line split point in AGENTS.md (1,600; main has 1,590). _PACKAGE_LINE_BUDGET goes from 32,056 to 32,688, the measured cat src/kiro_crew/security/*.py | wc -l, with a reason comment after main's. This revision folds in the capped-span never-close fix and the capped-run liveness fix above (_capped_tail_entries, plus dropping the per-opener whole-remainder slice and gating the heredoc glued-tail scan on a newline inside the span); is_denied keeps every existing verdict — 0 flips vs the prior head a722… across both the 5,104 and 2,268 sweeps — and the never-close family adds only intended protected-push denials (45 shapes, each a push real bash runs). The late-close process-sub fix changes 0 verdicts over the same two sweeps vs d42c8f4; over an 11,178-command corpus (those sweeps, the never-close family and the late-close family) it flips 1,152 rows, all allowed→denied, all refused by main, each a protected push in real bash (960 directly; the 192 || rows once the left side fails), and 0 denied→allowed. Nothing it allows pushes a protected branch (stub git), and it refuses all 357 protected pushes main allows there. A capped run at the 20 KB cap stays linear: 2.00× steps per doubling, 1.1 s (main: 3.81×, 4.2 s), and a run of late-closing spans 2.04×, 1.1 s (main: 3.59×, 25.4 s). The two outer-body / proven-suffix fixes change 0 verdicts over the 5,104 and 2,268 sweeps vs 67038bf. Over a 15,136-command corpus (the 11,178 above plus the two new families) they flip 1,508 rows, all allowed→denied and 0 denied→allowed, with 0 feature-push and 0 push-less flips; each is a protected push in real bash (stub git): 1,380 directly, and the 128 && rows whose left side fails as written ($(echo x) runs x; <(…) runs /dev/fd/N) once that side succeeds. Nothing head allows there pushes a protected branch, and it refuses all 646 protected pushes main allows. Every liveness shape stays linear at the 20 KB cap (steps N=30 → 60, wall time vs main): capped run 2.00×, 1.1 s (3.81×, 4.2 s); late-close run 2.04×, 1.1 s (3.59×, 25.3 s); sequential heredocs 1.98×, 4.4 s (3.84×, 63.3 s); process-sub run 1.98×, 0.3 s (3.80×, 9.0 s); outer-body capped run 2.00×, 1.0 s (3.72×, 8.4 s); proven-suffix run 2.00×, 1.0 s (3.82×, 165.8 s). This revision also folds in the capped-tail quoted/escaped-) fix above (_capped_tail_entries now tracks the quote state at each opener, so a ) ends the span only when unquoted-and-unescaped or a double-quoted ) closing a double-quoted opener — a data ")" or an escaped \) stays inside the run): is_denied keeps every verdict over the 5,104 push-gate and 2,268 nested sweeps vs 588749d9e2 (0 flips). Over a 20,328-command corpus (the 15,136 above plus the new quoted/escaped-) family: 6 outer wrappers × 4 nests × 4 decoys × 5 launchers × 5 targets × 2 push spellings, plus never-close variants) it flips 2,368 rows, all allowed→denied and 0 denied→allowed, with 0 feature-push and 0 push-less flips; each is a protected push in real bash (stub git) — 2,368/2,368 push directly, no short-circuit rows this time. Nothing head allows pushes a protected branch (0 of 3,949 head-allowed), and it refuses all 2,021 protected pushes main allows there. A capped tail whose span holds a double-quoted ) decoy stays linear at the 20 KB cap: 2.00× steps per doubling, 1.0 s (main: 3.85×, 6.9 s), and all prior liveness shapes keep their head numbers unchanged. main @ 574cb18 had split argv_floor.py into argv_floor_self.py / argv_floor_hosts.py / argv_floor_ssh.py / argv_floor_git.py behind a facade (#21952), so the kill-floor change lands in argv_floor_self.py (the <(/>( call gets can_arith=False, the $( call drops the removed open_end). rm_floor.py's one call becomes _matching_close_paren(body): it already passed the slice after $(, so the span is the same.

This revision folds in the self-kill raw-walk heredoc fix above (argv_floor_self._bare_kill_raw_bodies resumes past an unproven-but-closed span in both the $( and <(/>( branches, emitting the whole-remainder over-read once). It is now rebased on main @ d0622d0; the only conflict was _PACKAGE_LINE_BUDGET, and main's two new security commits (#21867, #22851) touch only _child_realpath.py and the paths_* modules, none of the files this change edits. is_denied is byte-identical to the pre-rebase head df55d19676 (0 flips over the push probe and 2,268 nested sweeps, identical verdicts over the 546-command self-kill corpus). is_denied keeps every verdict over the self-kill differential corpus — head is byte-identical to main (0 flips either direction) over 288 kill/pkill/killall/$k-alias × heredoc-vs-plain span × $(/<(/"$( × self-vs-nginx target commands — and over the push probe (5,104), nested-heredoc (2,268) and c97 (20,328) sweeps (0 flips vs the prior head 9ad63e8129, since only argv_floor_self.py's self-kill walk changed). The eight self-kill flips vs 9ad63e8129 are all allowed→denied, every one a kirocrew self-kill main also refuses. _PACKAGE_LINE_BUDGET goes from 32,690 to 32,710 (the measured cat src/kiro_crew/security/*.py | wc -l), with a reason comment after the prior one. Real bash with stub kill/pgrep confirms the four heredoc-decoy shapes reach the pgrep clause, so each is a live self-kill rather than dead syntax.

Run with pytest -n 4: test_push_branch_gate.py, test_push_branch_gate_parser.py, test_security_gate_liveness.py, test_security_split_patch_reach.py, test_denied_commands_security.py, test_denied_commands_authority.py: all pass. The paren / substitution / heredoc / backtick / rm subset of test_security.py (-k, every class that calls _substitution_bodies, _is_self_kill or a span matcher): 157 passed; the full file OOMs on this host. black / isort / flake8 / mypy clean on touched files; check_comment_history.py and check_file_line_cap.py pass.

Manual verification

Every sweep compares is_denied on origin/main vs this branch and runs each allowed command in real bash with a fake git on PATH that records its args.

Against main @ 0b89e0c (a fresh origin/main worktree):

Sweep Commands Branch allows Protected pushes among them main lets through Branch refuses
here-string / arithmetic / heredoc carriers 5,104 998 0 297 all 297
heredoc left pending by a nested construct, every tag spelling 2,268 310 0 80 all 80
  • No command main refuses and this branch allows runs a protected push (all newly-allowed are feature pushes after a quoted $(...)).
  • The heredoc commit idiom (git commit -m "$(cat <<'EOF' … EOF\n)" && git push origin feat/example) and the gh pr create --base main --body "$(cat <<'EOF' …)" idiom are refused by main and by this branch alike, so neither is newly refused.
  • The bare-(( rule refuses 96 more commands than the prior head cc4a08e; all 96 hold (( and git push origin main text, main refuses 84 of them, and none runs a protected push in real bash (each is a bash syntax error).
  • Every command newly refused vs main carries git push text, almost all inside heredoc data or a nested quoted $(; no clean command is newly blocked.
  • A 17,232-command rm -rf "$(<inner>)" ~ / rm -rf $(<inner>) ~ sweep over two- and three-piece inner bodies built from every quote, dollar, brace, paren, heredoc and process-sub fragment: the branch allows 15,036 and main 13,909; of the 1,136 the branch allows and main refuses, only 2 parse under bash -n (rm -rf "$(echo $((${)))" ~ and the ${x:- form), and both are ${-bad substitution errors bash aborts the command on, so rm never runs. Both were already allowed on the prior head 12ea71f, so neither is a regression.

Timing — is_denied on the adversarial opener shapes, this branch vs main @ 574cb18 (every probe wrapped in timeout 300):

Shape main N=100 branch N=100 main N=200 branch N=200
echo + $(×N 0.62 s 0.63 s 3.36 s 3.40 s
echo " + "$(×N 0.65 s 0.69 s 3.53 s 3.61 s
echo + '$(' "$(×N 2.10 s 1.30 s 8.28 s 4.92 s
echo + $(echo ×N + )×N 1.43 s 1.53 s 5.63 s 5.96 s
echo + $((×N 0.81 s 0.85 s 4.08 s 4.16 s
echo + $((x+×N + 1 + ))×N 0.75 s 0.86 s 2.96 s 3.44 s
echo + $((echo ×N 1.43 s 1.54 s 5.63 s 5.88 s
echo + $((x<<×N 0.66 s 0.76 s 2.42 s 2.67 s
echo + $((cat <<E ×N 2.22 s 2.39 s 8.73 s 9.10 s
echo " + "$(cat <<E\n) "×N 0.45 s 0.35 s 1.41 s 0.60 s

Every shape is within 1.17× of main (main @ 38ea505 for the first seven rows, and the base 6257f69 for the last two, run one after the other on the same host); the alternating-decoy shape is faster than main. The $(( shapes took up to 62.5 s at N=200 on cc1b7c7 because of a per-opener arithmetic pre-check; that check now happens inside the walk. The last two shapes took up to 90.8 s at N=200 on 82c1c47, because a failed seed re-read the span and so re-walked every nested span under it; the seed-off reading now rides along in the same pass. The sequential-heredoc row (last) is the O(n²) remainder-chain this commit fixes: 23.6 s at N=200 on the head before it (727…→725…), now 0.60 s — flat and faster than main, which is itself super-linear on this shape. _substitution_bodies also builds its single-quote map only when the text holds a ', since without one no offset can be single-quoted. The bare-(( rule costs 1–5% over cc4a08e on these shapes: at N=200 against main @ bdb1c87, $((x<<×N 2.86 s vs 2.47 s, $((cat <<E ×N 9.30 s vs 8.68 s, $(: ; ((x<<×N 9.25 s vs 8.60 s, and $(((x)<<×N 3.98 s vs 3.25 s (1.22×; 1.17× on cc4a08e).

Related Issues

Refs #20845. Follow-up of #15438; applies on top of current main.

Pattern harvest

Rule candidate: review-prompt
Pattern: a span matcher that walks a nested construct ($(, ${) with the enclosing word's quote state instead of skipping it as a unit, so an inner " or ) ends the outer span early; and a matcher that parses a construct it only needs to avoid (a heredoc) instead of failing closed on it.

Checklist

  • At most two commits (one here), Conventional Commits title
  • Existing tests pass and new tests added
  • Self-review completed; code follows project style
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

Contributed under the project's Apache-2.0 license.

@Maqix
Maqix requested review from a team and KiroCrewCaptain as code owners October 15, 2026 14:39
@Maqix
Maqix requested a review from bolichen97 October 15, 2026 14:39
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

UX Review (Opus 5.5, fork) — ⏭️ skipped

Revision 536d7e80d9057c97c5243f229765fb5da5347355 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Does not block merge.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of 536d7e80d9057c97c5243f229765fb5da5347355 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks: the bypasses are real, the parsing input really is a shell command line, and every caller of the changed _matching_close_paren signature is updated in the patch.

Notes

  • The rm floor now models heredocs, arithmetic shifts and the position of assignment prefixes, yet the security.md paragraph still calls it "a small seatbelt, not a parser" and lists "heredocs, … arithmetic" as out of scope by design. Fix that paragraph in this PR, or move these readings into the shared span layer, so the rm floor doesn't become a second parser.
  • This revision closes ten separate span readings, and each new reading adds another one-off flag (over_read, past_close, quote_blind, heredocs=False, arith_hits). The next finding of this kind should narrow the change (e.g. refuse any << after an arithmetic opener inside $() rather than add an eleventh reading.
  • The ## Backwards compatibility section falls after the description's truncation point, so I couldn't check it. I found no outside reader of the moved or re-signed names that the patch doesn't also update.

[DESIGN-REVIEWED] 536d7e8

Description read

[DESCRIPTION-READ] 4182cf1c5314b89612cce267d826e8f0bbadb51e3e1f9879d0af998162df4c4a

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5, fork) — ✅ PASS

Premise-level review of 536d7e80d9057c97c5243f229765fb5da5347355 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

First-Principles-Verdict: PASS

Nothing blocks, but the arithmetic << fallout is handled in each consumer, not in the span walker that causes it.

Notes

  • Item 5, 8 — symptom-level: _close_paren_walk already detects arithmetic (arith = True) yet still opens a heredoc on its <<. Three places then work around it: rm_floor (hits/shifted retry), argv_floor_self (a second walk with heredocs=False) and the span module's flat and quote-blind passes on spans that never close. _ARITH_CONTEXT_RE is a second spelling of that same arith test. Subtraction: delete _ARITH_CONTEXT_RE/_arith_shift_possible and read the walker's own arith_hits everywhere.
  • Item 6 — oversized: _in_assignment_value, a 60-line quote-aware word parser, exists only for spans nested past _MAX_SPAN_NESTING = 32. Subtraction: raise _UnreadSpan for a capped span too, and delete _in_assignment_value, _ASSIGNMENT_WORD_RE and the synthetic '\0" ~ "' operand.

What this change ships

Inventory (10 items) — 6 justified

Intent: let ordinary "$(…)" commit-then-push lines through while still refusing pushes hidden inside or after substitution bodies. This is a FIX.

  1. git commit -m "$(date)" && git push origin feat no longer refused — justified
  2. A nested $(, a ${…} default or a heredoc ) no longer hides a push — justified
  3. Openers after heredoc-unproven or never-closing spans are now scanned — symptom-level, arithmetic heredoc misread
  4. rm floor refuses a quoted $( span it cannot close — justified
  5. rm floor also reads with no heredocs after an arithmetic << — symptom-level
  6. A capped rm span adds a synthetic home operand, except in an assignment value — oversized
  7. <<$'E' delimiters and a glued EOF) terminator are now recognised — justified
  8. Kill walk resumes past unproven spans and adds a second walk with no heredocs — symptom-level
  9. Span reader moved to shell_normalizer_span.py and re-exported — justified
  10. Payload walk skips re-extracting suppressed over-reads, so cost stays linear — justified

[FIRST-PRINCIPLES-REVIEWED] 536d7e8

Description read

[DESCRIPTION-READ] 30c2774bbb24ba74d16bc21518f597b0c1c2bc118e2d51adc351f8426f285907

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review (fork) — 🔴 changes requested (blocking)

Reviewed 536d7e80d9057c97c5243f229765fb5da5347355 via the fork AI-review pipeline; updated in place on each push.

Blocking: two changed paths in the rm floor now allow a recursive-force home wipe (rm -fr ~) that the base code denied. No deny row matches rm -fr ~ (the home row's pattern is rm -rf ~.*), so the floor is the only control that refuses it.

BLOCKING — src/kiro_crew/security/rm_floor.py:431
seg.append('\0" ~ "' if fail_closed and not assignment_value else "\0")
Trigger: a capped (33-deep) double-quoted $(…) used as a leading redirection target, e.g. >"$(echo $(echo …33… x)…)" rm -fr ~ or > "$(…)" rm -fr ~. _segments is called with seg = ' >"'. _in_assignment_value returns False because the open word >" is not an assignment, so the fake ~ is inserted. The segment becomes >"\0" ~ "" rm -fr ~. _rm_wipe_targets steps over the >\0 redirection, takes ~ as the program, and skips the segment.
Outcome: bash applies the redirection and then runs rm -fr ~. The floor allows it, where base produced >\0 rm -fr ~ and denied.
Anchor: residual/security
Fix: only insert the fake home operand once a command word has finished in the segment. When the open word is a redirection or its target, or only assignments and redirections came before it, append plain \0.

BLOCKING — src/kiro_crew/security/shell_normalizer_span.py:1190
if c in " \t\n;&|<>()"'":/breakTrigger:rm -fr "$(cat <<$(echo E)⏎$(echo E)⏎)" ~ ; echo $(⏎$⏎) ; true. _heredoc_delimiterstops at(and returns the delimiter$. bash never expands a heredoc word, so its real delimiter is the literal $(echo E)and bash ends the body at line 2._close_paren_walkinstead skips)" ~ ; echo $(as heredoc data and closes at the last). That close is before the end of the text and no arithmetic hits are recorded, so _segmentsdoes not raise_UnreadSpan. It resumes past the late close, and the ~operand is never segmented. Outcome: bash runsrm -fr "" and wipes home. The rm floor allows it, while base's paren-counting close read theand denied. Anchor: residual/security Fix: in_heredoc_delimiter, return None(soheredoc_badmakes the span unread and the floor refuses it) when the delimiter word contains$(, ${` or a backtick. Or read that construct as one unit and keep its literal text as the delimiter.

[BLOCK-MERGE] 536d7e8
[OPUS-REVIEWED] 536d7e8

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review (fork) — 🔴 changes requested (blocking)

Reviewed 57363ec42c8769e770ba62e518649deeed20065a via the fork AI-review pipeline; updated in place on each push.

1 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/security/rm_floor.py:224 -- Truncated recovery drops a home-directory wipe

tail = _segments(body[agn_close : agn_close + _AGNOSTIC_SCAN_CAP], 2)
Trigger: PreToolUse → is_denied → _segments reads rm -r -f "$(echo $((1<<X))\n)", followed by 300 spaces and ~ ; : '\nX\n)' (\n denotes actual newlines).
Outcome: The recovery window ends before ~, while the primary reading swallows it as heredoc data. The gate returns None despite the enabled home-wipe rule; bash passes the home directory to recursive-force rm. The base refuses this command.
Anchor: residual/security
Fix: Detect incomplete bounded recovery and propagate the existing unread-command refusal, including when the earlier-close search exhausts its window.

BLOCKING -- src/kiro_crew/security/shell_normalizer_span.py:641 -- Valid heredoc delimiters refuse harmless cleanup

if not started or not word:
Trigger: PreToolUse → is_denied → _segments → _heredoc_delimiter reads rm -rf "$(cat <<''\n/tmp/x\n\n)" /tmp/y. The continued delimiter in rm -rf "$(cat <<E\\\nOF\n/tmp/x\nEOF\n)" /tmp/y also fails (\n denotes actual newlines).
Outcome: The empty quoted delimiter is rejected, and the continued delimiter retains its newline. Both commands raise _UnreadSpan and are refused, although bash supplies only /tmp/x and /tmp/y as operands and the base allows both.
Anchor: defect
Fix: Accept an explicitly started empty delimiter and remove unquoted backslash-newline pairs during delimiter quote removal.

[BLOCK-MERGE] 57363ec
[GPT-REVIEWED] 57363ec

Adjudication (Opus 5.5) — is blocking on each finding proportionate?

F2: LOW harm. The command is refused, never allowed. The trigger needs one of two delimiter spellings that ordinary commands almost never use: an empty quoted delimiter <<'', or a backslash-newline inside the delimiter word. The first returns None at authentic.patch:1172. The second keeps its newline at authentic.patch:1160-1165. Either way heredoc_bad is set (authentic.patch:1358-1360), the span reads to the end of the text, and _UnreadSpan is raised (authentic.patch:378-379).
The user sees the refusal: deny_gate.py:776-783 returns _RM_FLOOR_UNREAD_NOTE ("could not read this command. Simplify it."), and rewriting the delimiter fixes it.
The fix costs more than the harm. An empty delimiter changes the glued-) match at authentic.patch:1205: startswith("" + ")") would match any line that starts with ). That is new terminator behaviour in a security parser, added for a refusal that is safe, and AGENTS.md calls this kind of spelling-chase a review smell.

[ADJUDICATION] 57363ec42c8769e770ba62e518649deeed20065a total=1 uphold=0 downgrade=1
DOWNGRADE F2 src/kiro_crew/security/shell_normalizer_span.py:641 reason=disproportionate-remedy
[GPT-ADJUDICATED] 57363ec42c8769e770ba62e518649deeed20065a

[ADJUDICATION-FENCED] 57363ec42c8769e770ba62e518649deeed20065a fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/security/rm_floor.py:224 -- The fixed 256-char window (`_AGNOSTIC_SCAN_CAP`, authentic.patch:255, 402-407) is passed by ordinary whitespace padding that a prompt-injected agent can easily produce, so the missed home-wipe refusal is plausible in real operation and is not an extreme case.
[GPT-ADJUDICATED-FENCED] 57363ec42c8769e770ba62e518649deeed20065a

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Security Scope Review (fork) — ✅ PASS

Which legitimate operations does abda6f78af356611ec40a90b54ffeb464bee17b1 newly refuse? Model proposes, scripts/deny_diff.py decides — updated in place on each push. A script-confirmed regression blocks PR readiness.

adjudicated, zero confirmed regressions

Scope-Verdict: PASS

Reason: The tightening is the shared span reader in shell text, and the deny composite adjudicates all of it. That covers the self-kill raw walk, the rm floor's new whole-command refusal, and the payload walk's new over-reads. No script-confirmed regression came back for me to report. The Windows/PowerShell spelling of $(...) and backtick escapes is probed but not confirmed.

TIGHTENING: the shell span reader (shell_normalizer_span._close_paren_walk), the payload-body extraction built on it, and its rm-floor and self-kill callers. They now read heredoc bodies as data, take a second heredoc-free reading after an arithmetic opener, and over-read unproven spans. -- The ONE threat: a command hidden behind a heredoc or arithmetic << inside a substitution, which used to slip past the floors. In the rm floor that becomes a refusal of the whole command: "Any other mismatch ("$(echo $((1<<X))⏎)" ~ ; : '⏎X⏎)' hides the ~) refuses." / raise _UnreadSpan(command). In the self-kill walk the same threat is the decoy $k $(printf ')' ; pgrep -f kirocrew), which the raw walk now reaches because it resumes past a span instead of ending at it: if not proven and rel >= n - (off + 2): rel = _heredoc_agnostic_close(...).

ADJUDICATED: 19 candidates, 0 confirmed newly-refused

PLATFORM COVERAGE: posix: 16 rows. They cover heredoc commit messages and PR bodies (the agents' daily commit/PR flow), heredoc prose that mentions rm, main and git push, nested $(date) on a heredoc body line, double-quoted $(...) rm operands, assignment-value $(, $((1<<n)) shifts, array subscripts before a later <<, a bare kill of a pidfile or a non-protected process, bash -c payloads, and a comment with an apostrophe. Together they cover the posix bash grammar this diff models. windows: 3 rows, because the diff assumes bash grammar for $(...) and backticks, and PowerShell uses both differently. The rows are a PowerShell $(Get-Date ...) subexpression before a feature push, a commit message using the backtick-n escape (which the posix reader sees as unterminated backticks), and the PowerShell rm -r -foalias with a$(Get-Location)\buildoperand. I wrote noany rows: every probe depends on shell-specific quoting. PowerShell here-strings (@'...'@) and cmd.exe %VAR%spellings have no rows; the diff makes no assumption about them beyond the shared$(`/backtick reading.

[SCOPE-REVIEWED] abda6f7

Security scope review -- adjudicated candidates

  • macos-latest (platform posix): 16 classified, 0 newly refused, 10 still allowed.
  • ubuntu-latest (platform posix): 16 classified, 0 newly refused, 10 still allowed.
  • windows-latest (platform windows): 3 classified, 0 newly refused, 2 still allowed.

No candidate flipped from allowed to refused.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Oct 15, 2026
@Maqix
Maqix force-pushed the fix/heredoc-substitution-extract branch from 183fd1d to 2e9c4b5 Compare October 15, 2026 17:35
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Oct 15, 2026

@cruisercohen cruisercohen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 2e9c4b5 against main e492fda.

What checks out. The nested $(, ${...} and heredoc-body skips close the span where bash does. The new class TestANestedConstructDoesNotHideAPushBehindItsOwnQuoting passes on this head, and 2 of its 4 methods fail with main's shell_normalizer.py (the nested-construct denies and the feature-push allow). test_push_branch_gate.py and test_security_gate_liveness.py pass on the head (145 passed). Across 15 heredoc and substitution commands plus 24 cross-floor commands, I found nothing that main refuses and this head allows. I compared each verdict with what bash actually runs, using a recording git stub.

Requested change: two << forms that are not heredocs are read as heredoc operators. _matching_close_paren (shell_normalizer.py:2604-2607) calls _heredoc_redirect on every active <. That gives two wrong spans, and both are still reported as proven:

  1. Here-string. _heredoc_redirect returns None at the first < of <<< (line 2408), but the walk then reaches the second <, where <<x matches. The span then runs to a later line that reads x.
  2. Arithmetic shift. In (( 1<<2 )), the <<2 is a shift, but it is noted as a heredoc tagged 2.
from kiro_crew.security.shell_normalizer import _matching_close_paren

def test_a_here_string_is_not_a_heredoc():
    # bash closes at the first ")" (index 10)
    assert _matching_close_paren("cat <<<x\n) ; P\nx\n)") == (10, True)   # head: (18, True)

def test_an_arithmetic_shift_is_not_a_heredoc():
    # bash closes at the first ")" (index 13)
    assert _matching_close_paren(" (( 1<<2 ))\n) ; P\n2\n)") == (13, True)  # head: (21, True)

Main returns the expected value for both, because it has no heredoc reading. is_denied still refuses the full commands I built around these spans, because the per-line push reading catches them. But this function is the shared span computation for the extractor and the boundary walk, so a proven span past the real ) should not ship. Possible fixes: step over the whole <<< run, for example by skipping a < whose previous character is <. And read ((…)) as one unit with no redirects, the way ${...} is.

Scope note (not blocking). The heredoc commit form from #20845 is still refused on this head with git-publish-target-unverifiable, the same as on main. I tried 6 variants: &&, ;, unquoted, <<- and "EOF" tags. The command is git commit -m "$(cat <<'EOF'\nsubject\n\nbody\nEOF\n)" && git push origin feat. It is also still refused with #15438 merged on top. So this PR does not resolve #20845, which matches the body's "Refs".

Nits.

  • Two added comments describe the old behaviour rather than the current one: "Without this the walk read the inner " … went unscanned" (around line 2558), and ":func:_matching_close_paren counted it: … went unscanned" (line 2399). The comment-history gate passes, but present tense would read better.
  • The branch conflicts with main in test/test_security_gate_liveness.py (the _PACKAGE_LINE_BUDGET pin), and CI has not run on this head.

@Maqix
Maqix force-pushed the fix/heredoc-substitution-extract branch from 2e9c4b5 to c772cac Compare October 15, 2026 20:36
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Oct 15, 2026

@cruisercohen cruisercohen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review on c772cac9a9 (rebased onto main c6d610673b). Compared with 2e9c4b52f1, the source change is in _matching_close_paren only (shell_normalizer.py), plus the new test class and the rebased _PACKAGE_LINE_BUDGET pin. Main did not touch these files between the two bases.

My requested change is met.

  • Here-string: a <<< run is now one operator (here_string_end), so its later < characters no longer open a heredoc. _matching_close_paren("cat <<<x\n) ; P\nx\n)") returns (10, True), where bash closes. 2e9c4b52f1 returned (18, True).
  • Arithmetic: inside ((…)), and for a $(( body, no heredoc is read. " (( 1<<2 ))\n) ; P\n2\n)" returns (13, True). 2e9c4b52f1 returned (21, True).
  • TestALessThanPairThatIsNotAHeredocKeepsTheSpan pins both cases, plus the $(( expansion and a heredoc read after the arithmetic closes or after a here-string on the same line. Each half is load-bearing. Dropping the <<< skip turns 2 of the 4 tests red. Dropping the arithmetic guard turns 3 of the 4 red.

No new allows. I ran 29 commands through is_denied on this head, on 2e9c4b52f1 and on main c6d610673b. They cover here-strings, arithmetic, $(( subshell fallbacks, heredocs hiding a protected push, the issue's heredoc commit forms, and pkill/force-push cross-floor cases. Nothing main refuses runs a protected push on this head. Four commands move from refused to allowed:

  • three feature pushes: echo "$(cat <<<x)" && git push origin feat, echo "$(( 1<<2 ))" && git push origin feat, and git commit -m "$(printf 'a)b')" && git push origin feat;
  • one echo "$(cat <<'E'\n)\nE\n) ; bash -c '<push main>'", where the push text sits inside the double-quoted echo argument after the substitution closes. A recording git stub confirms that bash never runs it.

Nits from last time: both fixed. The two comments are now in present tense. The branch no longer conflicts.

Scope (unchanged, not blocking). The issue's own git commit -m "$(cat <<'EOF' … EOF\n)" && git push origin feat form is still refused here, the same as on main, in 5 variants. That matches the body's "Refs #20845".

Local checks: test_push_branch_gate.py, test_security_gate_liveness.py and test_security.py give 1604 passed and 1 skipped. flake8, black, isort and the comment-history gate are clean on the changed files.

Approval: held. This PR changes the shell command security floor, so approval is the repository owner's call. Separately, CI has not run on c772cac9a9: PR Readiness reports 5 workflows awaiting maintainer approval.

@Maqix

Maqix commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the quick re-review, and for checking each half is load-bearing.

For the record, the fix in c772cac:

  • <<< is read as one operator, so its later < is no longer checked for a heredoc.
  • ((…)) and $((…)) are tracked as arithmetic, with no heredoc reading while open.
  • Both comments are in present tense.

Beyond your 29 commands, I ran a 5,104-command sweep (heredoc, here-string and arithmetic carriers, protected and feature targets) in real bash with a recording git stub. None of the 1,325 commands the head allows pushes a protected branch.

Agreed on scope: #20845's heredoc commit form is still refused, so this stays Refs.

Understood that approval is the owner's call. The remaining blocker is the 5 workflows awaiting maintainer approval, so CI has not run on this head yet.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Oct 15, 2026
@Maqix
Maqix force-pushed the fix/heredoc-substitution-extract branch from c772cac to 8ff800a Compare October 16, 2026 00:35
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Oct 16, 2026
@Maqix
Maqix force-pushed the fix/heredoc-substitution-extract branch from 8ff800a to 945e463 Compare October 16, 2026 02:42
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Oct 16, 2026
@Maqix
Maqix force-pushed the fix/heredoc-substitution-extract branch from 945e463 to 75ec9cc Compare October 16, 2026 16:50
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 16, 2026
@Maqix

Maqix commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@bolichen97 thanks for the re-review on 3032ab76a0. Both inline notes are addressed on f5bf048215 (one commit, rebased onto origin/main b2b87b0f1a):

  • Quote on a heredoc body line (🔴, span.py:653): an unpaired ', " or ANSI-C $' on a body line is now read as data, so it can no longer carry quote state past the span's real ). A quote that closes on its own body line (e.g. ")") keeps main's paired reading, so the ) inside it can't end the span early either.
  • Quadratic nested heredoc units (🟡, span.py:579): an over-read span is now scanned once as a whole and no longer re-extracted, so the walk is linear. A new liveness test doubles the unit count and asserts near-linear growth.
  • Reviewer name in a code comment: replaced with a description of the shape.

Checked against main b2b87b0f1a and the prior head:

Check Result
Heredoc-body quote family, 288 rows (', ", $' × 3 tag forms × direct / nested / post-terminator × home / root / /tmp) 0 home/root rows that main refuses get through; /tmp twins match main. It also refuses 90 real wipes that main allows
Earlier heredoc and rm probes (120 + 24 + 58 rows) match main on every row
rm corpus, 4,795 commands 0 home/root allows that main refuses; 0 changes vs 3032ab76a0
Kill corpus, 546 commands identical to main
Real bash, protected pushes 0 allowed
Your shape at 50 / 100 / 200 units (1.4 / 2.8 / 5.6 KB) 0.19 / 0.37 / 0.73 s, about 2x per doubling (was 3.1 / 12 / 47 s)
Gates 1,883 passed; only the 2 markdown_it host-import failures

One change to be upfront about: 6 push commands of one shape, echo $(cat <<<"$(cat <<E⏎)⏎E⏎)"⏎bash -c 'git push origin feat') (also sh -c, eval, -u), move from refused to allowed. In real bash every one pushes only feat, and no protected-push verdict moved.

@cruisercohen this head refuses every row family raised so far; if your private input still gets through, I'd be glad to have it.

A maintainer will need to approve the fork workflows for the new head. Thanks!

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turn on f5bf048215. Re-review on this head, compared against current main with real bash and a stub binary.

What is fixed on this head:

  • The heredoc body quote family from my last review is closed. Every row in that set that this branch allowed and main refused now refuses, and the temp directory twins keep the verdict main gives.
  • The superlinear run of nested heredoc spans is gone. My doubling probe went from roughly 4x per doubling on the previous head to about 2x, and it is now faster than main.
  • Push gate sweeps: every verdict is identical to the previous head, and the long run probes stay near linear.
  • Self kill sweep: byte identical to main.
  • The targeted security, push gate, kill, normalizer and liveness test files pass locally (7488 passed, 2 skipped). One unrelated test fails the same way on main in a bare worktree because the package is not installed there.

Still blocking (security path, details sent privately as before):

  1. A close paren on a heredoc body line, inside a nested quoted substitution, still ends the inner span early. If an opener that never closes (a brace parameter expansion, a nested substitution, an arithmetic or subshell opener) follows on the same or a later body line, the remainder is walked as one unproven span to the end, and the home operand behind the real close is never read. This branch allows those rows, main refuses them, and bash does delete the home directory. The set was also present on the previous head; my last pass did not cover it.
  2. The new same line pairing check for body quotes does not follow ANSI C quoting. An ANSI C quote on a body line whose next single quote is backslash escaped is treated as closing on its line, but the walk itself keeps that quote open past the real close, so the operand after the span is again unread. Same verdict split: this branch allows, main refuses, bash deletes.

A structural fix would cover both: once a span reaches a heredoc body, read every body line as data until its terminator line, rather than special casing each character class. If that is too large here, an unproven span that runs to the end of the text could be treated like a capped one so the rm floor fails closed on a home operand.

The earlier change request from another maintainer stays in place; a maintainer sign off is still needed on this security path after the fix.

continue
depth += 1
elif ch == ")":
depth -= 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 A body line close paren still ends the inner span early when an unclosed opener follows it

Inside a nested quoted substitution, a close paren on a heredoc body line reaches this branch and returns the inner span at the body line. The caller then resumes inside the body text, so any opener on the rest of the body that never closes (brace parameter expansion, a nested substitution, an arithmetic or subshell opener) is walked as one unproven span to the end of the text. The real close and the operand behind it are never read, so a home operand is not seen by the rm floor. This head allows those rows, main refuses them, and real bash deletes the home directory. Rows were sent privately.

            elif ch == ")":
                depth -= 1
                if depth == 0:
                    return (off + len(step.text), proven, pending, capped)

Suggestion: while heredoc_body is set, treat a body line close paren as data (as the bare open paren branch above already does at nest >= 1), and stop the body at its terminator line. Please add the class to the heredoc body family test with a home operand and a temp directory twin.

Rule: an unproven span must never hide an operand that bash executes; fail closed.

while i < len(text) and text[i] != "\n":
if text[i] == quote:
return True
i += 2 if quote == '"' and text[i] == "\\" else 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The same line pairing check does not follow ANSI C quoting

_quote_closes_on_line treats a backslash as an escape only inside double quotes. For an ANSI C quote on a body line, a backslash escaped single quote counts here as the closing quote, so the branch keeps the paired reading. The shell walk does follow ANSI C escapes, so it keeps the quote open past the span's real close and the operand after the span is never read. This head allows those rows, main refuses them, and real bash deletes the home directory. Rows were sent privately.

        i += 2 if quote == '"' and text[i] == "\\" else 1

Suggestion: pass the ANSI C flag in and skip backslash pairs for it too, or reuse the same quote stepping the walk uses so the two readings cannot disagree. Please add an ANSI C body row to the heredoc body family test.

Rule: two quote machines over the same text must agree, or the gate fails closed.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

@bolichen97 thanks for the re-review on f5bf048215, and for pushing for the structural fix. It's in on 76e9cc8a4a (one commit, still on origin/main b2b87b0f1a; a test rebase onto current main is clean):

  • Heredoc bodies are now read structurally. At a << the span walk parses the delimiter (<<- tab-strip; <<'E', <<"E", <<\E), and at the end of that line it skips every body line as data until its terminator line. Several pending heredocs on one line are consumed in order. This replaces the per-character body branches and _quote_closes_on_line, so both of your rows (a body ) before an unclosed opener, and an ANSI-C quote with \') close at the real ).
  • Backstop, as you suggested: a span the reader can't prove that also runs to the end of its text (an unparsable delimiter, or no terminator line) now fails closed on a home/root operand in the rm floor, like a capped span. /tmp twins keep main's verdict.

Checked against main 41708c96ab and the prior head:

Check Result
Both new families, 232 rows (body ) + ${ / $( / $(( / ( / backtick, ANSI-C \'; <<E / <<'E'; direct / nested / post; home / root / /tmp) 0 home/root rows that main refuses get through (the prior head let 44 through); /tmp twins match main
Earlier heredoc probes (288 + 120 + 24 + 58 rows) 0 rows that main refuses get through; /tmp twins match main
Rows refused here that main allows 159 across those probes; each one hands $HOME or / to a stub rm in real bash
rm corpus, 4,795 commands 0 home/root allows that main refuses; 0 changes vs f5bf048215
Push sweeps (6,688 + 2,268 + 20,328) 0 changes vs f5bf048215; 0 protected pushes allowed
Kill corpus, 546 commands identical to main
Nested heredoc run at 50 / 100 / 200 units 0.19 / 0.37 / 0.74 s, about 2x per doubling
Gates 1,884 passed; only the 2 markdown_it host-import failures

New tests: test_the_heredoc_body_is_read_structurally_to_its_terminator covers both families with /tmp twins, a <<-E tab-indented terminator and two heredocs on one line.

A maintainer will need to approve the fork workflows for the new head. Thanks!

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review on head 76e9cc8a4a, compared against current main b835fe9ca7 with real bash and stub binaries.

The structural rewrite is a real improvement and it closes both items from my last review:

  • The heredoc body families I flagged (a close paren on a body line before an unclosed opener, and an ANSI C body quote) now refuse. Across a full sweep of the heredoc rm shapes, there is not one row this head allows that main refuses, and the temporary directory twins keep the verdict main gives. The rm, push and self kill sweeps show this head is at least as strict as main on every row, and it closes several rows main still allows.
  • The superlinear run on nested heredoc spans is gone. My doubling probe is now about 2x per doubling and faster than main.
  • The targeted security, push gate, kill, normalizer and liveness tests pass locally (7489 passed, 2 skipped). One unrelated test fails the same way on main in a bare worktree because the package is not installed there.

Two items still block, both on the security path, and both match the two automated reviews on this head. Exact rows sent privately as before.

  1. A process substitution placed after a heredoc terminator line, inside an outer command substitution, is not scanned on this head. When the span reader skips the heredoc body and resumes, the resumed region is handed to the extractor as non extracting, so an opener that starts a process substitution on that resumed text never becomes its own unit. A protected branch push, and the product command that restarts this service, hidden in that position, are allowed here while main refuses them, and real bash runs both. The nested command substitution form of the same payload is correctly refused on this head, so only the process substitution form leaks.

  2. One advisory, same path. An ANSI C quoted heredoc delimiter is misread, so the reader does not match the terminator line, the span runs to the end unproven, and the rm floor refuses a legitimate temporary directory cleanup that main allows. This is the fail closed direction, so it is not a hole, but it is a usability regression worth fixing in the same pass.

Suggested direction for the first item: once the walk resumes after a skipped heredoc body, either read into the unproven resumed region so a process substitution or command substitution opener there becomes its own scanned unit, or emit the resumed body as a scanned entry, before the single pass that marks the over read as suppressed. Please cover both process substitution directions and the push, self kill and self restart floors, with a temporary directory twin for each new row. For the second item, decode the ANSI C quoting when reading the delimiter.

The earlier change request from another maintainer stays in place, and a maintainer sign off is still needed on this security path after the fix.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up review on head 76e9cc8a4a, compared against current main with real bash and stub binaries.

Verdict: Needs changes. 🔴 2 · 🟡 1

The structural heredoc rewrite is a real improvement, and it closes both items from my last review.

  • The earlier quote class and close paren class are fixed. A close paren on a heredoc body line before an unclosed opener, and an ANSI C quote inside a heredoc body, now both refuse. Across a full sweep of heredoc rm shapes, no row is allowed on this head that main refuses, and on the rm, push and self kill sweeps this head is at least as strict as main everywhere.
  • Nested heredoc timing is now linear. My doubling probe grows about 2x per doubling and runs faster than main.
  • The targeted security, push gate, kill, normalizer and liveness tests pass locally. One unrelated test fails the same way on main in a bare worktree because the package is not installed there.

Findings

🔴 1. Protected branch push not scanned. A process substitution placed after a heredoc terminator line, inside an outer command substitution, is not scanned on this head. When the span reader skips the heredoc body and resumes, the resumed region is treated as non extracting, so a process substitution opener on that text never becomes its own scanned unit. A protected branch push in that position is allowed here while main refuses it, and real bash runs it.

🔴 2. Service restart not scanned. The same class lets the product command that restarts this service through, in both quoted and unquoted outer forms. Again main refuses it and real bash runs it. The nested command substitution form of the same payload is correctly refused on this head, so only the process substitution form leaks.

🟡 3. Legitimate cleanup refused. An ANSI C quoted heredoc delimiter is misread, so the terminator line is never matched, the span runs to the end unproven, and a legitimate temporary directory cleanup that main allows is refused. This fails closed, so it is not a hole, but it is a usability regression worth fixing in the same pass.

Suggested direction

For the two blocking items: once the walk resumes after a skipped heredoc body, either read into the resumed region so a process substitution or command substitution opener there becomes its own scanned unit, or emit the resumed text as a scanned entry before the pass that marks the over read as suppressed. Please cover both process substitution directions and the push, self kill and self restart floors, with a temporary directory twin for each new case. For the advisory: decode ANSI C quoting when reading the delimiter.

I am happy to share the exact reproduction cases privately with the author, as before.

The earlier change request from another maintainer stays in place, and a maintainer sign off is still needed on this security path after the fix.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks to the Opus 5.5 and GPT 6.1 reviews on 76e9cc8a4a. All three findings are real and fixed on 7cd2aa735e (one commit, still on origin/main b2b87b0f1a; it merges cleanly onto current main):

  • A <(…) after a heredoc inside a $(…) was never extracted (Opus span.py:207, GPT shell_normalizer.py:1474). The unproven span's readings were all suppressed and the walk jumped past the inner opener. The span's own body up to its real close is now emitted as a non-suppressed entry, bounded to that span, so its <(bash -c '…'), $(…) or `…` reaches the push floor and the self-protection floors. A run of heredoc spans stays linear.
  • An apostrophe inside a double-quoted assignment prefix (Opus rm_floor.py:75). _in_assignment_value now tracks double quotes, so MSG="it's" no longer flips it into single-quote state and the home-operand backstop is applied again.
  • <<$'E' read as $E (GPT rm_floor.py:134). The heredoc delimiter now decodes ANSI-C $'…' and locale $"…" quoting, so the body ends at its real terminator and a /tmp cleanup gets main's verdict again.

Each finding has a regression row (they fail on 76e9cc8a4a and pass here), with feat/x, nginx and /tmp twins.

Checked against main 41708c96ab and 76e9cc8a4a:

Check Result
The review rows, real bash with stub git / rm / kirocrew <(…) push to main, kirocrew restart and the MSG="it's" home wipe are refused; the feat/x, nginx and <<$'E' /tmp twins are allowed, as on main
Heredoc and rm probes (232 + 288 + 120 + 24 + 58 rows) 0 home/root rows that main refuses get through; /tmp twins match main
Rows refused here that main allows 159 across those probes; each one hands $HOME or / to a stub rm in real bash
rm corpus, 4,795 commands 0 home/root allows that main refuses; 0 changes vs 76e9cc8a4a
Push sweeps (6,688 + 2,268 + 20,328) 0 protected pushes allowed. The only changes vs 76e9cc8a4a are the 6 echo $(cat <<<"$(cat <<E⏎)⏎E⏎)"⏎<launcher> 'git push origin feat') rows I mentioned earlier; they are refused again, as on 3032ab76a0
Kill corpus, 546 commands identical to main
Nested heredoc run at 50 / 100 / 200 units 0.19 / 0.37 / 0.74 s, about 2x per doubling
Gates 1,891 passed; only the 2 markdown_it host-import failures

One note: MSG="it's" rm -rf "$(echo $(( 1<<2⏎)))" /tmp/x is now refused, the same as the same command without the apostrophe. That's the arithmetic << refusal this PR already documents.

A maintainer will need to approve the fork workflows for the new head. Thanks!

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re review on head 7cd2aa735e, compared against current main with real bash and stub binaries.

Both items from my last review are fixed on this head:

  • A process substitution after a heredoc terminator inside an outer command substitution is now scanned. Every row I sent privately is refused here, as main refuses it. A wider sweep of runnable openers placed after a heredoc body (eleven header spellings, both process substitution directions, nested substitutions, backticks, five outer forms, and the push, self kill, self restart and rm floors) has no row this head allows, and the benign twins keep the verdict main gives.
  • The ANSI C heredoc delimiter is decoded, and the temporary directory cleanup I flagged is allowed again, as on main.
  • Every earlier sweep (the full heredoc rm sweep, the quote rows, the rm corpus, and the push and kill rows) gives the same verdict as the previous head, so nothing regressed.
  • The targeted security, push gate, kill, normalizer and liveness tests pass locally (7496 passed, 2 skipped). One test fails the same way on main in a bare worktree because the package is not installed there.

One item still blocks, on the security path. It matches the blocking finding the automated review posted on this head, and I reproduced it. Exact rows are sent privately as before.

  1. The new non suppressed reading is emitted only when the unproven span ends in its own close paren. A span can also be left unproven by a left shift that bash reads as arithmetic, never as a heredoc. When such a span runs to the end of the command (text after its close, or a double quoted outer word), every reading of it stays suppressed, so a process substitution or nested command substitution later in that span is never extracted. A protected branch push, a self restart and a self kill placed there are allowed on this head while main refuses them, and real bash runs all three. The previous head had the same gap. The commit message calls the arithmetic case the fail closed direction, but that holds for the rm floor only, not for the payload walk.

Suggested direction: when an unproven span that is not capped runs to the end, emit its remainder non suppressed (or keep the flat walk going from the opener) so its nested openers become their own entries. Another option is to stop counting a shift inside arithmetic as a heredoc start. Please add rows for both process substitution directions and a nested substitution, with unquoted, quoted and trailing text outers, on the push, self kill and self restart floors, each with a benign twin.

One advisory, same path. Because the new reading is extracted again at every nesting level, nested heredoc spans close to the nesting cap now cost about fourteen times the previous head, and about one and a half times main, at the scan cap. main has a row about as slow, so this is not a new class, but a test that pins nested spans near the cap would keep it from growing.

The earlier change request from another maintainer stays in place, and a maintainer still needs to sign off on this security path after the fix.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks to the Opus 5.5 review on 7cd2aa735e. The finding is real: with $((1<<2)) before a newline, the << is read as a heredoc with no terminator line, so the outer $(...) runs to the end of the text. The fix from the last round only covers a span that still closes, so the <(bash -c '…') after the newline was no payload source, and bash runs it.

Fixed on 94a81f2fce (one commit, still on origin/main b2b87b0f1a; it merges cleanly onto current main). When an unproven span runs to the end of its text and an opener sits in that remainder, the remainder is now scanned once, flat, the same way the capped never-close case already is, and the walk ends there. New test test_a_run_command_in_a_span_that_never_closes_is_denied: the <(…), $(…) and backtick forms, with and without ; true, plus kirocrew restart, are refused; the feat/x and nginx twins are allowed. It fails on 7cd2aa735e and passes here.

Checked against main 41708c96ab and 7cd2aa735e:

Check Result
Your row and its variants, real bash with stub git / kirocrew each pushes main or restarts; all refused now, matching main; the feat/x and nginx twins are allowed
Push sweeps (6,688 + 2,268 + 20,328) 0 changes vs 7cd2aa735e; 0 protected pushes allowed
Heredoc and rm probes (232 + 288 + 120 + 24 + 58 rows) 0 home/root rows that main refuses get through; /tmp twins match main
rm corpus, 4,795 commands 0 home/root allows that main refuses; 0 changes
Kill corpus, 546 commands identical to main
Nested heredoc run at 50 / 100 / 200 units 0.19 / 0.37 / 0.75 s, about 2x per doubling
Gates 1,892 passed; only the 2 markdown_it host-import failures

One note: a long run of nested echo $(echo $((1<<2))⏎ units is slow on this branch (43 s at 4.6 KB vs 21 s on main). It's the same on 7cd2aa735e, and main also grows about 4x per doubling there, so this push didn't change it.

A maintainer will need to approve the fork workflows for the new head. Thanks!

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re review on head 94a81f2fce, compared against current main and the previous head with real bash and stub binaries.

The blocking item from my last review is fixed on this head:

  • A span left open by an arithmetic shift that runs to the end of the command now has its runnable openers scanned. Every row I sent privately is refused here, as main refuses it, and so is the row from the automated review on the previous head. A wider sweep (nine spellings of a << that bash does not read as a heredoc, with and without a later line that looks like a terminator, eleven opener shapes, eleven outer shapes, and the push, self kill, self restart and rm floors) has no row this head allows, and every benign twin keeps the verdict main gives.
  • Every earlier sweep (the heredoc rm sweeps, the quote rows, the delimiter spellings, the assignment prefixes, the rm corpus, and the push and kill rows) gives the same verdict as the previous head or a stricter one, so nothing regressed.
  • The new flat pass adds no measurable cost. Nested and sequential spans of this shape at the scan cap stay near linear and are faster than main.
  • The targeted security, push gate, kill, normalizer and liveness tests pass locally (7498 passed, 2 skipped).

One item still blocks, on the security path. Exact rows are sent privately as before.

  1. The new never close branch scans the remainder with _capped_tail_entries, which starts from a neutral quote state and has no heredoc model. When a real heredoc sits between the arithmetic line and the opener, and its body holds a quote that does not close on its line (an apostrophe, or an ANSI C quoted escape), that pass opens a single quote over the rest of the text, so the later process substitution is never its own entry. A self restart and a self kill placed there are allowed on this head while main refuses them, and real bash runs both. The previous head allowed these too, so this is a remainder of the same gap rather than a regression. The span walk already reads such a body as data; the flat pass does not.

Suggested direction: skip heredoc bodies as data in this pass (the delimiter reader and body skipper already exist), or emit the remainder's openers through the structural walk instead. Please add rows for both quote kinds on a body line, both process substitution directions, unquoted, quoted and trailing text outers, and the push, self kill and self restart floors, each with a benign twin.

One advisory from my last review is unchanged: nested heredoc spans near the nesting cap still cost about one and a half to two times main at the scan cap. A test that pins that shape would keep it from growing.

The earlier change request from another maintainer stays in place, and a maintainer still needs to sign off on this security path after the fix.

# in that remainder, scan it ONCE, flat, as the capped never-close
# branch does, so each opener is its own entry; the walk ends
# here, so a run stays linear.
entries.extend(_capped_tail_entries(text, i + 2))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The never close flat pass reads a heredoc body quote as live, so a later process substitution is not scanned

Cause: this branch hands the remainder to _capped_tail_entries(text, i + 2), which walks from a neutral quote state with no heredoc model. Mechanism: a real heredoc after the arithmetic line whose body line holds an unpaired quote opens a single quote in that pass, and the <( or >( behind it becomes data inside one long run. Consequence: a self restart or self kill there is no payload source; this head allows it, main refuses it, and bash runs it.

            elif end >= len(text) and any(
                op in text[i + 2 :] for op in ("$(", "<(", ">(", "`")
            ):
                ...
                entries.extend(_capped_tail_entries(text, i + 2))

Suggestion: skip heredoc bodies as data in this pass (reuse _heredoc_delimiter and _skip_heredoc_bodies), or emit the remainder's openers through the structural walk. Add rows with an apostrophe and an ANSI C escape on a body line, both process substitution directions and the three floors, each with a benign twin.

Rule: an unproven span must never scan less than main scans; fail closed on the payload walk.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks to the Opus 5.5 review on 94a81f2fce. The finding is real. A double-quoted $( holding an arithmetic << and then a newline is read as a heredoc with no terminator, so the span runs to the end of the text. _segments then resumed at the end, and no later command was produced as a segment. In real bash, echo "$(echo $((1<<2)) + newline + )" ; rm -r -f $HOME closes the span and runs the rm.

Fixed in 631c965cc2, still one commit, now rebased onto main @ 241e39cf78. The only conflict was _PACKAGE_LINE_BUDGET, which is re-pinned to the measured 33,053. I took the review's first option: when the rm floor meets an unproven span that runs to the end of the text, it refuses the whole command through the existing unread-command refusal (_RM_FLOOR_UNREAD_NOTE). A span closed at a guess past the nesting cap keeps its existing handling.

What I checked:

  • 90 probe rows (echo / x= / rm -f lead × ; / && / || / | / newline × $HOME / ~ / ${HOME} / / / /tmp): head was allowing 75 that main refuses. 60 of them are real home or / wipes under a stub rm; the || rows never reach rm. All 90 are refused now. That includes 15 /tmp twins, since the span cannot be read.
  • 4,795-command rm corpus: 0 home/root rows head allows that main refuses. 27 rows move allowed→denied vs 94a81f2fce, and main allows all 27 too (a deep x="$(…${x:-(}…)" rm -fr … chain): 24 are real home or / wipes, and 3 are /tmp over-refusals.
  • Push sweeps vs 94a81f2fce: 0 changes (6,688 + 2,268 + 20,328), and 0 protected pushes allowed. The kill corpus (546) is identical to main. Every earlier probe set (c102 to c113) is identical to the prior head.
  • Speed: a run of nested heredoc spans still grows about 2x per doubling (0.21 / 0.41 / 0.81 s).
  • Gates: 1,894 passed, plus the 2 markdown_it host-import failures. black / isort / flake8 / mypy and comment-history are clean.

New test: test_a_later_rm_after_a_span_that_never_closes_is_denied.

The Security Scope lane is red only because its per-platform reports could not be folded; its own verdict is PASS. @iamwhatever @bolichen97 @cruisercohen, could one of you approve the workflow runs on this head?

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re review on head 631c965cc2, compared against current main and the previous head with real bash and stub binaries.

What changed since my last review: the branch is rebased onto current main, and the rm floor now refuses the whole command when a double quoted $( span runs to the end of the text. The span reader, the payload walk and the self kill walk are the same as on the previous head.

Verified on this head:

  • Every row from my earlier sweeps gives the same verdict as the previous head. The wider sweeps of << spellings that bash does not read as a heredoc, and their benign twins, still match main.
  • The targeted security, push gate, kill, normalizer and liveness tests pass locally (11718 passed, 885 skipped). One unrelated worktree fence test fails in my environment only; it is not in this diff.
  • The black check fails on shell_normalizer_span.py and test_push_branch_gate_parser.py; CI already names the fix.

Two items still block, both on the security path. Exact rows are sent privately as before.

  1. The blocking item from my last review is not fixed. The never close flat pass still reads a quote on a real heredoc body line as live, so a process substitution after that heredoc is not scanned. A self restart and a self kill there are allowed on this head while main refuses them, and real bash runs both. Inline comment 1.

  2. The automated Opus review on this head is right, and the class is wider than its single row. The self kill walk ends at a span the matcher cannot close (an arithmetic << before a newline, in any of the three arithmetic spellings), but bash closes that span and runs the next command. A later kill whose verb comes from a variable, or a literal kill after a double quoted span, is never read. Across a small sweep (three arithmetic spellings, four span shapes, three separators, five decoy shapes) this head allows over a hundred rows that main refuses, and real bash kills the target process in each one. The previous head allowed them too, so this is not new in this push, but it is a regression against main. One correction to the automated row: as written, its decoy output joins onto the process id, so bash rejects that one argument. Changing only the spacing makes it kill. Inline comment 2.

Two advisories:

  • The automated security scope review is also right. The new whole command refusal fires whenever the text holds an rm substring anywhere (confirm, format, terminal, arm64). So a commit or PR body heredoc with its terminator joined to the closing paren, which bash accepts with a warning, is now refused with the rm message. That also applies to an arithmetic shift inside a quoted span. I count 28 benign rows that main and the previous head allow and this head refuses. Requiring a real rm command word in the cut segments before raising would keep the fail closed case and drop these. Inline comment 3.
  • Nested heredoc spans near the nesting cap still cost about 1.6 times main at the scan cap (16.3 s against 10.3 s, two runs), a little more than the previous head (14.8 s). The quoted and process substitution shapes are now much faster than main.

The earlier change request from another maintainer stays in place, and a maintainer still needs to sign off on this security path after the fix.

# in that remainder, scan it ONCE, flat, as the capped never-close
# branch does, so each opener is its own entry; the walk ends
# here, so a run stays linear.
entries.extend(_capped_tail_entries(text, i + 2))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The never close flat pass still reads a heredoc body quote as live, so a later process substitution is not scanned

Cause: this branch hands the remainder to _capped_tail_entries(text, i + 2), which starts from a neutral quote state and has no heredoc model. Mechanism: a real heredoc after the arithmetic line, with a body line that holds a quote that does not close on that line, opens a single quote in this pass, and the <( or >( behind it becomes data. Consequence: a self restart or self kill there is not a payload source. This head allows it, main refuses it, and bash runs it. The previous head behaved the same.

                entries.extend(_capped_tail_entries(text, i + 2))

Suggestion: skip heredoc bodies as data in this pass (reuse _heredoc_delimiter and _skip_heredoc_bodies), or emit the remainder's openers through the structural walk. Add rows with an apostrophe and with an ANSI C escape on a body line, for both process substitution directions and the push, self kill and self restart floors, each with a benign twin.

Rule: an unproven span must never scan less than main scans; fail closed on the payload walk.

# closes has ``rel == len(remainder)``, so this lands at ``n``
# and the walk ends -- bash cannot execute past an unterminated
# substitution either, so there is no later command to lose.
i = off + 2 + rel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The self kill walk ends at a span bash closes, so a later kill is never read

Cause: when _matching_close_paren cannot close the span, rel is the whole remainder and the walk resumes at the end. Mechanism: the comment above this line says bash cannot run past such a span, but an arithmetic << before a newline (as $((..)), $[..] or ((..))) leaves the matcher waiting for a heredoc while bash closes the span on the next line and runs the next command. Consequence: a later kill whose verb comes from a variable, or a literal kill after a double quoted span, with a decoy in its argument, is not read by this walk, and nothing that runs earlier catches it. This head allows the self kill and main refuses it. Real bash kills the target in every row I ran. The previous head behaved the same.

                # Resume past the close (``off + 2 + rel``); a span that NEVER
                # closes has ``rel == len(remainder)``, so this lands at ``n``
                # and the walk ends -- bash cannot execute past an unterminated
                # substitution either, so there is no later command to lose.
                i = off + 2 + rel

Suggestion: when the span is unproven and runs to the end, keep the whole remainder over read but resume at a heredoc agnostic close (the flat quote aware paren count main uses), so later sibling segments are still walked. Apply the same to the <( and >( branch. Add rows for the three arithmetic spellings, x=$(..), echo $(..), echo "$(..)" and cat <(..) spans, ;, && and newline separators, and variable and literal kill verbs with printf, echo and process substitution decoys, each with a benign twin.

Rule: a span the reader cannot prove must not end a walk that main continued; fail closed on the kill floor.

Comment thread src/kiro_crew/security/rm_floor.py Outdated
# then read as an argument of the ``~``, not as the command, so
# its home wipe is missed.
if not proven and close >= len(body):
raise _UnreadSpan(command)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The whole command refusal fires on any rm substring, so benign heredoc bodies are refused

Cause: _segments raises _UnreadSpan for any unproven span that runs to the end, and the gate reaches it whenever _can_fire sees the letters rm anywhere. Mechanism: a commit or PR body heredoc whose terminator is joined to the closing paren (EOF), which bash accepts with a warning), or an arithmetic shift inside a quoted span, runs to the end in this reader. Consequence: a message containing confirm, format, terminal or arm64 is refused with the rm message. main and the previous head allow it. I count 28 such benign rows across commit and PR create bodies.

                if not proven and close >= len(body):
                    raise _UnreadSpan(command)

Suggestion: raise only when a cut segment, or the unread remainder, holds rm as a command word, not as a substring. Pin the rows above as allowed and keep the home and root wipe rows refused.

Rule: a fail closed branch should refuse what could hide the dangerous command, not every text that spells its letters.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks to the Opus 5.5 and Security Scope reviews on 631c965cc2. Both findings are real, and both are fixed in 083ca1d4ce (still one commit on 241e39cf78).

Opus: self-kill after a span that never closes. Reproduced: k=kill; x=$(echo $((1<<2)) + newline + ); $k $(printf ')' ; pgrep -f kirocrew) was allowed on 631c965cc2 and refused on main. The raw walk in _bare_kill_raw_bodies ended at the span the heredoc rule never closes. Both the $( and <(/>( branches now keep the whole-remainder over-read and then resume at the close a heredoc-agnostic walk finds (_heredoc_agnostic_close, the same span walk with << opening no heredoc). So the later $k $(...) sibling is walked and refused. The nginx twin stays allowed.

Security Scope: EOF)" commit message newly refused. Reproduced: git commit -m "$(cat <<'EOF' + newline + fix: confirm the terminal format + newline + EOF)" was refused on 631c965cc2 and allowed on main. Real bash 5.2 ends the body at that glued EOF) inside $(...) (with a warning) and runs the commit. _skip_heredoc_bodies now ends the last pending body at a line that starts with the delimiter followed by ), so the span closes there and the rm floor no longer sees an unreadable span. The 'EOF', EOF, "EOF" and <<- forms are back to main's verdict. ; rm -rf ~ after them, and a ~ operand after the span, stay refused. EOFX) is not the delimiter, so it is still read as a body line and refused (the fail-closed side).

Also fixed: the two files black flagged in Backend Lint.

Verification against 631c965cc2 and main d2417a48c1:

  • New 36-row probe: identical to main except the EOFX) row.
  • Every earlier probe: identical to the prior head.
  • Push sweeps: 0 changes (6,688 + 2,268 + 20,328), 0 protected pushes allowed.
  • rm corpus (4,795): 0 changes. Kill corpus (546): identical to main.
  • Span timing still linear (0.82 s at 5.8 KB).
  • Gates: 1,896 passed, plus the 2 markdown_it host-import failures. Line budget re-pinned to the measured 33,076.

Two new tests pin both cases. The PR description's "This revision" section has items 6 and 7.

Could a maintainer please approve the workflow runs on 083ca1d4ce?

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the two fixes on this revision. I reran the earlier probes against this head and against the base.

Confirmed fixed, and now matching the base:

  • The self stop case that used an indirect verb after a span the here document rule never closed. The variable verb sweep is now identical to the base on every row.
  • The commit message whose here document terminator is glued to the closing parenthesis of the substitution. Ordinary messages that hold a two letter removal substring are allowed again, and a real removal after the span still refuses.

Still blocking (the base refuses it, this head allows it, and a plain shell runs the dangerous action):

There is a remaining class the two fixes do not cover. When an arithmetic style shift is followed on a later line by a genuine here document whose body contains an unbalanced quote or parenthesis, and then by an opener that runs a control command, in one of the line orderings, the reader treats the here document body as live shell and stops early. The later opener is never read, so a command that stops or restarts the running agent is allowed on this head while the base refuses it. I confirmed in a plain shell that the control action actually fires, across the stop and the restart payloads, for the same count of rows seen on the parent revision. This is the same body as the quote class the walk resume was meant to cover, but it is reached through a real here document rather than an indirect verb, so the resume does not catch it.

The structural note from the design review still applies. Rather than adding another branch for each new spelling, an unreadable or never balancing span should refuse the whole command in the self floor, the way the removal floor already does. That would close this ordering without another round.

Non blocking over refusals, noted only so they are tracked, both fail closed:

  • An empty or escaped here document delimiter inside a double quoted substitution makes an ordinary temporary directory cleanup refuse, where the base allows it.
  • A here document terminator with a space before the closing parenthesis, and an arithmetic shift inside a quoted substitution, still refuse a few benign commit messages and description bodies that hold a two letter removal substring. The no space form of that terminator is fixed this revision.

Span timing on this head is linear and no slower than the base at depth, so the earlier timing note is resolved.

The change request already on this pull request from the other reviewer still stands on the blocking class above. I am leaving this as a comment and handing the merge decision to a maintainer, since this touches the security floor.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Status on 083ca1d4ce: Opus 5.5, GPT 6.1 and Design pass, UX skipped, Backend Lint is green again, and Linux backend shards are passing (two still running).

Security Scope. The red check is a fold failure ("the per-platform reports could not be folded"); the review's own verdict is Scope-Verdict: PASS. It left one candidate unadjudicated, so I ran it myself:

Command This head 631c965cc2 main
mask="$(⏎for b in 0 1 2; do printf '%d ' $((1 << b)); done⏎)"⏎rm -f /tmp/mask.txt refused refused allowed
same, rm -rf $HOME / rm -rf / after it refused refused refused

The refusal comes from the arithmetic << shift followed by a newline inside "$(…)": the span reader does not model arithmetic, so the span is unproven and scanned whole. That is the documented fail-closed direction (docs/ security notes: "a shift followed by a newline before the span closes leaves the span unproven and scanned whole"), it predates this push, and it only triggers when the text also holds a literal rm-like word. The review itself says "do not drop the refusal". If a maintainer wants this narrowed, the scoped fix is to give $((/(( a non-heredoc reading in _close_paren_walk; I'm happy to do that in a follow-up, but I'd keep this PR fail-closed.

Could a maintainer re-run Security Scope Review so its report folds?

@cruisercohen your CHANGES_REQUESTED is on f1d2e350b; the inputs posted on this PR since then are refused on this head, as on main. Could you re-review 083ca1d4ce? If your private second input still gets through, I'll fix it as soon as I have it.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the rebase. This revision moves the branch onto a newer base; the security code under the touched module is unchanged from the previous revision, apart from the package line budget pin, which follows the larger base. I reran every earlier probe against this head and against the current base.

Still blocking (the base refuses it, this head allows it, and a plain shell runs the dangerous action):

The class from my previous comment is still open, with the same rows. When an arithmetic style shift is followed on a later line by a genuine here document whose body holds an unbalanced quote or parenthesis, and then by an opener that runs a control command, in one line ordering, the reader treats the here document body as live shell and stops early. The later opener is never read, so a command that stops or restarts the running agent is allowed on this head while the base refuses it. I confirmed again in a plain shell that the control action fires, for the stop and the restart payloads, with the same row count as before.

The structural suggestion still applies: an unreadable or never balancing span should refuse the whole command in the self floor, the way the removal floor already does, rather than adding a branch per spelling.

What this head does well: in the same sweep the base allows and runs a set of push and restart rows that this head refuses, so the change closes real base gaps. The indirect verb sweep, the protected push sweep and the benign twins of every sweep match the base on every row.

Non blocking over refusals, both fail closed and unchanged from the previous revision:

  • An empty or escaped here document delimiter inside a double quoted substitution makes an ordinary temporary directory cleanup refuse, where the base allows it.
  • A here document terminator with a space before the closing parenthesis, and an arithmetic shift inside a quoted substitution, still refuse a few benign commit messages and description bodies that hold a two letter removal substring.

On the review lanes for this head: the security scope lane passes with no newly refused candidate, which matches my benign sweeps. The second model lane reports an incomplete review on this head, so its earlier delimiter note has not been adjudicated again here.

The change request already on this pull request from the other reviewer still stands on the blocking class above. I am leaving this as a comment and handing the merge decision to a maintainer, since this touches the security floor.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

@bolichen97 thanks for the re-review on ef9fb000ed, and for keeping this class open. It's real, and it is fixed on b7a0dcbcf9 (still one commit on main 5f77c3f85c).

Cause. This is the never-close flat pass from your inline note on shell_normalizer_span.py:264. When a span never closes, its remainder was scanned once by _capped_tail_entries, which reads quotes from a neutral state and has no heredoc model. A real heredoc after the arithmetic line, with a body line holding an unbalanced quote (it's, $'x\'y), opened a quote over the rest of the text. So a later <(...) / >(...) self restart or self kill was no payload source, while bash runs it.

Fix. The never-close remainder is now also scanned quote-blind (_capped_tail_entries(quote_blind=True)): every opener, backtick and ) is a boundary, and no quote is read at all. So no body line can swallow an opener there, whatever character it holds. The quote-aware pass stays, so this only adds payload sources. Its runs are disjoint, so the walk stays linear.

On the structural suggestion: refusing the whole command in the self floor would also refuse every benign command with an unprovable span and a self-floor word in it. The blind pass closes the same ordering for every body character without that, which is why I took it. If your private rows still get through, I'll switch to the whole-command refusal.

My sweep built from your description (1,920 rows): 3 arithmetic spellings, 5 body lines (', $'x\'y, ", (, )), <(, >(, $( and plain-line openers, echo $(…), x=$(…), echo "$(…)" and top-level outers, both line orderings, with a self restart, a self kill (pkill -f) and an nginx twin.

ef9fb000ed b7a0dcbcf9
Rows allowed that main refuses 48 0
...of which real bash runs the restart or kill (stubs) 24 0
Twins whose verdict differs from main 0 0

Checked on b7a0dcbcf9:

  • Every earlier probe set gives the same verdict as ef9fb000ed.
  • Push sweeps: 0 changes over 6,688 + 2,268 + 20,328 commands, 0 protected pushes allowed.
  • rm corpus (4,795): 0 changes. Kill corpus (546): identical to main.
  • Span timing is linear (0.82 s at 5.8 KB). The arithmetic run shape is unchanged from ef9fb000ed.
  • New test test_a_heredoc_quote_after_a_span_that_never_closes_hides_no_opener. Package line budget re-pinned to the measured 33,333.

Could you rerun your private rows on b7a0dcbcf9? The two non-blocking over-refusals you listed are unchanged and both fail closed. Could a maintainer please approve the workflow runs for this head?

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix. I reviewed head 3a84d08. It is the fix revision rebased onto a newer base; the security code it adds is the same as the previous fix revision, apart from the package line budget pin. I reran every earlier probe against this head, against the fix revision before the rebase, and against the current base.

The blocking class from my previous comment is fixed. Scanning the never closing remainder a second time without reading quotes means an unbalanced quote on a genuine here document body line can no longer hide a later opener. Every one of the 64 rows that ran a stop or restart action in a plain shell is now refused, and the head no longer allows any row in that sweep that the base refuses. The earlier single repro is refused too. The new test covers the single and ANSI quote bodies, both opener directions and all three outer forms, with a benign twin for each.

No new over refusal: the benign twins of the same sweep (43200 rows) and the indirect verb sweep match the base on every row. The head still refuses the push and restart rows that the base allows and runs, so the change keeps closing real base gaps. The touched security test files pass locally on this head (503 tests).

Still open, non blocking, both fail closed and unchanged:

  • An empty or escaped here document delimiter inside a double quoted substitution makes an ordinary temporary directory cleanup refuse, where the base allows it (3 rows).
  • A here document terminator with a space before the closing parenthesis, and an arithmetic shift inside a quoted substitution, still refuse a few benign commit messages and description bodies that hold a two letter removal substring (16 rows).

These are fine as a follow up if you prefer to keep this change focused.

The review lanes have not reported on this head yet, so I am not citing them. The change request already on this pull request from the other reviewer is theirs to clear. I am leaving this as a comment and handing the merge decision to a maintainer, since this touches the security floor.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a9d641d379 (one commit, still on origin/main @ 33cdb8f1ee; no conflict with the current main).

GPT 6.1 — double-quoted heredoc delimiter (upheld, fixed). Reproduced on 3a84d0827c: rm -rf "$(cat <<"E\q" + newline + " + newline + Eq)" + newline + E\q + newline + )" ~ was allowed, main refuses it, and bash (5.2, stub rm) hands rm the home directory. Inside "..." bash drops a backslash only before $, a backtick, ", \ or a newline, so the delimiter is E\q; the span reader dropped every backslash, read Eq, and closed the span at the body line Eq)". _heredoc_delimiter now unescapes both "..." and $"..." with bash's double-quote rules (_dq_unescape); "E\$" still ends at E$, and "E\\" at E\, both checked against bash.

Check Result
Your row + variants (~, $HOME, /, E\z) 6 rows refused now, as on main; /tmp twins allowed, as on main
288-row sweep (12 escaped characters x "/$" x kept/dropped early line x ~ / / / /tmp x 2 outer forms), each run in real bash 0 rows allowed that wipe home or / (prior head: 20, all refused by main); 0 rows refused that main allows and bash runs harmlessly
Every earlier probe set identical to 3a84d0827c
Push sweeps (6,688 + 2,268 + 20,328) 0 changes, 0 protected pushes allowed
rm corpus (4,795) / kill corpus (546) 0 changes / identical to main
Span timing linear, 0.82 s at 5.8 KB
Gate tests 1,848 passed + the 2 markdown_it host-import failures

New test: test_a_double_quoted_delimiter_keeps_a_backslash_bash_keeps. The description has a new item 9.

@bolichen97 thank you for re-running everything on 3a84d0827c. Agreed on your two non-blocking fail-closed rows (the empty / escaped delimiter /tmp cleanup, and the spaced EOF ) / quoted arithmetic shift commit messages): they only refuse, so I'll keep this change focused and take them as a follow-up.

@cruisercohen your changes-requested review is on f1d2e350b; when you have a moment, could you re-check it against this head?

Could a maintainer please approve the workflow runs on a9d641d379? Thank you.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. I reviewed head a9d641d against the previous head 3a84d08 and the current base.

What changed: a double quoted (and locale quoted) here document delimiter is now unescaped by double quote rules, so a backslash before a character that is not escapable inside double quotes stays part of the delimiter, as bash keeps it. I checked this against bash directly: for every printable character after a backslash, in both quote forms (186 delimiters), I asked bash which line ends the here document and confirmed the gate still refuses a home directory removal placed after that span. The head refuses every row; the previous head allowed half of them in the plain double quoted form, so this closes a real gap. The new test covers both quote forms, a non escapable and an escapable character, and a benign twin.

No regression: the earlier stop and restart sweep (54000 rows) has no row the head allows and the base refuses, its benign twins (43200 rows) match the base on every row, and the earlier single repro is still refused. The touched security test files pass locally on this head (8259 passed; the one failure also fails on the base in my environment, as it needs an installed package).

Status of the two non blocking items from my last comment, both fail closed:

  • Empty or escaped delimiter: partly fixed. The double quoted empty delimiter and the double quoted escaped delimiter now allow the ordinary temporary directory cleanup, matching the base. The single quoted empty delimiter (<<'') still refuses it (1 row).
  • A terminator with a space before the closing parenthesis, and an arithmetic shift inside a quoted substitution: unchanged (16 rows).

Both remain fine as a follow up.

The branch now has a merge conflict with main, so it needs a rebase before it can land. The change request already on this pull request from the other reviewer is theirs to clear. I am leaving this as a comment and handing the merge decision to a maintainer, since this touches the security floor.

@Maqix

Maqix commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto origin/main @ 44702cbcda as one commit: b4cac74105. Thanks @bolichen97 for the review of a9d641d379.

Conflict. Main #23292 (74e66024bd) also changed rm_floor.py. Both sides are kept: main's backtick-body-in-double-quotes read, glued-redirection word, leading-redirect skip and _names_rm_word floor-error rule stay as they are, and this PR's heredoc / unread-span reads sit beside them. git diff origin/main HEAD -- rm_floor.py is the same 82 lines this PR changed before. security.md keeps main's paragraph plus this PR's two sentences. Budget 33,440, measured.

One behaviour change from the rebase, matching main. Main's floor-error path now refuses only when the text names rm as a word. So 2 rows that the old head refused because a span could not be read are now allowed, as on main:

  • git commit -m "$(cat <<EOF⏎fix: confirm the terminal format⏎EOFX)"
  • x="$(⏎printf -v f '%d' $((1 << b)); echo format⏎)"⏎echo done (the Security Scope case from round 116)

In real bash with stub rm/git, neither calls rm. A real rm after an unread span is still refused (the c114/c116 home and root rows are unchanged).

Checks on b4cac74105 vs the old head and vs main:

  • 0 rows anywhere that this head allows and main refuses.
  • Push sweeps 6,688 / 2,268 / 20,328: 0 flips, 0 protected pushes allowed.
  • rm corpus 4,795: 0 changes vs the old head. Kill corpus 546: same as main.
  • The 1,920 restart/kill rows from round 118: same as before and main on every row.
  • An extra 40-row probe of rm built from variables after an unread span: same as the old head on every row.
  • Main fix(security): read backtick bodies and glued redirects in the rm floor #23292's own rows pass (169). Gate set: 1,932 pass, plus the 2 usual markdown_it host-import failures. Timing still linear.

Could a maintainer approve the runs on the new head?

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. I reviewed head 57363ec against the previous head f671e86 (same base), using a range diff plus local probes.

What changed: the rm floor now re-reads an unproven double quoted $( span with << treated as an arithmetic shift, over a fixed 256 character window, and unions in the operand that reading exposes. This closes the short form of the late close hole: rm -r -f "$(echo $((1<<X)) newline )" ~ ; : ' newline X newline )' is refused on this head and was allowed on f671e86. The new test and the line budget bump match the change, and the touched parser and liveness test files pass locally on this head (106 passed).

One blocking item remains, inline below: the fixed window can be stepped over with padding, so the same home removal is still allowed when the span body or the gap before the operand is longer than 256 characters. The base refuses those rows, so this is still an allow regression against main. I checked it three ways: the gate verdict on base, f671e86 and this head, and real bash, which passes the home directory to the command in both padded shapes.

Status of the earlier non blocking items, both fail closed: the single quoted empty delimiter (<<'') still refuses the ordinary temporary directory cleanup, and the plain arithmetic shift cleanup rm -rf "$(echo $((1<<2)))" /tmp/x is allowed. Both are fine as a follow up.

The change request already on this pull request from the other reviewer is theirs to clear.

Comment thread src/kiro_crew/security/rm_floor.py Outdated
)
if agn_proven and agn_close < close:
try:
tail = _segments(body[agn_close : agn_close + _AGNOSTIC_SCAN_CAP], 2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Padding past the fixed window still hides the home operand

Cause: the shift re-read only sees body[:_AGNOSTIC_SCAN_CAP], and the exposed tail is cut to the next 256 characters. Mechanism: when the real ) sits past 256 characters of span body, the shift reading cannot prove an earlier close, so no tail is added; when the gap after the real ) is longer than 256 characters, the tail is cut before the operand. In both cases the primary heredoc reading still swallows the operand as body data, close < len(body), so _UnreadSpan is not raised either. Consequence: the gate allows a command that bash runs as a recursive forced removal of the home directory, and main refuses it.

agn_close, agn_proven, _, _ = _close_paren_walk(
    body[:_AGNOSTIC_SCAN_CAP], heredocs=False
)
if agn_proven and agn_close < close:
    try:
        tail = _segments(body[agn_close : agn_close + _AGNOSTIC_SCAN_CAP], 2)

Repro (NL is a real newline): rm -r -f "$(echo <150 x "a "> $((1<<X))NL)" ~ ; : 'NLXNL)', or the short span followed by 300 spaces before ~. Base: refused. f671e86: allowed. This head: allowed. Real bash with a printf in place of the command prints the home directory for both.

Suggestion: when the heredoc reading is unproven and the bounded shift reading either finds no proven close inside the window or its tail is cut by the window, fail closed (raise _UnreadSpan, or append the synthesized home operand as the capped span path already does). A per command budget instead of a per span window would also keep the scan linear while always reaching the real ). A test row with padding on each side of the real ) would pin it.

Rule: a security floor change must not allow any command main refuses; a bounded scan that runs out must fail closed.

@Maqix

Maqix commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Opus 5.5 finding on f671e86 (arithmetic << closing the span late inside a quote): confirmed and fixed in 3418e15, still one commit.

Root cause. In rm -r -f "$(echo $((1<<X))⏎)" ~ ; : '⏎X⏎)', _close_paren_walk recorded the <<X inside $(( as a pending heredoc X. The lines up to the later X line were skipped as body, so the span closed unproven at the ) inside the trailing single quote. That close is < len(body), so it was not the run-to-end case the floor already refuses, and _segments resumed after it. The ~ after the real ) was never segmented, and bash runs rm -r -f 1 ~.

Fix (rm_floor.py, +7 lines). An unproven span whose first << follows an arithmetic opener ((( or $[) now refuses the whole command, exactly like the run-to-end case: bash reads a shift there, so the walk's close can sit anywhere past the real ). A genuine heredoc span ($(cat <<EOF ...)) has no such opener and keeps its own reading.

I first tried a narrower reading (re-read the span with << as a shift and union in the operand it exposes). It had to scan a bounded window to stay linear, so a command that padded the span past the window slipped again. Refusing the shape closes it at any length.

Checks, against f671e86f5c and main 12419547e5:

  • Your command, / and $HOME operands, spaced and y = 1<<X forms, && / | / newline second rm, x="$(...)"; rm -r -f ~, and the same rows padded past 400 characters: all refused now, as on main. Each wipes home or / in real bash (stub rm).
  • Trade-off: a /tmp cleanup in this exact rare shape (an arithmetic << on a line inside a multi-line "$(...)", in an rm command) is refused too, as the run-to-end shape already is. Genuine heredoc /tmp twins stay allowed.
  • Every earlier probe set (c102 to c120x): 0 flips. 4,795 rm commands: 0 leaks, 0 flips. Push sweeps (6,688 + 2,268 + 20,328): 0 flips, 0 protected pushes allowed. 546 kill commands: same as main. Span timing still linear.
  • Tests: 1,914 pass, plus the 2 known markdown_it host-import failures. The new test fails on f671e86f5c and passes now. black / isort / flake8 / mypy clean. Budget 33,458.

Security Scope: its verdict line on f671e86 is PASS; the red is the "per-platform reports could not be folded" error, not a finding.

@iamwhatever @bolichen97 @cruisercohen could one of you approve the workflow runs on 3418e15 and re-run Security Scope?

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. I reviewed head 3418e15 against the previous head 57363ec (same base), with gate probes on main and on both heads, and real bash for every allow row below.

What changed: the 256 character shift re-read is gone. An unproven double quoted $( span whose first << follows an arithmetic opener ((( or $[) now refuses the whole command. That closes the padding hole from my last review: the short form, 300 characters of span body before the arithmetic, and 300 spaces before ~ are all refused on this head, as on main. The four touched security test files pass locally on this head (505 passed).

Two blocking items remain, inline below. Both allow a recursive forced removal of the home directory that main refuses, and real bash passes the home directory to the command in both shapes.

  1. The new check only looks at the FIRST <<. A real heredoc before the arithmetic shift steps over it (the Opus lane found the same shape).
  2. A leading redirection before an assignment with a capped nested span makes the floor synthesize ~ as the program word, so the real rm is skipped. This one was already present on 57363ec; I missed it last round.

Non blocking, both fail closed: an ordinary cleanup with an arithmetic expression before a real heredoc (rm -r -f "$(echo $((1)); cat <<X ... X ... )" /tmp/x) and the newline form rm -rf "$(echo $((1<<2)) newline )" /tmp/x are now refused, where main allows them. The single quoted empty delimiter (<<'') cleanup is still refused. Fine as a follow up.

The change request already on this pull request from the other reviewer is theirs to clear.

Comment thread src/kiro_crew/security/rm_floor.py Outdated
# as a heredoc body and may close at a ``)`` far past the real one
# (``"$(echo $((1<<X))⏎)" ~ ; : '⏎X⏎)'``), hiding the ``~``.
heredoc_at = body.find("<<", 0, close) if not proven else -1
if heredoc_at > 0 and _ARITH_OPEN_RE.search(body, 0, heredoc_at):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Only the first << is checked, so a real heredoc in front hides the arithmetic shift

Cause: heredoc_at is the first << in the span, and the arithmetic opener search stops there. Mechanism: in rm -r -f "$(cat <<E NL E NL echo $((1<<X)) NL )" ~ ; : ' NL X NL )' (NL is a real newline), the first << is a real heredoc with no opener before it, so the check passes. The walk then reads the later <<X inside $(( as a heredoc, skips to the X line inside the trailing quote and closes at the last ), so close < len(body) and the run to end check does not fire either. Consequence: the ~ after the real ) is read as body data and the command is allowed. Bash shifts, closes at the first ) and runs rm -r -f on the home directory. Main refuses it.

heredoc_at = body.find("<<", 0, close) if not proven else -1
if heredoc_at > 0 and _ARITH_OPEN_RE.search(body, 0, heredoc_at):

Suggestion: for an unproven span, refuse when an arithmetic opener precedes ANY << in body[:close], for example by searching up to body.rfind("<<", 0, close). That stays linear. A test row with a real heredoc before the shift, and one with padding after it, would pin it.

Rule: a security floor change must not allow any command main refuses.

Comment thread src/kiro_crew/security/rm_floor.py Outdated
continue
if not word_open:
word_open = True
word_is_assignment = bool(_ASSIGNMENT_WORD_RE.match(text, i))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 A leading redirection before the assignment turns the synthesized ~ into the program word

Cause: _in_assignment_value treats 2>/dev/null as an ordinary word, so for 2>/dev/null x="<40 nested $( ... )>" rm -r -f ~ it returns False. Mechanism: the nested span is capped and unproven, so the segment gets '\0" ~ "' instead of '\0'. _rm_wipe_targets then drops the redirection, skips x= as an environment assignment, and takes the synthesized ~ as the program word. Its basename is not rm, so the whole segment is skipped. Consequence: the real rm -r -f ~ after it is never checked. Bash treats a leading redirection as transparent, keeps x= as an assignment and runs rm -r -f on the home directory. Main refuses this; 57363ec and this head allow it, and without the 2>/dev/null prefix both refuse it.

word_is_assignment = bool(_ASSIGNMENT_WORD_RE.match(text, i))

Suggestion: step over a redirection word and its separate target here, the same way _rm_wipe_targets does, so an assignment after it is still seen as one. Or never synthesize ~ when the segment before the $( holds any assignment word. A test row with a leading redirection would pin it.

Rule: a security floor change must not allow any command main refuses.

@Maqix

Maqix commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Opus 5.5 findings on 3418e15: both confirmed and fixed in 05b0cb9, still one commit. Security Scope passes on 3418e15.

1. A real heredoc before the arithmetic <<. In rm -r -f "$(cat <<E⏎E⏎echo $((1<<X))⏎)" ~ ; : '⏎X⏎)' the check looked only at the span's first <<, which was the real heredoc, so the later shift still closed the span late and hid the ~. The check now refuses an unproven span when any << follows an arithmetic opener ((( or $[), not just the first one.

2. A leading redirection before a capped assignment. In 2>/dev/null x="$(<span past the nesting cap>)" rm -r -f ~, _in_assignment_value read 2>/dev/null as the command word, so the capped span's synthetic ~ went in front of the real rm and the home wipe was lost. It now steps over a word _shell._push_token_redirection recognises (and its separate target word), the same way _rm_wipe_targets does.

Checks, against 3418e15a0d and main 4d5d86af80:

  • Both triggers, with ~ / $HOME / / operands, the <<-E and assignment forms, and the redirections 2>/dev/null, 2> /dev/null, >/tmp/log, </dev/null, &>/dev/null: 24 rows, all refused now, as on main. Each wipes home or / in real bash (stub rm).
  • The /tmp twins of finding 2 stay allowed. The 3 /tmp twins of finding 1 are now refused, the same disclosed trade-off as the rest of this arithmetic shape.
  • Every earlier probe set, including last round's padded rows: 0 flips. 4,795 rm commands: 0 leaks, 0 flips. Push sweeps (6,688 + 2,268 + 20,328): 0 flips, 0 protected pushes allowed. 546 kill commands: same as main. Span timing still linear.
  • Tests: 1,914 pass, plus the 2 known markdown_it host-import failures. The extended tests fail on 3418e15a0d and pass now. black / isort / flake8 / mypy clean. Budget 33,468.

The new head needs its workflow runs approved.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. I reviewed head 05b0cb9 against the previous head 3418e15 (same base), with gate probes on main, on 3418e15 and on this head, and real bash (with rm replaced by a printer) for every home row below.

Both blocking items from my last review are fixed:

  1. The arithmetic check now looks for any << after an arithmetic opener in an unproven span, not just the first <<. A real heredoc before $((1<<X)) (also with 300 characters of padding, with two real heredocs first, and with $[1<<X]) is refused on this head, as on main. 3418e15 allowed all four; bash passes the home directory in each.
  2. _in_assignment_value now steps over a leading redirection and its separate target word. 2>/dev/null x="<capped span>" rm -r -f ~ is refused, and so are 2> /dev/null, {fd}>/dev/null, 2>&1, 2>"/dev/null" and an earlier a=1 before the redirection. 3418e15 allowed all of them. A redirection followed by a real command word, and a separate target that looks like an assignment (2> x="..." rm -rf ~), keep main's verdict.

The earlier padding row (300 spaces before ~) stays refused. The touched test files pass locally on this head (106 passed in test/test_push_branch_gate_parser.py and test/test_security_gate_liveness.py). The AI lanes and several CI jobs have not run on this head yet, so I have not relied on them.

Non blocking, all fail closed, fine as a follow up: these ordinary cleanups are still refused here where main allows them:

  • an arithmetic expression before a real heredoc: rm -r -f "$(echo $((1)); cat <<X newline d newline X newline )" /tmp/x
  • the newline form rm -rf "$(echo $((1<<2)) newline )" /tmp/x
  • the single quoted empty delimiter: rm -rf "$(cat <<'' newline d newline newline )" /tmp/x

I am dismissing my two earlier change requests since their findings are fixed. The change request from the other reviewer is still open and is theirs to clear.

@Maqix

Maqix commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

@bolichen97 thank you for the careful re-check and for dismissing the two earlier requests.

The Opus review and the Security Scope run on 05b0cb9d0f found three real problems. All three are fixed in abda6f78af. I reproduced each one on 05b0cb9d0f and on main, and ran the self-kill shapes in real bash.

Opus 1 and 2: a self kill hidden behind an arithmetic <<. In k=kill; x=$(echo $((1<<X))⏎); $k $(printf ')' ; pgrep -f <name>) ; : '⏎X⏎)', the heredoc reading closed past the decoyed $k $(...), and its bodies were dropped with the x= segment. Bash runs kill on the gateway's pids there. The ${y:-$(echo $((1<<2))⏎)} form hid it the same way, because _matching_close_brace never handed heredocs to its nested walks. Now:

  • _matching_close_brace takes heredocs (and the new report list below) and passes both to every nested walk.
  • When an arithmetic opener comes before a <<, the raw self-kill walk also runs with no heredocs, and both readings are searched.
  • All three shapes ($(, <(, ${) are refused, as on main. Their nginx twins stay allowed.

Opus 3 and Security Scope: a shift quoted in heredoc DATA was refused. The arithmetic check searched the span's whole text, heredoc body included. So a commit or PR message quoting $((1<<2)) was refused whenever the command named rm. Now _close_paren_walk reports each << it reads as a heredoc after an arithmetic opener, in shell source only. A << inside a skipped body reports nothing. The rm floor then also takes the heredoc-free reading:

  • Both readings close at the same ): no doubt, keep going.
  • The heredoc reading runs to the end: bash either shifts, or the heredoc never ends and bash stops on a syntax error that runs nothing (checked in bash 5.2). So the heredoc-free close is used, and the span's own text is read with no heredocs. An rm on a line after the shift is still a command.
  • Any other mismatch (the late close from the last round) still refuses the whole command.

All three confirmed Scope rows (the commit, the gh pr create body, and BYTES="$(⏎echo $((4 << 20))⏎)"⏎rm -rf ./dist) are allowed now, as on main. Your first two non-blocking rows are allowed too. The empty <<'' delimiter row is still refused; I left it for a follow-up, as you suggested.

One more hole I found while checking. Bash reads an array subscript as arithmetic (a[1<<X ]=1 and a=( [1<<X ]=1 ) in bash 5.2), so that << is a shift too. On 05b0cb9d0f, rm -r -f "$(a[1<<X ]=1⏎)" ~ ; : '⏎X⏎)' was allowed, and real bash wipes the home folder. main refuses it. Subscripts and =( [ now count as arithmetic openers, so it is refused again.

Checks, against 05b0cb9d0f and main:

Check Result
New probe (self kill, heredoc data, Scope rows, kept holes) Matches main on every row except the <<'' row (still refused) and x="$(echo $((1<<2))⏎rm -rf $HOME⏎)" (refused here, allowed on main; bash wipes home)
Array-subscript rows All home rows refused, as on main
Earlier probes (c102 to c125) Only /tmp twins move, denied -> allowed, each matching main
rm corpus (4,795) 0 changes, 0 home or root rows allowed that main refuses
Push sweeps (6,688 + 2,268 + 20,328) 0 changes, 0 protected pushes allowed
Kill corpus (546) Identical to main
Span timing Still linear (0.83 s at 5.8 KB)
Tests 1,918 pass, plus the usual 2 markdown_it host-import failures. The new tests fail on 05b0cb9d0f

One row moves the other way and I want to name it. In c120x, c=${0:0:0}rm; $c -rf ~ after or inside one of these spans was refused only because the whole command was unreadable. It is allowed now. main allows it too, and so does this head and main with no span at all: the rm floor does not resolve ${0:0:0}rm. That gap predates this PR.

Trade-off, unchanged in kind: a /tmp cleanup whose arithmetic << closes late inside a multi-line "$(...)" is still refused.

The _PACKAGE_LINE_BUDGET is re-pinned to the measured 33,523. The PR description is updated.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. I compared head abda6f7 with the previous head 05b0cb9 (git range-diff: the base moved, and the security commit itself changed) and re-ran the earlier gate probes on main, on 05b0cb9 and on this head, with real bash (rm replaced by a printer) for every home and root row.

No regression found:

  • Every earlier home and root row stays refused, as on main: the arithmetic << shapes ($((, ((, $[, an array subscript), a real heredoc or two before the shift, the padding rows, the redirection before a capped assignment rows, and ${y:-$(...)}.
  • The self kill row with an arithmetic shift before a decoyed $k $(...) is refused on this head and on main. 05b0cb9 allowed it, so the new heredoc free walk closes a gap I had not caught.
  • A few extra shapes (a heredoc body holding ) or a single quote after an arithmetic opener, a=(1) before a shift, let, a for ((...)) header) are refused on this head. Bash passes the home directory in each. main allows the first two.

Two of my three earlier non blocking items are fixed. These cleanups are allowed again, as on main: an arithmetic expression before a real heredoc, and the newline form rm -rf "$(echo $((1<<2)) newline )" /tmp/x. The BYTES="$(...)" example from the commit message is allowed too.

Still refused where main allows it (fails closed, fine as a follow up): the single quoted empty delimiter rm -rf "$(cat <<'' newline d newline newline )" /tmp/x, and the late close shape the commit message names as its trade off.

The touched test files pass locally on this head (509 passed). CI and the AI lanes are still running on this head, so I have not relied on them. The other reviewer's change request is still open and is theirs to clear.

@Maqix

Maqix commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

CI on abda6f78af found one real failure of mine, now fixed in 536d7e80d9 (same single commit, amended):

  • test_facade_mirror_guards.py::test_every_mirrored_facade_manifest_names_what_it_forwards (Backend Tests shard 2): the argv_floor facade mirrors argv_floor_self, and the two helpers the last revision added there (_ARITH_CONTEXT_RE, _arith_shift_possible) were missing from its import manifest. They are listed now. No logic change; _PACKAGE_LINE_BUDGET is +2 (33,525, measured).
  • I missed it locally because that test module imports markdown_it, which my test venv lacked (the "2 markdown_it failures" I kept reporting). With it installed, all 69 facade-mirror tests pass, plus 9,058 security / push / argv-floor / denied-rule tests.

The AI reviews on abda6f78af all passed (Opus, Design, First Principles, Security Scope; UX skipped). The GPT 6.1 lane ended "review incomplete" with no verdict, so it will need the next run.

…osed on arithmetic

_substitution_bodies proved each $( / <( / >( span by walking the paren
matcher from the start of the whole command, so the quote state of the
enclosing word leaked into the body. The fresh-context walk fixes the
simple "$(date)" case but, on its own, still mis-delimited a body whose
own interior re-establishes quoting:

* a $( nested inside the double-quoted word -- the inner " flipped the
  walk's state, so echo "$(echo "$(echo ")")" ; bash -c '<publish main>')"
  closed the span at the first ) after the inner " and the bash -c
  behind it ran unscanned;
* a ${...} default value carrying a ) -- "${x:-")"}";
* a heredoc body line holding a ) -- $(cat <<'E'\n)\nE\n ... ).

The matcher skips a nested $( / <( / >( and a ${...} (new
_matching_close_brace) as units, so their inner quotes and parens stay
sealed inside them and the enclosing span closes at its own matching ).

A heredoc is not parsed: a << only marks one pending, and a newline
reached while one is pending (in this span, or handed up from a nested
one) leaves the span unproven, so the whole remainder is scanned. The )
before that newline (the close in $(cat <<E)) still proves the span, and
the index of an unproven span is where the walk closes reading the body
lines as commands, so the rm floor still sees the words after it. A
here-string <<< is not a heredoc. Nesting past _MAX_SPAN_NESTING is read
without unit skipping (unproven), so a deep ${a:-${a:-...}} chain cannot
raise RecursionError.

Arithmetic is not modelled, by maintainer decision: $(( and (( are read
as plain parens, so a << shift inside them counts as a heredoc like any
other <<. A shift followed by a newline before the span closes therefore
leaves the span unproven and scanned whole -- the fail-closed direction.
The (( 1<<2 )) cases are expected refusals, documented as such. Dropping
the arithmetic seed also removes the per-opener/second-walk cost it
carried, so the span walk is a single linear pass.

A capped span whose close is a GUESS is unreliable in BOTH directions. A
data ) read at the cap can close it LATE (at a sibling's )) -- scanned
once past the close already -- or EARLY (one ) too soon, hiding the
span's own later ; bash -c '<push>' after the guessed close). The text
past a capped span's close is now scanned once per walk for its
substitution segments, so a launcher an early close hid is still read,
and a run of capped spans stays linear.

The rm floor's capped-span home-operand fail-closed (reading a capped
$(...) output as ~) is applied only in OPERAND position -- after a
command word has completed -- not when the span is a NAME= assignment
value, a redirection operator, or a redirection target. In each of those
the synthetic ~ landed before a real rm and masked a home wipe:

* x="$(<capped>)" rm -rf $HOME -- the ~ went between the assignment and
  the rm, read as the command ~ with rm as its argument;
* >"$(<capped>)" rm -fr ~ -- the ~ landed in the leading redirection
  target, which _rm_wipe_targets steps over, taking ~ as the program and
  skipping the segment so the real rm -fr ~ after it was never read.

Position is now decided from word structure (_rm_operand_position),
quote-aware, ordering the simple command as bash does (assignment
prefixes, then redirections with their glued or separate targets, then
the command word, then operands); the synthetic ~ is appended only when
the open word holding the $( is an operand after a completed command
word. This replaces a start-anchored opener regex that matched only when
NAME=" began the segment, so a separator-cut leading space (true;
x="..."), an earlier completed assignment (a="p q" x="..."), a partial
value (x="a$(...") and += slipped past it; and it closes the redirection
-target hole the not-an-assignment check left open. All are read as the
prefix they are now; the real rm is seen and main refuses it too. +=
stays at main's verdict (main's env-strip leaves x+= in place).

bash performs no expansion on a heredoc DELIMITER word, but it does not
treat a $(...) or backtick in it as word-ending metacharacters -- the
whole construct is the delimiter (bash 5.2: <<$(echo E) ends the body at
a line $(echo E), not $, while a bare <<a(b ends the word at the (). The
delimiter parser broke at the (, left the delimiter as $, and read the
)" <target> operator-line tail as body, closing the enclosing span early
and hiding the rm target after it. Reading the construct to its real end
costs a balanced walk at EACH <<, so echo $((x<<$((x<<... (a shift run,
every << seen by the parser) would go quadratic. So a delimiter opening a
$( substitution or a backtick now leaves the span unread and the floor
refuses it, fail-closed; a plain ${NAME} delimiter has no word-ending
char and is still read whole, unchanged. The home rows refuse as on main;
a rare /tmp twin of the $( form is over-refused, the disclosed cost.

A command substitution whose heredoc body holds an UNTERMINATED backtick
left the span unproven AND made the backtick scan swallow the rest of the
command, so the walk never reached the real ) and the enclosing rm
segment never saw the home operand after it: rm -rf "$(cat <<E` + nl + `
+ nl + `E` + nl + `)" ~ wiped $HOME while the floor allowed it. The span
walk's unterminated-backtick branch now steps over just the one broken
backtick and keeps scanning (marking the span unproven) instead of
jumping to the end, so the span resumes at the real ) and the ~/$HOME
after it reaches the rm floor, or fails closed on a home operand. The
/tmp twin stays allowed, as on main.

The same heredoc-span raw walk in argv_floor_self._bare_kill_raw_bodies
resumes past an unproven span's close (both the $( and <(/>( branches)
instead of ending it, so the decoyed-alias clause the raw walk alone
catches is reached, with the whole-remainder over-read emitted once to
keep a heredoc run linear.

A ' / " / ANSI-C $' on a heredoc BODY line is printed stdin data, not a
live quote, but the span walk let an unpaired one open a quote whose
state ran PAST the span's real ) -- so a home operand after the span
(rm -rf "$(cat <<E` + nl + `it's` + nl + `E` + nl + `)" $HOME) never
reached the rm floor and a real wipe was allowed. The walk now reads a
body-line quote that does not close on its own line as data, like the
backtick and opener branches, so the span closes at its real ) and the
operand after it is read: every direct, nested and post-terminator
home/root wipe the parent allowed now refuses, and a /tmp twin stays at
main's verdict because the REAL operand is read rather than synthesised.

A body-line quote that DOES close on its own line keeps the paired
reading, so its interior stays quoted. Reading only the two quote
characters as data left a body `")"` interior live: its ) closed the
span early and the line's closing " then opened a quote over the operand
behind the span (rm -rf "$(cat <<E` + nl + `")"` + nl + `E` + nl + `)" ~).
Those rows refuse again, as on main.

The nested-payload walk also no longer re-extracts a span-walk OVER-READ:
its text is scanned whole and its real nested openers are emitted by the
structural walk, so re-walking each near-full remainder of a run of
nested heredoc spans -- an O(n^2) that stalled the gate (47s -> 0.7s at
n=200) -- buys no finding. A liveness test pins near-linear (~2x per
doubling) growth.

Regression rows (fail on the prior head, pass here) cover: the early
capped-close launcher, the arithmetic shift expected-refusals, the
separator/prefix NAME= assignment-value home-rm bypass, the
redirection-target capped-span home-rm bypass, the substitution
heredoc-delimiter home-rm bypass, the heredoc-backtick masked home-rm
bypass, the heredoc-body quote family, and the body-line paired quote
around a ). No push or kill verdict moved
beyond the documented feature-push rows (probe/nested/c97/kill sweeps);
0 protected pushes allowed; one commit over origin/main.

The heredoc body is now read STRUCTURALLY, which ends the per-character
patching of body lines. Once a << operator's delimiter is parsed, every
line of its body is skipped as DATA -- no quote, opener, backtick, paren
or depth state -- until the terminator line (<<- strips leading tabs;
<<'E' / <<"E" / <<\E unquote the delimiter), and the walk then resumes
normal reading. Several body-line characters still mis-sized the span
one at a time: a ) inside a nested quoted substitution closed the inner
span early, and an unclosed ${ / $( / $(( / ( / backtick later on the
same body line ran one unproven span to the end of the text, hiding the
home operand after the real close; and an ANSI-C $'a\'b was read as
closing on its line while the walk kept it open. Reading the whole body
as data fixes the family at once: the span stays unproven (the push
floor still over-reads the body), but its close INDEX is the real ),
so the rm floor reads the operand after it. The _heredoc_delimiter
parser and the _skip_heredoc_bodies line skipper replace the
_quote_closes_on_line helper and the per-character body-quote,
body-backtick, body-opener and bare-( branches.

A span the reader could not prove that also runs to the END of its text
-- an unparsable delimiter, or a heredoc with no terminator line -- now
fails closed on a home/root operand in the rm floor, beside the capped
span it already covered. Every rm verdict that moves is allow->deny and
hands $HOME or / to rm in real bash (c109/c104/c103 probes, confirmed
with stub binaries); each /tmp twin stays at main's verdict. No push or
kill verdict moved (c97 and kill sweeps flat; 0 protected pushes
allowed), and the nested-heredoc run stays linear (~2x per doubling).

Three more span readings let a command through that bash runs. A $(...)
left unproven by a HEREDOC still closes, and a process substitution (or a
nested $(...) / backtick) bash runs can sit after the body inside it --
echo $(cat <<E...E...cat <(bash -c '<publish>')) ran the inner command
while the gate scanned the text but never emitted that opener as its own
payload, so the publish (and its self-restart and self-kill twins) was no
source and the floor never ran. The span's proven-close body is emitted
NON-suppressed now, bounded to the one span, so the payload walk reaches
the nested opener while a heredoc run stays linear. A ' inside a
double-quoted assignment prefix (MSG="it's" rm -rf ...) desynced the
quote tracking in the assignment-value check, which then read the unproven span
as an assignment VALUE and dropped the fail-closed home operand; double
quotes are tracked now, so the rm is seen as a command and its home wipe
refused. And a heredoc delimiter written with ANSI-C or locale quoting --
<<$'E' / <<$"E" -- was read as the literal $E, so its body never
terminated and a harmless rm -rf "$(cat <<$'E'...E...)" /tmp cleanup was
over-refused; the delimiter decodes the quote now, so the span closes and
the /tmp twin returns to main's verdict while a home operand after it is
still refused.

A span that never closes -- an arithmetic << before a newline is read
as a heredoc with no terminator line, so the span runs to the end of
the text -- had no close to bound a body by, and every reading of it was
a suppressed over-read, so a <(...) / $(...) / backtick command after
the newline was no payload source. When an opener sits in such a
remainder, it is now scanned once, flat, as the capped never-close case
is, and the walk ends there, so a run stays linear.

In the rm floor, a double-quoted $( span the reader could not close
before the text ended -- an arithmetic << before a newline, or a heredoc
with no terminator -- made _segments resume at the end of the command,
so no later ;/&&/|/newline-separated command was ever produced as a
segment: bash closes "$(echo $((1<<2))
)" and runs the following rm -r -f $HOME. Such a span now refuses the
whole command through the floor's existing unread-command refusal. A
span closed at a guess past the nesting cap keeps segmenting after the
guess and its output is still read as a home operand.

Inside a substitution bash also ends a heredoc body at a line that
starts with the delimiter glued to the closing paren (EOF)", a common
commit-message spelling). That line now ends the body, so the span
closes there instead of running to the end, and an ordinary heredoc
commit message holding "confirm" or "format" is no longer refused by the
rm floor. The self-kill raw walk no longer ends at a span the heredoc
rule never closes: it resumes at the close a heredoc-agnostic walk finds,
so a later sibling $k $(...) kill is still walked.

The flat never-close pass read quotes from a neutral state with no
heredoc model, so a real heredoc in that remainder whose body line holds
an unbalanced quote (it's, or $'x\'y) opened a quote over the rest of
the text and a later <(...) / >(...) self restart or self kill was no
payload source, while bash runs it. The remainder is now also scanned
quote-blind: every opener, backtick and ) is a boundary, so no body
quote can swallow an opener. Its runs stay disjoint, so a run stays
linear.

A double-quoted heredoc delimiter is unquoted the way bash does it.
Inside "..." (and $"...") bash drops a backslash only before $, a
backtick, ", \ or a newline, so <<"E\q" ends at a line E\q. The span
reader dropped every backslash and read it as Eq, so a body line Eq)"
closed the span early and the quote state after it hid a later home
operand that bash still hands to rm.

An arithmetic << read as a heredoc could also close the span LATE inside
a quote and hide the operand after the real ). In
rm -r -f "$(echo $((1<<X))\n)" ~ ; : '\nX\n)' the <<X inside $(( was
recorded as a pending heredoc X, the body lines up to the later X line
were skipped as data, and the span closed UNPROVEN at the ) inside the
trailing single quote -- < len(body), so it was not the run-to-end case
that already refuses. The ~ after the real ) was skipped as heredoc data
and never segmented, so bash's rm -r -f 1 ~ slipped.

The span walk now reports each << it reads as a heredoc after an
arithmetic opener in shell SOURCE: $((, ((, $[, an array subscript a[
or a compound assignment's =( [ (bash reads a[1<<X]=1 as arithmetic,
bash-measured). A << quoted inside a skipped heredoc body is data and
reports nothing, so a commit or PR message that quotes $((1<<2)) keeps
main's verdict. For a reported span the rm floor also takes the
heredoc-free reading. Same close: no doubt. Heredoc reading runs to the
end: bash either shifts there or, were it a heredoc, never finds the
terminator and stops on a syntax error that runs nothing (bash-measured),
so the heredoc-free close is used and the span's own text is read with
no heredocs (an rm on a line after the shift is still a command). Any
other mismatch, like the late close above, refuses the whole command,
however much text comes between. So "$(echo $((1<<2))\n)" ; rm -r -f
/tmp/x and BYTES="$(\necho $((4 << 20))\n)"\nrm -rf ./dist keep main's
verdict, while the home and root rows stay refused. Any << after the
opener counts, so a real heredoc first ("$(cat <<E\nE\necho
$((1<<X))\n)" ~) does not hide the later shift. The trade-off: a /tmp
cleanup whose shift closes late in that rare shape is refused too.

The self-kill raw walk had the same blind spot: in
k=kill; x=$(echo $((1<<X))\n); $k $(printf ')' ; pgrep -f <name>) ; : '\nX\n)'
the heredoc reading closed past the decoyed $k $(...), whose bodies fell
to the x= segment and were dropped. When an arithmetic opener comes
before a << the walk now also runs with no heredocs and both readings
are searched. The brace walk now hands heredocs (and the report) to its
nested walks, so ${y:-$(echo $((1<<2))\n)} no longer runs the
heredoc-free reading to the end.

A leading redirection is applied before the command, so the assignment
check now steps over it (and its separate target word) the way the
command-word reader does. 2>/dev/null x="$(<capped span>)" rm -r -f ~
had read 2>/dev/null as the command word, put the capped span's
synthetic ~ before the real rm, and lost the home wipe.
@Maqix

Maqix commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Both findings on 536d7e80d9 confirmed and fixed in fefe107439, still one commit. I reproduced each on 536d7e80d9 and on main 9d2df153d2 and ran the home/root rows in real bash with a stub rm.

1 — rm_floor.py:431, capped span as a leading redirection target. Confirmed. >"$(<33-deep>)" rm -fr ~ (and > "…", 2>"…", <"…", x=1 >"…", >"…" x=1, ; >"…") inserted the synthetic ~ because the open word >" is not an assignment, it landed in the redirection target, _rm_wipe_targets stepped the > over it and took ~ as the program, and the real rm -fr ~ was skipped. Fix: the synthetic operand is now gated by _rm_operand_position instead of the not-an-assignment check — it goes in only when the open word holding the $( is an OPERAND after a completed command word, ordering the simple command as bash does (assignment prefixes → redirections with their glued/separate targets → command word → operands). So a redirection operator, its target, and the assignment-prefix run all get a plain placeholder and the real rm is read.

2 — shell_normalizer_span.py:1190, substitution heredoc delimiter. Confirmed. rm -fr "$(cat <<$(echo E)⏎$(echo E)⏎)" ~ ; …: the delimiter parser broke at the (, left the delimiter as $, and read the )" ~ operator-line tail as body, so the span closed early and the ~ was never segmented. bash keeps the whole $(echo E) as the delimiter (measured on bash 5.2: <<$(echo E) ends the body at a line $(echo E), not $; a bare <<a(b ends the word at the (). I took your option 1 (return None): reading the construct to its real end would need a balanced walk at EACH <<, which echo $((x<<$((x<<… turns quadratic (every << is seen by this parser), and the liveness guard test_a_run_of_arithmetic_openers_holding_a_shift_is_fast catches it. So a delimiter opening a $( or a backtick now leaves the span unread and the floor refuses it. A plain ${NAME} delimiter has no word-ending char and is still read whole, so it is unchanged.

Checks, against 536d7e80d9 and main 9d2df153d2:

  • A 113-row probe (F1 redirection-target × 33/40-deep caps × ~///$HOME//tmp × three rm spellings; F2 $(echo E)/${E}/backtick/x$(e) delimiters × the same operands): 0 home/root rows this head allows that main refuses. 45 rows flip allowed→denied vs 536d7e80d9 (42 F1 + 3 F2), each a real $HOME or / wipe in bash.
  • Trade-off: the one $(echo E) /tmp twin is over-refused (the unread-delimiter fail-closed), the same family as the backtick / x$(e) /tmp twins already refused. The capped span as a plain rm operand still fails closed, unchanged.
  • Every earlier probe (c102…c126_sub2): 0 flips vs 536d7e80d9. rm corpus 4,795: 0 home/root leaks, 0 flips. Push sweeps 6,688 + 2,268: 0 flips, 0 protected pushes allowed. Kill corpus 546: identical to main. Span timing linear and the arithmetic-shift liveness test passes.
  • Tests: 1,643 security / push / argv-floor / denied-rule pass + 69 facade-mirror; the two new tests fail on 536d7e80d9 and pass here. black / isort / flake8 / mypy clean. Budget 33,554 (measured). Description updated.

This branch has not been deployed

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

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants