-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Global Styles: Make the config resilient to changes to safecss_filter_attr()
#30888
Global Styles: Make the config resilient to changes to safecss_filter_attr()
#30888
Conversation
The CSS values in the theme object can not be unsafe, but we also shouldn't be so strict that values that would be accepted by a CSS parser are rejected.
safecss_filter_attr()
safecss_filter_attr()
lib/class-wp-theme-json.php
Outdated
* @param string $property_value Value in a CSS declaration, i.e. the `red` in `color: red`. | ||
* @return boolean | ||
*/ | ||
private static function validate_style_declaration( $property_name, $property_value ) { |
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.
Can we call this is_safe_declaration
or something along those lines?
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.
Agree, it has no side-effects and returns a boolean so should start with is_
I've gone with is_safe_css_declaration
to disambiguate. There's a lot of different types of declaration in this file, and this method assumes it has already been converted into CSS syntax.
private static function validate_style_declaration( $property_name, $property_value ) { | ||
$style_to_validate = $property_name . ': ' . $property_value; | ||
$filtered = esc_html( safecss_filter_attr( $style_to_validate ) ); | ||
return ! empty( trim( $filtered ) ); |
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 a better check, thanks! By reading the safecss_filter_attr
code I've found other cases in which the current code could have failed:
- the input string has some null value (null, backspace)
- the input string has new line characters (
\t
,\r
,\n
) - the input string has leading or trailing whitespace
note that "input string" is property: value
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.
Thanks for taking a look at this, Philip. I've left a comment I'd like us to address (method name) before merging, other than this is ready.
The method was assuming a particular implementation of the safecss_filter_attr() function. In the future it could do something like remove whitespace around the ":", or convert lowercase colours (#abcabc) to uppercase (#ABCABC), which would break saving of Global Styles in Gutenberg. In general it doesn't feel like we should assume filtering functions will return the input unchanged. Switching the semantics of the check from "style was unchanged" to "style was not stripped" fits better with the description of the safecss_filter_attr() function.
Thanks, Philip! This was a good sleuthing and well-scoped PR. |
Description
Unsafe user styles are removed from Global Styles by
WP_Theme_JSON::remove_insecure_properties()
, however the test for whether a style is unsafe is too strict, and makes us more likely to break if WordPress core changes.remove_insecure_properties()
relies on a particular implementation of the coresafecss_filter_attr()
function: that it leaves the CSS unchanged if it's safe. But this is an assumption that could change in the future: maybe it removes whitespace around the:
; or it converts lowercase colours (#abcabc
) to uppercase (#ABCABC
). The output of the filter would still be valid but it would break the saving of Global Styles in Gutenberg. In factsafecss_filter_attr()
already does break this assumption, we just happen to be getting away with it at the moment because of the way we're calling it.In general it doesn't feel like we should assume filtering functions will return the input unchanged.
Switching the semantics of the safety check from "style was unchanged" to "style was not stripped" fits better with the description of the
safecss_filter_attr()
function.This PR:
How has this been tested?
Added a unit test that stretches the
remove_insecure_properties()
method a little to show how it could fail for unexpected reasons. It adds trailing whitespace to a CSS colour declaration, which failed before I made the change. However it didn't fail when adding leading whitespace, just due to the idiosyncrasies of howsafecss_filter_attr()
is implemented. Which is sort of my point.In terms of manual test, this code is exercised when updating and saving Global Styles in the site editor as user without the
unfiltered_html
permission. I've smoke tested this and it works as expected.Types of changes
Bug fix (or code quality)
Checklist:
*.native.js
files for terms that need renaming or removal).CC @nosolosw @vindl