Replace angular-sanitize with DOMPurify

In preparation for the move away from AngularJS, replace
angular-sanitize with the DOMPurify library, which is a modern and
widely used HTML sanitization library with no framework dependency.

The sanitization logic is now encapsulated fully within the
`render-markdown` module which no longer takes a sanitization function
as an argument. That argument used to be necessary because
the angular-sanitize function had to be obtained via Angular's
dependency injection. DOMPurify can just be required as a module inside
render-markdown.js.

One improvement that is possible with DOMPurify but not done here is to
generate sanitized DOM nodes directly instead of having showdown
generate HTML, which is then parsed to DOM and sanitized, then converted
back to HTML and finally converted back to DOM by Angular.

Fixes #975

package.json

@@ -13,7 +13,6 @@
     "angular": "^1.7.5",
     "angular-mocks": "^1.7.5",
     "angular-route": "^1.7.5",
-    "angular-sanitize": "^1.7.5",
     "angular-toastr": "^2.1.1",
     "angulartics": "0.17.2",
     "autofill-event": "0.0.1",
@@ -44,6 +43,7 @@
     "dom-anchor-text-quote": "^4.0.2",
     "dom-node-iterator": "^3.5.3",
     "dom-seek": "^4.0.3",
+    "dompurify": "^1.0.10",
     "end-of-stream": "^1.1.0",
     "escape-html": "^1.0.3",
     "escape-string-regexp": "^1.0.5",

scripts/gulp/vendor-bundles.js

