mosaicstack/stack
16
0
Fork 1
Code Issues 330 Pull Requests 12 Actions Packages Projects Releases 15 Wiki Activity

pr-review.sh: no expected-commit argument, so a caller cannot pin the SHA it reviewed #1223

New Issue
Open
opened 2026-08-15 03:32:27 +00:00 by Ghost · 1 comment
Ghost commented 2026-08-15 03:32:27 +00:00
Copy Link
Copy Source

pr-review.sh binds a review to a head it reads itself, so a caller cannot pin the commit it actually reviewed. Any wrapper-driven review pipeline therefore has an unclosable race: it can post a valid approval on a commit nobody examined.

The seam

pr-review.sh resolves the head on its own (gitea_pr_head_sha), sends that value as commit_id, and then verifies the created review matches it. That closes the wrapper's own read→submit race, and it is correct as far as it goes.

What it cannot close is the caller→wrapper race, because there is no way to tell it which commit the caller reviewed:

  1. caller builds and evaluates commit A;
  2. caller re-reads the PR head, sees A, decides it is safe to post;
  3. author pushes B;
  4. caller invokes pr-review.sh, which independently reads the head, sees B, and binds the review to B;
  5. the wrapper's self-check passes — it compared B against B — and exits 0.

The result is a review whose commit_id is B, whose body describes A, and whose exit status says success. Nothing in the chain is detectably wrong at any single step.

The failure is worst in the approve direction: an approval on an unexamined commit is indistinguishable from a real one, and under a dismiss_stale_approvals protection it is exactly the state that would otherwise have been cleared.

Requested change

Accept an expected-commit argument, e.g. --expect-head <sha40>:

  • when supplied, compare it against the live head before submitting and refuse on mismatch, rather than silently retargeting;
  • bind commit_id to the caller-supplied value, so the posted object records the commit that was actually reviewed;
  • keep the existing self-read behaviour when the flag is absent, so no current caller changes.

This makes the reviewed commit an input rather than something the wrapper re-derives, which is the only shape that lets a caller prove what its verdict applies to.

Why a caller-side workaround is not sufficient

A caller can read the review back afterwards and compare commit_id to what it reviewed, and that is worth doing — but the review is public by then and cannot be unposted. The best a caller can manage on its own is a loud self-retraction after the fact. Refusing before submission needs the wrapper.

Related

Same shape as the two-resolver identity seam: a value is resolved independently in two places, nothing binds them, and neither the exit code nor the success message reports the disagreement. The fix shape is the same one — one handle, resolved once, used by both.

Found while a docs-review pipeline was being independently security-reviewed; the reviewer traced the data flow and demonstrated the gap. No credential or operator-specific detail is involved.

`pr-review.sh` binds a review to a head it reads itself, so a caller cannot pin the commit it actually reviewed. Any wrapper-driven review pipeline therefore has an unclosable race: it can post a valid approval on a commit nobody examined. ## The seam `pr-review.sh` resolves the head on its own (`gitea_pr_head_sha`), sends that value as `commit_id`, and then verifies the created review matches it. That closes the wrapper's **own** read→submit race, and it is correct as far as it goes. What it cannot close is the **caller→wrapper** race, because there is no way to tell it which commit the caller reviewed: 1. caller builds and evaluates commit `A`; 2. caller re-reads the PR head, sees `A`, decides it is safe to post; 3. **author pushes `B`**; 4. caller invokes `pr-review.sh`, which independently reads the head, sees `B`, and binds the review to `B`; 5. the wrapper's self-check passes — it compared `B` against `B` — and exits 0. The result is a review whose `commit_id` is `B`, whose body describes `A`, and whose exit status says success. Nothing in the chain is detectably wrong at any single step. The failure is worst in the approve direction: an approval on an unexamined commit is indistinguishable from a real one, and under a `dismiss_stale_approvals` protection it is exactly the state that would otherwise have been cleared. ## Requested change Accept an expected-commit argument, e.g. `--expect-head <sha40>`: - when supplied, compare it against the live head **before** submitting and refuse on mismatch, rather than silently retargeting; - bind `commit_id` to the caller-supplied value, so the posted object records the commit that was actually reviewed; - keep the existing self-read behaviour when the flag is absent, so no current caller changes. This makes the reviewed commit an input rather than something the wrapper re-derives, which is the only shape that lets a caller prove what its verdict applies to. ## Why a caller-side workaround is not sufficient A caller can read the review back afterwards and compare `commit_id` to what it reviewed, and that is worth doing — but the review is public by then and cannot be unposted. The best a caller can manage on its own is a loud self-retraction after the fact. Refusing before submission needs the wrapper. ## Related Same shape as the two-resolver identity seam: a value is resolved independently in two places, nothing binds them, and neither the exit code nor the success message reports the disagreement. The fix shape is the same one — one handle, resolved once, used by both. Found while a docs-review pipeline was being independently security-reviewed; the reviewer traced the data flow and demonstrated the gap. No credential or operator-specific detail is involved.
Ghost commented 2026-08-15 21:34:31 +00:00
Copy Link
Copy Source

A second, smaller half of the same property: the bound SHA is verified and then discarded

Verified in pr-review.sh at head today. The wrapper does bind and check the commit — it sends
"commit_id": os.environ["REVIEW_COMMIT"] on submit, refuses when the read-back's commit_id
differs from expected_head, and re-reads the head afterwards to catch an advance between submit
and verification. All of that is right.

What it does not do is tell the caller which SHA it bound. The three success lines are:

Approved and verified Gitea PR #N (review ID <id>)
Requested changes and verified on Gitea PR #N (review ID <id>)
Added and verified comment on Gitea PR #N (comment ID <id>)

and stdout carries the created id alone. The SHA exists in the process — it is the value the
wrapper just refused to disagree with — and it is dropped on the way out.

Why that is not merely cosmetic

A caller that wants to record "this verdict applies to commit X" in its own audit trail has only
two routes today, and both are the seam this issue is about:

  1. Re-derive the head itself. That is a second independent resolution of the same value, which
    is exactly the shape the issue argues against — and it is resolved at a different instant from
    the one the wrapper used, so it can differ.
  2. Read the review back. There is no wrapper that lists reviews, so this is a hand-rolled raw
    query per caller.

So the value that makes a verdict traceable is available exactly once, inside the one component
that already got it right, and cannot be obtained by anyone else without reintroducing the race.

Suggested addition

Emit it machine-readably alongside the created id — e.g. review_id=<id> commit_id=<sha40> on
stdout, or a --porcelain mode if changing the current stdout contract is unwelcome. Both success
messages should also name the SHA, since an operator reading a pane is a caller too.

This is the same patch as --expect-head, not a separate change: once the reviewed commit is an
input, echoing it back is what lets the caller prove the input was honoured. Without the echo,
--expect-head is checked by the wrapper and unverifiable by anyone else.

Related: the missing review-list wrapper is the other reason route 2 above is hand-rolled.

## A second, smaller half of the same property: the bound SHA is verified and then discarded Verified in `pr-review.sh` at head today. The wrapper does bind and check the commit — it sends `"commit_id": os.environ["REVIEW_COMMIT"]` on submit, refuses when the read-back's `commit_id` differs from `expected_head`, and re-reads the head afterwards to catch an advance between submit and verification. All of that is right. What it does not do is **tell the caller which SHA it bound.** The three success lines are: ``` Approved and verified Gitea PR #N (review ID <id>) Requested changes and verified on Gitea PR #N (review ID <id>) Added and verified comment on Gitea PR #N (comment ID <id>) ``` and stdout carries the created id alone. The SHA exists in the process — it is the value the wrapper just refused to disagree with — and it is dropped on the way out. ## Why that is not merely cosmetic A caller that wants to record *"this verdict applies to commit X"* in its own audit trail has only two routes today, and both are the seam this issue is about: 1. **Re-derive the head itself.** That is a second independent resolution of the same value, which is exactly the shape the issue argues against — and it is resolved at a different instant from the one the wrapper used, so it can differ. 2. **Read the review back.** There is no wrapper that lists reviews, so this is a hand-rolled raw query per caller. So the value that makes a verdict traceable is available exactly once, inside the one component that already got it right, and cannot be obtained by anyone else without reintroducing the race. ## Suggested addition Emit it machine-readably alongside the created id — e.g. `review_id=<id> commit_id=<sha40>` on stdout, or a `--porcelain` mode if changing the current stdout contract is unwelcome. Both success messages should also name the SHA, since an operator reading a pane is a caller too. This is the same patch as `--expect-head`, not a separate change: once the reviewed commit is an *input*, echoing it back is what lets the caller prove the input was honoured. Without the echo, `--expect-head` is checked by the wrapper and unverifiable by anyone else. Related: the missing review-list wrapper is the other reason route 2 above is hand-rolled.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
code-be-01 (Mosaic fleet seat code-be-01) code-be-02 (Mosaic fleet seat code-be-02) code-infra-01 (Mosaic fleet seat code-infra-01) fargo fred gate-merge-01 (Mosaic fleet seat gate-merge-01) happy jason.woltje (Jason Woltje) merge-gate ops-01 (Mosaic fleet seat ops-01) ops-02 (Mosaic fleet seat ops-02) ops-03 (Mosaic fleet seat ops-03) ops-ci-01 (Mosaic fleet seat ops-ci-01) ops-deploy-01 (Mosaic fleet seat ops-deploy-01) pepper rev-code-01 rev-code-02 rev-security-01 rev-security-02 rev-security-03 (Mosaic fleet seat rev-security-03) sanity scooby (Scooby) scrappy shaggy tiny velma veronica (Mosaic fleet seat veronica) vision woodpecker
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1223