http2: add compat trailers, adjust multi-headers

Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.
End readable side of the stream on trailers or bodyless requests

Refs: https://github.com/expressjs/express/pull/3390#discussion_r136718729
PR-URL: https://github.com/nodejs/node/pull/15193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

10 files changed, 298 insertions, 49 deletions

diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
index cb4ba5f5d0..c959824d82 100644
--- a/lib/internal/http2/compat.js
+++ b/lib/internal/http2/compat.js
@@ -13,7 +13,9 @@ const kStream = Symbol('stream');
 const kRequest = Symbol('request');
 const kResponse = Symbol('response');
 const kHeaders = Symbol('headers');
+const kRawHeaders = Symbol('rawHeaders');
 const kTrailers = Symbol('trailers');
+const kRawTrailers = Symbol('rawTrailers');
 
 let statusMessageWarned = false;
 
@@ -45,12 +47,28 @@ function isPseudoHeader(name) {
   }
 }
 
+function statusMessageWarn() {
+  if (statusMessageWarned === false) {
+    process.emitWarning(
+      'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
+      'UnsupportedWarning'
+    );
+    statusMessageWarned = true;
+  }
+}
+
 function onStreamData(chunk) {
   const request = this[kRequest];
   if (!request.push(chunk))
     this.pause();
 }
 
