From dd2135384db616c24545c28967da4d241fa460c2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 27 Sep 2018 15:14:05 -0700 Subject: [PATCH 1/3] Fix undefined index notice when video lacks src attribute --- includes/sanitizers/class-amp-video-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index bb0dd199168..ac8fb165e99 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -130,7 +130,7 @@ public function sanitize() { * @return array Modified attributes. */ protected function filter_video_dimensions( $new_attributes ) { - if ( empty( $new_attributes['width'] ) || empty( $new_attributes['height'] ) ) { + if ( isset( $new_attributes['src'] ) && ( empty( $new_attributes['width'] ) || empty( $new_attributes['height'] ) ) ) { // Get the width and height from the file. $ext = pathinfo( $new_attributes['src'], PATHINFO_EXTENSION ); From cf6d2a281add8a652944500b6eaf39e61d872e97 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 27 Sep 2018 16:29:18 -0700 Subject: [PATCH 2/3] Preserve extra video attributes and non-source children in amp-video conversion There is no need to remove unrecognized attributes because these will be handled by the whitelist sanitizer. Also preserve all original child nodes of video so that track elements will be preserved. --- .../sanitizers/class-amp-base-sanitizer.php | 2 +- .../sanitizers/class-amp-video-sanitizer.php | 96 ++++++++----------- tests/test-amp-video-sanitizer.php | 6 +- 3 files changed, 44 insertions(+), 60 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index fe9c65bd56a..5319a9f7dce 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -317,7 +317,7 @@ public function add_or_append_attribute( &$attributes, $key, $value, $separator * @return string URL which may have been updated with HTTPS, or may have been made empty. */ public function maybe_enforce_https_src( $src, $force_https = false ) { - $protocol = strtok( $src, ':' ); + $protocol = strtok( $src, ':' ); // @todo What about relative URLs? This should use wp_parse_url( $src, PHP_URL_SCHEME ) if ( 'https' !== $protocol ) { // Check if https is required. if ( isset( $this->args['require_https_src'] ) && true === $this->args['require_https_src'] ) { diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index ac8fb165e99..b52c13da009 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -62,46 +62,43 @@ public function sanitize() { $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); $old_attributes = $this->filter_data_amp_attributes( $old_attributes, $amp_data ); + $sources_count = 0; $new_attributes = $this->filter_attributes( $old_attributes ); $layout = isset( $amp_data['layout'] ) ? $amp_data['layout'] : false; - $new_attributes = $this->filter_video_dimensions( $new_attributes ); + if ( isset( $new_attributes['src'] ) ) { + $new_attributes = $this->filter_video_dimensions( $new_attributes, $new_attributes['src'] ); + if ( $new_attributes['src'] ) { + $sources_count++; + } + } + + // Gather all child nodes and supply empty video dimensions from sources. + $child_nodes = array(); + while ( $node->firstChild ) { + $child_node = $node->removeChild( $node->firstChild ); + if ( $child_node instanceof DOMElement && 'source' === $child_node->nodeName && $child_node->hasAttribute( 'src' ) ) { + $src = $this->maybe_enforce_https_src( $child_node->getAttribute( 'src' ), true ); + if ( ! $src ) { + // @todo $this->remove_invalid_child( $child_node ), but this will require refactoring the while loop since it uses firstChild. + continue; // Skip adding source. + } + $sources_count++; + $child_node->setAttribute( 'src', $src ); + $new_attributes = $this->filter_video_dimensions( $new_attributes, $src ); + } + $child_nodes[] = $child_node; + } + $new_attributes = $this->filter_attachment_layout_attributes( $node, $new_attributes, $layout ); - $new_attributes = $this->set_layout( $new_attributes ); if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['width'] ) && ! empty( $new_attributes['height'] ) ) { $new_attributes['layout'] = 'responsive'; } + $new_attributes = $this->set_layout( $new_attributes ); + // @todo Make sure poster and artwork attributes are HTTPS. $new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes ); - - foreach ( $node->childNodes as $child_node ) { - /** - * Child node. - * - * @todo: Fix when `source` has no closing tag as DOMDocument does not handle well. - * - * @var DOMNode $child_node - */ - $new_child_node = $child_node->cloneNode( true ); - if ( ! $new_child_node instanceof DOMElement ) { - continue; - } - - $old_child_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $new_child_node ); - $new_child_attributes = $this->filter_attributes( $old_child_attributes ); - - if ( empty( $new_child_attributes['src'] ) ) { - continue; - } - if ( 'source' !== $new_child_node->tagName ) { - continue; - } - - $this->update_src( $new_child_node, $new_child_attributes['src'], $old_child_attributes['src'] ); - - /** - * Only append source tags with a valid src attribute - */ - $new_node->appendChild( $new_child_node ); + foreach ( $child_nodes as $child_node ) { + $new_node->appendChild( $child_node ); } /* @@ -111,7 +108,7 @@ public function sanitize() { * TODO: Add a fallback handler. * See: https://github.com/ampproject/amphtml/issues/2261 */ - if ( 0 === $new_node->childNodes->length && empty( $new_attributes['src'] ) ) { + if ( 0 === $sources_count ) { $this->remove_invalid_child( $node ); } else { $node->parentNode->replaceChild( $new_node, $node ); @@ -125,16 +122,17 @@ public function sanitize() { /** * Filter video dimensions, try to get width and height from original file if missing. * - * @param array $new_attributes Attributes. - * + * @param array $new_attributes Attributes. + * @param string $src Video URL. * @return array Modified attributes. */ - protected function filter_video_dimensions( $new_attributes ) { - if ( isset( $new_attributes['src'] ) && ( empty( $new_attributes['width'] ) || empty( $new_attributes['height'] ) ) ) { + protected function filter_video_dimensions( $new_attributes, $src ) { + if ( empty( $new_attributes['width'] ) || empty( $new_attributes['height'] ) ) { // Get the width and height from the file. - $ext = pathinfo( $new_attributes['src'], PATHINFO_EXTENSION ); - $name = wp_basename( $new_attributes['src'], ".$ext" ); + $path = wp_parse_url( $src, PHP_URL_PATH ); + $ext = pathinfo( $path, PATHINFO_EXTENSION ); + $name = sanitize_title( wp_basename( $path, ".$ext" ) ); $args = array( 'name' => $name, 'post_type' => 'attachment', @@ -193,9 +191,9 @@ private function filter_attributes( $attributes ) { $out[ $name ] = $this->sanitize_dimension( $value, $name ); break; + // @todo Convert to HTTPS when is_ssl(). case 'poster': - case 'class': - case 'sizes': + case 'artwork': $out[ $name ] = $value; break; @@ -217,24 +215,10 @@ private function filter_attributes( $attributes ) { break; default: - break; + $out[ $name ] = $value; } } return $out; } - - /** - * Update the node's src attribute if it is different from the old src attribute. - * - * @param DOMNode $node The given DOMNode. - * @param string $new_src The new src attribute. - * @param string $old_src The old src attribute. - */ - protected function update_src( &$node, $new_src, $old_src ) { - if ( $old_src === $new_src ) { - return; - } - $node->setAttribute( 'src', $new_src ); - } } diff --git a/tests/test-amp-video-sanitizer.php b/tests/test-amp-video-sanitizer.php index bd14a307add..b385c169d85 100644 --- a/tests/test-amp-video-sanitizer.php +++ b/tests/test-amp-video-sanitizer.php @@ -40,9 +40,9 @@ public function get_data() { '', ), - 'video_with_blacklisted_attribute' => array( - '', - '', + 'video_with_custom_attribute' => array( + '', + '', ), 'video_with_sizes_attribute_is_overridden' => array( From c1f3cfff769db078d8f4b71417edab05e8d692c0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 27 Sep 2018 17:08:10 -0700 Subject: [PATCH 3/3] Set fallback attribute on first non-source/track child element of amp-video --- includes/class-amp-theme-support.php | 1 + includes/sanitizers/class-amp-video-sanitizer.php | 7 +++++++ tests/test-amp-video-sanitizer.php | 14 +++++++++----- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index f860cbeae68..3341572ba50 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -766,6 +766,7 @@ public static function add_hooks() { remove_action( 'wp_print_styles', 'print_emoji_styles' ); remove_action( 'wp_head', 'wp_oembed_add_host_js' ); + // @todo The wp_mediaelement_fallback() should still run to be injected inside of the audio/video generated by wp_audio_shortcode()/wp_video_shortcode() respectively. // Prevent MediaElement.js scripts/styles from being enqueued. add_filter( 'wp_video_shortcode_library', function() { return 'amp'; diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index b52c13da009..201975e631c 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -73,6 +73,7 @@ public function sanitize() { } // Gather all child nodes and supply empty video dimensions from sources. + $fallback = null; $child_nodes = array(); while ( $node->firstChild ) { $child_node = $node->removeChild( $node->firstChild ); @@ -86,6 +87,12 @@ public function sanitize() { $child_node->setAttribute( 'src', $src ); $new_attributes = $this->filter_video_dimensions( $new_attributes, $src ); } + + if ( ! $fallback && $child_node instanceof DOMElement && ! ( 'source' === $child_node->nodeName || 'track' === $child_node->nodeName ) ) { + $fallback = $child_node; + $fallback->setAttribute( 'fallback', '' ); + } + $child_nodes[] = $child_node; } diff --git a/tests/test-amp-video-sanitizer.php b/tests/test-amp-video-sanitizer.php index b385c169d85..9f37f3ab154 100644 --- a/tests/test-amp-video-sanitizer.php +++ b/tests/test-amp-video-sanitizer.php @@ -102,11 +102,8 @@ public function get_data() { ), 'http_video_with_children' => array( - '', - '', + '', + 'http://example.com/video.mp4', ), ); } @@ -118,6 +115,10 @@ public function test_converter( $source, $expected ) { $dom = AMP_DOM_Utils::get_dom_from_content( $source ); $sanitizer = new AMP_Video_Sanitizer( $dom ); $sanitizer->sanitize(); + + $whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom ); + $whitelist_sanitizer->sanitize(); + $content = AMP_DOM_Utils::get_content_from_dom( $dom ); $this->assertEquals( $expected, $content ); } @@ -132,6 +133,9 @@ public function test__https_required() { ) ); $sanitizer->sanitize(); + $whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom ); + $whitelist_sanitizer->sanitize(); + $content = AMP_DOM_Utils::get_content_from_dom( $dom ); $this->assertEquals( $expected, $content ); }