From 670f31f5c8a67e1ac3fd8251b84cff6e552568c5 Mon Sep 17 00:00:00 2001 From: Paul Xue Date: Tue, 3 Oct 2023 15:10:13 -0400 Subject: [PATCH 1/7] fix: bypass DELETE method content-length mismatch when request.contentLength=0 --- lib/client.js | 7 ++++++- test/content-length.js | 37 ++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/lib/client.js b/lib/client.js index b5170d4f88d..e97ef18d4d7 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1538,7 +1538,12 @@ function write (client, request) { contentLength = null } - if (request.contentLength !== null && request.contentLength !== contentLength) { + // https://github.com/nodejs/undici/issues/2046 + // A user agent may send a Content-Length header with 0 value, this should be + // allowed. + const bypassDelete = method === 'DELETE' && contentLength === null && request.contentLength === 0 + + if (!bypassDelete && request.contentLength !== null && request.contentLength !== contentLength) { if (client[kStrictContentLength]) { errorRequest(client, request, new RequestContentLengthMismatchError()) return false diff --git a/test/content-length.js b/test/content-length.js index 66c010a7c9e..edc6f7a5926 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -7,7 +7,7 @@ const { Readable } = require('stream') const { maybeWrapStream, consts } = require('./utils/async-iterators') test('request invalid content-length', (t) => { - t.plan(10) + t.plan(11) const server = createServer((req, res) => { res.end() @@ -134,6 +134,16 @@ test('request invalid content-length', (t) => { }, (err, data) => { t.type(err, errors.RequestContentLengthMismatchError) }) + + client.request({ + path: '/', + method: 'DELETE', + headers: { + 'content-length': 4 + } + }, (err, data) => { + t.type(err, errors.RequestContentLengthMismatchError) + }) }) }) @@ -329,3 +339,28 @@ test('request streaming with Readable.from(buf)', (t) => { }) }) }) + +test('request DELETE and content-length=0', (t) => { + t.plan(2) + const server = createServer((req, res) => { + res.end() + }) + t.teardown(server.close.bind(server)) + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`) + t.teardown(client.destroy.bind(client)) + + client.request({ + path: '/', + method: 'DELETE', + headers: { + 'content-length': 0 + } + }, (err) => { + t.error(err) + }) + client.on('disconnect', () => { + t.pass() + }) + }) +}) From 2461dbc103e69fb1c8b9e7092a075b8c8ad719ee Mon Sep 17 00:00:00 2001 From: Paul Xue Date: Thu, 5 Oct 2023 22:45:42 -0400 Subject: [PATCH 2/7] add shouldSendContentLength to better define conditions where content-length is added to the header --- lib/client.js | 24 ++++++++++--- test/content-length.js | 78 +++++++++++++++++++++++++----------------- 2 files changed, 66 insertions(+), 36 deletions(-) diff --git a/lib/client.js b/lib/client.js index e97ef18d4d7..1f152622499 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1495,6 +1495,23 @@ function _resume (client, sync) { } } +function shouldSendContentLength (contentLength, method) { + if (contentLength > 0) { + return true + } + // Many servers expect a Content-Length for these methods + if (method === 'POST' || method === 'PUT' || method === 'PATCH') { + return true + } + if (contentLength === 0) { + if (method === 'GET' || method === 'HEAD') { + return false + } + return true + } + return false +} + function write (client, request) { if (client[kHTTPConnVersion] === 'h2') { writeH2(client, client[kHTTP2Session], request) @@ -1539,11 +1556,8 @@ function write (client, request) { } // https://github.com/nodejs/undici/issues/2046 - // A user agent may send a Content-Length header with 0 value, this should be - // allowed. - const bypassDelete = method === 'DELETE' && contentLength === null && request.contentLength === 0 - - if (!bypassDelete && request.contentLength !== null && request.contentLength !== contentLength) { + // A user agent may send a Content-Length header with 0 value, this should be allowed. + if (shouldSendContentLength(contentLength, method) && request.contentLength !== null && request.contentLength !== contentLength) { if (client[kStrictContentLength]) { errorRequest(client, request, new RequestContentLengthMismatchError()) return false diff --git a/test/content-length.js b/test/content-length.js index edc6f7a5926..7ec33e67fbe 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -7,7 +7,7 @@ const { Readable } = require('stream') const { maybeWrapStream, consts } = require('./utils/async-iterators') test('request invalid content-length', (t) => { - t.plan(11) + t.plan(8) const server = createServer((req, res) => { res.end() @@ -61,26 +61,6 @@ test('request invalid content-length', (t) => { t.type(err, errors.RequestContentLengthMismatchError) }) - client.request({ - path: '/', - method: 'HEAD', - headers: { - 'content-length': 10 - } - }, (err, data) => { - t.type(err, errors.RequestContentLengthMismatchError) - }) - - client.request({ - path: '/', - method: 'GET', - headers: { - 'content-length': 0 - } - }, (err, data) => { - t.type(err, errors.RequestContentLengthMismatchError) - }) - client.request({ path: '/', method: 'GET', @@ -134,16 +114,6 @@ test('request invalid content-length', (t) => { }, (err, data) => { t.type(err, errors.RequestContentLengthMismatchError) }) - - client.request({ - path: '/', - method: 'DELETE', - headers: { - 'content-length': 4 - } - }, (err, data) => { - t.type(err, errors.RequestContentLengthMismatchError) - }) }) }) @@ -364,3 +334,49 @@ test('request DELETE and content-length=0', (t) => { }) }) }) + +test('content-length shouldSendContentLength=false', (t) => { + t.plan(5) + const server = createServer((req, res) => { + res.end() + }) + t.teardown(server.close.bind(server)) + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`) + t.teardown(client.destroy.bind(client)) + + client.request({ + path: '/', + method: 'HEAD', + headers: { + 'content-length': 10 + } + }, (err) => { + t.error(err) + }) + + client.request({ + path: '/', + method: 'GET', + headers: { + 'content-length': 0 + } + }, (err) => { + t.error(err) + }) + + client.request({ + path: '/', + method: 'DELETE', + headers: { + 'content-length': 4 + } + }, (err) => { + t.error(err) + }) + + client.on('disconnect', () => { + t.pass() + }) + }) +}) From 75aefbd44b8928f943f5d73368d6656ec29b9284 Mon Sep 17 00:00:00 2001 From: Paul Xue Date: Sun, 8 Oct 2023 22:33:30 -0400 Subject: [PATCH 3/7] refactor to ignore content-length on methods that do not anticipate a body --- lib/client.js | 17 +++----- test/content-length.js | 68 ++++++++++++++++---------------- test/no-strict-content-length.js | 4 -- 3 files changed, 39 insertions(+), 50 deletions(-) diff --git a/lib/client.js b/lib/client.js index 1f152622499..d0d9bee5211 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1497,19 +1497,12 @@ function _resume (client, sync) { function shouldSendContentLength (contentLength, method) { if (contentLength > 0) { - return true - } - // Many servers expect a Content-Length for these methods - if (method === 'POST' || method === 'PUT' || method === 'PATCH') { - return true - } - if (contentLength === 0) { - if (method === 'GET' || method === 'HEAD') { - return false + if (method === 'POST' || method === 'PUT' || method === 'PATCH') { + return true } - return true + return false } - return false + return (method === 'POST' || method === 'PUT' || method === 'PATCH') } function write (client, request) { @@ -1774,7 +1767,7 @@ function writeH2 (client, session, request) { contentLength = null } - if (request.contentLength != null && request.contentLength !== contentLength) { + if (shouldSendContentLength(contentLength, method) && request.contentLength != null && request.contentLength !== contentLength) { if (client[kStrictContentLength]) { errorRequest(client, request, new RequestContentLengthMismatchError()) return false diff --git a/test/content-length.js b/test/content-length.js index 7ec33e67fbe..2c6c847886a 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -7,7 +7,7 @@ const { Readable } = require('stream') const { maybeWrapStream, consts } = require('./utils/async-iterators') test('request invalid content-length', (t) => { - t.plan(8) + t.plan(6) const server = createServer((req, res) => { res.end() @@ -61,38 +61,6 @@ test('request invalid content-length', (t) => { t.type(err, errors.RequestContentLengthMismatchError) }) - client.request({ - path: '/', - method: 'GET', - headers: { - 'content-length': 4 - }, - body: new Readable({ - read () { - this.push('asd') - this.push(null) - } - }) - }, (err, data) => { - t.type(err, errors.RequestContentLengthMismatchError) - }) - - client.request({ - path: '/', - method: 'GET', - headers: { - 'content-length': 4 - }, - body: new Readable({ - read () { - this.push('asasdasdasdd') - this.push(null) - } - }) - }, (err, data) => { - t.type(err, errors.RequestContentLengthMismatchError) - }) - client.request({ path: '/', method: 'GET', @@ -336,7 +304,7 @@ test('request DELETE and content-length=0', (t) => { }) test('content-length shouldSendContentLength=false', (t) => { - t.plan(5) + t.plan(8) const server = createServer((req, res) => { res.end() }) @@ -375,6 +343,38 @@ test('content-length shouldSendContentLength=false', (t) => { t.error(err) }) + client.request({ + path: '/', + method: 'GET', + headers: { + 'content-length': 4 + }, + body: new Readable({ + read () { + this.push('asd') + this.push(null) + } + }) + }, (err) => { + t.error(err) + }) + + client.request({ + path: '/', + method: 'GET', + headers: { + 'content-length': 4 + }, + body: new Readable({ + read () { + this.push('asasdasdasdd') + this.push(null) + } + }) + }, (err) => { + t.error(err) + }) + client.on('disconnect', () => { t.pass() }) diff --git a/test/no-strict-content-length.js b/test/no-strict-content-length.js index 1d0eae6c789..993b0fdcf7a 100644 --- a/test/no-strict-content-length.js +++ b/test/no-strict-content-length.js @@ -90,7 +90,6 @@ tap.test('strictContentLength: false', (t) => { 'content-length': 10 } }, (err, data) => { - assertEmitWarningCalledAndReset() t.error(err) }) @@ -101,7 +100,6 @@ tap.test('strictContentLength: false', (t) => { 'content-length': 0 } }, (err, data) => { - assertEmitWarningCalledAndReset() t.error(err) }) @@ -118,7 +116,6 @@ tap.test('strictContentLength: false', (t) => { } }) }, (err, data) => { - assertEmitWarningCalledAndReset() t.error(err) }) @@ -135,7 +132,6 @@ tap.test('strictContentLength: false', (t) => { } }) }, (err, data) => { - assertEmitWarningCalledAndReset() t.error(err) }) }) From c8bf1f62eea981e3486885a9c1ed90a5f08f442d Mon Sep 17 00:00:00 2001 From: Paul Xue Date: Thu, 12 Oct 2023 23:46:15 -0400 Subject: [PATCH 4/7] fix up content-length to handle all cases with no defined semantics --- lib/client.js | 15 +++++-------- test/content-length.js | 50 +++++++++++++++++++++++++++++++++++------- 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/lib/client.js b/lib/client.js index d0d9bee5211..be428f4231e 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1495,14 +1495,9 @@ function _resume (client, sync) { } } -function shouldSendContentLength (contentLength, method) { - if (contentLength > 0) { - if (method === 'POST' || method === 'PUT' || method === 'PATCH') { - return true - } - return false - } - return (method === 'POST' || method === 'PUT' || method === 'PATCH') +// https://www.rfc-editor.org/rfc/rfc7230#section-3.3.2 +function shouldSendContentLength (method) { + return method !== 'GET' && method !== 'HEAD' && method !== 'OPTIONS' && method !== 'TRACE' && method !== 'CONNECT' } function write (client, request) { @@ -1550,7 +1545,7 @@ function write (client, request) { // https://github.com/nodejs/undici/issues/2046 // A user agent may send a Content-Length header with 0 value, this should be allowed. - if (shouldSendContentLength(contentLength, method) && request.contentLength !== null && request.contentLength !== contentLength) { + if (shouldSendContentLength(method) && contentLength > 0 && request.contentLength !== contentLength) { if (client[kStrictContentLength]) { errorRequest(client, request, new RequestContentLengthMismatchError()) return false @@ -1767,7 +1762,7 @@ function writeH2 (client, session, request) { contentLength = null } - if (shouldSendContentLength(contentLength, method) && request.contentLength != null && request.contentLength !== contentLength) { + if (shouldSendContentLength(method) && contentLength > 0 && request.contentLength !== contentLength) { if (client[kStrictContentLength]) { errorRequest(client, request, new RequestContentLengthMismatchError()) return false diff --git a/test/content-length.js b/test/content-length.js index 2c6c847886a..b95f0c1bafe 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -7,7 +7,7 @@ const { Readable } = require('stream') const { maybeWrapStream, consts } = require('./utils/async-iterators') test('request invalid content-length', (t) => { - t.plan(6) + t.plan(7) const server = createServer((req, res) => { res.end() @@ -82,6 +82,17 @@ test('request invalid content-length', (t) => { }, (err, data) => { t.type(err, errors.RequestContentLengthMismatchError) }) + + client.request({ + path: '/', + method: 'DELETE', + headers: { + 'content-length': 4 + }, + body: ['asasdasdasdd'] + }, (err, data) => { + t.type(err, errors.RequestContentLengthMismatchError) + }) }) }) @@ -278,8 +289,8 @@ test('request streaming with Readable.from(buf)', (t) => { }) }) -test('request DELETE and content-length=0', (t) => { - t.plan(2) +test('request DELETE, content-length=0, with body', (t) => { + t.plan(3) const server = createServer((req, res) => { res.end() }) @@ -288,6 +299,22 @@ test('request DELETE and content-length=0', (t) => { const client = new Client(`http://localhost:${server.address().port}`) t.teardown(client.destroy.bind(client)) + client.request({ + path: '/', + method: 'DELETE', + headers: { + 'content-length': 0 + }, + body: new Readable({ + read () { + this.push('asd') + this.push(null) + } + }), + }, (err) => { + t.type(err, errors.RequestContentLengthMismatchError) + }) + client.request({ path: '/', method: 'DELETE', @@ -297,6 +324,7 @@ test('request DELETE and content-length=0', (t) => { }, (err) => { t.error(err) }) + client.on('disconnect', () => { t.pass() }) @@ -304,7 +332,7 @@ test('request DELETE and content-length=0', (t) => { }) test('content-length shouldSendContentLength=false', (t) => { - t.plan(8) + t.plan(9) const server = createServer((req, res) => { res.end() }) @@ -335,10 +363,16 @@ test('content-length shouldSendContentLength=false', (t) => { client.request({ path: '/', - method: 'DELETE', + method: 'GET', headers: { 'content-length': 4 - } + }, + body: new Readable({ + read () { + this.push('asd') + this.push(null) + } + }) }, (err) => { t.error(err) }) @@ -351,7 +385,7 @@ test('content-length shouldSendContentLength=false', (t) => { }, body: new Readable({ read () { - this.push('asd') + this.push('asasdasdasdd') this.push(null) } }) @@ -361,7 +395,7 @@ test('content-length shouldSendContentLength=false', (t) => { client.request({ path: '/', - method: 'GET', + method: 'HEAD', headers: { 'content-length': 4 }, From 4587f379b70c3cc9eb086f7e7d0d653966a98df1 Mon Sep 17 00:00:00 2001 From: Paul Xue Date: Fri, 13 Oct 2023 19:34:04 -0400 Subject: [PATCH 5/7] fix up tests --- .prettierignore | 1 + lib/client.js | 6 ++++-- test/content-length.js | 23 ++++++++++++++++++----- 3 files changed, 23 insertions(+), 7 deletions(-) create mode 100644 .prettierignore diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 00000000000..798c60da1f5 --- /dev/null +++ b/.prettierignore @@ -0,0 +1 @@ +*/** diff --git a/lib/client.js b/lib/client.js index be428f4231e..6e4c61e1830 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1545,7 +1545,7 @@ function write (client, request) { // https://github.com/nodejs/undici/issues/2046 // A user agent may send a Content-Length header with 0 value, this should be allowed. - if (shouldSendContentLength(method) && contentLength > 0 && request.contentLength !== contentLength) { + if (shouldSendContentLength(method) && contentLength > 0 && request.contentLength !== null && request.contentLength !== contentLength) { if (client[kStrictContentLength]) { errorRequest(client, request, new RequestContentLengthMismatchError()) return false @@ -1762,7 +1762,9 @@ function writeH2 (client, session, request) { contentLength = null } - if (shouldSendContentLength(method) && contentLength > 0 && request.contentLength !== contentLength) { + // https://github.com/nodejs/undici/issues/2046 + // A user agent may send a Content-Length header with 0 value, this should be allowed. + if (shouldSendContentLength(method) && contentLength > 0 && request.contentLength != null && request.contentLength !== contentLength) { if (client[kStrictContentLength]) { errorRequest(client, request, new RequestContentLengthMismatchError()) return false diff --git a/test/content-length.js b/test/content-length.js index b95f0c1bafe..082c3f3e0e6 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -290,7 +290,7 @@ test('request streaming with Readable.from(buf)', (t) => { }) test('request DELETE, content-length=0, with body', (t) => { - t.plan(3) + t.plan(4) const server = createServer((req, res) => { res.end() }) @@ -310,7 +310,7 @@ test('request DELETE, content-length=0, with body', (t) => { this.push('asd') this.push(null) } - }), + }) }, (err) => { t.type(err, errors.RequestContentLengthMismatchError) }) @@ -321,7 +321,8 @@ test('request DELETE, content-length=0, with body', (t) => { headers: { 'content-length': 0 } - }, (err) => { + }, (err, resp) => { + t.equal(resp.headers['content-length'], '0') t.error(err) }) @@ -332,7 +333,7 @@ test('request DELETE, content-length=0, with body', (t) => { }) test('content-length shouldSendContentLength=false', (t) => { - t.plan(9) + t.plan(12) const server = createServer((req, res) => { res.end() }) @@ -341,13 +342,25 @@ test('content-length shouldSendContentLength=false', (t) => { const client = new Client(`http://localhost:${server.address().port}`) t.teardown(client.destroy.bind(client)) + client.request({ + path: '/', + method: 'PUT', + headers: { + 'content-length': 0 + } + }, (err, resp) => { + t.equal(resp.headers['content-length'], '0') + t.error(err) + }) + client.request({ path: '/', method: 'HEAD', headers: { 'content-length': 10 } - }, (err) => { + }, (err, resp) => { + t.equal(resp.headers['content-length'], undefined) t.error(err) }) From 194a60caab334db5a1441f15b7cb68aff14e7683 Mon Sep 17 00:00:00 2001 From: Paul Xue Date: Fri, 13 Oct 2023 19:35:42 -0400 Subject: [PATCH 6/7] remove prettierignore --- .prettierignore | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .prettierignore diff --git a/.prettierignore b/.prettierignore deleted file mode 100644 index 798c60da1f5..00000000000 --- a/.prettierignore +++ /dev/null @@ -1 +0,0 @@ -*/** From 741b8118e1452d9c00358b2c33d7068683e9dcc6 Mon Sep 17 00:00:00 2001 From: Paul Xue Date: Sun, 15 Oct 2023 17:09:08 -0400 Subject: [PATCH 7/7] test content-length in request is undefined or 0 --- test/content-length.js | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/test/content-length.js b/test/content-length.js index 082c3f3e0e6..9ce74051d8b 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -290,10 +290,13 @@ test('request streaming with Readable.from(buf)', (t) => { }) test('request DELETE, content-length=0, with body', (t) => { - t.plan(4) + t.plan(5) const server = createServer((req, res) => { res.end() }) + server.on('request', (req, res) => { + t.equal(req.headers['content-length'], undefined) + }) t.teardown(server.close.bind(server)) server.listen(0, () => { const client = new Client(`http://localhost:${server.address().port}`) @@ -333,17 +336,30 @@ test('request DELETE, content-length=0, with body', (t) => { }) test('content-length shouldSendContentLength=false', (t) => { - t.plan(12) + t.plan(15) const server = createServer((req, res) => { res.end() }) + server.on('request', (req, res) => { + switch (req.url) { + case '/put0': + t.equal(req.headers['content-length'], '0') + break + case '/head': + t.equal(req.headers['content-length'], undefined) + break + case '/get': + t.equal(req.headers['content-length'], undefined) + break + } + }) t.teardown(server.close.bind(server)) server.listen(0, () => { const client = new Client(`http://localhost:${server.address().port}`) t.teardown(client.destroy.bind(client)) client.request({ - path: '/', + path: '/put0', method: 'PUT', headers: { 'content-length': 0 @@ -354,7 +370,7 @@ test('content-length shouldSendContentLength=false', (t) => { }) client.request({ - path: '/', + path: '/head', method: 'HEAD', headers: { 'content-length': 10 @@ -365,7 +381,7 @@ test('content-length shouldSendContentLength=false', (t) => { }) client.request({ - path: '/', + path: '/get', method: 'GET', headers: { 'content-length': 0