Skip to content

Commit 5daef70

Browse files
committed
fix(otp): reserve a verification attempt atomically before comparing the code
Chat and public file OTP verify read the attempt count, compared the guess, and only then incremented, so concurrent submissions all passed the limit check on the same stale count and every one was compared. Reserve the attempt first (Redis Lua script / one conditional Postgres UPDATE) and compare only against the reserved value, so at most MAX_OTP_ATTEMPTS guesses are ever checked against one code. Chat verify now uses safeCompare like the file path.
1 parent 472399b commit 5daef70

6 files changed

Lines changed: 225 additions & 166 deletions

File tree

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
import { db } from '@sim/db'
2+
import { chat, user, verification, workflow, workspace } from '@sim/db/schema'
3+
import { readTestRedisUrl } from '@sim/db/testing/test-infrastructure'
4+
import { deleteWorkspaceFixture, insertWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
5+
import { createRouteContext } from '@sim/testing/helpers/http'
6+
import { generateId } from '@sim/utils/id'
7+
import { eq, like } from 'drizzle-orm'
8+
import Redis from 'ioredis'
9+
import { NextRequest } from 'next/server'
10+
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
11+
12+
const { storage, redisClient } = vi.hoisted(() => ({
13+
storage: { backend: 'database' as 'database' | 'redis' },
14+
redisClient: { current: undefined as Redis | undefined },
15+
}))
16+
vi.mock('@/lib/core/storage', () => ({ getStorageMethod: () => storage.backend }))
17+
vi.mock('@/lib/core/config/redis', async (importOriginal) => ({
18+
...(await importOriginal<typeof import('@/lib/core/config/redis')>()),
19+
getRedisClient: () => redisClient.current,
20+
}))
21+
22+
import { MAX_OTP_ATTEMPTS, storeOTP } from '@/lib/core/security/otp'
23+
import { PUT } from '@/app/api/chat/[identifier]/otp/route'
24+
25+
const redisUrl = readTestRedisUrl()
26+
const ownerId = generateId()
27+
const workspaceId = generateId()
28+
const workflowId = generateId()
29+
const chatId = generateId()
30+
const identifier = `otp-${chatId}`
31+
const email = `${generateId()}@otp.invalid`
32+
const CODE = '246810'
33+
const NO_CODE = 'No verification code found, request a new one'
34+
const INVALID = 'Invalid verification code'
35+
const LOCKED = 'Too many failed attempts. Please request a new code.'
36+
37+
async function verify(otp: string) {
38+
const request = new NextRequest(`http://localhost:3000/api/chat/${identifier}/otp`, {
39+
method: 'PUT',
40+
headers: { 'content-type': 'application/json' },
41+
body: JSON.stringify({ email, otp }),
42+
})
43+
const response = await PUT(request, createRouteContext({ identifier }))
44+
const body = (await response.json()) as { error?: string; message?: string }
45+
return {
46+
status: response.status,
47+
error: body.error ?? body.message,
48+
cookie: response.headers.get('set-cookie'),
49+
}
50+
}
51+
52+
/** Distinct wrong six-digit guesses that never equal {@link CODE}. */
53+
function wrongGuesses(count: number) {
54+
return Array.from({ length: count }, (_, index) => String(100000 + index))
55+
}
56+
57+
beforeAll(async () => {
58+
await db.insert(user).values({
59+
id: ownerId,
60+
name: 'OTP fixture',
61+
email: `${ownerId}@otp.invalid`,
62+
emailVerified: true,
63+
createdAt: new Date(),
64+
updatedAt: new Date(),
65+
})
66+
await insertWorkspaceFixture(db, {
67+
id: workspaceId,
68+
name: 'OTP fixture',
69+
ownerId,
70+
billedAccountUserId: ownerId,
71+
})
72+
await db.insert(workflow).values({
73+
id: workflowId,
74+
userId: ownerId,
75+
workspaceId,
76+
name: 'OTP fixture',
77+
lastSynced: new Date(),
78+
createdAt: new Date(),
79+
updatedAt: new Date(),
80+
})
81+
await db.insert(chat).values({
82+
id: chatId,
83+
workflowId,
84+
userId: ownerId,
85+
identifier,
86+
title: 'OTP fixture',
87+
authType: 'email',
88+
allowedEmails: [email],
89+
})
90+
})
91+
92+
afterAll(async () => {
93+
await db.delete(verification).where(like(verification.identifier, `chat-otp:${chatId}:%`))
94+
await deleteWorkspaceFixture(db, eq(workspace.id, workspaceId))
95+
await db.delete(user).where(eq(user.id, ownerId))
96+
await redisClient.current?.quit()
97+
})
98+
99+
describe.each(['database', 'redis'] as const)('%s chat OTP verification', (backend) => {
100+
describe.runIf(backend === 'database' || Boolean(redisUrl))('real storage', () => {
101+
beforeAll(async () => {
102+
if (backend === 'redis' && !redisClient.current) {
103+
redisClient.current = new Redis(redisUrl!, { lazyConnect: true, maxRetriesPerRequest: 0 })
104+
await redisClient.current.connect()
105+
}
106+
})
107+
beforeEach(async () => {
108+
storage.backend = backend
109+
await storeOTP('chat', chatId, email, CODE)
110+
})
111+
112+
it('compares at most the attempt limit of guesses however many arrive concurrently', async () => {
113+
const results = await Promise.all(wrongGuesses(30).map(verify))
114+
115+
const compared = results.filter((result) => result.error !== NO_CODE)
116+
expect(compared).toHaveLength(MAX_OTP_ATTEMPTS)
117+
expect(compared.filter((result) => result.error === INVALID)).toHaveLength(
118+
MAX_OTP_ATTEMPTS - 1
119+
)
120+
expect(compared.filter((result) => result.error === LOCKED)).toHaveLength(1)
121+
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
122+
})
123+
124+
it('accepts the correct code on the last remaining attempt and mints the auth cookie', async () => {
125+
for (const guess of wrongGuesses(MAX_OTP_ATTEMPTS - 1)) {
126+
expect(await verify(guess)).toMatchObject({ status: 400, error: INVALID })
127+
}
128+
const accepted = await verify(CODE)
129+
expect(accepted.status).toBe(200)
130+
expect(accepted.cookie).toContain(`chat_auth_${chatId}=`)
131+
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE })
132+
})
133+
134+
it('keeps the code expiry when a failed attempt is recorded', async () => {
135+
const readExpiry = async () =>
136+
backend === 'redis'
137+
? redisClient.current?.ttl(`otp:${email}:${chatId}`)
138+
: (
139+
await db
140+
.select({ expiresAt: verification.expiresAt })
141+
.from(verification)
142+
.where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
143+
)[0]?.expiresAt.getTime()
144+
const before = await readExpiry()
145+
expect(await verify('999999')).toMatchObject({ status: 400, error: INVALID })
146+
const after = await readExpiry()
147+
if (backend === 'redis') expect(after).toBeGreaterThan(0)
148+
else expect(after).toBe(before)
149+
})
150+
151+
it.runIf(backend === 'database')('rejects an expired code', async () => {
152+
await db
153+
.update(verification)
154+
.set({ expiresAt: new Date(Date.now() - 1000) })
155+
.where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
156+
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
157+
})
158+
})
159+
})

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

Lines changed: 3 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ describe('Chat OTP API Route', () => {
316316
describe('PUT - Verify OTP (authType re-check)', () => {
317317
beforeEach(() => {
318318
mockGetStorageMethod.mockReturnValue('redis')
319-
mockRedisGet.mockResolvedValue(`${mockOTP}:0`)
319+
mockRedisEval.mockResolvedValue(`${mockOTP}:1`)
320320
})
321321

322322
it('rejects verification when the chat has switched away from email auth', async () => {
@@ -338,7 +338,7 @@ describe('Chat OTP API Route', () => {
338338
'This chat does not use email authentication',
339339
400
340340
)
341-
expect(mockRedisGet).not.toHaveBeenCalled()
341+
expect(mockRedisEval).not.toHaveBeenCalled()
342342
expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
343343
})
344344

@@ -355,83 +355,8 @@ describe('Chat OTP API Route', () => {
355355
await PUT(request, createRouteContext({ identifier: mockIdentifier }))
356356

357357
expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith('Email not authorized', 403)
358-
expect(mockRedisGet).not.toHaveBeenCalled()
358+
expect(mockRedisEval).not.toHaveBeenCalled()
359359
expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
360360
})
361361
})
362-
363-
describe('PUT - Verify OTP (Database path)', () => {
364-
beforeEach(() => {
365-
mockGetStorageMethod.mockReturnValue('database')
366-
mockGetRedisClient.mockReturnValue(null)
367-
})
368-
369-
it('should reject expired OTP from database', async () => {
370-
queueDeployment({ id: mockChatId, authType: 'email' })
371-
queueTableRows(schemaMock.verification, [])
372-
373-
const request = createMockRequest({
374-
method: 'PUT',
375-
url: 'http://localhost:3000/api/chat/test/otp',
376-
rawBody: JSON.stringify({ email: mockEmail, otp: mockOTP }),
377-
})
378-
379-
await PUT(request, createRouteContext({ identifier: mockIdentifier }))
380-
381-
expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith(
382-
'No verification code found, request a new one',
383-
400
384-
)
385-
})
386-
})
387-
388-
describe('Brute-force protection', () => {
389-
beforeEach(() => {
390-
mockGetStorageMethod.mockReturnValue('redis')
391-
})
392-
393-
it('should atomically increment attempts on wrong OTP', async () => {
394-
mockRedisGet.mockResolvedValue('654321:0')
395-
mockRedisEval.mockResolvedValue('654321:1')
396-
397-
queueDeployment({ id: mockChatId, authType: 'email' })
398-
399-
const request = createMockRequest({
400-
method: 'PUT',
401-
url: 'http://localhost:3000/api/chat/test/otp',
402-
rawBody: JSON.stringify({ email: mockEmail, otp: 'wrong1' }),
403-
})
404-
405-
await PUT(request, createRouteContext({ identifier: mockIdentifier }))
406-
407-
expect(mockRedisEval).toHaveBeenCalledWith(
408-
expect.any(String),
409-
1,
410-
`otp:${mockEmail}:${mockChatId}`,
411-
5
412-
)
413-
expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith('Invalid verification code', 400)
414-
})
415-
416-
it('should invalidate OTP and return 429 after max failed attempts', async () => {
417-
mockRedisGet.mockResolvedValue('654321:4')
418-
mockRedisEval.mockResolvedValue('LOCKED')
419-
420-
queueDeployment({ id: mockChatId, authType: 'email' })
421-
422-
const request = createMockRequest({
423-
method: 'PUT',
424-
url: 'http://localhost:3000/api/chat/test/otp',
425-
rawBody: JSON.stringify({ email: mockEmail, otp: 'wrong5' }),
426-
})
427-
428-
await PUT(request, createRouteContext({ identifier: mockIdentifier }))
429-
430-
expect(mockRedisEval).toHaveBeenCalled()
431-
expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith(
432-
'Too many failed attempts. Please request a new code.',
433-
429
434-
)
435-
})
436-
})
437362
})

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

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { db } from '@sim/db'
22
import { chat } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
4+
import { safeCompare } from '@sim/security/compare'
45
import { normalizeEmail } from '@sim/utils/string'
56
import { and, eq, isNull } from 'drizzle-orm'
67
import type { NextRequest } from 'next/server'
@@ -10,15 +11,13 @@ import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
1011
import { RateLimiter } from '@/lib/core/rate-limiter'
1112
import { isEmailAllowed } from '@/lib/core/security/deployment'
1213
import {
13-
decodeOTPValue,
1414
deleteOTP,
1515
generateOTP,
16-
getOTP,
17-
incrementOTPAttempts,
1816
MAX_OTP_ATTEMPTS,
1917
OTP_EMAIL_RATE_LIMIT,
2018
OTP_IP_RATE_LIMIT,
2119
OTP_RESOURCE_RATE_LIMIT,
20+
reserveOTPAttempt,
2221
storeOTP,
2322
} from '@/lib/core/security/otp'
2423
import { afterResponse } from '@/lib/core/utils/after-response'
@@ -196,22 +195,13 @@ export const PUT = withRouteHandler(
196195
return createCodedErrorResponse('Email not authorized', 403)
197196
}
198197

199-
const storedValue = await getOTP('chat', deployment.id, email)
200-
if (!storedValue) {
198+
const reserved = await reserveOTPAttempt('chat', deployment.id, email)
199+
if (!reserved) {
201200
return createCodedErrorResponse('No verification code found, request a new one', 400)
202201
}
203202

204-
const { otp: storedOTP, attempts } = decodeOTPValue(storedValue)
205-
206-
if (attempts >= MAX_OTP_ATTEMPTS) {
207-
await deleteOTP('chat', deployment.id, email)
208-
logger.warn(`OTP already locked out for ${email}`)
209-
return createCodedErrorResponse('Too many failed attempts. Please request a new code.', 429)
210-
}
211-
212-
if (storedOTP !== otp) {
213-
const result = await incrementOTPAttempts('chat', deployment.id, email, storedValue)
214-
if (result === 'locked') {
203+
if (!safeCompare(reserved.otp, otp)) {
204+
if (reserved.attempts >= MAX_OTP_ATTEMPTS) {
215205
logger.warn(`OTP invalidated after max failed attempts for ${email}`)
216206
return createCodedErrorResponse(
217207
'Too many failed attempts. Please request a new code.',

0 commit comments

Comments
 (0)