sec: parse X-Forwarded-For from trusted proxies via WIREFAN_TRUSTED_PROXIES - #19
Merged
Conversation
4 tasks
…ROXIES Fixes #9. clientIP previously returned RemoteAddr unconditionally. Behind the documented Caddy deployment that means every connection is 127.0.0.1, so the per-IP cap of 200 was effectively a global cap of 200 — one attacker exhausts the slot pool for everyone. Adds opt-in XFF parsing: - New env WIREFAN_TRUSTED_PROXIES, comma-separated CIDRs or single IPs (e.g. "127.0.0.1,10.0.0.0/8"). Single IPs become /32 or /128. - When a request's RemoteAddr is in trustedProxies, walk X-Forwarded-For right-to-left and return the first untrusted hop. This is the rightmost-untrusted-hop algorithm — clients can write the leftmost entry themselves, so the leftmost is never authoritative. - When trustedProxies is empty (default), behavior is unchanged: XFF is ignored, RemoteAddr is the source of truth. Helpers: - parseTrustedProxies(string) []netip.Prefix — handles bare IPs, CIDRs, malformed entries dropped silently. - addrInPrefixes / stripHostPort — small utilities. Tests cover: untrusted source ignores XFF, trusted proxy walks right-to-left, all-trusted falls back to leftmost, IPv6 loopback, single-client and chained-proxy paths.
EthanY33
force-pushed
the
sec/trust-proxy-xff
branch
from
May 7, 2026 23:02
0f9a1b1 to
c36ba65
Compare
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.
Summary
Fixes #9.
clientIPreturnedRemoteAddrunconditionally. Behind the documented Caddy deployment every connection is127.0.0.1, sodefaultIPCap = 200is a global cap — one attacker exhausts the slot pool for everyone.Adds opt-in
X-Forwarded-Forparsing using rightmost-untrusted-hop:WIREFAN_TRUSTED_PROXIES, comma-separated CIDRs or single IPs (e.g.127.0.0.1,10.0.0.0/8). Single IPs become/32(IPv4) or/128(IPv6).RemoteAddris intrustedProxies, walkX-Forwarded-Forright-to-left and return the first hop whose IP is not intrustedProxies. The leftmost entry is never authoritative — clients can pick that themselves.trustedProxiesis empty (default), behavior is unchanged: XFF is ignored.What changed
internal/server/upgrade.go:UpgradeHandlergainstrustedProxies []netip.Prefix, populated fromWIREFAN_TRUSTED_PROXIESat construction.parseTrustedProxies,addrInPrefixes,stripHostPorthelpers.clientIPtakestrustedProxiesand parses XFF when the source is trusted.internal/server/upgrade_test.go:TestParseTrustedProxies(CIDR / bare IP / malformed dropped)TestClientIPNoTrust(XFF ignored without trust)TestClientIPTrustedProxyPicksRightmostUntrusted(six scenarios)Operator notes
The Caddyfile already sets
X-Forwarded-For; setting the env makes the per-IP cap actually per-client-IP again.Test plan
go build ./...clean.go test ./internal/server/...passes (existing + 8 new cases).WIREFAN_TRUSTED_PROXIES=127.0.0.1, sendcurl -H "X-Forwarded-For: 1.2.3.4" localhost:8080/v1/connect?key=...from loopback → metrics show source IP1.2.3.4, not127.0.0.1.Conflict notes
This PR touches
internal/server/upgrade.go(also touched by #16 sanitize-logs). Conflicts are minimal — different lines — andgit mergeshould resolve cleanly. Whichever lands first, the other will need a trivial rebase.