From ff4a1d16cbcd25ea9ff716165d60760eb709e867 Mon Sep 17 00:00:00 2001 From: Luke Vella Date: Thu, 13 Feb 2025 10:14:03 +0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20infinite=20loop=20when=20t?= =?UTF-8?q?rying=20to=20migrate=20legacy=20cookie=20(#1561)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .env.development | 3 + apps/web/.env.test | 1 + apps/web/src/auth/edge/index.ts | 1 + apps/web/src/auth/edge/with-auth.ts | 63 ++++++++++++ apps/web/src/auth/legacy/helpers/jwt.ts | 3 +- apps/web/src/auth/legacy/helpers/types.ts | 46 --------- .../auth/legacy/next-auth-cookie-migration.ts | 95 ++++++++++--------- apps/web/src/auth/middleware.ts | 15 --- apps/web/src/middleware.ts | 55 +++++------ apps/web/src/next-auth.ts | 19 +++- apps/web/src/utils/session/session-config.ts | 4 +- apps/web/tests/helpers/next-auth-v4.ts | 36 +++++++ apps/web/tests/next-auth-migration.spec.ts | 57 +++++++++++ scripts/docker-start.sh | 1 + 14 files changed, 260 insertions(+), 139 deletions(-) create mode 100644 apps/web/src/auth/edge/index.ts create mode 100644 apps/web/src/auth/edge/with-auth.ts delete mode 100644 apps/web/src/auth/legacy/helpers/types.ts delete mode 100644 apps/web/src/auth/middleware.ts create mode 100644 apps/web/tests/helpers/next-auth-v4.ts create mode 100644 apps/web/tests/next-auth-migration.spec.ts diff --git a/.env.development b/.env.development index 90d0e02ab..14fb5c81d 100644 --- a/.env.development +++ b/.env.development @@ -5,6 +5,9 @@ SECRET_PASSWORD=abcdef1234567890abcdef1234567890 # Example: https://example.com NEXT_PUBLIC_BASE_URL=http://localhost:3000 +# AUTH_URL should be the same as NEXT_PUBLIC_BASE_URL +AUTH_URL=http://localhost:3000 + # A connection string to your Postgres database DATABASE_URL="postgres://postgres:postgres@localhost:5450/rallly" diff --git a/apps/web/.env.test b/apps/web/.env.test index a0696e836..dace8322a 100644 --- a/apps/web/.env.test +++ b/apps/web/.env.test @@ -1,5 +1,6 @@ PORT=3002 NEXT_PUBLIC_BASE_URL=http://localhost:3002 +AUTH_URL=$NEXT_PUBLIC_BASE_URL SECRET_PASSWORD=abcdef1234567890abcdef1234567890 DATABASE_URL=postgres://postgres:postgres@localhost:5450/rallly SUPPORT_EMAIL=support@rallly.co diff --git a/apps/web/src/auth/edge/index.ts b/apps/web/src/auth/edge/index.ts new file mode 100644 index 000000000..349f48458 --- /dev/null +++ b/apps/web/src/auth/edge/index.ts @@ -0,0 +1 @@ +export * from "./with-auth"; diff --git a/apps/web/src/auth/edge/with-auth.ts b/apps/web/src/auth/edge/with-auth.ts new file mode 100644 index 000000000..226c7f5d1 --- /dev/null +++ b/apps/web/src/auth/edge/with-auth.ts @@ -0,0 +1,63 @@ +import type { NextResponse } from "next/server"; +import type { NextAuthRequest, Session } from "next-auth"; +import NextAuth from "next-auth"; + +import { nextAuthConfig } from "@/next-auth.config"; + +import { + getLegacySession, + migrateLegacyJWT, +} from "../legacy/next-auth-cookie-migration"; + +const { auth } = NextAuth(nextAuthConfig); + +export const withAuth = ( + middleware: (request: NextAuthRequest) => Promise, +) => { + return async (request: NextAuthRequest) => { + let legacySession: Session | null = null; + + try { + legacySession = await getLegacySession(); + } catch (e) { + console.error(e); + } + + let session = legacySession; + + if (!session) { + try { + session = await auth(); + } catch (e) { + console.error(e); + } + } + + try { + const res = await nextAuthConfig.callbacks.authorized({ + request, + auth: session, + }); + + if (res !== true) { + return res; + } + } catch (e) { + console.error(e); + } + + request.auth = session; + + const middlewareRes = await middleware(request); + + if (legacySession) { + try { + await migrateLegacyJWT(middlewareRes); + } catch (e) { + console.error(e); + } + } + + return middlewareRes; + }; +}; diff --git a/apps/web/src/auth/legacy/helpers/jwt.ts b/apps/web/src/auth/legacy/helpers/jwt.ts index d7f3269cf..c93ceeff7 100644 --- a/apps/web/src/auth/legacy/helpers/jwt.ts +++ b/apps/web/src/auth/legacy/helpers/jwt.ts @@ -1,7 +1,6 @@ import hkdf from "@panva/hkdf"; import { jwtDecrypt } from "jose"; - -import type { JWT } from "./types"; +import type { JWT } from "next-auth/jwt"; /** Decodes a NextAuth.js issued JWT. */ export async function decodeLegacyJWT(token: string): Promise { diff --git a/apps/web/src/auth/legacy/helpers/types.ts b/apps/web/src/auth/legacy/helpers/types.ts deleted file mode 100644 index 82b95ff3b..000000000 --- a/apps/web/src/auth/legacy/helpers/types.ts +++ /dev/null @@ -1,46 +0,0 @@ -export interface DefaultJWT extends Record { - name?: string | null; - email?: string | null; - picture?: string | null; - sub?: string; -} - -/** - * Returned by the `jwt` callback and `getToken`, when using JWT sessions - * - * [`jwt` callback](https://next-auth.js.org/configuration/callbacks#jwt-callback) | [`getToken`](https://next-auth.js.org/tutorials/securing-pages-and-api-routes#using-gettoken) - */ -export interface JWT extends Record, DefaultJWT {} - -export interface JWTEncodeParams { - /** The JWT payload. */ - token?: JWT; - /** - * Used in combination with `secret` when deriving the encryption secret for the various NextAuth.js-issued JWTs. - * @note When no `salt` is passed, we assume this is a session token. - * This is for backwards-compatibility with currently active sessions, so they won't be invalidated when upgrading the package. - */ - salt?: string; - /** The key material used to encode the NextAuth.js issued JWTs. Defaults to `NEXTAUTH_SECRET`. */ - secret: string | Buffer; - /** - * The maximum age of the NextAuth.js issued JWT in seconds. - * @default 30 * 24 * 60 * 60 // 30 days - */ - maxAge?: number; -} - -export interface JWTDecodeParams { - /** The NextAuth.js issued JWT to be decoded */ - token?: string; - /** - * Used in combination with `secret` when deriving the encryption secret for the various NextAuth.js-issued JWTs. - * @note When no `salt` is passed, we assume this is a session token. - * This is for backwards-compatibility with currently active sessions, so they won't be invalidated when upgrading the package. - */ - salt?: string; - /** The key material used to decode the NextAuth.js issued JWTs. Defaults to `NEXTAUTH_SECRET`. */ - secret: string | Buffer; -} - -export type Secret = string | Buffer; diff --git a/apps/web/src/auth/legacy/next-auth-cookie-migration.ts b/apps/web/src/auth/legacy/next-auth-cookie-migration.ts index b4f51ec71..0faac5613 100644 --- a/apps/web/src/auth/legacy/next-auth-cookie-migration.ts +++ b/apps/web/src/auth/legacy/next-auth-cookie-migration.ts @@ -1,65 +1,70 @@ -import type { NextRequest } from "next/server"; -import { NextResponse } from "next/server"; +import { absoluteUrl } from "@rallly/utils/absolute-url"; +import { cookies } from "next/headers"; +import type { NextResponse } from "next/server"; +import type { Session } from "next-auth"; import { encode } from "next-auth/jwt"; import { decodeLegacyJWT } from "./helpers/jwt"; -const isSecureCookie = - process.env.NEXT_PUBLIC_BASE_URL?.startsWith("https://") ?? false; +const isSecureCookie = absoluteUrl().startsWith("https://"); const prefix = isSecureCookie ? "__Secure-" : ""; const oldCookieName = prefix + "next-auth.session-token"; const newCookieName = prefix + "authjs.session-token"; +export async function getLegacySession(): Promise { + const cookieStore = cookies(); + const legacySessionCookie = cookieStore.get(oldCookieName); + if (legacySessionCookie) { + const decodedCookie = await decodeLegacyJWT(legacySessionCookie.value); + + if (decodedCookie?.sub) { + const { sub: id, ...rest } = decodedCookie; + return { + user: { id, ...rest }, + expires: decodedCookie.exp + ? new Date(decodedCookie.exp * 1000).toISOString() + : new Date(Date.now() + 30 * 60 * 60 * 1000).toISOString(), + }; + } + } + + return null; +} + +async function getLegacyJWT() { + const cookieStore = cookies(); + const legacySessionCookie = cookieStore.get(oldCookieName); + if (legacySessionCookie) { + const decodedCookie = await decodeLegacyJWT(legacySessionCookie.value); + if (decodedCookie) { + return decodedCookie; + } + } + return null; +} + /** - * Migrates the next-auth cookies to the new authjs cookie names - * This is needed for next-auth v5 which renamed the cookie prefix from 'next-auth' to 'authjs' + * Replace the old legacy cookie with the new one */ -export function withAuthMigration( - middleware: (req: NextRequest) => void | Response | Promise, -) { - return async (req: NextRequest) => { - const oldCookie = req.cookies.get(oldCookieName); +export async function migrateLegacyJWT(res: NextResponse) { + const legacyJWT = await getLegacyJWT(); - // If the old cookie doesn't exist, return the middleware - if (!oldCookie) { - return middleware(req); - } - - const response = NextResponse.redirect(req.url); - response.cookies.delete(oldCookieName); - - // If the new cookie exists, delete the old cookie first and rerun middleware - if (req.cookies.get(newCookieName)) { - return response; - } - - const decodedCookie = await decodeLegacyJWT(oldCookie.value); - - // If old cookie is invalid, delete the old cookie first and rerun middleware - if (!decodedCookie) { - return response; - } - - // Set the new cookie - const encodedCookie = await encode({ - token: decodedCookie, + if (legacyJWT) { + const newJWT = await encode({ + token: legacyJWT, secret: process.env.SECRET_PASSWORD, salt: newCookieName, }); - // Set the new cookie with the same value and attributes - response.cookies.set(newCookieName, encodedCookie, { - path: "/", - secure: isSecureCookie, - sameSite: "lax", + res.cookies.set(newCookieName, newJWT, { httpOnly: true, + secure: isSecureCookie, + expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * 7), + sameSite: "lax", + path: "/", }); - - // Delete the old cookie - response.cookies.delete(oldCookieName); - - return response; - }; + res.cookies.delete(oldCookieName); + } } diff --git a/apps/web/src/auth/middleware.ts b/apps/web/src/auth/middleware.ts deleted file mode 100644 index e6a7038c0..000000000 --- a/apps/web/src/auth/middleware.ts +++ /dev/null @@ -1,15 +0,0 @@ -import type { NextRequest } from "next/server"; -import type { NextAuthRequest } from "next-auth"; -import NextAuth from "next-auth"; - -import { nextAuthConfig } from "@/next-auth.config"; - -const { auth } = NextAuth(nextAuthConfig); - -export function withAuth( - middleware: ( - req: NextAuthRequest, - ) => void | Response | Promise, -): (req: NextRequest) => void | Response | Promise { - return (req: NextRequest) => auth(middleware)(req, undefined as never); -} diff --git a/apps/web/src/middleware.ts b/apps/web/src/middleware.ts index a9104eda6..3d8fa2d38 100644 --- a/apps/web/src/middleware.ts +++ b/apps/web/src/middleware.ts @@ -3,44 +3,41 @@ import { withPostHog } from "@rallly/posthog/next/middleware"; import { NextResponse } from "next/server"; import { getLocaleFromHeader } from "@/app/guest"; -import { withAuthMigration } from "@/auth/legacy/next-auth-cookie-migration"; -import { withAuth } from "@/auth/middleware"; +import { withAuth } from "@/auth/edge"; const supportedLocales = Object.keys(languages); -export const middleware = withAuthMigration( - withAuth(async (req) => { - const { nextUrl } = req; - const newUrl = nextUrl.clone(); +export const middleware = withAuth(async (req) => { + const { nextUrl } = req; + const newUrl = nextUrl.clone(); - const isLoggedIn = req.auth?.user?.email; + const isLoggedIn = req.auth?.user?.email; - // if the user is already logged in, don't let them access the login page - if (/^\/(login)/.test(newUrl.pathname) && isLoggedIn) { - newUrl.pathname = "/"; - return NextResponse.redirect(newUrl); - } + // if the user is already logged in, don't let them access the login page + if (/^\/(login)/.test(newUrl.pathname) && isLoggedIn) { + newUrl.pathname = "/"; + return NextResponse.redirect(newUrl); + } - // Check if locale is specified in cookie - let locale = req.auth?.user?.locale; - if (locale && supportedLocales.includes(locale)) { - newUrl.pathname = `/${locale}${newUrl.pathname}`; - } else { - // Check if locale is specified in header - locale = await getLocaleFromHeader(req); - newUrl.pathname = `/${locale}${newUrl.pathname}`; - } + // Check if locale is specified in cookie + let locale = req.auth?.user?.locale; + if (locale && supportedLocales.includes(locale)) { + newUrl.pathname = `/${locale}${newUrl.pathname}`; + } else { + // Check if locale is specified in header + locale = await getLocaleFromHeader(req); + newUrl.pathname = `/${locale}${newUrl.pathname}`; + } - const res = NextResponse.rewrite(newUrl); - res.headers.set("x-pathname", newUrl.pathname); + const res = NextResponse.rewrite(newUrl); + res.headers.set("x-pathname", newUrl.pathname); - if (req.auth?.user?.id) { - await withPostHog(res, { distinctID: req.auth.user.id }); - } + if (req.auth?.user?.id) { + await withPostHog(res, { distinctID: req.auth.user.id }); + } - return res; - }), -); + return res; +}); export const config = { matcher: ["/((?!api|_next/static|_next/image|static|.*\\.).*)"], diff --git a/apps/web/src/next-auth.ts b/apps/web/src/next-auth.ts index ddfb83bc1..9f34fa13b 100644 --- a/apps/web/src/next-auth.ts +++ b/apps/web/src/next-auth.ts @@ -7,6 +7,7 @@ import z from "zod"; import { CustomPrismaAdapter } from "./auth/adapters/prisma"; import { isEmailBlocked } from "./auth/helpers/is-email-blocked"; import { mergeGuestsIntoUser } from "./auth/helpers/merge-user"; +import { getLegacySession } from "./auth/legacy/next-auth-cookie-migration"; import { EmailProvider } from "./auth/providers/email"; import { GoogleProvider } from "./auth/providers/google"; import { GuestProvider } from "./auth/providers/guest"; @@ -22,7 +23,12 @@ const sessionUpdateSchema = z.object({ weekStart: z.number().nullish(), }); -export const { auth, handlers, signIn, signOut } = NextAuth({ +const { + auth: originalAuth, + handlers, + signIn, + signOut, +} = NextAuth({ ...nextAuthConfig, adapter: CustomPrismaAdapter({ migrateData: async (userId) => { @@ -169,3 +175,14 @@ export const { auth, handlers, signIn, signOut } = NextAuth({ }, }, }); + +const auth = async () => { + const session = await getLegacySession(); + if (session) { + return session; + } + + return originalAuth(); +}; + +export { auth, handlers, signIn, signOut }; diff --git a/apps/web/src/utils/session/session-config.ts b/apps/web/src/utils/session/session-config.ts index b2bfe774a..87d2b31dc 100644 --- a/apps/web/src/utils/session/session-config.ts +++ b/apps/web/src/utils/session/session-config.ts @@ -1,8 +1,10 @@ +import { absoluteUrl } from "@rallly/utils/absolute-url"; + export const sessionConfig = { password: process.env.SECRET_PASSWORD ?? "", cookieName: "rallly-session", cookieOptions: { - secure: process.env.NEXT_PUBLIC_BASE_URL?.startsWith("https://") ?? false, + secure: absoluteUrl().startsWith("https://") ?? false, }, ttl: 60 * 60 * 24 * 30, // 30 days }; diff --git a/apps/web/tests/helpers/next-auth-v4.ts b/apps/web/tests/helpers/next-auth-v4.ts new file mode 100644 index 000000000..90a07c25c --- /dev/null +++ b/apps/web/tests/helpers/next-auth-v4.ts @@ -0,0 +1,36 @@ +import hkdf from "@panva/hkdf"; +import { EncryptJWT } from "jose"; +import type { JWT } from "next-auth/jwt"; + +const now = () => (Date.now() / 1000) | 0; +export async function getDerivedEncryptionKey( + keyMaterial: string | Buffer, + salt: string, +) { + return await hkdf( + "sha256", + keyMaterial, + salt, + `NextAuth.js Generated Encryption Key${salt ? ` (${salt})` : ""}`, + 32, + ); +} + +interface JWTEncodeParams { + token?: JWT; + salt?: string; + secret: string | Buffer; + maxAge?: number; +} + +export async function encode(params: JWTEncodeParams) { + /** @note empty `salt` means a session token. See {@link JWTEncodeParams.salt}. */ + const { token = {}, secret, maxAge = 30 * 24 * 60 * 60, salt = "" } = params; + const encryptionSecret = await getDerivedEncryptionKey(secret, salt); + return await new EncryptJWT(token) + .setProtectedHeader({ alg: "dir", enc: "A256GCM" }) + .setIssuedAt() + .setExpirationTime(now() + maxAge) + .setJti("some-random-id") + .encrypt(encryptionSecret); +} diff --git a/apps/web/tests/next-auth-migration.spec.ts b/apps/web/tests/next-auth-migration.spec.ts new file mode 100644 index 000000000..e2b61650d --- /dev/null +++ b/apps/web/tests/next-auth-migration.spec.ts @@ -0,0 +1,57 @@ +import { expect, test } from "@playwright/test"; +import { prisma } from "@rallly/database"; + +import { encode } from "./helpers/next-auth-v4"; + +const legacyGuestId = "user-1234"; + +test.describe.serial(() => { + test.beforeAll(async () => { + await prisma.poll.create({ + data: { + id: "legacy-guest-poll", + title: "Test Poll", + adminUrlId: "admin-url-id", + participantUrlId: "participant-url-id", + guestId: legacyGuestId, + }, + }); + }); + test.afterAll(async () => { + await prisma.poll.delete({ + where: { + id: "legacy-guest-poll", + }, + }); + }); + + test("should see poll on login page", async ({ page }) => { + const context = page.context(); + const legacyToken = await encode({ + token: { + sub: legacyGuestId, + }, + secret: process.env.SECRET_PASSWORD, + }); + + // set cookie to simulate legacy guest + await context.addCookies([ + { + name: "next-auth.session-token", + value: legacyToken, + httpOnly: true, + expires: Date.now() / 1000 + 60 * 60 * 24 * 7, + secure: false, + sameSite: "Lax", + domain: "localhost", + path: "/", + }, + ]); + + // For some reason it doesn't work unless we need to redirect + await page.goto("/login"); + + // Check if the poll title exists in the page content + await expect(page.getByText("Test Poll")).toBeVisible(); + }); +}); diff --git a/scripts/docker-start.sh b/scripts/docker-start.sh index 262f0754f..c9988fa0e 100755 --- a/scripts/docker-start.sh +++ b/scripts/docker-start.sh @@ -2,6 +2,7 @@ set -e export DIRECT_DATABASE_URL=$DATABASE_URL +export AUTH_URL=$NEXT_PUBLIC_BASE_URL prisma migrate deploy --schema=./prisma/schema.prisma node apps/web/server.js