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 security=restricted and handle marginheight/marginwidth attributes in iframe sanitizer #3950

Closed
westonruter opened this issue Dec 17, 2019 · 1 comment · Fixed by #3954
Labels
QA passed Has passed QA and is done
Milestone

Comments

@westonruter
Copy link
Member

Bug Description

When adding an <iframe> with security=restricted attribute, a validation error is raised. This security attribute is obsolete (only ever supported in IE, AFAIK) in favor of the sandbox which amp-iframe supports and the iframe sanitizer provides by default. So I believe the security=restricted attribute should be omitted entirely, as was done with loading=lazy in #3939.

Similarly, iframes have legacy attributes marginwidth and marginheight which were made obsolete in HTML5. When these attributes have a value of 0 they should also be omitted from copying to the amp-iframe.

These three attributes—security, marginwidth, and marginheight—are all added by WordPress's post embeds. By preventing these from being copied, post embeds will at least not cause validation errors when used in WordPress but they still won't be fully supported. For that, see #3426.

Expected Behaviour

No validation errors should be raised.

Steps to reproduce

  1. Create a post and insert a WordPress embed block with the URL https://blog.amp.dev/2019/10/18/the-amp-contributor-summit-hits-new-york/
  2. Save the post
  3. See validation error warning.

Screenshots

image


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@csossi
Copy link

csossi commented Dec 29, 2019

No validation errors raised:
image

@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA passed Has passed QA and is done
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants