diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 3a4b52cc58fbcdfc0a436287f57d529355fc6f12..22af4e43e52f7c688aafe305ae9a3a7cefff64c5 100644 --- a/actionpack/CHANGELOG.md +++ b/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* diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index 909132a05b01f9f7801f59f413397bbc80a0e373..37c41ba356c7c50975d9145a2158cdc6ca490496 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -57,7 +57,6 @@ def parameters 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 @@ def parameters 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 @@ def path_parameters 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? diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index ed9ac6922e5a49ab3216e4b3e18715a59c36ba6d..b0ea368c580d1553b77bb64743cef48399f9daec 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -379,6 +379,9 @@ def session_options=(options) 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 @@ def POST 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 diff --git a/actionpack/lib/action_dispatch/request/utils.rb b/actionpack/lib/action_dispatch/request/utils.rb index 8ec933069748a7babf6df2e9db7ec724e82c3c56..e2e5023f38e049041fc29f1a8820834681eee101 100644 --- a/actionpack/lib/action_dispatch/request/utils.rb +++ b/actionpack/lib/action_dispatch/request/utils.rb @@ -41,6 +41,10 @@ def self.check_param_encoding(params) 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 @@ def self.handle_array(params) 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 diff --git a/actionpack/test/dispatch/request/multipart_params_parsing_test.rb b/actionpack/test/dispatch/request/multipart_params_parsing_test.rb index da8233c074cf7c17fb5390326b470110bba9e356..01a06bf2db6e5261f86d8a8c6481101a6f6442ec 100644 --- a/actionpack/test/dispatch/request/multipart_params_parsing_test.rb +++ b/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 @@ def teardown 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 @@ def fixture(name) 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 diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index e0ee910227a50d70fccb656ae871218752701f17..ad3c1432d37c17d55ca64dc5f81c4929a55f04e7 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/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: �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",