fix(rate-limit): key limiter on real client IP behind Cloudflare (#47) - #48
Merged
Conversation
slowapi's get_remote_address keys on request.client.host, which behind Cloudflare is the shared CF edge IP. That bucketed the whole audience into a few buckets: false login lockouts for legit users under load, and no real per-attacker brute-force limit on POST /auth/jwt/login. Add app/limiting.client_ip, which trusts CF-Connecting-IP (then the left-most X-Forwarded-For hop) only when the connecting peer is itself within Cloudflare's published IP ranges; any other peer falls back to the peer address. This stops a direct caller to the Cloud Run origin from spoofing the header to dodge the login limiter. Wire it into both the Limiter key_func and the rate_limit_auth middleware.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The slowapi limiter keyed on
get_remote_address=request.client.host, which behind Cloudflare (auth.criticalbit.ggis in the CF-proxied zone) is the CF edge IP, shared by every client routed through that edge. So the limiter bucketed the whole audience behind a handful of edge IPs instead of per real client.On the
POST /auth/jwt/loginlimiter (app/main.py) this is doubly bad: keyed on the edge IP it (a) causes false lockouts for legit users sharing a CF edge under load, and (b) doesn't actually limit per-attacker — so the brute-force protection isn't doing its job. Same root cause as the 429 storm inaoe2-live-standings-api#176.Fix
New
app/limiting.pywith aclient_ipkey func that reads the real client IP fromCF-Connecting-IP(then the left-mostX-Forwarded-Forhop) — but only when the connecting peer is itself within Cloudflare's published IP ranges. Any other peer (a direct hit to the Cloud Run origin, a spoof attempt, a local test client) falls back to the peer address.That peer check is the load-bearing difference from blanket header-trust: the
*.run.apporigin is publicly reachable (no ingress lock indeploy.yml), so blindly trustingCF-Connecting-IPwould let a direct caller send a fresh value per request and walk straight past the login limiter — worse than the edge-IP bug. Gating on the peer being a real CF edge closes that by construction.Wired into both the
Limiterkey func and therate_limit_authmiddleware.Test plan
uv run pytest— 148 passed, 1 skippeduv run ruff check .+ruff format --check— cleantests/test_client_ip.py(18 tests):CF-Connecting-IP/ falls back to the left-most XFF hop / returns peer with no headers; IPv6 CF peer; unparseable peer doesn't raise.CF-Connecting-IP/X-Forwarded-Foris ignored — keyed on the real peer.Notes
max_connections/ stale-revision items the issue also mentions are out of scope here; can file a separate follow-up.Closes #47