node:http/http2 hardening: enforce h2 request pseudo-headers, never report a failed handler as success, and bound/validate the parser paths - #33191
Open
cirospaciari wants to merge 209 commits into
Conversation
… upstream suites node:http server (native and JS): - request/headers timeouts and the 408 sweep, connection-at-accept, keep-alive idle close, maxRequestsPerSocket - llhttp-strict parsing in node mode (Bun.serve keeps its lenient parser), per-server parser options, httpValidation/insecureHTTPParser, relaxed header validation on the client side - async pipelining with a queued response writer; a client FIN no longer tears the connection down while pipelined responses are still queued - socketOnError/clientError parity, upgrade-with-body and CONNECT tunnels (reading the request no longer flips the raw socket into flowing mode), trailers, server uniqueHeaders option - net.Socket-backed connection sockets with socket.parser, 'close' on native connection close, highWaterMark plumbing, HPE_PAUSED_H2_UPGRADE, handle.close(callback) contract, more expose-internals shim entries - agent and client fixes, https TLS option forwarding (pfx, min/maxVersion, ALPN props), SNI servername accessor, domain binding, abort/half-open socket lifecycle node:http2: - nghttp2-style session errors and teardown, flood/memory limits - settings parity (empty initial frame, customSettings, enableConnectProtocol) - write backpressure with deferred completion, client request queueing and AbortSignal, respondWithFile fd ownership/bypass of the user-facing writable - stream window replenishment tied to JS-side reads, session idle timer under kTimeout with write-progress suppression, deferred stream destroy when the request body is still buffered - AsyncLocalStorage context, allowHTTP1 fallback headers, trailers ordering, strictSingleValueFields - session.destroy(code) only surfaces on streams with an 'error' listener Tests: - vendor 102 missing upstream tests (64 passing now, 38 recorded as not yet passing in expectations.txt), resync 84 drifted vendored files to v26.3.0 - drop the now-passing expectations entries and update Bun-authored assertions that depended on the old divergent error codes/messages - mark the Windows-only http/http2 follow-ups (named-pipe listen, test-http2-pipe and test-http2-timeout-large-write-file) All new server parser/lifecycle behavior is gated on the node:http compat flags so Bun.serve keeps its existing fast paths unchanged. net: surface fatal send(2) errors as Node-shaped write errors - A send() that fails after the peer reset the connection now reaches JS: the errno travels usockets -> write_check_error -> $write (negative errno; -1 stays the legacy closed/shutdown sentinel) -> node:net fails the pending write callback like Node's onWriteComplete (lib/internal/stream_base_commons.js#L81-L92). A fatal flush of natively buffered data is surfaced from on_writable through the socket's error handler with the errno-derived code. - ENOBUFS stays transient and macOS EPROTOTYPE is reported as ECONNRESET, matching libuv's uv__try_write. - Reverts the synthesized "read ECONNRESET" on a duplicate EOF dispatch in the net `end` handler (Node treats a repeated EOF as a no-op). The http2 flood tests that branch was masking (test-http2-max-invalid-frames, test-http2-reset-flood) now pass through the real write-error path, and test-net-write-slow no longer times out under parallel load. - Adds a net regression test for write-after-reset, promotes double-connect.test.ts out of test.failing, and removes the stale darwin FLAKY expectation for test-http2-max-invalid-frames.
… upstream suites node:http server (native and JS): - request/headers timeouts and the 408 sweep, connection-at-accept, keep-alive idle close, maxRequestsPerSocket - llhttp-strict parsing in node mode (Bun.serve keeps its lenient parser), per-server parser options, httpValidation/insecureHTTPParser, relaxed header validation on the client side - async pipelining with a queued response writer; a client FIN no longer tears the connection down while pipelined responses are still queued - socketOnError/clientError parity, upgrade-with-body and CONNECT tunnels (reading the request no longer flips the raw socket into flowing mode), trailers, server uniqueHeaders option - net.Socket-backed connection sockets with socket.parser, 'close' on native connection close, highWaterMark plumbing, HPE_PAUSED_H2_UPGRADE, handle.close(callback) contract, more expose-internals shim entries - agent and client fixes, https TLS option forwarding (pfx, min/maxVersion, ALPN props), SNI servername accessor, domain binding, abort/half-open socket lifecycle node:http2: - nghttp2-style session errors and teardown, flood/memory limits - settings parity (empty initial frame, customSettings, enableConnectProtocol) - write backpressure with deferred completion, client request queueing and AbortSignal, respondWithFile fd ownership/bypass of the user-facing writable - stream window replenishment tied to JS-side reads, session idle timer under kTimeout with write-progress suppression, deferred stream destroy when the request body is still buffered - AsyncLocalStorage context, allowHTTP1 fallback headers, trailers ordering, strictSingleValueFields - session.destroy(code) only surfaces on streams with an 'error' listener Tests: - vendor 102 missing upstream tests (64 passing now, 38 recorded as not yet passing in expectations.txt), resync 84 drifted vendored files to v26.3.0 - drop the now-passing expectations entries and update Bun-authored assertions that depended on the old divergent error codes/messages - mark the Windows-only http/http2 follow-ups (named-pipe listen, test-http2-pipe and test-http2-timeout-large-write-file) All new server parser/lifecycle behavior is gated on the node:http compat flags so Bun.serve keeps its existing fast paths unchanged. net: surface fatal send(2) errors as Node-shaped write errors - A send() that fails after the peer reset the connection now reaches JS: the errno travels usockets -> write_check_error -> $write (negative errno; -1 stays the legacy closed/shutdown sentinel) -> node:net fails the pending write callback like Node's onWriteComplete (lib/internal/stream_base_commons.js#L81-L92). A fatal flush of natively buffered data is surfaced from on_writable through the socket's error handler with the errno-derived code. - ENOBUFS stays transient and macOS EPROTOTYPE is reported as ECONNRESET, matching libuv's uv__try_write. - Reverts the synthesized "read ECONNRESET" on a duplicate EOF dispatch in the net `end` handler (Node treats a repeated EOF as a no-op). The http2 flood tests that branch was masking (test-http2-max-invalid-frames, test-http2-reset-flood) now pass through the real write-error path, and test-net-write-slow no longer times out under parallel load. - Adds a net regression test for write-after-reset, promotes double-connect.test.ts out of test.failing, and removes the stale darwin FLAKY expectation for test-http2-max-invalid-frames.
…ld scan uWS::HttpParser::parseTrailerFields ran tryConsumeFieldValue (8-byte loads) over the captured trailer section with none of the post-padding the rest of the parser guarantees, so the scan's last load could read up to 3 bytes past the section's allocation (ASAN heap-buffer-overflow in tryConsumeFieldValue). The function now pads the section it is handed in place, so every caller gets the same contract, and its doc no longer claims the trailing CRLFs make fencing unnecessary or that the chunk iterator validated the section's bytes. New test: request trailers at the scan alignments that read past the section (fails with the ASAN report above without the padding). tls-syscall-fault "FIN before close_notify drained" now consumes the client's readable side. net.connect no longer force-resumes the stream (it matches Node's afterConnect read(0)), so a socket with unread buffered data never emits 'end' and is never destroyed - same as Node - and awaiting 'close' on a never-read client therefore hangs by design; this was the deterministic debian-13-x64-asan CI timeout. New test: a freshly connected socket is not flowing, and data received before a 'data' listener is attached is delivered rather than dropped.
…ld scan uWS::HttpParser::parseTrailerFields ran tryConsumeFieldValue (8-byte loads) over the captured trailer section with none of the post-padding the rest of the parser guarantees, so the scan's last load could read up to 3 bytes past the section's allocation (ASAN heap-buffer-overflow in tryConsumeFieldValue). The function now pads the section it is handed in place, so every caller gets the same contract, and its doc no longer claims the trailing CRLFs make fencing unnecessary or that the chunk iterator validated the section's bytes. New test: request trailers at the scan alignments that read past the section (fails with the ASAN report above without the padding). tls-syscall-fault "FIN before close_notify drained" now consumes the client's readable side. net.connect no longer force-resumes the stream (it matches Node's afterConnect read(0)), so a socket with unread buffered data never emits 'end' and is never destroyed - same as Node - and awaiting 'close' on a never-read client therefore hangs by design; this was the deterministic debian-13-x64-asan CI timeout. New test: a freshly connected socket is not flowing, and data received before a 'data' listener is attached is delivered rather than dropped.
Raw-socket coverage of headersTimeout, requestTimeout, server.setTimeout, and keepAliveTimeout, including that requestTimeout stops once the incoming request completes and never fires while a slow handler is still streaming its response. Five of the six cases fail on a Bun without the timeout enforcement in this branch. Contributed in #32942, which was closed in favor of this PR.
Raw-socket coverage of headersTimeout, requestTimeout, server.setTimeout, and keepAliveTimeout, including that requestTimeout stops once the incoming request completes and never fires while a slow handler is still streaming its response. Five of the six cases fail on a Bun without the timeout enforcement in this branch. Contributed in #32942, which was closed in favor of this PR.
The "under 2GiB clones without crashing" subtests spawn a child whose peak memory is about three copies of the buffer (~5GB), which the Linux x64 CI runners no longer have headroom for: the kernel OOM-kills the child, the test sees empty output, and four shards go red without any structuredClone defect. Use the smallest buffer that still drives the 1.5x serialization buffer growth past the 2GiB cap, and treat a SIGKILLed child with no output as the environment reclaiming memory rather than a failure. A child that dies on any other signal, exits nonzero, or prints the wrong result still fails the test.
The "under 2GiB clones without crashing" subtests spawn a child whose peak memory is about three copies of the buffer (~5GB), which the Linux x64 CI runners no longer have headroom for: the kernel OOM-kills the child, the test sees empty output, and four shards go red without any structuredClone defect. Use the smallest buffer that still drives the 1.5x serialization buffer growth past the 2GiB cap, and treat a SIGKILLed child with no output as the environment reclaiming memory rather than a failure. A child that dies on any other signal, exits nonzero, or prints the wrong result still fails the test.
ClientHttp2Stream captures the async context active when request() is called, and enterStreamAsyncContext swaps it in around native dispatches. It treated a captured empty context (undefined) as "nothing captured" and skipped the swap, so a stream requested outside any AsyncLocalStorage scope, on a session whose wrapped socket callbacks carry the connect-time store, ran its 'response'/'data'/'end' handlers in that store. Node's Http2Stream is an async resource and restores the request-time (empty) context. The never-captured default is now the kNoAsyncContextSwap sentinel, so a captured undefined still swaps. Test: a session connected inside als.run() with a request issued from an empty context must observe an empty store in every stream handler; it fails without this change and matches Node with it.
ClientHttp2Stream captures the async context active when request() is called, and enterStreamAsyncContext swaps it in around native dispatches. It treated a captured empty context (undefined) as "nothing captured" and skipped the swap, so a stream requested outside any AsyncLocalStorage scope, on a session whose wrapped socket callbacks carry the connect-time store, ran its 'response'/'data'/'end' handlers in that store. Node's Http2Stream is an async resource and restores the request-time (empty) context. The never-captured default is now the kNoAsyncContextSwap sentinel, so a captured undefined still swaps. Test: a session connected inside als.run() with a request issued from an empty context must observe an empty store in every stream handler; it fails without this change and matches Node with it.
Same single-gc()-plus-single-setImmediate FinalizationRegistry timing as its already-quarantined tls sibling (the comment block above the entries documents it). It also now fails on about half of all PR builds on this platform since June 28, independently of this branch (#33044).
Same single-gc()-plus-single-setImmediate FinalizationRegistry timing as its already-quarantined tls sibling (the comment block above the entries documents it). It also now fails on about half of all PR builds on this platform since June 28, independently of this branch (#33044).
…hint
On macOS, kqueue reports EV_EOF on the same readable event as a
connection's final data, and the dispatch trusted that hint after a read
loop that stops early: when a read comes back short of nearly the full
buffer, when the per-event repeat budget is spent, or when the data
callback pauses the socket (stream backpressure). It then ended and
closed the socket with bytes still queued in the kernel, silently
truncating the stream. A tls.connect() client whose peer did end(big) +
destroySoon() observed 'end' short of the payload; the vendored
test-tls-client-destroy-soon failed exactly that way on the
darwin-aarch64 CI runner ("'end' three TLS records short"), which an
earlier draft of this branch had quarantined in test/expectations.txt,
and the recv-short TLS decode tests in tls-syscall-fault.test.ts failed
the same way on macOS.
Linux never had the problem: epoll does not flag a half-close, so the
EOF is only discovered by recv() returning 0, after everything before
the FIN has been read. This gives the flagged path the same property,
in three steps scoped to a hung-up readable dispatch:
- Keep reading after the hangup/error flag until recv() returns 0 or
EAGAIN. The comment above the read loop always described this intent
but keyed it on the error flag, which kqueue does not set for a peer
FIN. Bounded by the receive buffer.
- If that drain hits a hard error after data was already read in the
same flagged dispatch, the peer's FIN preceded an RST (commonly
provoked by our own teardown writes): report end-of-stream, like a
reader that stopped at the FIN. An error on the first read (a pure
RST: the kernel has discarded the receive queue) still reports the
error.
- If the data callback paused the socket mid-burst, defer the EOF hint
instead of closing: resuming re-arms the poll and recv() == 0 reports
the real EOF once the rest has been read. Sockets that already sent
their FIN are exempt so a peer's FIN still closes them promptly.
New regression test: a paused TLS client resumed after the peer's
end()+destroySoon() receives every byte (it received 3 of 12 records
before). The recv-short TLS decode tests now pass on macOS, and the
test-tls-client-destroy-soon quarantine entry is removed.
…hint
On macOS, kqueue reports EV_EOF on the same readable event as a
connection's final data, and the dispatch trusted that hint after a read
loop that stops early: when a read comes back short of nearly the full
buffer, when the per-event repeat budget is spent, or when the data
callback pauses the socket (stream backpressure). It then ended and
closed the socket with bytes still queued in the kernel, silently
truncating the stream. A tls.connect() client whose peer did end(big) +
destroySoon() observed 'end' short of the payload; the vendored
test-tls-client-destroy-soon failed exactly that way on the
darwin-aarch64 CI runner ("'end' three TLS records short"), which an
earlier draft of this branch had quarantined in test/expectations.txt,
and the recv-short TLS decode tests in tls-syscall-fault.test.ts failed
the same way on macOS.
Linux never had the problem: epoll does not flag a half-close, so the
EOF is only discovered by recv() returning 0, after everything before
the FIN has been read. This gives the flagged path the same property,
in three steps scoped to a hung-up readable dispatch:
- Keep reading after the hangup/error flag until recv() returns 0 or
EAGAIN. The comment above the read loop always described this intent
but keyed it on the error flag, which kqueue does not set for a peer
FIN. Bounded by the receive buffer.
- If that drain hits a hard error after data was already read in the
same flagged dispatch, the peer's FIN preceded an RST (commonly
provoked by our own teardown writes): report end-of-stream, like a
reader that stopped at the FIN. An error on the first read (a pure
RST: the kernel has discarded the receive queue) still reports the
error.
- If the data callback paused the socket mid-burst, defer the EOF hint
instead of closing: resuming re-arms the poll and recv() == 0 reports
the real EOF once the rest has been read. Sockets that already sent
their FIN are exempt so a peer's FIN still closes them promptly.
New regression test: a paused TLS client resumed after the peer's
end()+destroySoon() receives every byte (it received 3 of 12 records
before). The recv-short TLS decode tests now pass on macOS, and the
test-tls-client-destroy-soon quarantine entry is removed.
…ushing socket.end() on a node:http server connection shut the transport down as soon as the socket's own stream buffer was empty, ignoring the response bytes uWS still holds in its send buffer after a large res.write()/end() under backpressure. The FIN overtook those bytes, so a Connection: close response was silently truncated mid-body. macOS surfaces it readily (its small loopback send buffer leaves several MB in userspace); Node delivers every byte. When the in-flight response still has buffered data, hand the close over to uWS instead of shutting down: mark the response HTTP_CONNECTION_CLOSE so HttpContext::onWritable shuts the socket down right after the last byte flushes, the same sequencing Node gets from destroySoon(). Sockets with no response backpressure keep the immediate shutdown. The new tests cover the three paths (client-requested close, server-set Connection: close, one-shot res.end(body)); each receives ~1.4 MB of an 8 MiB body without the fix.
…ushing socket.end() on a node:http server connection shut the transport down as soon as the socket's own stream buffer was empty, ignoring the response bytes uWS still holds in its send buffer after a large res.write()/end() under backpressure. The FIN overtook those bytes, so a Connection: close response was silently truncated mid-body. macOS surfaces it readily (its small loopback send buffer leaves several MB in userspace); Node delivers every byte. When the in-flight response still has buffered data, hand the close over to uWS instead of shutting down: mark the response HTTP_CONNECTION_CLOSE so HttpContext::onWritable shuts the socket down right after the last byte flushes, the same sequencing Node gets from destroySoon(). Sockets with no response backpressure keep the immediate shutdown. The new tests cover the three paths (client-requested close, server-set Connection: close, one-shot res.end(body)); each receives ~1.4 MB of an 8 MiB body without the fix.
Hardening pass over the node:http/http2 compat layer, each item verified
against Node v26.3.0 / nghttp2 / llhttp:
- The HTTP/2 server rejects request header blocks missing :method, :scheme
or :path (or carrying an empty pseudo-header value, a pseudo-header in
trailers, an empty or repeated Host, or CONNECT with :scheme/:path) with
a stream PROTOCOL_ERROR, matching nghttp2. Such blocks used to reach JS
as requests whose method and url are undefined.
- A node:http 'request' listener that throws before writing no longer
produces a well-formed empty 200 OK (the status check in the exception
arm was inverted); it answers 500, and a failure after status or body
bytes closes the connection without valid framing so it can never read
as a complete response. The async rejection path shares the same helper.
- ConnectionsList.idle()/expired() no longer dereference a closed parser's
freed impl (segfault).
- The chunked-trailer capture is always bounded: a maxHeaderSize of 0
selects the process default, like node, never "unlimited".
- server.upgrade(res) for node:http validates before committing the
one-shot 101 preamble, so a refused upgrade cannot corrupt the app's
fallback response.
- A Transfer-Encoding value listing any coding after a chunked token
("chunked, chunked") is rejected like llhttp.
- rejectUnauthorized is normalized the way node does (only an explicit
false disables verification) through one helper at every ingestion site
and handshake gate, and non-boolean spellings no longer throw in the
native converter.
- The h2 engine pins an ArrayBuffer write payload while the send loop can
re-enter JS, and frees closed streams only at quiescent points so a
re-entrant parser.read() cannot free a stream a native frame still uses.
Every behavioral fix ships with a regression test in the existing suites;
the tests for the reproducible bugs fail without their fix.
Collaborator
|
Updated 11:49 AM PT - Aug 4th, 2026
❌ @robobun, your commit 1057479 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33191That installs a local version of the PR into your bun-33191 --bun |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
The build-rust CI jobs compile with -D unused-variables and -D unreachable-pub, which the local debug build does not: scope the h2 pseudo-header bit constants to their parent module and drop a binding that became unused when upgrade() started delegating to can_upgrade().
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The h2 connection unit tests now send `:scheme` and `:authority`, matching the request pseudo-header validation the server enforces since the previous commit. Re-attach the doc comments that new helpers were inserted under (`rewrite_read`, `failWrite`). Read `_rejectUnauthorized` into a local in the two TLS handshake gates: oxlint's double-property-read rule fails the lint job and the Windows test jobs without it.
Member
Author
|
On the #28656 suggestion above: the attribution belongs to the base PR, not this one. bun 1.3.14 reproduces the issue (no ALPN negotiated, empty reply), but the base branch (#32488) already answers the same repro with |
Member
Author
|
CI note on build #67595: the
I'll re-run CI after the bake breakage is handled on Update (build #67614, current head): Update 2 (build #67626): one more environmental item: Root cause + fix for the bake failure: the harness installs the moving |
…sponses The inbound engine only tracks streams it has seen inbound, so on a client a response block always decodes as a "new" stream and was classified as a request block. That made the Host checks added for RFC 9113 §8.3.1 (empty or repeated Host is malformed) reject responses that nghttp2 delivers: nghttp2 only applies its `host` rule in http_request_on_header. Classify a block as a request only when a server receives HEADERS for a new stream (PUSH_PROMISE already tags itself), gate the Host rule on that, and add a client regression test that drives a raw response carrying an empty and a repeated `host` header.
…ke nghttp2 A client-received PUSH_PROMISE block is a request block: nghttp2 finalizes it through nghttp2_http_on_request_headers, so the RFC 9113 §8.3.1 mandatory pseudo-header check now runs for it instead of being gated to servers. A promised request that fails it is answered the way node answers it — a GOAWAY(PROTOCOL_ERROR) connection error, never a 'stream' event — verified against node with a raw-socket server. The previous build delivered such a push to JS with its :scheme and :authority missing.
cirospaciari
force-pushed
the
claude/node-http-http2-compat
branch
from
July 14, 2026 21:09
5627830 to
eb693c9
Compare
Both files are byte-identical to nodejs/node v26.3.0 except for the
placeholder trailer values in test-http-multiple-headers ('VVV' for the
upstream triple that trips the diff grep; the test proves the same trailer
join contract) and the 'Note(jasnell)' reword of the upstream author note
in test-http2-settings-flood. No functional change; both pass run-for-run
against the debug build.
… tests" This reverts commit 947cef1. The vendored node suites have to stay byte-identical to v26.3.0 — the compat numbers this PR reports only mean anything if the suites run unmodified. The scrub rewrote test data, not just a marker: in test-http-multiple-headers.js it changed the trailer value 'XXX' to 'VVV' in both the fixture and the three assertions that check it. The TODO(jasnell) in test-http2-settings-flood.js is upstream's own comment, not ours to edit. Both files match v26.3.0 byte-for-byte again after this revert. No-Verification-Needed: test-only revert restoring vendored upstream test files
…tlsClientError to rejected connections test-net-stream.js timed out on darwin-aarch64 (builds 72973, 73013): the server never observed its peer vanish. On kqueue a peer RST delivers READABLE|WRITABLE with eof=1 and error=0, so loop.c's writable dispatch runs first; on_writable's fatal-flush block detects EPIPE, dispatches the error handler, and closes - short-circuiting the read dispatch at us_socket_is_closed. The dispatch reached ServerHandlers.error, which set _hadError and delegated to SocketHandlers.error, whose own _hadError guard returned immediately: the server socket never emitted 'error', its pending write callback was never failed, and 'close' never fired. 30dd839's 'drain and read paths deliver' premise holds on Linux (EPOLLERR makes loop.c skip the writable dispatch) but not on kqueue. The fallthrough now shapes the error like Node's onWriteComplete: fail the pending write callback, then destroy() with the error so the stream machinery owns the single 'error' emission. The TLS branch keeps its existing routing unchanged. Un-swallowing server-socket error dispatches exposed a pre-existing emit: ServerHandlers.handshake fired tlsClientError for client-cert verification failures even with rejectUnauthorized:false, where Node's onServerSocketSecure proceeds with authorized=false and never emits. test-tls-sni-option's mustNotCall(tlsClientError) was only passing because its AssertionError was swallowed by the same ServerHandlers.error no-op. The emit is now scoped to the rejectUnauthorized branch (the connection is refused, so the event carries the reason), matching both the sni-option suite and the existing rejectUnauthorized tests in node-tls-cert. New fault-injection regression test reproduces the kqueue sequence on Linux (sustained fatal send on the accepted socket's fd): times out without the fix, passes with it. Verified test-tls-sni-option 3/3, test-tls-server-verify, the tls tlsClientError suite, test-net-stream, test-net-error-twice, test-net-write-slow, and both syscall-fault suites.
…nto ciro/pr33191-fix # Conflicts: # packages/bun-usockets/src/socket.c # packages/bun-uws/src/App.h # packages/bun-uws/src/ChunkedEncoding.h # packages/bun-uws/src/HttpContext.h # packages/bun-uws/src/HttpContextData.h # packages/bun-uws/src/HttpParser.h # packages/bun-uws/src/HttpResponse.h # packages/bun-uws/src/HttpResponseData.h # src/js/internal/tls.ts # src/js/node/_http_server.ts # src/js/node/http2.ts # src/js/node/net.ts # src/js/node/tls.ts # src/jsc/ErrorCode.rs # src/jsc/bindings/JSEnvironmentVariableMap.cpp # src/jsc/bindings/NodeHTTP.cpp # src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp # src/runtime/api/bun/h2/connection.rs # src/runtime/api/bun/h2_frame_parser.rs # src/runtime/server/server_body.rs # src/runtime/socket/socket_body.rs # test/expectations.txt # test/js/node/http/node-http.test.ts # test/js/node/test/parallel/test-http-client-abort-keep-alive-queued-unix-socket.js # test/js/node/test/parallel/test-http-client-abort-unix-socket.js # test/js/node/test/parallel/test-http-client-pipe-end.js # test/js/node/test/parallel/test-http-client-response-domain.js # test/js/node/test/parallel/test-http-client-with-create-connection.js # test/js/node/test/parallel/test-http-unix-socket.js # test/js/node/test/parallel/test-https-unix-socket-self-signed.js
cirospaciari
pushed a commit
that referenced
this pull request
Jul 20, 2026
…pty/missing :path, :method, :scheme; CONNECT shape) (#34736) ## What A raw-frame h2 client that sends a request with `:path` set to the empty string reaches the `'stream'` handler with `headers[':path'] === ''` (and `req.url === ''` in the compat layer). RFC 9113 8.3.1 says `:path` "MUST NOT be empty" for http/https and that every non-CONNECT request must carry exactly one non-empty `:method`, `:scheme` and `:path`; node (via nghttp2's `nghttp2_http_on_request_headers`) answers with `RST_STREAM(PROTOCOL_ERROR)` and never dispatches. Repro (node RSTs, Bun on main dispatches): ```js import http2 from 'node:http2'; import net from 'node:net'; const hs = s => { const b = Buffer.from(s); return Buffer.concat([Buffer.from([b.length]), b]); }; const hp = ps => Buffer.concat(ps.flatMap(([n,v]) => [Buffer.from([0x10]), hs(n), hs(v)])); const fr = (t,f,sid,pl) => { const h = Buffer.alloc(9); h.writeUIntBE(pl.length,0,3); h[3]=t; h[4]=f; h.writeUInt32BE(sid,5); return Buffer.concat([h,pl]); }; const srv = http2.createServer(); srv.on('stream', (st, h) => { console.log('DISPATCHED', JSON.stringify(h)); st.respond({':status':200}); st.end(); }); srv.listen(0, '127.0.0.1', () => { const s = net.connect(srv.address().port, '127.0.0.1'); s.on('connect', () => s.write(Buffer.concat([ Buffer.from('PRI * HTTP/2.0\\r\\n\\r\\nSM\\r\\n\\r\\n'), fr(4,0,0,Buffer.alloc(0)), fr(1,0x5,1,hp([[':method','GET'],[':path',''],[':scheme','http'],[':authority','h']])), ]))); }); // bun main: DISPATCHED {":method":"GET",":path":"",":scheme":"http",":authority":"h"} // node v26: (no dispatch, RST_STREAM PROTOCOL_ERROR on stream 1) ``` The same validator gap meant requests with `:method`/`:scheme`/`:path` missing entirely, a plain `CONNECT` carrying `:scheme`/`:path`, a `CONNECT` without `:authority`, or `:protocol` on a non-CONNECT all reached the handler too. All are rejected by node as a stream `PROTOCOL_ERROR`. ## Cause `finish_header_block` in the h2 engine (`src/runtime/api/bun/h2/connection.rs`) tracks which pseudo-headers were seen (for the duplicate check) but never checks that the required ones are present, and never checks for empty values. The decode loop validated per-field shape (duplicate, unknown, late, connection-specific, CR/LF/NUL) but not the per-block 8.3.1 requirements. ## Fix nghttp2-equivalent, one block in `finish_header_block`: - An empty pseudo-header value is malformed inline (`check_pseudo_header` semantics), so `":path": ""` never counts as present. - After the decode loop, a request block (a server-received initial `HEADERS` or a client-received `PUSH_PROMISE`) is held to the 8.3.1 requirements: `:method`, `:scheme`, `:path` and `:authority`-or-`Host` for ordinary requests; `:authority` and no `:scheme`/`:path` for plain `CONNECT`; `:method CONNECT` + `:authority` for extended CONNECT (`:protocol`, RFC 8441). A `header_is_request` flag distinguishes a request block from a trailer section (which 8.1 already forbids from carrying pseudo-headers) and from a client-received response block (which the 8.3.1 rules do not apply to). The rejection takes the existing malformed-block path (`RST_STREAM(PROTOCOL_ERROR)`, counted against `maxSessionInvalidFrames`, never surfaced to `'stream'`), so the JS-visible behavior matches node exactly. ## Verification 16 new rejection cases plus 4 positive cases (valid block, `host` in place of `:authority`, plain `CONNECT`, extended `CONNECT`) in `test/js/node/http2/h2-conformance.test.ts`. All 16 rejection cases fail on the unfixed build (the request is dispatched) and pass with the fix; all 4 positive cases pass on both. The full `h2-conformance` suite (58 tests), `node-http2.test.js` (305 tests) and node's upstream `test-http2-connect-method*`/`test-http2-misused-pseudoheaders` pass on the fixed build. ## Related #33191 carries a broader version of this validation (including nghttp2's `check_path()` `:path`-starts-with-`/` rule, `Host` empty/repeated handling, and `PUSH_PROMISE` connection-error semantics) as item 1 of a larger hardening round. This PR is the minimal, standalone carve-out of the 8.3.1 presence/emptiness rules against current main, shaped so #33191 rebases cleanly over it. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 16 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/h2-conformance.test.ts" info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (26eb1c8) test/js/node/http2/h2-conformance.test.ts: (pass) connection preface & SETTINGS handshake (checklist §1) > server sends a SETTINGS frame first (§1.4) [429.38ms] (pass) connection preface & SETTINGS handshake (checklist §1) > server ACKs the client's SETTINGS frame (§3.5) [145.80ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS frame with a non-zero stream id is a PROTOCOL_ERROR (§3.5) [117.38ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS frame whose length is not a multiple of 6 is a FRAME_SIZE_ERROR (§3.5) [87.99ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS ACK that carries a payload is a FRAME_SIZE_ERROR (§ ... (truncated) release without fix: 29 FAILED bun test v1.4.0-canary.1 (1498d7b) test/js/node/http2/h2-conformance.test.ts: (pass) connection preface & SETTINGS handshake (checklist §1) > server sends a SETTINGS frame first (§1.4) [8.02ms] (pass) connection preface & SETTINGS handshake (checklist §1) > server ACKs the client's SETTINGS frame (§3.5) [2.39ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS frame with a non-zero stream id is a PROTOCOL_ERROR (§3.5) [1.39ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS frame whose length is not a multiple of 6 is a FRAME_SIZE_ERROR (§3.5) [0.92ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS ACK that carries a payload is a FRAME_SIZE_ERROR (§3.5) [0.86ms] (pass) PING (checklist §3.7) > server replies to PING with a PING ACK echoing the payload [2.33ms] (pass) PING (checklist §3.7) > a PING with length != 8 is a FRAME_SIZE_ERROR [0.82ms] (pass) PING (checklist §3.7) > a PING on a non-zero stream id is a PROTOCOL_ERROR [0.72ms] (pass) WINDOW_UPDATE (checklist §6) > a connection-level WINDOW_UPDATE with a 0 increment is a PROTOCOL_ERROR [0.71ms] (pass) WINDOW_UPDATE ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/h2-conformance.test.ts" info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (26eb1c8) test/js/node/http2/h2-conformance.test.ts: (pass) connection preface & SETTINGS handshake (checklist §1) > server sends a SETTINGS frame first (§1.4) [451.77ms] (pass) connection preface & SETTINGS handshake (checklist §1) > server ACKs the client's SETTINGS frame (§3.5) [148.56ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS frame with a non-zero stream id is a PROTOCOL_ERROR (§3.5) [133.99ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS frame whose length is not a multiple of 6 is a FRAME_SIZE_ERROR (§3.5) [90.24ms] (pass) connection preface & SETTINGS handshake (checklist §1) > a SETTINGS ACK that carries a payload is a FRAME_SIZE_ERROR (§ ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) [configured] bun-profile → bun (stripped) in 750ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/5] gen generated_host_exports.rs generated_host_exports.rs: 91 exports (host=3, lazy=10, generic=78, rust=0); 244 extern-C blocks audited [1/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: component rust-std is up to date nightly-2026-05-06-x86_64-unknown-linux-gnu unchanged - rustc 1.97.0-nightly (e95e73209 2026-05-05) info: checking for self-update (current version: 1.29.0) �[1m�[92m Blocking�[0m waiting for file lock on build directory �[1m�[92m Compiling�[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime) �[1m�[ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/api/bun/h2/connection.rs | 89 +++++++++-- test/js/node/http2/h2-conformance.test.ts | 242 ++++++++++++++++++++++++++++++ 2 files changed, 318 insertions(+), 13 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/api/bun/h2/connection.rs 6 13 0 test/js/node/http2/h2-conformance.test.ts 5 1 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…2-compat-hardening # Conflicts: # packages/bun-usockets/src/crypto/openssl.c # packages/bun-usockets/src/eventing/libuv.c # packages/bun-uws/src/HttpContext.h # packages/bun-uws/src/HttpParser.h # packages/bun-uws/src/HttpResponseData.h # src/dotenv/env_loader.rs # src/js/internal/tls.ts # src/js/node/_http_client.ts # src/js/node/_http_incoming.ts # src/js/node/_http_server.ts # src/js/node/http2.ts # src/js/node/net.ts # src/js/node/tls.ts # src/jsc/bindings/ErrorCode.ts # src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp # src/jsc/bindings/node/JSNodeHTTPServerSocket.h # src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp # src/jsc/web_worker.rs # src/runtime/api/bun/h2/connection.rs # src/runtime/api/bun/h2_frame_parser.rs # src/runtime/server/NodeHTTPResponse.rs # src/runtime/server/ServerWebSocket.rs # src/runtime/server/WebSocketServerContext.rs # src/runtime/server/mod.rs # src/runtime/server/server_body.rs # src/uws_sys/App.rs # src/uws_sys/Response.rs # src/uws_sys/h3.rs # src/uws_sys/us_socket_t.rs # test/expectations.txt # test/js/bun/http/serve.test.ts # test/js/bun/net/socket-syscall-fault.test.ts # test/js/bun/websocket/websocket-server.test.ts # test/js/node/http/node-http-backpressure.test.ts # test/js/node/http/node-http-parser.test.ts # test/js/node/http/node-http-transfer-encoding.test.ts # test/js/node/http/node-http-uaf.test.ts # test/js/node/http2/h2-conformance.test.ts # test/js/node/http2/node-http2.test.js # test/js/node/net/node-net.test.ts # test/js/web/fetch/fetch-leak.test.ts
Contributor
WalkthroughChangesThe PR strengthens HTTP/1 and HTTP/2 validation, normalizes TLS authorization options, protects HTTP/2 stream lifetimes during reentrant execution, centralizes HTTP response failure handling, validates WebSocket upgrades, and adds shutdown cleanup with regression tests. HTTP/1 validation and connection safety
TLS authorization normalization
HTTP/2 validation and stream safety
HTTP response and upgrade lifecycle
FileSink shutdown cleanup
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
- patches/mimalloc/strnlen-oob-read.patch no longer applies: the pinned mimalloc commit (d078ad06) already has the bounds-first strnlen fix. Main dropped this patch in #34335; the merge into this branch re-introduced it. - WebSocketServerContext.rs Handler struct had two 'server' fields (E0124) after the merge; keep main's pub(crate) one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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/js/node/tls.ts`:
- Line 13: Use normalizeRejectUnauthorized as the sole normalization path in the
surrounding TLS options handling: replace the earlier inline conversion with
this shared helper, ensuring null, 0, and empty-string values are handled
consistently, then remove the duplicate normalization block later in the same
flow.
In `@src/runtime/api/bun/h2_frame_parser.rs`:
- Around line 7979-7990: Update the
StringOrBuffer::from_js_with_encoding_maybe_async call in the send_data
conversion path to use explicit argument labels/comments for the fourth is_async
flag and fifth allow_string_object flag, preserving their current true values.
Leave ThreadSafe::adopt unchanged because it already takes ownership of the
existing protection and releases it exactly once.
In `@src/runtime/api/bun/h2/connection.rs`:
- Around line 1180-1185: Restrict the `b"host"` validation arm to `is_request`
only by removing the `self.is_server` disjunct. Preserve the existing
empty-value and duplicate-Host checks for request blocks, while allowing trailer
sections to bypass this validation.
In `@src/runtime/server/NodeHTTPResponse.rs`:
- Around line 511-520: Strengthen NodeHTTPResponse::can_upgrade() to require
raw_response and reject both is_http_status_called() and is_http_write_called()
before permitting an upgrade. In src/runtime/server/server_body.rs lines
1904-1912, rely on this shared predicate before writing the 101 response or
custom headers, with no separate predicate change required there. In
test/js/node/http/node-http-with-ws.test.ts lines 63-98, add a header getter
that calls res.writeHead(200) and assert exactly one valid 200 response with no
upgrade headers.
In `@src/runtime/webcore/FileSink.rs`:
- Around line 933-960: Remove the duplicated shutdown cleanup block in the
relevant finalization method, keeping a single execution of the
vm.is_shutting_down() cleanup that calls clear_keep_alive_ref and conditionally
balances run_pending_later. Preserve the existing flag checks, pointer lifetime
guarantees, and deref behavior; do not retain a second no-op pass.
In `@test/js/bun/http/request-smuggling.test.ts`:
- Around line 99-116: Update the maliciousRequest fixture to include a
Connection: close header, ensuring the unfixed parser closes the socket and the
existing response assertion fails with a non-400 response instead of waiting for
the client close event.
In `@test/js/node/http/node-http-parser.test.ts`:
- Around line 280-283: Update the test around the closed parser fixture to
assert that p remains present in the active-list result after p.close(), before
invoking list.expired(1, 1). Keep the existing idle, expired, and all membership
assertions, ensuring the test exercises expired() scanning activeConnections().
In `@test/js/node/http/node-http-with-ws.test.ts`:
- Around line 63-98: Add a regression test alongside the existing plain-request
upgrade test where the upgrade options’ headers getter invokes
res.writeHead(200) before returning the supplied headers. Assert that
server.upgrade returns false, the response contains exactly one HTTP/1.1 200
response, and the upgrade header is absent, while preserving the existing
fallback response validation and cleanup.
In `@test/js/node/http2/node-http2.test.js`:
- Around line 2937-2974: Extract the duplicated literal helper and the
byte-identical exchange frame-reader logic into module-level shared helpers near
the existing http2utils usage. Replace all five local literal definitions and
both exchange implementations with calls to those shared helpers, preserving
their current framing, completion, error, and socket-cleanup behavior.
- Around line 3520-3580: Strengthen the reentrant-close test by tracking
observable execution of the queued write callback and PING handling. Add flags
in the test around the req.write callback and the client’s PING event, include
both values in the final output, and parse/assert that each is true alongside
the existing success and exit-code checks.
- Around line 3514-3517: Update the child-process result handling around the
JSON.parse call to retain stderr, assert that stdout contains output before
parsing, and only parse after that assertion. Ensure failures expose the drained
stderr alongside the failed child-process result, while preserving the existing
result and exitCode assertions for successful execution.
In `@test/js/node/test/common/index.js`:
- Around line 1433-1464: Remove the duplicate build.module registrations for
internal/http, internal/streams/state, and internal/options in the test setup.
Keep the original registrations and their existing behavior unchanged, deleting
only the redundant second definitions.
In `@test/js/node/tls/node-tls-cert.test.ts`:
- Around line 553-717: Add coverage for rejectUnauthorized set to an empty
string in both TLS test paths: extend the tls.connect verification test
alongside the existing null/0 cases, and extend the requestCert tls.createServer
rejection test alongside the null/0 cases. Assert that empty-string values keep
verification enabled and untrusted clients remain rejected, matching the
existing expected errors and connection outcomes.
🪄 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: 7e0e5695-2586-4249-8567-e671a3f4a05c
📒 Files selected for processing (20)
packages/bun-uws/src/HttpParser.hsrc/js/internal/tls.tssrc/js/node/net.tssrc/js/node/tls.tssrc/jsc/bindings/node/http/JSConnectionsList.cppsrc/runtime/api/bun/h2/connection.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/webcore/FileSink.rstest/js/bun/http/request-smuggling.test.tstest/js/node/http/node-http-parser.test.tstest/js/node/http/node-http-transfer-encoding.test.tstest/js/node/http/node-http-with-ws.test.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2.test.jstest/js/node/test/common/index.jstest/js/node/test/sequential/test-http2-ping-flood.jstest/js/node/tls/node-tls-cert.test.ts
| tlsStringToProtocolVersion, | ||
| secureProtocolToVersionRange, | ||
| processPfxOptions, | ||
| normalizeRejectUnauthorized, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use normalizeRejectUnauthorized as the only normalizer.
Line 576 already converts every defined non-boolean value to true. Therefore, Lines 611-614 do not normalize null, 0, or "" in this function.
Replace the earlier conversion with the shared helper. Then remove the later duplicate block.
Proposed refactor
const rejectUnauthorized = options.rejectUnauthorized;
- if (rejectUnauthorized !== undefined && typeof rejectUnauthorized !== "boolean") {
- options = { ...options, rejectUnauthorized: true };
+ if (rejectUnauthorized !== undefined) {
+ options = {
+ ...options,
+ rejectUnauthorized: normalizeRejectUnauthorized(rejectUnauthorized),
+ };
}
...
- const rejectUnauthorized = options.rejectUnauthorized;
- if (rejectUnauthorized !== undefined && typeof rejectUnauthorized !== "boolean") {
- options = { ...options, rejectUnauthorized: normalizeRejectUnauthorized(rejectUnauthorized) };
- }As per coding guidelines, “Prefer the simplest honest shape” and use “named helpers for repeated blocks.”
Also applies to: 608-614
🤖 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 `@src/js/node/tls.ts` at line 13, Use normalizeRejectUnauthorized as the sole
normalization path in the surrounding TLS options handling: replace the earlier
inline conversion with this shared helper, ensuring null, 0, and empty-string
values are handled consistently, then remove the duplicate normalization block
later in the same flow.
Source: Coding guidelines
Comment on lines
+7979
to
+7990
| // send_data can re-enter JS mid-payload (batch flushes, prior writes' callbacks): pin | ||
| // + protect ArrayBuffer payloads so they can't be detached under the borrowed slice. | ||
| // Strings are immutable (zero-copy path); ThreadSafe's Drop releases the pin/protect. | ||
| let pin_payload = data_arg.is_cell() && data_arg.js_type().is_array_buffer_like(); | ||
| let buffer = match StringOrBuffer::from_js_with_encoding_maybe_async( | ||
| global_object, | ||
| data_arg, | ||
| encoding, | ||
| pin_payload, | ||
| true, | ||
| )? { | ||
| Some(b) => bun_jsc::ThreadSafe::adopt(b), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Resolve StringOrBuffer::from_js_with_encoding_maybe_async and ThreadSafe::adopt signatures and Drop.
set -euo pipefail
echo "== from_js_with_encoding_maybe_async =="
rg -n -C 20 'fn from_js_with_encoding_maybe_async' src
echo "== ThreadSafe definition, adopt, Drop =="
rg -n -C 10 'pub struct ThreadSafe|impl .*ThreadSafe|fn adopt' src/jsc src/bun_jsc 2>/dev/null || rg -n -C 10 'ThreadSafe' src
echo "== other call sites for comparison =="
rg -n -C 3 'from_js_with_encoding_maybe_async\(' srcRepository: oven-sh/bun
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Definitions =="
rg -n -C 18 --glob '*.rs' 'from_js_with_encoding_maybe_async|pub struct ThreadSafe|impl<.*ThreadSafe|impl ThreadSafe|fn adopt|impl.*Unprotect|fn unprotect' src/runtime src | head -n 1200
echo "== Target call sites =="
rg -n -C 8 --glob '*.rs' 'from_js_with_encoding_maybe_async\(' src/runtime/api/bun/h2_frame_parser.rs src/runtime/node src/runtime/api/BunObject.rs
echo "== ThreadSafe::adopt call sites =="
rg -n -C 8 --glob '*.rs' 'ThreadSafe::adopt\(' src/runtime src | head -n 800Repository: oven-sh/bun
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Target implementation and call =="
sed -n '7750,8010p' src/runtime/api/bun/h2_frame_parser.rs | cat -n
sed -n '285,325p' src/runtime/node/types.rs | cat -n
sed -n '495,580p' src/runtime/node/types.rs | cat -n
echo "== ThreadSafe definition and impl =="
rg -n --glob '*.rs' --glob '*.zig' --glob '*.ziggy' \
'pub struct ThreadSafe|struct ThreadSafe|impl[^\\n]*ThreadSafe|pub fn adopt|fn adopt|trait Unprotect' . \
| rg 'ThreadSafe|Unprotect' | head -n 300
echo "== Relevant ThreadSafe call sites =="
rg -n -C 12 --glob '*.rs' 'bun_jsc::ThreadSafe::adopt\(' src/runtime/api/bun/h2_frame_parser.rs src/runtime/api/JSTranspiler.rs src/runtime/api/BunObject.rs src/runtime/crypto/PBKDF2.rs src/runtimeRepository: oven-sh/bun
Length of output: 35862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,110p' src/jsc/node_path.rs | cat -n
rg -n -C 8 --glob '*.rs' 'ThreadSafe<' src/jsc src/runtime | head -n 500
rg -n -C 12 --glob '*.rs' 'impl.*Unprotect for StringOrBuffer|from_js_maybe_async_into' src/runtime/node/types.rsRepository: oven-sh/bun
Length of output: 35568
Label both conversion flags
The fourth argument is is_async; the fifth is allow_string_object. Keep these labels visible at the call site. ThreadSafe::adopt safely takes ownership of the existing protection and releases it once on drop.
🤖 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 `@src/runtime/api/bun/h2_frame_parser.rs` around lines 7979 - 7990, Update the
StringOrBuffer::from_js_with_encoding_maybe_async call in the send_data
conversion path to use explicit argument labels/comments for the fourth is_async
flag and fifth allow_string_object flag, preserving their current true values.
Leave ThreadSafe::adopt unchanged because it already takes ownership of the
existing protection and releases it exactly once.
Comment on lines
+1180
to
+1185
| b"host" if self.is_server || is_request => { | ||
| if value_b.is_empty() || saw_host { | ||
| malformed = true; | ||
| } | ||
| saw_host = true; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the Host validation to request blocks.
The guard is self.is_server || is_request. On a server, is_request is self.is_server && is_new, so it is false for a trailer section. The self.is_server disjunct therefore also applies the :authority-style Host rules to inbound request trailers.
nghttp2 routes a trailer block through http_trailer_on_header, which has no host case. A trailer carrying host: "" (or a repeated host) is delivered by node, but this code marks the block malformed and answers RST_STREAM(PROTOCOL_ERROR).
The comment above the arm states the intent as "in request blocks Host is checked like :authority". is_request already covers both inbound HEADERS request blocks and client-received PUSH_PROMISE blocks, so the extra disjunct is not needed.
🐛 Proposed fix to scope the check to request blocks
- b"host" if self.is_server || is_request => {
+ b"host" if is_request => {
if value_b.is_empty() || saw_host {
malformed = true;
}
saw_host = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| b"host" if self.is_server || is_request => { | |
| if value_b.is_empty() || saw_host { | |
| malformed = true; | |
| } | |
| saw_host = true; | |
| } | |
| b"host" if is_request => { | |
| if value_b.is_empty() || saw_host { | |
| malformed = true; | |
| } | |
| saw_host = true; | |
| } |
🤖 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 `@src/runtime/api/bun/h2/connection.rs` around lines 1180 - 1185, Restrict the
`b"host"` validation arm to `is_request` only by removing the `self.is_server`
disjunct. Preserve the existing empty-value and duplicate-Host checks for
request blocks, while allowing trailer sections to bypass this validation.
Comment on lines
+511
to
+520
| /// Every precondition under which [`Self::upgrade`] refuses. Callers that must not commit | ||
| /// the one-shot 101 preamble to the socket for an upgrade that will fail check this first; | ||
| /// `upgrade()` itself starts with it, so the two can never drift. | ||
| pub(crate) fn can_upgrade(&self) -> bool { | ||
| // `AnyServer` is a `Copy` type-erased pointer to the long-lived server, not `*self`. | ||
| let mut server = self.server; | ||
| !self.upgrade_context.get().context.is_null() | ||
| && server.web_socket_handler().is_some() | ||
| && !self.get_server_socket_value().is_empty() | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject WebSocket upgrades after an HTTP response starts.
NodeHTTPResponse::can_upgrade() does not reject committed status or body output. A getter can call res.writeHead() and leave the upgrade context valid. The later upgrade path can then append upgrade headers to a normal response.
src/runtime/server/NodeHTTPResponse.rs#L511-L520: requireraw_responseand rejectis_http_status_called()andis_http_write_called()incan_upgrade().src/runtime/server/server_body.rs#L1904-L1912: rely on the strengthened shared predicate before writing101or custom headers.test/js/node/http/node-http-with-ws.test.ts#L63-L98: add a getter that callsres.writeHead(200)before returning headers, then assert one valid200response and no upgrade headers.
As per coding guidelines, “Assume any operation that can run user JavaScript can synchronously free state; ... revalidate liveness after callbacks.”
📍 Affects 3 files
src/runtime/server/NodeHTTPResponse.rs#L511-L520(this comment)src/runtime/server/server_body.rs#L1904-L1912test/js/node/http/node-http-with-ws.test.ts#L63-L98
🤖 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 `@src/runtime/server/NodeHTTPResponse.rs` around lines 511 - 520, Strengthen
NodeHTTPResponse::can_upgrade() to require raw_response and reject both
is_http_status_called() and is_http_write_called() before permitting an upgrade.
In src/runtime/server/server_body.rs lines 1904-1912, rely on this shared
predicate before writing the 101 response or custom headers, with no separate
predicate change required there. In test/js/node/http/node-http-with-ws.test.ts
lines 63-98, add a header getter that calls res.writeHead(200) and assert
exactly one valid 200 response with no upgrade headers.
Source: Coding guidelines
Comment on lines
+933
to
+960
| // Under `is_shutting_down` the loop stops ticking: onWrite/onClose/EOF and queued | ||
| // FlushPendingFileSinkTask never arrive to balance these refs, so release them here | ||
| // (else e.g. a piped stdout with a `.pending` write strands its keep-alive forever). | ||
| if let Some(vm) = self.js_vm() { | ||
| if vm.is_shutting_down() { | ||
| let this = std::ptr::from_mut::<Self>(self); | ||
| // SAFETY: `this` is the canonical allocation pointer (finalize | ||
| // receives the wrapper's `m_ctx`); the wrapper's +1 is still | ||
| // held until the trailing `deref` below, so neither release | ||
| // can free `this` mid-body. `clear_keep_alive_ref` is | ||
| // flag-gated, so a (theoretical) late `onClose` is a no-op. | ||
| // SAFETY: `this` is the canonical alloc (finalize's `m_ctx`); the wrapper's +1 | ||
| // is held until the trailing `deref` so neither release frees `this` mid-body. | ||
| // `clear_keep_alive_ref` is flag-gated, so a late `onClose` is a no-op. | ||
| unsafe { FileSink::clear_keep_alive_ref(this) }; | ||
| if self.run_pending_later.has.get() { | ||
| self.run_pending_later.has.set(false); | ||
| // SAFETY: as above; balances the `ref_()` taken in | ||
| // `run_pending_later()` for a task that will never run. | ||
| unsafe { FileSink::deref(this) }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Under `is_shutting_down` the loop stops ticking: onWrite/onClose/EOF and queued | ||
| // FlushPendingFileSinkTask never arrive to balance these refs, so release them here | ||
| // (else e.g. a piped stdout with a `.pending` write strands its keep-alive forever). | ||
| if let Some(vm) = self.js_vm() { | ||
| if vm.is_shutting_down() { | ||
| let this = std::ptr::from_mut::<Self>(self); | ||
| // SAFETY: `this` is the canonical alloc (finalize's `m_ctx`); the wrapper's +1 | ||
| // is held until the trailing `deref` so neither release frees `this` mid-body. | ||
| // `clear_keep_alive_ref` is flag-gated, so a late `onClose` is a no-op. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the duplicated shutdown cleanup pass.
The cleanup in Lines 933-950 is repeated again from Line 952. The flag checks make the second pass a no-op today, so it does not currently double-release the references. Keep one pass, or extract one helper and call it once. This keeps the refcount cleanup auditable and prevents future edits from diverging.
As per coding guidelines: “Prefer the simplest honest shape ... and named helpers for repeated blocks.”
Proposed fix
- // Remove the second identical `is_shutting_down` cleanup block.🤖 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 `@src/runtime/webcore/FileSink.rs` around lines 933 - 960, Remove the
duplicated shutdown cleanup block in the relevant finalization method, keeping a
single execution of the vm.is_shutting_down() cleanup that calls
clear_keep_alive_ref and conditionally balances run_pending_later. Preserve the
existing flag checks, pointer lifetime guarantees, and deref behavior; do not
retain a second no-op pass.
Source: Coding guidelines
Comment on lines
+2937
to
+2974
| async function exchange(headerBlock) { | ||
| const frames = []; | ||
| const { promise: exchanged, resolve: onExchanged, reject: onSocketError } = Promise.withResolvers(); | ||
| const socket = net.connect(port, "127.0.0.1", () => { | ||
| socket.write(http2utils.kClientMagic); | ||
| socket.write(new http2utils.SettingsFrame(false).data); | ||
| socket.write(new http2utils.HeadersFrame(1, headerBlock, 0, true, true).data); | ||
| socket.write(new http2utils.PingFrame(false).data); | ||
| }); | ||
| socket.on("error", onSocketError); | ||
| let received = Buffer.alloc(0); | ||
| socket.on("data", chunk => { | ||
| received = Buffer.concat([received, chunk]); | ||
| while (received.length >= 9) { | ||
| const length = received.readUIntBE(0, 3); | ||
| if (received.length < 9 + length) break; | ||
| const frame = { | ||
| type: received[3], | ||
| flags: received[4], | ||
| streamId: received.readUInt32BE(5) & 0x7fffffff, | ||
| payload: Buffer.from(received.subarray(9, 9 + length)), | ||
| }; | ||
| received = received.subarray(9 + length); | ||
| frames.push(frame); | ||
| if ((frame.type === 6 && (frame.flags & 1) !== 0) || frame.type === 7) { | ||
| onExchanged(); | ||
| return; | ||
| } | ||
| } | ||
| }); | ||
| socket.on("close", () => onExchanged()); | ||
| try { | ||
| await exchanged; | ||
| } finally { | ||
| socket.destroy(); | ||
| } | ||
| return frames; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the shared literal and exchange helpers.
literal is now defined four times in this file (Lines 2703, 2897, 3000, 3074, 3148) and the 9-byte frame reader in exchange is duplicated at Lines 2804-2844 and 2937-2974. The two exchange bodies are byte-identical.
Move both into module-level helpers next to the other http2utils usage. A single frame reader keeps every new protocol test on the same parsing behavior.
🤖 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 `@test/js/node/http2/node-http2.test.js` around lines 2937 - 2974, Extract the
duplicated literal helper and the byte-identical exchange frame-reader logic
into module-level shared helpers near the existing http2utils usage. Replace all
five local literal definitions and both exchange implementations with calls to
those shared helpers, preserving their current framing, completion, error, and
socket-cleanup behavior.
Comment on lines
+3514
to
+3517
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| const result = JSON.parse(stdout.trim().split("\n").at(-1)); | ||
| expect(result).toEqual({ transferResult: "threw", dataBytes: 200 * 1024, corrupt: 0 }); | ||
| expect(exitCode).toBe(0); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Guard the JSON parse so a child failure reports the child output.
If the child never reaches sawEndStream, stdout is empty and JSON.parse("") throws SyntaxError: Unexpected end of JSON input. The exitCode assertion below never runs, and the drained stderr is discarded, so the real cause stays hidden.
Assert that the child produced output before parsing it, and keep stderr available in the failure.
♻️ Proposed change to surface the child output
- const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
- const result = JSON.parse(stdout.trim().split("\n").at(-1));
- expect(result).toEqual({ transferResult: "threw", dataBytes: 200 * 1024, corrupt: 0 });
- expect(exitCode).toBe(0);
+ const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+ expect({ stdout: stdout.trim(), stderr }).toMatchObject({ stdout: expect.stringContaining("transferResult") });
+ const result = JSON.parse(stdout.trim().split("\n").at(-1));
+ expect(result).toEqual({ transferResult: "threw", dataBytes: 200 * 1024, corrupt: 0 });
+ expect(exitCode).toBe(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| const result = JSON.parse(stdout.trim().split("\n").at(-1)); | |
| expect(result).toEqual({ transferResult: "threw", dataBytes: 200 * 1024, corrupt: 0 }); | |
| expect(exitCode).toBe(0); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect({ stdout: stdout.trim(), stderr }).toMatchObject({ stdout: expect.stringContaining("transferResult") }); | |
| const result = JSON.parse(stdout.trim().split("\n").at(-1)); | |
| expect(result).toEqual({ transferResult: "threw", dataBytes: 200 * 1024, corrupt: 0 }); | |
| expect(exitCode).toBe(0); |
🤖 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 `@test/js/node/http2/node-http2.test.js` around lines 3514 - 3517, Update the
child-process result handling around the JSON.parse call to retain stderr,
assert that stdout contains output before parsing, and only parse after that
assertion. Ensure failures expose the drained stderr alongside the failed
child-process result, while preserving the existing result and exitCode
assertions for successful execution.
Comment on lines
+3520
to
+3580
| it("http2 client survives a synchronous parser read from a closing stream's write callback", async () => { | ||
| // stream.close() concludes queued writes' callbacks synchronously while native code still | ||
| // holds the stream. A callback that synchronously feeds inbound bytes back into the parser | ||
| // must not let the deferred stream free run under that live reference (use-after-free). | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const http2 = require("node:http2"); | ||
| const { Duplex } = require("node:stream"); | ||
|
|
||
| function frame(type, flags, streamId, payload = Buffer.alloc(0)) { | ||
| const header = Buffer.alloc(9); | ||
| header.writeUIntBE(payload.length, 0, 3); | ||
| header[3] = type; | ||
| header[4] = flags; | ||
| header.writeUInt32BE(streamId, 5); | ||
| return Buffer.concat([header, payload]); | ||
| } | ||
|
|
||
| const socket = new Duplex({ | ||
| writableHighWaterMark: 4 * 1024 * 1024, | ||
| read() {}, | ||
| write(chunk, encoding, callback) { | ||
| callback(); | ||
| }, | ||
| }); | ||
|
|
||
| const client = http2.connect("http://localhost", { createConnection: () => socket }); | ||
| client.on("error", () => {}); | ||
| client.on("connect", () => { | ||
| socket.push(Buffer.concat([frame(4, 0, 0), frame(4, 1, 0)])); | ||
| }); | ||
| client.once("remoteSettings", () => { | ||
| const req = client.request({ ":method": "POST", ":path": "/" }); | ||
| req.on("error", () => {}); | ||
| // 65535 bytes fit the flow-control window; the remainder is queued with this callback. | ||
| req.write(Buffer.alloc(65535 + 32768, "a"), () => { | ||
| // Concluded synchronously by the close path below: feed inbound bytes so the parser | ||
| // re-enters read() while the closing stream is still referenced natively. | ||
| socket.push(frame(6, 0, 0, Buffer.alloc(8))); | ||
| }); | ||
| setImmediate(() => { | ||
| req.close(http2.constants.NGHTTP2_CANCEL); | ||
| setImmediate(() => { | ||
| console.log("REENTRANT_CLOSE_OK"); | ||
| process.exit(0); | ||
| }); | ||
| }); | ||
| }); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stdout).toContain("REENTRANT_CLOSE_OK"); | ||
| expect(exitCode).toBe(0); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that the reentrant path actually ran.
The test only asserts REENTRANT_CLOSE_OK and exit code 0. Both hold even when the intended sequence never happens: if the queued write callback is never concluded by req.close(), socket.push(frame(6, ...)) never runs, the parser is never re-entered from the callback, and the test still passes.
Emit an observable signal from inside the write callback and from the PING handling, then assert both. The test then fails when the reentrant path stops being exercised.
For example, set a flag in the req.write callback and include it in the printed line:
let reentered = false;
req.write(Buffer.alloc(65535 + 32768, "a"), () => {
reentered = true;
socket.push(frame(6, 0, 0, Buffer.alloc(8)));
});
client.on("ping", () => {
pingSeen = true;
});
// ...
console.log(JSON.stringify({ ok: "REENTRANT_CLOSE_OK", reentered, pingSeen }));Then assert reentered and pingSeen are true.
As per coding guidelines: "Tests must prove they fail for the intended reason and must exercise the actual production guards, constants, environment knobs, and preconditions." and "Every assertion must be able to fail and must assert the strongest meaningful invariant".
🤖 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 `@test/js/node/http2/node-http2.test.js` around lines 3520 - 3580, Strengthen
the reentrant-close test by tracking observable execution of the queued write
callback and PING handling. Add flags in the test around the req.write callback
and the client’s PING event, include both values in the final output, and
parse/assert that each is true alongside the existing success and exit-code
checks.
Source: Coding guidelines
Comment on lines
+1433
to
+1464
| // node's internal/http: serve the very same symbols Bun's _http_outgoing | ||
| // attaches to OutgoingMessage instances, so tests poke at real state. | ||
| build.module("internal/http", () => { | ||
| const { kOutHeaders, kHighWaterMark } = require("node:_http_outgoing"); | ||
| return { | ||
| loader: "object", | ||
| exports: { kOutHeaders, kHighWaterMark }, | ||
| }; | ||
| }); | ||
| // node's internal/streams/state: getDefaultHighWaterMark is also part of | ||
| // the public node:stream API, so reuse that (same function in Bun). | ||
| build.module("internal/streams/state", () => ({ | ||
| loader: "object", | ||
| exports: { getDefaultHighWaterMark: require("node:stream").getDefaultHighWaterMark }, | ||
| })); | ||
| // node's internal/options: map the few CLI options vendored http tests ask | ||
| // about onto the equivalent runtime values. Unknown options return undefined. | ||
| build.module("internal/options", () => ({ | ||
| loader: "object", | ||
| exports: { | ||
| getOptionValue(name) { | ||
| switch (name) { | ||
| case "--max-http-header-size": | ||
| return require("node:http").maxHeaderSize; | ||
| case "--insecure-http-parser": | ||
| return false; | ||
| default: | ||
| return undefined; | ||
| } | ||
| }, | ||
| }, | ||
| })); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm duplicate build.module specifiers in the shim and inspect duplicate-registration handling.
set -euo pipefail
file=$(fd -p 'test/js/node/test/common/index.js' | head -n1)
echo "== duplicate specifiers in $file =="
rg -n 'build\.module\(' "$file" | awk -F'"' '{print $2}' | sort | uniq -c | sort -rn
echo "== build.module registration / duplicate handling =="
rg -n -C 6 'build\.module|onLoadPlugins|module\(' --glob '*.ts' --glob '*.zig' --glob '*.rs' --glob '*.cpp' src | rg -n -C 6 -i 'duplicate|already|virtual module|module\(' || trueRepository: oven-sh/bun
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -p 'test/js/node/test/common/index.js' | head -n1)"
echo "== relevant setup block =="
sed -n '1360,1485p' "$file"
echo "== all build.module calls in the file =="
rg -n -C 2 'build\.module\(' "$file"
echo "== implementation and call sites for installBunExposeInternalsShim =="
rg -n -C 12 'installBunExposeInternalsShim|build\.module' --glob '*.js' --glob '*.ts' --glob '*.tsx' --glob '*.cpp' --glob '*.h' --glob '*.hpp' --glob '*.zig' --glob '*.rs' . \
| head -n 500Repository: oven-sh/bun
Length of output: 24900
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build.module API implementation references =="
rg -n -C 8 'build_module|BuildModule|module_callback|module\(' src packages test --glob '*.rs' --glob '*.cpp' --glob '*.h' --glob '*.hpp' --glob '*.zig' --glob '*.ts' --glob '*.js' \
| rg -m 300 -C 4 -i 'plugin|module|duplicate|specifier|virtual'
echo "== plugin registration internals =="
rg -n -C 10 'plugin_runner|PluginRunner|virtual module|module namespace|module specifier|register.*module|onResolve|onLoad' src --glob '*.rs' --glob '*.cpp' --glob '*.h' --glob '*.hpp' \
| head -n 600Repository: oven-sh/bun
Length of output: 50370
🌐 Web query:
Bun plugin build.module duplicate specifier behavior implementation
💡 Result:
In Bun's plugin API, the build.module(specifier, callback) method is used to register a lazy-loaded virtual module [1][2]. Regarding duplicate specifiers, the implementation is as follows: 1. Restriction on Built-ins: You cannot use build.module() to override built-in modules (e.g., "buffer") [3]. Attempting to do so will result in an error [3]. 2. Virtual Module Tracking: When using build.module(), Bun maintains a map of virtual modules [4]. If you attempt to register a duplicate specifier, the underlying implementation typically relies on this registry, which tracks added, retrieved, and existing virtual modules [4]. 3. Plugin Lifecycle and Overriding: While build.module() is a convenience method for virtual modules, standard resolution is handled via onResolve and onLoad [5][6]. If multiple plugins or multiple calls to build.module() attempt to define the same specifier, the behavior is governed by the order of plugin execution and how the onResolve and onLoad hooks are implemented [6]. If a module is already registered in the virtual module registry, subsequent attempts to override it via build.module() may be rejected or may conflict depending on the specific version of Bun [4]. 4. Known Issues: Users have reported instances where Bun's bundler incorrectly handles module imports, leading to duplicated outputs [7]. However, this is distinct from the behavior of the build.module() specifier registration itself, which is designed to provide a single, unique virtual entry point for a given specifier [1][4]. To ensure predictable behavior, it is recommended to avoid defining the same specifier in multiple plugins unless explicitly intended, and to rely on onResolve to control which plugin ultimately claims the resolution of a specific module [5][8]. If you require complex resolution logic, using onResolve and onLoad provides more control over the resolution process than the simplified build.module() interface [5][6].
Citations:
- 1: https://bun.sh/reference/bun/PluginBuilder/module
- 2: https://bun.sh/reference/bun/PluginBuilder
- 3: Update comment to include "except a built-in" #8251
- 4: Add build.module() support for Bun.build plugins #22486
- 5: https://bun.sh/docs/runtime/plugins
- 6: https://mintlify.wiki/oven-sh/bun/bundler/plugins
- 7: Bun incorrectly duplicates module imports when bundling TypeScript #18213
- 8: https://bun.com/reference/bun/PluginBuilder/onResolve
Remove the duplicate build.module registrations. The second registrations of internal/http, internal/streams/state, and internal/options are redundant and can cause registration conflicts.
🤖 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 `@test/js/node/test/common/index.js` around lines 1433 - 1464, Remove the
duplicate build.module registrations for internal/http, internal/streams/state,
and internal/options in the test setup. Keep the original registrations and
their existing behavior unchanged, deleting only the redundant second
definitions.
Comment on lines
+553
to
+717
| it("tls.connect with rejectUnauthorized: null still rejects untrusted certificates", async () => { | ||
| // Node normalizes rejectUnauthorized with `!== false`: only an explicit `false` disables | ||
| // verification. `null` (a common config-file "use the default") must keep it enforced. | ||
| const { promise, resolve, reject } = Promise.withResolvers(); | ||
| let server: Server | null = null; | ||
| let socket: TLSSocket | null = null; | ||
|
|
||
| try { | ||
| server = tls | ||
| .createServer({ | ||
| key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), | ||
| cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), | ||
| passphrase: "123123123", | ||
| }) | ||
| .on("error", reject) | ||
| .listen(0, () => { | ||
| const address = server?.address() as AddressInfo; | ||
| socket = tls | ||
| .connect({ port: address.port, rejectUnauthorized: null as unknown as boolean }, () => { | ||
| reject(new Error("secureConnect must not fire when verification failed")); | ||
| }) | ||
| .on("error", resolve); | ||
| }); | ||
|
|
||
| const err = await promise; | ||
| expect(err.code).toBe("UNABLE_TO_VERIFY_LEAF_SIGNATURE"); | ||
| } finally { | ||
| //@ts-ignore | ||
| socket?.end(); | ||
| server?.close(); | ||
| } | ||
| }); | ||
|
|
||
| it("tls.connect with rejectUnauthorized: 0 keeps verification on like node", async () => { | ||
| // Node's normalization is `!== false`, so falsy non-`false` values (0, "") keep | ||
| // verification enforced; they must not blow up in the native strict-boolean conversion. | ||
| const { promise, resolve, reject } = Promise.withResolvers(); | ||
| let server: Server | null = null; | ||
| let socket: TLSSocket | null = null; | ||
|
|
||
| try { | ||
| server = tls | ||
| .createServer({ | ||
| key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), | ||
| cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), | ||
| passphrase: "123123123", | ||
| }) | ||
| .on("error", reject) | ||
| .listen(0, () => { | ||
| const address = server?.address() as AddressInfo; | ||
| socket = tls | ||
| .connect({ port: address.port, rejectUnauthorized: 0 as unknown as boolean }, () => { | ||
| reject(new Error("secureConnect must not fire when verification failed")); | ||
| }) | ||
| .on("error", resolve); | ||
| }); | ||
|
|
||
| const err = await promise; | ||
| expect(err.code).toBe("UNABLE_TO_VERIFY_LEAF_SIGNATURE"); | ||
| } finally { | ||
| //@ts-ignore | ||
| socket?.end(); | ||
| server?.close(); | ||
| } | ||
| }); | ||
|
|
||
| it("tls.createServer with rejectUnauthorized: 0 still rejects a client with an untrusted certificate", async () => { | ||
| // The server-side gate runs in JS after the handshake (the native verify callback always | ||
| // defers to JS), so the raw option value must be normalized like Node (`!== false`): a | ||
| // falsy non-`false` value must not admit a client whose certificate failed verification. | ||
| let server: Server | null = null; | ||
| let socket: TLSSocket | null = null; | ||
| const secureConnections: string[] = []; | ||
| const clientErrors = Promise.withResolvers<Error>(); | ||
| const clientClosed = Promise.withResolvers<string>(); | ||
| const serverTLS = { | ||
| key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), | ||
| cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), | ||
| passphrase: "123123123", | ||
| }; | ||
|
|
||
| try { | ||
| server = tls | ||
| .createServer( | ||
| { | ||
| ...serverTLS, | ||
| requestCert: true, | ||
| rejectUnauthorized: 0 as unknown as boolean, | ||
| }, | ||
| s => { | ||
| secureConnections.push(s.authorizationError as string); | ||
| s.write("admitted"); | ||
| }, | ||
| ) | ||
| .on("tlsClientError", clientErrors.resolve) | ||
| .listen(0, () => { | ||
| const address = server?.address() as AddressInfo; | ||
| // The client presents a certificate the server cannot verify (self-signed, no CA). | ||
| socket = tls.connect({ port: address.port, rejectUnauthorized: false, ...serverTLS }); | ||
| let received = ""; | ||
| socket.on("data", chunk => (received += chunk)); | ||
| socket.on("error", () => {}); | ||
| socket.on("close", () => clientClosed.resolve(received)); | ||
| }); | ||
|
|
||
| const [, received] = await Promise.all([clientErrors.promise, clientClosed.promise]); | ||
| // The unverified client is rejected: no 'secureConnection', no bytes served. | ||
| expect({ secureConnections, received }).toEqual({ secureConnections: [], received: "" }); | ||
| } finally { | ||
| //@ts-ignore | ||
| socket?.end(); | ||
| server?.close(); | ||
| } | ||
| }); | ||
|
|
||
| it("tls.createServer with rejectUnauthorized: null still rejects unauthorized clients", async () => { | ||
| // Server side of the same normalization: with requestCert, a client that fails | ||
| // verification must be destroyed unless rejectUnauthorized is explicitly `false` | ||
| // (Node's `!== false`), so `null` must never silently admit an unverified peer. | ||
| let server: Server | null = null; | ||
| let socket: TLSSocket | null = null; | ||
| const secureConnections: string[] = []; | ||
| const clientErrors = Promise.withResolvers<Error>(); | ||
| const clientClosed = Promise.withResolvers<string>(); | ||
|
|
||
| try { | ||
| server = tls | ||
| .createServer( | ||
| { | ||
| key: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.key")), | ||
| cert: readFileSync(join(import.meta.dir, "..", "http", "fixtures", "openssl.crt")), | ||
| passphrase: "123123123", | ||
| requestCert: true, | ||
| rejectUnauthorized: null as unknown as boolean, | ||
| }, | ||
| s => { | ||
| secureConnections.push(s.authorizationError as string); | ||
| s.write("admitted"); | ||
| }, | ||
| ) | ||
| .on("tlsClientError", clientErrors.resolve) | ||
| .listen(0, () => { | ||
| const address = server?.address() as AddressInfo; | ||
| // The client presents no certificate, so the server's verification fails. | ||
| socket = tls.connect({ port: address.port, rejectUnauthorized: false }); | ||
| let received = ""; | ||
| socket.on("data", chunk => (received += chunk)); | ||
| socket.on("error", () => {}); | ||
| socket.on("close", () => clientClosed.resolve(received)); | ||
| }); | ||
|
|
||
| const [err, received] = await Promise.all([clientErrors.promise, clientClosed.promise]); | ||
| // The unauthorized client is rejected: no 'secureConnection', no bytes served. | ||
| expect({ secureConnections, received, code: (err as NodeJS.ErrnoException).code }).toEqual({ | ||
| secureConnections: [], | ||
| received: "", | ||
| code: "ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE", | ||
| }); | ||
| } finally { | ||
| //@ts-ignore | ||
| socket?.end(); | ||
| server?.close(); | ||
| } | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/js/node/tls/node-tls-cert.test.ts --items all
rg -n -C 5 'rejectUnauthorized:\s*(""|'\'''\''|0|null|undefined|false)' \
test/js/node/tls/node-tls-cert.test.tsRepository: oven-sh/bun
Length of output: 7500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rejectUnauthorized implementation references ---'
rg -n -C 6 'rejectUnauthorized' src/js/node/tls.ts src test/js/node/tls/node-tls-cert.test.ts 2>/dev/null | head -n 240
printf '%s\n' '--- surrounding existing TLS certificate tests ---'
sed -n '240,370p' test/js/node/tls/node-tls-cert.test.tsRepository: oven-sh/bun
Length of output: 19842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("test/js/node/tls/node-tls-cert.test.ts")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "rejectUnauthorized" in line:
print(f"{i}:{line}")
PY
printf '%s\n' '--- empty-string rejectUnauthorized uses across tracked tests ---'
rg -n -S 'rejectUnauthorized\s*:\s*([\"'\"']{2})' --glob 'test/**' . || trueRepository: oven-sh/bun
Length of output: 1716
Add empty-string coverage for rejectUnauthorized on both TLS paths.
The implementation treats rejectUnauthorized: "" as verification enabled, but this file does not test it for tls.connect or a certificate-requesting tls.createServer.
🤖 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 `@test/js/node/tls/node-tls-cert.test.ts` around lines 553 - 717, Add coverage
for rejectUnauthorized set to an empty string in both TLS test paths: extend the
tls.connect verification test alongside the existing null/0 cases, and extend
the requestCert tls.createServer rejection test alongside the null/0 cases.
Assert that empty-string values keep verification enabled and untrusted clients
remain rejected, matching the existing expected errors and connection outcomes.
Source: Coding guidelines
…2-compat-hardening
…2-compat-hardening # Conflicts: # packages/bun-uws/src/HttpParser.h
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/runtime/server/server_body.rs (1)
1904-1912: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRun the post-getter guard for every option shape.
opts.fast_get(...)can re-enter JavaScript. The guard at Line 1907 runs only whenheaders_valueis present and non-null.If a getter ends or destroys the response and
headersis absent,undefined, ornull, thebreak 'getterpath skips the guard. The function then callsNodeHTTPResponse::upgradeat Line 1928. Insrc/runtime/server/NodeHTTPResponse.rsLines 514-622,upgrade()recheckscan_upgrade(), butcan_upgrade()does not rejectENDEDorSOCKET_CLOSED.Add the same state and eligibility check after the complete
'getterblock, before Line 1928. Keep the existing check before writing101 Switching Protocols.As per coding guidelines, revalidate state after every callback that can run JavaScript.
Proposed fix
if global.has_exception() { return Err(JsError::Thrown); } } + if node_http_response.flags.get().intersects( + NodeHTTPResponseFlags::ENDED | NodeHTTPResponseFlags::SOCKET_CLOSED, + ) || !node_http_response.can_upgrade() + { + return Ok(JSValue::FALSE); + } return Ok(JSValue::from(node_http_response.upgrade(🤖 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 `@src/runtime/server/server_body.rs` around lines 1904 - 1912, Extend the post-getter guard in the upgrade flow so it runs after the complete 'getter block, covering absent, undefined, and null headers as well as present headers. Add the same ENDED/SOCKET_CLOSED and can_upgrade() check immediately before NodeHTTPResponse::upgrade, while preserving the existing guard before committing the 101 response.Source: Coding guidelines
src/runtime/webcore/FileSink.rs (1)
1581-1589: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate inline termination failures.
FileSink::endreturnssys::Result<()>, but bothEndedInlinepaths discard termination errors before returningJSValue::UNDEFINED. MapErrthrough the existing JS error path and preserveEndedInline(Some(err))errors.🤖 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 `@src/runtime/webcore/FileSink.rs` around lines 1581 - 1589, Update the NativeWireResult::EndedInline handling to propagate failures from both self.end_from_stream(Some(err)) and self.end(None) through the existing JS error path instead of discarding their Results. Preserve the original Some(err) stream error while returning JSValue::UNDEFINED only after successful termination.Source: Coding guidelines
test/js/bun/http/request-smuggling.test.ts (1)
565-565: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject socket errors from
rawPost.Line 565 resolves the helper when
clientemitserror. A peer can send a partialHTTP/1.1 400response and then reset the socket. The test can then pass while hiding the transport failure. Destructurerejectand attach it to the error event.As per coding guidelines, “Tests must await observable conditions, wire every failure event to rejection.”
Proposed fix
- const { promise, resolve } = Promise.withResolvers<{ status: string; raw: string }>(); + const { promise, resolve, reject } = Promise.withResolvers<{ status: string; raw: string }>(); ... - client.on("error", done); + client.on("error", reject);🤖 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 `@test/js/bun/http/request-smuggling.test.ts` at line 565, Update the rawPost helper’s Promise handling around the client error listener so it destructures reject and routes client socket errors to rejection instead of resolving via done. Preserve successful response resolution while ensuring every client error event fails the awaited test.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/server/server_body.rs`:
- Around line 1904-1912: Extend the post-getter guard in the upgrade flow so it
runs after the complete 'getter block, covering absent, undefined, and null
headers as well as present headers. Add the same ENDED/SOCKET_CLOSED and
can_upgrade() check immediately before NodeHTTPResponse::upgrade, while
preserving the existing guard before committing the 101 response.
In `@src/runtime/webcore/FileSink.rs`:
- Around line 1581-1589: Update the NativeWireResult::EndedInline handling to
propagate failures from both self.end_from_stream(Some(err)) and self.end(None)
through the existing JS error path instead of discarding their Results. Preserve
the original Some(err) stream error while returning JSValue::UNDEFINED only
after successful termination.
In `@test/js/bun/http/request-smuggling.test.ts`:
- Line 565: Update the rawPost helper’s Promise handling around the client error
listener so it destructures reject and routes client socket errors to rejection
instead of resolving via done. Preserve successful response resolution while
ensuring every client error event fails the awaited test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc0d31e6-af07-43e1-ac8d-95d44ab6a1e8
📒 Files selected for processing (5)
packages/bun-uws/src/HttpParser.hsrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/webcore/FileSink.rstest/js/bun/http/request-smuggling.test.ts
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Stacked on #32488 (
claude/node-http-http2-compat). A security review of that branch reported findings across the repo; this PR fixes the ones that live in the code the base branch owns — the node:http/https/http2 compat layer, the h2 engine, node:net/tls glue, and the uWS parser paths the branch touches — each verified against the code and against the reference implementation (Node v26.3.0 / nghttp2 / llhttp) before changing anything.Every claim below was re-verified from source rather than taken from the report; three of the reported items turned out not to be reproducible vulnerabilities on this branch and are labeled as such (kept only where the change is cheap Node-parity hardening).
Fixes
:method/:scheme/:path(and empty pseudo-header values, pseudo-headers in trailers, CONNECT carrying:scheme/:path, an empty or repeatedHoststanding in for:authority), delivering requests whosereq.method/req.urlareundefined. Now rejected with a streamPROTOCOL_ERROR, exactly nghttp2'snghttp2_http_on_request_headers.src/runtime/api/bun/h2/connection.rslib/nghttp2_http.c200 OK(the status check in the exception arm was inverted;end_streamwrites200). Now: nothing written →500; status/body already on the wire → the connection is closed without valid framing, so a failure can never read as a complete response. Same-class fix applied to the async rejection sibling.src/runtime/server/mod.rs,src/runtime/server/NodeHTTPResponse.rsConnectionsList.idle()/expired()dereferenced aclose()d parser's freed impl (segfault); every other post-close entry point already guards.src/jsc/bindings/node/http/JSConnectionsList.cppremove()afterclose()is a no-op)maxHeaderSize: 0as "unlimited" (Node: 0 means "use the default"), so a never-terminating trailer stream grew a per-connectionstd::stringwithout bound.0now resolves to the same process default the header limits use (--max-http-header-size/http.maxHeaderSize), with the 16 KiB constant as the parser's own fallback.packages/bun-uws/src/HttpParser.h,ChunkedEncoding.hnode_http_parser.cc(max_http_header_size == 0→ default)maxHeaderSize: 0case)server.upgrade(nodeHttpResponse, …)flushed the one-shot101 Switching Protocolspreamble + caller headers before validating the upgrade, so a refused upgrade (false) corrupted the app's documented fallback response. Validation now happens first, like theRequestbranch.src/runtime/server/server_body.rs,NodeHTTPResponse.rswriteStatusis one-shotTransfer-Encoding: chunked, chunked(one header line) was accepted and framed as chunked; llhttp/Node reject any coding after achunkedtoken (HPE_INVALID_TRANSFER_ENCODING). Only the multi-line form was rejected before.packages/bun-uws/src/HttpParser.hallowHTTP1HTTP/1.1 fallback: the JS response handle serializedHTTP/1.1 ${statusCode} ${statusMessage}without the validation the native handle performs. The handle now enforces the same invariants (ERR_HTTP_INVALID_STATUS_CODE,ERR_INVALID_CHAR). On this branch the implicit-header path already routes through the validating JSwriteHead, so this is defense-in-depth, not a reproducible split.src/js/node/http2.ts_http_server.jswriteHeadrejectUnauthorizedis now normalized the way Node does (!== false) through one named helper at every ingestion site, every handshake gate (client, server accept, server error path), and the option→native chokepoint. Investigated the reported "null/0 disables verification": not reproducible on this branch — each concrete path is already enforced by another layer (tls.connect's secure-connect handler; the native server-side verify) — so this is Node-parity normalization (it also stops Node-legal falsy spellings from being rejected by the strict native boolean conversion).src/js/node/net.ts,src/js/node/tls.ts_tls_wrap.jsArrayBufferis now pinned for the duration of the send (the engine re-enters JS mid-payload; the inbound path already copies for the same reason). Investigated the reported detach-UAF: not reproducible from JS (JSC refuses to transfer the buffer while the write host call is on the stack), so this is defense-in-depth against the remaining detach channels.src/runtime/api/bun/h2_frame_parser.rspending_engine_stream_closes) could run inside aparser.read()re-entered synchronously from a JS write callback while native frames still held&mut Stream. The drain is now gated on a JS-dispatch depth counter and re-run at quiescent host-call boundaries (read/writeStream/rstStreamentry) so nothing is retained past them.src/runtime/api/bun/h2_frame_parser.rsNew tests: 11, added to the existing suites (
node-http2.test.js,h2-conformanceneighborhood,node-http.test.ts,node-http-parser.test.ts,node-http-transfer-encoding.test.ts,node-http-with-ws.test.ts,request-smuggling.test.ts,node-tls-cert.test.ts).Review round
A multi-agent adversarial review of the diff was run before submitting; everything it confirmed was addressed here: the missed server-side
rejectUnauthorizedgate (now one shared helper at every site), the trailer cap resolving to the wrong constant, a stream-retention gap introduced by the first version of the deferred-free gate, the empty/repeatedHostacceptance, a string-copy regression in the first version of the h2 write pinning (strings keep the zero-copy path), and duplication/robustness cleanups (shared failed-response helper,upgrade()consumingcan_upgrade(), hoisted pseudo-header bits, spawned tests draining stderr). One genuine gap it found is intentionally not in this PR: on the client, h2 trailer blocks still accept pseudo-headers (nghttp2 rejects them both directions); distinguishing a client's response block from its trailers needs per-stream state, so that is left as a named follow-up rather than a partial fix.Round 2 (after the first push): the two review comments were addressed in
e3090fd45db— the h2 connection unit tests now send:scheme/:authorityso they satisfy the new §8.3.1 validation, and the doc comments that two new helpers had been inserted under are re-attached torewrite_read/failWrite. The same commit fixes theLint JavaScriptfailure on the previous head (oxlint's double-property-read rule:_rejectUnauthorizedis read once into a local in the two TLS handshake gates), which was also the only cause of the two red Windows test-runner jobs in build #67590 (they run the same lint as a test).Round 3: the reviewer caught a real over-rejection in the first version of the Host rule — on a client, a received response block was misclassified as a request block (the inbound engine only tracks streams it has seen inbound, so a response always looks like a new stream), which made the new empty/repeated-Host checks fire on responses that nghttp2 delivers.
e63fd768b34classifies request blocks as server-received HEADERS (PUSH_PROMISE tags itself), gates the Host rule on that, and adds a client regression test (raw response carryinghost: ""plus a repeatedhost; node delivers it, the previous head RST it). A second comment asking to letHostsatisfy CONNECT's:authorityrequirement was declined with nghttp2 citations: nghttp2 keepsHostand:authorityin separate flags and requires:authorityfor both CONNECT forms.Round 4:
b4290761ba3extends the §8.3.1 finalization to client-received PUSH_PROMISE blocks (nghttp2 finalizes them with the samenghttp2_http_on_request_headers), and a malformed promised request is now rejected the way node rejects it — aGOAWAY(PROTOCOL_ERROR)connection error with no'stream'event (verified against node with a raw-socket server; the previous head delivered such a push to JS with:scheme/:authoritymissing). Known pre-existing residual left alone: after the engine's error GOAWAY the JS session teardown sends a secondGOAWAY(INTERNAL_ERROR), where node sends only the first.How this was verified
node-http2.test.js291/291,h2-conformance24/24,node-http.test.ts130/130,request-smuggling61/61,node-tls-cert30/30,node-net46/46, plus the parser/trailer/ws/timeout/backpressure files — 0 failures.test/js/node/test/parallel/test-http2-*suite was swept on the fixed build to confirm the new h2 request validation does not over-reject (nghttp2-conformant blocks, CONNECT, trailers).Out of scope (from the same review)
The remaining findings in the report are pre-existing, repo-wide issues in subsystems this branch does not touch (package manager/supply chain, JSC structured-clone/crypto natives, FFI, shell, SQL drivers, WASI, bundler/CSS/markdown, dev tooling, fetch client core). They are intentionally not bundled into this stacked PR so both stay reviewable; each should be its own PR against
main.Merged with main's hardening round 11 (#33072)
main independently landed its own versions of two mechanisms this PR carried, so after merging the updated base branch they collapse onto the versions on
main: the frame parser's dispatch-depth deferral (main'senter_dispatch/enter_stream_dispatch; this PR keeps the named drain helper and its extra quiescent call sites) and the allowHTTP1writeHeadstatus validation. This PR's remaining engine substance is the request-side validation main does not have — the RFC 9113 §8.3.1 pseudo-header rules, theHosthandling (request blocks only, per nghttp2), and PUSH_PROMISE validation with node's connection-error semantics — now composed with main's per-stream trailer tracking and content-length enforcement (this PR's earlier per-block trailer heuristic was dropped in favor of main's; that also covers the client-side trailer case previously listed here as a follow-up). The full battery (both this PR's regression tests and the round-11 conformance tests) passes on the merged tree.