Skip to content

fix: resolve the trusted client IP behind the load balancer - #4033

Merged
rebelchris merged 3 commits into
mainfrom
fix-client-ip-trust
Aug 6, 2026
Merged

fix: resolve the trusted client IP behind the load balancer#4033
rebelchris merged 3 commits into
mainfrom
fix-client-ip-trust

Conversation

@rebelchris

@rebelchris rebelchris commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Context

A security report claimed daily.dev's password reset endpoint has no rate limiting, and that 100 reset emails could be sent to a user via Burp Intruder.

The literal claim is wrong — better-auth 1.5.0 applies a built-in rule of 3 requests / 60s to /request-password-reset, it's enabled in prod (nodeEnv: production), and it's Redis-backed so it's shared across pods. But while verifying that, I found the limit doesn't actually bind, for a reason that turned out to be much broader than password reset.

The bug

src/index.ts configured Fastify with trustProxy: true. That makes request.ip resolve to the leftmost x-forwarded-for entry — which is the one the client sends, not the one the load balancer vouches for.

The intuitive assumption is that a proxy strips a client-supplied X-Forwarded-For. GCP does not do this by default; it appends:

X-Forwarded-For: <client-supplied>,<client-ip>,<load-balancer-ip>

The load balancer does not verify any IP addresses that precede <client-ip>,<load-balancer-ip> in this header.
External Application Load Balancer overview

Stripping is opt-in, via a backend-service custom request header (--custom-request-header=x-forwarded-for:{client_ip_address},{server_ip_address}). We have not configured that — there is no custom-request-header, BackendConfig, or Cloud Armor policy anywhere in daily-infra, and no Cloudflare in front of api.daily.dev.

Confirmed against production

/boot derives geo.region from request.ip, so this is directly observable today:

Request to https://api.daily.dev/boot geo.region
no header ZA (real egress IP)
X-Forwarded-For: 133.11.0.1 JP
X-Forwarded-For: 200.160.2.3 BR

request.ip is whatever the caller claims.

Password reset was the least of it. Everything keyed on request.ip is affected:

Consumer Impact
routes/webhooks/paddle IP allowlist bypassable (signature check still holds, so defense-in-depth only)
routes/webhooks/apple Same — JWS verification still holds
common/vordr.ts Anti-abuse IP checks bypassable
routes/public, routes/outbound, directive/rateLimit.ts Rate limit keys rotatable at will
Context.ts, entity/user/utils.ts, schema/users.ts, routes/boot.ts Geo → coresRole spoofable, i.e. self-selected pricing region at signup

Sign-in throttling (3/10s) was bypassable the same way, which matters more than the reported email flood.

The fix

trustProxy: 2 — trust the two hops the load balancer guarantees and discard the client-supplied prefix. One setting, fixes every consumer above.

The hop count of 2 assumes nothing appends to x-forwarded-for after the load balancer. Verified: the ingress is a plain VPC-native GKE Ingress (container-native LB, GFE → pod directly), with no service mesh, no Envoy/nginx sidecar, and no init container in the api deployment. So the address list is exactly [GFE, lb-vip, client, ...client-supplied] and trusting 2 hops lands on the real client.

better-auth parses x-forwarded-for itself and takes the first entry, with no trusted-proxy support (trustedProxies exists only in a warning string in 1.5.0). Since callBetterAuth is the sole entry point into its handler, the header is collapsed there to the resolved IP.

Verification

  • New test asserts a client-supplied prefix is discarded, using the real LB header shape (<spoofed>,<client>,<lb>).
  • Existing suites pass unchanged: routes/betterAuth (31), webhooks/apple, webhooks/paddle, boot, outbound (164 total), plus all 19 routes/public + routes/private suites. Tests that set a single-entry x-forwarded-for are unaffected by the hop count.
  • pnpm run build and pnpm run lint clean.

Post-deploy check — this should stop tracking the spoofed header and report the real egress region instead of JP:

curl -s https://api.daily.dev/boot -H 'X-Forwarded-For: 133.11.0.1' | jq .geo

