Skip to content

install: resolve .npmrc credentials by path-segment ancestor - #33869

Open
alii wants to merge 17 commits into
mainfrom
claude/npmrc-auth-path-matching
Open

install: resolve .npmrc credentials by path-segment ancestor#33869
alii wants to merge 17 commits into
mainfrom
claude/npmrc-auth-path-matching

Conversation

@alii

@alii alii commented Jul 9, 2026

Copy link
Copy Markdown
Member

Fixes #30311

Related: #26241 (does not reproduce on current main — see the comment there)

What does this PR do?

An .npmrc auth line only applied to a registry when their URL pathnames matched exactly, so a token declared at the host root was silently dropped for a registry mounted under a path prefix:

@myorg:registry=https://gitlab.example.com/api/v4/packages/npm/
//gitlab.example.com/:_authToken=${MY_TOKEN}

//api/v4/packages/npm/, so the token was never attached, the request went out unauthenticated, and the registry answered 401. That's the shape GitLab's docs tell you to write, and it's how any registry served under a path prefix is configured.

This implements the algorithm npm actually uses. npm keeps config in a flat map and walks literal string keys up the registry URL, taking the first key that supplies complete auth (npm-registry-fetch/lib/auth.js):

let regKey = `//${parsed.host}${parsed.pathname}`
while (regKey.length > '//'.length) {
  const authKey = hasAuth(regKey, opts)      // opts['//host/a/:_authToken'] — a map lookup
  if (authKey) return { regKey, authKey }
  regKey = regKey.replace(/([^/]+|\/)$/, '') // strip a segment, then the slash, alternating
}

Credentials are then read from that one key, via npm's precedence chain: _authToken, else _auth, else a complete username + _password pair.

Modelling that directly, rather than reconstructing its ordering, makes several properties fall out of the structure instead of needing to be enforced:

  • Keys repeated across .npmrc files collapse last-write-wins before resolution. A project file's _authToken= falsifies the home file's token rather than shadowing it at a distance.
  • The segment boundary is structural. //host/api/v4/projects/12 cannot authorize a registry at /api/v4/projects/123/ — a string prefix is not a key on the walk.
  • "An empty value supplies nothing" is one truthiness test on the collapsed value.
  • A trailing slash is simply a distinct key, visited before its unslashed twin.
  • Config keys are never parsed as URLs. ConfigItem.registry_url already is the key.

Because resolution is a pure function of the collapsed config, it runs once after every .npmrc is parsed rather than once per file. That removes the snapshot/restore layer whose only job was to undo the previous file's writes, and lets each diagnostic name the file that actually contains the offending line (previously a home-file loc was reported against the project file, once per subsequent file).

certfile/keyfile remain unsupported and ignored. npm treats a complete pair as auth and stops the walk, sending a client certificate and no Authorization header; Bun has no mTLS, so stopping there would send no credentials — a guaranteed 401 whose only diagnostic says the option "was not applied". They must not suppress a credential we can send.

Layering a half credential (a lone username) over bunfig.toml's is Bun-only — npm sends nothing for a half pair — and stays exact-path, so an ancestor's stray username= cannot rebind a deeper registry's stored password to a new identity.

src/install/npm.rs: embedded credentials no longer leak into the request path

Scope::from_api's yarn-style parser both extracts a credential from the registry URL and truncates the pathname, but it was gated on registry.token.is_empty(). Once the walk can populate that token from an ancestor, the gate closes and :_authToken=SECRET stays in the path:

registry=http://host/api/:_authToken=SECRET  +  //host/:_authToken=T

before   GET /api/:_authToken=SECRET/@myorg%2fpkg   Bearer T     ← secret in the path
after    GET /api/@myorg%2fpkg                      Bearer T

The pathname is now sanitized regardless of credential state. Only segments that parse as a credential name=value are stripped, so a registry mounted under a path containing a plain colon keeps its path — which also fixes a pre-existing bug: main rewrites http://host/a:b/c/ to /a/ whenever no credential is configured.

Why the linked issues were previously misdiagnosed as a %2f encoding bug

Both issues, and three earlier PRs (#30312, #33774, #33784), blamed Bun encoding the scope slash as lowercase %2f instead of %2F. That theory is wrong, and this PR does not touch percent-encoding. The confusion is understandable — the encoded slash sits right next to the failure:

error: GET https://gitlab.example.com/api/v4/.../npm/@myorg%2fpackage - 401
  • npm sends lowercase %2f too. npm-package-arg@13 does name.replace('/', '%2f'); the comment directly above that line says %2F, which is almost certainly where the claim came from. Confirmed on the wire with npm 10.9.3 and 11.15.0.
  • GitLab treats both spellings identically. Against live gitlab.com, %2f, %2F and a raw / return byte-identical responses. Rack::Utils.unescape is case-insensitive.

The 401 was always auth. #26241 says so directly.

The lookup key is npm's key

npm builds its key from a WHATWG URL, whose host is lowercased and drops a default port. bun_url::URL::parse normalizes neither, so registry=https://Registry.Example.COM/api/ and registry=https://host:443/api/ both silently dropped their tokens. The registry side is now normalized the same way, comparing the scheme case-insensitively.

Config keys are still compared byte for byte, as npm compares them. npm gets away with that because @npmcli/config's nerfDart runs the URL through new URL() before writing a key, so every key npm writes is already lowercase and free of a default port. A key hand-written any other way applies to nothing — in npm, and now here.

That leaves two ways to write a key that silently matches nothing, and silently dropping a credential is the bug this PR exists to fix. So when respelling a key the way npm would write it changes which credential gets selected, Bun says so:

warn: this .npmrc line applies to no registry: keys are matched literally,
      so the host must be lowercase and must not spell out a default port
  2 | //Registry.Example.COM/:_authToken=***********

The check runs the real selection over a normalized copy of the config and compares results, rather than asking whether the key merely appears on the walk. An ancestor email, an ancestor's lone username, an empty value, and a key already shadowed by a deeper one all resolve identically either way — warning there would send the reader to fix something that changes nothing. Nine tests pin both directions.

It goes through a new Log::add_warning_opts, the warning twin of the existing add_error_opts, so redact_sensitive_information masks the token in the printed source line.

⚠️ Upgrade note: a key that spells out a default port (//host:443/) no longer matches. Released Bun matched it. It now warns rather than failing silently.

How this behaved across releases

All measured against the same mock GitLab-shaped registry (/api/v4/projects/<id>/packages/npm/), capturing the Authorization header on the wire.

.npmrc shape 1.2.23 1.3.14 main this PR npm
//host/:_authToken=T, registry at /api/v4/… sends dropped dropped sends sends
//host/api/v4/projects/12/:_authToken=T, registry at /projects/123/ sends ⚠️ no auth no auth no auth no auth
two projects on one host, a token keyed to each wrong token ⚠️ correct correct correct correct

1.2.23 matched on the host alone. That is why the token in #30311 worked there — and also why a token keyed to //host/api/v4/projects/12/ was sent to project 123, and why two per-project tokens collapsed to whichever was read last (#26241).

1.3.x replaced that with exact-pathname equality. It fixed both of those, and broke the ordinary case: a token declared at a shallower path than the registry stopped applying at all.

Neither behaviour is npm's. npm walks the registry path upwards and takes the deepest key that supplies auth, which is what this PR implements — so a host-root token applies, a sibling path does not, and per-project tokens keep working.

The %2f in the 404/401 message is a red herring; it is what npm sends too.

Behavior

Measured against npm 10.9.3 / 11.15.0 by capturing the Authorization header on a local registry. Registry pathname /api/v4/projects/123/packages/npm/:

.npmrc line npm Bun before Bun after
//host/:_authToken=T sends dropped sends
//host/api/:_authToken=T sends dropped sends
//host/api/v4/projects/123/packages/npm/:_authToken=T sends sends sends
//host/api/v4/projects/123/packages/npm:_authToken=T sends sends sends
//host/api/v4/projects/12/:_authToken=T no auth no auth no auth
//host/api/v4/projects/12:_authToken=T no auth no auth no auth

How did you verify your code works?

Tests live alongside the existing should handle @scoped authentication test in test/cli/install/bun-install.test.ts, plus unit coverage in npmrc.test.ts. Each spawns a real bun install against a local Bun.serve({ port: 0 }) registry and asserts the exact Authorization header — and, for the embedded-credential cases, the exact request path:

  • the full matrix above, including both string-prefix-not-an-ancestor cases;
  • longest-match precedence in both file orders;
  • _authToken, _auth, username + _password, and the precedence between them at one key;
  • a deeper half-pair / email / lone certfile not shadowing a shallower _authToken;
  • the trailing-slash-is-a-distinct-key rule, including the bare-host spelling;
  • cross-file collapse: a project _authToken= / username= / _password= clearing the home file's value, and overriding it;
  • an ancestor's partial credential not rebinding a credential the registry URL's userinfo stored, and the registry's own key still layering over it;
  • registries declared in bunfig.toml: a credential-less one resolves through the same walk (host-root _authToken, _auth, and the string-prefix negative), and one that bunfig.toml gave credentials keeps them whatever .npmrc says;
  • embedded :_authToken= stripped from the path with and without a competing .npmrc token, and a plain colon in the path left alone.

The walk-up and cross-file tests fail on the previous build. The negative/security tests pass on both — they are guards against a future startsWith, not evidence of the fix. Reverting each load-bearing clause individually fails exactly the tests that cover it.

Beyond the suite: 6,000 randomized (.npmrc, registry) pairs diffed against npm-registry-fetch's own getAuth — uppercase hosts, default and non-default ports, IPv6 literals, duplicate keys, empty values, %2f — match npm exactly, {missing: 0, wrongCred: 0, crossHostLeak: 0}. A further 200 two-file cases cover cross-file key collapse, and 128 adversarial registry-URL shapes confirm no credential ever reaches the request path. The whole table was also replayed end-to-end against real npm and released bun 1.3.14, driving bun install of a scoped private package through an auth-requiring mock registry, plus probes for a bare-host registry, doubled slashes, a 40-segment path, an IPv6 literal host, and a config key deeper than the registry.

Merge with main (#38333, #38796, #38828, #38183)

main now applies .npmrc credentials to bunfig.toml-declared registries in a separate pass: load_npmrc_config returns what it read and apply_registry_auth applies it to each bunfig registry that bunfig itself did not give credentials (project config beats .npmrc). This branch keeps that contract and that precedence rule, but both passes resolve with the key walk:

  • load_npmrc_config returns the collapsed Vec<ConfigItem>; apply_registry_auth runs credential_items over it for each credential-less bunfig registry, through the same apply_to_registry the .npmrc-declared registries use. main's exact-match RegistryAuth / RegistryCredential / parse_auth are superseded and removed.
  • load_npmrc_config also reads bunfig's install config so the "matches no registry" warning and the empty-_auth error are computed over the registries both passes resolve (one selection rule, bunfig_registry_url, feeds both). A registry bunfig gave credentials is left out: no .npmrc line can apply to it, so there is nothing to diagnose.
  • NpmRegistry::has_credentials (install: send credentials embedded in --registry and registry env var URLs #38796) also counts the auth field this branch adds.
  • Three tests here had an .npmrc _auth or a lone username override bunfig.toml credentials. Under main's rule bunfig wins, so they now exercise the same layering against credentials stored from the registry URL's userinfo (the case where it still applies), and new cases pin both directions of the bunfig rule under the walk. main's own config-precedence.test.ts (51 cases) passes unchanged against the walk.
  • The bracketed-IPv6 key tests from url: parse a bare bracketed IPv6 host so .npmrc //[::1]:port/ credential keys match their registry #38828 are kept (they pass under literal key matching because only the registry side is parsed) and extended with a host-root key and an address whose last group spells the scheme's default port.
  • bun audit reads credentials from install: canonical registry URL in Scope; redact secrets in bun audit registry URLs #38183's AuditRegistry, keeping the byte-exact header append; add_warning_fmt_opts_with_note follows add_formatted_msg's new arity.

_auth is forwarded verbatim

npm never decodes _authnpm-registry-fetch sends Basic <value> as written. Bun base64-decoded it and rejected anything that wasn't user:pass, so an opaque blob (Artifactory, Gemfury) or a token-as-username with a blank password was a hard error. And once _auth won the precedence chain it suppressed username/_password on the same key, so a config that authenticated in 1.3.x sent no Authorization header at all.

.npmrc 1.3.14 this PR npm
_auth=<opaque blob> + username/_password Basic <user:pass> Basic <blob> Basic <blob>
_auth=<b64 "tok:"> alone nothing Basic <b64> Basic <b64>
_auth=<b64 "ab:cd"> + username/_password Basic <x:y> Basic <ab:cd> Basic <ab:cd>

NpmRegistry carries the raw value through to Scope::from_api. The decode stays, but only to recover a username for bun pm whoami.

Two credential leaks in the diagnostics, both pre-existing

Found while testing the warning above, both fixed here because this PR leans on the same redaction path:

  • A quoted config key ("//host/:_authToken"=secret) is a string literal, not an identifier, so the highlighter's redaction never armed and the value printed verbatim in the source frame under color. Only the config path sets redact_sensitive_information, so the REPL and markdown highlighters are untouched.
  • dupe_value_decoded was the one credential diagnostic that didn't redact — and _password is not valid base64 fires exactly when the value is a live plaintext password. It printed it to stderr.

The redaction suite asserted only that some * appeared, never that the secret was absent, which is how both survived it. It now asserts absence, in both color modes. Three new cases; all three fail without the fixes.


no test proof · iteration 3 · 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 test/cli/install/bun-publish.test.ts

An .npmrc auth line only applied to a registry when their URL pathnames
matched exactly, so a token declared at the host root was silently dropped
for a registry mounted on a deeper path:

    @myorg:registry=https://gitlab.example.com/api/v4/packages/npm/
    //gitlab.example.com/:_authToken=${TOKEN}

The request went out unauthenticated and the registry answered 401. This is
the common shape for GitLab, and for any registry served under a path prefix.

npm resolves credentials by walking up the registry URL's path segments (the
"nerf dart" walk in npm-registry-fetch) and reading them from the deepest
ancestor that supplies complete auth. Match that:

  - An .npmrc path applies when it equals the registry path or is a
    path-*segment* ancestor of it. A bare string prefix is not an ancestor:
    //host/api/v4/projects/12 must never authorize /api/v4/projects/123/.
  - The deepest matching path wins, regardless of order in the file.
  - Credentials come from that one path only, via npm's precedence chain
    (_authToken, else _auth, else a complete username + _password pair).
  - A trailing slash makes a distinct config path, ranked above its
    unslashed twin, as npm's walk does.
  - email is not a credential and resolves to its own deepest path.

Both the default-registry and scoped-registry branches go through the same
predicate. certfile/keyfile remain unsupported and ignored; they must not
suppress a credential we can actually send.

Layering a half credential (a lone username) over bunfig.toml's stays
exact-path only. Bun has that second credential layer and npm does not, so
letting an ancestor supply half a pair would rebind a deeper registry's
stored password to a username chosen elsewhere.

The behavior table was measured against npm 10.9.3 and 11.15.0 by capturing
the Authorization header on a local registry, and is covered by tests.
@robobun

robobun commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator
Updated 3:01 PM PT - Aug 16th, 2026

@robobun, your commit b42ab443cb1a14168ee8d23f47589b5636f6871e passed in Build #99612! 🎉


🧪   To try this PR locally:

bunx bun-pr 33869

That installs a local version of the PR into your bun-33869 executable, so you can run:

bun-33869 --bun

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. _authToken with with scopes stopped working in bun v1.3.11 works in 1.3.10 #28233 - _authToken at host root not matched to scoped registries at deeper paths; the ancestor walk-up now finds the host-root token, matching npm's nerf-dart behavior

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #28233

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR centralizes .npmrc credential resolution across files using path-segment ancestor matching, sanitizes embedded registry credentials, adds warning and redaction support, and expands authentication and diagnostic tests.

Changes

npmrc path-ancestor authentication

Layer / File(s) Summary
Credential collection and centralized resolution
src/ini/lib.rs, src/options_types/schema.rs
Collects credential entries with source attribution, resolves them across .npmrc files, applies npm precedence, and preserves verbatim _auth values in NpmRegistry.
Embedded registry credential parsing
src/install/npm.rs
Extracts credentials from registry URL paths, removes credential suffixes from requests, and preserves opaque _auth values.
Warning and secret redaction support
src/ast/lib.rs, src/bun_core/fmt.rs
Adds configurable warning emission and redacts INI-style credential values in formatted diagnostics.
End-to-end authentication tests
test/cli/install/bun-install.test.ts
Covers ancestor matching, precedence, credential layering, multi-file collapse, embedded credentials, path sanitization, and unscoped registry behavior.
npmrc diagnostics and normalization tests
test/cli/install/npmrc.test.ts
Covers empty-auth diagnostics, path matching, credential decoding, email handling, authority normalization, and host-case warnings.
Configuration log redaction tests
test/cli/install/redacted-config-logs.test.ts
Verifies that token, auth, and password values remain absent from colored and uncolored diagnostics.
🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning [30311] The PR does not implement the requested percent-encoding fix for scoped package URLs, so it misses the issue's primary acceptance criteria. Implement the scoped package URL encoding fix for manifest requests, ensuring uppercase %2F while preserving compatibility with registries that require encoded slashes.
Out of Scope Changes check ⚠️ Warning Most changes are unrelated to #30311, including broad .npmrc auth-resolution, diagnostics, redaction, and embedded-auth handling work. Split the auth-resolution, diagnostics, and redaction changes into separate PRs, and keep this one focused on the scoped %2F encoding fix.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: resolving .npmrc credentials by path-segment ancestor.
Description check ✅ Passed The description includes both required sections and provides detailed implementation context and verification results.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@src/ini/lib.rs`:
- Around line 2040-2063: The empty _auth diagnostic loop in ini/lib.rs is
re-scanning the shared configs list across the whole load chain, so earlier
files’ errors get added again on later passes. Update the logic around the
existing conf_item_matches_exactly check and log.add_error_opts call to only
iterate the entries appended for the current .npmrc by capturing the pre-load
configs.len() and starting from that offset. Keep the registry-matching behavior
the same, but ensure only this file’s new ConfigOpt::_Auth entries can produce
the diagnostic.

In `@test/cli/install/bun-install.test.ts`:
- Line 702: Mark the ".npmrc auth resolves by path-segment ancestor" suite in
bun-install.test.ts as concurrent by switching the top-level describe around
this matrix to describe.concurrent. The cases are isolated because each one
creates its own port: 0 server and tempDir, so no shared mutable state needs
protection; keep the existing test bodies unchanged and apply the concurrent
wrapper to the describe block identified by that suite name.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: a585b4cc-9794-44ee-9bb0-e9a57cce3adb

📥 Commits

Reviewing files that changed from the base of the PR and between fc865b3 and 54451e4.

📒 Files selected for processing (3)
  • src/ini/lib.rs
  • test/cli/install/bun-install.test.ts
  • test/cli/install/npmrc.test.ts

Comment thread src/ini/lib.rs Outdated
Comment thread test/cli/install/bun-install.test.ts
Comment thread test/cli/install/npmrc.test.ts
Comment thread src/ini/lib.rs Outdated
@alii
alii marked this pull request as draft July 9, 2026 22:12
@alii

alii commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Moving this to draft — a design review turned up a regression and a much smaller design. Details so the issues don't look fixed when they aren't.

Regression, introduced by this PR. Duplicate config keys across .npmrc files are not collapsed before resolution, so an empty value in the project file shadows a real one from the home file:

# ~/.npmrc
//host/api/v4/:_authToken=HOMETOKEN
# ./.npmrc
//host/api/v4/:_authToken=
//host/api/v4/:_auth=YWxpY2U6czNjcmV0
npm         -> Basic YWxpY2U6czNjcmV0
bun 1.3.14  -> Basic YWxpY2U6czNjcmV0
this PR     -> no Authorization header

auth_mechanism picks the mechanism from the first non-empty duplicate (the home file's _authToken) while the apply loop writes the last duplicate's value (the project file's empty one). Within a single file this can't happen — the object parser overwrites in place — which is why every test here passes.

The design is wrong, and that's why it's 583 lines. npm doesn't scan for a deepest ancestor. It builds a flat key -> value config map and walks literal string keys up the registry path:

let regKey = `//${parsed.host}${parsed.pathname}`
while (regKey.length > '//'.length) {
  const authKey = hasAuth(regKey, opts)      // opts['//host/a/:_authToken'] — a map lookup
  if (authKey) return { regKey, authKey }
  regKey = regKey.replace(/([^/]+|\/)$/, '') // strip a segment, then the slash, alternating
}

Everything this PR hand-builds falls out of that structure for free:

  • Duplicate collapsing — it's a map, so last-write-wins across files happens before resolution. The regression above becomes unrepresentable.
  • The segment-boundary security check (/projects/12 must not authorize /projects/123/) — structural, not a hand-written predicate to get right.
  • "An empty value supplies nothing"hasAuth tests truthiness of a map lookup; "" is falsy.
  • The trailing-slash-is-a-distinct-key rule — it's just the two spellings the walk visits in order.
  • No URL::parse of config keys at all. This PR parses one per comparison, inside a doubly-nested scan.

My (len, trailing_slash) depth ordering does turn out to be an order-isomorphism of npm's key sequence — a fuzz over 60k random .npmrc/registry pairs found zero mismatches against npm for the winning key and mechanism. So the walk itself is right. But it's a reimplementation of a map lookup over the wrong data structure, and the wrong structure is what forces the dedupe bug, the Baselines/restore_baseline layer (which exists only to undo a per-file re-resolution that a parse-all-then-resolve-once split would never do), and the O(n²) scan.

Rewriting as key -> {optname -> value} + literal walk should land net negative against main, not +583.

Two more things it surfaced, both worth fixing here:

  1. NpmRegistry has no _auth field, so handle_auth base64-decodes it into username/_password and rejects anything without a colon. npm never decodes _auth — it forwards the string verbatim. So an opaque _auth blob, or _auth for a blank-password/token-as-username registry, authenticates under npm and sends nothing here. (1.3.14 sent the wrong credential instead — also broken, differently.)
  2. The ancestor walk silently disables Scope::from_api's yarn-style URL-embedded-auth stripper, which is gated on registry.token.is_empty(). With @myorg:registry=http://host/api/:_authToken=EMBED plus any ancestor .npmrc auth line, the :_authToken=EMBED segment stops being stripped and goes out in the request path. npm behaves the same way (it doesn't understand the yarn form), but 1.3.14 stripped it, and a secret in a URL path ends up in logs and proxies.

The diagnosis in the PR body stands — this is an auth-resolution bug, not the %2f encoding — and the reproductions in #30311 / #26241 are unchanged. Reworking around the map.

Replaces the path-depth scan with the algorithm npm actually uses.

npm keeps config in a flat map and walks literal string keys up the
registry URL, taking the first key that supplies complete auth:

    regKey = `//${host}${pathname}`
    while regKey.length > 2:
        if hasAuth(regKey): use it
        regKey = regKey.replace(/([^/]+|\/)$/, '')

Modelling that directly, rather than reconstructing its ordering as a
comparable depth, makes several things fall out for free:

  - Keys repeated across .npmrc files collapse last-write-wins before
    resolution, so a project file's `_authToken=` falsifies a token from
    the home file instead of shadowing it at a distance.
  - The segment boundary is structural. `//host/api/v4/projects/12`
    cannot authorize a registry at `/api/v4/projects/123/`.
  - "An empty value supplies nothing" is one truthiness test on the
    collapsed value, not a filter applied at three separate sites.
  - A trailing slash is simply a distinct key, visited first.
  - Config keys are never parsed as URLs. `ConfigItem.registry_url` is
    already the key.

Credentials are then read from that one key via npm's precedence chain:
_authToken, else _auth, else a complete username + _password pair.

Because resolution is a pure function of the collapsed config, it runs
once after every .npmrc is parsed rather than once per file. That
removes the snapshot/restore layer whose only job was to undo the
previous file's writes, and lets each diagnostic name the file that
actually contains the offending line.

certfile/keyfile remain unsupported and ignored; they must not suppress
a credential we can send. Layering a half credential over bunfig.toml's
is Bun-only and stays exact-path, so an ancestor's stray `username=`
cannot rebind a deeper registry's stored password.

Separately, the yarn-style credential segments embedded in a registry
URL are now stripped from the pathname before the request is built,
whether or not the credential is adopted -- otherwise the secret ships
in the request path once .npmrc supplies a token. Only segments that
parse as a credential `name=value` are stripped, so a registry mounted
under a path containing a plain colon keeps its path.

Behavior verified against npm 10.9.3 and 11.15.0 by capturing the
Authorization header and request path on a local registry.
@alii
alii marked this pull request as ready for review July 10, 2026 00:43
@alii

alii commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

Restructured. src/ini/lib.rs is now +207 / −143 against main instead of +583 / −114, and the resolver is npm's key walk rather than a reimplementation of its ordering.

The rewrite fixed the cross-file regression by construction — a flat map has no duplicate keys to disagree about — and deleted the Baselines/restore_baseline layer, path_is_ancestor, path_match_depth, conf_item_match_depth, auth_match_depth, applied_depth, email_match_depth, CredentialSource, source_reads, and credential_fallback_depth. Config keys are no longer parsed as URLs at all; ConfigItem.registry_url already is the key.

Reviewing the restructure turned up two more bugs, both introduced by it, both now fixed with tests that fail when the fix is reverted:

  1. credential_items filtered empty values before applying last-write-wins. A project .npmrc that explicitly cleared username= resurrected the home file's username/_password and sent Basic alice:s3cret; npm and 1.3.14 both send nothing. The emptiness test now happens after the collapse, everywhere.
  2. Making parse_embedded_auth unconditional truncated any registry path containing a bare colon (/a:b/c//a/). Only segments that parse as a credential name=value are stripped now. That also fixes a pre-existing bug: main mangles http://host/a:b/c/ whenever no credential is configured.

Also worth knowing: on the embedded-credential case Bun now behaves better than npm, which ships :_authToken=SECRET in the request path because it doesn't understand the yarn form.

Verified end-to-end against real npm and released bun by driving bun install of a scoped private package through an auth-requiring local registry — plus probes for a bare-host registry, doubled slashes, a 40-segment path, an IPv6 literal host, and a config key deeper than the registry. 295 pass / 0 fail across the touched suites; 343 pass / 0 fail across bun-install-registry, bun-add, bun-info, redacted-config-logs, bun-publish.

Two pre-existing bunx.test.ts failures (postinstall scripts correctly with symlinked bunx, bunx claude requests @anthropic-ai/claude-code) reproduce on origin/main; unrelated to this branch.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@test/cli/install/bun-install.test.ts`:
- Around line 1173-1174: Update probeEmbedded to retain the drained stderr
output and include it in the object passed to expect, using a non-constraining
matcher such as expect.any(String), so diagnostics appear in assertion failures
without constraining the expected content.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 4e2a854d-260f-46df-b1f7-042657ba0678

📥 Commits

Reviewing files that changed from the base of the PR and between 54451e4 and 44a66ee.

📒 Files selected for processing (4)
  • src/ini/lib.rs
  • src/install/npm.rs
  • test/cli/install/bun-install.test.ts
  • test/cli/install/npmrc.test.ts

Comment thread test/cli/install/bun-install.test.ts Outdated
Three gaps between the config-key walk and the key npm actually builds,
plus a credential that could still reach the request path.

npm keys on a WHATWG URL's `host`, so its key is always lowercased and
never spells out a default port. `bun_url::URL::parse` normalizes
neither, so `registry=https://Registry.Example.COM/api/` never matched
`//registry.example.com/:_authToken=`, and `https://host:443/` never
matched `//host/:_authToken=`. Fold the authority on both sides -- the
registry's, and the config key's -- comparing the scheme
case-insensitively. Hosts are case-insensitive, so folding the key too
is a superset of npm: a key that names the same host in a different case
keeps working. Paths stay case-sensitive. A key that spells out the
default port (`//host:443/`) no longer matches, as in npm.

The stored key is kept exactly as written, because diagnostics quote it
back; the folded form lives alongside it.

`parse_embedded_auth` anchored its scan on the last `:` in the pathname.
A `:` inside a credential value therefore ended the scan before anything
was stripped, so `registry=http://host/api/:_authToken=aa:bb` shipped the
token in the request path and then failed to send it. Anchor on the
`:<name>=` marker instead. A bare `:` still belongs to the path and is
left alone, so a registry under `/a:b/c/` keeps its path -- which it did
not before, credential or no credential.

A terminal `_authToken`/`_auth` also used to stop the scan outright,
leaving any credential written to its left in the pathname:
`/api/:_password=cA==:_authToken=T` requested `/api/:_password=cA==/...`.
Terminal now ends what is read, not what is stripped. Which credential
wins is unchanged: rightmost terminal marker, leftmost of duplicates.

Bun-only layering of a half credential over bunfig.toml's now takes both
halves from one config key. It could pair a `username` from `//host` with
a `_password` from `//host/`, which npm treats as two distinct keys that
each supply nothing.

Verified against npm 10.9.3 and 11.15.0 by capturing the Authorization
header and the request path on a local registry.
@alii

alii commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

Pushed 9ad985e. A second adversarial pass over the key-walk found four more things; all four are fixed with tests that fail when the fix is reverted.

Two were credential-in-the-request-path leaks.

parse_embedded_auth anchored its scan on the last : in the pathname, so a : inside a credential value ended the scan before anything was stripped:

registry=http://host/api/:_authToken=aa:bb
  1.3.14   Bearer aa       GET /api/@myorg%2fpkg          (token silently truncated at the colon)
  before   null            GET /api/:_authToken=aa:bb/…   (token in the path, and not sent)
  now      Bearer aa:bb    GET /api/@myorg%2fpkg

And a terminal _authToken/_auth stopped the scan outright, leaving anything to its left in the path. This one reproduces on released Bun too:

registry=http://host/api/:_password=cA==:_authToken=T
  1.3.14   Bearer T   GET /api/:_password=cA==/@myorg%2fpkg   ← base64 password in the path
  now      Bearer T   GET /api/@myorg%2fpkg

Terminal now ends what is read, not what is stripped. Which credential wins is unchanged — rightmost terminal marker, leftmost of duplicates, both pinned by tests.

The lookup key wasn't npm's key. npm builds it from a WHATWG URL, whose host is lowercased and drops a default port. bun_url::URL::parse does neither, so:

registry=https://Registry.Example.COM/api/  +  //registry.example.com/:_authToken=T   → dropped (npm sends it)
registry=https://host:443/api/              +  //host/:_authToken=T                   → dropped (npm sends it)

Both sides of the comparison now fold the authority, case-insensitively on the scheme too. Paths stay case-sensitive. Folding the key as well as the registry is a deliberate superset of npm — hosts are case-insensitive, so a key naming the same host in a different case keeps working, and it cannot match a host the user didn't name.

⚠️ One behavior change worth calling out: a key that spells out the default port, //host:443/, no longer matches. npm's key never contains :443 either. Released Bun matched it. There's a test pinning the new behavior.

The stored key is now kept exactly as written, because the certfile/keyfile warning quotes it back — it was echoing a lowercased line the user never typed.

Finally, the Bun-only half-credential layer could pair a username from //host with a _password from //host/ — two distinct keys, each of which supplies nothing to npm. Both halves now come from one key.

Verification: 13,000 random (.npmrc, registry-URL) pairs diffed against npm-registry-fetch's own getAuth{ancestorLeak: 0, missing: 0, wrongCred: 0}; every divergence was the documented Bun-only layer. Plus the usual end-to-end bun install against an auth-requiring local registry, and probes for a bare-host registry, doubled slashes, a 40-segment path, an IPv6 literal, uppercase schemes, and a 900-marker pathname. 310 pass in the touched suites, 348 across bun-install-registry, bun-add, bun-info, redacted-config-logs and bun-publish, 0 fail.

…y misses

The previous commit folded the case of a config key's authority before
comparing it, so `//REGISTRY.EXAMPLE.COM/:_authToken=` matched a registry
at `registry.example.com`. npm does not: it compares keys byte for byte.
It gets away with that because `@npmcli/config`'s `nerfDart` runs the URL
through a WHATWG `URL` before writing a key, so every key npm itself
writes is already lowercase and free of a default port. A hand-written
key spelled any other way applies to nothing, in npm and now in Bun.

Only the registry side is normalized, which is what `URL.host` does.

That leaves two ways for a key to silently apply to nothing -- an
uppercase host, or a spelled-out default port -- and silently dropping a
credential is the bug this branch exists to fix. So when respelling a
key the way npm would write it changes which credential gets selected,
say so. The check runs the real selection over a normalized copy of the
config and compares, rather than asking whether the key is merely on the
walk: an ancestor `email`, an ancestor's lone `username`, an empty value
and a key already shadowed by a deeper one all resolve to the same
credential either way, and a warning there would send the reader off to
fix something that changes nothing.

The diagnostic goes through a new `Log::add_warning_opts`, the warning
twin of `add_error_opts`, so `redact_sensitive_information` masks the
token in the printed source line.

Resolution now matches npm on all 6000 pairs of a randomized differential
against `npm-registry-fetch`'s own `getAuth` -- uppercase hosts, default
and non-default ports, IPv6 literals, duplicate keys, empty values -- with
no divergence left to document.
@alii

alii commented Jul 11, 2026

Copy link
Copy Markdown
Member Author

Pushed e29ad93. Config keys are now compared byte-for-byte, exactly as npm compares them, so there is no divergence left to document.

The previous commit folded the case of a config key's authority, which made //REGISTRY.EXAMPLE.COM/:_authToken= match a registry at registry.example.com. npm doesn't do that. It gets away with a literal compare because @npmcli/config's nerfDart runs the URL through a WHATWG URL before writing a key — so every key npm writes is already lowercase and without a default port. Only the registry side needs normalizing, and that's all this does now.

The cost of dropping the fold is that two spellings silently match nothing: an uppercase host, and a spelled-out default port (//host:443/, which released Bun did match). Silently dropping a credential is the bug this PR exists to fix, so both now warn — and only when respelling the key would actually change which credential gets selected:

warn: this .npmrc line applies to no registry: keys are matched literally,
      so the host must be lowercase and must not spell out a default port
  2 | //Registry.Example.COM/:_authToken=***********

Getting that precise mattered. A first cut asked "is this key on the walk?", which warns for an ancestor email, an ancestor's lone username, an empty value, and a key already shadowed by a deeper one — none of which change anything when respelled. The reader would have fixed the line, seen no difference, and watched the warning disappear: the same silent-failure shape as the original bug. It now runs the real selection over a normalized copy of the config and compares. Nine tests pin both directions; two of them fail if the warning is removed, seven if it over-fires.

The warning goes through a new Log::add_warning_opts — the warning twin of the existing add_error_opts — so redact_sensitive_information masks the token in the source frame. Without it the code frame prints the secret, which would have been worse than the silent drop.

Result: 6,000 randomized (.npmrc, registry) pairs now match npm-registry-fetch's own getAuth exactly{exactMatchWithNpm: 6000, missing: 0, wrongCred: 0, crossHostLeak: 0} — over uppercase hosts, default and non-default ports, IPv6 literals, duplicate keys and empty values. Previously 1,110 of those were the case-folding superset. Plus 200 cross-file cases and 128 adversarial registry-URL shapes (0 credentials in the request path).

Known remaining divergences from WHATWG's host, all pre-existing and none reachable without an exotic registry: IDN hosts aren't punycoded, %-escapes in an authority aren't decoded, and the default-port table omits ftp.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
src/ini/lib.rs (1)

1575-1595: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Diagnose only the effective _auth value.

This loop scans every raw empty _auth entry, while credential resolution uses last-write-wins. An earlier :_auth= is still reported as invalid even when a later file overrides the same key with valid credentials. Restrict the diagnostic to the index returned by lookup(...) or collapse duplicate keys first.

Proposed fix
-        for conf_item in configs.iter() {
+        for (i, conf_item) in configs.iter().enumerate() {
             if !matches!(conf_item.optname, ConfigOpt::_Auth) || !conf_item.value.is_empty() {
                 continue;
             }
+            if lookup(configs, &conf_item.registry_url, ConfigOpt::_Auth) != Some(i) {
+                continue;
+            }
🤖 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 `@src/ini/lib.rs` around lines 1575 - 1595, Restrict the empty `_auth`
diagnostic in the config-validation loop to the effective last-write-wins entry.
Use `lookup(...)` for each relevant registry/key and only report when its
returned index matches `conf_item.source_idx` (or otherwise deduplicate entries
before scanning), so earlier overridden `_auth` values are ignored while
preserving the existing registry matching and error reporting.
🤖 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 `@test/cli/install/npmrc.test.ts`:
- Around line 823-839: stderrOf currently ignores the spawned install process
exit status. Retain concurrent stdout/stderr draining, capture the process exit
code from proc.exited, assert it is successful, and only then return stderr so
unrelated installation failures cannot make negative cases pass.

---

Outside diff comments:
In `@src/ini/lib.rs`:
- Around line 1575-1595: Restrict the empty `_auth` diagnostic in the
config-validation loop to the effective last-write-wins entry. Use `lookup(...)`
for each relevant registry/key and only report when its returned index matches
`conf_item.source_idx` (or otherwise deduplicate entries before scanning), so
earlier overridden `_auth` values are ignored while preserving the existing
registry matching and error reporting.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: ee40235d-7cd6-4f60-b615-2e7e891f8aca

📥 Commits

Reviewing files that changed from the base of the PR and between 9ad985e and e29ad93.

📒 Files selected for processing (3)
  • src/ast/lib.rs
  • src/ini/lib.rs
  • test/cli/install/npmrc.test.ts

Comment thread test/cli/install/npmrc.test.ts Outdated
Comment thread src/ini/lib.rs
Comment thread test/cli/install/npmrc.test.ts Outdated
Comment thread src/ini/lib.rs
…agnostics

Four problems, all reachable from an ordinary .npmrc.

`_auth` was base64-decoded and rejected unless it held `user:pass`. npm
never decodes it -- `npm-registry-fetch` sends `Basic <value>` verbatim --
so an opaque blob, or a token-as-username with a blank password, is a
credential to npm and was a hard error here. Worse, once `_auth` won the
precedence chain it also suppressed `username`/`_password` on the same
key, so a config that authenticated in 1.3.x began sending no
Authorization header at all. `NpmRegistry` now carries the raw value
through to `Scope::from_api`, which forwards it. The decode remains, but
only to recover a username for `bun pm whoami`; it no longer gates the
credential and no longer errors on shapes npm accepts.

The near-miss warning added in the previous commit derived a global claim
from per-registry state: `before` and the default port belonged to the
registry being walked, while the message and the `warned` set did not. It
therefore fired on a line that WAS supplying the credential -- a `:443`
key is legitimate for an `http://` registry, since 443 is not http's
default port -- and telling the reader to drop the port deleted a working
credential. It now warns only where the line supplies nothing to ANY
registry as written and would supply something if respelled.

A quoted config key (`"//host/:_authToken"=secret`) is a string literal,
not an identifier, so the highlighter's redaction never armed and the
value printed verbatim in the source frame. The string-literal branch now
arms it for a key naming a credential. Only the config path sets
`redact_sensitive_information`, so the REPL and markdown highlighters are
unaffected.

`dupe_value_decoded` was the one credential diagnostic that did not
redact, and "`_password` is not valid base64" is exactly the case where
the value is a live plaintext secret. It printed it into stderr.

The redaction suite asserted only that some `*` appeared, never that the
secret did not -- which is why the two leaks survived it. It now asserts
absence, in both color modes.

Also: the resolve-phase error header named the file of the first message,
which is a warning's file whenever one precedes the error. It names the
first error's file.
@alii

alii commented Jul 11, 2026

Copy link
Copy Markdown
Member Author

Pushed 6feb82f. A close read of the diff turned up four more problems, two of them credential leaks. All four are fixed with tests that fail when the fix is reverted.

The warning I added in the last commit was worse than the bug it warned about. It derived a global claim from per-registry state: before and the default port belonged to the registry being walked, while the message and the warned-set did not. Given

registry=http://example.com:443/api/
@s:registry=https://example.com/api/
//example.com:443/api/:_authToken=SECRETTOKEN

it printed "this line applies to no registry … must not spell out a default port" — but 443 is not http's default port, the line is supplying the token, and following the advice sets the credential to empty. It now warns only where the line supplies nothing to any registry as written and would supply something if respelled. Nine tests pin both directions, including a two-registry config where a :443 key is legitimate.

_auth was rejected where npm accepts it. npm never decodes _auth; npm-registry-fetch sends Basic <value> verbatim. Bun decoded it and hard-errored unless it held user:pass — so an opaque Artifactory-style blob, or a token-as-username with a blank password, failed. Worse, once _auth won the precedence chain it suppressed username/_password on the same key, so a config that authenticated on 1.3.x sent no Authorization header at all:

.npmrc 1.3.14 before this commit now npm
_auth=<opaque> + username/_password Basic <user:pass> none Basic <opaque> Basic <opaque>
_auth=<b64 "tok:"> alone none none Basic <b64> Basic <b64>

NpmRegistry now carries the raw value to Scope::from_api. The decode remains, but only to recover a username for bun pm whoami — verified it still prints alice.

Two credential leaks in the diagnostics, both pre-existing on main. A quoted key ("//host/:_authToken"=secret) is a string literal, not an identifier, so the highlighter's redaction never armed and the value printed verbatim in the source frame under color. And dupe_value_decoded was the one credential diagnostic that didn't redact — which is unfortunate, because _password is not valid base64 fires exactly when the value is a live plaintext password.

The redaction suite asserted only that some * appeared, never that the secret was absent. That is how both survived it. It now asserts absence, in both colour modes.

Also: the resolve-phase error header named the file of the first message, which is a warning's file whenever one precedes the error — so it pointed at the wrong .npmrc.

Re-verified on 6feb82f: 6,000 randomized pairs still match npm-registry-fetch's getAuth exactly (missing 0, wrongCred 0, crossHostLeak 0), 200 cross-file cases match, 128 adversarial registry-URL shapes leave no credential in the request path, and the _auth matrix above matches npm on all five rows. 343 + 303 + 34 + 123 tests pass across the touched suites, the install suites, bun-publish, and the bundler (for the highlighter change). No cargo warnings.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/options_types/schema.rs`:
- Around line 239-241: Update the layout note associated with NpmRegistry’s
clone implementation to change “five Box<[u8]> views” to “six Box<[u8]> views,”
keeping the field count consistent throughout the comment.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 561c179d-54a7-4dfc-8564-8811087d3ba1

📥 Commits

Reviewing files that changed from the base of the PR and between 6862c3b and 6feb82f.

📒 Files selected for processing (7)
  • src/bun_core/fmt.rs
  • src/ini/lib.rs
  • src/install/npm.rs
  • src/options_types/schema.rs
  • test/cli/install/bun-install.test.ts
  • test/cli/install/npmrc.test.ts
  • test/cli/install/redacted-config-logs.test.ts

Comment thread src/options_types/schema.rs Outdated
Comment thread src/install/npm.rs
Comment thread src/options_types/schema.rs Outdated
Six problems, each reproduced on the wire before fixing.

An empty yarn-style marker in a registry URL (`registry=https://h/api/:_auth=`)
counted as a found credential: it terminated the scan and shadowed the
credential the `.npmrc` supplied, sending an unauthenticated request with no
diagnostic -- the same silent 401 this branch exists to end, through a
different door. An empty marker is still stripped from the path, but supplies
nothing and no longer ends the scan.

`bun pm view` and `bun audit` panicked on a non-UTF-8 credential: the header
was appended through a lossy Display (U+FFFD expands one byte to three) after
reserving the raw byte count. `_authToken` had this crash already; forwarding
`_auth` verbatim opened the same door for `_auth`, since the removed base64
gate had been accidentally guarding it. Both now use the raw-byte append that
`NetworkTask::append_auth` already uses, with the same comment.

`bun publish`'s NeedAuth gate never consulted `registry.auth`, so an
`_auth`-only `.npmrc` that installs fine was told to run `npm login`, and the
four `Basic <auth>` branches below the gate were unreachable.

The quoted-key redaction matched the option name by exact suffix, while the
ini parser recognizes options by substring -- so any spelling one byte off the
exact form (a trailing space inside the quotes) parsed as a credential line
but printed its value verbatim under color. Redaction now matches at least as
loosely as the parser: a keyword prefix after any colon.

An `.npmrc` `_auth` now supersedes URL-embedded credentials outright, so
`Scope.user` can only be derived from the value that produced `Scope.auth` --
previously `bun pm whoami` could report an identity the registry never
authenticated.

The decode-failure diagnostics for `_auth` are gone: npm sends the value
regardless, so failing to decode only means `bun pm whoami` cannot show a
username. Printing `error: invalid _auth value` for a credential that is then
sent successfully was noise, and the empty-`_auth` message no longer claims
base64 is expected.
@alii

alii commented Jul 11, 2026

Copy link
Copy Markdown
Member Author

Pushed 2bf89c3. A close pass over 6feb82f found six more problems — every one reproduced on the wire before fixing, and each fix verified fail-before/pass-after.

An empty embedded marker silently discarded the .npmrc credential. registry=https://h/api/:_auth= counted as "credential found": it terminated the scan and shadowed the real _auth from .npmrc, sending an unauthenticated request with no diagnostic — the same silent-401 class this PR exists to end, reintroduced through the newest path in it.

control  registry=http://H/api/              + //H/:_auth=<blob>   ->  Basic <blob>
before   registry=http://H/api/:_auth=       + same                ->  (no Authorization)
now      registry=http://H/api/:_auth=       + same                ->  Basic <blob>

bun pm view / bun audit panicked on a non-UTF-8 credential. The header was built with a lossy format_args! Display (U+FFFD expands 1→3 bytes) after reserving the raw byte count. The _authToken variant of this crash exists on main today; forwarding _auth verbatim opened the same door for _auth, because the removed base64 gate had been accidentally guarding it. Both commands now use the raw-byte append NetworkTask::append_auth already uses — which fixes the pre-existing _authToken crash too.

bun publish rejected an _auth-only .npmrc. The NeedAuth gate checked token and URL userinfo but never registry.auth, so a config that installs fine was told to run npm login, and the Basic <auth> branches below the gate were dead.

The quoted-key redaction missed spellings one byte off. The ini parser recognizes options by substring, my redaction matched by exact suffix — so "//host/:_authToken " (trailing space inside the quotes) parsed as a credential line but printed its value verbatim under color. Redaction now matches at least as loosely as the parser (keyword prefix after any colon).

bun pm whoami could report an identity the registry never saw. With an opaque .npmrc _auth plus yarn-style credentials embedded in the registry URL, the wire carried the _auth blob while Scope.user was built from the embedded pair. An .npmrc _auth now supersedes embedded credentials outright, so user can only come from the value that produced auth. Decodable _auth still yields a working whoami.

error: invalid _auth value printed for a credential that was then sent successfully. npm is silent on all these shapes (it never decodes). The decode-failure diagnostics are gone; the decode survives only to give whoami a username, and the empty-_auth message no longer claims base64 is expected.

Re-verified on 2bf89c3: 6,000 randomized pairs still match npm-registry-fetch's getAuth exactly; 200 cross-file cases match; 128 adversarial registry-URL shapes leave no credential in the request path (the three new deltas vs 1.3.14 are the empty-marker fix, including one where 1.3.14 sent nothing and the real credential now goes out). 348 + 303 + 34 + 16 tests pass across the touched suites, install suites, publish, and audit.

Comment thread test/cli/install/npmrc.test.ts Outdated
The `_auth` base64 decode lived in the config layer, writing the decoded
halves into `username`/`password` -- fields that `Scope::from_api` reads
back as credential-selection inputs. So which credential went on the wire
could depend on whether an unrelated value happened to decode. The config
layer now stores `_auth` verbatim and nothing else; `Scope::from_api`
decodes the value it actually chose, only to derive `user` for
`bun pm whoami`, and never falls through to build `user` from credentials
the wire does not send.

`_password` decoding now matches `_auth` and npm: lenient, no error, no
credential drop. npm decodes with `Buffer.from(value, "base64")`, which
never fails. (For a value that is not valid base64 the recovered bytes
differ from npm's, which round-trips through lossy UTF-8 first; both are
garbage, and the valid-base64 case is byte-identical.)

The dead-key warning now names things instead of explaining a rule:

    warn: the .npmrc key "//EXAMPLE.COM:443/api/" matches no registry
    note: npm writes this key as "//example.com/api/"

via a new `Log::add_warning_fmt_opts_with_note`; the source frame still
redacts the value in both color modes.

New coverage: `_authToken` beats `_auth` at the same key in both line
orders; an `.npmrc` `_auth` wins over bunfig username/password whether or
not it decodes (the two cases must behave identically -- that is the
point); `bun publish` and `bun pm view` send `Basic <value>` from an
`_auth`-only config on the wire. Stale comments claiming `_auth` must
decode, or that URL credential suffixes survive into `Scope.url`, are
corrected.
@alii

alii commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

Pushed 7f29db9. This closes out the remaining review items — one structural change, coverage for everything it touches, and the message rewrites.

The _auth decode has left the config layer. It used to write the decoded halves into username/password, which Scope::from_api reads back as selection inputs — so which credential went on the wire could depend on whether an unrelated value happened to decode. The config layer now stores _auth verbatim and nothing else; from_api decodes the value it actually chose, solely to give bun pm whoami a username, and never falls through to build an identity from credentials the wire doesn't send. Two wire tests pin the invariant directly: bunfig username/password plus an .npmrc _auth sends Basic <that value> whether or not it decodes — the two cases must behave identically, and now do.

_password decoding matches _auth and npm: lenient, no error:, no credential drop (npm's Buffer.from(value, "base64") never fails). One documented divergence: for a value that isn't valid base64, the recovered bytes differ from npm's — npm round-trips through lossy UTF-8 (inserting U+FFFD) before re-encoding, we use the decoded bytes directly. Both are garbage no registry accepts; the valid-base64 case is byte-identical, and the invalid case prints nothing and leaks nothing in either color mode.

The dead-key warning now names things:

warn: the .npmrc key "//EXAMPLE.COM:443/api/" matches no registry
note: npm writes this key as "//example.com/api/"

with the source frame still redacting the value in both color modes.

New coverage: _authToken beats _auth at the same key in both line orders (the precedence line that decides which secret ships was previously unpinned — swapping the branches kept the suite green); bun publish and bun pm view each send Basic <value> from an _auth-only config, asserted on the wire; and the stale comments claiming _auth must decode, or that URL credential suffixes survive into Scope.url, are corrected.

Re-verified on 7f29db9: 6,000 randomized pairs match npm-registry-fetch's getAuth exactly; 200 two-file wire cases match npm; 0 credentials in any request path; whoami prints the username for a decodable _auth and an honest missing-authentication error for an opaque one. 419 tests across the six touched suites, 0 fail.

Comment thread src/runtime/cli/publish_command.rs
Comment thread src/ast/lib.rs Outdated
@alii

alii commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

@robobun get this mergable.

@robobun

robobun commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

✅ Mergeable at b42ab44: merged main (rewired #38333's apply_registry_auth onto the key walk, summarized in the PR body under "Merge with main"); the dead-key diagnostics cover bunfig.toml registries and the empty-_auth error lands only on the line its key collapsed to. All review threads addressed. Locally green: npmrc (95), main's config-precedence (51), the bun-install auth matrix (91), redacted-config-logs, bun-audit, the publish credential blocks, and the source lints.

CodeRabbit's %2F pre-merge warning is the misdiagnosis the PR body's collapsed section already refutes (npm sends lowercase %2f; the 401 was always auth).

Resolves conflicts in src/ini/lib.rs (adopts the forbid-unsafe Parser refactor
from #35320 and the pub(crate) narrowing from #36184 under this branch's
parse_npmrc_into split; keeps lenient _password decode), npmrc.test.ts (keeps
this branch's suite; the _password-diagnostic test from #36165 is superseded
by lenient decoding and its redaction is covered in redacted-config-logs), and
bun-info.test.ts (import set union).

Also addresses the open review items: publish_command.rs now writes the
Authorization value byte-exact (matching pm view / audit), the two
empty-_auth tests isolate HOME, the non-UTF-8 pm view test probes a local
server, probeEmbedded surfaces stderr on failure, add_warning_opts is removed,
and the misplaced doc comments on default_port_for / the WHATWG-normalization
describe / NpmRegistry::dupe are corrected.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found on 436c123, but this rewrites credential resolution end-to-end (npm's ancestor walk, verbatim _auth forwarding, redaction in the highlighter, header builds in publish/view/audit) and ships a behavior change (//host:443/ keys stop matching) — worth a human pass before merge.

What was reviewed:

  • The ancestor-walk in resolve_credentials / credential_items against npm's regFromURI/hasAuth semantics, including the string-prefix-not-an-ancestor guard and cross-file key collapse.
  • parse_embedded_auth for path sanitization independent of credential state, and the .npmrc _auth precedence over embedded/bunfig credentials in Scope::from_api.
  • Redaction: quoted-key arming in fmt.rs scoped to the config path only; the new warning routes through redact_sensitive_information.
  • Test hermeticity — the two remaining HOME-only tests were re-checked and now override USERPROFILE/XDG_CONFIG_HOME per robobun's fix.
Extended reasoning...

Overview

This PR replaces exact-pathname .npmrc credential matching with npm's path-segment ancestor walk (src/ini/lib.rs: ~400 new lines implementing resolve_credentials, auth_for_registry, has_auth, credential_items, host/port normalization, and a dead-key warning). It adds NpmRegistry.auth to carry .npmrc's _auth verbatim through Scope::from_api (src/install/npm.rs), extracts yarn-style embedded-credential parsing into parse_embedded_auth so the pathname is sanitized unconditionally, switches bun pm view/bun audit/bun publish to raw-byte header appends, arms redaction for quoted ini keys in the syntax highlighter (src/bun_core/fmt.rs), and adds Log::add_warning_fmt_opts_with_note. ~1000 lines of new tests across six suites cover the full precedence matrix, cross-file collapse, embedded-credential stripping, non-UTF-8 credentials, whoami identity derivation, and redaction in both color modes.

Security risks

This is credential-handling code. The relevant risk classes are: (a) sending a credential to the wrong registry (cross-host or sibling-path leak), (b) leaving a credential in the request path, (c) printing a credential in a diagnostic, and (d) silently dropping a credential. The tests explicitly pin (a) via the /projects/12 vs /projects/123 string-prefix cases and the two-registry :443 case; (b) via probeEmbedded asserting exact request paths; (c) via the strengthened redaction suite asserting secret absence in both color modes; (d) via the ancestor-walk matrix and the dead-key warning. The behavior change — //host:443/ no longer matching — is intentional npm parity but is a potential breakage for existing configs, mitigated by the new warning.

Level of scrutiny

High. This is production package-manager auth on a path every bun install against a private registry hits, with a documented upgrade-affecting behavior change. Earlier review rounds on this PR surfaced real issues (credential leaks in diagnostics, a panic on non-UTF-8, wrong precedence for opaque _auth), all now addressed — which is evidence the change is subtle enough to merit maintainer sign-off.

Other factors

All eight prior review threads (mine, CodeRabbit's, and the two hermeticity findings) are resolved as of 436c123. CI was green on the pre-merge commit and robobun reports the touched suites green locally post-merge. Test coverage is unusually thorough (wire-level assertions, both file orders, both slash spellings, 6000-pair fuzz against npm-registry-fetch per the description). The candidate issues raised this run (two USERPROFILE hermeticity concerns) were verified addressed in 436c123. Deferring solely because of scope and security sensitivity, not because of any open concern.

Comment thread src/bun_core/fmt.rs Outdated
Comment thread src/runtime/cli/audit_command.rs Outdated
…key over-redaction

npm::whoami() was the last registry-credential site still building the header
via write!("Bearer {}", BStr::new(...)); switched to extend_from_slice to
match pm view / audit / publish.

The quoted-key colon scan in the highlighter now runs after the three value
redactors, so a URL whose userinfo password happens to start with a redacted
keyword does not carry an armed flag across continue 'outer and mask the rest
of the line. Over-redaction only; new redacted-config-logs case pins it.
Comment thread src/ini/lib.rs
The None arm was unreachable and the new caller in resolve_credentials would
have silently desynced the normalized vec from configs/applied/dead if it ever
became live. Returning OOM<ConfigItem> directly makes the index correspondence
structural.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings this pass — the last two commits addressed the remaining nits (whoami() byte-exact Authorization, the quoted-key colon-scan ordering in fmt.rs, and the vestigial Option on ConfigItem::dupe()). Deferring to a human for final sign-off given this rewrites credential selection end-to-end and includes an intentional behavior break (//host:443/ keys stop matching).

What was reviewed:

  • The resolve_credentials walk against npm's regFromURI/hasAuth semantics — segment-boundary matching, cross-file collapse, precedence, and the Bun-only half-credential layer staying exact-path.
  • parse_embedded_auth running unconditionally so no credential segment survives into the request path, and its marker-anchored scan leaving plain-colon paths intact.
  • Redaction: quoted-key scan now runs after the URL/UUID/npm-secret redactors so should_redact_value cannot leak across continue 'outer; the new dead-key warning goes through redact_sensitive_information.
  • Test hermeticity on Windows (USERPROFILE/XDG_CONFIG_HOME overrides) — the two flagged cases were fixed in 436c123.
Extended reasoning...

Overview

This PR replaces .npmrc credential matching (previously exact-pathname equality) with npm's actual algorithm: walk the registry URL's config keys deepest-first and take the first key supplying complete auth. It restructures src/ini/lib.rs so all .npmrc files are collapsed into one flat config before a single resolve_credentials pass, adds NpmRegistry.auth to carry _auth verbatim (npm never decodes it), rewrites the yarn-style embedded-credential parser in src/install/npm.rs to sanitize the request path unconditionally, sweeps every Authorization-header build site (pm view, audit, publish, whoami) off lossy BStr Display onto raw-byte appends, and adds a redaction path for quoted .npmrc keys plus a new "matches no registry" warning. ~760 lines of new tests assert the exact Authorization header and request path on the wire across the full matrix.

Security risks

This is squarely security-sensitive: it decides which secret is attached to which registry request, whether a secret can leak into a request path or a diagnostic, and whether a string-prefix match can send project 12's token to project 123. The design choices here (segment-boundary key equality, half-credentials only layering at the registry's own key, certfile/keyfile never stopping the walk, config keys compared byte-for-byte) are all defended in the description and pinned by negative tests, and the two pre-existing credential leaks in diagnostics are fixed with absence assertions in both color modes. I found no new leak surface this pass, but the blast radius of a mistake here is credential misdelivery.

Level of scrutiny

High. This is a ~1,500-line behavioral rewrite of the package manager's auth layer with an explicit compat break called out in the description (keys spelling out a default port stop matching — released Bun matched them). It has been through many review rounds; all ten prior threads (mine and CodeRabbit's) are resolved, and the most recent commits (869e004, f5635df) close out the last two nits I raised. The 6,000-case fuzz against npm-registry-fetch and end-to-end replay against real npm are strong evidence, but they are author-reported.

Other factors

Test coverage is unusually thorough — every claim in the description maps to a wire-level assertion, and the redaction suite now asserts secret absence rather than mere * presence. The finder-raised Windows-hermeticity concern (missing USERPROFILE) was addressed in 436c123 and re-verified. No outstanding reviewer comments. Given the auth surface and the deliberate upgrade-visible change, a maintainer should give this a final look before merge.

Jarred-Sumner added a commit that referenced this pull request Aug 15, 2026
… registry URLs (#38183)

### Problem
- A registry configured as `http:host:port/path/` (scheme followed by a
single colon, which `new URL()` and npm accept) fails every resolution
before a request is made:
  ```
error: Invalid package name "react": manifest URL
"http://host:port/path/react" is not on registry "http:host:port/path/"
  error: InvalidURL
  ```
- Same failure for the other spellings the WHATWG parser rewrites (probe
against a local server on the released 1.4.0, full output in the details
below): a `..` segment in the path, an unencoded space in the path,
backslashes, surrounding whitespace. Three entries of the "Registry
URLs" table in `bun-install.test.ts` (`https:example.org`,
`https://////example.com///`, `http://點看`) hit it too; the table did not
notice because `failed to resolve` is also printed after this rejection.
- An upper-case scheme passes the manifest check (it compares
case-insensitively) but fails `for_tarball`'s same-origin comparison
(`src/install/NetworkTask.rs`, `send_auth`), which is case-sensitive, so
the tarball is requested without the `Authorization` header.
- Cause: `Scope::from_api` (`src/install/npm.rs`) and the `--registry`
branch of `Options::load`
(`src/install/PackageManager/PackageManagerOptions.rs`) store the
registry href as written. `NetworkTask::for_manifest` builds the
manifest URL with `bun_url::join`, which runs the href through the
WHATWG parser, and then compares the result against `URL::parse` of the
stored string. `URL::parse` is Bun's lenient scanner: it only recognizes
a scheme followed by `://`, does not resolve `..`, percent-encode, strip
whitespace or lower-case, so for these spellings the two sides describe
different URLs (`http:host:port/...` parses as hostname `http` with no
protocol). The same stored string feeds `for_tarball`'s origin
comparison, `extract_tarball::build_url`, `url_is_under_registry` in
`bun.lock.rs`, the DNS prefetch and the `@@<hostname>` cache folder
name.

### Fix
- Adds `Scope::set_url`: stores the WHATWG serialization of the
configured URL (`bun_url::URL::from_string`, the same parser `join`
uses) and derives `url_hash` from it. `Scope::from_api`, the
`--registry` branch and the `parseManifest` test helper
(`src/install_jsc/npm_jsc.rs`) all build the URL through it, so there is
one place that decides what a `Scope` holds.
- Correct because the stored href now equals the base `join` resolves
against, so every consumer that compares with, concatenates onto or
hashes the href agrees with the URL actually requested. The on-registry
check itself is unchanged and still rejects a name that joins outside
the registry directory (tested).
- Credentials cannot reach anything new. The manifest request URL is
unchanged (`join` parsed its base with the WHATWG parser before this
change too, so `join(as written, name)` and `join(normalized, name)` are
the same URL); only the value it is compared against changes. Both
checks compare against the stored href, so the only origin a tarball
request can now carry credentials to is the normalized registry origin,
which is where the manifest request (which always carries them) already
went. Before, a non-canonical spelling could only make the comparisons
fail. The one request URL that does change is the
`extract_tarball::build_url` fallback (manifest or `bun.lock` entry
without a tarball URL), which concatenates onto the href and now
produces a URL on that same origin instead of a string that failed the
`http(s)://` prefix check.
- A string the WHATWG parser rejects is stored as written, so the
`Failed to join registry "<as written>"` diagnostics for the invalid
entries of the table are unchanged (the table still asserts them).
- `set_url` runs after `from_api` has split the `/:_authToken=` style
credentials off the path, because the WHATWG parser would percent-encode
them. This is also why the normalization is not applied earlier, in the
config loaders: the `.npmrc` / bunfig string form extracts userinfo
credentials with the lenient parser first (an unencoded `#` in a
password is accepted there today).
- Already-canonical URLs serialize to themselves, so their `url_hash`,
manifest cache files and cache folder names are unchanged;
`https://registry.npmjs.org/` in particular still hashes to
`DEFAULT_URL_HASH`. The hash changes only for spellings the parser
rewrites; of those, only upper-case spellings worked before, and for
them the cost is one re-download of the cache.
- Not covered, on purpose: `.npmrc` credential lines
(`//host/path/:_authToken=`) are matched against the registry URL in
`src/ini/lib.rs` before a `Scope` exists, still by lenient parse of the
string as written, so a registry spelled `https:host/path/` gets its
requests but not its `.npmrc` token. That matching is being reworked in
#33869; filed separately. Spellings where the lenient parser takes the
port for a `:key=value` credential suffix (`http:/host:port/`,
`http:////host:port/`) are still mangled by the credential stripping
that runs before `set_url`; without a port they work.
- Verified: `test/cli/install/bun-install.test.ts` ("Registry URLs"):
new `spellings the WHATWG parser rewrites` block (bunfig registry object
with a token for each spelling, asserting the paths and `Authorization`
header of the manifest and tarball requests plus the cache folder name;
`.npmrc registry=`; `--registry`; the rejection message for a name that
joins outside the registry), and the table's handled entries now also
assert the rejection did not happen. 12 tests fail on the released build
(9 with the error above, the upper-case one with `authorization: null`
on the tarball, the rejection test because the message quoted the raw
spelling), all pass with this change.
- Also run with the change: the rest of `bun-install.test.ts` (remaining
failures are the bitbucket/gitlab/`some.url` network tests and
`--registry CLI flag`, which fail identically on the released build in
this container), `npmrc.test.ts`, the registry/whoami/manifest-cache
tests of `bun-install-registry.test.ts`,
`bun-install-pathname-trailing-slash.test.ts`, `cargo clippy` on
`bun_install` and `bun_install_jsc`, and the source lints.

### Background
- `npm::registry::Scope` is the package manager's record of one registry
(the default one or an `[install.scopes]` entry): its URL, credentials
and `url_hash`. `url_hash` keys the manifest cache files and tells
whether the default registry was overridden, which switches cache folder
names from `name@version` to `name@version@@<hostname>`.
- Bun has two URL parsers. `bun_url::URL::parse` is a lenient,
allocation-free scanner over the input bytes that the HTTP client and
the package manager use to read components out of a URL they already
hold. `bun_url::join` / `URL::from_string` call WTF::URL, the WHATWG
parser behind `new URL()`, which normalizes (scheme and host case,
missing slashes, `.`/`..` segments, percent-encoding, IDN) and rejects
what it cannot parse. The lenient scanner gives correct answers on
WHATWG output; the bug was feeding it the user's input instead.
- The "is not on registry" check exists so that a dependency name that
joins to another origin (an alias like `npm:\\other-host\pkg`) cannot
make Bun send the scope's credentials there; the tarball check in
`for_tarball` does the same for `dist.tarball` URLs returned by the
registry. Both are same-origin comparisons of a request URL against the
stored registry URL.

<details>
<summary>Probe: registry spellings against a local server (released
1.4.0 vs this branch)</summary>

Each row configures the spelling in `bunfig.toml` with one dependency
and records what reaches the server.

| registry as written | 1.4.0 | this branch |
| --- | --- | --- |
| `http://localhost:PORT/some/path/` | `GET /some/path/react` | same |
| `http:localhost:PORT/some/path/` | no request, `is not on registry` |
`GET /some/path/react` |
| `http:\\localhost:PORT\some\path\` | no request, `is not on registry`
| `GET /some/path/react` |
| `http://localhost:PORT/some/x/../path/` | no request, `is not on
registry` | `GET /some/path/react` |
| `http://localhost:PORT/some path/` | no request, `is not on registry`
| `GET /some%20path/react` |
| ` http://localhost:PORT/some/path/` (leading space) | no request, `is
not on registry` | `GET /some/path/react` |
| `HTTP://LOCALHOST:PORT/some/path/` | `GET /some/path/react` (tarball
would be sent without `Authorization`) | same request, tarball
authorized (covered by the test) |
| `http:/localhost:PORT/some/path/` | stored as `http://http/localhost/`
by the credential-suffix stripping | unchanged (see Fix) |
| `http:////localhost:PORT/some/path/` | stored as
`http://localhost/localhost/` by the credential-suffix stripping |
unchanged (see Fix) |

</details>

---

## Also folded in: audit — redact secrets in the registry URLs printed
by `bun audit` / `bun audit fix` (from #38844)

#### Problem
- With a registry URL that carries a secret, `bun audit` and `bun audit
fix` print it. With
`npm_config_registry=http://alice:s3cret@127.0.0.1:PORT/` and a registry
answering 404, stderr is `error: POST
http://alice:s3cret@127.0.0.1:PORT/-/npm/v1/security/advisories/bulk -
404`. A token in the registry path (`http://host/npm_.../`) is printed
the same way, and reaches these lines from every config source,
including `.npmrc`.
- Four outputs format the registry href verbatim (`BStr::new`), where
the rest of the package manager formats such URLs with
`bun_core::fmt::redacted_npm_url` (`Npm::response_error` in
`src/install/npm.rs`, the verbose request trace in `src/http/lib.rs`,
`bun pm whoami`):
- `src/runtime/cli/audit_command.rs` `send_audit_request`: the `POST
<url> - <status or error>` line (the repro above).
- `src/runtime/cli/audit_command.rs` `report_non_json_response`:
`<registry> returned a non-JSON audit response`, reached from the
report, `--json` and `audit fix` paths.
- `src/install/audit_fix.rs` `print_unaudited`: `warn: <registry> did
not answer the audit request (<reason>); skipped <packages>`, printed by
both commands for a scoped registry that failed.
- `src/install/audit_fix/json.rs`: the `registry` field of each
`unaudited` entry in `bun audit fix --json`.
- The href is `scope.url.href()` as configured
(`AuditRegistry::from_scope`; `unaudited()` copies it into the
`UnauditedRegistry` record behind the last two outputs). `.npmrc` and
bunfig registry strings move `user:password@` out of it while loading,
the bunfig object form, the registry env vars and `--registry` keep it
(#38796 and #38834 change the latter two), and a token in the path stays
in it in every case.
- The 1.3.x binaries print `audit request failed (status N)` without a
URL; the URL in these lines came with the audit rewrite in #38333, so
this has not shipped in a release.

#### Fix
- The `POST` line and `report_non_json_response` format the URL with
`redacted_npm_url`: these quote the request URL, so they get the same
masked form as the manifest and tarball error lines
(`http://alice:******@host/...`, path token as `***`).
`AuditRegistry.href` itself stays raw because it is also what the
request is sent to.
- The skipped-registry record names a registry rather than quoting a
request, so `unaudited()` builds it from `href_without_auth()` (trailing
slash stripped), the same form `bun publish` prints as its registry:
credentials written into the URL are left out instead of masked, and the
record reads the same whichever config source the scope came from
(`http://host:PORT`, matching what `.npmrc` scopes already produced).
Its two emitters, the warning and the `--json` field, format it with
`redacted_npm_url` for tokens in the path (`http://host:PORT/***`); the
`--json` value is rendered into a buffer first because the JSON string
writer takes bytes. The `unaudited` array is new in #38333, so nothing
depends on the raw form.
- For a URL without a password, UUID or `npm_` token the output is byte
for byte unchanged; the 177 existing tests in `bun-audit.test.ts`, many
of which assert these exact lines with plain URLs, still pass.
- Not touched, same class elsewhere: `bun audit fix`'s `was not checked
for updates` lines repeat the install log's `GET <url> - <status>` text,
which #38817 redacts at its source; the registry URL lines in
`src/install/NetworkTask.rs` (`Failed to join registry ...`, `... is not
on registry ...`) and `src/install/pnpm.rs` (`fetching pnpm registry ...
from <url>`) are install-side and have been filed separately. This PR
and #38817 touch disjoint files.
- Tests: `test/cli/install/bun-audit.test.ts`, new `bun audit with a
secret in the registry URL` block, one test per output: the `POST` line,
the non-JSON line from the response check, the non-JSON line from the
parse step in report, `--json` and `fix` mode, the skipped-registry
warning and `unaudited[].registry` with a token in the registry path
(`.npmrc` scope), and the same two outputs with `user:password@` in the
URL (bunfig object-form scope, which keeps it), asserting the
credential-free form. The path-token cases use a token rather than a
password because a token reaches the audit command from every config
source; the credential case asserts the stripped form, which stays true
once #38796 / #38834 strip credentials earlier, so this PR does not
depend on their landing order. All five fail on this branch with `src/`
stashed (the token or password is printed); the credential case also
fails with only the `unaudited()` hunk removed (it then prints
`alice:******@`), and the `--json` case with only the `json.rs` hunk
removed; all 182 tests in the file pass with the change.
- Also ran `cargo clippy -p bun_install -p bun_runtime`, `cargo fmt
--check` on both crates, and `test/internal/source-lints`.

#### Background
- `bun audit` POSTs the lockfile's package versions to
`<registry>/-/npm/v1/security/advisories/bulk`. Packages whose scope
(`@foo/*`) is configured with its own registry are sent to that registry
instead; when a non-default registry fails to answer (HTTP error,
connection error, non-JSON body) its packages are reported as skipped
(one `UnauditedRegistry` record per registry, printed as the warning
and, by `audit fix --json`, as the `unaudited` entries) rather than
failing the command, while a failure from the default registry fails the
command with the `POST` or non-JSON line.
- `redacted_npm_url` (`src/bun_core/fmt.rs`) is a `Display` adapter over
URL bytes: the password of `scheme://user:password@host` is written as
one `*` per byte (the per-byte form is shared with the config-excerpt
redactor, which needs column alignment), and any UUID or `npm_`/`npms_`
token anywhere in the string as `***`; everything else is written
through unchanged. `Output::err_generic`, `warn!` and `pretty_errorln!`
take any `Display` argument, so it is a drop-in replacement for
`BStr::new` (`err_generic`'s `{s}`/`{f}` placeholder letters are
cosmetic; `{f}` is what the other `redacted_npm_url` call site uses).
- `URL::href_without_auth()` (`src/url/lib.rs`) rebuilds
`scheme://host[:port]/path/` from the parsed URL, dropping any userinfo;
it is what the config loaders use to store a registry URL whose
credentials were split out, and what `bun publish` prints as
`Registry:`. It currently yields `http://host//` for a root-path
registry (#38812 changes that to one slash); `unaudited()` strips
trailing slashes afterwards, so the record is `http://host:PORT` either
way. Tokens that are part of the path survive it, which is why the
record's emitters still go through `redacted_npm_url`.

<details>
<summary>Before / after on a debug build (404 registry, non-JSON
registry, scoped registry with a path token, scoped registry configured
as <code>{ url = "http://alice:s3cret@..." }</code>)</summary>

Before:

```
error: POST http://alice:s3cret@127.0.0.1:PORT/npm_a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8/-/npm/v1/security/advisories/bulk - 404
error: http://alice:s3cret@127.0.0.1:PORT/npm_a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8 returned a non-JSON audit response
warn: http://127.0.0.1:PORT/npm_a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8 did not answer the audit request (404); skipped @foo/bar
{"dryRun":false,...,"unaudited":[{"registry":"http://127.0.0.1:PORT/npm_a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8","packages":["@foo/bar"],"reason":"404"}],...}
warn: http://alice:s3cret@localhost:PORT did not answer the audit request (404); skipped @foo/bar
{"dryRun":false,...,"unaudited":[{"registry":"http://alice:s3cret@localhost:PORT","packages":["@foo/bar"],"reason":"404"}],...}
```

After:

```
error: POST http://alice:******@127.0.0.1:PORT/***/-/npm/v1/security/advisories/bulk - 404
error: http://alice:******@127.0.0.1:PORT/*** returned a non-JSON audit response
warn: http://127.0.0.1:PORT/*** did not answer the audit request (404); skipped @foo/bar
{"dryRun":false,...,"unaudited":[{"registry":"http://127.0.0.1:PORT/***","packages":["@foo/bar"],"reason":"404"}],...}
warn: http://localhost:PORT did not answer the audit request (404); skipped @foo/bar
{"dryRun":false,...,"unaudited":[{"registry":"http://localhost:PORT","packages":["@foo/bar"],"reason":"404"}],...}
```

</details>

<details>
<summary>Earlier revision of this PR</summary>

The first revision applied `redacted_npm_url` to the `UnauditedRegistry`
record as well, so for the config sources that keep credentials in the
URL the warning and the `--json` field came out as
`http://alice:******@host` (username and password length, and a
different shape from the `.npmrc` case, which had no userinfo to begin
with). Review pointed out that every other place bun emits a registry
URL as data drops the credentials instead; the record is now built with
`href_without_auth()` and the per-byte masking is confined to the two
lines that quote the request URL.

</details>

Closes #38844

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 0 · 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

<!-- robobun:evidence:end -->

---------

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Main's #38333 split .npmrc credential application in two: load_npmrc_config
now returns the credentials it read, and apply_registry_auth applies them to
the registries bunfig.toml declares, skipping any that bunfig already gave
credentials (project config beats .npmrc). This branch keeps that contract but
resolves with npm's key walk: load_npmrc_config returns the collapsed
Vec<ConfigItem>, and apply_registry_auth walks it for each credential-less
bunfig registry via the same apply_to_registry used for .npmrc's own
registries. RegistryAuth/RegistryCredential/parse_auth (exact-match, decoded
_auth) are superseded and removed; ConfigItem carries both main's optname_loc
and this branch's source_idx, and derives Clone instead of dupe().

NpmRegistry::has_credentials also counts the new auth field. The tests that
had .npmrc _auth / a lone username override bunfig.toml credentials now
exercise the same layering against credentials stored from the registry URL's
userinfo (the case that still applies), and new cases pin both directions of
the bunfig rule under the walk. The IPv6 key tests from #38828 are kept and
extended with a host-root key and an address ending in the default port
digits. add_warning_fmt_opts_with_note follows add_formatted_msg's new arity;
audit/publish keep the byte-exact Authorization appends over main's lossy
Display, now reading AuditRegistry from #38183.
main's byte-search source lint rejects the libcore scalar form.
Comment thread src/ini/lib.rs Outdated
Comment thread src/ini/lib.rs Outdated
The "matches no registry" warning and the empty _auth error were computed
over the registries .npmrc itself declares, so a key aimed at a registry that
only bunfig.toml declares was dropped silently, which is the failure mode the
warning exists to report. load_npmrc_config now also reads bunfig's install
config and adds every registry apply_registry_auth will later resolve (the
ones bunfig gave no credentials) to the set the diagnostics run over; the
selection rule lives in one place, bunfig_registry_url, so the two passes
cannot drift. Resolution itself is unchanged and still split the way #38333
arranged it.

While restructuring: the registry key is a small struct instead of a tuple,
apply_conf_item decodes _Password inline (dupe_value_decoded's _Auth arm was
unreachable and contradicted the verbatim forwarding), the now-infallible
apply path drops its OOM plumbing, and normalize_conf_key only treats the
colon after a closing bracket as a port so a bracketed address ending in the
default port's digits is not mangled when respelled.
Comment thread src/ini/lib.rs Outdated
Comment thread src/install/npm.rs Outdated
A home-file `_auth=` that a later file overrides with a value supplies
nothing either way, and the credential is sent, so reporting it as an error
was wrong; the check now requires the line to be the one lookup() returns for
its key. Two cases pin both directions, alongside the two existing two-file
cases, which now share a helper.

Scope::from_api: the two auth.is_empty() checks were always true, since an
embedded _auth is terminal and the .npmrc _auth branch breaks; drop them and
note the invariant where it is established.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Private GitLab npm registry: scoped package resolution fails (401/404) since 1.3.x

2 participants