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

Jump to bottom

Convert video src to HTTPS #1274

Merged
merged 5 commits into from
Jul 23, 2018
Merged

Convert video src to HTTPS #1274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56dc958
Force HTTPS on video and source src attribute.
Jul 19, 2018
95259cb
Force HTTPS for video poster.
Jul 19, 2018
3ebea25
Revert `poster` attribute back to not force HTTPS.
Jul 19, 2018
68ded0a
Abstract update src functionality to new method.
Jul 20, 2018
06e23e0
Adjust code style format for consistency.
Jul 23, 2018
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
41 changes: 30 additions & 11 deletions includes/sanitizers/class-amp-video-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public function get_selector_conversion_mapping() {
* Sanitize the <video> elements from the HTML contained in this instance's DOMDocument.
*
* @since 0.2
* @since 1.0 Set the filtered child node's src attribute.
*/
public function sanitize() {
$nodes = $this->dom->getElementsByTagName( self::$tag );
Expand Down Expand Up @@ -95,11 +96,12 @@ public function sanitize() {
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 );

}

/*
Expand All @@ -124,6 +126,7 @@ public function sanitize() {
* Filter video dimensions, try to get width and height from original file if missing.
*
* @param array $new_attributes Attributes.
*
* @return array Modified attributes.
*/
protected function filter_video_dimensions( $new_attributes ) {
Expand Down Expand Up @@ -151,26 +154,28 @@ protected function filter_video_dimensions( $new_attributes ) {
}
}
}

return $new_attributes;
}

/**
* "Filter" HTML attributes for <amp-audio> elements.
*
* @since 0.2
* @since 1.0 Force HTTPS for the src attribute.
*
* @param string[] $attributes {
* Attributes.
*
* @type string $src Video URL - Empty if HTTPS required per $this->args['require_https_src']
* @type int $width <video> attribute - Set to numeric value if px or %
* @type int $height <video> attribute - Set to numeric value if px or %
* @type string $poster <video> attribute - Pass along if found
* @type string $class <video> attribute - Pass along if found
* @type bool $controls <video> attribute - Convert 'false' to empty string ''
* @type bool $loop <video> attribute - Convert 'false' to empty string ''
* @type bool $muted <video> attribute - Convert 'false' to empty string ''
* @type bool $autoplay <video> attribute - Convert 'false' to empty string ''
* @type string $src Video URL - Empty if HTTPS required per $this->args['require_https_src']
* @type int $width <video> attribute - Set to numeric value if px or %
* @type int $height <video> attribute - Set to numeric value if px or %
* @type string $poster <video> attribute - Pass along if found
* @type string $class <video> attribute - Pass along if found
* @type bool $controls <video> attribute - Convert 'false' to empty string ''
* @type bool $loop <video> attribute - Convert 'false' to empty string ''
* @type bool $muted <video> attribute - Convert 'false' to empty string ''
* @type bool $autoplay <video> attribute - Convert 'false' to empty string ''
* }
* @return array Returns HTML attributes; removes any not specifically declared above from input.
*/
Expand All @@ -180,7 +185,7 @@ private function filter_attributes( $attributes ) {
foreach ( $attributes as $name => $value ) {
switch ( $name ) {
case 'src':
$out[ $name ] = $this->maybe_enforce_https_src( $value );
$out[ $name ] = $this->maybe_enforce_https_src( $value, true );
break;

case 'width':
Expand Down Expand Up @@ -218,4 +223,18 @@ private function filter_attributes( $attributes ) {

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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem but curious why this logic isn't inlined in the sanitize method? It seems a bit unnecessary to have a new method for this when putting an if statement would do.

Copy link
Contributor Author

@hellofromtonya hellofromtonya Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this logic isn't inlined in the sanitize method?

@westonruter I abstracted to reduce the cognitive load.

At first, I had this if code block inline within that method. But as I read down line-by-line in the method, there's quite of bit of tasks going on.

Now when reading the method and you get to the $this->update_src(), you understand that it handles updating the src without having to worry about the details.

Also, note the pattern in the sanitize method and how it deals with all of the attributes and each node. There is an exception with the layout, which could be abstracted into the set_layout() that first calls the parent and then sets the 'responsive' layout. But other than that, the method works at a bigger level, i.e. all attributes, the node, and its child nodes. Setting the src is a granular task such as setting the layout or video dimensions.

@westonruter If you prefer, I can move back to the inline solution and remove the abstracted method.

if ( $old_src === $new_src ) {
return;
}
$node->setAttribute( 'src', $new_src );
}
}
10 changes: 9 additions & 1 deletion tests/test-amp-video-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,15 @@ public function get_data() {

'https_not_required' => array(
'<video width="300" height="300" src="http://example.com/video.mp4"></video>',
'<amp-video width="300" height="300" src="http://example.com/video.mp4" layout="responsive"></amp-video>',
'<amp-video width="300" height="300" src="https://example.com/video.mp4" layout="responsive"></amp-video>',
),

'http_video_with_children' => array(
'<video width="480" height="300" poster="http://example.com/video-image.gif">
<source src="http://example.com/video.mp4" type="video/mp4">
<source src="http://example.com/video.ogv" type="video/ogg">
</video>',
'<amp-video width="480" height="300" poster="http://example.com/video-image.gif" layout="responsive"><source src="https://example.com/video.mp4" type="video/mp4"><source src="https://example.com/video.ogv" type="video/ogg"></amp-video>',
),
);
}
Expand Down