Skip to content

Loading…

The first IP address in the X-Forwarded-For header is the originating IP #2490

Merged
merged 1 commit into from

6 participants

@gsterndale

When geocoding IP addresses, I found that remote_ip values for requests coming through multiple proxies were incorrect.

Currently RemoteIpGetter instances are looking at the right-most IP address in the X-Forwarded-For header for the originating remote IP.

Wikipedia's X-Forwarded-For article describes the format of the (non-RFC-standard) field as "a comma+space separated list of IP addresses, the left-most being the farthest downstream client", which is consistent with what I'm seeing.

Have you seen otherwise in the wild?

Thanks!

@adamcrown

As best I can figure, the logic behind this line

return forwarded_ips.reject { |ip| ip =~ @trusted_proxies }.last || @env["REMOTE_ADDR"]

is that it's trying to remove all of the proxy IPs leaving only one client IP. In which case first or last wouldn't matter. However that doesn't always work. Just because an IP is in the trusted_proxies range doesn't mean it's actually a proxy. And this line can wind up stripping out the client's IP as well.

There's another issues that covers this problem: #1010.

It seems like this would be a better solution.

return forwarded_ips.first || @env["REMOTE_ADDR"]

@gsterndale

Thanks adamcrown!

I went back and forth on that. In the end, I wasn't confident enough that all proxies follow the format described in Wikipedia, so I decided to keep rejecting those IPs that matched @trusted_proxies.

Are you finding that the @trusted_proxies Regexp is incorrect, or that proxies can also be the originating clients?

Best,
Greg

@adamcrown

The @trusted_proxies regexp is correct in that it matches any IP addresses that are reserved for LANs like 192.168.. or 10...*. The problem is, if the client happens to be connecting from the LAN and has an IP address like 192.168.0.10, it will be rejected from the forwarded_ips array, resulting in an empty array, and the REMOTE_ADDR header winds up getting returned. And the REMOTE_ADDR is the IP for the proxy, not the client.

So the end result we're seeing in our app (which is being reverse proxied, with both the proxy server and the app server being hosted on the LAN) is that request.remote_ip returns the correct IP for clients connecting over the internet, but clients within our network are being seen as having the proxy's IP.

For a while I thought I could just set trusted_ips to our proxy server's exact IP only, but it turns out you can't overwrite the regular expression used to find trusted proxies, you can only add to it. So at the moment we're just grabbing request.headers['X_FORWARDED_FOR'] directly.

@gsterndale

Hi adamcrown,

I just issued another pull request that might help resolve your troubles.

#2632

Take a look and let me know what you think.

Best,
Greg

@gsterndale

@arunagw, @josevalim this PR is related to the one you just merged #2632. Thanks!

@josevalim josevalim merged commit 275c3a1 into rails:master
@courtland

It would be nice to get this into 3.2.x at some point. I've been monkey-patching this for months. @spastorino can you do anything?

@carlosantoniodasilva
Ruby on Rails member

The only problem to backport it is that it can break a considerable number of apps I think, so it'd be probably safer to only have it in master / 4.0.

@rafaelfranca
Ruby on Rails member

Yes, I don't think is safe to backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
View
2 actionpack/lib/action_dispatch/middleware/remote_ip.rb
@@ -57,7 +57,7 @@ def calculate_ip
"HTTP_X_FORWARDED_FOR=#{@env['HTTP_X_FORWARDED_FOR'].inspect}"
end
- not_proxy = client_ip || forwarded_ips.last || remote_addrs.first
+ not_proxy = client_ip || forwarded_ips.first || remote_addrs.first
# Return first REMOTE_ADDR if there are no other options
not_proxy || ips_from('REMOTE_ADDR', :allow_proxies).first
View
8 actionpack/test/dispatch/request_test.rb
@@ -42,7 +42,7 @@ def url_for(options = {})
'HTTP_X_FORWARDED_FOR' => '3.4.5.6'
assert_equal '3.4.5.6', request.remote_ip
- request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,3.4.5.6'
+ request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,unknown'
assert_equal '3.4.5.6', request.remote_ip
request = stub_request 'HTTP_X_FORWARDED_FOR' => '172.16.0.1,3.4.5.6'
@@ -63,7 +63,7 @@ def url_for(options = {})
request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,192.168.0.1'
assert_equal 'unknown', request.remote_ip
- request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 3.4.5.6, 10.0.0.1, 172.31.4.4'
+ request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6, 9.9.9.9, 10.0.0.1, 172.31.4.4'
assert_equal '3.4.5.6', request.remote_ip
request = stub_request 'HTTP_X_FORWARDED_FOR' => '1.1.1.1',
@@ -85,7 +85,7 @@ def url_for(options = {})
:ip_spoofing_check => false
assert_equal '2.2.2.2', request.remote_ip
- request = stub_request 'HTTP_X_FORWARDED_FOR' => '8.8.8.8, 9.9.9.9'
+ request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 8.8.8.8'
assert_equal '9.9.9.9', request.remote_ip
end
@@ -116,7 +116,7 @@ def url_for(options = {})
request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,67.205.106.73'
assert_equal 'unknown', request.remote_ip
- request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 3.4.5.6, 10.0.0.1, 67.205.106.73'
+ request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6, 9.9.9.9, 10.0.0.1, 67.205.106.73'
assert_equal '3.4.5.6', request.remote_ip
end
Something went wrong with that request. Please try again.