From 35331cbd1350a64b5ec5daf1e3ae7e096b7e6bd3 Mon Sep 17 00:00:00 2001 From: Qingyu Deng Date: Thu, 17 Jun 2021 22:29:03 +0800 Subject: [PATCH] http,https: align server option of https with http MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: https://github.com/nodejs/node/issues/38954 PR-URL: https://github.com/nodejs/node/pull/38992 Refs: https://github.com/nodejs/node/pull/30570 Reviewed-By: Anna Henningsen Reviewed-By: Michaël Zasso --- lib/_http_server.js | 28 ++-- lib/https.js | 8 +- .../test-https-insecure-parse-per-stream.js | 131 ++++++++++++++++++ .../test-https-max-header-size-per-stream.js | 119 ++++++++++++++++ 4 files changed, 268 insertions(+), 18 deletions(-) create mode 100644 test/parallel/test-https-insecure-parse-per-stream.js create mode 100644 test/parallel/test-https-max-header-size-per-stream.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 97df58a007daba..11119169d56f2c 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -353,18 +353,7 @@ function writeHead(statusCode, reason, obj) { // Docs-only deprecated: DEP0063 ServerResponse.prototype.writeHeader = ServerResponse.prototype.writeHead; -function Server(options, requestListener) { - if (!(this instanceof Server)) return new Server(options, requestListener); - - if (typeof options === 'function') { - requestListener = options; - options = {}; - } else if (options == null || typeof options === 'object') { - options = { ...options }; - } else { - throw new ERR_INVALID_ARG_TYPE('options', 'object', options); - } - +function storeHTTPOptions(options) { this[kIncomingMessage] = options.IncomingMessage || IncomingMessage; this[kServerResponse] = options.ServerResponse || ServerResponse; @@ -377,7 +366,21 @@ function Server(options, requestListener) { if (insecureHTTPParser !== undefined) validateBoolean(insecureHTTPParser, 'options.insecureHTTPParser'); this.insecureHTTPParser = insecureHTTPParser; +} + +function Server(options, requestListener) { + if (!(this instanceof Server)) return new Server(options, requestListener); + + if (typeof options === 'function') { + requestListener = options; + options = {}; + } else if (options == null || typeof options === 'object') { + options = { ...options }; + } else { + throw new ERR_INVALID_ARG_TYPE('options', 'object', options); + } + storeHTTPOptions.call(this, options); net.Server.call(this, { allowHalfOpen: true }); if (requestListener) { @@ -991,6 +994,7 @@ module.exports = { STATUS_CODES, Server, ServerResponse, + storeHTTPOptions, _connectionListener: connectionListener, kServerResponse }; diff --git a/lib/https.js b/lib/https.js index 765e1a22b60696..695a9020994852 100644 --- a/lib/https.js +++ b/lib/https.js @@ -40,16 +40,14 @@ const tls = require('tls'); const { Agent: HttpAgent } = require('_http_agent'); const { Server: HttpServer, + storeHTTPOptions, _connectionListener, - kServerResponse } = require('_http_server'); const { ClientRequest } = require('_http_client'); let debug = require('internal/util/debuglog').debuglog('https', (fn) => { debug = fn; }); const { URL, urlToHttpOptions, searchParamsSymbol } = require('internal/url'); -const { IncomingMessage, ServerResponse } = require('http'); -const { kIncomingMessage } = require('_http_common'); function Server(opts, requestListener) { if (!(this instanceof Server)) return new Server(opts, requestListener); @@ -67,9 +65,7 @@ function Server(opts, requestListener) { opts.ALPNProtocols = ['http/1.1']; } - this[kIncomingMessage] = opts.IncomingMessage || IncomingMessage; - this[kServerResponse] = opts.ServerResponse || ServerResponse; - + FunctionPrototypeCall(storeHTTPOptions, this, opts); FunctionPrototypeCall(tls.Server, this, opts, _connectionListener); this.httpAllowHalfOpen = false; diff --git a/test/parallel/test-https-insecure-parse-per-stream.js b/test/parallel/test-https-insecure-parse-per-stream.js new file mode 100644 index 00000000000000..645fbcf2637654 --- /dev/null +++ b/test/parallel/test-https-insecure-parse-per-stream.js @@ -0,0 +1,131 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) { + common.skip('missing crypto'); +} + +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const https = require('https'); +const MakeDuplexPair = require('../common/duplexpair'); +const tls = require('tls'); +const { finished } = require('stream'); + +const certFixture = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + ca: fixtures.readKey('ca1-cert.pem'), +}; + + +// Test that setting the `insecureHTTPParse` option works on a per-stream-basis. + +// Test 1: The server sends an invalid header. +{ + const { clientSide, serverSide } = MakeDuplexPair(); + + const req = https.request({ + rejectUnauthorized: false, + createConnection: common.mustCall(() => clientSide), + insecureHTTPParser: true + }, common.mustCall((res) => { + assert.strictEqual(res.headers.hello, 'foo\x08foo'); + res.resume(); // We don’t actually care about contents. + res.on('end', common.mustCall()); + })); + req.end(); + + serverSide.resume(); // Dump the request + serverSide.end('HTTP/1.1 200 OK\r\n' + + 'Hello: foo\x08foo\r\n' + + 'Content-Length: 0\r\n' + + '\r\n\r\n'); +} + +// Test 2: The same as Test 1 except without the option, to make sure it fails. +{ + const { clientSide, serverSide } = MakeDuplexPair(); + + const req = https.request({ + rejectUnauthorized: false, + createConnection: common.mustCall(() => clientSide) + }, common.mustNotCall()); + req.end(); + req.on('error', common.mustCall()); + + serverSide.resume(); // Dump the request + serverSide.end('HTTP/1.1 200 OK\r\n' + + 'Hello: foo\x08foo\r\n' + + 'Content-Length: 0\r\n' + + '\r\n\r\n'); +} + +// Test 3: The client sends an invalid header. +{ + const testData = 'Hello, World!\n'; + const server = https.createServer( + { insecureHTTPParser: true, + ...certFixture }, + common.mustCall((req, res) => { + res.statusCode = 200; + res.setHeader('Content-Type', 'text/plain'); + res.end(testData); + })); + + server.on('clientError', common.mustNotCall()); + + server.listen(0, common.mustCall(() => { + const client = tls.connect({ + port: server.address().port, + rejectUnauthorized: false + }); + client.write( + 'GET / HTTP/1.1\r\n' + + 'Hello: foo\x08foo\r\n' + + '\r\n\r\n'); + client.end(); + + client.on('data', () => {}); + finished(client, common.mustCall(() => { + server.close(); + })); + })); +} + +// Test 4: The same as Test 3 except without the option, to make sure it fails. +{ + const server = https.createServer( + { ...certFixture }, + common.mustNotCall()); + + server.on('clientError', common.mustCall()); + + server.listen(0, common.mustCall(() => { + const client = tls.connect({ + port: server.address().port, + rejectUnauthorized: false + }); + client.write( + 'GET / HTTP/1.1\r\n' + + 'Hello: foo\x08foo\r\n' + + '\r\n\r\n'); + client.end(); + + client.on('data', () => {}); + finished(client, common.mustCall(() => { + server.close(); + })); + })); +} + +// Test 5: Invalid argument type +{ + assert.throws( + () => https.request({ insecureHTTPParser: 0 }, common.mustNotCall()), + common.expectsError({ + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "options.insecureHTTPParser" property must be of' + + ' type boolean. Received type number (0)' + }) + ); +} diff --git a/test/parallel/test-https-max-header-size-per-stream.js b/test/parallel/test-https-max-header-size-per-stream.js new file mode 100644 index 00000000000000..f7117e16fb43f6 --- /dev/null +++ b/test/parallel/test-https-max-header-size-per-stream.js @@ -0,0 +1,119 @@ +'use strict'; +const common = require('../common'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); +} + +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const https = require('https'); +const http = require('http'); +const tls = require('tls'); +const MakeDuplexPair = require('../common/duplexpair'); +const { finished } = require('stream'); + +const certFixture = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + ca: fixtures.readKey('ca1-cert.pem'), +}; + + +// Test that setting the `maxHeaderSize` option works on a per-stream-basis. + +// Test 1: The server sends larger headers than what would otherwise be allowed. +{ + const { clientSide, serverSide } = MakeDuplexPair(); + + const req = https.request({ + createConnection: common.mustCall(() => clientSide), + maxHeaderSize: http.maxHeaderSize * 4 + }, common.mustCall((res) => { + assert.strictEqual(res.headers.hello, 'A'.repeat(http.maxHeaderSize * 3)); + res.resume(); // We don’t actually care about contents. + res.on('end', common.mustCall()); + })); + req.end(); + + serverSide.resume(); // Dump the request + serverSide.end('HTTP/1.1 200 OK\r\n' + + 'Hello: ' + 'A'.repeat(http.maxHeaderSize * 3) + '\r\n' + + 'Content-Length: 0\r\n' + + '\r\n\r\n'); +} + +// Test 2: The same as Test 1 except without the option, to make sure it fails. +{ + const { clientSide, serverSide } = MakeDuplexPair(); + + const req = https.request({ + createConnection: common.mustCall(() => clientSide) + }, common.mustNotCall()); + req.end(); + req.on('error', common.mustCall()); + + serverSide.resume(); // Dump the request + serverSide.end('HTTP/1.1 200 OK\r\n' + + 'Hello: ' + 'A'.repeat(http.maxHeaderSize * 3) + '\r\n' + + 'Content-Length: 0\r\n' + + '\r\n\r\n'); +} + +// Test 3: The client sends larger headers than what would otherwise be allowed. +{ + const testData = 'Hello, World!\n'; + const server = https.createServer( + { maxHeaderSize: http.maxHeaderSize * 4, + ...certFixture }, + common.mustCall((req, res) => { + res.statusCode = 200; + res.setHeader('Content-Type', 'text/plain'); + res.end(testData); + })); + + server.on('clientError', common.mustNotCall()); + + server.listen(0, common.mustCall(() => { + const client = tls.connect({ + port: server.address().port, + rejectUnauthorized: false + }); + client.write( + 'GET / HTTP/1.1\r\n' + + 'Hello: ' + 'A'.repeat(http.maxHeaderSize * 3) + '\r\n' + + '\r\n\r\n'); + client.end(); + + client.on('data', () => {}); + finished(client, common.mustCall(() => { + server.close(); + })); + })); +} + +// Test 4: The same as Test 3 except without the option, to make sure it fails. +{ + const server = https.createServer({ ...certFixture }, common.mustNotCall()); + + // clientError may be emitted multiple times when header is larger than + // maxHeaderSize. + server.on('clientError', common.mustCallAtLeast(() => {}, 1)); + + server.listen(0, common.mustCall(() => { + const client = tls.connect({ + port: server.address().port, + rejectUnauthorized: false + }); + client.write( + 'GET / HTTP/1.1\r\n' + + 'Hello: ' + 'A'.repeat(http.maxHeaderSize * 3) + '\r\n' + + '\r\n\r\n'); + client.end(); + + client.on('data', () => {}); + finished(client, common.mustCall(() => { + server.close(); + })); + })); +}