Serving the CSS definitions in a CSP-compliant manner, while remaining compatible with v2.0 #6015

sebiniemann · 2019-01-25T20:29:05Z

Implements the solution proposed in #5208 (comment)

Introduces a simple switch to deactiviate the CSS injection in /src/platforms/platform.dom.js
Provides the otherwise injected CSS definition via chartjs.css

TODO:

Add user documentation on how to use the opt-in process:
- Set Chart.platform.useSecureCSS = true;
- Add <style src="chartjs/chartjs.css"></style>
Add some css linting to the gulp process
Generate a minified css file during the build process

Regarding the todos, I would suggest to merge this MR first (in case it is accepted), together with an explanation within the release notes, to quickly address the security concerns for fast adopters/peers in need of this. Afterwards, it could then be documented on https://chartjs.org and improved by adding a linting process and a minified version.

sebiniemann · 2019-01-25T20:31:27Z

@simonbrunel I hope I saw everything involved with the build process. But please have another look, as I am not familiar with yours 😃

src/platforms/platform.dom.js

src/platforms/platform.js

simonbrunel · 2019-01-26T08:35:21Z

I think I like this approach more than all other suggestions trying to "workaround" CSP issues, though I don't think I would promote it as the recommended one in the next major release since it makes the library integration a bit more complex. Users who don't care about CSP shouldn't care about linking against the extra (almost empty) CSS. Anyway, that's another topic we will have time to figure out and maybe we will find a way to get rid of this stylesheet in v3 instead.

About the TODO, I agree that the two last points can be done separately but I would like the first one (documentation) part of this PR once we all agree about the public API.

However, I'm quite sure that it doesn't solve the CSP issues, so in order to review it, can you build a minimal test case that links against Chart.js and Chart.css, with Chart.platform.useSecureCSS: true?

sebiniemann · 2019-01-26T08:47:59Z

@benmccann Thanks for the Review 👍 , added all of your changes.

@simonbrunel Sounds good. Could you list the parts about the public API that need to be agreed on? This way, I could already prepare some documentation.

I can also understand your scepticism regarding whether or not this solves the CSP issue, after a few failed attempts. To clearly demonstrate that it works with only setting style-src: self (so neither unsafe-inline or unsafe-eval is required), I will gladly provide a demo.

However, I am not really sure how a unit test (maybe with puppeteer? Will it check CSP in headless mode?) would look like, so is a simple Github.io webpage fine? I could do this a lot quicker 😅

simonbrunel · 2019-01-26T09:13:24Z

Could you list the parts about the public API that need to be agreed on?

Chart.platform.useSecureCSS seems the only public API. I think I prefer @benmccann suggestion here Chart.platform.useExternalStylesheet since it makes the whole feature explicit. Also, I would prefer a boolean than a function (you can define a getter / setter instead). I'm working on refactoring the style to inject a single stylesheet instead of using cssText in createDiv, so we will not need the global useExternalStylesheet variable anymore.

I can also understand your scepticism regarding whether or not this solves the CSP issue...

It's not skepticism but I don't think your PR fixes the remaining CSP issue but only anticipates CSP implementation of cssText. I'm working on deferring the injection of the keyframe / animation style, which will be required in order to be able to prevent any CSP issues.

Will it check CSP in headless mode?

No, unit tests should simply make sure that the chart still work against the external style sheet. Let's not make the test process more complicated by trying to check CSP issues.

sebiniemann · 2019-01-26T09:21:16Z

It's not skepticism but I don't think your PR fixes the remaining CSP issue but only anticipates CSP implementation of cssText.

Ah, I see. I also just stumbled over the injectCSS function, which I do not saw before. This should also be excluded by my last commit.

While searching for other functions adding style elements or setting cssText, I did not find another line. Now all such cases should be covered.

Also, I would prefer a boolean than a function (you can define a getter / setter instead)

Ok, I will add this in a moment. The demo page is also on its way.

sebiniemann · 2019-01-26T10:07:54Z

@simonbrunel While building the demo page, I think I understood what you meant. If I got it correctly, Chart.js is already injecting the CSS the moment the library is loaded and not just before a Chart needs to be rendered, if missing.

Before, I assumed the later case, that the library is not affecting the DOM without explicitly calling its API.

This way,

Importing Chart.js
Configuring useExternalStylesheet
Calling new Chart

would have worked fine. But it seems as of now, it requires

Configuring useExternalStylesheet
Importing Chart.js
Calling new Chart

To prevent a system wide global variable, this (at least for the css injection part) needs to be changed. Am I correct in assuming that this is what you are refactoring or want to refactor?

Users who don't care about CSP shouldn't care about linking against the extra (almost empty) CSS. Anyway, that's another topic we will have time to figure out and maybe we will find a way to get rid of this stylesheet in v3 instead.

Just my personal thought, I would like to add to this discussion (feel free to ignore it 😉, I am happy either way, because Chart.js is doing an awesome job 👍 and it's just my opinion): I think this whole security and implementation issue arose because of the non-standard static CSS-injection via JS. In case we cannot get rid of the CSS information, I would suggest to provide it as a file, which is

very common/standard for UI libraries (takes no time to add another file to get started, compared with using a library, i.e. setting up the Charts and styling everything to match personal taste -- not meaning that Chart.js is complicated)
completely resolves this security issue
reduces Chart.js' code complexity, as the whole self-implemented JS code to inject the CSS can be dropped
easy to implement (moving the CSS to a single file), so you could focus on more important things

kurkle

I'm not sure about the API, shouldn't it be specific to DOM?
I also feel useExternalStylesheet is better name for the export.

kurkle · 2019-01-26T10:08:34Z

src/platforms/platform.dom.js

-		style.setAttribute('type', 'text/css');
-		document.getElementsByTagName('head')[0].appendChild(style);
-	}
+	if (!useExternalStylesheet) {


I'd move this check to initialize, where injectCSS is called.

Should I also do this for createDiv and its invocations?

kurkle · 2019-01-26T10:37:34Z

src/platforms/platform.dom.js

@@ -325,6 +331,13 @@ module.exports = {
 	 */
 	_enabled: typeof window !== 'undefined' && typeof document !== 'undefined',

+	get useSecureCSS() {


I would use Object.defineProperty instead, to be consistent with other parts of library.
For example: core.animation.js

(might want to wait for other comments, before changing)

Based on https://caniuse.com/#search=ECMAScript%205, Getter and Setter (introduced by ECMAScript 5), are supported at least by IE 10+, Edge, Firefox, Chrome, Safari, Opera (and their mobile variants).

Which ECMAScript version/browser should it support (so I know this for other changes 😃 )? I can also adjust it just for the sake of doing it in a consistent way.

sebiniemann · 2019-01-26T10:50:15Z

@kurkle

I'm not sure about the API, shouldn't it be specific to DOM?

Do you mean the line in src/platforms/platform.js? I thought this might be required for TypeScript folks, ensuring that the API exists in any environment, so other users don't have to make sure of it.

simonbrunel · 2019-01-26T12:04:26Z

Am I correct in assuming that this is what you are refactoring or want to refactor?

Yes, you got it correctly. I started this work when exploring how to fix CSP issues but have been busy on other tasks since. I should be able to submit a PR next week with these changes (I'm currently traveling). The idea is that all the CSS rules will be injected when needed (i.e. when creating the first resize listener) which will impact when the injectCSS is called but also will remove the cssText form createDiv.

I'm also investigating how to store the CSS rules in a separate file (platform.dom.css) and import them at build time JavaScript side. If we are going to offer a side CSS (Chart.js), it will be really helpful to not have to maintain it in 2 places (platform.dom.js and platform.dom.css).

I think my upcoming PR will invalidate most of these changes. If that makes sense, I can integrate the remaining work in my branch to avoid more iterations here (moving the .css file, adjusting gulpfile.js, etc.)?

sebiniemann · 2019-01-26T13:32:10Z

I think my upcoming PR will invalidate most of these changes. If that makes sense, I can integrate the remaining work in my branch to avoid more iterations here (moving the .css file, adjusting gulpfile.js, etc.)?

Sounds good to me. If It makes sense, I will gladly help to finish open work in your branch / PR. However, I don't want to steal you further time to explain how everything is planned to work to me, if it will already be finished in a few days by your own.

I'm also investigating how to store the CSS rules in a separate file (platform.dom.css) and import them at build time JavaScript side.

A simple approach (assuming the build process runs in a NodeJS environment) could be to either inject the CSS information directly into the JS file at build time

const fs = require('fs');
fs.appendFileSync('src/platforms/platform.dom.js', `
  const CSS = '${fs.readFileSync('src/platforms/platform.dom.css').replace(\'\g, '\'')}'
`);

or to prepare an empty dummy file (platform.dom.css.js), which is required by platform.dom.js and filled at build time:

const fs = require('fs');
fs.writeFileSync('src/platforms/platform.dom.css.js', `
module.exports = {
    CSS: '${fs.readFileSync('src/platforms/platform.dom.css').replace(\'\g, '\'')}',
}`);

Personally, I would prefer the later one, as it remains compatible with the linter (as it requires the constant CSS variable directly in platform.dom.js) instead of introducing it as preexisting.

Beware, I did not tested this explicit code (might have some syntax errors), but I believe you got the idea. This is also how similar problems are solves in https://github.com/vuejs/vuepress, where they need to create the router component based on which files are found at build time.

simonbrunel · 2019-01-26T13:48:45Z

I didn't test yet but I think this rollup plugin can do the job (with minification support built-in), so would be:

var style = require('./platform.dom.css');

// ...

injectCSS(style);

simonbrunel · 2019-01-26T13:57:23Z

I will gladly help to finish open work in your branch / PR.

Thanks, I will ping you as soon as I create the PR. In the meantime, if you feel to write the documentation about CSP issues and how to use the external CSS file, you can update that PR then I will grab it from here. I'm not sure where it should reside, maybe in "integration"?

sebiniemann · 2019-01-26T15:30:38Z

In the meantime, if you feel to write the documentation about CSP issues and how to use the external CSS file

Sure, I will add some documentation over the weekend. I also feel that "integration" could be a good place for this.

benmccann · 2019-01-26T23:15:31Z

docs/getting-started/integration.md

@@ -34,3 +34,23 @@ require(['path/to/chartjs/dist/Chart.js'], function(Chart){
 ```

 > **Important:** RequireJS [can **not** load CommonJS module as is](http://www.requirejs.org/docs/commonjs.html#intro), so be sure to require one of the built UMD files instead (i.e. `dist/Chart.js`, `dist/Chart.min.js`, etc.).
+
+## Content Security Policy (CSP)


It might make sense to add an extra # in the four headers above this. They're all about including the JS and this one is about including the CSS, so it may make sense to differentiate somehow

sebastianniemann added 6 commits January 25, 2019 20:44

Opt-in switch to serve the CSS definitions in a CSP-compliant manner

99d8466

Update chart.js

1609d13

Update platform.dom.js

60a0738

Update .travis.yml

47ef690

Update gulpfile.js

bd2c86a

Create chart.css

cf41353

sebiniemann changed the title ~~Serving the CSS definitions in a CSP-compliant manner (as opt-in), while remaining 2.0 compliant~~ Serving the CSS definitions in a CSP-compliant manner (as opt-in), while remaining compatible with v2.0 Jan 25, 2019

sebastianniemann added 2 commits January 25, 2019 21:39

Update platform.dom.js

f94cc36

Rename chart.css to chartjs.css

d7a3577

sebiniemann changed the title ~~Serving the CSS definitions in a CSP-compliant manner (as opt-in), while remaining compatible with v2.0~~ Serving the CSS definitions in a CSP-compliant manner, while remaining compatible with v2.0 Jan 25, 2019

sebastianniemann added 3 commits January 25, 2019 22:27

Update platform.js

7bbc01e

Update platform.dom.js

9244b57

Update platform.js

248d837

benmccann reviewed Jan 26, 2019

View reviewed changes

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

benmccann reviewed Jan 26, 2019

View reviewed changes

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

sebastianniemann added 2 commits January 26, 2019 09:34

Update platform.js

c909a35

Update platform.dom.js

12a7bfe

Update platform.dom.js

3d15927

sebastianniemann added 6 commits January 26, 2019 10:24

Update platform.dom.js

83614b4

Using getter/setter

6427ea3

Tab > space

e9f84cb

Update chartjs.css

73b8124

Fixed typo

5cd9e53

Spaces -> Tabs

5c1f916

kurkle requested changes Jan 26, 2019

View reviewed changes

sebastianniemann added 5 commits January 26, 2019 20:18

Update integration.md

9df71d0

Update integration.md

beebd2a

Update integration.md

9625c33

Update integration.md

4de0fb9

Update integration.md

5cbe85c

benmccann reviewed Jan 26, 2019

View reviewed changes

simonbrunel mentioned this pull request Jan 29, 2019

How to keep both extracted CSS and inline CSS? egoist/rollup-plugin-postcss#120

Open

simonbrunel mentioned this pull request Feb 6, 2019

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

Merged

simonbrunel closed this in #6048 Feb 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serving the CSS definitions in a CSP-compliant manner, while remaining compatible with v2.0 #6015

Serving the CSS definitions in a CSP-compliant manner, while remaining compatible with v2.0 #6015

sebiniemann commented Jan 25, 2019 •

edited

Loading

sebiniemann commented Jan 25, 2019

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019 •

edited

Loading

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019 •

edited

Loading

sebiniemann commented Jan 26, 2019 •

edited

Loading

kurkle left a comment

kurkle Jan 26, 2019

sebiniemann Jan 26, 2019

kurkle Jan 26, 2019 •

edited

Loading

sebiniemann Jan 26, 2019

sebiniemann commented Jan 26, 2019 •

edited

Loading

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019

simonbrunel commented Jan 26, 2019

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019

benmccann Jan 26, 2019

Serving the CSS definitions in a CSP-compliant manner, while remaining compatible with v2.0 #6015

Serving the CSS definitions in a CSP-compliant manner, while remaining compatible with v2.0 #6015

Conversation

sebiniemann commented Jan 25, 2019 • edited Loading

sebiniemann commented Jan 25, 2019

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019 • edited Loading

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019 • edited Loading

sebiniemann commented Jan 26, 2019 • edited Loading

kurkle left a comment

Choose a reason for hiding this comment

kurkle Jan 26, 2019

Choose a reason for hiding this comment

sebiniemann Jan 26, 2019

Choose a reason for hiding this comment

kurkle Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

sebiniemann Jan 26, 2019

Choose a reason for hiding this comment

sebiniemann commented Jan 26, 2019 • edited Loading

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019

simonbrunel commented Jan 26, 2019

simonbrunel commented Jan 26, 2019

sebiniemann commented Jan 26, 2019

benmccann Jan 26, 2019

Choose a reason for hiding this comment

sebiniemann commented Jan 25, 2019 •

edited

Loading

sebiniemann commented Jan 26, 2019 •

edited

Loading

sebiniemann commented Jan 26, 2019 •

edited

Loading

sebiniemann commented Jan 26, 2019 •

edited

Loading

kurkle Jan 26, 2019 •

edited

Loading

sebiniemann commented Jan 26, 2019 •

edited

Loading