Merge pull request rails#53471 from matthewd/rack-2-parsing

Maintain Rack 2 parameter parsing behaviour

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