KSES: Add support for rgb(a) color.#3097
Conversation
|
Hi @t-hamano! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've added a few notes inline.
src/wp-includes/kses.php
Outdated
|
|
||
| if ( $found && $rgba_attr ) { | ||
| $css_value = trim( $parts[1] ); | ||
| if ( preg_match( '/^rgba\((\d{1,3}%?),\s*(\d{1,3}%?),\s*(\d{1,3}%?),\s*(\d*(?:\.\d+)?)\)$/', $css_value ) ) { |
There was a problem hiding this comment.
Is it possible to accept both rgb and rgba? It will be super confusing to people as to why one is accepted but not the other. rgb now accepts a forth argument for transparency so they're essentially the same function. See the MDN color docs.
This will require some changes to the test_safecss_filter_attr_filtered data provider to promote rgb to be always allowed.
| array( | ||
| 'css' => 'background-color: rgba(0, 0, 0, .1)', | ||
| 'expected' => 'background-color: rgba(0, 0, 0, .1)', | ||
| ), |
There was a problem hiding this comment.
There's an test below under the comment "RGBA color values are not allowed." that will now fail.
It would be good to test for some malformed values too.
There was a problem hiding this comment.
I am thinking about regular expression and test cases, am I correct in this perception?
✅ color: rgba(0, 0, 0 / 0.1)
❌ color: rgba(0, 0, 0/ 0.1) (don't allow no space before slash)
❌ color: rgba(0, 0, 0 /0.1) (don't allow no space after slash)
✅ color: rgba(0, 0, 0, 0)
✅ color: rgba(0,0,0,0) (allow no space after comma)
❌ color: rgba(0 , 0, 0, 0) (don't allow space before comma)
❌ color: rgba(0, 0, 0, 0) (don't allow multiple spaces after comma *it would appear as single space on the comments)
✅ color: rgba(100%, 0, 0, 0)
❌ color: rgba(100px, 0, 0, 0) (don't allow units other than %)
❌ color: rgba(red, 0, 0, 0) (don't allow string value)
According to MDN, I believe the same rules can be applied to both rgb() and rgba().
There was a problem hiding this comment.
I am thinking about regular expression and test cases, am I correct in this perception?
Yes, you are. These are the sort of tests I was suggesting.
You may also wish to include something containing either a " or a ' as an attempt to close the style tag. The expectation is that kses would return an empty string.
Thanks so much.
|
I have added support for RGB and allowed some properties, and updated regular expressions. |
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've added a few more notes inline.
There's a few cases in tests in which the value is valid but poorly formatted that are getting dropped. These ought to be allowed.
I've made a suggestion for much simpler regex too.
| 'border-left-color', | ||
| 'border-top-color', | ||
|
|
||
| 'background-color', |
There was a problem hiding this comment.
Also don't forget the shorthand
backgroundborderborder-(left|right|top|bottom)
That's assuming support :)
tests/phpunit/tests/kses.php
Outdated
| 'expected' => '', | ||
| ), | ||
| array( | ||
| 'css' => 'border-right-color: rgba(0, 0, 0, 0)', |
There was a problem hiding this comment.
I'd expect this to pass and be retained. The format is a bit ugly but it's valid https://jsbin.com/viripuz/1/edit?css,output
src/wp-includes/kses.php
Outdated
|
|
||
| if ( $found && $color_attr ) { | ||
| $css_value = trim( $parts[1] ); | ||
| if ( preg_match( '/^rgb[a]?\((\d{1,3}%?),\s?(\d{1,3}%?),\s?(\d{1,3}%?)(,\s?|\s\/\s)(\d*(?:\.\d+)?)\)$/', $css_value ) ) { |
There was a problem hiding this comment.
It might be possible to simplify the regex a lot here.
The goal of kses is to make the HTML (and CSS it contains) safe rather than valid. It's nice if it's valid but that's a problem for other parts of the code base.
You might be able to use /^rgba?\([\s\d%\/.,]+\)$/ to limit the user to the allowed characters
|
I have relaxed the regex rules and updated the existing tests. |
|
The problem this PR is trying to solve appears to have been fixed by Changeset 54117. |
I realized this wasn't correct. This changeset only allowed CSS variables, so RGB(A) colors are still not allowed. We will reopen this PR and resume work on it. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
1 similar comment
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
I started by using the equivalent regular expression from the colored_parse_rgba_string() function. This logic should work well for both comma and space value separators. I also overhauled the tests to test as many patterns as possible without overdoing it. |
|
Oh, I re-forked the |
Trac ticket: https://core.trac.wordpress.org/ticket/56391
Related gutenberg issue: WordPress/gutenberg#39402
Related gutenberg PR: WordPress/gutenberg#39488
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.