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

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

Closed

Conversation

@addaleax
Copy link
Member

addaleax commented Nov 20, 2019

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
Copy link
Member

mcollina left a comment

LGTM

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

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 21, 2019

(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

This comment has been minimized.

Copy link
Member

mcollina commented Nov 21, 2019

Exactly.

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
Copy link
Member

sam-github left a comment

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 Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 22, 2019

@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

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 22, 2019

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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

lundibundi left a comment

👍

lib/_http_client.js Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
addaleax added 3 commits Nov 20, 2019
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
…server
…server
@addaleax addaleax force-pushed the addaleax:http-max-header-size-configurable branch from cc20b41 to 65d8813 Nov 30, 2019
@nodejs-github-bot

This comment has been minimized.

…server
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Dec 1, 2019

Landed in 6bf5a1d

@addaleax addaleax closed this Dec 1, 2019
@addaleax addaleax deleted the addaleax:http-max-header-size-configurable branch Dec 1, 2019
addaleax added a commit that referenced this pull request Dec 1, 2019
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>
targos added a commit that referenced this pull request Dec 1, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
BridgeAR added a commit that referenced this pull request Dec 3, 2019
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
BridgeAR added a commit that referenced this pull request Dec 3, 2019
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
BridgeAR added a commit that referenced this pull request Dec 3, 2019
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
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
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

PR-URL: nodejs#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>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    nodejs#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) nodejs#30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) nodejs#30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) nodejs#30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    nodejs#30258

PR-URL: nodejs#30774
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.