From d6d41801536884f09f92d7a36fa9d1fae181523e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 18:12:08 -0700 Subject: [PATCH] fix(security): resolve the client IP from the trusted proxy chain, not the leftmost header The leftmost x-forwarded-for token is written by the client, so every IP-derived decision was steerable by sending your own header: per-IP rate limits could be reset per request, audit rows recorded an attacker-chosen address, and per-webhook IP allowlists could be satisfied outright. Resolution now walks the chain right to left against AUTH_TRUSTED_PROXIES and returns null when no address can be trusted, delegating to Better Auth's own resolver so this agrees by construction with the ipAddress on every session row. Callers split on that null deliberately: rate limits share one strict bucket, the webhook allowlist denies, audit records blank. --- .../platform/self-hosting/authentication.mdx | 12 +- .../self-hosting/environment-variables.mdx | 2 +- .../en/platform/self-hosting/security.mdx | 19 +- apps/sim/.env.example | 2 +- .../api/chat/[identifier]/otp/route.test.ts | 8 +- .../app/api/chat/[identifier]/otp/route.ts | 4 +- .../app/api/chat/[identifier]/sso/route.ts | 4 +- apps/sim/app/api/chat/utils.test.ts | 4 + apps/sim/app/api/contact/route.ts | 4 +- apps/sim/app/api/demo-requests/route.ts | 4 +- .../app/api/files/public/[token]/otp/route.ts | 4 +- .../app/api/files/public/[token]/sso/route.ts | 4 +- .../app/api/help/integration-request/route.ts | 4 +- apps/sim/lib/analytics/profound.ts | 7 +- apps/sim/lib/auth/auth.ts | 41 +++- apps/sim/lib/auth/internal.ts | 6 +- apps/sim/lib/core/config/env.ts | 2 +- .../core/rate-limiter/route-helpers.test.ts | 50 +++-- .../lib/core/rate-limiter/route-helpers.ts | 9 +- apps/sim/lib/core/security/deployment-auth.ts | 7 +- apps/sim/lib/core/utils/request.ts | 36 +++- apps/sim/lib/public-shares/rate-limit.ts | 4 +- apps/sim/lib/webhooks/providers/generic.ts | 8 +- apps/sim/proxy.ts | 4 +- bun.lock | 3 + docker-compose.prod.yml | 11 +- helm/sim/values.yaml | 14 +- packages/audit/package.json | 1 + packages/audit/src/log.test.ts | 49 +++-- packages/audit/src/log.ts | 21 +- packages/security/package.json | 5 + packages/security/src/client-ip.test.ts | 190 ++++++++++++++++++ packages/security/src/client-ip.ts | 102 ++++++++++ packages/testing/package.json | 1 + packages/testing/src/mocks/request.mock.ts | 22 +- 35 files changed, 551 insertions(+), 117 deletions(-) create mode 100644 packages/security/src/client-ip.test.ts create mode 100644 packages/security/src/client-ip.ts diff --git a/apps/docs/content/docs/en/platform/self-hosting/authentication.mdx b/apps/docs/content/docs/en/platform/self-hosting/authentication.mdx index 15bcd34c7f7..739b9cf30e9 100644 --- a/apps/docs/content/docs/en/platform/self-hosting/authentication.mdx +++ b/apps/docs/content/docs/en/platform/self-hosting/authentication.mdx @@ -109,13 +109,21 @@ Both SSO flags are needed: the server-side one grants access, and the `NEXT_PUBL ## Behind a load balancer -Tell Better Auth which forwarding hops to trust when resolving the client IP: +Tell Sim which forwarding hops to trust when resolving the client IP: ```bash AUTH_TRUSTED_PROXIES=10.0.0.0/24,192.0.2.10 ``` -Better Auth walks `X-Forwarded-For` right to left, skips these hops, and uses the first untrusted address as the client IP for session records and its own IP-based checks. Use your proxies' actual addresses — a broad private range that also covers client traffic defeats the purpose. See [Security](/platform/self-hosting/security). +The forwarded chain is walked right to left, skipping these hops, and the first untrusted address becomes the client IP. Use your proxies' actual addresses — a broad private range that also covers client traffic defeats the purpose, and `0.0.0.0/0` matches every hop, so the walk finds no client at all. + +This one variable governs every IP-derived behavior in Sim: session records, per-IP rate limits on public endpoints, the `ipAddress` column on audit log entries, and per-webhook IP allowlists. + + + Leave it unset behind a proxy and Sim resolves no client IP at all. Nothing breaks outright — per-IP rate limits fall back to one shared bucket per endpoint (stricter, not weaker), audit rows record a blank IP, and any webhook configured with an IP allowlist rejects every delivery. Sim will not read the leftmost `X-Forwarded-For` value as a fallback, because a client can set that header itself. + + +Sim reads `X-Forwarded-For` first and falls back to `X-Real-IP`. If your ingress sets only `X-Real-IP` (nginx's `proxy_set_header X-Real-IP $remote_addr`), that is enough — but make sure the header is *overwritten* at the edge rather than passed through from the client. See [Security](/platform/self-hosting/security). ## Disabling authentication entirely diff --git a/apps/docs/content/docs/en/platform/self-hosting/environment-variables.mdx b/apps/docs/content/docs/en/platform/self-hosting/environment-variables.mdx index 8a2f01bf27f..287a2faae7c 100644 --- a/apps/docs/content/docs/en/platform/self-hosting/environment-variables.mdx +++ b/apps/docs/content/docs/en/platform/self-hosting/environment-variables.mdx @@ -30,7 +30,7 @@ import { Callout } from 'fumadocs-ui/components/callout' | `REDIS_TLS_SERVERNAME` | TLS SNI override. Required when `REDIS_URL` uses `rediss://` with a bare IP, or the app throws at startup | | `NEXT_PUBLIC_SOCKET_URL` | WebSocket URL — defaults to the page origin; set only if realtime is on a separate host | | `TRUSTED_ORIGINS` | Comma-separated additional origins to trust for auth (apex + `www`, alias domains) | -| `AUTH_TRUSTED_PROXIES` | Comma-separated reverse-proxy IPs/CIDRs so the client IP cannot be forged through `X-Forwarded-For` | +| `AUTH_TRUSTED_PROXIES` | Comma-separated reverse-proxy IPs/CIDRs so the client IP cannot be forged through `X-Forwarded-For`. Governs sessions, per-IP rate limits, audit log IPs, and webhook IP allowlists — behind a proxy, leaving it unset means no client IP resolves at all | | `INTERNAL_API_BASE_URL` | Internal URL for server-side self-calls, e.g. `http://sim-app.simstudio.svc.cluster.local:3000`. Required for PII log redaction; defaults to `NEXT_PUBLIC_APP_URL` | | `DATABASE_REPLICA_URL` | Read-replica connection string for log listing, audit logs, and dashboard aggregations. Falls back to the primary when unset | diff --git a/apps/docs/content/docs/en/platform/self-hosting/security.mdx b/apps/docs/content/docs/en/platform/self-hosting/security.mdx index 359cb3557c4..a170aaa9198 100644 --- a/apps/docs/content/docs/en/platform/self-hosting/security.mdx +++ b/apps/docs/content/docs/en/platform/self-hosting/security.mdx @@ -148,7 +148,22 @@ ALLOW_PRIVATE_DATABASE_HOSTS=true ## Client IP and forwarded headers -Behind a load balancer, `X-Forwarded-For` is client-controllable. Set `AUTH_TRUSTED_PROXIES` to your proxies' actual addresses so Better Auth resolves the real client IP, and `TRUSTED_ORIGINS` if users reach Sim from more than one origin. Both are covered in [Authentication](/platform/self-hosting/authentication#behind-a-load-balancer). +`X-Forwarded-For` is appended to by each hop, and the leftmost value is written by the client — so behind a load balancer it is attacker-controlled. Sim never reads it directly. It walks the chain right to left, skips the hops you declare in `AUTH_TRUSTED_PROXIES`, and takes the first address it does not recognize as a proxy. A chain it cannot verify resolves to no IP at all rather than to a guess. + +Set `AUTH_TRUSTED_PROXIES` to your proxies' actual addresses, and `TRUSTED_ORIGINS` if users reach Sim from more than one origin. Both are covered in [Authentication](/platform/self-hosting/authentication#behind-a-load-balancer). + +What depends on it: + +| Surface | With it set | Without it, behind a proxy | +|---|---|---| +| Session records | Real client IP on the session row | Blank | +| Per-IP rate limits on public endpoints | One bucket per client | One shared bucket per endpoint — stricter, and not amplifiable | +| Audit log `ipAddress` | Real client IP | Blank | +| Per-webhook IP allowlists | Enforced against the real client | Every delivery rejected | + + + Do not set it to `0.0.0.0/0` (or `::/0`) to "just make it work". Every hop then counts as a trusted proxy, the walk runs past the end of the chain, and *no* request resolves an IP. Sim logs an error at startup if you do, and likewise if an entry is not a valid IP or CIDR — a typo silently drops that entry. + ## Restricting who can use the instance @@ -179,7 +194,7 @@ The service bundles ~2.2 GB of spaCy models, so first start takes around three m - [ ] Images pinned to an explicit tag or digest on app, realtime, and migrations - [ ] TLS terminating at the ingress; HTTP redirected or disabled - [ ] `NEXT_PUBLIC_APP_URL` and `BETTER_AUTH_URL` set to the real public origin -- [ ] `AUTH_TRUSTED_PROXIES` set if behind a load balancer +- [ ] `AUTH_TRUSTED_PROXIES` set to the ingress addresses if behind a load balancer (not `0.0.0.0/0`) - [ ] Signup restricted (`DISABLE_REGISTRATION` or `ALLOWED_LOGIN_DOMAINS`) - [ ] `DISABLE_AUTH` **not** set - [ ] NetworkPolicy enabled and `ingressFrom` scoped to the ingress controller diff --git a/apps/sim/.env.example b/apps/sim/.env.example index 4b3c6dc81c5..bb1eee13d84 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. Sim walks x-forwarded-for right to left, skips these hops, and uses the first untrusted address as the client IP (prevents forwarded-header spoofing). Governs session IPs, per-IP rate limits, audit log IPs, and webhook IP allowlists. Use your proxies' actual addresses — not broad private ranges that also cover clients, and never 0.0.0.0/0 (matches every hop, so nothing resolves). # 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.test.ts b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts index 1c60db08fa3..ea3c752d235 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.test.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.test.ts @@ -201,7 +201,7 @@ describe('Chat OTP API Route', () => { })) requestUtilsMockFns.mockGenerateRequestId.mockReturnValue('req-123') - requestUtilsMockFns.mockGetClientIp.mockReturnValue('1.2.3.4') + requestUtilsMockFns.mockResolveClientIp.mockReturnValue('1.2.3.4') mockCheckRateLimitDirect.mockResolvedValue({ allowed: true, @@ -342,8 +342,8 @@ describe('Chat OTP API Route', () => { expect(headerSet).toHaveBeenCalledWith('Retry-After', '900') }) - it('folds spoofed `unknown` client IPs into a single shared bucket', async () => { - requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce('unknown') + it('folds untrustworthy client IPs into a single shared bucket', async () => { + requestUtilsMockFns.mockResolveClientIp.mockReturnValueOnce(null) queueDeployment(emailDeployment) const request = new NextRequest('http://localhost:3000/api/chat/test/otp', { @@ -355,7 +355,7 @@ describe('Chat OTP API Route', () => { expect(mockCheckRateLimitDirect).toHaveBeenCalledTimes(2) expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( - expect.stringMatching(/^chat-otp:ip:.*:unknown$/), + expect.stringMatching(/^chat-otp:ip:.*:unresolved$/), expect.any(Object) ) expect(mockCheckRateLimitDirect).toHaveBeenCalledWith( diff --git a/apps/sim/app/api/chat/[identifier]/otp/route.ts b/apps/sim/app/api/chat/[identifier]/otp/route.ts index 9f7fffd741f..e113f853f95 100644 --- a/apps/sim/app/api/chat/[identifier]/otp/route.ts +++ b/apps/sim/app/api/chat/[identifier]/otp/route.ts @@ -19,7 +19,7 @@ import { OTP_IP_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { generateRequestId, getRateLimitIpKey } 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' @@ -35,7 +35,7 @@ export const POST = withRouteHandler( const requestId = generateRequestId() try { - const ip = getClientIp(request) + const ip = getRateLimitIpKey(request) const ipRateLimit = await rateLimiter.checkRateLimitDirect( `chat-otp:ip:${identifier}:${ip}`, OTP_IP_RATE_LIMIT diff --git a/apps/sim/app/api/chat/[identifier]/sso/route.ts b/apps/sim/app/api/chat/[identifier]/sso/route.ts index c6ab98cfe94..730b78c98f1 100644 --- a/apps/sim/app/api/chat/[identifier]/sso/route.ts +++ b/apps/sim/app/api/chat/[identifier]/sso/route.ts @@ -8,7 +8,7 @@ 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 { generateRequestId, getRateLimitIpKey } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { createErrorResponse, createSuccessResponse } from '@/app/api/workflows/utils' @@ -29,7 +29,7 @@ export const POST = withRouteHandler( async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => { const requestId = generateRequestId() - const ip = getClientIp(request) + const ip = getRateLimitIpKey(request) const ipRateLimit = await rateLimiter.checkRateLimitDirect( `chat-sso:ip:${ip}`, SSO_IP_RATE_LIMIT diff --git a/apps/sim/app/api/chat/utils.test.ts b/apps/sim/app/api/chat/utils.test.ts index 6c41eeb21cc..9d91cdb7db2 100644 --- a/apps/sim/app/api/chat/utils.test.ts +++ b/apps/sim/app/api/chat/utils.test.ts @@ -197,6 +197,7 @@ describe('Chat API Utils', () => { const mockRequest = { method: 'POST', + headers: new Headers(), cookies: { get: vi.fn().mockReturnValue(null), }, @@ -221,6 +222,7 @@ describe('Chat API Utils', () => { const mockRequest = { method: 'POST', + headers: new Headers(), cookies: { get: vi.fn().mockReturnValue(null), }, @@ -247,6 +249,7 @@ describe('Chat API Utils', () => { const mockRequest = { method: 'POST', + headers: new Headers(), cookies: { get: vi.fn().mockReturnValue(null), }, @@ -291,6 +294,7 @@ describe('Chat API Utils', () => { const mockRequest = { method: 'POST', + headers: new Headers(), cookies: { get: vi.fn().mockReturnValue(null), }, diff --git a/apps/sim/app/api/contact/route.ts b/apps/sim/app/api/contact/route.ts index 2b610ec2114..0aaa6434b1b 100644 --- a/apps/sim/app/api/contact/route.ts +++ b/apps/sim/app/api/contact/route.ts @@ -11,7 +11,7 @@ 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 { generateRequestId, getRateLimitIpKey } 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' @@ -53,7 +53,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => { const requestId = generateRequestId() try { - const ip = getClientIp(req) + const ip = getRateLimitIpKey(req) const storageKey = `public:contact:${ip}` const { allowed, remaining, resetAt } = await rateLimiter.checkRateLimitDirect( diff --git a/apps/sim/app/api/demo-requests/route.ts b/apps/sim/app/api/demo-requests/route.ts index 7553239e7b2..6d05e6b1aa8 100644 --- a/apps/sim/app/api/demo-requests/route.ts +++ b/apps/sim/app/api/demo-requests/route.ts @@ -8,7 +8,7 @@ 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 { generateRequestId, getRateLimitIpKey } 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' @@ -27,7 +27,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => { const requestId = generateRequestId() try { - const ip = getClientIp(req) + const ip = getRateLimitIpKey(req) const storageKey = `public:demo-request:${ip}` const { allowed, remaining, resetAt } = await rateLimiter.checkRateLimitDirect( 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..2894b13fe59 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,7 @@ import { OTP_IP_RATE_LIMIT, storeOTP, } from '@/lib/core/security/otp' -import { generateRequestId, getClientIp } from '@/lib/core/utils/request' +import { generateRequestId, getRateLimitIpKey } 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' @@ -57,7 +57,7 @@ export const POST = withRouteHandler( const requestId = generateRequestId() try { - const ip = getClientIp(request) + const ip = getRateLimitIpKey(request) const ipRateLimit = await rateLimiter.checkRateLimitDirect( `file-otp:ip:${ip}`, OTP_IP_RATE_LIMIT 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..3f084a5462f 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,7 @@ 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 { generateRequestId, getRateLimitIpKey } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { resolveActiveShareByToken } from '@/lib/public-shares/share-manager' @@ -33,7 +33,7 @@ export const POST = withRouteHandler( async (request: NextRequest, context: { params: Promise<{ token: string }> }) => { const requestId = generateRequestId() - const ip = getClientIp(request) + const ip = getRateLimitIpKey(request) const ipRateLimit = await rateLimiter.checkRateLimitDirect( `file-sso:ip:${ip}`, SSO_IP_RATE_LIMIT diff --git a/apps/sim/app/api/help/integration-request/route.ts b/apps/sim/app/api/help/integration-request/route.ts index 6a8faf682b6..82b62f81f57 100644 --- a/apps/sim/app/api/help/integration-request/route.ts +++ b/apps/sim/app/api/help/integration-request/route.ts @@ -5,7 +5,7 @@ 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 { generateRequestId, getRateLimitIpKey } 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' @@ -25,7 +25,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => { const requestId = generateRequestId() try { - const ip = getClientIp(req) + const ip = getRateLimitIpKey(req) const storageKey = `public:integration-request:${ip}` const { allowed, remaining, resetAt } = await rateLimiter.checkRateLimitDirect( diff --git a/apps/sim/lib/analytics/profound.ts b/apps/sim/lib/analytics/profound.ts index ff8c568e14d..29e67a2132a 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 { resolveClientIp } from '@/lib/core/utils/request' import { getBaseDomain } from '@/lib/core/utils/urls' const logger = createLogger('ProfoundAnalytics') @@ -102,10 +102,7 @@ export function sendToProfound(request: Request, statusCode: number): void { host: getBaseDomain(), path: url.pathname, status_code: statusCode, - ip: (() => { - const resolved = getClientIp(request) - return resolved === 'unknown' ? '0.0.0.0' : resolved - })(), + ip: resolveClientIp(request) ?? '0.0.0.0', user_agent: request.headers.get('user-agent') || '', ...(Object.keys(queryParams).length > 0 && { query_params: queryParams }), ...(request.headers.get('referer') && { referer: request.headers.get('referer')! }), diff --git a/apps/sim/lib/auth/auth.ts b/apps/sim/lib/auth/auth.ts index dbf25fdaf02..d2edb33ca18 100644 --- a/apps/sim/lib/auth/auth.ts +++ b/apps/sim/lib/auth/auth.ts @@ -4,6 +4,12 @@ import { stripe } from '@better-auth/stripe' import { db } from '@sim/db' import * as schema from '@sim/db/schema' import { createLogger } from '@sim/logger' +import { + CLIENT_IP_HEADERS, + findMalformedTrustedProxies, + isAllTrustingProxyEntry, + parseTrustedProxies, +} from '@sim/security/client-ip' import { toError } from '@sim/utils/errors' import { type BetterAuthOptions, betterAuth, type User } from 'better-auth' import { drizzleAdapter } from 'better-auth/adapters/drizzle' @@ -141,15 +147,31 @@ if (validStripeKey) { } /** - * Reverse-proxy hops trusted for forwarded-IP resolution. When configured, - * Better Auth walks the x-forwarded-for chain right to left, skips these - * hops, and records the first untrusted address as the session client IP — - * preventing header spoofing behind multi-hop proxies. + * Reverse-proxy hops trusted for forwarded-IP resolution: the chain is walked + * right to left, these hops are skipped, and the first untrusted address is the + * client. Parsed with the same helper `resolveClientIp` uses so the session's + * recorded IP and the one every other caller resolves cannot diverge. + */ +const trustedProxies = parseTrustedProxies(env.AUTH_TRUSTED_PROXIES) + +/** + * Both misconfigurations below leave every request resolving no IP at all, and + * neither throws — an operator can only discover them if we say so. */ -const trustedProxies = (env.AUTH_TRUSTED_PROXIES ?? '') - .split(',') - .map((entry) => entry.trim()) - .filter(Boolean) +const malformedTrustedProxies = findMalformedTrustedProxies(trustedProxies) +if (malformedTrustedProxies.length > 0) { + logger.error('AUTH_TRUSTED_PROXIES contains entries that are not an IP or CIDR range', { + malformedTrustedProxies, + }) +} + +const allTrustingProxies = trustedProxies.filter(isAllTrustingProxyEntry) +if (allTrustingProxies.length > 0) { + logger.error( + 'AUTH_TRUSTED_PROXIES trusts every hop, so no request will resolve a client IP — scope it to your ingress ranges', + { allTrustingProxies } + ) +} export const auth = betterAuth({ baseURL: getBaseUrl(), @@ -198,6 +220,9 @@ export const auth = betterAuth({ }, advanced: { ipAddress: { + // The same header list and proxy set `resolveClientIp` uses, so the + // address recorded on a session row is the one every other caller sees. + ipAddressHeaders: [...CLIENT_IP_HEADERS], ...(trustedProxies.length > 0 ? { trustedProxies } : {}), }, }, diff --git a/apps/sim/lib/auth/internal.ts b/apps/sim/lib/auth/internal.ts index 1880efee202..c10de356420 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 { resolveClientIp } from '@/lib/core/utils/request' const logger = createLogger('CronAuth') @@ -101,7 +101,7 @@ export function verifyCronAuth(request: NextRequest, context?: string): NextResp if (!env.CRON_SECRET) { const contextInfo = context ? ` for ${context}` : '' logger.warn(`CRON endpoint accessed but CRON_SECRET is not configured${contextInfo}`, { - ip: getClientIp(request), + ip: resolveClientIp(request), userAgent: request.headers.get('user-agent') ?? 'unknown', context, }) @@ -115,7 +115,7 @@ export function verifyCronAuth(request: NextRequest, context?: string): NextResp const contextInfo = context ? ` for ${context}` : '' logger.warn(`Unauthorized CRON access attempt${contextInfo}`, { hasAuthorizationHeader: authHeader !== null, - ip: getClientIp(request), + ip: resolveClientIp(request), userAgent: request.headers.get('user-agent') ?? 'unknown', context, }) diff --git a/apps/sim/lib/core/config/env.ts b/apps/sim/lib/core/config/env.ts index fd72ec23a89..51ee59920fd 100644 --- a/apps/sim/lib/core/config/env.ts +++ b/apps/sim/lib/core/config/env.ts @@ -525,7 +525,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. When set, the forwarded-IP chain is walked right to left, these trusted hops are skipped, and the first untrusted address becomes the client IP. Governs sessions, per-IP rate limits, audit log IPs, and webhook IP allowlists. Leave unset to trust only single-value IP headers. // SSO Configuration (for script-based registration) SSO_ENABLED: z.boolean().optional(), // Enable SSO functionality 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..ca10680357d 100644 --- a/apps/sim/lib/core/rate-limiter/route-helpers.test.ts +++ b/apps/sim/lib/core/rate-limiter/route-helpers.test.ts @@ -1,7 +1,7 @@ /** * @vitest-environment node */ -import { createMockRequest, requestUtilsMockFns } from '@sim/testing' +import { createMockRequest } from '@sim/testing' import { beforeEach, describe, expect, it, type Mock, vi } from 'vitest' const { mockAdapter } = vi.hoisted(() => ({ @@ -22,15 +22,6 @@ vi.mock('@/lib/core/rate-limiter/storage', async () => { } }) -function passThroughClientIp() { - requestUtilsMockFns.mockGetClientIp.mockImplementation( - (req: { headers: { get(name: string): string | null } }) => - req.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || - req.headers.get('x-real-ip')?.trim() || - 'unknown' - ) -} - import { enforceIpRateLimit, enforceUserOrIpRateLimit, enforceUserRateLimit } from './route-helpers' const consume = mockAdapter.consumeTokens as Mock @@ -102,18 +93,14 @@ describe('route-helpers rate limiting', () => { }) describe('enforceIpRateLimit', () => { - beforeEach(() => { - passThroughClientIp() - }) - - it('uses the X-Forwarded-For client IP in the bucket key', async () => { + it('uses a trustworthy client IP in the bucket key', async () => { consume.mockResolvedValueOnce({ allowed: true, tokensRemaining: 9, resetAt: new Date(), }) const request = createMockRequest('POST', undefined, { - 'x-forwarded-for': '203.0.113.7, 10.0.0.1', + 'x-forwarded-for': '203.0.113.7', }) await enforceIpRateLimit('public-bucket', request) @@ -125,7 +112,30 @@ describe('route-helpers rate limiting', () => { ) }) - it('folds spoofed `X-Forwarded-For: unknown` into a single shared bucket', async () => { + it('does not let a prepended forwarded hop mint a per-request bucket', async () => { + // No trusted proxies are configured, so the chain is unverifiable and + // every such request shares one bucket. Reading the leftmost token + // instead would hand an attacker a fresh full allowance per request. + consume.mockResolvedValue({ + allowed: true, + tokensRemaining: 9, + resetAt: new Date(), + }) + + const reqA = createMockRequest('POST', undefined, { + 'x-forwarded-for': '203.0.113.7, 10.0.0.1', + }) + const reqB = createMockRequest('POST', undefined, { + 'x-forwarded-for': '198.51.100.4, 10.0.0.1', + }) + await enforceIpRateLimit('otp', reqA) + await enforceIpRateLimit('otp', reqB) + + const keys = consume.mock.calls.map((call) => call[0]) + expect(keys).toEqual(['route:otp:ip:unresolved', 'route:otp:ip:unresolved']) + }) + + it('folds an unparseable forwarded header into the same shared bucket', async () => { consume.mockResolvedValue({ allowed: true, tokensRemaining: 9, @@ -138,7 +148,7 @@ describe('route-helpers rate limiting', () => { await enforceIpRateLimit('otp', reqB) const keys = consume.mock.calls.map((call) => call[0]) - expect(keys).toEqual(['route:otp:ip:unknown', 'route:otp:ip:unknown']) + expect(keys).toEqual(['route:otp:ip:unresolved', 'route:otp:ip:unresolved']) }) it('returns a 429 with Retry-After on rate limit', async () => { @@ -159,9 +169,7 @@ describe('route-helpers rate limiting', () => { }) describe('enforceUserOrIpRateLimit', () => { - beforeEach(() => { - passThroughClientIp() - }) + beforeEach(() => {}) it('keys per-user when userId is present', async () => { consume.mockResolvedValueOnce({ diff --git a/apps/sim/lib/core/rate-limiter/route-helpers.ts b/apps/sim/lib/core/rate-limiter/route-helpers.ts index f71115bf532..8472dac6823 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 { getRateLimitIpKey } from '@/lib/core/utils/request' const logger = createLogger('RouteRateLimit') const rateLimiter = new RateLimiter() @@ -55,16 +55,15 @@ export async function enforceUserRateLimit( } /** - * Apply a per-IP token bucket to an unauthenticated route. The `unknown` IP - * fallback shares one global bucket per route so it cannot be amplified by - * `X-Forwarded-For: unknown` spoofing. + * Apply a per-IP token bucket to an unauthenticated route. Requests with no + * trustworthy IP share one bucket per route — see {@link getRateLimitIpKey}. */ export async function enforceIpRateLimit( bucketName: string, request: NextRequest, config: TokenBucketConfig = DEFAULT_PUBLIC_IP_ROUTE_LIMIT ): Promise { - const ip = getClientIp(request) + const ip = getRateLimitIpKey(request) const key = `route:${bucketName}:ip:${ip}` const { allowed, resetAt } = await rateLimiter.checkRateLimitDirect(key, config) if (allowed) return null diff --git a/apps/sim/lib/core/security/deployment-auth.ts b/apps/sim/lib/core/security/deployment-auth.ts index 69c842def61..f830b29178e 100644 --- a/apps/sim/lib/core/security/deployment-auth.ts +++ b/apps/sim/lib/core/security/deployment-auth.ts @@ -1,8 +1,7 @@ import { createLogger } from '@sim/logger' import { safeCompare } from '@sim/security/compare' import type { NextRequest } from 'next/server' -import type { TokenBucketConfig } from '@/lib/core/rate-limiter' -import { RateLimiter } from '@/lib/core/rate-limiter' +import { RateLimiter, type TokenBucketConfig } from '@/lib/core/rate-limiter' import { type DeploymentAuthKind, deploymentAuthCookieName, @@ -10,7 +9,7 @@ import { validateAuthToken, } from '@/lib/core/security/deployment' import { decryptSecret } from '@/lib/core/security/encryption' -import { getClientIp } from '@/lib/core/utils/request' +import { getRateLimitIpKey } from '@/lib/core/utils/request' const logger = createLogger('DeploymentAuth') @@ -105,7 +104,7 @@ export async function validateDeploymentAuth( return { authorized: false, error: 'Authentication configuration error' } } - const ip = getClientIp(request) + const ip = getRateLimitIpKey(request) const ipRateLimit = await rateLimiter.checkRateLimitDirect( `${cookiePrefix}-password:ip:${resource.id}:${ip}`, PASSWORD_IP_RATE_LIMIT diff --git a/apps/sim/lib/core/utils/request.ts b/apps/sim/lib/core/utils/request.ts index 84150f4a38c..568fb9138ab 100644 --- a/apps/sim/lib/core/utils/request.ts +++ b/apps/sim/lib/core/utils/request.ts @@ -1,5 +1,13 @@ import { getRequestContext } from '@sim/logger' +import { + type ClientIpHeaders, + parseTrustedProxies, + resolveClientIp as resolveClientIpWith, + UNRESOLVED_CLIENT_IP_BUCKET, +} from '@sim/security/client-ip' import { generateId } from '@sim/utils/id' +import { env } from '@/lib/core/config/env' + /** * Generate a short request ID for correlation. If called inside a request * context (see `withRouteHandler` and `runWithRequestContext`), returns the @@ -10,13 +18,27 @@ export function generateRequestId(): string { return getRequestContext()?.requestId ?? generateId().slice(0, 8) } +/** Shared with Better Auth's `advanced.ipAddress.trustedProxies` — see auth.ts. */ +const trustedProxies = parseTrustedProxies(env.AUTH_TRUSTED_PROXIES) + +/** + * The request's client IP, or `null` when none can be trusted. `null` is a real + * outcome, not an error: behind an unconfigured proxy the forwarded chain is + * unverifiable, and resolving it anyway would return a client-controlled value. + * Never treat it as a match. + */ +export function resolveClientIp(request: { headers: ClientIpHeaders }): string | null { + return resolveClientIpWith(request, { trustedProxies }) +} + /** - * Extract the client IP from a request, checking `x-forwarded-for` then `x-real-ip`. + * The client IP to key a rate limit on, falling back to one shared bucket when + * no trustworthy address exists — the opposite of what an IP-gated check does + * with the same `null`. Callers behind an unverifiable chain are + * indistinguishable, so they must not each get a full allowance; that is the + * amplification a spoofable IP hands an attacker. Use this for every IP-keyed + * limit rather than resolving and defaulting at the call site. */ -export function getClientIp(request: { headers: { get(name: string): string | null } }): string { - return ( - request.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || - request.headers.get('x-real-ip')?.trim() || - 'unknown' - ) +export function getRateLimitIpKey(request: { headers: ClientIpHeaders }): string { + return resolveClientIp(request) ?? UNRESOLVED_CLIENT_IP_BUCKET } diff --git a/apps/sim/lib/public-shares/rate-limit.ts b/apps/sim/lib/public-shares/rate-limit.ts index 60f7223a60d..ce5657ed162 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 { getRateLimitIpKey } from '@/lib/core/utils/request' const rateLimiter = new RateLimiter() @@ -29,7 +29,7 @@ export async function enforcePublicFileRateLimit( request: { headers: { get(name: string): string | null } }, scope: 'metadata' | 'content' ): Promise { - const ip = getClientIp(request) + const ip = getRateLimitIpKey(request) const config = scope === 'content' ? CONTENT_RATE_LIMIT : METADATA_RATE_LIMIT const result = await rateLimiter.checkRateLimitDirect(`public-file:${scope}:${ip}`, config) if (result.allowed) return null diff --git a/apps/sim/lib/webhooks/providers/generic.ts b/apps/sim/lib/webhooks/providers/generic.ts index 71372bebad6..5f3f174dd79 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 { NextResponse } from 'next/server' -import { getClientIp } from '@/lib/core/utils/request' +import { resolveClientIp } from '@/lib/core/utils/request' import type { AuthContext, EventFilterContext, @@ -31,9 +31,11 @@ export const genericHandler: WebhookProviderHandler = { const allowedIps = providerConfig.allowedIps if (allowedIps && Array.isArray(allowedIps) && allowedIps.length > 0) { - const clientIp = getClientIp(request) + // An unresolvable IP denies — this allowlist is the only thing between + // the webhook and the public internet. + const clientIp = resolveClientIp(request) - if (clientIp === 'unknown' || !allowedIps.includes(clientIp)) { + if (clientIp === null || !allowedIps.includes(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 64adc724673..46c038ee16d 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 { resolveClientIp } from './lib/core/utils/request' import { isNonCanonicalSimHost } from './lib/core/utils/urls' const logger = createLogger('Proxy') @@ -212,7 +212,7 @@ function handleSecurityFiltering(request: NextRequest): NextResponse | null { if (isSuspicious && !isWebhookEndpoint && !isMcpEndpoint && !isMcpOauthDiscoveryEndpoint) { logger.warn('Blocked suspicious request', { userAgent, - ip: getClientIp(request), + ip: resolveClientIp(request), url: request.url, method: request.method, pattern: SUSPICIOUS_UA_PATTERNS.find((pattern) => pattern.test(userAgent))?.toString(), diff --git a/bun.lock b/bun.lock index 80d2428e5b0..e8bbc8df236 100644 --- a/bun.lock +++ b/bun.lock @@ -380,6 +380,7 @@ "dependencies": { "@sim/db": "workspace:*", "@sim/logger": "workspace:*", + "@sim/security": "workspace:*", "@sim/utils": "workspace:*", "drizzle-orm": "^0.45.2", }, @@ -576,6 +577,7 @@ "name": "@sim/security", "version": "0.1.0", "dependencies": { + "@better-auth/core": "1.6.23", "ipaddr.js": "2.3.0", }, "devDependencies": { @@ -601,6 +603,7 @@ "@sim/utils": "workspace:*", }, "devDependencies": { + "@sim/security": "workspace:*", "@sim/tsconfig": "workspace:*", "typescript": "^7.0.2", "vitest": "^4.1.0", diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index 7c733774c9e..c834183037c 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -23,10 +23,13 @@ services: # (apex + www, alias hostnames, reverse-proxy IPs). Empty by default. - TRUSTED_ORIGINS=${TRUSTED_ORIGINS:-} # AUTH_TRUSTED_PROXIES: comma-separated reverse-proxy IPs or CIDR ranges in - # front of the app (ingress, load balancer). Better Auth walks - # x-forwarded-for right to left, skips these hops, and uses the first - # untrusted address as the client IP. Required for correct session IPs and - # rate-limit keying behind a multi-hop proxy chain. Empty by default. + # front of the app (ingress, load balancer). Sim walks x-forwarded-for + # right to left, skips these hops, and uses the first untrusted address as + # the client IP. Governs session IPs, per-IP rate-limit keying, audit log + # IPs, and webhook IP allowlists. Behind a proxy, leaving this empty means + # no client IP resolves at all — the leftmost x-forwarded-for value is + # never used as a fallback, since clients can set it themselves. Do not + # use 0.0.0.0/0: it matches every hop and resolves nothing. - AUTH_TRUSTED_PROXIES=${AUTH_TRUSTED_PROXIES:-} # Required. Compose aborts with this message rather than starting the app # with an empty secret, which would silently corrupt stored credentials. diff --git a/helm/sim/values.yaml b/helm/sim/values.yaml index 43686a3715f..d708f3cc081 100644 --- a/helm/sim/values.yaml +++ b/helm/sim/values.yaml @@ -80,9 +80,17 @@ 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). Sim walks x-forwarded-for right to left, skips these + # hops, and uses the first untrusted address as the client IP; x-real-ip is the fallback when + # x-forwarded-for yields nothing. Governs session IPs, per-IP rate-limit keying, the audit + # log ipAddress column, and per-webhook IP allowlists. + # + # Set this to your ingress controller's pod/service CIDR (e.g. "10.0.0.0/16"), NOT to + # 0.0.0.0/0 — that matches every hop, so the walk finds no client and nothing resolves an IP. + # Left empty behind a proxy, no client IP resolves: rate limits fall back to one shared + # bucket per endpoint, audit rows record a blank IP, and webhook IP allowlists reject all + # deliveries. The leftmost x-forwarded-for value is never used as a fallback — clients can + # set that header themselves. 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..ecfaed64fa8 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,10 +127,10 @@ describe('recordAudit', () => { ) }) - it('extracts IP address from x-forwarded-for header', async () => { + it('extracts IP address from a single-value x-forwarded-for header', async () => { const request = new Request('https://example.com', { headers: { - 'x-forwarded-for': '1.2.3.4, 5.6.7.8', + 'x-forwarded-for': '1.2.3.4', 'user-agent': 'TestAgent/1.0', }, }) @@ -167,6 +155,37 @@ describe('recordAudit', () => { ) }) + it('records no IP for an unverifiable forwarded chain', async () => { + // With no trusted proxies configured the leftmost token is client-supplied. + // A blank `ipAddress` is honest; recording `1.2.3.4` would put an + // attacker-chosen string into the audit trail as though it were evidence. + const request = new Request('https://example.com', { + headers: { + 'x-forwarded-for': '1.2.3.4, 5.6.7.8', + 'user-agent': 'TestAgent/1.0', + }, + }) + + 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: undefined, + userAgent: 'TestAgent/1.0', + }) + ) + }) + 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..57491939e3b 100644 --- a/packages/audit/src/log.ts +++ b/packages/audit/src/log.ts @@ -1,5 +1,6 @@ import { auditLog, db, user } from '@sim/db' import { createLogger } from '@sim/logger' +import { type ClientIpHeaders, parseTrustedProxies, resolveClientIp } from '@sim/security/client-ip' import { generateShortId } from '@sim/utils/id' import { eq } from 'drizzle-orm' import type { AuditActionType, AuditResourceTypeValue } from './types' @@ -26,12 +27,18 @@ interface AuditLogParams { request?: { headers: { get(name: string): string | null } } } -function getClientIp(request: { headers: { get(name: string): string | null } }): string { - return ( - request.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || - request.headers.get('x-real-ip')?.trim() || - 'unknown' - ) +/** Same variable the app and Better Auth read, so all three agree on the IP. */ +const trustedProxies = parseTrustedProxies( + typeof process !== 'undefined' ? process.env.AUTH_TRUSTED_PROXIES : undefined +) + +/** + * The actor's client IP, or `undefined` when none can be trusted. A blank + * `ipAddress` is honest; a client-supplied one is worse than blank, because in + * an audit trail it reads as evidence. + */ +function resolveAuditIpAddress(request: { headers: ClientIpHeaders }): string | undefined { + return resolveClientIp(request, { trustedProxies }) ?? undefined } /** @@ -89,7 +96,7 @@ function buildAuditRow( resourceName: params.resourceName, description: params.description, metadata: params.metadata ?? {}, - ipAddress: params.request ? getClientIp(params.request) : undefined, + ipAddress: params.request ? resolveAuditIpAddress(params.request) : undefined, userAgent: params.request?.headers.get('user-agent') ?? undefined, } } diff --git a/packages/security/package.json b/packages/security/package.json index 68b9e74dfb6..9cc6f80766e 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" @@ -53,6 +57,7 @@ "test:watch": "vitest" }, "dependencies": { + "@better-auth/core": "1.6.23", "ipaddr.js": "2.3.0" }, "devDependencies": { diff --git a/packages/security/src/client-ip.test.ts b/packages/security/src/client-ip.test.ts new file mode 100644 index 00000000000..e5229749f32 --- /dev/null +++ b/packages/security/src/client-ip.test.ts @@ -0,0 +1,190 @@ +import { describe, expect, it } from 'vitest' +import { + findMalformedTrustedProxies, + isAllTrustingProxyEntry, + parseTrustedProxies, + resolveClientIp, +} from './client-ip' + +/** Builds a request stub carrying only the given headers. */ +function requestWith(headers: Record) { + const normalized = new Map(Object.entries(headers).map(([k, v]) => [k.toLowerCase(), v])) + return { headers: { get: (name: string) => normalized.get(name.toLowerCase()) ?? null } } +} + +/** A CIDR covering the load balancer subnet in the multi-hop cases below. */ +const LB_SUBNET = ['10.0.0.0/8'] + +describe('resolveClientIp', () => { + describe('without trusted proxies', () => { + it('trusts a single-value forwarded header', () => { + expect(resolveClientIp(requestWith({ 'x-forwarded-for': '203.0.113.7' }))).toBe('203.0.113.7') + }) + + it('refuses to resolve a multi-hop chain', () => { + // The chain cannot be verified without knowing which hops are ours, and + // the leftmost token is client-supplied. Returning either end would be a + // guess; only `null` is honest. + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': '203.0.113.7, 198.51.100.1' })) + ).toBeNull() + }) + + it('returns null when no forwarded header is present', () => { + expect(resolveClientIp(requestWith({}))).toBeNull() + }) + + it('never returns the literal "unknown"', () => { + expect(resolveClientIp(requestWith({ 'x-forwarded-for': 'unknown' }))).toBeNull() + }) + }) + + describe('with trusted proxies', () => { + it('returns the first untrusted hop, walking right to left', () => { + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': '203.0.113.7, 10.0.0.5' }), { + trustedProxies: LB_SUBNET, + }) + ).toBe('203.0.113.7') + }) + + it('ignores a client-prepended address ahead of the real one', () => { + // The spoof case: an attacker sends `X-Forwarded-For: 203.0.113.9`, the + // load balancer appends the true peer, and the header arrives as the + // chain below. The attacker's value must not win. + const resolved = resolveClientIp( + requestWith({ 'x-forwarded-for': '203.0.113.9, 198.51.100.1, 10.0.0.5' }), + { trustedProxies: LB_SUBNET } + ) + expect(resolved).toBe('198.51.100.1') + expect(resolved).not.toBe('203.0.113.9') + }) + + it('fails closed on a malformed hop rather than skipping it', () => { + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': '203.0.113.7, not-an-ip, 10.0.0.5' }), { + trustedProxies: LB_SUBNET, + }) + ).toBeNull() + }) + + it('returns null when every hop is trusted', () => { + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': '10.0.0.4, 10.0.0.5' }), { + trustedProxies: LB_SUBNET, + }) + ).toBeNull() + }) + + it('resolves nothing when the trusted set covers every address', () => { + // `0.0.0.0/0` is valid CIDR, so it survives parsing and then matches + // every hop — the walk runs off the end and yields null rather than + // falling back to the client-supplied leftmost token. + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': '203.0.113.9, 10.0.0.5' }), { + trustedProxies: ['0.0.0.0/0'], + }) + ).toBeNull() + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': '203.0.113.9' }), { + trustedProxies: ['0.0.0.0/0'], + }) + ).toBeNull() + }) + + it('ignores malformed trusted-proxy entries instead of trusting the chain', () => { + // A typo must not silently disable the chain walk and hand back a real + // proxy hop as though it were the client. + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': '203.0.113.7, 10.0.0.5' }), { + trustedProxies: ['not-a-cidr'], + }) + ).toBeNull() + }) + }) + + describe('header precedence', () => { + it('prefers x-forwarded-for over x-real-ip', () => { + expect( + resolveClientIp( + requestWith({ 'x-forwarded-for': '203.0.113.7', 'x-real-ip': '198.51.100.1' }) + ) + ).toBe('203.0.113.7') + }) + + it('falls back to x-real-ip when x-forwarded-for yields nothing', () => { + expect( + resolveClientIp(requestWith({ 'x-forwarded-for': 'garbage', 'x-real-ip': '198.51.100.1' })) + ).toBe('198.51.100.1') + }) + + it('honors an explicit header list', () => { + expect( + resolveClientIp(requestWith({ 'x-real-ip': '198.51.100.1' }), { + headers: ['x-forwarded-for'], + }) + ).toBeNull() + }) + }) + + describe('IPv6', () => { + it('unwraps an IPv4-mapped address to its IPv4 form', () => { + expect(resolveClientIp(requestWith({ 'x-forwarded-for': '::ffff:203.0.113.7' }))).toBe( + '203.0.113.7' + ) + }) + + it('preserves the full address rather than masking it to a /64', () => { + // Better Auth masks IPv6 to a /64 by default, which would collapse every + // address in a subnet to one value. Identification needs the whole thing, + // returned in the fully-expanded canonical form. + expect(resolveClientIp(requestWith({ 'x-forwarded-for': '2001:db8::dead:beef' }))).toBe( + '2001:0db8:0000:0000:0000:0000:dead:beef' + ) + }) + + it('canonicalizes two spellings of one address to the same value', () => { + // The property downstream equality checks depend on: a caller must never + // see the same client as two different addresses because the proxy + // compressed the zero groups differently. + const compressed = resolveClientIp(requestWith({ 'x-forwarded-for': '2001:db8::1' })) + const expanded = resolveClientIp( + requestWith({ 'x-forwarded-for': '2001:0db8:0000:0000:0000:0000:0000:0001' }) + ) + expect(compressed).toBe(expanded) + }) + }) +}) + +describe('parseTrustedProxies', () => { + it('splits, trims, and drops empty entries', () => { + expect(parseTrustedProxies(' 10.0.0.0/8 , ,192.168.0.0/16 ')).toEqual([ + '10.0.0.0/8', + '192.168.0.0/16', + ]) + }) + + it('returns an empty list for an absent value', () => { + expect(parseTrustedProxies(undefined)).toEqual([]) + expect(parseTrustedProxies('')).toEqual([]) + }) +}) + +describe('findMalformedTrustedProxies', () => { + it('reports only the entries that are not an IP or CIDR', () => { + expect( + findMalformedTrustedProxies(['10.0.0.0/8', 'nope', '203.0.113.7', '10.0.0.0/8x']) + ).toEqual(['nope', '10.0.0.0/8x']) + }) +}) + +describe('isAllTrustingProxyEntry', () => { + it('flags the ranges that trust every hop', () => { + expect(isAllTrustingProxyEntry('0.0.0.0/0')).toBe(true) + expect(isAllTrustingProxyEntry(' ::/0 ')).toBe(true) + }) + + it('leaves a bounded range alone', () => { + expect(isAllTrustingProxyEntry('10.0.0.0/8')).toBe(false) + }) +}) diff --git a/packages/security/src/client-ip.ts b/packages/security/src/client-ip.ts new file mode 100644 index 00000000000..52dcfbed035 --- /dev/null +++ b/packages/security/src/client-ip.ts @@ -0,0 +1,102 @@ +import { findInvalidTrustedProxies, getIPFromHeader } from '@better-auth/core/utils/ip' + +/** + * Headers consulted for the client IP, in precedence order. `x-forwarded-for` + * carries the proxy chain and is the only one verifiable against a trusted-proxy + * set; `x-real-ip` is a single-value fallback for ingresses that set it instead. + * + * Also passed to Better Auth as `advanced.ipAddress.ipAddressHeaders`, whose own + * default is `x-forwarded-for` alone — without it an `x-real-ip`-only deployment + * would resolve an IP here and record none on the session. + */ +export const CLIENT_IP_HEADERS = ['x-forwarded-for', 'x-real-ip'] as const + +/** + * Better Auth masks IPv6 to a /64 by default, which groups a subnet into one + * rate-limit bucket but cannot identify a caller. Everything here identifies, so + * it stays exact — at the cost that results come back fully expanded + * (`2001:0db8:0000:...`), so anything comparing against operator-entered text + * must canonicalize that side too. + */ +const EXACT_IPV6_SUBNET = 128 + +/** Anything exposing header lookup by name — `Request.headers`, `Headers`, or a stub. */ +export interface ClientIpHeaders { + get(name: string): string | null +} + +export interface ResolveClientIpOptions { + /** + * Reverse-proxy hops trusted to have appended the forwarded chain, as IPs or + * CIDR ranges. Empty declares no proxy, so only a single-value header is + * trusted and any multi-hop chain resolves to `null`. + */ + trustedProxies?: string[] + /** Overrides {@link CLIENT_IP_HEADERS}. */ + headers?: readonly string[] +} + +/** + * Resolves the client IP of a request, or `null` when no trustworthy address + * can be established. + * + * The leftmost `x-forwarded-for` token is written by the client, so it is never + * read directly. With `trustedProxies` set the chain is walked right to left, + * known hops are skipped, and the first untrusted address is the client; a + * malformed hop fails closed. + * + * Delegates to Better Auth so this agrees by construction with the `ipAddress` + * on every session row — a second parser would be a second definition of "the + * client IP", and the two would drift. + * + * Callers must treat `null` as unknown, never as a match: a shared rate-limit + * bucket, or a denial for anything IP-gated. + */ +export function resolveClientIp( + request: { headers: ClientIpHeaders }, + options: ResolveClientIpOptions = {} +): string | null { + const { trustedProxies = [], headers = CLIENT_IP_HEADERS } = options + + for (const header of headers) { + const value = request.headers.get(header) + if (!value) continue + const ip = getIPFromHeader(value, { trustedProxies, ipv6Subnet: EXACT_IPV6_SUBNET }) + if (ip) return ip + } + + return null +} + +/** + * Bucket key segment standing in for a client IP that could not be trusted. + * Never a valid IP, so no client can steer itself into or out of it. + */ +export const UNRESOLVED_CLIENT_IP_BUCKET = 'unresolved' + +/** Splits an `AUTH_TRUSTED_PROXIES` value into entries. */ +export function parseTrustedProxies(value: string | null | undefined): string[] { + return (value ?? '') + .split(',') + .map((entry) => entry.trim()) + .filter(Boolean) +} + +/** + * Trusted-proxy entries that are not a valid IP or CIDR range. Malformed entries + * are dropped during resolution rather than throwing, so reporting them is the + * only way an operator learns a typo left the chain unverified. + */ +export function findMalformedTrustedProxies(entries: string[]): string[] { + return findInvalidTrustedProxies(entries) +} + +/** + * Whether an entry trusts every possible hop, which makes the walk skip every + * address and run off the end so no request resolves an IP. It fails closed, but + * silently — the symptom is indistinguishable from having no clients. + */ +export function isAllTrustingProxyEntry(entry: string): boolean { + const normalized = entry.trim() + return normalized === '0.0.0.0/0' || normalized === '::/0' +} diff --git a/packages/testing/package.json b/packages/testing/package.json index 826d9fb6920..43e3619ad54 100644 --- a/packages/testing/package.json +++ b/packages/testing/package.json @@ -57,6 +57,7 @@ "vitest": "^3.0.0 || >=4.1.0 <5.0.0" }, "devDependencies": { + "@sim/security": "workspace:*", "@sim/tsconfig": "workspace:*", "typescript": "^7.0.2", "vitest": "^4.1.0" diff --git a/packages/testing/src/mocks/request.mock.ts b/packages/testing/src/mocks/request.mock.ts index 614366ad938..6f1a8e8dfe2 100644 --- a/packages/testing/src/mocks/request.mock.ts +++ b/packages/testing/src/mocks/request.mock.ts @@ -1,6 +1,11 @@ /** * Mock request utilities for API testing */ +import { + type ClientIpHeaders, + resolveClientIp, + UNRESOLVED_CLIENT_IP_BUCKET, +} from '@sim/security/client-ip' import { NextRequest } from 'next/server' import { vi } from 'vitest' @@ -69,17 +74,27 @@ export function createMockFormDataRequest( /** * Controllable mock functions for `@/lib/core/utils/request`. * + * `generateRequestId` is stubbed for determinism. The IP helpers deliberately + * run the REAL resolver (with no trusted proxies, as a test environment + * declares none) so route tests see production's IP semantics — a stubbed + * passthrough here would be a second definition of "the client IP". Override + * per test when a specific address is needed. + * * @example * ```ts * import { requestUtilsMockFns } from '@sim/testing' * * requestUtilsMockFns.mockGenerateRequestId.mockReturnValueOnce('test-req-42') - * requestUtilsMockFns.mockGetClientIp.mockReturnValueOnce('10.0.0.5') + * requestUtilsMockFns.mockResolveClientIp.mockReturnValueOnce('10.0.0.5') * ``` */ export const requestUtilsMockFns = { mockGenerateRequestId: vi.fn(() => 'mock-request-id'), - mockGetClientIp: vi.fn(() => '127.0.0.1'), + mockResolveClientIp: vi.fn((request: { headers: ClientIpHeaders }) => resolveClientIp(request)), + mockGetRateLimitIpKey: vi.fn( + (request: { headers: ClientIpHeaders }) => + resolveClientIp(request) ?? UNRESOLVED_CLIENT_IP_BUCKET + ), } /** @@ -92,6 +107,7 @@ export const requestUtilsMockFns = { */ export const requestUtilsMock = { generateRequestId: requestUtilsMockFns.mockGenerateRequestId, - getClientIp: requestUtilsMockFns.mockGetClientIp, + resolveClientIp: requestUtilsMockFns.mockResolveClientIp, + getRateLimitIpKey: requestUtilsMockFns.mockGetRateLimitIpKey, noop: () => {}, }