Don’t ignore X-Forwarded-For IPs with ports attached

Rack decided to tolerate proxies which choose to attach ports to
X-Forwarded-For IPs by stripping the port:
https://github.com/rack/rack/pull/1251. Attaching a port is rare in the
wild but some proxies (notably Microsoft Azure's App Service) do it.

Without this patch, remote_ip will ignore X-Forwarded-For IPs with ports
attached and the return value is less likely to be useful.

Rails should do the same thing. The stripping logic is already available
in Rack::Request::Helpers, so change the X-Forwarded-For retrieval
method from ActionDispatch::Request#x_forwarded_for (which returns the
raw header) to #forwarded_for, which returns a stripped array of IP
addresses, or nil. There may be other benefits hiding in Rack's
implementation.

We can't call ips_from with an array (and legislating for that inside
ips_from doesn't appeal), so refactor out the bit we need to apply in
both cases (verifying the IP is acceptable to IPAddr and that it's not a
range) to a separate method called #sanitize_ips which reduces an array of
maybe-ips to an array of acceptable ones.

actionpack/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   `remote_ip` will no longer ignore IPs in X-Forwarded-For headers if they
+    are accompanied by port information.
+
+    *Duncan Brown*, *Prevenios Marinos*
+
 *   `fixture_file_upload` now uses path relative to `file_fixture_path`
 
     Previously the path had to be relative to `fixture_path`.

actionpack/lib/action_dispatch/middleware/remote_ip.rb

@@ -111,11 +111,11 @@ module ActionDispatch
       # the last address left, which was presumably set by one of those proxies.
       def calculate_ip
         # Set by the Rack web server, this is a single value.
-        remote_addr = ips_from(@req.remote_addr).last
+        remote_addr = sanitize_ips(ips_from(@req.remote_addr)).last
 
         # Could be a CSV list and/or repeated headers that were concatenated.
-        client_ips    = ips_from(@req.client_ip).reverse
-        forwarded_ips = ips_from(@req.x_forwarded_for).reverse
+        client_ips    = sanitize_ips(ips_from(@req.client_ip)).reverse
+        forwarded_ips = sanitize_ips(@req.forwarded_for || []).reverse
 
         # +Client-Ip+ and +X-Forwarded-For+ should not, generally, both be set.
         # If they are both set, it means that either:
@@ -160,7 +160,10 @@ module ActionDispatch
       def ips_from(header) # :doc:
         return [] unless header
         # Split the comma-separated list into an array of strings.
-        ips = header.strip.split(/[,\s]+/)
+        header.strip.split(/[,\s]+/)
+      end
+
+      def sanitize_ips(ips) # :doc:
         ips.select do |ip|
           # Only return IPs that are valid according to the IPAddr#new method.
           range = IPAddr.new(ip).to_range

actionpack/test/dispatch/request_test.rb

@@ -102,6 +102,9 @@ class RequestIP < BaseRequestTest
     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" => "3.4.5.6:1234,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 "192.168.0.1", request.remote_ip