url: emit a single slash in href_without_auth() for root-path registry URLs - #38812
url: emit a single slash in href_without_auth() for root-path registry URLs#38812robobun wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesRegistry URL normalization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 0ae2bd4 has some failures in 🧪 To try this PR locally: bunx bun-pr 38812That installs a local version of the PR into your bun-38812 --bun |
|
Status: fix pushed (rebased on main after #38183 merged), waiting on CI. Reproduced on bun 1.4.0 with a The new tests in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a 4-line guard so href_without_auth() emits http://host/ instead of http://host// for root-path registry URLs, with thorough test coverage. The inline nit about the undrained stdout pipe is a harness-convention consistency point, not a blocker (a failing single-dep install writes far under 64KB to stdout).
Extended reasoning...
Overview
The Rust change in src/url/lib.rs is 4 lines: href_without_auth() previously wrote / + trimmed-path + / unconditionally, so a root pathname (which trims to "") produced a trailing //. The fix wraps the path + second / in if !path.is_empty(), so the result always ends in exactly one slash. The doc comment is updated to match. The non-empty-path branch is byte-identical to before.
The test change adds a describe.concurrent block to test/cli/install/npmrc.test.ts with 8 parametrized loadNpmrc assertions (user:pass@ and :token@ forms, root vs. path, with/without port and trailing slash) and 3 integration tests that spawn bun install against a local 401 mock server, asserting the printed GET line, the request path the server received, and the Authorization header.
Security risks
None. This is display/serialization of the stored registry href. The actual HTTP request path was already correct (URL::parse collapses leading // in the pathname). Credentials handling is unchanged — the tests confirm the Basic/Bearer header is still sent. No new user input reaches this code path.
Level of scrutiny
Low-to-medium. The Rust change is mechanically obvious and touches a single serialization helper. I verified all four call sites of href_without_auth():
src/api/lib.rs:47,52andsrc/install/npm.rs:503— these store the result as the registry URL; they are the intended fix targets.src/runtime/cli/publish_command.rs:895,1500— both pass the result throughstrings::without_trailing_slash, which loops off all trailing slashes, so these sites produce identical output before and after.
The PR description's consumer audit (url_hash, tarball build_url, manifest cache header, url_is_under_registry) and the acknowledged one-time cache-folder-hash side effect for >32-byte hostnames are consistent with what I see.
Other factors
The one finding from the bug-hunting run is a nit: the new install() helper pipes stdout but only awaits stderr + exited. This is a real harness-convention inconsistency (neighboring helpers in the same file drain both), but a failing bun install for one dependency writes ~dozens of bytes to stdout, so no deadlock risk in practice. It doesn't block approval. Test coverage is strong — the test.each matrix plus the three end-to-end cases cover both entry points (.npmrc userinfo and bunfig.toml _authToken= suffix) and include a path-form negative case to guard against regression.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the earlier nit (undrained stdout pipe in the install() helper) is addressed in aa47e37, and the 4-line href_without_auth() change is straightforward.
What was reviewed:
- Traced all four
href_without_auth()callers (src/api/lib.rs,src/install/npm.rs, bothpublish_command.rssites) — the publish sites strip trailing slashes before use, so output is unchanged there. - New tests are hermetic (
port: 0, localBun.serve, per-dir cache), drain both pipes, and cover root/path/port and both.npmrcuserinfo +bunfig.toml_authToken=entry points. - The comment-cop bot flag on
lib.rs:395targets a doc comment stating the function's output contract, not a workaround justification — looks like a false positive.
Extended reasoning...
Overview
Two files: a 4-line change in src/url/lib.rs (href_without_auth(): skip appending path + '/' when the trimmed pathname is empty, so a root-path registry serializes as http://host/ instead of http://host//) plus a 2-line doc-comment update, and a new describe.concurrent block in test/cli/install/npmrc.test.ts with 8 test.each parse-only cases and 3 subprocess integration tests against a local 401 mock registry.
Security risks
None. The function already stripped user:password from the URL; this change only affects how many trailing slashes appear when the pathname is root. No new user input is parsed, no auth handling changes — the tests confirm the same Authorization header is still sent.
Level of scrutiny
Low. The Rust change is a trivial conditional guard on an already-computed value; the invariant ("result always ends in exactly one slash") is easy to verify by inspection. Vec::with_capacity now over-reserves by 1 byte in the empty-path case, which is harmless. I checked all four call sites: src/api/lib.rs:47,52 and src/install/npm.rs:503 store the href as the registry URL (the fix target), and both publish_command.rs sites call without_trailing_slash on the result first, so their output is unchanged.
Other factors
- My prior inline nit (piped stdout not drained) was fixed in commit aa47e37 — the helper now reads
proc.stdout.text()in the samePromise.all. - The github-actions comment-cop bot flagged the doc comment at
lib.rs:395. That comment is a 3-line doc string describing the function's output format ("…so the result always ends in exactly one slash"), not an inline justification for a workaround; the code itself is the fix, not a workaround. I read this as a heuristic false positive rather than an actionable review request. - Tests follow harness conventions:
tempDir,bunEnvspread,port: 0,await usingfor server/proc, exit-code asserted last, no external network, per-testBUN_INSTALL_CACHE_DIR. The PR description confirms 7/11 new tests fail on 1.4.0 (the//cases) and all pass with the change.
There was a problem hiding this comment.
LGTM — the earlier nit (drain stdout in the install() helper) and the comment-cop feedback (doc comment is now one line) are both addressed.
Checked: href_without_auth() now emits exactly one trailing / when the trimmed path is empty; verified all four call sites (src/api/lib.rs, src/install/npm.rs, and both in publish_command.rs) — the two publish sites already without_trailing_slash() the result, so behavior there is unchanged. Tests cover the user:pass@ and :token@ forms with and without a port/path via loadNpmrc, plus end-to-end .npmrc and bunfig.toml installs against a local 401 server asserting the printed URL, request path, and that credentials are still sent.
Extended reasoning...
Overview
The PR fixes URL::href_without_auth() in src/url/lib.rs so a root-path registry URL serializes as http://host/ instead of http://host//. The Rust change is 4 lines: wrap the trailing path + / append in if !path.is_empty(). The doc comment is condensed to one line. test/cli/install/npmrc.test.ts gains a describe.concurrent block with 8 loadNpmrc table cases and 3 subprocess tests running bun install against a local 401-answering Bun.serve mock.
Security risks
None. The function's purpose is to strip userinfo from a registry URL; the change only affects how many trailing slashes appear when the pathname is root. Credentials are still split off correctly (the new tests assert default_registry_username/password/token and the Authorization header the mock server receives). No auth, crypto, or permission code touched.
Level of scrutiny
Low. The Rust change is a trivially correct guard on an empty string. I checked every caller of href_without_auth() (grep across the repo): src/api/lib.rs:47,52 and src/install/npm.rs:503 store the result as the registry href (the paths the tests exercise); both publish_command.rs sites strip trailing slashes before use, so their output is unchanged. The one documented side effect — the @@<host>__<hash> cache folder name changing for >32-byte hostnames with embedded credentials — is benign (fresh cache once). The bug was cosmetic to begin with: the actual HTTP request already collapsed the //.
Other factors
Since my prior review, three follow-up commits landed: aa47e377 added proc.stdout.text() to the Promise.all in the install() helper (my nit), and 61a496b4 + 432adca1 shortened the href_without_auth doc comment to a single line (the comment-cop bot's feedback). The tests follow harness conventions: tempDir, bunEnv spread, port: 0, await using for server/proc, all pipes drained, test.each for the matrix, exit code asserted last. The PR description enumerates every consumer of the stored href and confirms which ones strip trailing slashes; it also names the adjacent test files that were re-run with the fix. Straightforward, well-tested, and all prior feedback resolved.
There was a problem hiding this comment.
LGTM — the earlier nit (undrained stdout: "pipe") and the comment-cop warnings are both addressed.
What was reviewed:
href_without_auth(): skipping thepath + '/'append when the trimmed path is empty; non-empty-path output is byte-identical.- All four callers (
api/lib.rs,install/npm.rs, and the two inpublish_command.rs): the publish sites alreadywithout_trailing_slashthe result, so only the two store-verbatim sites see a change, which is the intended fix. - New tests: cover root/port/path variants for both the
.npmrcuserinfo form and the bunfig_authToken=suffix form;run()now drains stdout.
Extended reasoning...
Overview
Two files: a 4-line change to URL::href_without_auth() in src/url/lib.rs (skip appending the trimmed path + trailing slash when the path trims to empty, so root-path registry URLs serialize as http://host/ instead of http://host//), plus a shortened doc comment; and a new describe.concurrent block in test/cli/install/npmrc.test.ts with 11 assertions covering the .npmrc user:pass@ / :token@ forms via loadNpmrc, the bunfig _authToken= suffix form via bun pm whoami, and the end-to-end bun install GET line + Authorization header.
Security risks
None. The function only reformats an already-parsed registry URL string when credentials are being stripped out; no new parsing, no auth logic changes. The tests confirm the credential (Bearer tok, Basic ...) is still sent unchanged.
Level of scrutiny
Low-to-medium. The Rust change is a trivially-inspectable conditional guard around two existing lines; the non-empty-path branch produces byte-identical output to before. I checked all four call sites: src/api/lib.rs:47,52 and src/install/npm.rs:503 store the result verbatim as the registry href (the sites the fix targets), while both publish_command.rs uses pass through strings::without_trailing_slash first and are therefore unaffected either way. The Vec::with_capacity hint is now over by 1 byte in the empty-path case, which is harmless.
Other factors
My prior review left one nit (the run() helper piped stdout without draining it) — fixed in aa47e37. The comment-cop bot flagged the multi-line doc comment three times — reduced to one line in 432adca. All inline threads are resolved. Tests follow harness conventions (tempDir, bunEnv spread, port: 0, await using for server/proc, three-way Promise.all draining both pipes, exit-code asserted last, test.each for the matrix, describe.concurrent for independent subprocess tests). The PR description documents that 7/11 new tests fail on 1.4.0 and pass with the change, and that the surrounding suites (redacted-config-logs, config-precedence, whoami, publish) still pass.
There was a problem hiding this comment.
LGTM — the empty-path guard in href_without_auth() is a minimal, obviously-correct fix, and the earlier nit (undrained stdout) and comment-cop feedback have both been addressed.
What was reviewed:
- Traced all four
href_without_auth()call sites (src/api/lib.rs,src/install/npm.rs, two inpublish_command.rs) — non-empty-path output is byte-identical, empty-path drops from//to/; the publish callers either append their own/or strip trailing slashes, so no regression there. - New tests cover both credential forms (
user:pass@and:token@), root vs. path vs. port variants, and assert both the stored href and the actual request path/auth header via a local mock registry. - Confirmed the
run()helper now drains stdout in the samePromise.all, and the doc comment is down to one line.
Extended reasoning...
Overview
Two files touched: a 4-line guard in src/url/lib.rs::URL::href_without_auth() that skips appending <path>/ when the trimmed pathname is empty (so a root-path registry serializes as http://host/ instead of http://host//), plus a new describe.concurrent block in test/cli/install/npmrc.test.ts with 11 cases covering loadNpmrc, bun pm whoami, and bun install against a local mock registry.
Security risks
None. href_without_auth() exists to strip userinfo from a configured registry URL; this change only affects how many trailing slashes the credential-free result carries when the path is empty. Credential extraction (url.username/url.password) is unchanged, and the new tests assert the extracted username/password/token and the Authorization header sent on the wire.
Level of scrutiny
Low-to-medium. The Rust change is a trivial conditional whose non-empty-path branch is byte-for-byte identical to the old unconditional code; only the empty-path case changes, and it now matches what the same registry stores when no credentials need stripping. I grepped for all call sites: the two the description names plus two in publish_command.rs — one wraps the result in without_trailing_slash (unaffected), the other appends its own / in a format string (a pre-existing quirk this PR strictly improves for the root case and leaves unchanged otherwise). The one persisted side effect (a fresh @@<host>__<hash> cache dir for >32-byte hostnames configured with URL-embedded credentials) is called out in the description and is benign.
Other factors
All prior review feedback is resolved: my stdout-drain nit is fixed (the run() helper now does Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])), and the comment-cop complaints were addressed by shrinking the doc comment to one line. The tests follow harness conventions (tempDir, port: 0, await using, test.each, describe.concurrent, per-test cache dir), stay offline, and assert exact values. The PR description enumerates related open PRs and confirms none touches this file.
…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>
… 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>
href_without_auth() appended "/" + trimmed pathname + "/" unconditionally, so a registry URL whose credentials were stripped and whose path was "/" was stored as "http://host//". Manifest URLs are joined onto that href, so every "GET <url> - <status>" line for such a registry printed a double slash. Skip the path and its trailing slash when the trimmed path is empty.
The loadNpmrc rows and the whoami cases read the stored href directly, so they keep exercising href_without_auth() even once the manifest join normalizes its base on its own. One install case keeps pinning the GET line.
b2aa825 to
0ae2bd4
Compare
There was a problem hiding this comment.
LGTM — the earlier nit (drain stdout) and the comment-cop feedback are both addressed.
What was reviewed:
- The
href_without_authchange: skipping the{path}/suffix when the trimmed path is empty produces exactly one trailing slash; the capacity hint over-reserves by one byte in that case, which is harmless. - All 6 call sites:
options_types/schema.rsandinstall/npm.rsstore the result (the fix);audit_command.rsandpublish_command.rs:1501pass it throughwithout_trailing_slash(loops, so both//and/reduce to the same string — no change);publish_command.rs:896is display-only. - Tests follow the file's harness conventions (
test.each,describe.concurrent,tempDir,port: 0, localBun.servemock, all pipes drained, exit code asserted last).
Extended reasoning...
Overview
Two files. src/url/lib.rs changes URL::href_without_auth() to skip appending {path}/ when the trimmed pathname is empty, so a root-path registry serializes as http://host/ instead of http://host//. The 6-line doc comment is replaced with a one-liner (per the comment-cop bot). test/cli/install/npmrc.test.ts gains a describe.concurrent block with 11 cases: 8 loadNpmrc rows asserting the stored default_registry_url for the user:pass@ and :token@ forms across root/port/path variants, 2 bun pm whoami cases exercising the bunfig _authToken= suffix form via a local Bun.serve mock, and 1 bun install case pinning the printed GET line and the Basic header sent.
Security risks
None. The helper strips userinfo from a URL string used for display and as a request base; credentials are removed before this code runs and are stored separately. The change only affects how many trailing slashes the auth-free href has. The tests spin up local Bun.serve on 127.0.0.1:0 and never touch the network.
Level of scrutiny
Low-to-medium. The Rust change is 4 effective lines with an obvious invariant ("emit exactly one trailing slash"). I checked every caller (rg href_without_auth): the two config-loading sites store the result verbatim (this is what the PR fixes); audit_command.rs:422 and publish_command.rs:1501 immediately feed it through strings::without_trailing_slash, which loops over all trailing slashes, so http://host// and http://host/ both reduce to http://host — no behavior change; publish_command.rs:896 prints Registry: {}/ for display, which is cosmetic and only improved. Nothing depends on the double slash.
Other factors
My earlier nit about the undrained stdout pipe was fixed in aa47e37 — the run() helper now does the three-way Promise.all. The comment-cop feedback on the multi-line doc comment was addressed. All inline threads are resolved. The tests match the surrounding file's conventions (harness tempDir/bunEnv, await using for server and process, port: 0, stderr asserted before exit code) and the loadNpmrc rows are pure — registry.example.com is never contacted.
1 participant
Problem
.npmrcregistry=http://alice:s3cret@127.0.0.1:PORT/, the same string inbunfig.toml, or the documentedurl = "http://host/_authToken=TOKEN"suffix form) is stored ashttp://127.0.0.1:PORT//. The same registry configured without credentials is stored as written, with one slash.bun pm whoami(failed to authenticate with registry 'http://127.0.0.1:PORT//') and quoted in thefor_manifestdiagnostics (src/install/NetworkTask.rs:480-554), and everything that builds a request from it inherits the extra slash: today the manifest URL (error: GET http://127.0.0.1:PORT//somepkg - 401) and the tarball URLs synthesized by the package-lock.json migration (http://127.0.0.1:PORT//foo/-/foo-1.0.0.tgz, written intobun.lock).bun_url::URL::parse, which collapses a leading//in the pathname, so the server still receivesGET /somepkg. Found by inspection while working on install: send credentials embedded in --registry and registry env var URLs #38796 (which leaves this helper alone), not from a user report.URL::href_without_auth()(src/url/lib.rs) writes<proto>://<host>+/+trim(pathname, "/")+/unconditionally, so a root pathname (trimmed to an empty string) yields//. Registries with a path (/npm/) come out right. The Zig implementation used the same format string, so this is not a regression.Fix
/. The result now always ends in exactly one slash:http://host/,http://host:8080/,http://host/npm/.http://host/is what the same registry is stored as when nothing needs stripping, and it is the only producer of the//form: both credential-stripping callers (src/api/lib.rsfor.npmrc/bunfig.tomlstrings,Scope::from_apiinsrc/install/npm.rsfor the_authToken=/_auth=/:username=/:_password=suffixes) store its return value, and install: send credentials embedded in --registry and registry env var URLs #38796 still calls it after moving the first caller.url_hash, tarballbuild_url, the manifest cache header,url_is_under_registry,bun publish,bun pm view,bun audit) already strip trailing slashes before use. The one persisted effect is the@@<host>__<hash>cache folder name, used for hostnames longer than 32 bytes, which hashes the href: such a registry configured with credentials in its URL gets a fresh cache folder once. (install: preserve last registry path segment when URL has no trailing slash #36294 and install: canonical registry URL in Scope; redact secrets in bun audit registry URLs #38183 deliberately leave the stored href as configured; this change rewrites only the value this helper already rewrites.)//(new URL("http://h//").hrefishttp://h//), so the 7 failing tests below still fail on current main without this change. The open install: preserve last registry path segment when URL has no trailing slash #36294 normalizes the join base infor_manifestand Fix package-lock.json migration for scope registries configured without a trailing slash #38698 fixes the migration concatenation, so each of those removes the//from the request it builds regardless of what is stored; neither changes the stored value,whoami, or the quoted diagnostics, which is what this change fixes. None of them touchessrc/url/lib.rs; this composes with all of them. If the stored href should get a trailing-slash policy of its own,Scope::set_urlfrom install: canonical registry URL in Scope; redact secrets in bun audit registry URLs #38183 is the place, as a follow-up.test/cli/install/npmrc.test.ts, newregistry URL with embedded credentialsblock. TheloadNpmrcrows assert the storeddefault_registry_urlfor theuser:pass@and:token@forms (root path with and without a trailing slash or port, plus path forms that must stay unchanged); thebun pm whoamicases assert the stored href thatScope::from_apiproduces for the_authToken=suffix form (root and/npm/) and that the token is still sent. Both assert the stored value directly, so they keep exercising this change once install: preserve last registry path segment when URL has no trailing slash #36294 lands. Thebun installcase pins today'sGETline for the.npmrcuserinfo form and checks the request path andBasicheader. 7 of the 11 new tests fail on bun 1.4.0 and on a debug build of current main (//), all pass with this change.npmrc.test.ts,redacted-config-logs.test.ts,config-precedence.test.ts, thewhoamitests inbun-install-registry.test.ts, and the twobun-publish.test.tstests that print or embed the registry href.Background
bun_url::URLis bun's lenient zero-copy URL scanner: each field is a slice of the input, andpathnamedefaults to/when the input has no path.href_without_auth()re-serializes such a URL without itsuser:pass@part and is only used when bun removes credentials from a configured registry URL; the resulting string becomes the registry's stored href (Scope.url).NetworkTask::for_manifestbuilds manifest URLs withbun_url::join, the WHATWG parser. Likenew URL("pkg", "http://host//"), it keeps the double slash, which is how the stored value reached theGETline.Output on bun 1.4.0 (mock registry answering 401)
In every install case the mock server logged
GET /somepkg; whoami requested/-/whoamiwithBearer abc.