rework Geocoder::Request.location(), add Geocoder::Request.paranoid_location(), tests, fixtures

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.
     def location
-      @location ||= begin
-        detected_ip = env['HTTP_X_REAL_IP'] || (
-          env['HTTP_X_FORWARDED_FOR'] &&
-          env['HTTP_X_FORWARDED_FOR'].split(",").first.strip
-        )
-        detected_ip = IpAddress.new(detected_ip.to_s)
-        if detected_ip.valid? and !detected_ip.loopback?
-          real_ip = detected_ip.to_s
-        else
-          real_ip = self.ip
+      @location ||= Geocoder.search(geocoder_spoofable_ip).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.
+    def paranoid_location
+      @location ||= Geocoder.search(ip).first
+    end
+
+    # There's a whole zoo of nonstandard headers added by various
+    #   proxy softwares to indicate original client IP.
+    # 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',
+                                  'HTTP_X_FORWARDED',
+                                  'HTTP_X_CLUSTER_CLIENT_IP',
+                                  'HTTP_FORWARDED_FOR',
+                                  'HTTP_FORWARDED',
+                                  'REMOTE_ADDR']
+
+    def geocoder_spoofable_ip
+      GEOCODER_CANDIDATE_HEADERS.each do |header|
+        if @env.has_key? header
+          addrs = geocoder_split_ip_addresses(@env[header])
+          addrs = geocoder_reject_trusted_ip_addresses(addrs)
+          return addrs.first if addrs.any?
         end
-        Geocoder.search(real_ip).first
       end
-      @location
+
+      @env['REMOTE_ADDR']
+    end
+
+    private
+
+    def geocoder_split_ip_addresses(ip_addresses)
+      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.
+    def geocoder_reject_trusted_ip_addresses(ip_addresses)
+      ip_addresses.reject { |ip| trusted_proxy?(ip) }
     end
   end
 end

test/fixtures/freegeoip_74_200_247_60

+{
+  "city": "Nuevo Laredo",
+  "region_code": "TAM",
+  "region_name": "Tamaulipas",
+  "metrocode": "0",
+  "zipcode": "",
+  "country_name": "Mexico",
+  "country_code": "MX",
+  "ip": "74.200.247.60",
+  "latitude": "27.5",
+  "longitude": "-99.517"
+}

test/unit/request_test.rb

@@ -3,33 +3,54 @@ $: << File.join(File.dirname(__FILE__), "..")
 require 'test_helper'
 
 class RequestTest < GeocoderTestCase
-  class MockRequest
+  class MockRequest < Rack::Request
     include Geocoder::Request
-    attr_accessor :env, :ip
-    def initialize(env={}, ip="")
-      @env = env
-      @ip  = ip
+    def initialize(headers={}, ip="")
+      super_env = headers
+      super_env.merge!({'REMOTE_ADDR' => ip}) unless ip == ""
+      super(super_env)
     end
   end
   def test_http_x_real_ip
     req = MockRequest.new({"HTTP_X_REAL_IP" => "74.200.247.59"})
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
   end
+  def test_http_x_client_ip
+    req = MockRequest.new({"HTTP_X_CLIENT_IP" => "74.200.247.59"})
+    assert req.location.is_a?(Geocoder::Result::Freegeoip)
+  end
+  def test_http_x_cluster_client_ip
+    req = MockRequest.new({"HTTP_X_CLUSTER_CLIENT_IP" => "74.200.247.59"})
+    assert req.location.is_a?(Geocoder::Result::Freegeoip)
+  end
   def test_http_x_forwarded_for_without_proxy
     req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59"})
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
   end
   def test_http_x_forwarded_for_with_proxy
-    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59, 74.200.247.59"})
+    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59, 74.200.247.60"})
+    assert req.geocoder_spoofable_ip == '74.200.247.59'
+    assert req.ip == '74.200.247.60'
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
+    assert_equal "US", req.location.country_code
+  end
+  def test_paranoid_http_x_forwarded_for_with_proxy
+    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "74.200.247.59, 74.200.247.60"})
+    assert req.geocoder_spoofable_ip == '74.200.247.59'
+    assert req.ip == '74.200.247.60'
+    assert req.paranoid_location.is_a?(Geocoder::Result::Freegeoip)
+    assert_equal "MX", req.paranoid_location.country_code
   end
   def test_with_request_ip
     req = MockRequest.new({}, "74.200.247.59")
     assert req.location.is_a?(Geocoder::Result::Freegeoip)
   end
-
   def test_with_loopback_x_forwarded_for
     req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => "127.0.0.1"}, "74.200.247.59")
     assert_equal "US", req.location.country_code
   end
+  def test_http_x_forwarded_for_with_misconfigured_proxies
+    req = MockRequest.new({"HTTP_X_FORWARDED_FOR" => ","}, "74.200.247.59")
+    assert req.location.is_a?(Geocoder::Result::Freegeoip)
+  end
 end