DeveloperExceptionPageMiddleware always returns HTML, sometimes JSON would be better

[aspnet-hello]

From @tjrobinson on Friday, February 3, 2017 3:54:20 AM

We're using the DeveloperExceptionPageMiddleware and it would be really useful if it could be changed/configured so that AJAX requests to APIs, e.g. from an Angular application were given a JSON response back instead of HTML. This would make the response much more readable when using F12 tools or testing the API through other non-browser tools like Postman.

I hope that's enough information to make sense, let me know if not.

There's a similar problem discussed here: http://stackoverflow.com/questions/35245893/mvc-6-webapi-returning-html-error-page-instead-of-json-version-of-exception-obje

Thanks!

Copied from original issue: aspnet/Diagnostics#346

[aspnet-hello]

From @davidfowl on Sunday, August 6, 2017 12:15:32 PM

Feels like we could do this as part of the new API work @rynowak.

/cc @Eilon

[aspnet-hello]

From @rynowak on Sunday, August 6, 2017 9:18:48 PM

https://tools.ietf.org/html/rfc7807

And then you want formatters and get bit by the layering bug 😆

[mkArtakMSFT]

Thank you for your feature request. We'll consider this feature during the next release planning period and update the status of this issue accordingly.

[Eilon]

I've been thinking about this space and started some prototypes, so self-assigning for now.

[PureKrome]

Thanks heaps @Eilon !

[khellang]

See this Twitter thread and this blog post if you want something for APIs 😄

[dotnetchris]

This will be a big deal.

Also it would be super helpful if this is exposed a way that easily integrates with existing custom exception handlers, maybe something like

public void OnException(ExceptionContext context)
{
    if(Debugger.IsAttached) 
    {
        context.Result = new DeveloperExceptionObjectResult(context/context.Exception);
        return;

...

That it's as simple as returning a specific result object type. [should also be able to work straight from a controller action method too]

[PureKrome]

Thanks for listening @rynowak .

My thoughts in this are pretty weak - plain text solution basically solves it all :) But I'll just jot down my thoughts cause this is the opportunity. Maybe someone else might get an 'a-ha' moment, based of this.

But having plain text is soo awesome! It will be usable :)

---

So this is all about web api's -> which usually are responses sent back as JSON-or-XML (either hardcoded by the developer(s) or dynamic via Connection Negotiation (which again is usually JSON/XML again .. because this is an API.

So with this in mind, I thought it might be nice to make it easier to parse the error message(s) on the client side. Plain text is 💯 waaaay better than before (cheers! 🍻 ) but I sometimes like to display various parts of the error message to the browser client.

So the scenario is this:

• Test with Postman : errors can be nice text (as is now the option) or json/xml (more verbose, doesn't gain anything really) (rinse/repeat a bazillion times....)

then finally

• Start testing my web app (which does Ajax/API calls to my backend api) and some errors occur so we could display the errors a bit nicer if we could split out the key/values. But having plain text is sooo much better and usable than before.

I also understand that adding the extra complexity of JSON or XML output might pull in other dependencies, etc... so this could be a crap thing. Maybe have an option for people to extend this themselves, easily, if they really want .. similar to how logging only has a few targets/sinks but can be extended easily for others. Prolly not worth the effort?

Anways, that's my 2cents. Cheers for the plain text change! appreciated!!!!

[ggirard07]

Not sure this is a real good case either but simply because our clients always parse response from the API as JSON as this is the only content type we use/support. Having a plain text response is going to throw exceptions on the client side that will only add noise the the debug experience...

[khellang]

You know... There are alternatives to this linked further up in this issue 😂😜

[rynowak]

Thanks for the feedback - some high level thoughts on this.

We divide error handling into Developer error handing and Production error handling. That's why you use see:

if (env.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
}
else
{
    app.UseErrorHandler("/some/path"); 
}

The developer exception page pretty prints a bunch of stuff about the request, but the main event is giving you the exception details. This middleware is needed because we don't want to anything built into the stack by default that does this. If it's not built in, you can remove it or replace it because it's just a line of code in your startup. The emphasis here is on being informative by default since it's for a development scenario.

The error handler middleware runs your code to do what you want for error handling. We have another work item planned for 3.0 to make this better by default for APIs #8539 - but the emphasis here is that you as the application owner need to make a decision about what it means to report error an error.

In the past we had some pretty complicated logic that tried to figure out if we would show you the exception details or not. We made a conscious decision to separate error handling for development and error handling for production scenarios using code to make this clear and put the application developer in control.

---

Responding to your comments:

Test with Postman : errors can be nice text (as is now the option) or json/xml (more verbose, doesn't gain anything really) (rinse/repeat a bazillion times....)

This is what expected to get better. I would argue that making this experience based on JSON would be worse than plain text. You're going to get escaped newlines in stack trace etc.

Start testing my web app (which does Ajax/API calls to my backend api) and some errors occur so we could display the errors a bit nicer if we could split out the key/values. But having plain text is sooo much better and usable than before.

My question here is do you actually want to see exception details from the server in your browser console (development error handling), or do you want to see what's going to come from your production error handling path?

The idea that someone is going to write code to parse data that we return from the DEVELOPMENT error page makes me a little nervous. This inevitably leads to people putting the DEVELOPMENT error page in production so that they can keep using this, which we'd strongly recommend against.

These factors lead me to thinking that returning JSON with the exception details is a bad default. I think it's totally fine if you want to use extensibility (or the example that @khellang ) linked to do this for your apps. You chose that after all. I haven't seen anything yet that makes me to enable this as a default.

[khellang]

My middleware (that always returns HTTP problem details as JSON) will include exception details when IsDevelopment is true by default. But it will still be there in production.

My client-side apps that consume these APIs are then guaranteed a particular error response format and use a React component (kinda like https://github.com/smooth-code/error-overlay-webpack-plugin) that will format these errors nicely for a really smooth dev experience.

So yeah, I definitely think this is a valid story, but don't really need it built-in or on-by-default as I've already scratched my own itch 😉

[PureKrome]

@khellang when I was writing those few paragraphs from a comment (a few slots above this comment) I was thinking of your lovely ProblemDetails library and nearly deleted my entire reply because I think that's the best solution IMO. I also use your library extensively as the defacto error payload for all my wenapi's.

I've also mentioned in another GH issue somewhere about how, IMO, all-and-any errors in an ASP.NET Core webapp should return a PD for all responses .. and this functionality should be baked into the framework.

But these days, you're library is a god send and, IMO, should be the default error handling.

[syndicatedshannon]

TLDR: Isn't this actually better solved through enhancing UseExceptionHandler() and UseStatusCodePagesWithReExecute()?

First, sorry to weigh in, in a rather pattern-opinionated manner, after some work is already underway or complete. In my team's defence, exception handling patterns have changed so frequently in ASP that we couldn't dig into this until now.

The primary issue UseDeveloperPages addresses is balancing development ease with security of sensitive data. I agree with the premise of this issue that this is relevant to both WebAPI/JSON and MVC/HTML.

I disagree with the premise here that the format should be differentiated by route. For appropriate SoC it is typically a question of the requested format (i.e. the HTTP Accept header), not the route. We could still evaluate the Accept header in MapWhen(), but... shudders at the exponential code explosion

This issue has been around for some time in different forms, seen 3 years ago in aspnet/Mvc#5130, aspnet/Diagnostics#346, and dotnet/templating#949. Other occurrences surely exist.

I agree with this issue's overall premise that we generally want the same sensitive information hidden or exposed, regardless of whether we are returning JSON or HTML. I also agree with @dotnetchris 's premise that we typically transform Exceptions similarly and map our custom Exception hierarchy into status codes that apply regardless of response format.

As mentioned frequently in these related threads, for a JSON error result to be useful, we require a fixed data structure. Without ASP.NET forcing a JSON format (while acknowledging RFC7807 exists), don't we have to assume the developer is creating a custom response?

I have another concern with UseDeveloperExceptionPages(), in that it encourages code to go to production without testing. Instead of switching code blocks between development and production, we should encourage minimal augmentation in development.

Lastly, I personally wish all custom error handling in ASP.NET would make it much easier to hand off more information; at a minimum a unique problem ID (e.g. GUID).

[syndicatedshannon]

Ultimately, what I personally would prefer to be able to accomplish effortlessly is:

1. Transform the exception (either by re-throwing or simply wrapping). Do this the same way regardless of the environment or requested format.
2. Add a unique problem ID. This could potentially be included in the transformed exception.
3. Add other context, especially Identity key. This could potentially be included in the transformed exception.
4. Log the Exception and the augmented information.
5. Apply a StatusCode conversion. This could potentially be included in the transformed exception.
6. Capture/forward the IHostingEnvironment (through a either default parameter or lambda) status as a flag to display sensitive debugging information.
7. Diverge response format at the last possible moment to reduce code duplication and improve development code coverage, as is the practice in most ASP.NET implementations.
8. Include the above-identified information to the appropriate HTML or JSON renderer.
9. Continue to offer default implementation of HTML exception renderers, but also offer a default RFC7807 rendered for JSON.
10. Include the augmented information in the rendered template
11. Allow override of both JSON and HTML renderers (similar to UseExceptionHandler() today).

[davidfowl]

I don’t think we’d build something that supports all of those features. It does sound like something that could be written in a custom middleware though. I’d suggest doing so and we’ll add any supporting platform features you’d need to achieve that.

[syndicatedshannon]

@davidfowl I agree. I wouldn't expect the platform to completely implement my specific requirements.

I should note that we already do all of the above in middleware. We did have to re-implement blocks which I would not expect. So when I think about platform features I expect, I mainly look to what is already in the ASP.NET Core library; if I have to copy multiple significant blocks mostly unedited from the repo (e.g. StatusCodePages) to make use of it, my instinct is to ask for it to be exposed in a new way.

I'll give it some thought and follow up within a day.

[khellang]

@syndicatedshannon See #2590 (comment). It does pretty much all you mentioned 😊

[syndicatedshannon]

@khellang Thank you. I briefly followed the twitter link before I commented, but I didn't see the relevant code immediately. Now that I look more closely at the blog post I see the ProblemDetails implementation portion. I'll look further for the other bits.

[syndicatedshannon]

@khellang Very nice code, thank you. Unfortunately it doesn't comply well with our pre-existing opinions expressed in our code. I'm going to think more about this, perhaps my own opinions will be more flexible given a few days. In the meantime, are you available for a chat?

[syndicatedshannon]

@davidfowl So, did more digging.

Much of the core library code to accomplish the workflow I mentioned is not reusable:

• UseErrorHandler, including the following knowledge:
  • application of ExceptionDispatchInfo
  • code to re-execute pipeline
  • pass Exception on DI stack as IExceptionHandlerPathFeature
  • something I didn't follow about the choice to return Task from synchronous Invoke
• UseDeveloperExceptionPage including this work:
  • capture/display headers and other HTTP context
  • parse/format both compile and runtime exceptions
  • another process to re-execute request
• UseStatusCodePagesWithReexecute includes knowledge such as:
  • something apparently about an IE document size defect
  • new handling for endpoints and clearing cache
  • rendering reason phrases
  • another process to re-execute request

None of that is accessible. I also see work on various threads going towards rendering JSON problem details, and if it follows the same patterns may also be inaccessible.

Regarding new features, the main new functionality I'm seeking is something we see accomplished in a different way in each of the above: the ability to re-execute a request to another endpoint, passing a model (in this case, the ExceptionDispatchInfo, the original HTTP contextual information such as the caller's Identity, the trace ID, and the unique exception identity assigned at logging time).

[syndicatedshannon]

Many of the things that happen in UseErrorHandler, and other ASP.NET Core handlers, we want to happen regardless of formatter.

• Performant/robust catch (knowledge of ExceptionDispatch / stack performance)
• Performant/robust async pattern (knowledge of Task.IsCompletedSuccessfully)
• Sensible default re-throw operation (existing contributor comments demonstrate complexity).
• TraceID association (for logging correlation).
• Optional error ID assignment (ideally something that can be provided to end-user).
• Capture/deconstruction of HttpContext, such as HTTP headers (for logging and sensitive display).
• Capture/deconstruction of exception information (for logging and sensitive display).
• Assignment of default human-readable messages.
• Delegated/configured mapping of proprietary non-sensitive human-readable messages (as an optional aspect of exception handling to reduce middleware layers).
• Delegated/configured status code mapping.
• Capture/aggregation of Exceptions thrown while handling, especially from optional attached delegates.
• Call DiagnosticListener.
• Call ILogger.
• Handle response already started.
• Discriminated capture of both sensitive and non-sensitive information (opinion. see below).

In my opinion, a thoughtfully-proscribed ordering of the above actions should be a core reusable component of ASP.NET, as demonstrated by existing code and the expert ASP.NET considerations behind it. Then, regardless of development/production modes or sensitive information, we want to be able conditionally (e.g. Accept: application/json, Accept: text/html, etc.) apply default or overridden formatters to the above collected information:

• HTML w/ reexecute
• HTML w/ redirect (selected information in parameters)
• RFC JSON ProblemDetails
• lambda or similar

Lastly, (and this is also rather opinionated), we want to decide whether to include the sensitive information in our formatter output, as a strict superset of the standard response. I would prefer to take the above prepared error model, including information clearly discriminated as sensitive and non-sensitive, and provide it to the renderer as the last step. My reason for this opinion is that otherwise we are implementing user-facing code but leaving it out of development cycles, and the documented/pre-instrumented/best-practice patterns typically shouldn't encourage this. I want to avoid branching middleware to a separate pipeline in development, because it can cause non-integration defect discovery to be deferred to integration. In the case of JSON responses, we definitely want our SPA implementation to receive a standard error base object and exhibit the designed error response to errors, even in development mode.

The great majority of the above capabilities are already implemented in various elements. However, they aren't consistently applied across the various features. They aren't composable, and in some cases we can't achieve at all for certain formats, because certain implementations don't exist in all branches. To resolve, we have to re-implement because they are in non-reusable blocks. Even in the core code we see repeat, but inconsistent implementations.

This is a problem because there is a lot of expertise behind the feature implementations I listed above. To use that expertise we need to apply multiple layers of middleware, inspect if the layer should be active, and rethrow if it should not. This adds multiple layers to our call stack. It makes diagnosing our diagnostic capabilities challenging. And it's an important, core feature of a well-behaved application.

I know this is an ugly/incomplete location for this conversation; I'm still thinking it through and promised I'd get back to you.

[syndicatedshannon]

@khellang Getting back to you on the library you recommended; it's very nice, but AFAICT it only handles the last step of what I just described above, requires early branching, and doesn't provide an easy means to receive the common/pre-determined knowledge. For example, it requires re-statement of StatusCode maps via options.Map<NotImplementedException>(ex => new ExceptionProblemDetails(ex, StatusCodes.Status501NotImplemented));

I agree it does integrate similarly as an additional middleware layer with the existing error handling, using similar concepts.

[khellang]

AFAICT it only handles the last step of what I just described above

What is that step exactly? It's hard to tell which part of the above text is "steps" 😄

requires early branching

What do you mean by this? There's no branching involved in the middleware.

doesn't provide an easy means to receive the pre-determined information

What do you mean by "pre-determined information"?

[syndicatedshannon]

What is that step exactly? It's hard to tell which

Agreed :) I was editing the referenced comment quite a bit. I think I was referring to RFC ProblemDetails at the time.

There's no branching involved in the middleware.

Agreed. The branching has to occur earlier, prior to your middleware, cutting various steps entirely, to handle development scenarios, etc. Some code somewhere has to branch, to respond to format (HTML/JSON) and sensitive display (IsDevelopment). IMO, ideally the branching, especially when it will never see the light of day until integration-time, occurs as late as possible and changes as little as possible. My strong preference is only to add functionality at dev-time, not replace.

means to receive the common/pre-determined knowledge.

See the example provided. I may have been adding that example as an edit while you were responding. The map between exceptions and status codes is one example of common knowledge that must be shared between JSON and HTML message formats. Another is human-readable text representation of the exception. Another is differentiation and representation of sensitive information in a development mode.

[syndicatedshannon]

I'm still trying to sort this out, so I can perhaps validate these thoughts and put them in appropriate distinct issues, but it's rather time consuming to sort through disparate implementations in the existing code. I'm finding capabilities that I've not seen documented, so it also is taking some time to verify some of these capabilities I believe are not accessible, are actually not.

Discovering the ability to set IStatusCodePagesFeature.Enabled = false was a surprise to me. That smells strange to me, that another implementation might be expected to disable a specific feature to keep from breaking. This further complicates understanding possible interactions between middlewares that inspect and alter StatusCode and ResponseStarted, handle and rethrow exceptions, etc. The truth is hard (at least for me) to identify here.

Understanding the reason for difference between the implementation of page re-execute in the following locations: https://github.com/aspnet/AspNetCore/blob/c0161c7e777fffde7d914abbe0cd2716b6304dd7/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs#L117
https://github.com/aspnet/AspNetCore/blob/10863cfaead78e02eb788611fc1f64a889db6941/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesExtensions.cs#L202
https://github.com/aspnet/AspNetCore/blob/10863cfaead78e02eb788611fc1f64a889db6941/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddleware.cs#L178

The use of "Features" service locators to pass various values around the diagnostic middleware is taking me a while to sort out.

I appreciate that these components have grown organically. They generally work independently. But if I can't predict how they interact with other middleware, then IMO they aren't complete. I think their presence implies they are valuable in the library. I certainly want them there. I just can't currently use them in part or whole.

[IanKemp]

To paraphrase what I believe the gist of @syndicatedshannon's comments to be: while the work done for 3.0 (return plaintext exception responses where appropriate) solves 99% of developer gripes with DeveloperExceptionPageMiddleware, it's still not good enough because there's a lack of actual extensibility there. This was originally surfaced in #8536 which was closed as fixed - incorrectly, I believe.

@khellang's ProblemDetails package isn't an acceptable solution for me because it requires you to write your code in a way that's dependent on that library. I just want to be able to control the way that DeveloperExceptionPageMiddleware behaves when an exception occurs:

• get hold of the exception context (including stack trace information)
• decide in what format to respond (based on e.g. request headers)
• use a standard MVC view to return the relevant data

An example desired API in Configure in Startup.cs:

app.UseDeveloperExceptionPage((request, exceptionContext) =>
{
    if (request.ContentType.IsText())
    {
        return View("MyPlainTextView", exceptionContext);
    }

    return View("MyHtmlView", exceptionContext);
});

[khellang]

@IanKemp That's exactly what the error handling middleware does. It let's you re-execute a route (i.e. an MVC action) and return whatever you want. That includes conneg.

[syndicatedshannon]

Thanks Ian. I'm still investigating. I know it's hard to pin down what my issue is, other than that I dislike the current implementation.

I'm working on an implementation of my request, primarily to better understand the problem, but possibly also to contribute. I've put more time and focus into this than I can afford.

If I were to bail on this investigation now, here are some of the things I would probably ask for:

• A clarified pattern or feature to divert/re-execute a response to a strongly-typed view. This is essential to error handling on "Accept: text/html" requests, and thus is implemented multiple times in existing ASP.NET Core error-handling code... none of which is publicly accessible.
• Reusable implementation of the existing developer Exception/StackTrace HTML visualizer, e.g. as an HTML partial with public supporting models. The current ASP.NET Core implementation is sweet, too bad I have to engage in severe CopyPasta.
• Single hub middleware that can be configured (e.g. via delegate injection) to perform most error handling steps (exception capture, error sanitization, status code assignment, response formatting) in a conventional manner. Currently I have to use 4 ASP.NET Core middlewares to get all of this in multiple response formats. Ignoring any performance/debugging issues, it's really confusing when some middlewares "bow out" because hidden flags have been set.
• Support for reusable error responses provided a strongly-typed model between Exceptional and non-Exceptional errors. Exception handling is important, but ideally we handle predictable problems via a non-exceptional workflow; chances are we'll want to use some of the same code either way. We can't do that easily if we have to carve up our handlers along the current inconsistent axes of sometimes Development/Production, sometimes Status Code, sometimes JSON, etc.

[syndicatedshannon]

@IanKemp That's exactly what the error handling middleware does. It let's you re-execute a route (i.e. an MVC action) and return whatever you want. That includes conneg.

@khellang How would you call this from your own middleware? Keep in mind part of the issue is the assumption we'll engage custom middleware per comment by @davidfowl:

I don’t think we’d build something that supports all of those features. It does sound like something that could be written in a custom middleware though. I’d suggest doing so and we’ll add any supporting platform features you’d need to achieve that.

[khellang]

How would you call this from your own middleware?

You don't. Why would you need your own middleware for that? The error handling middleware captures the exception and re-execute the pipeline. Whether you have a custom middleware or an MVC controller to handle that exception, that's up to you.

[syndicatedshannon]

You don't. Why would you need your own middleware for that?

I think we aren't talking the same language. Or maybe you are being intentionally adversarial, I'm actually not sure. But I'd rather not get in a chest-beating contest about the "right" implementation. That's why I referred to David's suggestion that we might need to build our own middleware. Additionally, I'd like to note you also built your own middleware for a related problem, and are suggesting others use it.

Again, I really don't want to get in an argument about it. I am often wrong, and may be here again, but, here are a few more specific examples of where I imagine such custom middleware may be necessary:

• any time we need to provide something to handler, e.g. via DI, but the existing ExceptionHandlerOptions of PathString and RequestDelegate are insufficient
• if we desire our view to receive a custom type
• if we desire the outcome to be variant

[khellang]

I'm just getting more and more confused. I've tried to go back and read your posts multiple times, but it's very unclear what you're asking for (or even just talking about).

Yes, I wrote my own middleware, it solves a bunch of people's issues. I thought it might help someone here as well. If you'd spent half the time you've spent researching and writing in here, you could've probably written your own as well by now 😏

I don't know what you think I'm trying to accomplish here, but I'm out, sorry 🤷🏻‍♂️

[syndicatedshannon]

@khellang No worries. Thank you for your contribution.