Raise ActionController::BadRequest for malformed parameter hashes.

Currently Rack raises a TypeError when it encounters a malformed or
ambiguous hash like `foo[]=bar&foo[4]=bar`. Rather than pass this
through to the application this commit captures the exception and
re-raises it using a new ActionController::BadRequest exception.

The new ActionController::BadRequest exception returns a 400 error
instead of the 500 error that would've been returned by the original
TypeError. This allows exception notification libraries to ignore
these errors if so desired.

Closes #3051

actionpack/CHANGELOG.md

@@ -1,5 +1,7 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Malformed query and request parameter hashes now raise ActionController::BadRequest. *Andrew White*
+
 *   Add `divider` option to `grouped_options_for_select` to generate a separator
     `optgroup` automatically, and deprecate `prompt` as third argument, in favor
     of using an options hash. *Nicholas Greenfield*

actionpack/lib/action_controller/metal/exceptions.rb

@@ -2,6 +2,9 @@ module ActionController
   class ActionControllerError < StandardError #:nodoc:
   end
 
+  class BadRequest < ActionControllerError #:nodoc:
+  end
+
   class RenderError < ActionControllerError #:nodoc:
   end
 
@@ -38,7 +41,7 @@ def initialize(message = nil)
 
   class UnknownHttpMethod < ActionControllerError #:nodoc:
   end
-  
+
   class UnknownFormat < ActionControllerError #:nodoc:
   end
 end

actionpack/lib/action_dispatch/http/request.rb

@@ -231,17 +231,24 @@ def session_options=(options)
 
     # Override Rack's GET method to support indifferent access
     def GET
-      @env["action_dispatch.request.query_parameters"] ||= (normalize_parameters(super) || {})
+      begin
+        @env["action_dispatch.request.query_parameters"] ||= (normalize_parameters(super) || {})
+      rescue TypeError => e
+        raise ActionController::BadRequest, "Invalid query parameters: #{e.message}"
+      end
     end
     alias :query_parameters :GET
 
     # Override Rack's POST method to support indifferent access
     def POST
-      @env["action_dispatch.request.request_parameters"] ||= (normalize_parameters(super) || {})
+      begin
+        @env["action_dispatch.request.request_parameters"] ||= (normalize_parameters(super) || {})
+      rescue TypeError => e
+        raise ActionController::BadRequest, "Invalid request parameters: #{e.message}"
+      end
     end
     alias :request_parameters :POST
 
-
     # Returns the authorization header regardless of whether it was specified directly or through one of the
     # proxy alternatives.
     def authorization

actionpack/lib/action_dispatch/middleware/exception_wrapper.rb

@@ -12,7 +12,8 @@ class ExceptionWrapper
       'ActionController::MethodNotAllowed'         => :method_not_allowed,
       'ActionController::NotImplemented'           => :not_implemented,
       'ActionController::UnknownFormat'            => :not_acceptable,
-      'ActionController::InvalidAuthenticityToken' => :unprocessable_entity
+      'ActionController::InvalidAuthenticityToken' => :unprocessable_entity,
+      'ActionController::BadRequest'               => :bad_request
     )
 
     cattr_accessor :rescue_templates

actionpack/test/dispatch/debug_exceptions_test.rb

@@ -35,6 +35,8 @@ def call(env)
         raise ActionController::InvalidAuthenticityToken
       when "/not_found_original_exception"
         raise ActionView::Template::Error.new('template', AbstractController::ActionNotFound.new)
+      when "/bad_request"
+        raise ActionController::BadRequest
       else
         raise "puke!"
       end
@@ -88,6 +90,10 @@ def call(env)
     get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true}
     assert_response 405
     assert_match(/ActionController::MethodNotAllowed/, body)
+
+    get "/bad_request", {}, {'action_dispatch.show_exceptions' => true}
+    assert_response 400
+    assert_match(/ActionController::BadRequest/, body)
   end
 
   test "does not show filtered parameters" do

actionpack/test/dispatch/request/query_string_parsing_test.rb

@@ -105,6 +105,17 @@ def teardown
     )
   end
 
+  test "ambiguous query string returns a bad request" do
+    with_routing do |set|
+      set.draw do
+        get ':action', :to => ::QueryStringParsingTest::TestController
+      end
+
+      get "/parse", nil, "QUERY_STRING" => "foo[]=bar&foo[4]=bar"
+      assert_response :bad_request
+    end
+  end
+
   private
     def assert_parses(expected, actual)
       with_routing do |set|

actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb

@@ -126,6 +126,17 @@ def teardown
     assert_parses expected, query
   end
 
+  test "ambiguous params returns a bad request" do
+    with_routing do |set|
+      set.draw do
+        post ':action', :to => ::UrlEncodedParamsParsingTest::TestController
+      end
+
+      post "/parse", "foo[]=bar&foo[4]=bar"
+      assert_response :bad_request
+    end
+  end
+
   private
     def with_test_routing
       with_routing do |set|

actionpack/test/dispatch/request_test.rb

@@ -561,7 +561,7 @@ def url_for(options = {})
     begin
       request = stub_request(mock_rack_env)
       request.parameters
-    rescue TypeError
+    rescue ActionController::BadRequest
       # rack will raise a TypeError when parsing this query string
     end
     assert_equal({}, request.parameters)