From 59dbe4b633f8d6a9368818801213e11c542d353a Mon Sep 17 00:00:00 2001 From: Deokjin Kim Date: Fri, 20 Jan 2023 09:14:27 +0900 Subject: [PATCH 1/6] http: throw error if options of http.Server is array If options of http.Server is array, throw ERR_INVALID_ARG_TYPE. Refs: https://github.com/nodejs/node/pull/24176 --- lib/_http_server.js | 2 +- test/parallel/test-http-server.js | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 4f16ab7b4033d6..c37beeedca587f 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -503,7 +503,7 @@ function Server(options, requestListener) { if (typeof options === 'function') { requestListener = options; options = {}; - } else if (options == null || typeof options === 'object') { + } else if (options == null || (typeof options === 'object' && !ArrayIsArray(options))) { options = { ...options }; } else { throw new ERR_INVALID_ARG_TYPE('options', 'object', options); diff --git a/test/parallel/test-http-server.js b/test/parallel/test-http-server.js index a26912c409a24c..b93258b198afe1 100644 --- a/test/parallel/test-http-server.js +++ b/test/parallel/test-http-server.js @@ -27,10 +27,7 @@ const http = require('http'); const url = require('url'); const qs = require('querystring'); -// TODO: documentation does not allow Array as an option, so testing that -// should fail, but currently http.Server does not typecheck further than -// if `option` is `typeof object` - so we don't test that here right now -const invalid_options = [ 'foo', 42, true ]; +const invalid_options = [ 'foo', 42, true, [] ]; invalid_options.forEach((option) => { assert.throws(() => { From 882d62d6acac931cd347d604c8ce1121aba301d6 Mon Sep 17 00:00:00 2001 From: Deokjin Kim Date: Sun, 22 Jan 2023 10:54:34 +0900 Subject: [PATCH 2/6] refactor to use validateObject --- lib/_http_server.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index c37beeedca587f..4195b10001256d 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -75,7 +75,6 @@ const { ERR_HTTP_HEADERS_SENT, ERR_HTTP_INVALID_STATUS_CODE, ERR_HTTP_SOCKET_ENCODING, - ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, ERR_INVALID_CHAR } = codes; @@ -503,10 +502,10 @@ function Server(options, requestListener) { if (typeof options === 'function') { requestListener = options; options = {}; - } else if (options == null || (typeof options === 'object' && !ArrayIsArray(options))) { - options = { ...options }; + } else if (options == null) { + options = {}; } else { - throw new ERR_INVALID_ARG_TYPE('options', 'object', options); + validateObject(options, 'options'); } storeHTTPOptions.call(this, options); From fc78793fb7df11b75f8ceeb8105150c833fa966b Mon Sep 17 00:00:00 2001 From: Deokjin Kim Date: Wed, 25 Jan 2023 00:43:30 +0900 Subject: [PATCH 3/6] use kEmptyObject for options and apply same change to https.server --- lib/_http_server.js | 12 ++++++------ lib/https.js | 23 +++++++++++++---------- test/parallel/test-https-simple.js | 9 +++++++++ 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 4195b10001256d..038845de2908f2 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -78,6 +78,9 @@ const { ERR_INVALID_ARG_VALUE, ERR_INVALID_CHAR } = codes; +const { + kEmptyObject, +} = require('internal/util'); const { validateInteger, validateBoolean, @@ -432,9 +435,6 @@ function storeHTTPOptions(options) { validateBoolean(insecureHTTPParser, 'options.insecureHTTPParser'); this.insecureHTTPParser = insecureHTTPParser; - if (options.noDelay === undefined) - options.noDelay = true; - const requestTimeout = options.requestTimeout; if (requestTimeout !== undefined) { validateInteger(requestTimeout, 'requestTimeout', 0); @@ -501,9 +501,9 @@ function Server(options, requestListener) { if (typeof options === 'function') { requestListener = options; - options = {}; + options = kEmptyObject; } else if (options == null) { - options = {}; + options = kEmptyObject; } else { validateObject(options, 'options'); } @@ -511,7 +511,7 @@ function Server(options, requestListener) { storeHTTPOptions.call(this, options); net.Server.call( this, - { allowHalfOpen: true, noDelay: options.noDelay, + { allowHalfOpen: true, noDelay: options.noDelay || true, keepAlive: options.keepAlive, keepAliveInitialDelay: options.keepAliveInitialDelay }); diff --git a/lib/https.js b/lib/https.js index e871b35bcacbc5..160b2e0e427172 100644 --- a/lib/https.js +++ b/lib/https.js @@ -53,25 +53,28 @@ let debug = require('internal/util/debuglog').debuglog('https', (fn) => { debug = fn; }); const { URL, urlToHttpOptions, searchParamsSymbol } = require('internal/url'); +const { validateObject } = require('internal/validators'); function Server(opts, requestListener) { if (!(this instanceof Server)) return new Server(opts, requestListener); if (typeof opts === 'function') { requestListener = opts; - opts = undefined; - } - opts = { ...opts }; - - if (!opts.ALPNProtocols) { - // http/1.0 is not defined as Protocol IDs in IANA - // https://www.iana.org/assignments/tls-extensiontype-values - // /tls-extensiontype-values.xhtml#alpn-protocol-ids - opts.ALPNProtocols = ['http/1.1']; + opts = kEmptyObject; + } else if (opts == null) { + opts = kEmptyObject; + } else { + validateObject(opts, 'options'); } FunctionPrototypeCall(storeHTTPOptions, this, opts); - FunctionPrototypeCall(tls.Server, this, opts, _connectionListener); + FunctionPrototypeCall(tls.Server, this, + { ...opts, + // http/1.0 is not defined as Protocol IDs in IANA + // https://www.iana.org/assignments/tls-extensiontype-values + // /tls-extensiontype-values.xhtml#alpn-protocol-ids + ALPNProtocols: opts.ALPNProtocols || ['http/1.1'] }, + _connectionListener); this.httpAllowHalfOpen = false; diff --git a/test/parallel/test-https-simple.js b/test/parallel/test-https-simple.js index 89707cc6468d68..a65883162f60a2 100644 --- a/test/parallel/test-https-simple.js +++ b/test/parallel/test-https-simple.js @@ -44,6 +44,15 @@ const serverCallback = common.mustCall(function(req, res) { res.end(body); }); +const invalid_options = [ 'foo', 42, true, [] ]; +invalid_options.forEach((option) => { + assert.throws(() => { + new https.Server(option); + }, { + code: 'ERR_INVALID_ARG_TYPE', + }); +}); + const server = https.createServer(options, serverCallback); server.listen(0, common.mustCall(() => { From 548bdf5b0bfd88d37643e7ea583b80eb34fe593a Mon Sep 17 00:00:00 2001 From: Deokjin Kim Date: Wed, 25 Jan 2023 01:13:35 +0900 Subject: [PATCH 4/6] fixup! pass ommited noDelay option in https --- lib/https.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/https.js b/lib/https.js index 160b2e0e427172..cf56c9387ef140 100644 --- a/lib/https.js +++ b/lib/https.js @@ -70,6 +70,7 @@ function Server(opts, requestListener) { FunctionPrototypeCall(storeHTTPOptions, this, opts); FunctionPrototypeCall(tls.Server, this, { ...opts, + noDelay: opts.noDelay || true, // http/1.0 is not defined as Protocol IDs in IANA // https://www.iana.org/assignments/tls-extensiontype-values // /tls-extensiontype-values.xhtml#alpn-protocol-ids From 141501dc76eddd5d047b76d4e5ad7a29d8c1f62a Mon Sep 17 00:00:00 2001 From: Deokjin Kim Date: Wed, 25 Jan 2023 01:23:30 +0900 Subject: [PATCH 5/6] remove overwriting options Co-authored-by: Antoine du Hamel --- lib/_http_server.js | 2 +- lib/https.js | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 038845de2908f2..7e96a0b2bb3397 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -511,7 +511,7 @@ function Server(options, requestListener) { storeHTTPOptions.call(this, options); net.Server.call( this, - { allowHalfOpen: true, noDelay: options.noDelay || true, + { allowHalfOpen: true, noDelay: options.noDelay ?? true, keepAlive: options.keepAlive, keepAliveInitialDelay: options.keepAliveInitialDelay }); diff --git a/lib/https.js b/lib/https.js index cf56c9387ef140..ea8a7ec4451308 100644 --- a/lib/https.js +++ b/lib/https.js @@ -69,12 +69,14 @@ function Server(opts, requestListener) { FunctionPrototypeCall(storeHTTPOptions, this, opts); FunctionPrototypeCall(tls.Server, this, - { ...opts, - noDelay: opts.noDelay || true, + { + noDelay: true, // http/1.0 is not defined as Protocol IDs in IANA // https://www.iana.org/assignments/tls-extensiontype-values // /tls-extensiontype-values.xhtml#alpn-protocol-ids - ALPNProtocols: opts.ALPNProtocols || ['http/1.1'] }, + ALPNProtocols: ['http/1.1'], + ...opts, + }, _connectionListener); this.httpAllowHalfOpen = false; From a41a49685281072f5eac8c1c28b828e0d3c42a26 Mon Sep 17 00:00:00 2001 From: Deokjin Kim Date: Wed, 25 Jan 2023 01:30:09 +0900 Subject: [PATCH 6/6] fixup! lint error --- lib/https.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/https.js b/lib/https.js index ea8a7ec4451308..c3ecbc45ee41ed 100644 --- a/lib/https.js +++ b/lib/https.js @@ -69,14 +69,14 @@ function Server(opts, requestListener) { FunctionPrototypeCall(storeHTTPOptions, this, opts); FunctionPrototypeCall(tls.Server, this, - { + { noDelay: true, // http/1.0 is not defined as Protocol IDs in IANA // https://www.iana.org/assignments/tls-extensiontype-values // /tls-extensiontype-values.xhtml#alpn-protocol-ids ALPNProtocols: ['http/1.1'], ...opts, - }, + }, _connectionListener); this.httpAllowHalfOpen = false;