diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index b9a51bceab7..6c31e2db830 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -3518,7 +3518,7 @@ "password": { "description": "Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400.", "type": "string", - "minLength": 1, + "minLength": 15, "maxLength": 1024 }, "allowedEmails": { diff --git a/apps/sim/app/api/auth/forget-password/route.test.ts b/apps/sim/app/api/auth/forget-password/route.test.ts index 44e40cda51c..ed7d96fa86d 100644 --- a/apps/sim/app/api/auth/forget-password/route.test.ts +++ b/apps/sim/app/api/auth/forget-password/route.test.ts @@ -3,7 +3,7 @@ * * @vitest-environment node */ -import { createMockRequest, resetEnvMock, setEnv } from '@sim/testing' +import { createMockRequest, requestUtilsMockFns, resetEnvMock, setEnv } from '@sim/testing' import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { mockCheckRateLimitDirect } = vi.hoisted(() => ({ @@ -138,6 +138,16 @@ describe('Forget Password API Route', () => { expect(mockRequestPasswordReset).not.toHaveBeenCalled() }) + it('uses the recipient backstop when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + + const response = await POST(createMockRequest('POST', { email: 'test@example.com' })) + + expect(response.status).toBe(200) + expect(recipientKeys()).toHaveLength(1) + expect(mockRequestPasswordReset).toHaveBeenCalledOnce() + }) + it('should reject external redirectTo URL', async () => { const req = createMockRequest('POST', { email: 'test@example.com', diff --git a/apps/sim/app/api/auth/forget-password/route.ts b/apps/sim/app/api/auth/forget-password/route.ts index 43e2e2e0c60..4eaf161b37f 100644 --- a/apps/sim/app/api/auth/forget-password/route.ts +++ b/apps/sim/app/api/auth/forget-password/route.ts @@ -8,7 +8,7 @@ import { forgetPasswordContract } from '@/lib/api/contracts' import { getValidationErrorMessage, parseRequest } from '@/lib/api/server' import { auth } from '@/lib/auth' import { - enforceIpRateLimit, + enforceIpRateLimitWithIndependentBackstop, enforceRecipientRateLimit, type TokenBucketConfig, } from '@/lib/core/rate-limiter' @@ -27,7 +27,10 @@ const RESET_EMAIL_RATE_LIMIT: TokenBucketConfig = { export const POST = withRouteHandler(async (request: NextRequest) => { try { - const ipRateLimited = await enforceIpRateLimit('forget-password', request) + const ipRateLimited = await enforceIpRateLimitWithIndependentBackstop( + 'forget-password', + request + ) if (ipRateLimited) return ipRateLimited const parsed = await parseRequest( 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 e49b485cca3..b882ed310ff 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts @@ -283,6 +283,48 @@ describe('Chat OTP API Route', () => { }) 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: true, + remaining: 99, + 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 }, + })) + + queueDeployment(emailDeployment) + + 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('returns 429 with Retry-After when the chat resource rate limit is exceeded', async () => { mockCheckRateLimitDirect .mockResolvedValueOnce({ allowed: true, @@ -343,8 +385,8 @@ describe('Chat OTP API Route', () => { expect(headerSet).toHaveBeenCalledWith('Retry-After', '900') }) - it('folds spoofed `unknown` client IPs into a single shared bucket', async () => { - requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce('unknown') + it('retains resource and email backstops when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) queueDeployment(emailDeployment) const request = new NextRequest('http://localhost:3000/api/chat/test/otp', { @@ -355,13 +397,17 @@ describe('Chat OTP API Route', () => { await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) }) expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2) - expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( - expect.stringMatching(/^chat-otp:ip:.*:unknown$/), - expect.any(Object) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 1, + 'chat-otp:resource:chat-123', + expect.any(Object), + { failClosed: true } ) - expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 2, expect.stringContaining('chat-otp:email:'), - expect.any(Object) + expect.any(Object), + { failClosed: true } ) }) }) diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.ts b/apps/sim/app/api/chat/[identifier]/otp/route.ts index aa936877cc5..0b778675ccd 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.ts @@ -17,6 +17,7 @@ import { MAX_OTP_ATTEMPTS, OTP_EMAIL_RATE_LIMIT, OTP_IP_RATE_LIMIT, + OTP_RESOURCE_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' import { generateRequestId, getClientIp } from '@/lib/core/utils/request' @@ -36,18 +37,21 @@ 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 ?? OTP_IP_RATE_LIMIT.refillIntervalMs) / 1000 + if (ip) { + const ipRateLimit = await rateLimiter.checkRateLimitDirect( + `chat-otp:ip:${identifier}:${ip}`, + OTP_IP_RATE_LIMIT, + { failClosed: true } ) - const response = createErrorResponse('Too many requests. Please try again later.', 429) - response.headers.set('Retry-After', String(retryAfter)) - return response + 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 response + } } const parsed = await parseRequest(requestChatEmailOtpContract, request, context, { @@ -85,13 +89,32 @@ export const POST = withRouteHandler( ? deployment.allowedEmails : [] + 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}`) + 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 + } + 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 + OTP_EMAIL_RATE_LIMIT, + { failClosed: true } ) if (!emailRateLimit.allowed) { logger.warn( diff --git a/apps/sim/app/api/chat/[identifier]/sso/route.test.ts b/apps/sim/app/api/chat/[identifier]/sso/route.test.ts new file mode 100644 index 00000000000..57156d93981 --- /dev/null +++ b/apps/sim/app/api/chat/[identifier]/sso/route.test.ts @@ -0,0 +1,92 @@ +/** + * @vitest-environment node + */ +import { queueTableRows, requestUtilsMockFns, resetDbChainMock, schemaMock } from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockIsEmailAllowed, mockCheckRateLimitDirect } = vi.hoisted(() => ({ + mockIsEmailAllowed: vi.fn(), + mockCheckRateLimitDirect: vi.fn(), +})) + +vi.mock('@/lib/core/security/deployment', () => ({ isEmailAllowed: mockIsEmailAllowed })) +vi.mock('@/lib/core/rate-limiter', () => ({ + RateLimiter: class { + checkRateLimitDirect = mockCheckRateLimitDirect + }, +})) + +import { POST } from '@/app/api/chat/[identifier]/sso/route' + +const deployment = { + id: 'chat-1', + authType: 'sso', + allowedEmails: ['@acme.com'], + isActive: true, +} + +function post(email: string): NextRequest { + return new NextRequest('http://localhost/api/chat/support/sso', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ email }), + }) +} + +const context = { params: Promise.resolve({ identifier: 'support' }) } + +describe('POST /api/chat/[identifier]/sso', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + queueTableRows(schemaMock.chat, [deployment]) + requestUtilsMockFns.mockGetClientIp.mockReturnValue('127.0.0.1') + mockCheckRateLimitDirect.mockResolvedValue({ allowed: true }) + mockIsEmailAllowed.mockReturnValue(true) + }) + + it('applies both client-IP and chat-resource limits', async () => { + const response = await POST(post('user@acme.com'), context) + + expect(response.status).toBe(200) + expect(await response.json()).toEqual({ eligible: true }) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 1, + 'chat-sso:ip:127.0.0.1', + expect.objectContaining({ maxTokens: 20 }), + { failClosed: true } + ) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 2, + 'chat-sso:resource:chat-1', + expect.objectContaining({ maxTokens: 100 }), + { failClosed: true } + ) + }) + + it('returns 429 when the chat-resource limit is exceeded', async () => { + mockCheckRateLimitDirect + .mockResolvedValueOnce({ allowed: true }) + .mockResolvedValueOnce({ allowed: false, retryAfterMs: 3000 }) + + const response = await POST(post('user@acme.com'), context) + + expect(response.status).toBe(429) + expect(response.headers.get('Retry-After')).toBe('3') + }) + + it('retains the chat-resource limit when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + + const response = await POST(post('user@acme.com'), context) + + expect(response.status).toBe(200) + expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1) + expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + 'chat-sso:resource:chat-1', + expect.objectContaining({ maxTokens: 100 }), + { failClosed: true } + ) + }) +}) diff --git a/apps/sim/app/api/chat/[identifier]/sso/route.ts b/apps/sim/app/api/chat/[identifier]/sso/route.ts index c6ab98cfe94..d29f66789f6 100644 --- a/apps/sim/app/api/chat/[identifier]/sso/route.ts +++ b/apps/sim/app/api/chat/[identifier]/sso/route.ts @@ -25,23 +25,33 @@ const SSO_IP_RATE_LIMIT: TokenBucketConfig = { refillIntervalMs: 15 * 60_000, } +const SSO_RESOURCE_RATE_LIMIT: TokenBucketConfig = { + maxTokens: 100, + refillRate: 100, + refillIntervalMs: 15 * 60_000, +} + +function rateLimited(retryAfterMs: number | undefined, fallbackMs: number) { + const response = createErrorResponse('Too many requests. Please try again later.', 429) + response.headers.set('Retry-After', String(Math.ceil((retryAfterMs ?? fallbackMs) / 1000))) + return response +} + export const POST = withRouteHandler( async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => { const requestId = generateRequestId() const ip = getClientIp(request) - const ipRateLimit = await rateLimiter.checkRateLimitDirect( - `chat-sso:ip:${ip}`, - SSO_IP_RATE_LIMIT - ) - if (!ipRateLimit.allowed) { - logger.warn(`[${requestId}] SSO eligibility rate limit exceeded from ${ip}`) - const retryAfter = Math.ceil( - (ipRateLimit.retryAfterMs ?? SSO_IP_RATE_LIMIT.refillIntervalMs) / 1000 + if (ip) { + const ipRateLimit = await rateLimiter.checkRateLimitDirect( + `chat-sso:ip:${ip}`, + SSO_IP_RATE_LIMIT, + { failClosed: true } ) - const response = createErrorResponse('Too many requests. Please try again later.', 429) - response.headers.set('Retry-After', String(retryAfter)) - return response + if (!ipRateLimit.allowed) { + logger.warn(`[${requestId}] SSO eligibility rate limit exceeded from ${ip}`) + return rateLimited(ipRateLimit.retryAfterMs, SSO_IP_RATE_LIMIT.refillIntervalMs) + } } const parsed = await parseRequest(chatSSOContract, request, context) @@ -52,6 +62,7 @@ export const POST = withRouteHandler( const [deployment] = await db .select({ + id: chat.id, authType: chat.authType, allowedEmails: chat.allowedEmails, isActive: chat.isActive, @@ -69,6 +80,18 @@ export const POST = withRouteHandler( return createErrorResponse('Chat is not configured for SSO authentication', 400) } + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( + `chat-sso:resource:${deployment.id}`, + SSO_RESOURCE_RATE_LIMIT, + { failClosed: true } + ) + if (!resourceRateLimit.allowed) { + logger.warn(`[${requestId}] SSO eligibility resource rate limit exceeded`, { + deploymentId: deployment.id, + }) + return rateLimited(resourceRateLimit.retryAfterMs, SSO_RESOURCE_RATE_LIMIT.refillIntervalMs) + } + const eligible = isEmailAllowed(email, (deployment.allowedEmails as string[]) || []) return createSuccessResponse({ eligible }) diff --git a/apps/sim/app/api/chat/utils.test.ts b/apps/sim/app/api/chat/utils.test.ts index 6c41eeb21cc..5b5675763a1 100644 --- a/apps/sim/app/api/chat/utils.test.ts +++ b/apps/sim/app/api/chat/utils.test.ts @@ -5,9 +5,11 @@ */ import { authMockFns, + createMockRequest, encryptionMock, encryptionMockFns, loggingSessionMock, + requestUtilsMockFns, workflowsUtilsMock, } from '@sim/testing' import type { NextResponse } from 'next/server' @@ -208,6 +210,18 @@ describe('Chat API Utils', () => { const result = await validateChatAuth('request-id', deployment, mockRequest, parsedBody) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 1, + 'chat-password:ip:chat-id:127.0.0.1', + expect.objectContaining({ maxTokens: 10 }), + { failClosed: true } + ) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 2, + 'chat-password:resource:chat-id', + expect.objectContaining({ maxTokens: 100 }), + { failClosed: true } + ) expect(decryptSecret).toHaveBeenCalledWith('encrypted-password') expect(result.authorized).toBe(true) }) @@ -236,7 +250,7 @@ describe('Chat API Utils', () => { expect(result.error).toBe('Invalid password') }) - it('should return 429 when the password attempt rate limit is exceeded', async () => { + it('should return 429 when the password IP rate limit is exceeded', async () => { mockCheckRateLimitDirect.mockResolvedValueOnce({ allowed: false, retryAfterMs: 60_000 }) const deployment = { @@ -260,6 +274,63 @@ describe('Chat API Utils', () => { expect(result.status).toBe(429) expect(result.retryAfterMs).toBe(60_000) expect(decryptSecret).not.toHaveBeenCalled() + expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + 'chat-password:ip:chat-id:127.0.0.1', + expect.objectContaining({ maxTokens: 10 }), + { failClosed: true } + ) + }) + + it('should return 429 when the password resource rate limit is exceeded', async () => { + mockCheckRateLimitDirect + .mockResolvedValueOnce({ allowed: true }) + .mockResolvedValueOnce({ allowed: false, retryAfterMs: 30_000 }) + + const deployment = { + id: 'chat-id', + authType: 'password', + password: 'encrypted-password', + } + const mockRequest = createMockRequest('POST') + const candidate = 'password-attempt-fixture' + + const result = await validateChatAuth('request-id', deployment, mockRequest, { + password: candidate, + }) + + expect(result).toEqual( + expect.objectContaining({ authorized: false, status: 429, retryAfterMs: 30_000 }) + ) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 2, + 'chat-password:resource:chat-id', + expect.objectContaining({ maxTokens: 100 }), + { failClosed: true } + ) + expect(decryptSecret).not.toHaveBeenCalled() + }) + + it('should retain the password resource limit when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + const deployment = { + id: 'chat-id', + authType: 'password', + password: 'encrypted-password', + } + const mockRequest = createMockRequest('POST') + const candidate = 'correct-password' + + const result = await validateChatAuth('request-id', deployment, mockRequest, { + password: candidate, + }) + + expect(result.authorized).toBe(true) + expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1) + expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + 'chat-password:resource:chat-id', + expect.objectContaining({ maxTokens: 100 }), + { failClosed: true } + ) }) it('should request email auth for email-protected chats', async () => { diff --git a/apps/sim/app/api/contact/route.ts b/apps/sim/app/api/contact/route.ts index 5d1c0404eba..69df8e97457 100644 --- a/apps/sim/app/api/contact/route.ts +++ b/apps/sim/app/api/contact/route.ts @@ -54,8 +54,11 @@ export const POST = withRouteHandler(async (req: NextRequest) => { try { const ip = getClientIp(req) + if (!ip) { + logger.warn(`[${requestId}] Unable to resolve client IP for public rate limit`) + return NextResponse.json(TOO_MANY_REQUESTS_RESPONSE, { status: 429 }) + } const storageKey = `public:contact:${ip}` - const { allowed, remaining, resetAt } = await rateLimiter.checkRateLimitDirect( storageKey, PUBLIC_ENDPOINT_RATE_LIMIT diff --git a/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.test.ts b/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.test.ts index acb2344a2b6..ff16fe62b7c 100644 --- a/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.test.ts +++ b/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.test.ts @@ -20,7 +20,7 @@ vi.mock('@/lib/credential-groups/application/public-enrollment', () => ({ })) vi.mock('@/lib/credential-groups/rate-limit', () => ({ - enforcePublicCredentialGroupIpRateLimit: mocks.ipRateLimit, + enforcePublicCredentialGroupOAuthStartIpRateLimit: mocks.ipRateLimit, enforceCredentialGroupEnrollmentOAuthRateLimit: mocks.enrollmentRateLimit, })) diff --git a/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.ts b/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.ts index 22921bc1f62..bc66315f68e 100644 --- a/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.ts +++ b/apps/sim/app/api/credential-groups/enroll/[token]/oauth/[optionId]/route.ts @@ -10,7 +10,7 @@ import { startPublicCredentialGroupOAuth } from '@/lib/credential-groups/applica import { CredentialGroupOAuthError } from '@/lib/credential-groups/provider-adapter' import { enforceCredentialGroupEnrollmentOAuthRateLimit, - enforcePublicCredentialGroupIpRateLimit, + enforcePublicCredentialGroupOAuthStartIpRateLimit, } from '@/lib/credential-groups/rate-limit' import { createCredentialGroupEnrollmentRedirect } from '@/app/api/credential-groups/enrollment-redirect' @@ -24,7 +24,7 @@ export const GET = withRouteHandler( request: NextRequest, context: { params: Promise<{ token: string; optionId: string }> } ) => { - const limited = await enforcePublicCredentialGroupIpRateLimit(request, 'oauth-start') + const limited = await enforcePublicCredentialGroupOAuthStartIpRateLimit(request) const parsed = await parseRequest(startCredentialGroupOAuthContract, request, context) if (!parsed.success) return limited ?? parsed.response diff --git a/apps/sim/app/api/demo-requests/route.ts b/apps/sim/app/api/demo-requests/route.ts index 7553239e7b2..56c1bbededb 100644 --- a/apps/sim/app/api/demo-requests/route.ts +++ b/apps/sim/app/api/demo-requests/route.ts @@ -28,8 +28,14 @@ export const POST = withRouteHandler(async (req: NextRequest) => { try { const ip = getClientIp(req) + if (!ip) { + logger.warn(`[${requestId}] Unable to resolve client IP for public rate limit`) + return NextResponse.json( + { error: 'Too many requests. Please try again later.' }, + { status: 429 } + ) + } const storageKey = `public:demo-request:${ip}` - const { allowed, remaining, resetAt } = await rateLimiter.checkRateLimitDirect( storageKey, PUBLIC_ENDPOINT_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 bce515c9237..a30aecc4bd9 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 @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { requestUtilsMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -49,6 +50,7 @@ vi.mock('@/lib/core/security/otp', () => ({ MAX_OTP_ATTEMPTS: 5, OTP_IP_RATE_LIMIT: { maxTokens: 10, refillRate: 10, refillIntervalMs: 1000 }, OTP_EMAIL_RATE_LIMIT: { maxTokens: 3, refillRate: 3, refillIntervalMs: 1000 }, + OTP_RESOURCE_RATE_LIMIT: { maxTokens: 100, refillRate: 100, refillIntervalMs: 1000 }, })) vi.mock('@/components/emails', () => ({ getOtpSubject: (label: string) => `Verification code for ${label}`, @@ -128,6 +130,40 @@ describe('POST /api/files/public/[token]/otp', () => { expect(res.status).toBe(429) expect(res.headers.get('Retry-After')).toBe('1') }) + + it('returns 429 when the share resource rate 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(mockStoreOTP).not.toHaveBeenCalled() + expect(mockSendEmail).not.toHaveBeenCalled() + }) + + it('retains resource and email backstops when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + + const res = await POST(post('user@acme.com'), params()) + + expect(res.status).toBe(200) + expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 1, + 'file-otp:resource:sh_1', + expect.any(Object), + { failClosed: true } + ) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 2, + 'file-otp:email:sh_1:user@acme.com', + expect.any(Object), + { failClosed: true } + ) + }) }) describe('PUT /api/files/public/[token]/otp', () => { 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 c6b556ad41d..ba5ae83593b 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.ts @@ -19,6 +19,7 @@ import { MAX_OTP_ATTEMPTS, OTP_EMAIL_RATE_LIMIT, OTP_IP_RATE_LIMIT, + OTP_RESOURCE_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' import { generateRequestId, getClientIp } from '@/lib/core/utils/request' @@ -58,13 +59,16 @@ export const POST = withRouteHandler( try { const ip = getClientIp(request) - const ipRateLimit = await rateLimiter.checkRateLimitDirect( - `file-otp:ip:${ip}`, - OTP_IP_RATE_LIMIT - ) - if (!ipRateLimit.allowed) { - logger.warn(`[${requestId}] OTP IP rate limit exceeded from ${ip}`) - return rateLimited(ipRateLimit.retryAfterMs, OTP_IP_RATE_LIMIT.refillIntervalMs) + if (ip) { + const ipRateLimit = await rateLimiter.checkRateLimitDirect( + `file-otp:ip:${ip}`, + OTP_IP_RATE_LIMIT, + { failClosed: true } + ) + if (!ipRateLimit.allowed) { + logger.warn(`[${requestId}] OTP IP rate limit exceeded from ${ip}`) + return rateLimited(ipRateLimit.retryAfterMs, OTP_IP_RATE_LIMIT.refillIntervalMs) + } } const parsed = await parseRequest(requestPublicFileOtpContract, request, context) @@ -85,13 +89,26 @@ export const POST = withRouteHandler( ) } + 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 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 + OTP_EMAIL_RATE_LIMIT, + { failClosed: true } ) if (!emailRateLimit.allowed) { logger.warn(`[${requestId}] OTP email rate limit exceeded for ${email}`) diff --git a/apps/sim/app/api/files/public/[token]/sso/route.test.ts b/apps/sim/app/api/files/public/[token]/sso/route.test.ts index 92d78cd8b13..f771ce021eb 100644 --- a/apps/sim/app/api/files/public/[token]/sso/route.test.ts +++ b/apps/sim/app/api/files/public/[token]/sso/route.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { requestUtilsMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -49,6 +50,18 @@ describe('POST /api/files/public/[token]/sso', () => { const res = await POST(post('user@acme.com'), params()) expect(res.status).toBe(200) expect(await res.json()).toEqual({ eligible: true }) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 1, + 'file-sso:ip:127.0.0.1', + expect.objectContaining({ maxTokens: 20 }), + { failClosed: true } + ) + expect(mockCheckRateLimitDirect).toHaveBeenNthCalledWith( + 2, + 'file-sso:resource:sh_1', + expect.objectContaining({ maxTokens: 100 }), + { failClosed: true } + ) }) it('returns eligible:false for a non-listed email', async () => { @@ -79,4 +92,29 @@ describe('POST /api/files/public/[token]/sso', () => { expect(res.status).toBe(429) expect(res.headers.get('Retry-After')).toBe('2') }) + + it('returns 429 when the share resource limit is exceeded', async () => { + mockCheckRateLimitDirect + .mockResolvedValueOnce({ allowed: true }) + .mockResolvedValueOnce({ allowed: false, retryAfterMs: 3000 }) + + const res = await POST(post('user@acme.com'), params()) + + expect(res.status).toBe(429) + expect(res.headers.get('Retry-After')).toBe('3') + }) + + it('uses the share resource limit when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + + const res = await POST(post('user@acme.com'), params()) + + expect(res.status).toBe(200) + expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(1) + expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + 'file-sso:resource:sh_1', + expect.objectContaining({ maxTokens: 100 }), + { failClosed: true } + ) + }) }) diff --git a/apps/sim/app/api/files/public/[token]/sso/route.ts b/apps/sim/app/api/files/public/[token]/sso/route.ts index b5185149440..bc94fdcd0b6 100644 --- a/apps/sim/app/api/files/public/[token]/sso/route.ts +++ b/apps/sim/app/api/files/public/[token]/sso/route.ts @@ -24,6 +24,21 @@ const SSO_IP_RATE_LIMIT: TokenBucketConfig = { refillIntervalMs: 15 * 60_000, } +const SSO_RESOURCE_RATE_LIMIT: TokenBucketConfig = { + maxTokens: 100, + refillRate: 100, + refillIntervalMs: 15 * 60_000, +} + +function rateLimited(retryAfterMs: number | undefined, fallbackMs: number): NextResponse { + const response = NextResponse.json( + { error: 'Too many requests. Please try again later.' }, + { status: 429 } + ) + response.headers.set('Retry-After', String(Math.ceil((retryAfterMs ?? fallbackMs) / 1000))) + return response +} + /** * POST /api/files/public/[token]/sso * Reports whether an email is on the allow-list for an SSO-gated share. The actual @@ -34,21 +49,16 @@ export const POST = withRouteHandler( const requestId = generateRequestId() const ip = getClientIp(request) - const ipRateLimit = await rateLimiter.checkRateLimitDirect( - `file-sso:ip:${ip}`, - SSO_IP_RATE_LIMIT - ) - if (!ipRateLimit.allowed) { - logger.warn(`[${requestId}] SSO eligibility rate limit exceeded from ${ip}`) - const response = NextResponse.json( - { error: 'Too many requests. Please try again later.' }, - { status: 429 } - ) - response.headers.set( - 'Retry-After', - String(Math.ceil((ipRateLimit.retryAfterMs ?? SSO_IP_RATE_LIMIT.refillIntervalMs) / 1000)) + if (ip) { + const ipRateLimit = await rateLimiter.checkRateLimitDirect( + `file-sso:ip:${ip}`, + SSO_IP_RATE_LIMIT, + { failClosed: true } ) - return response + if (!ipRateLimit.allowed) { + logger.warn(`[${requestId}] SSO eligibility rate limit exceeded from ${ip}`) + return rateLimited(ipRateLimit.retryAfterMs, SSO_IP_RATE_LIMIT.refillIntervalMs) + } } const parsed = await parseRequest(publicFileSSOContract, request, context) @@ -64,6 +74,18 @@ export const POST = withRouteHandler( return NextResponse.json({ error: 'This file is not configured for SSO' }, { status: 400 }) } + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( + `file-sso:resource:${resolved.share.id}`, + SSO_RESOURCE_RATE_LIMIT, + { failClosed: true } + ) + if (!resourceRateLimit.allowed) { + logger.warn(`[${requestId}] SSO eligibility resource rate limit exceeded`, { + shareId: resolved.share.id, + }) + return rateLimited(resourceRateLimit.retryAfterMs, SSO_RESOURCE_RATE_LIMIT.refillIntervalMs) + } + const allowedEmails = Array.isArray(resolved.share.allowedEmails) ? (resolved.share.allowedEmails as string[]) : [] diff --git a/apps/sim/app/api/help/integration-request/route.ts b/apps/sim/app/api/help/integration-request/route.ts index 6a8faf682b6..19084dc1e16 100644 --- a/apps/sim/app/api/help/integration-request/route.ts +++ b/apps/sim/app/api/help/integration-request/route.ts @@ -26,8 +26,14 @@ export const POST = withRouteHandler(async (req: NextRequest) => { try { const ip = getClientIp(req) + if (!ip) { + logger.warn(`[${requestId}] Unable to resolve client IP for public rate limit`) + return NextResponse.json( + { error: 'Too many requests. Please try again later.' }, + { status: 429 } + ) + } const storageKey = `public:integration-request:${ip}` - const { allowed, remaining, resetAt } = await rateLimiter.checkRateLimitDirect( storageKey, PUBLIC_ENDPOINT_RATE_LIMIT diff --git a/apps/sim/app/api/v2/files/[fileId]/share/route.test.ts b/apps/sim/app/api/v2/files/[fileId]/share/route.test.ts index 7d33f8c91f9..4a45583c1ae 100644 --- a/apps/sim/app/api/v2/files/[fileId]/share/route.test.ts +++ b/apps/sim/app/api/v2/files/[fileId]/share/route.test.ts @@ -210,7 +210,7 @@ describe('PATCH /api/v2/files/[fileId]/share', () => { workspaceId: WORKSPACE_ID, isActive: true, authType: 'password', - password: 'hunter2hunter2', + password: 'hunter2hunter2!', }) expect(response.status).toBe(200) @@ -222,7 +222,7 @@ describe('PATCH /api/v2/files/[fileId]/share', () => { assertedWorkspaceId: WORKSPACE_ID, isActive: true, authType: 'password', - password: 'hunter2hunter2', + password: 'hunter2hunter2!', allowedEmails: undefined, }, request: expect.anything(), diff --git a/apps/sim/lib/analytics/profound.ts b/apps/sim/lib/analytics/profound.ts index ff8c568e14d..80b39ba3526 100644 --- a/apps/sim/lib/analytics/profound.ts +++ b/apps/sim/lib/analytics/profound.ts @@ -102,10 +102,7 @@ export function sendToProfound(request: Request, statusCode: number): void { host: getBaseDomain(), path: url.pathname, status_code: statusCode, - ip: (() => { - const resolved = getClientIp(request) - return resolved === 'unknown' ? '0.0.0.0' : resolved - })(), + ip: getClientIp(request) ?? '0.0.0.0', user_agent: request.headers.get('user-agent') || '', ...(Object.keys(queryParams).length > 0 && { query_params: queryParams }), ...(request.headers.get('referer') && { referer: request.headers.get('referer')! }), diff --git a/apps/sim/lib/api/contracts/chats.password.test.ts b/apps/sim/lib/api/contracts/chats.password.test.ts index 3aad3347859..b594a4d1a03 100644 --- a/apps/sim/lib/api/contracts/chats.password.test.ts +++ b/apps/sim/lib/api/contracts/chats.password.test.ts @@ -28,8 +28,18 @@ describe('chat deployment password contract', () => { expect(result.error?.issues[0].message).toBe('Password cannot contain only whitespace') }) + it('requires at least 15 characters for a new password', () => { + const result = chatDeploymentPasswordSchema.safeParse('short-password') + expect(result.success).toBe(false) + expect(result.error?.issues[0].message).toBe('Password must be at least 15 characters') + }) + + it('continues accepting legacy short passwords at the deployed login gate', () => { + expect(deployedChatAuthBodySchema.safeParse({ password: 'legacy' }).success).toBe(true) + }) + it('preserves surrounding whitespace, which login compares byte-exact', () => { - expect(chatDeploymentPasswordSchema.parse(' hunter2 ')).toBe(' hunter2 ') + expect(chatDeploymentPasswordSchema.parse(' hunter2hunter2 ')).toBe(' hunter2hunter2 ') }) /** @@ -54,7 +64,9 @@ describe('chat deployment password contract', () => { expect(createChatBodySchema.safeParse({ ...createBody, password: ' ' }).success).toBe(false) expect(createChatBodySchema.safeParse({ ...createBody, password: tooLong }).success).toBe(false) - expect(createChatBodySchema.safeParse({ ...createBody, password: 'ok' }).success).toBe(true) + expect( + createChatBodySchema.safeParse({ ...createBody, password: 'correct-password' }).success + ).toBe(true) expect(updateChatBodySchema.safeParse({ password: ' ' }).success).toBe(false) expect(updateChatBodySchema.safeParse({ password: tooLong }).success).toBe(false) diff --git a/apps/sim/lib/api/contracts/chats.ts b/apps/sim/lib/api/contracts/chats.ts index 2755536a5a2..082d3f434b5 100644 --- a/apps/sim/lib/api/contracts/chats.ts +++ b/apps/sim/lib/api/contracts/chats.ts @@ -10,6 +10,7 @@ export type ChatAuthType = z.output * would lock every visitor out of the deployment permanently. */ const MAX_CHAT_PASSWORD_CHARS = 1024 +const MIN_CHAT_PASSWORD_CHARS = 15 /** * Password accepted when setting or changing a chat deployment's password. The @@ -24,6 +25,10 @@ export const chatDeploymentPasswordSchema = z (password) => password.length === 0 || password.trim().length > 0, 'Password cannot contain only whitespace' ) + .refine( + (password) => password.length === 0 || password.length >= MIN_CHAT_PASSWORD_CHARS, + `Password must be at least ${MIN_CHAT_PASSWORD_CHARS} characters` + ) export const chatIdParamsSchema = z.object({ id: z.string().min(1), diff --git a/apps/sim/lib/api/contracts/public-shares.password.test.ts b/apps/sim/lib/api/contracts/public-shares.password.test.ts new file mode 100644 index 00000000000..7cf090c35ca --- /dev/null +++ b/apps/sim/lib/api/contracts/public-shares.password.test.ts @@ -0,0 +1,36 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { + authenticatePublicFileBodySchema, + sharePasswordSchema, + upsertFileShareBodySchema, +} from '@/lib/api/contracts/public-shares' +import { v2UpsertFileShareBodySchema } from '@/lib/api/contracts/v2/files' + +describe('public file share password contracts', () => { + it('requires 15 characters when a password is created or changed', () => { + expect(sharePasswordSchema.safeParse('short-password').success).toBe(false) + expect(sharePasswordSchema.safeParse('correct-password').success).toBe(true) + expect( + upsertFileShareBodySchema.safeParse({ + isActive: true, + authType: 'password', + password: 'short-password', + }).success + ).toBe(false) + expect( + v2UpsertFileShareBodySchema.safeParse({ + workspaceId: 'workspace-1', + isActive: true, + authType: 'password', + password: 'short-password', + }).success + ).toBe(false) + }) + + it('continues accepting legacy short passwords at the public login gate', () => { + expect(authenticatePublicFileBodySchema.safeParse({ password: 'legacy' }).success).toBe(true) + }) +}) diff --git a/apps/sim/lib/api/contracts/public-shares.ts b/apps/sim/lib/api/contracts/public-shares.ts index 41519234256..84a6362fc69 100644 --- a/apps/sim/lib/api/contracts/public-shares.ts +++ b/apps/sim/lib/api/contracts/public-shares.ts @@ -12,6 +12,12 @@ export type ShareAuthType = z.output /** An allowed email address or `@domain` pattern for email/SSO shares. */ const allowedEmailSchema = z.string().min(1).max(320) +/** Password accepted when a file share is created or its password is changed. */ +export const sharePasswordSchema = z + .string() + .min(15, 'Password must be at least 15 characters') + .max(1024, 'Password is too long') + /** * Public-safe representation of a `public_share` row. Never carries the * underlying storage key or the (encrypted) password — `hasPassword` is the @@ -42,11 +48,7 @@ const fileShareParamsSchema = z.object({ export const upsertFileShareBodySchema = z.object({ isActive: z.boolean(), authType: shareAuthTypeSchema.optional(), - password: z - .string() - .min(1, 'Password cannot be empty') - .max(1024, 'Password is too long') - .optional(), + password: sharePasswordSchema.optional(), allowedEmails: z.array(allowedEmailSchema).max(200, 'Too many allowed emails').optional(), /** Client-reserved token persisted on first share. Ignored once the share row exists. */ token: z @@ -149,7 +151,7 @@ export const getPublicInlineFileContract = defineRouteContract({ }, }) -const authenticatePublicFileBodySchema = z.object({ +export const authenticatePublicFileBodySchema = z.object({ password: z.string().min(1, 'Password is required').max(1024, 'Password is too long'), }) @@ -162,8 +164,9 @@ const authenticatePublicFileResponseSchema = z.object({ export type AuthenticatePublicFileResponse = z.output /** - * Exchanges a share password for a `file_auth_{shareId}` cookie. IP rate-limited; - * returns 401 (`Invalid password`) on mismatch and 429 when throttled. + * Exchanges a share password for a `file_auth_{shareId}` cookie. Client-IP and + * share-resource rate-limited; returns 401 (`Invalid password`) on mismatch and + * 429 when throttled. */ export const authenticatePublicFileContract = defineRouteContract({ method: 'POST', diff --git a/apps/sim/lib/api/contracts/v2/files.ts b/apps/sim/lib/api/contracts/v2/files.ts index 8b0ba62b39b..254fb6abe1b 100644 --- a/apps/sim/lib/api/contracts/v2/files.ts +++ b/apps/sim/lib/api/contracts/v2/files.ts @@ -6,7 +6,11 @@ import { workspaceFileNameSchema, workspaceIdSchema, } from '@/lib/api/contracts/primitives' -import { shareAuthTypeSchema, shareRecordSchema } from '@/lib/api/contracts/public-shares' +import { + shareAuthTypeSchema, + sharePasswordSchema, + shareRecordSchema, +} from '@/lib/api/contracts/public-shares' import { defineRouteContract } from '@/lib/api/contracts/types' import { V2_FALSE_VALUES, @@ -547,10 +551,7 @@ export const v2UpsertFileShareBodySchema = z .describe( 'How access to the share is gated. The stored mode is kept when omitted. Enabling `public` clears the stored password and empties `allowedEmails`; `password` empties `allowedEmails`; `email` and `sso` clear the stored password.' ), - password: z - .string() - .min(1, 'password cannot be empty') - .max(1024, 'password is too long') + password: sharePasswordSchema .optional() .describe( 'Password for a password-gated share. Kept when omitted; enabling `password` with neither a supplied nor a stored password is a 400.' diff --git a/apps/sim/lib/api/server/routes/v2-json-route.ts b/apps/sim/lib/api/server/routes/v2-json-route.ts index f13eb0416b5..691d4c8134f 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.ts @@ -277,6 +277,16 @@ export const v2OrchestrationErrorPolicy = { async function enforceV2PreAuthIpLimit(request: NextRequest): Promise { const ip = getClientIp(request) + if (!ip) { + const resetAt = new Date(Date.now() + V2_PREAUTH_IP_LIMIT.refillIntervalMs) + return v2RateLimitError({ + allowed: false, + limit: V2_PREAUTH_IP_LIMIT.maxTokens, + remaining: 0, + resetAt, + retryAfterMs: V2_PREAUTH_IP_LIMIT.refillIntervalMs, + }) + } const abuseLimit = await rateLimiter.checkRateLimitDirect( `v2:preauth:ip:${ip}`, V2_PREAUTH_IP_LIMIT, diff --git a/apps/sim/lib/auth/auth.ts b/apps/sim/lib/auth/auth.ts index 8f7cb189175..2f65fc1bbf5 100644 --- a/apps/sim/lib/auth/auth.ts +++ b/apps/sim/lib/auth/auth.ts @@ -95,6 +95,7 @@ import { isSsoEnabled, } from '@/lib/core/config/env-flags' import { PlatformEvents } from '@/lib/core/telemetry' +import { trustedProxies } from '@/lib/core/utils/request' import { getBaseUrl, isLocalhostUrl, parseOriginList } from '@/lib/core/utils/urls' import { captureOAuthCredentialDraftBinding, @@ -165,17 +166,6 @@ if (validStripeKey) { }) } -/** - * Reverse-proxy hops trusted for forwarded-IP resolution. When configured, - * Better Auth walks the x-forwarded-for chain right to left, skips these - * hops, and records the first untrusted address as the session client IP — - * preventing header spoofing behind multi-hop proxies. - */ -const trustedProxies = (env.AUTH_TRUSTED_PROXIES ?? '') - .split(',') - .map((entry) => entry.trim()) - .filter(Boolean) - /** * Resolves the org's API instance URL for a freshly linked Salesforce account. * diff --git a/apps/sim/lib/core/config/env.ts b/apps/sim/lib/core/config/env.ts index d41d35f4a72..cf5fa33c642 100644 --- a/apps/sim/lib/core/config/env.ts +++ b/apps/sim/lib/core/config/env.ts @@ -612,7 +612,8 @@ export const env = createEnv({ REACT_SCAN_ENABLED: z.boolean().optional(), // Enable React Scan for performance debugging (dev only) // Network / proxy trust - AUTH_TRUSTED_PROXIES: z.string().optional(), // Comma-separated reverse-proxy IPs or CIDR ranges. When set, Better Auth walks the forwarded-IP chain right to left, skips these trusted hops, and uses the first untrusted address as the client IP. Leave unset to trust only single-value IP headers. + /** Comma-separated proxy IPs/CIDRs skipped while resolving the forwarded client chain. */ + AUTH_TRUSTED_PROXIES: z.string().optional(), // SSO Configuration (for script-based registration) SSO_ENABLED: z.boolean().optional(), // Enable SSO functionality diff --git a/apps/sim/lib/core/rate-limiter/index.ts b/apps/sim/lib/core/rate-limiter/index.ts index 3c8a19d2e78..16761324068 100644 --- a/apps/sim/lib/core/rate-limiter/index.ts +++ b/apps/sim/lib/core/rate-limiter/index.ts @@ -12,6 +12,7 @@ export { DEFAULT_PUBLIC_IP_ROUTE_LIMIT, DEFAULT_USER_ROUTE_LIMIT, enforceIpRateLimit, + enforceIpRateLimitWithIndependentBackstop, enforceRecipientRateLimit, enforceUserOrIpRateLimit, enforceUserRateLimit, diff --git a/apps/sim/lib/core/rate-limiter/route-helpers.test.ts b/apps/sim/lib/core/rate-limiter/route-helpers.test.ts index 0f895e81e1a..42786dc3210 100644 --- a/apps/sim/lib/core/rate-limiter/route-helpers.test.ts +++ b/apps/sim/lib/core/rate-limiter/route-helpers.test.ts @@ -22,16 +22,12 @@ vi.mock('@/lib/core/rate-limiter/storage', async () => { } }) -function passThroughClientIp() { - requestUtilsMockFns.mockGetClientIp.mockImplementation( - (req: { headers: { get(name: string): string | null } }) => - req.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || - req.headers.get('x-real-ip')?.trim() || - 'unknown' - ) -} - -import { enforceIpRateLimit, enforceUserOrIpRateLimit, enforceUserRateLimit } from './route-helpers' +import { + enforceIpRateLimit, + enforceIpRateLimitWithIndependentBackstop, + enforceUserOrIpRateLimit, + enforceUserRateLimit, +} from './route-helpers' const consume = mockAdapter.consumeTokens as Mock @@ -103,7 +99,7 @@ describe('route-helpers rate limiting', () => { describe('enforceIpRateLimit', () => { beforeEach(() => { - passThroughClientIp() + requestUtilsMockFns.mockGetClientIp.mockReturnValue('203.0.113.7') }) it('uses the X-Forwarded-For client IP in the bucket key', async () => { @@ -125,20 +121,25 @@ describe('route-helpers rate limiting', () => { ) }) - it('folds spoofed `X-Forwarded-For: unknown` into a single shared bucket', async () => { - consume.mockResolvedValue({ - allowed: true, - tokensRemaining: 9, - resetAt: new Date(), - }) + it('fails closed without creating a shared bucket when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + const request = createMockRequest('POST') - const reqA = createMockRequest('POST', undefined, { 'x-forwarded-for': 'unknown' }) - const reqB = createMockRequest('POST', undefined, { 'x-forwarded-for': 'unknown' }) - await enforceIpRateLimit('otp', reqA) - await enforceIpRateLimit('otp', reqB) + const result = await enforceIpRateLimit('otp', request) - const keys = consume.mock.calls.map((call) => call[0]) - expect(keys).toEqual(['route:otp:ip:unknown', 'route:otp:ip:unknown']) + expect(result?.status).toBe(429) + expect(result?.headers.get('Retry-After')).toBe('60') + expect(consume).not.toHaveBeenCalled() + }) + + it('defers an unresolved client only when the caller declares an independent backstop', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + const request = createMockRequest('POST') + + const result = await enforceIpRateLimitWithIndependentBackstop('password-reset', request) + + expect(result).toBeNull() + expect(consume).not.toHaveBeenCalled() }) it('returns a 429 with Retry-After on rate limit', async () => { @@ -160,7 +161,7 @@ describe('route-helpers rate limiting', () => { describe('enforceUserOrIpRateLimit', () => { beforeEach(() => { - passThroughClientIp() + requestUtilsMockFns.mockGetClientIp.mockReturnValue('203.0.113.7') }) it('keys per-user when userId is present', async () => { diff --git a/apps/sim/lib/core/rate-limiter/route-helpers.ts b/apps/sim/lib/core/rate-limiter/route-helpers.ts index 26e267c8748..c826f18853e 100644 --- a/apps/sim/lib/core/rate-limiter/route-helpers.ts +++ b/apps/sim/lib/core/rate-limiter/route-helpers.ts @@ -56,17 +56,22 @@ export async function enforceUserRateLimit( return buildRateLimitResponse(resetAt) } -/** - * Apply a per-IP token bucket to an unauthenticated route. The `unknown` IP - * fallback shares one global bucket per route so it cannot be amplified by - * `X-Forwarded-For: unknown` spoofing. - */ -export async function enforceIpRateLimit( +async function enforceIpRateLimitWithPolicy( bucketName: string, request: NextRequest, - config: TokenBucketConfig = DEFAULT_PUBLIC_IP_ROUTE_LIMIT + config: TokenBucketConfig, + unresolvedClientPolicy: 'deny' | 'defer' ): Promise { const ip = getClientIp(request) + if (!ip) { + logger.warn('Unable to resolve client IP for public rate limit', { + bucket: bucketName, + unresolvedClientPolicy, + }) + return unresolvedClientPolicy === 'deny' + ? buildRateLimitResponse(new Date(Date.now() + config.refillIntervalMs)) + : null + } const key = `route:${bucketName}:ip:${ip}` const { allowed, resetAt } = await rateLimiter.checkRateLimitDirect(key, config) if (allowed) return null @@ -74,6 +79,27 @@ export async function enforceIpRateLimit( return buildRateLimitResponse(resetAt) } +/** Apply a per-IP token bucket and fail closed when the client cannot be resolved safely. */ +export async function enforceIpRateLimit( + bucketName: string, + request: NextRequest, + config: TokenBucketConfig = DEFAULT_PUBLIC_IP_ROUTE_LIMIT +): Promise { + return enforceIpRateLimitWithPolicy(bucketName, request, config, 'deny') +} + +/** + * Apply a per-IP bucket when resolvable, deferring unresolved clients to an + * independent non-IP limit that the caller must enforce before any side effect. + */ +export async function enforceIpRateLimitWithIndependentBackstop( + bucketName: string, + request: NextRequest, + config: TokenBucketConfig = DEFAULT_PUBLIC_IP_ROUTE_LIMIT +): Promise { + return enforceIpRateLimitWithPolicy(bucketName, request, config, 'defer') +} + /** * Apply a per-recipient token bucket to a route that mails an address the * caller chooses. A per-IP bucket cannot stop a distributed attempt to bomb one diff --git a/apps/sim/lib/core/security/deployment-auth.ts b/apps/sim/lib/core/security/deployment-auth.ts index 69c842def61..69290f04f15 100644 --- a/apps/sim/lib/core/security/deployment-auth.ts +++ b/apps/sim/lib/core/security/deployment-auth.ts @@ -26,6 +26,29 @@ const PASSWORD_IP_RATE_LIMIT: TokenBucketConfig = { refillIntervalMs: 15 * 60_000, } +/** + * Caps guesses against one resource independently of client identity. This is + * the backstop for distributed attempts and for requests whose proxy chain + * cannot be resolved safely. + */ +const PASSWORD_RESOURCE_RATE_LIMIT: TokenBucketConfig = { + maxTokens: 100, + refillRate: 100, + refillIntervalMs: 15 * 60_000, +} + +function passwordRateLimitResult( + retryAfterMs: number | undefined, + fallbackMs: number +): DeploymentAuthResult { + return { + authorized: false, + error: 'Too many attempts. Please try again later.', + status: 429, + retryAfterMs: retryAfterMs ?? fallbackMs, + } +} + /** * A password/email-gated resource (a deployed chat or a public file share). Only * the fields the auth check needs — the `password` is the encrypted secret. @@ -106,22 +129,41 @@ export async function validateDeploymentAuth( } const ip = getClientIp(request) - const ipRateLimit = await rateLimiter.checkRateLimitDirect( - `${cookiePrefix}-password:ip:${resource.id}:${ip}`, - PASSWORD_IP_RATE_LIMIT - ) - if (!ipRateLimit.allowed) { - logger.warn( - `[${requestId}] Password attempt IP rate limit exceeded for ${resource.id} from ${ip}` + if (ip) { + const ipRateLimit = await rateLimiter.checkRateLimitDirect( + `${cookiePrefix}-password:ip:${resource.id}:${ip}`, + PASSWORD_IP_RATE_LIMIT, + { failClosed: true } ) - return { - authorized: false, - error: 'Too many attempts. Please try again later.', - status: 429, - retryAfterMs: ipRateLimit.retryAfterMs ?? PASSWORD_IP_RATE_LIMIT.refillIntervalMs, + if (!ipRateLimit.allowed) { + logger.warn(`[${requestId}] Password attempt IP rate limit exceeded`, { + resourceId: resource.id, + cookiePrefix, + ip, + }) + return passwordRateLimitResult( + ipRateLimit.retryAfterMs, + PASSWORD_IP_RATE_LIMIT.refillIntervalMs + ) } } + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( + `${cookiePrefix}-password:resource:${resource.id}`, + PASSWORD_RESOURCE_RATE_LIMIT, + { failClosed: true } + ) + if (!resourceRateLimit.allowed) { + logger.warn(`[${requestId}] Password attempt resource rate limit exceeded`, { + resourceId: resource.id, + cookiePrefix, + }) + return passwordRateLimitResult( + resourceRateLimit.retryAfterMs, + PASSWORD_RESOURCE_RATE_LIMIT.refillIntervalMs + ) + } + const { decrypted } = await decryptSecret(resource.password) if (!safeCompare(password, decrypted)) { return { authorized: false, error: 'Invalid password' } diff --git a/apps/sim/lib/core/security/otp.ts b/apps/sim/lib/core/security/otp.ts index 326a48cec8a..e8ac4636c9f 100644 --- a/apps/sim/lib/core/security/otp.ts +++ b/apps/sim/lib/core/security/otp.ts @@ -28,6 +28,13 @@ export const OTP_EMAIL_RATE_LIMIT: TokenBucketConfig = { refillIntervalMs: 15 * 60_000, } +/** Caps OTP requests against one deployment independently of client identity. */ +export const OTP_RESOURCE_RATE_LIMIT: TokenBucketConfig = { + maxTokens: 100, + refillRate: 100, + refillIntervalMs: 15 * 60_000, +} + /** * Key formats are kept per-kind to preserve any in-flight OTPs already issued * against existing chat deployments. The chat Redis key uses the legacy `otp:` diff --git a/apps/sim/lib/core/security/turnstile.ts b/apps/sim/lib/core/security/turnstile.ts index 4bef713ae1a..df8f4b27f8c 100644 --- a/apps/sim/lib/core/security/turnstile.ts +++ b/apps/sim/lib/core/security/turnstile.ts @@ -55,7 +55,7 @@ export async function verifyTurnstileToken({ const body = new URLSearchParams() body.set('secret', secret) body.set('response', token) - if (remoteIp && remoteIp !== 'unknown') body.set('remoteip', remoteIp) + if (remoteIp) body.set('remoteip', remoteIp) if (idempotencyKey) body.set('idempotency_key', idempotencyKey) const controller = new AbortController() diff --git a/apps/sim/lib/core/utils/request.ts b/apps/sim/lib/core/utils/request.ts index 84150f4a38c..a32a7b84e68 100644 --- a/apps/sim/lib/core/utils/request.ts +++ b/apps/sim/lib/core/utils/request.ts @@ -1,5 +1,12 @@ import { getRequestContext } from '@sim/logger' +import { createClientIpResolver } from '@sim/security/ip' import { generateId } from '@sim/utils/id' +import { env } from '@/lib/core/config/env' + +const clientIpResolver = createClientIpResolver(env.AUTH_TRUSTED_PROXIES) + +export const trustedProxies = clientIpResolver.trustedProxies + /** * Generate a short request ID for correlation. If called inside a request * context (see `withRouteHandler` and `runWithRequestContext`), returns the @@ -11,12 +18,13 @@ export function generateRequestId(): string { } /** - * Extract the client IP from a request, checking `x-forwarded-for` then `x-real-ip`. + * Resolves the first untrusted client address from a proxy-appended forwarded + * chain. Returns `null` when the chain is missing, malformed, or contains only + * trusted proxies so callers can apply an explicit policy without sharing one + * synthetic rate-limit identity. */ -export function getClientIp(request: { headers: { get(name: string): string | null } }): string { - return ( - request.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || - request.headers.get('x-real-ip')?.trim() || - 'unknown' - ) +export function getClientIp(request: { + headers: { get(name: string): string | null } +}): string | null { + return clientIpResolver.resolve(request.headers) } diff --git a/apps/sim/lib/credential-groups/rate-limit.test.ts b/apps/sim/lib/credential-groups/rate-limit.test.ts new file mode 100644 index 00000000000..aeacf42a8e0 --- /dev/null +++ b/apps/sim/lib/credential-groups/rate-limit.test.ts @@ -0,0 +1,50 @@ +/** + * @vitest-environment node + */ +import { createMockRequest, requestUtilsMockFns } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockCheckRateLimitDirect } = vi.hoisted(() => ({ + mockCheckRateLimitDirect: vi.fn(), +})) + +vi.mock('@/lib/core/rate-limiter', () => ({ + RateLimitError: class extends Error {}, + RateLimiter: class { + checkRateLimitDirect = mockCheckRateLimitDirect + }, +})) + +import { + enforcePublicCredentialGroupIpRateLimit, + enforcePublicCredentialGroupOAuthStartIpRateLimit, +} from '@/lib/credential-groups/rate-limit' + +describe('public credential group rate limits', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('fails closed without a client IP when no independent backstop is declared', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + + const response = await enforcePublicCredentialGroupIpRateLimit( + createMockRequest('GET'), + 'metadata' + ) + + expect(response?.status).toBe(429) + expect(mockCheckRateLimitDirect).not.toHaveBeenCalled() + }) + + it('defers unresolved OAuth clients to the per-enrollment backstop', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + + const response = await enforcePublicCredentialGroupOAuthStartIpRateLimit( + createMockRequest('GET') + ) + + expect(response).toBeNull() + expect(mockCheckRateLimitDirect).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/credential-groups/rate-limit.ts b/apps/sim/lib/credential-groups/rate-limit.ts index a16f46304c7..373531c0ad5 100644 --- a/apps/sim/lib/credential-groups/rate-limit.ts +++ b/apps/sim/lib/credential-groups/rate-limit.ts @@ -58,13 +58,18 @@ function configForPublicScope(scope: PublicCredentialGroupRateLimitScope): Token return PUBLIC_OAUTH_CALLBACK_RATE_LIMIT } -/** Per-IP guard for unauthenticated enrollment reads and OAuth endpoints. */ -export async function enforcePublicCredentialGroupIpRateLimit( +async function enforcePublicCredentialGroupIpRateLimitWithPolicy( request: { headers: { get(name: string): string | null } }, - scope: PublicCredentialGroupRateLimitScope + scope: PublicCredentialGroupRateLimitScope, + unresolvedClientPolicy: 'deny' | 'defer' ): Promise { const config = configForPublicScope(scope) const ip = getClientIp(request) + if (!ip) { + return unresolvedClientPolicy === 'deny' + ? rateLimitResponse(undefined, config.refillIntervalMs) + : null + } const result = await rateLimiter.checkRateLimitDirect( `public-credential-group:${scope}:ip:${ip}`, config, @@ -73,6 +78,24 @@ export async function enforcePublicCredentialGroupIpRateLimit( return result.allowed ? null : rateLimitResponse(result.retryAfterMs, config.refillIntervalMs) } +/** Per-IP guard for unauthenticated enrollment reads and OAuth endpoints. */ +export async function enforcePublicCredentialGroupIpRateLimit( + request: { headers: { get(name: string): string | null } }, + scope: PublicCredentialGroupRateLimitScope +): Promise { + return enforcePublicCredentialGroupIpRateLimitWithPolicy(request, scope, 'deny') +} + +/** + * Applies the OAuth-start IP bucket when resolvable and otherwise defers to the + * mandatory per-enrollment OAuth budget applied after invitation authentication. + */ +export async function enforcePublicCredentialGroupOAuthStartIpRateLimit(request: { + headers: { get(name: string): string | null } +}): Promise { + return enforcePublicCredentialGroupIpRateLimitWithPolicy(request, 'oauth-start', 'defer') +} + /** Prevents one leaked invitation from starting unbounded provider consent flows. */ export async function enforceCredentialGroupEnrollmentOAuthRateLimit( enrollmentId: string diff --git a/apps/sim/lib/public-shares/rate-limit.test.ts b/apps/sim/lib/public-shares/rate-limit.test.ts new file mode 100644 index 00000000000..26c500c2db7 --- /dev/null +++ b/apps/sim/lib/public-shares/rate-limit.test.ts @@ -0,0 +1,33 @@ +/** + * @vitest-environment node + */ +import { requestUtilsMockFns } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockCheckRateLimitDirect } = vi.hoisted(() => ({ + mockCheckRateLimitDirect: vi.fn(), +})) + +vi.mock('@/lib/core/rate-limiter', () => ({ + RateLimiter: class { + checkRateLimitDirect = mockCheckRateLimitDirect + }, +})) + +import { enforcePublicFileRateLimit } from '@/lib/public-shares/rate-limit' + +describe('enforcePublicFileRateLimit', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('fails closed without creating a shared bucket when the client IP cannot be resolved', async () => { + requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce(null) + + const response = await enforcePublicFileRateLimit(new Request('http://localhost'), 'content') + + expect(response?.status).toBe(429) + expect(response?.headers.get('Retry-After')).toBe('60') + expect(mockCheckRateLimitDirect).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/public-shares/rate-limit.ts b/apps/sim/lib/public-shares/rate-limit.ts index 60f7223a60d..5602104bd5d 100644 --- a/apps/sim/lib/public-shares/rate-limit.ts +++ b/apps/sim/lib/public-shares/rate-limit.ts @@ -22,15 +22,21 @@ const CONTENT_RATE_LIMIT: TokenBucketConfig = { * Per-IP rate limit for the unauthenticated public share endpoints, returning a * `429` response when exceeded (or `null` to proceed). The token is unguessable, * so this defends a *known* link against hammering (DoS / S3 egress) rather than - * enumeration. Fails open on storage errors (availability over strictness), - * matching the chat public route. + * enumeration. Fails open on storage errors (availability over strictness), but + * fails closed when the forwarded chain cannot identify a safe client key. */ export async function enforcePublicFileRateLimit( request: { headers: { get(name: string): string | null } }, scope: 'metadata' | 'content' ): Promise { - const ip = getClientIp(request) const config = scope === 'content' ? CONTENT_RATE_LIMIT : METADATA_RATE_LIMIT + const ip = getClientIp(request) + if (!ip) { + return NextResponse.json( + { error: 'Too many requests. Please try again later.' }, + { status: 429, headers: { 'Retry-After': String(config.refillIntervalMs / 1000) } } + ) + } const result = await rateLimiter.checkRateLimitDirect(`public-file:${scope}:${ip}`, config) if (result.allowed) return null diff --git a/apps/sim/lib/webhooks/providers/generic.ts b/apps/sim/lib/webhooks/providers/generic.ts index a97317d6fbe..a0f97af1dba 100644 --- a/apps/sim/lib/webhooks/providers/generic.ts +++ b/apps/sim/lib/webhooks/providers/generic.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { normalizeIpAddress } from '@sim/security/ip' import { isRecordLike } from '@sim/utils/object' import { NextResponse } from 'next/server' import { getClientIp } from '@/lib/core/utils/request' @@ -161,8 +162,11 @@ export const genericHandler: WebhookProviderHandler = { const allowedIps = providerConfig.allowedIps if (allowedIps && Array.isArray(allowedIps) && allowedIps.length > 0) { const clientIp = getClientIp(request) + const clientIpAllowed = allowedIps.some( + (allowedIp) => typeof allowedIp === 'string' && normalizeIpAddress(allowedIp) === clientIp + ) - if (clientIp === 'unknown' || !allowedIps.includes(clientIp)) { + if (!clientIp || !clientIpAllowed) { logger.warn(`[${requestId}] Forbidden webhook access attempt - IP not allowed: ${clientIp}`) return new NextResponse('Forbidden - IP not allowed', { status: 403, diff --git a/bun.lock b/bun.lock index d5b11db7e06..17126f64f7b 100644 --- a/bun.lock +++ b/bun.lock @@ -389,6 +389,7 @@ "dependencies": { "@sim/db": "workspace:*", "@sim/logger": "workspace:*", + "@sim/security": "workspace:*", "@sim/utils": "workspace:*", "drizzle-orm": "^0.45.2", }, diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index c2c5c920377..17a32481749 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -23,10 +23,10 @@ services: # (apex + www, alias hostnames, reverse-proxy IPs). Empty by default. - TRUSTED_ORIGINS=${TRUSTED_ORIGINS:-} # AUTH_TRUSTED_PROXIES: comma-separated reverse-proxy IPs or CIDR ranges in - # front of the app (ingress, load balancer). Better Auth walks - # x-forwarded-for right to left, skips these hops, and uses the first - # untrusted address as the client IP. Required for correct session IPs and - # rate-limit keying behind a multi-hop proxy chain. Empty by default. + # front of the app (ingress, load balancer). Sim and Better Auth walk + # x-forwarded-for right to left, skip these hops, and use the first + # untrusted address as the client IP. Required to look through known + # intermediate proxies in a multi-hop chain. Empty by default. - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-} # Required. Compose aborts with this message rather than starting the app # with an empty secret, which would silently corrupt stored credentials. diff --git a/packages/audit/package.json b/packages/audit/package.json index 46b357f5db3..160e360bd42 100644 --- a/packages/audit/package.json +++ b/packages/audit/package.json @@ -27,6 +27,7 @@ "dependencies": { "@sim/db": "workspace:*", "@sim/logger": "workspace:*", + "@sim/security": "workspace:*", "@sim/utils": "workspace:*", "drizzle-orm": "^0.45.2" }, diff --git a/packages/audit/src/log.test.ts b/packages/audit/src/log.test.ts index ad4a52517f4..2b7ace185f9 100644 --- a/packages/audit/src/log.test.ts +++ b/packages/audit/src/log.test.ts @@ -1,13 +1,7 @@ /** * @vitest-environment node */ -import { - auditMock, - dbChainMock, - dbChainMockFns, - requestUtilsMockFns, - resetDbChainMock, -} from '@sim/testing' +import { auditMock, dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('@sim/db', () => ({ @@ -81,12 +75,6 @@ describe('recordAudit', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() - requestUtilsMockFns.mockGetClientIp.mockImplementation( - (request: { headers: { get(name: string): string | null } }) => - request.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || - request.headers.get('x-real-ip')?.trim() || - 'unknown' - ) }) afterEach(() => { @@ -187,13 +175,13 @@ describe('recordAudit', () => { expect(dbChainMockFns.values).toHaveBeenCalledWith( expect.objectContaining({ - ipAddress: '1.2.3.4', + ipAddress: '5.6.7.8', userAgent: 'TestAgent/1.0', }) ) }) - it('falls back to x-real-ip when x-forwarded-for is absent', async () => { + it('records null when x-forwarded-for is absent', async () => { const request = new Request('https://example.com', { headers: { 'x-real-ip': '10.0.0.1' }, }) @@ -212,7 +200,7 @@ describe('recordAudit', () => { expect(dbChainMockFns.values).toHaveBeenCalledWith( expect.objectContaining({ - ipAddress: '10.0.0.1', + ipAddress: null, userAgent: undefined, }) ) diff --git a/packages/audit/src/log.ts b/packages/audit/src/log.ts index 29082cdd543..69debb5a64b 100644 --- a/packages/audit/src/log.ts +++ b/packages/audit/src/log.ts @@ -1,10 +1,12 @@ import { auditLog, db, user } from '@sim/db' import { createLogger } from '@sim/logger' +import { createClientIpResolver } from '@sim/security/ip' import { generateShortId } from '@sim/utils/id' import { eq } from 'drizzle-orm' import type { AuditActionType, AuditResourceTypeValue } from './types' const logger = createLogger('AuditLog') +const clientIpResolver = createClientIpResolver(process.env.AUTH_TRUSTED_PROXIES) export interface AuditLogParams { workspaceId?: string | null @@ -26,14 +28,6 @@ export interface AuditLogParams { request?: { headers: { get(name: string): string | null } } } -function getClientIp(request: { headers: { get(name: string): string | null } }): string { - return ( - request.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || - request.headers.get('x-real-ip')?.trim() || - 'unknown' - ) -} - /** * Fire-and-forget audit log write. Never throws; failures are logged. * Resolves actorName/actorEmail from the user table when both are omitted. @@ -90,7 +84,7 @@ function buildAuditRow( resourceName: params.resourceName, description: params.description, metadata: params.metadata ?? {}, - ipAddress: params.request ? getClientIp(params.request) : undefined, + ipAddress: params.request ? clientIpResolver.resolve(params.request.headers) : undefined, userAgent: params.request?.headers.get('user-agent') ?? undefined, } } diff --git a/packages/security/package.json b/packages/security/package.json index 234b2a8dc1f..782ecd26c5d 100644 --- a/packages/security/package.json +++ b/packages/security/package.json @@ -34,6 +34,10 @@ "types": "./src/hostnames.ts", "default": "./src/hostnames.ts" }, + "./ip": { + "types": "./src/ip.ts", + "default": "./src/ip.ts" + }, "./ssrf": { "types": "./src/ssrf.ts", "default": "./src/ssrf.ts" diff --git a/packages/security/src/ip.test.ts b/packages/security/src/ip.test.ts new file mode 100644 index 00000000000..76a24a7655a --- /dev/null +++ b/packages/security/src/ip.test.ts @@ -0,0 +1,77 @@ +import { createClientIpResolver, normalizeIpAddress } from '@sim/security/ip' +import { describe, expect, it } from 'vitest' + +function headers(values: Record): Headers { + return new Headers(values) +} + +describe('createClientIpResolver', () => { + it('returns null when x-forwarded-for is absent', () => { + const resolver = createClientIpResolver() + + expect(resolver.resolve(headers({ 'x-real-ip': '192.0.2.10' }))).toBeNull() + }) + + it('uses a single forwarded address', () => { + const resolver = createClientIpResolver() + + expect(resolver.resolve(headers({ 'x-forwarded-for': '192.0.2.10' }))).toBe('192.0.2.10') + }) + + it('ignores attacker-controlled values to the left of an appended peer', () => { + const resolver = createClientIpResolver() + + expect(resolver.resolve(headers({ 'x-forwarded-for': '198.51.100.20, 203.0.113.30' }))).toBe( + '203.0.113.30' + ) + }) + + it('walks past configured trusted proxy hops', () => { + const resolver = createClientIpResolver('10.0.0.0/8, 2001:db8:abcd::/48') + + expect( + resolver.resolve(headers({ 'x-forwarded-for': '192.0.2.10, 10.0.0.12, 10.0.0.15' })) + ).toBe('192.0.2.10') + expect(resolver.trustedProxies).toEqual(['10.0.0.0/8', '2001:db8:abcd::/48']) + }) + + it('normalizes forwarded addresses with proxy-added ports', () => { + const resolver = createClientIpResolver() + + expect(resolver.resolve(headers({ 'x-forwarded-for': '192.0.2.10:4312' }))).toBe('192.0.2.10') + expect(resolver.resolve(headers({ 'x-forwarded-for': '[2001:db8::1]:4312' }))).toBe( + '2001:db8::1' + ) + }) + + it('returns null when a required hop is malformed', () => { + const resolver = createClientIpResolver('10.0.0.0/8') + + expect(resolver.resolve(headers({ 'x-forwarded-for': 'spoofed, 10.0.0.12' }))).toBeNull() + }) + + it('returns null when every forwarded hop is trusted', () => { + const resolver = createClientIpResolver('10.0.0.0/8') + + expect(resolver.resolve(headers({ 'x-forwarded-for': '10.0.0.12, 10.0.0.15' }))).toBeNull() + }) + + it('fails fast for invalid or catch-all trusted proxy configuration', () => { + expect(() => createClientIpResolver('not-an-ip')).toThrow( + 'Invalid AUTH_TRUSTED_PROXIES entry "not-an-ip"' + ) + expect(() => createClientIpResolver('0.0.0.0/0')).toThrow('catch-all networks') + expect(() => createClientIpResolver('10.0.0.0/8,')).toThrow('cannot contain empty entries') + }) +}) + +describe('normalizeIpAddress', () => { + it('canonicalizes equivalent IPv6 and IPv4-mapped addresses', () => { + expect(normalizeIpAddress('2001:0db8:0000:0000:0000:0000:0000:0001')).toBe('2001:db8::1') + expect(normalizeIpAddress('::ffff:192.0.2.10')).toBe('192.0.2.10') + }) + + it('rejects invalid addresses', () => { + expect(normalizeIpAddress('not-an-ip')).toBeNull() + }) +}) diff --git a/packages/security/src/ip.ts b/packages/security/src/ip.ts new file mode 100644 index 00000000000..1acb82e1930 --- /dev/null +++ b/packages/security/src/ip.ts @@ -0,0 +1,104 @@ +import * as ipaddr from 'ipaddr.js' + +type IpAddress = ipaddr.IPv4 | ipaddr.IPv6 + +interface TrustedNetwork { + address: IpAddress + prefixLength: number +} + +export interface ForwardedIpHeaders { + get(name: string): string | null +} + +export interface ClientIpResolver { + trustedProxies: string[] + resolve(headers: ForwardedIpHeaders): string | null +} + +function parseForwardedAddress(value: string): IpAddress | null { + const trimmed = value.trim() + if (!trimmed) return null + + let candidate = trimmed + if (trimmed.startsWith('[')) { + const match = /^\[([^\]]+)\](?::\d+)?$/.exec(trimmed) + if (!match) return null + candidate = match[1] + } else if (trimmed.split(':').length === 2) { + const match = /^([^:]+):\d+$/.exec(trimmed) + if (match) candidate = match[1] + } + + if (!ipaddr.isValid(candidate)) return null + const address = ipaddr.process(candidate) + if (address.kind() === 'ipv6' && (address as ipaddr.IPv6).zoneId) return null + return address +} + +/** Returns the canonical form of an IP literal, or `null` when it is invalid. */ +export function normalizeIpAddress(value: string): string | null { + return parseForwardedAddress(value)?.toString() ?? null +} + +function parseTrustedNetwork(value: string): TrustedNetwork { + if (value.includes('/')) { + if (!ipaddr.isValidCIDR(value)) { + throw new Error(`Invalid AUTH_TRUSTED_PROXIES entry "${value}"`) + } + const [address, prefixLength] = ipaddr.parseCIDR(value) + if (prefixLength === 0) { + throw new Error( + `Invalid AUTH_TRUSTED_PROXIES entry "${value}": catch-all networks are unsafe` + ) + } + return { address, prefixLength } + } + + if (!ipaddr.isValid(value)) { + throw new Error(`Invalid AUTH_TRUSTED_PROXIES entry "${value}"`) + } + const address = ipaddr.parse(value) + return { address, prefixLength: address.kind() === 'ipv4' ? 32 : 128 } +} + +function isTrustedProxy(address: IpAddress, networks: TrustedNetwork[]): boolean { + return networks.some( + (network) => + network.address.kind() === address.kind() && + address.match(network.address, network.prefixLength) + ) +} + +/** + * Creates a resolver for proxy-appended `X-Forwarded-For` chains. Resolution + * walks right to left, skips only explicitly trusted proxy hops, and returns + * the first untrusted address. With no trusted proxy list, the rightmost value + * is the only safe address because a reverse proxy appends the network peer + * after any client-supplied values. + */ +export function createClientIpResolver(trustedProxyConfig?: string): ClientIpResolver { + const config = trustedProxyConfig?.trim() + const trustedProxies = config ? config.split(',').map((entry) => entry.trim()) : [] + if (trustedProxies.some((entry) => entry.length === 0)) { + throw new Error('AUTH_TRUSTED_PROXIES cannot contain empty entries') + } + const trustedNetworks = trustedProxies.map(parseTrustedNetwork) + + return { + trustedProxies, + resolve(headers) { + const forwardedFor = headers.get('x-forwarded-for') + if (!forwardedFor) return null + + const hops = forwardedFor.split(',') + for (let index = hops.length - 1; index >= 0; index -= 1) { + const address = parseForwardedAddress(hops[index]) + if (!address) return null + if (!isTrustedProxy(address, trustedNetworks)) return address.toString() + } + + return null + }, + } +} diff --git a/packages/testing/src/mocks/request.mock.ts b/packages/testing/src/mocks/request.mock.ts index 614366ad938..d39dccb3e6c 100644 --- a/packages/testing/src/mocks/request.mock.ts +++ b/packages/testing/src/mocks/request.mock.ts @@ -79,7 +79,9 @@ export function createMockFormDataRequest( */ export const requestUtilsMockFns = { mockGenerateRequestId: vi.fn(() => 'mock-request-id'), - mockGetClientIp: vi.fn(() => '127.0.0.1'), + mockGetClientIp: vi.fn( + (_request: { headers: { get(name: string): string | null } }): string | null => '127.0.0.1' + ), } /** @@ -93,5 +95,6 @@ export const requestUtilsMockFns = { export const requestUtilsMock = { generateRequestId: requestUtilsMockFns.mockGenerateRequestId, getClientIp: requestUtilsMockFns.mockGetClientIp, + trustedProxies: [], noop: () => {}, }