ci: publish preview builds from fork PRs via GitHub OIDC - #2387
Merged
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
fengmk2
added a commit
to voidzero-dev/pkg-pr-registry-bridge
that referenced
this pull request
Aug 10, 2026
Implements RFC 0002 (rfcs/0002-zero-trust-github-oidc-publishing.md), steps 1 and 2 of its rollout: the bridge-side verifier and the action's pack/upload mode split. Consumer wiring is voidzero-dev/vite-plus#2387. The three publish endpoints now accept either the operator's admin token or a short-lived GitHub-signed OIDC token, so a consumer repo can publish without storing a bridge credential and fork PRs can publish at all. /-/purge stays admin-only: an OIDC identity may add preview builds and nothing else. Verification is hand-rolled on WebCrypto. RS256 is hardcoded on both the key import and the verify, so the token header never selects an algorithm; the key comes from GitHub's JWKS by kid (KV-cached, memoized per isolate, unknown-kid refetch behind a cooldown); every length is capped before any crypto runs; and identity is anchored on the immutable repository_id and repository_owner_id ahead of workflow_ref, because that claim embeds a repository NAME and names can be renamed, transferred, or released and reclaimed. The action gains a `mode` input. `pack` writes raw pnpm pack output and holds no credentials, so it can run in a job building an untrusted fork PR; `upload` validates every archive against a canonical policy, rebuilds it, and publishes from a trusted workflow_run job; `publish` is the original single-job path, unchanged, and is the only one that may not use OIDC (it runs prepack scripts). Also fixes, all found while implementing or in review: - /-/register took prUrl from the caller and derived the pr-<n> dist-tag from it, so any publisher could retarget another PR's tag and change what VP_PR_VERSION installs. Predates this work. - nanotar's parser reports mtime in seconds while its writer expects milliseconds, so the repack divided by 1000 twice and stamped 1970-01-06. Pinning the mtime also makes rebuilds deterministic, so republishing a commit lands on the same content-addressed key. - The repack passed the tar header's mode straight through, so a crafted setuid/setgid/sticky bit survived into the published tarball. - nanotar's writer truncates names past the 100-byte ustar field, so two paths sharing their first 100 bytes collapsed into one entry. Refused now, in the writer, so publish mode is covered too. Deploying this alone is safe: with the OIDC vars unset the admin path is unchanged and a JWT is a plain 401, which is the configuration the staging smoke runs under. Note the four OIDC_* vars must be set together; a partial config returns 503 for every publish, including admin-token ones.
fengmk2
force-pushed
the
feat/oidc-preview-publish
branch
from
August 10, 2026 02:21
fd59e70 to
c327c57
Compare
fengmk2
marked this pull request as ready for review
August 10, 2026 02:21
wan9chi
approved these changes
Aug 10, 2026
fengmk2
force-pushed
the
feat/oidc-preview-publish
branch
from
August 10, 2026 03:14
c327c57 to
fbcdf88
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
fengmk2
force-pushed
the
feat/oidc-preview-publish
branch
from
August 10, 2026 04:21
390d8f1 to
0a63b58
Compare
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Splits preview publishing into an untrusted build leg and a trusted publish leg so external contributors' PRs can be installed and reviewed. Until now the bridge step was gated on the PR coming from this repo, because GitHub withholds secrets from fork pull_request runs. Fork PRs therefore got no preview build, no install instructions, and no Docker image, which is most of the value of the preview flow for exactly the contributions that need review most. publish-preview.yml keeps building, but now only packs (`mode: pack`, no network, no credentials) and uploads a workflow artifact. The new publish-preview-register.yml triggers on it completing, runs from main in base-repo context, and mints a GitHub Actions OIDC token that the bridge verifies against GitHub's JWKS. No bridge secret in this repo. The `authorize` job is the security boundary, not the label check in the build leg: on pull_request events GitHub runs the workflow file from the merge ref, so a PR author can edit that file to delete its own gate, or add a workflow with a matching name to trigger the trusted leg. So `authorize` re-resolves the PR from workflow_run.head_sha via the API, requires it to be open against this repo and currently labeled preview-build, and fails closed otherwise. It also supplies the PR url, which the bridge maps to the pr-<n> dist-tag; taking that from the artifact would let one PR retarget what VP_PR_VERSION installs for another. The job holding id-token does nothing but move bytes. The comment and Docker jobs are separate jobs with their own permissions, so the job that runs a preview package's install scripts cannot mint a publish token. The Docker preview stays same-repo only for now. It installs the preview package and pushes to ghcr.io/voidzero-dev/vite-plus, and before this change a fork could not reach it (fork runs get a read-only token, so the push failed). Running in base-repo context is what would make it succeed, so whether to put unreviewed fork code under the org's namespace is left as a deliberate decision: drop the is-fork condition to enable it. Fork builds get a warning banner naming the source fork above the install instructions, since those lines are official-looking and end in `curl | bash`. Requires on the bridge side (voidzero-dev/pkg-pr-registry-bridge#86): OIDC_AUDIENCE, OIDC_TRUSTED_WORKFLOWS, OIDC_TRUSTED_REPOSITORY_ID and OIDC_TRUSTED_OWNER_ID set on the production Worker. The action pin here points at that PR's head and must be repointed to a main SHA before this merges.
The bridge branch was rewritten to sign two commits that were created unsigned, so 4b0fcc2 no longer exists on it. Still a temporary pin: both must move to a main SHA once pkg-pr-registry-bridge#86 merges.
voidzero-dev/pkg-pr-registry-bridge#86 is merged and deployed, so both pins move from that PR's branch head to the resulting main commit and the TODO comments go away. Worth noting the old pin was stale even against the branch: fd9ad0e predated the simplify pass and the second review round, so it would have run the action without the SR-6 hardening (tar size-field agreement, artifact size and count bounds, the long-path refusal). Verified action.yml and the bundled dist resolve at the new SHA.
The Security Analysis job fails on the new workflow with dangerous-triggers: "workflow_run is almost always used insecurely" (1 high, exit 14). The warning is a good default and the danger it names is real: this file runs in base-repo context and is triggered by a run a fork PR can cause. But the trigger is unavoidable. Fork pull_request runs are denied both secrets and id-token, so publishing a fork's build requires a second workflow in base-repo context; there is no variant that keeps the publish in the build leg. Suppressed inline with the reasoning next to it, listing what a reviewer must not remove: the authorize job that re-derives authorization from the API and fails closed, trusting only GitHub-signed payload fields, pinning the artifact download to the triggering run, and keeping id-token away from any job that executes preview content. Inline rather than a repo-wide .github/zizmor.yml, since the repo has no zizmor config today and this should not become a blanket exemption for future workflow_run uses.
Adds a human approval on the one job that mints a bridge credential. The authorize job proves the PR is open and carries the label; this proves someone looked at the specific run before a token existed, and records who. It is the only control here that does not depend on my own logic being right: if authorize were ever weakened or removed, an unattended publish still cannot happen. Environment `preview-publish` created with the same five reviewers as the existing `release` environment. prevent_self_review is false, unlike release: the label is already a maintainer action, so requiring a SECOND person for a preview build seemed like more friction than the risk warrants. Flip it to true if you want two-person control. Note the failure mode this comment exists to prevent: a workflow referencing a MISSING environment gets one created implicitly with no protection rules, so it reads as a gate while enforcing nothing. The environment must exist with rules configured, which it now does. Cost: every preview publish now waits for an approval click, on top of the label. Downstream comment and Docker jobs wait with it.
Matches the `preview-build` label it gates and the existing `release` environment naming, rather than the ad-hoc `preview-publish` I used first. Self-review stays allowed, and the comment now says why: the label is already a maintainer action, so requiring a second person for every external contributor's preview would cost more than the risk warrants. What the gate buys is a deliberate confirmation on the one job that mints a credential, plus an audit trail of who approved it, not two-person control. Same five reviewers as `release`. The superseded `preview-publish` environment is deleted; it had no deployments and nothing referenced it.
From four parallel quality reviews. No intended behavior change except the label trigger, noted below. Waste: - Only the `preview-build` label triggers a build now. The gate was `contains(labels.*.name, ...)`, which is true whenever the label is already present, so adding ANY unrelated label to an already-labeled PR re-ran the full 8-target Rust matrix. Roughly 3-5 runner-hours per spurious trigger, and since the split it also queues a required-reviewer approval a human has to dismiss. Removing and re-adding the label still re-runs. - Docker preview gets a gha layer cache. The Dockerfile always comes from the default branch, so its apt layer is identical across previews, and this build sits on the post-approval critical path where someone is watching. - The artifact upload stops re-deflating ~19 already-gzipped .tgz files. Clarity: - The two header blocks restated each other (fork/secrets/id-token, the signed payload fields, the label caveat, and the RFC link all appeared twice). Folded into one, with the load-bearing controls as a single list. - Build-leg job renamed to "Pack preview packages"; it had the display name "Pkg Preview", which the trusted leg's publish job also has. main is not branch-protected, so no required check depends on the old name. - The `workflow_run` name coupling is now noted on both sides. - Two comments were wrong: nothing "registers below" in the build leg any more, and a fork PR did reach the Docker job before the split, it failed at the push. - The package list no longer claims to equal the action's default, which nothing enforces. Permissions and dead config: - `contents: read` dropped from `publish`: no checkout, and the bridge action makes no GitHub API calls. - `continue-on-error` dropped from `comment`. It guarded a step inside the old publish job; as a leaf job its only remaining effect was hiding failures. - `issues: write` KEPT on `comment`, with the explanation this split had dropped restored. It looks removable next to comment-docker-preview, which runs with less, but that job only ever updates an existing comment. - `IMAGE` was declared per job and its tag spelled three ways; hoisted to workflow scope. Skipped, with reasons: - Extracting the sticky-comment upsert into a composite action. It is genuinely duplicated (5 copies repo-wide), but a local composite needs actions/checkout in jobs that currently need none, and fixing 2 of 5 copies is not the win. Worth doing repo-wide, separately. - Folding comment-docker-preview into the Docker job: would put pull-requests:write on the job that builds preview content. - Deriving `is-fork` from `head-repo`: computing the predicate once is a feature, not duplication. - Removing the rolldown-binaries step: plausibly unused, but it predates this diff and a wrong call breaks packing. One review finding was a false positive: the RFC 0002 cross-reference is not dead, that agent read a stale clone. The link now names the file rather than the number.
Two review findings, both valid. P1: two open PRs can share a head commit (the same branch opened against two bases, for instance), and `find` took an arbitrary first match. The workflow_run payload does not say which PR caused the run, and `pull_requests` is empty for forks, so there is no way to pick correctly. The harmless direction is a labeled PR's run picking an unlabeled one and failing. The direction that matters is the reverse: an unlabeled PR's run picking a labeled one, borrowing its authorization, and publishing under that PR's tag. Now refuses when more than one candidate matches, naming them. Checked the resolution against one open PR, two sharing a sha, one open plus one closed, none, and a PR against a different base repo. P2: the artifact had retention-days: 1, sized for "consumed minutes later by the workflow_run job". That predates the required-reviewer environment. The publish job now waits on approval, which GitHub keeps pending far longer than a day, so an approval given the next morning would hit an expired artifact. Raised to 7 days, which covers a weekend without keeping ~150-300MB around indefinitely.
Two review findings. The second is a regression I introduced. Revalidate after the approval wait. `authorize` runs BEFORE the environment gate, so its verdict is a snapshot from potentially days ago — and the artifact is now retained 7 days precisely to allow that wait. During it the PR can be closed, lose the `preview-build` label (which is how a maintainer revokes consent), or advance to a new head commit. The last one matters most: publishing then moves the pr-<n> dist-tag BACKWARDS onto an older commit than the PR points at, so VP_PR_VERSION serves stale code. The publish job now re-asserts open + labeled + same head, after the wait and before a token exists, and names every reason it refused. Verified against unchanged, closed, unlabeled, advanced head, and all three at once. Require the artifact before requesting approval. My previous cleanup narrowed the build leg's gate to `github.event.label.name == 'preview-build'` to stop unrelated labels re-running the Rust matrix. That worked, but the WORKFLOW still triggers on every `labeled` event, so an unrelated label now produces a run where all jobs skip — and an all-skipped run still concludes "success". The PR legitimately still carries the label, so authorize passed and queued an environment approval for a run that built nothing, which could only end in a failed download. So I traded runner-hours for a pointless human approval request, which is the same cost I was trying to remove. `authorize` now checks the triggering run actually produced bridge-packages, which also catches a build leg that succeeded without uploading. It skips rather than fails: adding an unrelated label to a labeled PR is a normal action and a red X on each one would be noise.
fengmk2
force-pushed
the
feat/oidc-preview-publish
branch
from
August 10, 2026 05:18
06d3395 to
897791d
Compare
fengmk2
added a commit
that referenced
this pull request
Aug 10, 2026
Two fixes to `main`, both found by the first real runs of the publishing workflow. It could not be exercised before merge, because `workflow_run` only fires for workflow files already on the default branch. **First, the good news: the design works.** PR #2328 published end to end through the new path with an OIDC token, no admin token involved. `authorize`, `Pkg Preview`, and the sticky comment all succeeded, and `commit.a7180fa85c06fad48` is on the bridge: ``` commit.a7180fa85c06fad48 | pr: .../pull/2328 | at: 2026-08-10T07:35:31.617Z ``` ## 1. Fork PRs could not be resolved at all #2391 (from `liangmiQwQ`) failed in `authorize` with `no open PR of voidzero-dev/vite-plus has head c7e51be…` while that PR was open with exactly that head. `listPullRequestsAssociatedWithCommit` returns **empty** for a fork PR's head commit. Confirmed against the live API: | commit | result | | --- | --- | | #2387 head (same-repo) | returns `#2387` | | #2391 head (fork) | **empty** | So it worked for every case reachable before merge and failed for the only case this feature exists for. `workflow_run.pull_requests` is empty for forks too, which is what sent me to the commit endpoint originally — I swapped one fork-blind source for another. Now resolves via `pulls?state=open&head=<head_owner>:<head_branch>`, both GitHub-signed payload fields. The head-sha match is a separate step so the message distinguishes "no such PR" from "the PR moved on": ``` fork PR 2391 (real failure) -> OK: #2391 labeled=true fork=true stale head -> FAIL: PR #2391 now at c7e51be, built 0000000 no such branch -> FAIL: no open PR from liangmiQwQ:does-not-exist ``` I re-checked the rest of the publishing workflow for the same blind spot. Everything else keys off the PR number or the run id, which are base-repo objects and fork-safe: the post-approval `pulls.get` re-check returns correct state, head and labels for #2391, and the artifact download and the `listWorkflowRunArtifacts` precondition both see that run's 148MB `bridge-packages`. ## 2. The Docker gha cache broke the image push ``` #14 exporting to GitHub Actions Cache #14 ERROR: error writing layer blob: failed to reserve cache #13 exporting to image ... CANCELED ``` The cache export is fatal to the build, so it cancelled the push. I added this in the cleanup pass; it broke the job it was meant to speed up, and #2328's npm preview published while its Docker image did not. Reverted rather than repaired. Making it work needs `actions: write` on the one job that installs and executes the preview package, which is the job SR-5 says to keep unprivileged, and this was the only `type=gha` usage in the repo so there was no working precedent. It was saving 60-90s of apt on a path that already waits on a human approval measured in minutes to days. ## 3. Terminology "Trusted leg" and "build leg" were my own coinage and meant nothing to a reader who was not in the design conversation. The two workflows are now described as **the build workflow** and **the publishing workflow**, and where trust was the point the property is stated rather than encoded in a name. This also surfaced something worth fixing later: `publish-preview.yml` is named "Publish preview build" and no longer publishes anything. Renaming it is the real fix, but the publishing workflow matches it by `name:`, so that has to be a coordinated change. The header says so outright for now. The same terminology fix for the RFC and bridge docs is voidzero-dev/pkg-pr-registry-bridge#93, which also corrects SR-1 for the fork-blind endpoint above. ## After merging Re-label #2391 to get the first genuine fork preview.
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.
Lets fork pull requests publish preview builds, so external contributions can actually be installed and reviewed.
Why
The bridge step was gated on
head.repo.full_name == github.repository, because GitHub withholds secrets from forkpull_requestruns. Fork PRs got no preview build, no install instructions and no Docker image, so reviewers could not try an external contributor's change.What
Preview publishing splits in two, because a fork's build job cannot hold a credential under any configuration: GitHub's rule is that secrets are never passed to a fork-triggered run, and it cannot be otherwise, since the fork controls the workflow file on
pull_request.publish-preview.yml(untrusted, same trigger) builds and now only packs, via the action'smode: pack. No network, no credentials, noid-token. Runs for forks.publish-preview-register.yml(new, trusted) runs frommainin base-repo context onworkflow_run, and mints a GitHub Actions OIDC token the bridge verifies against GitHub's JWKS. No bridge secret in this repo.The part worth reviewing closely
The label check in the build leg is not the security boundary. On
pull_requestevents GitHub runs the workflow file from the merge ref, so a PR author can editpublish-preview.ymlto delete its ownpreview-buildgate, or add a workflow with a matchingname:to trigger the trusted leg (workflow_runmatches on workflow name).The
authorizejob is what makes the label mean anything: it re-resolves the PR fromworkflow_run.head_shavia the API, requires it open against this repo and currently labeled, and fails closed. Every other job is behind it vianeeds.Note also that
github.repository == 'voidzero-dev/vite-plus'does not distinguish a fork PR from a same-repo one. A fork's run executes in base-repo context, so that check passes for a hostile PR exactly as it does for yours. What it stops is this workflow doing anything in a downstream fork of the whole repository.Other deliberate choices:
download-artifactpinsrun-idto the triggering run. That input defaults to the current run, so omitting it would silently look in the wrong place.publishholdsid-token, and it does nothing but move bytes. The Docker job installs the preview package (running itspostinstall) and holdspackages: writebut noid-token.publishis gated on thepreview-build-releaseenvironment with required reviewers. Self-review is allowed, since the label is already a maintainer action. This is the one control that does not depend on theauthorizelogic being right.cancel-in-progress: falseon the trusted leg. Cancelling a publish part-way caused the 2026-07-02 packument/tarball mismatch.curl | bash.workflow_runtrigger is suppressed for zizmor inline, with the reasoning and the controls that must not be removed next to it. Inline rather than a repo-wide config, so it does not become a blanket exemption for futureworkflow_runworkflows.Docker preview: same-repo only, for now
It installs the preview package and pushes to
ghcr.io/voidzero-dev/vite-plus:pr-<n>. Before this change a fork could not reach it at all (fork runs get a read-onlyGITHUB_TOKEN, so the push failed); running in base-repo context is what would make it succeed. Publishing unreviewed fork code under the org namespace is a product call, so it is gated off. Drop theis-forkcondition to enable it.Merge risk, stated plainly
workflow_runonly fires for workflow files already on the default branch, so the trusted leg has never executed and cannot be exercised from this PR. Merging switches preview publishing over wholesale: the build leg no longer passes an admin token, so if the trusted leg is broken, previews break until it is fixed.Two things soften that. The bridge still accepts the admin token, so
pnpm warm --repo <checkout> <sha>is a working manual fallback. And the action registers the ref last, so a failed publish leaves invisible artifacts rather than a half-published version.What can be checked before merge is the build leg: labeling this PR
preview-buildrunsmode: packfor real and produces thebridge-packagesartifact.After merge
authorizepasses, environment approval prompt, publish, sticky comment.Then delete
PKG_PR_BRIDGE_ADMIN_TOKENfrom this repo's secrets. Nothing references it after this PR.