Navigation Menu

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

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 2 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions includes/sanitizers/class-amp-video-sanitizer.php
View file
Open in desktop
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,14 @@ public function sanitize() {
continue;
}

if ( $old_child_attributes['src'] !== $new_child_attributes['src'] ) {
$new_child_node->setAttribute( 'src', $new_child_attributes['src'] );
}

/**
* Only append source tags with a valid src attribute
*/
$new_node->appendChild( $new_child_node );

}

/*
Expand Down Expand Up @@ -158,6 +162,7 @@ protected function filter_video_dimensions( $new_attributes ) {
* "Filter" HTML attributes for <amp-audio> elements.
*
* @since 0.2
* @since 1.0 Force HTTPS for src and poster attributes.
*
* @param string[] $attributes {
* Attributes.
Expand All @@ -179,16 +184,16 @@ private function filter_attributes( $attributes ) {

foreach ( $attributes as $name => $value ) {
switch ( $name ) {
case 'poster':
Copy link
Contributor

@kienstra kienstra Jul 19, 2018 •
edited

Choose a reason for hiding this comment

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

This is an edge case. But do you think the poster should be forced to HTTPS like it is here?

It looks like HTTP is allowed for poster. For example, passing this amp-video with an HTTP poster to the AMP Validator results in a valid page.

The HTTPS request might fail if we upgrade it from HTTP. But we're already upgrading the src to HTTPS, and that might be on the same domain. So this probably wouldn't be a common issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good observation, @kienstra. You're right, it does validate. Therefore, we do not need to force the poster to HTTPS per the edge case you identified. I'll roll that part back.

case 'src':
$out[ $name ] = $this->maybe_enforce_https_src( $value );
$out[ $name ] = $this->maybe_enforce_https_src( $value, true );
break;

case 'width':
case 'height':
$out[ $name ] = $this->sanitize_dimension( $value, $name );
break;

case 'poster':
case 'class':
case 'sizes':
$out[ $name ] = $value;
Expand Down
10 changes: 9 additions & 1 deletion tests/test-amp-video-sanitizer.php
View file
Open in desktop
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="https://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>',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests to ensure HTTPS is forced.

),
);
}
Expand Down