timothy/ersatztv
1
0
Fork 0
Code Issues 51 Pull Requests 5 Actions Packages Projects Releases 9 Wiki Activity

fix(629): re-review fixes — the READ path must fail closed too

Browse Source 
Re-review of 8d0df553 returned BLOCKED. Both findings are the same class as the bugs this
issue is about, one level down: the reader, not the grammar.

1. A here-document makes bash materialise a temp file. When that fails (read-only or
   restricted TMPDIR) the loop reads NOTHING, and the classifier returned `absent` — silently
   discarding a real BLOCKED verdict. Reproduced with TMPDIR=/nonexistent: `absent` before,
   `negative` now. Both loops are off here-docs (a pipe for the reader, process substitution
   for the classifier so its flags stay in the current shell).

   "The environment could not supply a temp file" is not evidence that a PR was approved.

2. A malformed payload degraded to `absent` instead of an input error: an object-valued
   `.body` reached the scanner and simply matched nothing. Shape is now asserted in jq — must
   be an array, bodies must be strings — so it exits 2 and callers fail closed.

Also closed a pre-existing false-open the review found while probing (present before #629, so
not a regression, but cheap to fix here): bash strips NULs in command substitution, so a body
of `Review<NUL>-verdict: MERGEABLE @ <head>` arrived at the matcher as a valid verdict line —
text that is not a verdict becoming one in transit. Bodies containing NUL are now rejected.

161 tests. Verified: 80k body still classifies; empty array, null body still `absent`;
malformed JSON, non-array, array-of-non-objects, object body, NUL body all exit 2; a hostile
TMPDIR still returns `negative`.

