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

Easier and coherent IP handling for proxy and non-proxy requests #1150

Closed
wants to merge 2 commits into from
Closed

Conversation

buschtoens
Copy link
Contributor

req.ip will now return a string containing the client's IP. Regardless if the client uses a proxy, multiple proxies or none.

I fixed a minor security issue in req.ips too.

var val = this.get('X-Forwarded-For');
return val
return trustProxy && val
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security fix. req.ips now behaves according to the specs and only returns an array of IPs if app.set("trust proxy", true);

@tj
Copy link
Member

tj commented May 30, 2012

oh damn i just pushed this haha, bad timing :p

@buschtoens
Copy link
Contributor Author

Coincidence? I think not! ... Just kidding. :D

When I compare my commit to your's I see that you:
a) omitted an unnecessary trustProxy check I did in req.ip (well done)
b) you probably got something twisted...

This is the structure of the header: x-forwarded-for: client, proxy1, proxy2, proxyn.
In req.ips you reverse the proxy list and end up with an array like this: ["proxyn", "proxy2", "proxy1", "client"]
In req.ip you then simply return 'req.ips[0]' which would be proxyn and not the client's ip.
You'll need to reverse it again like so: req.ips.reverse()[0].
In context it would be:

req.__defineGetter__('ip', function(){
  return this.ips.reverse()[0] || this.connection.remoteAddress;
});

Correct me if I'm wrong. :)

@tj
Copy link
Member

tj commented May 30, 2012

a commit or two before that I removed the .reverse(), it makes sense for req.subdomains IMHO but maybe not so much for req.ips

@buschtoens
Copy link
Contributor Author

I absolutely share your opinion. Since you already did everything I was going to do, I'll close this pull request.

And again, I'm amazed by your continous and fast work. Keep it on!

@buschtoens buschtoens closed this May 30, 2012
@tj
Copy link
Member

tj commented May 30, 2012

np! thanks for pointing out that flaw I probably wouldn't have seen it until someone complained with 3.0 haha

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

Successfully merging this pull request may close these issues.

None yet

2 participants