audit: fail closed on malformed registry responses and escape control characters in the report - #38557
audit: fail closed on malformed registry responses and escape control characters in the report#38557robobun wants to merge 1 commit into
Conversation
… characters in the report
A 200 response that is not `{ [package]: Advisory[] }` used to exit 0 from
`bun audit` (non-object bodies were echoed to stdout, objects of the wrong
shape printed nothing at all), while `--json` exited 1 for the same bodies.
Both modes now reject such a response with an error naming the audit URL
and exit 1, and a body that is not JSON is no longer written to stdout in
either mode.
Package names, titles, URLs and version ranges from the response were
printed byte for byte, so an advisory could emit terminal escape sequences
or overwrite lines of the report. They now go through
bun_core::fmt::escape_control_chars, which renders C0 controls, DEL and C1
controls as \n / \u001b style escapes.
A report whose advisories are all filtered out (or whose packages all have
empty advisory lists) prints "No vulnerabilities found" instead of nothing,
and `--json` exits 0 for it instead of 1.
|
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 (4)
Comment |
|
Status: ready for review. The diff is green on every CI lane that ran; the build's failed state comes from a lane that never started. Reproduced on bun 1.4.0-canary.1 with a local stub registry answering the bulk advisory POST: CI (build 96167): 177 of 179 jobs passed, |
|
Two notes from a branch that was about to fix the same escaping in
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes bun audit's exit-code contract (a CI security gate), adds a new public formatter to bun_core::fmt, and overlaps with the #38333 rewrite, a human sign-off would still be worthwhile.
What was reviewed:
bulk_advisoriesshape validation — confirmedJsonValue::as_array(notExpr::as_array) is used, so{"ms":[]}correctly yieldsSome(vec[])rather than being rejected.escape_control_charsbyte scanning — run/index bookkeeping, the0xC2 80..9FC1 detection, and truncated-UTF-8 safety viatext.get(i+1)all check out;hex_u16::<true>output is ASCII sowrite_bytesis sound.- Removal of the
if total > 0guard is safe: every retained advisory increments a severity bucket, so the summary is only reached withtotal >= 1. - Error path uses
redacted_npm_url, so auth material in the registry URL is not leaked to stderr.
Extended reasoning...
Overview
This PR hardens bun audit against malformed registry responses and terminal escape injection. It touches four files: src/runtime/cli/audit_command.rs (refactors audit() to parse once, adds bulk_advisories/reject_response, replaces BStr::new at print sites with escape_control_chars, and de-indents print_enhanced_audit_report after removing now-redundant if let wrappers), src/bun_core/fmt.rs (new escape_control_chars Display formatter for C0/DEL/C1 controls), test/cli/install/bun-audit.test.ts (21 new concurrent subprocess tests covering the response-shape × output-mode matrix), and docs/pm/cli/audit.mdx (one paragraph on the new exit-1 case).
Security risks
The change is net security-positive: it converts a fail-open (mirror returns {"error":...} → exit 0 → CI passes) into fail-closed, and neutralizes terminal escape sequences (CSI/OSC, CR line-forgery, C1 8-bit introducers) in registry-supplied strings. I checked that the new error path routes the URL through redacted_npm_url so tokens embedded in the registry href are not printed. The non-JSON body is never echoed; a JSON-but-wrong-shape body is still echoed under --json (per the documented contract) with exit 1 and an stderr error, which is a defensible choice but one a maintainer may want to confirm.
Level of scrutiny
Medium-high. bun audit is a security gate whose exit code is consumed by CI pipelines, so changing when it exits 0 vs 1 is user-visible and needs to be deliberate. The PR also adds public API surface to bun_core::fmt (a shared crate), and the description flags a known overlap with #38333 (multi-registry / audit fix rewrite) that will require coordination. None of this is a correctness concern — the code and tests look solid — but it exceeds the "simple, mechanical, obvious" bar for auto-approval.
Other factors
Test coverage is thorough and follows the repo's harness conventions (test.concurrent.each, await using for server/proc, Promise.all pipe draining, exact {stdout, stderr, exitCode} object assertions, port: 0, no external network). I verified the one subtle correctness edge: PropertyJSON.value is a JsonValue, whose as_array() returns Some for empty arrays (unlike Expr::as_array), so {"pkg":[]} is a clean audit as tested. The de-indented print_enhanced_audit_report preserves the original logic; the removed if total > 0 guard is provably dead once all_vulnerabilities is non-empty since every advisory increments exactly one bucket. The escape_control_chars design (backslashes pass through unchanged, so output is display-safe but not round-trippable) is appropriate for its stated purpose and is exercised by the \\d+ case in the escape test.
|
Two notes for whoever signs this off:
|
|
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..])
}
} |
… characters in the report (#38557)
2 participants
Problem
bun audittreats a 200 response that is not an advisory report as a clean audit. A body of[],null,"..."or a number is written to stdout and the command exits 0; an object whose values are not arrays (for example{"error":"audit is not supported"}from a mirror) prints nothing and exits 0.bun audit --jsonexits 1 for the same bodies, so the two modes disagree, and a misconfigured mirror or proxy reads as "no vulnerabilities" in CI.print_enhanced_audit_reportparse-failure branch, and--jsonechoed before parsing). bun audit outputs non readable text #35887 is this path in the wild: a registry served a gzip body withoutContent-Encodingandbun auditprinted the compressed bytes. With this change that case prints theresponse is not JSONerror instead; making that audit actually succeed is audit: decompress gzip responses served without Content-Encoding #35890, so bun audit outputs non readable text #35887 stays open after this PR (it is left to audit: decompress gzip responses served without Content-Encoding #35890).title,urlandvulnerable_versionsfrom the response are printed byte for byte (src/runtime/cli/audit_command.rs, theBStr::new(..)arguments inprint_enhanced_audit_report), so an advisory can emit terminal escape sequences (clear screen, set title, OSC 8 links) or use CR/LF to overwrite or forge report lines.print_enhanced_audit_reportonly counted advisories it could find and returned 0 otherwise, including for theelsebranch that echoed a non-object root (audit_command.rs~745-750 and ~975-980 before this change).Fix
audit()parses the body once and runs it through the newbulk_advisories(), which accepts exactly{ [package]: Advisory[] }(an object whose values are arrays of objects) and flattens it into(package, advisory)pairs. Anything else, and any body that is not JSON, goes toreject_response(): one error line on stderr naming the audit URL (error: audit request to <url> failed: response is not JSON/... is not a list of advisories) and exit 1, in both modes. The body is never echoed on the non-JSON path;--jsonstill relays a body that is JSON, as documented, and only its exit code changes.print_enhanced_audit_reportnow takes the validated pairs, printsNo vulnerabilities foundwhen nothing is left to report (covers{}, packages listed with empty arrays, and everything filtered out by--audit-level/--ignore, which used to print nothing), and returns 1 otherwise.--jsonexits on whether the response lists any advisory, so{"ms":[]}exits 0 like the human report does (it exited 1 before). Most of the diff in this function is de-indentation from removing theif letwrappers the validation made redundant (git diff -w).bun_core::fmt::escape_control_chars, a newDisplayformatter next toquote/escape_powershell: C0 controls, DEL and C1 controls (U+0080..U+009F) are rendered as\n,\r,\t,\^[,\^[and so on; everything else, backslashes included, passes through unchanged, and invalid UTF-8 is rendered the wayBStrrenders it. Other commands that print registry metadata (bun pm view, the security scanner output) are not touched here; they can adopt the same formatter.audit()and passed tosend_audit_request, so the error message names the URL that was actually requested.{}), unchanged on purpose: the previous code handled that case explicitly and some registries may answer that way; the new test pins it.test/cli/install/bun-audit.test.ts(newregistry responsesblock): 20 of the 21 new cases fail on the released bun (USE_SYSTEM_BUN=1) and all 38 tests in the file pass withbun bd test. The one case that passes both ways (an empty body is a clean audit) pins the behavior left unchanged. Also rancargo clippy -p bun_core -p bun_runtime,cargo fmt --check, and thesource-lintsbyte-search / dead-code tests.docs/pm/cli/audit.mdx.audit_command.rs(multi-registry audit,bun audit fix). This change is independent of it and will need a rebase if it lands first; as of its current head, that branch still exits 0 for{"error":...}-shaped bodies, still echoes a non-JSON body in--jsonmode, and still prints advisory strings unescaped, so the three pieces here (bulk_advisories,reject_response,escape_control_charsat the print sites) carry over as-is.Background
<registry>/-/npm/v1/security/advisories/bulk.bun auditPOSTs{ [package]: [versions] }and the registry answers{ [package]: Advisory[] }, where each advisory is an object withseverity,title,url,vulnerable_versionsandid; a package with nothing to report is normally omitted, so{}means clean. The registry is chosen by the project's.npmrc/bunfig.toml, which is why a mirror or proxy can answer with anything.C2 80..C2 9F, and xterm-like terminals honor them, which is why the formatter escapes that range as well as the ASCII range.pretty!/prettyln!rewrite<red>-style tags in the format string at compile time; interpolated arguments are written as-is, so escaping has to happen in the argument, which is what aDisplayformatter gives us without allocating.bun_json::ParsedJson::parse_jsonis strict JSON; a raw ESC byte inside a string is a parse error (checked), so a valid document can only carry DEL and C1 controls raw, and everything else reaches the report through\uXXXXescapes that the parser decodes. The formatter handles both forms.Before / after for the bodies from the report (stub registry answering every POST with the body, project with one npm dependency)
Before (bun 1.4.0-canary.1):
After: