Skip to content

fix: usage with CSP and nonce #8

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

patte
Copy link

@patte patte commented Apr 19, 2025

This PR provides a way make this library work with a content security policy and SvelteKit's way of adding nonces to scripts in app.html.

I tried to make it possible with the least changes to the existing code and invisible to users without CSP.

  • remove useless support to supply nonce
  • param for ThemeProvider to disable head script injection
  • refactor stringify script in dom.ts and head-script.svelte
  • export scriptAsString from dom.ts
  • add csp example
  • describe steps in README

fixes: #7

Copy link

changeset-bot bot commented Apr 19, 2025

⚠️ No Changeset found

Latest commit: 5d7582d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Apr 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-themes-multi-theme-example ❌ Failed (Inspect) Apr 26, 2025 11:44am
svelte-themes-tailwind-example ❌ Failed (Inspect) Apr 26, 2025 11:44am

@patte
Copy link
Author

patte commented Apr 25, 2025

@elliott-with-the-longest-name-on-github could you have a look at this?

@elliott-with-the-longest-name-on-github

Oh shoot, thanks for the tag, and thanks for the PR! I'll do my best to get to this ASAP, but I have three days before vacation and a lot of stuff to get done. But I won't let it die!

@patte
Copy link
Author

patte commented Apr 26, 2025

Thanks for message! No worry and no rush!

@elliott-with-the-longest-name-on-github

I'm struggling a bit with this, actually. First, from a UX perspective, using %sveltekit.nonce% prevents prerendering in your app. At all. That's a big ouch.

Second, this is not a safe use of nonce. I ported it over from the Next version, but that one is unsafe too. Basically, what you're saying is: "Inject this third-party script from NPM into my application, and sign it as secure." If this library were to be compromised, it could then inject signed, trusted scripts into any app using the latest version. (Also, if someday I become a villain, I could thoroughly screw anyone using this library.)

The correct way to do this from the end of the consumer is to create a hash of the script we embed and add that to script-src. So maybe a good way to do that would be to compute the hash and add it to the README, then add CI to make sure that hash never changes without a major version bump. Does that make sense? I'm testing it now to see how it would work...

@elliott-with-the-longest-name-on-github

Ugh yeah, that doesn't work either because of the dynamic script attributes and arguments. Duh. Let me think on this a little more...

@patte
Copy link
Author

patte commented Apr 30, 2025

Thanks for having a look!!

I'm struggling a bit with this, actually.

Glad I'm not the only one.

First, from a UX perspective, using %sveltekit.nonce% prevents prerendering in your app. At all. That's a big ouch.

I wasn't aware of that (as we don't use prerendering), but that makes total sense and is not ideal. But that is also true if CSP is used without the head script, but for the other js code that is bundled, or how does this work?

Second, this is not a safe use of nonce. I ported it over from the Next version, but that one is unsafe too. Basically, what you're saying is: "Inject this third-party script from NPM into my application, and sign it as secure." If this library were to be compromised, it could then inject signed, trusted scripts into any app using the latest version. (Also, if someday I become a villain, I could thoroughly screw anyone using this library.)

You're absolutely right. However, whether your code is inserted via a <script> tag or bundled by Vite/Svelte during build and sent on the first request makes no real difference in terms of security, both approaches treat third-party code as trusted. In fact the later is bundled and also imported in a <script> tag. So even if the headScript would be signed, you could still do whatever you want in e.g. ThemeProvider.

The way I see it, CSP (with script-src: self, nonce-...) protects first and foremost from XSS. And it's also a nice detection for libraries trying to do wired things with eval, but that's not the main point. Or do I not see something important here?

The correct way to do this from the end of the consumer is to create a hash of the script we embed and add that to script-src. So maybe a good way to do that would be to compute the hash and add it to the README, then add CI to make sure that hash never changes without a major version bump. Does that make sense? I'm testing it now to see how it would work...

That could be a solution to the prerendering limitation. We could put up a generator on the demo page that allows users to choose the desired settings and then generate the head script, the hash for it, and the config to use for ThemeProvider. I could help with that.

That said, I think the current implementation in this PR offers the same security as importing this library in the first place.

What do you think would be the cleanest way forward?

@patte
Copy link
Author

patte commented Apr 30, 2025

Reading about it, what you might have been thinking about is Subresource Integrity (SRI)

SRI is a security feature that enables browsers to verify that resources they fetch (for example, from a CDN are delivered without unexpected manipulation.

But that's for including scripts from a CDN, not bundled dependencies like this library here.

And reading about prerendering and CSP with mode: nonce, this is not supported by SvelteKit anyway. And the changes I did are not interfering with using this library without CSP and prerendering, that's still totally possible.

I think the current PR is architectually as convenient as it gets for using CSP with SvelteKit. The security regarding third-party code stays the same (users need to trust you and or review all updates) and overall it's increased due to the use of the CSP which protection against XSS. The way I implemented it (with transformPageChunk) there is no way for unauthorized code to get the nonce and validate itself.

What do you think?

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.

Clarify nonce usage (export scriptBody from head-script.svelte)
2 participants