fix(security): fail closed on unreadable process-substitution bodies by CrysisDeu · Pull Request #8719 · kirodotdev/KiroCrew

CrysisDeu · 2026-09-05T09:20:57Z

Summary

Follow-up to #8712 (c791f0f1d), which reconciled #7356 with #7808 and turned main green on the nine push-gate assertions. This PR keeps #8712's structure exactly as merged (_iter_shell_chars, _shell_quote_walk, the proven quote-aware process-substitution boundary) and closes the two allow-direction gaps that survive it, both found by the GPT 5.6 review lane over eight rounds on this PR's earlier revision and each re-verified with a read-only probe against origin/main at c791f0f1d.

Refs #8695, #8706 (both resolved by #8712 -- this PR does not re-fix them).

What still escapes on `main` (probe: `is_denied(...)` returned `None`)

#	Shape bash runs	Why #8712 misses it
1	`echo \$'foo\'; git push origin main; eval 'git push origin feature'`	`_iter_shell_chars` opens ANSI-C on a raw `text[i-1] == "$"` lookback, so the ESCAPED `\$` opens `$'...'`, the quote stays open across `;`, the segment never splits, the `git` word is "quoted" and the outer line defers to the eval payload (a feature push)
2	`git push origin feature > >(cat >/dev/null # fake )`⏎`) main`	the boundary walk models quoting only; the commented `)` balances the count, `main` on the next line is never scanned
3	`git push origin feature-x > >(case x in x) git push;; esac)`	a `case` pattern's `)` is syntax, not the closer; the nested bare push runs
4	`git push origin feature > >(/usr/bin/g?t push origin main)` (also `gi[t]`, extglob `/usr/bin/@(git)`)	the body closes cleanly, but the payload walk that judges it cannot resolve a globbed / extglob program word; bash expands it to `git` at run time

Fix (implementation only, `src/kiro_crew/security.py`, +102/-5)

ANSI-C is exact. $' opens an ANSI-C string only after a literal, unpaired $ (an odd run; $$ is the PID parameter, \$ is data). The docstring's "only ever over-flags" claim is retired: since fix(security): hand the push arity scan raw words, not cut ones #8712 the segment split and the program anchor read this state in the allow direction, so a false "still open" hides a separator or a git word.
Unreadable body words fail closed (allowlist). In front of fix(security): hand the push arity scan raw words, not cut ones #8712's proven boundary walk, _process_substitution_word_is_opaque admits an unquoted body word only if every unquoted character is in _PROCESS_SUBSTITUTION_SAFE_CHARS -- letters, digits, / - _ . = : (the alphabet of an ordinary program invocation) -- plus the quote delimiters the walk owns, the $ of an ANSI-C $'...', and the closing ). Anything else (glob * ? [, extglob / nested (, #, & ; |, ~, !, braces, $/backtick expansions, and any unquoted backslash escape such as \g\i\t) or an unquoted bash reserved word (case, if, for, while, function, {, [[, ...), or an unescaped $ / backtick inside double quotes ("$GIT" push origin main expands at run time) makes the body opaque: _git_push_args returns None and the segment takes the non-opt-out-able sentinel. A redirection whose attached target opens a paren (--push-option 2>(cat >/dev/null # fake )) gets the same answer -- skipping it as a self-contained redirection left the body's remainder to be read as argv, where a # truncated the real refspecs. Rationale: the earlier denylist grew by one shell metacharacter per review round (*?[ → extglob ( → # → & ; |), and an enumeration of what bash can do with a character is never finished -- so the rule names what IS readable instead. Known over-block: a redirection or other metacharacter inside the body (> >(cat >/dev/null)) is ungated rather than allowed.

Everything #8712 pins keeps its precise reading (probed): > >(echo '(' ) main and > >(echo $'a\'b') main still recover ["origin", "feature", "main"] and report protected-branch-name; > >(tee log.txt) / > >(tee 'log.txt') stay allowed; 2> >(cmd) still yields ["--force", "origin"]; unterminated bodies still fail closed; (git push origin main) keeps #8712's precise row. No test assertion was changed or removed.

Differences from #8712

Area	#8712	this PR
ANSI-C opener	`text[i-1] == "$"` (escaped `\$'` misread)	literal, unpaired `$` only
process-sub body: quoted/escaped parens	proven by quote-aware walk	unchanged
process-sub body: any unquoted character outside the safe alphabet (comment, operator, glob, extglob, expansion, tilde, brace) or a reserved word	not modelled -> boundary "proven" wrongly	fail closed (ungated), allowlist
everything else in the push gate	as merged	untouched

Pattern harvest

Rule candidate: review-prompt -- when a security parser "proves" a boundary, ask what grammar the proof covers and enumerate the bash constructs outside it (comments, reserved words, globs, expansions); a proof over quoting alone is not a proof over bash. The eight GPT rounds on this PR were exactly that enumeration.

Not generalizable as semgrep: the defect is a semantic gap between a state machine's coverage and the shell's grammar, not a syntactic anti-pattern.

Verification

Static gates only on this desk (CI is the test oracle): mypy src/kiro_crew/security.py clean; scripts/check_black_formatting.py passes (new hunks black-clean). Read-only probes: 4 escaping shapes above + the earlier 4 GPT shapes all denied; #8712's pinned shapes unchanged.

github-actions · 2026-09-05T09:23:24Z

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 399e773deaf80e91e74b253a81cf41f08b52b95f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound fix shape (allowlist over an unbounded denylist), but four closed security bypasses ship with zero pinned regression tests.

Watch

Untested security closures. The PR closes four concrete allow-direction bypasses of the protected-branch gate, yet touches only security.py — "No test assertion was changed or removed" and "Static gates only on this desk (CI is the test oracle)" mean the only evidence the shapes are denied is desk probes in the PR body. Every one of these gaps existed because no test held; a future refactor of _iter_shell_chars or the opaque-word check silently reopens them the same way. Pin the four shapes (and the \$' ANSI-C case) in test/test_push_branch_gate.py before merge.
Two walkers must agree on one state. _process_substitution_word_is_opaque re-scans each body word with _iter_shell_chars while the caller advances state via _shell_quote_walk; any divergence in how the two read (state, ansi) is the next allow-direction bug. Worth collapsing into one walk, or at least a test asserting they stay in lockstep.

Suggestions

Given the failure direction is an approval prompt (not a denial), weigh the simpler containment — any process substitution in a push segment is ungated — which deletes the safe-alphabet machinery entirely at the cost of prompting on > >(tee log.txt); if precision for that case is worth ~100 lines of bash semantics, record that decision.

[DESIGN-REVIEWED] 399e773

github-actions · 2026-09-05T09:26:01Z

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 399e773deaf80e91e74b253a81cf41f08b52b95f — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in. The change is a security fix to the git-push gate in security.py: an exact ANSI-C opener and a fail-closed allowlist for process-substitution body words. Both close probed allow-direction bypasses. Two depth/duplication findings survive verification: the fix guards only one of the two paren-boundary proof sites, and the new reserved-words set is a near-copy of one in channel.py.

First-Principles-Verdict: CONCERNS

The allowlist is the right cause-level cure, but it guards one of two boundary-proof sites, and its reserved-word list re-spells channel.py's.

What this change ships

Intent: stop bash spellings the push gate cannot read from smuggling a protected-branch push past it — a FIX.

Escaped \$' (and paired $$') no longer misread as ANSI-C, denying the hidden push — justified (probed bypass)
Comment, reserved word, glob, extglob, tilde, brace, or operator in a substitution body now blocks the push — justified, cause-level allowlist
Unescaped $/backtick inside double quotes in a body now blocks — justified (runtime expansion)
Unquoted escape spellings (\g\i\t) in a body now block — justified
Redirection glued to a paren-opening target (2>(...)) now blocks — justified (probed truncation)
Benign body redirections (> >(cat >/dev/null)) now denied with no operator opt-out — declared changed default
New bash reserved-words set — near-duplicate of channel.py:144

Watch

The harvest section's own rule — "a proof over quoting alone is not a proof over bash" — is applied to one of the two proof sites. Grepped _matching_close_paren: its one other consumer, _substitution_bodies (security.py:4687), still proves $(...)/<(...) spans over quoting alone, so a case-pattern ) or word-initial # mis-sizes the extracted body there with no opacity check. Accepted-and-deferred if the caller's outer scan covers the tail; the description does not say which level this leaves that site at.

Subtractions

Replace _SHELL_RESERVED_WORDS (security.py:6855) with the existing _CHANNEL_SHELL_RESERVED_WORDS (channel.py:144), shared from one owning module — the 18 alphabetic words are identical, and two hand-kept copies of bash's word list will diverge.
Drop !, [[, ]], {, } from that set — each contains a character outside _PROCESS_SUBSTITUTION_SAFE_CHARS, so the char walk already refuses them; the entries decide nothing.

[FIRST-PRINCIPLES-REVIEWED] 399e773

github-actions · 2026-09-05T09:27:27Z

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 399e773deaf80e91e74b253a81cf41f08b52b95f and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/security.py:6897 -- The prose-enumerated allowlist lacks the round-trip assertion required by recurring-defect-patterns -> Fix: replace the member enumeration with a non-enumerating description.
[GPT-REVIEWED] 399e773

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 399e773deaf80e91e74b253a81cf41f08b52b95f: <one-sentence reason>

github-actions · 2026-09-05T09:32:57Z

Opus 4.8 Review — ✅ no blocking findings

Reviewed 399e773deaf80e91e74b253a81cf41f08b52b95f — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 399e773

Verdict parsed from the review's SHA-scoped output markers for commit 399e773deaf80e91e74b253a81cf41f08b52b95f.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 399e773deaf80e91e74b253a81cf41f08b52b95f: <one-sentence reason>

Follow-up to #8712 (c791f0f), which reconciled #7356 with #7808 and turned main green. Two allow-direction gaps survive it, both found by the GPT 5.6 review lane over eight rounds on #8719 and each verified with a read-only probe against main: * The ANSI-C decision in _iter_shell_chars used a raw `text[i-1] == "$"` lookback, so an ESCAPED dollar (`\$'foo\'`) opened `$'...'`, kept the quote open across a `;`, and hid the publish behind it -- with the segment split and the program anchor now reading this state, a false "still open" is an allow-direction error, not the over-flag the docstring claimed. `$'` now opens ANSI-C only after a literal, unpaired dollar (an odd run; `$$` is the PID parameter). * The process-substitution boundary walk proves the closer for QUOTING only. Three constructs outside quoting moved it or hid the program while the count still balanced: a word-initial `#` comments out the `)` after it (`> >(cat >/dev/null # fake )` + newline + `) main` pushed main), a `case` pattern's `)` is syntax (`> >(case x in x) git push;; esac)` ran the nested bare push), and a globbed program word (`> >(/usr/bin/g?t push origin main)`) closed cleanly but resolves to `git` only at run time, so the payload walk that judges the skipped body saw no push. Modelling bash constructs one at a time is unbounded, so the boundary is refused as a class: a body word the walk cannot read -- a word-initial unquoted `#`, an unquoted reserved word, an unquoted glob or expansion character (`$'` excepted, the walk models it) -- returns None and the segment takes the non-opt-out-able sentinel. #8712's proven quote-aware walk stays exactly as merged; the rule is layered in front of it, so every shape #8712 pins (quoted parens, ANSI-C escapes, `> >(tee log.txt)`, `2> >(cmd)`) keeps its precise reading. No test assertion was changed. Refs #8695, #8706 (both resolved by #8712).

Follow-up to #8712 (c791f0f), which reconciled #7356 with #7808 and turned main green. Two allow-direction gaps survive it, both found by the GPT 5.6 review lane over eight rounds on #8719 and each verified with a read-only probe against main: * The ANSI-C decision in _iter_shell_chars used a raw `text[i-1] == "$"` lookback, so an ESCAPED dollar (`\$'foo\'`) opened `$'...'`, kept the quote open across a `;`, and hid the publish behind it -- with the segment split and the program anchor now reading this state, a false "still open" is an allow-direction error, not the over-flag the docstring claimed. `$'` now opens ANSI-C only after a literal, unpaired dollar (an odd run; `$$` is the PID parameter). * The process-substitution boundary walk proves the closer for QUOTING only. Three constructs outside quoting moved it or hid the program while the count still balanced: a word-initial `#` comments out the `)` after it (`> >(cat >/dev/null # fake )` + newline + `) main` pushed main), a `case` pattern's `)` is syntax (`> >(case x in x) git push;; esac)` ran the nested bare push), and a globbed program word (`> >(/usr/bin/g?t push origin main)`) closed cleanly but resolves to `git` only at run time, so the payload walk that judges the skipped body saw no push. Modelling bash constructs one at a time is unbounded, so the boundary is refused as a class: a body word the walk cannot read -- a word-initial unquoted `#`, an unquoted reserved word, an unquoted glob or expansion character (`$'` excepted, the walk models it), or a nested unquoted `(` (an extglob program `/usr/bin/@(git)` balances the count and resolves only at run time) -- returns None and the segment takes the non-opt-out-able sentinel. #8712's proven quote-aware walk stays exactly as merged; the rule is layered in front of it, so every shape #8712 pins (quoted parens, ANSI-C escapes, `> >(tee log.txt)`, `2> >(cmd)`) keeps its precise reading. No test assertion was changed. Refs #8695, #8706 (both resolved by #8712).

