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.

Sign up for GitHub

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

Jump to bottom

Remove innerHTML usage from our DOM platform #5909

Merged
merged 1 commit into from
Dec 14, 2018
Merged

Remove innerHTML usage from our DOM platform #5909

merged 1 commit into from
Dec 14, 2018

Conversation

simonbrunel
Copy link
Member

Prevent "Unsafe assignment to innerHTML" reported by Firefox when submitting addon to their store.

Enhances #5208
Fixes #5902

@simonbrunel
Remove innerHTML usage from our DOM platform
a224bec 
Prevent "Unsafe assignment to innerHTML" reported by Firefox when submitting addon to their store.
@simonbrunel simonbrunel mentioned this pull request Dec 13, 2018
Closed
@jeroenheijmans
Copy link

I have succesfully built the branch, and tested it by:

  • included that custom build in my application, verifying (at a glance) that everything worked as expected
  • included that custom build in my repro for [BUG] Consider removing innerHTML usage #5902 and tried uploading it to the Mozilla add-on store, confirming the store no longer complains about potential vulnerabilities in Chart.min.js

In short: LGTM! Thanks.

@simonbrunel
Copy link
Member Author

Thanks @jeroenheijmans and quite unexpectedly, this version generates smaller builds.

@etimberg etimberg merged commit f2a9e66 into chartjs:master Dec 14, 2018
@simonbrunel simonbrunel added this to the Version 2.8 milestone Dec 14, 2018
@simonbrunel simonbrunel deleted the fix/inner-html branch December 14, 2018 05:25
jelhan added a commit to jelhan/Chart.js that referenced this pull request Dec 31, 2018
@jelhan
fix: injecting style element violates CSP
1356a1f 
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 mentioned this pull request Dec 31, 2018
Closed
jelhan added a commit to jelhan/Chart.js that referenced this pull request Dec 31, 2018
@jelhan
fix: injecting style element violates CSP
90f55cc 
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 pull request Jan 2, 2019
@jelhan
fix CSP by injecting styles using CSSOM
cb2db07 
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 jelhan mentioned this pull request Jan 2, 2019
Closed
jelhan added a commit to jelhan/Chart.js that referenced this pull request Jan 3, 2019
@jelhan
fix CSP by injecting styles using CSSOM
4581859 
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 pull request Jan 3, 2019
@jelhan
fix CSP by injecting styles using CSSOM
abb832d 
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 pull request Jan 3, 2019
@jelhan
fix CSP by injecting styles using CSSOM
7cd3961 
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 simonbrunel mentioned this pull request Apr 2, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Nov 9, 2019
Merged
This was referenced Mar 4, 2020
Open
Open
@vpetersson vpetersson mentioned this pull request Mar 7, 2020
Merged
This was referenced Mar 9, 2020
Closed
Merged
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 14, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 15, 2020
Open
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
@simonbrunel @etimberg
Remove innerHTML usage from our DOM platform (chartjs#5909)
2c77c45 
Prevent "Unsafe assignment to innerHTML" reported by Firefox when submitting addon to their store.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etimberg etimberg etimberg approved these changes

@benmccann benmccann benmccann approved these changes

@nagix nagix Awaiting requested review from nagix

Assignees
No one assigned
Labels
type: bug
Projects
None yet
Milestone
Version 2.8
Development

Successfully merging this pull request may close these issues.
4 participants
@simonbrunel @jeroenheijmans @benmccann @etimberg