Restore original remote_ip algorithm.

We incorrectly were using the first address, rather than the last one. Added config option for X_FORWARDED_FOR header We don't need to filter values from X_FORWARDED_FOR header. Client IP can be only first IP in list by specification and only last for ruby servers and Apache. We should return nil if it's proxy IP or invalid IP instead of client IP. Fixes rails#7979
steveklabnik · Nov 17, 2012 · cc8556e · cc8556e
1 parent f63545c
commit cc8556e
Show file tree

Hide file tree

Showing 7 changed files with 165 additions and 75 deletions.
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
@@ -1,5 +1,15 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Added option `config.action_dispatch.last_forwarded_ip` to enable different
+    order for getting `remote_ip` from `X_FORWARDED_FOR` header.
+    Set last valid IP from `X_FORWARDED_FOR` header as `client_ip` by default.
+    To change order to first valid IP, set option `config.action_dispatch.last_forwarded_ip`
+    to `false`.
+    Reverted filtering for valid IPs from `X_FORWARDED_FOR` header.
+    Fix #7979
+
+    *Steve Klabnik*, *André Arko*, *Alexey Gaziev*
+
 *   Introduce `ActionView::Template::Handlers::ERB.escape_whitelist`. This is a list
     of mime types where template text is not html escaped by default. It prevents `Jack & Joe`
     from rendering as `Jack &amp; Joe` for the whitelisted mime types. The default whitelist

