Refactor and extend to version 0.2

bustle · Nov 21, 2016 · b89d508 · b89d508
1 parent 1a98c0e
commit b89d508
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 21 deletions.
diff --git a/lib/renderers/0-2.js b/lib/renderers/0-2.js
@@ -13,6 +13,9 @@ import {
   isValidMarkerType
 } from '../utils/tag-names';
 import kvReduce from '../utils/kv-reduce';
+import {
+  isSafeMarker
+} from '../utils/sanitization-utils';
 
 export const MOBILEDOC_VERSION = '0.2.0';
 
@@ -156,7 +159,7 @@ export default class Renderer {
       for (let j=0, m=openTypes.length; j<m; j++) {
         let markerType = this.markerTypes[openTypes[j]];
         let [tagName, attrs=[]] = markerType;
-        if (isValidMarkerType(tagName)) {
+        if (isValidMarkerType(tagName) && isSafeMarker(tagName, attrs)) {
           let lowerCaseTagName = tagName.toLowerCase();
           if (this.markupElementRenderer[lowerCaseTagName]) {
             let attrObj = attrs.reduce(kvReduce, {});

diff --git a/lib/renderers/0-3.js b/lib/renderers/0-3.js
@@ -14,7 +14,7 @@ import {
 } from '../utils/tag-names';
 import kvReduce from '../utils/kv-reduce';
 import {
-  isUnsafeUri
+  isSafeMarker
 } from '../utils/sanitization-utils';
 
 import {
@@ -46,19 +46,6 @@ function validateVersion(version) {
   }
 }
 
-function isSafe(tagName, attrs) {
-  if (tagName === 'a') {
-    let attrObj = attrs.reduce(kvReduce, {});
-
-    let href = attrObj.href;
-    if (href.length > 0 && isUnsafeUri(href)) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 export default class Renderer {
   constructor(mobiledoc, state) {
 
@@ -187,7 +174,7 @@ export default class Renderer {
       for (let j=0, m=openTypes.length; j<m; j++) {
         let markerType = this.markerTypes[openTypes[j]];
         let [tagName, attrs=[]] = markerType;
-        if (isValidMarkerType(tagName) && isSafe(tagName, attrs)) {
+        if (isValidMarkerType(tagName) && isSafeMarker(tagName, attrs)) {
           let lowerCaseTagName = tagName.toLowerCase();
           if (this.markupElementRenderer[lowerCaseTagName]) {
             let attrObj = attrs.reduce(kvReduce, {});

diff --git a/lib/utils/sanitization-utils.js b/lib/utils/sanitization-utils.js
@@ -1,5 +1,20 @@
+import kvReduce from '../utils/kv-reduce';
+
 const UNSAFE_URI_REGEX = /^(javascript:|vbscript:)/;
 
-export function isUnsafeUri(s) {
+function isUnsafeUri(s) {
   return UNSAFE_URI_REGEX.test(s);
 }
+
+export function isSafeMarker(tagName, attrs) {
+  if (tagName === 'a') {
+    let attrObj = attrs.reduce(kvReduce, {});
+
+    let href = attrObj.href;
+    if (href.length > 0 && isUnsafeUri(href)) {
+      return false;
+    }
+  }
+
+  return true;
+}
diff --git a/tests/unit/renderers/0-2-test.js b/tests/unit/renderers/0-2-test.js
@@ -535,6 +535,26 @@ test('XSS: unexpected markup types are not rendered', (assert) => {
   assert.ok(content.indexOf('script') === -1, 'no script tag rendered');
 });
 
+test('XSS: links with dangerous href values are not rendered', (assert) => {
+  let mobiledoc = {
+    version: MOBILEDOC_VERSION,
+    sections: [
+      [
+        ["a", [ "href", "javascript:alert('XSS')" ]] // jshint ignore: line
+      ],
+      [
+        [MARKUP_SECTION_TYPE, "p", [
+          [[0], 1, "link text"],
+          [[], 0, "plain text"]]
+        ]
+      ]
+    ]
+  };
+  let { result } = renderer.render(mobiledoc);
+  let content = outerHTML(result);
+  assert.equal(content, "<p>link textplain text</p>");
+});
+
 test('renders a mobiledoc with sectionElementRenderer', (assert) => {
   let mobiledoc = {
     version: MOBILEDOC_VERSION,

diff --git a/tests/unit/utils/sanitization-utils-test.js b/tests/unit/utils/sanitization-utils-test.js
@@ -1,20 +1,22 @@
 /* global QUnit */
 
-import { isUnsafeUri } from 'mobiledoc-dom-renderer/utils/sanitization-utils';
+import {
+  isSafeMarker
+} from 'mobiledoc-dom-renderer/utils/sanitization-utils';
 
 const { test, module } = QUnit;
 
 module('Unit: Mobiledoc DOM Renderer - Sanitization utils');
 
-test('#isUnsafeUri', (assert) => {
+test('#isSafeMarker - a', (assert) => {
   let unsafe = [
     'javascript:alert("XSS")', // jshint ignore: line
     'vbscript:alert("XSS")' // jshint ignore: line
   ];
 
   for (let i = 0; i < unsafe.length; i++) {
     let uri = unsafe[i];
-    assert.ok(isUnsafeUri(uri), `${uri} should be unsafe`);
+    assert.ok(! isSafeMarker('a', ['href', uri]), `${uri} should be unsafe`);
   }
 
   let safe = [
@@ -29,6 +31,6 @@ test('#isUnsafeUri', (assert) => {
 
   for (let i = 0; i < safe.length; i++) {
     let uri = safe[i];
-    assert.ok(! isUnsafeUri(uri), `${uri} should be safe`);
+    assert.ok(isSafeMarker('a', ['href', uri]), `${uri} should be safe`);
   }
 });