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

Change transitions to not require style-src 'unsafe-inline' CSP. #6662

Closed
Karlinator opened this issue Aug 17, 2021 · 22 comments
Closed

Change transitions to not require style-src 'unsafe-inline' CSP. #6662

Karlinator opened this issue Aug 17, 2021 · 22 comments
Labels
awaiting submitter needs a reproduction, or clarification runtime Changes relating to runtime APIs

Comments

@Karlinator
Copy link
Contributor

Describe the problem

The way CSS transitions are currently handled they inject inline attribute styles into the element. This requires the site use style-src 'unsafe-inline'.

Describe the proposed solution

I'm not entirely sure how this works, but I've seen some inline attribute styles be accepted under style-src 'self' while others are not. Specifically, these spinners seem to work just fine even under a strict CSP.

It appears as though defining CSS variables inline is actually allowed? I haven't seen any documentation about that in particular, though. If true, Svelte transitions could in principle be changed to work like the above, where the transitions are defined in CSS documents, configured by inline variables, and applied by a class change.

This is essentially a cnange only in the internal handling of transitions (and possible animations?), and wouldn't have any effect on how they are used in Svelte code.

Alternatives considered

Don't use transitions if you want a strict CSP.

Make your own transitions by hand.

Importance

would make my life easier

@dummdidumm dummdidumm added the runtime Changes relating to runtime APIs label Aug 17, 2021
@Conduitry Conduitry added the awaiting submitter needs a reproduction, or clarification label Aug 24, 2021
@cor3000
Copy link

cor3000 commented Aug 30, 2021

I had the same experience a while back, luckily creating a custom (fly in/out) transition for my purpose was trivial, thanks to the brilliantly simple but flexible API.
Instead of setting/animating the style property, I used JS api to set element styles directly.

e.g. a simple fly in animation (similar to slide which caused CSP errors):
https://svelte.dev/repl/e177537996964518a97c27dd0bee2d43?version=3.42.4

@Karlinator
Copy link
Contributor Author

I think I actually have a lead on this!

I've traced the error, and it turns out to be a single instance of appending an empty stylesheet to the <head> at the start of the first transition.

const stylesheet = doc.__svelte_stylesheet || (doc.__svelte_stylesheet = append_empty_stylesheet(node).sheet as CSSStyleSheet);

append_empty_stylesheet (via calling append_stylesheet and append) eventually calls target.appendChild(node).

This act of inserting an inline CSS style sheet (empty or otherwise) is disallowed under CSP. As far as I can see, all subsequent operations are performed via the CSSOM which are allowed.

Experimentally replacing append_empty_stylesheet(node).sheet with document.styleSheets[0] (just using the first stylesheet that happened to be loaded by the page) did allow the transitions to work under CSP. This is of course not a stable solution.

Ideally we would modify Svelte to in some way use an existing stylesheet at document load. It is then the responsibility of the application framework to ensure that said stylesheet is loaded in a CSP-friendly way (via a <link> to a whitelisted source, a nonce, etc).

@Karlinator
Copy link
Contributor Author

Something like this:

const stylesheet = doc.__svelte_stylesheet || (doc.__svelte_stylesheet = (document.styleSheets.filter(s => s.title === 'svelte-stylesheet')[0] || append_empty_stylesheet(node)) as CSSStyleSheet);

is a little better. In theory we could use some method (like looking for a styleSheet with a specific title or from an element with a specific id) to use a application/framework-provided (hopefully) CSP-approved stylesheet, and fallback to doing the same as today in order to avoid breaking. The title/id would have to be passed to the runtime somehow? Or use a pre-defined value (like 'svelte-stylesheet')?

@Karlinator
Copy link
Contributor Author

A workaround for this issue: adding 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=' (the hash value for an empty string) to your CSP's style-src allows Svelte to insert the empty document which it can then modify without violating CSP.

This is not in my opinion a very good solution (it's a very weird and specific thing to require), but it does work.

@DrCBeatz
Copy link

A workaround for this issue: adding 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=' (the hash value for an empty string) to your CSP's style-src allows Svelte to insert the empty document which it can then modify without violating CSP.

This is a cool trick, unfortunately it doesn't work in Safari (at least up to version 14.1) and breaks any code using transitions. This may be due to a charset issue in Safari:

Content Security Policy hash not recognized by Safari 11.0.3:
https://stackoverflow.com/questions/48938671/content-security-policy-hash-not-recognized-by-safari-11-0-3#comment110503861_49163882

See this answer in particular:
https://stackoverflow.com/a/49163882

Some were able to solve this by changing the output of the minimizer (Uglify JS) to ascii_only. I don't know how to do this in Rollup/Terser (any suggestions are welcome!).

In any case, I'll have to go with unsafe-inline for style-src for now. If the built-in Svelte transitions can be modified to allow strict CSP in the near future that would be awesome (sorry I can't help with that!).

@DrCBeatz
Copy link

Experimentally replacing append_empty_stylesheet(node).sheet with document.styleSheets[0] (just using the first stylesheet that happened to be loaded by the page) did allow the transitions to work under CSP. This is of course not a stable solution.

A question: since by default the first stylesheet loaded by Svelte should be global.css, (opening a Svelte project/website in a web browser and typing 'document.styleSheets[0]' in the console shows the loaded global.css for me), might using the global.css here (instead of appending a blank stylesheet as is currently done which as you suggest is the problem) actually be the simplest solution? This may likely be what happened in your case and it did work (i.e. the transitions under a strict CSP).

Perhaps there should be some way to verify that it is the correct file. Checking in the console, the title field for the global.css styleSheet object is null (the href field does contain the file url), perhaps at the very least it should be given one when it is loaded?

In any case, since global.css both needs to be loaded by Svelte in order to work at all, and therefore must be allowed under a strict CSP, this should fix the issue with the transitions without having to add any additional CSP directives (assuming the transitions don't actually require any inline styles as you suggest). Unless there is a good reason a blank or additional external stylesheet needs to be loaded? I'm new to all this so please forgive me if I'm missing something :)

@DrCBeatz
Copy link

DrCBeatz commented Sep 19, 2021

Ok, so my initial idea above definitely did not work; appending global.css messes up the styles, seems obvious in hindsight.

However I did find another workaround that suits my needs. I added a 3rd blank style-sheet called 'svelte-style.css' to index.html of an a test Svelte project as follows (after global.css and bundle.css):

<link rel='stylesheet' href='/global.css'>
<link rel='stylesheet' href='/build/bundle.css'>
<link rel='stylesheet' href='/svelte-style.css'>

Then in line 34 of stylemanager.ts I simply replaced append_empty_stylesheet(node).sheet with document.styleSheets[2] (which is of course the 3rd style sheet loaded) and recompiled Svelte and then the test project. For the CSP I was simply able to remove thestyle-src: unsafe-inline directive (observatory.mozilla.org confirms that this works) and keep my transitions in Safari (and Chrome and Firefox).

I might attempt a better implementation of this tomorrow, but at least this hack works for me. Props to Karlinator for finding the cause of this issue!

@DrCBeatz
Copy link

DrCBeatz commented Sep 19, 2021

So I made a working version (see link below, which includes all the changes) that looks for a stylesheet which has the text 'svelte-style.css' (a blank file in the same directory/location as 'global.css') in the href field. If it finds this, then it uses this for the stylesheet object. If not, it uses append_empty_stylesheet(node).sheet as before. In the case of the former, the transitions work under a strict CSP without the script-src unsafe-inline directive, and the order in which the external stylesheets are loaded doesn't matter (this wouldn't work in the previous example). In the latter case, everything works the same as before including requiring script-src unsafe-inline for the transitions to work under a strict CSP.

I might modify this to check that the script is from a tag (i.e. external) as opposed to a <style> tag, and/or to confirm that the 'svelte-styles.css' file is empty as extra precautions. If anyone has any suggestions for a better/more appropriate name for the added stylesheet feel free to let me know. Cheers!

https://github.com/DrCBeatz/svelte/blob/40a0e33d8dbf86ec5615aa66833f82813a89cff1/src/runtime/internal/style_manager.ts#L35-L47

@Karlinator
Copy link
Contributor Author

