fix: make OTP attempt counting atomic to prevent TOCTOU race

Redis path: use Lua script for atomic read-increment-conditional-delete. DB path: use optimistic locking (UPDATE WHERE value = currentValue) with re-read fallback on conflict. Prevents concurrent wrong guesses from each counting as a single attempt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
simstudioai · waleedlatif1 · Mar 26, 2026 · Mar 26, 2026 · Mar 26, 2026 · Mar 26, 2026
commit 16072b5663ab06e2cd41100dc389715ba1f84139
diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts
@@ -11,6 +11,7 @@ const {
   mockRedisGet,
   mockRedisDel,
   mockRedisTtl,
+  mockRedisEval,
   mockGetRedisClient,
   mockRedisClient,
   mockDbSelect,
@@ -32,11 +33,13 @@ const {
   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 mockGetRedisClient = vi.fn()
   const mockDbSelect = vi.fn()
@@ -59,6 +62,7 @@ const {
     mockRedisGet,
     mockRedisDel,
     mockRedisTtl,
+    mockRedisEval,
     mockGetRedisClient,
     mockRedisClient,
     mockDbSelect,
@@ -573,9 +577,9 @@ describe('Chat OTP API Route', () => {
       mockGetStorageMethod.mockReturnValue('redis')
     })
 
-    it('should update stored value with incremented attempts on wrong OTP', async () => {
+    it('should atomically increment attempts on wrong OTP', async () => {
       mockRedisGet.mockResolvedValue('654321:0')
-      mockRedisTtl.mockResolvedValue(600)
+      mockRedisEval.mockResolvedValue('654321:1')
 
       mockDbSelect.mockImplementationOnce(() => ({
         from: vi.fn().mockReturnValue({
@@ -592,16 +596,18 @@ describe('Chat OTP API Route', () => {
 
       await PUT(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
 
-      expect(mockRedisSet).toHaveBeenCalledWith(
+      expect(mockRedisEval).toHaveBeenCalledWith(
+        expect.any(String),
+        1,
         `otp:${mockEmail}:${mockChatId}`,
-        '654321:1',
-        'KEEPTTL'
+        5
       )
       expect(mockCreateErrorResponse).toHaveBeenCalledWith('Invalid verification code', 400)
     })
 
-    it('should invalidate OTP and return 429 after 5 failed attempts', async () => {
+    it('should invalidate OTP and return 429 after max failed attempts', async () => {
       mockRedisGet.mockResolvedValue('654321:4')
+      mockRedisEval.mockResolvedValue('LOCKED')
 
       mockDbSelect.mockImplementationOnce(() => ({
         from: vi.fn().mockReturnValue({
@@ -618,7 +624,7 @@ describe('Chat OTP API Route', () => {
 
       await PUT(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
 
-      expect(mockRedisDel).toHaveBeenCalledWith(`otp:${mockEmail}:${mockChatId}`)
+      expect(mockRedisEval).toHaveBeenCalled()
       expect(mockCreateErrorResponse).toHaveBeenCalledWith(
         'Too many failed attempts. Please request a new code.',
         429

diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.ts b/apps/sim/app/api/chat/[identifier]/otp/route.ts
@@ -94,7 +94,40 @@ async function getOTP(email: string, chatId: string): Promise<string | null> {
   return record?.value ?? null
 }
 
-async function updateOTPValue(email: string, chatId: string, value: string): Promise<void> {
+/**
+ * Lua script for atomic OTP attempt increment.
+ * Returns: "LOCKED" if max attempts reached (key deleted), new encoded value otherwise, nil if key missing.
+ */
+const ATOMIC_INCREMENT_SCRIPT = `
+local val = redis.call('GET', KEYS[1])
+if not val then return nil end
+local colon = val:find(':([^:]*$)')
+local otp, attempts
+if colon then
+  otp = val:sub(1, colon - 1)
+  attempts = tonumber(val:sub(colon + 1)) or 0
+else
+  otp = val
+  attempts = 0
+end
+attempts = attempts + 1
+if attempts >= tonumber(ARGV[1]) then
+  redis.call('DEL', KEYS[1])
+  return 'LOCKED'
+end
+local newVal = otp .. ':' .. attempts
+redis.call('SET', KEYS[1], newVal, 'KEEPTTL')
+return newVal
+`
+
+/**
+ * Atomically increments OTP attempts. Returns 'locked' if max reached, 'incremented' otherwise.
+ */
+async function incrementOTPAttempts(
+  email: string,
+  chatId: string,
+  currentValue: string
+): Promise<'locked' | 'incremented'> {
   const identifier = `chat-otp:${chatId}:${email}`
   const storageMethod = getStorageMethod()
 
@@ -104,13 +137,35 @@ async function updateOTPValue(email: string, chatId: string, value: string): Pro
       throw new Error('Redis configured but client unavailable')
     }
     const key = `otp:${email}:${chatId}`
-    await redis.set(key, value, 'KEEPTTL')
-  } else {
-    await db
-      .update(verification)
-      .set({ value, updatedAt: new Date() })
-      .where(eq(verification.identifier, identifier))
+    const result = await redis.eval(ATOMIC_INCREMENT_SCRIPT, 1, key, MAX_OTP_ATTEMPTS)
+    return result === 'LOCKED' ? 'locked' : 'incremented'
+  }
+
+  // DB path: optimistic locking — only update if value hasn't changed since we read it
+  const { otp, attempts } = decodeOTPValue(currentValue)
+  const newAttempts = attempts + 1
+
+  if (newAttempts >= MAX_OTP_ATTEMPTS) {
+    await db.delete(verification).where(eq(verification.identifier, identifier))
+    return 'locked'
   }
+
+  const newValue = encodeOTPValue(otp, newAttempts)
+  const updated = await db
+    .update(verification)
+    .set({ value: newValue, updatedAt: new Date() })
+    .where(and(eq(verification.identifier, identifier), eq(verification.value, currentValue)))
+    .returning({ id: verification.id })
+
+  // If no rows updated, another request already incremented — re-read to check state
+  if (updated.length === 0) {
+    const fresh = await getOTP(email, chatId)
+    if (!fresh) return 'locked'
+    const { attempts: freshAttempts } = decodeOTPValue(fresh)
+    return freshAttempts >= MAX_OTP_ATTEMPTS ? 'locked' : 'incremented'
+  }
+
+  return 'incremented'
 }
 
 async function deleteOTP(email: string, chatId: string): Promise<void> {
@@ -257,18 +312,14 @@ export async function PUT(
     const { otp: storedOTP, attempts } = decodeOTPValue(storedValue)
 
     if (storedOTP !== otp) {
-      const newAttempts = attempts + 1
-      if (newAttempts >= MAX_OTP_ATTEMPTS) {
-        await deleteOTP(email, deployment.id)
-        logger.warn(
-          `[${requestId}] OTP invalidated after ${newAttempts} failed attempts for ${email}`
-        )
+      const result = await incrementOTPAttempts(email, deployment.id, storedValue)
+      if (result === 'locked') {
+        logger.warn(`[${requestId}] OTP invalidated after max failed attempts for ${email}`)
         return addCorsHeaders(
           createErrorResponse('Too many failed attempts. Please request a new code.', 429),
           request
         )
       }
-      await updateOTPValue(email, deployment.id, encodeOTPValue(storedOTP, newAttempts))
       return addCorsHeaders(createErrorResponse('Invalid verification code', 400), request)
     }