From 5d0d32f294c87eec9fba5e48ddb59e11fbebf374 Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Fri, 26 Jun 2026 14:17:36 +0100 Subject: [PATCH] fix(auth): address sync review feedback --- .../src/db/revoke-unproven-account-access.ts | 86 +++++++++++++++---- .../src/plugins/email-otp/routes.ts | 20 +++-- .../src/plugins/magic-link/index.ts | 12 ++- .../oauth2/reject-redirects.runtime.test.ts | 13 +++ packages/core/src/oauth2/reject-redirects.ts | 11 ++- .../src/oauth2/validate-authorization-code.ts | 4 +- 6 files changed, 112 insertions(+), 34 deletions(-) diff --git a/packages/better-auth/src/db/revoke-unproven-account-access.ts b/packages/better-auth/src/db/revoke-unproven-account-access.ts index b6c9c81d61..fbc47c0ca8 100644 --- a/packages/better-auth/src/db/revoke-unproven-account-access.ts +++ b/packages/better-auth/src/db/revoke-unproven-account-access.ts @@ -1,4 +1,32 @@ import type { GenericEndpointContext } from "@better-auth/core"; +import { BetterAuthError } from "@better-auth/core/error"; +import type { User } from "../types"; + +const cleanupLockExpiresInMs = 5000; +const cleanupLockWaitMs = 2000; +const cleanupLockPollMs = 25; + +const cleanupLockIdentifier = (userId: string) => + `revoke-unproven-account-access:${userId}`; + +async function waitForCleanupLock( + ctx: GenericEndpointContext, + identifier: string, +): Promise { + const deadline = Date.now() + cleanupLockWaitMs; + while (Date.now() < deadline) { + const lock = + await ctx.context.internalAdapter.findVerificationValue(identifier); + if (!lock) return; + if (lock.expiresAt <= new Date()) { + await ctx.context.internalAdapter.deleteVerificationByIdentifier( + identifier, + ); + return; + } + await new Promise((resolve) => setTimeout(resolve, cleanupLockPollMs)); + } +} /** * Strip every credential and session a pre-existing account accrued before @@ -8,29 +36,55 @@ import type { GenericEndpointContext } from "@better-auth/core"; * to the mailbox owner. When an email-primary proof (magic link, email OTP) * resolves to such a row, deleting the `credential` account and revoking standing * sessions makes the verified owner inherit no password or session that predates - * the proof. Call this before flipping `emailVerified` and minting the owner's - * session; it no-ops if a concurrent flow has already verified the account. + * the proof. This helper also flips `emailVerified` after cleanup and returns + * the current user for the caller to use when minting the owner's session. * * @param userId - The pre-existing, not-yet-verified user being promoted. */ export async function revokeUnprovenAccountAccess( ctx: GenericEndpointContext, userId: string, -): Promise { - // Re-read so the strip sees current state, not the caller's earlier check. - // FIXME: re-read and strip are not atomic and span the DB and secondary session - // store, so a verify-email landing in this window can clear a just-confirmed - // password (recoverable by reset, never a security loss). A durable fix needs - // cross-store reconciliation. - const user = await ctx.context.internalAdapter.findUserById(userId); - if (!user || user.emailVerified) { - return; +): Promise { + const lockIdentifier = cleanupLockIdentifier(userId); + const lockAcquired = await ctx.context.internalAdapter + .reserveVerificationValue({ + identifier: lockIdentifier, + value: userId, + expiresAt: new Date(Date.now() + cleanupLockExpiresInMs), + }) + .catch((error) => { + if ( + error instanceof BetterAuthError && + error.message.includes("requires database-backed verification storage") + ) { + return true; + } + throw error; + }); + if (!lockAcquired) { + await waitForCleanupLock(ctx, lockIdentifier); + return ctx.context.internalAdapter.findUserById(userId); } - const accounts = await ctx.context.internalAdapter.findAccounts(userId); - for (const account of accounts) { - if (account.providerId === "credential") { - await ctx.context.internalAdapter.deleteAccount(account.id); + + try { + // Re-read so the strip sees current state, not the caller's earlier check. + const user = await ctx.context.internalAdapter.findUserById(userId); + if (!user || user.emailVerified) { + return user; } + const accounts = await ctx.context.internalAdapter.findAccounts(userId); + for (const account of accounts) { + if (account.providerId === "credential") { + await ctx.context.internalAdapter.deleteAccount(account.id); + } + } + await ctx.context.internalAdapter.deleteUserSessions(userId); + return ctx.context.internalAdapter.updateUser(userId, { + emailVerified: true, + }); + } finally { + await ctx.context.internalAdapter + .deleteVerificationByIdentifier(lockIdentifier) + .catch(() => {}); } - await ctx.context.internalAdapter.deleteUserSessions(userId); } diff --git a/packages/better-auth/src/plugins/email-otp/routes.ts b/packages/better-auth/src/plugins/email-otp/routes.ts index 41a316ff70..6c5cd9debe 100644 --- a/packages/better-auth/src/plugins/email-otp/routes.ts +++ b/packages/better-auth/src/plugins/email-otp/routes.ts @@ -679,23 +679,27 @@ export const signInEmailOTP = (opts: RequiredEmailOTPOptions) => }); } - if (!user.user.emailVerified) { - await revokeUnprovenAccountAccess(ctx, user.user.id); - await ctx.context.internalAdapter.updateUser(user.user.id, { - emailVerified: true, - }); + let verifiedUser = user.user; + if (!verifiedUser.emailVerified) { + const promotedUser = await revokeUnprovenAccountAccess( + ctx, + verifiedUser.id, + ); + if (promotedUser) { + verifiedUser = promotedUser; + } } const session = await ctx.context.internalAdapter.createSession( - user.user.id, + verifiedUser.id, ); await setSessionCookie(ctx, { session, - user: user.user, + user: verifiedUser, }); return ctx.json({ token: session.token, - user: parseUserOutput(ctx.context.options, user.user), + user: parseUserOutput(ctx.context.options, verifiedUser), }); }, ); diff --git a/packages/better-auth/src/plugins/magic-link/index.ts b/packages/better-auth/src/plugins/magic-link/index.ts index 539a27350d..1729f8671e 100644 --- a/packages/better-auth/src/plugins/magic-link/index.ts +++ b/packages/better-auth/src/plugins/magic-link/index.ts @@ -433,10 +433,14 @@ export const magicLink = (options: MagicLinkOptions) => { } if (!user.emailVerified) { - await revokeUnprovenAccountAccess(ctx, user.id); - user = await ctx.context.internalAdapter.updateUser(user.id, { - emailVerified: true, - }); + const promotedUser = await revokeUnprovenAccountAccess( + ctx, + user.id, + ); + if (!promotedUser) { + redirectWithError("user_not_found"); + } + user = promotedUser; } const session = await ctx.context.internalAdapter.createSession( diff --git a/packages/core/src/oauth2/reject-redirects.runtime.test.ts b/packages/core/src/oauth2/reject-redirects.runtime.test.ts index a107342166..f1bebdb88c 100644 --- a/packages/core/src/oauth2/reject-redirects.runtime.test.ts +++ b/packages/core/src/oauth2/reject-redirects.runtime.test.ts @@ -59,4 +59,17 @@ describe("server-side OAuth fetch refuses redirects via real betterFetch", () => expect(onError).toHaveBeenCalledTimes(1); expect(onError.mock.calls[0]?.[0].response.status).toBe(401); }); + + it("throws the redirect-specific error when betterFetch throw mode is enabled", async () => { + mockedFetch.mockResolvedValueOnce( + new Response("", { + status: 302, + headers: { location: "http://169.254.169.254/" }, + }), + ); + + await expect( + fetchRefusingRedirects("https://idp.example/token", { throw: true }), + ).rejects.toThrow(/refuse redirects to prevent SSRF/); + }); }); diff --git a/packages/core/src/oauth2/reject-redirects.ts b/packages/core/src/oauth2/reject-redirects.ts index 114dad3fad..7e0dc84fa5 100644 --- a/packages/core/src/oauth2/reject-redirects.ts +++ b/packages/core/src/oauth2/reject-redirects.ts @@ -3,7 +3,7 @@ import type { BetterFetchOption } from "@better-fetch/fetch"; import { betterFetch } from "@better-fetch/fetch"; import { BetterAuthError } from "../error"; -const HTTP_REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); +const httpRedirectStatuses = new Set([301, 302, 303, 307, 308]); /** * Whether a response from a `redirect: "manual"` fetch is a redirect. @@ -15,7 +15,7 @@ const HTTP_REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); function isRedirectResponse(response: Response): boolean { return ( response.type === "opaqueredirect" || - HTTP_REDIRECT_STATUSES.has(response.status) + httpRedirectStatuses.has(response.status) ); } @@ -32,7 +32,7 @@ function redirectRefused(endpoint: string): BetterAuthError { * used and the resolved response is checked with {@link assertResponseNotRedirect} * (or, for betterFetch, with {@link fetchRefusingRedirects}). */ -export const NO_FOLLOW_REDIRECT = { redirect: "manual" } as const; +export const noFollowRedirect = { redirect: "manual" } as const; /** * Throw when a native-`fetch` response (e.g. jose's JWKS loader) resolved to a @@ -61,11 +61,14 @@ export async function fetchRefusingRedirects( const onError = options?.onError; const result = await betterFetch(url, { ...options, - ...NO_FOLLOW_REDIRECT, + ...noFollowRedirect, async onError(context) { if (isRedirectResponse(context.response)) redirected = true; await onError?.(context); }, + }).catch((error) => { + if (redirected) throw redirectRefused(url); + throw error; }); if (redirected) throw redirectRefused(url); return result; diff --git a/packages/core/src/oauth2/validate-authorization-code.ts b/packages/core/src/oauth2/validate-authorization-code.ts index 668f579cc2..da7ac06a05 100644 --- a/packages/core/src/oauth2/validate-authorization-code.ts +++ b/packages/core/src/oauth2/validate-authorization-code.ts @@ -5,7 +5,7 @@ import { getOAuth2Tokens } from "./index"; import { assertResponseNotRedirect, fetchRefusingRedirects, - NO_FOLLOW_REDIRECT, + noFollowRedirect, } from "./reject-redirects"; import type { TokenEndpointAuth, @@ -171,7 +171,7 @@ export async function validateToken( ) { const jwks = createRemoteJWKSet(new URL(jwksEndpoint), { [customFetch]: async (url, init) => { - const response = await fetch(url, { ...init, ...NO_FOLLOW_REDIRECT }); + const response = await fetch(url, { ...init, ...noFollowRedirect }); assertResponseNotRedirect(String(url), response); return response; },