+function onStreamTrailers(trailers, flags, rawTrailers) {
+  const request = this[kRequest];
+  Object.assign(request[kTrailers], trailers);
+  request[kRawTrailers].push(...rawTrailers);
+}
+
 function onStreamEnd() {
   // Cause the request stream to end as well.
   const request = this[kRequest];
@@ -106,7 +124,7 @@ function onAborted(hadError, code) {
 }
 
 class Http2ServerRequest extends Readable {
-  constructor(stream, headers, options) {
+  constructor(stream, headers, options, rawHeaders) {
     super(options);
     this[kState] = {
       statusCode: null,
@@ -114,12 +132,16 @@ class Http2ServerRequest extends Readable {
       closedCode: constants.NGHTTP2_NO_ERROR
     };
     this[kHeaders] = headers;
+    this[kRawHeaders] = rawHeaders;
+    this[kTrailers] = {};
+    this[kRawTrailers] = [];
     this[kStream] = stream;
     stream[kRequest] = this;
 
     // Pause the stream..
     stream.pause();
     stream.on('data', onStreamData);
+    stream.on('trailers', onStreamTrailers);
     stream.on('end', onStreamEnd);
     stream.on('error', onStreamError);
     stream.on('close', onStreamClosedRequest);
@@ -155,18 +177,17 @@ class Http2ServerRequest extends Readable {
   }
 
   get rawHeaders() {
-    const headers = this[kHeaders];
-    if (headers === undefined)
-      return [];
-    const tuples = Object.entries(headers);
-    const flattened = Array.prototype.concat.apply([], tuples);
-    return flattened.map(String);
+    return this[kRawHeaders];
   }
 
   get trailers() {
     return this[kTrailers];
   }
 
+  get rawTrailers() {
+    return this[kRawTrailers];
+  }
+
   get httpVersionMajor() {
     return 2;
   }
@@ -382,30 +403,25 @@ class Http2ServerResponse extends Stream {
   }
 
   get statusMessage() {
-    if (statusMessageWarned === false) {
-      process.emitWarning(
-        'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
-        'UnsupportedWarning'
-      );
-      statusMessageWarned = true;
-    }
+    statusMessageWarn();
 
     return '';
   }
 
+  set statusMessage(msg) {
+    statusMessageWarn();
+  }
+
   flushHeaders() {
     if (this[kStream].headersSent === false)
       this[kBeginSend]();
   }
 
   writeHead(statusCode, statusMessage, headers) {
-    if (typeof statusMessage === 'string' && statusMessageWarned === false) {
-      process.emitWarning(
-        'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
-        'UnsupportedWarning'
-      );
-      statusMessageWarned = true;
+    if (typeof statusMessage === 'string') {
+      statusMessageWarn();
     }
+
     if (headers === undefined && typeof statusMessage === 'object') {
       headers = statusMessage;
     }
@@ -542,9 +558,10 @@ class Http2ServerResponse extends Stream {
   }
 }
 
-function onServerStream(stream, headers, flags) {
+function onServerStream(stream, headers, flags, rawHeaders) {
   const server = this;
-  const request = new Http2ServerRequest(stream, headers);
+  const request = new Http2ServerRequest(stream, headers, undefined,
+                                         rawHeaders);
   const response = new Http2ServerResponse(stream);
 
   // Check for the CONNECT method
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 479d0d36f6..1ca1a89c69 100755
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -185,7 +185,7 @@ function onSessionHeaders(id, cat, flags, headers) {
                     'report this as a bug in Node.js');
     }
     streams.set(id, stream);
-    process.nextTick(emit.bind(owner, 'stream', stream, obj, flags));
+    process.nextTick(emit.bind(owner, 'stream', stream, obj, flags, headers));
   } else {
     let event;
     let status;
@@ -218,7 +218,10 @@ function onSessionHeaders(id, cat, flags, headers) {
                     'report this as a bug in Node.js');
     }
     debug(`[${sessionName(owner[kType])}] emitting stream '${event}' event`);
-    process.nextTick(emit.bind(stream, event, obj, flags));
+    process.nextTick(emit.bind(stream, event, obj, flags, headers));
+  }
+  if (endOfStream) {
+    stream.push(null);
   }
 }
 
@@ -2271,9 +2274,9 @@ function socketOnTimeout() {
 
 // Handles the on('stream') event for a session and forwards
 // it on to the server object.
-function sessionOnStream(stream, headers, flags) {
+function sessionOnStream(stream, headers, flags, rawHeaders) {
   debug(`[${sessionName(this[kType])}] emit server stream event`);
-  this[kServer].emit('stream', stream, headers, flags);
+  this[kServer].emit('stream', stream, headers, flags, rawHeaders);
 }
 
 function sessionOnPriority(stream, parent, weight, exclusive) {
diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js
index 09f55fdc65..17f4c22252 100644
--- a/lib/internal/http2/util.js
+++ b/lib/internal/http2/util.js
@@ -35,6 +35,7 @@ const {
   HTTP2_HEADER_RANGE,
   HTTP2_HEADER_REFERER,
   HTTP2_HEADER_RETRY_AFTER,
+  HTTP2_HEADER_SET_COOKIE,
   HTTP2_HEADER_USER_AGENT,
 
   HTTP2_HEADER_CONNECTION,
@@ -474,18 +475,36 @@ function toHeaderObject(headers) {
     if (existing === undefined) {
       obj[name] = value;
     } else if (!kSingleValueHeaders.has(name)) {
-      if (name === HTTP2_HEADER_COOKIE) {
-        // https://tools.ietf.org/html/rfc7540#section-8.1.2.5
-        // "...If there are multiple Cookie header fields after decompression,
-        //  these MUST be concatenated into a single octet string using the
-        //  two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before
-        //  being passed into a non-HTTP/2 context."
-        obj[name] = `${existing}; ${value}`;
-      } else {
-        if (Array.isArray(existing))
-          existing.push(value);
-        else
-          obj[name] = [existing, value];
+      switch (name) {
+        case HTTP2_HEADER_COOKIE:
+          // https://tools.ietf.org/html/rfc7540#section-8.1.2.5
+          // "...If there are multiple Cookie header fields after decompression,
+          //  these MUST be concatenated into a single octet string using the
+          //  two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before
+          //  being passed into a non-HTTP/2 context."
+          obj[name] = `${existing}; ${value}`;
+          break;
+        case HTTP2_HEADER_SET_COOKIE:
+          // https://tools.ietf.org/html/rfc7230#section-3.2.2
+          // "Note: In practice, the "Set-Cookie" header field ([RFC6265]) often
+          // appears multiple times in a response message and does not use the
+          // list syntax, violating the above requirements on multiple header
+          // fields with the same name.  Since it cannot be combined into a
+          // single field-value, recipients ought to handle "Set-Cookie" as a
+          // special case while processing header fields."
+          if (Array.isArray(existing))
+            existing.push(value);
+          else
+            obj[name] = [existing, value];
+          break;
+        default:
+          // https://tools.ietf.org/html/rfc7230#section-3.2.2
+          // "A recipient MAY combine multiple header fields with the same field
+          // name into one "field-name: field-value" pair, without changing the
+          // semantics of the message, by appending each subsequent field value
+          // to the combined field value in order, separated by a comma."
+          obj[name] = `${existing}, ${value}`;
+          break;
       }
     }
   }
diff --git a/test/parallel/test-http2-compat-serverrequest-end.js b/test/parallel/test-http2-compat-serverrequest-end.js
new file mode 100644
index 0000000000..a84f73883d
--- /dev/null
+++ b/test/parallel/test-http2-compat-serverrequest-end.js
@@ -0,0 +1,40 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const h2 = require('http2');
+
+// Http2ServerRequest should always end readable stream
+// even on GET requests with no body
+
+const server = h2.createServer();
+server.listen(0, common.mustCall(function() {
+  const port = server.address().port;
+  server.once('request', common.mustCall(function(request, response) {
+    request.on('data', () => {});
+    request.on('end', common.mustCall(() => {
+      response.on('finish', common.mustCall(function() {
+        server.close();
+      }));
+      response.end();
+    }));
+  }));
+
+  const url = `http://localhost:${port}`;
+  const client = h2.connect(url, common.mustCall(function() {
+    const headers = {
+      ':path': '/foobar',
+      ':method': 'GET',
+      ':scheme': 'http',
+      ':authority': `localhost:${port}`
+    };
+    const request = client.request(headers);
+    request.resume();
+    request.on('end', common.mustCall(function() {
+      client.destroy();
+    }));
+    request.end();
+  }));
+}));
diff --git a/test/parallel/test-http2-compat-serverrequest-trailers.js b/test/parallel/test-http2-compat-serverrequest-trailers.js
new file mode 100644
index 0000000000..7fa649fe7f
--- /dev/null
+++ b/test/parallel/test-http2-compat-serverrequest-trailers.js
@@ -0,0 +1,71 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+
+// Http2ServerRequest should have getter for trailers & rawTrailers
+
+const expectedTrailers = {
+  'x-foo': 'xOxOxOx, OxOxOxO, xOxOxOx, OxOxOxO',
+  'x-foo-test': 'test, test'
+};
+
+const server = h2.createServer();
+server.listen(0, common.mustCall(function() {
+  const port = server.address().port;
+  server.once('request', common.mustCall(function(request, response) {
+    let data = '';
+    request.setEncoding('utf8');
+    request.on('data', common.mustCall((chunk) => data += chunk));
+    request.on('end', common.mustCall(() => {
+      const trailers = request.trailers;
+      for (const [name, value] of Object.entries(expectedTrailers)) {
+        assert.strictEqual(trailers[name], value);
+      }
+      assert.deepStrictEqual([
+        'x-foo',
+        'xOxOxOx',
+        'x-foo',
+        'OxOxOxO',
+        'x-foo',
+        'xOxOxOx',
+        'x-foo',
+        'OxOxOxO',
+        'x-foo-test',
+        'test, test'
+      ], request.rawTrailers);
+      assert.strictEqual(data, 'test\ntest');
+      response.end();
+    }));
+  }));
+
+  const url = `http://localhost:${port}`;
+  const client = h2.connect(url, common.mustCall(function() {
+    const headers = {
+      ':path': '/foobar',
+      ':method': 'POST',
+      ':scheme': 'http',
+      ':authority': `localhost:${port}`
+    };
+    const request = client.request(headers, {
+      getTrailers(trailers) {
+        trailers['x-fOo'] = 'xOxOxOx';
+        trailers['x-foO'] = 'OxOxOxO';
+        trailers['X-fOo'] = 'xOxOxOx';
+        trailers['X-foO'] = 'OxOxOxO';
+        trailers['x-foo-test'] = 'test, test';
+      }
+    });
+    request.resume();
+    request.on('end', common.mustCall(function() {
+      server.close();
+      client.destroy();
+    }));
+    request.write('test\n');
+    request.end('test');
+  }));
+}));
diff --git a/test/parallel/test-http2-compat-serverresponse-statusmessage-property-set.js b/test/parallel/test-http2-compat-serverresponse-statusmessage-property-set.js
new file mode 100644
index 0000000000..bec056d013
--- /dev/null
+++ b/test/parallel/test-http2-compat-serverresponse-statusmessage-property-set.js
@@ -0,0 +1,51 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+
+// Http2ServerResponse.statusMessage should warn
+
+const unsupportedWarned = common.mustCall(1);
+process.on('warning', ({ name, message }) => {
+  const expectedMessage =
+    'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)';
+  if (name === 'UnsupportedWarning' && message === expectedMessage)
+    unsupportedWarned();
+});
+
+const server = h2.createServer();
+server.listen(0, common.mustCall(function() {
+  const port = server.address().port;
+  server.once('request', common.mustCall(function(request, response) {
+    response.on('finish', common.mustCall(function() {
+      response.statusMessage = 'test';
+      response.statusMessage = 'test'; // only warn once
+      assert.strictEqual(response.statusMessage, ''); // no change
+      server.close();
+    }));
+    response.end();
+  }));
+
+  const url = `http://localhost:${port}`;
+  const client = h2.connect(url, common.mustCall(function() {
+    const headers = {
+      ':path': '/',
+      ':method': 'GET',
+      ':scheme': 'http',
+      ':authority': `localhost:${port}`
+    };
+    const request = client.request(headers);
+    request.on('response', common.mustCall(function(headers) {
+      assert.strictEqual(headers[':status'], 200);
+    }, 1));
+    request.on('end', common.mustCall(function() {
+      client.destroy();
+    }));
+    request.end();
+    request.resume();
+  }));
+}));
diff --git a/test/parallel/test-http2-compat-serverresponse-statusmessage-property.js b/test/parallel/test-http2-compat-serverresponse-statusmessage-property.js
index 782c239f0f..3b3ef6bdc2 100644
--- a/test/parallel/test-http2-compat-serverresponse-statusmessage-property.js
+++ b/test/parallel/test-http2-compat-serverresponse-statusmessage-property.js
@@ -23,6 +23,7 @@ server.listen(0, common.mustCall(function() {
   server.once('request', common.mustCall(function(request, response) {
     response.on('finish', common.mustCall(function() {
       assert.strictEqual(response.statusMessage, '');
+      assert.strictEqual(response.statusMessage, ''); // only warn once
       server.close();
     }));
     response.end();
diff --git a/test/parallel/test-http2-cookies.js b/test/parallel/test-http2-cookies.js
index 6e2ae0bdfd..3e20a5749d 100644
--- a/test/parallel/test-http2-cookies.js
+++ b/test/parallel/test-http2-cookies.js
@@ -19,11 +19,8 @@ server.on('stream', common.mustCall(onStream));
 
 function onStream(stream, headers, flags) {
 
-  assert(Array.isArray(headers.abc));
-  assert.strictEqual(headers.abc.length, 3);
-  assert.strictEqual(headers.abc[0], '1');
-  assert.strictEqual(headers.abc[1], '2');
-  assert.strictEqual(headers.abc[2], '3');
+  assert.strictEqual(typeof headers.abc, 'string');
+  assert.strictEqual(headers.abc, '1, 2, 3');
   assert.strictEqual(typeof headers.cookie, 'string');
   assert.strictEqual(headers.cookie, 'a=b; c=d; e=f');
 
diff --git a/test/parallel/test-http2-multiheaders-raw.js b/test/parallel/test-http2-multiheaders-raw.js
new file mode 100644
index 0000000000..f9c7cc5f9b
--- /dev/null
+++ b/test/parallel/test-http2-multiheaders-raw.js
@@ -0,0 +1,50 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+
+const server = http2.createServer();
+
+const src = Object.create(null);
+src['www-authenticate'] = 'foo';
+src['WWW-Authenticate'] = 'bar';
+src['WWW-AUTHENTICATE'] = 'baz';
+src['test'] = 'foo, bar, baz';
+
+server.on('stream', common.mustCall((stream, headers, flags, rawHeaders) => {
+  const expected = [
+    ':path',
+    '/',
+    ':scheme',
+    'http',
+    ':authority',
+    `localhost:${server.address().port}`,
+    ':method',
+    'GET',
+    'www-authenticate',
+    'foo',
+    'www-authenticate',
+    'bar',
+    'www-authenticate',
+    'baz',
+    'test',
+    'foo, bar, baz'
+  ];
+
+  assert.deepStrictEqual(expected, rawHeaders);
+  stream.respond(src);
+  stream.end();
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request(src);
+  req.on('streamClosed', common.mustCall(() => {
+    server.close();
+    client.destroy();
+  }));
+}));
diff --git a/test/parallel/test-http2-multiheaders.js b/test/parallel/test-http2-multiheaders.js
index 54a900b04e..bcdc9458b3 100644
--- a/test/parallel/test-http2-multiheaders.js
+++ b/test/parallel/test-http2-multiheaders.js
@@ -31,15 +31,15 @@ src['__Proto__'] = 'baz';
 
 function checkHeaders(headers) {
   assert.deepStrictEqual(headers['accept'],
-                         [ 'abc', 'def', 'ghijklmnop' ]);
+                         'abc, def, ghijklmnop');
   assert.deepStrictEqual(headers['www-authenticate'],
-                         [ 'foo', 'bar', 'baz' ]);
+                         'foo, bar, baz');
   assert.deepStrictEqual(headers['proxy-authenticate'],
-                         [ 'foo', 'bar', 'baz' ]);
-  assert.deepStrictEqual(headers['x-foo'], [ 'foo', 'bar', 'baz' ]);
-  assert.deepStrictEqual(headers['constructor'], [ 'foo', 'bar', 'baz' ]);
+                         'foo, bar, baz');
+  assert.deepStrictEqual(headers['x-foo'], 'foo, bar, baz');
+  assert.deepStrictEqual(headers['constructor'], 'foo, bar, baz');
   // eslint-disable-next-line no-proto
-  assert.deepStrictEqual(headers['__proto__'], [ 'foo', 'bar', 'baz' ]);
+  assert.deepStrictEqual(headers['__proto__'], 'foo, bar, baz');
 }
 
 server.on('stream', common.mustCall((stream, headers) => {