From 59fd72d6becc9ba3565c331d2c16247af6aeef71 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 12:27:32 -0700 Subject: [PATCH 1/8] fix(security): key per-IP rate limits on the proxy-written forwarded hop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getClientIp derived the client IP from the leftmost X-Forwarded-For entry. Under any proxy that appends to that header — nginx-ingress, HAProxy, Cloudflare, and both reference deployments in this repo — the leftmost entry is supplied by the caller, so rotating it minted a fresh token bucket per request and every per-IP throttle became a no-op: the contact and demo-request mailers, telemetry, the docs Ask-AI endpoint, and public-deployment password attempts. The repo already treated that hop as untrusted for Better Auth via AUTH_TRUSTED_PROXIES; Sim's own helper never consulted it. packages/audit carried a second copy of the same function, forging audit-row IPs. Resolve the chain right to left instead, skipping configured trusted hops and returning the first untrusted address — the closest hop the infrastructure actually vouched for. Shared from @sim/security/client-ip so the app, the docs app, and the audit package cannot drift again. - fall back to the rightmost hop, never the leftmost, when every hop is trusted, so forging an address inside a broad configured range (the docs recommend 10.0.0.0/16) cannot reinstate the bypass - strip IPv6 zone ids, which ipaddr accepts at arbitrary length and would otherwise hand a caller unlimited distinct bucket keys - canonicalize addresses so equivalent spellings share one bucket - bound consecutive failed password guesses per deployment, not just per IP, since a distributed caller gets a fresh IP bucket per source The generic webhook allowlist keeps leftmost semantics via getAssertedOriginIp: it names the sending service, not the proxy, so resolving it like a throttle key would have 403'd every allowlisted delivery. Both sides are now canonicalized. Operators behind a multi-hop chain should set AUTH_TRUSTED_PROXIES to their real hops; unset is safe but collapses callers onto the edge address. --- apps/docs/app/api/chat/route.ts | 19 +- apps/docs/package.json | 1 + apps/sim/.env.example | 2 +- .../app/api/chat/[identifier]/otp/route.ts | 3 +- .../app/api/chat/[identifier]/sso/route.ts | 3 +- apps/sim/app/api/chat/utils.test.ts | 71 ++++++ apps/sim/app/api/contact/route.ts | 3 +- apps/sim/app/api/demo-requests/route.ts | 3 +- .../app/api/files/public/[token]/otp/route.ts | 3 +- .../app/api/files/public/[token]/sso/route.ts | 3 +- .../app/api/help/integration-request/route.ts | 3 +- apps/sim/app/api/speech/token/route.ts | 2 +- apps/sim/lib/analytics/profound.ts | 2 +- apps/sim/lib/auth/internal.ts | 2 +- apps/sim/lib/core/config/env.ts | 2 +- .../sim/lib/core/rate-limiter/rate-limiter.ts | 20 ++ .../core/rate-limiter/route-helpers.test.ts | 21 +- .../lib/core/rate-limiter/route-helpers.ts | 2 +- apps/sim/lib/core/security/deployment-auth.ts | 47 +++- apps/sim/lib/core/utils/client-ip.ts | 28 +++ apps/sim/lib/core/utils/request.ts | 11 - apps/sim/lib/public-shares/rate-limit.ts | 2 +- apps/sim/lib/webhooks/providers/generic.ts | 23 +- apps/sim/proxy.ts | 2 +- apps/sim/vitest.setup.ts | 2 + bun.lock | 2 + docker-compose.prod.yml | 13 +- helm/sim/values.yaml | 10 +- packages/audit/package.json | 1 + packages/audit/src/log.test.ts | 40 ++-- packages/audit/src/log.ts | 28 ++- packages/security/package.json | 4 + packages/security/src/client-ip.test.ts | 202 +++++++++++++++++ packages/security/src/client-ip.ts | 206 ++++++++++++++++++ packages/testing/src/mocks/index.ts | 1 + packages/testing/src/mocks/request.mock.ts | 18 +- 36 files changed, 729 insertions(+), 76 deletions(-) create mode 100644 apps/sim/lib/core/utils/client-ip.ts create mode 100644 packages/security/src/client-ip.test.ts create mode 100644 packages/security/src/client-ip.ts diff --git a/apps/docs/app/api/chat/route.ts b/apps/docs/app/api/chat/route.ts index 915fe9a39c4..76f7c40f6ec 100644 --- a/apps/docs/app/api/chat/route.ts +++ b/apps/docs/app/api/chat/route.ts @@ -1,4 +1,5 @@ import { openai } from '@ai-sdk/openai' +import { parseTrustedProxies, resolveClientIp } from '@sim/security/client-ip' import { convertToModelMessages, jsonSchema, @@ -69,11 +70,21 @@ const RATE_LIMIT_MAX = 20 const RATE_LIMIT_WINDOW_MS = 60_000 const rateLimitHits = new Map() -/** Resolve the client IP from forwarding headers, falling back to a shared bucket. */ +/** + * Reverse-proxy hops trusted for forwarded-IP resolution — the same + * `AUTH_TRUSTED_PROXIES` the main app reads. Parsed once at module load. + */ +const trustedProxies = parseTrustedProxies(process.env.AUTH_TRUSTED_PROXIES) + +/** + * Resolve the client IP from forwarding headers, falling back to a shared + * bucket. Walks the chain right to left: the leftmost `X-Forwarded-For` entry is + * caller-supplied, so keying this limit on it would let anyone rotate the header + * to mint a fresh bucket per request — and, on this endpoint, unmetered model + * spend plus unbounded growth of `rateLimitHits`. See {@link resolveClientIp}. + */ function getClientIp(req: Request): string { - const forwarded = req.headers.get('x-forwarded-for') - if (forwarded) return forwarded.split(',')[0].trim() - return req.headers.get('x-real-ip') ?? 'unknown' + return resolveClientIp(req, trustedProxies) } /** Fixed-window check. Returns retry-after seconds when the caller is over the limit, else null. */ diff --git a/apps/docs/package.json b/apps/docs/package.json index a1f451235d1..682c35b0890 100644 --- a/apps/docs/package.json +++ b/apps/docs/package.json @@ -21,6 +21,7 @@ "@ai-sdk/react": "2.0.205", "@sim/db": "workspace:*", "@sim/emcn": "workspace:*", + "@sim/security": "workspace:*", "@sim/workflow-renderer": "workspace:*", "ai": "5.0.203", "class-variance-authority": "^0.7.1", diff --git a/apps/sim/.env.example b/apps/sim/.env.example index db177410995..7209da4e6e3 100644 --- a/apps/sim/.env.example +++ b/apps/sim/.env.example @@ -19,7 +19,7 @@ BETTER_AUTH_URL=http://localhost:3000 NEXT_PUBLIC_APP_URL=http://localhost:3000 # INTERNAL_API_BASE_URL=http://sim-app.default.svc.cluster.local:3000 # Optional: internal URL for server-side /api self-calls; defaults to NEXT_PUBLIC_APP_URL # TRUSTED_ORIGINS=https://www.example.com,https://app.example.com # Optional: comma-separated additional public origins to trust for auth (apex+www, alias domains). Merged into Better Auth trustedOrigins. -# AUTH_TRUSTED_PROXIES=10.0.0.0/24,192.0.2.10 # Optional: reverse-proxy IPs/CIDRs in front of the app. Better Auth walks x-forwarded-for right to left, skips these hops, and uses the first untrusted address as the client IP (prevents forwarded-header spoofing). Use your proxies' actual addresses, not broad private ranges that also cover clients. +# AUTH_TRUSTED_PROXIES=10.0.0.0/24,192.0.2.10 # Optional: reverse-proxy IPs/CIDRs in front of the app. Better Auth and Sim's own per-IP throttles walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP (the leftmost entry is caller-supplied and would otherwise let anyone mint a fresh rate-limit bucket per request). Unset trusts no hop and keys on the rightmost entry — safe, but a multi-hop chain collapses callers onto the edge addresses. Use your proxies' actual addresses, not broad private ranges that also cover clients. # Chat (Optional) # COPILOT_API_KEY= # Mint one at https://sim.ai. Without it the Sim Chat block, prompt jobs, and Inbox cannot run diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.ts b/apps/sim/app/api/chat/[identifier]/otp/route.ts index 9f7fffd741f..09670f4a7ad 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.ts @@ -19,7 +19,8 @@ import { OTP_IP_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' +import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { sendEmail } from '@/lib/messaging/email/mailer' import { setChatAuthCookie } from '@/app/api/chat/utils' diff --git a/apps/sim/app/api/chat/[identifier]/sso/route.ts b/apps/sim/app/api/chat/[identifier]/sso/route.ts index c6ab98cfe94..08bebebf245 100644 --- a/apps/sim/app/api/chat/[identifier]/sso/route.ts +++ b/apps/sim/app/api/chat/[identifier]/sso/route.ts @@ -8,7 +8,8 @@ import { parseRequest } from '@/lib/api/server' import type { TokenBucketConfig } from '@/lib/core/rate-limiter' import { RateLimiter } from '@/lib/core/rate-limiter' import { isEmailAllowed } from '@/lib/core/security/deployment' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' +import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { createErrorResponse, createSuccessResponse } from '@/app/api/workflows/utils' diff --git a/apps/sim/app/api/chat/utils.test.ts b/apps/sim/app/api/chat/utils.test.ts index 6c41eeb21cc..0dd9883000b 100644 --- a/apps/sim/app/api/chat/utils.test.ts +++ b/apps/sim/app/api/chat/utils.test.ts @@ -20,6 +20,7 @@ const { mockSetDeploymentAuthCookie, mockIsEmailAllowed, mockCheckRateLimitDirect, + mockResetRateLimitBucket, } = vi.hoisted(() => ({ mockMergeSubblockStateWithValues: vi.fn().mockReturnValue({}), mockMergeSubBlockValues: vi.fn().mockReturnValue({}), @@ -27,11 +28,13 @@ const { mockSetDeploymentAuthCookie: vi.fn(), mockIsEmailAllowed: vi.fn(), mockCheckRateLimitDirect: vi.fn().mockResolvedValue({ allowed: true }), + mockResetRateLimitBucket: vi.fn().mockResolvedValue(undefined), })) vi.mock('@/lib/core/rate-limiter', () => ({ RateLimiter: class { checkRateLimitDirect = mockCheckRateLimitDirect + resetRateLimitBucket = mockResetRateLimitBucket }, })) @@ -212,6 +215,74 @@ describe('Chat API Utils', () => { expect(result.authorized).toBe(true) }) + it('clears the per-resource failure counter once a password verifies', async () => { + const deployment = { + id: 'chat-id', + authType: 'password', + password: 'encrypted-password', + } + + const mockRequest = { + method: 'POST', + cookies: { get: vi.fn().mockReturnValue(null) }, + } as any + + await validateChatAuth('request-id', deployment, mockRequest, { + password: 'correct-password', + }) + + expect(mockResetRateLimitBucket).toHaveBeenCalledWith('chat-password:resource:chat-id') + }) + + it('leaves the per-resource failure counter consumed when the password is wrong', async () => { + const deployment = { + id: 'chat-id', + authType: 'password', + password: 'encrypted-password', + } + + const mockRequest = { + method: 'POST', + cookies: { get: vi.fn().mockReturnValue(null) }, + } as any + + const result = await validateChatAuth('request-id', deployment, mockRequest, { + password: 'wrong-password', + }) + + expect(result.authorized).toBe(false) + expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( + 'chat-password:resource:chat-id', + expect.objectContaining({ maxTokens: 500 }) + ) + expect(mockResetRateLimitBucket).not.toHaveBeenCalled() + }) + + it('rejects guesses once the per-resource counter is exhausted, without decrypting', async () => { + const deployment = { + id: 'chat-id', + authType: 'password', + password: 'encrypted-password', + } + + const mockRequest = { + method: 'POST', + cookies: { get: vi.fn().mockReturnValue(null) }, + } as any + + mockCheckRateLimitDirect.mockImplementation(async (key: string) => + key.includes(':resource:') ? { allowed: false, retryAfterMs: 900_000 } : { allowed: true } + ) + + const result = await validateChatAuth('request-id', deployment, mockRequest, { + password: 'guess', + }) + + expect(result.authorized).toBe(false) + expect(result.status).toBe(429) + expect(decryptSecret).not.toHaveBeenCalled() + }) + it('should reject incorrect password', async () => { const deployment = { id: 'chat-id', diff --git a/apps/sim/app/api/contact/route.ts b/apps/sim/app/api/contact/route.ts index 2b610ec2114..a8738e63f67 100644 --- a/apps/sim/app/api/contact/route.ts +++ b/apps/sim/app/api/contact/route.ts @@ -11,7 +11,8 @@ import { env } from '@/lib/core/config/env' import type { TokenBucketConfig } from '@/lib/core/rate-limiter' import { RateLimiter } from '@/lib/core/rate-limiter' import { isTurnstileConfigured, verifyTurnstileToken } from '@/lib/core/security/turnstile' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' +import { generateRequestId } from '@/lib/core/utils/request' import { getEmailDomain } from '@/lib/core/utils/urls' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { sendEmail } from '@/lib/messaging/email/mailer' diff --git a/apps/sim/app/api/demo-requests/route.ts b/apps/sim/app/api/demo-requests/route.ts index 7553239e7b2..013353b44bf 100644 --- a/apps/sim/app/api/demo-requests/route.ts +++ b/apps/sim/app/api/demo-requests/route.ts @@ -8,7 +8,8 @@ import { parseRequest } from '@/lib/api/server' import { env } from '@/lib/core/config/env' import type { TokenBucketConfig } from '@/lib/core/rate-limiter' import { RateLimiter } from '@/lib/core/rate-limiter' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' +import { generateRequestId } from '@/lib/core/utils/request' import { getEmailDomain } from '@/lib/core/utils/urls' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { sendEmail } from '@/lib/messaging/email/mailer' 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 0dd240788fd..4ea958eee9e 100644 --- a/apps/sim/app/api/files/public/[token]/otp/route.ts +++ b/apps/sim/app/api/files/public/[token]/otp/route.ts @@ -21,7 +21,8 @@ import { OTP_IP_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' +import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { sendEmail } from '@/lib/messaging/email/mailer' import { resolveActiveShareByToken } from '@/lib/public-shares/share-manager' 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..ed1f93e84df 100644 --- a/apps/sim/app/api/files/public/[token]/sso/route.ts +++ b/apps/sim/app/api/files/public/[token]/sso/route.ts @@ -7,7 +7,8 @@ import { parseRequest } from '@/lib/api/server' import type { TokenBucketConfig } from '@/lib/core/rate-limiter' import { RateLimiter } from '@/lib/core/rate-limiter' import { isEmailAllowed } from '@/lib/core/security/deployment' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' +import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { resolveActiveShareByToken } from '@/lib/public-shares/share-manager' diff --git a/apps/sim/app/api/help/integration-request/route.ts b/apps/sim/app/api/help/integration-request/route.ts index 6a8faf682b6..f78141aa497 100644 --- a/apps/sim/app/api/help/integration-request/route.ts +++ b/apps/sim/app/api/help/integration-request/route.ts @@ -5,7 +5,8 @@ import { parseRequest, validationErrorResponse } from '@/lib/api/server' import { env } from '@/lib/core/config/env' import type { TokenBucketConfig } from '@/lib/core/rate-limiter' import { RateLimiter } from '@/lib/core/rate-limiter' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' +import { generateRequestId } from '@/lib/core/utils/request' import { getEmailDomain } from '@/lib/core/utils/urls' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { sendEmail } from '@/lib/messaging/email/mailer' diff --git a/apps/sim/app/api/speech/token/route.ts b/apps/sim/app/api/speech/token/route.ts index aacadc145d5..4b520dc651a 100644 --- a/apps/sim/app/api/speech/token/route.ts +++ b/apps/sim/app/api/speech/token/route.ts @@ -22,7 +22,7 @@ import { env } from '@/lib/core/config/env' import { getCostMultiplier, isBillingEnabled } from '@/lib/core/config/env-flags' import { RateLimiter } from '@/lib/core/rate-limiter' import { validateAuthToken } from '@/lib/core/security/deployment' -import { getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { verifyWorkspaceMembership } from '@/app/api/workflows/utils' diff --git a/apps/sim/lib/analytics/profound.ts b/apps/sim/lib/analytics/profound.ts index ff8c568e14d..2a254d1265a 100644 --- a/apps/sim/lib/analytics/profound.ts +++ b/apps/sim/lib/analytics/profound.ts @@ -8,7 +8,7 @@ import { createLogger } from '@sim/logger' import { env } from '@/lib/core/config/env' import { isHosted } from '@/lib/core/config/env-flags' -import { getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' import { getBaseDomain } from '@/lib/core/utils/urls' const logger = createLogger('ProfoundAnalytics') diff --git a/apps/sim/lib/auth/internal.ts b/apps/sim/lib/auth/internal.ts index a94ee42011a..95d7ffb613a 100644 --- a/apps/sim/lib/auth/internal.ts +++ b/apps/sim/lib/auth/internal.ts @@ -3,7 +3,7 @@ import { safeCompare } from '@sim/security/compare' import { jwtVerify, SignJWT } from 'jose' import { type NextRequest, NextResponse } from 'next/server' import { env } from '@/lib/core/config/env' -import { getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' const logger = createLogger('CronAuth') diff --git a/apps/sim/lib/core/config/env.ts b/apps/sim/lib/core/config/env.ts index 9d16e13ab4f..efca1f5f53f 100644 --- a/apps/sim/lib/core/config/env.ts +++ b/apps/sim/lib/core/config/env.ts @@ -507,7 +507,7 @@ 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. + AUTH_TRUSTED_PROXIES: z.string().optional(), // Comma-separated reverse-proxy IPs or CIDR ranges. Better Auth and getClientIp (per-IP rate-limit keys, audit rows) walk the forwarded-IP chain right to left, skip these trusted hops, and use the first untrusted address as the client IP. Unset trusts no hop and keys on the rightmost, proxy-written entry — never the caller-supplied leftmost one. // SSO Configuration (for script-based registration) SSO_ENABLED: z.boolean().optional(), // Enable SSO functionality diff --git a/apps/sim/lib/core/rate-limiter/rate-limiter.ts b/apps/sim/lib/core/rate-limiter/rate-limiter.ts index 9e274839d86..8230801dc35 100644 --- a/apps/sim/lib/core/rate-limiter/rate-limiter.ts +++ b/apps/sim/lib/core/rate-limiter/rate-limiter.ts @@ -204,6 +204,26 @@ export class RateLimiter { } } + /** + * Clears a single bucket addressed by its exact storage key — the counterpart + * to {@link checkRateLimitDirect}. Lets a caller keep a failure-only counter + * (consume on every attempt, reset once the attempt succeeds) so legitimate + * traffic never walks the bucket down. + * + * Never throws: a reset that fails leaves tokens consumed, which only makes + * the limit stricter. + */ + async resetRateLimitBucket(storageKey: string): Promise { + try { + await this.storage.resetBucket(storageKey) + } catch (error) { + logger.warn('Failed to reset rate limit bucket', { + storageKey, + error: toError(error).message, + }) + } + } + async resetRateLimit(rateLimitKey: string): Promise { try { await Promise.all([ 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..17b0af73a94 100644 --- a/apps/sim/lib/core/rate-limiter/route-helpers.test.ts +++ b/apps/sim/lib/core/rate-limiter/route-helpers.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { type ClientIpHeaderSource, resolveClientIp } from '@sim/security/client-ip' import { createMockRequest, requestUtilsMockFns } from '@sim/testing' import { beforeEach, describe, expect, it, type Mock, vi } from 'vitest' @@ -22,12 +23,14 @@ vi.mock('@/lib/core/rate-limiter/storage', async () => { } }) +/** + * Route the globally-mocked `getClientIp` through the real resolver, so these + * assertions exercise the actual forwarded-header semantics rather than a + * hand-rolled restatement of them that could drift from the implementation. + */ 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' + requestUtilsMockFns.mockGetClientIp.mockImplementation((req: ClientIpHeaderSource) => + resolveClientIp(req) ) } @@ -106,7 +109,7 @@ describe('route-helpers rate limiting', () => { passThroughClientIp() }) - it('uses the X-Forwarded-For client IP in the bucket key', async () => { + it('keys on the proxy-appended hop, not the caller-supplied leftmost one', async () => { consume.mockResolvedValueOnce({ allowed: true, tokensRemaining: 9, @@ -118,11 +121,7 @@ describe('route-helpers rate limiting', () => { await enforceIpRateLimit('public-bucket', request) - expect(consume).toHaveBeenCalledWith( - 'route:public-bucket:ip:203.0.113.7', - 1, - expect.any(Object) - ) + expect(consume).toHaveBeenCalledWith('route:public-bucket:ip:10.0.0.1', 1, expect.any(Object)) }) it('folds spoofed `X-Forwarded-For: unknown` into a single shared bucket', async () => { diff --git a/apps/sim/lib/core/rate-limiter/route-helpers.ts b/apps/sim/lib/core/rate-limiter/route-helpers.ts index f71115bf532..bb5b4e512cd 100644 --- a/apps/sim/lib/core/rate-limiter/route-helpers.ts +++ b/apps/sim/lib/core/rate-limiter/route-helpers.ts @@ -2,7 +2,7 @@ import { createLogger } from '@sim/logger' import { type NextRequest, NextResponse } from 'next/server' import { RateLimiter } from '@/lib/core/rate-limiter/rate-limiter' import type { TokenBucketConfig } from '@/lib/core/rate-limiter/storage' -import { getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' const logger = createLogger('RouteRateLimit') const rateLimiter = new RateLimiter() diff --git a/apps/sim/lib/core/security/deployment-auth.ts b/apps/sim/lib/core/security/deployment-auth.ts index 69c842def61..2a56d6bd148 100644 --- a/apps/sim/lib/core/security/deployment-auth.ts +++ b/apps/sim/lib/core/security/deployment-auth.ts @@ -10,7 +10,7 @@ import { validateAuthToken, } from '@/lib/core/security/deployment' import { decryptSecret } from '@/lib/core/security/encryption' -import { getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' const logger = createLogger('DeploymentAuth') @@ -26,6 +26,31 @@ const PASSWORD_IP_RATE_LIMIT: TokenBucketConfig = { refillIntervalMs: 15 * 60_000, } +/** + * Bounds *consecutive failed* guesses against one deployment secret, keyed on + * the resource rather than the caller. The IP bucket above cannot be the only + * defense: a distributed caller simply gets a fresh IP bucket per source, which + * leaves the secret itself with no ceiling at all. + * + * A token is consumed per attempt and the bucket is reset the moment a password + * verifies, so this counts *consecutive* failures — a resource that anyone is + * successfully signing into never drifts toward the limit. + * + * The ceiling is a deliberate trade, not a free win: because the check must run + * before the comparison to be worth anything, an exhausted bucket also rejects + * the correct password, so whoever burns it through locks out new visitors for + * the rest of the window (holders of an auth cookie are unaffected — that path + * returns before this one). It is sized so that only a genuinely distributed + * attack can get there: at 10 attempts per IP per window, tripping it takes ~50 + * distinct source addresses, while still capping blind guessing at 500 per 15 + * minutes instead of the unbounded rate a single spoofed header used to buy. + */ +const PASSWORD_RESOURCE_RATE_LIMIT: TokenBucketConfig = { + maxTokens: 500, + refillRate: 500, + refillIntervalMs: 15 * 60_000, +} + /** * 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. @@ -122,11 +147,31 @@ export async function validateDeploymentAuth( } } + const resourceKey = `${cookiePrefix}-password:resource:${resource.id}` + const resourceRateLimit = await rateLimiter.checkRateLimitDirect( + resourceKey, + PASSWORD_RESOURCE_RATE_LIMIT + ) + if (!resourceRateLimit.allowed) { + logger.warn( + `[${requestId}] Password attempt resource rate limit exceeded for ${resource.id}` + ) + return { + authorized: false, + error: 'Too many attempts. Please try again later.', + status: 429, + retryAfterMs: + resourceRateLimit.retryAfterMs ?? PASSWORD_RESOURCE_RATE_LIMIT.refillIntervalMs, + } + } + const { decrypted } = await decryptSecret(resource.password) if (!safeCompare(password, decrypted)) { return { authorized: false, error: 'Invalid password' } } + await rateLimiter.resetRateLimitBucket(resourceKey) + return { authorized: true } } catch (error) { logger.error(`[${requestId}] Error validating password:`, error) diff --git a/apps/sim/lib/core/utils/client-ip.ts b/apps/sim/lib/core/utils/client-ip.ts new file mode 100644 index 00000000000..a03c98486d5 --- /dev/null +++ b/apps/sim/lib/core/utils/client-ip.ts @@ -0,0 +1,28 @@ +import { + type ClientIpHeaderSource, + parseTrustedProxies, + resolveClientIp, +} from '@sim/security/client-ip' +import { env } from '@/lib/core/config/env' + +/** + * Reverse-proxy hops trusted for forwarded-IP resolution, shared with Better + * Auth's `advanced.ipAddress.trustedProxies` so session IPs and Sim's own + * rate-limit keys agree on who the caller is. Parsed once at module load. + */ +const trustedProxies = parseTrustedProxies(env.AUTH_TRUSTED_PROXIES) + +/** + * Extract the client IP from a request for logging, audit trails, and — most + * importantly — per-IP rate-limit keys. + * + * Server-only: kept out of `@/lib/core/utils/request` so the `ipaddr.js` + * dependency never reaches a client bundle through that module's other exports. + * + * See {@link resolveClientIp} for why the chain is walked right to left. In + * short: the leftmost `X-Forwarded-For` entry is supplied by the caller, so + * keying a throttle on it lets anyone mint a fresh bucket per request. + */ +export function getClientIp(request: ClientIpHeaderSource): string { + return resolveClientIp(request, trustedProxies) +} diff --git a/apps/sim/lib/core/utils/request.ts b/apps/sim/lib/core/utils/request.ts index 3634c2f38c9..07b5f8ad814 100644 --- a/apps/sim/lib/core/utils/request.ts +++ b/apps/sim/lib/core/utils/request.ts @@ -10,17 +10,6 @@ export function generateRequestId(): string { return getRequestContext()?.requestId ?? generateId().slice(0, 8) } -/** - * Extract the client IP from a request, checking `x-forwarded-for` then `x-real-ip`. - */ -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' - ) -} - /** * No-operation function for use as default callback */ diff --git a/apps/sim/lib/public-shares/rate-limit.ts b/apps/sim/lib/public-shares/rate-limit.ts index 60f7223a60d..696782afdb5 100644 --- a/apps/sim/lib/public-shares/rate-limit.ts +++ b/apps/sim/lib/public-shares/rate-limit.ts @@ -1,6 +1,6 @@ import { NextResponse } from 'next/server' import { RateLimiter, type TokenBucketConfig } from '@/lib/core/rate-limiter' -import { getClientIp } from '@/lib/core/utils/request' +import { getClientIp } from '@/lib/core/utils/client-ip' const rateLimiter = new RateLimiter() diff --git a/apps/sim/lib/webhooks/providers/generic.ts b/apps/sim/lib/webhooks/providers/generic.ts index 71372bebad6..1777fe21422 100644 --- a/apps/sim/lib/webhooks/providers/generic.ts +++ b/apps/sim/lib/webhooks/providers/generic.ts @@ -1,6 +1,6 @@ import { createLogger } from '@sim/logger' +import { canonicalizeIp, getAssertedOriginIp } from '@sim/security/client-ip' import { NextResponse } from 'next/server' -import { getClientIp } from '@/lib/core/utils/request' import type { AuthContext, EventFilterContext, @@ -31,9 +31,26 @@ export const genericHandler: WebhookProviderHandler = { const allowedIps = providerConfig.allowedIps if (allowedIps && Array.isArray(allowedIps) && allowedIps.length > 0) { - const clientIp = getClientIp(request) + /** + * Matches the *asserted* origin — the leftmost forwarded hop — because the + * operator's allowlist names the sending service (Stripe, GitHub, …), not + * the proxy in front of us. Rate-limit keys deliberately use the opposite + * end of the chain; see {@link getAssertedOriginIp} for why this value is + * a filter against honest senders rather than authentication. `requireAuth` + * is the control that actually authenticates. + * + * Both sides are canonicalized so an entry written as `::ffff:1.2.3.4` or + * `01.02.03.04` still matches the same address on the wire. + */ + const clientIp = getAssertedOriginIp(request) + const allowed = new Set( + allowedIps.flatMap((entry) => { + const canonical = typeof entry === 'string' ? canonicalizeIp(entry) : null + return canonical ? [canonical] : [] + }) + ) - if (clientIp === 'unknown' || !allowedIps.includes(clientIp)) { + if (!clientIp || !allowed.has(clientIp)) { logger.warn(`[${requestId}] Forbidden webhook access attempt - IP not allowed: ${clientIp}`) return new NextResponse('Forbidden - IP not allowed', { status: 403, diff --git a/apps/sim/proxy.ts b/apps/sim/proxy.ts index 73d03e9b797..adbffe8a528 100644 --- a/apps/sim/proxy.ts +++ b/apps/sim/proxy.ts @@ -5,7 +5,7 @@ import { sendToProfound } from './lib/analytics/profound' import { getEnv } from './lib/core/config/env' import { isAuthDisabled, isDev, isHosted } from './lib/core/config/env-flags' import { generateRuntimeCSP } from './lib/core/security/csp' -import { getClientIp } from './lib/core/utils/request' +import { getClientIp } from './lib/core/utils/client-ip' import { isNonCanonicalSimHost } from './lib/core/utils/urls' const logger = createLogger('Proxy') diff --git a/apps/sim/vitest.setup.ts b/apps/sim/vitest.setup.ts index 25c34639607..b0e7859ee9e 100644 --- a/apps/sim/vitest.setup.ts +++ b/apps/sim/vitest.setup.ts @@ -1,5 +1,6 @@ import { authMock, + clientIpMock, databaseMock, drizzleOrmMock, envFlagsMock, @@ -38,6 +39,7 @@ vi.mock('@sim/platform-authz/workflow', () => workflowAuthzMock) vi.mock('@/lib/auth', () => authMock) vi.mock('@/lib/auth/hybrid', () => hybridAuthMock) vi.mock('@/lib/core/utils/request', () => requestUtilsMock) +vi.mock('@/lib/core/utils/client-ip', () => clientIpMock) vi.mock('@/lib/core/config/env-flags', () => envFlagsMock) vi.mock('@/lib/core/config/env', () => envMock) vi.mock('@/lib/core/utils/urls', () => urlsMock) diff --git a/bun.lock b/bun.lock index 7670781748c..2166f729872 100644 --- a/bun.lock +++ b/bun.lock @@ -65,6 +65,7 @@ "@ai-sdk/react": "2.0.205", "@sim/db": "workspace:*", "@sim/emcn": "workspace:*", + "@sim/security": "workspace:*", "@sim/workflow-renderer": "workspace:*", "ai": "5.0.203", "class-variance-authority": "^0.7.1", @@ -384,6 +385,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 363422c3013..b796472d8da 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -21,10 +21,15 @@ 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). Better Auth AND Sim's own + # per-IP throttles walk x-forwarded-for right to left, skip these hops, and + # use the first untrusted address as the client IP — never the leftmost + # entry, which the caller supplies and could otherwise rotate to mint a + # fresh rate-limit bucket per request. Empty by default, which trusts no + # hop and keys on the rightmost (proxy-written) entry: safe, but behind a + # multi-hop chain (e.g. CDN in front of ingress) it collapses callers onto + # the edge addresses. Set it to your real hops for per-client keying and + # correct session IPs. - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-} - BETTER_AUTH_SECRET=${BETTER_AUTH_SECRET} - ENCRYPTION_KEY=${ENCRYPTION_KEY} diff --git a/helm/sim/values.yaml b/helm/sim/values.yaml index 72f8cf52551..803fa7c10d2 100644 --- a/helm/sim/values.yaml +++ b/helm/sim/values.yaml @@ -80,9 +80,13 @@ app: # Merged into Better Auth `trustedOrigins` alongside NEXT_PUBLIC_APP_URL. Leave empty when serving from a single origin. TRUSTED_ORIGINS: "" # AUTH_TRUSTED_PROXIES: comma-separated reverse-proxy IPs or CIDR ranges in front of the app - # (ingress controller, 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 (e.g. "10.0.0.0/16"). + # (ingress controller, load balancer). Better Auth AND Sim's own per-IP throttles walk + # x-forwarded-for right to left, skip these hops, and use the first untrusted address as the + # client IP — never the leftmost entry, which the caller supplies and could otherwise be + # rotated to mint a fresh rate-limit bucket per request. Empty trusts no hop and keys on the + # rightmost (proxy-written) entry: safe, but behind a multi-hop chain (e.g. CDN in front of + # ingress) it collapses callers onto the edge addresses. Set your real hops (e.g. "10.0.0.0/16") + # for per-client keying and correct session IPs. AUTH_TRUSTED_PROXIES: "" # SOCKET_SERVER_URL: Auto-detected when realtime.enabled=true (uses internal service) # NEXT_PUBLIC_SOCKET_URL: public WebSocket URL for browsers. Leave empty to default to the diff --git a/packages/audit/package.json b/packages/audit/package.json index caaf323d8d5..dec8f4ae398 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 98a71773e65..c95726ffc03 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', () => ({ @@ -75,12 +69,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(() => { @@ -139,7 +127,7 @@ describe('recordAudit', () => { ) }) - it('extracts IP address from x-forwarded-for header', async () => { + it('records the proxy-supplied x-forwarded-for hop, not the caller-supplied one', async () => { const request = new Request('https://example.com', { headers: { 'x-forwarded-for': '1.2.3.4, 5.6.7.8', @@ -161,12 +149,34 @@ describe('recordAudit', () => { expect(dbChainMockFns.values).toHaveBeenCalledWith( expect.objectContaining({ - ipAddress: '1.2.3.4', + ipAddress: '5.6.7.8', userAgent: 'TestAgent/1.0', }) ) }) + it('does not let a caller forge the audited IP by prepending to x-forwarded-for', async () => { + const request = new Request('https://example.com', { + headers: { 'x-forwarded-for': '203.0.113.9, 5.6.7.8' }, + }) + + recordAudit({ + workspaceId: 'ws-1', + actorId: 'user-1', + actorName: 'Test', + actorEmail: 'test@test.com', + action: AuditAction.MEMBER_INVITED, + resourceType: AuditResourceType.WORKSPACE, + request, + }) + + await flush() + + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ ipAddress: '5.6.7.8' }) + ) + }) + it('falls back to x-real-ip when x-forwarded-for is absent', async () => { const request = new Request('https://example.com', { headers: { 'x-real-ip': '10.0.0.1' }, diff --git a/packages/audit/src/log.ts b/packages/audit/src/log.ts index 93381ae43b7..704f1e48804 100644 --- a/packages/audit/src/log.ts +++ b/packages/audit/src/log.ts @@ -1,5 +1,10 @@ import { auditLog, db, user } from '@sim/db' import { createLogger } from '@sim/logger' +import { + type ClientIpHeaderSource, + parseTrustedProxies, + resolveClientIp, +} from '@sim/security/client-ip' import { generateShortId } from '@sim/utils/id' import { eq } from 'drizzle-orm' import type { AuditActionType, AuditResourceTypeValue } from './types' @@ -23,15 +28,24 @@ interface AuditLogParams { resourceName?: string description?: string metadata?: Record - request?: { headers: { get(name: string): string | null } } + request?: ClientIpHeaderSource } -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' - ) +/** + * Reverse-proxy hops trusted for forwarded-IP resolution. Read from the + * environment directly rather than the app's env module so this package stays + * free of `apps/*` imports; the value is the same `AUTH_TRUSTED_PROXIES` Better + * Auth and Sim's rate limiters use, so an audit row's IP matches the session's. + */ +const trustedProxies = parseTrustedProxies(process.env.AUTH_TRUSTED_PROXIES) + +/** + * An audit row's `ipAddress` is forensic evidence, so it must not be whatever + * the caller put in the leftmost `X-Forwarded-For` entry. See + * {@link resolveClientIp}. + */ +function getClientIp(request: ClientIpHeaderSource): string { + return resolveClientIp(request, trustedProxies) } /** diff --git a/packages/security/package.json b/packages/security/package.json index 68b9e74dfb6..e839e44f229 100644 --- a/packages/security/package.json +++ b/packages/security/package.json @@ -10,6 +10,10 @@ "node": ">=20.0.0" }, "exports": { + "./client-ip": { + "types": "./src/client-ip.ts", + "default": "./src/client-ip.ts" + }, "./compare": { "types": "./src/compare.ts", "default": "./src/compare.ts" diff --git a/packages/security/src/client-ip.test.ts b/packages/security/src/client-ip.test.ts new file mode 100644 index 00000000000..12d53e84523 --- /dev/null +++ b/packages/security/src/client-ip.test.ts @@ -0,0 +1,202 @@ +import { describe, expect, it } from 'vitest' +import { + canonicalizeIp, + getAssertedOriginIp, + parseTrustedProxies, + resolveClientIp, + UNKNOWN_CLIENT_IP, +} from './client-ip' + +function req(headers: Record) { + return { headers: new Headers(headers) } +} + +describe('resolveClientIp', () => { + describe('spoofing resistance', () => { + it('ignores a caller-supplied leftmost hop in favour of the proxy-appended one', () => { + expect(resolveClientIp(req({ 'x-forwarded-for': '203.0.113.7, 198.51.100.4' }))).toBe( + '198.51.100.4' + ) + }) + + it('returns the same address regardless of what the caller prepends', () => { + const a = resolveClientIp(req({ 'x-forwarded-for': '10.0.0.1, 198.51.100.4' })) + const b = resolveClientIp(req({ 'x-forwarded-for': '10.0.0.2, 198.51.100.4' })) + const c = resolveClientIp(req({ 'x-forwarded-for': 'unknown, 198.51.100.4' })) + expect(new Set([a, b, c])).toEqual(new Set(['198.51.100.4'])) + }) + + it('strips IPv6 zone ids so they cannot mint unbounded distinct keys', () => { + const zoned = ['fe80::1%eth0', 'fe80::1%evil', `fe80::1%${'x'.repeat(200)}`].map((value) => + resolveClientIp(req({ 'x-forwarded-for': value })) + ) + expect(new Set(zoned)).toEqual(new Set(['fe80::1'])) + expect(resolveClientIp(req({ 'x-real-ip': 'fe80::1%evil' }))).toBe('fe80::1') + expect(resolveClientIp(req({ 'x-forwarded-for': '[fe80::1%eth0]:8080' }))).toBe('fe80::1') + }) + + it('collapses equivalent spellings of one address onto a single value', () => { + const forms = ['198.51.100.4', '::ffff:198.51.100.4', '0xc6336404', '198.51.100.4:4444'] + const resolved = forms.map((form) => resolveClientIp(req({ 'x-forwarded-for': form }))) + expect(new Set(resolved)).toEqual(new Set(['198.51.100.4'])) + }) + }) + + describe('trusted proxy chain', () => { + it('skips trusted hops and returns the first untrusted address', () => { + const trusted = parseTrustedProxies('198.51.100.0/24') + expect( + resolveClientIp(req({ 'x-forwarded-for': '203.0.113.7, 198.51.100.4' }), trusted) + ).toBe('203.0.113.7') + }) + + it('accepts bare addresses as single-host trusted ranges', () => { + const trusted = parseTrustedProxies('198.51.100.4, 192.0.2.10') + expect( + resolveClientIp( + req({ 'x-forwarded-for': '203.0.113.7, 192.0.2.10, 198.51.100.4' }), + trusted + ) + ).toBe('203.0.113.7') + }) + + it('stops at the first untrusted hop rather than walking to the leftmost', () => { + const trusted = parseTrustedProxies('198.51.100.4') + expect( + resolveClientIp(req({ 'x-forwarded-for': '10.0.0.1, 203.0.113.7, 198.51.100.4' }), trusted) + ).toBe('203.0.113.7') + }) + + it('falls back to the rightmost hop when the whole chain is trusted', () => { + const trusted = parseTrustedProxies('198.51.100.0/24') + expect( + resolveClientIp(req({ 'x-forwarded-for': '198.51.100.1, 198.51.100.4' }), trusted) + ).toBe('198.51.100.4') + }) + + it('cannot be bypassed by forging a hop from inside a broad trusted range', () => { + // The docs recommend ranges like 10.0.0.0/16. A caller who forges an + // address from inside it makes every hop "trusted"; the resolver must + // still land on the proxy-written hop, not the forged one. + const trusted = parseTrustedProxies('10.0.0.0/16') + const forged = ['10.0.99.99', '10.0.7.7', '10.0.1.2'].map((spoof) => + resolveClientIp(req({ 'x-forwarded-for': `${spoof}, 10.0.0.1` }), trusted) + ) + expect(new Set(forged)).toEqual(new Set(['10.0.0.1'])) + }) + + it('does not treat an IPv6 hop as matching an IPv4 trusted range', () => { + const trusted = parseTrustedProxies('0.0.0.0/0') + expect(resolveClientIp(req({ 'x-forwarded-for': '2001:db8::1' }), trusted)).toBe( + '2001:db8::1' + ) + }) + + it('matches an IPv4-mapped IPv6 trusted range against the unwrapped hop', () => { + const trusted = parseTrustedProxies('::ffff:10.0.0.0/104') + expect( + resolveClientIp(req({ 'x-forwarded-for': '203.0.113.5, ::ffff:10.0.0.1' }), trusted) + ).toBe('203.0.113.5') + }) + + it('matches IPv6 trusted ranges', () => { + const trusted = parseTrustedProxies('2001:db8::/32') + expect(resolveClientIp(req({ 'x-forwarded-for': '203.0.113.7, 2001:db8::1' }), trusted)).toBe( + '203.0.113.7' + ) + }) + }) + + describe('header parsing', () => { + it('handles a single-hop chain', () => { + expect(resolveClientIp(req({ 'x-forwarded-for': '198.51.100.4' }))).toBe('198.51.100.4') + }) + + it('strips brackets and ports from IPv6 hops', () => { + expect(resolveClientIp(req({ 'x-forwarded-for': '[2001:db8::1]:8080' }))).toBe('2001:db8::1') + }) + + it('preserves a bare IPv6 literal', () => { + expect(resolveClientIp(req({ 'x-forwarded-for': '2001:db8::1' }))).toBe('2001:db8::1') + }) + + it('skips unparseable hops while walking right to left', () => { + expect(resolveClientIp(req({ 'x-forwarded-for': '198.51.100.4, _hidden' }))).toBe( + '198.51.100.4' + ) + }) + + it('falls back to x-real-ip when x-forwarded-for holds no address', () => { + expect( + resolveClientIp(req({ 'x-forwarded-for': 'unknown', 'x-real-ip': '198.51.100.4' })) + ).toBe('198.51.100.4') + }) + + it('prefers x-forwarded-for over x-real-ip when both parse', () => { + expect( + resolveClientIp(req({ 'x-forwarded-for': '198.51.100.4', 'x-real-ip': '10.0.0.1' })) + ).toBe('198.51.100.4') + }) + + it('returns the unknown sentinel when no header yields an address', () => { + expect(resolveClientIp(req({}))).toBe(UNKNOWN_CLIENT_IP) + expect(resolveClientIp(req({ 'x-forwarded-for': 'unknown, garbage' }))).toBe( + UNKNOWN_CLIENT_IP + ) + }) + }) +}) + +describe('getAssertedOriginIp', () => { + it('returns the leftmost hop — the sender the delivery claims to be', () => { + expect(getAssertedOriginIp(req({ 'x-forwarded-for': '203.0.113.7, 10.0.0.1' }))).toBe( + '203.0.113.7' + ) + }) + + it('is the opposite end of the chain from the throttling key', () => { + const request = req({ 'x-forwarded-for': '203.0.113.7, 10.0.0.1' }) + expect(getAssertedOriginIp(request)).not.toBe(resolveClientIp(request)) + }) + + it('canonicalizes so an allowlist entry matches any spelling of the address', () => { + expect(getAssertedOriginIp(req({ 'x-forwarded-for': '::ffff:203.0.113.7' }))).toBe( + canonicalizeIp('203.0.113.7') + ) + expect(canonicalizeIp('01.02.03.04')).toBe('1.2.3.4') + expect(canonicalizeIp('2001:0db8:0000:0000:0000:0000:0000:0001')).toBe('2001:db8::1') + }) + + it('skips unparseable leading hops', () => { + expect(getAssertedOriginIp(req({ 'x-forwarded-for': 'unknown, 203.0.113.7' }))).toBe( + '203.0.113.7' + ) + }) + + it('returns null when there is no forwarded chain or no address in it', () => { + expect(getAssertedOriginIp(req({}))).toBeNull() + expect(getAssertedOriginIp(req({ 'x-forwarded-for': 'unknown' }))).toBeNull() + expect(canonicalizeIp('not-an-ip')).toBeNull() + }) +}) + +describe('parseTrustedProxies', () => { + it('treats empty, null, and undefined input as trusting nothing', () => { + expect(parseTrustedProxies('').cidrs).toHaveLength(0) + expect(parseTrustedProxies(null).cidrs).toHaveLength(0) + expect(parseTrustedProxies(undefined).cidrs).toHaveLength(0) + }) + + it('drops malformed entries instead of throwing, keeping the valid ones', () => { + const trusted = parseTrustedProxies('not-an-ip, 10.0.0.0/99, , 198.51.100.4') + expect(trusted.cidrs).toHaveLength(1) + expect(resolveClientIp(req({ 'x-forwarded-for': '203.0.113.7, 198.51.100.4' }), trusted)).toBe( + '203.0.113.7' + ) + }) + + it('ignores surrounding whitespace', () => { + const trusted = parseTrustedProxies(' 198.51.100.0/24 , 192.0.2.10 ') + expect(trusted.cidrs).toHaveLength(2) + }) +}) diff --git a/packages/security/src/client-ip.ts b/packages/security/src/client-ip.ts new file mode 100644 index 00000000000..aad49dc5625 --- /dev/null +++ b/packages/security/src/client-ip.ts @@ -0,0 +1,206 @@ +import * as ipaddr from 'ipaddr.js' + +type ParsedIp = ipaddr.IPv4 | ipaddr.IPv6 +type ParsedCidr = readonly [ParsedIp, number] + +/** Anything with a case-insensitive header getter — `Request`, `NextRequest`, `Headers`. */ +export interface ClientIpHeaderSource { + headers: { get(name: string): string | null } +} + +/** + * A prepared trusted-proxy list. Build it once at module scope with + * {@link parseTrustedProxies} and reuse it — parsing on every request would + * re-tokenize the CIDRs for no benefit. + */ +export interface TrustedProxyList { + readonly cidrs: readonly ParsedCidr[] +} + +/** The default list: no proxy hop is trusted to have vouched for the hop left of it. */ +const NO_TRUSTED_PROXIES: TrustedProxyList = { cidrs: [] } + +/** Returned when no header yields a parseable address. Callers share one bucket for it. */ +export const UNKNOWN_CLIENT_IP = 'unknown' + +/** + * Strips a port and IPv6 brackets from a forwarded-hop token, leaving a bare + * address. Handles `[::1]:8080`, `[::1]`, and `1.2.3.4:5678`; a bare IPv6 + * literal (two or more colons, no brackets) is returned untouched. + */ +function stripPortAndBrackets(value: string): string { + let host = value + if (host.startsWith('[')) { + const end = host.indexOf(']') + host = end === -1 ? host.slice(1) : host.slice(1, end) + } else { + const firstColon = host.indexOf(':') + if (firstColon !== -1 && host.indexOf(':', firstColon + 1) === -1) { + host = host.slice(0, firstColon) + } + } + // Drop any IPv6 zone id (`fe80::1%eth0`). `ipaddr.isValid` accepts an + // arbitrary-length zone and `process()` preserves it verbatim, so keeping it + // would hand a caller an unbounded supply of distinct-but-equivalent strings + // to use as rate-limit keys — and let them write arbitrary text into keys and + // audit rows. The zone is a local interface selector, never client identity. + const zone = host.indexOf('%') + return zone === -1 ? host : host.slice(0, zone) +} + +/** + * Parses one forwarded-hop token into a canonical address, or `null` when it is + * not an IP at all (`unknown`, `_hidden`, an injected junk value). `process()` + * collapses equivalent spellings — IPv4-mapped IPv6, octal and hex IPv4 — onto + * one representation, so a caller cannot multiply rate-limit buckets by varying + * the encoding of a single address. + */ +function parseHop(raw: string): ParsedIp | null { + const value = stripPortAndBrackets(raw.trim()) + if (!value || !ipaddr.isValid(value)) return null + try { + return ipaddr.process(value) + } catch { + return null + } +} + +/** + * Rewrites an IPv4-mapped IPv6 range (`::ffff:10.0.0.0/104`) to its IPv4 form so + * it can match hops, which {@link parseHop} always unwraps to IPv4. Without this + * the kinds never agree and the entry is silently inert. + */ +function normalizeCidr(cidr: ParsedCidr): ParsedCidr { + const [addr, bits] = cidr + if (addr.kind() !== 'ipv6') return cidr + const v6 = addr as ipaddr.IPv6 + if (!v6.isIPv4MappedAddress() || bits < 96) return cidr + return [v6.toIPv4Address(), bits - 96] +} + +function isTrustedProxy(addr: ParsedIp, trustedProxies: TrustedProxyList): boolean { + for (const [range, bits] of trustedProxies.cidrs) { + if (addr.kind() !== range.kind()) continue + if (addr.kind() === 'ipv4') { + if ((addr as ipaddr.IPv4).match(range as ipaddr.IPv4, bits)) return true + } else if ((addr as ipaddr.IPv6).match(range as ipaddr.IPv6, bits)) return true + } + return false +} + +/** + * Parses a comma-separated trusted-proxy setting (`AUTH_TRUSTED_PROXIES`) into + * matchable ranges. Entries may be CIDRs (`10.0.0.0/24`) or bare addresses, + * which become single-host ranges. + * + * Unparseable entries are dropped rather than thrown on: a typo must not take + * the app down, and dropping an entry can only make IP resolution stricter + * (one fewer hop is skipped), never more permissive. + */ +export function parseTrustedProxies(raw: string | null | undefined): TrustedProxyList { + const cidrs: ParsedCidr[] = [] + for (const entry of (raw ?? '').split(',')) { + const value = entry.trim() + if (!value) continue + try { + if (value.includes('/')) { + cidrs.push(normalizeCidr(ipaddr.parseCIDR(value))) + continue + } + const addr = parseHop(value) + if (addr) cidrs.push([addr, addr.kind() === 'ipv4' ? 32 : 128]) + } catch { + // Malformed entry — skip it. + } + } + return { cidrs } +} + +/** + * Resolves the client IP behind a reverse proxy, safely enough to key a rate + * limit on. + * + * `X-Forwarded-For` is a chain that every hop *appends* to, so the **leftmost** + * entry is whatever the original caller sent — fully attacker-controlled — while + * the **rightmost** was written by the proxy directly in front of the app. This + * walks the chain right to left, skips hops that match {@link TrustedProxyList}, + * and returns the first address that is not a trusted proxy. That is the closest + * hop the infrastructure actually vouched for. + * + * With no trusted proxies configured the rightmost entry wins. Behind a longer + * chain (e.g. a CDN in front of an ingress) that collapses callers onto the edge + * addresses and throttles them together — coarse, but it fails closed. Listing + * the real hops in `AUTH_TRUSTED_PROXIES` restores per-client keying. + * + * When *every* entry is trusted the walk falls back to the **rightmost** hop, + * never the leftmost. This matters: operators are told to configure ranges like + * `10.0.0.0/16`, and a caller who forges `X-Forwarded-For: 10.0..` + * from inside that range would otherwise make the whole chain "trusted" and get + * their own forged value back — reinstating the very bucket-per-request bypass + * this function exists to close. The rightmost hop is the one the adjacent proxy + * wrote, so it is the only entry a caller can never author. + * + * `X-Real-IP` is the fallback when `X-Forwarded-For` carries no parseable + * address, and {@link UNKNOWN_CLIENT_IP} when neither header does. + * + * @param request Any object exposing a header getter. + * @param trustedProxies Prepared list from {@link parseTrustedProxies}. + */ +export function resolveClientIp( + request: ClientIpHeaderSource, + trustedProxies: TrustedProxyList = NO_TRUSTED_PROXIES +): string { + const forwarded = request.headers.get('x-forwarded-for') + if (forwarded) { + const hops = forwarded.split(',') + let rightmostParsed: string | null = null + for (let i = hops.length - 1; i >= 0; i--) { + const addr = parseHop(hops[i]) + if (!addr) continue + const canonical = addr.toString() + if (!isTrustedProxy(addr, trustedProxies)) return canonical + rightmostParsed ??= canonical + } + if (rightmostParsed) return rightmostParsed + } + + const realIp = parseHop(request.headers.get('x-real-ip') ?? '') + return realIp ? realIp.toString() : UNKNOWN_CLIENT_IP +} + +/** + * The **leftmost** `X-Forwarded-For` hop — the origin address *asserted* by the + * caller — in canonical form, or `null` when none parses. + * + * Deliberately the opposite end of the chain from {@link resolveClientIp}, and + * usable for exactly one thing: comparing against an operator-configured + * allowlist of expected senders, where the question is "which address does this + * delivery claim to come from" rather than "who do I throttle". + * + * **Never key a rate limit, quota, or lockout on this.** Under any proxy that + * appends to `X-Forwarded-For` the value is caller-controlled and can be rotated + * per request. An allowlist built on it is a filter against honest senders, not + * an authentication mechanism — pair it with a shared secret or signature. + */ +export function getAssertedOriginIp(request: ClientIpHeaderSource): string | null { + const forwarded = request.headers.get('x-forwarded-for') + if (!forwarded) return null + for (const hop of forwarded.split(',')) { + const addr = parseHop(hop) + if (addr) return addr.toString() + } + return null +} + +/** + * Canonicalizes an operator-supplied address so it can be compared against + * {@link getAssertedOriginIp}. Returns `null` when the entry is not an IP. + * + * Needed because both sides must agree on spelling: `::ffff:1.2.3.4`, + * `01.02.03.04`, and `1.2.3.4` are one address, and a config entry typed in a + * non-canonical form would otherwise never match. + */ +export function canonicalizeIp(value: string): string | null { + const addr = parseHop(value) + return addr ? addr.toString() : null +} diff --git a/packages/testing/src/mocks/index.ts b/packages/testing/src/mocks/index.ts index 7ef622c4d61..b4fc53e5b1e 100644 --- a/packages/testing/src/mocks/index.ts +++ b/packages/testing/src/mocks/index.ts @@ -134,6 +134,7 @@ export { } from './redis-config.mock' // Request mocks export { + clientIpMock, createMockFormDataRequest, createMockRequest, requestUtilsMock, diff --git a/packages/testing/src/mocks/request.mock.ts b/packages/testing/src/mocks/request.mock.ts index 614366ad938..1298b3e1527 100644 --- a/packages/testing/src/mocks/request.mock.ts +++ b/packages/testing/src/mocks/request.mock.ts @@ -67,7 +67,8 @@ export function createMockFormDataRequest( } /** - * Controllable mock functions for `@/lib/core/utils/request`. + * Controllable mock functions for `@/lib/core/utils/request` and + * `@/lib/core/utils/client-ip`. * * @example * ```ts @@ -92,6 +93,19 @@ export const requestUtilsMockFns = { */ export const requestUtilsMock = { generateRequestId: requestUtilsMockFns.mockGenerateRequestId, - getClientIp: requestUtilsMockFns.mockGetClientIp, noop: () => {}, } + +/** + * Static mock module for `@/lib/core/utils/client-ip`. Separate from + * {@link requestUtilsMock} because the real module pulls `ipaddr.js` and the + * app env, which is exactly why it is not part of `utils/request`. + * + * @example + * ```ts + * vi.mock('@/lib/core/utils/client-ip', () => clientIpMock) + * ``` + */ +export const clientIpMock = { + getClientIp: requestUtilsMockFns.mockGetClientIp, +} From ff6f5dbe7381cc1340b46a10cfa707f17e3c2900 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 12:35:23 -0700 Subject: [PATCH 2/8] fix(security): restore the X-Real-IP fallback for webhook allowlists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getAssertedOriginIp only read X-Forwarded-For, but the helper it replaced also accepted X-Real-IP. A proxy that sets only X-Real-IP left the allowlist with no address at all, so every permitted delivery 403'd. Fall back to X-Real-IP when the forwarded chain yields nothing. It is the same question the chain answers — which address does this delivery claim to come from — and with no chain present it is the only record of the sender. --- packages/security/src/client-ip.test.ts | 15 ++++++++++++++- packages/security/src/client-ip.ts | 13 ++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/security/src/client-ip.test.ts b/packages/security/src/client-ip.test.ts index 12d53e84523..17ea5fab234 100644 --- a/packages/security/src/client-ip.test.ts +++ b/packages/security/src/client-ip.test.ts @@ -173,7 +173,20 @@ describe('getAssertedOriginIp', () => { ) }) - it('returns null when there is no forwarded chain or no address in it', () => { + it('falls back to x-real-ip for proxies that set it instead of a chain', () => { + expect(getAssertedOriginIp(req({ 'x-real-ip': '203.0.113.7' }))).toBe('203.0.113.7') + expect( + getAssertedOriginIp(req({ 'x-forwarded-for': 'unknown', 'x-real-ip': '203.0.113.7' })) + ).toBe('203.0.113.7') + }) + + it('prefers the forwarded chain over x-real-ip when both parse', () => { + expect( + getAssertedOriginIp(req({ 'x-forwarded-for': '203.0.113.7', 'x-real-ip': '10.0.0.1' })) + ).toBe('203.0.113.7') + }) + + it('returns null when no header yields an address', () => { expect(getAssertedOriginIp(req({}))).toBeNull() expect(getAssertedOriginIp(req({ 'x-forwarded-for': 'unknown' }))).toBeNull() expect(canonicalizeIp('not-an-ip')).toBeNull() diff --git a/packages/security/src/client-ip.ts b/packages/security/src/client-ip.ts index aad49dc5625..170c81ee88b 100644 --- a/packages/security/src/client-ip.ts +++ b/packages/security/src/client-ip.ts @@ -170,12 +170,15 @@ export function resolveClientIp( /** * The **leftmost** `X-Forwarded-For` hop — the origin address *asserted* by the - * caller — in canonical form, or `null` when none parses. + * caller — in canonical form, falling back to `X-Real-IP`, or `null` when + * neither header yields an address. * * Deliberately the opposite end of the chain from {@link resolveClientIp}, and * usable for exactly one thing: comparing against an operator-configured * allowlist of expected senders, where the question is "which address does this - * delivery claim to come from" rather than "who do I throttle". + * delivery claim to come from" rather than "who do I throttle". The `X-Real-IP` + * fallback matters because a proxy may set it *instead of* a forwarded chain, + * and it is then the only record of the sender. * * **Never key a rate limit, quota, or lockout on this.** Under any proxy that * appends to `X-Forwarded-For` the value is caller-controlled and can be rotated @@ -184,12 +187,12 @@ export function resolveClientIp( */ export function getAssertedOriginIp(request: ClientIpHeaderSource): string | null { const forwarded = request.headers.get('x-forwarded-for') - if (!forwarded) return null - for (const hop of forwarded.split(',')) { + for (const hop of forwarded?.split(',') ?? []) { const addr = parseHop(hop) if (addr) return addr.toString() } - return null + const realIp = parseHop(request.headers.get('x-real-ip') ?? '') + return realIp ? realIp.toString() : null } /** From f922fde718f78aea54c6e70dd97c093261402475 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 12:42:23 -0700 Subject: [PATCH 3/8] chore(helm): bump chart to 1.4.1 for the AUTH_TRUSTED_PROXIES doc update --- helm/sim/Chart.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helm/sim/Chart.yaml b/helm/sim/Chart.yaml index d167b0cad4e..5f6ee8ff61e 100644 --- a/helm/sim/Chart.yaml +++ b/helm/sim/Chart.yaml @@ -2,7 +2,7 @@ apiVersion: v2 name: sim description: A Helm chart for Sim - the open-source AI workspace where teams build, deploy, and manage AI agents type: application -version: 1.4.0 +version: 1.4.1 appVersion: "v0.7.44" kubeVersion: ">=1.25.0-0" home: https://sim.ai From 19f536d5d975067fcef7549af36fb988d10079a4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:51:12 -0700 Subject: [PATCH 4/8] fix(security): mask IPv6 rate-limit keys to the routed /64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single IPv6 client is delegated a whole /64 — the standard residential and cloud allocation — so it can legitimately source every request from a different address. Keying a per-IP throttle on the full /128 therefore left the exact bypass this fix exists to close wide open over IPv6, with no header spoofing at all: the proxy itself writes the varying value and nothing looks wrong. Mask IPv6 to its routed prefix when producing a key, so one subscriber is one bucket. Matches Better Auth's `ipv6Subnet` default, so session and throttle keys agree. IPv4 is untouched — a v4 address is already a single host. Masking happens only where a key is produced, never before the trusted-proxy comparison, which must see the full address. `getAssertedOriginIp` stays unmasked: the webhook allowlist needs the exact sender. Also corrects what the docs claim about Better Auth. With no trusted proxies configured it does not walk the chain — `getIPFromHeader` returns null for any multi-value header — so the previous wording (and the env.ts line this replaces, which had been accurate) overstated the agreement between the two. Each surface now states where they align and where they deliberately differ, warns against a trusted range broad enough to cover clients, and notes that none of it helps an app exposed without a proxy. - values.schema.json carried the same stale claim as values.yaml - profound.ts compares against UNKNOWN_CLIENT_IP instead of a bare literal - cover the env -> parseTrustedProxies wiring, which was globally mocked and so never executed in CI, and de-vacuum the IPv6/IPv4 kind-mismatch test --- apps/docs/app/api/chat/route.ts | 8 ++- apps/sim/.env.example | 2 +- apps/sim/lib/analytics/profound.ts | 3 +- apps/sim/lib/core/config/env.ts | 2 +- apps/sim/lib/core/utils/client-ip.test.ts | 67 ++++++++++++++++++++ apps/sim/lib/core/utils/client-ip.ts | 11 +++- docker-compose.prod.yml | 23 ++++--- helm/sim/values.schema.json | 2 +- helm/sim/values.yaml | 17 +++--- packages/security/src/client-ip.test.ts | 74 ++++++++++++++++++++--- packages/security/src/client-ip.ts | 53 +++++++++++++--- 11 files changed, 221 insertions(+), 41 deletions(-) create mode 100644 apps/sim/lib/core/utils/client-ip.test.ts diff --git a/apps/docs/app/api/chat/route.ts b/apps/docs/app/api/chat/route.ts index 76f7c40f6ec..d247afee499 100644 --- a/apps/docs/app/api/chat/route.ts +++ b/apps/docs/app/api/chat/route.ts @@ -71,8 +71,12 @@ const RATE_LIMIT_WINDOW_MS = 60_000 const rateLimitHits = new Map() /** - * Reverse-proxy hops trusted for forwarded-IP resolution — the same - * `AUTH_TRUSTED_PROXIES` the main app reads. Parsed once at module load. + * Reverse-proxy hops trusted for forwarded-IP resolution, named after the main + * app's setting so the two behave alike where both are deployed. The docs site + * ships separately and does not normally set it, so this is usually empty — + * which is safe (the rightmost, proxy-written hop wins) but coarse: if the docs + * edge presents more than one hop, visitors share one bucket. Set it here too if + * that shows up as spurious 429s. */ const trustedProxies = parseTrustedProxies(process.env.AUTH_TRUSTED_PROXIES) diff --git a/apps/sim/.env.example b/apps/sim/.env.example index 7209da4e6e3..7c4e6e7e9bb 100644 --- a/apps/sim/.env.example +++ b/apps/sim/.env.example @@ -19,7 +19,7 @@ BETTER_AUTH_URL=http://localhost:3000 NEXT_PUBLIC_APP_URL=http://localhost:3000 # INTERNAL_API_BASE_URL=http://sim-app.default.svc.cluster.local:3000 # Optional: internal URL for server-side /api self-calls; defaults to NEXT_PUBLIC_APP_URL # TRUSTED_ORIGINS=https://www.example.com,https://app.example.com # Optional: comma-separated additional public origins to trust for auth (apex+www, alias domains). Merged into Better Auth trustedOrigins. -# AUTH_TRUSTED_PROXIES=10.0.0.0/24,192.0.2.10 # Optional: reverse-proxy IPs/CIDRs in front of the app. Better Auth and Sim's own per-IP throttles walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP (the leftmost entry is caller-supplied and would otherwise let anyone mint a fresh rate-limit bucket per request). Unset trusts no hop and keys on the rightmost entry — safe, but a multi-hop chain collapses callers onto the edge addresses. Use your proxies' actual addresses, not broad private ranges that also cover clients. +# AUTH_TRUSTED_PROXIES=10.0.0.0/24,192.0.2.10 # Optional: reverse-proxy IPs/CIDRs in front of the app. When set, Better Auth and Sim's own per-IP throttles both walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP (the leftmost entry is caller-supplied and would otherwise let anyone mint a fresh rate-limit bucket per request). Unset, the two differ: Better Auth trusts only single-value headers, while Sim's throttles key on the rightmost, proxy-written entry — never spoofable, but a multi-hop chain collapses callers onto the edge addresses. Use your proxies' actual addresses, NOT broad private ranges that also cover clients: a caller whose own address falls inside a trusted range makes the whole chain trusted. # Chat (Optional) # COPILOT_API_KEY= # Mint one at https://sim.ai. Without it the Sim Chat block, prompt jobs, and Inbox cannot run diff --git a/apps/sim/lib/analytics/profound.ts b/apps/sim/lib/analytics/profound.ts index 2a254d1265a..7c9507fbe7a 100644 --- a/apps/sim/lib/analytics/profound.ts +++ b/apps/sim/lib/analytics/profound.ts @@ -6,6 +6,7 @@ * @see https://docs.tryprofound.com/agent-analytics/custom */ import { createLogger } from '@sim/logger' +import { UNKNOWN_CLIENT_IP } from '@sim/security/client-ip' import { env } from '@/lib/core/config/env' import { isHosted } from '@/lib/core/config/env-flags' import { getClientIp } from '@/lib/core/utils/client-ip' @@ -104,7 +105,7 @@ export function sendToProfound(request: Request, statusCode: number): void { status_code: statusCode, ip: (() => { const resolved = getClientIp(request) - return resolved === 'unknown' ? '0.0.0.0' : resolved + return resolved === UNKNOWN_CLIENT_IP ? '0.0.0.0' : resolved })(), user_agent: request.headers.get('user-agent') || '', ...(Object.keys(queryParams).length > 0 && { query_params: queryParams }), diff --git a/apps/sim/lib/core/config/env.ts b/apps/sim/lib/core/config/env.ts index efca1f5f53f..2bca035477e 100644 --- a/apps/sim/lib/core/config/env.ts +++ b/apps/sim/lib/core/config/env.ts @@ -507,7 +507,7 @@ 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. Better Auth and getClientIp (per-IP rate-limit keys, audit rows) walk the forwarded-IP chain right to left, skip these trusted hops, and use the first untrusted address as the client IP. Unset trusts no hop and keys on the rightmost, proxy-written entry — never the caller-supplied leftmost one. + AUTH_TRUSTED_PROXIES: z.string().optional(), // Comma-separated reverse-proxy IPs or CIDR ranges. When set, Better Auth and getClientIp (per-IP rate-limit keys, audit rows) both walk the forwarded-IP chain right to left, skip these trusted hops, and use the first untrusted address as the client IP. Leave unset and the two differ: Better Auth trusts only single-value IP headers (recording no IP for a multi-hop chain), while getClientIp keys on the rightmost, proxy-written entry — never the caller-supplied leftmost one. // SSO Configuration (for script-based registration) SSO_ENABLED: z.boolean().optional(), // Enable SSO functionality diff --git a/apps/sim/lib/core/utils/client-ip.test.ts b/apps/sim/lib/core/utils/client-ip.test.ts new file mode 100644 index 00000000000..a4ea497d11c --- /dev/null +++ b/apps/sim/lib/core/utils/client-ip.test.ts @@ -0,0 +1,67 @@ +/** + * Covers the wiring between `AUTH_TRUSTED_PROXIES` and the shared resolver. + * `@/lib/core/utils/client-ip` is mocked globally in `vitest.setup.ts` (it is + * what every route consumes), so without the `vi.unmock` below the real module + * — and therefore the env read that makes trusted proxies take effect — would + * never execute anywhere in CI. + * + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockEnv } = vi.hoisted(() => ({ + mockEnv: { AUTH_TRUSTED_PROXIES: undefined as string | undefined }, +})) + +vi.mock('@/lib/core/config/env', () => ({ env: mockEnv })) +vi.unmock('@/lib/core/utils/client-ip') + +/** + * The module parses the env once at import — that is the behavior under test — + * so each case needs a fresh module instance. This is the deliberate exception + * to the repo's "no `vi.resetModules()` + dynamic import" performance rule + * (`.cursor/rules/sim-testing.mdc`): module-init behavior cannot be observed any + * other way, and the cost here is four imports of a six-line module. + */ +async function loadGetClientIp(trustedProxies: string | undefined) { + mockEnv.AUTH_TRUSTED_PROXIES = trustedProxies + vi.resetModules() + return (await import('@/lib/core/utils/client-ip')).getClientIp +} + +function req(headers: Record) { + return { headers: new Headers(headers) } +} + +describe('getClientIp', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('keys on the proxy-appended hop, not the caller-supplied leftmost one', async () => { + const getClientIp = await loadGetClientIp(undefined) + + expect(getClientIp(req({ 'x-forwarded-for': '203.0.113.7, 10.0.0.1' }))).toBe('10.0.0.1') + }) + + it('honors AUTH_TRUSTED_PROXIES, resolving past the configured hop', async () => { + const getClientIp = await loadGetClientIp('10.0.0.0/24') + + expect(getClientIp(req({ 'x-forwarded-for': '203.0.113.7, 10.0.0.1' }))).toBe('203.0.113.7') + }) + + it('gives one bucket per caller no matter what they prepend', async () => { + const getClientIp = await loadGetClientIp(undefined) + const keys = ['9.9.9.9', 'unknown', '203.0.113.250'].map((spoof) => + getClientIp(req({ 'x-forwarded-for': `${spoof}, 10.0.0.1` })) + ) + + expect(new Set(keys)).toEqual(new Set(['10.0.0.1'])) + }) + + it('falls back to a shared bucket when no header yields an address', async () => { + const getClientIp = await loadGetClientIp(undefined) + + expect(getClientIp(req({}))).toBe('unknown') + }) +}) diff --git a/apps/sim/lib/core/utils/client-ip.ts b/apps/sim/lib/core/utils/client-ip.ts index a03c98486d5..118d7e7fe56 100644 --- a/apps/sim/lib/core/utils/client-ip.ts +++ b/apps/sim/lib/core/utils/client-ip.ts @@ -6,9 +6,14 @@ import { import { env } from '@/lib/core/config/env' /** - * Reverse-proxy hops trusted for forwarded-IP resolution, shared with Better - * Auth's `advanced.ipAddress.trustedProxies` so session IPs and Sim's own - * rate-limit keys agree on who the caller is. Parsed once at module load. + * Reverse-proxy hops trusted for forwarded-IP resolution, read from the same + * `AUTH_TRUSTED_PROXIES` as Better Auth's `advanced.ipAddress.trustedProxies` + * (see `lib/auth/auth.ts`). Parsed once at module load. + * + * Configured, the two agree on who the caller is. Left unset they diverge by + * design: Better Auth trusts only a single-value header and records no IP for a + * longer chain, whereas a throttle cannot opt out of having a key, so this falls + * back to the rightmost — still proxy-written, never caller-authored. */ const trustedProxies = parseTrustedProxies(env.AUTH_TRUSTED_PROXIES) diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index b796472d8da..ca62be1d659 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -21,15 +21,20 @@ 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 AND Sim's own - # per-IP throttles walk x-forwarded-for right to left, skip these hops, and - # use the first untrusted address as the client IP — never the leftmost - # entry, which the caller supplies and could otherwise rotate to mint a - # fresh rate-limit bucket per request. Empty by default, which trusts no - # hop and keys on the rightmost (proxy-written) entry: safe, but behind a - # multi-hop chain (e.g. CDN in front of ingress) it collapses callers onto - # the edge addresses. Set it to your real hops for per-client keying and - # correct session IPs. + # front of the app (ingress, load balancer). When set, Better Auth AND + # Sim's own per-IP throttles walk x-forwarded-for right to left, skip these + # hops, and use the first untrusted address as the client IP — never the + # leftmost entry, which the caller supplies and could otherwise be rotated + # to mint a fresh rate-limit bucket per request. Empty by default, and then + # the two differ: Better Auth trusts only a single-value header (recording + # no IP for a multi-hop chain), while Sim's throttles key on the rightmost + # (proxy-written) entry — not spoofable, but behind a multi-hop chain + # (e.g. CDN in front of ingress) it collapses callers onto the edge + # addresses. Set your real hops for per-client keying and correct session + # IPs. Use the proxies' actual addresses, NOT a broad private range that + # also covers clients: a caller inside a trusted range makes the whole + # chain trusted. This all assumes a proxy that appends the peer address — + # an app exposed directly to the internet sees only what the caller wrote. - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-} - BETTER_AUTH_SECRET=${BETTER_AUTH_SECRET} - ENCRYPTION_KEY=${ENCRYPTION_KEY} diff --git a/helm/sim/values.schema.json b/helm/sim/values.schema.json index 9088f9bd6f1..7a7f7e71b23 100644 --- a/helm/sim/values.schema.json +++ b/helm/sim/values.schema.json @@ -157,7 +157,7 @@ }, "AUTH_TRUSTED_PROXIES": { "type": "string", - "description": "Comma-separated reverse-proxy IPs or CIDR ranges in front of the app (e.g. '10.0.0.0/16'). Better Auth walks x-forwarded-for right to left, skips these hops, and uses the first untrusted address as the client IP." + "description": "Comma-separated reverse-proxy IPs or CIDR ranges in front of the app (e.g. the ingress pods, '10.42.0.0/24'). When set, Better Auth and Sim's per-IP rate limits both walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP. Leave empty and the two differ: Better Auth trusts only a single-value header, while Sim's throttles key on the rightmost, proxy-written entry. Do not use a range broad enough to also cover client traffic — a caller inside a trusted range makes the whole chain trusted." }, "SSO_TRUSTED_PROVIDER_IDS": { "type": "string", diff --git a/helm/sim/values.yaml b/helm/sim/values.yaml index 803fa7c10d2..1a29d3bdfec 100644 --- a/helm/sim/values.yaml +++ b/helm/sim/values.yaml @@ -80,13 +80,16 @@ app: # Merged into Better Auth `trustedOrigins` alongside NEXT_PUBLIC_APP_URL. Leave empty when serving from a single origin. TRUSTED_ORIGINS: "" # AUTH_TRUSTED_PROXIES: comma-separated reverse-proxy IPs or CIDR ranges in front of the app - # (ingress controller, load balancer). Better Auth AND Sim's own per-IP throttles walk - # x-forwarded-for right to left, skip these hops, and use the first untrusted address as the - # client IP — never the leftmost entry, which the caller supplies and could otherwise be - # rotated to mint a fresh rate-limit bucket per request. Empty trusts no hop and keys on the - # rightmost (proxy-written) entry: safe, but behind a multi-hop chain (e.g. CDN in front of - # ingress) it collapses callers onto the edge addresses. Set your real hops (e.g. "10.0.0.0/16") - # for per-client keying and correct session IPs. + # (ingress controller, load balancer). When set, Better Auth AND Sim's own per-IP throttles + # walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as + # the client IP — never the leftmost entry, which the caller supplies and could otherwise be + # rotated to mint a fresh rate-limit bucket per request. Empty, the two differ: Better Auth + # trusts only a single-value header (recording no IP for a multi-hop chain), while Sim's + # throttles key on the rightmost (proxy-written) entry — not spoofable, but behind a multi-hop + # chain (e.g. CDN in front of ingress) it collapses callers onto the edge addresses. + # Set the ingress pods' actual addresses (e.g. "10.42.0.0/24"). Do NOT use a range broad enough + # to also cover client traffic: a caller whose own address falls inside a trusted range makes + # the whole chain trusted and can then forge the value Sim keys on. AUTH_TRUSTED_PROXIES: "" # SOCKET_SERVER_URL: Auto-detected when realtime.enabled=true (uses internal service) # NEXT_PUBLIC_SOCKET_URL: public WebSocket URL for browsers. Leave empty to default to the diff --git a/packages/security/src/client-ip.test.ts b/packages/security/src/client-ip.test.ts index 17ea5fab234..97dd36d8f6c 100644 --- a/packages/security/src/client-ip.test.ts +++ b/packages/security/src/client-ip.test.ts @@ -27,12 +27,51 @@ describe('resolveClientIp', () => { }) it('strips IPv6 zone ids so they cannot mint unbounded distinct keys', () => { + // `ipaddr.isValid` accepts an arbitrary-length zone and `process()` keeps + // it verbatim, so an unstripped zone would be attacker-chosen text in the + // key. The /64 mask happens to drop zones from v6 keys too — the + // getAssertedOriginIp cases below pin the stripping on its own, since + // that path is deliberately unmasked. const zoned = ['fe80::1%eth0', 'fe80::1%evil', `fe80::1%${'x'.repeat(200)}`].map((value) => resolveClientIp(req({ 'x-forwarded-for': value })) ) - expect(new Set(zoned)).toEqual(new Set(['fe80::1'])) - expect(resolveClientIp(req({ 'x-real-ip': 'fe80::1%evil' }))).toBe('fe80::1') - expect(resolveClientIp(req({ 'x-forwarded-for': '[fe80::1%eth0]:8080' }))).toBe('fe80::1') + expect(new Set(zoned)).toEqual(new Set(['fe80::'])) + expect(resolveClientIp(req({ 'x-real-ip': 'fe80::1%evil' }))).toBe('fe80::') + expect(canonicalizeIp('fe80::1%evil')).toBe('fe80::1') + }) + + it('masks IPv6 to its routed /64 so one subscriber is one bucket', () => { + // A single IPv6 client is delegated a whole /64, so the proxy honestly + // writes a different address per request. Keying on the full /128 would + // leave per-IP throttles bypassable with no spoofing at all. + const withinOnePrefix = [ + '2001:db8:1:2::1', + '2001:db8:1:2::dead:beef', + '2001:db8:1:2:ffff:ffff:ffff:ffff', + ].map((value) => resolveClientIp(req({ 'x-forwarded-for': value }))) + expect(new Set(withinOnePrefix)).toEqual(new Set(['2001:db8:1:2::'])) + }) + + it('keeps distinct IPv6 /64s in distinct buckets', () => { + const a = resolveClientIp(req({ 'x-forwarded-for': '2001:db8:1:2::1' })) + const b = resolveClientIp(req({ 'x-forwarded-for': '2001:db8:1:3::1' })) + expect(a).not.toBe(b) + }) + + it('does not mask IPv4, which is already a single host', () => { + expect(resolveClientIp(req({ 'x-forwarded-for': '198.51.100.4' }))).toBe('198.51.100.4') + }) + + it('masks the x-real-ip fallback too', () => { + expect(resolveClientIp(req({ 'x-real-ip': '2001:db8:1:2::99' }))).toBe('2001:db8:1:2::') + }) + + it('matches a trusted range against the full address, not the masked key', () => { + // Masking before the trust check would compare a different address. + const trusted = parseTrustedProxies('2001:db8:1:2::abcd/128') + expect( + resolveClientIp(req({ 'x-forwarded-for': '203.0.113.7, 2001:db8:1:2::abcd' }), trusted) + ).toBe('203.0.113.7') }) it('collapses equivalent spellings of one address onto a single value', () => { @@ -86,10 +125,12 @@ describe('resolveClientIp', () => { }) it('does not treat an IPv6 hop as matching an IPv4 trusted range', () => { + // Two hops, so a wrongly-trusted rightmost entry would visibly shift the + // answer left rather than merely avoiding a kind-mismatch throw. const trusted = parseTrustedProxies('0.0.0.0/0') - expect(resolveClientIp(req({ 'x-forwarded-for': '2001:db8::1' }), trusted)).toBe( - '2001:db8::1' - ) + expect( + resolveClientIp(req({ 'x-forwarded-for': '2001:db8:1::1, 2001:db8:2::2' }), trusted) + ).toBe('2001:db8:2::') }) it('matches an IPv4-mapped IPv6 trusted range against the unwrapped hop', () => { @@ -113,11 +154,13 @@ describe('resolveClientIp', () => { }) it('strips brackets and ports from IPv6 hops', () => { - expect(resolveClientIp(req({ 'x-forwarded-for': '[2001:db8::1]:8080' }))).toBe('2001:db8::1') + // Masked to /64 like every IPv6 key; unmasked forms are covered by + // getAssertedOriginIp below. + expect(resolveClientIp(req({ 'x-forwarded-for': '[2001:db8::1]:8080' }))).toBe('2001:db8::') }) - it('preserves a bare IPv6 literal', () => { - expect(resolveClientIp(req({ 'x-forwarded-for': '2001:db8::1' }))).toBe('2001:db8::1') + it('accepts a bare IPv6 literal', () => { + expect(resolveClientIp(req({ 'x-forwarded-for': '2001:db8::1' }))).toBe('2001:db8::') }) it('skips unparseable hops while walking right to left', () => { @@ -186,6 +229,19 @@ describe('getAssertedOriginIp', () => { ).toBe('203.0.113.7') }) + it('does not mask IPv6 — an allowlist needs the exact address', () => { + expect(getAssertedOriginIp(req({ 'x-forwarded-for': '2001:db8:1:2::99' }))).toBe( + '2001:db8:1:2::99' + ) + }) + + it('strips brackets, ports, and zone ids like the resolver does', () => { + expect(getAssertedOriginIp(req({ 'x-forwarded-for': '[2001:db8::1%eth0]:8080' }))).toBe( + '2001:db8::1' + ) + expect(getAssertedOriginIp(req({ 'x-forwarded-for': '203.0.113.7:4444' }))).toBe('203.0.113.7') + }) + it('returns null when no header yields an address', () => { expect(getAssertedOriginIp(req({}))).toBeNull() expect(getAssertedOriginIp(req({ 'x-forwarded-for': 'unknown' }))).toBeNull() diff --git a/packages/security/src/client-ip.ts b/packages/security/src/client-ip.ts index 170c81ee88b..50bdc0a420c 100644 --- a/packages/security/src/client-ip.ts +++ b/packages/security/src/client-ip.ts @@ -24,9 +24,25 @@ const NO_TRUSTED_PROXIES: TrustedProxyList = { cidrs: [] } export const UNKNOWN_CLIENT_IP = 'unknown' /** - * Strips a port and IPv6 brackets from a forwarded-hop token, leaving a bare - * address. Handles `[::1]:8080`, `[::1]`, and `1.2.3.4:5678`; a bare IPv6 - * literal (two or more colons, no brackets) is returned untouched. + * Prefix an IPv6 address is masked to before it becomes a rate-limit key. + * + * A single IPv6 client is routinely delegated a whole /64 — that is the standard + * residential and cloud allocation — so every request can legitimately carry a + * different source address with no spoofing whatsoever. Keying on the full /128 + * would therefore leave per-IP throttles just as bypassable over IPv6 as the + * forwarded-header bug this module exists to close, except the proxy itself + * writes the varying value and nothing looks wrong. + * + * Masking to the routed prefix makes one subscriber one bucket. Matches Better + * Auth's `ipv6Subnet` default, so session and throttle keys agree. + */ +const IPV6_KEY_PREFIX_BITS = 64 + +/** + * Reduces a forwarded-hop token to a bare address, dropping brackets, a port, + * and any IPv6 zone id — `[::1]:8080`, `[::1]`, `1.2.3.4:5678`, `fe80::1%eth0`. + * A bare IPv6 literal (two or more colons, no brackets) keeps its colons; the + * single-colon rule only strips a port from `host:port`. */ function stripPortAndBrackets(value: string): string { let host = value @@ -78,6 +94,22 @@ function normalizeCidr(cidr: ParsedCidr): ParsedCidr { return [v6.toIPv4Address(), bits - 96] } +/** + * Renders an address as a rate-limit key, masking IPv6 to + * {@link IPV6_KEY_PREFIX_BITS} so one delegated prefix is one bucket. IPv4 is + * returned exactly — a v4 address is a single host. + * + * Applied only at the point a key is produced, never before + * {@link isTrustedProxy}: a masked address would compare against trusted ranges + * as a different (and wrong) address. + */ +function toKey(addr: ParsedIp): string { + if (addr.kind() !== 'ipv6') return addr.toString() + const bytes = (addr as ipaddr.IPv6).toByteArray() + for (let i = IPV6_KEY_PREFIX_BITS / 8; i < bytes.length; i++) bytes[i] = 0 + return ipaddr.fromByteArray(bytes).toString() +} + function isTrustedProxy(addr: ParsedIp, trustedProxies: TrustedProxyList): boolean { for (const [range, bits] of trustedProxies.cidrs) { if (addr.kind() !== range.kind()) continue @@ -143,6 +175,14 @@ export function parseTrustedProxies(raw: string | null | undefined): TrustedProx * `X-Real-IP` is the fallback when `X-Forwarded-For` carries no parseable * address, and {@link UNKNOWN_CLIENT_IP} when neither header does. * + * The result is a **bucket key, not an address**: IPv6 is masked to + * {@link IPV6_KEY_PREFIX_BITS} (see there for why a /128 key is bypassable). + * Use {@link getAssertedOriginIp} when an exact address is required. + * + * None of this helps if no proxy sets the header at all — a directly-exposed + * app sees only what the caller wrote, and no parsing rule can recover from + * that. Terminate at a proxy that appends the peer address. + * * @param request Any object exposing a header getter. * @param trustedProxies Prepared list from {@link parseTrustedProxies}. */ @@ -157,15 +197,14 @@ export function resolveClientIp( for (let i = hops.length - 1; i >= 0; i--) { const addr = parseHop(hops[i]) if (!addr) continue - const canonical = addr.toString() - if (!isTrustedProxy(addr, trustedProxies)) return canonical - rightmostParsed ??= canonical + if (!isTrustedProxy(addr, trustedProxies)) return toKey(addr) + rightmostParsed ??= toKey(addr) } if (rightmostParsed) return rightmostParsed } const realIp = parseHop(request.headers.get('x-real-ip') ?? '') - return realIp ? realIp.toString() : UNKNOWN_CLIENT_IP + return realIp ? toKey(realIp) : UNKNOWN_CLIENT_IP } /** From 0870d171517bd5e6e442d91cccdd075497fe57ba Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:07:52 -0700 Subject: [PATCH 5/8] fix(security): stop believing forwarded headers when nothing appends them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Walking the chain right to left only means anything if a proxy wrote part of it. docker-compose.prod.yml publishes port 3000 directly and ships no reverse proxy, so on that reference deployment the whole header is caller-authored and every per-IP limit stayed bypassable no matter which hop we read. No parsing rule can recover a real address from a header nobody vouched for, so make it explicit: TRUST_PROXY_HEADERS declares whether a proxy is in front. False, getClientIp reports 'unknown' and per-IP limits collapse into one shared bucket — blunt, and it throttles unrelated callers together, but it fails closed instead of handing out a fresh bucket per request. Defaults to true, preserving behavior for the ingress-fronted chart and hosted deployments. docker-compose.prod.yml defaults it to false, because that file knows it has no proxy; operators flip it when they put one in front. The audit package mirrors the flag: recording a caller-authored address as forensic evidence is worse than recording none. --- apps/sim/.env.example | 1 + apps/sim/lib/core/config/env.ts | 1 + apps/sim/lib/core/utils/client-ip.test.ts | 38 ++++++++++++++++++++--- apps/sim/lib/core/utils/client-ip.ts | 20 +++++++++++- docker-compose.prod.yml | 10 ++++++ helm/sim/values.schema.json | 4 +++ helm/sim/values.yaml | 7 +++++ packages/audit/src/log.ts | 11 +++++++ 8 files changed, 87 insertions(+), 5 deletions(-) diff --git a/apps/sim/.env.example b/apps/sim/.env.example index 7c4e6e7e9bb..e503a83202a 100644 --- a/apps/sim/.env.example +++ b/apps/sim/.env.example @@ -20,6 +20,7 @@ NEXT_PUBLIC_APP_URL=http://localhost:3000 # INTERNAL_API_BASE_URL=http://sim-app.default.svc.cluster.local:3000 # Optional: internal URL for server-side /api self-calls; defaults to NEXT_PUBLIC_APP_URL # TRUSTED_ORIGINS=https://www.example.com,https://app.example.com # Optional: comma-separated additional public origins to trust for auth (apex+www, alias domains). Merged into Better Auth trustedOrigins. # AUTH_TRUSTED_PROXIES=10.0.0.0/24,192.0.2.10 # Optional: reverse-proxy IPs/CIDRs in front of the app. When set, Better Auth and Sim's own per-IP throttles both walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP (the leftmost entry is caller-supplied and would otherwise let anyone mint a fresh rate-limit bucket per request). Unset, the two differ: Better Auth trusts only single-value headers, while Sim's throttles key on the rightmost, proxy-written entry — never spoofable, but a multi-hop chain collapses callers onto the edge addresses. Use your proxies' actual addresses, NOT broad private ranges that also cover clients: a caller whose own address falls inside a trusted range makes the whole chain trusted. +# TRUST_PROXY_HEADERS=false # Optional: set false when the app is exposed directly with NO reverse proxy in front. With nothing appending the peer address, x-forwarded-for/x-real-ip are written entirely by the caller, so believing them lets anyone rotate a header for a fresh per-IP rate-limit bucket per request. While false, getClientIp reports 'unknown' and per-IP limits become one shared bucket (blunt, but fails closed). Defaults to true. # Chat (Optional) # COPILOT_API_KEY= # Mint one at https://sim.ai. Without it the Sim Chat block, prompt jobs, and Inbox cannot run diff --git a/apps/sim/lib/core/config/env.ts b/apps/sim/lib/core/config/env.ts index 2bca035477e..2c94aa35f2e 100644 --- a/apps/sim/lib/core/config/env.ts +++ b/apps/sim/lib/core/config/env.ts @@ -508,6 +508,7 @@ export const env = createEnv({ // Network / proxy trust AUTH_TRUSTED_PROXIES: z.string().optional(), // Comma-separated reverse-proxy IPs or CIDR ranges. When set, Better Auth and getClientIp (per-IP rate-limit keys, audit rows) both walk the forwarded-IP chain right to left, skip these trusted hops, and use the first untrusted address as the client IP. Leave unset and the two differ: Better Auth trusts only single-value IP headers (recording no IP for a multi-hop chain), while getClientIp keys on the rightmost, proxy-written entry — never the caller-supplied leftmost one. + TRUST_PROXY_HEADERS: z.boolean().optional(), // Whether x-forwarded-for / x-real-ip may be believed at all. Default true: the app is assumed to sit behind a proxy that appends the peer address. Set false when it is exposed directly (no proxy), where those headers are written entirely by the caller — getClientIp then reports 'unknown' so per-IP limits become one shared bucket instead of a per-request bypass. // SSO Configuration (for script-based registration) SSO_ENABLED: z.boolean().optional(), // Enable SSO functionality diff --git a/apps/sim/lib/core/utils/client-ip.test.ts b/apps/sim/lib/core/utils/client-ip.test.ts index a4ea497d11c..5120ad9f75d 100644 --- a/apps/sim/lib/core/utils/client-ip.test.ts +++ b/apps/sim/lib/core/utils/client-ip.test.ts @@ -10,10 +10,17 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockEnv } = vi.hoisted(() => ({ - mockEnv: { AUTH_TRUSTED_PROXIES: undefined as string | undefined }, + mockEnv: { + AUTH_TRUSTED_PROXIES: undefined as string | undefined, + TRUST_PROXY_HEADERS: undefined as string | boolean | undefined, + }, })) -vi.mock('@/lib/core/config/env', () => ({ env: mockEnv })) +vi.mock('@/lib/core/config/env', () => ({ + env: mockEnv, + isFalsy: (value: string | boolean | number | undefined) => + value === false || value === 'false' || value === 0 || value === '0', +})) vi.unmock('@/lib/core/utils/client-ip') /** @@ -21,10 +28,14 @@ vi.unmock('@/lib/core/utils/client-ip') * so each case needs a fresh module instance. This is the deliberate exception * to the repo's "no `vi.resetModules()` + dynamic import" performance rule * (`.cursor/rules/sim-testing.mdc`): module-init behavior cannot be observed any - * other way, and the cost here is four imports of a six-line module. + * other way, and the cost here is a handful of imports of a tiny module. */ -async function loadGetClientIp(trustedProxies: string | undefined) { +async function loadGetClientIp( + trustedProxies: string | undefined, + trustProxyHeaders?: string | boolean +) { mockEnv.AUTH_TRUSTED_PROXIES = trustedProxies + mockEnv.TRUST_PROXY_HEADERS = trustProxyHeaders vi.resetModules() return (await import('@/lib/core/utils/client-ip')).getClientIp } @@ -64,4 +75,23 @@ describe('getClientIp', () => { expect(getClientIp(req({}))).toBe('unknown') }) + + it('declines to read forwarded headers when TRUST_PROXY_HEADERS is false', async () => { + // No proxy in front: the whole header is caller-authored, so every caller + // shares one bucket rather than each minting their own. + const getClientIp = await loadGetClientIp(undefined, 'false') + const keys = ['203.0.113.7, 10.0.0.1', '9.9.9.9', '2001:db8::1'].map((value) => + getClientIp(req({ 'x-forwarded-for': value })) + ) + + expect(new Set(keys)).toEqual(new Set(['unknown'])) + expect(getClientIp(req({ 'x-real-ip': '203.0.113.7' }))).toBe('unknown') + }) + + it('still reads forwarded headers when TRUST_PROXY_HEADERS is unset or true', async () => { + for (const value of [undefined, 'true'] as const) { + const getClientIp = await loadGetClientIp(undefined, value) + expect(getClientIp(req({ 'x-forwarded-for': '203.0.113.7, 10.0.0.1' }))).toBe('10.0.0.1') + } + }) }) diff --git a/apps/sim/lib/core/utils/client-ip.ts b/apps/sim/lib/core/utils/client-ip.ts index 118d7e7fe56..80fddbbf773 100644 --- a/apps/sim/lib/core/utils/client-ip.ts +++ b/apps/sim/lib/core/utils/client-ip.ts @@ -2,8 +2,9 @@ import { type ClientIpHeaderSource, parseTrustedProxies, resolveClientIp, + UNKNOWN_CLIENT_IP, } from '@sim/security/client-ip' -import { env } from '@/lib/core/config/env' +import { env, isFalsy } from '@/lib/core/config/env' /** * Reverse-proxy hops trusted for forwarded-IP resolution, read from the same @@ -17,6 +18,17 @@ import { env } from '@/lib/core/config/env' */ const trustedProxies = parseTrustedProxies(env.AUTH_TRUSTED_PROXIES) +/** + * Whether forwarded headers may be believed at all. + * + * Every rule about which hop to read presumes a proxy wrote at least one of + * them. Reachable directly — no proxy, port published straight to the internet — + * the entire header is caller-authored and no parsing strategy can recover a + * real address from it. Operators of such a deployment set + * `TRUST_PROXY_HEADERS=false`, which makes {@link getClientIp} decline to guess. + */ +const trustForwardedHeaders = !isFalsy(env.TRUST_PROXY_HEADERS) + /** * Extract the client IP from a request for logging, audit trails, and — most * importantly — per-IP rate-limit keys. @@ -27,7 +39,13 @@ const trustedProxies = parseTrustedProxies(env.AUTH_TRUSTED_PROXIES) * See {@link resolveClientIp} for why the chain is walked right to left. In * short: the leftmost `X-Forwarded-For` entry is supplied by the caller, so * keying a throttle on it lets anyone mint a fresh bucket per request. + * + * With `TRUST_PROXY_HEADERS=false` this returns {@link UNKNOWN_CLIENT_IP} for + * every caller, collapsing per-IP limits to a single shared bucket. That is + * deliberately blunt — it throttles unrelated callers together — but it fails + * closed, which a header nobody vouched for does not. */ export function getClientIp(request: ClientIpHeaderSource): string { + if (!trustForwardedHeaders) return UNKNOWN_CLIENT_IP return resolveClientIp(request, trustedProxies) } diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index ca62be1d659..3f3c7e7f75a 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -36,6 +36,16 @@ services: # chain trusted. This all assumes a proxy that appends the peer address — # an app exposed directly to the internet sees only what the caller wrote. - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-} + # TRUST_PROXY_HEADERS: whether x-forwarded-for / x-real-ip may be believed. + # Defaults to FALSE here because this file publishes port 3000 directly and + # ships no reverse proxy — with nothing in front, those headers are written + # entirely by the caller, and believing them would let anyone rotate a + # header to get a fresh per-IP rate-limit bucket on every request. While + # false, per-IP limits collapse into one shared bucket: blunt, but it fails + # closed. Set it to true once a proxy that APPENDS the peer address (nginx, + # Caddy, Traefik, an ALB, Cloudflare) terminates in front of the app, and + # set AUTH_TRUSTED_PROXIES to that proxy's address at the same time. + - TRUST_PROXY_HEADERS=${TRUST_PROXY_HEADERS:-false} - BETTER_AUTH_SECRET=${BETTER_AUTH_SECRET} - ENCRYPTION_KEY=${ENCRYPTION_KEY} - API_ENCRYPTION_KEY=${API_ENCRYPTION_KEY:-} diff --git a/helm/sim/values.schema.json b/helm/sim/values.schema.json index 7a7f7e71b23..7964b17a7a5 100644 --- a/helm/sim/values.schema.json +++ b/helm/sim/values.schema.json @@ -159,6 +159,10 @@ "type": "string", "description": "Comma-separated reverse-proxy IPs or CIDR ranges in front of the app (e.g. the ingress pods, '10.42.0.0/24'). When set, Better Auth and Sim's per-IP rate limits both walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP. Leave empty and the two differ: Better Auth trusts only a single-value header, while Sim's throttles key on the rightmost, proxy-written entry. Do not use a range broad enough to also cover client traffic — a caller inside a trusted range makes the whole chain trusted." }, + "TRUST_PROXY_HEADERS": { + "type": "string", + "description": "Whether x-forwarded-for / x-real-ip may be believed at all. Empty (the default) means yes, which is correct behind the chart's ingress. Set to 'false' only when the app is exposed with no proxy appending the peer address, where those headers are entirely caller-written; per-IP rate limits then collapse to one shared bucket rather than being bypassable per request." + }, "SSO_TRUSTED_PROVIDER_IDS": { "type": "string", "description": "Comma-separated SSO provider IDs to trust for automatic account linking when an SSO sign-in matches an existing account's email. Only needed for IdPs that do not assert email_verified. Merged into Better Auth accountLinking.trustedProviders." diff --git a/helm/sim/values.yaml b/helm/sim/values.yaml index 1a29d3bdfec..d4c60970da9 100644 --- a/helm/sim/values.yaml +++ b/helm/sim/values.yaml @@ -91,6 +91,13 @@ app: # to also cover client traffic: a caller whose own address falls inside a trusted range makes # the whole chain trusted and can then forge the value Sim keys on. AUTH_TRUSTED_PROXIES: "" + # TRUST_PROXY_HEADERS: whether x-forwarded-for / x-real-ip may be believed at all. Left empty + # (= true), which is correct here because the chart runs behind an ingress that appends the peer + # address. Set to "false" only if you expose the Service directly with no proxy in front: with + # nothing appending, those headers are written entirely by the caller and believing them lets + # anyone rotate a header for a fresh per-IP rate-limit bucket per request. While false, per-IP + # limits collapse into one shared bucket — blunt, but it fails closed. + TRUST_PROXY_HEADERS: "" # SOCKET_SERVER_URL: Auto-detected when realtime.enabled=true (uses internal service) # NEXT_PUBLIC_SOCKET_URL: public WebSocket URL for browsers. Leave empty to default to the # page's own origin (assumes the ingress/reverse proxy routes /socket.io to the realtime service). diff --git a/packages/audit/src/log.ts b/packages/audit/src/log.ts index 704f1e48804..c3ad7f66855 100644 --- a/packages/audit/src/log.ts +++ b/packages/audit/src/log.ts @@ -4,6 +4,7 @@ import { type ClientIpHeaderSource, parseTrustedProxies, resolveClientIp, + UNKNOWN_CLIENT_IP, } from '@sim/security/client-ip' import { generateShortId } from '@sim/utils/id' import { eq } from 'drizzle-orm' @@ -39,12 +40,22 @@ interface AuditLogParams { */ const trustedProxies = parseTrustedProxies(process.env.AUTH_TRUSTED_PROXIES) +/** + * Mirrors the app's `TRUST_PROXY_HEADERS`. Recording a caller-authored address + * as forensic evidence is worse than recording none, so a deployment that + * declares it has no proxy in front gets `unknown` rather than a fabrication. + */ +const trustForwardedHeaders = !/^(false|0|no|off)$/i.test( + (process.env.TRUST_PROXY_HEADERS ?? '').trim() +) + /** * An audit row's `ipAddress` is forensic evidence, so it must not be whatever * the caller put in the leftmost `X-Forwarded-For` entry. See * {@link resolveClientIp}. */ function getClientIp(request: ClientIpHeaderSource): string { + if (!trustForwardedHeaders) return UNKNOWN_CLIENT_IP return resolveClientIp(request, trustedProxies) } From eb578bdc269d9e3457d55bd82748a9040fda2fe6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:20:22 -0700 Subject: [PATCH 6/8] fix(security): apply the proxy-trust gate in the docs app too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs Ask-AI limiter honored the trusted-proxy list but not TRUST_PROXY_HEADERS, so on a direct exposure it still keyed on a caller-authored header — leaving paid inference unmetered on the one endpoint where that costs real money. Same gate as the app and audit package now. Consolidate the predicate into parseTrustForwardedHeaders rather than keep a third copy of the spelling check. Three hand-rolled copies of a security predicate drifting apart is the exact failure this PR started as. --- apps/docs/app/api/chat/route.ts | 17 ++++++++++++- apps/sim/lib/core/utils/client-ip.test.ts | 6 +---- apps/sim/lib/core/utils/client-ip.ts | 5 ++-- packages/audit/src/log.ts | 5 ++-- packages/security/src/client-ip.test.ts | 29 +++++++++++++++++++++++ packages/security/src/client-ip.ts | 19 +++++++++++++++ 6 files changed, 70 insertions(+), 11 deletions(-) diff --git a/apps/docs/app/api/chat/route.ts b/apps/docs/app/api/chat/route.ts index d247afee499..141808c03ea 100644 --- a/apps/docs/app/api/chat/route.ts +++ b/apps/docs/app/api/chat/route.ts @@ -1,5 +1,10 @@ import { openai } from '@ai-sdk/openai' -import { parseTrustedProxies, resolveClientIp } from '@sim/security/client-ip' +import { + parseTrustedProxies, + parseTrustForwardedHeaders, + resolveClientIp, + UNKNOWN_CLIENT_IP, +} from '@sim/security/client-ip' import { convertToModelMessages, jsonSchema, @@ -80,6 +85,15 @@ const rateLimitHits = new Map() */ const trustedProxies = parseTrustedProxies(process.env.AUTH_TRUSTED_PROXIES) +/** + * Mirrors the main app's `TRUST_PROXY_HEADERS`. Every rule about which hop to + * read presumes a proxy wrote one of them; with nothing in front, the header is + * caller-authored and this limiter guards paid inference, so decline to guess + * and let all callers share one bucket. Defaults to true — the docs site is + * served behind an edge that sets the header. + */ +const trustForwardedHeaders = parseTrustForwardedHeaders(process.env.TRUST_PROXY_HEADERS) + /** * Resolve the client IP from forwarding headers, falling back to a shared * bucket. Walks the chain right to left: the leftmost `X-Forwarded-For` entry is @@ -88,6 +102,7 @@ const trustedProxies = parseTrustedProxies(process.env.AUTH_TRUSTED_PROXIES) * spend plus unbounded growth of `rateLimitHits`. See {@link resolveClientIp}. */ function getClientIp(req: Request): string { + if (!trustForwardedHeaders) return UNKNOWN_CLIENT_IP return resolveClientIp(req, trustedProxies) } diff --git a/apps/sim/lib/core/utils/client-ip.test.ts b/apps/sim/lib/core/utils/client-ip.test.ts index 5120ad9f75d..61ce7d4f0c6 100644 --- a/apps/sim/lib/core/utils/client-ip.test.ts +++ b/apps/sim/lib/core/utils/client-ip.test.ts @@ -16,11 +16,7 @@ const { mockEnv } = vi.hoisted(() => ({ }, })) -vi.mock('@/lib/core/config/env', () => ({ - env: mockEnv, - isFalsy: (value: string | boolean | number | undefined) => - value === false || value === 'false' || value === 0 || value === '0', -})) +vi.mock('@/lib/core/config/env', () => ({ env: mockEnv })) vi.unmock('@/lib/core/utils/client-ip') /** diff --git a/apps/sim/lib/core/utils/client-ip.ts b/apps/sim/lib/core/utils/client-ip.ts index 80fddbbf773..cd0ee003571 100644 --- a/apps/sim/lib/core/utils/client-ip.ts +++ b/apps/sim/lib/core/utils/client-ip.ts @@ -1,10 +1,11 @@ import { type ClientIpHeaderSource, parseTrustedProxies, + parseTrustForwardedHeaders, resolveClientIp, UNKNOWN_CLIENT_IP, } from '@sim/security/client-ip' -import { env, isFalsy } from '@/lib/core/config/env' +import { env } from '@/lib/core/config/env' /** * Reverse-proxy hops trusted for forwarded-IP resolution, read from the same @@ -27,7 +28,7 @@ const trustedProxies = parseTrustedProxies(env.AUTH_TRUSTED_PROXIES) * real address from it. Operators of such a deployment set * `TRUST_PROXY_HEADERS=false`, which makes {@link getClientIp} decline to guess. */ -const trustForwardedHeaders = !isFalsy(env.TRUST_PROXY_HEADERS) +const trustForwardedHeaders = parseTrustForwardedHeaders(env.TRUST_PROXY_HEADERS) /** * Extract the client IP from a request for logging, audit trails, and — most diff --git a/packages/audit/src/log.ts b/packages/audit/src/log.ts index c3ad7f66855..b38e422ebcc 100644 --- a/packages/audit/src/log.ts +++ b/packages/audit/src/log.ts @@ -3,6 +3,7 @@ import { createLogger } from '@sim/logger' import { type ClientIpHeaderSource, parseTrustedProxies, + parseTrustForwardedHeaders, resolveClientIp, UNKNOWN_CLIENT_IP, } from '@sim/security/client-ip' @@ -45,9 +46,7 @@ const trustedProxies = parseTrustedProxies(process.env.AUTH_TRUSTED_PROXIES) * as forensic evidence is worse than recording none, so a deployment that * declares it has no proxy in front gets `unknown` rather than a fabrication. */ -const trustForwardedHeaders = !/^(false|0|no|off)$/i.test( - (process.env.TRUST_PROXY_HEADERS ?? '').trim() -) +const trustForwardedHeaders = parseTrustForwardedHeaders(process.env.TRUST_PROXY_HEADERS) /** * An audit row's `ipAddress` is forensic evidence, so it must not be whatever diff --git a/packages/security/src/client-ip.test.ts b/packages/security/src/client-ip.test.ts index 97dd36d8f6c..f980a7e2d40 100644 --- a/packages/security/src/client-ip.test.ts +++ b/packages/security/src/client-ip.test.ts @@ -3,6 +3,7 @@ import { canonicalizeIp, getAssertedOriginIp, parseTrustedProxies, + parseTrustForwardedHeaders, resolveClientIp, UNKNOWN_CLIENT_IP, } from './client-ip' @@ -269,3 +270,31 @@ describe('parseTrustedProxies', () => { expect(trusted.cidrs).toHaveLength(2) }) }) + +describe('parseTrustForwardedHeaders', () => { + it('defaults to trusting the headers when unset', () => { + // Unset must never silently disable IP resolution — that would turn every + // per-IP limit into one global bucket on an ordinary proxied deployment. + expect(parseTrustForwardedHeaders(undefined)).toBe(true) + expect(parseTrustForwardedHeaders(null)).toBe(true) + expect(parseTrustForwardedHeaders('')).toBe(true) + expect(parseTrustForwardedHeaders(' ')).toBe(true) + }) + + it('accepts the usual falsey spellings, case- and space-insensitively', () => { + for (const value of ['false', 'FALSE', ' False ', '0', 'no', 'off', 'OFF']) { + expect(parseTrustForwardedHeaders(value)).toBe(false) + } + }) + + it('accepts a real boolean, since one caller reads a parsed env', () => { + expect(parseTrustForwardedHeaders(false)).toBe(false) + expect(parseTrustForwardedHeaders(true)).toBe(true) + }) + + it('treats anything else as trusting', () => { + for (const value of ['true', 'yes', 'on', '1', 'anything']) { + expect(parseTrustForwardedHeaders(value)).toBe(true) + } + }) +}) diff --git a/packages/security/src/client-ip.ts b/packages/security/src/client-ip.ts index 50bdc0a420c..bf88879b69d 100644 --- a/packages/security/src/client-ip.ts +++ b/packages/security/src/client-ip.ts @@ -148,6 +148,25 @@ export function parseTrustedProxies(raw: string | null | undefined): TrustedProx return { cidrs } } +/** + * Reads the `TRUST_PROXY_HEADERS` setting: may `X-Forwarded-For` / `X-Real-IP` + * be believed at all? + * + * Every rule about *which* hop to read presumes a proxy wrote one of them. An + * app reachable directly sees a header authored entirely by the caller, and no + * parsing strategy recovers a real address from that — so this is a deployment + * fact the operator has to state, not something the code can detect. + * + * Defaults to `true` (a proxy is assumed) so an unset value never silently + * disables IP resolution. Accepts a boolean or the usual string spellings, + * because the value arrives parsed from the app's env module in one caller and + * raw from `process.env` in others. + */ +export function parseTrustForwardedHeaders(raw: string | boolean | null | undefined): boolean { + if (typeof raw === 'boolean') return raw + return !/^(false|0|no|off)$/i.test((raw ?? '').trim()) +} + /** * Resolves the client IP behind a reverse proxy, safely enough to key a rate * limit on. From 64bc54530cc1c0b2a784663f6f2360ac1d5cda3d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:37:12 -0700 Subject: [PATCH 7/8] fix(security): derive Helm proxy trust from ingress.enabled, fail the password ceiling closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps in the previous commit. The chart defaulted TRUST_PROXY_HEADERS to true, but ingress.enabled defaults to FALSE — so the out-of-the-box install reaches the Service directly (port-forward, LoadBalancer, NodePort) with nothing appending a peer address, and trusted a header written entirely by the caller. Derive the default from ingress.enabled instead: on with the ingress, off without it. An explicit app.env value still wins, for edges the chart cannot see (Gateway API, a service mesh, an external LB that appends). Compare the stringified override, never the raw one — an explicit `false` is falsy in Go templates, so the obvious `if $explicit` silently discarded the one override that turns trust off. Caught by rendering all four combinations; the schema now also accepts a bare YAML boolean, which is what a Helm user writes. The per-resource password ceiling called checkRateLimitDirect without failClosed, and that helper allows on storage error. It is the only bound on distributed guessing at the secret, so failing open removed it during exactly the outage an attacker could wait for. Matches the contact captcha backstop, which already opts in for the same reason. --- apps/sim/app/api/chat/utils.test.ts | 26 ++++++++++++++++++- apps/sim/lib/core/security/deployment-auth.ts | 10 ++++++- helm/sim/templates/_helpers.tpl | 25 ++++++++++++++++++ helm/sim/templates/deployment-app.yaml | 2 ++ helm/sim/values.schema.json | 4 +-- helm/sim/values.yaml | 14 +++++----- 6 files changed, 71 insertions(+), 10 deletions(-) diff --git a/apps/sim/app/api/chat/utils.test.ts b/apps/sim/app/api/chat/utils.test.ts index 0dd9883000b..d843a1372c2 100644 --- a/apps/sim/app/api/chat/utils.test.ts +++ b/apps/sim/app/api/chat/utils.test.ts @@ -253,11 +253,35 @@ describe('Chat API Utils', () => { expect(result.authorized).toBe(false) expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( 'chat-password:resource:chat-id', - expect.objectContaining({ maxTokens: 500 }) + expect.objectContaining({ maxTokens: 500 }), + { failClosed: true } ) expect(mockResetRateLimitBucket).not.toHaveBeenCalled() }) + it('checks the per-resource ceiling fail-closed so an outage cannot lift it', async () => { + // It is the only bound on distributed guessing at the secret; failing open + // would silently remove it during exactly the outage an attacker waits for. + const deployment = { + id: 'chat-id', + authType: 'password', + password: 'encrypted-password', + } + const mockRequest = { + method: 'POST', + cookies: { get: vi.fn().mockReturnValue(null) }, + } as any + + await validateChatAuth('request-id', deployment, mockRequest, { + password: 'correct-password', + }) + + const resourceCall = mockCheckRateLimitDirect.mock.calls.find((call: unknown[]) => + String(call[0]).includes(':resource:') + ) + expect(resourceCall?.[2]).toEqual({ failClosed: true }) + }) + it('rejects guesses once the per-resource counter is exhausted, without decrypting', async () => { const deployment = { id: 'chat-id', diff --git a/apps/sim/lib/core/security/deployment-auth.ts b/apps/sim/lib/core/security/deployment-auth.ts index 2a56d6bd148..d1777e990e8 100644 --- a/apps/sim/lib/core/security/deployment-auth.ts +++ b/apps/sim/lib/core/security/deployment-auth.ts @@ -148,9 +148,17 @@ export async function validateDeploymentAuth( } const resourceKey = `${cookiePrefix}-password:resource:${resource.id}` + /** + * `failClosed` because this is the only bound on distributed guessing at + * the secret: failing open would silently remove it during exactly the + * storage outage an attacker could wait for. The cost is bounded — the + * bucket store is Redis or the app database, and if the database is down + * the deployment is unreachable anyway. + */ const resourceRateLimit = await rateLimiter.checkRateLimitDirect( resourceKey, - PASSWORD_RESOURCE_RATE_LIMIT + PASSWORD_RESOURCE_RATE_LIMIT, + { failClosed: true } ) if (!resourceRateLimit.allowed) { logger.warn( diff --git a/helm/sim/templates/_helpers.tpl b/helm/sim/templates/_helpers.tpl index 4ecaf0d263b..05180a70422 100644 --- a/helm/sim/templates/_helpers.tpl +++ b/helm/sim/templates/_helpers.tpl @@ -446,6 +446,31 @@ Ollama URL {{- end }} {{- end }} +{{/* +Whether the app may believe x-forwarded-for / x-real-ip. + +Derived from ingress.enabled rather than defaulted to "true": every rule about +which forwarded hop to read presumes a proxy wrote one of them. With the ingress +off, the Service is reached directly (ClusterIP port-forward, LoadBalancer, +NodePort) and the header is authored entirely by the caller — trusting it would +let anyone rotate it for a fresh per-IP rate-limit bucket per request. An +explicit app.env.TRUST_PROXY_HEADERS always wins, for edges the chart cannot see +(a Gateway API listener, a service mesh, an external LB that appends). +*/}} +{{- define "sim.trustProxyHeaders" -}} +{{- $explicit := toString ((default (dict) .Values.app.env).TRUST_PROXY_HEADERS) -}} +{{- /* + Compare the STRINGIFIED value, never the raw one: an explicit `false` is falsy + in Go templates, so `if $explicit` would silently discard the one override + that turns trust off and fall through to the ingress default. +*/ -}} +{{- if or (eq $explicit "") (eq $explicit "") -}} +{{- ternary "true" "false" .Values.ingress.enabled -}} +{{- else -}} +{{- $explicit -}} +{{- end -}} +{{- end }} + {{/* PII (Presidio) service URL */}} diff --git a/helm/sim/templates/deployment-app.yaml b/helm/sim/templates/deployment-app.yaml index f3506edf53c..ffbea7ce79a 100644 --- a/helm/sim/templates/deployment-app.yaml +++ b/helm/sim/templates/deployment-app.yaml @@ -91,6 +91,8 @@ spec: value: {{ include "sim.ollamaUrl" . | quote }} - name: PII_URL value: {{ include "sim.piiUrl" . | quote }} + - name: TRUST_PROXY_HEADERS + value: {{ include "sim.trustProxyHeaders" . | quote }} {{- /* Skip envDefaults keys that the user has explicitly overridden in app.env with a non-empty value. K8s `env` takes precedence over `envFrom`, so an diff --git a/helm/sim/values.schema.json b/helm/sim/values.schema.json index 7964b17a7a5..79206dc8732 100644 --- a/helm/sim/values.schema.json +++ b/helm/sim/values.schema.json @@ -160,8 +160,8 @@ "description": "Comma-separated reverse-proxy IPs or CIDR ranges in front of the app (e.g. the ingress pods, '10.42.0.0/24'). When set, Better Auth and Sim's per-IP rate limits both walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP. Leave empty and the two differ: Better Auth trusts only a single-value header, while Sim's throttles key on the rightmost, proxy-written entry. Do not use a range broad enough to also cover client traffic — a caller inside a trusted range makes the whole chain trusted." }, "TRUST_PROXY_HEADERS": { - "type": "string", - "description": "Whether x-forwarded-for / x-real-ip may be believed at all. Empty (the default) means yes, which is correct behind the chart's ingress. Set to 'false' only when the app is exposed with no proxy appending the peer address, where those headers are entirely caller-written; per-IP rate limits then collapse to one shared bucket rather than being bypassable per request." + "type": ["string", "boolean"], + "description": "Whether x-forwarded-for / x-real-ip may be believed at all. Leave empty to derive it from ingress.enabled: on with the ingress (which appends the peer address), off without it, since a directly-reached Service sees a header written entirely by the caller and trusting it makes per-IP rate limits bypassable per request. Set explicitly ('true'/'false', quoted or bare) only for an edge the chart cannot see, e.g. a Gateway API listener, a service mesh, or an external load balancer that appends." }, "SSO_TRUSTED_PROVIDER_IDS": { "type": "string", diff --git a/helm/sim/values.yaml b/helm/sim/values.yaml index d4c60970da9..a9299ff0c01 100644 --- a/helm/sim/values.yaml +++ b/helm/sim/values.yaml @@ -91,12 +91,14 @@ app: # to also cover client traffic: a caller whose own address falls inside a trusted range makes # the whole chain trusted and can then forge the value Sim keys on. AUTH_TRUSTED_PROXIES: "" - # TRUST_PROXY_HEADERS: whether x-forwarded-for / x-real-ip may be believed at all. Left empty - # (= true), which is correct here because the chart runs behind an ingress that appends the peer - # address. Set to "false" only if you expose the Service directly with no proxy in front: with - # nothing appending, those headers are written entirely by the caller and believing them lets - # anyone rotate a header for a fresh per-IP rate-limit bucket per request. While false, per-IP - # limits collapse into one shared bucket — blunt, but it fails closed. + # TRUST_PROXY_HEADERS: whether x-forwarded-for / x-real-ip may be believed at all. + # Left empty it is DERIVED from ingress.enabled — on with the ingress (which appends the peer + # address), off without it. That matters because ingress.enabled defaults to false: a Service + # reached directly (port-forward, LoadBalancer, NodePort) sees a header written entirely by the + # caller, and believing it lets anyone rotate a header for a fresh per-IP rate-limit bucket on + # every request. While off, per-IP limits collapse into one shared bucket — blunt, but it fails + # closed. Set it explicitly ("true"/"false") only for an edge the chart cannot see: a Gateway + # API listener, a service mesh, or an external load balancer that appends the peer address. TRUST_PROXY_HEADERS: "" # SOCKET_SERVER_URL: Auto-detected when realtime.enabled=true (uses internal service) # NEXT_PUBLIC_SOCKET_URL: public WebSocket URL for browsers. Leave empty to default to the From c0f6ace6992c2006b56e887a48b96b272dbdd7dd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:54:24 -0700 Subject: [PATCH 8/8] fix(helm): treat TRUST_PROXY_HEADERS as chart-computed on both deployments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is inlined on the app container like PII_URL, so it has to be in the $chartComputed lists. It was not, which meant setting the documented app.env.TRUST_PROXY_HEADERS override under externalSecrets.enabled failed template validation and demanded a remoteRefs mapping for a value the container never reads from a Secret. It also wrote the key into the chart-managed Secret. Inline it on the realtime deployment too. @sim/audit runs there and reads this to decide whether a forwarded header may be believed when stamping an audit row's ipAddress, and the chart-managed Secret is shared with realtime via envFrom — so excluding the key from that Secret without inlining it would have quietly left realtime trusting headers the operator declared untrustworthy. Verified by rendering: inline, existingSecret, and ESO modes each emit exactly one entry per pod carrying the same value, the key never reaches the Secret, and ESO no longer demands a remoteRef for it. --- helm/sim/templates/_helpers.tpl | 2 +- helm/sim/templates/deployment-app.yaml | 2 +- helm/sim/templates/deployment-realtime.yaml | 11 ++++++++++- helm/sim/templates/secrets-app.yaml | 2 +- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/helm/sim/templates/_helpers.tpl b/helm/sim/templates/_helpers.tpl index 05180a70422..6d040bde233 100644 --- a/helm/sim/templates/_helpers.tpl +++ b/helm/sim/templates/_helpers.tpl @@ -313,7 +313,7 @@ than enforced. {{- define "sim.validateExternalSecretCoverage" -}} {{- if and .Values.externalSecrets .Values.externalSecrets.enabled -}} {{- $remoteRefs := default (dict) (default (dict) .Values.externalSecrets.remoteRefs).app -}} -{{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" -}} +{{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" "TRUST_PROXY_HEADERS" -}} {{- $appEnv := default (dict) .Values.app.env -}} {{/* Required-key coverage: these are non-optional at runtime. With ESO enabled diff --git a/helm/sim/templates/deployment-app.yaml b/helm/sim/templates/deployment-app.yaml index ffbea7ce79a..86bc91c2704 100644 --- a/helm/sim/templates/deployment-app.yaml +++ b/helm/sim/templates/deployment-app.yaml @@ -121,7 +121,7 @@ spec: and in inline mode (values flow through the chart-managed Secret). */}} {{- if and .Values.app.secrets.existingSecret.enabled (not .Values.externalSecrets.enabled) }} - {{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" }} + {{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" "TRUST_PROXY_HEADERS" }} {{- range $key, $value := $appEnv }} {{- if and (ne (toString $value) "") (ne (toString $value) "") (not (has $key $chartComputed)) }} - name: {{ $key }} diff --git a/helm/sim/templates/deployment-realtime.yaml b/helm/sim/templates/deployment-realtime.yaml index 1e3487164bd..2db673eb020 100644 --- a/helm/sim/templates/deployment-realtime.yaml +++ b/helm/sim/templates/deployment-realtime.yaml @@ -62,6 +62,15 @@ spec: env: - name: DATABASE_URL value: {{ include "sim.databaseUrl" . | quote }} + {{- /* + Inlined for the same reason as on the app pod: @sim/audit runs here + too and reads this to decide whether a forwarded header may be + believed when stamping an audit row's ipAddress. Chart-computed, so + it is excluded from the shared Secret and must be set explicitly on + both deployments. + */}} + - name: TRUST_PROXY_HEADERS + value: {{ include "sim.trustProxyHeaders" . | quote }} {{- if .Values.telemetry.enabled }} {{- $nodeEnv := default (default "production" (index (.Values.realtime.envDefaults | default dict) "NODE_ENV")) (index (.Values.realtime.env | default dict) "NODE_ENV") }} # OpenTelemetry configuration @@ -112,7 +121,7 @@ spec: deployment. */}} {{- if and .Values.app.secrets.existingSecret.enabled (not .Values.externalSecrets.enabled) }} - {{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" }} + {{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" "TRUST_PROXY_HEADERS" }} {{- /* Build the effective realtime env from app.env as the base, then overlay non-empty realtime.env values. Sprig's `merge` keeps the diff --git a/helm/sim/templates/secrets-app.yaml b/helm/sim/templates/secrets-app.yaml index cf598096b5f..9385c206b63 100644 --- a/helm/sim/templates/secrets-app.yaml +++ b/helm/sim/templates/secrets-app.yaml @@ -18,7 +18,7 @@ metadata: {{- include "sim.app.labels" . | nindent 4 }} type: Opaque stringData: - {{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" }} + {{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" "TRUST_PROXY_HEADERS" }} {{- /* Intent: app.env is authoritative for shared keys (both pods envFrom this Secret, so the app container must not be silently overwritten by a