Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Apply styles to SVG text elements allowed by strict CSPs
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).
  • Loading branch information
martian111 committed Oct 27, 2024
commit 91f668c03e5b474152842565f54ced1b577d4340
83 changes: 64 additions & 19 deletions src/lib/svg_text_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,15 +328,15 @@ var TAG_STYLES = {
// would like to use baseline-shift for sub/sup but FF doesn't support it
// so we need to use dy along with the uber hacky shift-back-to
// baseline below
sup: 'font-size:70%',
sub: 'font-size:70%',
s: 'text-decoration:line-through',
u: 'text-decoration:underline',
b: 'font-weight:bold',
i: 'font-style:italic',
a: 'cursor:pointer',
span: '',
em: 'font-style:italic;font-weight:bold'
sup: {'font-size':'70%'},
sub: {'font-size':'70%'},
s: {'text-decoration':'line-through'},
u: {'text-decoration':'underline'},
b: {'font-weight': 'bold'},
i: {'font-style':'italic'},
a: {'cursor':'pointer'},
span: {},
em: {'font-style':'italic','font-weight':'bold'}
};

// baseline shifts for sub and sup
Expand Down Expand Up @@ -383,9 +383,6 @@ exports.BR_TAG_ALL = /<br(\s+.*)?>/gi;
* convention and will not make a popup if this string is empty.
* per the spec, cannot contain whitespace.
*
* Because we hack in other attributes with style (sub & sup), drop any trailing
* semicolon in user-supplied styles so we can consistently append the tag-dependent style
*
* These are for tag attributes; Chrome anyway will convert entities in
* attribute values, but not in attribute names
* you can test this by for example:
Expand All @@ -394,7 +391,7 @@ exports.BR_TAG_ALL = /<br(\s+.*)?>/gi;
* > p.innerHTML
* <- '<span styl&#x65;="font-color:red;">Hi</span>'
*/
var STYLEMATCH = /(^|[\s"'])style\s*=\s*("([^"]*);?"|'([^']*);?')/i;
var STYLEMATCH = /(^|[\s"'])style\s*=\s*("([^"]*)"|'([^']*)')/i;
var HREFMATCH = /(^|[\s"'])href\s*=\s*("([^"]*)"|'([^']*)')/i;
var TARGETMATCH = /(^|[\s"'])target\s*=\s*("([^"\s]*)"|'([^'\s]*)')/i;
var POPUPMATCH = /(^|[\s"'])popup\s*=\s*("([\w=,]*)"|'([\w=,]*)')/i;
Expand Down Expand Up @@ -495,7 +492,8 @@ var entityToUnicode = {
nbsp: ' ',
times: '×',
plusmn: '±',
deg: '°'
deg: '°',
quot: "'",
};

// NOTE: in general entities can contain uppercase too (so [a-zA-Z]) but all the
Expand Down Expand Up @@ -537,6 +535,50 @@ function fromCodePoint(code) {
);
}

var SPLIT_STYLES = /([^;]+;|$)|&(#\d+|#x[\da-fA-F]+|[a-z]+);/;

var ONE_STYLE = /^\s*([^:]+)\s*:\s*(.+?)\s*;?$/i;

function applyStyles(node, styles) {
var parts = styles.split(SPLIT_STYLES);
var filteredParts = [];
for(var i = 0; i < parts.length; i++) {
if(parts[i] && typeof parts[i] === "string" && parts[i].length > 0) {
filteredParts.push(parts[i]);
}
}
parts = filteredParts;

for(var i = 0; i < parts.length; i++) {
var parti = parts[i];

// Recombine parts that was split due to HTML entity's semicolon
var partToSearch = parti;
do {
var matchEntity = partToSearch.match(ENTITY_MATCH);
if(matchEntity) {
var entity = matchEntity[0];
partToSearch = parts[i+1];
if(parti.endsWith(entity) && partToSearch) {
// Matched HTML entity is at the end, and thus, need to
// combine with next part to complete the style (when it ends
// with a semicolon that is not part of a HTML entity)
parti += partToSearch;
i++;
}
} else {
partToSearch = undefined;
}
} while (partToSearch);

var match = parti.match(ONE_STYLE);
if(match) {
var decodedStyle = convertEntities(match[2]);
d3.select(node).style(match[1], decodedStyle);
}
}
}

/*
* buildSVGText: convert our pseudo-html into SVG tspan elements, and attach these
* to containerNode
Expand Down Expand Up @@ -613,8 +655,6 @@ function buildSVGText(containerNode, str) {
}
} else nodeType = 'tspan';

if(nodeSpec.style) nodeAttrs.style = nodeSpec.style;

var newNode = document.createElementNS(xmlnsNamespaces.svg, nodeType);

if(type === 'sup' || type === 'sub') {
Expand All @@ -633,6 +673,10 @@ function buildSVGText(containerNode, str) {
}

d3.select(newNode).attr(nodeAttrs);
if(nodeSpec.style) applyStyles(newNode, nodeSpec.style)
if(nodeSpec.tagStyle) {
d3.select(newNode).style(nodeSpec.tagStyle);
}

currentNode = nodeSpec.node = newNode;
nodeStack.push(nodeSpec);
Expand Down Expand Up @@ -693,10 +737,10 @@ function buildSVGText(containerNode, str) {
var css = getQuotedMatch(extra, STYLEMATCH);
if(css) {
css = css.replace(COLORMATCH, '$1 fill:');
if(tagStyle) css += ';' + tagStyle;
} else if(tagStyle) css = tagStyle;
}

if(css) nodeSpec.style = css;
if(tagStyle) nodeSpec.tagStyle = tagStyle;

if(tagType === 'a') {
hasLink = true;
Expand Down Expand Up @@ -770,7 +814,7 @@ exports.sanitizeHTML = function sanitizeHTML(str) {
var extra = match[4];

var css = getQuotedMatch(extra, STYLEMATCH);
var nodeAttrs = css ? {style: css} : {};
var nodeAttrs = {};

if(tagType === 'a') {
var href = getQuotedMatch(extra, HREFMATCH);
Expand All @@ -790,6 +834,7 @@ exports.sanitizeHTML = function sanitizeHTML(str) {
var newNode = document.createElement(tagType);
currentNode.appendChild(newNode);
d3.select(newNode).attr(nodeAttrs);
if(css) applyStyles(newNode, css);

currentNode = newNode;
nodeStack.push(newNode);
Expand Down
2 changes: 1 addition & 1 deletion test/image/mocks/pseudo_html.json
Original file line number Diff line number Diff line change
Expand Up @@ -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?"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? Would the former family list be handled differently by the new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
],
"layout": {
Expand Down
6 changes: 3 additions & 3 deletions test/jasmine/tests/hover_label_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ describe('hover info', function() {

it('provides exponents correctly for z data', function(done) {
function expFmt(val, exp) {
return val + '×10\u200b<tspan style="font-size:70%" dy="-0.6em">' +
return val + '×10\u200b<tspan dy="-0.6em" style="font-size: 70%;">' +
(exp < 0 ? MINUS_SIGN + -exp : exp) +
'</tspan><tspan dy="0.42em">\u200b</tspan>';
}
Expand Down Expand Up @@ -2071,7 +2071,7 @@ describe('hover info', function() {
expect(hoverTrace.y).toEqual(1);

assertHoverLabelContent({
nums: '<tspan style="font-weight:bold">$1.00</tspan>\nPV learning curve.txt',
nums: '<tspan style="font-weight: bold;">$1.00</tspan>\nPV learning curve.txt',
name: '',
axis: '0.388'
});
Expand Down Expand Up @@ -2120,7 +2120,7 @@ describe('hover info', function() {
expect(hoverTrace.y).toEqual(1);

assertHoverLabelContent({
nums: 'Cost ($/W​<tspan style="font-size:70%" dy="0.3em">P</tspan><tspan dy="-0.21em">​</tspan>):$1.00\nCumulative Production (GW):0.3880',
nums: 'Cost ($/W​<tspan dy="0.3em" style="font-size: 70%;">P</tspan><tspan dy="-0.21em">​</tspan>):$1.00\nCumulative Production (GW):0.3880',
name: '',
axis: '0.388'
});
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/icicle_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ describe('Test icicle hover:', function() {
exp: {
label: {
nums: 'Abel :: 6.00',
name: '<tspan style="font-weight:bold">N.B.</tspan>'
name: '<tspan style="font-weight: bold;">N.B.</tspan>'
},
ptData: {
curveNumber: 0,
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/sunburst_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ describe('Test sunburst hover:', function() {
exp: {
label: {
nums: 'Abel :: 6.00',
name: '<tspan style="font-weight:bold">N.B.</tspan>'
name: '<tspan style="font-weight: bold;">N.B.</tspan>'
},
ptData: {
curveNumber: 0,
Expand Down
Loading