PR previews: Resolve fork pull requests when publishing - #528
Merged
Conversation
Look the pull request up by head repository and branch instead of by commit. listPullRequestsAssociatedWithCommit does not resolve commits that live in a fork, so every fork PR failed the resolve step and never got a Playground preview. The head SHA still has to match before anything is published, and head_repository and head_branch come from the same trusted workflow_run payload as head_sha, so the trust model is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The head filter is server-side and fails open: a value GitHub does not recognise is ignored rather than rejected, and the call returns every PR. Re-check owner, branch and SHA against the workflow_run payload so the ownership guarantee holds locally. Also drops the comment's reference to a pr-meta artifact, which pr-preview-build.yml does not produce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
pr-preview-publish.ymlnow resolves the pull request by head repository and branch instead of by commit. The lookup moves fromlistPullRequestsAssociatedWithCommittopulls.listwith ahead=owner:branchfilter.Owner, branch and head SHA are then re-checked locally against the
workflow_runpayload, so the resolved PR has to match on all three before the artifact is exposed or the comment posted.The query uses
state: 'all'rather than open PRs only, so a PR that merges between the build finishing and this run starting still publishes, as it did before. When a branch matches more than one PR, the open one wins.Why are these changes being made?
Because fork pull requests never got a Playground preview.
listPullRequestsAssociatedWithCommitdoes not resolve commits that live in a fork, so the resolve step failed and every later step was skipped:The endpoint returns nothing for fork heads and one result for same-repo heads:
Neither #513 nor #443 has ever received a preview comment. This is not a regression from the recent action bumps: the same error hit a run on 6 August, and in the failing run above the
actions/checkoutstep succeeded and only the resolve step failed.The workflow already knew about half of this. Its comment notes that
workflow_run.pull_requestsis empty for fork PRs and reaches for the commits API as the workaround, but that API has the same blind spot.Why ownership is checked locally
head_repositoryandhead_branchare set by GitHub on theworkflow_runpayload, exactly likehead_sha, and PR code cannot spoof them. The owner segment of the filter is what stops a fork from matching someone else's PR, and it is enforced:But the filter is server-side and fails open. A value GitHub does not parse is ignored rather than rejected, and the call returns everything:
The
!headOwner || !headBranchguard makes a malformed value impossible today, since git refs cannot contain:and logins are alphanumeric, so this is not a live bug. It is still the whole ownership guarantee resting on remote filter behaviour that nothing local would notice changing. Re-checkinghead.repo.owner.login,head.refandhead.shaagainst the payload turns it into a local invariant for one line.head.repois nullable when the head fork has been deleted, hence the optional chaining.The comment block also drops its reference to a
pr-metaartifact. No such artifact exists:pr-preview-build.ymluploads onlybuilt-plugin, and its own header says it deliberately emits no PR metadata. The reasoning it was making is kept, without naming a file that is not produced.Testing Instructions
The
workflow_runtrigger always runs the copy of this file that is on the default branch, so the end-to-end path can only be exercised after merge. Steps 1 to 4 can be run now.1. The new lookup resolves both fork and same-repo PRs
Run each of these and make sure a PR number and head SHA come back:
Then pick any open same-repo PR and run the same call with
head=WordPress:<its branch>. Make sure it also returns one row. That is the regression check: same-repo PRs must keep working.2. The old lookup is the thing that was broken
gh api repos/WordPress/openstation/commits/00dba781c2747209cf17be014ab65a1364cc4e49/pulls -q 'length'Make sure it prints
0, while step 1 resolved the same branch to #513.3. The local check survives a fail-open response
Fetch the worst case, where the filter is ignored and every PR comes back, then run the resolution logic against it:
node -e "const prs=require('/tmp/allprs.json');const r=(o,b,s)=>{const m=prs.filter(p=>p.head.sha===s&&p.head.repo?.owner?.login===o&&p.head.ref===b);const p=m.find(x=>x.state==='open')??m[0];return p?'#'+p.number:'REFUSED'};const t=prs.find(p=>p.number===513);console.log('size',prs.length);console.log('correct ->',r(t.head.repo.owner.login,t.head.ref,t.head.sha));console.log('bad owner ->',r('attacker',t.head.ref,t.head.sha));console.log('bad branch->',r(t.head.repo.owner.login,'other',t.head.sha));console.log('bad sha ->',r(t.head.repo.owner.login,t.head.ref,'0'.repeat(40)))"Make sure the response size is 100, the correct triple resolves to
#513, and all three tampered variants printREFUSED.4. The file still parses
5. After merge, a fork PR publishes
trunkinto the branch is enough).6. After merge, a same-repo PR still publishes
WordPress/openstationbranch.7. After merge, a stale run still refuses
No pull request for <owner>:<branch> at head SHA ...and publishes nothing, and that the run for the newer SHA succeeds.Automated
Make sure
git statusis clean afterwards. No source files change here, so the build is only a sync check.