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

fix(cspNonce): don't overwrite existing nonce values #16415

Merged
merged 2 commits into from Apr 18, 2024

Conversation

thebanjomatic
Copy link
Contributor

Fixes: #16414

Description

With this change, we now verify that the nonce attribute doesn't already exist on the tag prior to injecting the nonce value.

Copy link

stackblitz bot commented Apr 12, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@thebanjomatic
Copy link
Contributor Author

@sapphi-red as a side-note, do you remember why <style> tags were removed from this feature: 020c971

It caught me off guard when looking at this that <script> and <link> tags are automatically tagged with a nonce value, but inline <style> tags are not. Is this something the team would be open to adding back in?

@sapphi-red
Copy link
Member

While I was working on #16052, I was thinking hard about what tags should have the nonce attribute injected. The first idea was to only inject tags that cannot be achieved by chaning the input HTML (e.g. <script type="module">). The second idea was to inject it into all tags that CSP allows using nonce (e.g. <script>, <style>). When I did that commit, I decided to go with the first idea. The reason was because I wasn't sure the whole list of tags that needs the nonce attribute to be set and another reason was because I thought it's easy to add it later rather than removing it. But it seems I forgot to skip injecting nonce attributes to script tags without type=module.

On second thought, I think it makes sense to go with the second idea. Vite already injects the nonce to <script>, <link rel="stylesheet">, <link rel="modulepreload">, <link rel="preload">. If I understand the CSP spec correctly, the only missing tag is the <style> tag.

@thebanjomatic
Copy link
Contributor Author

Thanks for confirming. I can add a commit to do that to this PR if you think that would be reasonable.

packages/vite/src/node/plugins/html.ts Outdated Show resolved Hide resolved
playground/csp/vite.config.js Outdated Show resolved Hide resolved
playground/csp/index.html Show resolved Hide resolved
@sapphi-red sapphi-red added feat: html p3-minor-bug An edge case that only affects very specific usage (priority) labels Apr 13, 2024
@sapphi-red
Copy link
Member

Thanks for confirming. I can add a commit to do that to this PR if you think that would be reasonable.

I would appreciate it if you could create another PR that cherry-picks that commit.

@thebanjomatic
Copy link
Contributor Author

Thanks for the review. Feedback has been addressed and I have split the adding of nonce to <style> tags into its own PR

@sapphi-red
Copy link
Member

/ecosystem-ci run

@patak-dev patak-dev merged commit b872635 into vitejs:main Apr 18, 2024
10 checks passed
danroux pushed a commit to danroux/sk8l-ui that referenced this pull request May 8, 2024
## 5.2.11 (2024-05-02)

