Skip to content

Commit

Permalink
Properly type event.data
Browse files Browse the repository at this point in the history
…to ensure the data is considered an external `JsonObject`. Forces the use of a `getData` function instead of accessing the `.data` property directly. The function returns the proper type.

Part of ampproject#9876
  • Loading branch information
cramforce committed Jun 19, 2017
1 parent 065ee9b commit 57e6996
Show file tree
Hide file tree
Showing 28 changed files with 166 additions and 112 deletions.
6 changes: 3 additions & 3 deletions 3p/ampcontext.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ export class AbstractAmpContext {
*/
onPageVisibilityChange(callback) {
return this.client_.registerCallback(MessageType.EMBED_STATE, data => {
callback({hidden: data.pageHidden});
callback({hidden: data['pageHidden']});
});
}

Expand Down Expand Up @@ -194,7 +194,7 @@ export class AbstractAmpContext {
*/
onResizeSuccess(callback) {
this.client_.registerCallback(MessageType.EMBED_SIZE_CHANGED, obj => {
callback(obj.requestedHeight, obj.requestedWidth); });
callback(obj['requestedHeight'], obj['requestedWidth']); });
};

/**
Expand All @@ -206,7 +206,7 @@ export class AbstractAmpContext {
*/
onResizeDenied(callback) {
this.client_.registerCallback(MessageType.EMBED_SIZE_DENIED, obj => {
callback(obj.requestedHeight, obj.requestedWidth);
callback(obj['requestedHeight'], obj['requestedWidth']);
});
};

Expand Down
7 changes: 4 additions & 3 deletions 3p/iframe-messaging-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
serializeMessage,
deserializeMessage,
} from '../src/3p-frame-messaging';
import {getData} from '../src/event-helper';
import {getMode} from '../src/mode';
import {dev} from '../src/log';

Expand Down Expand Up @@ -66,7 +67,7 @@ export class IframeMessagingClient {
* All future calls will overwrite any previously registered
* callbacks.
* @param {string} messageType The type of the message.
* @param {function(Object)} callback The callback function to call
* @param {function(?JsonObject)} callback The callback function to call
* when a message with type messageType is received.
*/
registerCallback(messageType, callback) {
Expand Down Expand Up @@ -105,7 +106,7 @@ export class IframeMessagingClient {
return;
}

const message = deserializeMessage(event.data);
const message = deserializeMessage(getData(event));
if (!message || message['sentinel'] != this.sentinel_) {
return;
}
Expand All @@ -130,7 +131,7 @@ export class IframeMessagingClient {

/**
* @param {string} messageType
* @return {!Observable<Object>}
* @return {!Observable<?JsonObject>}
*/
getOrCreateObservableFor_(messageType) {
if (!(messageType in this.observableFor_)) {
Expand Down
14 changes: 7 additions & 7 deletions 3p/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,8 @@ function getHtml(selector, attributes, callback) {
}));

const unlisten = listenParent(window, 'get-html-result', data => {
if (data.messageId === messageId) {
callback(data.content);
if (data['messageId'] === messageId) {
callback(data['content']);
unlisten();
}
});
Expand All @@ -610,7 +610,7 @@ function observeIntersection(observerCallback) {
// Send request to received records.
nonSensitiveDataPostMessage('send-intersections');
return listenParent(window, 'intersection', data => {
observerCallback(data.changes);
observerCallback(data['changes']);
});
}

Expand All @@ -621,8 +621,8 @@ function observeIntersection(observerCallback) {
*/
function updateVisibilityState(global) {
listenParent(window, 'embed-state', function(data) {
global.context.hidden = data.pageHidden;
dispatchVisibilityChangeEvent(global, data.pageHidden);
global.context.hidden = data['pageHidden'];
dispatchVisibilityChangeEvent(global, data['pageHidden']);
});
}

Expand All @@ -642,7 +642,7 @@ function dispatchVisibilityChangeEvent(win, isHidden) {
*/
function onResizeSuccess(observerCallback) {
return listenParent(window, 'embed-size-changed', data => {
observerCallback(data.requestedHeight, data.requestedWidth);
observerCallback(data['requestedHeight'], data['requestedWidth']);
});
}

Expand All @@ -654,7 +654,7 @@ function onResizeSuccess(observerCallback) {
*/
function onResizeDenied(observerCallback) {
return listenParent(window, 'embed-size-denied', data => {
observerCallback(data.requestedHeight, data.requestedWidth);
observerCallback(data['requestedHeight'], data['requestedWidth']);
});
}

Expand Down
13 changes: 8 additions & 5 deletions 3p/messaging.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import {parseJson} from '../src/json';
import {getData} from '../src/event-helper';

/**
* Send messages to parent frame. These should not contain user data.
Expand All @@ -34,15 +35,15 @@ export function nonSensitiveDataPostMessage(type, opt_object) {

/**
* Message event listeners.
* @const {!Array<{type: string, cb: function(!Object)}>}
* @const {!Array<{type: string, cb: function(!JsonObject)}>}
*/
const listeners = [];

/**
* Listen to message events from document frame.
* @param {!Window} win
* @param {string} type Type of messages
* @param {function(*)} callback Called with data payload of message.
* @param {function(!JsonObject)} callback Called with data payload of message.
* @return {function()} function to unlisten for messages.
*/
export function listenParent(win, type, callback) {
Expand Down Expand Up @@ -72,14 +73,16 @@ function startListening(win) {
win.AMP_LISTENING = true;
win.addEventListener('message', function(event) {
// Cheap operations first, so we don't parse JSON unless we have to.
const eventData = getData(event);
if (event.source != win.parent ||
event.origin != win.context.location.origin ||
typeof event.data != 'string' ||
event.data.indexOf('amp-') != 0) {
typeof eventData != 'string' ||
eventData.indexOf('amp-') != 0) {
return;
}
// Parse JSON only once per message.
const data = parseJson(event.data.substr(4));
const data = /** @type {!JsonObject} */ (
parseJson(/**@type {string} */ (getData(event)).substr(4)));
if (win.context.sentinel && data['sentinel'] != win.context.sentinel) {
return;
}
Expand Down
5 changes: 3 additions & 2 deletions ads/inabox/inabox-messaging-host.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
MessageType,
} from '../../src/3p-frame-messaging';
import {dev} from '../../src/log';
import {getData} from '../../src/event-helper';
import {dict} from '../../src/utils/object';
import {expandFrame, collapseFrame} from './frame-overlay-helper';

Expand Down Expand Up @@ -90,11 +91,11 @@ export class InaboxMessagingHost {
* {type: string, sentinel: string}. The allowed types are listed in the
* REQUEST_TYPE enum.
*
* @param message {!{data: *, source: !Window, origin: string}}
* @param {!MessageEvent} message
* @return {boolean} true if message get successfully processed
*/
processMessage(message) {
const request = deserializeMessage(message.data);
const request = deserializeMessage(getData(message));
if (!request || !request['sentinel']) {
dev().fine(TAG, 'Ignored non-AMP message:', message);
return false;
Expand Down
16 changes: 16 additions & 0 deletions build-system/conformance-config.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,22 @@ requirement: {
whitelist: 'src/json.js' # Where jsonParse itself is implemented.
}

requirement: {
type: BANNED_PROPERTY_READ
error_message: 'Use eventHelper#getData to read the data property. OK to whitelist 3p ads for now.'
value: 'Event.prototype.data'
value: 'MessageEvent.prototype.data'
whitelist: 'src/event-helper.js' # getData is implemented here
whitelist: 'src/web-worker/amp-worker.js' # OK to use in version bound worker
whitelist: 'src/web-worker/web-worker.js' # OK to use in version bound worker
# 3p ads are OK
whitelist: 'ads/adfox.js'
whitelist: 'ads/google/imaVideo.js'
whitelist: 'ads/netletix.js'
whitelist: 'ads/yandex.js'
}

requirement: {
type: RESTRICTED_METHOD_CALL
error_message: 'postMessage must be called with a string or a JsonObject'
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-ad/0.1/a2a-listener.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import {closestByTag} from '../../../src/dom';
import {isExperimentOn} from '../../../src/experiments';
import {getData} from '../../../src/event-helper';
import {user} from '../../../src/log';
import {viewerForDoc} from '../../../src/services';
import {isProxyOrigin} from '../../../src/url';
Expand Down Expand Up @@ -45,7 +46,7 @@ export function setupA2AListener(win) {
* @visibleForTesting
*/
export function handleMessageEvent(win, event) {
const data = event.data;
const data = getData(event);
// Only handle messages starting with the magic string.
if (typeof data != 'string' || data.indexOf('a2a;') != 0) {
return;
Expand Down
18 changes: 10 additions & 8 deletions extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {dev} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {timerFor} from '../../../src/services';
import {setStyle} from '../../../src/style';
import {loadPromise} from '../../../src/event-helper';
import {getData, loadPromise} from '../../../src/event-helper';
import {getHtml} from '../../../src/get-html';
import {removeElement} from '../../../src/dom';
import {getServiceForDoc} from '../../../src/service';
Expand Down Expand Up @@ -136,7 +136,7 @@ export class AmpAdXOriginIframeHandler {
// iframe.
listenForOncePromise(this.iframe, 'entity-id', true)
.then(info => {
this.element_.creativeId = info.data.id;
this.element_.creativeId = info.data['id'];
});

this.unlisteners_.push(listenFor(this.iframe, 'get-html',
Expand All @@ -145,7 +145,9 @@ export class AmpAdXOriginIframeHandler {
return;
}

const {selector, attributes, messageId} = info;
const selector = info['selector'];
const attributes = info['attributes'];
const messageId = info['messageId'];
let content = '';

if (this.element_.hasAttribute('data-html-access-allowed')) {
Expand All @@ -164,7 +166,7 @@ export class AmpAdXOriginIframeHandler {
// Install iframe resize API.
this.unlisteners_.push(listenFor(this.iframe, 'embed-size',
(data, source, origin) => {
this.handleResize_(data.height, data.width, source, origin);
this.handleResize_(data['height'], data['width'], source, origin);
}, true, true));

this.unlisteners_.push(this.viewer_.onVisibilityChanged(() => {
Expand Down Expand Up @@ -204,7 +206,7 @@ export class AmpAdXOriginIframeHandler {
listenForOncePromise(this.iframe,
['render-start', 'no-content'], true).then(info => {
const data = info.data;
if (data.type == 'render-start') {
if (data['type'] == 'render-start') {
this.renderStart_(info);
renderStartResolve();
} else {
Expand Down Expand Up @@ -273,17 +275,17 @@ export class AmpAdXOriginIframeHandler {

/**
* callback functon on receiving render-start
* @param {!Object=} opt_info
* @param {{data: !JsonObject}=} opt_info
* @private
*/
renderStart_(opt_info) {
this.baseInstance_.renderStarted();
if (!opt_info) {
return;
}
const data = opt_info.data;
const data = getData(opt_info);
this.handleResize_(
data.height, data.width, opt_info.source, opt_info.origin);
data['height'], data['width'], opt_info['source'], opt_info['origin']);
if (this.baseInstance_.emitLifecycleEvent) {
this.baseInstance_.emitLifecycleEvent('renderCrossDomainStart');
}
Expand Down
7 changes: 4 additions & 3 deletions extensions/amp-brid-player/0.1/amp-brid-player.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {VideoEvents} from '../../../src/video-interface';
import {videoManagerForDoc} from '../../../src/services';
import {assertAbsoluteHttpOrHttpsUrl} from '../../../src/url';
import {removeElement} from '../../../src/dom';
import {listen} from '../../../src/event-helper';
import {getData, listen} from '../../../src/event-helper';

/**
* @implements {../../../src/video-interface.VideoInterface}
Expand Down Expand Up @@ -216,13 +216,14 @@ class AmpBridPlayer extends AMP.BaseElement {

/** @private */
handleBridMessages_(event) {
const eventData = /** @type {?string|undefined} */ (getData(event));
if (event.origin !== 'https://services.brid.tv' ||
event.source != this.iframe_.contentWindow ||
typeof event.data !== 'string' || event.data.indexOf('Brid') !== 0) {
typeof eventData !== 'string' || eventData.indexOf('Brid') !== 0) {
return;
}

const params = event.data.split('|');
const params = eventData.split('|');

if (params[2] == 'trigger') {
if (params[3] == 'ready') {
Expand Down
6 changes: 3 additions & 3 deletions extensions/amp-dailymotion/0.1/amp-dailymotion.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {VideoEvents} from '../../../src/video-interface';
import {
installVideoManagerForDoc,
} from '../../../src/service/video-manager-impl';
import {listen} from '../../../src/event-helper';
import {getData, listen} from '../../../src/event-helper';
import {videoManagerForDoc} from '../../../src/services';
import {parseQueryString} from '../../../src/url';

Expand Down Expand Up @@ -185,10 +185,10 @@ class AmpDailymotion extends AMP.BaseElement {
event.source != this.iframe_.contentWindow) {
return;
}
if (!event.data || !event.type || event.type != 'message') {
if (!getData(event) || !event.type || event.type != 'message') {
return; // Event empty
}
const data = parseQueryString(event.data);
const data = parseQueryString(/** @type {string} */ (getData(event)));
if (data === undefined) {
return; // The message isn't valid
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class AmpFacebookComments extends AMP.BaseElement {
this.applyFillContent(iframe);
// Triggered by context.updateDimensions() inside the iframe.
listenFor(iframe, 'embed-size', data => {
this./*OK*/changeHeight(data.height);
this./*OK*/changeHeight(data['height']);
}, /* opt_is3P */true);
this.element.appendChild(iframe);
this.iframe_ = iframe;
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-facebook-like/0.1/amp-facebook-like.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class AmpFacebookLike extends AMP.BaseElement {
this.applyFillContent(iframe);
// Triggered by context.updateDimensions() inside the iframe.
listenFor(iframe, 'embed-size', data => {
this.attemptChangeHeight(data.height).catch(() => {
this.attemptChangeHeight(data['height']).catch(() => {
/* ignore failures */
});
}, /* opt_is3P */true);
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-facebook/0.1/amp-facebook.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class AmpFacebook extends AMP.BaseElement {
this.applyFillContent(iframe);
// Triggered by context.updateDimensions() inside the iframe.
listenFor(iframe, 'embed-size', data => {
this./*OK*/changeHeight(data.height);
this./*OK*/changeHeight(data['height']);
}, /* opt_is3P */true);
this.element.appendChild(iframe);
this.iframe_ = iframe;
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-gist/0.1/amp-gist.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export class AmpGist extends AMP.BaseElement {
this.applyFillContent(iframe);
// Triggered by window.context.requestResize() inside the iframe.
listenFor(iframe, 'embed-size', data => {
this./*OK*/changeHeight(data.height);
this./*OK*/changeHeight(data['height']);
}, /* opt_is3P */true);

this.element.appendChild(iframe);
Expand Down
Loading

0 comments on commit 57e6996

Please sign in to comment.