Boost: stop stripping SVG markup from Critical CSS while keeping style-tag breakout protection

projects/plugins/boost/app/lib/critical-css/class-display-critical-css.php

@@ -106,13 +106,31 @@ public function display_critical_css() {
 
 		echo '<style id="jetpack-boost-critical-css">';
 
-		// Ensure no </style> tag (or any HTML tags) in output.
+		// Ensure the CSS cannot terminate the style element early.
 		// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
-		echo wp_strip_all_tags( $critical_css );
+		echo self::sanitize_css( $critical_css );
 
 		echo '</style>';
 	}
 
+	/**
+	 * Sanitize CSS for output inside a <style> element.
+	 *
+	 * Avoids wp_strip_all_tags(), which corrupts valid CSS values that contain
+	 * markup - e.g. `background-image: url("data:image/svg+xml,<svg ...></svg>")`.
+	 *
+	 * The only sequence which can terminate a <style> element early is a closing
+	 * style tag, so neutralize that sequence (case-insensitively) by escaping its
+	 * forward slash. Inside CSS strings and url() tokens `\/` is a valid escape
+	 * for `/`, so legitimate CSS keeps its meaning.
+	 *
+	 * @param string $css CSS to sanitize.
+	 * @return string CSS that is safe to print inside a <style> element.
+	 */
+	public static function sanitize_css( $css ) {
+		return str_ireplace( '</style', '<\/style', $css );
+	}
+
 	/**
 	 * Add a small piece of JavaScript to the footer, which on load flips all
 	 * linked stylesheets from media="not all" to "all", and switches the

projects/plugins/boost/app/modules/optimizations/critical-css/class-css-proxy.php

@@ -3,6 +3,7 @@
 namespace Automattic\Jetpack_Boost\Modules\Optimizations\Critical_CSS;
 
 use Automattic\Jetpack_Boost\Lib\Critical_CSS\Critical_CSS_State;
+use Automattic\Jetpack_Boost\Lib\Critical_CSS\Display_Critical_CSS;
 
 /**
  * Add an ajax endpoint to proxy external CSS files.
@@ -59,7 +60,12 @@ public function handle_css_proxy() {
 		}
 
 		$css = '';
-		if ( false === $response ) {
+		if ( is_string( $response ) ) {
+			// Cache hit: the transient stores the CSS body from a previous
+			// successful fetch. Without this, cache hits served an empty
+			// response, feeding the Critical CSS generator no CSS at all.
+			$css = $response;
+		} elseif ( false === $response ) {
 			$response     = wp_safe_remote_get( $proxy_url );
 			$content_type = wp_remote_retrieve_header( $response, 'content-type' );
 			if ( strpos( $content_type, 'text/css' ) === false ) {
@@ -77,9 +83,12 @@ public function handle_css_proxy() {
 
 		if ( $css ) {
 			header( 'Content-type: text/css' );
-			// Outputting proxied CSS contents unescaped.
+			header( 'X-Content-Type-Options: nosniff' );
+			// Outputting proxied CSS contents unescaped. Do not strip tags here;
+			// valid CSS values may contain markup (e.g. inline SVGs in data: URIs),
+			// and stripping them corrupts the CSS fed to the generator.
 			// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
-			echo wp_strip_all_tags( $css );
+			echo Display_Critical_CSS::sanitize_css( $css );
 			die( 0 );
 		}
 	}

projects/plugins/boost/changelog/fix-critical-css-svg-stripping

@@ -0,0 +1,4 @@
+Significance: patch
+Type: fixed
+
+Critical CSS: stop stripping inline SVG markup and double quotes from valid CSS values while still preventing style-tag breakout.

projects/plugins/boost/phpunit.11.xml.dist

@@ -22,6 +22,7 @@
 		<testsuite name="unit">
 			<directory suffix="Test.php">tests/php</directory>
 			<exclude>tests/php/lib/critical-css/Display_Critical_CSS_Test.php</exclude>
+			<exclude>tests/php/lib/critical-css/Critical_CSS_Storage_Test.php</exclude>
 			<exclude>tests/php/Jetpack_Boost_Test.php</exclude>
 			<exclude>tests/php/modules/optimizations/lcp/LCP_Optimize_Bg_Image_Test.php</exclude>
 			<exclude>tests/php/modules/optimizations/lcp/LCP_Optimize_Img_Tag_Test.php</exclude>
@@ -30,6 +31,7 @@
 		<testsuite name="with-wordpress">
 			<file>tests/php/Jetpack_Boost_Test.php</file>
 			<file>tests/php/lib/critical-css/Display_Critical_CSS_Test.php</file>
+			<file>tests/php/lib/critical-css/Critical_CSS_Storage_Test.php</file>
 			<file>tests/php/modules/optimizations/lcp/LCP_Optimize_Bg_Image_Test.php</file>
 			<file>tests/php/modules/optimizations/lcp/LCP_Optimize_Img_Tag_Test.php</file>
 			<file>tests/php/abilities/Boost_Abilities_Test.php</file>

projects/plugins/boost/phpunit.9.xml.dist

@@ -13,6 +13,7 @@
 		<testsuite name="unit">
 			<directory suffix="Test.php">tests/php</directory>
 			<exclude>tests/php/lib/critical-css/Display_Critical_CSS_Test.php</exclude>
+			<exclude>tests/php/lib/critical-css/Critical_CSS_Storage_Test.php</exclude>
 			<exclude>tests/php/Jetpack_Boost_Test.php</exclude>
 			<exclude>tests/php/modules/optimizations/lcp/LCP_Optimize_Bg_Image_Test.php</exclude>
 			<exclude>tests/php/modules/optimizations/lcp/LCP_Optimize_Img_Tag_Test.php</exclude>
@@ -21,6 +22,7 @@
 		<testsuite name="with-wordpress">
 			<file>tests/php/Jetpack_Boost_Test.php</file>
 			<file>tests/php/lib/critical-css/Display_Critical_CSS_Test.php</file>
+			<file>tests/php/lib/critical-css/Critical_CSS_Storage_Test.php</file>
 			<file>tests/php/modules/optimizations/lcp/LCP_Optimize_Bg_Image_Test.php</file>
 			<file>tests/php/modules/optimizations/lcp/LCP_Optimize_Img_Tag_Test.php</file>
 			<file>tests/php/abilities/Boost_Abilities_Test.php</file>

projects/plugins/boost/tests/php/lib/critical-css/Critical_CSS_Storage_Test.php

@@ -0,0 +1,100 @@
+<?php
+/**
+ * Tests for Critical_CSS_Storage class.
+ *
+ * @package automattic/jetpack-boost
+ */
+
+namespace Automattic\Jetpack_Boost\Tests\Lib\Critical_CSS;
+
+use Automattic\Jetpack_Boost\Lib\Critical_CSS\Critical_CSS_Storage;
+use WorDBless\BaseTestCase;
+
+/**
+ * Class Critical_CSS_Storage_Test
+ *
+ * Verifies that generated Critical CSS survives a save/load round-trip intact,
+ * including double quotes and inline SVG markup in CSS values.
+ *
+ * @see https://github.com/Automattic/jetpack/issues/42321
+ */
+class Critical_CSS_Storage_Test extends BaseTestCase {
+
+	/**
+	 * Set up test environment.
+	 *
+	 * WorDBless does not implement the SQL used by WP_Query post_name lookups,
+	 * so emulate them from the WorDBless in-memory post store.
+	 */
+	public function set_up() {
+		parent::set_up();
+
+		add_filter( 'posts_pre_query', array( $this, 'emulate_name_query' ), 10, 2 );
+	}
+
+	/**
+	 * Tear down test environment.
+	 */
+	public function tear_down() {
+		remove_filter( 'posts_pre_query', array( $this, 'emulate_name_query' ), 10 );
+		\WorDBless\Posts::init()->clear_all_posts();
+		parent::tear_down();
+	}
+
+	/**
+	 * Emulate WP_Query post_name lookups against the WorDBless post store.
+	 *
+	 * @param \WP_Post[]|null $posts Posts with which to short-circuit the query, or null.
+	 * @param \WP_Query       $query The running query.
+	 * @return \WP_Post[]|null
+	 */
+	public function emulate_name_query( $posts, $query ) {
+		$name      = $query->get( 'name' );
+		$post_type = $query->get( 'post_type' );
+
+		if ( ! $name || ! $post_type ) {
+			return $posts;
+		}
+
+		foreach ( \WorDBless\Posts::init()->posts as $id => $post ) {
+			if ( $post->post_name === $name && $post->post_type === $post_type ) {
+				return array( get_post( $id ) );
+			}
+		}
+
+		return array();
+	}
+
+	/**
+	 * Test that double quotes and inline SVG markup in CSS values survive a
+	 * save/load round-trip.
+	 */
+	public function test_store_css_round_trip_preserves_double_quotes_and_svg() {
+		$css = '.test-svg-background { background-image: url("data:image/svg+xml,<svg xmlns=\'http://www.w3.org/2000/svg\' viewBox=\'0 0 8 8\'><circle cx=\'4\' cy=\'4\' r=\'4\' fill=\'%23f00\'/></svg>"); }';
+
+		$storage = new Critical_CSS_Storage();
+		$storage->store_css( 'core_front_page', $css );
+
+		$result = $storage->get_css( array( 'core_front_page' ) );
+
+		$this->assertIsArray( $result );
+		$this->assertSame( 'core_front_page', $result['key'] );
+		$this->assertSame( $css, $result['css'] );
+	}
+
+	/**
+	 * Test that updating an existing entry does not corrupt double quotes.
+	 */
+	public function test_store_css_update_preserves_double_quotes() {
+		$storage = new Critical_CSS_Storage();
+		$storage->store_css( 'core_posts_page', '.a { content: "first"; }' );
+
+		$css = '.quote::before { content: "\201C"; font-family: "Times New Roman", serif; }';
+		$storage->store_css( 'core_posts_page', $css );
+
+		$result = $storage->get_css( array( 'core_posts_page' ) );
+
+		$this->assertIsArray( $result );
+		$this->assertSame( $css, $result['css'] );
+	}
+}

projects/plugins/boost/tests/php/lib/critical-css/Display_Critical_CSS_Test.php

@@ -70,19 +70,66 @@ public function test_display_critical_css_with_empty_css() {
 	}
 
 	/**
-	 * Test display_critical_css() strips HTML tags.
+	 * Test display_critical_css() neutralizes closing style tags, so the CSS
+	 * cannot terminate the style element early and break out into HTML context.
 	 */
-	public function test_display_critical_css_strips_html() {
-		$css_with_html = 'body { color: red; }</style><script>alert("xss")</script>';
-		$instance      = new Display_Critical_CSS( $css_with_html );
+	public function test_display_critical_css_neutralizes_style_breakout() {
+		$css_with_breakout = 'body { color: red; }</style><script>alert("xss")</script>';
+		$instance          = new Display_Critical_CSS( $css_with_breakout );
 
 		ob_start();
 		$instance->display_critical_css();
 		$output = ob_get_clean();
 
-		$this->assertStringNotContainsString( '<script>', $output );
-		$this->assertStringNotContainsString( 'alert', $output );
-		$this->assertSame( 1, substr_count( $output, '</style>' ) );
+		// The only `</style` sequence left must be the real closing tag, so the
+		// injected markup stays inert inside the style element's raw text.
+		$this->assertSame( 1, substr_count( strtolower( $output ), '</style' ) );
+		$this->assertStringEndsWith( '</style>', $output );
+		$this->assertStringContainsString( '<\/style><script>', $output );
+	}
+
+	/**
+	 * Test display_critical_css() neutralizes closing style tags regardless of case.
+	 */
+	public function test_display_critical_css_neutralizes_style_breakout_case_insensitively() {
+		$instance = new Display_Critical_CSS( 'body { color: red; }</StYlE ><p>injected</p>' );
+
+		ob_start();
+		$instance->display_critical_css();
+		$output = ob_get_clean();
+
+		$this->assertSame( 1, substr_count( strtolower( $output ), '</style' ) );
+		$this->assertStringEndsWith( '</style>', $output );
+	}
+
+	/**
+	 * Test display_critical_css() preserves inline SVGs in CSS values.
+	 *
+	 * @see https://github.com/Automattic/jetpack/issues/42321
+	 */
+	public function test_display_critical_css_preserves_inline_svg_data_uri() {
+		$css      = '.hero { background-image: url("data:image/svg+xml,<svg xmlns=\'http://www.w3.org/2000/svg\' viewBox=\'0 0 8 8\'><circle cx=\'4\' cy=\'4\' r=\'4\' fill=\'%23f00\'/></svg>"); }';
+		$instance = new Display_Critical_CSS( $css );
+
+		ob_start();
+		$instance->display_critical_css();
+		$output = ob_get_clean();
+
+		$this->assertStringContainsString( $css, $output );
+	}
+
+	/**
+	 * Test display_critical_css() preserves double quotes in CSS values.
+	 */
+	public function test_display_critical_css_preserves_double_quotes() {
+		$css      = '.quote::before { content: "\201C"; font-family: "Times New Roman", serif; }';
+		$instance = new Display_Critical_CSS( $css );
+
+		ob_start();
+		$instance->display_critical_css();
+		$output = ob_get_clean();
+
+		$this->assertStringContainsString( $css, $output );
 	}
 
 	/**