Style engine: permit wp custom CSS properties

[ramonjd]

What?

A slight variation on @aristath's idea to add a custom CSS property check in the style engine.

How

Adds an array VALID_CUSTOM_PROPERTIES of valid CSS custom properties to check against.

Why?

CSS custom properties don't pass the tests in safecss_filter_attr() at if ( in_array( $css_selector, $allowed_attr, true ) ) {.

Testing Instructions

Run

npm run test:unit:php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-css-declarations-test.php

[ramonjd]

This approach might be completely bonkers. Hoping that someone will tell me 😄

[andrewserong]

Thanks for exploring this @ramonjd! Are you imagining this as a replacement / alternative to #43004, or a follow-up?

The style engine code can eventually be deleted when core supports CSS custom properties.

Is there a trac ticket currently proposing adding being able to set CSS custom properties from safecss_filter_attr? My understanding was that it was intentionally not allowed, but referencing an existing variable via var() was allowed.

For cases like the Gallery block, and the custom properties in global styles, I wonder if we should include a style-engine specific allow list and/or use a "already validated" flag of some kind when we generate declarations. The idea being that if we're allowing things that safecss_filter_attr doesn't allow, then we might want to be quite specific about it for the intended use cases, rather than trying to come up with something that exposes the behaviour outside of how we're generating styles for Gutenberg?

[ramonjd]

Thanks for thinking about this @andrewserong

Are you imagining this as a replacement / alternative to #43004, or a follow-up?

It was supposed to be a follow up/enhancement.

The quest here is to permit --wp--* custom properties.

Is there a trac ticket currently proposing adding being able to set CSS custom properties from safecss_filter_attr?
My understanding was that it was intentionally not allowed, but referencing an existing variable via var() was allowed.

All I could find was https://core.trac.wordpress.org/ticket/46498, but there's no patch attached.

I couldn't find anything to support the argument against or for allowing CSS custom properties.

I guess that's why the safe_style_css hook exists (?).

I wonder if we should include a style-engine specific allow list and/or use a "already validated" flag of some kind when we generate declarations. The idea being that if we're allowing things that safecss_filter_attr doesn't allow, then we might want to be quite specific about it for the intended use cases, rather than trying to come up with something that exposes the behaviour outside of how we're generating styles for Gutenberg?

This is the lo-fi version, and personally I think it'd be 100% fine. And to be honest, starting off more simply is probably better.

A couple of related points come to mind:

• we'd need to let folks know they have to maintain this list and associated tests when adding or removing new CSS properties.
• for theme authors and others wishing to use the style engine, we might have to add a filter so they can add their own valid properties.
• do we need to validate/sanitize the custom prop values?

The primary intention of this PR, regardless of whether safecss_filter_attr() will ever support CSS custom properties, was to run all values through safecss_filter_attr for consistency, seeing as we're doing it for all other values. safecss_filter_attr parses things like url, gradient and calc in value strings.

[andrewserong]

All I could find was https://core.trac.wordpress.org/ticket/46498, but there's no patch attached.

It's an interesting one. From that discussion it looks like only calc() and var() were added in this changeset: https://core.trac.wordpress.org/changeset/50923

I wasn't able to find any other discussion either, I'm sorry — I might've been thinking about previously Github PR discussions when folks have previously tried to add declaring CSS variables, but my memory on where or when is foggy! 😅

for theme authors and others wishing to use the style engine, we might have to add a filter so they can add their own valid properties.

Good points. I think the theme author case is one of the reasons that I'm wondering if defining at call time rather than by a filter might make sense (e.g. kind of an equivalent to React's dangerouslySetInnerHTML), so that the required allowed HTML can be passed through directly without needing an explicit list. For themes or similar cases, I suspect they'll be hardcoding a lot of values rather than using user-entered values, so the security implications are probably a lot less in those particular cases.

The primary intention of this PR, regardless of whether safecss_filter_attr() will ever support CSS custom properties, was to run all values through safecss_filter_attr for consistency, seeing as we're doing it for all other values.

For our current purposes, that sounds like a good idea to me — for block supports and global styles where there are user entered values, we do want those to be parsed at some point whether just prior to calling the style engine, or as part of the style engine itself. I'm not quite sure how we'd do this, but I wonder if there's some way for us to treat the desired CSS variable as the desired real CSS property... unfortunately given that declarations are currently key => value pairs, we don't have a way of adding metadata when adding a declaration 🤔. Maybe if we were to move the validation to when we add a declaration instead of when we output it as a string?

For the global styles case, I suppose one of the other things to consider is that the settings and styles should already be sanitized before the style engine is called (e.g. in remove_insecure_settings and remove_secure_styles) so we might arguably have another case there where we want the style engine to pass through values rather than attempt to filter / sanitize them a second time? There's a neat bit of logic on this line where the "real" property associated with the CSS variable is used for the safe CSS declaration check: https://github.com/WordPress/wordpress-develop/blob/1cffa3f82b2de2df13e89767a4714de6fbe3de78/src/wp-includes/class-wp-theme-json.php#L1911

[ramonjd]

Thanks a lot for doing the heavy thinking on this @andrewserong !

I think you've convinced me to go the hard-coded route for now. We can always make it clever later.

I guess with validation I was really thinking of 3rd party usage. Maybe later, as you say, we can work on a better validation method for the style engine as a separate task.

🙇 🙇 🙇 🙇 🙇 🙇 🙇

[ramonjd]

Related trac ticket https://core.trac.wordpress.org/ticket/56353 Props to @aristath

[ramonjd]

Looks like CSS custom properties will be supported in 6.1

https://core.trac.wordpress.org/ticket/56353 has been committed to WordPress trunk 🎉

[ramonjd]

I've updated this PR with backward compatibility comments.

I'm guessing Gutenberg will still need to support WordPress < 6.1 so, to continue development using CSS custom props, this PR is still valid.

[andrewserong]

Just took this for another spin and quick smoke test, all looking good to me!

✅ Logic reads well, nice choice to just go with an allow list for the properties we want to deal with for the moment
✅ Tests are passing, and confirmed that https://core.trac.wordpress.org/ticket/56353 links to the correct fix in core for 6.1

LGTM! ✨