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

Normalize frameborder=no|yes to frameborder=0|1 in iframe sanitizer #2335

Closed
westonruter opened this issue May 16, 2019 · 2 comments · Fixed by #3064
Closed

Normalize frameborder=no|yes to frameborder=0|1 in iframe sanitizer #2335

westonruter opened this issue May 16, 2019 · 2 comments · Fixed by #3064

Comments

@westonruter
Copy link
Member

When adding an iframe such as:

<iframe frameborder="no" height="200" scrolling="no" src="https://player.megaphone.fm/DEM1119155060?" width="100%"></iframe>

It gets converted into an amp-iframe successfully as:

<amp-iframe frameborder="0" height="200" scrolling="no" src="https://player.megaphone.fm/DEM1119155060?" width="640" sandbox="allow-scripts allow-same-origin" layout="intrinsic" class="amp-wp-enforced-sizes">
    <span placeholder="" class="amp-wp-iframe-placeholder"></span>
    <noscript>
        <iframe height="200" scrolling="no" src="https://player.megaphone.fm/DEM1119155060?" width="100%"></iframe>
    </noscript>
</amp-iframe>

Nevertheless, one AMP validation error is raised in the process:

image

This validation error is pointless, however, because the iframe that amp-iframe component generates already provides frameborder=0:

image

It seems that while HTML allows no as a synonym for 0 (and yes for 1), this is not supported by AMP:

  attrs: {
    name: "frameborder"
    value: "0"
    value: "1"
  }

Apparently this is not even allowed in HTML4, so it could just be a common author mistake (which browsers gracefully handle, and it has been deprecated/removed in HTML5):

  frameborder (1|0)          1         -- request frame borders? --

The AMP plugin makes the default frameborder=0 as seen in the generated example above. What is missing is when the $value is no it should be normalized to 0, and when yes normalized to 1:

case 'frameborder':
if ( '0' !== $value && '1' !== $value ) {
$value = '0';
}
$out[ $name ] = $value;
break;

This will prevent a validation error from being raised, and it will allow iframe[frameborder=yes] to actually result in amp-iframe[frameborder=1] as the user intends.

@westonruter westonruter added this to the v1.1.2 milestone May 16, 2019
@swissspidy
Copy link
Collaborator

so it could just be a common author mistake (which browsers gracefully handle

Perhaps that comes from the other frame elements:

A frameset or frame element has a border if the following algorithm returns true:

If the element has a frameborder attribute whose value is not the empty string and whose first character is either a U+0031 DIGIT ONE (1) character, a U+0079 LATIN SMALL LETTER Y character (y), or a U+0059 LATIN CAPITAL LETTER Y character (Y), then return true.

Interesting read :)

@westonruter
Copy link
Member Author

Good find. So it's not just yes and no but y and n that we also should handle 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants