change header search order, add comments, add test for non-IP in proxy header

lib/geocoder/request.rb

 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

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