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

Embed fallbacks #493

Closed
wants to merge 9 commits into from
Closed

Embed fallbacks #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2c11b6a
Sanitize audio/video: update source attributes
coreymckrill Sep 27, 2016
696d1d0
Fallbacks: Replace invalid audio/video element with fallback element
coreymckrill Sep 28, 2016
3832b59
Fallbacks: update audio/video tests to reflect https is now required
coreymckrill Sep 29, 2016
9e02d56
Fallbacks: Replace invalid iframe element with fallback element
coreymckrill Sep 29, 2016
e5a6253
Maybe set https scheme on URLs with relative protocols
coreymckrill Sep 29, 2016
a689a2c
Fallbacks: update iframe tests to reflect that https is now required
coreymckrill Sep 29, 2016
2365364
Fallbacks: abstract fallback node creation to a base class method
coreymckrill Oct 6, 2016
b1077d6
Fallbacks: update tests
coreymckrill Oct 6, 2016
f7dafe5
Detect and handle protocol- and path-relative URLs in base sanitizer
coreymckrill Oct 6, 2016
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
33 changes: 29 additions & 4 deletions includes/sanitizers/class-amp-audio-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ class AMP_Audio_Sanitizer extends AMP_Base_Sanitizer {
private static $script_slug = 'amp-audio';
private static $script_src = 'https://cdn.ampproject.org/v0/amp-audio-0.1.js';

protected $DEFAULT_ARGS = array(
'require_https_src' => true,
);

public function get_scripts() {
if ( ! $this->did_convert_elements ) {
return array();
Expand All @@ -24,9 +28,15 @@ public function sanitize() {
}

for ( $i = $num_nodes - 1; $i >= 0; $i-- ) {
$orig_src = array();

$node = $nodes->item( $i );
$old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node );

if ( isset( $old_attributes['src'] ) ) {
$orig_src[] = $old_attributes['src'];
}

$new_attributes = $this->filter_attributes( $old_attributes );

$new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-audio', $new_attributes );
Expand All @@ -35,21 +45,36 @@ public function sanitize() {
foreach ( $node->childNodes as $child_node ) {
$new_child_node = $child_node->cloneNode( true );
$old_child_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $new_child_node );

if ( isset( $old_child_attributes['src'] ) ) {
$orig_src[] = $old_child_attributes['src'];
}

$new_child_attributes = $this->filter_attributes( $old_child_attributes );

// Only append source tags with a valid src attribute
if ( ! empty( $new_child_attributes['src'] ) && 'source' === $new_child_node->tagName ) {
AMP_DOM_Utils::add_attributes_to_node( $new_child_node, $new_child_attributes );
$new_node->appendChild( $new_child_node );
}
}

// If the node has at least one valid source, replace the old node with it.
// If the node has no valid sources, but at least one invalid (http) one, add a fallback element.
// Otherwise, just remove the node.
//
// TODO: Add a fallback handler.
// See: https://github.com/ampproject/amphtml/issues/2261
if ( 0 === $new_node->childNodes->length && empty( $new_attributes['src'] ) ) {
$node->parentNode->removeChild( $node );
if ( ! empty( $orig_src ) ) {
$fallback_node = $this->create_fallback_node(
sprintf(
wp_kses( __( 'Could not load <a href="%s">audio</a>.', 'amp' ), array( 'a' => array( 'href' => true ) ) ),
esc_url( array_shift( $orig_src ) )
)
);

$node->parentNode->replaceChild( $fallback_node, $node );
} else {
$node->parentNode->removeChild( $node );
}
} else {
$node->parentNode->replaceChild( $new_node, $node );
}
Expand Down
44 changes: 41 additions & 3 deletions includes/sanitizers/class-amp-base-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,57 @@ public function add_or_append_attribute( &$attributes, $key, $value, $separator
* @return string
*/
public function maybe_enforce_https_src( $src, $force_https = false ) {
$https_required = isset( $this->args['require_https_src'] ) && true === $this->args['require_https_src'];
$protocol = strtok( $src, ':' );

if ( $protocol === $src && ( $https_required || $force_https ) ) {
if ( 0 === strpos( $src, '//' ) ) {
// src has relative protocol, ie //example.com/asdf, so add https
$src = set_url_scheme( $src, 'https' );
} else if ( 0 === strpos( $src, '/' ) ) {
// src is URL relative to the site root
$src = home_url( $src );
} else {
// src is URL relative to current URI
global $wp;
$src = home_url( trailingslashit( $wp->request ) . $src );
}

$protocol = strtok( $src, ':' );
}

if ( 'https' !== $protocol ) {
// Check if https is required
if ( isset( $this->args['require_https_src'] ) && true === $this->args['require_https_src'] ) {
if ( $https_required ) {
// Remove the src. Let the implementing class decide what do from here.
$src = '';
} elseif ( ( ! isset( $this->args['require_https_src'] ) || false === $this->args['require_https_src'] )
&& true === $force_https ) {
} elseif ( ! $https_required && true === $force_https ) {
// Don't remove the src, but force https instead
$src = set_url_scheme( $src, 'https' );
}
}

return $src;
}

protected function create_fallback_node( $content, $container_el = 'blockquote', $attributes = array() ) {
$defaults = array(
'class' => 'amp-wp-fallback',
);
$attributes = wp_parse_args( $attributes, $defaults );

$fallback_node = AMP_DOM_Utils::create_node(
$this->dom,
$container_el,
$attributes
);

if ( $content ) {
$fallback_content = $this->dom->createDocumentFragment();
$fallback_content->appendXML( $content );
$fallback_node->appendChild( $fallback_content );
}

return $fallback_node;
}
}
30 changes: 22 additions & 8 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 @@ -15,7 +15,8 @@ class AMP_Iframe_Sanitizer extends AMP_Base_Sanitizer {
private static $script_src = 'https://cdn.ampproject.org/v0/amp-iframe-0.1.js';

protected $DEFAULT_ARGS = array(
'add_placeholder' => false,
'require_https_src' => true,
'add_placeholder' => false,
);

public function get_scripts() {
Expand All @@ -37,16 +38,29 @@ public function sanitize() {
$node = $nodes->item( $i );
$old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node );

$orig_src = '';
if ( isset( $old_attributes['src'] ) ) {
$orig_src = $old_attributes['src'];
}

$new_attributes = $this->filter_attributes( $old_attributes );

// If the src doesn't exist, remove the node.
// This means that it never existed or was invalidated
// while filtering attributes above.
//
// TODO: add a filter to allow for a fallback element in this instance.
// See: https://github.com/ampproject/amphtml/issues/2261
// If the filtered src doesn't exist, but there's an invalid src, add a fallback element.
// Otherwise remove the node.
if ( empty( $new_attributes['src'] ) ) {
$node->parentNode->removeChild( $node );
if ( ! empty( $orig_src ) ) {
$fallback_node = $this->create_fallback_node(
sprintf(
wp_kses( __( 'Could not load <a href="%s">iframe</a>.', 'amp' ), array( 'a' => array( 'href' => true ) ) ),
esc_url( $orig_src )
)
);

$node->parentNode->replaceChild( $fallback_node, $node );
} else {
$node->parentNode->removeChild( $node );
}

continue;
}

Expand Down
33 changes: 29 additions & 4 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 @@ -10,6 +10,10 @@ class AMP_Video_Sanitizer extends AMP_Base_Sanitizer {

public static $tag = 'video';

protected $DEFAULT_ARGS = array(
'require_https_src' => true,
);

public function sanitize() {
$nodes = $this->dom->getElementsByTagName( self::$tag );
$num_nodes = $nodes->length;
Expand All @@ -18,9 +22,15 @@ public function sanitize() {
}

for ( $i = $num_nodes - 1; $i >= 0; $i-- ) {
$orig_src = array();

$node = $nodes->item( $i );
$old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node );

if ( isset( $old_attributes['src'] ) ) {
$orig_src[] = $old_attributes['src'];
}

$new_attributes = $this->filter_attributes( $old_attributes );

$new_attributes = $this->enforce_fixed_height( $new_attributes );
Expand All @@ -32,21 +42,36 @@ public function sanitize() {
foreach ( $node->childNodes as $child_node ) {
$new_child_node = $child_node->cloneNode( true );
$old_child_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $new_child_node );

if ( isset( $old_child_attributes['src'] ) ) {
$orig_src[] = $old_child_attributes['src'];
}

$new_child_attributes = $this->filter_attributes( $old_child_attributes );

// Only append source tags with a valid src attribute
if ( ! empty( $new_child_attributes['src'] ) && 'source' === $new_child_node->tagName ) {
AMP_DOM_Utils::add_attributes_to_node( $new_child_node, $new_child_attributes );
$new_node->appendChild( $new_child_node );
}
}

// If the node has at least one valid source, replace the old node with it.
// If the node has no valid sources, but at least one invalid (http) one, add a fallback element.
// Otherwise, just remove the node.
//
// TODO: Add a fallback handler.
// See: https://github.com/ampproject/amphtml/issues/2261
if ( 0 === $new_node->childNodes->length && empty( $new_attributes['src'] ) ) {
$node->parentNode->removeChild( $node );
if ( ! empty( $orig_src ) ) {
$fallback_node = $this->create_fallback_node(
sprintf(
wp_kses( __( 'Could not load <a href="%s">video</a>.', 'amp' ), array( 'a' => array( 'href' => true ) ) ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we switch the statement from an error ("Could not load") to more of a action ("Watch video"). It's not entirely accurate to say that we couldn't load the video. (Note: this applies to all the sanitizers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use "Watch video" and "Listen to audio", but I'm not sure what we'd do for the iframe one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I'm not sure "Could not load" is really misleading, since the AMP code is preventing non-https sources from loading.

esc_url( array_shift( $orig_src ) )
)
);

$node->parentNode->replaceChild( $fallback_node, $node );
} else {
$node->parentNode->removeChild( $node );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This block of code (and the similar ones across the other sanitizers) could probably be abstracted to a new method?

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 idea. 2365364

} else {
$node->parentNode->replaceChild( $new_node, $node );
}
Expand Down
9 changes: 8 additions & 1 deletion tests/test-amp-audio-converter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,17 @@ public function get_data() {
'<amp-audio width="400" height="300"><source src="https://example.com/foo.wav" type="audio/wav"/></amp-audio><amp-audio width="400" height="300" src="https://example.com/audio/file.ogg"></amp-audio><amp-audio height="500" width="300"><source src="https://example.com/foo2.wav" type="audio/wav"/></amp-audio>'
),

/*
'https_not_required' => array(
'<audio width="400" height="300" src="http://example.com/audio/file.ogg"></audio>',
'<amp-audio width="400" height="300" src="http://example.com/audio/file.ogg"></amp-audio>',
),
*/

'https_required' => array(
'<audio width="400" height="300" src="http://example.com/audio/file.ogg"></audio>',
'<blockquote class="amp-wp-fallback">Could not load <a href="http://example.com/audio/file.ogg">audio</a>.</blockquote>',
),
);
}

Expand All @@ -90,7 +97,7 @@ public function test_converter( $source, $expected ) {

public function test__https_required() {
$source = '<audio width="400" height="300" src="http://example.com/audio/file.ogg"></audio>';
$expected = '';
$expected = '<blockquote class="amp-wp-fallback">Could not load <a href="http://example.com/audio/file.ogg">audio</a>.</blockquote>';

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Audio_Sanitizer( $dom, array(
Expand Down
4 changes: 2 additions & 2 deletions tests/test-amp-iframe-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public function get_data() {

'force_https' => array(
'<iframe src="http://example.com/embed/132886713" width="500" height="281" frameborder="0" 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" sizes="(min-width: 500px) 500px, 100vw"></amp-iframe>',
'<blockquote class="amp-wp-fallback">Could not load <a href="http://example.com/embed/132886713">iframe</a>.</blockquote>',
),

'iframe_without_dimensions' => array(
Expand Down Expand Up @@ -116,7 +116,7 @@ public function test_converter( $source, $expected ) {

public function test__https_required() {
$source = '<iframe src="http://example.com/embed/132886713"></iframe>';
$expected = '';
$expected = '<blockquote class="amp-wp-fallback">Could not load <a href="http://example.com/embed/132886713">iframe</a>.</blockquote>';

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Iframe_Sanitizer( $dom, array(
Expand Down
11 changes: 9 additions & 2 deletions tests/test-amp-video-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,17 @@ public function get_data() {
'<amp-video src="https://example.com/video1.mp4" width="480" height="300" sizes="(min-width: 480px) 480px, 100vw" class="amp-wp-enforced-sizes"></amp-video><amp-video src="https://example.com/video2.ogv" width="300" height="480" sizes="(min-width: 300px) 300px, 100vw" class="amp-wp-enforced-sizes"></amp-video><amp-video src="https://example.com/video3.webm" height="100" width="200" sizes="(min-width: 200px) 200px, 100vw" class="amp-wp-enforced-sizes"></amp-video>'
),

/*
'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" sizes="(min-width: 300px) 300px, 100vw" class="amp-wp-enforced-sizes"></amp-video>',
)
),
*/

'https_required' => array(
'<video width="300" height="300" src="http://example.com/video.mp4"></video>',
'<blockquote class="amp-wp-fallback">Could not load <a href="http://example.com/video.mp4">video</a>.</blockquote>',
),
);
}

Expand All @@ -91,7 +98,7 @@ public function test_converter( $source, $expected ) {

public function test__https_required() {
$source = '<video width="300" height="300" src="http://example.com/video.mp4"></video>';
$expected = '';
$expected = '<blockquote class="amp-wp-fallback">Could not load <a href="http://example.com/video.mp4">video</a>.</blockquote>';

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Video_Sanitizer( $dom, array(
Expand Down