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: injecting style element violates CSP #5946

Closed
wants to merge 1 commit into from

Conversation

jelhan
Copy link

@jelhan jelhan commented Dec 31, 2018

Using a link element is also recommended approach linked StackOverflow
question. Code is mostly a copy and paste from this answer.

Fixes #5208 together with #5909

// https://stackoverflow.com/q/3922139
var style = platform._style || document.createElement('style');
if (!platform._style) {
Copy link
Author

Choose a reason for hiding this comment

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

injectCSS() is only called once so we don't have to care about caching created element.

Copy link
Member

@etimberg etimberg Dec 31, 2018

Choose a reason for hiding this comment

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

The reason this cached is for the case of multiple charts on a single page. We only want 1 style element then

Copy link
Author

Choose a reason for hiding this comment

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

But the styles should be injected multiple times? Please note that style.appendChild(document.createTextNode(css)); wasn't part of that if.

Copy link
Member

Choose a reason for hiding this comment

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

No, the style in platform.initialize() shouldn't be injected multiple times, but injectCSS() could be called multiple time to inject different style under the same style element. That's not the case currently but that could change so injectCSS() still need to be able to be called multiple times (which I guess is still the case).

Copy link
Author

Choose a reason for hiding this comment

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

It's still possible to call injectCSS() multiple times but that will cause multiple link elements to be added. I didn't benchmark but I guess this is better from a performance perspective cause replacing may cause a more expensive rerendering.

@simonbrunel
Copy link
Member

Tests are failing. Can you also share a live example that shows the CSP issue resolved?

Using a link element is also recommended approach linked StackOverflow
question. Code is mostly a copy and paste from this answer.

Fixes chartjs#5208 together with chartjs#5909
@jelhan jelhan force-pushed the issue-5208-csp-violation branch from 1356a1f to 90f55cc Compare December 31, 2018 17:34
@jelhan
Copy link
Author

jelhan commented Dec 31, 2018

Tests are failing.

Missed to append the link element to document. 😲 Fixed that one. Tests are passing now.

Can you also share a live example that shows the CSP issue resolved?

I'm not quite sure if setting CSP is possible in JS Fiddle and similar services. Will investigate that one next year. 😆

@jelhan
Copy link
Author

jelhan commented Jan 2, 2019

This fix still violates CSP:

Refused to load the stylesheet 'data:text/css;charset=UTF-8,%40-webkit-keyframes%20chartjs-render-animation%7Bfrom%7Bopacity%3A0.99%7Dto%7Bopacity%3A1%7D%7D%40keyframes%20chartjs-render-animation%7Bfrom%7Bopacity%3A0.99%7Dto%7Bopacity%3A1%7D%7D.chartjs-render-monitor%7B-webkit-animation%3Achartjs-render-animation%200.001s%3Banimation%3Achartjs-render-animation%200.001s%3B%7D' because it violates the following Content Security Policy directive: "style-src 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=' 'sha256-OTeu7NEHDo6qutIWo0F2TmYrDhsKWCzrUgGoxxHGJ8o='". Note that 'style-src-elem' was not explicitly set, so 'style-src' is used as a fallback.

Here is a live example: https://codepen.io/anon/pen/EGoggR

I'm sorry for the noise. Will dig deeper in this topic and try to come up with another solution.

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.

3 participants