node:http: honor insecureHTTPParser on the server - #33637
Conversation
The http.createServer insecureHTTPParser option was validated and stored on the Server instance but never reached the request parser, so the lenient behaviours it is meant to enable (obs-fold header continuation lines and Content-Length together with Transfer-Encoding: chunked) were still rejected with the same clientError as strict mode. Thread the option through setServerCustomOptions and the uws_app_set_flags chain into the uWS request parser and implement those two lenient cases: obs-fold CRLF is dropped in place inside the header value span (bytes after the value are untouched so consumed-byte accounting is unchanged), and when both Content-Length and Transfer-Encoding: chunked are present the request is accepted and chunked framing wins per RFC 9112 6.3. The default (strict) parse path is unchanged.
|
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 (3)
WalkthroughIntroduces an ChangesinsecureHTTPParser end-to-end plumbing
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:00 AM PT - Jul 7th, 2026
❌ @robobun, your commit cef740f has some failures in 🧪 To try this PR locally: bunx bun-pr 33637That installs a local version of the PR into your bun-33637 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate flag: #32488 threads the same The lenient behaviors implemented do not overlap though:
Happy to rebase on top of #32488, fold the obs-fold + CL+TE pieces into that branch, or leave this as the focused fix that lands first and gets reconciled when the omnibus merges, whichever is preferred. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-uws/src/HttpParser.h (1)
787-816: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn
shortRead()for an ambiguous obs-fold boundaryIn
insecureHTTPParsermode,\r\nat the end of the current buffer is still ambiguous: the next byte may beSP/HTAB. Falling through here makes the same request parse differently depending on chunk boundaries, so this path should wait for one more byte instead of treating it as the header terminator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bun-uws/src/HttpParser.h` around lines 787 - 816, The obs-fold handling in HttpParser.h is treating a trailing CRLF at the end of the current buffer as a completed header terminator in insecureHTTPParser mode, which makes parsing depend on chunk boundaries. Update the header-value scan around tryConsumeFieldValue and the obs-fold check so that when CRLF is seen but the next SP/HTAB byte is not yet available, the parser returns shortRead() instead of breaking as if the header ended; keep the existing maxHeaderSize and invalid-token error behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/server/server_body.rs`:
- Around line 3693-3719: The error strings in the server flag-setting path are
stale and reference the wrong options, so update the messages in the `set_flags`
flow to match what this routine actually configures. In the `server_body.rs`
handler around the `handle!` macro and the final `Err(global.throw(...))`,
replace the `requireHostHeader` and `timeout` wording with a single consistent
message that reflects setting server flags such as
`use_strict_method_validation` and `insecure_http_parser`, keeping the wording
aligned across all early-return and fallback error paths.
In `@test/js/node/http/node-http.test.ts`:
- Around line 2047-2126: The new insecureHTTPParser test cases in the
`describe("insecureHTTPParser")` block should be made concurrent since each
`runServer` call uses its own ephemeral server and socket I/O, so convert the
individual `test(...)` cases to `test.concurrent(...)` to match repo
conventions. Also add a dedicated case in this block for the leniency claim
around `Transfer-Encoding` without `chunked` by exercising `runServer(true,
...)` with a non-chunked transfer-encoding value and asserting it is still
rejected, alongside the existing `clientError` expectations, to lock in the
`HttpParser` behavior.
---
Outside diff comments:
In `@packages/bun-uws/src/HttpParser.h`:
- Around line 787-816: The obs-fold handling in HttpParser.h is treating a
trailing CRLF at the end of the current buffer as a completed header terminator
in insecureHTTPParser mode, which makes parsing depend on chunk boundaries.
Update the header-value scan around tryConsumeFieldValue and the obs-fold check
so that when CRLF is seen but the next SP/HTAB byte is not yet available, the
parser returns shortRead() instead of breaking as if the header ended; keep the
existing maxHeaderSize and invalid-token error behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4baac2d7-dbe0-4efb-9358-99cc6801caf5
📒 Files selected for processing (12)
packages/bun-uws/src/App.hpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpContextData.hpackages/bun-uws/src/HttpParser.hsrc/js/internal/http.tssrc/js/node/_http_server.tssrc/jsc/bindings/NodeHTTP.cppsrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/uws_sys/App.rssrc/uws_sys/libuwsockets.cpptest/js/node/http/node-http.test.ts
|
Re the outside-diff Verified by sending the same obs-folded request split at each of the four interesting boundaries (after the CR, after the LF, after the SP, and unsplit); all four parse to |
node-http.test.ts carries an unrelated non-hermetic proxy test that fails in sandboxed environments; split the new coverage into its own topic-specific file like the rest of this directory.
|
CI status: the diff itself is green. The new The remaining red is unrelated to this change:
Ready for review; needs a maintainer to merge past the unrelated flake (or rerun the x64-asan shard). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
When getHeaders compacts an obs-fold value in place and then returns shortRead (final CRLF-CRLF not yet received), the mutated bytes are appended to the fallback buffer and re-parsed. The stale tail bytes after the write cursor would then leak into the header value. Fill that tail with SP so the re-parse sees the same value (the trailing SP is removed by the existing whitespace trim).
When a folded header's value scan stops at a CRLF whose following byte has not yet arrived, the parser cannot tell whether the fold continues. Compacting there and then short-reading leaves SP-fill bytes that a later continuation folds into the middle of the value. Return shortRead before mutating the buffer in that case; the SP-fill is still needed for re-parses triggered by a later header.
There was a problem hiding this comment.
Both fragmentation issues I flagged are addressed in 57e71d3 and cef740f and I found nothing new this pass, but the in-place buffer mutation in getHeaders() plus the relaxed CL+TE guard are subtle enough that a maintainer familiar with the uWS parser should sign off.
Extended reasoning...
Overview
This PR threads insecureHTTPParser from http.createServer options through the JS→C++→Rust→uWS FFI chain into HttpParser.h, and implements two lenient behaviours there: RFC 7230 obs-fold header continuation (with in-place value compaction) and tolerating Content-Length alongside Transfer-Encoding: chunked. Twelve files touched across packages/bun-uws, src/uws_sys, src/runtime/server, src/jsc/bindings, src/js/node, plus a new dedicated test file.
Security risks
This is squarely security-sensitive: it modifies the HTTP/1.1 request parser's header-field scanning and the Transfer-Encoding/Content-Length conflict check — both canonical request-smuggling surfaces. The obs-fold implementation is the first place getHeaders() mutates its input buffer, breaking the prior invariant that a shortRead() return leaves the buffer untouched for the fallback re-parse. That invariant break already produced two client-controllable header-corruption bugs during review (both now fixed via the shortRead guard + SP-fill in cef740f). The lenient branches are gated on the flag and default off, and the strict path is byte-for-byte unchanged, so the default posture is unaffected — but the lenient path itself needs a careful human read.
Level of scrutiny
High. packages/bun-uws/src/HttpParser.h is the hot-path parser for every node:http/Bun.serve request; the fallback/re-parse machinery is subtle and this PR intentionally interacts with it in a new way. Two rounds of correctness fixes were needed to get the fragmentation semantics right. A maintainer with context on the fence-byte / fallback-buffer design should confirm the compaction is now fully idempotent under all shortRead orderings.
Other factors
- Overlaps textually with open PR #32488 (same
setFlags/HttpContextData/getHeadersplumbing, different lenient behaviour) — merge order needs a decision. - Test coverage is good: obs-fold (SP + HTAB), CL+TE:chunked, strict-mode rejection, and the two split-boundary regressions from my earlier reviews. The
setTimeout(r, 20)between socket writes is load-sensitive but consistent with the repo's TCP-yield convention for this test class. - CI on the diff itself is green per the author; remaining red is an unrelated lsquic LSan flake.
|
Build 69853 at cef740f shows the same pattern: the only hard failure is Both review rounds on the obs-fold compaction are addressed in 57e71d3 and cef740f. Ready for maintainer review. |
1 participant
What
The
http.createServer({ insecureHTTPParser: true })option was validated and stored on theServerinstance but never reached the request parser, so the lenient behaviours it is meant to enable were still rejected with the sameclientErroras strict mode. The client side already plumbed it through to llhttp; the server side (which parses with uWS, not llhttp) dropped it on the floor.Fix
Thread the option through
setServerCustomOptionsand theuws_app_set_flagschain into the uWS request parser, and implement the two canonical lenient cases it is expected to enable:one two, notone two).lenient_chunked_length): both headers are accepted, chunked framing wins per RFC 9112 6.3, and both headers remain visible inreq.headerslike Node. ATransfer-Encodingwithoutchunkedis still rejected since request body framing would otherwise be undefined here.The default (strict) parse path is unchanged; the new branches only execute when the option is explicitly set.
Verification
New
insecureHTTPParserdescribe intest/js/node/http/node-http.test.tscovers obs-fold (SP + HTAB, multi-line), CL+TE:chunked, and asserts the strict parser still rejects both. All assertions verified byte-identical under Node v26.3.0.