Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#55966 closed defect (bug) (fixed)

safecss_filter_attr() returns empty if containing min()

Reported by: uxl's profile uxl Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: major Version: 6.0
Component: Formatting Keywords: has-patch early has-unit-tests add-to-field-guide
Focuses: css Cc:

Description (last modified by SergeyBiryukov)

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

Attachments (5)

55966.diff (3.3 KB) - added by johnregan3 2 years ago.
Patch to allow min, max, minmax, and calc, including unit tests
55966.1.diff (3.8 KB) - added by johnregan3 2 years ago.
Improve support for var()
55966.2.diff (3.8 KB) - added by johnregan3 2 years ago.
Minor update.
55966.3.diff (4.2 KB) - added by SergeyBiryukov 2 years ago.
55966.4.diff (2.7 KB) - added by cbravobernal 2 years ago.

Download all attachments as: .zip

Change History (45)

#1 @SergeyBiryukov
2 years ago

  • Component changed from General to Formatting
  • Description modified (diff)
  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.1

@johnregan3
2 years ago

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

#2 @johnregan3
2 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

@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.

#3 @isabel_brison
2 years ago

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.

Last edited 2 years ago by isabel_brison (previous) (diff)

#4 @johnregan3
2 years ago

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

@johnregan3
2 years ago

Improve support for var()

@johnregan3
2 years ago

Minor update.

#5 @johnregan3
2 years ago

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

#6 @andrewserong
2 years ago

@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

#7 follow-up: @ramonopoly
2 years ago

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?

This ticket was mentioned in PR #3199 on WordPress/wordpress-develop by ramonjd.


2 years ago
#8

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:

#9 in reply to: ↑ 7 ; follow-up: @noisysocks
2 years ago

  • Keywords changes-requested added

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.

Last edited 2 years ago by noisysocks (previous) (diff)

#10 @noisysocks
2 years ago

  • Keywords early added

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

This ticket was mentioned in Slack in #core by bernhardreiter. View the logs.


2 years ago

#12 @joyously
2 years ago

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?

#13 in reply to: ↑ 9 @SergeyBiryukov
2 years ago

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.

#14 @SergeyBiryukov
2 years ago

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.

#15 follow-up: @SergeyBiryukov
2 years ago

  • Keywords needs-unit-tests added; has-unit-tests changes-requested removed
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

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 :)

#16 @SergeyBiryukov
2 years ago

In 54093:

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

See #55966.

#17 @SergeyBiryukov
2 years ago

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

@cbravobernal
2 years ago

#18 in reply to: ↑ 15 @Bernhard Reiter
2 years ago

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 :)

#19 follow-up: @cbravobernal
2 years ago

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

#20 in reply to: ↑ 19 ; follow-up: @SergeyBiryukov
2 years ago

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 :)

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#21 in reply to: ↑ 20 @cbravobernal
2 years ago

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 :)

#22 @peterwilsoncc
2 years ago

@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.

This ticket was mentioned in PR #3212 on WordPress/wordpress-develop by noisysocks.


2 years ago
#23

  • Keywords has-unit-tests added; needs-unit-tests removed

#24 @noisysocks
2 years ago

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.

ramonjd commented on PR #3199:


2 years ago
#25

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.

noisysocks commented on PR #3212:


2 years ago
#26

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.

noisysocks commented on PR #3212:


2 years ago
#27

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

#28 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

SergeyBiryukov commented on PR #3212:


2 years ago
#29

Thanks for the PR! Merged in r54100.

ramonjd commented on PR #3199:


2 years ago
#30

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

Thanks for the reviews, folks.

ramonjd commented on PR #3199:


2 years ago
#31

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 😱

ramonjd commented on PR #3199:


2 years ago
#32

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 👍

ockham commented on PR #3199:


2 years ago
#33

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? 🙏

ockham commented on PR #3199:


2 years ago
#34

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? 🙏

costdev commented on PR #3199:


2 years ago
#35

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.

ramonjd commented on PR #3199:


2 years ago
#36

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.

andrewserong commented on PR #3199:


2 years ago
#37

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.

aristath commented on PR #3199:


2 years ago
#38

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 👍

#40 @milana_cap
2 years ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.