Bun.serve: cap chunk-extension bytes at 16 KiB per chunk - #34504
Conversation
The parser already counted chunk-extension bytes and capped them at MAX_CHUNK_EXTENSION_SIZE (16 KiB, matching Node/llhttp), but the check and the per-connection counter were gated to the node:http personality. A Bun.serve server therefore accepted unbounded chunk-extension bytes: a client could stream hundreds of MB of extension garbage inside one request (2-byte body, 200 OK) that maxRequestBodySize never sees. Move the counter into the base HttpResponseData so it exists for both server personalities, pass it unconditionally from HttpContext, and remove the IsNodeHttp gate on the six overflow checks and the per- request reset. Bun.serve now answers 413 and closes the connection the same way node:http (and every llhttp-based peer) already did.
|
Status: reproduced (32 MiB extension bytes accepted, 200 OK), fixed (413 + close), fail-before/pass-after verified. PR #34504. CI build 74724: 285/286 jobs passed; the one red lane is |
|
Updated 3:34 PM PT - Jul 17th, 2026
❌ @robobun, your commit 2fc4246 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34504That installs a local version of the PR into your bun-34504 --bun |
WalkthroughChangesChunk-extension byte tracking is shared across HTTP server personalities, and the parser now enforces a 16 KiB per-chunk extension limit. Regression tests cover oversized-extension rejection, smaller extensions across multiple chunks, and fragmented requests. Chunk extension enforcement
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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
🤖 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 `@test/js/bun/http/http-server-chunking.test.ts`:
- Around line 253-256: Update the chunking tests around the existing
extension-byte cases to cover the exact 16 KiB boundary: add an accepted chunk
with exactly 16 KiB of extensions and a rejected case with 16 KiB plus one byte,
while retaining multiple accepted chunks to verify the limit applies per chunk
rather than to the whole message.
- Line 259: Update both error handlers in the fixtures at
test/js/bun/http/http-server-chunking.test.ts lines 259-259 and 303-303 to
retain the reject function from Promise.withResolvers() and invoke it with the
socket failure; ensure both fixture promises reject immediately on error instead
of discarding failures.
🪄 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: 9b0ed5bd-91f2-4d04-ba15-f2a2bc63705a
📒 Files selected for processing (4)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpParser.hpackages/bun-uws/src/HttpResponseData.htest/js/bun/http/http-server-chunking.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The HTTP_PARSER_ERROR_CHUNK_EXTENSIONS_OVERFLOW doc comment still said 'node:http compat only' but the cap now applies to every server personality. The tiny-send-buffer test used hostname 'localhost' for Bun.serve, which on hosts where 'localhost' resolves to ::1 binds IPv6 only while the Bun.connect side tries IPv4, giving ECONNREFUSED. 127.0.0.1 is unambiguous.
There was a problem hiding this comment.
No issues found — the change is a clean, mechanical un-gating of the existing node:http chunk-extension cap so it also applies to Bun.serve, and the stale enum comment I flagged earlier is now fixed. Deferring only because this alters Bun.serve's request-acceptance behavior in the HTTP parser hot path (previously-accepted >16 KiB chunk-extension lines now 413), which is worth a human sign-off.
What was reviewed
- Verified
chunkedExtensionsByteCountis now always passed non-null from the soleconsumePostPaddedcaller, so the newly-unconditional dereferences are safe. - Confirmed
consumeHexNumberin ChunkedEncoding.h already null-checks the counter and resets it per chunk-size line, matching the "per chunk, not per message" contract the accept-test covers. - Checked the overflow check runs before each chunk dispatch, so no body bytes reach
fetch()on the reject path; the 413 is written by the existinghttpErrorResponsespath.
Extended reasoning...
Overview
The PR moves the chunkedExtensionsByteCount field from the node:http-only HttpResponseData<SSL, true> specialization into the shared base, passes it unconditionally from HttpContext::onData, and drops the IsNodeHttp && gate on the six overflow checks and the per-request reset in HttpParser.h. Two new tests in http-server-chunking.test.ts cover reject (20 KiB → 413) and accept (10×8 KiB per-chunk → 200), plus a drive-by pin of an existing test to 127.0.0.1. The stale "node:http compat only" comment on the HTTP_PARSER_ERROR_CHUNK_EXTENSIONS_OVERFLOW enum value that I flagged in a prior run was fixed in 2fc4246.
Security risks
This is itself a security hardening: it closes an unmetered-bandwidth / connection-holding vector where a hostile client could stream unbounded chunk-extension bytes past maxRequestBodySize. The counter, cap constant, and error path already existed and shipped for node:http servers; this PR only enables them for the Bun.serve template instantiation. I verified there is exactly one caller of consumePostPadded (HttpContext.h:340) and it now always passes a non-null counter pointer, so the newly-unconditional *chunkedExtensionsByteCount dereferences cannot fault. consumeHexNumber still null-checks its own pointer parameter, so any future caller passing nullptr remains safe at that layer. No new parsing logic is introduced, so there is no new smuggling/desync surface.
Level of scrutiny
High — packages/bun-uws/src/HttpParser.h is the request-line/body parser for every Bun.serve connection, and this change makes previously-accepted requests (>16 KiB of chunk-extension bytes on one chunk-size line) now return 413 and close. That is almost certainly desirable and matches Node/llhttp, but it is a user-visible behavior change to Bun's native HTTP server default, which per repo guidance is the kind of change a maintainer should explicitly sign off on rather than have auto-approved.
Other factors
All prior review threads are resolved: CodeRabbit's boundary-test suggestion was addressed (reject case tightened to 20 KiB in 8342a4e with a reasoned explanation for not pinning the exact byte, since the counter includes the leading ;), CodeRabbit's error-handler suggestion was declined as matching file convention (8 existing empty error() {} handlers in the same file, with close() guaranteed after error()), and my own stale-comment nit was fixed. The PR carries fail-before/pass-after evidence on both debug+ASAN and release builds, and the surrounding chunking/smuggling suites stayed green. The new tests' expect(stderr).toBe("") pattern matches six pre-existing instances in the same file. The 8-byte per-connection cost added to the Bun.serve HttpResponseData layout is negligible.
…eview - resolve HttpContext.h conflict with #34504: keep useLenientTransferEncoding and use the now-base-struct chunkedExtensionsByteCount - fallback: hoist onHttp1SocketEnd so the Upgrade/CONNECT handoff removes it before closing the parser - clear_on_data_callback: prefer armed_this_value over the socket-current wrapper when clearing the cached ondata slot - connectionListener test: reject on error/close for a useful failure message
2 participants
Repro
A hostile client can hold a connection, a parser, and unmetered inbound bandwidth with bytes that
maxRequestBodySizenever sees: the body is 2 bytes, the limit is 1 MiB, yet 32 MiB+ of chunk-extension garbage flows and the request completes 200.idleTimeoutnever fires while extension bytes trickle. Node (llhttp) and Bun's ownnode:httpserver reject the same request with 413/400 at ~16 KiB.Cause
HttpParser::MAX_CHUNK_EXTENSION_SIZE(16 KiB, matching Node'skMaxChunkExtensionsSize) already existed and the parser already counted extension bytes, but every overflow check was gated on theIsNodeHttptemplate parameter and the per-connection counter lived only in theHttpResponseData<SSL, true>(node:http) specialization; theBun.serveinstantiation passednullptrand its template contained no use of the counter.Fix
Move
chunkedExtensionsByteCountinto the baseHttpResponseData<SSL, false>so it exists for both server personalities, pass it unconditionally fromHttpContext, and drop theIsNodeHttp &&gate on the six overflow checks and the per-request reset inHttpParser.h.Bun.servenow answers413 Payload Too Largeand closes the connection when a single chunk's extension bytes exceed 16 KiB, matchingnode:httpand llhttp.The cap is per chunk (reset in
consumeHexNumberat the start of each chunk-size line, same as llhttp'son_chunk_header), not per message; a second test covers 10 chunks x 8 KiB extension = 80 KiB total still being accepted.Verification
Also green:
test/js/bun/http/request-smuggling.test.ts(75 pass),test/js/node/test/parallel/test-http-chunked-smuggling.js,test/js/node/http/node-http-server-timeouts.test.ts.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file