From fbf1d82e0db55a60610b1df56f531e4200cf1e26 Mon Sep 17 00:00:00 2001 From: Duncan Brown Date: Sun, 10 May 2020 03:23:34 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20ignore=20X-Forwarded-For=20IPs?= =?UTF-8?q?=20with=20ports=20attached?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 5 +++++ .../lib/action_dispatch/middleware/remote_ip.rb | 11 +++++++---- actionpack/test/dispatch/request_test.rb | 3 +++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 73fdad1822..6f24cb88d1 100644 --- a/actionpack/CHANGELOG.md +++ b/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`. diff --git a/actionpack/lib/action_dispatch/middleware/remote_ip.rb b/actionpack/lib/action_dispatch/middleware/remote_ip.rb index 02ab819dc1..7e1a7d7a06 100644 --- a/actionpack/lib/action_dispatch/middleware/remote_ip.rb +++ b/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 diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index d725e171c4..9554149b31 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/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