http2: report invalid for spaced header names #33161

rexagod · 2020-04-30T07:28:34Z

Refs: #29829

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR · 2020-05-03T17:11:56Z

@nodejs/http @nodejs/http2 @nodejs/streams PTAL

ronag

LGTM

lib/internal/http2/compat.js

mcollina

lgtm

addaleax · 2020-05-19T21:55:12Z

@rexagod Is this semver-major? It looks that way to me.

nodejs-github-bot · 2020-05-19T21:55:58Z

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

rexagod · 2020-05-20T12:03:27Z

@addaleax +1. Thank you for tagging this!

test/parallel/test-http2-invalidheaderfield.js

mcollina

lgtm

test/parallel/test-http2-invalidheaderfield.js

lib/internal/http2/util.js

lib/internal/http2/compat.js

test/parallel/test-http2-invalidheaderfield.js

rexagod · 2020-06-08T02:21:26Z

test/parallel/test-http2-invalidheaderfield.js

+  }, TypeError);
+
+  // should not fail
+  res.setHeader(' test', 123);


This will be equivalent to 'test', hence a valid header. See below.

node/lib/internal/http2/compat.js

Line 604 in a29d7c7

name = name.trim().toLowerCase();

Hm, I wonder, the http fails with ERR_INVALID_HTTP_TOKEN if you try to set the header name of ' test'.

Each header field consists
of a name followed by a colon (":") and the field value. Field names
are case-insensitive. The field value MAY be preceded by any amount
of LWS, though a single SP is preferred.

@lundibundi The RFC2616 for HTTP/1 states that the field value may be preceded by any amount of LWS, but nothing similar is stated for the field names. Furthermore, this is not a problem in HTTP/2 owing to the binary protocol.

That part of the RFC if referring to the format of the HTTP message (i.e. actual text representation of the message), not the specific field name/value. Just down below in that section, we can see
field-name = token
And the token is
token = 1*<any CHAR except CTLs or separators>

Therefore there shouldn't be any leading whitespace in the user-provided value for header name which our http/1 implementation is enforcing correctly (by throwing ERR_INVALID_HTTP_TOKEN).

It's fine to do that in a follow-up PR, I just mentioned it so that we won't forget about it.

codecov-commenter · 2020-06-08T03:15:08Z

Codecov Report

Merging #33161 into master will increase coverage by 0.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #33161      +/-   ##
==========================================
+ Coverage   96.26%   96.71%   +0.44%     
==========================================
  Files         197      203       +6     
  Lines       65980    67388    +1408     
==========================================
+ Hits        63518    65171    +1653     
+ Misses       2462     2217     -245

Impacted Files	Coverage Δ
lib/internal/idna.js	`66.66% <0.00%> (-33.34%)`	⬇️
lib/internal/net.js	`75.00% <0.00%> (-25.00%)`	⬇️
lib/internal/inspector_async_hook.js	`86.84% <0.00%> (-13.16%)`	⬇️
lib/internal/source_map/source_map.js	`84.50% <0.00%> (-11.71%)`	⬇️
lib/internal/fs/rimraf.js	`67.90% <0.00%> (-11.49%)`	⬇️
lib/internal/v8_prof_processor.js	`92.85% <0.00%> (-4.77%)`	⬇️
lib/inspector.js	`94.30% <0.00%> (-4.39%)`	⬇️
lib/internal/source_map/source_map_cache.js	`80.21% <0.00%> (-3.77%)`	⬇️
lib/internal/modules/cjs/loader.js	`94.85% <0.00%> (-3.06%)`	⬇️
lib/internal/fs/utils.js	`94.97% <0.00%> (-1.99%)`	⬇️
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd4052c...2ffb264. Read the comment docs.

test/parallel/test-http2-invalidheaderfield.js

mcollina

lgtm

Refs: nodejs#29829

fix lint error

mcollina

still lgtm

nodejs-github-bot · 2020-06-14T09:52:09Z

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

mcollina · 2020-06-14T10:16:00Z

@rexagod there are still a few comments to fix, then we can land.

rexagod · 2020-06-16T11:49:24Z

@mcollina I think all the comments have been resolved now. Please let me know if there's anything else left to fix here.

lundibundi

LGTM.

Making explicit that IMO the leading whitespace in header names can be handled in a follow-up PR since this PR already improves things in compat layer a bit and there is no need to handle everything in one PR.

nodejs-github-bot · 2020-06-16T12:08:10Z

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

Refs: #29829 PR-URL: #33161 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

mcollina · 2020-06-17T08:27:06Z

Landed in 1e4187f

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 http2 Issues or PRs related to the http2 subsystem. label Apr 30, 2020

ronag approved these changes May 3, 2020

View reviewed changes

ronag reviewed May 3, 2020

View reviewed changes

lib/internal/http2/compat.js Show resolved Hide resolved

mcollina approved these changes May 3, 2020

View reviewed changes

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 19, 2020

jasnell approved these changes May 22, 2020

View reviewed changes

trivikr approved these changes May 31, 2020

View reviewed changes

test/parallel/test-http2-invalidheaderfield.js Outdated Show resolved Hide resolved

mcollina approved these changes May 31, 2020

View reviewed changes

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

lundibundi reviewed Jun 3, 2020

View reviewed changes

test/parallel/test-http2-invalidheaderfield.js Outdated Show resolved Hide resolved

lib/internal/http2/util.js Outdated Show resolved Hide resolved

lundibundi reviewed Jun 3, 2020

View reviewed changes

lib/internal/http2/compat.js Outdated Show resolved Hide resolved

rexagod mentioned this pull request Jun 6, 2020

The differences between http2 compatibility API and http[s] #29829

Open

32 tasks

rexagod requested a review from lundibundi June 7, 2020 07:58

lundibundi approved these changes Jun 7, 2020

View reviewed changes

test/parallel/test-http2-invalidheaderfield.js Show resolved Hide resolved

test/parallel/test-http2-invalidheaderfield.js Outdated Show resolved Hide resolved

rexagod commented Jun 8, 2020

View reviewed changes

rexagod force-pushed the i-29829-5 branch from 2ffb264 to 330f768 Compare June 8, 2020 07:54

lundibundi reviewed Jun 8, 2020

View reviewed changes

test/parallel/test-http2-invalidheaderfield.js Outdated Show resolved Hide resolved

mcollina approved these changes Jun 8, 2020

View reviewed changes

rexagod added 5 commits June 14, 2020 12:25

http2: add invalidheaders test

e90bf14

Refs: nodejs#29829

fix test structure and checks

e492874

fix tests

f3ba48d

fix tests

8dedf1f

fix tests

e173ca1

fix lint error

rexagod force-pushed the i-29829-5 branch from 487f23e to e173ca1 Compare June 14, 2020 06:56

mcollina approved these changes Jun 14, 2020

View reviewed changes

lundibundi approved these changes Jun 16, 2020

View reviewed changes

mcollina closed this Jun 17, 2020

This was referenced Jun 18, 2020

CI Relability 2020-06-18 JaneaSystems/nodejs-reliability#11

Open

CI Relability 2020-06-19 JaneaSystems/nodejs-reliability#12

Open

CI Relability 2020-06-20 JaneaSystems/nodejs-reliability#13

Open

CI Relability 2020-06-21 JaneaSystems/nodejs-reliability#14

Open

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

richardlau mentioned this pull request Jul 19, 2021

test: fix typo in test-http2-invalidheaderfield.js #39021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: report invalid for spaced header names #33161

http2: report invalid for spaced header names #33161

rexagod commented Apr 30, 2020 •

edited by ronag

BridgeAR commented May 3, 2020

ronag left a comment

mcollina left a comment

addaleax commented May 19, 2020

nodejs-github-bot commented May 19, 2020

rexagod commented May 20, 2020

mcollina left a comment

rexagod Jun 8, 2020 •

edited

lundibundi Jun 8, 2020

rexagod Jun 15, 2020

lundibundi Jun 16, 2020

codecov-commenter commented Jun 8, 2020

mcollina left a comment

mcollina left a comment

nodejs-github-bot commented Jun 14, 2020

mcollina commented Jun 14, 2020

rexagod commented Jun 16, 2020 •

edited

lundibundi left a comment

nodejs-github-bot commented Jun 16, 2020

mcollina commented Jun 17, 2020

http2: report invalid for spaced header names #33161

http2: report invalid for spaced header names #33161

Conversation

rexagod commented Apr 30, 2020 • edited by ronag

Checklist

BridgeAR commented May 3, 2020

ronag left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented May 19, 2020

nodejs-github-bot commented May 19, 2020

rexagod commented May 20, 2020

mcollina left a comment

Choose a reason for hiding this comment

rexagod Jun 8, 2020 • edited

Choose a reason for hiding this comment

lundibundi Jun 8, 2020

Choose a reason for hiding this comment

rexagod Jun 15, 2020

Choose a reason for hiding this comment

lundibundi Jun 16, 2020

Choose a reason for hiding this comment

codecov-commenter commented Jun 8, 2020

Codecov Report

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 14, 2020

mcollina commented Jun 14, 2020

rexagod commented Jun 16, 2020 • edited

lundibundi left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 16, 2020

mcollina commented Jun 17, 2020

rexagod commented Apr 30, 2020 •

edited by ronag

rexagod Jun 8, 2020 •

edited

rexagod commented Jun 16, 2020 •

edited