Story Player should set parent origin if current window origin is null #28225

gmajoulet · 2020-05-05T21:58:35Z

Story Player fails to set up the communication with its Story when in a <iframe srcdoc="<playerhtml>"> context.
The location.href that the player gets in this context is about:srcdoc, and sets the #origin=... parameter to an invalid about:srcdoc value.

If we detect that the window.location.origin is null, we should fallback to something else. Maybe window.parent.location.origin? I don't know if that would work. Setting #origin=* works but might not be the best fix.

Repro case (replacing SOME_STORY_URL with an actual Story URL):

<iframe  srcdoc="<html>  <head>        <script async=&quot;&quot; src=&quot;../../../dist/amp-story-player.js&quot;></script>    <link href=&quot;https://cdn.ampproject.org/amp-story-player-v0.css&quot; rel=&quot;stylesheet&quot; type=&quot;text/css&quot;></head>  <body>    <amp-story-player>      <a href=&quot;SOME_STORY_URL&quot;>        Story title      </a>    </amp-story-player></body></html>"> </iframe>

cc @ampproject/wg-stories @Enriqe

The text was updated successfully, but these errors were encountered:

cramforce · 2020-05-05T22:09:17Z

You can use the parent origin but this needs to be guarded by being able to access it. The iframe could also be sandboxed without allow-same-origin which would also create a null origin, but would allow reading the parent origin.

The question here is whether checking the origin is actually desired? A check like event.source == parent to accept messages only from the parent frame might be enough (or checking that the message is coming from the expected child in the opposite direction).

gmajoulet · 2020-05-05T22:18:39Z

The question here is whether checking the origin is actually desired?

I'm not very familiar with this part of the AMP codebase, we're trying to work with the amp-viewer-integration.
It actually looks like it is designed to handle this case, and expects #origin=null.
cf:

amphtml/extensions/amp-viewer-integration/0.1/messaging/messaging.js

Lines 88 to 96 in fd86263

    
             /** 
        
              * @param {JsonObject} data 
        
              */ 
        
             postMessage(data) { 
        
               // Opaque (null) origin can only receive messages sent to "*" 
        
               const targetOrigin = this.origin_ === 'null' ? '*' : this.origin_; 
        
               this.target_./*OK*/ postMessage(data, targetOrigin); 
        
             }

The issue comes from how the Story player retrieves the origin:

amphtml/src/amp-story-player/amp-story-player-impl.js

