fix(otp): reserve a verification attempt atomically before comparing the code

apps/sim/app/api/chat/[identifier]/otp/route.integration.ts

@@ -0,0 +1,193 @@
+import { db } from '@sim/db'
+import { chat, user, verification, workflow, workspace } from '@sim/db/schema'
+import { readTestRedisUrl } from '@sim/db/testing/test-infrastructure'
+import { deleteWorkspaceFixture, insertWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
+import { createRouteContext } from '@sim/testing/helpers/http'
+import { createMockRequest } from '@sim/testing/mocks/request.mock'
+import { generateId } from '@sim/utils/id'
+import { eq, like } from 'drizzle-orm'
+import Redis from 'ioredis'
+import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { storage, redisClient } = vi.hoisted(() => ({
+  storage: { backend: 'database' as 'database' | 'redis' },
+  redisClient: { current: undefined as Redis | undefined },
+}))
+vi.mock('@/lib/core/storage', () => ({ getStorageMethod: () => storage.backend }))
+vi.mock('@/lib/core/config/redis', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('@/lib/core/config/redis')>()),
+  getRedisClient: () => redisClient.current,
+}))
+
+import { MAX_OTP_ATTEMPTS, storeOTP } from '@/lib/core/security/otp'
+import { PUT } from '@/app/api/chat/[identifier]/otp/route'
+
+const redisUrl = readTestRedisUrl()
+const ownerId = generateId()
+const workspaceId = generateId()
+const workflowId = generateId()
+const chatId = generateId()
+const identifier = `otp-${chatId}`
+const email = `${generateId()}@otp.invalid`
+const CODE = '246810'
+const NO_CODE = 'No verification code found, request a new one'
+const INVALID = 'Invalid verification code'
+const LOCKED = 'Too many failed attempts. Please request a new code.'
+
+async function verify(otp: string) {
+  const request = createMockRequest({
+    method: 'PUT',
+    url: `/api/chat/${identifier}/otp`,
+    body: { email, otp },
+  })
+  const response = await PUT(request, createRouteContext({ identifier }))
+  const body = (await response.json()) as { error?: string; message?: string }
+  return {
+    status: response.status,
+    error: body.error ?? body.message,
+    cookie: response.headers.get('set-cookie'),
+  }
+}
+
+/** Distinct wrong six-digit guesses that never equal {@link CODE}. */
+function wrongGuesses(count: number) {
+  return Array.from({ length: count }, (_, index) => String(100000 + index))
+}
+
+beforeAll(async () => {
+  await db.insert(user).values({
+    id: ownerId,
+    name: 'OTP fixture',
+    email: `${ownerId}@otp.invalid`,
+    emailVerified: true,
+    createdAt: new Date(),
+    updatedAt: new Date(),
+  })
+  await insertWorkspaceFixture(db, {
+    id: workspaceId,
+    name: 'OTP fixture',
+    ownerId,
+    billedAccountUserId: ownerId,
+  })
+  await db.insert(workflow).values({
+    id: workflowId,
+    userId: ownerId,
+    workspaceId,
+    name: 'OTP fixture',
+    lastSynced: new Date(),
+    createdAt: new Date(),
+    updatedAt: new Date(),
+  })
+  await db.insert(chat).values({
+    id: chatId,
+    workflowId,
+    userId: ownerId,
+    identifier,
+    title: 'OTP fixture',
+    authType: 'email',
+    allowedEmails: [email],
+  })
+})
+
+afterAll(async () => {
+  await db.delete(verification).where(like(verification.identifier, `chat-otp:${chatId}:%`))
+  await deleteWorkspaceFixture(db, eq(workspace.id, workspaceId))
+  await db.delete(user).where(eq(user.id, ownerId))
+  await redisClient.current?.quit()
+})
+
+describe.each(['database', 'redis'] as const)('%s chat OTP verification', (backend) => {
+  describe.runIf(backend === 'database' || Boolean(redisUrl))('real storage', () => {
+    beforeAll(async () => {
+      if (backend === 'redis' && redisUrl && !redisClient.current) {
+        redisClient.current = new Redis(redisUrl, { lazyConnect: true, maxRetriesPerRequest: 0 })
+        await redisClient.current.connect()
+      }
+    })
+    beforeEach(async () => {
+      storage.backend = backend
+      await storeOTP('chat', chatId, email, CODE)
+    })
+    afterEach(async () => {
+      await db
+        .update(chat)
+        .set({ authType: 'email', allowedEmails: [email] })
+        .where(eq(chat.id, chatId))
+    })
+
+    it.each([
+      {
+        rejection: 'the chat no longer uses email auth',
+        change: { authType: 'public' },
+        response: { status: 400, error: 'This chat does not use email authentication' },
+      },
+      {
+        rejection: 'the email is no longer allowed',
+        change: { allowedEmails: ['someone-else@otp.invalid'] },
+        response: { status: 403, error: 'Email not authorized' },
+      },
+    ])('rejects without spending the code when $rejection', async ({ change, response }) => {
+      await db.update(chat).set(change).where(eq(chat.id, chatId))
+      for (let attempt = 0; attempt < MAX_OTP_ATTEMPTS; attempt++) {
+        expect(await verify(CODE)).toMatchObject({ ...response, cookie: null })
+      }
+      await db
+        .update(chat)
+        .set({ authType: 'email', allowedEmails: [email] })
+        .where(eq(chat.id, chatId))
+      expect(await verify(CODE)).toMatchObject({ status: 200 })
+    })
+
+    it('compares at most the attempt limit of guesses however many arrive concurrently', async () => {
+      const results = await Promise.all(wrongGuesses(30).map(verify))
+
+      const compared = results.filter((result) => result.error !== NO_CODE)
+      expect(compared).toHaveLength(MAX_OTP_ATTEMPTS)
+      expect(compared.filter((result) => result.error === INVALID)).toHaveLength(
+        MAX_OTP_ATTEMPTS - 1
+      )
+      expect(compared.filter((result) => result.error === LOCKED)).toHaveLength(1)
+      expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
+    })
+
+    it('accepts the correct code on the last remaining attempt and mints the auth cookie', async () => {
+      for (const guess of wrongGuesses(MAX_OTP_ATTEMPTS - 1)) {
+        expect(await verify(guess)).toMatchObject({ status: 400, error: INVALID })
+      }
+      const accepted = await verify(CODE)
+      expect(accepted.status).toBe(200)
+      expect(accepted.cookie).toContain(`chat_auth_${chatId}=`)
+      expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE })
+    })
+
+    it('keeps the code expiry when a failed attempt is recorded', async () => {
+      if (backend === 'redis') {
+        const key = `otp:${email}:${chatId}`
+        await redisClient.current?.pexpire(key, 900)
+        expect(await verify('999999')).toMatchObject({ status: 400, error: INVALID })
+        const ttl = await redisClient.current?.pttl(key)
+        expect(ttl).toBeGreaterThan(0)
+        expect(ttl).toBeLessThanOrEqual(900)
+        return
+      }
+      const readExpiry = async () =>
+        (
+          await db
+            .select({ expiresAt: verification.expiresAt })
+            .from(verification)
+            .where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
+        )[0]?.expiresAt.getTime()
+      const before = await readExpiry()
+      expect(await verify('999999')).toMatchObject({ status: 400, error: INVALID })
+      expect(await readExpiry()).toBe(before)
+    })
+
+    it.runIf(backend === 'database')('rejects an expired code', async () => {
+      await db
+        .update(verification)
+        .set({ expiresAt: new Date(Date.now() - 1000) })
+        .where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
+      expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
+    })
+  })
+})

apps/sim/app/api/chat/[identifier]/otp/route.test.ts

@@ -24,10 +24,6 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
 
 const {
   mockRedisSet,
-  mockRedisGet,
-  mockRedisDel,
-  mockRedisTtl,
-  mockRedisEval,
   mockRedisClient,
   mockSetChatAuthCookie,
   mockIsEmailAllowed,
@@ -36,16 +32,8 @@ const {
   mockAfterResponse,
 } = vi.hoisted(() => {
   const mockRedisSet = vi.fn()
-  const mockRedisGet = vi.fn()
-  const mockRedisDel = vi.fn()
-  const mockRedisTtl = vi.fn()
-  const mockRedisEval = vi.fn()
   const mockRedisClient = {
     set: mockRedisSet,
-    get: mockRedisGet,
-    del: mockRedisDel,
-    ttl: mockRedisTtl,
-    eval: mockRedisEval,
   }
   const mockSetChatAuthCookie = vi.fn()
   const mockIsEmailAllowed = vi.fn((email: string, allowedEmails: string[]) => {
@@ -59,10 +47,6 @@ const {
 
   return {
     mockRedisSet,
-    mockRedisGet,
-    mockRedisDel,
-    mockRedisTtl,
-    mockRedisEval,
     mockRedisClient,
     mockSetChatAuthCookie,
     mockIsEmailAllowed,
@@ -139,7 +123,7 @@ vi.mock('zod', () => {
   }
 })
 
-import { PUT, POST as routePost } from './route'
+import { POST as routePost } from './route'
 
 const mockCheckRateLimitDirect = rateLimiterMockFns.mockCheckRateLimitDirect
 const { mockSendEmail } = emailMailerMockFns
@@ -156,7 +140,6 @@ describe('Chat OTP API Route', () => {
   const mockEmail = 'test@example.com'
   const mockChatId = 'chat-123'
   const mockIdentifier = 'test-chat'
-  const mockOTP = '123456'
 
   /** Queues the chat-deployment row the route reads before touching OTP storage. */
   const queueDeployment = (row: Record<string, unknown>) => {
@@ -183,9 +166,6 @@ describe('Chat OTP API Route', () => {
 
     mockGetRedisClient.mockReturnValue(mockRedisClient)
     mockRedisSet.mockResolvedValue('OK')
-    mockRedisGet.mockResolvedValue(null)
-    mockRedisDel.mockResolvedValue(1)
-    mockRedisTtl.mockResolvedValue(600)
 
     mockGetStorageMethod.mockReturnValue('redis')
 
@@ -312,126 +292,4 @@ describe('Chat OTP API Route', () => {
       )
     })
   })
-
-  describe('PUT - Verify OTP (authType re-check)', () => {
-    beforeEach(() => {
-      mockGetStorageMethod.mockReturnValue('redis')
-      mockRedisGet.mockResolvedValue(`${mockOTP}:0`)
-    })
-
-    it('rejects verification when the chat has switched away from email auth', async () => {
-      queueDeployment({
-        id: mockChatId,
-        authType: 'password',
-        password: 'encrypted-password',
-      })
-
-      const request = createMockRequest({
-        method: 'PUT',
-        url: 'http://localhost:3000/api/chat/test/otp',
-        rawBody: JSON.stringify({ email: mockEmail, otp: mockOTP }),
-      })
-
-      await PUT(request, createRouteContext({ identifier: mockIdentifier }))
-
-      expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith(
-        'This chat does not use email authentication',
-        400
-      )
-      expect(mockRedisGet).not.toHaveBeenCalled()
-      expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
-    })
-
-    it('rejects verification when the email is no longer allowed', async () => {
-      mockIsEmailAllowed.mockReturnValueOnce(false)
-      queueDeployment({ id: mockChatId, authType: 'email' })
-
-      const request = createMockRequest({
-        method: 'PUT',
-        url: 'http://localhost:3000/api/chat/test/otp',
-        rawBody: JSON.stringify({ email: mockEmail, otp: mockOTP }),
-      })
-
-      await PUT(request, createRouteContext({ identifier: mockIdentifier }))
-
-      expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith('Email not authorized', 403)
-      expect(mockRedisGet).not.toHaveBeenCalled()
-      expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
-    })
-  })
-
-  describe('PUT - Verify OTP (Database path)', () => {
-    beforeEach(() => {
-      mockGetStorageMethod.mockReturnValue('database')
-      mockGetRedisClient.mockReturnValue(null)
-    })
-
-    it('should reject expired OTP from database', async () => {
-      queueDeployment({ id: mockChatId, authType: 'email' })
-      queueTableRows(schemaMock.verification, [])
-
-      const request = createMockRequest({
-        method: 'PUT',
-        url: 'http://localhost:3000/api/chat/test/otp',
-        rawBody: JSON.stringify({ email: mockEmail, otp: mockOTP }),
-      })
-
-      await PUT(request, createRouteContext({ identifier: mockIdentifier }))
-
-      expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith(
-        'No verification code found, request a new one',
-        400
-      )
-    })
-  })
-
-  describe('Brute-force protection', () => {
-    beforeEach(() => {
-      mockGetStorageMethod.mockReturnValue('redis')
-    })
-
-    it('should atomically increment attempts on wrong OTP', async () => {
-      mockRedisGet.mockResolvedValue('654321:0')
-      mockRedisEval.mockResolvedValue('654321:1')
-
-      queueDeployment({ id: mockChatId, authType: 'email' })
-
-      const request = createMockRequest({
-        method: 'PUT',
-        url: 'http://localhost:3000/api/chat/test/otp',
-        rawBody: JSON.stringify({ email: mockEmail, otp: 'wrong1' }),
-      })
-
-      await PUT(request, createRouteContext({ identifier: mockIdentifier }))
-
-      expect(mockRedisEval).toHaveBeenCalledWith(
-        expect.any(String),
-        1,
-        `otp:${mockEmail}:${mockChatId}`,
-        5
-      )
-      expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith('Invalid verification code', 400)
-    })
-
-    it('should invalidate OTP and return 429 after max failed attempts', async () => {
-      mockRedisGet.mockResolvedValue('654321:4')
-      mockRedisEval.mockResolvedValue('LOCKED')
-
-      queueDeployment({ id: mockChatId, authType: 'email' })
-
-      const request = createMockRequest({
-        method: 'PUT',
-        url: 'http://localhost:3000/api/chat/test/otp',
-        rawBody: JSON.stringify({ email: mockEmail, otp: 'wrong5' }),
-      })
-
-      await PUT(request, createRouteContext({ identifier: mockIdentifier }))
-
-      expect(mockRedisEval).toHaveBeenCalled()
-      expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith(
-        'Too many failed attempts. Please request a new code.',
-        429
-      )
-    })
-  })
 })