diff --git a/scripts/check-review-verdict.sh b/scripts/check-review-verdict.sh index 4cca325de..690109297 100755 --- a/scripts/check-review-verdict.sh +++ b/scripts/check-review-verdict.sh @@ -72,30 +72,50 @@ comments=$(cat) # 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) +# 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-verdict: MERGEABLE @ ` 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 -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 + printf 'check-review-verdict: stdin is not a JSON array of comment objects with string bodies\n' >&2 + exit 2 fi # 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 +# +# 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 - body=$(printf '%s' "$encoded_body" | jq -r '.' 2>/dev/null) || continue - found=$(printf '%s\n' "$body" | awk ' + # A body that fails to decode is fatal, not skippable — skipping one could drop the only BLOCKED + # 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 } !fence { print } - ' | grep -iE '^[[:space:]]*review-verdict:' || true) - [ -n "$found" ] && verdicts="${verdicts}${found} -" -done <&2; exit 2 +fi [ -n "$(printf '%s' "$verdicts" | tr -d '[:space:]')" ] || { printf 'absent\n'; exit 0; } # The verdict field, anchored: token then its own `@ `. 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 ;; *) stale=1 ;; esac -done <-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(): p = subprocess.run( ["bash", str(SCRIPT), "--head", "refs/heads/main"],