Whitelist actions on the special AMP target #13192

hamousavi · 2018-01-31T21:26:11Z

If a whitelist of actions on AMP target is specified via a meta tag for example as follows
<meta name="amp-action-whitelist" content="setState,pushState">
then the runtime should respect it. In our example the runtime must reject the print, gotBack and navigateTo actions.

In order to do this, we parse the meta tag in StandardActions's constructor and cache the result. In handleAmpTarget we ensure that the given action is whitelisted before allowing it to trigger.

hamousavi · 2018-01-31T21:34:58Z

Also cc'ing @raywainman @zhangsu

zhangsu · 2018-02-05T22:32:18Z

src/service/standard-actions-impl.js

+
+      // Cache the whitelist of allowed AMP actions (if provided).
+      if (meta) {
+        /** @const @private {!Array<string>} */


Actually, given the current code this field should really be type-annotated as !Array<string>|undefined given that it's conditionally initialized. It might be better to extract this logic to a helper method, and use null as the default value.

dreamofabear · 2018-02-06T16:52:16Z

src/service/standard-actions-impl.js

+    // in its content attribute, a whitelist of actions on the special AMP target.
+    if (this.ampdoc.getRootNode() && this.ampdoc.getRootNode().head) {
+      const meta =
+        this.ampdoc.getRootNode().head


Hoist duplicate calls to a local var.

dreamofabear · 2018-02-06T16:53:05Z

src/service/standard-actions-impl.js

@@ -68,6 +68,21 @@ export class StandardActions {
    /** @const @private {!./viewport/viewport-impl.Viewport} */
    this.viewport_ = Services.viewportForDoc(ampdoc);

+    // A meta[name="amp-action-whitelist"] tag, if present, contains,
+    // in its content attribute, a whitelist of actions on the special AMP target.
+    if (this.ampdoc.getRootNode() && this.ampdoc.getRootNode().head) {


Let's lazily create this whitelist instead.

dreamofabear · 2018-02-06T16:54:16Z

src/service/standard-actions-impl.js

   */
  handleAmpTarget(invocation, opt_actionIndex, opt_actionInfos) {
    const method = invocation.method;
+    if (this.ampActionWhitelist_ &&
+      !this.ampActionWhitelist_.includes(method)) {


Let's include the target as well to disambiguate.

<meta name="amp-action-whitelist" content="AMP.setState, AMP.pushState">

dreamofabear

Is this feature meant to be valid AMP? If so, we need a validator change too since <meta> tags whose name starts with "amp" are whitelisted.

dreamofabear · 2018-02-06T16:58:44Z

test/functional/test-standard-actions.js

+          createElementWithAttributes(window.document, 'meta', {
+            name: 'amp-action-whitelist',
+            content: 'pushState,setState',
+          }));


This will mutate the test window which is shared by other tests. Instead, use stubs or describes.realWin to test in an iframe that will be teared down after the test.

dreamofabear · 2018-02-06T16:59:51Z

test/functional/test-standard-actions.js

+      return standardActions.handleAmpTarget(pushState, 0, []).then(result => {
+        expect(result).to.equal('push-state-complete');
+        expect(pushStateWithExpression).to.be.calledOnce;
+        expect(pushStateWithExpression).to.be.calledWith('{foo: 123}');


Simply stubbing/spying handleAmpBindAction_ should suffice. Let's not dupe test logic from other functions unless it pertains to the unit under test.

dreamofabear · 2018-02-06T17:05:01Z

src/service/standard-actions-impl.js

   */
  handleAmpTarget(invocation, opt_actionIndex, opt_actionInfos) {
    const method = invocation.method;
+    if (this.ampActionWhitelist_ &&
+      !this.ampActionWhitelist_.includes(method)) {
+      throw user().createError('AMP action', method, 'is not whitelisted');


"AMP.print" is not whitelisted in <meta name="amp-action-whitelist" content="AMP.setState">. See (link) for more details."

(link) should point to a new section in amp-actions-and-events.md.

Adding this meta tag makes the amp document invalid (as the insertion of meta tag is post validation). With this in mind, could you please clarify what should go in the documentation?

No documentation update then. 😄 Just want to make sure the error message is developer friendly.

@lannka This is the conversation I told you about

dreamofabear · 2018-02-06T17:06:47Z

Sorry for the late review, this slipped under my radar.

hamousavi

Thanks a lot Will. Regarding your previous comment, this feature is not meant to be valid AMP. This embedding of meta tag containing the whitelist will happen post validation.

hamousavi · 2018-02-07T16:21:47Z

test/functional/test-standard-actions.js

+      return standardActions.handleAmpTarget(pushState, 0, []).then(result => {
+        expect(result).to.equal('push-state-complete');
+        expect(pushStateWithExpression).to.be.calledOnce;
+        expect(pushStateWithExpression).to.be.calledWith('{foo: 123}');


zhangsu · 2018-02-09T04:58:31Z

src/service/standard-actions-impl.js

+   */
+  createWhitelist_() {
+    const head = this.ampdoc.getRootNode().head;
+    if (head) {


Consider exiting early here with the negative condition to make the precondition clear and the code below more width-efficient.

zhangsu · 2018-02-09T04:58:36Z

src/service/standard-actions-impl.js

+      const meta =
+        head.querySelector('meta[name="amp-action-whitelist"]');
+
+      if (meta) {


zhangsu · 2018-02-09T05:04:48Z

src/service/standard-actions-impl.js

    this.installActions_(this.actions_);
  }

+  /**
+   * @return {?Array<string>} the whitelist of allowed AMP actions


... on the special AMP target.

zhangsu · 2018-02-09T05:05:34Z

src/service/standard-actions-impl.js

    const method = invocation.method;
+    if (this.ampActionWhitelist_ &&
+      !this.ampActionWhitelist_.includes('AMP.' + method)) {


`AMP.${method}`

zhangsu · 2018-02-09T05:07:00Z

src/service/standard-actions-impl.js

    const method = invocation.method;
+    if (this.ampActionWhitelist_ &&
+      !this.ampActionWhitelist_.includes('AMP.' + method)) {
+      throw user().createError('AMP.${method}$ is not whitelisted.');


Don't you need a template string (escaped with the backtick) for string interpolation?

zhangsu · 2018-02-09T05:12:31Z

test/functional/test-standard-actions.js

+        getAttribute: () => 'AMP.pushState,AMP.setState',
+      };
+      sandbox.stub(window.document.head,
+          'querySelector').callsFake(() => fakeMeta);


I think I'd much prefer using describes.realWin to create a real <meta> tag and use real querySelector. The test would be much useful that way, since it actually tests the real thing that will be running under the code in prod.

zhangsu

LGTM, just one last set of suggestions...

zhangsu · 2018-02-09T22:38:55Z

test/functional/test-standard-actions.js

@@ -354,6 +354,57 @@ describes.sandboxed('StandardActions', {}, () => {
    });
  });

+  describes.realWin('#whitelist', {


I think #foo is usually for describing a method in the class under test. In this case, whitelist is not really a method, so we probably want to describe the behavior in English (i.e., drop the #).

zhangsu · 2018-02-09T22:43:34Z

src/service/standard-actions-impl.js

+   * special AMP target.
+   * @private
+   */
+  createWhitelist_() {


How about calling this method getAmpActionWhitelist, and return this.ampActionWhitelist_ if it is already cached (and update the JSDoc accordingly). That way, callers don't need to worry about this.ampActionWhitelist_ and can just call this.getAmpActionWhitelist_() everywhere.

zhangsu · 2018-02-09T22:45:13Z

src/service/standard-actions-impl.js

+   * the special AMP target, e.g.,
+   * <meta name="amp-action-whitelist" content="AMP.setState,AMP.pushState">
+   * @return {?Array<string>} the whitelist of actions on the
+   * special AMP target.


Doesn't the text here fit on the previous line? If not, this line should be indented 4 space, starting from the column where the @ on the previous line is.

zhangsu · 2018-02-09T22:46:57Z

src/service/standard-actions-impl.js

    const method = invocation.method;
+    if (this.ampActionWhitelist_ &&
+      !this.ampActionWhitelist_.includes(`AMP.${method}`)) {
+      throw user().createError(`AMP.${method}$ is not whitelisted.`);


What is the second $ doing?

hamousavi · 2018-02-13T16:58:42Z

Thanks a lot Su.

hamousavi · 2018-02-20T22:33:19Z

@choumx appreciate if you could take a look

dreamofabear · 2018-02-20T23:23:05Z

Try merging master to restart the presubmit. Also need to fix a minor import sorting issue.

hamousavi · 2018-02-23T16:29:53Z

Thanks @choumx. Fixed the lint error.

* respect the whitelist of actions specified in meta tags * fixed the failing tests * lint fix * Fixed some of the concerns in the review * Faking the window * ran lint --fix * simplifying the functional tests * more code review fixes * using real window in test * a few suggestions by Su * fixing import orders * fixed lint errors * removing const identifier

hamousavi added 2 commits January 31, 2018 15:20

respect the whitelist of actions specified in meta tags

2277522

fixed the failing tests

d68b396

googlebot added the cla: yes label Jan 31, 2018

lint fix

f1dd89b

dreamofabear added Type: Feature Request WG: runtime P2: Soon labels Jan 31, 2018

dreamofabear self-requested a review January 31, 2018 21:34

hamousavi mentioned this pull request Jan 31, 2018

amp runtime should respect the whitelist of actions specified in meta hamousavi/amphtml#2

Open

zhangsu reviewed Feb 5, 2018

View reviewed changes

dreamofabear requested changes Feb 6, 2018

View reviewed changes

dreamofabear reviewed Feb 6, 2018

View reviewed changes

dreamofabear mentioned this pull request Feb 6, 2018

Blacklist on attribute for <amp-story standalone> #11855

Closed

hamousavi added 4 commits February 6, 2018 17:53

Fixed some of the concerns in the review

3d5b478

Faking the window

08dcca5

ran lint --fix

96a0132

simplifying the functional tests

191e708

hamousavi commented Feb 7, 2018

View reviewed changes

zhangsu reviewed Feb 9, 2018

View reviewed changes

hamousavi added 2 commits February 9, 2018 16:28

more code review fixes

21362cd

using real window in test

792386e

zhangsu reviewed Feb 9, 2018

View reviewed changes

a few suggestions by Su

94ebeca

hamousavi mentioned this pull request Feb 14, 2018

Dedup the code for creation of whitelist from meta tags #13497

Closed

dreamofabear approved these changes Feb 20, 2018

View reviewed changes

hamousavi added 4 commits February 22, 2018 10:19

Merge branch 'master' into whitelist_actions

66a101c

fixing import orders

9d23e13

fixed lint errors

d064930

removing const identifier

2de1710

dreamofabear merged commit 19e6e78 into ampproject:master Feb 23, 2018

hamousavi mentioned this pull request Feb 28, 2018

Support whitelisting of AMP variable substitutions #13533

Merged

Whitelist actions on the special AMP target #13192

Whitelist actions on the special AMP target #13192

Conversation

hamousavi commented Jan 31, 2018 • edited by dreamofabear Loading

hamousavi commented Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Feb 6, 2018

hamousavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamousavi commented Feb 13, 2018

hamousavi commented Feb 20, 2018

dreamofabear commented Feb 20, 2018

hamousavi commented Feb 23, 2018

hamousavi commented Jan 31, 2018 •

edited by dreamofabear

Loading

hamousavi commented Jan 31, 2018 •

edited

Loading

dreamofabear Feb 6, 2018 •

edited

Loading