From 27c38e9d36dc04ce930c6abc253679a2a36626a0 Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Mon, 26 Feb 2018 11:17:24 -0500 Subject: [PATCH 1/9] Refines handling of 0 and unset height/width attrs --- .../sanitizers/class-amp-base-sanitizer.php | 13 ++++++++- .../sanitizers/class-amp-img-sanitizer.php | 28 +++++++++++++------ tests/test-amp-img-sanitizer.php | 20 +++++++++++++ 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index de5c08c8881..74e62143348 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -174,7 +174,11 @@ protected function get_body_node() { * @return float|int|string Returns a numeric dimension value, or an empty string. */ public function sanitize_dimension( $value, $dimension ) { - if ( empty( $value ) ) { + + /** + * Allows 0 as valid dimension. + */ + if ( null === $value ) { return ''; } @@ -182,6 +186,13 @@ public function sanitize_dimension( $value, $dimension ) { return absint( $value ); } + /** + * Allows floats but parses them to integers. + */ + if ( false !== filter_var( $value, FILTER_VALIDATE_FLOAT ) ) { + return absint( $value ); + } + if ( AMP_String_Utils::endswith( $value, 'px' ) ) { return absint( $value ); } diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 971e8d6cdec..fe585abce65 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -79,7 +79,7 @@ public function sanitize() { } // Determine which images need their dimensions determined/extracted. - if ( '' === $node->getAttribute( 'width' ) || '' === $node->getAttribute( 'height' ) ) { + if ( ! is_numeric( $node->getAttribute( 'width' ) ) || ! is_numeric( $node->getAttribute( 'height' ) ) ) { $need_dimensions[ $node->getAttribute( 'src' ) ][] = $node; } else { $this->adjust_and_replace_node( $node ); @@ -154,18 +154,30 @@ private function determine_dimensions( $need_dimensions ) { if ( ! $node instanceof DOMElement ) { continue; } - - // Provide default dimensions for images whose dimensions we couldn't fetch. - if ( false !== $dimensions ) { - $node->setAttribute( 'width', $dimensions['width'] ); - $node->setAttribute( 'height', $dimensions['height'] ); - } else { - $width = isset( $this->args['content_max_width'] ) ? $this->args['content_max_width'] : self::FALLBACK_WIDTH; + if ( + ! is_numeric( $node->getAttribute( 'width' ) ) && + ! is_numeric( $node->getAttribute( 'height' ) ) + ) { $height = self::FALLBACK_HEIGHT; + $width = self::FALLBACK_WIDTH; $node->setAttribute( 'width', $width ); $node->setAttribute( 'height', $height ); $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size' : 'amp-wp-unknown-size'; $node->setAttribute( 'class', $class ); + } else if ( + ! is_numeric( $node->getAttribute( 'height' ) + ) ) { + $height = self::FALLBACK_HEIGHT; + $node->setAttribute( 'height', $height ); + $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-height' : 'amp-wp-unknown-size amp-wp-unknown-height'; + $node->setAttribute( 'class', $class ); + } else if ( + ! is_numeric( $node->getAttribute( 'width' ) + ) ) { + $width = self::FALLBACK_WIDTH; + $node->setAttribute( 'width', $width ); + $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-width' : 'amp-wp-unknown-size amp-wp-unknown-width'; + $node->setAttribute( 'class', $class ); } } } diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 2c437b7deff..47ff6a4cd80 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -27,6 +27,26 @@ public function get_data() { '

', ), + 'image_with_empty_width' => array( + '

', + '

', + ), + + 'image_with_empty_height' => array( + '

', + '

', + ), + + 'image_with_zero_width' => array( + '

', + '

', + ), + + 'image_with_decimal_width' => array( + '

', + '

', + ), + 'image_with_self_closing_tag' => array( 'Placeholder!', '', From 722d17366aa5897479a7fcd8c67fc0332fabfceb Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Mon, 26 Feb 2018 11:43:02 -0500 Subject: [PATCH 2/9] Applies PHPCS cleanup --- includes/sanitizers/class-amp-img-sanitizer.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index fe585abce65..caa9728546f 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -159,23 +159,23 @@ private function determine_dimensions( $need_dimensions ) { ! is_numeric( $node->getAttribute( 'height' ) ) ) { $height = self::FALLBACK_HEIGHT; - $width = self::FALLBACK_WIDTH; + $width = self::FALLBACK_WIDTH; $node->setAttribute( 'width', $width ); $node->setAttribute( 'height', $height ); $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size' : 'amp-wp-unknown-size'; $node->setAttribute( 'class', $class ); - } else if ( + } elseif ( ! is_numeric( $node->getAttribute( 'height' ) ) ) { $height = self::FALLBACK_HEIGHT; $node->setAttribute( 'height', $height ); $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-height' : 'amp-wp-unknown-size amp-wp-unknown-height'; $node->setAttribute( 'class', $class ); - } else if ( - ! is_numeric( $node->getAttribute( 'width' ) - ) ) { + } elseif ( + ! is_numeric( $node->getAttribute( 'width' ) ) + ) { $width = self::FALLBACK_WIDTH; - $node->setAttribute( 'width', $width ); + $node->setAttribute( 'width', $width ); $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-width' : 'amp-wp-unknown-size amp-wp-unknown-width'; $node->setAttribute( 'class', $class ); } From ba2aa93c83650d32abb2e64f87a6a4c9a6e32823 Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Mon, 26 Feb 2018 11:50:23 -0500 Subject: [PATCH 3/9] Fixes spacing on multi-line function call --- includes/sanitizers/class-amp-img-sanitizer.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index caa9728546f..3de72196a5f 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -165,8 +165,7 @@ private function determine_dimensions( $need_dimensions ) { $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size' : 'amp-wp-unknown-size'; $node->setAttribute( 'class', $class ); } elseif ( - ! is_numeric( $node->getAttribute( 'height' ) - ) ) { + ! is_numeric( $node->getAttribute( 'height' ) ) ) { $height = self::FALLBACK_HEIGHT; $node->setAttribute( 'height', $height ); $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-height' : 'amp-wp-unknown-size amp-wp-unknown-height'; From 94424d6f05540924f2162fdd99691e330f7a2798 Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Mon, 26 Feb 2018 12:04:30 -0500 Subject: [PATCH 4/9] Adjusts spacing in img sanitizer test --- tests/test-amp-img-sanitizer.php | 34 ++++++++++++++++---------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 47ff6a4cd80..9b04f12c344 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -12,62 +12,62 @@ public function setUp() { public function get_data() { return array( - 'no_images' => array( + 'no_images' => array( '

Lorem Ipsum Demet Delorit.

', '

Lorem Ipsum Demet Delorit.

', ), - 'image_without_src' => array( + 'image_without_src' => array( '

', '

', ), - 'image_with_empty_src' => array( + 'image_with_empty_src' => array( '

', '

', ), - 'image_with_empty_width' => array( + 'image_with_empty_width' => array( '

', '

', ), - 'image_with_empty_height' => array( + 'image_with_empty_height' => array( '

', '

', ), - 'image_with_zero_width' => array( + 'image_with_zero_width' => array( '

', '

', ), - 'image_with_decimal_width' => array( + 'image_with_decimal_width' => array( '

', '

', ), - 'image_with_self_closing_tag' => array( + 'image_with_self_closing_tag' => array( 'Placeholder!', '', ), - 'image_with_no_end_tag' => array( + 'image_with_no_end_tag' => array( 'Placeholder!', '', ), - 'image_with_end_tag' => array( + 'image_with_end_tag' => array( 'Placeholder!', '', ), - 'image_with_on_attribute' => array( + 'image_with_on_attribute' => array( '', '', ), - 'image_with_blacklisted_attribute' => array( + 'image_with_blacklisted_attribute' => array( '', '', ), @@ -77,22 +77,22 @@ public function get_data() { '', ), - 'image_with_sizes_attribute_is_overridden' => array( + 'image_with_sizes_attribute_is_overridden' => array( '', '', ), - 'gif_image_conversion' => array( + 'gif_image_conversion' => array( 'Placeholder!', '', ), - 'gif_image_url_with_querystring' => array( + 'gif_image_url_with_querystring' => array( 'Placeholder!', '', ), - 'multiple_same_image' => array( + 'multiple_same_image' => array( ' @@ -101,7 +101,7 @@ public function get_data() { '', ), - 'multiple_different_images' => array( + 'multiple_different_images' => array( ' From 6bdc18c233a07b63a6f6377011299143ab79c7d0 Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Mon, 26 Feb 2018 12:09:31 -0500 Subject: [PATCH 5/9] Applies better spacing to `=>` --- tests/test-amp-img-sanitizer.php | 34 ++++++++++++++++---------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 9b04f12c344..44518a4f124 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -12,62 +12,62 @@ public function setUp() { public function get_data() { return array( - 'no_images' => array( + 'no_images' => array( '

Lorem Ipsum Demet Delorit.

', '

Lorem Ipsum Demet Delorit.

', ), - 'image_without_src' => array( + 'image_without_src' => array( '

', '

', ), - 'image_with_empty_src' => array( + 'image_with_empty_src' => array( '

', '

', ), - 'image_with_empty_width' => array( + 'image_with_empty_width' => array( '

', '

', ), - 'image_with_empty_height' => array( + 'image_with_empty_height' => array( '

', '

', ), - 'image_with_zero_width' => array( + 'image_with_zero_width' => array( '

', '

', ), - 'image_with_decimal_width' => array( + 'image_with_decimal_width' => array( '

', '

', ), - 'image_with_self_closing_tag' => array( + 'image_with_self_closing_tag' => array( 'Placeholder!', '', ), - 'image_with_no_end_tag' => array( + 'image_with_no_end_tag' => array( 'Placeholder!', '', ), - 'image_with_end_tag' => array( + 'image_with_end_tag' => array( 'Placeholder!', '', ), - 'image_with_on_attribute' => array( + 'image_with_on_attribute' => array( '', '', ), - 'image_with_blacklisted_attribute' => array( + 'image_with_blacklisted_attribute' => array( '', '', ), @@ -77,22 +77,22 @@ public function get_data() { '', ), - 'image_with_sizes_attribute_is_overridden' => array( + 'image_with_sizes_attribute_is_overridden' => array( '', '', ), - 'gif_image_conversion' => array( + 'gif_image_conversion' => array( 'Placeholder!', '', ), - 'gif_image_url_with_querystring' => array( + 'gif_image_url_with_querystring' => array( 'Placeholder!', '', ), - 'multiple_same_image' => array( + 'multiple_same_image' => array( ' @@ -101,7 +101,7 @@ public function get_data() { '', ), - 'multiple_different_images' => array( + 'multiple_different_images' => array( ' From b1da90576ea5ad25f98bd83e912aa4e603e0dd47 Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Mon, 26 Feb 2018 18:49:32 -0500 Subject: [PATCH 6/9] Actually include float value in decimal test --- tests/test-amp-img-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 44518a4f124..63796765599 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -43,7 +43,7 @@ public function get_data() { ), 'image_with_decimal_width' => array( - '

', + '

', '

', ), From 88d9d7a98fe11b1e3eec3c9f83a869664c9d2f80 Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Tue, 27 Feb 2018 07:29:00 -0500 Subject: [PATCH 7/9] Incorporates feedback from code review --- .../sanitizers/class-amp-base-sanitizer.php | 23 ++++++++----------- .../sanitizers/class-amp-img-sanitizer.php | 3 ++- tests/test-amp-img-sanitizer.php | 7 +++++- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index 74e62143348..71cf355bd60 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -175,22 +175,14 @@ protected function get_body_node() { */ public function sanitize_dimension( $value, $dimension ) { - /** - * Allows 0 as valid dimension. - */ + // Allows 0 to be used as valid dimension. if ( null === $value ) { return ''; } - if ( false !== filter_var( $value, FILTER_VALIDATE_INT ) ) { - return absint( $value ); - } - - /** - * Allows floats but parses them to integers. - */ - if ( false !== filter_var( $value, FILTER_VALIDATE_FLOAT ) ) { - return absint( $value ); + // Accepts both integers and floats & prevents negative values. + if ( is_numeric( $value ) ) { + return max( 0, floatval( $value ) ); } if ( AMP_String_Utils::endswith( $value, 'px' ) ) { @@ -263,7 +255,12 @@ public function enforce_sizes_attribute( $attributes ) { $max_width = $this->args['content_max_width']; } - $attributes['sizes'] = sprintf( '(min-width: %1$dpx) %1$dpx, 100vw', absint( $max_width ) ); + // Allows floats and integers but prevents negative values. + // Uses string format to prevent additional modification. + $attributes['sizes'] = sprintf( + '(min-width: %1$spx) %1$spx, 100vw', + max( 0, floatval( $max_width ) ) + ); $this->add_or_append_attribute( $attributes, 'class', 'amp-wp-enforced-sizes' ); diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 3de72196a5f..86fe45e2770 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -165,7 +165,8 @@ private function determine_dimensions( $need_dimensions ) { $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size' : 'amp-wp-unknown-size'; $node->setAttribute( 'class', $class ); } elseif ( - ! is_numeric( $node->getAttribute( 'height' ) ) ) { + ! is_numeric( $node->getAttribute( 'height' ) ) + ) { $height = self::FALLBACK_HEIGHT; $node->setAttribute( 'height', $height ); $class = $node->hasAttribute( 'class' ) ? $node->getAttribute( 'class' ) . ' amp-wp-unknown-size amp-wp-unknown-height' : 'amp-wp-unknown-size amp-wp-unknown-height'; diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 63796765599..8b292f4431f 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -27,6 +27,11 @@ public function get_data() { '

', ), + 'image_with_empty_width_and_height' => array( + '

', + '

', + ), + 'image_with_empty_width' => array( '

', '

', @@ -44,7 +49,7 @@ public function get_data() { 'image_with_decimal_width' => array( '

', - '

', + '

', ), 'image_with_self_closing_tag' => array( From 6926f9b5bf99a09e18fd11f64b8cf11e4a7b38eb Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Tue, 27 Feb 2018 07:34:13 -0500 Subject: [PATCH 8/9] Removes superfluous whitespace --- tests/test-amp-img-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 8b292f4431f..fa2f1c216ba 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -31,7 +31,7 @@ public function get_data() { '

', '

', ), - + 'image_with_empty_width' => array( '

', '

', From 929698eaf618e572967762787c68b99c371f50df Mon Sep 17 00:00:00 2001 From: Davis Shaver Date: Tue, 27 Feb 2018 15:07:48 -0500 Subject: [PATCH 9/9] Add test for zero width and height --- tests/test-amp-img-sanitizer.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index fa2f1c216ba..c0cc0f10a3e 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -47,6 +47,11 @@ public function get_data() { '

', ), + 'image_with_zero_width_and_height' => array( + '

', + '

', + ), + 'image_with_decimal_width' => array( '

', '

',