[GHS] Unable to limit attribute values: ::allowedElementsStringToHtmlSupportConfig() does not generate configuration that CKEditor 5 expects

Related: we do not yet properly take this into account in the automatic upgrade/migration path — we have #3231328: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets for that, and its blocker #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object is what I've been actively working on for the past ~week.

What you're describing here is going further though: it's not that the CKEditor 4 configuration was not transformed correctly to equivalent CKEditor 5 configuration, it's that CKEditor 5 does not support restricting these attributes at all. This is what CKEditor 4's Advanced Content Filter ("ACF" for short) allowed to configure, which we brought to Drupal core in #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings and automatically configured it to match the HTML restrictions of the text format (usually, just those of filter_html).

To my knowledge, they intentionally did not port ACF to CKEditor 5 because it was extremely complicated to maintain. But that's what I vaguely recall, I'm not 100% certain about it.

I will raise this in today's CKEditor 5 meeting.

P.S.: You marked this Normal. Is that intentional? Would you consider this a Major bug, or in other words, a stable-blocking regression that prevents us from moving to CKEditor 5? You did make #3238333: Roadmap to CKEditor 5 stable in Drupal 9 the parent issue, so I suspect you would.

Alright, marking Major and tagging stable blocker.

P.S.: I personally agree, but was especially interested in your perspective as a core committer 😊

Per yesterday's CKE5 meeting, looks like we should be able to restrict this at least for attributes that no pre-existing CKE5 plugin supports natively using GHS — only for restricting attributes on values that are supported by CKE5 plugins will this be a problem.

Investigating now.

I get a PHP assertion error if follow your steps to reproduce:

The website encountered an unexpected error. Please try again later.
AssertionError: assert($value === TRUE || Inspector::assertAllStrings($value)) in assert() (line 242 of core/modules/ckeditor5/src/HTMLRestrictionsUtilities.php).
assert(, 'assert($value === TRUE || Inspector::assertAllStrings($value))') (Line: 242)
Drupal\ckeditor5\HTMLRestrictionsUtilities::allowedElementsStringToHtmlSupportConfig('<cite><dl><dt><dd><a hreflang class="button"><blockquote cite><ul type><ol start type>') (Line: 78)
Drupal\ckeditor5\Plugin\CKEditor5Plugin\SourceEditing->getDynamicPluginConfig(Array, Object) (Line: 254)
Drupal\ckeditor5\Plugin\CKEditor5PluginManager->getCKEditor5PluginConfig(Object) (Line: 805)
…

Any chance you're running this with assertions off, @larowlan?

But … this is basically a bug in \Drupal\ckeditor5\HTMLRestrictionsUtilities::allowedElementsStringToHtmlSupportConfig(), because it generates this datastructure:

array (
  0 => 
  array (
    'name' => 'cite',
  ),
…
  4 => 
  array (
    'name' => 'a',
    'attributes' => 
    array (
      'hreflang' => true,
      'class' => 
      array (
        'button' => true,
      ),
    ),
  ),
…
  ),
)

So: attributes: { class: { button: true }}. This is wrong per https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/gener.... This is a bug in \Drupal\ckeditor5\HTMLRestrictionsUtilities::allowedElementsStringToHtmlSupportConfig(), which will be easier to fix after #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object lands, because then we'll be able to write unit tests for this.

That being said, even if I fix that manually, I can see that it still does not work as expected. Will continue investigating on Monday.

Alright. I fixed it: I made \Drupal\ckeditor5\HTMLRestrictionsUtilities::allowedElementsStringToHtmlSupportConfig() generate CKE5 configuration matching the docs at https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/gener....

Now your original use case (allow class="button" but not anything else, including class="use-ajax") works, but I can find several that don't. Basically, it looks like custom attributes (foo="bar" on various tags) do not work at all:

1. not for <a> (links are pretty special)
2. not for <blockquote> nor <p> (block quotes are not very special)
3. not for <h6> (headings are "medium special")
4. not even for <cite> which is not provided by any CKE5 plugin, but allowed thanks to GHS!
5. not for Drupal's custom <drupal-media>

For the latter, we already have #3227822: [GHS] Ensure GHS works with our custom plugins, to allow adding additional attributes — I would not expect that one to work just yet. But the others should. I created https://github.com/ckeditor/ckeditor5/issues/11155 for those.

Digging deeper … turns out that https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_view_matche... actually suggests that listing allowed values is not possible, unlike what https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/gener... suggests.

https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/gener... explicitly lists classes: ['foo', 'bar']. But it does not list that for individual attributes … 🤔 The other docs page suggests we should use regular expressions. And while PHP's json_encode() cannot generate those, this module's JS fortunately already supports that.

And … yes … that does the trick!

That means this does not need an upstream feature. It works fine. This is just a big bug in \Drupal\ckeditor5\HTMLRestrictionsUtilities::allowedElementsStringToHtmlSupportConfig()! 😬 There is zero pre-existing test coverage. But over at #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object I've already written basic test coverage for it. So let's mark this issue postponed on that one. Once that's in, this will be easy to do.

But that will just cover the unit test portion of this. I think we should have at least 1 functional JS test too, to prove that it actually works. That part is not actually blocked/postponed.

Note that while we're tackling this, we should probably also add test coverage for cases like id="jump-*" — i.e. for allowed attribute values with wildcards. We could descope that, but it would require touching the same code, so it feels like a good fit.

Note that while we're tackling this, we should probably also add test coverage for cases like id="jump-*" — i.e. for allowed attribute values with wildcards. We could descope that, but it would require touching the same code, so it feels like a good fit.

We have an explicit issue for that: #3260853: [GHS] Partial wildcard attributes (<foo data-*>, <foo *-bar-*>, <foo *-bar>) and attribute values (<h2 id="jump-*">) not yet supported — let's tackle that there.

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

Rerolling #11.

Should we add integration test for this too to confirm that the configuration is working?

IMHO that's not necessary: that not working would be a bug in upstream CKEditor 5. We don't want to write Drupal-owned tests for every piece of CKEditor 5 functionality, right?

Still, I see the appealing reassurance of it too!

I don't feel strongly, could go either way :)

The reason I'm asking that is because the configuration looks good for me, but that doesn't give me 100% confidence that this actually works so I would test that manually. Instead of testing that manually, we could write an automated test that ensures that this continues to work in future.

Fair enough — better to remove all doubt.

+++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
@@ -807,8 +807,19 @@ public function toGeneralHtmlSupportConfig(): array {
+          // Drupal never allows style attributes due to security concerns.
+          // @see \Drupal\Component\Utility\Xss
+          if ($name === 'style') {
+            continue;
+          }

This will get validation in #3260857: Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values.

Here's the test coverage that @lauriii requested. It will fail. Because the unit test coverage was not complete enough.

Here's a unit test for the bug discovered in #22. But we still want/need the functional JS test coverage too, for the reasons @lauriii mentioned.

Fix.

Test coverage looks good ✨

Glad that we discovered #22 before committing this 😅

Just some nits!

1. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -807,8 +807,19 @@ public function toGeneralHtmlSupportConfig(): array {
   +            : $value;
   

   Add a doc describing what is happening here.

2. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -807,8 +807,19 @@ public function toGeneralHtmlSupportConfig(): array {
   +          $to_allow['attributes'][$name] = is_array($value)
   +            ? ['regexp' => ['pattern' => '/^(' . implode('|', $value) . ')$/']]
   +            : $value;
   

   This bit should get a doc

3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
   @@ -0,0 +1,205 @@
   +   * A host entity with a body field to embed images in.
   

   I'm guessing this is a copypaste artifact since this isn't testing image stuff.

4. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
   @@ -0,0 +1,205 @@
   +  protected $defaultTheme = 'classy';
   

   This works fine with plain old Stark as the default theme.

5. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
   @@ -0,0 +1,205 @@
   +    // Create a sample host entity to embed images in.
   

   I think this is another copypaste artifact

6. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
   @@ -0,0 +1,205 @@
   +  public function testAllowingExtraAttributes(string $expected_markup, ?string $allowed_elements_string = NULL) {
   

   GOOD TEST

Per #3260853-13: [GHS] Partial wildcard attributes (<foo data-*>, <foo *-bar-*>, <foo *-bar>) and attribute values (<h2 id="jump-*">) not yet supported.

Addressed all points in #28.

🛳 🚀 💥

+++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
@@ -817,6 +817,12 @@ public function toGeneralHtmlSupportConfig(): array {
+          // @todo Expand to support partial wildcards in https://www.drupal.org/project/drupal/issues/3260853.

Nit: We might want to move the URL to the next line (not sure since this isn't consistent across core). Can be fixed on commit.

This now also blocks #3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal.

Committed to 10.0.x, 9.4.x and backported to 9.3.x since CKEditor 5 is experimental. Addressed #32 on commit.

Pleased that these use cases are being spotted sooner than later, and even more pleased at the level of test coverage accompanying the fix!