diff --git a/actionpack/lib/action_dispatch/middleware/remote_ip.rb b/actionpack/lib/action_dispatch/middleware/remote_ip.rb
@@ -8,19 +8,20 @@ class IpSpoofAttackError < StandardError ; end
     # http://en.wikipedia.org/wiki/Private_network#Private_IPv6_addresses.
     TRUSTED_PROXIES = %r{
       ^127\.0\.0\.1$                | # localhost
-      ^::1$                         |
-      ^(10                          | # private IP 10.x.x.x
-        172\.(1[6-9]|2[0-9]|3[0-1]) | # private IP in the range 172.16.0.0 .. 172.31.255.255
-        192\.168                    | # private IP 192.168.x.x
-        fc00::                        # private IP fc00
+      ^::1$                         | # localhost
+      ^fc00:                        | # private IP range fc00
+      ^(10                          | # private IP range 10.x.x.x
+        172\.(1[6-9]|2[0-9]|3[0-1]) | # private IP range 172.16.0.0 .. 172.31.255.255
+        192\.168                    | # private IP range 192.168.x.x
        )\.
     }x
 
-    attr_reader :check_ip, :proxies
+    attr_reader :check_ip, :proxies, :last_ip
 
-    def initialize(app, check_ip_spoofing = true, custom_proxies = nil)
+    def initialize(app, check_ip_spoofing = true, custom_proxies = nil, last_forwarded_ip = true)
       @app = app
       @check_ip = check_ip_spoofing
+      @last_ip = last_forwarded_ip
       @proxies = case custom_proxies
         when Regexp
           custom_proxies
@@ -72,53 +73,47 @@ def initialize(env, middleware)
       # but will be wrong if the user is behind a proxy. Proxies will set
       # HTTP_CLIENT_IP and/or HTTP_X_FORWARDED_FOR, so we prioritize those.
       # HTTP_X_FORWARDED_FOR may be a comma-delimited list in the case of
-      # multiple chained proxies. The first address which is in this list
-      # if it's not a known proxy will be the originating IP.
-      # Format of HTTP_X_FORWARDED_FOR:
-      # client_ip, proxy_ip1, proxy_ip2...
-      # http://en.wikipedia.org/wiki/X-Forwarded-For
+      # multiple chained proxies. The last address which is not a known proxy
+      # by default will be the originating IP according to this bug in Apache:
+      # https://issues.apache.org/bugzilla/show_bug.cgi?id=50453 and behavior of
+      # ruby servers: http://andre.arko.net/2011/12/26/repeated-headers-and-ruby-web-servers
+      # In some exception cases it can be changed with config option
+      # config.action_dispatch.last_forwarded_ip set to false.
+      # It compiles to w3c spec:
+      # http://www.w3.org/TR/2009/WD-ct-guidelines-20091006/#sec-additional-headers
+      # See also http://en.wikipedia.org/wiki/X-Forwarded-For
       def calculate_ip
-        client_ip    = @env['HTTP_CLIENT_IP']
-        forwarded_ip = ips_from('HTTP_X_FORWARDED_FOR').first
-        remote_addrs = ips_from('REMOTE_ADDR')
+        client_ip     = @env['HTTP_CLIENT_IP']
+        forwarded_ips = ips_from('HTTP_X_FORWARDED_FOR')
+        forwarded_ips.reverse! if @middleware.last_ip
+        remote_addrs  = ips_from('REMOTE_ADDR').reverse
 
         check_ip = client_ip && @middleware.check_ip
-        if check_ip && forwarded_ip != client_ip
+        if check_ip && !forwarded_ips.include?(client_ip)
           # We don't know which came from the proxy, and which from the user
           raise IpSpoofAttackError, "IP spoofing attack?!" \
             "HTTP_CLIENT_IP=#{@env['HTTP_CLIENT_IP'].inspect}" \
             "HTTP_X_FORWARDED_FOR=#{@env['HTTP_X_FORWARDED_FOR'].inspect}"
         end
 
-        client_ips = remove_proxies [client_ip, forwarded_ip, remote_addrs].flatten
-        if client_ips.present?
-          client_ips.first
-        else
-          # If there is no client ip we can return first valid proxy ip from REMOTE_ADDR
-          remote_addrs.find { |ip| valid_ip? ip }
-        end
+        client_ips = [client_ip, forwarded_ips, remote_addrs].flatten.compact
+        # Without a client IP, just return the first REMOTE_ADDR
+        remove_proxies(client_ips).first || remote_addrs.first
       end
 
       def to_s
         @ip ||= calculate_ip
       end
 
-      private
+    protected
 
       def ips_from(header)
-        @env[header] ? @env[header].strip.split(/[,\s]+/) : []
-      end
-
-      def valid_ip?(ip)
-        ip =~ VALID_IP
-      end
-
-      def not_a_proxy?(ip)
-        ip !~ @middleware.proxies
+        ips = @env[header] ? @env[header].strip.split(/[,\s]+/) : []
+        ips.select{ |ip| ip =~ VALID_IP }
       end
 
       def remove_proxies(ips)
-        ips.select { |ip| valid_ip?(ip) && not_a_proxy?(ip) }
+        ips.reject { |ip| ip =~ @middleware.proxies }
       end
 
     end

diff --git a/actionpack/lib/action_dispatch/railtie.rb b/actionpack/lib/action_dispatch/railtie.rb
@@ -5,6 +5,7 @@ class Railtie < Rails::Railtie
     config.action_dispatch = ActiveSupport::OrderedOptions.new
     config.action_dispatch.x_sendfile_header = nil
     config.action_dispatch.ip_spoofing_check = true
+    config.action_dispatch.last_forwarded_ip = true
     config.action_dispatch.show_exceptions = true
     config.action_dispatch.best_standards_support = true
     config.action_dispatch.tld_length = 1

diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
@@ -32,7 +32,7 @@ def url_for(options = {})
     assert_equal '1.2.3.4', request.remote_ip
 
     request = stub_request 'REMOTE_ADDR' => '1.2.3.4,3.4.5.6'
-    assert_equal '1.2.3.4', request.remote_ip
+    assert_equal '3.4.5.6', request.remote_ip
 
     request = stub_request 'REMOTE_ADDR' => '1.2.3.4',
                            'HTTP_X_FORWARDED_FOR' => '3.4.5.6'
@@ -45,25 +45,25 @@ def url_for(options = {})
     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'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,172.16.0.1'
+    assert_equal '3.4.5.6', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '192.168.0.1,3.4.5.6'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,192.168.0.1'
+    assert_equal '3.4.5.6', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '10.0.0.1,3.4.5.6'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,10.0.0.1'
+    assert_equal '3.4.5.6', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '10.0.0.1, 10.0.0.1, 3.4.5.6'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6, 10.0.0.1, 10.0.0.1'
+    assert_equal '3.4.5.6', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '127.0.0.1,3.4.5.6'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,127.0.0.1'
+    assert_equal '3.4.5.6', request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,192.168.0.1'
     assert_equal nil, request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6, 9.9.9.9, 10.0.0.1, 172.31.4.4'
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 10.0.0.1, 172.31.4.4, 3.4.5.6'
     assert_equal '3.4.5.6', request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => 'not_ip_address'
@@ -89,14 +89,60 @@ def url_for(options = {})
     assert_equal '2.2.2.2', request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 8.8.8.8'
+    assert_equal '8.8.8.8', request.remote_ip
+  end
+
+  test "remote ip with last_forwarded_ip set to false" do
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,unknown',
+                           :last_forwarded_ip => false
+    assert_equal '3.4.5.6', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,172.16.0.1',
+                           :last_forwarded_ip => false
+    assert_equal '3.4.5.6', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,192.168.0.1',
+                           :last_forwarded_ip => false
+    assert_equal '3.4.5.6', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,10.0.0.1',
+                           :last_forwarded_ip => false
+    assert_equal '3.4.5.6', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6, 10.0.0.1, 10.0.0.1',
+                           :last_forwarded_ip => false
+    assert_equal '3.4.5.6', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6,127.0.0.1',
+                           :last_forwarded_ip => false
+    assert_equal '3.4.5.6', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '127.0.0.1,3.4.5.6',
+                           :last_forwarded_ip => false
+    assert_equal '3.4.5.6', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,192.168.0.1',
+                           :last_forwarded_ip => false
+    assert_equal nil, request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 10.0.0.1, 172.31.4.4, 3.4.5.6',
+                           :last_forwarded_ip => false
+    assert_equal '9.9.9.9', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'not_ip_address',
+                           :last_forwarded_ip => false
+    assert_equal nil, request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 8.8.8.8',
+                           :last_forwarded_ip => false
     assert_equal '9.9.9.9', request.remote_ip
   end
 
   test "remote ip v6" do
     request = stub_request 'REMOTE_ADDR' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334'
     assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
 
-    request = stub_request 'REMOTE_ADDR' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334,fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
+    request = stub_request 'REMOTE_ADDR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329,2001:0db8:85a3:0000:0000:8a2e:0370:7334'
     assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
 
     request = stub_request 'REMOTE_ADDR' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334',
@@ -107,26 +153,20 @@ def url_for(options = {})
                            'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
     assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
-    assert_equal nil, request.remote_ip
-
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '::1,fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
-    assert_equal nil, request.remote_ip
-
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '::1,fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329,unknown'
+    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '::1,fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329,::1'
+    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '::1, ::1, fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329, ::1, ::1'
+    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,::1'
     assert_equal nil, request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334, fe80:0000:0000:0000:0202:b3ff:fe1e:8329, ::1, fc00::'
-    assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
+    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => 'not_ip_address'
     assert_equal nil, request.remote_ip
@@ -151,6 +191,36 @@ def url_for(options = {})
     assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329, 2001:0db8:85a3:0000:0000:8a2e:0370:7334'
+    assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
+  end
+
+  test "remote ip v6 with last_forwarded_ip set to false" do
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329,unknown',
+                           :last_forwarded_ip => false
+    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329,::1',
+                           :last_forwarded_ip => false
+    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329, ::1, ::1',
+                           :last_forwarded_ip => false
+    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,::1',
+                           :last_forwarded_ip => false
+    assert_equal nil, request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334, fe80:0000:0000:0000:0202:b3ff:fe1e:8329, ::1, fc00::',
+                           :last_forwarded_ip => false
+    assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'not_ip_address',
+                           :last_forwarded_ip => false
+    assert_equal nil, request.remote_ip
+
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329, 2001:0db8:85a3:0000:0000:8a2e:0370:7334',
+                           :last_forwarded_ip => false
     assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
   end
 
@@ -166,18 +236,18 @@ def url_for(options = {})
                            'HTTP_X_FORWARDED_FOR' => '67.205.106.73'
     assert_equal '3.4.5.6', request.remote_ip
 
-    request = stub_request 'REMOTE_ADDR' => '172.16.0.1,67.205.106.73',
+    request = stub_request 'REMOTE_ADDR' => '67.205.106.73,172.16.0.1',
                            'HTTP_X_FORWARDED_FOR' => '67.205.106.73'
     assert_equal '172.16.0.1', request.remote_ip
 
-    request = stub_request 'REMOTE_ADDR' => '67.205.106.73,3.4.5.6',
+    request = stub_request 'REMOTE_ADDR' => '3.4.5.6,67.205.106.73',
                            'HTTP_X_FORWARDED_FOR' => '67.205.106.73'
     assert_equal '3.4.5.6', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,67.205.106.73'
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '67.205.106.73,unknown'
     assert_equal nil, request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '3.4.5.6, 9.9.9.9, 10.0.0.1, 67.205.106.73'
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 10.0.0.1, 67.205.106.73, 3.4.5.6'
     assert_equal '3.4.5.6', request.remote_ip
   end
 
@@ -194,24 +264,24 @@ def url_for(options = {})
 
     request = stub_request 'REMOTE_ADDR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329,::1',
                            'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
-    assert_equal 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329', request.remote_ip
+    assert_equal '::1', request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => 'unknown,fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
     assert_equal nil, request.remote_ip
 
     request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329,2001:0db8:85a3:0000:0000:8a2e:0370:7334'
-    assert_equal nil, request.remote_ip
+    assert_equal "2001:0db8:85a3:0000:0000:8a2e:0370:7334", request.remote_ip
   end
 
   test "remote ip with user specified trusted proxies Regexp" do
     @trusted_proxies = /^67\.205\.106\.73$/i
 
-    request = stub_request 'REMOTE_ADDR' => '67.205.106.73',
-                           'HTTP_X_FORWARDED_FOR' => '3.4.5.6'
+    request = stub_request 'REMOTE_ADDR' => '3.4.5.6',
+                           'HTTP_X_FORWARDED_FOR' => '67.205.106.73'
     assert_equal '3.4.5.6', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => '67.205.106.73, 10.0.0.1, 9.9.9.9, 3.4.5.6'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '10.0.0.1, 9.9.9.9, 3.4.5.6, 67.205.106.73'
+    assert_equal '3.4.5.6', request.remote_ip
   end
 
   test "remote ip v6 with user specified trusted proxies Regexp" do
@@ -221,8 +291,8 @@ def url_for(options = {})
                            'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
     assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
 
-    request = stub_request 'HTTP_X_FORWARDED_FOR' => 'fe80:0000:0000:0000:0202:b3ff:fe1e:8329, 2001:0db8:85a3:0000:0000:8a2e:0370:7334'
-    assert_equal nil, request.remote_ip
+    request = stub_request 'HTTP_X_FORWARDED_FOR' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334, fe80:0000:0000:0000:0202:b3ff:fe1e:8329'
+    assert_equal '2001:0db8:85a3:0000:0000:8a2e:0370:7334', request.remote_ip
   end
 
   test "domains" do
@@ -804,8 +874,9 @@ def url_for(options = {})
 
   def stub_request(env = {})
     ip_spoofing_check = env.key?(:ip_spoofing_check) ? env.delete(:ip_spoofing_check) : true
+    last_ip = env.key?(:last_forwarded_ip) ? env.delete(:x_forwarded_for_last_ip) : true
     @trusted_proxies ||= nil
-    ip_app = ActionDispatch::RemoteIp.new(Proc.new { }, ip_spoofing_check, @trusted_proxies)
+    ip_app = ActionDispatch::RemoteIp.new(Proc.new { }, ip_spoofing_check, @trusted_proxies, last_ip)
     tld_length = env.key?(:tld_length) ? env.delete(:tld_length) : 1
     ip_app.call(env)
     ActionDispatch::Http::URL.tld_length = tld_length