Change how AMP inabox validates that the source of a postMessage is valid #12541

bradfrizzell · 2017-12-20T16:54:10Z

This PR separates out how amp inabox 1. validates that the source of a post message is valid and 2. returns which iframe on the page should be measured. Before, it always looked to measure the direct child frame of the window level at which the host script runs. After this change, it now:

Checks that the source of the postMessage contains one of the registered amp inabox iframes in its parent chain
Either returns the source iframe if it is measureable, or returns whichever xdomain frame of the source's parent hierarchy is measureable.

keithwrightbos · 2017-12-20T16:56:39Z

ads/inabox/inabox-messaging-host.js

+    let tempWin = source.parent;
+    while (tempWin !== this.win_ && tempWin !== this.win_.top) {
+      if (tempWin.location.origin !== this.win_.location.origin) {
+        return null;


Can we dev().warn here? Typically we've determined if a frame is cross by attempting to access state within it as opposed to looking at origin.

keithwrightbos · 2017-12-20T17:00:24Z

ads/inabox/inabox-messaging-host.js

+    // If there is, the host doc will not be able to accurately measure
+    // the creative's positions in the page, so return null.
+    let tempWin = source.parent;
+    while (tempWin !== this.win_ && tempWin !== this.win_.top) {


Let's limit the amount of iterations (say 10) as we've seen situations where somehow windows get into an infinite loop. If we hit the limit, return null.

jonkeller · 2017-12-21T20:08:26Z

Nit: in intro comment, change <iframe src="origin1.biz"> to <iframe src="https://origin1.biz"> so it doesn't look like you're looking for a relative URL of a file called origin1.biz

jonkeller · 2017-12-21T20:11:18Z

src/3p-frame-messaging.js

+  }
+}
+
+function canTouchProperty(obj, prop) {


Nit: Comment describing method

keithwrightbos · 2017-12-22T16:46:08Z

ads/inabox/inabox-messaging-host.js

+    // If there is, the host doc will not be able to accurately measure
+    // the creative's positions in the page, so return null.
+    // Limit to 10 iterations.
+    let tempWin = source.parent;


Let's only do this if the frame owning the message exists in the array. This will potentially save CPU cycles.

keithwrightbos · 2017-12-22T16:47:36Z

src/3p-frame-messaging.js

+ * @param {Window} win
+ * @return {boolean}
+ */
+export function canInspectWindow(win) {


IMO, only centralize code if and when there are more than two consumers but your call. If you do decide to leave here, you need explicit test coverage.

keithwrightbos · 2017-12-22T18:55:11Z

ads/inabox/inabox-messaging-host.js

+	  // Limit to 10 iterations.
+	  let tempWin = source.parent;
+	  for (let i = 0; i < 10; i++) {
+	      if (tempWin == this.win_ || tempWin == this.win_.top) {


Is this indentation correct?

lint says so.... but I'll try something else!

keithwrightbos · 2017-12-22T18:56:06Z

ads/inabox/inabox-messaging-host.js

+	  // the creative's positions in the page, so return null.
+	  // Limit to 10 iterations.
+	  let tempWin = source.parent;
+	  for (let i = 0; i < 10; i++) {


You can move the if break statement directly into the for loop declaration, I also think we should make sure tempWin exists:

for (let i = 0; i < 10 && tempWin && tempWin != this.win_ && tempWin != this.win_.top; i++) {

BTW if you want to be really fancy you can include the declaration of tempWin:

for (let i = 0, tempWin = source.parent; i < 10 && tempWin && tempWin != this.win_ && tempWin != this.win_.top; i++, tempWin = source.parent) {
if (!canInspectWindow(tempWin)) {
return null;
}
}

Now that I think of it, your loop is not properly returning null when we hit the more than 10 case, so really this should be:

for (let i = 0, tempWin = source.parent; tempWin && tempWin != this.win_ && tempWin != this.win_.top; i++, tempWin = source.parent) {
if (i == 10 || !canInspectWindow(tempWin)) return null;
}

keithwrightbos · 2017-12-22T18:57:33Z

ads/inabox/inabox-messaging-host.js

+		  break;
+	      }
+	      if (!canInspectWindow(tempWin)) {
+		  return null;


let's dev().warn here preferably with actionable info though I suppose we expect it in the passback case though we are indicating in the ad request that inabox is not eligible. I still wonder what we should do about this case as we need to ensure we meet the same level of viewability measurement support.

keithwrightbos · 2017-12-22T19:00:16Z

ads/inabox/inabox-messaging-host.js

+ */
+function canInspectWindow(win) {
+  try {
+    return !!win && win.location.href != null && canTouchProperty(win, 'test');


I think this can just be the following. If win or win location do not exist, it will throw which is equivalent:

try {
const /* eslint no-unused-vars: 0 */ unused = win.location.href && win['test'];
return true;
} catch (unusedErr) {
return false;
}

keithwrightbos · 2017-12-22T19:23:26Z

ads/inabox/inabox-messaging-host.js

+	  // the creative's positions in the page, so return null.
+	  // Limit to 10 iterations.
+	  let tempWin = source.parent;
+	  for (let i = 0; i < 10; i++) {


BTW if you want to be really fancy you can include the declaration of tempWin:

for (let i = 0, tempWin = source.parent; i < 10 && tempWin && tempWin != this.win_ && tempWin != this.win_.top; i++, tempWin = source.parent) {
if (!canInspectWindow(tempWin)) {
return null;
}
}

Now that I think of it, your loop is not properly returning null when we hit the more than 10 case, so really this should be:

for (let i = 0, tempWin = source.parent; tempWin && tempWin != this.win_ && tempWin != this.win_.top; i++, tempWin = source.parent) {
if (i == 10 || !canInspectWindow(tempWin)) return null;
}

lannka · 2018-01-02T21:40:08Z

Currently, the AMP inabox host script does not allow messages to be processed from iframes that are nested, i.e. only a direct child frame of the window level at which the host script is running are allowed.

Didn't look into the code, but the above description isn't very accurate.

The host script does allow messages from nested iframe.

host -> iframe 1 -> iframe 2 -> iframe 3

say if host receives a message from iframe3, it will try to locate iframe 1 by walking up in the window tree.

it will reject the message only if iframe1 is not in the array. that's to say, if you want to accept the message, the ads tag should add iframe1 element to the array.

lannka

As discussed offline, we can either fix the issue in ads tag side, by always registering the direct child iframe.

Or, we can make the host script more error tolerant, by accepting registration of any level of nested iframe, and measuring the closest friendly iframe ancestor of the message source window.

keithwrightbos · 2018-01-11T17:00:54Z

ads/inabox/inabox-messaging-host.js

+   * is found. Then, it returns the frame element for that window.
+   * For when win is friendly framed, returns the frame element for win.
+   * @param {!Window} win
+   * @return {?Element}


Add @VisibleForTesting

keithwrightbos · 2018-01-11T17:01:50Z

ads/inabox/inabox-messaging-host.js

+  getMeasureableFrame(win) {
+    let measureableWin;
+    let measureableFrame = win.frameElement;
+    for (let j = 0, tempWin = win; j < 5 && tempWin != tempWin.top;


Why limit to 5? I think 10 is a more reasonable value

The reason that I limited it to 5 was because the previous design iteration you'd suggested limited to a total depth of 10. The way this is designed now splits up the way we check the depth into being a check of how deeply nested the source frame is from the top-most xdomain frame, and how deeply nested the top-most xdomain frame is from window.top. This 5 here was putting a limit on the depth of the source in regards to the top-most x-domain frame. Fine to move it to 10

keithwrightbos · 2018-01-11T17:02:18Z

ads/inabox/inabox-messaging-host.js

+    let measureableFrame = win.frameElement;
+    for (let j = 0, tempWin = win; j < 5 && tempWin != tempWin.top;
+      j++, tempWin = tempWin.parent) {
+      if (!canInspectWindow(tempWin)) {


Let's invert these so you don't have a not condition

keithwrightbos · 2018-01-11T17:18:05Z

ads/inabox/inabox-messaging-host.js

+    if (!!measureableWin) {
+      const parent = measureableWin.parent;
+      const iframes = parent.document.querySelectorAll('iframe');
+      for (let k = 0, frame = iframes[k]; k < iframes.length;


Some code golf for you, perhaps wrapping in Array is bad for performance... not sure:
Array(measureableWin.parent.document.getElementsByTagName('iframe')).forEach(iframe => frame.contentWindow == measureableWin && measureableFrame = frame);

gonna meet you halfway on this, and still use querySelectorAll as that already returns a nodelist, and find instead of forEach so that we only run until we actually find the right frame.

actually nvm can't do find on a nodelist. Was gonna get overly complicated. Left with a regular for loop and made sure it terminates as soon as we find the right frame.

keithwrightbos · 2018-01-11T17:19:03Z

ads/inabox/inabox-messaging-host.js

+ * This functioned is used to determine if a window is cross-domained
+ * from the perspective of the current window.
+ * @param {Window} win
+ * @return {boolean}


Make private?

lannka · 2018-01-11T22:50:14Z

ads/inabox/inabox-messaging-host.js

+        }
+      }
+    }
+    return win.frameElement;


i'm very confused. did you mean measuarableWin.frameElement?

in fact, can we just do:

function getMeasureableFrame(win) { if (win.parent === win) { return null; } if (win.frameElement) { return win.frameElement; } return getMeasureableFrame(win.parent); }

I should add more comments explaining why this is the way it is as I agree it's pretty confusing

The reason that this is so complicated, is that if you have a window reference to a xdomain frame, you can not do win.frameElement, even if you actually do have access to that window's frame element. So, instead we have to find the top-most x-domain window, then go up to its parent, and query select on that parent's document for all its child iframes, and then loop over them to find the correct frame element by comparing the frame element's contentWindow to the xdomain window we care about.

However, if there is no xdomain iframe, then we can just return the source's frameElement

bradfrizzell · 2018-01-16T20:00:30Z

Manually tested as working.

keithwrightbos · 2018-01-16T20:38:25Z

ads/inabox/inabox-messaging-host.js

+ * @return {boolean}
+ * @private
+ */
+function canInspectWindow(win) {


Private function names should end in underscore

keithwrightbos · 2018-01-16T20:41:26Z

ads/inabox/inabox-messaging-host.js

+    let topXDomainWin;
+    for (let j = 0, tempWin = win; j < 10 && tempWin != tempWin.top;
+      j++, tempWin = tempWin.parent) {
+      if (canInspectWindow(tempWin)) {


Can we not just move canInspectWindow into the for loop declaration:

for (let j = 0, tempWin = win; j < 10 && tempWin != tempWin.top && !canInspectWindow(tempWin); topXDomainWin = tempWin, j++, tempWin = tempWin.parent);

done. A loop with no body, I like it.

keithwrightbos · 2018-01-16T20:42:40Z

ads/inabox/inabox-messaging-host.js

+    // that corresponds to topXDomainWin.
+    if (!!topXDomainWin) {
+      const parent = topXDomainWin.parent;
+      const iframes = parent.document.querySelectorAll('iframe');


No need to create parent var, can instead just do const iframes = topXDomainWin.parent.document...

keithwrightbos · 2018-01-16T20:42:57Z

ads/inabox/inabox-messaging-host.js

+ * Returns true if win's properties can be accessed and win is defined.
+ * This functioned is used to determine if a window is cross-domained
+ * from the perspective of the current window.
+ * @param {Window} win


…alid (ampproject#12541) * Allow nested frames if they are friendly * Add tests * Respond to feedback * Respond to feedback * Respond to feedback * Fix dumb dumb * small fix * See...my....test! See my test! Made of real gorilla chest * Continued work * More work in progress * Continued work, added getMeasureableFrame * WIP - fixing tests * Change how getFrameElement works * Fix check-types and tests * Respond to feedback * Make getMeasureableFrame clearer * minor changes * Respond to feedback * Respond to feedback

Allow nested frames if they are friendly

2de4d4f

googlebot added the cla: yes label Dec 20, 2017

bradfrizzell requested a review from keithwrightbos December 20, 2017 16:54

keithwrightbos requested changes Dec 20, 2017

View reviewed changes

Add tests

1f82af1

jonkeller reviewed Dec 21, 2017

View reviewed changes

jonkeller approved these changes Dec 21, 2017

View reviewed changes

Respond to feedback

3d72b16

keithwrightbos requested changes Dec 22, 2017

View reviewed changes

Respond to feedback

7efa9f3

keithwrightbos requested changes Dec 22, 2017

View reviewed changes

bradfrizzell added 3 commits December 22, 2017 14:36

Respond to feedback

2ad2e44

Fix dumb dumb

79bfdd7

small fix

209fee3

keithwrightbos approved these changes Dec 22, 2017

View reviewed changes

See...my....test! See my test! Made of real gorilla chest

40b4a2d

bradfrizzell requested a review from lannka December 22, 2017 20:34

lannka requested changes Jan 3, 2018

View reviewed changes

bradfrizzell added 6 commits January 9, 2018 13:39

Continued work

9cb33c0

More work in progress

2883f7c

Continued work, added getMeasureableFrame

9abf2b0

WIP - fixing tests

f15e432

Change how getFrameElement works

4018e43

Fix check-types and tests

5c3e3d5

keithwrightbos requested changes Jan 11, 2018

View reviewed changes

Respond to feedback

a9b859f

bradfrizzell changed the title ~~Allow nested frames to send AMP inabox post messages if intermediate frames are friendly.~~ Change how AMP inabox validates that the source of a postMessage is valid Jan 11, 2018

lannka requested changes Jan 11, 2018

View reviewed changes

bradfrizzell added 2 commits January 12, 2018 10:30

Make getMeasureableFrame clearer

e24b2e2

minor changes

d3ad1b0

keithwrightbos requested changes Jan 16, 2018

View reviewed changes

bradfrizzell added 2 commits January 16, 2018 15:52

Respond to feedback

674cac0

Respond to feedback

e17c848

keithwrightbos approved these changes Jan 17, 2018

View reviewed changes

lannka approved these changes Jan 17, 2018

View reviewed changes

lannka merged commit 8a94d59 into master Jan 17, 2018

lannka deleted the frizz-nested-inabox branch January 17, 2018 17:37

ampprojectbot added the PR use: In Stable label Jan 31, 2018

lannka mentioned this pull request Mar 14, 2018

inabox visible trigger broken for nested friendly iframe #14010

Closed

Change how AMP inabox validates that the source of a postMessage is valid #12541

Change how AMP inabox validates that the source of a postMessage is valid #12541

Conversation

bradfrizzell commented Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeller commented Dec 21, 2017

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

keithwrightbos Dec 22, 2017 • edited

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

lannka commented Jan 2, 2018

lannka 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

Choose a reason for hiding this comment

bradfrizzell commented Jan 16, 2018

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

bradfrizzell commented Dec 20, 2017 •

edited

keithwrightbos Dec 22, 2017 •

edited