diff --git a/.github/actions/publish-preview/src/artifact.ts b/.github/actions/publish-preview/src/artifact.ts index 46e871a..beb1147 100644 --- a/.github/actions/publish-preview/src/artifact.ts +++ b/.github/actions/publish-preview/src/artifact.ts @@ -26,9 +26,9 @@ import { join } from 'node:path' /** Advisory description of a packed batch. Never trusted by the reader. */ export interface PackManifest { - /** `commit.` the pack ran for. Re-derived by the trusted leg. */ + /** `commit.` the pack ran for. Re-derived by the publishing workflow. */ ref: string - /** `0.0.0-commit.`. Re-derived by the trusted leg. */ + /** `0.0.0-commit.`. Re-derived by the publishing workflow. */ version: string /** One entry per packed tarball, for human inspection of a failed run. */ packages: Array<{ file: string; name: string; dir: string }> @@ -39,7 +39,7 @@ export const MANIFEST_NAME = 'manifest.json' /** * Ceiling on how many packages one artifact may carry. The vite-plus batch is * ~11; this leaves room to grow while stopping a modified build workflow from - * handing the trusted leg tens of thousands of small archives to validate. + * handing the publishing workflow tens of thousands of small archives. */ export const MAX_ARTIFACT_PACKAGES = 128 @@ -66,15 +66,15 @@ function isActionOutput(entry: string): boolean { * Create `dir` and remove this action's own outputs from any previous run. * * Packing writes `pkg-0..N`, so a rerun that produces FEWER packages than the - * last one would otherwise leave the higher indices behind. The upload leg - * enumerates every `pkg-.tgz` it finds and ignores `manifest.json`, so those - * leftovers would be republished under the new commit version, or collide as a - * duplicate package name and fail the run. CI gets a clean runner, but local - * runs and `pnpm warm` reuse a workspace. + * last one would otherwise leave the higher indices behind. The publishing + * workflow enumerates every `pkg-.tgz` it finds and ignores `manifest.json`, + * so those leftovers would be republished under the new commit version, or + * collide as a duplicate package name and fail the run. CI gets a clean runner, + * but local runs and `pnpm warm` reuse a workspace. * * Only action-owned names are removed. Anything else in the directory is left - * alone and will make the upload leg refuse the artifact, which is better than - * this quietly deleting a file it did not create. + * alone and will make the publishing workflow refuse the artifact, which is + * better than this quietly deleting a file it did not create. */ export function prepareOutputDir(dir: string): void { mkdirSync(dir, { recursive: true }) diff --git a/.github/actions/publish-preview/src/index.ts b/.github/actions/publish-preview/src/index.ts index f7623d9..69b77d2 100644 --- a/.github/actions/publish-preview/src/index.ts +++ b/.github/actions/publish-preview/src/index.ts @@ -23,7 +23,7 @@ * `output-dir`. No network and no credentials, so it can run in a job that * builds an untrusted fork pull request. * - `upload`: the remote half only, reading `input-dir`. Runs in the trusted - * `workflow_run` leg and treats the artifact as hostile: every archive is + * `workflow_run` workflow and treats the artifact as hostile: every archive * validated against the SR-6 canonical policy, the version comes from the * trusted `sha` input, and each package is REBUILT here so the bytes * published are ones this step constructed. @@ -360,7 +360,7 @@ async function main(): Promise { const dirs = expandPackageDirs(parsePackagesInput(input('packages') || DEFAULT_PACKAGES), cwd) const packages = dirs.map((dir) => ({ label: dir, manifest: readManifest(dir) })) // Fail on the batch before packing anything, so a missing platform dir - // stops the build leg rather than producing a partial artifact. + // stops the build workflow rather than producing a partial artifact. assertValidBatch(packages, env) prepareOutputDir(outputDir) diff --git a/.github/actions/publish-preview/src/oidcToken.ts b/.github/actions/publish-preview/src/oidcToken.ts index 0681f6c..a0482e4 100644 --- a/.github/actions/publish-preview/src/oidcToken.ts +++ b/.github/actions/publish-preview/src/oidcToken.ts @@ -5,7 +5,7 @@ * `permissions: id-token: write`. Requesting a token with the bridge as * audience proves "this workflow, in this repository, ran this" without the * repository storing a bridge credential, which is what lets fork PRs publish - * through the trusted `workflow_run` leg. + * through the publishing `workflow_run` workflow. * * Reads the runner environment directly rather than depending on * `@actions/core`, keeping the bundled action free of runtime deps. diff --git a/docs/ci-setup.md b/docs/ci-setup.md index 04589e8..e17a79e 100644 --- a/docs/ci-setup.md +++ b/docs/ci-setup.md @@ -14,13 +14,13 @@ Publishing is split across two workflows, because GitHub denies fork authenticate to the bridge from the job that builds it, no matter how the credential is delivered. -**Build leg** (`publish-preview.yml`, on `pull_request`, label `preview-build`). +**Build workflow** (`publish-preview.yml`, on `pull_request`, label `preview-build`). Builds the PR and runs the bridge action in `mode: pack`, which packs each locally built package directory with `pnpm pack` and stops. No network, no credentials. The packed tarballs upload as the `bridge-packages` artifact. This runs for forks. -**Trusted leg** (`publish-preview-register.yml`, on `workflow_run`). Runs the +**Publishing workflow** (`publish-preview-register.yml`, on `workflow_run`). Runs the workflow file from the default branch in base-repo context, so it can mint an OIDC token whatever repository the PR came from. It downloads the artifact and runs the action in `mode: upload`, which validates every archive, rewrites and @@ -30,10 +30,10 @@ metadata never diverge for longer than one upload. A final `POST /-/register` flips the whole version visible atomically, so a run cancelled mid-way leaves only invisible artifacts. -The trusted leg treats the artifact as hostile, because it was produced by a job +The publishing workflow treats the artifact as hostile, because it was produced by a job that ran the PR's code. The published version comes from `workflow_run.head_sha`, every package name is read back out of a validated -archive, and the bytes published are ones the trusted leg rebuilt. Nothing in +archive, and the bytes published are ones the publishing workflow rebuilt. Nothing in the artifact selects what gets published. The Worker then only serves bytes from R2, with `dist.integrity` matching the @@ -43,15 +43,20 @@ exact bytes served. pkg.pr.new is not involved anywhere. `preview-build` is the consent step, and only maintainers can apply it. But on `pull_request` events GitHub runs the workflow file from the merge ref, so a PR -author can edit the build leg and delete its own label check, or add a workflow -with a matching `name:` to trigger the trusted leg (`workflow_run` matches on +author can edit the build workflow and delete its own label check, or add a workflow +with a matching `name:` to trigger the publishing workflow (`workflow_run` matches on workflow name). The `authorize` job is what makes the label mean anything. It re-resolves the PR -from `workflow_run.head_sha` through the API, requires it open against this -repository and currently labeled, and fails closed on a missing PR, a missing -label, or an API error. Do not remove it, and do not let the publish job run -without `needs: authorize`. +through the API, requires it open against this repository, currently labeled, +and still pointing at the commit that was built, and fails closed on a missing +PR, a missing label, or an API error. Do not remove it, and do not let the +publish job run without `needs: authorize`. + +Resolve by head repository and branch, not by commit: +`listPullRequestsAssociatedWithCommit` returns nothing for a fork PR's head +commit, and `workflow_run.pull_requests` is empty for forks too, so both are +blind to exactly the case this setup exists for. ## Setup @@ -84,7 +89,7 @@ someone else. A rename keeps the same `repository_id` but changes `workflow_ref`, so publishes fail until the allowlist is updated. That is the correct direction to fail, but it will look like an outage to whoever hits it. -### 2. Build leg +### 2. Build workflow In the job that assembles the build artifacts, after `pnpm install`, the artifact downloads, and `publish-native-addons.ts --mode pkg-pr-new` (which @@ -117,7 +122,7 @@ than publishing a version with a dangling dep. > events `github.sha` is the ephemeral **merge** commit, whereas the checkout > being packed is the PR **head** commit. -### 3. Trusted leg +### 3. Publishing workflow ```yaml on: @@ -183,7 +188,7 @@ would let one PR change what installs for another. Derive it from the API. ### 4. Verify -After a labeled PR builds and the trusted leg runs: +After a labeled PR builds and the publishing workflow runs: ```bash curl https://registry-bridge.viteplus.dev/-/refs @@ -193,7 +198,7 @@ Verify a same-repo PR first, then a fork PR, then confirm an unlabeled fork PR does **not** publish. Note that `workflow_run` only fires for workflow files already on the default -branch, so the trusted leg cannot be exercised from the pull request that +branch, so the publishing workflow cannot be exercised from the pull request that introduces it. The first real verification happens after merge. ## Notes diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 0a9785f..ec9c601 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -173,7 +173,7 @@ manual operations. This alone does not give you fork-PR publishing: that also needs the two-workflow split on the consumer side, because fork `pull_request` runs cannot mint an OIDC token either. [`ci-setup.md`](./ci-setup.md) has the full wiring -(build leg, `authorize` job, trusted leg) and RFC 0002 has the reasoning. +(build workflow, `authorize` job, publishing workflow) and RFC 0002 has the reasoning. ## 6. Attach a custom domain (optional) diff --git a/rfcs/0002-zero-trust-github-oidc-publishing.md b/rfcs/0002-zero-trust-github-oidc-publishing.md index 90c888f..4e6082e 100644 --- a/rfcs/0002-zero-trust-github-oidc-publishing.md +++ b/rfcs/0002-zero-trust-github-oidc-publishing.md @@ -9,8 +9,8 @@ Replace the shared `PKG_PR_BRIDGE_ADMIN_TOKEN` secret on the publish path with GitHub Actions OIDC tokens that the Worker verifies against GitHub's public -keys. Split the consumer workflow into an untrusted build leg (`pull_request`, -runs for fork PRs, holds no credentials) and a trusted publish leg +keys. Split the consumer workflow into an build workflow (`pull_request`, +runs for fork PRs, holds no credentials) and a trusted publishing workflow (`workflow_run`, runs in base-repo context, mints a short-lived OIDC token). After this change a fork PR labeled `preview-build` publishes a preview build through the same path as a same-repo PR, and the consumer repo stores no @@ -65,7 +65,7 @@ Two GitHub platform rules shape the design: GitHub runs the file from the merge ref, so a fork PR can edit the build workflow (including its label gate) or add another file with a matching `name:`. The listening workflow is trusted; what caused it to fire is - not. Everything the trusted leg needs about authorization it must read + not. Everything the publishing workflow needs about authorization it must read from the API, not from the triggering run (SR-1). `pull_request_target` also gets credentials but is rejected: it would build @@ -246,10 +246,10 @@ comment installs. Three changes close it (see [SR-2](#sr-2-pr-number-binding)): -- The trusted leg resolves the PR number from `head_sha` via - `GET /repos/{repo}/commits/{head_sha}/pulls` and constructs `prUrl` from - that. It is never read from the artifact. This is the authoritative - binding; the two bridge-side checks below are containment. +- The publishing workflow resolves the PR from the triggering run's head + repository and branch, then constructs `prUrl` from it. It is never read + from the artifact. This is the authoritative binding; the two bridge-side + checks below are containment. - `/-/register` rejects a `prUrl` outside `https://github.com//pull/`, so a CI identity cannot name a pull request in another repository. @@ -268,7 +268,7 @@ multi-commit PR. The per-ref immutability above is the version that holds. The bridge cannot do better on its own: verifying that a commit really belongs to a PR needs a GitHub API call, which would put an API dependency and its rate limits on the publish path. That check belongs in the trusted -leg, which is already talking to the API for SR-1. +workflow, which is already talking to the API for SR-1. This gap predates the RFC: the admin-token path has it today, bounded by the fact that publishing currently requires repository write access. Handing the @@ -289,9 +289,9 @@ The action gains a `mode` input with three values: - **`pack`**: run the local half only. `pnpm pack` each directory, rewrite to the synthetic version, re-pack, hash, and write the tarballs plus a `manifest.json` (ref, version, file list) to `output-dir`. No network, no - credentials. Runs in the untrusted leg. + credentials. Runs in the build workflow. - **`upload`**: run the remote half only, from `input-dir`. Runs in the - trusted leg. + publishing workflow. `admin-token` becomes optional. In `upload` mode without it, the action mints an OIDC token itself via the runner's @@ -318,16 +318,16 @@ inside the tarball, which land under the attacking PR's own `0.0.0-commit.` version. A preview build of a PR already carries that PR's arbitrary code by definition; the blast radius is unchanged. -Since the trusted leg now rebuilds anyway, `pack` mode arguably should not +Since the publishing workflow now rebuilds anyway, `pack` mode arguably should not rewrite and re-pack at all: it could emit raw `pnpm pack` output (the one step that genuinely needs the workspace and its `node_modules`), leaving the version rewrite, dependency pinning, canonical re-pack, and hashing to the -trusted leg, which can derive the batch from the tarball names it validated. +publishing workflow, which can derive the batch from the tarball names it validated. That would put every step whose output the bridge trusts on the trusted side of the boundary, and shrink `pack` mode to a thin wrapper. It costs the trusted job more CPU, which is free here: the CPU constraint in RFC 0001 was the Worker's, never CI's. Left as a refinement for the action PR rather than -settled now, since it changes the artifact contract between the two legs. +settled now, since it changes the artifact contract between the two workflows. ## 7. Consumer workflow changes (vite-plus) @@ -336,7 +336,7 @@ label `preview-build`) and its build jobs, drops the same-repo gate, and replaces the bridge step with `mode: pack` plus an `actions/upload-artifact` step (short retention, one day). Its `permissions` stay `contents: read`. -A new `publish-preview-register.yml` handles the trusted leg: +A new `publish-preview-register.yml` handles the publishing workflow: ```yaml on: @@ -348,7 +348,7 @@ permissions: {} jobs: # Gate: re-establish authorization from repository state, because the - # build leg's own label check ran in a file the PR author can edit. + # build workflow's own label check ran in a file the PR author can edit. authorize: if: >- github.event.workflow_run.conclusion == 'success' && @@ -399,7 +399,7 @@ jobs: Details that matter: - **The `authorize` job is a security control, not a payload workaround.** - See [SR-1](#sr-1-trusted-leg-authorization); it is the only thing that + See [SR-1](#sr-1-publishing-workflow-authorization); it is the only thing that makes the label a boundary. It also supplies the PR number and URL, which `github.event.workflow_run.pull_requests` leaves empty for fork PRs. - `download-artifact` pins `run-id` to the triggering run. Downloading "the @@ -427,12 +427,12 @@ resource-exhaustion cases below. It does not bound as much as it looks like it does, for two reasons. First, on `pull_request` events GitHub runs the workflow file from the merge -ref, so the PR's own edits to that file take effect. The build leg's +ref, so the PR's own edits to that file take effect. The build workflow's `if: contains(github.event.pull_request.labels.*.name, 'preview-build')` therefore runs inside a file the PR author controls, and a fork PR can delete it, or add a second workflow file carrying the same `name:`, and produce a successful run that `workflow_run` matches by name. Authorization -is real; enforcement in the build leg is not. That is why SR-1 exists. +is real; enforcement in the build workflow is not. That is why SR-1 exists. Second, the label authorizes "build a preview of this contributor's PR." It is applied precisely so reviewers can test code nobody has audited yet, so @@ -450,19 +450,33 @@ add much against a patient attacker. These are implementation requirements, not defense-in-depth. The design is not safe to ship without them. - -**SR-1. The trusted leg re-establishes authorization from repository -state.** Before publishing, resolve the PR from -`github.event.workflow_run.head_sha` via the API, and fail unless it is open -against this repository and currently carries `preview-build`. Fail closed -on a missing PR, a missing label, or an API error. Without this, any fork PR -publishes to production without a label, because the build leg's own check -is attacker-editable (8.1). Gating on `workflow_run.path` as well blocks the -extra-workflow-file variant but not an edit to the original, so it is a -supplement, never the control. + +**SR-1. The publishing workflow re-establishes authorization from repository +state.** Before publishing, resolve the PR through the API and fail unless it +is open against this repository, currently carries `preview-build`, and still +points at the commit that was built. Fail closed on a missing PR, a missing +label, or an API error. Without this, any fork PR publishes to production +without a label, because the build workflow's own check is attacker-editable +(8.1). Gating on `workflow_run.path` as well blocks the extra-workflow-file +variant but not an edit to the original, so it is a supplement, never the +control. + +Resolve it by **head repository and branch** +(`GET /pulls?state=open&head=:`, both from +GitHub-signed `workflow_run` payload fields), then check the head sha +separately so the failure distinguishes "no such PR" from "the PR moved on". + +An earlier draft said to resolve from `head_sha` via +`GET /repos/{repo}/commits/{head_sha}/pulls`. That endpoint returns EMPTY for +a fork PR's head commit while working correctly for a same-repo one, so it +passes every test reachable before the workflow is on the default branch and +fails for the only case this design exists for. `workflow_run.pull_requests` +is fork-blind in the same way, which is what makes the commit endpoint look +like the alternative. Anything keyed on a fork's commit is suspect; the PR +number, the run id, and the head branch are all base-repo facts and are not. -**SR-2. The PR number is derived, never accepted.** The trusted leg builds +**SR-2. The PR number is derived, never accepted.** The publishing workflow builds `prUrl` from the API lookup in SR-1. The bridge contains it: `/-/register` rejects a `prUrl` outside the token's `repository` claim, and refuses to re-point an existing ref at a different `prUrl` (5.4). Otherwise a single @@ -499,7 +513,7 @@ default, but one added `build-arg` or `run:` step would reintroduce `ACTIONS_ID_TOKEN_REQUEST_TOKEN`. -**SR-6. The trusted leg validates archives against a canonical policy and +**SR-6. The publishing workflow validates archives against a canonical policy and republishes its own bytes.** The tar codec now reads attacker-supplied tarballs, having only ever seen `pnpm pack` output before. Two parts: @@ -518,9 +532,9 @@ tarballs, having only ever seen `pnpm pack` output before. Two parts: Symlinks must also not be followed when enumerating `input-dir` itself. *Then canonicalize*: rather than forwarding the fork's bytes, the trusted -leg rebuilds the tarball with the Worker's own codec, emitting exactly one +workflow rebuilds the tarball with the Worker's own codec, emitting exactly one entry per path with normalized metadata, and hashes what it emitted. The -shasum and integrity published to the bridge describe bytes the trusted leg +shasum and integrity published to the bridge describe bytes the publishing workflow constructed. The canonical rebuild is what makes the reject list robust rather than @@ -546,7 +560,7 @@ org fails closed rather than continuing to publish (5.1). Set both whenever - **Fork PR modifies workflows to publish directly.** Fork `pull_request` runs get no secrets and no `id-token`; SR-1 rejects the forged trigger. -- **Fork PR poisons the artifact.** The trusted leg validates, rebuilds and +- **Fork PR poisons the artifact.** The publishing workflow validates, rebuilds and re-hashes, and forces name and version from trusted inputs (SR-6). Damage stays inside that PR's own preview version, which carries the PR's code by design. @@ -707,7 +721,7 @@ fix stands alone and is worth landing regardless of whether the rest ships. one repo's packages today. 3. Bind the token to the commit it may publish? Today the capability is scoped by audience and workflow, so a stolen token can publish anything - that workflow could during its lifetime. The trusted leg already knows + that workflow could during its lifetime. The publishing workflow already knows `workflow_run.head_sha`, so it could request `audience = #` and the bridge could require every version in the request to equal `0.0.0-commit.`. Distinct from `jti` @@ -725,7 +739,7 @@ fix stands alone and is worth landing regardless of whether the rest ships. 5. Should the sticky comment render differently for fork-originated builds (8.4), and how loudly? Proposed: a one-line banner naming the source fork above the install instructions. -6. Does the Docker preview job move into the trusted leg at all? Moving it +6. Does the Docker preview job move into the publishing workflow at all? Moving it is what lets fork code reach `ghcr.io/voidzero-dev/vite-plus:pr-` (8.4); leaving it on `pull_request` keeps forks out of the org namespace but also leaves them without a preview image, which is part of what this diff --git a/src/security/auth.ts b/src/security/auth.ts index 9a88e92..a47231c 100644 --- a/src/security/auth.ts +++ b/src/security/auth.ts @@ -86,10 +86,11 @@ export async function requirePublisher({ * cannot register a ref pointing at a pull request in some other repository. * * This is a containment check, not the real binding. The authoritative one is - * in the trusted workflow leg, which resolves the PR from `workflow_run.head_sha` - * via the GitHub API and constructs the URL itself, so the value never comes - * from the (untrusted) build artifact. The bridge cannot repeat that lookup - * without a GitHub API dependency, so it enforces the containment it can. + * in the publishing workflow, which resolves the PR from the triggering run's + * head branch via the GitHub API and constructs the URL itself, so the value + * never comes from the (untrusted) build artifact. The bridge cannot repeat + * that lookup without a GitHub API dependency, so it enforces the containment + * it can. */ export function assertPrUrlInRepository( prUrl: string, diff --git a/src/security/oidc.ts b/src/security/oidc.ts index 90d130c..191a19a 100644 --- a/src/security/oidc.ts +++ b/src/security/oidc.ts @@ -3,9 +3,9 @@ * * The bridge accepts a short-lived, GitHub-signed identity token instead of a * shared secret, so a consumer repo stores no bridge credential and fork PRs - * can publish through a trusted `workflow_run` leg. Nothing here is secret: - * the signing key is GitHub's public JWKS and the allowlists hold public - * identifiers. + * can publish through a separate `workflow_run` workflow. Nothing here is + * secret: the signing key is GitHub's public JWKS and the allowlists hold + * public identifiers. * * Verification is hand-rolled on WebCrypto rather than pulled from a JWT * library (the Worker avoids heavy deps), which means owning the standard diff --git a/src/tarball/validateArchive.ts b/src/tarball/validateArchive.ts index b26374a..87b963f 100644 --- a/src/tarball/validateArchive.ts +++ b/src/tarball/validateArchive.ts @@ -2,7 +2,7 @@ * Canonical archive policy for tarballs the bridge did not produce * (RFC 0002 SR-6). * - * The trusted publish leg reads tarballs out of an artifact built by an + * The trusted publishing workflow reads tarballs out of an artifact built by an * untrusted job, so this validates them against a strict shape before anything * downstream reads their contents. The output of validation feeds * `buildPreviewTarball`, which emits a fresh archive: the bytes published are diff --git a/test/action/artifact.test.ts b/test/action/artifact.test.ts index 4359431..a8aaaf0 100644 --- a/test/action/artifact.test.ts +++ b/test/action/artifact.test.ts @@ -1,5 +1,5 @@ /** - * The artifact directory is attacker-controlled whenever the build leg ran for + * The artifact directory is attacker-controlled whenever the build workflow ran for * a fork PR, so the reader refuses anything it did not expect rather than * skipping it: a silently ignored entry would publish a subset of the batch and * look like success. @@ -104,7 +104,7 @@ describe('readArtifactTarballs', () => { /** * Packing writes pkg-0..N. A rerun producing FEWER packages would otherwise - * leave the higher indices behind, and the upload leg enumerates every + * leave the higher indices behind, and the publishing workflow enumerates every * pkg-.tgz it finds, so a stale one would be republished under the new * commit version or collide as a duplicate package name. */ @@ -125,7 +125,7 @@ describe('prepareOutputDir', () => { }) it('leaves files it did not create alone', () => { - // Better to let the upload leg refuse an unexpected file than to have pack + // Better to let the publishing workflow refuse an unexpected file than to have pack // silently delete something a user put there. packed(0) writeFileSync(join(dir, 'notes.txt'), 'keep me') diff --git a/test/oidc.test.ts b/test/oidc.test.ts index 1b05258..8c321b8 100644 --- a/test/oidc.test.ts +++ b/test/oidc.test.ts @@ -246,7 +246,7 @@ describe('OIDC publishing: rejected tokens', () => { ], ['unlisted workflow_ref', () => mint({ workflow_ref: `${REPOSITORY}/.github/workflows/evil.yml@refs/heads/main` }), 403], [ - 'build-leg workflow_ref', + 'build-workflow workflow_ref', () => mint({ workflow_ref: `${REPOSITORY}/.github/workflows/publish-preview.yml@refs/heads/main` }), 403, ],