`req.ips` value is different from the documented one

[thinety]

According to documentation, req.ips is

an array of IP addresses specified in the X-Forwarded-For request header.

My understanding is that this array should contain all IPs up to, but not including, the first trusted one. Although it is not explicit in the documentation, this excerpt endorses my speculation:

For example, if X-Forwarded-For is client, proxy1, proxy2, req.ips would be ["client", "proxy1", "proxy2"], where proxy2 is the furthest downstream.

The problem is that this expected behavior is not what actually happens. In the getter for req.ips, proxyaddr.all is used, which according to documentation returns

all the addresses of the request, optionally stopping at the first untrusted. This array is ordered from closest to furthest (i.e. arr[0] === req.connection.remoteAddress).

So, supposing app.set('trust proxy', 1), with the X-Forwarded-For header mentioned above, req.ips is ["proxy2"], going against the documentation.

[dougwilson]

Thanks for the report on the issue in our documentation! I have moved you report over to the documentation repository so we can track fixing the documentation. You are always welcome to make a PR to fix the documentation if you're willing, otherwise we will make sure the text is updated to better reflect the design.

[thinety]

Wouldn't this be a software bug, instead of a documentation one? For me, the documented behavior makes much more sense than the proxyaddr.all one.

[dougwilson]

Hi @thinety it certainly could be, though it is more often than not that the documentation is wrong, especially as the majority of it was written by a third-party company that was hired to write it. Of course, suddenly changing the behavior would cause all types of breakage for the code written on express that expects the current behavior. If you believe this is some regression in the code, if you could point out which version of Express the behavior this changed in, we can assess on how impactful it would be on the ecosystem to actually alter the behavior as a bug fix.

[thinety]

Oh yes, I see. I don't think it is a regression (git blame shows commits from 7 years ago), so in that case it altering this behavior as a bug fix would probably break more core than fix.