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

Should not send x-content-security-policy by default #59

Closed
tunetheweb opened this issue Jul 9, 2020 · 5 comments · Fixed by #60
Closed

Should not send x-content-security-policy by default #59

tunetheweb opened this issue Jul 9, 2020 · 5 comments · Fixed by #60

Comments

@tunetheweb
Copy link
Contributor

tunetheweb commented Jul 9, 2020

x-content-security-policy was previously supported by some browsers before content-security-policy was fully supported. It is poorly documented and does not support the full feature-set of the standardised content-security-policy.

IE11 is the only commonly in use browser now supporting this, however it only support the sandbox attribute.

We don't support X-Webkit-CSP which was the other older name used by Safari.

I think it's wrong to have this turned on by default and to use the same CSP as the standardised one. Website owners may not notice it's on by default, may assume it has same support as CSP, and will be less likely to test older browsers to see if it breaks.

I'd suggest removing it from the code completely as the standard CSP header is now well supported and standardised. We could also leave it there but in but with a default off status, but I'd really question the value of this. The alternative would be to be able to specify its setting separately to CSP but again I think it's of little value so I say get rid.

This would technically be a breaking change, in that anyone depending on this header will need to change their config to enable it. However, given its poor support, its complete lack of documentation and, the fact that CSP is used in preference to it anyway on any browser that supports that, I think the risk is low and it's preferable to leaving it in place.

Happy to submit a PR for this but wanted to open an issue for discussion first in case anyone disagreed.

@theacodes
Copy link
Contributor

theacodes commented Jul 9, 2020 via email

@tunetheweb
Copy link
Contributor Author

So happy to remove completely? Or prefer to limit this to turning off by default for now?

@tunetheweb
Copy link
Contributor Author

FYI @Heisendev who I see added option to disable in #38 and #39

@theacodes
Copy link
Contributor

theacodes commented Jul 9, 2020 via email

@tunetheweb
Copy link
Contributor Author

Just to avoid confusion from anyone coming here later, you agreed to remove in the PR itself:

On second though, I'm happy to remove this.

Thanks!

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

Successfully merging a pull request may close this issue.

2 participants