nextauthjs / next-auth

Authentication for the Web.
https://authjs.dev
ISC License
24.31k stars 3.39k forks source link

Are token cookies updated when getting the session in `@auth/sveltekit`? #8034

Closed torablien closed 8 months ago

torablien commented 1 year ago

Question 💬

I noticed that when my token expires, every subsequent request refreshes the token, seemingly because that refreshed token doesn't persist.

I found https://github.com/nextauthjs/next-auth/issues/7025#issuecomment-1479462037 which states that getServerSession in the app/ directory in NextJS only provides read access to cookies. Does the same apply to Sveltekit? Are there any workarounds so that we can support token refresh?

How to reproduce ☕️

I've setup auth with Cognito with @auth/sveltekit per https://authjs.dev/guides/basics/refresh-token-rotation. When the token expires, observe the token be continually refreshed every time it is grabbed via getSession() with the expiry time of the original token, not the most recent refreshed token expiry time.

import type { Provider } from "@auth/core/providers";
import Cognito from "@auth/core/providers/cognito";
import type { TokenSet } from "@auth/core/types";
import { SvelteKitAuth } from "@auth/sveltekit";

import {
  AUTH_SECRET,
  AWS_COGNITO_CLIENT_ID,
  AWS_COGNITO_CLIENT_SECRET,
  AWS_COGNITO_DOMAIN,
  AWS_COGNITO_REGION,
  AWS_COGNITO_USER_POOL_ID,
} from "$env/static/private";

export const handle = SvelteKitAuth({
  secret: AUTH_SECRET,
  trustHost: true,
  providers: [
    Cognito({
      clientId: AWS_COGNITO_CLIENT_ID,
      clientSecret: AWS_COGNITO_CLIENT_SECRET,
      issuer: `https://cognito-idp.${AWS_COGNITO_REGION}.amazonaws.com/${AWS_COGNITO_USER_POOL_ID}`,
      authorization: {
        params: {
          scope: "openid profile email phone",
        },
      },
    }),
  ] as Provider[], // See https://github.com/nextauthjs/next-auth/issues/6174.
  callbacks: {
    jwt: async ({ token, user, account, profile, trigger, session }) => {
      if (account) {
        return {
          access_token: account.access_token,
          expires_at: Date.now() + account.expires_in! * 1000,
          refresh_token: account.refresh_token,
          user: {
            id: profile?.sub,
            name: profile?.name,
            email: profile?.email,
            username: profile?.["cognito:username" as keyof typeof profile],
            groups: profile?.["cognito:groups" as keyof typeof profile],
          },
        };
      } else if (Date.now() < (token.expires_at! as number)) {
        // If the access token has not expired yet, return it.
        return token;
      } else {
        // If the access token has expired, try to refresh it.
        console.log("Token expired at", token.expires_at! as number);
        try {
          const response = await fetch(
            `https://${AWS_COGNITO_DOMAIN}.auth.${AWS_COGNITO_REGION}.amazoncognito.com/oauth2/token`,
            {
              headers: { "Content-Type": "application/x-www-form-urlencoded" },
              body: new URLSearchParams({
                client_id: AWS_COGNITO_CLIENT_ID,
                client_secret: AWS_COGNITO_CLIENT_SECRET,
                grant_type: "refresh_token",
                refresh_token: token.refresh_token as string,
              }),
              method: "POST",
            }
          );

          const tokens: TokenSet = await response.json();

          if (!response.ok) {
            throw tokens;
          }

          console.log("Token refreshed: new token expires at", Date.now() + tokens.expires_in! * 1000);

          return {
            ...token,
            access_token: tokens.access_token,
            expires_at: Date.now() + tokens.expires_in! * 1000,
            refresh_token: tokens.refresh_token ?? token.refresh_token,
          };
        } catch (error) {
          console.error("Error refreshing access token", error);
          // From the docs:
          // * Returning `null` will invalidate the JWT session by clearing
          // * the user's cookies. You'll still have to monitor and invalidate
          // * unexpired tokens from future requests yourself to prevent
          // * unauthorized access.
          return null;
          // return { ...token, error: 'RefreshAccessTokenError' as const }
        }
      }
    },
    session: ({ session, token }) => {
      const { access_token, user, expires_at } = token;
      return {
        ...session,
        user: user as any,
        accessToken: access_token as string | undefined,
        tokenExpiresAt: expires_at as number | undefined,
      };
    },
  },
});

