Opened 6 years ago
Closed 6 years ago
#45067 closed enhancement (fixed)
Add CSS URL sanitization to kses.
Reported by: | peterwilsoncc | Owned by: | pento |
---|---|---|---|
Milestone: | 5.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Editor | Keywords: | has-patch has-unit-tests fixed-5.0 |
Focuses: | Cc: |
Description
Kses allows author and contributor level users to add images via HTML, passing the sources through some error checking and sanitization.
Neither authors or contributors can add background images via CSS as any style attributes containing brackets (and several other characters) are stripped out.
Basic CSS santization for URLs needs to be added to allow unprivileged users to use the cover image block in the new editor.
Related to Gutenberg issue 2539.
Attachments (6)
Change History (24)
#2
@
6 years ago
Looking at the patch, it does:
$css_test_string = str_replace( $url_match, '', $css_test_string );
where $url_match
is the actual URL. That leaves background-image: url()
in place which still triggers removal of the whole selector afterwards. Updating.
#3
@
6 years ago
45067.2.diff is based on 45067.diff and:
- Removes the whole
url(*)
part from the CSS property. - Makes the first regex a bit shorter/faster, and moves the trimming of the actual URL to the second regex.
- A bit of cleanup and some white space fixes (this is a very old code...).
#4
@
6 years ago
Thanks for the update @azaozz.
45067.3.diff is based on the second patch but includes a few unit tests.
#6
@
6 years ago
- Keywords commit added
45067.4.diff removes the white space changes my IDE helpfully made outside of the modified functions.
The current patch includes Oz's code and my unit tests. I reckon it's good to go.
#7
@
6 years ago
- Keywords commit removed
Let's not change the behaviour of wp_kses_bad_protocol()
. If an empty array is passed in the $allowed_protocols
parameter, it should continue to block everything, rather than defaulting to whatever wp_allowed_protocols()
returns.
#8
@
6 years ago
Changes to wp_kses_bad_protocol()
removed in 45067.5.diff.
#10
follow-up:
↓ 12
@
6 years ago
- Keywords commit removed
Hitting a problem in the shortcode validation tests as a result of this:
1) Tests_Shortcode::test_escaping with data set #8 ('<div style="background:url([[...y]])">', '<div style="background:url([[...y]])">') Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'<div style="background:url([[gallery]])">' +'<div style="background:url([gallery])">' tests/phpunit/tests/shortcode.php:457
[[gallery]]
is a valid relative URL, I'll step over the code to see where the outer brackets are stripped.
#11
follow-up:
↓ 13
@
6 years ago
It looks like it is getting stripped in do_shortcodes_in_html_tags()
but I unable to figure out where, @azaozz do you have any knowledge around this?
wp> do_shortcodes_in_html_tags( '<p><a href="[[gallery]]" style="background:url([[gallery]])">Pens and pencils</a></p>', false, [ 2 => 'gallery' ] ); => string(97) "<p><a href="[gallery]" style="background:url([gallery])">Pens and pencils</a></p>"
#12
in reply to:
↑ 10
@
6 years ago
Replying to peterwilsoncc:
[[gallery]]
is a valid relative URL
Hmmm, looking at https://tools.ietf.org/html/rfc3986#section-2.2 [
and ]
seem "reserved" as "general delimiters", together with :
, /
, ?
, @
, and #
. So for [[gallery]]
to be a valid relative URL the [
and ]
will have to be ulrencoded, or if they are used as delimiters, they will have to be after ?
char?
It looks like it is getting stripped in
do_shortcodes_in_html_tags()
...
Ugh, shortcodes again!! :(
Yeah, we do a lot of sanitization for all kinds of weird uses of shortcodes, will try to figure out what's going on there.
This actually brings a valid question: do we want to support shortcodes in style attributes that have url(*)
for users without unfiltered_html
? Currently they are not supported (as we remove the whole CSS rule) and seems we should keep it that way.
#13
in reply to:
↑ 11
@
6 years ago
Replying to peterwilsoncc:
It looks like it is getting stripped in
do_shortcodes_in_html_tags()
Right, think I figured it out. It is in do_shortcodes_in_html_tags()
at https://core.trac.wordpress.org/browser/branches/5.0/src/wp-includes/shortcodes.php#L415.
The first thing do_shortcode_tag()
(from preg_replace_callback( "/$pattern/", 'do_shortcode_tag', $attr, -1, $count );
) does is to check if the shortcode was "escaped" by using [[
and ]]
. If it is, it strips the extra [
and ]
and returns the remaining shortcode without executing the callback for it. Then the shortcode output is additionally filtered with wp_kses_one_attr( $new_attr, $elname );
.
The failing test is for escaped shortcode [[gallery]]
, so the expected behavior is to get [gallery]
back. That's what happens, however until now the whole background:url([gallery])
was stripped by safecss_filter_attr()
, i.e. $new_attr
was an empty string, and because of
if ( '' !== trim( $new_attr ) ) { // The shortcode is safe to use now. $attr = $new_attr; }
the $attr
remained unchanged (it's safe as it has already passed through kses when saving) and no shortcode is "executed".
After the patch here we keep background:url(*)
CSS values, so $new_attr
is not empty any more. So $attr
gets set to $new_attr
in the above code, and shows as test error when in fact is the proper output.
#14
@
6 years ago
In 45067.6.diff:
- Fixed the shortcodes test, replaced the selector with an invalid one (assuming the original test was for a value that won't pass through
wp_kses_one_attr();
). - Added couple of tests for malformed values.
- Added another conditional to look for malformed values and prevent notices.
#15
@
6 years ago
Thanks for following that up, boss.
I've run the full test suite locally and it's passing with the new patch.
As it's deep in the weeds of both KSES and shortcodes, I'll leave it in @pento's hands to review (again) and add the commit keyword (again, hopefully) before taking any further action.
In 45067.diff:
style
attribute following @pento's comment on the related gutenberg ticketbackground-image
as whitelisted CSS for inline styles