Catch invalid UTF-8 querystring values and respond with BadRequest

4 files changed, 37 insertions, 7 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index a3b4f6e989..4f95a9bab9 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Catch invalid UTF-8 querystring values and respond with BadRequest
+
+    Check querystring params for invalid UTF-8 characters, and raise an
+    ActionController::BadRequest error if present. Previously these strings
+    would typically trigger errors further down the stack.
+
+    *Grey Baker*
+
 *   Parse RSS/ATOM responses as XML, not HTML.
 
     *Alexander Kaupanin*
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index c6ab4dbc9a..35e3ac304f 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -338,7 +338,10 @@ module ActionDispatch
     # Override Rack's GET method to support indifferent access
     def GET
       fetch_header("action_dispatch.request.query_parameters") do |k|
-        set_header k, Request::Utils.normalize_encode_params(super || {})
+        rack_query_params = super || {}
+        # 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)
       end
     rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
       raise ActionController::BadRequest.new("Invalid query parameters: #{e.message}", e)
diff --git a/actionpack/lib/action_dispatch/request/utils.rb b/actionpack/lib/action_dispatch/request/utils.rb
index a8151a8224..bb3df3c311 100644
--- a/actionpack/lib/action_dispatch/request/utils.rb
+++ b/actionpack/lib/action_dispatch/request/utils.rb
@@ -13,6 +13,21 @@ module ActionDispatch
         end
       end
 
+      def self.check_param_encoding(params)
+        case params
+        when Array
+          params.each { |element| check_param_encoding(element) }
+        when Hash
+          params.each_value { |value| check_param_encoding(value) }
+        when String
+          unless params.valid_encoding?
+            # Raise Rack::Utils::InvalidParameterError for consistency with Rack.
+            # ActionDispatch::Request#GET will re-raise as a BadRequest error.
+            raise Rack::Utils::InvalidParameterError, "Non UTF-8 value: #{params}"
+          end
+        end
+      end
+
       class ParamEncoder # :nodoc:
         # Convert nested Hash to HashWithIndifferentAccess.
         #
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index dfedc8ae25..e9896a71f4 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -977,13 +977,17 @@ class RequestParameters < BaseRequestTest
 
   test "parameters not accessible after rack parse error of invalid UTF8 character" do
     request = stub_request("QUERY_STRING" => "foo%81E=1")
+    assert_raises(ActionController::BadRequest) { request.parameters }
+  end
 
-    2.times do
-      assert_raises(ActionController::BadRequest) do
-        # rack will raise a Rack::Utils::InvalidParameterError when parsing this query string
-        request.parameters
-      end
-    end
+  test "parameters containing an invalid UTF8 character" do
+    request = stub_request("QUERY_STRING" => "foo=%81E")
+    assert_raises(ActionController::BadRequest) { request.parameters }
+  end
+
+  test "parameters containing a deeply nested invalid UTF8 character" do
+    request = stub_request("QUERY_STRING" => "foo[bar]=%81E")
+    assert_raises(ActionController::BadRequest) { request.parameters }
   end
 
   test "parameters not accessible after rack parse error 1" do