safecss_filter_attr() returns empty if containing min()

[uxl]

I have block themes that define layout (contentSize and wideSize in theme.json) using min()

The issue is that wp_get_layout_style() returns nothing as safecss_filter_attr() does not recognize min() as valid CSS.

This was not previously an issue because it used to be filtered using wp_strip_all_tags() as gutenberg_get_layout_style() still does when Gutenberg is active.

However there is a TODO note in Gutenberg that it will switch to using safecss_filter_attr in future.

I would ask that min() and any other missing valid CSS be allowed similar to how this was fixed to allow calc() and var()

[50923] / #46197

[johnregan3]

Patch to allow min, max, minmax, and calc, including unit tests

[johnregan3]

@SergeyBiryukov Patch added, building upon #46197. This includes min(), max(), minmax(), and clamp(). I didn't include more CSS functions at this point as I didn't want to stray too far beyond the scope of this ticket. If requested, I can add more.

I also moved some of the test data from #46197 to a more appropriate method.

[isabel_brison]

Thanks for the patch @johnregan3!

I'm wondering if we could fix an issue with var() while we're at it. Currently we don't support adding fallback values to variables, such as var(--my-var, 10px), because we're only allowing alphanumeric characters, dash and underscore as contents for var. It would be good to allow at least commas and brackets (because a fallback value can be another variable).

Would this be doable as part of this ticket? Happy to open a new one if you'd rather not.

[johnregan3]

@isabel_brison Totally! I'll work on this soon.

[johnregan3]

Improve support for var()

[johnregan3]

Minor update.

[johnregan3]

@isabel_brison I've added support for commas, square brackets, and nested var()s.

[andrewserong]

@johnregan3 the patch is testing nicely for me, thanks for putting it together! Just an update that we've adopted this updated Regex in the Gutenberg plugin in ​https://github.com/WordPress/gutenberg/pull/43004 as it resolves issues with style output for the work in progress style engine project: ​https://github.com/WordPress/gutenberg/issues/38167

[ramonopoly]

Do folks think the patch for this ticket will be committed in time for WordPress 6.1?

Otherwise should we migrate the [filter added in Gutenberg]​https://github.com/WordPress/gutenberg/commit/60adc79292c4a8c41ac206152ace4f2dd5faed8f#diff-4afd960d3bcdfb07993478077c20aac762bece548a901eafc7f93b4abae08023R39?

I assume it would be preferable to have the core patch rather than a workaround filter?

Status: BLOCKED by https://core.trac.wordpress.org/ticket/55966#comment:7

DO NOT MERGE until ​https://github.com/WordPress/gutenberg/pull/43840 has landed

This PR migrates the Style Engine into Core for 6.1.

It reflects the latest changes from ​https://github.com/WordPress/gutenberg/pull/43840

## TODO

• [ ] Corresponding Gutenberg PR to switch usage from gutenberg_ and _Gutenberg functions and classes to Core-compatible naming convention

Trac ticket:

[noisysocks]

Yes @ramonopoly definitely better to fix this for real in wp_kses than using a filter.

Nice work on the patch @johnregan3! I applied 55966.2.diff​ locally and ran phpunit --group kses. All green.

The code looks good to me aside from one small thing: @johnregan3, could you please add 'css' => and 'expected' => to the array()s in the tests? That way the code is a little more self-documenting.

Current:

// Allow min().
array(
        'width: min(50%, 400px)',
        'width: min(50%, 400px)',
),

Better:

// Allow min().
array(
        'css'      => 'width: min(50%, 400px)',
        'expected' => 'width: min(50%, 400px)',
),

I can't comment much on the proposed regex other than to say it doesn't look unreasonable. If Gutenberg has been using this regex for some time as @andrewserong says then that's really encouraging.

With that small bit of feedback above fixed I'd be happy to commit this. Out of an abundance of caution, since wp_kses scares me, I'll ping @peterwilsoncc and @hellofromTonya here for a second pair of eyes.

I'm adding early since this blocks ​https://github.com/WordPress/wordpress-develop/pull/3199 which needs to land before beta 1.

[noisysocks]

I'm adding early since this blocks ​https://github.com/WordPress/wordpress-develop/pull/3199 which needs to land before beta 1.

[joyously]

Is there a test case for using a CSS function in something that has parentheses, like a gradient?
Does the regex for var allow a fallback value like 50%? When would a var have square brackets? (maybe a data-uri?)
Is there a test case for two of the functions in the same CSS, like clamp and min?

[SergeyBiryukov]

Replying to noisysocks:

could you please add 'css' => and 'expected' => to the array()s in the tests? That way the code is a little more self-documenting.

Current:

// Allow min().
array(
        'width: min(50%, 400px)',
        'width: min(50%, 400px)',
),

Better:

// Allow min().
array(
        'css'      => 'width: min(50%, 400px)',
        'expected' => 'width: min(50%, 400px)',
),

Good point, 55966.3.diff​ refreshes the patch to address that.

[SergeyBiryukov]

In 54092:

KSES: Allow min(), max(), minmax(), and clamp() values to be used in inline CSS.

Follow-up to [50923].

Props johnregan3, uxl, isabel_brison, andrewserong, ramonopoly, noisysocks, joyously.
See #55966.

[SergeyBiryukov]

The patch is now committed to help unblock Gutenberg backports, as ​requested on Slack.

Keeping the ticket open for now to add a few more test cases as suggested in comment:12. A patch for that would be appreciated :)

[SergeyBiryukov]

In 54093:

KSES: Revert [54092] for now to address unit test failures.

See #55966.

[SergeyBiryukov]

It looks like the tests for WP_Theme_JSON::remove_insecure_properties() need some adjustments. as they ​fail with these changes.

Since var(--color, var(--unsafe-fallback)) is now allowed per comment:3, I guess these would need some new examples of unsafe values?

There were 3 failures:

1) Tests_Theme_wpThemeJson::test_remove_insecure_properties_removes_unsafe_styles
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
             'core/group' => Array (...)
+            'core/cover' => Array (...)
         )
     )
     'version' => 2
 )

/var/www/tests/phpunit/includes/abstract-testcase.php:1006
/var/www/tests/phpunit/tests/theme/wpThemeJson.php:1665
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
/var/www/vendor/bin/phpunit:118

2) Tests_Theme_wpThemeJson::test_remove_insecure_properties_removes_unsafe_styles_sub_properties
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
                 'bottomLeft' => '6px'
+                'topRight' => 'var(--top-right, var(--unsafe...back))'
@@ @@
                 'left' => '1px'
+                'bottom' => 'var(--bottom, var(--unsafe-fallback))'
@@ @@
                         'left' => '2px'
+                        'bottom' => 'var(--bottom, var(--unsafe-fallback))'
@@ @@
                         'bottomLeft' => '5px'
+                        'topRight' => 'var(--top-right, var(--unsafe...back))'
@@ @@
                                 'left' => '4px'
+                                'bottom' => 'var(--bottom, var(--unsafe-fallback))'
                             )
                         )
                     )
                 )
             )
         )
     )
     'version' => 2
 )

/var/www/tests/phpunit/includes/abstract-testcase.php:1006
/var/www/tests/phpunit/tests/theme/wpThemeJson.php:1800
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
/var/www/vendor/bin/phpunit:118

3) Tests_Theme_wpThemeJson::test_remove_insecure_properties_removes_unsafe_preset_settings
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
                     0 => Array (
-                        'name' => 'Pink'
-                        'slug' => 'pink'
-                        'color' => '#FFC0CB'
+                        'name' => 'Blue'
+                        'slug' => 'blue'
+                        'color' => 'var(--color, var(--unsafe-fallback))'
                     )
+                    1 => Array (...)
@@ @@
                     0 => Array (...)
+                    1 => Array (...)
@@ @@
                             0 => Array (
-                                'name' => 'Pink'
-                                'slug' => 'pink'
-                                'color' => '#FFC0CB'
+                                'name' => 'Blue'
+                                'slug' => 'blue'
+                                'color' => 'var(--color, var(--unsafe--fallback))'
                             )
+                            1 => Array (...)
                         )
                     )
                 )
             )
         )
     )
     'version' => 2
 )

/var/www/tests/phpunit/includes/abstract-testcase.php:1006
/var/www/tests/phpunit/tests/theme/wpThemeJson.php:2059
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
/var/www/vendor/bin/phpunit:118

[Bernhard Reiter]

Replying to SergeyBiryukov:

The patch is now committed to help unblock Gutenberg backports, as ​requested on Slack.

Thanks a lot for committing, Sergey!

Keeping the ticket open for now to add a few more test cases as suggested in comment:12. A patch for that would be appreciated :)

@cbravobernal is looking into that :)

[cbravobernal]

I just added some more use cases, some of them with unsafe fallback values. Let me know if we have to add more!

[SergeyBiryukov]

Replying to cbravobernal:

I just added some more use cases, some of them with unsafe fallback values. Let me know if we have to add more!

Thanks! 55966.4.diff​ looks good to me.

I had to revert the original commit for now due the WP_Theme_JSON::remove_insecure_properties() test failures, see comment:17. Removing those tests did not seem like the correct approach, I think they would need some new examples of unsafe values. Let's address that and re-commit the patch :)

[cbravobernal]

Replying to SergeyBiryukov:

I had to revert the original commit for now due the WP_Theme_JSON::remove_insecure_properties() test failures, see comment:17. Removing those tests did not seem like the correct approach, I think they would need some new examples of unsafe values. Let's address that and re-commit the patch :)

Happy to look at that :)

[peterwilsoncc]

@noisysocks and I were discussing this yesterday and may have some additional test cases (currently failing) to add to the unit tests. I'll try to get some notes on this ticket today.

Trac ticket: https://core.trac.wordpress.org/ticket/55966

[noisysocks]

I opened a PR containing 55966.3.diff​ and 55966.4.diff​ so that we can more easily review the code and have CI run against the changes.

​https://github.com/WordPress/wordpress-develop/pull/3212

@peterwilsoncc flagged to me yesterday some additional failing tests that he'd like to include so I'll work with him on including those and committing this today.

