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

cli: add --max-http-header-size flag #24811

Merged
merged 3 commits into from
Dec 20, 2018
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 3, 2018

Allow the maximum size of HTTP headers to be overridden from the command line.

Refs: nodejs/http-parser#453
Fixes: #24692

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

@nodejs-github-bot 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 Dec 3, 2018
@mcollina
Copy link
Member

mcollina commented Dec 3, 2018

Is there a matching PR for http-parser?

@cjihrig do you see this as an alternative to #24716?

@mcollina
Copy link
Member

mcollina commented Dec 3, 2018

I think we should expose that value as a read-only property under http.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 3, 2018

Is there a matching PR for http-parser?

Yes, nodejs/http-parser#453

do you see this as an alternative to #24716?

Not strictly an alternative. See #24716 (comment) for my feelings on having both.

src/node_http_parser.cc Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor

Probably an obvious suggestion, but perhaps you can reuse the existing tests if they are made slightly context aware wrt. the size setting, then wrap them in one that specifies a node option, like https://github.com/nodejs/node/blob/master/test/parallel/test-tls-cli-min-version-1.0.js

doc/api/cli.md Outdated Show resolved Hide resolved
src/node_options.h Outdated Show resolved Hide resolved
mcollina referenced this pull request Dec 4, 2018
CVE-2018-12121

PR-URL: nodejs-private/node-private#143
Ref: nodejs-private/security#139
Ref: nodejs-private/http-parser-private#2
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dglozic
Copy link

dglozic commented Dec 4, 2018

Can this be back-ported to Node 8 LTS? As of v8.14.0, people blessed with domains with a lot of cookies are broken without this option, effectively preventing them from consuming any more LTS fixes (and 8.14.0 contains a security fix as well).

@cjihrig cjihrig force-pushed the max-headers-size branch 3 times, most recently from d4a3a0a to 3275c64 Compare December 5, 2018 23:02
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 5, 2018

I think I've addressed all the nits.

@mcollina, you signed off on this, but did you still want this added to the HTTP API?

@mcollina
Copy link
Member

mcollina commented Dec 5, 2018

It would be better but it can come later if somebody needs it.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 6, 2018

It would be better but it can come later if somebody needs it.

Opened #24860 to discuss separately.

@mcollina
Copy link
Member

mcollina commented Dec 7, 2018

@mcollina
Copy link
Member

@cjihrig what's the status of this?

@mcollina mcollina added tsc-review tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Dec 10, 2018
@lxe
Copy link

lxe commented Jan 10, 2019

Thanks for this!

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This commit adds support for uint64_t option parsing.

PR-URL: nodejs#24811
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Allow the maximum size of HTTP headers to be overridden from
the command line.

co-authored-by: Matteo Collina <[email protected]>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
The 6.15.0 security release introduced some unexpected breakages on
the 6.x release line. This is a special release to fix a regression
in the HTTP binary upgrade response body and add a missing CLI flag
to adjust the max header size of the http parser.

Notable changes:

* cli:
  - add --max-http-header-size flag (cjihrig)
    nodejs#24811
* http:
  - add maxHeaderSize property (cjihrig)
    nodejs#24860

PR-URL: nodejs#25178
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
The 8.14.0 security release introduced some unexpected breakages on
the 8.x release line. This is a special release to fix a regression
in the HTTP binary upgrade response body and add a missing CLI flag
to adjust the max header size of the http parser.

Notable changes:

* cli:
  - add --max-http-header-size flag (cjihrig)
    nodejs#24811
* http:
  - add maxHeaderSize property (cjihrig)
    nodejs#24860

PR-URL: nodejs#25177
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
The 10.14.0 security release introduced some unexpected breakages on
the 10.x release line. This is a special release to fix a regression
in the HTTP binary upgrade response body and add a missing CLI flag
to adjust the max header size of the http parser.

Notable Changes:

* cli:
  - add --max-http-header-size flag (cjihrig)
    nodejs#24811
* http:
  - add maxHeaderSize property (cjihrig)
    nodejs#24860

PR-URL: nodejs#25176
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Notable Changes:

* cli:
  - add --max-http-header-size flag (cjihrig)
    nodejs#24811
* crypto:
  - always accept certificates as public keys (Tobias Nießen)
    nodejs#24234
  - add key object API (Tobias Nießen) [nodejs#24234](nodejs#24234)
  - update root certificates (Sam Roberts)
    nodejs#25113
* deps:
  - upgrade to libuv 1.24.1 (cjihrig)
    nodejs#25078
  - upgrade npm to 6.5.0 (Audrey Eschright)
    nodejs#24734
* http:
  - add maxHeaderSize property (cjihrig)
    nodejs#24860

PR-URL: nodejs#25175
@pharatesid
Copy link

how do i use this feature and configure the http header size

@losdevnull
Copy link

Does this apply to https as well? It does not seem to work with https.createServer.

@mcollina
Copy link
Member

mcollina commented Feb 2, 2019

@losdevnull it should! If not please open an new issue with a script to reproduce and tag me.

@losdevnull
Copy link

@mcollina never mind.. it turns out that we did not set the arg correctly in command line by appending it in the end instead of in front of the app to be started. After fix that it worked as expected for https. Thanks!

@@ -916,6 +913,14 @@ const parser_settings_t Parser::settings = {
};


#ifndef NODE_EXPERIMENTAL_HTTP
void InitMaxHttpHeaderSizeOnce() {
const uint32_t max_http_header_size = per_process_opts->max_http_header_size;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cjihrig I tried to dig but couldn't figure out the answer.

Does per_process get populated with .npmrc or system environment values? Or can this only be set with the CLI option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can configure this via the CLI option or NODE_OPTIONS environment variable (search the tests in this PR for NODE_OPTIONS).

@mike503
Copy link

mike503 commented Feb 22, 2019

It would be great if node responded with an HTTP 413 “Request Header fields too large” (or something similar) is this was encountered instead of behaving like it is crashing (or empty return) or timing out (with nginx in front it will result in a 502/400 based on the node version or a 504 timeout)

@richardlau
Copy link
Member

It would be great if node responded with an HTTP 413 “Request Header fields too large” (or something similar) is this was encountered instead of behaving like it is crashing (or empty return) or timing out (with nginx in front it will result in a 502/400 based on the node version or a 504 timeout)

See #25528 and #25605 which changes the response to 431.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make HTTP_MAX_HEADER_SIZE configurable