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(generator): generate CSP hashes for inline styles and scripts in SPA mode #8022

Closed
wants to merge 3 commits into from

Conversation

BenceSzalai
Copy link

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

It is a modification to generator.js, so that it considers the CSP options and adds the Content-Security-Policy hashes for inline scripts and styles when building an SPA.

When SPA is built the only solution to avoid CSP problems is to explicitly allow all inline scripts and styles (using the unsafe-inline directive). However this is considered bad practice and makes SPAs more vulnerable to XSS attacks.

Resolves: #6592

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

…tyles and scripts in SPA mode & add them to html meta tag, write to the console or save to file
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020

Codecov Report

Merging #8022 into dev will decrease coverage by 1.02%.
The diff coverage is 6.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #8022      +/-   ##
==========================================
- Coverage   68.87%   67.85%   -1.03%     
==========================================
  Files          91       91              
  Lines        3849     3913      +64     
  Branches     1044     1063      +19     
==========================================
+ Hits         2651     2655       +4     
- Misses        971     1013      +42     
- Partials      227      245      +18     
Flag Coverage Δ
#unittests 67.85% <6.25%> (-1.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/generator/src/generator.js 66.53% <6.25%> (-19.69%) ⬇️

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 a7da6fd...89061a8. Read the comment docs.

@@ -217,6 +218,77 @@ export default class Generator {
consola.warn('HTML minification failed for SPA fallback')
}

const { csp } = this.options.render
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is CSP string in response headers of renderRoute returned value, we may use it to generate meta.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked. this.nuxt.server.renderRoute(...) returns an object with only two items:

  • html (string): the rendered HTML
  • preloadFiles Array of Objects, each representing one of the key JS files

No CSP headers are returned.

Afaics renderRoute() calls different renderer for SSR and SPA. While the SSRRenderer's render() method really returns cspScriptSrcHashes, the SPARenderer's render() only returns the html and the files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, csp in response only works in ssr request.

I think a proper way is supporting csp meta tag in spa renderer and also extract csp hash generation and getCspString code.

Copy link
Author

@BenceSzalai BenceSzalai Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my first approach, but it is impossible, since the rendered HTML is minified after being returned here so any hashes calculated during generation will become invalid after minification. That is why I've put the CSP generation code into the generator, so it can act on the minified JS and CSS tags.

We could do the other way around though: move the minification code into the SPA renderer as well. Doesn't make a difference from my perspective, but the key is that the CSP hashes can only be calculated after minification.

@pi0 pi0 requested a review from clarkdo December 1, 2020 12:01
@codecov-io
Copy link

Codecov Report

Merging #8022 (89061a8) into dev (ab039f0) will decrease coverage by 0.21%.
The diff coverage is 6.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #8022      +/-   ##
==========================================
- Coverage   68.06%   67.85%   -0.22%     
==========================================
  Files          91       91              
  Lines        3911     3913       +2     
  Branches     1068     1063       -5     
==========================================
- Hits         2662     2655       -7     
- Misses       1012     1013       +1     
- Partials      237      245       +8     
Flag Coverage Δ
unittests 67.85% <6.25%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/generator/src/generator.js 66.53% <6.25%> (-14.31%) ⬇️
packages/webpack/src/config/client.js 62.50% <0.00%> (-6.95%) ⬇️
packages/cli/src/commands/generate.js 88.23% <0.00%> (-0.34%) ⬇️
packages/cli/src/commands/webpack.js 93.18% <0.00%> (-0.16%) ⬇️
packages/cli/src/run.js 100.00% <0.00%> (ø)
packages/utils/src/task.js 100.00% <0.00%> (ø)
packages/cli/src/utils/serve.js 72.41% <0.00%> (ø)
packages/server/src/listener.js 100.00% <0.00%> (ø)
packages/cli/src/utils/config.js 100.00% <0.00%> (ø)
packages/cli/src/utils/generate.js 0.00% <0.00%> (ø)
... and 16 more

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 ab039f0...f973ceb. Read the comment docs.

@danielroe
Copy link
Member

As Nuxt 2 is currently in maintenance-mode until June 30, 2024, we are not aiming to merge any more features and so, regretfully, I am closing this PR. My apologies that we weren't able to include it in v2.17 🙏

@danielroe danielroe closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with generating CSP policy meta tag for (static) SPAs
7 participants