Skip to content
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.

Sign up for GitHub

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

safecss_filter_attr: Add exceptions for min(), max(), etc. functions #3212

Closed
wants to merge 9 commits into from
Closed

safecss_filter_attr: Add exceptions for min(), max(), etc. functions #3212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0afb69f
55966.3.diff
noisysocks Sep 7, 2022
45a9bee
55966.4.diff
noisysocks Sep 7, 2022
78b4eeb
Add a `@since` note.
SergeyBiryukov Sep 7, 2022
a59b4a7
Ensure ()s are balanced
noisysocks Sep 8, 2022
80d4c9e
Use a different invalid CSS in WP_Theme_JSON tests
noisysocks Sep 8, 2022
170ee45
Use non-capturing group
noisysocks Sep 8, 2022
313ee8a
Simplify regex
noisysocks Sep 8, 2022
77b5880
Remove image that somehow got in here
noisysocks Sep 8, 2022
3724998
Update comments per the doc standards.
SergeyBiryukov Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions src/wp-includes/kses.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2228,6 +2228,8 @@ function kses_init() {
* @since 5.3.1 Added support for gradient backgrounds.
* @since 5.7.1 Added support for `object-position`.
* @since 5.8.0 Added support for `calc()` and `var()` values.
* @since 6.1.0 Added support for `min()`, `max()`, `minmax()`, `clamp()`,
* and nested `var()` values.
*
* @param string $css A string of CSS rules.
* @param string $deprecated Not used.
Expand Down Expand Up @@ -2467,13 +2469,20 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
}

