Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Report 200 OK on all CORS preflight requests. #565

Closed
jgauffin opened this issue May 26, 2014 · 14 comments
Closed

Report 200 OK on all CORS preflight requests. #565

jgauffin opened this issue May 26, 2014 · 14 comments

Comments

@jgauffin
Copy link

This is a copy of my Codeplex issue as requested by yishai galatzer

A CORS preflight request is about establishing trust.

It's purpose is not to tell whether or not a resource might work, but if a trust can be established between between the calling domain and the requested resource.

Hence a 200 OK should always be returned if the defined CORS rules are OK for the requested resource.

In my case I've added a global CORS filter, using your nuget package, which should accept all requests from a specific domain. Hence it should work.

The problem is that if the action is not found in the controller ASP.NET will return a 404 for the OPTIONS request. That is interpreted as there is no trust between the requester and the web server. Ultimately the JavaScript have no idea why the request fails.

This is therefore a feature request: Return 200 for the OPTIONS request and 404 for the real ajax request

@davidfowl
Copy link
Member

CORS will likely go through a full redesign, but we'll take this into account when we do it.

@yishaigalatzer
Copy link
Contributor

Thanks @jgauffin

@yishaigalatzer yishaigalatzer added this to the 1.0.0-rc1 milestone May 28, 2014
@danroth27 danroth27 removed this from the 1.0.0-rc1 milestone May 29, 2014
@danroth27 danroth27 mentioned this issue May 29, 2014
@yishaigalatzer yishaigalatzer added this to the Backlog milestone Jul 22, 2014
@Eilon
Copy link
Member

Eilon commented Dec 27, 2016

I believe the current implementation meets the CORS spec in regard to status codes. I couldn't find anything in http://www.w3.org/TR/cors/ that contradicts the implemented behavior.

I kind of get what you're saying about returning a 200, but it also seems perfectly reasonable to return a more specific status code if it is safe to do so.

@Eilon Eilon closed this as completed Dec 27, 2016
@jgauffin
Copy link
Author

What I'm saying is that ajax fails without giving you the status code in the error object. How are you supposed to diagnose that from javascript?

@Eilon
Copy link
Member

Eilon commented Dec 28, 2016

@jgauffin I think in that case all the JavaScript will know is that there was an error - but you're right that it won't know which error. Ultimately the application itself will need to provide more meaningful errors for known cases.

Suppose the opposite were true and CORS returned a 200 in this case. Then the "real" request would go through and the server would throw some other error, which the client also wouldn't understand unless the server was already designed to send back a more meaningful error (such as a JSON payload describing the error condition).

So what we recommend in this case is to follow an approach such as aspnet/CORS#46 (comment), where the person wrote an MVC filter that translates certain errors into JavaScript-friendly data that can be interpreted meaningfully on the client (though you can use many other mechanisms as well).

@jgauffin
Copy link
Author

jgauffin commented Dec 28, 2016

You cannot mix reasoning regarding regular ajax requests and preflight requests. They are very different and should not be handled in the same way by ASP.NET. What you say is true for regular ajax requests only.

The CORS preflight request isn't exposed to the JS engine in the browser like normal ajax requests are. The HTTP status code is always 0 if it fails to indicate that it was the preflight and not the real Ajax request that failed. You can for instance read about it here: http://stackoverflow.com/questions/31312703/whenever-a-cors-http-request-fails-the-response-returned-is-always-0

So 404 doesn't make sense since the preflight request is just to check if the remote peer is allowed to access the resource. Let the preflight request succeed with 200 since the remote peer is allowed to access the resource and then let the tailing ajax request fail with 404.

@jgauffin
Copy link
Author

@davidfowl what do you think?

@Eilon Eilon removed this from the Backlog milestone Dec 28, 2016
@Eilon Eilon reopened this Dec 28, 2016
@Eilon
Copy link
Member

Eilon commented Dec 28, 2016

Ah, you know what, I'm seeing this differently now, thanks for your patience with me 😄

Let me see if I understand. You're saying that for a CORS pre-flight request the only thing that should matter is whether the "CORS stuff" is valid, i.e. the various CORS access request matches what the endpoint says should be allowed to be accessed. Then, one THAT is done, when the "real" request goes through, just behave normally with regard to errors, etc.?

Is this correct?

@jgauffin
Copy link
Author

jgauffin commented Dec 28, 2016

