Merge pull request #40789 from csutter/deprecate-remote-ip-single-value

Deprecate assigning single `trusted_proxies` value

actionpack/CHANGELOG.md

@@ -56,6 +56,14 @@
 
     *Alex Robbin*
 
+*   Deprecate the ability to assign a single value to `config.action_dispatch.trusted_proxies`
+    as `RemoteIp` middleware behaves inconsistently depending on whether this is configured
+    with a single value or an enumerable.
+
+    Fixes #40772
+
+    *Christian Sutter*
+
 *   Add `redirect_back_or_to(fallback_location, **)` as a more aesthetically pleasing version of `redirect_back fallback_location:, **`.
     The old method name is retained without explicit deprecation.
 

actionpack/lib/action_dispatch/middleware/remote_ip.rb

@@ -51,10 +51,8 @@ module ActionDispatch
     # clients (like WAP devices), or behind proxies that set headers in an
     # incorrect or confusing way (like AWS ELB).
     #
-    # The +custom_proxies+ argument can take an Array of string, IPAddr, or
-    # Regexp objects which will be used instead of +TRUSTED_PROXIES+. If a
-    # single string, IPAddr, or Regexp object is provided, it will be used in
-    # addition to +TRUSTED_PROXIES+. Any proxy setup will put the value you
+    # The +custom_proxies+ argument can take an enumerable which will be used
+    # instead of +TRUSTED_PROXIES+. Any proxy setup will put the value you
     # want in the middle (or at the beginning) of the X-Forwarded-For list,
     # with your proxy servers after it. If your proxies aren't removed, pass
     # them in via the +custom_proxies+ parameter. That way, the middleware will
@@ -67,6 +65,20 @@ module ActionDispatch
       elsif custom_proxies.respond_to?(:any?)
         custom_proxies
       else
+        ActiveSupport::Deprecation.warn(<<~EOM)
+          Setting config.action_dispatch.trusted_proxies to a single value has
+          been deprecated. Please set this to an enumerable instead. For
+          example, instead of:
+
+          config.action_dispatch.trusted_proxies = IPAddr.new("10.0.0.0/8")
+
+          Wrap the value in an Array:
+
+          config.action_dispatch.trusted_proxies = [IPAddr.new("10.0.0.0/8")]
+
+          Note that unlike passing a single argument, passing an enumerable
+          will *replace* the default set of trusted proxies.
+        EOM
         Array(custom_proxies) + TRUSTED_PROXIES
       end
     end

railties/test/application/middleware/remote_ip_test.rb

@@ -63,20 +63,35 @@ module ApplicationTests
       assert_equal "4.2.42.42", remote_ip("REMOTE_ADDR" => "1.1.1.1", "HTTP_X_FORWARDED_FOR" => "4.2.42.42")
     end
 
-    test "the user can set trusted proxies" do
+    test "the user can set trusted proxies with an enumerable of case equality comparable values" do
       make_basic_app do |app|
-        app.config.action_dispatch.trusted_proxies = /^4\.2\.42\.42$/
+        app.config.action_dispatch.trusted_proxies = [IPAddr.new("4.2.42.0/24"), /^4\.2\.42\.43$/, "4.2.42.44"]
       end
 
-      assert_equal "1.1.1.1", remote_ip("REMOTE_ADDR" => "1.1.1.1", "HTTP_X_FORWARDED_FOR" => "4.2.42.42")
+      assert_not_deprecated do
+        assert_equal "1.1.1.1",
+                     remote_ip("REMOTE_ADDR" => "1.1.1.1", "HTTP_X_FORWARDED_FOR" => "4.2.42.42,4.2.42.43,4.2.42.44")
+      end
     end
 
-    test "the user can set trusted proxies with an IPAddr argument" do
+    test "setting trusted proxies replaces the default set of trusted proxies" do
+      make_basic_app do |app|
+        app.config.action_dispatch.trusted_proxies = [IPAddr.new("4.2.42.0/24")]
+      end
+
+      assert_equal "10.0.0.0",
+                   remote_ip("REMOTE_ADDR" => "1.1.1.1", "HTTP_X_FORWARDED_FOR" => "10.0.0.0,4.2.42.42")
+    end
+
+    test "setting trusted proxies to a single value (deprecated) adds value to default trusted proxies" do
       make_basic_app do |app|
         app.config.action_dispatch.trusted_proxies = IPAddr.new("4.2.42.0/24")
       end
 
-      assert_equal "1.1.1.1", remote_ip("REMOTE_ADDR" => "1.1.1.1", "HTTP_X_FORWARDED_FOR" => "10.0.0.0,4.2.42.42")
+      assert_deprecated(/Setting config\.action_dispatch\.trusted_proxies to a single value/) do
+        assert_equal "1.1.1.1",
+                     remote_ip("REMOTE_ADDR" => "1.1.1.1", "HTTP_X_FORWARDED_FOR" => "10.0.0.0,4.2.42.42")
+      end
     end
   end
 end