Repository navigation
fix: bind the heredoc sink allowlist to its own clause, fix the closing-delimiter scan, and drop the stale reserved-name exception - #485
Merged
Conversation
…ng-delimiter scan, and drop the stale reserved-name exception (#447, #455) #447: jit_heredoc_opener_is_known_sink() checked the WHOLE line for a known data sink, so a decoy sink separated by ; or && (cat > /dev/null </dev/null; bash <<EOF) made the allowlist match a different, non-sink command's own heredoc -- including the round-1 fake-arithmetic-opener shape hiding behind the same decoy. The closing-delimiter scan also kept only one optional quote or backslash on each side of the identifier, so EOF"X", EOF-1 and E\OF all closed at the wrong line. Binds the sink check (and the danger-token check) to the operator's own clause, and replays bash's own word-formation rule to compute the real delimiter. #455: _only_reserved_name_error() in smoke_release_tree.py downgraded `claude plugin validate --strict`'s reserved-name error to a warning -- moot since #452 renamed the plugin away from the claude- prefix that triggered it -- and could separately mask a second real error under an unrecognized heading. Removed entirely; any validate failure now fails the smoke test. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…, skip-vs-fail exit code, engine-matrix coverage Explore review and oss:auditor both ran against commit 2fe319c. Three real findings, all fixed here; one real but pre-existing (not a #447 regression) finding logged to trap.d/ rather than fixed inline or filed, since it ranks non-blocking (misreports) in the manager skill's own table. - jit_heredoc_clause_at() scanned for a bare ";"/"&" with no quote tracking, so a quoted one inside the operator's own clause could forge a fake boundary. Threaded the per-line quote state through it (the same quote_in[i] jit_heredoc_opener_is_suppressed already uses) so a ";"/"&" only ends a clause when the state at that character is 0. Also updated two stale "whole line"/"same line" comments left behind by the earlier clause-binding change. - tests/test-smoke-reserved-name-445.sh's new wrapper folded a legitimate SKIPPED (exit 2, no /bin/sh to run the fake claude shim on) into the script's own exit 1 -- tests/run-all.sh reads exit 2 as SKIPPED and anything else as FAILED, so this would have mislabeled a platform-driven skip as a real failure on a leg with no /bin/sh (Windows). Fixed to propagate exit 2 separately from the static check's own exit 1. - The new #447 assertions (jit_heredoc_scan_word/jit_heredoc_clause_at) only ran under the default $PATH awk, not the awk/gawk/mawk matrix this file already runs #442's own heredoc state-machine assertions through. Extended the matrix to cover them too. Logged to trap.d/447.sink-regexes-quote-blind.md: jit_heredoc_opener_is_known_sink()'s five sink regexes are quote-blind, so a quoted ";"+sink-keyword inside a non-sink command's own argument can still false-match as a sink. Confirmed this reproduces identically on bda0282 (before any #447 change), so it is not a regression; fixing it is a separate, larger change to the matching primitives themselves. Co-Authored-By: Claude Sonnet 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #447
Closes #455
#447 -- heredoc sink allowlist fail-opened on 10 reproducible shapes
jit_heredoc_opener_is_known_sink()checked the WHOLE physical line for a known data sink (cat>/tee/supertool/git commit -F -/gh --body-file -), so a decoy sink sharing the line with a heredoc operator, separated by;/&, made the allowlist match even though the heredoc belonged to a different, non-sink command (e.g.cat > /dev/null </dev/null; bash <<EOF). This also let round-1's fake-arithmetic-opener bug ((( y = 1<<EOF ))) hide behind the same decoy. Separately, the closing-delimiter scan kept only one optional quote/backslash character on each side of the bare identifier, so delimiters bash itself builds by concatenation (<<EOF"X",<<EOF-1,<<E\OF) were misread.Fix:
jit_heredoc_clause_at()binds both the sink check and the danger-token check to just the clause containing the<<operator (quote-aware, bounded by the nearest unquoted;/&), andjit_heredoc_scan_word()replays bash's own word-formation rule character by character to compute the real closing delimiter.TDD: 8 new assertions in
tests/test-pre-tool-hook.shwritten before the fix, verified red (7/8 failing) by stashing onlyscripts/common-awk.sh, green after restoring. Verified identical behavior across /usr/bin/awk, /usr/local/bin/gawk and /opt/homebrew/bin/mawk.#455 -- stale reserved-name exception in the release smoke test
_only_reserved_name_error()in.github/scripts/smoke_release_tree.pydowngraded exactly oneclaude plugin validate --stricterror (a reserved-name error) to a warning -- moot since #452 renamed the plugin away from theclaude-prefix that triggered it -- and had a separate parsing bug where a second real error under an unrecognized per-file heading was never counted, letting a genuine second error through. Removed the function and its call site entirely so any validate failure now fails the smoke test outright.TDD: rewrote
tests/test-smoke-reserved-name-445.shto assert the exception is gone, verified red (3/7) against the unfixed source, green (7/7) after.Self-review
Two reviewers (an Explore-type reviewer and
oss:auditor) ran against the committed diff. Findings and dispositions:jit_heredoc_clause_at()was quote-unaware (a bare;/&scan), reopening a shape of the jit_strip_heredoc_body()'s sink allowlist still fail-opens on 10 reproducible shapes (carried forward from v0.12.0 gate 3 round 2) #447 bypass. Fixed by threading the existing per-line quote state through the clause scan. A deeper, related bug was found during verification of this fix -- the sink/danger-token regexes themselves are quote-blind and a quoted sink-keyword-and-separator sequence still matches -- confirmed to reproduce identically onmainbefore this PR, so it is not a regression here; logged totrap.d/447.sink-regexes-quote-blind.mdas out-of-scope follow-up rather than fixed inline./bin/sh, e.g. Windows) into the same exit code as FAIL. Fixed by propagating the two exit codes separately.$PATHawk, not the file's existing awk/gawk/mawk matrix. Fixed by extending the per-engine loop with the new assertions.Tests
tests/test-pre-tool-hook.sh: 422/422 (was 398/398 before this change).tests/test-smoke-reserved-name-445.sh: 7/7.python3 .oss/assemble_changelog.py --check: passed. Full suite not run as the gating check (CI is the three-platform authority per this repo's own convention); it was run once early to rule out two pre-existing, unrelated failures as regressions.🤖 Generated with Claude Code
[AI-generated]