Cure53 fix hook detach (#1523)

* * feat: block declarative-partial-updates patch directives
* feat: harden style mXSS check against foreign-content teleport
* fefend against declarative partial updates (Chrome 150)
* test: add tests for partial-update patch directives

* feat: loosened the style check a bit again
test: added correct test coverage for template scrubbing

* fix: slightly changed behavior of node removal to avoid breakage, see #1521

* chore: regenerated dist files to be in sync again

Author: cure53

dist/purify.cjs.js

@@ -1551,9 +1551,15 @@ function createDOMPurify() {
    * @param currentNode to check for permission to exist
    * @return true if node was killed, false if left alive
    */
-  const _sanitizeElements = function _sanitizeElements(currentNode) {
+  // eslint-disable-next-line complexity
+  const _sanitizeElements = function _sanitizeElements(currentNode, root) {
     /* Execute a hook if present */
     _executeHooks(hooks.beforeSanitizeElements, currentNode, null);
+    /* A hook may have detached the node — treat it as removed (see the
+       detached-node comment after the uponSanitizeElement hook below). */
+    if (currentNode !== root && getParentNode(currentNode) === null) {
+      return true;
+    }
     /* Check if element is clobbered or can clobber */
     if (_isClobbered(currentNode)) {
       _forceRemove(currentNode);
@@ -1566,6 +1572,24 @@ function createDOMPurify() {
       tagName,
       allowedTags: ALLOWED_TAGS
     });
+    /* A hook may have detached the node from the tree — a long-standing
+       user pattern (issue #469; draw.io-style foreignObject filtering).
+       Per the cached, unclobberable parentNode getter the node is
+       genuinely out of the tree, so it can reach neither the serialized
+       output nor an IN_PLACE live tree; treat it as removed and stop
+       processing it. Without this guard, the unsafe-node / namespace
+       checks below would call _forceRemove on a parentless node and hit
+       the REPORT-3 fail-closed throw — which exists for nodes DOMPurify
+       wants gone but *cannot* detach (clobbered / parentless roots), the
+       opposite of a node that is already safely gone. The walk root is
+       exempt: a detached IN_PLACE root is legitimate input and must still
+       be fully sanitized, and a kill-decision on it must keep hitting the
+       REPORT-3 throw. Nodes detached by hooks are the hook's
+       responsibility: they are not recorded in DOMPurify.removed and are
+       not neutralized by the post-walk IN_PLACE pass. */
+    if (currentNode !== root && getParentNode(currentNode) === null) {
+      return true;
+    }
     /* Remove mXSS vectors, processing instructions and risky comments */
     if (_isUnsafeNode(currentNode, tagName)) {
       _forceRemove(currentNode);
@@ -1847,7 +1871,7 @@ function createDOMPurify() {
       /* Execute a hook if present */
       _executeHooks(hooks.uponSanitizeShadowNode, shadowNode, null);
       /* Sanitize tags and elements */
-      _sanitizeElements(shadowNode);
+      _sanitizeElements(shadowNode, fragment);
       /* Check attributes next */
       _sanitizeAttributes(shadowNode);
       /* Deep shadow DOM detected.
@@ -2113,7 +2137,8 @@ function createDOMPurify() {
       _forceRemove(body.firstChild);
     }
     /* Get node iterator */
-    const nodeIterator = _createNodeIterator(inPlace ? dirty : body);
+    const walkRoot = inPlace ? dirty : body;
+    const nodeIterator = _createNodeIterator(walkRoot);
     /* Now start iterating over the created document.
        The walk runs inside an exception barrier (campaign-3 F2): a re-entrant
        engine/custom-element mutation can detach a node mid-walk so
@@ -2126,7 +2151,7 @@ function createDOMPurify() {
     try {
       while (currentNode = nodeIterator.nextNode()) {
         /* Sanitize tags and elements */
-        _sanitizeElements(currentNode);
+        _sanitizeElements(currentNode, walkRoot);
         /* Check attributes next */
         _sanitizeAttributes(currentNode);
         /* Shadow DOM detected, sanitize it.

dist/purify.cjs.js.map

@@ -1748,10 +1748,17 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
    * @param currentNode to check for permission to exist
    * @return true if node was killed, false if left alive
    */
-  const _sanitizeElements = function (currentNode: any): boolean {
+  // eslint-disable-next-line complexity
+  const _sanitizeElements = function (currentNode: any, root: Node): boolean {
     /* Execute a hook if present */
     _executeHooks(hooks.beforeSanitizeElements, currentNode, null);
 
+    /* A hook may have detached the node — treat it as removed (see the
+       detached-node comment after the uponSanitizeElement hook below). */
+    if (currentNode !== root && getParentNode(currentNode) === null) {
+      return true;
+    }
+
     /* Check if element is clobbered or can clobber */
     if (_isClobbered(currentNode)) {
       _forceRemove(currentNode);
@@ -1769,6 +1776,25 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
       allowedTags: ALLOWED_TAGS,
     });
 
+    /* A hook may have detached the node from the tree — a long-standing
+       user pattern (issue #469; draw.io-style foreignObject filtering).
+       Per the cached, unclobberable parentNode getter the node is
+       genuinely out of the tree, so it can reach neither the serialized
+       output nor an IN_PLACE live tree; treat it as removed and stop
+       processing it. Without this guard, the unsafe-node / namespace
+       checks below would call _forceRemove on a parentless node and hit
+       the REPORT-3 fail-closed throw — which exists for nodes DOMPurify
+       wants gone but *cannot* detach (clobbered / parentless roots), the
+       opposite of a node that is already safely gone. The walk root is
+       exempt: a detached IN_PLACE root is legitimate input and must still
+       be fully sanitized, and a kill-decision on it must keep hitting the
+       REPORT-3 throw. Nodes detached by hooks are the hook's
+       responsibility: they are not recorded in DOMPurify.removed and are
+       not neutralized by the post-walk IN_PLACE pass. */
+    if (currentNode !== root && getParentNode(currentNode) === null) {
+      return true;
+    }
+
     /* Remove mXSS vectors, processing instructions and risky comments */
     if (_isUnsafeNode(currentNode, tagName)) {
       _forceRemove(currentNode);
@@ -2210,7 +2236,7 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
       _executeHooks(hooks.uponSanitizeShadowNode, shadowNode, null);
 
       /* Sanitize tags and elements */
-      _sanitizeElements(shadowNode);
+      _sanitizeElements(shadowNode, fragment);
 
       /* Check attributes next */
       _sanitizeAttributes(shadowNode);
@@ -2514,7 +2540,8 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
     }
 
     /* Get node iterator */
-    const nodeIterator = _createNodeIterator(inPlace ? dirty : body);
+    const walkRoot: Node = inPlace ? (dirty as Node) : body;
+    const nodeIterator = _createNodeIterator(walkRoot);
 
     /* Now start iterating over the created document.
        The walk runs inside an exception barrier (campaign-3 F2): a re-entrant
@@ -2528,7 +2555,7 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
     try {
       while ((currentNode = nodeIterator.nextNode())) {
         /* Sanitize tags and elements */
-        _sanitizeElements(currentNode);
+        _sanitizeElements(currentNode, walkRoot);
 
         /* Check attributes next */
         _sanitizeAttributes(currentNode);

dist/purify.es.mjs

@@ -3254,6 +3254,53 @@
 
     QUnit.module('Hooks — addHook / removeHook');
 
+    QUnit.test(
+      'uponSanitizeElement hook may detach the current node; it is treated as removed (issue #469 pattern, foreignObject filtering)',
+      (assert) => {
+        // A hook that removes the current node via parentNode.removeChild()
+        // must not trip the REPORT-3 parentless-node throw in _forceRemove:
+        // the throw exists for nodes DOMPurify wants gone but cannot detach,
+        // not for nodes a hook has already safely detached.
+        DOMPurify.addHook('uponSanitizeElement', (node) => {
+          if (node.nodeName === 'foreignObject' && node.parentNode) {
+            node.parentNode.removeChild(node);
+          }
+        });
+        const clean = DOMPurify.sanitize(
+          '<svg><switch><foreignObject><div>content</div></foreignObject></switch></svg>',
+          {
+            ADD_TAGS: ['foreignObject'],
+            USE_PROFILES: { svg: true, html: true },
+          }
+        );
+        assert.equal(clean, '<svg><switch></switch></svg>');
+        assert.notOk(
+          DOMPurify.removed.some(
+            (entry) =>
+              entry.element && entry.element.nodeName === 'foreignObject'
+          ),
+          'hook-detached nodes are not claimed in DOMPurify.removed'
+        );
+        DOMPurify.removeHooks('uponSanitizeElement');
+      }
+    );
+
+    QUnit.test(
+      'beforeSanitizeElements hook may detach the current node; it is treated as removed',
+      (assert) => {
+        DOMPurify.addHook('beforeSanitizeElements', (node) => {
+          if (node.nodeName === 'ARTICLE' && node.parentNode) {
+            node.parentNode.removeChild(node);
+          }
+        });
+        assert.equal(
+          DOMPurify.sanitize('<div><article><b>x</b></article><i>y</i></div>'),
+          '<div><i>y</i></div>'
+        );
+        DOMPurify.removeHooks('beforeSanitizeElements');
+      }
+    );
+
     QUnit.test(
       'hook can add allowed tags / attributes on the fly',
       (assert) => {