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

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

Merged
merged 3 commits into from
Aug 22, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60fb347
Normalize frameborder=no|yes to frameborder=0|1 in iframe sanitizer
schlessera Aug 21, 2019
7e8f290
Add inline comments
schlessera Aug 21, 2019
6418bf0
Move sanitize_boolean_digit() into iframe sanitizer for now
schlessera Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 37 additions & 5 deletions includes/sanitizers/class-amp-iframe-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ public function sanitize() {
if ( $this->args['add_noscript_fallback'] ) {
$node->setAttribute( 'src', $normalized_attributes['src'] );

// AMP is stricter than HTML5 for this attribute, so make sure we use a normalized value.
if ( $node->hasAttribute( 'frameborder' ) ) {
$node->setAttribute( 'frameborder', $normalized_attributes['frameborder'] );
}

// Preserve original node in noscript for no-JS environments.
$this->append_old_node_noscript( $new_node, $node, $this->dom );
}
Expand All @@ -155,7 +160,7 @@ public function sanitize() {
* @type bool $allowfullscreen <iframe> `allowfullscreen` attribute - Convert 'false' to empty string ''
* @type bool $allowtransparency <iframe> `allowtransparency` attribute - Convert 'false' to empty string ''
* }
* @return array Returns HTML attributes; normalizes src, dimensions, frameborder, sandox, allowtransparency and allowfullscreen
* @return array Returns HTML attributes; normalizes src, dimensions, frameborder, sandbox, allowtransparency and allowfullscreen
*/
private function normalize_attributes( $attributes ) {
$out = [];
Expand Down Expand Up @@ -189,10 +194,7 @@ private function normalize_attributes( $attributes ) {
break;

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

case 'allowfullscreen':
Expand Down Expand Up @@ -271,4 +273,34 @@ private function build_placeholder( $parent_attributes ) {
return $placeholder_node;
}

/**
* Sanitizes a boolean character (or string) into a '0' or '1' character.
*
* @param string $value A boolean character to sanitize. If a string containing more than a single
* character is provided, only the first character is taken into account.
*
* @return string Returns either '0' or '1'.
*/
private function sanitize_boolean_digit( $value ) {

// Default to false if the value was forgotten.
if ( empty( $value ) ) {
return '0';
}

// Default to false if the value has an unexpected type.
if ( ! is_string( $value ) && ! is_numeric( $value ) ) {
return '0';
}

// See: https://github.com/ampproject/amp-wp/issues/2335#issuecomment-493209861.
switch ( substr( (string) $value, 0, 1 ) ) {
case '1':
case 'y':
case 'Y':
return '1';
}

return '0';
}
}
34 changes: 33 additions & 1 deletion tests/php/test-amp-iframe-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public function get_data() {
'
<amp-iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0" sandbox="allow-scripts allow-same-origin" layout="intrinsic" class="amp-wp-enforced-sizes">
<noscript>
<iframe src="https://example.com/embed/132886713" width="500" height="281"></iframe>
<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0"></iframe>
</noscript>
</amp-iframe>
',
Expand Down Expand Up @@ -342,6 +342,38 @@ public function get_data() {
'alias_origin' => 'https://alt.example.org',
],
],

'iframe_with_frameborder_no' => [
'<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="no" class="iframe-class" allowtransparency="false" allowfullscreen></iframe>',
'
<amp-iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0" class="iframe-class amp-wp-enforced-sizes" allowfullscreen="" sandbox="allow-scripts allow-same-origin" layout="intrinsic">
<span placeholder="" class="amp-wp-iframe-placeholder"></span>
<noscript>
<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0" class="iframe-class"></iframe>
</noscript>
</amp-iframe>
',
[
'add_noscript_fallback' => true,
'add_placeholder' => true,
],
],

'iframe_with_frameborder_yes' => [
'<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="yes" class="iframe-class" allowtransparency="false" allowfullscreen></iframe>',
'
<amp-iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="1" class="iframe-class amp-wp-enforced-sizes" allowfullscreen="" sandbox="allow-scripts allow-same-origin" layout="intrinsic">
<span placeholder="" class="amp-wp-iframe-placeholder"></span>
<noscript>
<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="1" class="iframe-class"></iframe>
</noscript>
</amp-iframe>
',
[
'add_noscript_fallback' => true,
'add_placeholder' => true,
],
],
];
}

Expand Down