fix(auth): address sync review feedback

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<void> {
+	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<void> {
-	// 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<User | null> {
+	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);
 }

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),
 			});
 		},
 	);

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(

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/);
+	});
 });

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<T>(
 	const onError = options?.onError;
 	const result = await betterFetch<T>(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;

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;
 		},