igrigorik / http-client-hints

401 stars 24 forks source link

Unclear on cross-origin interaction & header lifetime #68

Closed jakearchibald closed 8 years ago

jakearchibald commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

If:

…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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

igrigorik commented 8 years ago

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

jakearchibald commented 8 years ago

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 commented 8 years ago

(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 commented 8 years ago

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 commented 8 years ago

Migrating issues to HTTP-WG repo. Let's continue this at https://github.com/httpwg/http-extensions/issues/141.