From c7d348a565056d08edaafcb6ba7e040f2e2ff695 Mon Sep 17 00:00:00 2001 From: Bereket Engida <86073083+Bekacru@users.noreply.github.com> Date: Tue, 25 Feb 2025 09:47:10 +0300 Subject: [PATCH] fix(two-factor): should respect `remember me` value when a user logins in with 2fa (#1566) * fix(two-factor): respect remember me value when using 2fa login * fix(two-factor): clean up temporary cookies --- .../better-auth/src/cookies/cookies.test.ts | 1 - packages/better-auth/src/cookies/index.ts | 23 +++++++++++++++---- .../src/plugins/two-factor/index.ts | 2 +- .../src/plugins/two-factor/otp/index.ts | 1 + .../src/plugins/two-factor/two-factor.test.ts | 16 +++++++++++-- .../plugins/two-factor/verify-middleware.ts | 14 ++++++++++- 6 files changed, 47 insertions(+), 10 deletions(-) diff --git a/packages/better-auth/src/cookies/cookies.test.ts b/packages/better-auth/src/cookies/cookies.test.ts index a44a84e1f2..d2124bde47 100644 --- a/packages/better-auth/src/cookies/cookies.test.ts +++ b/packages/better-auth/src/cookies/cookies.test.ts @@ -13,7 +13,6 @@ describe("cookies", async () => { { onResponse(context) { const setCookie = context.response.headers.get("set-cookie"); - console; expect(setCookie).toBeDefined(); expect(setCookie).toContain("Path=/"); expect(setCookie).toContain("HttpOnly"); diff --git a/packages/better-auth/src/cookies/index.ts b/packages/better-auth/src/cookies/index.ts index bd9b22bc4b..b2d6ff54dc 100644 --- a/packages/better-auth/src/cookies/index.ts +++ b/packages/better-auth/src/cookies/index.ts @@ -148,6 +148,14 @@ export async function setSessionCookie( dontRememberMe?: boolean, overrides?: Partial, ) { + const dontRememberMeCookie = await ctx.getSignedCookie( + ctx.context.authCookies.dontRememberToken.name, + ctx.context.secret, + ); + // if dontRememberMe is not set, use the cookie value + dontRememberMe = + dontRememberMe !== undefined ? dontRememberMe : !!dontRememberMeCookie; + const options = ctx.context.authCookies.sessionToken.options; const maxAge = dontRememberMe ? undefined @@ -192,7 +200,10 @@ export async function setSessionCookie( } } -export function deleteSessionCookie(ctx: GenericEndpointContext) { +export function deleteSessionCookie( + ctx: GenericEndpointContext, + skipDontRememberMe?: boolean, +) { ctx.setCookie(ctx.context.authCookies.sessionToken.name, "", { ...ctx.context.authCookies.sessionToken.options, maxAge: 0, @@ -201,10 +212,12 @@ export function deleteSessionCookie(ctx: GenericEndpointContext) { ...ctx.context.authCookies.sessionData.options, maxAge: 0, }); - ctx.setCookie(ctx.context.authCookies.dontRememberToken.name, "", { - ...ctx.context.authCookies.dontRememberToken.options, - maxAge: 0, - }); + if (!skipDontRememberMe) { + ctx.setCookie(ctx.context.authCookies.dontRememberToken.name, "", { + ...ctx.context.authCookies.dontRememberToken.options, + maxAge: 0, + }); + } } export function parseCookies(cookieHeader: string) { diff --git a/packages/better-auth/src/plugins/two-factor/index.ts b/packages/better-auth/src/plugins/two-factor/index.ts index ab801d1480..c9bf1fb3cd 100644 --- a/packages/better-auth/src/plugins/two-factor/index.ts +++ b/packages/better-auth/src/plugins/two-factor/index.ts @@ -289,7 +289,7 @@ export const twoFactor = (options?: TwoFactorOptions) => { /** * remove the session cookie. It's set by the sign in credential */ - deleteSessionCookie(ctx); + deleteSessionCookie(ctx, true); await ctx.context.internalAdapter.deleteSession(data.session.token); const twoFactorCookie = ctx.context.createAuthCookie( TWO_FACTOR_COOKIE_NAME, diff --git a/packages/better-auth/src/plugins/two-factor/otp/index.ts b/packages/better-auth/src/plugins/two-factor/otp/index.ts index 74087a281b..0a0989af95 100644 --- a/packages/better-auth/src/plugins/two-factor/otp/index.ts +++ b/packages/better-auth/src/plugins/two-factor/otp/index.ts @@ -204,6 +204,7 @@ export const otp2fa = (options?: OTPOptions) => { await ctx.context.internalAdapter.deleteSession( ctx.context.session.session.token, ); + await setSessionCookie(ctx, { session: newSession, user: updatedUser, diff --git a/packages/better-auth/src/plugins/two-factor/two-factor.test.ts b/packages/better-auth/src/plugins/two-factor/two-factor.test.ts index 103c5c9fd9..41a3c8caa5 100644 --- a/packages/better-auth/src/plugins/two-factor/two-factor.test.ts +++ b/packages/better-auth/src/plugins/two-factor/two-factor.test.ts @@ -113,19 +113,27 @@ describe("two factor", async () => { const res = await client.signIn.email({ email: testUser.email, password: testUser.password, + rememberMe: false, fetchOptions: { - onSuccess(context) { + onResponse(context) { const parsed = parseSetCookieHeader( context.response.headers.get("Set-Cookie") || "", ); expect(parsed.get("better-auth.session_token")?.value).toBe(""); expect(parsed.get("better-auth.two_factor")?.value).toBeDefined(); + expect(parsed.get("better-auth.dont_remember")?.value).toBeDefined(); headers.append( "cookie", `better-auth.two_factor=${ parsed.get("better-auth.two_factor")?.value }`, ); + headers.append( + "cookie", + `better-auth.dont_remember=${ + parsed.get("better-auth.dont_remember")?.value + }`, + ); }, }, }); @@ -151,11 +159,15 @@ describe("two factor", async () => { code: OTP, fetchOptions: { headers, - onSuccess(context) { + onResponse(context) { const parsed = parseSetCookieHeader( context.response.headers.get("Set-Cookie") || "", ); expect(parsed.get("better-auth.session_token")?.value).toBeDefined(); + // max age should be undefined because we are not using remember me + expect( + parsed.get("better-auth.session_token")?.["max-age"], + ).not.toBeDefined(); }, }, }); diff --git a/packages/better-auth/src/plugins/two-factor/verify-middleware.ts b/packages/better-auth/src/plugins/two-factor/verify-middleware.ts index 398cde3c92..3bc8033a5b 100644 --- a/packages/better-auth/src/plugins/two-factor/verify-middleware.ts +++ b/packages/better-auth/src/plugins/two-factor/verify-middleware.ts @@ -39,9 +39,14 @@ export const verifyTwoFactorMiddleware = createAuthMiddleware( message: "invalid two factor cookie", }); } + const dontRememberMe = await ctx.getSignedCookie( + ctx.context.authCookies.dontRememberToken.name, + ctx.context.secret, + ); const session = await ctx.context.internalAdapter.createSession( userId, ctx.request, + !!dontRememberMe, ); if (!session) { throw new APIError("INTERNAL_SERVER_ERROR", { @@ -69,13 +74,20 @@ export const verifyTwoFactorMiddleware = createAuthMiddleware( ctx.context.secret, `${user.id}!${session.token}`, ); - await ctx.setSignedCookie( trustDeviceCookie.name, `${token}!${session.token}`, ctx.context.secret, trustDeviceCookie.attributes, ); + // delete the dont remember me cookie + ctx.setCookie(ctx.context.authCookies.dontRememberToken.name, "", { + maxAge: 0, + }); + // delete the two factor cookie + ctx.setCookie(cookieName.name, "", { + maxAge: 0, + }); } return ctx.json({ token: session.token,