http2: throw from mapToHeaders

PR-URL: #24063 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Note: Landed with one collaborator approval after PR was open for 18 days
nodejs · Nov 28, 2018 · cd7df56 · cd7df56
1 parent 1a74fad
commit cd7df56
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 56 deletions.
diff --git 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
@@ -406,22 +406,22 @@ function assertValidPseudoHeader(key) {
   if (!kValidPseudoHeaders.has(key)) {
     const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
     Error.captureStackTrace(err, assertValidPseudoHeader);
-    return err;
+    throw err;
   }
 }
 
 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
@@ -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
@@ -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'] });
 
 
 {