if ( $found ) {
// Allow CSS calc().
$css_test_string = preg_replace( '/calc\(((?:\([^()]*\)?|[^()])*)\)/', '', $css_test_string );
// Allow CSS var().
$css_test_string = preg_replace( '/\(?var\(--[a-zA-Z0-9_-]*\)/', '', $css_test_string );
/*
* Allow CSS functions like var(), calc(), etc. by removing them from the test string.
* Nested functions and parentheses are also removed, so long as the parentheses are balanced.
*/
$css_test_string = preg_replace(
'/\b(?:var|calc|min|max|minmax|clamp)(\((?:[^()]|(?1))*\))/',
'',
$css_test_string
);

// Check for any CSS containing \ ( & } = or comments,
// except for url(), calc(), or var() usage checked above.
/*
* Disallow CSS containing \ ( & } = or comments, except for within url(), var(), calc(), etc.
* which were removed from the test string above.
*/
$allow_css = ! preg_match( '%[\\\(&=}]|/\*%', $css_test_string );

/**
Expand Down
146 changes: 116 additions & 30 deletions tests/phpunit/tests/kses.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,6 +935,7 @@ public function test_wp_kses_attr_no_attributes_allowed_with_false() {
* @ticket 37248
* @ticket 42729
* @ticket 48376
* @ticket 55966
* @dataProvider data_test_safecss_filter_attr
*
* @param string $css A string of CSS rules.
Expand Down Expand Up @@ -1120,6 +1121,121 @@ public function data_test_safecss_filter_attr() {
'css' => 'color: rgb( 100, 100, 100, .4 )',
'expected' => '',
),
// Allow min().
array(
'css' => 'width: min(50%, 400px)',
'expected' => 'width: min(50%, 400px)',
),
// Allow max().
array(
'css' => 'width: max(50%, 40rem)',
'expected' => 'width: max(50%, 40rem)',
),
// Allow minmax().
array(
'css' => 'width: minmax(100px, 50%)',
'expected' => 'width: minmax(100px, 50%)',
),
// Allow clamp().
array(
'css' => 'width: clamp(100px, 50%, 100vw)',
'expected' => 'width: clamp(100px, 50%, 100vw)',
),
// Allow two functions in the same CSS.
array(
'css' => 'width: clamp(min(100px, 350px), 50%, 500px), 600px)',
'expected' => 'width: clamp(min(100px, 350px), 50%, 500px), 600px)',
),
// Allow gradient() function.
array(
'css' => 'background: linear-gradient(90deg, rgba(2,0,36,1) 0%, rgba(9,9,121,1) 35%, rgba(0,212,255,1) 100%)',
'expected' => 'background: linear-gradient(90deg, rgba(2,0,36,1) 0%, rgba(9,9,121,1) 35%, rgba(0,212,255,1) 100%)',
),
// Combined CSS function names.
array(
'css' => 'width: calcmax(100px + 50%)',
'expected' => '',
),
// Allow calc().
array(
'css' => 'width: calc(2em + 3px)',
'expected' => 'width: calc(2em + 3px)',
),
// Allow calc() with nested brackets.
array(
'css' => 'width: calc(3em + (10px * 2))',
'expected' => 'width: calc(3em + (10px * 2))',
),
// Allow var().
array(
'css' => 'padding: var(--wp-var1) var(--wp-var2)',
'expected' => 'padding: var(--wp-var1) var(--wp-var2)',
),
// Allow var() with fallback (commas).
array(
'css' => 'padding: var(--wp-var1, 10px)',
'expected' => 'padding: var(--wp-var1, 10px)',
),
// Allow var() with fallback (percentage).
array(
'css' => 'padding: var(--wp-var1, 50%)',
'expected' => 'padding: var(--wp-var1, 50%)',
),
// Allow var() with fallback var().
array(
'css' => 'background-color: var(--wp-var, var(--wp-var-fallback, pink))',
'expected' => 'background-color: var(--wp-var, var(--wp-var-fallback, pink))',
),
// Allow var() with square brackets.
array(
'css' => 'background-color: var(--wp-var, [pink])',
'expected' => 'background-color: var(--wp-var, [pink])',
),
// Allow calc() with var().
array(
'css' => 'margin-top: calc(var(--wp-var1) * 3 + 2em)',
'expected' => 'margin-top: calc(var(--wp-var1) * 3 + 2em)',
),
// Malformed min, no closing `)`.
array(
'css' => 'width: min(3em + 10px',
'expected' => '',
),
// Malformed max, no closing `)`.
array(
'css' => 'width: max(3em + 10px',
'expected' => '',
),
// Malformed minmax, no closing `)`.
array(
'css' => 'width: minmax(3em + 10px',
'expected' => '',
),
// Malformed calc, no closing `)`.
array(
'css' => 'width: calc(3em + 10px',
'expected' => '',
),
// Malformed var, no closing `)`.
array(
'css' => 'width: var(--wp-var1',
'expected' => '',
),
// Malformed calc, mismatching brackets.
array(
'css' => 'width: calc(3em + (10px * 2)',
'expected' => '',
),
// Malformed var, mismatching brackets.
array(
'css' => 'background-color: var(--wp-var, var(--wp-var-fallback, pink)',
'expected' => '',
),
// Don't allow expressions outside of a calc().
array(
'css' => 'width: (3em + (10px * 2))',
'expected' => '',
),
);
}

Expand Down Expand Up @@ -1301,24 +1417,6 @@ public function data_kses_style_attr_with_url() {
'background: red',
),

// CSS calc().
array(
'width: calc(2em + 3px)',
'width: calc(2em + 3px)',
),

// CSS variable.
array(
'padding: var(--wp-var1) var(--wp-var2)',
'padding: var(--wp-var1) var(--wp-var2)',
),

// CSS calc() with var().
array(
'margin-top: calc(var(--wp-var1) * 3 + 2em)',
'margin-top: calc(var(--wp-var1) * 3 + 2em)',
),

/*
* Invalid use cases.
*/
Expand Down Expand Up @@ -1382,18 +1480,6 @@ public function data_kses_style_attr_with_url() {
'background-image: url( "http://example.com );',
'',
),

// Malformed calc, no closing `)`.
array(
'width: calc(3em + 10px',
'',
),

// Malformed var, no closing `)`.
array(
'width: var(--wp-var1',
'',
),
);
}