Follow-up to #8712 (c791f0f), which reconciled #7356 with #7808 and turned main green. Two allow-direction gaps survive it, both found by the GPT 5.6 review lane over eight rounds on #8719 and each verified with a read-only probe against main: * The ANSI-C decision in _iter_shell_chars used a raw `text[i-1] == "$"` lookback, so an ESCAPED dollar (`\$'foo\'`) opened `$'...'`, kept the quote open across a `;`, and hid the publish behind it -- with the segment split and the program anchor now reading this state, a false "still open" is an allow-direction error, not the over-flag the docstring claimed. `$'` now opens ANSI-C only after a literal, unpaired dollar (an odd run; `$$` is the PID parameter). * The process-substitution boundary walk proves the closer for QUOTING only. Three constructs outside quoting moved it or hid the program while the count still balanced: a word-initial `#` comments out the `)` after it (`> >(cat >/dev/null # fake )` + newline + `) main` pushed main), a `case` pattern's `)` is syntax (`> >(case x in x) git push;; esac)` ran the nested bare push), and a globbed program word (`> >(/usr/bin/g?t push origin main)`) closed cleanly but resolves to `git` only at run time, so the payload walk that judges the skipped body saw no push. Modelling bash constructs one at a time is unbounded, so the boundary is refused as a class: a body word the walk cannot read -- a word-initial unquoted `#`, an unquoted reserved word, an unquoted glob or expansion character (`$'` excepted, the walk models it), a nested unquoted `(` (an extglob program `/usr/bin/@(git)` balances the count and resolves only at run time), or an unquoted control operator / `#` inside the word (`>(cat&# fake )` opens a comment after the `&`, and to bash `#` is word-initial after an operator) -- returns None and the segment takes the non-opt-out-able sentinel. #8712's proven quote-aware walk stays exactly as merged; the rule is layered in front of it, so every shape #8712 pins (quoted parens, ANSI-C escapes, `> >(tee log.txt)`, `2> >(cmd)`) keeps its precise reading. No test assertion was changed. Refs #8695, #8706 (both resolved by #8712).

