fix(ext/node): address node:http rewrite review comments#33299
Conversation
- Restore emitBindingWarning() in internal/test/binding.ts - Add comment explaining root cert store fallback in tls_wrap.rs - Validate statusMessage for invalid chars in writeHead() (CRLF injection) - Remove setUnrefTimeout from node:timers public exports - Add comment explaining sync pipe writes - Enable test-http-status-reason-invalid-chars compat test Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add ERR_HTTP_BODY_NOT_ALLOWED error and throw it in OutgoingMessage write path when _hasBody is false and rejectNonStandardBodyWrites is enabled. Enables test-http-head-throw-on-response-body-write compat test. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Store the async resource passed to parser.initialize() and wrap execute() with AsyncResource.runInAsyncScope(), emulating Node's MakeCallback behavior. This ensures AsyncLocalStorage context is preserved through native HTTP parser callbacks. Enables test-async-local-storage-http-multiclients compat test. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When shutdown() is called before the TLS handshake completes (e.g.
conn.end('') before the connect callback), defer send_close_notify()
and underlying stream shutdown until after the handshake finishes.
Previously rustls would send malformed data mid-handshake, causing
the peer to emit a fatal DecodeError alert.
Enables test-tls-zero-clear-in compat test.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The restored emitBindingWarning() emits a process warning when internal test bindings are used. Update .out files to expect this warning line. Also fix clippy lint (map_or -> is_some_and). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The internalBinding warning fires during the call, before the pipe close callback, so it appears before the PASS line. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…comments # Conflicts: # ext/node/polyfills/internal_binding/pipe_wrap.ts
llhttp_finish() can trigger callbacks (e.g. on_message_complete) but finish() was called without setting up the ExecuteContext on parser.data. This caused a null pointer dereference when the HTTP parser received EOF on an upgrade-rejected connection. Pass the callbacks object and scope to finish(), matching execute(). Fixes #28654 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Regression test for #28654: socket.write() + socket.end() in an upgrade handler must not crash the HTTP parser. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Most of this looks like good review-followup work, but I think the TLS shutdown fix changes behavior in a risky way. Node’s That avoids the rustls decode error for I’d want either a stronger proof that rustls will always drive |
|
Good catch on the stalled-handshake concern. I tested this:
const server = net.createServer((socket) => { /* never send TLS data */ });
const socket = tls.connect({ port, rejectUnauthorized: false });
setTimeout(() => socket.end(), 100);
// Both Deno and Node: socket never closes
setTimeout(() => socket.destroy(), 100);
// Both Deno and Node: socket closes immediatelySo the behavior matches Node.js. |
2 participants
Summary
emitBindingWarning()ininternal/test/binding.ts, add comment explaining root cert store fallback intls_wrap.rs, removesetUnrefTimeoutfromnode:timerspublic exports, add comment explaining sync pipe writeswriteHeadstatusMessage validation: validatestatusMessagefor invalid characters usingcheckInvalidHeaderChar, throwingERR_INVALID_CHARto prevent CRLF injectionrejectNonStandardBodyWrites: addERR_HTTP_BODY_NOT_ALLOWEDerror and throw inOutgoingMessagewrite path when_hasBodyis false andrejectNonStandardBodyWritesis enabledparser.initialize()and wrapexecute()withAsyncResource.runInAsyncScope(), emulating Node'sMakeCallbackbehavior to preserveAsyncLocalStoragecontext through native HTTP parser callbackssend_close_notify()and underlying stream shutdown when TLS handshake is still in progress, executing them after handshake completes. Fixes rustlsDecodeErrorwhenconn.end('')is called before the connect callbackfinish():llhttp_finish()can trigger callbacks (e.g.on_message_complete) butfinish()was called without setting up theExecuteContext, causing a null pointer dereference when the client-side HTTP parser processed a rejected-upgrade response. Now passes the callbacks object and scope tofinish(), matchingexecute().Closes #28654
Test plan
test-http-status-reason-invalid-chars— now passingtest-http-head-throw-on-response-body-write— now passingtest-async-local-storage-http-multiclients— now passingtest-tls-zero-clear-in— now passing[node/http] upgrade rejection via socket.write + socket.end does not crash— new test🤖 Generated with Claude Code