Expand Down
20 changes: 10 additions & 10 deletions tests/phpunit/tests/theme/wpThemeJson.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1602,7 +1602,7 @@ public function test_remove_insecure_properties_removes_unsafe_styles() {
),
'core/cover' => array(
'filter' => array(
'duotone' => 'var(--wp--preset--duotone--blue-red, var(--fallback-unsafe))',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes to the WP_Theme_JSON tests are similar to WordPress/gutenberg#31982.

We were relying on the fact safecss_filter_attr doesn't support nested var() functions in order to test that attributes were being passed through safecss_filter_attr. Since this PR makes it so that safecss_filter_attr supports nested var() functions, we need to change the test cases to a different invalid CSS string.

cc. @oandregal to make sure I'm not mistaken 😀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is my understanding too 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, changes make sense. Thanks!

'duotone' => 'var(--invalid',
),
),
'core/group' => array(
Expand Down Expand Up @@ -1676,7 +1676,7 @@ public function test_remove_insecure_properties_removes_unsafe_styles_sub_proper
'border' => array(
'radius' => array(
'topLeft' => '6px',
'topRight' => 'var(--top-right, var(--unsafe-fallback))',
'topRight' => 'var(--invalid',
'bottomRight' => '6px',
'bottomLeft' => '6px',
),
Expand All @@ -1685,7 +1685,7 @@ public function test_remove_insecure_properties_removes_unsafe_styles_sub_proper
'padding' => array(
'top' => '1px',
'right' => '1px',
'bottom' => 'var(--bottom, var(--unsafe-fallback))',
'bottom' => 'var(--invalid',
'left' => '1px',
),
),
Expand All @@ -1695,7 +1695,7 @@ public function test_remove_insecure_properties_removes_unsafe_styles_sub_proper
'padding' => array(
'top' => '2px',
'right' => '2px',
'bottom' => 'var(--bottom, var(--unsafe-fallback))',
'bottom' => 'var(--invalid',
'left' => '2px',
),
),
Expand All @@ -1706,7 +1706,7 @@ public function test_remove_insecure_properties_removes_unsafe_styles_sub_proper
'border' => array(
'radius' => array(
'topLeft' => '5px',
'topRight' => 'var(--top-right, var(--unsafe-fallback))',
'topRight' => 'var(--invalid',
'bottomRight' => '5px',
'bottomLeft' => '5px',
),
Expand All @@ -1715,7 +1715,7 @@ public function test_remove_insecure_properties_removes_unsafe_styles_sub_proper
'padding' => array(
'top' => '3px',
'right' => '3px',
'bottom' => 'var(bottom, var(--unsafe-fallback))',
'bottom' => 'var(--invalid',
'left' => '3px',
),
),
Expand All @@ -1725,7 +1725,7 @@ public function test_remove_insecure_properties_removes_unsafe_styles_sub_proper
'padding' => array(
'top' => '4px',
'right' => '4px',
'bottom' => 'var(--bottom, var(--unsafe-fallback))',
'bottom' => 'var(--invalid',
'left' => '4px',
),
),
Expand Down Expand Up @@ -1944,7 +1944,7 @@ public function test_remove_insecure_properties_removes_unsafe_preset_settings()
array(
'name' => 'Blue',
'slug' => 'blue',
'color' => 'var(--color, var(--unsafe-fallback))',
'color' => 'var(--invalid',
),
array(
'name' => 'Pink',
Expand Down Expand Up @@ -1975,7 +1975,7 @@ public function test_remove_insecure_properties_removes_unsafe_preset_settings()
array(
'name' => 'Helvetica Arial',
'slug' => 'helvetica-arial',
'fontFamily' => 'var(--fontFamily, var(--unsafe-fallback))',
'fontFamily' => 'var(--invalid',
),
),
),
Expand All @@ -1998,7 +1998,7 @@ public function test_remove_insecure_properties_removes_unsafe_preset_settings()
array(
'name' => 'Blue',
'slug' => 'blue',
'color' => 'var(--color, var(--unsafe--fallback))',
'color' => 'var(--invalid',
),
array(
'name' => 'Pink',
Expand Down