[dart:html] Fix sanitization for HTML templates

Bug: b/143778164

Resolves an issue where sanitization wasn't properly handled
when templates were involved.

Change-Id: Ic8f6f28036e18981eb934c2b39c2c0cd4e6f1a96
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195056
Reviewed-by: Sigmund Cherem <sigmund@google.com>

sdk/lib/html/dart2js/html_dart2js.dart

@@ -40994,8 +40994,8 @@ class _ThrowsNodeValidator implements NodeValidator {
 class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
   NodeValidator validator;
 
-  /// Did we modify the tree by removing anything.
-  bool modifiedTree = false;
+  /// Number of tree modifications this instance has made.
+  int numTreeModifications = 0;
   _ValidatingTreeSanitizer(this.validator) {}
 
   void sanitizeTree(Node node) {
@@ -41026,20 +41026,20 @@ class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
       }
     }
 
-    modifiedTree = false;
-    walk(node, null);
-    while (modifiedTree) {
-      modifiedTree = false;
+    // Walk the tree until no new modifications are added to the tree.
+    var previousTreeModifications;
+    do {
+      previousTreeModifications = numTreeModifications;
       walk(node, null);
-    }
+    } while (previousTreeModifications != numTreeModifications);
   }
 
   /// Aggressively try to remove node.
   void _removeNode(Node node, Node? parent) {
     // If we have the parent, it's presumably already passed more sanitization
     // or is the fragment, so ask it to remove the child. And if that fails
     // try to set the outer html.
-    modifiedTree = true;
+    numTreeModifications++;
     if (parent == null || parent != node.parentNode) {
       node.remove();
     } else {

tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart

@@ -453,6 +453,20 @@ main() {
             "<input id='bad' onmouseover='alert(1)'>",
         "");
 
+    // Walking templates triggers a recursive sanitization call, which shouldn't
+    // invalidate the information collected from the previous visit of the later
+    // nodes in the walk.
+    testHtml(
+        'DOM clobbering with recursive sanitize call using templates',
+        validator,
+        "<form><div>"
+            "<input id=childNodes />"
+            "<template></template>"
+            "<input id=childNodes name=lastChild />"
+            "<img id=exploitImg src=0 onerror='alert(1)' />"
+            "</div></form>",
+        "");
+
     test('tagName makes containing form invalid', () {
       var fragment = document.body!.createFragment(
           "<form onmouseover='alert(2)'><input name='tagName'>",

tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart

@@ -478,6 +478,20 @@ main() {
         "<input id='bad' onmouseover='alert(1)'>",
         "");
 
+    // Walking templates triggers a recursive sanitization call, which shouldn't
+    // invalidate the information collected from the previous visit of the later
+    // nodes in the walk.
+    testHtml(
+        'DOM clobbering with recursive sanitize call using templates',
+        validator,
+        "<form><div>"
+          "<input id=childNodes />"
+          "<template></template>"
+          "<input id=childNodes name=lastChild />"
+          "<img id=exploitImg src=0 onerror='alert(1)' />"
+          "</div></form>",
+        "");
+
     test('tagName makes containing form invalid', () {
       var fragment = document.body.createFragment(
           "<form onmouseover='alert(2)'><input name='tagName'>",

tools/dom/src/Validators.dart

@@ -158,8 +158,8 @@ class _ThrowsNodeValidator implements NodeValidator {
 class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
   NodeValidator validator;
 
-  /// Did we modify the tree by removing anything.
-  bool modifiedTree = false;
+  /// Number of tree modifications this instance has made.
+  int numTreeModifications = 0;
   _ValidatingTreeSanitizer(this.validator) {}
 
   void sanitizeTree(Node node) {
@@ -190,20 +190,20 @@ class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
       }
     }
 
-    modifiedTree = false;
-    walk(node, null);
-    while (modifiedTree) {
-      modifiedTree = false;
+    // Walk the tree until no new modifications are added to the tree.
+    var previousTreeModifications;
+    do {
+      previousTreeModifications = numTreeModifications;
       walk(node, null);
-    }
+    } while (previousTreeModifications != numTreeModifications);
   }
 
   /// Aggressively try to remove node.
   void _removeNode(Node node, Node? parent) {
     // If we have the parent, it's presumably already passed more sanitization
     // or is the fragment, so ask it to remove the child. And if that fails
     // try to set the outer html.
-    modifiedTree = true;
+    numTreeModifications++;
     if (parent == null || parent != node.parentNode) {
       node.remove();
     } else {