pm view: escape control characters coming from the registry - #38536
pm view: escape control characters coming from the registry#38536robobun wants to merge 1 commit into
Conversation
`bun pm view` / `bun info` printed every packument string byte for byte, so a package description, maintainer name, dist-tag, dependency name or dist field could emit terminal escape sequences (clear screen, set title, OSC 52 clipboard writes, OSC 8 hyperlinks, CR overwrites) or forge extra output lines. Add `bun_core::fmt::escape_control_chars`, which renders C0 controls, DEL and the C1 controls as visible escapes, and print every registry-controlled string through it. `bun pm view <pkg> <field>` uses the multi-line variant so readme-style fields keep their tabs and line breaks while a bare CR and everything else is still escaped.
|
Warning Review limit reached
Next review available in: 37 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 (3)
Comment |
|
Status: reproduced on bun 1.4.0 with a loopback registry serving control bytes in every packument field ( Related open PRs: #38525 adds the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — well-scoped terminal-escape hardening with thorough coverage.
What was reviewed:
EscapeControlCharsWriter::write_strchar-index/flush logic: C0/DEL/C1 ranges,\rpeek-for-\nin multiline mode, andstartadvancement viac.len_utf8()for 2-byte C1 chars all check out.- Traced every registry-controlled
BStr::newsite in the summary view — all now go throughescape_control_chars; remainingBStr::newuses are URL/auth-header construction and the JSON printer path, none registry-string-to-terminal. - Considered
\rsplit acrosswrite_strchunks — input isparse_utf8output viaBStr, so written as one chunk; worst case would over-escape, never under-escape. - Tests use a local
Bun.serveregistry, drain pipes concurrently, assert no control bytes remain via regex plus a full snapshot, and cover both the single-line and multiline variants.
Extended reasoning...
Overview
This PR hardens bun pm view / bun info against terminal escape injection from registry packuments. It adds two Display adapters to bun_core::fmt (EscapeControlChars for single-line fields, EscapeControlCharsMultiline for the field-view path where readme etc. are legitimately multi-line), plus &[u8] convenience wrappers over BStr. pm_view_command.rs then mechanically swaps BStr::new(x) for bun_fmt::escape_control_chars(x) at every print site that renders a registry-controlled string. Two new tests in bun-info.test.ts serve a hostile packument from a local Bun.serve and assert the summary view contains no raw control bytes (regex + full snapshot) and that the field view keeps CRLF/tab while escaping everything else.
Security risks
The change is strictly defensive — it adds escaping where there was none. The escape logic writes the passed-through prefix, then a visible escape (\\n, \\x1b, \\u009b), then advances start by c.len_utf8(); the C1 range '\u{80}'..='\u{9f}' is handled as chars so the 2-byte UTF-8 encoding is accounted for. Invalid UTF-8 from the registry is already normalized to U+FFFD by BStr's Display before it reaches the writer, so it can't smuggle a control byte through. The multiline variant's \r-followed-by-\n peek is per-write_str-call, but the only input is BStr over parse_utf8 output (one contiguous chunk), and any theoretical split would over-escape rather than leak a bare CR. No new attack surface is introduced.
Level of scrutiny
Medium. The helper is ~50 lines of straightforward char iteration in safe Rust with no allocation or unsafe; the call-site changes are a mechanical BStr::new → escape_control_chars swap. I walked every remaining BStr::new in pm_view_command.rs and confirmed they're either building the request URL/headers, printing the JSON-printer buffer (covered by #38150 per the description), or echoing the user's own prop_path in an error — none are registry text reaching the terminal. The PR notes output for clean packages is byte-identical, which the untouched existing snapshots in the same test file confirm.
Other factors
Test quality is high: local port: 0 server, tempDir + using, pipes drained via Promise.all, exit code asserted last, and the regex [\x00-\x09\x0b-\x1f\x7f\u0080-\u009f] gives a non-snapshot assertion that no control byte survives (only bun's own LFs are allowed). The hostile string exercises ESC/OSC/BEL, bare CR vs CRLF, tab, DEL, and both C1 controls the description calls out (NEL, CSI). The PR description also explicitly scopes out --json and bun audit with references to the PRs that handle them, and notes the helper matches #38525 so the two rebase cleanly.
|
Small doc nit from #38525, which adds the same helper: the rustdoc on |
|
Heads-up on the shared struct EscapeControlCharsWriter<'a, 'f>(&'a mut Formatter<'f>);
impl fmt::Write for EscapeControlCharsWriter<'_, '_> {
fn write_str(&mut self, s: &str) -> fmt::Result {
let bytes = s.as_bytes();
let mut start = 0;
let mut cursor = 0;
// `\` doubles as the quote char so the scan stops at nothing else extra.
while let Some(offset) =
strings::index_of_needs_escape_for_java_script_string(&bytes[cursor..], b'\\')
{
let i = cursor + offset as usize;
let (code_point, len) = match bytes[i] {
byte @ (0x00..=0x1F | 0x7F) => (byte as u32, 1),
0xC2 if matches!(bytes.get(i + 1), Some(0x80..=0x9F)) => (bytes[i + 1] as u32, 2),
byte => {
let char_len = strings::wtf8_byte_sequence_length(byte) as usize;
cursor = (i + char_len).min(bytes.len());
continue;
}
};
self.0.write_str(&s[start..i])?;
match code_point {
0x0A => self.0.write_str("\\n")?,
0x0D => self.0.write_str("\\r")?,
0x09 => self.0.write_str("\\t")?,
0x00..=0x7F => write!(self.0, "\\x{:02x}", code_point)?,
_ => write!(self.0, "\\u{:04x}", code_point)?,
}
start = i + len;
cursor = start;
}
self.0.write_str(&s[start..])
}
} |
2 participants
Problem
bun pm view <pkg>/bun info <pkg>write every string of the registry packument to the terminal byte for byte: description, license, homepage, keywords, dependency names and ranges,dist.tarball/shasum/integrity, dist-tag names, maintainer names and emails, and thetimeentry.ESC[2Jclears the screen,ESC]0;...BELsets the window title,ESC]52;...writes the clipboard,ESC]8;;urlturns a label such asreactinto a link to another host,\roverwrites the line bun printed so far, and an embedded\nforges further lines ofbun infooutput (for example a fakedependenciessection). 8-bit C1 controls (U+0080..U+009F,C2 80..C2 9Fin UTF-8) go through the same way. The same bytes come out on a pipe; bun only strips its own colors there.src/runtime/cli/pm_view_command.rsformats each field withBStr, which only replaces invalid UTF-8. The same applies tobun pm view <pkg> <field>for a string field (line 319).bun pm view hv | od -cshows the raw033bytes after bun's own color codes.Fix
bun_core::fmt::escape_control_chars/EscapeControlChars(src/bun_core/fmt.rs): aDisplayadapter that writes C0 controls, DEL and C1 controls as\n,\r,\t,\x1b,\x7f,\u009band passes every other character through unchanged. Same helper and output format as the one install: escape control characters in the bun pm untrusted/trust script listing #38525 adds for thebun pm untrustedlisting, so the two rebase onto each other cleanly; this one also addsescape_control_chars_multiline.pm viewprints every registry-controlled string through it. Fields of the summary view are single-line, so there a newline is escaped too; output for packages without control characters is byte-identical to before (the existingbun-info.test.tssnapshots still pass).bun pm view <pkg> <field>on a string field uses the multi-line variant:\t,\nand\r\npass through, since fields likereadmeare legitimately multi-line and the value is the whole output; a\rnot followed by\nand every other control is still escaped.test/cli/install/bun-info.test.ts("bun info with control characters in the packument"): a localBun.serveregistry puts ESC, BEL, bare CR, CRLF, tab, DEL, U+0085 and U+009B in every field; one test snapshots the whole summary view with each field escaped and asserts no control byte remains in stdout, the other checks the field view keeps CRLF and tab and escapes the rest. Both fail on the released binary (raw bytes in stdout) and pass with this change.bun-info.test.tsfile passes with the debug build;cargo fmt,cargo clippy -p bun_core,test/internal/source-lintsclean.--jsonand non-string fields (bun pm view <pkg> versions) are printed byjs_printer::print_json, which spells control characters as JavaScript\x1Bescapes and so produces invalid JSON. That is SyntaxError parsing package.json #4823 and is fixed by the open js_printer: preserve JSON escaping in package manifests #38150, which I confirmed also fixesbun pm view --jsonoutput.bun auditprints advisory titles and URLs the same raw way and can adopt the helper separately.Background
GET <registry>/<name>): top-leveldescription,dist-tags,maintainers,time, plus one manifest per version. Its free-text fields are whatever the publisher put inpackage.json, so forbun info <name>they are attacker-controlled for any package name the user is persuaded to look up.ESC [). In UTF-8 output they are two-byte sequences, which is why the adapter works on decoded chars rather than bytes; non-ASCII text such as»or a non-English description is not a control and is left alone.prettyln!only interprets<b>/<r>markup in the compile-time template; arguments are ordinaryDisplayvalues, so wrapping each argument is sufficient and the adapter is acore::fmt::Writeshim placed between the argument'sDisplayand the realFormatter.