Do more request parameter parsing ourselves

matthewd · matthewd · commit ce5f1817bc23 · 2024-10-11T19:55:03.000+10:30
We still rely on Rack to actually read the input stream, but by
constructing the parameter hashes ourselves, we restore our ability to
extend the meaning of parameter-name formatting in the future, while
also concentrating all our preprocessing into a single walk.

This is an important step in defining a more consistent boundary to our
Rack dependency, allowing them to evolve their behaviour with less
chance of creating some edge-case incompatibility.
diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb
@@ -53,7 +53,13 @@ class MissingController < NameError
   eager_autoload do
     autoload_under "http" do
       autoload :ContentSecurityPolicy
+      autoload :InvalidParameterError, "action_dispatch/http/param_error"
+      autoload :ParamBuilder
+      autoload :ParamError
+      autoload :ParameterTypeError, "action_dispatch/http/param_error"
+      autoload :ParamsTooDeepError, "action_dispatch/http/param_error"
       autoload :PermissionsPolicy
+      autoload :QueryParser
       autoload :Request
       autoload :Response
     end
diff --git a/actionpack/lib/action_dispatch/http/param_builder.rb b/actionpack/lib/action_dispatch/http/param_builder.rb
@@ -0,0 +1,163 @@
+# frozen_string_literal: true
+
+module ActionDispatch
+  class ParamBuilder
+    def self.make_default(param_depth_limit)
+      new param_depth_limit
+    end
+
+    attr_reader :param_depth_limit
+
+    def initialize(param_depth_limit)
+      @param_depth_limit = param_depth_limit
+    end
+
+    cattr_accessor :default
+    self.default = make_default(100)
+
+    class << self
+      delegate :from_query_string, :from_pairs, :from_hash, to: :default
+    end
+
+    def from_query_string(qs, separator: nil, encoding_template: nil)
+      from_pairs QueryParser.each_pair(qs, separator), encoding_template: encoding_template
+    end
+
+    def from_pairs(pairs, encoding_template: nil)
+      params = make_params
+
+      pairs.each do |k, v|
+        if Hash === v
+          v = ActionDispatch::Http::UploadedFile.new(v)
+        end
+
+        store_nested_param(params, k, v, 0, encoding_template)
+      end
+
+      params
+    rescue ArgumentError => e
+      raise InvalidParameterError, e.message, e.backtrace
+    end
+
+    def from_hash(hash, encoding_template: nil)
+      # Force encodings from encoding template
+      hash = Request::Utils::CustomParamEncoder.encode_for_template(hash, encoding_template)
+
+      # Assert valid encoding
+      Request::Utils.check_param_encoding(hash)
+
+      # Convert hashes to HWIA (or UploadedFile), and deep-munge nils
+      # out of arrays
+      hash = Request::Utils.normalize_encode_params(hash)
+
+      hash
+    end
+
+    private
+      def store_nested_param(params, name, v, depth, encoding_template = nil)
+        raise ParamsTooDeepError if depth >= param_depth_limit
+
+        if !name
+          # 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]
+          else
+            # Plain parameter with no nesting
+            k = name
+            after = ""
+          end
+        elsif name.start_with?("[]")
+          # Array nesting
+          k = "[]"
+          after = name[2, name.length]
+        elsif name.start_with?("[") && (start = name.index("]", 1))
+          # Hash nesting, use the part inside brackets as the key
+          k = name[1, start - 1]
+          after = name[start + 1, name.length]
+        else
+          # Probably malformed input, nested but not starting with [
+          # treat full name as key for backwards compatibility.
+          k = name
+          after = ""
+        end
+
+        return if k.empty?
+
+        if depth == 0 && String === v
+          # We have to wait until we've found the top part of the name,
+          # because that's what the encoding template is configured with
+          if encoding_template && (designated_encoding = encoding_template[k]) && !v.frozen?
+            v.force_encoding(designated_encoding)
+          end
+
+          # ... and we can't validate the encoding until after we've
+          # applied any template override
+          unless v.valid_encoding?
+            raise InvalidParameterError, "Invalid encoding for parameter: #{v.scrub}"
+          end
+        end
+
+        if after == ""
+          if k == "[]" && depth != 0
+            return (v || !ActionDispatch::Request::Utils.perform_deep_munge) ? [v] : []
+          else
+            params[k] = v
+          end
+        elsif after == "["
+          params[name] = v
+        elsif after == "[]"
+          params[k] ||= []
+          raise ParameterTypeError, "expected Array (got #{params[k].class.name}) for param `#{k}'" unless params[k].is_a?(Array)
+          params[k] << v if v || !ActionDispatch::Request::Utils.perform_deep_munge
+        elsif after.start_with?("[]")
+          # Recognize x[][y] (hash inside array) parameters
+          unless after[2] == "[" && after.end_with?("]") && (child_key = after[3, after.length - 4]) && !child_key.empty? && !child_key.index("[") && !child_key.index("]")
+            # Handle other nested array parameters
+            child_key = after[2, after.length]
+          end
+          params[k] ||= []
+          raise ParameterTypeError, "expected Array (got #{params[k].class.name}) for param `#{k}'" unless params[k].is_a?(Array)
+          if params_hash_type?(params[k].last) && !params_hash_has_key?(params[k].last, child_key)
+            store_nested_param(params[k].last, child_key, v, depth + 1)
+          else
+            params[k] << store_nested_param(make_params, child_key, v, depth + 1)
+          end
+        else
+          params[k] ||= make_params
+          raise ParameterTypeError, "expected Hash (got #{params[k].class.name}) for param `#{k}'" unless params_hash_type?(params[k])
+          params[k] = store_nested_param(params[k], after, v, depth + 1)
+        end
+
+        params
+      end
+
+      def make_params
+        ActiveSupport::HashWithIndifferentAccess.new
+      end
+
+      def new_depth_limit(param_depth_limit)
+        self.class.new @params_class, param_depth_limit
+      end
+
+      def params_hash_type?(obj)
+        Hash === obj
+      end
+
+      def params_hash_has_key?(hash, key)
+        return false if key.include?("[]")
+
+        key.split(/[\[\]]+/).inject(hash) do |h, part|
+          next h if part == ""
+          return false unless params_hash_type?(h) && h.key?(part)
+          h[part]
+        end
+
+        true
+      end
+  end
+end
diff --git a/actionpack/lib/action_dispatch/http/param_error.rb b/actionpack/lib/action_dispatch/http/param_error.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module ActionDispatch
+  class ParamError < ActionDispatch::Http::Parameters::ParseError
+    def initialize(message = nil)
+      super
+    end
+
+    def self.===(other)
+      super || (
+        defined?(Rack::Utils::ParameterTypeError) && Rack::Utils::ParameterTypeError === other ||
+        defined?(Rack::Utils::InvalidParameterError) && Rack::Utils::InvalidParameterError === other ||
+        defined?(Rack::QueryParser::ParamsTooDeepError) && Rack::QueryParser::ParamsTooDeepError === other
+      )
+    end
+  end
+
+  class ParameterTypeError < ParamError
+  end
+
+  class InvalidParameterError < ParamError
+  end
+
+  class ParamsTooDeepError < ParamError
+  end
+end
diff --git a/actionpack/lib/action_dispatch/http/query_parser.rb b/actionpack/lib/action_dispatch/http/query_parser.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require "uri"
+
+module ActionDispatch
+  class QueryParser
+    DEFAULT_SEP = /& */n
+    COMMON_SEP = { ";" => /; */n, ";," => /[;,] */n, "&" => /& */n }
+
+    #--
+    # Note this departs from WHATWG's specified parsing algorithm by
+    # giving a nil value for keys that do not use '='. Callers that need
+    # the standard's interpretation can use `v.to_s`.
+    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|
+        next if part.empty?
+
+        k, v = part.split("=", 2)
+
+        k = URI.decode_www_form_component(k)
+        v &&= URI.decode_www_form_component(v)
+
+        yield k, v
+      end
+
+      nil
+    end
+  end
+end
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
@@ -63,6 +63,9 @@ def self.empty
 
     def initialize(env)
       super
