feat(audit): add registry signature verification#11405
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
actor CLI as pnpm CLI
participant Cmd as Audit Command
participant Verifier as verifySignatures
participant Registry as Registry
participant Crypto as Crypto
CLI->>Cmd: run `pnpm audit signatures`
Cmd->>Cmd: loadAuditContext(), build package list, pick registries
Cmd->>Registry: GET /-/npm/v1/keys (per registry)
Registry-->>Cmd: signing keys or 400/404 (no keys)
Cmd->>Verifier: verifySignatures(packages, getAuthHeader, opts)
Verifier->>Registry: GET /<package> (packument, fullMetadata)
Registry-->>Verifier: packument with dist.integrity, tarball, signatures, time
loop per package (concurrent, limited)
Verifier->>Crypto: verify `${name}@${version}:${integrity}` with public key
Crypto-->>Verifier: valid / invalid
end
Verifier-->>Cmd: { audited, verified[], invalid[], missing[] }
Cmd->>CLI: render report and exitCode
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/compliance/audit/src/verifySignatures.ts`:
- Around line 102-118: The code currently assumes version.dist.signatures is a
well-formed array and calls verifyPackageSignatures which can throw on malformed
input; before calling verifyPackageSignatures (in verifySignatures.ts where
version, signatures and verifyPackageSignatures are used) add defensive
validation that version?.dist?.signatures is an Array and that each entry has
the expected shape (e.g., required properties used by verifyPackageSignatures);
if the shape is wrong, push the package into result.invalid or result.missing
with an appropriate reason (e.g., "Malformed signatures metadata") and return
instead of invoking verifyPackageSignatures.
In `@deps/compliance/commands/src/audit/signatures.ts`:
- Around line 74-77: The message "No dependencies were installed from a registry
with signing keys" is emitted when result.audited === 0 even if there are
recorded issues (e.g., result.invalid), which is misleading; update the
condition that pushes that message to only run when the result is truly empty by
checking audited === 0 AND that there are no entries in result.invalid (and any
other issue arrays used in this module, e.g., result.failed or result.errors)
before pushing to lines; locate the block that currently tests result.audited
and change it to a combined check (e.g., result.audited === 0 &&
(!result.invalid || result.invalid.length === 0) && (!result.failed ||
result.failed.length === 0)) so the "No dependencies..." message only appears
for an actually empty result state.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc77cfef-889d-43d1-90da-f8a97b401d34
⛔ Files ignored due to path filters (2)
deps/compliance/commands/test/audit/fixtures/has-signatures/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/audit-signatures.mdcspell.jsondeps/compliance/audit/README.mddeps/compliance/audit/package.jsondeps/compliance/audit/src/index.tsdeps/compliance/audit/src/verifySignatures.tsdeps/compliance/audit/test/verifySignatures.tsdeps/compliance/commands/package.jsondeps/compliance/commands/src/audit/audit.tsdeps/compliance/commands/src/audit/auditContext.tsdeps/compliance/commands/src/audit/signatures.tsdeps/compliance/commands/test/audit/fixtures/has-signatures/package.jsondeps/compliance/commands/test/audit/index.tsdeps/compliance/commands/tsconfig.json
a378a0a to
ac30f3d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deps/compliance/commands/test/audit/index.ts (1)
396-427: 💤 Low valueConsider extracting shared test utilities.
The
createSigningKeyandmockRegistryKeyhelper functions are duplicated between this file anddeps/compliance/audit/test/verifySignatures.ts. While test file duplication is sometimes acceptable for self-containment, if these helpers are expected to be reused across more test files, consider extracting them to a shared test utilities module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/compliance/commands/test/audit/index.ts` around lines 396 - 427, Extract the duplicated helpers createSigningKey and mockRegistryKey into a new shared test utilities module and replace the local definitions in both deps/compliance/commands/test/audit/index.ts and deps/compliance/audit/test/verifySignatures.ts with imports; specifically, move the crypto-based createSigningKey function (including its return shape: keyid, publicKey, sign) and the mockRegistryKey implementation that uses getMockAgent().get(...).intercept(...).reply(...) into the shared module, export them, update both files to import createSigningKey and mockRegistryKey (preserving types such as ReturnType<typeof createSigningKey> where used), and ensure any references to getMockAgent remain valid by importing it where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deps/compliance/commands/test/audit/index.ts`:
- Around line 396-427: Extract the duplicated helpers createSigningKey and
mockRegistryKey into a new shared test utilities module and replace the local
definitions in both deps/compliance/commands/test/audit/index.ts and
deps/compliance/audit/test/verifySignatures.ts with imports; specifically, move
the crypto-based createSigningKey function (including its return shape: keyid,
publicKey, sign) and the mockRegistryKey implementation that uses
getMockAgent().get(...).intercept(...).reply(...) into the shared module, export
them, update both files to import createSigningKey and mockRegistryKey
(preserving types such as ReturnType<typeof createSigningKey> where used), and
ensure any references to getMockAgent remain valid by importing it where needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb972617-8531-4e08-bc3a-dbeb51e504ee
⛔ Files ignored due to path filters (1)
deps/compliance/commands/test/audit/fixtures/has-signatures/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/audit-signatures.mdcspell.jsondeps/compliance/audit/README.mddeps/compliance/audit/package.jsondeps/compliance/audit/src/index.tsdeps/compliance/audit/src/verifySignatures.tsdeps/compliance/audit/test/verifySignatures.tsdeps/compliance/commands/package.jsondeps/compliance/commands/src/audit/audit.tsdeps/compliance/commands/src/audit/auditContext.tsdeps/compliance/commands/src/audit/signatures.tsdeps/compliance/commands/test/audit/fixtures/has-signatures/package.jsondeps/compliance/commands/test/audit/index.tsdeps/compliance/commands/tsconfig.json
✅ Files skipped from review due to trivial changes (8)
- deps/compliance/audit/package.json
- deps/compliance/commands/package.json
- deps/compliance/commands/tsconfig.json
- cspell.json
- deps/compliance/commands/test/audit/fixtures/has-signatures/package.json
- .changeset/audit-signatures.md
- deps/compliance/audit/README.md
- deps/compliance/audit/src/verifySignatures.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- deps/compliance/audit/src/index.ts
A registry returning malformed PEM key material made verifier.verify throw synchronously, rejecting the Promise.all and crashing the whole audit run. Treat any verify failure as an invalid signature for that single package.
Both fetchRegistryKeys and fetchPackument repeated the same JSON.parse + PnpmError wrapping pattern. Collapse into a single helper.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deps/compliance/audit/src/verifySignatures.ts (1)
193-198: ⚡ Quick winReduce
getPackumentparameter count to meet repo guideline.
getPackumentcurrently takes 4 arguments; wrap context dependencies into a single object.As per coding guidelines, "limit functions to two or three arguments or use a single options object instead".♻️ Proposed refactor
- const packument = await getPackument(pkg, getAuthHeader, opts, packumentCache) + const packument = await getPackument(pkg, { getAuthHeader, opts, packumentCache }) +interface GetPackumentContext { + getAuthHeader: GetAuthHeader + opts: VerifySignaturesOptions + packumentCache: Map<string, Promise<Packument | undefined>> +} + async function getPackument ( pkg: SignaturePackage, - getAuthHeader: GetAuthHeader, - opts: VerifySignaturesOptions, - packumentCache: Map<string, Promise<Packument | undefined>> + ctx: GetPackumentContext ): Promise<Packument | undefined> { + const { getAuthHeader, opts, packumentCache } = ctx const cacheKey = `${pkg.registry}:${pkg.name}` let packument = packumentCache.get(cacheKey) if (!packument) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/compliance/audit/src/verifySignatures.ts` around lines 193 - 198, The function getPackument currently takes four separate parameters (pkg: SignaturePackage, getAuthHeader: GetAuthHeader, opts: VerifySignaturesOptions, packumentCache: Map<string, Promise<Packument | undefined>>) — refactor it to accept a single options object (e.g., { pkg, getAuthHeader, opts, packumentCache }) and update the function signature and body to destructure those fields; then update all call sites to pass the new options object instead of four positional args and adjust any TypeScript types/interfaces to reference the new options type so callers and inference remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deps/compliance/audit/src/verifySignatures.ts`:
- Around line 193-198: The function getPackument currently takes four separate
parameters (pkg: SignaturePackage, getAuthHeader: GetAuthHeader, opts:
VerifySignaturesOptions, packumentCache: Map<string, Promise<Packument |
undefined>>) — refactor it to accept a single options object (e.g., { pkg,
getAuthHeader, opts, packumentCache }) and update the function signature and
body to destructure those fields; then update all call sites to pass the new
options object instead of four positional args and adjust any TypeScript
types/interfaces to reference the new options type so callers and inference
remain correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1eca668b-cf39-4120-b14b-1b6a83725fca
📒 Files selected for processing (1)
deps/compliance/audit/src/verifySignatures.ts
Move verifySignatures from @pnpm/deps.compliance.audit into a new @pnpm/deps.compliance.signatures package. Vulnerability auditing and signature verification are conceptually distinct trust subsystems, and sigstore provenance verification is in scope for a future change — keeping all signature work in its own package avoids growing the audit module into two unrelated concerns.
The signature verification implementation moved to @pnpm/deps.compliance.signatures; that package's README documents the behavior. The audit package no longer needs to mention it.
Place the new signature verification package under deps/security/ rather than deps/compliance/. Compliance is a fuzzy fit for tamper detection; security is the right home, and sigstore provenance verification (future scope) will live alongside it. Existing audit/license/sbom packages stay where they are — this only changes where the new package lands.
|
Congrats on merging your first pull request! 🎉🎉🎉 |
Summary
As reported in #7909, I tried running
pnpm audit signatures, assuming a similar behavior as npm audit signatures. The positionalsignaturesargument was accepted but ignored, so pnpm ran a normal vulnerability audit instead of doing any registry signature verification.This PR routes
signaturesas anpnpm auditsubcommand and adds ECDSA registry signature verification for installed packages. The implementation reuses the lockfile audit indexing logic to collect the installed package/version pairs, fetches the registry signing keys from/-/npm/v1/keys, fetches full packuments for packages from registries that provide keys, and verifies eachdist.signaturesentry against the signed${name}@${version}:${integrity}payload. Unknown audit subcommands now fail explicitly instead of being silently ignored.NPM's implementation of
npm audit signatureschecks sigstore provenance attestations in addition to registry ECDSA signatures. While these would provide an additional trust signal about how a package was built and published, I chose to leave them out of this PR. They have a larger implementation and policy surface: TUF state, certificate transparency verification, attestation bundle fetching, identity checks, and evolving attestation formats.Test Plan
pnpm initin a new project. Add a.npmrcfile withregistry=https://registry.npmjs.org/.pnpm add lodash && pd audit signatures. Confirm the command succeeds with:pnpm remove lodash. Runnode fake-registry.mjs(see below) and update.npmrcwithregistry=http://127.0.0.1:4873.pnpm add valid@1.0.0 && pd audit signatures. Confirm the command succeeds with:pnpm add missing@1.0.0 && pd audit signatures. Confirm the command fails with:pnpm add tampered@1.0.0 && pd audit signatures. Confirm the command fails with:audited 3 packages 1 package has a verified registry signature 1 package is missing registry signature but the registry is providing signing keys: ┌───────────────┬────────────────────────┐ │ missing@1.0.0 │ http://127.0.0.1:4873/ │ └───────────────┴────────────────────────┘ 1 package has an invalid registry signature: ┌────────────────┬────────────────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ tampered@1.0.0 │ http://127.0.0.1:4873/ │ tampered@1.0.0 has an invalid registry signature with keyid SHA256:3O1jPM9qsCevotALUCf5xcwcDYW6IOm7bzozhEngVJ4 │ └────────────────┴────────────────────────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ Someone might have tampered with this package since it was published on the registry!pd audit signatures --json. The JSON output should look like:{ "audited": 3, "invalid": [ { "integrity": "sha512-ajg3qLh9/4tt+64vlvZezWkdgRQ90u8yPr6y6WIS1vTOooZZSXWBbGH2K0ttGtVgN5QuJ/lcZmrsnluXe9MKvw==", "name": "tampered", "reason": "tampered@1.0.0 has an invalid registry signature with keyid SHA256:3O1jPM9qsCevotALUCf5xcwcDYW6IOm7bzozhEngVJ4", "registry": "http://127.0.0.1:4873/", "resolved": "http://127.0.0.1:4873/tampered-1.0.0.tgz", "version": "1.0.0" } ], "missing": [ { "name": "missing", "registry": "http://127.0.0.1:4873/", "version": "1.0.0", "integrity": "sha512-YcKW2tvRGzAGBdd00Outu+u3LKx3eouD7LmyLMc/mLWiO+/3Irw8WfXjlQEwPno6I+Ne9FxF956nKtb3qQZiAg==", "resolved": "http://127.0.0.1:4873/missing-1.0.0.tgz" } ], "verified": 1 }pd audit unknown-> this should now fail with:Simulating an NPM registry
Summary by CodeRabbit
New Features
pnpm audit signaturescommand to verify installed package ECDSA registry signatures; respects scoped registries, skips registries without signing keys, supports JSON or colored CLI reports, and returns non-zero when signatures are missing/invalid. CLI help updated.Documentation
Tests
Chores