From 95b4259aa6e8163b1d3726da7f0e4e52945f98f4 Mon Sep 17 00:00:00 2001 From: Loup-Garou911XD <90267658+Loup-Garou911XD@users.noreply.github.com> Date: Tue, 8 Sep 2026 12:02:33 +0530 Subject: [PATCH] ci: resolve the PR by head repo + branch, not by commit sha PR Apply has never once applied fixups to a fork PR. It resolved the target PR through repos/{repo}/commits/{sha}/pulls, and that endpoint cannot answer for a fork PR's head commit: the commit is not reachable from any ref of this repo, so the association index has nothing to return and the call yields [] forever. This is not eventual consistency that a longer poll would ride out. The failure used to be invisible. Before the loud-failure change the lookup fell through to `skip`, which exits 0, so the job painted itself green having applied nothing - which is how #479 reached main unstamped. Since then it fails loudly instead, correctly, and every fork PR has gone red at that step (runs 34018273545, 34023650032, 34040502180, 34042614720, 34048956687, 34192652242). Look the PR up by head repo owner + head branch instead. That filter is exact, is available the moment the PR exists, and narrows on precisely the fields HARD RULE 3 already requires the resolved PR to match. Nothing is relaxed: state, base, head repo, head branch and head sha are all still pinned against the workflow_run payload before anything is applied, so a run can still only ever write to the branch it came from. state=all, not state=open, so a PR merged between PR Check and this run still resolves and takes the existing "PR not open; skipping" path rather than looking unidentifiable. Several PRs from one head branch is ordinary history for a reused branch, not the sha-adoption ambiguity HARD RULE 3 guards against - every candidate shares the same head repo and branch by construction - so the newest by number wins instead of failing closed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSZrwNCXvibEJ9PXCW1uqi --- .github/workflows/ci-apply.yml | 80 ++++++++++++++++++++++++---------- 1 file changed, 57 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci-apply.yml b/.github/workflows/ci-apply.yml index 546267a1..5d1e9ef2 100644 --- a/.github/workflows/ci-apply.yml +++ b/.github/workflows/ci-apply.yml @@ -21,10 +21,12 @@ name: PR Apply # 3. NEVER identify the target PR by commit sha alone. A sha is a value, not an # identity: forks share object storage, so anyone can push ANOTHER PR's head # commit onto a branch of their own, open a PR at it, and then close that PR -# mid-run so the commit -> PR lookup below resolves to the victim's PR - at -# which point this job would push the attacker's artifact to the victim's -# branch. The resolved PR must be pinned to workflow_run.head_repository AND +# mid-run so a commit -> PR lookup resolves to the victim's PR - at which +# point this job would push the attacker's artifact to the victim's branch. +# The resolved PR must be pinned to workflow_run.head_repository AND # head_branch AND head_sha, so a run can only ever write to its own branch. +# The lookup below therefore searches by head repo + head branch and then +# re-checks all three; do not "simplify" it back to a sha lookup. # # On trust: a fork PR fully controls ci-check.yml itself (GitHub runs the # workflow file from the PR's own merge ref for `pull_request` events - that @@ -58,10 +60,10 @@ jobs: # Resolves which PR this run belongs to using ONLY trusted inputs: the # workflow_run payload (set by GitHub, not forgeable by the PR author) # and the REST API. workflow_run.pull_requests is empty for fork PRs, - # hence the commit -> PR association lookup. That lookup ANSWERS with a - # PR but does not PROVE it is this run's PR - a sha can be adopted by any - # fork even though it cannot be forged - so the result is pinned to the - # payload's head repo/branch/sha below. See HARD RULE 3 in the header. + # hence the lookup by head repo + head branch below. That lookup ANSWERS + # with a PR but does not PROVE it is this run's PR, so the result is + # pinned to the payload's head repo/branch/sha before anything is + # applied. See HARD RULE 3 in the header. - name: Resolve and validate PR (trusted sources only) id: pr env: @@ -80,9 +82,27 @@ jobs: exit 1 } - # The sha -> PR association is eventually consistent: a run that - # starts seconds after the contributor's push can legitimately see - # an empty list. Poll before giving up. + # The PR is looked up by HEAD REPO + HEAD BRANCH, not through the + # sha -> PR association endpoint (repos/{repo}/commits/{sha}/pulls). + # That endpoint does not answer for a fork PR: the head commit is not + # reachable from any ref of THIS repo, so the association index has + # nothing to return and the call yields [] indefinitely - it is not + # slow, it never resolves. It has in fact never resolved a fork PR + # here; the runs that looked green before the loud-failure change + # were the old `skip` path exiting 0 on "Not exactly one PR + # associated" having applied nothing at all. + # + # Listing by head is exact and available the moment the PR exists, + # and it narrows on precisely the two payload fields HARD RULE 3 + # already demands the resolved PR match. Every pin below - head repo, + # head branch, head sha, state, base - still has to hold, so this + # relaxes nothing: it only replaces a lookup that returns nothing + # with one that returns the branch's PRs. + # + # `state=all` rather than `state=open` so a PR closed or merged + # between PR Check and this run still resolves and takes the + # "PR not open; skipping" path below, instead of looking like a PR + # that could not be identified at all. # # Both terminal outcomes below FAIL rather than `skip`. `skip` exits # 0, which paints the whole job green - and a green PR Apply that @@ -92,30 +112,44 @@ jobs: # none of our business (PR closed, not targeting main). "I could not # identify the PR" is neither: it means the fixups were dropped on # the floor, and that must be visible. + + # Both values land in a URL below, so they are shape-checked here + # rather than trusted for being payload-supplied. `head` takes the + # OWNER of the head repo, not its full name. + RUN_HEAD_OWNER="${RUN_HEAD_REPO%%/*}" + [[ "$RUN_HEAD_OWNER" =~ ^[A-Za-z0-9._-]+$ ]] || { + echo "Unexpected workflow_run head repo shape; refusing." >&2 + exit 1 + } + [[ "$RUN_HEAD_BRANCH" =~ ^[A-Za-z0-9._/-]{1,255}$ ]] || { + echo "Unexpected workflow_run head branch shape; refusing." >&2 + exit 1 + } + HEAD_FILTER="${RUN_HEAD_OWNER}:${RUN_HEAD_BRANCH}" + for attempt in 1 2 3 4 5; do - gh api "repos/${REPO}/commits/${RUN_HEAD_SHA}/pulls" > pulls.json + gh api "repos/${REPO}/pulls?state=all&per_page=100&head=${HEAD_FILTER}" \ + > pulls.json N_PULLS="$(jq 'length' pulls.json)" [ "$N_PULLS" = "0" ] || break - echo "No PR associated with ${RUN_HEAD_SHA} yet (attempt ${attempt}/5); retrying in 10s." + echo "No PR for ${HEAD_FILTER} yet (attempt ${attempt}/5); retrying in 10s." sleep 10 done if [ "$N_PULLS" = "0" ]; then - echo "No PR associated with ${RUN_HEAD_SHA} after 5 attempts." >&2 + echo "No PR found for head ${HEAD_FILTER} after 5 attempts." >&2 echo "Formatting/metadata fixups were NOT applied to the branch." >&2 exit 1 fi - # More than one PR sharing a head sha is the ambiguity HARD RULE 3 - # exists for: pushing would risk writing to the wrong branch. Fail - # closed, but fail loudly - silence here is what bit us before. - if [ "$N_PULLS" != "1" ]; then - echo "${N_PULLS} PRs share head ${RUN_HEAD_SHA}; cannot resolve which one this run belongs to." >&2 - echo "Failing closed: nothing was applied. A maintainer must apply the fixups manually." >&2 - exit 1 - fi - - PR_NUMBER="$(jq -r '.[0].number' pulls.json)" + # Several PRs from one head branch is ordinary history - a branch + # reused after an earlier PR was closed - and NOT the ambiguity HARD + # RULE 3 guards against: every candidate here shares the same head + # repo and branch by construction, so whichever is picked, this job + # can only ever write to the branch the run came from. Take the + # newest by PR number (explicitly, rather than leaning on the API's + # default ordering); if it is stale or closed the pins below skip. + PR_NUMBER="$(jq -r 'max_by(.number) | .number' pulls.json)" [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || skip "Bad PR number; skipping." gh pr view "$PR_NUMBER" --repo "$REPO" \