Merge pull request #25816 from greysteil/check-path-param-encoding

Check `request.path_parameters` encoding at the point they're set

6 files changed, 35 insertions, 29 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 1bb37d661a..1aeb58a612 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,13 @@
+*   Check `request.path_parameters` encoding at the point they're set
+
+    Check for any non-UTF8 characters in path parameters at the point they're
+    set in `env`. Previously they were checked for when used to get a controller
+    class, but this meant routes that went directly to a Rack app, or skipped
+    controller instantiation for some other reason, had to defend against
+    non-UTF8 characters themselves.
+
+    *Grey Baker*
+
 *   Don't raise ActionController::UnknownHttpMethod from ActionDispatch::Static
 
     Pass `Rack::Request` objects to `ActionDispatch::FileHandler` to avoid it
diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb
index ff5031d7d5..3f0e51790c 100644
--- a/actionpack/lib/action_dispatch/http/parameters.rb
+++ b/actionpack/lib/action_dispatch/http/parameters.rb
@@ -44,7 +44,14 @@ module ActionDispatch
 
       def path_parameters=(parameters) #:nodoc:
         delete_header('action_dispatch.request.parameters')
+
+        # 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)
+
         set_header PARAMETERS_KEY, parameters
+      rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
+        raise ActionController::BadRequest.new("Invalid path parameters: #{e.message}")
       end
 
       # Returns a hash with the \parameters used to form the \path of the request.
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index b0ed681623..954dd4f354 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -66,24 +66,12 @@ module ActionDispatch
     def commit_cookie_jar! # :nodoc:
     end
 
-    def check_path_parameters!
-      # If any of the path parameters has an invalid encoding then
-      # raise since it's likely to trigger errors further on.
-      path_parameters.each do |key, value|
-        next unless value.respond_to?(:valid_encoding?)
-        unless value.valid_encoding?
-          raise ActionController::BadRequest, "Invalid parameter encoding: #{key} => #{value.inspect}"
-        end
-      end
-    end
-
     PASS_NOT_FOUND = Class.new { # :nodoc:
       def self.action(_); self; end
       def self.call(_); [404, {'X-Cascade' => 'pass'}, []]; end
     }
 
     def controller_class
-      check_path_parameters!
       params = path_parameters
 
       if params.key?(:controller)
diff --git a/actionpack/lib/action_dispatch/routing/redirection.rb b/actionpack/lib/action_dispatch/routing/redirection.rb
index d6987f4d09..3265caa00b 100644
--- a/actionpack/lib/action_dispatch/routing/redirection.rb
+++ b/actionpack/lib/action_dispatch/routing/redirection.rb
@@ -22,7 +22,6 @@ module ActionDispatch
       end
 
       def serve(req)
-        req.check_path_parameters!
         uri = URI.parse(path(req.path_parameters, req))
 
         unless uri.host
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index 8a5d85ab84..634f6d80c4 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -1018,17 +1018,13 @@ class RequestParameters < BaseRequestTest
   end
 
   test "path parameters with invalid UTF8 encoding" do
-    request = stub_request(
-      "action_dispatch.request.path_parameters" => { foo: "\xBE" }
-    )
+    request = stub_request
 
     err = assert_raises(ActionController::BadRequest) do
-      request.check_path_parameters!
+      request.path_parameters = { foo: "\xBE" }
     end
 
-    assert_match "Invalid parameter encoding", err.message
-    assert_match "foo", err.message
-    assert_match "\\xBE", err.message
+    assert_equal "Invalid path parameters: Non UTF-8 value: \xBE", err.message
   end
 
   test "parameters not accessible after rack parse error of invalid UTF8 character" do
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index d54cdf7247..5298e63fef 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -4331,15 +4331,16 @@ class TestInvalidUrls < ActionDispatch::IntegrationTest
 
   test "invalid UTF-8 encoding returns a 400 Bad Request" do
     with_routing do |set|
-      ActiveSupport::Deprecation.silence do
-        set.draw do
-          get "/bar/:id", :to => redirect("/foo/show/%{id}")
-          get "/foo/show(/:id)", :to => "test_invalid_urls/foo#show"
+      set.draw do
+        get "/bar/:id", :to => redirect("/foo/show/%{id}")
+        get "/foo/show(/:id)", :to => "test_invalid_urls/foo#show"
 
-          ActiveSupport::Deprecation.silence do
-            get "/foo(/:action(/:id))", :controller => "test_invalid_urls/foo"
-            get "/:controller(/:action(/:id))"
-          end
+        ok = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }
+        get '/foobar/:id', to: ok
+
+        ActiveSupport::Deprecation.silence do
+          get "/foo(/:action(/:id))", :controller => "test_invalid_urls/foo"
+          get "/:controller(/:action(/:id))"
         end
       end
 
@@ -4354,6 +4355,9 @@ class TestInvalidUrls < ActionDispatch::IntegrationTest
 
       get "/bar/%E2%EF%BF%BD%A6"
       assert_response :bad_request
+
+      get "/foobar/%E2%EF%BF%BD%A6"
+      assert_response :bad_request
     end
   end
 end
@@ -4774,7 +4778,9 @@ class TestPathParameters < ActionDispatch::IntegrationTest
         end
       end
 
-      get ':controller(/:action/(:id))'
+      ActiveSupport::Deprecation.silence do
+        get ':controller(/:action/(:id))'
+      end
     end
   end