install: send credentials embedded in --registry and registry env var URLs - #38796
Merged
Conversation
… URLs A registry URL of the form http://user:pass@host/ (or http://:token@host/) written in .npmrc or bunfig.toml is split into credentials and a bare URL by the config parsers, so requests carry Authorization. The same URL given through --registry or BUN_CONFIG_REGISTRY / NPM_CONFIG_REGISTRY / npm_config_registry skipped that step: the userinfo stayed inside the stored registry URL, where the HTTP client ignores it, so install, pm view and publish sent no Authorization header (publish still proceeded because its pre-flight check accepted the userinfo as credentials), and the credentials were echoed in install error output. Move the splitter onto the config type as NpmRegistry::from_url and run the flag and env var URLs through it. Credentials found in the URL replace the ones configured for the registry, matching how a registry= line with userinfo beats host-keyed auth in .npmrc; a URL without credentials keeps the previous behavior. publish's pre-flight check now also accepts Scope.auth, which is where the Basic credentials land.
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
Comment |
Collaborator
Author
|
Status
|
Contributor
|
Found 2 issues this PR may fix:
Ruled out after checking the diff: #7416 (userinfo on a tarball dependency URL — this PR only touches the registry-config layer, not dependency URL resolution) and #34168 (
🤖 Generated with Claude Code |
Collaborator
Author
|
Not adding
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
Author
|
Updated 12:01 AM PT - Aug 15th, 2026
✅ @robobun, your commit 28f14893395478167d8022ba4abe780e2aef33a1 passed in 🧪 To try this PR locally: bunx bun-pr 38796That installs a local version of the PR into your bun-38796 --bun |
This was referenced Aug 15, 2026
Contributor
There was a problem hiding this comment.
I re-reviewed after the follow-up commits and didn't find any bugs — the earlier has_credentials dedup nit is addressed and the comment-cop items are all resolved. Because this changes how registry credentials are extracted and which credentials win when --registry/env-var userinfo meets a same-host .npmrc token, and it sits adjacent to #38764/#38183 which touch the same --registry block, a human sign-off on the precedence semantics would still be worthwhile.
What was reviewed:
NpmRegistry::from_urlis a byte-for-byte move of the existingparse_registry_url_string_implbody;bun_options_typesalready depends onbun_url, and the bunfig/.npmrc entry point now delegates so those paths are unchanged.- The no-credentials branches in both the env-var and
--registryblocks preserve prior behavior (same-host token carry / same-origin credential drop);from_urlreturns the input href unchanged there. - Credentials are stripped from the stored URL so they no longer reach error output or the
dist.tarballmanifest field — checked that the updatedredacted-config-logstest still coversredacted_npm_urlmasking via the new verbose-fetch case.
Extended reasoning...
Overview
Routes --registry and the three registry env vars through the same URL-userinfo splitter that .npmrc/bunfig string forms already use, by moving that splitter onto NpmRegistry::from_url in bun_options_types. Adds NpmRegistry::has_credentials() (and removes the duplicate free function in bun_ini), drops the now-unused bun_url dep from bun_api, and adds registry.auth.is_empty() to publish's pre-flight NeedAuth check. ~15 new tests across three files exercise Basic/Bearer for the flag and each env var, precedence over a same-host .npmrc _authToken, and that the password no longer appears in output.
Security risks
This is credential-handling code. The change makes bun send Authorization headers it was previously silently dropping, using credentials the user explicitly put in the URL — matching what bun already does for registry= in .npmrc and what npm does for the flag/env var. Stripping userinfo from the stored URL is a net improvement (removes it from error lines and the published dist.tarball). No cross-origin carry: when the URL has credentials the scope is rebuilt from that URL alone; when it doesn't, the existing same-host/same-origin gates are byte-identical. I did not spot a leak or downgrade path.
Level of scrutiny
High — auth-adjacent, and it encodes a precedence decision (URL userinfo replaces a same-host _authToken, mirroring apply_registry_auth) that a maintainer should confirm is the intended rule for the CLI/env layers too. The PR also notes two open PRs (#38764, #38183) touching the same --registry block; a human is better placed to sequence those.
Other factors
The from_url body is a verbatim move; bun_options_types already depended on bun_url. The env-var block's switch from prev_scope.clone().token to core::mem::take(&mut self.scope.token) is equivalent since self.scope is overwritten on the next line. The updated redacted-config-logs test replaces an assertion that depended on the old bug (masked userinfo in the 401 URL) with one that asserts Basic is sent and the URL has no userinfo, and adds a verbose-fetch test so redacted_npm_url masking stays covered. My earlier nit and the comment-cop flags are all resolved in the current diff.
This was referenced Aug 15, 2026
BUN_CONFIG_TOKEN / NPM_CONFIG_TOKEN were read before --registry ran, so the host-mismatch check that drops credentials configured for the previous registry also dropped the env token and the install failed with a 401. The env token is not tied to a host; apply it after --registry has picked the registry. It also takes precedence over a token embedded in the --registry URL, matching what a configured token does over URL userinfo in npm and what BUN_CONFIG_REGISTRY + BUN_CONFIG_TOKEN already do here.
Contributor
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues; my earlier has_credentials nit was addressed in 5a9e23d. Because this reorders credential precedence in Options::load (moving --registry ahead of the token env vars) and adopts the "URL credentials replace a same-host configured token" rule that the status comment explicitly flags for a maintainer to confirm, a human sign-off on that behavior choice is still warranted.
What was reviewed:
NpmRegistry::from_urlis a pure move of the existing splitter;parse_registry_url_string_impldelegates to it, so bunfig/.npmrc string forms are unchanged.- The env-var path's no-credentials branch preserves the prior same-host token carry-forward; the
--registryno-credentials branch keeps the same-origintoken/auth/userretention from #36165. cli.tokenstill applies after both new blocks, so--tokenprecedence is unchanged;Scope::from_apiis what turnsusername/passwordintoScope.auth, so the publishregistry.authcheck is the right gate.
Extended reasoning...
Overview
The PR routes --registry and the BUN_CONFIG_REGISTRY / NPM_CONFIG_REGISTRY / npm_config_registry env vars through the same URL-userinfo splitter that bunfig and .npmrc already use, by hoisting that splitter onto NpmRegistry::from_url in src/options_types/schema.rs. src/api/lib.rs::parse_registry_url_string_impl becomes a one-line delegate (dropping bun_api's bun_url dep), src/ini/lib.rs swaps its private has_credentials for the new method, src/install/PackageManager/PackageManagerOptions.rs reorders Options::load so --registry is applied before the token env vars, and src/runtime/cli/publish_command.rs adds registry.auth to the pre-flight check. Tests cover install, publish and pm view for both the Basic (user:pass@) and Bearer (:token@) forms across the flag and all three env vars, plus precedence against .npmrc tokens and BUN_CONFIG_TOKEN.
Security risks
This is credential-handling code. The change starts sending an Authorization header that was previously dropped, which is the intended fix and matches npm's behavior and bun's own .npmrc handling. The userinfo is stripped from the stored URL, so it stops appearing in error output and in the published dist.tarball (both covered by tests). I did not find a path where credentials leak to a host other than the one they were embedded in — the env-var branch rebuilds the scope via Scope::from_api from the split registry, and the --registry branch either does the same (credentials present) or keeps the existing same-origin retention logic (credentials absent). No new external inputs are trusted beyond what config-file registry strings already accept.
Level of scrutiny
High. Two independent reasons: (1) auth/credential precedence is security-adjacent and easy to get subtly wrong across the config-file / env-var / flag layering; (2) the PR reorders the --registry block relative to the token env vars in Options::load, which is a deliberate precedence change (folded in from #38764). The author's own status comment flags one precedence rule — URL-embedded credentials replace rather than merge with a same-host configured token — as needing maintainer confirmation. That is a product decision, not a mechanical fix.
Other factors
CI on 28f1489 passed (177/179 jobs; the two remaining were queued macOS shards, no failures in the touched areas). Test coverage is strong: each new behavior has a test that asserts the exact Authorization header received by a mock registry, verdaccio actually accepts the Basic credentials for @needs-auth/test-pkg, and the reordering is pinned by "BUN_CONFIG_TOKEN beats credentials embedded in the --registry URL" and "--registry drops the _authToken of the .npmrc registry but keeps BUN_CONFIG_TOKEN". All prior review threads (my has_credentials dedup, the comment-cop long-comment nits) are resolved. Given the explicit maintainer-decision flag on the replace-vs-merge precedence and the security-sensitive surface, deferring rather than shadow-approving.
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 15, 2026
…38824) ### Problem - `registry = { url = "http://alice:s3cret@host/" }` in bunfig.toml (and the same object shape under `[install.scopes]`) sends no `Authorization` header: the registry sees `authorization: null` on the manifest and the tarball request. The same URL written as the string form, `registry = "http://alice:s3cret@host/"`, sends `Authorization: Basic YWxpY2U6czNjcmV0` on both. `{ url = "http://:token@host/" }` drops the token the same way. - Because the credentials stay inside the stored URL, they are also printed when a request fails: `error: GET http://alice:s3cret@127.0.0.1:PORT/no-deps - 401`. - Cause: `parse_registry_object` in `src/bunfig/bunfig.rs` copies `url` into `NpmRegistry.url` verbatim. Only the string form runs the userinfo splitter (`bun_api::npm_registry::Parser::parse_registry_url_string_impl`, `src/api/lib.rs`). Nothing downstream reads userinfo out of `NpmRegistry.url`: `Scope::from_api` (`src/install/npm.rs`) builds the header from `token` / `username` / `password` only, and the HTTP client ignores userinfo in a request URL. ### Fix - `parse_registry_object` now runs `url` through the same splitter the string form uses (`parse_registry_url`, shared by both forms), so the URL's credentials land in the credential fields and the stored URL no longer carries them. The `match obj.get(b"url")` block is the fix; the rest of the diff renames the string-form helper to take bytes. - If the object has any `username` / `password` / `token` key, the credentials taken from the URL are dropped before the keys are applied. Objects with credential keys therefore produce exactly the `NpmRegistry` they produce today (the URL's userinfo was always ignored for them); the only configs whose behavior changes are the ones that sent nothing. This has to be all-or-nothing rather than per field: `from_api` sends a token in preference to a username/password pair, so merging per field would let a `:token@` in the URL outrank `username` / `password` keys written next to it, which works today. It is also the rule the `.npmrc` merge already uses for the next layer down (`apply_registry_auth` in `src/ini/lib.rs` only fills in `.npmrc` credentials when the registry has none). - Correct because `NpmRegistry` is read on the assumption that credentials live in its fields and `url` carries none: `Scope::from_api` only reads the fields, and the `.npmrc` merge decides whether to apply `//host/:_authToken=` lines by checking those fields. The string form establishes that shape at parse time; the object form was the one producer that did not. Splitting at the same point makes a given `url` text mean the same thing in both forms, including in the `.npmrc` precedence check (tested). - URLs without userinfo come back from the splitter byte for byte (it returns the input href unchanged in that case), so existing object-form configs, with or without a trailing slash, are unaffected. - Not changed here: `--registry` and the `*_config_registry` env vars (#38796), `registry = "$VAR"` where the variable's value carries credentials (the expansion happens later, in `from_api`; filed separately), and the `http://host//` spelling the splitter produces for root-path URLs (#38812, cosmetic, same for both forms). No docs change: the documented way to pass credentials in the object form stays the explicit keys; this only makes the `url` value behave like the string form's. - Verified: `test/cli/install/config-precedence.test.ts` gains three string/object pairs (`user:password` in the URL, `:token` in the URL, `user:password` in a scoped registry URL), a `.npmrc` precedence test (URL credentials in the object beat a `~/.npmrc` `_authToken` for the same host), and a three-row table pinning that credential keys beat the URL (`username`/`password` keys over `user:password` in the URL, `username`/`password` keys over `:token` in the URL, `token` key over `user:password` in the URL). Every test asserts the exact `Authorization` header seen on each request and that the auth-only package installs. On the released build the four url-only object tests fail (`authorization: null`, 401, password in the error line); the string rows and the keyed rows pass there, as they must, since that behavior is unchanged. The `:token` row fails against a per-field merge, which is the regression it guards. All 39 tests in the file pass with this branch. - Also run with the fix: the Registry URLs / scoped authentication / same-origin tarball tests of `bun-install.test.ts`, `npmrc.test.ts`, `bun-install-retry.test.ts`, `redacted-config-logs.test.ts`, `bun-install-pathname-trailing-slash.test.ts`, `bun-run-bunfig.test.ts`, `test/config/bunfig`, `cargo clippy -p bun_bunfig`, and the source lints. In `bun-publish.test.ts` the lifecycle tests hit the local 5 s default timeout both with and without this change (CI passes a longer `--timeout`); the rest of the file passes. ### Background - bunfig accepts a registry in two shapes: a string, `registry = "<url>"`, and an object, `registry = { url, username, password, token }`. Both `[install] registry` and each `[install.scopes]` entry go through `parse_registry`, so one fix covers both. - `api::NpmRegistry` (`src/options_types/schema.rs`) is the config-level record produced by the bunfig and `.npmrc` loaders: `url` plus `username` / `password` / `token`. After the loaders run, `.npmrc` credential lines are merged into it, and the package manager turns it into an `npm::registry::Scope`, which holds the final URL and the pre-computed `Authorization` value. `from_api` uses `token` when it is set and only otherwise builds Basic auth from `username` / `password`. - The userinfo splitter takes `scheme://user:pass@host/path` and returns an `NpmRegistry` with `username` / `password` set (or `token`, when the user part is empty) and `url` rewritten without the userinfo. The bunfig string form and the `.npmrc` `registry=` / `@scope:registry=` keys all use it. <details> <summary>Probe: mock registry on port 0 logging the Authorization header, released 1.4.0 vs this branch</summary> ``` registry = "http://alice:s3cret@127.0.0.1:PORT/" (string form) 1.4.0: /no-deps Basic YWxpY2U6czNjcmV0 /no-deps/-/x-1.0.0.tgz Basic YWxpY2U6czNjcmV0 this branch: same registry = { url = "http://alice:s3cret@127.0.0.1:PORT/" } 1.4.0: /no-deps null /no-deps/-/x-1.0.0.tgz null this branch: /no-deps Basic YWxpY2U6czNjcmV0 /no-deps/-/x-1.0.0.tgz Basic YWxpY2U6czNjcmV0 registry = { url = "http://:tok123@127.0.0.1:PORT/" } 1.4.0: null on both this branch: Bearer tok123 on both registry = { url = "http://alice:s3cret@127.0.0.1:PORT/", token = "explicit" } 1.4.0: Bearer explicit on both this branch: same registry = { url = "http://alice:s3cret@127.0.0.1:PORT/", password = "override" } 1.4.0: null on both (no username configured by keys) this branch: same [install.scopes] myorg = { url = "http://bob:hunter2@127.0.0.1:PORT/" } 1.4.0: null on both this branch: Basic Ym9iOmh1bnRlcjI= on both (same as the scoped string form) 401 from the registry, object form: 1.4.0: error: GET http://alice:s3cret@127.0.0.1:PORT/no-deps - 401 this branch: error: GET http://127.0.0.1:PORT//no-deps - 401 (the // is #38812) ``` </details> <details> <summary>Superseded first version</summary> The first push merged the URL's credentials with the keys field by field (a `password` key on top of `user:pass@` in the URL gave `user:<key>`). Review pointed out that `{ url = "http://:t@host/", username = "u", password = "p" }`, which sends `Basic u:p` today, would have started sending `Bearer t`, because the URL's token survived next to the keys and `from_api` prefers a token. The current version drops the URL's credentials whenever a credential key is present, and the keyed rows in the test table pin that. </details>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
http://user:pass@host/, orhttp://:token@host/) passed as--registry, or throughBUN_CONFIG_REGISTRY/NPM_CONFIG_REGISTRY/npm_config_registry, sends noAuthorizationheader. Against a registry that logs the header (bun 1.4.0, alsomain):bun publish --registry http://alice:s3cret@127.0.0.1:PORT/publishes with exit 0 and the registry seesPUT /ui-pkg authorization=null; same withnpm_config_registry=....bun pm view somepkg --registry http://alice:s3cret@...andbun install --registry http://alice:s3cret@...:GET /somepkg authorization=null.registry=in.npmrcsendsAuthorization: Basic YWxpY2U6czNjcmV0on the manifest and the tarball. npm 11 sends Basic for the flag and the env var too (minipass-fetchpasses URL userinfo as node'sauthoption).bun installprints them on failure:error: GET http://alice:s3cret@127.0.0.1:PORT/somepkg - 401.parse_registry_url_string_impl,src/api/lib.rs), which movesuser:passintoNpmRegistry.username/password(ortoken) and strips it from the URL. The flag and env var paths did not:src/install/PackageManager/PackageManagerOptions.rs, registry env vars: builtApi::NpmRegistry { url: <raw value> }and calledScope::from_api, which does not read URL userinfo.--registry: stored the raw href intoself.scope.urldirectly.src/runtime/cli/publish_command.rspublish(): the "missing authentication" check accepted userinfo in the URL as credentials, so publish went ahead and sent the PUT without a header. The HTTP client (src/http) never reads URL userinfo.BUN_CONFIG_TOKEN/NPM_CONFIG_TOKENwere applied before--registry, so the host-mismatch check in the--registryblock dropped them too andBUN_CONFIG_TOKEN=... bun install --registry <other host>failed with a 401.Fix
NpmRegistry::from_url(src/options_types/schema.rs) is the splitter, moved next to the type sobun_installcan call it;bun_api'sparse_registry_url_string_impl(the bunfig and.npmrcentry point) delegates to it, so there is one parse for every registry URL string.bun_apiloses its now unusedbun_urldependency, andbun_ini's privatehas_credentialshelper is replaced by the method the new call sites needed.--registryrun their value throughfrom_url:Scope::from_api, exactly as for a config file registry string, soScope.auth(Basic) orScope.token(Bearer) is populated and the stored URL no longer contains the userinfo. They replace whatever the config files set up for that registry, which is the rule.npmrcalready applies (aregistry=line with userinfo wins over//host/:_authToken=:apply_registry_authinsrc/ini/lib.rsskips registries that already have credentials).--registrykeeps same-origin credentials and drops them otherwise);from_urlreturns the input href unchanged in that case.--registryblock now runs before the token env vars are read (cdca4ff), so the env token survives a registry switch and layers on top of both overrides. Resulting order, lowest to highest: config files, credentials in the--registry/ registry env var URL,BUN_CONFIG_TOKEN/NPM_CONFIG_TOKEN,--token.publish()also acceptsScope.authin its pre-flight check, which is where the Basic credentials land. Same one-line hunk as publish: accept basic auth credentials from .npmrc and bunfig.toml #38776 (the config file side of that check, reported in Bun Publish doesn't support token based login (Azure Devops) #17531); it merges in either order and closing those reports is left to publish: accept basic auth credentials from .npmrc and bunfig.toml #38776.Scope::from_api: the loaders need the split result before a scope exists..npmrcloading decides whether host-keyed_authTokenentries apply by looking at the parsed registry's credentials, and the two override layers decide whether to carry configured credentials over the same way. So the parse-time split is required regardless, and this change wires in the two string entry points that were missing from it. Two ways a URL with userinfo can still reachfrom_apiunsplit remain: the bunfig object formregistry = { url = "http://u:p@host/" }(documented with separateusername/password/tokenfields) and aregistry = "$VAR"whose value carries userinfo, since$VARis only expanded insidefrom_api. Both need a split insidefrom_apiitself, which is tracked separately and kept out of this change because install: canonical registry URL in Scope; redact secrets in bun audit registry URLs #38183 is rewriting that function's tail; this is also why the publish check keeps its URL userinfo clause.test/cli/install/config-precedence.test.ts:bun installwith--registryand with each of the three env vars, user:pass form (Basic arrives on every request and verdaccio accepts it,@needs-auth/test-pkginstalls, password not in stderr) and:token@form (Bearer); URL credentials replace a same-host.npmrc_authToken(flag and env var); from cdca4ff:BUN_CONFIG_TOKENbeats a token in the--registryURL,BUN_CONFIG_TOKEN/NPM_CONFIG_TOKENapply to the--registryregistry, and--registrydrops the.npmrctoken for the previous host but keepsBUN_CONFIG_TOKEN.test/cli/install/bun-publish.test.ts,credentials in the registry url:--registryuser:pass (PUT carries Basic, summary line has no userinfo),--registry:token@(Bearer; previously "missing authentication"), the three env vars.test/cli/install/redacted-config-logs.test.ts: thepm viewtest that asserted the password is masked in the 401 line now asserts Basic is sent and the printed URL has no userinfo (the masked form depended on this bug); a verbosefetch()case keeps the URL password masking ofredacted_npm_urlcovered.bun-publish.test.ts,config-precedence.test.ts,redacted-config-logs.test.ts,npmrc.test.ts, the.envregistry override and env var priority tests inbun-install-registry.test.ts;cargo clippyon the touched crates; the source lints.from_urlwhile deletingbun_apiandschema.rs(whichever lands second rebases onto the other's copy; the call sites are the same either way); install: canonical registry URL in Scope; redact secrets in bun audit registry URLs #38183 touches the same--registryblock for URL normalization.Background
Api::NpmRegistryis the parsed config for one registry:urlplus optionalusername/password/token, produced by bunfig and.npmrcloading.Scope::from_api(src/install/npm.rs) turns it into aScope, the resolved registry the package manager uses: the URL,token(sent asAuthorization: Bearer) andauth(base64user:password, sent asAuthorization: Basic;userkeeps the plain pair forpm whoami). Requests only ever look atScope.token/Scope.auth; a URL's userinfo is never sent by the HTTP client.Options::loadbuilds the default scope in layers: config files, the registry env vars,--registry, the token env vars, then the remaining CLI flags (--token). The registry env vars and--registryare the two layers that accept a registry URL string without going through the config parsers, which is why they are the ones changed.URL::href_without_auth()is the existing helper the splitter uses to rebuild the URL without its userinfo. For a registry with no path it yieldshttp://host//; every consumer strips or collapses the extra slash (this pre-dates the change and applies to the.npmrcform today), so it is left alone here.Probe against a mock registry (released bun 1.4.0 vs this branch)