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

rubenstolk · 2020-12-17T12:03:40Z

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

The text was updated successfully, but these errors were encountered:

rubenstolk · 2020-12-17T12:07:17Z

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

ExE-Boss · 2020-12-17T12:10:13Z

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 · 2020-12-17T12:14:11Z

This likely related to #35281.

ExE-Boss · 2020-12-17T12:16:15Z

This is definitely caused by #35281.

aduh95 · 2020-12-17T12:22:01Z

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 · 2020-12-17T12:31:48Z

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

BethGriggs · 2020-12-17T13:02:27Z

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 · 2020-12-17T13:11:54Z

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

mhdawson · 2020-12-17T13:58:49Z

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

This reverts commit b58725c. Fixes: nodejs#36550

BethGriggs · 2020-12-17T15:42:50Z

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 · 2020-12-17T17:58:43Z

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.

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>

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 · 2020-12-17T23:40:53Z

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!

Flarna · 2020-12-17T23:51:33Z

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 · 2020-12-18T07:43:23Z

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!

Flarna · 2020-12-18T09:55:27Z

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 · 2020-12-18T10:50:19Z

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 · 2020-12-18T11:07:42Z

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 · 2020-12-18T13:44:27Z

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.

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.

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

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

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

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>

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>

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.

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.

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"

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

rubenstolk mentioned this issue Dec 17, 2020

vm: refactor to use more primordials #36023

Closed

2 tasks

aduh95 mentioned this issue Dec 17, 2020

http: lazy create IncomingMessage.headers #35281

Closed

4 tasks

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

wvankuipers mentioned this issue Dec 17, 2020

UnhandledRejection: Cannot read property 'range' of undefined karma-runner/karma#3588

Closed

devoto13 mentioned this issue Dec 17, 2020

ng test crashes on Node 14.15.2 and Node 15 angular/angular-cli#19644

Closed

15 tasks

BethGriggs added a commit to BethGriggs/node that referenced this issue Dec 17, 2020

Revert "http: lazy create IncomingMessage.headers"

db5461a

This reverts commit b58725c. Fixes: nodejs#36550

BethGriggs mentioned this issue Dec 17, 2020

[v14.x] Revert "http: lazy create IncomingMessage.headers" #36553

Closed

2 tasks

BethGriggs mentioned this issue Dec 17, 2020

v14.15.3 proposal #36555

Merged

aduh95 closed this as completed Dec 17, 2020

ExE-Boss mentioned this issue Dec 22, 2020

doc: document "http: lazy create IncomingMessage.headers" #36601

Merged

2 tasks

tripodsan mentioned this issue Jan 14, 2021

hlx up fails with node 15.x adobe/helix-simulator#566

Closed

This was referenced May 29, 2021

Update Node.js to v14 telemark/robot-post-payload#143

Open

Update Node.js to v14 - autoclosed vtfk/robot-move-error#12

Closed

Update Node.js to v14 telemark/pdftemplater-webservice-docker#235

Open

svkoskin mentioned this issue Aug 27, 2021

Credentials provider: request headers undefined in authorize callback on new Nodes nextauthjs/next-auth#2612

Closed

devoto13 mentioned this issue Feb 17, 2022

ERROR [karma]: { inspect: [Function: inspect] } karma-runner/karma#3764

Closed

dafortune mentioned this issue Feb 25, 2022

fix: some request properties are not being passed to hawk in Node16 jfromaniello/passport-hawk#15

Merged

lionralfs mentioned this issue Feb 26, 2022

cloneIncomingMessage does not handle headers field in Node.js v16 mswjs/interceptors#212

Closed

efdali mentioned this issue Oct 23, 2023

fix: forward-compatibility adaptation for request headers puzzle-js/puzzle-js#407

Merged

kyptin mentioned this issue Dec 12, 2023

Fix mount for Node.js >= 15.1.0 articulate/paperplane#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rubenstolk commented Dec 17, 2020 •

edited by aduh95

rubenstolk commented Dec 17, 2020

ExE-Boss commented Dec 17, 2020 •

edited

aduh95 commented Dec 17, 2020

ExE-Boss commented Dec 17, 2020

aduh95 commented Dec 17, 2020

rubenstolk commented Dec 17, 2020

BethGriggs commented Dec 17, 2020

mcollina commented Dec 17, 2020

mhdawson commented Dec 17, 2020

BethGriggs commented Dec 17, 2020

mcollina commented Dec 17, 2020

aduh95 commented Dec 17, 2020

Flarna commented Dec 17, 2020

rubenstolk commented Dec 18, 2020

Flarna commented Dec 18, 2020

mcollina commented Dec 18, 2020

aduh95 commented Dec 18, 2020

Flarna commented Dec 18, 2020

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

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

Comments

rubenstolk commented Dec 17, 2020 • edited by aduh95

What steps will reproduce the bug?

What is the expected behavior?

What do you see instead?

Additional info

rubenstolk commented Dec 17, 2020

ExE-Boss commented Dec 17, 2020 • edited

aduh95 commented Dec 17, 2020

ExE-Boss commented Dec 17, 2020

aduh95 commented Dec 17, 2020

rubenstolk commented Dec 17, 2020

BethGriggs commented Dec 17, 2020

mcollina commented Dec 17, 2020

mhdawson commented Dec 17, 2020

BethGriggs commented Dec 17, 2020

mcollina commented Dec 17, 2020

aduh95 commented Dec 17, 2020

Flarna commented Dec 17, 2020

rubenstolk commented Dec 18, 2020

Flarna commented Dec 18, 2020

mcollina commented Dec 18, 2020

aduh95 commented Dec 18, 2020

Flarna commented Dec 18, 2020

rubenstolk commented Dec 17, 2020 •

edited by aduh95

ExE-Boss commented Dec 17, 2020 •

edited