fix: consume one-time credentials atomically

.changeset/atomic-one-time-credentials.md

@@ -0,0 +1,5 @@
+---
+'seamless-auth-api': patch
+---
+
+Consume one-time credentials atomically. Email and phone OTPs are now cleared with an update conditional on the issuance that was compared, and a TOTP code is only accepted while the stored counter is still older than it, so concurrent submissions of one valid code yield exactly one success. TOTP enrollment enables the pending credential the same way. The losing request gets the same response and audit event as an invalid code.

.github/workflows/postgres.yml

@@ -23,6 +23,9 @@ on:
       - 'src/services/reviewAccountUsage.ts'
       - 'src/services/schemaGuard.ts'
       - 'src/services/signInMetrics.ts'
+      # Conditional updates whose single winner only a real database can show.
+      - 'src/services/totpService.ts'
+      - 'src/utils/otp.ts'
       - 'tests/postgres/**'
       - 'scripts/upgrade-check/**'
       - 'vitest.config.ts'

src/services/totpService.ts

@@ -5,6 +5,7 @@
  */
 
 import { createCipheriv, createDecipheriv, createHash } from 'crypto';
+import { Op } from 'sequelize';
 
 import { authSecret } from '../config/authSecrets.js';
 import { TotpCredential } from '../models/totpCredentials.js';
@@ -20,6 +21,8 @@ import {
 
 const TOTP_CIPHER = 'aes-256-gcm';
 
+class TotpClaimLost extends Error {}
+
 type TotpCredentialLike = Pick<
   TotpCredential,
   'secretCiphertext' | 'secretIv' | 'secretTag' | 'lastUsedCounter' | 'update' | 'destroy'
@@ -155,12 +158,37 @@ export async function verifyTotpEnrollment(userId: string, code: string) {
   }
 
   const now = new Date();
-  await TotpCredential.update({ enabled: false }, { where: { userId, enabled: true } });
-  await credential.update({
+  const counter = verification.counter;
+  try {
+    await TotpCredential.sequelize!.transaction(async (transaction) => {
+      await TotpCredential.update(
+        { enabled: false },
+        { where: { userId, enabled: true, id: { [Op.ne]: credential.id } }, transaction },
+      );
+
+      // A concurrent enrollment with the same code, or a newer one that destroyed this
+      // pending row, leaves nothing to enable, and the disable above must not stand alone.
+      const [claimed] = await TotpCredential.update(
+        { enabled: true, verifiedAt: now, lastUsedAt: now, lastUsedCounter: counter },
+        { where: { id: credential.id, enabled: false }, transaction },
+      );
+
+      if (claimed === 0) {
+        throw new TotpClaimLost();
+      }
+    });
+  } catch (error) {
+    if (error instanceof TotpClaimLost) {
+      return { verified: false, reason: 'invalid_code' };
+    }
+    throw error;
+  }
+
+  Object.assign(credential, {
     enabled: true,
     verifiedAt: now,
     lastUsedAt: now,
-    lastUsedCounter: verification.counter,
+    lastUsedCounter: counter,
   });
 
   return { verified: true, credential };
@@ -179,10 +207,28 @@ export async function verifyEnabledTotp(userId: string, code: string) {
     return { verified: false, reason: 'invalid_code' };
   }
 
-  await credential.update({
-    lastUsedAt: new Date(),
-    lastUsedCounter: verification.counter,
-  });
+  // Two requests with the same code both pass verifyTotpCode against the counter they
+  // read, so only the update that still finds an older counter accepts the code.
+  const now = new Date();
+  const [claimed] = await TotpCredential.update(
+    { lastUsedAt: now, lastUsedCounter: verification.counter },
+    {
+      where: {
+        id: credential.id,
+        enabled: true,
+        [Op.or]: [
+          { lastUsedCounter: null },
+          { lastUsedCounter: { [Op.lt]: verification.counter } },
+        ],
+      },
+    },
+  );
+
+  if (claimed === 0) {
+    return { verified: false, reason: 'invalid_code' };
+  }
+
+  Object.assign(credential, { lastUsedAt: now, lastUsedCounter: verification.counter });
 
   return { verified: true, credential };
 }

src/utils/otp.ts

@@ -55,6 +55,37 @@ function otpMatchesStoredValue(
   return safeStringEqual(storedToken, hashOtpToken(normalize(verificationToken)));
 }
 
+type OtpIssuance =
+  | { emailVerificationToken: string; emailVerificationTokenExpiry: number }
+  | { phoneVerificationToken: string; phoneVerificationTokenExpiry: number };
+
+// Two requests holding the same code both pass the in-memory comparison, so the clear is
+// conditional on the issuance that was compared: whichever clears it first wins and the
+// other sees no affected rows. The expiry is part of the match because a review account
+// is issued the same code each time, so the hash alone does not identify one issuance.
+async function consumeOtp(
+  user: User,
+  issuance: OtpIssuance,
+  applyVerification: () => void,
+): Promise<boolean> {
+  const cleared = Object.fromEntries(Object.keys(issuance).map((column) => [column, null]));
+
+  return User.sequelize!.transaction(async (transaction) => {
+    const [claimed] = await User.update(cleared, {
+      where: { id: user.id, ...issuance },
+      transaction,
+    });
+
+    if (claimed === 0) {
+      return false;
+    }
+
+    applyVerification();
+    await user.save({ transaction });
+    return true;
+  });
+}
+
 export const generateRandomEmailOTP = (): string => {
   let result = '';
   for (let i = 0; i < 6; i++) {
@@ -153,21 +184,34 @@ export const verifyPhoneOTP = async (
     otpMatchesStoredValue(user.phoneVerificationToken, verificationToken, normalizePhoneOtp) &&
     user.phoneVerificationTokenExpiry > new Date().getTime()
   ) {
-    user.phoneVerified = true;
-    user.phoneVerificationToken = null;
-    user.phoneVerificationTokenExpiry = null;
-    user.phoneVerifiedAttemptId = attemptId ?? null;
-
-    if (user.emailVerified && !user.verified) {
-      user.verified = true;
-    }
-
+    let claimed: boolean;
     try {
-      await user.save();
+      claimed = await consumeOtp(
+        user,
+        {
+          phoneVerificationToken: user.phoneVerificationToken,
+          phoneVerificationTokenExpiry: user.phoneVerificationTokenExpiry,
+        },
+        () => {
+          user.phoneVerified = true;
+          user.phoneVerificationToken = null;
+          user.phoneVerificationTokenExpiry = null;
+          user.phoneVerifiedAttemptId = attemptId ?? null;
+
+          if (user.emailVerified && !user.verified) {
+            user.verified = true;
+          }
+        },
+      );
     } catch (error) {
       logger.error(`Error verifying phone OTP: ${error}`);
       throw new Error('Failed to update user verification via phone OTP', { cause: error });
     }
+
+    if (!claimed) {
+      logger.warn('Phone OTP was consumed by a concurrent verification');
+      return { user, verified: false };
+    }
   } else {
     return { user, verified: false };
   }
@@ -188,18 +232,31 @@ export const verifyEmailOTP = async (
     otpMatchesStoredValue(user.emailVerificationToken, verificationToken, normalizeEmailOtp) &&
     user.emailVerificationTokenExpiry > new Date().getTime()
   ) {
-    user.emailVerified = true;
-    user.emailVerificationToken = null;
-    user.emailVerificationTokenExpiry = null;
-
-    claimUnverifiedAccount(user, attemptId);
-
+    let claimed: boolean;
     try {
-      await user.save();
+      claimed = await consumeOtp(
+        user,
+        {
+          emailVerificationToken: user.emailVerificationToken,
+          emailVerificationTokenExpiry: user.emailVerificationTokenExpiry,
+        },
+        () => {
+          user.emailVerified = true;
+          user.emailVerificationToken = null;
+          user.emailVerificationTokenExpiry = null;
+
+          claimUnverifiedAccount(user, attemptId);
+        },
+      );
     } catch (error) {
       logger.error(`Error verifying email OTP: ${error}`);
       throw new Error('Failed to update user verification via email OTP', { cause: error });
     }
+
+    if (!claimed) {
+      logger.warn('Email OTP was consumed by a concurrent verification');
+      return { user, verified: false };
+    }
   } else {
     return { user, verified: false };
   }

tests/postgres/oneTimeCredentials.spec.ts

@@ -0,0 +1,127 @@
+import { randomUUID } from 'crypto';
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+
+import { getSequelize, initializeModels } from '../../src/models/index.js';
+import { TotpCredential } from '../../src/models/totpCredentials.js';
+import { User } from '../../src/models/users.js';
+import {
+  encryptTotpSecret,
+  verifyEnabledTotp,
+  verifyTotpEnrollment,
+} from '../../src/services/totpService.js';
+import { hashOtpToken, verifyEmailOTP, verifyPhoneOTP } from '../../src/utils/otp.js';
+import { generateTotpCode } from '../../src/utils/totp.js';
+
+const SECRET = 'JBSWY3DPEHPK3PXP';
+const CONTENDERS = 5;
+
+async function createUser(values: Partial<User> = {}) {
+  const id = randomUUID();
+  await User.create({ id, email: `race-${id}@example.com`, roles: ['user'], ...values });
+  return id;
+}
+
+// Each contender holds its own instance read before any of them writes, which is the
+// state two concurrent requests are in once both have passed the comparison.
+async function contend<T>(load: () => Promise<T>, run: (loaded: T) => Promise<boolean>) {
+  const loaded = await Promise.all(Array.from({ length: CONTENDERS }, load));
+  const outcomes = await Promise.all(loaded.map(run));
+  return outcomes.filter(Boolean).length;
+}
+
+beforeAll(async () => {
+  process.env.TOTP_SECRET_ENCRYPTION_KEY = 'postgres-suite-totp-key';
+  await initializeModels();
+});
+
+afterAll(async () => {
+  await getSequelize().close();
+});
+
+describe('one-time credentials under concurrent submission', () => {
+  it('accepts one valid email OTP exactly once', async () => {
+    const id = await createUser({
+      emailVerificationToken: hashOtpToken('ABCDEF'),
+      emailVerificationTokenExpiry: Date.now() + 60_000,
+    });
+
+    const winners = await contend(
+      async () => (await User.findByPk(id))!,
+      async (user) => (await verifyEmailOTP(user, 'abcdef')).verified,
+    );
+
+    expect(winners).toBe(1);
+    const stored = await User.findByPk(id);
+    expect(stored?.emailVerificationToken).toBeNull();
+    expect(stored?.emailVerified).toBe(true);
+  });
+
+  it('accepts one valid phone OTP exactly once', async () => {
+    const id = await createUser({
+      phone: '+14155552671',
+      phoneVerificationToken: hashOtpToken('123456'),
+      phoneVerificationTokenExpiry: Date.now() + 60_000,
+    });
+
+    const winners = await contend(
+      async () => (await User.findByPk(id))!,
+      async (user) => (await verifyPhoneOTP(user, '123456')).verified,
+    );
+
+    expect(winners).toBe(1);
+    const stored = await User.findByPk(id);
+    expect(stored?.phoneVerificationToken).toBeNull();
+    expect(stored?.phoneVerified).toBe(true);
+  });
+
+  it('accepts one TOTP code exactly once', async () => {
+    const userId = await createUser({ verified: true, emailVerified: true });
+    await TotpCredential.create({
+      userId,
+      ...encryptTotpSecret(SECRET),
+      issuer: 'Seamless Auth',
+      accountName: `race-${userId}@example.com`,
+      enabled: true,
+      verifiedAt: new Date(),
+      lastUsedAt: null,
+      lastUsedCounter: null,
+    });
+    const code = generateTotpCode({ secret: SECRET });
+
+    const outcomes = await Promise.all(
+      Array.from({ length: CONTENDERS }, () => verifyEnabledTotp(userId, code)),
+    );
+
+    expect(outcomes.filter((outcome) => outcome.verified)).toHaveLength(1);
+  });
+
+  it('enables a pending TOTP credential exactly once', async () => {
+    const userId = await createUser({ verified: true, emailVerified: true });
+    const totp = (enabled: boolean) =>
+      TotpCredential.create({
+        userId,
+        ...encryptTotpSecret(SECRET),
+        issuer: 'Seamless Auth',
+        accountName: `race-${userId}@example.com`,
+        enabled,
+        verifiedAt: enabled ? new Date() : null,
+        lastUsedAt: null,
+        lastUsedCounter: null,
+      });
+    const previous = await totp(true);
+    const pending = await totp(false);
+    const code = generateTotpCode({ secret: SECRET });
+
+    const outcomes = await Promise.allSettled(
+      Array.from({ length: CONTENDERS }, () => verifyTotpEnrollment(userId, code)),
+    );
+
+    const verified = outcomes.filter(
+      (outcome) => outcome.status === 'fulfilled' && outcome.value.verified,
+    );
+    expect(verified).toHaveLength(1);
+    expect(outcomes.every((outcome) => outcome.status === 'fulfilled')).toBe(true);
+    await expect(TotpCredential.findByPk(previous.id)).resolves.toMatchObject({ enabled: false });
+    await expect(TotpCredential.findByPk(pending.id)).resolves.toMatchObject({ enabled: true });
+  });
+});

tests/setup/mocks.ts

@@ -71,9 +71,13 @@ vi.mock('../../src/models/webauthnChallenges.js', () => ({
 
 vi.mock('../../src/models/totpCredentials.js', () => ({
   TotpCredential: {
+    sequelize: {
+      transaction: vi.fn((fn: any) => fn({})),
+    },
     create: vi.fn(),
     findOne: vi.fn(),
-    update: vi.fn(),
+    // Defaults to one affected row, the uncontended outcome of a conditional update.
+    update: vi.fn(async () => [1]),
     destroy: vi.fn(),
     count: vi.fn(),
   },
@@ -129,7 +133,8 @@ vi.mock('../../src/models/users.js', () => ({
       transaction: vi.fn((fn: any) => fn({})),
     },
     create: vi.fn(),
-    update: vi.fn(),
+    // Defaults to one affected row, the uncontended outcome of a conditional update.
+    update: vi.fn(async () => [1]),
     findOne: vi.fn(),
     findByPk: vi.fn(),
     findAll: vi.fn(),