http2: allow Host in HTTP/2 requests #34664

mildsunrise · 2020-08-07T11:39:43Z

See #29858 for context. There were no objections, so I think we can go ahead and fix it.
This should align Node.js behaviour with the HTTP/2 spec and with nghttp2. @nodejs/http2

Proposed changes:

Remove host from the connection header list (allowing it in requests).
If host is present, don't autofill :authority (this would be an entirely different request).
In the compatibility API (request.authority), fall back to using host if :authority is not present.
This is consistent with nghttp2.

Side-effects of these changes (due to nghttp2):

As the spec says, nghttp2 enforces non-CONNECT requests to have either :authority or host present.
We now allow requests without :authority. Code relying on :authority being set may break, hence I've marked this semver-major.
The same validations from :authority apply to host.

I've also added a note to the documentation explaining all this.

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: nodejs#29858

nodejs-github-bot · 2020-08-07T11:43:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32663/

test/parallel/test-http2-compat-serverrequest-host.js

mcollina

lgtm!

nodejs-github-bot · 2020-08-07T18:09:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32671/

mildsunrise · 2020-08-07T18:19:03Z

Maybe this should be a notable change?

doc/api/http2.md

Trott · 2020-08-09T22:05:23Z

doc/api/http2.md

+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).


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).

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)

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

doc/api/http2.md

Trott

Left several comments, but they are all optional or can be deferred.

mcollina · 2020-08-10T22:59:16Z

@mildsunrise would you mind to address @Trott nits?

mildsunrise · 2020-08-11T11:46:41Z

Yes, don't land yet :)

mildsunrise · 2020-08-11T12:29:34Z

