fix(audit): resolve the client IP from the trusted proxy chain, not the leftmost header - #6466
fix(audit): resolve the client IP from the trusted proxy chain, not the leftmost header#6466waleedlatif1 wants to merge 1 commit into
Conversation
…t 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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview New Call sites across public APIs, rate-limit helpers, deployment auth, generic webhooks, audit logging, and Self-hosting docs, Helm, Compose, and Reviewed by Cursor Bugbot for commit d6d4180. Configure here. |
Greptile SummaryThe PR centralizes client-IP resolution around Better Auth’s trusted-proxy-chain logic and migrates rate limiting, audit logging, session recording, analytics, and webhook allowlists to the shared result.
Confidence Score: 4/5The IPv6 allowlist comparison should be corrected before merging because valid compressed addresses are rejected after resolver canonicalization. The shared resolver substantially improves proxy-chain handling, but the webhook authorization path compares its expanded IPv6 output against raw configured strings, causing legitimate allowlisted webhook requests to receive 403 responses. Files Needing Attention: apps/sim/lib/webhooks/providers/generic.ts, packages/security/src/client-ip.test.ts
|
| Filename | Overview |
|---|---|
| packages/security/src/client-ip.ts | Introduces shared trusted-proxy-chain resolution, header precedence, IPv6 canonicalization, and configuration helpers; canonical output requires downstream allowlist normalization. |
| apps/sim/lib/webhooks/providers/generic.ts | Makes webhook IP checks fail closed, but directly compares canonicalized IPv6 output against unnormalized stored entries. |
| apps/sim/lib/core/utils/request.ts | Wraps shared resolution with application proxy configuration and provides a stable unresolved rate-limit bucket. |
| apps/sim/lib/auth/auth.ts | Aligns Better Auth’s IP headers and trusted proxies with the shared resolver and adds startup diagnostics. |
| packages/audit/src/log.ts | Replaces the local leftmost-header parser with shared fail-closed resolution and records no IP when unresolved. |
| packages/security/src/client-ip.test.ts | Adds broad resolver coverage but violates the repository’s absolute-import convention. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Request[Incoming request] --> Headers[X-Forwarded-For / X-Real-IP]
Headers --> Resolver[Shared client-IP resolver]
Proxies[AUTH_TRUSTED_PROXIES] --> Resolver
Resolver -->|Resolved IP| Sessions[Session IP]
Resolver -->|Resolved IP| Audit[Audit IP]
Resolver -->|Resolved IP| Allowlist[Webhook allowlist]
Resolver -->|Resolved IP| RateLimit[Per-IP rate-limit bucket]
Resolver -->|null| SharedBucket[Shared unresolved bucket]
Resolver -->|null| BlankAudit[Blank audit/session IP]
Resolver -->|null| DenyWebhook[Deny allowlisted webhook]
Reviews (1): Last reviewed commit: "fix(security): resolve the client IP fro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const clientIp = resolveClientIp(request) | ||
|
|
||
| if (clientIp === 'unknown' || !allowedIps.includes(clientIp)) { | ||
| if (clientIp === null || !allowedIps.includes(clientIp)) { |
There was a problem hiding this comment.
When an allowlist contains a compressed IPv6 address such as 2001:db8::1, resolveClientIp returns its fully expanded form and this direct includes comparison treats the equivalent addresses as different, causing the legitimate webhook request to receive a 403.
| import { | ||
| findMalformedTrustedProxies, | ||
| isAllTrustingProxyEntry, | ||
| parseTrustedProxies, | ||
| resolveClientIp, | ||
| } from './client-ip' |
There was a problem hiding this comment.
The new test imports ./client-ip despite the repository-wide requirement to use absolute imports, introducing an inconsistent pattern that makes future module relocation more error-prone.
| import { | |
| findMalformedTrustedProxies, | |
| isAllTrustingProxyEntry, | |
| parseTrustedProxies, | |
| resolveClientIp, | |
| } from './client-ip' | |
| import { | |
| findMalformedTrustedProxies, | |
| isAllTrustingProxyEntry, | |
| parseTrustedProxies, | |
| resolveClientIp, | |
| } from '@sim/security/client-ip' |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d6d4180. Configure here.
|
|
||
| try { | ||
| const ip = getClientIp(req) | ||
| const ip = getRateLimitIpKey(req) |
There was a problem hiding this comment.
Turnstile gets unresolved remote IP
Medium Severity
getRateLimitIpKey can return the sentinel unresolved, and that value is passed straight into Turnstile as remoteIp. The Turnstile helper only skips the old unknown sentinel, so Cloudflare may receive remoteip=unresolved and reject otherwise-valid captchas whenever no trustworthy client IP resolves.
Reviewed by Cursor Bugbot for commit d6d4180. Configure here.
| const clientIp = resolveClientIp(request) | ||
|
|
||
| if (clientIp === 'unknown' || !allowedIps.includes(clientIp)) { | ||
| if (clientIp === null || !allowedIps.includes(clientIp)) { |
There was a problem hiding this comment.
IPv6 allowlist match fails
Medium Severity
Webhook IP allowlisting still uses exact includes against the resolved address, but resolveClientIp now returns fully expanded IPv6. Operator-entered compressed forms no longer match, so legitimate IPv6 webhook sources can be rejected with 403.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d6d4180. Configure here.
| 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], |
There was a problem hiding this comment.
Session IPv6 diverges from resolver
Medium Severity
resolveClientIp forces ipv6Subnet: 128, but Better Auth’s advanced.ipAddress is configured without ipv6Subnet, so it keeps the library default of /64. Session-recorded IPv6 addresses therefore will not match the IP every other caller resolves, despite the shared-header/proxy setup.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d6d4180. Configure here.
| mockGetRateLimitIpKey: vi.fn( | ||
| (request: { headers: ClientIpHeaders }) => | ||
| resolveClientIp(request) ?? UNRESOLVED_CLIENT_IP_BUCKET | ||
| ), |
There was a problem hiding this comment.
Rate-limit mock wired incorrectly
Low Severity
mockGetRateLimitIpKey calls the real resolveClientIp instead of mockResolveClientIp, while migrated route tests still stub only mockResolveClientIp. Routes that now key limits with getRateLimitIpKey ignore those stubs, so IP-related assertions can pass for the wrong reason.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d6d4180. Configure here.
waleedlatif1
changed the title
1 participant
Summary
getClientIpread the leftmostx-forwarded-fortoken, which is written by the client. Every IP-derived decision was therefore steerable by sending your own header: per-IP rate limits could be reset on every request, audit rows recorded an attacker-chosen address, and per-webhook IP allowlists could be satisfied outright.@sim/security/client-ip, which walks the chain right to left againstAUTH_TRUSTED_PROXIESand returnsnullwhen no address can be trusted. It delegates to Better Auth's own resolver (@better-auth/core/utils/ip) rather than reimplementing the parse, so it agrees by construction with theipAddressrecorded on every session row.apps/sim/lib/core/utils/request.ts,packages/audit/src/log.ts) and migrated all 14 call sites. Renamed toresolveClientIpso every site had to be revisited — the return type wentstring→string | null, and a template-literal rate-limit key would have silently stringifiednull.nulldeliberately: rate limits share one strict bucket viagetRateLimitIpKey, the webhook allowlist denies, audit records blank.ipAddressHeadersto Better Auth from the same shared constant. Its default isx-forwarded-foralone, so anx-real-ip-only ingress previously resolved an IP for our code but recorded none on the session.AUTH_TRUSTED_PROXIEScontains a malformed entry or an all-trusting range (0.0.0.0/0) — both silently leave every request resolving no IP..env.exampleupdated: the variable now governs sessions, per-IP rate limits, audit log IPs, and webhook IP allowlists.Behavior change worth reviewing
Behind a proxy with
AUTH_TRUSTED_PROXIESunset, no client IP resolves. Nothing breaks outright — rate limits fall back to one shared bucket per endpoint (stricter, not weaker), audit rows blank the IP, and any webhook with an IP allowlist rejects deliveries. Sim will not fall back to the client-supplied value. Prod sets the variable; this mainly affects self-hosters, and the docs now call it out.No new dependencies (
@better-auth/coreis already transitive viabetter-auth), no migration, no feature flag.Type of Change
Testing
apps/simsuite green (21,046 passed), plus@sim/security,@sim/audit,@sim/realtime. All 23 workspaces type-check.check:api-validation:strict,check:boundaries,check:realtime-prune,check:import-specifiers,check:native-typecheckall pass.Checklist