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.

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

Allow invocation of arbitrary 3p methods inside iframes using standard AMP actions. #9074

Closed
aghassemi opened this Issue May 1, 2017 · 24 comments

Comments

@aghassemi
Member

aghassemi commented May 1, 2017

Sample use-case

<amp-iframe src="some-map-embed.html" id="mapEmbed"></amp-iframe>
<button on="tap:mapEmbed.callMethod(method="changeLocation", data="Toronto")>
     Show Toronto's Map
</button>

inside some-map-embed.html:

context.onMethodCall(methodName, data) {
...
}

context is AMP Context

The real use-case that motivated this FR is using iframes to handle Web Payments. Ideally the iframe that uses the Web Payment API to show the native payment sheet is already loaded and when users taps a "buy" button on the AMP page, it can just send a request to the already-loaded iframe to show the payment sheet (as opposed to the alternative slow approach of loading the iframe when "buy" is tapped)

another goal is to support more interactivity between parent page and iframed content. Would be useful for highly interactive journalism (scroll-driven storytelling, data visualizations toggled by page-level filters), but is expected to have a wide range of applications beyond that.

Might also consider taking arbitrary messages passed * out of * amp-iframe, but would need to make sure these aren't executed as code in any way that violates the assumptions of AMP

/cc @cramforce @dvoytenko @rudygalfi @ericlindley-g

@aghassemi aghassemi self-assigned this May 1, 2017

@aghassemi aghassemi added this to the New FRs milestone May 1, 2017

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce May 2, 2017

Member
Member

cramforce commented May 2, 2017

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi May 2, 2017

Member

@lannka mentioned we compile amp context to an external JS file that can included in any 3p page. His opinion (which I really like) is to use this JS object to add APIs for AMP and 3P communication instead of relying on documented postMessage formats.

on a related note, @lannka we should start namespacing amp context object. we don't want everything at top level.

Member

aghassemi commented May 2, 2017

@lannka mentioned we compile amp context to an external JS file that can included in any 3p page. His opinion (which I really like) is to use this JS object to add APIs for AMP and 3P communication instead of relying on documented postMessage formats.

on a related note, @lannka we should start namespacing amp context object. we don't want everything at top level.

@lannka

This comment has been minimized.

Show comment
Hide comment
@lannka

lannka May 2, 2017

Collaborator

This totally makes sense to me. It's very like our existing API such as context.observeIntersection(callback).

Just want to point out that the proposal is only one way communication, i.e. it does not wait for a response. Would that limit the usages? Of course, designing a 2-way communication will be quite complicated, and probably would have involved amp-bind.

Re namespacing:

Right now, the existing APIs are still quite generic. We can start scoping the APIs once we have specific domain such as video iframe.

Here is a full list of existing APIs:

Static data:

    this.canary = context.canary;
    this.canonicalUrl = context.canonicalUrl;
    this.clientId = context.clientId;
    this.container = context.container;
    this.data = context.tagName;
    this.hidden = context.hidden;
    this.initialLayoutRect = context.initialLayoutRect;
    this.initialIntersection = context.initialIntersection;
    this.location = context.location;
    this.mode = context.mode;
    this.pageViewId = context.pageViewId;
    this.referrer = context.referrer;
    this.sentinel = context.sentinel;
    this.sourceUrl = context.sourceUrl;
    this.startTime = context.startTime;
    this.tagName = context.tagName;

Methods:

onPageVisibilityChange(callback)
observeIntersection(callback) 
requestResize(width, height)
onResizeSuccess(callback)
onResizeDenied(callback)
addContextToIframe(iframe)
noContentAvailable()
Collaborator

lannka commented May 2, 2017

This totally makes sense to me. It's very like our existing API such as context.observeIntersection(callback).

Just want to point out that the proposal is only one way communication, i.e. it does not wait for a response. Would that limit the usages? Of course, designing a 2-way communication will be quite complicated, and probably would have involved amp-bind.

Re namespacing:

Right now, the existing APIs are still quite generic. We can start scoping the APIs once we have specific domain such as video iframe.

Here is a full list of existing APIs:

Static data:

    this.canary = context.canary;
    this.canonicalUrl = context.canonicalUrl;
    this.clientId = context.clientId;
    this.container = context.container;
    this.data = context.tagName;
    this.hidden = context.hidden;
    this.initialLayoutRect = context.initialLayoutRect;
    this.initialIntersection = context.initialIntersection;
    this.location = context.location;
    this.mode = context.mode;
    this.pageViewId = context.pageViewId;
    this.referrer = context.referrer;
    this.sentinel = context.sentinel;
    this.sourceUrl = context.sourceUrl;
    this.startTime = context.startTime;
    this.tagName = context.tagName;

Methods:

onPageVisibilityChange(callback)
observeIntersection(callback) 
requestResize(width, height)
onResizeSuccess(callback)
onResizeDenied(callback)
addContextToIframe(iframe)
noContentAvailable()
@choumx

This comment has been minimized.

Show comment
Hide comment
@choumx

choumx Aug 28, 2017

Collaborator

Cool feature! A couple thoughts:

  • It's not clear to me that using AmpContext is a win here. Besides the origin check, what else is gained? Does AmpContext add version skew risk?
  • Let's use postMessage to avoid name collisions with global actions like show.

A syntax idea:

<!-- Without amp-bind. -->
<button on="tap:myIframe.postMessage(method='myMethod', arg1=123)">

<!-- With amp-bind (to reference state variables). -->
<button on="tap:myIframe.postMessage({method: 'myMethod', arg1: myAmpState.foo})">

This only considers AMP -> iframe communication. I'm not sure that the opposite direction (iframe -> AMP) is safe for non-trivial APIs, e.g. clicking a button in an interactive graphic and showing a popup in the AMP page.

Collaborator

choumx commented Aug 28, 2017

Cool feature! A couple thoughts:

  • It's not clear to me that using AmpContext is a win here. Besides the origin check, what else is gained? Does AmpContext add version skew risk?
  • Let's use postMessage to avoid name collisions with global actions like show.

A syntax idea:

<!-- Without amp-bind. -->
<button on="tap:myIframe.postMessage(method='myMethod', arg1=123)">

<!-- With amp-bind (to reference state variables). -->
<button on="tap:myIframe.postMessage({method: 'myMethod', arg1: myAmpState.foo})">

This only considers AMP -> iframe communication. I'm not sure that the opposite direction (iframe -> AMP) is safe for non-trivial APIs, e.g. clicking a button in an interactive graphic and showing a popup in the AMP page.

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Aug 29, 2017

Member

If we do postMessage then targetOrigin should be required.

Member

cramforce commented Aug 29, 2017

If we do postMessage then targetOrigin should be required.

@choumx

This comment has been minimized.

Show comment
Hide comment
@choumx

choumx Sep 11, 2017

Collaborator

Thoughts on iframe -> AMP messaging:

  • Can't detect user gestures within cross-domain iframe, so postMessage event should be low-trust (like scroll events).
  • However, AMP.setState is a high-trust action. Add new low-trust variant that only mutates children of amp-iframe and doesn't allow resizing (similar to amp-list).

More optional constraints:

  • Restrict postMessage event to only fire while amp-iframe has focus?
  • Only allow postMessage from original origin? Disallow new origins via [src] binding to avoid risky use of UGC.
<amp-iframe on="postMessage:AMP.setState({
  foo: 'Message ' + event.method + ' received with args: ' + event.arg1,
})">
  <p [text]="foo">Waiting for message from amp-iframe...</p>
</amp-iframe>

<!-- Not a child of amp-iframe, so will not mutate on postMessage event. -->
<p [text]="foo">But this will change on the next high-trust AMP.setState.</p>

Any other abuse concerns? Will probably need security review.

Collaborator

choumx commented Sep 11, 2017

Thoughts on iframe -> AMP messaging:

  • Can't detect user gestures within cross-domain iframe, so postMessage event should be low-trust (like scroll events).
  • However, AMP.setState is a high-trust action. Add new low-trust variant that only mutates children of amp-iframe and doesn't allow resizing (similar to amp-list).

More optional constraints:

  • Restrict postMessage event to only fire while amp-iframe has focus?
  • Only allow postMessage from original origin? Disallow new origins via [src] binding to avoid risky use of UGC.
<amp-iframe on="postMessage:AMP.setState({
  foo: 'Message ' + event.method + ' received with args: ' + event.arg1,
})">
  <p [text]="foo">Waiting for message from amp-iframe...</p>
</amp-iframe>

<!-- Not a child of amp-iframe, so will not mutate on postMessage event. -->
<p [text]="foo">But this will change on the next high-trust AMP.setState.</p>

Any other abuse concerns? Will probably need security review.

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Sep 11, 2017

Member
Member

cramforce commented Sep 11, 2017

@ericlindley-g

This comment has been minimized.

Show comment
Hide comment
@ericlindley-g

ericlindley-g Sep 12, 2017

Collaborator

@choumx 's proposal sounds good (if I understand correctly, this would offer enough flexibility that developers could do something like a complex seat picker—even if it adds an extra tap outside of the iframe for the rest of the page to reflect seat choices made in the iframe)

The additional origin restriction sounds good, too, as well as explicitly configuring source and target origins, and security review.

Could take this to design review as well to get more folks thinking about any potential vectors for bad UX

Collaborator

ericlindley-g commented Sep 12, 2017

@choumx 's proposal sounds good (if I understand correctly, this would offer enough flexibility that developers could do something like a complex seat picker—even if it adds an extra tap outside of the iframe for the rest of the page to reflect seat choices made in the iframe)

The additional origin restriction sounds good, too, as well as explicitly configuring source and target origins, and security review.

Could take this to design review as well to get more folks thinking about any potential vectors for bad UX

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Sep 12, 2017

Member

@choumx Feature Malte is talking about is this https://bugs.chromium.org/p/chromium/issues/detail?id=621631 , I put another comment in there asking to see if anyone can think of a decent, light hack for detecting user initiated actions.

For low-trust version of AMP.setState, I like the idea but what I had in mind is a bit different:

I was imagining assigning low-high trust values to attributes similar to how you did for actions. So when a low-trust event does setState, mutation happens only for low-trust attributes (e.g. value of input or disabled but not class or text). Restricting to only mutating children of amp-iframe can still have bad UX effects as components don't really create a sandbox around their children.

Member

aghassemi commented Sep 12, 2017

@choumx Feature Malte is talking about is this https://bugs.chromium.org/p/chromium/issues/detail?id=621631 , I put another comment in there asking to see if anyone can think of a decent, light hack for detecting user initiated actions.

For low-trust version of AMP.setState, I like the idea but what I had in mind is a bit different:

I was imagining assigning low-high trust values to attributes similar to how you did for actions. So when a low-trust event does setState, mutation happens only for low-trust attributes (e.g. value of input or disabled but not class or text). Restricting to only mutating children of amp-iframe can still have bad UX effects as components don't really create a sandbox around their children.

@choumx

This comment has been minimized.

Show comment
Hide comment
@choumx

choumx Sep 13, 2017

Collaborator

@aghassemi Cool, thanks for the crbug reference.

[text] would be nice to update a label, e.g. "Selected seat A in row 43", and shouldn't cause relayout if it's a child of the i-amphtml-fill-content element. [class] could cause funky stuff, e.g. changing to position: fixed, but this is already possible with amp-list.

This is nice-to-have anyways. For MVP, we could just have zero-mutation low-trust AMP.setState and require the iframe itself contain any such labels.

Collaborator

choumx commented Sep 13, 2017

@aghassemi Cool, thanks for the crbug reference.

[text] would be nice to update a label, e.g. "Selected seat A in row 43", and shouldn't cause relayout if it's a child of the i-amphtml-fill-content element. [class] could cause funky stuff, e.g. changing to position: fixed, but this is already possible with amp-list.

This is nice-to-have anyways. For MVP, we could just have zero-mutation low-trust AMP.setState and require the iframe itself contain any such labels.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Sep 13, 2017

Member

I agree that for MVP, we should just do no-mutation, just merge state viasetState.

Issue with only mutating children of amp-iframe is that children get overlayed by iframe anyway, if z-index is used to bring them to top of frame, I don't believe it handles many use-cases even for seat selection. If updating text that is overlayed on top of the iframe is the issue, they could actually integrated that with the iframe page itself, chances are they need to update another part of the page (total price somewhere else, or value of a hidden input outside of amp-iframe).

We definitely have some loopholes in our protections (amp list you mentioned, also CSS animations) shhh.. :) but I guess there is the performance aspect of the protection too, without concept of low-trust mutations, postMessage can cause list, iframe src changes triggering network calls if those are children of the iframe.

The right solution for this is really just https://bugs.chromium.org/p/chromium/issues/detail?id=621631 :(

Member

aghassemi commented Sep 13, 2017

I agree that for MVP, we should just do no-mutation, just merge state viasetState.

Issue with only mutating children of amp-iframe is that children get overlayed by iframe anyway, if z-index is used to bring them to top of frame, I don't believe it handles many use-cases even for seat selection. If updating text that is overlayed on top of the iframe is the issue, they could actually integrated that with the iframe page itself, chances are they need to update another part of the page (total price somewhere else, or value of a hidden input outside of amp-iframe).

We definitely have some loopholes in our protections (amp list you mentioned, also CSS animations) shhh.. :) but I guess there is the performance aspect of the protection too, without concept of low-trust mutations, postMessage can cause list, iframe src changes triggering network calls if those are children of the iframe.

The right solution for this is really just https://bugs.chromium.org/p/chromium/issues/detail?id=621631 :(

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Sep 14, 2017

Member
Member

cramforce commented Sep 14, 2017

@dvoytenko

This comment has been minimized.

Show comment
Hide comment
@dvoytenko

dvoytenko Sep 14, 2017

Collaborator

Non-audible tracks sometimes still mute other tracks on the phone, so could be pretty negative UX. I previously used copy-paste, but that (a) not possible to attribute to any one particular iframe; (b) also bad UX due to lost copy buffer from before.

Collaborator

dvoytenko commented Sep 14, 2017

Non-audible tracks sometimes still mute other tracks on the phone, so could be pretty negative UX. I previously used copy-paste, but that (a) not possible to attribute to any one particular iframe; (b) also bad UX due to lost copy buffer from before.

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Sep 21, 2017

Member

I think I have found a decent polyfill for mobile. It is based on <audio> tag but without a need to have a src or append it to the DOM. It does not pause other audio streams either.

essentially:

function isUserInteraction() {
  const detectionElement = window.document.createElement('audio');
  detectionElement.play();
  return !detectionElement.paused;
}

Tested on iOS and Chrome/Android and works fine. (it is based on a similar approach we take to detect autoplay for amp-video)

jsbin: https://output.jsbin.com/mohujitehe

Now if we combined this with @cramforce's suggestion of checking if iframe has focus and considering that both desktop Chrome and Safari are changing audio autoplay policy on desktop anyway, abuse gets really limited to desktop-old-browser-only cases, which is good enough protection IMO.

Member

aghassemi commented Sep 21, 2017

I think I have found a decent polyfill for mobile. It is based on <audio> tag but without a need to have a src or append it to the DOM. It does not pause other audio streams either.

essentially:

function isUserInteraction() {
  const detectionElement = window.document.createElement('audio');
  detectionElement.play();
  return !detectionElement.paused;
}

Tested on iOS and Chrome/Android and works fine. (it is based on a similar approach we take to detect autoplay for amp-video)

jsbin: https://output.jsbin.com/mohujitehe

Now if we combined this with @cramforce's suggestion of checking if iframe has focus and considering that both desktop Chrome and Safari are changing audio autoplay policy on desktop anyway, abuse gets really limited to desktop-old-browser-only cases, which is good enough protection IMO.

@dvoytenko

This comment has been minimized.

Show comment
Hide comment
@dvoytenko

dvoytenko Sep 21, 2017

Collaborator

@aghassemi This is awesome! How does it work if a gesture originated from a x-origin iframe?

Collaborator

dvoytenko commented Sep 21, 2017

@aghassemi This is awesome! How does it work if a gesture originated from a x-origin iframe?

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Sep 21, 2017

Member
Member

cramforce commented Sep 21, 2017

@cramforce

This comment has been minimized.

Show comment
Hide comment
@cramforce

cramforce Sep 21, 2017

Member

One more thing: Both Chrome and Safari are aligning their audio playback policies on desktop with mobile. So, this should work in even more places. Not sure about Firefox

Member

cramforce commented Sep 21, 2017

One more thing: Both Chrome and Safari are aligning their audio playback policies on desktop with mobile. So, this should work in even more places. Not sure about Firefox

@choumx

This comment has been minimized.

