fix: resolve client IP from trusted proxy hops, not uvicorn's wildcard - #1
Merged
Merged
Conversation
The image ran uvicorn with `--forwarded-allow-ips "*"`, on the reasoning that Railway/Render/Fly always have their proxy in front so trusting it is safe. The flag doesn't mean that. With `"*"`, uvicorn rewrites request.client from the LEFTMOST X-Forwarded-For entry -- the end of the chain furthest from the proxy, written by whoever sent the request. Since client_ip() keys on request.client.host, a different random address per request bought a fresh budget on every limiter in the app: login's 5/min brute-force protection, register, password reset, /translate, and the app-wide backstop. It also made the `ip=` field in every security log line attacker-authored, and -- because RateLimiter never evicts keys -- let a single caller grow the limiter's dict without bound. The header is now read by the app and counted from the RIGHT: each proxy appends the address of its own immediate peer, so with TRUSTED_PROXY_HOPS proxies in front, the real client is that many entries from the end and everything to its left is ignored. Entries must parse as IP addresses; an unparseable one, or a chain shorter than the configured hop count, falls back to the socket peer, which over-limits rather than under-limits. Parsed addresses are normalized, so respelling an IPv6 address doesn't buy a second budget. TRUSTED_PROXY_HOPS defaults to 0 (no proxy -- correct for local dev and docker compose, where the browser reaches the backend directly). BEHAVIOUR CHANGE: deployments behind a proxy must set TRUSTED_PROXY_HOPS=1 or rate limiting will count every visitor as one client. DEPLOYMENT.md explains how to count, which direction is safe to get wrong, and how to verify both failure modes after deploying -- the old guide asserted the Dockerfile "handles this", which was exactly the misreading that caused the bug. The uvicorn flags are gone from the image; nothing else needed them, since this app reads request.url.path only and builds no absolute URLs. Tests, and their honest limit: the resolution logic is pinned directly (which entry becomes the key, ports stripped, IPv6 normalized, both fallbacks), plus end-to-end checks that an unconfigured deployment ignores the header. Those end-to-end tests cannot reproduce the original bypass -- uvicorn's ProxyHeadersMiddleware is installed by the server, not by app.main:app, so TestClient never runs it and they would have passed before this fix too. The flag is guarded where it actually lives: a contract test that fails if any --forwarded-allow-ips returns to the Dockerfile CMD, and another that fails if DEPLOYMENT.md stops documenting the setting. 180 passed (was 168). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
The image ran uvicorn with
--forwarded-allow-ips "*", on the reasoning that Railway/Render/Fly always have their proxy in front so trusting it is safe. The flag doesn't mean that. With"*", uvicorn rewrites request.client from the LEFTMOST X-Forwarded-For entry -- the end of the chain furthest from the proxy, written by whoever sent the request.Since client_ip() keys on request.client.host, a different random address per request bought a fresh budget on every limiter in the app: login's 5/min brute-force protection, register, password reset, /translate, and the app-wide backstop. It also made the
ip=field in every security log line attacker-authored, and -- because RateLimiter never evicts keys -- let a single caller grow the limiter's dict without bound.The header is now read by the app and counted from the RIGHT: each proxy appends the address of its own immediate peer, so with TRUSTED_PROXY_HOPS proxies in front, the real client is that many entries from the end and everything to its left is ignored. Entries must parse as IP addresses; an unparseable one, or a chain shorter than the configured hop count, falls back to the socket peer, which over-limits rather than under-limits. Parsed addresses are normalized, so respelling an IPv6 address doesn't buy a second budget.
TRUSTED_PROXY_HOPS defaults to 0 (no proxy -- correct for local dev and docker compose, where the browser reaches the backend directly).
BEHAVIOUR CHANGE: deployments behind a proxy must set TRUSTED_PROXY_HOPS=1 or rate limiting will count every visitor as one client. DEPLOYMENT.md explains how to count, which direction is safe to get wrong, and how to verify both failure modes after deploying -- the old guide asserted the Dockerfile "handles this", which was exactly the misreading that caused the bug.
The uvicorn flags are gone from the image; nothing else needed them, since this app reads request.url.path only and builds no absolute URLs.
Tests, and their honest limit: the resolution logic is pinned directly (which entry becomes the key, ports stripped, IPv6 normalized, both fallbacks), plus end-to-end checks that an unconfigured deployment ignores the header. Those end-to-end tests cannot reproduce the original bypass -- uvicorn's ProxyHeadersMiddleware is installed by the server, not by app.main:app, so TestClient never runs it and they would have passed before this fix too. The flag is guarded where it actually lives: a contract test that fails if any --forwarded-allow-ips returns to the Dockerfile CMD, and another that fails if DEPLOYMENT.md stops documenting the setting.
180 passed (was 168).