request.ip via x-forwarded-for should strip out ports

[jonathanong]

on azure (yes, i know), ports are also being sent as well as the ip in x-forwarded-for:

'x-forwarded-for': '23.243.1.1:38242'

request.ip should not return the port section. aye or nay?

[dougwilson]

Weird, I have not seen the port being included on Azure before. It's unfortunate that x-forwarded-for is not a standard in anyway, and is up to specify by any software. But I have never heard of something including anything other than either just the IP address or a hostname before.

Because ports are split from the host by a colon, it's possible that I could simply add the stripping in forwarded, though the load balancers I've seen with IPv6 just include the bare IPv6 address in x-forwarded-for, making blind splitting on the colons a bit harder.

[jonathanong]

yeah, i've never seen it before either. i'm using "App Services", if that matters.

doing it in forwarded would be better. i'm currently doing

ip = url.parse(`http://{ip}`).hostname

which seems to strip out the port correctly. let me know if there's a better method

[dougwilson]

Unfortunately that url.parse trick seems to fail for the most basic IPv6 test case:

node -e 'var ip = "::1", url = require("url"); console.dir(url.parse(`http://{ip}`).hostname)'
''

[dougwilson]

So what we can probably do is the following:

1. If net.isIPv6(ip) === true, then return ip
2. If ip.lastIndexOf(':') === -1 then return ip
3. ip = ip.substr(0, ip.lastIndexOf(':'))
4. If ip[0] === '[' && ip[ip.length - 1] === ']' then return ip.slice(1, -1)
5. return ip

That is a rough draft of what to apply to each address entry. What the overhead of that will be I haven't looked into yet.

[fl0w]

Since X-Forwarded-For is not a standard header, wouldn't it be sane to extract X-Forwarded-For as a separate - "replaceable" - function/repo? Allow user space to modify as needed? Maybe this is a minuscule problem but PaaS's are exploding, all with minor peculiarities.

[jonathanong]

@fl0w yes! if someone can get started with a module, we can move it to https://github.com/jshttp if it's good

[dougwilson]

The X-Forwarded-For parsing is already extracted into a module that only does that: https://github.com/jshttp/forwarded

[fl0w]

@dougwilson are you suggesting having https://github.com/jshttp/forwarded as a Koa dependency? Seems a bit overkill compared to current x-forwarded-for header field parsing, no?

[dougwilson]

@fl0w you asked:

wouldn't it be sane to extract X-Forwarded-For as a separate - "replaceable" - function/repo?

And I was just pointing out there is already a separate repo. Maybe you can elaborate on what you are asking for?

[dougwilson]

@fl0w

Allow user space to modify as needed?

You already can: Use Object.defineProperty to replace the getter at app.request.ips after you create your app object to whatever function you want to run to get the IPs from any header / parsing method you choose. This is why app.request and app.response exist so you can alter the prototypes.

[fl0w]

@dougwilson I understand now as I went through the lib source. My question came from ignorance. I was pondering about exposing e.g.

// pseudo fn-naming
app.registerParserForHeaderField('x-forwarded-for', myCustomParser)

It should/would work for all custom headers, not only for x-forwarded-for. But that's something that could be done in user space as you mentioned.

Apologies.