* feat: improve dynamic import variable failure error message (#16519) ([f8feeea](vitejs/vite@f8feeea)), closes [#16519](vitejs/vite#16519)
* fix: dynamic-import-vars plugin normalize path issue (#16518) ([f71ba5b](vitejs/vite@f71ba5b)), closes [#16518](vitejs/vite#16518)
* fix: scripts and styles were missing from built HTML on Windows (#16421) ([0e93f58](vitejs/vite@0e93f58)), closes [#16421](vitejs/vite#16421)
* fix(deps): update all non-major dependencies (#16488) ([2d50be2](vitejs/vite@2d50be2)), closes [#16488](vitejs/vite#16488)
* fix(deps): update all non-major dependencies (#16549) ([2d6a13b](vitejs/vite@2d6a13b)), closes [#16549](vitejs/vite#16549)
* fix(dev): watch publicDir explicitly to include it outside the root (#16502) ([4d83eb5](vitejs/vite@4d83eb5)), closes [#16502](vitejs/vite#16502)
* fix(preload): skip preload for non-static urls (#16556) ([bb79c9b](vitejs/vite@bb79c9b)), closes [#16556](vitejs/vite#16556)
* fix(ssr): handle class declaration and expression name scoping (#16569) ([c071eb3](vitejs/vite@c071eb3)), closes [#16569](vitejs/vite#16569)
* fix(ssr): handle function expression name scoping (#16563) ([02db947](vitejs/vite@02db947)), closes [#16563](vitejs/vite#16563)

## 5.2.10 (2024-04-20)

* revert: perf: use workspace root for fs cache (#15712) (#16476) ([77e7359](vitejs/vite@77e7359)), closes [#15712](vitejs/vite#15712) [#16476](vitejs/vite#16476)
* fix: add base to virtual html (#16442) ([721f94d](vitejs/vite@721f94d)), closes [#16442](vitejs/vite#16442)
* fix: adjust esm syntax judgment logic (#16436) ([af72eab](vitejs/vite@af72eab)), closes [#16436](vitejs/vite#16436)
* fix: don't add outDirs to watch.ignored if emptyOutDir is false (#16453) ([6a127d6](vitejs/vite@6a127d6)), closes [#16453](vitejs/vite#16453)
* fix(cspNonce): don't overwrite existing nonce values (#16415) ([b872635](vitejs/vite@b872635)), closes [#16415](vitejs/vite#16415)
* feat: show warning if root is in build.outDir (#16454) ([11444dc](vitejs/vite@11444dc)), closes [#16454](vitejs/vite#16454)
* feat: write cspNonce to style tags (#16419) ([8e54bbd](vitejs/vite@8e54bbd)), closes [#16419](vitejs/vite#16419)
* chore(deps): update dependency eslint-plugin-n to v17 (#16381) ([6cccef7](vitejs/vite@6cccef7)), closes [#16381](vitejs/vite#16381)
danroux pushed a commit to danroux/sk8l-ui that referenced this pull request May 8, 2024
## 5.2.11 (2024-05-02)

* feat: improve dynamic import variable failure error message (#16519) ([f8feeea](vitejs/vite@f8feeea)), closes [#16519](vitejs/vite#16519)
* fix: dynamic-import-vars plugin normalize path issue (#16518) ([f71ba5b](vitejs/vite@f71ba5b)), closes [#16518](vitejs/vite#16518)
* fix: scripts and styles were missing from built HTML on Windows (#16421) ([0e93f58](vitejs/vite@0e93f58)), closes [#16421](vitejs/vite#16421)
* fix(deps): update all non-major dependencies (#16488) ([2d50be2](vitejs/vite@2d50be2)), closes [#16488](vitejs/vite#16488)
* fix(deps): update all non-major dependencies (#16549) ([2d6a13b](vitejs/vite@2d6a13b)), closes [#16549](vitejs/vite#16549)
* fix(dev): watch publicDir explicitly to include it outside the root (#16502) ([4d83eb5](vitejs/vite@4d83eb5)), closes [#16502](vitejs/vite#16502)
* fix(preload): skip preload for non-static urls (#16556) ([bb79c9b](vitejs/vite@bb79c9b)), closes [#16556](vitejs/vite#16556)
* fix(ssr): handle class declaration and expression name scoping (#16569) ([c071eb3](vitejs/vite@c071eb3)), closes [#16569](vitejs/vite#16569)
* fix(ssr): handle function expression name scoping (#16563) ([02db947](vitejs/vite@02db947)), closes [#16563](vitejs/vite#16563)

## 5.2.10 (2024-04-20)

* revert: perf: use workspace root for fs cache (#15712) (#16476) ([77e7359](vitejs/vite@77e7359)), closes [#15712](vitejs/vite#15712) [#16476](vitejs/vite#16476)
* fix: add base to virtual html (#16442) ([721f94d](vitejs/vite@721f94d)), closes [#16442](vitejs/vite#16442)
* fix: adjust esm syntax judgment logic (#16436) ([af72eab](vitejs/vite@af72eab)), closes [#16436](vitejs/vite#16436)
* fix: don't add outDirs to watch.ignored if emptyOutDir is false (#16453) ([6a127d6](vitejs/vite@6a127d6)), closes [#16453](vitejs/vite#16453)
* fix(cspNonce): don't overwrite existing nonce values (#16415) ([b872635](vitejs/vite@b872635)), closes [#16415](vitejs/vite#16415)
* feat: show warning if root is in build.outDir (#16454) ([11444dc](vitejs/vite@11444dc)), closes [#16454](vitejs/vite#16454)
* feat: write cspNonce to style tags (#16419) ([8e54bbd](vitejs/vite@8e54bbd)), closes [#16419](vitejs/vite#16419)
* chore(deps): update dependency eslint-plugin-n to v17 (#16381) ([6cccef7](vitejs/vite@6cccef7)), closes [#16381](vitejs/vite#16381)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: html p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

html.cspNonce option injects nonce values onto <script> tags that already contain a nonce
4 participants