@@ -12,7 +12,6 @@ module.exports = {
     angular: [
       'angular',
       'angular-route',
-      'angular-sanitize',
       'ng-tags-input',
       'angular-toastr',
       'angulartics/src/angulartics',

src/sidebar/components/markdown.js

@@ -8,7 +8,7 @@ const renderMarkdown = require('../render-markdown');
 const scopeTimeout = require('../util/scope-timeout');
 
 // @ngInject
-function MarkdownController($element, $sanitize, $scope) {
+function MarkdownController($element, $scope) {
   const input = $element[0].querySelector('.js-markdown-input');
   const output = $element[0].querySelector('.js-markdown-preview');
 
@@ -140,7 +140,7 @@ function MarkdownController($element, $sanitize, $scope) {
 
   // Re-render the markdown when the view needs updating.
   $scope.$watch('vm.text', function() {
-    output.innerHTML = renderMarkdown(self.text || '', $sanitize);
+    output.innerHTML = renderMarkdown(self.text || '');
     mediaEmbedder.replaceLinksWithEmbeds(output);
   });
 

src/sidebar/components/test/markdown-test.js

@@ -40,7 +40,7 @@ describe('markdown', function() {
   }
 
   before(function() {
-    angular.module('app', ['ngSanitize']).component(
+    angular.module('app').component(
       'markdown',
       proxyquire(
         '../markdown',
@@ -57,8 +57,8 @@ describe('markdown', function() {
               fn();
             };
           },
-          '../render-markdown': noCallThru(function(markdown, $sanitize) {
-            return $sanitize('rendered:' + markdown);
+          '../render-markdown': noCallThru(function(markdown) {
+            return 'rendered:' + markdown;
           }),
 
           '../markdown-commands': {
@@ -118,14 +118,6 @@ describe('markdown', function() {
       assert.equal(getRenderedHTML(editor), 'rendered:');
     });
 
-    it('should sanitize the result', function() {
-      const editor = util.createDirective(document, 'markdown', {
-        readOnly: true,
-        text: 'Hello <script>alert("attack");</script> World',
-      });
-      assert.equal(getRenderedHTML(editor), 'rendered:Hello  World');
-    });
-
     it('should replace links with embeds in rendered output', function() {
       const editor = util.createDirective(document, 'markdown', {
         readOnly: true,

src/sidebar/index.js

@@ -107,7 +107,6 @@ function startAngularApp(config) {
       // Angular addons which export the Angular module name
       // via module.exports
       require('angular-route'),
-      require('angular-sanitize'),
       require('angular-toastr'),
 
       // Angular addons which do not export the Angular module

src/sidebar/render-markdown.js

 'use strict';
 
+const createDOMPurify = require('dompurify');
 const escapeHtml = require('escape-html');
 const katex = require('katex');
 const showdown = require('showdown');
 
+const DOMPurify = createDOMPurify(window);
+
+// Ensure that any links generated either by Showdown or in the markdown/HTML
+// passed to Showdown open in an external window.
+DOMPurify.addHook('afterSanitizeAttributes', node => {
+  if ('target' in node) {
+    node.setAttribute('target', '_blank');
+  }
+});
+
 function targetBlank() {
   function filter(text) {
     return text.replace(/<a href=/g, '<a target="_blank" href=');
@@ -120,13 +131,13 @@ function insertMath(html, mathBlocks) {
   }, html);
 }
 
-function renderMathAndMarkdown(markdown, sanitizeFn) {
+function renderMathAndMarkdown(markdown) {
   // KaTeX takes care of escaping its input, so we want to avoid passing its
   // output through the HTML sanitizer. Therefore we first extract the math
   // blocks from the input, render and sanitize the remaining markdown and then
   // render and re-insert the math blocks back into the output.
   const mathInfo = extractMath(markdown);
-  const markdownHTML = sanitizeFn(renderMarkdown(mathInfo.content));
+  const markdownHTML = DOMPurify.sanitize(renderMarkdown(mathInfo.content));
   const mathAndMarkdownHTML = insertMath(markdownHTML, mathInfo.mathBlocks);
   return mathAndMarkdownHTML;
 }

src/sidebar/test/bootstrap.js

@@ -13,4 +13,3 @@ sinon.assert.expose(assert, { prefix: null });
 window.jQuery = window.$ = require('jquery');
 require('angular');
 require('angular-mocks');
-require('angular-sanitize');

src/sidebar/test/render-markdown-test.js

@@ -6,14 +6,6 @@ describe('render-markdown', function() {
   let render;
   let renderMarkdown;
 
-  function fakeSanitize(html) {
-    return '{safe}' + html + '{/safe}';
-  }
-
-  function noopSanitize(html) {
-    return html;
-  }
-
   beforeEach(function() {
     renderMarkdown = proxyquire('../render-markdown', {
       katex: {
@@ -26,9 +18,8 @@ describe('render-markdown', function() {
         },
       },
     });
-    render = function(markdown, sanitizeFn) {
-      sanitizeFn = sanitizeFn || noopSanitize;
-      return renderMarkdown(markdown, sanitizeFn);
+    render = function(markdown) {
+      return renderMarkdown(markdown);
     };
   });
 
@@ -36,7 +27,7 @@ describe('render-markdown', function() {
     it('should autolink URLs', function() {
       assert.equal(
         render('See this link - http://arxiv.org/article'),
-        '<p>See this link - <a target="_blank" href="http://arxiv.org/article">' +
+        '<p>See this link - <a href="http://arxiv.org/article" target="_blank">' +
           'http://arxiv.org/article</a></p>'
       );
     });
@@ -46,8 +37,9 @@ describe('render-markdown', function() {
         render(
           'See this https://hypothes.is/stream?q=tag:group_test_needs_card'
         ),
-        '<p>See this <a target="_blank" ' +
-          'href="https://hypothes.is/stream?q=tag:group_test_needs_card">' +
+        '<p>See this <a ' +
+          'href="https://hypothes.is/stream?q=tag:group_test_needs_card" ' +
+          'target="_blank">' +
           'https://hypothes.is/stream?q=tag:group_test_needs_card</a></p>'
       );
     });
@@ -62,9 +54,19 @@ describe('render-markdown', function() {
     });
 
     it('should sanitize the result', function() {
+      // Check that the rendered HTML is fed through the HTML sanitization
+      // library. This is not an extensive test of sanitization behavior, that
+      // is left to DOMPurify's tests.
       assert.equal(
-        renderMarkdown('one **two** three', fakeSanitize),
-        '{safe}<p>one <strong>two</strong> three</p>{/safe}'
+        renderMarkdown('one **two** <script>alert("three")</script>'),
+        '<p>one <strong>two</strong> </p>'
+      );
+    });
+
+    it('should open links in a new window', () => {
+      assert.equal(
+        renderMarkdown('<a href="http://example.com">test</a>'),
+        '<p><a href="http://example.com" target="_blank">test</a></p>'
       );
     });
   });
@@ -83,13 +85,10 @@ describe('render-markdown', function() {
     });
 
     it('should not sanitize math renderer output', function() {
-      const fakeSanitize = function(html) {
-        return html.toLowerCase();
-      };
-      assert.equal(
-        render('$$X*2$$ FOO', fakeSanitize),
-        '<p>math+display:X*2</p>\n<p>foo</p>'
-      );
+      // Check that KaTeX's rendered output is not corrupted in any way by
+      // sanitization.
+      const html = render('$$ <unknown-tag>foo</unknown-tag> $$');
+      assert.include(html, '<unknown-tag>foo</unknown-tag>');
     });
 
     it('should render mixed inline and block math', function() {

yarn.lock

@@ -810,11 +810,6 @@ angular-route@^1.7.5:
   resolved "https://registry.yarnpkg.com/angular-route/-/angular-route-1.7.7.tgz#0573c8efa87e735bcd4f64f6551a176faca3b6d1"
   integrity sha512-nshsCti751h8GfvYQ2nlTbQnaY/q5zvxLctj78sS5SjB8iRDqNbT1NVNz8B/nDQUudjc7etE7xYaTdM8idzq6g==
 
-angular-sanitize@^1.7.5:
-  version "1.7.7"
-  resolved "https://registry.yarnpkg.com/angular-sanitize/-/angular-sanitize-1.7.7.tgz#5852592ea885d48db1f2718a3ec7b6e3330950d6"
-  integrity sha512-DZ0kidLOOrq/mbqwKbeT2aSlT2Kdy25tZh5KcWOrRXx26imbhQfwbuncMYMFh7uPqfWySAI534+XhjnmhJ626Q==
-
 angular-toastr@^2.1.1:
   version "2.1.1"
   resolved "https://registry.yarnpkg.com/angular-toastr/-/angular-toastr-2.1.1.tgz#9f8350ca482145a44d011a755b8fb3623d60544c"
@@ -3112,6 +3107,11 @@ domain-browser@^1.2.0:
   resolved "https://registry.yarnpkg.com/domain-browser/-/domain-browser-1.2.0.tgz#3d31f50191a6749dd1375a7f522e823d42e54eda"
   integrity sha512-jnjyiM6eRyZl2H+W8Q/zLMA481hzi0eszAaBUzIVnmYVDBbnLxVNnfu1HgEBvCbL+71FrxMl3E6lpKH7Ge3OXA==
 
+dompurify@^1.0.10:
+  version "1.0.10"
+  resolved "https://registry.yarnpkg.com/dompurify/-/dompurify-1.0.10.tgz#18d7353631c86ee25049e38fbca8c6b2c5a2af87"
+  integrity sha512-huhl3DSWX5LaA7jDtnj3XQdJgWW1wYouNW7N0drGzQa4vEUSVWyeFN+Atx6HP4r5cang6oQytMom6I4yhGJj5g==
+
 dot-parts@~1.0.0:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/dot-parts/-/dot-parts-1.0.1.tgz#884bd7bcfc3082ffad2fe5db53e494d8f3e0743f"