Skip to content

Commit ed1b07d

Browse files
TheodoreSpeaksTheodoreSpeaks
committed
fix(security): make OTP requests non-enumerating
1 parent 0543de6 commit ed1b07d

4 files changed

Lines changed: 62 additions & 145 deletions

File tree

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

Lines changed: 11 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ describe('Chat OTP API Route', () => {
252252
})
253253

254254
describe('POST - Rate limiting', () => {
255-
it('isolates rejected emails from the OTP send bucket without a client IP', async () => {
255+
it('returns the generic acceptance response for a rejected email without a client IP', async () => {
256256
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
257257
queueDeployment(emailDeployment)
258258

@@ -265,43 +265,10 @@ describe('Chat OTP API Route', () => {
265265
params: Promise.resolve({ identifier: mockIdentifier }),
266266
})
267267

268-
expect(response.status).toBe(403)
269-
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1)
270-
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
271-
'chat-otp:rejected:chat-123',
272-
expect.any(Object),
273-
{ failClosed: true }
274-
)
275-
expect(mockSendEmail).not.toHaveBeenCalled()
276-
})
277-
278-
it('rate limits rejected emails independently without a client IP', async () => {
279-
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
280-
mockCheckRateLimitDirect.mockResolvedValueOnce({
281-
allowed: false,
282-
remaining: 0,
283-
resetAt: new Date(Date.now() + 900_000),
284-
retryAfterMs: 900_000,
285-
})
286-
const headerSet = vi.fn()
287-
mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({
288-
json: () => Promise.resolve({ error: message }),
289-
status,
290-
headers: { set: headerSet },
291-
}))
292-
queueDeployment(emailDeployment)
293-
294-
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
295-
method: 'POST',
296-
body: JSON.stringify({ email: 'not-allowed@example.com' }),
297-
})
298-
299-
const response = await POST(request, {
300-
params: Promise.resolve({ identifier: mockIdentifier }),
301-
})
302-
303-
expect(response.status).toBe(429)
304-
expect(headerSet).toHaveBeenCalledWith('Retry-After', '900')
268+
expect(response.status).toBe(200)
269+
await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' })
270+
expect(mockCheckRateLimitDirect).not.toHaveBeenCalled()
271+
expect(mockRedisSet).not.toHaveBeenCalled()
305272
expect(mockSendEmail).not.toHaveBeenCalled()
306273
})
307274

@@ -335,7 +302,7 @@ describe('Chat OTP API Route', () => {
335302
expect(dbChainMockFns.select).not.toHaveBeenCalled()
336303
})
337304

