Review/update $adminTags variable for new html elements to be allowed

Protocol filtering should also be disabled for the media attribute to avoid complex breakpoints to be mangled.
Currently protocol filtering is disabled for other non-uri attibutes such as "title, alt, rel, property".
It can be easily disabled for media by adding it to the $skip_protocol_filtering array in the attributes function:

$skip_protocol_filtering = substr($attribute_name, 0, 5) === 'data-' || in_array($attribute_name, array(
              'title',
              'alt',
              'rel',
              'property',
              'media',
            ));

Hi,

Seems like we also need to add 'media' to $skip_protocol_filtering, otherwise half of the media attribute value is removed.

@postovan-dumitru Many thanks!
The patch in #12 works for me (d8.7.4).

How about video tag? Is there a reason It shouldn't be whitelisted?

Here's a small test change to illustrate the media attribute issue. The patch in #12 fixes the issue so re-uploading with test updates too.

The allowed admin tags test only checks for tags that are not allowed, so there's no test to update for whitelisted tags.

Added video tag support

Added img sizes attribute to the $skip_protocol_filtering list.

Test only patch failed as expected. Reverting status to Needs review

Applied the patch #22 on drupal 9.2.6 successfully. We had the problem that a views rewrite of an image field caused the loss of the picture element. Now fixed! Thanks!

#22 Seems working as expected, just adding new patch adding noscript tag to list, is something provided by core too & can be found in quite common related scenarios like using blazy contrib module with no js fallback, @see Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag().
Think there is no need to add extra test, please review/any suggestion is accepted. Thanks!

Patch #27 working for me
Core: 9.3.5

Requesting issue credit be added here for those who worked on the duplicate issue:

#2687479: Responsive Image not working in rewritten Views field/area due to XSS filtering

Thanks for this issue. Tagging for some cleanup.

1. Issue summary needs steps to reproduce

2. Issue summary needs to be updated with noscript inclusion

@CRZDEV Why don't you think there should be a test added for noscript?

Updated issue summary to use similar steps to reproduce from the duplicate issue: #2687479: Responsive Image not working in rewritten Views field/area due to XSS filtering

I hope this issue fix will be backported to D9 and not only cover D10

I can confirm patch #27 fixing the problem on our websites. Is the issue ready for RTBC?

#31 still needs to be answered

Hi @Kristen Pol, @larowlan feel free to add any possible scenario to test coverage if you think is needed. I don't know any noscript vulnerabilities that could cause, taking into account that will be used for Xss::filterAdmin() not for default Xss::filter method. Thanks in advance!

Yeah, if you want it to stick around, you'll need a test to make sure there's no regressions.

I also see the video element has been added, there's no mention of that in the issue summary either.

Also, I'm not sure this is a bug, its a feature request or perhaps a task.

Since this seems to be an overall review of the $adminTags variable and allowed elements in general, I'd like to propose the <button> element be considered as allowed markup in addition to the <video>, <source>, and <picture> tags.

There are tickets like https://www.drupal.org/project/drupal/issues/3217767 that have expressed a want for it, and use cases in our Drupal instance where the <button> element gets stripped away from patterns because it's not an allowed tag--causing developers to create hacky or less accessible workarounds when a native <button> should be used.

I'm not sure if that suggestion needs a new ticket since this covers "new html5 elements," but figured I should throw it out there anyway so it's at least known and can be addressed.

Re-rolling patch 22 for 9.5.0

Thank you very much. I use views fields to rewrite stuff a LOT!
Searched for hours on why the browser does always only use the fallback option. And indeed your media querys will be butchered and then they will not work.
#41 works and fixes the problem as sizes and media are excluded from filtering.

CarlyGerard
She/Her/Hers
CreditAttribution: CarlyGerard at Western Washington University commented 2 months ago

Since this seems to be an overall review of the $adminTags variable and allowed elements in general, I'd like to propose the element be considered as allowed markup in addition to the , , and
tags.

There are tickets like https://www.drupal.org/project/drupal/issues/3217767 that have expressed a want for it, and use cases in our Drupal instance where the element gets stripped away from patterns because it's not an allowed tag--causing developers to create hacky or less accessible workarounds when a native should be used.

I'm not sure if that suggestion needs a new ticket since this covers "new html5 elements," but figured I should throw it out there anyway so it's at least known and can be addressed.

<button> tag should be definitely added to list of allowed tags, moreover I don't see any specific reason why it's not allowed. It's allowed by full html text format. Lack of <button> tag produces strange situation where placing simple button on a page needs tricky hacks. Since all content produced by Global: Custom Text gets filtered button is removed from field content even if it was previously allowed by text format, but if instead of using fields content is displayed by views it's allowed.

Now let's assume that <button> is not allowed and try to change it to <a> for example in content type body managed by CKEditor5 so from:

<button class="rounded-pill btn-rounded" type="button">
   Label
   <span><i class="fas fa-arrow-right"> </i></span>
</button>

we change to that:

<a class="rounded-pill btn-rounded">
   Label
   <span><i class="fas fa-arrow-right"> </i></span>
</a>

But one of CKEditor5 filters is correcting HTML to take care of user publishing content and instead of wanted markup we get:

    <a class="rounded-pill btn-rounded" type="button">Label&nbsp;</a>

This time CKEditor5 filtered <span> and <i> changing it to   but if we use <button> CKEditor5 lefts syntax intact.

Why placing simple <button> on site when using fields is almost impossible when using fields?

Same applies to <svg> so both button and svg should be added to allowed tags.

IMHO, if we use a new line for each tag in this $adminTags variable declaration, it'll be easier to understand the diffs in the future.

I think #27 fixes the original bug/issue for this issue. Maybe this one should be merged and a follow-up issue opened where those topics are addressed?

Side question, does #27 apply cleanly to 10.0 too?

The last Drupal update 9.5.9 breaks patch #41 / it no longer applies (due to changes in file in https://www.drupal.org/project/drupal/issues/2692451).

It would be helpful to get this into the core soon to prevent such issues in the future.

Also to my comment #45, I think further work could be done in another issue.

A reroll patch request does not constitute a priority update to Major.

Here is the re-roll for 9.5.9

@Webbeh my reasoning was not the need for re-roll, but the fact that this issue prevents websites from using responsive images in rewrites in views, which quite reduces the possibilities one has with responsive images and views in the D9/D10 core.

Looking at #2692451, it was merged without additional changes that seem to be holding back this issue.

Not sure why noscript tag was removed from the admintag list in #41 after being applied in #27, just added it again.

Here is the reroll for 10.1

The patch in #54 accidentally reverted some changes. Rerolled for 10.1 again

The patch in #50 still applies to 10.1. My issue with it not applying was due to another composer conflict. Merge request !4231 currently reflects the patch from #50.

No comments from those making patches about #40 and #43 and the recommendation to support the button element? Is this issue correctly named or is it just a responsive image issue? Button is valid and ought to be included.

Adding button element.

Tried to fix failures in #58.

Drupal\Component\Utility\Xss::needsRemoval() requires an array. But, the patch has removed string[]:

   /**
    * Whether this element needs to be removed altogether.
    *
-   * @param string[] $html_tags
+   * @param $html_tags
    *   The list of HTML tags.
-   * @param string $elem
+   * @param $elem
    *   The name of the HTML element.
    *
    * @return bool
    *   TRUE if this element needs to be removed.
    */
-  protected static function needsRemoval(array $html_tags, $elem) {
+  protected static function needsRemoval($html_tags, $elem) {
     return !isset($html_tags[strtolower($elem)]);
   }

That is breaking "core/modules/editor/src/EditorXssFilter/Standard.php", which wants string[].

Fixed the PHPCS issue showing in #59. Please review.

Was previously tagged for IS update which believe still needs to happen.

Also existing patch appears to be removing the typehints previously there, that doesn't seem correct.

Adding IS details for "button" proposed by @CarlyGerard and @prauat.

@sourabhjain, @murrow are you taking care of this patch or should I do it as I'm in need of that functionality?

Button is in #58, @prauat

@murrow yes, but I also see there type hinting removed which @smustgrave mentioned. Why did this type has been removed?

-   * @param string[] $html_tags
+   * @param $html_tags
    *   The list of HTML tags.
-   * @param string $elem
+   * @param $elem

That was not in my patch @prauat. I mentioned the issue in #60, but the fix that was applied afterwards just exacerbated the problem. IMO, the type hints need to be returned.

- Fix for most of code style warnings and errors
- Revert of type hinting
- Add type hinting for primitive type string for following methods:

core/lib/Drupal/Component/Utility/Xss.php

-  protected static function needsRemoval(array $html_tags, $elem) {
+  protected static function needsRemoval(array $html_tags, string $elem) {

core/modules/editor/src/EditorXssFilter/Standard.php

-  protected static function needsRemoval(array $html_tags, $elem) {
+  protected static function needsRemoval(array $html_tags, string $elem) {

I don't see a reason why $elem shouldn't be typed here

#32 update the issue summary and steps were from a closed duplicate but haven't seen anyone confirm those yet. If that could happen please. Added to the tasks in the IS.

The proposed solution talks about reviewing/updating the adminTags. But it should specifically say what tags are being added. Already part of the remaining tasks. So leaving the needs issue summary tag for that one.

Also #68 seems to have some comment changes, breaking them into new lines that seem out of scope of this issue.

@smustgrave, are you saying that the note (now removed) stating that the button tag had been added to adminTags was in the wrong place, incorrectly specified or shouldn't have been there at all?

Adding a nudge and support for being added for accessibility reasons.

Off topic / separate question, but would there ever be the possibility for site administrators to control the list of allowed tags via a screen in the Configuration? Like a page with checkboxes that might default to a set (safe) list and include links to some sort of security vulnerabilities page for each HTML element about what the risks are?

Thank you everyone for your hard work on this! :)

Adding a list to cover "Review what HTML elements to add" task.

Can confirm #49 still applies to D10.2 and fixes the original issue of this ticket before it branched to fixing more than just that:

Xss::filterAdmin() is currently stripping out the picture & source html elements that are part of the Core module Responsive Image. $adminTags sets the elements that are whitelisted and would need to be updated.

I hope this issue will be merged into the core soon as without this fix, responsive images can't be used in the views rewrite filter at all.

I think this issue should be reduced to the original case of responsive images not working in rewritten fields and a new ticket should be open for other HTML tags needed.

As this issue is still stalling, does anyone have a patch for 10.3 at hand, otherwise I can try to prepare one later?

TBH I never understood why Xss is not a service. Almost everything else in Drupal can at least be overridden/extended by a module. I find it a bit "un-Drupalish" that we say Xss::filterAdmin and not \Drupal::service("xss")->filterAdmin(...). This way, other modules like response_image could at least easily extend the list of allowed tags.

#49 still applies cleanly in 10.3

another tag that gets strips out that's good to be whitelisted is "drupal-media" when using core's ckeditor5.

#49 woks as expected
This is very helpful to ensure we render the picture and source tag from the responsive image in a custom text field in views.

Re-roll for patch #49 to add noscript tag.

@ahmad-abbad Many thanks! The patch in #80 works for me (d10.4.4).

Patch in #80 worked well for me. I needed video and source tags and this patch enabled them. Thanks!

Is the button tag going in? See #40 and #43 above.

Rerolled and cleaned MR.
Added <button> to the allowed list.
Patch attached for composer (11.x).

If video is added, audio should probably be added as well...

Fixed missing attributes lost in the reroll.
Added <audio> tag as well. (thanks #86 for that suggestion)
Again, attached patch for composer.

Here is a simplified patch for Drupal 10.4 for live projects.

Glad to see tracting here again! :)

Added <template> to allowed tags in the MR (from #3443362: Xss::filterAdmin() to allow "template" elements).
Patches for composer.

Added to allowed tags in the MR (from #3348218: <data> element stripped by $adminTags).
Patches for composer.

Updated IS with references for every tag added to the list.

Think we need coverage for all the tags being added

@smustgrave I didn't manage to find existing coverage for other allowed tags. Would you help me find it so I can extend it, please?

If we allow noscript, we should probably add a test for this kind of attack: https://www.acunetix.com/blog/web-security-zone/mutation-xss-in-google-s...

Hello, i suggest the svg tag should be allowed too.

The svg tag would need to allow a lot of other tags to be useful (circle, rect, etc) and there is a lot of ways to use SVG for XSS attacks so adding SVG support would massively increase the scope of this issue.

Patch in #97 applies cleanly to 11.2.5 and works well.

Instead of adding arbitrary tags to the list, wouldn't it make more sense to add a way to alter the list?
This way modules could allow tag needed for their specific use case.

(WordPress has a hook that does this: https://developer.wordpress.org/reference/hooks/wp_kses_allowed_html/)

That is a good idea with a configurable list.

Patch for D 10 in #91 applies cleanly to 10.6.5 and works well.
No videos in views without this patch!

Tested the #91 patch and it works as expected for me. The issue is resolved and I did not find any problems during testing but I agree with @prudloff. An alter hook would be a more flexible and maintainable solution than continually adding tags to the hardcoded list. Modules could then extend the allowed tags for their own use cases while core maintains a secure default set.

so a module could do:

function mymodule_xss_admin_tags_alter(array &$tags) {
$tags[] = 'picture';
$tags[] = 'source';
}

Couldn't this functionality be tied to the very similar text format configurations, where basic html, restricted html, etc. already have configurable filters for allowed tags, etc?

Attaching a small patch that extends the admin tag/attribute allowlist in Xss.php to include the picture and source elements plus the media, sizes and srcset attributes, so responsive image markup survives filterAdmin()-based filtering (e.g. Views token rewriting). I see MR !4231 already covers a much broader set of elements/attributes here, so this is really just a confirmation of the same underlying need from a narrower, single-purpose patch -- feel free to close this out in favor of the MR if it already includes these.

Patch attached: xss-responsive-image-tags.patch

My $0.02

I am not entirely sold on the idea that an XSS filter class (which is there for security purposes) should be easily extensible.

At the very least, it seems like a question that would merit some discussion.

I note also that the original issue that was reported here has been fixed with a perfectly adequate patch for quite some time, and the fix appears to be being held up by turning this from a fix to a feature request.

I think if we really want an extensible XSS filter, then a feature request/discussion should be opened for that.

In the meantime, let's not hold up good work that actually fixes the reported issue with a feature discussion, or by expanding the original issue into an ever expanding list of missing elements. What do you guys think?

@apotek: Just my thoughts on this... I'm personally not a huge fan of this "educational" concept to be honest. Drupal is usally known for being fully extensible and flexible and I think that's also what its success and appeal to a large part come from. Developer can do all kind of bad stuff - right now and nothing in Drupal actually keeps them from doing it. Introducing SQL injections, taking an unvalidated query argument and building a file path from it, etc etc. - you name it. I think if XSS was a service and someone wrote a Drupal module, overrode that service - I assume they know what they're doing and at the very least what XSS is about. There's a myriad of other ways people could shoot themselves in the foot if they absolutely want to. But the opposite could also be true: That person might have a very valid reason if they wanted to XSS like a new upcoming HTML tag for example. Considering how long this issue takes to be actually resolved, I don't think it's completely unreasonable to have the idea that there might be absolutely valid reasons to extend the XSS via a module. It's just another part of Drupal like any other that should be extensible and replaceable IMHO. You might do weird unsafe shenanigans with it - or you might do completely sensible stuff. It's ultimately up to the developer. Right now, people will need to resort to overriding other services that call the XSS filter. Not much more complicated considering you could use AI right now. But will produce much more convoluted code and might actually make the security landscape worse.

What I totally agree with, though, is that we should separate this. The one thing are the extensions to the XSS proposed here in this issue. Another thing is turning XSS into a service if that is something people can agree on for the future. Good idea!

I agree this should be separated into another ticket for a feature request and the original fix should be merged as this issue has been requiring patching core for quite a while now