Conversation
respond(), respondWithFile() and respondWithFD() share a port of node's prepareResponseHeadersObject() and validatePreparedResponseHeaders(): a :status that coerces to 0 becomes 200, the date default is added at that point, and the status range is checked before the never-index list. The file methods validate options first, then fd, then the headers, like node, so the synchronous status throw does not change which error wins on doubly invalid input. A never-index list that is not an array now throws from respondWithFile() and respondWithFD() instead of arriving later as a stream error, and statCheck sees the integer :status and the date.
|
Updated 4:39 PM PT - Sep 19th, 2026
✅ @robobun, your commit 93874fb11d7b66a551845383b8c2994be21c758b passed in 🧪 To try this PR locally: bunx bun-pr 43526That installs a local version of the PR into your bun-43526 --bun |
StatusReproduced against node v26.3.0 with the fixture from this PR: printf 'file body' > /tmp/body.txt
node test/js/node/http2/http2-response-status.fixture.js /tmp/body.txt > node.txt
bun test/js/node/http2/http2-response-status.fixture.js /tmp/body.txt > bun.txt
diff node.txt bun.txtNode prints 56 lines. Bun on the base branch (#43453) differs in three places:
With this branch the two outputs are identical. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/http2.ts— A statCheck callback that writes a non-numeric, empty, null or NaN :status now results in a 200 being sent silently after merging; on the base branch the same callback produced ERR_HTTP2_STATUS_INVALID through onError or the stream 'error' event. doSendFileFD at http2.ts:3043 calls this.respond(headers, options) after statCheck, and respond() at http2.ts:3590 re-runs prepareResponseHeadersObject, whose| 0 || 200at http2.ts:3186 rewrites the mutated value to 200. Fix: after statCheck, validate rather than re-default: range-check the statCheck result and reject a 0-coerced :status, or pass the prepared headers to a respond path that does not re-apply the default. [also at: src/js/node/http2.ts:3186 - Servers whose statCheck removes the date header, or resets :status to 0 to mean 'use default', get a header set different from node: the second prepare in respond() re-adds date and sets 200 silently. prepareResponseHeadersObject at src/js/node/http2.ts:3186-3190 runs once before fstat and again…]Extended reasoning...
respondWithFile is called with valid headers; http2.ts:3367 prepares :status to 200 and adds date. fstat completes and doSendFileFD:3022 calls options.statCheck(stat, headers, options). The callback sets headers[':status'] = someValue where someValue is null, '', NaN or a non-numeric string (for example a lookup that returned nothing). On the base branch respond() then ran
headers[':status'] |= 0giving 0 and threw ERR_HTTP2_STATUS_INVALID, caught at 3044 and delivered to onError or this.destroy, so the app saw the mistake. After this diff respond() calls prepareResponseHeadersObject (http2.ts:3590), which computes0 || 200and sends a 200 with the file body. Node does not re-prepare at all: it sends the raw value so the client observes a protocol failure, also an error. Bun is the only runtime that silently reports success. The PR text lists this as unchanged, but the base did not…Verification: nit — acknowledged in diff: the PR description's "Not changed here" bullet says "A statCheck that changes :status or deletes date. doSendFileFD() calls respond(), which prepares the headers again. Node does not check them again at that point" — that note is accurate about the mechanism but does not state the visible effect (a silent 200 where the base surfaced an error). Triggering…
-
🟣
src/js/node/http2.ts— A respondWithFD() caller whose fd fails fstat gets a 200 response header sent and then an INTERNAL_ERROR reset, and if the stream was destroyed meanwhile the process gets an uncaught throw. doSendFileFD's error branch at http2.ts:2965-2969 calls this.respond(headers, options) with no try/catch, unlike the guarded call at http2.ts:3042-3053, and unlike node's doSendFD which only calls this.destroy(err). Fix: on fstat error route to onError or this.destroy(err) without calling respond(), and wrap any remaining respond() in the same try/catch as line 3043.Extended reasoning...
stream.respondWithFD(fd, headers) is called with an fd that later fails fstat (already closed by the caller, EBADF), or respondWithFile hits a fstat failure. afterOpen at http2.ts:3132 calls fs.fstat; the callback lands in doSendFileFD with err set. Line 2965: if no onError is given, line 2967 runs this.respond(headers, options). respond() at http2.ts:3519-3525 throws ERR_HTTP2_INVALID_STREAM if the stream was destroyed during the fstat round trip (client RST arriving in between), and throws ERR_HTTP2_HEADERS_SENT if the app responded in the meantime. That throw happens inside an fs callback with no catch, so it is an uncaught exception that ends the process. When respond() does not throw, the client receives a 200 HEADERS frame followed by RST_STREAM INTERNAL_ERROR, while node destroys the stream with the fstat error and sends no headers. The base has the identical line, but this PR now prepares status and date before fstat so the headers object always looks valid here and the branch is reached with the same shape as the guarded call at 3043; the finder read the line but treated the…
Verification: pre-existing. Trigger: respondWithFD()/respondWithFile() without options.onError whose fstat fails (e.g. caller already closed the fd -> EBADF), and for the crash variant the stream is destroyed/closed (peer RST_STREAM, session close) between the call and the fstat callback. Mechanism verified in /home/claude/bun/src/js/node/http2.ts. doSendFileFD's error branch (lines 2960-2970) is: ``` if…
|
Overlap note: #43550 changes the same lines of #43550 also adds |
…m in file responses respond() prepares the headers again after statCheck. Its 200 default is for the caller's headers, so doSendFileFD() rejects a :status that statCheck replaced with a value that coerces to 0, as before the shared port. respondWithFile() and respondWithFD() throw ERR_HTTP2_INVALID_STREAM for a stream that is closed but not destroyed yet, like node.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Follow-up to the review, in 47e5dbd and 93874fb. A The The I shortened the new code comments to one line each. |
|
A note on an overlap with #43587. The test The two PRs have no text conflict. So the one that lands second makes |
This branch has not been deployed
1 participant
Stacked on #43453 (the base branch). Merge #43453 first.
Problem
respond(),respondWithFile()andrespondWithFD()throwERR_HTTP2_STATUS_INVALIDfor a:statusthat coerces to 0 (0,"",null,NaN,"abc"). Node v26.3.0 sends 200.fstat(client:RST_STREAMINTERNAL_ERROR). Node throwsERR_INVALID_ARG_VALUEfrom the call. Some checks run in another order than in node, so another error wins on doubly invalid input.src/js/node/http2.tshas its own copy of the:statusdefault and range check. None is node'sprepareResponseHeadersObject().Fix
prepareResponseHeadersObject()andvalidatePreparedResponseHeaders()replaces the copies::status | 0 || 200, the date default, the 200-599 range check, the never-index list check.options,fdand the headers, in node's order.respond()runs the port before it walks the header list.statChecknow sees the integer:statusand thedate, like node.test/js/node/http2/node-http2.test.js. One runs a fixture (56 calls) under Bun and under node and compares every line. It fails on the base branch. Also all oftest/js/node/http2/and the 256 vendoredtest-http2-*tests.Background
:status | 0turns"404"into 404 and a non-numeric value into 0. Node replaces 0 with 200.headers[http2.sensitiveHeaders]: header names that HPACK must not add to its table.respondWithFile()callsfstatbefore it sends headers. A later error reaches the user only throughoptions.onErroror the stream 'error' event.statCheckis a user callback that runs afterfstat.Notes
Node source:
prepareResponseHeadersObject,validatePreparedResponseHeaders,respondWithFD,respondWithFile. The| 0 || 200default and the< 200check are the same in node v20, v22 and v24 (processHeadersthere).No user report exists for this. It comes from a comparison with node. #43453 left the 0 to 200 default out on purpose ("pre-existing"). This PR takes it, because
.claude/docs/landing-prs.mdsays never validate stricter than node. The visible effect: a call that threwERR_HTTP2_STATUS_INVALIDfor:status0 now sends a 200, as node does.The fixture (
test/js/node/http2/http2-response-status.fixture.js) makes 56 calls, each on its own stream, and reports one line per call: what the call threw and what the client received. The test runs it in-process under Bun against an inline snapshot, then as a script under node, and compares the lines. Node v26.3.0 and this branch print the same 56 lines. Lines that differ on the base branch (-node and this branch,+base branch):The date default moves with the port. Node adds
datebefore it walks the header list. Two results follow, and both match node.respond({ date: "x\r\n" })sends no date: the value that cannot be sent is dropped and no default takes its place (before: a default date).respond({ Date: "x" })throwsERR_HTTP2_HEADER_SINGLE_VALUEfrom the JS check for single-value headers. Before, the same error came from the native header walk, after earlier fields were already in the HPACK table (#41520 covers that class of problem).Not changed here
additionalHeaders(): node:http2: validate the server stream :status like node #43453 and node:http2: let additionalHeaders() send a 1xx block on a HEAD request #43449 cover it.respond()with a falsy:status(respond([":status", 0])). Node prepends a default:status, finds two, and throwsERR_HTTP2_HEADER_SINGLE_VALUE. Bun throwsERR_HTTP2_STATUS_INVALID. An existing comment documents that choice. This PR adds what node does to that comment. The raw-array form gets the same range and never-index checks throughvalidatePreparedResponseHeaders().headersandoptions. Node throws fornullheaders and for a non-objectoptions. Bun accepts both. For a non-objectheadersthe message differs (must be of type object, node:must be an instance of Array or Object).statCheckthat changes the headers.doSendFileFD()callsrespond()afterstatCheck, andrespond()prepares the headers again. AdatethatstatCheckdeleted comes back (node sends none). A:statusthatstatCheckset to a value that coerces to 0 is still an error (ERR_HTTP2_STATUS_INVALIDthroughonErroror the stream 'error'), as on main. The 200 default is for the caller's headers only. Node sends such a value as is, and the client fails the stream. The second new test covers this.respond()on a stream that is closed but not destroyed. Node throwsERR_HTTP2_INVALID_STREAM, Bun does not. Internal callers reachrespond()(the implicit response in_write(), the compat layer), so that guard needs its own change.fstatfailure branch ofdoSendFileFD(), which callsrespond()before it destroys the stream: node:http2: a failed respondWithFD()/respondWithFile() calls respond() before it destroys the stream #43448, node:http2: do not call respond() when a file response fails before the headers are sent #43463.Review follow-up: the
statCheckcase above regressed in the first push of this PR (a silent 200) and the second push restores it. The second push also adds theclosedhalf of node'sdestroyed || closedguard to the two file methods.Self-review: 5 concerns raised, 4 addressed (check order on doubly invalid input, the never-index check missing from the file methods, the date default missing from the port, a node-run fixture in place of a hand-typed table). The fifth, the
:statuscopy inadditionalHeaders(), is covered by #43453 and #43449.Suites run with the debug build:
test/js/node/http2/*.test.*(all files), 256 vendoredtest/js/node/test/parallel/test-http2-*.js, grpc-jstest-server,test-server-errors,test-server-interceptors.[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file