Exceptions with http status code 400 ignore exception settings #1204
Comments
|
@tamoyal Did you set |
|
@mwpastore I only used true/false values for the above |
|
There's your problem. 😄 From the README (emphasis mine):
|
|
@mwpastore Gotcha. Seems a little strange to me but hey, if I'm the only one who thinks that, the community has spoken. I would just think it's good default behavior to not eat exceptions in development. I was also looking specifically at the configuration settings docs which does not mention this. Do you think that's worth an update? |
|
Maybe I misunderstood your original issue. Exceptions should always show in the logs. If they're not showing, something else is wrong. I want to focus on this sentence you wrote here:
This is not accurate. Koala can raise exceptions, but it can't set HTTP status codes. You would have to catch the exception (either inline with the call to Koala, or in an Maybe you can share a sample application that demonstrates what you want to happen, and what's happening instead? |
|
Seems a bit strange the code would pier into any exception and decide how to behave based on it responding to |
|
You appear to have found some undocumented behavior, or what I suspect is a private API. Sinatra does indeed look to see if an exception responds to |
|
I certainly want to catch those exceptions but let's say Koala updates their codebase and introduce a new exception. If I don't know about that introduction, it could be eaten causing a hard to track down bug. Again, not knowing the Sinatra code very well, do you think the class of the exception should be checked so Sinatra doesn't eat exceptions from 3rd party libraries? I'm not sure the best thing to do here. |
|
Koala exceptions are organized into a hierarchy, and any new exceptions will be added within the existing hierarchy, so as long as you're catching near the top of the hierarchy it should be fine. If Koala releases a new major version you'll probably want to scan the changelog and rerun your test suite—but you should be doing that anyway! Something like this: def events
graph = Koala::Facebook::API.new(FB_TOKEN, FB_APP_SECRET)
graph.get_connections("me", "events")
rescue Koala::Facebook::APIError
halt 403, "Please try again later" # or whatever
rescue Koala::KoalaError
halt 503, "Unknown error" # or whatever
end |
|
I'm cool with the resolution being that I should stay up to date on that level of detail about a library I'm using. I initially thought you'd consider this to be a flaw in the Sinatra design. Close? |
|
I'm just a Sinatra user so it might be worth leaving this issue open to see what the actual Sinatra maintainers have to say. There might be a way to refactor this behavior such that it doesn't interfere with Koala, but there's no telling what other libraries out there (such as Sinatra extensions) might rely on this behavior. I don't consider it a flaw in Sinatra as much as a weird coincidence that is going to be tricky to untangle! |
|
Gotcha. I agree it's best to at least let them see this and determine if it's a flaw. Thanks! |
|
I have no idea about Koala, but this seems to be you're running into surprising, yet intended, behavior. Could you confirm that enabling this setting fixes your issue? Thanks @tamoyal |
|
@zzak Yo! Amnesia has already set in on this issue but I remember it being more of an aggressive duck typing issue that a setting doesn't resolve but catching the right exceptions from the library did. You may want to take a look at the sample project I posted |
|
Thanks @tamoyal. Please close the issue if there's nothing further. Have a great day! |
|
@mwpastore I still think it's best for a maintainer to look at this and close the issue as you suggested. I mean...we can always close it and wait for someone else to complain. It certainly does not seem to be critical |
|
Same exact issue with this library - https://github.com/tbalthazar/onesignal-ruby I had to look at the source code and specifically catch their errors (luckily I knew to do this) because Sinatra was eating them. |
|
@tamoyal Thanks Tony. If nothing else it's good to have this thread here being indexed by Google for future seekers of wisdom. @zzak How would you feel about adding a configurable e.g. |
|
@mwpastore For sure - I think this will cause head banging the first time you experience this. Why are any exceptions not in the Sinatra namespace caught? I'm confused about that use case. |
|
@tamoyal I don't know, but I can guess: duck typing is the preferred pattern, so that's what was used; and it probably simply didn't occur to whoever implemented this functionality originally that libraries making outbound HTTP calls would want/need to raise exceptions that respond to |
|
Couple things, first what is the use case for this? I'm not sure I understand the behavior you're suggesting, perhaps it would be easier with code. Also, any behavior change we introduce at this point should be set behind a flag which defaults to the original behavior. Especially the area around exception handling in Sinatra is rather sticky, and could use an overhaul anyways so I'm less motivated to add more to it. |
|
The use case is some library throws an exception that happens to respond to http_status and sinatra eats it so you have no idea what happened. Code to show this is here - https://github.com/tamoyal/wtfrank |
|
@zzak That's what I'm proposing. I'll put a PR together.
|
|
Thanks guys! Let me know if I can help |
|
And maybe we should reopen this issue until we decide to close it again with no fix or merge in this patch or another |
By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues sinatra#1204 and sinatra#1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues sinatra#1204 and sinatra#1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues sinatra#1204 and sinatra#1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues sinatra#1204 and sinatra#1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues sinatra#1204 and sinatra#1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues #1204 and #1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
|
Closed by #1519. Will be released in v3.0. |
By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues #1204 and #1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
Regardless of my exception settings in Sinatra (the flags
raise_errors,dump_errors,show_exceptions), exceptions get eaten and not logged if there is a non 500-599 http status error code associated with them. For example, I am using the koala library to communicate with Facebook and the library can raise exceptions with an http status code of 400 if an OAuth token is invalid. I feel like if this happens and my Sinatra application is not rescuing that error, it should appear in the logs in development mode. It doesn't because of the following line in thehandle_exception!method:return res if res or not server_error?This seems very intentional given Sinatra is relatively mature. Am I off here or does this seem bad to anyone else?
The text was updated successfully, but these errors were encountered: