Skip to content

Commit 4906c7e

Browse files
TheodoreSpeaksTheodoreSpeaks
committed
fix(security): harden public auth rate limits
1 parent 2c61b29 commit 4906c7e

40 files changed

Lines changed: 730 additions & 214 deletions

File tree

‎apps/docs/openapi-v2-files-audit.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3518,7 +3518,7 @@
35183518
"password": {
35193519
"description": "Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400.",
35203520
"type": "string",
3521-
"minLength": 1,
3521+
"minLength": 15,
35223522
"maxLength": 1024
35233523
},
35243524
"allowedEmails": {

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

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -343,8 +343,8 @@ describe('Chat OTP API Route', () => {
343343
expect(headerSet).toHaveBeenCalledWith('Retry-After', '900')
344344
})
345345

346-
it('folds spoofed `unknown` client IPs into a single shared bucket', async () => {
347-
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce('unknown')
346+
it('skips the IP bucket when the client IP cannot be resolved', async () => {
347+
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
348348
queueDeployment(emailDeployment)
349349

350350
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
@@ -354,14 +354,11 @@ describe('Chat OTP API Route', () => {
354354

355355
await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
356356

357-
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2)
358-
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
359-
expect.stringMatching(/^chat-otp:ip:.*:unknown$/),
360-
expect.any(Object)
361-
)
357+
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1)
362358
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
363359
expect.stringContaining('chat-otp:email:'),
364-
expect.any(Object)
360+
expect.any(Object),
361+
{ failClosed: true }
365362
)
366363
})
367364
})

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

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,21 @@ export const POST = withRouteHandler(
3636

3737
try {
3838
const ip = getClientIp(request)
39-
const ipRateLimit = await rateLimiter.checkRateLimitDirect(
40-
`chat-otp:ip:${identifier}:${ip}`,
41-
OTP_IP_RATE_LIMIT
42-
)
43-
if (!ipRateLimit.allowed) {
44-
logger.warn(`[${requestId}] OTP IP rate limit exceeded for ${identifier} from ${ip}`)
45-
const retryAfter = Math.ceil(
46-
(ipRateLimit.retryAfterMs ?? OTP_IP_RATE_LIMIT.refillIntervalMs) / 1000
39+
if (ip) {
40+
const ipRateLimit = await rateLimiter.checkRateLimitDirect(
41+
`chat-otp:ip:${identifier}:${ip}`,
42+
OTP_IP_RATE_LIMIT,
43+
{ failClosed: true }
4744
)
48-
const response = createErrorResponse('Too many requests. Please try again later.', 429)
49-
response.headers.set('Retry-After', String(retryAfter))
50-
return response
45+
if (!ipRateLimit.allowed) {
46+
logger.warn(`[${requestId}] OTP IP rate limit exceeded for ${identifier} from ${ip}`)
47+
const retryAfter = Math.ceil(
48+
(ipRateLimit.retryAfterMs ?? OTP_IP_RATE_LIMIT.refillIntervalMs) / 1000
49+
)
50+
const response = createErrorResponse('Too many requests. Please try again later.', 429)
51+
response.headers.set('Retry-After', String(retryAfter))
52+
return response
53+
}
5154
}
5255

5356
const parsed = await parseRequest(requestChatEmailOtpContract, request, context, {
@@ -91,7 +94,8 @@ export const POST = withRouteHandler(
9194

9295
const emailRateLimit = await rateLimiter.checkRateLimitDirect(
9396
`chat-otp:email:${deployment.id}:${email.toLowerCase()}`,
94-
OTP_EMAIL_RATE_LIMIT
97+
OTP_EMAIL_RATE_LIMIT,
98+
{ failClosed: true }
9599
)
96100
if (!emailRateLimit.allowed) {
97101
logger.warn(

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

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { queueTableRows, requestUtilsMockFns, resetDbChainMock, schemaMock } from '@sim/testing'
5+
import { NextRequest } from 'next/server'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
const { mockIsEmailAllowed, mockCheckRateLimitDirect } = vi.hoisted(() => ({
9+
mockIsEmailAllowed: vi.fn(),
10+
mockCheckRateLimitDirect: vi.fn(),
11+
}))
12+
13+
vi.mock('@/lib/core/security/deployment', () => ({ isEmailAllowed: mockIsEmailAllowed }))
14+
vi.mock('@/lib/core/rate-limiter', () => ({
15+
RateLimiter: class {
16+
checkRateLimitDirect = mockCheckRateLimitDirect
17+
},
18+
}))
19+
20+
import { POST } from '@/app/api/chat/[identifier]/sso/route'
21+
22+
const deployment = {
23+
id: 'chat-1',
24+
authType: 'sso',
25+
allowedEmails: ['@acme.com'],
26+
isActive: true,
27+
}
28+
29+
function post(email: string): NextRequest {
30+
return new NextRequest('http://localhost/api/chat/support/sso', {
31+
method: 'POST',
32+
headers: { 'content-type': 'application/json' },
33+
body: JSON.stringify({ email }),
34+
})
35+
}
36+
37+
const context = { params: Promise.resolve({ identifier: 'support' }) }
38+
39+
describe('POST /api/chat/[identifier]/sso', () => {
40+
beforeEach(() => {
41+
vi.clearAllMocks()
42+
resetDbChainMock()
43+
queueTableRows(schemaMock.chat, [deployment])
44+
requestUtilsMockFns.mockGetClientIp.mockReturnValue('127.0.0.1')
45+
mockCheckRateLimitDirect.mockResolvedValue({ allowed: true })
46+
mockIsEmailAllowed.mockReturnValue(true)
47+
})
48+
49+
it('applies both client-IP and chat-resource limits', async () => {
50+
const response = await POST(post('user@acme.com'), context)
51+
52+
expect(response.status).toBe(200)
53+
expect(await response.json()).toEqual({ eligible: true })
54+
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
55+
1,
56+
'chat-sso:ip:127.0.0.1',
57+
expect.objectContaining({ maxTokens: 20 }),
58+
{ failClosed: true }
59+
)
60+
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
61+
2,
62+
'chat-sso:resource:chat-1',
63+
expect.objectContaining({ maxTokens: 100 }),
64+
{ failClosed: true }
65+
)
66+
})
67+
68+
it('returns 429 when the chat-resource limit is exceeded', async () => {
69+
mockCheckRateLimitDirect
70+
.mockResolvedValueOnce({ allowed: true })
71+
.mockResolvedValueOnce({ allowed: false, retryAfterMs: 3000 })
72+
73+
const response = await POST(post('user@acme.com'), context)
74+
75+
expect(response.status).toBe(429)
76+
expect(response.headers.get('Retry-After')).toBe('3')
77+
})
78+
79+
it('retains the chat-resource limit when the client IP cannot be resolved', async () => {
80+
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
81+
82+
const response = await POST(post('user@acme.com'), context)
83+
84+
expect(response.status).toBe(200)
85+
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1)
86+
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
87+
'chat-sso:resource:chat-1',
88+
expect.objectContaining({ maxTokens: 100 }),
89+
{ failClosed: true }
90+
)
91+
})
92+
})

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

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,23 +25,33 @@ const SSO_IP_RATE_LIMIT: TokenBucketConfig = {
2525
refillIntervalMs: 15 * 60_000,
2626
}
2727

28+
const SSO_RESOURCE_RATE_LIMIT: TokenBucketConfig = {
29+
maxTokens: 100,
30+
refillRate: 100,
31+
refillIntervalMs: 15 * 60_000,
32+
}
33+
34+
function rateLimited(retryAfterMs: number | undefined, fallbackMs: number) {
35+
const response = createErrorResponse('Too many requests. Please try again later.', 429)
36+
response.headers.set('Retry-After', String(Math.ceil((retryAfterMs ?? fallbackMs) / 1000)))
37+
return response
38+
}
39+
2840
export const POST = withRouteHandler(
2941
async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => {
3042
const requestId = generateRequestId()
3143

3244
const ip = getClientIp(request)
33-
const ipRateLimit = await rateLimiter.checkRateLimitDirect(
34-
`chat-sso:ip:${ip}`,
35-
SSO_IP_RATE_LIMIT
36-
)
37-
if (!ipRateLimit.allowed) {
38-
logger.warn(`[${requestId}] SSO eligibility rate limit exceeded from ${ip}`)
39-
const retryAfter = Math.ceil(
40-
(ipRateLimit.retryAfterMs ?? SSO_IP_RATE_LIMIT.refillIntervalMs) / 1000
45+
if (ip) {
46+
const ipRateLimit = await rateLimiter.checkRateLimitDirect(
47+
`chat-sso:ip:${ip}`,
48+
SSO_IP_RATE_LIMIT,
49+
{ failClosed: true }
4150
)
42-
const response = createErrorResponse('Too many requests. Please try again later.', 429)
43-
response.headers.set('Retry-After', String(retryAfter))
44-
return response
51+
if (!ipRateLimit.allowed) {
52+
logger.warn(`[${requestId}] SSO eligibility rate limit exceeded from ${ip}`)
53+
return rateLimited(ipRateLimit.retryAfterMs, SSO_IP_RATE_LIMIT.refillIntervalMs)
54+
}
4555
}
4656

4757
const parsed = await parseRequest(chatSSOContract, request, context)
@@ -52,6 +62,7 @@ export const POST = withRouteHandler(
5262

5363
const [deployment] = await db
5464
.select({
65+
id: chat.id,
5566
authType: chat.authType,
5667
allowedEmails: chat.allowedEmails,
5768
isActive: chat.isActive,
@@ -69,6 +80,18 @@ export const POST = withRouteHandler(
6980
return createErrorResponse('Chat is not configured for SSO authentication', 400)
7081
}
7182

83+
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
84+
`chat-sso:resource:${deployment.id}`,
85+
SSO_RESOURCE_RATE_LIMIT,
86+
{ failClosed: true }
87+
)
88+
if (!resourceRateLimit.allowed) {
89+
logger.warn(`[${requestId}] SSO eligibility resource rate limit exceeded`, {
90+
deploymentId: deployment.id,
91+
})
92+
return rateLimited(resourceRateLimit.retryAfterMs, SSO_RESOURCE_RATE_LIMIT.refillIntervalMs)
93+
}
94+
7295
const eligible = isEmailAllowed(email, (deployment.allowedEmails as string[]) || [])
7396

7497
return createSuccessResponse({ eligible })

‎apps/sim/app/api/chat/utils.test.ts‎

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
encryptionMock,
99
encryptionMockFns,
1010
loggingSessionMock,
11+
requestUtilsMockFns,
1112
workflowsUtilsMock,
1213
} from '@sim/testing'
1314
import type { NextResponse } from 'next/server'
@@ -208,6 +209,18 @@ describe('Chat API Utils', () => {
208209

209210
const result = await validateChatAuth('request-id', deployment, mockRequest, parsedBody)
210211

212+
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
213+
1,
214+
'chat-password:ip:chat-id:127.0.0.1',
215+
expect.objectContaining({ maxTokens: 10 }),
216+
{ failClosed: true }
217+
)
218+
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
219+
2,
220+
'chat-password:resource:chat-id',
221+
expect.objectContaining({ maxTokens: 100 }),
222+
{ failClosed: true }
223+
)
211224
expect(decryptSecret).toHaveBeenCalledWith('encrypted-password')
212225
expect(result.authorized).toBe(true)
213226
})
@@ -236,7 +249,7 @@ describe('Chat API Utils', () => {
236249
expect(result.error).toBe('Invalid password')
237250
})
238251

239-
it('should return 429 when the password attempt rate limit is exceeded', async () => {
252+
it('should return 429 when the password IP rate limit is exceeded', async () => {
240253
mockCheckRateLimitDirect.mockResolvedValueOnce({ allowed: false, retryAfterMs: 60_000 })
241254

242255
const deployment = {
@@ -260,6 +273,67 @@ describe('Chat API Utils', () => {
260273
expect(result.status).toBe(429)
261274
expect(result.retryAfterMs).toBe(60_000)
262275
expect(decryptSecret).not.toHaveBeenCalled()
276+
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
277+
'chat-password:ip:chat-id:127.0.0.1',
278+
expect.objectContaining({ maxTokens: 10 }),
279+
{ failClosed: true }
280+
)
281+
})
282+
283+
it('should return 429 when the password resource rate limit is exceeded', async () => {
284+
mockCheckRateLimitDirect
285+
.mockResolvedValueOnce({ allowed: true })
286+
.mockResolvedValueOnce({ allowed: false, retryAfterMs: 30_000 })
287+
288+
const deployment = {
289+
id: 'chat-id',
290+
authType: 'password',
291+
password: 'encrypted-password',
292+
}
293+
const mockRequest = {
294+
method: 'POST',
295+
cookies: { get: vi.fn().mockReturnValue(null) },
296+
} as any
297+
298+
const result = await validateChatAuth('request-id', deployment, mockRequest, {
299+
password: 'any-guess',
300+
})
301+
302+
expect(result).toEqual(
303+
expect.objectContaining({ authorized: false, status: 429, retryAfterMs: 30_000 })
304+
)
305+
expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith(
306+
2,
307+
'chat-password:resource:chat-id',
308+
expect.objectContaining({ maxTokens: 100 }),
309+
{ failClosed: true }
310+
)
311+
expect(decryptSecret).not.toHaveBeenCalled()
312+
})
313+
314+
it('should retain the password resource limit when the client IP cannot be resolved', async () => {
315+
requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null)
316+
const deployment = {
317+
id: 'chat-id',
318+
authType: 'password',
319+
password: 'encrypted-password',
320+
}
321+
const mockRequest = {
322+
method: 'POST',
323+
cookies: { get: vi.fn().mockReturnValue(null) },
324+
} as any
325+
326+
const result = await validateChatAuth('request-id', deployment, mockRequest, {
327+
password: 'correct-password',
328+
})
329+
330+
expect(result.authorized).toBe(true)
331+
expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1)
332+
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
333+
'chat-password:resource:chat-id',
334+
expect.objectContaining({ maxTokens: 100 }),
335+
{ failClosed: true }
336+
)
263337
})
264338

265339
it('should request email auth for email-protected chats', async () => {

0 commit comments

Comments
 (0)