From 50739207c5cb12f39231a0590e79f3a5e693d6c5 Mon Sep 17 00:00:00 2001 From: Miina Sikk Date: Thu, 24 May 2018 15:34:30 +0300 Subject: [PATCH 1/5] Try to fetch content of external stylesheets instead of removing. --- .../sanitizers/class-amp-style-sanitizer.php | 30 +++++++++++++--- tests/test-amp-style-sanitizer.php | 34 ++++++++++++++++--- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index f99ad2bceb2..d527a076d70 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -358,7 +358,7 @@ public function sanitize() { * * @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. + * @return string|WP_Error|boolean Style's absolute validated filesystem path, or WP_Error when error. */ public function get_validated_url_file_path( $url, $allowed_extensions = array() ) { $needs_base_url = ( @@ -391,8 +391,7 @@ public function get_validated_url_file_path( $url, $allowed_extensions = array() $url_host = wp_parse_url( $url, PHP_URL_HOST ); if ( ! in_array( $url_host, $allowed_hosts, true ) ) { - /* translators: %s is the file URL */ - return new WP_Error( 'disallowed_external_file_url', sprintf( __( 'Skipped file which does not have a recognized local host (%s).', 'amp' ), $url_host ) ); + return false; } // Validate file extensions. @@ -482,16 +481,37 @@ private function process_link_element( DOMElement $element ) { } $css_file_path = $this->get_validated_url_file_path( $href, array( 'css', 'less', 'scss', 'sass' ) ); + + $stylesheet = false; if ( is_wp_error( $css_file_path ) ) { $this->remove_invalid_child( $element, array( 'code' => $css_file_path->get_error_code(), 'message' => $css_file_path->get_error_message(), ) ); return; + } elseif ( false === $css_file_path ) { + + // If CSS local path was not received, this must be external URL. + $cache_key = md5( $normalized_font_href ); + $contents = get_transient( $cache_key ); + if ( false === $contents ) { + $r = wp_remote_get( $normalized_font_href ); + if ( 200 !== wp_remote_retrieve_response_code( $r ) ) { + $contents = new WP_Error( wp_remote_retrieve_response_code( $r ) ); + } else { + $contents = wp_remote_retrieve_body( $r ); + } + set_transient( $cache_key, $contents, MONTH_IN_SECONDS ); + } + if ( ! is_wp_error( $contents ) ) { + $stylesheet = $contents; + } + } else { + + // Load the CSS from the filesystem. + $stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. } - // Load the CSS from the filesystem. - $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' ), diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 7726fba5a8d..eb6d3fe7594 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -524,6 +524,32 @@ public function test_relative_background_url_handling() { $this->assertContains( sprintf( '.spinner{background-image:url("%s")', admin_url( 'images/spinner-2x.gif' ) ), $stylesheet ); } + /** + * Test handling external stylesheet. + */ + public function test_external_stylesheet_handling() { + $href = 'https://www.w3schools.com/css/mystyle.css'; + $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( 'background-color:lightblue', $stylesheet ); + + // Make sure the stylesheet got saved to cache. + $cache_key = md5( $href ); + $contents = get_transient( $cache_key ); + $this->assertNotFalse( $contents ); + $this->assertContains( 'lightblue', $contents ); + } + /** * Get amp-keyframe styles. * @@ -653,6 +679,10 @@ public function get_stylesheet_urls() { null, 'file_path_not_found', ), + 'amp_external_file' => array( + '//s.w.org/wp-includes/css/dashicons.css', + false, + ), ); } @@ -710,10 +740,6 @@ public function get_font_urls() { 'https://maxcdn.bootstrapcdn.com/font-awesome/123/css/font-awesome.min.css', array(), ), - 'bad_host' => array( - 'https://bad.example.com/font.css', - array( 'disallowed_external_file_url' ), - ), 'bad_ext' => array( home_url( '/bad.php' ), array( 'disallowed_file_extension' ), From a28f3d257e8211fadaae7cedaf90a98587a47bb5 Mon Sep 17 00:00:00 2001 From: Miina Sikk Date: Thu, 24 May 2018 15:39:25 +0300 Subject: [PATCH 2/5] Remove shortcircuiting allowed extensions check. --- includes/sanitizers/class-amp-style-sanitizer.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index d527a076d70..9e07be2f4b0 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -390,9 +390,6 @@ public function get_validated_url_file_path( $url, $allowed_extensions = array() ); $url_host = wp_parse_url( $url, PHP_URL_HOST ); - if ( ! in_array( $url_host, $allowed_hosts, true ) ) { - return false; - } // Validate file extensions. if ( ! empty( $allowed_extensions ) ) { @@ -403,6 +400,10 @@ public function get_validated_url_file_path( $url, $allowed_extensions = array() } } + if ( ! in_array( $url_host, $allowed_hosts, true ) ) { + return false; + } + $file_path = null; if ( 0 === strpos( $url, $content_url ) ) { $file_path = WP_CONTENT_DIR . substr( $url, strlen( $content_url ) - 1 ); From 7fc6bf678485934f89dc17609b0f15cc07017abb Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 29 May 2018 21:49:51 -0700 Subject: [PATCH 3/5] Prevent doing remote stylesheet request during unit tests Improve method for detecting whether cache is being used --- .../sanitizers/class-amp-style-sanitizer.php | 2 +- tests/test-amp-style-sanitizer.php | 58 ++++++++++++------- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 498c5f176c8..04fb6891f5d 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -410,7 +410,7 @@ public function sanitize() { * * @param string $url The file URL. * @param string[] $allowed_extensions Allowed file extensions. - * @return string|WP_Error|boolean Style's absolute validated filesystem path, or WP_Error when error. + * @return string|WP_Error|bool Style's absolute validated filesystem path, or WP_Error when error. */ public function get_validated_url_file_path( $url, $allowed_extensions = array() ) { $needs_base_url = ( diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 1d5ca7fb783..a1f0e031b7a 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -527,28 +527,46 @@ public function test_relative_background_url_handling() { /** * Test handling external stylesheet. + * + * @covers AMP_Style_Sanitizer::process_link_element() */ public function test_external_stylesheet_handling() { - $href = 'https://www.w3schools.com/css/mystyle.css'; - $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( 'background-color:lightblue', $stylesheet ); - - // Make sure the stylesheet got saved to cache. - $cache_key = md5( $href ); - $contents = get_transient( $cache_key ); - $this->assertNotFalse( $contents ); - $this->assertContains( 'lightblue', $contents ); + $test_case = $this; // For PHP 5.3. + $href = 'https://stylesheets.example.com/style.css'; + $count = 0; + add_filter( 'pre_http_request', function( $preempt, $request, $url ) use ( $href, &$count ) { + unset( $request ); + if ( $url === $href ) { + $count++; + $preempt = array( + 'response' => array( + 'code' => 200, + ), + 'body' => 'html { background-color:lightblue; }', + ); + } + return $preempt; + }, 10, 3 ); + + $sanitize_and_get_stylesheet = function() use ( $href, $test_case ) { + $html = sprintf( '', esc_url( $href ) ); // 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() ); + $test_case->assertCount( 1, $actual_stylesheets ); + return $actual_stylesheets[0]; + }; + + $this->assertEquals( 0, $count ); + $this->assertContains( 'background-color:lightblue', $sanitize_and_get_stylesheet() ); + $this->assertEquals( 1, $count ); + $this->assertContains( 'background-color:lightblue', $sanitize_and_get_stylesheet() ); + $this->assertEquals( 1, $count ); } /** From b22ce9321c8a25ab45f235230054fec39369e474 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 29 May 2018 22:37:55 -0700 Subject: [PATCH 4/5] Allow remote URLs for import rules --- .../sanitizers/class-amp-style-sanitizer.php | 95 ++++++++++++------- tests/test-amp-style-sanitizer.php | 74 ++++++--------- 2 files changed, 90 insertions(+), 79 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 04fb6891f5d..c152edab265 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -208,7 +208,6 @@ public static function get_css_parser_validation_error_codes() { 'illegal_css_property', 'removed_unused_css_rules', 'unrecognized_css', - 'disallowed_external_file_url', 'disallowed_file_extension', 'file_path_not_found', ); @@ -410,7 +409,7 @@ public function sanitize() { * * @param string $url The file URL. * @param string[] $allowed_extensions Allowed file extensions. - * @return string|WP_Error|bool Style's absolute validated filesystem path, or WP_Error when error. + * @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error. */ public function get_validated_url_file_path( $url, $allowed_extensions = array() ) { $needs_base_url = ( @@ -453,7 +452,8 @@ public function get_validated_url_file_path( $url, $allowed_extensions = array() } if ( ! in_array( $url_host, $allowed_hosts, true ) ) { - return false; + /* translators: %s is file URL */ + return new WP_Error( 'external_file_url', sprintf( __( 'URL is located on an external domain: %s.', 'amp' ), $url_host ) ); } $file_path = null; @@ -542,43 +542,34 @@ private function process_link_element( DOMElement $element ) { $href = $element->getAttribute( 'href' ); // Allow font URLs, including protocol-less URLs and recognized URLs that use HTTP instead of HTTPS. - $normalized_font_href = preg_replace( '#^(http:)?(?=//)#', 'https:', $href ); - if ( $this->allowed_font_src_regex && preg_match( $this->allowed_font_src_regex, $normalized_font_href ) ) { - if ( $href !== $normalized_font_href ) { - $element->setAttribute( 'href', $normalized_font_href ); + $normalized_url = preg_replace( '#^(http:)?(?=//)#', 'https:', $href ); + if ( $this->allowed_font_src_regex && preg_match( $this->allowed_font_src_regex, $normalized_url ) ) { + if ( $href !== $normalized_url ) { + $element->setAttribute( 'href', $normalized_url ); } return; } $css_file_path = $this->get_validated_url_file_path( $href, array( 'css', 'less', 'scss', 'sass' ) ); - $stylesheet = false; - if ( is_wp_error( $css_file_path ) ) { + if ( is_wp_error( $css_file_path ) && 'external_file_url' === $css_file_path->get_error_code() ) { + $contents = $this->fetch_external_stylesheet( $normalized_url ); + if ( is_wp_error( $contents ) ) { + $this->remove_invalid_child( $element, array( + 'code' => $css_file_path->get_error_code(), + 'message' => $css_file_path->get_error_message(), + ) ); + return; + } else { + $stylesheet = $contents; + } + } elseif ( is_wp_error( $css_file_path ) ) { $this->remove_invalid_child( $element, array( 'code' => $css_file_path->get_error_code(), 'message' => $css_file_path->get_error_message(), ) ); return; - } elseif ( false === $css_file_path ) { - - // If CSS local path was not received, this must be external URL. - $cache_key = md5( $normalized_font_href ); - $contents = get_transient( $cache_key ); - if ( false === $contents ) { - $r = wp_remote_get( $normalized_font_href ); - if ( 200 !== wp_remote_retrieve_response_code( $r ) ) { - $contents = new WP_Error( wp_remote_retrieve_response_code( $r ) ); - } else { - $contents = wp_remote_retrieve_body( $r ); - } - set_transient( $cache_key, $contents, MONTH_IN_SECONDS ); - } - if ( ! is_wp_error( $contents ) ) { - $stylesheet = $contents; - } } else { - - // Load the CSS from the filesystem. $stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. } @@ -617,6 +608,30 @@ private function process_link_element( DOMElement $element ) { $this->set_current_node( null ); } + /** + * Fetch external stylesheet. + * + * @param string $url External stylesheet URL. + * @return string|WP_Error Stylesheet contents or WP_Error. + */ + private function fetch_external_stylesheet( $url ) { + $cache_key = md5( $url ); + $contents = get_transient( $cache_key ); + if ( false === $contents ) { + $r = wp_remote_get( $url ); + if ( 200 !== wp_remote_retrieve_response_code( $r ) ) { + $contents = new WP_Error( + wp_remote_retrieve_response_code( $r ), + wp_remote_retrieve_response_message( $r ) + ); + } else { + $contents = wp_remote_retrieve_body( $r ); + } + set_transient( $cache_key, $contents, MONTH_IN_SECONDS ); + } + return $contents; + } + /** * Process stylesheet. * @@ -721,10 +736,25 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti } $this->processed_imported_stylesheet_urls[ $import_stylesheet_url ] = true; - // @todo Handle external per . $css_file_path = $this->get_validated_url_file_path( $import_stylesheet_url, array( 'css', 'less', 'scss', 'sass' ) ); - if ( is_wp_error( $css_file_path ) ) { + if ( is_wp_error( $css_file_path ) && 'external_file_url' === $css_file_path->get_error_code() ) { + $contents = $this->fetch_external_stylesheet( $import_stylesheet_url ); + if ( is_wp_error( $contents ) ) { + $error = array( + 'code' => $contents->get_error_code(), + 'message' => $contents->get_error_message(), + ); + $sanitized = $this->should_sanitize_validation_error( $error ); + if ( $sanitized ) { + $css_list->remove( $item ); + } + $results[] = compact( 'error', 'sanitized' ); + return $results; + } else { + $stylesheet = $contents; + } + } elseif ( is_wp_error( $css_file_path ) ) { $error = array( 'code' => $css_file_path->get_error_code(), 'message' => $css_file_path->get_error_message(), @@ -735,11 +765,10 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti } $results[] = compact( 'error', 'sanitized' ); return $results; + } else { + $stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. } - // Load the CSS from the filesystem. - $stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. - if ( $media_query ) { $stylesheet = sprintf( '@media %s { %s }', $media_query, $stylesheet ); } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index a1f0e031b7a..c355cfcdbfe 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -701,6 +701,7 @@ public function get_stylesheet_urls() { 'amp_external_file' => array( '//s.w.org/wp-includes/css/dashicons.css', false, + 'external_file_url', ), ); } @@ -846,65 +847,46 @@ public function test_font_urls( $url, $error_codes ) { } } - /** - * Get data for CSS @import test. - * - * @return array - */ - public function get_css_import_data() { - $css_url = admin_url( 'css/login.css' ); - $html = sprintf( '', $css_url ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet - $stylesheet_patterns = array( - '/' . preg_quote( 'input[type="checkbox"]:disabled' ) . '/', - '/' . preg_quote( 'body.rtl' ) . '/', - '/' . preg_quote( '.login .message' ) . '/', - '/^' . preg_quote( 'body{color:red}' ) . '$/', - ); - return array( - 'sanitized' => array( - $html, - $stylesheet_patterns, - '/^(?!@import)/', - true, - ), - 'unsanitized' => array( - $html, - $stylesheet_patterns, - '/^@import/', - false, - ), - ); - } - /** * Test CSS imports. * - * @dataProvider get_css_import_data * @covers AMP_Style_Sanitizer::parse_import_stylesheet() - * - * @param string $markup Markup. - * @param array $stylesheet_patterns Stylesheet patterns. - * @param string $style_pattern Style pattern for resulting style element. - * @param bool $sanitized Whether validation errors should be sanitized. */ - public function test_css_import( $markup, $stylesheet_patterns, $style_pattern, $sanitized ) { + public function test_css_import() { + $local_css_url = admin_url( 'css/login.css' ); + $import_css_url = 'https://stylesheets.example.com/style.css'; + $markup = sprintf( 'hello', $local_css_url, $import_css_url ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + + add_filter( 'pre_http_request', function( $preempt, $request, $url ) use ( $import_css_url ) { + unset( $request ); + if ( $url === $import_css_url ) { + $preempt = array( + 'response' => array( + 'code' => 200, + ), + 'body' => 'html { background-color:lightblue; }', + ); + } + return $preempt; + }, 10, 3 ); + $dom = AMP_DOM_Utils::get_dom( $markup ); $sanitizer = new AMP_Style_Sanitizer( $dom, array( - 'use_document_element' => true, - 'remove_unused_rules' => 'never', - 'validation_error_callback' => function() use ( $sanitized ) { - return $sanitized; - }, + 'use_document_element' => true, + 'remove_unused_rules' => 'never', ) ); $sanitizer->sanitize(); $stylesheets = array_values( $sanitizer->get_stylesheets() ); + $stylesheet_patterns = array( + '/' . preg_quote( 'input[type="checkbox"]:disabled' ) . '/', + '/' . preg_quote( 'body.rtl' ) . '/', + '/' . preg_quote( '.login .message' ) . '/', + '/^' . preg_quote( 'html{background-color:lightblue}' ) . '$/', + '/^' . preg_quote( 'body{color:red}' ) . '$/', + ); $this->assertCount( count( $stylesheet_patterns ), $stylesheets ); do { $this->assertRegExp( array_shift( $stylesheet_patterns ), array_shift( $stylesheets ) ); } while ( ! empty( $stylesheet_patterns ) ); - - $dom_xpath = new DOMXPath( $dom ); - $amp_custom = $dom_xpath->query( '//style[@amp-custom]' )->item( 0 ); - $this->assertRegExp( $style_pattern, $amp_custom->textContent ); } } From 727fe9f7925f8ee85a5e6c6388107701cddec73f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 29 May 2018 23:58:18 -0700 Subject: [PATCH 5/5] Refactor imported stylesheet by replacing import rule with parsed CSS list * Ensures that the imported stylesheet will be cached and that the imported stylesheet will not be ignored when cached. * Opt to base64-encode the calc() substitutions instead of using placeholders. --- composer.lock | 8 +- .../sanitizers/class-amp-style-sanitizer.php | 178 ++++++++++-------- tests/test-amp-style-sanitizer.php | 25 ++- 3 files changed, 114 insertions(+), 97 deletions(-) diff --git a/composer.lock b/composer.lock index fcf465756a0..aa74972e85e 100644 --- a/composer.lock +++ b/composer.lock @@ -12,12 +12,12 @@ "source": { "type": "git", "url": "https://github.com/xwp/PHP-CSS-Parser.git", - "reference": "69a4b9a002745a47120a1c02f2ece0859917694b" + "reference": "ccad0dcd0a234a49528e330f22f2332676b3bd95" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/xwp/PHP-CSS-Parser/zipball/69a4b9a002745a47120a1c02f2ece0859917694b", - "reference": "69a4b9a002745a47120a1c02f2ece0859917694b", + "url": "https://api.github.com/repos/xwp/PHP-CSS-Parser/zipball/ccad0dcd0a234a49528e330f22f2332676b3bd95", + "reference": "ccad0dcd0a234a49528e330f22f2332676b3bd95", "shasum": "" }, "require": { @@ -50,7 +50,7 @@ "support": { "source": "https://github.com/xwp/PHP-CSS-Parser/tree/master" }, - "time": "2018-05-27 00:18:56" + "time": "2018-05-30 06:47:52" } ], "packages-dev": [ diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index c152edab265..970e2de3c43 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -16,6 +16,7 @@ use \Sabberworm\CSS\CSSList\AtRuleBlockList; use \Sabberworm\CSS\Value\RuleValueList; use \Sabberworm\CSS\Value\URL; +use \Sabberworm\CSS\CSSList\Document; /** * Class AMP_Style_Sanitizer @@ -174,16 +175,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ private $current_sources; - /** - * Placeholders for calc() values that are temporarily removed from CSS since they cause parse errors. - * - * @since 1.0 - * @see AMP_Style_Sanitizer::add_calc_placeholders() - * - * @var array - */ - private $calc_placeholders = array(); - /** * Log of the stylesheet URLs that have been imported to guard against infinite loops. * @@ -692,7 +683,7 @@ private function process_stylesheet( $stylesheet, $options = array() ) { } if ( ! $parsed || ! isset( $parsed['stylesheet'] ) || ! is_array( $parsed['stylesheet'] ) ) { - $parsed = $this->parse_stylesheet( $stylesheet, $options ); + $parsed = $this->prepare_stylesheet( $stylesheet, $options ); if ( wp_using_ext_object_cache() ) { wp_cache_set( $cache_key, $parsed, $cache_group ); @@ -773,25 +764,28 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti $stylesheet = sprintf( '@media %s { %s }', $media_query, $stylesheet ); } - $stylesheet = $this->process_stylesheet( $stylesheet, 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'], - 'stylesheet_url' => $import_stylesheet_url, - 'stylesheet_path' => $css_file_path, - ) ); + $options['stylesheet_url'] = $import_stylesheet_url; - $pending_stylesheet = array( - 'keyframes' => false, - 'stylesheet' => $stylesheet, - 'node' => $this->current_node, - 'sources' => $this->current_sources, - 'imported' => $import_stylesheet_url, + $parsed_stylesheet = $this->parse_stylesheet( $stylesheet, $options ); + + $results = array_merge( + $results, + $parsed_stylesheet['validation_results'] ); - // Stylesheet will now be inlined, so @import can be removed. - $css_list->remove( $item ); + /** + * CSS Doc. + * + * @var Document $css_document + */ + $css_document = $parsed_stylesheet['css_document']; + + if ( ! empty( $parsed_stylesheet['css_document'] ) ) { + $css_list->replace( $item, $css_document->getContents() ); + } else { + $css_list->remove( $item ); + } - $this->pending_stylesheets[] = $pending_stylesheet; return $results; } @@ -805,11 +799,72 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti * @return array { * Parsed stylesheet. * + * @type Document $css_document CSS Document. + * @type array $validation_results Validation results, array containing arrays with error and sanitized keys. + * } + */ + private function parse_stylesheet( $stylesheet_string, $options ) { + $validation_results = array(); + $css_document = null; + try { + // Remove spaces from data URLs, which cause errors and PHP-CSS-Parser can't handle them. + $stylesheet_string = $this->remove_spaces_from_data_urls( $stylesheet_string ); + + // Find calc() functions and replace with placeholders since PHP-CSS-Parser can't handle them. + $stylesheet_string = $this->add_calc_placeholders( $stylesheet_string ); + + $parser_settings = Sabberworm\CSS\Settings::create(); + $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_results = array_merge( + $validation_results, + $this->process_css_list( $css_document, $options ) + ); + } catch ( Exception $exception ) { + $error = array( + 'code' => 'css_parse_error', + 'message' => $exception->getMessage(), + ); + + /* + * This is not a recoverable error, so sanitized here is just used to give user control + * over whether to proceed with serving this exception-raising stylesheet in AMP. + */ + $sanitized = $this->should_sanitize_validation_error( $error ); + + $validation_results[] = compact( 'error', 'sanitized' ); + } + return compact( 'validation_results', 'css_document' ); + } + + /** + * Prepare stylesheet. + * + * @since 1.0 + * + * @param string $stylesheet_string Stylesheet. + * @param array $options Options. See definition in \AMP_Style_Sanitizer::process_stylesheet(). + * @return array { + * Prepared stylesheet. + * * @type array $stylesheet Stylesheet parts, where arrays are tuples for declaration blocks. * @type array $validation_results Validation results, array containing arrays with error and sanitized keys. * } */ - private function parse_stylesheet( $stylesheet_string, $options = array() ) { + private function prepare_stylesheet( $stylesheet_string, $options = array() ) { $start_time = microtime( true ); $options = array_merge( @@ -828,32 +883,11 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $options ); - // Remove spaces from data URLs, which cause errors and PHP-CSS-Parser can't handle them. - $stylesheet_string = $this->remove_spaces_from_data_urls( $stylesheet_string ); - - // Find calc() functions and replace with placeholders since PHP-CSS-Parser can't handle them. - $stylesheet_string = $this->add_calc_placeholders( $stylesheet_string ); - $stylesheet = array(); - $validation_results = array(); - try { - $parser_settings = Sabberworm\CSS\Settings::create(); - $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_results = array_merge( $validation_results, $this->process_css_list( $css_document, $options ) ); + $parsed_stylesheet = $this->parse_stylesheet( $stylesheet_string, $options ); + $validation_results = $parsed_stylesheet['validation_results']; + if ( ! empty( $parsed_stylesheet['css_document'] ) ) { + $css_document = $parsed_stylesheet['css_document']; $output_format = Sabberworm\CSS\OutputFormat::createCompact(); $output_format->setSemicolonAfterLastRule( false ); @@ -918,13 +952,13 @@ function( $matches ) use ( $selector, &$selectors_parsed ) { } // Restore calc() functions that were replaced with placeholders. - if ( ! empty( $this->calc_placeholders ) ) { - $declaration = str_replace( - array_keys( $this->calc_placeholders ), - array_values( $this->calc_placeholders ), - $declaration - ); - } + $declaration = preg_replace_callback( + '/-wp-calc-placeholder\("(.+?)"\)/', + function( $matches ) { + return base64_decode( $matches[1] ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode + }, + $declaration + ); $stylesheet[] = array( $selectors_parsed, @@ -934,22 +968,6 @@ function( $matches ) use ( $selector, &$selectors_parsed ) { $stylesheet[] = $split_stylesheet[ $i ]; } } - - // Reset calc placeholders. - $this->calc_placeholders = array(); - } catch ( Exception $exception ) { - $error = array( - 'code' => 'css_parse_error', - 'message' => $exception->getMessage(), - ); - - /* - * This is not a recoverable error, so sanitized here is just used to give user control - * over whether to proceed with serving this exception-raising stylesheet in AMP. - */ - $sanitized = $this->should_sanitize_validation_error( $error ); - - $validation_results[] = compact( 'error', 'sanitized' ); } $this->parse_css_duration += ( microtime( true ) - $start_time ); @@ -1005,7 +1023,7 @@ public function should_sanitize_validation_error( $validation_error, $data = arr } /** - * Add placeholders for calc() functions which the PHP-CSS-Parser doesn't handle them properly yet. + * Add encoded placeholders for calc() functions which the PHP-CSS-Parser doesn't handle them properly yet. * * @since 1.0 * @link https://github.com/sabberworm/PHP-CSS-Parser/issues/79 @@ -1034,10 +1052,7 @@ private function add_calc_placeholders( $css ) { // Found the end of the calc() function, so replace it with a placeholder function. if ( 0 === $open_parens ) { $matched_calc = substr( $css, $match_offset, $final_offset - $match_offset + 1 ); - $placeholder = sprintf( '-wp-calc-placeholder(%d)', count( $this->calc_placeholders ) ); - - // Store the placeholder function so the original calc() can be put in its place. - $this->calc_placeholders[ $placeholder ] = $matched_calc; + $placeholder = sprintf( '-wp-calc-placeholder("%s")', base64_encode( $matched_calc ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode // Update the CSS to replace the matched calc() with the placeholder function. $css = substr( $css, 0, $match_offset ) . $placeholder . substr( $css, $final_offset + 1 ); @@ -1690,9 +1705,6 @@ private function finalize_styles() { $message .= sprintf( '[%s=%s]', $attribute->nodeName, $attribute->nodeValue ); } } - if ( ! empty( $pending_stylesheet['imported'] ) ) { - $message .= sprintf( ' @import url(%s)', $pending_stylesheet['imported'] ); - } $message .= sprintf( /* translators: %d is number of bytes */ _n( ' (%d byte)', ' (%d bytes)', $pending_stylesheet['size'], 'amp' ), diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index c355cfcdbfe..8e7b4d8e338 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -877,16 +877,21 @@ public function test_css_import() { ) ); $sanitizer->sanitize(); $stylesheets = array_values( $sanitizer->get_stylesheets() ); - $stylesheet_patterns = array( - '/' . preg_quote( 'input[type="checkbox"]:disabled' ) . '/', - '/' . preg_quote( 'body.rtl' ) . '/', - '/' . preg_quote( '.login .message' ) . '/', - '/^' . preg_quote( 'html{background-color:lightblue}' ) . '$/', - '/^' . preg_quote( 'body{color:red}' ) . '$/', + $this->assertCount( 2, $stylesheets ); + $this->assertRegExp( + '/' . implode( '.*', array( + preg_quote( 'input[type="checkbox"]:disabled' ), + preg_quote( 'body.rtl' ), + preg_quote( '.login .message' ), + ) ) . '/s', + $stylesheets[0] + ); + $this->assertRegExp( + '/' . implode( '.*', array( + preg_quote( 'html{background-color:lightblue}' ), + preg_quote( 'body{color:red}' ), + ) ) . '/s', + $stylesheets[1] ); - $this->assertCount( count( $stylesheet_patterns ), $stylesheets ); - do { - $this->assertRegExp( array_shift( $stylesheet_patterns ), array_shift( $stylesheets ) ); - } while ( ! empty( $stylesheet_patterns ) ); } }