Lines 459 to 465 in fd86263

    
           const {location} = this.win_; 
        
           const url = parseUrlWithA(this.cachedA_, location.href); 
        
           const params = dict({ 
        
             'amp_js_v': '0.1', 
        
             'visibilityState': visibilityState, 
        
             'origin': url.origin,

Replacing this with location.origin instead of parsing the href would retrieve either the origin, or null, which would work with amp-story-viewer.
Parsing the href like it is done gives about:srcdoc when the expected result from location.origin is null.

cramforce · 2020-05-05T23:09:26Z

Relevant: location.ancestorOrigins helps with this in modern browsers.

Which line of code is the one that is failing today?

chunyahua · 2020-05-06T00:19:06Z

Thank you Gabriel for helping on our issue.
The line which is failing is at:
this.target_./OK/ postMessage(data, targetOrigin);
The error is "Uncaught (in promise) DOMException: Failed to execute 'postMessage' on 'Window': Invalid target origin 'about:srcdoc' in a call to 'postMessage'."

I also want to mention that in our use case, window.parent.location.origin is also null, so not sure if fall back to parent origin will work here.

gmajoulet · 2020-05-06T01:17:33Z

Always passing location.origin will either pass the origin or null, but the amp-viewer-integration.js script will fallback to '*' if it sees null to work around the issue Chunya found, which will work.

cramforce · 2020-05-06T01:34:18Z

OK, this is really quite edge-casey. I missed that it is the parent here who doesn't have a proper origin.

Looks like this line needs an do add || this.origin_ == 'about:srcdoc' https://github.com/ampproject/amphtml/blob/master/extensions/amp-viewer-integration/0.1/messaging/messaging.js#L93

Enriqe · 2020-05-06T15:02:14Z

Sorry, late to the party here. But like @gmajoulet mentioned, this is a bug on the player side. We were getting the origin through url.origin, while we should've been using location.origin. I sent #28238 to fix it.

Enriqe · 2020-05-07T19:48:59Z

After playing around with this a bit more I found an issue with the messaging / viewer-integration code:

The viewer-integration.js takes the origin from the #origin=... param that the viewer set and creates a WindowPortEmulator with it.

amphtml/extensions/amp-viewer-integration/0.1/amp-viewer-integration.js

Line 87 in 471bbb9

const origin = viewer.getParam('origin') || '';

amphtml/extensions/amp-viewer-integration/0.1/amp-viewer-integration.js

Lines 122 to 124 in 471bbb9

    
           const port = new WindowPortEmulator( 
        
             this.win, 
        
             origin,

In the original sample provided in Story Player should set parent origin if current window origin is null #28225 (comment), the viewer is inside an iframe with <iframe srcdoc=..., which will return null when calling location.origin (this weirdly doesn't happen embedding the player by doing <iframe src=...).
When the WindowPortEmulator receives a message from the viewer window, it will compare its own this.origin_ (that was declared in (1), which is null) with the event.origin coming from the window event.

amphtml/extensions/amp-viewer-integration/0.1/messaging/messaging.js

Lines 81 to 85 in 471bbb9

    
           this.win_.addEventListener('message', (event) => { 
        
             if (event.origin == this.origin_ && event.source == this.target_) { 
        
               handler(event); 
        
             } 
        
           });

The event.origin actually has an origin, but the stored this.origin_ property inside the WindowPortEmulator does not. This interrupts the handshake and thus the communication with the viewer <> ampdoc.

A solution here would be to pass a messagingToken to the WindowPortEmulator to verify the sender instead of comparing the window and event origins.

molnarg · 2020-05-15T10:17:45Z

Null origin inside srcdoc was pretty surprizing to me, but after some digging, it makes sense and I think we can just use window.origin instead of window.location.origin. From the spec at https://html.spec.whatwg.org/multipage/webappapis.html#dom-origin-dev :

Developers are strongly encouraged to use self.origin over location.origin. The former returns the origin of the environment, the latter of the URL of the environment.

Browser support should be checked though. MDN says not all browsers are supported, but I've tested on Safari which is listed as not supported and it works for me: https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/origin

Enriqe · 2020-05-15T15:20:11Z

Thank you for taking a look at this @molnarg !

self.origin sounds promising, and checking webkit changelog's it looks like it was actually implemented in Safari preview 27 and Firefox 54. Docs just seem outdated regarding this.

Source

gmajoulet added Type: Bug P1: High Priority WG: stories labels May 5, 2020

gmajoulet assigned Enriqe May 5, 2020

Enriqe mentioned this issue May 6, 2020

[amp-story-player] Use window.origin rather than url origin #28238

Merged

Enriqe mentioned this issue May 8, 2020

Support verifying a sender using messagingToken #28295

Closed

Enriqe closed this as completed in #28238 May 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Story Player should set parent origin if current window origin is null #28225

Story Player should set parent origin if current window origin is null #28225

gmajoulet commented May 5, 2020

cramforce commented May 5, 2020

gmajoulet commented May 5, 2020

cramforce commented May 5, 2020

chunyahua commented May 6, 2020

gmajoulet commented May 6, 2020

cramforce commented May 6, 2020

Enriqe commented May 6, 2020 •

edited

Enriqe commented May 7, 2020 •

edited

molnarg commented May 15, 2020

Enriqe commented May 15, 2020 •

edited

Story Player should set parent origin if current window origin is null #28225

Story Player should set parent origin if current window origin is null #28225

Comments

gmajoulet commented May 5, 2020

cramforce commented May 5, 2020

gmajoulet commented May 5, 2020

cramforce commented May 5, 2020

chunyahua commented May 6, 2020

gmajoulet commented May 6, 2020

cramforce commented May 6, 2020

Enriqe commented May 6, 2020 • edited

Enriqe commented May 7, 2020 • edited

molnarg commented May 15, 2020

Enriqe commented May 15, 2020 • edited

Enriqe commented May 6, 2020 •

edited

Enriqe commented May 7, 2020 •

edited

Enriqe commented May 15, 2020 •

edited