feat: install pnpm from the npm registry, verified against npm's signature - #24
Conversation
The action checked the release archive against the `digest` GitHub publishes for it. GitHub serves both the asset and the digest, so whoever can replace one can replace the other: that catches a corrupted download, not a tampered one. From v12 the npm registry carries the same executable, byte for byte, and npm signs `<name>@<version>:<integrity>` with a key that is pinned here. That signature cannot be produced without npm's private key, and behind it sits the maintainer's approval of the staged publish — so v12 and newer are now fetched from the registry and refused unless both the signature and the checksum check out. v11 keeps using the release asset and its digest. Its `dist/` bundles dependencies that the registry copy declares instead, and this action has no step that would install them. Two things fall out of not touching the GitHub API for v12+: - `token` stops mattering there. It exists to lift the anonymous 60 requests/hour limit on the release lookup. - Versions published to npm without a GitHub release install fine now, instead of failing the lookup this action warns about. The pinned keys are the ones pnpm itself pins for `pnpm audit signatures`.
📝 WalkthroughWalkthroughThe installer now resolves pnpm v12+ from the npm registry through Changespnpm registry installation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant run
participant downloadPnpm
participant getPnpm
participant GitHubArchive
run->>downloadPnpm: Resolve pnpm version
alt pnpm v12+
downloadPnpm->>getPnpm: Install registry package
getPnpm-->>downloadPnpm: Return installed pnpm
else Older pnpm
downloadPnpm->>GitHubArchive: Download and verify archive
GitHubArchive-->>downloadPnpm: Return extracted pnpm
end
downloadPnpm-->>run: Report selected source
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoVerify pnpm v12+ installs using npm registry signatures (fallback to GitHub for v11)
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/install-pnpm/download.ts (1)
92-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the registry cutover major.
The function already uses
MIN_SUPPORTED_MAJORfor the lower bound. Add a matching constant for the registry cutover, for exampleMIN_REGISTRY_MAJOR = 12, so the two version boundaries read the same way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install-pnpm/download.ts` around lines 92 - 95, Define a named registry cutover constant, such as MIN_REGISTRY_MAJOR = 12, alongside MIN_SUPPORTED_MAJOR, and update the semver.major(version) comparison in the download selection logic to use it instead of the literal 12.src/install-pnpm/verify-signature.ts (1)
28-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSelect the signature that matches a pinned key.
The code checks only
signatures[0].dist.signaturesis an array. If npm ever returns more than one entry and the pinned key is not first, the install fails even though a pinned key signed the package. Match by keyid across all entries instead.♻️ Proposed refactor
- const signature = opts.signatures?.[0] - if (!signature) { + const signatures = opts.signatures ?? [] + if (signatures.length === 0) { throw new Error(`${pkg} carries no npm registry signature, so it cannot be verified.`) } - const key = NPM_SIGNING_KEYS.find(({ keyid }) => keyid === signature.keyid) - if (!key) { - throw new Error(`${pkg} is signed with an unexpected npm key (${signature.keyid}). ` + const match = signatures + .map((signature) => ({ signature, key: NPM_SIGNING_KEYS.find(({ keyid }) => keyid === signature.keyid) })) + .find(({ key }) => key != null) + if (!match?.key) { + throw new Error(`${pkg} is signed with an unexpected npm key (${signatures.map((s) => s.keyid).join(', ')}). ` + 'If npm has rotated its signing key, this action needs updating.') } + const { signature, key } = match🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install-pnpm/verify-signature.ts` around lines 28 - 37, Update the signature selection in the verification flow to search all entries in opts.signatures for one whose keyid matches a pinned key in NPM_SIGNING_KEYS, rather than assuming signatures[0] is valid. Preserve the existing errors for packages with no signatures and for signatures that have no matching pinned key, using the selected signature’s keyid in the latter message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install-pnpm/download.ts`:
- Around line 320-330: Update verifyIntegrity to report only pkg.name in the
checksum error, parse pkg.integrity by splitting at the first hyphen while
preserving the remainder as expected, and validate the algorithm against an
explicit supported-hash allowlist before calling createHash, raising a clear
error for unsupported algorithms.
---
Nitpick comments:
In `@src/install-pnpm/download.ts`:
- Around line 92-95: Define a named registry cutover constant, such as
MIN_REGISTRY_MAJOR = 12, alongside MIN_SUPPORTED_MAJOR, and update the
semver.major(version) comparison in the download selection logic to use it
instead of the literal 12.
In `@src/install-pnpm/verify-signature.ts`:
- Around line 28-37: Update the signature selection in the verification flow to
search all entries in opts.signatures for one whose keyid matches a pinned key
in NPM_SIGNING_KEYS, rather than assuming signatures[0] is valid. Preserve the
existing errors for packages with no signatures and for signatures that have no
matching pinned key, using the selected signature’s keyid in the latter message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d964069-c2ac-4acc-9d36-92be7d5c162e
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
src/install-pnpm/download.tssrc/install-pnpm/npm-signing-keys.tssrc/install-pnpm/run.tssrc/install-pnpm/verify-signature.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-11T16:19:49.450Z
Learnt from: zkochan
Repo: pnpm/setup PR: 1
File: src/cache-restore/run.ts:35-35
Timestamp: 2026-05-11T16:19:49.450Z
Learning: When using `actions/exec` (`getExecOutput` / `exec`), it is valid for the `commandLine` option to include both the command and its arguments in a single string (e.g., `getExecOutput('pnpm store path --silent')`). The library tokenizes `commandLine` internally (via `argStringToArray()`), so this behaves like passing an equivalent command + args array (e.g., `getExecOutput('pnpm', ['store','path','--silent'])`). In code reviews, do not flag this as incorrect—this matches documented behavior and a production-tested pattern.
Applied to files:
src/install-pnpm/run.tssrc/install-pnpm/download.tssrc/install-pnpm/npm-signing-keys.tssrc/install-pnpm/verify-signature.ts
🪛 Betterleaks (1.7.3)
src/install-pnpm/npm-signing-keys.ts
[high] 14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
src/install-pnpm/npm-signing-keys.ts (1)
1-29: LGTM!src/install-pnpm/download.ts (5)
5-10: LGTM!Also applies to: 24-63
109-116: LGTM!
158-170: LGTM!
291-318: LGTM!
123-144: 🗄️ Data Integrity & IntegrationNo change needed for platform package resolution.
The platform package names and the lifted tarball entries match the
keepvalues.src/install-pnpm/run.ts (1)
21-23: LGTM!src/install-pnpm/verify-signature.ts (1)
43-47: 🔒 Security & PrivacyNo change needed.
Line 44 builds the SPKI PEM body as a single unwrapped base64 line, and
createVerify().verify()accepts it with the DER ECDSAsiginput passed as base64.
The verification landed here as its own implementation — a pinned key list, a signature check, a registry download — because the package that does the same thing was not published yet. It is now, so this drops ~140 lines and depends on it. `downloadPnpm` from get-pnpm resolves the platform package, checks npm's signature over its checksum against the pinned key, checks the download against that checksum, and places the executable beside the `dist/` tree it loads. The action keeps what is its own: version resolution with semver ranges, the alias hardlinks, and PATH. That also puts the pinned keys in one place. get.pnpm.io refreshes them for all three of its installers on a schedule, so a rotation reaches this action through a dependency bump rather than through someone noticing. v11 still comes from the GitHub release: its `dist/` bundles dependencies that the registry copy declares instead, and this action has no step that would install them. get-pnpm is excluded from minimumReleaseAge, as pnpm asked when adding a package published minutes earlier.
|
Correction. This PR says v11 stays on the GitHub asset because "switching v11 would silently drop A controlled A/B — same filesystem, same pnpm version, only the dependency manifest suppressed — clones either way:
The release tarball agrees: its bundled So the constraint I built this around does not exist, and v11 could be fetched from the registry here too. That is worth doing rather than leaving: I have not changed it in this PR, because the remaining differences between the two v11 sources ( |
v11 was left on the GitHub release asset because the registry copy declares `@reflink/reflink` as a dependency where the release tarball bundles it, and this action has no step that would install it. That reasoning does not survive testing: an A/B with the dependency present and absent clones either way, and the tarball's bundled copy has no native binding at all. So every version this action installs now comes from the registry, and every one of them is checked against npm's signature. That matters more for v11 than for v12: `latest` resolves to v11, so until now the common case was verified only by GitHub's digest — served by the same host as the asset it describes. With no source left that needs it, the GitHub release lookup goes, and with it the asset naming, the sha256 check, the archive extraction, and the failure mode where a version published to npm without a release could not be installed at all. `token` existed to lift the anonymous rate limit on that lookup. It is now unused, kept as a deprecated input so workflows that pass it keep working. Up to v11 get-pnpm writes a manifest for `pnpm setup` to install the wrapper's dependencies from. This action never runs setup, so the manifest is removed rather than left as a stray project file in a directory that ends up on PATH — matching what the release tarball, which has no manifest, produced here before.
zkochan
changed the title
1 participant
Summary
The action verified the release archive against the
digestGitHub publishes for it. GitHub serves both the asset and the digest, so whoever can replace one can replace the other — that catches a corrupted download, not a tampered one.The npm registry carries the same executable, byte for byte, and npm signs
<name>@<version>:<integrity>with a keyget-pnpmpins. That signature can't be produced without npm's private key, and behind it sits the maintainer's approval of the staged publish. Every version this action installs now comes from the registry, and is refused unless the signature and the checksum both check out.I verified the executables are identical across both sources before relying on it:
f58dff16…f58dff16…180f2c62…180f2c62…pnpm.exe)290aac61…290aac61…v11 was going to stay on the release asset, because the registry copy declares
@reflink/reflinkas a dependency where the tarball bundles it, and this action has no step that would install it. That reasoning did not survive testing — an A/B with the dependency present and absent clones either way, and the tarball's bundled copy carries no native binding at all — so v11 comes from the registry too.That is the more important half:
latestresolves to v11, so until now the common case was the one verified only by GitHub's digest.Two things fall out of not touching the GitHub API at all
tokenis unused. It existed to lift the anonymous 60 requests/hour limit on the release lookup, which no longer happens. Kept as a deprecated input so workflows passing it keep working.With no source left that needs it, the release lookup, the asset naming, the sha256 check and the archive extraction are all gone —
download.tsloses about 200 lines.Testing
Every spec form, end to end against the real registry, each one placed and executed:
next-12dist, pn, pnpm, pnpx, pnxlatestdist, pn, pnpm, pnpx, pnx^11.0.0dist, pn, pnpm, pnpx, pnx11.20.0dist, pn, pnpm, pnpx, pnxThe existing smoke matrix covers Ubuntu (x64 + arm), macOS and Windows.
It uses
get-pnpmrather than its own copyThe verification first landed here as its own implementation, because the package that does the same thing wasn't published. It is now (
get-pnpm@0.0.1, from pnpm/get.pnpm.io), so the second commit deletes ~140 lines and depends on it.downloadPnpmverifies and places the executable and nothing else — it exists precisely so a caller that manages its own directory can use it without the global install and shell-rc editing thatpnpm setupdoes. The action keeps what is genuinely its own: version resolution with semver ranges, the alias hardlinks, and PATH.That also puts the pinned keys in one place.
get.pnpm.iorefreshes them for all three of its installers on a schedule, so a rotation reaches this action as a dependency bump rather than by someone noticing.get-pnpmis excluded fromminimumReleaseAgeinpnpm-workspace.yaml— pnpm added that itself when I installed a package published minutes earlier. Worth a look if you'd rather wait for it to age instead.Written by an agent (Claude Code, claude-opus-5).