-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Global Styles: Allow arbitrary CSS, protect from KSES mangling #10641
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 67 commits
7c01020
511284e
edca2a2
db273b7
62d59c2
f3a1716
e793caa
aa7852c
93cddc3
41a1fe3
99b8733
2ea6a12
81679af
e3a6d78
2c19861
05a45e1
c124eb5
e055156
33f9616
606539e
c938d4c
dd919f1
d29900a
6c6a72b
aad4744
96849ff
1ab58ec
ac2c8e6
c3ae9a9
4e88745
c8f8a4d
731bce7
1eb76e7
99b1ba4
d296d6c
0050789
d3ac6a8
b185a8c
96ea3c4
56e19b5
d9ae831
12a6775
0141653
6585099
5a8ce12
30ca7cb
e1322b0
407d43f
ccf1625
adb0a13
6a9a7b0
793eed8
cc5c89e
4eb526b
0bd4e51
0c8e3a0
9d65051
846d9d2
23a1c96
0f43cc1
0768d90
c3cda32
3e5d018
b9b5315
a36dc2f
842e35e
e8b1ea5
4c492c3
fc47b77
82a4b3f
5e891a2
2076cb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2386,7 +2386,13 @@ function wp_filter_global_styles_post( $data ) { | |||||
| $data_to_encode = WP_Theme_JSON::remove_insecure_properties( $decoded_data, 'custom' ); | ||||||
|
|
||||||
| $data_to_encode['isGlobalStylesUserThemeJSON'] = true; | ||||||
| return wp_slash( wp_json_encode( $data_to_encode ) ); | ||||||
| /** | ||||||
| * JSON encode the data stored in post content. | ||||||
| * Escape characters that are likely to be mangled by HTML filters: "<>&". | ||||||
| * | ||||||
| * This matches the escaping in {@see WP_REST_Global_Styles_Controller::prepare_item_for_database}. | ||||||
| */ | ||||||
| return wp_slash( wp_json_encode( $data_to_encode, JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_HEX_AMP ) ); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Should there be a test case to confirm this?
Oh I see it's running through My intellectual crutch on these matters is usually @oandregal and @andrewserong
This is the state as I understand it as well:
Cheers!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, this is correct. IIRC, it was introduced to take into account The suggested improvement could be nice as an early return. There's no situation in which
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unfamiliar with how custom CSS was implemented in global styles, but checking
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That verbatim check is gated on I'd prefer to keep any changes to that location separate from this PR.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yah we do that for any type of post type, that happens because the WordPress security sanitization is per field, so content is sanitized without knowing what the post type field is. I guess this is intentional to avoid cases where there is unsafe content in a post type (e.g: a malicious global styles payload) that content is inserted using a different post type, and then somehow e.g: plug-in or something changes the post type.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn’t help, it kept staring at me, but I didn’t push any changes. diff --git a/src/wp-includes/kses.php b/src/wp-includes/kses.php
index bff9cc38ba..ad0da1e0e3 100644
--- a/src/wp-includes/kses.php
+++ b/src/wp-includes/kses.php
@@ -2376,6 +2376,22 @@ function wp_filter_post_kses( $data ) {
* @return string Filtered post content with unsafe rules removed.
*/
function wp_filter_global_styles_post( $data ) {
+ // As only JSON objects are allowed, skip attempting the parse if it’s not possible.
+ if ( ! is_string( $data ) ) {
+ return $data;
+ }
+
+ /**
+ * PHP implements RFC7159, which skips leading and trailing whitespace.
+ * After this initial whitespace, the first character _must_ be %x7B.
+ *
+ * @see https://datatracker.ietf.org/doc/html/rfc7159
+ */
+ $first_non_ws_at = strspn( $data, " \t\r\n" );
+ if ( $first_non_ws_at >= strlen( $data ) || '{' !== $data[ $first_non_ws_at ] ) {
+ return $data;
+ }
+
$decoded_data = json_decode( wp_unslash( $data ), true );
$json_decoding_error = json_last_error();
if (I don’t have time to collect the data, but this avoids the initial processing and allocation costs for strings which cannot be JSON objects. it would involve some review to assert that the code is right. I did check the implementation inside of PHP and the |
||||||
| } | ||||||
| return $data; | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,7 +275,14 @@ protected function prepare_item_for_database( $request ) { | |
| } | ||
| $config['isGlobalStylesUserThemeJSON'] = true; | ||
| $config['version'] = WP_Theme_JSON::LATEST_SCHEMA; | ||
| $changes->post_content = wp_json_encode( $config ); | ||
| /** | ||
| * JSON encode the data stored in post content. | ||
| * Escape characters that are likely to be mangled by HTML filters: "<>&". | ||
| * | ||
| * This data is later re-encoded by {@see wp_filter_global_styles_post}. | ||
sirreal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * The escaping is also applied here as a precaution. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thus bypassing the need to do any KSES dance?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. |
||
| */ | ||
| $changes->post_content = wp_json_encode( $config, JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_HEX_AMP ); | ||
| } | ||
|
|
||
| // Post title. | ||
|
|
@@ -659,22 +666,87 @@ public function get_theme_items( $request ) { | |
| /** | ||
| * Validate style.css as valid CSS. | ||
| * | ||
| * Currently just checks for invalid markup. | ||
| * Currently just checks that CSS will not break an HTML STYLE tag. | ||
| * | ||
| * @since 6.2.0 | ||
| * @since 6.4.0 Changed method visibility to protected. | ||
| * @since 7.0.0 Only restricts contents which risk prematurely closing the STYLE element, | ||
| * either through a STYLE end tag or a prefix of one which might become a | ||
| * full end tag when combined with the contents of other styles. | ||
| * | ||
| * @param string $css CSS to validate. | ||
| * @return true|WP_Error True if the input was validated, otherwise WP_Error. | ||
| */ | ||
| protected function validate_custom_css( $css ) { | ||
sirreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if ( preg_match( '#</?\w+#', $css ) ) { | ||
| return new WP_Error( | ||
| 'rest_custom_css_illegal_markup', | ||
| __( 'Markup is not allowed in CSS.' ), | ||
| array( 'status' => 400 ) | ||
| $length = strlen( $css ); | ||
| for ( | ||
| $at = strcspn( $css, '<' ); | ||
| $at < $length; | ||
| $at += strcspn( $css, '<', ++$at ) | ||
| ) { | ||
| $remaining_strlen = $length - $at; | ||
| /** | ||
| * Custom CSS text is expected to render inside an HTML STYLE element. | ||
| * A STYLE closing tag must not appear within the CSS text because it | ||
| * would close the element prematurely. | ||
| * | ||
| * The text must also *not* end with a partial closing tag (e.g., `<`, | ||
| * `</`, … `</style`) because subsequent styles which are concatenated | ||
| * could complete it, forming a valid `</style>` tag. | ||
| * | ||
| * Example: | ||
| * | ||
| * $style_a = 'p { font-weight: bold; </sty'; | ||
| * $style_b = 'le> gotcha!'; | ||
| * $combined = "{$style_a}{$style_b}"; | ||
| * | ||
| * $style_a = 'p { font-weight: bold; </style'; | ||
| * $style_b = 'p > b { color: red; }'; | ||
| * $combined = "{$style_a}\n{$style_b}"; | ||
| * | ||
| * Note how in the second example, both of the style contents are benign | ||
| * when analyzed on their own. The first style was likely the result of | ||
| * improper truncation, while the second is perfectly sound. It was only | ||
| * through concatenation that these two scripts combined to form content | ||
| * that would have broken out of the containing STYLE element, thus | ||
| * corrupting the page and potentially introducing security issues. | ||
| * | ||
| * @see https://html.spec.whatwg.org/multipage/parsing.html#rawtext-end-tag-name-state | ||
| */ | ||
| $possible_style_close_tag = 0 === substr_compare( | ||
| $css, | ||
| '</style', | ||
| $at, | ||
| min( 7, $remaining_strlen ), | ||
| true | ||
| ); | ||
| if ( $possible_style_close_tag ) { | ||
| if ( $remaining_strlen < 8 ) { | ||
| return new WP_Error( | ||
| 'rest_custom_css_illegal_markup', | ||
| sprintf( | ||
| /* translators: %s is the CSS that was provided. */ | ||
| __( 'The CSS must not end in "%s".' ), | ||
| esc_html( substr( $css, $at ) ) | ||
sirreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ), | ||
| array( 'status' => 400 ) | ||
| ); | ||
| } | ||
|
|
||
| if ( 1 === strspn( $css, " \t\f\r\n/>", $at + 7, 1 ) ) { | ||
| return new WP_Error( | ||
| 'rest_custom_css_illegal_markup', | ||
| sprintf( | ||
| /* translators: %s is the CSS that was provided. */ | ||
| __( 'The CSS must not contain "%s".' ), | ||
| esc_html( substr( $css, $at, 8 ) ) | ||
| ), | ||
| array( 'status' => 400 ) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -650,6 +650,7 @@ public function test_update_item_valid_styles_css() { | |
| /** | ||
| * @covers WP_REST_Global_Styles_Controller::update_item | ||
| * @ticket 57536 | ||
| * @ticket 64418 | ||
| */ | ||
| public function test_update_item_invalid_styles_css() { | ||
| wp_set_current_user( self::$admin_id ); | ||
|
|
@@ -659,7 +660,7 @@ public function test_update_item_invalid_styles_css() { | |
| $request = new WP_REST_Request( 'PUT', '/wp/v2/global-styles/' . self::$global_styles_id ); | ||
| $request->set_body_params( | ||
| array( | ||
| 'styles' => array( 'css' => '<p>test</p> body { color: red; }' ), | ||
| 'styles' => array( 'css' => '</style>' ), | ||
| ) | ||
| ); | ||
| $response = rest_get_server()->dispatch( $request ); | ||
|
|
@@ -826,4 +827,113 @@ public function test_global_styles_route_args_schema() { | |
| $this->assertArrayHasKey( 'type', $route_data[0]['args']['id'] ); | ||
| $this->assertSame( 'integer', $route_data[0]['args']['id']['type'] ); | ||
| } | ||
|
|
||
| /** | ||
| * @covers WP_REST_Global_Styles_Controller::update_item | ||
| * @ticket 64418 | ||
| */ | ||
| public function test_update_allows_valid_css_with_more_syntax() { | ||
| wp_set_current_user( self::$admin_id ); | ||
| if ( is_multisite() ) { | ||
| grant_super_admin( self::$admin_id ); | ||
| } | ||
| $request = new WP_REST_Request( 'PUT', '/wp/v2/global-styles/' . self::$global_styles_id ); | ||
| $css = <<<'CSS' | ||
| @property --animate { | ||
| syntax: "<custom-ident>"; | ||
| inherits: true; | ||
| initial-value: false; | ||
| } | ||
| h1::before { content: "fun & games"; } | ||
| CSS; | ||
| $request->set_body_params( | ||
| array( | ||
| 'styles' => array( 'css' => $css ), | ||
| ) | ||
| ); | ||
|
|
||
| $response = rest_get_server()->dispatch( $request ); | ||
| $data = $response->get_data(); | ||
| $this->assertSame( $css, $data['styles']['css'] ); | ||
|
|
||
| // Compare expected API output to WP internal values. | ||
| $request = new WP_REST_Request( 'GET', '/wp/v2/global-styles/' . self::$global_styles_id ); | ||
| $response = rest_get_server()->dispatch( $request ); | ||
| $this->assertSame( $css, $response->get_data()['styles']['css'] ); | ||
| } | ||
|
|
||
| /** | ||
| * @covers WP_REST_Global_Styles_Controller::validate_custom_css | ||
| * @ticket 64418 | ||
| * | ||
| * @dataProvider data_custom_css_allowed | ||
| */ | ||
| public function test_validate_custom_css_allowed( string $custom_css ) { | ||
| $controller = new WP_REST_Global_Styles_Controller(); | ||
| $validate = Closure::bind( | ||
| function ( $css ) { | ||
| return $this->validate_custom_css( $css ); | ||
| }, | ||
| $controller, | ||
| $controller | ||
| ); | ||
|
|
||
| $this->assertTrue( $validate( $custom_css ) ); | ||
| } | ||
|
|
||
| public static function data_custom_css_allowed() { | ||
sirreal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return array( | ||
| '@property declaration' => array( | ||
| '@property --prop { syntax: "<custom-ident>"; inherits: true; initial-value: false; }', | ||
| ), | ||
| 'Different close tag' => array( '</stylesheet>' ), | ||
| 'Not a style close tag' => array( '/*</style*/' ), | ||
| 'Not a style close tag 2' => array( '/*</style_' ), | ||
| 'Empty' => array( '' ), | ||
| 'Short content' => array( '/**/' ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @covers WP_REST_Global_Styles_Controller::validate_custom_css | ||
| * @ticket 64418 | ||
| * | ||
| * @dataProvider data_custom_css_disallowed | ||
| */ | ||
| public function test_validate_custom_css( string $custom_css, string $expected_error_message ) { | ||
| $controller = new WP_REST_Global_Styles_Controller(); | ||
| $validate = Closure::bind( | ||
| function ( $css ) { | ||
| return $this->validate_custom_css( $css ); | ||
| }, | ||
| $controller, | ||
| $controller | ||
| ); | ||
|
|
||
| $result = $validate( $custom_css ); | ||
| $this->assertWPError( $result ); | ||
| $this->assertSame( $expected_error_message, $result->get_error_message() ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose this isn’t the biggest deal, but it seems fragile to hard-code error messages into our test assertions. perhaps there’s a more-durable code we could check for? or html-decode the error message and look for the offending contents (which still would be fragile because who know if we’ll enforce that aspect in the error message or switch it to something else).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love the hard-coding either. I do want to confirm the correct behavior with the "contain" or "end in" messages and the relevant text snippet that helps make the problem actionable. If you have a good alternative, I'm happy to take a different approach. I didn't want to spend a lot of effort building abstractions to address this. If these tests fail and need to be updated, it should be easy to find and fix them. |
||
| } | ||
|
|
||
| public static function data_custom_css_disallowed() { | ||
sirreal marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return array( | ||
| 'style close tag' => array( 'css…</style>…css', 'The CSS must not contain "</style>".' ), | ||
| 'style close tag upper case' => array( '</STYLE>', 'The CSS must not contain "</STYLE>".' ), | ||
| 'style close tag mixed case' => array( '</sTyLe>', 'The CSS must not contain "</sTyLe>".' ), | ||
| 'style close tag in comment' => array( '/*</style>*/', 'The CSS must not contain "</style>".' ), | ||
| 'style close tag (/)' => array( '</style/', 'The CSS must not contain "</style/".' ), | ||
| 'style close tag (\t)' => array( "</style\t", "The CSS must not contain \"</style\t\"." ), | ||
| 'style close tag (\f)' => array( "</style\f", "The CSS must not contain \"</style\f\"." ), | ||
| 'style close tag (\r)' => array( "</style\r", "The CSS must not contain \"</style\r\"." ), | ||
| 'style close tag (\n)' => array( "</style\n", "The CSS must not contain \"</style\n\"." ), | ||
| 'style close tag (" ")' => array( '</style ', 'The CSS must not contain "</style ".' ), | ||
| 'truncated "<"' => array( '<', 'The CSS must not end in "<".' ), | ||
| 'truncated "</"' => array( '</', 'The CSS must not end in "</".' ), | ||
| 'truncated "</s"' => array( '</s', 'The CSS must not end in "</s".' ), | ||
| 'truncated "</ST"' => array( '</ST', 'The CSS must not end in "</ST".' ), | ||
| 'truncated "</sty"' => array( '</sty', 'The CSS must not end in "</sty".' ), | ||
| 'truncated "</STYL"' => array( '</STYL', 'The CSS must not end in "</STYL".' ), | ||
| 'truncated "</stYle"' => array( '</stYle', 'The CSS must not end in "</stYle".' ), | ||
| ); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.