Maintain Rack 2 parameter parsing behaviour

When running under Rack 3, we'll proceed with the new behaviour. That
means we won't affect applications that already use Rack 3, but does
also mean we can't help an application that is simultaneously bumping
Rails and Rack together.

However, with this commit, we at least ensure we maintain compatibility
for applications that are currently still on Rack 2 -- and we can help
with the transition using deprecation warnings.

Fixes rails#53394

Author: matthewd

actionpack/lib/action_dispatch/http/param_builder.rb

@@ -2,6 +2,10 @@
 
 module ActionDispatch
   class ParamBuilder
+    # --
+    # This implementation is based on Rack::QueryParser,
+    # Copyright (C) 2007-2021 Leah Neukirchen <http://leahneukirchen.org/infopage.html>
+
     def self.make_default(param_depth_limit)
       new param_depth_limit
     end
@@ -12,6 +16,10 @@ def initialize(param_depth_limit)
       @param_depth_limit = param_depth_limit
     end
 
+    cattr_accessor :ignore_leading_brackets
+
+    LEADING_BRACKETS_COMPAT = defined?(::Rack::RELEASE) && ::Rack::RELEASE.to_s.start_with?("2.")
+
     cattr_accessor :default
     self.default = make_default(100)
 
@@ -61,15 +69,30 @@ def store_nested_param(params, name, v, depth, encoding_template = nil)
           # nil name, treat same as empty string (required by tests)
           k = after = ""
         elsif depth == 0
-          # Start of parsing, don't treat [] or [ at start of string specially
-          if start = name.index("[", 1)
-            # Start of parameter nesting, use part before brackets as key
-            k = name[0, start]
-            after = name[start, name.length]
+          if ignore_leading_brackets || (ignore_leading_brackets.nil? && LEADING_BRACKETS_COMPAT)
+            # Rack 2 compatible behavior, ignore leading brackets
+            if name =~ /\A[\[\]]*([^\[\]]+)\]*/
+              k = $1
+              after = $' || ""
+
+              if !ignore_leading_brackets && (k != $& || !after.empty? && !after.start_with?("["))
+                ActionDispatch.deprecator.warn("Skipping over leading brackets in parameter name #{name.inspect} is deprecated and will parse differently in Rails 8.1 or Rack 3.0.")
+              end
+            else
+              k = name
+              after = ""
+            end
           else
-            # Plain parameter with no nesting
-            k = name
-            after = ""
+            # Start of parsing, don't treat [] or [ at start of string specially
+            if start = name.index("[", 1)
+              # Start of parameter nesting, use part before brackets as key
+              k = name[0, start]
+              after = name[start, name.length]
+            else
+              # Plain parameter with no nesting
+              k = name
+              after = ""
+            end
           end
         elsif name.start_with?("[]")
           # Array nesting

actionpack/lib/action_dispatch/http/query_parser.rb

@@ -1,11 +1,17 @@
 # frozen_string_literal: true
 
 require "uri"
+require "rack"
 
 module ActionDispatch
   class QueryParser
     DEFAULT_SEP = /& */n
-    COMMON_SEP = { ";" => /; */n, ";," => /[;,] */n, "&" => /& */n }
+    COMPAT_SEP = /[&;] */n
+    COMMON_SEP = { ";" => /; */n, ";," => /[;,] */n, "&" => /& */n, "&;" => /[&;] */n }
+
+    cattr_accessor :strict_query_string_separator
+
+    SEMICOLON_COMPAT = defined?(::Rack::QueryParser::DEFAULT_SEP) && ::Rack::QueryParser::DEFAULT_SEP.to_s.include?(";")
 
     #--
     # Note this departs from WHATWG's specified parsing algorithm by
@@ -14,7 +20,23 @@ class QueryParser
     def self.each_pair(s, separator = nil)
       return enum_for(:each_pair, s, separator) unless block_given?
 
