Add CSS URL sanitization to kses.

[peterwilsoncc]

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.

[peterwilsoncc]

In 45067.diff​:

• Parses URLs in style attribute following @pento's ​comment on the related gutenberg ticket
• Adds background-image as whitelisted CSS for inline styles
• Needs unit tests, several of them

[azaozz]

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.

[azaozz]

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

[peterwilsoncc]

Thanks for the update @azaozz.

45067.3.diff​ is based on the second patch but includes a few unit tests.

[peterwilsoncc]

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.

[pento]

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.

[peterwilsoncc]

Changes to wp_kses_bad_protocol() removed in 45067.5.diff​.

[pento]

👍🏻

[peterwilsoncc]

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.

[peterwilsoncc]

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="&#91;gallery&#93;" style="background:url(&#91;gallery&#93;)">Pens and pencils</a></p>"

[azaozz]

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.

[azaozz]

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(), 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 already has passed through kses when saving) and no shortcode is "executed".

After the patch here we keep background:url(*) which is replaced in the above code and shows as test error when in fact is the proper output.

[azaozz]

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.

[peterwilsoncc]

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.

[pento]

In 43781:

KSES: Allow url() to be used in inline CSS.

The cover image block uses the url() function in its inline CSS, to show the cover image. KSES didn't allow this, causing the block to not save correctly for Author and Contributor users. As KSES does already check each attribute name against an allowed list, we're able to add an extra check for certain attributes to be able to use the url() function, too.

Props peterwilsoncc, azaozz, pento, dd32.
See #45067.

[pento]

In 44136:

KSES: Allow url() to be used in inline CSS.

The cover image block uses the url() function in its inline CSS, to show the cover image. KSES didn't allow this, causing the block to not save correctly for Author and Contributor users. As KSES does already check each attribute name against an allowed list, we're able to add an extra check for certain attributes to be able to use the url() function, too.

Merges [43781] from the 5.0 branch to core.

Props peterwilsoncc, azaozz, pento, dd32.
Fixes #45067.