Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jonathanong opened this issue Oct 4, 2016 · 11 comments · May be fixed by #1205
Open

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

jonathanong opened this issue Oct 4, 2016 · 11 comments · May be fixed by #1205

Comments

@jonathanong
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

jonathanong commented Oct 4, 2016

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

fl0w commented Oct 4, 2016

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
Copy link
Member Author

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

@dougwilson
Copy link
Contributor

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

@fl0w
Copy link
Contributor

fl0w commented Nov 29, 2016

@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
Copy link
Contributor

@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
Copy link
Contributor

dougwilson commented Nov 29, 2016

@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
Copy link
Contributor

fl0w commented Nov 29, 2016

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants