Skip to content

Commit

Permalink
Merge pull request #3728 from ampproject/fix/2146-erroneous-attributes
Browse files Browse the repository at this point in the history
Validate height, width & layout attributes; fix cover block with video background
  • Loading branch information
westonruter authored Dec 30, 2019
2 parents c26b635 + e42411f commit 2d0dab0
Show file tree
Hide file tree
Showing 20 changed files with 1,050 additions and 62 deletions.
8 changes: 4 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,16 @@ jobs:
php: "5.6"
env: WP_VERSION=latest DEV_LIB_ONLY=phpunit,phpsyntax INSTALL_PWA_PLUGIN=1

- name: PHP unit tests (5.6, WordPress 5.1)
php: "5.6"
- name: PHP unit tests (5.4, WordPress 5.1)
php: "5.4"
env: WP_VERSION=5.1 DEV_LIB_ONLY=phpunit

- name: PHP unit tests (5.5, WordPress 5.0)
php: "5.5"
env: WP_VERSION=5.0 DEV_LIB_ONLY=phpunit,phpsyntax

- name: PHP unit tests w/ external-http (5.4, WordPress 4.9)
php: "5.4"
- name: PHP unit tests w/ external-http (5.6, WordPress 4.9)
php: "5.6"
env: WP_VERSION=4.9 DEV_LIB_ONLY=phpunit,phpsyntax PHPUNIT_EXTRA_GROUP=external-http

- name: PHP unit tests (7.3, WordPress trunk)
Expand Down
1 change: 1 addition & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -916,6 +916,7 @@ function amp_get_content_sanitizers( $post = null ) {
'include_manifest_comment' => ( defined( 'WP_DEBUG' ) && WP_DEBUG ) ? 'always' : 'when_excessive',
],
'AMP_Meta_Sanitizer' => [],
'AMP_Layout_Sanitizer' => [],
'AMP_Tag_And_Attribute_Sanitizer' => [], // Note: This whitelist sanitizer must come at the end to clean up any remaining issues the other sanitizers didn't catch.
];

Expand Down
2 changes: 2 additions & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class AMP_Autoloader {
'AMP_Style_Sanitizer' => 'includes/sanitizers/class-amp-style-sanitizer',
'AMP_Script_Sanitizer' => 'includes/sanitizers/class-amp-script-sanitizer',
'AMP_Embed_Sanitizer' => 'includes/sanitizers/class-amp-embed-sanitizer',
'AMP_Layout_Sanitizer' => 'includes/sanitizers/class-amp-layout-sanitizer',
'AMP_Tag_And_Attribute_Sanitizer' => 'includes/sanitizers/class-amp-tag-and-attribute-sanitizer',
'AMP_Video_Sanitizer' => 'includes/sanitizers/class-amp-video-sanitizer',
'AMP_Core_Theme_Sanitizer' => 'includes/sanitizers/class-amp-core-theme-sanitizer',
Expand All @@ -104,6 +105,7 @@ class AMP_Autoloader {
'AMP_Validation_Callback_Wrapper' => 'includes/validation/class-amp-validation-callback-wrapper',
'AMP_Validated_URL_Post_Type' => 'includes/validation/class-amp-validated-url-post-type',
'AMP_Validation_Error_Taxonomy' => 'includes/validation/class-amp-validation-error-taxonomy',
'AMP_CSS_Length' => 'includes/validation/class-amp-css-length',
'AMP_CLI_Namespace' => 'includes/cli/class-amp-cli-namespace',
'AMP_CLI_Validation_Command' => 'includes/cli/class-amp-cli-validation-command',
'AMP_String_Utils' => 'includes/utils/class-amp-string-utils',
Expand Down
23 changes: 23 additions & 0 deletions includes/embeds/class-amp-core-block-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class AMP_Core_Block_Handler extends AMP_Base_Embed_Handler {
'core/categories' => 'ampify_categories_block',
'core/archives' => 'ampify_archives_block',
'core/video' => 'ampify_video_block',
'core/cover' => 'ampify_cover_block',
];

/**
Expand Down Expand Up @@ -155,4 +156,26 @@ public function ampify_video_block( $block_content, $block ) {

return $block_content;
}

/**
* Ampify cover block.
*
* This specifically fixes the layout of the block when a background video is assigned.
*
* @see \AMP_Video_Sanitizer::filter_video_dimensions()
*
* @param string $block_content The block content about to be appended.
* @param array $block The full block, including name and attributes.
* @return string Filtered block content.
*/
public function ampify_cover_block( $block_content, $block ) {
if ( isset( $block['attrs']['backgroundType'] ) && 'video' === $block['attrs']['backgroundType'] ) {
$block_content = preg_replace(
'/(?<=<video\s)/',
'layout="fill" object-fit="cover" ',
$block_content
);
}
return $block_content;
}
}
12 changes: 5 additions & 7 deletions includes/embeds/class-amp-facebook-embed.php
Original file line number Diff line number Diff line change
Expand Up @@ -191,14 +191,12 @@ private function get_embed_type( DOMElement $node ) {
private function create_amp_facebook_and_replace_node( Document $dom, DOMElement $node, $embed_type ) {

$attributes = [
'layout' => 'responsive',
'width' => $node->hasAttribute( 'data-width' ) ? $node->getAttribute( 'data-width' ) : $this->DEFAULT_WIDTH,
'height' => $node->hasAttribute( 'data-height' ) ? $node->getAttribute( 'data-height' ) : $this->DEFAULT_HEIGHT,
// The layout sanitizer will convert this to `layout` when being sanitized.
// The data attribute needs to be used so that the layout sanitizer will process it.
'data-amp-layout' => 'responsive',
'width' => $node->hasAttribute( 'data-width' ) ? $node->getAttribute( 'data-width' ) : $this->DEFAULT_WIDTH,
'height' => $node->hasAttribute( 'data-height' ) ? $node->getAttribute( 'data-height' ) : $this->DEFAULT_HEIGHT,
];
if ( '100%' === $attributes['width'] ) {
$attributes['layout'] = 'fixed-height';
$attributes['width'] = 'auto';
}

$node->removeAttribute( 'data-width' );
$node->removeAttribute( 'data-height' );
Expand Down
46 changes: 33 additions & 13 deletions includes/sanitizers/class-amp-base-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -248,17 +248,31 @@ public function sanitize_dimension( $value, $dimension ) {
return '';
}

/**
* Determine if an attribute value is empty.
*
* @param string|null $value Attribute value.
* @return bool True if empty, false if not.
*/
public function is_empty_attribute_value( $value ) {
return ! isset( $value ) || '' === $value;
}

/**
* Sets the layout, and possibly the 'height' and 'width' attributes.
*
* @param array $attributes {
* Attributes.
*
* @type string $bottom
* @type int|string $height
* @type int|string $width
* @type string $sizes
* @type string $class
* @type string $layout
* @type string $left
* @type string $position
* @type string $right
* @type string $style
* @type string $top
* @type int|string $width
* }
* @return array Attributes.
*/
Expand Down Expand Up @@ -312,21 +326,27 @@ public function set_layout( $attributes ) {
&& '100%' === $attributes['width']
&& '100%' === $attributes['height']
) {
unset( $attributes['style'], $attributes['width'], $attributes['height'] );
unset( $attributes['style'], $styles['position'], $attributes['width'], $attributes['height'] );
if ( ! empty( $styles ) ) {
$attributes['style'] = $this->reassemble_style_string( $styles );
}
$attributes['layout'] = 'fill';
unset( $attributes['height'], $attributes['width'] );
return $attributes;
}
}

if ( empty( $attributes['height'] ) ) {
unset( $attributes['width'] );
$attributes['height'] = self::FALLBACK_HEIGHT;
}

if ( empty( $attributes['width'] ) || '100%' === $attributes['width'] ) {
$attributes['layout'] = 'fixed-height';
$attributes['width'] = 'auto';
if ( isset( $attributes['width'], $attributes['height'] ) && '100%' === $attributes['width'] && '100%' === $attributes['height'] ) {
unset( $attributes['width'], $attributes['height'] );
$attributes['layout'] = 'fill';
} else {
if ( empty( $attributes['height'] ) ) {
unset( $attributes['width'] );
$attributes['height'] = self::FALLBACK_HEIGHT;
}
if ( empty( $attributes['width'] ) || '100%' === $attributes['width'] ) {
$attributes['layout'] = 'fixed-height';
$attributes['width'] = 'auto';
}
}

return $attributes;
Expand Down
118 changes: 118 additions & 0 deletions includes/sanitizers/class-amp-layout-sanitizer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
<?php
/**
* Class AMP_Layout_Sanitizer
*
* @since 1.5.0
* @package AMP
*/

/**
* Class AMP_Layout_Sanitizer
*
* @since 1.5.0
*/
class AMP_Layout_Sanitizer extends AMP_Base_Sanitizer {
use AMP_Noscript_Fallback;

/**
* Sanitize any element that has the `layout` or `data-amp-layout` attribute.
*/
public function sanitize() {
$xpath = new DOMXPath( $this->dom );

// Elements with the `layout` attribute will be validated by `AMP_Tag_And_Attribute_Sanitizer`.
$nodes = $xpath->query( '//*[ not( @layout ) and ( @data-amp-layout or @width or @height or @style ) ]' );

foreach ( $nodes as $node ) {
/**
* Element.
*
* @var DOMElement $node
*/

// Layout does not apply inside of noscript.
if ( $this->is_inside_amp_noscript( $node ) ) {
continue;
}

$width = $node->getAttribute( 'width' );
$height = $node->getAttribute( 'height' );
$style = $node->getAttribute( 'style' );

// The `layout` attribute can also be defined through the `data-amp-layout` attribute.
if ( $node->hasAttribute( 'data-amp-layout' ) ) {
$layout = $node->getAttribute( 'data-amp-layout' );
$node->setAttribute( 'layout', $layout );
$node->removeAttribute( 'data-amp-layout' );
}

if ( ! $this->is_empty_attribute_value( $style ) ) {
$styles = $this->parse_style_string( $style );

/*
* If both height & width descriptors are 100%, or
* width attribute is 100% and height style descriptor is 100%, or
* height attribute is 100% and width style descriptor is 100%
* then apply fill layout.
*/
if (
(
isset( $styles['width'], $styles['height'] ) &&
( '100%' === $styles['width'] && '100%' === $styles['height'] )
) ||
(
( ! $this->is_empty_attribute_value( $width ) && '100%' === $width ) &&
( isset( $styles['height'] ) && '100%' === $styles['height'] )
) ||
(
( ! $this->is_empty_attribute_value( $height ) && '100%' === $height ) &&
( isset( $styles['width'] ) && '100%' === $styles['width'] )
)
) {
unset( $styles['width'], $styles['height'] );
$node->removeAttribute( 'width' );
$node->removeAttribute( 'height' );

if ( empty( $styles ) ) {
$node->removeAttribute( 'style' );
} else {
$node->setAttribute( 'style', $this->reassemble_style_string( $styles ) );
}

$this->set_layout_fill( $node );
continue;
}
}

// If the width & height are `100%` then apply fill layout.
if ( '100%' === $width && '100%' === $height ) {
$this->set_layout_fill( $node );
continue;
}

// If the width is `100%`, convert the layout to `fixed-height` and width to `auto`.
if ( '100%' === $width ) {
$node->setAttribute( 'width', 'auto' );
$node->setAttribute( 'layout', AMP_Rule_Spec::LAYOUT_FIXED_HEIGHT );
}
}

$this->did_convert_elements = true;
}

/**
* Apply the `fill` layout.
*
* @param DOMElement $node Node.
*/
private function set_layout_fill( DOMElement $node ) {
if ( $node->hasAttribute( 'width' ) && $node->hasAttribute( 'height' ) ) {
$node->removeAttribute( 'width' );
$node->removeAttribute( 'height' );
}

if ( AMP_Rule_Spec::LAYOUT_FILL !== $node->getAttribute( 'layout' ) ) {
$node->setAttribute( 'layout', AMP_Rule_Spec::LAYOUT_FILL );
}
}
}
22 changes: 17 additions & 5 deletions includes/sanitizers/class-amp-rule-spec.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,14 @@
* Set of constants used throughout the sanitizer.
*/
abstract class AMP_Rule_Spec {

/**
/*
* AMP rule_spec types
*/
const ATTR_SPEC_LIST = 'attr_spec_list';
const TAG_SPEC = 'tag_spec';
const CDATA = 'cdata';

/**
/*
* AMP attr_spec value check results.
*
* In 0.7 these changed from strings to integers to speed up comparisons.
Expand All @@ -28,7 +27,7 @@ abstract class AMP_Rule_Spec {
const FAIL = 0;
const NOT_APPLICABLE = -1;

/**
/*
* HTML Element Tag rule names
*/
const DISALLOWED_ANCESTOR = 'disallowed_ancestor';
Expand All @@ -37,7 +36,7 @@ abstract class AMP_Rule_Spec {
const DESCENDANT_TAG_LIST = 'descendant_tag_list';
const CHILD_TAGS = 'child_tags';

/**
/*
* HTML Element Attribute rule names
*/
const ALLOW_EMPTY = 'allow_empty';
Expand All @@ -53,6 +52,19 @@ abstract class AMP_Rule_Spec {
const VALUE_PROPERTIES = 'value_properties';
const VALUE_URL = 'value_url';

/*
* AMP layout types
*/
const LAYOUT_NODISPLAY = 'nodisplay';
const LAYOUT_FIXED = 'fixed';
const LAYOUT_FIXED_HEIGHT = 'fixed-height';
const LAYOUT_RESPONSIVE = 'responsive';
const LAYOUT_CONTAINER = 'container';
const LAYOUT_FILL = 'fill';
const LAYOUT_FLEX_ITEM = 'flex-item';
const LAYOUT_FLUID = 'fluid';
const LAYOUT_INTRINSIC = 'intrinsic';

/**
* Attribute name for AMP dev mode.
*
Expand Down
Loading

0 comments on commit 2d0dab0

Please sign in to comment.