From 124e439ad6b7df47331560d8277118a8cc30e270 Mon Sep 17 00:00:00 2001 From: Avocado Date: Mon, 24 Aug 2026 16:24:26 +0900 Subject: [PATCH 1/3] http: frame client requests that carry a payload A client request whose method defaults to no chunked encoding (GET, HEAD, DELETE, OPTIONS, TRACE) skipped both Content-Length and Transfer-Encoding even when a payload was written. The payload was still sent, so the peer read those bytes as the start of the next request, which allows request smuggling. Frame such requests like any other: use Content-Length when the length is known and a chunked Transfer-Encoding when it is not. Server responses and requests with no payload keep the previous behaviour, and CONNECT is left untouched because its payload is tunnelled data that must not be re-framed. Headers can be rendered before anything is written, for example when the headers are passed as an array or an Expect header is set. Track whether a payload write actually happened so that case is not mistaken for a body of unknown length. The two known_issues tests covering this now pass and move to parallel. They also asserted a response body for HEAD, which cannot have one, and compared buffers with strictEqual, so both are corrected in the move. Fixes: https://github.com/nodejs/node/issues/27880 Refs: https://github.com/nodejs/node/pull/34066 Refs: https://github.com/nodejs/node/pull/60718 Signed-off-by: Avocado --- lib/_http_outgoing.js | 10 +++- ...st-http-clientrequest-end-contentlength.js | 51 ------------------ .../test-http-clientrequest-write-chunked.js | 52 ------------------- ...st-http-clientrequest-end-contentlength.js | 42 +++++++++++++++ .../test-http-clientrequest-write-chunked.js | 42 +++++++++++++++ ...test-http-request-method-delete-payload.js | 27 ++++++---- 6 files changed, 110 insertions(+), 114 deletions(-) delete mode 100644 test/known_issues/test-http-clientrequest-end-contentlength.js delete mode 100644 test/known_issues/test-http-clientrequest-write-chunked.js create mode 100644 test/parallel/test-http-clientrequest-end-contentlength.js create mode 100644 test/parallel/test-http-clientrequest-write-chunked.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index ebbd24acdcb2..06a280a57b9b 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -94,6 +94,7 @@ const kEndCallbacks = Symbol('kEndCallbacks'); const kFlushError = Symbol('kFlushError'); const kHighWaterMark = Symbol('kHighWaterMark'); const kRejectNonStandardBodyWrites = Symbol('kRejectNonStandardBodyWrites'); +const kPayloadPending = Symbol('kPayloadPending'); const nop = () => {}; @@ -143,6 +144,7 @@ function OutgoingMessage(options) { this.strictContentLength = false; this[kBytesWritten] = 0; + this[kPayloadPending] = false; this._contentLength = null; this._hasBody = true; this._trailer = ''; @@ -560,7 +562,12 @@ function _storeHeader(firstLine, headers) { if (!this._hasBody) { // Make sure we don't end the 0\r\n\r\n at the end of the message. this.chunkedEncoding = false; - } else if (!this.useChunkedEncodingByDefault) { + } else if (!this.useChunkedEncodingByDefault && + (!this.method || this.method === 'CONNECT' || + !this[kPayloadPending])) { + // Responses, and requests that are known to carry no payload, are framed + // by the absence of both headers. CONNECT is excluded because its payload + // is tunnelled data that must not be re-framed. this._last = true; } else if (!state.trailer && !this._removedContLen && @@ -1010,6 +1017,7 @@ function write_(msg, chunk, encoding, callback, fromEnd) { len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; msg._contentLength = len; } + msg[kPayloadPending] = true; msg._implicitHeader(); } diff --git a/test/known_issues/test-http-clientrequest-end-contentlength.js b/test/known_issues/test-http-clientrequest-end-contentlength.js deleted file mode 100644 index 988dd08bd26e..000000000000 --- a/test/known_issues/test-http-clientrequest-end-contentlength.js +++ /dev/null @@ -1,51 +0,0 @@ -'use strict'; - -// Refs: https://github.com/nodejs/node/pull/34066 - -const common = require('../common'); -const assert = require('assert'); -const http = require('http'); - -// Test that ClientRequest#end with default options -// computes and sends a Content-Length header -const upload = 'PUT / HTTP/1.1\r\n\r\n'; -const response = 'content-length: 19\r\n'; - -// Test that the upload is properly received with the same headers, -// regardless of request method -const methods = [ 'GET', 'HEAD', 'DELETE', 'POST', 'PATCH', 'PUT', 'OPTIONS' ]; - -const server = http.createServer(common.mustCall(function(req, res) { - req.on('data', common.mustCall((chunk) => { - assert.strictEqual(chunk, Buffer.from(upload)); - })); - res.setHeader('Content-Type', 'text/plain'); - let payload = `${req.method}\r\n`; - for (let i = 0; i < req.rawHeaders.length; i += 2) { - // Ignore a couple headers that may vary - if (req.rawHeaders[i].toLowerCase() === 'host') continue; - if (req.rawHeaders[i].toLowerCase() === 'connection') continue; - payload += `${req.rawHeaders[i]}: ${req.rawHeaders[i + 1]}\r\n`; - } - res.end(payload); -}), methods.length); - -server.listen(0, common.mustCall(function tryNextRequest() { - const method = methods.pop(); - if (method === undefined) return; - const port = server.address().port; - const req = http.request({ method, port }, common.mustCall((res) => { - const chunks = []; - res.on('data', function(chunk) { - chunks.push(chunk); - }); - res.on('end', common.mustCall(() => { - const received = Buffer.concat(chunks).toString(); - const expected = method.toLowerCase() + '\r\n' + response; - assert.strictEqual(received.toLowerCase(), expected); - tryNextRequest(); - })); - })); - - req.end(upload); -})).unref(); diff --git a/test/known_issues/test-http-clientrequest-write-chunked.js b/test/known_issues/test-http-clientrequest-write-chunked.js deleted file mode 100644 index 77d9f503fbd0..000000000000 --- a/test/known_issues/test-http-clientrequest-write-chunked.js +++ /dev/null @@ -1,52 +0,0 @@ -'use strict'; - -// Refs: https://github.com/nodejs/node/pull/34066 - -const common = require('../common'); -const assert = require('assert'); -const http = require('http'); - -// Test that ClientRequest#write with default options -// uses a chunked Transfer-Encoding -const upload = 'PUT / HTTP/1.1\r\n\r\n'; -const response = 'transfer-encoding: chunked\r\n'; - -// Test that the upload is properly received with the same headers, -// regardless of request method. -const methods = [ 'GET', 'HEAD', 'DELETE', 'POST', 'PATCH', 'PUT', 'OPTIONS' ]; - -const server = http.createServer(common.mustCall(function(req, res) { - req.on('data', common.mustCall((chunk) => { - assert.strictEqual(chunk.toString(), upload); - })); - res.setHeader('Content-Type', 'text/plain'); - res.write(`${req.method}\r\n`); - for (let i = 0; i < req.rawHeaders.length; i += 2) { - // Ignore a couple headers that may vary - if (req.rawHeaders[i].toLowerCase() === 'host') continue; - if (req.rawHeaders[i].toLowerCase() === 'connection') continue; - res.write(`${req.rawHeaders[i]}: ${req.rawHeaders[i + 1]}\r\n`); - } - res.end(); -}), methods.length); - -server.listen(0, common.mustCall(function tryNextRequest() { - const method = methods.pop(); - if (method === undefined) return; - const port = server.address().port; - const req = http.request({ method, port }, common.mustCall((res) => { - const chunks = []; - res.on('data', function(chunk) { - chunks.push(chunk); - }); - res.on('end', common.mustCall(() => { - const received = Buffer.concat(chunks).toString(); - const expected = method.toLowerCase() + '\r\n' + response; - assert.strictEqual(received.toLowerCase(), expected); - tryNextRequest(); - })); - })); - - req.write(upload); - req.end(); -})).unref(); diff --git a/test/parallel/test-http-clientrequest-end-contentlength.js b/test/parallel/test-http-clientrequest-end-contentlength.js new file mode 100644 index 000000000000..6c5f490eeed5 --- /dev/null +++ b/test/parallel/test-http-clientrequest-end-contentlength.js @@ -0,0 +1,42 @@ +'use strict'; + +// Test that ClientRequest#end computes and sends a Content-Length header, +// regardless of the request method. +// Refs: https://github.com/nodejs/node/issues/27880 + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const upload = 'PUT / HTTP/1.1\r\n\r\n'; + +const methods = ['GET', 'HEAD', 'DELETE', 'POST', 'PATCH', 'PUT', 'OPTIONS']; + +const server = http.createServer(common.mustCall(function(req, res) { + const chunks = []; + req.on('data', (chunk) => chunks.push(chunk)); + req.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), Buffer.from(upload)); + assert.strictEqual(req.headers['content-length'], + String(upload.length)); + assert.strictEqual(req.headers['transfer-encoding'], undefined); + + // A HEAD response carries no body, so report the outcome in a header + // instead of writing it to the response stream. + res.setHeader('x-received-method', req.method); + res.end(); + })); +}, methods.length)); + +server.listen(0, common.mustCall(function tryNextRequest() { + const method = methods.pop(); + if (method === undefined) return; + const port = server.address().port; + const req = http.request({ method, port }, common.mustCall((res) => { + assert.strictEqual(res.headers['x-received-method'], method); + res.resume(); + res.on('end', common.mustCall(tryNextRequest)); + })); + + req.end(upload); +})).unref(); diff --git a/test/parallel/test-http-clientrequest-write-chunked.js b/test/parallel/test-http-clientrequest-write-chunked.js new file mode 100644 index 000000000000..b251812bf18d --- /dev/null +++ b/test/parallel/test-http-clientrequest-write-chunked.js @@ -0,0 +1,42 @@ +'use strict'; + +// Test that ClientRequest#write uses a chunked Transfer-Encoding when the +// payload length is not known in advance, regardless of the request method. +// Refs: https://github.com/nodejs/node/issues/27880 + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const upload = 'PUT / HTTP/1.1\r\n\r\n'; + +const methods = ['GET', 'HEAD', 'DELETE', 'POST', 'PATCH', 'PUT', 'OPTIONS']; + +const server = http.createServer(common.mustCall(function(req, res) { + const chunks = []; + req.on('data', (chunk) => chunks.push(chunk)); + req.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), Buffer.from(upload)); + assert.strictEqual(req.headers['transfer-encoding'], 'chunked'); + assert.strictEqual(req.headers['content-length'], undefined); + + // A HEAD response carries no body, so report the outcome in a header + // instead of writing it to the response stream. + res.setHeader('x-received-method', req.method); + res.end(); + })); +}, methods.length)); + +server.listen(0, common.mustCall(function tryNextRequest() { + const method = methods.pop(); + if (method === undefined) return; + const port = server.address().port; + const req = http.request({ method, port }, common.mustCall((res) => { + assert.strictEqual(res.headers['x-received-method'], method); + res.resume(); + res.on('end', common.mustCall(tryNextRequest)); + })); + + req.write(upload); + req.end(); +})).unref(); diff --git a/test/parallel/test-http-request-method-delete-payload.js b/test/parallel/test-http-request-method-delete-payload.js index ffefd7d470d8..dca16cfe6095 100644 --- a/test/parallel/test-http-request-method-delete-payload.js +++ b/test/parallel/test-http-request-method-delete-payload.js @@ -1,26 +1,33 @@ 'use strict'; const common = require('../common'); +const assert = require('assert'); const http = require('http'); +// A DELETE request with a payload must be framed like any other request, so +// that the payload is received as a body instead of being parsed as the start +// of a following request. +// Refs: https://github.com/nodejs/node/issues/27880 + const data = 'PUT / HTTP/1.1\r\n\r\n'; const server = http.createServer(common.mustCall(function(req, res) { - req.on('data', common.mustNotCall()); - res.setHeader('Content-Type', 'text/plain'); - for (let i = 0; i < req.rawHeaders.length; i += 2) { - if (req.rawHeaders[i].toLowerCase() === 'host') continue; - if (req.rawHeaders[i].toLowerCase() === 'connection') continue; - res.write(`${req.rawHeaders[i]}: ${req.rawHeaders[i + 1]}\r\n`); - } - res.end(); + const chunks = []; + req.on('data', (chunk) => chunks.push(chunk)); + req.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), Buffer.from(data)); + assert.strictEqual(req.headers['transfer-encoding'], 'chunked'); + + res.setHeader('Content-Type', 'text/plain'); + res.end(); + })); })).unref(); server.listen(0, common.mustCall(() => { const port = server.address().port; - const req = http.request({ method: 'DELETE', port }, function(res) { + const req = http.request({ method: 'DELETE', port }, common.mustCall((res) => { res.resume(); - }); + })); req.write(data); req.end(); From b0fa393ca9299062b40e7656ebb8514aa8f624a4 Mon Sep 17 00:00:00 2001 From: Avocado Date: Tue, 25 Aug 2026 09:27:48 +0900 Subject: [PATCH 2/3] test: update pipeline expectation for framed client requests A client request that writes a payload is now framed with a chunked Transfer-Encoding. When the body source is destroyed before EOF the request ends without its terminating chunk, so the peer resets the connection instead of seeing a clean close, and the echoed response fails with ECONNRESET rather than ERR_STREAM_PREMATURE_CLOSE. Signed-off-by: Avocado --- test/parallel/test-stream-pipeline.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-stream-pipeline.js b/test/parallel/test-stream-pipeline.js index 65fb30d95c80..f68d097fc52d 100644 --- a/test/parallel/test-stream-pipeline.js +++ b/test/parallel/test-stream-pipeline.js @@ -271,9 +271,10 @@ tmpdir.refresh(); { const server = http.createServer(common.mustCallAtLeast((req, res) => { pipeline(req, res, common.mustCall((err) => { - // The client destroys the request body source before EOF below, so the - // echoed response cannot finish successfully either. - assert.strictEqual(err?.code, 'ERR_STREAM_PREMATURE_CLOSE'); + // The client destroys the request body source before EOF below. The + // request is framed as chunked, so it ends without its terminating + // chunk and the connection is reset rather than closed cleanly. + assert.strictEqual(err?.code, 'ECONNRESET'); })); })); From 3cca48dfa2d293d5239819f9ca19cfcaf75573db Mon Sep 17 00:00:00 2001 From: Avocado Date: Thu, 3 Sep 2026 16:51:00 +0900 Subject: [PATCH 3/3] http: track payloads framed by the fast end() path A client request that carries a payload is framed with Content-Length or a chunked Transfer-Encoding, which is decided from the payload flag set when a write reaches the message. The fast path for a known-length end() renders the headers itself without going through that write, so the flag was still unset and such requests were framed as if they had no payload. Set the flag there as well, so a request whose whole payload is passed to end() is framed like any other. The pipeline test aborts a request body once the echoed response has been received. Chunked writes are now coalesced, so the response arrives as a single data event and counting events never reaches the abort. Count the received bytes instead, the way the neighbouring case already does. Signed-off-by: Avocado --- lib/_http_outgoing.js | 1 + test/parallel/test-stream-pipeline.js | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 06a280a57b9b..356ffc7c42c2 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -1082,6 +1082,7 @@ function maybePrepareFinalChunk(msg, chunk, encoding) { if (!msg._header) { msg._contentLength = typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; + msg[kPayloadPending] = true; msg._implicitHeader(); } diff --git a/test/parallel/test-stream-pipeline.js b/test/parallel/test-stream-pipeline.js index f68d097fc52d..61c06e262bde 100644 --- a/test/parallel/test-stream-pipeline.js +++ b/test/parallel/test-stream-pipeline.js @@ -298,10 +298,10 @@ tmpdir.refresh(); })); req.on('response', (res) => { - let cnt = 10; - res.on('data', () => { - cnt--; - if (cnt === 0) rs.destroy(); + let received = 0; + res.on('data', (data) => { + received += data.length; + if (received >= 50) rs.destroy(); }); }); }));