Repository navigation
Conversation
additionalHeaders() threw ERR_HTTP2_PAYLOAD_FORBIDDEN when the request method was HEAD. node throws that error from respondWithFD() and respondWithFile() only. A 1xx block never carries a payload. The compat API calls additionalHeaders() from writeContinue(), writeInformation() and writeEarlyHints(). With no 'checkContinue' listener it calls writeContinue() from inside its 'stream' handler, so a HEAD request with expect: 100-continue was an uncaught exception in the server.
|
Status Reproduced on bun 1.4.3 against node v26.3.0:
The repro scripts are in the Notes block of the PR description. Fail-before: Not in this PR: #43447 (other CI: build 118324 passed, 181 of 181 jobs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe change removes payload-forbidden validation from HTTP/2 ChangesHTTP/2 HEAD request handling
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a small, well-scoped deletion that brings additionalHeaders() in line with Node, with tests for both the core API and the compat writeContinue() path.
What was reviewed:
- The removed block in
src/js/node/http2.ts(additionalHeaders): the 204/205/304 arms were unreachable after the preceding 1xx range check, and Node'sadditionalHeaders()has noheadRequestpayload guard, so only the divergent arm was live. - Sibling
ERR_HTTP2_PAYLOAD_FORBIDDENguards inrespondWithFD/respondWithFile(lines ~3340, ~3399) and therespond()check at ~3594 are untouched and still match Node; the status constants remain in use there. - Callers
writeContinue/writeInformation/writeEarlyHintsand the compatonServerStreampath (line 1247) — the uncaught-exception scenario on HEAD +expect: 100-continueis now exercised by the second test; the client-sidecontinue/headersemission (~5111) supports the asserted event order. - Candidate concerns about sending a 1xx HEADERS frame on a HALF_CLOSED_REMOTE or pushed HEAD stream were examined and ruled out as pre-existing behavior shared with GET streams, not introduced here.
Extended reasoning...
Overview
The PR removes a ten-line guard from ServerHttp2Stream.additionalHeaders() in /home/claude/bun/src/js/node/http2.ts. The guard sat inside the hasStatus branch after the code had already rejected any status outside 100–199, so the 204/205/304 comparisons could never match; the only live arm was this.headRequest, which threw ERR_HTTP2_PAYLOAD_FORBIDDEN for every 1xx block on a HEAD request. Node's additionalHeaders() performs the destroyed/headersSent/101/1xx-range checks and then sends the block; the payload-forbidden error is only raised from respondWithFD() and respondWithFile(). Those sibling guards (and the related one in respond()) are left intact in this diff, and the HTTP_STATUS_* constants they use remain referenced, so nothing becomes dead. Two tests were appended to /home/claude/bun/test/js/node/http2/node-http2.test.js: a core-API test asserting a 103 headers event followed by a 200 response on a HEAD request, and a compat-server test asserting continue then response 200 for a HEAD request carrying expect: 100-continue.
Security risks
None introduced. The change is in the direction of robustness: before, any peer could send a HEAD request with expect: 100-continue to a compat-API server with no checkContinue listener and trigger an uncaught exception from inside the internal 'stream' handler (onServerStream → response.writeContinue() → additionalHeaders()), with no user frame on the stack to catch it. Removing the throw closes that remote crash trigger. No input validation is weakened — the 101 and 1xx-range checks still run first.
Level of scrutiny
Low-to-moderate. This is a pure deletion in a node: compat module, with the reference implementation (Node's lib/internal/http2/core.js) as the spec, and the deleted code was demonstrably divergent from it. I checked the callers (writeContinue, writeInformation, writeEarlyHints, onServerStream) and the client-side header dispatch (continue emitted for status 100, headers emitted for 1xx before the response) to confirm the tests' asserted event sequences are what the runtime produces. The bug hunt exited on dry_streak with no findings; the four candidates it investigated (1xx frames on HALF_CLOSED_REMOTE or pushed HEAD streams, double 200 blocks when no :status is given) are all behaviors that already exist for GET streams on the base branch and are not caused by this diff. The PR description explicitly names the remaining :status divergences as tracked separately, which satisfies the "deliberate exclusion must be stated" convention.
Other factors
I was not able to execute the test file in this environment (no debug build present and test execution was not permitted), so my verification of the tests is static: they use port: 0, wire error on both client and request to reject, resolve on close, assert exact event arrays with toEqual, and clean up in finally. Per the PR description both tests fail on stock Bun, which is consistent with the code path — on the base branch the first test would record ["additionalHeaders threw", "ERR_HTTP2_PAYLOAD_FORBIDDEN"] and the second would surface an uncaught exception. The changed paths are not covered by .github/CODEOWNERS, and the timeline shows no outstanding third-party objections.
…6270) ### What does this PR do? Hardening follow-up to #46021. It fixes regressions from #46021 and #44618 that were never released, includes more of the open pull requests that fix a crash, and fixes a few crashes that had no pull request. #### Regressions fixed (none was released) - `spyOn(Bun, "$")`, and the same for `"sql"`, `"postgres"` and `"SQL"`, threw "does not support accessor properties yet". - Async `fs.read`, `fs.write` and `zlib.brotliCompress` on a view over a `WebAssembly.Memory` could lose data or copy far more than they needed. - `node:vm` with `vm.constants.DONT_CONTEXTIFY`: `this` in host accessors, prototype handling, `util.types.isProxy`, `Bun.deepEquals`, `assert.deepEqual` and `console.log` of a context. - `zlib` `stream.flush()` threw for arguments it used to accept, and streams had a new enumerable property. - `Bun.Transpiler` `exports.replace` with the name `"default"`. - `process.env.X = 3000` could store the number once the function was JIT-compiled. - `node:sqlite` and `bun:sqlite` could lose rows at exit. - Dev server: stylesheets shared between pages after a build error. - Shell `cp` of a file onto itself exited 1. - `error.stack`: the frame of `require()` reads `at anonymous (unknown)` again. - `import * as process from "process"` followed by `process.on(...)` threw at load. The EventEmitter methods that the module exports now act on `process`. - `delete res.statusCode; res.end()` in a `node:http` handler threw. It sends 200, as in Node. (from #46128) - `node:tls`, `node:https` and `node:http2`: connection teardown and write callbacks behave as in 1.4.3 and Node again. #### Crash fixes Bundler, parser and CSS: #42286, #46092, #46097, #46177, #46239, #36334, #43024, #40769. `bun test`, `--hot` and `--watch`: #30776, #38206, #38974. Shell: #43183, #37743, #46168, #46207, #34900. `node:http`, `node:http2`, `Bun.serve`, streams: #43449, #45380, #44457, #42197, #42485, #38082, #38124, #46074, #45999, #46045. `node:vm`, native addons, workers: #46129, #46198, #45615, #33412, #38031. Dev server: #42790, #42850, #39488. Other: #35991, #38030, #38586, #40526, #41830, #46029, #46078, #46114, and the bug of #42215. #### Behavior changes - `crypto.randomFill(view, cb)` reports `ERR_CRYPTO_OPERATION_FAILED` if it could not fill the view. It used to report success. - A function registered twice on `process` runs twice, as in Node. (#41830) - Under `--hot`, a reload after an uncaught exception cancels the timers that the failed run created. The new run's timers fire, and so do timers and `fs.watchFile` watchers that earlier runs kept on `globalThis`. The reload also resets the exit code to 0. (#38206) - The parent of a Worker no longer gets an `'error'` event for a failed `Bun.serve` request in that Worker. The error is printed, as on the main thread. - `hash.write()` after `digest()` drops the chunk, as in Node. (#35991) - `http2` over TLS with `server.emit("connection", socket)`: `session.socket` is a `TLSSocket` and a `'connection'` listener fires, as in Node. (#38124) - A `slice()` with no type of a body `Blob` has `type === ""`, as in Node. (#46114) - `util.types.isProxy(globalThis)` is false, as in Node. - A write on a `node:net` or `node:tls` socket that is still pending when the connection closes calls back with `ECANCELED`, as in Node. - `HTMLRewriter.on()` throws for a selector that is nested too deeply. - `Bun.Image`: `maxPixels` also bounds PNG metadata, `metadata()` of a GIF reports the size that gets decoded, and a path with a NUL byte throws. (#40526) - `bun build --no-bundle` with the `base64` or `dataurl` loader prints an error. (#36334) - Bundled output for a default-exported function or class expression changes to `var file_default = function name() {}`, as in esbuild. `.name` is unchanged. (#46097) - A Proxy in the prototype chain of a `node:vm` global is allowed. jsdom 30 with `runScripts` needs this. - `bun:sqlite` commits a write that `iterate()` left open when the main thread exits. An explicit `BEGIN` is still rolled back. - When a Worker stops while a native addon's async work is still running, the addon's finalizers run after that work is done, as in Node. - Shell: `mv -n` and `cp -n` don't overwrite, and `cp` reads `R`, `v` and `n` anywhere in a flag group. #### Performance - Ordinary `node:vm` contexts run at the same speed. With `DONT_CONTEXTIFY`, stores to globals and reads of inherited names in a hot loop cost 10 to 20 % more than on `main`. - A `process.env` write costs 20 to 35 % less than after #46021. - An async read into a `WebAssembly.Memory` view looks through the live memories: about 1.5 µs more with one memory. #### Not included - Taken in part, so these should stay open for the rest: #36334, #38974, #39488, #40526, #40769, #42485, #45615, #45999, #46029, #46092, #46114, #46168, #46207, #46239. - Needs a WebKit change: #34655 #39204 #40170 #40253 #41587 #41900 #42699 #42897 #42903 #43084 #43813 #44518 #44949 #45770 #45804 #45827. - #44723 (covered by #46097), #41675, #42753, #38762, #41475, #46016. ### How did you verify your code works? - Every item was reproduced first on release builds of `main`, of its parent and of 1.4.3, and compared with Node v26 where Node has the API. - Every fix has a test that fails on `main` and passes on a debug + ASAN build of this branch, except where that isn't practical (huge inputs, code that only runs on Windows or macOS). - The regressions were checked on grids of calls against the build before #46021 and against 1.4.3. - Bun's own test suites and Node's `test-vm-*`, `test-http2-*` and `test-zlib-*` files for the areas touched.
|
Closing: #46270 (commit a950fc5) landed this change on
Only in this PR, and tests only: a core API variant of the 103 test ( Reopen this PR if something is missing. |
Problem
ServerHttp2Stream#additionalHeaders()throwsERR_HTTP2_PAYLOAD_FORBIDDENwhen the request method is HEAD. Node v26.3.0 sends the 1xx block.'checkContinue'listener, the compat API callswriteContinue()inside its own'stream'handler (src/js/node/http2.ts:1247). One HEAD request withexpect: 100-continueis then an uncaught exception in the server.additionalHeaders()(src/js/node/http2.ts:3480, from feat(node:http2) Implement HTTP2 server support #14286). The status is already known to be 1xx there, so only thethis.headRequestarm can match.Fix
additionalHeaders()does not have it. Node throws this error fromrespondWithFD()andrespondWithFile()only.respond()still ends a HEAD stream on its HEADERS frame.:statusdifferences from Node (node:http2: ServerHttp2Stream checks:statusdifferently than Node (no integer conversion, respond() accepts 1xx) #43447) and therespond()calls in the file responder error paths (node:http2: a failed respondWithFD()/respondWithFile() calls respond() before it destroys the stream #43448). One effect: a HEAD stream now sends the string"101"as a 101 block, like a GET stream does (node:http2: ServerHttp2Stream checks:statusdifferently than Node (no integer conversion, respond() accepts 1xx) #43447). Before, it threw.test/js/node/http2/node-http2.test.js(two new tests, stock bun fails both). Also 46 vendoredtest-http2-*tests. Self-reviewed: 1 concern raised, 1 addressed (the two trackers above).Background
:status(100, 103). A server sends it before the final response headers.additionalHeaders()sends it. The compat methodswriteContinue(),writeInformation()andwriteEarlyHints()calladditionalHeaders().headRequestis true on a server stream whose request method is HEAD. That response has no body, sorespond()ends the stream and the file responders throwERR_HTTP2_PAYLOAD_FORBIDDEN.Notes
Shortest repro. Start the server, then send one request with curl. Bun 1.4.3 prints
error: Responses with 100 status must not have a payloadand exits with code 1. Node v26.3.0 and this branch answerHTTP/2 100andHTTP/2 200.The stack on bun 1.4.3 is
additionalHeaders<-writeContinue<-onServerStream<-emit<-streamHeaders. No user code is on it, so atry/catchin the request handler does not help.Core API repro. Node v26.3.0 prints
HEAD additionalHeaders: sentandHEAD client got info headers: 103. Bun 1.4.3 printsHEAD additionalHeaders: ERR_HTTP2_PAYLOAD_FORBIDDENand the client never gets the 103. With this change the output is identical to node.Other checks:
additionalHeaders()is inlib/internal/http2/core.jsat line 3186 (v26.3.0). It checks destroyed or closed,headersSent, 101, and the 1xx range. Then it sends the block.writeContinue()andwriteEarlyHints()on a HEAD request through a'checkContinue'listener: the output is identical to node with this change. Before, both threw.:method: HEAD:additionalHeaders()now sends the block, like node. The client events for a 1xx block on a pushed stream are different from node ('headers'then'push'in bun,'push'then'response'in node). That difference is the same for a pushed GET stream on bun 1.4.3, so this PR does not cause it.src/atmainand pass with the change. The compat test runs in the test process: an uncaught exception fails the test at once, with the stack above.additionalHeaders()or the same test file.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file