No CORS headers sent in case of error 500

[aspnet-hello]

From @iwbo on Wednesday, October 19, 2016 4:22:02 AM

There is a similar closed issue that I have commented on, but there hasn't been any response, so I created a new issue.
Closed issue: aspnet/CORS#46

Comment copy:

Has this issue been revisited?
I mean, is it really a CORS policies job to prevent sending out full exception details?

But the main problem for me is that if I allow some domain do make CORS requests, I expect them to get all the information.

If the request was made with "XMLHttpRequest" as fallows:

var xhttp = new XMLHttpRequest();
xhttp.onreadystatechange = function () {
console.log(this.status); // with no CORS headers allways 0
if(this.status === 500){
// make some noise
}
};
xhttp.open("GET", 'http://www.example.ee/api', true);
xhttp.send();

If the response is indeed 500 but without CORS headers, then the "XMLHttpRequest" status will be 0, not 500.

So what's your ideas on this? Or am I missing something?

Copied from original issue: aspnet/CORS#90

[aspnet-hello]

From @Eilon on Wednesday, November 23, 2016 2:40:30 PM

I think the issue here is that this is not likely to be safe to do in general in the CORS middleware, which is why we recommend customizing the behavior for your app. That is, it is not generally safe to send all error details to a remote caller. So, instead of letting the generic ASP.NET Core error handling mechanism propagate the error to the client, we recommend writing error handling code that effectively handles the error safely, and returns the appropriate message/info/data to the caller. That's essentially what is shown here: aspnet/CORS#46 (comment)

[aspnet-hello]

From @iwbo on Wednesday, November 23, 2016 3:20:48 PM

So if I need to let the caller know that an Internal Server Error occurred together with CORS middleware.
I would need to change the status code before CORS middleware and use some other status code or data structure to represent the Internal Server Error status ?

Also isn't detailed Error information hidden by default in asp.net core ?
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/error-handling

[aspnet-hello]

From @Eilon on Monday, December 19, 2016 9:15:57 AM

I'm not sure if this is getting philosophical, but why would a JavaScript app need to know that an internal server error occurred? CORS is just for browser clients, and presumably if the server had a server error, the client can't do much about that (that's what an HTTP 500 error is).

[aspnet-hello]

From @iwbo on Thursday, January 5, 2017 2:59:08 AM

Yes, I also think the question is a bit philosophical. And an alternative solutions has already been posted!

But here are some reasons:
The JavaScript app is built as a business application in browser by a third-party.
They should:

1. Let user know that an error occurred
2. Log from client side (not the error content, but that a 500 has happened)

[aspnet-hello]

From @hnrchrdl on Monday, May 8, 2017 1:54:32 AM

This issue should be addressed. CORS should be independent of status codes whatsoever.

[aspnet-hello]

From @Eilon on Tuesday, May 16, 2017 5:39:58 PM

Outside of the philosophical side of things, I think the thing being discussed here is that ASP.NET Core's exception-handling middleware (the Developer Exception Page and the Exception Handler) will clear out all the headers so that they can send their response, which includes clearing out CORS headers.

I don't think that the CORS middleware itself cares about status codes; it'll gladly add the CORS headers as necessary.

So, if your code explicitly returns some error status code (400, 404, 500, etc.), I'm guessing that CORS will work just fine. However, if ASP.NET Core is going to render a generic error page, it clears all the headers in order to ensure maximum security (data exposure) and maximum compatibility (header conflicts).

[aspnet-hello]

From @PureKrome on Thursday, November 16, 2017 3:03:21 PM

@Eilon I'd like to add some more input (hopefully valid) to this issue.
NOTE: I was originally going to post this in the aspnet/Diagnostics repo because this might be more relevant to the ExceptionHandlingFilter instead of the CORS repo .. but I might try here as the context is helpful.

---

I have a very standard ASPNet Core 2.0 .AddMvcCore() web app. I then have CORS enabled and all works 100% great. A typical SPA app with REST/API backend.

When a server error occurs I'm expecting the Client App to successfully receive a Response from the server ... even if it's an HTTP-500 response. Right now when I handle custom exceptions

i.e.:

app.UseExceptionHandler(options =>
    options.Run(async httpContext => 
        await CustomExceptionHandler.ExceptionResponseAsync(httpContext, true)));

the aspnet core code clears all headers so my client app cannot read the response because the browser rejects the non-cors-complient response. This has been justified as:

However, if ASP.NET Core is going to render a generic error page, it clears all the headers in order to ensure maximum security (data exposure) and maximum compatibility (header conflicts).

It took me ages to figure out this was happening.

There is no obvious documentation about this. Literally, I had to ask in StackOverflow and then by fluke I was given some help because another very kind person actually checked the source code (thank you for OSS, MS!!) and told me what to do :)

