Set-Cookie headers are concatenated, and are not working correctly

[TorbjornHoltmon]

Astro Info

Astro                    v4.2.4
Node                     v20.10.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   server
Adapter                  none
Integrations             @astrojs/tailwind
                         @astrojs/svelte

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Set cookie headers seem to be set incorrectly.

  const headers = new Headers();
  headers.append('Set-Cookie', 'my-other-cookie=love.it!; Path=/; HttpOnly; SameSite=Lax');
  headers.append('Set-Cookie', 'my-cookie=its.very.good; Path=/; HttpOnly; SameSite=Lax');
  return new Response('hello world', { headers });

Will give me:

set-cookie: my-other-cookie=love.it!; Path=/; HttpOnly; SameSite=Lax,my-cookie=its.very.good; Path=/; HttpOnly; SameSite=Lax

This will only set the first my-other-cookie as the second cookie is concatenated with a comma and is invalid.

I tested with a simple Hono API using Vite, and set-cookie worked correctly. So I am assuming this is an Astro problem.

There are no issues when hosting on Cloudflare pages, which makes me think this is an issue with the node dev server.

What's the expected result?

Set-Cookie headers must not be concatenated

Link to Minimal Reproducible Example

https://stackblitz.com/edit/withastro-astro-cgewux?file=src%2Fpages%2Fcookie-endpoint.ts

Participation

• I am willing to submit a pull request for this issue.

[lilnasy]

Astro tries to use a standard wherever possible, and the Headers object is one of them. The behavior you are seeing comes Node.js itself and aligns with browsers as well.

Are you expecting headers.set to perform some validation specific to the "Set-Cookie" header?

[matthewp]

@lilnasy There is a newer getSetCookie() method that didn't exist when we implemented all of this, but does now. It requires Node >19, however, and we support Node 18. I'm wondering if we should use that method in the dev server, if it exists.

[lilnasy]

@matthewp Luckily, it was backported to 18 exactly in our minimum supported version, 18.14.1. Where would we use it?

[TorbjornHoltmon]

Have any of you been able to replicate the issue?

Set-Cookie worked as expected back in Astro 3.
At some point, it broke. We didn't notice because we were not actively testing that part of the app in dev until recently.
That is when I discovered this.
Everything works as expected when deployed.

Stackblitz is not a good place to test cookies as there is no way to inspect them as far as I am aware.
I recommend trying to set more than one cookie from an Astro endpoint in the same API call.
For me this is impossible, and another colleague of mine had the same issue on Windows. He also runs node 20.

I have looked at the code, and I also thought it might be the culprit.

https://github.com/withastro/astro/blob/main/packages/astro/src/core/app/createOutgoingHttpHeaders.ts#L29

I am not too familiar with node:http. But from the docs this looks correct.

https://nodejs.org/api/http.html#messageheaders

set-cookie is always an array. Duplicates are added to the array.

For all other headers, the values are joined together with , .

For some reason, the set-cookie header is joined with ,. But the headers are set correctly according to the docs.

Looking at the Hono node-server implementation, where I had no issues.

https://github.com/honojs/node-server/blob/main/src/utils.ts#L44

It is the same, just that he does not use the getSetCookie function. So I think we can rule that out.

https://github.com/withastro/astro/blob/main/packages/astro/src/core/app/node.ts#L100

It is only used here, and the headers are not changed. So everything looks ok.
I think this only happens on the dev server, does the dev server even use this? Is that not handled through vite?

I have no clue what is causing this. This bug is very weird...

[TorbjornHoltmon]

I also want to preface. Set-Cookie is one of the exceptions, where HTTP requires you send multiple headers.
This is why getSetCookie exists, it is hard to parse set-cookies when they are comma separated.

https://www.rfc-editor.org/rfc/rfc9110.html#name-field-order

Note: In practice, the "Set-Cookie" header field ([COOKIE]) often appears in a response message across multiple field lines and does not use the list syntax, violating the above requirements on multiple field lines with the same field name. Since it cannot be combined into a single field value, recipients ought to handle "Set-Cookie" as a special case while processing fields. (See Appendix A.2.3 of [Kri2001] for details.)

[lilnasy]

The code you linked does work the way you describe if you look at the implementation of createOutgoingHttpHeaders(). However, it would act only on node, and node-based adapters in production.

It is possible dev doesn't special-case "Set-Cookie" to create an array.

Edit: that seems to be the problem - it special-cases but then concatenates the array anyway: writeWebResponse()

[matthewp]

@TorbjornHoltmon Yes, we are very aware of this issue and it's caused us a lot of pain in the past. The tldr is that the Headers API does not / did not support Set-Cookie as a special-case because the API was developed for use in browsers and there is no use-case for Set-Cookie there. After a lot of time they have changed their mind and getSetCookie is the result.

@lilnasy The code for this in dev is here:

astro/packages/astro/src/vite-plugin-astro-server/response.ts

Lines 59 to 75 in 70ae19d

	const setCookieHeaders = Array.from(getSetCookiesFromResponse(webResponse));
	if (setCookieHeaders.length) {
	// Always use `res.setHeader` because headers.append causes them to be concatenated.
	res.setHeader('set-cookie', setCookieHeaders);
	}
	const _headers = Object.fromEntries(headers.entries());
	// Undici 5.20.0+ includes a `getSetCookie` helper that returns an array of all the `set-cookies` headers.
	// Previously, `headers.entries()` would already have these merged, but it seems like this isn't the case anymore.
	if (headers.has('set-cookie')) {
	if ('getSetCookie' in headers && typeof headers.getSetCookie === 'function') {
	_headers['set-cookie'] = headers.getSetCookie().toString();
	} else {
	_headers['set-cookie'] = headers.get('set-cookie')!;
	}
	}

As you can see, we do check for getSetCookie already and use it, so why that's not working in this case I'm not sure.

@TorbjornHoltmon In general I'd advise using Astro.cookies.set() for setting cookies as it's more tested for this particular problem. Not to say this issue shouldn't be fixed.

[TorbjornHoltmon]

Nice work finding it lilnasy!

The Astro cookie API is great! But as lilnasy said, Astro is very much WinterCG compliant. This makes it possible for us to to use primitives and integrate other amazing projects that are also WinterCG compliant.

In this case, it was AuthJS, which removes and sets cookies with Set-Cookie on its callback routes.