Repository navigation
Conversation
UX Review (Opus 5.5, fork) — ⏭️ skippedRevision |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the bypasses are real, the parsing input really is a shell command line, and every caller of the changed Notes
[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 |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks, but the arithmetic Notes
What this change shipsInventory (10 items) — 6 justifiedIntent: let ordinary
[FIRST-PRINCIPLES-REVIEWED] 536d7e8 Description read[DESCRIPTION-READ] 30c2774bbb24ba74d16bc21518f597b0c1c2bc118e2d51adc351f8426f285907 That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review (fork) — 🔴 changes requested (blocking)Reviewed Blocking: two changed paths in the rm floor now allow a recursive-force home wipe ( BLOCKING — src/kiro_crew/security/rm_floor.py:431 BLOCKING — src/kiro_crew/security/shell_normalizer_span.py:1190 |
GPT 6.1 Review (fork) — 🔴 changes requested (blocking)Reviewed 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
BLOCKING -- src/kiro_crew/security/shell_normalizer_span.py:641 -- Valid heredoc delimiters refuse harmless cleanup
[BLOCK-MERGE] 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 |
Security Scope Review (fork) — ✅ PASSWhich legitimate operations does 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 TIGHTENING: the shell span reader ( 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 [SCOPE-REVIEWED] abda6f7Security scope review -- adjudicated candidates
No candidate flipped from allowed to refused. |
183fd1d to
2e9c4b5
Compare
cruisercohen
left a comment
There was a problem hiding this comment.
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:
- Here-string.
_heredoc_redirectreturns None at the first<of<<<(line 2408), but the walk then reaches the second<, where<<xmatches. The span then runs to a later line that readsx. - Arithmetic shift. In
(( 1<<2 )), the<<2is a shift, but it is noted as a heredoc tagged2.
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_parencounted 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_BUDGETpin), and CI has not run on this head.
2e9c4b5 to
c772cac
Compare
cruisercohen
left a comment
There was a problem hiding this comment.
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.2e9c4b52f1returned(18, True). - Arithmetic: inside
((…)), and for a$((body, no heredoc is read." (( 1<<2 ))\n) ; P\n2\n)"returns(13, True).2e9c4b52f1returned(21, True). TestALessThanPairThatIsNotAHeredocKeepsTheSpanpins 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, andgit 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 recordinggitstub 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.
|
Thanks for the quick re-review, and for checking each half is load-bearing. For the record, the fix in c772cac:
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 Agreed on scope: #20845's heredoc commit form is still refused, so this stays 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. |
c772cac to
8ff800a
Compare
8ff800a to
945e463
Compare
945e463 to
75ec9cc
Compare
|
@bolichen97 thanks for the re-review on
Checked against
One change to be upfront about: 6 push commands of one shape, @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
left a comment
There was a problem hiding this comment.
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
mainrefused now refuses, and the temp directory twins keep the verdictmaingives. - 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
mainin a bare worktree because the package is not installed there.
Still blocking (security path, details sent privately as before):
- 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,
mainrefuses them, and bash does delete the home directory. The set was also present on the previous head; my last pass did not cover it. - 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,
mainrefuses, 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 |
There was a problem hiding this comment.
🔴 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 |
There was a problem hiding this comment.
🔴 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 1Suggestion: 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.
|
@bolichen97 thanks for the re-review on
Checked against
New tests: A maintainer will need to approve the fork workflows for the new head. Thanks! |
bolichen97
left a comment
There was a problem hiding this comment.
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
mainrefuses, and the temporary directory twins keep the verdictmaingives. The rm, push and self kill sweeps show this head is at least as strict asmainon every row, and it closes several rowsmainstill 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
mainin 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.
-
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
mainrefuses 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. -
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
mainallows. 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
left a comment
There was a problem hiding this comment.
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
mainrefuses, and on the rm, push and self kill sweeps this head is at least as strict asmaineverywhere. - 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
mainin 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.
|
Thanks to the Opus 5.5 and GPT 6.1 reviews on
Each finding has a regression row (they fail on Checked against
One note: A maintainer will need to approve the fork workflows for the new head. Thanks! |
bolichen97
left a comment
There was a problem hiding this comment.
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
mainrefuses 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 verdictmaingives. - 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
mainin 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.
- 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
mainrefuses 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.
|
Thanks to the Opus 5.5 review on Fixed on Checked against
One note: a long run of nested A maintainer will need to approve the fork workflows for the new head. Thanks! |
bolichen97
left a comment
There was a problem hiding this comment.
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
mainrefuses 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 verdictmaingives. - 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.
- 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 whilemainrefuses 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)) |
There was a problem hiding this comment.
🔴 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.
|
Thanks to the Opus 5.5 review on Fixed in What I checked:
New test: 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
left a comment
There was a problem hiding this comment.
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 matchmain. - 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.pyandtest_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.
-
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
mainrefuses them, and real bash runs both. Inline comment 1. -
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 thatmainrefuses, 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 againstmain. 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
rmsubstring 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 thatmainand the previous head allow and this head refuses. Requiring a realrmcommand 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
mainat 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 thanmain.
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)) |
There was a problem hiding this comment.
🔴 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 |
There was a problem hiding this comment.
🔴 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 + relSuggestion: 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.
| # 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) |
There was a problem hiding this comment.
🟡 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.
|
Thanks to the Opus 5.5 and Security Scope reviews on Opus: self-kill after a span that never closes. Reproduced: Security Scope: Also fixed: the two files Verification against
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 |
bolichen97
left a comment
There was a problem hiding this comment.
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.
|
Status on Security Scope. The red check is a fold failure ("the per-platform reports could not be folded"); the review's own verdict is
The refusal comes from the arithmetic Could a maintainer re-run Security Scope Review so its report folds? @cruisercohen your CHANGES_REQUESTED is on |
bolichen97
left a comment
There was a problem hiding this comment.
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.
|
@bolichen97 thanks for the re-review on Cause. This is the never-close flat pass from your inline note on Fix. The never-close remainder is now also scanned quote-blind ( 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 (
Checked on
Could you rerun your private rows on |
bolichen97
left a comment
There was a problem hiding this comment.
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.
|
Pushed GPT 6.1 — double-quoted heredoc delimiter (upheld, fixed). Reproduced on
New test: @bolichen97 thank you for re-running everything on @cruisercohen your changes-requested review is on Could a maintainer please approve the workflow runs on |
bolichen97
left a comment
There was a problem hiding this comment.
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.
|
Rebased onto Conflict. Main #23292 ( One behaviour change from the rebase, matching
In real bash with stub Checks on
Could a maintainer approve the runs on the new head? |
bolichen97
left a comment
There was a problem hiding this comment.
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.
| ) | ||
| if agn_proven and agn_close < close: | ||
| try: | ||
| tail = _segments(body[agn_close : agn_close + _AGNOSTIC_SCAN_CAP], 2) |
There was a problem hiding this comment.
🔴 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.
|
Opus 5.5 finding on f671e86 (arithmetic Root cause. In Fix ( I first tried a narrower reading (re-read the span with Checks, against
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
left a comment
There was a problem hiding this comment.
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.
- 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). - A leading redirection before an assignment with a capped nested span makes the floor synthesize
~as the program word, so the realrmis 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.
| # 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): |
There was a problem hiding this comment.
🔴 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.
| continue | ||
| if not word_open: | ||
| word_open = True | ||
| word_is_assignment = bool(_ASSIGNMENT_WORD_RE.match(text, i)) |
There was a problem hiding this comment.
🔴 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.
|
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 2. A leading redirection before a capped assignment. In Checks, against
The new head needs its workflow runs approved. |
bolichen97
left a comment
There was a problem hiding this comment.
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:
- 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. _in_assignment_valuenow steps over a leading redirection and its separate target word.2>/dev/null x="<capped span>" rm -r -f ~is refused, and so are2> /dev/null,{fd}>/dev/null,2>&1,2>"/dev/null"and an earliera=1before 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 <<XnewlinednewlineXnewline)" /tmp/x - the newline form
rm -rf "$(echo $((1<<2))newline)" /tmp/x - the single quoted empty delimiter:
rm -rf "$(cat <<''newlinednewline 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.
|
@bolichen97 thank you for the careful re-check and for dismissing the two earlier requests. The Opus review and the Security Scope run on Opus 1 and 2: a self kill hidden behind an arithmetic
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
All three confirmed Scope rows (the commit, the One more hole I found while checking. Bash reads an array subscript as arithmetic ( Checks, against
One row moves the other way and I want to name it. In c120x, Trade-off, unchanged in kind: a The |
bolichen97
left a comment
There was a problem hiding this comment.
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, afor ((...))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.
|
CI on
The AI reviews on |
…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.
|
Both findings on 1 — 2 — Checks, against
|
Problem / Motivation
Goal: a
$(...)inside a double-quoted word is extracted for nested scanning at the span bash actually runs, so a feature push aftergit commit -m "$(date)"is allowed and a push hidden inside or after the body is refused._substitution_bodies(the nested-payload extractor, now inshell_normalizer_span.py) proved each$(/<(/>(span by walking_matching_close_parenfrom 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. Onmain@ 574cb18:Three interior constructs each open their own quoting/parsing context, so a
)or"inside them is data, not the close of the enclosing$(:$(inside the double-quoted word — the inner"flips the walk's state, the span closes at the first)after it, and thebash -c '<push main>'behind it is never scanned (the Opus-review BLOCK);${...}default value carrying a)—"${x:-")"}";)—$(cat <<'E'\n)\nE\n ... ).bash runs the push for all three (verified in a subprocess with a fake
gitonPATHthat records its args).This revision (base
origin/mainb9ff59ab5e, headfefe107439)One commit, rebased onto
origin/main@b9ff59ab5e(main #23292 also edits the rm floor: the conflicts inrm_floor.py, itssecurity.mdparagraph and_PACKAGE_LINE_BUDGETkeep both sides -- main's backtick-body and glued-redirection reads and its_names_rm_wordfloor-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/tmpverdict beyond what each fix requires.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.An apostrophe in a double-quoted assignment prefix desynced the fail-closed home operand.
_in_assignment_valueopened a single quote on a'even inside a double-quoted word, soMSG="it's" rm -rf "$(…)" ~read the unproven span as an assignment VALUE and dropped the synthetic home operand, allowing the$HOMEwipe. Double quotes are tracked now, so thermis seen as a command and its home wipe refused;mainrefuses it too.An ANSI-C / locale heredoc delimiter was read as the literal
$E.<<$'E'(and<<$"E") has delimiterEafter quote removal, so the body terminates and the span closes; reading it as$Eleft the span unproven to end of text and over-refused a harmlessrm -rf "$(cat <<$'E'+ newline +/tmp/x+ newline +E+ newline +)" /tmp/ycleanup._heredoc_delimiterdecodes the quote now, so the/tmptwin returns tomain's verdict while a home operand after the same, now-proven span is still refused.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 vs7cd2aa735e: 0 changes (6,688 + 2,268 + 20,328); 0 protected pushes allowed; rm and kill corpora unchanged.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_segmentsresume at the end of the command, soecho "$(echo $((1<<2))+ newline +)" ; rm -r -f $HOME(and&&/|/ newline,~,${HOME},/, anx=orrm -flead) was never read: bash closes the span and runs therm. 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 thatmainrefuses (60 are real home or/wipes in bash; the||rows never reachrm); all 90 are refused now, including 15/tmptwins, since the span cannot be read. 4,795-command rm corpus: 0 home/root rows head allows thatmainrefuses; 27 rows move allowed→denied vs94a81f2fce, all allowed onmaintoo (a deepx="$(…${x:-(}…)" rm -fr …chain): 24 are real home or/wipes, 3 are/tmpover-refusals. Push sweeps vs94a81f2fce: 0 changes (6,688 + 2,268 + 20,328), 0 protected pushes allowed; kill corpus identical tomain; every earlier probe identical to the prior head. Gates: 1,894 passed, plus the 2markdown_ithost-import failures.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;mainrefuses 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; thenginxtwin stays allowed.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 ordinarygit commit -m "$(cat <<'EOF'+ newline +fix: confirm the terminal format+ newline +EOF)"(it holds a literalrminconfirm/format). A line that starts with the delimiter followed by)now ends the last pending body;'EOF',EOF,"EOF"and<<-forms return tomain'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
631c965cc2andmaind2417a48c1: c115 probe (36 rows) identical tomainexcept theEOFX)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 tomain; span timing still linear (0.82 s at 5.8 KB). Gates: 1,896 passed, plus the 2markdown_ithost-import failures.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 on94a81f2fcethroughef9fb000ed). 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
ef9fb000edandmain7d63942c4c: 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 /nginxtwin) has 0 rows this head allows thatmainrefuses (48 before, 24 of them run the restart or kill in real bash with stubs), and every twin keepsmain'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 tomain; 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 2markdown_ithost-import failures.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 lineE\q(measured on bash 5.2)._heredoc_delimiterdropped every backslash and read it asEq, so a body lineEq)"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 handsrmthe home directory (GPT 6.1, review on3a84d0827c). Both quote forms now unescape with bash's double-quote rules (_dq_unescape).An arithmetic
<<that closes the span LATE inside a quote hid the operand after the real). Inrm -r -f "$(echo $((1<<X))+ newline +)" ~ ; : '+ newline +X+ newline +)'the<<Xinside$((was recorded as a pending heredocX, the body lines up to the laterXline 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'srm -r -f 1 ~slipped (Opus review onf671e86f5c). 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, so2>/dev/null x="$(<capped>)" rm -r -f ~keeps itsrm(both Opus findings on3418e15a0d). 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/tmptwins): 0 home/root rows head allows thatmainrefuses; every row that moves allowed->denied vsf671e86f5cis a real home or/wipe, or a/tmptwin of this rare arithmetic shape (the disclosed trade-off, as for item 5); genuine-heredoc/tmptwins stay allowed. Every earlier probe (c102/c103/c104/c106/c109/c111/c113/c114/c115/c116/c119/c120x) is 0 flips vsf671e86f5c; 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 tomain; span timing linear. Gates: 1,914 passed (only the 2markdown_ithost-import failures); black / isort / flake8 / mypy clean.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 namedrm(Opus and Security Scope on05b0cb9d0f;mainallows those)._close_paren_walknow reports (arith_hits) each SOURCE<<it reads as a heredoc after an arithmetic opener --$((,((,$[, an array subscripta[or=( [(bash readsa[1<<X ]=1as arithmetic, bash-measured; that subscript shape hid~on05b0cb9d0f,mainrefuses 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. SoBYTES="$(⏎echo $((4 << 20))⏎)"⏎rm -rf ./distandrm -rf "$(echo $((1<<2))⏎)" /tmp/xkeepmain'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_bracehandsheredocsto its nested walks.Verification of 11 vs
05b0cb9d0fandmain4d5d86af80: c126 probe (self kill x3 +nginxtwins, heredoc-data commit / PR bodies, the 3 Scope rows, bolichen97's 3 rows, 24 kept home/root rows) matchesmainexcept the<<''empty-delimiter/tmprow (still refused, left for a follow-up) andx="$(echo $((1<<2))⏎rm -rf $HOME⏎)"(refused here, allowed onmain; bash wipes home); array-subscript rows: home refused as onmain,/tmptwins refused (the item 10 trade-off). Every earlier probe moves only/tmptwins denied->allowed, each tomain's verdict, plus c120x'sc=${0:0:0}rm; $c -rf ~after the span, whichmainand 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 tomain; span timing linear (0.83 s at 5.8 KB). Gates: 1,918 passed, plus the 2markdown_ithost-import failures; the new tests fail on05b0cb9d0f.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 aNAME=assignment value, so a leading redirection target got it too: in>"$(<capped>)" rm -fr ~the synthetic~landed in the redirection target,_rm_wipe_targetsstepped the>redirection over it, took~as the program, and skipped the segment, so the realrm -fr ~after it was never read and bash ran a home wipe (Opus review on536d7e80d9,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 realrmis seen.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(bends the word at the()._heredoc_delimiterbroke at the(, left the delimiter as$, and read the)" <target>operator-line tail as body, closing the enclosing span early and hiding thermtarget after it:rm -fr "$(cat <<$(echo E)⏎$(echo E)⏎)" ~ ; ...wiped home while the floor allowed it (Opus review on536d7e80d9,shell_normalizer_span.py:1190). Reading the construct to its real end costs a balanced walk at EACH<<, soecho $((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
536d7e80d9andmain9d2df153d2: a 113-row c128 probe (F1:>/>/2>/</x=1 >/>...x=1/;-led redirection targets holding a 33- and 40-deep capped span, x~///$HOME//tmpxrm -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 thatmainrefuses; vs536d7e80d9, 45 rows flip allowed->denied (42 F1 redirection-target home/root wipes + 3 F2$(echo E)home/root wipes), each a real$HOMEor/wipe in bash with a stubrm, and the one$(echo E)/tmptwin is over-refused (the disclosed fail-closed cost of the unread delimiter, same family as the backtick /x$(e)/tmptwins already refused). The capped span as a plainrmOPERAND still fails closed (unchanged). Every earlier probe (c102..c126_sub2) is 0 flips vs536d7e80d9; 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 tomain; 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 overorigin/main. The new teststest_a_capped_span_as_a_redirection_target_cannot_mask_a_home_rmandtest_a_substitution_heredoc_delimiter_cannot_mask_a_home_rmfail on536d7e80d9and pass here.Verification of 9 vs
3a84d0827candmainea89e96911: c119 probe (GPT's row and variants) — the 6 home and/rows the prior head allowed are refused, as onmain, and every row matchesmain; c119 sweep (288 rows: 12 escaped characters x both quote forms x kept/dropped early line x~////tmpx 2 outer forms, each run in real bash with a stubrm) — 0 rows this head allows that wipe the home directory or/(20 on the prior head, all refused bymain), 0 rows this head refuses thatmainallows and bash runs harmlessly; every earlier probe identical to the prior head; c118 rows (1,920) 0 rows this head allows thatmainrefuses; push sweeps 0 changes (6,688 + 2,268 + 20,328), 0 protected pushes allowed; rm corpus (4,795) 0 changes; kill corpus (546) identical tomain; span timing linear (0.82 s at 5.8 KB). Gates: 1,848 passed, plus the 2markdown_ithost-import failures; one commit.Verification against
main@41708c96aband the prior headf5bf048215(every allowed/denied command confirmed in real bash with stub binaries that delete/push/kill nothing):nginxtwins and the (c)/tmprows stay atmain's verdict. The (b)/tmparithmetic twin is refused as the documented fail-closed consequence of unmodelled arithmetic ($(( 1<<2 ))reads as a heredoc), not a new class.mainallows, every one a real$HOMEor/wipe under a stubrm, 0 harmless over-refusals. 4,795-command rm corpus: 0 home/root rows head allows thatmainrefuses; 0 verdict changes vsf5bf048215(head andf5bf048215both deny 3,200).mainrefuses; 0 protected pushes allowed (head allows 3,715). Six rows flip allowed→denied vsf5bf048215, each a heredoc-obfuscated push the (a) fix now reaches, refused fail-closed — a tightening, never a loosening. Kill corpus (546): identical tomain. Nested / heredoc / backtick push probes: 0 loosening flips.c104v_timen = 50 / 100 / 200: 0.19 / 0.38 / 0.75 s)._PACKAGE_LINE_BUDGETre-pinned to the measuredcat src/kiro_crew/security/*.py | wc -l= 33,525 (+2: theargv_floorfacade manifest now names the two new forwarded helpers,_ARITH_CONTEXT_REand_arith_shift_possible, whichtest_facade_mirror_guards.pyrequires). 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 andnginxtwins allowed),test_an_apostrophe_in_a_quoted_assignment_prefix_keeps_the_home_rm(theMSG="it's"home rows refused), andtest_an_ansi_c_heredoc_delimiter_is_decoded(the$'E'/$"E"/tmpcleanup 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_rmasserts 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/tmptwin 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_rmandtest_a_self_kill_after_a_shift_that_closes_late_is_denied.Prior revision (the heredoc body read structurally as data, base
origin/mainb2b87b0f1a, head76e9cc8a4a)One commit, still based on
origin/main@b2b87b0f1a(a test rebase onto the currentorigin/mainis 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"/<<\Eunquote 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:
)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;$'a\'bon 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_linehelper 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@41708c96aband the prior headf5bf048215:<<E/<<'E'× direct / nested / post-terminator × home/root//tmp): 0 home/root rows head allows thatmainrefuses; head refuses 12 direct ANSI-C home/root rowsmainallows, each a real wipe (bash hands$HOMEor/to a stubrm); all/tmptwins matchmain.mainallows, each a real wipe; every/tmptwin matchesmain; 0 rows head allows thatmainrefuses. c102: identical tomain.mainrefuses; 0 verdict changes vsf5bf048215.main. Push corpus (20,328 commands): 0 flips vsf5bf048215; 0 protected pushes allowed (head andmaineach allow 3,721 corpus rows, 0 protected among either).c104v_timen = 50 / 100 / 200: 0.19 / 0.37 / 0.73 s)._PACKAGE_LINE_BUDGETre-pinned to the measuredcat src/kiro_crew/security/*.py | wc -l= 32,969. Gates green (pytest: the only 2 failures are themarkdown_ithost-import gap, 1,884 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit overorigin/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/tmptwin allowed; plus a terminator-tracking row — a<<-Ewith a tab-indented terminator and two heredocs pending on one line — refused on home/root and allowed on its/tmptwin).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 ontoorigin/main@b2b87b0f1a(one commit). The rebase conflicts were_PACKAGE_LINE_BUDGETandtest/test_security.py, whichmainsplit (#23119): the five assertion updates this change makes there now land intest/test_security_shell_grammar.py, where those tests moved. None of main's new commits touch the security files this change edits.'/"/ 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.Verification against
main@b2b87b0f1aand the prior head3032ab76a0:mainon every row. Heredoc-body quote probe (288 rows): 0 home/root rows head allows thatmainrefuses; head refuses 90 home/root rowsmainallows, each a real wipe; all 72/tmptwins matchmain.mainrefuses; 0 verdict changes vs3032ab76a0.main. Nested sweep (2,268): 0 flips. Heredoc and backtick push probes: 0 flips.3032ab76a0. Every one is a FEATURE-branch push (git push [-u] origin featthroughbash -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,main3,715; 0 protected among either)._PACKAGE_LINE_BUDGETre-pinned to the measuredcat src/kiro_crew/security/*.py | wc -l= 32,927. Gates green (pytest: the only 2 failures are themarkdown_ithost-import gap, 1,883 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit overorigin/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/tmptwins stay allowed) andtest_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/tmptwins stay allowed). A liveness row pins the near-linear growth of the nested-span run.Prior revision (the heredoc-body
rmopener family, baseorigin/main64576241e0, head3032ab76a0)The heredoc-body
rmfamily reviewers flagged onaca7ad2404is closed, rebased ontoorigin/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:; 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.(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 keepmain's verdict.Verification against the prior head
aca7ad2404andmain:(/$(/<(/>(bodies ×<<E/<<'E'× direct / nested / later-backtick × home / root //tmp): head matchesmainon every row;aca7ad2404allowed 30 rowsmainrefuses. The earlier rm probes (24 + 58 rows) also matchmainon every row.mainrefuses; 0 verdict changes vsaca7ad2404.aca7ad2404across 29,284 commands (6,688 + 2,268 probe/nested sweeps and the 20,328-command corpus), plus the heredoc and backtick push probes.main._PACKAGE_LINE_BUDGETre-pinned to the measuredcat src/kiro_crew/security/*.py | wc -l= 32,811. Gates green (pytest -n 4: the only 2 failures are themarkdown_ithost-import gap, 1,880 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit overorigin/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 onmain; every/tmptwin stays allowed).Prior revision (the last heredoc-body
rmhole, baseorigin/main8e80404a00, headaca7ad2404)One more reviewer-flagged home-directory
rmbypass is closed on top of the prior pushed headf1d2e350b4, rebased ontoorigin/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 thermsegment never saw the home operand after it —rm -rf "$(cat <<E+ newline +${+ newline +E+ newline +)" ~wiped$HOMEwhile the floor allowed it, wheremainrefuses 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/tmptwin of the same shape stays allowed, as onmain. (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
rmthat deletes nothing):rm/home/root//tmpspellings): head is byte-identical tomain(0 head-allow/main-deny, 0/tmp-twin over-deny); vs the prior headf1d2e350b4, 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 matchesmainwith 0 home head-allow/main-deny (the 14/tmpdivergences aremainover-refusing capped spans, 0 home).rm): the body-line${/${x/${x:-shapes the prior head allowed are confirmed live$HOME//wipes and are all refused now; their/tmptwins stay allowed on both. (The$(/<(/>(body-line shapes are allowed on BOTH head andmain— 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.)f1d2e350b4in either direction; 0 protected pushes allowed in real bash.${-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_BUDGETre-pinned to the measuredcat src/kiro_crew/security/*.py | wc -l= 32,775. Gates green (pytest -n 4: the only 2 failures are themarkdown_ithost-import gap, 1,879 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit overorigin/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 onf1d2e350b4, refused here and onmain; the/tmptwins stay allowed).Prior revision (two home-directory
rmholes, baseorigin/main5ea67b3cb4, headf1d2e350b4)Two reviewer-confirmed home-directory
rmbypasses are closed on top of the prior pushed head8b6cfaabcc, rebased ontoorigin/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):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 (~) sorm -rf "$(<capped>)" ~fails closed, but it must NOT do that when the span is aNAME=assignment VALUE — there the synthetic~lands between the assignment and a realrmafter it, sox="$(<capped>)" rm -rf $HOMEwas read as the command~withrmas its argument and the home wipe was missed. The prior head decided "assignment value" with a start-anchored regex that matched only whenNAME="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 thermanyway. "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 aNAME=/NAME+=word and every completed word before it was too. All the prefixed shapes are read as the assignment values they are, so the realrmis seen;mainrefuses them too.+=stays atmain's verdict (its env-strip leavesx+=in place).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 enclosingrmsegment never saw the home operand after it — arm -rf "$(…)" ~shape wiped$HOMEwhile the floor allowed it, wheremainrefuses 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~/$HOMEafter it reaches the rm floor, or fails closed on a home operand. The/tmptwin of the same shape stays allowed, as onmain. (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
rmthat deletes nothing):rm/home//tmpspellings): head is byte-identical tomain(0 head-allow/main-deny, 0 tmp-twin over-deny); vs the prior head8b6cfaabcc, 270 home rows flip allowed→denied and 0 denied→allowed. The parent's independent 4,795-command rm corpus: of the 14 head allows thatmainrefuses, all are/tmpoperands (mainover-refusing capped spans), 0 are home.rm): the 14 separator/prefix shapes the prior head allowed are confirmed live$HOMEwipes and are all refused now; the confirmed backtick heredoc shape is refused now (matchingmain) and its/tmptwin stays allowed.8b6cfaabccin either direction._PACKAGE_LINE_BUDGETre-pinned to the measuredcat src/kiro_crew/security/*.py | wc -l= 32,736. Gates green (pytest -n 4: the only 2 failures are themarkdown_ithost-import gap, 1,878 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit overorigin/main.Prior revision (maintainer round, base
398772ffdd, head8b6cfaabcc)Three changes on top of the prior pushed head
8939c641f2, rebased ontoorigin/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):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 pushesmain. 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.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_walkis a single linear pass with nocan_arithparameter.A home-directory
rmbypass (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 (~) sorm -rf "$(<capped>)" ~fails closed. That synthetic~was inserted even when the capped span was aVAR="..."assignment VALUE, where the span feeds a variable, not anrm: inx="$(<capped>)" rm -rf $HOMEthe~landed between the assignment and the realrm, so_rm_wipe_targetsread~as the command andrm -rf $HOMEas its arguments and missed the home wipe — ALLOWED since at least629d0e4292, whilemainrefuses it. The synthetic operand is now added only in operand/command position (_ASSIGNMENT_VALUE_OPENER_RErecognises a leading[VAR=val …] NAME="so the assignment-value case is excluded), so the realrmis seen;mainrefuses 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 stubrmthat deletes nothing /bash -nfor the/bin/rmshapes):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.rmspellings × 7 home operands × many decoy spans × positions): of the 358 head allows thatmainrefuses, 0 actually wipe~/$HOMEin real bash — 320 run but target no home dir, and the other 38 (/bin/rmforms behind an unterminated backtick) are allbash -nsyntax errors, sormnever runs. The 82VAR="$(<capped>)" rm -rf $HOMEshapes that the prior head allowed are now refused._PACKAGE_LINE_BUDGETis re-pinned to the measuredcat 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 themarkdown_ithost-import gap, 1,877 passed); black / isort / flake8 / mypy clean; comment-history and file-line-cap pass; one commit overorigin/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 pushmainwith a command the gate allows. The same mis-delimiting also refuses ordinary commands: a feature push aftergit commit -m "$(date)"is denied with an "unverifiable" reason, and the agent has to reword and retry.Not a goal
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._matching_close_paren/_substitution_bodies(both inshell_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_bodiesand the_substitution_bodiesscan inargv_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
):$(/<(/>(recurses through the walk and resumes the outer quote state across the jump;${...}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, sorm -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);<(/>(process substitution opens only unquoted (state 0), so the double-quoted<(inrm -rf "$(echo ${x:-"<("})" ~is text and the span still closes at its)with the~intact;`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:$(cat <<E)closing before its newline) is passed up, so the enclosing span's next newline is unproven too;)before that newline still proves the span, sorm -rf "$(cat <<E)" ~keeps its~operand for the rm floor;<<<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 aRecursionError.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 asbash -c 'git push')", reads asgit 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_NESTINGhas 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-byteecho $(echo $(…took 69 s before and is now within ~1.2× ofmain). 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 abash -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, soecho $(echo …33-deep… ${x:-(} )…33… ; echo "$(bash -c '<push main>')"was ALLOWED while bash pushedmain(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 inecho $(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$(bashspelling of the same tail was refused only because a$(bashtoken reads as a launcher, which<(bashandx=$(bashdo not (bolichen97's BLOCK on d42c8f4:cat <(bash -c|sh -c|eval '<push main>')andecho x > >(bash -c '<push main>'), after;or&&, were ALLOWED whilemainrefuses 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 inTestAMisreadSpanFailsClosed::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_entriesnow 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 inTestAMisreadSpanFailsClosed::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 andecho donetwins 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 plainpushspelling too). Suppression is now by entry:_substitution_body_entriesflags 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_bodiesis removed. Rows inTestAMisreadSpanFailsClosed::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 andecho donetwins 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 madeecho "$(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/evalpayloads), 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 thanmain), 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 — inecho '$(' "$(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'ton 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_bodiesre-derives a barekill'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_parenmark a span holding a<<unproven, and this raw walk treated unproven as "ends here": it recorded the remainder and seti = 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 thatpgrep(confirmed in real bash with stubkill/pgrep), sok=kill; x=$(cat <<E\nE\n); $k $(printf ')' ; pgrep -f <self>)and its<(and"$(twins were ALLOWED on the prior pushed head9ad63e8129whilemainrefuses 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)) landsiatnand 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_walkproven 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-killraw 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 featis 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_parenpass the slice after the opener at neutral state, so itsopen_endparameter and theoff < open_endskip are removed.File split: this change would take
shell_normalizer_walk.pypast 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_bodiesmove to a new sibling,shell_normalizer_span.py(794 lines, including the new_capped_tail_entriesnever-close tail scan). It reads the lexer helpers fromshell_normalizer_walk.py, now 1,181 lines, which imports nothing back.shell_normalizerand thekiro_crew.securityname map still serve all three names callers use, so every import path holds.Backwards compatibility
Compatible: every command newly refused versus
mainembeds 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_parenlosing its unusedopen_endparameter (all three callers updated)._substitution_bodieskeeps its signature but can now return more than one body per opener; four one-body unpacks intest_security.pyread the first body (body, *_ = ...), which is the same reading as before.Tests
All in
test/test_push_branch_gate_parser.py(mainmoved this half oftest_push_branch_gate.pythere).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 baregit 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\Etags, 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.TestAnArithmeticShiftIsAnExpectedRefusalpins 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 abash -cpush,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 anrm -rftarget. All 3 fail on the head before that round, 86f817f (two allows, oneRecursionError), and pass with the fix. The fourth round addstest_a_single_quoted_opener_cannot_swallow_the_real_oneandtest_a_bare_brace_in_a_default_is_data(rm -rf "$(echo ${x:-{})" ~plus the_matching_close_bracespan), both failing on eb1bdbc, andtest_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) andtest_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_dollartable),test_a_quote_inside_an_earlier_span_quotes_nothing_after_it(echo "$(echo "'")" "$(git p""ush origin main)") andtest_a_capped_span_still_scans_past_its_heredoc(a 32-deep$(echoprefix then a heredoc and abash -cpush). The eighth round addstest_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 addstest_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 ofecho "$(bash -c '<push main>')" # '. All four fail on 3ffd044 and pass with the fix. The tenth round addstest_a_bare_double_paren_without_an_adjacent_close_is_two_subshells: four spellings of((cat <<E\n)\nE\n) )in front of abash -cpush, 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) andtest_an_rm_target_after_a_capped_span_fails_closed(an 18-deep"$(echo ${x:-chain inrm -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 toTestAMisreadSpanFailsClosed, 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$(echospan thenecho "$(echo "$(bash -c '<push main>')")"— the real opener after the capped-but-balanced span was never scanned because the capped branch stopped there) andtest_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 protectedgit 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 head588749dand 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 on8939c641f2and denied here; most push in real bash with a stub git, the rest over-refused) andtest_a_capped_span_as_an_assignment_value_cannot_mask_a_home_rm(thex="$(<capped>)" rm -rf $HOME/${HOME}/~family across a deep$(echonest and an 18-deep${x:-brace nest × severalrmspellings and home spellings — each ALLOWED on8939c641f2, refused here and onmain; the capped-span-as-operandrm -rf "$(<capped>)" ~still fails closed, and a capped assignment value with normafter it stays allowed).TestAnArithmeticShiftIsAnExpectedRefusalcarries the arithmetic expected-refusals.This revision also adds
TestSelfKillHeredocSpanDoesNotStopTheRawWalktotest/test_denied_commands_security_kill.pyfor the self-kill raw-walk regression:test_heredoc_decoy_kill_is_still_a_self_kill(7 rows — the three issue spellingsk=kill; x=$(cat <<E…); $k $(printf ')' ; pgrep -f <self>), the<(and"$(twins, a backtick-alias decoy, a literal-killheredoc decoy, a heredoc run, and a heredoc whose close has a)glued after it so the target rides the whole-remainder over-read) andtest_heredoc_decoy_twins_stay_allowed(4 rows — the same shapes with a non-protectednginxtarget, and a heredoc sibling owned by a different command, all still allowed). The four alias-decoy rows FAIL on the prior pushed head9ad63e8129(ALLOWED) and pass with the fix;mainrefuses all 11.test/test_security_gate_liveness.pygains 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 insideis_denied(in bothshell_normalizer_walkandshell_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 andmainare 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+×N1))×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-openerfind/rfindmade 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
TestSubstitutionInsideDoubleQuotesIsExtractedFromAFreshContextand the_matching_close_parenunit assertions use the sliced signature, and the exact-body lists in it and inTestNestedPayloadExtractionSpansTheProvenBoundarynow include the inner opener's body and the last-)reading.Rebased on
main@ a247d03 (no conflict; none of main's new commits touch security; everyis_deniedverdict is byte-identical to the pre-rebase headfe3b23e079, 0 flips across 5,104 push-gate and 2,268 nested commands). The comment above the suffix-sub check in_shell_payload_walkis shortened soshell_normalizer.pystays at the 1,600-line split point in AGENTS.md (1,600;mainhas 1,590)._PACKAGE_LINE_BUDGETgoes from 32,056 to 32,688, the measuredcat 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_deniedkeeps 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 bymain, 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 pushesmainallows 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)runsx;<(…)runs/dev/fd/N) once that side succeeds. Nothing head allows there pushes a protected branch, and it refuses all 646 protected pushesmainallows. Every liveness shape stays linear at the 20 KB cap (steps N=30 → 60, wall time vsmain): 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_entriesnow 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_deniedkeeps every verdict over the 5,104 push-gate and 2,268 nested sweeps vs588749d9e2(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 pushesmainallows 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 splitargv_floor.pyintoargv_floor_self.py/argv_floor_hosts.py/argv_floor_ssh.py/argv_floor_git.pybehind a facade (#21952), so the kill-floor change lands inargv_floor_self.py(the<(/>(call getscan_arith=False, the$(call drops the removedopen_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_bodiesresumes past an unproven-but-closed span in both the$(and<(/>(branches, emitting the whole-remainder over-read once). It is now rebased onmain@ d0622d0; the only conflict was_PACKAGE_LINE_BUDGET, and main's two new security commits (#21867, #22851) touch only_child_realpath.pyand thepaths_*modules, none of the files this change edits.is_deniedis byte-identical to the pre-rebase headdf55d19676(0 flips over the push probe and 2,268 nested sweeps, identical verdicts over the 546-command self-kill corpus).is_deniedkeeps every verdict over the self-kill differential corpus — head is byte-identical tomain(0 flips either direction) over 288 kill/pkill/killall/$k-alias × heredoc-vs-plain span ×$(/<(/"$(× self-vs-nginxtarget commands — and over the push probe (5,104), nested-heredoc (2,268) and c97 (20,328) sweeps (0 flips vs the prior head9ad63e8129, since onlyargv_floor_self.py's self-kill walk changed). The eight self-kill flips vs9ad63e8129are all allowed→denied, every one akirocrewself-killmainalso refuses._PACKAGE_LINE_BUDGETgoes from 32,690 to 32,710 (the measuredcat src/kiro_crew/security/*.py | wc -l), with a reason comment after the prior one. Real bash with stubkill/pgrepconfirms the four heredoc-decoy shapes reach thepgrepclause, 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 oftest_security.py(-k, every class that calls_substitution_bodies,_is_self_killor a span matcher): 157 passed; the full file OOMs on this host.black/isort/flake8/mypyclean on touched files;check_comment_history.pyandcheck_file_line_cap.pypass.Manual verification
Every sweep compares
is_deniedonorigin/mainvs this branch and runs each allowed command in real bash with a fakegitonPATHthat records its args.Against
main@ 0b89e0c (a freshorigin/mainworktree):mainlets throughmainrefuses and this branch allows runs a protected push (all newly-allowed are feature pushes after a quoted$(...)).git commit -m "$(cat <<'EOF' … EOF\n)" && git push origin feat/example) and thegh pr create --base main --body "$(cat <<'EOF' …)"idiom are refused bymainand by this branch alike, so neither is newly refused.((rule refuses 96 more commands than the prior head cc4a08e; all 96 hold((andgit push origin maintext,mainrefuses 84 of them, and none runs a protected push in real bash (each is a bash syntax error).maincarriesgit pushtext, almost all inside heredoc data or a nested quoted$(; no clean command is newly blocked.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 andmain13,909; of the 1,136 the branch allows andmainrefuses, only 2 parse underbash -n(rm -rf "$(echo $((${)))" ~and the${x:-form), and both are${-bad substitutionerrors bash aborts the command on, sormnever runs. Both were already allowed on the prior head 12ea71f, so neither is a regression.Timing —
is_deniedon the adversarial opener shapes, this branch vsmain@ 574cb18 (every probe wrapped intimeout 300):mainN=100mainN=200echo+$(×Necho "+"$(×Necho+'$(' "$(×Necho+$(echo×N +)×Necho+$((×Necho+$((x+×N +1+))×Necho+$((echo×Necho+$((x<<×Necho+$((cat <<E×Necho "+"$(cat <<E\n) "×NEvery 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 thanmain. 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 thanmain, which is itself super-linear on this shape._substitution_bodiesalso 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 againstmain@ 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
Contribution License Agreement
Contributed under the project's Apache-2.0 license.