install: escape control characters in resolutions, specifiers, bin names and registry error text - #38631
install: escape control characters in resolutions, specifiers, bin names and registry error text#38631robobun wants to merge 8 commits into
Conversation
…mes and registry error text Wrap the remaining package-manager output that prints text authored by a dependency or by the registry in bun_core::fmt::escape_control_chars: - the install summary (`+ name@resolution`, `installed x@y with binaries:` and the bin names under it), `bun pm ls` and `bun why`, where a git/tarball resolution and the dependent's version range are printed - the dist-tag, version range and `file:` path in the "not found" errors reported while resolving, and the `failed to resolve` summary lines - the tarball URL in `GET <url> - <status>`, in the hoisted `downloading tarball name@resolution` error and in the isolated installer's `failed to download` report - the status text and `error` field of a registry error response, the `npm-notice` and `www-authenticate` headers, and the `error` field of a packument The helper is the one #38525, #38536 and #38615 add, byte for byte, so the branches merge in any order.
|
Status: ready for review. Reproduced on the current build (1.4.0-canary.1, b7a0431) with a loopback registry; every one of the fourteen cases in Review: the helper now scans with the SIMD string helpers as requested (2fcf0d2, 1e3a599); output is unchanged and the dist-tag test additionally pins that ordinary non-ASCII text and backslashes pass through. #38525, #38536, #38615 and #38557 have been asked to carry the same body so the shared CI: build 97638 (the reworked helper) finished with every job that ran green, Windows x64 and aarch64 included, and one of the two macOS 14 arm64 shards among them; its failure list holds only tests the pipeline marks as flaky (each passed alone or on retry, none in this area). The build is red solely because the other macOS 14 arm64 shard expired on every automatic retry in that lane's fleet-wide queue backlog, which has been happening to builds all day; the earlier builds (96681, 96750) ended the same way. Re-pushing only re-enters the same queue, so this is left as is. Nothing in the diff is platform specific beyond the path separator the test already accounts for. |
|
Warning Review limit reached
Next review available in: 27 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 (21)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The --verbose resolve trace, the peer dependency warning, the dependency loop error, the isolated installer's enqueue failures, the verbose blocked scripts line, the unparsable specifier errors and the workspace range mismatch all print a resolution or a version specifier; route those through escape_control_chars like the other sites.
…messages Covers the candidates bun patch lists when a name is ambiguous, the resolution label in the patch apply error, the committish in the git errors, the folder path in "error occurred while resolving", and the resolution in the package.json parse errors.
There was a problem hiding this comment.
I reviewed this PR across four passes and the current revision (e83afa4) looks correct — the three missed-sibling sites raised earlier are all wrapped now, and the final sweep in e83afa4 covers the remaining Resolution / version-literal prints I could find. Because this is a 21-file security-hardening sweep that adds a new public bun_core::fmt helper and is coordinated with four sibling PRs (#38525, #38536, #38557, #38615, plus #37895 for the arg-order fix), a maintainer sign-off on the exclusion list and merge order would still be worthwhile.
What was reviewed
- The
EscapeControlCharswriter infmt.rs: C0/DEL as\xNN, C1 as\uNNNN, everything else passed through — ordinary output is byte-identical. - Each wrapped site against its unwrapped predecessor: same argument, only the formatter changed; no argument reordering except the intentionally-left-alone age-gate branch (#37895).
- The 13 tests: local registry / raw socket only,
test.concurrent, per-test cache dir, and each asserts both the escaped form and the absence of the raw byte.
Extended reasoning...
Overview
The PR wraps ~60 format-argument sites across 20 package-manager source files in escape_control_chars(...) / EscapeControlChars(...) so that resolutions, version specifiers, bin names, tarball URLs and registry error text cannot smuggle terminal escape sequences into bun's own output. It adds the escape helper to src/bun_core/fmt.rs (shared byte-for-byte with #38525/#38536/#38615) and a 13-case test file that drives each print path with a C0 or C1 control character and asserts the escaped spelling reaches stdout/stderr while the raw byte does not.
Security risks
The change is purely additive output hardening: it cannot make anything less safe than before. The helper passes non-control characters through unchanged, so ordinary names/versions/URLs render identically. The intentionally-excluded sites (--json output, bun pm hash-string, lifecycle-script/git output relay, pm view <field>) are documented in the description and are the places where byte-for-byte fidelity is the contract.
Level of scrutiny
Medium-high. Each individual hunk is mechanical (wrap an existing argument), and I verified the pattern is applied consistently. But the PR's correctness depends on the completeness of the sweep and the correctness of the exclusion list — three earlier passes each turned up a missed sibling (retry warning, resolve trace, bun patch prompt), and the author's response to the last one says every remaining Resolution-formatter and version-literal use in src/install and src/runtime/cli was audited. That claim, plus the merge coordination with four sibling PRs and #38557's differently-shaped helper of the same name, is the part a maintainer should confirm.
Other factors
All three of my earlier inline findings are resolved in b0c69d3 / 58a48bf / e83afa4, each with a test added. The pre-existing swapped-argument note on the age-gate message was intentionally left for #37895 to avoid a same-line conflict. The bug-hunting pass on e83afa4 found nothing. The test file follows harness conventions (tempDir, bunEnv, concurrent subprocess tests, per-test cache dir, raw-socket registry for the C1-in-status-line case, isWindows branch for the platform separator in the resolve-trace assertion), and each case asserts both the presence of the escaped form and not.toMatch(RAW_CONTROL) on the combined output.
There was a problem hiding this comment.
LGTM — the four earlier findings (retry warning, verbose resolve trace, bun patch candidate list, plus the argument-order note deferred to #37895) are all addressed, and this pass found nothing new.
What was reviewed:
- The
EscapeControlCharswriter infmt.rs:char_indices()slicing keeps char boundaries, C0/DEL as\xNN, C1 as\uNNNN, tail flushed;escape_control_charsroutes bytes throughBStrso invalid UTF-8 becomes U+FFFD before it reaches the writer. - Each call-site hunk is a pure wrap of the existing argument — no reordering (the one swapped pair is intentionally left for #37895), no new borrows that outlive the format call.
- Test file: 14 concurrent cases share one registry; the mutable
brokenTarballPathsadd in the isolated-installer test is scoped to a package name no other test touches, and the raw-socket registry test uses its own port-0 listener.
Extended reasoning...
Overview
Wraps ~40 package-manager print sites that emit dependency-/registry-controlled text (resolutions, specifiers, bin names, tarball URLs, registry status/error text, npm-notice/www-authenticate headers) in a new EscapeControlChars Display adapter so C0/DEL/C1 bytes come out as \xNN / \uNNNN instead of reaching the terminal. Twenty source files across src/install/ and src/runtime/cli/, plus the shared bun_core::fmt helper and a 14-case test file.
Security risks
The change is defensive hardening: it removes a terminal-escape-injection vector. It does not touch auth, crypto, permissions, or any parsing/validation path — only the display side. A mistake here degrades to the pre-PR behavior (raw byte printed) or over-escapes a benign string; the latter would surface in the existing bun-pm / bun-add / bun-why suites, which the author reports still pass.
Level of scrutiny
Medium — many files, but every hunk is the same mechanical transform (bstr::BStr::new(x) → escape_control_chars(x), res.fmt(..) → EscapeControlChars(res.fmt(..))). The one non-mechanical addition, the write_str impl, is short and I walked through it: byte offsets from char_indices(), start = i + c.len_utf8() keeps slice boundaries valid, final tail write covers the trailing run. The bstr::BStr wrapper for raw bytes means invalid UTF-8 is already U+FFFD before it hits the escaper, so no non-UTF-8 char boundary panic.
Other factors
This PR has been through four review rounds: each prior finding named a sibling print site sharing the same field, and each was fixed with a matching test in the follow-up commit. After the last round the author swept every remaining use of the Resolution formatter and version.literal in src/install / src/runtime/cli and documented what is intentionally left raw (cache keys, hashes, lockfile/JSON output, debug logs). The fmt.rs helper is byte-identical to the one carried by #38525/#38536/#38615 so whichever lands first collapses the others' hunk on rebase. The only commit since my last review is a CI retrigger — no code change.
|
Heads up on a small overlap: #38817 changes the URL argument of the two tarball |
EscapeControlChars now jumps between candidate bytes with strings::index_of_needs_escape_for_java_script_string instead of iterating every character; output is unchanged.
2 participants
Problem
1.4.0-canary.1, b7a0431) with a loopback registry; the same bytes come out on a pipe.+ name@resolutionin the install summary (src/install/lockfile/printer/tree_printer.rs),bun pm ls(src/runtime/cli/package_manager_command.rs),bun whyincluding its(requires ...)suffix (src/runtime/cli/why_command.rs), the--verboseresolve trace and theincorrect peer dependencywarning (src/install/PackageManager/PackageManagerEnqueue.rs), the dependency loop error (src/install/lockfile/Tree.rs), the isolated installer'sfailed to enqueue ...errors (src/install/isolated_install.rs) and its--verboseblocked-scripts line (src/install/PackageInstaller.rs), the candidate listbun patch <name>prints when the name is ambiguous (src/install/PackageManager/patchPackage.rs) and the patch apply error (src/install/patch_install.rs), the package.json parse errors (src/install/PackageManager/processDependencyList.rs) and the folder path inerror occurred while resolving(PackageManagerEnqueue.rs,install_with_manager.rs)installed x@y with binaries:afterbun add(tree_printer.rs)file:path of a dependency that cannot be resolved (PackageManagerEnqueue.rs~803-895), thex@range failed to resolvesummary (PackageManagerResolution.rs), a specifier bun cannot parse at all (src/install/dependency.rs), aworkspace:range no workspace satisfies (src/install/lockfile/Package.rs) and the committish of a git dependency in theno commit matching/invalid git commiterrors (src/install/repository.rs)dist.tarballURL when the download fails:GET <url> - 404, the... downloading tarball name@resolutionerror and its--verboseretry warning (src/install/PackageManager/runTasks.rs), and the isolated installer'sfailed to download name@version: ...report, which prints the URL on its own line (src/install/isolated_install/Installer.rs). C0 bytes in such a URL are rejected asInvalidURL(and the rejection message printed them raw); C1 characters are accepted and were printed raw on every path.errorfield (npm::response_error, used bybun pm viewandbun publish), thenpm-noticeandwww-authenticateheaders (src/install/npm.rs,src/runtime/cli/publish_command.rs), and theerrorfield of a packument (npm.rs). HTTP parsers reject C0 bytes in a status line or header but let bytes >= 0x80 through, so C1 controls are the live case there.BStr::new(..)or the lockfileResolutionformatter, neither of which touches control characters.Fix
bun_core::fmt::escape_control_chars(byte slices) orEscapeControlChars(..)(theResolutionand semver string formatters). It spells C0 controls and DEL as\n,\x1b,\x7fand a C1 control as a backslash-u escape of its code point (six characters, e.g. backslash,u,009b), and passes everything else through, so output for ordinary names, versions and URLs is unchanged.src/bun_core/fmt.rsis shared with install: escape control characters in the bun pm untrusted/trust script listing #38525 (bun pm untrusted), pm view: escape control characters coming from the registry #38536 (bun pm view) and install: reject dependency names containing control characters #38615 (rejecting dependency names with control characters): same name, signature and output, so the branches merge in any order. Per review it now finds the bytes to escape withstrings::index_of_needs_escape_for_java_script_string(one SIMD scan per run of clean text) instead of walking characters; the other three PRs still carry the earlier body and have been asked to adopt this one, so whichever lands first defines it and the rest collapse on rebase. ThePackageManagerResolution.rshunk is install: reject dependency names containing control characters #38615's. This PR covers the sites none of them touch. Arguments that are a package name (progress line, lifecycle-script errors,bun outdated,bun update -i, and the name half of the lines changed here) are left to install: reject dependency names containing control characters #38615, which refuses such names before they are printed or installed. audit: fail closed on malformed registry responses and escape control characters in the report #38557 (bun audit) currently carries a differentescape_control_charsunder the same name; that is already noted on that PR. The argument order of the minimum-release-age message is fixed separately in install: print version before package name in the minimum-release-age error #37895.bun pm view <pkg> <field>,--jsonoutput andbun pm whoamiprint a value the user asked for as-is (--jsonalready escapes);bun pm hash-stringand the--verbosehash dump print the exact bytes that were hashed; the output of a failing lifecycle script and ofgitis relayed unmodified; the HTTP client's--verboserequest trace and the security scanner's advisories are separate print paths and are not touched here.test/cli/install/escape-control-characters.test.ts(14 tests: install summary,pm ls,why, thebun patchcandidate list,bun addbin names, dist-tag,file:andworkspace:range errors, theInvalidURLreport,GET ... - 404, the--verboseresolve trace for the npm and workspace arms, the--verboseretry warning, the isolated installer's report, and a registry error response served from a raw socket so the status line can carry a C1 character). All 14 fail on the unfixed build and pass with this branch;bun-pm,bun-add,bun-pm-why,bun-audit,bun-patch,hoist,bun-workspacesand theoutdatedcases ofbun-install-registrystill pass.Background
Resolutionis how a package was resolved: an npm version (whose pre-release and build tags the semver parser restricts to[A-Za-z0-9.+-], so they cannot carry these bytes), or a git, GitHub, folder, workspace or tarball specifier, which is printed as the string the declaring package.json (or the workspace directory name) contained. Only the latter forms needed escaping, which is why theVersionformatters are untouched.EscapeControlChars<T>wraps anyDisplayvalue and escapes the text it writes, so formatters likeResolution::fmtare escaped in place rather than rendered into a buffer first.Probe output on the unfixed build
<C1>below stands for a raw U+009B character and<ESC>,<BEL>for the raw bytes; with this branch each of them prints as its escape instead (\x1b,\x07, and backslash-u009b), which is what the tests assert.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/escape-control-characters.test.ts