-
Notifications
You must be signed in to change notification settings - Fork 112
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
feat: add csp hash for color script #94
Conversation
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 22 27 +5
Branches 2 2
=========================================
+ Hits 22 27 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the import, this change works for me.
Is there anything we can help @clarkdo on this? |
I’ll add test this week and merge |
@atinux Updated |
Thanks @clarkdo ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I open a new issue about this or do I just have to change my configuration somewhere?
expect(body).toContain('nuxt-color-mode-script') | ||
expect(headers['content-security-policy']).toBeUndefined() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it expected that there is no CSP header when the target is server
? Does vue-renderer
set the CSP itself then? I'm using the server target and SSR and within development everything works fine, but when I deploy my project to production, the CSP hash seems to be missing from script-src
again and the color-mode cannot be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is default scenario which build.csp is not enabled in nuxt.config.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I deploy my project to production, the CSP hash seems to be missing from script-src again and the color-mode cannot be changed.
CSP hash should work same in dev and production, can you double check the nuxt.config and http response header of your page ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the header only includes the always changing hash for nuxt state on production (and development), but sha256-SzzThFtAnNrq8hsItwHFrLjbWhI0pfrqgYInXTrgWRI=
is only added on development. Currently, you can see it here yourself: https://alpha.maev.si/ which is running version 0.93.1 of https://github.com/maevsi/maevsi/. In the footer there are links to change the color mode, but the console shows a CSP error. Not so when starting the project using yarn dev
. During yarn build
L57 is ran across, but when vue-renderer
calls vue-renderer:ssr:csp
L58-L61 is not run. Here you can see my projet's csp configuration: https://github.com/maevsi/maevsi/blob/1c19ad303f8ce275e67dbe204adcfd1bc0ab66c9/nuxt/nuxt.config.js#L373
Btw, why is this plugin not registered by using addPlugin
like here? https://github.com/nuxt-community/ackee-module/blob/fa6ac7d45dddbca7be5fea47294b9e5cb2206568/src/module.ts#L33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is because you’re using this module as buildModule which is only for dev and build instead of production running. @atinux Should we recommend using this module as a normal module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, and indeed that solved the issue!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed adding a note in the README when using CSP could be nice. PR welcome ❤️
Resolve #93
TODO: