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.

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

Omit mozallowfullscreen & webkitallowfullscreen when sanitizing iframe #3698

Merged
merged 2 commits into from
Nov 7, 2019

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Nov 7, 2019

Summary

Omit the mozallowfullscreen & webkitallowfullscreen attributes when sanitizing an iframe.

Fixes #3425.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 7, 2019
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Would you add one unit test to AMP_Iframe_Sanitizer using the test code:

<iframe title="Why Backbone" id="talk_frame_48643" src="//speakerdeck.com/player/4648d440a3230130452522b217532879" width="640" height="480" style="border:0; padding:0; margin:0; background:transparent;" frameborder="0" allowtransparency="true" allowfullscreen="allowfullscreen" mozallowfullscreen="true" webkitallowfullscreen="true"></iframe>

To ensure it gets converted without those attributes as expected?

@westonruter westonruter added this to the v1.4.1 milestone Nov 7, 2019
@pierlon
Copy link
Contributor Author

pierlon commented Nov 7, 2019

Done!

@westonruter westonruter merged commit 8412733 into develop Nov 7, 2019
@westonruter westonruter deleted the fix/3425-omit-certain-iframe-attributes branch November 7, 2019 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validation errors reported for invalid iframe attributes mozallowfullscreen and webkitallowfullscreen
3 participants