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

tools/dom/src/Validators.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of dart.dom.html;
 
 
 /**
  * Interface used to validate that only accepted elements and attributes are
  * allowed while parsing HTML strings into DOM nodes.
@@ skipping 157 matching lines @@
       node.remove();
     } else {
       parent._removeChild(node);
     }
   }
   
   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}>');
+        // 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 {

[sra1, 2015/05/19 19:54:38]

Try to move try-catches to helper functions.
They inhibit optimizations in the entire containing function, both for dart2js
and then V8.

[Alan Knight, 2015/05/19 23:12:12]

Split it out into two additional functions, one which extracts the values in
try/catch blocks and it then calls the one that does the actual work, passing in
those values.

+          // If getting/indexing attributes throws, count that as corrupt.
+          attrs = element.attributes;
+          isAttr = attrs['is'];
+          corrupted = element._hasCorruptedAttributes;

[sra1, 2015/05/19 19:54:38]

It seems dangerous to store this information of element.
Can it be stored elsewhere?

[Alan Knight, 2015/05/19 21:54:55]

We're not storing it, it's a temp, and it's only used in the if statement below.
What do you mean?

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

Sorry, I thought that _hasCorruptedAttributes was a field.
I see it is a getter.

+        } catch(e) {}
+        var elementText = 'element unprintable';
+        try {
+          elementText = element.toString();
+        } catch(e) {}
+        var elementTagName = 'element tag unavailable';
+        try {
+          elementTagName = element.tagName;
+        } catch(e) {}
+        if (corrupted) {

[sra1, 2015/05/19 19:54:38]

corrupted could be anything from the assignment at 194.
Test
  if (false != corrupted) ..

[Alan Knight, 2015/05/19 23:12:12]

Done. But did it as (true = corrupted). I assume that's ok.

But how can it be anything if it's the result of calling a method with one
implementation which is return JS('bool', ...  ? We don't use the JS return
type?

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

Sorry, this again came from the misunderstanding that _hasCorruptedAttributes
was a field which could have any value.

It might be an idea to make _hasCorruptedAttributes be a static method to avoid
the need to dispatch (getInterceptor) to get to the code.

+          window.console.warn(
+              'Removing element due to corrupted attributes on <$elementText>');
           _removeNode(node, parent);
           break;
         }
-        var attrs = element.attributes;
         if (!validator.allowsElement(element)) {
           window.console.warn(
-              'Removing disallowed element <${element.tagName}>');
+              'Removing disallowed element <$elementTagName>');
           _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">');
+                '<$elementTagName 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];
@@ skipping 13 matching lines @@
       case Node.COMMENT_NODE:
       case Node.DOCUMENT_FRAGMENT_NODE:
       case Node.TEXT_NODE:
       case Node.CDATA_SECTION_NODE:
         break;
       default:
         _removeNode(node, parent);
     }
   }
 }