From 244e5c83e6385cb455d6af6711e6db8e04d8fdf0 Mon Sep 17 00:00:00 2001 From: hamousavi Date: Tue, 30 Jan 2018 10:37:58 -0500 Subject: [PATCH 1/4] amp runtime should respect the whitelist of actions specified in meta tags --- src/service/standard-actions-impl.js | 13 ++++- test/functional/test-standard-actions.js | 61 ++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/service/standard-actions-impl.js b/src/service/standard-actions-impl.js index 950fd62e9211..81e80843fa32 100644 --- a/src/service/standard-actions-impl.js +++ b/src/service/standard-actions-impl.js @@ -68,6 +68,13 @@ export class StandardActions { /** @const @private {!./viewport/viewport-impl.Viewport} */ this.viewport_ = Services.viewportForDoc(ampdoc); + const meta = + window.document.head.querySelector('meta[name="amp-action-whitelist"]'); + if (meta) { + /** @const @private {Array} */ + this.ampActionWhitelist_ = meta.getAttribute('content').split(','); + } + this.installActions_(this.actions_); } @@ -101,10 +108,14 @@ export class StandardActions { * @param {number=} opt_actionIndex * @param {!Array=} opt_actionInfos * @return {?Promise} - * @throws {Error} If action is not recognized. + * @throws {Error} If action is not recognized or is not whitelisted. */ handleAmpTarget(invocation, opt_actionIndex, opt_actionInfos) { const method = invocation.method; + if (this.ampActionWhitelist_ && + this.ampActionWhitelist_.indexOf(method) == -1) { + throw user().createError('AMP action', method, 'is not whitelisted'); + } switch (method) { case 'pushState': case 'setState': diff --git a/test/functional/test-standard-actions.js b/test/functional/test-standard-actions.js index 922fe1e1ff69..5244a3797d28 100644 --- a/test/functional/test-standard-actions.js +++ b/test/functional/test-standard-actions.js @@ -20,6 +20,7 @@ import {StandardActions} from '../../src/service/standard-actions-impl'; import {Services} from '../../src/services'; import {installHistoryServiceForDoc} from '../../src/service/history-impl'; import {setParentWindow} from '../../src/service'; +import {createElementWithAttributes} from '../../src/dom'; describes.sandboxed('StandardActions', {}, () => { @@ -352,6 +353,66 @@ describes.sandboxed('StandardActions', {}, () => { standardActions.handleAmpTarget(invocation); expect(printStub).to.be.calledOnce; }); + + it('should not implement print when not whitelisted', () => { + window.document.head.appendChild( + createElementWithAttributes(window.document, 'meta', { + name: 'amp-action-whitelist', + content: 'pushState,setState', + })); + + ampdoc = new AmpDocSingle(window); + standardActions = new StandardActions(ampdoc); + + const windowApi = { + print: () => {}, + }; + const printStub = sandbox.stub(windowApi, 'print'); + const invocation = { + method: 'print', + satisfiesTrust: () => true, + target: { + ownerDocument: { + defaultView: windowApi, + }, + }, + }; + expect(() => {standardActions.handleAmpTarget(invocation);}).to.throw(); + expect(printStub).to.not.be.called; + }); + + it('should implement psuhState when whitelisted', () => { + window.document.head.appendChild( + createElementWithAttributes(window.document, 'meta', { + name: 'amp-action-whitelist', + content: 'pushState', + })); + + ampdoc = new AmpDocSingle(window); + standardActions = new StandardActions(ampdoc); + + const pushStateWithExpression = sandbox.stub(); + // Bind.pushStateWithExpression() doesn't resolve with a value, + // but add one here to check that the promise is chained. + pushStateWithExpression.returns(Promise.resolve('push-state-complete')); + + window.services.bind = { + obj: {pushStateWithExpression}, + }; + + const args = { + [OBJECT_STRING_ARGS_KEY]: '{foo: 123}', + }; + const target = ampdoc; + const satisfiesTrust = () => true; + const pushState = {method: 'pushState', args, target, satisfiesTrust}; + + 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}'); + }); + }); }); describes.fakeWin('adoptEmbedWindow', {}, env => { From 7ecac68fe3b778fc9637bcb194bcc7ec9a356b27 Mon Sep 17 00:00:00 2001 From: hamousavi Date: Tue, 30 Jan 2018 10:53:43 -0500 Subject: [PATCH 2/4] trims the name of actions --- src/service/standard-actions-impl.js | 3 ++- test/functional/test-standard-actions.js | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/service/standard-actions-impl.js b/src/service/standard-actions-impl.js index 81e80843fa32..beab420edc96 100644 --- a/src/service/standard-actions-impl.js +++ b/src/service/standard-actions-impl.js @@ -72,7 +72,8 @@ export class StandardActions { window.document.head.querySelector('meta[name="amp-action-whitelist"]'); if (meta) { /** @const @private {Array} */ - this.ampActionWhitelist_ = meta.getAttribute('content').split(','); + this.ampActionWhitelist_ = meta.getAttribute('content').split(',') + .map(action => {return action.trim();}); } this.installActions_(this.actions_); diff --git a/test/functional/test-standard-actions.js b/test/functional/test-standard-actions.js index 5244a3797d28..36fcaa0f18b3 100644 --- a/test/functional/test-standard-actions.js +++ b/test/functional/test-standard-actions.js @@ -381,11 +381,11 @@ describes.sandboxed('StandardActions', {}, () => { expect(printStub).to.not.be.called; }); - it('should implement psuhState when whitelisted', () => { + it('should implement pushState when whitelisted', () => { window.document.head.appendChild( createElementWithAttributes(window.document, 'meta', { name: 'amp-action-whitelist', - content: 'pushState', + content: 'setState, pushState', })); ampdoc = new AmpDocSingle(window); From 668519d92f119a9f4bf3d2fc5bdc7a15f1570442 Mon Sep 17 00:00:00 2001 From: hamousavi Date: Tue, 30 Jan 2018 17:01:34 -0500 Subject: [PATCH 3/4] adding a bit of documentation --- src/service/standard-actions-impl.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/service/standard-actions-impl.js b/src/service/standard-actions-impl.js index beab420edc96..20684ce723f0 100644 --- a/src/service/standard-actions-impl.js +++ b/src/service/standard-actions-impl.js @@ -68,8 +68,11 @@ 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. const meta = window.document.head.querySelector('meta[name="amp-action-whitelist"]'); + // Cache the whitelist of allowed AMP actions (if provided). if (meta) { /** @const @private {Array} */ this.ampActionWhitelist_ = meta.getAttribute('content').split(',') From c4a40c47c51774fea55346b2c5bfbc68dfa53e3c Mon Sep 17 00:00:00 2001 From: hamousavi Date: Wed, 31 Jan 2018 11:53:37 -0500 Subject: [PATCH 4/4] slight modifications --- src/service/standard-actions-impl.js | 8 ++++---- test/functional/test-standard-actions.js | 4 +--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/service/standard-actions-impl.js b/src/service/standard-actions-impl.js index 20684ce723f0..d2e82f424116 100644 --- a/src/service/standard-actions-impl.js +++ b/src/service/standard-actions-impl.js @@ -71,12 +71,12 @@ export class StandardActions { // A meta[name="amp-action-whitelist"] tag, if present, contains, // in its content attribute, a whitelist of actions on the special AMP target. const meta = - window.document.head.querySelector('meta[name="amp-action-whitelist"]'); + this.ampdoc.getRootNode().head.querySelector('meta[name="amp-action-whitelist"]'); // Cache the whitelist of allowed AMP actions (if provided). if (meta) { - /** @const @private {Array} */ + /** @const @private {!Array} */ this.ampActionWhitelist_ = meta.getAttribute('content').split(',') - .map(action => {return action.trim();}); + .map(action => action.trim()); } this.installActions_(this.actions_); @@ -117,7 +117,7 @@ export class StandardActions { handleAmpTarget(invocation, opt_actionIndex, opt_actionInfos) { const method = invocation.method; if (this.ampActionWhitelist_ && - this.ampActionWhitelist_.indexOf(method) == -1) { + !this.ampActionWhitelist_.includes(method)) { throw user().createError('AMP action', method, 'is not whitelisted'); } switch (method) { diff --git a/test/functional/test-standard-actions.js b/test/functional/test-standard-actions.js index 36fcaa0f18b3..c08ac0a57f0d 100644 --- a/test/functional/test-standard-actions.js +++ b/test/functional/test-standard-actions.js @@ -361,7 +361,6 @@ describes.sandboxed('StandardActions', {}, () => { content: 'pushState,setState', })); - ampdoc = new AmpDocSingle(window); standardActions = new StandardActions(ampdoc); const windowApi = { @@ -377,7 +376,7 @@ describes.sandboxed('StandardActions', {}, () => { }, }, }; - expect(() => {standardActions.handleAmpTarget(invocation);}).to.throw(); + expect(() => standardActions.handleAmpTarget(invocation)).to.throw(); expect(printStub).to.not.be.called; }); @@ -388,7 +387,6 @@ describes.sandboxed('StandardActions', {}, () => { content: 'setState, pushState', })); - ampdoc = new AmpDocSingle(window); standardActions = new StandardActions(ampdoc); const pushStateWithExpression = sandbox.stub();