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

Improve hint-no-disallowed-headers #1096

Closed
alrra opened this issue May 23, 2018 · 3 comments · Fixed by #3776
Closed

Improve hint-no-disallowed-headers #1096

alrra opened this issue May 23, 2018 · 3 comments · Fixed by #3776

Comments

@alrra
Copy link
Contributor

alrra commented May 23, 2018

Look over: https://www.fastly.com/blog/headers-we-dont-want, and see if we can add more unneeded headers to the list.

The code for this hint is under packages/hint-no-disallowed-headers/

@alrra alrra self-assigned this May 24, 2018
@alrra alrra removed their assignment Jan 14, 2019
@molant molant changed the title Improve rule-no-disallowed-headers Improve hint-no-disallowed-headers Oct 10, 2019
@captainbrosset
Copy link
Contributor

captainbrosset commented May 22, 2020

After reading that article and looking at the code from this hint, I think we could safely add the following headers into the list of disallowed headers:

  • Expires
  • Host
  • P3P
  • Pragma
  • Via
  • X-Frame-Options
  • X-UA-Compatible

If I understand correctly, the hint also supports configuring ignored headers, so even if, by some chance, someone was using those outdated headers, they would be able to ignore the warnings.

The only thing I'm wondering about is: it's easy enough to add them to the disallowedHeaders array, but since the article goes to great length explaining what these headers are and why they're not necessary, I'm wondering if instead they should each have some specific logic with corresponding localized strings, so users can learn why they're getting warnings.

What do you think @antross ?

@antross
Copy link
Member

antross commented May 22, 2020

@captainbrosset having more detailed messages on a per-header basis would be great! We can keep the general message for any headers we don't have something more to say about (or additional ones included via the webhint configuration that aren't part of the default set).

Also, there are a couple of related issues in this space:

antross pushed a commit that referenced this issue May 28, 2020
This change adds the `Expires`, `Host`, `P3P`, `Pragma`, `Via` and
`X-Frame-Options` headers to the `hint-no-disallowed-headers` hint.
This was based on advices from
https://www.fastly.com/blog/headers-we-dont-want.

These hints are introduced with specific warning messages, rather than
by being added to the general list of headers already present in the
hint.

Specifically, this change moves some of the logic from the constructor
to other places to make it smaller and easier to read.
It renames the `disallowedHeaders` property to something more
explanatory and introduces a new special list of disallowed headers,
each with their own warning message.

- - - - - - - - - - - - - - - - - - - -

Fix #1096
Close #3776
@KittyGiraudel
Copy link

Taking the liberty to comment on this issue, especially regarding the Via HTTP header. More often that not, it appears that this header is added by proxies, which makes it difficult or impossible to fix. For instance, Google Cloud Load Balancing adds Via: 1.1 google to incoming requests and responses. Any CDN sitting behind GCLB, such as Sanity’s, will display this header.

The article linked in the original message from this issue also mentions this. Although it does say it’s most likely safe to remove (which I’m not arguing here), it also states that proxies have to set it up, and most people don’t have control over their proxy.

More debatable perhaps is Via, which is required (by RFC7230) to be added to the request by any proxy through which it passes to identify the proxy. This can be the proxy’s hostname, but is more likely to be a generic identifier like “vegur”, “varnish”, or “squid”. Removing (or not setting) this header on a request can cause proxy forwarding loops. However, interestingly it is also copied into the response on the way back to the browser, and here it's just informational and no browsers do anything with it, so it’s reasonably safe to get rid of it if you want to.

I was wondering if you’d be considering dropping the Via warning.

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

Successfully merging a pull request may close this issue.

5 participants