Embedio: Add custom global exception handler

[Genteure]

Is your feature request related to a problem? Please describe.
Currently when a module throw a exception, both 2.x and 3.x return html containing call stack of the exception with no method to disable it. While it is useful when developing, it's not a good practice when deploying.

https://security.stackexchange.com/q/84465

https://github.com/unosquare/embedio/blob/ce1eb941740292c46d51269218889066357386a1/src/Unosquare.Labs.EmbedIO/HttpHandler.cs#L113-L128
https://github.com/unosquare/embedio/blob/5d2270385c3b94445f1036ec1d050a3034e4fe53/src/EmbedIO/WebServerBase.cs#L122-L130

Describe the solution you'd like

• Unhandled exceptions should always log to console (or whatever target SWAN is set to output to).
• Return 500 error page without exception information by default when debugger is not attached. Prevent leaking call path or SQL error message or other sensitive information.
• Add a handler to WebServerBase to allow custom exception processing. For example to send a more beautiful and user-friendly error page, or a encrypted version of exception message like YouTube (idea taken from here).

I propose add the following:

// Oops.
// public Func<bool, IHttpContext, Exception> UnhandledException { get; set; }

public Func<IHttpContext, Exception, Task<bool>> UnhandledException { get; set; }

If UnhandledException is null, or returns false, or throw exception itself, then return default error page.

Side question
https://github.com/unosquare/embedio/blob/5d2270385c3b94445f1036ec1d050a3034e4fe53/src/EmbedIO/WebServerBase.cs#L90-L154

Currently 3.0 would only process one request at a time right? Every Task is being awaited. Next request would not be accepted until current one is finished.

Edit: I thought v3.0 branch is almost done, turns out it's still work-in-progress now. 😅 I was going to do a pull request to branch v3.0.

[rdeago]

Hello @Genteure, thanks a lot for your input!

Exception handling is a topic that was bound to come up shortly in version 3.0. Latest commits brought some news:

• A OnExceptionAsync callback in modules to adjust behavior for the different purposes of different modules (for example, a REST client is hardly doing anything with a HTML description of an exception).
• No more JSON-encoded exceptions in WebApiModule, not because I forgot 😁 but because I think exception-to-JSON mapping should be well-defined (and overridable) in EmbedIO itself, and I was rushing to make the module work. ~JSON~ Formatted exceptions are certainly coming back, but in a more generalized and documented fashion.

I like your idea of having a callback property in WebServerBase to handle exceptions. Only I'd make it async, with the same signature as WebModuleBase.OnExceptionAsync (which of course you hardly could know, having been written last night). Apart from mere API consistency, an async callback would let one load a page template from disk, save exception data to a database _while_ sending the response, etc.

---

As for your side question: it's a bug, and it's my fault. Thanks a lot for the heads-up!

---

Finally, I'm glad to let you know that the v3.0 branch now compiles! It's still work-in-progress, but at least you may run the sample program.

You're very welcome to join us on EmbedIO's Slack workspace, by the way!

[rdeago]

Regarding encrypted exceptions: they'd be a cool feature indeed, but making them part of EmbedIO's core may be overkill.

There would be two ways of sending out encrypted exception information in either a HTML page or a text response:

1. Exception information > JSON > GZip > TripleDES > Base64 (feel free to substitute any of these with your preferred serialization / compression / encryption / encoding);

2. Store exception information on the server, together with a generated unique ID (again, feel free to choose between plain text files, SQLite, SQL Server, MongoDB, etc. etc.) and send out just the ID.

As you can see, both methods have their pros and cons, and both have extremely subjective implementation details. A decent implementation would need to be abstract, have sensible defaults, be easily pluggable. A library of its own merit, in other words, that you or anyone is more than welcome to write, while keeping EmbedIO as minimal as possible.

[geoperez]

@rdeago What do you suggest to add to v2.0 for better error handler? Obviously OnExceptionAsync is out of the discussion but the proposed callback to WebServerBase can add it without breaking the current behavior.

[rdeago]

I concur. I still think it'd better be asynchronous, but it's not extremely important.

By the way @Genteure, what's the bool parameter for in your callback signature? Verbosity, maybe?

[geoperez]

The boolean is the response status. The proposal is null handler or return
false then use default handler.

On Mon, May 27, 2019, 8:55 AM Riccardo De Agostini [e-mail]
wrote:

I concur. I still think it'd better be asynchronous, but it's not
extremely important.

By the way @Genteure https://github.com/Genteure, what's the bool
parameter for in your callback signature? Verbosity, maybe?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/unosquare/embedio/issues/300?email_source=notifications&email_token=AANRRMB2NPUZPFPAFHNKD6TPXPR43A5CNFSM4HPYSOIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWJ3XSI#issuecomment-496221129,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANRRMHPW5NKKRDQ2VBH5WDPXPR43ANCNFSM4HPYSOIA
.

[rdeago]

The boolean is the response status. The proposal is null handler or return false then use default handler.

Which response status? Do you mean whether the request has been handled or not?

(I'm sorry, today is more... Monday than a usual Monday)

[geoperez]

Yeah, in fact, the property should look like:

Func<IHttpContext, Exception, Task<bool>> UnhandledException { get; set; }

[geoperez]

I created a POC of what could be easily added to the current v2.0

[rdeago]

Now that makes perfect sense! Thanks for clarifying.

[Genteure]

Yeah, in fact, the property should look like:

Func<IHttpContext, Exception, Task<bool>> UnhandledException { get; set; }

Oops, you're right. I don't write Func<> very often, I thought it was Func<TResult, T1, T2>.

[geoperez]

This feature is already published for v2.0 in the nuget version 2.8.0.

[geoperez]

Can I close this issue?

[rdeago]

Since the requested feature has been implemented in both version 2 and 3, I'm closing this.

@Genteure feel free to reopen if you think there's something missing.