Roll forward "Delegate trust check... (#21553)" (#21653)

* Revert "Revert "Delegate trust check to action handler (#21553)" (#21563)"

This reverts commit d96498b.

* Fix type check.

* Fix test-amp-access.js.

* Fix test-amp-form.js.

* Test trust check in amp-access.

extensions/amp-access/0.1/amp-access.js

@@ -16,6 +16,7 @@
 
 import {AccessSource, AccessType} from './amp-access-source';
 import {AccessVars} from './access-vars';
+import {ActionTrust} from '../../../src/action-constants';
 import {AmpEvents} from '../../../src/amp-events';
 import {CSS} from '../../../build/amp-access-0.1.css';
 import {Observable} from '../../../src/observable';
@@ -652,6 +653,9 @@ export class AccessService {
    * @private
    */
   handleAction_(invocation) {
+    if (!invocation.satisfiesTrust(ActionTrust.HIGH)) {
+      return null;
+    }
     if (invocation.method == 'login') {
       if (invocation.event) {
         invocation.event.preventDefault();

extensions/amp-access/0.1/test/test-amp-access.js

@@ -1025,7 +1025,12 @@ describes.fakeWin('AccessService refresh', {
         .withExactArgs()
         .once();
     const event = {preventDefault: sandbox.spy()};
-    service.handleAction_({method: 'refresh', event});
+    const invocation = {method: 'refresh', event, satisfiesTrust: () => false};
+    service.handleAction_(invocation);
+    expect(event.preventDefault).to.not.be.called;
+
+    invocation.satisfiesTrust = () => true;
+    service.handleAction_(invocation);
     expect(event.preventDefault).to.be.calledOnce;
   });
 });
@@ -1096,7 +1101,12 @@ describes.fakeWin('AccessService login', {
         .withExactArgs('')
         .once();
     const event = {preventDefault: sandbox.spy()};
-    service.handleAction_({method: 'login', event});
+    const invocation = {method: 'login', event, satisfiesTrust: () => false};
+    service.handleAction_(invocation);
+    expect(event.preventDefault).to.not.be.called;
+
+    invocation.satisfiesTrust = () => true;
+    service.handleAction_(invocation);
     expect(event.preventDefault).to.be.calledOnce;
   });
 
@@ -1105,7 +1115,13 @@ describes.fakeWin('AccessService login', {
         .withExactArgs('other')
         .once();
     const event = {preventDefault: sandbox.spy()};
-    service.handleAction_({method: 'login-other', event});
+    const invocation =
+        {method: 'login-other', event, satisfiesTrust: () => false};
+    service.handleAction_(invocation);
+    expect(event.preventDefault).to.not.be.called;
+
+    invocation.satisfiesTrust = () => true;
+    service.handleAction_(invocation);
     expect(event.preventDefault).to.be.calledOnce;
   });
 

extensions/amp-form/0.1/amp-form.js

@@ -206,7 +206,7 @@ export class AmpForm {
         this.form_, () => this.handleXhrVerify_());
 
     this.actions_.installActionHandler(
-        this.form_, this.actionHandler_.bind(this), ActionTrust.HIGH);
+        this.form_, this.actionHandler_.bind(this));
     this.installEventHandlers_();
     this.installInputMasking_();
 
@@ -313,6 +313,9 @@ export class AmpForm {
    * @private
    */
   actionHandler_(invocation) {
+    if (!invocation.satisfiesTrust(ActionTrust.HIGH)) {
+      return null;
+    }
     if (invocation.method == 'submit') {
       return this.whenDependenciesReady_().then(() => {
         return this.handleSubmitAction_(invocation);

extensions/amp-form/0.1/test/test-amp-form.js

@@ -1619,16 +1619,30 @@ describes.repeated('', {
 
       expect(actions.installActionHandler).to.be.calledWith(form);
       sandbox.spy(ampForm, 'handleSubmitAction_');
-      ampForm.actionHandler_({method: 'anything'});
+      ampForm.actionHandler_({method: 'anything', satisfiesTrust: () => true});
       expect(ampForm.handleSubmitAction_).to.have.not.been.called;
-      ampForm.actionHandler_({method: 'submit'});
+      ampForm.actionHandler_({method: 'submit', satisfiesTrust: () => true});
 
       return whenCalled(ampForm.xhr_.fetch).then(() => {
         expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
         document.body.removeChild(form);
       });
     });
 
+    it('should not invoke low-trust action invocations', () => {
+      const form = getForm();
+      document.body.appendChild(form);
+
+      const ampForm = new AmpForm(form);
+      sandbox.stub(ampForm.xhr_, 'fetch').returns(Promise.resolve());
+
+      sandbox.spy(ampForm, 'handleSubmitAction_');
+      ampForm.actionHandler_({method: 'submit', satisfiesTrust: () => false});
+      expect(ampForm.handleSubmitAction_).to.have.not.been.called;
+
+      document.body.removeChild(form);
+    });
+
     it('should handle clear action and restore initial values', () => {
       const form = getForm();
       document.body.appendChild(form);
@@ -1646,11 +1660,14 @@ describes.repeated('', {
         ampForm.form_.elements.name.value = 'Jack Sparrow';
 
         sandbox.spy(ampForm, 'handleClearAction_');
-        ampForm.actionHandler_({method: 'anything'});
+        ampForm.actionHandler_({
+          method: 'anything',
+          satisfiesTrust: () => true,
+        });
         expect(ampForm.handleClearAction_).to.have.not.been.called;
 
         expect(ampForm.getFormAsObject_()).to.not.deep.equal(initalFormValues);
-        ampForm.actionHandler_({method: 'clear'});
+        ampForm.actionHandler_({method: 'clear', satisfiesTrust: () => true});
         expect(ampForm.handleClearAction_).to.have.been.called;
 
         expect(ampForm.getFormAsObject_()).to.deep.equal(initalFormValues);
@@ -1726,7 +1743,10 @@ describes.repeated('', {
             .returns(new Promise(unusedResolve => {}));
         sandbox.spy(ampForm, 'handleSubmitAction_');
 
-        const submitPromise = ampForm.actionHandler_({method: 'submit'});
+        const submitPromise = ampForm.actionHandler_({
+          method: 'submit',
+          satisfiesTrust: () => true,
+        });
         expect(ampForm.handleSubmitAction_).to.have.not.been.called;
         return timer.promise(1).then(() => {
           expect(ampForm.handleSubmitAction_).to.have.not.been.called;
@@ -1753,7 +1773,7 @@ describes.repeated('', {
             .returns(Promise.resolve());
         sandbox.spy(ampForm, 'handleSubmitAction_');
 
-        ampForm.actionHandler_({method: 'submit'});
+        ampForm.actionHandler_({method: 'submit', satisfiesTrust: () => true});
         expect(ampForm.handleSubmitAction_).to.have.not.been.called;
         return timer.promise(1).then(() => {
           expect(ampForm.handleSubmitAction_).to.have.not.been.called;

extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js

@@ -111,8 +111,7 @@ export class AmpViewerAssistance {
         return this;
       }
       this.action_.installActionHandler(
-          this.assistanceElement_, this.actionHandler_.bind(this),
-          ActionTrust.HIGH);
+          this.assistanceElement_, this.actionHandler_.bind(this));
 
       this.getIdTokenPromise();
 

src/service/action-impl.js

@@ -54,7 +54,7 @@ const DEFAULT_DEBOUNCE_WAIT = 300; // ms
 const DEFAULT_THROTTLE_INTERVAL = 100; // ms
 
 /** @const {!Object<string,!Array<string>>} */
-const ELEMENTS_ACTIONS_MAP_ = {
+const NON_AMP_ELEMENTS_ACTIONS_ = {
   'form': ['submit', 'clear'],
 };
 
@@ -254,6 +254,7 @@ export class ActionService {
      * @const @private {!Object<string, {handler: ActionHandlerDef, minTrust: ActionTrust}>}
      */
     this.globalMethodHandlers_ = map();
+
     // Add core events.
     this.addEvent('tap');
     this.addEvent('submit');
@@ -393,39 +394,37 @@ export class ActionService {
   }
 
   /**
-   * Installs action handler for the specified element.
+   * Installs action handler for the specified element. The action handler is
+   * responsible for checking invocation trust.
+   *
+   * For AMP elements, use base-element.registerAction() instead.
+   *
    * @param {!Element} target
    * @param {ActionHandlerDef} handler
-   * @param {ActionTrust} minTrust
    */
-  installActionHandler(target, handler, minTrust = ActionTrust.HIGH) {
+  installActionHandler(target, handler) {
     // TODO(dvoytenko, #7063): switch back to `target.id` with form proxy.
     const targetId = target.getAttribute('id') || '';
-    const debugId = target.tagName + '#' + targetId;
-    devAssert((targetId && targetId.substring(0, 4) == 'amp-') ||
-        target.tagName.toLowerCase() in ELEMENTS_ACTIONS_MAP_,
-    'AMP element or a whitelisted target element is expected: %s', debugId);
+
+    devAssert(isAmpTagName(targetId) ||
+        target.tagName.toLowerCase() in NON_AMP_ELEMENTS_ACTIONS_,
+    'AMP or special element expected: %s', target.tagName + '#' + targetId);
 
     if (target[ACTION_HANDLER_]) {
       dev().error(TAG_, `Action handler already installed for ${target}`);
       return;
     }
+    target[ACTION_HANDLER_] = handler;
 
     /** @const {Array<!ActionInvocation>} */
-    const currentQueue = target[ACTION_QUEUE_];
-
-    target[ACTION_HANDLER_] = {handler, minTrust};
-
-    // Dequeue the current queue.
-    if (isArray(currentQueue)) {
+    const queuedInvocations = target[ACTION_QUEUE_];
+    if (isArray(queuedInvocations)) {
+      // Invoke and clear all queued invocations now handler is installed.
       Services.timerFor(toWin(target.ownerDocument.defaultView)).delay(() => {
         // TODO(dvoytenko, #1260): dedupe actions.
-        currentQueue.forEach(invocation => {
+        queuedInvocations.forEach(invocation => {
           try {
-            if (invocation.satisfiesTrust(
-                /** @type {ActionTrust} */ (minTrust))) {
-              handler(invocation);
-            }
+            handler(invocation);
           } catch (e) {
             dev().error(TAG_, 'Action execution failed:', invocation, e);
           }
@@ -552,7 +551,6 @@ export class ActionService {
     }
   }
 
-
   /**
    * @param {!ActionInvocation} invocation
    * @return {?Promise}
@@ -563,7 +561,7 @@ export class ActionService {
 
     // Check that this action is whitelisted (if a whitelist is set).
     if (this.whitelist_) {
-      if (!isActionWhitelisted_(invocation, this.whitelist_)) {
+      if (!isActionWhitelisted(invocation, this.whitelist_)) {
         this.error_(`"${tagOrTarget}.${method}" is not whitelisted ${
           JSON.stringify(this.whitelist_)}.`);
         return null;
@@ -587,7 +585,7 @@ export class ActionService {
 
     // Handle element-specific actions.
     const lowerTagName = node.tagName.toLowerCase();
-    if (lowerTagName.substring(0, 4) == 'amp-') {
+    if (isAmpTagName(lowerTagName)) {
       if (node.enqueAction) {
         node.enqueAction(invocation);
       } else {
@@ -596,18 +594,15 @@ export class ActionService {
       return null;
     }
 
-    // Special elements with AMP ID or known supported actions.
-    const supportedActions = ELEMENTS_ACTIONS_MAP_[lowerTagName];
+    // Special non-AMP elements with AMP ID or known supported actions.
+    const nonAmpActions = NON_AMP_ELEMENTS_ACTIONS_[lowerTagName];
     // TODO(dvoytenko, #7063): switch back to `target.id` with form proxy.
     const targetId = node.getAttribute('id') || '';
-    if ((targetId && targetId.substring(0, 4) == 'amp-') ||
-        (supportedActions && supportedActions.indexOf(method) > -1)) {
-      const holder = node[ACTION_HANDLER_];
-      if (holder) {
-        const {handler, minTrust} = holder;
-        if (invocation.satisfiesTrust(minTrust)) {
-          handler(invocation);
-        }
+    if (isAmpTagName(targetId) ||
+        (nonAmpActions && nonAmpActions.indexOf(method) > -1)) {
+      const handler = node[ACTION_HANDLER_];
+      if (handler) {
+        handler(invocation);
       } else {
         node[ACTION_QUEUE_] = node[ACTION_QUEUE_] || [];
         node[ACTION_QUEUE_].push(invocation);
@@ -766,6 +761,15 @@ export class ActionService {
   }
 }
 
+/**
+ * @param {string} lowercaseTagName
+ * @return {boolean}
+ * @private
+ */
+function isAmpTagName(lowercaseTagName) {
+  return lowercaseTagName.substring(0, 4) === 'amp-';
+}
+
 /**
  * Returns `true` if the given action invocation is whitelisted in the given
  * whitelist. Default actions' alias, 'activate', are automatically
@@ -775,7 +779,7 @@ export class ActionService {
  * @return {boolean}
  * @private
  */
-function isActionWhitelisted_(invocation, whitelist) {
+function isActionWhitelisted(invocation, whitelist) {
   let {method} = invocation;
   const {node, tagOrTarget} = invocation;
   // Use alias if default action is invoked.

test/unit/test-action.js

@@ -869,20 +869,19 @@ describe('installActionHandler', () => {
     expect(callArgs.trust).to.be.equal(ActionTrust.HIGH);
   });
 
-  it('should check trust level before invoking action', () => {
+  it('should not check trust level (handler should check)', () => {
     const handlerSpy = sandbox.spy();
     const target = document.createElement('form');
-    action.installActionHandler(target, handlerSpy, ActionTrust.HIGH);
+    action.installActionHandler(target, handlerSpy);
 
-    action.invoke_(new ActionInvocation(target, 'submit', /* args */ null,
-        'button', 'button', 'tapEvent', ActionTrust.HIGH));
+    const invocation = new ActionInvocation(target, 'submit', /* args */ null,
+        'button', 'button', 'tapEvent', ActionTrust.HIGH);
+    action.invoke_(invocation);
     expect(handlerSpy).to.be.calledOnce;
 
-    return allowConsoleError(() => {
-      action.invoke_(new ActionInvocation(target, 'submit', /* args */ null,
-          'button', 'button', 'tapEvent', ActionTrust.LOW));
-      expect(handlerSpy).to.be.calledOnce;
-    });
+    invocation.trust = ActionTrust.LOW;
+    action.invoke_(invocation);
+    expect(handlerSpy).to.be.calledTwice;
   });
 });