Well, we want to make sure that the server understands CORS, and also understands how CORS deals with credentials.

That is why there is this separate cache for credentialed requests as that also requires an additional header from the server and an explicit origin.

I could see us loosening that for preflight fetches as they are basically about making sure the server is aware of CORS, but given the distinction that exists today a server might become vulnerable if we change this.

E.g. a server could know that for OPTIONS it does the right thing and always signifies it does not support credentials, but has no such policy in place for other methods as that is not required by the specification as it is today.

> That is why there is this separate cache for credentialed requests as that also requires an additional header from the server and an explicit origin.

I understand that
- actual requests may differ if the "credentials mode" value is different,
- and the responses for them may also differ,
- so if we cache something from the responses, we must key the cache with the "credentials mode" value.

But,
- preflight requests are always made with credentials mode == omit
-- "10. Set preflight's credentials mode to omit."
- The CORS preflight cache is updated only based on responses to preflight requests
-- "12. For each method in methods for which there is no method cache match using request, create a new entry in CORS preflight cache as follows:"
-- "14. For each headerName in headerNames for which there is no header name cache match using request, create a new entry in CORS preflight cache as follows:"
So, I think
- there's no way for the server to detect the value of "credentials mode" of an actual request for which the received CORS preflight request was made.
- so, unless the server is broken and behaving randomly, it should send the same reply to the preflight requests independent of the "credentials mode" of the actual request.
- so, CORS preflight cache entries whose keys differ only for "credentials" always have the same value

I guessed that the reason why we key the CORS preflight cache with "request's credentials mode" because there's some attributes of "preflight" inherited from "request" that may be altered based on "credentials mode" value of "request" before "CORS preflight fetch" algorithm. But it seems not if I read the spec correctly.

> I could see us loosening that for preflight fetches as they are basically about making sure the server is aware of CORS, but given the distinction that exists today a server might become vulnerable if we change this.

I'm not suggesting any loosening of policy, algorithm, etc.

> E.g. a server could know that for OPTIONS it does the right thing and always signifies it does not support credentials, but has no such policy in place for other methods as that is not required by the specification as it is today.

Hmm, the CORS preflight cache is updated only for responses to OPTIONS requests. Right? I don't see any step for issuing non-preflight request in the spec that updates the CORS preflight cache.

Yeah, I see no good reason to key the preflight cache on the credentials mode. Especially since we don't send the credentials mode in the preflight request. So if two requests are made to the same server that only differ on the credentials mode, that would mean that we make the exact same preflight request to the server twice.

Or am I misunderstanding or misremembering? (It's been a while since I looked at CORS code or spec)

Jonas, I think the idea was that if the server did not respond with Access-Control-Allow-Credentials, it would fail for the credentialed case. You are right that the request is otherwise identical.

Previously this was tracked with the "omit credentials flag" but now we have more detailed and layered algorithms it seems I introduced a bug and a response to a preflight will never do a CORS check that looks at the credentials mode.

I would be okay with simplifying this though and never requiring Access-Control-Allow-Credentials as the current specification already does.

Actually, it seems good that servers need to indicate in the preflight that they can handle requests with cookies. Most websites do not need cookies since they instead pass credentials explicitly in the request. Exposing those servers to requests with cookies just adds risk.

Another way to put it is: Right now it's safe for any server that is connected to the public internet to have a OPTIONS handler which echos back any access-control-request-header/method headers. As long as it doesn't set the access-control-allow-credentials it won't be exposed to any requests that couldn't be done server-to-server.

That seems like a good property to maintain. Not to mention that at this point it would be a security problem to change.

What we could do is to allow that if a preflight response comes back with access-control-allow-credentials, that we enable both requests with and without credentials.

But if a preflight response comes back without access-control-allow-credentials, we'd still only allow requests without credentials.

Yeah, that seems fair. This will require some refactoring. Takeshi, are you in agreement with comment 5?

(In reply to Anne from comment #4)
> Previously this was tracked with the "omit credentials flag" but now we have
> more detailed and layered algorithms it seems I introduced a bug and a
> response to a preflight will never do a CORS check that looks at the
> credentials mode.

I see. There was "resource sharing check" in
"Cross-Origin Request with Preflight"
(http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0).



(In reply to Jonas Sicking from comment #5)
> the access-control-allow-credentials it won't be exposed to any requests
> that couldn't be done server-to-server.

I couldn't get what server-to-server here means. Do you mean that it's
good that:

unless the server explicitly declare that it's willing to handle requests
with credentials, the user won't be issuing requests with special method
or special headers with credentials. So, servers that don't implement
CORS correctly won't be exposed to such special and credentialed (actual)
requests

?

> 
> That seems like a good property to maintain. Not to mention that at this
> point it would be a security problem to change.
> 
> What we could do is to allow that if a preflight response comes back with
> access-control-allow-credentials, that we enable both requests with and
> without credentials.
> 
> But if a preflight response comes back without
> access-control-allow-credentials, we'd still only allow requests without
> credentials.

and maybe add something like "request's credentials mode is include" to
the conditions to set CORS preflight flag?



(In reply to Anne from comment #6)
> Yeah, that seems fair. This will require some refactoring. Takeshi, are you
> in agreement with comment 5?

I'm fine with adding back the Access-Control-Allow-Credentials check to
preflight response handling. Blink does that for now.

Oops. Sorry. There's "CORS check" inside "HTTP fetch" algorithm. CORS preflight fetch uses "HTTP fetch". So, CORS preflight fetch does check Access-Control-Allow-Credentials. Sorry I was misunderstanding.

So, Jonas's suggestion is to memorize the Access-Control-Allow-Credentials value in the response to a preflight request as part of the preflight cache?

Ah. Never mind #8

https://github.com/whatwg/fetch/commit/adec3d2bf35726b46dd6c0079ff01dba8e154711

This big refactoring factored out preflight issuing code and it broke.

What I mean is:

I could write a custom C++ program which makes HTTP requests to any server which is connected to the internet. Such a request can contain any headers and use any method of my choosing. It does not matter what CORS headers the server does or does not send in any situations.

Any webserver that is connected to the internet need to be written such that this does not become a security problem.

What enables many web servers to take sensitive actions is that the request contains some security credentials, usually in the form of cookies. This way the server knows that the request comes from a trustworthy source, rather than my C++ program.

Assuming that a webserver has been written in such a way, it is also currently secure for that webserver to write a OPTIONS handler which returns a 200 OK response with the following headers:

access-control-allow-headers: <value from access-control-request-headers>
access-control-allow-methods: <value from access-control-request-method>


However, if we were to change the spec such that this allowed POST requests with credentials, this would no longer be safe. Because this would allow an evildoer to:

1. Set up a website called "evildoer.com"
2. Send an email to a victim which contains a link to "evildoer.com"
3. Wait until the victim visits "evildoer.com" 
4. Once victim visits evildoer.com, send a cross-site request to
   "bank.com" for transferring all money to the evildoer's bank account.
5. Make the POST request contain CSRF-busting headers and/or methods as needed
   to make "bank.com" honor the request.

(In reply to Jonas Sicking from comment #10)

Please let me confirm my understanding.

In the latest spec, the attacker can guide the victim to issue
either of:

(a) cross-site req with credentials but without special headers
(b) cross-site req without credentials but with special headers
(c) cross-site req with both credentials and special headers

from a page placed at evildoer.com.

(b) is not so problematic as Jonas explained.

If the CSRF busting header the server uses is not one-time, allowing (c) type request would be a security issue for the server.

If we fix the "CORS preflight fetch" to do CORS check based on the actual request's "credential mode" (*), user agents will be again unable to issue (c).

Even before Anne's refactoring, CORS allowed (a). Only response tainting happened. But making the following addition I suggested in my comment #7 would break compatibility so we're not doing that. Right?

> and maybe add something like "request's credentials mode is include" to
> the conditions to set CORS preflight flag?

----

Anyway, Anne, I agree (*).

Here's a proposal to fix this that includes the change proposed in comment 5:

* Step 11 of CORS preflight fetch no longer sets the CORS flag.
* We introduce one new step after step 11 of CORS preflight fetch that performs a CORS check using /request/ (not /preflight/) and /response/. This ensures the original credentials mode is used for the purposes of the CORS check. (We'll add a note to that effect here too.)
* We update cache match to say "and either credentials is false and request's credentials mode is not include or credentials is true". That is, if credentials is true we do not care about request's credentials mode.

(In reply to Anne from comment #12)

Looks good

I made it slightly better still. CORS preflight fetch now directly invokes HTTP network or cache fetch rather than HTTP fetch.

https://github.com/whatwg/fetch/commit/49eb9d0e649331f9364a06767e5a17fc6155107b
https://github.com/whatwg/fetch/commit/1a06d2e1d5b9a1d6b27524e5063910dc11f2f178

(In reply to Anne from comment #14)
> I made it slightly better still. CORS preflight fetch now directly invokes
> HTTP network or cache fetch rather than HTTP fetch.
> 
> https://github.com/whatwg/fetch/commit/
> 49eb9d0e649331f9364a06767e5a17fc6155107b
> https://github.com/whatwg/fetch/commit/
> 1a06d2e1d5b9a1d6b27524e5063910dc11f2f178

It's a good side-effect that we now skip the SW handling steps.

It seems that "CORS preflight fetch" will lose 407 handling code. Is this OK?

(In reply to Takeshi Yoshino from comment #15)
> It seems that "CORS preflight fetch" will lose 407 handling code. Is this OK?

I think so. It's still a bit unclear to me how the 407 handling is supposed to work and how it works in browsers in practice. It seems somewhat bad that each request can potentially pop up a dialog.

(In reply to Anne from comment #16)

OK. Then, looks good.