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

[BUG] Consider removing innerHTML usage #5902

Closed
jeroenheijmans opened this issue Dec 9, 2018 · 7 comments
Closed

[BUG] Consider removing innerHTML usage #5902

jeroenheijmans opened this issue Dec 9, 2018 · 7 comments
Milestone

Comments

@jeroenheijmans
Copy link

jeroenheijmans commented Dec 9, 2018

Expected Behavior

Firefox addons being accepted without warnings around Chart.js.

Current Behavior

Submitting an addon to the Firefox store gives:

Unsafe assignment to innerHTML

Warning: Due to both security....

When searching through master, I get one offending line:

resizer.innerHTML =
'<div class="' + cls + '-expand" style="' + style + '">' +
'<div style="' +
'position:absolute;' +
'width:' + maxSize + 'px;' +
'height:' + maxSize + 'px;' +
'left:0;' +
'top:0">' +
'</div>' +
'</div>' +
'<div class="' + cls + '-shrink" style="' + style + '">' +
'<div style="' +
'position:absolute;' +
'width:200%;' +
'height:200%;' +
'left:0; ' +
'top:0">' +
'</div>' +
'</div>';

At first glance this seems to be the same usage in the minified build.

Possible Solution

Unsure, but perhaps there's another way to do the same thing in that line?

Steps to Reproduce (for bugs)

  1. Follow the Firefox addon submission wizard for an addon that includes Chart.min.js as a content_script

Context

Reviews of such addons tend to take longer, or the addon might even be rejected based on this.

Environment

  • Chart.js version: 2.7.3
  • Browser name and version: n/a
  • Link to your project: n/a
@benmccann
Copy link
Contributor

You could use document.createElement, appendChild, etc. to create the dom structure without using innerHTML. Feel free to send a PR

@jeroenheijmans
Copy link
Author

Thx for letting me know, and indicating that a PR is welcome. Will have a go at creating one some time soon.

@simonbrunel
Copy link
Member

simonbrunel commented Dec 13, 2018

Converting the whole innerHTML block as createElement, appendChild, setAttribute, etc. (as @benmccann suggested) will be a mess if the inline static style is not first converted to CSS classes and injected when initialized (fixing #5208 btw). But that means it would break even more projects using Chart.js within shadow DOM, so we first need to fix #5763 before considering removing this innerHTML.

@jeroenheijmans I may have a look on these issues, knowing pretty well that part of the code.

@jeroenheijmans
Copy link
Author

Okay, thank you for the ping @simonbrunel, I'll hold off for the moment then. Let me know if I can help (e.g. test).

@simonbrunel
Copy link
Member

@jeroenheijmans if Firefox doesn't complain about .cssText then what @benmccann suggested could be a viable solution since we will not have to convert the style to JS properties or CSS classes.

@simonbrunel
Copy link
Member

@jeroenheijmans can you build #5909 and verify if it fixes this issue?

@jeroenheijmans
Copy link
Author

Built that branch and verified my app keeps working as expected, and that the Mozilla add on store doesn't give any warnings anymore.

Thanks!

@simonbrunel simonbrunel added this to the Version 2.8 milestone Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants