A fork's pull request gets a preview, on a maintainer's click - #220
Open
abernier wants to merge 5 commits into
Open
A fork's pull request gets a preview, on a maintainer's click#220abernier wants to merge 5 commits into
abernier wants to merge 5 commits into
Conversation
`preview-job` asked `secrets.VERCEL_TOKEN != ''` and skipped its own build when the answer was no, which on a fork's pull request it always is: pmndrs#219 had a green `preview-job` that ran a checkout, an install, and nothing else. Contributors who do not have push here are the ones with the least other way to look at what they changed. The token cannot be handed to that run -- everything it executes is the pull request's code, `pnpm install` included -- so the deploy moves to a second workflow that `workflow_run` starts back in this repository, with the secrets, after the first one finishes. What makes that one safe is that it runs none of that code: no checkout of the head, no install, no build. It downloads the artifact and uploads it. Which makes the artifact the boundary, and everything in it untrusted: - `.vercel/output/config.json` is written there, never taken from the artifact, and `out` can only become `static/`. Otherwise a pull request could land serverless functions on the account. - `base_path` has to travel with the build (the deploying workflow cannot run `configure-pages` without a permission it should not hold), and it reaches a `Location` header, so it is checked for the one thing that matters: a leading `//` is somebody else's host. - the sha for the deployment box comes from the `workflow_run` event, not from the artifact. The cost is a ~2min build that a fork's pull request used to skip, on a free runner, and a preview that arrives on its own run rather than as a check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The deploy job joins a `preview` environment, so its required reviewers hold it until someone says yes. Worth being exact about what that click is. It is not an approval of the pull request's code -- nothing in this workflow runs that code, and no click could honestly vouch for what a lockfile's `postinstall` scripts do, which is why the gated-`pull_request_target` version of this feature is weaker than it looks. It is an approval of a tarball of static files going onto our Vercel account. A question a human can answer by looking. So it stacks on the artifact boundary rather than replacing it: with no reviewers set the environment lets everything through, and the workflow is exactly as safe as it was in the previous commit. One ordering trap, and it is why this says so in the file: GitHub creates a missing environment on first use, unprotected. The reviewers have to exist before this merges, or the first preview sails past the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit put the maintainer's approval on the `preview`
environment, which is a real gate and is in the Actions tab. Nobody looks at
a pull request from there. Moving it to where the decision is actually made:
- a branch on this repository still previews by itself, on `workflow_run`,
as it did before the split. Nobody approves a colleague's preview.
- a fork previews when a maintainer puts the `preview` label on it. The
label comes straight back off, so the next click is another deploy.
Adding a label needs triage rights, so the authorization is GitHub's and not
ours to get wrong -- and the contributor cannot label their own pull request.
`pull_request_target` is what lets that event see the secrets. It is a
footgun when a workflow checks out the pull request's head and then executes
it; this one has no checkout at all, and the artifact boundary from two
commits ago is unchanged. The environment stays for the URL box on the run,
without reviewers: the label is the gate, and a second one in another tab is
the thing this commit is undoing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`workflow_run` fires when a run finishes, not when a job does. With `preview-job` still inside `ci.yml`, the deploy would have started after the eight test shards -- which is pmndrs#202 undone by the back door, three months after it was measured and fixed. Measured again, on pmndrs#220: | | | |---|---| | `preview-job` alone | 1.3min | | the `ci.yml` run it sat in | 6min | | the same run, pull request touching many examples | ~20min | So it moves to `preview-build.yml`, a workflow that contains it and nothing else, and finishes when it finishes. Nothing about the build moved with it: same runner, same install, same `BASE_PATH`, so the same `build2` hashes as `ci.yml` and the shards. The Actions cache -- turbo's and pnpm's both -- is scoped by repository and branch and never by workflow, so the sharing the old comment describes is unaffected. Permissions are `ci.yml`'s verbatim rather than a guess at the smaller set `configure-pages` needs. `main` has no branch protection, so no required check names to follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three commits of this were spent moving `preview-job` around so that `workflow_run` could deploy behind it. `workflow_run` was only ever there to keep the automatic preview automatic -- and `preview-job` already does that, today, for every pull request that can read the secrets. It never needed replacing. So `ci.yml` keeps its inline deploy, untouched: same job, same timing, same line in the checks, and pmndrs#202 stays fixed because nothing moved out of it. What a fork's pull request adds is one thing, an artifact, on the branch of the `if` that used to do nothing at all. `preview.yml` loses the `workflow_run` trigger with it, and is now what it should have been from the first commit: `pull_request_target` on a label, for a fork, on request. The whole of it against main is now +24 lines in `ci.yml` and a file that runs only when somebody presses the button. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
abernier
changed the title
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
1 participant
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.
preview-jobopens withPREVIEW: ${{ secrets.VERCEL_TOKEN != '' }}and skips its build, its deploy and its deployment box when the answer is no. On a fork's pull request the answer is always no -- secrets are not readable from apull_requestrun that came from a fork. #219 has a greenpreview-jobthat ran a checkout, an install, and then nothing, in 42s.So the contributors with the least other way to look at what they changed are the ones who get no preview.
Why the token cannot simply be given to that job
Everything
preview-jobruns is the pull request's own code.pnpm installalone executes arbitrarypostinstallscripts.pull_requestwithholding the secrets from a fork is not an obstacle to route around, it is the property being relied on -- andpull_request_targetwith a checkout of the head, or Vercel's own "build fork PRs", both hand that code a runner that can see the token.What this does
Nothing to the path that works.
preview-jobkeeps its inline deploy: same job, same timing, same line in the checks, and #202 stays fixed because nothing moved out ofci.yml.What a fork's pull request adds is one thing -- an artifact -- on the branch of the
ifthat used to do nothing at all:ci.yml--pnpm buildloses itsif, so a fork builds too (~1.3min, free runner). When there is no token to deploy with,outand the Pagesbase_pathgo up as an artifact instead, kept 7 days.preview.yml-- new,pull_request_targeton a label, fork pull requests only. Runs none of the pull request's code: no checkout of the head, no install, no build. Downloads that artifact and uploads it to Vercel.+24 lines in
ci.yml, and a file that runs only when somebody presses the button.The button: a
previewlabelA maintainer puts the
previewlabel on the pull request. One click, on the page where the decision is being made. The label comes straight back off, so the next click is another deploy.Applying a label needs triage rights on this repository -- 5 people today, against 55 collaborators on
read-- so the authorization is GitHub's own and not a check we could write wrong, and the contributor cannot label their own pull request.pull_request_targetis what lets that label event see the secrets. It is the usual footgun when a workflow checks out the pull request's head and then executes it; this one has no checkout at all. What the click decides is whether a tarball of static files goes onto the Vercel account -- a question a human can answer by looking. It is not an approval of the code: nothing here runs it, and no click could honestly vouch for what a lockfile'spostinstallscripts do.The artifact is the boundary
Everything inside it is untrusted input:
config.json,functions/preview.yml, never taken from the artifact -- otherwise a pull request could put serverless functions on the accountoutstatic/. Afunctions/written inside it lands understatic/as a file, not as code Vercel runsbase_pathLocation. Validated for the one thing that matters: a leading//is somebody else's hostWhat is left for a fork to reach: static files it wrote, on a
*.vercel.appURL, in a project with no environment variables and no domain of ours. Which is what a preview of its own code is.What it costs
A fork's pull request pays a ~1.3min build it used to skip, on a free runner, and 235MB of artifact for 7 days -- both only when a fork opens a pull request.
Seeing it work
Before merging: the
previewlabel has to exist on the repository, or there is nothing to click. It does now.pull_request_targetonly ever runs the copy of the file on the default branch, so the label does nothing until this is merged -- a pull request that adds the mechanism cannot demonstrate it on itself. What this one shows, being a fork's pull request fromabernier/pmndrs-examples:preview-jobbuilding and uploading thepreviewartifact with no secrets, where on #219 it skipped everything.Then, on the first fork pull request after the merge: #219 needs one fresh CI run for its artifact to exist (close and reopen it), and the label on it is the button.
馃 Generated with Claude Code