Node Version 18.18.2 Fails to Instantiate Fetch Request with Proper Options

[gannan08]

Version

v18.18.2

Platform

Darwin 23.0.0 Darwin Kernel Version 23.0.0: Fri Sep 15 14:41:43 PDT 2023; root:xnu-10002.1.13~1/RELEASE_ARM64_T6000 arm64

Subsystem

n/a

What steps will reproduce the bug?

n/a

How often does it reproduce? Is there a required condition?

n/a

What is the expected behavior? Why is that the expected behavior?

n/a

What do you see instead?

Options are not preserved when instantiating a new Fetch Request object using a Request as the first parameter. This causes the options, that should have been copied, to be missing on the new Fetch Request object.

This behavior was fine on <= Node v18.18.1. It is broken on Node v18.18.2.

this.request = new globalThis.Request(this.request, {body: this._options.body}); 

The options fail to be copied. Is this the new expected behavior of the Request constructor after the patch?

See: https://github.com/nodejs/node/releases/tag/v18.18.2
See: sindresorhus/ky#535
See: https://github.com/sindresorhus/ky/blob/d372e2eccf9f47597f2c0f49cba286e19d7974b3/source/core/Ky.ts#L203

Additional information

May be related to latest CVE fixes.

[justinbather]

Has anyone taken this issue?

[BrettHoutz]

I am seeing the same behavior in Node v20.8.1 (the analogous security release for the v20 line).

[KhafraDev]

reproducible by the following code, but works properly in undici.

const request = new Request('https://a', {
  headers: {
    'test': 'abc'
  },
  method: 'POST'
})

const request1 = new Request(request, { body: 'abc' })

console.log(request1.headers.get('test')) // should be 'abc', is null

[KhafraDev]

this will be fixed by the next undici update (cc @nodejs/undici)

[mcollina]

@KhafraDev what PR is this fixed in?

[targos]

I believe it's nodejs/undici#2359

[KhafraDev]

I have no idea, I only had time to test if the new bundle fixed the issue, which it did.

[bricss]

Same issue here right now with both Node 18.8.2 and 21.0.0 ⚠️
All request headers are absolutely empty 💀 all the time in test cases, when fetch is used 🥶
Seems like a catastrophic bug 🪲 and needs an emergency 🚑 hotfix, asap 😱

[bricss]

Repro script:

import { rest } from 'msw';
import { setupServer } from 'msw/node';
import assert from 'node:assert/strict';

const url = 'https://somewhe.re/somewhat/endpoint';

const server = setupServer(
  rest.get(url, (req, res, ctx) => {
    const headers = [...req.headers];

    console.info(headers); // Will be empty []
    assert(headers.length, 'Headers should not be empty'); // Will throw here

    return res(ctx.status(200));
  }),
);

await server.listen();

await fetch(url, {
  headers: {
    'accept': 'application/json',
    'authorization': 'Bearer <token>',
  },
});

await server.close();

[mcollina]

Can you reproduce without msw?

[KhafraDev]

I posted a reproduction above

[mattcollier]

Is it true that once #50274 lands that this issue will be resolved?

I don't see anything in the release notes for undici@5.26.4 that seems directly related to this issue. Can anyone provide a status update?

[KhafraDev]

Nothing related to 5.26.3 should have broken this either, it's a strange issue.

[KhafraDev]

git bisect says that KhafraDev/undici@7bcb80c caused this. Probably fixed by the later build updates.

[bricss]

@mcollina I'mma tried with real one local server and headers made it through 🙏
But all integration test with msw at work went kaboom 🧑‍🚒

[bricss]

Btw, why undici using bundler anyway? What is the purpose?
It's less than 1% of code in TypeScript in da repo...

[KhafraDev]

nodejs/undici#1183 (comment) it wouldn't work otherwise

[bricss]

I dunno, maybe dis may help with core relative paths:

const dirname = path.dirname(url.fileURLToPath(import.meta.url));
const pathway = path.resolve(dirname, '../woot.🙃');

This is how we deal with relative path in monorepo(s), created with npm workspace(s) for example 🤷

[KhafraDev]

undici is written in cjs and deps/ don't have access to __filename or other cjs things. Bundling is the best solution in this case.

[panva]

undici is written in cjs and deps/ don't have access to __filename or other cjs things. Bundling is the best solution in this case.

@KhafraDev How about just checking for typeof __filename === 'undefined' to know it's ran from deps/?

[mcollina]

@KhafraDev could you send a PR with the regression test so we don't break this anymore?

[scmx]

Here's a quick way to reproduce it. fetch(request, options) will ignore headers etc from request whenever options is passed.

request = new Request('https://echo.zuplo.io/', { headers: { Authentication: 'basic test' } });
res = await fetch(request, { method: 'POST' });
await res.json()
{  method: 'POST', headers: {} }

res = await fetch('https://echo.zuplo.io/', { headers: { Authentication: 'basic test' }, method: 'POST' });
await res.json()
{  method: 'POST', headers: { authentication: 'basic test'  } }

With fetch(request) without second argument there is no problem:

request = new Request('https://echo.zuplo.io/', { method: 'POST', headers: { Authentication: 'basic test' } });
res = await fetch(request);
await res.json()
{  method: 'POST', headers: { authentication: 'basic test'  } }

(Ran into this two days ago and rolled back to 18.18.0, now I was able to understand what the problem was 😄 )

[KhafraDev]

@KhafraDev How about just checking for typeof __filename === 'undefined' to know it's ran from deps/?

I suggested similar I think, but it's not very future-proof. Ie if deps got access to __filename/__dirname in the future it'd break again. Since we're already bundling, using esbuild to handle it is fine.

[KhafraDev]

@KhafraDev could you send a PR with the regression test so we don't break this anymore?

It might be nice to run the entirety of the fetch tests on the bundle, rather than adding in a useless test. Or maybe we could figure out a way to not bundle undici, which would prevent all of these issues from happening (including the issue regarding names recently).

[MajorTanya]

This issue shouldn't be closed as the problem still persists in the newly released 20.9.0 as well as the as-of-now current 18.18.2 listed in the issue title

[mcollina]

@KhafraDev can you send a PR with a regression test on Node.js for this?

[mattcollier]

Hello all, can someone provide a status update on this issue? As @MajorTanya stated, this issue is not resolved in the 20.9.0 release.

[davidlehn]

@mattcollier #50274 has comments about the fix being cherry-picked into v18.x-staging and v20.x-staging.

[johndalvik]

Hey all!
Any update on this, when can we expect a release containing the fixes?

[MajorTanya]

Not a contributor, but this seems to be fully fixed with the 20.10.0 LTS release

Edit: due to the lack of a new release for 18.x at this point, this remains an issue for 18.18.2

[MajorTanya]

With 18.19.0 landing in 8787acb, this issue should be resolved across v18, v20, and v21, which are all supported versions.