Make WordPress Core

Opened 8 years ago

Closed 4 years ago

Last modified 2 years ago

#37134 closed enhancement (fixed)

Allow filtering of safecss_filter_attr

Reported by: paulschreiber's profile paulschreiber Owned by: adamsilverstein's profile adamsilverstein
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.6
Component: Formatting Keywords: kses has-patch has-unit-tests needs-dev-note commit
Focuses: Cc:

Description

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.

Attachments (10)

37134.diff (443 bytes) - added by paulschreiber 8 years ago.
37134.2.diff (435 bytes) - added by paulschreiber 8 years ago.
37134.3.diff (779 bytes) - added by bartekcholewa 6 years ago.
safecss_filter_attr - filter to unallowed chars
37134.4.diff (1.0 KB) - added by miinasikk 5 years ago.
Update patch based on changes. Add CSS value as filter param.
37134.5.diff (1.2 KB) - added by adamsilverstein 4 years ago.
37134.6.diff (4.6 KB) - added by adamsilverstein 4 years ago.
37134.7.diff (4.7 KB) - added by adamsilverstein 4 years ago.
37134.8.diff (3.0 KB) - added by adamsilverstein 4 years ago.
37134.9.diff (5.1 KB) - added by azaozz 4 years ago.
37134.10.diff (5.0 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (41)

@paulschreiber
8 years ago

#1 @swissspidy
8 years ago

  • Component changed from General to Formatting
  • Keywords kses added

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

#2 @rmccue
8 years ago

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.

#3 @paulschreiber
8 years ago

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

#4 @rmccue
8 years ago

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.

#5 @paulschreiber
8 years ago

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

#6 @paulschreiber
8 years ago

  • Keywords has-patch added

#7 @paulschreiber
8 years ago

@rmccue?

#8 @paulschreiber
7 years ago

Can we include this in 4.7?

#9 @swissspidy
7 years ago

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.

Last edited 7 years ago by swissspidy (previous) (diff)

@bartekcholewa
6 years ago

safecss_filter_attr - filter to unallowed chars

#10 @bartekcholewa
6 years ago

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

@miinasikk
5 years ago

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

#11 @paulschreiber
4 years ago

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

#12 @adamsilverstein
4 years ago

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?

#13 @paulschreiber
4 years ago

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

This ticket was mentioned in PR #272 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#14

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

-->

Trac ticket:

#15 @adamsilverstein
4 years ago

  • Keywords needs-unit-tests added
  • Owner set to adamsilverstein
  • Status changed from new to accepted

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.

#16 @adamsilverstein
4 years ago

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?

#17 @adamsilverstein
4 years ago

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

#18 @adamsilverstein
4 years ago

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

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.

#19 @adamsilverstein
4 years ago

  • Keywords needs-dev-note added
  • Milestone changed from Awaiting Review to 5.5

#20 @azaozz
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.
Last edited 4 years ago by azaozz (previous) (diff)

#21 @adamsilverstein
4 years ago

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?

#22 follow-up: @adamsilverstein
4 years ago

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

This ticket was mentioned in PR #314 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#23

Trac ticket:

#24 in reply to: ↑ 22 @azaozz
4 years ago

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

#25 @azaozz
4 years ago

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
4 years ago

@azaozz
4 years ago

#26 @azaozz
4 years ago

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

#27 @adamsilverstein
4 years ago

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.

This ticket was mentioned in PR #339 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#28

Trac ticket:

#29 @adamsilverstein
4 years ago

  • Keywords commit added

Tests are passing, I'll get this committed.

#30 @adamsilverstein
4 years ago

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

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.

#31 @swissspidy
2 years ago

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

Note: See TracTickets for help on using tickets.