Add message whitelist functionality to inabox messaging host #18698
Conversation
ads/inabox/inabox-messaging-host.js
Outdated
| @@ -129,6 +129,15 @@ export class InaboxMessagingHost { | |||
| return false; | |||
| } | |||
|
|
|||
| const allowedTypes = | |||
| this.iframeMap_[request['sentinel']].iframe.dataset['ampAllowed']; | |||
There was a problem hiding this comment.
Let's refactor this so you can rely on the output of getFrameElement by doing the following:
- change getFrameElement_ to return ?AdFrameDef
- you can then do const allowedTypes = adFrame.iframe.dataSet['ampAllowed'];
- modify iframe usage below to be adFrame.measurableFrame
ads/inabox/inabox-messaging-host.js
Outdated
| this.iframeMap_[request['sentinel']].iframe.dataset['ampAllowed']; | ||
| // having no whitelist is legacy behavior so assume all types are allowed | ||
| if (allowedTypes && | ||
| !allowedTypes.split(/ *, */).includes(request['type'])) { |
There was a problem hiding this comment.
I assume commenting out of split here was unintentional and/or can be removed?
There was a problem hiding this comment.
If you're talking about the "/ *, */" part, that's actually a regex that matches commas with any number of spaces before or after it as a separator.
There was a problem hiding this comment.
Ah, I suggest using \s for whitespace character as its both easier to read here and allows for other whitespace characters. Or you could save yourself a split operation and then includes (two O(N) operation) and do:
if (allowedTypes && new RegExp((^|,)\\s*${allowedTypes['type']}\\s*($|,)).test(allowedTypes)) {
Although this could allow someone to subvert the regexp but since the type is checked later essentially against a whitelist, it should be fine. I'd be curious to know how often inabox host script receives messages.
Last option would be to cache the result of calling split on AdFrame but that means if the publisher changes the whitelist, you wouldn't see it.
There was a problem hiding this comment.
Thanks for the tip. I changed it as you suggested.
| const allowedTypes = adFrame.iframe.dataset['ampAllowed']; | ||
| const hasAllowedTypeRegex = | ||
| new RegExp(`(^|,)\\s*${request['type']}\\s*($|,)`); | ||
| // having no whitelist is legacy behavior so assume all types are allowed |
There was a problem hiding this comment.
I actually spoke with Jeff about this and I believe we agreed that in absence of a whitelist we should assume read-only operations are allowed but not write (e.g. resize).
There was a problem hiding this comment.
We need to not break creatives that are using this properly, and while I don't currently think there are any there are two ways to find out:
-
The current plan: no whitelist means allow everything, ad tags start putting a limited whitelist on all iframes, if that experiment looks good then we know we haven't broken anything
-
Alternate plan: add beacons for when we would reject something but don't reject it yet. If we don't get these beacons then we know we haven't broken anything.
In both cases, after we're sure that this didn't break anything we switch to "absence of a whitelist we should assume read-only operations are allowed but not write"
| dev().info(TAG, 'Ignored message from untrusted iframe:', message); | ||
| return false; | ||
| } | ||
|
|
||
| const allowedTypes = adFrame.iframe.dataset['ampAllowed']; |
There was a problem hiding this comment.
Sorry I realized this was here in previous iterations but I'm slightly concerned that we're now mixing APIs where frames are added to the "whitelist" programmatically via global variable but allowed list is set via element attribute. We should consider a single way to pass information so it does not become fragmented.
There was a problem hiding this comment.
I don't see how this is a problem (or another way to do this) considering that the whitelist themselves are an attribute of each of the frames, which is within the registered frames array. Another level down the hierarchy, so to speak.
There was a problem hiding this comment.
Nevermind, I saw your point. Will follow up later on.
ads/inabox/inabox-messaging-host.js
Outdated
| const hasAllowedTypeRegex = | ||
| new RegExp(`(^|,)\\s*${request['type']}\\s*($|,)`); | ||
| new RegExp(`(^|,)\\s*${type}\\s*($|,)`); |
There was a problem hiding this comment.
nit: can go on previous line
|
Hey all, have we reached a consensus on how to best store the whitelist beyond storing it as an attribute within the iframe itself? |
I think that is good, and I thought @keithwrightbos was on board with it too? |
ads/inabox/inabox-messaging-host.js
Outdated
| dev().info(TAG, 'Ignored message from untrusted iframe:', message); | ||
| return false; | ||
| } | ||
|
|
||
| const allowedTypes = adFrame.iframe.dataset['ampAllowed']; | ||
| // escape the type before including it in the regex | ||
| const type = request['type'].replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); |
There was a problem hiding this comment.
This feels like over engineering... apologies if my outlining options backfired. Let's go back to the split contains you had before but using \s for whitespace as what you have here isn't any faster anyway.
zombifier
force-pushed
the
message_whitelist
branch
from
cfbbd32
to
7788a17
Compare
…ect#18698) * Add whitelist functionality to inabox messaging host * Refactor getFrameElement_ to return the entire AdFrameDef * Fix lint errors * Change how whitelist is matched using regex * Fix missing semicolon * Escaping the type before putting it in the regex. * Minor fix - line is short enough * Switch back to using split.
Right now, any AMP ads in a box can ask the host script to resize itself. Add and verify a per-iframe whitelist to restrict the functionality each creative within the frame has.
Frames without whitelist are still allowed to do anything since corresponding behavior from GPT/AdSense is not yet added.