ActionController doesn't handle TypeError in Rack parameter parsing #3051

Closed
manlon opened this Issue Sep 16, 2011 · 11 comments

Projects

None yet

7 participants

@manlon
manlon commented Sep 16, 2011

Parsing a query string using Rack::Utils.parse_nested_query raises a TypeError when query parameters are ambiguous in that they seem to represent an Array and a Hash with the same name (e.g. f[]=&f[4]=). This is not handled by ActionController and so unparseable parameters result in a 500 error in a new app. (v3.1.0)

Since this happens pretty deep in the Rack code and TypeError is rather general it seems wrong to have to handle this in the rails app layer to prevent a 500.

It's easy to find examples of this issue in the wild:
https://github.com/?f[]=&f[4]=
http://twitter.com/?f[]=&f[4]=

I initially filed this bug against Rack (rack/rack#222) but it seems that ActionController may be the place to handle this.

@jeremy
Member
jeremy commented Oct 8, 2011

Do you have a patch + tests?

Ideally we could rescue_from a more specific error raised from Rack. Then we'd respond with a 400 Bad Request by default.

@jeremy
Member
jeremy commented Oct 8, 2011

Judging from the Rack feedback, they think generic TypeError is specific enough. So we should rescue that and re-raise a rack params parsing error. Then if the app doesn't rescue it, we default to giving a 400 Bad Request response. Seem reasonable?

@isaacsanders
Contributor

@manlon Is this still an issue?

@pixeltrix pixeltrix was assigned May 8, 2012
@pixeltrix
Member

@manlon yes, it's still an issue - I'll take a look at patching it.

@pixeltrix pixeltrix added a commit that closed this issue May 20, 2012
@pixeltrix pixeltrix 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
66eb3f0
@pixeltrix pixeltrix closed this in 66eb3f0 May 20, 2012
@lmars lmars pushed a commit to econsultancy/rails that referenced this issue Nov 29, 2012
@pixeltrix pixeltrix + Lewis Marshall 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

Conflicts:
	actionpack/CHANGELOG.md
	actionpack/lib/action_controller/metal/exceptions.rb
	actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
	actionpack/test/dispatch/debug_exceptions_test.rb
f2abf0d
@slonia
slonia commented Jun 11, 2013

Rails still process this error like 500, not 400. And I can't catch the TypeError in applicaton controller via rescue_from.
https://github.com/?f[]=&f[4]=
http://twitter.com/?f[]=&f[4]=
This links also render 500 server error

@pixeltrix
Member

@slonia have you tried it in Rails 4? The patch isn't in 3.2.13.

@slonia
slonia commented Jun 11, 2013

no, project I'm using is on rails branch: '3-2-stable'
Why this patch isn't in rails 3.2.13 if it is a year old?

@pixeltrix
Member

Because it's a change of behaviour so can't go in a patch release

@slonia
slonia commented Jun 11, 2013

@pixeltrix oh, thank you

@larryzhao

Hi @pixeltrix , I am using Rails 4.1.1 now, ActionController::BadRequest still could not be rescued by rescue_from in ApplicationController.

When I received a request like: http://127.0.0.1:3000/p/%E9, ActionController::BadRequest is raised from actionpack (4.1.1) lib/action_dispatch/routing/route_set.rb line #37, but I still couldn't catch it with rescue_from in ApplicationController.

Is there anything else to be done to catch it?

Thanks a lot.

@Fjan
Contributor
Fjan commented Jul 24, 2014

@larryzhao I think the error gets raised before the request ever reaches the controller, so it will never be possible to rescue it. You can put in a piece of middleware to catch the error on the way back out, this blog post has a pretty good explanation on how to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment