fix(batch): add missing early return in nodeHttpBatchRpcResponse on non-POST#195
fix(batch): add missing early return in nodeHttpBatchRpcResponse on non-POST#195aleister1102 wants to merge 2 commits into
Conversation
…on-POST Without response.end() and return, a non-POST request would write the 405 status line then fall through into the full RPC pipeline. This caused the subsequent writeHead(200) to throw ERR_HTTP_HEADERS_SENT, left the TCP connection open (response.end never called), and — when the caller uses the documented try/catch pattern — caused a second writeHead(500) to throw uncaught, terminating the Node.js process. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi @teamchong @kentonv, could you take a look when you get a chance? This is the fix for the missing early return discussed in the HackerOne report. The change is minimal - two lines in |
commit: |
|
verified the fix locally. on main a GET to this endpoint never gets a response, the socket just stays open until timeout, and there's an unhandled ERR_HTTP_HEADERS_SENT rejection later when the pipeline runs anyway. with your branch it returns the 405 immediately. So the change is correct. two things to add before this can merge (snippets below are just a starting point, adjust as you see fit): a changeset (the release automation needs it):
---
"capnweb": patch
---
Fix nodeHttpBatchRpcResponse leaving the connection open and crashing with
ERR_HTTP_HEADERS_SENT on non-POST requests. It now returns 405 immediately.and a regression test, since this path had no coverage. In it("rejects non-POST requests with 405", async () => {
let response = await fetch(`http://${inject("testServerHost")}`, { method: "GET" });
expect(response.status).toBe(405);
await response.text();
});this hangs on main and passes with your fix, so it actually catches the regression. Thanks! |
🦋 Changeset detectedLatest commit: f684df8 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Add changeset for nodeHttpBatchRpcResponse early-return fix.
aleister1102
force-pushed
the
fix/node-batch-405-early-return
branch
from
4883dd7 to
f684df8
Compare
|
Done - thanks for verifying the fix locally, @teamchong. Added both items: Confirmed locally that the new test passes on this branch. Should be ready for another look. |
2 participants
Summary
nodeHttpBatchRpcResponsewrote a 405 status for non-POST requests viaresponse.writeHead(405, ...)but was missingresponse.end()andreturn. Execution fell through into the full RPC pipeline unconditionally, then crashed withERR_HTTP_HEADERS_SENTwhen the function later tried to write the 200 response — leaving the TCP connection open until timeout.The Fetch API counterpart (
newHttpBatchRpcResponse) was unaffected; it already returned a 405Responseimmediately.Changes
src/batch.ts: addresponse.end()andreturnafter the 405writeHeadcall innodeHttpBatchRpcResponseTest plan
nodeHttpBatchRpcResponseendpoint now receives a 405 with a closed connection and no RPC handlers are invoked