No problem. Giving up is not my thing ;)

You are correct. The purpose of the preflight request is to validate access when using custom headers or custom HTTP verbs. It doesn't care about the actual resource. That's why it's an OPTIONS request. It should either succeed (200) or fail due to access errors.

When it's done the actual HTTP request will be made (using one of the more common HTTP verbs). That request can fail with 404. This request is the one which is exposed by the JS engine in the web browser.

The CORS specification isn't so clear about the HTTP status codes. Here is the section about the preflight requests: https://www.w3.org/TR/cors/#preflight-request

@Eilon
Copy link
Member

Eilon commented Dec 30, 2016

@jgauffin thanks again, this makes sense to me.

@kichalla let's review with @blowdart when everyone is back in town and come up with a plan.

@kichalla
Copy link
Member

kichalla commented Jan 4, 2017

@kichalla let's review with @blowdart when everyone is back in town and come up with a plan.

Sure

Just thinking out loud. To a certain point probably this feature can already work i.e. to use the CorsMiddleware in front of MVC, so for any pre-flight request either if the policy succeeds/fails, a 204 response is sent back to the client. Yeah, its currently 204 but it could be changed to return a 200 as a fix.
MVC's Cors implementation is tied to filters which is in turn tied to resources, so if a resource is not present we wouldn't having filters to begin with for any pre-flight request evaluation.

Preflight request Resource found Policy Preflight response Actual request More details
Yes Yes Success 200 OK with expected Cors headers 404/500/200/400/etc.
Yes Yes Failure 200 OK not having expected Cors headers Client shouldn't be making a call here to begin with
Yes No Success 200 OK having expected Cors headers 404/etc.
Yes No Failure 200 OK not having expected Cors headers Client shouldn't be making a call here to begin with

@kichalla
Copy link
Member

kichalla commented Jan 5, 2017

@jgauffin We discussed about this in our team and came up with the following solution:

Register the Cors middleware after the MVC middleware. Some info:

  • Any request which MVC cannot handle (example: /foo/bar for which no controller named foo exists) passes through it to the next middleware in the pipeline, which in this case would be the Cors middleware which on seeing the preflight request, send backs a success status code. And on the actual request, it would pass through MVC, Cors middleware (without any of them responding to the request) and finally the server would respond back with a 404.

  • We do not want the Cors middleware to be before MVC because requests can be valid or not-valid(ex:404). In case of valid requests, we want MVC's Cors policy infrastructure(ex: cors policy specific to controller/action etc.) to take effect without being trumped by the Cors middleware upfront.

public void ConfigureServices(IServiceCollection services)
{
    services.AddCors(corsOptions =>
    {
        corsOptions.AddPolicy("policy1", corsPolicyBuilder =>
        {
            corsPolicyBuilder
            .WithOrigins("http://abc.com")
            .WithMethods("GET", "POST", "PUT", "DELETE")
            .AllowAnyHeader();
        });
        corsOptions.AddPolicy("policy2", corsPolicyBuilder =>
        {
            corsPolicyBuilder
            .WithOrigins("http://def.com")
            .AllowAnyHeader()
            .AllowAnyMethod();
        });
    });

    services.AddMvc();
}

public void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)
{
    app.UseMvc(routes =>
    {
        routes.MapRoute(
            name: "default",
            template: "{controller=Home}/{action=Index}/{id?}");
    });

    // A more relaxed policy (obviously because as in MVC you do not get the granularity of applying policy
    // to an action/controller, for example) to allow the pre-flight requests to succeed and allow the actual
    // requests to fail (ex: 404)
    app.UseCors(corsPolicyBuilder =>
    {
        corsPolicyBuilder
        .WithOrigins("http://abc.com", "http://def.com")
        .AllowAnyHeader()
        .AllowAnyMethod();
    });
}

@kichalla
Copy link
Member

kichalla commented Jan 9, 2017

Closing this issue. Let us know if the above workaround does not work for you.

@kichalla kichalla closed this as completed Jan 9, 2017
@jgauffin
Copy link
Author

jgauffin commented Jan 10, 2017

Thanks for taking time to discuss this.

Regarding 204, it's fine since no body is required.

Got a question regarding the last option in the table (Failure 200 OK not having expected Cors headers*). Shouldn't that return a 403 if the CORS middleware policy has not allowed custom headers?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants