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

CH processing, cross-origin redirects and service workers #800

Closed
yoavweiss opened this issue Aug 21, 2018 · 14 comments
Closed

CH processing, cross-origin redirects and service workers #800

yoavweiss opened this issue Aug 21, 2018 · 14 comments
Labels
needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan topic: client hints

Comments

@yoavweiss
Copy link
Collaborator

During an IRC discussion about #773, @annevk brought up a few points we need to consider when it comes to CH processing:

  • If we want to make sure that CH headers are not sent on cross-origin request headers, including cross-origin redirects, we need to have origin checks relatively low in the stack and only add those headers there.
  • However, that would mean that the headers will not be exposed to service workers, which would limit some of their use-cases.
  • An alternative would be to add the headers up the stack and remove them further down for cross-origin redirects, but that would mean we'd also remove them for user-added CH headers. It'd also mean that CH is the first feature introducing header removal.

So, we need to decide on the trade-off between privacy, usability and functionality here:

  • How awful would it be to send CH to cross-origin redirects?
  • If the answer to the above is "really awful", we need to find a way to avoid doing that without harming SW use of CH as well as user-added CH headers. Personally, I think the SW use case is more important than the user-added headers case.

@igrigorik @arturjanc - thoughts?

@annevk annevk added needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan topic: client hints labels Aug 21, 2018
@igrigorik
Copy link
Member

First, for context, per current plan we will use Feature Policy to enable CH delegation. Under that model, 1P will have to explicitly delegate delivery of CH to 3P origins; CH hints will not be sent to 3P origins by default.

So, above question should be (re)scoped to: if a CH-whitelisted origin redirects to a non-whitelisted origin, what's the behavior we expect — correct?

If so, I think the answer should be that CH headers are stripped on such redirects because the destination origin is not on the whitelist. Any reasons for why this shouldn't be the case?


I'm not sure I follow why SW wouldn't see these headers? We definitely do want SW to see them, as they can and should be used to do resource selection + caching when doing CH content negotiation.

@annevk
Copy link
Member

annevk commented Aug 24, 2018

@igrigorik I think what's missing in your analysis is that 1P is the attacker. So it safelisting 3P is immaterial.

@yoavweiss
Copy link
Collaborator Author

If 1P is the attacker, they can already leak that (not-really-private) information willingly to any 3P by many different means.

@annevk
Copy link
Member

annevk commented Aug 24, 2018

It's not about leaking, it's about attacking 3P through those headers. There's a reason we did #736.

@yoavweiss
Copy link
Collaborator Author

OK, so we're talking about different threat models. Is the threat model user-generated headers? Would limiting the headers' values similarly to #736 help?

That seems doable without sacrificing any of the use-cases (and regardless of FP).

@annevk
Copy link
Member

annevk commented Aug 24, 2018

It helps, but better would be to not extend that safelist.

@yoavweiss
Copy link
Collaborator Author

Not extending the safelist for UA generated values would add a significant cost to introducing any new request headers. That makes little sense. (For both CH as well as other UA generated headers. e.g. upgrade-insecure-requests).

Preflights don't come for free, and we should carefully consider the (infinitesimal IMO) risk of sending newly-introduced UA generated headers (with value restrictions) to servers vs. the certainty of inflicting performance pain on all/many users as a result.

/cc @mikewest

@annevk
Copy link
Member

annevk commented Aug 24, 2018

If Origin Policy is successful preflights do in fact come for "free". But also, routinely breaking the same-origin policy is a dangerous game with lots of potential bad consequences.

@yoavweiss
Copy link
Collaborator Author

@annevk just to make sure, was your main concern in the IRC discussion mentioned in the OP about CH being CORS safelisted? Are you also concerned about info leakage?

@annevk
Copy link
Member

annevk commented Aug 24, 2018

I posted my thoughts here: https://freenode.logbot.info/whatwg/20180824#c1678383. TL;DR is that the extending-SOP-exceptions concern can probably be ignored for now, though we should add tests to ensure values are adequately validated and preflighted as appropriate.

(SW wouldn't see these headers if we added them at a late point, avoiding the need to have to remove them. OP doesn't mention it, but removing request headers is somewhat unprecedented at the moment, though #609 will change that as well.)

Another concern not mentioned here is that we need to deal with the case where these headers were already set by the developer. If a developer sets DPR a browser would end up appending to it with the current wording. Is that desired? I'd argue it's better to avoid that and only set the header if it's not already set. (We need tests for this too.)

For the concerns mentioned in OP, it sounds like setting the headers early and potentially removing them upon redirects, even if they were developer-set, is the away to go.

Hope this helps.

@arturjanc
Copy link

I agree with @annevk's comment in #800 (comment).

When it comes to the level of badness of sending CH on cross-origin redirects, I'd rather this didn't happen, but I expect it's not a huge risk. The threat model in this case is accidentally disclosing CH information to third parties, and the 1p -> 3p redirect pattern doesn't seem very common, so the leakage potential is low. Still, I expect this to be moot because -- as Ilya said -- Feature Policy should disable the feature when it sees a redirect to a non-safelisted origin (similarly to how CSP rejects resource loads from outside of its safelist upon redirects).

@yoavweiss
Copy link
Collaborator Author

I posted my thoughts here: https://freenode.logbot.info/whatwg/20180824#c1678383. TL;DR is that the extending-SOP-exceptions concern can probably be ignored for now, though we should add tests to ensure values are adequately validated and preflighted as appropriate.

Thanks. Restricting the values SGTM.

Another concern not mentioned here is that we need to deal with the case where these headers were already set by the developer. If a developer sets DPR a browser would end up appending to it with the current wording. Is that desired? I'd argue it's better to avoid that and only set the header if it's not already set. (We need tests for this too.)

That makes sense. I can add that to #773

For the concerns mentioned in OP, it sounds like setting the headers early and potentially removing them upon redirects, even if they were developer-set, is the away to go.

OK, so a tentative design which may work can look something like:

  • Perform cross-origin/Feature-Policy checks at step 1.7 of fetch, before adding any hints
  • Perform cross-origin/Feature-Policy checks again in http redirect fetch and remove any of the hint headers there, if they shouldn't be sent.

That would indeed have the side effect of removing user-added CH headers from cross-origin redirects (unless they were opted-in using Feature-Policy). I think that's an edge case which we can live with.

@annevk - does that make sense?

@annevk
Copy link
Member

annevk commented Aug 27, 2018

I think so, I'd rather have them happen in the same place, but I guess that doesn't work due to navigation and service workers.

@yoavweiss
Copy link
Collaborator Author

Duplicate of #1398, as this is defined in CH infrastructure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan topic: client hints
Development

No branches or pull requests

4 participants