-
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
base: trunk
Are you sure you want to change the base?
Global Styles: Allow arbitrary CSS, protect from KSES mangling #10641
Conversation
STYLE tags may contain any raw text up to a closing STYLE tag https://html.spec.whatwg.org/multipage/parsing.html#generic-raw-text-element-parsing-algorithm
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
src/wp-includes/customize/class-wp-customize-custom-css-setting.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
Strictly speaking, a `STYLE` tag will be closed by a `STYLE` close tag. However, the CSS contents are concatenated together and the strict check for a STYLE close tag may later be invalidated by concatenation.
c0dab9b to
99b8733
Compare
src/wp-includes/customize/class-wp-customize-custom-css-setting.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
|
As I explored the space, this PR grew in scope. There are at least two concerns that I plan to split up:
|
Co-authored-by: Weston Ruter <westonruter@gmail.com>
|
YES! KSES indeed breaks this as can be seen in the multisite tests: That's almost certainly a missing |
|
I've updated the check to disallow:
These changes should make the style tag contents safe to save, print, and concatenate with other arbitrary contents even without the STYLE tag change in #10656. They are safe to port to Gutenberg. (This was another approach I discussed with @dmsnell and it seems like the most appropriate.) |
ramonjd
left a comment
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.
This is working well for me. Do folks have any other test cases or concerns?
I've prepped a Gutenberg PR over at
I think this could go in at any time.
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Show resolved
Hide resolved
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.
Pull request overview
This pull request enhances the Global Styles custom CSS handling to allow arbitrary CSS content while protecting it from corruption by WordPress KSES HTML filtering. The solution uses JSON encoding flags (JSON_HEX_TAG and JSON_HEX_AMP) to escape characters that could be mangled when KSES processes the post content as HTML.
Key changes:
- Improved CSS validation to allow legitimate CSS containing
<and&characters while blocking actual</style>closing tags - Added JSON encoding flags to escape potentially problematic characters in both REST API and KSES filter functions
- Comprehensive test coverage for validation logic including edge cases
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php |
Replaced strict regex validation with precise algorithm that allows CSS with HTML-like syntax while blocking style tag closure; added JSON_HEX_TAG and JSON_HEX_AMP flags to JSON encoding |
src/wp-includes/kses.php |
Added same JSON encoding flags to wp_filter_global_styles_post() for consistency with REST API |
tests/phpunit/tests/rest-api/rest-global-styles-controller.php |
Added comprehensive test coverage for new validation logic including allowed CSS patterns and various forms of disallowed style tag closures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
| $at += strcspn( $css, '<', ++$at ) | ||
| ) { | ||
| $remaining_strlen = $length - $at; | ||
| $possible_style_close_tag = 0 === substr_compare( $css, '</style', $at, min( 7, $remaining_strlen ), true ); |
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.
noted in the backport PR, what is the purpose of the min() here? It seems like we would want to always use 7 because anything less than that, a prefix of </style does not break out of the STYLE element.
is this an attempt at being extra cautious?
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.
The text is concatenated with other text to form the contents of the STYLE tag. Usually, WordPress will concatenate with newlines, however I believe I've also seen cases from WordPress that are joined with no separation. The idea is that the partial matches could combine with subsequent text to form dangerous STYLE tag contents. I also don't believe it's valid to end any CSS in these partial matches.
If some text ends in </ (invalid) and another starts with style {} (unusual, but valid), it would be a problem if they're joined to form </style {} as this would close the STYLE element.
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.
sounds good, but I think this warrants an explanation in the code. what you wrote here is almost perfect.
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 added a descriptive comment:
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 text could complete it, forming a valid</style>tag.
Co-authored-by: Weston Ruter <westonruter@gmail.com>
|
|
||
| $result = $validate( $custom_css ); | ||
| $this->assertWPError( $result ); | ||
| $this->assertSame( $expected_error_message, $result->get_error_message() ); |
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 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).
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 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.
dmsnell
left a comment
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.
this seems sound, and I think people will be really happy with the more-permissive rules. I don’t see any obvious issues with the escaping or security side, but I also didn’t anticipate the issue with concatenation which you did, so maybe there are more? I think this is safer in every way since this only impacts encoding and doesn’t open up any permissive decoding.
though I left some comments, I don’t consider them blockers. if you were to incorporate them my approval would still stand.
src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
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 flagsJSON_HEX_TAGandJSON_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:
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.