Contributing 🙌🏽

Yes, I am willing to help answer this question in a PR

claraisrael commented 1 year ago

Facing the same issue, has there been any update on this matter?

torablien commented 1 year ago

@balazsorban44 @ThangHuuVu - would you mind confirming if my understanding is correct here?

AFAICT, it does not seem like cookies are handled in getSession() so I believe refresh token rotation is not supported yet. I'm not familiar with the internal implementation but it seems like, in the getSession() implementation: https://github.com/nextauthjs/next-auth/blob/d0dd2ababc82a1501c031df50db56e85fa7253de/packages/frameworks-sveltekit/src/lib/index.ts#L212-L230 @auth/core will return the cookies in response but then they aren't handled as only the status and body (.json()) are read from that response?

I took a look at the NextJS implementation and it looks like there is more explicit cookie handling in getServerSession() here: https://github.com/nextauthjs/next-auth/blob/d0dd2ababc82a1501c031df50db56e85fa7253de/packages/next-auth/src/next/index.ts#L166-L234

I'd really appreciate if you have suggestions on workarounds (Cognito has max 1 day access token expiry so it's pretty much unusable without refreshes). One thought was to flag in the session when the token is refreshed and then try to manually overwrite the (Secure-)next-auth.session-token.0 and (Secure-)next-auth.session-token.1 but I'm still looking into if it is feasible to generate those values myself from the refreshed token set. Another was to monkey patch getSession in @auth/sveltekit to pass down the new cookies and then set the cookies with those values when calling getSession. These don't seem particularly great and my lack of familiarity with the implementation internals here makes me concerned about potential footguns so it'd be great if you could provide some pointers! Many thanks!

shadohead commented 1 year ago

Seeing the same issue as well, would be great if there's any update on it.

kubieduber commented 1 year ago

Same issue here. Haven't been able to resolve. Is there any kind of workaround or fix?

torablien commented 11 months ago

This remains an open question

benjaminknox commented 10 months ago

So I ran into this issue @torablien your analysis in your comment above is correct, when getSession() is called it returns only the body from the backend and the header to set the authentication cookie is lost.

@kubieduber @torablien I was able to create a workaround by creating another function getSessionWithSetCookies function to return the set-cookie headers and then set them myself in +layout.server.ts. I plan on opening a PR with a change with something like this to fix it too.

Here are details on my workaround:

It uses a change to +layout.server.ts where I'm calling getSessionWithSetCookies:

// src/routes/+layout.server.ts
import type { LayoutServerLoad } from "./$types"

export const load: LayoutServerLoad = async (event) => {
  const sessionWithCookies = await event.locals.getSessionWithSetCookies();

  for(const cookieName in sessionWithCookies?.cookies) {
    const { value, options } = sessionWithCookies.cookies[cookieName]
    event.cookies.set(cookieName, value, options)
  }

  return {
    session: sessionWithCookies?.session
  }
}

And a patch to @auth/sveltekit (see patch-package for info on how to apply this):

