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

Unclear on cross-origin interaction & header lifetime #68

Closed
jakearchibald opened this issue Dec 10, 2015 · 16 comments
Closed

Unclear on cross-origin interaction & header lifetime #68

jakearchibald opened this issue Dec 10, 2015 · 16 comments

Comments

@jakearchibald
Copy link

http://igrigorik.github.io/http-client-hints/#advertising-support-for-client-hints

When a client receives Accept-CH, it SHOULD append the Client Hint headers that match the advertised field-values. For example, based on Accept-CH example above, the client would append DPR, Width, Viewport-Width, and Downlink headers to all subsequent requests.

What does "all subsequent requests" mean in this context? Here's my guess:

"subsequent requests with this client, to the same origin as client, with destination 'subresource' " - this seems to make the most sense to me. Without the same-origin restriction, the new headers will trigger CORS preflight requests because they aren't simple. Servers such as Google Fonts don't support preflights.

You probably want a way to opt into client hints for additional origins too. Eg:

Accept-CH: DPR, Width, Viewport-Width, Downlink; add-origin 'http://example.com'
@yoavweiss
Copy link
Contributor

Headers are not limited to same origin, and adding an opt-in specific per origin would add complexity and burden users (and may not be practical for some of the use-cases).

I agree with you that for CORS enabled requests this will create an issue due to the current definition of "simple request". I think that definition needs to be updated to include headers that the browser normally sends such as "DPR", "Width", "Viewport-Width", "Save-Data", "Downlink", "Beacon-Age", "Upgrade-Insecure-Request", etc.

IMO sending these headers without preflight won't increase any risk of triggering unwanted behavior in internal servers, as an attacker can already send a request for a non-CORS enabled resource (e.g. image) to such servers, which would include these headers.

/cc @mikewest @annevk

P.S. I must be missing something in current definition you linked to, but my reading of it is that no request today is a simple request (as requests regularly contains headers such as Host:, which don't qualify as simple headers).

@jakearchibald
Copy link
Author

I think that definition needs to be updated to include headers that the browser normally sends such as "DPR", "Width", "Viewport-Width", "Save-Data", "Downlink", "Beacon-Age", "Upgrade-Insecure-Request"

If adding those isn't a security issue, then yeah that's another solution. The scope of "subsequent requests" still needs to be defined though, in terms of lifetime and the types of requests effected.

my reading of it is that no request today is a simple request (as requests regularly contains headers such as Host:, which don't qualify as simple headers).

https://fetch.spec.whatwg.org/#http-fetch - the header check happens in step 4.1, but the host header isn't added until part of 4.4.

@yoavweiss
Copy link
Contributor

https://fetch.spec.whatwg.org/#http-fetch - the header check happens in step 4.1, but the host header isn't added until 4.4.

Oh, cool, so this is what I was missing. Alternatively, we could also add all those browser-generated headers at that point as well.

@jakearchibald
Copy link
Author

If:

  • The user should be able to perform a CORS fetch() to another origin including a DPR header without triggering a preflight, or:
  • The DPR header should be observable within a service worker's fetch event for cross-origin fetches

…the header must be put on the simple list. Otherwise it can be set at the same time as the host header.

Or to put it another way, if I pull a request out of the cache API, should I be able to see a DPR header on it if the original request had one? If I then fetch(thatRequest), would the request have the original DPR header, or a new one based on the request's client?

@igrigorik
Copy link
Owner

Right, I believe the right solution here is to add the necessary CH headers (DPR, Width, Viewport-Width, Save-Data) to the "simple list": https://fetch.spec.whatwg.org/#simple-header

@annevk
Copy link

annevk commented Dec 15, 2015

I think this comes down to several issues:

  1. Was sending new headers cross-origin security reviewed?
  2. Do we want web developers to be able to set these headers to any value cross-origin without server optin? (This is what "simple headers" is about.)
  3. At what point do these headers get initialized and set?

Maybe there's more?

@igrigorik
Copy link
Owner

Or to put it another way, if I pull a request out of the cache API, should I be able to see a DPR header on it if the original request had one?

I believe so. You can see Content-Type, etc, correct? You should be able to see the DPR + Content-DPR headers, as that may affect how you store and process the response.

If I then fetch(thatRequest), would the request have the original DPR header, or a new one based on the request's client?

Interesting question.. I guess original? The reason I say that is because while we could update the DPR value, the Width value is conditional on the element that initiated the original request. Updating one but not the other could break things.

Was sending new headers cross-origin security reviewed?

Yes, we went through a security review prior to shipping it in Chrome.

Do we want web developers to be able to set these headers to any value cross-origin without server optin? (This is what "simple headers" is about.)

Yes.

At what point do these headers get initialized and set?

They're set when the image request is created by relevant element -- e.g. img/srcset/etc.

@annevk
Copy link

annevk commented Dec 16, 2015

Content-Type is not a request header so no, you can't see that.

Yes.

Did the security team review this too? That seems like a pretty drastic change from before.

@igrigorik
Copy link
Owner

Content-Type is not a request header so no, you can't see that.

Sorry, meant to say Accept.

Did the security team review this too? That seems like a pretty drastic change from before.

Yes. Note that CH opt-in applies to all subresources and origins on the page, and not just for own origin. I would expect that hint values should be accessible from within SW, both to inspect and to modify.

@annevk
Copy link

annevk commented Dec 17, 2015

That is different from being able to set the header to any value though. Are you saying Chrome has already modified its CORS implementation and forgot to file an issue? Seems doubtful.

@jakearchibald
Copy link
Author

No, at the moment fetch(fetchEvent.request) fails if the request is CORS and contains CH headers.

@igrigorik
Copy link
Owner

So, I'm confused on what our options are here..

  • Have the browser set the headers but don't allow developer override?
  • Have the browser set the headers + allow developer override without preflight - i.e. treat them as simple headers.
  • Others?

@jakearchibald
Copy link
Author

Options are:

  1. Add headers as part of 5.4 https://fetch.spec.whatwg.org/#http-network-or-cache-fetch - this means the headers cannot be set by developers, cannot be observed in the service worker, and will not be included in the request when stored in the cache API.
  2. Add a flag to a header to indicate if it was developer-added - headers that aren't developer added would not trigger a CORS preflight, and would be allowed in no-cors requests. Add CH headers (not developer-added) before step 3 of https://fetch.spec.whatwg.org/#http-fetch, unless they're already set.
  3. Add CH headers to the simple headers list, allowing developers to set them to whatever they want.

1 sounds like the safest option, but may be lacking due to lack of visibility in a service worker. If we went from 1 & later moved to 2 or 3… that could be a non-breaking change.

@igrigorik
Copy link
Owner

(1) is a non-starter: it's counter what we've already shipped in Chrome; it breaks important developer use cases for CH; it breaks caching.

What is the argument for disallowing developers from setting CH header values - i.e. treating them as simple headers?

@annevk
Copy link

annevk commented Jan 1, 2016

I don't understand. Are you asking about 2 or 3? The problem with 3 might be that servers do not anticipate hostile values in these headers since to date only user agents were able to generate them.

@igrigorik
Copy link
Owner

Migrating issues to HTTP-WG repo. Let's continue this at httpwg/http-extensions#141.

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

4 participants