Skip to content

Commit 5123abb

Browse files
waleedlatif1waleedlatif1
authored
fix(otp): reserve a verification attempt atomically before comparing the code (#9774)
* 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. * improvement(otp): centralize compare-and-consume in verifyOTP, tighten tests Move the constant-time compare and consume-on-match into verifyOTP so both OTP callers share it; drop getOTP and the decodeOTPValue export now that nothing reads a code without reserving an attempt; remove the unreachable Lua branch; keep the lockout log free of the email address. Tests: assert the Redis TTL is preserved rather than reset, drop the non-null assertion, and use createMockRequest. * fix(otp): keep sub-second code lifetimes and drop unused deleteOTP export TTL rounds a key with under a second left to 0, so the reservation script rewrote it without an expiry and the code never expired. Read PTTL and rewrite with PX, deleting a code with no remaining lifetime. deleteOTP is now only used inside the module. * test(otp): prove rejected verify requests never spend the code against real storage Replace the mock-asserting authorization unit tests with integration cases: with email auth disabled or the address removed from the allow list, five requests carrying the correct code are refused without a cookie, and the code still works once the chat is restored.
1 parent 13c5924 commit 5123abb

6 files changed

Lines changed: 288 additions & 282 deletions

File tree

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

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
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 { createMockRequest } from '@sim/testing/mocks/request.mock'
7+
import { generateId } from '@sim/utils/id'
8+
import { eq, like } from 'drizzle-orm'
9+
import Redis from 'ioredis'
10+
import { afterAll, afterEach, 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 = createMockRequest({
39+
method: 'PUT',
40+
url: `/api/chat/${identifier}/otp`,
41+
body: { 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' && redisUrl && !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+
afterEach(async () => {
112+
await db
113+
.update(chat)
114+
.set({ authType: 'email', allowedEmails: [email] })
115+
.where(eq(chat.id, chatId))
116+
})
117+
118+
it.each([
119+
{
120+
rejection: 'the chat no longer uses email auth',
121+
change: { authType: 'public' },
122+
response: { status: 400, error: 'This chat does not use email authentication' },
123+
},
124+
{
125+
rejection: 'the email is no longer allowed',
126+
change: { allowedEmails: ['someone-else@otp.invalid'] },
127+
response: { status: 403, error: 'Email not authorized' },
128+
},
129+
])('rejects without spending the code when $rejection', async ({ change, response }) => {
130+
await db.update(chat).set(change).where(eq(chat.id, chatId))
131+
for (let attempt = 0; attempt < MAX_OTP_ATTEMPTS; attempt++) {
132+
expect(await verify(CODE)).toMatchObject({ ...response, cookie: null })
133+
}
134+
await db
135+
.update(chat)
136+
.set({ authType: 'email', allowedEmails: [email] })
137+
.where(eq(chat.id, chatId))
138+
expect(await verify(CODE)).toMatchObject({ status: 200 })
139+
})
140+
141+
it('compares at most the attempt limit of guesses however many arrive concurrently', async () => {
142+
const results = await Promise.all(wrongGuesses(30).map(verify))
143+
144+
const compared = results.filter((result) => result.error !== NO_CODE)
145+
expect(compared).toHaveLength(MAX_OTP_ATTEMPTS)
146+
expect(compared.filter((result) => result.error === INVALID)).toHaveLength(
147+
MAX_OTP_ATTEMPTS - 1
148+
)
149+
expect(compared.filter((result) => result.error === LOCKED)).toHaveLength(1)
150+
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
151+
})
152+
153+
it('accepts the correct code on the last remaining attempt and mints the auth cookie', async () => {
154+
for (const guess of wrongGuesses(MAX_OTP_ATTEMPTS - 1)) {
155+
expect(await verify(guess)).toMatchObject({ status: 400, error: INVALID })
156+
}
157+
const accepted = await verify(CODE)
158+
expect(accepted.status).toBe(200)
159+
expect(accepted.cookie).toContain(`chat_auth_${chatId}=`)
160+
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE })
161+
})
162+
163+
it('keeps the code expiry when a failed attempt is recorded', async () => {
164+
if (backend === 'redis') {
165+
const key = `otp:${email}:${chatId}`
166+
await redisClient.current?.pexpire(key, 900)
167+
expect(await verify('999999')).toMatchObject({ status: 400, error: INVALID })
168+
const ttl = await redisClient.current?.pttl(key)
169+
expect(ttl).toBeGreaterThan(0)
170+
expect(ttl).toBeLessThanOrEqual(900)
171+
return
172+
}
173+
const readExpiry = async () =>
174+
(
175+
await db
176+
.select({ expiresAt: verification.expiresAt })
177+
.from(verification)
178+
.where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
179+
)[0]?.expiresAt.getTime()
180+
const before = await readExpiry()
181+
expect(await verify('999999')).toMatchObject({ status: 400, error: INVALID })
182+
expect(await readExpiry()).toBe(before)
183+
})
184+
185+
it.runIf(backend === 'database')('rejects an expired code', async () => {
186+
await db
187+
.update(verification)
188+
.set({ expiresAt: new Date(Date.now() - 1000) })
189+
.where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
190+
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
191+
})
192+
})
193+
})

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

