-
Notifications
You must be signed in to change notification settings - Fork 532
proxy and non-heroku ip address capture #43
base: master
Are you sure you want to change the base?
Conversation
|
||
address = request.env['HTTP_X_FORWARDED_FOR'] | ||
address = request.env['HTTP_X_FORWARDED_FOR'].try(:split).try(:first) || request.remote_ip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be careful about 'split' here. X-Forwarded-For typically uses comma-space separation
(http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/x-forwarded-headers.html) so we would need to strip the trailing commas, but it's not clear that X-Forwaded-For is a strict format.
address = request.env.fetch('HTTP_X_FORWARDED_FOR', request.remote_ip).try(:split).try(:first).gsub(',', '')
But then I looked at the docs for remote_ip
and that seems to actually be applying these proxy-aware rules already, so I think we should just do this:
address = request.remote_ip
Unless I'm reading the docs wrong. http://apidock.com/rails/ActionDispatch/Request/remote_ip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right about X-Forwarded-For comma-space separation, that was a mistake.
What about request.remote_ip, it probably should walk around proxies, but on Heroku it returns Heroku router ip address instead of client's. http://stackoverflow.com/questions/18264304/get-clients-real-ip-address-on-heroku
Despite there's mentioned that client's remote ip address will be the last one, actually I've got Heroku + Cloudflare CDN returning client's remote ip the first one in sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, good to know.
I think if we use fetch
we can safely drop the two try
. Slightly more readable to me, happy to accept a working alternative though.
address = request.env.fetch('HTTP_X_FORWARDED_FOR', request.remote_ip).split.first.gsub(',', '')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, dropping try
will increase readability, I would like to propose maybe even better variant:
address = request.env.fetch('HTTP_X_FORWARDED_FOR', request.remote_ip).split(', ').first
Using split.first in case client is behind non-anonymous proxy or application using CDN. In this case threre will be several ip addresses in request.env['HTTP_X_FORWARDED_FOR'] and the first one will be the client's one.
Also If application is deployed on standalone server request.env['HTTP_X_FORWARDED_FOR'] will be blank and request.remote_ip will be used instead.