fix(auth): land stock email-change links on the status page and stop retries voiding pending mails (#2199)
* fix(auth): land stock email-change links on the status page and stop retries voiding pending mails A secure email change needs one click in each mailbox. Stock GoTrue links verify on the GoTrue host and return to /auth/callback through redirect_to with ?message= (first click), ?error= (dead link) or ?code= (completing click); none carries a token_hash, so the callback bounced every one of them to /login with no message. Users read that as a failure and pressed "Byt" again, and because the claims fast path carries no new_email, the route re-issued both tokens on every press and voided the links they were about to click. - /api/account/email stamps flow=email_change on emailRedirectTo and reads pending state from GoTrue when the session claims lack it, so a repeat request inside the 30-minute window is a no-op instead of a re-send. - /auth/callback routes flow=email_change redirects to /auth/email-change?status=partial|done|failed; hook-style token_hash links keep using the existing verifyOtp branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LMFybWJqw8vScQiEDwKXGi * fix(auth): let signed-in stock email-change redirects through the proxy and treat a minted code as done Skeptic findings on e5639fb43: - The proxy bounced authenticated /auth/callback requests to / unless they carried type=email_change. Stock GoTrue links return with only the flow=email_change marker, so the new status branch was unreachable from the signed-in browser the change usually starts in. Exempt the marker too. - A completing click opened in a browser without the PKCE verifier (phone mail app) failed the code exchange and, with no session to inspect, was reported as a failed change although GoTrue had already flipped the address. A code is only minted after that verify, so report done. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LMFybWJqw8vScQiEDwKXGi * fix(auth): gate email-change requests with an atomic per-user claim CodeRabbit on PR #2199: the pending-state read from GoTrue is not atomic, so two concurrent POST /api/account/email calls (two tabs, a retried fetch) could both see nothing pending and both re-issue the confirmation tokens, voiding each other's mails. Migration 20260903083000 adds email_change_requests (one row per auth user, RLS with no policies) and two SECURITY DEFINER RPCs: claim_email_change_request(p_email, p_window_seconds) is a single INSERT ... ON CONFLICT DO UPDATE whose row lock serialises concurrent claimers, so exactly one caller per address per window wins; a different address always wins; release_email_change_request drops the claim when GoTrue refuses the change so the user can retry. The route claims right before updateUser, answers resent:false when the claim is held, releases on GoTrue failure, and falls through to GoTrue if the RPC itself errors. pg-real test covers sequential, windowed, concurrent, per-user, release and RLS behaviour. Applied to staging with the same version. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LMFybWJqw8vScQiEDwKXGi --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Fable 5.1
Claude Fable 5.1
parent
51b68afc87
commit
828628d882
@@ -12,14 +12,41 @@ import { POST } from '../route'
|
||||
function mockUserClient(opts: {
|
||||
user: { id: string; email?: string } | null
|
||||
updateUserError?: { message: string; status?: number; code?: string } | null
|
||||
// What GoTrue returns for the fresh user (the pending-change fields the
|
||||
// claims fast path lacks). Defaults to "no pending change".
|
||||
freshUser?: {
|
||||
new_email?: string
|
||||
email_change_sent_at?: string
|
||||
} | null
|
||||
// Outcome of claim_email_change_request: true (won, default), false
|
||||
// (another request holds the claim), or an error object (RPC failed).
|
||||
claim?: boolean | { message: string; code?: string }
|
||||
}) {
|
||||
const updateUser = vi.fn().mockResolvedValue({
|
||||
data: {},
|
||||
error: opts.updateUserError ?? null,
|
||||
})
|
||||
const rpc = vi.fn().mockImplementation(async (name: string) => {
|
||||
if (name === 'claim_email_change_request') {
|
||||
const claim = opts.claim ?? true
|
||||
return typeof claim === 'boolean'
|
||||
? { data: claim, error: null }
|
||||
: { data: null, error: claim }
|
||||
}
|
||||
return { data: null, error: null }
|
||||
})
|
||||
const getUser = vi.fn().mockResolvedValue({
|
||||
data: {
|
||||
user:
|
||||
opts.freshUser === null
|
||||
? null
|
||||
: { id: opts.user?.id, email: opts.user?.email, ...(opts.freshUser ?? {}) },
|
||||
},
|
||||
error: null,
|
||||
})
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
const supabase = { auth: { updateUser } } as any
|
||||
const supabase = { auth: { updateUser, getUser }, rpc } as any
|
||||
|
||||
if (opts.user) {
|
||||
requireAuthMock.mockResolvedValue({ user: opts.user, supabase, error: null })
|
||||
@@ -31,7 +58,7 @@ function mockUserClient(opts: {
|
||||
})
|
||||
}
|
||||
|
||||
return { updateUser }
|
||||
return { updateUser, getUser, rpc }
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -101,7 +128,11 @@ describe('POST /api/account/email', () => {
|
||||
expect(updateUser).toHaveBeenCalledTimes(1)
|
||||
const [attrs, options] = updateUser.mock.calls[0]
|
||||
expect(attrs).toEqual({ email: 'new@testbrand.example' })
|
||||
expect(String(options.emailRedirectTo)).toMatch(/\/auth\/callback$/)
|
||||
// flow=email_change routes the stock GoTrue redirect (message/error/code)
|
||||
// to the email-change status page in /auth/callback.
|
||||
expect(String(options.emailRedirectTo)).toMatch(
|
||||
/\/auth\/callback\?flow=email_change$/,
|
||||
)
|
||||
})
|
||||
|
||||
it('short-circuits a repeat request while the pending mails are fresh', async () => {
|
||||
@@ -168,6 +199,181 @@ describe('POST /api/account/email', () => {
|
||||
expect(updateUser).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('reads pending state from GoTrue when the session claims lack it (fresh: no-op)', async () => {
|
||||
// The claims fast path carries no new_email; before this the route
|
||||
// re-issued tokens on every re-submit and voided the mails just sent.
|
||||
const { updateUser, getUser } = mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
freshUser: {
|
||||
new_email: 'pending@testbrand.example',
|
||||
email_change_sent_at: new Date(Date.now() - 3 * 60_000).toISOString(),
|
||||
},
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'pending@testbrand.example' },
|
||||
})
|
||||
const { status, body } = await parseJsonResponse<{
|
||||
data?: { ok: boolean; pending_email: string; resent: boolean }
|
||||
}>(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(body.data?.resent).toBe(false)
|
||||
expect(getUser).toHaveBeenCalledTimes(1)
|
||||
expect(updateUser).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('reads pending state from GoTrue when the session claims lack it (stale: re-send)', async () => {
|
||||
const { updateUser } = mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
freshUser: {
|
||||
new_email: 'pending@testbrand.example',
|
||||
email_change_sent_at: new Date(
|
||||
Date.now() - 2 * 60 * 60 * 1000,
|
||||
).toISOString(),
|
||||
},
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'pending@testbrand.example' },
|
||||
})
|
||||
const { status, body } = await parseJsonResponse<{
|
||||
data?: { resent: boolean }
|
||||
}>(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(body.data?.resent).toBe(true)
|
||||
expect(updateUser).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('requests a different address even while another change is pending and fresh', async () => {
|
||||
const { updateUser } = mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
freshUser: {
|
||||
new_email: 'pending@testbrand.example',
|
||||
email_change_sent_at: new Date(Date.now() - 60_000).toISOString(),
|
||||
},
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'other@testbrand.example' },
|
||||
})
|
||||
const { status } = await parseJsonResponse(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(updateUser).toHaveBeenCalledTimes(1)
|
||||
expect(updateUser.mock.calls[0][0]).toEqual({ email: 'other@testbrand.example' })
|
||||
})
|
||||
|
||||
it('does not consult GoTrue when the claims already carry the pending change', async () => {
|
||||
const { updateUser, getUser } = 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 },
|
||||
})
|
||||
|
||||
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(getUser).not.toHaveBeenCalled()
|
||||
expect(updateUser).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('claims the address atomically before calling GoTrue', async () => {
|
||||
const { updateUser, rpc } = mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'new@testbrand.example' },
|
||||
})
|
||||
const { status } = await parseJsonResponse(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(rpc).toHaveBeenCalledWith('claim_email_change_request', {
|
||||
p_email: 'new@testbrand.example',
|
||||
p_window_seconds: 30 * 60,
|
||||
})
|
||||
// Claim strictly before the GoTrue call.
|
||||
expect(rpc.mock.invocationCallOrder[0]).toBeLessThan(
|
||||
updateUser.mock.invocationCallOrder[0],
|
||||
)
|
||||
expect(rpc).not.toHaveBeenCalledWith('release_email_change_request')
|
||||
})
|
||||
|
||||
it('answers already-pending without calling GoTrue when a concurrent request holds the claim', async () => {
|
||||
// Both requests read "nothing pending" from GoTrue; only the claim
|
||||
// winner may re-issue the tokens.
|
||||
const { updateUser } = mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
claim: false,
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'new@testbrand.example' },
|
||||
})
|
||||
const { status, body } = await parseJsonResponse<{
|
||||
data?: { ok: boolean; pending_email: string; resent: boolean }
|
||||
}>(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(body.data).toEqual({
|
||||
ok: true,
|
||||
pending_email: 'new@testbrand.example',
|
||||
resent: false,
|
||||
})
|
||||
expect(updateUser).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('proceeds without the claim when the RPC itself fails', async () => {
|
||||
const { updateUser, rpc } = mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
claim: { message: 'function does not exist', code: '42883' },
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'new@testbrand.example' },
|
||||
})
|
||||
const { status } = await parseJsonResponse(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(updateUser).toHaveBeenCalledTimes(1)
|
||||
expect(rpc).not.toHaveBeenCalledWith('release_email_change_request')
|
||||
})
|
||||
|
||||
it('releases the claim when GoTrue refuses the change', async () => {
|
||||
const { rpc } = mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
updateUserError: {
|
||||
message: 'AAL2 session is required',
|
||||
status: 403,
|
||||
code: 'insufficient_aal',
|
||||
},
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'new@testbrand.example' },
|
||||
})
|
||||
const { status } = await parseJsonResponse(await POST(req))
|
||||
|
||||
expect(status).toBe(400)
|
||||
expect(rpc).toHaveBeenCalledWith('release_email_change_request')
|
||||
})
|
||||
|
||||
it('returns 409 when the address already belongs to another account', async () => {
|
||||
mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
|
||||
@@ -59,13 +59,25 @@ export async function POST(request: Request) {
|
||||
// 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()) {
|
||||
const sentAt = user.email_change_sent_at
|
||||
? Date.parse(user.email_change_sent_at)
|
||||
: Number.NaN
|
||||
// change and re-sends both mails.
|
||||
//
|
||||
// new_email/email_change_sent_at live on the GoTrue user, not in the JWT,
|
||||
// so they are absent on the claims-mapped fast path of requireAuth. Reading
|
||||
// them from the claims alone made every re-submit look like a brand-new
|
||||
// request: GoTrue re-issued both tokens and voided the links the user was
|
||||
// about to click, which is exactly the "link invalid" loop users hit after
|
||||
// pressing the button twice. Fetch the fresh user when the claims carry no
|
||||
// pending state; the extra round trip is fine on a route this rare.
|
||||
let pendingEmail = user.new_email
|
||||
let pendingSentAt = user.email_change_sent_at
|
||||
if (!pendingEmail) {
|
||||
const { data } = await supabase.auth.getUser()
|
||||
pendingEmail = data?.user?.new_email
|
||||
pendingSentAt = data?.user?.email_change_sent_at
|
||||
}
|
||||
|
||||
if (pendingEmail && email === pendingEmail.toLowerCase()) {
|
||||
const sentAt = pendingSentAt ? Date.parse(pendingSentAt) : Number.NaN
|
||||
const fresh =
|
||||
Number.isFinite(sentAt) && Date.now() - sentAt < FRESH_PENDING_MS
|
||||
if (fresh) {
|
||||
@@ -79,10 +91,40 @@ export async function POST(request: Request) {
|
||||
// can be an internal origin (dead confirmation links on self-hosted), and
|
||||
// auth links may never follow an attacker-chosen host. Registered
|
||||
// white-label hosts pass through so the mail carries the right brand.
|
||||
//
|
||||
// flow=email_change marks the callback so the stock GoTrue links (verified
|
||||
// on the GoTrue host, returned here via redirect_to with ?message=, ?error=
|
||||
// or ?code= instead of a token_hash) land on the email-change status page
|
||||
// rather than the silent login bounce. The Send Email hook preserves this
|
||||
// query on its token_hash links, so both link styles share the marker.
|
||||
const origin = resolveRequestAppOrigin(request)
|
||||
|
||||
// Cross-instance gate (migration 20260903083000). The pending-state read
|
||||
// above is not atomic: two concurrent requests (two tabs, a retried fetch)
|
||||
// can both see nothing pending, and each updateUser re-issues the tokens
|
||||
// and voids the other's mails. claim_email_change_request is one
|
||||
// INSERT ... ON CONFLICT row lock per user, so exactly one caller per
|
||||
// address per window proceeds; the rest answer "already pending" and send
|
||||
// nothing. A different address always wins the claim. Best effort: if the
|
||||
// RPC itself fails, fall through to GoTrue rather than block the change.
|
||||
const { data: claimed, error: claimError } = await supabase.rpc(
|
||||
'claim_email_change_request',
|
||||
{ p_email: email, p_window_seconds: FRESH_PENDING_MS / 1000 },
|
||||
)
|
||||
if (claimError) {
|
||||
log.warn('email change claim failed; proceeding without it', {
|
||||
userId: user.id,
|
||||
code: claimError.code,
|
||||
})
|
||||
} else if (claimed === false) {
|
||||
return NextResponse.json({
|
||||
data: { ok: true, pending_email: email, resent: false },
|
||||
})
|
||||
}
|
||||
|
||||
const { error: updateError } = await supabase.auth.updateUser(
|
||||
{ email },
|
||||
{ emailRedirectTo: `${origin}/auth/callback` },
|
||||
{ emailRedirectTo: `${origin}/auth/callback?flow=email_change` },
|
||||
)
|
||||
|
||||
if (updateError) {
|
||||
@@ -91,6 +133,17 @@ export async function POST(request: Request) {
|
||||
code: updateError.code,
|
||||
status: updateError.status,
|
||||
})
|
||||
// GoTrue sent nothing, so the claim must not block a retry (after MFA,
|
||||
// with another address, once the network is back).
|
||||
if (!claimError) {
|
||||
const { error: releaseError } = await supabase.rpc('release_email_change_request')
|
||||
if (releaseError) {
|
||||
log.warn('email change claim release failed', {
|
||||
userId: user.id,
|
||||
code: releaseError.code,
|
||||
})
|
||||
}
|
||||
}
|
||||
// Addresses are unique per auth user: a change to an already-registered
|
||||
// address is refused by GoTrue, never merged. Accounts are consolidated
|
||||
// via company invitations, not email changes.
|
||||
|
||||
Reference in New Issue
Block a user