Proper servant-server exception handling for warp/wai

[symbiont-joseph-kachmar]

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]

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]

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]

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

[ch1bo]

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]

✨ 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]

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

[fisx]

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

[fisx]

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"?