From 388915a89f82135e2f3736991eb19159eca3b267 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Fri, 1 May 2020 16:49:02 +0530 Subject: [PATCH 1/6] http2: header field valid checks checks validity for request, writeHead, and setHeader methods refs: https://github.com/nodejs/node/issues/29829 --- doc/api/errors.md | 5 +++ lib/internal/errors.js | 2 ++ lib/internal/http2/compat.js | 14 ++++++++- lib/internal/http2/core.js | 14 ++++++++- lib/internal/http2/util.js | 1 + .../test-http2-invalidheaderfields-client.js | 31 +++++++++++++++++++ 6 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-invalidheaderfields-client.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 172eadf7686794..0d2701d3377780 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1047,6 +1047,11 @@ An invalid value has been specified for an HTTP/2 setting. An operation was performed on a stream that had already been destroyed. + +### `ERR_HTTP2_INVALID_TOKEN` + +An invalid HTTP/2 header was specified. + ### `ERR_HTTP2_MAX_PENDING_SETTINGS_ACK` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 2a162f14b24206..648f325bd5f918 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -861,6 +861,8 @@ E('ERR_HTTP2_INVALID_SETTING_VALUE', return `Invalid value for setting "${name}": ${actual}`; }, TypeError, RangeError); E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed', Error); +E('ERR_HTTP2_INVALID_TOKEN', '%s must be a valid HTTP2 token ["%s"]', + TypeError); E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK', 'Maximum number of pending settings acknowledgements', Error); E('ERR_HTTP2_NESTED_PUSH', diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 3abe9ba2ac6f82..fa01d62c5ecb41 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -35,6 +35,7 @@ const { ERR_HTTP2_INFO_STATUS_NOT_ALLOWED, ERR_HTTP2_INVALID_HEADER_VALUE, ERR_HTTP2_INVALID_STREAM, + ERR_HTTP2_INVALID_TOKEN, ERR_HTTP2_NO_SOCKET_MANIPULATION, ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED, ERR_HTTP2_STATUS_INVALID, @@ -46,7 +47,13 @@ const { hideStackFrames } = require('internal/errors'); const { validateString } = require('internal/validators'); -const { kSocket, kRequest, kProxySocket } = require('internal/http2/util'); +const { + kSocket, + kRequest, + kProxySocket, + assertValidPseudoHeader +} = require('internal/http2/util'); +const { _checkIsHttpToken: checkIsHttpToken } = require('_http_common'); const kBeginSend = Symbol('begin-send'); const kState = Symbol('state'); @@ -586,6 +593,11 @@ class Http2ServerResponse extends Stream { return; } + if (name[0] === ':') + assertValidPseudoHeader(name); + else if (!checkIsHttpToken(name)) + this.destroy(new ERR_HTTP2_INVALID_TOKEN('Header name', name)); + this[kHeaders][name] = value; } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index ef5d61843448c6..7abdfa0e640246 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -35,7 +35,10 @@ const tls = require('tls'); const { URL } = require('url'); const { setImmediate } = require('timers'); -const { kIncomingMessage } = require('_http_common'); +const { + kIncomingMessage, + _checkIsHttpToken: checkIsHttpToken +} = require('_http_common'); const { kServerResponse } = require('_http_server'); const JSStreamSocket = require('internal/js_stream_socket'); @@ -62,6 +65,7 @@ const { ERR_HTTP2_INVALID_SESSION, ERR_HTTP2_INVALID_SETTING_VALUE, ERR_HTTP2_INVALID_STREAM, + ERR_HTTP2_INVALID_TOKEN, ERR_HTTP2_MAX_PENDING_SETTINGS_ACK, ERR_HTTP2_NESTED_PUSH, ERR_HTTP2_NO_SOCKET_MANIPULATION, @@ -108,6 +112,7 @@ const { onServerStream, const { assertIsObject, + assertValidPseudoHeader, assertValidPseudoHeaderResponse, assertValidPseudoHeaderTrailer, assertWithinRange, @@ -1622,6 +1627,13 @@ class ClientHttp2Session extends Http2Session { this[kUpdateTimer](); + for (const i in headers) { + if (i[0] === ':') { + assertValidPseudoHeader(i); + } else if (i && !checkIsHttpToken(i)) + this.destroy(new ERR_HTTP2_INVALID_TOKEN('Header name', i)); + } + assertIsObject(headers, 'headers'); assertIsObject(options, 'options'); diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index dcc1355a3230fd..9f3d0e9d9b25bc 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -595,6 +595,7 @@ function sessionName(type) { module.exports = { assertIsObject, + assertValidPseudoHeader, assertValidPseudoHeaderResponse, assertValidPseudoHeaderTrailer, assertWithinRange, diff --git a/test/parallel/test-http2-invalidheaderfields-client.js b/test/parallel/test-http2-invalidheaderfields-client.js new file mode 100644 index 00000000000000..6a90b91910a0ee --- /dev/null +++ b/test/parallel/test-http2-invalidheaderfields-client.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) { common.skip('missing crypto'); } +const assert = require('assert'); +const http2 = require('http2'); + +const server1 = http2.createServer(); + +server1.listen(0, common.mustCall(() => { + const session = http2.connect(`http://localhost:${server1.address().port}`); + session.request({ 'no underscore': 123 }); + session.on('error', common.mustCall((e) => { + assert.strictEqual(e.code, 'ERR_HTTP2_INVALID_TOKEN'); + server1.close(); + })); +})); + +const server2 = http2.createServer(common.mustCall((req, res) => { + res.setHeader('x y z', 123); + res.end(); +})); + +server2.listen(0, common.mustCall(() => { + const session = http2.connect(`http://localhost:${server2.address().port}`); + const req = session.request(); + req.on('error', common.mustCall((e) => { + assert.strictEqual(e.code, 'ERR_HTTP2_STREAM_ERROR'); + session.close(); + server2.close(); + })); +})); From d092e96b9f5eb5b53bc272eff2ac3bce8a3e5b76 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sat, 2 May 2020 12:32:41 +0530 Subject: [PATCH 2/6] fixup: remove unnecessary error --- doc/api/errors.md | 5 ----- lib/internal/errors.js | 2 -- lib/internal/http2/compat.js | 3 +-- lib/internal/http2/core.js | 4 ++-- test/parallel/test-http2-invalidheaderfields-client.js | 2 +- 5 files changed, 4 insertions(+), 12 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 0d2701d3377780..172eadf7686794 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1047,11 +1047,6 @@ An invalid value has been specified for an HTTP/2 setting. An operation was performed on a stream that had already been destroyed. - -### `ERR_HTTP2_INVALID_TOKEN` - -An invalid HTTP/2 header was specified. - ### `ERR_HTTP2_MAX_PENDING_SETTINGS_ACK` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 648f325bd5f918..2a162f14b24206 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -861,8 +861,6 @@ E('ERR_HTTP2_INVALID_SETTING_VALUE', return `Invalid value for setting "${name}": ${actual}`; }, TypeError, RangeError); E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed', Error); -E('ERR_HTTP2_INVALID_TOKEN', '%s must be a valid HTTP2 token ["%s"]', - TypeError); E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK', 'Maximum number of pending settings acknowledgements', Error); E('ERR_HTTP2_NESTED_PUSH', diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index fa01d62c5ecb41..f36705f6512808 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -35,7 +35,6 @@ const { ERR_HTTP2_INFO_STATUS_NOT_ALLOWED, ERR_HTTP2_INVALID_HEADER_VALUE, ERR_HTTP2_INVALID_STREAM, - ERR_HTTP2_INVALID_TOKEN, ERR_HTTP2_NO_SOCKET_MANIPULATION, ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED, ERR_HTTP2_STATUS_INVALID, @@ -596,7 +595,7 @@ class Http2ServerResponse extends Stream { if (name[0] === ':') assertValidPseudoHeader(name); else if (!checkIsHttpToken(name)) - this.destroy(new ERR_HTTP2_INVALID_TOKEN('Header name', name)); + this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', name)); this[kHeaders][name] = value; } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 7abdfa0e640246..941cf9a344dcc9 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -65,7 +65,6 @@ const { ERR_HTTP2_INVALID_SESSION, ERR_HTTP2_INVALID_SETTING_VALUE, ERR_HTTP2_INVALID_STREAM, - ERR_HTTP2_INVALID_TOKEN, ERR_HTTP2_MAX_PENDING_SETTINGS_ACK, ERR_HTTP2_NESTED_PUSH, ERR_HTTP2_NO_SOCKET_MANIPULATION, @@ -92,6 +91,7 @@ const { ERR_INVALID_ARG_TYPE, ERR_INVALID_CALLBACK, ERR_INVALID_CHAR, + ERR_INVALID_HTTP_TOKEN, ERR_INVALID_OPT_VALUE, ERR_OUT_OF_RANGE, ERR_SOCKET_CLOSED @@ -1631,7 +1631,7 @@ class ClientHttp2Session extends Http2Session { if (i[0] === ':') { assertValidPseudoHeader(i); } else if (i && !checkIsHttpToken(i)) - this.destroy(new ERR_HTTP2_INVALID_TOKEN('Header name', i)); + this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', i)); } assertIsObject(headers, 'headers'); diff --git a/test/parallel/test-http2-invalidheaderfields-client.js b/test/parallel/test-http2-invalidheaderfields-client.js index 6a90b91910a0ee..25985f9716f83f 100644 --- a/test/parallel/test-http2-invalidheaderfields-client.js +++ b/test/parallel/test-http2-invalidheaderfields-client.js @@ -10,7 +10,7 @@ server1.listen(0, common.mustCall(() => { const session = http2.connect(`http://localhost:${server1.address().port}`); session.request({ 'no underscore': 123 }); session.on('error', common.mustCall((e) => { - assert.strictEqual(e.code, 'ERR_HTTP2_INVALID_TOKEN'); + assert.strictEqual(e.code, 'ERR_INVALID_HTTP_TOKEN'); server1.close(); })); })); From a0575e048d0b03e16e4696df23a185d33f2a06ed Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Mon, 11 May 2020 01:34:38 +0530 Subject: [PATCH 3/6] test: add checks for writeHead --- .../test-http2-invalidheaderfields-client.js | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/parallel/test-http2-invalidheaderfields-client.js b/test/parallel/test-http2-invalidheaderfields-client.js index 25985f9716f83f..7b846597c44331 100644 --- a/test/parallel/test-http2-invalidheaderfields-client.js +++ b/test/parallel/test-http2-invalidheaderfields-client.js @@ -8,6 +8,7 @@ const server1 = http2.createServer(); server1.listen(0, common.mustCall(() => { const session = http2.connect(`http://localhost:${server1.address().port}`); + // check for req headers session.request({ 'no underscore': 123 }); session.on('error', common.mustCall((e) => { assert.strictEqual(e.code, 'ERR_INVALID_HTTP_TOKEN'); @@ -16,6 +17,7 @@ server1.listen(0, common.mustCall(() => { })); const server2 = http2.createServer(common.mustCall((req, res) => { + // check for setHeader res.setHeader('x y z', 123); res.end(); })); @@ -29,3 +31,25 @@ server2.listen(0, common.mustCall(() => { server2.close(); })); })); + +const server3 = http2.createServer(common.mustCall((req, res) => { + // check for writeHead + assert.throws(common.mustCall(()=>{ + res.writeHead(200, { + 'an invalid header': 123 + }); + }), { + code: 'ERR_HTTP2_INVALID_STREAM' + }); + res.end(); +})); + +server3.listen(0, common.mustCall(() => { + const session = http2.connect(`http://localhost:${server3.address().port}`); + const req = session.request(); + req.on('error', common.mustCall((e) => { + assert.strictEqual(e.code, 'ERR_HTTP2_STREAM_ERROR'); + server3.close(); + session.close(); + })); +})); \ No newline at end of file From 502c54963d15552b95457dad209ad5420bc4a432 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Mon, 11 May 2020 01:38:39 +0530 Subject: [PATCH 4/6] fixup: lint --- test/parallel/test-http2-invalidheaderfields-client.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-http2-invalidheaderfields-client.js b/test/parallel/test-http2-invalidheaderfields-client.js index 7b846597c44331..90a3ea4622fccc 100644 --- a/test/parallel/test-http2-invalidheaderfields-client.js +++ b/test/parallel/test-http2-invalidheaderfields-client.js @@ -8,7 +8,7 @@ const server1 = http2.createServer(); server1.listen(0, common.mustCall(() => { const session = http2.connect(`http://localhost:${server1.address().port}`); - // check for req headers + // Check for req headers session.request({ 'no underscore': 123 }); session.on('error', common.mustCall((e) => { assert.strictEqual(e.code, 'ERR_INVALID_HTTP_TOKEN'); @@ -34,7 +34,7 @@ server2.listen(0, common.mustCall(() => { const server3 = http2.createServer(common.mustCall((req, res) => { // check for writeHead - assert.throws(common.mustCall(()=>{ + assert.throws(common.mustCall(() => { res.writeHead(200, { 'an invalid header': 123 }); @@ -52,4 +52,4 @@ server3.listen(0, common.mustCall(() => { server3.close(); session.close(); })); -})); \ No newline at end of file +})); From 49f5092c9ccbb7892f4a810d1e9f4c38bef3f8b6 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sun, 24 May 2020 02:08:44 +0530 Subject: [PATCH 5/6] Update lib/internal/http2/core.js Co-authored-by: Ruben Bridgewater --- lib/internal/http2/core.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 941cf9a344dcc9..f3c2e1d0ede2e8 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1627,11 +1627,11 @@ class ClientHttp2Session extends Http2Session { this[kUpdateTimer](); - for (const i in headers) { - if (i[0] === ':') { - assertValidPseudoHeader(i); - } else if (i && !checkIsHttpToken(i)) - this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', i)); + for (const header of Object.keys(headers)) { + if (header[0] === ':') { + assertValidPseudoHeader(header); + } else if (header && !checkIsHttpToken(header)) + this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', header)); } assertIsObject(headers, 'headers'); From 32689ba21fec12df7ad043465682f0b7f2c6ac15 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Sun, 24 May 2020 03:58:14 +0530 Subject: [PATCH 6/6] fixup: review suggestions --- lib/internal/http2/core.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index f3c2e1d0ede2e8..941df2bfb6b767 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -9,6 +9,7 @@ const { MathMin, ObjectAssign, ObjectCreate, + ObjectKeys, ObjectDefineProperty, ObjectPrototypeHasOwnProperty, Promise, @@ -1627,11 +1628,13 @@ class ClientHttp2Session extends Http2Session { this[kUpdateTimer](); - for (const header of Object.keys(headers)) { - if (header[0] === ':') { - assertValidPseudoHeader(header); - } else if (header && !checkIsHttpToken(header)) - this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', header)); + if (headers !== null && headers !== undefined) { + for (const header of ObjectKeys(headers)) { + if (header[0] === ':') { + assertValidPseudoHeader(header); + } else if (header && !checkIsHttpToken(header)) + this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', header)); + } } assertIsObject(headers, 'headers');