http2: report invalid for spaced header names by rexagod · Pull Request #33161 · nodejs/node

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

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!

mcollina

lgtm

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

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.

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/

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

Comment thread lib/internal/http2/compat.js Outdated

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

Comment thread test/parallel/test-http2-invalidheaderfield.js Outdated

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

Comment thread test/parallel/test-http2-invalidheaderfield.js Outdated

Comment thread lib/internal/http2/util.js Outdated

lundibundi reviewed Jun 3, 2020

View reviewed changes

Comment thread lib/internal/http2/compat.js Outdated

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

Comment thread test/parallel/test-http2-invalidheaderfield.js Outdated

Comment thread test/parallel/test-http2-invalidheaderfield.js Outdated

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

Comment thread test/parallel/test-http2-invalidheaderfield.js Outdated

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 pushed a commit that referenced this pull request Jun 17, 2020

Uh oh!

Conversation

rexagod commented Apr 30, 2020 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

BridgeAR commented May 3, 2020

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 19, 2020

Uh oh!

nodejs-github-bot commented May 19, 2020

Uh oh!

rexagod commented May 20, 2020

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rexagod Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lundibundi Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

rexagod Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 8, 2020

Codecov Report

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jun 14, 2020

Uh oh!

mcollina commented Jun 14, 2020

Uh oh!

rexagod commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lundibundi left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jun 16, 2020

Uh oh!

rexagod commented Apr 30, 2020 •

edited by ronag

Loading

rexagod Jun 8, 2020 •

edited

Loading

rexagod commented Jun 16, 2020 •

edited

Loading