Add proper RFC 7239 support for 'Forwarded' header

[jwarkentin]

See RFC 7239 for specifics, but basically the Forwarded header should support values like these:

for="_gazonk"
for=192.0.2.60; proto=http; by=203.0.113.43
for=192.0.2.43, for=198.51.100.17
For="[2001:db8:cafe::17]:4711"

According the the RFC the Forwarded header supports the following case-insensitive parameters separated by a semicolon (and optional space): for, by, proto and host

It does not support just a plain IP address as the code currently accepts.

Each of those parameters can be a comma separated list.

The RFC is pretty straight forward. You've created a very helpful module here and it would be very nice to have support for that header with it!

[lpinca]

It seems that we should handle the forwarded header in special way as it combines all the information within one single header field. If I understand this correctly, there is no forwarded-port or forwarded-proto header.
Maybe it makes sence to create another module with its own tests to parse the forwarded header and integrate it here.

[3rd-Eden]

Agreed, it makes more sense to have forwarded header parsing as separate module and just consume that library in this module.

[jwarkentin]

I actually disagree. I believe this module should really be refactored because it already has some other issues. For example, it doesn't handle x-forwarded-for properly. It only works when x-forwarded-for contains a single IP, while the de facto standard allows it to contain a comma separated list of IPs with the first in the list being the client IP and the following being proxy IPs. This, in fact, created a big problem for me yesterday so I wrote my own parsing instead.

It also doesn't even look at the x-forwarded-proto header unless x-forwarded-for is also set.

I would be happy to submit a refactor that handles these different cases properly later if I get a few minutes. The fundamental problem is that this module tries to handle all the different headers in a generic way and in reality each header needs special treatment. I'll take a crack at it and create a pull request later today hopefully if you guys are open to it.

[lpinca]

It seems to me that it correctly handles a comma separated list of IPs for the x-forwarded-for header. Then ofc it only returns the client.

[jwarkentin]

Ah, it looks like spaces are breaking the handling of the x-forwarded-for header. That line should probably be changed to:

ips = (headers[proxies[i].ip] || '').split(/\s*,\s*/);

[lpinca]

It should work anyway as the "standard" syntax is this:

x-forwarded-for: client, proxy1, proxy2

The resulting ips array will be [ 'client', ' proxy1', ' proxy2' ] and all the elements after the first are trimmed when checking against the whitelist.

In the worst case you'll end up having client with trailing spaces but I think that it is good practice to validate the ip before using it. We can also trim it here.

P.S. Here is a parser for the Forwarded header: https://github.com/lpinca/forwarded-parse but it probably needs to be reviewed/improved by someone else before being used.

[lpinca]

@jwarkentin you were right here.
The ips are validated with net.isIP and validation fails if there are spaces.

This should be fixed in the latest version (1.0.1).

[mrlannigan]

Fixed by #8 and later refined by 2f9bd0e.

[lpinca]

Forwarded header (RFC 7239) is still not supported.

[mrlannigan]

True, I should've clarified that I was speaking about the x-forwarded-for header.

[mkolmhuber]

Forwarded header (RFC 7239) is still not supported.

[lpinca]

@mkolmhuber feel free to open a PR to add support for it via forwarded-parse.