Lines changed: 1 addition & 143 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,6 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
2424

2525
const {
2626
mockRedisSet,
27-
mockRedisGet,
28-
mockRedisDel,
29-
mockRedisTtl,
30-
mockRedisEval,
3127
mockRedisClient,
3228
mockSetChatAuthCookie,
3329
mockIsEmailAllowed,
@@ -36,16 +32,8 @@ const {
3632
mockAfterResponse,
3733
} = vi.hoisted(() => {
3834
const mockRedisSet = vi.fn()
39-
const mockRedisGet = vi.fn()
40-
const mockRedisDel = vi.fn()
41-
const mockRedisTtl = vi.fn()
42-
const mockRedisEval = vi.fn()
4335
const mockRedisClient = {
4436
set: mockRedisSet,
45-
get: mockRedisGet,
46-
del: mockRedisDel,
47-
ttl: mockRedisTtl,
48-
eval: mockRedisEval,
4937
}
5038
const mockSetChatAuthCookie = vi.fn()
5139
const mockIsEmailAllowed = vi.fn((email: string, allowedEmails: string[]) => {
@@ -59,10 +47,6 @@ const {
5947

6048
return {
6149
mockRedisSet,
62-
mockRedisGet,
63-
mockRedisDel,
64-
mockRedisTtl,
65-
mockRedisEval,
6650
mockRedisClient,
6751
mockSetChatAuthCookie,
6852
mockIsEmailAllowed,
@@ -139,7 +123,7 @@ vi.mock('zod', () => {
139123
}
140124
})
141125

142-
import { PUT, POST as routePost } from './route'
126+
import { POST as routePost } from './route'
143127

144128
const mockCheckRateLimitDirect = rateLimiterMockFns.mockCheckRateLimitDirect
145129
const { mockSendEmail } = emailMailerMockFns
@@ -156,7 +140,6 @@ describe('Chat OTP API Route', () => {
156140
const mockEmail = 'test@example.com'
157141
const mockChatId = 'chat-123'
158142
const mockIdentifier = 'test-chat'
159-
const mockOTP = '123456'
160143

161144
/** Queues the chat-deployment row the route reads before touching OTP storage. */
162145
const queueDeployment = (row: Record<string, unknown>) => {
@@ -183,9 +166,6 @@ describe('Chat OTP API Route', () => {
183166

184167
mockGetRedisClient.mockReturnValue(mockRedisClient)
185168
mockRedisSet.mockResolvedValue('OK')
186-
mockRedisGet.mockResolvedValue(null)
187-
mockRedisDel.mockResolvedValue(1)
188-
mockRedisTtl.mockResolvedValue(600)
189169

190170
mockGetStorageMethod.mockReturnValue('redis')
191171

@@ -312,126 +292,4 @@ describe('Chat OTP API Route', () => {
312292
)
313293
})
314294
})
315-
316-
describe('PUT - Verify OTP (authType re-check)', () => {
317-
beforeEach(() => {
318-
mockGetStorageMethod.mockReturnValue('redis')
319-
mockRedisGet.mockResolvedValue(`${mockOTP}:0`)
320-
})
321-
322-
it('rejects verification when the chat has switched away from email auth', async () => {
323-
queueDeployment({
324-
id: mockChatId,
325-
authType: 'password',
326-
password: 'encrypted-password',
327-
})
328-
329-
const request = createMockRequest({
330-
method: 'PUT',
331-
url: 'http://localhost:3000/api/chat/test/otp',
332-
rawBody: JSON.stringify({ email: mockEmail, otp: mockOTP }),
333-
})
334-
335-
await PUT(request, createRouteContext({ identifier: mockIdentifier }))
336-
337-
expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith(
338-
'This chat does not use email authentication',
339-
400
340-
)
341-
expect(mockRedisGet).not.toHaveBeenCalled()
342-
expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
343-
})
344-
345-
it('rejects verification when the email is no longer allowed', async () => {
346-
mockIsEmailAllowed.mockReturnValueOnce(false)
347-
queueDeployment({ id: mockChatId, authType: 'email' })
348-
349-
const request = createMockRequest({
350-
method: 'PUT',
351-
url: 'http://localhost:3000/api/chat/test/otp',
352-
rawBody: JSON.stringify({ email: mockEmail, otp: mockOTP }),
353-
})
354-
355-
await PUT(request, createRouteContext({ identifier: mockIdentifier }))
356-
357-
expect(mockCreateCodedErrorResponse).toHaveBeenCalledWith('Email not authorized', 403)
358-
expect(mockRedisGet).not.toHaveBeenCalled()
359-
expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
360-
})
361-
})
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-
})
437295
})

0 commit comments

Comments
 (0)