refs #629

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
timothy
2026-07-26 00:47:40 +02:00
co-authored by Claude Opus 5
parent f151b93245
commit 3bfd925baf
2 changed files with 84 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
scripts/check-review-verdict.sh
+33 -15
View File
@@ -72,30 +72,50 @@ comments=$(cat)
# a comment containing that sentinel could forge a boundary, reset fence state mid-body, and expose a # 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 # 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. # the data — and here that is anyone who can comment on the PR.
encoded=$(printf '%s' "$comments" | jq -ce '.[] | (.body // "")' 2>/dev/null) # The shape is asserted IN jq so a payload that isn't an array of comment objects with STRING bodies
# is an input error (exit 2), not a silent `absent`. An object-valued `.body` used to reach the
# scanner and simply match nothing — a malformed payload reading as "no verdict posted" is a
# fail-OPEN on a gate whose whole job is to withhold approval.
# A body containing a NUL is rejected outright: bash strips NULs in command substitution, so
# `Review<NUL>-verdict: MERGEABLE @ <head>` would arrive at the matcher as a valid verdict line —
# text that is not a verdict silently becoming one.
encoded=$(printf '%s' "$comments" | jq -ce '
if type != "array" then error("not an array") else .[] end
| (.body // "")
| if type != "string" then error("non-string body")
elif contains("\u0000") then error("NUL in body")
else . end' 2>/dev/null)
jq_rc=$? jq_rc=$?
# `jq -e` exits 4 when a filter produced NO output — which is exactly the legitimate empty-comment-list # `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, # 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. # 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 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 printf 'check-review-verdict: stdin is not a JSON array of comment objects with string bodies\n' >&2
exit 2
fi fi
# Strip fenced code blocks per body, so fence state cannot leak between comments. Both fence markers # 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). # markdown accepts are honoured: ``` and ~~~ (a verdict inside a `~~~` block was still counted).
verdicts="" #
while IFS= read -r encoded_body; do # Fed by a PIPE, not a here-document. A here-doc makes bash materialise a temp file, and when that
# fails (read-only or restricted TMPDIR) the loop silently reads nothing — the classifier returns
# `absent` and a real BLOCKED verdict disappears. Gate failures must never land on the permissive
# side, and "the environment could not supply a temp file" is not evidence that a PR was approved.
# The loop runs in a subshell, so its result is captured through stdout rather than a variable.
verdicts=$(printf '%s\n' "$encoded" | while IFS= read -r encoded_body; do
[ -n "$encoded_body" ] || continue [ -n "$encoded_body" ] || continue
body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || continue # A body that fails to decode is fatal, not skippable — skipping one could drop the only BLOCKED
found=$(printf '%s\n' "$body" | awk ' # verdict on the PR. 3 is distinct from the exit-2 paths above so the caller sees where it broke.
body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || exit 3
printf '%s\n' "$body" | awk '
/^[[:space:]]*(```|~~~)/ { fence = !fence; next } /^[[:space:]]*(```|~~~)/ { fence = !fence; next }
!fence { print } !fence { print }
' | grep -iE '^[[:space:]]*review-verdict:' || true) ' | grep -iE '^[[:space:]]*review-verdict:' || true
[ -n "$found" ] && verdicts="${verdicts}${found} done)
" verdict_rc=$?
done <<ENCODED if [ "$verdict_rc" -ne 0 ]; then
$encoded printf 'check-review-verdict: failed to read comment bodies (rc=%s)\n' "$verdict_rc" >&2; exit 2
ENCODED fi
[ -n "$(printf '%s' "$verdicts" | tr -d '[:space:]')" ] || { printf 'absent\n'; exit 0; } [ -n "$(printf '%s' "$verdicts" | tr -d '[:space:]')" ] || { printf 'absent\n'; exit 0; }
# The verdict field, anchored: token then its own `@ <sha>`. Two greps rather than a capture group, # The verdict field, anchored: token then its own `@ <sha>`. Two greps rather than a capture group,
@@ -136,9 +156,7 @@ while IFS= read -r line; do
"$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;; "$ref"*) if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi ;;
*) stale=1 ;; *) stale=1 ;;
esac esac
done <<VERDICTS done < <(printf '%s\n' "$verdicts")
$verdicts
VERDICTS
if [ "$head_neg" = 1 ]; then printf 'negative\n'; exit 0; fi if [ "$head_neg" = 1 ]; then printf 'negative\n'; exit 0; fi
if [ "$head_pos" = 1 ]; then printf 'positive\n'; exit 0; fi if [ "$head_pos" = 1 ]; then printf 'positive\n'; exit 0; fi
scripts/tests/test_check_review_verdict.py
+51
View File
@@ -10,6 +10,7 @@ tests rather than descriptions.
""" """
import json import json
import os
import subprocess import subprocess
from pathlib import Path from pathlib import Path
@@ -257,6 +258,56 @@ def test_missing_head_argument_is_an_input_error():
assert p.returncode == 2 assert p.returncode == 2
# --- found by re-review of the fix commit: the READ path must fail closed too ------------------
def test_a_hostile_tmpdir_does_not_hide_a_verdict():
"""A gate failure must never land on the permissive side.
Reading bodies via a here-document makes bash materialise a temp file; when that fails the loop
reads nothing and the classifier returned `absent` — silently discarding a real BLOCKED verdict.
"""
env = {**os.environ, "TMPDIR": "/nonexistent-dir-for-this-test"}
payload = json.dumps([{"body": verdict("BLOCKED", HEAD)}])
p = subprocess.run(
["bash", str(SCRIPT), "--head", HEAD],
input=payload,
capture_output=True,
text=True,
env=env,
)
assert (p.stdout.strip(), p.returncode) == ("negative", 0)
def test_a_very_large_body_still_classifies():
body = ("x" * 80000) + "\n" + verdict("BLOCKED", HEAD)
assert classify([body]) == ("negative", 0)
@pytest.mark.parametrize(
"payload",
[
'[{"body": {"nested": "Review-verdict: MERGEABLE @ ' + HEAD + '"}}]', # object body
"{}", # not an array
"[1,2,3]", # array of non-objects
],
)
def test_malformed_shapes_are_input_errors_not_silent_absent(payload):
"""`absent` reads as "no verdict posted", which is a fail-OPEN for a malformed payload."""
p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input=payload, capture_output=True, text=True)
assert p.returncode == 2, f"got rc={p.returncode} out={p.stdout!r}"
def test_a_nul_in_the_body_is_rejected():
"""bash strips NULs in command substitution, so `Review<NUL>-verdict:` would arrive as a verdict.
Text that is not a verdict must not become one on the way through the reader.
"""
payload = json.dumps([{"body": "Review" + chr(0) + "-verdict: MERGEABLE @ " + HEAD}])
p = subprocess.run(["bash", str(SCRIPT), "--head", HEAD], input=payload, capture_output=True, text=True)
assert p.returncode == 2, f"got rc={p.returncode} out={p.stdout!r}"
def test_non_hex_head_is_an_input_error(): def test_non_hex_head_is_an_input_error():
p = subprocess.run( p = subprocess.run(
["bash", str(SCRIPT), "--head", "refs/heads/main"], ["bash", str(SCRIPT), "--head", "refs/heads/main"],