fix(rate-limit): key on the real client behind a proxy, not the proxy - #25
Merged
Merged
Conversation
Production sets no TRUSTED_PROXY_IP and sits behind a cloudflared container, so every request arrived from one peer address and the whole user base shared a single rate-limit bucket: ten auth requests a minute between all users, and twenty registrations a day globally. Today's traffic is a fraction of that, so it has been invisible; a login wave or eleven clients refreshing in the same minute would 429 real people, and the registration cap is a growth ceiling. Setting the old variable would not have fixed it. It accepted a single address, and the address that needs trusting belongs to a container, which takes a new one on every recreate. TRUSTED_PROXIES accepts a list of addresses or CIDR ranges, so the proxy's network can be named once. Trusting a proxy also has to stop being trivially bypassable. The header was read left to right, and proxies append, so a caller could prepend an address of its own and mint a fresh bucket per request. The client is now taken from the rightmost entry that is not itself a trusted proxy, which also handles a chain of proxies. The auth limit is configurable as AUTH_RATE_LIMIT, matching the other limiters, and the trusted set is parsed once at startup rather than read from the environment on every request. compose.prod.yml now refuses to start without TRUSTED_PROXIES, since the failure it prevents is silent.
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
1 participant
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.
Found while load testing. Production sets no
TRUSTED_PROXY_IPand sits behind acloudflaredcontainer on the same docker network — tunnel172.22.0.5, server172.22.0.8.extract_iptherefore falls back to the peer address, correctly refusing to trustX-Forwarded-For, and every user's request keys on the same address.What that caps, across all users combined:
/v1/auth/{challenge,login,refresh,verify-email}and/v1/invitations/:token)REGISTER_RATE_LIMIT=20per dayCurrent traffic hides it —
/v1/auth/refreshran 60 times in three hours, about 0.33/min. A login wave, a refresh burst after a restart, or eleven clients in one minute starts returning 429 to real people, and the registration cap is a growth ceiling right now.Why setting the old variable was not the fix
TRUSTED_PROXY_IPtook one address, and the address to trust belongs to a container that gets a new one on every recreate.TRUSTED_PROXIEStakes a comma-separated list of addresses or CIDR ranges, so the proxy's network is named once (172.22.0.0/16here). The old name still works.Trusting a proxy had to stop being bypassable
The header was read left to right. Proxies append, so with trust enabled a caller could prepend an address of its own and get a fresh bucket on every request — trading a global cap for no cap at all. The client address is now the rightmost entry that is not itself a trusted proxy, which also handles a chain of proxies correctly.
Verified by test, and end to end before writing any of this: 12 requests to
/v1/auth/challengefrom an isolated stack, each with a differentX-Forwarded-For, returned400 ×10then429 429. With the trusted peer configured, all 12 distinct values passed, and 12 requests sharing one value still capped at 10.Also
AUTH_RATE_LIMITmakes the 10/min limit configurable like every other limiter.compose.prod.ymlrefuses to start withoutTRUSTED_PROXIES, because the failure it prevents is silent.Checks run locally
cargo test --all-targets405 passed,cargo clippy --all-targets -- -D warningsclean, touched filerustfmt-clean. 13 new tests cover CIDR parsing and prefixes that do not land on a byte boundary, IPv6, never matching across families, junk specs, untrusted peers ignoring the header, forged prefixes being ignored, chained trusted hops, and the shared-bucket behaviour itself as a regression guard.Deploying this needs
TRUSTED_PROXIES=172.22.0.0/16added to the production env file first.