safecss_filter_attr() should support query strings with "&" as used by Gutenberg

[philippmuenchen]

Gutenberg transforms "&" to "&" when saving content.

E.g. for the Media/Text-Block the content that is filtered by safecss_filter_attr() might contain "&" as here:

style="background-image:url(https://example.com/uploads/sites/2/2023/10/image.jpg?width=1024&height=600);background-position:46% 43%"

As safecss_filter_attr() simply explodes the style value by semicolons. Therefore the example above does not pass and gets striped out. Finally the block layout breaks as the saved result is:

style="background-position:46% 43%"

Fixing it for the moment by filtering the content before kses-functions:

<?php
add_filter('pre_kses', function ($content) {
    // Replace all '&amp;' with '&' in the parameters of every URL in the content
    return preg_replace_callback('/(https?:\/\/[^\s]*?)&amp;([^#]*?)/', function($matches) {
        return str_replace('&amp;', '&', $matches[0]);
    }, $content);
});

[khoipro]

Great catch! I have the same issue with one of my project.

Trac ticket: https://core.trac.wordpress.org/ticket/60979

[narenin]

Hi,

I have added the PR with Fix.

This PR look good for me

Trac ticket: Core-60979

Trac ticket: Core-60979

[nhrrob]

We are very close 6.6 RC1
Moving to next milestone

[hellofromTonya]

Patch: ​https://github.com/WordPress/wordpress-develop/pull/6645

The scope of work in the patch appears to be more of an enhancement, as it adds a lot of new code and new functions to add support for query strings with "&".

Changing this ticket to enhancement.

Trac ticket: Core-60979

By accident I pushed an additional commit to this branch after merging in trunk and then I didn't notice for the past two weeks. I've removed the commit I pushed (74d3c4c94a) to bring it back to the state @narenin left it in, just with the previous trunk merge.

[desrosj]

@dmsnell Are you looking to try and commit this for 6.7? The beta 1 deadline is in roughly 24 hours.

[hellofromTonya]

Patch: ​https://github.com/WordPress/wordpress-develop/pull/6645

Converting this ticket back to a defect. The scope of the patch is directly focused on fixing the bug. No new functionality is added.

Removing dev-feedback given multiple committers involved in the patch's review.

[hellofromTonya]

Patch: ​https://github.com/WordPress/wordpress-develop/pull/6645

Has an approval from @dmsnell.

I also reviewed, tested, and approved it.

The patch is ready for commit. Self-assigning to commit it.

As I was preparing a commit for this, I started to wonder:

$css = WP_HTML_Decoder::decode_attribute( $css );

Within a URL query params, _could there ever_ be valid entities that should not be decoded?

@dmsnell @peterwilsoncc @sabernhardt (for CSS expertise)

Within a URL query params, could there ever be valid entities that should not be decoded?

@hellofromtonya this is a good question, and one reason to avoid html_entity_decode(). there are special rules regarding an "ambiguous ampersand" in attribute values specifically to handle cases where people don't properly encode the value.

otherwise, the value in the attribute _will_ be decoded by a browser, so the argument here is that we should work with the same value a browser will, otherwise errors and potential vulnerabilities will slip in.

_if_ the value comes from an HTML attribute, unless it was retrieved by the HTML API's get_attribute() (which automatically performs the decoding), then we will want to transform from HTML space to PHP space, process, and then transform back.

Thank you @dmsnell for confirming. I added another dataset with valid entities in the path and query params, just to validate it before and after the changes.

Alrighty, now ready for commit.

[chaion07]

Thanks @philippmuenchen for reporting this. We reviewed this Ticket during a recent bug-scrub session. Here's the summary:

1. There's a small change request comment recently made under PR
2. We are adding the changes-requested label
3. We should follow up with the PR submitter

Thank you.

Props to @pratiklondhe for these suggestions.

Cheers!

Do the tests still need to cover multiple properties grouped together, as in the example from the ticket's description?
background-image:url(https://example.com/uploads/sites/2/2023/10/image.jpg?width=1024&height=600);background-position:46% 43%

Thanks @sabernhardt and @peterwilsoncc for the test scenarios.

I have added these and it is passing, please check.

[hellofromTonya]

Removing commit. Need to first explore the possibility of a ​safety concern for decoding without re-encoding:

What if the incoming string has encoded nefarious HTML in it? For example, what if it's this

'background-image: url("&#60script&#62alert`1`&#60/script&#62")'

Result:

background-image: url("<script>alert`1`</script>")

Seems unsafe unless it gets re-encoded before returning.

[hellofromTonya]

As this is not yet ready for commit, i.e. as the discussion continues, removing myself as committer.

[stoyangeorgiev]

Discussed in a bug-scrub. As the PR is pending review at the moment of discussion and Beta 3 coming in a few hours, moving this one to 6.8