Allow filtering of safecss_filter_attr

[paulschreiber]

It would be helpful if sites could customize the allowed CSS. This would work around #24157 (which is 3+ years old).

In kses.php, we could solve this by filtering safecss_filter_attr. Diff attached.

[swissspidy]

Filters with the same name as the function are usually inside that function, not wrapped around it.

[rmccue]

This seems already possible via the safe_style_css filter, which controls the allowed properties in the CSS. It would potentially be nice to have more granular control though, allowing only specific values for fields e.g.

[paulschreiber]

@rmccue The safe_style_css filter won't work in this case, because, as described in #24157 safecss_filter_attr rejects any CSS with parentheses. Meaning <p style="color: rgb(100,100,100,0.5)"> gets rejected even though the color attribute is already whitelisted (see safecss_filter_attr() in kses.php around line 1714).

[rmccue]

Got it, so this needs to be a short-circuit hook at the start of the function then; probably pre_safe_style_css for the name.

[paulschreiber]

Okay. I uploaded a new diff which uses pre_safe_style_css as the filter name.

[paulschreiber]

@rmccue?

[paulschreiber]

Can we include this in 4.7?

[swissspidy]

37134.2.diff​ would need inline docs as per the ​PHP Documentation Standards.

Edit: Also, the original $value should probably be passed to the filter as the second argument. Otherwise it seems a bit pointless.

[bartekcholewa]

safecss_filter_attr - filter to unallowed chars

[bartekcholewa]

We could simply add filter to pattern in preg_match, i've added 36134.3.fdiff

[miinasikk]

Update patch based on changes. Add CSS value as filter param.

[paulschreiber]

Can someone review this? It's been sitting around for 3 years.

[adamsilverstein]

I'll take a look @paulschreiber - a filter here seems sensible and unlikely to introduce backwards compatibility issues. We already have similar filters elsewhere in kses.php to enable developer flexibility.

Does this get you what you need to solve the issue in https://core.trac.wordpress.org/ticket/24157? Does that issue really need solving with a new regex, or would adding this filter here also solve that issue essentially?

[paulschreiber]

We should fix #24157 separately — rgba() / rgb() should work for everyone, not just developers.

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (​https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: ​https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

• FAQs for New Contributors: ​https://make.wordpress.org/core/handbook/tutorials/faq-for-new-contributors/
• Contributing with Code Guide: ​https://make.wordpress.org/core/handbook/contribute/
• WordPress Coding Standards: ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/
• Inline Documentation Standards: ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/
• Browser Support Policies: ​https://make.wordpress.org/core/handbook/best-practices/browser-support/
• Proper spelling and grammar related best practices: ​https://make.wordpress.org/core/handbook/best-practices/spelling/

-->

Trac ticket:

[adamsilverstein]

37134.5.diff​ builds on 37134.4.diff​ from @miinasikk - still needs some tests to verify this works as expected, for example solving https://core.trac.wordpress.org/ticket/24157 which is mentioned in the ticket description.

[adamsilverstein]

37134.6.diff​ adds some unit tests that verify the filter works correctly.

I included the test cases @joehoyle added in #24157 (https://core.trac.wordpress.org/attachment/ticket/24157/kses.diff) and verified both that these are cleared currently and that the filter can be used to enable these cases.

Adding a filter unblocks these specific use cases without changing the current behavior.

@azaozz does this look ok to you, or do you think we should go further and change the regex itself?

[adamsilverstein]

In 47891:

Formatting: add a new 'safe_style_disallowed_chars' filter.

Enable developers to change the regex used in safecss_filter_attr to limit characters in the parsed CSS.

Props paulschreiber, swissspidy, rmccue, bartekcholewa, miinasikk.
Fixes #37134.

[azaozz]

Having some "second thoughts" on this patch:

• (minor) The filter is for a regex, not "chars", perhaps the filter name and the var name should mention that :)
• Allowing (, ), etc. can be done now, but it's still a potential security breach. Thinking there should be at least some more documentation on why these are not allowed by default, and what would be "safe" there.
• Perhaps instead of filtering/replacing the regex, the whole check can be done preemptively? Seems to make more sense if/when plugins want to use another method of checking for safe CSS. This will also allow several plugins to run their checks instead of overriding one another.

[adamsilverstein]

Thanks for the feedback @azaozz... Happy to work on this more.

I like the suggestion of overriding the check "preemptively". Would this be running the existing regex, then passing the result to a filter?

[adamsilverstein]

37134.8.diff​ implements an alternate approach filtering the (0/1 used as boolean) result of the Regex

Trac ticket:

[azaozz]

Replying to adamsilverstein:

Yes, 37134.8.diff​ looks/works a lot better imho. Perhaps the filter can be "reversed", true to allow the CSS, false to discard it. Also maybe the var and filter names can be tweaked a bit. Patch coming up :)

[azaozz]

In 37134.9.diff:

• Based on 37134.8.diff.
• Reverse the filter, true to allow, false to discard the css.
• Tweak the filter and var names to (hopefully) make them more self-explanatory.

[azaozz]

In 37134.10.diff: do not run the (new) safecss_filter_attr_allow_css filter when a CSS string was not "found".

This looks good here. @adamsilverstein could you have a look too, and lets add it :)

[adamsilverstein]

Looks good @azaozz thanks for the code update and ping. I'll give this a test to verify, run the tests in Travis, and get it committed.

Trac ticket:

[adamsilverstein]

Tests are passing, I'll get this committed.

[adamsilverstein]

In 48086:

Formatting: new filter safecss_filter_attr_allow_css on css parts.

Enables developers to determine whether a section of CSS should be allowed or discarded. By default, the value will be false if the part contains \ ( & } = or comments. Returning true allows the CSS part to be included in the output.

Replaces the safe_style_disallowed_chars filter introduced in r47891.

Props azaozz.
Fixes #37134.

[swissspidy]

@adamsilverstein Am I missing something or is this safecss_filter_attr_allow_css filter pretty useless? It does not provide the *original* CSS value ($css_item), but only the already heavily modified $css_test_string value.