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

CSP - style-src 'unsafe-inline' should not be required #5208

Closed
fxOne opened this issue Jan 30, 2018 · 15 comments · Fixed by #6048
Closed

CSP - style-src 'unsafe-inline' should not be required #5208

fxOne opened this issue Jan 30, 2018 · 15 comments · Fixed by #6048

Comments

@fxOne
Copy link

fxOne commented Jan 30, 2018

Expected Behavior

Chart.js should not depend on the the Content-Security-Policy: style-src 'unsafe-inline' directive.

Current Behavior

Chart.js adds errors to the console as the css is refused by the CSP rules

Possible Solution

Add a nonce attribute and make it possible to set the nonce.
See also: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#Unsafe_inline_script

Steps to Reproduce (for bugs)

  1. Add Chart.js to a page
  2. Open the page with the Content-Security-Policy: style-src 'self' directive set

Context

This are the error messages:

Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self'". Either the 'unsafe-inline' keyword, a hash ('sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU='), or a nonce ('nonce-...') is required to enable inline execution.
Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self'". Either the 'unsafe-inline' keyword, a hash ('sha256-OTeu7NEHDo6qutIWo0F2TmYrDhsKWCzrUgGoxxHGJ8o='), or a nonce ('nonce-...') is required to enable inline execution.

The first error occurs in platform.dom.js:308 and the 2nd in platform.dom.js:311

Environment

  • Chart.js version: 2.7.1
  • Browser name and version: Chrome Version 66.0.3334.0
@ETNyx
Copy link

ETNyx commented Jan 31, 2018

I confirm the bug,

2.6 is not affected

@jrberlin
Copy link

jrberlin commented Feb 6, 2018

We are having the same issue after upgrading to 2.7.1, is there any workaround to fix this without disabling/changing our current CSP policy ?

Thank you !!
Julian

@fxOne
Copy link
Author

fxOne commented Feb 13, 2018

@jrberlin you can add the hashes 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=' and 'sha256-OTeu7NEHDo6qutIWo0F2TmYrDhsKWCzrUgGoxxHGJ8o=' to the CSP header to solve the problem.

@jrberlin
Copy link

Hi
Thanks for the response, I checked it out and it works in all browsers except MS Edge, in such browser I get:

CSP14321: Resource violated directive 'style-src 'self' 'sha256-fviu5RwuBYFcCd5CDanhy6NCLufcwvCAbm061aSqhoQ=' 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=' 'sha256-OTeu7NEHDo6qutIWo0F2TmYrDhsKWCzrUgGoxxHGJ8o=' 'sha256-wS7xf+bhXBr5EM064hQkAW0vX3ks5VoxbGn+KQC/Vhk=' 'sha256-cxL35Ug49Sl1zHMOdz/r0xinQ6BYGgClHdDCk2XPTzE='' in Content-Security-Policy: inline style, in ourUrl at line 0 column 0. Resource will be blocked.

the version of MS EDGE is 40.15063.674.0 and MS EdgeHTML is 15.15063, but I think is not the latest version.

@NicoHaase
Copy link

Any news on this?

@vletoux
Copy link

vletoux commented Mar 30, 2018

+1
There is also an issue in:

