From 9fce688236c7aa81e93e715d55c14717d088964b Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Tue, 1 Oct 2024 00:15:01 +0200 Subject: [PATCH 1/4] fix: check maxHeadersSize on client instantiation and not on Parser instantion --- lib/dispatcher/client-h1.js | 2 -- lib/dispatcher/client.js | 2 +- test/node-test/client-errors.js | 40 +++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index a079e028e7f..fc4e0db9ec4 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -201,8 +201,6 @@ class Parser { * @param {*} llhttp */ constructor (client, socket, { exports }) { - assert(Number.isFinite(client[kMaxHeadersSize]) && client[kMaxHeadersSize] > 0) - this.llhttp = exports this.ptr = this.llhttp.llhttp_alloc(constants.TYPE.RESPONSE) this.client = client diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js index 65380f19167..ef6ce647c0d 100644 --- a/lib/dispatcher/client.js +++ b/lib/dispatcher/client.js @@ -123,7 +123,7 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('unsupported maxKeepAliveTimeout, use keepAliveMaxTimeout instead') } - if (maxHeaderSize != null && !Number.isFinite(maxHeaderSize)) { + if (maxHeaderSize != null && (!Number.isInteger(maxHeaderSize) || maxHeaderSize < 1)) { throw new InvalidArgumentError('invalid maxHeaderSize') } diff --git a/test/node-test/client-errors.js b/test/node-test/client-errors.js index d53e9ffc146..2a07a4cfbf7 100644 --- a/test/node-test/client-errors.js +++ b/test/node-test/client-errors.js @@ -399,6 +399,46 @@ test('invalid options throws', (t, done) => { assert.strictEqual(err.message, 'invalid maxHeaderSize') } + try { + new Client(new URL('http://localhost:200'), { // eslint-disable-line + maxHeaderSize: 0 + }) + assert.ok(0) + } catch (err) { + assert.ok(err instanceof errors.InvalidArgumentError) + assert.strictEqual(err.message, 'invalid maxHeaderSize') + } + + try { + new Client(new URL('http://localhost:200'), { // eslint-disable-line + maxHeaderSize: 0 + }) + assert.ok(0) + } catch (err) { + assert.ok(err instanceof errors.InvalidArgumentError) + assert.strictEqual(err.message, 'invalid maxHeaderSize') + } + + try { + new Client(new URL('http://localhost:200'), { // eslint-disable-line + maxHeaderSize: -10 + }) + assert.ok(0) + } catch (err) { + assert.ok(err instanceof errors.InvalidArgumentError) + assert.strictEqual(err.message, 'invalid maxHeaderSize') + } + + try { + new Client(new URL('http://localhost:200'), { // eslint-disable-line + maxHeaderSize: 1.5 + }) + assert.ok(0) + } catch (err) { + assert.ok(err instanceof errors.InvalidArgumentError) + assert.strictEqual(err.message, 'invalid maxHeaderSize') + } + try { new Client(1) // eslint-disable-line assert.ok(0) From 998f885bb03fc10dfc302a3a3b26a7979291a0ca Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Thu, 3 Oct 2024 15:16:33 +0200 Subject: [PATCH 2/4] ensure that http.maxHeadersize is valid --- lib/dispatcher/client.js | 11 ++++++++--- test/client-node-max-header-size.js | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js index ef6ce647c0d..e66441a8b13 100644 --- a/lib/dispatcher/client.js +++ b/lib/dispatcher/client.js @@ -1,5 +1,3 @@ -// @ts-check - 'use strict' const assert = require('node:assert') @@ -60,6 +58,13 @@ const connectH2 = require('./client-h2.js') const kClosedResolve = Symbol('kClosedResolve') +const getDefaultNodeMaxHeaderSize = http && + http.maxHeaderSize && + Number.isInteger(http.maxHeaderSize) && + http.maxHeaderSize > 0 + ? () => http.maxHeaderSize + : () => { throw new InvalidArgumentError('http module not available or http.maxHeaderSize invalid') } + const noop = () => {} function getPipelining (client) { @@ -204,7 +209,7 @@ class Client extends DispatcherBase { this[kUrl] = util.parseOrigin(url) this[kConnector] = connect this[kPipelining] = pipelining != null ? pipelining : 1 - this[kMaxHeadersSize] = maxHeaderSize || http.maxHeaderSize + this[kMaxHeadersSize] = maxHeaderSize || getDefaultNodeMaxHeaderSize() this[kKeepAliveDefaultTimeout] = keepAliveTimeout == null ? 4e3 : keepAliveTimeout this[kKeepAliveMaxTimeout] = keepAliveMaxTimeout == null ? 600e3 : keepAliveMaxTimeout this[kKeepAliveTimeoutThreshold] = keepAliveTimeoutThreshold == null ? 2e3 : keepAliveTimeoutThreshold diff --git a/test/client-node-max-header-size.js b/test/client-node-max-header-size.js index 379c5e6970a..4c23113dde8 100644 --- a/test/client-node-max-header-size.js +++ b/test/client-node-max-header-size.js @@ -41,4 +41,23 @@ describe("Node.js' --max-http-header-size cli option", () => { await t.completed }) + + test('--max-http-header-size with Client API', async (t) => { + t = tspl(t, { plan: 6 }) + const command = 'node -e "new (require(\'.\').Client)(new URL(\'http://localhost:200\'))"' + + exec(`${command} --max-http-header-size=0`, { stdio: 'pipe' }, (err, stdout, stderr) => { + t.strictEqual(err.code, 1) + t.strictEqual(stdout, '') + t.match(stderr, /http module not available or http.maxHeaderSize invalid/, '--max-http-header-size=0 should result in an Error when using the Client API') + }) + + exec(command, { stdio: 'pipe' }, (err, stdout, stderr) => { + t.ifError(err) + t.strictEqual(stdout, '') + t.strictEqual(stderr, '', 'default max-http-header-size should not throw') + }) + + await t.completed + }) }) From 6b3066489352594bb223ed493ca3083f6654b5c7 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Thu, 3 Oct 2024 15:20:30 +0200 Subject: [PATCH 3/4] fail early, fail fast --- lib/dispatcher/client.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js index e66441a8b13..3cb0fb695bf 100644 --- a/lib/dispatcher/client.js +++ b/lib/dispatcher/client.js @@ -130,6 +130,10 @@ class Client extends DispatcherBase { if (maxHeaderSize != null && (!Number.isInteger(maxHeaderSize) || maxHeaderSize < 1)) { throw new InvalidArgumentError('invalid maxHeaderSize') + } else { + // If maxHeaderSize is not provided, use the default value from the http module + // or if that is not available, throw an error. + maxHeaderSize = getDefaultNodeMaxHeaderSize() } if (socketPath != null && typeof socketPath !== 'string') { @@ -209,7 +213,7 @@ class Client extends DispatcherBase { this[kUrl] = util.parseOrigin(url) this[kConnector] = connect this[kPipelining] = pipelining != null ? pipelining : 1 - this[kMaxHeadersSize] = maxHeaderSize || getDefaultNodeMaxHeaderSize() + this[kMaxHeadersSize] = maxHeaderSize this[kKeepAliveDefaultTimeout] = keepAliveTimeout == null ? 4e3 : keepAliveTimeout this[kKeepAliveMaxTimeout] = keepAliveMaxTimeout == null ? 600e3 : keepAliveMaxTimeout this[kKeepAliveTimeoutThreshold] = keepAliveTimeoutThreshold == null ? 2e3 : keepAliveTimeoutThreshold From d2a1290ef65059b8ab43eb4d31c34bb9b7e12928 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Sat, 5 Oct 2024 23:11:43 +0200 Subject: [PATCH 4/4] fix --- lib/dispatcher/client.js | 6 ++++-- test/client-errors.js | 31 ------------------------------- test/node-test/client-errors.js | 31 ++++++++++++++++++++++++++++--- 3 files changed, 32 insertions(+), 36 deletions(-) delete mode 100644 test/client-errors.js diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js index 3cb0fb695bf..3939b89b5b4 100644 --- a/lib/dispatcher/client.js +++ b/lib/dispatcher/client.js @@ -128,8 +128,10 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('unsupported maxKeepAliveTimeout, use keepAliveMaxTimeout instead') } - if (maxHeaderSize != null && (!Number.isInteger(maxHeaderSize) || maxHeaderSize < 1)) { - throw new InvalidArgumentError('invalid maxHeaderSize') + if (maxHeaderSize != null) { + if (!Number.isInteger(maxHeaderSize) || maxHeaderSize < 1) { + throw new InvalidArgumentError('invalid maxHeaderSize') + } } else { // If maxHeaderSize is not provided, use the default value from the http module // or if that is not available, throw an error. diff --git a/test/client-errors.js b/test/client-errors.js deleted file mode 100644 index 0c935d7cdac..00000000000 --- a/test/client-errors.js +++ /dev/null @@ -1,31 +0,0 @@ -'use strict' - -const { tspl } = require('@matteo.collina/tspl') -const { test, after } = require('node:test') -const { Client } = require('..') -const net = require('node:net') - -// TODO: move to test/node-test/client-connect.js -test('parser error', async (t) => { - t = tspl(t, { plan: 2 }) - - const server = net.createServer() - server.once('connection', (socket) => { - socket.write('asd\n\r213123') - }) - after(() => server.close()) - - server.listen(0, () => { - const client = new Client(`http://localhost:${server.address().port}`) - after(() => client.destroy()) - - client.request({ path: '/', method: 'GET' }, (err) => { - t.ok(err) - client.close((err) => { - t.ifError(err) - }) - }) - }) - - await t.completed -}) diff --git a/test/node-test/client-errors.js b/test/node-test/client-errors.js index 2a07a4cfbf7..39f04678dc6 100644 --- a/test/node-test/client-errors.js +++ b/test/node-test/client-errors.js @@ -1,12 +1,13 @@ 'use strict' -const { test } = require('node:test') const assert = require('node:assert') +const https = require('node:https') +const net = require('node:net') +const { Readable } = require('node:stream') +const { test, after } = require('node:test') const { Client, Pool, errors } = require('../..') const { createServer } = require('node:http') -const https = require('node:https') const pem = require('https-pem') -const { Readable } = require('node:stream') const { tspl } = require('@matteo.collina/tspl') const { kSocket } = require('../../lib/core/symbols') @@ -1364,3 +1365,27 @@ test('SocketError should expose socket details (tls)', async (t) => { await p.completed }) + +test('parser error', async (t) => { + t = tspl(t, { plan: 2 }) + + const server = net.createServer() + server.once('connection', (socket) => { + socket.write('asd\n\r213123') + }) + after(() => server.close()) + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`) + after(() => client.destroy()) + + client.request({ path: '/', method: 'GET' }, (err) => { + t.ok(err) + client.close((err) => { + t.ifError(err) + }) + }) + }) + + await t.completed +})