Conversation
|
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 lockfile parsing validation rejecting tarball-shaped resolutions missing an ChangesTarball Integrity Validation
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
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)
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 |
Review Summary by QodoRequire integrity field for tarball-shaped lockfile resolutions
WalkthroughsDescription• Reject tarball resolutions missing integrity field at lockfile-read time • Prevents attacker from installing tampered packages via lockfile tampering • Adds ERR_PNPM_MISSING_TARBALL_INTEGRITY error for all tarball types • Covers plain remote, registry, file:, and git-hosted tarball entries Diagramflowchart LR
A["Lockfile Read"] --> B["pkgSnapshotToResolution"]
B --> C{"Tarball-shaped<br/>resolution?"}
C -->|Yes| D{"Has integrity<br/>field?"}
D -->|No| E["Throw ERR_PNPM_MISSING_TARBALL_INTEGRITY"]
D -->|Yes| F["Return resolution"]
C -->|No| F
E --> G["Installation blocked"]
F --> H["Safe installation"]
File Changes1. lockfile/utils/src/pkgSnapshotToResolution.ts
|
…n in snapshot_cache_key Match the upstream guard landed alongside #11966 (`lockfile/utils/src/pkgSnapshotToResolution.ts`) with a test on the pacquet side: a `LockfileResolution::Tarball` with `integrity: None` — what a tampered lockfile looks like — must short-circuit the warm-batch cache-key derivation by surfacing `InstallPackageBySnapshotError::MissingTarballIntegrity`. The structural guard already existed but had no negative test.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/create_virtual_store/tests.rs (1)
258-267: ⚡ Quick winTrim the test doc comment to the contract-level intent only.
This block duplicates behavioral detail already captured by the test body; keep only brief why/contract context.
As per coding guidelines, "Tests are documentation. Do not duplicate them in prose." and "Doc comments ... are not a re-narration of the body."
🤖 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 `@pacquet/crates/package-manager/src/create_virtual_store/tests.rs` around lines 258 - 267, The test doc comment above the test for snapshot_cache_key/MissingTarballIntegrity is overly detailed; trim it to a single-sentence contract-level intent describing that a Tarball resolution missing integrity must be rejected (e.g., "Ensure snapshot_cache_key returns MissingTarballIntegrity for resolutions missing integrity"), remove the implementation/rationale history and upstream references (pnpm/pnpm#11966) so the comment only states the expected behavior and context.
🤖 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.
Nitpick comments:
In `@pacquet/crates/package-manager/src/create_virtual_store/tests.rs`:
- Around line 258-267: The test doc comment above the test for
snapshot_cache_key/MissingTarballIntegrity is overly detailed; trim it to a
single-sentence contract-level intent describing that a Tarball resolution
missing integrity must be rejected (e.g., "Ensure snapshot_cache_key returns
MissingTarballIntegrity for resolutions missing integrity"), remove the
implementation/rationale history and upstream references (pnpm/pnpm#11966) so
the comment only states the expected behavior and context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d82f97d6-aff0-4150-9460-bf619729fdea
📒 Files selected for processing (1)
pacquet/crates/package-manager/src/create_virtual_store/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/package-manager/src/create_virtual_store/tests.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/package-manager/src/create_virtual_store/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/create_virtual_store/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/package-manager/src/create_virtual_store/tests.rs
🔇 Additional comments (1)
pacquet/crates/package-manager/src/create_virtual_store/tests.rs (1)
2-3: LGTM!Also applies to: 269-305
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11966 +/- ##
==========================================
+ Coverage 87.93% 87.95% +0.01%
==========================================
Files 228 228
Lines 27810 27819 +9
==========================================
+ Hits 24456 24469 +13
+ Misses 3354 3350 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.9245815834800002,
"stddev": 0.0527983981835926,
"median": 1.9144956128800001,
"user": 2.7676179000000003,
"system": 3.2526983199999995,
"min": 1.8463744633799999,
"max": 2.0220719483800003,
"times": [
1.94003576238,
1.90310172338,
1.9095198263800002,
1.8463744633799999,
1.98045952638,
1.8623207613800001,
2.0220719483800003,
1.9045097773800002,
1.95795064638,
1.91947139938
]
},
{
"command": "pacquet@main",
"mean": 1.90589989538,
"stddev": 0.043962395887157724,
"median": 1.89668446438,
"user": 2.7572688,
"system": 3.213904219999999,
"min": 1.84148682738,
"max": 1.98381722638,
"times": [
1.94668986638,
1.87584375638,
1.84148682738,
1.90018175038,
1.9306281333800002,
1.9406379813799999,
1.85803101438,
1.88849521938,
1.98381722638,
1.89318717838
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6391949954999999,
"stddev": 0.03354166546545221,
"median": 0.6275619306000001,
"user": 0.35133523999999994,
"system": 1.3478787999999997,
"min": 0.6132971591,
"max": 0.7306369511,
"times": [
0.7306369511,
0.6232349901,
0.6256207991000001,
0.6275941581000001,
0.6254678531000001,
0.6275297031000001,
0.6329519091000001,
0.6508673841000001,
0.6347490481000001,
0.6132971591
]
},
{
"command": "pacquet@main",
"mean": 0.640504841,
"stddev": 0.01080651134339348,
"median": 0.6378664426,
"user": 0.36111313999999994,
"system": 1.3567674999999997,
"min": 0.6288607891000001,
"max": 0.6669768101000001,
"times": [
0.6669768101000001,
0.6418517551,
0.6398801531000001,
0.6350400031000001,
0.6288607891000001,
0.6343026451000001,
0.6330376001000001,
0.6358527321,
0.6486739711000001,
0.6405719511000001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2720905974999996,
"stddev": 0.06066994237659803,
"median": 2.2582653174000002,
"user": 3.85872264,
"system": 3.0269531599999993,
"min": 2.2102259318999997,
"max": 2.4153205419,
"times": [
2.2198657299,
2.4153205419,
2.2402636249,
2.3362212599,
2.2509791059,
2.2616740309,
2.2663663589,
2.2548566039,
2.2651327868999998,
2.2102259318999997
]
},
{
"command": "pacquet@main",
"mean": 2.2077301376999996,
"stddev": 0.03563975784746927,
"median": 2.2125300158999996,
"user": 3.8479572399999995,
"system": 3.0026020599999996,
"min": 2.1615463969,
"max": 2.2613509999,
"times": [
2.1646443558999997,
2.2028921039,
2.2201200438999997,
2.2120889888999997,
2.2195473938999997,
2.2129710429,
2.2567967759,
2.1653432748999997,
2.2613509999,
2.1615463969
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.43125309546,
"stddev": 0.01581410854166845,
"median": 1.43156360036,
"user": 1.69980844,
"system": 1.8585918199999998,
"min": 1.4083581453600003,
"max": 1.4562338883600001,
"times": [
1.4358264803600003,
1.4533736483600002,
1.4090904963600002,
1.4292671063600002,
1.4255496123600002,
1.4358098383600002,
1.4083581453600003,
1.4338600943600002,
1.4251616443600001,
1.4562338883600001
]
},
{
"command": "pacquet@main",
"mean": 1.43487602646,
"stddev": 0.059168490401430705,
"median": 1.41619060936,
"user": 1.7072929399999999,
"system": 1.8171071199999997,
"min": 1.39115108836,
"max": 1.5955969543600002,
"times": [
1.4134406413600002,
1.43877908636,
1.4092668943600002,
1.45186349036,
1.5955969543600002,
1.4212932873600002,
1.39115108836,
1.4005222273600002,
1.4189405773600001,
1.4079060173600002
]
}
]
} |
|
Bencher Report| Branch | pr/11966 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,272.09 ms(-15.23%)Baseline: 2,680.22 ms | 3,216.27 ms (70.64%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,431.25 ms(-21.03%)Baseline: 1,812.37 ms | 2,174.84 ms (65.81%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 1,924.58 ms(-6.23%)Baseline: 2,052.38 ms | 2,462.85 ms (78.14%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 639.19 ms(-2.65%)Baseline: 656.60 ms | 787.92 ms (81.12%) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lockfile/utils/src/toLockfileResolution.ts (1)
77-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten git-hosted URL detection to commit-pinned patterns only.
The current predicate (
known host+contains tar.gz) is too permissive and can classify mutable git-host tarballs (e.g., branch/tag archives) as exempt fromintegrity, weakening the fail-closed guarantee.Use a stricter matcher that only accepts commit-pinned URL shapes (or remove URL fallback and rely solely on explicit
gitHosted: truewhen safely produced upstream).🤖 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 `@lockfile/utils/src/toLockfileResolution.ts` around lines 77 - 83, The isGitHostedTarballUrl predicate is too permissive (host + 'tar.gz') and must be tightened to only accept commit-pinned tarball shapes or be removed in favor of an explicit gitHosted flag; update the isGitHostedTarballUrl function to match commit-specific URL patterns (e.g., codeload.github.com/.../tar.gz/<commit-sha> or .../archive/<commit-sha>.tar.gz and equivalent GitLab/Bitbucket patterns that include a full commit SHA or explicit commit segment) and reject branch/tag archive URLs, or remove the fallback and rely on upstream-produced gitHosted: true where available. Ensure the matcher validates a commit SHA (40 hex chars or valid shortened SHA) in the URL path so mutable archives are not treated as safe.
🤖 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 @.changeset/require-tarball-integrity.md:
- Line 6: Replace the lowercase platform name in the release note prose: find
the occurrence of "github.com" inside the .changeset markdown content (the
sentence listing exempt git-hosted tarballs) and change it to the official
capitalization "GitHub" so the text reads "GitHub / bitbucket.org / gitlab.com"
(or similar) for consistent branding.
---
Outside diff comments:
In `@lockfile/utils/src/toLockfileResolution.ts`:
- Around line 77-83: The isGitHostedTarballUrl predicate is too permissive (host
+ 'tar.gz') and must be tightened to only accept commit-pinned tarball shapes or
be removed in favor of an explicit gitHosted flag; update the
isGitHostedTarballUrl function to match commit-specific URL patterns (e.g.,
codeload.github.com/.../tar.gz/<commit-sha> or .../archive/<commit-sha>.tar.gz
and equivalent GitLab/Bitbucket patterns that include a full commit SHA or
explicit commit segment) and reject branch/tag archive URLs, or remove the
fallback and rely on upstream-produced gitHosted: true where available. Ensure
the matcher validates a commit SHA (40 hex chars or valid shortened SHA) in the
URL path so mutable archives are not treated as safe.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9afff73-2e6f-4552-9dba-8999ad8ce147
📒 Files selected for processing (4)
.changeset/require-tarball-integrity.mdlockfile/utils/src/pkgSnapshotToResolution.tslockfile/utils/src/toLockfileResolution.tslockfile/utils/test/pkgSnapshotToResolution.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
lockfile/utils/src/toLockfileResolution.tslockfile/utils/test/pkgSnapshotToResolution.tslockfile/utils/src/pkgSnapshotToResolution.ts
🧠 Learnings (1)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
lockfile/utils/src/toLockfileResolution.tslockfile/utils/test/pkgSnapshotToResolution.tslockfile/utils/src/pkgSnapshotToResolution.ts
🪛 LanguageTool
.changeset/require-tarball-integrity.md
[uncategorized] ~6-~6: The official name of this software platform is spelled with a capital “H”.
Context: ...(gitHosted: true or a URL on codeload.github.com / bitbucket.org / gitlab.com) and `...
(GITHUB)
…solutions A tampered lockfile that strips the `integrity` field from a tarball resolution let the worker download the URL contents and mint a fresh integrity from the unverified bytes, so an attacker who could also serve content at the referenced URL would install a tampered package without any error — including under `--frozen-lockfile`. pnpm now rejects such entries at lockfile-read time with `ERR_PNPM_MISSING_TARBALL_INTEGRITY`, matching pacquet's existing `pacquet_package_manager::missing_tarball_integrity` guard.
…ls strict typecheck
…n in snapshot_cache_key Match the upstream guard landed alongside #11966 (`lockfile/utils/src/pkgSnapshotToResolution.ts`) with a test on the pacquet side: a `LockfileResolution::Tarball` with `integrity: None` — what a tampered lockfile looks like — must short-circuit the warm-batch cache-key derivation by surfacing `InstallPackageBySnapshotError::MissingTarballIntegrity`. The structural guard already existed but had no negative test.
…tegrity guard The strict guard added in the parent commit broke pnpm's own `with-git-protocol-dep` and `with-non-package-dep` fixtures: the install pipeline writes git-hosted tarball entries (codeload.github.com URLs) to the lockfile without an `integrity:` line, because the commit SHA in the URL is the integrity anchor — git's content-addressed model binds the bytes to the commit, so a separate hash adds nothing. Exempt git-hosted tarballs (detected either via the `gitHosted: true` flag or a URL on the known git hosts, matching the URL fallback in `toLockfileResolution`) and `file:` tarballs (local paths the user already controls). The strict check still fires for any other remote tarball — which is where the AutoFyn-reported vector actually manifests. Also export `isGitHostedTarballUrl` from `toLockfileResolution.ts` so the URL fallback can be shared rather than duplicated.
… intent Per the repo convention that tests are documentation, the test name and body already cover what's being asserted; the prior comment duplicated that. Keep only the non-obvious why: why this guard exists at the cache-key site at all (warm-batch short-circuit) when the install-side check also rejects the same input.
|
Responses to the two review-body comments: Nitpick on
|
…ort #11966 to v10) (#12007) A tampered `pnpm-lock.yaml` that strips the `integrity` field from a tarball resolution let the worker download the URL contents and mint a fresh integrity from the unverified bytes. An attacker who could both alter the lockfile (e.g. via a pull request that drops `integrity:`) and serve modified content at the referenced tarball URL could install a tampered package without any error — including under `--frozen-lockfile`. `pkgSnapshotToResolution` now fails closed at lockfile-read time with `ERR_PNPM_MISSING_TARBALL_INTEGRITY` whenever a tarball-shaped resolution (no `type` field — covers plain remote, registry-derived, `file:`, and `gitHosted` entries) lacks integrity. Git-hosted tarballs and `file:` tarballs remain exempt: the commit SHA in a git-host URL and the user-controlled local path already anchor the bytes. The fix sits at the lockfile-read chokepoint every install path flows through (deps-resolver, deps-restorer, graph-builder), so both isolated and hoisted node-linkers are covered. Credit to AutoFyn for finding and reporting the issue.
Summary
A tampered
pnpm-lock.yamlthat strips theintegrityfield from a tarball resolution let the worker download the URL contents and mint a fresh integrity from the unverified bytes (worker/src/start.ts:189-204:if (integrity)skipped verification when absent, thenintegrity: integrity ?? calcIntegrity(buffer)stored whatever it got). An attacker who could both alter the lockfile (e.g. via a pull request that dropsintegrity:) and serve modified content at the referenced tarball URL could install a tampered package without any error — including under--frozen-lockfile.pkgSnapshotToResolutionnow fails closed at lockfile-read time withERR_PNPM_MISSING_TARBALL_INTEGRITYwhenever a tarball-shaped resolution (notypefield — covers plain remote, registry-derived,file:, andgitHostedentries) lacks integrity. This matches pacquet's existingpacquet_package_manager::missing_tarball_integrityguard atinstall_package_by_snapshot.rs:113-114.The fix sits at the lockfile-read chokepoint every install path flows through (
deps-resolver,deps-restorer,graph-builder), so both isolated and hoisted node-linkers are covered. The worker's ownif (integrity)mint path is intentionally left in place because it is still the legitimate fresh-install code path for resolutions that have not yet been recorded in a lockfile (the resolver attaches the computed integrity back to the resolution atpackageRequester.ts:297-298before the lockfile write).I scanned every
pnpm-lock.yamlfixture in the repo for tarball entries without integrity — there are none, so no existing test breaks from the stricter check.Pacquet's structural guard for the same condition already existed at both the canonical install-side site (
tarball_url_and_integrity) and the upfront warm-batch short-circuit (snapshot_cache_key), but had no negative test. Added one againstsnapshot_cache_keythat constructs aLockfileResolution::Tarball { integrity: None }— the exact shape a tampered lockfile produces — and asserts the orchestrator surfacesInstallPackageBySnapshotError::MissingTarballIntegrityupfront, before the rayon batch runs.Credit to AutoFyn for finding and reporting the issue.
Test plan
lockfile/utils/test/pkgSnapshotToResolution.tsasserts the throw fires for plain remote, git-hosted, andfile:tarball entries that lack integrity.snapshot_cache_key_rejects_tarball_without_integritytest inpacquet/crates/package-manager/src/create_virtual_store/tests.rscovers the equivalent guard on the pacquet side.pnpm --filter @pnpm/lockfile.utils run lintclean.pnpm --filter @pnpm/lockfile.utils test pkgSnapshotToResolution.tspasses.cargo nextest run -p pacquet-package-manager snapshot_cache_key— all three (including the new one) pass.cargo clippy --locked -p pacquet-package-manager --all-targets -- --deny warningsclean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests
Documentation