Global Styles: Allow arbitrary CSS, protect from KSES mangling #10641
+54
−39
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,12 +253,6 @@ protected function prepare_item_for_database( $request ) { | |
| if ( isset( $request['styles'] ) || isset( $request['settings'] ) ) { | ||
| $config = array(); | ||
| if ( isset( $request['styles'] ) ) { | ||
| $config['styles'] = $request['styles']; | ||
| } elseif ( isset( $existing_config['styles'] ) ) { | ||
| $config['styles'] = $existing_config['styles']; | ||
|
|
@@ -275,7 +269,14 @@ protected function prepare_item_for_database( $request ) { | |
| } | ||
| $config['isGlobalStylesUserThemeJSON'] = true; | ||
| $config['version'] = WP_Theme_JSON::LATEST_SCHEMA; | ||
| /** | ||
| * JSON encode the data stored in post content. | ||
| * Escape characters that are likely be mangled by HTML filters: "<>&". | ||
| * | ||
| * This data is later re-encoded by {@see wp_filter_global_styles_post}. | ||
| * The escaping is also applied here as a precaution. | ||
|
Member
There was a problem hiding this comment. Thus bypassing the need to do any KSES dance? |
||
| */ | ||
| $changes->post_content = wp_json_encode( $config, JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_HEX_AMP ); | ||
| } | ||
|
|
||
| // Post title. | ||
|
|
@@ -657,24 +658,17 @@ public function get_theme_items( $request ) { | |
| } | ||
|
|
||
| /** | ||
| * @since 6.2.0 | ||
| * @since 6.4.0 Changed method visibility to protected. | ||
| * @deprecated 7.0.0 This method is deprecated and always returns true. | ||
| * | ||
| * @ignore | ||
| * | ||
| * @param string $css CSS to validate. | ||
| * @return true|WP_Error Always returns true. | ||
| */ | ||
| protected function validate_custom_css( $css ) { | ||
|
Member
There was a problem hiding this comment. Do you think this method needs deprecation now that it returns Also, would it be worth documenting why arbitrary CSS is allowed, how it's protected, and what users should know?
Member
There was a problem hiding this comment. 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:
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;
}
Member
Author
There was a problem hiding this comment. Good feedback. In e1322b0 I've:
|
||
| _deprecated_function( __METHOD__, '7.0.0' ); | ||
| return true; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 guessjson_decode()will fail quickly, but only after cloning the string and performing string replacements on it viawp_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
As will the REST endpoint:
wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Line 304 in 4b96129
The REST API later responds with a JSON body, but that encoding happens elsewhere and the client is expected to parse the JSON response.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Oh I see it's running through
_save_prehooks, 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
This is the state as I understand it as well:
json_decodeinWP_Theme_JSON_Resolverto parse and merge user data for the most recent published post for internal processing, rendering CSS etcjson_decodeinWP_REST_Global_Styles_Controllerto parse handle a specific wp_global_styles post by ID in RESTCheers!