Show comment
Hide comment
@choumx

choumx Sep 21, 2017

Collaborator

Great find!

@dvoytenko it appears to work. Here's an example that postMessage's to a null-origin iframe: http://output.jsbin.com/vejasem

Collaborator

choumx commented Sep 21, 2017

Great find!

@dvoytenko it appears to work. Here's an example that postMessage's to a null-origin iframe: http://output.jsbin.com/vejasem

@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi Sep 21, 2017

Member

Great, yeah user intent should carry forward through postMessage and other async calls.

Few notes:
1- It is still very important to keep "iframe has focus" as a check. This will ensure dev environment (Desktop) behaves the same as mobile unless someone is actually trying to abuse the system.
2- We still need a decision whether to still allow non-user-initiated messages to change the state (without mutation) or we wanna completely restrict the whole thing to just user-initiated messages now. I vote for the latter since the former doesn't really help the use-cases we wanna solve. Can be revised later.
3- If we go with "only accepting user-initiated messages", we can expect a special "sentinel" for these messages and as soon as we receive (a few?) non-user-initiated messages of that type, we can blacklist the iframe and start disposing their messages to avoid having the detection code get flooded

Member

aghassemi commented Sep 21, 2017

Great, yeah user intent should carry forward through postMessage and other async calls.

Few notes:
1- It is still very important to keep "iframe has focus" as a check. This will ensure dev environment (Desktop) behaves the same as mobile unless someone is actually trying to abuse the system.
2- We still need a decision whether to still allow non-user-initiated messages to change the state (without mutation) or we wanna completely restrict the whole thing to just user-initiated messages now. I vote for the latter since the former doesn't really help the use-cases we wanna solve. Can be revised later.
3- If we go with "only accepting user-initiated messages", we can expect a special "sentinel" for these messages and as soon as we receive (a few?) non-user-initiated messages of that type, we can blacklist the iframe and start disposing their messages to avoid having the detection code get flooded

@choumx

This comment has been minimized.

Show comment
Hide comment
@choumx

choumx Sep 21, 2017

Collaborator
  1. Agree.
  2. Agree.
  3. Sentinel may not be necessary here. We can just keep track of number of message events from the child iframe and un-listen at N invalid messages.

One interesting find is that this polyfill works with async calls up to 1 second after the user gesture, which makes this more robust than one of the suggested W3C solutions (add a new property to MessageEvent). I might follow up with Chrome/Webkit about how this is implemented.

Collaborator

choumx commented Sep 21, 2017

  1. Agree.
  2. Agree.
  3. Sentinel may not be necessary here. We can just keep track of number of message events from the child iframe and un-listen at N invalid messages.

One interesting find is that this polyfill works with async calls up to 1 second after the user gesture, which makes this more robust than one of the suggested W3C solutions (add a new property to MessageEvent). I might follow up with Chrome/Webkit about how this is implemented.

@ampprojectbot

This comment has been minimized.

Show comment
Hide comment
@ampprojectbot

ampprojectbot Dec 25, 2017

Collaborator

This issue hasn't been updated in awhile. @choumx Do you have any updates?

Collaborator

ampprojectbot commented Dec 25, 2017

This issue hasn't been updated in awhile. @choumx Do you have any updates?

@choumx

This comment has been minimized.

Show comment
Hide comment
@choumx

choumx Dec 26, 2017

Collaborator

This is currently blocked on #12498.

Collaborator

choumx commented Dec 26, 2017

This is currently blocked on #12498.

@choumx

This comment has been minimized.

Show comment
Hide comment
@choumx

choumx May 26, 2018

Collaborator

Unfortunately this feature has been disabled pending some investigation into alternate solutions to detect user gestures in iframes: #15617

Collaborator

choumx commented May 26, 2018

Unfortunately this feature has been disabled pending some investigation into alternate solutions to detect user gestures in iframes: #15617

@Makac

This comment has been minimized.

Show comment
Hide comment
@Makac

Makac Aug 28, 2018

@choumx, I notice that iframe-messaging is still available on the Experiments page. Is this intentional? If so is there any documentation available for this feature?

Makac commented Aug 28, 2018

@choumx, I notice that iframe-messaging is still available on the Experiments page. Is this intentional? If so is there any documentation available for this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment