FilterHtml accepts <*> but does not support it, resulting in inaccurate ::getHtmlRestrictions() return value

To get us started, creating test coverage that exercises all different aspects. See 431262b1d5706eb1bbddbabf7c20ae3408d34b0e.

Fix is easy, but it's not actually a bug

Thanks to that first commit, adding explicit test coverage for the reported bug is very simple. And the fix is tiny too. See for yourself:

diff --git a/core/modules/filter/src/Plugin/Filter/FilterHtml.php b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
index 9c1c9c2a90..0350ed3a23 100644
--- a/core/modules/filter/src/Plugin/Filter/FilterHtml.php
+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -326,6 +326,11 @@ public function getHTMLRestrictions() {
       'lang' => TRUE,
       'dir' => ['ltr' => TRUE, 'rtl' => TRUE],
     ];
+    if (isset($restrictions['allowed'][$star_protector])) {
+      $restrictions['allowed']['*'] += $restrictions['allowed'][$star_protector];
+      unset($restrictions['allowed'][$star_protector]);
+    }
+
     // Save this calculated result for re-use.
     $this->restrictions = $restrictions;
 
diff --git a/core/modules/filter/tests/src/Unit/FilterHtmlTest.php b/core/modules/filter/tests/src/Unit/FilterHtmlTest.php
index 4921083bc6..d6850feed7 100644
--- a/core/modules/filter/tests/src/Unit/FilterHtmlTest.php
+++ b/core/modules/filter/tests/src/Unit/FilterHtmlTest.php
@@ -166,7 +166,7 @@ public function providerGetHtmlRestrictions() {
     ];
 
     yield '<p class="foo bar" data-*> <br>' => [
-      '<p class="foo bar" data-*> <br>',
+      '<p class="foo bar" data-*> <br> <* data-entity-type data-entity-uuid>',
       [
         'allowed' => [
           'p' => [
@@ -177,7 +177,10 @@ public function providerGetHtmlRestrictions() {
             'data-*' => TRUE,
           ],
           'br' => FALSE,
-          '*' => $hardcoded_asterisk_restrictions,
+          '*' => $hardcoded_asterisk_restrictions + [
+            'data-entity-type' => TRUE,
+            'data-entity-uuid' => TRUE,
+          ],
         ],
       ]
     ];

⚠️ However … that made me realize that FilterHtml was simply never designed to accept and process/enforce <*> as an allowed tag! 😬

Understanding FilterHtml

FilterHtml only returns <*> in its restrictions because it's the only way it can communicate that additional restrictions apply/are enforced that cannot be configured. The stripping of style and on* attributes is a very deeply rooted mechanism that cannot be disabled or overridden in Drupal for security reasons. Analogously yet differently, lang and dir are allowed on every tag by the HTML filter (again with no configuration possible, hence "analogously") to allow proper rich multilingual support throughout user-entered content without the need for perfectly accurate configuration (which would otherwise allow multilingual Drupal sites to easily be broken).

Those facts combined fully explain this piece of code in FilterHtml::getHTMLRestrictions():

    // The 'style' and 'on*' ('onClick' etc.) attributes are always forbidden,
    // and are removed by Xss::filter().
    // The 'lang', and 'dir' attributes apply to all elements and are always
    // allowed. The list of allowed values for the 'dir' attribute is enforced
    // by self::filterAttributes(). Note that those two attributes are in the
    // short list of globally usable attributes in HTML5. They are always
    // allowed since the correct values of lang and dir may only be known to
    // the content author. Of the other global attributes, they are not usually
    // added by hand to content, and especially the class attribute can have
    // undesired visual effects by allowing content authors to apply any
    // available style, so specific values should be explicitly allowed.
    // @see http://www.w3.org/TR/html5/dom.html#global-attributes
    $restrictions['allowed']['*'] = [
      'style' => FALSE,
      'on*' => FALSE,
      'lang' => TRUE,
      'dir' => ['ltr' => TRUE, 'rtl' => TRUE],
    ];

Finally, you can also see in \Drupal\filter\Plugin\Filter\FilterHtml::process() that <*> is explicitly not allowed/is ignored — again confirming that this is solely a way to convey hardcoded behaviors.

But then why even have this "star protector" parsing mechanism?

Well, as the code indicates: for allowing trailing-wildcard-attribute names! Specifically: on* (for disallowing onClick et cetera) and data-* (for allowing arbitrary data- attributes).

In the "restrictions interpreting" code:

          // Put back any trailing * on wildcard attribute name.
          $name = str_replace($star_protector, '*', $name);

          // Put back any trailing * on wildcard attribute value and parse out
          // the allowed attribute values.
          $allowed_attribute_values = preg_split('/\s+/', str_replace($star_protector, '*', $attribute->value), -1, PREG_SPLIT_NO_EMPTY);

and in the "restrictions applying" code: see \Drupal\filter\Plugin\Filter\FilterHtml::filterAttributes().

IOW: the "star protector" stuff in ::getHTMLRestrictions() was only ever intended for the data-* use case (attributes!), it just happens to accidentally also partially work for the <*> use case (tags!).

The solution is therefore simple: tighten the parsing logic to ignore <*>. It also means the bug originally reported is not the actual bug…

Not sure this is a bug report, as I don't see anywhere in FilterHtml that says * is allowed for tags - while getHTMLRestrictions() allows * tags, the FilterHtml docs only mention attributes.

edit: crosspost with above, which explains it in much more detail!

#7: oh YAY! 😄 Commits incoming that fully harden against this and inform the user — stay tuned, would love your reviews! 😊

The bug has been fixed in the past two commits. Next: form-level validation to inform the end user.

Validation added: .

This is ready for review. Unassigning.

While we are improving validation, do we need to consider the case where the tag contains a wildcard? I can imagine someone perhaps trying <h*> to allow all heading tags...

Also, we need a functional test to cover the new error message.

Fair!

Done in bfe30afe727adaf9434e63272ea53fb20726a3af, expanded to cover #12 in 336ad92cf3f0d14d27b73f9b2a9e66752fabe196.

Addressed #12 in c2b6acba05d76c1dba63c4777597ef7aff553a28.

WTF, d.o's gitlab integration shows 39e99a35 and c2b6acba, but not the commit in between the two: 336ad92. Which makes this issue infuriatingly difficult to interpret, since 336ad92 was supposed to fail! 🤐😫

One question about the validation, other than that this looks great!

Great catch! 👍

Fixed now 😊

Looks perfect - RTBC if bot agrees.

Also removing Markdown from the issue title :)

