fix(security): address greptile feedback on OTP rate limiting

Author: waleedlatif1

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

@@ -112,11 +112,13 @@ vi.mock('@/lib/core/storage', () => ({
   getStorageMethod: mockGetStorageMethod,
 }))
 
+const { mockCheckRateLimitDirect } = vi.hoisted(() => ({
+  mockCheckRateLimitDirect: vi.fn(),
+}))
+
 vi.mock('@/lib/core/rate-limiter', () => ({
   RateLimiter: class {
-    async checkRateLimitDirect() {
-      return { allowed: true, remaining: 10, resetAt: new Date(Date.now() + 60_000) }
-    }
+    checkRateLimitDirect = mockCheckRateLimitDirect
   },
 }))
 
@@ -242,6 +244,13 @@ describe('Chat OTP API Route', () => {
     }))
 
     requestUtilsMockFns.mockGenerateRequestId.mockReturnValue('req-123')
+    requestUtilsMockFns.mockGetClientIp.mockReturnValue('1.2.3.4')
+
+    mockCheckRateLimitDirect.mockResolvedValue({
+      allowed: true,
+      remaining: 10,
+      resetAt: new Date(Date.now() + 60_000),
+    })
 
     mockZodParse.mockImplementation((data: unknown) => data)
 
@@ -291,6 +300,134 @@ describe('Chat OTP API Route', () => {
     })
   })
 
+  describe('POST - Rate limiting', () => {
+    const buildDeploymentSelect = () =>
+      mockDbSelect.mockImplementationOnce(() => ({
+        from: vi.fn().mockReturnValue({
+          where: vi.fn().mockReturnValue({
+            limit: vi.fn().mockResolvedValue([
+              {
+                id: mockChatId,
+                authType: 'email',
+                allowedEmails: [mockEmail],
+                title: 'Test Chat',
+              },
+            ]),
+          }),
+        }),
+      }))
+
+    it('returns 429 with Retry-After when IP rate limit is exceeded', async () => {
+      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 },
+      }))
+
+      const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
+        method: 'POST',
+        body: JSON.stringify({ email: mockEmail }),
+      })
+
+      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()
+      expect(mockDbSelect).not.toHaveBeenCalled()
+    })
+
+    it('returns 429 with Retry-After when email rate limit is exceeded', async () => {
+      mockCheckRateLimitDirect
+        .mockResolvedValueOnce({
+          allowed: true,
+          remaining: 9,
+          resetAt: new Date(Date.now() + 60_000),
+        })
+        .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 },
+      }))
+
+      buildDeploymentSelect()
+
+      const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
+        method: 'POST',
+        body: JSON.stringify({ email: mockEmail }),
+      })
+
+      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('falls back to refill interval when retryAfterMs is missing', async () => {
+      mockCheckRateLimitDirect.mockResolvedValueOnce({
+        allowed: false,
+        remaining: 0,
+        resetAt: new Date(Date.now() + 900_000),
+      })
+
+      const headerSet = vi.fn()
+      mockCreateErrorResponse.mockImplementationOnce((message: string, status: number) => ({
+        json: () => Promise.resolve({ error: message }),
+        status,
+        headers: { set: headerSet },
+      }))
+
+      const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
+        method: 'POST',
+        body: JSON.stringify({ email: mockEmail }),
+      })
+
+      await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
+
+      expect(headerSet).toHaveBeenCalledWith('Retry-After', '900')
+    })
+
+    it('skips IP rate limit when client IP is unknown', async () => {
+      requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce('unknown')
+      buildDeploymentSelect()
+
+      const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
+        method: 'POST',
+        body: JSON.stringify({ email: mockEmail }),
+      })
+
+      await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
+
+      // Only the email-scoped check should run, not the IP-scoped one
+      expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1)
+      expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
+        expect.stringContaining('chat-otp:email:'),
+        expect.any(Object)
+      )
+    })
+  })
+
   describe('POST - Store OTP (Database path)', () => {
     beforeEach(() => {
       mockGetStorageMethod.mockReturnValue('database')

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

@@ -231,16 +231,20 @@ export const POST = withRouteHandler(
 
     try {
       const ip = getClientIp(request)
-      const ipRateLimit = await rateLimiter.checkRateLimitDirect(
-        `chat-otp:ip:${identifier}:${ip}`,
-        OTP_IP_RATE_LIMIT
-      )
-      if (!ipRateLimit.allowed) {
-        logger.warn(`[${requestId}] OTP IP rate limit exceeded for ${identifier} from ${ip}`)
-        const retryAfter = Math.ceil((ipRateLimit.retryAfterMs ?? 60_000) / 1000)
-        const response = createErrorResponse('Too many requests. Please try again later.', 429)
-        response.headers.set('Retry-After', String(retryAfter))
-        return addCorsHeaders(response, request)
+      if (ip !== 'unknown') {
+        const ipRateLimit = await rateLimiter.checkRateLimitDirect(
+          `chat-otp:ip:${identifier}:${ip}`,
+          OTP_IP_RATE_LIMIT
+        )
+        if (!ipRateLimit.allowed) {
+          logger.warn(`[${requestId}] OTP IP rate limit exceeded for ${identifier} from ${ip}`)
+          const retryAfter = Math.ceil(
+            (ipRateLimit.retryAfterMs ?? OTP_IP_RATE_LIMIT.refillIntervalMs) / 1000
+          )
+          const response = createErrorResponse('Too many requests. Please try again later.', 429)
+          response.headers.set('Retry-After', String(retryAfter))
+          return addCorsHeaders(response, request)
+        }
       }
 
       const body = await request.json()
@@ -292,7 +296,9 @@ export const POST = withRouteHandler(
         logger.warn(
           `[${requestId}] OTP email rate limit exceeded for ${email} on chat ${deployment.id}`
         )
-        const retryAfter = Math.ceil((emailRateLimit.retryAfterMs ?? 60_000) / 1000)
+        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