Support for bind expressions in mustache templates (ampproject#7602)

* first pass at adding/removing subtrees * tests passing * Add ability to add and remove bindings in the subtree rooted at a specific node * lint fixes * merge conflict * ci issues * some cleanup * pr comments * pr comments, new method of removing bindings for node and its children * fix some lint warnings * test and lint fixes * mutation observer * update comments * add observer * naive implementation * cleanup * clean * cleanup bind impl * very simple example of bind in a template * pr comments * lint errors * pr comments * cleanup * lint errors * pr comments * pr comments and lint warnings * typo * comments * more lint warnings * pr comment * change README * cleanup * pr comments * pr comments 2 * lint warnings * don't use foreach on Nodelist * casting * more ci warnings * even more ci warnings * even more ci warnings * even more ci issues * even more ci comments * even more ci issues * pr comments * more pr comments * fix refactoring oversight * pr comments * pr comments * merge * pr comments * ci issues * pr comments * assertElement * pr comments * typo * updating sanitizer * fixing up sanitizer test * pr comments * pr comments * expanded on amp-mustache tests * pr comments * remove class from whitelist * restore whole sandbox * update mustache validator test and output * pr comments
mrjoro · Apr 28, 2017 · e0b7ba3 · e0b7ba3
1 parent 9256144
commit e0b7ba3
Show file tree

Hide file tree

Showing 12 changed files with 418 additions and 53 deletions.
diff --git a/examples/bind/forms.amp.html b/examples/bind/forms.amp.html
@@ -0,0 +1,47 @@
+<!doctype html>
+<html ⚡>
+<head>
+    <meta charset="utf-8">
+    <title>Forms Examples in AMP</title>
+    <link rel="canonical" href="amps.html" >
+    <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+    <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+    <script async src="https://cdn.ampproject.org/v0.js"></script>
+    <script async custom-element="amp-form" src="https://cdn.ampproject.org/v0/amp-form-0.1.js"></script>
+    <script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>
+    <script async custom-element="amp-bind" src="https://cdn.ampproject.org/v0/amp-bind-0.1.js"></script>
+</head>
+<body>
+
+  <p>Submit the form and then press a button to change the text in the below template</p>
+  <button on="tap:AMP.setState(selected=1)">1</button>
+  <button on="tap:AMP.setState(selected=2)">2</button>
+  <button on="tap:AMP.setState(selected=3)">3</button>
+  <button on="tap:AMP.setState(selected=4)">4</button>
+
+  <h4>Enter your name and email.</h4>
+  <form method="post"
+        action-xhr="/form/echo-json/post"
+        target="_blank">
+      <fieldset>
+          <label>
+              <span>Your name</span>
+              <input type="text" name="name" id="name1" required>
+          </label>
+          <label>
+              <span>Your email</span>
+              <input type="email" name="email" id="email1" required>
+          </label>
+          <input type="submit" value="Subscribe">
+      </fieldset>
+
+      <div submit-success>
+          <template type="amp-mustache">
+              Success! Thanks {{name}} for entering your email: {{email}}
+
+              <p>You have selected: <span [text]="selected ? selected : 'No selection'">0</span></p>
+          </template>
+      </div>
+  </form>
+</body>
+</html>
diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js
@@ -28,6 +28,7 @@ import {reportError} from '../../../src/error';
 import {resourcesForDoc} from '../../../src/resources';
 import {filterSplice} from '../../../src/utils/array';
 import {rewriteAttributeValue} from '../../../src/sanitizer';
+import {timerFor} from '../../../src/timer';
 
 const TAG = 'amp-bind';
 
@@ -100,10 +101,15 @@ export class Bind {
     this.resources_ = resourcesForDoc(ampdoc);
 
     /**
-     * True if a digest is triggered before scan for bindings completes.
-     * @private {boolean}
+     * @const @private {!Array<Promise>}
      */
-    this.digestQueuedAfterScan_ = false;
+    this.mutationPromises_ = [];
+
+    /**
+     * @const @private {MutationObserver}
+     */
+    this.mutationObserver_ =
+        new MutationObserver(this.onMutationsObserved_.bind(this));
 
     /** @const @private {boolean} */
     this.workerExperimentEnabled_ = isExperimentOn(this.win_, 'web-worker');
@@ -282,7 +288,7 @@ export class Bind {
   /**
    * Scans `node` for attributes that conform to bind syntax and returns
    * a tuple containing bound elements and binding data for the evaluator.
-   * @param {!Element} node
+   * @param {!Node} node
    * @return {
    *   !Promise<{
    *     boundElements: !Array<BoundElementDef>,
@@ -307,11 +313,18 @@ export class Bind {
     // Helper function for scanning the tree walker's next node.
     // Returns true if the walker has no more nodes.
     const scanNextNode_ = () => {
-      const element = walker.nextNode();
-      if (!element) {
+      const node = walker.currentNode;
+      if (!node) {
         return true;
       }
+      // Walker is filtered to only return elements
+      const element = dev().assertElement(node);
       const tagName = element.tagName;
+      if (tagName === 'TEMPLATE') {
+        // Listen for changes in amp-mustache templates
+        this.observeElementForMutations_(element);
+      }
+
       const boundProperties = this.scanElement_(element);
       if (boundProperties.length > 0) {
         boundElements.push({element, boundProperties});
@@ -325,7 +338,7 @@ export class Bind {
         }
         expressionToElements[expressionString].push(element);
       });
-      return false;
+      return !walker.nextNode();
     };
 
     return new Promise(resolve => {
@@ -653,6 +666,61 @@ export class Bind {
     }
   }
 
+  /**
+   * Begin observing mutations to element. Presently, all supported elements
+   * that can add/remove bindings add new elements to their parent, so parent
+   * node should be observed for mutations.
+   * @private
+   */
+  observeElementForMutations_(element) {
+    // TODO(kmh287): What if parent is the body tag?
+    // TODO(kmh287): Generify logic for node observation strategy
+    // when bind supprots more dynamic nodes.
+    const elementToObserve = element.parentElement;
+    this.mutationObserver_.observe(elementToObserve, {childList: true});
+  }
+
+  /**
+   * Respond to observed mutations. Adds all bindings for newly added elements
+   * removes bindings for removed elements, then immediately applies the current
+   * scope to the new bindings.
+   *
+   * @param mutations {Array<MutationRecord>}
+   * @private
+   */
+  onMutationsObserved_(mutations) {
+    mutations.forEach(mutation => {
+      // Add bindings for new nodes first to ensure that a binding isn't removed
+      // and then subsequently re-added.
+      const addPromises = [];
+      const addedNodes = mutation.addedNodes;
+      for (let i = 0; i < addedNodes.length; i++) {
+        const addedNode = addedNodes[i];
+        if (addedNode.nodeType == Node.ELEMENT_NODE) {
+          const addedElement = dev().assertElement(addedNode);
+          addPromises.push(this.addBindingsForNode_(addedElement));
+        }
+      }
+      const mutationPromise = Promise.all(addPromises).then(() => {
+        const removePromises = [];
+        const removedNodes = mutation.removedNodes;
+        for (let i = 0; i < removedNodes.length; i++) {
+          const removedNode = removedNodes[i];
+          if (removedNode.nodeType == Node.ELEMENT_NODE) {
+            const removedElement = dev().assertElement(removedNode);
+            removePromises.push(this.removeBindingsForNode_(removedElement));
+          }
+        }
+        return Promise.all(removePromises);
+      }).then(() => {
+        return this.digest_();
+      });
+      if (getMode().test) {
+        this.mutationPromises_.push(mutationPromise);
+      }
+    });
+  }
+
   /**
    * Returns true if both arrays contain the same strings.
    * @param {!(IArrayLike<string>|Array<string>)} a
@@ -723,4 +791,23 @@ export class Bind {
 
     return false;
   }
+
+  /**
+   * Wait for DOM mutation observer callbacks to fire. Returns a promise
+   * that resolves when mutation callbacks have fired.
+   *
+   * @return {Promise}
+   *
+   * @visibleForTesting
+   */
+  waitForAllMutationsForTesting() {
+    return timerFor(this.win_).poll(5, () => {
+      return this.mutationPromises_.length > 0;
+    }).then(() => {
+      return Promise.all(this.mutationPromises_);
+    }).then(() => {
+      this.mutationPromises_.length = 0;
+    });
+  }
+
 }
diff --git a/extensions/amp-bind/0.1/test/test-bind-impl.js b/extensions/amp-bind/0.1/test/test-bind-impl.js
@@ -21,6 +21,7 @@ import {chunkInstanceForTesting} from '../../../../src/chunk';
 import {toArray} from '../../../../src/types';
 import {toggleExperiment} from '../../../../src/experiments';
 import {user} from '../../../../src/log';
+import {installTimerService} from '../../../../src/service/timer-impl';
 
 describes.realWin('amp-bind', {
   amp: {
@@ -33,6 +34,7 @@ describes.realWin('amp-bind', {
   let canBindStub;
 
   beforeEach(() => {
+    installTimerService(env.win);
     toggleExperiment(env.win, 'amp-bind', true);
 
     // Stub validator methods to return true for ease of testing.
@@ -144,6 +146,68 @@ describes.realWin('amp-bind', {
     });
   });
 
+  describe('under dynamic tags', () => {
+    it('should dynamically detect new bindings', () => {
+      const doc = env.win.document;
+      const template = doc.createElement('template');
+      doc.getElementById('parent').appendChild(template);
+      return onBindReady().then(() => {
+        expect(bind.boundElements_.length).to.equal(0);
+        createElementWithBinding('[onePlusOne]="1+1"');
+        return bind.waitForAllMutationsForTesting();
+      }).then(() => {
+        expect(bind.boundElements_.length).to.equal(1);
+      });
+    });
+
+    //TODO(kmh287): Move to integration test
+    it('should NOT bind blacklisted attributes', () => {
+      // Restore real implementations of canBind and isResultValid
+      sandbox.restore();
+      const doc = env.win.document;
+      const template = doc.createElement('template');
+      let textElement;
+      doc.getElementById('parent').appendChild(template);
+      return onBindReady().then(() => {
+        expect(bind.boundElements_.length).to.equal(0);
+        const binding = '[onclick]="\'alert(document.cookie)\'" ' +
+          '[onmouseover]="\'alert()\'" ' +
+          '[style]="\'background=color:black\'"';
+        textElement = createElementWithBinding(binding);
+        return bind.waitForAllMutationsForTesting();
+      }).then(() => {
+        expect(bind.boundElements_.length).to.equal(0);
+        expect(textElement.getAttribute('onclick')).to.be.null;
+        expect(textElement.getAttribute('onmouseover')).to.be.null;
+        expect(textElement.getAttribute('style')).to.be.null;
+      });
+    });
+
+    //TODO(kmh287): Move to integration test
+    it('should NOT allow unsecure attribute values', () => {
+      // Restore real implementations of canBind and isResultValid
+      sandbox.restore();
+      const doc = env.win.document;
+      const template = doc.createElement('template');
+      let aElement;
+      doc.getElementById('parent').appendChild(template);
+      return onBindReady().then(() => {
+        expect(bind.boundElements_.length).to.equal(0);
+        const binding = '[href]="javascript:alert(1)"';
+        const div = env.win.document.createElement('div');
+        div.innerHTML = '<a ' + binding + '></a>';
+        aElement = div.firstElementChild;
+        // Templated HTML is added as a sibling to the template,
+        // not as a child
+        doc.getElementById('parent').appendChild(aElement);
+        return bind.waitForAllMutationsForTesting();
+      }).then(() => {
+        expect(aElement.getAttribute('href')).to.be.null;
+      });
+    });
+  });
+
+
   it('should NOT apply expressions on first load', () => {
     const element = createElementWithBinding('[onePlusOne]="1+1"');
     expect(element.getAttribute('onePlusOne')).to.equal(null);

diff --git a/extensions/amp-mustache/0.1/test/test-amp-mustache.js b/extensions/amp-mustache/0.1/test/test-amp-mustache.js
@@ -40,7 +40,47 @@ describe('amp-mustache template', () => {
     expect(result./*OK*/innerHTML).to.equal('value = <a target="_top">abc</a>');
   });
 
+  it('should sanitize templated tag names', () => {
+    const templateElement = document.createElement('div');
+    templateElement./*OK*/innerHTML =
+        'value = <{{value}} href="javascript:alert(0)">abc</{{value}}>';
+    const template = new AmpMustache(templateElement);
+    template.compileCallback();
+    const result = template.render({
+      value: 'a',
+    });
+    expect(result./*OK*/innerHTML).to.not
+        .equal('<a href="javascript:alert(0)">abc</a>');
+    expect(result.firstElementChild).to.be.null;
+  });
+
   describe('Sanitizing data- attributes', () => {
+
+    it('should sanitize templated attribute names', () => {
+      const templateElement = document.createElement('div');
+      templateElement./*OK*/innerHTML =
+          'value = <a {{value}}="javascript:alert(0)">abc</a>';
+      let template = new AmpMustache(templateElement);
+      template.compileCallback();
+      let result = template.render({
+        value: 'href',
+      });
+      expect(result).to.not.equal('<a href="javascript:alert(0)">abc</a>');
+      expect(result.firstElementChild.getAttribute('href')).to.be.null;
+
+      templateElement./*OK*/innerHTML =
+          'value = <p [{{value}}]="javascript:alert()">ALERT</p>';
+      template = new AmpMustache(templateElement);
+      template.compileCallback();
+      result = template.render({
+        value: 'onclick',
+      });
+      expect(result).to.not
+          .equal('<p [onclick]="javascript:alert()">ALERT</p>');
+      expect(result.firstElementChild.getAttribute('[onclick]')).to.be.null;
+      expect(result.firstElementChild.getAttribute('onclick')).to.be.null;
+    });
+
     it('should parse data-&style=value output correctly', () => {
       const templateElement = document.createElement('div');
       templateElement./*OK*/innerHTML = 'value = <a href="{{value}}"' +

diff --git a/extensions/amp-mustache/0.1/test/validator-amp-mustache.html b/extensions/amp-mustache/0.1/test/validator-amp-mustache.html
@@ -33,8 +33,10 @@
   <p title="{{allowed}}">{{allowed}}</p>
   <p {{notallowed}}></p>
   <p {{notallowed}}=0></p>
+  <p [{{notallowed}}]=0></p>
   <p data-{notallowed}=0></p>
   <p data-{{notallowed}}=0></p>
+  <p data-[{{notallowed}}]=0></p>
   <p data-{{{notallowed}}}=0></p>
   <p data-{{&notallowed}}=0></p>
   <p data-{{#notallowed}}=0></p>