Can't set multiple cookies in response on v2

[michael-land]

What version of astro are you using?

2.1.2

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

tried on vercel and node

What package manager are you using?

pnpm

What operating system are you using?

Mac

What browser are you using?

Chrome

Describe the Bug

    Astro.cookies.set(`is_auth`, '1', {
      httpOnly: true,
      secure: true,
      sameSite: 'strict',
      expires: addMinutes(new Date(), 5),
      path: '/',
    });
    Astro.cookies.set(`authorization`, accessToken, {
      httpOnly: true,
      secure: true,
      sameSite: 'strict',
      expires: addMinutes(new Date(), 5),
      path: '/',
    });

Link to Minimal Reproducible Example

todo

Participation

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

[Princesseuh]

Hello! What version of Node are you using? We've recently had a whole dilemma around cookie handling where specific versions of Node behaved differently and stuff, so it would be helpful to know!

[michael-land]

Hello! What version of Node are you using? We've recently had a whole dilemma around cookie handling where specific versions of Node behaved differently and stuff, so it would be helpful to know!

ah, i see. i was using node 18.2, it working after upgrade to 18.5

[jeffdrumgod]

I'm using 18.12 and have the same problem here.

Edited: only in dev mode. Using production mode (after build, using @astrojs/node) works fine

[romanzy313]

Please reopen the issue, I am having the same problem as @jeffdrumgod. Setting multiple cookies works in build but not in dev. Really makes it impossible to use cookies while developing.

Running node 18.14.0, astro 2.3.3, @astrojs/node 5.1.2

Edit:
updating node to latest LTS version 18.16.0 or version 20.0.0 makes cookies work in prod and dev. Probably node issue more then anything

[matthewp]

Ok, will recheck.

[matthewp]

Think a regression happened here. https://github.com/withastro/astro/pull/6355/files

[alex-sherwin]

I'm seeing multiple set-cookie's work in dev and not in prod (nodejs adapter) using current Node LTS v18.16.0 and latest Astro 2.5.5

I'm not even using Astro.cookies at all, this is me directly setting up a Response with a Headers object with multiple set-cookie headers, e.g.

const h = new Headers();
h.append("set-cookie", "cookie1=abc");
h.append("set-cookie", "cookie2=abc");
return new Response("abc", { headers });

Works in dev, but in prod (using Astro's nodejs adapter) only the first set-cookie is used.

Feels like a bug with some naive code grabbing only the first occurrence of a key (e.g. "set-cookie" header name), still digging through Astro code to see if I can find the culprit

[alex-sherwin]

And there it is https://github.com/withastro/astro/blob/main/packages/integrations/node/src/nodeMiddleware.ts#L52

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

The line Object.fromEntries(headers.entries()) is too naive and not accounting for the fact that Headers is a special purpose multi-value map (i.e. can contain > 1 value per key), it cannot be converted to a simple object in this manner.

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 multi-value object of header key/value pairs

Since this issue was about Astro cookies (and what I'm referring to is the nodejs adapter for any headers specifically set on the Response) I'll open a new ticket)

[matthewp]

Multiple set-cookie headers is not supported by the Headers API. If you Google you'll see lots of discussion on this.

[alex-sherwin]

@matthewp, historically that's correct, however, it is now addressed in the official Fetch spec and now supported in undici since 5.19.0 via Headers.getSetCookie()

I opened #7226 and I'm wrapping up a PR now that's based on the built-in Headers.getSetCookie() that I think is a pretty clean solution