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

Cannot "UseCors()" and "UseDeveloperExceptionPage()" together #247

Closed
sjdirect opened this Issue Feb 9, 2016 · 18 comments

Comments

Projects
None yet
8 participants
@sjdirect

sjdirect commented Feb 9, 2016

Trying to "UseCors()" and "UseDeveloperExceptionPage()" together in an Asp.net 5 web application but getting cors issues from the client.

The issue is that UseCors (which internally uses CorsMiddleWare.cs ) will set the response headers needed to support cors but then if there is an http error the UseDeveloperExceptionPage (which uses DeveloperExceptionPageMiddleware.cs ) somehow clears these headers.

This is the exact console log error....

XMLHttpRequest cannot load http://localhost:37734/badvalues/v1. No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://localhost:64369' is therefore not allowed access. The response had HTTP status code 500."

This also blocks the ability for javascript to get the http status that was returned since the browser will block the response from the point that it detects a cors issue.

@Tratcher

This comment has been minimized.

Member

Tratcher commented Feb 9, 2016

DeveloperExceptionPage is a really big hammer designed to give you some debug output in a scenario that otherwise would have failed. If it's taking control then aren't client console messages are the least of your worries? Do those messages prevent you from getting the diagnostic information from the error page?

@sjdirect

This comment has been minimized.

sjdirect commented Feb 9, 2016

No its not preventing me from getting the error details using fiddler. The problem is that most users of my api first assume that there is a cors error and they focus their efforts on that instead of the real error. It's just a bit misleading.

If the cors response headers were correctly passed along then the error message would be exactly what the issue is and not a side effect of it.

Thanks for responding so quickly!

@sjdirect

This comment has been minimized.

sjdirect commented Feb 9, 2016

Just added more detail to the original description...

@Eilon

This comment has been minimized.

Member

Eilon commented Feb 9, 2016

@kichalla I think you looked at a similar issue before. Do you recall the conclusion?

@kichalla

This comment has been minimized.

Member

kichalla commented Mar 2, 2016

@Eilon yes, its a similar issue as in the case of ExceptionHandlerMiddleware. So should we special case these headers from being deleted in these 2 middlewares?

@Tratcher

@Eilon

This comment has been minimized.

Member

Eilon commented Mar 2, 2016

Ah so we already clear some headers in ExceptionHandlerMiddleware? I would think we want the exact same behavior for the Dev Exception page too. Ultimately they're the same scenario, just one for dev, one for prod.

Should they both call some helper e.g. PrepareResponseForAwesomeErrors(httpResponse);?

@Tratcher

This comment has been minimized.

Member

Tratcher commented Mar 2, 2016

ExceptionHandlerMiddleware doesn't special case any headers either, both middleware just call Clear. I don't think we should start guessing at what headers to keep until we run into functional blockers. So far it's just a mild inconvenience in what is already an error case.

The ExceptionHandlerMiddleware isn't as impacted in its re-execution mode because middleware after it (CorsMiddleware) will get a chance to re-run.

@Eilon

This comment has been minimized.

Member

Eilon commented Mar 2, 2016

Ah ok so the fix is to always just fully clear everything that can be cleared? I can dig that.

@kichalla

This comment has been minimized.

Member

kichalla commented Mar 2, 2016

The ExceptionHandlerMiddleware isn't as impacted in its re-execution mode because middleware after it (CorsMiddleware) will get a chance to re-run.

Are you sure about this? The Cors middleware looks for Cors specifc headers (ex: Origin) to consider it a Cors request and when try to re-execute the request in ExceptionHandlerMiddleware we should be having that header in the new request again. I am not sure if we currently do this.

@brockallen

This comment has been minimized.

brockallen commented Mar 3, 2016

Do you really want to have to special case one middleware to know how another works?

The VS templates should encourage people to split their apps or at least the pipeline into 2: web apps, and web apis. If there was a better partition in the apps or the pipeline then stuff like this wouldn't happen as often.

@Tratcher

This comment has been minimized.

Member

Tratcher commented Mar 4, 2016

@Eilon No changes required, everything already gets cleared.

@kichalla we only clear the response, not the request.

@sjdirect

This comment has been minimized.

sjdirect commented Mar 4, 2016

I believe that clearing the RESPONSE headers IS the problem. I just wanted to add that because I don't know if that is clear after reading the latest comments. If the response headers are cleared of headers like the following...

Access-Control-Allow-Origin: http://foo.example
Access-Control-Allow-Methods: POST, GET, OPTIONS
Access-Control-Allow-Headers: X-PINGOTHER
Access-Control-Max-Age: 1728000

Then cors is not compatible with UseDeveloperExceptionPage.

@Tratcher

This comment has been minimized.

Member

Tratcher commented Mar 4, 2016

Yes, we know, but we've decided not to special case CORS in UseDeveloperExceptionPage as it's not a blocking issue, it only causes some warnings on the client.

@sjdirect

This comment has been minimized.

sjdirect commented Mar 4, 2016

I understand that its not a high priority. However, is this a "we will fix it in the future" kind of thing or is it a "middleware should not have any knowledge of other middleware and we choose not to fix it " kind of thing?

Either way, thanks for considering the change and we love where Asp.net Core is heading.

@Tratcher

This comment has been minimized.

Member

Tratcher commented Mar 4, 2016

We much prefer the latter, keeping things decoupled, up until we hit a functional blocker. If this doesn't block any scenarios then we won't fix it.

@kichalla kichalla added 1 - Ready and removed 2 - Working labels Mar 8, 2016

@Eilon Eilon added this to the 1.0.0 milestone Mar 18, 2016

@Eilon Eilon modified the milestones: 1.0.1, 1.0.0 May 22, 2016

@Eilon Eilon modified the milestones: 1.1.0, 1.1.0-preview1 Oct 6, 2016

@Eilon Eilon modified the milestones: 1.2.0, 1.1.0 Oct 26, 2016

@Eilon Eilon removed this from the 2.0.0-preview1 milestone Apr 19, 2017

@Eilon Eilon modified the milestones: 2.0.0-preview2, 2.0.0-preview1 Apr 19, 2017

@DamianEdwards

This comment has been minimized.

Member

DamianEdwards commented Jun 5, 2017

We have no intention of doing anything with this issue in the upcoming milestones so I'm closing.

@benaadams

This comment has been minimized.

benaadams commented Nov 17, 2017

Problem is you can't read the 500 status code

@jonathaneckman

This comment has been minimized.

jonathaneckman commented Jan 14, 2018

This creates confusing errors downstream. I spent all day walking from a fetch error in my SPA back to this middleware. I wrote about the issue in detail here. If it wont be fixed, it would help a lot of devs if it was documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment