Apply styles to SVG text elements directly as allowed by strict CSPs #7256
Conversation
Strict Content Security Policies (those without 'unsafe-inline' keyword)
does not permit inline styles (setting the 'style' attribute in code).
However, setting individual style properties on an element object is
allowed.
Therefore, fix the "svg_text_utils.js" by changing the code that
retrieves, manipulates and applies the style attribute strings of the
"pseudo-HTML" configuration to instead parse and/or apply styles
directly on the element. In other words, instead of using
`d3.select(node).attr("style", "some string value")`, use
`d3.select(node).style(name, value)` as shown in the D3JS docs:
https://d3js.org/d3-selection/selecting#select
With this method, in addition to it being allowed by string CSPs, the
D3 JS library and/or the browser seems to do some level of input
validation and normalization. As such, unit test cases were updated to
account for this differences, which includes:
- Order and format of the attributes were changed. For example,
there will be a space after the colon of the CSS style when read back
from the browser.
- Invalid style attributes would not be applied. Thus, fixed test cases
with actual valid styles.
- Setting the "color" style attribute in SVG text actually normalizes to
setting the "fill" color attribute.
- Using "Times New Roman" font will cause "make-baseline" test to fail
due to "error 525: plotly.js error" when run by the Kaleido Python
library. Root cause of that is probably too deep to get into and
removing it does not change the substance of that test case (using
"Times, serif" achieves the same result).
|
Hi @archmoj and @birkskyum, this new PR also relates to strict CSPs. I mentioned "another issue" in PR #7109 since this is unrelated to fixes discussed in that PR and its history. Instead, it relates to "pseudo-HTML" configurations that convert to SVG text elements/spans, such as those generated by formatted |
gvwilson
added
community
| @@ -3,7 +3,7 @@ | |||
| { | |||
| "x": ["<b>b</b> <i>i</i>", "line <em>1</em><br>line <b>2</b>"], | |||
| "y": ["sub<sub>1</sub>", "sup<sup>2</sup>"], | |||
| "name": "<b>test</b> <i>pseudo</i>HTML<br><sup>3</sup>H<sub>2</sub>O is <em>heavy!</em><br>and <span style=\"color:#f00;font-family:'Times New Roman', Times, serif\">Fonts,</span><br>oh my?" | |||
| "name": "<b>test</b> <i>pseudo</i>HTML<br><sup>3</sup>H<sub>2</sub>O is <em>heavy!</em><br>and <span style=\"color:#f00;font-family:'Times', serif\">Fonts,</span><br>oh my?" | |||
There was a problem hiding this comment.
Is this change necessary? Would the former family list be handled differently by the new code?
There was a problem hiding this comment.
If I remember correctly (it's been a while now), it caused errors with the test automation when run by CircleCI (don't have exact details at the moment). I assumed it was an environment/setup issue, for example, font not installed. Removing Times New Roman helped get the test case to run and pass without errors. I believe the spirit of the test case is intact to show that the font style did get applied as expected.
There was a problem hiding this comment.
hmph, the CI env hasn't changed though, so if the same input that used to work now causes errors, doesn't that mean there's something different now about how we're handling these attributes?
Overall this is a fantastic change! Just want to make sure we aren't creating a subtle bug along the way.
There was a problem hiding this comment.
Thanks! Yes, it makes sense to double check this change. I can try to reproduce when I get a chance (maybe in the next week or two). I vaguely remember it causing the CircleCI tests to fail, but no errors when testing the fonts via the browser on my laptop. I can try to verify with a CodeSandbox.
This addresses another part of the Plotly code that uses inline CSS (should be related to #2355). This case is less severe than the situation fixed by PR #7109 because it only affects some "advanced" configurations, such as those using "pseudo-HTML" in data set names or in
hovertemplatesettings. Plots that did not utilize "pseudo-HTML" were not impacted. Even if it did use "pseudo-HTML", depending on how much text formatting was done, the impact of strict CSPs may not be too noticeable. For example, bolded text would have rendered, but without the bold font.Strict Content Security Policies (those without 'unsafe-inline' keyword) does not permit inline styles (setting the 'style' attribute in code). However, setting individual style properties on an element object is allowed.
Therefore, this fixes the "svg_text_utils.js" by changing the code that retrieves, manipulates, and applies the style attribute strings of the "pseudo-HTML" configuration to instead parse and/or apply styles directly on the element. In other words, instead of using
d3.select(node).attr("style", "some string value"), used3.select(node).style(name, value)as shown in the D3JS docs: https://d3js.org/d3-selection/selecting#selectWith this method, in addition to it being allowed by string CSPs, the D3 JS library and/or the browser seems to do some level of input validation and normalization. As such, unit test cases were updated to account for this differences, which includes:
Testing
I tested using the
plotly-basic.jsbuild before and after the fix and configured a basic scatter chart with ahovertemplateconfiguration containing a lot of formatting using the "pseudo-HTML" allowed by this library. When hovering on the data points, it's very obvious when styles are properly applied.Note: The "before fix" CSB uses the
plotly-basic.jsbuilt frommasterthat is the base of this branch since it needs PR #7109 to work properly.