✨ Create whitelist for amp-viewer-assistance.updateActionState action…

…Status (#21630) * Add actionStatus whitelist for amp-viewer-assistance.updateActionState. Change-Id: I9aeff992b2cde5215463a1f4ad717f7426818498 * update docs Change-Id: I9147bfe84b34bbe5ed0e86c59155cdf51e972cc6 * Address PR comments. Change-Id: I4673794d9e0a469e1bf5ee7a755682b40f8ae300
ampproject · Apr 3, 2019 · cf42cef · cf42cef
1 parent 7b16d1c
commit cf42cef
Show file tree

Hide file tree

Showing 3 changed files with 123 additions and 37 deletions.
diff --git a/extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js b/extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js
@@ -28,6 +28,13 @@ const TAG = 'amp-viewer-assistance';
 /** @const {string} */
 const GSI_TOKEN_PROVIDER = 'actions-on-google-gsi';
 
+/** @const {!Array<string>} */
+const ACTION_STATUS_WHITELIST = [
+  'ACTIVE_ACTION_STATUS',
+  'FAILED_ACTION_STATUS',
+  'COMPLETED_ACTION_STATUS',
+];
+
 export class AmpViewerAssistance {
   /**
    * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
@@ -70,10 +77,13 @@ export class AmpViewerAssistance {
    */
   actionHandler_(invocation) {
     const {method, args} = invocation;
-    if (method == 'updateActionState' && !!args) {
-      this.viewer_./*OK*/sendMessageAwaitResponse(method, args).catch(error => {
-        user().error(TAG, error.toString());
-      });
+    if (method == 'updateActionState') {
+      if (args && this.isValidActionStatusArgs_(args)) {
+        this.viewer_./*OK*/sendMessageAwaitResponse(method, args)
+            .catch(error => {
+              user().error(TAG, error.toString());
+            });
+      }
     } else if (method == 'signIn') {
       this.requestSignIn_();
     }
@@ -148,6 +158,33 @@ export class AmpViewerAssistance {
     });
   }
 
+
+  /**
+   * Checks the 'actionStatus' field of the updateActionState arguments against
+   * a whitelist.
+   * @private
+   * @param {!Object} args
+   * @return {boolean}
+   */
+  isValidActionStatusArgs_(args) {
+    const update = args['update'];
+    const actionStatus = update ? update['actionStatus'] : undefined;
+    if (!update || !actionStatus) {
+      user().error(TAG, 'Invalid arguments for updateActionState! Must have' +
+          ' an "update" object with an "actionStatus" field.');
+      return false;
+    }
+
+    if (!ACTION_STATUS_WHITELIST.includes(actionStatus)) {
+      user().error(TAG, 'Invalid actionStatus for updateActionState! '
+          + actionStatus);
+      return false;
+    }
+
+    user().info(TAG, 'Sending actionStatus: ' + actionStatus);
+    return true;
+  }
+
   /**
    * Toggles the CSS classes related to the status of the identity token.
    * @private
@@ -178,7 +215,6 @@ export class AmpViewerAssistance {
     this.vsync_.mutate(() => {
       this.getRootElement_().classList.toggle(className, on);
     });
-
   }
 }
 

diff --git a/extensions/amp-viewer-assistance/0.1/test/test-amp-viewer-assistance.js b/extensions/amp-viewer-assistance/0.1/test/test-amp-viewer-assistance.js
@@ -90,39 +90,83 @@ describes.fakeWin('AmpViewerAssistance', {
     });
   });
 
-  it('should send updateActionState to the viewer', () => {
-    const config = {
-      'providerId': 'foo-bar',
-    };
-    element.textContent = JSON.stringify(config);
-    const service = new AmpViewerAssistance(ampdoc);
-    const sendMessageStub = service.viewer_.sendMessageAwaitResponse;
-    const invocationArgs = {
-      'foo': 'bar',
-    };
-    return service.start_().then(() => {
-      sendMessageStub.resetHistory();
-      const invocation = new ActionInvocation(
-          element, 'updateActionState', invocationArgs);
-      service.actionHandler_(invocation);
-      expect(sendMessageStub).to.be.calledOnce;
-      expect(sendMessageStub.firstCall.args[0]).to.equal('updateActionState');
-      expect(sendMessageStub.firstCall.args[1]).to.deep.equal(invocationArgs);
+  describe('updateActionState', () => {
+    it('should send updateActionState to the viewer', () => {
+      const config = {
+        'providerId': 'foo-bar',
+      };
+      element.textContent = JSON.stringify(config);
+      const service = new AmpViewerAssistance(ampdoc);
+      const sendMessageStub = service.viewer_.sendMessageAwaitResponse;
+      const invocationArgs = {
+        'update': {'actionStatus': 'COMPLETED_ACTION_STATUS'},
+      };
+      return service.start_().then(() => {
+        sendMessageStub.resetHistory();
+        const invocation = new ActionInvocation(
+            element, 'updateActionState', invocationArgs);
+        service.actionHandler_(invocation);
+        expect(sendMessageStub).to.be.calledOnce;
+        expect(sendMessageStub.firstCall.args[0]).to.equal('updateActionState');
+        expect(sendMessageStub.firstCall.args[1]).to.deep.equal(invocationArgs);
+      });
     });
-  });
 
-  it('should fail to send updateActionState if args are missing', () => {
-    const config = {
-      'providerId': 'foo-bar',
-    };
-    element.textContent = JSON.stringify(config);
-    const service = new AmpViewerAssistance(ampdoc);
-    const sendMessageStub = service.viewer_.sendMessage;
-    return service.start_().then(() => {
-      sendMessageStub.reset();
-      const invocation = new ActionInvocation(element, 'updateActionState');
-      service.actionHandler_(invocation);
-      expect(sendMessageStub).to.not.be.called;
+    it('should fail to send updateActionState if args are missing', () => {
+      const config = {
+        'providerId': 'foo-bar',
+      };
+      element.textContent = JSON.stringify(config);
+      const service = new AmpViewerAssistance(ampdoc);
+      const sendMessageStub = service.viewer_.sendMessageAwaitResponse;
+      return service.start_().then(() => {
+        sendMessageStub.resetHistory();
+        const invocation = new ActionInvocation(element, 'updateActionState');
+        service.actionHandler_(invocation);
+        expect(sendMessageStub).to.not.be.called;
+      });
+    });
+
+    it('should fail to send updateActionState if args is missing update ' +
+        'object', () => {
+      const config = {
+        'providerId': 'foo-bar',
+      };
+      element.textContent = JSON.stringify(config);
+      const service = new AmpViewerAssistance(ampdoc);
+      const sendMessageStub = service.viewer_.sendMessageAwaitResponse;
+      return service.start_().then(() => {
+        sendMessageStub.reset();
+        const invocationArgs = {'update': {}};
+        const invocation = new ActionInvocation(element, 'updateActionState',
+            invocationArgs);
+        allowConsoleError(() => {
+          service.actionHandler_(invocation);
+        });
+        expect(sendMessageStub).to.not.be.called;
+      });
+    });
+
+    it('should fail to send updateActionState if args has an invalid ' +
+        'actionStatus', () => {
+      const config = {
+        'providerId': 'foo-bar',
+      };
+      element.textContent = JSON.stringify(config);
+      const service = new AmpViewerAssistance(ampdoc);
+      const sendMessageStub = service.viewer_.sendMessageAwaitResponse;
+      return service.start_().then(() => {
+        sendMessageStub.reset();
+        const invocationArgs = {
+          'update': {'actionStatus': 'INVALID_ACTION_STATUS'},
+        };
+        const invocation = new ActionInvocation(element, 'updateActionState',
+            invocationArgs);
+        allowConsoleError(() => {
+          service.actionHandler_(invocation);
+        });
+        expect(sendMessageStub).to.not.be.called;
+      });
     });
   });
 

diff --git a/extensions/amp-viewer-assistance/amp-viewer-assistance.md b/extensions/amp-viewer-assistance/amp-viewer-assistance.md
@@ -63,7 +63,13 @@ The `amp-viewer-assistance` extension currently has two functions that can be in
   </tr>
   <tr>
     <td class="col-fourty"><code>updateActionState</code></td>
-    <td>A function to send a message to the outer viewer representing a state change. Should contain an argument of the resulting state change.</td>
+    <td>A function to send a message to the outer viewer representing a state change. Requires an <code>update</code> object parameter of the following format:
+
+    {
+      "actionStatus": "COMPLETED_ACTION_STATUS" | "ACTIVE_ACTION_STATUS" |
+          "FAILED_ACTION_STATUS",
+      "result": { ... }, // optional field used with COMPLETED_ACTION_STATUS
+    }
   </tr>
 </table>