ky: Header not set on node version 18.18.2

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: https://github.com/sindresorhus/ky/blob/d372e2eccf9f47597f2c0f49cba286e19d7974b3/source/core/Ky.ts#L203

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.

About this issue

Original URL
State: closed
Created 8 months ago
Reactions: 19
Comments: 15

Most upvoted comments

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.

sholladay on Nov 3, 2023

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

philiparvidsson on Oct 23, 2023

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!

rypete on Nov 1, 2023

@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.

tom-streller-tutti on Oct 26, 2023

FYI: This should now be fixed in nodejs v 18.19.0

TommySorensen on Nov 30, 2023

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.

sholladay on Oct 24, 2023