From 9d46db4f7761554d0306b2345dde68ca1593bfa7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 4 Feb 2018 22:30:07 -0800 Subject: [PATCH 1/7] Eliminate custom WP_Styles in favor of consolidating logic in AMP_Style_Sanitizer Run !important and overflow removal logic on inlined external stylesheets as well as style elements and style attributes. See #927. --- includes/class-amp-autoloader.php | 1 - includes/class-amp-theme-support.php | 87 +----- includes/class-amp-wp-styles.php | 251 --------------- .../sanitizers/class-amp-style-sanitizer.php | 285 ++++++++++++++---- tests/test-amp-style-sanitizer.php | 203 +++++++++++-- tests/test-class-amp-theme-support.php | 19 -- tests/test-class-amp-wp-styles.php | 221 -------------- 7 files changed, 413 insertions(+), 654 deletions(-) delete mode 100644 includes/class-amp-wp-styles.php delete mode 100644 tests/test-class-amp-wp-styles.php diff --git a/includes/class-amp-autoloader.php b/includes/class-amp-autoloader.php index f9dfb3dc175..16880ed1325 100644 --- a/includes/class-amp-autoloader.php +++ b/includes/class-amp-autoloader.php @@ -31,7 +31,6 @@ class AMP_Autoloader { private static $_classmap = array( 'AMP_Theme_Support' => 'includes/class-amp-theme-support', 'AMP_Comment_Walker' => 'includes/class-amp-comment-walker', - 'AMP_WP_Styles' => 'includes/class-amp-wp-styles', 'AMP_Template_Customizer' => 'includes/admin/class-amp-customizer', 'AMP_Post_Meta_Box' => 'includes/admin/class-amp-post-meta-box', 'AMP_Post_Type_Support' => 'includes/class-amp-post-type-support', diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index ffae0b69ea0..9a71c112404 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -19,13 +19,6 @@ class AMP_Theme_Support { */ const SCRIPTS_PLACEHOLDER = ''; - /** - * Replaced with the necessary styles. - * - * @var string - */ - const STYLES_PLACEHOLDER = ''; - /** * Sanitizer classes. * @@ -152,16 +145,11 @@ public static function register_hooks() { // Remove core actions which are invalid AMP. remove_action( 'wp_head', 'wp_post_preview_js', 1 ); - remove_action( 'wp_head', 'locale_stylesheet' ); // Replaced below in add_amp_styles_placeholder() method. remove_action( 'wp_head', 'print_emoji_detection_script', 7 ); - remove_action( 'wp_head', 'wp_print_styles', 8 ); // Replaced below in add_amp_styles_placeholder() method. remove_action( 'wp_head', 'wp_print_head_scripts', 9 ); - remove_action( 'wp_head', 'wp_custom_css_cb', 101 ); // Replaced below in add_amp_styles_placeholder() method. remove_action( 'wp_footer', 'wp_print_footer_scripts', 20 ); remove_action( 'wp_print_styles', 'print_emoji_styles' ); - add_action( 'wp_enqueue_scripts', array( __CLASS__, 'override_wp_styles' ), -1 ); - /* * Add additional markup required by AMP . * Note that the meta[name=viewport] is not added here because a theme may want to define one with additional @@ -171,8 +159,8 @@ public static function register_hooks() { * in this case too we should defer to the theme as well to output the meta charset because it is possible the * install is not on utf-8 and we may need to do a encoding conversion. */ - add_action( 'wp_head', array( __CLASS__, 'add_amp_styles_placeholder' ), 8 ); // Because wp_print_styles() normally happens at 8. add_action( 'wp_head', array( __CLASS__, 'add_amp_component_scripts' ), 10 ); + add_action( 'wp_head', array( __CLASS__, 'print_amp_styles' ) ); add_action( 'wp_head', 'amp_add_generator_metadata', 20 ); add_action( 'wp_head', 'amp_print_schemaorg_metadata' ); @@ -313,21 +301,6 @@ public static function register_widgets() { } } - /** - * Override $wp_styles as AMP_WP_Styles, ideally before first instantiated as WP_Styles. - * - * @see wp_styles() - * @global AMP_WP_Styles $wp_styles - * @return AMP_WP_Styles Instance. - */ - public static function override_wp_styles() { - global $wp_styles; - if ( ! ( $wp_styles instanceof AMP_WP_Styles ) ) { - $wp_styles = new AMP_WP_Styles(); // WPCS: global override ok. - } - return $wp_styles; - } - /** * Register content embed handlers. * @@ -621,53 +594,11 @@ public static function filter_cancel_comment_reply_link( $formatted_link, $link, } /** - * Print placeholder for Custom AMP styles. - * - * The actual styles for the page injected into the placeholder when output buffering is completed. - * - * @see AMP_Theme_Support::finish_output_buffering() + * Print AMP boilerplate and custom styles. */ - public static function add_amp_styles_placeholder() { - echo self::STYLES_PLACEHOLDER; // WPCS: XSS OK. - - $wp_styles = wp_styles(); - if ( ! ( $wp_styles instanceof AMP_WP_Styles ) ) { - trigger_error( esc_html__( 'wp_styles() does not return an instance of AMP_WP_Styles as required.', 'amp' ), E_USER_WARNING ); // phpcs:ignore - return; - } - - $wp_styles->do_items(); // Normally done at wp_head priority 8. - $wp_styles->do_locale_stylesheet(); // Normally done at wp_head priority 10. - $wp_styles->do_custom_css(); // Normally done at wp_head priority 101. - } - - /** - * Get AMP boilerplate and custom styles. - * - * @param string[] $stylesheets Initial stylesheets. - * @see wp_custom_css_cb() - * @return string Concatenated stylesheets. - */ - public static function get_amp_styles( $stylesheets ) { - $css = wp_styles()->print_code; - - $css .= join( $stylesheets ); - - /** - * Filters AMP custom CSS before it is injected onto the output buffer for the response. - * - * Plugins may add their own styles, such as for rendered widgets, by amending them via this filter. - * - * @since 0.7 - * - * @param string $css AMP CSS. - */ - $css = apply_filters( 'amp_custom_styles', $css ); - - $css = wp_strip_all_tags( $css ); - - return amp_get_boilerplate_code() . "\n" - . ''; + public static function print_amp_styles() { + echo amp_get_boilerplate_code() . "\n"; // WPCS: XSS OK. + echo "\n"; // This will by populated by AMP_Style_Sanitizer. } /** @@ -880,14 +811,6 @@ public static function prepare_response( $response ) { 1 ); - // Inject styles. - $response = preg_replace( - '#' . preg_quote( self::STYLES_PLACEHOLDER, '#' ) . '#', - self::get_amp_styles( $assets['stylesheets'] ), - $response, - 1 - ); - return $response; } } diff --git a/includes/class-amp-wp-styles.php b/includes/class-amp-wp-styles.php deleted file mode 100644 index c397ce4c28b..00000000000 --- a/includes/class-amp-wp-styles.php +++ /dev/null @@ -1,251 +0,0 @@ -allowed_font_src_regex = '@^(' . $spec_rule[ AMP_Rule_Spec::ATTR_SPEC_LIST ]['href']['value_regex'] . ')$@'; - break; - } - } - } - - /** - * Generates an enqueued style's fully-qualified file path. - * - * @since 0.7 - * @see WP_Styles::_css_href() - * - * @param string $src The source URL of the enqueued style. - * @param string $handle The style's registered handle. - * @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error. - */ - public function get_validated_css_file_path( $src, $handle ) { - $needs_base_url = ( - ! is_bool( $src ) - && - ! preg_match( '|^(https?:)?//|', $src ) - && - ! ( $this->content_url && 0 === strpos( $src, $this->content_url ) ) - ); - if ( $needs_base_url ) { - $src = $this->base_url . $src; - } - - /** This filter is documented in wp-includes/class.wp-styles.php */ - $src = apply_filters( 'style_loader_src', $src, $handle ); - $src = esc_url_raw( $src ); - - // Strip query and fragment from URL. - $src = preg_replace( ':[\?#].*$:', '', $src ); - - if ( ! preg_match( '/\.(css|less|scss|sass)$/i', $src ) ) { - /* translators: %1$s is stylesheet handle, %2$s is stylesheet URL */ - return new WP_Error( 'amp_css_bad_file_extension', sprintf( __( 'Skipped stylesheet %1$s which does not have recognized CSS file extension (%2$s).', 'amp' ), $handle, $src ) ); - } - - $includes_url = includes_url( '/' ); - $content_url = content_url( '/' ); - $admin_url = get_admin_url( null, '/' ); - $css_path = null; - if ( 0 === strpos( $src, $content_url ) ) { - $css_path = WP_CONTENT_DIR . substr( $src, strlen( $content_url ) - 1 ); - } elseif ( 0 === strpos( $src, $includes_url ) ) { - $css_path = ABSPATH . WPINC . substr( $src, strlen( $includes_url ) - 1 ); - } elseif ( 0 === strpos( $src, $admin_url ) ) { - $css_path = ABSPATH . 'wp-admin' . substr( $src, strlen( $admin_url ) - 1 ); - } - - if ( ! $css_path || false !== strpos( '../', $css_path ) || 0 !== validate_file( $css_path ) || ! file_exists( $css_path ) ) { - /* translators: %1$s is stylesheet handle, %2$s is stylesheet URL */ - return new WP_Error( 'amp_css_path_not_found', sprintf( __( 'Unable to locate filesystem path for stylesheet %1$s (%2$s).', 'amp' ), $handle, $src ) ); - } - - return $css_path; - } - - /** - * Processes a style dependency. - * - * @since 0.7 - * @see WP_Styles::do_item() - * - * @param string $handle The style's registered handle. - * @return bool True on success, false on failure. - */ - public function do_item( $handle ) { - if ( ! WP_Dependencies::do_item( $handle ) ) { - return false; - } - $obj = $this->registered[ $handle ]; - - // Conditional styles and alternate styles aren't supported in AMP. - if ( isset( $obj->extra['conditional'] ) || isset( $obj->extra['alt'] ) ) { - return false; - } - - if ( isset( $obj->args ) ) { - $media = esc_attr( $obj->args ); - } else { - $media = 'all'; - } - - // A single item may alias a set of items, by having dependencies, but no source. - if ( ! $obj->src ) { - $inline_style = $this->print_inline_style( $handle, false ); - if ( $inline_style ) { - $this->print_code .= $inline_style; - } - return true; - } - - // Allow font URLs. - if ( $this->allowed_font_src_regex && preg_match( $this->allowed_font_src_regex, $obj->src ) ) { - $this->do_concat = false; - $result = parent::do_item( $handle ); - $this->do_concat = true; - return $result; - } - - $css_file_path = $this->get_validated_css_file_path( $obj->src, $handle ); - if ( is_wp_error( $css_file_path ) ) { - trigger_error( esc_html( $css_file_path->get_error_message() ), E_USER_WARNING ); // phpcs:ignore - return false; - } - $css_rtl_file_path = ''; - - // Handle RTL styles. - if ( 'rtl' === $this->text_direction && isset( $obj->extra['rtl'] ) && $obj->extra['rtl'] ) { - if ( is_bool( $obj->extra['rtl'] ) || 'replace' === $obj->extra['rtl'] ) { - $suffix = isset( $obj->extra['suffix'] ) ? $obj->extra['suffix'] : ''; - $css_rtl_file_path = $this->get_validated_css_file_path( - str_replace( "{$suffix}.css", "-rtl{$suffix}.css", $obj->src ), - "$handle-rtl" - ); - } else { - $css_rtl_file_path = $this->get_validated_css_file_path( $obj->extra['rtl'], "$handle-rtl" ); - } - - if ( is_wp_error( $css_rtl_file_path ) ) { - trigger_error( esc_html( $css_rtl_file_path->get_error_message() ), E_USER_WARNING ); // phpcs:ignore - $css_rtl_file_path = null; - } elseif ( 'replace' === $obj->extra['rtl'] ) { - $css_file_path = null; - } - } - - // Load the CSS from the filesystem. - foreach ( array_filter( array( $css_file_path, $css_rtl_file_path ) ) as $css_path ) { - $css = file_get_contents( $css_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. - if ( 'all' !== $media ) { - $css = sprintf( '@media %s { %s }', $media, $css ); - } - $this->print_code .= $css; - } - - // Add inline styles. - $inline_style = $this->print_inline_style( $handle, false ); - if ( $inline_style ) { - $this->print_code .= $inline_style; - } - - return true; - } - - /** - * Get the locale stylesheet if it exists. - * - * @since 0.7 - * @see locale_stylesheet() - * @return bool Whether locale stylesheet was done. - */ - public function do_locale_stylesheet() { - if ( $this->did_locale_stylesheet ) { - return true; - } - - $src = get_locale_stylesheet_uri(); - if ( ! $src ) { - return false; - } - $path = $this->get_validated_css_file_path( $src, get_stylesheet() . '-' . get_locale() ); - if ( is_wp_error( $path ) ) { - return false; - } - $this->print_code .= file_get_contents( $path ); // phpcs:ignore -- The path has been validated, and it is not a remote path. - $this->did_locale_stylesheet = true; - return true; - } - - /** - * Append Customizer Custom CSS. - * - * @since 0.7 - * @see wp_custom_css() - * @see wp_custom_css_cb() - * @return bool Whether locale Custom CSS was done. - */ - public function do_custom_css() { - if ( $this->did_custom_css ) { - return true; - } - - $css = trim( wp_get_custom_css() ); - if ( ! $css ) { - return false; - } - - $this->print_code .= $css; - - $this->did_custom_css = true; - return true; - } -} diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 6e926518b1e..eb222744071 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -8,7 +8,6 @@ /** * Class AMP_Style_Sanitizer * - * @todo This needs to also run on the CSS that is gathered for amp-custom. * Collects inline styles and outputs them in the amp-custom stylesheet. */ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { @@ -40,6 +39,36 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $keyframes_max_size; + /** + * The style[amp-custom] element. + * + * @var DOMElement + */ + private $amp_custom_style_element; + + /** + * Regex for allowed font stylesheet URL. + * + * @var string + */ + private $allowed_font_src_regex; + + /** + * Base URL for styles. + * + * Full URL with trailing slash. + * + * @var string + */ + private $base_url; + + /** + * URL of the content directory. + * + * @var string + */ + private $content_url; + /** * AMP_Base_Sanitizer constructor. * @@ -58,6 +87,21 @@ public function __construct( DOMDocument $dom, array $args = array() ) { break; } } + + $spec_name = 'link rel=stylesheet for fonts'; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'link' ) as $spec_rule ) { + if ( isset( $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) && $spec_name === $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) { + $this->allowed_font_src_regex = '@^(' . $spec_rule[ AMP_Rule_Spec::ATTR_SPEC_LIST ]['href']['value_regex'] . ')$@'; + break; + } + } + + $guessurl = site_url(); + if ( ! $guessurl ) { + $guessurl = wp_guess_url(); + } + $this->base_url = $guessurl; + $this->content_url = WP_CONTENT_URL; } /** @@ -81,7 +125,7 @@ public function get_styles() { * @returns array Values are the CSS stylesheets. Keys are MD5 hashes of the stylesheets. */ public function get_stylesheets() { - return array_merge( parent::get_stylesheets(), $this->stylesheets ); + return array_merge( $this->stylesheets, parent::get_stylesheets() ); } /** @@ -90,50 +134,192 @@ public function get_stylesheets() { * @since 0.4 */ public function sanitize() { - $body = $this->root_element; - $this->collect_style_elements(); + $elements = array(); + + /* + * Note that xpath is used to query the DOM so that the link and style elements will be + * in document order. DOMNode::compareDocumentPosition() is not yet implemented. + */ + $xpath = new DOMXPath( $this->dom ); + + $lower_case = 'translate( %s, "ABCDEFGHIJKLMNOPQRSTUVWXYZ", "abcdefghijklmnopqrstuvwxyz" )'; // In XPath 2.0 this is lower-case(). + $predicates = array( + sprintf( '( self::style and not( @amp-boilerplate ) and ( not( @type ) or %s = "text/css" ) )', sprintf( $lower_case, '@type' ) ), + sprintf( '( self::link and @href and %s = "stylesheet" )', sprintf( $lower_case, '@rel' ) ), + ); + + foreach ( $xpath->query( '//*[ ' . implode( ' or ', $predicates ) . ' ]' ) as $element ) { + $elements[] = $element; + } + + /** + * Element. + * + * @var DOMElement $element + */ + foreach ( $elements as $element ) { + $node_name = strtolower( $element->nodeName ); + if ( 'style' === $node_name ) { + $this->process_style_element( $element ); + } elseif ( 'link' === $node_name ) { + $this->process_link_element( $element ); + } + } - $this->collect_styles_recursive( $body ); + $elements = array(); + foreach ( $xpath->query( '//*[ @style ]' ) as $element ) { + $elements[] = $element; + } + foreach ( $elements as $element ) { + $this->collect_inline_styles( $element ); + } $this->did_convert_elements = true; + + // Now make sure the amp-custom style is in the DOM and populated, if we're working with the document element. + if ( ! empty( $this->args['use_document_element'] ) ) { + if ( ! $this->amp_custom_style_element ) { + $this->amp_custom_style_element = $this->dom->createElement( 'style' ); + $this->amp_custom_style_element->setAttribute( 'amp-custom', '' ); + $this->dom->getElementsByTagName( 'head' )->item( 0 )->appendChild( $this->amp_custom_style_element ); + } + + $css = implode( "\n", $this->get_stylesheets() ); + $this->amp_custom_style_element->textContent = $css; + } + } + + /** + * Generates an enqueued style's fully-qualified file path. + * + * @since 0.7 + * @see WP_Styles::_css_href() + * + * @param string $src The source URL of the enqueued style. + * @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error. + */ + public function get_validated_css_file_path( $src ) { + $needs_base_url = ( + ! is_bool( $src ) + && + ! preg_match( '|^(https?:)?//|', $src ) + && + ! ( $this->content_url && 0 === strpos( $src, $this->content_url ) ) + ); + if ( $needs_base_url ) { + $src = $this->base_url . $src; + } + + // Strip query and fragment from URL. + $src = preg_replace( ':[\?#].*$:', '', $src ); + + if ( ! preg_match( '/\.(css|less|scss|sass)$/i', $src ) ) { + /* translators: %s is stylesheet URL */ + return new WP_Error( 'amp_css_bad_file_extension', sprintf( __( 'Skipped stylesheet which does not have recognized CSS file extension (%s).', 'amp' ), $src ) ); + } + + $includes_url = includes_url( '/' ); + $content_url = content_url( '/' ); + $admin_url = get_admin_url( null, '/' ); + $css_path = null; + if ( 0 === strpos( $src, $content_url ) ) { + $css_path = WP_CONTENT_DIR . substr( $src, strlen( $content_url ) - 1 ); + } elseif ( 0 === strpos( $src, $includes_url ) ) { + $css_path = ABSPATH . WPINC . substr( $src, strlen( $includes_url ) - 1 ); + } elseif ( 0 === strpos( $src, $admin_url ) ) { + $css_path = ABSPATH . 'wp-admin' . substr( $src, strlen( $admin_url ) - 1 ); + } + + if ( ! $css_path || false !== strpos( '../', $css_path ) || 0 !== validate_file( $css_path ) || ! file_exists( $css_path ) ) { + /* translators: %s is stylesheet URL */ + return new WP_Error( 'amp_css_path_not_found', sprintf( __( 'Unable to locate filesystem path for stylesheet %s.', 'amp' ), $src ) ); + } + + return $css_path; } + /** - * Collect and sanitize all style elements. + * Process style element. + * + * @param DOMElement $element Style element. */ - public function collect_style_elements() { - $style_elements = $this->dom->getElementsByTagName( 'style' ); - $nodes_to_remove = array(); - - foreach ( $style_elements as $style_element ) { - /** - * Style element. - * - * @var DOMElement $style_element - */ - - if ( 'body' === $style_element->parentNode->nodeName && $style_element->hasAttribute( 'amp-keyframes' ) ) { - $validity = $this->validate_amp_keyframe( $style_element ); - if ( true === $validity ) { - continue; - } + private function process_style_element( DOMElement $element ) { + if ( 'body' === $element->parentNode->nodeName && $element->hasAttribute( 'amp-keyframes' ) ) { + $validity = $this->validate_amp_keyframe( $element ); + if ( true !== $validity ) { + $element->parentNode->removeChild( $element ); // @todo Add reporting. } + return; + } - $nodes_to_remove[] = $style_element; + $rules = trim( $element->textContent ); + $rules = $this->remove_illegal_css( $rules ); - // @todo This should perhaps be done in document order to ensure proper cascade. - $rules = trim( $style_element->textContent ); + $this->stylesheets[ md5( $rules ) ] = $rules; - // @todo This needs proper CSS parser, and de-duplication with \AMP_Style_Sanitizer::filter_style(). - $rules = preg_replace( '/\s*!important\s*(?=\s*;|})/', '', $rules ); - $rules = preg_replace( '/overflow\s*:\s*(auto|scroll)\s*;?\s*/', '', $rules ); + if ( $element->hasAttribute( 'amp-custom' ) ) { + if ( ! $this->amp_custom_style_element ) { + $this->amp_custom_style_element = $element; + } else { + $element->parentNode->removeChild( $element ); // There can only be one. #highlander. + } + } else { - $this->stylesheets[ md5( $rules ) ] = $rules; + // Remove from DOM since we'll be adding it to amp-custom. + $element->parentNode->removeChild( $element ); } + } + + /** + * Process link element. + * + * @param DOMElement $element Link element. + */ + private function process_link_element( DOMElement $element ) { - foreach ( $nodes_to_remove as $node_to_remove ) { - $node_to_remove->parentNode->removeChild( $node_to_remove ); + $href = $element->getAttribute( 'href' ); + + // Allow font URLs. + if ( $this->allowed_font_src_regex && preg_match( $this->allowed_font_src_regex, $href ) ) { + return; + } + + $css_file_path = $this->get_validated_css_file_path( $href ); + if ( is_wp_error( $css_file_path ) ) { + $element->parentNode->removeChild( $element ); // @todo Report removal. + return; } + + // Load the CSS from the filesystem. + $css = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. + + $css = $this->remove_illegal_css( $css ); + + $media = $element->getAttribute( 'media' ); + if ( $media && 'all' !== $media ) { + $css = sprintf( '@media %s { %s }', $media, $css ); + } + + $this->stylesheets[ md5( $css ) ] = $css; + + // Remove now that styles have been processed. + $element->parentNode->removeChild( $element ); + } + + /** + * Remove illegal CSS from the stylesheet. + * + * @since 0.7 + * + * @todo This needs proper CSS parser and to take an alternative approach to removing !important by extracting the rule into a separate style rule with a very specific selector. + * @param string $stylesheet Stylesheet. + * @return string Scrubbed stylesheet. + */ + private function remove_illegal_css( $stylesheet ) { + $stylesheet = preg_replace( '/\s*!important\s*(?=\s*;|})/', '', $stylesheet ); + $stylesheet = preg_replace( '/overflow\s*:\s*(auto|scroll)\s*;?\s*/', '', $stylesheet ); + return $stylesheet; } /** @@ -171,39 +357,28 @@ private function validate_amp_keyframe( $style ) { * @see Retrieve array of styles using $this->get_styles() after calling this method. * * @since 0.4 + * @since 0.7 Modified to use element passed by XPath query. * * @note Uses recursion to traverse down the tree of DOMDocument nodes. - * @todo This could use XPath to more efficiently find all elements with style attributes. * - * @param DOMNode $node Node. + * @param DOMElement $element Node. */ - private function collect_styles_recursive( $node ) { - if ( XML_ELEMENT_NODE !== $node->nodeType ) { + private function collect_inline_styles( $element ) { + $style = $element->getAttribute( 'style' ); + if ( ! $style ) { return; } + $class = $element->getAttribute( 'class' ); - if ( $node->hasAttributes() && $node instanceof DOMElement ) { - $style = $node->getAttribute( 'style' ); - $class = $node->getAttribute( 'class' ); - - if ( $style ) { - $style = $this->process_style( $style ); - if ( ! empty( $style ) ) { - $class_name = $this->generate_class_name( $style ); - $new_class = trim( $class . ' ' . $class_name ); - - $node->setAttribute( 'class', $new_class ); - $this->styles[ '.' . $class_name ] = $style; - } - $node->removeAttribute( 'style' ); - } - } + $style = $this->process_style( $style ); + if ( ! empty( $style ) ) { + $class_name = $this->generate_class_name( $style ); + $new_class = trim( $class . ' ' . $class_name ); - $length = $node->childNodes->length; - for ( $i = $length - 1; $i >= 0; $i -- ) { - $child_node = $node->childNodes->item( $i ); - $this->collect_styles_recursive( $child_node ); + $element->setAttribute( 'class', $new_class ); + $this->styles[ '.' . $class_name ] = $style; } + $element->removeAttribute( 'style' ); } /** diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 98d8e868f53..d55bc19b1c4 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -5,6 +5,8 @@ * @package AMP */ +// phpcs:disable WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned + /** * Test AMP_Style_Sanitizer. */ @@ -15,7 +17,7 @@ class AMP_Style_Sanitizer_Test extends WP_UnitTestCase { * * @return array */ - public function get_data() { + public function get_body_style_attribute_data() { return array( 'empty' => array( '', @@ -116,10 +118,41 @@ public function get_data() { 'div > span { font-weight:bold; font-style: italic; }', ), ), + ); + } + + /** + * Test sanitizer for style attributes that appear in the body. + * + * @dataProvider get_body_style_attribute_data + * @param string $source Source. + * @param string $expected_content Expected content. + * @param string $expected_stylesheets Expected stylesheets. + */ + public function test_body_style_attribute_sanitizer( $source, $expected_content, $expected_stylesheets ) { + $dom = AMP_DOM_Utils::get_dom_from_content( $source ); - 'styles_in_head_and_body_both_handled' => array( - '', - '', + $sanitizer = new AMP_Style_Sanitizer( $dom ); + $sanitizer->sanitize(); + + // Test content. + $content = AMP_DOM_Utils::get_content_from_dom( $dom ); + $content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content ); + $this->assertEquals( $expected_content, $content ); + + // Test stylesheet. + $this->assertEquals( $expected_stylesheets, array_values( $sanitizer->get_stylesheets() ) ); + } + + /** + * Get link and style test data. + * + * @return array + */ + public function get_link_and_style_test_data() { + return array( + 'multiple_amp_custom_andother_styles' => array( + '', array( 'b {color:red}', 'i {color:blue}', @@ -127,24 +160,29 @@ public function get_data() { 's {color:yellow}', ), ), + array( + sprintf( + '', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + includes_url( 'css/dashicons.css' ) + ), + array( + 'strong.before-dashicon', + '.dashicons-dashboard:before', + 'strong.after-dashicon', + 's {color:yellow}', + ), + ), ); } /** - * Test sanitizer. + * Test style elements and link elements. * - * @dataProvider get_data + * @dataProvider get_link_and_style_test_data * @param string $source Source. - * @param string $expected_content Expected content. - * @param string $expected_stylesheets Expected stylesheets. + * @param array $expected_stylesheets Expected stylesheets. */ - public function test_sanitizer( $source, $expected_content, $expected_stylesheets ) { - $html_doc_format = '%s'; - if ( false === strpos( $source, 'sanitize(); - $whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom ); - $whitelist_sanitizer->sanitize(); - - // Test content. - $content = AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); - $content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content ); - $this->assertEquals( $expected_content, $content ); - - // Test stylesheet. - $this->assertEquals( $expected_stylesheets, array_values( $sanitizer->get_stylesheets() ) ); + $actual_stylesheets = array_values( $sanitizer->get_stylesheets() ); + $this->assertCount( count( $expected_stylesheets ), $actual_stylesheets ); + foreach ( $expected_stylesheets as $i => $expected_stylesheet ) { + $this->assertContains( $expected_stylesheet, $actual_stylesheets[ $i ] ); + } } /** @@ -212,4 +245,124 @@ public function test_keyframe_sanitizer( $source, $expected = null ) { $content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content ); $this->assertEquals( $expected, $content ); } + + /** + * Get stylesheet URLs. + * + * @returns array Stylesheet URL data. + */ + public function get_stylesheet_urls() { + return array( + 'theme_stylesheet_without_host' => array( + '/wp-content/themes/twentyseventeen/style.css', + WP_CONTENT_DIR . '/themes/twentyseventeen/style.css', + ), + 'theme_stylesheet_with_host' => array( + WP_CONTENT_URL . '/themes/twentyseventeen/style.css', + WP_CONTENT_DIR . '/themes/twentyseventeen/style.css', + ), + 'dashicons_without_host' => array( + '/wp-includes/css/dashicons.css', + ABSPATH . WPINC . '/css/dashicons.css', + ), + 'dashicons_with_host' => array( + includes_url( 'css/dashicons.css' ), + ABSPATH . WPINC . '/css/dashicons.css', + ), + 'admin_without_host' => array( + '/wp-admin/css/common.css', + ABSPATH . 'wp-admin/css/common.css', + ), + 'admin_with_host' => array( + admin_url( 'css/common.css' ), + ABSPATH . 'wp-admin/css/common.css', + ), + 'amp_css_bad_file_extension' => array( + content_url( 'themes/twentyseventeen/index.php' ), + null, + 'amp_css_bad_file_extension', + ), + 'amp_css_path_not_found' => array( + content_url( 'themes/twentyseventeen/404.css' ), + null, + 'amp_css_path_not_found', + ), + ); + } + + /** + * Tests get_validated_css_file_path. + * + * @dataProvider get_stylesheet_urls + * @covers AMP_Style_Sanitizer::get_validated_css_file_path() + * @param string $source Source URL. + * @param string|null $expected Expected path or null if error. + * @param string $error_code Error code. Optional. + */ + public function test_get_validated_css_file_path( $source, $expected, $error_code = null ) { + $dom = AMP_DOM_Utils::get_dom( '' ); + + $sanitizer = new AMP_Style_Sanitizer( $dom ); + $actual = $sanitizer->get_validated_css_file_path( $source ); + if ( isset( $error_code ) ) { + $this->assertInstanceOf( 'WP_Error', $actual ); + $this->assertEquals( $error_code, $actual->get_error_code() ); + } else { + $this->assertEquals( $expected, $actual ); + } + } + + /** + * Get font url test data. + * + * @return array Data. + */ + public function get_font_urls() { + return array( + 'tangerine' => array( + 'https://fonts.googleapis.com/css?family=Tangerine', + true, + ), + 'typekit' => array( + 'https://use.typekit.net/abc.css', + true, + ), + 'fontscom' => array( + 'https://fast.fonts.net/abc.css', + true, + ), + 'fontawesome' => array( + 'https://maxcdn.bootstrapcdn.com/font-awesome/123/css/font-awesome.min.css', + true, + ), + 'fontbad' => array( + 'https://bad.example.com/font.css', + false, + ), + ); + } + + /** + * Tests that font URLs get validated. + * + * @dataProvider get_font_urls + * @param string $url Font URL. + * @param bool $pass Whether the font URL is ok. + */ + public function test_font_urls( $url, $pass ) { + $dom = AMP_DOM_Utils::get_dom( sprintf( '', $url ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + + $sanitizer = new AMP_Style_Sanitizer( $dom, array( + 'use_document_element' => true, + ) ); + $sanitizer->sanitize(); + + $link = $dom->getElementsByTagName( 'link' )->item( 0 ); + if ( $pass ) { + $this->assertInstanceOf( 'DOMElement', $link ); + $this->assertEquals( $url, $link->getAttribute( 'href' ) ); + } else { + $this->assertEmpty( $link ); + } + } } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 84dc66300be..3c6dbf11eee 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -203,25 +203,6 @@ public function test_purge_amp_query_vars() { // phpcs:enable WordPress.Security.NonceVerification.NoNonceVerification } - /** - * Test get_amp_styles(). - * - * @covers AMP_Theme_Support::get_amp_styles() - */ - public function test_get_amp_styles() { - $styles = AMP_Theme_Support::get_amp_styles( array() ); - $this->assertStringStartsWith( amp_get_boilerplate_code(), $styles ); - - $injected_css = 'b strong { color: red; }'; - add_filter( 'amp_custom_styles', function( $css ) use ( $injected_css ) { - return $css . $injected_css; - } ); - $styles = AMP_Theme_Support::get_amp_styles( array() ); - - $this->assertStringStartsWith( amp_get_boilerplate_code(), $styles ); - $this->assertContains( $injected_css, $styles ); - } - /** * Test get_amp_scripts(). * diff --git a/tests/test-class-amp-wp-styles.php b/tests/test-class-amp-wp-styles.php deleted file mode 100644 index 1882acf0c13..00000000000 --- a/tests/test-class-amp-wp-styles.php +++ /dev/null @@ -1,221 +0,0 @@ -assertInstanceOf( 'AMP_WP_Styles', wp_styles() ); - } - - /** - * Return bad URL. - * - * @return string Bad URL. - */ - public function return_bad_style_loader_src() { - return site_url( 'wp-config.php' ); - } - - /** - * Tests get_validated_css_file_path. - * - * @covers AMP_WP_Styles::get_validated_css_file_path() - */ - public function test_get_validated_css_file_path() { - $wp_styles = AMP_Theme_Support::override_wp_styles(); - - // Theme. - $expected = WP_CONTENT_DIR . '/themes/twentyseventeen/style.css'; - $path = $wp_styles->get_validated_css_file_path( '/wp-content/themes/twentyseventeen/style.css', 'twentyseventeen-style' ); - $this->assertEquals( $expected, $path ); - $path = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/style.css' ), 'dashicons' ); - $this->assertEquals( $expected, $path ); - - add_filter( 'style_loader_src', array( $this, 'return_bad_style_loader_src' ) ); - $r = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/style.css' ), 'dashicons' ); - $this->assertInstanceOf( 'WP_Error', $r ); - $this->assertEquals( 'amp_css_bad_file_extension', $r->get_error_code() ); - remove_filter( 'style_loader_src', array( $this, 'return_bad_style_loader_src' ) ); - - // Includes. - $expected = ABSPATH . WPINC . '/css/dashicons.css'; - $path = $wp_styles->get_validated_css_file_path( '/wp-includes/css/dashicons.css', 'dashicons' ); - $this->assertEquals( $expected, $path ); - $path = $wp_styles->get_validated_css_file_path( includes_url( 'css/dashicons.css' ), 'dashicons' ); - $this->assertEquals( $expected, $path ); - - // Admin. - $expected = ABSPATH . 'wp-admin/css/common.css'; - $path = $wp_styles->get_validated_css_file_path( '/wp-admin/css/common.css', 'dashicons' ); - $this->assertEquals( $expected, $path ); - $path = $wp_styles->get_validated_css_file_path( admin_url( 'css/common.css' ), 'common' ); - $this->assertEquals( $expected, $path ); - - // Bad URLs. - $r = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/index.php' ), 'bad' ); - $this->assertInstanceOf( 'WP_Error', $r ); - $this->assertEquals( 'amp_css_bad_file_extension', $r->get_error_code() ); - - $r = $wp_styles->get_validated_css_file_path( content_url( 'themes/twentyseventeen/404.css' ), 'bad' ); - $this->assertInstanceOf( 'WP_Error', $r ); - $this->assertEquals( 'amp_css_path_not_found', $r->get_error_code() ); - - $r = $wp_styles->get_validated_css_file_path( get_template_directory() . '/style.css', 'bad' ); - $this->assertInstanceOf( 'WP_Error', $r ); - $this->assertEquals( 'amp_css_path_not_found', $r->get_error_code() ); - } - - /** - * Tests test_do_item. - * - * @covers AMP_WP_Styles::do_item() - */ - public function test_do_item() { - $wp_styles = AMP_Theme_Support::override_wp_styles(); - $this->assertFalse( $wp_styles->do_item( 'non-existent' ) ); - - // Conditional stylesheets are ignored. - $wp_styles->registered['buttons-conditional'] = clone $wp_styles->registered['buttons']; - $wp_styles->registered['buttons-conditional']->extra['conditional'] = 'IE8'; - $this->assertFalse( $wp_styles->do_item( 'buttons-conditional' ) ); - - // Alt stylesheets are ignored. - $wp_styles->registered['buttons-alt'] = clone $wp_styles->registered['buttons']; - $wp_styles->registered['buttons-alt']->extra['alt'] = true; - $this->assertFalse( $wp_styles->do_item( 'buttons-alt' ) ); - - // Media. - $wp_styles->registered['admin-bar-print'] = clone $wp_styles->registered['admin-bar']; - $wp_styles->registered['admin-bar-print']->args = 'x_virtual_reality'; - $wp_styles->print_code = ''; - $this->assertTrue( $wp_styles->do_item( 'admin-bar-print' ) ); - $this->assertStringStartsWith( '@media x_virtual_reality {', $wp_styles->print_code ); - - // Inline style. - $wp_styles->print_code = ''; - $inline = '/* INLINE STYLE FOR BUTTONS */'; - wp_add_inline_style( 'buttons', $inline ); - $this->assertTrue( $wp_styles->do_item( 'buttons' ) ); - $wp_styles->do_item( 'buttons' ); - $this->assertStringEndsWith( $inline, $wp_styles->print_code ); - $this->assertContains( '.wp-core-ui .button-link', $wp_styles->print_code ); - - // Buttons bad. - $this->setExpectedException( 'PHPUnit_Framework_Error_Warning', 'Skipped stylesheet buttons-bad which does not have recognized CSS file extension (http://example.org/wp-config.php).' ); - $wp_styles->base_url = 'http://example.org'; - $wp_styles->print_code = ''; - $wp_styles->registered['buttons-bad'] = clone $wp_styles->registered['buttons']; - $wp_styles->registered['buttons-bad']->src = $this->return_bad_style_loader_src(); - $this->assertFalse( $wp_styles->do_item( 'buttons-bad' ) ); - $this->assertEmpty( $wp_styles->print_code ); - } - - /** - * Tests test_do_item for stylesheets. - * - * @covers AMP_WP_Styles::do_item() - */ - public function test_do_item_font_stylesheet() { - $wp_styles = AMP_Theme_Support::override_wp_styles(); - - $ok_styles = array( - 'tangerine' => 'https://fonts.googleapis.com/css?family=Tangerine', - 'typekit' => 'https://use.typekit.net/abc.css', - 'fontscom' => 'https://fast.fonts.net/abc.css', - 'fontawesome' => 'https://maxcdn.bootstrapcdn.com/font-awesome/123/css/font-awesome.min.css', - ); - foreach ( $ok_styles as $handle => $src ) { - $wp_styles->add( $handle, $src ); - ob_start(); - $this->assertTrue( $wp_styles->do_item( $handle ) ); - $output = ob_get_clean(); - $this->assertContains( 'assertContains( $src, $output ); - } - - $this->setExpectedException( 'PHPUnit_Framework_Error_Warning', 'Unable to locate filesystem path for stylesheet fontillegal (https://illegal.example.com/forbidden.css).' ); - $handle = 'fontillegal'; - $wp_styles->add( $handle, 'https://illegal.example.com/forbidden.css' ); - $this->assertFalse( $wp_styles->do_item( $handle ) ); - } - - /** - * Tests do_locale_stylesheet. - * - * @covers AMP_WP_Styles::do_locale_stylesheet() - */ - public function test_do_locale_stylesheet() { - $wp_styles = AMP_Theme_Support::override_wp_styles(); - add_filter( 'locale_stylesheet_uri', '__return_false' ); - $this->assertFalse( $wp_styles->do_locale_stylesheet() ); - $this->assertEmpty( $wp_styles->print_code ); - remove_filter( 'locale_stylesheet_uri', '__return_false' ); - - add_filter( 'locale_stylesheet_uri', array( $this, 'return_css_url' ) ); - $this->assertTrue( $wp_styles->do_locale_stylesheet() ); - $this->assertNotEmpty( $wp_styles->print_code ); - } - - /** - * Tests do_custom_css. - * - * @covers AMP_WP_Styles::do_custom_css() - */ - public function test_do_custom_css() { - $wp_styles = AMP_Theme_Support::override_wp_styles(); - $this->assertFalse( $wp_styles->do_custom_css() ); - $this->assertEmpty( $wp_styles->print_code ); - - add_filter( 'wp_get_custom_css', array( $this, 'return_css_rule' ) ); - $wp_styles = null; - $wp_styles = AMP_Theme_Support::override_wp_styles(); - $wp_styles->do_custom_css(); - $this->assertEquals( $this->return_css_rule(), $wp_styles->print_code ); - $wp_styles->do_custom_css(); - $this->assertEquals( $this->return_css_rule(), $wp_styles->print_code ); - remove_filter( 'wp_get_custom_css', array( $this, 'return_css_rule' ) ); - } - - /** - * Return sample CSS rule. - * - * @return string - */ - public function return_css_rule() { - return 'body { color:black; }'; - } - - /** - * Return URL to valid CSS file. - * - * @return string - */ - public function return_css_url() { - return includes_url( 'css/buttons.css' ); - } -} From 033c2bbdde76651701506e0f0fb2c1ec8a4e9940 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Feb 2018 00:17:41 -0800 Subject: [PATCH 2/7] Enforce that style sanitizer and whitelist sanitizer always run last --- includes/amp-helper-functions.php | 13 ++++++++++++- tests/test-amp-helper-functions.php | 10 ++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index a408add9e57..b06a2bbebbc 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -293,7 +293,7 @@ function amp_get_content_sanitizers( $post = null ) { * @param array $handlers Handlers. * @param WP_Post $post Post. Deprecated. */ - return apply_filters( 'amp_content_sanitizers', + $sanitizers = apply_filters( 'amp_content_sanitizers', array( 'AMP_Img_Sanitizer' => array(), 'AMP_Form_Sanitizer' => array(), @@ -309,6 +309,17 @@ function amp_get_content_sanitizers( $post = null ) { ), $post ); + + // Force style sanitizer and whitelist sanitizer to be at end. + foreach ( array( 'AMP_Style_Sanitizer', 'AMP_Tag_And_Attribute_Sanitizer' ) as $class_name ) { + if ( isset( $sanitizers[ $class_name ] ) ) { + $sanitizer = $sanitizers[ $class_name ]; + unset( $sanitizers[ $class_name ] ); + $sanitizers[ $class_name ] = $sanitizer; + } + } + + return $sanitizers; } /** diff --git a/tests/test-amp-helper-functions.php b/tests/test-amp-helper-functions.php index f7cde3d0e7a..d6846c33708 100644 --- a/tests/test-amp-helper-functions.php +++ b/tests/test-amp-helper-functions.php @@ -196,6 +196,16 @@ public function test_amp_get_content_sanitizers() { $this->assertEquals( 'amp_content_sanitizers', $this->last_filter_call['current_filter'] ); $this->assertEquals( $handlers, $this->last_filter_call['args'][0] ); $this->assertEquals( $post, $this->last_filter_call['args'][1] ); + + // Make sure the style and whitelist sanitizers are always at the end, even after filtering. + add_filter( 'amp_content_sanitizers', function( $classes ) { + $classes['Even_After_Whitelist_Sanitizer'] = array(); + return $classes; + } ); + $orderd_sanitizers = array_keys( amp_get_content_sanitizers() ); + $this->assertEquals( 'Even_After_Whitelist_Sanitizer', $orderd_sanitizers[ count( $orderd_sanitizers ) - 3 ] ); + $this->assertEquals( 'AMP_Style_Sanitizer', $orderd_sanitizers[ count( $orderd_sanitizers ) - 2 ] ); + $this->assertEquals( 'AMP_Tag_And_Attribute_Sanitizer', $orderd_sanitizers[ count( $orderd_sanitizers ) - 1 ] ); } /** From 2e2d22da0ac4e685aa98bd34c2484bce8aa14178 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Feb 2018 00:22:03 -0800 Subject: [PATCH 3/7] Include URL for concatenated external stylesheet as comment See https://github.com/Automattic/amp-wp/issues/927#issuecomment-363210765 --- includes/sanitizers/class-amp-style-sanitizer.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index eb222744071..512753ae926 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -292,7 +292,8 @@ private function process_link_element( DOMElement $element ) { } // Load the CSS from the filesystem. - $css = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. + $css = "\n/* $href */\n"; + $css .= file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. $css = $this->remove_illegal_css( $css ); From 97eaa6e8ca09f90288c9735abd3af19087d3e74d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Feb 2018 00:46:15 -0800 Subject: [PATCH 4/7] Stop concatenating CSS stylesheets to output if surpasses max size; show HTML comment --- .../sanitizers/class-amp-style-sanitizer.php | 44 +++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 512753ae926..734edf9c372 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -39,6 +39,14 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $keyframes_max_size; + /** + * Maximum number of bytes allowed for a AMP Custom style. + * + * @since 0.7 + * @var int + */ + private $custom_max_size; + /** * The style[amp-custom] element. * @@ -88,6 +96,14 @@ public function __construct( DOMDocument $dom, array $args = array() ) { } } + $spec_name = 'style amp-custom'; + foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'style' ) as $spec_rule ) { + if ( isset( $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) && $spec_name === $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) { + $this->custom_max_size = $spec_rule[ AMP_Rule_Spec::CDATA ]['max_bytes']; + break; + } + } + $spec_name = 'link rel=stylesheet for fonts'; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'link' ) as $spec_rule ) { if ( isset( $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) && $spec_name === $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) { @@ -184,8 +200,30 @@ public function sanitize() { $this->dom->getElementsByTagName( 'head' )->item( 0 )->appendChild( $this->amp_custom_style_element ); } - $css = implode( "\n", $this->get_stylesheets() ); + // Gather stylesheets to print as long as they don't surpass the limit. + $skipped = array(); + $css = ''; + $total_size = 0; + foreach ( $this->get_stylesheets() as $key => $stylesheet ) { + $sheet_size = strlen( $stylesheet ); + if ( $total_size + $sheet_size > $this->custom_max_size ) { + $skipped[] = $key; + } else { + $css .= $stylesheet; + $total_size += $sheet_size; + } + } + $this->amp_custom_style_element->textContent = $css; + + // @todo This would be a candidate for sanitization reporting. + // Add comments to indicate which sheets were not included. + foreach ( array_reverse( $skipped ) as $skip ) { + $this->amp_custom_style_element->parentNode->insertBefore( + $this->dom->createComment( sprintf( 'Skipped including %s stylesheet since too large.', $skip ) ), + $this->amp_custom_style_element->nextSibling + ); + } } } @@ -287,7 +325,7 @@ private function process_link_element( DOMElement $element ) { $css_file_path = $this->get_validated_css_file_path( $href ); if ( is_wp_error( $css_file_path ) ) { - $element->parentNode->removeChild( $element ); // @todo Report removal. + $element->parentNode->removeChild( $element ); // @todo Report removal. Show HTML comment? return; } @@ -302,7 +340,7 @@ private function process_link_element( DOMElement $element ) { $css = sprintf( '@media %s { %s }', $media, $css ); } - $this->stylesheets[ md5( $css ) ] = $css; + $this->stylesheets[ $href ] = $css; // Remove now that styles have been processed. $element->parentNode->removeChild( $element ); From e472a69bde4ceadd9141ac0dd9712f3ced684e35 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Feb 2018 00:55:19 -0800 Subject: [PATCH 5/7] Make sure that !important does not appear _anywhere_ in CSS, including comments, as otherwise invalid --- includes/sanitizers/class-amp-style-sanitizer.php | 2 +- tests/test-amp-style-sanitizer.php | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 734edf9c372..b55740602d3 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -356,7 +356,7 @@ private function process_link_element( DOMElement $element ) { * @return string Scrubbed stylesheet. */ private function remove_illegal_css( $stylesheet ) { - $stylesheet = preg_replace( '/\s*!important\s*(?=\s*;|})/', '', $stylesheet ); + $stylesheet = preg_replace( '/\s*!important/', '', $stylesheet ); // Note this has to also replace inside comments to be valid. $stylesheet = preg_replace( '/overflow\s*:\s*(auto|scroll)\s*;?\s*/', '', $stylesheet ); return $stylesheet; } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index d55bc19b1c4..a06e0dc7e3d 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -152,7 +152,7 @@ public function test_body_style_attribute_sanitizer( $source, $expected_content, public function get_link_and_style_test_data() { return array( 'multiple_amp_custom_andother_styles' => array( - '', + '', array( 'b {color:red}', 'i {color:blue}', @@ -190,10 +190,17 @@ public function test_link_and_style_elements( $source, $expected_stylesheets ) { ) ); $sanitizer->sanitize(); + $whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, array( + 'use_document_element' => true, + ) ); + $whitelist_sanitizer->sanitize(); + + $sanitized_html = AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); $actual_stylesheets = array_values( $sanitizer->get_stylesheets() ); $this->assertCount( count( $expected_stylesheets ), $actual_stylesheets ); foreach ( $expected_stylesheets as $i => $expected_stylesheet ) { $this->assertContains( $expected_stylesheet, $actual_stylesheets[ $i ] ); + $this->assertContains( $expected_stylesheet, $sanitized_html ); } } From 0a15e52d02d2f293276066744b63179f3242b018 Mon Sep 17 00:00:00 2001 From: ThierryA Date: Tue, 6 Feb 2018 18:16:45 +0100 Subject: [PATCH 6/7] Code formatting --- includes/sanitizers/class-amp-style-sanitizer.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index b55740602d3..4b4d0149b88 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -150,7 +150,6 @@ public function get_stylesheets() { * @since 0.4 */ public function sanitize() { - $elements = array(); /* @@ -276,7 +275,6 @@ public function get_validated_css_file_path( $src ) { return $css_path; } - /** * Process style element. * @@ -315,7 +313,6 @@ private function process_style_element( DOMElement $element ) { * @param DOMElement $element Link element. */ private function process_link_element( DOMElement $element ) { - $href = $element->getAttribute( 'href' ); // Allow font URLs. @@ -351,7 +348,8 @@ private function process_link_element( DOMElement $element ) { * * @since 0.7 * - * @todo This needs proper CSS parser and to take an alternative approach to removing !important by extracting the rule into a separate style rule with a very specific selector. + * @todo This needs proper CSS parser and to take an alternative approach to removing !important by extracting + * the rule into a separate style rule with a very specific selector. * @param string $stylesheet Stylesheet. * @return string Scrubbed stylesheet. */ From 4f61eef0c21f93f663b51d9f91ae8149cdb7b1dc Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 6 Feb 2018 10:41:28 -0800 Subject: [PATCH 7/7] Fix libxml issues with parsing noscript elements and modifying style element textContent --- .../sanitizers/class-amp-style-sanitizer.php | 14 +++++- includes/utils/class-amp-dom-utils.php | 48 ++++++++++++++++--- tests/test-amp-style-sanitizer.php | 4 +- tests/test-class-amp-theme-support.php | 4 +- 4 files changed, 59 insertions(+), 11 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 4b4d0149b88..f643a1d7f0b 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -208,12 +208,24 @@ public function sanitize() { if ( $total_size + $sheet_size > $this->custom_max_size ) { $skipped[] = $key; } else { + if ( $total_size ) { + $css .= ' '; + } $css .= $stylesheet; $total_size += $sheet_size; } } - $this->amp_custom_style_element->textContent = $css; + /* + * Let the style[amp-custom] be populated with the concatenated CSS. + * !important: Updating the contents of this style element by setting textContent is not + * reliable across PHP/libxml versions, so this is why the children are removed and the + * text node is then explicitly added containing the CSS. + */ + while ( $this->amp_custom_style_element->firstChild ) { + $this->amp_custom_style_element->removeChild( $this->amp_custom_style_element->firstChild ); + } + $this->amp_custom_style_element->appendChild( $this->dom->createTextNode( $css ) ); // @todo This would be a candidate for sanitization reporting. // Add comments to indicate which sheets were not included. diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php index d60aa6da72a..5288c044f2f 100644 --- a/includes/utils/class-amp-dom-utils.php +++ b/includes/utils/class-amp-dom-utils.php @@ -42,13 +42,21 @@ class AMP_DOM_Utils { 'wbr', ); + /** + * Stored noscript/comment replacements for libxml<2.8. + * + * @since 0.7 + * @var array + */ + public static $noscript_placeholder_comments = array(); + /** * Return a valid DOMDocument representing HTML document passed as a parameter. * * @since 0.7 + * @see AMP_DOM_Utils::get_content_from_dom_node() * * @param string $document Valid HTML document to be represented by a DOMDocument. - * * @return DOMDocument|false Returns DOMDocument, or false if conversion failed. */ public static function get_dom( $document ) { @@ -82,6 +90,24 @@ public static function get_dom( $document ) { $document ); + /* + * Replace noscript elements with placeholders since libxml<2.8 can parse them incorrectly. + * When appearing in the head element, a noscript can cause the head to close prematurely + * and the noscript gets moved to the body and anything after it which was in the head. + * See . + */ + if ( version_compare( LIBXML_DOTTED_VERSION, '2.8', '<' ) ) { + $document = preg_replace_callback( + '#]*>.*?#si', + function( $matches ) { + $placeholder = sprintf( '', (string) wp_rand() ); + AMP_DOM_Utils::$noscript_placeholder_comments[ $placeholder ] = $matches[0]; + return $placeholder; + }, + $document + ); + } + /* * Wrap in dummy tags, since XML needs one parent node. * It also makes it easier to loop through nodes. @@ -266,15 +292,14 @@ public static function get_dom_from_content( $content ) { } /** - * Return valid HTML content extracted from the DOMDocument passed as a parameter. - * - * @see Reciprocal function get_dom_from_content() + * Return valid HTML *body* content extracted from the DOMDocument passed as a parameter. * * @since 0.2 + * @see AMP_DOM_Utils::get_content_from_dom_node() Reciprocal function. * * @param DOMDocument $dom Represents an HTML document from which to extract HTML content. * - * @return string Returns the HTML content represented in the DOMDocument + * @return string Returns the HTML content of the body element represented in the DOMDocument. */ public static function get_content_from_dom( $dom ) { @@ -305,9 +330,9 @@ public static function get_content_from_dom( $dom ) { /** * Return valid HTML content extracted from the DOMNode passed as a parameter. * - * @see Called by function get_content_from_dom() - * * @since 0.6 + * @see AMP_DOM_Utils::get_dom() Where the operations in this method are mirrored. + * @see AMP_DOM_Utils::get_content_from_dom() Reciprocal function. * @todo In the future consider an AMP_DOMDocument subclass that does this automatically at saveHTML(). See . * * @param DOMDocument $dom Represents an HTML document. @@ -338,6 +363,15 @@ public static function get_content_from_dom_node( $dom, $node ) { return ''; } + // Restore noscript elements which were temporarily removed to prevent libxml<2.8 parsing problems. + if ( version_compare( LIBXML_DOTTED_VERSION, '2.8', '<' ) ) { + $html = str_replace( + array_keys( self::$noscript_placeholder_comments ), + array_values( self::$noscript_placeholder_comments ), + $html + ); + } + $html = self::restore_amp_bind_attributes( $html ); // Restore amp-mustache placeholders which were replaced to prevent URL-encoded corruption by saveHTML. diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index a06e0dc7e3d..9280a2117b4 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -151,7 +151,7 @@ public function test_body_style_attribute_sanitizer( $source, $expected_content, */ public function get_link_and_style_test_data() { return array( - 'multiple_amp_custom_andother_styles' => array( + 'multiple_amp_custom_and_other_styles' => array( '', array( 'b {color:red}', @@ -160,7 +160,7 @@ public function get_link_and_style_test_data() { 's {color:yellow}', ), ), - array( + 'style_eleemnts_with_link_elements' => array( sprintf( '', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet includes_url( 'css/dashicons.css' ) diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 3c6dbf11eee..3429ab836cb 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -113,6 +113,8 @@ public function test_prepare_response() { data-aax_pubname="test123" data-aax_src="302"> + + assertContains( '', $sanitized_html ); $this->assertContains( '', $sanitized_html ); $this->assertContains( '