Update http parser 2.9.1 v10.x #30471

sam-github · 2019-11-13T18:35:17Z

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

sam-github · 2019-11-17T18:23:55Z

We might not want to do this, there's a reported bug against http-parser 2.9.x: #30515

BridgeAR · 2020-01-07T20:15:33Z

@sam-github could maybe the crypto or security team give a recommendation if this should be included in v10.x or not? Seems like not including it is not that safe?

sam-github · 2020-01-07T20:34:35Z

Should not be included until it can be released with a backport of #30567

sam-github · 2020-01-07T22:30:28Z

#31253 should also be backported onto this, I'll do it once it has been approved.

nodejs-github-bot · 2020-01-07T22:44:20Z

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

Reapplying HTTP_MAX_HEADER_SIZE=8192 to http_parser.gyp. CVE-2018-12121 PR-URL: https://github.com/nodejs-private/node-private/pull/143 Ref: https://github.com/nodejs-private/security/issues/139 Ref: https://github.com/nodejs-private/http-parser-private/pull/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]>

sam-github · 2020-01-10T00:16:45Z

Pulled in the test from #30473 (comment), but removed the test I just added for the legacy parser.... it only needs the original default test, because v10.x only has the one parser.

nodejs-github-bot · 2020-01-10T00:18:01Z

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

nodejs-github-bot · 2020-01-10T00:23:43Z

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

Allow insecure HTTP header parsing. Make clear it is insecure. See: - nodejs#30553 - nodejs#27711 (comment) - nodejs#30515 PR-URL: nodejs#30567 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]>

Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs#30567 PR-URL: nodejs#31253 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot · 2020-01-10T01:20:39Z

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

nodejs-github-bot · 2020-01-10T05:48:56Z

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

sam-github · 2020-01-10T14:56:57Z

Despited what GH shows above, the ci was green, this is ready to land on v10.x-staging:

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

nodejs-github-bot · 2020-01-10T16:40:26Z

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

richardlau · 2020-01-10T16:46:24Z

Despited what GH shows above, the ci was green,

This is a bug in the way our builds post status to GitHub PRs -- in the case of the containered *sharedlibs builds they only post when the build fails (notice they don't show up otherwise as passing/pending) so resumes/rebuilds don't overwrite the failing status. I'll raise an issue over in build later on and see what we can do to fix it.

richardlau · 2020-01-10T16:49:16Z

@sam-github Would this be considered semver-minor because of potential breakage of apps that previously accepted invalid headers without using the new non-default flag?

sam-github · 2020-01-10T17:11:03Z

Its semver-major, but since its a security issue, policy allows releasing it in LTS anyway.

It could be called semver-minor, but that delays release, unless we want to push it out early as a sec release?

When the CLI option is used, it becomes semver-patch.

I just searched our docs, and I can't find where the sec release policy is stated, though I'm pretty sure it used to be. Probably I'm just not finding it.

richardlau · 2020-01-10T17:16:08Z

Just to be clear, I'm not too hung up on the semverness of this, but would just like to make sure it has been considered.

sam-github · 2020-01-10T18:00:44Z

I'm sure you are not! :-) But we would like to do the right/documented thing, though, this won't be the last time we make a breaking change in LTS for sec purposes.

@nodejs/lts please weigh in on what semver tag this should get, what has been done in the past?

I poked about a bit, for example https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V010.md#2016-09-27-version-01047-maintenance-rvagg made breaking sec changes, but the PRs were private and didn't get semver tags:

So, that might be the pattern, not assign semverity/leave it at default (semver-patch).

richardlau · 2020-01-10T18:10:15Z

FWIW the relevant policy is in the Release readme which says this should be semver-minor:

Note that while it is possible that critical security and bug fixes may lead to
semver-major changes landing within an LTS stream, such situations will be
rare and will land as semver-minor bumps in the LTS covered version.

What isn't stated in the policy is whether if this is done for security purposes the release should be labelled as a security release.

sam-github · 2020-01-13T19:33:39Z

@nodejs/lts opinion on semverity?

MylesBorins · 2020-01-13T19:58:16Z

I would say semver minor makes sense. The next 10.x release is semver-minor so it seems reasonable

MylesBorins · 2020-01-13T20:01:29Z

Also from a process perspective I'm -1 on breaking changes landing as patch. We can always do an out of band semver minor if it is time sensitive. Folks shouldn't have things change from under them in a semver-patch release imho. Doing it as a minor makes it a slightly more explicit upgrade pathl.

BethGriggs · 2020-01-14T10:43:58Z

What isn't stated in the policy is whether if this is done for security purposes the release should be labelled as a security release.

I think it should be labelled as a security release, but as we don't have that specific policy documented (yet) ping @nodejs/lts for opinions.

If it is labelled as a security release (and includes breaking changes) - should we be adding new features in the same release?

richardlau · 2020-01-16T18:27:19Z

What isn't stated in the policy is whether if this is done for security purposes the release should be labelled as a security release.

I think it should be labelled as a security release, but as we don't have that specific policy documented (yet) ping @nodejs/lts for opinions.

If it is labelled as a security release (and includes breaking changes) - should we be adding new features in the same release?

I'd support anything released under the policy (breaking changes as semver-minor for security reasons) to be labelled as a security release and for security releases to not contain other non-security features/fixes (other than any necessary test/build changes to ensure the builds still work).

From the issue: > Some servers deviate from HTTP spec enougth that Node.js can't > communicate with them, but "work" when `--insecure-http-parser` > is enabled globally. It would be useful to be able to use this > mode, as a client, only when connecting to known bad servers. This is largely equivalent to nodejs#31446 in terms of code changes. Fixes: nodejs#31440 Refs: nodejs#31446 PR-URL: nodejs#31448 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

nodejs-github-bot · 2020-01-24T20:37:23Z

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

nodejs-github-bot · 2020-01-24T23:24:08Z

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

sam-github · 2020-01-27T19:15:37Z

this backport also has #31500 pulled in, since it depends on it.

sam-github · 2020-02-07T18:13:51Z

Landed in a28e5cc a9849c0 d616722 0082f62 49f4220 via security release.

Ported from OpenSUSE:nodejs8-8.17.0-lp152.147.1:CVE-2019-15605.patch Original commit message: commit e2c8f89 Author: Sam Roberts <[email protected]> Date: Thu Jan 16 11:55:52 2020 -0800 test: using TE to smuggle reqs is not possible See: https://hackerone.com/reports/735748 PR-URL: https://github.com/nodejs-private/node-private/pull/192 Reviewed-By: Beth Griggs <[email protected]> commit 49f4220 Author: Sam Roberts <[email protected]> Date: Tue Feb 4 10:36:57 2020 -0800 deps: upgrade http-parser to v2.9.3 PR-URL: https://github.com/nodejs-private/http-parser-private/pull/4 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]> commit d616722 Author: Sam Roberts <[email protected]> Date: Tue Jan 7 14:24:54 2020 -0800 test: check that --insecure-http-parser works Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs#30567 Backport-PR-URL: nodejs#30471 PR-URL: nodejs#31253 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> commit a9849c0 Author: Sam Roberts <[email protected]> Date: Wed Nov 20 11:48:58 2019 -0800 http: opt-in insecure HTTP header parsing Allow insecure HTTP header parsing. Make clear it is insecure. See: - nodejs#30553 - nodejs#27711 (comment) - nodejs#30515 Backport-PR-URL: nodejs#30471 PR-URL: nodejs#30567 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> commit a28e5cc Author: Sam Roberts <[email protected]> Date: Wed Nov 13 10:05:38 2019 -0800 deps: upgrade http-parser to v2.9.1 PR-URL: nodejs#30471 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Signed-off-by: Su Baocheng <[email protected]>

nodejs-github-bot added http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. v10.x labels Nov 13, 2019

sam-github mentioned this pull request Nov 13, 2019

Update http parser 2.9.1 v12.x #30473

Closed

4 tasks

gengjiawen approved these changes Nov 15, 2019

View reviewed changes

richardlau mentioned this pull request Nov 17, 2019

HPE_INVALID_HEADER_TOKEN on https requests #30515

Closed

mscdex added the wip Issues and PRs that are still a work in progress. label Nov 17, 2019

MylesBorins force-pushed the v10.x-staging branch from 99915bc to 8f3b8ca Compare December 17, 2019 22:08

BridgeAR removed the wip Issues and PRs that are still a work in progress. label Jan 7, 2020

sam-github added the dont-land-on-v10.x label Jan 7, 2020

BridgeAR added blocked PRs that are blocked by other issues or PRs. and removed dont-land-on-v10.x labels Jan 7, 2020

sam-github force-pushed the update-http_parser-2.9.1-v10.x branch from d86dbab to 27e1c28 Compare January 7, 2020 21:26

sam-github and others added 2 commits January 9, 2020 16:13

deps: upgrade http-parser to v2.9.1

8e0206f

sam-github force-pushed the update-http_parser-2.9.1-v10.x branch from e9bd6b9 to e6b0d25 Compare January 10, 2020 00:15

sam-github removed the blocked PRs that are blocked by other issues or PRs. label Jan 10, 2020

sam-github added 2 commits January 9, 2020 17:03

test: check that --insecure-http-parser works

58ec670

Test that using --insecure-http-parser will disable validation of invalid characters in HTTP headers. See: - nodejs#30567 PR-URL: nodejs#31253 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

sam-github force-pushed the update-http_parser-2.9.1-v10.x branch from d8b167e to 58ec670 Compare January 10, 2020 01:07

jasnell approved these changes Jan 10, 2020

View reviewed changes

richardlau approved these changes Jan 10, 2020

View reviewed changes

sam-github mentioned this pull request Jan 13, 2020

suggestion: that nodejs release information be moved to nodejs/node nodejs/Release#522

Closed

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 13, 2020

BethGriggs approved these changes Jan 14, 2020

View reviewed changes

addaleax mentioned this pull request Jan 24, 2020

http: make --insecure-http-parser configurable per-stream or per-server #31448

Closed

4 tasks

sam-github closed this Feb 7, 2020

sam-github deleted the update-http_parser-2.9.1-v10.x branch February 7, 2020 18:13

Uh oh!

Uh oh!

Update http parser 2.9.1 v10.x #30471

Update http parser 2.9.1 v10.x #30471

Uh oh!

Conversation

sam-github commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

sam-github commented Nov 17, 2019

Uh oh!

BridgeAR commented Jan 7, 2020

Uh oh!

sam-github commented Jan 7, 2020

Uh oh!

sam-github commented Jan 7, 2020

Uh oh!

nodejs-github-bot commented Jan 7, 2020

Uh oh!

sam-github commented Jan 10, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

sam-github commented Jan 10, 2020

Uh oh!

nodejs-github-bot commented Jan 10, 2020

Uh oh!

richardlau commented Jan 10, 2020

Uh oh!

richardlau commented Jan 10, 2020

Uh oh!

sam-github commented Jan 10, 2020

Uh oh!

richardlau commented Jan 10, 2020

Uh oh!

sam-github commented Jan 10, 2020

Uh oh!

richardlau commented Jan 10, 2020

Uh oh!

sam-github commented Jan 13, 2020

Uh oh!

MylesBorins commented Jan 13, 2020

Uh oh!

MylesBorins commented Jan 13, 2020

Uh oh!

BethGriggs commented Jan 14, 2020

Uh oh!

richardlau commented Jan 16, 2020

Uh oh!

nodejs-github-bot commented Jan 24, 2020

Uh oh!

nodejs-github-bot commented Jan 24, 2020

Uh oh!

sam-github commented Jan 27, 2020

Uh oh!

sam-github commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

sam-github commented Nov 13, 2019 •

edited

Loading