Follow-up to #8712 (c791f0f), which reconciled #7356 with #7808 and turned main green. Two allow-direction gaps survive it, both found by the GPT 5.6 review lane over eight rounds on #8719 and each verified with a read-only probe against main: * The ANSI-C decision in _iter_shell_chars used a raw `text[i-1] == "$"` lookback, so an ESCAPED dollar (`\$'foo\'`) opened `$'...'`, kept the quote open across a `;`, and hid the publish behind it -- with the segment split and the program anchor now reading this state, a false "still open" is an allow-direction error, not the over-flag the docstring claimed. `$'` now opens ANSI-C only after a literal, unpaired dollar (an odd run; `$$` is the PID parameter). * The process-substitution boundary walk proves the closer for QUOTING only. Constructs outside quoting moved it or hid the program while the count still balanced: a word-initial `#` comments out the `)` after it (`> >(cat >/dev/null # fake )` + newline + `) main` pushed main; so does `>(cat&# fake )`, since `#` is word-initial after an operator), a `case` pattern's `)` is syntax (`> >(case x in x) git push;; esac)` ran the nested bare push), and a globbed or extglob program word (`> >(/usr/bin/g?t push origin main)`, `>(/usr/bin/@(git) ...)`) closed cleanly but resolves to `git` only at run time, so the payload walk that judges the skipped body saw no push. Enumerating the offending metacharacters grew by one per review round, so the rule is an ALLOWLIST: an unquoted body word may consist only of letters, digits and `/ - _ . = :` (the alphabet of an ordinary program invocation) plus the quote delimiters the walk owns, the `$` of an ANSI-C `$'...'`, and a closing `)`; any other unquoted character, an unquoted backslash escape (`\g\i\t` reaches the program as `git` while no scanner word spells it), an unescaped `$` or backtick inside DOUBLE quotes (`"$GIT" push origin main` expands), or an unquoted reserved word, makes the body opaque and `_git_push_args` returns None so the segment takes the non-opt-out-able sentinel. The same fail-closed answer applies to a redirection whose ATTACHED target opens a paren (`--push-option 2>(cat >/dev/null # fake )`): skipping it as a self-contained redirection left the body's remainder to be read as argv, where a `#` truncated the real refspecs. Known over-block: a redirection or other metacharacter inside the body (`> >(cat >/dev/null)`) is ungated rather than allowed. #8712's proven quote-aware walk stays exactly as merged; the rule is layered in front of it, so every shape #8712 pins (quoted parens, ANSI-C escapes, `> >(tee log.txt)`, `2> >(cmd)`) keeps its precise reading. No test assertion was changed. Refs #8695, #8706 (both resolved by #8712).

CrysisDeu requested a review from a team as a code owner September 5, 2026 09:20

CrysisDeu requested a review from buluoray September 5, 2026 09:20

github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from 72d7aa5 to 74af3e3 Compare September 5, 2026 09:36

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from 74af3e3 to 04a8db3 Compare September 5, 2026 09:56

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from 04a8db3 to 964d5f2 Compare September 5, 2026 10:18

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from 964d5f2 to 1dd64e7 Compare September 5, 2026 10:48

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from 1dd64e7 to b9a69ac Compare September 5, 2026 11:10

bolichen97 mentioned this pull request Sep 5, 2026

refactor(probes): flatten the watch collapse and correct its comments #8701

Merged

5 tasks

github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 5, 2026

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from c96bddf to d9f06b3 Compare September 5, 2026 12:50

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 5, 2026

CrysisDeu changed the title ~~fix(security): reconcile #7356 and #7808 in the git-publish floor~~ fix(security): fail closed on unreadable process-substitution bodies Sep 5, 2026

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from d9f06b3 to a5feb39 Compare September 5, 2026 13:20

CrysisDeu force-pushed the fix/main-red-push-gate-8704 branch from a5feb39 to bbde828 Compare September 5, 2026 13:37

iamwhatever approved these changes Sep 5, 2026

View reviewed changes

pepmach mentioned this pull request Sep 5, 2026

fix(security): reconcile the publish gate's two merged lineages #8721

Closed

Navigation Menu

Uh oh!

fix(security): fail closed on unreadable process-substitution bodies - #8719

fix(security): fail closed on unreadable process-substitution bodies#8719
iamwhatever merged 1 commit into
mainfrom
fix/main-red-push-gate-8704

CrysisDeu commented Sep 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 5, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

CrysisDeu commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What still escapes on main (probe: is_denied(...) returned None)

Fix (implementation only, src/kiro_crew/security.py, +102/-5)

Differences from #8712

Pattern harvest

Verification

Uh oh!

github-actions Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Review (Fable 5) — 🟡 CONCERNS

Watch

Suggestions

Uh oh!

github-actions Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First Principles Review (Fable 5) — 🟡 CONCERNS

What this change ships

Watch

Subtractions

Uh oh!

github-actions Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review — ✅ no blocking findings

Uh oh!

github-actions Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Opus 4.8 Review — ✅ no blocking findings

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CrysisDeu commented Sep 5, 2026 •

edited

Loading

What still escapes on `main` (probe: `is_denied(...)` returned `None`)

Fix (implementation only, `src/kiro_crew/security.py`, +102/-5)

github-actions Bot commented Sep 5, 2026 •

edited

Loading

github-actions Bot commented Sep 5, 2026 •

edited

Loading

github-actions Bot commented Sep 5, 2026 •

edited

Loading

github-actions Bot commented Sep 5, 2026 •

edited

Loading