Skip to content

Commit d831c09

Browse files
TheodoreSpeaksTheodoreSpeaks
authored
fix(security): isolate rejected OTP attempts (#7008)
* fix(security): isolate rejected OTP attempts * fix(security): make OTP requests non-enumerating * fix(security): defer OTP delivery work
1 parent 33feaa1 commit d831c09

5 files changed

Lines changed: 204 additions & 133 deletions

File tree

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

Lines changed: 43 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ const {
3131
mockSetChatAuthCookie,
3232
mockGetStorageMethod,
3333
mockZodParse,
34+
mockAfterResponse,
3435
} = vi.hoisted(() => {
3536
const mockRedisSet = vi.fn()
3637
const mockRedisGet = vi.fn()
@@ -49,6 +50,7 @@ const {
4950
const mockSetChatAuthCookie = vi.fn()
5051
const mockGetStorageMethod = vi.fn()
5152
const mockZodParse = vi.fn()
53+
const mockAfterResponse = vi.fn()
5254

5355
return {
5456
mockRedisSet,
@@ -62,6 +64,7 @@ const {
6264
mockSetChatAuthCookie,
6365
mockGetStorageMethod,
6466
mockZodParse,
67+
mockAfterResponse,
6568
}
6669
})
6770

@@ -84,6 +87,10 @@ vi.mock('@/lib/core/rate-limiter', () => ({
8487
},
8588
}))
8689

90+
vi.mock('@/lib/core/utils/after-response', () => ({
91+
afterResponse: mockAfterResponse,
92+
}))
93+
8794
vi.mock('@/lib/messaging/email/mailer', () => ({
8895
sendEmail: mockSendEmail,
8996
}))
@@ -149,7 +156,14 @@ vi.mock('zod', () => {
149156
}
150157
})
151158

152-
import { POST, PUT } from './route'
159+
import { PUT, POST as routePost } from './route'
160+
161+
const POST: typeof routePost = async (...args) => {
162+
const response = await routePost(...args)
163+
const task = mockAfterResponse.mock.calls.at(-1)?.[0] as (() => Promise<void>) | undefined
164+
if (task) await task()
165+
return response
166+
}
153167

154168
describe('Chat OTP API Route', () => {
155169
const mockEmail = 'test@example.com'
@@ -209,7 +223,6 @@ describe('Chat OTP API Route', () => {
209223
remaining: 10,
210224
resetAt: new Date(Date.now() + 60_000),
211225
})
212-
213226
mockZodParse.mockImplementation((data: unknown) => data)
214227

215228
setEnv({ NEXT_PUBLIC_APP_URL: 'http://localhost:3000', NODE_ENV: 'test' })
@@ -252,6 +265,27 @@ describe('Chat OTP API Route', () => {
252265
})
253266

254267
describe('POST - Rate limiting', () => {
268+
it('returns the generic acceptance response for a rejected email without a client IP', async () => {
269+
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
270+
queueDeployment(emailDeployment)
271+
272+
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
273+
method: 'POST',
274+
body: JSON.stringify({ email: 'not-allowed@example.com' }),
275+
})
276+
277+
const response = await POST(request, {
278+
params: Promise.resolve({ identifier: mockIdentifier }),
279+
})
280+
281+
expect(response.status).toBe(200)
282+
await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' })
283+
expect(mockAfterResponse).toHaveBeenCalledTimes(1)
284+
expect(mockCheckRateLimitDirect).not.toHaveBeenCalled()
285+
expect(mockRedisSet).not.toHaveBeenCalled()
286+
expect(mockSendEmail).not.toHaveBeenCalled()
287+
})
288+
255289
it('returns 429 with Retry-After when IP rate limit is exceeded', async () => {
256290
mockCheckRateLimitDirect.mockResolvedValueOnce({
257291
allowed: false,
@@ -282,7 +316,7 @@ describe('Chat OTP API Route', () => {
282316
expect(dbChainMockFns.select).not.toHaveBeenCalled()
283317
})
284318

285-
it('returns 429 with Retry-After when email rate limit is exceeded', async () => {
319+
it('returns the generic acceptance response when the email rate limit is exceeded', async () => {
286320
mockCheckRateLimitDirect
287321
.mockResolvedValueOnce({
288322
allowed: true,
@@ -301,13 +335,6 @@ describe('Chat OTP API Route', () => {
301335
retryAfterMs: 900_000,
302336
})
303337

304-
const headerSet = vi.fn()
305-
mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({
306-
json: () => Promise.resolve({ error: message }),
307-
status,
308-
headers: { set: headerSet },
309-
}))
310-
311338
queueDeployment(emailDeployment)
312339

313340
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
@@ -319,12 +346,12 @@ describe('Chat OTP API Route', () => {
319346
params: Promise.resolve({ identifier: mockIdentifier }),
320347
})
321348

322-
expect(response.status).toBe(429)
323-
expect(headerSet).toHaveBeenCalledWith('Retry-After', '900')
349+
expect(response.status).toBe(200)
350+
await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' })
324351
expect(mockSendEmail).not.toHaveBeenCalled()
325352
})
326353

327-
it('returns 429 with Retry-After when the chat resource rate limit is exceeded', async () => {
354+
it('returns the generic acceptance response when the chat resource limit is exceeded', async () => {
328355
mockCheckRateLimitDirect
329356
.mockResolvedValueOnce({
330357
allowed: true,
@@ -338,13 +365,6 @@ describe('Chat OTP API Route', () => {
338365
retryAfterMs: 900_000,
339366
})
340367

341-
const headerSet = vi.fn()
342-
mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({
343-
json: () => Promise.resolve({ error: message }),
344-
status,
345-
headers: { set: headerSet },
346-
}))
347-
348368
queueDeployment(emailDeployment)
349369

350370
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
@@ -356,8 +376,8 @@ describe('Chat OTP API Route', () => {
356376
params: Promise.resolve({ identifier: mockIdentifier }),
357377
})
358378

359-
expect(response.status).toBe(429)
360-
expect(headerSet).toHaveBeenCalledWith('Retry-After', '900')
379+
expect(response.status).toBe(200)
380+
await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' })
361381
expect(mockSendEmail).not.toHaveBeenCalled()
362382
})
363383

@@ -396,6 +416,7 @@ describe('Chat OTP API Route', () => {
396416

397417
await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
398418

419+
expect(mockAfterResponse).toHaveBeenCalledTimes(1)
399420
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2)
400421
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
401422
1,

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

Lines changed: 49 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
OTP_RESOURCE_RATE_LIMIT,
2121
storeOTP,
2222
} from '@/lib/core/security/otp'
23+
import { afterResponse } from '@/lib/core/utils/after-response'
2324
import { generateRequestId, getClientIp } from '@/lib/core/utils/request'
2425
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
2526
import { sendEmail } from '@/lib/messaging/email/mailer'
@@ -30,6 +31,49 @@ const logger = createLogger('ChatOtpAPI')
3031

3132
const rateLimiter = new RateLimiter()
3233

34+
function otpRequestAccepted() {
35+
return createSuccessResponse({ message: 'Verification code sent' })
36+
}
37+
38+
async function deliverOtp(requestId: string, deploymentId: string, title: string, email: string) {
39+
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
40+
`chat-otp:resource:${deploymentId}`,
41+
OTP_RESOURCE_RATE_LIMIT,
42+
{ failClosed: true }
43+
)
44+
if (!resourceRateLimit.allowed) {
45+
logger.warn(`[${requestId}] OTP resource rate limit exceeded for chat ${deploymentId}`)
46+
return
47+
}
48+
49+
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
50+
`chat-otp:email:${deploymentId}:${email.toLowerCase()}`,
51+
OTP_EMAIL_RATE_LIMIT,
52+
{ failClosed: true }
53+
)
54+
if (!emailRateLimit.allowed) {
55+
logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deploymentId}`)
56+
return
57+
}
58+
59+
const otp = generateOTP()
60+
await storeOTP('chat', deploymentId, email, otp)
61+
62+
const emailHtml = await renderOTPEmail(otp, email, 'email-verification', title)
63+
const emailResult = await sendEmail({
64+
to: email,
65+
subject: getOtpSubject(title),
66+
html: emailHtml,
67+
})
68+
69+
if (!emailResult.success) {
70+
logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message)
71+
return
72+
}
73+
74+
logger.info(`[${requestId}] OTP sent to ${email} for chat ${deploymentId}`)
75+
}
76+
3377
export const POST = withRouteHandler(
3478
async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => {
3579
const { identifier } = await context.params
@@ -88,72 +132,13 @@ export const POST = withRouteHandler(
88132
const allowedEmails: string[] = Array.isArray(deployment.allowedEmails)
89133
? deployment.allowedEmails
90134
: []
135+
const emailAllowed = isEmailAllowed(email, allowedEmails)
91136

92-
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
93-
`chat-otp:resource:${deployment.id}`,
94-
OTP_RESOURCE_RATE_LIMIT,
95-
{ failClosed: true }
96-
)
97-
if (!resourceRateLimit.allowed) {
98-
logger.warn(`[${requestId}] OTP resource rate limit exceeded for chat ${deployment.id}`)
99-
const retryAfter = Math.ceil(
100-
(resourceRateLimit.retryAfterMs ?? OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) / 1000
101-
)
102-
const response = createErrorResponse(
103-
'Too many verification code requests. Please try again later.',
104-
429
105-
)
106-
response.headers.set('Retry-After', String(retryAfter))
107-
return response
108-
}
109-
110-
if (!isEmailAllowed(email, allowedEmails)) {
111-
return createErrorResponse('Email not authorized for this chat', 403)
112-
}
113-
114-
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
115-
`chat-otp:email:${deployment.id}:${email.toLowerCase()}`,
116-
OTP_EMAIL_RATE_LIMIT,
117-
{ failClosed: true }
118-
)
119-
if (!emailRateLimit.allowed) {
120-
logger.warn(
121-
`[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deployment.id}`
122-
)
123-
const retryAfter = Math.ceil(
124-
(emailRateLimit.retryAfterMs ?? OTP_EMAIL_RATE_LIMIT.refillIntervalMs) / 1000
125-
)
126-
const response = createErrorResponse(
127-
'Too many verification code requests. Please try again later.',
128-
429
129-
)
130-
response.headers.set('Retry-After', String(retryAfter))
131-
return response
132-
}
133-
134-
const otp = generateOTP()
135-
await storeOTP('chat', deployment.id, email, otp)
136-
137-
const emailHtml = await renderOTPEmail(
138-
otp,
139-
email,
140-
'email-verification',
141-
deployment.title || 'Chat'
142-
)
143-
144-
const emailResult = await sendEmail({
145-
to: email,
146-
subject: getOtpSubject(deployment.title || 'Chat'),
147-
html: emailHtml,
137+
afterResponse(async () => {
138+
if (!emailAllowed) return
139+
await deliverOtp(requestId, deployment.id, deployment.title || 'Chat', email)
148140
})
149-
150-
if (!emailResult.success) {
151-
logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message)
152-
return createErrorResponse('Failed to send verification email', 500)
153-
}
154-
155-
logger.info(`[${requestId}] OTP sent to ${email} for chat ${deployment.id}`)
156-
return createSuccessResponse({ message: 'Verification code sent' })
141+
return otpRequestAccepted()
157142
} catch (error) {
158143
logger.error(`[${requestId}] Error processing OTP request:`, error)
159144
return createErrorResponse('Failed to process request', 500)

0 commit comments

Comments
 (0)