So - would you gals/guys consider maybe enhancing the api to include an option to keep the headers etc .. because we the developer can make that call based upon what we intend to do with our custom exception handling.

for example (for one of the methods in the ExceptionHandlerExtensions class) :

public static IApplicationBuilder UseExceptionHandler(this IApplicationBuilder app, Action<IApplicationBuilder> configure, bool areHeadersCleared = true);

I find something like this important because it's actually describing some 'black magic' that would normally occur "under the hood/inside the box" and allow us, the developer, to make a more accurate decision about what we are trying to do with exception handling now.

Now - this is a lot of chat for something that can be considered smallish, so I'll give you guys some context to what we are doing and why we find this important.

• We have services/BLL nuget library code that returns expected results (yay) or throws various errors, like a ValidationException (code failed to validate like name to short or age too old, etc)
• We have code that has bugs and just throws normal Exception's like a db timed out cause network dropped and we forgot to use Polly for retrying, etc or db deadlocks (urgh..) etc..
• Possible security creds failed, like .. they tried to update an item but they don't have the proper creds for it. (i.e. manually hacking querystring or POST form vals, etc)

So our libraries throw exceptions to handle non-happy-path scenarios and our custom exception handler (called by the exception handling middleware) checks the exception.GetType() (basically) to determine the final status code and json payload.

Finally, we return our error info in the response body as json so a consuming client can (if they wish) interrogate the json payload to determine what to show on the client. At the very least, check the status code and update the UI accordingly.

if error == 400, display validation errors (info is in the json payload).
if error == 500, then display the error message in the payload. If dev, then detailed, if production, then some vague apology etc.

(NOTE: this is getting into the "philosophical side of things" so I don't want to debate this, just give some context to what and why we are doing things as such.

---

Counter point:

The counter to this interesting too. Basically, have a try/catch inside each Action Method in the controller. This can be easily abstracted into a custom base controller where we could have a single protected method like this (in a base controller) ...

Note: browser pseduo code, etc...

protected async Task<dynamic> TryAsync(Func<Task<dynamic>> func)
{
    try
    {
        return await func();
    }
    catch (ServiceException serviceException)
    {
        return HandleExceptionWithMessage(HttpStatusCode.BadRequest, serviceException);
    }
    catch (ValidationException validationException)
    {
        return HandleValidationException(validationException);
    }
    ... snipper
}

and called like this..

[HttpGet("")]
public async Task<IActionResult> GetProductNotificationsAsync(ProductSearchQuery productSearchQuery,
                                                               CancellationToken cancellationToken)
{
     return await TryAsync(async () => 
        await _myService.GetProductsAsync(productSearchQuery, cancellationToken));
}

---

So yeah - some context, some thoughts, some extra discussion. Not sure if this should be asked in the aspnet/diagnostics repo.

thanks for reading this and keep on being awesome :)

[aspnet-hello]

From @Jungers42 on Thursday, November 30, 2017 1:15:53 PM

Ugh ... I just spent half a day wrestling with ways to solve this (to aid in our migration from a full framework ASP.NET WebApi project into a new .NET Core API project (.NET Standard 2). After playing lots of games, this was my only reasonable solution to still have the useful dev exception message from the comfort of our actual web app (Chrome dev tools or the like) which is using both OAuth and CORS.

The startup bits look like this:

        public void Configure(IApplicationBuilder app, IHostingEnvironment env) {
            app.UseCors(c => {
                            c.AllowAnyOrigin()
                             .AllowAnyMethod()
                             .AllowAnyHeader()
                             .WithExposedHeaders("Content-Disposition", "Content-Length");
                        });

            if (env.IsDevelopment()) {
                app.UseMaintainCorsHeaders();
                app.UseDeveloperExceptionPage(); // Beautiful exception pages!
            }

Notice that CORS is the first middleware in the pipeline (pretty wide open). Next, only for development, we add our own custom middleware MaintainCorsHeader followed by the standard DeveloperExceptionPage.

Here is that custom middleware which does the magic necessary to "bring back" the CORS headers. It could be more restrictive, but it's only holding onto headers we've chosen to add in anyhow so it should be safe.

        public async Task Invoke(HttpContext httpContext) {
            // Find and hold onto any CORS related headers ...
            var corsHeaders = new HeaderDictionary();
            foreach (var pair in httpContext.Response.Headers) {
                if (!pair.Key.ToLower().StartsWith("access-control-")) { continue; } // Not CORS related
                corsHeaders[pair.Key] = pair.Value;
            }

            // Bind to the OnStarting event so that we can make sure these CORS headers are still included going to the client
            httpContext.Response.OnStarting(o => {
                var ctx     = (HttpContext)o;
                var headers = ctx.Response.Headers;
                // Ensure all CORS headers remain or else add them back in ...
                foreach (var pair in corsHeaders) {
                    if (headers.ContainsKey(pair.Key)) { continue; } // Still there!
                    headers.Add(pair.Key, pair.Value);
                }
                return Task.CompletedTask;
            }, httpContext);

            // Call the pipeline ...
            await _Next(httpContext);
        }
    }

First thing, it looks for and holds onto any CORS related (starts with "Access-Control-") headers. Then it binds to the OnStarting event of the Response so that it can double check that those headers STILL exist. If not (as is the case with the dev exception middleware responses), then this puts them back in place.

[aspnet-hello]

From @PureKrome on Thursday, November 30, 2017 2:57:35 PM

@Jungers42

1. What happens if there's a 4xx/5xx error when !env.IsDevelopment()? No CORS headers then on the response? Assumption: you production "Web App" is SPA.

2. Nitpick: use StringComparison.OrdinalIgnoreCase (or whatever enum option suits the scenario) instead of ToLower or ToUpper for string checks.

if (!pair.Key.StartsWith("access-control-", StringComparison.OrdinalIgnoreCase))
{
   ... 
}

[aspnet-hello]

From @Jungers42 on Friday, December 1, 2017 6:03:31 AM

@PureKrome
Good point on the StringComparison ... I was working quickly trying to get past the problem yesterday and didn't go back to improve the code afterwards (surprised ReSharper didn't suggest it to me too!).

We are just starting to test the deployment of this replace API (and yes, we're consuming it in an Angular 4 SPA primarily ... at least where we are using CORS). My logic tells me that a 4xx/5xx err when NOT in Development will be just fine, CORS headers and all. Remember that the CORS middleware is adding the response headers for CORS into the response very early on (first thing in my startup) ... so unless some other middleware does a Response.Clear() (as the DevelopmentExceptionPage middleware does) or otherwise modifies the headers to remove the CORS headers, then they'll still be there on whatever response is generated. I only wanted to ensure that our pretty exception stack traces would be visible in dev tools when doing development to vastly simplify the debug process.

[szalapski]

I must agree with @PureKrome and @jungers. We need a way to configure even ASP.NET-generated errors to have the right CORS headers. I wasted much time with this issue: https://stackoverflow.com/questions/49900141