FormData: normalize lone CR and lone LF to CRLF in serialized multipart names and string values - #32975
Conversation
…ipart names and string values The WHATWG multipart/form-data encoding algorithm replaces every lone CR, lone LF, and CRLF in an entry's name (and in its value when it is not a File) with CRLF before the name is percent-encoded. Bun skipped that step, so a name like "a\rb" was emitted as name="a%0Db" and "a\nb" as name="a%0Ab" where Node, undici, and browsers all emit name="a%0D%0Ab". Non-File string values were emitted with their newlines unnormalized. Replace escape_form_data_name with encode_form_data_component, which takes the component kind (name, string value, filename) and applies the spec's two transforms accordingly. Filenames are still only percent-encoded, never normalized, matching the spec and Node.
|
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 (1)
Walkthrough
ChangesMultipart form-data component-aware encoding
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Updated 10:30 PM PT - Jun 28th, 2026
❌ @robobun, your commit 906d608 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32975That installs a local version of the PR into your bun-32975 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean spec-compliance fix with exact-byte test coverage matching Node's output.
Extended reasoning...
Overview
This PR modifies the multipart/form-data serializer in src/runtime/webcore/Blob.rs to implement step 1 of the WHATWG encoding algorithm: normalizing lone CR and lone LF to CRLF in entry names and non-File string values before percent-encoding. The single helper escape_form_data_name is replaced with encode_form_data_component, parameterized by a three-variant enum that selects which transforms apply (Name: normalize + escape; StringValue: normalize only; Filename: escape only). A new test in FormData-multipart-serialization.test.ts locks in the exact serialized bytes for all three newline spellings in names, values, and filenames.
Security risks
None introduced. The change only affects how CR/LF bytes already present in user-supplied form field names/values are encoded on the wire. The pre-existing percent-encoding of "/CR/LF in names and filenames (the header-injection guard) is preserved unchanged for those components. Normalizing lone CR/LF in string values to CRLF is, if anything, slightly safer for downstream parsers that mishandle bare CR.
Level of scrutiny
Low-to-moderate. This is a ~50-line localized change to a single pure helper function plus its three call sites in the same file, bringing output into byte-for-byte agreement with Node, undici, and browsers per the cited spec. I traced each FormDataComponent variant through the match arms: the fast-path None return correctly accounts for the per-component escape flag, the CRLF-pair lookahead correctly avoids double-expanding \r\n, the non-normalizing \r/\n arms are only reachable for Filename (which always escapes), and the StringValue path correctly leaves " literal while normalizing newlines.
Other factors
The new test asserts the full serialized body byte-for-byte, the PR description demonstrates the before/after diff against Node's wire output, and adjacent FormData/body suites (486 tests) were re-run clean. No CODEOWNERS cover these paths, the bug-hunting system found no issues, and there are no outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — focused spec-compliance fix with exact-bytes test coverage.
Extended reasoning...
Overview
This PR rewrites escape_form_data_name in src/runtime/webcore/Blob.rs into encode_form_data_component, which takes a FormDataComponent enum (Name / StringValue / Filename) and applies the correct combination of the two WHATWG multipart/form-data transforms: newline normalization (lone CR / lone LF / CRLF → CRLF) and percent-encoding of "/CR/LF. The three call sites in FormDataContext are updated to pass the right component kind, and a new test in FormData-multipart-serialization.test.ts locks in the exact serialized bytes for all three newline spellings in names, a string value, and a filename.
Security risks
None introduced. Names and filenames were already percent-encoded before this change, so header-injection / quoted-string-termination was already prevented; that behavior is preserved (the bare b'\r' / b'\n' arms still fire for Filename where normalize is false). String values now have lone CR/LF normalized to CRLF, which is what the spec mandates and what Node/browsers emit — this reduces, not increases, parser ambiguity on the receiving side. File contents are untouched.
Level of scrutiny
Low-to-medium. The change is ~50 lines in a single self-contained encoding function plus its three call sites, all within one file. I traced the match arms for each of the three FormDataComponent variants and confirmed: Name → normalize+escape (emits %0D%0A, consumes CRLF as one unit), StringValue → normalize only (emits literal \r\n, " falls through to the literal push since the if escape guard is false), Filename → escape only (if normalize guard is false so it falls through to the unchanged %0D / %0A arms). The early-return fast path correctly skips allocation when no relevant byte is present for the given component.
Other factors
No CODEOWNERS coverage for these paths. The bug hunter found no issues. The new test asserts the full wire format byte-for-byte against Node-matching output, and the PR description reports the adjacent FormData/body suites still pass (486/486). The PR is the serializer half of a pair with #32958 (parser side) but is independent of it.
|
CI status. No CI run of this diff has surfaced a failure related to the change. Across every run there is no failure in One agent is failing on three consecutive builds. Builds 66132 (2b3b028), 66580 (12f00b9), and 66595 (906d608) all fail The other failures, per build:
Build 66132: 274 jobs passed, 2 failed, 10 expired.
Build 66580: flagged flaky and green on retry: I spent the one |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Use SIMD or indexOfAny - these form data components can be quite large.
…ncoder encode_form_data_component scanned its input byte-by-byte, once to decide whether a copy is needed and again to build it. String values go through it now and can be large, so switch both to bun_core::strings::index_of_any (highway SIMD): the first hit doubles as the fast-path check, and the rewrite loop copies the ordinary runs between hits with extend_from_slice instead of pushing one byte at a time. Add a test with 4 KiB runs around each newline spelling so the bulk-copy path is exercised, not just the per-byte rewrites.
As of 12f00b9, The needle set differs per component ( I added a test that puts 4 KiB runs around each newline spelling in a value so the bulk-copy path is exercised, not just the per-byte rewrites. Output is still byte-identical to Node (same length and SHA-256 of the serialized body on the repro plus the 16 KiB value). |
…re-export bun_core::strings::index_of_any is explicitly re-exported from the scalar strings_impl shim (iter().position over the needle set), which shadows the immutable::* glob, so the previous commit never actually reached the SIMD scan. bun_core::immutable::index_of_any is the canonical version whose >= 2 needle path dispatches to highway::index_of_any_char; call it directly. No behavior change.
There was a problem hiding this comment.
LGTM — 12f00b9 addresses the scalar-vs-SIMD resolution; the remaining >4 GiB u32 nit is non-blocking.
Extended reasoning...
Overview
This PR replaces escape_form_data_name in src/runtime/webcore/Blob.rs with encode_form_data_component, implementing the WHATWG multipart/form-data encoding algorithm's CRLF-normalization step that Bun was missing: lone CR and lone LF in entry names and non-File string values are now normalized to CRLF before percent-encoding, matching Node/undici/browser output byte-for-byte. Filenames keep escape-only behavior per spec. Two new byte-exact tests in FormData-multipart-serialization.test.ts lock in all three newline spellings across names, string values, and filenames, plus a 16 KiB value to exercise the bulk-copy path.
Since my last review on bf33dc1, commit 12f00b9 swapped strings::index_of_any (the scalar shim) for bun_core::immutable::index_of_any, which actually dispatches to the highway SIMD scan — exactly the fix I'd suggested. I traced the three component variants through the new match arms (Name: normalize+escape → %0D%0A; StringValue: normalize-only → raw CRLF, " not in needle set so no spurious copy; Filename: escape-only → %0D/%0A stay distinct) and they line up with the spec table and the test expectations.
Security risks
This is the multipart serializer, so header-injection is the relevant threat surface. The change strictly tightens output: CR/LF/" in names and filenames are still percent-encoded (so they cannot terminate the quoted-string or inject Content-Disposition headers), and string values now emit canonical CRLF instead of lone CR/LF — which is what parsers already expect between part body and the next boundary. File contents remain untouched. No new escaping is removed. I see no security regression.
Level of scrutiny
Moderate. It's a ~60-line localized rewrite of one helper plus its three call sites, in a code path with existing byte-exact serialization tests (the pre-existing crlf\r\nname and weird"file\r\nname.html cases continue to pass unchanged). The fast path (None when no needle found → zero-copy) is preserved. CI on the prior identical-logic commits was clean modulo unrelated infra flakes; build #66580 on 12f00b9 is the only new run and the delta is a one-token import path change.
Other factors
The one new finding this round is a nit: immutable::index_of_any narrows to u32 via try_from().unwrap(), so a FormData string value with its first CR/LF past byte 2³² would panic. That requires ~15+ GiB resident on the outbound serialization path (the program builds the FormData itself), so it's not a realistic DoS — and the u32 index is the documented module-wide convention in string::immutable. Worth a one-line follow-up to call highway::index_of_any_char directly, but it doesn't block this fix.
…ndex narrowing bun_core::immutable::index_of_any wraps highway::index_of_any_char with a u32::try_from(i).unwrap() on the returned index, which panics once the index passes u32::MAX. FormData string values are the one component that can be that large (the UTF-8 encoding of a large JS string), and they are newly routed through this scan. Call the highway primitive directly: same SIMD scan, Option<usize>, no unwrap. No behavior change.
There was a problem hiding this comment.
LGTM — both review nits addressed in 12f00b9 and 906d608; bun_highway::index_of_any_char is the right call (SIMD scan, Option<usize>, no narrowing).
Extended reasoning...
Overview
Replaces escape_form_data_name in src/runtime/webcore/Blob.rs with encode_form_data_component, which applies the WHATWG multipart/form-data encoding algorithm's component-specific transforms: names get newline→CRLF normalization then "/CR/LF percent-encoding, string values get newline→CRLF normalization only, filenames get percent-encoding only (no normalization). The three push_string_slice call sites in on_entry now pass the appropriate FormDataComponent instead of a bare escape: bool. Two new tests in FormData-multipart-serialization.test.ts lock in exact wire bytes for all three newline spellings across all three component kinds, plus a 16 KiB value to exercise the bulk-copy path.
Review history
I reviewed this twice already. The first round (bf33dc1) flagged that strings::index_of_any resolved to the scalar iter().position shim rather than the SIMD scan the comment claimed; fixed in 12f00b9. The second round (12f00b9) flagged that immutable::index_of_any narrows the index to u32 via try_from().unwrap(), which a multi-GiB string value could in principle overflow; fixed in 906d608 by calling bun_highway::index_of_any_char directly (returns Option<usize>, same SIMD dispatch, already a workspace dep used in HashObject.rs). Both inline threads are resolved and the current diff matches what I asked for. The author also correctly noted that my suggested alternative index_of_any_pos_comptime is scalar — index_of_any_char is the right pick.
Security risks
None introduced. The pre-existing percent-encoding of "/CR/LF in names and filenames (the header-injection guard) is preserved; the new normalization step runs before escaping and only affects which percent-encoded sequence appears, never allowing a raw CR/LF/" into a Content-Disposition header. String values are part bodies, not headers, so emitting raw CRLF there is correct and spec-required. File contents remain untouched. The serializer is outbound-only (the program constructs the FormData), so this isn't parser-facing attack surface.
Level of scrutiny
Medium. This is production runtime code on the new Response(formData) / fetch({body: formData}) hot path and is a user-visible wire-format change for the (rare) case of lone CR/LF in field names or string values. But the change is small (~60 lines), spec-mandated, byte-identical to Node/undici/browsers per the PR's net-server capture, and covered by exact-byte assertions. I traced the match arms for all three (escape, normalize) combinations and the b'"' / _ if normalize / b'\r' / _ ordering is correct for each component's needle set.
Other factors
CI is green on the changed tests across all lanes; the only failures across three builds are unrelated infra (MySQL container health-check, Buildkite artifact-download timeout on darwin-26-aarch64) and known flakes that retried green. No CODEOWNERS entry covers src/runtime/webcore/. The bug-hunting system found nothing this round.
2 participants
What
The WHATWG multipart/form-data encoding algorithm starts by replacing every lone CR, lone LF, and CRLF in an entry's name (and in its value, when the value is not a File) with CRLF, and only then percent-encodes
"/CR/LF in names and filenames. Bun's serializer skipped the normalization step, so lone CR and lone LF survived into the escaped output and the same logical field name left Bun looking different than it does from Node, undici, or any browser.Raw bytes on the wire (recorded by a plain
netserver):Non-File string values have the same missing step:
fd.append("v", "x\ry\nz")serialized the value as the literal bytesx\ry\nzwhere Node emitsx\r\ny\r\nz.Servers that key on the decoded field name see a different name from Bun than from every other client, and the lone CR / lone LF vs CRLF distinction is exactly the ambiguity that parsers which re-normalize disagree on.
Fix
src/runtime/webcore/Blob.rs: replaceescape_form_data_namewithencode_form_data_component, which takes which piece of the entry it is encoding and applies the spec's transforms for it:"/CR/LFFilenames are deliberately left unnormalized: the spec's normalization step applies only to names and non-File values, and Node and browsers agree (a filename of
q\rw\neserializes asq%0Dw%0Aein both Node and Bun, before and after this change). File contents are untouched.After the fix Bun's output for the repro above is byte-identical to Node's.
This is the serializer half of the pair; #32958 fixes the inverse bug in the parser (the
%0D/%0A/%22escapes were never decoded back). The two are independent.Test
Added
normalizes lone CR and lone LF to CRLF in names and string valuestotest/js/web/html/FormData-multipart-serialization.test.ts, alongside the existing test that already covered the (previously correct) CRLF-in-name case. It locks in the exact serialized bytes for all three newline spellings of a name, a string value containing all three, and a filename containing lone CR / lone LF.Also re-ran the adjacent FormData/body suites (
FormData.test.ts,FormData-file-error-leak.test.ts,form-data-boundary-crash.test.ts,form-data-set-append.test.js,body.test.ts,content-length.test.js): 486 pass, 0 fail.