Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: allow Host in HTTP/2 requests #34664

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 28 additions & 3 deletions doc/api/http2.md
Expand Up @@ -2,6 +2,10 @@
<!-- YAML
added: v8.4.0
changes:
- version: REPLACEME
pr-url: REPLACEME
description: Requests with the `host` header (with or without
`:authority`) can now be sent / received.
mildsunrise marked this conversation as resolved.
Show resolved Hide resolved
- version: v10.10.0
pr-url: https://github.com/nodejs/node/pull/22466
description: HTTP/2 is now Stable. Previously, it had been Experimental.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2908,8 +2912,9 @@ 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, `req.headers['host']` otherwise.
mildsunrise marked this conversation as resolved.
Show resolved Hide resolved

#### `request.complete`
<!-- YAML
Expand Down Expand Up @@ -3708,6 +3713,25 @@ 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. Using `:authority` should be preferred when
constructing an HTTP/2 request directly, and `host` should be
preferred when converting from HTTP/1 (in proxies, for instance).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional suggestion:

Suggested change
or the `host` header. Using `:authority` should be preferred when
constructing an HTTP/2 request directly, and `host` should be
preferred when converting from HTTP/1 (in proxies, for instance).
or the `host` header. Use `:authority` when
constructing an HTTP/2 request directly, and `host`
when converting from HTTP/1 (in proxies, for instance).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers 💙 I'm not sure on this one, though... the spec only says it's a "preferred form", not imperative or anything, and I'm not sure how often this preference is honored in practice... (we didn't even allow that form to begin with)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll apply the suggestion with Prefer instead of Use


Historically, Node.js required all requests to have the `:authority`
pseudo-header and rejected with the `host` header present.
This has been corrected; the `host` header is now allowed, and the
only requirement is for either `host` or `:authority` to be
present (or both). The same validations from `:authority` extend
to `host`.

The compatibility API falls back to `host` if `:authority` is not
present, see [`request.authority`][]. However, if you don't use the
mildsunrise marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -3748,6 +3772,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
Expand Down
5 changes: 3 additions & 2 deletions lib/internal/http2/compat.js
Expand Up @@ -50,7 +50,8 @@ const {
kSocket,
kRequest,
kProxySocket,
assertValidPseudoHeader
assertValidPseudoHeader,
getAuthority
} = require('internal/http2/util');
const { _checkIsHttpToken: checkIsHttpToken } = require('_http_common');

Expand Down Expand Up @@ -395,7 +396,7 @@ class Http2ServerRequest extends Readable {
}

get authority() {
return this[kHeaders][HTTP2_HEADER_AUTHORITY];
return getAuthority(this[kHeaders]);
}

get scheme() {
Expand Down
9 changes: 5 additions & 4 deletions lib/internal/http2/core.js
Expand Up @@ -118,6 +118,7 @@ const {
assertValidPseudoHeaderResponse,
assertValidPseudoHeaderTrailer,
assertWithinRange,
getAuthority,
getDefaultSettings,
getSessionState,
getSettings,
Expand Down Expand Up @@ -1633,7 +1634,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);
Expand Down Expand Up @@ -1664,7 +1665,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)
Expand Down Expand Up @@ -2456,7 +2457,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
Expand Down Expand Up @@ -2499,7 +2500,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];
Expand Down
16 changes: 14 additions & 2 deletions lib/internal/http2/util.js
Expand Up @@ -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,
Expand All @@ -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,

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -418,7 +419,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:
Expand Down Expand Up @@ -614,12 +614,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,
Expand Down
63 changes: 63 additions & 0 deletions 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 hos
mildsunrise marked this conversation as resolved.
Show resolved Hide resolved

// :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();
}));
}));
21 changes: 18 additions & 3 deletions test/parallel/test-http2-util-headers-list.js
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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 = [
Expand Down