install: honor .npmrc //host/:_authToken= for tarballs on a different host than the registry - #34329
install: honor .npmrc //host/:_authToken= for tarballs on a different host than the registry#34329robobun wants to merge 5 commits into
Conversation
… host than the registry 1.4 added a same-origin guard on the registry Authorization header for tarball downloads: a manifest's dist.tarball can point anywhere, so a malicious registry could otherwise exfiltrate the scope token to an attacker-controlled host. That guard also breaks legitimate setups (Artifactory/Nexus with a separate tarball/CDN host) with no config escape hatch, because .npmrc //cdn-host/:_authToken= lines that do not match a configured registry were parsed and then dropped. npm's own model (npm-registry-fetch getAuth) is: look up auth by the URL being fetched. If the tarball host differs from the registry host, npm sends no Authorization unless the user has an explicit //tarball-host/ nerf-dart entry, in which case it sends that entry's credential. Keep .npmrc //host/:*= entries that match no registry and consult them in for_tarball when the tarball origin differs from the scope registry origin. The same-origin guard is unchanged: a registry still cannot forward scope credentials to a host the user has not configured.
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 5:47 AM PT - Jul 16th, 2026
❌ @robobun, your commit d45c404 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34329That installs a local version of the PR into your bun-34329 --bun |
|
Reproduced with a registry on one port serving manifests and an authenticated CDN on another port serving tarballs; with a All three new tests fail with CI: the diff's tests pass on every lane that ran. Build #73846 is red on |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate/related-issue bot comments: #33869 is complementary, not a duplicate. It rewrites Related issues: of the four flagged, only #30513 is directly fixed here (it is specifically about tarball URLs whose path diverges from the scoped registry path; the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…arball test - tarball_url_auth_for: match a portless .npmrc key against a tarball URL whose port is the scheme default (npm builds the key from WHATWG URL.host, which strips default ports) - rename the Basic-auth test to reflect what it exercises (username+_password, not _auth) - add a test for direct-URL tarball dependencies (NoAuthorization path) with a matching .npmrc //host/ entry - doc comment on Authorization::NoAuthorization clarifying it gates only the registry-scope credential
There was a problem hiding this comment.
Beyond the inline nit: the three fixes in e89bba7 look right — default-port normalization now mirrors the same-origin check, the Basic-auth test is renamed, and the direct-URL tarball path is covered with a test + doc comment on NoAuthorization. Also confirmed the security model is unchanged: the registry-scope credential still only crosses the same-origin gate, and tarball_url_auth_for only ever returns a credential the user explicitly keyed to that host in .npmrc. Leaving final sign-off to a human given this is credential-routing code and overlaps with #33869.
Extended reasoning...
The author addressed all three of my prior inline nits in e89bba7; I verified each fix in the current diff. This run surfaced one additional narrow nit (stale tarball_url_auth entry across multi-file .npmrc loads), which is posted inline. I checked that the new tarball_url_auth_for path cannot leak a credential to an unconfigured host — every returned scope comes from an entry the user wrote in .npmrc for that exact host (with default-port normalization), and the existing same-origin guard on the registry-scope token is untouched. Not approving because this is auth/credential routing in the package manager, and there is a design-level overlap with #33869 that a maintainer should reconcile.
The configs vec accumulates across .npmrc files and is re-iterated in full each call; clearing tarball_url_auth before the loop ensures an entry that was unmatched in the global file is not left stale when the project file introduces a scoped registry at the same host.
There was a problem hiding this comment.
All four nits from earlier review rounds are addressed (default-port normalization, test rename, direct-URL tarball test + NoAuthorization doc comment, and the tarball_url_auth.clear() before the configs re-iteration). No further issues found. Deferring to a human because this changes credential-attachment logic in the package manager — worth a second pair of eyes on the host/path-prefix matching and the NoAuthorization semantics change, and on how this should land relative to #33869 (which will conflict in src/ini/lib.rs).
What was reviewed:
tarball_url_auth_forhost+port match and path-segment-boundary prefix check — no substring/partial-segment hazard; longest-prefix wins.- Same-origin guard is unchanged; the
unrelatedAuth: [null]assertion covers the no-leak-to-unconfigured-host case. apply_config_optextraction is a byte-identical dedup of the two existing match blocks..clear()before the configs loop keeps multi-.npmrc ordering consistent with the scoped-registry re-iteration.
Extended reasoning...
Overview
The PR adds an escape hatch to the 1.4 same-origin guard for tarball downloads: .npmrc //host/path/:*= entries that don't match any configured registry are now retained in a new tarball_url_auth list, and NetworkTask::for_tarball consults it when the tarball origin differs from the registry origin. Touches src/ini/lib.rs (loader + new apply_config_opt helper deduped from two identical match blocks), src/install/NetworkTask.rs (new tarball_url_auth_for lookup + rework of the send_auth gate into an auth_scope: Option<&Scope>), PackageManagerOptions.rs / schema.rs (plumbing the new field), plus three new tests and a docs section.
Security risks
This is credential-handling code. The core question is whether a credential can be sent to a host the user did not explicitly name. I checked:
- The same-origin guard on the registry-scope credential is unchanged — a registry still cannot exfiltrate the scope token to an arbitrary
dist.tarballhost. - The fallback only sends a credential to a host that appears verbatim as a
.npmrc//host/key the user wrote. Host comparison is exact (including port, with one-sided default-port normalization on the tarball side); path is a segment-boundary prefix (guarded by the== b'/'check), so//host/foo/does not match/foobar/.... - The first new test asserts
unrelatedAuth: [null]for a third host with no.npmrcentry, which is the negative contract. Authorization::NoAuthorizationnow consultstarball_url_authtoo — a semantics change, but again only for hosts the user explicitly configured; matchesnpm-registry-fetchbehavior for direct-URL tarballs and now has both a doc comment and a dedicated test.
I did not find a leak vector, but this is exactly the surface where a subtle matching bug becomes a credential disclosure, so human review is appropriate.
Level of scrutiny
High. Package-manager auth-header attachment is production-critical and security-sensitive. The implementation is well-scoped, thoroughly commented, and mirrors npm's documented behavior, but the guidelines say not to approve auth/credential changes without human review.
Other factors
- Four prior nits from earlier review rounds were all addressed in e89bba7 and 5935d02; all threads are resolved.
- There is an overlapping open PR (#33869) that rewrites
.npmrccredential resolution more broadly and will conflict insrc/ini/lib.rs; a maintainer should decide sequencing. - Test coverage is good: cross-origin CDN with Bearer, username+
_password→ Basic, direct-URL tarball, and the existing same-origin/cross-origin negative test still applies. Tests use in-process servers withport: 0, drain pipes concurrently, and assert exact header values.
There was a problem hiding this comment.
All four earlier nits are addressed (default-port normalization, test rename, direct-URL tarball test + NoAuthorization doc comment, and the .clear() for multi-file .npmrc staleness) and I found nothing new on this pass. Deferring to a human because this is credential-forwarding logic on the install hot path, and the overlap with #33869 (which rewrites the same .npmrc credential resolution in src/ini/lib.rs) is a landing-order call a maintainer should make.
What was reviewed:
for_tarballauth-scope selection — same-origin gate is unchanged; cross-origin fallback only consults user-written//host/:*=entries, and the negative test asserts an unconfigured third host receives noAuthorization.tarball_url_auth_forhost/port match and path-prefix logic, including the segment-boundary check (tarball_path[entry_path.len()] == b'/') so/foodoes not match/foobar..npmrcloader: theapply_config_optextraction is byte-identical to the two removed match blocks;tarball_url_authis now cleared and rebuilt each pass so project-over-global precedence holds.
Extended reasoning...
Overview
Adds an escape hatch for the 1.4 same-origin tarball auth guard: .npmrc //host/path/:*= entries that match neither the default nor any scoped registry are retained in a new BunInstall::tarball_url_auth list, resolved into Options::tarball_url_auth via Scope::from_api, and consulted by NetworkTask::for_tarball when the tarball URL is cross-origin from the registry (or when Authorization::NoAuthorization is passed for direct-URL tarball deps). Touches src/ini/lib.rs (loader + apply_config_opt dedup), src/install/NetworkTask.rs (tarball_url_auth_for + for_tarball), PackageManagerOptions.rs / schema.rs (new field), three tests in bun-install.test.ts, and a docs section.
Security risks
The relevant risk is credential leakage to a registry-controlled host. The design is sound: the same-origin guard on the registry-scope credential is unchanged, and the fallback only ever returns a Scope the user explicitly keyed by host in .npmrc. A malicious registry pointing dist.tarball at an attacker host still receives nothing unless the user themselves wrote a //attacker/:_authToken= line. The first new test asserts exactly this (unrelatedAuth: [null]). The NoAuthorization branch now sending a matching //host/ credential to a direct-URL tarball is a strict improvement (matches npm) and is covered by the third test. I did not find a way to route a credential to a host the user did not name.
Level of scrutiny
High — this is Authorization-header attachment in bun install's network path, squarely in the "security-sensitive code (auth)" bucket where the guidelines say not to auto-approve. It also intersects #33869, which rewrites credential resolution more broadly; whichever lands second needs a rebase in src/ini/lib.rs, and a maintainer should decide sequencing.
Other factors
I reviewed this PR twice previously and left four nits, all now addressed and marked resolved. The bug-hunting system found nothing on this run. The apply_config_opt refactor is a pure extraction of two identical match blocks. Tests are hermetic (local Bun.serve on port: 0), assert exact header values, include the negative case, and cover both AllowAuthorization and NoAuthorization paths plus Bearer and Basic. Net: I think this is correct, but auth-header changes plus the #33869 coordination warrant a human sign-off.
|
Closing in favor of #38800, which resolves Checked against a build of #38800's branch: the two cross-host tarball tests from this PR pass on it unmodified (both still fail on current main, which has neither change), and a basic-auth variant of the cross-host case has been added to #38800's test block. The one thing here that #38800 does not carry forward is sending |
1 participant
Problem
1.4 added a same-origin guard in
NetworkTask::for_tarball: the registryAuthorizationheader is only attached when the tarball URL origin matches the configured registry origin. That guard is correct (a malicious registry could otherwise pointdist.tarballat an attacker host and harvest the scope token), but it breaks legitimate setups where the registry serves manifests from one host and tarballs from a separate authenticated host (Artifactory / Nexus / self-hosted with a CDN). There was no config escape hatch: a.npmrc//cdn-host/:_authToken=line is parsed but then dropped in the.npmrcloader because it matches neither the default registry nor any scoped registry.What npm does
npm-registry-fetchresolves auth by the URL being fetched, not by the package scope.getAuth(uri)nerf-darts the fetch URL (//host/path, walking path segments up to//host/) and uses whatever_authToken/_auth/username:_passwordis configured for that key. If nothing is configured for the tarball host, andsameHost(tarball, registry)is false, npm sends noAuthorizationat all (and on a 4xx warns the user to add a//<tarball-host>/:_authTokenentry). So npm already has the same cross-origin protection, but honors an explicit host-keyed.npmrcentry.Fix
.npmrc//host/path/:*=entries that match neither the default nor any scoped registry are kept inBunInstall::tarball_url_auth(and resolved intoOptions::tarball_url_authviaScope::from_api, which handles$ENV_VARexpansion and username/password -> Basic).NetworkTask::for_tarballkeeps the existing same-origin gate; on a miss it looks up the tarball URL against those entries by host (including port) and path prefix, sending that entry's credential instead.Tests
should send .npmrc //host/:_authToken= to a tarball host that is not the registry: registry on port A, authenticated CDN on port B, an unconfigured third host on port C. CDN receivesBearer cdn-token; the third host receives noAuthorization.should send .npmrc //host/:_auth= Basic auth to a tarball host that is not the registry:username+_passwordentries for the CDN host produce aBasicheader.should send .npmrc _authToken on same-origin tarball download and withhold it cross-origintest still passes (cross-origin host with no.npmrcentry receives no auth).Both new tests fail with
USE_SYSTEM_BUN=1(cdnAuth: [null],401) and pass withbun bd test.Docs: added a short section to
docs/pm/npmrc.mdxexplaining the tarball-host escape hatch.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts
Fixes #30513