Add error handling for requests with invalid params (Fixes #1058) #1070

Merged
merged 1 commit into from May 9, 2016

Conversation

Projects
None yet
2 participants
@jkowens
Member

jkowens commented Jan 20, 2016

Requests that raise errors when parsing query parameters will now respond with a 400 status instead of 500 as suggested by the Rack docs. Fixes #1058. See #1058 for steps to reproduce.

Exceptions caused by parsing bad parameters were raised outside of the dispatch! method so the default exception handler was not invoked. That is why these exceptions bubbled up to the app server and exception messages were being displayed in the browser.

@jkowens jkowens changed the title from Add bad_request error handler to Add bad_request error handler (Fixes #1058) Jan 21, 2016

@jkowens jkowens changed the title from Add bad_request error handler (Fixes #1058) to Add error handling for requests with invalid params (Fixes #1058) Jan 22, 2016

@jkowens

This comment has been minimized.

Show comment
Hide comment
@jkowens

jkowens Jan 22, 2016

Member

I've iterated on this several times, but I think I've finally got it figured out. If someone could review this and provide comments/suggestions that would be awesome!

Member

jkowens commented Jan 22, 2016

I've iterated on this several times, but I think I've finally got it figured out. If someone could review this and provide comments/suggestions that would be awesome!

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Jan 24, 2016

Member

Yeesh, I'm afraid this issue is too ambitious to try to make into the next stable release.

I'm worried that my eyes are too weary to make this decision alone. Let's wait for more feedback.

Member

zzak commented Jan 24, 2016

Yeesh, I'm afraid this issue is too ambitious to try to make into the next stable release.

I'm worried that my eyes are too weary to make this decision alone. Let's wait for more feedback.

@zzak zzak added the feedback label Jan 24, 2016

@zzak

This comment has been minimized.

Show comment
Hide comment
Member

zzak commented Jan 24, 2016

Add error handling for bad requests (invalid params)
Requests that raise an error when parameters are parsed will
now respond with a 400 status instead of 500.
@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak May 9, 2016

Member

After looking into this I think it's ok to merge for the next major release.

@jkowens Thank you!

Member

zzak commented May 9, 2016

After looking into this I think it's ok to merge for the next major release.

@jkowens Thank you!

@zzak zzak merged commit 939ce04 into sinatra:master May 9, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jkowens jkowens deleted the jkowens:fix-1058 branch May 9, 2016

@@ -73,6 +73,12 @@ def unlink?
request_method == "UNLINK"
end
+ def params
+ super
+ rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e

This comment has been minimized.

@jkowens

jkowens May 9, 2016

Member

@zzak If there is any interest in backporting this fix to stable this would have to be changed to rescuing TypeError and ArgumentError.

@jkowens

jkowens May 9, 2016

Member

@zzak If there is any interest in backporting this fix to stable this would have to be changed to rescuing TypeError and ArgumentError.

This comment has been minimized.

@zzak

zzak May 9, 2016

Member

No, since I consider this a behavior change and added functionality

@zzak

zzak May 9, 2016

Member

No, since I consider this a behavior change and added functionality

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