How to set multiple Set-Cookie headers in a single response?

[kentcdodds]

I tried copying concatSetCookieHeaders from remix but that... eh, doesn't work... Doing that I end up with a single set-cookie header.

Here's the relevant code:

const sessionIdCookie = await getUserSessionCookieFromMagicLink(request)
if (sessionIdCookie) {
  const destroyCookie = await loginInfoSession.destroy()
  return redirect('/me', {
    headers: {
      'Set-Cookie': concatSetCookieHeaders(destroyCookie, sessionIdCookie),
    },
  })
}

What I need is two Set-Cookie headers, and this gives me a single Set-Cookie header that doesn't end up updating either one.

If I use either of these cookie values by itself then they each work as expected. But I need to set them both in the same response.

[kentcdodds]

I get an interesting result if I try this:

// import {Headers} from 'remix'

const headers = new Headers()
headers.append('Set-Cookie', destroyCookie)
headers.append('Set-Cookie', sessionIdCookie)
return redirect('/me', {headers})

I still only get a single set-cookie header, and it looks like this:

set-cookie: KCD_login=; Max-Age=1800000; Path=/; Expires=Thu, 01 Jan 1970 00:00:00 GMT; SameSite=Lax, KCD_root_session=eyJfX3Nlc3Npb25faWRfXyI6IjYxMTllMDA4LWJhMWMtNDA1Zi05MmViLTdkYmUwYTY2NzY1NCJ9.bbbrkMWekmxsFv1GOP1t%2FIK12L4ZBQcVUnwS54p9nkE; Max-Age=2592000000; Path=/; SameSite=Lax

I'm pretty sure that's exactly the same result as using concatSetCookiesHeaders which is good news at least. Means I don't need to vendor concatSetCookieHeaders :)

But I'm thinking remix has a bug in handling this situation. It should be sending multiple Set-Cookie headers. I'm using the express adapter.

[kentcdodds]

I got it working. Now we're properly setting two cookies :)

However, to make it work, I needed to create these patches 😬

