From e94d16daf23bf471ac438860216bfa4c92c86525 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 3 Nov 2018 10:27:18 -0700 Subject: [PATCH] http2: throw from mapToHeaders PR-URL: https://github.com/nodejs/node/pull/24063 Reviewed-By: Matteo Collina Note: Landed with one collaborator approval after PR was open for 18 days --- lib/internal/http2/core.js | 20 ++++------- lib/internal/http2/util.js | 14 ++++---- ...st-http2-util-assert-valid-pseudoheader.js | 30 ++++++---------- test/parallel/test-http2-util-headers-list.js | 34 ++++++++++--------- 4 files changed, 42 insertions(+), 56 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index ecf243f845b6d5..53a141a9632723 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1484,8 +1484,6 @@ class ClientHttp2Session extends Http2Session { } const headersList = mapToHeaders(headers); - if (!Array.isArray(headersList)) - throw headersList; const stream = new ClientHttp2Stream(this, undefined, undefined, {}); stream[kSentHeaders] = headers; @@ -1890,8 +1888,6 @@ class Http2Stream extends Duplex { this[kUpdateTimer](); const headersList = mapToHeaders(headers, assertValidPseudoHeaderTrailer); - if (!Array.isArray(headersList)) - throw headersList; this[kSentTrailers] = headers; // Send the trailers in setImmediate so we don't do it on nghttp2 stack. @@ -2058,12 +2054,14 @@ function processRespondWithFD(self, fd, headers, offset = 0, length = -1, const state = self[kState]; state.flags |= STREAM_FLAGS_HEADERS_SENT; - const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); - self[kSentHeaders] = headers; - if (!Array.isArray(headersList)) { - self.destroy(headersList); + let headersList; + try { + headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); + } catch (err) { + self.destroy(err); return; } + self[kSentHeaders] = headers; // Close the writable side of the stream, but only as far as the writable // stream implementation is concerned. @@ -2287,8 +2285,6 @@ class ServerHttp2Stream extends Http2Stream { options.readable = false; const headersList = mapToHeaders(headers); - if (!Array.isArray(headersList)) - throw headersList; const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0; @@ -2365,8 +2361,6 @@ class ServerHttp2Stream extends Http2Stream { } const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); - if (!Array.isArray(headersList)) - throw headersList; this[kSentHeaders] = headers; state.flags |= STREAM_FLAGS_HEADERS_SENT; @@ -2527,8 +2521,6 @@ class ServerHttp2Stream extends Http2Stream { this[kUpdateTimer](); const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); - if (!Array.isArray(headersList)) - throw headersList; if (!this[kInfoHeaders]) this[kInfoHeaders] = [headers]; else diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index c8701af616f327..9dc8be6e83ddeb 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -406,7 +406,7 @@ function assertValidPseudoHeader(key) { if (!kValidPseudoHeaders.has(key)) { const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key); Error.captureStackTrace(err, assertValidPseudoHeader); - return err; + throw err; } } @@ -414,14 +414,14 @@ function assertValidPseudoHeaderResponse(key) { if (key !== ':status') { const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key); Error.captureStackTrace(err, assertValidPseudoHeaderResponse); - return err; + throw err; } } function assertValidPseudoHeaderTrailer(key) { const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key); Error.captureStackTrace(err, assertValidPseudoHeaderTrailer); - return err; + throw err; } function mapToHeaders(map, @@ -454,26 +454,26 @@ function mapToHeaders(map, break; default: if (isSingleValueHeader) - return new ERR_HTTP2_HEADER_SINGLE_VALUE(key); + throw new ERR_HTTP2_HEADER_SINGLE_VALUE(key); } } else { value = String(value); } if (isSingleValueHeader) { if (singles.has(key)) - return new ERR_HTTP2_HEADER_SINGLE_VALUE(key); + throw new ERR_HTTP2_HEADER_SINGLE_VALUE(key); singles.add(key); } if (key[0] === ':') { err = assertValuePseudoHeader(key); if (err !== undefined) - return err; + throw err; ret = `${key}\0${value}\0${ret}`; count++; continue; } if (isIllegalConnectionSpecificHeader(key, value)) { - return new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key); + throw new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key); } if (isArray) { for (var k = 0; k < value.length; k++) { diff --git a/test/parallel/test-http2-util-assert-valid-pseudoheader.js b/test/parallel/test-http2-util-assert-valid-pseudoheader.js index badd7442ada5b7..b0d7ac0e58f1d5 100644 --- a/test/parallel/test-http2-util-assert-valid-pseudoheader.js +++ b/test/parallel/test-http2-util-assert-valid-pseudoheader.js @@ -8,24 +8,16 @@ const common = require('../common'); // have to test it through mapToHeaders const { mapToHeaders } = require('internal/http2/util'); -const assert = require('assert'); -function isNotError(val) { - assert(!(val instanceof Error)); -} +// These should not throw +mapToHeaders({ ':status': 'a' }); +mapToHeaders({ ':path': 'a' }); +mapToHeaders({ ':authority': 'a' }); +mapToHeaders({ ':scheme': 'a' }); +mapToHeaders({ ':method': 'a' }); -function isError(val) { - common.expectsError({ - code: 'ERR_HTTP2_INVALID_PSEUDOHEADER', - type: TypeError, - message: '":foo" is an invalid pseudoheader or is used incorrectly' - })(val); -} - -isNotError(mapToHeaders({ ':status': 'a' })); -isNotError(mapToHeaders({ ':path': 'a' })); -isNotError(mapToHeaders({ ':authority': 'a' })); -isNotError(mapToHeaders({ ':scheme': 'a' })); -isNotError(mapToHeaders({ ':method': 'a' })); - -isError(mapToHeaders({ ':foo': 'a' })); +common.expectsError(() => mapToHeaders({ ':foo': 'a' }), { + code: 'ERR_HTTP2_INVALID_PSEUDOHEADER', + type: TypeError, + message: '":foo" is an invalid pseudoheader or is used incorrectly' +}); diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js index 2402e01cbf329e..2814613df2c770 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -175,11 +175,11 @@ const { ':statuS': 204, }; - common.expectsError({ + common.expectsError(() => mapToHeaders(headers), { code: 'ERR_HTTP2_HEADER_SINGLE_VALUE', type: TypeError, message: 'Header field ":status" must only have a single value' - })(mapToHeaders(headers)); + }); } // The following are not allowed to have multiple values @@ -224,10 +224,10 @@ const { HTTP2_HEADER_X_CONTENT_TYPE_OPTIONS ].forEach((name) => { const msg = `Header field "${name}" must only have a single value`; - common.expectsError({ + common.expectsError(() => mapToHeaders({ [name]: [1, 2, 3] }), { code: 'ERR_HTTP2_HEADER_SINGLE_VALUE', message: msg - })(mapToHeaders({ [name]: [1, 2, 3] })); + }); }); [ @@ -281,30 +281,32 @@ const { 'Proxy-Connection', 'Keep-Alive' ].forEach((name) => { - common.expectsError({ + common.expectsError(() => mapToHeaders({ [name]: 'abc' }), { code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]', message: 'HTTP/1 Connection specific headers are forbidden: ' + `"${name.toLowerCase()}"` - })(mapToHeaders({ [name]: 'abc' })); + }); }); -common.expectsError({ +common.expectsError(() => mapToHeaders({ [HTTP2_HEADER_TE]: ['abc'] }), { code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]', message: 'HTTP/1 Connection specific headers are forbidden: ' + `"${HTTP2_HEADER_TE}"` -})(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc'] })); +}); -common.expectsError({ - code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', - name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]', - message: 'HTTP/1 Connection specific headers are forbidden: ' + - `"${HTTP2_HEADER_TE}"` -})(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc', 'trailers'] })); +common.expectsError( + () => mapToHeaders({ [HTTP2_HEADER_TE]: ['abc', 'trailers'] }), { + code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', + name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]', + message: 'HTTP/1 Connection specific headers are forbidden: ' + + `"${HTTP2_HEADER_TE}"` + }); -assert(!(mapToHeaders({ te: 'trailers' }) instanceof Error)); -assert(!(mapToHeaders({ te: ['trailers'] }) instanceof Error)); +// These should not throw +mapToHeaders({ te: 'trailers' }); +mapToHeaders({ te: ['trailers'] }); {