Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss)

This should wait for #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object to finish; that will make this simpler.

#3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object is in!

This issue will have to make it possible to delete the current work-around in the 3 places where it occurs. I.e. it should make the attached patch pass tests.

This now blocks #3228691: Restrict allowed additional attributes to prevent self XSS. See comments 14, 15 and 16 there.

Add support for the * special case used by FilterHtml.

Update FundamentalCompatibilityConstraintValidator to be aware of the * special case.

Turns out that a few of the @todos were wrongly pointing to this issue when they should've been pointing to #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated.

That means this is basically … done! The only thing that remains is adding some test coverage for operations.

This conflicted heavily with #3231328: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets. Rebased.

Let's run only CKE5 tests to make DrupalCI faster.

+++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
@@ -136,6 +143,14 @@ private static function validateAllowedRestrictionsPhase1(array $elements): void
+      // The `*` HTML tag is a special case: it allows specifying specific
+      // attributes that are allowed on all tags (f.e. `lang`, because
+      // translations are an orthogonal concern) or disallowed on all tags (f.e.
+      // `style`, because security is an orthogonal concern).
+      if ($html_tag_name === '*' && !is_array($html_tag_restrictions)) {
+        throw new \InvalidArgumentException(sprintf('The value for the special "*" HTML tag must be an array of attribute restrictions.'));
+      }

This added validation, but we didn't have tests for it yet.

This adds those.

+++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
@@ -101,6 +101,34 @@ public function providerConstruct(): \Generator {
+    yield 'VALID: joker wildcard with attribute allowed, specific attribute values forbidden' => [
+      ['*' => ['foo' => ['a' => FALSE, 'b' => FALSE]]],
+      NULL,
+    ];

This case fails. Because it's the first time we need to deal with "attribute values are forbidden".

Nothing in Drupal core can configure this, not even FilterHtml.

Theoretically, there could be filters in custom/contrib modules that implement this. But we've yet to see this.

So I want to go ahead with just expecting this test failure and adding a @todo.

Thorough unit test coverage. I expect this to fail, hard. Because none of the logic in \Drupal\ckeditor5\HTMLRestrictions has been updated to support this special "joker wildcard tag" yet.

Once these new unit tests pass, all tests should pass. 🤞

Add support for operations involving the joker wildcard tag *.

This will fix the \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testOperations() failures.

What's special about the joker wildcard tag: it does not resolve into concrete tags. But it is a wildcard tag. Update the logic accordingly.

This will fix the \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testSubsets() failures.

While working on this, I realized that this in filter_html is not actually supported by CKEditor 5 today:

    // 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],
    ];

… which means that content in Drupal 8/9 with CKEditor 4 that uses the lang attribute, or sets the dir="rtl" attribute, would be lost upon editing that content with CKEditor 5.

Here's a failing test that proves it.

While #27 proved the bug, it mixes it with other test coverage. That's bad.

Furthermore, once the #3259555: Organize and rename CKEditor 5 tests and test classes test reorganization happens, it'll be even more confusing.

So let's get this test right the first time.

Fix cspell + typo.

The test coverage in #7 was wrong: it shouldn't have expected the * tag to be returned by ::getAllowedElements(), only by ::getAllowedElements(FALSE) — because only the latter returns wildcards.

#3268307: $block wildcard resolves into a superset of the actual $block tags landed and conflicts. Rebased.

I've been working to get SmartDefaultSettingsTest to pass …

😬 Only now I discovered https://html.spec.whatwg.org/multipage/dom.html#global-attributes, which in turn led me to find #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default which introduced

    // 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],
    ];

😬 This made me realize that the approach in all of the patches above has been wrong on two counts:

1. The <*> "joker" tag is misnamed: it should be named the "global attributes" tag.
2. It should not behave like a wildcard tag, but like a concrete tag: precisely because it is to allow a particular attribute globally, it should not be resolved into a concrete set of tags.

As a nice consequence, the solution becomes very elegant too, and the original scope of the issue easily gets solved: FundamentalCompatibilityConstraintValidator starts to complain about global attributes supported by filter_html but not by CKEditor 5! 🥳

(This reverts #8's change to FundamentalCompatibilityConstraintValidator.)

As of #36, we should start to see concrete helpful failures. But … they're lacking a list of unsupported tags and attributes! That's because the violation constraint messages are using ::toFilterHtmlAllowedTagsString(), but as of #35, that's been updated to not ever generate a <* something> entry for the allowed_tags setting of FilterHtml, because that is not supported by that plugin.

So, we need to generate a list of unsupported tags/attributes slightly differently.

Now we should start to see The following elements are not supported: <* lang dir="ltr rtl"> as expected 🤓

So now we've established that <*> is the global attribute `*` HTML tag. Great.

… but ckeditor5_sourceEditing today has this in its plugin definition:

     # This is the only CKEditor 5 plugin allowed to generate a superset of elements.
     # @see \Drupal\ckeditor5\Plugin\CKEditor5Plugin\SourceEditing::getElementsSubset()
    elements: ['<*>']

… that is now problematic. And it sort of always was, but that's only become clear now 🙈

Let's refactor it to set elements: [] by default.

Yay, #36 is showing a huge increase in the number of test failures! That proves that the original scope of this issue has been met: validation for attributes allowed/forbidden on all elements (aka global attributes that are allowed or forbidden).

Time to add the plugins that we need to solve the data loss bug!

Introducing:

ckeditor5_globalAttributeDir

Supports <* dir="ltr rtl"> by using GHS

ckeditor5_globalAttributeLang

Supports <* lang> by using GHS

They're automatically enabled for text formats using filter_html.

I think this should be the final thing — making sure that we do not treat CKEditor 5 not supporting forbidding global attributes a problem at all, because CKEditor 5 specifically requires everything to be explicitly supported. If it's not supported at all, there's also no need to then explicitly forbid it.

Hopefully this will be green 🤞

The WildcardHtmlSupportTest failures are simple to fix.

One particular edge case that is being tested in \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::testPair() needed to have an expectation updated — easy!

Looks like the remaining failures are a genuine regression that this introduced. Will look into that tomorrow.

First, clean-up. Let's make this more consistent.

This way, reviews could already start without being too confusing 🤓

The current test failures seem to be related to html restrictions no longer restricting as expected (for example, a not-allowed <div> is permitted by CKEditor 5 instead of being stripped/converted to something supported).
I believe this is due to CKEditor 5's htmlSupport syntax behaving differently from Drupal's core

So a rule like

htmlSupport:
        allow:
          -
            name:
              regexp:
                pattern: /.*/
            attributes:
              - key: dir
                value:
                  regexp:
                    pattern: /^(ltr|rtl)$/

Seems to be saying ANY tag is allowed, and those tags may have a dir attribute.

With the plugins added in this issue I'm able to add any tag when editing source and it remains there. The newly-allowed tags can only use the attributes specified in ckeditor5_globalAttributeDir + ckeditor5_globalAttributeLang.
This could be addressed (I believe) by creating plugin classes that dynamically provide the GHS config, I'm not sure if there are aspects that should be handled upstream instead.

#57 is spot-on. The solution is very simple though: we just need to not pass this global attribute * HTML tag to CKEditor 5, because it's a Drupal-specific concept, not a CKEditor 5 one … and we can instruct CKEditor 5 exactly which tags it should allow these attributes on, because we know what the full list of supported HTML tags is!

That idea is very simple, now to actually implement it 🤓

This implements #58.

100% unrelated test failures, from Quick Edit to Migrate 🤪

Feedback addressed, this is RTBC!

3759c178 in patch form!

All feedback addressed 👍

Error: Call to undefined method Drupal\ckeditor5\HTMLRestrictions::isEmpty()

/var/www/html/core/modules/ckeditor5/src/SmartDefaultSettings.php:178

This makes no sense at all, because that line in this MR contains return [$editor, $messages]; 🤪🙃.

There are only two possible ways that this only reason I could possibly understand this happening: GitLab is wildly broken and testing the wrong commit, or the branch needs to be rebased. Let's assume it's the latter.

Ah, now it makes sense! #3261943: Confusing behavior after pressing "Apply changes to allowed tags" with invalid value just landed and it added code to SmartDefaultSettings, and that needs to be updated still. 👍

Fixed now 😄

Back to RTBC. Leaving the method renaming thread open since that's a bit subjective, but I offered some feedback there.

Commit 5c0879dec73257c25762825bf8b4427cab0f3b71 in patch form, should apply to all 3 branches.

This unblocked #3228691: Restrict allowed additional attributes to prevent self XSS, unpostponed! 👍

Discovered an overlooked aspect: #3314478: Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified.