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

[v12.x] http: make --insecure-http-parser configurable per-stream or per-server #31500

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jan 24, 2020

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 #31446
in terms of code changes.

(Side note: Not changing the commit message for the backport, but this should have been #30570 and not #31446.)

Fixes: #31440
Refs: #31446

PR-URL: #31448

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

cjihrig and others added 30 commits January 13, 2020 16:40
Notable changes:

- Fix handling of large files in uv_fs_copyfile().
  Fixes: nodejs#30085
- Fix Android build errors.
- uv_sleep() has been added.
- uv_interface_addresses() IPv6 netmask support has been fixed.
  Fixes: nodejs#30504
- uv_fs_mkstemp() has been added.

PR-URL: nodejs#30783
Fixes: nodejs#30085
Fixes: nodejs#30504
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: nodejs#30787
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
This commit replaces common.busyLoop() with sleep().

PR-URL: nodejs#30787
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Current CC flag points to g++ instead of gcc which is causing failures
when compiling V8.

PR-URL: nodejs#30817
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
There was no point in lazy loading the string_decoder, since it
would be used in all cases anyway.

PR-URL: nodejs#30807
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#30838
Refs: nodejs#30786
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
The extra headers are needed when deprecated APIs are disabled as ssl.h
no longer includes them implicitly.

PR-URL: nodejs#30812
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This implements a memory-tracking allocator that can be used to
provide memory allocation facilities to several thread-safe C
libraries, including nghttp2, nghttp3, ngtcp3 and uvwasi.

Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Co-authored-by: James M Snell <[email protected]>

PR-URL: nodejs#30745
Refs: nodejs/quic#126
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
The extensive testing done on http2 makes it easier to make sure
the implementation is correct (and doesn’t diverge unnecessarily).

Refs: nodejs/quic#126
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#30745
Refs: https://github.com/nodejs/quic/blob/34ee0bc96f804c73cb22b2945a1a78f780938492/src/node_mem.h
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Documents the existence and purpose of the `line` and `cursor`
properties.  `line` can be used for reading the current input value
during runtime, if reading from a TTY stream.  Both properties are
necessary when developing a custom CLI input process using `readline`
as a backend.

Refs: nodejs#30347
Refs: DefinitelyTyped/DefinitelyTyped#40513

PR-URL: nodejs#30667
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#30848
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Addon developers may run into this when not closing libuv handles
inside Workers.

Previously, output may have included unhelpful statements such as
`uv loop at ... has 0 active handles`, which may sound like
everything’s supposed to be fine actually.

So, instead of printing the active handle count, print the total
handle count and mark active handles individually.

Also, fix the test for this to work properly and make sure that
parsing finishes properly.

PR-URL: nodejs#30814
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
The test was using fixed timeouts and that seems to be causing sporadic
test failures on pi1-docker host (which is a very slow machine.)

Fixes: nodejs#30828

PR-URL: nodejs#30834
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
This commit gives the synchronous version of rimraf the same
linear retry logic as the asynchronous version. Prior to this
commit, sync rimraf kept retrying the operation as soon as
possible until maxRetries was reached.

PR-URL: nodejs#30785
Fixes: nodejs#30580
Refs: nodejs#30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rimraf should only retry if certain errors are encountered.
Additionally, there is no point sleeping if an error occurs
on the last try.

PR-URL: nodejs#30785
Fixes: nodejs#30580
Refs: nodejs#30569
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
On IBMi PASE isatty() always returns true for stdin, stdout and stderr.
Use ioctl() instead to identify whether it's actually a TTY.

PR-URL: nodejs#30829
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#29943
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Document arguments for the 'frameError', 'timeout' and 'trailers'
event.

PR-URL: nodejs#30373
Fixes: nodejs/help#877
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Implicitly pretending being FreeBSD and disable
large pages for this platform.

PR-URL: nodejs#30201
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  * Let users provide more than one pattern by repeating the flag
  * Add new flag --exclude to exclude patterns
  * Add tests for --filter
  * Document --filter

This commit also fixes a bug where things like
`compare.js --new --old binary --new binary` was acceptable (now the
script will exit and print the usage message).

PR-URL: nodejs#29987
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Clarify the explanation of Tier 2 platforms and binary releases.

PR-URL: nodejs#30866
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Refs: nodejs#30281 (comment)

PR-URL: nodejs#30509
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Compiling a library with -fPIE won't do, and on Android libraries
are not versioned.

PR-URL: nodejs#29388
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
And other errors like lost promises

PR-URL: nodejs#29388
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#30863
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#30840
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Disable colorMode in test-console-group so that the test will succeed if
run without the test runner from the command line.

PR-URL: nodejs#30886
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Add information about what it means when colorMode is set to false.

PR-URL: nodejs#30887
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
telenord and others added 14 commits January 14, 2020 16:56
PR-URL: nodejs#30284
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
This also allows us to remove backslash escaping for `[` and `]`
inside of header code, which makes the bare markdown more readable.

Refs: nodejs#31086

PR-URL: nodejs#31149
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#31186
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
The API caught errors from inside of the users passed through callback.
This never caused any issues, since this API is only used internally.
Otherwise it would have potentially hidden bugs in user code.

Refs: nodejs#31133

PR-URL: nodejs#31159
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: nodejs#31049
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
To avoid problem with the behavior of new V8 BackingStore API,
change the offset. The base address of each test case will be
different.

Fixes: nodejs#31061

PR-URL: nodejs#31171
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#31175
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: David Carlier <[email protected]>
PR-URL: nodejs#31185
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.

PR-URL: nodejs#31172
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: David Carlier <[email protected]>
More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: nodejs#30339 (comment)

PR-URL: nodejs#30475
Backport-PR-URL: nodejs#31355
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Invoke the free callback for a given `Buffer` if it was created
with one, and mark the underlying `ArrayBuffer` as detached.

This makes sure that the memory is released e.g. when addons inside
Workers create such `Buffer`s.

PR-URL: nodejs#30551
Backport-PR-URL: nodejs#31355
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Original commit message:

    S390x: improve performance by skipping Debug Hook if not needed

    Change-Id: Ib4b2821f2941cdc131f9c75b89a3baced7554f8d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1991802
    Reviewed-by: Junliang Yan <[email protected]>
    Commit-Queue: Milad Farazmand <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65644}

Refs: v8/v8@d89f4ef

PR-URL: nodejs#31354
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
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 nodejs-github-bot added http Issues or PRs related to the http subsystem. v12.x labels Jan 24, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 24, 2020

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

LGTM

@sam-github sam-github mentioned this pull request Jan 27, 2020
4 tasks
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

addaleax commented Feb 7, 2020

This has landed as part of the security release

@addaleax addaleax closed this Feb 7, 2020
@addaleax addaleax deleted the backport-insecure-http-parser-v12 branch February 7, 2020 23:48
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 11, 2020
Backport 7fc5656

Original commit message:

    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/node#31446 in terms of code changes.

    Fixes: nodejs/node#31440
    Refs: nodejs/node#31446

    Backport-PR-URL: nodejs/node#31500
    PR-URL: nodejs/node#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]>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
Backport 7fc5656

Original commit message:

    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/node#31446 in terms of code changes.

    Fixes: nodejs/node#31440
    Refs: nodejs/node#31446

    Backport-PR-URL: nodejs/node#31500
    PR-URL: nodejs/node#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]>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
Backport 7fc5656

Original commit message:

    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/node#31446 in terms of code changes.

    Fixes: nodejs/node#31440
    Refs: nodejs/node#31446

    Backport-PR-URL: nodejs/node#31500
    PR-URL: nodejs/node#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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.