Repository navigation
fix(ip): strip IPv6 zone before parsing IPs in ExtractIPFromXFFHeader - #3180
TheNetherWatcher wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
The fix addresses the reported IPv6 zone issue, but the current implementation introduces a couple of edge cases.
stripIPv6Zone() strips everything after % without validating whether the address is actually IPv6. This means malformed XFF entries like 127.0.0.2%garbage are now accepted instead of triggering the previous fallback behavior.
Additionally, with TrustLinkLocal(false), the same untrusted peer (fe80::1%eth0) produces different RealIP() values depending on whether an XFF header is present. This could lead to inconsistent IP-based rate limiting or access control.
Could we validate scoped IPv6 addresses more strictly, preserve the original direct peer address when it's untrusted, and add regression tests for these cases?
The overall approach looks good, but these edge cases should be addressed before merging. @aldas @vishr
|
|
||
| // stripIPv6Zone removes an IPv6 zone, e.g. "fe80::1%eth0", which net.ParseIP rejects. | ||
| func stripIPv6Zone(host string) string { | ||
| if i := strings.IndexByte(host, '%'); i != -1 { |
There was a problem hiding this comment.
The helper strips % suffixes from any address, including malformed IPv4 addresses such as 127.0.0.2%garbage. This allows previously invalid XFF entries to pass validation.
| ips[i] = strings.TrimPrefix(ips[i], "[") | ||
| ips[i] = strings.TrimSuffix(ips[i], "]") | ||
| ip := net.ParseIP(ips[i]) | ||
| ip := net.ParseIP(stripIPv6Zone(ips[i])) |
There was a problem hiding this comment.
With TrustLinkLocal(false), an untrusted zoned IPv6 peer returns fe80::1%eth0 without XFF but fe80::1 when XFF is present. This causes inconsistent RealIP() values for the same peer.
2 participants
Fixes #3179
ExtractIPFromXFFHeaderpassed the peer address straight tonet.ParseIP. For a link-local peer with a zone (fe80::1%eth0) that returnsnil, so the loop gave up and returned the proxy address instead of the client IP fromX-Forwarded-For.Changes:
stripIPv6Zoneand use it beforenet.ParseIPin the XFF loop. The returned value is unchanged when every hop is trusted (still the raw furthest entry).isTrustedPeer(scheme.go), which already stripped the zone inline.TestExtractIPFromXFFHeader: a zoned link-local peer with an external XFF entry, and an all-trusted zoned chain.go test -race ./...andgo vetpass. Both new cases fail on master.