Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper servant-server exception handling for warp/wai #1192

Open
symbiont-joseph-kachmar opened this issue Jul 3, 2019 · 8 comments
Open

Comments

@symbiont-joseph-kachmar
Copy link

I spent some time reading through issue #309 and the associated discussions in yesodweb/wai#496 and PR #954 about how servant-server appears to incorrectly handle exceptions with respect to how wai/warp expects to operate.

The unfortunate side effect of this behavior is that any uncaught exceptions within a Servant application will not be registered by any wai middleware as a 500 exception, which can seriously negatively impact user experience around automated logging, metrics tracking, etc.

As far as I can tell, the solution proposed in #309 around deeply evaluating Responses in toApplication before creating a ResponseReceived (to provide to wai) won't work for ensuring that any impure exceptions in thunks are caught (Response cannot have a valid NFData instance to the best of my knowledg).

The machinery for forcing evaluation, catching exceptions, and generating correct wai Responses would have to live somewhere else in Servant, probably using a similar combination of techniques as to what Yesod uses:

https://github.com/yesodweb/yesod/blob/6a7370a9e61ca48787c539842602b161c219e6ba/yesod-core/src/Yesod/Core/Internal/Run.hs#L43-L48

https://github.com/yesodweb/yesod/blob/6a7370a9e61ca48787c539842602b161c219e6ba/yesod-core/src/Yesod/Core/Internal/Run.hs#L50-L55

https://github.com/yesodweb/yesod/blob/6a7370a9e61ca48787c539842602b161c219e6ba/yesod-core/src/Yesod/Core/Internal/Run.hs#L104-L130


I'm opening a new issue since it seems as if the old ones have languished a bit, and the strategies for handling this problem outlined in them don't appear to actually address the core issue here.

Does anyone have thoughts on what a good architecture for this might look like?

Perhaps some pointers towards code paths within Servant that would allow us to implement the correct behavior, even just as an opt-in for users who want to ensure that middleware operates as-expected?

@fisx
Copy link
Member

fisx commented Jun 13, 2020

In my mind, catching impure exceptions cannot happen in servant without potentially serious performance degradation. Also, it is against the idea that the types represent the behavior of the rest API as completely as possible.

I can see two things working:

  1. Don't allow partial handlers and impure exceptions.
  2. setOnExceptionResponse, setOnException for making a better error and logging it, resp.

1 is not always easy, especially if you're using libraries that are sometimes subtle about being patrial; but together with 2., I think it is the way to go. Impure exceptions should be 5xx errors and treated as software bugs.

I've now read through #309 and #954 a little, and changed my mind to "it's conceivable that we find a consensus for toApplication to force the Response under certain conditions, but I don't like it:: it complicates the library and encourages the use of impure exceptions.

This is exclusively about impure exceptions, or is that where I'm wrong?

@fisx
Copy link
Member

fisx commented Jun 14, 2020

to re-iterate: we're not talking about having the servant types represent exceptions, but about impure exceptions (ie. undefined or error "bla", ie. errors that are outside the control of the types by necessity). the issue is that due to lazyness, these impure exceptions can not only make past servant, but also past the middleware (where they are already outside the control of the types), and thus choke warp in a way that does not get logged or rendered into a properly shaped 5xx (with proper body content type and all).

the more i think about this, the more i'm certain the solution should be the warp hooks.

@fisx
Copy link
Member

fisx commented Jun 14, 2020

related: #1022, #309, #954, maybe others?

@fisx fisx mentioned this issue Jun 15, 2020
@ch1bo
Copy link

ch1bo commented Nov 25, 2020

As the author of servant-exceptions, here are my 2cents:

  • Do not use partial functions / impure exceptions for the exact problem of not knowing when they are raised (forced)
  • Handling exceptions is quite convenient with servant-exceptions and could provide an alternative to the warp hooks.

FWIW I drafted a quick example here: https://gist.github.com/ch1bo/cbfff47cd8a8630974c974c46632773c

@brandon-leapyear
Copy link
Contributor

brandon-leapyear commented Dec 7, 2021

This is an old work account. Please reference @brandonchinn178 for all future communication


FWIW I drafted a quick example here: https://gist.github.com/ch1bo/cbfff47cd8a8630974c974c46632773c

This example doesn't use impure exceptions at all. The error here

getUser :: MonadThrow m => Text -> m User
getUser n
  | n == "foo" = return $ User "foo"
  | n == "bar" = error "oops"
  | otherwise = throwM UserNotFound

has the type error "oops" :: MonadThrow m => m a. An example of an impure exception would be

  | n == "bar" = return $ User $ error "unknown name"

which wouldn't be handled by the mapException below.

@yellowbean
Copy link

Hi @fisx , thanks everyone. But may I know whether this issue has been fixed yet ?

@fisx
Copy link
Member

fisx commented Jun 18, 2024

not to my knowledge. sorry, i haven't been paying attention to this since i last commented.

@fisx
Copy link
Member

fisx commented Jun 19, 2024

i gave this a little more thought.

as i understand, the problem is that data is lazily streamed through the application logic, servant, wai, warp, and there may be an impure exception (like error or undefined) in the application logic. since warp may already have sent the http header with the 2xx status, it is impossible to change it to 5xx.

this is certainly not nice, especially since you can't rule out impure exceptions like lightning strike or vulcanoes. but i also don't see a solution that doesn't involve abandoning the laziness and the performance gain it gives you.

i suppose it would help prioritize this issue if you could come up with a hypothetical scenario where this is exploited by an attacker? something along the lines of "client gets a 200 on request X, doesn't realize the body is incomplete, and goes ahead and does something to the backend that constitutes a breach because the backend is not in the state the client assumes"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants