Repository navigation
feat: install pnpm from the npm registry, verified against npm's signature - #24
Merged
Merged
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.
Member
Author
|
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
4 tasks done
fionnachan
added a commit
to OffchainLabs/arbitrum-portal
that referenced
this pull request
Aug 12, 2026
Context: The workflow now fails at the pnpm/setup step, before any build runs: Downloading pnpm 11.13.0 from the npm registry Error: ENOENT: no such file or directory, rename '/home/runner/setup-pnpm/.unpack/@pnpm-linux-x64/package/pnpm' pnpm/setup@v2 moved from v2.0.1 to v2.0.2, which changed pnpm provisioning from GitHub releases to the npm registry (pnpm/setup#24) and expects the native binary at @pnpm/<platform>/package/pnpm. Unpacking the tarballs shows @pnpm/linux-x64@11.13.0 contains only README, package.json, LICENSE and CHANGELOG, no binary at all. Every release from 11.13.1 onward ships package/pnpm, so 11.13.0 is the last version whose platform package is a stub, and the repo was pinned to exactly it. Why: Moving the pin past the stub fixes CI forward and keeps the single step setup from 808dd80. Two alternatives were rejected: pinning the action to d36251dc (v2.0.1) works but strands this workflow on a superseded release for as long as pnpm/setup#29 stays open, and reverting to action-setup plus setup-node undoes the modernization for a bug that is not in that pairing. 11.21.0 is the current latest rather than 11.13.1, so the pin does not sit one patch above a known bad release. Note this is a repo wide toolchain bump, not a workflow local one. The other workflows use pnpm/action-setup@v5, which reads packageManager too, so they move to 11.21.0 as well. They were passing on 11.13.0, and the install is a no-op against the existing lockfile. What: - packageManager: pnpm@11.13.0 to pnpm@11.21.0. Verified locally: pnpm provisions 11.21.0 from the field with no corepack, pnpm install --frozen-lockfile reports the lockfile up to date and passes the new supply chain policy check, pnpm-lock.yaml is unchanged (still lockfileVersion 9.0), and prettier passes. References: - pnpm/setup#29 (v2.0.2 failing, open) - pnpm/setup#24 (install pnpm from the npm registry, the change that introduced this) - 01c0458 ci: use package.json pnpm version - 808dd80 ci: modernize add-orbit-chain action versions and pnpm setup Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Open
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
1 participant
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.
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).