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

Jump to bottom

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

wants to merge 9 commits into from

Conversation

noisysocks
Copy link
Member

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

noisysocks
noisysocks commented Sep 8, 2022
View reviewed changes
src/wp-includes/kses.php Outdated Show resolved Hide resolved
src/wp-includes/kses.php Outdated Show resolved Hide resolved
@noisysocks
Copy link
Member Author

noisysocks commented Sep 8, 2022 •
edited
Loading

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
diff --git a/tests/phpunit/data/images/canola-100x75-jpg.webp b/tests/phpunit/data/images/canola-100x75-jpg.webp
index ef484c7..24bc477 100644
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 @SergeyBiryukov?

edit: lol I accidentally git added a test fixture.

noisysocks
noisysocks commented Sep 8, 2022
View reviewed changes
tests/phpunit/tests/theme/wpThemeJson.php
@@ -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!

@noisysocks
Copy link
Member Author

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

@ockham ockham mentioned this pull request Sep 8, 2022
Closed
37 tasks
@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged in r54100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oandregal oandregal oandregal left review comments

@SergeyBiryukov SergeyBiryukov SergeyBiryukov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
WordPress 6.1 Editor Tasks
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@noisysocks @SergeyBiryukov @oandregal