fix(security): key per-IP rate limits on the proxy-written forwarded hop - #6171
fix(security): key per-IP rate limits on the proxy-written forwarded hop#6171waleedlatif1 wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Shared The app, docs site, and Reviewed by Cursor Bugbot for commit c0f6ace. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit cc2cef5. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c3074b3. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c3074b3. Configure here.
|
@greptile review |
Greptile SummaryThe PR centralizes forwarded-client-IP resolution and updates per-IP throttles and audit logging to use proxy-aware, canonicalized identities. The follow-up changes also make forwarded-header trust explicit for directly exposed Compose and Helm deployments.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported direct-exposure paths now disable forwarded-header trust by default, and affected rate-limit consumers use the gated resolver.
|
| Filename | Overview |
|---|---|
| packages/security/src/client-ip.ts | Introduces the shared proxy-aware resolver with canonicalization, trusted-hop traversal, and IPv6 rate-limit key normalization. |
| apps/sim/lib/core/utils/client-ip.ts | Gates application client-IP resolution on the deployment’s forwarded-header trust setting. |
| docker-compose.prod.yml | Defaults directly published production deployments to distrust caller-controlled forwarding headers. |
| helm/sim/templates/_helpers.tpl | Derives forwarded-header trust from ingress enablement while correctly preserving explicit string or boolean overrides. |
| helm/sim/templates/deployment-app.yaml | Injects the chart-computed trust setting directly into the app deployment without Secret override ambiguity. |
| apps/sim/lib/core/security/deployment-auth.ts | Adds a fail-closed deployment-scoped ceiling for consecutive failed password attempts and resets it after successful verification. |
| packages/audit/src/log.ts | Replaces duplicated IP extraction with the shared resolver and respects the forwarded-header trust setting. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Req[Incoming request] --> Trust{TRUST_PROXY_HEADERS?}
Trust -->|false| Unknown[Shared unknown identity]
Trust -->|true| Chain[Parse forwarded chain]
Chain --> Walk[Walk right to left past trusted proxies]
Walk --> Canon[Canonicalize address]
Canon --> Mask[Mask IPv6 identity to /64]
Mask --> Key[Per-client rate-limit key]
Unknown --> Shared[Shared fail-closed rate-limit bucket]
Reviews (6): Last reviewed commit: "fix(helm): treat TRUST_PROXY_HEADERS as ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
c3074b3 to
bb6950b
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bb6950b. Configure here.
bb6950b to
ccb4e57
Compare
|
Addressed the 4/5 blocker — the direct-deployment bypass. You're right that walking the chain right to left is worthless if nothing appends to it. No parsing rule can recover a real address from a header nobody vouched for, so it's now explicit rather than assumed:
|
|
@cursor review |
ccb4e57 to
9f0cd26
Compare
|
@cursor review |
9f0cd26 to
c695655
Compare
|
@cursor review |
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.
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.
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
…them 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.
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.
… password ceiling closed 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.
…ments 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.
c695655 to
c0f6ace
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c0f6ace. Configure here.
1 participant
Summary
getClientIpread the leftmostX-Forwarded-Forentry. Under any proxy that appends to that header — nginx-ingress, HAProxy, Cloudflare, and both reference deployments in this repo — that entry is caller-supplied, so rotating it minted a fresh token bucket per request and every per-IP throttle became a no-op.Affected: contact + demo-request mailers (unmetered outbound email from our sending domain, to an attacker-supplied address with an attacker-supplied subject), help integration requests, telemetry, the docs Ask-AI endpoint (unmetered LLM spend), and public-deployment password brute force.
We already treated that hop as untrusted for Better Auth via
AUTH_TRUSTED_PROXIES— Sim's own helper just never consulted it.packages/auditcarried a second copy of the buggy function (forging audit-row IPs) andapps/docsa third. All three now share@sim/security/client-ip, which walks the chain right to left, skips configured trusted hops, and returns the first untrusted address.getClientIpalso moved out oflib/core/utils/request.ts(a client component imports that module fornoop) into a server-onlylib/core/utils/client-ip.ts.Walking right-to-left is necessary but not sufficient
The bug is "one caller, many buckets" — the header is only one way to get there. Three more had to close before this actually holds:
IPv6. A single client is delegated a whole /64 — the standard residential and cloud allocation — so it can legitimately source every request from a different address, with no spoofing at all: the proxy writes the varying value itself and nothing looks wrong. Keys are now masked to the routed /64, matching Better Auth's
ipv6Subnetdefault. Masking happens only where a key is produced, never before the trusted-proxy comparison (which needs the full address).Broad trusted ranges. Forging an address inside a configured range —
10.0.0.0/16, which our own docs recommended — makes the whole chain trusted, and the all-trusted fallback then handed the caller their own forged value back. It falls back to the rightmost hop now, the one entry a caller can never author. The docs no longer recommend a range that covers client traffic.No proxy at all. Every rule about which hop to read presumes something appended one.
docker-compose.prod.ymlpublishes3000:3000and ships no proxy, and the Helm chart defaultsingress.enabled=false— so on both shipped artifacts the whole header was caller-authored and per-IP limits stayed bypassable regardless.TRUST_PROXY_HEADERSnow states that deployment fact explicitly: false makesgetClientIpdecline to guess and per-IP limits collapse to one shared bucket — blunt, but it fails closed. Compose defaults it false; the chart derives it fromingress.enabled; the hosted/ingress path is unchanged.Other hardening
ipaddr.isValid('fe80::1%<200 chars>')istrueandprocess()keeps the zone verbatim, an unbounded supply of distinct keys and arbitrary attacker text in Redis keys and audit rows.::ffff:1.2.3.4,0xc6336404, and1.2.3.4:portshare one bucket.::ffff:10.0.0.0/104), silently inert on a kind mismatch.failClosed— it is the only bound on guessing at the secret, so failing open removed it during exactly the outage an attacker could wait for.getIPFromHeaderreturns null for any multi-value header). One of my own edits had replaced an accurate sentence inenv.tswith an inaccurate one.Deliberately unchanged
The generic webhook
allowedIpscheck names the sending service (Stripe, GitHub), not the proxy, so resolving it like a throttle key would have 403'd every allowlisted delivery whereverAUTH_TRUSTED_PROXIESis unset. It usesgetAssertedOriginIp— leftmost, unmasked, explicitly documented as caller-asserted and never valid as a throttle key — with both sides canonicalized and theX-Real-IPfallback the old helper had.Deployment note
Set
AUTH_TRUSTED_PROXIESto the ingress pods' actual addresses — not a broad private range that also covers client traffic. Unset is safe but coarse: behind a multi-hop chain it collapses callers onto the edge address, turning several per-IP limits into global ones, notably the contact form's captcha-unavailable bucket (3/min) andstt-token(3 per 72s per chat).Type of Change
Testing
packages/audit,apps/sim/app/api/chat, and a new test for theenv → parseTrustedProxieswiring, which was globally mocked and therefore never executed in CI.bits - 96(1), the kind guard (2), the walk direction (4), trusted-proxy pass-through (1), the trust gate (1),failClosed(2), and defaulting trust off (2) each turn their own tests red.route-helpers.test.ts, whose mock reimplemented the old leftmost-first logic and asserted the vulnerable behavior as correct; de-vacuumed the IPv6/IPv4 kind-mismatch test, which passed only because deleting the guard madematchthrow.$chartComputedentry reproduces the ESO validation failure.cloud-review-tools) is a local env issue —rgis a shell function here, not a binary — and reproduces on cleanstaging.tscclean acrossapps/sim,apps/docs, and touched packages;helm lintpasses,values.schema.jsonvalid.check:api-validation,check:boundaries,check:client-boundary,check:realtime-pruneall pass.Checklist