Move CSS in a separate file to be CSP-compliant #6048

simonbrunel · 2019-02-06T19:00:11Z

In order to be compatible with any CSP, we need to prevent the automatic creation of the DOM 'style' element and offer our CSS as a separate file that can be manually loaded (Chart.js or Chart.min.js). Users can now opt-out the style injection using Chart.platform.disableCSSInjection = true (note that the style sheet is now injected on the first chart creation).

To prevent duplicating and maintaining the same CSS code at different places, move all these rules in platform.dom.css and write a minimal rollup plugin to inject that style as string in platform.dom.js. Additionally, this plugin extract the imported style in ./dist/Chart.js and ./dist/Chart.min.js.

@jelhan @SebastianNiemann can you guys review and test that PR, then confirm it fixes CSP issues?

Closes #5952
Closes #6015
Fixes #5208
Fixes #5295

In order to be compatible with any CSP, we need to prevent the automatic creation of the DOM 'style' element and offer our CSS as a separate file that can be manually loaded (`Chart.js` or `Chart.min.js`). Users can now opt-out the style injection using `Chart.platform.useExternalStylesheet = true` (note that the style sheet is now injected on the first chart creation). To prevent duplicating and maintaining the same CSS code at different places, move all these rules in `platform.dom.css` and write a minimal rollup plugin to inject that style as string in `platform.dom.js`. Additionally, this plugin extract the imported style in `./dist/Chart.js` and `./dist/Chart.min.js`.

sebiniemann · 2019-02-06T22:54:14Z

@simonbrunel

I just build the new Chart.js bundle and added it to my test page. Everything seems to work fine (CSP was set via the <meta http-equiv="Content-Security-Policy"): https://sebastianniemann.github.io/Chart.js-CSP-demo/

Given that I am still neither familiar with the Chart.js coding style (I am used to very strict linting rules doing this job 😅 ) nor which ECMAScript/browser versions should be supported, my review was mostly focused on whether the PR fixes the CSP as well as looking for any obvious oversight.

etimberg

looks OK to me

benmccann

this is great!

docs/getting-started/integration.md

samples/advanced/content-security-policy.html

samples/samples.js

samples/advanced/content-security-policy.html

src/platforms/platform.dom.css

simonbrunel · 2019-02-07T09:26:07Z

Thanks @SebastianNiemann

I am used to very strict linting rules doing this job

Similar rules are setup for that project and automatically checked by our CI

... mostly focused on whether the PR fixes the CSP as well as looking for any obvious oversight.

Exactly what I was expecting, thanks ;)

simonbrunel · 2019-02-07T09:31:20Z

As explained in this comment, I'm not completely happy with the option name: useExternalStylesheet.

What about: disableStylesheetInjection (similar to the initial proposal)? (or shorter disableCSSInjection)

Or if we don't want a negative option, enable(Stylesheet|CSS)Injection (default true)

Better suggestions?

kurkle · 2019-02-07T09:36:31Z

How about just injectCSS (default true)?

sebiniemann · 2019-02-07T11:56:00Z

Something like injectCSS or disableCSSInjection sound good to me. Being used to activate flags (= true) and given that I usually only want to set this option to disable the injection, I would favour disableCSSInjection. However, both ways seem clear to me.

Similar rules are setup for that project and automatically checked by our CI

I didn't meant to depreciate the linter process in any means 😃 , just wanted to explain myself, as I always have to work with a very nitpicking and merciless linter at our company on a daily basis. Therefore, I usually do not to review any non-company code regarding its coding style, to avoid to be too nitpicking myself 😉

docs/getting-started/integration.md

benmccann · 2019-02-07T14:54:14Z

I agree with the renaming. My vote would be for disableCSSInjection since CSS is shorter than Stylesheet. I would name the variable internally as var injectCSS = !platform.disableCSSInjection to avoid the negative in the code, but I think for user options it makes sense to set an option to true when using it, so like disable in the name

simonbrunel · 2019-02-07T16:03:06Z

I also prefer disableCSSInjection since I think a falsy value is a better default.

kurkle · 2019-02-07T19:13:25Z

I don't personally like having enable/disable in a boolean property name, because the value brings that part. But lets ignore that :)

benmccann · 2019-02-07T19:39:50Z

Also a good point. I would be fine with injectCSS as well

sebiniemann · 2019-02-07T19:52:17Z

I would say it the other way around. Because disable/enable/is/has/use is making it very clear that a variable holds a boolean value, it adds to the readability. Being able to guess/know a variable's type correctly just by reading its name seems to be a good thing to me.

simonbrunel · 2019-02-07T20:17:10Z

I would be fine with injectCSS if it was a property of an options/config object but as part of the platform interface, it sounds too much like a method with immediate impact. I would also prefer false as default and I'm not sure what would be the negative/opposite of injectCSS?

What about preventCSSInjection? @kurkle is it better than disableCSSInjection?

benmccann · 2019-02-07T20:28:31Z

Maybe let's stick with disableCSSInjection then. I like it more than preventCSSInjection

kurkle · 2019-02-07T20:29:24Z

To me, disableCSSInjection is better than preventCSSInjection. Prevent does not sound absolute.

kurkle

Only one question remains, but it does not affect my approval.

src/platforms/platform.dom.js

In order to be compatible with any CSP, we need to prevent the automatic creation of the DOM 'style' element and offer our CSS as a separate file that can be manually loaded (`Chart.js` or `Chart.min.js`). Users can now opt-out the style injection using `Chart.platform.disableCSSInjection = true` (note that the style sheet is now injected on the first chart creation). To prevent duplicating and maintaining the same CSS code at different places, move all these rules in `platform.dom.css` and write a minimal rollup plugin to inject that style as string in `platform.dom.js`. Additionally, this plugin extract the imported style in `./dist/Chart.js` and `./dist/Chart.min.js`.

