install: redact secrets in the URLs printed for failed manifest and tarball downloads - #38817
install: redact secrets in the URLs printed for failed manifest and tarball downloads#38817robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (3)
Comment |
|
Status: reproduced on the 1.4.0 release binary and on main, fix pushed (now a single commit, dc39126, rebased onto main after #38796 and #38183 landed), waiting on CI. Repro used: Scope: this PR covers the request URL operand of the six failure lines. A tarball or git URL the user wrote into package.json is still echoed verbatim by the lines that print the dependency's resolution ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical swap of bstr::BStr::new for the existing redacted_npm_url formatter at six print sites.
What was reviewed:
- Confirmed
redacted_npm_url(&[u8])returns aDisplayadapter, so it drops in forBStr::newin the format-args position; URLs without secrets pass through byte-for-byte. - Checked
audit_fix::mentions_package, which re-reads these log lines — it matches on/<name>in the path, unaffected by password/token redaction. - Tests cover all six sites (manifest error/warn, tarball error/warn, isolated linker), use a local server with
port: 0, and assert both the redacted line and absence of the raw secret.
Extended reasoning...
Overview
Six one-line changes across src/install/PackageManager/runTasks.rs (four GET {} - {} lines) and src/install/isolated_install/Installer.rs (two failed to download lines), replacing bstr::BStr::new(url) with bun_core::fmt::redacted_npm_url(url) in error/warning format arguments. Five new tests in test/cli/install/redacted-config-logs.test.ts exercise each print site with a secret arriving via redirect or dist.tarball.
Security risks
None introduced — this reduces exposure by masking URL passwords, UUIDs, and npm_/npms_ tokens in log output. The formatter is already used for the same purpose in npm.rs, http/lib.rs, and bun pm whoami; this brings the install-loop lines into line.
Level of scrutiny
Low. The change is a formatter substitution in log-message arguments only; no control flow, allocation, or lifetime changes. redacted_npm_url has the same signature shape (&[u8] -> impl Display) as BStr::new, so the swap is type-equivalent at each site. I verified the one downstream consumer that parses these lines (audit_fix::mentions_package) matches on the package-name path segment, which the redactor never touches, and the PR description reports the relevant tests were re-run.
Other factors
Tests follow harness conventions: tempDir, bunEnv spread, local Bun.serve({port: 0}), concurrent pipe draining, describe.concurrent, await using for the server and process. Each test asserts the exact redacted line (toContain("...token=*** - 404")) plus not.toContain(password)/not.toContain(token) on both stdout and stderr, so a regression prints a useful diff rather than a bare boolean. The isolated-linker case seeds a lockfile with the secret-bearing tarball URL directly, hitting the on_task_fail path independently of the resolve-phase GET line.
|
Updated 9:53 AM PT - Aug 15th, 2026
✅ @robobun, your commit dc39126d4e02e411d3a9eac42da50cce8bfc5836 passed in 🧪 To try this PR locally: bunx bun-pr 38817That installs a local version of the PR into your bun-38817 --bun |
There was a problem hiding this comment.
LGTM — mechanical swap of bstr::BStr::new for the existing bun_core::fmt::redacted_npm_url formatter at six error-message sites.
What was reviewed
- Confirmed
redacted_npm_urltakes&[u8]and implsDisplay, so it drops in forBStr::newat all six format-arg positions; same helper already used innpm.rs,http/lib.rs, andpackage_manager_command.rs. - Read the formatter impl — for URLs with no password/UUID/
npm_token it writes bytes through unchanged, so callers that parse theGET <url> - <status>line back out are unaffected. - Tests follow harness conventions (
tempDir,port: 0, per-test cache dir, concurrent pipe drain, exit code asserted last) and cover each of the fourrunTasks.rssites plus the isolated-linkeron_task_failpath; the.tododocuments the adjacent resolution-formatter case that is intentionally out of scope.
Extended reasoning...
Overview
The PR swaps bstr::BStr::new(url) for bun_core::fmt::redacted_npm_url(url) at six format-argument positions: four GET {} - {} lines in src/install/PackageManager/runTasks.rs (manifest error/warn, tarball error/warn) and two failed to download ... {url} lines in src/install/isolated_install/Installer.rs (on_package_download_error and the TaskError::Download arm of on_task_fail). Five new tests in test/cli/install/redacted-config-logs.test.ts exercise each print site with secrets that arrive via a manifest redirect target or dist.tarball, plus a .todo for the resolution-formatter case that is not addressed here.
Security risks
None introduced. The change is strictly security-positive: it stops bun install from echoing URL userinfo passwords and npm_/UUID tokens to stderr on 4xx/5xx download failures. redacted_npm_url is a pure Display adapter over the same &[u8] that was already being printed; no new parsing, allocation, or side effects.
Level of scrutiny
Low. This is a one-for-one formatter substitution at six call sites, using an in-tree helper that is already the established pattern for the same purpose (Npm::response_error in src/install/npm.rs:252, the verbose HTTP request line in src/http/lib.rs:1443, bun pm whoami in package_manager_command.rs:318). Both BStr::new and redacted_npm_url take &[u8] and return impl Display, so no signature or borrow changes ripple out. Reading the RedactedNpmUrlFormatter::fmt impl at src/bun_core/fmt.rs:233-278 confirms that a URL with no password/UUID/npm_ token is written through byte-for-byte, so any downstream code (the PR mentions bun audit fix/bun update) that pattern-matches these lines is unaffected for plain URLs.
Other factors
The tests are well-constructed per the repo's review guide: local Bun.serve({port:0}) registry, tempDir + isolated BUN_INSTALL_CACHE_DIR, await using for server/proc, Promise.all on stdout/stderr/exited, exact assertion on the redacted line (carol:******@.../...?token=***), not.toContain on the raw secret in both streams, exit code asserted last, describe.concurrent for speed. The .todo test transparently documents that a tarball URL written directly in package.json still leaks via the resolution formatter — a distinct print site the author scoped out. No CODEOWNERS entry covers src/install/. The robobun CI comment flags failures on the first commit (08f5201); a follow-up commit (d9f6ead) tightened the tests, and the six-line source change itself cannot plausibly cause unrelated failures.
… 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.
LGTM — mechanical swap of bstr::BStr::new for the existing redacted_npm_url formatter at six error-message sites, with a test per site.
What was reviewed:
- Confirmed
redacted_npm_url(&[u8])matches the operand type at all six call sites and is already used for the same purpose innpm.rs,http/lib.rs, andpackage_manager_command.rs. - Checked that URLs without secrets pass through unchanged, so existing tests parsing the
GET <url> - <status>line are unaffected. - Tests are hermetic (local
Bun.serve,port: 0, per-test cache dir), cover error+warn variants and both linkers, and inject secrets via redirect/dist.tarball rather than the configured registry so they survive #38796.
Extended reasoning...
Overview
Six one-line changes replacing bstr::BStr::new(url) with bun_core::fmt::redacted_npm_url(url) in error/warning messages: four GET {} - {} lines in src/install/PackageManager/runTasks.rs (manifest error/warn, tarball error/warn) and two failed to download lines in src/install/isolated_install/Installer.rs. A new describe.concurrent block in test/cli/install/redacted-config-logs.test.ts adds five tests (one per print site) plus a .todo documenting the out-of-scope resolution-display case.
Security risks
None introduced. This is a security improvement — it stops bun install from printing registry-supplied credentials (userinfo passwords, npm_ tokens, UUIDs) verbatim to stderr on HTTP failures, which would otherwise land in CI logs. The formatter is passthrough for URLs without secrets.
Level of scrutiny
Low. The change is a pure Display-formatter substitution at log sites; it does not touch control flow, request handling, or the URL bytes themselves. redacted_npm_url is the established in-tree helper for exactly this and is already used at the sibling bun pm view/bun publish/verbose-HTTP sites (per #36165). The signature fn redacted_npm_url(&[u8]) -> RedactedNpmUrlFormatter<'_> matches every call site (metadata.url.slice(), url: &[u8], &dl.url).
Other factors
The tests follow harness conventions well: tempDir, bunEnv spread with a per-test BUN_INSTALL_CACHE_DIR, concurrent pipe draining, await using for the server and process, describe.concurrent for independent subprocess tests, exit code asserted last, and both negative (not.toContain(secret)) and positive (toContain(maskedLine)) assertions so the test can't pass vacuously. The secrets are injected via a manifest redirect and dist.tarball rather than the --registry URL, keeping the tests meaningful after #38796. The PR description explicitly enumerates the sibling sites intentionally left for the follow-up on top of #38631 and pins the remaining shape with a .todo test. No outstanding reviewer comments.
…arball downloads The GET <url> - <status> lines in runTasks.rs and the isolated linker's "failed to download" line formatted the URL verbatim, so a password in the URL's userinfo or an npm_ token / UUID in its query ended up on stderr. Format them with bun_core::fmt::redacted_npm_url, which the publish / pm view error path and the verbose request line already use. The tests feed the secret in through a manifest redirect and through dist.tarball, so they cover the registry-supplied URLs this change is about; the tarball spec a user writes into package.json is still echoed by the resolution-printing lines and is recorded as a todo.
8dba0f8 to
dc39126
Compare
There was a problem hiding this comment.
LGTM — mechanical swap of bstr::BStr::new for the existing bun_core::fmt::redacted_npm_url formatter at six error-message sites, with per-site test coverage.
What was reviewed:
- Confirmed
redacted_npm_url(&[u8]) -> impl Displayis a drop-in replacement at all six format-arg positions; no control-flow or lifetime change. - Checked the other in-tree callers (
npm.rs,http/lib.rs,audit_fix,package_manager_command) — this brings the install failure lines in line with the same helper they already use. - Tests: local
Bun.serveregistry,port: 0,tempDir, pipes drained concurrently, assert both the masked output and absence of the raw secret; secrets enter via redirect /dist.tarballso the tests stay valid after #38796.
Extended reasoning...
Overview
Six one-line edits across src/install/PackageManager/runTasks.rs (four GET {} - {} lines: manifest error/warning, tarball error/warning) and src/install/isolated_install/Installer.rs (two failed to download ... <url> lines), replacing bstr::BStr::new(url) with bun_core::fmt::redacted_npm_url(url) in the format-argument position. test/cli/install/redacted-config-logs.test.ts gains a describe.concurrent block with five tests (one per print site) plus a test.todo documenting the resolution-display sites that are intentionally out of scope pending #38631.
Security risks
None introduced; the change is strictly security-positive. It redacts userinfo passwords and npm_/UUID tokens from URLs that reach stderr on download failure — URLs that come from the registry (redirect targets, dist.tarball) rather than from the user's own config, so the user may never have seen the secret that would otherwise land in a CI log. redacted_npm_url passes non-secret URLs through byte-for-byte, so there is no information loss for the debuggable case and no behavior change for the bun audit fix / bun update code that parses these lines.
Level of scrutiny
Low. This is a formatter swap at print sites only — no control flow, allocation, or lifetime changes. The helper is the established in-tree redaction formatter already used by Npm::response_error, the verbose HTTP request line, bun pm whoami, and audit_fix; this PR just closes the gap left by #36165. Both operands (metadata.url.slice(), url, &dl.url) are &[u8], matching the helper's signature exactly.
Other factors
- Tests follow the repo's harness conventions: local
Bun.serve({ port: 0 }),tempDir,{...bunEnv, ...}, concurrent pipe drain, exit-code asserted last, and assert both presence of the masked string and absence of the raw password/token on both stdout and stderr. The secrets are injected via a 302 redirect and viadist.tarball(not via the configured--registryURL), so the tests remain meaningful once #38796 strips userinfo from the configured registry. - The variant matrix is covered: required vs optional dependency (error vs warn), manifest vs tarball, hoisted vs isolated linker. The
test.todowith an explanatory comment records the resolution-display sweep that is deferred to land on top of #38631. - No CODEOWNERS coverage for the touched paths. No prior reviewer comments to address.
1 participant
Problem
bun installprints the URL of that request verbatim, including any secret inside it:error: GET http://alice:s3cret@127.0.0.1:PORT/no-deps - 404error: GET http://127.0.0.1:PORT/cdn/pkg-1.0.0.tgz?token=npm_... - 404error: failed to download pkg@1.0.0: 404 Not Foundfollowed by the same unredacted URL (isolated linker)dist.tarballURL out of the manifest (also what bun.lock records for an npm package whose tarball is not at the default location). Registries and CDNs do hand out URLs with a token or password in them, so the user may never have seen the secret that ends up in their CI log. Credentials written into the configured registry URL itself used to end up here too; install: send credentials embedded in --registry and registry env var URLs #38796 (merged) moves those into the Authorization header, which removes that one source but not the other two, which still print on current main.GET {} - {}lines insrc/install/PackageManager/runTasks.rs(manifest and tarball, error and warning variants) and the twofailed to downloadlines insrc/install/isolated_install/Installer.rsformat the URL withbstr::BStr::new. Robustness pass across install, css, ffi, crypto, spawn, shell, and node compat #36165 routed thebun publish/bun pm viewerror path and the verbose request line throughredacted_npm_url; these lines were missed.Fix
bun_core::fmt::redacted_npm_urlinstead ofBStr::new. The lines now readGET http://alice:******@127.0.0.1:PORT/no-deps - 404and...tgz?token=*** - 404.Npm::response_errorinsrc/install/npm.rs, the verbose> HTTP/1.1 GETline insrc/http/lib.rs,bun pm whoami), so the install lines now behave like those. A URL without a password, UUID ornpm_token prints byte for byte as before, andbun audit fix/bun update, which re-read theGET <url>/<name> - <status>text out of the log, still match it (their tests pass).name@<url> failed to resolve, thename@<url>prefix of the isolated linker'sfailed to downloadline, the install summary,bun pm ls, ...) and those still print it verbatim. That is a sweep over theResolutiondisplay sites, which install: escape control characters in resolutions, specifiers, bin names and registry error text #38631 is wrapping for control-character escaping at the same time; it is tracked separately so it can be done once, on top of install: escape control characters in resolutions, specifiers, bin names and registry error text #38631, rather than half of it here. The todo test at the end of the new block pins the shape that is still open.runTasks.rstarball error/warning, bothInstaller.rslines) to wrap the URL inescape_control_chars(..). Whichever lands second resolves those four lines toEscapeControlChars(bun_core::fmt::redacted_npm_url(url))(redact the raw bytes first, since the password scan stops at a newline; then escape what is printed). The manifest lines are not touched by install: escape control characters in resolutions, specifiers, bin names and registry error text #38631.test/cli/install/redacted-config-logs.test.ts: five new tests, one per print site, where the registry URL bun is configured with carries no secret and the secret reaches bun only through a manifest redirect ordist.tarball(so they stay meaningful after install: send credentials embedded in --registry and registry env var URLs #38796). All five fail on current main (with install: send credentials embedded in --registry and registry env var URLs #38796 and install: canonical registry URL in Scope; redact secrets in bun audit registry URLs #38183 in) with the unredacted URL on stderr and pass with this change; the whole file passes (21 pass, 1 todo).bun-install-retry.test.ts(plain URLs through both the error and warning variants), thebun audit fixandbun updatetests that parse these lines, and theoptionalDependenciestests inbun-install-registry.test.ts.Background
redacted_npm_url(src/bun_core/fmt.rs) is aDisplayadapter over URL bytes. It replaces the password part ofscheme://user:password@hostwith one*per character, and any UUID ornpm_/npms_token anywhere in the string with***; everything else is written through unchanged.task.url_bufis what it asked for,metadata.urlis the URL of the response it actually got (the redirect target when the registry redirected). TheGETlines print the latter.pkg@1.0.0), for a tarball or git dependency written in package.json it displays as that URL. Only the registry case is touched here, because there the URL is carried by the separate URL operand that this PR redacts.--linker isolated, tarballs still missing from the cache at install time are downloaded by the store installer; its failures (HTTP status, network error, extract failure) all go throughon_package_download_error/on_task_failand print thefailed to downloadline instead of theGETline. The hoisted linker prints theGETline in both phases.