mirror of
https://github.com/better-auth/better-auth.git
synced 2026-07-19 04:08:43 -05:00
fix: propagate sendVerificationEmail errors (#8863)
Co-authored-by: Bereket Engida <86073083+Bekacru@users.noreply.github.com>
This commit is contained in:
co-authored by
Bereket Engida
parent
04debbff04
commit
7d18175637
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"better-auth": patch
|
||||
---
|
||||
|
||||
`sendVerificationEmail` was invoked via `runInBackgroundOrAwait`, which could defer work when `advanced.backgroundTasks.handler` is configured (so the handler could return **200** before the email callback finished) and, in the default path, **caught and logged errors without rethrowing**. User callbacks that throw `APIError` (e.g. **429** from a rate limiter) were therefore not reliably reflected in the HTTP response ([better-auth/better-auth#8757](https://github.com/better-auth/better-auth/issues/8757)).
|
||||
|
||||
Now we await `sendVerificationEmailFn` so failures surface to the client with the correct status. The unauthenticated `/send-verification-email` path enforces a constant-time floor (500 ms) so that the response duration does not reveal whether the email belongs to a real unverified user.
|
||||
@@ -22,3 +22,4 @@ ciba
|
||||
CIBA
|
||||
Suliman
|
||||
Abdulrazzaq
|
||||
rethrowing
|
||||
@@ -1,3 +1,4 @@
|
||||
import { APIError } from "@better-auth/core/error";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { getTestInstance } from "../../test-utils/test-instance";
|
||||
|
||||
@@ -151,6 +152,45 @@ describe("Email Verification", async () => {
|
||||
);
|
||||
});
|
||||
|
||||
/**
|
||||
* @see https://github.com/better-auth/better-auth/issues/8757
|
||||
*/
|
||||
it("should return APIError status when sendVerificationEmail throws (e.g. rate limit)", async () => {
|
||||
let sendCalls = 0;
|
||||
const { auth, testUser } = await getTestInstance({
|
||||
emailAndPassword: {
|
||||
enabled: true,
|
||||
requireEmailVerification: true,
|
||||
},
|
||||
emailVerification: {
|
||||
async sendVerificationEmail() {
|
||||
sendCalls += 1;
|
||||
if (sendCalls >= 2) {
|
||||
throw APIError.from("TOO_MANY_REQUESTS", {
|
||||
code: "RATE_LIMIT_EXCEEDED",
|
||||
message: "Too many requests. Please try again later.",
|
||||
});
|
||||
}
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
try {
|
||||
await auth.api.sendVerificationEmail({
|
||||
body: {
|
||||
email: testUser.email,
|
||||
},
|
||||
});
|
||||
expect.fail("Expected sendVerificationEmail to throw");
|
||||
} catch (error) {
|
||||
expect(error).toBeInstanceOf(APIError);
|
||||
if (error instanceof APIError) {
|
||||
expect(error.status).toBe("TOO_MANY_REQUESTS");
|
||||
expect(error.body?.code).toBe("RATE_LIMIT_EXCEEDED");
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
it("should send a verification email if verification is required and user is not verified", async () => {
|
||||
await signInWithUser(testUser.email, testUser.password);
|
||||
|
||||
@@ -468,6 +508,77 @@ describe("Email Verification", async () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("Email Verification - Timing-safe unauthenticated path", () => {
|
||||
it("should enforce a constant-time floor for both existing and non-existing emails", async () => {
|
||||
const mockSendEmail = vi.fn();
|
||||
const { auth, testUser } = await getTestInstance({
|
||||
emailAndPassword: {
|
||||
enabled: true,
|
||||
requireEmailVerification: true,
|
||||
},
|
||||
emailVerification: {
|
||||
async sendVerificationEmail({ user }) {
|
||||
mockSendEmail(user.email);
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const startExisting = Date.now();
|
||||
await auth.api.sendVerificationEmail({
|
||||
body: { email: testUser.email },
|
||||
});
|
||||
const elapsedExisting = Date.now() - startExisting;
|
||||
|
||||
const startMissing = Date.now();
|
||||
await auth.api.sendVerificationEmail({
|
||||
body: { email: "nonexistent@example.com" },
|
||||
});
|
||||
const elapsedMissing = Date.now() - startMissing;
|
||||
|
||||
expect(elapsedExisting).toBeGreaterThanOrEqual(450);
|
||||
expect(elapsedMissing).toBeGreaterThanOrEqual(450);
|
||||
|
||||
expect(mockSendEmail).toHaveBeenCalledWith(testUser.email);
|
||||
expect(mockSendEmail).not.toHaveBeenCalledWith("nonexistent@example.com");
|
||||
});
|
||||
|
||||
it("should still surface errors from sendVerificationEmail after the minimum delay", async () => {
|
||||
let calls = 0;
|
||||
const { auth, testUser } = await getTestInstance({
|
||||
emailAndPassword: {
|
||||
enabled: true,
|
||||
requireEmailVerification: true,
|
||||
},
|
||||
emailVerification: {
|
||||
async sendVerificationEmail() {
|
||||
calls += 1;
|
||||
if (calls >= 2) {
|
||||
throw APIError.from("TOO_MANY_REQUESTS", {
|
||||
code: "RATE_LIMIT_EXCEEDED",
|
||||
message: "Too many requests.",
|
||||
});
|
||||
}
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const start = Date.now();
|
||||
try {
|
||||
await auth.api.sendVerificationEmail({
|
||||
body: { email: testUser.email },
|
||||
});
|
||||
expect.fail("Expected to throw");
|
||||
} catch (error) {
|
||||
expect(error).toBeInstanceOf(APIError);
|
||||
if (error instanceof APIError) {
|
||||
expect(error.status).toBe("TOO_MANY_REQUESTS");
|
||||
}
|
||||
}
|
||||
const elapsed = Date.now() - start;
|
||||
expect(elapsed).toBeGreaterThanOrEqual(450);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Email Verification Secondary Storage", async () => {
|
||||
const store = new Map<string, string>();
|
||||
let token: string;
|
||||
|
||||
@@ -64,15 +64,15 @@ export async function sendVerificationEmailFn(
|
||||
? encodeURIComponent(ctx.body.callbackURL)
|
||||
: encodeURIComponent("/");
|
||||
const url = `${ctx.context.baseURL}/verify-email?token=${token}&callbackURL=${callbackURL}`;
|
||||
await ctx.context.runInBackgroundOrAwait(
|
||||
ctx.context.options.emailVerification.sendVerificationEmail(
|
||||
{
|
||||
user: user,
|
||||
url,
|
||||
token,
|
||||
},
|
||||
ctx.request?.clone(),
|
||||
),
|
||||
// Await directly: `runInBackgroundOrAwait` may defer work or swallow errors (see #8757).
|
||||
// This path only runs once a real unverified user is known, so timing here does not weaken the unauthenticated anti-enumeration behavior above.
|
||||
await ctx.context.options.emailVerification.sendVerificationEmail(
|
||||
{
|
||||
user: user,
|
||||
url,
|
||||
token,
|
||||
},
|
||||
ctx.request,
|
||||
);
|
||||
}
|
||||
export const sendVerificationEmail = createAuthEndpoint(
|
||||
@@ -171,7 +171,16 @@ export const sendVerificationEmail = createAuthEndpoint(
|
||||
const { email } = ctx.body;
|
||||
const session = await getSessionFromCtx(ctx);
|
||||
if (!session) {
|
||||
/**
|
||||
* Enforce a constant-time floor so an attacker cannot distinguish
|
||||
* "email not found / already verified" (fast local JWT sign) from
|
||||
* "email found and unverified" (slow external email-send) by
|
||||
* comparing response times.
|
||||
*/
|
||||
const MINIMUM_MS = 500;
|
||||
const start = Date.now();
|
||||
const user = await ctx.context.internalAdapter.findUserByEmail(email);
|
||||
let error: unknown;
|
||||
if (!user || user.user.emailVerified) {
|
||||
await createEmailVerificationToken(
|
||||
ctx.context.secret,
|
||||
@@ -179,12 +188,18 @@ export const sendVerificationEmail = createAuthEndpoint(
|
||||
undefined,
|
||||
ctx.context.options.emailVerification?.expiresIn,
|
||||
);
|
||||
// We're returning true to avoid leaking information about the user
|
||||
return ctx.json({
|
||||
status: true,
|
||||
});
|
||||
} else {
|
||||
try {
|
||||
await sendVerificationEmailFn(ctx, user.user);
|
||||
} catch (e) {
|
||||
error = e;
|
||||
}
|
||||
}
|
||||
await sendVerificationEmailFn(ctx, user.user);
|
||||
const remaining = MINIMUM_MS - (Date.now() - start);
|
||||
if (remaining > 0) {
|
||||
await new Promise((resolve) => setTimeout(resolve, remaining));
|
||||
}
|
||||
if (error) throw error;
|
||||
return ctx.json({
|
||||
status: true,
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user