(also, just in case, I didn't actually request those reviews, that was an automated event)

nodejs-github-bot · 2020-08-11T23:57:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32740/ (💚)

mildsunrise · 2020-08-16T16:37:56Z

I think there's enough feedback to land this, will do shortly

mildsunrise · 2020-08-17T11:51:12Z

Landed in f5c0e28

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: #29858 PR-URL: #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>

Notable changes: Deprecations and Removals: - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **timers**: introduce timers/promises (James M Snell) (#33950) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) npm 7 (#35631): Node.js 15 comes with a new major release of npm, npm 7. npm 7 comes with many new features - including npm workspaces and a new package-lock.json format. npm 7 also includes yarn.lock file support. One of the big changes in npm 7 is that peer dependencies are now installed by default. Throw On Unhandled Rejections (#33021): As of Node.js 15, the default mode for `unhandledRejection` is changed to `throw` (from `warn`). In `throw` mode, if an `unhandledRejection` hook is not set, the `unhandledRejection` is raised as an uncaught exception. Users that have an `unhandledRejection` hook should see no change in behavior, and it’s still possible to switch modes using the `--unhandled-rejections=mode` process flag. QUIC (#32379): Node.js 15 comes with experimental support QUIC, which can be enabled by compiling Node.js with the `--experimental-quic` configuration flag. The Node.js QUIC implementation is exposed by the core `net` module. V8 8.6 (#35415): The V8 JavaScript engine has been updated to V8 8.6 (V8 8.4 is the latest available in Node.js 14). Along with performance tweaks and improvements the V8 update also brings the following language features: * `Promise.any()` (from V8 8.5) * `AggregateError` (from V8 8.5) * `String.prototype.replaceAll()` (from V8 8.5) * Logical assignment operators `&&=`, `||=`, and `??=` (from V8 8.5) Other Notable Changes: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **dns**: add dns/promises alias (shisama) (#32953) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **stream**: construct (Robert Nagy) (#29656) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) Semver-Major Commits: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **build**: reset embedder string to "-node.0" (Michaël Zasso) (#35415) - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **crypto**: refactoring internals, add WebCrypto (James M Snell) (#35093) - **crypto**: move node\_crypto files to src/crypto (James M Snell) (#35093) - **deps**: V8: cherry-pick d76abfed3512 (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 717543bbf0ef (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 6be2f6e26e8d (Michaël Zasso) (#35415) - **deps**: fix V8 build issue with inline methods (Jiawen Geng) (#35415) - **deps**: fix platform-embedded-file-writer-win for ARM64 (Michaël Zasso) (#35415) - **deps**: update V8 postmortem metadata script (Colin Ihrig) (#35415) - **deps**: update V8 to 8.6.395 (Michaël Zasso) (#35415) - **deps**: upgrade npm to 7.0.0 (Myles Borins) (#35631) - **deps**: update npm to 7.0.0-rc.3 (Myles Borins) (#35474) - **deps**: V8: cherry-pick 0d6debcc5f08 (Gus Caplan) (#33600) - **dns**: add dns/promises alias (shisama) (#32953) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **doc**: update support macos version for 15.x (Ash Cripps) (#35022) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http**: fixed socket.setEncoding fatal error (iskore) (#33405) - **http**: emit 'error' on aborted server request (Robert Nagy) (#33172) - **http**: cleanup end argument handling (Robert Nagy) (#31818) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **http2**: add `invalidheaders` test (Pranshu Srivastava) (#33161) - **http2**: refactor state code validation for the http2Stream class (rickyes) (#33535) - **http2**: header field valid checks (Pranshu Srivastava) (#33193) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: remove ERR\_INVALID\_OPT\_VALUE and ERR\_INVALID\_OPT\_VALUE\_ENCODING (Denys Otrishko) (#34682) - **lib**: handle one of args case in ERR\_MISSING\_ARGS (Denys Otrishko) (#34022) - **lib**: remove NodeError from the prototype of errors with code (Michaël Zasso) (#33857) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: check args in net.connect() and socket.connect() calls (Denys Otrishko) (#34022) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **process**: update v8 fast api calls usage (Maya Lekova) (#35415) - **process**: change default --unhandled-rejections=throw (Dan Fabulich) (#33021) - **process**: use v8 fast api calls for hrtime (Gus Caplan) (#33600) - **process**: delay throwing an error using `throwDeprecation` (Ruben Bridgewater) (#32312) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: implement NodePlatform::PostJob (Clemens Backes) (#35415) - **src**: update NODE\_MODULE\_VERSION to 88 (Michaël Zasso) (#35415) - **src**: error reporting on CPUUsage (Yash Ladha) (#34762) - **src**: use node:moduleName as builtin module filename (Michaël Zasso) (#35498) - **src**: enable wasm trap handler on windows (Gus Caplan) (#35033) - **src**: update NODE\_MODULE\_VERSION to 86 (Michaël Zasso) (#33579) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **src**: remove \_third\_party\_main support (Anna Henningsen) (#33971) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **src**: remove unused CancelPendingDelayedTasks (Anna Henningsen) (#32859) - **stream**: try to wait for flush to complete before 'finish' (Robert Nagy) (#34314) - **stream**: cleanup and fix Readable.wrap (Robert Nagy) (#34204) - **stream**: add promises version to utility functions (rickyes) (#33991) - **stream**: fix writable.end callback behavior (Robert Nagy) (#34101) - **stream**: construct (Robert Nagy) (#29656) - **stream**: write should throw on unknown encoding (Robert Nagy) (#33075) - **stream**: fix \_final and 'prefinish' timing (Robert Nagy) (#32780) - **stream**: simplify Transform stream implementation (Robert Nagy) (#32763) - **stream**: use callback to properly propagate error (Robert Nagy) (#29179) - **test**: update tests after increasing typed array size to 4GB (Kim-Anh Tran) (#35415) - **test**: fix tests for npm 7.0.0 (Myles Borins) (#35631) - **test**: fix test suite to work with npm 7 (Myles Borins) (#35474) - **test**: update WPT harness and tests (Michaël Zasso) (#33770) - **timers**: introduce timers/promises (James M Snell) (#33950) - **tools**: disable x86 safe exception handlers in V8 (Michaël Zasso) (#35415) - **tools**: update V8 gypfiles for 8.6 (Ujjwal Sharma) (#35415) - **tools**: update V8 gypfiles for 8.5 (Ujjwal Sharma) (#35415) - **url**: file URL path normalization (Daijiro Wachi) (#35477) - **url**: verify domain is not empty after "ToASCII" (Michaël Zasso) (#33770) - **url**: remove U+0000 case in the fragment state (Michaël Zasso) (#33770) - **url**: remove gopher from special schemes (Michaël Zasso) (#33325) - **url**: forbid lt and gt in url host code point (Yash Ladha) (#33328) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) - **win, child_process**: sanitize env variables (Bartosz Sosnowski) (#35210) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) - **worker**: set trackUnmanagedFds to true by default (Anna Henningsen) (#34394) - **worker**: rename error code to be more accurate (Anna Henningsen) (#33872) PR-URL: #35014

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 7, 2020

mildsunrise added http2 Issues or PRs related to the http2 subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 7, 2020

jasnell approved these changes Aug 7, 2020

View reviewed changes

Flarna approved these changes Aug 7, 2020

View reviewed changes

ZYSzys approved these changes Aug 7, 2020

View reviewed changes

test/parallel/test-http2-compat-serverrequest-host.js Outdated Show resolved Hide resolved

mcollina approved these changes Aug 7, 2020

View reviewed changes

typos

fe83df7

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 7, 2020

addaleax approved these changes Aug 7, 2020

View reviewed changes

This was referenced Aug 8, 2020

CI Relability 2020-08-08 JaneaSystems/nodejs-reliability#62

Open

CI Reliability 2020-08-08 nodejs/reliability#37

Open

CI Relability 2020-08-09 JaneaSystems/nodejs-reliability#63

Open

CI Reliability 2020-08-09 nodejs/reliability#38

Open

Trott reviewed Aug 9, 2020

View reviewed changes

doc/api/http2.md Outdated Show resolved Hide resolved

Trott reviewed Aug 9, 2020

View reviewed changes

doc/api/http2.md Outdated Show resolved Hide resolved

Trott reviewed Aug 9, 2020

View reviewed changes

doc/api/http2.md Outdated Show resolved Hide resolved

Trott reviewed Aug 9, 2020

View reviewed changes

doc/api/http2.md Outdated Show resolved Hide resolved

Trott approved these changes Aug 9, 2020

View reviewed changes

This was referenced Aug 10, 2020

CI Relability 2020-08-10 JaneaSystems/nodejs-reliability#64

Open

CI Reliability 2020-08-10 nodejs/reliability#39

Open

rickyes approved these changes Aug 10, 2020

View reviewed changes

mildsunrise requested review from a team as code owners August 10, 2020 16:06

This was referenced Aug 11, 2020

CI Relability 2020-08-11 JaneaSystems/nodejs-reliability#65

Open

CI Reliability 2020-08-11 nodejs/reliability#40

Open

improvements in docs

f96d134

koliberek0302 approved these changes Aug 12, 2020

View reviewed changes

mildsunrise closed this Aug 17, 2020

mildsunrise deleted the allow-host-http2 branch August 17, 2020 11:51

BethGriggs mentioned this pull request Sep 1, 2020

2020-10-20, Version 15.0.0 (Current) #35014

Merged

bl-ue mentioned this pull request Jun 8, 2021

http2 stream.end error #36860

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: allow Host in HTTP/2 requests #34664

http2: allow Host in HTTP/2 requests #34664

mildsunrise commented Aug 7, 2020 •

edited

nodejs-github-bot commented Aug 7, 2020

mcollina left a comment

nodejs-github-bot commented Aug 7, 2020

mildsunrise commented Aug 7, 2020

Trott Aug 9, 2020

mildsunrise Aug 11, 2020

mildsunrise Aug 11, 2020

Trott left a comment

mcollina commented Aug 10, 2020

mildsunrise commented Aug 11, 2020

mildsunrise commented Aug 11, 2020

nodejs-github-bot commented Aug 11, 2020 •

edited by mildsunrise

mildsunrise commented Aug 16, 2020

mildsunrise commented Aug 17, 2020

http2: allow Host in HTTP/2 requests #34664

http2: allow Host in HTTP/2 requests #34664

Conversation

mildsunrise commented Aug 7, 2020 • edited

nodejs-github-bot commented Aug 7, 2020

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 7, 2020

mildsunrise commented Aug 7, 2020

Trott Aug 9, 2020

Choose a reason for hiding this comment

mildsunrise Aug 11, 2020

Choose a reason for hiding this comment

mildsunrise Aug 11, 2020

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

mcollina commented Aug 10, 2020

mildsunrise commented Aug 11, 2020

mildsunrise commented Aug 11, 2020

nodejs-github-bot commented Aug 11, 2020 • edited by mildsunrise

mildsunrise commented Aug 16, 2020

mildsunrise commented Aug 17, 2020

mildsunrise commented Aug 7, 2020 •

edited

nodejs-github-bot commented Aug 11, 2020 •

edited by mildsunrise