Skip to content

Commit abd36ff

Browse files
TheodoreSpeaksTheodoreSpeaks
committed
fix(security): defer OTP delivery work
1 parent ed1b07d commit abd36ff

5 files changed

Lines changed: 124 additions & 90 deletions

File tree

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

Lines changed: 17 additions & 2 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' })
@@ -267,6 +280,7 @@ describe('Chat OTP API Route', () => {
267280

268281
expect(response.status).toBe(200)
269282
await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' })
283+
expect(mockAfterResponse).toHaveBeenCalledTimes(1)
270284
expect(mockCheckRateLimitDirect).not.toHaveBeenCalled()
271285
expect(mockRedisSet).not.toHaveBeenCalled()
272286
expect(mockSendEmail).not.toHaveBeenCalled()
@@ -402,6 +416,7 @@ describe('Chat OTP API Route', () => {
402416

403417
await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
404418

419+
expect(mockAfterResponse).toHaveBeenCalledTimes(1)
405420
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2)
406421
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
407422
1,

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

Lines changed: 44 additions & 47 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'
@@ -34,6 +35,45 @@ function otpRequestAccepted() {
3435
return createSuccessResponse({ message: 'Verification code sent' })
3536
}
3637

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+
3777
export const POST = withRouteHandler(
3878
async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => {
3979
const { identifier } = await context.params
@@ -92,55 +132,12 @@ export const POST = withRouteHandler(
92132
const allowedEmails: string[] = Array.isArray(deployment.allowedEmails)
93133
? deployment.allowedEmails
94134
: []
135+
const emailAllowed = isEmailAllowed(email, allowedEmails)
95136

96-
if (!isEmailAllowed(email, allowedEmails)) {
97-
return otpRequestAccepted()
98-
}
99-
100-
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
101-
`chat-otp:resource:${deployment.id}`,
102-
OTP_RESOURCE_RATE_LIMIT,
103-
{ failClosed: true }
104-
)
105-
if (!resourceRateLimit.allowed) {
106-
logger.warn(`[${requestId}] OTP resource rate limit exceeded for chat ${deployment.id}`)
107-
return otpRequestAccepted()
108-
}
109-
110-
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
111-
`chat-otp:email:${deployment.id}:${email.toLowerCase()}`,
112-
OTP_EMAIL_RATE_LIMIT,
113-
{ failClosed: true }
114-
)
115-
if (!emailRateLimit.allowed) {
116-
logger.warn(
117-
`[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deployment.id}`
118-
)
119-
return otpRequestAccepted()
120-
}
121-
122-
const otp = generateOTP()
123-
await storeOTP('chat', deployment.id, email, otp)
124-
125-
const emailHtml = await renderOTPEmail(
126-
otp,
127-
email,
128-
'email-verification',
129-
deployment.title || 'Chat'
130-
)
131-
132-
const emailResult = await sendEmail({
133-
to: email,
134-
subject: getOtpSubject(deployment.title || 'Chat'),
135-
html: emailHtml,
137+
afterResponse(async () => {
138+
if (!emailAllowed) return
139+
await deliverOtp(requestId, deployment.id, deployment.title || 'Chat', email)
136140
})
137-
138-
if (!emailResult.success) {
139-
logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message)
140-
return otpRequestAccepted()
141-
}
142-
143-
logger.info(`[${requestId}] OTP sent to ${email} for chat ${deployment.id}`)
144141
return otpRequestAccepted()
145142
} catch (error) {
146143
logger.error(`[${requestId}] Error processing OTP request:`, error)

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ const {
1818
mockRenderOTPEmail,
1919
mockSendEmail,
2020
mockCheckRateLimitDirect,
21+
mockAfterResponse,
2122
} = vi.hoisted(() => ({
2223
mockResolveActiveShareByToken: vi.fn(),
2324
mockIsEmailAllowed: vi.fn(),
@@ -31,6 +32,7 @@ const {
3132
mockRenderOTPEmail: vi.fn(),
3233
mockSendEmail: vi.fn(),
3334
mockCheckRateLimitDirect: vi.fn(),
35+
mockAfterResponse: vi.fn(),
3436
}))
3537

3638
vi.mock('@/lib/public-shares/share-manager', () => ({
@@ -62,8 +64,18 @@ vi.mock('@/lib/core/rate-limiter', () => ({
6264
checkRateLimitDirect = mockCheckRateLimitDirect
6365
},
6466
}))
67+
vi.mock('@/lib/core/utils/after-response', () => ({
68+
afterResponse: mockAfterResponse,
69+
}))
70+
71+
import { PUT, POST as routePost } from '@/app/api/files/public/[token]/otp/route'
6572

66-
import { POST, PUT } from '@/app/api/files/public/[token]/otp/route'
73+
const POST: typeof routePost = async (...args) => {
74+
const response = await routePost(...args)
75+
const task = mockAfterResponse.mock.calls.at(-1)?.[0] as (() => Promise<void>) | undefined
76+
if (task) await task()
77+
return response
78+
}
6779

6880
const params = (token = 'tok_1') => ({ params: Promise.resolve({ token }) })
6981
const post = (email: string, token = 'tok_1') =>
@@ -98,6 +110,7 @@ describe('POST /api/files/public/[token]/otp', () => {
98110
it('sends a code to an allow-listed email', async () => {
99111
const res = await POST(post('user@acme.com'), params())
100112
expect(res.status).toBe(200)
113+
expect(mockAfterResponse).toHaveBeenCalledTimes(1)
101114
expect(mockStoreOTP).toHaveBeenCalledWith('file', 'sh_1', 'user@acme.com', '123456')
102115
expect(mockSendEmail).toHaveBeenCalled()
103116
})
@@ -120,6 +133,7 @@ describe('POST /api/files/public/[token]/otp', () => {
120133

121134
expect(res.status).toBe(200)
122135
await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' })
136+
expect(mockAfterResponse).toHaveBeenCalledTimes(1)
123137
expect(mockCheckRateLimitDirect).not.toHaveBeenCalled()
124138
expect(mockStoreOTP).not.toHaveBeenCalled()
125139
expect(mockSendEmail).not.toHaveBeenCalled()

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

Lines changed: 43 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
OTP_RESOURCE_RATE_LIMIT,
2323
storeOTP,
2424
} from '@/lib/core/security/otp'
25+
import { afterResponse } from '@/lib/core/utils/after-response'
2526
import { generateRequestId, getClientIp } from '@/lib/core/utils/request'
2627
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
2728
import { sendEmail } from '@/lib/messaging/email/mailer'
@@ -53,6 +54,44 @@ function otpRequestAccepted(): NextResponse {
5354
return NextResponse.json({ message: 'Verification code sent' })
5455
}
5556

57+
async function deliverOtp(requestId: string, shareId: string, email: string): Promise<void> {
58+
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
59+
`file-otp:resource:${shareId}`,
60+
OTP_RESOURCE_RATE_LIMIT,
61+
{ failClosed: true }
62+
)
63+
if (!resourceRateLimit.allowed) {
64+
logger.warn(`[${requestId}] OTP resource rate limit exceeded for share ${shareId}`)
65+
return
66+
}
67+
68+
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
69+
`file-otp:email:${shareId}:${email}`,
70+
OTP_EMAIL_RATE_LIMIT,
71+
{ failClosed: true }
72+
)
73+
if (!emailRateLimit.allowed) {
74+
logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email}`)
75+
return
76+
}
77+
78+
const otp = generateOTP()
79+
await storeOTP('file', shareId, email, otp)
80+
81+
const emailHtml = await renderOTPEmail(otp, email, 'email-verification', SHARE_EMAIL_LABEL)
82+
const emailResult = await sendEmail({
83+
to: email,
84+
subject: getOtpSubject(SHARE_EMAIL_LABEL),
85+
html: emailHtml,
86+
})
87+
if (!emailResult.success) {
88+
logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message)
89+
return
90+
}
91+
92+
logger.info(`[${requestId}] OTP sent for share ${shareId}`)
93+
}
94+
5695
/**
5796
* POST /api/files/public/[token]/otp
5897
* Sends a 6-digit verification code to an allow-listed email for an email-gated share.
@@ -92,48 +131,12 @@ export const POST = withRouteHandler(
92131
{ status: 400 }
93132
)
94133
}
134+
const emailAllowed = isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails))
95135

96-
if (!isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails))) {
97-
return otpRequestAccepted()
98-
}
99-
100-
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
101-
`file-otp:resource:${resolved.share.id}`,
102-
OTP_RESOURCE_RATE_LIMIT,
103-
{ failClosed: true }
104-
)
105-
if (!resourceRateLimit.allowed) {
106-
logger.warn(
107-
`[${requestId}] OTP resource rate limit exceeded for share ${resolved.share.id}`
108-
)
109-
return otpRequestAccepted()
110-
}
111-
112-
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
113-
`file-otp:email:${resolved.share.id}:${email}`,
114-
OTP_EMAIL_RATE_LIMIT,
115-
{ failClosed: true }
116-
)
117-
if (!emailRateLimit.allowed) {
118-
logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email}`)
119-
return otpRequestAccepted()
120-
}
121-
122-
const otp = generateOTP()
123-
await storeOTP('file', resolved.share.id, email, otp)
124-
125-
const emailHtml = await renderOTPEmail(otp, email, 'email-verification', SHARE_EMAIL_LABEL)
126-
const emailResult = await sendEmail({
127-
to: email,
128-
subject: getOtpSubject(SHARE_EMAIL_LABEL),
129-
html: emailHtml,
136+
afterResponse(async () => {
137+
if (!emailAllowed) return
138+
await deliverOtp(requestId, resolved.share.id, email)
130139
})
131-
if (!emailResult.success) {
132-
logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message)
133-
return otpRequestAccepted()
134-
}
135-
136-
logger.info(`[${requestId}] OTP sent for share ${resolved.share.id}`)
137140
return otpRequestAccepted()
138141
} catch (error) {
139142
logger.error(`[${requestId}] Error processing OTP request:`, error)

‎apps/sim/lib/core/utils/after-response.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import { after } from 'next/server'
2+
3+
export function afterResponse(task: () => Promise<void>): void {
4+
after(task)
5+
}

0 commit comments

Comments
 (0)