Global Styles: Allow arbitrary CSS, protect from KSES mangling

[sirreal]

• Allow arbitrary text in global styles custom CSS.
• Protect the CSS data from mangling by KSES HTML filters.

Under some circumstances KSES would run post content filters and change the resulting content like this:

 @property --animate {
-  syntax: "<custom-ident>";
+  syntax: "";
   inherits: true;
   initial-value: false;
 }

The Custom CSS is stored as JSON-encoded data in post content. KSES filters this content as HTML.

I explored a change that would remove and restore the offending KSES filters when saving the custom CSS posts, however the simplest way of protecting the data is by escaping the JSON-encoded data so that it does not contain HTML-like syntax. <>& and Unicode-escaped by the JSON flags JSON_HEX_TAG and JSON_HEX_AMP.

This PR does not change the Customizer Custom CSS handling. That will be done separately.

This depends on #10656 to ensure styles output is safely printed in the HTML (merged here).

Trac ticket: https://core.trac.wordpress.org/ticket/64418

To do:

• Land Styles: Use HTML API to generate <style> tags #10656.
• Check whether revisions are working correctly.

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[sirreal]

As I explored the space, this PR grew in scope. There are at least two concerns that I plan to split up:

• Use the HTML API to correctly produce STYLE tags across the codebase.
• Relax CSS "validation," (requires additional safety on STYLE tags).

[sirreal]

YES! KSES indeed breaks this as can be seen in the multisite tests:

1) WP_REST_Global_Styles_Controller_Test::test_update_allows_valid_css_with_more_syntax
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 '@property --animate {
-	syntax: "<custom-ident>";
+	syntax: "";
 	inherits: true;
 	initial-value: false;
 }'

That's almost certainly a missing unfiltered_html cap triggering KSES and strip_html.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, dmsnell, westonruter, ramonopoly.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[westonruter]

Thus bypassing the need to do any KSES dance?

[sirreal]

Exactly.

[dmsnell]

this is a bit of personal preference, but @westonruter do you have thoughts about adding the newline on the echo instead of in the Tag Processor? it seems a bit odd to me seeing it there when our purpose is building the tag.

I support the changes as they are, but I do find echo "{$processor->get_updated_html()}\n"; a bit nicer than adding an extra whitespace-only text node into the HTML parsing.

[westonruter]

Keeping that newline token outside of the tag processor is cool to me. So yeah, printing the newline after like you have looks good. If that could made consistent in each case, especially.

[sirreal]

I've addressed this in #10656 / 407d43f.

[ramonjd]

I'm coming at this pretty fresh, but from a global styles perspective, it's looking good to me.

I tested on top of #10656

Not sure if this helps you, but I smoke tested the scenario in question (@property --animate), as well as:

• nested CSS, including @media @keyframes @supports @layer
• weird attribute selectors
• CSS vars
• CSS functions like clamp() calc() etc
• HTML tags - I'm seeing the HTML API's escaping in action \3c\2fstyle> and no injected javascript executes

I can save "Additional CSS" without error and the frontend rendering is as expected.

An aside, this will have to be synced with Gutenberg:

https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-rest-global-styles-controller-gutenberg.php

I'm happy to do it if you don't have time once this PR lands.

Thanks a lot for working on this! 🙇🏻

[ramonjd]

Do you think this method needs deprecation now that it returns true?

Also, would it be worth documenting why arbitrary CSS is allowed, how it's protected, and what users should know?

[dmsnell]

I had a long rant here and something happened and the Github page reloaded and it’s gone, but I came to say similar things. I don’t want to repeat it all, but the salient points can be distilled:

• there are zero matches for classes extending WP_REST_Global_Styles_Controller in the Directory, and the only call to this method in Core is on line 257 above. We can remove that check if we’re going to defang it here.
• proprietary or private code might still call this through a subclass. perhaps we should deprecate it and leave in the check.
• the DocBlock comment was almost meaningless and now is actively misleading. at least we could have said something like ”detects if the given CSS might prematurely close its containing STYLE element” but “validate as valid” is not only ambiguous here, but entirely inaccurate, as is the new change to “not break” HTML. the function now does absolutely nothing.

if we leave this in then I’m more in favor of deprecating it, removing the call above, and potentially even expanding the check to be more precise.

/**
 * Returns an error if the given CSS would prematurely end
 * a containing STYLE element, or `true` otherwise.
 */
protected function validate_custom_css( $css ) {
	$at  = 0;
	$end = strlen( $css ) - 7; // Minimum length of terminating end tag prefix.

	while ( $at < $end ) {
		$at = stripos( $css, '</style', $at );
		if ( false === $at ) {
			return true;
		}

		$at += 7;

		if ( 1 === strspn( $css, " \t\f\r\n/>", $at, 1 ) ) {
			return new WP_Error( … );
		}
	}

	return true;
}

[sirreal]

Good feedback. In e1322b0 I've:

• removed the call to the method
• Added a _deprecated_function() to the method.
• Added relevant annotations to the methods documentation to mark it as deprecated and to exclude it from generated documentation.
• I removed the method's description since the method should not be used or documented.

[dmsnell]

well this is a surprising function, and one perhaps that warrants considerably more refactoring. if my understanding is correct, this is attempting to JSON-parse every single post of every single kind of post type on save, even though its name seems to suggest that it’s designed to process global styles posts.

maybe @jorgefilipecosta has some context that is’t obvious.

