Repository navigation
MILAB-7081: npm trusted publishing (OIDC) for node-simple-pnpm - #217
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.
npm no longer accepts 2FA-bypass tokens for direct publish, so the token-based path (changesets/action + pnpm -r publish on the build runner) cannot publish anymore and an opt-in flag has no use. Remove the `npm-trusted-publishing` input and the old publish and tag steps: every node-simple-pnpm caller now packs on its runner and uploads from the GitHub-hosted `npm-publish` job with trusted publishing. `npm-token-fallback` stays (default true): npm tries OIDC first, so the token only matters while it can still publish.
mike-ainsel
changed the title
npm no longer accepts 2FA-bypass tokens for direct publish (GitHub changelog), so
node-simple-pnpmcallers can no longer publish withNPMJS_TOKEN. The replacement is npm trusted publishing (OIDC), which npm accepts only from GitHub-hosted runners ("Self-hosted runners are not currently supported"), while 57 of the 123 callers build onhz-ubuntu-dind.This PR splits the publish: the build job keeps running on the caller's runner and does everything
pnpm -r publishdid except the upload; a new job onubuntu-latestuploads the tarballs with trusted publishing. It is the only publish path now (no opt-in flag).Changes
node-simple-pnpm.yamlbuild-test-publish: thechangesets/action+pnpm -r publishsteps and the tag step are replaced byactions/node/pack-unpublished, which uploads tarballs and a manifest as artifactnpm-tarballs-<app-name-slug>.npm-publishonubuntu-latest(id-token,contents,packageswrite): masks and passesNPMJS_TOKENonly whennpm-token-fallbackis on, runsactions/node/publish-tarballs, then creates the release tag.notify-slack-releasewaits for it.npm-token-fallback(defaulttrue): npm tries OIDC first and uses the token only if the exchange fails.actions/node/npm-split-publish(TypeScript, built into committedlib/; the two composite actions are thin wrappers):pack: for every unpublished non-private package, in dependency order, runsprepublishOnly,prepublishandpnpm pack. Every package is checked before the first script runs (refusespublish/postpublishscripts). For npmjs uploads from public repos it fills a missingpackage.jsonrepositoryinto the packed copy, because npm auto-enables provenance and the registry rejects it (E422) otherwise.publish:npm publish <tarball> --ignore-scripts, always with--tag(publishConfig.tag, elselatest, as npm 10 did on the old path).ManifestEntry), tarball manifests read the way npm reads them (node-tar with pacote's options) and matched against the manifest, dangerous packedpublishConfigkeys refused,--flag=valuearguments, npm run from an empty directory,GITHUB_TOKENsent only to npm.pkg.github.com, https registries only.scripts/npm-trust-setup.sh:discoverlists the packages of caller repos;applycreates trusted publishers (npm trust github … --allow-publish) with interactive 2FA, skipping configured and unpublished packages.0-test-npm-publish-split.yaml: vitest (unit, real-pnpm integration on pnpm 9.12 and 10, real-npm dry run, tarball reads compared with npm's),lib/freshness check, bats for the setup script.Behaviour, trade-offs and recovery from a failed upload ("Re-run failed jobs") are in
actions/node/npm-split-publish/README.md.Verified
Canary repo platforma-open/npm-trusted-publishing-test, two packages,
hz-ubuntu-dind, versions 0.0.1–0.0.9. With the token fallback off, the registry records_npmUser: "GitHub Actions"withtrustedPublisher: githuband SLSA provenance naming the caller'sbuild.yaml. 0.0.5 reproduced the provenance E422 for a package withoutrepositoryand published after the fix; 0.0.6 confirmed the fallback token is masked; 0.0.9 ran without the former opt-in input. Two independent reviews were addressed in this branch.To check a published version:
npm view <pkg>@<version> _npmUserandnpm audit signatures.Rollout notes
build.yaml, with "npm publish" allowed. A new trusted publisher expires unless a publish goes through it within 2 days, so configure and release in batches.node-matrix*,java-gradle,node-simple,github-ci-internal'snode-go-simple, and repos with their own publish workflows (platforma,pframes-rs,visualizations).MILAB-7081