diff --git a/.changeset/atomic-one-time-credentials.md b/.changeset/atomic-one-time-credentials.md new file mode 100644 index 00000000..0a82b123 --- /dev/null +++ b/.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. diff --git a/.github/workflows/postgres.yml b/.github/workflows/postgres.yml index 8afb7427..148e8663 100644 --- a/.github/workflows/postgres.yml +++ b/.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' diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index c931b02b..322cf29d 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 99.2% + + coverage: 99.1% @@ -17,7 +17,7 @@ coverage coverage - 99.2% - 99.2% + 99.1% + 99.1% diff --git a/src/services/totpService.ts b/src/services/totpService.ts index 84c571bf..55c22249 100644 --- a/src/services/totpService.ts +++ b/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 }; } diff --git a/src/utils/otp.ts b/src/utils/otp.ts index 26016859..38690b9b 100644 --- a/src/utils/otp.ts +++ b/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 { + 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 }; } diff --git a/tests/postgres/oneTimeCredentials.spec.ts b/tests/postgres/oneTimeCredentials.spec.ts new file mode 100644 index 00000000..da177e41 --- /dev/null +++ b/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 = {}) { + 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(load: () => Promise, run: (loaded: T) => Promise) { + 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 }); + }); +}); diff --git a/tests/setup/mocks.ts b/tests/setup/mocks.ts index 9926a1e6..d7ce324e 100644 --- a/tests/setup/mocks.ts +++ b/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(), diff --git a/tests/unit/services/totpService.spec.ts b/tests/unit/services/totpService.spec.ts index fc0037f6..fc7b4b31 100644 --- a/tests/unit/services/totpService.spec.ts +++ b/tests/unit/services/totpService.spec.ts @@ -1,4 +1,5 @@ import { randomBytes } from 'crypto'; +import { Op } from 'sequelize'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { TotpCredential } from '../../../src/models/totpCredentials.js'; @@ -62,16 +63,92 @@ describe('totpService', () => { expect(result.verified).toBe(true); expect(TotpCredential.update).toHaveBeenCalledWith( { enabled: false }, - { where: { userId: 'user-1', enabled: true } }, + { + where: { userId: 'user-1', enabled: true, id: { [Op.ne]: 'totp-1' } }, + transaction: expect.anything(), + }, ); - expect(credential.update).toHaveBeenCalledWith( + expect(TotpCredential.update).toHaveBeenCalledWith( expect.objectContaining({ enabled: true, verifiedAt: expect.any(Date), lastUsedAt: expect.any(Date), lastUsedCounter: expect.any(Number), }), + { where: { id: 'totp-1', enabled: false }, transaction: expect.anything() }, ); + expect(credential.enabled).toBe(true); + }); + + it('rejects enrollment when a concurrent verification already enabled the credential', async () => { + const secret = 'JBSWY3DPEHPK3PXP'; + const credential = buildTotpCredential({ + ...encryptTotpSecret(secret), + enabled: false, + lastUsedCounter: null, + }); + + (TotpCredential.findOne as any).mockResolvedValue(credential); + (TotpCredential.update as any).mockResolvedValueOnce([0]).mockResolvedValueOnce([0]); + + await expect(verifyTotpEnrollment('user-1', generateTotpCode({ secret }))).resolves.toEqual({ + verified: false, + reason: 'invalid_code', + }); + expect(credential.enabled).toBe(false); + }); + + it('propagates a database failure during enrollment', async () => { + const secret = 'JBSWY3DPEHPK3PXP'; + const credential = buildTotpCredential({ + ...encryptTotpSecret(secret), + enabled: false, + lastUsedCounter: null, + }); + + (TotpCredential.findOne as any).mockResolvedValue(credential); + (TotpCredential.update as any).mockRejectedValueOnce(new Error('db down')); + + await expect(verifyTotpEnrollment('user-1', generateTotpCode({ secret }))).rejects.toThrow( + 'db down', + ); + }); + + it('accepts a code only while the stored counter is still older', async () => { + const secret = 'JBSWY3DPEHPK3PXP'; + const credential = buildTotpCredential({ ...encryptTotpSecret(secret), lastUsedCounter: 5 }); + + (TotpCredential.findOne as any).mockResolvedValue(credential); + + const result = await verifyEnabledTotp('user-1', generateTotpCode({ secret })); + + expect(result.verified).toBe(true); + const [values, options] = (TotpCredential.update as any).mock.calls[0]; + expect(options).toEqual({ + where: { + id: 'totp-1', + enabled: true, + [Op.or]: [ + { lastUsedCounter: null }, + { lastUsedCounter: { [Op.lt]: values.lastUsedCounter } }, + ], + }, + }); + expect(credential.lastUsedCounter).toBe(values.lastUsedCounter); + }); + + it('rejects a code another request consumed between the read and the write', async () => { + const secret = 'JBSWY3DPEHPK3PXP'; + const credential = buildTotpCredential({ ...encryptTotpSecret(secret), lastUsedCounter: null }); + + (TotpCredential.findOne as any).mockResolvedValue(credential); + (TotpCredential.update as any).mockResolvedValueOnce([0]); + + await expect(verifyEnabledTotp('user-1', generateTotpCode({ secret }))).resolves.toEqual({ + verified: false, + reason: 'invalid_code', + }); + expect(credential.lastUsedCounter).toBeNull(); }); it('rejects reused TOTP counters', async () => { @@ -86,7 +163,7 @@ describe('totpService', () => { (TotpCredential.findOne as any).mockResolvedValue(firstCredential); const firstResult = await verifyEnabledTotp('user-1', code); - const usedCounter = firstCredential.update.mock.calls[0][0].lastUsedCounter; + const usedCounter = (TotpCredential.update as any).mock.calls[0][0].lastUsedCounter; const replayedCredential = buildTotpCredential({ ...encrypted, lastUsedCounter: usedCounter, @@ -98,7 +175,7 @@ describe('totpService', () => { expect(firstResult.verified).toBe(true); expect(replayResult.verified).toBe(false); - expect(replayedCredential.update).not.toHaveBeenCalled(); + expect(TotpCredential.update).toHaveBeenCalledTimes(1); }); it('reports enabled status from the active credential', async () => { @@ -150,7 +227,6 @@ describe('totpService', () => { verified: false, reason: 'invalid_code', }); - expect(credential.update).not.toHaveBeenCalled(); expect(TotpCredential.update).not.toHaveBeenCalled(); }); @@ -173,7 +249,7 @@ describe('totpService', () => { verified: false, reason: 'invalid_code', }); - expect(credential.update).not.toHaveBeenCalled(); + expect(TotpCredential.update).not.toHaveBeenCalled(); }); it('rejects disabling when no enabled credential exists', async () => { diff --git a/tests/unit/utils/otp.spec.ts b/tests/unit/utils/otp.spec.ts index 8187c59e..f460059d 100644 --- a/tests/unit/utils/otp.spec.ts +++ b/tests/unit/utils/otp.spec.ts @@ -16,6 +16,7 @@ import { verifyEmailOTP, } from '../../../src/utils/otp.js'; +import { User } from '../../../src/models/users.js'; import { sendOTPEmail, sendOTPSMS } from '../../../src/services/messagingService'; function buildUser(overrides: any = {}) { return { @@ -429,3 +430,111 @@ describe('OTP verification save failures', () => { expect(user.verified).toBe(true); }); }); + +describe('OTP consumption under concurrency', () => { + // Two requests holding the same valid code both pass the comparison, so the clear is + // conditional on the issuance that was compared and only one of them may win it. + beforeEach(() => { + vi.clearAllMocks(); + }); + + const emailPending = (overrides: any = {}) => + buildUser({ + id: 'user-1', + emailVerificationToken: hashOtpToken('ABCDEF'), + emailVerificationTokenExpiry: 1_900_000_000_000, + phone: null, + ...overrides, + }); + + const phonePending = (overrides: any = {}) => + buildUser({ + id: 'user-1', + phoneVerificationToken: hashOtpToken('123456'), + phoneVerificationTokenExpiry: 1_900_000_000_000, + emailVerified: true, + ...overrides, + }); + + it('clears the email OTP only where it still holds the compared issuance', async () => { + const user = emailPending(); + + await expect(verifyEmailOTP(user as any, 'abcdef')).resolves.toMatchObject({ + verified: true, + }); + + expect(User.update).toHaveBeenCalledWith( + { emailVerificationToken: null, emailVerificationTokenExpiry: null }, + { + where: { + id: 'user-1', + emailVerificationToken: hashOtpToken('ABCDEF'), + emailVerificationTokenExpiry: 1_900_000_000_000, + }, + transaction: expect.anything(), + }, + ); + expect(user.save).toHaveBeenCalledWith({ transaction: expect.anything() }); + }); + + it('fails the email OTP when a concurrent verification already consumed it', async () => { + (User.update as any).mockResolvedValueOnce([0]); + const user = emailPending(); + + const result = await verifyEmailOTP(user as any, 'abcdef'); + + expect(result.verified).toBe(false); + expect(user.save).not.toHaveBeenCalled(); + expect(user.emailVerified).toBe(false); + expect(user.verified).toBe(false); + }); + + it('clears the phone OTP only where it still holds the compared issuance', async () => { + const user = phonePending(); + + await expect(verifyPhoneOTP(user as any, '123456')).resolves.toMatchObject({ + verified: true, + }); + + expect(User.update).toHaveBeenCalledWith( + { phoneVerificationToken: null, phoneVerificationTokenExpiry: null }, + { + where: { + id: 'user-1', + phoneVerificationToken: hashOtpToken('123456'), + phoneVerificationTokenExpiry: 1_900_000_000_000, + }, + transaction: expect.anything(), + }, + ); + expect(user.save).toHaveBeenCalledWith({ transaction: expect.anything() }); + }); + + it('fails the phone OTP when a concurrent verification already consumed it', async () => { + (User.update as any).mockResolvedValueOnce([0]); + const user = phonePending(); + + const result = await verifyPhoneOTP(user as any, '123456', 'attempt-a'); + + expect(result.verified).toBe(false); + expect(user.save).not.toHaveBeenCalled(); + expect(user.phoneVerified).toBe(false); + expect(user.phoneVerifiedAttemptId).toBeUndefined(); + }); + + it('throws when the conditional email OTP clear fails', async () => { + (User.update as any).mockRejectedValueOnce(new Error('db down')); + + await expect(verifyEmailOTP(emailPending() as any, 'abcdef')).rejects.toThrow( + 'Failed to update user verification via email OTP', + ); + }); + + it('throws when the conditional phone OTP clear fails', async () => { + (User.update as any).mockRejectedValueOnce(new Error('db down')); + + await expect(verifyPhoneOTP(phonePending() as any, '123456')).rejects.toThrow( + 'Failed to update user verification via phone OTP', + ); + }); +});