Follow-up for #3231334: global attributes should result in HTMLRestrictions becoming simplified

Expand test coverage more: let's make sure that we really did not miss any edge case this time around! 🤓

1. +++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
   @@ -538,6 +538,74 @@ public function providerConvenienceConstructors(): \Generator {
   +    yield '<p foo="a b" bar> + <* foo="b a"> results in <p> getting simplified' => [
   +      '<p foo="a b" bar> <* foo="b a">',
   +      ['p' => ['bar' => TRUE], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]],
   +    ];
   +    yield '<* foo="b a"> <p foo="a b" bar> results in <p> getting simplified' => [
   +      '<* foo="b a"> <p foo="a b" bar>',
   +      ['p' => ['bar' => TRUE], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]],
   +    ];
   +    yield '<p foo="a b c"> + <* foo="b a"> results in <p> getting simplified' => [
   +      '<p foo="a b c"> <* foo="b a">',
   +      ['p' => ['foo' => ['c' => TRUE]], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]],
   +    ];
   +    yield '<* foo="b a"> <p foo="a b c"> results in <p> getting simplified' => [
   +      '<* foo="b a"> <p foo="a b c">',
   +      ['p' => ['foo' => ['c' => TRUE]], '*' => ['foo' => ['b' => TRUE, 'a' => TRUE]]],
   +    ];
   

   This is adding test coverage for attribute restrictions on a tag being a superset of the restrictions on the global attributes <*> tag.

2. +++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
   @@ -538,6 +538,74 @@ public function providerConvenienceConstructors(): \Generator {
   +    // Attribute restrictions that match the global attribute restrictions
   +    // should be omitted from wildcard tags.
   +    yield '<p> <$text-container foo> <* foo> results in <$text-container> getting simplified' => [
   

   This (and the dozens of subsequent test cases) is adding test coverage for wildcard tags — the test coverage up to this point only dealt with concrete tags.

Fix.

That had some unintended consequences. Fixing…

Docs + clean-up.

Run all tests again.

This is what I allow. With the patch I noticed that no element is getting the "dir" attribute now. Is that expected?

Yes, it is! And for clarity: by "no element is getting […]", you mean filter_html's allowed_html setting.

Because the <dir> attribute gets special treatment from filter_html:

'#description' => $this->t('A list of HTML tags that can be used. By default only the <em>lang</em> and <em>dir</em> attributes are allowed for all HTML tags. Each HTML tag may have attributes which are treated as allowed attribute names for that HTML tag. Each attribute may allow all values, or only allow specific values. Attribute names or values may be written as a prefix and wildcard like <em>jump-*</em>. JavaScript event attributes, JavaScript URLs, and CSS are always stripped.'),

and

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

Very interesting question! 🤓

filter_html does not allow configuring <* bar>. One can configure filter_html to allow <p style>, but <* style> will still be applied anyway.

It's only for security reasons that HTML restrictions are able to specify globally disallowed attributes. It does not make sense to override it, at all, ever, because filter_html will simply ignore it. That's why it never even crossed my mind to test this! 🤓

But you're of course right that if we're explicitly testing the behavior for the "collision with globally allowed attributes" case, that we should also do the same for the "collision with globally disallowed attributes" case.

ℹ️ In fact, this is very closely related to #3312807: Harden CKEditor 5 against self-XSS through source editing, where we will be improving the UX to explicitly fail validation when that occurs. Documented this explicitly at #3312807-4: Harden CKEditor 5 against self-XSS through source editing.

Conclusion: it's important to provide explicit DX when a user/developer tries to override globally disallowed attributes to become allowed. We need test coverage for this. I'm happy to split this off into a separate issue if you prefer, but I'm equally fine with doing it here. For now, getting it going here, because it is the other side of the same aspect. Attached patch adds unit test coverage with expected failures.

Tricky: Unfortunately, like many other aspects of the Filter module, there is no validation logic in \Drupal\filter\Plugin\Filter\FilterHtml at all. This means that it's totally possible there are sites out there that have <p style> configured as their allowed_html … and that configuration is treated as valid, but 100% certainly ignored. So we need to handle that gracefully. I'll do that in a next patch iteration.

Fix.

Conclusion: it's important to provide explicit DX when a user/developer tries to override globally disallowed attributes to become allowed. We need test coverage for this. I'm happy to split this off into a separate issue if you prefer, but I'm equally fine with doing it here. For now, getting it going here, because it is the other side of the same aspect. Attached patch adds unit test coverage with expected failures.

This was addressed by #18 (tests) and #20 (fix).

Next up:

Tricky: Unfortunately, like many other aspects of the Filter module, there is no validation logic in \Drupal\filter\Plugin\Filter\FilterHtml at all. This means that it's totally possible there are sites out there that have <p style> configured as their allowed_html … and that configuration is treated as valid, but 100% certainly ignored. So we need to handle that gracefully. I'll do that in a next patch iteration.

Here's the test coverage. And … the new test failure in #20 actually reveals that this would've been caught anyway! 🤓 So this is just adding explicit test coverage.

This should fix all tests … except one.

😶

Hah, that last failure is actually correct. It is not something you would ever see in the UI, because as described at #3312807: Harden CKEditor 5 against self-XSS through source editing, n thanks to #3228691: Restrict allowed additional attributes to prevent self XSS the UI will warn you:

The additional expectation added in this interdiff is for the validation error thrown by FundamentalCompatibilityConstraintValidator. And it is accurate, because it validates an existing text editor config entity that DOES have the SourceEditing plugin configured to allow e.g. <p onhover>, which filter_html indeed does not allow, so it is indeed a fundamental compatibility problem. You'll just never be able to create such an invalid text editor config through the UI, only by manipulating config by hand and forcefully importing it.

Hence this interdiff is actually just confirming that the changes in #24 are correct! 🥳

Green! Time to run all tests again 🤓

#31: Thank you, I was struggling with the wording 😅

#32:

1. Discussed with @lauriii — we actually are mostly using self elsewhere, I just keep forgetting about it 🙈 — now consistently using self 👍

1. 👍 Done!

1. 👍 Done!
2. Yes! 😅 Good catch :)

Unrelated random fail; re-testing.

…