From f5c0e282ccf98e17f295c11850649ad19a6fff51 Mon Sep 17 00:00:00 2001
From: Alba Mendez <me@alba.sh>
Date: Fri, 7 Aug 2020 12:54:08 +0200
Subject: [PATCH] http2: allow Host in HTTP/2 requests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The HTTP/2 spec allows Host to be used instead of :authority in
requests, and this is in fact *preferred* when converting from HTTP/1.

We erroneously treated Host as a connection header, thus disallowing
it in requests. The patch corrects this, aligning Node.js behaviour
with the HTTP/2 spec and with nghttp2:

 - Treat Host as a single-value header instead of a connection header.
 - Don't autofill :authority if Host is present.
 - The compatibility API (request.authority) falls back to using Host
   if :authority is not present.

This is semver-major because requests are no longer guaranteed to
have :authority set. An explanatory note was added to the docs.

Fixes: https://github.com/nodejs/node/issues/29858

PR-URL: https://github.com/nodejs/node/pull/34664
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
---
 doc/api/http2.md                              | 25 +++++++-
 lib/internal/http2/compat.js                  |  5 +-
 lib/internal/http2/core.js                    |  9 +--
 lib/internal/http2/util.js                    | 16 ++++-
 .../test-http2-compat-serverrequest-host.js   | 63 +++++++++++++++++++
 test/parallel/test-http2-util-headers-list.js | 21 ++++++-
 6 files changed, 125 insertions(+), 14 deletions(-)
 create mode 100644 test/parallel/test-http2-compat-serverrequest-host.js

diff --git a/doc/api/http2.md b/doc/api/http2.md
index c8c51ed92ef247..5adf84e27f8b53 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -2,6 +2,10 @@
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/34664
+    description: Requests with the `host` header (with or without
+                 `:authority`) can now be sent/received.
   - version: v10.10.0
     pr-url: https://github.com/nodejs/node/pull/22466
     description: HTTP/2 is now Stable. Previously, it had been Experimental.
@@ -2530,7 +2534,7 @@ For incoming headers:
   `access-control-max-age`, `access-control-request-method`, `content-encoding`,
   `content-language`, `content-length`, `content-location`, `content-md5`,
   `content-range`, `content-type`, `date`, `dnt`, `etag`, `expires`, `from`,
-  `if-match`, `if-modified-since`, `if-none-match`, `if-range`,
+  `host`, `if-match`, `if-modified-since`, `if-none-match`, `if-range`,
   `if-unmodified-since`, `last-modified`, `location`, `max-forwards`,
   `proxy-authorization`, `range`, `referer`,`retry-after`, `tk`,
   `upgrade-insecure-requests`, `user-agent` or `x-content-type-options` are
@@ -2909,8 +2913,10 @@ added: v8.4.0
 
 * {string}
 
-The request authority pseudo header field. It can also be accessed via
-`req.headers[':authority']`.
+The request authority pseudo header field. Because HTTP/2 allows requests
+to set either `:authority` or `host`, this value is derived from
+`req.headers[':authority']` if present. Otherwise, it is derived from
+`req.headers['host']`.
 
 #### `request.complete`
 <!-- YAML
@@ -3709,6 +3715,18 @@ following additional properties:
 * `type` {string} Either `'server'` or `'client'` to identify the type of
   `Http2Session`.
 
+## Note on `:authority` and `host`
+
+HTTP/2 requires requests to have either the `:authority` pseudo-header
+or the `host` header. Prefer `:authority` when constructing an HTTP/2
+request directly, and `host` when converting from HTTP/1 (in proxies,
+for instance).
+
+The compatibility API falls back to `host` if `:authority` is not
+present. See [`request.authority`][] for more information. However,
+if you don't use the compatibility API (or use `req.headers` directly),
+you need to implement any fall-back behaviour yourself.
+
 [ALPN Protocol ID]: https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids
 [ALPN negotiation]: #http2_alpn_negotiation
 [Compatibility API]: #http2_compatibility_api
@@ -3749,6 +3767,7 @@ following additional properties:
 [`net.Socket.prototype.unref()`]: net.html#net_socket_unref
 [`net.Socket`]: net.html#net_class_net_socket
 [`net.connect()`]: net.html#net_net_connect
+[`request.authority`]: #http2_request_authority
 [`request.socket`]: #http2_request_socket
 [`request.socket.getPeerCertificate()`]: tls.html#tls_tlssocket_getpeercertificate_detailed
 [`response.end()`]: #http2_response_end_data_encoding_callback
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
index 4038a4fdb5321d..94e8d81f047eec 100644
--- a/lib/internal/http2/compat.js
+++ b/lib/internal/http2/compat.js
@@ -50,7 +50,8 @@ const {
   kSocket,
   kRequest,
   kProxySocket,
-  assertValidPseudoHeader
+  assertValidPseudoHeader,
+  getAuthority
 } = require('internal/http2/util');
 const { _checkIsHttpToken: checkIsHttpToken } = require('_http_common');
 