at a minimum, it surprises me we aren’t even checking if the first and last bytes are { and }, since those are the only allowable JSON. on the positive side, I guess json_decode() will fail quickly, but only after cloning the string and performing string replacements on it via wp_unslash().

---

on to the question I wanted to discuss on this line, which is a question about backwards compatibility: will this change any existing code? or should it preserve properly given the fact that this should run directly into json_decode() on the other end and parse to the same values?

I would want us to be careful about anything else in the pipeline which might start interpreting content differently given the escaping. my guess is that this is fine, but I’d like to have some confirmation on some of the differences in encoding and how those will be interpreted later on and in the editor and on render.

[sirreal]

will this change any existing code? or should it preserve properly given the fact that this should run directly into json_decode() on the other end and parse to the same values?

I'm not deeply familiar with global styles, so I was happy to get @ramonjd's review (and more are welcome).

It's hard to imagine using this data without parsing the JSON. Compliant JSON parsers will parse this correctly. Anything that relies on this data being JSON encoded in a specific way (with specific escaping) would be surprising and does not seem like behavior that needs to be supported.

That said, I know that the global styles system will parse the JSON:

wordpress-develop/src/wp-includes/class-wp-theme-json-resolver.php

Line 563 in 4b96129

$decoded_data = json_decode( $user_cpt['post_content'], true );

As will the REST endpoint:

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php

Line 304 in 4b96129

$raw_config = json_decode( $post->post_content, true );

The REST API later responds with a JSON body, but that encoding happens elsewhere and the client is expected to parse the JSON response.

[ramonjd]

will this change any existing code? or should it preserve properly given the fact that this should run directly into json_decode() on the other end and parse to the same values?

In my limited knowledge, and as far as I'm aware it's expected that the content will eventually meet json_decode(), which handles things identically.

Should there be a test case to confirm this?

if my understanding is correct, this is attempting to JSON-parse every single post of every single kind of post type on save

Oh I see it's running through _save_pre hooks, so I understand the same. Bit before my time, but I like your optimization idea to check for { and } before parsing.

My intellectual crutch on these matters is usually @oandregal and @andrewserong

That said, I know that the global styles system will parse the JSON:
As will the REST endpoint:

This is the state as I understand it as well:

• json_decode in WP_Theme_JSON_Resolver to parse and merge user data for the most recent published post for internal processing, rendering CSS etc
• json_decode in WP_REST_Global_Styles_Controller to parse handle a specific wp_global_styles post by ID in REST

Cheers!

[sirreal]

An aside, this will have to be synced with Gutenberg:

https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-rest-global-styles-controller-gutenberg.php

Thank you for flagging that @ramonjd. This is an interesting case and I'd like to get your feedback.

This depends on the work in #10656. That's another Core change to how STYLE tags are constructed. Without that change, unsafe styles can be produced. That's a problem if Gutenberg removes the CSS content restrictions because it won't include the necessary changes to STYLE tags.

How should we approach that?

One option is to maintain the validation in such a way that unsafe CSS is not allowed, @dmsnell shared a suitable snippet in this comment. This relaxes the check and should be safe to include in Gutenberg today because the STYLE tag cannot be prematurely closed.

[ramonjd]

This depends on the work in #10656. That's another Core change to how STYLE tags are constructed. Without that change, unsafe styles can be produced. That's a problem if Gutenberg removes the CSS content restrictions because it won't include the necessary changes to STYLE tags.

Dang! Can I take that back about syncing?

Yeah, I think we'll have to have validate_custom_css do something if plugin users aren't running 7.0 or whatever version these changes will live in.

In my opinion, Dennis's comment is a safe bet.

perhaps we should deprecate it and leave in the check.

Or, since it's plugin-only, use Dennis's new and improved version! Does that sound reasonable?

[sirreal]

I'm quite ignorant on a lot of this.

The last change to that file in Gutenberg was WordPress/gutenberg@f77a466 (Sep 18, 2024).

• Does Gutenberg need to keep its version or can it be removed and rely on Core now?
• Does it strictly need to be in sync ASAP, or could it come into sync when Core is ready?
• Gutenberg could use a simplified version of the check (a stripos() check for </style is probably sufficient) until it requires WordPress 7.0, then fully sync and remove the validation check.

[ramonjd]

Good questions:

Does Gutenberg need to keep its version or can it be removed and rely on Core now?

For context, the global styles controller was created as a dedicated, bundled class because the Core equivalent was being extended and overridden so frequently, and referenced others classes like WP_Theme_JSON and others that were also being extended and overridden, that it became a burden to deal with the compat files and classes.

For example, the controller relies on WP_Theme_JSON_Resolver_Gutenberg or WP_Theme_JSON_Gutenberg, which has been modified in the plugin, and then these might further rely on modified functions/methods and so on.

My instinct is that Gutenberg needs to keep its changes for this reason.

Folks are pretty good at migrating said changes to Core, and theoretically they should be synced in that direction (plugin > Core).

Does it strictly need to be in sync ASAP, or could it come into sync when Core is ready?

I reckon after this merges is fine. Not speaking specifically this PR, but if Core introduces a bugfix/new feature in global styles controller is generally good to sync that back before the next plugin update so plugin users get the benefit (since the plugin uses it's own class WP_REST_Global_Styles_Controller_Gutenberg)

Gutenberg could use a simplified version of the check (a stripos() check for </style is probably sufficient) until it requires WordPress 7.0, then fully sync and remove the validation check.

This sounds wholly reasonable to me. I appreciate you being on top of this! Thank you!