cli: add --max-http-header-size flag by cjihrig · Pull Request #24811 · nodejs/node

cjihrig · 2018-12-03T17:33:00Z

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 · 2018-12-03T17:33:04Z

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1807/pipeline

mcollina · 2018-12-03T18:05:15Z

Is there a matching PR for http-parser?

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

mcollina · 2018-12-03T18:05:45Z

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

cjihrig · 2018-12-03T18:12:02Z

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

sam-github · 2018-12-03T18:23:38Z

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

src/node_options.h

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 <apapirovski@mac.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Anna Henningsen <anna@addaleax.net>

mcollina

LGTM

dglozic · 2018-12-04T18:54:07Z

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 · 2018-12-05T23:11:48Z

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 · 2018-12-05T23:26:05Z

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

cjihrig · 2018-12-06T01:10:05Z

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

Opened #24860 to discuss separately.

mcollina · 2018-12-07T10:00:12Z

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

mcollina · 2018-12-10T18:28:54Z

@cjihrig what's the status of this?

This commit adds support for uint64_t option parsing. Backport-PR-URL: #25168 PR-URL: #24811 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Allow the maximum size of HTTP headers to be overridden from the command line. Backport-PR-URL: #25168 co-authored-by: Matteo Collina <hello@matteocollina.com> PR-URL: #24811 Fixes: #24692 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

lxe · 2019-01-10T00:06:51Z

Thanks for this!

pharatesid · 2019-01-30T06:47:47Z

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

losdevnull · 2019-01-30T10:21:13Z

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

mcollina · 2019-02-02T14:15:38Z

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

losdevnull · 2019-02-02T14:19:23Z

@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!

JSMike · 2019-02-04T18:27:41Z

src/node_http_parser_impl.h


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


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?

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

mike503 · 2019-02-22T00:15:38Z

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 · 2019-02-22T01:53:42Z

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.

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

richardlau mentioned this pull request Dec 3, 2018

build: make HTTP_MAX_HEADER_SIZE configurable #24716

Closed

2 tasks

addaleax approved these changes Dec 3, 2018

View reviewed changes

src/node_http_parser.cc Outdated Show resolved Hide resolved

Fishrock123 reviewed Dec 3, 2018

View reviewed changes

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

bnoordhuis reviewed Dec 3, 2018

View reviewed changes

src/node_options.h Outdated Show resolved Hide resolved

mcollina approved these changes Dec 4, 2018

View reviewed changes

orangejulius mentioned this pull request Dec 5, 2018

Support Travis VM Infrastructure pelias/schema#337

Closed

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

cjihrig mentioned this pull request Dec 6, 2018

http: add maxHeaderSize property #24860

Merged

4 tasks

cjihrig force-pushed the max-headers-size branch from 3275c64 to 67e5595 Compare December 6, 2018 01:09

mcollina added lts-watch-v6.x labels Dec 10, 2018

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

This was referenced Dec 21, 2018

v10.15.0 proposal #25176

Merged

v8.15.0 proposal #25177

Merged

v6.16.0 proposal #25178

Merged

spalger mentioned this pull request Dec 22, 2018

Upgrade to node 10.15/8.15 elastic/kibana#27724

Closed

missinglink mentioned this pull request Jan 4, 2019

Please update Pelias software requirements doc with the supported Node.js versions pelias/pelias#765

Closed

hyj1991 mentioned this pull request Jan 8, 2019

feat: fix unexpected breakages which 8.14.0 security release introduced cnpm/bug-versions#34

Closed

figureone mentioned this pull request Jan 9, 2019

Upstream prematurely closed connection while reading response header from upstream LearningLocker/learninglocker#1166

Closed

hoffmc mentioned this pull request Jan 12, 2019

Enable setting max http header size via environment variable cloudfoundry/nodejs-buildpack#176

Closed

2 tasks

JSMike reviewed Feb 4, 2019

View reviewed changes

heyimalex mentioned this pull request Mar 13, 2019

How to set max http header size in create react app facebook/create-react-app#6582

Closed

flotwig mentioned this pull request Jul 29, 2019

Electron v5.0.10 cypress-io/cypress#4720

Merged

flotwig mentioned this pull request Oct 24, 2019

431 status code on stubbed XHR requests with large response in 3.5.0 cypress-io/cypress#5431

Closed

sam-github mentioned this pull request Nov 20, 2019

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

Closed

4 tasks

cben mentioned this pull request May 9, 2021

Silent 400 error when headers exceed 8KiB RedHatInsights/insights-proxy#38

Open

3 tasks

Uh oh!

Conversation

cjihrig commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Dec 3, 2018

Uh oh!

mcollina commented Dec 3, 2018

Uh oh!

mcollina commented Dec 3, 2018

Uh oh!

cjihrig commented Dec 3, 2018

Uh oh!

Uh oh!

sam-github commented Dec 3, 2018

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

dglozic commented Dec 4, 2018

Uh oh!

cjihrig commented Dec 5, 2018

Uh oh!

mcollina commented Dec 5, 2018

Uh oh!

cjihrig commented Dec 6, 2018

Uh oh!

mcollina commented Dec 7, 2018

Uh oh!

mcollina commented Dec 10, 2018

Uh oh!

lxe commented Jan 10, 2019

Uh oh!

pharatesid commented Jan 30, 2019

Uh oh!

losdevnull commented Jan 30, 2019

Uh oh!

mcollina commented Feb 2, 2019

Uh oh!

losdevnull commented Feb 2, 2019

Uh oh!

JSMike Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

cjihrig Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

mike503 commented Feb 22, 2019

Uh oh!

richardlau commented Feb 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

cjihrig commented Dec 3, 2018 •

edited

Loading