stanvit / go-forwarded Goto Github PK

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.

The code right now chooses the rightmost X-Forwarded-For or Forwarded value. This only works if there is only one reverse proxy.

Instead, AllowedNets could be used to check from the rightmost for the first (from the right) IP that is not in the AllowedNets ranges. This would allow for any number of trusted reverse proxies between the internet and the server using this library.

If you do change to searching from the right, make sure to include all matching headers rather than just the last one. Otherwise you might search through the whole last header without finding what you want, and miss other headers.

More details here: https://adam-p.ca/blog/2022/03/x-forwarded-for/#algorithms

(I applaud you for using a rightmost approach rather than leftmost. This library is better than the vast majority of similar libraries that I looked at.)