338-
it('returns 429 with Retry-After when email rate limit is exceeded', async () => {
305+
it('returns the generic acceptance response when the email rate limit is exceeded', async () => {
339306
mockCheckRateLimitDirect
340307
.mockResolvedValueOnce({
341308
allowed: true,
@@ -354,13 +321,6 @@ describe('Chat OTP API Route', () => {
354321
retryAfterMs: 900_000,
355322
})
356323

357-
const headerSet = vi.fn()
358-
mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({
359-
json: () => Promise.resolve({ error: message }),
360-
status,
361-
headers: { set: headerSet },
362-
}))
363-
364324
queueDeployment(emailDeployment)
365325

366326
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
@@ -372,12 +332,12 @@ describe('Chat OTP API Route', () => {
372332
params: Promise.resolve({ identifier: mockIdentifier }),
373333
})
374334

375-
expect(response.status).toBe(429)
376-
expect(headerSet).toHaveBeenCalledWith('Retry-After', '900')
335+
expect(response.status).toBe(200)
336+
await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' })
377337
expect(mockSendEmail).not.toHaveBeenCalled()
378338
})
379339

380-
it('returns 429 with Retry-After when the chat resource rate limit is exceeded', async () => {
340+
it('returns the generic acceptance response when the chat resource limit is exceeded', async () => {
381341
mockCheckRateLimitDirect
382342
.mockResolvedValueOnce({
383343
allowed: true,
@@ -391,13 +351,6 @@ describe('Chat OTP API Route', () => {
391351
retryAfterMs: 900_000,
392352
})
393353

394-
const headerSet = vi.fn()
395-
mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({
396-
json: () => Promise.resolve({ error: message }),
397-
status,
398-
headers: { set: headerSet },
399-
}))
400-
401354
queueDeployment(emailDeployment)
402355

403356
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
@@ -409,8 +362,8 @@ describe('Chat OTP API Route', () => {
409362
params: Promise.resolve({ identifier: mockIdentifier }),
410363
})
411364

412-
expect(response.status).toBe(429)
413-
expect(headerSet).toHaveBeenCalledWith('Retry-After', '900')
365+
expect(response.status).toBe(200)
366+
await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' })
414367
expect(mockSendEmail).not.toHaveBeenCalled()
415368
})
416369

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

Lines changed: 9 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@ const logger = createLogger('ChatOtpAPI')
3030

3131
const rateLimiter = new RateLimiter()
3232

33+
function otpRequestAccepted() {
34+
return createSuccessResponse({ message: 'Verification code sent' })
35+
}
36+
3337
export const POST = withRouteHandler(
3438
async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => {
3539
const { identifier } = await context.params
@@ -90,23 +94,7 @@ export const POST = withRouteHandler(
9094
: []
9195

9296
if (!isEmailAllowed(email, allowedEmails)) {
93-
const rejectedRateLimit = await rateLimiter.checkRateLimitDirect(
94-
`chat-otp:rejected:${deployment.id}`,
95-
OTP_RESOURCE_RATE_LIMIT,
96-
{ failClosed: true }
97-
)
98-
if (!rejectedRateLimit.allowed) {
99-
logger.warn(
100-
`[${requestId}] OTP rejected-email rate limit exceeded for chat ${deployment.id}`
101-
)
102-
const retryAfter = Math.ceil(
103-
(rejectedRateLimit.retryAfterMs ?? OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) / 1000
104-
)
105-
const response = createErrorResponse('Too many requests. Please try again later.', 429)
106-
response.headers.set('Retry-After', String(retryAfter))
107-
return response
108-
}
109-
return createErrorResponse('Email not authorized for this chat', 403)
97+
return otpRequestAccepted()
11098
}
11199

112100
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
@@ -116,15 +104,7 @@ export const POST = withRouteHandler(
116104
)
117105
if (!resourceRateLimit.allowed) {
118106
logger.warn(`[${requestId}] OTP resource rate limit exceeded for chat ${deployment.id}`)
119-
const retryAfter = Math.ceil(
120-
(resourceRateLimit.retryAfterMs ?? OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) / 1000
121-
)
122-
const response = createErrorResponse(
123-
'Too many verification code requests. Please try again later.',
124-
429
125-
)
126-
response.headers.set('Retry-After', String(retryAfter))
127-
return response
107+
return otpRequestAccepted()
128108
}
129109

130110
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
@@ -136,15 +116,7 @@ export const POST = withRouteHandler(
136116
logger.warn(
137117
`[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deployment.id}`
138118
)
139-
const retryAfter = Math.ceil(
140-
(emailRateLimit.retryAfterMs ?? OTP_EMAIL_RATE_LIMIT.refillIntervalMs) / 1000
141-
)
142-
const response = createErrorResponse(
143-
'Too many verification code requests. Please try again later.',
144-
429
145-
)
146-
response.headers.set('Retry-After', String(retryAfter))
147-
return response
119+
return otpRequestAccepted()
148120
}
149121

150122
const otp = generateOTP()
@@ -165,11 +137,11 @@ export const POST = withRouteHandler(
165137

166138
if (!emailResult.success) {
167139
logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message)
168-
return createErrorResponse('Failed to send verification email', 500)
140+
return otpRequestAccepted()
169141
}
170142

171143
logger.info(`[${requestId}] OTP sent to ${email} for chat ${deployment.id}`)
172-
return createSuccessResponse({ message: 'Verification code sent' })
144+
return otpRequestAccepted()
173145
} catch (error) {
174146
logger.error(`[${requestId}] Error processing OTP request:`, error)
175147
return createErrorResponse('Failed to process request', 500)

‎apps/sim/app/api/files/public/[token]/otp/route.test.ts‎

Lines changed: 33 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -102,46 +102,25 @@ describe('POST /api/files/public/[token]/otp', () => {
102102
expect(mockSendEmail).toHaveBeenCalled()
103103
})
104104

105-
it('rejects an email not on the allow-list with 403', async () => {
105+
it('returns the generic acceptance response for an email not on the allow-list', async () => {
106106
mockIsEmailAllowed.mockReturnValueOnce(false)
107107
const res = await POST(post('user@evil.com'), params())
108-
expect(res.status).toBe(403)
109-
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2)
110-
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
111-
2,
112-
'file-otp:rejected:sh_1',
113-
expect.any(Object),
114-
{ failClosed: true }
115-
)
116-
expect(mockStoreOTP).not.toHaveBeenCalled()
117-
})
118-
119-
it('isolates rejected emails from the OTP send bucket without a client IP', async () => {
120-
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
121-
mockIsEmailAllowed.mockReturnValueOnce(false)
122-
123-
const res = await POST(post('user@evil.com'), params())
124-
125-
expect(res.status).toBe(403)
108+
expect(res.status).toBe(200)
109+
await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' })
126110
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1)
127-
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
128-
'file-otp:rejected:sh_1',
129-
expect.any(Object),
130-
{ failClosed: true }
131-
)
132111
expect(mockStoreOTP).not.toHaveBeenCalled()
133112
expect(mockSendEmail).not.toHaveBeenCalled()
134113
})
135114

136-
it('rate limits rejected emails independently without a client IP', async () => {
115+
it('does not consume a send bucket for a rejected email without a client IP', async () => {
137116
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
138117
mockIsEmailAllowed.mockReturnValueOnce(false)
139-
mockCheckRateLimitDirect.mockResolvedValueOnce({ allowed: false, retryAfterMs: 1000 })
140118

141119
const res = await POST(post('user@evil.com'), params())
142120

143-
expect(res.status).toBe(429)
144-
expect(res.headers.get('Retry-After')).toBe('1')
121+
expect(res.status).toBe(200)
122+
await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' })
123+
expect(mockCheckRateLimitDirect).not.toHaveBeenCalled()
145124
expect(mockStoreOTP).not.toHaveBeenCalled()
146125
expect(mockSendEmail).not.toHaveBeenCalled()
147126
})
@@ -168,19 +147,42 @@ describe('POST /api/files/public/[token]/otp', () => {
168147
expect(res.headers.get('Retry-After')).toBe('1')
169148
})
170149

171-
it('returns 429 when the share resource rate limit is exceeded', async () => {
150+
it('returns the generic acceptance response when the share resource limit is exceeded', async () => {
172151
mockCheckRateLimitDirect
173152
.mockResolvedValueOnce({ allowed: true })
174153
.mockResolvedValueOnce({ allowed: false, retryAfterMs: 1000 })
175154

176155
const res = await POST(post('user@acme.com'), params())
177156

178-
expect(res.status).toBe(429)
179-
expect(res.headers.get('Retry-After')).toBe('1')
157+
expect(res.status).toBe(200)
158+
await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' })
180159
expect(mockStoreOTP).not.toHaveBeenCalled()
181160
expect(mockSendEmail).not.toHaveBeenCalled()
182161
})
183162

163+
it('returns the generic acceptance response when the email rate limit is exceeded', async () => {
164+
mockCheckRateLimitDirect
165+
.mockResolvedValueOnce({ allowed: true })
166+
.mockResolvedValueOnce({ allowed: true })
167+
.mockResolvedValueOnce({ allowed: false, retryAfterMs: 1000 })
168+
169+
const res = await POST(post('user@acme.com'), params())
170+
171+
expect(res.status).toBe(200)
172+
await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' })
173+
expect(mockStoreOTP).not.toHaveBeenCalled()
174+
expect(mockSendEmail).not.toHaveBeenCalled()
175+
})
176+
177+
it('returns the generic acceptance response when email delivery fails', async () => {
178+
mockSendEmail.mockResolvedValueOnce({ success: false, message: 'Delivery failed' })
179+
180+
const res = await POST(post('user@acme.com'), params())
181+
182+
expect(res.status).toBe(200)
183+
await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' })
184+
})
185+
184186
it('retains resource and email backstops when the client IP cannot be resolved', async () => {
185187
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
186188

‎apps/sim/app/api/files/public/[token]/otp/route.ts‎

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ function rateLimited(retryAfterMs: number | undefined, fallbackMs: number): Next
4949
return response
5050
}
5151

52+
function otpRequestAccepted(): NextResponse {
53+
return NextResponse.json({ message: 'Verification code sent' })
54+
}
55+
5256
/**
5357
* POST /api/files/public/[token]/otp
5458
* Sends a 6-digit verification code to an allow-listed email for an email-gated share.
@@ -90,21 +94,7 @@ export const POST = withRouteHandler(
9094
}
9195

9296
if (!isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails))) {
93-
const rejectedRateLimit = await rateLimiter.checkRateLimitDirect(
94-
`file-otp:rejected:${resolved.share.id}`,
95-
OTP_RESOURCE_RATE_LIMIT,
96-
{ failClosed: true }
97-
)
98-
if (!rejectedRateLimit.allowed) {
99-
logger.warn(
100-
`[${requestId}] OTP rejected-email rate limit exceeded for share ${resolved.share.id}`
101-
)
102-
return rateLimited(
103-
rejectedRateLimit.retryAfterMs,
104-
OTP_RESOURCE_RATE_LIMIT.refillIntervalMs
105-
)
106-
}
107-
return NextResponse.json({ error: 'Email not authorized for this file' }, { status: 403 })
97+
return otpRequestAccepted()
10898
}
10999

110100
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
@@ -116,7 +106,7 @@ export const POST = withRouteHandler(
116106
logger.warn(
117107
`[${requestId}] OTP resource rate limit exceeded for share ${resolved.share.id}`
118108
)
119-
return rateLimited(resourceRateLimit.retryAfterMs, OTP_RESOURCE_RATE_LIMIT.refillIntervalMs)
109+
return otpRequestAccepted()
120110
}
121111

122112
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
@@ -126,7 +116,7 @@ export const POST = withRouteHandler(
126116
)
127117
if (!emailRateLimit.allowed) {
128118
logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email}`)
129-
return rateLimited(emailRateLimit.retryAfterMs, OTP_EMAIL_RATE_LIMIT.refillIntervalMs)
119+
return otpRequestAccepted()
130120
}
131121

132122
const otp = generateOTP()
@@ -140,11 +130,11 @@ export const POST = withRouteHandler(
140130
})
141131
if (!emailResult.success) {
142132
logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message)
143-
return NextResponse.json({ error: 'Failed to send verification email' }, { status: 500 })
133+
return otpRequestAccepted()
144134
}
145135

146136
logger.info(`[${requestId}] OTP sent for share ${resolved.share.id}`)
147-
return NextResponse.json({ message: 'Verification code sent' })
137+
return otpRequestAccepted()
148138
} catch (error) {
149139
logger.error(`[${requestId}] Error processing OTP request:`, error)
150140
return NextResponse.json({ error: 'Failed to process request' }, { status: 500 })

0 commit comments

Comments
 (0)