-      (s || "").split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |part|
+      s ||= ""
+
+      splitter =
+        if separator
+          COMMON_SEP[separator] || /[#{separator}] */n
+        elsif strict_query_string_separator
+          DEFAULT_SEP
+        elsif SEMICOLON_COMPAT && s.include?(";")
+          if strict_query_string_separator.nil?
+            ActionDispatch.deprecator.warn("Using semicolon as a query string separator is deprecated and will not be supported in Rails 8.1 or Rack 3.0. Use `&` instead.")
+          end
+          COMPAT_SEP
+        else
+          DEFAULT_SEP
+        end
+
+      s.split(splitter).each do |part|
         next if part.empty?
 
         k, v = part.split("=", 2)

actionpack/lib/action_dispatch/railtie.rb

@@ -31,6 +31,9 @@ class Railtie < Rails::Railtie # :nodoc:
     config.action_dispatch.debug_exception_log_level = :fatal
     config.action_dispatch.strict_freshness = false
 
+    config.action_dispatch.ignore_leading_brackets = nil
+    config.action_dispatch.strict_query_string_separator = nil
+
     config.action_dispatch.default_headers = {
       "X-Frame-Options" => "SAMEORIGIN",
       "X-XSS-Protection" => "1; mode=block",
@@ -52,6 +55,9 @@ class Railtie < Rails::Railtie # :nodoc:
       ActionDispatch::Http::URL.secure_protocol = app.config.force_ssl
       ActionDispatch::Http::URL.tld_length = app.config.action_dispatch.tld_length
 
+      ActionDispatch::ParamBuilder.ignore_leading_brackets = app.config.action_dispatch.ignore_leading_brackets
+      ActionDispatch::QueryParser.strict_query_string_separator = app.config.action_dispatch.strict_query_string_separator
+
       ActiveSupport.on_load(:action_dispatch_request) do
         self.ignore_accept_header = app.config.action_dispatch.ignore_accept_header
         ActionDispatch::Request::Utils.perform_deep_munge = app.config.action_dispatch.perform_deep_munge

actionpack/test/dispatch/param_builder_test.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+
+class ParamBuilderTest < ActiveSupport::TestCase
+  # Much of the behavioral details are covered by long-standing
+  # integration tests in test/request/query_string_parsing_test.rb
+  #
+  # This test doesn't need to duplicate all of that: it just
+  # offers a simple baseline of unit tests.
+
+  test "simple query string" do
+    result = ActionDispatch::ParamBuilder.from_query_string("foo=bar&baz=quux")
+    assert_equal({ "foo" => "bar", "baz" => "quux" }, result)
+    assert_instance_of ActiveSupport::HashWithIndifferentAccess, result
+  end
+
+  test "nested parameters" do
+    result = ActionDispatch::ParamBuilder.from_query_string("foo[bar]=baz")
+    assert_equal({ "foo" => { "bar" => "baz" } }, result)
+    assert_instance_of ActiveSupport::HashWithIndifferentAccess, result[:foo]
+  end
+
+  if ::Rack::RELEASE.start_with?("2.")
+    test "(rack 2) defaults to ignoring leading bracket" do
+      assert_deprecated(ActionDispatch.deprecator) do
+        result = ActionDispatch::ParamBuilder.from_query_string("[foo]=bar")
+        assert_equal({ "foo" => "bar" }, result)
+      end
+
+      assert_deprecated(ActionDispatch.deprecator) do
+        result = ActionDispatch::ParamBuilder.from_query_string("[foo][bar]=baz")
+        assert_equal({ "foo" => { "bar" => "baz" } }, result)
+      end
+    end
+  else
+    test "(rack 3) defaults to retaining leading bracket" do
+      result = ActionDispatch::ParamBuilder.from_query_string("[foo]=bar")
+      assert_equal({ "[foo]" => "bar" }, result)
+
+      result = ActionDispatch::ParamBuilder.from_query_string("[foo][bar]=baz")
+      assert_equal({ "[foo]" => { "bar" => "baz" } }, result)
+    end
+  end
+
+  test "configured for strict brackets" do
+    previous_brackets = ActionDispatch::ParamBuilder.ignore_leading_brackets
+    ActionDispatch::ParamBuilder.ignore_leading_brackets = false
+
+    result = ActionDispatch::ParamBuilder.from_query_string("[foo]=bar")
+    assert_equal({ "[foo]" => "bar" }, result)
+
+    result = ActionDispatch::ParamBuilder.from_query_string("[foo][bar]=baz")
+    assert_equal({ "[foo]" => { "bar" => "baz" } }, result)
+  ensure
+    ActionDispatch::ParamBuilder.ignore_leading_brackets = previous_brackets
+  end
+
+  test "configured for ignoring leading brackets" do
+    previous_brackets = ActionDispatch::ParamBuilder.ignore_leading_brackets
+    ActionDispatch::ParamBuilder.ignore_leading_brackets = true
+
+    result = ActionDispatch::ParamBuilder.from_query_string("[foo]=bar")
+    assert_equal({ "foo" => "bar" }, result)
+
+    result = ActionDispatch::ParamBuilder.from_query_string("[foo][bar]=baz")
+    assert_equal({ "foo" => { "bar" => "baz" } }, result)
+  ensure
+    ActionDispatch::ParamBuilder.ignore_leading_brackets = previous_brackets
+  end
+end

actionpack/test/dispatch/query_parser_test.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+
+class QueryParserTest < ActiveSupport::TestCase
+  test "simple query string" do
+    assert_equal [["foo", "bar"], ["baz", "quux"]], parsed_pairs("foo=bar&baz=quux")
+  end
+
+  test "query string with empty and missing values" do
+    assert_equal [["foo", "bar"], ["empty", ""], ["missing", nil], ["baz", "quux"]], parsed_pairs("foo=bar&empty=&missing&baz=quux")
+  end
+
+  test "custom separator" do
+    assert_equal [["foo", "bar"], ["baz", "quux"]], parsed_pairs("foo=bar;baz=quux", ";")
+  end
+
+  test "non-standard separator" do
+    assert_equal [["foo", "bar"], ["baz", "quux"]], parsed_pairs("foo=bar/baz=quux", "/")
+  end
+
+  test "mixed separators" do
+    assert_equal [["a", "aa"], ["b", "bb"], ["c", "cc"]], parsed_pairs("a=aa&b=bb;c=cc", "&;")
+  end
+
+  if ::Rack::RELEASE.start_with?("2.")
+    test "(rack 2) defaults to mixed separators" do
+      assert_deprecated(ActionDispatch.deprecator) do
+        assert_equal [["a", "aa"], ["b", "bb"], ["c", "cc"]], parsed_pairs("a=aa&b=bb;c=cc")
+      end
+    end
+  else
+    test "(rack 3) defaults to ampersand separator only" do
+      assert_equal [["a", "aa"], ["b", "bb;c=cc"]], parsed_pairs("a=aa&b=bb;c=cc")
+    end
+  end
+
+  test "configured for strict separator" do
+    previous_separator = ActionDispatch::QueryParser.strict_query_string_separator
+    ActionDispatch::QueryParser.strict_query_string_separator = true
+    assert_equal [["a", "aa"], ["b", "bb;c=cc"]], parsed_pairs("a=aa&b=bb;c=cc", "&")
+  ensure
+    ActionDispatch::QueryParser.strict_query_string_separator = previous_separator
+  end
+
+  test "configured for mixed separator" do
+    previous_separator = ActionDispatch::QueryParser.strict_query_string_separator
+    ActionDispatch::QueryParser.strict_query_string_separator = false
+    assert_equal [["a", "aa"], ["b", "bb"], ["c", "cc"]], parsed_pairs("a=aa&b=bb;c=cc", "&;")
+  ensure
+    ActionDispatch::QueryParser.strict_query_string_separator = previous_separator
+  end
+
+  private
+    def parsed_pairs(query, separator = nil)
+      ActionDispatch::QueryParser.each_pair(query, separator).to_a
+    end
+end