@@ -395,7 +396,7 @@ class Http2ServerRequest extends Readable {
   }
 
   get authority() {
-    return this[kHeaders][HTTP2_HEADER_AUTHORITY];
+    return getAuthority(this[kHeaders]);
   }
 
   get scheme() {
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index a04403996010c7..1a07a09d29fb83 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -118,6 +118,7 @@ const {
   assertValidPseudoHeaderResponse,
   assertValidPseudoHeaderTrailer,
   assertWithinRange,
+  getAuthority,
   getDefaultSettings,
   getSessionState,
   getSettings,
@@ -1636,7 +1637,7 @@ class ClientHttp2Session extends Http2Session {
     const connect = headers[HTTP2_HEADER_METHOD] === HTTP2_METHOD_CONNECT;
 
     if (!connect || headers[HTTP2_HEADER_PROTOCOL] !== undefined) {
-      if (headers[HTTP2_HEADER_AUTHORITY] === undefined)
+      if (getAuthority(headers) === undefined)
         headers[HTTP2_HEADER_AUTHORITY] = this[kAuthority];
       if (headers[HTTP2_HEADER_SCHEME] === undefined)
         headers[HTTP2_HEADER_SCHEME] = this[kProtocol].slice(0, -1);
@@ -1667,7 +1668,7 @@ class ClientHttp2Session extends Http2Session {
     const stream = new ClientHttp2Stream(this, undefined, undefined, {});
     stream[kSentHeaders] = headers;
     stream[kOrigin] = `${headers[HTTP2_HEADER_SCHEME]}://` +
-                      `${headers[HTTP2_HEADER_AUTHORITY]}`;
+                      `${getAuthority(headers)}`;
 
     // Close the writable side of the stream if options.endStream is set.
     if (options.endStream)
@@ -2459,7 +2460,7 @@ class ServerHttp2Stream extends Http2Stream {
     handle.owner = this;
     this[kInit](id, handle);
     this[kProtocol] = headers[HTTP2_HEADER_SCHEME];
-    this[kAuthority] = headers[HTTP2_HEADER_AUTHORITY];
+    this[kAuthority] = getAuthority(headers);
   }
 
   // True if the remote peer accepts push streams
@@ -2502,7 +2503,7 @@ class ServerHttp2Stream extends Http2Stream {
 
     if (headers[HTTP2_HEADER_METHOD] === undefined)
       headers[HTTP2_HEADER_METHOD] = HTTP2_METHOD_GET;
-    if (headers[HTTP2_HEADER_AUTHORITY] === undefined)
+    if (getAuthority(headers) === undefined)
       headers[HTTP2_HEADER_AUTHORITY] = this[kAuthority];
     if (headers[HTTP2_HEADER_SCHEME] === undefined)
       headers[HTTP2_HEADER_SCHEME] = this[kProtocol];
diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js
index f2ec0804b683ca..93994ca29ea0e5 100644
--- a/lib/internal/http2/util.js
+++ b/lib/internal/http2/util.js
@@ -61,6 +61,7 @@ const {
   HTTP2_HEADER_ETAG,
   HTTP2_HEADER_EXPIRES,
   HTTP2_HEADER_FROM,
+  HTTP2_HEADER_HOST,
   HTTP2_HEADER_IF_MATCH,
   HTTP2_HEADER_IF_NONE_MATCH,
   HTTP2_HEADER_IF_MODIFIED_SINCE,
@@ -84,7 +85,6 @@ const {
   HTTP2_HEADER_HTTP2_SETTINGS,
   HTTP2_HEADER_TE,
   HTTP2_HEADER_TRANSFER_ENCODING,
-  HTTP2_HEADER_HOST,
   HTTP2_HEADER_KEEP_ALIVE,
   HTTP2_HEADER_PROXY_CONNECTION,
 
@@ -131,6 +131,7 @@ const kSingleValueHeaders = new Set([
   HTTP2_HEADER_ETAG,
   HTTP2_HEADER_EXPIRES,
   HTTP2_HEADER_FROM,
+  HTTP2_HEADER_HOST,
   HTTP2_HEADER_IF_MATCH,
   HTTP2_HEADER_IF_MODIFIED_SINCE,
   HTTP2_HEADER_IF_NONE_MATCH,
@@ -429,7 +430,6 @@ function isIllegalConnectionSpecificHeader(name, value) {
   switch (name) {
     case HTTP2_HEADER_CONNECTION:
     case HTTP2_HEADER_UPGRADE:
-    case HTTP2_HEADER_HOST:
     case HTTP2_HEADER_HTTP2_SETTINGS:
     case HTTP2_HEADER_KEEP_ALIVE:
     case HTTP2_HEADER_PROXY_CONNECTION:
@@ -625,12 +625,24 @@ function sessionName(type) {
   }
 }
 
+function getAuthority(headers) {
+  // For non-CONNECT requests, HTTP/2 allows either :authority
+  // or Host to be used equivalently. The first is preferred
+  // when making HTTP/2 requests, and the latter is preferred
+  // when converting from an HTTP/1 message.
+  if (headers[HTTP2_HEADER_AUTHORITY] !== undefined)
+    return headers[HTTP2_HEADER_AUTHORITY];
+  if (headers[HTTP2_HEADER_HOST] !== undefined)
+    return headers[HTTP2_HEADER_HOST];
+}
+
 module.exports = {
   assertIsObject,
   assertValidPseudoHeader,
   assertValidPseudoHeaderResponse,
   assertValidPseudoHeaderTrailer,
   assertWithinRange,
+  getAuthority,
   getDefaultSettings,
   getSessionState,
   getSettings,
diff --git a/test/parallel/test-http2-compat-serverrequest-host.js b/test/parallel/test-http2-compat-serverrequest-host.js
new file mode 100644
index 00000000000000..36600f787ca2e3
--- /dev/null
+++ b/test/parallel/test-http2-compat-serverrequest-host.js
@@ -0,0 +1,63 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+
+// Requests using host instead of :authority should be allowed
+// and Http2ServerRequest.authority should fall back to host
+
+// :authority should NOT be auto-filled if host is present
+
+const server = h2.createServer();
+server.listen(0, common.mustCall(function() {
+  const port = server.address().port;
+  server.once('request', common.mustCall(function(request, response) {
+    const expected = {
+      ':path': '/foobar',
+      ':method': 'GET',
+      ':scheme': 'http',
+      'host': `localhost:${port}`
+    };
+
+    assert.strictEqual(request.authority, expected.host);
+
+    const headers = request.headers;
+    for (const [name, value] of Object.entries(expected)) {
+      assert.strictEqual(headers[name], value);
+    }
+
+    const rawHeaders = request.rawHeaders;
+    for (const [name, value] of Object.entries(expected)) {
+      const position = rawHeaders.indexOf(name);
+      assert.notStrictEqual(position, -1);
+      assert.strictEqual(rawHeaders[position + 1], value);
+    }
+
+    assert(!Object.hasOwnProperty.call(headers, ':authority'));
+    assert(!Object.hasOwnProperty.call(rawHeaders, ':authority'));
+
+    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',
+      'host': `localhost:${port}`
+    };
+    const request = client.request(headers);
+    request.on('end', common.mustCall(function() {
+      client.close();
+    }));
+    request.end();
+    request.resume();
+  }));
+}));
diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js
index 6b9ce079076ce8..bc9455731c4908 100644
--- a/test/parallel/test-http2-util-headers-list.js
+++ b/test/parallel/test-http2-util-headers-list.js
@@ -8,7 +8,11 @@ const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
 const assert = require('assert');
-const { mapToHeaders, toHeaderObject } = require('internal/http2/util');
+const {
+  getAuthority,
+  mapToHeaders,
+  toHeaderObject
+} = require('internal/http2/util');
 const { sensitiveHeaders } = require('http2');
 const { internalBinding } = require('internal/test/binding');
 const {
@@ -34,6 +38,7 @@ const {
   HTTP2_HEADER_ETAG,
   HTTP2_HEADER_EXPIRES,
   HTTP2_HEADER_FROM,
+  HTTP2_HEADER_HOST,
   HTTP2_HEADER_IF_MATCH,
   HTTP2_HEADER_IF_MODIFIED_SINCE,
   HTTP2_HEADER_IF_NONE_MATCH,
@@ -86,7 +91,6 @@ const {
   HTTP2_HEADER_HTTP2_SETTINGS,
   HTTP2_HEADER_TE,
   HTTP2_HEADER_TRANSFER_ENCODING,
-  HTTP2_HEADER_HOST,
   HTTP2_HEADER_KEEP_ALIVE,
   HTTP2_HEADER_PROXY_CONNECTION
 } = internalBinding('http2').constants;
@@ -225,6 +229,7 @@ const {
   HTTP2_HEADER_ETAG,
   HTTP2_HEADER_EXPIRES,
   HTTP2_HEADER_FROM,
+  HTTP2_HEADER_HOST,
   HTTP2_HEADER_IF_MATCH,
   HTTP2_HEADER_IF_MODIFIED_SINCE,
   HTTP2_HEADER_IF_NONE_MATCH,
@@ -289,7 +294,6 @@ const {
   HTTP2_HEADER_HTTP2_SETTINGS,
   HTTP2_HEADER_TE,
   HTTP2_HEADER_TRANSFER_ENCODING,
-  HTTP2_HEADER_HOST,
   HTTP2_HEADER_PROXY_CONNECTION,
   HTTP2_HEADER_KEEP_ALIVE,
   'Connection',
@@ -327,6 +331,17 @@ assert.throws(
 mapToHeaders({ te: 'trailers' });
 mapToHeaders({ te: ['trailers'] });
 
+// HTTP/2 encourages use of Host instead of :authority when converting
+// from HTTP/1 to HTTP/2, so we no longer disallow it.
+// Refs: https://github.com/nodejs/node/issues/29858
+mapToHeaders({ [HTTP2_HEADER_HOST]: 'abc' });
+
+// If both are present, the latter has priority
+assert.strictEqual(getAuthority({
+  [HTTP2_HEADER_AUTHORITY]: 'abc',
+  [HTTP2_HEADER_HOST]: 'def'
+}), 'abc');
+
 
 {
   const rawHeaders = [