http: escape control characters in the verbose request/response trace - #38673
http: escape control characters in the verbose request/response trace#38673robobun wants to merge 1 commit into
Conversation
The trace printed for fetch(..., { verbose }) and bun install --verbose wrote
the request URL, header names and values, and the response status text to
stderr byte for byte. HTTP/1.1 lets a peer put any byte >= 0x80 (including
UTF-8 encoded C1 controls such as U+009B, the one-byte CSI) in a field value
or reason phrase, HPACK/QPACK only reject NUL, CR and LF, and the tarball URL
bun install prints comes straight out of a dependency's metadata, so a peer or
a dependency could emit terminal control sequences through the trace.
Print those fields through bun_core::fmt::escape_control_chars, which spells
out C0 controls, DEL and C1 controls, and have redacted_npm_url render its
runs via BStr so a URL that is not valid UTF-8 shows up as U+FFFD instead of
being reinterpreted as a str inside the escaping wrapper.
|
Warning Review limit reached
Next review available in: 5 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 (5)
Comment |
Status
|
1 participant
Problem
fetch(url, { verbose: true | "curl" }),BUN_CONFIG_VERBOSE_FETCH,bun install --verbose) writes the request URL, every request and response header name and value, and the response status text to stderr byte for byte:print_requestinsrc/http/lib.rs:1439-1459and theDisplayimpls forHeader,HeaderCurlFormatter,RequestCurlFormatterandResponseinsrc/picohttp/lib.rs.is_malformed_response_value), so ESC gets through there too.bun installpasses the tarball URL from a dependency'spackage.json/ registry manifest to the trace unmodified.bun install --verbosewith"dep": "http://127.0.0.1:PORT/dep-\u009b31m.tgz"): stderr receivesHTTP/1.1 GET http://127.0.0.1:PORT/dep-<U+009B>31m.tgz; a 404 response withx-evil: a<C2 9B>31mbprints< x-evil: a<U+009B>31mb. U+009B is the one-byte CSI, so<U+009B>31mis a colour change when a terminal reads it, and CSI/OSC sequences in general can erase or repaint the lines around them.Fix
bun_core::fmt::escape_control_chars/EscapeControlChars(the same hunk as install: escape control characters in resolutions, specifiers, bin names and registry error text #38631, install: escape control characters in the bun pm untrusted/trust script listing #38525 and install: reject dependency names containing control characters #38615, so whichever lands first the others merge cleanly) and prints every peer- or dependency-supplied field of the trace through it: the request URL, header names and values in both the>/<lines and thecurlline, the redactedAuthorizationscheme, and the response status text.Headers(used to build the WebSocket upgrade request) and the method (aMethodenum name) are untouched.Displayimpls rather than around them, because those impls also emit bun's own ANSI colour codes when stderr is a TTY; only the data is escaped (checked by hand withFORCE_COLOR=1).EscapeControlChars(redacted_npm_url(url)), so redaction runs on the real bytes and escaping on the result. For that to be soundRedactedNpmUrlFormatternow writes its runs throughBStrinstead ofwrite_bytes(from_utf8_unchecked):bun installhands the trace whatever bytes thepackage.jsoncontained, and a Latin-1 file gets bytes that are not UTF-8 (verified: a dependency URL containing82 41 9B 41reachesprint_requestas-is). Runs only ever split at ASCII bytes, so valid UTF-8 renders exactly as before; invalid sequences now render as U+FFFD, which is how the package manager's own error lines already print them.\u009b,\x1b,\r) rather than dropped, matching the convention the sibling PRs establish for the rest of the install output. The request body in thecurlline is already a JSON string literal (C0 controls escaped) and is the caller's own data, so it is left alone.test/js/web/fetch/fetch.test.ts"verbose fetch logging escapes control characters coming from the peer": raw TCP server sends a reason phrase and a header value containing U+009B, the client sendsx-reqandAuthorizationvalues containing it,verbose: "curl". Covers the>header lines, the redactedAuthorizationbranch, thecurlline and the<status and header lines. Fails on the released binary (the raw U+009B is printed in all five places), passes with this branch.test/cli/install/bun-install.test.ts"--verbose escapes control characters in the tarball URL and response headers it traces":package.jsonwith one tarball URL containing U+009B and one containing raw non-UTF-8 bytes, against a local 404 server. Covers the request-line URL path (whichfetch()cannot reach, since it percent-encodes the path) and the non-UTF-8 rendering. Fails on the released binary, passes with this branch.bun bd test test/regression/issue/12042.test.ts(curl trace body) andtest/cli/install/redacted-config-logs.test.ts(URL redaction) still pass; the remaining failures when running the two edited files in full here arelocalhost/internet-dependent tests that fail identically with the released binary.Background
print_request(HTTP/1.1, h2 and h3 all call it) andprint_responseinsrc/http/lib.rs; the line formatting for headers and the response lives in theDisplayimpls of thebun_picohttpcrate.HTTPVerboseLevel::Headersprints the>/<lines,HTTPVerboseLevel::Curladditionally prints a copy-pasteablecurlcommand first;bun install --verboseusesHeaders.ESC [, CSI; U+009D =ESC ], OSC), and in UTF-8 mode xterm and others recognise them when they arrive UTF-8 encoded (C2 9B), which is the form that passes through an HTTP/1.1 parser.bstr::BStr'sDisplaywrites bytes as UTF-8 and substitutes U+FFFD for invalid sequences, which is what the rest of the trace already used;escape_control_chars(bytes)builds on it, andEscapeControlChars<T: Display>escapes whatever the innerDisplaywrites, so it must only ever be fed valid&strchunks (the reason for theRedactedNpmUrlFormatterchange).redacted_npm_urlreplaces URL passwords, UUIDs andnpm_tokens with***; it is the existing formatter for printing registry/tarball URLs.