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.

Extend illegal connection header message to include the name of
the problematic header.

Extend tests to cover this behaviour.

PR-URL: https://github.com/nodejs/node/pull/16575
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

4 files changed, 38 insertions, 22 deletions

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index a01d9b30cd..5fe33dafae 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -201,7 +201,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 013642d40d..4b6f8cc5c6 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 5055c081b4..73fa064b43 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 f5e2025385..0bbe1972d0 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));