From 8d8a35825caca6b51c2651a256413cbcc52e0118 Mon Sep 17 00:00:00 2001 From: Avocado Date: Mon, 24 Aug 2026 16:24:26 +0900 Subject: [PATCH 1/2] 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 0d0507c93f47..6d983b69c7d4 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -93,6 +93,7 @@ const kEndCallbacks = Symbol('kEndCallbacks'); const kFlushError = Symbol('kFlushError'); const kHighWaterMark = Symbol('kHighWaterMark'); const kRejectNonStandardBodyWrites = Symbol('kRejectNonStandardBodyWrites'); +const kPayloadPending = Symbol('kPayloadPending'); const nop = () => {}; @@ -142,6 +143,7 @@ function OutgoingMessage(options) { this.strictContentLength = false; this[kBytesWritten] = 0; + this[kPayloadPending] = false; this._contentLength = null; this._hasBody = true; this._trailer = ''; @@ -542,7 +544,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 && @@ -992,6 +999,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 c7cdfa0f74c2c8e392898326798edf736230f57b Mon Sep 17 00:00:00 2001 From: Avocado Date: Tue, 25 Aug 2026 09:27:48 +0900 Subject: [PATCH 2/2] 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 8ee197b44e0b..ef0d5492c579 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'); })); }));