Merge pull request #1888 from plotly/popup-fix

safer construction of popup click handler

Author: alexcjohnson

src/lib/svg_text_utils.js

@@ -258,7 +258,7 @@ var BR_TAG = /<br(\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*("([^"\s]*)"|'([^'\s]*)')/i;
+var POPUPMATCH = /(^|[\s"'])popup\s*=\s*("([\w=,]*)"|'([\w=,]*)')/i;
 
 // dedicated matcher for these quoted regexes, that can return their results
 // in two different places
@@ -360,7 +360,9 @@ function buildSVGText(containerNode, str) {
                     'xlink:xlink:href': href
                 };
                 if(popup) {
-                    nodeAttrs.onclick = 'window.open("' + href + '","' + target + '","' +
+                    // security: href and target are not inserted as code but
+                    // as attributes. popup is, but limited to /[A-Za-z0-9_=,]/
+                    nodeAttrs.onclick = 'window.open(this.href.baseVal,this.target.baseVal,"' +
                         popup + '");return false;';
                 }
             }
@@ -459,7 +461,7 @@ function buildSVGText(containerNode, str) {
                         var dummyAnchor = document.createElement('a');
                         dummyAnchor.href = href;
                         if(PROTOCOLS.indexOf(dummyAnchor.protocol) !== -1) {
-                            nodeSpec.href = href;
+                            nodeSpec.href = encodeURI(href);
                             nodeSpec.target = getQuotedMatch(extra, TARGETMATCH) || '_blank';
                             nodeSpec.popup = getQuotedMatch(extra, POPUPMATCH);
                         }

test/jasmine/tests/svg_text_utils_test.js

@@ -190,7 +190,38 @@ describe('svg+text utils', function() {
         it('attaches onclick if popup is specified', function() {
             var node = mockTextSVGElement('<a href="x" target="fred" popup="width=500,height=400">link</a>');
             assertAnchorLink(node, 'x', 'fred', 'new');
-            assertAnchorAttrs(node, {onclick: 'window.open("x","fred","width=500,height=400");return false;'});
+            assertAnchorAttrs(node, {onclick: 'window.open(this.href.baseVal,this.target.baseVal,"width=500,height=400");return false;'});
+        });
+
+        it('drops XSS attacks via popup script', function() {
+            var textCases = [
+                [
+                    '<a href=\'#\' target=\'b\' popup=\'1");alert(document.cookie);//\'>XSS</a>',
+                    '#', 'b', null
+                ],
+                [
+                    '<a href=\'#\' target=\'b");alert(document.cookie);//\' popup=\'1\'>XSS</a>',
+                    '#', 'b");alert(document.cookie);//', '1'
+                ],
+                [
+                    '<a href=\'#");alert(document.cookie);//\' target=\'b\' popup=\'1\'>XSS</a>',
+                    '#%22);alert(document.cookie);//', 'b', '1'
+                ]
+            ];
+
+            textCases.forEach(function(textCase) {
+                var node = mockTextSVGElement(textCase[0]);
+
+                var attrs = {};
+                if(textCase[3]) {
+                    attrs.onclick = 'window.open(this.href.baseVal,this.target.baseVal,"' +
+                        textCase[3] + '");return false;';
+                }
+
+                expect(node.text()).toEqual('XSS');
+                assertAnchorAttrs(node, attrs, textCase[0]);
+                assertAnchorLink(node, textCase[1], textCase[2], 'new', textCase[0]);
+            });
         });
 
         it('keeps query parameters in href', function() {