+
+      @rack_request = Rack::Request.new(env)
+
       @method            = nil
       @request_method    = nil
       @remote_ip         = nil
@@ -71,6 +74,8 @@ def initialize(env)
       @ip                = nil
     end
 
+    attr_reader :rack_request
+
     def commit_cookie_jar! # :nodoc:
     end
 
@@ -388,34 +393,67 @@ def session_options=(options)
     # Override Rack's GET method to support indifferent access.
     def GET
       fetch_header("action_dispatch.request.query_parameters") do |k|
-        rack_query_params = super || {}
-        controller = path_parameters[:controller]
-        action = path_parameters[:action]
-        rack_query_params = Request::Utils.set_binary_encoding(self, rack_query_params, controller, action)
-        # Check for non UTF-8 parameter values, which would cause errors later
-        Request::Utils.check_param_encoding(rack_query_params)
-        set_header k, Request::Utils.normalize_encode_params(rack_query_params)
+        encoding_template = Request::Utils::CustomParamEncoder.action_encoding_template(self, path_parameters[:controller], path_parameters[:action])
+        rack_query_params = ActionDispatch::ParamBuilder.from_query_string(rack_request.query_string, encoding_template: encoding_template)
+
+        set_header k, rack_query_params
       end
-    rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError, Rack::QueryParser::ParamsTooDeepError => e
+    rescue ActionDispatch::ParamError => e
       raise ActionController::BadRequest.new("Invalid query parameters: #{e.message}")
     end
     alias :query_parameters :GET
 
     # Override Rack's POST method to support indifferent access.
     def POST
       fetch_header("action_dispatch.request.request_parameters") do
