From 48a538ba0f03642630165d9f2e313406090dd11f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 26 Mar 2018 10:49:55 -0700 Subject: [PATCH 01/31] Add sabberworm/php-css-parser composer dependency --- composer.json | 3 +++ composer.lock | 71 +++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 61 insertions(+), 13 deletions(-) diff --git a/composer.json b/composer.json index 33409a6e7de..09eda2e54a9 100644 --- a/composer.json +++ b/composer.json @@ -5,6 +5,9 @@ "type": "wordpress-plugin", "license": "GPL-2.0", "version": "1.0.0", + "require": { + "sabberworm/php-css-parser": "*" + }, "require-dev": { "wp-coding-standards/wpcs": "^0.14.0", "dealerdirect/phpcodesniffer-composer-installer": "^0.4.4", diff --git a/composer.lock b/composer.lock index de486f97bc5..bc268227ed9 100644 --- a/composer.lock +++ b/composer.lock @@ -4,8 +4,53 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file", "This file is @generated automatically" ], - "content-hash": "984ff0ea17c1c8dff71e6e2e2e9dc8a0", - "packages": [], + "content-hash": "4859d4cee2cb9fec9dc6e86f80459837", + "packages": [ + { + "name": "sabberworm/php-css-parser", + "version": "8.1.0", + "source": { + "type": "git", + "url": "https://github.com/sabberworm/PHP-CSS-Parser.git", + "reference": "850cbbcbe7fbb155387a151ea562897a67e242ef" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/sabberworm/PHP-CSS-Parser/zipball/850cbbcbe7fbb155387a151ea562897a67e242ef", + "reference": "850cbbcbe7fbb155387a151ea562897a67e242ef", + "shasum": "" + }, + "require": { + "php": ">=5.3.2" + }, + "require-dev": { + "phpunit/phpunit": "*" + }, + "type": "library", + "autoload": { + "psr-0": { + "Sabberworm\\CSS": "lib/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Raphael Schweikert" + } + ], + "description": "Parser for CSS Files written in PHP", + "homepage": "http://www.sabberworm.com/blog/2010/6/10/php-css-parser", + "keywords": [ + "css", + "parser", + "stylesheet" + ], + "time": "2016-07-19T19:14:21+00:00" + } + ], "packages-dev": [ { "name": "dealerdirect/phpcodesniffer-composer-installer", @@ -77,16 +122,16 @@ }, { "name": "squizlabs/php_codesniffer", - "version": "3.2.2", + "version": "3.2.3", "source": { "type": "git", "url": "https://github.com/squizlabs/PHP_CodeSniffer.git", - "reference": "d7c00c3000ac0ce79c96fcbfef86b49a71158cd1" + "reference": "4842476c434e375f9d3182ff7b89059583aa8b27" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/squizlabs/PHP_CodeSniffer/zipball/d7c00c3000ac0ce79c96fcbfef86b49a71158cd1", - "reference": "d7c00c3000ac0ce79c96fcbfef86b49a71158cd1", + "url": "https://api.github.com/repos/squizlabs/PHP_CodeSniffer/zipball/4842476c434e375f9d3182ff7b89059583aa8b27", + "reference": "4842476c434e375f9d3182ff7b89059583aa8b27", "shasum": "" }, "require": { @@ -96,7 +141,7 @@ "php": ">=5.4.0" }, "require-dev": { - "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0" + "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0" }, "bin": [ "bin/phpcs", @@ -124,7 +169,7 @@ "phpcs", "standards" ], - "time": "2017-12-19T21:44:46+00:00" + "time": "2018-02-20T21:35:23+00:00" }, { "name": "wimg/php-compatibility", @@ -180,16 +225,16 @@ }, { "name": "wp-coding-standards/wpcs", - "version": "0.14.0", + "version": "0.14.1", "source": { "type": "git", "url": "https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards.git", - "reference": "8cadf48fa1c70b2381988e0a79e029e011a8f41c" + "reference": "cf6b310caad735816caef7573295f8a534374706" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/WordPress-Coding-Standards/WordPress-Coding-Standards/zipball/8cadf48fa1c70b2381988e0a79e029e011a8f41c", - "reference": "8cadf48fa1c70b2381988e0a79e029e011a8f41c", + "url": "https://api.github.com/repos/WordPress-Coding-Standards/WordPress-Coding-Standards/zipball/cf6b310caad735816caef7573295f8a534374706", + "reference": "cf6b310caad735816caef7573295f8a534374706", "shasum": "" }, "require": { @@ -216,7 +261,7 @@ "standards", "wordpress" ], - "time": "2017-11-01T15:10:46+00:00" + "time": "2018-02-16T01:57:48+00:00" } ], "aliases": [], From 0e994a90ffed30844ed82cb077d0c3c942767ee7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 26 Mar 2018 22:34:36 -0700 Subject: [PATCH 02/31] Add initial Server Timing response headers See #990 --- includes/class-amp-autoloader.php | 1 + includes/class-amp-response-headers.php | 88 +++++++++++++++++++ includes/class-amp-theme-support.php | 66 ++++---------- .../templates/class-amp-content-sanitizer.php | 3 + tests/test-class-amp-theme-support.php | 34 +++---- 5 files changed, 128 insertions(+), 64 deletions(-) create mode 100644 includes/class-amp-response-headers.php diff --git a/includes/class-amp-autoloader.php b/includes/class-amp-autoloader.php index b98afe871be..dbc01f6feea 100644 --- a/includes/class-amp-autoloader.php +++ b/includes/class-amp-autoloader.php @@ -30,6 +30,7 @@ class AMP_Autoloader { */ private static $_classmap = array( 'AMP_Theme_Support' => 'includes/class-amp-theme-support', + 'AMP_Response_Headers' => 'includes/class-amp-response-headers', 'AMP_Comment_Walker' => 'includes/class-amp-comment-walker', 'AMP_Template_Customizer' => 'includes/admin/class-amp-customizer', 'AMP_Post_Meta_Box' => 'includes/admin/class-amp-post-meta-box', diff --git a/includes/class-amp-response-headers.php b/includes/class-amp-response-headers.php new file mode 100644 index 00000000000..d8a5e62d635 --- /dev/null +++ b/includes/class-amp-response-headers.php @@ -0,0 +1,88 @@ + true, + 'status_code' => null, + ), + $args + ); + + self::$headers_sent[] = array_merge( compact( 'name', 'value' ), $args ); + if ( headers_sent() ) { + return false; + } + + header( + sprintf( '%s: %s', $name, $value ), + $args['replace'], + $args['status_code'] + ); + return true; + } + + /** + * Send Server-Timing header. + * + * @since 1.0 + * @todo What is the ordering in Chrome dev tools? What are the colors about? + * @todo Is there a better name standardization? + * @todo Is there a way to indicate nested server timings, so an outer method's own time can be seen separately from the inner method's time? + * + * @param string $name Name. + * @param float $duration Duration. If negative, will be added to microtime( true ). Optional. + * @param string $description Description. Optional. + * @return bool Return value of send_header call. + */ + public static function send_server_timing( $name, $duration = null, $description = null ) { + $value = $name; + if ( isset( $description ) ) { + $value .= sprintf( ';desc=%s', wp_json_encode( $description ) ); + } + if ( isset( $duration ) ) { + if ( $duration < 0 ) { + $duration = microtime( true ) + $duration; + } + $value .= sprintf( ';dur=%f', $duration * 1000 ); + } + return self::send_header( 'Server-Timing', $value, array( 'replace' => false ) ); + } +} diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 6373d2e6764..59d08e5a6d4 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -68,22 +68,25 @@ class AMP_Theme_Support { public static $purged_amp_query_vars = array(); /** - * Headers sent (or attempted to be sent). + * Start time when init was called. * - * @since 0.7 - * @see AMP_Theme_Support::send_header() - * @var array[] + * @since 1.0 + * @var float */ - public static $headers_sent = array(); + public static $init_start_time; /** * Initialize. + * + * @since 0.7 */ public static function init() { if ( ! current_theme_supports( 'amp' ) ) { return; } + self::$init_start_time = microtime( true ); + self::purge_amp_query_vars(); self::handle_xhr_request(); self::add_temporary_discussion_restrictions(); @@ -318,45 +321,6 @@ public static function purge_amp_query_vars() { } } - /** - * Send an HTTP response header. - * - * This largely exists to facilitate unit testing but it also provides a better interface for sending headers. - * - * @since 0.7.0 - * - * @param string $name Header name. - * @param string $value Header value. - * @param array $args { - * Args to header(). - * - * @type bool $replace Whether to replace a header previously sent. Default true. - * @type int $status_code Status code to send with the sent header. - * } - * @return bool Whether the header was sent. - */ - public static function send_header( $name, $value, $args = array() ) { - $args = array_merge( - array( - 'replace' => true, - 'status_code' => null, - ), - $args - ); - - self::$headers_sent[] = array_merge( compact( 'name', 'value' ), $args ); - if ( headers_sent() ) { - return false; - } - - header( - sprintf( '%s: %s', $name, $value ), - $args['replace'], - $args['status_code'] - ); - return true; - } - /** * Hook into a POST form submissions, such as the comment form or some other form submission. * @@ -377,7 +341,7 @@ public static function handle_xhr_request() { // Send AMP response header. $origin = wp_validate_redirect( wp_sanitize_redirect( esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] ) ) ); if ( $origin ) { - self::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); + AMP_Response_Headers::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); } // Intercept POST requests which redirect. @@ -526,8 +490,8 @@ public static function intercept_post_request_redirect( $location ) { $absolute_location .= '#' . $parsed_location['fragment']; } - self::send_header( 'AMP-Redirect-To', $absolute_location ); - self::send_header( 'Access-Control-Expose-Headers', 'AMP-Redirect-To' ); + AMP_Response_Headers::send_header( 'AMP-Redirect-To', $absolute_location ); + AMP_Response_Headers::send_header( 'Access-Control-Expose-Headers', 'AMP-Redirect-To' ); wp_send_json_success(); } @@ -1000,6 +964,7 @@ public static function start_output_buffering() { * @see AMP_Theme_Support::start_output_buffering() */ public static function finish_output_buffering() { + AMP_Response_Headers::send_server_timing( 'amp_output_buffer', -self::$init_start_time, 'AMP Output Buffer' ); echo self::prepare_response( ob_get_clean() ); // WPCS: xss ok. } @@ -1067,6 +1032,8 @@ public static function prepare_response( $response, $args = array() ) { $args ); + $dom_parse_start = microtime( true ); + /* * Make sure that is present in output prior to parsing. * Note that the meta charset is supposed to appear within the first 1024 bytes. @@ -1098,8 +1065,11 @@ public static function prepare_response( $response, $args = array() ) { $dom->documentElement->setAttribute( 'amp', '' ); } + AMP_Response_Headers::send_server_timing( 'amp_dom_parse', -$dom_parse_start, 'AMP DOM Parse' ); + $assets = AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args ); + $dom_serialize_start = microtime( true ); self::ensure_required_markup( $dom ); // @todo If 'utf-8' is not the blog charset, then we'll need to do some character encoding conversation or "entityification". @@ -1145,6 +1115,8 @@ public static function prepare_response( $response, $args = array() ) { ); } + AMP_Response_Headers::send_server_timing( 'amp_dom_serialize', -$dom_serialize_start, 'AMP DOM Serialize' ); + return $response; } diff --git a/includes/templates/class-amp-content-sanitizer.php b/includes/templates/class-amp-content-sanitizer.php index d7b75c28ec3..7c24ee563e0 100644 --- a/includes/templates/class-amp-content-sanitizer.php +++ b/includes/templates/class-amp-content-sanitizer.php @@ -63,6 +63,7 @@ public static function sanitize_document( &$dom, $sanitizer_classes, $args ) { $return_styles = ! empty( $args['return_styles'] ); unset( $args['return_styles'] ); foreach ( $sanitizer_classes as $sanitizer_class => $sanitizer_args ) { + $sanitize_class_start = microtime( true ); if ( ! class_exists( $sanitizer_class ) ) { /* translators: %s is sanitizer class */ _doing_it_wrong( __METHOD__, sprintf( esc_html__( 'Sanitizer (%s) class does not exist', 'amp' ), esc_html( $sanitizer_class ) ), '0.4.1' ); @@ -90,6 +91,8 @@ public static function sanitize_document( &$dom, $sanitizer_classes, $args ) { } else { $stylesheets = array_merge( $stylesheets, $sanitizer->get_stylesheets() ); } + + AMP_Response_Headers::send_server_timing( 'amp_sanitize', -$sanitize_class_start, $sanitizer_class ); } return compact( 'scripts', 'styles', 'stylesheets' ); diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 439807a584d..a4cb1762865 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -27,7 +27,7 @@ public function tearDown() { $_SERVER['QUERY_STRING'] = ''; unset( $_SERVER['REQUEST_URI'] ); unset( $_SERVER['REQUEST_METHOD'] ); - AMP_Theme_Support::$headers_sent = array(); + AMP_Response_Headers::$headers_sent = array(); } /** @@ -349,7 +349,7 @@ public function test_purge_amp_query_vars() { public function test_handle_xhr_request() { AMP_Theme_Support::purge_amp_query_vars(); AMP_Theme_Support::handle_xhr_request(); - $this->assertEmpty( AMP_Theme_Support::$headers_sent ); + $this->assertEmpty( AMP_Response_Headers::$headers_sent ); $_GET['_wp_amp_action_xhr_converted'] = '1'; @@ -358,14 +358,14 @@ public function test_handle_xhr_request() { $_SERVER['REQUEST_METHOD'] = 'POST'; AMP_Theme_Support::purge_amp_query_vars(); AMP_Theme_Support::handle_xhr_request(); - $this->assertEmpty( AMP_Theme_Support::$headers_sent ); + $this->assertEmpty( AMP_Response_Headers::$headers_sent ); // Try home source origin. $_GET['__amp_source_origin'] = home_url(); $_SERVER['REQUEST_METHOD'] = 'POST'; AMP_Theme_Support::purge_amp_query_vars(); AMP_Theme_Support::handle_xhr_request(); - $this->assertCount( 1, AMP_Theme_Support::$headers_sent ); + $this->assertCount( 1, AMP_Response_Headers::$headers_sent ); $this->assertEquals( array( 'name' => 'AMP-Access-Control-Allow-Source-Origin', @@ -373,7 +373,7 @@ public function test_handle_xhr_request() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent[0] + AMP_Response_Headers::$headers_sent[0] ); $this->assertEquals( PHP_INT_MAX, has_filter( 'wp_redirect', array( 'AMP_Theme_Support', 'intercept_post_request_redirect' ) ) ); $this->assertEquals( PHP_INT_MAX, has_filter( 'comment_post_redirect', array( 'AMP_Theme_Support', 'filter_comment_post_redirect' ) ) ); @@ -476,7 +476,7 @@ public function test_intercept_post_request_redirect() { } ); // Test redirecting to full URL with HTTPS protocol. - AMP_Theme_Support::$headers_sent = array(); + AMP_Response_Headers::$headers_sent = array(); ob_start(); AMP_Theme_Support::intercept_post_request_redirect( $url ); $this->assertEquals( '{"success":true}', ob_get_clean() ); @@ -487,7 +487,7 @@ public function test_intercept_post_request_redirect() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent + AMP_Response_Headers::$headers_sent ); $this->assertContains( array( @@ -496,11 +496,11 @@ public function test_intercept_post_request_redirect() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent + AMP_Response_Headers::$headers_sent ); // Test redirecting to non-HTTPS URL. - AMP_Theme_Support::$headers_sent = array(); + AMP_Response_Headers::$headers_sent = array(); ob_start(); $url = home_url( '/', 'http' ); AMP_Theme_Support::intercept_post_request_redirect( $url ); @@ -512,7 +512,7 @@ public function test_intercept_post_request_redirect() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent + AMP_Response_Headers::$headers_sent ); $this->assertContains( array( @@ -521,11 +521,11 @@ public function test_intercept_post_request_redirect() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent + AMP_Response_Headers::$headers_sent ); // Test redirecting to host-less location. - AMP_Theme_Support::$headers_sent = array(); + AMP_Response_Headers::$headers_sent = array(); ob_start(); AMP_Theme_Support::intercept_post_request_redirect( '/new-location/' ); $this->assertEquals( '{"success":true}', ob_get_clean() ); @@ -536,11 +536,11 @@ public function test_intercept_post_request_redirect() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent + AMP_Response_Headers::$headers_sent ); // Test redirecting to scheme-less location. - AMP_Theme_Support::$headers_sent = array(); + AMP_Response_Headers::$headers_sent = array(); ob_start(); $url = home_url( '/new-location/' ); AMP_Theme_Support::intercept_post_request_redirect( substr( $url, strpos( $url, ':' ) + 1 ) ); @@ -552,11 +552,11 @@ public function test_intercept_post_request_redirect() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent + AMP_Response_Headers::$headers_sent ); // Test redirecting to empty location. - AMP_Theme_Support::$headers_sent = array(); + AMP_Response_Headers::$headers_sent = array(); ob_start(); AMP_Theme_Support::intercept_post_request_redirect( '' ); $this->assertEquals( '{"success":true}', ob_get_clean() ); @@ -567,7 +567,7 @@ public function test_intercept_post_request_redirect() { 'replace' => true, 'status_code' => null, ), - AMP_Theme_Support::$headers_sent + AMP_Response_Headers::$headers_sent ); } From 8271fac4d9a0dd098a7db2cd26c4fa9ae60cdb90 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 29 Mar 2018 21:26:16 -0700 Subject: [PATCH 03/31] Use sabberworm/php-css-parser to sanitize CSS * Compress CSS output with compact format, removing whitespace and comments. * Reduce length of generated class names for inline styles. * Unify logic for handling inline style attributes with handling stylesheets in style/link. --- includes/class-amp-autoloader.php | 4 + .../sanitizers/class-amp-style-sanitizer.php | 332 ++++++++++-------- tests/test-amp-style-sanitizer.php | 54 +-- tests/test-class-amp-theme-support.php | 2 +- 4 files changed, 218 insertions(+), 174 deletions(-) diff --git a/includes/class-amp-autoloader.php b/includes/class-amp-autoloader.php index dbc01f6feea..c86307f0a77 100644 --- a/includes/class-amp-autoloader.php +++ b/includes/class-amp-autoloader.php @@ -131,6 +131,10 @@ protected static function autoload( $class_name ) { * Called at the end of this file; calling a second time has no effect. */ public static function register() { + if ( file_exists( AMP__DIR__ . '/vendor/autoload.php' ) ) { + require_once AMP__DIR__ . '/vendor/autoload.php'; + } + if ( ! self::$is_registered ) { spl_autoload_register( array( __CLASS__, 'autoload' ) ); self::$is_registered = true; diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index fe07b47e46c..3502a6614e1 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -87,6 +87,20 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $content_url; + /** + * XPath. + * + * @var DOMXPath + */ + private $xpath; + + /** + * Amount of time that was spent parsing CSS. + * + * @var float + */ + private $parse_css_duration = 0.0; + /** * AMP_Base_Sanitizer constructor. * @@ -128,6 +142,7 @@ public function __construct( DOMDocument $dom, array $args = array() ) { } $this->base_url = $guessurl; $this->content_url = WP_CONTENT_URL; + $this->xpath = new DOMXPath( $dom ); } /** @@ -171,7 +186,7 @@ public function sanitize() { * 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 ); + $xpath = $this->xpath; $lower_case = 'translate( %s, "ABCDEFGHIJKLMNOPQRSTUVWXYZ", "abcdefghijklmnopqrstuvwxyz" )'; // In XPath 2.0 this is lower-case(). $predicates = array( @@ -232,6 +247,10 @@ public function sanitize() { } $this->amp_custom_style_element->appendChild( $this->dom->createTextNode( $css ) ); } + + if ( $this->parse_css_duration > 0.0 ) { + AMP_Response_Headers::send_server_timing( 'amp_parse_css', $this->parse_css_duration, 'AMP Parse CSS' ); + } } /** @@ -299,11 +318,13 @@ private function process_style_element( DOMElement $element ) { return; } - $rules = trim( $element->textContent ); - $rules = $this->remove_illegal_css( $rules, $element ); + $stylesheet = trim( $element->textContent ); + if ( $stylesheet ) { + $stylesheet = $this->process_stylesheet( $stylesheet, $element ); + } // Remove if surpasses max size. - $length = strlen( $rules ); + $length = strlen( $stylesheet ); if ( $this->current_custom_size + $length > $this->custom_max_size ) { $this->remove_invalid_child( $element, array( 'message' => __( 'Too much CSS enqueued.', 'amp' ), @@ -311,8 +332,10 @@ private function process_style_element( DOMElement $element ) { return; } - $this->stylesheets[ md5( $rules ) ] = $rules; - $this->current_custom_size += $length; + $hash = md5( $stylesheet ); + + $this->stylesheets[ $hash ] = $stylesheet; + $this->current_custom_size += $length; if ( $element->hasAttribute( 'amp-custom' ) ) { if ( ! $this->amp_custom_style_element ) { @@ -349,58 +372,164 @@ private function process_link_element( DOMElement $element ) { } // Load the CSS from the filesystem. - $rules = "\n/* $href */\n"; - $rules .= file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. - - $rules = $this->remove_illegal_css( $rules, $element ); + $stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. + if ( false === $stylesheet ) { + $this->remove_invalid_child( $element, array( + 'message' => __( 'Unable to load stylesheet from filesystem.', 'amp' ), + ) ); + return; + } + // Honor the link's media attribute. $media = $element->getAttribute( 'media' ); if ( $media && 'all' !== $media ) { - $rules = sprintf( '@media %s { %s }', $media, $rules ); + $stylesheet = sprintf( '@media %s { %s }', $media, $stylesheet ); } - // Remove if surpasses max size. - $length = strlen( $rules ); + $stylesheet = $this->process_stylesheet( $stylesheet, $element ); + + // Skip if surpasses max size. + $length = strlen( $stylesheet ); if ( $this->current_custom_size + $length > $this->custom_max_size ) { $this->remove_invalid_child( $element, array( 'message' => __( 'Too much CSS enqueued.', 'amp' ), ) ); return; } + $hash = md5( $stylesheet ); + $this->stylesheets[ $hash ] = $stylesheet; $this->current_custom_size += $length; - $this->stylesheets[ $href ] = $rules; // Remove now that styles have been processed. $element->parentNode->removeChild( $element ); } /** - * Remove illegal CSS from the stylesheet. + * Process stylesheet. * - * @since 0.7 + * Sanitized invalid CSS properties and rules, removes rules which do not + * apply to the current document, and compresses the CSS to remove whitespace and comments. * - * @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. - * @param DOMElement $element Element where the stylesheet came from. - * @return string Scrubbed stylesheet. + * @since 1.0 + * @todo Add flag for whether to do tree shaking. + * + * @param string $stylesheet Stylesheet. + * @param DOMElement|DOMAttr $node Element (link/style) or style attribute where the stylesheet came from. + * @param array $args { + * Args. + * + * @type bool $convert_width_to_max_width Convert width to max-width. + * } + * @return string Processed stylesheet. */ - private function remove_illegal_css( $stylesheet, $element ) { - $stylesheet = preg_replace( '/\s*!important/', '', $stylesheet, -1, $important_count ); // Note this has to also replace inside comments to be valid. - if ( $important_count > 0 && ! empty( $this->args['validation_error_callback'] ) ) { - call_user_func( $this->args['validation_error_callback'], array( - 'code' => 'css_important_removed', - 'node' => $element, - ) ); + private function process_stylesheet( $stylesheet, $node, $args = array() ) { + $start_time = microtime( true ); + + $args = array_merge( + array( + 'convert_width_to_max_width' => false, + ), + $args + ); + + $cache_key = md5( $stylesheet ); + $cached_processed = wp_cache_get( $cache_key, 'amp-stylesheet' ); + if ( $cached_processed ) { + if ( ! empty( $this->args['validation_error_callback'] ) ) { + foreach ( $cached_processed['validation_errors'] as $validation_error ) { + call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); + } + } + return $cached_processed['stylesheet']; } - $stylesheet = preg_replace( '/overflow(-[xy])?\s*:\s*(auto|scroll)\s*;?\s*/', '', $stylesheet, -1, $overlow_count ); - if ( $overlow_count > 0 && ! empty( $this->args['validation_error_callback'] ) ) { - call_user_func( $this->args['validation_error_callback'], array( - 'code' => 'css_overflow_property_removed', - 'node' => $element, - ) ); + + $parser_settings = Sabberworm\CSS\Settings::create()->withMultibyteSupport( false ); + $css_parser = new Sabberworm\CSS\Parser( $stylesheet, $parser_settings ); + $css_document = $css_parser->parse(); + + // @todo Fetch an @import. + // @todo Disallow anything except @font-face, @keyframes, @media, @supports. + /** + * Rulesets. + * + * @var Sabberworm\CSS\RuleSet\RuleSet[] $rulesets + * @var Sabberworm\CSS\Rule\Rule $rule + */ + $rulesets = $css_document->getAllRuleSets(); + $validation_errors = array(); + foreach ( $rulesets as $ruleset ) { + /** + * Properties. + * + * @var Sabberworm\CSS\Rule\Rule[] $properties + */ + + // Remove properties that have illegal values. See . + // @todo Limit transition to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. + $properties = $ruleset->getRules( 'overflow-' ); + foreach ( $properties as $property ) { + if ( in_array( $property->getValue(), array( 'auto', 'scroll' ), true ) ) { + $validation_errors[] = array( + 'code' => 'illegal_css_property_value', + 'property_name' => $property->getRule(), + 'property_value' => $property->getValue(), + ); + $ruleset->removeRule( $property->getRule() ); + } + } + + // Remove important qualifiers. See . + // @todo Try to convert into something else, like https://www.npmjs.com/package/replace-important. + $properties = $ruleset->getRules(); + foreach ( $properties as $property ) { + if ( $property->getIsImportant() ) { + $validation_errors[] = array( + 'code' => 'illegal_css_important_qualifier', + 'property_name' => $property->getRule(), + 'property_value' => $property->getValue(), + ); + $property->setIsImportant( false ); + } + } + + // Convert width to max-width when requested. See . + if ( $args['convert_width_to_max_width'] ) { + $properties = $ruleset->getRules( 'width' ); + foreach ( $properties as $property ) { + $width_property = new \Sabberworm\CSS\Rule\Rule( 'max-width' ); + $width_property->setValue( $property->getValue() ); + $ruleset->removeRule( $property ); + $ruleset->addRule( $width_property, $property ); + } + } + + // Remove the ruleset if it is now empty. + if ( 0 === count( $ruleset->getRules() ) ) { + $css_document->remove( $ruleset ); + } + // @todo Sort?? + // @todo Delete rules with selectors for -amphtml- class and i-amphtml- tags. } + + $output_format = Sabberworm\CSS\OutputFormat::createCompact(); + $stylesheet = $css_document->render( $output_format ); + + if ( ! empty( $this->args['validation_error_callback'] ) ) { + foreach ( $validation_errors as $validation_error ) { + call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); + } + } + + $this->parse_css_duration += ( microtime( true ) - $start_time ); + + // @todo We need to cache an object that allows us to identify the rules that can be deleted. + wp_cache_set( + $cache_key, + compact( 'stylesheet', 'validation_errors' ), + 'amp-stylesheet' + ); + return $stylesheet; } @@ -450,129 +579,40 @@ private function validate_amp_keyframe( $style ) { * @param DOMElement $element Node. */ private function collect_inline_styles( $element ) { - $value = $element->getAttribute( 'style' ); - if ( ! $value ) { + $style_attribute = $element->getAttributeNode( 'style' ); + if ( ! $style_attribute || ! trim( $style_attribute->nodeValue ) ) { return; } - $class = $element->getAttribute( 'class' ); - $properties = $this->process_style( $value ); + // @todo Use hash from resulting processed CSS so that we can potentially re-use? We need to use the hash of the original rules as the cache key. + $class = 'amp-wp-' . substr( md5( $style_attribute->nodeValue ), 0, 7 ); + $rule = sprintf( '.%s { %s }', $class, $style_attribute->nodeValue ); + $hash = md5( $rule ); + $rule = $this->process_stylesheet( $rule, $style_attribute, array( + 'convert_width_to_max_width' => true, + ) ); + $length = strlen( $rule ); - if ( ! empty( $properties ) ) { - $class_name = $this->generate_class_name( $properties ); - $new_class = trim( $class . ' ' . $class_name ); - - $selector = '.' . $class_name; - $length = strlen( sprintf( '%s { %s }', $selector, join( '; ', $properties ) . ';' ) ); - - if ( $this->current_custom_size + $length > $this->custom_max_size ) { - $this->remove_invalid_attribute( $element, 'style', array( - 'message' => __( 'Too much CSS.', 'amp' ), - ) ); - return; - } - - $element->setAttribute( 'class', $new_class ); - $this->styles[ $selector ] = $properties; - } $element->removeAttribute( 'style' ); - } - - /** - * Sanitize and convert individual styles. - * - * @since 0.4 - * - * @param string $css Style string. - * @return array Style properties. - */ - private function process_style( $css ) { - - // Normalize whitespace. - $css = str_replace( array( "\n", "\r", "\t" ), '', $css ); - /* - * Use preg_split to break up rules by `;` but only if the - * semi-colon is not inside parens (like a data-encoded image). - */ - $styles = preg_split( '/\s*;\s*(?![^(]*\))/', trim( $css, '; ' ) ); - $styles = array_filter( $styles ); - - // Normalize the order of the styles. - sort( $styles ); - - $processed_styles = array(); - - // Normalize whitespace and filter rules. - foreach ( $styles as $index => $rule ) { - $tuple = preg_split( '/\s*:\s*/', $rule, 2 ); - if ( 2 !== count( $tuple ) ) { - continue; - } - - list( $property, $value ) = $this->filter_style( $tuple[0], $tuple[1] ); - if ( empty( $property ) || empty( $value ) ) { - continue; - } - - $processed_styles[ $index ] = "{$property}:{$value}"; + if ( 0 === $length ) { + return; } - return $processed_styles; - } - - /** - * Filter individual CSS name/value pairs. - * - * - Remove overflow if value is `auto` or `scroll` - * - Change `width` to `max-width` - * - Remove !important - * - * @since 0.4 - * - * @param string $property Property. - * @param string $value Value. - * @return array - */ - private function filter_style( $property, $value ) { - /* - * Remove overflow if value is `auto` or `scroll`; not allowed in AMP - * - * @todo This removal needs to be reported. - * @see https://www.ampproject.org/docs/reference/spec.html#properties - */ - if ( preg_match( '#^overflow(-[xy])?$#i', $property ) && preg_match( '#^(auto|scroll)$#i', $value ) ) { - return array( false, false ); + if ( $this->current_custom_size + $length > $this->custom_max_size ) { + $this->remove_invalid_attribute( $element, $style_attribute, array( + 'message' => __( 'Too much CSS.', 'amp' ), + ) ); + return; } - if ( 'width' === $property ) { - $property = 'max-width'; - } + $this->current_custom_size += $length; + $this->stylesheets[ $hash ] = $rule; - /* - * Remove `!important`; not allowed in AMP - * - * @todo This removal needs to be reported. - */ - if ( false !== strpos( $value, 'important' ) ) { - $value = preg_replace( '/\s*\!\s*important$/', '', $value ); + if ( $element->hasAttribute( 'class' ) ) { + $element->setAttribute( 'class', $element->getAttribute( 'class' ) . ' ' . $class ); + } else { + $element->setAttribute( 'class', $class ); } - - return array( $property, $value ); - } - - /** - * Generate a unique class name - * - * Use the md5() of the $data parameter - * - * @since 0.4 - * - * @param string $data Data. - * @return string Class name. - */ - private function generate_class_name( $data ) { - $string = maybe_serialize( $data ); - return 'amp-wp-inline-' . md5( $string ); } } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 7c708b42d29..986099c7d23 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -27,41 +27,41 @@ public function get_body_style_attribute_data() { 'span_one_style' => array( 'This is green.', - 'This is green.', + 'This is green.', array( - '.amp-wp-inline-ad0e57ab02197f7023aa5b93bcf6c97e { color:#00ff00; }', + '.amp-wp-bb01159{color:#0f0;}', ), ), 'span_one_style_bad_format' => array( 'This is green.', - 'This is green.', + 'This is green.', array( - '.amp-wp-inline-ad0e57ab02197f7023aa5b93bcf6c97e { color:#00ff00; }', + '.amp-wp-0837823{color:#0f0;}', ), ), 'span_two_styles_reversed' => array( 'This is green.', - 'This is green.', + 'This is green.', array( - '.amp-wp-inline-58550689c128f3d396444313296e4c47 { background-color:#000; color:#00ff00; }', + '.amp-wp-c71affe{color:#0f0;background-color:#000;}', ), ), 'width_to_max-width' => array( '
', - '
', + '
', array( - '.amp-wp-inline-2676cd1bfa7e8feb4f0e0e8086ae9ce4 { max-width:300px; }', + '.amp-wp-343bce0{max-width:300px;}', ), ), 'span_display_none' => array( 'Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.', - 'Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.', + 'Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.', array( - '.amp-wp-inline-0f1bf07c72fdf1784fff2e164d9dca98 { display:none; }', + '.amp-wp-224b51a{display:none;}', ), ), @@ -73,42 +73,42 @@ public function get_body_style_attribute_data() { '!important_not_allowed' => array( '!important not allowed.', - '!important not allowed.', + '!important not allowed.', array( - '.amp-wp-inline-b370df7c42957a3192cac40a8ddcff79 { margin:1px; }', + '.amp-wp-416ccd3{margin:1px;}', ), ), '!important_with_spaces_not_allowed' => array( '!important not allowed.', - '!important not allowed.', + '!important not allowed.', array( - '.amp-wp-inline-5b88d03e432f20476a218314084d3a05 { color:red; }', + '.amp-wp-952600b{color:red;}', ), ), '!important_multiple_not_allowed' => array( '!important not allowed.', - '!important not allowed.', + '!important not allowed.', array( - '.amp-wp-inline-ef4329d562b6b3486a8a661df5c5280f { background:blue; color:red; }', + '.amp-wp-1e2bfaa{color:red;background:blue;}', ), ), 'two_nodes' => array( 'This is red.', - 'This is red.', + 'This is red.', array( - '.amp-wp-inline-ad0e57ab02197f7023aa5b93bcf6c97e { color:#00ff00; }', - '.amp-wp-inline-f146f9bb819d875bbe5cf83e36368b44 { color:#ff0000; }', + '.amp-wp-bb01159{color:#0f0;}', + '.amp-wp-cc68ddc{color:#f00;}', ), ), 'existing_class_attribute' => array( '
', - '
', + '
', array( - '.amp-wp-inline-3be9b2f79873ad78941ba2b3c03025a3 { background:#000; }', + '.amp-wp-2864855{background:#000;}', ), ), @@ -117,7 +117,7 @@ public function get_body_style_attribute_data() { '
bold!
', '
bold!
', array( - 'div > span { font-weight:bold; font-style: italic; }', + 'div > span{font-weight:bold;font-style:italic;}', ), ), ); @@ -156,10 +156,10 @@ public function get_link_and_style_test_data() { 'multiple_amp_custom_and_other_styles' => array( '', array( - 'b {color:red}', - 'i {color:blue}', - 'u {color:green}', - 's {color:yellow}', + 'b{color:red;}', + 'i{color:blue;}', + 'u{color:green;}', + 's{color:yellow;}', ), ), 'style_elements_with_link_elements' => array( @@ -171,7 +171,7 @@ public function get_link_and_style_test_data() { 'strong.before-dashicon', '.dashicons-dashboard:before', 'strong.after-dashicon', - 's {color:yellow}', + 's{color:yellow;}', ), ), 'style_with_no_head' => array( diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index a4cb1762865..71984f8b8a2 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -190,7 +190,7 @@ public function test_prepare_response() { $this->assertContains( '', $sanitized_html ); $this->assertContains( '', $sanitized_html ); $this->assertContains( '', + '', + array( + 'button{font-weight:bold;}', + ), + ), ); } From 06fca3909c85a1061da33e3f301eafd2e3fef372 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 29 Mar 2018 23:31:22 -0700 Subject: [PATCH 05/31] Include sabberworm/php-css-parser from composer in build; always composer install in Travis build --- .travis.yml | 1 + Gruntfile.js | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 30bd83eaa27..647451c1326 100644 --- a/.travis.yml +++ b/.travis.yml @@ -59,6 +59,7 @@ matrix: env: WP_VERSION=4.7 install: + - if [[ $DEV_LIB_SKIP =~ composer ]]; then composer install --no-dev; fi - nvm install 6 && nvm use 6 - export DEV_LIB_PATH=dev-lib - source $DEV_LIB_PATH/travis.install.sh diff --git a/Gruntfile.js b/Gruntfile.js index 24f1475ee3b..09b3eec6eb9 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -84,15 +84,21 @@ module.exports = function( grunt ) { args: [ 'ls-files' ] }, function( err, res ) { + var paths; if ( err ) { throw new Error( err.message ); } + paths = res.stdout.trim().split( /\n/ ).filter( function( file ) { + return ! /^(\.|bin|([^/]+)+\.(md|json|xml)|Gruntfile\.js|tests|wp-assets|dev-lib|readme\.md|composer\..*)/.test( file ); + } ); + paths.push( 'vendor/autoload.php' ); + paths.push( 'vendor/composer/**' ); + paths.push( 'vendor/sabberworm/php-css-parser/lib/**' ); + grunt.config.set( 'copy', { build: { - src: res.stdout.trim().split( /\n/ ).filter( function( file ) { - return ! /^(\.|bin|([^/]+)+\.(md|json|xml)|Gruntfile\.js|tests|wp-assets|dev-lib|readme\.md|composer\..*)/.test( file ); - } ), + src: paths, dest: 'build', expand: true } From c27e9a2d6f0bf435e8ee8776c6e3c87a017ab3a3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 29 Mar 2018 23:53:57 -0700 Subject: [PATCH 06/31] Add check for composer install; update contributing with dev installation instructions --- amp.php | 21 +++++++++++++++++++-- contributing.md | 18 +++++++++++++++--- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/amp.php b/amp.php index 6481ebcb119..76a73b248e8 100644 --- a/amp.php +++ b/amp.php @@ -21,8 +21,8 @@ function _amp_print_php_version_admin_notice() { ?>
-

-
+

+ +
+

+
+ Date: Fri, 30 Mar 2018 00:07:02 -0700 Subject: [PATCH 07/31] Add tests for AMP_Response_Headers --- tests/test-class-amp-response-headers.php | 116 ++++++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 tests/test-class-amp-response-headers.php diff --git a/tests/test-class-amp-response-headers.php b/tests/test-class-amp-response-headers.php new file mode 100644 index 00000000000..5257ffb45e0 --- /dev/null +++ b/tests/test-class-amp-response-headers.php @@ -0,0 +1,116 @@ +assertContains( + array( + 'name' => 'Foo', + 'value' => 'Bar', + 'replace' => true, + 'status_code' => null, + ), + AMP_Response_Headers::$headers_sent + ); + } + + /** + * Test \AMP_Response_Headers::send_header() when replace arg is passed. + * + * @covers \AMP_Response_Headers::send_header() + */ + public function test_send_header_replace_arg() { + AMP_Response_Headers::send_header( 'Foo', 'Bar', array( + 'replace' => false, + ) ); + $this->assertContains( + array( + 'name' => 'Foo', + 'value' => 'Bar', + 'replace' => false, + 'status_code' => null, + ), + AMP_Response_Headers::$headers_sent + ); + } + + /** + * Test \AMP_Response_Headers::send_header() when status code is passed. + * + * @covers \AMP_Response_Headers::send_header() + */ + public function test_send_header_status_code() { + AMP_Response_Headers::send_header( 'Foo', 'Bar', array( + 'status_code' => 400, + ) ); + $this->assertContains( + array( + 'name' => 'Foo', + 'value' => 'Bar', + 'replace' => true, + 'status_code' => 400, + ), + AMP_Response_Headers::$headers_sent + ); + } + + /** + * Test \AMP_Response_Headers::send_server_timing() when positive duration passed. + * + * @covers \AMP_Response_Headers::send_server_timing() + */ + public function test_send_server_timing_positive_duration() { + AMP_Response_Headers::send_server_timing( 'name', 123, 'Description' ); + $this->assertCount( 1, AMP_Response_Headers::$headers_sent ); + $this->assertEquals( 'Server-Timing', AMP_Response_Headers::$headers_sent[0]['name'] ); + $values = preg_split( '/\s*;\s*/', AMP_Response_Headers::$headers_sent[0]['value'] ); + $this->assertEquals( 'name', $values[0] ); + $this->assertEquals( 'desc="Description"', $values[1] ); + $this->assertStringStartsWith( 'dur=123000.', $values[2] ); + $this->assertFalse( AMP_Response_Headers::$headers_sent[0]['replace'] ); + $this->assertNull( AMP_Response_Headers::$headers_sent[0]['status_code'] ); + } + + /** + * Test \AMP_Response_Headers::send_server_timing() when positive duration passed. + * + * @covers \AMP_Response_Headers::send_server_timing() + */ + public function test_send_server_timing_negative_duration() { + AMP_Response_Headers::send_server_timing( 'name', -microtime( true ) ); + $this->assertCount( 1, AMP_Response_Headers::$headers_sent ); + $this->assertEquals( 'Server-Timing', AMP_Response_Headers::$headers_sent[0]['name'] ); + $values = preg_split( '/\s*;\s*/', AMP_Response_Headers::$headers_sent[0]['value'] ); + $this->assertEquals( 'name', $values[0] ); + $this->assertStringStartsWith( 'dur=0.', $values[1] ); + $this->assertFalse( AMP_Response_Headers::$headers_sent[0]['replace'] ); + $this->assertNull( AMP_Response_Headers::$headers_sent[0]['status_code'] ); + } +} From e16aba2d4368b27229461227622a8f9bd6876790 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 30 Mar 2018 10:41:37 -0700 Subject: [PATCH 08/31] Add missing phpdoc for functions in amp.php --- amp.php | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/amp.php b/amp.php index 76a73b248e8..f93837c5897 100644 --- a/amp.php +++ b/amp.php @@ -59,6 +59,12 @@ function _amp_print_composer_install_admin_notice() { require_once AMP__DIR__ . '/includes/admin/functions.php'; register_activation_hook( __FILE__, 'amp_activate' ); + +/** + * Handle activation of plugin. + * + * @since 0.2 + */ function amp_activate() { amp_after_setup_theme(); if ( ! did_action( 'amp_init' ) ) { @@ -68,8 +74,14 @@ function amp_activate() { } register_deactivation_hook( __FILE__, 'amp_deactivate' ); + +/** + * Handle deactivation of plugin. + * + * @since 0.2 + */ function amp_deactivate() { - // We need to manually remove the amp endpoint + // We need to manually remove the amp endpoint. global $wp_rewrite; foreach ( $wp_rewrite->endpoints as $index => $endpoint ) { if ( amp_get_slug() === $endpoint[1] ) { @@ -147,8 +159,16 @@ function amp_init() { add_action( 'wp', 'amp_maybe_add_actions' ); } -// Make sure the `amp` query var has an explicit value. -// Avoids issues when filtering the deprecated `query_string` hook. +/** + * Make sure the `amp` query var has an explicit value. + * + * This avoids issues when filtering the deprecated `query_string` hook. + * + * @since 0.3.3 + * + * @param array $query_vars Query vars. + * @return array Query vars. + */ function amp_force_query_var_value( $query_vars ) { if ( isset( $query_vars[ amp_get_slug() ] ) && '' === $query_vars[ amp_get_slug() ] ) { $query_vars[ amp_get_slug() ] = 1; @@ -267,20 +287,41 @@ function amp_is_canonical() { return false; } +/** + * Load classes. + * + * @since 0.2 + * @deprecated As of 0.6 since autoloading is now employed. + */ function amp_load_classes() { _deprecated_function( __FUNCTION__, '0.6' ); } +/** + * Add frontend actions. + * + * @since 0.2 + */ function amp_add_frontend_actions() { require_once AMP__DIR__ . '/includes/amp-frontend-actions.php'; } +/** + * Add post template actions. + * + * @since 0.2 + */ function amp_add_post_template_actions() { require_once AMP__DIR__ . '/includes/amp-post-template-actions.php'; require_once AMP__DIR__ . '/includes/amp-post-template-functions.php'; amp_post_template_init_hooks(); } +/** + * Add action to do post template rendering at template_redirect action. + * + * @since 0.2 + */ function amp_prepare_render() { add_action( 'template_redirect', 'amp_render' ); } From 036c6317336df3b08095a7bc97f88cf8182fd01d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 30 Mar 2018 11:18:03 -0700 Subject: [PATCH 09/31] Improve handling of style[amp-keyframes] by merging multiples and ensuring end of body --- .../sanitizers/class-amp-style-sanitizer.php | 63 +++++++++++++++---- tests/test-amp-style-sanitizer.php | 10 +-- 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index fcc35215a00..8f1af18bdee 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -64,6 +64,14 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $amp_custom_style_element; + /** + * The found style[amp-keyframe] elements. + * + * @since 1.0 + * @var DOMElement[] + */ + private $amp_keyframes_style_elements = array(); + /** * Regex for allowed font stylesheet URL. * @@ -219,6 +227,9 @@ public function sanitize() { foreach ( $elements as $element ) { $this->collect_inline_styles( $element ); } + + $this->finalize_amp_keyframes_styles(); + $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. @@ -555,29 +566,23 @@ private function process_stylesheet( $stylesheet, $node, $args = array() ) { * * @since 0.7 * @link https://github.com/ampproject/amphtml/blob/b685a0780a7f59313666225478b2b79b463bcd0b/validator/validator-main.protoascii#L1002-L1043 + * @todo Limit @keyframes to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. + * @todo Use CSS parser to process. * * @param DOMElement $style Style element. * @return true|WP_Error Validity. */ private function validate_amp_keyframe( $style ) { - if ( 'body' !== $style->parentNode->nodeName ) { - return new WP_Error( 'mandatory_body_child', __( 'amp-keyframes is not child of body element.', 'amp' ) ); - } - + // @todo Check size of previous $this->amp_keyframes_style_elements to determine if this one is too big, since they get combined later.. if ( $this->keyframes_max_size && strlen( $style->textContent ) > $this->keyframes_max_size ) { return new WP_Error( 'max_bytes', __( 'amp-keyframes is too large', 'amp' ) ); } - // This logic could be in AMP_Tag_And_Attribute_Sanitizer, but since it only applies to amp-keyframes it seems unnecessary. - $next_sibling = $style->nextSibling; - while ( $next_sibling ) { - if ( $next_sibling instanceof DOMElement ) { - return new WP_Error( 'mandatory_last_child', __( 'amp-keyframes is not last element in body.', 'amp' ) ); - } - $next_sibling = $next_sibling->nextSibling; - } - // @todo Also add validation of the CSS spec itself. + // @todo Minification needed. + // @todo Just capture the keyframes stylesheet and then amend a new style[amp-keyframes] at the end of the doc. + $this->amp_keyframes_style_elements[] = $style; + return true; } @@ -632,4 +637,36 @@ private function collect_inline_styles( $element ) { $element->setAttribute( 'class', $class ); } } + + /** + * Finalize style[amp-keyframes] elements. + * + * Combine all amp-keyframe elements and enforece that it is at the end of the body. + * + * @since 1.0 + * @see https://www.ampproject.org/docs/fundamentals/spec#keyframes-stylesheet + */ + private function finalize_amp_keyframes_styles() { + if ( empty( $this->amp_keyframes_style_elements ) ) { + return; + } + $body = $this->dom->getElementsByTagName( 'body' )->item( 0 ); + if ( ! $body ) { + foreach ( $this->amp_keyframes_style_elements as $other_amp_keyframe_style ) { + $this->remove_invalid_child( $other_amp_keyframe_style, array( + 'code' => 'missing_body_element', + 'message' => __( 'amp-keyframes must be last child of body element.', 'amp' ), + ) ); + } + } else { + $first_amp_keyframes_style = array_shift( $this->amp_keyframes_style_elements ); + foreach ( $this->amp_keyframes_style_elements as $other_amp_keyframe_style ) { + foreach ( $other_amp_keyframe_style->childNodes as $text_node ) { + $first_amp_keyframes_style->appendChild( $text_node ); + } + $other_amp_keyframe_style->parentNode->removeChild( $other_amp_keyframe_style ); + } + $body->appendChild( $first_amp_keyframes_style ); + } + } } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 9ffe0fcec08..c74f7b2e639 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -235,18 +235,18 @@ public function get_keyframe_data() { return array( 'style_amp_keyframes' => array( - '', + '', null, // No Change. ), 'style_amp_keyframes_max_overflow' => array( - '', + '', '', ), 'style_amp_keyframes_last_child' => array( - ' as after', - ' as after', + 'before between as after', + 'before between as after', ), ); } @@ -264,6 +264,8 @@ public function test_keyframe_sanitizer( $source, $expected = null ) { $sanitizer = new AMP_Style_Sanitizer( $dom ); $sanitizer->sanitize(); $content = AMP_DOM_Utils::get_content_from_dom( $dom ); + $content = preg_replace( '#\s+(?=@keyframes)#', '', $content ); + $content = preg_replace( '#\s+(?=)#', '', $content ); $content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content ); $this->assertEquals( $expected, $content ); } From fd04b2099a4341e5cd7c187f31600a0587d4ecc0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 30 Mar 2018 17:42:33 -0700 Subject: [PATCH 10/31] Transform !important property qualifiers into rules with more specific selectors (following replace-important package) Eliminate illegal_css_important_qualifier validation error now that transformed --- .../sanitizers/class-amp-style-sanitizer.php | 65 ++++++++++++++----- tests/test-amp-style-sanitizer.php | 40 +++++++----- 2 files changed, 71 insertions(+), 34 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 8f1af18bdee..eac0f37f808 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -459,17 +459,22 @@ private function process_stylesheet( $stylesheet, $node, $args = array() ) { $css_parser = new Sabberworm\CSS\Parser( $stylesheet, $parser_settings ); $css_document = $css_parser->parse(); - // @todo Fetch an @import. - // @todo Disallow anything except @font-face, @keyframes, @media, @supports. /** * Rulesets. * * @var Sabberworm\CSS\RuleSet\RuleSet[] $rulesets - * @var Sabberworm\CSS\Rule\Rule $rule + * @var Sabberworm\CSS\Rule\Rule $ruleset */ $rulesets = $css_document->getAllRuleSets(); $validation_errors = array(); foreach ( $rulesets as $ruleset ) { + + // @todo Fetch an @import. + // @todo Disallow anything except @font-face, @keyframes, @media, @supports. + if ( ! ( $ruleset instanceof \Sabberworm\CSS\RuleSet\DeclarationBlock ) ) { + continue; + } + /** * Properties. * @@ -490,19 +495,7 @@ private function process_stylesheet( $stylesheet, $node, $args = array() ) { } } - // Remove important qualifiers. See . - // @todo Try to convert into something else, like https://www.npmjs.com/package/replace-important. - $properties = $ruleset->getRules(); - foreach ( $properties as $property ) { - if ( $property->getIsImportant() ) { - $validation_errors[] = array( - 'code' => 'illegal_css_important_qualifier', - 'property_name' => $property->getRule(), - 'property_value' => $property->getValue(), - ); - $property->setIsImportant( false ); - } - } + $this->transform_important_qualifiers( $ruleset, $css_document ); // Delete illegal properties. See . $property_blacklist = array( @@ -561,6 +554,46 @@ private function process_stylesheet( $stylesheet, $node, $args = array() ) { return $stylesheet; } + /** + * Replace !important qualifiers with more specific rules. + * + * @since 1.0 + * @see https://www.npmjs.com/package/replace-important + * @see https://www.ampproject.org/docs/fundamentals/spec#important + * + * @param \Sabberworm\CSS\RuleSet\DeclarationBlock $ruleset Ruleset. + * @param \Sabberworm\CSS\CSSList\Document $css_document CSS Document. + */ + private function transform_important_qualifiers( \Sabberworm\CSS\RuleSet\DeclarationBlock $ruleset, \Sabberworm\CSS\CSSList\Document $css_document ) { + /** + * Properties. + * + * @var Sabberworm\CSS\Rule\Rule[] $properties + */ + $properties = $ruleset->getRules(); + $importants = array(); + foreach ( $properties as $property ) { + if ( $property->getIsImportant() ) { + $ruleset->removeRule( $property->getRule() ); + $property->setIsImportant( false ); + $importants[] = $property; + } + } + if ( empty( $importants ) ) { + return; + } + + $important_ruleset = new \Sabberworm\CSS\RuleSet\DeclarationBlock(); + $important_ruleset->setSelectors( array_map( + function( \Sabberworm\CSS\Property\Selector $old_selector ) { + return new \Sabberworm\CSS\Property\Selector( ':root:not(#FK_ID) ' . $old_selector->getSelector() ); + }, + $ruleset->getSelectors() + ) ); + $important_ruleset->setRules( $importants ); + $css_document->append( $important_ruleset ); // @todo It would be preferable if the important ruleset were inserted adjacent to the original rule. + } + /** * Validate amp-keyframe style. * diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index c74f7b2e639..1ea68cb733b 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -71,27 +71,27 @@ public function get_body_style_attribute_data() { array(), ), - '!important_not_allowed' => array( - '!important not allowed.', - '!important not allowed.', + '!important_is_ok' => array( + '!important is converted.', + '!important is converted.', array( - '.amp-wp-416ccd3{margin:1px;}', + '.amp-wp-6a75598{padding:1px;outline:3px;}:root:not(#FK_ID) .amp-wp-6a75598{margin:2px;}', ), ), - '!important_with_spaces_not_allowed' => array( - '!important not allowed.', - '!important not allowed.', + '!important_with_spaces_also_converted' => array( + '!important is converted.', + '!important is converted.', array( - '.amp-wp-952600b{color:red;}', + ':root:not(#FK_ID) .amp-wp-952600b{color:red;}', ), ), - '!important_multiple_not_allowed' => array( - '!important not allowed.', - '!important not allowed.', + '!important_multiple_is_converted' => array( + '!important is converted.', + '!important is converted.', array( - '.amp-wp-1e2bfaa{color:red;background:blue;}', + ':root:not(#FK_ID) .amp-wp-1e2bfaa{color:red;background:blue;}', ), ), @@ -116,7 +116,7 @@ public function get_body_style_attribute_data() { '
bold!
', '
bold!
', array( - 'div > span{font-weight:bold;font-style:italic;}', + 'div > span{font-style:italic;}:root:not(#FK_ID) div > span{font-weight:bold;}', ), ), @@ -161,11 +161,11 @@ public function test_body_style_attribute_sanitizer( $source, $expected_content, public function get_link_and_style_test_data() { return array( 'multiple_amp_custom_and_other_styles' => array( - '', + '', array( - 'b{color:red;}', + ':root:not(#FK_ID) b{color:red;}', 'i{color:blue;}', - 'u{color:green;}', + 'u{color:green;}:root:not(#FK_ID) u{text-decoration:underline;}', 's{color:yellow;}', ), ), @@ -178,7 +178,7 @@ public function get_link_and_style_test_data() { 'strong.before-dashicon', '.dashicons-dashboard:before', 'strong.after-dashicon', - 's{color:yellow;}', + ':root:not(#FK_ID) s{color:yellow;}', ), ), 'style_with_no_head' => array( @@ -214,7 +214,11 @@ public function test_link_and_style_elements( $source, $expected_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 ] ); + if ( false === strpos( $expected_stylesheet, '{' ) ) { + $this->assertContains( $expected_stylesheet, $actual_stylesheets[ $i ] ); + } else { + $this->assertStringStartsWith( $expected_stylesheet, $actual_stylesheets[ $i ] ); + } $this->assertContains( $expected_stylesheet, $sanitized_html ); } } From 5da10f3aeead4a5b049941ae36ee73a6ab655884 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 30 Mar 2018 23:01:29 -0700 Subject: [PATCH 11/31] Remove illegal @-rules and ensure properties inside @media/@supports are processed --- .../sanitizers/class-amp-style-sanitizer.php | 281 ++++++++++++------ tests/test-amp-style-sanitizer.php | 20 +- 2 files changed, 216 insertions(+), 85 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index eac0f37f808..321e7304b15 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -5,6 +5,17 @@ * @package AMP */ +use \Sabberworm\CSS\RuleSet\DeclarationBlock; +use \Sabberworm\CSS\CSSList\CSSList; +use \Sabberworm\CSS\Property\Selector; +use \Sabberworm\CSS\RuleSet\RuleSet; +use \Sabberworm\CSS\Rule\Rule; +use \Sabberworm\CSS\Property\AtRule; +use \Sabberworm\CSS\CSSList\KeyFrame; +use \Sabberworm\CSS\RuleSet\AtRuleSet; +use \Sabberworm\CSS\Property\Import; +use \Sabberworm\CSS\CSSList\AtRuleBlockList; + /** * Class AMP_Style_Sanitizer * @@ -120,6 +131,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { public function __construct( DOMDocument $dom, array $args = array() ) { parent::__construct( $dom, $args ); + // @todo See https://github.com/ampproject/amphtml/blob/d39efc401241421fe0830e4a710b342c44b1f7b5/validator/validator-main.protoascii#L1335-L1361. $spec_name = 'style[amp-keyframes]'; 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'] ) { @@ -128,6 +140,7 @@ public function __construct( DOMDocument $dom, array $args = array() ) { } } + // @todo Make sure css_spec gets parsed and included in AMP_Allowed_Tags_Generated. See and https://github.com/ampproject/amphtml/blob/d39efc401241421fe0830e4a710b342c44b1f7b5/validator/validator-main.protoascii#L838-L918 $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'] ) { @@ -427,21 +440,21 @@ private function process_link_element( DOMElement $element ) { * * @param string $stylesheet Stylesheet. * @param DOMElement|DOMAttr $node Element (link/style) or style attribute where the stylesheet came from. - * @param array $args { - * Args. + * @param array $options { + * Options. * * @type bool $convert_width_to_max_width Convert width to max-width. * } * @return string Processed stylesheet. */ - private function process_stylesheet( $stylesheet, $node, $args = array() ) { + private function process_stylesheet( $stylesheet, $node, $options = array() ) { $start_time = microtime( true ); - $args = array_merge( + $options = array_merge( array( 'convert_width_to_max_width' => false, ), - $args + $options ); $cache_key = md5( $stylesheet ); @@ -462,96 +475,192 @@ private function process_stylesheet( $stylesheet, $node, $args = array() ) { /** * Rulesets. * - * @var Sabberworm\CSS\RuleSet\RuleSet[] $rulesets - * @var Sabberworm\CSS\Rule\Rule $ruleset + * @var CSSList $css_list + * @var Rule $ruleset */ - $rulesets = $css_document->getAllRuleSets(); - $validation_errors = array(); - foreach ( $rulesets as $ruleset ) { + $validation_errors = $this->process_css_list( $css_document, $options ); + + $output_format = Sabberworm\CSS\OutputFormat::createCompact(); + $stylesheet = $css_document->render( $output_format ); - // @todo Fetch an @import. - // @todo Disallow anything except @font-face, @keyframes, @media, @supports. - if ( ! ( $ruleset instanceof \Sabberworm\CSS\RuleSet\DeclarationBlock ) ) { - continue; + if ( ! empty( $this->args['validation_error_callback'] ) ) { + foreach ( $validation_errors as $validation_error ) { + call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); } + } - /** - * Properties. - * - * @var Sabberworm\CSS\Rule\Rule[] $properties - */ + $this->parse_css_duration += ( microtime( true ) - $start_time ); - // Remove properties that have illegal values. See . - // @todo Limit transition to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. - $properties = $ruleset->getRules( 'overflow-' ); - foreach ( $properties as $property ) { - if ( in_array( $property->getValue(), array( 'auto', 'scroll' ), true ) ) { + // @todo We need to cache an object that allows us to identify the rules that can be deleted. + wp_cache_set( + $cache_key, + compact( 'stylesheet', 'validation_errors' ), + 'amp-stylesheet' + ); + + return $stylesheet; + } + + /** + * Process CSS list. + * + * @since 1.0 + * + * @param CSSList $css_list CSS List. + * @param array $options Options. + * @return array Validation errors. + */ + private function process_css_list( CSSList $css_list, $options ) { + $validation_errors = array(); + + foreach ( $css_list->getContents() as $css_item ) { + if ( $css_item instanceof DeclarationBlock ) { + $validation_errors = array_merge( + $validation_errors, + $this->process_css_declaration_block( $css_item, $css_list, $options ) + ); + } elseif ( $css_item instanceof AtRuleBlockList ) { + // @todo Let the list be informed by the spec. + if ( in_array( $css_item->atRuleName(), array( 'media', 'supports' ), true ) ) { + $validation_errors = array_merge( + $validation_errors, + $this->process_css_list( $css_item, $options ) + ); + } else { $validation_errors[] = array( - 'code' => 'illegal_css_property_value', - 'property_name' => $property->getRule(), - 'property_value' => $property->getValue(), + 'code' => 'illegal_css_at_rule', + /* translators: %s is the CSS at-rule name. */ + 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), ); - $ruleset->removeRule( $property->getRule() ); + $css_list->remove( $css_item ); } - } - - $this->transform_important_qualifiers( $ruleset, $css_document ); - - // Delete illegal properties. See . - $property_blacklist = array( - 'behavior', - '-moz-binding', - ); - foreach ( $property_blacklist as $illegal_property_name ) { - $properties = $ruleset->getRules( $illegal_property_name ); - foreach ( $properties as $property ) { + } elseif ( $css_item instanceof Import ) { + $validation_errors[] = array( + 'code' => 'illegal_css_import_rule', + 'message' => __( 'CSS @import is currently disallowed.', 'amp' ), + ); + $css_list->remove( $css_item ); + } elseif ( $css_item instanceof AtRuleSet ) { + if ( in_array( $css_item->atRuleName(), array( 'font-face' ), true ) ) { + $validation_errors = array_merge( + $validation_errors, + $this->process_css_declaration_block( $css_item, $css_list, $options ) + ); + } else { $validation_errors[] = array( - 'code' => 'illegal_css_property', - 'property_name' => $property->getRule(), - 'property_value' => $property->getValue(), + 'code' => 'illegal_css_at_rule', + /* translators: %s is the CSS at-rule name. */ + 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), ); - $ruleset->removeRule( $property->getRule() ); + $css_list->remove( $css_item ); } + } elseif ( $css_item instanceof KeyFrame ) { + $validation_errors = array_merge( + $validation_errors, + $this->process_css_keyframes( $css_item, $options ) + ); + } elseif ( $css_item instanceof AtRule ) { + $validation_errors[] = array( + 'code' => 'illegal_css_at_rule', + /* translators: %s is the CSS at-rule name. */ + 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), + ); + $css_list->remove( $css_item ); + } else { + $validation_errors[] = array( + 'code' => 'unrecognized_css', + 'message' => __( 'Unrecognized CSS removed.', 'amp' ), + ); + $css_list->remove( $css_item ); } + } + return $validation_errors; + } - // Convert width to max-width when requested. See . - if ( $args['convert_width_to_max_width'] ) { - $properties = $ruleset->getRules( 'width' ); - foreach ( $properties as $property ) { - $width_property = new \Sabberworm\CSS\Rule\Rule( 'max-width' ); - $width_property->setValue( $property->getValue() ); - $ruleset->removeRule( $property ); - $ruleset->addRule( $width_property, $property ); - } - } + /** + * Process CSS rule set. + * + * @since 1.0 + * + * @param RuleSet $ruleset Ruleset. + * @param CSSList $css_list CSS List. + * @param array $options Options. + * + * @return array Validation errors. + */ + private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_list, $options ) { + $validation_errors = array(); - // Remove the ruleset if it is now empty. - if ( 0 === count( $ruleset->getRules() ) ) { - $css_document->remove( $ruleset ); + /** + * Properties. + * + * @var Rule[] $properties + */ + + // Remove properties that have illegal values. See . + // @todo Limit transition to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. + $properties = $ruleset->getRules( 'overflow-' ); + foreach ( $properties as $property ) { + if ( in_array( $property->getValue(), array( 'auto', 'scroll' ), true ) ) { + $validation_errors[] = array( + 'code' => 'illegal_css_property_value', + 'property_name' => $property->getRule(), + 'property_value' => $property->getValue(), + ); + $ruleset->removeRule( $property->getRule() ); } - // @todo Sort?? - // @todo Delete rules with selectors for -amphtml- class and i-amphtml- tags. } - $output_format = Sabberworm\CSS\OutputFormat::createCompact(); - $stylesheet = $css_document->render( $output_format ); + $this->transform_important_qualifiers( $ruleset, $css_list ); - if ( ! empty( $this->args['validation_error_callback'] ) ) { - foreach ( $validation_errors as $validation_error ) { - call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); + // Delete illegal properties. See . + $property_blacklist = array( + 'behavior', + '-moz-binding', + ); + foreach ( $property_blacklist as $illegal_property_name ) { + $properties = $ruleset->getRules( $illegal_property_name ); + foreach ( $properties as $property ) { + $validation_errors[] = array( + 'code' => 'illegal_css_property', + 'property_name' => $property->getRule(), + 'property_value' => $property->getValue(), + ); + $ruleset->removeRule( $property->getRule() ); } } - $this->parse_css_duration += ( microtime( true ) - $start_time ); + // Convert width to max-width when requested. See . + if ( $options['convert_width_to_max_width'] ) { + $properties = $ruleset->getRules( 'width' ); + foreach ( $properties as $property ) { + $width_property = new Rule( 'max-width' ); + $width_property->setValue( $property->getValue() ); + $ruleset->removeRule( $property ); + $ruleset->addRule( $width_property, $property ); + } + } - // @todo We need to cache an object that allows us to identify the rules that can be deleted. - wp_cache_set( - $cache_key, - compact( 'stylesheet', 'validation_errors' ), - 'amp-stylesheet' - ); + // Remove the ruleset if it is now empty. + if ( 0 === count( $ruleset->getRules() ) ) { + $css_list->remove( $ruleset ); + } + // @todo Sort?? + // @todo Delete rules with selectors for -amphtml- class and i-amphtml- tags. + return $validation_errors; + } - return $stylesheet; + /** + * Process CSS keyframes. + * + * @since 1.0 + * + * @param KeyFrame $css_list Ruleset. + * @param array $options Options. + * @return array Validation errors. + */ + private function process_css_keyframes( KeyFrame $css_list, $options ) { + return array(); // @todo Add validation. } /** @@ -560,38 +669,43 @@ private function process_stylesheet( $stylesheet, $node, $args = array() ) { * @since 1.0 * @see https://www.npmjs.com/package/replace-important * @see https://www.ampproject.org/docs/fundamentals/spec#important + * @todo Further tailor for specificity. See https://github.com/ampproject/ampstart/blob/4c21d69afdd07b4c60cd190937bda09901955829/tools/replace-important/lib/index.js#L87-L126. * - * @param \Sabberworm\CSS\RuleSet\DeclarationBlock $ruleset Ruleset. - * @param \Sabberworm\CSS\CSSList\Document $css_document CSS Document. + * @param RuleSet $ruleset Rule set. + * @param CSSList $css_list CSS List. */ - private function transform_important_qualifiers( \Sabberworm\CSS\RuleSet\DeclarationBlock $ruleset, \Sabberworm\CSS\CSSList\Document $css_document ) { + private function transform_important_qualifiers( RuleSet $ruleset, CSSList $css_list ) { /** * Properties. * - * @var Sabberworm\CSS\Rule\Rule[] $properties + * @var Rule[] $properties */ $properties = $ruleset->getRules(); $importants = array(); foreach ( $properties as $property ) { if ( $property->getIsImportant() ) { - $ruleset->removeRule( $property->getRule() ); $property->setIsImportant( false ); - $importants[] = $property; + + // An !important doesn't make sense for rulesets that don't have selectors. + if ( $ruleset instanceof DeclarationBlock ) { + $importants[] = $property; + $ruleset->removeRule( $property->getRule() ); + } } } - if ( empty( $importants ) ) { + if ( ! ( $ruleset instanceof DeclarationBlock ) || empty( $importants ) ) { return; } - $important_ruleset = new \Sabberworm\CSS\RuleSet\DeclarationBlock(); + $important_ruleset = clone $ruleset; $important_ruleset->setSelectors( array_map( - function( \Sabberworm\CSS\Property\Selector $old_selector ) { - return new \Sabberworm\CSS\Property\Selector( ':root:not(#FK_ID) ' . $old_selector->getSelector() ); + function( Selector $old_selector ) { + return new Selector( ':root:not(#FK_ID) ' . $old_selector->getSelector() ); }, $ruleset->getSelectors() ) ); $important_ruleset->setRules( $importants ); - $css_document->append( $important_ruleset ); // @todo It would be preferable if the important ruleset were inserted adjacent to the original rule. + $css_list->append( $important_ruleset ); // @todo It would be preferable if the important ruleset were inserted adjacent to the original rule. } /** @@ -601,6 +715,7 @@ function( \Sabberworm\CSS\Property\Selector $old_selector ) { * @link https://github.com/ampproject/amphtml/blob/b685a0780a7f59313666225478b2b79b463bcd0b/validator/validator-main.protoascii#L1002-L1043 * @todo Limit @keyframes to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. * @todo Use CSS parser to process. + * @todo Eliminate in favor of \AMP_Style_Sanitizer::process_css_keyframes(). * * @param DOMElement $style Style element. * @return true|WP_Error Validity. diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 1ea68cb733b..eebd272ad3a 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -113,10 +113,10 @@ public function get_body_style_attribute_data() { ), 'inline_style_element_with_multiple_rules_containing_selectors_is_removed' => array( - '
bold!
', + '
bold!
', '
bold!
', array( - 'div > span{font-style:italic;}:root:not(#FK_ID) div > span{font-weight:bold;}', + 'div > span{font-style:italic;}@media screen and ( max-width: 640px ){div > span{font-style:normal;}:root:not(#FK_ID) div > span{font-weight:normal;}}:root:not(#FK_ID) div > span{font-weight:bold;}', ), ), @@ -127,6 +127,22 @@ public function get_body_style_attribute_data() { 'button{font-weight:bold;}', ), ), + + 'illegal_at_rules_removed' => array( + '', + '', + array( + 'body{color:black;}', + ), + ), + + 'allowed_at_rules_retained' => array( + '', + '', + array( + '@media screen and ( max-width: 640px ){body{font-size:small;}}@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2");}@supports (display: grid){div{display:grid;}}@-moz-keyframes appear{from{opacity:0;}to{opacity:1;}}@keyframes appear{from{opacity:0;}to{opacity:1;}}', + ), + ), ); } From 523d5ee4faede1a4228b48f1bf71ae507abdf000 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 31 Mar 2018 00:08:43 -0700 Subject: [PATCH 12/31] Include css_spec from validator in AMP_Allowed_Tags_Generated --- bin/amphtml-update.py | 37 ++++++++++++--- .../class-amp-allowed-tags-generated.php | 46 +++++++++++++++++++ 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/bin/amphtml-update.py b/bin/amphtml-update.py index 399d2e07377..e073688bbf8 100644 --- a/bin/amphtml-update.py +++ b/bin/amphtml-update.py @@ -356,12 +356,37 @@ def GetTagSpec(tag_spec, attr_lists): for (field_descriptor, field_value) in tag_spec.cdata.ListFields(): if isinstance(field_value, (unicode, str, bool, int)): cdata_dict[ field_descriptor.name ] = field_value - else: - if hasattr( field_value, '_values' ): - cdata_dict[ field_descriptor.name ] = {} - for _value in field_value._values: - for (key,val) in _value.ListFields(): - cdata_dict[ field_descriptor.name ][ key.name ] = val + elif hasattr( field_value, '_values' ): + cdata_dict[ field_descriptor.name ] = {} + for _value in field_value._values: + for (key,val) in _value.ListFields(): + cdata_dict[ field_descriptor.name ][ key.name ] = val + elif 'css_spec' == field_descriptor.name: + css_spec = {} + + css_spec['at_rules'] = [] + for at_rule_spec in field_value.at_rule_spec: + if '$DEFAULT' == at_rule_spec.name: + continue + css_spec['at_rules'].append( at_rule_spec.name ) + + for css_spec_field_name in ( 'allowed_declarations', 'font_url_spec', 'image_url_spec', 'validate_keyframes' ): + if not hasattr( field_value, css_spec_field_name ): + continue + css_spec_field_value = getattr( field_value, css_spec_field_name ) + if isinstance(css_spec_field_value, (list, collections.Sequence, google.protobuf.internal.containers.RepeatedScalarFieldContainer)): + css_spec[ css_spec_field_name ] = [ val for val in css_spec_field_value ] + elif hasattr( css_spec_field_value, 'ListFields' ): + css_spec[ css_spec_field_name ] = {} + for (css_spec_field_item_descriptor, css_spec_field_item_value) in getattr( field_value, css_spec_field_name ).ListFields(): + if isinstance(css_spec_field_item_value, (list, collections.Sequence, google.protobuf.internal.containers.RepeatedScalarFieldContainer)): + css_spec[ css_spec_field_name ][ css_spec_field_item_descriptor.name ] = [ val for val in css_spec_field_item_value ] + else: + css_spec[ css_spec_field_name ][ css_spec_field_item_descriptor.name ] = css_spec_field_item_value + else: + css_spec[ css_spec_field_name ] = css_spec_field_value + + cdata_dict['css_spec'] = css_spec if len( cdata_dict ) > 0: tag_spec_dict['cdata'] = cdata_dict diff --git a/includes/sanitizers/class-amp-allowed-tags-generated.php b/includes/sanitizers/class-amp-allowed-tags-generated.php index 30e232001d9..61869e8e0b3 100644 --- a/includes/sanitizers/class-amp-allowed-tags-generated.php +++ b/includes/sanitizers/class-amp-allowed-tags-generated.php @@ -8422,6 +8422,35 @@ class AMP_Allowed_Tags_Generated { 'error_message' => 'CSS !important', 'regex' => '!important', ), + 'css_spec' => array( + 'allowed_declarations' => array(), + 'at_rules' => array( + 'font-face', + 'keyframes', + 'media', + 'supports', + ), + 'font_url_spec' => array( + 'allow_empty' => true, + 'allow_relative' => true, + 'allowed_protocol' => array( + 'https', + 'http', + 'data', + ), + ), + 'image_url_spec' => array( + 'allow_empty' => true, + 'allow_relative' => true, + 'allowed_protocol' => array( + 'https', + 'http', + 'data', + 'absolute', + ), + ), + 'validate_keyframes' => false, + ), 'max_bytes' => 50000, 'max_bytes_spec_url' => 'https://www.ampproject.org/docs/reference/spec#maximum-size', ), @@ -8482,6 +8511,23 @@ class AMP_Allowed_Tags_Generated { ), ), 'cdata' => array( + 'css_spec' => array( + 'allowed_declarations' => array( + 'animation-timing-function', + 'offset-distance', + 'opacity', + 'transform', + 'visibility', + ), + 'at_rules' => array( + 'keyframes', + 'media', + 'supports', + ), + 'font_url_spec' => array(), + 'image_url_spec' => array(), + 'validate_keyframes' => true, + ), 'max_bytes' => 500000, 'max_bytes_spec_url' => 'https://www.ampproject.org/docs/reference/spec#keyframes-stylesheet', ), From 1889f7020f605c965d6748ebad786d0cf8871d49 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 31 Mar 2018 00:23:01 -0700 Subject: [PATCH 13/31] Capture full cdata spec for amp-keyframes and amp-custom to use for validation --- .../sanitizers/class-amp-style-sanitizer.php | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 321e7304b15..76fb98afb2d 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -44,20 +44,20 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { private $stylesheets = array(); /** - * Maximum number of bytes allowed for a keyframes style. + * Spec for style[amp-custom] cdata. * - * @since 0.7 - * @var int + * @since 1.0 + * @var array */ - private $keyframes_max_size; + private $style_custom_cdata_spec; /** - * Maximum number of bytes allowed for a AMP Custom style. + * Spec for style[amp-keyframes] cdata. * - * @since 0.7 - * @var int + * @since 1.0 + * @var array */ - private $custom_max_size; + private $style_keyframes_cdata_spec; /** * Current CSS size. @@ -131,21 +131,14 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { public function __construct( DOMDocument $dom, array $args = array() ) { parent::__construct( $dom, $args ); - // @todo See https://github.com/ampproject/amphtml/blob/d39efc401241421fe0830e4a710b342c44b1f7b5/validator/validator-main.protoascii#L1335-L1361. - $spec_name = 'style[amp-keyframes]'; 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->keyframes_max_size = $spec_rule[ AMP_Rule_Spec::CDATA ]['max_bytes']; - break; + if ( ! isset( $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) ) { + continue; } - } - - // @todo Make sure css_spec gets parsed and included in AMP_Allowed_Tags_Generated. See and https://github.com/ampproject/amphtml/blob/d39efc401241421fe0830e4a710b342c44b1f7b5/validator/validator-main.protoascii#L838-L918 - $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; + if ( 'style[amp-keyframes]' === $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) { + $this->style_keyframes_cdata_spec = $spec_rule[ AMP_Rule_Spec::CDATA ]; + } elseif ( 'style amp-custom' === $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) { + $this->style_custom_cdata_spec = $spec_rule[ AMP_Rule_Spec::CDATA ]; } } @@ -349,7 +342,7 @@ private function process_style_element( DOMElement $element ) { // Remove if surpasses max size. $length = strlen( $stylesheet ); - if ( $this->current_custom_size + $length > $this->custom_max_size ) { + if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { $this->remove_invalid_child( $element, array( 'message' => __( 'Too much CSS enqueued.', 'amp' ), ) ); @@ -414,7 +407,7 @@ private function process_link_element( DOMElement $element ) { // Skip if surpasses max size. $length = strlen( $stylesheet ); - if ( $this->current_custom_size + $length > $this->custom_max_size ) { + if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { $this->remove_invalid_child( $element, array( 'message' => __( 'Too much CSS enqueued.', 'amp' ), ) ); @@ -722,7 +715,7 @@ function( Selector $old_selector ) { */ private function validate_amp_keyframe( $style ) { // @todo Check size of previous $this->amp_keyframes_style_elements to determine if this one is too big, since they get combined later.. - if ( $this->keyframes_max_size && strlen( $style->textContent ) > $this->keyframes_max_size ) { + if ( $this->style_keyframes_cdata_spec && strlen( $style->textContent ) > $this->style_keyframes_cdata_spec['max_bytes'] ) { return new WP_Error( 'max_bytes', __( 'amp-keyframes is too large', 'amp' ) ); } @@ -769,7 +762,7 @@ private function collect_inline_styles( $element ) { return; } - if ( $this->current_custom_size + $length > $this->custom_max_size ) { + if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { $this->remove_invalid_attribute( $element, $style_attribute, array( 'message' => __( 'Too much CSS.', 'amp' ), ) ); From 9187710c95ccc65eb2655cf516bdcf82f7127ae2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 31 Mar 2018 00:28:53 -0700 Subject: [PATCH 14/31] Discontinue deleting scroll/auto values for overflow properties since not forbidden --- includes/sanitizers/class-amp-style-sanitizer.php | 12 ------------ tests/test-amp-style-sanitizer.php | 14 ++++---------- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 76fb98afb2d..da476e1ac8a 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -592,18 +592,6 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l // Remove properties that have illegal values. See . // @todo Limit transition to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. - $properties = $ruleset->getRules( 'overflow-' ); - foreach ( $properties as $property ) { - if ( in_array( $property->getValue(), array( 'auto', 'scroll' ), true ) ) { - $validation_errors[] = array( - 'code' => 'illegal_css_property_value', - 'property_name' => $property->getRule(), - 'property_value' => $property->getValue(), - ); - $ruleset->removeRule( $property->getRule() ); - } - } - $this->transform_important_qualifiers( $ruleset, $css_list ); // Delete illegal properties. See . diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index eebd272ad3a..6b80d3096e3 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -65,12 +65,6 @@ public function get_body_style_attribute_data() { ), ), - 'div_amp_banned_style' => array( - 'Scrollbars not allowed.', - 'Scrollbars not allowed.', - array(), - ), - '!important_is_ok' => array( '!important is converted.', '!important is converted.', @@ -113,7 +107,7 @@ public function get_body_style_attribute_data() { ), 'inline_style_element_with_multiple_rules_containing_selectors_is_removed' => array( - '
bold!
', + '
bold!
', '
bold!
', array( 'div > span{font-style:italic;}@media screen and ( max-width: 640px ){div > span{font-style:normal;}:root:not(#FK_ID) div > span{font-weight:normal;}}:root:not(#FK_ID) div > span{font-weight:bold;}', @@ -121,10 +115,10 @@ public function get_body_style_attribute_data() { ), 'illegal_unsafe_properties' => array( - '', + '', '', array( - 'button{font-weight:bold;}', + 'button{font-weight:bold;}@media screen{button{font-weight:bold;}}', ), ), @@ -198,7 +192,7 @@ public function get_link_and_style_test_data() { ), ), 'style_with_no_head' => array( - 'Not good!', + 'Not good!', array( 'body{color:red;}', ), From 3d18b8a1b13e4c761e8df5edaef7cca6e591b5a5 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 31 Mar 2018 16:27:31 -0700 Subject: [PATCH 15/31] Add parsing and validation of keyframes * Handle CSS parse errors. * Let spec inform which @-rules are allowed. * Improve handling of vendor-prefixed properties in whitelist/blacklists. * Inform when !important qualifier is removed from rules with selectors (that aren't keyframes). --- bin/amphtml-update.py | 4 +- .../class-amp-allowed-tags-generated.php | 14 +- .../sanitizers/class-amp-style-sanitizer.php | 304 +++++++++++------- tests/test-amp-style-sanitizer.php | 22 +- 4 files changed, 219 insertions(+), 125 deletions(-) diff --git a/bin/amphtml-update.py b/bin/amphtml-update.py index e073688bbf8..15d609a6454 100644 --- a/bin/amphtml-update.py +++ b/bin/amphtml-update.py @@ -364,11 +364,11 @@ def GetTagSpec(tag_spec, attr_lists): elif 'css_spec' == field_descriptor.name: css_spec = {} - css_spec['at_rules'] = [] + css_spec['allowed_at_rules'] = [] for at_rule_spec in field_value.at_rule_spec: if '$DEFAULT' == at_rule_spec.name: continue - css_spec['at_rules'].append( at_rule_spec.name ) + css_spec['allowed_at_rules'].append( at_rule_spec.name ) for css_spec_field_name in ( 'allowed_declarations', 'font_url_spec', 'image_url_spec', 'validate_keyframes' ): if not hasattr( field_value, css_spec_field_name ): diff --git a/includes/sanitizers/class-amp-allowed-tags-generated.php b/includes/sanitizers/class-amp-allowed-tags-generated.php index 61869e8e0b3..c7d8f0c357a 100644 --- a/includes/sanitizers/class-amp-allowed-tags-generated.php +++ b/includes/sanitizers/class-amp-allowed-tags-generated.php @@ -8423,13 +8423,13 @@ class AMP_Allowed_Tags_Generated { 'regex' => '!important', ), 'css_spec' => array( - 'allowed_declarations' => array(), - 'at_rules' => array( + 'allowed_at_rules' => array( 'font-face', 'keyframes', 'media', 'supports', ), + 'allowed_declarations' => array(), 'font_url_spec' => array( 'allow_empty' => true, 'allow_relative' => true, @@ -8512,6 +8512,11 @@ class AMP_Allowed_Tags_Generated { ), 'cdata' => array( 'css_spec' => array( + 'allowed_at_rules' => array( + 'keyframes', + 'media', + 'supports', + ), 'allowed_declarations' => array( 'animation-timing-function', 'offset-distance', @@ -8519,11 +8524,6 @@ class AMP_Allowed_Tags_Generated { 'transform', 'visibility', ), - 'at_rules' => array( - 'keyframes', - 'media', - 'supports', - ), 'font_url_spec' => array(), 'image_url_spec' => array(), 'validate_keyframes' => true, diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index da476e1ac8a..67344171484 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -43,6 +43,15 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $stylesheets = array(); + /** + * Current amp-custom CSS size. + * + * Sum of CSS located in $styles and $stylesheets. + * + * @var int + */ + private $current_custom_size = 0; + /** * Spec for style[amp-custom] cdata. * @@ -52,36 +61,35 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { private $style_custom_cdata_spec; /** - * Spec for style[amp-keyframes] cdata. + * The style[amp-custom] element. * - * @since 1.0 - * @var array + * @var DOMElement */ - private $style_keyframes_cdata_spec; + private $amp_custom_style_element; /** - * Current CSS size. + * The found style[amp-keyframe] stylesheets. * - * Sum of CSS located in $styles and $stylesheets. - * - * @var int + * @since 1.0 + * @var string[] */ - private $current_custom_size = 0; + private $keyframes_stylesheets = array(); /** - * The style[amp-custom] element. + * Current amp-keyframes CSS size. * - * @var DOMElement + * @since 1.0 + * @var int */ - private $amp_custom_style_element; + private $current_keyframes_size = 0; /** - * The found style[amp-keyframe] elements. + * Spec for style[amp-keyframes] cdata. * * @since 1.0 - * @var DOMElement[] + * @var array */ - private $amp_keyframes_style_elements = array(); + private $style_keyframes_cdata_spec; /** * Regex for allowed font stylesheet URL. @@ -109,6 +117,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { /** * XPath. * + * @since 1.0 * @var DOMXPath */ private $xpath; @@ -116,6 +125,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { /** * Amount of time that was spent parsing CSS. * + * @since 1.0 * @var float */ private $parse_css_duration = 0.0; @@ -325,24 +335,22 @@ public function get_validated_css_file_path( $src ) { * @param DOMElement $element Style element. */ private function process_style_element( DOMElement $element ) { - if ( $element->hasAttribute( 'amp-keyframes' ) ) { - $validity = $this->validate_amp_keyframe( $element ); - if ( is_wp_error( $validity ) ) { - $this->remove_invalid_child( $element, array( - 'message' => $validity->get_error_message(), - ) ); - } - return; - } - $stylesheet = trim( $element->textContent ); + // @todo Any @keyframes rules could be removed from amp-custom and instead added to amp-keyframes. + $is_keyframes = $element->hasAttribute( 'amp-keyframes' ); + $stylesheet = trim( $element->textContent ); + $cdata_spec = $is_keyframes ? $this->style_keyframes_cdata_spec : $this->style_custom_cdata_spec; if ( $stylesheet ) { - $stylesheet = $this->process_stylesheet( $stylesheet, $element ); + $stylesheet = $this->process_stylesheet( $stylesheet, $element, array( + 'allowed_at_rules' => $cdata_spec['css_spec']['allowed_at_rules'], + 'property_whitelist' => $cdata_spec['css_spec']['allowed_declarations'], + 'validate_keyframes' => $cdata_spec['css_spec']['validate_keyframes'], + ) ); } // Remove if surpasses max size. $length = strlen( $stylesheet ); - if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { + if ( ( $is_keyframes ? $this->current_keyframes_size : $this->current_custom_size ) + $length > $cdata_spec['max_bytes'] ) { $this->remove_invalid_child( $element, array( 'message' => __( 'Too much CSS enqueued.', 'amp' ), ) ); @@ -351,8 +359,13 @@ private function process_style_element( DOMElement $element ) { $hash = md5( $stylesheet ); - $this->stylesheets[ $hash ] = $stylesheet; - $this->current_custom_size += $length; + if ( $is_keyframes ) { + $this->keyframes_stylesheets[ $hash ] = $stylesheet; + $this->current_keyframes_size += $length; + } else { + $this->stylesheets[ $hash ] = $stylesheet; + $this->current_custom_size += $length; + } if ( $element->hasAttribute( 'amp-custom' ) ) { if ( ! $this->amp_custom_style_element ) { @@ -403,7 +416,10 @@ private function process_link_element( DOMElement $element ) { $stylesheet = sprintf( '@media %s { %s }', $media, $stylesheet ); } - $stylesheet = $this->process_stylesheet( $stylesheet, $element ); + $stylesheet = $this->process_stylesheet( $stylesheet, $element, array( + 'allowed_at_rules' => $this->style_custom_cdata_spec['css_spec']['allowed_at_rules'], + 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], + ) ); // Skip if surpasses max size. $length = strlen( $stylesheet ); @@ -436,7 +452,9 @@ private function process_link_element( DOMElement $element ) { * @param array $options { * Options. * - * @type bool $convert_width_to_max_width Convert width to max-width. + * @type string[] $property_whitelist Exclusively-allowed properties. + * @type string[] $property_blacklist Disallowed properties. + * @type bool $convert_width_to_max_width Convert width to max-width. * } * @return string Processed stylesheet. */ @@ -445,7 +463,15 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { $options = array_merge( array( + 'allowed_at_rules' => array(), 'convert_width_to_max_width' => false, + 'property_blacklist' => array( + // See . + 'behavior', + '-moz-binding', + ), + 'property_whitelist' => array(), + 'validate_keyframes' => false, ), $options ); @@ -461,20 +487,24 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { return $cached_processed['stylesheet']; } - $parser_settings = Sabberworm\CSS\Settings::create()->withMultibyteSupport( false ); - $css_parser = new Sabberworm\CSS\Parser( $stylesheet, $parser_settings ); - $css_document = $css_parser->parse(); + try { + $parser_settings = Sabberworm\CSS\Settings::create()->withMultibyteSupport( false ); + $css_parser = new Sabberworm\CSS\Parser( $stylesheet, $parser_settings ); + $css_document = $css_parser->parse(); - /** - * Rulesets. - * - * @var CSSList $css_list - * @var Rule $ruleset - */ - $validation_errors = $this->process_css_list( $css_document, $options ); + $validation_errors = $this->process_css_list( $css_document, $options ); - $output_format = Sabberworm\CSS\OutputFormat::createCompact(); - $stylesheet = $css_document->render( $output_format ); + $output_format = Sabberworm\CSS\OutputFormat::createCompact(); + $stylesheet = $css_document->render( $output_format ); + } catch ( Exception $exception ) { + $stylesheet = ''; + $validation_errors = array( + array( + 'code' => 'css_parse_error', + 'message' => $exception->getMessage(), + ), + ); + } if ( ! empty( $this->args['validation_error_callback'] ) ) { foreach ( $validation_errors as $validation_error ) { @@ -507,14 +537,13 @@ private function process_css_list( CSSList $css_list, $options ) { $validation_errors = array(); foreach ( $css_list->getContents() as $css_item ) { - if ( $css_item instanceof DeclarationBlock ) { + if ( $css_item instanceof DeclarationBlock && empty( $options['validate_keyframes'] ) ) { $validation_errors = array_merge( $validation_errors, $this->process_css_declaration_block( $css_item, $css_list, $options ) ); } elseif ( $css_item instanceof AtRuleBlockList ) { - // @todo Let the list be informed by the spec. - if ( in_array( $css_item->atRuleName(), array( 'media', 'supports' ), true ) ) { + if ( in_array( $css_item->atRuleName(), $options['allowed_at_rules'], true ) ) { $validation_errors = array_merge( $validation_errors, $this->process_css_list( $css_item, $options ) @@ -534,7 +563,7 @@ private function process_css_list( CSSList $css_list, $options ) { ); $css_list->remove( $css_item ); } elseif ( $css_item instanceof AtRuleSet ) { - if ( in_array( $css_item->atRuleName(), array( 'font-face' ), true ) ) { + if ( in_array( $css_item->atRuleName(), $options['allowed_at_rules'], true ) ) { $validation_errors = array_merge( $validation_errors, $this->process_css_declaration_block( $css_item, $css_list, $options ) @@ -548,10 +577,18 @@ private function process_css_list( CSSList $css_list, $options ) { $css_list->remove( $css_item ); } } elseif ( $css_item instanceof KeyFrame ) { - $validation_errors = array_merge( - $validation_errors, - $this->process_css_keyframes( $css_item, $options ) - ); + if ( in_array( 'keyframes', $options['allowed_at_rules'], true ) ) { + $validation_errors = array_merge( + $validation_errors, + $this->process_css_keyframes( $css_item, $options ) + ); + } else { + $validation_errors[] = array( + 'code' => 'illegal_css_at_rule', + /* translators: %s is the CSS at-rule name. */ + 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), + ); + } } elseif ( $css_item instanceof AtRule ) { $validation_errors[] = array( 'code' => 'illegal_css_at_rule', @@ -574,6 +611,8 @@ private function process_css_list( CSSList $css_list, $options ) { * Process CSS rule set. * * @since 1.0 + * @link https://www.ampproject.org/docs/design/responsive/style_pages#disallowed-styles + * @link https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles * * @param RuleSet $ruleset Ruleset. * @param CSSList $css_list CSS List. @@ -590,27 +629,39 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l * @var Rule[] $properties */ - // Remove properties that have illegal values. See . - // @todo Limit transition to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. - $this->transform_important_qualifiers( $ruleset, $css_list ); - - // Delete illegal properties. See . - $property_blacklist = array( - 'behavior', - '-moz-binding', - ); - foreach ( $property_blacklist as $illegal_property_name ) { - $properties = $ruleset->getRules( $illegal_property_name ); + // Remove disallowed properties. + if ( ! empty( $options['property_whitelist'] ) ) { + $properties = $ruleset->getRules(); foreach ( $properties as $property ) { - $validation_errors[] = array( - 'code' => 'illegal_css_property', - 'property_name' => $property->getRule(), - 'property_value' => $property->getValue(), - ); - $ruleset->removeRule( $property->getRule() ); + $vendorless_property_name = preg_replace( '/^-\w+-/', '', $property->getRule() ); + if ( ! in_array( $vendorless_property_name, $options['property_whitelist'], true ) ) { + $validation_errors[] = array( + 'code' => 'illegal_css_property', + 'property_name' => $property->getRule(), + 'property_value' => $property->getValue(), + ); + $ruleset->removeRule( $property->getRule() ); + } + } + } else { + foreach ( $options['property_blacklist'] as $illegal_property_name ) { + $properties = $ruleset->getRules( $illegal_property_name ); + foreach ( $properties as $property ) { + $validation_errors[] = array( + 'code' => 'illegal_css_property', + 'property_name' => $property->getRule(), + 'property_value' => $property->getValue(), + ); + $ruleset->removeRule( $property->getRule() ); + } } } + $validation_errors = array_merge( + $validation_errors, + $this->transform_important_qualifiers( $ruleset, $css_list ) + ); + // Convert width to max-width when requested. See . if ( $options['convert_width_to_max_width'] ) { $properties = $ruleset->getRules( 'width' ); @@ -626,7 +677,6 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l if ( 0 === count( $ruleset->getRules() ) ) { $css_list->remove( $ruleset ); } - // @todo Sort?? // @todo Delete rules with selectors for -amphtml- class and i-amphtml- tags. return $validation_errors; } @@ -635,13 +685,52 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l * Process CSS keyframes. * * @since 1.0 + * @link https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. + * @link https://github.com/ampproject/amphtml/blob/b685a0780a7f59313666225478b2b79b463bcd0b/validator/validator-main.protoascii#L1002-L1043 + * @todo Tree shaking could be extended to keyframes, to omit a keyframe if it is not referenced by any rule. * * @param KeyFrame $css_list Ruleset. * @param array $options Options. * @return array Validation errors. */ private function process_css_keyframes( KeyFrame $css_list, $options ) { - return array(); // @todo Add validation. + $validation_errors = array(); + if ( ! empty( $options['property_whitelist'] ) ) { + foreach ( $css_list->getContents() as $rules ) { + if ( ! ( $rules instanceof DeclarationBlock ) ) { + $validation_errors[] = array( + 'code' => 'unrecognized_css', + 'message' => __( 'Unrecognized CSS removed.', 'amp' ), + ); + $css_list->remove( $rules ); + continue; + } + + $validation_errors = array_merge( + $validation_errors, + $this->transform_important_qualifiers( $rules, $css_list ) + ); + + /** + * Properties. + * + * @var Rule[] $properties + */ + $properties = $rules->getRules(); + foreach ( $properties as $property ) { + $vendorless_property_name = preg_replace( '/^-\w+-/', '', $property->getRule() ); + if ( ! in_array( $vendorless_property_name, $options['property_whitelist'], true ) ) { + $validation_errors[] = array( + 'code' => 'illegal_css_property', + 'property_name' => $property->getRule(), + 'property_value' => $property->getValue(), + ); + $rules->removeRule( $property->getRule() ); + } + } + } + } + return $validation_errors; } /** @@ -652,10 +741,18 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) { * @see https://www.ampproject.org/docs/fundamentals/spec#important * @todo Further tailor for specificity. See https://github.com/ampproject/ampstart/blob/4c21d69afdd07b4c60cd190937bda09901955829/tools/replace-important/lib/index.js#L87-L126. * - * @param RuleSet $ruleset Rule set. - * @param CSSList $css_list CSS List. + * @param RuleSet|DeclarationBlock $ruleset Rule set. + * @param CSSList $css_list CSS List. + * @return array Validation errors. */ private function transform_important_qualifiers( RuleSet $ruleset, CSSList $css_list ) { + $validation_errors = array(); + $allow_transformation = ( + $ruleset instanceof DeclarationBlock + && + ! ( $css_list instanceof KeyFrame ) + ); + /** * Properties. * @@ -668,14 +765,19 @@ private function transform_important_qualifiers( RuleSet $ruleset, CSSList $css_ $property->setIsImportant( false ); // An !important doesn't make sense for rulesets that don't have selectors. - if ( $ruleset instanceof DeclarationBlock ) { + if ( $allow_transformation ) { $importants[] = $property; $ruleset->removeRule( $property->getRule() ); + } else { + $validation_errors[] = array( + 'code' => 'illegal_css_important', + 'message' => __( 'Illegal CSS !important qualifier.', 'amp' ), + ); } } } - if ( ! ( $ruleset instanceof DeclarationBlock ) || empty( $importants ) ) { - return; + if ( ! $allow_transformation || empty( $importants ) ) { + return $validation_errors; } $important_ruleset = clone $ruleset; @@ -687,32 +789,8 @@ function( Selector $old_selector ) { ) ); $important_ruleset->setRules( $importants ); $css_list->append( $important_ruleset ); // @todo It would be preferable if the important ruleset were inserted adjacent to the original rule. - } - /** - * Validate amp-keyframe style. - * - * @since 0.7 - * @link https://github.com/ampproject/amphtml/blob/b685a0780a7f59313666225478b2b79b463bcd0b/validator/validator-main.protoascii#L1002-L1043 - * @todo Limit @keyframes to opacity, transform and -vendorPrefix-transform. See https://www.ampproject.org/docs/design/responsive/style_pages#restricted-styles. - * @todo Use CSS parser to process. - * @todo Eliminate in favor of \AMP_Style_Sanitizer::process_css_keyframes(). - * - * @param DOMElement $style Style element. - * @return true|WP_Error Validity. - */ - private function validate_amp_keyframe( $style ) { - // @todo Check size of previous $this->amp_keyframes_style_elements to determine if this one is too big, since they get combined later.. - if ( $this->style_keyframes_cdata_spec && strlen( $style->textContent ) > $this->style_keyframes_cdata_spec['max_bytes'] ) { - return new WP_Error( 'max_bytes', __( 'amp-keyframes is too large', 'amp' ) ); - } - - // @todo Also add validation of the CSS spec itself. - // @todo Minification needed. - // @todo Just capture the keyframes stylesheet and then amend a new style[amp-keyframes] at the end of the doc. - $this->amp_keyframes_style_elements[] = $style; - - return true; + return $validation_errors; } /** @@ -741,6 +819,8 @@ private function collect_inline_styles( $element ) { $hash = md5( $rule ); $rule = $this->process_stylesheet( $rule, $style_attribute, array( 'convert_width_to_max_width' => true, + 'allowed_at_rules' => array(), + 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], ) ); $length = strlen( $rule ); @@ -770,32 +850,30 @@ private function collect_inline_styles( $element ) { /** * Finalize style[amp-keyframes] elements. * - * Combine all amp-keyframe elements and enforece that it is at the end of the body. + * Combine all amp-keyframe elements and enforce that it is at the end of the body. * * @since 1.0 * @see https://www.ampproject.org/docs/fundamentals/spec#keyframes-stylesheet */ private function finalize_amp_keyframes_styles() { - if ( empty( $this->amp_keyframes_style_elements ) ) { + if ( empty( $this->keyframes_stylesheets ) ) { return; } + $body = $this->dom->getElementsByTagName( 'body' )->item( 0 ); if ( ! $body ) { - foreach ( $this->amp_keyframes_style_elements as $other_amp_keyframe_style ) { - $this->remove_invalid_child( $other_amp_keyframe_style, array( + if ( ! empty( $this->args['validation_error_callback'] ) ) { + call_user_func( $this->args['validation_error_callback'], array( 'code' => 'missing_body_element', 'message' => __( 'amp-keyframes must be last child of body element.', 'amp' ), ) ); } - } else { - $first_amp_keyframes_style = array_shift( $this->amp_keyframes_style_elements ); - foreach ( $this->amp_keyframes_style_elements as $other_amp_keyframe_style ) { - foreach ( $other_amp_keyframe_style->childNodes as $text_node ) { - $first_amp_keyframes_style->appendChild( $text_node ); - } - $other_amp_keyframe_style->parentNode->removeChild( $other_amp_keyframe_style ); - } - $body->appendChild( $first_amp_keyframes_style ); + return; } + + $style_element = $this->dom->createElement( 'style' ); + $style_element->setAttribute( 'amp-keyframes', '' ); + $style_element->appendChild( $this->dom->createTextNode( implode( '', $this->keyframes_stylesheets ) ) ); + $body->appendChild( $style_element ); } } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 6b80d3096e3..eea125297f1 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -122,6 +122,12 @@ public function get_body_style_attribute_data() { ), ), + 'illegal_at_rule_in_style_attribute' => array( + 'Parse error.', + 'Parse error.', + array(), + ), + 'illegal_at_rules_removed' => array( '', '', @@ -249,8 +255,8 @@ public function get_keyframe_data() { return array( 'style_amp_keyframes' => array( - '', - null, // No Change. + '', + '', ), 'style_amp_keyframes_max_overflow' => array( @@ -260,7 +266,17 @@ public function get_keyframe_data() { 'style_amp_keyframes_last_child' => array( 'before between as after', - 'before between as after', + 'before between as after', + ), + + 'blacklisted_and_whitelisted_keyframe_properties' => array( + '', + '', + ), + + 'style_amp_keyframes_with_disallowed_rules' => array( + '', + '', ), ); } From f245a8f3e5b5ecf75a1972bcd8bd202bf8527597 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 31 Mar 2018 18:04:42 -0700 Subject: [PATCH 16/31] Ensure stylesheets are output in legacy post templates --- includes/amp-post-template-actions.php | 6 +++ .../templates/class-amp-content-sanitizer.php | 1 + includes/templates/class-amp-content.php | 46 ++++++++++++++----- .../templates/class-amp-post-template.php | 20 ++++---- 4 files changed, 54 insertions(+), 19 deletions(-) diff --git a/includes/amp-post-template-actions.php b/includes/amp-post-template-actions.php index b956faaf6c6..88d717c2c6e 100644 --- a/includes/amp-post-template-actions.php +++ b/includes/amp-post-template-actions.php @@ -107,6 +107,12 @@ function amp_post_template_add_schemaorg_metadata() { * @param AMP_Post_Template $amp_template Template. */ function amp_post_template_add_styles( $amp_template ) { + $stylesheets = $amp_template->get( 'post_amp_stylesheets' ); + if ( ! empty( $stylesheets ) ) { + echo '/* Inline stylesheets */' . PHP_EOL; // WPCS: XSS OK. + echo implode( '', $stylesheets ); // WPCS: XSS OK. + } + $styles = $amp_template->get( 'post_amp_styles' ); if ( ! empty( $styles ) ) { echo '/* Inline styles */' . PHP_EOL; // WPCS: XSS OK. diff --git a/includes/templates/class-amp-content-sanitizer.php b/includes/templates/class-amp-content-sanitizer.php index 7c24ee563e0..d9206b427f3 100644 --- a/includes/templates/class-amp-content-sanitizer.php +++ b/includes/templates/class-amp-content-sanitizer.php @@ -17,6 +17,7 @@ class AMP_Content_Sanitizer { * * @since 0.4.1 * @since 0.7 Passing return_styles=false in $global_args causes stylesheets to be returned instead of styles. + * @deprecated Since 1.0 * * @param string $content HTML content string or DOM document. * @param string[] $sanitizer_classes Sanitizer classes. diff --git a/includes/templates/class-amp-content.php b/includes/templates/class-amp-content.php index 06897b8edf1..2dc7890d0fe 100644 --- a/includes/templates/class-amp-content.php +++ b/includes/templates/class-amp-content.php @@ -34,10 +34,19 @@ class AMP_Content { /** * AMP styles. * + * @deprecated * @var array */ private $amp_styles = array(); + /** + * AMP stylesheets. + * + * @since 1.0 + * @var array + */ + private $amp_stylesheets = array(); + /** * Args. * @@ -97,10 +106,22 @@ public function get_amp_scripts() { /** * Get AMP styles. * - * @return array + * @deprecated Since 1.0 in favor of the get_amp_stylesheets method. + * @return array Empty list. */ public function get_amp_styles() { - return $this->amp_styles; + _deprecated_function( __METHOD__, '1.0', __CLASS__ . '::get_amp_stylesheets' ); + return array(); + } + + /** + * Get AMP styles. + * + * @since 1.0 + * @return array + */ + public function get_amp_stylesheets() { + return $this->amp_stylesheets; } /** @@ -130,12 +151,13 @@ private function add_scripts( $scripts ) { } /** - * Add styles. + * Add stylesheets. * - * @param array $styles Styles. + * @since 1.0 + * @param array $stylesheets Styles. */ - private function add_styles( $styles ) { - $this->amp_styles = array_merge( $this->amp_styles, $styles ); + private function add_stylesheets( $stylesheets ) { + $this->amp_stylesheets = array_merge( $this->amp_stylesheets, $stylesheets ); } /** @@ -179,14 +201,16 @@ private function unregister_embed_handlers( $embed_handlers ) { * * @see AMP_Content_Sanitizer::sanitize() * @param string $content Content. - * @return array Sanitized content. + * @return string Sanitized content. */ private function sanitize( $content ) { - list( $sanitized_content, $scripts, $styles ) = AMP_Content_Sanitizer::sanitize( $content, $this->sanitizer_classes, $this->args ); + $dom = AMP_DOM_Utils::get_dom_from_content( $content ); + + $results = AMP_Content_Sanitizer::sanitize_document( $dom, $this->sanitizer_classes, $this->args ); - $this->add_scripts( $scripts ); - $this->add_styles( $styles ); + $this->add_scripts( $results['scripts'] ); + $this->add_stylesheets( $results['stylesheets'] ); - return $sanitized_content; + return AMP_DOM_Utils::get_content_from_dom( $dom ); } } diff --git a/includes/templates/class-amp-post-template.php b/includes/templates/class-amp-post-template.php index a917e7aaf10..d0d142d3d36 100644 --- a/includes/templates/class-amp-post-template.php +++ b/includes/templates/class-amp-post-template.php @@ -129,7 +129,8 @@ public function __construct( $post ) { 'merriweather' => 'https://fonts.googleapis.com/css?family=Merriweather:400,400italic,700,700italic', ), - 'post_amp_styles' => array(), + 'post_amp_stylesheets' => array(), + 'post_amp_styles' => array(), // Deprecated. 'amp_analytics' => amp_add_custom_analytics(), ); @@ -319,7 +320,7 @@ private function build_post_content() { $this->add_data_by_key( 'post_amp_content', $amp_content->get_amp_content() ); $this->merge_data_for_key( 'amp_component_scripts', $amp_content->get_amp_scripts() ); - $this->merge_data_for_key( 'post_amp_styles', $amp_content->get_amp_styles() ); + $this->add_data_by_key( 'post_amp_stylesheets', $amp_content->get_amp_stylesheets() ); } /** @@ -347,14 +348,17 @@ private function build_post_featured_image() { $featured_image = get_post( $featured_id ); - list( $sanitized_html, $featured_scripts, $featured_styles ) = AMP_Content_Sanitizer::sanitize( - $featured_html, + $dom = AMP_DOM_Utils::get_dom_from_content( $featured_html ); + $assets = AMP_Content_Sanitizer::sanitize_document( + $dom, array( 'AMP_Img_Sanitizer' => array() ), array( 'content_max_width' => $this->get( 'content_max_width' ), ) ); + $sanitized_html = AMP_DOM_Utils::get_content_from_dom( $dom ); + $this->add_data_by_key( 'featured_image', array( 'amp_html' => $sanitized_html, @@ -362,12 +366,12 @@ private function build_post_featured_image() { ) ); - if ( $featured_scripts ) { - $this->merge_data_for_key( 'amp_component_scripts', $featured_scripts ); + if ( $assets['scripts'] ) { + $this->merge_data_for_key( 'amp_component_scripts', $assets['scripts'] ); } - if ( $featured_styles ) { - $this->merge_data_for_key( 'post_amp_styles', $featured_styles ); + if ( $assets['stylesheets'] ) { + $this->merge_data_for_key( 'post_amp_stylesheets', $assets['stylesheets'] ); } } From 6c4980740d049f380ec8d31cc7a7837f41ad8891 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 31 Mar 2018 21:19:46 -0700 Subject: [PATCH 17/31] Bump required PHP to 5.3.2 for sake of php-css-parser --- amp.php | 2 +- readme.md | 2 +- readme.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/amp.php b/amp.php index f93837c5897..860e4371cd9 100644 --- a/amp.php +++ b/amp.php @@ -25,7 +25,7 @@ function _amp_print_php_version_admin_notice() { Date: Sun, 1 Apr 2018 00:44:05 -0700 Subject: [PATCH 18/31] Add class-based selector tree shaking of CSS rules --- composer.json | 8 +- composer.lock | 24 +- .../sanitizers/class-amp-style-sanitizer.php | 225 ++++++++++++------ tests/test-amp-style-sanitizer.php | 2 +- 4 files changed, 178 insertions(+), 81 deletions(-) diff --git a/composer.json b/composer.json index 09eda2e54a9..bf538e3783b 100644 --- a/composer.json +++ b/composer.json @@ -5,8 +5,14 @@ "type": "wordpress-plugin", "license": "GPL-2.0", "version": "1.0.0", + "repositories": [ + { + "type": "vcs", + "url": "https://github.com/xwp/PHP-CSS-Parser" + } + ], "require": { - "sabberworm/php-css-parser": "*" + "sabberworm/php-css-parser": "dev-master" }, "require-dev": { "wp-coding-standards/wpcs": "^0.14.0", diff --git a/composer.lock b/composer.lock index bc268227ed9..4579f0f7cd1 100644 --- a/composer.lock +++ b/composer.lock @@ -4,27 +4,27 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file", "This file is @generated automatically" ], - "content-hash": "4859d4cee2cb9fec9dc6e86f80459837", + "content-hash": "3e2682fa9441981321b1f903b75abb25", "packages": [ { "name": "sabberworm/php-css-parser", - "version": "8.1.0", + "version": "dev-master", "source": { "type": "git", - "url": "https://github.com/sabberworm/PHP-CSS-Parser.git", - "reference": "850cbbcbe7fbb155387a151ea562897a67e242ef" + "url": "https://github.com/xwp/PHP-CSS-Parser.git", + "reference": "e3204589287c28396b3db16b92ec30dab19ac2e9" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sabberworm/PHP-CSS-Parser/zipball/850cbbcbe7fbb155387a151ea562897a67e242ef", - "reference": "850cbbcbe7fbb155387a151ea562897a67e242ef", + "url": "https://api.github.com/repos/xwp/PHP-CSS-Parser/zipball/e3204589287c28396b3db16b92ec30dab19ac2e9", + "reference": "e3204589287c28396b3db16b92ec30dab19ac2e9", "shasum": "" }, "require": { "php": ">=5.3.2" }, "require-dev": { - "phpunit/phpunit": "*" + "phpunit/phpunit": "~4.8" }, "type": "library", "autoload": { @@ -32,7 +32,6 @@ "Sabberworm\\CSS": "lib/" } }, - "notification-url": "https://packagist.org/downloads/", "license": [ "MIT" ], @@ -48,7 +47,10 @@ "parser", "stylesheet" ], - "time": "2016-07-19T19:14:21+00:00" + "support": { + "source": "https://github.com/xwp/PHP-CSS-Parser/tree/master" + }, + "time": "2018-04-01 07:35:36" } ], "packages-dev": [ @@ -266,7 +268,9 @@ ], "aliases": [], "minimum-stability": "stable", - "stability-flags": [], + "stability-flags": { + "sabberworm/php-css-parser": 20 + }, "prefer-stable": false, "prefer-lowest": false, "platform": [], diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 67344171484..c218057bb76 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -114,6 +114,14 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $content_url; + /** + * Class names used in document. + * + * @since 1.0 + * @var array + */ + private $used_class_names = array(); + /** * XPath. * @@ -193,6 +201,20 @@ public function get_stylesheets() { return array_merge( $this->stylesheets, parent::get_stylesheets() ); } + /** + * Get list of all the class names used in the document. + * + * @since 1.0 + * @return array Used class names. + */ + private function get_used_class_names() { + $classes = ' '; + foreach ( $this->xpath->query( '//*/@class' ) as $class_attribute ) { + $classes .= ' ' . $class_attribute->nodeValue; + } + return array_unique( array_filter( preg_split( '/\s+/', trim( $classes ) ) ) ); + } + /** * Sanitize CSS styles within the HTML contained in this instance's DOMDocument. * @@ -206,6 +228,8 @@ public function sanitize() { return; } + $this->used_class_names = $this->get_used_class_names(); + /* * 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. @@ -342,17 +366,20 @@ private function process_style_element( DOMElement $element ) { $cdata_spec = $is_keyframes ? $this->style_keyframes_cdata_spec : $this->style_custom_cdata_spec; if ( $stylesheet ) { $stylesheet = $this->process_stylesheet( $stylesheet, $element, array( - 'allowed_at_rules' => $cdata_spec['css_spec']['allowed_at_rules'], - 'property_whitelist' => $cdata_spec['css_spec']['allowed_declarations'], - 'validate_keyframes' => $cdata_spec['css_spec']['validate_keyframes'], + 'allowed_at_rules' => $cdata_spec['css_spec']['allowed_at_rules'], + 'property_whitelist' => $cdata_spec['css_spec']['allowed_declarations'], + 'validate_keyframes' => $cdata_spec['css_spec']['validate_keyframes'], + 'class_selector_tree_shaking' => ! $cdata_spec['css_spec']['validate_keyframes'], ) ); } // Remove if surpasses max size. - $length = strlen( $stylesheet ); - if ( ( $is_keyframes ? $this->current_keyframes_size : $this->current_custom_size ) + $length > $cdata_spec['max_bytes'] ) { + $length = strlen( $stylesheet ); + $current_size = $is_keyframes ? $this->current_keyframes_size : $this->current_custom_size; + if ( $current_size + $length > $cdata_spec['max_bytes'] ) { $this->remove_invalid_child( $element, array( - 'message' => __( 'Too much CSS enqueued.', 'amp' ), + /* translators: %d is the number of bytes over the limit */ + 'message' => sprintf( __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), ( $current_size + $length ) - $cdata_spec['max_bytes'] ), ) ); return; } @@ -417,15 +444,17 @@ private function process_link_element( DOMElement $element ) { } $stylesheet = $this->process_stylesheet( $stylesheet, $element, array( - 'allowed_at_rules' => $this->style_custom_cdata_spec['css_spec']['allowed_at_rules'], - 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], + 'allowed_at_rules' => $this->style_custom_cdata_spec['css_spec']['allowed_at_rules'], + 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], + 'class_selector_tree_shaking' => true, ) ); // Skip if surpasses max size. $length = strlen( $stylesheet ); if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { $this->remove_invalid_child( $element, array( - 'message' => __( 'Too much CSS enqueued.', 'amp' ), + /* translators: %d is the number of bytes over the limit */ + 'message' => sprintf( __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), ( $this->current_custom_size + $length ) - $this->style_custom_cdata_spec['max_bytes'] ), ) ); return; } @@ -445,20 +474,67 @@ private function process_link_element( DOMElement $element ) { * apply to the current document, and compresses the CSS to remove whitespace and comments. * * @since 1.0 - * @todo Add flag for whether to do tree shaking. * * @param string $stylesheet Stylesheet. * @param DOMElement|DOMAttr $node Element (link/style) or style attribute where the stylesheet came from. - * @param array $options { + * @param array $options { * Options. * - * @type string[] $property_whitelist Exclusively-allowed properties. - * @type string[] $property_blacklist Disallowed properties. - * @type bool $convert_width_to_max_width Convert width to max-width. + * @type bool $class_selector_tree_shaking Whether to perform tree shaking to delete rules that reference class names not extant in the current document. + * @type string[] $property_whitelist Exclusively-allowed properties. + * @type string[] $property_blacklist Disallowed properties. + * @type bool $convert_width_to_max_width Convert width to max-width. * } * @return string Processed stylesheet. */ private function process_stylesheet( $stylesheet, $node, $options = array() ) { + $should_tree_shake = ! empty( $options['class_selector_tree_shaking'] ); + unset( $options['class_selector_tree_shaking'] ); + $cache_key = md5( $stylesheet . serialize( $options ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + + $cache_group = 'amp-parsed-stylesheet-v1'; + $parsed = wp_cache_get( $cache_key, $cache_group ); + if ( ! $parsed || ! isset( $parsed['stylesheet'] ) || ! is_array( $parsed['stylesheet'] ) ) { + $parsed = $this->parse_stylesheet( $stylesheet, $options ); + wp_cache_set( $cache_key, $parsed, $cache_group ); + } + + if ( ! empty( $this->args['validation_error_callback'] ) && ! empty( $parsed['validation_errors'] ) ) { + foreach ( $parsed['validation_errors'] as $validation_error ) { + call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); + } + } + + $stylesheet = ''; + foreach ( $parsed['stylesheet'] as $stylesheet_part ) { + if ( is_array( $stylesheet_part ) ) { + list( $referenced_classes, $selectors, $declaration_block ) = $stylesheet_part; + if ( ! $should_tree_shake || ( empty( $referenced_classes ) || 0 !== count( array_intersect( $referenced_classes, $this->used_class_names ) ) ) ) { + $stylesheet .= $selectors . $declaration_block; + } + } else { + $stylesheet .= $stylesheet_part; + } + } + + return $stylesheet; + } + + /** + * Parse stylesheet. + * + * @since 1.0 + * + * @param string $stylesheet_string Stylesheet. + * @param array $options Options. See definition in \AMP_Style_Sanitizer::process_stylesheet(). + * @return array { + * Parsed stylesheet. + * + * @type array $stylesheet Stylesheet parts, where arrays are tuples for declaration blocks. + * @type array $validation_errors Validation errors. + * } + */ + private function parse_stylesheet( $stylesheet_string, $options = array() ) { $start_time = microtime( true ); $options = array_merge( @@ -476,52 +552,77 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { $options ); - $cache_key = md5( $stylesheet ); - $cached_processed = wp_cache_get( $cache_key, 'amp-stylesheet' ); - if ( $cached_processed ) { - if ( ! empty( $this->args['validation_error_callback'] ) ) { - foreach ( $cached_processed['validation_errors'] as $validation_error ) { - call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); - } - } - return $cached_processed['stylesheet']; - } - + $stylesheet = array(); + $validation_errors = array(); try { $parser_settings = Sabberworm\CSS\Settings::create()->withMultibyteSupport( false ); - $css_parser = new Sabberworm\CSS\Parser( $stylesheet, $parser_settings ); + $css_parser = new Sabberworm\CSS\Parser( $stylesheet_string, $parser_settings ); $css_document = $css_parser->parse(); $validation_errors = $this->process_css_list( $css_document, $options ); $output_format = Sabberworm\CSS\OutputFormat::createCompact(); - $stylesheet = $css_document->render( $output_format ); - } catch ( Exception $exception ) { - $stylesheet = ''; - $validation_errors = array( - array( - 'code' => 'css_parse_error', - 'message' => $exception->getMessage(), - ), - ); - } - if ( ! empty( $this->args['validation_error_callback'] ) ) { - foreach ( $validation_errors as $validation_error ) { - call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); + $before_declaration_block = '/*AMP_WP_BEFORE_DECLARATION_BLOCK*/'; + $between_selectors = '/*AMP_WP_BETWEEN_SELECTORS*/'; + $after_declaration_block_selectors = '/*AMP_WP_BEFORE_DECLARATION_SELECTORS*/'; + $after_declaration_block = '/*AMP_WP_AFTER_DECLARATION*/'; + + $output_format->set( 'BeforeDeclarationBlock', $before_declaration_block ); + $output_format->set( 'AfterDeclarationBlockSelectors', $after_declaration_block_selectors ); + $output_format->set( 'AfterDeclarationBlock', $after_declaration_block ); + $output_format->set( 'SpaceBeforeSelectorSeparator', $between_selectors ); + + $stylesheet_string = $css_document->render( $output_format ); + + $pattern = '#'; + $pattern .= '(' . preg_quote( $before_declaration_block, '#' ) . ')'; + $pattern .= '(.+?)'; + $pattern .= preg_quote( $after_declaration_block_selectors, '#' ); + $pattern .= '(.+?)'; + $pattern .= preg_quote( $after_declaration_block, '#' ); + $pattern .= '#s'; + + $split_stylesheet = preg_split( $pattern, $stylesheet_string, -1, PREG_SPLIT_DELIM_CAPTURE ); + $length = count( $split_stylesheet ); + for ( $i = 0; $i < $length; $i++ ) { + if ( $before_declaration_block === $split_stylesheet[ $i ] ) { + $selectors = explode( $between_selectors, $split_stylesheet[ ++$i ] ); + $declaration = $split_stylesheet[ ++$i ]; + + $classes = array(); + foreach ( $selectors as $selector ) { + if ( preg_match_all( '/(?<=\.)([a-zA-Z0-9_-]+)/', $selector, $matches ) ) { + $classes = array_merge( $classes, $matches[0] ); + } else { + /* + * If one of the selectors lacks class names, then consider the entire entire + * declaration block to not reference class names so that it will never be tree-shaken. + */ + $classes = array(); + break; + } + } + + $stylesheet[] = array( + $classes, + implode( '', $selectors ), + $declaration, + ); + } else { + $stylesheet[] = $split_stylesheet[ $i ]; + } } + } catch ( Exception $exception ) { + $validation_errors[] = array( + 'code' => 'css_parse_error', + 'message' => $exception->getMessage(), + ); } $this->parse_css_duration += ( microtime( true ) - $start_time ); - // @todo We need to cache an object that allows us to identify the rules that can be deleted. - wp_cache_set( - $cache_key, - compact( 'stylesheet', 'validation_errors' ), - 'amp-stylesheet' - ); - - return $stylesheet; + return compact( 'stylesheet', 'validation_errors' ); } /** @@ -623,12 +724,6 @@ private function process_css_list( CSSList $css_list, $options ) { private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_list, $options ) { $validation_errors = array(); - /** - * Properties. - * - * @var Rule[] $properties - */ - // Remove disallowed properties. if ( ! empty( $options['property_whitelist'] ) ) { $properties = $ruleset->getRules(); @@ -711,11 +806,6 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) { $this->transform_important_qualifiers( $rules, $css_list ) ); - /** - * Properties. - * - * @var Rule[] $properties - */ $properties = $rules->getRules(); foreach ( $properties as $property ) { $vendorless_property_name = preg_replace( '/^-\w+-/', '', $property->getRule() ); @@ -753,11 +843,6 @@ private function transform_important_qualifiers( RuleSet $ruleset, CSSList $css_ ! ( $css_list instanceof KeyFrame ) ); - /** - * Properties. - * - * @var Rule[] $properties - */ $properties = $ruleset->getRules(); $importants = array(); foreach ( $properties as $property ) { @@ -818,21 +903,22 @@ private function collect_inline_styles( $element ) { $rule = sprintf( '.%s { %s }', $class, $style_attribute->nodeValue ); $hash = md5( $rule ); $rule = $this->process_stylesheet( $rule, $style_attribute, array( - 'convert_width_to_max_width' => true, - 'allowed_at_rules' => array(), - 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], + 'convert_width_to_max_width' => true, + 'allowed_at_rules' => array(), + 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], + 'class_selector_tree_shaking' => false, ) ); $length = strlen( $rule ); - $element->removeAttribute( 'style' ); - if ( 0 === $length ) { + $element->removeAttribute( 'style' ); return; } if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { $this->remove_invalid_attribute( $element, $style_attribute, array( - 'message' => __( 'Too much CSS.', 'amp' ), + /* translators: %d is the number of bytes over the limit */ + 'message' => sprintf( __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), ( $this->current_custom_size + $length ) - $this->style_custom_cdata_spec['max_bytes'] ), ) ); return; } @@ -840,6 +926,7 @@ private function collect_inline_styles( $element ) { $this->current_custom_size += $length; $this->stylesheets[ $hash ] = $rule; + $element->removeAttribute( 'style' ); if ( $element->hasAttribute( 'class' ) ) { $element->setAttribute( 'class', $element->getAttribute( 'class' ) . ' ' . $class ); } else { diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index eea125297f1..fa662adf6b3 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -187,7 +187,7 @@ public function get_link_and_style_test_data() { ), 'style_elements_with_link_elements' => array( sprintf( - '', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + '', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet includes_url( 'css/dashicons.css' ) ), array( From 6b5f97e09a8c4e96f06b5086ddf65736e9534f9e Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 1 Apr 2018 01:11:13 -0700 Subject: [PATCH 19/31] Further reduce CSS size by deleting individual selectors that lack valid class references --- .../sanitizers/class-amp-style-sanitizer.php | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index c218057bb76..0ed12d39edd 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -508,9 +508,20 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { $stylesheet = ''; foreach ( $parsed['stylesheet'] as $stylesheet_part ) { if ( is_array( $stylesheet_part ) ) { - list( $referenced_classes, $selectors, $declaration_block ) = $stylesheet_part; - if ( ! $should_tree_shake || ( empty( $referenced_classes ) || 0 !== count( array_intersect( $referenced_classes, $this->used_class_names ) ) ) ) { - $stylesheet .= $selectors . $declaration_block; + list( $selectors_parsed, $declaration_block ) = $stylesheet_part; + + $selectors = array(); + if ( $should_tree_shake ) { + foreach ( $selectors_parsed as $selector => $class_names ) { + if ( count( $class_names ) === count( array_intersect( $class_names, $this->used_class_names ) ) ) { + $selectors[] = $selector; + } + } + } else { + $selectors = array_keys( $selectors_parsed ); + } + if ( ! empty( $selectors ) ) { + $stylesheet .= implode( ',', $selectors ) . $declaration_block; } } else { $stylesheet .= $stylesheet_part; @@ -569,9 +580,9 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $after_declaration_block = '/*AMP_WP_AFTER_DECLARATION*/'; $output_format->set( 'BeforeDeclarationBlock', $before_declaration_block ); + $output_format->set( 'SpaceBeforeSelectorSeparator', $between_selectors ); $output_format->set( 'AfterDeclarationBlockSelectors', $after_declaration_block_selectors ); $output_format->set( 'AfterDeclarationBlock', $after_declaration_block ); - $output_format->set( 'SpaceBeforeSelectorSeparator', $between_selectors ); $stylesheet_string = $css_document->render( $output_format ); @@ -587,26 +598,21 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $length = count( $split_stylesheet ); for ( $i = 0; $i < $length; $i++ ) { if ( $before_declaration_block === $split_stylesheet[ $i ] ) { - $selectors = explode( $between_selectors, $split_stylesheet[ ++$i ] ); + $selectors = explode( $between_selectors . ',', $split_stylesheet[ ++$i ] ); $declaration = $split_stylesheet[ ++$i ]; - $classes = array(); + $selectors_parsed = array(); foreach ( $selectors as $selector ) { + $classes = array(); + // @todo There could be a false negative here, such as when :not(body.home) is used. Nots should be first removed prior to matching. if ( preg_match_all( '/(?<=\.)([a-zA-Z0-9_-]+)/', $selector, $matches ) ) { - $classes = array_merge( $classes, $matches[0] ); - } else { - /* - * If one of the selectors lacks class names, then consider the entire entire - * declaration block to not reference class names so that it will never be tree-shaken. - */ - $classes = array(); - break; + $classes = $matches[0]; } + $selectors_parsed[ $selector ] = $classes; } $stylesheet[] = array( - $classes, - implode( '', $selectors ), + $selectors_parsed, $declaration, ); } else { From ae68eee389c1138de15b15be579337745ba5d11a Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 1 Apr 2018 14:05:52 -0700 Subject: [PATCH 20/31] Prevent :not() pseudo-selectors from tree shaking classes erroneously --- includes/sanitizers/class-amp-style-sanitizer.php | 6 +++--- tests/test-amp-style-sanitizer.php | 8 +++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 0ed12d39edd..1bb98b7edc2 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -603,9 +603,9 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $selectors_parsed = array(); foreach ( $selectors as $selector ) { - $classes = array(); - // @todo There could be a false negative here, such as when :not(body.home) is used. Nots should be first removed prior to matching. - if ( preg_match_all( '/(?<=\.)([a-zA-Z0-9_-]+)/', $selector, $matches ) ) { + $classes = array(); + $reduced_selector = preg_replace( '/:not\(.+?\)/', '', $selector ); // Remove :not() to eliminate false negatives. + if ( preg_match_all( '/(?<=\.)([a-zA-Z0-9_-]+)/', $reduced_selector, $matches ) ) { $classes = $matches[0]; } $selectors_parsed[ $selector ] = $classes; diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index fa662adf6b3..15779591deb 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -203,6 +203,12 @@ public function get_link_and_style_test_data() { 'body{color:red;}', ), ), + 'style_with_not_selectors' => array( + '

Hello

', + array( + 'body.foo:not(.bar) > p{color:blue;}body.foo:not(.bar) p:not(.baz){color:green;}body.foo p{color:yellow;}', + ), + ), ); } @@ -233,7 +239,7 @@ public function test_link_and_style_elements( $source, $expected_stylesheets ) { if ( false === strpos( $expected_stylesheet, '{' ) ) { $this->assertContains( $expected_stylesheet, $actual_stylesheets[ $i ] ); } else { - $this->assertStringStartsWith( $expected_stylesheet, $actual_stylesheets[ $i ] ); + $this->assertEquals( $expected_stylesheet, $actual_stylesheets[ $i ] ); } $this->assertContains( $expected_stylesheet, $sanitized_html ); } From 6d4778e3200143d86e50f676bb07c817517dfb5f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 1 Apr 2018 20:52:59 -0700 Subject: [PATCH 21/31] Remove attribute selectors prior to searching for class names to prevent false matches --- includes/sanitizers/class-amp-style-sanitizer.php | 10 ++++++++-- tests/test-amp-style-sanitizer.php | 6 ++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 1bb98b7edc2..2f0d72e4c38 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -603,8 +603,14 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $selectors_parsed = array(); foreach ( $selectors as $selector ) { - $classes = array(); - $reduced_selector = preg_replace( '/:not\(.+?\)/', '', $selector ); // Remove :not() to eliminate false negatives. + $classes = array(); + + // Remove :not() to eliminate false negatives, such as with `body:not(.title-tagline-hidden) .site-branding-text`. + $reduced_selector = preg_replace( '/:not\(.+?\)/', '', $selector ); + + // Remove attribute selectors to eliminate false negative, such as with `.social-navigation a[href*="example.com"]:before`. + $reduced_selector = preg_replace( '/\[\w.*?\]/', '', $reduced_selector ); + if ( preg_match_all( '/(?<=\.)([a-zA-Z0-9_-]+)/', $reduced_selector, $matches ) ) { $classes = $matches[0]; } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 15779591deb..cc5a839b6e9 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -209,6 +209,12 @@ public function get_link_and_style_test_data() { 'body.foo:not(.bar) > p{color:blue;}body.foo:not(.bar) p:not(.baz){color:green;}body.foo p{color:yellow;}', ), ), + 'style_with_attribute_selectors' => array( + '', + array( + '.social-navigation a[href*="example.com"]{color:red;}', + ), + ), ); } From 2693f7e94e97fc689b06c497782fab79b92bbe82 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 1 Apr 2018 20:54:06 -0700 Subject: [PATCH 22/31] Simplify accounting for used class names when tree shaking --- includes/sanitizers/class-amp-style-sanitizer.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 2f0d72e4c38..f43fb82457c 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -509,11 +509,10 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { foreach ( $parsed['stylesheet'] as $stylesheet_part ) { if ( is_array( $stylesheet_part ) ) { list( $selectors_parsed, $declaration_block ) = $stylesheet_part; - - $selectors = array(); if ( $should_tree_shake ) { + $selectors = array(); foreach ( $selectors_parsed as $selector => $class_names ) { - if ( count( $class_names ) === count( array_intersect( $class_names, $this->used_class_names ) ) ) { + if ( 0 === count( array_diff( $class_names, $this->used_class_names ) ) ) { // If all class names are used in the doc. $selectors[] = $selector; } } From faef934162822255655b9ee18775cbecf7d73365 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 1 Apr 2018 21:40:58 -0700 Subject: [PATCH 23/31] Make font-face source urls non-relative; replace data: URLs with (assumed) file URLs --- .../sanitizers/class-amp-style-sanitizer.php | 138 ++++++++++++++++++ tests/test-amp-style-sanitizer.php | 32 +++- 2 files changed, 167 insertions(+), 3 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index f43fb82457c..1ba87a000d9 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -15,6 +15,8 @@ use \Sabberworm\CSS\RuleSet\AtRuleSet; use \Sabberworm\CSS\Property\Import; use \Sabberworm\CSS\CSSList\AtRuleBlockList; +use \Sabberworm\CSS\Value\RuleValueList; +use \Sabberworm\CSS\Value\URL; /** * Class AMP_Style_Sanitizer @@ -447,6 +449,8 @@ private function process_link_element( DOMElement $element ) { 'allowed_at_rules' => $this->style_custom_cdata_spec['css_spec']['allowed_at_rules'], 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], 'class_selector_tree_shaking' => true, + 'stylesheet_url' => $href, + 'stylesheet_path' => $css_file_path, ) ); // Skip if surpasses max size. @@ -484,6 +488,8 @@ private function process_link_element( DOMElement $element ) { * @type string[] $property_whitelist Exclusively-allowed properties. * @type string[] $property_blacklist Disallowed properties. * @type bool $convert_width_to_max_width Convert width to max-width. + * @type string $stylesheet_url Original URL for stylesheet when originating via link (or @import?). + * @type string $stylesheet_path Original filesystem path for stylesheet when originating via link (or @import?). * } * @return string Processed stylesheet. */ @@ -558,6 +564,8 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { ), 'property_whitelist' => array(), 'validate_keyframes' => false, + 'stylesheet_url' => null, + 'stylesheet_path' => null, ), $options ); @@ -763,6 +771,10 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l } } + if ( $ruleset instanceof AtRuleSet && 'font-face' === $ruleset->atRuleName() ) { + $this->process_font_face_at_rule( $ruleset, $options ); + } + $validation_errors = array_merge( $validation_errors, $this->transform_important_qualifiers( $ruleset, $css_list ) @@ -787,6 +799,132 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l return $validation_errors; } + /** + * Process @font-face by making src URLs non-relative and converting data: URLs into (assumed) file URLs. + * + * @since 1.0 + * + * @param AtRuleSet $ruleset Ruleset for @font-face. + * @param array $options Options. + */ + private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { + $src_properties = $ruleset->getRules( 'src' ); + if ( empty( $src_properties ) ) { + return; + } + + $base_url = null; + if ( ! empty( $options['stylesheet_url'] ) ) { + $base_url = preg_replace( ':[^/]+(\?.*)?(#.*)?$:', '', $options['stylesheet_url'] ); + } + + /** + * Convert a relative path URL into a real/absolute path. + * + * @param URL $url Stylesheet URL. + */ + $real_path = function ( URL $url ) use ( $base_url ) { + if ( empty( $base_url ) ) { + return; + } + $parsed_url = wp_parse_url( $url->getURL()->getString() ); + if ( ! empty( $parsed_url['host'] ) || empty( $parsed_url['path'] ) || '/' === substr( $parsed_url['path'], 0, 1 ) ) { + return; + } + $relative_url = preg_replace( '#^\./#', '', $url->getURL()->getString() ); + $url->getURL()->setString( $base_url . $relative_url ); + }; + + foreach ( $src_properties as $src_property ) { + $value = $src_property->getValue(); + if ( $value instanceof URL ) { + $real_path( $value ); + } elseif ( $value instanceof RuleValueList ) { + /* + * The CSS Parser parses a src such as: + * + * url(data:application/font-woff;...) format('woff'), + * url('Genericons.ttf') format('truetype'), + * url('Genericons.svg#genericonsregular') format('svg') + * + * As a list of components consisting of: + * + * URL, + * RuleValueList( CSSFunction, URL ), + * RuleValueList( CSSFunction, URL ), + * CSSFunction + * + * Clearly the components here are not logically grouped. So the first step is to fix the order. + */ + $sources = array(); + foreach ( $value->getListComponents() as $component ) { + if ( $component instanceof RuleValueList ) { + $subcomponents = $component->getListComponents(); + $subcomponent = array_shift( $subcomponents ); + if ( $subcomponent ) { + if ( empty( $sources ) ) { + $sources[] = array( $subcomponent ); + } else { + $sources[ count( $sources ) - 1 ][] = $subcomponent; + } + } + foreach ( $subcomponents as $subcomponent ) { + $sources[] = array( $subcomponent ); + } + } else { + if ( empty( $sources ) ) { + $sources[] = array( $component ); + } else { + $sources[ count( $sources ) - 1 ][] = $component; + } + } + } + + /** + * Source URL lists. + * + * @var URL[] $source_file_urls + * @var URL[] $source_data_urls + */ + $source_file_urls = array(); + $source_data_urls = array(); + foreach ( $sources as $i => $source ) { + if ( $source[0] instanceof URL ) { + if ( 'data:' === substr( $source[0]->getURL()->getString(), 0, 5 ) ) { + $source_data_urls[ $i ] = $source[0]; + } else { + $real_path( $source[0] ); + $source_file_urls[ $i ] = $source[0]; + } + } + } + + // Convert data: URLs into regular URLs, assuming there will be a file present (e.g. woff fonts in core themes). + if ( empty( $source_file_urls ) ) { + continue; + } + $source_file_url = current( $source_file_urls ); + foreach ( $source_data_urls as $i => $data_url ) { + $mime_type = strtok( substr( $data_url->getURL()->getString(), 5 ), ';' ); + if ( ! $mime_type ) { + continue; + } + $extension = preg_replace( ':.+/(.+-)?:', '', $mime_type ); + $guessed_url = preg_replace( + ':(?<=\.)\w+(\?.*)?(#.*)?$:', // Match the file extension in the URL. + $extension, + $source_file_url->getURL()->getString(), + 1, + $count + ); + if ( $count ) { + $data_url->getURL()->setString( $guessed_url ); + } + } + } + } + } + /** * Process CSS keyframes. * diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index cc5a839b6e9..a03468959da 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -198,19 +198,19 @@ public function get_link_and_style_test_data() { ), ), 'style_with_no_head' => array( - 'Not good!', + 'Not good!', array( 'body{color:red;}', ), ), 'style_with_not_selectors' => array( - '

Hello

', + '

Hello

', array( 'body.foo:not(.bar) > p{color:blue;}body.foo:not(.bar) p:not(.baz){color:green;}body.foo p{color:yellow;}', ), ), 'style_with_attribute_selectors' => array( - '', + '', array( '.social-navigation a[href*="example.com"]{color:red;}', ), @@ -251,6 +251,32 @@ public function test_link_and_style_elements( $source, $expected_stylesheets ) { } } + /** + * Test handling of stylesheets with @font-face that have data: url source. + * + * Also confirm that class-based tree-shaking is working. + * + * @covers AMP_Style_Sanitizer::process_font_face_at_rule() + */ + public function test_font_data_url_handling() { + $html = ''; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $dom = AMP_DOM_Utils::get_dom( $html ); + + $sanitizer = new AMP_Style_Sanitizer( $dom, array( + 'use_document_element' => true, + ) ); + $sanitizer->sanitize(); + AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); + $actual_stylesheets = array_values( $sanitizer->get_stylesheets() ); + $this->assertCount( 1, $actual_stylesheets ); + $stylesheet = $actual_stylesheets[0]; + $this->assertContains( 'dashicons.woff") format("woff")', $stylesheet ); + $this->assertNotContains( 'data:application/font-woff;', $stylesheet ); + $this->assertContains( '.dashicons{', $stylesheet ); + $this->assertContains( '.dashicons-admin-appearance:before{', $stylesheet ); + $this->assertNotContains( '.dashicons-format-chat:before', $stylesheet ); + } + /** * Get amp-keyframe styles. * From cb3cc2cb64202ef0799962657840d3dc24568c38 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 1 Apr 2018 21:58:42 -0700 Subject: [PATCH 24/31] Use expiring transients when no external object cache present --- includes/sanitizers/class-amp-style-sanitizer.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 1ba87a000d9..9d7e6ea5223 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -499,10 +499,19 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { $cache_key = md5( $stylesheet . serialize( $options ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize $cache_group = 'amp-parsed-stylesheet-v1'; - $parsed = wp_cache_get( $cache_key, $cache_group ); + if ( wp_using_ext_object_cache() ) { + $parsed = wp_cache_get( $cache_key, $cache_group ); + } else { + $parsed = get_transient( $cache_key . $cache_group ); + } if ( ! $parsed || ! isset( $parsed['stylesheet'] ) || ! is_array( $parsed['stylesheet'] ) ) { $parsed = $this->parse_stylesheet( $stylesheet, $options ); - wp_cache_set( $cache_key, $parsed, $cache_group ); + if ( wp_using_ext_object_cache() ) { + wp_cache_set( $cache_key, $parsed, $cache_group ); + } else { + // The expiration is to ensure transient doesn't stick around forever since no LRU flushing like with external object cache. + set_transient( $cache_key . $cache_group, $parsed, MONTH_IN_SECONDS ); + } } if ( ! empty( $this->args['validation_error_callback'] ) && ! empty( $parsed['validation_errors'] ) ) { From 346eae25673edcc26b2f4cfac4f035c376303f2b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 1 Apr 2018 23:44:06 -0700 Subject: [PATCH 25/31] Handle selector specificity variability; give high specificity to rules from style attrs * Reduce length of dummy ID from "FK_ID" to just "_". * Ensure !important added to selectors on html element are handled. --- .../sanitizers/class-amp-style-sanitizer.php | 32 +++++++++++-- tests/test-amp-style-sanitizer.php | 47 +++++++++++++------ 2 files changed, 61 insertions(+), 18 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 9d7e6ea5223..114e0877d58 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -1025,8 +1025,34 @@ private function transform_important_qualifiers( RuleSet $ruleset, CSSList $css_ $important_ruleset = clone $ruleset; $important_ruleset->setSelectors( array_map( + /** + * Modify selectors to be more specific to roughly match the effect of !important. + * + * @link https://github.com/ampproject/ampstart/blob/4c21d69afdd07b4c60cd190937bda09901955829/tools/replace-important/lib/index.js#L88-L109 + * + * @param Selector $old_selector Original selector. + * @return Selector The new more-specific selector. + */ function( Selector $old_selector ) { - return new Selector( ':root:not(#FK_ID) ' . $old_selector->getSelector() ); + $specific = ':not(#_)'; // Here "_" is just a short single-char ID. + + $selector_mod = str_repeat( $specific, floor( $old_selector->getSpecificity() / 100 ) ); + if ( $old_selector->getSpecificity() % 100 > 0 ) { + $selector_mod .= $specific; + } + if ( $old_selector->getSpecificity() % 10 > 0 ) { + $selector_mod .= $specific; + } + + $new_selector = $old_selector->getSelector(); + + // Amend the selector mod to the first element in selector if it is already the root; otherwise add new root ancestor. + if ( preg_match( '/^\s*(html|:root)\b/i', $new_selector, $matches ) ) { + $new_selector = substr( $new_selector, 0, strlen( $matches[0] ) ) . $selector_mod . substr( $new_selector, strlen( $matches[0] ) ); + } else { + $new_selector = sprintf( ':root%s %s', $selector_mod, $new_selector ); + } + return new Selector( $new_selector ); }, $ruleset->getSelectors() ) ); @@ -1056,9 +1082,9 @@ private function collect_inline_styles( $element ) { return; } - // @todo Use hash from resulting processed CSS so that we can potentially re-use? We need to use the hash of the original rules as the cache key. $class = 'amp-wp-' . substr( md5( $style_attribute->nodeValue ), 0, 7 ); - $rule = sprintf( '.%s { %s }', $class, $style_attribute->nodeValue ); + $root = ':root' . str_repeat( ':not(#_)', 5 ); // @todo The correctness of using "5" should be validated. + $rule = sprintf( '%s .%s { %s }', $root, $class, $style_attribute->nodeValue ); $hash = md5( $rule ); $rule = $this->process_stylesheet( $rule, $style_attribute, array( 'convert_width_to_max_width' => true, diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index a03468959da..4ca65ee2554 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -29,7 +29,7 @@ public function get_body_style_attribute_data() { 'This is green.', 'This is green.', array( - '.amp-wp-bb01159{color:#0f0;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-bb01159{color:#0f0;}', ), ), @@ -37,7 +37,7 @@ public function get_body_style_attribute_data() { 'This is green.', 'This is green.', array( - '.amp-wp-0837823{color:#0f0;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-0837823{color:#0f0;}', ), ), @@ -45,7 +45,7 @@ public function get_body_style_attribute_data() { 'This is green.', 'This is green.', array( - '.amp-wp-c71affe{color:#0f0;background-color:#000;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-c71affe{color:#0f0;background-color:#000;}', ), ), @@ -53,7 +53,7 @@ public function get_body_style_attribute_data() { '
', '
', array( - '.amp-wp-343bce0{max-width:300px;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-343bce0{max-width:300px;}', ), ), @@ -61,7 +61,7 @@ public function get_body_style_attribute_data() { 'Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.', 'Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.', array( - '.amp-wp-224b51a{display:none;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-224b51a{display:none;}', ), ), @@ -69,7 +69,7 @@ public function get_body_style_attribute_data() { '!important is converted.', '!important is converted.', array( - '.amp-wp-6a75598{padding:1px;outline:3px;}:root:not(#FK_ID) .amp-wp-6a75598{margin:2px;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-6a75598{padding:1px;outline:3px;}:root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-6a75598{margin:2px;}', ), ), @@ -77,7 +77,7 @@ public function get_body_style_attribute_data() { '!important is converted.', '!important is converted.', array( - ':root:not(#FK_ID) .amp-wp-952600b{color:red;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-952600b{color:red;}', ), ), @@ -85,7 +85,7 @@ public function get_body_style_attribute_data() { '!important is converted.', '!important is converted.', array( - ':root:not(#FK_ID) .amp-wp-1e2bfaa{color:red;background:blue;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-1e2bfaa{color:red;background:blue;}', ), ), @@ -93,8 +93,8 @@ public function get_body_style_attribute_data() { 'This is red.', 'This is red.', array( - '.amp-wp-bb01159{color:#0f0;}', - '.amp-wp-cc68ddc{color:#f00;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-bb01159{color:#0f0;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cc68ddc{color:#f00;}', ), ), @@ -102,7 +102,7 @@ public function get_body_style_attribute_data() { '
', '
', array( - '.amp-wp-2864855{background:#000;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-2864855{background:#000;}', ), ), @@ -110,7 +110,7 @@ public function get_body_style_attribute_data() { '
bold!
', '
bold!
', array( - 'div > span{font-style:italic;}@media screen and ( max-width: 640px ){div > span{font-style:normal;}:root:not(#FK_ID) div > span{font-weight:normal;}}:root:not(#FK_ID) div > span{font-weight:bold;}', + 'div > span{font-style:italic;}@media screen and ( max-width: 640px ){div > span{font-style:normal;}:root:not(#_):not(#_) div > span{font-weight:normal;}}:root:not(#_):not(#_) div > span{font-weight:bold;}', ), ), @@ -143,6 +143,16 @@ public function get_body_style_attribute_data() { '@media screen and ( max-width: 640px ){body{font-size:small;}}@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2");}@supports (display: grid){div{display:grid;}}@-moz-keyframes appear{from{opacity:0;}to{opacity:1;}}@keyframes appear{from{opacity:0;}to{opacity:1;}}', ), ), + + 'selector_specificity' => array( + '
onetwothree
', + '
onetwothree
', + array( + ':root:not(#_) #child{color:red;}:root:not(#_):not(#_) #parent #child{color:pink;}:root:not(#_) .foo{color:blue;}:root:not(#_):not(#_) #me .foo{color:green;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-64b4fd4{color:yellow;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-ab79d9e{color:purple;}', + ), + ), ); } @@ -179,9 +189,9 @@ public function get_link_and_style_test_data() { 'multiple_amp_custom_and_other_styles' => array( '', array( - ':root:not(#FK_ID) b{color:red;}', + ':root:not(#_):not(#_) b{color:red;}', 'i{color:blue;}', - 'u{color:green;}:root:not(#FK_ID) u{text-decoration:underline;}', + 'u{color:green;}:root:not(#_):not(#_) u{text-decoration:underline;}', 's{color:yellow;}', ), ), @@ -194,7 +204,7 @@ public function get_link_and_style_test_data() { 'strong.before-dashicon', '.dashicons-dashboard:before', 'strong.after-dashicon', - ':root:not(#FK_ID) s{color:yellow;}', + ':root:not(#_):not(#_) s{color:yellow;}', ), ), 'style_with_no_head' => array( @@ -215,6 +225,13 @@ public function get_link_and_style_test_data() { '.social-navigation a[href*="example.com"]{color:red;}', ), ), + 'style_on_root_element' => array( + 'Hi', + array( + 'html:not(#_):not(#_){background-color:blue;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-10b06ba{color:red;}', + ), + ), ); } From 14c22c2f25490df3d1899fa1f71e969574372d90 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 2 Apr 2018 00:28:03 -0700 Subject: [PATCH 26/31] Convert all URLs (including background-image) to have absolute paths --- .../sanitizers/class-amp-style-sanitizer.php | 215 ++++++++++-------- tests/test-amp-style-sanitizer.php | 22 ++ 2 files changed, 140 insertions(+), 97 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 114e0877d58..4a9a7013089 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -586,6 +586,18 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $css_parser = new Sabberworm\CSS\Parser( $stylesheet_string, $parser_settings ); $css_document = $css_parser->parse(); + if ( ! empty( $options['stylesheet_url'] ) ) { + $this->real_path_urls( + array_filter( + $css_document->getAllValues(), + function ( $value ) { + return $value instanceof URL; + } + ), + $options['stylesheet_url'] + ); + } + $validation_errors = $this->process_css_list( $css_document, $options ); $output_format = Sabberworm\CSS\OutputFormat::createCompact(); @@ -736,6 +748,39 @@ private function process_css_list( CSSList $css_list, $options ) { return $validation_errors; } + /** + * Convert URLs in to non-relative real-paths. + * + * @param URL[] $urls URLs. + * @param string $stylesheet_url Stylesheet URL. + */ + private function real_path_urls( $urls, $stylesheet_url ) { + $base_url = preg_replace( ':[^/]+(\?.*)?(#.*)?$:', '', $stylesheet_url ); + if ( empty( $base_url ) ) { + return; + } + foreach ( $urls as $url ) { + $url_string = $url->getURL()->getString(); + if ( 'data:' === substr( $url_string, 0, 5 ) ) { + continue; + } + + $parsed_url = wp_parse_url( $url_string ); + if ( ! empty( $parsed_url['host'] ) || empty( $parsed_url['path'] ) || '/' === substr( $parsed_url['path'], 0, 1 ) ) { + continue; + } + + $relative_url = preg_replace( '#^\./#', '', $url->getURL()->getString() ); + + $real_url = $base_url . $relative_url; + do { + $real_url = preg_replace( '#[^/]+/../#', '', $real_url, -1, $count ); + } while ( 0 !== $count ); + + $url->getURL()->setString( $real_url ); + } + } + /** * Process CSS rule set. * @@ -781,7 +826,7 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l } if ( $ruleset instanceof AtRuleSet && 'font-face' === $ruleset->atRuleName() ) { - $this->process_font_face_at_rule( $ruleset, $options ); + $this->process_font_face_at_rule( $ruleset ); } $validation_errors = array_merge( @@ -814,121 +859,97 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l * @since 1.0 * * @param AtRuleSet $ruleset Ruleset for @font-face. - * @param array $options Options. */ - private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { + private function process_font_face_at_rule( AtRuleSet $ruleset ) { $src_properties = $ruleset->getRules( 'src' ); if ( empty( $src_properties ) ) { return; } - $base_url = null; - if ( ! empty( $options['stylesheet_url'] ) ) { - $base_url = preg_replace( ':[^/]+(\?.*)?(#.*)?$:', '', $options['stylesheet_url'] ); - } - - /** - * Convert a relative path URL into a real/absolute path. - * - * @param URL $url Stylesheet URL. - */ - $real_path = function ( URL $url ) use ( $base_url ) { - if ( empty( $base_url ) ) { - return; - } - $parsed_url = wp_parse_url( $url->getURL()->getString() ); - if ( ! empty( $parsed_url['host'] ) || empty( $parsed_url['path'] ) || '/' === substr( $parsed_url['path'], 0, 1 ) ) { - return; - } - $relative_url = preg_replace( '#^\./#', '', $url->getURL()->getString() ); - $url->getURL()->setString( $base_url . $relative_url ); - }; - foreach ( $src_properties as $src_property ) { $value = $src_property->getValue(); - if ( $value instanceof URL ) { - $real_path( $value ); - } elseif ( $value instanceof RuleValueList ) { - /* - * The CSS Parser parses a src such as: - * - * url(data:application/font-woff;...) format('woff'), - * url('Genericons.ttf') format('truetype'), - * url('Genericons.svg#genericonsregular') format('svg') - * - * As a list of components consisting of: - * - * URL, - * RuleValueList( CSSFunction, URL ), - * RuleValueList( CSSFunction, URL ), - * CSSFunction - * - * Clearly the components here are not logically grouped. So the first step is to fix the order. - */ - $sources = array(); - foreach ( $value->getListComponents() as $component ) { - if ( $component instanceof RuleValueList ) { - $subcomponents = $component->getListComponents(); - $subcomponent = array_shift( $subcomponents ); - if ( $subcomponent ) { - if ( empty( $sources ) ) { - $sources[] = array( $subcomponent ); - } else { - $sources[ count( $sources ) - 1 ][] = $subcomponent; - } - } - foreach ( $subcomponents as $subcomponent ) { - $sources[] = array( $subcomponent ); - } - } else { + if ( ! ( $value instanceof RuleValueList ) ) { + continue; + } + + /* + * The CSS Parser parses a src such as: + * + * url(data:application/font-woff;...) format('woff'), + * url('Genericons.ttf') format('truetype'), + * url('Genericons.svg#genericonsregular') format('svg') + * + * As a list of components consisting of: + * + * URL, + * RuleValueList( CSSFunction, URL ), + * RuleValueList( CSSFunction, URL ), + * CSSFunction + * + * Clearly the components here are not logically grouped. So the first step is to fix the order. + */ + $sources = array(); + foreach ( $value->getListComponents() as $component ) { + if ( $component instanceof RuleValueList ) { + $subcomponents = $component->getListComponents(); + $subcomponent = array_shift( $subcomponents ); + if ( $subcomponent ) { if ( empty( $sources ) ) { - $sources[] = array( $component ); + $sources[] = array( $subcomponent ); } else { - $sources[ count( $sources ) - 1 ][] = $component; + $sources[ count( $sources ) - 1 ][] = $subcomponent; } } + foreach ( $subcomponents as $subcomponent ) { + $sources[] = array( $subcomponent ); + } + } else { + if ( empty( $sources ) ) { + $sources[] = array( $component ); + } else { + $sources[ count( $sources ) - 1 ][] = $component; + } } + } - /** - * Source URL lists. - * - * @var URL[] $source_file_urls - * @var URL[] $source_data_urls - */ - $source_file_urls = array(); - $source_data_urls = array(); - foreach ( $sources as $i => $source ) { - if ( $source[0] instanceof URL ) { - if ( 'data:' === substr( $source[0]->getURL()->getString(), 0, 5 ) ) { - $source_data_urls[ $i ] = $source[0]; - } else { - $real_path( $source[0] ); - $source_file_urls[ $i ] = $source[0]; - } + /** + * Source URL lists. + * + * @var URL[] $source_file_urls + * @var URL[] $source_data_urls + */ + $source_file_urls = array(); + $source_data_urls = array(); + foreach ( $sources as $i => $source ) { + if ( $source[0] instanceof URL ) { + if ( 'data:' === substr( $source[0]->getURL()->getString(), 0, 5 ) ) { + $source_data_urls[ $i ] = $source[0]; + } else { + $source_file_urls[ $i ] = $source[0]; } } + } - // Convert data: URLs into regular URLs, assuming there will be a file present (e.g. woff fonts in core themes). - if ( empty( $source_file_urls ) ) { + // Convert data: URLs into regular URLs, assuming there will be a file present (e.g. woff fonts in core themes). + if ( empty( $source_file_urls ) ) { + continue; + } + $source_file_url = current( $source_file_urls ); + foreach ( $source_data_urls as $i => $data_url ) { + $mime_type = strtok( substr( $data_url->getURL()->getString(), 5 ), ';' ); + if ( ! $mime_type ) { continue; } - $source_file_url = current( $source_file_urls ); - foreach ( $source_data_urls as $i => $data_url ) { - $mime_type = strtok( substr( $data_url->getURL()->getString(), 5 ), ';' ); - if ( ! $mime_type ) { - continue; - } - $extension = preg_replace( ':.+/(.+-)?:', '', $mime_type ); - $guessed_url = preg_replace( - ':(?<=\.)\w+(\?.*)?(#.*)?$:', // Match the file extension in the URL. - $extension, - $source_file_url->getURL()->getString(), - 1, - $count - ); - if ( $count ) { - $data_url->getURL()->setString( $guessed_url ); - } + $extension = preg_replace( ':.+/(.+-)?:', '', $mime_type ); + $guessed_url = preg_replace( + ':(?<=\.)\w+(\?.*)?(#.*)?$:', // Match the file extension in the URL. + $extension, + $source_file_url->getURL()->getString(), + 1, + $count + ); + if ( $count ) { + $data_url->getURL()->setString( $guessed_url ); } } } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 4ca65ee2554..d68e727fb48 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -294,6 +294,28 @@ public function test_font_data_url_handling() { $this->assertNotContains( '.dashicons-format-chat:before', $stylesheet ); } + /** + * Test handling of stylesheets with relative background-image URLs. + * + * @covers AMP_Style_Sanitizer::real_path_urls() + */ + public function test_relative_background_url_handling() { + $html = ''; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $dom = AMP_DOM_Utils::get_dom( $html ); + + $sanitizer = new AMP_Style_Sanitizer( $dom, array( + 'use_document_element' => true, + ) ); + $sanitizer->sanitize(); + AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); + $actual_stylesheets = array_values( $sanitizer->get_stylesheets() ); + $this->assertCount( 1, $actual_stylesheets ); + $stylesheet = $actual_stylesheets[0]; + + $this->assertNotContains( '../images/spinner', $stylesheet ); + $this->assertContains( sprintf( '.spinner{background-image:url("%s");', admin_url( 'images/spinner-2x.gif' ) ), $stylesheet ); + } + /** * Get amp-keyframe styles. * From bede9de9ff0b5e4e9eca36d2a6ca59d1dd33c09b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 5 Apr 2018 12:00:46 -0700 Subject: [PATCH 27/31] Prevent filtering selectors which reference dynamic elements --- .../sanitizers/class-amp-base-sanitizer.php | 2 +- .../sanitizers/class-amp-style-sanitizer.php | 45 ++++++++++++++++++- tests/test-amp-style-sanitizer.php | 18 ++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index 0fc9a697085..cd1a06d1aa8 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -55,7 +55,7 @@ abstract class AMP_Base_Sanitizer { * @type bool $allow_dirty_styles * @type bool $allow_dirty_scripts * @type bool $disable_invalid_removal - * @type callable $remove_invalid_callback + * @type callable $validation_error_callback * } */ protected $args; diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 4a9a7013089..24d016beaba 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -25,6 +25,33 @@ */ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { + /** + * Array of flags used to control sanitization. + * + * @var array { + * @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered. + * @type bool $use_document_element Whether the root of the document should be used rather than the body. + * @type bool $require_https_src Require HTTPS URLs. + * @type bool $allow_dirty_styles Allow dirty styles. This short-circuits the sanitize logic; it is used primarily in Customizer preview. + * @type callable $validation_error_callback Function to call when a validation error is encountered. + * } + */ + protected $args; + + /** + * Default args. + * + * @var array + */ + protected $DEFAULT_ARGS = array( + 'dynamic_element_selectors' => array( + 'amp-list', + 'amp-live-list', + '[submit-error]', + '[submit-success]', + ), + ); + /** * Styles. * @@ -520,6 +547,16 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { } } + $dynamic_selector_pattern = null; + if ( ! empty( $this->args['dynamic_element_selectors'] ) ) { + $dynamic_selector_pattern = '#' . implode( '|', array_map( + function( $selector ) { + return preg_quote( $selector, '#' ); + }, + $this->args['dynamic_element_selectors'] + ) ) . '#'; + } + $stylesheet = ''; foreach ( $parsed['stylesheet'] as $stylesheet_part ) { if ( is_array( $stylesheet_part ) ) { @@ -527,7 +564,13 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { if ( $should_tree_shake ) { $selectors = array(); foreach ( $selectors_parsed as $selector => $class_names ) { - if ( 0 === count( array_diff( $class_names, $this->used_class_names ) ) ) { // If all class names are used in the doc. + $should_include = ( + ( $dynamic_selector_pattern && preg_match( $dynamic_selector_pattern, $selector ) ) + || + // If all class names are used in the doc. + 0 === count( array_diff( $class_names, $this->used_class_names ) ) + ); + if ( $should_include ) { $selectors[] = $selector; } } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index d68e727fb48..52d37994d49 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -232,6 +232,24 @@ public function get_link_and_style_test_data() { ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-10b06ba{color:red;}', ), ), + 'styles_with_dynamic_elements' => array( + implode( '', array( + '', + '', + '', + '', + '', + '
', + '
  • Hello
', + ' ', + '', + ) ), + array( + 'form [submit-success] b,div[submit-failure] b{color:green;}', + 'amp-live-list li .highlighted{background:yellow;}', + 'body amp-list .portland{color:blue;}', + ), + ), ); } From adb1fb14a1331ca3baffb8cc6b00eca4a500c481 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 5 Apr 2018 20:24:14 -0700 Subject: [PATCH 28/31] Skip removing style rules when the total CSS does not exceed max_bytes --- .../sanitizers/class-amp-base-sanitizer.php | 1 + .../sanitizers/class-amp-style-sanitizer.php | 446 +++++++++++------- includes/utils/class-amp-validation-utils.php | 4 +- tests/test-amp-style-sanitizer.php | 6 +- 4 files changed, 275 insertions(+), 182 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index cd1a06d1aa8..1c2cb183be6 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -130,6 +130,7 @@ public function get_scripts() { * Return array of values that would be valid as an HTML `style` attribute. * * @since 0.4 + * @deprecated As of 1.0, use get_stylesheets(). * * @return array[][] Mapping of CSS selectors to arrays of properties. */ diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 24d016beaba..76fd590e60b 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -29,6 +29,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { * Array of flags used to control sanitization. * * @var array { + * @type string $remove_unused_rules Enum 'never', 'sometimes' (default), 'always'. If total CSS is greater than max_bytes, whether to strip selectors (and then empty rules) when they are not found to be used in doc. A validation error will be emitted when stripping happens since it is not completely safe in the case of dynamic content. * @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered. * @type bool $use_document_element Whether the root of the document should be used rather than the body. * @type bool $require_https_src Require HTTPS URLs. @@ -44,6 +45,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { * @var array */ protected $DEFAULT_ARGS = array( + 'remove_unused_rules' => 'sometimes', 'dynamic_element_selectors' => array( 'amp-list', 'amp-live-list', @@ -52,20 +54,10 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { ), ); - /** - * Styles. - * - * List of CSS styles in HTML content of DOMDocument ($this->dom). - * - * @since 0.4 - * @var array[] - */ - private $styles = array(); - /** * Stylesheets. * - * Values are the CSS stylesheets. Keys are MD5 hashes of the stylesheets + * Values are the CSS stylesheets. Keys are MD5 hashes of the stylesheets, * * @since 0.7 * @var string[] @@ -73,13 +65,18 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { private $stylesheets = array(); /** - * Current amp-custom CSS size. + * List of stylesheet parts prior to selector/rule removal (tree shaking). * - * Sum of CSS located in $styles and $stylesheets. + * Keys are MD5 hashes of stylesheets. * - * @var int + * @since 1.0 + * @var array[] { + * @type array $stylesheet Array of stylesheet chunked, with declaration blocks being represented as arrays. + * @type DOMElement|DOMAttr $node Origin for styles. + * @type array $sources + * } */ - private $current_custom_size = 0; + private $pending_stylesheets = array(); /** * Spec for style[amp-custom] cdata. @@ -104,14 +101,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $keyframes_stylesheets = array(); - /** - * Current amp-keyframes CSS size. - * - * @since 1.0 - * @var int - */ - private $current_keyframes_size = 0; - /** * Spec for style[amp-keyframes] cdata. * @@ -210,14 +199,12 @@ public function __construct( DOMDocument $dom, array $args = array() ) { * Get list of CSS styles in HTML content of DOMDocument ($this->dom). * * @since 0.4 + * @deprecated As of 1.0, use get_stylesheets(). * * @return array[] Mapping CSS selectors to array of properties, or mapping of keys starting with 'stylesheet:' with value being the stylesheet. */ public function get_styles() { - if ( ! $this->did_convert_elements ) { - return array(); - } - return $this->styles; + return array(); } /** @@ -227,7 +214,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( $this->stylesheets, parent::get_stylesheets() ); + return $this->stylesheets; } /** @@ -237,11 +224,14 @@ public function get_stylesheets() { * @return array Used class names. */ private function get_used_class_names() { - $classes = ' '; - foreach ( $this->xpath->query( '//*/@class' ) as $class_attribute ) { - $classes .= ' ' . $class_attribute->nodeValue; + if ( empty( $this->used_class_names ) ) { + $classes = ' '; + foreach ( $this->xpath->query( '//*/@class' ) as $class_attribute ) { + $classes .= ' ' . $class_attribute->nodeValue; + } + $this->used_class_names = array_unique( array_filter( preg_split( '/\s+/', trim( $classes ) ) ) ); } - return array_unique( array_filter( preg_split( '/\s+/', trim( $classes ) ) ) ); + return $this->used_class_names; } /** @@ -257,7 +247,7 @@ public function sanitize() { return; } - $this->used_class_names = $this->get_used_class_names(); + $this->parse_css_duration = 0.0; /* * Note that xpath is used to query the DOM so that the link and style elements will be @@ -297,37 +287,10 @@ public function sanitize() { $this->collect_inline_styles( $element ); } - $this->finalize_amp_keyframes_styles(); + $this->finalize_styles(); $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', '' ); - $head = $this->dom->getElementsByTagName( 'head' )->item( 0 ); - if ( ! $head ) { - $head = $this->dom->createElement( 'head' ); - $this->dom->documentElement->insertBefore( $head, $this->dom->documentElement->firstChild ); - } - $head->appendChild( $this->amp_custom_style_element ); - } - - $css = implode( '', $this->get_stylesheets() ); - - /* - * 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 ) ); - } - if ( $this->parse_css_duration > 0.0 ) { AMP_Response_Headers::send_server_timing( 'amp_parse_css', $this->parse_css_duration, 'AMP Parse CSS' ); } @@ -394,33 +357,22 @@ private function process_style_element( DOMElement $element ) { $stylesheet = trim( $element->textContent ); $cdata_spec = $is_keyframes ? $this->style_keyframes_cdata_spec : $this->style_custom_cdata_spec; if ( $stylesheet ) { - $stylesheet = $this->process_stylesheet( $stylesheet, $element, array( - 'allowed_at_rules' => $cdata_spec['css_spec']['allowed_at_rules'], - 'property_whitelist' => $cdata_spec['css_spec']['allowed_declarations'], - 'validate_keyframes' => $cdata_spec['css_spec']['validate_keyframes'], - 'class_selector_tree_shaking' => ! $cdata_spec['css_spec']['validate_keyframes'], - ) ); - } - // Remove if surpasses max size. - $length = strlen( $stylesheet ); - $current_size = $is_keyframes ? $this->current_keyframes_size : $this->current_custom_size; - if ( $current_size + $length > $cdata_spec['max_bytes'] ) { - $this->remove_invalid_child( $element, array( - /* translators: %d is the number of bytes over the limit */ - 'message' => sprintf( __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), ( $current_size + $length ) - $cdata_spec['max_bytes'] ), + $stylesheet = $this->process_stylesheet( $stylesheet, $element, array( + 'allowed_at_rules' => $cdata_spec['css_spec']['allowed_at_rules'], + 'property_whitelist' => $cdata_spec['css_spec']['allowed_declarations'], + 'validate_keyframes' => $cdata_spec['css_spec']['validate_keyframes'], ) ); - return; - } - - $hash = md5( $stylesheet ); - if ( $is_keyframes ) { - $this->keyframes_stylesheets[ $hash ] = $stylesheet; - $this->current_keyframes_size += $length; - } else { - $this->stylesheets[ $hash ] = $stylesheet; - $this->current_custom_size += $length; + $pending_stylesheet = array( + 'keyframes' => $is_keyframes, + 'stylesheet' => $stylesheet, + 'node' => $element, + ); + if ( ! empty( $this->args['validation_error_callback'] ) ) { + $pending_stylesheet['sources'] = AMP_Validation_Utils::locate_sources( $element ); // Needed because node is removed below. + } + $this->pending_stylesheets[] = $pending_stylesheet; } if ( $element->hasAttribute( 'amp-custom' ) ) { @@ -473,26 +425,21 @@ private function process_link_element( DOMElement $element ) { } $stylesheet = $this->process_stylesheet( $stylesheet, $element, array( - 'allowed_at_rules' => $this->style_custom_cdata_spec['css_spec']['allowed_at_rules'], - 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], - 'class_selector_tree_shaking' => true, - 'stylesheet_url' => $href, - 'stylesheet_path' => $css_file_path, + 'allowed_at_rules' => $this->style_custom_cdata_spec['css_spec']['allowed_at_rules'], + 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], + 'stylesheet_url' => $href, + 'stylesheet_path' => $css_file_path, ) ); - // Skip if surpasses max size. - $length = strlen( $stylesheet ); - if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { - $this->remove_invalid_child( $element, array( - /* translators: %d is the number of bytes over the limit */ - 'message' => sprintf( __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), ( $this->current_custom_size + $length ) - $this->style_custom_cdata_spec['max_bytes'] ), - ) ); - return; + $pending_stylesheet = array( + 'keyframes' => false, + 'stylesheet' => $stylesheet, + 'node' => $element, + ); + if ( ! empty( $this->args['validation_error_callback'] ) ) { + $pending_stylesheet['sources'] = AMP_Validation_Utils::locate_sources( $element ); // Needed because node is removed below. } - $hash = md5( $stylesheet ); - - $this->stylesheets[ $hash ] = $stylesheet; - $this->current_custom_size += $length; + $this->pending_stylesheets[] = $pending_stylesheet; // Remove now that styles have been processed. $element->parentNode->removeChild( $element ); @@ -517,13 +464,18 @@ private function process_link_element( DOMElement $element ) { * @type bool $convert_width_to_max_width Convert width to max-width. * @type string $stylesheet_url Original URL for stylesheet when originating via link (or @import?). * @type string $stylesheet_path Original filesystem path for stylesheet when originating via link (or @import?). + * @type array $allowed_at_rules Allowed @-rules. + * @type bool $validate_keyframes Whether keyframes should be validated. * } - * @return string Processed stylesheet. + * @return array Processed stylesheet parts. */ private function process_stylesheet( $stylesheet, $node, $options = array() ) { - $should_tree_shake = ! empty( $options['class_selector_tree_shaking'] ); - unset( $options['class_selector_tree_shaking'] ); - $cache_key = md5( $stylesheet . serialize( $options ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $cache_impacting_options = wp_array_slice_assoc( + $options, + array( 'property_whitelist', 'property_blacklist', 'convert_width_to_max_width', 'stylesheet_url', 'allowed_at_rules' ) + ); + + $cache_key = md5( $stylesheet . serialize( $cache_impacting_options ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize $cache_group = 'amp-parsed-stylesheet-v1'; if ( wp_using_ext_object_cache() ) { @@ -547,45 +499,7 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { } } - $dynamic_selector_pattern = null; - if ( ! empty( $this->args['dynamic_element_selectors'] ) ) { - $dynamic_selector_pattern = '#' . implode( '|', array_map( - function( $selector ) { - return preg_quote( $selector, '#' ); - }, - $this->args['dynamic_element_selectors'] - ) ) . '#'; - } - - $stylesheet = ''; - foreach ( $parsed['stylesheet'] as $stylesheet_part ) { - if ( is_array( $stylesheet_part ) ) { - list( $selectors_parsed, $declaration_block ) = $stylesheet_part; - if ( $should_tree_shake ) { - $selectors = array(); - foreach ( $selectors_parsed as $selector => $class_names ) { - $should_include = ( - ( $dynamic_selector_pattern && preg_match( $dynamic_selector_pattern, $selector ) ) - || - // If all class names are used in the doc. - 0 === count( array_diff( $class_names, $this->used_class_names ) ) - ); - if ( $should_include ) { - $selectors[] = $selector; - } - } - } else { - $selectors = array_keys( $selectors_parsed ); - } - if ( ! empty( $selectors ) ) { - $stylesheet .= implode( ',', $selectors ) . $declaration_block; - } - } else { - $stylesheet .= $stylesheet_part; - } - } - - return $stylesheet; + return $parsed['stylesheet']; } /** @@ -1146,33 +1060,31 @@ private function collect_inline_styles( $element ) { return; } - $class = 'amp-wp-' . substr( md5( $style_attribute->nodeValue ), 0, 7 ); - $root = ':root' . str_repeat( ':not(#_)', 5 ); // @todo The correctness of using "5" should be validated. - $rule = sprintf( '%s .%s { %s }', $root, $class, $style_attribute->nodeValue ); - $hash = md5( $rule ); - $rule = $this->process_stylesheet( $rule, $style_attribute, array( - 'convert_width_to_max_width' => true, - 'allowed_at_rules' => array(), - 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], - 'class_selector_tree_shaking' => false, + $class = 'amp-wp-' . substr( md5( $style_attribute->nodeValue ), 0, 7 ); + $root = ':root' . str_repeat( ':not(#_)', 5 ); // @todo The correctness of using "5" should be validated. + $rule = sprintf( '%s .%s { %s }', $root, $class, $style_attribute->nodeValue ); + + $stylesheet = $this->process_stylesheet( $rule, $style_attribute, array( + 'convert_width_to_max_width' => true, + 'allowed_at_rules' => array(), + 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], ) ); - $length = strlen( $rule ); - if ( 0 === $length ) { + if ( empty( $stylesheet ) ) { $element->removeAttribute( 'style' ); return; } - if ( $this->current_custom_size + $length > $this->style_custom_cdata_spec['max_bytes'] ) { - $this->remove_invalid_attribute( $element, $style_attribute, array( - /* translators: %d is the number of bytes over the limit */ - 'message' => sprintf( __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), ( $this->current_custom_size + $length ) - $this->style_custom_cdata_spec['max_bytes'] ), - ) ); - return; + $pending_stylesheet = array( + 'stylesheet' => $stylesheet, + 'node' => $element, + 'keyframes' => false, + ); + if ( ! empty( $this->args['validation_error_callback'] ) ) { + $pending_stylesheet['sources'] = AMP_Validation_Utils::locate_sources( $element ); // Needed because node is removed below. } - $this->current_custom_size += $length; - $this->stylesheets[ $hash ] = $rule; + $this->pending_stylesheets[] = $pending_stylesheet; $element->removeAttribute( 'style' ); if ( $element->hasAttribute( 'class' ) ) { @@ -1183,32 +1095,206 @@ private function collect_inline_styles( $element ) { } /** - * Finalize style[amp-keyframes] elements. + * Finalize stylesheets for style[amp-custom] and style[amp-keyframes] elements. * - * Combine all amp-keyframe elements and enforce that it is at the end of the body. + * Concatenate all pending stylesheets, remove unused rules if necessary, and add to style elements in doc. + * Combine all amp-keyframe styles and add them to the end of the body. * * @since 1.0 * @see https://www.ampproject.org/docs/fundamentals/spec#keyframes-stylesheet */ - private function finalize_amp_keyframes_styles() { - if ( empty( $this->keyframes_stylesheets ) ) { + private function finalize_styles() { + + $stylesheet_sets = array( + 'custom' => array( + 'total_size' => 0, + 'cdata_spec' => $this->style_custom_cdata_spec, + 'pending_stylesheets' => array(), + 'final_stylesheets' => array(), + 'remove_unused_rules' => $this->args['remove_unused_rules'], + ), + 'keyframes' => array( + 'total_size' => 0, + 'cdata_spec' => $this->style_keyframes_cdata_spec, + 'pending_stylesheets' => array(), + 'final_stylesheets' => array(), + 'remove_unused_rules' => 'never', // Not relevant. + ), + ); + + // Divide pending stylesheet between custom and keyframes, and calculate size of each. + while ( ! empty( $this->pending_stylesheets ) ) { + $pending_stylesheet = array_shift( $this->pending_stylesheets ); + + $set_name = ! empty( $pending_stylesheet['keyframes'] ) ? 'keyframes' : 'custom'; + $size = 0; + foreach ( $pending_stylesheet['stylesheet'] as $part ) { + if ( is_string( $part ) ) { + $size += strlen( $part ); + } elseif ( is_array( $part ) ) { + $size += strlen( implode( ',', array_keys( $part[0] ) ) ); // Selectors. + $size += strlen( $part[1] ); // Declaration block. + } + } + $stylesheet_sets[ $set_name ]['total_size'] += $size; + $stylesheet_sets[ $set_name ]['pending_stylesheets'][] = $pending_stylesheet; + } + + // Process the pending stylesheets. + foreach ( array_keys( $stylesheet_sets ) as $set_name ) { + $stylesheet_sets[ $set_name ] = $this->finalize_stylesheet_set( $stylesheet_sets[ $set_name ] ); + } + + $this->stylesheets = $stylesheet_sets['custom']['final_stylesheets']; + + // If we're not working with the document element (e.g. for legacy post templates) then there is nothing left to do. + if ( empty( $this->args['use_document_element'] ) ) { return; } - $body = $this->dom->getElementsByTagName( 'body' )->item( 0 ); - if ( ! $body ) { - if ( ! empty( $this->args['validation_error_callback'] ) ) { - call_user_func( $this->args['validation_error_callback'], array( - 'code' => 'missing_body_element', - 'message' => __( 'amp-keyframes must be last child of body element.', 'amp' ), - ) ); + // Add style[amp-custom] to document. + if ( ! empty( $stylesheet_sets['custom']['final_stylesheets'] ) ) { + + // Ensure style[amp-custom] is present in the document. + if ( ! $this->amp_custom_style_element ) { + $this->amp_custom_style_element = $this->dom->createElement( 'style' ); + $this->amp_custom_style_element->setAttribute( 'amp-custom', '' ); + $head = $this->dom->getElementsByTagName( 'head' )->item( 0 ); + if ( ! $head ) { + $head = $this->dom->createElement( 'head' ); + $this->dom->documentElement->insertBefore( $head, $this->dom->documentElement->firstChild ); + } + $head->appendChild( $this->amp_custom_style_element ); + } + + $css = implode( '', $stylesheet_sets['custom']['final_stylesheets'] ); + + /* + * 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 ) ); + } + + // Add style[amp-keyframes] to document. + if ( ! empty( $stylesheet_sets['keyframes']['final_stylesheets'] ) ) { + $body = $this->dom->getElementsByTagName( 'body' )->item( 0 ); + if ( ! $body ) { + if ( ! empty( $this->args['validation_error_callback'] ) ) { + call_user_func( $this->args['validation_error_callback'], array( + 'code' => 'missing_body_element', + 'message' => __( 'amp-keyframes must be last child of body element.', 'amp' ), + ) ); + } + } else { + $style_element = $this->dom->createElement( 'style' ); + $style_element->setAttribute( 'amp-keyframes', '' ); + $style_element->appendChild( $this->dom->createTextNode( implode( '', $stylesheet_sets['keyframes']['final_stylesheets'] ) ) ); + $body->appendChild( $style_element ); + } + } + } + + /** + * Finalize a stylesheet set (amp-custom or amp-keyframes). + * + * @since 1.0 + * + * @param array $stylesheet_set Stylesheet set. + * @return array Finalized stylesheet set. + */ + private function finalize_stylesheet_set( $stylesheet_set ) { + $is_too_much_css = $stylesheet_set['total_size'] > $stylesheet_set['cdata_spec']['max_bytes']; + $should_tree_shake = ( + 'always' === $stylesheet_set['remove_unused_rules'] || ( + $is_too_much_css + && + 'sometimes' === $stylesheet_set['remove_unused_rules'] + ) + ); + + if ( $is_too_much_css && $should_tree_shake && ! empty( $this->args['validation_error_callback'] ) ) { + call_user_func( $this->args['validation_error_callback'], array( + 'code' => 'removed_unused_css_rules', + 'message' => __( 'Too much CSS is enqueued and so seemingly irrelevant rules have been removed.', 'amp' ), + ) ); + } + + $dynamic_selector_pattern = null; + if ( $should_tree_shake && ! empty( $this->args['dynamic_element_selectors'] ) ) { + $dynamic_selector_pattern = '#' . implode( '|', array_map( + function( $selector ) { + return preg_quote( $selector, '#' ); + }, + $this->args['dynamic_element_selectors'] + ) ) . '#'; + } + + $final_size = 0; + foreach ( $stylesheet_set['pending_stylesheets'] as $pending_stylesheet ) { + $stylesheet = ''; + foreach ( $pending_stylesheet['stylesheet'] as $stylesheet_part ) { + if ( is_string( $stylesheet_part ) ) { + $stylesheet .= $stylesheet_part; + } else { + list( $selectors_parsed, $declaration_block ) = $stylesheet_part; + if ( $should_tree_shake ) { + $selectors = array(); + foreach ( $selectors_parsed as $selector => $class_names ) { + $should_include = ( + ( $dynamic_selector_pattern && preg_match( $dynamic_selector_pattern, $selector ) ) + || + // If all class names are used in the doc. + 0 === count( array_diff( $class_names, $this->get_used_class_names() ) ) + ); + if ( $should_include ) { + $selectors[] = $selector; + } + } + } else { + $selectors = array_keys( $selectors_parsed ); + } + if ( ! empty( $selectors ) ) { + $stylesheet .= implode( ',', $selectors ) . $declaration_block; + } + } + } + + // Skip considering stylesheet if an identical one has already been processed. + $hash = md5( $stylesheet ); + if ( isset( $stylesheet_set['final_stylesheets'][ $hash ] ) ) { + continue; + } + + // Report validation error if size is now too big. + $sheet_size = strlen( $stylesheet ); + if ( $final_size + $sheet_size > $stylesheet_set['cdata_spec']['max_bytes'] ) { + if ( ! empty( $this->args['validation_error_callback'] ) ) { + $validation_error = array( + 'code' => 'excessive_css', + 'message' => sprintf( + /* translators: %d is the number of bytes over the limit */ + __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), + ( $final_size + $sheet_size ) - $stylesheet_set['cdata_spec']['max_bytes'] + ), + ); + if ( isset( $pending_stylesheet['sources'] ) ) { + $validation_error['sources'] = $pending_stylesheet['sources']; + } + call_user_func( $this->args['validation_error_callback'], $validation_error ); + } + } else { + $final_size += $sheet_size; + + $stylesheet_set['final_stylesheets'][ $hash ] = $stylesheet; } - return; } - $style_element = $this->dom->createElement( 'style' ); - $style_element->setAttribute( 'amp-keyframes', '' ); - $style_element->appendChild( $this->dom->createTextNode( implode( '', $this->keyframes_stylesheets ) ) ); - $body->appendChild( $style_element ); + return $stylesheet_set; } } diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index eff7c4cef3e..9982f4f3839 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -411,7 +411,9 @@ public static function add_validation_error( array $data ) { $node = $data['node']; unset( $data['node'] ); $data['node_name'] = $node->nodeName; - $data['sources'] = self::locate_sources( $node ); + if ( ! isset( $data['sources'] ) ) { + $data['sources'] = self::locate_sources( $node ); + } if ( $node->parentNode ) { $data['parent_name'] = $node->parentNode->nodeName; } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 52d37994d49..893a0e5c283 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -265,6 +265,7 @@ public function test_link_and_style_elements( $source, $expected_stylesheets ) { $sanitizer = new AMP_Style_Sanitizer( $dom, array( 'use_document_element' => true, + 'remove_unused_rules' => 'always', ) ); $sanitizer->sanitize(); @@ -299,6 +300,7 @@ public function test_font_data_url_handling() { $sanitizer = new AMP_Style_Sanitizer( $dom, array( 'use_document_element' => true, + 'remove_unused_rules' => 'always', ) ); $sanitizer->sanitize(); AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); @@ -386,7 +388,9 @@ public function get_keyframe_data() { public function test_keyframe_sanitizer( $source, $expected = null ) { $expected = isset( $expected ) ? $expected : $source; $dom = AMP_DOM_Utils::get_dom_from_content( $source ); - $sanitizer = new AMP_Style_Sanitizer( $dom ); + $sanitizer = new AMP_Style_Sanitizer( $dom, array( + 'use_document_element' => true, + ) ); $sanitizer->sanitize(); $content = AMP_DOM_Utils::get_content_from_dom( $dom ); $content = preg_replace( '#\s+(?=@keyframes)#', '', $content ); From a8fddf22d1a28bfbee4858d98adc71e38873c4ce Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 5 Apr 2018 22:13:17 -0700 Subject: [PATCH 29/31] Verify existence of font file source for data: URL --- .../sanitizers/class-amp-style-sanitizer.php | 74 +++++++++++-------- tests/test-amp-style-sanitizer.php | 17 +++-- tests/test-class-amp-response-headers.php | 4 +- 3 files changed, 55 insertions(+), 40 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 76fd590e60b..9a2419d0f96 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -297,52 +297,57 @@ public function sanitize() { } /** - * Generates an enqueued style's fully-qualified file path. + * Generate a URL'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 $url The file URL. + * @param string[] $allowed_extensions Allowed file extensions. * @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error. */ - public function get_validated_css_file_path( $src ) { + public function get_validated_url_file_path( $url, $allowed_extensions = array() ) { $needs_base_url = ( - ! is_bool( $src ) + ! is_bool( $url ) && - ! preg_match( '|^(https?:)?//|', $src ) + ! preg_match( '|^(https?:)?//|', $url ) && - ! ( $this->content_url && 0 === strpos( $src, $this->content_url ) ) + ! ( $this->content_url && 0 === strpos( $url, $this->content_url ) ) ); if ( $needs_base_url ) { - $src = $this->base_url . $src; + $url = $this->base_url . $url; } // 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 ) ); + $url = preg_replace( ':[\?#].*$:', '', $url ); + + // Validate file extensions. + if ( ! empty( $allowed_extensions ) ) { + $pattern = sprintf( '/\.(%s)$/i', implode( '|', $allowed_extensions ) ); + if ( ! preg_match( $pattern, $url ) ) { + /* translators: %s is the file URL */ + return new WP_Error( 'amp_disallowed_file_extension', sprintf( __( 'Skipped file which does not have an allowed file extension (%s).', 'amp' ), $url ) ); + } } $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 ); + $file_path = null; + if ( 0 === strpos( $url, $content_url ) ) { + $file_path = WP_CONTENT_DIR . substr( $url, strlen( $content_url ) - 1 ); + } elseif ( 0 === strpos( $url, $includes_url ) ) { + $file_path = ABSPATH . WPINC . substr( $url, strlen( $includes_url ) - 1 ); + } elseif ( 0 === strpos( $url, $admin_url ) ) { + $file_path = ABSPATH . 'wp-admin' . substr( $url, 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 ) ); + if ( ! $file_path || false !== strpos( '../', $file_path ) || 0 !== validate_file( $file_path ) || ! file_exists( $file_path ) ) { + /* translators: %s is file URL */ + return new WP_Error( 'amp_file_path_not_found', sprintf( __( 'Unable to locate filesystem path for %s.', 'amp' ), $url ) ); } - return $css_path; + return $file_path; } /** @@ -401,7 +406,7 @@ private function process_link_element( DOMElement $element ) { return; } - $css_file_path = $this->get_validated_css_file_path( $href ); + $css_file_path = $this->get_validated_url_file_path( $href, array( 'css', 'less', 'scss', 'sass' ) ); if ( is_wp_error( $css_file_path ) ) { $this->remove_invalid_child( $element, array( 'message' => $css_file_path->get_error_message(), @@ -539,7 +544,7 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $stylesheet = array(); $validation_errors = array(); try { - $parser_settings = Sabberworm\CSS\Settings::create()->withMultibyteSupport( false ); + $parser_settings = Sabberworm\CSS\Settings::create(); $css_parser = new Sabberworm\CSS\Parser( $stylesheet_string, $parser_settings ); $css_document = $css_parser->parse(); @@ -783,7 +788,7 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l } if ( $ruleset instanceof AtRuleSet && 'font-face' === $ruleset->atRuleName() ) { - $this->process_font_face_at_rule( $ruleset ); + $this->process_font_face_at_rule( $ruleset, $options ); } $validation_errors = array_merge( @@ -816,8 +821,9 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l * @since 1.0 * * @param AtRuleSet $ruleset Ruleset for @font-face. + * @param array $options Options. */ - private function process_font_face_at_rule( AtRuleSet $ruleset ) { + private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $src_properties = $ruleset->getRules( 'src' ); if ( empty( $src_properties ) ) { return; @@ -905,9 +911,18 @@ private function process_font_face_at_rule( AtRuleSet $ruleset ) { 1, $count ); - if ( $count ) { - $data_url->getURL()->setString( $guessed_url ); + if ( 1 !== $count ) { + continue; + } + + // Ensure file exists. + $path = $this->get_validated_url_file_path( $guessed_url ); + if ( is_wp_error( $path ) ) { + continue; } + + $data_url->getURL()->setString( $guessed_url ); + break; } } } @@ -965,7 +980,6 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) { * @since 1.0 * @see https://www.npmjs.com/package/replace-important * @see https://www.ampproject.org/docs/fundamentals/spec#important - * @todo Further tailor for specificity. See https://github.com/ampproject/ampstart/blob/4c21d69afdd07b4c60cd190937bda09901955829/tools/replace-important/lib/index.js#L87-L126. * * @param RuleSet|DeclarationBlock $ruleset Rule set. * @param CSSList $css_list CSS List. diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 893a0e5c283..a49b11ac8d2 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -430,33 +430,34 @@ public function get_stylesheet_urls() { admin_url( 'css/common.css' ), ABSPATH . 'wp-admin/css/common.css', ), - 'amp_css_bad_file_extension' => array( + 'amp_disallowed_file_extension' => array( content_url( 'themes/twentyseventeen/index.php' ), null, - 'amp_css_bad_file_extension', + 'amp_disallowed_file_extension', ), - 'amp_css_path_not_found' => array( + 'amp_file_path_not_found' => array( content_url( 'themes/twentyseventeen/404.css' ), null, - 'amp_css_path_not_found', + 'amp_file_path_not_found', ), ); } /** - * Tests get_validated_css_file_path. + * Tests get_validated_url_file_path. * * @dataProvider get_stylesheet_urls - * @covers AMP_Style_Sanitizer::get_validated_css_file_path() + * @covers AMP_Style_Sanitizer::get_validated_url_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 ) { + public function test_get_validated_url_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 ); + $actual = $sanitizer->get_validated_url_file_path( $source, array( 'css' ) ); if ( isset( $error_code ) ) { $this->assertInstanceOf( 'WP_Error', $actual ); $this->assertEquals( $error_code, $actual->get_error_code() ); diff --git a/tests/test-class-amp-response-headers.php b/tests/test-class-amp-response-headers.php index 5257ffb45e0..a6408dd10e2 100644 --- a/tests/test-class-amp-response-headers.php +++ b/tests/test-class-amp-response-headers.php @@ -1,13 +1,13 @@ Date: Thu, 5 Apr 2018 23:00:14 -0700 Subject: [PATCH 30/31] Add tests for validation errors raised during style sanitization --- .../sanitizers/class-amp-style-sanitizer.php | 15 ++--- tests/test-amp-style-sanitizer.php | 67 +++++++++++++++---- 2 files changed, 57 insertions(+), 25 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 9a2419d0f96..fda02ad269e 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -71,9 +71,10 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { * * @since 1.0 * @var array[] { - * @type array $stylesheet Array of stylesheet chunked, with declaration blocks being represented as arrays. - * @type DOMElement|DOMAttr $node Origin for styles. - * @type array $sources + * @type array $stylesheet Array of stylesheet chunked, with declaration blocks being represented as arrays. + * @type DOMElement|DOMAttr $node Origin for styles. + * @type array $sources Sources for the node. + * @type bool $keyframes Whether an amp-keyframes. * } */ private $pending_stylesheets = array(); @@ -93,14 +94,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $amp_custom_style_element; - /** - * The found style[amp-keyframe] stylesheets. - * - * @since 1.0 - * @var string[] - */ - private $keyframes_stylesheets = array(); - /** * Spec for style[amp-keyframes] cdata. * diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index a49b11ac8d2..2b0e66cb767 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -120,12 +120,14 @@ public function get_body_style_attribute_data() { array( 'button{font-weight:bold;}@media screen{button{font-weight:bold;}}', ), + array( 'illegal_css_property', 'illegal_css_property', 'illegal_css_property', 'illegal_css_property' ), ), 'illegal_at_rule_in_style_attribute' => array( 'Parse error.', 'Parse error.', array(), + array( 'css_parse_error' ), ), 'illegal_at_rules_removed' => array( @@ -134,6 +136,7 @@ public function get_body_style_attribute_data() { array( 'body{color:black;}', ), + array( 'illegal_css_at_rule', 'illegal_css_at_rule', 'illegal_css_at_rule', 'illegal_css_at_rule', 'illegal_css_at_rule' ), ), 'allowed_at_rules_retained' => array( @@ -163,11 +166,19 @@ public function get_body_style_attribute_data() { * @param string $source Source. * @param string $expected_content Expected content. * @param string $expected_stylesheets Expected stylesheets. + * @param array $expected_errors Expected error codes. */ - public function test_body_style_attribute_sanitizer( $source, $expected_content, $expected_stylesheets ) { + public function test_body_style_attribute_sanitizer( $source, $expected_content, $expected_stylesheets, $expected_errors = array() ) { $dom = AMP_DOM_Utils::get_dom_from_content( $source ); - $sanitizer = new AMP_Style_Sanitizer( $dom ); + $error_codes = array(); + $args = array( + 'validation_error_callback' => function( $error ) use ( &$error_codes ) { + $error_codes[] = $error['code']; + }, + ); + + $sanitizer = new AMP_Style_Sanitizer( $dom, $args ); $sanitizer->sanitize(); // Test content. @@ -177,6 +188,8 @@ public function test_body_style_attribute_sanitizer( $source, $expected_content, // Test stylesheet. $this->assertEquals( $expected_stylesheets, array_values( $sanitizer->get_stylesheets() ) ); + + $this->assertEquals( $expected_errors, $error_codes ); } /** @@ -194,6 +207,7 @@ public function get_link_and_style_test_data() { 'u{color:green;}:root:not(#_):not(#_) u{text-decoration:underline;}', 's{color:yellow;}', ), + array(), ), 'style_elements_with_link_elements' => array( sprintf( @@ -206,24 +220,28 @@ public function get_link_and_style_test_data() { 'strong.after-dashicon', ':root:not(#_):not(#_) s{color:yellow;}', ), + array(), ), 'style_with_no_head' => array( 'Not good!', array( 'body{color:red;}', ), + array(), ), 'style_with_not_selectors' => array( '

Hello

', array( 'body.foo:not(.bar) > p{color:blue;}body.foo:not(.bar) p:not(.baz){color:green;}body.foo p{color:yellow;}', ), + array(), ), 'style_with_attribute_selectors' => array( '', array( '.social-navigation a[href*="example.com"]{color:red;}', ), + array(), ), 'style_on_root_element' => array( 'Hi', @@ -231,6 +249,7 @@ public function get_link_and_style_test_data() { 'html:not(#_):not(#_){background-color:blue;}', ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-10b06ba{color:red;}', ), + array(), ), 'styles_with_dynamic_elements' => array( implode( '', array( @@ -249,6 +268,7 @@ public function get_link_and_style_test_data() { 'amp-live-list li .highlighted{background:yellow;}', 'body amp-list .portland{color:blue;}', ), + array(), ), ); } @@ -259,19 +279,24 @@ public function get_link_and_style_test_data() { * @dataProvider get_link_and_style_test_data * @param string $source Source. * @param array $expected_stylesheets Expected stylesheets. + * @param array $expected_errors Expected error codes. */ - public function test_link_and_style_elements( $source, $expected_stylesheets ) { + public function test_link_and_style_elements( $source, $expected_stylesheets, $expected_errors = array() ) { $dom = AMP_DOM_Utils::get_dom( $source ); - $sanitizer = new AMP_Style_Sanitizer( $dom, array( - 'use_document_element' => true, - 'remove_unused_rules' => 'always', - ) ); + $error_codes = array(); + $args = array( + 'use_document_element' => true, + 'remove_unused_rules' => 'always', + 'validation_error_callback' => function( $error ) use ( &$error_codes ) { + $error_codes[] = $error['code']; + }, + ); + + $sanitizer = new AMP_Style_Sanitizer( $dom, $args ); $sanitizer->sanitize(); - $whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, array( - 'use_document_element' => true, - ) ); + $whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, $args ); $whitelist_sanitizer->sanitize(); $sanitized_html = AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); @@ -285,6 +310,8 @@ public function test_link_and_style_elements( $source, $expected_stylesheets ) { } $this->assertContains( $expected_stylesheet, $sanitized_html ); } + + $this->assertEquals( $expected_errors, $error_codes ); } /** @@ -354,26 +381,31 @@ public function get_keyframe_data() { 'style_amp_keyframes' => array( '', '', + array(), ), 'style_amp_keyframes_max_overflow' => array( '', '', + array( 'excessive_css' ), ), 'style_amp_keyframes_last_child' => array( 'before between as after', 'before between as after', + array(), ), 'blacklisted_and_whitelisted_keyframe_properties' => array( '', '', + array( 'illegal_css_property', 'illegal_css_property', 'illegal_css_property' ), ), 'style_amp_keyframes_with_disallowed_rules' => array( '', '', + array( 'unrecognized_css', 'illegal_css_important', 'illegal_css_at_rule' ), ), ); } @@ -384,12 +416,17 @@ public function get_keyframe_data() { * @dataProvider get_keyframe_data * @param string $source Markup to process. * @param string $expected The markup to expect. + * @param array $expected_errors Expected error codes. */ - public function test_keyframe_sanitizer( $source, $expected = null ) { - $expected = isset( $expected ) ? $expected : $source; - $dom = AMP_DOM_Utils::get_dom_from_content( $source ); - $sanitizer = new AMP_Style_Sanitizer( $dom, array( + public function test_keyframe_sanitizer( $source, $expected = null, $expected_errors = array() ) { + $expected = isset( $expected ) ? $expected : $source; + $dom = AMP_DOM_Utils::get_dom_from_content( $source ); + $error_codes = array(); + $sanitizer = new AMP_Style_Sanitizer( $dom, array( 'use_document_element' => true, + 'validation_error_callback' => function( $error ) use ( &$error_codes ) { + $error_codes[] = $error['code']; + }, ) ); $sanitizer->sanitize(); $content = AMP_DOM_Utils::get_content_from_dom( $dom ); @@ -397,6 +434,8 @@ public function test_keyframe_sanitizer( $source, $expected = null ) { $content = preg_replace( '#\s+(?=)#', '', $content ); $content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content ); $this->assertEquals( $expected, $content ); + + $this->assertEquals( $expected_errors, $error_codes ); } /** From efd1965d47a0df9c9b6e7228e464ccf08d78c5a0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 5 Apr 2018 23:53:42 -0700 Subject: [PATCH 31/31] Add testing for remove_unused_rules=sometimes and excessive_css --- .../sanitizers/class-amp-style-sanitizer.php | 2 +- tests/test-amp-style-sanitizer.php | 93 ++++++++++++++++--- 2 files changed, 81 insertions(+), 14 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index fda02ad269e..8440c097369 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -1286,7 +1286,7 @@ function( $selector ) { 'code' => 'excessive_css', 'message' => sprintf( /* translators: %d is the number of bytes over the limit */ - __( 'Too much CSS enqueued (by %d bytes).', 'amp' ), + __( 'Too much CSS output (by %d bytes).', 'amp' ), ( $final_size + $sheet_size ) - $stylesheet_set['cdata_spec']['max_bytes'] ), ); diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 2b0e66cb767..5c7cd0a1afe 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -322,23 +322,89 @@ public function test_link_and_style_elements( $source, $expected_stylesheets, $e * @covers AMP_Style_Sanitizer::process_font_face_at_rule() */ public function test_font_data_url_handling() { - $html = ''; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet - $dom = AMP_DOM_Utils::get_dom( $html ); + $html = ''; + $html .= ''; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $html .= ''; - $sanitizer = new AMP_Style_Sanitizer( $dom, array( - 'use_document_element' => true, - 'remove_unused_rules' => 'always', + // Test with tree-shaking. + $dom = AMP_DOM_Utils::get_dom( $html ); + $error_codes = array(); + $sanitizer = new AMP_Style_Sanitizer( $dom, array( + 'use_document_element' => true, + 'remove_unused_rules' => 'always', + 'validation_error_callback' => function( $error ) use ( &$error_codes ) { + $error_codes[] = $error['code']; + }, ) ); $sanitizer->sanitize(); - AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement ); + $this->assertEquals( array(), $error_codes ); $actual_stylesheets = array_values( $sanitizer->get_stylesheets() ); $this->assertCount( 1, $actual_stylesheets ); - $stylesheet = $actual_stylesheets[0]; - $this->assertContains( 'dashicons.woff") format("woff")', $stylesheet ); - $this->assertNotContains( 'data:application/font-woff;', $stylesheet ); - $this->assertContains( '.dashicons{', $stylesheet ); - $this->assertContains( '.dashicons-admin-appearance:before{', $stylesheet ); - $this->assertNotContains( '.dashicons-format-chat:before', $stylesheet ); + $this->assertContains( 'dashicons.woff") format("woff")', $actual_stylesheets[0] ); + $this->assertNotContains( 'data:application/font-woff;', $actual_stylesheets[0] ); + $this->assertContains( '.dashicons{', $actual_stylesheets[0] ); + $this->assertContains( '.dashicons-admin-appearance:before{', $actual_stylesheets[0] ); + $this->assertNotContains( '.dashicons-format-chat:before', $actual_stylesheets[0] ); + + // Test with rule-removal not forced, since dashicons alone is not larger than 50KB. + $dom = AMP_DOM_Utils::get_dom( $html ); + $error_codes = array(); + $sanitizer = new AMP_Style_Sanitizer( $dom, array( + 'use_document_element' => true, + 'remove_unused_rules' => 'sometimes', + 'validation_error_callback' => function( $error ) use ( &$error_codes ) { + $error_codes[] = $error['code']; + }, + ) ); + $sanitizer->sanitize(); + $this->assertEquals( array(), $error_codes ); + $actual_stylesheets = array_values( $sanitizer->get_stylesheets() ); + $this->assertContains( 'dashicons.woff") format("woff")', $actual_stylesheets[0] ); + $this->assertNotContains( 'data:application/font-woff;', $actual_stylesheets[0] ); + $this->assertContains( '.dashicons,.dashicons-before:before{', $actual_stylesheets[0] ); + $this->assertContains( '.dashicons-admin-appearance:before{', $actual_stylesheets[0] ); + $this->assertContains( '.dashicons-format-chat:before', $actual_stylesheets[0] ); + } + + /** + * Test that auto-removal is performed when remove_unused_rules=sometimes (the default), and that excessive CSS will be removed entirely. + * + * @covers AMP_Style_Sanitizer::finalize_stylesheet_set() + */ + public function test_large_custom_css_and_rule_removal() { + $custom_max_size = null; + foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'style' ) as $spec_rule ) { + if ( isset( $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) && 'style amp-custom' === $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) { + $custom_max_size = $spec_rule[ AMP_Rule_Spec::CDATA ]['max_bytes']; + break; + } + } + $this->assertNotNull( $custom_max_size ); + + $html = ''; + $html .= ''; + $html .= ''; + $html .= '...'; + $dom = AMP_DOM_Utils::get_dom( $html ); + + $error_codes = array(); + $sanitizer = new AMP_Style_Sanitizer( $dom, array( + 'use_document_element' => true, + 'validation_error_callback' => function( $error ) use ( &$error_codes ) { + $error_codes[] = $error['code']; + }, + ) ); + $sanitizer->sanitize(); + + $this->assertEquals( + array( '.b{color:blue;}' ), + array_values( $sanitizer->get_stylesheets() ) + ); + + $this->assertEquals( + array( 'removed_unused_css_rules', 'excessive_css' ), + $error_codes + ); } /** @@ -369,13 +435,14 @@ public function test_relative_background_url_handling() { * @return array */ public function get_keyframe_data() { - $keyframes_max_size = 10; + $keyframes_max_size = null; foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'style' ) as $spec_rule ) { if ( isset( $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) && 'style[amp-keyframes]' === $spec_rule[ AMP_Rule_Spec::TAG_SPEC ]['spec_name'] ) { $keyframes_max_size = $spec_rule[ AMP_Rule_Spec::CDATA ]['max_bytes']; break; } } + $this->assertNotNull( $keyframes_max_size ); return array( 'style_amp_keyframes' => array(