data URL broken in amp-custom stylesheet

[felixarntz]

I was just testing Site Kit in AMP Native and there is a problem with the G menu icon in the admin bar in the frontend, it doesn't show. It uses background-image: url(data:image/svg+xml...), and the AMP plugin seems to change that SVG markup, causing it to be invalid.

Correct background image:
data:image/svg+xml;charset=utf-8,%3Csvg width='43' height='44' xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink'%3E%3Cdefs%3E%3Cpath d='M42.5 18H22v8.5h11.8C32.7 31.9 28.1 35 22 35c-7.2 0-13-5.8-13-13S14.8 9 22 9c3.1 0 5.9 1.1 8.1 2.9l6.4-6.4C32.6 2.1 27.6 0 22 0 9.8 0 0 9.8 0 22s9.8 22 22 22c11 0 21-8 21-22 0-1.3-.2-2.7-.5-4z' id='a'/%3E%3C/defs%3E%3Cuse fill='%23FFF' xlink:href='%23a' fill-rule='evenodd'/%3E%3C/svg%3E

Incorrect background image (with AMP Native):
data:image/svg+xml;charset=utf-8,%3Csvgwidth=\'43\'height=\'44\'xmlns=\'http://www.w3.org/2000/svg\'xmlns:xlink=\'http://www.w3.org/1999/xlink\'%3E%3Cdefs%3E%3Cpathd=\'M42.518H22v8.5h11.8C32.731.928.1352235c-7.20-13-5.8-13-13S14.89229c3.105.91.18.12.9l6.4-6.4C32.62.127.602209.8009.8022s9.8222222c11021-821-220-1.3-.2-2.7-.5-4z\'id=\'a\'/%3E%3C/defs%3E%3Cusefill=\'%23FFF\'xlink:href=\'%23a\'fill-rule=\'evenodd\'/%3E%3C/svg%3E

In other words, it appears to strip spaces, causing the SVG to be invalid.

cc @westonruter

[westonruter]

@felixarntz I don't think that is valid CSS:

I think it needs to be quoted, but even so, I can't seem to get it to work even on a regular HTML page (like in a Codepen). Can you share a standalone example of it working?

Base64-encoding it works for me:

.foo {
    background-image: url('data:image/svg+xml;base64,PHN2ZyB3aWR0aD0nNDMnIGhlaWdodD0nNDQnIHhtbG5zPSdodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZycgeG1sbnM6eGxpbms9J2h0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsnPjxkZWZzPjxwYXRoIGQ9J000Mi41IDE4SDIydjguNWgxMS44QzMyLjcgMzEuOSAyOC4xIDM1IDIyIDM1Yy03LjIgMC0xMy01LjgtMTMtMTNTMTQuOCA5IDIyIDljMy4xIDAgNS45IDEuMSA4LjEgMi45bDYuNC02LjRDMzIuNiAyLjEgMjcuNiAwIDIyIDAgOS44IDAgMCA5LjggMCAyMnM5LjggMjIgMjIgMjJjMTEgMCAyMS04IDIxLTIyIDAtMS4zLS4yLTIuNy0uNS00eicgaWQ9J2EnLz48L2RlZnM+PHVzZSBmaWxsPScjZmZmJyB4bGluazpocmVmPScjYScgZmlsbC1ydWxlPSdldmVub2RkJy8+PC9zdmc+');
}

An important thing is to quote the URL string when there are spaces. The AMP plugin strips spaces from data: URLs:

amp-wp/includes/sanitizers/class-amp-style-sanitizer.php

Lines 1475 to 1476 in d9315bf

	// 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 );

amp-wp/includes/sanitizers/class-amp-style-sanitizer.php

Lines 1758 to 1774 in d9315bf

	/**
	* Remove spaces from data URLs which PHP-CSS-Parser doesn't handle.
	*
	* @since 1.0
	*
	* @param string $css CSS.
	* @return string CSS with spaces removed from data URLs.
	*/
	private function remove_spaces_from_data_urls( $css ) {
	return preg_replace_callback(
	'/\burl\([^}]*?\)/',
	function( $matches ) {
	return preg_replace( '/\s+/', '', $matches[0] );
	},
	$css
	);
	}

But this is not accounting for data other than base64-encoded, so that could indeed be a bug. I think the logic needs to be prevented if the string inside the url() is quoted. In other words:

diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php
index eaee7b64..99f91b5a 100644
--- a/includes/sanitizers/class-amp-style-sanitizer.php
+++ b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -1765,8 +1765,12 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
 	 */
 	private function remove_spaces_from_data_urls( $css ) {
 		return preg_replace_callback(
-			'/\burl\([^}]*?\)/',
+			'/\burl\(\s*([^}]*?)\)/',
 			function( $matches ) {
+				// Skip stripping spaces if the URL is already quoted.
+				if ( '"' === $matches[1][0] || "'" === $matches[1][0] ) {
+					return $matches[0];
+				}
 				return preg_replace( '/\s+/', '', $matches[0] );
 			},
 			$css

This logic was first introduced in #1164 and you can see that SVG data: URLs was not anticipated.

[westonruter]

Aside: the name remove_spaces_from_data_urls is a misnomer because it strips spaces from non-data: URLs as well.

[westonruter]

Also, instead of stripping spaces it would be better if it would add quote marks around the string.

[felixarntz]

@westonruter Just checked this again with Site Kit, it doesn't appear to be an issue anymore, but I haven't been able to identify why. Here's the markup Site Kit produces now for the icon:
url("data:image/svg+xml;charset=UTF-8,%3Csvg%20width%3D%2243%22%20height%3D%2244%22%20viewBox%3D%220%200%2043%2044%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20xmlns%3Axlink%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%22%3E%3Cdefs%3E%3Cpath%20d%3D%22M42.5%2018H22v8.5h11.8C32.7%2031.9%2028.1%2035%2022%2035c-7.2%200-13-5.8-13-13S14.8%209%2022%209c3.1%200%205.9%201.1%208.1%202.9l6.4-6.4C32.6%202.1%2027.6%200%2022%200%209.8%200%200%209.8%200%2022s9.8%2022%2022%2022c11%200%2021-8%2021-22%200-1.3-.2-2.7-.5-4z%22%20id%3D%22a%22%2F%3E%3C%2Fdefs%3E%3Cuse%20fill%3D%22%23FFF%22%20xlink%3Ahref%3D%22%23a%22%20fill-rule%3D%22evenodd%22%2F%3E%3C%2Fsvg%3E")

So there are no spaces anymore, but that part of the Sass has been like that since early February (so way before I opened this). Potentially something was modified in the compiler that previously caused the spaces to be present.

Anyway, I think we can probably close this.

[westonruter]

The difference appears to be that now your image data: URL is quoted and its characters are URL-encoded. Previously I believe the URL string was not quoted, correct? Also the special characters weren't URL-encoded.

[westonruter]

Thanks for the follow-up. This was also reported in #3503 and the fix in #3561 will prevent stripping spaces in any url() value that is quoted. Since your data: URL is both quoted and lacks spaces, it won't be impacted by this change; but I've added tests in 9ff6593 to be sure of this.