We ran into this while working on the CKEditor 4 → CKEditor 5 migration path — so tagging d10.

#19: 😂🙈 — a consequence of being forced to work with GitLab and trying to make the most of its formatting idiosyncrasies…

I so wish we could just use Markdown in d.o comments, the backtick syntax is much quicker to write.

if (strpos($tag, $star_protector) !== FALSE) {
I like that this checks for a * anywhere in the tag name, but the test addition in FilterHtmlTest only covers <*> and <h*>. Let's add a case where there's more than one letter before and a case where there's one or more letters after. Perhaps those are less common to arise, but since people can enter whatever into the field, let's make sure we have predictable behavior for when they do.

if (preg_match_all('/\<([a-z0-9]?\*)/', $allowed_html_value, $matches, PREG_SET_ORDER) > 0) {
This regex covers any single alphanum before a *, but not more than one. For example, it would allow through <su*>. Also, because it ends at the *, if you had <*frame>, it would then tell you that * is an unsupported wildcard tag rather than telling you that <*frame> is an unsupported wildcard tag. Since we also have the other validation, I suppose we can choose to not cover all of the same cases here, and focus this one on only the common errors, but in that case, perhaps we should make the regex just '/\<(h?\*)/' instead of any leading character? Though I wonder if it would be better to parse the tag names the same way we do it in getHTMLRestrictions() so that we're validating the same way in both places?

Added a more complex test as per #22, improved the validation regex and also simplified the validation method a bit.

Needs a reroll for 9.4
Diff doesn't apply for FilterAdminTest.php

That's because FilterAdminTest.php line 392 was changed from
$this->assertSession()->pageTextNotContains(t('The text format %format has been updated.', ['%format' => 'Basic HTML']));
to
$this->assertSession()->pageTextNotContains("The text format Basic HTML has been updated.");
right before the new code insertion, thus the fail.

But, after manually solving the diff conflict, the issue is indeed fixed. So, it only needs a reroll.

Thank you for manually testing this. Just to let you know, there is no need to attach a screenshot of the failed patch, just commenting and tagging "needs reroll" is enough.

Rerolled for 9.4.x.

Rerolled #26

Try to fix the test failure

#33 still applies cleanly to 10.1.x and matches the state of the MR. We should just close the MR IMHO?

Let's find out if it still passes tests. I cannot RTBC this, I worked too much on it.

Let me know if I'm testing this wrong.

Tried <*> but get this error with and without the patch

InvalidArgumentException: The value for the special "*" global attribute HTML tag must be an array of attribute restrictions. in Drupal\ckeditor5\HTMLRestrictions::validateAllowedRestrictionsPhase2() (line 201 of core/modules/ckeditor5/src/HTMLRestrictions.php).

Tried <data *> page saves with the page but doesn't save the value.

Need reroll for 10.2

Just a reroll of #33 for 10.2.x

After reroll the patch and applied, but still couln't solve the problem of https://www.drupal.org/project/extended_html_filter/issues/3401513#comme...

Just a reroll of MR for 10.6.x. But even with this https://www.drupal.org/project/extended_html_filter/issues/3401513#comme... is not working. IMHO, the best approach now is to use https://www.drupal.org/project/htmlpurifier and disable drupal core filter.

Rerolled the MR against main.

Think maybe a bad rebase? Showing 1000+ changes.