diff --git a/node_modules/@remix-run/express/server.js b/node_modules/@remix-run/express/server.js
index 4fd2142..246a32e 100644
--- a/node_modules/@remix-run/express/server.js
+++ b/node_modules/@remix-run/express/server.js
@@ -73,8 +73,10 @@ function createRemixRequest(req) {
 function sendRemixResponse(res, response) {
   res.status(response.status);
 
-  for (let [key, value] of response.headers.entries()) {
-    res.set(key, value);
+  for (let [key, values] of Object.entries(response.headers.raw())) {
+    for (const value of values) {
+      res.append(key, value);
+    }
   }
 
   if (Buffer.isBuffer(response.body)) {

And one in @remix-run/node/server.js. This patch also has support for build.entry.module.handleDataRequest and I was too lazy to try and figure out how to update the patch content to remove that.

diff --git a/node_modules/@remix-run/node/server.js b/node_modules/@remix-run/node/server.js
index af7406e..93cf72d 100644
--- a/node_modules/@remix-run/node/server.js
+++ b/node_modules/@remix-run/node/server.js
@@ -58,7 +58,7 @@ async function handleDataRequest(request, loadContext, build, routes) {
   try {
     response = isActionRequest(request) ? await data.callRouteAction(build, routeMatch.route.id, clonedRequest, loadContext, routeMatch.params) : await data.loadRouteData(build, routeMatch.route.id, clonedRequest, loadContext, routeMatch.params);
   } catch (error) {
-    return responses.json(errors.serializeError(error), {
+    response = responses.json(errors.serializeError(error), {
       status: 500,
       headers: {
         "X-Remix-Error": "unfortunately, yes"
@@ -66,17 +66,29 @@ async function handleDataRequest(request, loadContext, build, routes) {
     });
   }
 
+  if (build.entry.module.handleDataRequest) {
+    try {
+      response = await build.entry.module.handleDataRequest(clonedRequest, response, loadContext, routeMatch.params);
+    } catch (error) {
+      return responses.json(errors.serializeError(error), {
+        status: 500,
+        headers: {
+          "X-Data-Request-Handler-Error": "Handle your errors, yo."
+        }
+      });
+    }
+  }
+
   if (isRedirectResponse(response)) {
     // We don't have any way to prevent a fetch request from following
     // redirects. So we use the `X-Remix-Redirect` header to indicate the
     // next URL, and then "follow" the redirect manually on the client.
     let locationHeader = response.headers.get("Location");
     response.headers.delete("Location");
+    response.headers.set("X-Remix-Redirect", locationHeader);
     return new nodeFetch.Response("", {
       status: 204,
-      headers: { ...Object.fromEntries(response.headers),
-        "X-Remix-Redirect": locationHeader
-      }
+      headers: response.headers,
     });
   }

As discussed on discord, response.headers.raw() is a node-fetch-only API and not standard. There doesn't appear to be any way to get the individual values of headers. Every way to get values from a header has the values concatenated by ,. Unfortunately we can't simply .split(',') because the Expires value on a cookie can have a , in it (as in ... Expires=Thu, 01 Jan 1970 00:00:00 GMT...).

[kentcdodds]

Another thing that will need to happen is all the templates and docs should update so instead of this:

headers: {
  ...Object.fromEntries(responseHeaders),
  // ...etc
}

They simply set values on the responseHeaders instead. Otherwise multiple header values will be lost.

I just looked and noticed that this doc is correct, so maybe you've already made this change.

responseHeaders.set("Content-Type", "text/html");

  return new Response("<!DOCTYPE html>" + markup, {
    status: responseStatusCode,
    headers: responseHeaders
  });

👍

[kentcdodds]

FYI, I had to add another patch for the @remix-run/node module to support preserving multiple header values in redirects. I updated my comment above.

[mjackson]

Thanks for all the detailed info here, @kentcdodds. I think you've uncovered a significant bug in the way we currently handle headers in Remix. We'll get this fixed very soon 👍

[mcansh]

this is fixed and included once 0.18 ships!

[shripadk]

This doesn't seem to be working when you have a parent loader and nested loader both trying to Set-Cookie. Since according to Remix docs, the nested loader headers override the parent loader headers, it can only be merged in the headers function that is exported. However, there is no way to merge both Set-Cookie headers in there either as the result expected by the function is a Javascript object or instance of Headers Fetch API.

How do I handle this situation? Or am I missing something?

[kiliman]

@shripadk This is a closed issue. I don't think your issue is a bug, but a support question. Can you create a new discussion?

[shripadk]

@kiliman Wasn't sure if this is a bug or a discussion so I posted both. A discussion in Discord as well as a report here. But I am now inclined to think this is definitely a bug.

Tested it out in the root loader (the Response headers contains only 1 Set-Cookie that is concatenating all values instead of setting a Set-Cookie for each cookie):

Network tab:

[shripadk]

@kiliman Interestingly it only works with redirect. This only tells me that this is definitely a bug as the user of the framework would expect same consistent behavior with respect to headers in both situations (when using json and when using redirect) as well as within the exported header function. I feel this issue should be re-opened as it only tackles one use case. You can see the behavior of redirect (which is the right behavior) and contrast it with behavior of json above:

Network tab:

[martinmckenna]

this still seems to be an issue in the action. I currently have this action in a route:

import type { ActionArgs } from "@remix-run/node";
import { createCookie, json } from "@remix-run/node";

import { AUTH_COOKIE, AUTH_EXP_COOKIE } from "app/cookies"

export const setCookie = (key: string, value: any) => {
  const createdCookie = createCookie(key, {
    path: "/",
    sameSite: "lax",
  });
  return createdCookie.serialize(value);
};

export async function action({ request }: ActionArgs) {
  return json(
    {},
    {
      headers: {
        "Set-Cookie": await setCookie(AUTH_COOKIE, ""),
        // @ts-expect-error
        "Set-Cookie": await setCookie(AUTH_EXP_COOKIE, ""),
      },
    }
  );
}

and here are my response headers:

[wenzf]

This way it seems to work

   return json(
            { success: true },
            {
                headers: [
                    ["Set-Cookie", await destroyFirstSession(firstSession)],
                    ["Set-Cookie", await commitSecondSession(secondSession)]

                ]
            }
        )

[alfredsgenkins]

I tried both cookie.append and setting array of cookie entries and it appears nothing works in production. Weirdly on local it's all good. But in production it breaks. Is there any caveat I need to avoid when declaring cookies? Or maybe the problem is actually in ION-SST Remix plugin (could be related to format of how AWS Lambda + API Gateway expect response to look like: this: https://stackoverflow.com/a/68994753).

Cookies:

const idTokenCookie = createCookie("id_token", {
  path: "/",
  httpOnly: true,
  secrets: [process.env.COOKIE_SECRET],
  secure: process.env.NODE_ENV === "production",
});

const refreshTokenCookie = createCookie("refresh_token", {
  path: "/",
  httpOnly: true,
  secrets: [process.env.COOKIE_SECRET],
  secure: process.env.NODE_ENV === "production",
});

And nor this:

class {
  async getLogoutHeaders() {
    return [
      ["Set-Cookie", await idTokenCookie.serialize("", { maxAge: 0 })],
      ["Set-Cookie", await refreshTokenCookie.serialize("", { maxAge: 0 })],
    ];
  }
}

And nor this:

class {
  async getLogoutHeaders() {
    const headers = new Headers();

    headers.append(
      "Set-Cookie",
      await idTokenCookie.serialize("", { maxAge: 0 })
    );

    headers.append(
      "Set-Cookie",
      await refreshTokenCookie.serialize("", { maxAge: 0 })
    );

    return headers;
  }
}

[alfredsgenkins]

A temporary solution from my side was to merge cookies:

class {
  async getLogoutHeaders() {
    const headers = new Headers();

    headers.append(
      "Set-Cookie",
      await idTokenCookie.serialize("", { maxAge: 0 })
    );

    headers.append(
      "Set-Cookie",
      await refreshTokenCookie.serialize("", { maxAge: 0 })
    );

    return new Headers([["Set-Cookie", headers.get("Set-Cookie") || ""]]);
  }
}

Yet, I do not think the solution is great.

[wenzf]

A temporary solution from my side was to merge cookies:
Yet, I do not think the solution is great.

I haven't found a stable solution for this issue which works in production mode yet. I don't fully understand why, my best guess is that is because of race conditions.

My case: I tried to delete a cookie1 and create a new cookie2 in the same header. My workaround was to delete cookie1 on one route, redirect to another route where cookie2 is added and redirect again to the final target route. That worked for me, but your workaround seems more elegant than mine.

What I can't really explain is that the solution I posted previously, worked in production for about a month and after updating the project, it broke.

[alfredsgenkins]

@wenzf have you tried combining them, as I did? I also noticed that despite being combined, on local it still sends cookies separately, and in production it does indeed split them apart. Seems like it could indeed be the difference in server implementation, I am using remix vite:dev in development and SST ion in production.

[wenzf]

@alfredsgenkins I haven't tried your solution combining the cookies yet, but I'll give it a try for sure in future!

I'm using arc with classic compiler. Seeing the same issue with different deployment pipelines doesn't indicate that different server implementations is the cause of this.

SST looks very interesting btw., I need to have a closer look at that. Would be great having an alternative to arc.

[enure]

@alfredsgenkins this might be related: sst/ion#702

[wenzf]

@alfredsgenkins check out this https://github.com/epicweb-dev/epic-stack/blob/main/app/utils/misc.tsx#L90