[D8] \Drupal\Component\Utility\Xss::filter introduced in 8.x-1.3 filters valid html from existing configuration

Fix makes sense and looks good (and still secure).

+1 Fix makes sense and looks good.

This is important as it breaks many existing sites - would be great to have a new release soon.

As mentioned on the equivalent issue for D7, #3038166-11: filter_xss() introduced in 7.x-1.26 filters valid html from existing configuration, why not use the text format? Surely that should be used instead of filter_xss or filter_xss_admin entirely for text that has gone through a text format?

For example, something like:

'#message' => check_markup(str_replace(["\r", "\n"], '', $config->get('popup_info.value'), $config->get('popup_info.format')),

(That said, I'm not sure why the line breaks need replacing, that seems odd to me.)

Thanks for all the prompt and great work finding a fix for the issue.

My only objection is that Xss::filterAdmin filters out the 'style' tag which some people (although I don't see any good reason) may have added in their config.

A suggestion that I like from the d7 thread was to rely on the text filter first and only use Xss::filter if the text format doesn't provide sanitation.
https://www.drupal.org/project/eu_cookie_compliance/issues/3038166#comme...

I'm inclined to make a patch that follows that suggestion and post it here for review shortly.

James, I have no idea why line breaks need replacing. I don't recall writing that line (although I can't say 100% it didn't originate from me), this is an old module that has had several maintainers throughout the years.

Now that we have that code for line breaks in there I'm afraid we have to leave it there as we may break some obscure config that relies on line breaks being removed.

I do not understand the need for a style tag - you can do this in the normal CSS.

I prefer the Xss::filterAdmin() solution (#2) or to use the text format itself (#8).

With #8 you can add the 'style' tag to your text format if you really need it.

Here's a patch that removes the xss filtering and relies on check_markup (which was already there in the code).

Can somebody RTBC so we can push this out in a new version?

The consensus seems to be that check_markup is sufficient? I'll check with a security team member if I can get hold of him.

Thanks Sven! Weirdly though, I now realise this is actually just a revert of the 8.x-1.3 security fix https://cgit.drupalcode.org/eu-cookie-compliance/commit/?id=f39f32ddaf8b... .

Which makes me think either I'm missing something, or the D8 security 'issue' was only due to abuse of text formats, which isn't really a security issue. So I'm very reluctant to push further. I'd like to ask what was the original security issue actually reported for, to identify whether it's something I've missed, and therefore this approach is not correct after all. But I realise that can't necessarily be done out in the open. Would you be open to adding me to the security issue so I can see that discussion? I won't take any offence if you're not willing to do that I just don't want to end up accidentally encouraging a security hole to be re-opened!

I suspect that the D7 security fix was needed, because that had some settings that got no sanitization settings. But settings that used text formatters and are then correctly put through check_markup() should have remained as they were, I think.

I have invited the security team to this thread. I don't think I can invite people to security issue threads.

The security team has responded and the person who responded recommends that we use Xss::filterAdmin, which was what was originally suggested in this issue. To me, it seems like we can use the patch in #2. Any thoughts on that? Seems like it was also RTBC before the discussion started.

Like in the D7-issue: #2 looks good to me.

Committed and tagged as new release - thanks to all that helped identify, fix and debate this issue.

Late to the party, but I want to second that the 8.x security fix in general seems weird. That's what text formats are there for, if you use an insecure text format then that's not a security issue as long as you correctly check access to that textformat so that only someone with permission to it can add/change text.

Just to reference a new issue we have because of this fix in #3047461: Use text format to format text messages and not filter_xss_admin.