fix(security): trusted-proxy-aware client IP resolution for rate limiting - #49
Conversation
…ting Both rate limiters derived "client IP" incorrectly: - login_rate_limit.py trusted a client-supplied X-Forwarded-For header unconditionally, with no trusted-proxy allowlist. Any client could reset its own login-brute-force bucket on every request just by sending a fresh XFF value, defeating the 10-attempts/15-minute limit entirely on any deployment where the header isn't stripped/overwritten at the edge. - rate_limit.py did the opposite: it ignored X-Forwarded-For entirely and always used request.client.host, which -- once actually behind a reverse proxy/load balancer in production -- is the proxy's own IP for every request, collapsing the general 120 req/min limit into one shared bucket for every real client. New app/core/client_ip.py::get_client_ip() is now shared by both: - Only honors X-Forwarded-For when the immediate TCP peer is in the new TRUSTED_PROXY_CIDRS setting (comma-separated CIDRs, empty by default -- so XFF is never trusted out of the box). - Walks the header right-to-left and returns the first hop that isn't itself a trusted proxy, so an attacker can't defeat this by prepending a fake IP before the real proxy's hop. Added tests/test_client_ip.py covering: XFF ignored with no trusted proxies configured, XFF ignored from an untrusted peer, XFF honored from a trusted peer, correct hop selection through chained trusted proxies, and that a spoofed prefix hop from an attacker connecting directly to a trusted proxy is still correctly bypassed in favor of their real IP. Verified against a live docker-compose stack: with uvicorn's own loopback proxy-trust explicitly disabled (--forwarded-allow-ips=""), hammering /api/auth/login with a different spoofed X-Forwarded-For on every request no longer resets the bucket -- the 11th attempt within the window correctly returns 429, and Redis shows a single login_rl:127.0.0.1 key rather than 12 separate spoofed-IP keys. Fixes #26
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Both rate limiters derived "client IP" incorrectly, in opposite ways:
login_rate_limit.pytrusted a client-suppliedX-Forwarded-Forheader unconditionally, with no trusted-proxy allowlist. Any client could reset its own login-brute-force bucket on every request just by sending a freshX-Forwarded-Forvalue, defeating the 10-attempts/15-minute limit entirely on any deployment where the header isn't stripped/overwritten at the edge (common for a self-hosted app that may be exposed directly).rate_limit.pydid the opposite: it ignoredX-Forwarded-Forentirely and always usedrequest.client.host, which -- once actually behind a reverse proxy/load balancer in production -- is the proxy's own IP for every request, collapsing the general 120 req/min limit into a single shared bucket for every real client.Changes
app/core/client_ip.py::get_client_ip(), shared by both limiters:X-Forwarded-Forwhen the immediate TCP peer is in the newTRUSTED_PROXY_CIDRSsetting (comma-separated CIDRs, empty by default -- so XFF is never trusted out of the box).login_rate_limit.pyandrate_limit.pyboth now callget_client_ip(request)instead of their own divergent logic.Test plan
api/app/tests/test_client_ip.py(6 tests, all deterministic, no network/Redis needed): XFF ignored with no trusted proxies configured; XFF ignored from an untrusted peer; XFF honored from a trusted peer; correct hop selection through chained trusted proxies; a spoofed prefix hop from an attacker connecting directly to a trusted proxy is still correctly bypassed in favor of their real IP; no-XFF-header fallback.ruff check .,mypy .,pytest-- all clean (45/45 backend tests)--forwarded-allow-ips="", matching a real deployment topology where the app isn't directly behind a trusted proxy), hammering/api/auth/loginwith a different spoofedX-Forwarded-Foron every request no longer resets the bucket -- the 11th attempt within the window correctly returns429, and Redis shows a singlelogin_rl:127.0.0.1key rather than 12 separate spoofed-IP keys.Note for reviewers: by default (
TRUSTED_PROXY_CIDRSempty),X-Forwarded-Foris never trusted, which is safe but means rate limiting always uses the direct TCP peer. Operators running behind a real reverse proxy/load balancer need to setTRUSTED_PROXY_CIDRSto that proxy's address/CIDR for rate limiting to see real client IPs instead of the proxy's.Fixes #26