diff --git a/.github/workflows/ds-shots-comment.yml b/.github/workflows/ds-shots-comment.yml index 4ad515deab..7057b65c0d 100644 --- a/.github/workflows/ds-shots-comment.yml +++ b/.github/workflows/ds-shots-comment.yml @@ -28,20 +28,24 @@ # any operational API failure → red job, comment untouched # # Supersession is enforced by head-SHA binding, not by cancellation. The -# concurrency group below serializes publishers for the SAME head only — -# cancel-in-progress stays false, because one branch can hold two open PRs -# (dev and main base) whose publishers must both run. Serialization closes -# the read-then-write race where a no-report run could overwrite the result -# a sibling same-head run posted between its marker read and its PATCH; -# once serialized, either order ends correctly, because the marker-head -# guard makes a no-report run keep any comment already posted for its head. -# queue: max keeps EVERY same-head publisher (FIFO, up to 100) — the default -# one-slot queue would let a third run drop a sibling PR's only pending -# publisher and leave that PR's comment stale. +# concurrency group below serializes ALL publishers for one head branch. +# Every writer of a given PR's comment lives in that group: report-path +# runs come from the PR's own branch, and the stale path only touches PRs +# of the run's branch. That closes both write races — a no-report run +# overwriting a sibling same-head result between its marker read and its +# PATCH, and an older-head publisher stalling past its bind check and +# overwriting a newer head's comment. Execution order inside the group +# does not matter: a run that executes after a newer push fails the head +# binding and no-ops. cancel-in-progress stays false and queue: max holds +# up to 100 pending runs (more than that are cancelled — far beyond real +# traffic for one branch) — one branch can hold two open PRs (dev and +# main base) whose publishers must both run, and the default one-slot +# queue would silently drop one of them. The group is repo-qualified so a +# fork branch sharing a name never lands in an origin branch's queue. # -# Deploy note: workflow_run only fires once this file exists on the DEFAULT -# branch (main). It lands on dev first, so the comment starts appearing after -# the next dev → main release. Until then this file is inert. +# Deploy note: workflow_run workflows execute the DEFAULT branch's copy of +# this file. It went live on main via the #2912 hotfix; keep the dev copy +# byte-identical so every dev → main release merges over it cleanly. name: ds-shots comment on: @@ -55,7 +59,7 @@ permissions: pull-requests: write # the sticky comment concurrency: - group: ds-shots-comment-${{ github.event.workflow_run.head_sha }} + group: ds-shots-comment-${{ github.event.workflow_run.head_repository.full_name || 'unknown' }}-${{ github.event.workflow_run.head_branch }} cancel-in-progress: false queue: max @@ -74,8 +78,11 @@ jobs: timeout-minutes: 5 steps: # For a workflow_run event github.sha is the default branch head, - # so this checks out trusted code — never the PR. + # so this checks out trusted code — never the PR. No git commands + # run after checkout, so the token is not persisted. - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-node@v4 with: @@ -128,6 +135,9 @@ jobs: REPO: ${{ github.repository }} PR: ${{ steps.art.outputs.pr }} RUN_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + RUN_HEAD_BRANCH: ${{ github.event.workflow_run.head_branch }} + # numbers only, joined — safe to hand to the shell + EVENT_PRS: ${{ join(github.event.workflow_run.pull_requests.*.number, ' ') }} run: | set -euo pipefail # The artifact name — and so the PR number — was chosen by @@ -136,12 +146,18 @@ jobs: # artifact after somebody else's PR posts nothing, and an # out-of-order older run cannot overwrite a newer comment. # + # The PR must also live in THIS repo and on the run's own + # head branch: any branch — including a fork's copy with + # the same name — can park on a public commit, and only + # same-repo same-branch writers share the concurrency + # group that serializes comment updates. + # # A true 404 (the number points at no PR) is that attack and # stays a no-op. Any other failure — 5xx, rate limit, # network — must fail this job loudly: swallowed, it would # exit green while an older result stands as current. set +e - OUT=$(gh api "repos/$REPO/pulls/$PR" --jq '.head.sha' 2>&1) + OUT=$(gh api "repos/$REPO/pulls/$PR" --jq '(.head.repo.full_name // "gone") + " " + .head.sha + " " + .head.ref' 2>&1) STATUS=$? set -e if [ "$STATUS" -ne 0 ]; then @@ -154,11 +170,54 @@ jobs: exit 1 fi HEAD=$OUT - if [ "$HEAD" != "$RUN_HEAD_SHA" ]; then - echo "PR #$PR head ${HEAD:-} != run head $RUN_HEAD_SHA — mismatched artifact name or stale run. Skipping." + if [ "$HEAD" != "$REPO $RUN_HEAD_SHA $RUN_HEAD_BRANCH" ]; then + echo "PR #$PR head (${HEAD:-}) != run head ($REPO $RUN_HEAD_SHA $RUN_HEAD_BRANCH) — mismatched artifact name, stale run, or cross-repo/branch writer. Skipping." echo 'match=false' >> "$GITHUB_OUTPUT" exit 0 fi + # Trusted cross-check when GitHub supplies it: the event's + # pull_requests array names the run's originating PR(s). + # The array is documented-unreliable (often empty), so an + # empty array falls back to the sha+ref binding above + # instead of silencing the publisher. When it is present, + # the artifact's number must be in it — which closes the + # last gap in the binding: a sibling PR on the SAME branch + # and commit (same author, same tree) could otherwise be + # named by the artifact and pass sha+ref. + if [ -n "$EVENT_PRS" ]; then + case " $EVENT_PRS " in + *" $PR "*) ;; + *) + echo "PR #$PR is not among the run's originating PRs ($EVENT_PRS) — mismatched artifact name. Skipping." + echo 'match=false' >> "$GITHUB_OUTPUT" + exit 0 + ;; + esac + else + # The event omitted the list — routine, ~9 in 10 runs. + # Resolve it ourselves: the artifact's PR must be the + # ONLY open same-repo PR on this branch+commit. Fork + # PRs are excluded, or an outsider could point a + # same-named fork branch at this public commit and + # force a false ambiguity that mutes the publisher. With two sibling + # PRs the runs are indistinguishable — code from one + # can name the other — so ambiguity posts nothing and + # the job summary stays the visible diff for that rare + # configuration. + # --paginate matters: this endpoint lists every PR whose + # branch CONTAINS the commit (a dev-tip commit sits in + # dozens of rebased branches), and the jq filter runs + # after pagination — page one alone can miss the real + # head match. + CANDIDATES=$(gh api --paginate "repos/$REPO/commits/$RUN_HEAD_SHA/pulls?per_page=100" \ + --jq '.[] | select(.state == "open" and .head.sha == env.RUN_HEAD_SHA and .head.ref == env.RUN_HEAD_BRANCH and (.head.repo.full_name // "") == env.REPO) | .number') + COUNT=$(printf '%s\n' "$CANDIDATES" | grep -c . || true) + if [ "$COUNT" -ne 1 ] || ! printf '%s\n' "$CANDIDATES" | grep -qx "$PR"; then + echo "Ambiguous or mismatched binding: open PRs on this branch+commit [$CANDIDATES] vs artifact PR #$PR — posting nothing." + echo 'match=false' >> "$GITHUB_OUTPUT" + exit 0 + fi + fi echo 'match=true' >> "$GITHUB_OUTPUT" # The download is a plain API call: a transient failure here is @@ -197,11 +256,11 @@ jobs: # from the (untrusted) archive is ever written to disk, so # zip-slip has nothing to work with. A missing member fails # the step loudly. - unzip -p report.zip report.json > report.json + unzip -p report.zip report.json > extracted-report.json if [ -n "$IMG_ID" ]; then export ARTIFACT_URL="https://github.com/$REPO/actions/runs/$RUN_ID/artifacts/$IMG_ID" fi - node scripts/ds-shots-publish.mjs report.json > body.md + node scripts/ds-shots-publish.mjs extracted-report.json > body.md echo 'ok=true' >> "$GITHUB_OUTPUT" # A report that exists but cannot be used must not leave the @@ -253,8 +312,11 @@ jobs: set -euo pipefail # env.* inside the jq program, not shell interpolation — a # branch name must never be able to edit the filter. - CANDIDATES=$(gh api "repos/$REPO/commits/$RUN_HEAD_SHA/pulls" \ - --jq '.[] | select(.state == "open" and .head.sha == env.RUN_HEAD_SHA and .head.ref == env.HEAD_BRANCH) | .number') + # --paginate for the same reason as in the bind step: the + # endpoint lists PRs containing the commit, and the head + # match can sit past page one. + CANDIDATES=$(gh api --paginate "repos/$REPO/commits/$RUN_HEAD_SHA/pulls?per_page=100" \ + --jq '.[] | select(.state == "open" and .head.sha == env.RUN_HEAD_SHA and .head.ref == env.HEAD_BRANCH and (.head.repo.full_name // "") == env.REPO) | .number') if [ -z "$CANDIDATES" ]; then echo 'No open PR has this branch+commit as its head — superseded or stale run. Nothing to do.' exit 0 diff --git a/scripts/__tests__/ds-shots-publish.test.js b/scripts/__tests__/ds-shots-publish.test.js index 22258205ff..8471398fb2 100644 --- a/scripts/__tests__/ds-shots-publish.test.js +++ b/scripts/__tests__/ds-shots-publish.test.js @@ -69,7 +69,6 @@ describe('ds-shots-publish', () => { ['newline smuggling', { file: 'a\n## fake@375.png', percent: 1, pixels: 1 }], ['overlong name', { file: `${'a'.repeat(200)}@375.png`, percent: 1, pixels: 1 }], ['non-string file', { file: 42, percent: 1, pixels: 1 }], - ['NaN percent', { file: 'home@375.png', percent: NaN, pixels: 1 }], ['percent out of range', { file: 'home@375.png', percent: 101, pixels: 1 }], ['string percent', { file: 'home@375.png', percent: '3.2', pixels: 1 }], ['injected note', { file: 'home@375.png', percent: 1, pixels: 1, note: '](x)