Catch invalid UTF-8 encodings on ActionDispatch::Http::Request#POST (#40124)

* Add binary encoding logic into ActionDispatch::Request::Utils

Moving the logic to set binary encoding into ActionDispatch::Request::Utils
will allow us to encode from GET and POST in ActionDispatch::Request.

* Refactor binary encoding logic

- Move binary encoding calls into GET, POST and path_parameters
- Remove binary encoding from ActionDispatch::Http::Request
- This way, we only raise an invalid encoding exception if the controller is not requesting
parameters in binary encoding

* Check if encoding is valid in ActionDispatch::Request#POST and raise BadRequest if invalid

* Fix multipart_params_test that has binary-encoded params containing invalid UTF-8 characters

* Address PR comments

* Pass action and controller to Request::Utils.set_binary_encoding

[Rafael Mendonça França + Adrianna Chang]

actionpack/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Catch invalid UTF-8 parameters for POST requests and respond with BadRequest.
+
+    Additionally, perform `#set_binary_encoding` in `ActionDispatch::Http::Request#GET` and
+    `ActionDispatch::Http::Request#POST` prior to validating encoding.
+
+    *Adrianna Chang*
+
 *   Allow `assert_recognizes` routing assertions to work on mounted root routes.
 
     *Gannon McGibbon*

actionpack/lib/action_dispatch/http/parameters.rb

@@ -57,7 +57,6 @@ module ActionDispatch
                    query_parameters.dup
                  end
         params.merge!(path_parameters)
-        params = set_binary_encoding(params, params[:controller], params[:action])
         set_header("action_dispatch.request.parameters", params)
         params
       end
@@ -66,7 +65,7 @@ module ActionDispatch
       def path_parameters=(parameters) #:nodoc:
         delete_header("action_dispatch.request.parameters")
 
-        parameters = set_binary_encoding(parameters, parameters[:controller], parameters[:action])
+        parameters = Request::Utils.set_binary_encoding(self, parameters, parameters[:controller], parameters[:action])
         # If any of the path parameters has an invalid encoding then
         # raise since it's likely to trigger errors further on.
         Request::Utils.check_param_encoding(parameters)
@@ -85,23 +84,6 @@ module ActionDispatch
       end
 
       private
-        def set_binary_encoding(params, controller, action)
-          return params unless controller && controller.valid_encoding?
-
-          if binary_params_for?(controller, action)
-            ActionDispatch::Request::Utils.each_param_value(params.except(:controller, :action)) do |param|
-              param.force_encoding ::Encoding::ASCII_8BIT
-            end
-          end
-          params
-        end
-
-        def binary_params_for?(controller, action)
-          controller_class_for(controller).binary_params_for?(action)
-        rescue MissingController
-          false
-        end
-
         def parse_formatted_parameters(parsers)
           return yield if content_length.zero? || content_mime_type.nil?
 

actionpack/lib/action_dispatch/http/request.rb

@@ -379,6 +379,9 @@ module ActionDispatch
     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)
@@ -394,6 +397,8 @@ module ActionDispatch
         pr = parse_formatted_parameters(params_parsers) do |params|
           super || {}
         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)
       end
     rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e

actionpack/lib/action_dispatch/request/utils.rb

@@ -41,6 +41,10 @@ module ActionDispatch
         end
       end
 
+      def self.set_binary_encoding(request, params, controller, action)
+        BinaryParamEncoder.encode(request, params, controller, action)
+      end
+
       class ParamEncoder # :nodoc:
         # Convert nested Hash to HashWithIndifferentAccess.
         def self.normalize_encode_params(params)
@@ -73,6 +77,26 @@ module ActionDispatch
           list
         end
       end
+
+      class BinaryParamEncoder # :nodoc:
+        def self.encode(request, params, controller, action)
+          return params unless controller && controller.valid_encoding?
+
+          if binary_params_for?(request, controller, action)
+            ActionDispatch::Request::Utils.each_param_value(params.except(:controller, :action)) do |param|
+              param.force_encoding ::Encoding::ASCII_8BIT
+            end
+          end
+
+          params
+        end
+
+        def self.binary_params_for?(request, controller, action)
+          request.controller_class_for(controller).binary_params_for?(action)
+        rescue MissingController
+          false
+        end
+      end
     end
   end
 end

actionpack/test/dispatch/request/multipart_params_parsing_test.rb

@@ -6,6 +6,20 @@ class MultipartParamsParsingTest < ActionDispatch::IntegrationTest
   class TestController < ActionController::Base
     class << self
       attr_accessor :last_request_parameters, :last_parameters
+
+      def binary_params_for?(action)
+        action == "parse_binary"
+      end
+    end
+
+    def parse_binary
+      self.class.last_request_parameters = begin
+        request.request_parameters
+      rescue EOFError
+        {}
+      end
+      self.class.last_parameters = request.parameters
+      head :ok
     end
 
     def parse
@@ -118,7 +132,7 @@ class MultipartParamsParsingTest < ActionDispatch::IntegrationTest
   end
 
   test "parses mixed files" do
-    params = parse_multipart("mixed_files")
+    params = parse_multipart("mixed_files", "/parse_binary")
     assert_equal %w(files foo), params.keys.sort
     assert_equal "bar", params["foo"]
 
@@ -180,10 +194,10 @@ class MultipartParamsParsingTest < ActionDispatch::IntegrationTest
       end
     end
 
-    def parse_multipart(name)
+    def parse_multipart(name, path = "/parse")
       with_test_routing do
         headers = fixture(name)
-        post "/parse", params: headers.delete("rack.input"), headers: headers
+        post path, params: headers.delete("rack.input"), headers: headers
         assert_response :ok
         TestController.last_request_parameters
       end

actionpack/test/dispatch/request_test.rb

@@ -1051,6 +1051,43 @@ class RequestParameters < BaseRequestTest
     assert_raises(ActionController::BadRequest) { request.parameters }
   end
 
+  test "POST parameters containing invalid UTF8 character" do
+    data = "foo=%81E"
+    request = stub_request(
+      "REQUEST_METHOD" => "POST",
+      "CONTENT_LENGTH" => data.length,
+      "CONTENT_TYPE" => "application/x-www-form-urlencoded; charset=utf-8",
+      "rack.input" => StringIO.new(data)
+    )
+
+    err = assert_raises(ActionController::BadRequest) { request.parameters }
+
+    assert_predicate err.message, :valid_encoding?
+    assert_equal "Invalid request parameters: Invalid encoding for parameter: <20>E", err.message
+  end
+
+  test "query parameters specified as ASCII_8BIT encoded do not raise InvalidParameterError" do
+    request = stub_request("QUERY_STRING" => "foo=%81E")
+
+    ActionDispatch::Request::Utils.stub(:set_binary_encoding, { "foo" => "\x81E".b }) do
+      request.parameters
+    end
+  end
+
+  test "POST parameters specified as ASCII_8BIT encoded do not raise InvalidParameterError" do
+    data = "foo=%81E"
+    request = stub_request(
+      "REQUEST_METHOD" => "POST",
+      "CONTENT_LENGTH" => data.length,
+      "CONTENT_TYPE" => "application/x-www-form-urlencoded; charset=utf-8",
+      "rack.input" => StringIO.new(data)
+    )
+
+    ActionDispatch::Request::Utils.stub(:set_binary_encoding, { "foo" => "\x81E".b }) do
+      request.parameters
+    end
+  end
+
   test "parameters not accessible after rack parse error 1" do
     request = stub_request(
       "REQUEST_METHOD" => "POST",