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

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?

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 this patch / it no longer applies.

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 📌 Review/update $adminTags variable for new html elements to be whitelisted Needs work 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 list to the MR (from ✨ Xss::filterAdmin() to allow "template" elements Active ).
Patches for composer.

Added to allowed tags in the MR (from 🐛 element stripped by $adminTags Active ).
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...