refactor RemoteIp middleware

- return the last forwarded IP before REMOTE_ADDR to handle proxies
- remove completely superfluous RemoteIpGetter class
- remove duplication of trusted proxies regexp
- remove unused constant from Request
- move comments from Request to where they are actually relevant
- edit comments for clarity of purpose

The original code (confusingly) tried to return REMOTE_ADDR both at the beginning and the end of the chain of options. Since REMOTE_ADDR is _always_ set, this is kind of silly. This change leaves REMOTE_ADDR as the last option, so that proxied requests will be assigned the correct remote IP address.

2 files changed, 45 insertions, 55 deletions

diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index 7a5237dcf3..69ca050d0c 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -155,24 +155,7 @@ module ActionDispatch
       @ip ||= super
     end
 
-    # Which IP addresses are "trusted proxies" that can be stripped from
-    # the right-hand-side of X-Forwarded-For.
-    #
-    # http://en.wikipedia.org/wiki/Private_network#Private_IPv4_address_spaces.
-    TRUSTED_PROXIES = %r{
-      ^127\.0\.0\.1$                | # localhost
-      ^(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
-       )\.
-    }x
-
-    # Determines originating IP address. REMOTE_ADDR is the standard
-    # but will fail if the user is behind a proxy. HTTP_CLIENT_IP and/or
-    # HTTP_X_FORWARDED_FOR are set by proxies so check for these if
-    # REMOTE_ADDR is a proxy. HTTP_X_FORWARDED_FOR may be a comma-
-    # delimited list in the case of multiple chained proxies; the last
-    # address which is not trusted is the originating IP.
+    # Originating IP address, usually set by the RemoteIp middleware.
     def remote_ip
       @remote_ip ||= (@env["action_dispatch.remote_ip"] || ip).to_s
     end
diff --git a/actionpack/lib/action_dispatch/middleware/remote_ip.rb b/actionpack/lib/action_dispatch/middleware/remote_ip.rb
index c7d710b98e..79f9ddcd04 100644
--- a/actionpack/lib/action_dispatch/middleware/remote_ip.rb
+++ b/actionpack/lib/action_dispatch/middleware/remote_ip.rb
@@ -2,50 +2,57 @@ module ActionDispatch
   class RemoteIp
     class IpSpoofAttackError < StandardError ; end
 
-    class RemoteIpGetter
-      def initialize(env, check_ip_spoofing, trusted_proxies)
-        @env = env
-        @check_ip_spoofing = check_ip_spoofing
-        @trusted_proxies = trusted_proxies
+    # IP addresses that are "trusted proxies" that can be stripped from
+    # the comma-delimited list in the X-Forwarded-For header. See also:
+    # http://en.wikipedia.org/wiki/Private_network#Private_IPv4_address_spaces
+    TRUSTED_PROXIES = %r{
+      ^127\.0\.0\.1$                | # localhost
+      ^(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
+       )\.
+    }x
+
+    def initialize(app, check_ip_spoofing = true, custom_proxies = nil)
+      @app = app
+      @check_ip_spoofing = check_ip_spoofing
+      if custom_proxies
+        custom_regexp = Regexp.new(custom_proxies, "i")
+        @trusted_proxies = Regexp.union(TRUSTED_PROXIES, custom_regexp)
+      else
+        @trusted_proxies = TRUSTED_PROXIES
       end
+    end
 
-      def remote_addrs
-        @remote_addrs ||= begin
-          list = @env['REMOTE_ADDR'] ? @env['REMOTE_ADDR'].split(/[,\s]+/) : []
-          list.reject { |addr| addr =~ @trusted_proxies }
-        end
+    # Determines originating IP address. REMOTE_ADDR is the standard
+    # 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 last address which is not a known proxy
+    # will be the originating IP.
+    def call(env)
+      client_ip     = env['HTTP_CLIENT_IP']
+      forwarded_ips = ips_from(env, 'HTTP_X_FORWARDED_FOR')
+      remote_addrs  = ips_from(env, 'REMOTE_ADDR')
+
+      if client_ip && @check_ip_spoofing && !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
 
-      def to_s
-        return remote_addrs.first if remote_addrs.any?
-
-        forwarded_ips = @env['HTTP_X_FORWARDED_FOR'] ? @env['HTTP_X_FORWARDED_FOR'].strip.split(/[,\s]+/) : []
-
-        if client_ip = @env['HTTP_CLIENT_IP']
-          if @check_ip_spoofing && !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
-          return client_ip
-        end
-
-        return forwarded_ips.reject { |ip| ip =~ @trusted_proxies }.last || @env["REMOTE_ADDR"]
-      end
+      remote_ip = client_ip || forwarded_ips.last || remote_addrs.last
+      env["action_dispatch.remote_ip"] = remote_ip
+      @app.call(env)
     end
 
-    def initialize(app, check_ip_spoofing = true, trusted_proxies = nil)
-      @app = app
-      @check_ip_spoofing = check_ip_spoofing
-      regex = '(^127\.0\.0\.1$|^(10|172\.(1[6-9]|2[0-9]|30|31)|192\.168)\.)'
-      regex << "|(#{trusted_proxies})" if trusted_proxies
-      @trusted_proxies = Regexp.new(regex, "i")
-    end
+  protected
 
-    def call(env)
-      env["action_dispatch.remote_ip"] = RemoteIpGetter.new(env, @check_ip_spoofing, @trusted_proxies)
-      @app.call(env)
+    def ips_from(env, header)
+      ips = env[header] ? env[header].strip.split(/[,\s]+/) : []
+      ips.reject{|ip| ip =~ @trusted_proxies }
     end
+
   end
 end
\ No newline at end of file