From 8aba2653267f723c9c602e64f1edea923ccf94a2 Mon Sep 17 00:00:00 2001 From: Tiago <1585571+xernobyl@users.noreply.github.com> Date: Thu, 27 Aug 2026 09:48:58 +0000 Subject: [PATCH] fix(auth): hand the post-verification session only to the browser that signed up (#8971) --- apps/web/app/api/auth/[...all]/route.ts | 10 +- apps/web/i18n.lock | 2 + apps/web/locales/de-DE.json | 2 + apps/web/locales/en-US.json | 2 + apps/web/locales/es-ES.json | 2 + apps/web/locales/fr-FR.json | 2 + apps/web/locales/hu-HU.json | 2 + apps/web/locales/ja-JP.json | 2 + apps/web/locales/nl-NL.json | 2 + apps/web/locales/pt-BR.json | 2 + apps/web/locales/pt-PT.json | 2 + apps/web/locales/ro-RO.json | 2 + apps/web/locales/ru-RU.json | 2 + apps/web/locales/sv-SE.json | 2 + apps/web/locales/tr-TR.json | 2 + apps/web/locales/zh-Hans-CN.json | 2 + apps/web/locales/zh-Hant-TW.json | 2 + .../modules/auth/lib/after-auth-hooks.test.ts | 24 +- apps/web/modules/auth/lib/after-auth-hooks.ts | 5 + apps/web/modules/auth/lib/auth-cookies.ts | 38 +++ ...uth-email-verification.integration.test.ts | 31 ++- apps/web/modules/auth/lib/auth.ts | 55 ++-- .../lib/better-auth-email-verification.ts | 23 ++ .../lib/better-auth-observability.test.ts | 37 +++ .../auth/lib/better-auth-observability.ts | 43 +++- ...erification-autosignin.integration.test.ts | 158 ++++++++++++ ...etter-auth-verification-autosignin.test.ts | 238 ++++++++++++++++++ .../better-auth-verification-autosignin.ts | 173 +++++++++++++ .../lib/email-verification-request-context.ts | 56 +++++ .../modules/auth/lib/signup-intent.test.ts | 150 +++++++++++ apps/web/modules/auth/lib/signup-intent.ts | 156 ++++++++++++ .../auth/login/components/login-form.tsx | 10 + apps/web/modules/auth/login/page.tsx | 4 + apps/web/modules/auth/signup/actions.test.ts | 31 ++- apps/web/modules/auth/signup/actions.ts | 33 +++ .../verification-requested/actions.test.ts | 73 ++++++ .../auth/verification-requested/actions.ts | 31 ++- 37 files changed, 1368 insertions(+), 43 deletions(-) create mode 100644 apps/web/modules/auth/lib/auth-cookies.ts create mode 100644 apps/web/modules/auth/lib/better-auth-verification-autosignin.integration.test.ts create mode 100644 apps/web/modules/auth/lib/better-auth-verification-autosignin.test.ts create mode 100644 apps/web/modules/auth/lib/better-auth-verification-autosignin.ts create mode 100644 apps/web/modules/auth/lib/email-verification-request-context.ts create mode 100644 apps/web/modules/auth/lib/signup-intent.test.ts create mode 100644 apps/web/modules/auth/lib/signup-intent.ts diff --git a/apps/web/app/api/auth/[...all]/route.ts b/apps/web/app/api/auth/[...all]/route.ts index bfb6f3a8b32f..548693302ee6 100644 --- a/apps/web/app/api/auth/[...all]/route.ts +++ b/apps/web/app/api/auth/[...all]/route.ts @@ -1,6 +1,7 @@ import { auth } from "@/modules/auth/lib/auth"; import { createAuthPathLabeller } from "@/modules/auth/lib/better-auth-path-label"; import { runWithBetterAuthRequestContext } from "@/modules/auth/lib/better-auth-request-context"; +import { runWithEmailVerificationRequestContext } from "@/modules/auth/lib/email-verification-request-context"; import { mapLegacySsoCallbackRequest } from "@/modules/auth/lib/legacy-sso-callback"; import { normalizeDcrRequest } from "@/modules/auth/lib/mcp-dcr-application-type"; import { runWithSsoRequestContext } from "@/modules/ee/sso/lib/sso-request-context"; @@ -63,7 +64,14 @@ const handler = async (request: Request): Promise => { const mappedRequest = await normalizeDcrRequest(mapLegacySsoCallbackRequest(request)); return runWithBetterAuthRequestContext( { path: labelAuthPath(mappedRequest.url), method: mappedRequest.method }, - () => runWithSsoRequestContext(() => auth.handler(mappedRequest)) + () => + runWithSsoRequestContext(() => + // ENG-2562: carries "this request just verified an email" from Better Auth's + // `afterEmailVerification` hook to the `hooks.after` chain, which is where the session can + // actually be minted. Innermost because it is the narrowest scope of the three — one endpoint, + // not the whole handler. + runWithEmailVerificationRequestContext(() => auth.handler(mappedRequest)) + ) ); }; diff --git a/apps/web/i18n.lock b/apps/web/i18n.lock index 885e3e81efa8..6da374bf295b 100644 --- a/apps/web/i18n.lock +++ b/apps/web/i18n.lock @@ -44,6 +44,8 @@ checksums: auth/last_used: 9049bf77c57c92b7a5a760ba2a40128c auth/login/backup_code: 870711a6df11ae55366d018255b466cf auth/login/create_an_account: bc53a57281d2701563f94f3169518028 + auth/login/email_verified_sign_in_description: 91d4c968d2e1a8e576755bc4a4724054 + auth/login/email_verified_sign_in_title: f960899104c4e22c27ab699272b2bbc1 auth/login/enter_your_backup_code: f2a6905ea0e8d2b4093b5773d72df88b auth/login/enter_your_two_factor_authentication_code: 07a020191cd54ee4a0aa1aa4ddc166bf auth/login/forgot_your_password: 0baf970b4598f1af36623b714410dfae diff --git a/apps/web/locales/de-DE.json b/apps/web/locales/de-DE.json index fa888f59a614..2a15582b689d 100644 --- a/apps/web/locales/de-DE.json +++ b/apps/web/locales/de-DE.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Backup-Code", "create_an_account": "Konto erstellen", + "email_verified_sign_in_description": "Deine E-Mail-Adresse ist bestätigt. Melde dich an, um fortzufahren. Falls du dieses Konto nicht erstellt hast, setze stattdessen das Passwort zurück.", + "email_verified_sign_in_title": "E-Mail bestätigt", "enter_your_backup_code": "Gib deinen Backup-Code ein", "enter_your_two_factor_authentication_code": "Gib deinen Zwei-Faktor-Authentifizierungscode ein", "forgot_your_password": "Passwort vergessen?", diff --git a/apps/web/locales/en-US.json b/apps/web/locales/en-US.json index 1f2ffb6f0b36..2cf95fbd8f4b 100644 --- a/apps/web/locales/en-US.json +++ b/apps/web/locales/en-US.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Backup code", "create_an_account": "Create an account", + "email_verified_sign_in_description": "Your email address is confirmed. Sign in to continue. If you didn't create this account, reset the password instead.", + "email_verified_sign_in_title": "Email verified", "enter_your_backup_code": "Enter your backup code", "enter_your_two_factor_authentication_code": "Enter your two-factor authentication code", "forgot_your_password": "Forgot your password?", diff --git a/apps/web/locales/es-ES.json b/apps/web/locales/es-ES.json index 07fd8f27e695..89d673464727 100644 --- a/apps/web/locales/es-ES.json +++ b/apps/web/locales/es-ES.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Código de respaldo", "create_an_account": "Crear una cuenta", + "email_verified_sign_in_description": "Tu dirección de correo electrónico está confirmada. Inicia sesión para continuar. Si no has creado esta cuenta, restablece la contraseña en su lugar.", + "email_verified_sign_in_title": "Correo electrónico verificado", "enter_your_backup_code": "Introduce tu código de respaldo", "enter_your_two_factor_authentication_code": "Introduce tu código de autenticación de dos factores", "forgot_your_password": "¿Has olvidado tu contraseña?", diff --git a/apps/web/locales/fr-FR.json b/apps/web/locales/fr-FR.json index cd6e41dee7c3..59a1127d9bd7 100644 --- a/apps/web/locales/fr-FR.json +++ b/apps/web/locales/fr-FR.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Code de sauvegarde", "create_an_account": "Créer un compte", + "email_verified_sign_in_description": "Votre adresse e-mail est confirmée. Connectez-vous pour continuer. Si vous n'avez pas créé ce compte, réinitialisez plutôt le mot de passe.", + "email_verified_sign_in_title": "E-mail vérifié", "enter_your_backup_code": "Entrez votre code de sauvegarde", "enter_your_two_factor_authentication_code": "Entrez votre code d'authentification à deux facteurs.", "forgot_your_password": "Mot de passe oublié ?", diff --git a/apps/web/locales/hu-HU.json b/apps/web/locales/hu-HU.json index 187c25564ef6..0e051f4762bf 100644 --- a/apps/web/locales/hu-HU.json +++ b/apps/web/locales/hu-HU.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Visszaszerzési kód", "create_an_account": "Fiók létrehozása", + "email_verified_sign_in_description": "Az e-mail-címe megerősítve. Jelentkezzen be a folytatáshoz. Ha nem Ön hozta létre ezt a fiókot, ehelyett állítsa vissza a jelszót.", + "email_verified_sign_in_title": "E-mail megerősítve", "enter_your_backup_code": "Visszaszerzési kód megadása", "enter_your_two_factor_authentication_code": "Kétfaktoros hitelesítési kód megadása", "forgot_your_password": "Elfelejtette a jelszavát?", diff --git a/apps/web/locales/ja-JP.json b/apps/web/locales/ja-JP.json index 81561f36efe0..d6cc2715caf6 100644 --- a/apps/web/locales/ja-JP.json +++ b/apps/web/locales/ja-JP.json @@ -56,6 +56,8 @@ "login": { "backup_code": "バックアップコード", "create_an_account": "アカウントを作成", + "email_verified_sign_in_description": "メールアドレスが確認されました。続行するにはログインしてください。このアカウントに心当たりがない場合は、代わりにパスワードをリセットしてください。", + "email_verified_sign_in_title": "メールアドレスを確認しました", "enter_your_backup_code": "バックアップコードを入力してください", "enter_your_two_factor_authentication_code": "二段階認証コードを入力してください", "forgot_your_password": "パスワードを忘れてしまいましたか？", diff --git a/apps/web/locales/nl-NL.json b/apps/web/locales/nl-NL.json index 80a84b9ac184..1802dd175701 100644 --- a/apps/web/locales/nl-NL.json +++ b/apps/web/locales/nl-NL.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Back-upcode", "create_an_account": "Maak een account aan", + "email_verified_sign_in_description": "Uw e-mailadres is bevestigd. Log in om door te gaan. Als u dit account niet hebt aangemaakt, stel dan in plaats daarvan het wachtwoord opnieuw in.", + "email_verified_sign_in_title": "E-mail geverifieerd", "enter_your_backup_code": "Voer uw back-upcode in", "enter_your_two_factor_authentication_code": "Voer uw tweefactorauthenticatiecode in", "forgot_your_password": "Wachtwoord vergeten?", diff --git a/apps/web/locales/pt-BR.json b/apps/web/locales/pt-BR.json index 1ece3165b4c3..0a22f2bab72a 100644 --- a/apps/web/locales/pt-BR.json +++ b/apps/web/locales/pt-BR.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Código de backup", "create_an_account": "Cria uma conta", + "email_verified_sign_in_description": "Seu endereço de e-mail foi confirmado. Faça login para continuar. Se você não criou esta conta, redefina a senha em vez disso.", + "email_verified_sign_in_title": "E-mail verificado", "enter_your_backup_code": "Digite seu código de backup", "enter_your_two_factor_authentication_code": "Digite seu código de autenticação de dois fatores", "forgot_your_password": "Esqueceu sua senha?", diff --git a/apps/web/locales/pt-PT.json b/apps/web/locales/pt-PT.json index ab9de5a62110..bbaa3bf8ad98 100644 --- a/apps/web/locales/pt-PT.json +++ b/apps/web/locales/pt-PT.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Código de backup", "create_an_account": "Criar uma conta", + "email_verified_sign_in_description": "O seu endereço de e-mail foi confirmado. Inicie sessão para continuar. Se não foi você que criou esta conta, redefina antes a palavra-passe.", + "email_verified_sign_in_title": "E-mail verificado", "enter_your_backup_code": "Introduza o seu código de backup", "enter_your_two_factor_authentication_code": "Introduza o seu código de autenticação de dois fatores", "forgot_your_password": "Esqueceu-se da sua palavra-passe?", diff --git a/apps/web/locales/ro-RO.json b/apps/web/locales/ro-RO.json index a9a0c964a2a6..290a58a54db5 100644 --- a/apps/web/locales/ro-RO.json +++ b/apps/web/locales/ro-RO.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Cod de rezervă", "create_an_account": "Creează un cont", + "email_verified_sign_in_description": "Adresa ta de e-mail este confirmată. Autentifică-te pentru a continua. Dacă nu tu ai creat acest cont, resetează parola în schimb.", + "email_verified_sign_in_title": "E-mail verificat", "enter_your_backup_code": "Introduceți codul de rezervă", "enter_your_two_factor_authentication_code": "Introduceți codul dvs. de autentificare în doi pași", "forgot_your_password": "Ai uitat parola?", diff --git a/apps/web/locales/ru-RU.json b/apps/web/locales/ru-RU.json index 914dca57243b..49b064804e51 100644 --- a/apps/web/locales/ru-RU.json +++ b/apps/web/locales/ru-RU.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Резервный код", "create_an_account": "Создать аккаунт", + "email_verified_sign_in_description": "Ваш адрес электронной почты подтверждён. Войдите, чтобы продолжить. Если вы не создавали эту учётную запись, вместо этого сбросьте пароль.", + "email_verified_sign_in_title": "Электронная почта подтверждена", "enter_your_backup_code": "Введите резервный код", "enter_your_two_factor_authentication_code": "Введите код двухфакторной аутентификации", "forgot_your_password": "Забыли пароль?", diff --git a/apps/web/locales/sv-SE.json b/apps/web/locales/sv-SE.json index 518110351a18..ff468cd256cc 100644 --- a/apps/web/locales/sv-SE.json +++ b/apps/web/locales/sv-SE.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Reservkod", "create_an_account": "Skapa ett konto", + "email_verified_sign_in_description": "Din e-postadress är bekräftad. Logga in för att fortsätta. Om du inte har skapat det här kontot, återställ lösenordet i stället.", + "email_verified_sign_in_title": "E-post verifierad", "enter_your_backup_code": "Ange din reservkod", "enter_your_two_factor_authentication_code": "Ange din tvåfaktorsautentiseringskod", "forgot_your_password": "Glömt ditt lösenord?", diff --git a/apps/web/locales/tr-TR.json b/apps/web/locales/tr-TR.json index 08e5635fe97a..714cabd35631 100644 --- a/apps/web/locales/tr-TR.json +++ b/apps/web/locales/tr-TR.json @@ -56,6 +56,8 @@ "login": { "backup_code": "Yedek kod", "create_an_account": "Hesap oluştur", + "email_verified_sign_in_description": "E-posta adresiniz doğrulandı. Devam etmek için giriş yapın. Bu hesabı siz oluşturmadıysanız bunun yerine şifreyi sıfırlayın.", + "email_verified_sign_in_title": "E-posta doğrulandı", "enter_your_backup_code": "Yedek kodunuzu girin", "enter_your_two_factor_authentication_code": "İki faktörlü doğrulama kodunuzu girin", "forgot_your_password": "Şifrenizi mi unuttunuz?", diff --git a/apps/web/locales/zh-Hans-CN.json b/apps/web/locales/zh-Hans-CN.json index 6a5bf6b6db88..26106dc8562c 100644 --- a/apps/web/locales/zh-Hans-CN.json +++ b/apps/web/locales/zh-Hans-CN.json @@ -56,6 +56,8 @@ "login": { "backup_code": "备用代码", "create_an_account": "创建一个帐户", + "email_verified_sign_in_description": "您的电子邮件地址已确认。请登录以继续。如果这个账户不是您创建的，请改为重置密码。", + "email_verified_sign_in_title": "邮箱已验证", "enter_your_backup_code": "输入 你的 备用代码", "enter_your_two_factor_authentication_code": "输入 你的 双因素 认证 代码", "forgot_your_password": "忘记你的密码？", diff --git a/apps/web/locales/zh-Hant-TW.json b/apps/web/locales/zh-Hant-TW.json index 3a950d1575c0..3c71dbd5abdf 100644 --- a/apps/web/locales/zh-Hant-TW.json +++ b/apps/web/locales/zh-Hant-TW.json @@ -56,6 +56,8 @@ "login": { "backup_code": "備份碼", "create_an_account": "建立帳戶", + "email_verified_sign_in_description": "您的電子郵件地址已確認。請登入以繼續。如果這個帳戶不是您建立的，請改為重設密碼。", + "email_verified_sign_in_title": "電子郵件已驗證", "enter_your_backup_code": "輸入您的備份碼", "enter_your_two_factor_authentication_code": "輸入您的雙重驗證碼", "forgot_your_password": "忘記密碼？", diff --git a/apps/web/modules/auth/lib/after-auth-hooks.test.ts b/apps/web/modules/auth/lib/after-auth-hooks.test.ts index ea381d8c717e..fa0750596a2e 100644 --- a/apps/web/modules/auth/lib/after-auth-hooks.test.ts +++ b/apps/web/modules/auth/lib/after-auth-hooks.test.ts @@ -5,12 +5,18 @@ import { } from "@/modules/ee/sso/lib/better-auth-hooks"; import { runAfterAuthHooks } from "./after-auth-hooks"; import { auditFailedAuthAfter } from "./better-auth-observability"; +import { twoFactorBackfillAfterHandler } from "./better-auth-two-factor-backfill"; +import { verificationAutoSignInAfterHandler } from "./better-auth-verification-autosignin"; vi.mock("@/modules/ee/sso/lib/better-auth-hooks", () => ({ ssoRecoveryAfterHandler: vi.fn(), blockedSignupDomainRedirectAfterHandler: vi.fn(), })); vi.mock("./better-auth-observability", () => ({ auditFailedAuthAfter: vi.fn() })); +vi.mock("./better-auth-two-factor-backfill", () => ({ twoFactorBackfillAfterHandler: vi.fn() })); +vi.mock("./better-auth-verification-autosignin", () => ({ + verificationAutoSignInAfterHandler: vi.fn(), +})); describe("runAfterAuthHooks", () => { test("records the failed-auth audit before the personal-email redirect handler (which throws)", async () => { @@ -21,6 +27,12 @@ describe("runAfterAuthHooks", () => { vi.mocked(auditFailedAuthAfter).mockImplementation(async () => { calls.push("audit"); }); + vi.mocked(twoFactorBackfillAfterHandler).mockImplementation(async () => { + calls.push("two-factor-backfill"); + }); + vi.mocked(verificationAutoSignInAfterHandler).mockImplementation(async () => { + calls.push("verification-auto-sign-in"); + }); vi.mocked(blockedSignupDomainRedirectAfterHandler).mockImplementation(async () => { calls.push("redirect"); throw new Error("ctx.redirect"); // mirrors the real handler's ctx.redirect throw @@ -30,6 +42,16 @@ describe("runAfterAuthHooks", () => { // Pins the intended order: the audit runs before the redirect throw. Future-proofing today // (auditFailedAuthAfter only records /sign-in/email, so an SSO /callback rejection no-ops it either // way), but locks the contract for when the failed-auth audit is extended to SSO callback paths. - expect(calls).toEqual(["recovery", "audit", "redirect"]); + // + // The ENG-2562 auto-sign-in also has to sit ahead of that throw: it is the only handler here that + // GRANTS something, and a redirect thrown before it would silently cost a legitimate same-browser + // sign-up its session on any request that hit both. + expect(calls).toEqual([ + "recovery", + "audit", + "two-factor-backfill", + "verification-auto-sign-in", + "redirect", + ]); }); }); diff --git a/apps/web/modules/auth/lib/after-auth-hooks.ts b/apps/web/modules/auth/lib/after-auth-hooks.ts index 830543404add..d9fbcf039633 100644 --- a/apps/web/modules/auth/lib/after-auth-hooks.ts +++ b/apps/web/modules/auth/lib/after-auth-hooks.ts @@ -6,6 +6,7 @@ import { } from "@/modules/ee/sso/lib/better-auth-hooks"; import { auditFailedAuthAfter } from "./better-auth-observability"; import { twoFactorBackfillAfterHandler } from "./better-auth-two-factor-backfill"; +import { verificationAutoSignInAfterHandler } from "./better-auth-verification-autosignin"; /** * Composed Better Auth `hooks.after` chain. Ordering rationale: `auditFailedAuthAfter` runs before @@ -23,5 +24,9 @@ export const runAfterAuthHooks = async (ctx: AuthHookContext): Promise => // ENG-1824: heal legacy 2FA enrollments (no `TwoFactor` row) on successful password sign-in, before // the 2FA challenge. Only touches `/sign-in/email`, so it's unaffected by the SSO-only redirect below. await twoFactorBackfillAfterHandler(ctx); + // ENG-2562: mint the post-verification session only for the browser that signed up. Only touches + // `/verify-email`, so it is order-independent with respect to the SSO redirects above — placed last + // because it is the only handler here that adds a session rather than inspecting one. + await verificationAutoSignInAfterHandler(ctx); await blockedSignupDomainRedirectAfterHandler(ctx); }; diff --git a/apps/web/modules/auth/lib/auth-cookies.ts b/apps/web/modules/auth/lib/auth-cookies.ts new file mode 100644 index 000000000000..e754b501a5b6 --- /dev/null +++ b/apps/web/modules/auth/lib/auth-cookies.ts @@ -0,0 +1,38 @@ +import "server-only"; +import { env } from "@/lib/env"; + +/** + * Cookie/token settings shared by `auth.ts` and the sign-up intent cookie (ENG-2562). + * + * Extracted rather than exported from `auth.ts` to keep the import graph acyclic: `signup-intent.ts` + * is reached from `auth.ts` (→ `after-auth-hooks.ts` → `better-auth-verification-autosignin.ts`), so + * importing `auth.ts` back from there would close a cycle. A leaf module both sides can import is the + * cheap way out — the same reason `session-revocation.ts` reaches for `auth` dynamically. + */ + +/** + * `__Secure-`/Secure cookies require HTTPS — on http://localhost the browser drops them and the session + * can't persist. Gate on the configured URL scheme (parity with NextAuth's URL-based useSecureCookies + * default) instead of hardcoding true, so local/dev over http works. + * + * WEBAPP_URL is part of the chain because all three vars are optional: a deployment that sets only + * WEBAPP_URL=https://… — the primary documented variable — would otherwise fall through to "" and serve + * the session cookie without `Secure`, letting a downgrade to plaintext HTTP leak it. + */ +export const USE_SECURE_COOKIES = ( + env.BETTER_AUTH_URL ?? + env.NEXTAUTH_URL ?? + env.WEBAPP_URL ?? + "" +).startsWith("https://"); + +/** + * Lifetime of a verification link, and therefore of the sign-up intent cookie that has to outlive it. + * + * One constant on purpose: the intent cookie exists to answer "is the browser presenting this + * verification link the one that registered?", so a cookie that died before the link it is paired with + * would silently withhold the session from a legitimate same-browser sign-up — the exact UX ENG-1746 + * added and this fix is meant to preserve. Consumed by `emailVerification.expiresIn` in `auth.ts` and + * by `issueSignupIntentCookie`. + */ +export const EMAIL_VERIFICATION_TTL_SECONDS = 60 * 60; diff --git a/apps/web/modules/auth/lib/auth-email-verification.integration.test.ts b/apps/web/modules/auth/lib/auth-email-verification.integration.test.ts index 282bbfffda04..3618cb67180f 100644 --- a/apps/web/modules/auth/lib/auth-email-verification.integration.test.ts +++ b/apps/web/modules/auth/lib/auth-email-verification.integration.test.ts @@ -68,19 +68,16 @@ describe("Better Auth email verification (real Postgres)", () => { const verifiedUser = await prisma.user.findUnique({ where: { email: "verify@example.com" } }); expect(verifiedUser?.emailVerified).toBe(true); - // autoSignInAfterVerification (ENG-1746): consuming the link establishes a session so the user - // lands in the app already logged in instead of bouncing to /auth/login... - expect(await prisma.session.count()).toBe(1); - // ...and that session is captured in the signedIn audit trail, tagged as a credential-account - // ("password") sign-in via the /verify-email allow-list entry in getSignInAuthMethod. - expect(queueAuditEventBackground).toHaveBeenCalledTimes(1); - expect(queueAuditEventBackground).toHaveBeenCalledWith( - expect.objectContaining({ - action: "signedIn", - userId: verifiedUser?.id, - newObject: expect.objectContaining({ authMethod: "password" }), - }) - ); + // ENG-2562: consuming the link verifies the address but does NOT establish a session. This row + // used to assert 1 — the ENG-1746 auto-sign-in — and that is precisely the pre-hijacking defect: + // whoever clicks the link is not necessarily whoever chose the password, so handing the clicker a + // session signs a victim into an attacker's account. The session is now minted only against a + // sign-up intent cookie, which a server-side `auth.api.verifyEmail` has no way to carry (there is + // no browser here) — see better-auth-verification-autosignin.integration.test.ts for the granted + // path driven through the real hook chain. + expect(await prisma.session.count()).toBe(0); + // No session means no `signedIn` audit either. + expect(queueAuditEventBackground).not.toHaveBeenCalled(); // afterEmailVerification re-homes the createBrevoCustomer side effect (fire-and-forget) expect(brevo.createBrevoCustomer).toHaveBeenCalledWith({ id: verifiedUser?.id, @@ -90,13 +87,13 @@ describe("Better Auth email verification (real Postgres)", () => { expect(capturePostHogEvent).toHaveBeenCalledWith(verifiedUser?.id, "user_email_confirmed"); // verify-email is a stateless signed JWT (no getAndDelete), so re-verifying is idempotent: - // the already-verified branch returns { status: true, user: null } and creates no second session + // the already-verified branch returns { status: true, user: null } and creates no session const replay = await auth.api.verifyEmail({ query: { token } }); expect(replay).toMatchObject({ status: true, user: null }); - expect(await prisma.session.count()).toBe(1); - // afterEmailVerification fires once per user — the replay must NOT re-emit the event or the audit + expect(await prisma.session.count()).toBe(0); + // afterEmailVerification fires once per user — the replay must NOT re-emit the event expect(capturePostHogEvent).toHaveBeenCalledTimes(1); - expect(queueAuditEventBackground).toHaveBeenCalledTimes(1); + expect(queueAuditEventBackground).not.toHaveBeenCalled(); }); }); diff --git a/apps/web/modules/auth/lib/auth.ts b/apps/web/modules/auth/lib/auth.ts index 4b414f66a1ed..a34fa183b22f 100644 --- a/apps/web/modules/auth/lib/auth.ts +++ b/apps/web/modules/auth/lib/auth.ts @@ -28,8 +28,9 @@ import { ssoDatabaseHooks, ssoLicenseGateBeforeHandler } from "@/modules/ee/sso/ import { ssoGenericOAuthConfig, ssoSocialProviders } from "@/modules/ee/sso/lib/better-auth-providers"; import { ssoRecoverySignInPlugin } from "@/modules/ee/sso/lib/better-auth-recovery-signin"; import { runAfterAuthHooks } from "./after-auth-hooks"; +import { EMAIL_VERIFICATION_TTL_SECONDS, USE_SECURE_COOKIES } from "./auth-cookies"; import { rejectInactiveUserOnSessionCreate } from "./better-auth-active-user-gate"; -import { createBrevoCustomerAfterEmailVerification } from "./better-auth-email-verification"; +import { runAfterEmailVerificationHooks } from "./better-auth-email-verification"; import { hibpBreachCheckBeforeHandler } from "./better-auth-hibp"; import { auditPasswordReset, betterAuthLogger, signInAuditDatabaseHook } from "./better-auth-observability"; import { requirePasswordResetEnabledBeforeHandler } from "./better-auth-password-reset-gate"; @@ -40,16 +41,9 @@ import { signupPolicyBeforeHandler } from "./signup-policy"; const DAY_IN_SECONDS = 60 * 60 * 24; -// `__Secure-`/Secure cookies require HTTPS — on http://localhost the browser drops them and the -// session can't persist. Gate on the configured URL scheme (parity with NextAuth's URL-based -// useSecureCookies default) instead of hardcoding true, so local/dev over http works. -// -// WEBAPP_URL is part of the chain because all three vars are optional: a deployment that sets only -// WEBAPP_URL=https://… — the primary documented variable — would otherwise fall through to "" and -// serve the session cookie without `Secure`, letting a downgrade to plaintext HTTP leak it. -const USE_SECURE_COOKIES = (env.BETTER_AUTH_URL ?? env.NEXTAUTH_URL ?? env.WEBAPP_URL ?? "").startsWith( - "https://" -); +// `USE_SECURE_COOKIES` and `EMAIL_VERIFICATION_TTL_SECONDS` live in auth-cookies.ts — the sign-up +// intent cookie (ENG-2562) needs both, and it is reached FROM this module, so keeping them here and +// exporting them would close an import cycle. Their derivation is documented there. /** Resolve a user's locale for transactional emails (Better Auth's callback user omits it). */ export const getUserLocale = async (userId: string): Promise => { @@ -163,12 +157,20 @@ export const auth = betterAuth({ // this flag BA would throw EMAIL_NOT_VERIFIED without sending anything, leaving no recovery path // and making that message untrue. (ENG-1054) sendOnSignIn: true, - // Establish the session on a successful verification so the user lands in the app instead of - // bouncing to /auth/login (ENG-1746). Safe — clicking the signed link proves email ownership. - // Distinct from the sibling `autoSignIn: false` above, which stays off (no auto-login at sign-up, - // before ownership is proven; also enumeration-safe). - autoSignInAfterVerification: true, - expiresIn: 60 * 60, // 1 hour + // OFF since ENG-2562, and the reason is worth keeping: this used to be `true` "because clicking the + // signed link proves email ownership". It does — but it does not prove the clicker chose the + // account's PASSWORD, and conflating the two is account pre-hijacking. An attacker registers an + // address that has no account yet with a password they pick, Formbricks mails the victim a + // verification link, and the victim's click signs the victim into the attacker's account. + // + // The ENG-1746 UX it existed for (land in the app, not on /auth/login) is preserved for the case it + // was actually meant to serve — the same browser that signed up — by + // `verificationAutoSignInAfterHandler` in the `hooks.after` chain, which mints the session only + // against a sign-up intent cookie. Leaving this `false` is what makes that fail-closed. + autoSignInAfterVerification: false, + // Shared with the sign-up intent cookie: the cookie has to outlive the link it is paired with, or a + // legitimate same-browser sign-up silently loses its session. See auth-cookies.ts. + expiresIn: EMAIL_VERIFICATION_TTL_SECONDS, // ENG-2091: a failed send must never present as a successful one. Two ways it can hide: // 1. a THROW — on the sign-up path Better Auth calls this through `runInBackgroundOrAwait`, whose // catch only logs, so sign-up still resolves 200. (The resend endpoint awaits it directly and @@ -201,8 +203,10 @@ export const auth = betterAuth({ throw error; } }, - // Re-home the "token" provider's Brevo-on-first-verification side effect (better-auth-email-verification.ts). - afterEmailVerification: createBrevoCustomerAfterEmailVerification, + // Composed: records the ENG-2562 "this request just verified someone" marker for the auto-sign-in + // after-hook, then re-homes the "token" provider's Brevo-on-first-verification side effect. See + // better-auth-email-verification.ts for why the marker goes first. + afterEmailVerification: runAfterEmailVerificationHooks, }, // Hash verification/reset token identifiers at rest (BA default is plaintext) — matches the @@ -316,6 +320,19 @@ export const auth = betterAuth({ "/sign-up/email": { window: 60, max: 3 }, "/request-password-reset": { window: 60, max: 3 }, "/reset-password": { window: 60, max: 5 }, + // ENG-2562: the two verification endpoints, deliberately asymmetric because their risk is. + // + // Sending is the amplification vector — unauthenticated, attacker-triggerable, and it puts mail in + // someone else's inbox. Without a cap the 1-hour link expiry is not a real constraint on + // pre-hijacking either, since fresh links can be re-issued until the victim clicks. Same budget as + // the sibling reset request. + "/send-verification-email": { window: 60, max: 3 }, + // Verifying is the opposite case and must stay generous. It is a top-level GET clicked out of a + // mail client — frequently behind a corporate NAT shared by many users, and often prefetched by a + // mail-security scanner before the human clicks. It also already requires a signed, expiring + // token, so a tight per-IP cap buys very little and risks locking a legitimate user out of + // verifying at all. This is a flood guard, not an authorization control. + "/verify-email": { window: 60, max: 30 }, "/oauth2/register": { window: 60, max: 5 }, "/oauth2/token": { window: 60, max: 20 }, "/oauth2/introspect": { window: 60, max: 60 }, diff --git a/apps/web/modules/auth/lib/better-auth-email-verification.ts b/apps/web/modules/auth/lib/better-auth-email-verification.ts index 9acd93be5185..3c0e37fdb715 100644 --- a/apps/web/modules/auth/lib/better-auth-email-verification.ts +++ b/apps/web/modules/auth/lib/better-auth-email-verification.ts @@ -2,6 +2,7 @@ import "server-only"; import { logger } from "@formbricks/logger"; import { capturePostHogEvent } from "@/lib/posthog"; import { createBrevoCustomer } from "@/modules/auth/lib/brevo"; +import { markEmailJustVerified } from "./email-verification-request-context"; /** * Better Auth `afterEmailVerification` hook (ENG-1054, Phase 7) — re-homes the NextAuth `"token"` @@ -21,3 +22,25 @@ export const createBrevoCustomerAfterEmailVerification = async (user: { logger.error(err, "Failed to create Brevo customer after email verification") ); }; + +/** + * The composed `afterEmailVerification` hook. + * + * Ordering is load-bearing: the ENG-2562 marker is recorded FIRST, so that a fault in the CRM/analytics + * side effect cannot cost a legitimate sign-up its session. `markEmailJustVerified` is a synchronous + * store write with no I/O, which is what makes putting it first safe rather than merely convenient. + * + * The CRM half is additionally wrapped: it is fire-and-forget by design, and Better Auth awaits this + * hook inside the verification request, so letting a throw escape here would 500 a request that has + * already flipped `emailVerified` — leaving the user verified, unsigned-in, and with no way back but + * the login page. Same reasoning as the auto-sign-in after-handler. + */ +export const runAfterEmailVerificationHooks = async (user: { id: string; email: string }): Promise => { + markEmailJustVerified(user.id); + + try { + await createBrevoCustomerAfterEmailVerification(user); + } catch (error) { + logger.error({ error, userId: user.id }, "afterEmailVerification side effect failed"); + } +}; diff --git a/apps/web/modules/auth/lib/better-auth-observability.test.ts b/apps/web/modules/auth/lib/better-auth-observability.test.ts index b82417502609..db2ca45c81cf 100644 --- a/apps/web/modules/auth/lib/better-auth-observability.test.ts +++ b/apps/web/modules/auth/lib/better-auth-observability.test.ts @@ -11,6 +11,7 @@ import { UNKNOWN_DATA } from "@/modules/ee/audit-logs/types/audit-log"; import { auditFailedAuthAfter, auditPasswordReset, + auditVerificationSessionWithheld, betterAuthLogger, getSignInAuthMethod, redactEmailsInLogMessage, @@ -309,6 +310,42 @@ describe("auditPasswordReset (onPasswordReset audit)", () => { }); }); +describe("auditVerificationSessionWithheld (ENG-2562)", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + // The reason is the payload's reason for existing: `absent` is the ordinary cross-device click or a + // mail-scanner prefetch, while `other_user` is a valid intent cookie naming a different account. Both + // reach this event, and without the reason recorded the distinction cannot be recovered afterwards. + test.each(["absent", "invalid", "other_user", "grant_failed"])( + "queues an updated/user audit carrying the withheld marker and the reason %s", + async (reason) => { + await auditVerificationSessionWithheld("user-1", reason); + + expect(queueAuditEventBackground).toHaveBeenCalledWith( + expect.objectContaining({ + action: "updated", + targetType: "user", + userId: "user-1", + targetId: "user-1", + status: "success", + userType: "user", + newObject: { verificationSessionWithheldMarker: true, reason }, + }) + ); + } + ); + + // The caller runs inside a verification request that has already flipped `emailVerified`, so a throw + // out of the audit would 500 a link that verified the user. + test("never throws when the audit queue fails", async () => { + vi.mocked(queueAuditEventBackground).mockRejectedValueOnce(new Error("redis down")); + + await expect(auditVerificationSessionWithheld("user-1", "absent")).resolves.toBeUndefined(); + }); +}); + describe("betterAuthLogger (Sentry capture gating, ENG-2037)", () => { // Optional on the BetterAuthOptions["logger"] type, but always defined here. const log = betterAuthLogger.log!; diff --git a/apps/web/modules/auth/lib/better-auth-observability.ts b/apps/web/modules/auth/lib/better-auth-observability.ts index 76de82684bb3..bf76ded4e1ed 100644 --- a/apps/web/modules/auth/lib/better-auth-observability.ts +++ b/apps/web/modules/auth/lib/better-auth-observability.ts @@ -43,9 +43,13 @@ export const getSignInAuthMethod = (path: string | undefined): string | null => // onto this route before Better Auth sees it. Do not "restore" /oauth2/ here. if (path.includes("/callback/")) return "sso"; if (path === "/sign-in/email") return "password"; - // Auto-login after email verification (autoSignInAfterVerification, ENG-1746) creates a session for - // a credential/email-password account, so audit it as "password". Idempotent replays of an - // already-verified token don't create a session, so this fires once, on the genuine first verify. + // A session created on the verification endpoint belongs to a credential/email-password account, so + // audit it as "password". Since ENG-2562 the session is minted by `verificationAutoSignInAfterHandler` + // rather than by `autoSignInAfterVerification` (now off), and only for the browser that signed up — + // but it still arrives here with this path, so the signedIn trail is unchanged. A withheld + // verification creates no session and so produces no event here; it is recorded separately by + // `auditVerificationSessionWithheld`. Idempotent replays of an already-verified token create no + // session either, so this fires once, on the genuine first verify. if (path === "/verify-email") return "password"; // The 2FA challenge completes the credentials sign-in → "password" (matches NextAuth). Deliberately // NOT /two-factor/verify-otp (also the first-time-enable path) nor /two-factor/disable|enable. @@ -348,6 +352,39 @@ export const auditFailedAuthAfter = async (ctx: AuthHookContext): Promise logAuthAttempt(failureReason, "credentials", "password", UNKNOWN_DATA, email); }; +/** + * ENG-2562: a verification completed but no session was granted. + * + * `reason` is the point of the record, not a detail. This fires on the ordinary cross-device click and on + * a mail-scanner prefetch (`absent`) as readily as on a genuine pre-hijack (`other_user`), so the event + * alone means "nobody was signed in", NOT "an attack happened". Only `other_user` — a valid intent cookie + * naming a different account — is inherently suspicious; `invalid` is worth a look; `grant_failed` is our + * own fault rather than the caller's. + * + * Uses the `updated` + marker idiom of `auditPasswordReset` below rather than a new `ZAuditAction` + * value: it is the established shape for auth-internal events, and it keeps a shared enum out of a fix + * that has to land on two release branches as well as main. Audit logging is enterprise-gated, so the + * caller also logs — a self-hoster must still see this. + */ +export const auditVerificationSessionWithheld = async (userId: string, reason: string): Promise => { + try { + await queueAuditEventBackground({ + action: "updated", + targetType: "user", + userId, + targetId: userId, + organizationId: UNKNOWN_DATA, + status: "success", + userType: "user", + newObject: { verificationSessionWithheldMarker: true, reason }, + }); + } catch { + logger + .withContext({ source: "better-auth" }) + .error("Failed to queue withheld-verification-session audit event"); + } +}; + /** * Audit a completed password reset — parity with the retired `completePasswordReset` action audit * (`updated`/`user`). Wired into Better Auth's `emailAndPassword.onPasswordReset` callback (auth.ts), diff --git a/apps/web/modules/auth/lib/better-auth-verification-autosignin.integration.test.ts b/apps/web/modules/auth/lib/better-auth-verification-autosignin.integration.test.ts new file mode 100644 index 000000000000..bef8705b5cae --- /dev/null +++ b/apps/web/modules/auth/lib/better-auth-verification-autosignin.integration.test.ts @@ -0,0 +1,158 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { prisma } from "@formbricks/database"; +import { resetDb } from "@/integration/reset-db"; +import { auth } from "@/modules/auth/lib/auth"; +import { runWithEmailVerificationRequestContext } from "@/modules/auth/lib/email-verification-request-context"; +import { SIGNUP_INTENT_COOKIE_NAME, createSignupIntentToken } from "@/modules/auth/lib/signup-intent"; +import { queueAuditEventBackground } from "@/modules/ee/audit-logs/lib/handler"; +import { sendVerificationLinkEmail } from "@/modules/email"; + +// Spy the audit queue so the signedIn trail can be asserted without the real setImmediate/headers() +// emission (which has no request scope under vitest). +vi.mock("@/modules/ee/audit-logs/lib/handler", async (importOriginal) => ({ + ...(await importOriginal()), + queueAuditEventBackground: vi.fn().mockResolvedValue(undefined), +})); + +/** + * ENG-2562 against a real Postgres and a real Better Auth hook chain. + * + * Asserts through sessions rather than columns, because the defect is a session that should not exist: + * `emailVerified` is `true` on every path here — that write is correct, the mailbox really was proven — + * so a column-level assertion would call the vulnerable behaviour a pass. + * + * The verification request is wrapped in `runWithEmailVerificationRequestContext` because that is what + * the `/api/auth/[...all]` route does in production; the store is how "this request just verified + * someone" reaches the after-hook. Without it a server-side `auth.api.verifyEmail` withholds the + * session by design, which auth-email-verification.integration.test.ts covers. + */ + +const VICTIM = "victim@example.com"; +const ATTACKER_PASSWORD = "AttackerPassw0rd!"; + +const tokenFromLink = (link: string): string => new URL(link).searchParams.get("token") ?? ""; + +/** Sign up and return the verification token that was mailed, plus the created user's id. */ +const signUpAndCaptureToken = async (): Promise<{ token: string; userId: string }> => { + await auth.api.signUpEmail({ + body: { email: VICTIM, password: ATTACKER_PASSWORD, name: "Vic" }, + asResponse: true, + }); + const user = await prisma.user.findUnique({ where: { email: VICTIM } }); + const token = tokenFromLink(vi.mocked(sendVerificationLinkEmail).mock.calls[0][0].verifyLink); + + return { token, userId: user!.id }; +}; + +/** Drive `/verify-email` the way the route does, optionally carrying a sign-up intent cookie. */ +const verifyEmail = async (token: string, intentCookieValue?: string): Promise => { + const headers = new Headers(); + if (intentCookieValue) { + headers.set("cookie", `${SIGNUP_INTENT_COOKIE_NAME}=${intentCookieValue}`); + } + + return await runWithEmailVerificationRequestContext(async () => { + // asResponse so the thrown redirect on the withheld path is returned rather than propagated — + // and so the response's status, Location and set-cookie headers can be asserted, not just DB rows. + return await auth.api.verifyEmail({ query: { token }, headers, asResponse: true }); + }); +}; + +beforeEach(async () => { + await resetDb(); + vi.clearAllMocks(); +}); + +describe("post-verification auto-sign-in (real Postgres)", () => { + test("withholds the session when the verifying browser did not sign up", async () => { + const { token } = await signUpAndCaptureToken(); + + // The victim clicks the link the attacker's sign-up caused to be sent. Their browser carries no + // sign-up intent cookie, because they never signed up. + const response = await verifyEmail(token); + + // The address is verified — that part is correct and unchanged. + expect((await prisma.user.findUnique({ where: { email: VICTIM } }))?.emailVerified).toBe(true); + // But no session exists, so the victim is never signed into the attacker's account. Before this + // fix this count was 1, and that session is the whole vulnerability. + expect(await prisma.session.count()).toBe(0); + // At the HTTP layer too: no session cookie on the response, and the user is not silently bounced — + // they land on the login page with the "verified, now sign in" explanation. + expect(response.headers.get("set-cookie") ?? "").not.toContain("session_token="); + expect(response.status).toBeGreaterThanOrEqual(300); + expect(response.status).toBeLessThan(400); + expect(response.headers.get("location")).toContain("/auth/login?verified=1"); + }); + + test("mints the session when the intent cookie names the just-verified user", async () => { + const { token, userId } = await signUpAndCaptureToken(); + + // Same browser that signed up: the ENG-1746 land-in-the-app UX this fix is careful to preserve. + const response = await verifyEmail(token, createSignupIntentToken(userId)); + + expect(await prisma.session.count()).toBe(1); + const session = await prisma.session.findFirst(); + expect(session?.userId).toBe(userId); + // The session must reach the BROWSER, not just the database — a row without a set-cookie would + // leave the user signed out while this table reports success. + const setCookie = response.headers.get("set-cookie") ?? ""; + expect(setCookie).toContain("session_token="); + // And the spent intent cookie is cleared (single use), on the same response. + expect(setCookie).toContain(`${SIGNUP_INTENT_COOKIE_NAME}=;`); + // The signedIn audit trail must survive the move: this session is now minted by our after-hook + // rather than by autoSignInAfterVerification, and losing the event would be a silent hole in the + // sign-in audit rather than a visible failure. + expect(queueAuditEventBackground).toHaveBeenCalledWith( + expect.objectContaining({ + action: "signedIn", + userId, + newObject: expect.objectContaining({ authMethod: "password" }), + }) + ); + }); + + test("withholds when the intent cookie names a different account", async () => { + const { token } = await signUpAndCaptureToken(); + + // A cookie from an earlier, unrelated sign-up in the same browser must not buy a session here: + // proof of registering account A is not proof of registering account B. + await verifyEmail(token, createSignupIntentToken("some_other_user_id")); + + expect(await prisma.session.count()).toBe(0); + }); + + test("withholds when the intent cookie is not a sign-up intent token", async () => { + const { token, userId } = await signUpAndCaptureToken(); + + // Correctly signed with the same secret, wrong purpose — the shared-keyspace confusion the + // dedicated verifier exists to refuse. Uses the app's own token minter for a different flow. + const { createToken } = await import("@/lib/jwt"); + await verifyEmail(token, createToken(userId, { purpose: "sso_recovery" })); + + expect(await prisma.session.count()).toBe(0); + }); + + test("withholds when the intent cookie is garbage, rather than failing the verification", async () => { + const { token } = await signUpAndCaptureToken(); + + await verifyEmail(token, "not-a-jwt"); + + // Verified (so the user is not stranded) but not signed in. + expect((await prisma.user.findUnique({ where: { email: VICTIM } }))?.emailVerified).toBe(true); + expect(await prisma.session.count()).toBe(0); + }); + + test("the attacker's password still works afterwards — the documented residual", async () => { + const { token } = await signUpAndCaptureToken(); + await verifyEmail(token); + + // Stated as a test rather than left implicit: this fix withholds the session, it does not evict + // the squatter. If a later change makes it destructive, this row should be updated deliberately — + // not discovered in production. + const signIn = await auth.api.signInEmail({ + body: { email: VICTIM, password: ATTACKER_PASSWORD }, + asResponse: true, + }); + expect(signIn.status).toBe(200); + }); +}); diff --git a/apps/web/modules/auth/lib/better-auth-verification-autosignin.test.ts b/apps/web/modules/auth/lib/better-auth-verification-autosignin.test.ts new file mode 100644 index 000000000000..64f8ea541504 --- /dev/null +++ b/apps/web/modules/auth/lib/better-auth-verification-autosignin.test.ts @@ -0,0 +1,238 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; + +const mocks = vi.hoisted(() => ({ + getSessionFromCtx: vi.fn(), + setSessionCookie: vi.fn(), + getJustVerifiedUserId: vi.fn(), + classifySignupIntent: vi.fn(), + auditVerificationSessionWithheld: vi.fn(), +})); + +vi.mock("better-auth/api", () => ({ getSessionFromCtx: mocks.getSessionFromCtx })); +vi.mock("better-auth/cookies", () => ({ setSessionCookie: mocks.setSessionCookie })); +vi.mock("./email-verification-request-context", () => ({ + getJustVerifiedUserId: mocks.getJustVerifiedUserId, +})); +vi.mock("./signup-intent", () => ({ + SIGNUP_INTENT_COOKIE_NAME: "formbricks.signup_intent", + SIGNUP_INTENT_COOKIE_OPTIONS: { httpOnly: true, secure: false, path: "/", sameSite: "lax", maxAge: 3600 }, + classifySignupIntent: mocks.classifySignupIntent, +})); +vi.mock("./better-auth-observability", () => ({ + auditVerificationSessionWithheld: mocks.auditVerificationSessionWithheld, +})); +vi.mock("@/lib/constants", () => ({ WEBAPP_URL: "https://app.formbricks.com" })); +vi.mock("@formbricks/logger", () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +/** What the handler redirects to whenever it withholds the session. */ +const LOGIN_VERIFIED = "https://app.formbricks.com/auth/login?verified=1"; + +const { verificationAutoSignInAfterHandler } = await import("./better-auth-verification-autosignin"); + +const VERIFIED_USER = { id: "user_1", email: "victim@example.com" }; + +const buildCtx = (overrides: Record = {}) => { + const findUserById = vi.fn().mockResolvedValue(VERIFIED_USER); + const createSession = vi.fn().mockResolvedValue({ token: "session-token" }); + + return { + path: "/verify-email", + getCookie: vi.fn().mockReturnValue("intent-cookie"), + setCookie: vi.fn(), + // Mirrors Better Auth: `ctx.redirect` RETURNS a value the caller throws. + redirect: vi.fn((url: string) => new Error(`REDIRECT:${url}`)), + context: { + internalAdapter: { findUserById, createSession }, + authCookies: { sessionToken: { name: "formbricks.session_token" } }, + responseHeaders: new Headers(), + }, + ...overrides, + } as never; +}; + +const sessionOf = (ctx: never) => + (ctx as unknown as { context: { internalAdapter: { createSession: ReturnType } } }).context + .internalAdapter.createSession; + +describe("verificationAutoSignInAfterHandler", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getJustVerifiedUserId.mockReturnValue(VERIFIED_USER.id); + mocks.classifySignupIntent.mockReturnValue("valid"); + mocks.getSessionFromCtx.mockResolvedValue(null); + }); + + test("mints a session when the intent cookie names the verified user", async () => { + const ctx = buildCtx(); + + await verificationAutoSignInAfterHandler(ctx); + + expect(sessionOf(ctx)).toHaveBeenCalledWith(VERIFIED_USER.id, false); + expect(mocks.setSessionCookie).toHaveBeenCalledOnce(); + }); + + test("clears the intent cookie after use, so a replayed link cannot mint a second session", async () => { + const ctx = buildCtx(); + + await verificationAutoSignInAfterHandler(ctx); + + expect((ctx as unknown as { setCookie: ReturnType }).setCookie).toHaveBeenCalledWith( + "formbricks.signup_intent", + "", + expect.objectContaining({ maxAge: 0 }) + ); + }); + + test("withholds the session when there is no intent cookie", async () => { + mocks.classifySignupIntent.mockReturnValue("absent"); + const ctx = buildCtx(); + + await expect(verificationAutoSignInAfterHandler(ctx)).rejects.toThrow(`REDIRECT:${LOGIN_VERIFIED}`); + + expect(sessionOf(ctx)).not.toHaveBeenCalled(); + expect(mocks.setSessionCookie).not.toHaveBeenCalled(); + // Recorded with the reason: this path is mostly the ordinary cross-device click or a scanner + // prefetch, and only the reason distinguishes those from an actual pre-hijack (`other_user`). + expect(mocks.auditVerificationSessionWithheld).toHaveBeenCalledWith(VERIFIED_USER.id, "absent"); + }); + + test("withholds the session when the intent cookie names a different user", async () => { + // The pre-hijacking case with a stale cookie in the mix: proof of signing up for account A must not + // buy a session on account B. + mocks.classifySignupIntent.mockReturnValue("other_user"); + const ctx = buildCtx(); + + await expect(verificationAutoSignInAfterHandler(ctx)).rejects.toThrow(`REDIRECT:${LOGIN_VERIFIED}`); + + expect(sessionOf(ctx)).not.toHaveBeenCalled(); + }); + + test("does nothing when this request did not verify anyone", async () => { + // A replay of an already-verified link: Better Auth returns before `afterEmailVerification`, so no + // marker is set and a still-valid intent cookie must not be spent. + mocks.getJustVerifiedUserId.mockReturnValue(undefined); + const ctx = buildCtx(); + + await verificationAutoSignInAfterHandler(ctx); + + expect(sessionOf(ctx)).not.toHaveBeenCalled(); + expect(mocks.classifySignupIntent).not.toHaveBeenCalled(); + // Asserts the marker guard specifically, by pinning that the handler returns before it does any + // work at all. Without this the row passed even with the guard deleted, because an absent session + // and an absent marker both read as `undefined` further down and matched each other. + expect(mocks.getSessionFromCtx).not.toHaveBeenCalled(); + }); + + test("ignores endpoints other than /verify-email", async () => { + const ctx = buildCtx({ path: "/sign-in/email" }); + + await verificationAutoSignInAfterHandler(ctx); + + expect(mocks.getJustVerifiedUserId).not.toHaveBeenCalled(); + expect(sessionOf(ctx)).not.toHaveBeenCalled(); + }); + + test("leaves an existing session for the same user alone", async () => { + // EMAIL_VERIFICATION_DISABLED=1 (the shipped self-host default) lets a user sign in before + // verifying, so verification can arrive already authenticated. Minting again would be pointless; + // signing them out would be a regression. + mocks.getSessionFromCtx.mockResolvedValue({ user: { id: VERIFIED_USER.id } }); + const ctx = buildCtx(); + + await verificationAutoSignInAfterHandler(ctx); + + expect(sessionOf(ctx)).not.toHaveBeenCalled(); + expect(mocks.setSessionCookie).not.toHaveBeenCalled(); + }); + + test("defers when the endpoint already attached a session cookie to the response", async () => { + // Better Auth's `updateTo` email-change branch mints its own session AND fires + // afterEmailVerification. changeEmail is not enabled today, but if it ever is, this hook must not + // stomp that flow's response with the login redirect. + const ctx = buildCtx(); + (ctx as unknown as { context: { responseHeaders: Headers } }).context.responseHeaders.set( + "set-cookie", + "formbricks.session_token=abc; Path=/; HttpOnly" + ); + + await verificationAutoSignInAfterHandler(ctx); + + expect(sessionOf(ctx)).not.toHaveBeenCalled(); + expect(mocks.setSessionCookie).not.toHaveBeenCalled(); + expect(mocks.auditVerificationSessionWithheld).not.toHaveBeenCalled(); + }); + + test("still mints when the session read appends a cookie (not an endpoint grant)", async () => { + // Pins that the guard asks what the ENDPOINT returned and is not perturbed by anything the session + // read adds afterwards. Note this scenario is not reachable in production: getSessionFromCtx appends + // to better-call's `ctx.responseHeaders`, a different Headers instance from the + // `ctx.context.responseHeaders` the guard reads (verified at runtime), and the two are merged only + // after this hook returns. Kept as a structural guard on the ordering, not as a regression test for + // a bug that existed. + const ctx = buildCtx(); + mocks.getSessionFromCtx.mockImplementation(async () => { + (ctx as unknown as { context: { responseHeaders: Headers } }).context.responseHeaders.append( + "set-cookie", + "formbricks.session_token=refreshed; Path=/; HttpOnly" + ); + return { user: { id: "someone_else" } }; + }); + + await verificationAutoSignInAfterHandler(ctx); + + expect(sessionOf(ctx)).toHaveBeenCalledWith(VERIFIED_USER.id, false); + expect(mocks.setSessionCookie).toHaveBeenCalledOnce(); + }); + + test("still mints when a DIFFERENT user is signed in in this browser", async () => { + mocks.getSessionFromCtx.mockResolvedValue({ user: { id: "someone_else" } }); + const ctx = buildCtx(); + + await verificationAutoSignInAfterHandler(ctx); + + expect(sessionOf(ctx)).toHaveBeenCalledWith(VERIFIED_USER.id, false); + }); + + // The stranding case: `emailVerified` is already committed when this hook runs, so a throw here + // would 500 a link that verified the user, and the retry hits Better Auth's already-verified early + // return — which never reaches this hook again. + test("swallows a failure instead of failing the verification request", async () => { + const ctx = buildCtx(); + sessionOf(ctx).mockRejectedValue(new Error("adapter exploded")); + + // The adapter error must NOT escape — it is converted into the login redirect. If it propagated, + // the user would get a 500 on a link that already verified them. + await expect(verificationAutoSignInAfterHandler(ctx)).rejects.toThrow(`REDIRECT:${LOGIN_VERIFIED}`); + expect(mocks.setSessionCookie).not.toHaveBeenCalled(); + // Deliberately NOT audited: this is the catch-all safety net, and an audit write there could throw + // on its own and escape the very guard that exists to stop a 500 on an already-verified link. The + // error log is the record for this path. The refused-mint case below is the one that gets audited. + expect(mocks.auditVerificationSessionWithheld).not.toHaveBeenCalled(); + }); + + test("withholds rather than throwing when the verified user cannot be loaded", async () => { + const ctx = buildCtx(); + ( + ctx as unknown as { context: { internalAdapter: { findUserById: ReturnType } } } + ).context.internalAdapter.findUserById.mockResolvedValue(null); + + await expect(verificationAutoSignInAfterHandler(ctx)).rejects.toThrow(`REDIRECT:${LOGIN_VERIFIED}`); + expect(mocks.setSessionCookie).not.toHaveBeenCalled(); + // The proof was good and we still granted nothing — the one withheld outcome that is our own fault, + // so it must not be the only silent one. + expect(mocks.auditVerificationSessionWithheld).toHaveBeenCalledWith(VERIFIED_USER.id, "grant_failed"); + }); + + test("audits the refused session when the inactive-user gate blocks the mint", async () => { + // `session.create.before` (rejectInactiveUserOnSessionCreate) returns no session for a deactivated + // user rather than throwing, so this arrives as a falsy createSession, not an exception. + const ctx = buildCtx(); + sessionOf(ctx).mockResolvedValue(null); + + await expect(verificationAutoSignInAfterHandler(ctx)).rejects.toThrow(`REDIRECT:${LOGIN_VERIFIED}`); + expect(mocks.setSessionCookie).not.toHaveBeenCalled(); + expect(mocks.auditVerificationSessionWithheld).toHaveBeenCalledWith(VERIFIED_USER.id, "grant_failed"); + }); +}); diff --git a/apps/web/modules/auth/lib/better-auth-verification-autosignin.ts b/apps/web/modules/auth/lib/better-auth-verification-autosignin.ts new file mode 100644 index 000000000000..aabca8d9e728 --- /dev/null +++ b/apps/web/modules/auth/lib/better-auth-verification-autosignin.ts @@ -0,0 +1,173 @@ +import "server-only"; +import { getSessionFromCtx } from "better-auth/api"; +import { setSessionCookie } from "better-auth/cookies"; +import { logger } from "@formbricks/logger"; +import { WEBAPP_URL } from "@/lib/constants"; +import type { AuthHookContext } from "@/modules/ee/sso/lib/better-auth-hooks"; +import { auditVerificationSessionWithheld } from "./better-auth-observability"; +import { getJustVerifiedUserId } from "./email-verification-request-context"; +import { + SIGNUP_INTENT_COOKIE_NAME, + SIGNUP_INTENT_COOKIE_OPTIONS, + type TWithheldReason, + classifySignupIntent, +} from "./signup-intent"; + +/** + * Where a verification lands when no session is granted. + * + * Without this the request would follow its `callbackURL` — `/` for every sign-up link — and the user + * would be silently bounced to the login page by middleware, having just been told their email was + * verified. `?verified=1` is what lets the login form say "verified, now sign in" instead. + */ +const getVerifiedButSignInRequiredUrl = (): string => + new URL("/auth/login?verified=1", WEBAPP_URL).toString(); + +/** + * Mint the post-verification session for the browser that signed up, and spend the intent cookie. + * + * Split out of the handler to keep that function's branching readable; it returns whether a session was + * actually granted, so the caller decides where to send the user without re-deriving it. + */ +const grantSessionToSignupBrowser = async ( + ctx: AuthHookContext, + verifiedUserId: string +): Promise => { + // Same shape as the SSO recovery sign-in plugin. Routing through the adapter rather than writing the + // row directly keeps the `session.create.before` database hook in play, so a deactivated user is still + // refused a session here — and the caller then withholds rather than letting them in. + const user = await ctx.context.internalAdapter.findUserById(verifiedUserId); + const session = user ? await ctx.context.internalAdapter.createSession(user.id, false) : null; + if (!user || !session) return false; + + await setSessionCookie(ctx, { session, user }); + + // Single use: the cookie has done its job. Replay is independently blocked (an already-verified link + // never fires afterEmailVerification, so the marker is absent), making this defence in depth — but it + // must actually land: the production name carries the `__Secure-` prefix, and a browser REJECTS any + // Set-Cookie for such a name without the `Secure` attribute, so clearing with a bare `{ maxAge: 0 }` + // would silently no-op on HTTPS. Full attribute set, zero lifetime. + ctx.setCookie(SIGNUP_INTENT_COOKIE_NAME, "", { ...SIGNUP_INTENT_COOKIE_OPTIONS, maxAge: 0 }); + + return true; +}; + +/** + * ENG-2562 — hand the post-verification session only to the browser that signed up. + * + * `emailVerification.autoSignInAfterVerification` used to be `true`, on the reasoning that "clicking + * the signed link proves email ownership". It does — but it does not prove the clicker chose the + * account's password, and those are different facts. An attacker could register an address that had no + * account yet, with a password of their choosing; Formbricks mailed the victim a verification link; and + * the victim's click signed the VICTIM into an account whose password the ATTACKER still held, in an + * organization the attacker had provisioned (`handlePostUserCreation` runs at sign-up time). That is + * textbook account pre-hijacking. + * + * The flag is now `false`, so Better Auth mints nothing and this hook is the only thing that can. It + * mints only when the request carries a sign-up intent cookie naming the user who was just verified — + * i.e. only when the browser completing the verification is the browser that started the sign-up. + * Everything else gets a verified email and no session, and is sent to the login page. + * + * Deliberately non-destructive: the stored password is never touched. A verifying stranger cannot enter + * the account, and the legitimate owner recovers through the ordinary reset flow, which rotates the + * password and revokes sessions (`revokeSessionsOnPasswordReset`). The accepted residual is that the + * attacker's password survives, so they can sign in until the victim resets — this removes the + * data-comingling harm, not the squatted account. + * + * Notable non-obvious cases: + * + * - **A mail scanner prefetching the link** lands on the withheld path, which is exactly why the + * non-destructive shape was chosen: it costs a session, not somebody's password. + * - **Sign-ups through Better Auth's native `/sign-up/email`** never get an intent cookie, so their + * verification withholds the session too. Fail-closed, and a deliberate behaviour change. + * - **`EMAIL_VERIFICATION_DISABLED=1`** (the shipped self-host default) lets a user sign in before + * verifying, so a verification can arrive on an already-authenticated request. Better Auth used to + * reuse that session; we must not sign such a user out, which is what the early return covers. + */ +export const verificationAutoSignInAfterHandler = async (ctx: AuthHookContext): Promise => { + if (ctx.path !== "/verify-email") return; + + // Set by the composed `afterEmailVerification` hook. Absent unless THIS request verified someone — + // which is the only trustworthy form of that question, because a successful verification carrying a + // callbackURL finishes as a thrown redirect and is therefore an `APIError`, exactly like a failure. + const verifiedUserId = getJustVerifiedUserId(); + if (!verifiedUserId) return; + + // Whether the request finishes without a session, and therefore has to be sent somewhere that says + // so. Set inside the try; acted on OUTSIDE it, because `ctx.redirect` throws and this catch would + // otherwise swallow the redirect — the same trap `better-auth-recovery-signin.ts` documents. + let sessionWithheld = false; + + // Nothing below may throw. Better Auth flips `emailVerified` BEFORE this hook runs, and an uncaught + // throw here propagates out of `runAfterHooks` as a 500 — on a link that already verified the user. + // On retry they hit the already-verified early return, which never reaches this hook again, so a + // single fault would strand them: verified, signed out, and with no route back but finding the login + // page by hand. Any failure therefore degrades to "no session", never to an error. + try { + // Whether the ENDPOINT already attached a session cookie, meaning it made its own sign-in decision + // and this hook must not second-guess it. Unreachable in today's config — the only `/verify-email` + // branch that does this is Better Auth's `updateTo` email-change flow and `user.changeEmail` is not + // enabled — but that branch also fires `afterEmailVerification`, so the guard keeps the hook correct + // if it is ever enabled. Server-controlled header, not spoofable. + // + // Read before `getSessionFromCtx` as belt-and-braces, NOT because a live bug needs it. That helper + // appends any Set-Cookie the session read produced — `getSession` re-issues the cookie once + // `updateAge` elapses — but it appends to better-call's `ctx.responseHeaders`, which is a DIFFERENT + // Headers instance from Better Auth's `ctx.context.responseHeaders` read here (verified at runtime + // in this hook: `ctx.responseHeaders !== ctx.context.responseHeaders`; the two are merged only after + // the hook returns, by `mergeResponseHeaders` in dispatch.mjs). So a session refresh cannot reach + // this value either way. The ordering is kept so the guard's meaning stays unambiguous — it asks + // what the ENDPOINT returned, and nothing else. + const sessionCookieName = ctx.context.authCookies?.sessionToken?.name; + const setCookieBeforeSessionRead = ctx.context.responseHeaders?.get("set-cookie") ?? ""; + const endpointAlreadyGrantedSession = Boolean( + sessionCookieName && setCookieBeforeSessionRead.includes(`${sessionCookieName}=`) + ); + + // Read through Better Auth's own resolver rather than `ctx.context.session`, which this route does + // not populate — `/verify-email` carries no session middleware, which is why the upstream handler + // calls `getSessionFromCtx` here too. `currentSession &&` is load-bearing rather than defensive + // noise: without it an absent session compares `undefined === undefined` against a missing + // verified-user id and reports "already signed in", swallowing the case instead of withholding it. + const currentSession = await getSessionFromCtx(ctx); + if (currentSession && currentSession.user?.id === verifiedUserId) return; + if (endpointAlreadyGrantedSession) return; + + const intent = classifySignupIntent(ctx.getCookie(SIGNUP_INTENT_COOKIE_NAME), verifiedUserId); + + // `withheldReason` doubles as the decision: non-null means no session. Carrying the REASON rather + // than a boolean is what makes this diagnosable — this branch is reached by the ordinary + // cross-device click and by a mail-scanner prefetch (`absent`) just as much as by a genuine + // pre-hijack (`other_user`), and only the latter is worth anyone's attention. Logged AND audited + // because the audit trail is enterprise-gated, so the log line is what a self-hoster gets; both + // carry the user id and the reason only, never the cookie value or the verification token. + let withheldReason: TWithheldReason | null = intent === "valid" ? null : intent; + + if (intent === "valid" && !(await grantSessionToSignupBrowser(ctx, verifiedUserId))) { + // The proof was good but the mint failed — a missing user row or a refused session (the + // `session.create.before` inactive-user gate lands here). Recorded like any other withheld + // outcome: without this the one path that is our own fault would be the only silent one. + withheldReason = "grant_failed"; + } + + if (withheldReason) { + logger.info( + { userId: verifiedUserId, reason: withheldReason }, + "Withheld the post-verification session" + ); + await auditVerificationSessionWithheld(verifiedUserId, withheldReason); + sessionWithheld = true; + } + } catch (error) { + // userId only — never the cookie value or the verification token. + logger.error({ error, userId: verifiedUserId }, "Post-verification auto-sign-in failed"); + sessionWithheld = true; + } + + // Outside the try on purpose: `ctx.redirect` throws, so putting it inside would feed the redirect + // straight into the catch above and land the user on `callbackURL` with no explanation of why they + // are not signed in. + if (sessionWithheld) { + throw ctx.redirect(getVerifiedButSignInRequiredUrl()); + } +}; diff --git a/apps/web/modules/auth/lib/email-verification-request-context.ts b/apps/web/modules/auth/lib/email-verification-request-context.ts new file mode 100644 index 000000000000..9c67680afbbb --- /dev/null +++ b/apps/web/modules/auth/lib/email-verification-request-context.ts @@ -0,0 +1,56 @@ +import "server-only"; +import { AsyncLocalStorage } from "node:async_hooks"; + +/** + * Request scope carrying "this request just verified an email" from Better Auth's + * `afterEmailVerification` hook to the `hooks.after` chain (ENG-2562). + * + * The two halves of the ENG-2562 fix run in different places and neither can see the other directly: + * `afterEmailVerification(user, request)` gets the verified user but no endpoint context, so it cannot + * touch cookies; the `hooks.after` middleware gets the full context but no reliable way to know what + * just happened. This store joins them. + * + * ## Why the response cannot be read instead + * + * Deriving "a verification succeeded" from `ctx.context.returned` looks cheaper and is wrong. A + * successful verification that carries a `callbackURL` — which is every verification reached from a + * sign-up email, since Better Auth defaults it — finishes as `throw ctx.redirect(...)`, and a thrown + * redirect is an `APIError`, structurally identical to the failure case. Worse, `/verify-email` also + * redirects on its already-verified early return, so even "is this a redirect?" does not answer "did + * this request verify someone?". + * + * `afterEmailVerification` answers exactly that and nothing else: Better Auth calls it immediately + * after the `emailVerified` write and returns before it on the already-verified path, so it fires once + * per user, on a genuine first verification only. Taking the signal from the framework rather than + * reverse-engineering its response shape is also what keeps this working across upgrades. + * + * Opened by the `/api/auth/[...all]` route around `auth.handler`, alongside the SSO and observability + * stores. The store survives the awaited handler because the async work starts synchronously inside + * `run()` — the same guarantee `sso-request-context.ts` and `better-auth-request-context.ts` rely on. + */ +interface EmailVerificationRequestStore { + /** Id of the user whose email this request verified. Absent on every other request. */ + verifiedUserId?: string; +} + +const emailVerificationRequestContext = new AsyncLocalStorage(); + +/** Run `fn` in a fresh scope so a verification in it can be observed by the after-hook chain. */ +export const runWithEmailVerificationRequestContext = (fn: () => T): T => + emailVerificationRequestContext.run({}, fn); + +/** Record, within the current scope, that this request verified `userId`'s email. */ +export const markEmailJustVerified = (userId: string): void => { + const store = emailVerificationRequestContext.getStore(); + if (store) store.verifiedUserId = userId; +}; + +/** + * The user this request verified, or `undefined` outside a verification. + * + * `undefined` is the safe answer and the common one: it means the auto-sign-in hook mints nothing. + * It is also what a server-side `auth.api.verifyEmail` call returns, since no store is open there — + * correct, because there is no browser to hand a cookie to. + */ +export const getJustVerifiedUserId = (): string | undefined => + emailVerificationRequestContext.getStore()?.verifiedUserId; diff --git a/apps/web/modules/auth/lib/signup-intent.test.ts b/apps/web/modules/auth/lib/signup-intent.test.ts new file mode 100644 index 000000000000..69d774e578c9 --- /dev/null +++ b/apps/web/modules/auth/lib/signup-intent.test.ts @@ -0,0 +1,150 @@ +import jwt from "jsonwebtoken"; +import { beforeEach, describe, expect, test, vi } from "vitest"; + +const NEXTAUTH_SECRET = "test-nextauth-secret"; +// 32 bytes hex — symmetricEncrypt/symmetricDecrypt expect a 64-char key. +const ENCRYPTION_KEY = "0".repeat(64); + +vi.mock("@/lib/constants", () => ({ NEXTAUTH_SECRET, ENCRYPTION_KEY, BETTER_AUTH_SECRET: undefined })); +vi.mock("@/lib/env", () => ({ env: { WEBAPP_URL: "http://localhost:3000" } })); +// Imported only so the boundary test below can call the REAL `verifyToken`; its gateway-auth import +// chain is irrelevant to that call and pulls in server env, so stub it. +vi.mock("@/modules/gateway-auth/lib/service", () => ({ getGatewayAuthServiceTokenPurpose: vi.fn() })); + +const { createSignupIntentToken, readSignupIntent, classifySignupIntent, SIGNUP_INTENT_COOKIE_NAME } = + await import("./signup-intent"); +const { symmetricEncrypt } = await import("@/lib/crypto"); + +describe("signup intent token", () => { + beforeEach(() => { + vi.useRealTimers(); + }); + + test("round-trips the user id it was issued for", () => { + const token = createSignupIntentToken("user_1"); + + expect(readSignupIntent(token)).toEqual({ userId: "user_1", reason: "valid" }); + }); + + test("does not carry the user id in the clear", () => { + // The cookie is httpOnly, but it is still a value that reaches the client. The id is encrypted in + // the payload rather than merely base64'd with the rest of the JWT. + expect(createSignupIntentToken("user_1")).not.toContain("user_1"); + }); + + test.each([ + ["absent", undefined], + ["empty", ""], + ["not a jwt", "not-a-jwt"], + ])("refuses %s cookie values", (_label, value) => { + expect(readSignupIntent(value).userId).toBeNull(); + }); + + test("refuses a token signed with a different secret", () => { + const forged = jwt.sign( + { id: symmetricEncrypt("user_1", ENCRYPTION_KEY), purpose: "signup_intent" }, + "not-our-secret", + { algorithm: "HS256" } + ); + + expect(readSignupIntent(forged)).toEqual({ userId: null, reason: expect.any(String) }); + }); + + test("refuses an expired token", () => { + const token = createSignupIntentToken("user_1"); + + // The TTL is one hour; jump past it rather than sleeping. + vi.useFakeTimers(); + vi.setSystemTime(Date.now() + 61 * 60 * 1000); + + expect(readSignupIntent(token)).toEqual({ userId: null, reason: expect.any(String) }); + }); + + test("refuses an unsigned (alg: none) token", () => { + // Guard, not a proof of the `algorithms: ["HS256"]` pin: jsonwebtoken already refuses `none` + // whenever a string secret is supplied, so this test stays green with the pin removed (verified by + // mutation). The pin is kept as defence-in-depth against algorithm confusion should this ever be + // handed a key object instead of a secret string — it is simply not what this row binds. + const unsigned = jwt.sign( + { id: symmetricEncrypt("user_1", ENCRYPTION_KEY), purpose: "signup_intent" }, + "", + { algorithm: "none" } + ); + + expect(readSignupIntent(unsigned)).toEqual({ userId: null, reason: expect.any(String) }); + }); + + // A token minted by any other flow, even correctly signed with the same secret, must not be + // spendable here. `createToken` in lib/jwt.ts uses `id`/`purpose`, so such a token carries no + // `kind` at all and is refused — which is the point of not sharing that claim shape. + test.each([["email_verification"], ["sso_recovery"], [undefined]])( + "refuses a correctly-signed lib/jwt.ts-shaped token whose purpose is %s", + (purpose) => { + const otherFlowToken = jwt.sign( + { id: symmetricEncrypt("user_1", ENCRYPTION_KEY), purpose }, + NEXTAUTH_SECRET, + { algorithm: "HS256" } + ); + + expect(readSignupIntent(otherFlowToken)).toEqual({ userId: null, reason: "invalid" }); + } + ); + + // Binds the `kind` check itself. The lib/jwt.ts-shaped rows above do not: those tokens carry no `uid`, + // so they die on the uid check and stay refused even with the kind check deleted (verified by + // mutation). Nothing mints a `uid`-carrying token today, which is exactly why this row exists — the + // check is what keeps that true if something ever does. + test.each([["other_kind"], [undefined], [""]])("refuses a uid-carrying token whose kind is %s", (kind) => { + const wrongKind = jwt.sign({ uid: symmetricEncrypt("user_1", ENCRYPTION_KEY), kind }, NEXTAUTH_SECRET, { + algorithm: "HS256", + }); + + expect(readSignupIntent(wrongKind)).toEqual({ userId: null, reason: "invalid" }); + }); + + // The other direction, and the reason the claims are named `uid`/`kind`: `getVerificationTokenPurpose` + // in lib/jwt.ts FAILS OPEN, rewriting an unrecognised purpose to "email_verification". Staying out of + // VERIFICATION_TOKEN_PURPOSES is therefore not protection by itself — carrying no `id` claim is, since + // `verifyToken` bails on `if (!payload?.id)` before any purpose is considered. + test("is not parseable as a lib/jwt.ts verification token", async () => { + const token = createSignupIntentToken("user_1"); + + // The claim-shape half: no `id`, no `purpose`. + const decoded = jwt.decode(token) as Record; + expect(decoded.id).toBeUndefined(); + expect(decoded.purpose).toBeUndefined(); + expect(decoded.kind).toBe("signup_intent"); + + // And the boundary itself: the REAL verifier refuses the token. The signature is genuinely valid + // to it (same secret on a NEXTAUTH_SECRET-only deployment), so what this binds is the + // `if (!payload?.id)` bail-out — the layout assertions above would keep passing if `verifyToken` + // ever started accepting `uid`/`kind`; this call would not. + const { verifyToken } = await import("@/lib/jwt"); + await expect(verifyToken(token)).rejects.toThrow("Invalid token"); + }); + + test("cookie name is namespaced so it cannot collide with a Better Auth cookie", () => { + expect(SIGNUP_INTENT_COOKIE_NAME).toBe("formbricks.signup_intent"); + }); +}); + +describe("classifySignupIntent", () => { + test("accepts a cookie issued for the user being verified", () => { + expect(classifySignupIntent(createSignupIntentToken("user_1"), "user_1")).toBe("valid"); + }); + + // The pre-hijacking case with a stale cookie in the mix. Distinguishing this from `absent` is the + // whole reason the reader returns a reason: `absent` is the ordinary cross-device click, while a + // valid cookie naming a different account is the one worth looking at in the audit log. + test("reports other_user for a valid cookie naming a different account", () => { + expect(classifySignupIntent(createSignupIntentToken("user_1"), "user_2")).toBe("other_user"); + }); + + test("reports absent when there is no cookie", () => { + expect(classifySignupIntent(undefined, "user_1")).toBe("absent"); + }); + + test("reports invalid for a cookie that does not verify", () => { + expect(classifySignupIntent("not-a-jwt", "user_1")).toBe("invalid"); + }); +}); diff --git a/apps/web/modules/auth/lib/signup-intent.ts b/apps/web/modules/auth/lib/signup-intent.ts new file mode 100644 index 000000000000..d4f8e72c84fc --- /dev/null +++ b/apps/web/modules/auth/lib/signup-intent.ts @@ -0,0 +1,156 @@ +import "server-only"; +import jwt from "jsonwebtoken"; +import { BETTER_AUTH_SECRET, ENCRYPTION_KEY, NEXTAUTH_SECRET } from "@/lib/constants"; +import { symmetricDecrypt, symmetricEncrypt } from "@/lib/crypto"; +import { EMAIL_VERIFICATION_TTL_SECONDS, USE_SECURE_COOKIES } from "./auth-cookies"; + +/** + * Sign-up intent cookie (ENG-2562) — evidence that THIS browser is the one that registered an account. + * + * Clicking a verification link proves the clicker controls the mailbox. It does NOT prove the clicker + * chose the account's password, and `autoSignInAfterVerification` used to treat the two as the same + * thing: an attacker could register a victim's address with a password of the attacker's choosing, and + * the victim's click would sign the victim into the attacker's account. This cookie is the missing + * discriminator — set when a sign-up creates an account, checked when a verification completes — so the + * session is only ever handed to the browser that actually signed up. + * + * It is not a credential and grants nothing on its own: without the emailed verification token there is + * no request it can influence, and the attacker's copy (of an account they created) is inert because the + * token goes to the victim's inbox. + * + * ## Why this does not use `verifyToken` from `lib/jwt.ts` + * + * Two reasons, both found in the security review of the ENG-2562 plan: + * + * 1. `getVerificationTokenPurpose` there **fails open** — an absent or unrecognised `purpose` claim is + * silently rewritten to `"email_verification"` and returned as if it had been asserted. Staying out of + * `VERIFICATION_TOKEN_PURPOSES` is therefore NOT protection on its own; it is precisely what would make + * `verifyToken` rewrite one of these tokens into a purpose it was never issued for. What actually keeps + * the two keyspaces apart is the claim names used below — `uid`/`kind` rather than `id`/`purpose` — so a + * token minted here cannot be parsed by `verifyToken` at all: it bails on `if (!payload?.id)` before any + * purpose is considered. Not exploitable either way today, since that helper's only caller demands + * `sso_recovery` — but a future consumer trusting a returned `email_verification` would otherwise have + * accepted this cookie, because both sign with the same secret on a deployment that sets only + * `NEXTAUTH_SECRET`. + * 2. `verifyToken` falls back, on signature failure, to looking the user up by token and re-verifying + * with `NEXTAUTH_SECRET + userEmail`. That would put one or two database queries behind every + * malformed cookie on an unauthenticated GET — including every mail-scanner prefetch of a + * verification link — which is the exact amplification the new rate limits exist to prevent. + * + * So: HS256 pinned, strict `kind` equality, expiry enforced by `jwt.verify`, no legacy fallback, and no + * database access. The user id is compared against the already-loaded verified user by the caller. + */ + +const SIGNUP_INTENT_KIND = "signup_intent"; + +// Same chain as auth.ts's Better Auth `secret`: NEXTAUTH_SECRET is `optional()` in the env schema and +// a deployment may run on BETTER_AUTH_SECRET alone, so pinning this token to NEXTAUTH_SECRET would +// make sign-up throw on exactly the configuration auth.ts goes out of its way to support. +const SIGNING_SECRET = BETTER_AUTH_SECRET ?? NEXTAUTH_SECRET; + +/** + * Better Auth is configured with `cookiePrefix: "formbricks"` and adds `__Secure-` under + * `useSecureCookies`, so this mirrors both. The `signup_intent` suffix is not a name Better Auth mints, + * so the two namespaces cannot collide. + */ +export const SIGNUP_INTENT_COOKIE_NAME = `${USE_SECURE_COOKIES ? "__Secure-" : ""}formbricks.signup_intent`; + +/** Cookie attributes, matching Better Auth's `advanced.defaultCookieAttributes`, with one exception. */ +export const SIGNUP_INTENT_COOKIE_OPTIONS = { + httpOnly: true, + secure: USE_SECURE_COOKIES, + path: "/", + // `lax`, not `strict`: the verification link is a top-level GET navigation arriving from a mail + // client, i.e. cross-site. `strict` would withhold the cookie on exactly the request that needs it, + // turning every legitimate same-browser sign-up into a withheld session. + sameSite: "lax", + // Paired with the verification token's TTL — but the pairing only holds for the CURRENT link. A + // resent link gets a fresh hour, so the resend action re-issues this cookie alongside it when (and + // only when) the requesting browser already holds a valid one for the same user + // (verification-requested/actions.ts). A browser whose cookie has already expired cannot be told + // apart from one that never signed up, so a resend after expiry stays on the withheld path. + maxAge: EMAIL_VERIFICATION_TTL_SECONDS, +} as const; + +/** + * Mint the cookie value for `userId`. Never log the result: it is a bearer value, and the precedent in + * `auth.ts`'s `sendVerificationEmail` is domain-only logging — never the address, the token, or the URL. + */ +export const createSignupIntentToken = (userId: string): string => { + if (!SIGNING_SECRET) { + throw new Error("Neither BETTER_AUTH_SECRET nor NEXTAUTH_SECRET is set"); + } + if (!ENCRYPTION_KEY) { + throw new Error("ENCRYPTION_KEY is not set"); + } + + // `uid`/`kind`, not `id`/`purpose` — see the header. Sharing `createToken`'s claim shape is what would + // let `verifyToken` resolve this cookie as an `email_verification` token. + return jwt.sign( + { uid: symmetricEncrypt(userId, ENCRYPTION_KEY), kind: SIGNUP_INTENT_KIND }, + SIGNING_SECRET, + { algorithm: "HS256", expiresIn: EMAIL_VERIFICATION_TTL_SECONDS } + ); +}; + +/** + * What a sign-up intent cookie says, and — when it says nothing usable — why. + * + * The reason is not decoration. The withheld path is reached by the ordinary cross-device click and by a + * mail-scanner prefetch as well as by a genuine pre-hijack, so collapsing all of them into one boolean + * throws away the only distinction the record exists to make: `absent` is the common, boring case, while + * `invalid` and `other_user` are the ones worth looking at. It cannot be recovered after the fact. + */ +export type TSignupIntentRead = + | { userId: string; reason: "valid" } + | { userId: null; reason: "absent" | "invalid" }; + +/** + * Read the cookie. `absent` is no cookie at all (or no secrets configured); `invalid` covers malformed, + * expired, wrong signature, wrong algorithm, and the wrong `kind`. + * + * Returns rather than throws, and never logs the token, because the caller runs inside a verification + * request that must not fail on a bad cookie: a garbage cookie has to degrade to "no proof", which + * withholds the session, not to a 500 on a link that already verified the user. + */ +export const readSignupIntent = (cookieValue: string | null | undefined): TSignupIntentRead => { + if (!cookieValue || !SIGNING_SECRET || !ENCRYPTION_KEY) return { userId: null, reason: "absent" }; + + try { + // `algorithms` pinned so a token cannot dictate its own verification algorithm, and `expiresIn` + // above means `jwt.verify` rejects a stale cookie for us. + const payload = jwt.verify(cookieValue, SIGNING_SECRET, { algorithms: ["HS256"] }); + if (typeof payload !== "object" || payload === null) return { userId: null, reason: "invalid" }; + + const { uid, kind } = payload as { uid?: unknown; kind?: unknown }; + // Strict equality, no defaulting. This is the check that keeps a token minted for another flow + // from being spent here, on top of the claim names that stop it parsing as one at all. + if (kind !== SIGNUP_INTENT_KIND) return { userId: null, reason: "invalid" }; + if (typeof uid !== "string" || uid.length === 0) return { userId: null, reason: "invalid" }; + + const userId = symmetricDecrypt(uid, ENCRYPTION_KEY); + + return userId.length > 0 ? { userId, reason: "valid" } : { userId: null, reason: "invalid" }; + } catch { + return { userId: null, reason: "invalid" }; + } +}; + +/** + * Why the post-verification session was withheld, for the log line and the audit row. + * + * `other_user` is the genuinely suspicious one — a valid cookie for a different account. `grant_failed` + * is an internal fault, not a judgement about the caller. + */ +export type TWithheldReason = "absent" | "invalid" | "other_user" | "grant_failed"; + +/** Classify the cookie against the user this request just verified. */ +export const classifySignupIntent = ( + cookieValue: string | null | undefined, + verifiedUserId: string +): "valid" | Exclude => { + const intent = readSignupIntent(cookieValue); + if (intent.reason !== "valid") return intent.reason; + + return intent.userId === verifiedUserId ? "valid" : "other_user"; +}; diff --git a/apps/web/modules/auth/login/components/login-form.tsx b/apps/web/modules/auth/login/components/login-form.tsx index 1d6bb40366bc..10d489a459f8 100644 --- a/apps/web/modules/auth/login/components/login-form.tsx +++ b/apps/web/modules/auth/login/components/login-form.tsx @@ -49,6 +49,7 @@ interface LoginFormProps { isSsoEnabled: boolean; samlSsoEnabled: boolean; oauthError?: string; + emailJustVerified?: boolean; prefilledEmail?: string; inviteToken?: string | null; resolvedCallbackPath: string; @@ -68,6 +69,7 @@ export const LoginForm = ({ isSsoEnabled, samlSsoEnabled, oauthError, + emailJustVerified, prefilledEmail, inviteToken, resolvedCallbackPath, @@ -192,6 +194,14 @@ export const LoginForm = ({

{formLabel}

+ {emailJustVerified && ( + + {t("auth.login.email_verified_sign_in_title")} + +

{t("auth.login.email_verified_sign_in_description")}

+ + + )} {oauthAccountNotLinked && ( {t("auth.login.oauth_account_not_linked_title")} diff --git a/apps/web/modules/auth/login/page.tsx b/apps/web/modules/auth/login/page.tsx index f1696baf756c..95f739a4e594 100644 --- a/apps/web/modules/auth/login/page.tsx +++ b/apps/web/modules/auth/login/page.tsx @@ -68,6 +68,10 @@ export const LoginPage = async ({ isSsoEnabled={isSsoEnabled} samlSsoEnabled={samlSsoEnabled} oauthError={oauthError} + // ENG-2562: set when a verification succeeded but the session was withheld, because the + // browser presenting the link was not the one that signed up. Without it the user is bounced + // here with no explanation right after being told their address was verified. + emailJustVerified={getSearchParamString(searchParams.verified) === "1"} prefilledEmail={getSearchParamString(searchParams.email)} inviteToken={inviteToken} resolvedCallbackPath={resolvedCallbackPath} diff --git a/apps/web/modules/auth/signup/actions.test.ts b/apps/web/modules/auth/signup/actions.test.ts index 3a48eceade59..4db274c4a4bf 100644 --- a/apps/web/modules/auth/signup/actions.test.ts +++ b/apps/web/modules/auth/signup/actions.test.ts @@ -12,6 +12,7 @@ import { capturePostHogEvent } from "@/lib/posthog"; import { getUserByEmail } from "@/lib/user/service"; import { AuditLoggingCtx } from "@/lib/utils/action-client/types/context"; import { auth } from "@/modules/auth/lib/auth"; +import { readSignupIntent } from "@/modules/auth/lib/signup-intent"; import { updateUser } from "@/modules/auth/lib/user"; import { getInvite, resolveInviteMatch } from "@/modules/auth/signup/lib/invite"; import { applyIPRateLimit } from "@/modules/core/rate-limit/helpers"; @@ -26,9 +27,15 @@ vi.mock("next/headers", () => ({ })); const requestHeaders = new Headers({ "x-formbricks-client-ip": "203.0.113.7" }); +/** Captures cookies the action sets, so the ENG-2562 sign-up intent cookie can be asserted on. */ +let setCookies: { name: string; value: string }[] = []; const mockNextRequestData = () => { + setCookies = []; vi.mocked(headers).mockResolvedValue(requestHeaders as never); - vi.mocked(cookies).mockResolvedValue({ get: () => undefined } as never); + vi.mocked(cookies).mockResolvedValue({ + get: () => undefined, + set: (name: string, value: string) => setCookies.push({ name, value }), + } as never); }; vi.mock("@formbricks/logger", () => ({ @@ -86,6 +93,11 @@ vi.mock("@/lib/constants", () => ({ WEBAPP_URL: "http://localhost:3000", IS_TURNSTILE_CONFIGURED: false, TURNSTILE_SECRET_KEY: undefined, + // Reached through the ENG-2562 sign-up intent cookie (signup-intent.ts → lib/crypto), which reads + // both at module load. A 64-char hex key so `symmetricEncrypt` takes its normal path. + ENCRYPTION_KEY: "0".repeat(64), + NEXTAUTH_SECRET: "test-nextauth-secret", + BETTER_AUTH_SECRET: undefined, get IS_FORMBRICKS_CLOUD() { return constantsOverrides.IS_FORMBRICKS_CLOUD; }, @@ -265,6 +277,23 @@ describe("createUserAction — signup verification email callbackURL", () => { // say so explicitly or a false creation record is written. expect(ctx.auditLoggingCtx.userId).toBe(""); expect(ctx.auditLoggingCtx.suppressEvent).toBe(true); + // ENG-2562: and no sign-up intent cookie. The caller is an unauthenticated stranger who has proven + // nothing about an account that already exists; arming one would hand them the auto-sign-in on the + // victim's eventual verification click — the exact pre-hijack this fix withholds — and would leak + // that the address is registered. + expect(setCookies.map((c) => c.name)).not.toContain("formbricks.signup_intent"); + }); + + // ENG-2562: the other half of the same rule — a real creation DOES get the cookie, because that is + // what preserves the ENG-1746 land-in-the-app UX for the browser that actually signed up. + test("issues a sign-up intent cookie bound to the created user", async () => { + const ctx = newCtx(); + await createUserAction({ ctx, parsedInput: baseInput } as never); + + const intent = setCookies.find((c) => c.name === "formbricks.signup_intent"); + expect(intent).toBeDefined(); + // Bound to this account and not readable as plaintext: assert through the reader, not the shape. + expect(readSignupIntent(intent?.value)).toEqual({ userId: createdUser.id, reason: "valid" }); }); // Regression: signup/page.tsx requires a valid invite once public sign-up is closed, but the action diff --git a/apps/web/modules/auth/signup/actions.ts b/apps/web/modules/auth/signup/actions.ts index abe84984bd74..627143b91def 100644 --- a/apps/web/modules/auth/signup/actions.ts +++ b/apps/web/modules/auth/signup/actions.ts @@ -30,6 +30,11 @@ import { ATTRIBUTION_COOKIE_NAME, getAttributionPropertiesFromCookies } from "@/ import { auth } from "@/modules/auth/lib/auth"; import { isPasswordCompromisedError } from "@/modules/auth/lib/better-auth-hibp"; import { isSignupEmailDomainBlocked } from "@/modules/auth/lib/signup-email-domain"; +import { + SIGNUP_INTENT_COOKIE_NAME, + SIGNUP_INTENT_COOKIE_OPTIONS, + createSignupIntentToken, +} from "@/modules/auth/lib/signup-intent"; import { isUninvitedSignupAllowed } from "@/modules/auth/lib/signup-policy"; import { markSignupDomainAllowed, @@ -406,6 +411,34 @@ export const createUserAction = actionClient.inputSchema(ZCreateUserAction).acti // so the caller has proven nothing about an account that already exists (ENG-2091). if (outcome.status === "created") { const { user } = outcome; + + // ENG-2562: remember, in THIS browser, that it is the one that created this account, so the + // verification click can be told apart from a stranger's and only this browser is auto-signed-in. + // + // Issued on the `created` path only, and that exclusion is the security-relevant half: on + // `already_existed` the caller is an unauthenticated stranger who has proven nothing about an + // account that already exists, so arming a cookie for it would hand them the very auto-sign-in + // this fix exists to withhold — and would leak that the address is registered. + // + // Before the other side effects: those can fail, and a sign-up that created the account but lost + // its intent cookie would silently degrade to "verify, then log in". + // + // Non-fatal for the same reason: the cookie buys UX (auto-sign-in after verification), so a + // failure to mint it must cost exactly that UX — never the sign-up itself, which has already + // created the account. Concretely reachable when neither secret is set. + try { + (await cookies()).set( + SIGNUP_INTENT_COOKIE_NAME, + createSignupIntentToken(user.id), + SIGNUP_INTENT_COOKIE_OPTIONS + ); + } catch (error) { + logger.error( + { error, userId: user.id }, + "Failed to issue the sign-up intent cookie; verification will require a manual sign-in" + ); + } + await handlePostUserCreation(ctx, outcome, inviteToken); await subscribeUserToMailingList({ diff --git a/apps/web/modules/auth/verification-requested/actions.test.ts b/apps/web/modules/auth/verification-requested/actions.test.ts index 06d9a9716f97..37497afd3cfb 100644 --- a/apps/web/modules/auth/verification-requested/actions.test.ts +++ b/apps/web/modules/auth/verification-requested/actions.test.ts @@ -36,8 +36,28 @@ vi.mock("@/modules/auth/lib/auth", () => ({ auth: { api: { sendVerificationEmail: vi.fn() } }, })); +const cookieMocks = vi.hoisted(() => ({ + get: vi.fn(), + set: vi.fn(), +})); + vi.mock("next/headers", () => ({ headers: vi.fn(() => Promise.resolve(new Headers())), + cookies: vi.fn(() => Promise.resolve(cookieMocks)), +})); + +// The intent-cookie refresh (ENG-2562): classify/mint are unit-tested in signup-intent.test.ts; here +// only the action's decision — refresh iff the browser already proves this user — is under test. +const signupIntentMocks = vi.hoisted(() => ({ + classifySignupIntent: vi.fn(), + createSignupIntentToken: vi.fn(), +})); + +vi.mock("@/modules/auth/lib/signup-intent", () => ({ + SIGNUP_INTENT_COOKIE_NAME: "formbricks.signup_intent", + SIGNUP_INTENT_COOKIE_OPTIONS: { httpOnly: true, secure: false, path: "/", sameSite: "lax", maxAge: 3600 }, + classifySignupIntent: signupIntentMocks.classifySignupIntent, + createSignupIntentToken: signupIntentMocks.createSignupIntentToken, })); vi.mock("@/lib/jwt", () => ({ @@ -93,6 +113,9 @@ describe("resendVerificationEmailAction", () => { vi.mocked(verifySsoRelinkIntent).mockImplementation(() => { throw new Error("invalid"); }); + cookieMocks.get.mockReturnValue(undefined); + signupIntentMocks.classifySignupIntent.mockReturnValue("absent"); + signupIntentMocks.createSignupIntentToken.mockReturnValue("fresh-intent-token"); }); afterEach(() => { @@ -316,6 +339,56 @@ describe("resendVerificationEmailAction", () => { expect(result).toEqual({ success: true }); }); + // ENG-2562: a resent link gets a fresh hour while the intent cookie's clock started at sign-up, so + // the action re-pairs the cookie with the new link — but only for a browser that already holds a + // valid cookie naming this user. Anything weaker would let an unauthenticated caller arm sign-up + // proof for an arbitrary account by asking for a resend. + describe("Sign-up intent cookie refresh (ENG-2562)", () => { + beforeEach(() => { + vi.mocked(applyIPRateLimit).mockResolvedValue({ allowed: true } as never); + vi.mocked(getUserByEmail).mockResolvedValue(mockUser as never); + cookieMocks.get.mockReturnValue({ value: "existing-intent-cookie" }); + }); + + test("re-issues the cookie when the browser already holds a valid one for this user", async () => { + signupIntentMocks.classifySignupIntent.mockReturnValue("valid"); + + await resendVerificationEmailAction({ ctx: mockCtx, parsedInput: validInput } as never); + + expect(signupIntentMocks.classifySignupIntent).toHaveBeenCalledWith( + "existing-intent-cookie", + mockUser.id + ); + expect(signupIntentMocks.createSignupIntentToken).toHaveBeenCalledWith(mockUser.id); + expect(cookieMocks.set).toHaveBeenCalledWith( + "formbricks.signup_intent", + "fresh-intent-token", + expect.objectContaining({ httpOnly: true, maxAge: 3600 }) + ); + }); + + test.each([["absent"], ["invalid"], ["other_user"]])( + "does not mint a cookie when the browser's proof classifies as %s", + async (classification) => { + signupIntentMocks.classifySignupIntent.mockReturnValue(classification as never); + + await resendVerificationEmailAction({ ctx: mockCtx, parsedInput: validInput } as never); + + expect(signupIntentMocks.createSignupIntentToken).not.toHaveBeenCalled(); + expect(cookieMocks.set).not.toHaveBeenCalled(); + } + ); + + test("does not touch the cookie when the user is already verified (no email sent)", async () => { + vi.mocked(getUserByEmail).mockResolvedValue(mockVerifiedUser as never); + signupIntentMocks.classifySignupIntent.mockReturnValue("valid"); + + await resendVerificationEmailAction({ ctx: mockCtx, parsedInput: validInput } as never); + + expect(cookieMocks.set).not.toHaveBeenCalled(); + }); + }); + test("should throw ResourceNotFoundError when user doesn't exist", async () => { vi.mocked(applyIPRateLimit).mockResolvedValue({ allowed: true }); vi.mocked(getUserByEmail).mockResolvedValue(null); diff --git a/apps/web/modules/auth/verification-requested/actions.ts b/apps/web/modules/auth/verification-requested/actions.ts index b61cca9f8818..b70f66a75ca1 100644 --- a/apps/web/modules/auth/verification-requested/actions.ts +++ b/apps/web/modules/auth/verification-requested/actions.ts @@ -1,7 +1,8 @@ "use server"; -import { headers } from "next/headers"; +import { cookies, headers } from "next/headers"; import { z } from "zod"; +import { logger } from "@formbricks/logger"; import { ResourceNotFoundError } from "@formbricks/types/errors"; import { ZUserEmail } from "@formbricks/types/user"; import { WEBAPP_URL } from "@/lib/constants"; @@ -9,6 +10,12 @@ import { verifySsoRelinkIntent } from "@/lib/jwt"; import { actionClient } from "@/lib/utils/action-client"; import { getValidatedCallbackUrl } from "@/lib/utils/url"; import { auth } from "@/modules/auth/lib/auth"; +import { + SIGNUP_INTENT_COOKIE_NAME, + SIGNUP_INTENT_COOKIE_OPTIONS, + classifySignupIntent, + createSignupIntentToken, +} from "@/modules/auth/lib/signup-intent"; import { getUserByEmail } from "@/modules/auth/lib/user"; import { TVerificationRequestPurpose } from "@/modules/auth/lib/verification-links"; import { applyIPRateLimit } from "@/modules/core/rate-limit/helpers"; @@ -90,6 +97,28 @@ export const resendVerificationEmailAction = actionClient.inputSchema(ZResendVer body: { email: user.email, callbackURL: validatedCallbackUrl }, headers: await headers(), }); + + // ENG-2562: re-pair the sign-up intent cookie with the link just minted. The cookie's clock + // starts at sign-up while every resent link gets a fresh hour, so without this a resend's link + // outlives the cookie and the sign-up browser itself lands on the withheld path. Strictly a + // refresh — the browser must already hold a valid cookie naming THIS user — so it extends + // evidence the browser has, and never arms one that lacks it: an unauthenticated caller must not + // be able to mint sign-up proof for an arbitrary account by asking for a resend. A browser whose + // cookie has already expired therefore stays on the withheld path (documented residual). + // Non-fatal, like the sign-up issuance: the email above has already gone out, so a failure here + // must cost exactly the auto-sign-in UX, never the resend itself. + try { + const cookieStore = await cookies(); + if (classifySignupIntent(cookieStore.get(SIGNUP_INTENT_COOKIE_NAME)?.value, user.id) === "valid") { + cookieStore.set( + SIGNUP_INTENT_COOKIE_NAME, + createSignupIntentToken(user.id), + SIGNUP_INTENT_COOKIE_OPTIONS + ); + } + } catch (error) { + logger.error({ error, userId: user.id }, "Failed to refresh the sign-up intent cookie on resend"); + } } return { success: true,