Fix concurrent password reset token reuse

apps/web/lib/auth.test.ts

@@ -2,10 +2,36 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
 
 vi.mock("server-only", () => ({}));
 
-const { execute } = vi.hoisted(() => ({ execute: vi.fn() }));
+const database = vi.hoisted(() => {
+  let closed = false;
+  const transactionExecute = vi.fn();
+  const commit = vi.fn(async () => {
+    closed = true;
+  });
+  const rollback = vi.fn(async () => {
+    closed = true;
+  });
+  const close = vi.fn(() => {
+    closed = true;
+  });
+  const transaction = vi.fn(async () => {
+    closed = false;
+    return {
+      execute: transactionExecute,
+      commit,
+      rollback,
+      close,
+      get closed() {
+        return closed;
+      },
+    };
+  });
+
+  return { execute: vi.fn(), transaction, transactionExecute, commit, rollback, close };
+});
 
 vi.mock("./db", () => ({
-  sqlClient: { execute },
+  sqlClient: { execute: database.execute, transaction: database.transaction },
 }));
 vi.mock("@aiornot/db", () => ({
   ids: { user: () => "user-1", verification: () => "verification-1" },
@@ -31,12 +57,17 @@ vi.mock("./referrals", () => ({
   rewardReferralOnVerify: vi.fn(async () => undefined),
 }));
 
-import { signup } from "./auth";
+import { resetPassword, signup } from "./auth";
 import { DISPLAY_NAME_MAX_LENGTH } from "./profile";
 
 describe("signup display names", () => {
   beforeEach(() => {
-    execute.mockReset();
+    database.execute.mockReset();
+    database.transaction.mockClear();
+    database.transactionExecute.mockReset();
+    database.commit.mockClear();
+    database.rollback.mockClear();
+    database.close.mockClear();
   });
 
   it("rejects names longer than the public profile limit before touching the database", async () => {
@@ -46,29 +77,85 @@ describe("signup display names", () => {
       ok: false,
       error: `Display name must be ${DISPLAY_NAME_MAX_LENGTH} characters or fewer.`,
     });
-    expect(execute).not.toHaveBeenCalled();
+    expect(database.execute).not.toHaveBeenCalled();
   });
 
   it("rejects non-string display names instead of coercing them", async () => {
     await expect(signup("player@example.com", "password123", { name: "Player" })).resolves.toEqual({
       ok: false,
       error: "Display name must be a string.",
     });
-    expect(execute).not.toHaveBeenCalled();
+    expect(database.execute).not.toHaveBeenCalled();
   });
 
   it("stores the same whitespace-normalized name used by profile updates", async () => {
-    execute.mockResolvedValueOnce({ rows: [] });
-    execute.mockResolvedValue({ rows: [], rowsAffected: 1 });
+    database.execute.mockResolvedValueOnce({ rows: [] });
+    database.execute.mockResolvedValue({ rows: [], rowsAffected: 1 });
 
     await expect(signup("player@example.com", "password123", "  Player\n  One  ")).resolves.toEqual({
       ok: true,
       userId: "user-1",
     });
 
-    expect(execute).toHaveBeenCalledWith({
+    expect(database.execute).toHaveBeenCalledWith({
       sql: expect.stringContaining("INSERT INTO users"),
       args: ["user-1", "player@example.com", "player@example.com", "password-hash", "Player One", "user"],
     });
   });
 });
+
+describe("password reset token consumption", () => {
+  beforeEach(() => {
+    database.execute.mockReset();
+    database.transaction.mockClear();
+    database.transactionExecute.mockReset();
+    database.commit.mockClear();
+    database.rollback.mockClear();
+    database.close.mockClear();
+  });
+
+  it("rejects a request that loses the atomic token claim", async () => {
+    database.execute.mockResolvedValueOnce({
+      rows: [{ id: "reset-1", user_id: "user-1", expires_at: "2999-01-01T00:00:00.000Z", consumed_at: null }],
+    });
+    database.transactionExecute.mockResolvedValueOnce({ rows: [], rowsAffected: 0 });
+
+    await expect(resetPassword("token", "new-password")).resolves.toEqual({
+      ok: false,
+      error: "This reset link was already used.",
+    });
+
+    expect(database.transactionExecute).toHaveBeenCalledTimes(1);
+    expect(database.transactionExecute).toHaveBeenCalledWith({
+      sql: expect.stringContaining("consumed_at IS NULL"),
+      args: ["reset-1"],
+    });
+    expect(database.rollback).toHaveBeenCalledOnce();
+    expect(database.commit).not.toHaveBeenCalled();
+  });
+
+  it("changes the password and invalidates sessions in the claiming transaction", async () => {
+    database.execute.mockResolvedValueOnce({
+      rows: [{ id: "reset-1", user_id: "user-1", expires_at: "2999-01-01T00:00:00.000Z", consumed_at: null }],
+    });
+    database.transactionExecute.mockResolvedValue({ rows: [], rowsAffected: 1 });
+
+    await expect(resetPassword("token", "new-password")).resolves.toEqual({
+      ok: true,
+      userId: "user-1",
+    });
+
+    expect(database.transaction).toHaveBeenCalledWith("write");
+    expect(database.transactionExecute).toHaveBeenCalledTimes(4);
+    expect(database.transactionExecute).toHaveBeenNthCalledWith(2, {
+      sql: expect.stringContaining("UPDATE users"),
+      args: ["password-hash", "user-1"],
+    });
+    expect(database.transactionExecute).toHaveBeenNthCalledWith(4, {
+      sql: "DELETE FROM sessions WHERE user_id = ?",
+      args: ["user-1"],
+    });
+    expect(database.commit).toHaveBeenCalledOnce();
+    expect(database.rollback).not.toHaveBeenCalled();
+  });
+});

apps/web/lib/auth.ts

@@ -257,25 +257,40 @@ export async function resetPassword(token: string, newPassword: string): Promise
 
   const userId = row.user_id as string;
   const pw = await hashPassword(newPassword);
-  await sqlClient.execute({
-    sql: `UPDATE users
-          SET password_hash = ?,
-              email_verified_at = COALESCE(email_verified_at, CURRENT_TIMESTAMP),
-              status = CASE WHEN status = 'pending_email_verification' THEN 'active' ELSE status END,
-              updated_at = CURRENT_TIMESTAMP
-          WHERE id = ?`,
-    args: [pw, userId],
-  });
-  await sqlClient.execute({
-    sql: "UPDATE password_reset_tokens SET consumed_at = CURRENT_TIMESTAMP WHERE id = ?",
-    args: [row.id],
-  });
-  // Invalidate any other outstanding reset tokens + all sessions for this user.
-  await sqlClient.execute({
-    sql: "UPDATE password_reset_tokens SET consumed_at = CURRENT_TIMESTAMP WHERE user_id = ? AND consumed_at IS NULL",
-    args: [userId],
-  });
-  await sqlClient.execute({ sql: "DELETE FROM sessions WHERE user_id = ?", args: [userId] });
+  const transaction = await sqlClient.transaction("write");
+  try {
+    const claim = await transaction.execute({
+      sql: `UPDATE password_reset_tokens
+            SET consumed_at = CURRENT_TIMESTAMP
+            WHERE id = ? AND consumed_at IS NULL AND expires_at >= CURRENT_TIMESTAMP`,
+      args: [row.id],
+    });
+    if (claim.rowsAffected !== 1) {
+      await transaction.rollback();
+      return { ok: false, error: "This reset link was already used." };
+    }
+
+    await transaction.execute({
+      sql: `UPDATE users
+            SET password_hash = ?,
+                email_verified_at = COALESCE(email_verified_at, CURRENT_TIMESTAMP),
+                status = CASE WHEN status = 'pending_email_verification' THEN 'active' ELSE status END,
+                updated_at = CURRENT_TIMESTAMP
+            WHERE id = ?`,
+      args: [pw, userId],
+    });
+    await transaction.execute({
+      sql: "UPDATE password_reset_tokens SET consumed_at = CURRENT_TIMESTAMP WHERE user_id = ? AND consumed_at IS NULL",
+      args: [userId],
+    });
+    await transaction.execute({ sql: "DELETE FROM sessions WHERE user_id = ?", args: [userId] });
+    await transaction.commit();
+  } catch (error) {
+    if (!transaction.closed) await transaction.rollback();
+    throw error;
+  } finally {
+    transaction.close();
+  }
 
   return { ok: true, userId };
 }