Repository navigation
Conversation
18023b7 to
524782e
Compare
There was a problem hiding this comment.
I've reviewed this across four passes now and the latest commit is clean — no findings.
Given this rewrites credential resolution end to end (which key's token reaches which registry host), a human look is still warranted before merge.
What was reviewed
- The key-walk (
RegistryKey::walk,strip_one_key_component,has_auth) against npm'sregFromURI/hasAuth: string-prefix vs path-segment ancestry, trailing-slash as a distinct key,_authToken > _auth > username+_passwordprecedence, cross-file last-write-wins collapse. normalize_key/RegistryKey::from_parsed: host lowercasing, default-port drop, bracketed-IPv6 port detection, scheme-prefixed keys.- Redaction paths in
fmt.rs/lib.rsfor the new unknown-option warning under bothNO_COLORandFORCE_COLOR, quoted and unquoted keys. - Test hermeticity (every new spawn overrides
HOME/USERPROFILE/XDG_CONFIG_HOME) and that the matrix asserts exactAuthorizationheaders against a local server.
Extended reasoning...
Overview
Part 2 of a 3-part stack. Replaces exact-path .npmrc credential matching with npm's key-walk algorithm from npm-registry-fetch/lib/auth.js: collect every //host/path:opt= line from every .npmrc into one flat list, then for each registry walk its key from full path up to bare host and take the first key supplying a complete credential. Also normalises config keys (lowercase host, drop default port, drop leading scheme), requires the option name to be an exact key suffix (fixing _authtoken matching _auth), warns on unknown option names, and closes two redaction gaps in the diagnostic printer. ~400 lines rewritten in src/ini/lib.rs, ~800 lines of new tests.
Security risks
Credential resolution is the security surface here. The two failure modes are (a) sending a credential to a host/path it wasn't configured for and (b) printing a credential in a diagnostic. For (a), the walk is path-segment-based (strip_one_key_component pops one / or one non-/ run), so /projects/12 is never an ancestor of /projects/123/ — covered by explicit tests in both bun-install.test.ts and npmrc.test.ts. Lookups are byte-equal against normalised keys, and lookup uses rfind so last-write-wins matches npm's flat map. For (b), the new unknown-option warning routes through redact_sensitive_information: true; earlier passes found and fixed a quoted-key gap and the password prefix omission, and redacted-config-logs.test.ts now asserts both colour modes for quoted/unquoted misspelt keys.
Level of scrutiny
High. This is a full rewrite of how registry credentials are selected, in the package manager's request path, and it interacts with three config sources (.npmrc files, bunfig.toml, URL userinfo) plus an env override in part 3. The algorithm is derived from a specific upstream (npm-registry-fetch), and the PR description and code comments cite it, but the port is hand-written rather than mechanical.
Other factors
Three prior automated review rounds surfaced six findings, all minor (dead code created by the PR's own edits, a redaction-prefix gap, a misleading diagnostic on non-string values, a non-hermetic test, a stale comment) and all addressed in follow-up commits. The test matrix is unusually thorough — measured against npm 10.9.3 and 11.15.0, asserting exact Authorization header bytes on a local Bun.serve — and every new subprocess test isolates HOME/XDG_CONFIG_HOME. That gives good confidence in correctness, but the scope and the security sensitivity of "which token goes where" put this outside what I'll auto-approve.
5ab8568 to
ddf775b
Compare
1f497c8 to
a751ac0
Compare
There was a problem hiding this comment.
All six earlier inline comments are addressed — thanks. No new issues found on this pass. Given this rewrites the credential-to-registry resolution algorithm (which auth header goes to which host) and is part 2 of a stacked series, a human look is still worthwhile before merge.
What was reviewed:
- npm key-walk (
credentials/has_auth/RegistryKey::walk) againstnpm-registry-fetchsemantics: precedence, trailing-slash distinctness, string-prefix-is-not-ancestor. normalize_key/RegistryKey::from_parsedfor host lowercasing, default-port stripping, and bracketed-IPv6 handling.- Redaction paths (color and NO_COLOR) after the
has()deletion — every keyword still covered byhas_prefix(). - Test hermeticity: every new subprocess spawn overrides HOME/USERPROFILE/XDG_CONFIG_HOME.
Extended reasoning...
Overview
This PR replaces exact-path matching for .npmrc credential lines with npm's regFromURI key-walk: config lines from every .npmrc file are collected into one flat list, keys are normalised (scheme dropped, host lowercased, default port removed), and for each registry the resolver walks its <host><path>/ key upward one component at a time until a key supplies a complete credential (_authToken > _auth > username+_password). It also adds an IniOption::Unknown warning for misspelt option names, tightens option-name matching to an exact suffix, and closes two redaction gaps in the diagnostic printer. ~400 lines of new Rust in src/ini/lib.rs (replacing RegistryAuth), small edits in bun_core/fmt.rs / bun_core/lib.rs / ast/lib.rs / install/PackageManager.rs, and ~1000 lines of new tests.
Security risks
Credential routing is the security surface here. The two failure modes to guard against are (a) sending a credential to a host/path it was not scoped to, and (b) dropping a credential that should apply. The new algorithm is a straight port of npm-registry-fetch/lib/auth.js: byte-equal key lookup after normalisation, with the walk stripping one / or one trailing non-/ run per step. The test matrix explicitly covers the string-prefix case (/projects/12 must not authorise /projects/123/), cross-file collapse ordering, and precedence within a single key. The redaction changes only widen what gets masked in local terminal output.
Level of scrutiny
High. This is production credential-handling code in bun install, and the change is an algorithmic rewrite rather than a targeted fix. It is part 2 of 3 stacked on #40423, with the request-time resolution (--registry, tarball hosts) deferred to part 3 — a maintainer should see the whole shape.
Other factors
I raised six inline findings across three earlier passes (dead _authToken redaction branch; has_prefix() missing password; known-option-with-non-string-value falling through to the Unknown warning; a non-hermetic test; has() becoming dead after the password addition; a stale ordering comment). All six were fixed in follow-up commits and the threads are resolved. This run's bug hunt found nothing new. Test coverage is extensive and measured against npm 10.9.3/11.15.0; the PR description states 62 rows fail on main.
ddf775b to
d726cf7
Compare
|
Issue #40549 reports this same bug (GitLab token on |
00b9f38 to
95b2e64
Compare
d726cf7 to
da02f52
Compare
…edential in the request path
…tighten the new tests
95b2e64 to
d7fb062
Compare
29ba4bb to
7cc6cf0
Compare
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/ini/lib.rs— nit: pre-existing comment "The singleinstall.scoped = registry_mapwrite-back happens at the bottom ofload_npmrc" is stale — this PR moved the write-back intoparse_npmrc_intoand repurposedload_npmrcas a thin single-file wrapper, so the comment now names the wrong functionExtended reasoning...
A reader following the comment at src/ini/lib.rs:1806-1808 looks in
load_npmrcfor theinstall.scoped = Some(registry_map)assignment and finds only the newparse_npmrc_into(…); resolve_credentials(…); diagnose_config(…)sequence with noregistry_mapin sight — the write-back is at line 1872 insideparse_npmrc_into. On the base branch the comment was accurate becauseload_npmrcwas the function that owned this block; the rename/split in this PR made it stale. Per REVIEW.md "Only comment what the code cannot say" and src/CLAUDE.md "NEVER add comments to deleted code blocks", the fix is to update the function name toparse_npmrc_intoor delete the comment (the write-back is ~60 lines below and self-evident).Verification: nit — the comment is now stale exactly as described. On the base branch (d7fb062), the comment at src/ini/lib.rs:1533-1534 ("The single
install.scoped = registry_mapwrite-back happens at the bottom ofload_npmrc…") sat insidepub fn load_npmrc(base line 1315), and the write-backinstall.scoped = Some(registry_map);was at base line 1604 in that same function — the comment was…
…hrough the scope builder, one Authorization builder
7cc6cf0 to
bcd9157
Compare
There was a problem hiding this comment.
This pull request has now been reviewed several times and this review found new issues. Before patching these one by one, step back: would one root-cause fix close several of them? Is the pull request's scope growing with each push? Prefer root-cause fixes, keep scope fixed, and note out-of-scope improvements as follow-ups.
… on read, warn on unknown options
…ted password keys; drop the dead _authToken redaction branch
…drop a stale ordering comment
…r applies them once by npm's walk
bcd9157 to
cb37a43
Compare
There was a problem hiding this comment.
This pull request has now been reviewed several times and this review found new issues. Before patching these one by one, step back: would one root-cause fix close several of them? Is the pull request's scope growing with each push? Prefer root-cause fixes, keep scope fixed, and note out-of-scope improvements as follow-ups.
176ecb7 to
cb87058
Compare
Part 2 of 3, split out of #33869. Stacked on #40423.
_authsent as written; no credential left in the request path.npmrclines, applied once in the package managerFixes #30311. Fixes #28233. Fixes #40549.
What this changes
An
.npmrccredential line only applied to a registry when the URL paths matched exactly, so a host-root token was dropped for a registry under a path (GitLab's documented shape, #30311 and #40549) and a scoped registry's own line stopped matching in 1.3.11 (#28233).npm's model is a flat config map and one walk per URL (
npm-registry-fetch'sregFromURI). This implements that model in two halves:bun_inicollects every//host/path/:<option>=line from every.npmrcfile into one map keyed by the literal text between//and:<option>=, last write wins, and collapses it to one entry per key that carries a complete credential (_authToken, else_auth, elseusername+_password; an empty value supplies nothing). That list isinstall.url_auth.bun_inino longer applies credentials to any registry.Options::loadapplies them once, after every source that can set a registry URL (.npmrc,bunfig.toml,$NPM_CONFIG_REGISTRY,--registry): each registry still without credentials gets the first key on the walk from its URL (the full path, then one segment shorter each time, down to the bare host; the key with a trailing slash is a distinct key visited first; a string prefix is not an ancestor). A credential from bunfig, the environment or the command line therefore outranks a.npmrcline by construction. A credential written into the registry URL itself (userinfo, or a:_authToken=segment in the path) is the weakest source, as in npm, whosegetAuthnever reads it: any complete.npmrcline for the key replaces it. A key carries no scheme, so a line applies to anhttp://registry with the same host too; only a credential carried over from a previous registry refuses an https-to-http downgrade.A registry's key comes from the WHATWG serialisation of its URL, the one requests are built from: host lowercased, a default port dropped, the query left out. Hand-written keys are compared as written, after one fold: a scheme is dropped (Bun's docs show
//http://localhost:4873/:_authToken=) with that scheme's default port, and the host is lowercased; a scheme-less key keeps its port, as in npm, so//host:443/is the key ofhttp://host:443/, not ofhttps://host/.The option name must end the key.
//host/:_authtoken=used to match_authas a substring and go out asBasic <token>; it now warns_authtoken is not a known .npmrc option; ignoring this lineat.npmrc:<line>and sends nothing. The warning never echoes the line, so a value under a misspelt name cannot reach stderr, and it only fires for an option-shaped word with a value: a bare//host:portline or a key without=prints nothing.--silentsuppresses it. A known option with a non-string value (_authToken=true) is ignored silently.Dropped on the way:
emailis accepted and ignored (npm never sends it, and nothing in Bun read it); a loneusernameor_passwordon a registry's own key no longer layers over the URL's userinfo (npm'shasAuthneeds the pair).Tests
bun-install.test.tsrunsbun installagainst a local registry for the walk matrix (ancestor keys, string prefixes, trailing slashes, scoped and default registries,bunfig.tomlregistries with and without credentials, home and project files);npmrc.test.tscovers key normalisation through the loader hook and the diagnostics end to end;redacted-config-logs.test.tspins that no credential value reaches stderr under any spelling.