Rework `Request#ip` to handle empty `forwarded_for`.request-ip

2 files changed, 23 insertions, 7 deletions

diff --git a/lib/rack/request.rb b/lib/rack/request.rb
index 5191c2b3..be8f0b3b 100644
--- a/lib/rack/request.rb
+++ b/lib/rack/request.rb
@@ -352,16 +352,26 @@ module Rack
       end
 
       def ip
-        remote_addrs = split_header(get_header('REMOTE_ADDR'))
-        remote_addrs = reject_trusted_ip_addresses(remote_addrs)
+        remote_addresses = split_header(get_header('REMOTE_ADDR'))
+        external_addresses = reject_trusted_ip_addresses(remote_addresses)
 
-        if remote_addrs.any?
-          remote_addrs.first
-        else
-          forwarded_ips = self.forwarded_for
+        unless external_addresses.empty?
+          return external_addresses.first
+        end
 
-          reject_trusted_ip_addresses(forwarded_ips).last || forwarded_ips.first || get_header("REMOTE_ADDR")
+        if forwarded_for = self.forwarded_for
+          unless forwarded_for.empty?
+            # The forwarded for addresses are ordered: client, proxy1, proxy2.
+            # So we reject all the trusted addresses (proxy*) and return the
+            # last client. Or if we trust everyone, we just return the first
+            # address.
+            return reject_trusted_ip_addresses(forwarded_for).last || forwarded_for.first
+          end
         end
+
+        # If all the addresses are trusted, and we aren't forwarded, just return
+        # the first remote address, which represents the source of the request.
+        remote_addresses.first
       end
 
       # The media type (type/subtype) portion of the CONTENT_TYPE header
diff --git a/test/spec_request.rb b/test/spec_request.rb
index 0d0b2f74..4e6e52d6 100644
--- a/test/spec_request.rb
+++ b/test/spec_request.rb
@@ -1270,6 +1270,12 @@ EOF
 
     res = mock.get '/', 'REMOTE_ADDR' => '1.2.3.4,3.4.5.6'
     res.body.must_equal '1.2.3.4'
+
+    res = mock.get '/', 'REMOTE_ADDR' => '127.0.0.1'
+    res.body.must_equal '127.0.0.1'
+
+    res = mock.get '/', 'REMOTE_ADDR' => '127.0.0.1,127.0.0.1'
+    res.body.must_equal '127.0.0.1'
   end
 
   it 'deals with proxies' do