This is great work! In general the solution being "the framework/app needs to provide an empty stylesheet" can be prone to errors (and is a bit of a hidden dependency), but I don't really think that can be avoided. CSP is a very app-level thing, and there's just no good way to get around it at this level. Grabbing a stylesheet deliberately prepared for the occasion and falling back to the old behaviour is probably the best we can do.

I would caution against restricting the source of the sheet too much I think. I would filter them based on the title (which is passed in by the element's title attribute, so <link rel="stylesheet" title="svelte-stylesheet" href="/svelte-stylesheet.css"/> or <style title="svelte-stylesheet"></style>). There are conditions where you'd want all content to compile into a single html document (and set certain hash or nonce CSP values), in which case you could supply a script tag allowed by your CSP. Those are probably rare situations, but I would leave it to the app level to decide on that if possible.

@DrCBeatz
Copy link

Thanks Karl! I changed it to filter by title (you have to specify the title attribute as 'svelte-stylesheet' in the link/style tag in index.html) and also check whether the css file is empty (using document.styleSheets.cssRules.length; note that I cast each stylesheet as a <CSStyleSheet> instance so Typescript knows the cssRules property is actually there). Otherwise it defaults to append_empty_stylesheet(node).sheet as before. I did some manual tests and everything seems to work as expected. Anything else you can think of to add or address?

https://github.com/DrCBeatz/svelte/blob/0d21b0adcb11a5d19fe37579dcf2d9b2cebda963/src/runtime/internal/style_manager.ts#L35-L48

@Karlinator
Copy link
Contributor Author

I would be really curious if there are actually any cases where that cast to CSSStyleSheet would be problematic. I mean, I guess there is, like, DSSSL and XSL? The MDN docs doesn't seem to mention any case where a StyleSheet isn't also a CSSStyleSheet. I suppose if it did even happen this would throw a cannot read property 'length' of undefined. You could maybe use conditional chaining there? Again, no idea if this would ever come up.

Other than that I think this should be good.

@DrCBeatz
Copy link

Following your suggestion, I removed the cast to <CSSStyleSheet> and evaluated whether the stylesheet is an instanceof CSSStyleSheet. Typescript now knows whether a style sheet is a CSSStyleSheet (in which case it evaluates the cssRules.length), otherwise it moves on to the next sheet. I think this is a good call, since the StyleSheet class was designed to be extendable to non-css style sheets, and using these needn't break our code.

Later this week I'll do more thorough testing/write some tests and see how it goes. Cheers!

https://github.com/DrCBeatz/svelte/blob/2b0ec4fd61709d1cd3b87c190c698c2cd5073b40/src/runtime/internal/style_manager.ts#L35-L52

@DrCBeatz
Copy link

Hmm, the above commit with instanceof CSSStyleSheet instead of casting to <CSSStyleSheet> failed 226 tests when I ran npm run test (without writing any additional tests, initial error message was ReferenceError: CSSStyleSheet is not defined), whereas the commit before with<CSSStyleSheet> passed all tests. I may have been on the wrong track, in any case I've gone back to the cast with added guardrails (.? operator on cssRules property and moving on to next sheet if not present). Typescript/Svelte seem to like the casting, hopefully the guardrails would handle the edge-case of a non-css stylesheet lacking cssRules.

https://github.com/DrCBeatz/svelte/blob/14f744756ffb3075f12fa781550325ae276dc189/src/runtime/internal/style_manager.ts#L35-L52

@Karlinator
Copy link
Contributor Author

The casting happens purely at compile time, instanceof actually happens at runtime. I think the native JS might just not have the CSSStyleSheet name in scope? Or at least in the test runners it doesn't?

I think the guard rails should be sufficient (in fact right now you're double checking the existence of cssRules). If it is a CSSStyleSheet it will have a cssRules with a cssRules.length. If it's a different kind of StyleSheet then css.cssRules should just be null or undefined.

There is also the StyleSheet.type property, which is 'text/css' for a CSSStyleSheet. I think that's how the Web APIs expect you to check this?

@DrCBeatz
Copy link

The casting happens purely at compile time, instanceof actually happens at runtime. I think the native JS might just not have the CSSStyleSheet name in scope? Or at least in the test runners it doesn't?

Interesting, I guess this makes sense since the instanceof version compiled and all the transitions worked in the test project with strict CSP, but even after casting immediately following an instanceof CSSStyleSheet check, it failed big time during npm run test.

I think the guard rails should be sufficient (in fact right now you're double checking the existence of cssRules). If it is a CSSStyleSheet it will have a cssRules with a cssRules.length. If it's a different kind of StyleSheet then css.cssRules should just be null or undefined.

I think it is worth keeping the check for cssRules since it is possible for this property to be null in a CSSStyleSheet in some browsers (e.g. if the stylesheet is from a non-local origin, or because of certain browser extensions).

https://stackoverflow.com/questions/46356349/document-stylesheetsx-cssrules-are-null

There is also the StyleSheet.type property, which is 'text/css' for a CSSStyleSheet. I think that's how the Web APIs expect you to check this?

Good call, I did add a check for this in the latest version. I honestly couldn't find a way to add a style sheet without it being converted to type="text/css" in the browser, but if there is a way for a web browser do this, the code won't blow up. This passes everything in npm run test which also helps. :)

I'm off to write some tests, cheers!

https://github.com/DrCBeatz/svelte/blob/5310005316aa4630fb2065891bc87fba25660348/src/runtime/internal/style_manager.ts#L35-L53

@Karlinator
Copy link
Contributor Author

Just adding a note here:

Re inline styles added by Svelte itself — I'm hoping that we can transition (geddit?) to use the Web Animations API before too long, so that we no longer need to use unsafe-inline.

sveltejs/kit#93 (comment)

@Rich-Harris
Copy link
Member

Came here via sveltejs/kit#5215 — thanks for all the research everyone. Long term the solution is definitely to switch to the Web Animations API, but I wonder if we can't get this to behave in the meantime.

As noted above, apps can specify style-src: 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=', since that's the SHA-256 hash of the empty string, and things will work in Chrome and Firefox. Safari, characteristically, shits the bed. But I think this bug is specific to the empty string. If I modify the append_empty_stylesheet function thusly...

function append_empty_stylesheet(node) {
    const style_element = element('style');
    console.log('appending empty stylesheet');
+    style_element.textContent = '/* empty */';
    append_stylesheet(get_root_for_style(node), style_element);
    return style_element.sheet;
}

...and then use this hash instead...

'sha256-9OlNO0DNEeaVzHL4RZwCLsBHA8WBQ8toBp/4F5XV2nc='

...things work in Chrome, Firefox and Safari.

I don't know if this is a good solution. It definitely feels hacky, and app developers would have to add that hash to their CSP headers (though frameworks like SvelteKit could add it automatically), but maybe it's better than the alternatives?

@DrCBeatz
Copy link

Hi Rich, I tried this out and it works in Safari for Mac/iOS and the other browsers like you said. Adding a hash to a CSP header isn't any trouble, so I think this 1 LOC solution is great until the Web Animations API is implemented. Thanks for this and everything you do. Cheers!

@DrCBeatz
Copy link

@Rich-Harris are you interested in opening a PR for the proposed fix? The instructions on using the hash could be added to the API documentation (maybe under runtime at the end of the transitions section?). All the best!

@DrCBeatz
Copy link

DrCBeatz commented Aug 1, 2022

It appears the proposed fix using the hash no longer works because #7662 removed the append_empty_stylesheet function. Any ideas on a workaround? Thanks!

dummdidumm added a commit that referenced this issue May 4, 2023
…Safari (#7800)

This changes the inserted style element for transitions to initially include the string '/* empty */'. This allows you to work around requiring unsafe-inline CSP discussed in #6662 by adding a hash to your CSP:

'sha256-9OlNO0DNEeaVzHL4RZwCLsBHA8WBQ8toBp/4F5XV2nc='

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@MathiasWP
Copy link
Contributor

Is this something that will be addressed in Svelte 5?

@dummdidumm
Copy link
Member

Fixed in Svelte 5 via using the web animations API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification runtime Changes relating to runtime APIs
Projects
None yet
7 participants