Path parameters should default to UTF8

This commit changes the behavior such the path_params now default to
UTF8 just like regular parameters. This also changes the behavior such
that if a path parameter contains invalid UTF8 it returns a 400 bad
request. Previously the behavior was to encode the path params as binary
but that's not the same as query params.

So this commit makes path params behave the same as query params.

It's important to test with a path that's encoded as binary because
that's how paths are encoded from the socket. The test that was altered
was changed to make the behavior for bad encoding the same as query
params. We want to treat path params the same as query params. The params
in the test are invalid UTF8 so they should return a bad request.

Fixes #29669

*Eileen M. Uchitelle, Aaron Patterson, & Tsukuru Tanimichi*

actionpack/lib/action_dispatch/http/parameters.rb

@@ -57,7 +57,7 @@ def parameters
                    query_parameters.dup
                  end
         params.merge!(path_parameters)
-        params = set_binary_encoding(params)
+        params = set_binary_encoding(params, params[:controller], params[:action])
         set_header("action_dispatch.request.parameters", params)
         params
       end
@@ -66,6 +66,7 @@ def parameters
       def path_parameters=(parameters) #:nodoc:
         delete_header("action_dispatch.request.parameters")
 
+        parameters = set_binary_encoding(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,9 +86,10 @@ def path_parameters
 
       private
 
-        def set_binary_encoding(params)
-          action = params[:action]
-          if binary_params_for?(action)
+        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) do |param|
               param.force_encoding ::Encoding::ASCII_8BIT
             end
@@ -95,8 +97,8 @@ def set_binary_encoding(params)
           params
         end
 
-        def binary_params_for?(action)
-          controller_class.binary_params_for?(action)
+        def binary_params_for?(controller, action)
+          controller_class_for(controller).binary_params_for?(action)
         rescue NameError
           false
         end

actionpack/lib/action_dispatch/http/request.rb

@@ -76,10 +76,13 @@ def self.binary_params_for?(action); false; end
 
     def controller_class
       params = path_parameters
+      params[:action] ||= "index"
+      controller_class_for(params[:controller])
+    end
 
-      if params.key?(:controller)
-        controller_param = params[:controller].underscore
-        params[:action] ||= "index"
+    def controller_class_for(name)
+      if name
+        controller_param = name.underscore
         const_name = "#{controller_param.camelize}Controller"
         ActiveSupport::Dependencies.constantize(const_name)
       else

actionpack/lib/action_dispatch/journey/router.rb

@@ -43,6 +43,10 @@ def serve(req)
             req.path_info = "/" + req.path_info unless req.path_info.start_with? "/"
           end
 
+          parameters = route.defaults.merge parameters.transform_values { |val|
+            val.dup.force_encoding(::Encoding::UTF_8)
+          }
+
           req.path_parameters = set_params.merge parameters
 
           status, headers, body = route.app.serve(req)
@@ -67,6 +71,7 @@ def recognize(rails_req)
             rails_req.path_info   = match.post_match.sub(/^([^\/])/, '/\1')
           end
 
+          parameters = route.defaults.merge parameters
           yield(route, parameters)
         end
       end
@@ -119,7 +124,7 @@ def find_routes(req)
 
           routes.map! { |r|
             match_data = r.path.match(req.path_info)
-            path_parameters = r.defaults.dup
+            path_parameters = {}
             match_data.names.zip(match_data.captures) { |name, val|
               path_parameters[name.to_sym] = Utils.unescape_uri(val) if val
             }

actionpack/test/controller/routing_test.rb

@@ -96,6 +96,22 @@ def test_symbols_with_dashes
     assert_equal({ "artist" => "journey", "song" => "faithfully" }, hash)
   end
 
+  def test_id_encoding
+    rs.draw do
+      get "/journey/:id", to: lambda { |env|
+        param = ActionDispatch::Request.new(env).path_parameters
+        resp = ActiveSupport::JSON.encode param
+        [200, {}, [resp]]
+      }
+    end
+
+    # The encoding of the URL in production is *binary*, so we add a
+    # .b here.
+    hash = ActiveSupport::JSON.decode get(URI("http://example.org/journey/%E5%A4%AA%E9%83%8E".b))
+    assert_equal({ "id" => "太郎" }, hash)
+    assert_equal ::Encoding::UTF_8, hash["id"].encoding
+  end
+
   def test_id_with_dash
     rs.draw do
       get "/journey/:id", to: lambda { |env|

actionpack/test/dispatch/routing_test.rb

@@ -4416,6 +4416,10 @@ def app; APP end
 
 class TestInvalidUrls < ActionDispatch::IntegrationTest
   class FooController < ActionController::Base
+    def self.binary_params_for?(action)
+      action == "show"
+    end
+
     def show
       render plain: "foo#show"
     end
@@ -4437,19 +4441,19 @@ def show
       end
 
       get "/%E2%EF%BF%BD%A6"
-      assert_response :not_found
+      assert_response :bad_request
 
       get "/foo/%E2%EF%BF%BD%A6"
-      assert_response :not_found
+      assert_response :bad_request
 
       get "/foo/show/%E2%EF%BF%BD%A6"
       assert_response :ok
 
       get "/bar/%E2%EF%BF%BD%A6"
-      assert_response :redirect
+      assert_response :bad_request
 
       get "/foobar/%E2%EF%BF%BD%A6"
-      assert_response :ok
+      assert_response :bad_request
     end
   end
 end