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

integrate proxy-addr #281

Open
tj opened this issue May 7, 2014 · 18 comments
Open

integrate proxy-addr #281

tj opened this issue May 7, 2014 · 18 comments

Comments

@tj
Copy link
Member

tj commented May 7, 2014

re: expressjs/expressjs.com#152 (comment)
or whatever we decide is the way to go in those discussions

https://github.com/expressjs/proxy-addr/

@jonathanong
Copy link
Member

need to make a PR with https://github.com/expressjs/proxy-addr. i'm not 100% sure what's going on here, but at least it'll match express

@jonathanong
Copy link
Member

waiting until @dougwilson redos proxy-addr to the point i can understand :D

@dougwilson
Copy link
Contributor

@jonathanong I'll try to get something today, time pending :) From the title of this, are you intending to use it to do hop counting instead of whitelisting?

@tj
Copy link
Member Author

tj commented Jun 10, 2014

opened this during the discussion before, I'm cool with doing whatever we end up doing in express, might as well keep them similar

@dougwilson
Copy link
Contributor

Thanks @visionmedia :) Express allows for both. I'm basically working to rip out all those accessors for express into a module to use cross-project (req.secure, req.protocol, req.ip, req.ips, req.host). It can be implemented easily in koa now, but it'd be some copy-pasta from express until those are moved into a direct library.

@tj
Copy link
Member Author

tj commented Jun 10, 2014

sweet! even if we have:

req = new Forwarded(req);
req.ip
req.ips
req.secure
req.protocol

it would be easy to wrap up so we're not directly manipulating req until the framework level. I just saw the official support for Forwarded in the new http 1.1 spec thing too, not sure how fast people will roll that out but having a similar set of libs between express/koa/others would definitely be sweet

@dougwilson
Copy link
Contributor

I just saw the official support for Forwarded in the new http 1.1 spec thing too

Yea, I saw that too and it is nice that there is something official now :) I was thinking of adding it, but adding support for it right now is a security risk, because no proxies are filtering out Forwarded, so it's super easy to forge currently. It would have to be like opt-in or something if you know your proxy is generating Forwarded headers instead of the old headers or whatever.

@tj tj changed the title add .trustProxy count integrate proxy-addr Jun 13, 2014
@tj tj modified the milestone: 1.0 Nov 7, 2014
@jonathanong jonathanong modified the milestones: 1.1.0, 2.0 Aug 22, 2015
@jonathanong
Copy link
Member

@dougwilson what do you think about integrating this now?

@jonathanong
Copy link
Member

@dougwilson or are you still planning to release a trust module?

@dougwilson
Copy link
Contributor

proxy-addr will be split apart some more at some point, but AFAIK there isn't a reason proxy-addr cannot be used as-is currently in koa.

@jonathanong
Copy link
Member

👍

@amit777
Copy link

amit777 commented Oct 8, 2015

Hi, we noticed an issue when there are potentially multiple proxies and app.proxy = true. We occasionally end up with private IP addresses in the logs. I think the situation can happen in this example:

User (10.1.1.1) -> Corporate Web Proxy(1.1.1.1) ->  Our HAProxy(2.2.2.2) -> KoaApp (10.2.2.2)

in this scenereo, 10.1.1.1 is returned in this.ip instead of 1.1.1.1

Maybe we also could use another option called "app.filterPrivateIps = true" which will actually filter out any RFC1918 ips from this.ips. indutny's "ip" module has an .isPrivate() method that may be helpful.

@amit777
Copy link

amit777 commented Oct 8, 2015

Oh cool, it looks like proxy-addr actually may solve the exact issue I described above.

@ruimarinho
Copy link
Contributor

@amit777 this may or may not be an option depending on your setup, but one alternative thing you can do is to filter those ips before they hit the koa app. nginx allows this via the realip, i.e. to trust the X-Forwarded-For header by pre-setting a list of trusted ips (including private ones).

@jonathanong jonathanong modified the milestones: 1.2.0, 1.1.0 Oct 11, 2015
@jonathanong jonathanong mentioned this issue Jan 17, 2016
@jonathanong
Copy link
Member

o i c. proxy-addr doesn't do as much as i thoguht it would in its current form. right now, the benefit is minimal.

@jonathanong jonathanong removed this from the v1.2.0 milestone Mar 4, 2016
@amit777
Copy link

amit777 commented Mar 4, 2016

Just as a note, I found this in one of my logs even though I don't have ipv6 enabled on the server. Seems like some strange proxy setting headers in a funny way. The IP address at the end is my own public ip (though i've changed it to something random and invalid for anonymity).

::ffff:267.211.128.66

@amit777
Copy link

amit777 commented Mar 4, 2016

Altenatively, maybe could have something like an array of trusted headers by priority...

app.trustedProxyHeaders = ['X-Real-IP', 'X-Forwaded-For'] and then the app.ips() method could use that. I'm going to configure HAProxy to set the X-Real-IP, but I was hoping for a simple way for me to configure all my koa apps to pick it up. ALl my koa apps sit behind haproxy.

@jonathanong jonathanong added this to the v3.0.0 milestone Mar 15, 2016
@ahmadnassri
Copy link

FYI for anybody interested: resumed progress on jshttp/forwarded/pull/1 any assistance is welcome, whether in the form of review, feedback, contribution, or simply cheering :)

niftylettuce added a commit to forwardemail/forwardemail.net that referenced this issue Feb 17, 2021
titanism pushed a commit to forwardemail/forwardemail.net that referenced this issue Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants