Skip to content
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.

Sign up for GitHub

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

Jump to bottom

amp-iframe: allow attribute #11953

Merged
merged 3 commits into from Dec 5, 2017
Merged

amp-iframe: allow attribute #11953

merged 3 commits into from Dec 5, 2017

Conversation

aghassemi
Copy link
Contributor

@aghassemi aghassemi commented Nov 7, 2017 •
edited

Closes #11937, closes #11541, closes #12282

The syntax for allow recently changed and is fairly complicated w3c/webappsec-permissions-policy#78 I suggest propagating as is without validating the value.

@aghassemi
Copy link
Contributor Author

/cc @cramforce since you expressed some thoughts on #11541

@dvoytenko
Copy link
Contributor

Uhh. Kind of scary. Is there a top-level constraint on these? I.e. can the top-level embedder constraint which of these are truly allowed?

@aghassemi
Copy link
Contributor Author

I hope so! At least that's how allowpayments worked. Asked for clarification here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/mG6vL09JMOQ

If that turns out to be the case (which is likely), which ones do we want viewer to enable? allow="geolocation" is the only one set by viewer at the moment. Payments and fullscreen are also specified via the old format (allowpayments allowfullscreen)

Canonical AMP sites like https://www.accordersaguitare.com/ want microphone as well.

It is sort of strange to let origin specify whatever it wants but then restrict them in the viewer.

@aghassemi
Copy link
Contributor Author

@dvoytenko confirmed, child frames can't give away policies their parent hasn't given them. So viewer can strict available policies of all descendant frames.

@ampprojectbot ampprojectbot added this to the Pending Triage milestone Nov 13, 2017
@dvoytenko
Copy link
Contributor

Still, AMP could be given more allowance than would be expected for embeds within AMP. Did we try to run it by security?

@cramforce
Copy link
Member

cramforce commented Nov 17, 2017 via email

@aghassemi
Copy link
Contributor Author

Chatted offline a bit with Dima yesterday.

Suggestion:
1- AMP does not restrict values of allow in the format (i.e. this PR removing validation restrictions)
2- Viewers can add restrictions. (Feature Policy is flexible enough that a viewer can grant more to the top-level page but restrict the page from granting it to other x-origin embeds by specifying domains for each policy)
3- Have our viewer allow payment geolocation and microphone for now. (it is missing microphone)

Note that these are not actual permissions, just whether the API is available or not. User still needs to grant permission when API is used.

@honeybadgerdontcare honeybadgerdontcare mentioned this pull request Dec 4, 2017
Closed
@lannka
Copy link
Contributor

lannka commented Dec 4, 2017

what's the status of this PR? it will unblock #12282

@aghassemi
Copy link
Contributor Author

ready to merge, needs approval :)
1- let's merge this
2- I will ask our viewer to allow payment geolocation, microphone and vr.

@aghassemi aghassemi merged commit 356a1de into ampproject:master Dec 5, 2017
@aghassemi
Copy link
Contributor Author

b/70231806

ghost pushed a commit that referenced this pull request Dec 6, 2017
@aghassemi
amp-iframe: allow allow attribute (#11953)
7760791
jpettitt pushed a commit to jpettitt/amphtml that referenced this pull request Dec 11, 2017
@aghassemi @jpettitt
amp-iframe: allow allow attribute (ampproject#11953)
6427276
Gregable pushed a commit that referenced this pull request Dec 20, 2017
Revision bump for #11953
1c211ad
Gregable added a commit that referenced this pull request Dec 20, 2017
@Gregable
Validator Rollup (#12544)
a31fac1 
* Revision bump for #11953

* Refactor validateTagAgainstSpec

* Further refactor validateTagAgainstSpec by extracting methods for additional validation steps which can add errors.

* Refactor. Extract a function from validateTagAgainstSpec named updateStateFromTagSpec.

* Refactor, merge UpdateStateFromTagSpec and UpdateGlobalSpecs.

* Revision bump for #12095

* Refactor. Move updateContextFromTagSpec into Context class.

* Refactor. Move code from Context::UpdateFromMatchingTagSpec into smaller methods and in some cases even closer to the relevant context objects (ExtensionsContext and TagStack)

* Revision bump for #12285

* Revision bump for #12452

* Refactor. Pull the 'update' part of validate-then-update further up the call stack.

* Revision bump for #12457

* Revision bump for #12462

* Refactor. Clean up recording of matching reference points. Push the logic for it into the tagspec methods.

* Revision bump for #12461

* Refactor. Use ValidationResult::PASS to indicate a tagspec is passing, rather than unknown, which is harder to reason about.

* Revision bump for #12196

* Make amp4ads boilerplate mandatory.

* Refactor. Split up the MatchChildTag method into a const validation method and a state mutation method. Push those into the relevant parts of the code.

* Revision bump for #12471

* Fix comments.

* Build in native support for vendor-prefixes in CSS rules.

* Revision bump for #12515

* Add new layout FLUID to validator.

* Refactor. Check for reference point collisions without involving the tag stack.

* Add missing test file.
@Gregable
Copy link
Member

Gregable commented Jan 3, 2018

The validator changes in here are now live.

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@aghassemi @gzgogo
amp-iframe: allow allow attribute (ampproject#11953)
943a377
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@Gregable @gzgogo
Validator Rollup (ampproject#12544)
6bb9523 
* Revision bump for ampproject#11953

* Refactor validateTagAgainstSpec

* Further refactor validateTagAgainstSpec by extracting methods for additional validation steps which can add errors.

* Refactor. Extract a function from validateTagAgainstSpec named updateStateFromTagSpec.

* Refactor, merge UpdateStateFromTagSpec and UpdateGlobalSpecs.

* Revision bump for ampproject#12095

* Refactor. Move updateContextFromTagSpec into Context class.

* Refactor. Move code from Context::UpdateFromMatchingTagSpec into smaller methods and in some cases even closer to the relevant context objects (ExtensionsContext and TagStack)

* Revision bump for ampproject#12285

* Revision bump for ampproject#12452

* Refactor. Pull the 'update' part of validate-then-update further up the call stack.

* Revision bump for ampproject#12457

* Revision bump for ampproject#12462

* Refactor. Clean up recording of matching reference points. Push the logic for it into the tagspec methods.

* Revision bump for ampproject#12461

* Refactor. Use ValidationResult::PASS to indicate a tagspec is passing, rather than unknown, which is harder to reason about.

* Revision bump for ampproject#12196

* Make amp4ads boilerplate mandatory.

* Refactor. Split up the MatchChildTag method into a const validation method and a state mutation method. Push those into the relevant parts of the code.

* Revision bump for ampproject#12471

* Fix comments.

* Build in native support for vendor-prefixes in CSS rules.

* Revision bump for ampproject#12515

* Add new layout FLUID to validator.

* Refactor. Check for reference point collisions without involving the tag stack.

* Add missing test file.
@nightpool
Copy link

nightpool commented Sep 27, 2018 •
edited

aside from payment, geolocation, microphone and vr, encrypted-media is now also required to use EME apis. Is there a way to get this considered for inclusion in the ampproject viewer?

@cramforce
Copy link
Member

@nightpool Yes! Would you mind filing a separate issue and we can track inclusion across major viewers.

@nightpool
Copy link

@cramforce will do

@nightpool
Copy link

done: #18430

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

@cramforce cramforce cramforce approved these changes

@honeybadgerdontcare honeybadgerdontcare Awaiting requested review from honeybadgerdontcare

@dvoytenko dvoytenko Awaiting requested review from dvoytenko

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental
Projects
None yet
Milestone
Pending Triage
Development

Successfully merging this pull request may close these issues.

amp-iframe allow="vr" amp-iframe allow="microphone" FR: amp-iframe allow="geolocation"
8 participants
@aghassemi @dvoytenko @cramforce @lannka @Gregable @nightpool @googlebot @ampprojectbot