amp-mustache: Output user errors when elements/attributes are sanitiz…

…ed (ampproject#20285)

* Output user errors for elements/attrs sanitized by DOMPurify.

* Make sanitizer error format consistent.

* Add documentation with sanitization caveat.

* Increase timeout of test-amp-bind to 15s.

extensions/amp-mustache/amp-mustache.md

@@ -77,6 +77,8 @@ target AMP element that uses this template to render its content (for example, i
 
 ## Restrictions
 
+### Validation
+
 Like all AMP templates, `amp-mustache` templates are required to be well-formed DOM fragments. This means
 that among other things, you can't use `amp-mustache` to:
 
@@ -85,6 +87,14 @@ that among other things, you can't use `amp-mustache` to:
 
 The output of "triple-mustache" is sanitized to only allow the following tags: `a`, `b`, `br`, `caption`, `colgroup`, `code`, `del`, `div`, `em`, `i`, `ins`, `li`, `mark`, `ol`, `p`, `q`, `s`, `small`, `span`, `strong`, `sub`, `sup`, `table`, `tbody`, `time`, `td`, `th`, `thead`, `tfoot`, `tr`, `u`, `ul`.
 
+### Sanitization
+
+Mustache output is sanitized for security reasons, which may result in certain elements and attributes being removed.
+
+A console error will be output with details of the sanitized element or attribute. For example:
+
+> [PURIFIER] Removed unsafe attribute: href="javascript:alert"
+
 ## Pitfalls
 
 ### Nested templates

src/purifier.js

@@ -38,7 +38,7 @@ let DomPurifyDef;
 const DomPurify = purify(self);
 
 /** @private @const {string} */
-const TAG = 'purifier';
+const TAG = 'PURIFIER';
 
 /** @private @const {string} */
 const ORIGINAL_TARGET_VALUE = '__AMP_ORIGINAL_TARGET_VALUE_';
@@ -339,6 +339,7 @@ export function addPurifyHooks(purifier, diffing) {
 
   /**
    * @param {!Node} unusedNode
+   * @this {{removed: !Array}} Contains list of removed elements/attrs so far.
    */
   const afterSanitizeElements = function(unusedNode) {
     // DOMPurify doesn't have a attribute-specific tag whitelist API and
@@ -348,6 +349,19 @@ export function addPurifyHooks(purifier, diffing) {
       delete allowedTags[tag];
     });
     allowedTagsChanges.length = 0;
+
+    // Output user errors for each removed attribute or element.
+    this.removed.forEach(r => {
+      if (r.attribute) {
+        const {name, value} = r.attribute;
+        user().error(TAG, `Removed unsafe attribute: ${name}="${value}"`);
+      } else if (r.element) {
+        const {nodeName} = r.element;
+        if (nodeName !== 'REMOVE') { // <remove> is added by DOMPurify.
+          user().error(TAG, 'Removed unsafe element:', r.element.nodeName);
+        }
+      }
+    });
   };
 
   /**
@@ -427,8 +441,6 @@ export function addPurifyHooks(purifier, diffing) {
         attrValue = rewriteAttributeValue(tagName, attrName, attrValue);
       }
     } else {
-      user().error(TAG, `Removing "${attrName}" attribute with invalid `
-          + `value in <${tagName} ${attrName}="${attrValue}">.`);
       data.keepAttr = false;
     }
 

src/sanitizer.js

@@ -30,7 +30,7 @@ import {startsWith} from './string';
 import {user} from './log';
 
 /** @private @const {string} */
-const TAG = 'sanitizer';
+const TAG = 'SANITIZER';
 
 /**
  * Whitelist of supported self-closing tags for Caja. These are used for
@@ -206,8 +206,8 @@ export function sanitizeHtml(html, diffing) {
         const attrName = attribs[i];
         const attrValue = attribs[i + 1];
         if (!isValidAttr(tagName, attrName, attrValue)) {
-          user().error(TAG, `Removing "${attrName}" attribute with invalid `
-              + `value in <${tagName} ${attrName}="${attrValue}">.`);
+          user().error(TAG,
+              `Removed unsafe attribute: ${attrName}="${attrValue}"`);
           continue;
         }
         emit(' ');

test/integration/test-amp-bind.js

@@ -19,13 +19,14 @@ import {FormEvents} from '../../extensions/amp-form/0.1/form-events';
 import {Services} from '../../src/services';
 import {poll as classicPoll, createFixtureIframe} from '../../testing/iframe';
 
+const TIMEOUT = 15000;
+
 // Skip Edge, which throws "Permission denied" errors when inspecting
 // element properties in the testing iframe (Edge 17, Windows 10).
 describe.configure().skipEdge().run('amp-bind', function() {
-  // Give more than default 2000ms timeout for local testing.
-  const TIMEOUT = Math.max(window.ampTestRuntimeConfig.mochaTimeout, 4000);
   this.timeout(TIMEOUT);
 
+  // Helper that sets the poll timeout.
   function poll(desc, condition, onError) {
     return classicPoll(desc, condition, onError, TIMEOUT);
   }