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.

Sign up for GitHub

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

Jump to bottom

Update http parser 2.9.1 v10.x #30471

Closed
wants to merge 5 commits into from
Closed

Update http parser 2.9.1 v10.x #30471

wants to merge 5 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 13, 2019 •
edited
Loading

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 http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. v10.x labels Nov 13, 2019
@sam-github sam-github mentioned this pull request Nov 13, 2019
Closed
4 tasks
@richardlau richardlau mentioned this pull request Nov 17, 2019
Closed
@sam-github
Copy link
Contributor Author

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

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

BridgeAR commented Jan 7, 2020

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

@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Jan 7, 2020
@sam-github
Copy link
Contributor Author

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

@BridgeAR 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 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
Copy link
Contributor Author

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

@nodejs-github-bot
Copy link
Collaborator

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

@sam-github 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
Copy link
Contributor Author

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.

@sam-github sam-github removed the blocked PRs that are blocked by other issues or PRs. label Jan 10, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@sam-github
http: opt-in insecure HTTP header parsing
056d8e5 
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]>
@sam-github
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 sam-github force-pushed the update-http_parser-2.9.1-v10.x branch from d8b167e to 58ec670 Compare January 10, 2020 01:07
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@sam-github
Copy link
Contributor Author

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

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

@richardlau
Copy link
Member

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

@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
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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

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 sam-github mentioned this pull request Jan 13, 2020
Closed
@sam-github
Copy link
Contributor Author

@nodejs/lts opinion on semverity?

@MylesBorins
Copy link
Contributor

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

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

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

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

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).

@addaleax
http: make --insecure-http-parser configurable per-stream or per-server
22525e2 
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
Copy link
Collaborator

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

@addaleax addaleax mentioned this pull request Jan 24, 2020
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@sam-github
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor Author

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

@sam-github sam-github closed this Feb 7, 2020
@sam-github sam-github deleted the update-http_parser-2.9.1-v10.x branch February 7, 2020 18:13
BaochengSu added a commit to BaochengSu/node that referenced this pull request Oct 21, 2020
@BaochengSu
CVE-2019-15605
a5c5539 
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]>
BaochengSu added a commit to BaochengSu/node that referenced this pull request Jul 14, 2022
@BaochengSu
CVE-2019-15605
65ab2f5 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@richardlau richardlau richardlau approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.