install: redact the registry URL in the manifest/tarball URL errors and keep namedRegistries credentials out of bun.lock - #38997
Conversation
|
Warning Review limit reached
Next review available in: 2 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 (4)
Comment |
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 6ac41d5 has some failures in 🧪 To try this PR locally: bunx bun-pr 38997That installs a local version of the PR into your bun-38997 --bun |
|
Status: reproduced and fixed in this PR (reworked after review, see the PR description).
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — routes the remaining registry-URL error/warning sites through the existing redacted_npm_url helper.
What was reviewed:
- The eight message sites in
NetworkTask.rsandpnpm.rs— each now redacts beforequote(); URLs without secrets print unchanged. - Moving
url_bytes = tmp.to_owned_slice()above the http:// check —tmp(anOwnedString) is still live forhas_prefix_comptime, and the extra allocation only hits the error-return path. - The
Received: "{}"→quote()switch — same output for plain URLs, consistent with sibling messages. - New tests are hermetic (every case bails before sending a request), assert both the masked form and the absence of the raw password/token, and the pnpm test confirms the lockfile still records the real URL.
Extended reasoning...
Overview
Routes the seven URL-printing error/warning messages in NetworkTask::for_manifest/for_tarball and one in pnpm.rs through bun_core::fmt::redacted_npm_url, so a password or npm_/UUID token embedded in the configured registry URL is masked when the URL is echoed in an error. A 4-line redacted_url helper renders the Display adapter into a Vec<u8> so the result can still go through quote() as before. Eight new subprocess tests cover every message variant plus the pnpm named-registry warning.
Security risks
None introduced — this reduces secret exposure in logs. Only message text changes; the install behaviour, the request URLs, and the lockfile the pnpm migration writes are untouched (the new pnpm test asserts the lockfile keeps the real URL). The redaction helper is already used at four other sites (npm.rs, http/lib.rs, bun pm whoami, verbose trace).
Level of scrutiny
Low-to-medium. Mechanical application of an existing formatter to remaining sibling sites, no new logic or control-flow beyond hoisting one allocation six lines earlier (only reached on an error path). redacted_npm_url is byte-identical for URLs without secrets, so the existing tests asserting on these messages keep passing.
Other factors
Tests are thorough: one case per message (error and warning variants), hermetic (each configured URL is invalid so no request is sent), describe.concurrent, isolated cache dir, and assert both the masked form and not.toContain(password/token) on stdout+stderr. The masked() regex tolerates the credential being moved out of the URL entirely by the referenced follow-up PRs. The known limitation (non-http-scheme passwords) is called out in the description and in the test comments, and is unchanged behaviour of the existing helper.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of either; the three PRs are one sweep split by print site, and none of them touches another one's lines:
All three add cases to The One interaction worth knowing: #38977 also extends the password scan from http(s) to any |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical change routing eight registry-URL error message operands through the existing redacted_npm_url formatter.
What was reviewed:
- Verified
redacted_npm_urlandquotesignatures insrc/bun_core/fmt.rs; the render-to-Vec<u8>-then-quote helper is needed becausequote()takes bytes and the redaction scan must run on the unescaped URL. - Checked that hoisting
url_bytes = tmp.to_owned_slice()above the http-scheme check is safe (tmpis duplicated, not consumed, and stays valid forhas_prefix_comptime). - The comment-cop flag on
NetworkTask.rs:440looks like a false positive: the doc comment is two lines explaining the redact-then-quote order, not a paragraph justifying a workaround. - CI failure (
test-http-chunk-problem.jsASAN) is unrelated to install code.
Extended reasoning...
Overview
Routes the URL operand of eight error/warning messages in src/install/NetworkTask.rs (for_manifest's three refusal messages × error/warning variants, plus for_tarball's scheme check) and one warning in src/install/pnpm.rs (the pnpm named-registry migration warning) through bun_core::fmt::redacted_npm_url, which is the existing formatter already used for the same purpose in npm.rs, bun pm whoami, and the verbose request trace. A small redacted_url helper renders the redacted Display adapter into a Vec<u8> so the result can go through quote() as before. One incidental change: Received: "{}" now uses quote() instead of literal quote characters, matching its siblings. Eight new test cases (7 in redacted-config-logs.test.ts, 1 in pnpm-lock-v9.test.ts) each assert both the masked form and the absence of the raw secret on stdout/stderr.
Security risks
None introduced. This is a security-positive, display-only change: it reduces credential exposure in error output without touching auth logic, request construction, or the lockfile writer. The pnpm test asserts the lockfile still records the unredacted URL (only the printed warning is masked). The write! to Vec<u8> cannot fail (Vec's io::Write impl is infallible), so let _ = is correct.
Level of scrutiny
Low-to-medium. The change is mechanical — wrapping existing print-site operands in an existing, already-tested redaction helper — and confined to error-path message formatting. No control flow, request building, or lockfile serialization changes. The one non-trivial edit (hoisting url_bytes computation above the http-scheme check) only adds an allocation on an error path and does not affect tmp's validity.
Other factors
- Test coverage is thorough: every touched message site has a case (both error and warning variants), each asserts the exact masked form plus
not.toContain(password)/not.toContain(token)on both streams and the exit code. The tests use127.0.0.1:1/ an unjoinable host so no request is sent. The pnpm test additionally asserts the lockfile keeps the real URL. - The PR description confirms existing tests asserting on these messages (
bun-install.test.ts,bun-install-registry.test.ts) pass unchanged, which makes sense sinceredacted_npm_urlis a byte-for-byte pass-through when no secret pattern is present. - The comment-cop bot flagged the two-line doc comment on
redacted_url; I read it as a false positive — the comment concisely explains why redaction must run before quoting, which is the one non-obvious ordering constraint here. - The single CI failure is an unrelated ASAN flake in
test/js/node/test/parallel/test-http-chunk-problem.js.
84641e9 to
dd583eb
Compare
…keep namedRegistries credentials out of bun.lock
The messages NetworkTask prints when it cannot build a request URL out of
the configured registry URL ("Failed to join registry ...", "Invalid
package name ...: manifest URL ... is not on registry ...", "Registry URL
must be http:// or https://", "Expected tarball URL to start with ...")
formatted the registry href and the URL built from it verbatim, so a
token written into the registry URL as a path segment ended up on
stderr. Format them through redacted_npm_url like the other registry URL
print sites; the URL is redacted into a buffer first and the buffer is
passed to quote(), which escapes but does not redact.
The pnpm migration's "fetching pnpm registry ... packages from <url>"
warning printed the namedRegistries URL from pnpm-workspace.yaml the
same way. That URL was also stored as written, so user:password@ in it
was copied into every tarball URL recorded in bun.lock (bun never sends
URL credentials) and a named registry that was the configured registry
spelled with credentials was not recognized as such. Read the value
through NpmRegistry::from_url, as every other registry URL source does,
so the credentials are split off and only the URL is kept; the warning
is redacted as well for a token in the path.
dd583eb to
6ac41d5
Compare
robobun
changed the title
There was a problem hiding this comment.
LGTM — routes the remaining install-side registry-URL error messages through the existing redacted_npm_url helper and normalizes pnpm namedRegistries URLs through NpmRegistry::from_url like every other registry source.
What was reviewed:
- The seven
NetworkTask.rsmessage sites and the onepnpm.rswarning: each now renders throughredacted_npm_urlfirst, thenquote(); the movedurl_bytes = tmp.to_owned_slice()line only adds an allocation on the cold error branch and is behavior-preserving on the success path. read_named_registries:NpmRegistry::from_url(str).urlreturnsBox::from(url.href)(the input bytes) when there are no credentials, so URLs without credentials are stored byte-for-byte as before; with credentials it useshref_without_auth(), matching the bunfig/.npmrc/--registry paths. The dropped credentials were never sent (HTTP client only reads them from proxy URLs), and the prefix-match against the configured registry now works when the named URL is spelled with credentials.- Tests: 7 concurrent subprocess cases covering every message (error + warning variants) with a token-in-path registry, plus 2 pnpm migration tests asserting the redacted warning, credential-free bun.lock, and configured-registry recognition. All hermetic (127.0.0.1:1 is never contacted — these are refusals before any request), drain pipes concurrently, and assert both the masked output and
not.toContain(token/password).
Extended reasoning...
Overview
The PR touches two source files and two test files. In src/install/NetworkTask.rs it wraps the URL operand of seven error/warning messages (three in for_manifest × error/warn variants, one in for_tarball) with a small redacted_url helper that renders bun_core::fmt::redacted_npm_url into a Vec<u8> so the result can go through quote() as before. It also switches Received: \"{}\" to Received: {quote(...)} for consistency with siblings (identical output for plain URLs). In src/install/pnpm.rs it (a) redacts the URL in the "fetching pnpm registry" warning and (b) stores NpmRegistry::from_url(value).url instead of the raw YAML value in read_named_registries, so user:password@ is stripped before the URL becomes a tarball base written into bun.lock.
Security risks
None introduced; the change is strictly defensive. It reduces credential exposure in stderr and in bun.lock by reusing two helpers that are already the canonical redaction/parsing paths elsewhere (redacted_npm_url for display, NpmRegistry::from_url for storage). No new parsing, no auth logic changes, no new user-controlled input reaches a new sink. The credentials dropped from the pnpm named-registry URL were never sent by bun's HTTP client (it only reads URL credentials from proxy URLs), so no request behavior changes.
Level of scrutiny
Low-to-medium. The NetworkTask.rs changes are pure error-message formatting on cold paths, and the redacted_url helper is a three-line write!-into-Vec (infallible on Vec<u8>, hence the let _). The one code-motion — computing url_bytes before the http(s) prefix check — is behavior-preserving on the success path and only adds a boxed-slice allocation on the error path. The pnpm.rs storage change is a one-line substitution that aligns this input source with every other registry-URL source in the codebase; I verified NpmRegistry::from_url returns Box::from(url.href) (the input bytes verbatim) when no credentials are present, so existing lockfiles are unaffected.
Other factors
Test coverage is thorough: one case per changed message including both error and warning variants, plus the tarball path via a bun.lock, plus two pnpm tests covering the redacted warning, the credential-free bun.lock tarball URL, and the fixed configured-registry recognition. Tests follow the harness conventions (describe.concurrent, tempDir, bunEnv spread, concurrent pipe drain, exit-code asserted last) and are hermetic — the refusal messages fire before any request is made, so 127.0.0.1:1 is never contacted. The two comment-cop bot notes about long comments were addressed in 6ac41d5 (both now one line) and are marked resolved. No CODEOWNERS entry covers these paths, and there are no prior reviews from me on this PR.
1 participant
Problem
bun installcannot turn the configured registry URL into a request URL, the message it prints contains that URL verbatim. A token written into the registry URL as a path segment (https://host/npm_a1b2.../) ends up on stderr:error: Failed to join registry "http://ex ample.org/npm_a1b2.../" and package "notapackage" URLserror: Invalid package name "..": manifest URL "http://127.0.0.1:1/" is not on registry "http://127.0.0.1:1/npm_a1b2.../"Registry URL must be http:// or https:///Received: "htp://127.0.0.1:1/npm_a1b2.../notapackage"andExpected tarball URL to start with https:// or http://, got "htp://127.0.0.1:1/npm_a1b2.../pkg/-/pkg-1.0.0.tgz" ...for a registry URL with a mistyped schemenamedRegistriesURL frompnpm-workspace.yamlthe same way:warn: fetching pnpm registry "work" packages from http://carol:s3cret@127.0.0.1:1/npm_a1b2.../; .... That URL is also stored as written, souser:password@in it was additionally copied into every tarball URL the migration records in bun.lock, although bun never sends URL credentials (the HTTP client only reads them from a proxy URL). A named registry that is the configured registry spelled with credentials was also not recognized as such, so it got the warning and its own tarball URLs instead of the configured registry's.src/install/NetworkTask.rs(for_manifest,for_tarball) format the URL withquote()or the plain string andsrc/install/pnpm.rswithBStr::new, none of which redact;read_named_registriesinsrc/install/pnpm.rsstores the YAML value verbatim, while every other registry URL source goes throughNpmRegistry::from_url, which splits credentials off the URL.Fix
bun_core::fmt::redacted_npm_url. TheNetworkTaskmessages keep their quoting: a small helper renders the redacted URL into a buffer and that buffer goes throughquote()as before. Redaction has to run first because the formatter's password scan only recognizes a string that starts with the URL;Received: "{}"now also usesquote()instead of literal quotes, so it prints like its siblings (identical output for a plain URL).read_named_registriesstoresNpmRegistry::from_url(value).url, the same splitting the bunfig, .npmrc,--registryand env var sources use. A URL without credentials is stored byte for byte as before; the credentials of one with them are dropped, which changes nothing about the requests (they were never sent) and keeps them out of bun.lock. The warning still tells the user to configure auth for that registry in bunfig.toml or .npmrc. A token in the path stays, since it is part of where the tarballs are; it is masked in the warning and recorded in bun.lock like any other tarball URL.redacted_npm_urlis what bun already uses when it prints a registry or request URL (registry response errors, the verbose request trace,bun pm whoami, thebun auditlines from install: canonical registry URL in Scope; redact secrets in bun audit registry URLs #38183, theGET <url> - <status>lines in install: redact secrets in the URLs printed for failed manifest and tarball downloads #38817); these were the remaining install-side sites. A URL with no password, UUID ornpm_token prints byte for byte as before, and the existing tests asserting on these messages pass unchanged.test/cli/install/redacted-config-logs.test.ts: new block with one case perNetworkTaskmessage (error and warning variants; the tarball one through abun.lock). The registry URLs carry the token only: since install: send credentials embedded in --registry and registry env var URLs #38796 and bunfig: send credentials written into the url of a registry object #38824 the config sources splituser:password@off before these messages see the URL ($VARregistries, install: send credentials embedded in a registry URL that comes from an env var #38834, are the remaining exception), so the token is what reaches them from every source. All 7 fail without the change (the token is on stderr) and pass with it; the whole file passes (23 tests).test/cli/install/migration/pnpm-lock-v9.test.ts: two new named-registries tests. One asserts the warning showshttp://127.0.0.1:1/***/and that bun.lock records the tarball URL with the token and without the password; the other that the configured registry written with credentials gets no warning and installs from the configured registry. Both fail without the change (credentials in the warning and in bun.lock; the warning is printed for the configured registry) and pass with it; the whole file passes (83 tests).bun-install.test.ts"Registry URLs", "providing invalid url in lockfile", the canonical registry URL tests;bun-install-registry.test.ts"different host than the registry") and the rest of the named-registries tests pass.cargo fmt -p bun_install -- --checkis clean.Background
redacted_npm_url(src/bun_core/fmt.rs) is aDisplayadapter over the bytes of a URL: it replaces the password ofscheme://user:password@hostwith one*per character, and any UUID ornpm_/npms_token anywhere in the string with***; everything else is written through unchanged.quote()(bun_core::fmt) wraps bytes in double quotes and JSON-escapes them if they contain a quote, a control character or non-ASCII. These messages have always used it for the package name and the URLs, and existing tests assert the quoted form.NpmRegistry::from_url(src/options_types/schema.rs) is how a registry URL string becomes a registry config:user:password@or:token@in the URL are moved into the username / password / token fields and the URL is stored without them.Scope(src/install/npm.rs) then holds the URL that requests are built from;scope.url.href()is what these messages print.NetworkTask::for_manifestbuilds a package's manifest URL by joining the package name onto the registry URL and refuses to send the request when the join fails, when the result is not http(s), or when it lands outside the registry's origin or directory (a name of..does that).for_tarballbuilds the tarball URL from the same registry URL when the lockfile carries no explicit one. The messages above are those refusals.pnpm-workspace.yamlcan name registries (namedRegistries:) thatpnpm-lock.yamlentries refer to aswork:1.0.0. When the named URL is not the configured registry, the migration records tarball URLs under it (<url><name>/-/<name>-<version>.tgz) and prints the warning once per registry; when it is, the packages are recorded against the configured registry. The comparison is a prefix match on the stored URL strings, which is why credentials left in the URL also defeated it.Earlier revision of this PR
The first revision was based on a main that predated #38796 and #38824, where the bunfig object form,
--registryand the registry env vars still storeduser:password@in the URL; its tests put a password and a token in the registry URL and accepted the credentials either masked or absent. Review pointed out that on current main only the token reaches these messages, and that the pnpm test was pinning the credentials being written into bun.lock. This revision switches the NetworkTask tests to token-only registry URLs and adds theread_named_registrieschange with its two tests; the message changes are unchanged.Output before / after for the pnpm case (
work: http://carol:s3cret@127.0.0.1:1/npm_a1b2.../in pnpm-workspace.yaml):