From 8d0df55398835cde40993357b608978cd679c8a4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 00:17:17 +0200 Subject: [PATCH] =?UTF-8?q?fix(629):=20review=20fixes=20=E2=80=94=20tilde?= =?UTF-8?q?=20fences,=20an=20unbounded=20sha=20field,=20and=20a=20forgeabl?= =?UTF-8?q?e=20comment=20boundary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-family review of 38a96f47 returned BLOCKED with three findings. All reproduced first: ~~~ fence -> positive fences were stripped for ``` only; markdown also takes ~~~ @ <40hex>f / ZZZ -> positive the sha matched {7,40} with NO right boundary, so an over-long or malformed token was TRUNCATED into a passing one \x01BODY-BOUNDARY\x01 -> positive an in-band separator joined comment bodies, so a body containing that line forged a boundary, reset fence state mid-comment, and exposed a verdict inside an unclosed fence Fixes: both fence markers honoured; the hex run matched whole, required to end at a non-alphanumeric boundary, with its length validated separately so an out-of-range token is rejected rather than trimmed to fit; and bodies carried OUT-OF-BAND (one JSON-encoded string per line), which removes the forgery class instead of escaping the sentinel. The third is the one worth remembering: an in-band delimiter is forgeable by whoever writes the data, and here that is anyone who can comment on the PR. Two of these fixes broke previously-green tests, both of which were right to break: - an over-long token now classifies `no-sha`, not `stale`. The fixture asserting `stale` was 45 hex chars, so it had been exercising the length guard while claiming to test the prefix rule. Rebuilt as a well-formed 40-char sha that contains the head prefix without starting with it. - `jq -e` exits 4 when a filter produces NO output, which is the legitimate empty-comment-list case. Treating it as an error turned "no comments yet" into an input error — and callers fail closed on those, so a new PR would have read as unclassifiable. Exit 4 is now accepted. 44 classifier tests, 155 total. `~~~` and the boundary fixes are each mutation-verified; the out-of-band fix has no equivalent mutation (it is structural, not a regex) so its evidence is the direct reproduction against the previous commit. refs #629 Co-Authored-By: Claude Opus 5 (1M context) Decisions-Edit: yes --- .../records/release/review-verdict-gate.md | 20 +++++++ scripts/check-review-verdict.sh | 60 ++++++++++++++----- scripts/tests/test_check_review_verdict.py | 53 +++++++++++++++- 3 files changed, 116 insertions(+), 17 deletions(-) diff --git a/docs/decisions/records/release/review-verdict-gate.md b/docs/decisions/records/release/review-verdict-gate.md index fc4aea04d..1d302fba5 100644 --- a/docs/decisions/records/release/review-verdict-gate.md +++ b/docs/decisions/records/release/review-verdict-gate.md @@ -45,11 +45,31 @@ lived inline in the hook with no tests, and each of these graded as a positive v from the verdict's **own** `@ ` field, the one immediately following the token, which also makes multi-`@` lines unambiguous. +A cross-family review of that first fix found **three more**, all reproduced before fixing — worth +recording because each is the same fix done half-way: + +- fences were stripped for ` ``` ` only, but markdown also accepts **`~~~`**; +- the sha field matched `{7,40}` with **no right boundary**, so an over-long or malformed token was + silently *truncated* into a passing one: `@ <40-hex-head>f` and `@ <40-hex-head>ZZZ` both graded as + a verdict for head. The hex run is now matched whole, must end at a non-alphanumeric boundary, and + its length is validated separately — an out-of-range token is rejected, never trimmed to fit; +- comment bodies were joined with a literal `\x01BODY-BOUNDARY\x01` line so fence state could reset + per comment. **An in-band delimiter is forgeable by whoever writes the data** — and here that is + anyone who can comment on the PR: a body containing that line reset the fence mid-comment and + exposed a verdict still inside an unclosed fence. Bodies are now carried out-of-band (one + JSON-encoded string per line), which removes the class rather than escaping the sentinel. + The lesson is about where a grammar lives, not about any one regex: this record described the intended behaviour accurately, the code did something looser, and nothing compared them. The grammar now lives in `scripts/check-review-verdict.sh` — one implementation, called by the hook and by `post-review-verdict.sh`'s cross-check, covered by `scripts/tests/test_check_review_verdict.py`. +Two of those fixes also broke previously-green tests, which is the useful part: an over-long token +became `no-sha` where a fixture expected `stale` (the fixture was 45 hex chars, so it had been +testing the length guard while claiming to test the prefix rule), and `jq -e` exits **4** on no +output, so an empty comment list started reading as a malformed payload — turning "no comments yet" +into an input error on a gate whose callers fail closed. + The classifications: - a MERGEABLE/APPROVED/LGTM verdict whose `@ ` is the current head → **allow**; - a **negative** verdict (BLOCKED/NOT-MERGEABLE) *on the head* → **deny**, and it *wins over* a positive diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 920cb8b54..4cca325de 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -66,24 +66,45 @@ comments=$(cat) # `jq -e` so malformed JSON is an input error (exit 2), never a silent "absent" — which would read as # "convention not adopted" and downgrade a hard block into an ask. -# A sentinel line after each body lets the fence stripper reset state per comment. -SEP=$'\001BODY-BOUNDARY\001' -bodies=$(printf '%s' "$comments" | jq -re --arg sep "$SEP" '[.[] | (.body // ""), $sep] | flatten | join("\n")' 2>/dev/null) || { - printf 'check-review-verdict: stdin is not a JSON array of comment objects\n' >&2; exit 2; } +# +# Each body is emitted as a JSON STRING on its own line (newlines escaped by JSON), so comment +# boundaries are carried out-of-band. An earlier version joined bodies with a literal sentinel line; +# a comment containing that sentinel could forge a boundary, reset fence state mid-body, and expose a +# verdict that was still inside an unclosed fence. In-band delimiters are forgeable by whoever writes +# the data — and here that is anyone who can comment on the PR. +encoded=$(printf '%s' "$comments" | jq -ce '.[] | (.body // "")' 2>/dev/null) +jq_rc=$? +# `jq -e` exits 4 when a filter produced NO output — which is exactly the legitimate empty-comment-list +# case, not a malformed payload. Treating it as an error turned "no comments yet" into an input error, +# and callers fail closed on those, so an unremarkable new PR would have read as unclassifiable. +if [ "$jq_rc" -ne 0 ] && [ "$jq_rc" -ne 4 ]; then + printf 'check-review-verdict: stdin is not a JSON array of comment objects\n' >&2; exit 2 +fi -# Strip fenced code blocks, resetting fence state at each comment boundary. -stripped=$(printf '%s\n' "$bodies" | awk -v sep="$SEP" ' - $0 == sep { fence = 0; next } - /^[[:space:]]*```/ { fence = !fence; next } - !fence { print } -') - -verdicts=$(printf '%s\n' "$stripped" | grep -iE '^[[:space:]]*review-verdict:' || true) +# Strip fenced code blocks per body, so fence state cannot leak between comments. Both fence markers +# markdown accepts are honoured: ``` and ~~~ (a verdict inside a `~~~` block was still counted). +verdicts="" +while IFS= read -r encoded_body; do + [ -n "$encoded_body" ] || continue + body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || continue + found=$(printf '%s\n' "$body" | awk ' + /^[[:space:]]*(```|~~~)/ { fence = !fence; next } + !fence { print } + ' | grep -iE '^[[:space:]]*review-verdict:' || true) + [ -n "$found" ] && verdicts="${verdicts}${found} +" +done <`. Two greps rather than a capture group, # because BSD/macOS grep has no -P and `sed -E` backreference portability is worse than this. -FIELD_RE='^[[:space:]]*review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]{7,40}' +# The hex run is matched WHOLE (`+`) and must end at a non-alphanumeric boundary or end-of-line, then +# its length is checked separately. Matching `{7,40}` directly had no right boundary, so an over-long +# or malformed token was silently TRUNCATED into a valid-looking one: `@ <40-hex-head>` and +# `@ <40-hex-head>ZZZ` both matched their first 40 chars and graded as a verdict for head. +FIELD_RE='^[[:space:]]*review-verdict:[[:space:]]*[A-Za-z][A-Za-z-]*[[:space:]]*@[[:space:]]*[0-9a-fA-F]+([^0-9a-zA-Z]|$)' POS_RE='^[[:space:]]*review-verdict:[[:space:]]*(mergeable|approved|lgtm)([[:space:]@]|$)' NEG_RE='^[[:space:]]*review-verdict:[[:space:]]*(blocked|not-mergeable)([[:space:]@]|$)' @@ -99,10 +120,17 @@ while IFS= read -r line; do continue fi - # The sha from THIS line's own field. Take the anchored field, then the trailing hex of it. + # The sha from THIS line's own field. Take the anchored field, then its LAST hex run — the sha sits + # after the token, and the field ends at the boundary, so the last run is always the candidate. field=$(printf '%s' "$line" | grep -ioE "$FIELD_RE" | head -1 || true) - ref=$(printf '%s' "$field" | grep -oiE '[0-9a-fA-F]{7,40}$' | tr 'A-F' 'a-f' || true) - [ -n "$ref" ] || continue # token recognized but no `@ ` field -> falls through to `no-sha` + ref=$(printf '%s' "$field" | grep -oE '[0-9a-fA-F]+' | tail -1 | tr 'A-F' 'a-f' || true) + # Length is validated HERE rather than in the regex, so an out-of-range token is rejected outright + # instead of being truncated to a passing prefix. + case "${#ref}" in + 7|8|9|1[0-9]|2[0-9]|3[0-9]|40) : ;; + *) ref="" ;; + esac + [ -n "$ref" ] || continue # token recognized but no valid `@ ` field -> falls through to `no-sha` case "$head" in "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index 9ada0990f..e7557ed35 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -77,6 +77,49 @@ def test_fence_state_does_not_leak_between_comments(): assert classify(["Example:\n```\nnot a verdict", verdict("MERGEABLE", HEAD)]) == ("positive", 0) +# --- found by cross-family review of the first fix (all three reproduced before fixing) ---------- + + +def test_falseopen_tilde_fence_is_also_stripped(): + """Markdown accepts `~~~` as well as ```; stripping only backticks left the hole half-open.""" + body = f"~~~\n{verdict('MERGEABLE', HEAD)}\n~~~" + assert classify([body]) == ("absent", 0) + + +@pytest.mark.parametrize( + "suffix", + [ + "f", # a 41st hex char -> over-long, must not truncate to a valid 40 + "9999", # a longer wrong sha + "ZZZ", # a non-hex suffix welded to a valid sha + ], +) +def test_falseopen_sha_field_needs_a_right_boundary(suffix): + """Matching `{7,40}` with no right boundary silently TRUNCATED a malformed token into a match. + + `@ <40-hex-head>` matched its first 40 characters and graded as a verdict for head. + """ + assert classify([verdict("MERGEABLE", HEAD + suffix)]) == ("no-sha", 0) + + +def test_falseopen_a_body_cannot_forge_a_comment_boundary(): + """The separator between comments must be out-of-band. + + An earlier version joined bodies with a literal `\\x01BODY-BOUNDARY\\x01` line. A comment + containing that line could reset fence state mid-body and expose a verdict still inside an + unclosed fence — an in-band delimiter is forgeable by whoever writes the data, and here that is + anyone who can comment on the PR. + """ + sep = "\x01BODY-BOUNDARY\x01" + body = f"```\n{sep}\n{verdict('MERGEABLE', HEAD)}" + assert classify([body]) == ("absent", 0) + + +def test_a_valid_verdict_may_carry_trailing_prose(): + """The right boundary must not reject the ordinary `@ (note)` shape.""" + assert classify([f"{verdict('MERGEABLE', HEAD)} (all findings resolved)"]) == ("positive", 0) + + # --- the happy paths ------------------------------------------------------------------------- @@ -131,7 +174,15 @@ def test_verdict_only_for_an_older_commit_is_stale(): def test_old_sha_containing_the_head_prefix_does_not_match(): - contains_head_prefix = "abcdef" + SHORT + "1234567890abcdef1234567890abcdef" + """A VALID 40-char sha that contains the head prefix but does not start with it is stale. + + The fixture is deliberately a well-formed sha: an over-long hex run is now rejected as malformed + (`no-sha`) rather than compared, so a 45-char fixture would have tested the length guard instead + of the prefix rule it is named for. + """ + contains_head_prefix = ("abcdef" + SHORT + "0" * 40)[:40] + assert len(contains_head_prefix) == 40 + assert SHORT in contains_head_prefix and not contains_head_prefix.startswith(SHORT) assert classify([verdict("MERGEABLE", contains_head_prefix)]) == ("stale", 0)