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

feat: add csp hash for color script #94

Merged
merged 3 commits into from
Aug 2, 2021
Merged

feat: add csp hash for color script #94

merged 3 commits into from
Aug 2, 2021

Conversation

clarkdo
Copy link
Contributor

@clarkdo clarkdo commented Jun 14, 2021

Resolve #93

TODO:

  • Test

@clarkdo clarkdo requested review from pi0 and atinux June 14, 2021 11:50
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #94 (2bf1dda) into master (2ca9edc) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #94   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           22        27    +5     
  Branches         2         2           
=========================================
+ Hits            22        27    +5     
Impacted Files Coverage Δ
lib/module.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca9edc...2bf1dda. Read the comment docs.

@atinux atinux assigned atinux and unassigned atinux Jun 15, 2021
Copy link

@dargmuesli dargmuesli left a 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.

lib/module.js Show resolved Hide resolved
@dargmuesli
Copy link

@atinux @clarkdo do you still have this on your radar? Just a friendly question :)

Copy link
Contributor

atinux commented Jul 28, 2021

Is there anything we can help @clarkdo on this?

@clarkdo
Copy link
Contributor Author

clarkdo commented Jul 28, 2021

I’ll add test this week and merge

@clarkdo
Copy link
Contributor Author

clarkdo commented Aug 2, 2021

@atinux Updated

@atinux atinux merged commit e2f1ffc into master Aug 2, 2021
@atinux
Copy link
Contributor

atinux commented Aug 2, 2021

Thanks @clarkdo ❤️

@atinux atinux deleted the csp branch August 2, 2021 14:02
Copy link

@dargmuesli dargmuesli left a 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()

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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 ?

Copy link

@dargmuesli dargmuesli Aug 4, 2021

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

Copy link
Contributor Author

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?

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!

Copy link
Contributor

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 ❤️

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 this pull request may close these issues.

feat: link script in head for CSP 'self' compliance
3 participants