Do not set 0.0.0.0 (for unparsable addresses) and a fake port

[0xjac]

When a forwarded address is detected, its parsed and if this fails, the address is set to 0.0.0.0. Then regardless, the fake port 65535 is always added (Source: lines 119-126, shown below).

		if _, _, err := net.SplitHostPort(addr); err != nil {
			// If the IP isn't parsebale, we just replace it with 0.0.0.0
			if net.ParseIP(addr) == nil {
				addr = "0.0.0.0"
			}
			// well, it's fake, but we need some port here
			addr = net.JoinHostPort(addr, "65535")
		}

This does not make much sense to me. (If I'm missing something please let me know. 😉 )

Digging in a little, I would suggest not replacing addresses which cannot be parsed. Proxies might intentionally obfuscate addresses in the Forwarded header. While obfuscated, those identifiers might be useful for tracing and debugging, as explained in RFC7239, Section 6.3:

A generated identifier may be used where there is a wish to keep the internal IP addresses secret, while still allowing the "Forwarded" header field to be used for tracing and debugging.

Regarding the port, I see two cases:

1. The forwarded for value has a valid port which is a valid case (as documented in RFC7239, Section 6) or someone configured a proxy with a X-Forwarded-For header which includes the port (not common, but since there are no standards, not impossible).
   In this case, the real port should be used instead of the fake port.
2. The forwarded for values or X-Forwarded-For header do not have a port.
   In this case I would suggest also dropping the fake port and set the request's RemoteAddr to just the IP which accurately represents the available data.
   As documented on the RemoteAddr:

   This field [...] has no defined format. The HTTP server in this package sets RemoteAddr to an "IP:port" address before invoking a handler.
   So, setting just the IP without the address should be acceptable without breaking the net/http API. Any code can (and should) easily check if there is a port and not assume there is one.

In short I would suggest just dropping lines 119-126. However if you think it's worth keeping the existing behavior, then how about a flag to disable the behavior as needed?

If you agree, let me know which way you are leaning (remove or toggle flag). I'll happily submit a PR.

[stanvit]

Hi Jacques @0xjac!

Sorry, it took me a white to remember this library and reread the code.

I find your concerns perfectly valid and the PR is welcome.

It looks like that the library is used in at least one other project, so the "proper" change where the lines 119-126 are removed is going to break the contract. This would be a perfect case for a v2, but I honestly have to time to maintain the library.

The least intrusive fix would be to add a boolean (?) flag/property to the Wrapper struct and change behaviour based on it, not even changing the constructor signature.

[0xjac]

@stanvit thanks for your reply. I've been doing some more digging and I have noticed a few more things that are needed such as:

1. Switch to a go module
2. Pass obfuscated identifiers as allowed to set the header (instead of just IPs and CIDR).
3. Handle multiple values as list and repeated headers for Forwarded
4. Expose a HandlerFunc or something a bit more friendly to integrate with popular web frameworks which don't support http.Handler natively (like gin)

All in all, this is a bit more work than I initially anticipated. I don't want to take up too much of your time, especially if you don't have time to maintain a v2. Hence I am thinking of writing (and maintaining) a new library based/inspired by yours (properly credited of course).

That being said, if you still want a PR for the flag/property addressing the 0.0.0.0 address and fake port. I'll happily submit one as I initially offered!

[stanvit]

Thanks for understanding. I think starting a new project would be the best - with all five points you listed here. I'll happily place a link to your project in my README when it's ready if you like.

Please don't bother with the PR, I'll just redirect everyone interested in newer/better functionality to your project.