fix: Fixed a config-dependent bypass caused by skipped attribute chec…

…ks, thanks @parrot409
cure53 · Dec 6, 2024 · c183cd6 · c183cd6
1 parent 6e76ece
commit c183cd6
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 19 deletions.
diff --git a/dist/purify.cjs.js b/dist/purify.cjs.js
diff --git a/dist/purify.es.mjs b/dist/purify.es.mjs
@@ -1147,6 +1147,8 @@ function createDOMPurify() {
     while (shadowNode = shadowIterator.nextNode()) {
       /* Execute a hook if present */
       _executeHooks(hooks.uponSanitizeShadowNode, shadowNode, null);
+      /* Check attributes first */
+      _sanitizeAttributes(shadowNode);
       /* Sanitize tags and elements */
       if (_sanitizeElements(shadowNode)) {
         continue;
@@ -1155,8 +1157,6 @@ function createDOMPurify() {
       if (shadowNode.content instanceof DocumentFragment) {
         _sanitizeShadowDOM(shadowNode.content);
       }
-      /* Check attributes, sanitize if necessary */
-      _sanitizeAttributes(shadowNode);
     }
     /* Execute a hook if present */
     _executeHooks(hooks.afterSanitizeShadowDOM, fragment, null);
@@ -1244,6 +1244,8 @@ function createDOMPurify() {
     const nodeIterator = _createNodeIterator(IN_PLACE ? dirty : body);
     /* Now start iterating over the created document */
     while (currentNode = nodeIterator.nextNode()) {
+      /* Check attributes first */
+      _sanitizeAttributes(currentNode);
       /* Sanitize tags and elements */
       if (_sanitizeElements(currentNode)) {
         continue;
@@ -1252,8 +1254,6 @@ function createDOMPurify() {
       if (currentNode.content instanceof DocumentFragment) {
         _sanitizeShadowDOM(currentNode.content);
       }
-      /* Check attributes, sanitize if necessary */
-      _sanitizeAttributes(currentNode);
     }
     /* If we sanitized `dirty` in-place, return it. */
     if (IN_PLACE) {

diff --git a/dist/purify.js b/dist/purify.js
diff --git a/dist/purify.min.js b/dist/purify.min.js
diff --git a/src/purify.ts b/src/purify.ts
@@ -1415,6 +1415,9 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
       /* Execute a hook if present */
       _executeHooks(hooks.uponSanitizeShadowNode, shadowNode, null);
 
+      /* Check attributes first */
+      _sanitizeAttributes(shadowNode);
+
       /* Sanitize tags and elements */
       if (_sanitizeElements(shadowNode)) {
         continue;
@@ -1424,9 +1427,6 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
       if (shadowNode.content instanceof DocumentFragment) {
         _sanitizeShadowDOM(shadowNode.content);
       }
-
-      /* Check attributes, sanitize if necessary */
-      _sanitizeAttributes(shadowNode);
     }
 
     /* Execute a hook if present */
@@ -1537,6 +1537,9 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
 
     /* Now start iterating over the created document */
     while ((currentNode = nodeIterator.nextNode())) {
+      /* Check attributes first */
+      _sanitizeAttributes(currentNode);
+
       /* Sanitize tags and elements */
       if (_sanitizeElements(currentNode)) {
         continue;
@@ -1546,9 +1549,6 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
       if (currentNode.content instanceof DocumentFragment) {
         _sanitizeShadowDOM(currentNode.content);
       }
-
-      /* Check attributes, sanitize if necessary */
-      _sanitizeAttributes(currentNode);
     }
 
     /* If we sanitized `dirty` in-place, return it. */

diff --git a/test/test-suite.js b/test/test-suite.js
@@ -2104,5 +2104,32 @@
       let clean = DOMPurify.sanitize(dirty, config);
       assert.contains(clean, expected);
     });
+
+    QUnit.test('Test proper handling of attributes with RETURN_DOM', function (assert) {
+      const dirty  = '<body onload="alert(1)">&lt;a<!-- <f --></body>';
+      const config = { 
+        RETURN_DOM: true 
+      };
+      const expected = '<body>&lt;a</body>';
+      let clean = DOMPurify.sanitize(dirty, config);
+
+      let iframe = document.createElement('iframe')
+      iframe.srcdoc = `<html><head></head>${clean.outerHTML}</html>`
+      document.body.appendChild(iframe); // alert test
+      assert.contains(clean.outerHTML, expected);
+    });
+
+    QUnit.test('Test proper handling of data-attribiutes in XML modes', function (assert) {
+      const dirty  = '<svg width="800" height="600" xmlns="http://www.w3.org/2000/svg"><a xmlns:data-slonser="http://www.w3.org/1999/xlink" data-slonser:href="javascript:alert(1)"><text  x="20" y="35">Click me!</text></a></svg>';
+      const config = { 
+        PARSER_MEDIA_TYPE: 'application/xhtml+xml'
+      };
+      const expected = [
+        '<svg xmlns=\"http://www.w3.org/2000/svg\" height=\"600\" width=\"800\"><a><text y=\"35\" x=\"20\">Click me!</text></a></svg>',
+        '<svg height=\"600\" width=\"800\" xmlns=\"http://www.w3.org/2000/svg\"><a><text y=\"35\" x=\"20\">Click me!</text></a></svg>'
+      ];
+      let clean = DOMPurify.sanitize(dirty, config);
+      assert.contains(clean, expected);
+    });
   };
 });