From b5cfd69f8735672e2b85de9acd9db3f885487217 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 12 May 2017 10:28:23 -0400 Subject: [PATCH 1/5] lint svg_text_utils_test grammar --- test/jasmine/tests/svg_text_utils_test.js | 28 +++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/jasmine/tests/svg_text_utils_test.js b/test/jasmine/tests/svg_text_utils_test.js index 3468f65317c..704530d464f 100644 --- a/test/jasmine/tests/svg_text_utils_test.js +++ b/test/jasmine/tests/svg_text_utils_test.js @@ -6,7 +6,7 @@ var util = require('@src/lib/svg_text_utils'); describe('svg+text utils', function() { 'use strict'; - describe('convertToTspans should', function() { + describe('convertToTspans', function() { function mockTextSVGElement(txt) { return d3.select('body') @@ -60,7 +60,7 @@ describe('svg+text utils', function() { d3.select('#text').remove(); }); - it('check for XSS attack in href', function() { + it('checks for XSS attack in href', function() { var node = mockTextSVGElement( 'XSS' ); @@ -70,7 +70,7 @@ describe('svg+text utils', function() { assertAnchorLink(node, null); }); - it('check for XSS attack in href (with plenty of white spaces)', function() { + it('checks for XSS attack in href (with plenty of white spaces)', function() { var node = mockTextSVGElement( 'XSS' ); @@ -80,7 +80,7 @@ describe('svg+text utils', function() { assertAnchorLink(node, null); }); - it('whitelist relative hrefs (interpreted as http)', function() { + it('whitelists relative hrefs (interpreted as http)', function() { var node = mockTextSVGElement( 'mylink' ); @@ -90,7 +90,7 @@ describe('svg+text utils', function() { assertAnchorLink(node, '/mylink'); }); - it('whitelist http hrefs', function() { + it('whitelists http hrefs', function() { var node = mockTextSVGElement( 'bl.ocks.org' ); @@ -100,7 +100,7 @@ describe('svg+text utils', function() { assertAnchorLink(node, 'http://bl.ocks.org/'); }); - it('whitelist https hrefs', function() { + it('whitelists https hrefs', function() { var node = mockTextSVGElement( 'plot.ly' ); @@ -110,7 +110,7 @@ describe('svg+text utils', function() { assertAnchorLink(node, 'https://plot.ly'); }); - it('whitelist mailto hrefs', function() { + it('whitelists mailto hrefs', function() { var node = mockTextSVGElement( 'support' ); @@ -120,7 +120,7 @@ describe('svg+text utils', function() { assertAnchorLink(node, 'mailto:support@plot.ly'); }); - it('wrap XSS attacks in href', function() { + it('wraps XSS attacks in href', function() { var textCases = [ 'Subtitle', 'Subtitle' @@ -135,7 +135,7 @@ describe('svg+text utils', function() { }); }); - it('should keep query parameters in href', function() { + it('keeps query parameters in href', function() { var textCases = [ 'abc.com?shared-key', 'abc.com?shared-key' @@ -150,7 +150,7 @@ describe('svg+text utils', function() { }); }); - it('allow basic spans', function() { + it('allows basic spans', function() { var node = mockTextSVGElement( 'text' ); @@ -159,7 +159,7 @@ describe('svg+text utils', function() { assertTspanStyle(node, null); }); - it('ignore unquoted styles in spans', function() { + it('ignores unquoted styles in spans', function() { var node = mockTextSVGElement( 'text' ); @@ -168,7 +168,7 @@ describe('svg+text utils', function() { assertTspanStyle(node, null); }); - it('allow quoted styles in spans', function() { + it('allows quoted styles in spans', function() { var node = mockTextSVGElement( 'text' ); @@ -177,7 +177,7 @@ describe('svg+text utils', function() { assertTspanStyle(node, 'quoted: yeah;'); }); - it('ignore extra stuff after span styles', function() { + it('ignores extra stuff after span styles', function() { var node = mockTextSVGElement( 'text' ); @@ -195,7 +195,7 @@ describe('svg+text utils', function() { assertTspanStyle(node, 'quoted: yeah&\';;'); }); - it('decode some HTML entities in text', function() { + it('decodes some HTML entities in text', function() { var node = mockTextSVGElement( '100μ & < 10 > 0  ' + '100 × 20 ± 0.5 °' From aa9fc00f791c09e509b91a8cb62b28052b616a9b Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 12 May 2017 14:53:54 -0400 Subject: [PATCH 2/5] fix #1674 - set cursor:pointer on svg text links also makes our handling of style and href more flexible and robust --- src/lib/svg_text_utils.js | 138 ++++++++++++---------- test/jasmine/tests/svg_text_utils_test.js | 31 ++++- 2 files changed, 105 insertions(+), 64 deletions(-) diff --git a/src/lib/svg_text_utils.js b/src/lib/svg_text_utils.js index bb76fa2d283..6e92555a318 100644 --- a/src/lib/svg_text_utils.js +++ b/src/lib/svg_text_utils.js @@ -221,19 +221,25 @@ function texToSVG(_texString, _config, _callback) { } var TAG_STYLES = { - // would like to use baseline-shift but FF doesn't support it yet + // 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%" dy="-0.6em', sub: 'font-size:70%" dy="0.3em', b: 'font-weight:bold', i: 'font-style:italic', - a: '', + a: 'cursor:pointer', span: '', br: '', em: 'font-style:italic;font-weight:bold' }; +// sub/sup: extra tspan with zero-width space to get back to the right baseline +var TAG_CLOSE = { + sup: '​', + sub: '​' +}; + var PROTOCOLS = ['http:', 'https:', 'mailto:']; var STRIP_TAGS = new RegExp(']*)?/?>', 'g'); @@ -254,6 +260,16 @@ var UNICODE_TO_ENTITY = Object.keys(stringMappings.unicodeToEntity).map(function var NEWLINES = /(\r\n?|\n)/g; +var SPLIT_TAGS = /(<[^<>]*>)/; + +var ONE_TAG = /<(\/?)([^ >]*)(\s+(.*))?>/i; + +var QUOTEDATTR = '\\s*=\\s*("([^"]*)"|\'([^\']*)\')'; +var STYLEMATCH = new RegExp('(^|[\\s"\'])style' + QUOTEDATTR, 'i'); +var HREFMATCH = new RegExp('(^|[\\s"\'])href' + QUOTEDATTR, 'i'); + +var COLORMATCH = /(^|;)\s*color:/; + exports.plainText = function(_str) { // strip out our pseudo-html so we have a readable // version to put into text fields @@ -280,84 +296,86 @@ function encodeForHTML(_str) { } function convertToSVG(_str) { - _str = convertEntities(_str); - - // normalize behavior between IE and others wrt newlines and whitespace:pre - // this combination makes IE barf https://github.com/plotly/plotly.js/issues/746 - // Chrome and FF display \n, \r, or \r\n as a space in this mode. - // I feel like at some point we turned these into
but currently we don't so - // I'm just going to cement what we do now in Chrome and FF - _str = _str.replace(NEWLINES, ' '); + _str = convertEntities(_str) + /* + * Normalize behavior between IE and others wrt newlines and whitespace:pre + * this combination makes IE barf https://github.com/plotly/plotly.js/issues/746 + * Chrome and FF display \n, \r, or \r\n as a space in this mode. + * I feel like at some point we turned these into
but currently we don't so + * I'm just going to cement what we do now in Chrome and FF + */ + .replace(NEWLINES, ' '); var result = _str - .split(/(<[^<>]*>)/).map(function(d) { - var match = d.match(/<(\/?)([^ >]*)\s*(.*)>/i), - tag = match && match[2].toLowerCase(), - style = TAG_STYLES[tag]; - - if(style !== undefined) { - var close = match[1], - extra = match[3], - /** - * extraStyle: any random extra css (that's supported by svg) - * use this like to change font in the middle - * - * at one point we supported but as this isn't even - * valid HTML anymore and we dropped it accidentally for many months, we will not - * resurrect it. - */ - extraStyle = extra.match(/^style\s*=\s*"([^"]+)"\s*/i); - - // anchor and br are the only ones that don't turn into a tspan + .split(SPLIT_TAGS).map(function(d) { + var match = d.match(ONE_TAG); + var tag = match && match[2].toLowerCase(); + var tagStyle = TAG_STYLES[tag]; + + if(tagStyle !== undefined) { + var isClose = match[1]; + if(isClose) return (tag === 'a' ? '' : '') + (TAG_CLOSE[tag] || ''); + + // break: later we'll turn these into newline s + // but we need to know about all the other tags first + if(tag === 'br') return '
'; + + /** + * extra includes href and any random extra css (that's supported by svg) + * use this like to change font in the middle + * + * at one point we supported but as this isn't even + * valid HTML anymore and we dropped it accidentally for many months, we will not + * resurrect it. + */ + var extra = match[4]; + + var out; + + // anchor is the only tag that doesn't turn into a tspan if(tag === 'a') { - if(close) return ''; - else if(extra.substr(0, 4).toLowerCase() !== 'href') return ''; - else { - // remove quotes, leading '=', replace '&' with '&' - var href = extra.substr(4) - .replace(/["']/g, '') - .replace(/=/, ''); - - // check protocol + var hrefMatch = extra && extra.match(HREFMATCH); + var href = hrefMatch && (hrefMatch[3] || hrefMatch[4]); + + out = ''; - - return ''; + if(PROTOCOLS.indexOf(dummyAnchor.protocol) !== -1) { + out += ' xlink:show="new" xlink:href="' + encodeForHTML(href) + '"'; + } } } - else if(tag === 'br') return '
'; - else if(close) { - // closing tag - - // sub/sup: extra tspan with zero-width space to get back to the right baseline - if(tag === 'sup') return '​'; - if(tag === 'sub') return '​'; - else return ''; - } else { - var tspanStart = ''; - } + if(tagStyle) css = tagStyle + ';' + (css || ''); + + if(css) return out + ' style="' + css + '">'; + + return out + '>'; } else { return exports.xml_entity_encode(d).replace(/'); index > 0; index = result.indexOf('
', index + 1)) { indices.push(index); diff --git a/test/jasmine/tests/svg_text_utils_test.js b/test/jasmine/tests/svg_text_utils_test.js index 704530d464f..afc187b7354 100644 --- a/test/jasmine/tests/svg_text_utils_test.js +++ b/test/jasmine/tests/svg_text_utils_test.js @@ -30,7 +30,7 @@ describe('svg+text utils', function() { expect(tspan.attr('style')).toBe(style); } - function assertAnchorAttrs(node) { + function assertAnchorAttrs(node, style) { var a = node.select('a'); var WHITE_LIST = ['xlink:href', 'xlink:show', 'style'], @@ -44,6 +44,8 @@ describe('svg+text utils', function() { }); expect(hasWrongAttr).toBe(false); + + expect(a.attr('style')).toBe('cursor:pointer;' + (style || '')); } function listAttributes(node) { @@ -120,7 +122,8 @@ describe('svg+text utils', function() { assertAnchorLink(node, 'mailto:support@plot.ly'); }); - it('wraps XSS attacks in href', function() { + it('drops XSS attacks in href', function() { + // "XSS" gets interpreted as a relative link (http) var textCases = [ 'Subtitle', 'Subtitle' @@ -130,8 +133,28 @@ describe('svg+text utils', function() { var node = mockTextSVGElement(textCase); expect(node.text()).toEqual('Subtitle'); - assertAnchorAttrs(node); - assertAnchorLink(node, 'XSS onmouseover=alert(1) style=font-size:300px'); + assertAnchorAttrs(node, 'font-size:300px'); + assertAnchorLink(node, 'XSS'); + }); + }); + + it('accepts href and style in in any order and tosses other stuff', function() { + var textCases = [ + 'z', + 'z', + 'z', + 'z', + 'z', + 'z', + 'z', + ]; + + textCases.forEach(function(textCase) { + var node = mockTextSVGElement(textCase); + + expect(node.text()).toEqual('z'); + assertAnchorAttrs(node, 'y'); + assertAnchorLink(node, 'x'); }); }); From 4e4b8c83015fcccf191a21152184fca7032128cf Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 12 May 2017 15:38:11 -0400 Subject: [PATCH 3/5] make the whole annotation box a link if that's all the text contains --- src/components/annotations/draw.js | 12 +++++++++ test/jasmine/tests/annotations_test.js | 34 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/components/annotations/draw.js b/src/components/annotations/draw.js index 25619fa9ebc..1c149d89741 100644 --- a/src/components/annotations/draw.js +++ b/src/components/annotations/draw.js @@ -188,6 +188,18 @@ function drawOne(gd, index) { } function drawGraphicalElements() { + // if the text has *only* a link, make the whole box into a link + var anchor = annText.selectAll('a'); + if(anchor.size() === 1 && anchor.text() === annText.text()) { + var wholeLink = annTextGroupInner.insert('a', ':first-child').attr({ + 'xlink:xlink:href': anchor.attr('xlink:href'), + 'xlink:xlink:show': anchor.attr('xlink:show') + }) + .style({cursor: 'pointer'}); + + wholeLink.node().appendChild(annTextBG.node()); + } + // make sure lines are aligned the way they will be // at the end, even if their position changes diff --git a/test/jasmine/tests/annotations_test.js b/test/jasmine/tests/annotations_test.js index edaf854741b..6a9d1c846c2 100644 --- a/test/jasmine/tests/annotations_test.js +++ b/test/jasmine/tests/annotations_test.js @@ -1251,4 +1251,38 @@ describe('annotation effects', function() { .catch(failTest) .then(done); }); + + it('makes the whole text box a link if the link is the whole text', function(done) { + makePlot([ + {x: 20, y: 20, text: 'Plot', showarrow: false}, + {x: 50, y: 50, text: 'or not', showarrow: false}, + {x: 80, y: 80, text: 'arrow'}, + {x: 20, y: 80, text: 'nor this'} + ]) + .then(function() { + function checkBoxLink(index, isLink) { + var boxLink = d3.selectAll('.annotation[data-index="' + index + '"] g>a'); + expect(boxLink.size()).toBe(isLink ? 1 : 0); + + var textLink = d3.selectAll('.annotation[data-index="' + index + '"] text a'); + expect(textLink.size()).toBe(1); + checkLink(textLink); + + if(isLink) checkLink(boxLink); + } + + function checkLink(link) { + expect(link.style('cursor')).toBe('pointer'); + expect(link.attr('xlink:href')).toBe('https://plot.ly'); + expect(link.attr('xlink:show')).toBe('new'); + } + + checkBoxLink(0, true); + checkBoxLink(1, false); + checkBoxLink(2, true); + checkBoxLink(3, false); + }) + .catch(failTest) + .then(done); + }); }); From 62ef3d7ce84fbfa9ff5df3c24a3ab857044e07a5 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 12 May 2017 18:44:50 -0400 Subject: [PATCH 4/5] fix subscript/superscript problem I just created and test these as well as tag auto-close/open --- src/lib/svg_text_utils.js | 21 +++++++--- test/jasmine/tests/svg_text_utils_test.js | 51 ++++++++++++++++++++++- 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/src/lib/svg_text_utils.js b/src/lib/svg_text_utils.js index 6e92555a318..e63e6485ca9 100644 --- a/src/lib/svg_text_utils.js +++ b/src/lib/svg_text_utils.js @@ -264,9 +264,11 @@ var SPLIT_TAGS = /(<[^<>]*>)/; var ONE_TAG = /<(\/?)([^ >]*)(\s+(.*))?>/i; -var QUOTEDATTR = '\\s*=\\s*("([^"]*)"|\'([^\']*)\')'; -var STYLEMATCH = new RegExp('(^|[\\s"\'])style' + QUOTEDATTR, 'i'); -var HREFMATCH = new RegExp('(^|[\\s"\'])href' + QUOTEDATTR, 'i'); +// Style and href: pull them out of either single or double quotes. +// 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 +var STYLEMATCH = /(^|[\s"'])style\s*=\s*("([^"]*);?"|'([^']*);?')/i; +var HREFMATCH = /(^|[\s"'])href\s*=\s*("([^"]*)"|'([^']*)')/i; var COLORMATCH = /(^|;)\s*color:/; @@ -362,9 +364,11 @@ function convertToSVG(_str) { // but font color is different (uses fill). Let our users ignore this. var cssMatch = extra && extra.match(STYLEMATCH); var css = cssMatch && (cssMatch[3] || cssMatch[4]); - if(css) css = encodeForHTML(css.replace(COLORMATCH, '$1 fill:')); - - if(tagStyle) css = tagStyle + ';' + (css || ''); + if(css) { + css = encodeForHTML(css.replace(COLORMATCH, '$1 fill:')); + if(tagStyle) css += ';' + tagStyle; + } + else if(tagStyle) css = tagStyle; if(css) return out + ' style="' + css + '">'; @@ -376,6 +380,11 @@ function convertToSVG(_str) { }); // now deal with line breaks + // TODO: this next section attempts to close and reopen tags that + // span a line break. But + // a) it only closes and reopens one tag, and + // b) all tags are treated like equivalent tspans (even which isn't a tspan even now!) + // we should really do this in a type-aware way *before* converting to tspans. var indices = []; for(var index = result.indexOf('
'); index > 0; index = result.indexOf('
', index + 1)) { indices.push(index); diff --git a/test/jasmine/tests/svg_text_utils_test.js b/test/jasmine/tests/svg_text_utils_test.js index afc187b7354..cacb9cfef0e 100644 --- a/test/jasmine/tests/svg_text_utils_test.js +++ b/test/jasmine/tests/svg_text_utils_test.js @@ -45,7 +45,11 @@ describe('svg+text utils', function() { expect(hasWrongAttr).toBe(false); - expect(a.attr('style')).toBe('cursor:pointer;' + (style || '')); + var fullStyle = style || ''; + if(style) fullStyle += ';'; + fullStyle += 'cursor:pointer'; + + expect(a.attr('style')).toBe(fullStyle); } function listAttributes(node) { @@ -226,5 +230,50 @@ describe('svg+text utils', function() { expect(node.text()).toEqual('100μ & < 10 > 0  100 × 20 ± 0.5 °'); }); + + it('supports superscript by itself', function() { + var node = mockTextSVGElement('123'); + expect(node.html()).toBe( + '​123' + + '​'); + }); + + it('supports subscript by itself', function() { + var node = mockTextSVGElement('123'); + expect(node.html()).toBe( + '​123' + + '​'); + }); + + it('supports superscript and subscript together with normal text', function() { + var node = mockTextSVGElement('SO42-'); + expect(node.html()).toBe( + 'SO​4' + + '​​' + + '2-' + + '​'); + }); + + it('allows one to span
s', function() { + var node = mockTextSVGElement('be Bold
and
Strong'); + expect(node.html()).toBe( + 'be ' + + 'Bold' + + '' + + 'and' + + '' + + '' + + 'Strong'); + }); + + it('allows one to span
s', function() { + var node = mockTextSVGElement('SO4
44'); + expect(node.html()).toBe( + 'SO​' + + '4' + + '​' + + '44' + + '​'); + }); }); }); From 1019d7ff60a1d362e3b56d53136f17bd403da7bd Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 12 May 2017 19:42:10 -0400 Subject: [PATCH 5/5] longer wait in sort transform interaction test --- test/jasmine/tests/transform_sort_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/transform_sort_test.js b/test/jasmine/tests/transform_sort_test.js index 6125eeadde3..b6f97ab793b 100644 --- a/test/jasmine/tests/transform_sort_test.js +++ b/test/jasmine/tests/transform_sort_test.js @@ -283,7 +283,7 @@ describe('Test sort transform interactions:', function() { function wait() { return new Promise(function(resolve) { - setTimeout(resolve, 60); + setTimeout(resolve, 100); }); }