feat: propagate contentSecurityPolicy.useDefaults through to helmet
#184
Conversation
| @@ -262,7 +320,6 @@ test('It should access the correct options property', async (t) => { | |||
| enableCSPNonces: true, | |||
| contentSecurityPolicy: { | |||
| directives: { | |||
| ...helmet.contentSecurityPolicy.getDefaultDirectives(), | |||
There was a problem hiding this comment.
With the exception of the test on L135 that explicitly tests this getDefaultDirectives() export, it seemed more appropriate in these tests to NOT explicitly provide these headers and instead rely on helmet's default behavior of injecting these default directives itself.
| @@ -280,7 +337,7 @@ test('It should access the correct options property', async (t) => { | |||
| t.ok(cspCache.script) | |||
| t.ok(cspCache.style) | |||
| t.has(response.headers, { | |||
| 'content-security-policy': `default-src 'self';base-uri 'self';block-all-mixed-content;font-src 'self' https: data:;form-action 'self';frame-ancestors 'self';img-src 'self' data:;object-src 'none';script-src 'self' 'unsafe-eval' 'unsafe-inline' 'nonce-${cspCache.script}';script-src-attr 'none';style-src 'self' 'unsafe-inline' 'nonce-${cspCache.style}';upgrade-insecure-requests` | |||
There was a problem hiding this comment.
The order of these directives changed now that helmet's default directives are injected after those explicity provided on L266-L267, but the CSP content is otherwise identical.
| @@ -85,7 +85,6 @@ test('It should add CSPNonce decorator and hooks when route `enableCSPNonces` op | |||
| enableCSPNonces: false, | |||
| contentSecurityPolicy: { | |||
| directives: { | |||
| ...helmet.contentSecurityPolicy.getDefaultDirectives(), | |||
There was a problem hiding this comment.
Similar explanation for this removal as in global.test.js on L265.
|
@mcollina - if/when the team merges this PR, would you consider making a branch on the latest 8.x release that I could use to target a backport of this change for |
https://github.com/fastify/fastify-helmet/tree/v8.x Here you are. |
|
I will review this afternoon 👍 |
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
There was a problem hiding this comment.
With this change we will push an opinionated change (that may break existing code). < Not meant to be published for this PR nor this public account.
Can you add a note in the readme stating that while in helmet it is true by default when useDefaults is set to false it will disable the behavior. < right sentence.
LGTM with this README addition 👍
You've lost me there @darkgl0w, it's still enabled by default isn't it, so not a breaking change? |
OMG 🤣 reviewed from my phone while I was on train and I mixed/misplaced my message editions while writing the comment @Fdawgs that was not the sentence I wanted to validate (I will edit the original message to avoid confusions xD). Edit: And I have a piece of review that was not aimed at this PR nor this account too ... ~~ |
@darkgl0w - are you asking for the above in addition to the blurb below that's already been added to the README in this PR?
If so, would you mind adding a suggestion of the specific changes you'd like to see in the README? |
|
@chrskrchr > no the section you already added to the readme is good :) |
The
@fastify/helmetplugin was bumped tohelmet@5in #164. This release ofhelmetincluded a breaking change whereuseDefaultnow defaults totrue, causing this plugin to now include a default set of directives on the response. We'd like to passuseDefault: falseto@fastify/helmetand have that propgated through tohelmetso we can disable this behavior.https://github.com/helmetjs/helmet/tree/main/middlewares/content-security-policy
Checklist
npm run testandnpm run benchmarkand the Code of conduct