pm licenses: escape control characters in text output (adds bun_core::fmt::escape_control_chars) - #39358
pm licenses: escape control characters in text output (adds bun_core::fmt::escape_control_chars)#39358robobun wants to merge 3 commits into
Conversation
…name@version column printable() only dropped ASCII controls, so C1 controls (U+0080..U+009F, which terminals execute like ESC sequences) went through raw in the license header and --long fields, and the name@version line was printed unsanitized altogether; a tarball or git resolution is text from a package.json. Print all of them through escape_control_chars, which spells C0 controls, DEL and C1 controls out as \n, \x1b, \u009b escapes instead of silently dropping them.
|
Status: ready for review. Reproduced on a debug build of main (8326d1b) with a project depending on a Fail-before / pass-after: the four control-character cases in This PR also carries the CI: build 99558 (first revision) was fully green; build 99745 (final revision, which only adds the name-column test case) is green on every lane except Windows 2019 x64, where |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe PR adds ChangesLicense output escaping
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, self-contained hardening of bun pm licenses text output.
What was reviewed:
EscapeControlCharswriter infmt.rs: traced the scan againstindex_of_needs_escape_for_javascript_string(stops at <0x20, ≥0x7F,\\) andwtf8_byte_sequence_length; every&s[start..i]slice lands on a UTF-8 char boundary because the input is&strand cursor only advances by whole chars.- C1 detection (
0xC2+0x80..=0x9F) and pass-through for U+00A0+ (same lead byte) and backslash — matches the new test's\\/©assertions. pm_licenses_command.rs: only the five text-output print sites change; grouping/sorting/version-matching still use the raw bytes and--jsonis untouched.printable()and theCowimport it needed are deleted.
Extended reasoning...
Overview
Three files: a new EscapeControlChars Display adapter added to src/bun_core/fmt.rs (~55 lines, pure safe Rust reusing the existing index_of_needs_escape_for_java_script_string SIMD scan and wtf8_byte_sequence_length); five call sites in src/runtime/cli/pm_licenses_command.rs::print_text swap printable() / raw BStr for escape_control_chars(), and the old printable() helper plus its Cow import are removed; and test/cli/install/bun-pm-licenses.test.ts updates three existing control-char tests from stripped→escaped expectations and adds one new test covering C1 controls, DEL, and the previously-unsanitized name@version column via a tarball specifier.
Security risks
The change reduces a terminal-injection risk: previously U+009B (single-byte CSI) and other C1 controls in package.json fields, plus any control byte in a tarball/git resolution string, reached the terminal raw. The new adapter renders C0, DEL, and C1 as \\n / \\xNN / \\uNNNN literals. Escaping happens only at the print layer, so grouping, sort keys, version equality against the lockfile, and --json output all still operate on the original bytes. There is no new parsing of untrusted input and no unsafe code.
Level of scrutiny
Moderate. bun pm licenses is a read-only diagnostic subcommand, and the diff is narrow: one new formatter and a handful of print-site substitutions. The only nontrivial logic is the char-boundary handling in write_str, which I verified — since the writer receives &str (valid UTF-8, including from bstr::BStr's Display, which emits U+FFFD for invalid input bytes), the scan always stops on either an ASCII byte or a UTF-8 lead byte, and start/cursor only advance by 1 (C0/DEL/backslash), 2 (C1), or the lead byte's declared length, so slicing cannot panic. Backslash falls through to the pass-through arm with char_len = 1, matching the documented behavior and the test.
Other factors
No CODEOWNERS entry covers these paths and there are no prior human review comments. The PR follows the repo's review checklist: dead code is deleted in the same commit, the helper lives in bun_core::fmt alongside escape_powershell_impl, tests go in the existing bun-pm-licenses.test.ts rather than a new file, and fail-before / pass-after was demonstrated (80/80 in the file). The one design choice — backslashes are not doubled, so the escaped form is for human display rather than round-trippable — is stated in the PR description and consistent with the --json path being the machine-readable one. The description also notes five sibling PRs add the identical fmt.rs hunk, so the shared placement is deliberate.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 7:50 PM PT - Aug 16th, 2026
❌ @robobun, your commit ac4cb95 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39358That installs a local version of the PR into your bun-39358 --bun |
robobun
changed the title
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it lands a new public bun_core::fmt helper that six other open PRs are meant to converge on (locking in the escape spelling and the backslash-pass-through choice), a human look at the API shape would still be worthwhile before it becomes the shared copy.
What was reviewed:
EscapeControlChars::write_str— cursor always stays on a UTF-8 char boundary (input is&str;wtf8_byte_sequence_lengthnever returns 0), so no infinite loop or non-boundary slice panic.- Backslash (0x5C) and non-C1
0xC2-prefixed chars (e.g. U+00A9) fall through the third match arm and pass through unchanged; the new test pins both. printable()and itsCowimport are deleted; every text-output print site inprint_textnow goes through the escaper (name and version were previously unsanitized).--jsonpath is untouched; tests confirm raw values round-trip.
Extended reasoning...
Overview
The PR replaces printable() (which dropped ASCII control bytes) in bun pm licenses with a new bun_core::fmt::EscapeControlChars<T: Display> adapter that spells out C0, DEL and C1 controls as visible escapes. It also extends coverage to the name and version columns, which were previously printed raw. Three existing tests are updated from stripped→escaped assertions, and one new test installs a tarball carrying U+009B in its filename, package name and manifest fields to cover the name/version column and the C1 range.
Security risks
This is a defensive change against terminal escape injection from untrusted package.json fields. The implementation covers the full control range (C0, DEL, C1) and applies at every text-output print site; --json is left to its existing JSON-string escaper. I don't see a way for the escaper to emit a raw control byte given valid &str input, and the &[u8] entry point routes through bstr::BStr's Display which replaces invalid sequences with U+FFFD before they reach write_str.
Level of scrutiny
The Rust logic is small and self-contained, and the test coverage is thorough (fail-before/pass-after verified per the description). What warrants a human look is not correctness but the fact that this hunk is explicitly the one six other open PRs (#38631, #38525, #38536, #38557, #38615, #38673) will rebase onto — the escape spelling (\\xNN for C0/DEL, \\uNNNN for C1, backslash left raw) and the generic-over-Display design become the shared contract. That is an API-shape decision a maintainer should sign off on.
Other factors
The two comment-cop inline flags on fmt.rs are resolved false positives (the section-banner comment is the file's convention across ~34 formatters; the rustdoc is a normal three-line doc comment). Dead code (printable, Cow import) is removed in the same PR.
1 participant
Problem
bun pm licenses(new in install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333, not in any release yet) prints text it did not write: thelicense,author,descriptionandhomepagefields of every installed package.json, and each package's name and resolution.printable()insrc/runtime/cli/pm_licenses_command.rs, only dropped bytes matchingu8::is_ascii_control. C1 controls (U+0080..U+009F; U+009B is the one-character form ofESC [, and xterm-style terminals execute it) went through raw in the license header and in the--longlines.name@versionline was not passed throughprintable()at all. The version column is the resolution rendered to text (resolution.fmt(...)at the top ofexec): for a tarball, folder or git dependency that is a path or URL taken from some package.json in the tree, and the name of such a package is whatever its own package.json says. A tarball nameddep-<U+009B>.tgzwhose manifest is nameddep-<U+009B>installs today and was printed as-is on both sides of the@.MIT<ESC>[31m<LF>EVILwas printed asMIT[31mEVIL.Fix
bun_core::fmt::escape_control_chars/EscapeControlChars<T: Display>: aDisplayadapter that writes C0 controls, DEL and C1 controls as\n,\r,\t,\xNN(other C0 and DEL) and\uNNNN(C1). Everything else passes through unchanged, backslashes included: the output is for reading,--jsonis the machine-readable form.pm licensesprints the license header, the name, the version and the--longfields through it;printable()is deleted.--jsonemits.--jsonis unchanged; its string escaping already makes the output parse back to the original bytes, which the tests check.char_indicesbody with the same output (pm view: escape control characters coming from the registry #38536 also adds a multi-line variant). They take this copy; pm view: escape control characters coming from the registry #38536 rebuilds its variant on it.\^[; it switches to this one and respells its assertions.redacted(..); with this helper generic overDisplaythat becomesEscapeControlChars(redacted(..)).test/cli/install/bun-pm-licenses.test.tsrather than by a Rust unit test:cargo test -p bun_coredoes not link (the crate's SIMD entry points such ashighway_memmemlive in the C++ build), and CI runs no unit tests for it.test/cli/install/bun-pm-licenses.test.ts: three existing control-character assertions move from the dropped to the escaped spelling, and a new case installs a tarball with U+009B in its file name and in its package name, and U+009B, DEL, a backslash and U+00A9 (same UTF-8 lead byte as the C1 range) in the manifest fields, then checks every line of the text output and the--jsonround trip. Against a build of main these four tests fail (raw U+009B in the output,MIT[31mEVILinstead ofMIT\x1b[31m\nEVIL); with the fix all 80 tests in the file pass.test/internal/source-lintspasses;cargo clippy -p bun_coreis clean.Background
C2 80..C2 9F; terminals treat several of them as one-byte equivalents of ESC sequences (U+009B = CSI, U+009D = OSC), so a byte-levelis_ascii_controlfilter does not cover them.Resolutionformatted as text: the version number for registry packages, otherwise the tarball path or URL, the git URL plus commit, or the folder path. The name column is the lockfile package name, which for tarball, folder and git packages is copied from the package's own package.json (install: reject dependency names containing control characters #38615 and install: reject tarball, folder and git packages whose package.json name is invalid #38633 are about rejecting such names at install time; this PR escapes whatever reaches the printer, and if they land the new test's package name moves to its tarball file name only).escape_control_charsfinds candidates withstrings::index_of_needs_escape_for_java_script_string, a SIMD scan whose stop set is everything outside 0x20..0x7E plus the quote character (\is passed as the quote so nothing extra is added), and only inspects those positions, stepping over ordinary multi-byte characters withwtf8_byte_sequence_length.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file