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

[Hint] CORS Vary issues #1635

Open
molant opened this issue Sep 6, 2018 · 6 comments
Open

[Hint] CORS Vary issues #1635

molant opened this issue Sep 6, 2018 · 6 comments

Comments

@molant
Copy link
Member

molant commented Sep 6, 2018

Originally reported by @antross in #1205

Not yet sure how to best test for this, but @ericlaw1979 recently wrote about the issue here: https://textslashplain.com/2018/08/02/cors-and-vary/

This can lead to CORS requests that intermittently fail in production based on the state of a user’s cache.

@Malvoz
Copy link
Member

Malvoz commented Dec 19, 2018

The Vary header is something that authors tend to get wrong, forget or misconfigure. In my opinion, this hint should cover the Vary header in general.

Some checks that I can think of:

  • Avoid Vary: *. For the reason described below.

  • Q: Consider avoiding Vary: User-Agent|Cookie|Referer|Host|Alt-Svc. Although some of these have very valid use-cases, e.g. not setting Vary: Cookie may lead to information leakage, however setting these pretty much negates caching (similar to Vary: *) in intermediary HTTP caches?

  • Q: Safe to assume authors should only send Vary for text/html resources? No, this was a bad assumption.

  • Q: Add a note about including custom HTTP header(s) in Vary if the response may be affected by it?


A few relevant resources:

https://tools.ietf.org/html/rfc7231#section-7.1.4
https://www.smashingmagazine.com/2017/11/understanding-vary-header/
https://www.fastly.com/blog/best-practices-using-vary-header
https://www.ctrl.blog/entry/http-vary-explained
https://www.keycdn.com/support/vary-header

@molant
Copy link
Member Author

molant commented Dec 23, 2018

Great ideas @Malvoz!

In my opinion, this hint should cover the Vary header in general.

Looks like you are proposing quite a few things. I tend to prefer to make the hints as simple as possible but maybe for this one it makes sense to have a single one. @webhintio/contributors what do you think? An alternative could be to have a multi-hint package for all Vary things.

Although some of these have very valid use-cases [...]

If there are valid cases, we should probably allow a custom configuration. Have you thought how this could look like? If we decide to go with the multi-rule option we could make vary/avoid-values (or whatever the name we decide) have a list of approved/forbidden values.

Q: Safe to assume authors should only send Vary for text/html resources?

I think it can be used for any text based resource (e.g.: CSS and JS).hint

Also @Mavoz would you like to work on this hint with our help?

@molant molant transferred this issue from webhintio/rfcs Jan 8, 2019
@Malvoz
Copy link
Member

Malvoz commented Feb 19, 2019

If there are valid cases, we should probably allow a custom configuration. Have you thought how this could look like?

I guess anything is valid per se, it boils down to how large of a cache key surface is wanted/expected by the developer. As with the Vary: Cookie example, I think it'd be unwise of webhint to say "Do not Vary: Cookie" just because it may negate caching (to whatever extent) - because not doing so (I suspect majority of devs don't) may lead to info-leakage. So a warning in this case, would be good enough to create awareness and ultimately it is up to the developer to decide what to compromise.

we could make vary/avoid-values (or whatever the name we decide) have a list of approved/forbidden values.

That'd be good, some straight forwards ones to warn about:

@Malvoz
Copy link
Member

Malvoz commented Feb 19, 2019

To further showcase the ambiguity of the Vary header: 🙆‍♂️

cdn2

@molant
Copy link
Member Author

molant commented Feb 19, 2019

Interesting, we can wait and see what KeyCDN has to say about Vary.

Thanks!

@Malvoz
Copy link
Member

Malvoz commented Feb 19, 2019

They updated the article to now include the Vary header:
https://www.keycdn.com/blog/client-hints#client-hints-and-the-vary-header

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

No branches or pull requests

2 participants