Add CORS section

[RubenVerborgh]

• Write background
• Explain which parts of the CORS spec should be implemented
• Write security considerations.

Closes #12.

[dmitrizagidulin]

👍 (nicely done so far!)

[dmitrizagidulin]

👍 , really nicely done.

[acoburn]

👍
I know this is really minor, but I especially appreciate the short line length in this PR. Short lines will mean that, when there are multiple PRs focused on similar or adjacent sections of the text, there will be considerably less rebasing required.

[RubenVerborgh]

@csarven Thanks for the review, made several editorial changes in cd7af5b based on your feedback.

[justinwb]

Nice work @RubenVerborgh 👍

[csarven]

@RubenVerborgh This is looking all around great so fine to merge (pending remaining minor edits). We'll surely revisit as we go.

There is one thing that we should consider whether to expand on or not (perhaps as a Note): While credentials and Origin shouldn't be relied upon, there are potholes that need to be navigated around. Should we insist on ACAO * and no credentials? And/or if ACAO echos the Origin, clients should request with Cache-Control no-store (minding the caveats that come with it)?

[RubenVerborgh]

Should we insist on ACAO * and no credentials?

We're already insisting on not using *. Because right now, it breaks for credentials, but who knows what CORS will break in the future because of its Living Standard status. I don't think * is a good workaround for caching, a problem that has its causes elsewhere.

And/or if ACAO echos the Origin, clients should request with Cache-Control no-store (minding the caveats that come with it)?

I know you've looked into this, but I would first need to see a reproducible example to make sure that this is indeed a fully functional workaround (and if it is, whether we cannot tweak it into a better solution).

This is looking all around great so fine to merge

Thanks for the reviews, all. @csarven If you could top off with an approval, would be nice.

[michielbdejong]

looks great!

explicitly enumerate
all used response headers under Access-Control-Expose-Headers
rather than resorting to *,
which does not cover all cases

is there a link about that somewhere?

(such as credentials mode set to include).

Is there a link somewhere explaining how Access-Control-Expose-Headers affects .withCredentials? I think that was the only part i didn't understand.

Also, I don't understand why an app would send credentials other than the Authorization header, because sending for instance cookies to a cross-origin API sounds like a pretty bad plan in general?

[RubenVerborgh]

Thanks @michielbdejong!

is there a link about that somewhere?

Yes; these are all part of the normative FETCH reference preceding them.

Is there a link somewhere explaining how Access-Control-Expose-Headers affects .withCredentials?

That would also be the FETCH reference. I've checked it there, and it does not seem affected. (There is no * mode for that header.)

Also, I don't understand why an app would send credentials other than the Authorization header, because sending for instance cookies to a cross-origin API sounds like a pretty bad plan in general?

On the one hand, this is a matter of spec orthogonality. Just in case there are cookies (for anything, not limited to auth), they should be supported. The CORS section does not mandate if and how authentication happens; could be with cookies, could be without. Also, they are just an example; the bottom line is that no request/response should be blocked at the browser level.

On the other hand, WebID-OIDC needs cookies for a crucial step in the authentication process, so we do have a concrete case for it already.

[kjetilk]

Four approvals, none against. Obviously, it needs more work, but I assume we're go on merging this?

[RubenVerborgh]

Thanks to @csarven, @dmitrizagidulin, @justinwb, @kjetilk, @michielbdejong, @pmcb55 for feedback!

Four approvals, none against. Obviously, it needs more work, but I assume we're go on merging this?

Merging seems currently blocked by solid/process#95; I will inquire about the status.

[michielbdejong]

in case there are cookies (for anything, not limited to auth), they should be supported

not sure I agree, because then i think they would be shared across apps if the user happens to have them open in the same browser session? For instance, if a UI like the data browser or a control panel on the pod's domain sets a cookie, then a third-party web app would do an XHR request and be allowed to do its request with that cookie from the on-pod UI?

[michielbdejong]

Let's make absolutely sure the text about the .withCredentials thing is safe, because I think in this case cookies would stick to the pod domain and not to the app domain, and thus could leak between apps. Other than that, 👍

[RubenVerborgh]

For instance, if a UI like the data browser or a control panel on the pod's domain sets a cookie, then a third-party web app would do an XHR request and be allowed to do its request with that cookie from the on-pod UI?

That is correct.

And our assertion in this document is that this MUST NOT be prevented by the browser. So we purposely do not rely not on browser security mechanisms.

However, the server is free to block cookies from third-party origins itself.

think in this case cookies would stick to the pod domain and not to the app domain, and thus could leak between apps.

Cookies will never "leak"; withCredentials is about whether or not cookies are included with the request. This option is not about who can see them; that is always only the pod server.

[michielbdejong]

i see how it's consistent with the "don't rely on cors" narrative, and i also see how the hole may still be stopped at the next layer.

we purposely do not rely not on browser security mechanisms.

I think that's at least unconventional and possibly also bad practice. so we should make sure we instruct server implementers about how to replace the security measures they are removing there.

i think it would look like a bug to any security auditor if you allow third-party web apps to send http requests with the cookie of an on-pod UI.

could you at least add a note to implementers saying that allowing .withCredentials means that if the server uses cookies for some on-pod UI then the server code needs an extra if-statement in front of the session handling middleware that removes the cookie from the request based on some regex of the http headers etc

[michielbdejong]

btw i do agree about the not-relying-on-browsers for the rest of it, just not for the cookies thing that's mentioned right at the end of the text.

[RubenVerborgh]

we purposely do not rely not on browser security mechanisms.

I think that's at least unconventional and possibly also bad practice.

Unconventional, yes, but by design (and without it, no Solid in browsers).
Bad practice, no, because we put in place other authorization mechanisms.

we should make sure we instruct server implementers about how to replace the security measures they are removing there.

Yes, agreed. Note that there already is a statement saying that data pods MUST NOT rely on cross-origin protections (such as those against cookies), but it is indeed better to say this explicitly.

could you at least add a note to implementers saying that allowing .withCredentials means that if the server uses cookies for some on-pod UI then the server code needs an extra if-statement in front of the session handling middleware that removes the cookie from the request based on some regex of the http headers etc

Good point. Since that solution is perhaps too implementation-specific, I added that untrusted cookies MUST be ignored in 0f4d5dc. Thanks!

[michielbdejong]