Sanitization should reject elements that we can't examine (e.g. embed/object on FF)

tools/dom/src/Validators.dart

Index: tools/dom/src/Validators.dart
diff --git a/tools/dom/src/Validators.dart b/tools/dom/src/Validators.dart
index ac51da13e6b62e1485283415d8d32f8647881883..7b3c71e614b579c342e6778694b14bc0828e22f9 100644
--- a/tools/dom/src/Validators.dart
+++ b/tools/dom/src/Validators.dart
@@ -170,51 +170,90 @@ class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
       parent._removeChild(node);
     }
   }
-  
+
+  /// Sanitize the element, assuming we can't trust anything about it.
+  void _sanitizeUntrustedElement(Element element, Node parent) {
+    // If the _hasCorruptedAttributes does not successfully return false,
+    // then we consider it corrupted and remove.
+    // TODO(alanknight): This is a workaround because on Firefox
+    // embed/object
+    // tags typeof is "function", not "object". We don't recognize them, and
+    // can't call methods. This does mean that you can't explicitly allow an
+    // embed tag. The only thing that will let it through is a null
+    // sanitizer that doesn't traverse the tree at all. But sanitizing while
+    // allowing embeds seems quite unlikely.
+    var corrupted = true;
+    var attrs;
+    var isAttr;
+    try {
+      // If getting/indexing attributes throws, count that as corrupt.
+      attrs = element.attributes;
+      isAttr = attrs['is'];
+      corrupted = element._hasCorruptedAttributes;
+    } catch(e) {}
+     var elementText = 'element unprintable';
+    try {
+      elementText = element.toString();
+    } catch(e) {}
+    var elementTagName = 'element tag unavailable';
+    try {
+      elementTagName = element.tagName;
+    } catch(e) {}
+    _sanitizeElement(element, parent, corrupted, elementText, elementTagName,
+        attrs, isAttr);
+  }
+
+  /// Having done basic sanity checking on the element, and computed the
+  /// important attributes we want to check, remove it if it's not valid
+  /// or not allowed, either as a whole or particular attributes.
+  void _sanitizeElement(Element element, Node parent, bool corrupted,
+      String text, String tag, Map attrs, String isAttr) {
+    if (true == corrupted) {
+      window.console.warn(
+          'Removing element due to corrupted attributes on <$text>');
+       _removeNode(element, parent);
+       return;
+    }
+    if (!validator.allowsElement(element)) {
+      window.console.warn(
+          'Removing disallowed element <$tag>');
+      _removeNode(element, parent);
+      return;
+    }
+
+    if (isAttr != null) {
+      if (!validator.allowsAttribute(element, 'is', isAttr)) {
+        window.console.warn('Removing disallowed type extension '
+            '<$tag is="$isAttr">');
+        _removeNode(element, parent);
+        return;
+      }
+    }
+
+    // TODO(blois): Need to be able to get all attributes, irrespective of
+    // XMLNS.
+    var keys = attrs.keys.toList();
+    for (var i = attrs.length - 1; i >= 0; --i) {

[sra1, 2015/05/19 23:45:02]

Why not keys.length?
Why do we go backwards?
It looks to me like one could just write:

for (var name in attrs.keys.toList()) {
  ...
}

[Alan Knight, 2015/05/21 20:38:41]

I don't know. But it makes me a little nervous that the code is so contrived.
There's either a subtle reason or it's just silly, but I don't know which, so
I'm reluctant to change it.

+      var name = keys[i];
+      if (!validator.allowsAttribute(element, name.toLowerCase(),
+          attrs[name])) {
+        window.console.warn('Removing disallowed attribute '
+            '<$tag $name="${attrs[name]}">');
+        attrs.remove(name);
+      }
+    }
+
+    if (element is TemplateElement) {
+      TemplateElement template = element;
+      sanitizeTree(template.content);
+     }
+  }
+
+  /// Sanitize the node and its children recursively.
   void sanitizeNode(Node node, Node parent) {
     switch (node.nodeType) {
       case Node.ELEMENT_NODE:
-        Element element = node;
-        if (element._hasCorruptedAttributes) {
-          window.console.warn('Removing element due to corrupted attributes on <${element}>');
-          _removeNode(node, parent);
-          break;
-        }
-        var attrs = element.attributes;
-        if (!validator.allowsElement(element)) {
-          window.console.warn(
-              'Removing disallowed element <${element.tagName}>');
-          _removeNode(node, parent);
-          break;
-        }
-
-        var isAttr = attrs['is'];
-        if (isAttr != null) {
-          if (!validator.allowsAttribute(element, 'is', isAttr)) {
-            window.console.warn('Removing disallowed type extension '
-                '<${element.tagName} is="$isAttr">');
-            _removeNode(node, parent);
-            break;
-          }
-        }
-
-        // TODO(blois): Need to be able to get all attributes, irrespective of
-        // XMLNS.
-        var keys = attrs.keys.toList();
-        for (var i = attrs.length - 1; i >= 0; --i) {
-          var name = keys[i];
-          if (!validator.allowsAttribute(element, name.toLowerCase(),
-              attrs[name])) {
-            window.console.warn('Removing disallowed attribute '
-                '<${element.tagName} $name="${attrs[name]}">');
-            attrs.remove(name);
-          }
-        }
-
-        if (element is TemplateElement) {
-          TemplateElement template = element;
-          sanitizeTree(template.content);
-        }
+        _sanitizeUntrustedElement(node, parent); 
         break;
       case Node.COMMENT_NODE:
       case Node.DOCUMENT_FRAGMENT_NODE: