Repository navigation
feat(proxy): trust the forwarding headers of a set of networks - #128
Conversation
- A peer in TRUST_NETWORKS has its X-Real-IP and X-Forwarded-* headers kept - A repeated or empty forwarding header no longer breaks the client address - A network written with its host part set now matches every address of it
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches📝 Generate docstrings
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 |
joamag
added
enhancement
|
@codex review |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a19137f. Configure here.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
A reverse proxy that sits behind other proxies had only two choices for the forwarding headers: trust every origin (
TRUST_ORIGIN=1) or none. Behind an HAProxy that relays raw TCP with the PROXY protocol, trusting every origin lets any client on the internet spoofX-Real-IP, while trusting none overwrites the real client address that an internal proxy (eg: a container on the Docker bridge) carefully passed on.The change
A new
TRUST_NETWORKSoption (trust_networksin the constructor) takes a list of IPv4 addresses and CIDR networks, semicolon separated as every other list option:TRUST_NETWORKS="172.17.0.0/16;10.0.0.1" python -m netius.extra.proxy_rProxyServer.is_trusted()decides the trust of a request: every peer is trusted underTRUST_ORIGIN, otherwise only an IPv4 peer inside one of the networks is.ReverseProxyServer.on_headers()now uses it in place of the global flag, so for a trusted peerX-Real-IP,X-Client-IP,X-Forwarded-For,X-Forwarded-Proto,X-Forwarded-PortandForwardedare kept and passed on, and for any other peer they keep being replaced by what the proxy sees. Under the PROXY middleware the address that is checked is the one reported by the front-end, so a public client relayed by HAProxy is never trusted.Two bugs found on the way
A network written with its host part set matched only some of its addresses.
in_subnet_ip4compared the bits of the address against the unmasked subnet, so a network written as the address of an interface, the wayip addrprints it, gave an answer that depended on the host bits:The subnet is now masked before the comparison. This also affected the existing
ALLOWEDoption.A repeated forwarding header broke the request. The parser stores a repeated header as a sequence, so a trusted peer sending two
X-Forwarded-Forlines madeon_headersraiseAttributeError: 'list' object has no attribute 'split', and a repeatedX-Forwarded-Protoended up inside the canonical URL. An emptyX-Real-IPwas passed on as an empty client address. The values are now read through_prx_header, the same helper the rest of the proxy uses: the forwarded for values are joined (the first being the client), the last definition prevails for the others, and an empty value falls back to what the proxy sees. This was already reachable underTRUST_ORIGIN.Tests
test_in_subnet_ip4_hostcovers a subnet written with its host part set, at both edges of the rangetest_is_trustedcovers the edges of a network, exact addresses, IPv6 and IPv4 mapped addresses, an invalid octet, an interface style network andTRUST_ORIGINtest_on_serveandtest_on_serve_envcover the option being given on creation and read from the environmenton_headerscases cover a trusted peer, repeated headers, empty headers and an untrusted peer that tries to spoof themBoth bugs were reproduced by a failing test before being fixed. The complete suite passes (2021 passed, 16 skipped),
black --checkandstubtestare clean, and the new and changed code is at 100% coverage (27/27 statements, 18/18 branches).Note
Medium Risk
Changes client IP derivation and spoofing rules for reverse proxy deployments; misconfigured
TRUST_NETWORKScould trust forged headers or drop legitimate upstream forwarding data.Overview
Adds
TRUST_NETWORKSso the reverse proxy can trustX-Real-IP,X-Forwarded-For, and related headers only from listed IPv4 addresses/CIDRs—not from every client (TRUST_ORIGIN) or none.ProxyServer.is_trusted()treats a peer as trusted whenTRUST_ORIGINis on or the connection’s IPv4 address matchestrust_networks(via env/constructor).ReverseProxyServeruses that instead of the global flag for reading forwarding headers, stripping spoofedForwardedfrom untrusted peers, and passing through values from trusted front proxies.in_subnet_ip4now masks the subnet prefix so CIDRs written with a host address (e.g.172.17.0.1/16) match the whole network—also relevant forALLOWED.Forwarding header handling goes through
_prx_header: repeated values no longer crash or corrupt URLs (joinX-Forwarded-For, last value wins elsewhere), and empty values fall back to the peer address.Reviewed by Cursor Bugbot for commit a19137f. Bugbot is set up for automated code reviews on this repo. Configure here.