fix(pacquet): resolve package-manager bootstrap through trusted registries#12471
Merged
Conversation
PR Summary by QodoFix pacquet package-manager bootstrap to use trusted registries only Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Bootstrap reads repo cache
|
Comment on lines
1859
to
1875
| // Package-manager bootstrap (auto-switching to the `packageManager` | ||
| // / `devEngines.packageManager` version) must resolve through | ||
| // TRUSTED sources only — the builtin default, user `.npmrc`, | ||
| // `auth.ini`, and URL-scoped env vars — never the project `.npmrc` | ||
| // or `pnpm-workspace.yaml`, which a malicious repository controls. | ||
| // Capture the trusted subset (everything except `project_source`) | ||
| // before the full fold consumes the sources. Mirrors pnpm's | ||
| // `packageManagerRegistries` / `packageManagerNetworkConfig`, built | ||
| // from `trustedConfig` | ||
| // ([`config/reader/src/index.ts`](https://github.com/pnpm/pnpm/blob/822beb5fa0/config/reader/src/index.ts#L328-L342)). | ||
| // See GHSA-j2hc-m6cf-6jm8. | ||
| let trusted_sources = | ||
| [env_scoped_source.clone(), auth_ini_source.clone(), user_source.clone()]; | ||
|
|
||
| // Fold high-priority-first: the first present source is the | ||
| // base, each lower source fills the gaps it left | ||
| // ([`NpmrcAuth::merge_under`]). |
There was a problem hiding this comment.
1. Bootstrap skips global config 🐞 Bug ≡ Correctness
Config::current builds package_manager_bootstrap before applying the trusted global config.yaml layer, so any user-configured global registry/registries affect normal installs but are ignored for package-manager bootstrap resolution. This can cause package-manager bootstrap to resolve via the npm default (or other earlier source) even when the user configured a trusted mirror globally, leading to surprising behavior or failures in restricted-network setups.
Agent Prompt
### Issue description
`package_manager_bootstrap` is constructed from trusted `.npmrc`/auth sources, but it is finalized **before** the trusted global config.yaml (`WorkspaceSettings::load_global`) is applied. As a result, a user’s global registry mirror (or global `registries:` routes) can steer normal installs but will not steer package-manager bootstrap resolution.
### Issue Context
- Bootstrap is created from a trusted subset of npmrc sources and stored in `self.package_manager_bootstrap`.
- The trusted global config.yaml layer is applied later to `self` (normal config), but never reflected into `package_manager_bootstrap`.
- Global settings can set `registry` and `registries`, and those are trusted/user-controlled (not repository-controlled).
### Fix Focus Areas
- pacquet/crates/config/src/lib.rs[1751-1943]
- pacquet/crates/config/src/lib.rs[1859-1889]
### Implementation sketch
- When building `package_manager_bootstrap`, also incorporate the trusted global settings layer (if present) **with the same precedence as normal config** but still excluding repository-controlled sources.
- Option A: Build a temporary `Config` for bootstrap (as today), then if `global_settings` is `Some`, apply it onto that temporary config (using the same save/restore for `workspace_dir` if needed), then extract `registry`, `registries`, `proxy`, `tls`, `tls_by_uri`, `auth_headers` into `PackageManagerBootstrap`.
- Option B: After applying `global_settings` to `self`, also update `self.package_manager_bootstrap.registry`/`.registries` from `global_settings`’ resolved values (keeping bootstrap’s proxy/tls/auth as-is), ensuring normalization matches `WorkspaceSettings::apply_to`.
- Keep the existing `PNPM_CONFIG_REGISTRY` override behavior for bootstrap.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
This is intentional, for parity with the TypeScript implementation. Upstream computes packageManagerRegistries from npmrcResult.trustedConfig (the .npmrc layers only); the global config.yaml registry is applied to the normal config but is never folded into the package-manager bootstrap registries either (config/reader/src/index.ts builds packageManagerRegistries from pickIniConfig(npmrcResult.trustedConfig), not from the global workspace-manifest config). pacquet mirrors that: the trusted bootstrap set is the builtin default + user .npmrc + auth.ini + URL-scoped env, and PNPM_CONFIG_REGISTRY overrides the default.
Per the pacquet cardinal rule (match pnpm exactly; don't "fix" pnpm behavior unless the same fix lands upstream first), I'm leaving this aligned with pnpm rather than adding global config.yaml to the bootstrap set. If we want global config to steer package-manager bootstrap, that should be a coordinated change in both stacks.
Written by an agent (Claude Code, claude-opus-4-8).
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces ChangesTrusted-source bootstrap for package manager resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pacquet/crates/config/src/lib.rs`:
- Around line 1451-1460: The `PackageManagerBootstrap` struct currently derives
Default which leaves the registry field as an empty string, causing
package-manager resolution to fail. Remove the Default derivation from the
struct definition and implement a custom Default trait for
PackageManagerBootstrap that explicitly sets the registry field to a proper
default value such as npmjs, while keeping the other fields at their default
values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85a0d671-8d18-4fd7-870e-7af9493b9414
📒 Files selected for processing (5)
pacquet/crates/cli/src/config_deps.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/npmrc_auth.rspacquet/crates/config/src/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rs
zkochan
added a commit
that referenced
this pull request
Jun 17, 2026
Give `PackageManagerBootstrap` a SmartDefault registry of the public npm registry so a directly-constructed `Config` (one not finalized through `Config::current`) never resolves the package manager against an empty registry. Addresses a CodeRabbit review note on #12471.
|
Code review by qodo was updated up to the latest commit a147d39 |
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12471 +/- ##
==========================================
- Coverage 88.13% 88.13% -0.01%
==========================================
Files 311 311
Lines 41941 42001 +60
==========================================
+ Hits 36966 37016 +50
- Misses 4975 4985 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code review by qodo was updated up to the latest commit d502bbb |
Contributor
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.11024558938,
"stddev": 0.16588416941104264,
"median": 4.159666681179999,
"user": 3.9602367999999992,
"system": 3.3454005599999994,
"min": 3.8818989046800003,
"max": 4.309246872679999,
"times": [
4.309246872679999,
4.196670279679999,
3.8818989046800003,
4.12266308268,
4.22399863968,
3.95626458168,
4.22847305168,
3.93448570468,
4.306548030679999,
3.94220674568
]
},
{
"command": "pacquet@main",
"mean": 4.08581337958,
"stddev": 0.1406831661500432,
"median": 4.065699428179999,
"user": 3.9227554999999996,
"system": 3.39089666,
"min": 3.88537039868,
"max": 4.32894250668,
"times": [
4.155887593679999,
3.98755640368,
4.29217659068,
4.01699719268,
4.082269483679999,
4.32894250668,
4.098239286679999,
3.88537039868,
3.96156496668,
4.0491293726799995
]
},
{
"command": "pnpr@HEAD",
"mean": 2.0923994773800003,
"stddev": 0.11287185269685002,
"median": 2.04350159868,
"user": 2.7424879000000004,
"system": 2.8773741599999996,
"min": 1.9605278136800002,
"max": 2.35518538668,
"times": [
2.19929009268,
2.04030398668,
2.04060359768,
2.04223792768,
1.9605278136800002,
2.04476526968,
2.35518538668,
2.10225745568,
2.12162420568,
2.01719903768
]
},
{
"command": "pnpr@main",
"mean": 2.08644769308,
"stddev": 0.12190851346507806,
"median": 2.0362280041800003,
"user": 2.7297076999999996,
"system": 2.8970473599999997,
"min": 1.9539352566800001,
"max": 2.29735101668,
"times": [
2.29735101668,
2.0346393376800003,
2.24061757268,
2.23250039868,
1.9539352566800001,
1.9783661566800002,
2.01823385268,
2.0401019966800003,
2.03091467168,
2.0378166706800003
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.63195399356,
"stddev": 0.02081323031581742,
"median": 0.62718181356,
"user": 0.36630468,
"system": 1.32967546,
"min": 0.5981292665600001,
"max": 0.6667986225600001,
"times": [
0.5981292665600001,
0.6430002305600001,
0.6488608185600001,
0.6273146885600001,
0.6667986225600001,
0.6270489385600001,
0.6188984515600001,
0.6183188865600001,
0.6168196235600001,
0.65435040856
]
},
{
"command": "pacquet@main",
"mean": 0.6638257988600001,
"stddev": 0.06943327226263984,
"median": 0.6395815880600001,
"user": 0.39506628000000005,
"system": 1.3219155599999999,
"min": 0.6093876745600001,
"max": 0.84934560156,
"times": [
0.6406800125600001,
0.6195268305600001,
0.6093876745600001,
0.63839442556,
0.6464929425600001,
0.6641231205600001,
0.6968261465600001,
0.6384831635600001,
0.6349980705600001,
0.84934560156
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7150839210600001,
"stddev": 0.09380983963750163,
"median": 0.6878681090600001,
"user": 0.37953528,
"system": 1.36389966,
"min": 0.6626484235600001,
"max": 0.9761252735600001,
"times": [
0.70826257256,
0.6892563285600001,
0.6864798895600001,
0.66348050156,
0.67776428056,
0.6642165505600001,
0.6626484235600001,
0.7039894925600001,
0.9761252735600001,
0.7186158975600001
]
},
{
"command": "pnpr@main",
"mean": 0.71275431016,
"stddev": 0.037440461011519766,
"median": 0.6991773515600002,
"user": 0.4059740799999999,
"system": 1.3655437600000002,
"min": 0.67411085956,
"max": 0.7847796705600001,
"times": [
0.7847796705600001,
0.7000762975600001,
0.7035489275600001,
0.67411085956,
0.6982784055600001,
0.6971797575600001,
0.6836081625600001,
0.7711804755600001,
0.7288014165600001,
0.6859791285600001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.25492015668,
"stddev": 0.05748769694724176,
"median": 4.247289389880001,
"user": 3.8023267599999997,
"system": 3.28143106,
"min": 4.18568931638,
"max": 4.385194712380001,
"times": [
4.385194712380001,
4.28088266338,
4.20089699638,
4.24390900838,
4.2922881333800005,
4.25066977138,
4.23258900138,
4.20841057438,
4.18568931638,
4.268671389380001
]
},
{
"command": "pacquet@main",
"mean": 4.18353184658,
"stddev": 0.03769593480882368,
"median": 4.18769521188,
"user": 3.75624976,
"system": 3.2697051599999996,
"min": 4.11848411138,
"max": 4.23797747538,
"times": [
4.23797747538,
4.19527037838,
4.14540143538,
4.15862512538,
4.18783148738,
4.23092819038,
4.163371451380001,
4.18755893638,
4.11848411138,
4.20986987438
]
},
{
"command": "pnpr@HEAD",
"mean": 2.10865966908,
"stddev": 0.12011924015100364,
"median": 2.0733314483800003,
"user": 2.5592941599999994,
"system": 2.7725949599999997,
"min": 1.93935436338,
"max": 2.3075348193800003,
"times": [
2.06427796038,
2.2863751433800004,
2.0765519393800003,
2.2053500723800004,
2.02987027038,
2.3075348193800003,
2.0701109573800003,
2.09802388738,
1.93935436338,
2.0091472773800003
]
},
{
"command": "pnpr@main",
"mean": 2.18470637238,
"stddev": 0.1442624470496162,
"median": 2.1464054428800003,
"user": 2.52960066,
"system": 2.7689479599999998,
"min": 2.0078610613800003,
"max": 2.4396897113800002,
"times": [
2.37880285038,
2.4396897113800002,
2.1437702133800003,
2.0463989713800004,
2.2151442073800003,
2.0078610613800003,
2.2799534873800003,
2.04566663638,
2.1407359123800003,
2.1490406723800004
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.3533911659800002,
"stddev": 0.01787899793670438,
"median": 1.34527902668,
"user": 1.3340099599999997,
"system": 1.7041494199999998,
"min": 1.3331922401800003,
"max": 1.3850607001800002,
"times": [
1.3742941381800002,
1.3428420951800002,
1.3331922401800003,
1.3406692581800002,
1.3466189211800002,
1.3386526971800001,
1.34393913218,
1.3552507551800002,
1.3850607001800002,
1.37339172218
]
},
{
"command": "pacquet@main",
"mean": 1.4394403899800003,
"stddev": 0.08044491839977713,
"median": 1.41466105268,
"user": 1.3716620599999998,
"system": 1.80066242,
"min": 1.39268398918,
"max": 1.6659021861800003,
"times": [
1.39268398918,
1.4153442921800001,
1.6659021861800003,
1.41110325318,
1.43340369118,
1.42718243818,
1.40989824418,
1.4018844301800002,
1.4139778131800003,
1.42302356218
]
},
{
"command": "pnpr@HEAD",
"mean": 0.68093222048,
"stddev": 0.044963452224076945,
"median": 0.66624974668,
"user": 0.31431025999999995,
"system": 1.3132282199999998,
"min": 0.6592960171800001,
"max": 0.80789164018,
"times": [
0.66552401218,
0.66697548118,
0.6762967521800001,
0.66422343118,
0.6592960171800001,
0.80789164018,
0.6635412901800001,
0.66119422218,
0.6682643111800001,
0.67611504718
]
},
{
"command": "pnpr@main",
"mean": 0.65486056948,
"stddev": 0.039754929491520284,
"median": 0.64717859968,
"user": 0.33461746000000003,
"system": 1.2735494200000002,
"min": 0.6268782821800001,
"max": 0.7656193611800001,
"times": [
0.63621788418,
0.64691840518,
0.64007308918,
0.6347440201800001,
0.6539285701800001,
0.64743879418,
0.64834367818,
0.6268782821800001,
0.7656193611800001,
0.6484436101800001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.0101464081800007,
"stddev": 0.03780752899032212,
"median": 3.00107045238,
"user": 1.7315262999999999,
"system": 1.9796722999999996,
"min": 2.9773782153800004,
"max": 3.0992848413800003,
"times": [
3.0393461043800003,
2.9773782153800004,
3.0310262263800003,
2.98678708038,
2.9779892413800004,
3.00366148238,
2.9823220853800003,
3.00518938238,
3.0992848413800003,
2.99847942238
]
},
{
"command": "pacquet@main",
"mean": 2.9914275764800005,
"stddev": 0.027286563645303578,
"median": 2.98222837238,
"user": 1.7485281000000001,
"system": 1.9812271,
"min": 2.9665855263800003,
"max": 3.05707779538,
"times": [
3.00331898238,
2.97668820138,
2.9873804443800003,
2.97714382738,
2.98232791138,
2.9681753953800003,
2.9665855263800003,
2.98212883338,
3.05707779538,
3.0134488473800003
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6458308821800001,
"stddev": 0.00979708075321192,
"median": 0.6468328698800001,
"user": 0.3237181,
"system": 1.2924101000000001,
"min": 0.63262468538,
"max": 0.6587839403800001,
"times": [
0.64181307438,
0.6370795843800001,
0.6572449613800001,
0.64775513738,
0.6326421123800001,
0.63262468538,
0.65669958638,
0.6587839403800001,
0.6474914583800001,
0.6461742813800001
]
},
{
"command": "pnpr@main",
"mean": 0.64418983328,
"stddev": 0.00855594103605648,
"median": 0.6410328508800001,
"user": 0.34108150000000004,
"system": 1.2663501000000001,
"min": 0.63295111538,
"max": 0.65680771138,
"times": [
0.63812392738,
0.63295111538,
0.64102767638,
0.64021328838,
0.6482129533800001,
0.6550091933800001,
0.6351050863800001,
0.65680771138,
0.6534093553800001,
0.6410380253800001
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12471 |
| 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 | 4,254.92 ms(+0.87%)Baseline: 4,218.39 ms | 5,062.06 ms (84.06%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 3,010.15 ms(-0.10%)Baseline: 3,013.03 ms | 3,615.63 ms (83.25%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,353.39 ms(+1.67%)Baseline: 1,331.12 ms | 1,597.35 ms (84.73%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,110.25 ms(-1.96%)Baseline: 4,192.59 ms | 5,031.11 ms (81.70%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 631.95 ms(+2.44%)Baseline: 616.90 ms | 740.29 ms (85.37%) |
Contributor
|
| Branch | pr/12471 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,108.66 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 645.83 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 680.93 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,092.40 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 715.08 ms |
zkochan
added a commit
that referenced
this pull request
Jun 17, 2026
Give `PackageManagerBootstrap` a SmartDefault registry of the public npm registry so a directly-constructed `Config` (one not finalized through `Config::current`) never resolves the package manager against an empty registry. Addresses a CodeRabbit review note on #12471.
d502bbb to
5ad2b2d
Compare
Comment on lines
+139
to
+154
| /// Context for resolving the package manager pnpm auto-switches to | ||
| /// (`pnpm` / `@pnpm/exe`). Resolves through TRUSTED registries and | ||
| /// network config only — never the repository-controlled project | ||
| /// `.npmrc` / `pnpm-workspace.yaml` — so a malicious workspace cannot | ||
| /// redirect the package-manager bytes. See GHSA-j2hc-m6cf-6jm8. | ||
| fn for_package_manager(config: &Config) -> Result<Self> { | ||
| let bootstrap = &config.package_manager_bootstrap; | ||
| Self::build( | ||
| config, | ||
| &bootstrap.proxy, | ||
| &bootstrap.tls, | ||
| &bootstrap.tls_by_uri, | ||
| bootstrap.resolved_registries(), | ||
| Arc::clone(&bootstrap.auth_headers), | ||
| ) | ||
| } |
There was a problem hiding this comment.
1. Bootstrap reads repo cache 🐞 Bug ⛨ Security
EnvInstallerContext::for_package_manager switches registries/auth/proxy/TLS to trusted sources but still builds the NpmResolver using Config.offline/prefer_offline and Config.cache_dir, which pnpm-workspace.yaml can set. A malicious repo can set preferOffline/offline and point cacheDir at a repo path containing a crafted metadata mirror so package-manager bootstrap resolves pnpm/@pnpm/exe from attacker-controlled metadata and writes poisoned resolution/integrity into pnpm-lock.yaml.
Agent Prompt
## Issue description
`EnvInstallerContext::for_package_manager` uses trusted bootstrap registries/proxy/TLS/auth, but the resolver it builds still inherits `offline`, `prefer_offline`, and `cache_dir` from `Config`, which are repository-controlled via `pnpm-workspace.yaml`. Because the npm resolver reads the on-disk mirror first (and can return without any network fetch) when `offline`/`prefer_offline` is enabled, a malicious repo can supply crafted mirror metadata and poison `packageManagerDependencies` integrity/resolution written into `pnpm-lock.yaml`.
## Issue Context
- `pnpm-workspace.yaml` can set `offline`, `prefer_offline`, and `cache_dir`, and these are applied onto `Config`.
- The npm resolver consults the on-disk mirror when `offline` or `prefer_offline` is set, and in `offline` mode it will return the mirror result without a network request.
- The package-manager bootstrap path (`sync_package_manager_dependencies`) uses this resolver to populate `packageManagerDependencies` in the env lockfile.
## Fix Focus Areas
- pacquet/crates/cli/src/config_deps.rs[139-214]
- pacquet/crates/config/src/workspace_yaml.rs[121-129]
- pacquet/crates/config/src/workspace_yaml.rs[726-742]
- pacquet/crates/config/src/workspace_yaml.rs[917-919]
- pacquet/crates/resolving-npm-resolver/src/pick_package.rs[496-533]
## What to change
1. Ensure the package-manager bootstrap resolver does **not** accept repository-controlled cache/offline settings.
- Option A (minimal, localized): In `EnvInstallerContext::for_package_manager`, override resolver config to a safe trusted posture (e.g. `offline=false`, `prefer_offline=false`, and use a non-workspace cache dir).
- Option B (more complete/parity-friendly): Extend `PackageManagerBootstrap` to carry trusted-only `offline`, `prefer_offline`, and `cache_dir` (built from trusted sources only) and have `for_package_manager` use those fields.
2. Add a regression test demonstrating the attack:
- A temp project with `pnpm-workspace.yaml` setting `preferOffline: true` and `cacheDir: ./.cache`.
- Pre-populate the expected mirror path for `pnpm` under that cache dir with crafted metadata.
- Assert bootstrap resolution incorrectly uses the mirror today; after the fix, it should fetch from the trusted registry (or at least ignore the repo cache).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Good catch, and it's a real concern — but it's deliberately out of scope for this PR, for parity with pnpm.
pnpm's current main has the same exposure: switchCliVersion() builds its store controller as createStoreController({ ...config, ...context, ...packageManagerConfig }), and packageManagerConfig (getPackageManagerBootstrapConfig) overrides only the registries + network config — it does not override cacheDir/storeDir/offline/preferOffline. So pnpm's package-manager bootstrap reads those same (workspace-influenceable) values. This PR mirrors that exactly: it closes the registry/network trust boundary of GHSA-j2hc-m6cf-6jm8 (the #12296 scope) and nothing more.
The cache/store trust boundary is a separate finding in the same campaign — upstream addresses it with dedicated trustedCacheDir/trustedStoreDir config that has not landed in main yet. Per the pacquet cardinal rule (match pnpm; don't add behavior that isn't upstream yet), hardening cache_dir/offline/prefer_offline for bootstrap should land in pnpm first and then be mirrored here, so the two stacks stay in lockstep. Doing it unilaterally in pacquet now would make pacquet diverge from pnpm.
Tracking it as follow-up rather than expanding this PR's scope.
Written by an agent (Claude Code, claude-opus-4-8).
|
Code review by qodo was updated up to the latest commit 5ad2b2d |
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pacquet/crates/config/src/lib.rs (1)
1863-1892:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t drop trusted registry settings outside
.npmrc.
package_manager_bootstrapis rebuilt fromConfig::default()plus folded.npmrcauth sources, before the trusted globalconfig.yamllayer is applied. That lets a user-levelconfig.yamlregistry— or any caller/CLI-populatedself.registry— drive normal installs while package-manager auto-switch still falls back to npmjs. In private-mirror setups,pnpm/@pnpm/exebootstrap bypasses the user’s trusted registry; thread trusted global/pre-populated registry values into the bootstrap before resolving package-manager dependencies. Upstream describes this path as using trusted config sources rather than project/workspace settings. (github.com)As per coding guidelines, “Keep pnpm (TypeScript) and pacquet (Rust) behavior in sync” and review registry/network/auth trust-boundary issues first.
Also applies to: 2094-2119
🤖 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/config/src/lib.rs` around lines 1863 - 1892, The issue is that when building package_manager_bootstrap, only the trusted auth information from trusted_auth is being passed to build_package_manager_bootstrap::<Sys>(), but trusted registry settings and network configuration from global config.yaml or pre-populated self.registry values are being dropped. You need to extract and preserve the registry and network settings from the trusted sources (env_scoped_source, auth_ini_source, and user_source) and thread them into the bootstrap construction along with trusted_auth to ensure the package-manager auto-switch respects the user's trusted registry configuration rather than falling back to the default npmjs registry.Source: Coding guidelines
pacquet/crates/cli/src/config_deps.rs (1)
186-201:⚠️ Potential issue | 🔴 CriticalThe resolver for package-manager bootstrap still uses repository-controlled cache/offline settings, bypassing the GHSA-j2hc-m6cf-6jm8 fix.
for_package_manager()correctly routes through trustedbootstrapregistries and auth to prevent a malicious workspace from redirecting package-manager bytes. However, the NpmResolver it builds still receivesconfig.cache_dir,config.offline, andconfig.prefer_offline— all three of which are populated frompnpm-workspace.yamlviaWorkspaceSettings::apply_to().A malicious workspace can set
offline: trueandcache_dir: ./attacker-cachein itspnpm-workspace.yaml, forcing the resolver to serve poisoned packument metadata from a workspace-controlled directory and skip network validation. This bypasses the trust boundary intended by the GHSA-j2hc-m6cf-6jm8 fix.Remove the three repository-controlled fields from the package-manager bootstrap resolver, or surface trusted versions from
PackageManagerBootstrap.🤖 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/cli/src/config_deps.rs` around lines 186 - 201, The NpmResolver being initialized for package-manager bootstrap is receiving three repository-controlled settings from config (cache_dir, offline, and prefer_offline) that originate from pnpm-workspace.yaml, allowing a malicious workspace to inject poisoned packument metadata and bypass the trust boundary. Remove these three fields from the NpmResolver struct initialization where cache_dir is set to Some(config.cache_dir.clone()), offline is set to config.offline, and prefer_offline is set to config.prefer_offline, either by using safe default values or by replacing them with trusted values from PackageManagerBootstrap instead of the user-controlled config object.Source: Coding guidelines
🧹 Nitpick comments (1)
pacquet/crates/config/src/tests.rs (1)
1665-1766: ⚡ Quick winAdd auth/proxy/TLS isolation assertions for the bootstrap surface.
These tests lock registry selection, but
PackageManagerBootstrapalso carriesauth_headers, proxy, TLS, and per-registry TLS. Add a small fixture where project.npmrcsets attacker credentials/proxy/TLS and user.npmrcsets trusted ones, then assertpackage_manager_bootstrapcontains only the trusted values. That keeps the security regression suite aligned with the full changed surface.As per coding guidelines, use real fixtures such as
tempfile::TempDirfor tests and review registry/network/auth trust-boundary issues first.🤖 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/config/src/tests.rs` around lines 1665 - 1766, Add a new test function following the existing pattern of the registry isolation tests (like package_manager_bootstrap_ignores_project_npmrc_registry) to verify that auth_headers, proxy, and TLS settings from a project .npmrc are not included in the package_manager_bootstrap configuration. Create a fixture with a user-level .npmrc containing trusted auth and proxy settings, and a project .npmrc containing attacker-controlled credentials and proxy settings. Use tempdir() to create real temporary files for both configs, then call load_with_project_and_user or Config::current to load the configuration and assert that package_manager_bootstrap contains only the trusted user-level values (auth_headers, proxy, and TLS settings) while the main config contains the project-controlled values. This extends the security regression test suite to cover the full trust boundary surface beyond just registry selection.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@pacquet/crates/cli/src/config_deps.rs`:
- Around line 186-201: The NpmResolver being initialized for package-manager
bootstrap is receiving three repository-controlled settings from config
(cache_dir, offline, and prefer_offline) that originate from
pnpm-workspace.yaml, allowing a malicious workspace to inject poisoned packument
metadata and bypass the trust boundary. Remove these three fields from the
NpmResolver struct initialization where cache_dir is set to
Some(config.cache_dir.clone()), offline is set to config.offline, and
prefer_offline is set to config.prefer_offline, either by using safe default
values or by replacing them with trusted values from PackageManagerBootstrap
instead of the user-controlled config object.
In `@pacquet/crates/config/src/lib.rs`:
- Around line 1863-1892: The issue is that when building
package_manager_bootstrap, only the trusted auth information from trusted_auth
is being passed to build_package_manager_bootstrap::<Sys>(), but trusted
registry settings and network configuration from global config.yaml or
pre-populated self.registry values are being dropped. You need to extract and
preserve the registry and network settings from the trusted sources
(env_scoped_source, auth_ini_source, and user_source) and thread them into the
bootstrap construction along with trusted_auth to ensure the package-manager
auto-switch respects the user's trusted registry configuration rather than
falling back to the default npmjs registry.
---
Nitpick comments:
In `@pacquet/crates/config/src/tests.rs`:
- Around line 1665-1766: Add a new test function following the existing pattern
of the registry isolation tests (like
package_manager_bootstrap_ignores_project_npmrc_registry) to verify that
auth_headers, proxy, and TLS settings from a project .npmrc are not included in
the package_manager_bootstrap configuration. Create a fixture with a user-level
.npmrc containing trusted auth and proxy settings, and a project .npmrc
containing attacker-controlled credentials and proxy settings. Use tempdir() to
create real temporary files for both configs, then call
load_with_project_and_user or Config::current to load the configuration and
assert that package_manager_bootstrap contains only the trusted user-level
values (auth_headers, proxy, and TLS settings) while the main config contains
the project-controlled values. This extends the security regression test suite
to cover the full trust boundary surface beyond just registry selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 888c0cf0-97bc-400b-beb6-b839c9d32bcb
📒 Files selected for processing (5)
pacquet/crates/cli/src/config_deps.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/npmrc_auth.rspacquet/crates/config/src/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rs
…tries Port the GHSA-j2hc-m6cf-6jm8 fix (#12296) to pacquet. When pnpm auto-switches to the version requested by `packageManager` / `devEngines.packageManager`, the bootstrap (`pnpm` / `@pnpm/exe`) must be resolved through trusted registries only. Pacquet was resolving it through `config.resolved_registries()`, which a malicious repository controls via the workspace `.npmrc` or `pnpm-workspace.yaml` `registries:` block. Add `Config::package_manager_bootstrap`, built in `Config::current()` from a trusted-only fold of the URL-scoped env, `auth.ini`, and user `.npmrc` sources (the project `.npmrc` is excluded), reusing the existing registry/proxy/TLS/auth application logic. It defaults to the public npm registry, and `PNPM_CONFIG_REGISTRY` still overrides the default because it is user-controlled. `EnvInstallerContext::for_package_manager` routes only the package-manager bootstrap path (`sync_package_manager_dependencies`) through this trusted config; project `configDependencies` resolution keeps the project registries, matching the narrow scope of the upstream TypeScript fix.
5ad2b2d to
14dde1d
Compare
|
Code review by qodo was updated up to the latest commit 14dde1d |
This was referenced Jun 21, 2026
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
2 participants
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.
What
Ports the GHSA-j2hc-m6cf-6jm8 fix (pnpm/pnpm#12296) to pacquet, keeping the two stacks in sync per the parity rule in
AGENTS.md.Why
When pnpm auto-switches to the version requested by
packageManager/devEngines.packageManager, the package-manager bytes (pnpm/@pnpm/exe) must be resolved through trusted registries only. The TypeScript CLI was fixed in #12296 (config.packageManagerRegistries/packageManagerNetworkConfig), but pacquet was never ported.Pacquet's
sync_package_manager_dependenciesbuilt its resolver fromconfig.resolved_registries()— the project/workspace registries, which a malicious repository controls via the workspace.npmrcor apnpm-workspace.yamlregistries:block. That lets a repository point package-manager bootstrap resolution at an attacker-controlled registry.Pacquet only resolves and writes integrity metadata into
pnpm-lock.yamlhere (it does not download/execute the bytes itself), so its standalone impact is lockfile poisoning rather than direct RCE — but the trust boundary must match pnpm regardless.How
Config::package_manager_bootstrap(PackageManagerBootstrap), built inConfig::current()from a trusted-only fold of the URL-scoped env,auth.ini, and user.npmrcsources. The project.npmrcandpnpm-workspace.yamlregistries:are excluded. It reuses the existing registry/proxy/TLS/auth application logic so the bootstrap cascade stays identical to the project cascade minus the repository-controlled sources.PNPM_CONFIG_REGISTRYstill overrides the bootstrap default because it is user-controlled (mirrors pnpm's env/CLIregistryhandling).EnvInstallerContext::for_package_manager, used only by the package-manager bootstrap path (sync_package_manager_dependencies). ProjectconfigDependenciesresolution keeps the project registries viaEnvInstallerContext::new, matching the narrow scope of the upstream TypeScript fix.Tests
New regression tests in
config/src/tests.rs:package_manager_bootstrap_ignores_project_npmrc_registrypackage_manager_bootstrap_ignores_workspace_yaml_registriespackage_manager_bootstrap_defaults_to_npm_registrypackage_manager_bootstrap_honors_env_registryEach asserts normal installs still follow the project/workspace/env registry while package-manager bootstrap stays on the trusted (or npm-default) registry.
Verified locally:
cargo fmt --check, workspacecargo clippy/dylint (-D warnings),cargo doc -D warnings, andcargo testforpacquet-config(291) /pacquet-env-installer(12) /pacquet-clicli_args (72) all pass.Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
PNPM_CONFIG_REGISTRYupdates both the effective runtime registry and the bootstrap default registry.PNPM_CONFIG_REGISTRYoverride behavior.Suggested squash commit message body