chore: back-merge main into dev (ds-shots publisher hotfix, TASK-22044)

.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:-<none>} != 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:-<none>}) != 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

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) <script>' }],
@@ -93,6 +92,10 @@ describe('ds-shots-publish', () => {
         ['negative unchanged', valid({ unchanged: -1 })],
         ['array report', [1, 2, 3]],
         ['not json at all', 'not json {'],
+        // raw text on purpose: JSON.stringify would turn non-finite numbers
+        // into null, and JSON's own path to Infinity is a 1e999 literal
+        ['infinite percent', JSON.stringify(valid()).replace('3.21', '1e999')],
+        ['oversized file', 'x'.repeat(1024 * 1024 + 10)],
     ])('rejects: %s', (_label, report) => {
         const result = run(report)
 

scripts/ds-shots-publish.mjs

@@ -27,7 +27,7 @@
 // publisher goes red and posts nothing, which is the wanted outcome for a
 // tampered report.
 
-import { readFileSync, statSync } from 'node:fs'
+import { closeSync, openSync, readSync } from 'node:fs'
 
 // The marker carries the head the comment describes, so a later run with no
 // report can tell a current comment (keep) from a stale one (clear) without
@@ -53,11 +53,25 @@ const fail = (why) => {
 
 const [reportPath] = process.argv.slice(2)
 if (!reportPath) fail('no report path given')
-if (statSync(reportPath).size > MAX_BYTES) fail('report too large')
+
+// One bounded read instead of stat-then-read: no size-check race (codeql
+// js/file-system-race) and never more than MAX_BYTES+1 bytes of a hostile
+// file in memory, however large it is on disk.
+let raw
+try {
+    const fd = openSync(reportPath, 'r')
+    const buf = Buffer.alloc(MAX_BYTES + 1)
+    const bytes = readSync(fd, buf, 0, MAX_BYTES + 1, 0)
+    closeSync(fd)
+    if (bytes > MAX_BYTES) fail('report too large')
+    raw = buf.toString('utf8', 0, bytes)
+} catch {
+    fail('cannot read the report file')
+}
 
 let report
 try {
-    report = JSON.parse(readFileSync(reportPath, 'utf8'))
+    report = JSON.parse(raw)
 } catch {
     fail('not valid json')
 }