Next-auth: Application cookies not returned as an array in res.getHeader('Set-Cookie') breaks the set cookie function used in callbacks

[brett-chisholm]

Describe the bug
Application cookies not returned as an array in res.getHeader('Set-Cookie') breaks the set cookie function used in callbacks.

To Reproduce
Set an additional cookie in your application e.g. next-i18next=fr; Path=/; Expires=Tue, 06 Jul 2021 14:50:02 GMT; SameSite=Strict.

Expected behavior
The cookie should potentially be ignored as it is not associated with next-auth.

Screenshots or error logs

(node:33194) UnhandledPromiseRejectionWarning: TypeError: setCookieHeader.push is not a function
    at Object.set ([PROJECT_PATH]/node_modules/next-auth/dist/server/lib/cookie.js:23:19)
    at [PROJECT_PATH]/node_modules/next-auth/dist/server/routes/callback.js:329:23

Additional context
As an example; next-i18next sets a cookie so as the app can remember what language was selected last. When this is included in the res, it causes the above error.

Documentation feedback
Documentation refers to searching through online documentation, code comments and issue history. The example project refers to next-auth-example.

• [ x ] Found the documentation helpful
• [ ] Found documentation but was incomplete
• [ ] Could not find relevant documentation
• [ x ] Found the example project helpful
• [ ] Did not find the example project helpful

[iaincollins]

Hmm that is odd, it's explicitly supposed to be fine with other cookies. If you are seeing an error it does sound like there is a bug there, just having an extra cookie in the request should not be a problem.

To confirm, can I ask how you are deploying this and if you are doing anything exotic regarding mutating the responses, like trying to wrap calls from NextAuth.js in a custom server or otherwise adding things to the response object?

[brett-chisholm]

I am also using nextI18NextMiddleware with a basic server, could this be the problem? Aside from this, none of the above!

const express = require('express');
const next = require('next');
const nextI18NextMiddleware = require('next-i18next/middleware').default;

const nextI18next = require('./i18n');

const port = process.env.PORT || 3000;
const app = next({ dev: process.env.NODE_ENV !== 'production' });
const handle = app.getRequestHandler();

(async () => {
    await app.prepare();
    const server = express();

    await nextI18next.initPromise;
    server.use(nextI18NextMiddleware(nextI18next));

    server.post('*', (req, res) => handle(req, res));
    server.get('*', (req, res) => handle(req, res));

    await server.listen(port);
    console.log(`> Ready on http://localhost:${port}`);
})();

[iaincollins]

Ah yeah that will cause a problem and I am not sure if we can work around it. I think _maybe_ in this instance, but not always, as different middleware solutions do different things to request/response objects and at different times in the flow.

If you can select not apply it on /auth/api/ routes that would be the easiest and most robust solution. It looks like it has an ignoreRoutes option which should work.

On a related note, I appreciate there is a lack of built in i18 support and that is a problem for built-in pages and errors, but it's a tricky one to handle well for all use cases so for now I've left it to folks to create custom pages to handle for themselves (but might be something that I revisit in future).

[brett-chisholm]

It looks like the ignoreRoutes option isn't done at a middleware level. Regardless, this doesn't look like a next-auth issue so apologies! A type check here does appear to fix my issue though and I can't see that affecting anything else as it is expecting an array anyway?

[iaincollins]

It looks like the ignoreRoutes option isn't done at a middleware level.

Oh, boo!

A type check here does appear to fix my issue though and I can't see that affecting anything else as it is expecting an array anyway?

It might be interesting to post your fix here for other folks!

I am not quite sure why it actually breaks either. I think _maybe_ it can be a string if it's only one item but an array also works, and changing nextAuth check which it is first might also be a solution. I would guess maybe if two cookies were being the bug doesn't get triggered, but if it's exactly one then it might explode.

[iaincollins]

e.g.

If we change this:

  // Preserve any existing cookies that have already been set in the same session
  const setCookieHeader = res.getHeader('Set-Cookie') || []
  setCookieHeader.push(_serialize(name, String(stringValue), options))
  res.setHeader('Set-Cookie', setCookieHeader)

to this:

  // Preserve any existing cookies that have already been set in the same session
  let setCookieHeader = res.getHeader('Set-Cookie') || []
  // If not an array, make it an array
  if (!Array.isArray(setCookieHeader)) { setCookieHeader = [setCookieHeader] }
  setCookieHeader.push(_serialize(name, String(stringValue), options))
  res.setHeader('Set-Cookie', setCookieHeader)

[brett-chisholm]

var setCookieHeader = typeof res.getHeader('Set-Cookie') === 'array' ? res.getHeader('Set-Cookie') : []; as opposed to var setCookieHeader = res.getHeader('Set-Cookie') || []; does the trick!

The problem here being: setCookieHeader.push(_serialize(name, String(stringValue), options)); is expecting an array.

[iaincollins]

That makes sense - we probably don't need to actually return a value for the other cookie as it's already set (and ignoring it if it's a string is fine), but I'll probably try the approach of convert it into an array so that any cookies set already on the response are preserved, for ideal compatibility with other middleware folks are using.

[iaincollins]

Lesson #8123154 in pitfalls of not just importing a library to handle stuff like this I guess 🙃

[brett-chisholm]

Amazing, thanks for above! Thanks for next-auth as well, I've really enjoyed integrating it into a couple of pre-existing apps I have, its helping no end with securely managing my user flows 😄

[zach-aries]

@brett-chisholm did you find a workaround for this issue in your custom server file? I'm getting this exact same problem.

[iaincollins]

@zach-aries I think we addressed this in the v3 beta (but is not in v2).