ci: isolate publish from installs and drop NPM_TOKEN for OIDC - #596
Draft
guyofeck wants to merge 1 commit into
Draft
ci: isolate publish from installs and drop NPM_TOKEN for OIDC#596guyofeck wants to merge 1 commit into
guyofeck wants to merge 1 commit into
Conversation
Embargo/packages-firewall does not cover the publish flow, so both publish
workflows unpin registry.npmjs.org before `npm publish`. Two problems with
that as it stood: the same job also carried secrets.NPM_TOKEN — exactly the
credential a poisoned transitive dependency would exfiltrate — and "nothing is
installed after the unpin" held only by step ordering, i.e. by comment.
Split each publish workflow into two jobs so the property is structural:
build - fully gatewayed. Every dependency install happens here, behind the
embargo gateway, so the cooldown applies to the whole tree. Never
unpins, holds no publish credential. Uploads the built package.
publish - installs nothing. Downloads build's artifact and runs `npm publish`.
No dependency is resolved while the gateway is bypassed, so the
unpin cannot pull an un-embargoed package.
Also:
- Authenticate via npm trusted publishing (OIDC). Drops
NODE_AUTH_TOKEN/secrets.NPM_TOKEN from preview-publish; manual-publish never
had a credential wired at all, so it could not have published regardless.
- Remove `npm install -g npm@latest` from both. Trusted publishing needs
npm >= 11.5.1 and every Node 24.x release bundles npm >= 11.6.2, so
.node-version already clears the floor. The upgrade also fetched through the
gateway, which refuses an npm release until it clears the cooldown.
- Least-privilege per-job permissions, replacing the workflow-level blocks:
build gets contents: read; publish gets id-token: write plus only what its
release steps need. Drops manual-publish's unused packages: write and
pull-requests: read.
- Document why the unpin exists, why it is safe, and that it is temporary:
`npm publish` sends PUT /<package>, which matches neither the gateway's
`^~ /-/` passthrough block nor `~ \.tgz$`, so it lands in `location /`
(proxy_metadata, a read path with caching); the gateway also sets no
client_max_body_size, so nginx's 1 MB default rejects a packument carrying
the base64 tarball.
Verified: .github/scripts/check_wix_proxy_steps.py passes 16 of 16 jobs across
13 workflows, and no job both unpins/publishes and installs.
Requires registry-side setup before either workflow can publish: a trusted
publisher on npmjs.com per repo AND per workflow filename, so manual-publish.yml
and preview-publish.yml each need their own entry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
Contributor
Author
|
Superseded by #597. @dimar's verdict on the build/publish split in this thread: "it's valid for sure, but too complicated for a long-term solution." He prescribed an interim policy instead — enforced lockfiles, a package manager honoring a minimal-age directive, and embargo only in non-publish tasks — which #597 implements. The trusted-publishing (OIDC) work from here carries over to #597 unchanged; the split and the unpin documentation do not. Keeping this open only as the record of the split approach and the embargo root-cause analysis. |
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.
What
Makes both publish workflows satisfy the two controls @dimar set out in this thread, so GitHub Actions can be re-enabled for
base44/cliat the org level.packages-firewall/embargo does not cover the publish flow, so both publish workflows unpinregistry.npmjs.orgbeforenpm publish. Two problems with that as it stood:secrets.NPM_TOKEN— exactly the credential a poisoned transitive dependency would exfiltrate.The split is the control
Each publish workflow becomes two jobs:
buildbun install --frozen-lockfilepublishbuilddoes every install behind the embargo gateway, so the cooldown applies to the whole tree, then uploads the built package as an artifact.publishdownloads that artifact and runs onlynpm publish— no dependency is ever resolved while the gateway is bypassed, so the unpin cannot pull an un-embargoed package. The property is now structural: the publish job has no package-manager install step at all.Also in here
NODE_AUTH_TOKEN/secrets.NPM_TOKENfrompreview-publish.yml. Notemanual-publish.ymlnever had a credential wired at all —setup-nodewrites an.npmrcexpecting${NODE_AUTH_TOKEN}and nothing set it — so it could not have published regardless of the org block. OIDC is what makes it work; no token was added back.npm install -g npm@latestfrom both. Trusted publishing needs npm >= 11.5.1, and every Node 24.x release bundles npm >= 11.6.2, so.node-versionalready clears the floor. The upgrade also fetched npm through the gateway, which refuses a release until it clears the cooldown — a self-inflicted flake.buildgetscontents: read;publishgetsid-token: writeplus only what its release steps need. Dropsmanual-publish's unusedpackages: writeandpull-requests: read.Root cause of the embargo publish failure
Documented in the workflow comments so the unpin gets removed once fixed rather than living forever. From
package-embargo/nginx.conf:npm publishsendsPUT /<package>, which matches neither the^~ /-/passthrough block nor~ \.tgz$— so it falls intolocation /→proxy_metadata(), a read path with caching, whose own comment scopes it to/lodash,/@babel/core.nginx.confsets noclient_max_body_size, so nginx's 1 MB default applies.npm publishbase64-encodes the tarball into the packument, making the body ~1.37x the tarball — over the limit forbase44, giving a 413 before Lua runs.lua/nginx.lua:152-164reads the entire body into worker memory (f:read("*all")), so raising the limit alone would buffer every publish tarball in RAM.The routing, missing
client_max_body_size, and full-body buffering are verified in the config. The 413 itself is inference — worth confirming against the actual response.Verification
.github/scripts/check_wix_proxy_steps.py— 16 of 16 jobs across 13 workflows (was 14; the split adds 2). Both new jobs have checkout at step 1, proxy at step 2.base44, so nothing here has executed. Review accordingly.Blocked on / needs decision
manual-publish.ymlandpreview-publish.ymleach need their own entry. Needs npm org admin. Without it both workflows fail to authenticate.min-release-age) is not addressed, deliberately. Every install here isbun install, and Bun has no equivalent directive; honoring it literally means migrating offbun.lockacross all six installing workflows, against this repo's Bun-for-everything rule. My read is that the requirement was premised on publish jobs bypassing the gateway wholesale — which is not what ci: enforce wix gateway proxy in all workflows #592 built, and is now structurally not the case. Does gatewayedbun installplus a publish job that installs nothing satisfy the control? If not, this PR needs a package-manager migration and will look quite different.dry_run=trueis worth anything as a smoke test:npm publish --dry-runnever authenticates, so it cannot exercise the OIDC path. First real proof will be apreviewpublish.🤖 Generated with Claude Code