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

Object.assign / spread of http request object difference between v14.15.1 and v14.15.2 #36550

Closed
rubenstolk opened this issue Dec 17, 2020 · 21 comments
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem.

Comments

@rubenstolk
Copy link

rubenstolk commented Dec 17, 2020

  • Version: 14.15.2
  • Platform: any
  • Subsystem: any

What steps will reproduce the bug?

While trying to create a cloned version of an http request object, prototype properties/methods such as headers and get get lost.

const assert = require('assert');
const http = require('http');

const server = http
  .createServer((req, res) => {
    const dummyReq = { ...req };
    res.writeHead(200, { 'Content-Type': 'text/plain' });
    res.end('ok');
    assert.deepStrictEqual(req.headers, dummyReq.headers);
  })
  .listen();

server.on('listening', () => {
  http.get(`http://localhost:${server.address().port}`);
});

What is the expected behavior?

I honestly don't know if the behavior from 14.15.1 or from 14.15.2 is expected.

Behavior until 14.15.1: dummyReq.headers is not undefined.

What do you see instead?

Output in 14.15.2: dummyReq.headers is undefined.

Additional info

The same happens while using Object.assign

@rubenstolk
Copy link
Author

As @ExE-Boss indicated here (#36023 (comment)), the http request object must now be of a different inheritance level...

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Dec 17, 2020

Object.assign also copies only own enumerable properties, but uses [[Set]] instead of [[Define]], so:

const target = {};
Object.assign(
	target,
	JSON.parse(`{
		"__proto__": null
	}`,
);

results in target having its prototype set to null instead of adding an own __proto__ property, because of the Object.prototype.__proto__ accessor.


Whereas:

const target = {
	...(JSON.parse(`{
		"__proto__": null
	}`)),
};

Results in an object with an own __proto__ property set to null and a prototype of Object.prototype.

@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2020

This likely related to #35281.

@ExE-Boss
Copy link
Contributor

This is definitely caused by #35281.

@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2020

I think a quick fix would be to do something like:

const dummyReq = { ...req, get headers() { return req.headers }, get trailers() { return req.trailers; } };

@rubenstolk
Copy link
Author

Yep, thanks for that! I have already implemented something similar.

@Flarna Flarna added http Issues or PRs related to the http subsystem. confirmed-bug Issues with confirmed bugs. labels Dec 17, 2020
@BethGriggs
Copy link
Member

Opinions on whether this warrants an urgent revert of #35281 and follow-up patch release this week? cc: @ronag @mcollina

(I'd need to get started on that today if so, as i'll be out-of-office/mostly offline from tomorrow until the New Year.)

@mcollina
Copy link
Member

I'd just revert it in v14, not v15.

@mhdawson
Copy link
Member

I agree with @mcollina that a revert in v14 would amke sense, and it would be good to get a release out.

@BethGriggs
Copy link
Member

Raised a revert PR for Node.js 14 (#36553). Hope to get this into a v14.15.3 very soon so that there are no barriers to adopting the upcoming security release on January 4th.

@mcollina
Copy link
Member

I've investigated this with a bit more detail, and I don't think it's a bug. I would recommend against using the spread operator on any stream. Nevertheless, this should be reverted in v14.x as it was probably a significant breaking change that slipped in a patch release.

BethGriggs added a commit that referenced this issue Dec 17, 2020
This reverts commit b58725c.

Fixes: #36550

PR-URL: #36553
Refs: #35281
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs added a commit that referenced this issue Dec 17, 2020
This reverts commit b58725c.

Fixes: #36550

PR-URL: #36553
Refs: #35281
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs added a commit that referenced this issue Dec 17, 2020
Notable Changes:

Node.js v14.15.2 included a commit that has caused reported breakages
when cloning request objects. This release reverts the commit that
introduced the behaviour change.

See #36550 for more details.

PR-URL: #36555
BethGriggs added a commit that referenced this issue Dec 17, 2020
Notable Changes:

Node.js v14.15.2 included a commit that has caused reported breakages
when cloning request objects. This release reverts the commit that
introduced the behaviour change.

See #36550 for more details.

PR-URL: #36555
@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2020

v14.15.3 has been released reverting the behaviour change. I think we can close this.

Thanks a lot for the report and the repro code @rubenstolk!

@aduh95 aduh95 closed this as completed Dec 17, 2020
@Flarna
Copy link
Member

Flarna commented Dec 17, 2020

How do we continue regarding this in NodeJs 15? The same behavior change is between 15.0.0 and 15.1.0.

Should we document this somehow?
IMHO such a change doesn't match to semver-patch.

@rubenstolk
Copy link
Author

I've investigated this with a bit more detail, and I don't think it's a bug. I would recommend against using the spread operator on any stream. Nevertheless, this should be reverted in v14.x as it was probably a significant breaking change that slipped in a patch release.

Totally agree here!

@mcollina
Copy link
Member

How do we continue regarding this in NodeJs 15? The same behavior change is between 15.0.0 and 15.1.0.

This change have been out for more than a month and I would not revert it there. I think it would create more friction to revert it there.

Should we document this somehow?
IMHO such a change doesn't match to semver-patch.

I would document that { ...stream }, { ...req }, { ...res } and all these ways that copies properties should not be used. These objects are essentially uncloneable.

@Flarna
Copy link
Member

Flarna commented Dec 18, 2020

This change have been out for more than a month and I would not revert it there. I think it would create more friction to revert it there.

Well, it's an uneven version and still quite new. More or less any production setup runs on LTS versions only.
Looking into the usage of our customers I see following distribution:
14: 4%
12: 39%
10: 37%
8: 19%

Node 15 (and other uneven versions) are more or less not existing there. So it's likely that we will see this popping up with 16.

@mcollina
Copy link
Member

Node 15 (and other uneven versions) are more or less not existing there. So it's likely that we will see this popping up with 16.

What would you recommend then?

@aduh95
Copy link
Contributor

aduh95 commented Dec 18, 2020

If #35281 had been labeled semver-major (as it would have been safe to do in retrospective), it would have landed in v16 anyway. Breaking changes are fine in semver major releases, I don't think we should be worried about v16.

Note that if it was reverted on v15, the revert would not be a breaking change, I don't think it would result in friction. I'm fine letting it slide for v15 though, the fact that we didn't get any bug report since it shipped seems to show it doesn't really matter there.

@Flarna
Copy link
Member

Flarna commented Dec 18, 2020

I think marking semver-major would be the usual approach if this change is really needed.

According to #35281 (review) the change is mostly a nop from performance point of view therefore I think reverting this also on master is also an option.

But if general consensus is to keep it in 15 and master as is I'm also ok.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Dec 21, 2020
Version 14.15.3 'Fermium' (LTS)

Notable Changes

Node.js v14.15.2 included a commit that has caused reported breakages when cloning request objects. This release reverts the commit that introduced the behaviour change. See nodejs/node#36550 for more details.
uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Dec 22, 2020
https://nodejs.org/en/blog/release/v14.15.3/

Node.js v14.15.2 included a commit that has caused reported breakages
when cloning request objects. This release reverts the commit that
introduced the behaviour change. See
nodejs/node#36550 for more details.

Sponsored by:	Miles AS


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@558895 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Dec 22, 2020
https://nodejs.org/en/blog/release/v14.15.3/

Node.js v14.15.2 included a commit that has caused reported breakages
when cloning request objects. This release reverts the commit that
introduced the behaviour change. See
nodejs/node#36550 for more details.

Sponsored by:	Miles AS
Jehops pushed a commit to Jehops/freebsd-ports-legacy that referenced this issue Dec 22, 2020
https://nodejs.org/en/blog/release/v14.15.3/

Node.js v14.15.2 included a commit that has caused reported breakages
when cloning request objects. This release reverts the commit that
introduced the behaviour change. See
nodejs/node#36550 for more details.

Sponsored by:	Miles AS


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@558895 35697150-7ecd-e111-bb59-0022644237b5
Trott pushed a commit to ExE-Boss/node that referenced this issue Dec 25, 2020
Refs: nodejs#35281
Refs: nodejs#36550

Co-authored-by: raisinten <raisinten@gmail.com>

PR-URL: nodejs#36601
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Refs: #35281
Refs: #36550

Co-authored-by: raisinten <raisinten@gmail.com>

PR-URL: #36601
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
dafortune added a commit to dafortune/hawk that referenced this issue Feb 25, 2022
Due to nodejs/node#36550 (it was reverted in
Node 14 but it is back in Node 16). req.headers and other properties are
not own properties of requests so they are not cloned.

Node documentation (see discussion on nodejs/node#36550)
advices against clonning the req (or any stream object). This PR fixes that
without modifying the original request object by creating a new object which
prototype is the original request object instead of trying to clone
it.
dafortune added a commit to dafortune/passport-hawk that referenced this issue Feb 25, 2022
Due to nodejs/node#36550 (it was reverted in
Node 14 but it is back in Node 16). req.headers and other properties are
not own properties of requests so they are not cloned.

Node documentation (see discussion on nodejs/node#36550)
advices against clonning the req (or any stream object). This PR fixes that
without modifying the original request object by creating a new object which
prototype is the original request object instead of trying to clone
it.
dafortune added a commit to dafortune/passport-hawk that referenced this issue Feb 25, 2022
Due to nodejs/node#36550 (it was reverted in
Node 14 but it is back in Node 16). req.headers and other properties are
not own properties of requests so they are not cloned.

Node documentation (see discussion on nodejs/node#36550)
advices against clonning the req (or any stream object). This PR fixes that
without modifying the original request object by creating a new object which
prototype is the original request object instead of trying to clone
it.
joebonrichie pushed a commit to solus-packages/nodejs that referenced this issue Aug 15, 2023
Node.js v14.15.2 included a commit that has caused reported breakages when cloning request objects. This release reverts the commit that introduced the behaviour change. See nodejs/node#36550 for more details.
kyptin added a commit to kyptin/paperplane that referenced this issue Dec 12, 2023
I tried the basic example in the README. It failed with a 500
Internal Server Error and the following JSON body:

    {
        "message": "Cannot read properties of undefined (reading 'x-forwarded-proto')",
        "name": "TypeError"
    }

The output from the server process includes a stack trace.
Sanitized a bit, it is:

    TypeError: Cannot read properties of undefined (reading 'x-forwarded-proto')
        at protocol (.../node_modules/paperplane/lib/parseUrl.js:18:14)
        at .../node_modules/ramda/src/converge.js:47:17
        at _map (.../node_modules/ramda/src/internal/_map.js:6:19)
        at .../node_modules/ramda/src/converge.js:46:33
        at f1 (.../node_modules/ramda/src/internal/_curry1.js:18:17)
        at .../node_modules/ramda/src/uncurryN.js:34:21
        at .../node_modules/ramda/src/internal/_curryN.js:37:27
        at .../node_modules/ramda/src/internal/_arity.js:10:19
        at .../node_modules/ramda/src/internal/_pipe.js:3:14
        at .../node_modules/ramda/src/internal/_arity.js:10:19

Yet the docker-based demo app works. That uses Node.js v12.22.12.
When I tried that Node version on the basic example, it also worked.

So, I used `nvm` to do a binary search on versions. I identified
that Node.js v15.1.0 is the first failing version, and every version
I tried that was newer (up to v21.2.0) also failed.

Scouring the Node.js change log revealed that the http module of
v15.1.0 started calculating req.headers lazily. There's a full
discussion in nodejs/node#35281 [1]. Note the referenced issue that
identifies the bug [2].

[1] nodejs/node#35281
[2] nodejs/node#36550

The workaround identified in this comment [3] is not to use the
spread operator or `Object.assign` on the request object. "These
objects are essentially uncloneable."

[3] nodejs/node#36550 (comment)

This is surprisingly tricky to do right. Various Ramda functions
like `merge` (and I think `converge`) do this implicitly, as does
funky's `assocWith`. So to work around this limitation, while
preserving all behavior, I had to resort to (gasp) mutating
procedures instead of pure functions. Not ideal, I know. I'm open to
better ideas.

So, maybe this isn't the best solution, but it least it gets the
example working again on modern Node versions. If it never gets
merged, at least it will be findable via the repo on GitHub.

For reference, to test this, I used a fresh NPM project with only
the paperplane dependency:

    mkdir paperplane
    cd paperplane
    npm init -y
    npm i -S paperplane

In which I added an index.js file with these contents:

    const { compose } = require('ramda')
    const http = require('http')
    const { json, logger, methods, mount, routes } = require('paperplane')

    const cookies = req => ({ cookies: req.cookies || 'none?' })
    const hello   = req => ({ message: `Hello ${req.params.name}!` })

    const app = routes({
      '/'           : methods({ GET: _ => ({ body: 'Hello anon.' }) }),
      '/cookies'    : methods({ GET: compose(json, cookies) }),
      '/hello/:name': methods({ GET: compose(json, hello) })
    })

    http.createServer(mount({ app })).listen(3000, logger)

And finally (with `fd` and `entr` and `httpie` installed), I started
a file-watching auto-test process:

    fd -e js | entr -rcc bash -c "node index.js & http -v get http://localhost:3000/cookies Cookie:foo=bar"
kyptin added a commit to kyptin/paperplane that referenced this issue Dec 12, 2023
I tried the basic example in the README. It failed with a 500
Internal Server Error and the following JSON body:

    {
        "message": "Cannot read properties of undefined (reading 'x-forwarded-proto')",
        "name": "TypeError"
    }

The output from the server process includes a stack trace.
Sanitized a bit, it is:

    TypeError: Cannot read properties of undefined (reading 'x-forwarded-proto')
        at protocol (.../node_modules/paperplane/lib/parseUrl.js:18:14)
        at .../node_modules/ramda/src/converge.js:47:17
        at _map (.../node_modules/ramda/src/internal/_map.js:6:19)
        at .../node_modules/ramda/src/converge.js:46:33
        at f1 (.../node_modules/ramda/src/internal/_curry1.js:18:17)
        at .../node_modules/ramda/src/uncurryN.js:34:21
        at .../node_modules/ramda/src/internal/_curryN.js:37:27
        at .../node_modules/ramda/src/internal/_arity.js:10:19
        at .../node_modules/ramda/src/internal/_pipe.js:3:14
        at .../node_modules/ramda/src/internal/_arity.js:10:19

Yet the docker-based demo app works. That uses Node.js v12.22.12.
When I tried that Node version on the basic example, it also worked.

So, I used `nvm` to do a binary search on versions. I identified
that Node.js v15.1.0 is the first failing version, and every version
I tried that was newer (up to v21.2.0) also failed.

Scouring the Node.js change log revealed that the http module of
v15.1.0 started calculating req.headers lazily. There's a full
discussion in nodejs/node#35281 [1]. Note the referenced issue that
identifies the bug [2].

[1] nodejs/node#35281
[2] nodejs/node#36550

The workaround identified in this comment [3] is not to use the
spread operator or `Object.assign` on the request object. "These
objects are essentially uncloneable."

[3] nodejs/node#36550 (comment)

This is surprisingly tricky to do right. Various Ramda functions
like `merge` (and I think `converge`) do this implicitly, as does
funky's `assocWith`. So to work around this limitation, while
preserving all behavior, I had to resort to (gasp) mutating
procedures instead of pure functions. Not ideal, I know. I'm open to
better ideas.

So, maybe this isn't the best solution, but it least it gets the
example working again on modern Node versions. If it never gets
merged, at least it will be findable via the repo on GitHub.

For reference, to test this, I used a fresh NPM project with only
the paperplane dependency:

    mkdir paperplane
    cd paperplane
    npm init -y
    npm i -S paperplane

In which I added an index.js file with these contents:

    const { compose } = require('ramda')
    const http = require('http')
    const { json, logger, methods, mount, routes } = require('paperplane')

    const cookies = req => ({ cookies: req.cookies || 'none?' })
    const hello   = req => ({ message: `Hello ${req.params.name}!` })

    const app = routes({
      '/'           : methods({ GET: _ => ({ body: 'Hello anon.' }) }),
      '/cookies'    : methods({ GET: compose(json, cookies) }),
      '/hello/:name': methods({ GET: compose(json, hello) })
    })

    http.createServer(mount({ app })).listen(3000, logger)

And finally (with `fd` and `entr` and `httpie` installed), I started
a file-watching auto-test process:

    fd -e js | entr -rcc bash -c "node index.js & http -v get http://localhost:3000/cookies Cookie:foo=bar"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants
@mcollina @rubenstolk @ExE-Boss @BethGriggs @mhdawson @aduh95 @Flarna and others