Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughRegistry configuration now extracts credentials embedded in environment-expanded registry URLs when no credentials are configured separately. Install and publish tests check authentication and confirm that credentials are omitted from error and command output. ChangesRegistry URL credential handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Expanded registry URLs now supply authentication while keeping embedded credentials out of reported URLs. Explicit credentials retain precedence. The change is mergeable subject to normal CI checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:30 PM PT - Sep 30th, 2026
❌ @robobun, your commit 0f909b3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38834That installs a local version of the PR into your bun-38834 --bun |
|
Status: ready for a maintainer. Rebased onto main bf42a52, the diff is one 9-line hunk in
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re #17531: that report (and #18670) is the publish pre-flight check, which this PR only touches because stripping the userinfo out of the stored URL would otherwise make an env var registry trip it. The hunk is the same line as #38776, which has the tests for those configurations, so I have linked the issues from the description as related rather than adding a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. The from_api change mirrors parse_registry_url_string_impl exactly and the publish NeedAuth guard update is the necessary consequence of stripping userinfo from Scope.url. Because this is credential-handling logic for bun install/bun publish and it overlaps with several open PRs (#38796, #38824, #38812), a human look would still be worthwhile.
What was reviewed
- Verified the userinfo →
token/username+passwordsplit matchesparse_registry_url_string_impl(same empty-username-means-token rule, username-only left alone). - Traced
needs_normalize→href_without_auth()so credentials are stripped from the stored URL even when explicit config wins. - Checked that
env.get_autoon the extracted values is a no-op for non-$strings, so behavior matches the literal-string path. - Confirmed the updated
redacted-config-logstest still keeps the masking helper covered via the new verbose-fetch case.
Extended reasoning...
Overview
This PR fixes Scope::from_api in src/install/npm.rs so that when a registry URL arrives with userinfo still embedded (the $ENV_VAR case, the object form's url, and the registry env vars), the user:pass@ / :token@ is moved into registry.username/password or registry.token and the stored href is rebuilt via href_without_auth(). It also adds registry.auth to the publish() NeedAuth check, since that check previously passed only because the userinfo was left inside the URL. Tests cover install (four config forms × Basic, one Bearer, error-output redaction, explicit-token precedence), publish, and npm_config_registry via bun pm view.
Security risks
This is credential-handling code. The change moves secrets from a URL field (which is printed in error output and never sent as an Authorization header by Bun's HTTP client) into the token/auth fields that are actually sent — a strict improvement over the current leak-in-error-output behavior. Precedence ("credentials configured next to the URL win") matches both the existing path-suffix handling and the literal-string path. I checked the odd corner of userinfo and a :_authToken= path suffix in the same URL: after this change the $VAR form behaves identically to the literal form (userinfo wins, suffix stays in the path), so no new divergence is introduced. get_auto only expands $-prefixed strings, so extracted plain values like "alice" pass through unchanged.
Level of scrutiny
High — this is auth-bearing config plumbing for bun install and bun publish. The logic itself is small (~20 new lines in from_api, one-line guard in publish) and closely mirrors the existing parse_registry_url_string_impl, but the guidance is to not auto-approve auth/credential changes.
Other factors
- The PR description is thorough and calls out three overlapping open PRs (#38796, #38824, #38812); a maintainer should coordinate merge order.
- Test coverage is strong: each new test asserts the exact
Authorizationheader the mock registry receives and that the secret does not appear in stderr; the modifiedredacted-config-logstest replaces a bug-certifying assertion with a real one and adds a separate verbose-fetch case soredacted_npm_urlmasking stays covered. - No prior human review comments to address; CI is still building.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how registry credentials are extracted and sent as Authorization headers, and it overlaps with several open sibling PRs (#38776, #38796, #38824), a human look would still be worthwhile.
What was reviewed:
- The new userinfo-splitting block in
Scope::from_apimirrorsparse_registry_url_string_implexactly (:pass@→ token,user:pass@→ username/password, username-only untouched);needs_normalizeis set unconditionally so the stored href is stripped even when explicit config wins. - The added
registry.auth.is_empty()check inpublish()is required because this change moves userinfo out ofregistry.urlintoregistry.auth, which the old check didn't accept. - The comment-cop warnings on
npm.rswere addressed by later commits (the remaining comment is one line). - The
redacted-config-logs.test.tschange replaces a test that certified the leaked-URL bug with one asserting Basic auth is sent and the URL is printed clean; a new verbose-fetch()test keepsredacted_npm_urlmasking covered.
Extended reasoning...
Overview
Two source hunks: (1) src/install/npm.rs — after $VAR expansion and URL::parse, Scope::from_api now pulls userinfo out of the parsed URL into registry.token or registry.username/password (only when none are already set) and sets needs_normalize = true so the stored href goes through href_without_auth(). (2) src/runtime/cli/publish_command.rs — the NeedAuth pre-flight check now also accepts a non-empty registry.auth, which is where the extracted user:pass lands after the first hunk. Three test files add coverage for install (string/object/scoped/env-var forms, Basic and Bearer, credential precedence, no-leak on error), publish (Basic auth PUT, printed URL clean), and rework a redaction test that was asserting the buggy masked-leak output.
Security risks
This is credential-handling code. The change starts sending Authorization headers for configurations that previously sent nothing, and rewrites the stored registry href. I verified the token-vs-basic rule matches the existing literal-string splitter (src/api/lib.rs:44-56), that explicitly configured token/username/password still take precedence (the URL creds are only adopted when all three are empty), and that needs_normalize is set regardless so the printed/stored URL never retains userinfo. The publish check is loosened only to accept registry.auth, which construct_publish_headers already knew how to send as Basic.
Level of scrutiny
High — this is auth header construction for the package manager and bun publish, and it composes with three other open PRs touching adjacent code paths. The diff itself is small and mechanical, but the merge-order interactions and the fact that an existing test's assertions were rewritten (correctly, as far as I can tell — the old assertion certified the bug) warrant a maintainer sign-off.
Other factors
CI's only failure is an unrelated ASAN issue in spawn-stdin-readable-stream.test.ts. The comment-cop bot flagged long comments on earlier commits; the current diff has a single one-line comment at the new block, so those look addressed. Tests are hermetic (local Bun.serve mocks, port: 0, per-test cache dir) and assert exact Authorization values plus absence of the secret in output.
… 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>
There was a problem hiding this comment.
The precedence issue I flagged earlier is fixed in 0fa97f6 — the URL-derived credentials are now gated on !registry.has_credentials(), and the explicitWins test matrix covers both directions (config token vs URL user:pass, and config user/pass vs URL :token@). No further issues found this run. Since this is credential handling (what Authorization header the package manager sends, what gets stripped from printed URLs), a human look is still worthwhile.
Reviewed: traced the config-username/password + URL-:token@ case through the new from_api block and confirmed Basic auth is now built at lines 482–497; checked that registry.url = from_url.url unconditionally strips userinfo so error output stays clean even when configured credentials win; confirmed NpmRegistry::from_url is a byte-for-byte move of the existing splitter and is idempotent for URLs already stripped by the config loaders.
Extended reasoning...
Overview
This PR fixes registry = "$VAR" in bunfig.toml when the env var contains user:pass@ or :token@ userinfo — previously the credentials were silently dropped (no Authorization header sent) and the secret was printed in error output. The fix runs the expanded URL through the same NpmRegistry::from_url splitter that literal strings already use, then fills in the credentials only when none were explicitly configured. Supporting changes: the splitter is moved from src/api/lib.rs to src/options_types/schema.rs as NpmRegistry::from_url (with src/api and src/ini delegating to it), a has_credentials() helper is added, and bun publish's pre-flight check now accepts registry.auth so Basic-auth registries don't report "missing authentication". Nine new tests cover the string form, object form, scoped registries, .env-sourced variables, Bearer tokens, credential redaction in error output, both precedence directions, and publish.
Follow-up on my previous review
My inline comment at 10:29 UTC identified that the earlier per-field fill (if registry.token.is_empty() { registry.token = from_url.token } etc.) let a URL-embedded :token@ short-circuit the 'outer block and displace an explicitly configured username/password pair. Commit 0fa97f63 replaces the per-field fill with a single if !registry.has_credentials() gate around all three assignments, which is the fix I suggested, and adds the reverse-direction precedence test. I re-traced the scenario through the current code (npm.rs:354–361, then 376–498) and confirmed the configured pair now reaches the base64-encode block and produces Basic auth. The URL is still unconditionally stripped (line 356), so the redaction test's stderr not containing s3cret assertion holds even when the URL credentials are discarded.
Security risks
This is squarely security-sensitive: it changes which credentials are sent as Authorization to package registries and what appears in error output. The direction of the change is safe (credentials that were previously dropped are now sent to the registry the user configured; secrets are removed from printed URLs rather than added), and the precedence rule ("explicit config wins over URL userinfo") is now correctly enforced and tested in both directions. I did not find a path where credentials leak to a different host or where a weaker credential silently replaces a stronger one.
Level of scrutiny
High — auth code for the package manager, with interactions across several sibling PRs (#38776, #38796, #38812, #38817, #38824) that share hunks. The PR description carefully documents which hunks are shared and why, but the coordination itself warrants a human confirming the merge order and that the shared hunks are indeed byte-identical.
Other factors
CI was green on 8e59c20 per robobun; three commits landed after that (the rework to use the shared splitter, the comment trim, and the precedence fix). The comment-cop bot thread from 10:05 UTC targets a version before 7d6036d reduced the comment to one line, so it's stale in practice even though the thread isn't marked resolved. Test coverage is thorough and each new test is stated to fail on main. Given the auth-sensitive scope and the multi-PR coordination, I'm deferring rather than auto-approving.
|
Heads-up from #39063: that PR adds |
…an env var
Scope::from_api expands `registry = "$VAR"` after the config loaders have
already split user:pass@ / :token@ out of literal registry strings, so a
URL that arrives through an env var reference (string form, object form
url, [install.scopes] entries) or through the registry env vars kept its
credentials inside Scope.url, where no request reads them, and printed
them in error output. Split them out in from_api, which every registry
goes through, into token (":token@") or username/password ("user:pass@"),
and store the URL without them. Credentials configured next to the URL
still win, as they do for the path suffix forms.
publish's pre-flight check accepts Scope.auth, which is where these
credentials now land.
…m_url Move the userinfo splitter from bun_api's Parser to NpmRegistry::from_url (with has_credentials next to it, used by the .npmrc loader) and call it from Scope::from_api on the expanded $ENV_VAR URL, before the URL is parsed, instead of splitting inline from the parsed URL. Explicitly configured credential fields take precedence field by field, matching the object form. Drops the object-literal test case (covered by the bunfig side) and adds a case where the variable comes from the project's .env.
Filling token, username and password from the URL one field at a time let a :token@ URL displace an explicitly configured username/password pair, since a token is sent in preference to the pair. Take the URL's credentials only when the registry has none, and pin both precedence directions.
0fa97f6 to
d60b2f9
Compare
|
Rebased onto main bf42a52 (was 0fa97f6, now d60b2f9).
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I checked the concern that the new unconditional from_url re-parse could alter already-clean URLs: bun_url::URL::parse keeps href as the input slice and from_url returns it unchanged when there is no userinfo, so registry.url and url_hash are byte-identical for literal or $VAR URLs without credentials (including :_authToken= pathname suffixes, which are still parsed by the unchanged code below the hunk). The publish test now reuses the shared registryMock/basicAuth helpers from the previous round.
Extended reasoning...
The change adds nine lines to Scope::from_api in src/install/npm.rs so an env-var-expanded registry URL goes through the same NpmRegistry::from_url splitter the config loaders use, gated on has_credentials() so explicit config wins, plus mock-registry tests in bun-install.test.ts and bun-publish.test.ts. It touches credential handling (Basic/Bearer auth for registry requests and redaction of secrets from error output), which is a security-sensitive surface. The Rust hunk is small and I verified the no-userinfo path is an identity on the URL, but because it is auth code the one reported finding is a nit and a human look is still worthwhile.
There was a problem hiding this comment.
I reviewed the rebased version and found no bugs; both earlier nits (it.each tables, publish test joining the shared credentials block) are addressed in the latest commits. Because this changes which credentials bun install/bun publish send, a human sign-off on the precedence rule is still worthwhile.
What was reviewed:
- Traced the new
from_urlpass inScope::from_apifor non-$VARregistries:bun_url::URL::parsekeepshrefas the input bytes andfrom_urlreturns it verbatim when there is no userinfo, so already-split loader URLs,:_authToken=pathname suffixes andurl_hashare unchanged. - Checked the all-or-nothing
has_credentials()gate against the latertoken.is_empty()branch: a configured username/password pair is no longer displaced by a:token@URL, and the secondexplicitWinsrow exercises exactly that. - Checked callers of
from_api(default registry,[install.registry]object form,[install.scopes],--registry,npm_config_registry) and the .npmrcapply_topath: all reach the same block, and .npmrc-provided credentials arrive with a stripped URL so the gate leaves them alone.
Extended reasoning...
The source change is a 9-line hunk in src/install/npm.rs that re-runs the shared NpmRegistry::from_url splitter on the registry URL after $ENV_VAR expansion, stores the userinfo-stripped URL, and copies URL-embedded token/username/password only when no credential was configured explicitly; the rest of the diff is tests in test/cli/install/bun-install.test.ts and bun-publish.test.ts using local Bun.serve registries. It touches credential selection for registry requests (auth headers and redaction of secrets from error output), which is a security-sensitive surface. The hunt exited on dry_streak with no findings, no CODEOWNERS entry covers these files, and the prior inline nits were addressed by the latest two commits; the auth-path nature of the change is what decided defer over approve.
This branch has not been deployed
1 participant
Related to #17531 and #18670. Found while probing registry credential handling, not from a user report.
Problem
registry = "$MY_REG"in bunfig.toml, withMY_REG=http://alice:s3cret@127.0.0.1:PORT/, sends noAuthorizationheader. On failure it prints the secret:error: GET http://alice:s3cret@127.0.0.1:PORT/no-deps - 404. Same for{ url = "$MY_REG" },[install.scopes]entries, the:token@form, andbun publish.NpmRegistry::from_url. A$VARvalue has no userinfo at that time.Scope::from_api(src/install/npm.rs) expands the variable later and never runs the split.Fix
Scope::from_apiruns the expanded URL throughNpmRegistry::from_url, the splitter the config loaders use. It stores the URL it returns (userinfo removed, otherwise byte-identical). If no credential was configured explicitly (has_credentials), it takes the credentials the URL carried.from_apiis unchanged, soregistry = "$VAR"behaves like the pasted value.tokenorusername/passwordwins as a set. A field-by-field fill let a:token@URL turn a configured Basic setup into Bearer. Tests pin both directions.test/cli/install/bun-install.test.tsand 1 intest/cli/install/bun-publish.test.ts. Each fails on a debug build of main bf42a52 and passes with this change. Also ran the rest of both files,npmrc.test.ts,config-precedence.test.tsandredacted-config-logs.test.ts.Background
Scope::from_apiturns the parsed config (Api::NpmRegistry) into what requests send:token(Bearer, preferred) orauth(Basic).from_apiresolves them against the package manager's env loader, which includes the project's.env. That loader does not exist when bunfig.toml is parsed, so the expansion cannot move earlier.NpmRegistry::from_url(on main from install: send credentials embedded in --registry and registry env var URLs #38796)::x@is a bearer token,u:p@is a Basic pair, a username alone is left in place.from_api(the first revision). Calling the shared one keeps the userinfo rule in one place.Downsides
from_urlreturns the input unchanged, sofrom_apistores the same href and computes the sameurl_hash.Notes
.npmrcis not affected: its${VAR}syntax is expanded while the file is parsed, before the split.NpmRegistry::from_url,has_credentials, thepublish_command.rspre-flight check and theredacted-config-logs.test.tscases, byte-for-byte from install: send credentials embedded in --registry and registry env var URLs #38796 and publish: accept basic auth credentials from .npmrc and bunfig.toml #38776. Main has those lines now, so they rebased to nothing. Thesrc/install/npm.rshunk is identical before and after the rebase.bun publish"missing authentication" reports for Basic credentials. install: send credentials embedded in --registry and registry env var URLs #38796 (merged) carries the pre-flight fix and its tests, so noFixesline here.NpmRegistry.credentials_from_url, set byfrom_urlwhen it takes credentials out of the URL. Whichever of the two lands second must copy that field in thefrom_apiblock too. Otherwiseregistry = "$VAR"with auser@URL keeps dropping the credentials.from_apibelow this hunk (the suffix parsing). This hunk operates on theNpmRegistrybefore that body starts, so it applies on either side of that rewrite.from_api: expanding at config time would also start matching.npmrc//host/credential lines against expanded URLs, a separate behaviour change. This PR only makes the expanded URL go through the split every other spelling already gets.Bun.servemock registry serves a manifest and a tarball and records theAuthorizationit receives.registry = "$VAR"(variable from the process environment, and from the project's.env),registry = { url = "$VAR" }and an[install.scopes]entry send Basic on both requests.:token@sends Bearer. A 404 is reported with the credentials removed from the printed URL. A configuredtokenbeatsuser:pass@in the URL, and a configured username/password beats:token@in the URL. The publish test checks one PUT with Basic auth and a clean registry URL in the output.from_apifrom the parsedbun_url::URLand also claimed the object form's literalurl. Self-review pointed out that this duplicated the splitter install: send credentials embedded in --registry and registry env var URLs #38796 consolidates and the object-form fix in bunfig: send credentials written into the url of a registry object #38824 (merged), so it was reworked to call the shared splitter. The second filledtoken/username/passwordone field at a time. Review caught that a:token@URL then displaced an explicit username/password pair, which the all-or-nothing gate and the second precedence test now cover.href_without_authproducinghttp://host//for a path-less registry (the tests here accept either form). install: redact secrets in the URLs printed for failed manifest and tarball downloads #38817 redacts the install error lines themselves.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-publish.test.ts, test/cli/install/bun-install.test.ts