To be honest I don't really like how safecss_filter_attr works. Denying all CSS that contains \, (, &, }, =, and /* seems too restrictive and is forcing us to constantly add exceptions for new CSS functions like var() and min(). I’d like to change how that function works but need to consult more with @peterwilsoncc and @azaozz to work out what the actual security concerns are with inline CSS. (evaluate() comes to mind.) But I'll create a new ticket for this discussion as right now we just have to get min, max, etc. working so that the Gutenberg team is unblocked.

Thanks @costdev for volunteering and taking the time to review the code, and also for explaining the required changes 🙇

I think I've covered just about everything, but will take another sweep over later to spot if I've missed something.

The final failing test should pass once https://core.trac.wordpress.org/ticket/55966 has been committed.

I added a few more test cases to this, combined the two regexes that handle CSS functions, and rewrote said regex so that it checks for balanced parentheses.

I'm not sure what this CI failure is about:

Run git diff --exit-code
[https://github.com/WordPress/wordpress-develop/runs/8242581342?check_suite_focus=true#step:24:13 12]
diff --git a/tests/phpunit/data/images/canola-100x75-jpg.webp b/tests/phpunit/data/images/canola-100x75-jpg.webp
[https://github.com/WordPress/wordpress-develop/runs/8242581342?check_suite_focus=true#step:24:14 13]
index ef484c7..24bc477 100644
[https://github.com/WordPress/wordpress-develop/runs/8242581342?check_suite_focus=true#step:24:15 14]
Binary files a/tests/phpunit/data/images/canola-100x75-jpg.webp and b/tests/phpunit/data/images/canola-100x75-jpg.webp differ

Do you know @hellofromtonya @peterwilsoncc?

edit: lol I accidentally git added a test fixture.

Tests passing! I’m pretty happy with this. What do you think @SergeyBiryukov?

[SergeyBiryukov]

In 54100:

KSES: Allow min(), max(), minmax(), and clamp() values to be used in inline CSS.

Additionally, this commit updates safecss_filter_attr() to add support for nested var() functions, so that a fallback value can be another CSS variable.

Follow-up to [50923].

Props johnregan3, noisysocks, cbravobernal, uxl, isabel_brison, andrewserong, ramonopoly, joyously, bernhard-reiter, peterwilsoncc.
Fixes #55966.

Thanks for the PR! Merged in r54100.

I'll fix up those failing tests later. Last commit was a bit rushed.

Thanks for the reviews, folks.

Also, the GHA failures don't seem to be test failures, but workflow failures, so it looks like your last commit was clean

Good to know! I was on my phone so wasn't sure. I see red and 😱

I might not be around next week. cc @andrewserong in case this PR and related PRs need any attention, though I think we should be sound 👍

LGTM pending the result of ​this discussion. 👍 Thanks @ramonjd!

That link doesn't seem to be working for me 😕 @costdev -- did you mean ​this discussion (about @access private and classes being final)? If so, has that been sufficiently ​answered by @ramonjd?

I'd love if we could land this PR soon, as it blocks a number of other Style Engine related PRs 😊
@andrewserong maybe you can help out a bit? 🙏

LGTM pending the result of ​this discussion. 👍 Thanks @ramonjd!

That link doesn't seem to be working for me 😕 @costdev -- did you mean ​this discussion (about @access private and classes being final)? If so, has that been sufficiently ​answered by @ramonjd?

I'd love if we could land this PR soon, as it blocks a number of other Style Engine related PRs 😊
@andrewserong maybe you can help out a bit? 🙏

That's right @ockham, not sure why the original link isn't working.

I think we're waiting on feedback from @aristath and @andrewsong on that topic. Not sure if @azaozz has anything more to add as well.

I think we're waiting on feedback from @aristath and @AndrewSong on that topic.

For my part, I've been thinking about this, and I'm pretty confident that the function signatures and method names are fine for now. The classes have been running in Gutenberg for some time.

Ultimately the purpose of all this code is to accept block style input and output compiled CSS. I expect there'll be updates to the body of some functions, and probably unforeseen future conditions that will require new methods, and later, deprecations, but that's the story of WordPress.

The unknowns will remain unknowns :D

I'd lean on @aristath's and @AndrewSong's opinion though.

Thanks for pings!

For my part, I've been thinking about this, and I'm pretty confident that the function signatures and method names are fine for now. The classes have been running in Gutenberg for some time.

I agree, and like you mention in the thread, maintaining backwards compatibility is expected for the style engine classes. One of the things I believe @aristath had in mind with these classes was the eventual use cases of plugins and themes, so we ultimately do want these to be stable and well-supported classes with consistent public methods. The structure and naming of the public methods went through lots of iterations in the Gutenberg plugin and appears to be pretty stable now from my perspective.

I think this PR looks in a good shape to land, personally.

As mentioned above, the Style Engine classes went through a lot of iterations and discussion. What we ended up with is a hierarchy and structure that is solid, stable, makes sense, and I feel it can withstand the test of time 👍

Committed in https://core.trac.wordpress.org/changeset/54156