From 0543de6b87ff576cb3e9187c769a7551032bb6f2 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Sat, 22 Aug 2026 23:47:08 -0700 Subject: [PATCH 1/3] fix(security): isolate rejected OTP attempts --- .../api/chat/[identifier]/otp/route.test.ts | 53 +++++++++++++++++++ .../app/api/chat/[identifier]/otp/route.ts | 24 +++++++-- .../files/public/[token]/otp/route.test.ts | 37 +++++++++++++ .../app/api/files/public/[token]/otp/route.ts | 22 ++++++-- 4 files changed, 128 insertions(+), 8 deletions(-) diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts index b882ed310ff..4ce945e633c 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts @@ -252,6 +252,59 @@ describe('Chat OTP API Route', () => { }) describe('POST - Rate limiting', () => { + it('isolates rejected emails from the OTP send bucket without a client IP', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + queueDeployment(emailDeployment) + + const request = new NextRequest('http://localhost:3000/api/chat/test/otp', { + method: 'POST', + body: JSON.stringify({ email: 'not-allowed@example.com' }), + }) + + const response = await POST(request, { + params: Promise.resolve({ identifier: mockIdentifier }), + }) + + expect(response.status).toBe(403) + expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1) + expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + 'chat-otp:rejected:chat-123', + expect.any(Object), + { failClosed: true } + ) + expect(mockSendEmail).not.toHaveBeenCalled() + }) + + it('rate limits rejected emails independently without a client IP', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + mockCheckRateLimitDirect.mockResolvedValueOnce({ + allowed: false, + remaining: 0, + resetAt: new Date(Date.now() + 900_000), + retryAfterMs: 900_000, + }) + const headerSet = vi.fn() + mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({ + json: () => Promise.resolve({ error: message }), + status, + headers: { set: headerSet }, + })) + queueDeployment(emailDeployment) + + const request = new NextRequest('http://localhost:3000/api/chat/test/otp', { + method: 'POST', + body: JSON.stringify({ email: 'not-allowed@example.com' }), + }) + + const response = await POST(request, { + params: Promise.resolve({ identifier: mockIdentifier }), + }) + + expect(response.status).toBe(429) + expect(headerSet).toHaveBeenCalledWith('Retry-After', '900') + expect(mockSendEmail).not.toHaveBeenCalled() + }) + it('returns 429 with Retry-After when IP rate limit is exceeded', async () => { mockCheckRateLimitDirect.mockResolvedValueOnce({ allowed: false, diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.ts b/apps/sim/app/api/chat/[identifier]/otp/route.ts index 0b778675ccd..8492b0f0414 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.ts @@ -89,6 +89,26 @@ export const POST = withRouteHandler( ? deployment.allowedEmails : [] + if (!isEmailAllowed(email, allowedEmails)) { + const rejectedRateLimit = await rateLimiter.checkRateLimitDirect( + `chat-otp:rejected:${deployment.id}`, + OTP_RESOURCE_RATE_LIMIT, + { failClosed: true } + ) + if (!rejectedRateLimit.allowed) { + logger.warn( + `[${requestId}] OTP rejected-email rate limit exceeded for chat ${deployment.id}` + ) + const retryAfter = Math.ceil( + (rejectedRateLimit.retryAfterMs ?? OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) / 1000 + ) + const response = createErrorResponse('Too many requests. Please try again later.', 429) + response.headers.set('Retry-After', String(retryAfter)) + return response + } + return createErrorResponse('Email not authorized for this chat', 403) + } + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( `chat-otp:resource:${deployment.id}`, OTP_RESOURCE_RATE_LIMIT, @@ -107,10 +127,6 @@ export const POST = withRouteHandler( return response } - if (!isEmailAllowed(email, allowedEmails)) { - return createErrorResponse('Email not authorized for this chat', 403) - } - const emailRateLimit = await rateLimiter.checkRateLimitDirect( `chat-otp:email:${deployment.id}:${email.toLowerCase()}`, OTP_EMAIL_RATE_LIMIT, diff --git a/apps/sim/app/api/files/public/[token]/otp/route.test.ts b/apps/sim/app/api/files/public/[token]/otp/route.test.ts index a30aecc4bd9..d3c06de9196 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.test.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.test.ts @@ -106,9 +106,46 @@ describe('POST /api/files/public/[token]/otp', () => { mockIsEmailAllowed.mockReturnValueOnce(false) const res = await POST(post('user@evil.com'), params()) expect(res.status).toBe(403) + expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 2, + 'file-otp:rejected:sh_1', + expect.any(Object), + { failClosed: true } + ) expect(mockStoreOTP).not.toHaveBeenCalled() }) + it('isolates rejected emails from the OTP send bucket without a client IP', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + mockIsEmailAllowed.mockReturnValueOnce(false) + + const res = await POST(post('user@evil.com'), params()) + + expect(res.status).toBe(403) + expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1) + expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + 'file-otp:rejected:sh_1', + expect.any(Object), + { failClosed: true } + ) + expect(mockStoreOTP).not.toHaveBeenCalled() + expect(mockSendEmail).not.toHaveBeenCalled() + }) + + it('rate limits rejected emails independently without a client IP', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + mockIsEmailAllowed.mockReturnValueOnce(false) + mockCheckRateLimitDirect.mockResolvedValueOnce({ allowed: false, retryAfterMs: 1000 }) + + const res = await POST(post('user@evil.com'), params()) + + expect(res.status).toBe(429) + expect(res.headers.get('Retry-After')).toBe('1') + expect(mockStoreOTP).not.toHaveBeenCalled() + expect(mockSendEmail).not.toHaveBeenCalled() + }) + it('lowercases the email for allow-list matching and OTP storage', async () => { await POST(post('User@ACME.com'), params()) expect(mockIsEmailAllowed).toHaveBeenCalledWith('user@acme.com', expect.anything()) diff --git a/apps/sim/app/api/files/public/[token]/otp/route.ts b/apps/sim/app/api/files/public/[token]/otp/route.ts index ba5ae83593b..6cbbd5400b1 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.ts @@ -89,6 +89,24 @@ export const POST = withRouteHandler( ) } + if (!isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails))) { + const rejectedRateLimit = await rateLimiter.checkRateLimitDirect( + `file-otp:rejected:${resolved.share.id}`, + OTP_RESOURCE_RATE_LIMIT, + { failClosed: true } + ) + if (!rejectedRateLimit.allowed) { + logger.warn( + `[${requestId}] OTP rejected-email rate limit exceeded for share ${resolved.share.id}` + ) + return rateLimited( + rejectedRateLimit.retryAfterMs, + OTP_RESOURCE_RATE_LIMIT.refillIntervalMs + ) + } + return NextResponse.json({ error: 'Email not authorized for this file' }, { status: 403 }) + } + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( `file-otp:resource:${resolved.share.id}`, OTP_RESOURCE_RATE_LIMIT, @@ -101,10 +119,6 @@ export const POST = withRouteHandler( return rateLimited(resourceRateLimit.retryAfterMs, OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) } - if (!isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails))) { - return NextResponse.json({ error: 'Email not authorized for this file' }, { status: 403 }) - } - const emailRateLimit = await rateLimiter.checkRateLimitDirect( `file-otp:email:${resolved.share.id}:${email}`, OTP_EMAIL_RATE_LIMIT, From ed1b07d0e66aba556d6d09527e3eb2f69ab89a35 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Sun, 23 Aug 2026 00:00:59 -0700 Subject: [PATCH 2/3] fix(security): make OTP requests non-enumerating --- .../api/chat/[identifier]/otp/route.test.ts | 69 +++---------------- .../app/api/chat/[identifier]/otp/route.ts | 46 +++---------- .../files/public/[token]/otp/route.test.ts | 64 ++++++++--------- .../app/api/files/public/[token]/otp/route.ts | 28 +++----- 4 files changed, 62 insertions(+), 145 deletions(-) diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts index 4ce945e633c..b3d1529bfee 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts @@ -252,7 +252,7 @@ describe('Chat OTP API Route', () => { }) describe('POST - Rate limiting', () => { - it('isolates rejected emails from the OTP send bucket without a client IP', async () => { + it('returns the generic acceptance response for a rejected email without a client IP', async () => { requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) queueDeployment(emailDeployment) @@ -265,43 +265,10 @@ describe('Chat OTP API Route', () => { params: Promise.resolve({ identifier: mockIdentifier }), }) - expect(response.status).toBe(403) - expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1) - expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( - 'chat-otp:rejected:chat-123', - expect.any(Object), - { failClosed: true } - ) - expect(mockSendEmail).not.toHaveBeenCalled() - }) - - it('rate limits rejected emails independently without a client IP', async () => { - requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) - mockCheckRateLimitDirect.mockResolvedValueOnce({ - allowed: false, - remaining: 0, - resetAt: new Date(Date.now() + 900_000), - retryAfterMs: 900_000, - }) - const headerSet = vi.fn() - mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({ - json: () => Promise.resolve({ error: message }), - status, - headers: { set: headerSet }, - })) - queueDeployment(emailDeployment) - - const request = new NextRequest('http://localhost:3000/api/chat/test/otp', { - method: 'POST', - body: JSON.stringify({ email: 'not-allowed@example.com' }), - }) - - const response = await POST(request, { - params: Promise.resolve({ identifier: mockIdentifier }), - }) - - expect(response.status).toBe(429) - expect(headerSet).toHaveBeenCalledWith('Retry-After', '900') + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' }) + expect(mockCheckRateLimitDirect).not.toHaveBeenCalled() + expect(mockRedisSet).not.toHaveBeenCalled() expect(mockSendEmail).not.toHaveBeenCalled() }) @@ -335,7 +302,7 @@ describe('Chat OTP API Route', () => { expect(dbChainMockFns.select).not.toHaveBeenCalled() }) - it('returns 429 with Retry-After when email rate limit is exceeded', async () => { + it('returns the generic acceptance response when the email rate limit is exceeded', async () => { mockCheckRateLimitDirect .mockResolvedValueOnce({ allowed: true, @@ -354,13 +321,6 @@ describe('Chat OTP API Route', () => { retryAfterMs: 900_000, }) - const headerSet = vi.fn() - mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({ - json: () => Promise.resolve({ error: message }), - status, - headers: { set: headerSet }, - })) - queueDeployment(emailDeployment) const request = new NextRequest('http://localhost:3000/api/chat/test/otp', { @@ -372,12 +332,12 @@ describe('Chat OTP API Route', () => { params: Promise.resolve({ identifier: mockIdentifier }), }) - expect(response.status).toBe(429) - expect(headerSet).toHaveBeenCalledWith('Retry-After', '900') + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' }) expect(mockSendEmail).not.toHaveBeenCalled() }) - it('returns 429 with Retry-After when the chat resource rate limit is exceeded', async () => { + it('returns the generic acceptance response when the chat resource limit is exceeded', async () => { mockCheckRateLimitDirect .mockResolvedValueOnce({ allowed: true, @@ -391,13 +351,6 @@ describe('Chat OTP API Route', () => { retryAfterMs: 900_000, }) - const headerSet = vi.fn() - mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({ - json: () => Promise.resolve({ error: message }), - status, - headers: { set: headerSet }, - })) - queueDeployment(emailDeployment) const request = new NextRequest('http://localhost:3000/api/chat/test/otp', { @@ -409,8 +362,8 @@ describe('Chat OTP API Route', () => { params: Promise.resolve({ identifier: mockIdentifier }), }) - expect(response.status).toBe(429) - expect(headerSet).toHaveBeenCalledWith('Retry-After', '900') + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' }) expect(mockSendEmail).not.toHaveBeenCalled() }) diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.ts b/apps/sim/app/api/chat/[identifier]/otp/route.ts index 8492b0f0414..f681d6a4085 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.ts @@ -30,6 +30,10 @@ const logger = createLogger('ChatOtpAPI') const rateLimiter = new RateLimiter() +function otpRequestAccepted() { + return createSuccessResponse({ message: 'Verification code sent' }) +} + export const POST = withRouteHandler( async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => { const { identifier } = await context.params @@ -90,23 +94,7 @@ export const POST = withRouteHandler( : [] if (!isEmailAllowed(email, allowedEmails)) { - const rejectedRateLimit = await rateLimiter.checkRateLimitDirect( - `chat-otp:rejected:${deployment.id}`, - OTP_RESOURCE_RATE_LIMIT, - { failClosed: true } - ) - if (!rejectedRateLimit.allowed) { - logger.warn( - `[${requestId}] OTP rejected-email rate limit exceeded for chat ${deployment.id}` - ) - const retryAfter = Math.ceil( - (rejectedRateLimit.retryAfterMs ?? OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) / 1000 - ) - const response = createErrorResponse('Too many requests. Please try again later.', 429) - response.headers.set('Retry-After', String(retryAfter)) - return response - } - return createErrorResponse('Email not authorized for this chat', 403) + return otpRequestAccepted() } const resourceRateLimit = await rateLimiter.checkRateLimitDirect( @@ -116,15 +104,7 @@ export const POST = withRouteHandler( ) if (!resourceRateLimit.allowed) { logger.warn(`[${requestId}] OTP resource rate limit exceeded for chat ${deployment.id}`) - const retryAfter = Math.ceil( - (resourceRateLimit.retryAfterMs ?? OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) / 1000 - ) - const response = createErrorResponse( - 'Too many verification code requests. Please try again later.', - 429 - ) - response.headers.set('Retry-After', String(retryAfter)) - return response + return otpRequestAccepted() } const emailRateLimit = await rateLimiter.checkRateLimitDirect( @@ -136,15 +116,7 @@ export const POST = withRouteHandler( logger.warn( `[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deployment.id}` ) - const retryAfter = Math.ceil( - (emailRateLimit.retryAfterMs ?? OTP_EMAIL_RATE_LIMIT.refillIntervalMs) / 1000 - ) - const response = createErrorResponse( - 'Too many verification code requests. Please try again later.', - 429 - ) - response.headers.set('Retry-After', String(retryAfter)) - return response + return otpRequestAccepted() } const otp = generateOTP() @@ -165,11 +137,11 @@ export const POST = withRouteHandler( if (!emailResult.success) { logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message) - return createErrorResponse('Failed to send verification email', 500) + return otpRequestAccepted() } logger.info(`[${requestId}] OTP sent to ${email} for chat ${deployment.id}`) - return createSuccessResponse({ message: 'Verification code sent' }) + return otpRequestAccepted() } catch (error) { logger.error(`[${requestId}] Error processing OTP request:`, error) return createErrorResponse('Failed to process request', 500) diff --git a/apps/sim/app/api/files/public/[token]/otp/route.test.ts b/apps/sim/app/api/files/public/[token]/otp/route.test.ts index d3c06de9196..9cfd11f2083 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.test.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.test.ts @@ -102,46 +102,25 @@ describe('POST /api/files/public/[token]/otp', () => { expect(mockSendEmail).toHaveBeenCalled() }) - it('rejects an email not on the allow-list with 403', async () => { + it('returns the generic acceptance response for an email not on the allow-list', async () => { mockIsEmailAllowed.mockReturnValueOnce(false) const res = await POST(post('user@evil.com'), params()) - expect(res.status).toBe(403) - expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2) - expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( - 2, - 'file-otp:rejected:sh_1', - expect.any(Object), - { failClosed: true } - ) - expect(mockStoreOTP).not.toHaveBeenCalled() - }) - - it('isolates rejected emails from the OTP send bucket without a client IP', async () => { - requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) - mockIsEmailAllowed.mockReturnValueOnce(false) - - const res = await POST(post('user@evil.com'), params()) - - expect(res.status).toBe(403) + expect(res.status).toBe(200) + await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' }) expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1) - expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( - 'file-otp:rejected:sh_1', - expect.any(Object), - { failClosed: true } - ) expect(mockStoreOTP).not.toHaveBeenCalled() expect(mockSendEmail).not.toHaveBeenCalled() }) - it('rate limits rejected emails independently without a client IP', async () => { + it('does not consume a send bucket for a rejected email without a client IP', async () => { requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) mockIsEmailAllowed.mockReturnValueOnce(false) - mockCheckRateLimitDirect.mockResolvedValueOnce({ allowed: false, retryAfterMs: 1000 }) const res = await POST(post('user@evil.com'), params()) - expect(res.status).toBe(429) - expect(res.headers.get('Retry-After')).toBe('1') + expect(res.status).toBe(200) + await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' }) + expect(mockCheckRateLimitDirect).not.toHaveBeenCalled() expect(mockStoreOTP).not.toHaveBeenCalled() expect(mockSendEmail).not.toHaveBeenCalled() }) @@ -168,19 +147,42 @@ describe('POST /api/files/public/[token]/otp', () => { expect(res.headers.get('Retry-After')).toBe('1') }) - it('returns 429 when the share resource rate limit is exceeded', async () => { + it('returns the generic acceptance response when the share resource limit is exceeded', async () => { mockCheckRateLimitDirect .mockResolvedValueOnce({ allowed: true }) .mockResolvedValueOnce({ allowed: false, retryAfterMs: 1000 }) const res = await POST(post('user@acme.com'), params()) - expect(res.status).toBe(429) - expect(res.headers.get('Retry-After')).toBe('1') + expect(res.status).toBe(200) + await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' }) expect(mockStoreOTP).not.toHaveBeenCalled() expect(mockSendEmail).not.toHaveBeenCalled() }) + it('returns the generic acceptance response when the email rate limit is exceeded', async () => { + mockCheckRateLimitDirect + .mockResolvedValueOnce({ allowed: true }) + .mockResolvedValueOnce({ allowed: true }) + .mockResolvedValueOnce({ allowed: false, retryAfterMs: 1000 }) + + const res = await POST(post('user@acme.com'), params()) + + expect(res.status).toBe(200) + await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' }) + expect(mockStoreOTP).not.toHaveBeenCalled() + expect(mockSendEmail).not.toHaveBeenCalled() + }) + + it('returns the generic acceptance response when email delivery fails', async () => { + mockSendEmail.mockResolvedValueOnce({ success: false, message: 'Delivery failed' }) + + const res = await POST(post('user@acme.com'), params()) + + expect(res.status).toBe(200) + await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' }) + }) + it('retains resource and email backstops when the client IP cannot be resolved', async () => { requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) diff --git a/apps/sim/app/api/files/public/[token]/otp/route.ts b/apps/sim/app/api/files/public/[token]/otp/route.ts index 6cbbd5400b1..85ef66e1757 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.ts @@ -49,6 +49,10 @@ function rateLimited(retryAfterMs: number | undefined, fallbackMs: number): Next return response } +function otpRequestAccepted(): NextResponse { + return NextResponse.json({ message: 'Verification code sent' }) +} + /** * POST /api/files/public/[token]/otp * Sends a 6-digit verification code to an allow-listed email for an email-gated share. @@ -90,21 +94,7 @@ export const POST = withRouteHandler( } if (!isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails))) { - const rejectedRateLimit = await rateLimiter.checkRateLimitDirect( - `file-otp:rejected:${resolved.share.id}`, - OTP_RESOURCE_RATE_LIMIT, - { failClosed: true } - ) - if (!rejectedRateLimit.allowed) { - logger.warn( - `[${requestId}] OTP rejected-email rate limit exceeded for share ${resolved.share.id}` - ) - return rateLimited( - rejectedRateLimit.retryAfterMs, - OTP_RESOURCE_RATE_LIMIT.refillIntervalMs - ) - } - return NextResponse.json({ error: 'Email not authorized for this file' }, { status: 403 }) + return otpRequestAccepted() } const resourceRateLimit = await rateLimiter.checkRateLimitDirect( @@ -116,7 +106,7 @@ export const POST = withRouteHandler( logger.warn( `[${requestId}] OTP resource rate limit exceeded for share ${resolved.share.id}` ) - return rateLimited(resourceRateLimit.retryAfterMs, OTP_RESOURCE_RATE_LIMIT.refillIntervalMs) + return otpRequestAccepted() } const emailRateLimit = await rateLimiter.checkRateLimitDirect( @@ -126,7 +116,7 @@ export const POST = withRouteHandler( ) if (!emailRateLimit.allowed) { logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email}`) - return rateLimited(emailRateLimit.retryAfterMs, OTP_EMAIL_RATE_LIMIT.refillIntervalMs) + return otpRequestAccepted() } const otp = generateOTP() @@ -140,11 +130,11 @@ export const POST = withRouteHandler( }) if (!emailResult.success) { logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message) - return NextResponse.json({ error: 'Failed to send verification email' }, { status: 500 }) + return otpRequestAccepted() } logger.info(`[${requestId}] OTP sent for share ${resolved.share.id}`) - return NextResponse.json({ message: 'Verification code sent' }) + return otpRequestAccepted() } catch (error) { logger.error(`[${requestId}] Error processing OTP request:`, error) return NextResponse.json({ error: 'Failed to process request' }, { status: 500 }) From abd36ff5ed4254818d70a3ff0dab7fab2f3b4c27 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Sun, 23 Aug 2026 00:15:01 -0700 Subject: [PATCH 3/3] fix(security): defer OTP delivery work --- .../api/chat/[identifier]/otp/route.test.ts | 19 +++- .../app/api/chat/[identifier]/otp/route.ts | 91 +++++++++---------- .../files/public/[token]/otp/route.test.ts | 16 +++- .../app/api/files/public/[token]/otp/route.ts | 83 +++++++++-------- apps/sim/lib/core/utils/after-response.ts | 5 + 5 files changed, 124 insertions(+), 90 deletions(-) create mode 100644 apps/sim/lib/core/utils/after-response.ts diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts index b3d1529bfee..c7b4f5d0490 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts @@ -31,6 +31,7 @@ const { mockSetChatAuthCookie, mockGetStorageMethod, mockZodParse, + mockAfterResponse, } = vi.hoisted(() => { const mockRedisSet = vi.fn() const mockRedisGet = vi.fn() @@ -49,6 +50,7 @@ const { const mockSetChatAuthCookie = vi.fn() const mockGetStorageMethod = vi.fn() const mockZodParse = vi.fn() + const mockAfterResponse = vi.fn() return { mockRedisSet, @@ -62,6 +64,7 @@ const { mockSetChatAuthCookie, mockGetStorageMethod, mockZodParse, + mockAfterResponse, } }) @@ -84,6 +87,10 @@ vi.mock('@/lib/core/rate-limiter', () => ({ }, })) +vi.mock('@/lib/core/utils/after-response', () => ({ + afterResponse: mockAfterResponse, +})) + vi.mock('@/lib/messaging/email/mailer', () => ({ sendEmail: mockSendEmail, })) @@ -149,7 +156,14 @@ vi.mock('zod', () => { } }) -import { POST, PUT } from './route' +import { PUT, POST as routePost } from './route' + +const POST: typeof routePost = async (...args) => { + const response = await routePost(...args) + const task = mockAfterResponse.mock.calls.at(-1)?.[0] as (() => Promise) | undefined + if (task) await task() + return response +} describe('Chat OTP API Route', () => { const mockEmail = 'test@example.com' @@ -209,7 +223,6 @@ describe('Chat OTP API Route', () => { remaining: 10, resetAt: new Date(Date.now() + 60_000), }) - mockZodParse.mockImplementation((data: unknown) => data) setEnv({ NEXT_PUBLIC_APP_URL: 'http://localhost:3000', NODE_ENV: 'test' }) @@ -267,6 +280,7 @@ describe('Chat OTP API Route', () => { expect(response.status).toBe(200) await expect(response.json()).resolves.toEqual({ message: 'Verification code sent' }) + expect(mockAfterResponse).toHaveBeenCalledTimes(1) expect(mockCheckRateLimitDirect).not.toHaveBeenCalled() expect(mockRedisSet).not.toHaveBeenCalled() expect(mockSendEmail).not.toHaveBeenCalled() @@ -402,6 +416,7 @@ describe('Chat OTP API Route', () => { await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) }) + expect(mockAfterResponse).toHaveBeenCalledTimes(1) expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2) expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( 1, diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.ts b/apps/sim/app/api/chat/[identifier]/otp/route.ts index f681d6a4085..8a3747c5ae8 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.ts @@ -20,6 +20,7 @@ import { OTP_RESOURCE_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' +import { afterResponse } from '@/lib/core/utils/after-response' import { generateRequestId, getClientIp } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { sendEmail } from '@/lib/messaging/email/mailer' @@ -34,6 +35,45 @@ function otpRequestAccepted() { return createSuccessResponse({ message: 'Verification code sent' }) } +async function deliverOtp(requestId: string, deploymentId: string, title: string, email: string) { + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( + `chat-otp:resource:${deploymentId}`, + OTP_RESOURCE_RATE_LIMIT, + { failClosed: true } + ) + if (!resourceRateLimit.allowed) { + logger.warn(`[${requestId}] OTP resource rate limit exceeded for chat ${deploymentId}`) + return + } + + const emailRateLimit = await rateLimiter.checkRateLimitDirect( + `chat-otp:email:${deploymentId}:${email.toLowerCase()}`, + OTP_EMAIL_RATE_LIMIT, + { failClosed: true } + ) + if (!emailRateLimit.allowed) { + logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deploymentId}`) + return + } + + const otp = generateOTP() + await storeOTP('chat', deploymentId, email, otp) + + const emailHtml = await renderOTPEmail(otp, email, 'email-verification', title) + const emailResult = await sendEmail({ + to: email, + subject: getOtpSubject(title), + html: emailHtml, + }) + + if (!emailResult.success) { + logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message) + return + } + + logger.info(`[${requestId}] OTP sent to ${email} for chat ${deploymentId}`) +} + export const POST = withRouteHandler( async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => { const { identifier } = await context.params @@ -92,55 +132,12 @@ export const POST = withRouteHandler( const allowedEmails: string[] = Array.isArray(deployment.allowedEmails) ? deployment.allowedEmails : [] + const emailAllowed = isEmailAllowed(email, allowedEmails) - if (!isEmailAllowed(email, allowedEmails)) { - return otpRequestAccepted() - } - - const resourceRateLimit = await rateLimiter.checkRateLimitDirect( - `chat-otp:resource:${deployment.id}`, - OTP_RESOURCE_RATE_LIMIT, - { failClosed: true } - ) - if (!resourceRateLimit.allowed) { - logger.warn(`[${requestId}] OTP resource rate limit exceeded for chat ${deployment.id}`) - return otpRequestAccepted() - } - - const emailRateLimit = await rateLimiter.checkRateLimitDirect( - `chat-otp:email:${deployment.id}:${email.toLowerCase()}`, - OTP_EMAIL_RATE_LIMIT, - { failClosed: true } - ) - if (!emailRateLimit.allowed) { - logger.warn( - `[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deployment.id}` - ) - return otpRequestAccepted() - } - - const otp = generateOTP() - await storeOTP('chat', deployment.id, email, otp) - - const emailHtml = await renderOTPEmail( - otp, - email, - 'email-verification', - deployment.title || 'Chat' - ) - - const emailResult = await sendEmail({ - to: email, - subject: getOtpSubject(deployment.title || 'Chat'), - html: emailHtml, + afterResponse(async () => { + if (!emailAllowed) return + await deliverOtp(requestId, deployment.id, deployment.title || 'Chat', email) }) - - if (!emailResult.success) { - logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message) - return otpRequestAccepted() - } - - logger.info(`[${requestId}] OTP sent to ${email} for chat ${deployment.id}`) return otpRequestAccepted() } catch (error) { logger.error(`[${requestId}] Error processing OTP request:`, error) diff --git a/apps/sim/app/api/files/public/[token]/otp/route.test.ts b/apps/sim/app/api/files/public/[token]/otp/route.test.ts index 9cfd11f2083..94429804c40 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.test.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.test.ts @@ -18,6 +18,7 @@ const { mockRenderOTPEmail, mockSendEmail, mockCheckRateLimitDirect, + mockAfterResponse, } = vi.hoisted(() => ({ mockResolveActiveShareByToken: vi.fn(), mockIsEmailAllowed: vi.fn(), @@ -31,6 +32,7 @@ const { mockRenderOTPEmail: vi.fn(), mockSendEmail: vi.fn(), mockCheckRateLimitDirect: vi.fn(), + mockAfterResponse: vi.fn(), })) vi.mock('@/lib/public-shares/share-manager', () => ({ @@ -62,8 +64,18 @@ vi.mock('@/lib/core/rate-limiter', () => ({ checkRateLimitDirect = mockCheckRateLimitDirect }, })) +vi.mock('@/lib/core/utils/after-response', () => ({ + afterResponse: mockAfterResponse, +})) + +import { PUT, POST as routePost } from '@/app/api/files/public/[token]/otp/route' -import { POST, PUT } from '@/app/api/files/public/[token]/otp/route' +const POST: typeof routePost = async (...args) => { + const response = await routePost(...args) + const task = mockAfterResponse.mock.calls.at(-1)?.[0] as (() => Promise) | undefined + if (task) await task() + return response +} const params = (token = 'tok_1') => ({ params: Promise.resolve({ token }) }) const post = (email: string, token = 'tok_1') => @@ -98,6 +110,7 @@ describe('POST /api/files/public/[token]/otp', () => { it('sends a code to an allow-listed email', async () => { const res = await POST(post('user@acme.com'), params()) expect(res.status).toBe(200) + expect(mockAfterResponse).toHaveBeenCalledTimes(1) expect(mockStoreOTP).toHaveBeenCalledWith('file', 'sh_1', 'user@acme.com', '123456') expect(mockSendEmail).toHaveBeenCalled() }) @@ -120,6 +133,7 @@ describe('POST /api/files/public/[token]/otp', () => { expect(res.status).toBe(200) await expect(res.json()).resolves.toEqual({ message: 'Verification code sent' }) + expect(mockAfterResponse).toHaveBeenCalledTimes(1) expect(mockCheckRateLimitDirect).not.toHaveBeenCalled() expect(mockStoreOTP).not.toHaveBeenCalled() expect(mockSendEmail).not.toHaveBeenCalled() diff --git a/apps/sim/app/api/files/public/[token]/otp/route.ts b/apps/sim/app/api/files/public/[token]/otp/route.ts index 85ef66e1757..86c871375e1 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.ts @@ -22,6 +22,7 @@ import { OTP_RESOURCE_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' +import { afterResponse } from '@/lib/core/utils/after-response' import { generateRequestId, getClientIp } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { sendEmail } from '@/lib/messaging/email/mailer' @@ -53,6 +54,44 @@ function otpRequestAccepted(): NextResponse { return NextResponse.json({ message: 'Verification code sent' }) } +async function deliverOtp(requestId: string, shareId: string, email: string): Promise { + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( + `file-otp:resource:${shareId}`, + OTP_RESOURCE_RATE_LIMIT, + { failClosed: true } + ) + if (!resourceRateLimit.allowed) { + logger.warn(`[${requestId}] OTP resource rate limit exceeded for share ${shareId}`) + return + } + + const emailRateLimit = await rateLimiter.checkRateLimitDirect( + `file-otp:email:${shareId}:${email}`, + OTP_EMAIL_RATE_LIMIT, + { failClosed: true } + ) + if (!emailRateLimit.allowed) { + logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email}`) + return + } + + const otp = generateOTP() + await storeOTP('file', shareId, email, otp) + + const emailHtml = await renderOTPEmail(otp, email, 'email-verification', SHARE_EMAIL_LABEL) + const emailResult = await sendEmail({ + to: email, + subject: getOtpSubject(SHARE_EMAIL_LABEL), + html: emailHtml, + }) + if (!emailResult.success) { + logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message) + return + } + + logger.info(`[${requestId}] OTP sent for share ${shareId}`) +} + /** * POST /api/files/public/[token]/otp * Sends a 6-digit verification code to an allow-listed email for an email-gated share. @@ -92,48 +131,12 @@ export const POST = withRouteHandler( { status: 400 } ) } + const emailAllowed = isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails)) - if (!isEmailAllowed(email, shareAllowedEmails(resolved.share.allowedEmails))) { - return otpRequestAccepted() - } - - const resourceRateLimit = await rateLimiter.checkRateLimitDirect( - `file-otp:resource:${resolved.share.id}`, - OTP_RESOURCE_RATE_LIMIT, - { failClosed: true } - ) - if (!resourceRateLimit.allowed) { - logger.warn( - `[${requestId}] OTP resource rate limit exceeded for share ${resolved.share.id}` - ) - return otpRequestAccepted() - } - - const emailRateLimit = await rateLimiter.checkRateLimitDirect( - `file-otp:email:${resolved.share.id}:${email}`, - OTP_EMAIL_RATE_LIMIT, - { failClosed: true } - ) - if (!emailRateLimit.allowed) { - logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email}`) - return otpRequestAccepted() - } - - const otp = generateOTP() - await storeOTP('file', resolved.share.id, email, otp) - - const emailHtml = await renderOTPEmail(otp, email, 'email-verification', SHARE_EMAIL_LABEL) - const emailResult = await sendEmail({ - to: email, - subject: getOtpSubject(SHARE_EMAIL_LABEL), - html: emailHtml, + afterResponse(async () => { + if (!emailAllowed) return + await deliverOtp(requestId, resolved.share.id, email) }) - if (!emailResult.success) { - logger.error(`[${requestId}] Failed to send OTP email:`, emailResult.message) - return otpRequestAccepted() - } - - logger.info(`[${requestId}] OTP sent for share ${resolved.share.id}`) return otpRequestAccepted() } catch (error) { logger.error(`[${requestId}] Error processing OTP request:`, error) diff --git a/apps/sim/lib/core/utils/after-response.ts b/apps/sim/lib/core/utils/after-response.ts new file mode 100644 index 00000000000..97dff377762 --- /dev/null +++ b/apps/sim/lib/core/utils/after-response.ts @@ -0,0 +1,5 @@ +import { after } from 'next/server' + +export function afterResponse(task: () => Promise): void { + after(task) +}