-        pr = parse_formatted_parameters(params_parsers) do |params|
-          super || {}
+        encoding_template = Request::Utils::CustomParamEncoder.action_encoding_template(self, path_parameters[:controller], path_parameters[:action])
+
+        param_list = nil
+        pr = parse_formatted_parameters(params_parsers) do
+          if param_list = request_parameters_list
+            ActionDispatch::ParamBuilder.from_pairs(param_list, encoding_template: encoding_template)
+          else
+            # We're not using a version of Rack that provides raw form
+            # pairs; we must use its hash (and thus post-process it below).
+            fallback_request_parameters
+          end
+        end
+
+        # If the request body was parsed by a custom parser like JSON
+        # (and thus the above block was not run), we need to
+        # post-process the result hash.
+        if param_list.nil?
+          pr = ActionDispatch::ParamBuilder.from_hash(pr, encoding_template: encoding_template)
         end
-        pr = Request::Utils.set_binary_encoding(self, pr, path_parameters[:controller], path_parameters[:action])
-        Request::Utils.check_param_encoding(pr)
-        self.request_parameters = Request::Utils.normalize_encode_params(pr)
+
+        self.request_parameters = pr
       end
-    rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError, Rack::QueryParser::ParamsTooDeepError, EOFError => e
+    rescue ActionDispatch::ParamError, EOFError => e
       raise ActionController::BadRequest.new("Invalid request parameters: #{e.message}")
     end
     alias :request_parameters :POST
 
+    def request_parameters_list
+      # We don't use Rack's parse result, but we must call it so Rack
+      # can populate the rack.request.* keys we need.
+      rack_post = rack_request.POST
+
+      if form_pairs = get_header("rack.request.form_pairs")
+        # Multipart
+        form_pairs
+      elsif form_vars = get_header("rack.request.form_vars")
+        # URL-encoded
+        ActionDispatch::QueryParser.each_pair(form_vars)
+      elsif rack_post && !rack_post.empty?
+        # It was multipart, but Rack did not preserve a pair list
+        # (probably too old). Flat parameter list is not available.
+        nil
+      else
+        # No request body, or not a format Rack knows
+        []
+      end
+    end
+
     # Returns the authorization header regardless of whether it was specified
     # directly or through one of the proxy alternatives.
     def authorization
@@ -492,6 +530,10 @@ def reset_stream(body_stream)
           yield
         end
       end
+
+      def fallback_request_parameters
+        rack_request.POST
+      end
   end
 end
 
diff --git a/actionpack/lib/action_dispatch/request/utils.rb b/actionpack/lib/action_dispatch/request/utils.rb
@@ -83,8 +83,8 @@ def self.handle_array(params)
       end
 
       class CustomParamEncoder # :nodoc:
-        def self.encode(request, params, controller, action)
-          return params unless encoding_template = action_encoding_template(request, controller, action)
+        def self.encode_for_template(params, encoding_template)
+          return params unless encoding_template
           params.except(:controller, :action).each do |key, value|
             ActionDispatch::Request::Utils.each_param_value(value) do |param|
               # If `param` is frozen, it comes from the router defaults
@@ -98,6 +98,11 @@ def self.encode(request, params, controller, action)
           params
         end
 
+        def self.encode(request, params, controller, action)
+          encoding_template = action_encoding_template(request, controller, action)
+          encode_for_template(params, encoding_template)
+        end
+
         def self.action_encoding_template(request, controller, action) # :nodoc:
           controller && controller.valid_encoding? &&
             request.controller_class_for(controller).action_encoding_template(action)
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
diff --git a/railties/test/application/middleware/exceptions_test.rb b/railties/test/application/middleware/exceptions_test.rb