function createResizer(handler) {

which injects data with hardcoded style elements when enabling responsive:true & maintainAspectRatio: false

Since the style injected is static, why not adding CSS classes and adding these class to the targeted element ? This is CSP compliant.

@smariel
Copy link

smariel commented Jul 15, 2018

+1
I confirm, in v2.7.2, there are conflicts with 'self' Content Security Policy and the following lines:

'<div class="' + cls + '-shrink" style="' + style + '">' +

document.getElementsByTagName('head')[0].appendChild(style);

style.appendChild(document.createTextNode(css));

@heapwolf
Copy link

heapwolf commented Sep 2, 2018

Hi, this is sort of blocking me from releasing https://tonic.technology — I'd be happy to make a PR. Possible fixes for the issues that @smariel points out could be...

  1. After the element is rendered, querySelect it and add the style with JS (that will pass CSP).

  2. For the head element, we need to set an attribute (either nonce or integrity on the programatically created style element (either could be passed in as a config option to the constructor.

  3. This would be solved by solving the second issue.

@schalkventer
Copy link

Have there been any movement on this issue? I'm happy to try my hand at a PR to fix this?

@panuhorsmalahti
Copy link

panuhorsmalahti commented Dec 13, 2018

I fixed this by

  1. Setting CSP header style-src 'self' 'sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=' 'sha256-OTeu7NEHDo6qutIWo0F2TmYrDhsKWCzrUgGoxxHGJ8o=';
  2. Adding the following styles:
.chartjs-size-monitor,
.chartjs-size-monitor-expand,
.chartjs-size-monitor-shrink {
    position: absolute;
    left: 0;
    top: 0;
    right: 0;
    bottom: 0;
    overflow: hidden;
    pointer-events: none;
    visibility: hidden;
    z-index: -1;
}

.chartjs-size-monitor-expand > div {
    position: absolute;
    width: 1000000px;
    height: 1000000px;
    left: 0;
    top: 0;
}

.chartjs-size-monitor-shrink > div {
    position: absolute;
    width: 200%;
    height: 200%;
    left: 0;
    top: 0;
}
  1. Forking Chart.js and removing the inline styles from https://github.com/chartjs/Chart.js/blob/master/src/platforms/platform.dom.js#L170

This works at least on latest Firefox and Chrome.

@simonbrunel
Copy link
Member

Add a nonce attribute and make it possible to set the nonce
For the head element, we need to set an attribute (either nonce or integrity on the programatically created style element (either could be passed in as a config option to the constructor.

@fxOne @hxoht what would be your approach to implement this solution? Can you write a small snippet showing how it would work in HTML / Javascript? (I'm not familiar with CSP so don't fully get how it's supposed to be configured).

jelhan added a commit to jelhan/Chart.js that referenced this issue 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 chartjs#5208 together with chartjs#5909
jelhan added a commit to jelhan/Chart.js that referenced this issue 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 chartjs#5208 together with chartjs#5909
jelhan added a commit to jelhan/Chart.js that referenced this issue Jan 2, 2019
This uses CSS Object Model (CSSOM) to modify stylesheets.
Changing stylesheets via CSSOM does not violate
Content-Security-Policy style-src 'none'.

CSSOM is still a working draft but the features been used should
be supported by all target browsers:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/styleSheets#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule#Browser_compatibility

Creating an empty style element does not violate CSP.

-webkit- prefix is not needed anymore for keyframe.
Inserting CSS rule @-webkit-keyframews throws a SyntaxError in IE11.

Done basic manual testing using samples in recent versions of:
- Chrome (Desktop & Mobile)
- Firefox
- Microsoft Edge
- IE 11

Fixes chartjs#5208 together with chartjs#5909

Live example: https://codepen.io/jelhan/pen/jXYymO

Please note the CSP meta tag definied in settings. You need to update SHA
hashes if you change any JavaScript in this Codepen as it violates CSP
otherwise.
jelhan added a commit to jelhan/Chart.js that referenced this issue Jan 3, 2019
This uses CSS Object Model (CSSOM) to modify stylesheets.
Changing stylesheets via CSSOM does not violate
Content-Security-Policy style-src 'none'.

CSSOM is still a working draft but the features been used should
be supported by all target browsers:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/styleSheets#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule#Browser_compatibility

Creating an empty style element does not violate CSP.

-webkit- prefix is not needed anymore for keyframe.
Inserting CSS rule @-webkit-keyframews throws a SyntaxError in IE11.

Done basic manual testing using samples in recent versions of:
- Chrome (Desktop & Mobile)
- Firefox
- Microsoft Edge
- IE 11

Fixes chartjs#5208 together with chartjs#5909

Live example: https://codepen.io/jelhan/pen/jXYymO

Please note the CSP meta tag definied in settings. You need to update SHA
hashes if you change any JavaScript in this Codepen as it violates CSP
otherwise.
jelhan added a commit to jelhan/Chart.js that referenced this issue Jan 3, 2019
This uses CSS Object Model (CSSOM) to modify stylesheets.
Changing stylesheets via CSSOM does not violate
Content-Security-Policy style-src 'none'.

CSSOM is still a working draft but the features been used should
be supported by all target browsers:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/styleSheets#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule#Browser_compatibility

Creating an empty style element does not violate CSP.

-webkit- prefix is not needed anymore for keyframe.
Inserting CSS rule @-webkit-keyframews throws a SyntaxError in IE11.

Done basic manual testing using samples in recent versions of:
- Chrome (Desktop & Mobile)
- Firefox
- Microsoft Edge
- IE 11

Fixes chartjs#5208 together with chartjs#5909

Live example: https://codepen.io/jelhan/pen/jXYymO

Please note the CSP meta tag definied in settings. You need to update SHA
hashes if you change any JavaScript in this Codepen as it violates CSP
otherwise.
jelhan added a commit to jelhan/Chart.js that referenced this issue Jan 3, 2019
This uses CSS Object Model (CSSOM) to modify stylesheets.
Changing stylesheets via CSSOM does not violate
Content-Security-Policy style-src 'none'.

CSSOM is still a working draft but the features been used should
be supported by all target browsers:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/styleSheets#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule#Browser_compatibility

Creating an empty style element does not violate CSP.

-webkit- prefix is not needed anymore for keyframe.
Inserting CSS rule @-webkit-keyframews throws a SyntaxError in IE11.

Done basic manual testing using samples in recent versions of:
- Chrome (Desktop & Mobile)
- Firefox
- Microsoft Edge
- IE 11

Fixes chartjs#5208 together with chartjs#5909

Live example: https://codepen.io/jelhan/pen/jXYymO

Please note the CSP meta tag definied in settings. You need to update SHA
hashes if you change any JavaScript in this Codepen as it violates CSP
otherwise.
@simonbrunel
Copy link
Member

Could someone who reported CSP errors be able to build PR #5952 and confirm that it solves this issue?

@smariel
Copy link

smariel commented Jan 4, 2019

I am not familiar with github and building a project, so I'm not sure I did the right thing...

In #5952, clicked on commits then on <> to browse the repo. I cloned it and built it following the instructions. Then I put the chart.js folder in my node_modules, in place of the "official", to keep the same context.

Keeping unsafe-inline, chart.js works well.

But if I remove this policy, I still have a violiation in platform.dom.js:312:

document.getElementsByTagName('head')[0].appendChild(style);

And I also have the following error at line 316:
Uncaught TypeError: Cannot read property 'insertRule' of null

style.sheet.insertRule(rule, style.sheet.cssRules.length);

@simonbrunel
Copy link
Member

Thanks @smariel, you did it right, and that confirms #5952 doesn't fix the CSP issue.

@sebiniemann
Copy link
Contributor

sebiniemann commented Jan 25, 2019

A nonce is usually a bad solution for most applications. The whole security relies on the hope that it cannot be guessed, since it doesn't rely on the content it protects. For this to work, the nonce must at least be changed for every request, which requires to dynamically create the HTML and adds further requirements for any user. Otherwise, it is only hiding the underlying issue and creates an (in my opinion) even bigger security problem, as somebody might now trust a still insecure implementation.

A good and secure solution was already pointed out by @panuhorsmalahti, using a hash instead of a nonce and removing the CSS injection.

Going further in this direction, the extracted CSS information could be inserted into a file (lets call it chartjs.css) and be delivered together with Chart.js. A simple boolean switch could control whether the createDiv function executes el.style.cssText = style || ''; (so everything remain compatible) or not.

The switch could be set via

import Chart from 'chart.js';
Chart.useSecureCSS = true;

Lastly, adding some documentation to it, stating how to securely integrate the CSS code:

  • setting the switch to true
  • including the CSS code via <style src="chartjs/chartjs.css"></style>

as well as deprecating the default injection (so folks know only the secure way would remain after the next major release and could prepare beforehand) could complete this solution and eases the change management pressure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet