From 341d61131aae685e6b945f814c781aacd4f6fc7a Mon Sep 17 00:00:00 2001 From: Mattsson <111893710+mattssonn@users.noreply.github.com> Date: Sun, 30 Aug 2026 20:21:16 +0200 Subject: [PATCH] fix(auth): email-change recovery re-send and confirmation feedback (#2034) * fix(auth): email-change recovery re-send and confirmation feedback A half-completed secure email change was a dead end: the pending-address short-circuit in /api/account/email swallowed every retry without re-sending mails, so once the confirmation links expired the user could never recover, and confirmation clicks landed on the dashboard with no feedback at all. - /api/account/email: only short-circuit a repeat request while the pending mails are fresh (30 min); a stale pending change falls through to GoTrue, which restarts the change and re-sends both mails - /auth/callback: type=email_change now redirects to a status page (/auth/email-change) that says whether one click remains, the change is complete, or the link was dead, instead of landing silently - auth mail templates: both email-change mails explain that two mails are sent and both links must be clicked - settings: the save button re-enables for the pending address as Skicka igen, so users can trigger the re-send themselves Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018sbGMZQE5W7KfSVFjK7E4p * fix(auth): exempt email-change confirmations from the authenticated /auth bounce (skeptic findings) - middleware: let /auth/email-change and /auth/callback?type=email_change through for authenticated users; the bounce to / swallowed confirmation clicks before verifyOtp ran (pre-existing since #2017) - email-change done page resolves the WL-14 landing destination for the CTA - /api/account/email returns resent flag; settings toast says mails were already sent instead of claiming a fresh send Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018sbGMZQE5W7KfSVFjK7E4p --------- Co-authored-by: Claude Fable 5 --- .../__tests__/email-change-status.test.ts | 99 +++++++++++++++++++ app/(auth)/auth/callback/route.ts | 23 ++++- app/(auth)/auth/email-change/page.tsx | 97 ++++++++++++++++++ app/api/account/email/__tests__/route.test.ts | 44 ++++++++- app/api/account/email/route.ts | 31 ++++-- .../sections/AccountSettingsContent.tsx | 15 ++- lib/email/__tests__/auth-templates.test.ts | 12 +++ lib/email/auth-templates.ts | 4 +- lib/supabase/__tests__/middleware.test.ts | 22 +++++ lib/supabase/middleware.ts | 15 +++ messages/en.json | 4 +- messages/sv.json | 4 +- 12 files changed, 354 insertions(+), 16 deletions(-) create mode 100644 app/(auth)/auth/callback/__tests__/email-change-status.test.ts create mode 100644 app/(auth)/auth/email-change/page.tsx diff --git a/app/(auth)/auth/callback/__tests__/email-change-status.test.ts b/app/(auth)/auth/callback/__tests__/email-change-status.test.ts new file mode 100644 index 00000000..c051bd07 --- /dev/null +++ b/app/(auth)/auth/callback/__tests__/email-change-status.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { NextRequest } from 'next/server' + +// The email_change branch returns before any of the invite/team/landing +// machinery runs; these mocks only keep the module importable in the test +// environment. +vi.mock('@/lib/auth/invite-tokens', () => ({ hashInviteToken: vi.fn() })) +vi.mock('@/lib/auth/consume-invite-cookie', () => ({ + INVITE_COOKIE_NAME: 'gnubok-invite-token', +})) +vi.mock('@/lib/company/landing-server', () => ({ + resolveLandingDestination: vi.fn().mockResolvedValue('/'), +})) +vi.mock('@/lib/company/pending-invites', () => ({ + acceptPendingTeamInviteByToken: vi.fn(), +})) + +const verifyOtp = vi.fn() +vi.mock('@supabase/ssr', () => ({ + createServerClient: () => ({ + auth: { + verifyOtp: (...args: unknown[]) => verifyOtp(...args), + exchangeCodeForSession: vi.fn(), + getUser: vi.fn().mockResolvedValue({ data: { user: null } }), + }, + }), +})) + +import { GET } from '../route' + +function makeRequest(params: Record) { + const url = new URL('https://app.testbrand.example/auth/callback') + for (const [key, value] of Object.entries(params)) { + url.searchParams.set(key, value) + } + return new NextRequest(url) +} + +beforeEach(() => { + vi.clearAllMocks() +}) + +describe('GET /auth/callback type=email_change', () => { + it('redirects a first confirmation (no session) to the partial status page', async () => { + verifyOtp.mockResolvedValue({ + data: { user: null, session: null }, + error: null, + }) + + const res = await GET(makeRequest({ token_hash: 'th', type: 'email_change' })) + + expect(res.headers.get('location')).toBe( + 'https://app.testbrand.example/auth/email-change?status=partial', + ) + expect(verifyOtp).toHaveBeenCalledWith({ + token_hash: 'th', + type: 'email_change', + }) + }) + + it('redirects the completing confirmation (session minted) to the done status page', async () => { + verifyOtp.mockResolvedValue({ + data: { user: { id: 'u1' }, session: { access_token: 'at' } }, + error: null, + }) + + const res = await GET(makeRequest({ token_hash: 'th', type: 'email_change' })) + + expect(res.headers.get('location')).toBe( + 'https://app.testbrand.example/auth/email-change?status=done', + ) + }) + + it('redirects an invalid or expired link to the failed status page', async () => { + verifyOtp.mockResolvedValue({ + data: { user: null, session: null }, + error: { message: 'Email link is invalid or has expired' }, + }) + + const res = await GET(makeRequest({ token_hash: 'th', type: 'email_change' })) + + expect(res.headers.get('location')).toBe( + 'https://app.testbrand.example/auth/email-change?status=failed', + ) + }) + + it('keeps the generic login-error redirect for other failed types', async () => { + verifyOtp.mockResolvedValue({ + data: { user: null, session: null }, + error: { message: 'Email link is invalid or has expired' }, + }) + + const res = await GET(makeRequest({ token_hash: 'th', type: 'signup' })) + + const location = res.headers.get('location') ?? '' + expect(location).toContain('/login?error=auth_error') + expect(location).not.toContain('/auth/email-change') + }) +}) diff --git a/app/(auth)/auth/callback/route.ts b/app/(auth)/auth/callback/route.ts index 71aaf419..e2348fa1 100644 --- a/app/(auth)/auth/callback/route.ts +++ b/app/(auth)/auth/callback/route.ts @@ -63,10 +63,31 @@ export async function GET(request: NextRequest) { } // Handle token hash flow (email verification / magic link) else if (token_hash && type) { - const { error } = await supabase.auth.verifyOtp({ + const { data, error } = await supabase.auth.verifyOtp({ token_hash, type: type as 'signup' | 'invite' | 'magiclink' | 'recovery' | 'email_change' | 'email', }) + + // Email change never lands silently: with secure email change enabled the + // user must confirm from BOTH addresses, and dropping them on the + // dashboard (or login) with no message is exactly how a half-completed + // change reads as "det funkar inte". The status page tells them whether + // one click remains, the change is complete, or the link was dead. + // Completing the second confirmation returns a session; the first (or a + // click from a logged-out mailbox) does not, which is what separates + // 'done' from 'partial'. Cookies are forwarded so a minted session + // survives the redirect. + if (type === 'email_change') { + const status = error ? 'failed' : data?.session ? 'done' : 'partial' + const response = NextResponse.redirect( + new URL(`/auth/email-change?status=${status}`, origin), + ) + for (const { name, value, options } of pendingCookies) { + response.cookies.set({ name, value, ...options }) + } + return response + } + authenticated = !error } diff --git a/app/(auth)/auth/email-change/page.tsx b/app/(auth)/auth/email-change/page.tsx new file mode 100644 index 00000000..02f2d8b9 --- /dev/null +++ b/app/(auth)/auth/email-change/page.tsx @@ -0,0 +1,97 @@ +import Link from 'next/link' +import { headers } from 'next/headers' +import { MailCheck, MailWarning, Mails } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { createClient } from '@/lib/supabase/server' +import { resolveLandingDestination } from '@/lib/company/landing-server' + +/** + * Landing page for email-change confirmation clicks (/auth/callback redirects + * here for type=email_change). Swedish-only like the other (auth) surfaces: + * the reader may not even have a session, so user-preference locale does not + * apply. + * + * Secure email change requires a click in BOTH mails (new address + current + * address), and this page is the only feedback the user gets after each + * click, so it must say exactly what remains. + */ + +type EmailChangeStatus = 'partial' | 'done' | 'failed' + +const CONTENT: Record< + EmailChangeStatus, + { heading: string; body: string; cta: string; href: string } +> = { + partial: { + heading: 'Ett klick kvar', + body: 'Din bekräftelse är registrerad. Av säkerhetsskäl skickades två mail, ett till din nya adress och ett till din nuvarande. Öppna det andra mailet och klicka på länken där för att slutföra bytet.', + cta: 'Gå till startsidan', + href: '/', + }, + done: { + heading: 'E-postadressen är ändrad', + body: 'Klart! Din nya e-postadress gäller nu när du loggar in med e-post. Loggar du in med Google fortsätter det att fungera som vanligt.', + cta: 'Gå till startsidan', + href: '/', + }, + failed: { + heading: 'Länken är ogiltig eller har gått ut', + body: 'Bekräftelselänken kunde inte användas. Begär bytet igen under Inställningar: Konto, så skickas två nya bekräftelsemail direkt.', + cta: 'Gå till kontoinställningar', + href: '/settings/account', + }, +} + +function isEmailChangeStatus(value: string | undefined): value is EmailChangeStatus { + return value === 'partial' || value === 'done' || value === 'failed' +} + +export default async function EmailChangeStatusPage({ + searchParams, +}: { + searchParams: Promise<{ status?: string }> +}) { + const { status } = await searchParams + const resolved: EmailChangeStatus = isEmailChangeStatus(status) ? status : 'failed' + const content = CONTENT[resolved] + + // On completion the CTA goes where the callback would have sent the user + // before this page existed (WL-14: byrå staff on their home domain land in + // the cockpit, everyone else on the dashboard). Any failure degrades to '/'. + let href = content.href + if (resolved === 'done') { + try { + const supabase = await createClient() + const { + data: { user }, + } = await supabase.auth.getUser() + if (user) { + const headerStore = await headers() + const host = + headerStore.get('x-forwarded-host') ?? headerStore.get('host') ?? '' + href = await resolveLandingDestination(supabase, user.id, host) + } + } catch { + href = content.href + } + } + const Icon = + resolved === 'done' ? MailCheck : resolved === 'partial' ? Mails : MailWarning + + return ( +
+
+
+
+ +
+
+

{content.heading}

+

{content.body}

+ +
+
+ ) +} diff --git a/app/api/account/email/__tests__/route.test.ts b/app/api/account/email/__tests__/route.test.ts index f2ef1266..35fcc110 100644 --- a/app/api/account/email/__tests__/route.test.ts +++ b/app/api/account/email/__tests__/route.test.ts @@ -104,12 +104,13 @@ describe('POST /api/account/email', () => { expect(String(options.emailRedirectTo)).toMatch(/\/auth\/callback$/) }) - it('short-circuits a repeat request for the already-pending address', async () => { + it('short-circuits a repeat request while the pending mails are fresh', async () => { const { updateUser } = mockUserClient({ user: { id: 'user-1', email: 'old@testbrand.example', new_email: 'pending@testbrand.example', + email_change_sent_at: new Date(Date.now() - 60_000).toISOString(), } as { id: string; email?: string }, }) @@ -126,6 +127,47 @@ describe('POST /api/account/email', () => { expect(updateUser).not.toHaveBeenCalled() }) + it('re-sends when the pending change is stale (expired-link recovery)', async () => { + const { updateUser } = mockUserClient({ + user: { + id: 'user-1', + email: 'old@testbrand.example', + new_email: 'pending@testbrand.example', + email_change_sent_at: new Date( + Date.now() - 2 * 60 * 60 * 1000, + ).toISOString(), + } as { id: string; email?: string }, + }) + + const req = createMockRequest('/api/account/email', { + method: 'POST', + body: { email: 'pending@testbrand.example' }, + }) + const { status } = await parseJsonResponse(await POST(req)) + + expect(status).toBe(200) + expect(updateUser).toHaveBeenCalledTimes(1) + }) + + it('re-sends when the pending change has no sent timestamp', async () => { + const { updateUser } = mockUserClient({ + user: { + id: 'user-1', + email: 'old@testbrand.example', + new_email: 'pending@testbrand.example', + } as { id: string; email?: string }, + }) + + const req = createMockRequest('/api/account/email', { + method: 'POST', + body: { email: 'pending@testbrand.example' }, + }) + const { status } = await parseJsonResponse(await POST(req)) + + expect(status).toBe(200) + expect(updateUser).toHaveBeenCalledTimes(1) + }) + it('returns 409 when the address already belongs to another account', async () => { mockUserClient({ user: { id: 'user-1', email: 'old@testbrand.example' }, diff --git a/app/api/account/email/route.ts b/app/api/account/email/route.ts index 3490efc8..01b14183 100644 --- a/app/api/account/email/route.ts +++ b/app/api/account/email/route.ts @@ -12,6 +12,11 @@ const ChangeEmailSchema = z.object({ email: z.string().trim().toLowerCase().max(320).pipe(z.string().email()), }) +// How long a pending change is considered fresh enough that re-submitting the +// same address is a no-op instead of a re-send. Kept well under the token +// expiry so a user with a dead link can always get new mails. +const FRESH_PENDING_MS = 30 * 60 * 1000 + /** * POST /api/account/email * @@ -50,12 +55,24 @@ export async function POST(request: Request) { } // Re-requesting the address that is already awaiting confirmation is a - // no-op success rather than another GoTrue round trip (which would re-send - // both confirmation mails and eat into the send rate limit). new_email is - // absent on the claims-mapped fast path; then GoTrue's own rate limit is - // the backstop. + // no-op success ONLY while the pending mails are fresh (protects the send + // rate limit against double-clicks). Once they are older than that, the + // confirmation links may have expired and the user's only recovery path is + // re-running the change, so fall through to GoTrue, which restarts the + // change and re-sends both mails. new_email/email_change_sent_at are absent + // on the claims-mapped fast path; then GoTrue's own rate limit is the + // backstop. if (user.new_email && email === user.new_email.toLowerCase()) { - return NextResponse.json({ data: { ok: true, pending_email: email } }) + const sentAt = user.email_change_sent_at + ? Date.parse(user.email_change_sent_at) + : Number.NaN + const fresh = + Number.isFinite(sentAt) && Date.now() - sentAt < FRESH_PENDING_MS + if (fresh) { + return NextResponse.json({ + data: { ok: true, pending_email: email, resent: false }, + }) + } } // Trusted-origin resolution, not request.url: behind a proxy request.url @@ -98,5 +115,7 @@ export async function POST(request: Request) { log.info('email change requested', { userId: user.id }) - return NextResponse.json({ data: { ok: true, pending_email: email } }) + return NextResponse.json({ + data: { ok: true, pending_email: email, resent: true }, + }) } diff --git a/components/settings/sections/AccountSettingsContent.tsx b/components/settings/sections/AccountSettingsContent.tsx index 265ce730..6d339758 100644 --- a/components/settings/sections/AccountSettingsContent.tsx +++ b/components/settings/sections/AccountSettingsContent.tsx @@ -128,8 +128,12 @@ export function AccountSettingsContent() { return } setPendingEmail(trimmed) + const resent = (json as { data?: { resent?: boolean } } | null)?.data?.resent toast({ - title: tSettings('email_change_requested'), + title: + resent === false + ? tSettings('email_change_already_pending') + : tSettings('email_change_requested'), description: tSettings('email_change_requested_help'), }) } catch (err) { @@ -242,11 +246,14 @@ export function AccountSettingsContent() { !currentEmail || savingEmail || !email.trim() || - email.trim().toLowerCase() === currentEmail.toLowerCase() || - email.trim().toLowerCase() === pendingEmail + email.trim().toLowerCase() === currentEmail.toLowerCase() } > - {savingEmail ? tCommon('saving') : tCommon('save')} + {savingEmail + ? tCommon('saving') + : email.trim().toLowerCase() === pendingEmail + ? tSettings('email_resend') + : tCommon('save')} diff --git a/lib/email/__tests__/auth-templates.test.ts b/lib/email/__tests__/auth-templates.test.ts index 54f09e2d..66cf9bd3 100644 --- a/lib/email/__tests__/auth-templates.test.ts +++ b/lib/email/__tests__/auth-templates.test.ts @@ -49,6 +49,18 @@ describe('buildAuthEmail', () => { expect(mail.text).toMatchSnapshot() }) + it('tells the user both mails must be clicked for an email change', () => { + for (const actionType of ['email_change', 'email_change_current'] as const) { + const mail = buildAuthEmail({ + actionType, + appName: 'Siffra', + actionUrl: URL_EXAMPLE, + }) + expect(mail.text).toContain('två mail') + expect(mail.text).toContain('länken i båda') + } + }) + it('renders the reauthentication code without a link', () => { const mail = buildAuthEmail({ actionType: 'reauthentication', diff --git a/lib/email/auth-templates.ts b/lib/email/auth-templates.ts index 434d2029..9b5bd499 100644 --- a/lib/email/auth-templates.ts +++ b/lib/email/auth-templates.ts @@ -60,14 +60,14 @@ const CONTENT: Record = { subject: 'Bekräfta din nya e-postadress', heading: 'Bekräfta din nya e-postadress', body: (appName) => - `Klicka på knappen nedan för att bekräfta din nya e-postadress hos ${appName}.`, + `Klicka på knappen nedan för att bekräfta din nya e-postadress hos ${appName}. Av säkerhetsskäl skickas två mail, ett till din nya adress och ett till din nuvarande. Bytet slutförs först när du klickat på länken i båda.`, cta: 'Bekräfta ny e-postadress', }, email_change_current: { subject: 'Godkänn ändrad e-postadress', heading: 'Godkänn ändrad e-postadress', body: (appName) => - `En ändring av e-postadressen för ditt konto hos ${appName} har begärts. Klicka på knappen nedan för att godkänna ändringen från din nuvarande adress.`, + `En ändring av e-postadressen för ditt konto hos ${appName} har begärts. Klicka på knappen nedan för att godkänna ändringen från din nuvarande adress. Av säkerhetsskäl skickas två mail, ett till din nuvarande adress och ett till din nya. Bytet slutförs först när du klickat på länken i båda.`, cta: 'Godkänn ändringen', }, reauthentication: { diff --git a/lib/supabase/__tests__/middleware.test.ts b/lib/supabase/__tests__/middleware.test.ts index 71bbaa87..ae4c1c56 100644 --- a/lib/supabase/__tests__/middleware.test.ts +++ b/lib/supabase/__tests__/middleware.test.ts @@ -525,6 +525,28 @@ describe('updateSession redirect destinations', () => { expect(locationOf(await run('/sandbox?next=%2Fsettings%2Ftax'))).toBe(`${ORIGIN}/`) expect(locationOf(await run('/auth/callback?next=%2Fsettings%2Ftax'))).toBe(`${ORIGIN}/`) }) + + // Email-change confirmation links are usually clicked while still logged + // in (the change starts in settings), and the completing verify mints a + // session before the status page renders. Bouncing either request off the + // /auth prefix silently swallowed the confirmation. + it('lets an authenticated email-change confirmation reach the callback', async () => { + const response = await run('/auth/callback?token_hash=abc&type=email_change') + expect(response.status).not.toBe(307) + expect(locationOf(response)).toBeNull() + }) + + it('lets an authenticated user see the email-change status page', async () => { + const response = await run('/auth/email-change?status=done') + expect(response.status).not.toBe(307) + expect(locationOf(response)).toBeNull() + }) + + it('still bounces other authenticated token types off the callback', async () => { + expect(locationOf(await run('/auth/callback?token_hash=abc&type=signup'))).toBe( + `${ORIGIN}/`, + ) + }) }) // ── Sites 2 and 3: MFA step-up and forced enrollment ────────────────── diff --git a/lib/supabase/middleware.ts b/lib/supabase/middleware.ts index 776662bf..5583f5d7 100644 --- a/lib/supabase/middleware.ts +++ b/lib/supabase/middleware.ts @@ -282,6 +282,21 @@ async function updateSessionInner( return supabaseResponse } + // Email-change confirmations are reachable in both auth states, for the + // same reason as /reset-password above. The change starts in settings, so + // the confirmation links are usually clicked while still logged in, and the + // completing verify mints a fresh session itself. Bouncing authenticated + // requests off these paths (a) dropped the confirmation click before + // verifyOtp could consume the token, so the change never completed, and + // (b) hid the /auth/email-change status page in exactly the success case. + if ( + pathname.startsWith('/auth/email-change') || + (pathname.startsWith('/auth/callback') && + request.nextUrl.searchParams.get('type') === 'email_change') + ) { + return supabaseResponse + } + // Public agent-discovery + API docs surfaces. /llms.txt and /llms-full.txt // exist FOR anonymous consumers (the llms.txt convention targets logged-out // crawlers and IDE agents), and /docs is the public API documentation the diff --git a/messages/en.json b/messages/en.json index 7b0f273e..09668964 100644 --- a/messages/en.json +++ b/messages/en.json @@ -587,8 +587,10 @@ "name_save_failed": "Could not save name", "email_label": "Email address", "email_description": "Your sign-in address. When changing it, confirmation links are sent to both your current and your new address.", - "email_change_pending": "Confirmation pending for {email}. Click the links in the emails sent to both your old and your new address.", + "email_change_pending": "Confirmation pending for {email}. Click the links in the emails sent to both your old and your new address. If the links have expired, use Send again to get new ones.", "email_change_requested": "Confirmation emails sent", + "email_change_already_pending": "Confirmation emails already sent", + "email_resend": "Send again", "email_change_requested_help": "Click the links in the emails sent to both your old and your new address to complete the change.", "email_change_failed": "Could not request email change", "section_appearance": "Appearance", diff --git a/messages/sv.json b/messages/sv.json index 3c5bb777..a21f8850 100644 --- a/messages/sv.json +++ b/messages/sv.json @@ -587,8 +587,10 @@ "name_save_failed": "Kunde inte spara namnet", "email_label": "E-postadress", "email_description": "Din inloggningsadress. Vid byte skickas bekräftelselänkar till både din nuvarande och din nya adress.", - "email_change_pending": "Bekräftelse väntar för {email}. Klicka på länkarna i mejlen till både din gamla och din nya adress.", + "email_change_pending": "Bekräftelse väntar för {email}. Klicka på länkarna i mejlen till både din gamla och din nya adress. Har länkarna gått ut kan du skicka nya med Skicka igen.", "email_change_requested": "Bekräftelsemejl skickade", + "email_change_already_pending": "Bekräftelsemejl är redan skickade", + "email_resend": "Skicka igen", "email_change_requested_help": "Klicka på länkarna i mejlen till både din gamla och din nya adress för att slutföra bytet.", "email_change_failed": "Kunde inte begära e-poständring", "section_appearance": "Utseende",