Chart.js: [BUG] Consider removing innerHTML usage

Created on 9 Dec 2018  ·  7Comments  ·  Source: chartjs/Chart.js

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:

https://github.com/chartjs/Chart.js/blob/75aa44eef677b790768262638df0de223fd01a56/src/platforms/platform.dom.js#L187-L205

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
bug

Most helpful comment

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

All 7 comments

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

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

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.

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).

@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.

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

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!

Was this page helpful?
0 / 5 - 0 ratings