install: resolve .npmrc //host/ credentials by request URL - #38800
install: resolve .npmrc //host/ credentials by request URL#38800robobun wants to merge 3 commits into
Conversation
|
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 (10)
Comment |
|
Status: ready for review and mergeable. Rebased onto main twice (on top of #38796, then #38183); current head 8816afc. The previous head b36bf6a passed CI on all 179 jobs (build 97798; the only annotations were batch flakes that passed on retry in unrelated files); the re-rebase only re-merged two added helpers in npm.rs. On 8816afc itself (build 98409) all 177 jobs that ran passed; the build was then canceled by a maintainer with only the two darwin 14 aarch64 shards still waiting for an agent, the same lane that took hours to start on every build today. Reproduced with Scope note: the first push also carried fixes for credentials embedded in Tests: the new block in |
f3ea94a to
f4fd5de
Compare
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit 8816afc has some failures in 🧪 To try this PR locally: bunx bun-pr 38800That installs a local version of the PR into your bun-38800 --bun |
f4fd5de to
727d5a2
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The 3cc622f follow-up correctly addresses the earlier note: NetworkTask.authorization is stored at enqueue time, RequestKind::Tarball carries it to missing_credentials_note, and the NoAuthorization arm returns empty so github:/URL-tarball 401s no longer get an .npmrc suggestion that wouldn't apply; the new test pins that. I re-reviewed and found no further issues. Because this changes which Authorization header is sent to which host, a human pass over the credential-routing surface (UrlAuth::matches/find, tarball_credentials, fill_credentials_from_url_auth) would still be worthwhile.
What was reviewed: the cross-origin guard is preserved (tarball_credentials returns the scope's creds only on is_same_origin, otherwise only what .npmrc names for that exact host); UrlAuth::matches requires host + effective-port equality and a directory-boundary path prefix (the /no vs /no-deps test covers the string-prefix trap); entries without a sendable header are dropped in from_api so they can't shadow a shallower key; write_init seeds the new authorization field so pooled slots don't carry a stale value.
Extended reasoning...
Overview
Adds npm-style by-URL resolution of .npmrc //host/path/ credential lines. bun_ini now records every such line into BunInstall.url_auth; Options::load converts them to npm::registry::UrlAuth and, after all config sources are applied, back-fills any credential-less scope from the entry matching its registry URL. NetworkTask::for_tarball now consults Options::tarball_credentials (scope creds if same-origin, else the url_auth entry for the tarball URL, else nothing). A 401/403 hint is appended to the two GET <url> - <status> sites, gated on the request having been allowed to carry credentials at all. 13 new subprocess tests assert the exact Authorization header each mock server receives.
Follow-up on prior review
My earlier finding was that the 401 hint would suggest an .npmrc line for github:/URL-tarball dependencies even though for_tarball short-circuits to no credentials for Authorization::NoAuthorization. Commit 3cc622f threads the enqueue-time Authorization through NetworkTask.authorization → RequestKind::Tarball(auth) → missing_credentials_note, whose NoAuthorization arm now returns empty. write_init seeds the field to NoAuthorization so a reused pool slot cannot leak a stale AllowAuthorization. The added test ("a 401 for a tarball dependency given by URL is reported without a suggestion") asserts both that no header was sent and that stderr contains no .npmrc mention. This fully resolves the concern.
Security risks
This is the security-sensitive part of the PR: it changes when an Authorization header is attached to an outbound request. I checked: (1) the same-origin guard on scope credentials is preserved — tarball_credentials returns the scope only when is_same_origin(tarball, scope.url), and is_same_origin compares protocol/hostname case-insensitively plus effective port, which is the correct normalization of the previous inline check; (2) the fallback only ever sends credentials the user explicitly keyed to that host in .npmrc, so a hostile registry pointing dist.tarball elsewhere still cannot exfiltrate anything the user didn't configure for that destination; (3) UrlAuth::matches enforces host, port (default-port aware), and directory-boundary path prefix — the /no ≠ /no-deps/... test covers the substring-prefix pitfall; (4) fill_credentials_from_url_auth only fills scopes that have no credentials, so --token / BUN_CONFIG_TOKEN and bunfig-set credentials are not overridden; (5) entries with no sendable header (email alone, empty hostname, unparseable port) are dropped in UrlAuth::from_api so they cannot shadow a shallower usable key via the deepest-path rule.
Level of scrutiny
High — this is credential routing in the package manager. The change is additive and well-tested (each test asserts the exact header the server received, plus negative cases where no header must be sent), and the design mirrors npm's nerf-dart semantics. Still, per the approval guidelines, auth-header decisions are exactly the class of change that should get a maintainer's eyes rather than a bot approval, particularly the interaction with the three sibling PRs (#38796, #38776, #33869) the description says this composes with.
Other factors
Test coverage is thorough: --registry, both env vars, basic auth from username/_password, cross-host tarballs, deepest-path precedence, string-prefix-not-directory rejection, GitLab-style divergent paths (#30513), moved-registry frozen-lockfile, and both hint-present and hint-absent cases. The overlay_bunfig_install destructure was updated for the new url_auth field. The refactor of the same-origin check in for_tarball from an inline block to is_same_origin + tarball_credentials is behavior-preserving modulo the case-insensitive protocol/hostname comparison, which is correct for origin equality.
There was a problem hiding this comment.
The 3cc622f fix addresses the earlier note — NetworkTask now stores the Authorization mode and missing_credentials_note returns empty for Tarball(NoAuthorization), with a regression test pinning it. No further issues found this run. Because this changes how credentials are selected and sent to registry/tarball hosts, a maintainer look at the security model (the tarball_credentials same-origin guard and UrlAuth::matches path-prefix semantics) is still worthwhile.
What was reviewed:
is_same_originvs the removed inline check infor_tarball— now case-insensitive on protocol/hostname (spec-correct), still compares effective port; scope credentials still cannot cross origins.UrlAuth::matchespath-prefix boundary (/nonot matching/no-deps/) and deepest-key precedence — both covered by tests.fill_credentials_from_url_authruns after--registry/env/--tokenare applied and skips scopes that already have credentials, so it does not override an explicit token.- The
RequestKind::Tarball(NoAuthorization)arm and the URL-dependency 401 test cover the earlier finding.
Extended reasoning...
Overview
Adds npm-style by-request-URL resolution of .npmrc //host/path/ credential lines. The .npmrc loader (src/ini/lib.rs) now records every credential line into BunInstall.url_auth alongside the existing per-registry attachment; Options::load converts them to npm::registry::UrlAuth entries and, after all config sources are applied, backfills any credential-less scope from the entry matching its registry URL. NetworkTask::for_tarball replaces its inline same-origin check with Options::tarball_credentials, which returns the scope's credentials on same-origin and otherwise the deepest matching UrlAuth entry. A 401/403 hint is added to the two GET <url> - <status> sites in runTasks.rs, gated on the request's stored Authorization mode so github:/URL-tarball dependencies (which never carry registry credentials) don't get a misleading suggestion. 13 new tests in npmrc.test.ts exercise --registry, NPM_CONFIG_REGISTRY/BUN_CONFIG_REGISTRY, separate tarball hosts, path-prefix precedence, the GitLab #30513 shape, moved-registry lockfiles, and both the presence and absence of the 401 note.
Security risks
This is credential-routing code. The primary risk is sending a token somewhere the user didn't intend. The existing cross-origin guard (a hostile registry cannot direct a scope's credentials to an arbitrary dist.tarball host) is preserved: tarball_credentials still requires is_same_origin before returning scope credentials, and the fallback UrlAuth::find only ever returns credentials the user explicitly keyed to that host/path in .npmrc — the same model npm uses. is_same_origin is now case-insensitive on protocol/hostname (previously byte-equal); hostnames are case-insensitive per RFC so this is a correctness relaxation, not a weakening. UrlAuth::matches checks a directory-boundary prefix (next byte must be / or end), and entries with no sendable credential (e.g. email alone) are dropped so they can't shadow a shallower key. I did not find a path where credentials cross an origin the user didn't configure them for.
Level of scrutiny
High. This is a non-trivial behavioral change to authentication in bun install — it adds new code paths where an Authorization header is emitted, reworks the tarball same-origin gate into a shared helper, and encodes npm's nerf-dart matching semantics (host/port equality, scheme-default port for portless keys, deepest-path-wins). A maintainer should confirm the matching semantics are the ones Bun wants and that the case-insensitive origin comparison is intentional.
Other factors
The one issue raised on the previous revision (the 401 hint suggesting an .npmrc line that NoAuthorization tarballs would ignore) was fixed by threading the Authorization mode through NetworkTask → RequestKind::Tarball, and a regression test was added. Test coverage is thorough — each shape is asserted against a Bun.serve registry that 401s unless the exact expected header arrives, and the negative cases (rejected token → no hint; wrong-path key → no auth sent) are covered. CI is reported green on 177/179 lanes with the two remaining being infrastructure (darwin agent queue). Given the security-sensitive surface and the number of interacting config sources, this warrants a human sign-off rather than automated approval.
|
@robobun fix conflicts |
f15d359 to
b36bf6a
Compare
|
Rebased onto main (on top of #38796) and force-pushed as b36bf6a. The only textual conflicts were the docs sentence and the end of npmrc.test.ts, where the IPv6 block from #38828 landed; both sides are kept. The Rust side merged without conflicts and stays additive around the new env var / --registry code. On the rebased tree the merged npmrc.test.ts (54 tests), config-precedence and redacted-config-logs pass with the debug build, and CI build 97798 on this head passed on all 179 jobs. |
The //host/path/:_authToken= lines of .npmrc were only ever attached to registries declared in a config file, at the time the file was read. A registry that arrives later (--registry, BUN_CONFIG_REGISTRY, NPM_CONFIG_REGISTRY) never picked up its line, and a tarball served from somewhere other than its registry's origin (a separate download host, a project-level path next to an instance-level registry path, a lockfile recorded against a registry that has since moved) was sent without credentials even when that URL had a line of its own. All of these failed with a bare "GET ... - 401". Keep every //host/path/ line past .npmrc loading (BunInstall.url_auth) and resolve them by URL the way npm does (same host and port, key path is the request path or a parent of it, deepest key wins): once every registry source has been applied, a scope still without credentials takes the line matching its registry URL, and a tarball whose origin is not its scope's origin takes the line matching the tarball URL. A scope's own credentials are still never sent to another origin. A 401/403 for a request that went out without credentials now names the .npmrc line that would have authenticated it.
…egistry credentials github: and URL tarball dependencies are fetched without registry credentials, so suggesting a //host/ line for them describes a fix the code would not apply. Remember what the request was allowed to send on the NetworkTask and only suggest a line when one would have been used.
b36bf6a to
8816afc
Compare
|
main moved again right after that (#38183 added Scope::set_url next to the two helpers this PR adds), so rebased once more; now at 8816afc and GitHub reports it mergeable. #38183 helps this change: the scope URL the end-of-load lookup and the tarball same-origin check read is now the canonical WHATWG form. Re-verified on this tree with the debug build: npmrc.test.ts (54), the Registry URLs / spellings block of bun-install.test.ts that #38183 added (tarball Authorization on the normalized origin still goes through tarball_credentials), config-precedence, redacted-config-logs, and bun-audit; CI is running on the new head. |
There was a problem hiding this comment.
I re-reviewed after the fix in 3cc622f and the rebase; no further issues found. The Authorization mode is now stored on the NetworkTask and threaded through as RequestKind::Tarball(task.authorization), so the .npmrc hint is correctly suppressed for github:/URL tarball dependencies, and the new regression test pins it. Because this changes which hosts receive Authorization headers during install, a human sign-off on the credential-routing model would still be worthwhile.
What was reviewed:
tarball_credentialspreserves the existing cross-origin guard (scope credentials only to scope's origin;url_authonly to hosts the user explicitly keyed) — a hostile registry still cannot redirect credentials.UrlAuth::matchespath check requires a directory boundary, not a string prefix; port-less keys default to the request scheme's port; entries without sendable credentials are dropped so they can't shadow a shallower key.fill_credentials_from_url_authruns after--token/BUN_CONFIG_TOKENand skips scopes that already have credentials, so it can't downgrade an explicitly-set token.
Extended reasoning...
Overview
This PR adds npm-style by-URL resolution of .npmrc //host/path/ credential lines. It touches the .npmrc loader (src/ini/lib.rs), the config schema (src/options_types/schema.rs, src/api/lib.rs), package-manager options (PackageManagerOptions.rs), the tarball request path (NetworkTask.rs), the 401/403 error reporting (runTasks.rs), the registry types (npm.rs), the bunfig overlay (PackageManager.rs), docs, and adds 14 end-to-end tests in npmrc.test.ts against Bun.serve mock registries that assert the exact Authorization header received.
Security risks
This is credential-routing code: it decides which Authorization header (if any) is attached to manifest and tarball requests. The primary risk is sending a credential to a host the user did not configure it for. I checked the two guards that prevent this: (1) tarball_credentials returns the scope's credentials only when is_same_origin(tarball, scope.url) — the same protocol+hostname+effective-port check the pre-PR code used, now factored into a helper; (2) for any other origin it returns only a UrlAuth entry the user wrote for that exact host+port, with the path required to be a parent directory of the request path (the /no vs /no-deps test pins the boundary check). A registry-controlled dist.tarball therefore cannot exfiltrate the scope's token, and url_auth credentials go only where the user keyed them. fill_credentials_from_url_auth only fills scopes that have no credentials, so it cannot overwrite --token, BUN_CONFIG_TOKEN, or credentials embedded in a --registry URL.
Level of scrutiny
High — this is production credential handling in bun install. The implementation is additive (the existing load-time attachment and the same-origin guard are unchanged), well-tested (each shape has a mock-registry test that fails on the release build and asserts the exact header), and CI passed on all 179 jobs. But per the approval guidelines, changes to which remote hosts receive auth headers warrant a maintainer's sign-off rather than bot approval.
Other factors
My previous review's finding (the 401 hint being shown for NoAuthorization tarballs where the suggested .npmrc line would be ignored) was addressed: NetworkTask now stores authorization, write_init initializes it, both for_manifest and for_tarball set it, and missing_credentials_note returns empty for RequestKind::Tarball(NoAuthorization). The added regression test ("a 401 for a tarball dependency given by URL is reported without a suggestion") covers it. Jarred has engaged with the PR (requested conflict resolution, which was done) but has not yet approved.
2 participants
Problem
.npmrccredential lines (//host/path/:_authToken=,:_auth=,:username=+:_password=) are only attached to registries declared in a config file, at the moment that file is read (src/ini/lib.rs,load_npmrc/apply_registry_auth). Nothing looks a credential up by the URL of the request being made, which is how npm applies them.Authorizationheader and fail with a bareerror: GET <url> - 401:~/.npmrccontaining only//reg/:_authToken=T, plusbun install --registry http://reg/.npmrc, plusNPM_CONFIG_REGISTRYorBUN_CONFIG_REGISTRY(both are applied inOptions::load, after the.npmrclines were matched)dist.tarballURLs are on another host that has its own//host/line (NetworkTask::for_tarballonly ever consulted the package's scope, and correctly refuses to send the scope's credentials cross-origin)bun.lockstill holds absolute tarball URLs on the old host,.npmrchas lines for both hosts,bun install --frozen-lockfileon a cold cache 401s on every tarball//host/with a registry at/api/v4/.../npm/)Fix
bun_inikeeps every credential line, merged per//host/path/key, in the newBunInstall.url_auth. The existing load-time attachment is unchanged.Options::loadturns them intonpm::registry::UrlAuthentries (npm.rs), matched the way npm resolves credentials for a URL: same host, same port (a key without a port means the scheme's default port), key path equal to or a parent directory of the request path, deepest matching key wins. Entries with nothing sendable (anemailline on its own) are dropped so they cannot shadow a shallower key.Options::load, after bunfig,.npmrc, the environment and the command line have all been applied, any scope still without credentials takes the entry matching its registry URL (fill_credentials_from_url_auth). This covers--registry, the environment variables and the parent-path case, and leaves a scope that already has credentials (including--token/BUN_CONFIG_TOKEN) alone.for_tarballasksOptions::tarball_credentials: the scope's credentials when the tarball is on the scope's origin (the existing guard, unchanged), otherwise the entry matching the tarball URL, otherwise nothing. A registry still cannot get a scope's credentials sent anywhere else; the existing cross-origin test inbun-install.test.tsis unchanged..npmrckey that would have authenticated it (Options::missing_credentials_note, called from the twoGET <url> - <status>sites inrunTasks.rs). When credentials were sent the output is unchanged, and so is it forgithub:and URL tarball dependencies, which never carry registry credentials (theNetworkTaskkeeps theAuthorizationmode it was enqueued with, so the note is only offered where a line would be used).--registrycode inOptions::loadis not touched. Rebased on top of install: send credentials embedded in --registry and registry env var URLs #38796 (credentials embedded in those URLs): a URL with credentials fills the scope before the lookup runs at the end ofload, so the lookup stays out of its way, and the mergednpmrc.test.ts(this PR's tests plus install: send credentials embedded in --registry and registry env var URLs #38796's and url: parse a bare bracketed IPv6 host so .npmrc //[::1]:port/ credential keys match their registry #38828's) passes as a whole. Independent of publish: accept basic auth credentials from .npmrc and bunfig.toml #38776 (bun publishwith basic auth). install: resolve .npmrc credentials by path-segment ancestor #33869 reworks how lines attach to declared registries at load time; this change leaves that code alone and adds the by-URL lookup next to it. Supersedes install: honor .npmrc //host/:_authToken= for tarballs on a different host than the registry #34329 (now closed), which handled the separate-tarball-host case only.test/cli/install/npmrc.test.ts, newdescribeblock: 14 tests againstBun.serveregistries that 401 unless the exact expected header arrives, one per shape above plus basic auth fromusername/_passwordlines (for the registry host and for a separate tarball host), deepest-key precedence, a/no/key not matching/no-deps/..., and the note (present when nothing was sent, absent when a sent token was rejected). 13 of them fail on the 1.4.0 release build (the URL-dependency one guards the note against suggesting a line that would not be used; it fails on the first revision of this PR); the whole file (45 tests) passes with this change.config-precedence,bun-install-retry,redacted-config-logs,bun-info,bun-audit,bun-publish, and the auth/registry subsets ofbun-installandbun-install-registry;cargo clippyon the touched crates andtest/internal/source-lintsare clean.Fixes #30513
Background
npm::registry::Scope): bun's unit of registry configuration, one for the default registry plus one per@scope:registry. It holds the registry URL and the credentials sent with every request to it;Options::scope_for_package_namepicks it per package, andNetworkTask::for_manifest/for_tarballsend itstoken(bearer) orauth(base64user:pass)..npmrckeys of the form//host/path/. npm never attaches them to a registry; for every request it builds//host/pathfrom the request URL and walks up the path until a key with credentials matches. A registry's own key matches its manifests and tarballs because they share its host and path; a tarball served elsewhere gets credentials only if that location has its own key.PackageManager::init, thenOptions::load):.npmrcfiles are parsed intoBunInstall, bunfig is overlaid, thenOptions::loadapplies environment variables and command-line flags. Registries set in that last step did not exist when the.npmrclines were attached, which is why the lookup has to run at the end ofloadand again at request time for tarballs.[review] gate passed · iteration 5 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 5
evidence per changed file