From d0fba5564845ce7a31efbcf8e86e1914ec6d540e Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sat, 28 Oct 2017 14:04:16 -0400 Subject: [PATCH] http2: simplify mapToHeaders, stricter validation No longer check whether key is a symbol as Object.keys does not return symbols. No longer convert key to string as it is always a string. Validate that only one value is passed for each pseudo-header. Expand tests to cover this behaviour. Expand illegal connection header message to include the name of the problematic header. --- lib/internal/errors.js | 2 +- lib/internal/http2/util.js | 29 ++++++++++--------- ...st-http2-server-push-stream-errors-args.js | 2 +- test/parallel/test-http2-util-headers-list.js | 27 +++++++++++++---- 4 files changed, 38 insertions(+), 22 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 9bf0951eece120..dfeadd51baba41 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -200,7 +200,7 @@ E('ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND', E('ERR_HTTP2_INFO_STATUS_NOT_ALLOWED', 'Informational status codes cannot be used'); E('ERR_HTTP2_INVALID_CONNECTION_HEADERS', - 'HTTP/1 Connection specific headers are forbidden'); + 'HTTP/1 Connection specific headers are forbidden: "%s"'); E('ERR_HTTP2_INVALID_HEADER_VALUE', 'Value must not be undefined or null'); E('ERR_HTTP2_INVALID_INFO_STATUS', (code) => `Invalid informational status code: ${code}`); diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 013642d40d1286..4b6f8cc5c687a3 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -399,10 +399,10 @@ function mapToHeaders(map, for (var i = 0; i < keys.length; i++) { let key = keys[i]; let value = map[key]; - let val; - if (typeof key === 'symbol' || value === undefined || !key) + if (value === undefined || key === '') continue; - key = String(key).toLowerCase(); + key = key.toLowerCase(); + const isSingleValueHeader = kSingleValueHeaders.has(key); let isArray = Array.isArray(value); if (isArray) { switch (value.length) { @@ -413,34 +413,35 @@ function mapToHeaders(map, isArray = false; break; default: - if (kSingleValueHeaders.has(key)) + if (isSingleValueHeader) return new errors.Error('ERR_HTTP2_HEADER_SINGLE_VALUE', key); } + } else { + value = String(value); + } + if (isSingleValueHeader) { + if (singles.has(key)) + return new errors.Error('ERR_HTTP2_HEADER_SINGLE_VALUE', key); + singles.add(key); } if (key[0] === ':') { const err = assertValuePseudoHeader(key); if (err !== undefined) return err; - ret = `${key}\0${String(value)}\0${ret}`; + ret = `${key}\0${value}\0${ret}`; count++; } else { - if (kSingleValueHeaders.has(key)) { - if (singles.has(key)) - return new errors.Error('ERR_HTTP2_HEADER_SINGLE_VALUE', key); - singles.add(key); - } if (isIllegalConnectionSpecificHeader(key, value)) { - return new errors.Error('ERR_HTTP2_INVALID_CONNECTION_HEADERS'); + return new errors.Error('ERR_HTTP2_INVALID_CONNECTION_HEADERS', key); } if (isArray) { for (var k = 0; k < value.length; k++) { - val = String(value[k]); + const val = String(value[k]); ret += `${key}\0${val}\0`; } count += value.length; } else { - val = String(value); - ret += `${key}\0${val}\0`; + ret += `${key}\0${value}\0`; count++; } } diff --git a/test/parallel/test-http2-server-push-stream-errors-args.js b/test/parallel/test-http2-server-push-stream-errors-args.js index 5055c081b40351..73fa064b439199 100644 --- a/test/parallel/test-http2-server-push-stream-errors-args.js +++ b/test/parallel/test-http2-server-push-stream-errors-args.js @@ -31,7 +31,7 @@ server.on('stream', common.mustCall((stream, headers) => { () => stream.pushStream({ 'connection': 'test' }, {}, () => {}), { code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', - message: 'HTTP/1 Connection specific headers are forbidden' + message: 'HTTP/1 Connection specific headers are forbidden: "connection"' } ); diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js index f5e202538502eb..0bbe1972d07981 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -158,7 +158,7 @@ const { // Arrays containing a single set-cookie value are handled correctly // (https://github.com/nodejs/node/issues/16452) const headers = { - 'set-cookie': 'foo=bar' + 'set-cookie': ['foo=bar'] }; assert.deepStrictEqual( mapToHeaders(headers), @@ -166,6 +166,20 @@ const { ); } +{ + // pseudo-headers are only allowed a single value + const headers = { + ':status': 200, + ':statuS': 204, + }; + + common.expectsError({ + code: 'ERR_HTTP2_HEADER_SINGLE_VALUE', + type: Error, + message: 'Header field ":status" must have only a single value' + })(mapToHeaders(headers)); +} + // The following are not allowed to have multiple values [ HTTP2_HEADER_STATUS, @@ -248,8 +262,6 @@ const { assert(!(mapToHeaders({ [name]: [1, 2, 3] }) instanceof Error), name); }); -const regex = - /^HTTP\/1 Connection specific headers are forbidden$/; [ HTTP2_HEADER_CONNECTION, HTTP2_HEADER_UPGRADE, @@ -269,18 +281,21 @@ const regex = ].forEach((name) => { common.expectsError({ code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', - message: regex + message: 'HTTP/1 Connection specific headers are forbidden: ' + + `"${name.toLowerCase()}"` })(mapToHeaders({ [name]: 'abc' })); }); common.expectsError({ code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', - message: regex + message: 'HTTP/1 Connection specific headers are forbidden: ' + + `"${HTTP2_HEADER_TE}"` })(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc'] })); common.expectsError({ code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', - message: regex + message: 'HTTP/1 Connection specific headers are forbidden: ' + + `"${HTTP2_HEADER_TE}"` })(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc', 'trailers'] })); assert(!(mapToHeaders({ te: 'trailers' }) instanceof Error));