jelhan · 2019-02-12T22:18:06Z

@simonbrunel Sorry for not replying earlier. I was in vacation for last weeks. This is awesome work. Thanks a lot!

timseverien · 2019-02-14T14:54:04Z

Looking good! When can we expect this to be released?

simonbrunel · 2019-02-14T15:05:50Z

@timseverien I would expect it soon (no date planned) but we still need to figure out a few things around the new date adapters before releasing it.

In order to be compatible with any CSP, we need to prevent the automatic creation of the DOM 'style' element and offer our CSS as a separate file that can be manually loaded (`Chart.js` or `Chart.min.js`). Users can now opt-out the style injection using `Chart.platform.disableCSSInjection = true` (note that the style sheet is now injected on the first chart creation). To prevent duplicating and maintaining the same CSS code at different places, move all these rules in `platform.dom.css` and write a minimal rollup plugin to inject that style as string in `platform.dom.js`. Additionally, this plugin extract the imported style in `./dist/Chart.js` and `./dist/Chart.min.js`.

simonbrunel added the type: enhancement label Feb 6, 2019

simonbrunel added this to the Version 2.8 milestone Feb 6, 2019

etimberg previously approved these changes Feb 6, 2019

View reviewed changes

sebiniemann previously approved these changes Feb 6, 2019

View reviewed changes

benmccann previously approved these changes Feb 7, 2019

View reviewed changes

Fix wording and add a note explaining the sample

1a380ee

simonbrunel dismissed stale reviews from benmccann, sebiniemann, and etimberg via 1a380ee February 7, 2019 08:56

sebiniemann reviewed Feb 7, 2019

View reviewed changes

docs/getting-started/integration.md Outdated Show resolved Hide resolved

Rename option name to disableCSSInjection

49f6c8c

benmccann approved these changes Feb 7, 2019

View reviewed changes

etimberg approved these changes Feb 7, 2019

View reviewed changes

kurkle approved these changes Feb 8, 2019

View reviewed changes

src/platforms/platform.dom.js Show resolved Hide resolved

simonbrunel merged commit 96f7344 into chartjs:master Feb 8, 2019

simonbrunel deleted the feat/external-css branch February 8, 2019 17:17

etimberg mentioned this pull request Feb 15, 2019

Chart does not resize inside shadow DOM (e.g. Polymer) #5763

Closed

simonbrunel mentioned this pull request Mar 2, 2019

Optimize the npm package size #6105

Merged

Matir mentioned this pull request Mar 9, 2019

Update Chart.js google/ctfscoreboard#259

Open

simonbrunel mentioned this pull request Apr 2, 2019

NPM missing source #6138

Closed

3 tasks

snyk-bot mentioned this pull request Nov 9, 2019

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.1 socialsoftware/quizzes-tutor#45

Merged

This was referenced Mar 4, 2020

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 ajesse11x/gravity#5

Open

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 qsays/ceph#4

Open

vpetersson mentioned this pull request Mar 7, 2020

[Snyk] Upgrade chart.js from 2.7.3 to 2.9.3 WoTTsecurity/api#764

Merged

This was referenced Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 MichaelKohler/reps-voting-analysis#14

Merged

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

This was referenced Apr 14, 2020

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 mc-ireiser/startbootstrap-sb-admin-2#2

Open

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 mc-ireiser/coreui-free-vue-admin-template#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move CSS in a separate file to be CSP-compliant #6048

Move CSS in a separate file to be CSP-compliant #6048

simonbrunel commented Feb 6, 2019 •

edited

sebiniemann commented Feb 6, 2019 •

edited

etimberg left a comment

benmccann left a comment

simonbrunel commented Feb 7, 2019

simonbrunel commented Feb 7, 2019

kurkle commented Feb 7, 2019

sebiniemann commented Feb 7, 2019 •

edited

benmccann commented Feb 7, 2019

simonbrunel commented Feb 7, 2019

kurkle commented Feb 7, 2019

benmccann commented Feb 7, 2019

sebiniemann commented Feb 7, 2019 •

edited

simonbrunel commented Feb 7, 2019

benmccann commented Feb 7, 2019

kurkle commented Feb 7, 2019

kurkle left a comment

jelhan commented Feb 12, 2019

timseverien commented Feb 14, 2019

simonbrunel commented Feb 14, 2019

Move CSS in a separate file to be CSP-compliant #6048

Move CSS in a separate file to be CSP-compliant #6048

Conversation

simonbrunel commented Feb 6, 2019 • edited

sebiniemann commented Feb 6, 2019 • edited

etimberg left a comment

Choose a reason for hiding this comment

benmccann left a comment

Choose a reason for hiding this comment

simonbrunel commented Feb 7, 2019

simonbrunel commented Feb 7, 2019

kurkle commented Feb 7, 2019

sebiniemann commented Feb 7, 2019 • edited

benmccann commented Feb 7, 2019

simonbrunel commented Feb 7, 2019

kurkle commented Feb 7, 2019

benmccann commented Feb 7, 2019

sebiniemann commented Feb 7, 2019 • edited

simonbrunel commented Feb 7, 2019

benmccann commented Feb 7, 2019

kurkle commented Feb 7, 2019

kurkle left a comment

Choose a reason for hiding this comment

jelhan commented Feb 12, 2019

timseverien commented Feb 14, 2019

simonbrunel commented Feb 14, 2019

simonbrunel commented Feb 6, 2019 •

edited

sebiniemann commented Feb 6, 2019 •

edited

sebiniemann commented Feb 7, 2019 •

edited

sebiniemann commented Feb 7, 2019 •

edited