Handle IP splitting more safely#4
Conversation
Update getRealClientIP to handle IPv4 and IPv6 addresses more safely.
| remoteAddr := strings.SplitN(fhctx.RemoteAddr().String(), ":", 2) | ||
| remoteAddr := fhctx.RemoteAddr().String() | ||
| // Split host and port - handling both IPv4 and IPv6 | ||
| if host, _, err := net.SplitHostPort(remoteAddr); err == nil { |
There was a problem hiding this comment.
Good call on using SplitHostPort.
| ipAddress := strings.Split(utils.B2S(xff), ",") | ||
| remoteAddr[0] = strings.TrimSpace(ipAddress[len(ipAddress)-1]) | ||
| // Index 0 represents the original client | ||
| return strings.TrimSpace(ipAddress[0]) |
There was a problem hiding this comment.
AIUI, index 0 represents the original client's claim, which might be spoofed.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Forwarded-For#security_and_privacy_concerns recommends only trusting values in X-Forwarded-For that have been added by a trusted proxy, which is more likely to be true of the len-1 index (hence the current code).
I intend to stick to len-1 in my deployments. If you need index 0, then let's add that as a configuration option in config.go (with the len-1 behaviour as the default behaviour for that configuration option).
In any case, please revert this portion of this particular PR. Thanks!
There was a problem hiding this comment.
If you need index 0, then let's add that as a configuration option in config.go (with the len-1 behaviour as the default behaviour for that configuration option).
In any case, please revert this portion of this particular PR. Thanks!
On second thoughts, I'll merge this PR and then implement that configuration option in another PR.
Update getRealClientIP to handle IPv4 and IPv6 addresses more safely.