node:http2: fix allowHTTP1 fallback mangling HTTP/1.1 response headers#28657
node:http2: fix allowHTTP1 fallback mangling HTTP/1.1 response headers#28657robobun wants to merge 5 commits into
Conversation
|
Updated 6:07 AM PT - Jun 18th, 2026
❌ @robobun, your commit b662b76 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28657That installs a local version of the PR into your bun-28657 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds HTTP/1.1 protocol fallback support to HTTP/2 secure servers. It implements request and response handling classes, chunked transfer decoding, header validation with size limits, and updates ALPN protocol advertisement. Includes comprehensive regression testing for the new functionality. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 3953-3961: The current onData handler repeatedly does buffer =
Buffer.concat([buffer, chunk]), which copies data each chunk; change to
accumulate incoming chunks in an array (e.g., chunks: Buffer[]) and maintain a
totalLength counter, push each chunk in onData, and only call
Buffer.concat(chunks, totalLength) inside tryParseRequest (or a helper invoked
by tryParseRequest) when you actually need a contiguous buffer for header
parsing; update references from buffer to the concatenated result and
clear/adjust chunks/totalLength after consumption.
- Around line 4038-4054: The chunked handler duplicates data and doesn't decode
chunked framing: modify the onChunkedData logic (and remove the immediate push
of remainder) so you accumulate raw bytes in chunkedBuffer and parse the chunked
encoding frames (read hex length lines, consume that many bytes, skip CRLFs) to
extract only the decoded payload bytes; push each decoded payload segment into
req (instead of raw chunk/frame bytes), stop and push null when a zero-length
chunk is seen, and remove the redundant initial push of remainder and the
existing req.push(chunk) that causes duplication; update socket.removeListener
usage to the onChunkedData function and set req.complete = true when finished.
🪄 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: 4f8a51ae-5667-4186-9af0-e0023288a160
📥 Commits
Reviewing files that changed from the base of the PR and between a27a507 and a0d6868fba08f694a795c6fa45f98185ecdd54cf.
📒 Files selected for processing (2)
src/js/node/http2.tstest/regression/issue/28656.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 4087-4121: The chunked branch currently discards any bytes after
the "0\r\n\r\n" terminator; update both locations that handle the terminator
(inside onChunkedData and the initial remainder check) to locate the terminator
index in accumulated, compute leftover = accumulated.slice(terminatorIndex +
terminatorLength), and if leftover.length > 0 re-insert those bytes back into
the socket/read stream (e.g. socket.unshift(leftover) or emit a "data" with
leftover) before removing the data listener and finishing the request;
references: onChunkedData, accumulated, decodeChunkedBody, and the initial
chunked remainder check.
🪄 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: a14b76ad-6d94-4f72-8af2-8c27f1a31cc2
📥 Commits
Reviewing files that changed from the base of the PR and between a0d6868fba08f694a795c6fa45f98185ecdd54cf and 7d4c2b02c3a6874f28773bcc963c0b850eb7d98e.
📒 Files selected for processing (2)
src/js/node/http2.tstest/regression/issue/28656.test.ts
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/webcore/Blob.zig (1)
1144-1151:⚠️ Potential issue | 🟡 MinorUse the destination store when formatting bytes→S3 upload failures.
Line 1150 now attaches async stack information, but it still pulls the path from the source bytes store captured on Line 1177. For
Bun.write("s3://...", bytes), a failed upload will usually report the source blob name ornullinstead of the destination object key.🛠️ Suggested fix
const Wrapper = struct { - store: *Store, + source_store: *Store, + destination_store: *Store, promise: jsc.JSPromise.Strong, global: *jsc.JSGlobalObject, @@ - .success => try this.promise.resolve(this.global, .jsNumber(this.store.data.bytes.len)), + .success => try this.promise.resolve(this.global, .jsNumber(this.source_store.data.bytes.len)), .failure => |err| { - try this.promise.reject(this.global, err.toJSWithAsyncStack(this.global, this.store.getPath(), this.promise.get())); + try this.promise.reject(this.global, err.toJSWithAsyncStack(this.global, this.destination_store.getPath(), this.promise.get())); }, @@ - this.store.deref(); + this.source_store.deref(); + this.destination_store.deref(); } }; - source_store.ref(); + source_store.ref(); + destination_store.ref(); @@ - .store = source_store, + .source_store = source_store, + .destination_store = destination_store,Also applies to: 1176-1178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/webcore/Blob.zig` around lines 1144 - 1151, The resolve callback for S3 uploads uses the source bytes store path when building the async-stack error (it calls this.store.getPath()), causing failed Bun.write("s3://...", bytes) uploads to show the source blob name; change the rejection path to use the destination store/object key instead by retrieving the destination store or destination path from the same context used for the upload (e.g., the destinationStore or destinationPath field captured for the S3 operation) and call its getPath()/equivalent when creating the JS error in resolve (function resolve and S3.S3UploadResult failure branch), and apply the same change in the analogous code at the other occurrence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Around line 2326-2328: The async stack-attachment code currently treats
globalObject->stackTraceLimit() == 0 as "attach nothing", causing inconsistency
with the synchronous stack path in FormatStackTraceForJS.cpp which normalizes 0
to Bun's default; update the logic around size_t limit =
globalObject->stackTraceLimit().value_or(10) in bindings.cpp so that when
stackTraceLimit() returns 0 you replace it with the same default used by
FormatStackTraceForJS.cpp (i.e., normalize 0 to the default frame count) before
early-returning, ensuring async-attached stacks and regular stacks use the same
limit behavior.
In `@src/bun.js/bindings/ErrorStackTrace.cpp`:
- Around line 135-150: The raw interpreter walk always requests the full stack
via vm.interpreter.getStackTrace(owner, rawFrames, 1,
std::numeric_limits<size_t>::max()), which can allocate huge rawFrames even when
caller is not an object and later we immediately cap to stackTraceLimit; change
the call to bound the requested max frames when caller is not an object by
passing a computed maxFrames (e.g. use stackTraceLimit when !caller.isObject(),
otherwise keep the existing unbounded behavior) so getStackTrace is only asked
for as many frames as we will use; update the call site near
vm.interpreter.getStackTrace and ensure downstream logic that reserves and
filters rawFrames remains correct.
In `@src/codegen/cppbind.ts`:
- Around line 765-770: Normalize the CLI-provided cxxSourcesPath to an absolute
path before performing file I/O: import/require the node path module (if not
already present), compute const absCxxSourcesPath =
path.resolve(cxxSourcesPath), and then call Bun.file(absCxxSourcesPath).text()
(replace the current Bun.file(cxxSourcesPath).text() usage that produces
allCppFiles). Ensure the new absCxxSourcesPath variable is used everywhere
cxxSourcesPath was passed to file operations so all file handling in functions
like the allCppFiles read is deterministic and absolute.
In `@src/js/node/http2.ts`:
- Around line 4279-4283: Validate header names, header values, trailer
names/values and status text before storing or concatenating them: use the
validators from internal/validators to check inputs in setHeader, the other
header setters around the 4315-4372 range (e.g., addTrailers, writeHead) and in
_flushHeaders; on invalid types throw the appropriate $ERR_* errors (e.g.,
$ERR_INVALID_ARG_TYPE, $ERR_INVALID_ARG_VALUE or the HTTP-specific
ERR_HTTP_INVALID_HEADER_NAME / ERR_HTTP_INVALID_STATUS_CODE) instead of
accepting raw strings so CR/LF injection cannot be written verbatim to the
socket.
- Around line 4387-4397: The framing logic that currently lives inline before
this._flushHeaders() should be extracted into a single helper (e.g.
determineResponseFraming or computeFraming) and invoked from write(), end(), and
flushHeaders(); the helper should inspect this._hasBody,
this[kHttp1FallbackHeadersSent], this.hasHeader("content-length"),
this.hasHeader("transfer-encoding") and the request HTTP version (only set
chunked for HTTP/1.1+), then set this._chunked and call
this.setHeader("Transfer-Encoding","chunked") when appropriate; update the
occurrences around the current block and the other places mentioned (the blocks
at 4432-4440 and 4467-4469) to call this shared helper instead of duplicating
the chunked decision.
- Around line 3961-3965: The code marks the chunked body complete as soon as a
zero-size chunk is seen, even if the final CRLF/trailer terminator ("\r\n\r\n")
hasn't fully arrived; change the logic so that when chunkSize === 0 you first
search for the full trailer terminator in buf (using trailerEnd =
buf.indexOf("\r\n\r\n", crlfIdx)) and only set complete = true and advance
offset (offset = trailerEnd + 4) if trailerEnd !== -1; if trailerEnd === -1, do
not set complete and return/wait for more data so the terminating blank line is
fully buffered (use the same variables: chunkSize, complete, trailerEnd, buf,
crlfIdx, offset).
- Around line 4085-4102: The request body data listeners (e.g., onBodyData and
onChunkedData) remain attached when a handler finishes, so finish() must not
re-arm the generic onData keep-alive parser while a body parser is active;
update the code around onBodyData/onChunkedData and the finish/onData re-arming
logic to (1) track an active body-parser flag (or check for existing body-parser
listeners) and only install the generic onData handler when no body parser is
attached, (2) ensure body-parser listeners are explicitly removed on request
completion or socket close, and (3) abort/cleanup the fallback request if the
socket closes mid-body so no stale parser consumes subsequent bytes; apply the
same pattern to the other similar blocks (the onChunkedData variants referenced
in the review).
---
Outside diff comments:
In `@src/bun.js/webcore/Blob.zig`:
- Around line 1144-1151: The resolve callback for S3 uploads uses the source
bytes store path when building the async-stack error (it calls
this.store.getPath()), causing failed Bun.write("s3://...", bytes) uploads to
show the source blob name; change the rejection path to use the destination
store/object key instead by retrieving the destination store or destination path
from the same context used for the upload (e.g., the destinationStore or
destinationPath field captured for the S3 operation) and call its
getPath()/equivalent when creating the JS error in resolve (function resolve and
S3.S3UploadResult failure branch), and apply the same change in the analogous
code at the other occurrence.
🪄 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: ec2f3d43-41ed-4b38-bed5-cccf2bb783f0
📥 Commits
Reviewing files that changed from the base of the PR and between 7d4c2b02c3a6874f28773bcc963c0b850eb7d98e and c5f49bc676d533bc9d2a208647bb5c54b6bdef9d.
📒 Files selected for processing (40)
src/Global.zigsrc/bun.js/api/Archive.zigsrc/bun.js/api/BunObject.zigsrc/bun.js/api/JSTranspiler.zigsrc/bun.js/api/bun/socket.zigsrc/bun.js/api/bun/subprocess.zigsrc/bun.js/api/cron.zigsrc/bun.js/api/crypto/PBKDF2.zigsrc/bun.js/api/crypto/PasswordObject.zigsrc/bun.js/api/glob.zigsrc/bun.js/bindings/AnyPromise.zigsrc/bun.js/bindings/ErrorStackTrace.cppsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/SystemError.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/node/node_fs.zigsrc/bun.js/node/node_process.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/webcore/Body.zigsrc/bun.js/webcore/S3File.zigsrc/bun.js/webcore/blob/Store.zigsrc/bun.js/webcore/blob/copy_file.zigsrc/bun.js/webcore/blob/read_file.zigsrc/bun.js/webcore/blob/write_file.zigsrc/bun.js/webcore/fetch/FetchTasklet.zigsrc/bun.js/webcore/streams.zigsrc/bun.zigsrc/codegen/bake-codegen.tssrc/codegen/bindgenv2/script.tssrc/codegen/cppbind.tssrc/deps/c_ares.zigsrc/js/builtins/ProcessObjectInternals.tssrc/js/node/http2.tssrc/s3/error.zigsrc/sql/mysql/MySQLConnection.zigsrc/sql/mysql/MySQLStatement.zigsrc/sql/mysql/protocol/ColumnDefinition41.zigsrc/sql/mysql/protocol/PreparedStatement.zigsrc/sys/Error.zig
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/js/node/http2.ts (3)
4288-4291:⚠️ Potential issue | 🔴 CriticalValidate response headers and status before serializing them.
setHeader(),appendHeader(), andwriteHead()currently store raw names/values/status text, and_flushHeaders()concatenates them verbatim into the TLS stream. Any\r/\nin user-controlled input becomes HTTP response splitting, and invalid status codes also slip through this path unlike the validated HTTP/2 response path at Lines 786-790.As per coding guidelines, "Validate function arguments using validators from
internal/validatorsand throw$ERR_*error codes for invalid arguments".Also applies to: 4324-4346, 4362-4380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/http2.ts` around lines 4288 - 4291, The response header and status APIs (setHeader, appendHeader, writeHead) are storing raw inputs allowing CR/LF and invalid status codes to be serialized in _flushHeaders; add input validation using validators from internal/validators and throw the appropriate $ERR_* errors: validate header name and value (no CR/LF, correct types) before calling this[kHttp1FallbackHeaders].set/append and validate status and statusMessage in writeHead (reject non-integer/invalid codes and CR/LF in statusMessage), mirroring the checks used by the HTTP/2 path (lines ~786-790); ensure _flushHeaders only serializes already-validated values.
4094-4111:⚠️ Potential issue | 🟠 MajorDon't re-arm keep-alive while the old body parser is still attached.
If the handler finishes early,
finishreattachesonDataeven thoughonBodyData/onChunkedDatamay still be listening. The next bytes are then consumed by both parsers, and the old listener can keep pushing into a request that was already ended at Lines 4162-4164. Track the active body listener and remove it, or close the socket, before re-arming keep-alive.Also applies to: 4138-4148, 4159-4181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/http2.ts` around lines 4094 - 4111, The finish handler currently re-attaches the main onData keep-alive parser while body listeners like onBodyData and onChunkedData may still be registered, causing duplicate consumption; modify the code in src/js/node/http2.ts so that finish (and any early-exit paths) checks and clears any active body data listener before re-arming keep-alive: track the active listener (e.g., a variable activeBodyListener assigned to the onBodyData/onChunkedData function), call socket.removeListener("data", activeBodyListener) (or set activeBodyListener = null) and only then reattach onData; ensure you update all affected blocks referenced (onBodyData, onChunkedData, and the finish flow where socket.on("data", onBodyData) / socket.removeListener are used) so no stale listener remains attached.
4392-4478:⚠️ Potential issue | 🟠 MajorCompute HTTP/1 framing/body eligibility in one helper.
flushHeaders()still bypasses the auto-framing path, sores.flushHeaders(); res.write(...)leaves a keep-alive response withoutContent-LengthorTransfer-Encoding. The same split also letswrite()pick chunked for HTTP/1.0, andend("...")on1xx/204/205/304still sends body bytes because_hasBodyonly tracksHEAD. Please move the decision into a shared helper used byflushHeaders(),write(), andend().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/http2.ts` around lines 4392 - 4478, Create a single helper (e.g. computeHttp1Framing or ensureHttp1Framing) and call it from flushHeaders(), write(), and end() to centralize framing decisions: it should sync this._chunked with getHeader("transfer-encoding"), respect kHttp1FallbackHeadersSent, detect HTTP/1.0 fallback to avoid chunked for 1.0, and set Content-Length when appropriate using hasHeader/setHeader (compute length when chunk is provided); also ensure the helper treats responses with no body (HEAD or statusCode 1xx, 204, 205, 304) as not having a body so end(...) and write(...) do not send body bytes. Use existing symbols (_chunked, _hasBody, kHttp1FallbackHeadersSent, getHeader, hasHeader, setHeader, statusCode, _flushHeaders) so the three call sites share identical logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 4064-4065: The fallback path creating Http1FallbackRequest and
Http1FallbackResponse skips the Expect/100-Continue handling; before emitting
"request" for the fallback, check the incoming headers for an "expect" header
and run the same logic as onServerStream (invoke checkExpectation/checkContinue
and/or emit "checkContinue" so a 100 Continue interim response can be sent) and
only proceed to construct/emit the request if expectation handling allows; apply
the same change to both fallback sites creating
Http1FallbackRequest/Http1FallbackResponse so HTTP/1.1 clients waiting for 100
Continue are handled consistently.
---
Duplicate comments:
In `@src/js/node/http2.ts`:
- Around line 4288-4291: The response header and status APIs (setHeader,
appendHeader, writeHead) are storing raw inputs allowing CR/LF and invalid
status codes to be serialized in _flushHeaders; add input validation using
validators from internal/validators and throw the appropriate $ERR_* errors:
validate header name and value (no CR/LF, correct types) before calling
this[kHttp1FallbackHeaders].set/append and validate status and statusMessage in
writeHead (reject non-integer/invalid codes and CR/LF in statusMessage),
mirroring the checks used by the HTTP/2 path (lines ~786-790); ensure
_flushHeaders only serializes already-validated values.
- Around line 4094-4111: The finish handler currently re-attaches the main
onData keep-alive parser while body listeners like onBodyData and onChunkedData
may still be registered, causing duplicate consumption; modify the code in
src/js/node/http2.ts so that finish (and any early-exit paths) checks and clears
any active body data listener before re-arming keep-alive: track the active
listener (e.g., a variable activeBodyListener assigned to the
onBodyData/onChunkedData function), call socket.removeListener("data",
activeBodyListener) (or set activeBodyListener = null) and only then reattach
onData; ensure you update all affected blocks referenced (onBodyData,
onChunkedData, and the finish flow where socket.on("data", onBodyData) /
socket.removeListener are used) so no stale listener remains attached.
- Around line 4392-4478: Create a single helper (e.g. computeHttp1Framing or
ensureHttp1Framing) and call it from flushHeaders(), write(), and end() to
centralize framing decisions: it should sync this._chunked with
getHeader("transfer-encoding"), respect kHttp1FallbackHeadersSent, detect
HTTP/1.0 fallback to avoid chunked for 1.0, and set Content-Length when
appropriate using hasHeader/setHeader (compute length when chunk is provided);
also ensure the helper treats responses with no body (HEAD or statusCode 1xx,
204, 205, 304) as not having a body so end(...) and write(...) do not send body
bytes. Use existing symbols (_chunked, _hasBody, kHttp1FallbackHeadersSent,
getHeader, hasHeader, setHeader, statusCode, _flushHeaders) so the three call
sites share identical logic.
🪄 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: 05f73722-a758-4891-838d-1e3288fe709e
📥 Commits
Reviewing files that changed from the base of the PR and between c5f49bc676d533bc9d2a208647bb5c54b6bdef9d and f7d6dc8eb6e5d9d3dd903b68f5042b92dc4bdf04.
📒 Files selected for processing (1)
src/js/node/http2.ts
robobun
force-pushed
the
farm/c243f436/fix-allowhttp1-fallback
branch
from
f7d6dc8 to
74504c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/js/node/http2.ts (3)
4288-4290:⚠️ Potential issue | 🔴 CriticalValidate header and status inputs before serializing the HTTP/1 response head.
setHeader(),appendHeader(), andwriteHead()accept raw names/values/status fields, and_flushHeaders()concatenates them verbatim into the socket write. Any embedded\r/\nbecomes response splitting here, andappendHeader()also skips the usualheadersSentguard. As per coding guidelines, "Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments".Also applies to: 4324-4360, 4362-4380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/http2.ts` around lines 4288 - 4290, Validate and sanitize inputs before storing or serializing HTTP/1 response head: in setHeader, appendHeader, and writeHead use the validators from internal/validators to check header name and value (reject non-strings, empty names, and names/values containing CR or LF) and validate status/code inputs, throwing the appropriate $ERR_* errors; ensure appendHeader honors the headers-sent guard (use kHttp1FallbackHeadersSent like setHeader does) and _flushHeaders only writes pre-validated headers from kHttp1FallbackHeaders to the socket so CR/LF injection cannot occur.
4094-4111:⚠️ Potential issue | 🟠 MajorDon't re-enable keep-alive while the previous body parser is still attached.
If user code finishes the response before the request body is fully read,
finishaddsonDataback butonBodyData/onChunkedDatastay registered. Subsequent bytes are then consumed by both parsers, and the request is markedcompleteeven though the body was cut short. Close the socket or explicitly detach the active body parser before re-arming keep-alive.Also applies to: 4138-4148, 4160-4180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/http2.ts` around lines 4094 - 4111, The keep-alive logic re-enables socket reuse while request body parsers (onBodyData / onChunkedData) may still be attached, causing future data to be consumed twice; modify the finish/response-completion path in the HTTP2 handling so it either closes the socket when a body parser is still active or explicitly removes/detaches any active body parser (removeListener for onBodyData and onChunkedData) before re-arming keep-alive; locate and update the handlers referenced as onBodyData, onChunkedData and the finish code path that re-adds onData to ensure you detach active parsers or close the socket (apply the same change to the other duplicate blocks around the other occurrences).
4396-4405:⚠️ Potential issue | 🟠 MajorCentralize response framing before headers are flushed.
write()unconditionally switches to chunked when no explicit length exists, even for HTTP/1.0 requests, whileflushHeaders()bypasses that logic entirely.res.flushHeaders(); res.write(...)can therefore produce an undelimited HTTP/1.1 response, and HTTP/1.0 keep-alive clients can be sent chunked bodies they cannot parse.Also applies to: 4476-4478
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/http2.ts` around lines 4396 - 4405, The chunked-framing decision is duplicated and inconsistent between write() and flushHeaders() causing undelimited responses (especially for HTTP/1.0); centralize the framing logic so headers are always framed before being flushed: move or consolidate the checks that currently live in write() (the block that checks this._hasBody, !this[kHttp1FallbackHeadersSent], !this.hasHeader("content-length"), !this.hasHeader("transfer-encoding") and sets this._chunked + setHeader("Transfer-Encoding", "chunked")) into flushHeaders() (and remove the ad-hoc chunked switch in write()), and ensure the logic respects the request HTTP version (reject setting chunked for HTTP/1.0 clients) and is also applied at the duplicate site referenced (around the 4476-4478 check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 4051-4055: The duplicate-header merge currently always appends
with ", " which produces "a=1, b=2" for repeated Cookie headers; update the
logic in the block that checks headers[lowerKey] (the code that mutates
headers[lowerKey], using the lowerKey variable) to detect when lowerKey ===
"cookie" and use the delimiter "; " for cookies (matching toHeaderObject()
behavior) while keeping ", " for other headers so repeated Cookie fields are
joined as "a=1; b=2".
- Around line 4076-4077: The code currently accepts content-length values like
"5, 7" because parseInt silences commas and also prefers Content-Length over
Transfer-Encoding; change the pre-parser validation so you only accept a
Content-Length when headers["content-length"] is a single non-negative integer
string (match /^\d+$/) and no Transfer-Encoding is present that includes
"chunked"; if Content-Length contains commas/multiple values or
Transfer-Encoding includes "chunked" (or both headers are present), treat the
request as malformed and reject it (400) or choose the chunked parser per RFC
instead of using parseInt; apply the same stricter validation at the other
selection site referenced (the branch around the contentLength check and the
similar check at the other location).
---
Duplicate comments:
In `@src/js/node/http2.ts`:
- Around line 4288-4290: Validate and sanitize inputs before storing or
serializing HTTP/1 response head: in setHeader, appendHeader, and writeHead use
the validators from internal/validators to check header name and value (reject
non-strings, empty names, and names/values containing CR or LF) and validate
status/code inputs, throwing the appropriate $ERR_* errors; ensure appendHeader
honors the headers-sent guard (use kHttp1FallbackHeadersSent like setHeader
does) and _flushHeaders only writes pre-validated headers from
kHttp1FallbackHeaders to the socket so CR/LF injection cannot occur.
- Around line 4094-4111: The keep-alive logic re-enables socket reuse while
request body parsers (onBodyData / onChunkedData) may still be attached, causing
future data to be consumed twice; modify the finish/response-completion path in
the HTTP2 handling so it either closes the socket when a body parser is still
active or explicitly removes/detaches any active body parser (removeListener for
onBodyData and onChunkedData) before re-arming keep-alive; locate and update the
handlers referenced as onBodyData, onChunkedData and the finish code path that
re-adds onData to ensure you detach active parsers or close the socket (apply
the same change to the other duplicate blocks around the other occurrences).
- Around line 4396-4405: The chunked-framing decision is duplicated and
inconsistent between write() and flushHeaders() causing undelimited responses
(especially for HTTP/1.0); centralize the framing logic so headers are always
framed before being flushed: move or consolidate the checks that currently live
in write() (the block that checks this._hasBody,
!this[kHttp1FallbackHeadersSent], !this.hasHeader("content-length"),
!this.hasHeader("transfer-encoding") and sets this._chunked +
setHeader("Transfer-Encoding", "chunked")) into flushHeaders() (and remove the
ad-hoc chunked switch in write()), and ensure the logic respects the request
HTTP version (reject setting chunked for HTTP/1.0 clients) and is also applied
at the duplicate site referenced (around the 4476-4478 check).
🪄 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: b7c69e0d-250f-4cf0-a9ee-b05ffb584114
📥 Commits
Reviewing files that changed from the base of the PR and between f7d6dc8eb6e5d9d3dd903b68f5042b92dc4bdf04 and 8ce4087.
📒 Files selected for processing (2)
src/js/node/http2.tstest/regression/issue/28656.test.ts
The HTTP/1.1 fallback for Http2SecureServer drives its response handle
with the flat [name, value, name, value, ...] array produced by
renderNativeHeaders(), but writeHeadToSocket() iterated it as if each
element were a [name, value] pair. Destructuring each string as
{ 0: name, 1: value } read the first two characters of every header, so
"content-type: application/json" was written as "c: o" and the real
headers were lost; the NUL-named sentinel pairs that encode the framing
decision (no body / close-delimited) were also emitted as bogus headers.
Walk the array two entries at a time, compare header names case
insensitively, and honor the framing sentinels instead of writing them
out, so HTTP/1.1 clients receive the response headers the handler set.
robobun
changed the title
robobun
force-pushed
the
farm/c243f436/fix-allowhttp1-fallback
branch
from
8ce4087 to
b3f4fcc
Compare
After the flat-array fix, writeHeadToSocket now correctly detects the Date
and Connection headers that renderNativeHeaders() already emits, so the
fallback's own !hasDate / !hasConnection synthesis only fired when the user
deliberately suppressed them (res.sendDate = false or
res.removeHeader("connection")), re-adding a header they asked to drop.
Synthesize those defaults only when no rendered header array is present (the
head-unset fallback), matching the native NodeHTTP.cpp consumer, and drop the
now-unused hasDate/hasConnection tracking.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/http2.ts:5459-5461— Pre-existing, but since this PR adds thecase "connection":branch and thesocket.end()inhandle.end(): a response-sideConnection: close(fromres.setHeader('Connection', 'close')orres.shouldKeepAlive = false) is correctly written to the wire here, but nothing on this fallback path then closes the socket —closeDelimitedonly covers the_removedTEsentinel, andhandle.onfinished(line 5550) only checks the request-sideshouldKeepAlive. The native path closes viares[kMustCloseConnection]→endSocketOnFinishIfNeeded; this path has no equivalent. Low impact (well-behaved clients close per RFC 7230 §6.6), but a ~3-line addition here — set amustCloseflag when theconnectionvalue matches/\bclose\b/iand OR it into the line-5459 condition — would close the gap. Same root concern previously raised as comment 3008419094 on the earlier implementation.Extended reasoning...
What the gap is
renderNativeHeaders()in_http_server.tssetsres[kMustCloseConnection] = trueand pushesConnection: closeinto the flat header array — without the NUL sentinel — in several cases that are distinct from the close-delimited body-framing case this PR handles:- Line 1519: the user calls
res.setHeader('Connection', 'close') - Line 1549: defective 204/304 with a Transfer-Encoding header
- Line 1578:
res._removedConnection && res.shouldKeepAlive === false - Line 1602:
res.shouldKeepAlive = false(graceful-shutdown helpers)
The NUL sentinel
"\u0000", "1"that this PR detects is pushed only in theres._removedTEbranch (lines 1565–1567). In every otherkMustCloseConnectioncase,Connection: closeappears in the flat array as an ordinary header.After this PR's loop fix,
writeHeadToSocket()correctly emits thatConnection: closeon the wire (thecase "connection":branch at line 5377 setshasConnection = trueand the header is written verbatim at line 5380). ButcloseDelimitedstaysfalse, so the newsocket.end()at line 5459 does not fire. Andhandle.onfinishedat line 5550 only checks the request-sideshouldKeepAlivecaptured fromHTTPParser— which istruefor any HTTP/1.1 client that didn't sendConnection: closeitself. Result: the server tells the clientConnection: closebut keeps the socket open and the parser armed.On the native NodeHTTP path,
res.once('finish', endSocketOnFinishIfNeeded)is registered at_http_server.ts:646and readsres[kMustCloseConnection](line 1649).connectionListenerHTTP1inhttp2.tsregisters no equivalent listener, andkMustCloseConnectionis a private symbol not exported to this file — so the flag is set but never read on this path.Addressing the "different problem class" objection
One reviewer correctly observes that the new
closeDelimitedsocket.end()at line 5459 solves body framing (no Content-Length, no chunked → EOF is the only delimiter; withoutsocket.end()the client hangs forever waiting for body end), whereaskMustCloseConnectionis connection-lifecycle hygiene (the body is properly framed via CL/chunked; closing is about reuse semantics, not framing). That distinction is correct, and I'm not characterizing this PR'scloseDelimitedhandling as incomplete — it does exactly what it needs to for the framing case.The point here is narrower: this PR adds the
case "connection":switch branch at line 5376 and thesocket.end()call at line 5459, so it is touching exactly the two places where a 3-line addition would also cover the response-sideConnection: closelifecycle. That makes it cheap to fold in, even though the underlying gap (inonfinishedat line 5550) pre-dates this PR.Step-by-step proof
- HTTP/1.1 keep-alive client connects to
http2.createSecureServer({ allowHTTP1: true }); ALPN negotiateshttp/1.1;connectionListenerHTTP1runs. - Parser's
kOnHeadersCompletefires withshouldKeepAlive = true(HTTP/1.1 default).createHttp1FallbackResponseHandle(socket, true, …)is called. - Handler does
res.setHeader('Connection', 'close'); res.end('bye');. renderNativeHeaders(res)at_http_server.ts:1518–1519:RE_CONN_CLOSEmatches →res[kMustCloseConnection] = true; pushes'Connection', 'close'into the flat array. No NUL sentinel.writeHeadToSocket(): loop hitsname = 'Connection',value = 'close'→case "connection": hasConnection = true(line 5377) → emitsConnection: close\r\n.closeDelimitedstaysfalse.handle.end(): line 5459if (closeDelimited && …)→false→ nosocket.end().onfinished(line 5548–5553):shouldKeepAliveistrue(request side) → nosocket.end().- Client receives
Connection: close, reads the (CL-framed) body, and per RFC 7230 §6.6 closes its side. Server'ssocket.once('close')at line 5583 then cleans up.
Impact
Low. The body is correctly framed, so this is not a hang or framing violation. Per RFC 7230 §6.6 a client receiving
Connection: closewill close after reading the response, which triggers the server's existing'close'handler. The divergence is that RFC 7230 §6.6 also says a server that sendsConnection: closeMUST initiate a close of the connection after sending the response, and Node.js does so on its native path. A misbehaving client that ignoresConnection: closeand sends another request would get it served here (Node would have closed first). This is the same root concern previously raised as inline comment 3008419094 on the earlier hand-rolled fallback, which was not carried over when the implementation was replaced with theHTTPParser-based one.Suggested fix
In the
case "connection":branch at line 5376, also test the value:case "connection": hasConnection = true; if (/\bclose\b/i.test(String(value))) closeDelimited = true; // or a separate mustClose flag break;
Reusing
closeDelimitedis safe here because every place it's read (lines 5383, 5395, 5459) does the right thing for an explicitConnection: closetoo: skip auto-framing only if no CL/TE is set (it will be), emitConnection: close(already in the array), andsocket.end()after the body. A separatemustCloseflag OR'd into line 5459 is the more conservative option if you'd rather keep the framing and lifecycle concerns visibly separate. - Line 1519: the user calls
|
Thanks for the detailed trace. This is a real gap, but I'm keeping it out of this PR. It's pre-existing: What this PR changes is that a response-side Making the server proactively close is a connection-lifecycle change (it has to coordinate with the keep-alive/teardown path, not just header serialization) and it warrants a deterministic server-initiated-close test rather than a timeout-based one. I'd rather do that as a focused follow-up than widen this header-correctness fix. Noting it here so it isn't lost. |
renderNativeHeaders() only appends the Keep-Alive: timeout header when res._keepAliveTimeout is set. The native node:http path sets it right after constructing the response (_http_server.ts), but connectionListenerHTTP1 never did, so the allowHTTP1 fallback advertised Connection: keep-alive without the accompanying Keep-Alive header. Set res._keepAliveTimeout from the server's keepAliveTimeout to match, so the already-computed value reaches the response.
|
The diff is green. The remaining CI failures are unrelated flake on lanes this PR doesn't touch (the change is confined to the
The regression test |
1 participant
Problem
With
http2.createSecureServer({ allowHTTP1: true }), HTTP/1.1 clients are served since #31584, but every response header set viawriteHead/setHeaderis mangled on the wire. A handler that responds withcontent-type: application/jsonproduces:so
response.headers['content-type']isundefinedon the client, and the defaultDate/Connectionheaders end up duplicated.Cause
The HTTP/1.1 fallback (
createHttp1FallbackResponseHandleinsrc/js/node/http2.ts) drives its response handle with the flat[name, value, name, value, ...]array thatrenderNativeHeaders()builds (seesrc/js/node/_http_server.ts).writeHeadToSocket()iterated it withfor (const { 0: name, 1: value } of headers), treating each element as a[name, value]pair. But each element is a single string, so{ 0: name, 1: value }reads its first two characters:"content-type"becomes name"c", value"o". The loop also emitted the NUL-named sentinel pairs (which encode the body-framing decision) as literal header lines, and re-addedDate/Connectionbecause it never recognized the ones already present.Fix
Walk the flat array two entries at a time (matching the native consumer in
src/jsc/bindings/NodeHTTP.cpp), compare header names case-insensitively, and honor the\u0000-named framing sentinels ("2"= no body,"1"= close-delimited) instead of writing them out.Verification
Without the fix the first test fails with
content-type: undefined. The raw HTTP/1.1 response is now well-formed:The fix also makes the Node.js conformance test
test/js/node/test/parallel/test-http2-https-fallback.jspass locally.Closes #28656