Header not set on node version 18.18.2

[phil-tutti]

Hello,

We were currently running into an issue, that with the new node version of 18.18.2 (where they introduced a security fix for undici) and the latest version of ky (1.1.0), headers are not properly set on the request with a json payload.

I tracked it down to this line of code:

ky/source/core/Ky.ts

Line 203 in d372e2e

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

Basically, with the node version 18.18.2, as soon a new Request is being created, it loses all the options (including headers) that were set on this.request, resulting in a new Request with only content-type: text/plain;charset=UTF-8 being set. In node 18.18.1, everything is working fine

I was able to make it work by spreading out the options.

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

However, it's hard for me to tell if this is actually a bug in node (undici), or if it was never supposed to work like this and they fixed it with the new version.

[tkrotoff]

@phil-tutti Thanks for reporting this, I spent several hours today understanding what you have perfectly described 🙏

Some dates:

• Node.js 18.18.2 has been released on 2023-10-13
• Docker Node.js 18.18.2 has been released on 2023-10-16

[BenjaminGaymay]

Thanks for sharing this!
Do you have a workaround that can be used until we understand the whole story?

[phil-tutti]

The workaround we're using is to not update to the latest version of node. So stick to 18.18.1 or lower

[fqf]

The same problem. In this case:
ky.post("url", { json: { field: "text" }, }).json();
request doesn't have conten-type header set as "application/json".

[sholladay]

Do the Ky tests fail on that version of Node?

Specifically, if you clone the Ky repo and run the tests, I'd like to know the result of this test:

ky/test/main.ts

Lines 92 to 110 in d372e2e

	test('POST JSON', async t => {
	t.plan(2);
	const server = await createHttpTestServer();
	server.post('/', async (request, response) => {
	t.is(request.headers['content-type'], 'application/json');
	response.json(request.body);
	});
	const json = {
	foo: true,
	};
	const responseJson = await ky.post(server.url, {json}).json();
	t.deepEqual(responseJson, json);
	await server.close();
	});

I don't love the idea of solving this by spreading the options, as it's semantically incorrect in terms of what that code is trying to do. We only want to modify one request property, not construct it from scratch. But if the test suite passes with that change, it's worth considering.

[philiparvidsson]

this is breaking everything that requires custom headers, i.e., all our internal api calls so we actually can't even deploy right now unless we rip ky out of everything. I think this is rather urgent to fix - or does anyone know how to do a workaround?

I tried using create and then hacking around to reach the request object and set headers on it directly but it's not feasible

[sholladay]

I understand the desire to use the latest Node with the security fix ASAP. But surely it would be easier to deploy with a specific version of Node (<=18.8.1) than to "rip Ky out of everything".

If you need this fixed quickly, you should submit a failing test case. That will make it easy for us to reproduce your exact scenario and be confident in the fix that we come up with.

Here is an example. You would put something like this in test/main.js and then submit a PR with your changes.

test('JSON header issue #535', async t => {
	t.plan(3);

	const server = await createHttpTestServer();
	server.post('/', async (request, response) => {
		// SOME ASSERTIONS ABOUT HEADERS HERE
		// t.is(request.headers['content-type'], 'application/json');
		response.json(request.body);
	});

	const json = {
		foo: true,
	};

	// SHOW HOW YOU ARE USING KY HERE
	const responseJson = await ky.post(server.url, {json}).json();

	t.deepEqual(responseJson, json);

	await server.close();
});

A new test might not even be necessary, though. Our existing tests may already catch this issue. The problem is, the last time they ran, it was on Node 18.18.0, since that was the latest 18.x version at the time, and so of course the tests passed.

It would be nice if Node included Ky in their CITGM regression tests to prevent this sort of issue in the future.

[linkvt]

I just came across the problem and am a bit confused about this issue, as I see no recommendation or action I could take as a user that can't/won't downgrade to a previous nodejs version.

@sholladay running the test locally causes immediately a lot of failures with the current node version. Please don't get me wrong, but I would have thought that this would be trivial for you to check as a maintainer?
Besides that, running the pipeline with the latest nodejs version should also be a very simple change like setting >=18.18.2 in the pipeline.

It is likely just a misunderstanding and I definitely don't want to take the contributors/maintainers work for granted, but I and likely a few other users would really appreciate some guidance on either topics you need support on to further investigate this or steps you already have taken.

Thanks!

[tom-streller-tutti]

@linkvt The core issue is unfortunately not in this library but in a bug with undici introduced into Node 18.18.2. The current recommendation is to use 18.18.1 and wait for the fix in 18.18.3 of node. Once that is fixed, no changes are necessary in ky.

The HTTP/2 security issue that was fixed is only urgent if your Node.js server is directly open to the internet and not behind a reverse proxy that already fixes the issue.

[rypete]

I was able to work around this by using body instead of json. I spent ages trying to figure out why my headers were getting dropped!

[sholladay]

I see no recommendation or action I could take as a user that can't/won't downgrade to a previous nodejs version.

Node has already fixed this and it should be released any day now.

The workaround mentioned by @rypete of using body should work. That just happens to take a different code path internally that shouldn't trigger the Node bug. That is the only action I would recommend to users at the moment, for those who MUST use the latest Node and cannot wait for the upstream fix.

Please don't get me wrong, but I would have thought that this would be trivial for you to check as a maintainer?

You're right. However, running the test suite is easy for anyone to do and I like to encourage people to get involved. Our priorities are also probably different, as I only use Ky in the browser, not in Node.

Given that Node has already implemented a true, proper fix, I don't think it makes sense for us to work around it here. But I'd be happy to look into the fix suggested by @phil-tutti if the next Node release doesn't solve it for some reason.

[TommySorensen]

FYI: This should now be fixed in nodejs v 18.19.0

[phil-tutti]

As it has been backported by node and is fixed in all their latest (supported) versions, I will close this issue. Thanks for all the help

[ilmpc]

FYI: this issue is presented in Node v20.8.1 as well.
Source problem is broken Request copy mechanizm.

const first = new Request(url, { header: { test: 123 }})
const second = new Request(first)
second.headers.get('test') // undefined

Latest LTS v20.10.0 is working alright

[baptisteArno]

Jeez, I spent an hour on this, I was on v20.8.1, updating to v20.12 fixed it 👍