Skip to content

Commit 6480db3

Browse files
authored
ci: gate preview publish approval on fork PRs only (#2404)
The preview-build-release environment approval slowed every preview publish, and reviewer notifications were easy to miss. The publish job now selects its environment by PR origin: - fork PRs: `preview-build-release` (required reviewers, unchanged) - same-repo PRs: `preview-build-release-auto` (no protection rules, publishes unattended) Same-repo publishes stay bound to maintainer consent without the click: the `preview-build` label is a maintainer action, the build runs only on `labeled` events, and `authorize` plus the re-check pin the publish to the labeled head sha. Fork PRs keep the human gate because their artifact carries unreviewed external code, and the gate is the one control that does not depend on the `authorize` logic being correct. The `preview-build-release-auto` environment is already created with no protection rules, so the first run does not create it implicitly. Verified with actionlint and zizmor (no findings, same suppressions as before).
1 parent af39c16 commit 6480db3

1 file changed

Lines changed: 43 additions & 29 deletions

File tree

.github/workflows/publish-preview-register.yml

Lines changed: 43 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ name: Register preview build
2525
# - the artifact is treated as hostile bytes, and its download pins run-id to
2626
# the triggering run
2727
# - no job that installs or executes preview content holds id-token
28-
# - `publish` is gated on an environment with required reviewers
28+
# - `publish` of a fork PR is gated on an environment with required
29+
# reviewers; a same-repo PR publishes without approval because the
30+
# maintainer-applied label already binds consent to the exact built sha
2931
#
3032
# zizmor's dangerous-triggers audit warns that workflow_run is "almost always
3133
# used insecurely". The danger it names is real here and the trigger is
@@ -166,11 +168,11 @@ jobs:
166168
// run for `preview-build`, so an unrelated label produces a run in
167169
// which everything skips — and an all-skipped run still concludes
168170
// "success". The PR legitimately still carries the label, so every
169-
// check above passes and this would queue an environment approval
170-
// for a run that built nothing, which could only end in a failed
171-
// download. Requiring the artifact is what separates "built
172-
// something" from "did nothing", and it also catches a build workflow
173-
// that succeeded without uploading.
171+
// check above passes and a run that built nothing would reach the
172+
// publish job: queued for approval on a fork, straight to a failed
173+
// download on a same-repo PR. Requiring the artifact is what
174+
// separates "built something" from "did nothing", and it also
175+
// catches a build workflow that succeeded without uploading.
174176
//
175177
// Not a failure: adding an unrelated label to a labeled PR is a
176178
// normal thing to do, and a red X on every one of them would be
@@ -201,39 +203,51 @@ jobs:
201203
needs: authorize
202204
if: needs.authorize.outputs.eligible == 'true'
203205
runs-on: ubuntu-latest
204-
# Required-reviewer gate on the ONE job that mints a publish credential.
205-
# The `authorize` job proves the PR is open and labeled; this proves a human
206-
# looked at this specific run before a token existed, and records who. It is
207-
# the only defense here that does not depend on my own logic being right:
208-
# if `authorize` were ever weakened, this still stops an unattended publish.
206+
# Required-reviewer gate on the ONE job that mints a publish credential,
207+
# applied to FORK PRs only. The `authorize` job proves the PR is open and
208+
# labeled; for a fork, the gate proves a human looked at this specific run
209+
# before a token existed, and records who. It is the only defense here that
210+
# does not depend on my own logic being right: if `authorize` were ever
211+
# weakened, it still stops an unattended publish of unreviewed external
212+
# code.
209213
#
210-
# Self-review is allowed: the `preview-build` label is already a maintainer
211-
# action, so requiring a SECOND person for every external contributor's
212-
# preview would cost more than the risk warrants. The value here is the
213-
# deliberate confirmation and the audit trail, not two-person control.
214+
# Same-repo PRs skip the gate. The label is already a deliberate maintainer
215+
# action, the build runs only on `labeled` events, and `authorize` plus the
216+
# re-check below pin the publish to the head sha the label was applied to,
217+
# so approving a self-labeled run confirmed nothing the label had not. The
218+
# approval wait was also easy to miss: reviewer notifications follow each
219+
# reviewer's Actions notification settings.
214220
#
215-
# The environment must exist with protection rules configured. A workflow
216-
# referencing a MISSING environment gets one created implicitly with no
217-
# rules, which looks like a gate and is not one.
218-
environment: preview-build-release
221+
# Self-review is allowed on the fork gate: requiring a SECOND person for
222+
# every external contributor's preview would cost more than the risk
223+
# warrants. The value is the deliberate confirmation and the audit trail,
224+
# not two-person control.
225+
#
226+
# BOTH environments must exist: preview-build-release WITH required
227+
# reviewers, preview-build-release-auto with NO protection rules. A
228+
# workflow referencing a MISSING environment gets one created implicitly
229+
# with no rules, which looks like a gate and is not one.
230+
environment: ${{ needs.authorize.outputs.is-fork == 'true' && 'preview-build-release' || 'preview-build-release-auto' }}
219231
# SR-5: this job mints the publish token, so it must never run anything out
220232
# of the artifact. It downloads bytes and hands them to the bridge action;
221233
# no install, no build, no scripts.
222234
permissions:
223235
id-token: write # mint the bridge OIDC token
224236
actions: read # download the triggering run's artifact
225-
pull-requests: read # re-check the PR after the approval wait
237+
pull-requests: read # re-check the PR before a token exists
226238
outputs:
227239
version: ${{ steps.bridge.outputs.version }}
228240
steps:
229-
# `authorize` ran BEFORE the approval gate, so its verdict is a snapshot
230-
# from potentially days ago (the artifact is retained 7 days precisely to
231-
# allow that). Re-assert it here, after the wait and before a token
232-
# exists: the PR can have been closed, had the label removed (which is how
233-
# a maintainer revokes consent), or advanced to a new head commit, in
234-
# which case publishing this artifact would move the pr-<n> dist-tag
235-
# BACKWARDS onto an older commit than the PR now points at.
236-
- name: Re-check authorization after approval
241+
# `authorize` ran BEFORE this job, so its verdict is a snapshot. On a
242+
# fork run that waits for approval the snapshot can be days old (the
243+
# artifact is retained 7 days precisely to allow that); on a same-repo
244+
# run the window is seconds, but the check is cheap, so it runs for both.
245+
# Re-assert the verdict here, before a token exists: the PR can have been
246+
# closed, had the label removed (which is how a maintainer revokes
247+
# consent), or advanced to a new head commit, in which case publishing
248+
# this artifact would move the pr-<n> dist-tag BACKWARDS onto an older
249+
# commit than the PR now points at.
250+
- name: Re-check authorization before publish
237251
uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9
238252
env:
239253
PR_NUMBER: ${{ needs.authorize.outputs.pr }}
@@ -258,7 +272,7 @@ jobs:
258272
}
259273
if (reasons.length) {
260274
core.setFailed(
261-
`PR #${pr.number} changed while waiting for approval (${reasons.join('; ')}); ` +
275+
`PR #${pr.number} changed between authorization and publish (${reasons.join('; ')}); ` +
262276
'refusing to publish. Re-apply the label to build the current head.',
263277
);
264278
}

0 commit comments

Comments
 (0)