Skip to content
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

Supporting "no-cors" mode requests seems problematic. #39

Open
horo-t opened this issue Jul 10, 2023 · 12 comments
Open

Supporting "no-cors" mode requests seems problematic. #39

horo-t opened this issue Jul 10, 2023 · 12 comments

Comments

@horo-t
Copy link
Contributor

horo-t commented Jul 10, 2023

When a browser fetches a cross-origin script (eg: <script src='https://static.example.com/script.js'> in https://www.example.com/index.html) , it sends a request with the mode set to no-cors and the credentials set to include.

The current explainer allows this type of request for both registering as a dictionary and using a registered dictionary for its decompression, as long as the response header contains a valid Access-Control-Allow-Origin header (* or https://www.example.com).

However, if we follow the CORS check step in the Fetch spec, the response header must also contain the Access-Control-Allow-Credentials: true header, and Access-Control-Allow-Origin header must be https://example.com. This means that the server must know the origin of the request, even though the request does not include an Origin header. (It may include a Referer header. But the Origin header and Referer header are conceptually different.)

For this reason, now I think supporting no-cors mode requests is problematic.
Maybe we should support only navigate, same-origin, and cors mode requests?

@pmeenan @yoavweiss
Do you have any thoughts?

@yoavweiss
Copy link
Collaborator

The current explainer allows this type of request for both registering as a dictionary and using a registered dictionary for its decompression, as long as the response header contains a valid Access-Control-Allow-Origin header (* or https://www.example.com).

That's not how I read the explainer (although I agree it can have tighter language). Specifically in static resource flow it says that "the response must also be non-opaque in order to be used as a dictionary"

@horo-t
Copy link
Contributor Author

horo-t commented Jul 10, 2023

Ah, I may have misunderstood.

@pmeenan Do you think it is ok not to support no-cors cross origin requests?

Supporting no-cors requests is desirable for ease of deployment. But it sounds too risky from the security point of view.

If we don't support no-cors cross origin requests, cross origin script tags in the explainer's examples should have crossorigin attribute.

@pmeenan
Copy link
Collaborator

pmeenan commented Jul 10, 2023

We definitely need to support no-cors cross origin requests assuming the set of headers would allow it for fetch, I just need to tighten up the language around it. Anything that is non-opaque to fetch should be allowed to be used for a dictionary and compressed payload.

I'll reach out to the security team to see if I can get some help crafting the exact combination of headers and language to spec it completely but for this case it sounds like Access-Control-Allow-Credentials: true needs to be added to the response.

There's a matrix of Sec-Fetch-Mode, Sec-Fetch-Site, Origin, Referer, Access-Control-Allow-Origin and Access-Control-Allow-Credentials that needs to be fleshed out better (and even in the case where an Origin request header isn't sent, if Access-Control-Allow-Origin allows for the actual origin it should still be allowed (I think).

I'm far from an expert in CORS though and the exact conditions for being non-opaque in all of the different fetch modes so I'll find someone to help clear up the specifics. There are a fair number of sites that have their static scripts and css on a separate subdomain even though they're "1st party" so we need to make sure it works for adoption.

@pmeenan
Copy link
Collaborator

pmeenan commented Jul 10, 2023

There's a good chance I'm wrong though and no-cors is by-definition CORS-opaque in which case the example needs to be updated. Just need to check quickly with those that live and breathe this on a daily basis.

pmeenan added a commit to pmeenan/compression-dictionary-transport that referenced this issue Jul 10, 2023
pmeenan added a commit to pmeenan/compression-dictionary-transport that referenced this issue Jul 10, 2023
@horo-t
Copy link
Contributor Author

horo-t commented Jul 11, 2023

How about same origin no-cors requests? For example, the request mode for <script src='./script.js'> is no-cors.

Supporting no-cors same origin requests would make the browser logic more complex. It needs to track redirects, and stop sending sec-available-dictionary header when cross origin redirect happens.

The request mode for <script src='./script.js' crossorigin> is cors. If we don't need to support no-cors same origin requests, the browser just need to check the mode only when starting the request.

@pmeenan
Copy link
Collaborator

pmeenan commented Jul 11, 2023 via email

@yoavweiss
Copy link
Collaborator

Supporting no-cors same origin requests would make the browser logic more complex. It needs to track redirects, and stop sending sec-available-dictionary header when cross origin redirect happens.

No-CORS same-origin requests are not opaque, and are something we should support. I'd imagine that the redirect tracking is something we're already doing (e.g. to enable such responses to be readable in fetch() calls.

@pmeenan pmeenan reopened this Jul 11, 2023
@pmeenan
Copy link
Collaborator

pmeenan commented Jul 11, 2023

Sorry for the delay, wanted to test the permutations to be sure. I used CSS because you can test for opaqueness by trying to access the cssRules from script. I set up a test page at https://test.patrickmeenan.com/opaque/ that loads a CSS from the same origin and from another subdomain under the same TLD.

The same-origin request is not opaque and is fetched with:

sec-fetch-mode: no-cors
sec-fetch-site: same-origin

The other request does not have sec-fetch-site.

I'm going to do a little more testing to verify that the sec-fetch-site state is updated as redirect chains are followed (I assume they are). If so, the solution is probably to always allow for a resource to be a dictionary as long as sec-fetch-site = same-origin.

Definitely need to get security peeps to weigh in though because sec-fetch-site is listed as a non-CORS header so we need to be sure that it can be used for this case.

@pmeenan
Copy link
Collaborator

pmeenan commented Jul 11, 2023

Updated the test page to also try a same-origin request that redirects to cross-origin and the sec-fetch-site correctly gets reset on the redirect (and it is opaque from JS).

WebPageTest result with the headers is here.

I'll update the processing flow in the ID to have a first step in the processing that allows for dictionary use for sec-fetch-site: same-origin

@pmeenan
Copy link
Collaborator

pmeenan commented Jul 11, 2023

I have an updated version of the processing in my github repo for the ID: https://pmeenan.github.io/i-d-compression-dictionary/draft-meenan-httpbis-compression-dictionary.html

@horo-t
Copy link
Contributor Author

horo-t commented Jul 12, 2023

In the Fetch spec, a request has an associated response tainting flag, which is "basic", "cors", or "opaque". This is initially "basic" for same-origin request. And it is set to "opaque" when the request is redirected to a cross origin URL.

I think browsers should use a shared dictionary only when the response tainting is "basic" or "cors".

In Chromium, the response tainting is calculated in network::cors::CorsURLLoader when sending a request and handling a redirect. Unfortunately, the SharedDictionaryNetworkTransaction class that decodes shared dictionary compression is a bit far from network::cors::CorsURLLoader. I need to figure out how to pass the flag.

@yoavweiss
Copy link
Collaborator

Can this be closed?

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

No branches or pull requests

3 participants