diff --git a/lib/geocoder/request.rb b/lib/geocoder/request.rb index 8f9fb0c14104910069cd4727fd9a2143a0f127c1..12bd57780d477dfca52e97953d3b747fe4c71b37 100644 --- a/lib/geocoder/request.rb +++ b/lib/geocoder/request.rb @@ -1,21 +1,23 @@ module Geocoder module Request - # This method is vulnerable to trivial IP spoofing. - # Don't use it in authorization/authentication code, - # or any other security-sensitive application. - # Use paranoid_location instead. + # The location() method is vulnerable to trivial IP spoofing. + # Don't use it in authorization/authentication code, or any + # other security-sensitive application. Use paranoid_location + # instead. def location - @location ||= Geocoder.search(geocoder_spoofable_ip).first + @location ||= Geocoder.search(geocoder_spoofable_ip, ip_address: true).first end - # This method protects you from trivial IP spoofing. For requests that go through a - # proxy that you haven't whitelisted as trusted in your Rack config, you will get - # the location for the IP of the last untrusted proxy in the chain, not the original - # client IP. You WILL NOT get the location corresponding to the original client IP - # for any request sent through a non-whitelisted proxy. + # This paranoid_location() protects you from trivial IP spoofing. + # For requests that go through a proxy that you haven't + # whitelisted as trusted in your Rack config, you will get the + # location for the IP of the last untrusted proxy in the chain, + # not the original client IP. You WILL NOT get the location + # corresponding to the original client IP for any request sent + # through a non-whitelisted proxy. def paranoid_location - @location ||= Geocoder.search(ip).first + @location ||= Geocoder.search(ip, ip_address: true).first end # There's a whole zoo of nonstandard headers added by various @@ -23,17 +25,32 @@ module Geocoder # ANY of these can be trivially spoofed! # (except REMOTE_ADDR, which should by set by your server, # and is included at the end as a fallback. - GEOCODER_CANDIDATE_HEADERS = ['HTTP_X_REAL_IP', - 'HTTP_X_CLIENT_IP', - 'HTTP_CLIENT_IP', - 'HTTP_X_FORWARDED_FOR', + # Order does matter: we're following the convention established in + # ActionDispatch::RemoteIp::GetIp::calculate_ip() + # https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/remote_ip.rb + # where the forwarded_for headers, possibly containing lists, + # are arbitrarily preferred over headers expected to contain a + # single address. + GEOCODER_CANDIDATE_HEADERS = ['HTTP_X_FORWARDED_FOR', 'HTTP_X_FORWARDED', - 'HTTP_X_CLUSTER_CLIENT_IP', 'HTTP_FORWARDED_FOR', 'HTTP_FORWARDED', + 'HTTP_X_CLIENT_IP', + 'HTTP_CLIENT_IP', + 'HTTP_X_REAL_IP', + 'HTTP_X_CLUSTER_CLIENT_IP', 'REMOTE_ADDR'] def geocoder_spoofable_ip + + # We could use a more sophisticated IP-guessing algorithm here, + # in which we'd try to resolve the use of different headers by + # different proxies. The idea is that by comparing IPs repeated + # in different headers, you can sometimes decide which header + # was used by a proxy further along in the chain, and thus + # prefer the headers used earlier. However, the gains might not + # be worth the performance tradeoff, since this method is likely + # to be called on every request in a lot of applications. GEOCODER_CANDIDATE_HEADERS.each do |header| if @env.has_key? header addrs = geocoder_split_ip_addresses(@env[header]) @@ -51,8 +68,10 @@ module Geocoder ip_addresses ? ip_addresses.strip.split(/[,\s]+/) : [] end - # use Rack's trusted_proxy?() method to filter out IPs - # that have been configured as trusted; includes private ranges by default. + # use Rack's trusted_proxy?() method to filter out IPs that have + # been configured as trusted; includes private ranges by + # default. (we don't want every lookup to return the location + # of our own proxy/load balancer) def geocoder_reject_trusted_ip_addresses(ip_addresses) ip_addresses.reject { |ip| trusted_proxy?(ip) } end diff --git a/test/unit/request_test.rb b/test/unit/request_test.rb index cf7db1a0edb769b8cd4e030f54bed253078889f7..e99d1e291b339ca3395c63de9863e065d2285a46 100644 --- a/test/unit/request_test.rb +++ b/test/unit/request_test.rb @@ -53,4 +53,8 @@ class RequestTest < GeocoderTestCase req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => ","}, "74.200.247.59") assert req.location.is_a?(Geocoder::Result::Freegeoip) end + def test_non_ip_in_proxy_header + req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "Albequerque NM"}) + assert req.location.is_a?(Geocoder::Result::Freegeoip) + end end