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.

Sign up for GitHub

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

Apply styles to SVG text elements directly as allowed by strict CSPs #7256

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Apply styles to SVG text elements directly as allowed by strict CSPs #7256

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91f668c
Apply styles to SVG text elements allowed by strict CSPs
martian111 Oct 27, 2024
d561644
Add draftlogs for Pull Request #7256
martian111 Oct 27, 2024
3ded59e
Update draftlogs/7256_fix.md
archmoj Dec 16, 2024
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
1 change: 1 addition & 0 deletions draftlogs/7256_fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove inline styles in SVG text elements generated from pseudo-HTML configurations, which break with strict CSP setups [[#7256](https://github.com/plotly/plotly.js/pull/7256)]
archmoj marked this conversation as resolved.
Show resolved Hide resolved
83 changes: 64 additions & 19 deletions src/lib/svg_text_utils.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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