amp-script: Add sandbox attr and "allow-forms" (ampproject#23143)

* Validation for sandbox attr. * Parse sandbox attr. * Clean up sanitize(). * Add example. * Update documentation. * Sandbox 'tokens'. * -and forms * Add more detail to allow-forms docs.
edelight · Jul 3, 2019 · b627467 · b627467
1 parent a276172
commit b627467
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 69 deletions.
diff --git a/examples/amp-script/hello-world.amp.html b/examples/amp-script/hello-world.amp.html
@@ -4,16 +4,14 @@
   <meta charset="utf-8">
   <link rel="canonical" href="self.html" />
   <meta name="viewport" content="width=device-width,minimum-scale=1">
-
   <!-- Token for "amp-script" experiment on "http://localhost:8000" that expires on 1/1/2030. -->
   <meta name="amp-experiment-token" content="AAAAAFd7Im9yaWdpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6ODAwMCIsImV4cGVyaW1lbnQiOiJhbXAtc2NyaXB0IiwiZXhwaXJhdGlvbiI6MTg5NjEzNDQwMDAwMH08Wb1WA4ZcsTLNK1pxwXdmfYUlv4ybtfIMw3yS66dZhsJl11JYrTanag4ZlAr4sfBRTVmQfKLsiJq/JJHdrpRzT9M6EQD/JYos1SlohtQiK2YfhvXgN7zot3WBuFkqoxSiyvX/l23UYKSo2ebNTnMDFhI+yrX8uc9BblbYniRtQGXBPktjJUIgavEBS5t45bgbPNutNvAreYGlz7na4lImrpnNa8K28Ow4hqOxcqqdFoHD9hlwtKlpydJQpEmtM+S+jdDLKWq6JtC+HoUSovuitrqkioJ4cfuVkFqtoaKWPLXSOwoc5UJl3zNjlTjvQ/Gnk33NdWs6/FtZbxCq+lWe">
-
-  <!-- CSP -->
-  <!-- <meta http-equiv="Content-Security-Policy" content="default-src * data: blob:; script-src blob: http://localhost:8000/dist/v0.js http://localhost:8000/dist/v0/amp-script-0.1.js https://cdn.ampproject.org/v0.js https://cdn.ampproject.org/v0/ https://cdn.ampproject.org/viewer/ https://cdn.ampproject.org/rtv/; object-src 'none'; style-src 'unsafe-inline' https://cdn.ampproject.org/rtv/ https://cdn.materialdesignicons.com https://cloud.typography.com https://fast.fonts.net https://fonts.googleapis.com https://maxcdn.bootstrapcdn.com https://p.typekit.net https://use.fontawesome.com https://use.typekit.net; report-uri https://csp-collector.appspot.com/csp/amp"> -->
-
   <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>
   <style amp-custom>
-    button {
+    h1 {
+      font-size: 1.2em;
+    }
+    button, input {
       border: solid 1px black;
     }
   </style>
@@ -23,25 +21,40 @@
 <body>
   <h1>Example: hello-world.js</h1>
   <amp-script layout=container src="/examples/amp-script/hello-world.js">
-    <div class="root">
-      <button id="hello">Insert Hello World!</button>
-      <button id="long">Long task</button>
-      <button id="amp-img">Insert amp-img</button>
-      <button id="script">Insert &lt;script&gt;</button>
-      <button id="img">Insert &lt;img&gt;</button>
-    </div>
+    <button id="hello">Insert Hello World!</button>
+    <button id="long">Long task</button>
+    <button id="amp-img">Insert amp-img</button>
+    <button id="script">Insert &lt;script&gt;</button>
+    <button id="img">Insert &lt;img&gt;</button>
+  </amp-script>
+
+  <h1>Example: empty.js</h1>
+  <amp-script layout=container src="/examples/amp-script/empty.js">
+    Should be empty.
   </amp-script>
 
   <h1>Example: #local-script</h1>
-  <amp-script width=400 height=100 script="local-script">
+  <amp-script width=300 height=20 script="local-script">
+    Should say "Hi there!".
   </amp-script>
   <script type="text/plain" target="amp-script" id="local-script">
     document.body.textContent = 'Hi there!';
   </script>
 
-  <h1>Example: empty.js</h1>
-  <amp-script layout=container src="/examples/amp-script/empty.js">
-    <div class="root">should be empty</div>
+  <h1>Example: sandbox=""</h1>
+  <amp-script width=300 height=20 script="no-sandbox">
+    Should be empty.
   </amp-script>
+  <script type="text/plain" target="amp-script" id="no-sandbox">
+    document.body.appendChild(document.createElement('input'));
+  </script>
+
+  <h1>Example: sandbox="allow-forms"</h1>
+  <amp-script width=300 height=20 script="sandbox-allow-forms" sandbox="allow-forms">
+    Should add an input field.
+  </amp-script>
+  <script type="text/plain" target="amp-script" id="sandbox-allow-forms">
+    document.body.appendChild(document.createElement('input'));
+  </script>
 </body>
 </html>
diff --git a/extensions/amp-form/amp-form.md b/extensions/amp-form/amp-form.md
@@ -633,7 +633,7 @@ In addition to following the details in the [AMP CORS spec](https://www.ampproje
 In general, keep in mind the following points when accepting input from the user:
 
 * Only use POST for state changing requests.
-* Use non-XHR GET for navigational purposes only (e.g., Search).
-    * non-XHR GET requests are not going to receive accurate origin/headers and backends won't be able to protect against XSRF with the above mechanism.
+* Use non-XHR GET for navigational purposes only (e.g. search).
+    * Non-XHR GET requests are will not receive accurate origin/headers and backends won't be able to protect against XSRF with the above mechanism.
     * In general, use XHR/non-XHR GET requests for navigational or information retrieval only.
-* non-XHR POST requests are not allowed in AMP documents. This is due to inconsistencies of setting `Origin` header on these requests across browsers. And the complications supporting it would introduce in protecting against XSRF. This might be reconsidered and introduced later, please file an issue if you think this is needed.
+* Non-XHR POST requests are not allowed in AMP documents. This is due to inconsistencies of setting `Origin` header on these requests across browsers, and the complications supporting it would introduce in protecting against XSRF. This might be reconsidered and introduced later &mdash; please file an issue if you think this is needed.
diff --git a/extensions/amp-script/0.1/amp-script.js b/extensions/amp-script/0.1/amp-script.js
@@ -140,6 +140,9 @@ export class AmpScript extends AMP.BaseElement {
       ? `amp-script[src="${this.element.getAttribute('src')}"].js`
       : `amp-script[script=#${this.element.getAttribute('script')}].js`;
 
+    const sandbox = this.element.getAttribute('sandbox') || '';
+    const sandboxTokens = sandbox.split(' ').map(s => s.trim());
+
     // @see src/main-thread/configuration.WorkerDOMConfiguration in worker-dom.
     const config = {
       authorURL: sourceURL,
@@ -148,7 +151,7 @@ export class AmpScript extends AMP.BaseElement {
         this.userActivation_.expandLongTask(promise);
         // TODO(dvoytenko): consider additional "progress" UI.
       },
-      sanitizer: new SanitizerImpl(this.win),
+      sanitizer: new SanitizerImpl(this.win, sandboxTokens),
       // Callbacks.
       onCreateWorker: data => {
         dev().info(TAG, 'Create worker:', data);
@@ -281,21 +284,23 @@ class AmpScriptService {
 export class SanitizerImpl {
   /**
    * @param {!Window} win
+   * @param {!Array<string>} sandboxTokens
    */
-  constructor(win) {
+  constructor(win, sandboxTokens) {
     /** @private {!DomPurifyDef} */
     this.purifier_ = createPurifier(win.document, dict({'IN_PLACE': true}));
 
     /** @private {!Object<string, boolean>} */
     this.allowedTags_ = getAllowedTags();
 
-    // TODO(choumx): Support opt-in for variable substitutions and forms.
-    // For now, only allow built-in AMP components except amp-pixel...
+    // TODO(choumx): Support opt-in for variable substitutions.
+    // For now, only allow built-in AMP components except amp-pixel.
     this.allowedTags_['amp-img'] = true;
     this.allowedTags_['amp-layout'] = true;
     this.allowedTags_['amp-pixel'] = false;
-    // ...and other elements that support variable substitutions, including
-    // form elements (tags included in HTMLFormElement.elements).
+
+    // "allow-forms" enables tags in HTMLFormElement.elements.
+    const allowForms = sandboxTokens.includes('allow-forms');
     const formElements = [
       'form',
       'button',
@@ -307,44 +312,26 @@ export class SanitizerImpl {
       'textarea',
     ];
     formElements.forEach(fe => {
-      this.allowedTags_[fe] = false;
+      this.allowedTags_[fe] = allowForms;
     });
-
-    /** @const @private {!Element} */
-    this.wrapper_ = win.document.createElement('div');
   }
 
   /**
-   * TODO(choumx): This is currently called by worker-dom on node creation,
-   * so all invocations are on super-simple nodes like <p></p>.
-   * Either it should be moved to node insertion to justify the more expensive
-   * sanitize() call, or this method should be a simple string lookup.
+   * This is called by worker-dom on node creation, so all invocations are on
+   * super-simple nodes like <p></p>.
    *
    * @param {!Node} node
    * @return {boolean}
    */
   sanitize(node) {
-    // DOMPurify sanitizes unsafe nodes by detaching them from parents.
-    // So, an unsafe `node` that has no parent will cause a runtime error.
-    // To avoid this, wrap `node` in a <div> if it has no parent.
-    const useWrapper = !node.parentNode;
-    if (useWrapper) {
-      this.wrapper_.appendChild(node);
-    }
-    const parent = node.parentNode || this.wrapper_;
-    this.purifier_.sanitize(parent);
-    const clean = parent.firstChild;
+    // TODO(choumx): allowedTags_[] is more strict than purifier.sanitize()
+    // because the latter has attribute-specific allowances.
+    const tag = node.nodeName.toLowerCase();
+    const clean = this.allowedTags_[tag];
     if (!clean) {
       user().warn(TAG, 'Sanitized node:', node);
-      return false;
-    }
-    // Detach `node` if we used a wrapper div.
-    if (useWrapper) {
-      while (this.wrapper_.firstChild) {
-        this.wrapper_.removeChild(this.wrapper_.firstChild);
-      }
     }
-    return true;
+    return clean;
   }
 
   /**
@@ -360,7 +347,6 @@ export class SanitizerImpl {
     const tag = node.nodeName.toLowerCase();
     // DOMPurify's attribute validation is tag-agnostic, so we need to check
     // that the tag itself is valid. E.g. a[href] is ok, but base[href] is not.
-    // TODO(choumx): This is actually more strict than sanitize().
     if (this.allowedTags_[tag]) {
       const attr = attribute.toLowerCase();
       if (validateAttributeChange(this.purifier_, node, attr, value)) {

diff --git a/extensions/amp-script/0.1/test/unit/test-amp-script.js b/extensions/amp-script/0.1/test/unit/test-amp-script.js
@@ -24,7 +24,7 @@ describe('SanitizerImpl', () => {
 
   beforeEach(() => {
     win = new FakeWindow();
-    s = new SanitizerImpl(win);
+    s = new SanitizerImpl(win, []);
     el = win.document.createElement('div');
   });
 
@@ -87,5 +87,19 @@ describe('SanitizerImpl', () => {
       s.mutateAttribute(input, 'value', 'foo');
       expect(input.getAttribute('value')).to.be.null;
     });
+
+    it('should allow changes to form elements if sandbox=allow-forms', () => {
+      s = new SanitizerImpl(win, ['allow-forms']);
+
+      const form = win.document.createElement('form');
+      s.mutateAttribute(form, 'action-xhr', 'https://example.com/post');
+      expect(form.getAttribute('action-xhr')).to.equal(
+        'https://example.com/post'
+      );
+
+      const input = win.document.createElement('input');
+      s.mutateAttribute(input, 'value', 'foo');
+      expect(input.getAttribute('value')).to.equal('foo');
+    });
   });
 });
diff --git a/extensions/amp-script/0.1/test/validator-amp-script.html b/extensions/amp-script/0.1/test/validator-amp-script.html
@@ -42,6 +42,10 @@
     </div>
   </amp-script>
 
+  <!-- Valid: 'sandbox' attribute. -->
+  <amp-script layout=container src="https://example.com/foo.js" sandbox="allow-forms">
+  </amp-script>
+
   <!-- Invalid: With neither 'script' or 'src' attributes. -->
   <amp-script layout=container></amp-script>
 

diff --git a/extensions/amp-script/0.1/test/validator-amp-script.out b/extensions/amp-script/0.1/test/validator-amp-script.out
@@ -45,53 +45,57 @@ amp-script/0.1/test/validator-amp-script.html:34:2 Custom JavaScript is not allo
 |      </div>
 |    </amp-script>
 |
+|    <!-- Valid: 'sandbox' attribute. -->
+|    <amp-script layout=container src="https://example.com/foo.js" sandbox="allow-forms">
+|    </amp-script>
+|
 |    <!-- Invalid: With neither 'script' or 'src' attributes. -->
 |    <amp-script layout=container></amp-script>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:46:2 The tag 'amp-script' is missing a mandatory attribute - pick at least one of ['script', 'src']. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM]
+amp-script/0.1/test/validator-amp-script.html:50:2 The tag 'amp-script' is missing a mandatory attribute - pick at least one of ['script', 'src']. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM]
 |
 |    <!-- Invalid: Relative URL. -->
 |    <amp-script layout=container src="/foo.js"></amp-script>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:49:2 The relative URL '/foo.js' for attribute 'src' in tag 'amp-script' is disallowed. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM]
+amp-script/0.1/test/validator-amp-script.html:53:2 The relative URL '/foo.js' for attribute 'src' in tag 'amp-script' is disallowed. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM]
 |
 |    <!-- Invalid: Non-https 'src' attribute. -->
 |    <amp-script layout=container src="http://not.https.url"></amp-script>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:52:2 Invalid URL protocol 'http:' for attribute 'src' in tag 'amp-script'. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM]
+amp-script/0.1/test/validator-amp-script.html:56:2 Invalid URL protocol 'http:' for attribute 'src' in tag 'amp-script'. (see https://amp.dev/documentation/components/amp-script) [AMP_TAG_PROBLEM]
 |
 |    <!-- Invalid: Local script without 'target' or 'id' attributes. -->
 |    <script type=text/plain>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:55:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
+amp-script/0.1/test/validator-amp-script.html:59:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
 |      document.body.textContent = "Hello World!";
 |    </script>
 |
 |    <!-- Invalid: Local script with invalid 'type'. -->
 |    <script type=text/amp-script target=amp-script id=hello>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:60:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
+amp-script/0.1/test/validator-amp-script.html:64:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
 |      document.body.textContent = "Hello World!";
 |    </script>
 |
 |    <!-- Invalid: Local script with invalid 'target'. -->
 |    <script type=text/plain target=amp-js id=hello>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:65:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
+amp-script/0.1/test/validator-amp-script.html:69:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
 |      document.body.textContent = "Hello World!";
 |    </script>
 |
 |    <!-- Invalid: Local script without 'target' attribute. -->
 |    <script type=text/plain id=hello>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:70:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
+amp-script/0.1/test/validator-amp-script.html:74:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
 |      document.body.textContent = "Hello World!";
 |    </script>
 |
 |    <!-- Invalid: Local script without 'id' attribute. -->
 |    <script type=text/plain target=amp-script>
 >>   ^~~~~~~~~
-amp-script/0.1/test/validator-amp-script.html:75:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
+amp-script/0.1/test/validator-amp-script.html:79:2 Custom JavaScript is not allowed. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags) [CUSTOM_JAVASCRIPT_DISALLOWED]
 |      document.body.textContent = "Hello World!";
 |    </script>
-|  </body>
+|  </body>
diff --git a/extensions/amp-script/amp-script.md b/extensions/amp-script/amp-script.md
@@ -98,15 +98,33 @@ For additional code samples, see [`examples/amp-script/`](https://github.com/amp
 For design details, see the ["Intent to Implement" issue](https://github.com/ampproject/amphtml/issues/13471).
 For more information on `worker-dom`, see the [@ampproject/worker-dom](https://github.com/ampproject/worker-dom/) repository.
 
-### Mutations and user actions
+### Mutations and user gestures
 
-`amp-script` generally requires a user action to perform mutates to avoid unexpected UI jumps without user's input, but there are some exception to this rule.
+`amp-script` generally requires a user gesture to perform mutations. This avoids content jumps that are not triggered by user gesture, but there are some exceptions:
 
-Overall mutation rules are:
+1. Mutations are always accepted for five seconds after a user gesture.
+2. The five second interval is extended if the author script performs a `fetch()` as a result of the user gesture.
+3. Mutations are always accepted for `amp-script` elements with `[layout!="container"]` and `height < 300px`.
 
-1. Mutations are always accepted after a user action for a user action interval of 5 seconds.
-2. The 5 seconds interval is extended if the user script performs a `fetch()` operation.
-3. Smaller `amp-script` elements with height under `300px` and non-`container` layout are allowed unlimited mutations.
+## Attributes
+
+**src**
+
+The URL of a JS file that will be executed in the context of this `<amp-script>`.
+
+**script**
+
+The `id` of a `script[type=text/plain][target=amp-script]` element whose text content contains JS that will be executed in the context of this `<amp-script>`.
+
+**sandbox (optional)**
+
+Applies extra restrictions to DOM that may be mutated by this `<amp-script>`. Similar to the `iframe[sandbox]` attribute, the value of the attribute can either be empty to apply all restrictions, or space-separated tokens to lift particular restrictions:
+
+- `allow-forms`: Allows [form elements](https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/elements) to be created and modified. AMP requires special handling to prevent unauthorized state changing requests from user input. See amp-form's [security considerations](https://amp.dev/documentation/components/amp-form#security-considerations) for more detail.
+
+**common attributes**
+
+This element includes [common attributes](https://www.ampproject.org/docs/reference/common_attributes) extended to AMP components.
 
 ## Interested in using `<amp-script>`?
 

diff --git a/extensions/amp-script/validator-amp-script.protoascii b/extensions/amp-script/validator-amp-script.protoascii
@@ -60,6 +60,7 @@ tags: {  # <amp-script>
   disallowed_ancestor: "AMP-SCRIPT"
   requires_extension: "amp-script"
   requires: "amp-experiment-token"
+  attrs: { name: "sandbox" }
   attrs: {
     name: "script"
     mandatory_anyof: "['script', 'src']"