Repository navigation
MILAB-7081: npm trusted publishing (OIDC) for node-simple-pnpm, opt-in - #217
mike-ainsel wants to merge 12 commits into
Conversation
npm trusted publishing rejects self-hosted runners, and bypass-2FA tokens lose direct publish around January 2027. New opt-in input `npm-trusted-publishing` on node-simple-pnpm.yaml: - build-test-publish (gha-runner-label) runs prepublishOnly and packs every unpublished package in topological order (actions/node/pack-unpublished); - new npm-publish job on ubuntu-latest uploads the tarballs via OIDC (actions/node/publish-tarballs) and creates the release tag. `npm-token-fallback` (default true) keeps NPMJS_TOKEN as a fallback while trusted publishers are configured package by package. Default behaviour is unchanged.
…lishers discover lists every non-private workspace package of repos whose build.yaml calls a github-ci node workflow; apply runs npm trust github for each, skipping configured and unpublished packages. Needs an interactive npm login with 2FA.
This reverts commit 6a45637.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| permissions: | ||
| id-token: write | ||
| contents: write |
There was a problem hiding this comment.
When trusted publishing is enabled for a workspace that also publishes to GitHub Packages, npm-publish uses GITHUB_TOKEN without packages: write. The old build job had that permission. GitHub Packages uploads will be rejected, stopping the release. Add packages: write to the new job.
| permissions: | |
| id-token: write | |
| contents: write | |
| permissions: | |
| id-token: write | |
| contents: write | |
| packages: write |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/node-simple-pnpm.yaml
Line: 1212-1214
Comment:
**GitHub Packages uploads fail**
When trusted publishing is enabled for a workspace that also publishes to GitHub Packages, `npm-publish` uses `GITHUB_TOKEN` without `packages: write`. The old build job had that permission. GitHub Packages uploads will be rejected, stopping the release. Add `packages: write` to the new job.
```suggestion
permissions:
id-token: write
contents: write
packages: write
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| else | ||
| echo "//${host}/:_authToken=\${NPM_AUTH_TOKEN}" >> "${NPM_CONFIG_USERCONFIG}" |
There was a problem hiding this comment.
Every registry other than registry.npmjs.org receives NPM_AUTH_TOKEN, which this workflow sets to a GitHub token with contents: write. If a package targets another private registry, the uploader sends that registry a repository write token, even when that host was never configured to receive it. The old create-npmrc path attached tokens only to configured hosts.
Restrict the GitHub token to GitHub Packages and require separate credentials for other registries.
How this was verified: The manifest takes the package's registry URL, and the uploader binds the repository write token to that URL before calling npm.
Prompt To Fix With AI
This is a comment left during a code review.
Path: actions/node/publish-tarballs/action.yaml
Line: 97-98
Comment:
**Other registries receive GitHub credentials**
Every registry other than `registry.npmjs.org` receives `NPM_AUTH_TOKEN`, which this workflow sets to a GitHub token with `contents: write`. If a package targets another private registry, the uploader sends that registry a repository write token, even when that host was never configured to receive it. The old `create-npmrc` path attached tokens only to configured hosts.
Restrict the GitHub token to GitHub Packages and require separate credentials for other registries.
**How this was verified:** The manifest takes the package's registry URL, and the uploader binds the repository write token to that URL before calling npm.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if jq -e --arg r "${repo}" --arg f "${file}" \ | ||
| 'any(.[]; .type == "github" and .repository == $r and .file == $f)' <<<"${existing}" >/dev/null; then | ||
| echo "OK ${pkg}: already trusts ${repo} ${file}" | ||
| skipped=$((skipped + 1)) | ||
| continue |
There was a problem hiding this comment.
apply checks the repository and workflow but not whether direct publishing is allowed. If an existing trusted publisher allows only staged publishing, the script reports OK and skips it. This workflow uses npm publish, so releases fail when token fallback is disabled.
Check the allowed actions too. Report a conflict when direct publishing is disabled rather than claiming the package is ready.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/npm-trust-setup.sh
Line: 98-102
Comment:
**Setup skips incomplete permissions**
`apply` checks the repository and workflow but not whether direct publishing is allowed. If an existing trusted publisher allows only staged publishing, the script reports `OK` and skips it. This workflow uses `npm publish`, so releases fail when token fallback is disabled.
Check the allowed actions too. Report a conflict when direct publishing is disabled rather than claiming the package is ready.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| exit 1 | ||
| fi | ||
| fi | ||
| existing="$(jq -s -c '.' <<<"${raw}" 2>/dev/null)" || existing='[]' |
There was a problem hiding this comment.
apply merges stderr into the JSON from npm trust list, then treats a parse failure as an empty configuration. If npm prints a warning or an unexpected response, the script attempts creation instead of reporting that it could not read the existing settings.
Keep stderr separate and stop with a clear error when the JSON cannot be read.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/npm-trust-setup.sh
Line: 97
Comment:
**Unreadable settings look empty**
`apply` merges stderr into the JSON from `npm trust list`, then treats a parse failure as an empty configuration. If npm prints a warning or an unexpected response, the script attempts creation instead of reporting that it could not read the existing settings.
Keep stderr separate and stop with a clear error when the JSON cannot be read.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Since you create separate action for this operation, make it runnable and testable locally.
You can easily extract shell scripts you run here into two files, make them executable and write tests for them.
Having an inlined script, that generates another script and runs it in a loop is crazy. It is hard to digest and very easy to fail.
nit: I'd preferred writing TS-based actions instead of shell for such complex things, but bash is OK too. I just feel TS is easier to test and harder to fail in an unpredictable/unexpected way.
Like problem found by greptile above: when apply merges stderr into the JSON from npm trust list and JQ parsing error then looks like 'no config'
There was a problem hiding this comment.
The same comment as for above: this script rely on contract with pack-unpublished, that is visible here (as both actions are in same PR) but is hard to understand when you look only at this action. I'd extract this script into a separate sh file, add test manifest.json and write tests that use this file to check everything works. We can use bats for that.
(nit: that is why I'd prefer TS for such things: it provides a standard way to bring all tools you need)
- pack-unpublished.sh / publish-tarballs.sh replace the inline scripts; the pack script calls itself per package via pnpm exec instead of generating a script. The manifest format is documented in pack-unpublished.sh. - bats suites for both scripts and for npm-trust-setup.sh apply, with fixtures and npm stubs; 0-test-npm-publish-split.yaml runs them with pnpm 9.12 and the repo's packageManager. - npm-publish job: add packages: write for GitHub Packages uploads. - publish-tarballs: GITHUB_TOKEN goes to npm.pkg.github.com only; any other non-npmjs registry fails before the first upload. - npm-trust-setup apply: require the createPackage (npm publish) permission, keep npm stderr out of the JSON, stop on unreadable output, and read the mapping from fd 3 so npm's 2FA prompt keeps stdin.
npm attaches a token by host regardless of scheme, so an http:// registry in the manifest would receive GITHUB_TOKEN or the npmjs token in clear text.
Review follow-up: the two composite actions now run one TS package, actions/node/npm-split-publish, instead of shell scripts. - ManifestEntry (src/manifest.ts) is the typed, validated contract between pack and publish. - pack orders packages itself (all dependency fields, as pnpm does; cycles are broken and reported) and checks every package before the first prepublishOnly runs, so a bad package fails without side effects. - publish parses registry URLs with URL: https only, no credentials or port in the URL, GITHUB_TOKEN for npm.pkg.github.com only. - vitest: unit tests with a fake command runner, integration tests with real pnpm on the fixture workspace (CI: pnpm 9.12 and the repo's pnpm). - lib/ is committed like the other TS actions; the test workflow fails when it does not match a fresh build. npm-trust-setup.sh stays bash with its bats suite.
…d job The manifest and tarballs reach the GitHub-hosted job as an artifact. - Validate every value (package name, semver version, .tgz basename, https registry, dist-tag, access) so none can be read by npm as an option; the pack side validates on write, so bad values fail in the build job. - Pass values as --flag=value. - Run npm from an empty temp directory: npm reads a project .npmrc from its working directory, and the artifact directory could carry one.
- Provenance: under trusted publishing npm turns provenance on for public packages and the registry rejects (E422) a package.json whose repository.url does not name the building repository; most of ours have none. pack fills a missing `repository` into the packed copy and fails early on a different repository. Reproduced on the canary (0.0.5). - Dist-tag: always pass --tag (publishConfig.tag, else latest). npm 11 refuses prereleases and versions below the highest one without it; npm 10 on the old path tagged them latest. Covered by a real-npm dry-run test. - Private packages without a version no longer fail pack. - The upload job checks each tarball's package.json against its manifest entry before the first upload. - The upload job no longer loads all caller secrets (actions/env); it gets NPMJS_TOKEN only, and only with npm-token-fallback. - Artifact named per call (app-name-slug); npm pinned to 11.21.0; pack step gets GITHUB_TOKEN as the old publish step did. - README: provenance, tag policy, artifact trust, partial-release recovery.
- Fallback token: extracted from the caller's env secret in its own step and masked (::add-mask::) before any step gets it as an input; GitHub masks the JSON secret as a whole, not values taken out of it. - Run `prepublish` after `prepublishOnly`, as pnpm publish does. - Checks (unsupported scripts, repository) run only on packages this release uploads; repository handling applies only to npmjs targets from public repositories, and the packed tarball is checked to carry it. - Tarball check: reject entries outside package/ and duplicate package.json (npm strips the top directory; last one wins), handle pax and long-name headers. - Refuse packed publishConfig keys that npm would apply for auth, proxy or scope registries, and a registry other than the entry's; correct the manifest comment that said npm ignores the tarball's publishConfig. - Re-run of a restricted package without a token: treat npm's duplicate refusal as already published. - Test workflow also runs on node-simple-pnpm.yaml changes; README documents the latest-tag trade-off.
…d unusual headers npm's unpacker normalizes paths (package/./package.json is package/package.json), so a second manifest spelled differently could pass the identity check. Paths are normalized before comparison; '..', links and other special entries, pax size records and non-octal sizes are refused.
Replace the hand-written tar reader with node-tar 7.5.22 extraction using pacote's options (strip 1, no links), as npm 11.21 reads a tarball before publishing. A custom parser can always disagree with npm on some edge case; tests now compare our result with npm's own (npm pack --dry-run --json) for duplicate manifests, odd path spellings, '..', links and missing manifests.
2 participants
npm is retiring publishing with 2FA-bypass tokens: GitHub expects bypass-2FA granular tokens to lose direct publish around January 2027 (changelog). That is how every
node-simple-pnpmcaller publishes today (NPMJS_TOKEN). The replacement is npm trusted publishing (OIDC), but npm accepts OIDC only from GitHub-hosted runners: "Self-hosted runners are not currently supported". 57 of the 123 callers build onhz-ubuntu-dind.This PR splits the publish so the build stays where it is and only the registry upload moves to a GitHub-hosted runner. It is opt-in: with the new input left at its default, nothing changes for any caller.
Changes
node-simple-pnpm.yaml: new inputsnpm-trusted-publishing(defaultfalse) andnpm-token-fallback(defaulttrue). Whennpm-trusted-publishingis on:build-test-publish(still ongha-runner-label) runsactions/node/pack-unpublishedinstead ofchangesets/action+pnpm -r publish. The tarballs go to an artifact.npm-publishonubuntu-latest(id-token: write,contents: write) uploads them withactions/node/publish-tarballsand creates the release tag. The tag step moves here so a tag only appears after the upload.notify-slack-releasealso waits fornpm-publish.actions/node/pack-unpublished: for each non-private workspace package whose version is not in its registry, in topological order:pnpm run --if-present prepublishOnly, thenpnpm pack.prepublishOnlyhas to run here because blocks use it forblock-tools publish(S3 block registry) and software packages forpl-pkg prepublish.pnpm packdoes not run it. Packages withpublish/postpublishscripts fail the step, since those would have to run after the upload on another runner.actions/node/publish-tarballs: Node 24 + npm^11.5.1,npm publish <tgz> --ignore-scriptsin manifest order. It passes--tag/--accessfrom the manifest, becausenpm publish <tarball>ignorespublishConfig.taginside the tarball (verified locally: anextpackage went out aslatest). Versions already in the registry are skipped, so the job is safe to re-run. registry.npmjs.org authenticates through OIDC; other registries (GitHub Packages) throughGITHUB_TOKEN.npm-token-fallback: also handsNPMJS_TOKENto the upload. npm tries OIDC first and falls back to the token, so a repo can switch on before every one of its packages has a trusted publisher. Set it tofalseto prove a package publishes through OIDC alone.scripts/npm-trust-setup.sh:discoverlists every non-private package of every repo whosebuild.yamlcalls a github-ci node workflow (today: 433 packages in 123 repos).apply <file>runsnpm trust github <pkg> --repo <repo> --file build.yaml --allow-publishfor each one. It skips packages that are already configured or not on the registry yet, and reports packages configured for something else instead of changing them. It needs npm >= 11.15 and an interactivenpm loginwith 2FA; npm does not allow this with bypass tokens.Verified
Canary repo platforma-open/npm-trusted-publishing-test: two packages, one depending on the other through
workspace:*, calling this branch fromhz-ubuntu-dind.0.0.1: published through the token fallback (run 38394168936). This created the packages so trusted publishers could be configured.0.0.2:npm-token-fallback: false(run 38396502404). The registry records_npmUser: "GitHub Actions" <npm-oidc-no-reply@github.com>withtrustedPublisher: github, plus SLSA provenance naming.github/workflows/build.yamland a GitHub-hosted builder. Each tarball containsprepublishOnly ran on hz-ubuntu-dind-… (self-hosted). The dependent package carries"…lib": "0.0.2", i.e.workspace:*was resolved.To check any version yourself:
npm view <pkg>@<version> _npmUserandnpm audit signatures. npm prints its "tokens that bypass 2FA" notice even on OIDC publishes, so that message does not tell you which auth was used.Things a reviewer should know
build.yamlin each repo), notnode-simple-pnpm.yaml. npm documents this, and the canary's provenance confirms it.npm publishwhen configuring on npmjs.com. Otherwise only staged publish is allowed. The script passes--allow-publish.Not in this PR
npm-trusted-publishing: trueper repo, in batches, together withnpm-trust-setup.sh applyfor those repos.node-simple-pnpm:node-matrix*,core/platforma,pframes-rs, the desktop app.@v4-betaonce this merges.MILAB-7081
Fix the registry token exposure, GitHub Packages permissions, and incomplete trusted-publisher checks before merging.
Fix with agent prompt
Summary
Adds opt-in npm trusted publishing while keeping builds and package preparation on the caller's runner.
Fix GitHub Packages permissions, restrict where the GitHub token is sent, and check existing publishers' allowed actions before merging. The setup script should also report unreadable JSON instead of treating it as empty.
Important touched terms:
npm-trusted-publishing: A boolean input selecting the new split publish path; defaults to off.npm-token-fallback: A boolean input allowingNPMJS_TOKENalongside OIDC; defaults to on.pack-unpublished: A new action that runsprepublishOnly, packs packages, and writes upload metadata.manifest.jsonl: The new file listing package names, versions, tarballs, registries, tags, and access settings.publish-tarballs: A new action that uploads packages in manifest order without running package scripts.npm-publish: The new GitHub-hosted job that uploads packages and creates the release.npm-trust-setup.sh: The new script for discovering candidate packages and configuring trusted publishers.Intentional choices acknowledged by
mike-ainsel: discovery lists all non-private package files for later trimming; packages withpublishorpostpublishhooks fail because those hooks cannot run correctly across the split.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Caller enables trusted publishing] --> B[Build and test on caller's runner] B --> C[Run prepublishOnly and pack unpublished packages] C --> D{Packages packed?} D -->|Yes| E[Upload tarballs and manifest] E --> F[Download on ubuntu-latest] F --> G[Publish through OIDC or token] D -->|No| H[Create release if requested] G --> H H --> I[Slack waits for upload job]Reviews (1) · Last reviewed commit: "MILAB-7081: revert canary refs back to @..." · Reviewed by Greptile