Fixing a XSS vector ( chart name )

[ryanrath]

Description

• A user was able to input raw html / javascript and have it treated as html /
  javascript as opposed to text.

Motivation and Context

XSS is generally frowned upon

Tests performed

Manual Tests:

• Load / Create a new Chart via Metrics Explorer
• Open the Chart Options and change the name to: " b"
• Press the Tab button
• Pre-fix: Notice that an alert has been generated on the page ( actually, there's probably 2, once for the change and once when the chart reloads ).
• Post-fix: The chart name text is changed to: b

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project as found in the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[jpwhite4]

This fix doesn't appear to address the described issue since it doesn't stop the user entering arbitrary html, it simple stops the arbitrary html being rendered in one place in the code. What about all the other places that this data is used? Surely the proper fix is to sanitise the data on the way in rather than the way out.

[ryanrath]

I think we have different opinions on what the described issue is. I maintain that the issue is not that the user "can" provide input that qualifies as valid html. But with how this input, that may possibly contain html, is treated when it comes time to include it in the dom. Keep in mind that we do not only use this information for display, there are at least 4 primary locations in the MetricExplorer code that utilize the 'name' property in a strict equivalence with other information. Which, if we change the way in which the 'name' property is stored, would need to be inspected to ensure that they still behave as expected.

Regardless of the encoding / sanitation of data prior to save, all points at which data is rendered into the dom should take steps to properly encode said data and due to Ext does not doing this for us in all cases ( as evidenced by finding this bug in the first place ), we need to take care of it ourselves.

As an aside: the reason the chart title doesn't act this way is that the title is only ever displayed in text inputs, or text elements in svg and so the executing context is never one in which html / javascript would be evaluated.

Let us also take a moment to curse the Ext.Element.update function for utilizing 'innerHtml' and the child components that use it without escaping where appropriate. As this is basically the root of the problem.

[ryanrath]

Note: I don't think that it's a bad thing / idea to sanitize data before saving / further use. I just think that in this particular case it's not going to be the whole solution and may incur some additional changes.