http: make maximum header size configurable per-stream or per-server by addaleax · Pull Request #30570 · nodejs/node

addaleax · 2019-11-20T23:04:57Z

Make maxHeaderSize a.k.a. --max-header-size configurable now that
the legacy parser is gone (which only supported a single global value).

Refs: #30567

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

mcollina

LGTM

(this is also backportable to Node 12 with a bit of work).

addaleax · 2019-11-21T09:48:53Z

(this is also backportable to Node 12 with a bit of work).

@mcollina Are you having something in mind that’s only enabled when not using --http-parser=legacy?

mcollina · 2019-11-21T10:05:11Z

Exactly.

doc/api/http.md

lib/_http_client.js

lib/_http_server.js

sam-github

Looks good, modulo a doc nit. Might be interesting to test tiny values, like 12 (itended to be 12 K, but not actually). Maybe there should be a minimum of 500 or something? <-- just a suggestion/question, not even a nit.

doc/api/http.md

addaleax · 2019-11-22T14:38:03Z

@cjihrig @sam-github Nits should be addressed now

Might be interesting to test tiny values, like 12 (itended to be 12 K, but not actually). Maybe there should be a minimum of 500 or something? <-- just a suggestion/question, not even a nit.

I’d figure somebody would notice pretty quickly if they set overly small values, but if a user is sure that that is what they want, I don’t think we need to stop them.

nodejs-github-bot · 2019-11-22T14:38:55Z

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

sam-github · 2019-11-22T16:30:33Z

I’d figure somebody would notice pretty quickly if they set overly small values

I'd be curious if setting a max size of 1 or 5 (bytes) will cause node to segv or abort() because its so unexpected, but that's unrelated to this PR, and I take your point about not bothering to protect people from non-functionally small valuea. If you ask for broken, you get broken.

nodejs-github-bot · 2019-11-29T15:34:50Z

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

nodejs-github-bot · 2019-11-30T00:46:29Z

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

lundibundi

👍

lib/_http_client.js

src/node_options.cc

test/parallel/test-http-max-header-size-per-stream.js

Make `maxHeaderSize` a.k.a. `--max-header-size` configurable now that the legacy parser is gone (which only supported a single global value). Refs: nodejs#30567

…server

nodejs-github-bot · 2019-11-30T14:50:12Z

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

…server

nodejs-github-bot · 2019-11-30T16:36:31Z

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

nodejs-github-bot · 2019-12-01T01:16:15Z

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

addaleax · 2019-12-01T02:01:03Z

Landed in 6bf5a1d

Make `maxHeaderSize` a.k.a. `--max-header-size` configurable now that the legacy parser is gone (which only supported a single global value). Refs: #30567 PR-URL: #30570 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Notable changes: * http: * Make maximum header size configurable per-stream or per-server (Anna Henningsen) #30570 * http2: * Make maximum tolerated rejected streams configurable (Denys Otrishko) #30534 * Allow to configure maximum tolerated invalid frames (Denys Otrishko) #30534 * wasi: * Introduce initial WASI support (cjihrig) #30258 PR-URL: #30774

Notable changes: * fs: * Reworked experimental recursive `rmdir()` (cjihrig) #30644 * The `maxBusyTries` option is renamed to `maxRetries`, and its default is set to 0. The `emfileWait` option has been removed, and `EMFILE` errors use the same retry logic as other errors. The `retryDelay` option is now supported. `ENFILE` errors are now retried. * http: * Make maximum header size configurable per-stream or per-server (Anna Henningsen) #30570 * http2: * Make maximum tolerated rejected streams configurable (Denys Otrishko) #30534 * Allow to configure maximum tolerated invalid frames (Denys Otrishko) #30534 * wasi: * Introduce initial WASI support (cjihrig) #30258 PR-URL: #30774

Fixes: nodejs#38954 Refs: nodejs#30570

Fixes: #38954 PR-URL: #38992 Refs: #30570 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

addaleax added http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 20, 2019

addaleax requested review from mcollina and sam-github November 20, 2019 23:04

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 20, 2019

addaleax mentioned this pull request Nov 20, 2019

http: llhttp opt-in insecure HTTP header parsing #30567

Closed

4 tasks

mcollina approved these changes Nov 21, 2019

View reviewed changes

devnexen approved these changes Nov 21, 2019

View reviewed changes

cjihrig approved these changes Nov 21, 2019

View reviewed changes

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

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

lib/_http_client.js Outdated Show resolved Hide resolved

lib/_http_server.js Outdated Show resolved Hide resolved

sam-github approved these changes Nov 21, 2019

View reviewed changes

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

sam-github approved these changes Nov 22, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2019

jasnell approved these changes Nov 29, 2019

View reviewed changes

lundibundi approved these changes Nov 30, 2019

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

src/node_options.cc Outdated Show resolved Hide resolved

test/parallel/test-http-max-header-size-per-stream.js Show resolved Hide resolved

addaleax added 3 commits November 30, 2019 15:35

http: make maximum header size configurable per-stream or per-server

abc91f4

Make `maxHeaderSize` a.k.a. `--max-header-size` configurable now that the legacy parser is gone (which only supported a single global value). Refs: nodejs#30567

fixup! http: make maximum header size configurable per-stream or per-…

a92936a

…server

fixup! http: make maximum header size configurable per-stream or per-…

65d8813

…server

addaleax force-pushed the http-max-header-size-configurable branch from cc20b41 to 65d8813 Compare November 30, 2019 14:49

fixup! http: make maximum header size configurable per-stream or per-…

550373b

…server

addaleax closed this Dec 1, 2019

addaleax deleted the http-max-header-size-configurable branch December 1, 2019 02:01

BridgeAR mentioned this pull request Dec 3, 2019

v13.3.0 proposal #30774

Merged

maxharlow mentioned this pull request Jan 13, 2020

Use modern HTTP parser maxharlow/reconcile#5

Closed

targos added dont-land-on-v10.x labels Jan 14, 2020

addaleax mentioned this pull request Jan 24, 2020

[v12.x] http: make --insecure-http-parser configurable per-stream or per-server #31500

Closed

4 tasks

joyeecheung mentioned this pull request Jun 8, 2020

src: retrieve binding data from the context #33139

Closed

3 tasks

richardlau mentioned this pull request Jun 10, 2021

maxHeaderSize option on HTTPS server is not working #38954

Closed

Ayase-252 added a commit to Ayase-252/node that referenced this pull request Jun 10, 2021

https: add maxHeaderSize option for https server

0029369

Fixes: nodejs#38954 Refs: nodejs#30570

Ayase-252 added a commit to Ayase-252/node that referenced this pull request Jun 10, 2021

https: add maxHeaderSize option for https server

3956f4e

Fixes: nodejs#38954 Refs: nodejs#30570

Ayase-252 mentioned this pull request Jun 10, 2021

http,https: align options of https's server with http's #38992

Closed

nodejs-github-bot pushed a commit that referenced this pull request Jun 26, 2021

http,https: align server option of https with http

8a8c7f4

Fixes: #38954 PR-URL: #38992 Refs: #30570 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos pushed a commit that referenced this pull request Jul 11, 2021

http,https: align server option of https with http

35331cb

Fixes: #38954 PR-URL: #38992 Refs: #30570 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos pushed a commit that referenced this pull request Sep 4, 2021

http,https: align server option of https with http

8b3feee

Fixes: #38954 PR-URL: #38992 Refs: #30570 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Uh oh!

Conversation

addaleax commented Nov 20, 2019

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 21, 2019

Uh oh!

mcollina commented Nov 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

addaleax commented Nov 22, 2019

Uh oh!

nodejs-github-bot commented Nov 22, 2019

Uh oh!

sam-github commented Nov 22, 2019

Uh oh!

nodejs-github-bot commented Nov 29, 2019

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

lundibundi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

nodejs-github-bot commented Dec 1, 2019

Uh oh!

addaleax commented Dec 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants