[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>
diff --git a/sdk/lib/html/dart2js/html_dart2js.dart b/sdk/lib/html/dart2js/html_dart2js.dart
index bc7bdfb..c60be00 100644
--- a/sdk/lib/html/dart2js/html_dart2js.dart
+++ b/sdk/lib/html/dart2js/html_dart2js.dart
@@ -40994,8 +40994,8 @@
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,12 +41026,12 @@
}
}
- 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.
@@ -41039,7 +41039,7 @@
// 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 {
diff --git a/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart b/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
index 5a3d0f2..2d7205c 100644
--- a/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
+++ b/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
@@ -453,6 +453,20 @@
"<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'>",
diff --git a/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart b/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
index 1b3ea60..ee31868 100644
--- a/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
+++ b/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
@@ -478,6 +478,20 @@
"<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'>",
diff --git a/tools/dom/src/Validators.dart b/tools/dom/src/Validators.dart
index 0c45b8c..1fbafbd 100644
--- a/tools/dom/src/Validators.dart
+++ b/tools/dom/src/Validators.dart
@@ -158,8 +158,8 @@
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,12 +190,12 @@
}
}
- 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.
@@ -203,7 +203,7 @@
// 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 {
