helmetjs / helmet

Help secure Express apps with various HTTP headers
https://helmetjs.github.io/
MIT License
10.18k stars 367 forks source link

remove block-all-mixed-content from helmet-csp default directives #460

Open NihilumPlays opened 4 months ago

NihilumPlays commented 4 months ago

block-all-mixed-content has been deprecated as detailed here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/block-all-mixed-content and is basically build into the Content-Security-Policy

while its not doing anything as chrome is ignoring the directive it just bothers me slightly that its there

EvanHahn commented 4 months ago

I keep this directive around for the legacy browsers that respect it. Do you think it should be removed despite that?

NihilumPlays commented 4 months ago

Ah I guess I didnt think of legacy scenarios

How about adding a way of disabling it instead?

I've tried with [], null, '' and it either results in an error or it gets put there anyway

EvanHahn commented 4 months ago

null should disable the directive. What does your code look like?

NihilumPlays commented 4 months ago

You are absolutely correct. Maybe I have misspelled it the first time I tried or had an error intersect while I was implementing it. Adding null removes it correctly.

Regarding if it should be removed or not I had a few after thoughts:

a) If it is to be kept it should be in the default directives in helmet so it matches helmet-csp to avoid any confusion, along with documentation for why it is there so developers can decide if they want to remove it or not, giving them the option to save a few additional bytes each request if they don't care about legacy browsers (which is exactly what I'm doing. Removing to save the bytes, since my website wouldn't work with a legacy browser anyway)

b) The standard today is https and if you're fiddling with content security policy chances are you have HSTS enabled as well, which means you will only be vulnerable the first visit to the site anyway, and not at all if preload has been set.

c) The policy that replaces block-all-mixed-content, upgrade-insecure-requests, was implemented in all major browser in around 2015-2018 depending on the developer, so the amount of vulnerable browsers shouldn't be that high. Combined with HSTS settings only edge cases should remain.

It is up to you to do keep it or not. If I can disable it it doesn't matter to me wether it is there or not.

If I read some of your stack overflow replies correctly I understand you want helmet to be as easy to setup and use as possible, so in the end it is a question of would you be removing it to save bandwidth more often than not

EvanHahn commented 4 months ago

This is helpful context. I'll think about it and consider removing it in the next major version.

webketje commented 4 months ago

👍 for removing from defaults (for both helmet & helmet-csp) and eventually adding a note to add legacy "block-all-mixed-content" directive