Node Adapter writes server.writeHead(..) headers incorrectly

[alex-sherwin]

What version of astro are you using?

2.5.5

Are you using an SSR adapter? If so, which one?

node

What package manager are you using?

pnpm

What operating system are you using?

macOS, linux

What browser are you using?

Chrome

Describe the Bug

In the NodeJS Adapter middleware https://github.com/withastro/astro/blob/main/packages/integrations/node/src/nodeMiddleware.ts#L52

There is this line to start writing the actual server response

res.writeHead(status, Object.fromEntries(headers.entries()));

The line Object.fromEntries(headers.entries()) is too naive and not accounting for the fact that Headers can contain set-cookie headers which are a one-off, special mutli-value header use case (set-cookie header values cannot be concatenated into a CSV like other header values, so multiple set-cookie values must appear as multiple headers in the response), so server-generated headers cannot be safely converted to a simple object in this manner (the background behind this is because the WebAPI Fetch Headers spec was originally created as a client-side API, and some warts are popping up when moving to usage on backend processes.)

For example:

const h = new Headers();
h.append("set-cookie", "cookie1=value");
h.append("set-cookie", "cookie2=value");

// results in { "set-cookie": "cookie1=value, cookie2=value" }
const naiveObject = Object.fromEntries(headers.entries());

Where it needs to be

{ "set-cookie": ["cookie1=value", "cookie2=value"] }

In reading the node spec for response.writeHead https://nodejs.org/docs/latest-v18.x/api/http.html#responsewriteheadstatuscode-statusmessage-headers interestingly it does not explicitly state that the object form of supplying headers can accept an array of values (but the docs for response.setHeader do say this, so I would assume the behavior is the same)

So this needs to be smarter about building a NodeJS headers object specifically around set-cookie multi-value object of header key/value pairs

There has been a bunch of recent issues opened up specifically around set-cookie behavior not working as expected (and indeed that's where I got tripped up on this), and I have a suspicion for most of them this is the root cause, and it's a general header problem, not specifically about cookies.

The linked stack blitz doesn't really work because this is only a problem once you build and run a standalone server build (because it's the nodejs adapter middleware that's the issue, the vite dev server doesn't exhibit this problem)

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-chr9rk?file=src%2Fpages%2Fapi.ts

Participation

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

[alex-sherwin]

I was putting together a PR and the actual behavior of the Header instance functions like .entries() and .forEach() is actually different then I was expecting (they do not expose the multi-value's per key, instead, it returns a single key that already combines multi values into a CSV string, which is valid for [all?] headers except Set-Cookie)

So now I'm down this rabbit hole: https://fetch.spec.whatwg.org/#terminology-headers

Set-Cookie is a special case header and needs special handling, but the Headers object has already mangled it by the time you've appended two Set-Cookie headers onto the Header instance.

[alex-sherwin]

So the proper answer is https://developer.mozilla.org/en-US/docs/Web/API/Headers/getSetCookie which was [supposedly] added to Node in 19.7.0 according to the MDN docs compatability table... but I'm not seeing anything related to that in the node docs.

Does the Astro codebase have an established pattern to selectively use new Node API's if they're available?

[Princesseuh]

The lowest version we support is Node 16 at this time, so any solutions would need to support that as well, but otherwise using the API if it exists works fine by us.

You might find the code here interesting: https://github.com/withastro/astro/blob/main/packages/integrations/netlify/src/netlify-functions.ts#LL140C26-L140C26

[alex-sherwin]

@Princesseuh so I actually stumbled across the fact this is already dealt with in @astrojs/webapi, which polyfills the WebAPI's for NodeJS based on undici, which added Headers.getSetCookies() in version 5.19.0: https://github.com/nodejs/undici/releases/tag/v5.19.0

So, it sounds like Headers.getSetCookie() it should actually be safely available to use from astro code already, the only problem from TypeScript code appears to be that however astro is applying those polyfills it's not changing the global definitions for WebAPI's like Header from the defaults that TypeScript applies from the built-in DOM lib types.

That's no big deal though, I'm just a bit stuck on why doing this:

import type { Headers } from "@astrojs/webapi";

Breaks all type inference in a given TypeScript file, so for the moment, I'm putting together a PR and tests to see if the polyfill actually works based with quick hacky type:

interface HeadersWithGetSetCookie extends Headers {
  // this is actually available on the @astrojs/webapi polyfill, but tsc doesn't pick it up on the built-in Headers type
  getSetCookie(): string[];
}

[alex-sherwin]

My branch has a pretty clean solution to this and test coverage, however, currently you can't mix using Response.headers and AstroCookies (this was already the case as-is)

Once I apply a solution for that I'll open up a PR