Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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
  • Loading branch information...
commit 66eb3f02cc0894f08c4f912ba8bf6fb1f87e9a4a 1 parent 972376a
Andrew White pixeltrix authored
2  actionpack/CHANGELOG.md
View
@@ -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*
5 actionpack/lib/action_controller/metal/exceptions.rb
View
@@ -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
13 actionpack/lib/action_dispatch/http/request.rb
View
@@ -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
3  actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
View
@@ -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
6 actionpack/test/dispatch/debug_exceptions_test.rb
View
@@ -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
11 actionpack/test/dispatch/request/query_string_parsing_test.rb
View
@@ -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|
11 actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
View
@@ -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|
2  actionpack/test/dispatch/request_test.rb
View
@@ -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)
Please sign in to comment.
Something went wrong with that request. Please try again.