Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Create whitelist for amp-viewer-assistance.updateActionState actionStatus #21630

Merged
merged 3 commits into from
Apr 3, 2019
Merged

✨ Create whitelist for amp-viewer-assistance.updateActionState actionStatus #21630

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0e1627
Add actionStatus whitelist for amp-viewer-assistance.updateActionState.
hellokoji Mar 28, 2019
f241d85
update docs
hellokoji Mar 28, 2019
6700a1a
Address PR comments.
hellokoji Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions extensions/amp-viewer-assistance/0.1/amp-viewer-assistance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ const TAG = 'amp-viewer-assistance';
/** @const {string} */
const GSI_TOKEN_PROVIDER = 'actions-on-google-gsi';

/** @const {Array<string>} */
hellokoji marked this conversation as resolved.
Show resolved Hide resolved
const ACTION_STATUS_WHITELIST = [
'ACTIVE_ACTION_STATUS',
'FAILED_ACTION_STATUS',
'COMPLETED_ACTION_STATUS',
];

export class AmpViewerAssistance {
/**
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
Expand Down Expand Up @@ -70,7 +77,8 @@ export class AmpViewerAssistance {
*/
actionHandler_(invocation) {
const {method, args} = invocation;
if (method == 'updateActionState' && !!args) {
if (method == 'updateActionState' && args
&& this.isValidActionStatusArgs_(args)) {
hellokoji marked this conversation as resolved.
Show resolved Hide resolved
this.viewer_./*OK*/sendMessageAwaitResponse(method, args).catch(error => {
user().error(TAG, error.toString());
});
Expand Down Expand Up @@ -148,6 +156,32 @@ 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'];
if (!update || !update['actionStatus']) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not sure we support object literals outside of AMP.setState (syntax). What would this look like in HTML?

<!-- Is this the intention? -->
<button on="tap:amp-viewer-assistance.updateActionStatus(update={actionStatus: ACTIVE_ACTION_STATUS})">

<!-- 
  Or maybe we can do this (case-insensitive)? 
  Uppercase and append "_ACTION_STATUS" before sending viewer message. 
-->
<button on="tap:amp-viewer-assistance.updateActionStatus(status=active)">

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first example is how it's usually implemented except with the payload being passed from an event:

<form id="my-form"
      action-xhr="some/XHR"
      on="submit-success:amp-viewer-assistance.updateActionState(update=event.response.result)">
 </form>

It's also important to note that the update payload can include additional data in another field (usually result), but it is too complex to validate on the run-time side.

{
  'actionStatus': 'COMPLETED_ACTION_STATUS',
  'result': { ... }
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks.

user().error(TAG, 'Invalid arguments for updateActionState! Must have' +
' an "update" object with an "actionStatus" field.');
return false;
}

if (!ACTION_STATUS_WHITELIST.includes(update['actionStatus'])) {
hellokoji marked this conversation as resolved.
Show resolved Hide resolved
user().error(TAG, 'Invalid actionStatus for updateActionState! '
+ update['actionStatus']);
return false;
}

user().info(TAG, 'Sending actionStatus: ' + update['actionStatus']);
return true;
}

/**
* Toggles the CSS classes related to the status of the identity token.
* @private
Expand Down Expand Up @@ -178,7 +212,6 @@ export class AmpViewerAssistance {
this.vsync_.mutate(() => {
this.getRootElement_().classList.toggle(className, on);
});

}
}

Expand Down
106 changes: 75 additions & 31 deletions extensions/amp-viewer-assistance/0.1/test/test-amp-viewer-assistance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});
});

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-viewer-assistance/amp-viewer-assistance.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ 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. Should contain an <code>update</code> object as well as an <code>actionStatus</code> field denoting the resulting state change. Supported <code>actionStatus</code> strings are <code>ACTIVE_ACTION_STATUS</code>, <code>FAILED_ACTION_STATUS</code>, and <code>COMPLETED_ACTION_STATUS</code>.</td>
hellokoji marked this conversation as resolved.
Show resolved Hide resolved
</tr>
</table>

Expand Down