Allow webviews to be set as trusted viewers #5592
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Webviews can't set `ancestorOrigins` properly (maybe?), we can't use it to tell if we are in a trusted viewer context. Instead, fall back to our "old browser" path, which creates a `trustedViewerResolver_`. When the webview's integration script [sets the message deliverer](https://github.com/ampproject/amphtml/blob/f28e116/src/service/viewer-impl.js#L947), it [will resolve](https://github.com/ampproject/amphtml/blob/f28e116/src/service/viewer-impl.js#L961-L962) to the webview's passed origin. Fixes ampproject#5563.
cramforce
reviewed
Oct 13, 2016
| * Whether the AMP document is embedded in a webview. | ||
| * @private @const {boolean} | ||
| */ | ||
| this.isWebviewEmbedded_ = this.params_['webview']; |
There was a problem hiding this comment.
Params are always strings. So, if webview == '0' that wouldn't work. Instead it should be this.params_['webview'] == '1' or such.
dvoytenko
reviewed
Oct 13, 2016
| @@ -293,7 +299,7 @@ export class Viewer { | |||
| // Not embedded in IFrame - can't trust the viewer. | |||
| trustedViewerResolved = false; | |||
| trustedViewerPromise = Promise.resolve(false); | |||
| } else if (this.win.location.ancestorOrigins) { | |||
| } else if (this.win.location.ancestorOrigins && !this.isWebviewEmbedded_) { | |||
There was a problem hiding this comment.
Could we also check that the document is NOT currently iframed?
There was a problem hiding this comment.
That's a great case. I've added this to the #isWebviewEmbeded boolean, and added tests.
We treat them like normal iframes.
|
Tons 'o tests. |
dvoytenko
approved these changes
Oct 13, 2016
jridgewell
force-pushed
the
webview-ancestorOrigins-trusted-viewer
branch
from
7423847
to
479470f
Compare
Lith
pushed a commit
to Lith/amphtml
that referenced
this pull request
Dec 22, 2016
* Allow webviews to be set as trusted viewers Webviews can't set `ancestorOrigins` properly (maybe?), we can't use it to tell if we are in a trusted viewer context. Instead, fall back to our "old browser" path, which creates a `trustedViewerResolver_`. When the webview's integration script [sets the message deliverer](https://github.com/ampproject/amphtml/blob/f28e116/src/service/viewer-impl.js#L947), it [will resolve](https://github.com/ampproject/amphtml/blob/f28e116/src/service/viewer-impl.js#L961-L962) to the webview's passed origin. Fixes ampproject#5563. * Add tests * Do not trust "webviews" that are really bad actor iframes We treat them like normal iframes. * Test for '1' explicitly * Fix test
Lith
pushed a commit
to Lith/amphtml
that referenced
this pull request
Dec 22, 2016
* Allow webviews to be set as trusted viewers Webviews can't set `ancestorOrigins` properly (maybe?), we can't use it to tell if we are in a trusted viewer context. Instead, fall back to our "old browser" path, which creates a `trustedViewerResolver_`. When the webview's integration script [sets the message deliverer](https://github.com/ampproject/amphtml/blob/f28e116/src/service/viewer-impl.js#L947), it [will resolve](https://github.com/ampproject/amphtml/blob/f28e116/src/service/viewer-impl.js#L961-L962) to the webview's passed origin. Fixes ampproject#5563. * Add tests * Do not trust "webviews" that are really bad actor iframes We treat them like normal iframes. * Test for '1' explicitly * Fix test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Webviews can't set
ancestorOriginsproperly (maybe?), we can't use it to tell if we are in a trusted viewer context. Instead, fall back to our "old browser" path, which creates atrustedViewerResolver_. When the webview's integration script sets the message deliverer, it will resolve to the webview's passed origin.Fixes #5563.