# patches/@auth+sveltekit+0.3.16.patch
diff --git a/node_modules/@auth/sveltekit/index.d.ts b/node_modules/@auth/sveltekit/index.d.ts
index 80a6b97..b223a79 100644
--- a/node_modules/@auth/sveltekit/index.d.ts
+++ b/node_modules/@auth/sveltekit/index.d.ts
@@ -223,6 +223,7 @@ declare global {
     namespace App {
         interface Locals {
             getSession(): Promise<Session | null>;
+            getSessionWithSetCookies(): Promise<{ session: Session, cookies: Record<string, Record<string, string>>} | null>;
         }
         interface PageData {
             session?: Session | null;
diff --git a/node_modules/@auth/sveltekit/index.js b/node_modules/@auth/sveltekit/index.js
index b383260..eeebaf3 100644
--- a/node_modules/@auth/sveltekit/index.js
+++ b/node_modules/@auth/sveltekit/index.js
@@ -203,7 +203,22 @@ import { dev } from "$app/environment";
 import { base } from "$app/paths";
 import { env } from "$env/dynamic/private";
 import { Auth } from "@auth/core";
-export async function getSession(req, config) {
+
+const parseCookies = (cookies) => cookies.reduce((accumulator, cookie) => {
+  const headerValue = cookie.pop()
+
+  if(!headerValue) return;
+  const cookieParameters = headerValue.split(';')
+  const [name, value] = cookieParameters.shift()?.split('=') ?? [];
+  const options = cookieParameters.reduce((accumulator, parameter) => {
+    const [name, value] = parameter.split('=')
+    return {[name]: value ?? true, ...accumulator}
+  }, {});
+
+  return {[name]: {value, options}, ...accumulator}
+}, {})
+
+export async function getSessionWithSetCookies(req, config) {
     config.secret ??= env.AUTH_SECRET;
     config.trustHost ??= true;
     const prefix = config.prefix ?? `${base}/auth`;
@@ -214,10 +229,19 @@ export async function getSession(req, config) {
     const data = await response.json();
     if (!data || !Object.keys(data).length)
         return null;
-    if (status === 200)
-        return data;
+    if (status === 200) return {
+      session: data,
+      cookies: parseCookies(Array.from(response.headers).filter(([headerName]) => headerName === 'set-cookie'))
+    }
     throw new Error(data.message);
 }
+
+export async function getSession(req, config) {
+  const data = await getSessionWithSetCookies(req, config)
+
+  return data.session
+}
+
 const actions = [
     "providers",
     "session",
@@ -236,6 +260,7 @@ function AuthHandle(svelteKitAuthOptions) {
         const { prefix = `${base}/auth` } = authOptions;
         const { url, request } = event;
         event.locals.getSession ??= () => getSession(request, authOptions);
+        event.locals.getSessionWithSetCookies ??= () => getSessionWithSetCookies(request, authOptions);
         const action = url.pathname
             .slice(prefix.length + 1)
             .split("/")[0];
ndom91 commented 9 months ago

@benjaminknox would love a pr of this!

ndom91 commented 9 months ago

@benjaminknox I made a draft PR of your changes, but I'm having trouble with the cookies types. I think the parseCookie fn is already getting them in in the format of [{ name: '', value: '' }, { .. }], could that be right?

axel-zarate commented 9 months ago

I was able to successfully replicate @benjaminknox 's workaround with a few changes.

if (status === 200) return {
  session: data,
-  cookies: parseCookies(Array.from(response.headers).filter(([headerName]) => headerName === 'set-cookie'))
+  cookies: parseCookies(response.headers.getSetCookie())
}

I modified the parseCookies to accept the result of response.headers.getSetCookie(), added type information and a function to convert the cookie option names to camelCase:

interface SetCookie { value: string; options: CookieSerializeOptions & { path: string }; }

function camelize(str: string) {
  return str.replace(/(?:^\w|[A-Z]|\b\w)/g, (word, index) => {
    return index === 0 ? word.toLowerCase() : word.toUpperCase();
  }).replace(/\s+/g, '');
}

const parseCookies = (cookies: string[]) => cookies.reduce((accumulator, headerValue) => {

  if (!headerValue) {
    return accumulator;
  }
  const cookieParameters = headerValue.split(';')
  const [name, value] = cookieParameters.shift()?.split('=') ?? [];
  const options = cookieParameters.reduce((accumulator2, parameter) => {
    const [name, value] = parameter.split('=');
    const camelCaseName = camelize(name.trim());
    if (camelCaseName === 'expires') {
      const expires = new Date(value);
      return { ...accumulator2, expires };
    }
    return { ...accumulator2, [camelCaseName]: value ?? true };
  }, { path: '/' });

  return { [name]: { value, options }, ...accumulator }
}, {} as Record<string, SetCookie>);

I also had to add a special case for expires because a Date value is expected.

Instead of setting the cookies in +layout.server.ts, I created a wrapper for SvelteKitAuth:

export function SvelteKitAuth2(
  options: SvelteKitAuthConfig
): Handle {
  const auth = SvelteKitAuth(options);

  return async function ({ event, resolve }) {
    let r: ReturnType<typeof getSessionWithSetCookies> | null = null;
    event.locals.getSession = async () => {
      if (!r) {
        r = getSessionWithSetCookies(event.request, options).then(result => {
          if (result?.cookies) {
            for (const cookieName in result.cookies) {
              const { value, options } = result.cookies[cookieName]
              event.cookies.set(cookieName, value, options)
            }
          }
          return result;
        });
      }

      return r.then(result => result?.session ?? null);
    };
    const response = await auth({ event, resolve });

    return response;
  };
}