If it instead returns an empty/unexpected region for every caller, the hop count is off by one and request.ip is resolving to the load balancer VIP — revert and switch to trusting Google's LB ranges plus the ingress VIP by CIDR.

Deliberately not included

  • Per-recipient limit on reset. The key is still (ip, path), so a distributed IP pool can still flood one inbox. rateLimit.customRules can't change the key, so this needs a small Redis counter in the existing hooks.before middleware.
  • Turnstile on reset. Already integrated but scoped to ['/sign-up/email', '/sign-in/email']. Adding reset requires a matching apps change to send the token, so it can't ship from this repo alone.

Unrelated, worth a separate look

Kratos is still deployed in heimdall and still serves /self-service/* at sso.daily.dev via a catch-all ingress rule, with recovery enabled in every checked-in config — a second reset-email surface, rate-limited by Cloud Armor at 30/60s. heimdall/services/ba_user_creator also mirrors better-auth users into Kratos with their password hashes, so a Kratos-side recovery could set a credential better-auth doesn't honor. Confirming prod requires reading the live kratos-config secret, which is gitignored.


Replaces #4032, which was accidentally branched off feat-squad-privileged-rate-limit and so carried that PR's commits in its diff.

🤖 Generated with Claude Code

Fastify was configured with `trustProxy: true`, which resolves `request.ip`
to the leftmost x-forwarded-for entry. The GCP load balancer appends
`<client-ip>,<load-balancer-ip>` to whatever the client sent and explicitly
does not validate the preceding values, so that entry is attacker-controlled.

Trust 2 hops instead, which discards the client-supplied prefix. This fixes
every consumer of `request.ip`: the Paddle and Apple webhook IP allowlists,
Vordr, the REST and GraphQL rate limiters, and geo/coresRole resolution.

BetterAuth parses x-forwarded-for itself and takes the first entry, so
collapse the header to the resolved IP before handing the request over. This
makes its built-in 3-per-60s limit on /request-password-reset actually bind;
previously it could be bypassed by varying the header per request.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pulumi

pulumi Bot commented Jul 30, 2026

Copy link
Copy Markdown

🍹 The Update (preview) for dailydotdev/api/prod (at 7320896) was successful.

Resource Changes

    Name                                                       Type                           Operation
~   vpc-native-clean-gifted-plus-cron                          kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-opportunities-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-rotate-weekly-quests-cron                       kubernetes:batch/v1:CronJob    update
-   vpc-native-api-db-migration-35a35304                       kubernetes:batch/v1:Job        delete
~   vpc-native-private-deployment                              kubernetes:apps/v1:Deployment  update
~   vpc-native-update-achievement-rarity-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-updated-sync-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-channel-digests-cron                            kubernetes:batch/v1:CronJob    update
-   vpc-native-api-clickhouse-migration-35a35304               kubernetes:batch/v1:Job        delete
~   vpc-native-clean-zombie-users-cron                         kubernetes:batch/v1:CronJob    update
~   vpc-native-personalized-digest-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-subscription-anniversary-achievements-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-worker-job-deployment                           kubernetes:apps/v1:Deployment  update
~   vpc-native-update-highlighted-views-cron                   kubernetes:batch/v1:CronJob    update
~   vpc-native-post-analytics-achievements-cron                kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-analytics-clickhouse-cron          kubernetes:batch/v1:CronJob    update
~   vpc-native-user-world-clickhouse-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-daily-digest-cron                               kubernetes:batch/v1:CronJob    update
~   vpc-native-materialize-yearly-best-post-archives-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-interest-scheduled-run-cron                     kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-expired-better-auth-sessions-cron         kubernetes:batch/v1:CronJob    update
~   vpc-native-update-views-cron                               kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-old-notifications-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-post-lifecycle-state-clickhouse-cron            kubernetes:batch/v1:CronJob    update
~   vpc-native-validate-active-users-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-check-analytics-report-cron                     kubernetes:batch/v1:CronJob    update
~   vpc-native-update-source-public-threshold-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-images-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-ws-deployment                                   kubernetes:apps/v1:Deployment  update
~   vpc-native-rotate-daily-quests-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-channel-highlights-cron                   kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-stale-user-transactions-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-temporal-deployment                             kubernetes:apps/v1:Deployment  update
~   vpc-native-post-analytics-clickhouse-cron                  kubernetes:batch/v1:CronJob    update
~   vpc-native-hourly-notification-cron                        kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tags-str-cron                            kubernetes:batch/v1:CronJob    update
~   vpc-native-user-profile-analytics-history-clickhouse-cron  kubernetes:batch/v1:CronJob    update
~   vpc-native-sync-subscription-with-cio-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-update-current-streak-cron                      kubernetes:batch/v1:CronJob    update
~   vpc-native-user-posts-analytics-refresh-cron               kubernetes:batch/v1:CronJob    update
+   vpc-native-api-db-migration-a146f858                       kubernetes:batch/v1:Job        create
... and 17 other changes

Comment thread src/index.ts
// client-supplied x-forwarded-for and does not validate what precedes them.
// Trusting 2 hops discards the client-supplied prefix, so `request.ip`
// cannot be spoofed. `true` would resolve to the leftmost (attacker) entry.
trustProxy: 2,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you validated, gcp should strip x-forwarded-for from client, thats the whole point of proxy

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated — and against prod rather than just the docs, because you're right that stripping is what you'd expect a proxy to do. GCP just doesn't do it by default.

It appends; stripping is opt-in.

If the incoming request already includes an X-Forwarded-For header, the load balancer appends its values to the existing header: X-Forwarded-For: <existing-value>,<client-ip>,<load-balancer-ip>

The load balancer does not verify any IP addresses that precede <client-ip>,<load-balancer-ip> in this header.

External Application Load Balancer overview

To get the strip behaviour you have to ask for it explicitly on the backend service, via --custom-request-header=x-forwarded-for:{client_ip_address},{server_ip_address}. We haven't: there's no custom-request-header, no BackendConfig, and no Cloud Armor policy anywhere in daily-infra, and nothing else (Cloudflare etc.) sits in front of api.daily.dev. Append is also what AWS ALB and Cloudflare do — the header's purpose is to preserve the proxy chain, so dropping it would break legitimate multi-hop setups.

Confirmed live on prod. /boot derives geo.region from request.ip, so it's directly observable:

curl https://api.daily.dev/boot geo.region
no header ZA (real egress)
-H 'X-Forwarded-For: 133.11.0.1' JP
-H 'X-Forwarded-For: 200.160.2.3' BR

So request.ip is caller-controlled in prod today.

Your question did catch something, though. The hop count of 2 is only correct if nothing appends to the header after the LB — the same docs note that backend reverse-proxy software can append ,<GFE-ip>,<backend-ip>, which would put us off by one. That failure mode is worse than the bug: request.ip would resolve to the LB VIP for everyone, collapsing all traffic into a single shared rate-limit bucket and flattening geo.

Checked it: plain VPC-native GKE Ingress with container-native load balancing, so the GFE talks to the pod directly — no service mesh, no Envoy/nginx sidecar, no init container in the api deployment. The address list is therefore [GFE, lb-vip, client, ...client-supplied] and trusting 2 hops lands on the real client. I've added that reasoning to the PR description.

Sanity check once this deploys — it should stop echoing the spoofed header and report the real egress region:

curl -s https://api.daily.dev/boot -H 'X-Forwarded-For: 133.11.0.1' | jq .geo

If it instead returns the same region for every caller, the hop count is off and we should switch to trusting Google's LB ranges plus the ingress VIP by CIDR instead of by count.

@capJavert capJavert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't want added cost this is a solution for API 👍

@rebelchris
rebelchris enabled auto-merge (squash) August 6, 2026 09:32
@rebelchris
rebelchris merged commit 933625e into main Aug 6, 2026
9 checks passed
@rebelchris
rebelchris deleted the fix-client-ip-trust branch August 6, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants