Broken "Allowed Tags" updating: after all values for an attribute are allowed, it should not be overridden to allow only certain attribute values

Good find!

And thank you for the very thorough steps to reproduce, that makes this much easier to fix!

I can reproduce this error.

Still an issue

The issue seems to be in this file core/modules/ckeditor/js/ckeditor.stylescombo.admin.es6.js

This block of code:

  /**
   * Ensures that the "stylescombo" button's metadata remains up-to-date.
   *
   * Triggers the CKEditorPluginSettingsChanged event whenever the "stylescombo"
   * plugin settings change, to ensure that the corresponding feature metadata
   * is immediately updated — i.e. ensure that HTML tags and classes entered
   * here are known to be "required", which may affect filter settings.
   *
   * @type {Drupal~behavior}
   *
   * @prop {Drupal~behaviorAttach} attach
   *   Attaches admin behavior to the "stylescombo" button.
   */
  Drupal.behaviors.ckeditorStylesComboSettings = {
    attach(context) {
      const $context = $(context);

      // React to changes in the list of user-defined styles: calculate the new
      // stylesSet setting up to 2 times per second, and if it is different,
      // fire the CKEditorPluginSettingsChanged event with the updated parts of
      // the CKEditor configuration. (This will, in turn, cause the hidden
      // CKEditor instance to be updated and a drupalEditorFeatureModified event
      // to fire.)
      const $ckeditorActiveToolbar = $context
        .find('.ckeditor-toolbar-configuration')
        .find('.ckeditor-toolbar-active');
      let previousStylesSet =
        drupalSettings.ckeditor.hiddenCKEditorConfig.stylesSet;
      const that = this;
      $context
        .find('[name="editor[settings][plugins][stylescombo][styles]"]')
        .on('blur.ckeditorStylesComboSettings', function() {
          const styles = $.trim($(this).val());
          const stylesSet = that._generateStylesSetSetting(styles);
          if (!_.isEqual(previousStylesSet, stylesSet)) {
            previousStylesSet = stylesSet;
            $ckeditorActiveToolbar.trigger('CKEditorPluginSettingsChanged', [
              { stylesSet },
            ]);
          }
        });
    },
}

I think we should replace that !_.isEqual() check with something that instead checks to see if the rules are satisfied. It shouldn't really matter if they are equal.

So the logic should really be:

if not _isSatified(previousStylesSet, stylesSet)
  previousStyleSet = _generateStyleRules(styleSet)
  trigger CKEditorPluginSettingsChanged

_generateStyleRules = function(CKEditorStyleRules)
  textFormatStyleRules = drupalSettings.ckeditor.hiddenCKEditorConfig.stylesSet
  satisfiedRules = CKEditorStyleRules.reduce()
    if CKEditorStyleRule not in textFormatStyleRule
      return CKEditorStyleRules
  return CKEditorStyleRules + textFormatStyleRules
  

The above is Pseudo code. _isSatisfied will need to check to see if the new style rules are in the existing rules.

I'm also seeing this issue, and it's interfering with our sitewide CSS. We need to both create specific classes that will be available in the Styles dropdown, but also allow all classes on certain tags.

In our case it was working fine until we updated to Drupal Core 9.2.8 today. Now when we correct the tags to remove the specific class declarations (<h3 class>) and save the results, the settings revert back show the same tags with restricted classes (<h3 class=" classname" > ). Adding a wildcard doesn't help either (<h3 class=" * classname">).

Are there any suggestions on how we could fix this?

Edited to add:

This does work, but it's less than ideal:
<h3 class="a* b* c* d* e* f* g* h* i* j* k* l* m* n* o* p* q* r* s* t* u* v* w* x* y* z*">

unfortunately, beyond being unnecessary, this issue can be partcularly detrimental for some users: https://www.drupal.org/project/drupal/issues/3248984

This patch will ignore the classes provided in the "styles dropdown" as long as the tag added in Allowed HTML field has empty classes i.e <div class="">

I tried #15 patch, but it did not work. I have classes set for `p`, `span`, 'div` in the style dropdown. I set the class to `class=""` in the Allowed HTML Tags for them and save. When I edit the text format again and check the Allowed HTML Tags it has put back all the classes from the Styles dropdown.

Can confirm I'm also seeing this and #15 did not work for me either.

I think this could be a major issue because it's forcing users to either abandon ckeditor styles or turn off limiting html tags. This bug is causing issues on several gov sites leveraging uswds library

it is a major issue fr various reasons. I lost styling for various pages as a result.

So the patch in #15 actually did appear to work
Updated it for 9.3 and allowed for users to put '' or '*'

I don't think the check is broken actually, seems to me that is by design and actually doing exactly what it needs to do.
Allowing any classes with the '*' pattern and also forcing to limit and correct HTML seems impossible to me. You can't limit html and then allow every class... It is either limiting html and defining all usable classes, or not limiting HTML.

You can get around this issue by simply defining all classes that you want to allow within the ckeditor.

What was a actual issue, for me at least, is that putting styles on the p-element also requires you to define all classes that can come from other buttons in ckeditor. For example: p.text-align-center comes from the text alignment buttons and that class does not get added automatically.

The issue is you are allowed to add class * right now to any tag. But ckeditor styles overrides whatever you have. And some design libraries have 1000s of classes

> You can't limit html and then allow every class... It is either limiting html and defining all usable classes, or not limiting HTML.

By doing this we are forcing people who use design libraries to not limit the html and open up the site to XSS. They are being forced because that is the easiest way for this to work. This isn't and impossible problem to solve <p class> works already. The problem is changing the setting in the CKEditor styles dropdown overwrites <p class> to only include the styles from the CKEditor plugin.

By design <p class> works. CKEditor breaks it.

@smustgrave has explained why @weseze's analysis is flawed, which is my specific use case here.

Comments

I agree that this can negatively affect a site (happened to mine last month) because the messaging is not clear. If the behavior is by design, which I think it largely is, then there should be a clearer warning about what enabling the "Limit allowed HTML tags..." checkbox will do if you already have classes assigned in your content. The checkbox label, as it is now, says nothing about affecting CSS classes.

Perhaps the label can say, "Limit allowed HTML tags, correct faulty HTML and restrict CSS classes" with help text that includes something like, "Restricts CSS classes to only those listed in the CKEditor Styles Dropdown config above. Warning: enabling this will remove all existing classes from CKEditor content that are not listed above and change the Filter Settings for Limit allowed HTML tags to reflect allowed classes defined above." Something like that.

Possible remediation for affected sites

If you have recently been affected by this bug/issue, there may be a way to restore the original classes to CKEditor content if the node hasn't been edited since the affected date. I just successfully accomplished this on two installations.

I simply disabled the "Limit allowed HTML tags..." checkbox and saved the configuration. All the CKEditor content that had lost styles due to this bug now have said styles again, as long as I haven't edited the node since. This may be due to the fact that the content is still saved in the database with styles intact and CKEditor strips them upon loading the content. I haven't dug that far to confirm this. But it worked for me.

As far as restoring your "Allowed HTML tags" filter settings (which would have been replaced when "Limit allowed HTML tags..." was enabled), I don't know how to retrieve the original value of that field. Hopefully you have a backup copy for that :)

Cheers

I agree with other saying this issue is a bug. The most important part about the HTML filter is to limit allowed tags -- requiring that attribute values get limited in order to use the styles drop-down is overstepping. Optionally, you should be allowed to limit classes and other attribute values by entering allowed values allowed classes, but that should be optional.

A fragile workaround we've got right now is to change the allowed HTML tags setting to allow any classes, like it was before, and save -- it seems to stick that way, until the next time you have to edit that text format.
>> But, I'll codify this change, so as long as we're super careful (risky) not to undo this change, and then I can just config import the settings back.

Of course, you can change this before you save, but you'll have to do it every time you edit the configuration.

This patch is cleaner and will skip updating the "Allowed HTML tags" altogether if the tag has * in the classes list i.e <p class="*">

This patch is cleaner and will skip updating the "Allowed HTML tags" altogether if the tag has * in the classes list i.e

Fixing custom commands failures

Does anyone have a guess as to why a bunch of us are coming to this issue now / in the last few months?

Is it really just that we all had to upgrade to D9 in November, or did something change in the filter module in the last couple months? I have no proof, but the effects of this issue are REALLY significant for us, and it feels unlikely that no one on our team would've noticed the regressions between early November and now (and actually, most of our sites went to D9 last summer). I will attempt to find out for sure.

-------
(Also changing to NR.)

I wonder if another way to go about fixing this issue could be to change how the filter treats <tagname class="class1 class2 *"> -- I guess maybe it's icky to have those specific classes superfluously added to the allowed HTML setting, but, if the * wildcard meant all classes would be permitted, that would be great -- and then the "automatically update allowed_html based on text editor plugin settings" can still do its thing, without causing harm?...

Doesn't it seem like a bug that wildcards have the effect you'd expect if "part of" a class name, but not on their own, if the setting for that attribute also includes specific values?
>> For example, if I have <div class="social-icon button-*"> in the allowed html settings, when I edit a rich text field, the text filter allows <div class="button-link">, but if I have <div class="social-icon *">, the text filter strips the "button-link" class.

Anyway, I'm not sure I like this idea, but, brainstorming.

-------
Other comments/thoughts from my digging this evening:

(1) It's just clicked for me that the recommended (required?) syntax for allowing a tag with an attribute with no restrictions on the attribute values is <tagname attribute="*">, not <tagname attribute> as many of us have been doing for a while.
>> Do y'all agree, or am I misunderstanding?
>> The fix proposed by @akhoury only works if you use the class="*" syntax, right? -- if you've got <tagname class> in your allowed html settings, the proposed fixes aren't going to help you. (And maybe that's okay, because maybe we're supposed to be doing class="*", I just don't actually know.)

(2) Speaking of which...
Another manifestation of the issue we're all having, if you are using the <tagname attribute> syntax like I was: All tags in "allowed html tags" that had class on them before (not class="*") and that DON'T have any related setting in the styles dropdown are all now class="".
>> BUT, if I changed these instances to class="*", they didn't get messed up when I went to the text format configuration form.

(3) I'm running 9.3.6, but I did try updating to 9.3.7 -- no luck (I tried because 9.3.7 does touch FilterHtml.php -- couldn't hurt to check).
>> I also tried the patch on #20 on this issue concerning SmartDefaultSettings -- no dice.

(4) I can't find any recent change to filter.filter_html.admin.es6.js (or ckeditor.stylescombo.admin.es6.js) that could've caused this bug to (re?)surface in the last few months, so probably it's a not-new Drupal 9 issue, and the recent attention is because we all had to upgrade to Drupal 9 a few months ago.
>> For whatever it's worth...
I tried reversing the fix for JS error with elements in "allowed HTML tags" that can't be direct descendants of a div -- before that commit/fix, when I go to configure my text format, all instances of <tagname class> were changed to <tagname class=""> -- still broken, just, the JS error meant the stylecombo classes didn't get added.

----
Lastly! A reminder / comment of validation for those of us struggling with this issue: Here's the help text for filter_html > allowed_html shown to users on the text format/editor config page (emphasis added):

A list of HTML tags that can be used. By default only the lang and dir 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 jump-*. JavaScript event attributes, JavaScript URLs, and CSS are always stripped.

This seems critical because it will lead to data loss if a node is saved: classes will be stripped permanently from the HTML.

> Does anyone have a guess as to why a bunch of us are coming to this issue now / in the last few months?

Something definitely changed very recently, because I had just "<a class>" in my allowed HTML settings and it was working on 9.3.0 (held back to not mess up migrations), then I pushed a site live and updated to 9.3.9. Today I needed to adjust some format settings and that's when it broke.

After operating numerous production sites for many years, this issue has recently started causing data loss on production sites. Any edit to the text format, including simply updating a filter configuration and never activating the tab for allowed HTML tags causes the correct list to be overwritten, and a truncated list to replace it, leading to data loss on page edits.

Changing the allowed HTML tags without prompting, and with no way to keep it from happening is very problematic. At a minimum, the user should be provided with a prompt to OPTIONALLY update the allowed HTML tag list if the issue can not be readily resolved.

The given patch showing some undefined errors. I have corrected the condition and updated the patch.

(1) It's just clicked for me that the recommended (required?) syntax for allowing a tag with an attribute with no restrictions on the attribute values is , not as many of us have been doing for a while.

That has never been correct.

If you want to allow the attribute bar on the tag foo with no restrictions on which values can be set on that attribute, it's always been <foo bar. So for example, to allow classes on <div>s: <div class>.

The only way the asterisk should be used is what the form description says: Attribute names or values may be written as a prefix and wildcard like jump-*. So you could say <div data-*> to allow any data- attribute on <div>s, or you could write <div class="themename-*"> to only allow classes provided by a particular theme.

For that reason #31/#38 is definitely wrong.

I think the #1 problem here is that \Drupal\filter\Plugin\Filter\FilterHtml::settingsForm() has a horrible UX with no validation whatsoever. So things that seemed to work, do not actually work. 😔 And that is a historical problem that's kinda hard to fix.

Reproducing

I just added

<div class>

to the list of allowed HTML tags. And saved it. I can confirm that this is saved correctly into configuration.

Upon editing, I see this in the UI:

<div class="">

→ this means no classes are allowed at all. This is definitely a critical bug! This also means this must be happening on the client side, in JS.

I looked at recent changes to core/modules/filter/filter.filter_html.admin.es6.js and found something that looked suspicious. Reverting that fixes the bug I just mentioned: #2556069: JS error with elements in "allowed HTML tags" that can't be direct descendants of a div.

Except … that just brings back the JS error that that fixed: TypeError: Cannot read property 'tagName' of null in Drupal.behaviors.filterFilterHtmlUpdating. It was that crash which was preventing this problem from happening 🙃

Conclusion

To be able to sanely push this forward, the first thing we need is test coverage. Failing tests. Because I think that #15 is on the right track, but without test coverage it's hard to know for sure.

Is there a doc page on how to write js tests for Drupal core?

@frob:

https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/phpunit-...

This WILL lead to data loss if not resolved with the 9.4.0 release.

@KarlShea, it's been leading to data loss for months. Some maintainers may not even know they've been bitten yet.

Thanks for the investigation and clarifications @Wim Leers!

Just in case it makes a difference:

The only way the asterisk should be used is what the form description says: Attribute names or values may be written as a prefix and wildcard like jump-*. So you could say <div data-*> to allow any data- attribute on <div>s, or you could write <div class="themename-*"> to only allow classes provided by a particular theme.

The other way we use asterisks is a variation on <div class="themename-*">:
<div class="intro highlight *">

But, as I think about it, that's probably only because simply using <div class> doesn't work, because the next time you're on the page, it gets filled-in based on what's in the "Styles dropdown" setting.

So, probably it doesn't matter, but I'm saying it anyway, apparently? 🙃 Y'never know what'll spark something for someone.

Meanwhile, I'll update our internal documentation about the intended/correct way to allow all possible values of an attribute (<div class>), so that we're in the right spot later on when the dust settles.

Based on #39, here is a reroll of #15 for 9.5.x, that also fixes the <div class=""> to be <div class>.

Novice attempt at writing a test.

@ChaseOnTheWeb if you're going to upload a tests-only patch and regular you should include them in the same comment. Uploading the tests-only patch first then the regular. Also your regular patch should include the tests also, ie a full patch. Essentially you want to see the tests-only patch fail and the regular one pass to show a good test case.

Also you can precheck the code for standards before uploading using /core/scripts/dev/commit-code-check.sh

Closed #2693659: Empty quotes get added to class attribute in "Limit allowed HTML tags" as duplicate of this ticket.

Moving credit from the duplicate to here, as asked for in #bugsmash.

So removed the code that was checking for class="" but tested manually that

Example 'H2 class' works and if you add a ckeditor style it will add the class to the allowed format correctly too. Example h4 class="test"

Per request in #bugsmash, adding credit for @smulvih2 for work done in closed duplicate #3218499: Allow toggle of automatic update functionality for allowed HTML list

So removed the code that was checking for class="" but tested manually that

Example 'H2 class' works and if you add a ckeditor style it will add the class to the allowed format correctly too. Example h4 class="test"

0 Idea why that resubmitted ignroed #63

I created a patch in related ticket from comment #52 above that allows users to disable/enable this feature. I use this patch on a few sites and it works great. I think even if the bug(s) in this ticket are resolved it would be nice to have the ability to disable this feature altogether.

@smulvih2 what issue did you create that patch for because that sounds exactly like what I want.

@smulvih2 looks like this is the patch https://www.drupal.org/files/issues/2022-05-18/drupal-core-filter-automa...

It would be great if this could get rolled into this issue but I think this should be opened as a feature request in another issue or be rolled into a contrib module somehow.

@frob, the issue I am having is when elements are defined in the Styles dropdown, the allowed HTML tags are being updated with these classes, removing the values I had previously defined. For example, if I have a Style defined for <p> with a specific class, this automatically updates my allowed HTML list to only allow that specific class. So if my allowed HTML list has <p class> to allow any class, it is changed to only allow the one class I have defined in my Styles dropdown. This was causing major issues for me across my site. This functionality is coming from core/modules/filter/filter.filter_html.admin.js. My patch allow us to toggle this functionality. I think it's a nice option, and even if the root cause of this issue is fixed I would still want to disable this JS on my sites.

Maybe the related ticket from comment #52 should be reopened to allow this toggle, as this current ticket looks to resolve the JS issue; I think both fixes would be nice to have.

https://www.drupal.org/project/drupal/issues/3218499#comment-14519316

@smulvih2
I understand the problem. When I said issue I was referring to the issue node. I was able to find it.

The other issue (the one you posted that patch in originally) was closed as a duplicate and should stay that way. You should open a new feature request for your patch so this one doesn't get any more delayed than it already is. I would recommend opening a new issue for your patch (because I think it is a really good idea) and also see if you can create a contrib module that does what your patch does. It would be more difficult in the contrib space but this issue is over 6 years old and it has become just as likely for CKEditor4 to be removed from core as it is for this issue to be resolved.

nice that it's beeing discussed already, similar issue with CKE5: #3294908: Configuration overlaps between Styles and other CKE5 plugins

Verified that the patch in #68 works in Drupal 9.4.3 on several different sites. Thanks for this long-needed fix!

@SamLerner if you feel this fixes your issue please review your logs and the patch and mark RTBC so we can maybe get this fix in. Thanks!

Setting as RTBC. Thanks again @smustgrave!

Needs a re-roll for 10.x

Not 100% sure this is needed in D10

I added a style for a link "a.test|Link Test"
Then updated "source editing" field to <a hreflang class>
And the allowed tag is <a class hreflang href>

So in CKEditor 5 the allowed tags field is readonly and controlled by CKEditor 5 itself. But I think we should still commit the change to filter.filter_html.admin.js to 10.0.x to keep it in sync with 9.5.x, even though this codepath is unused in core and I'm not sure that we can test it? But if this had been committed before CKEditor 4 was removed, it would also have made it into 10.0.x.

Can't you still have allowed tags in Drupal 10 without ckeditor5 being enabled at all?

Here's a D10.1 patch

Fixed the test case

The test alone from #79 doesn't fail for me, which suggests either the test isn't quite right or the affected codepath is bypassed when CKEditor 5 is used?

the affected codepath is bypassed when CKEditor 5 is used?

This is correct.

core/modules/editor/js/editor.admin.js provides the following public JS APIs:

1. Drupal.editorConfiguration
2. Drupal.EditorFeatureHTMLRule
3. Drupal.EditorFeature
4. Drupal.FilterStatus
5. Drupal.FilterHTMLRule
6. Drupal.filterConfiguration
7. Drupal.behaviors.initializeFilterConfiguration

Crucially, none of this has any test coverage, because it predates FunctionalJavascript testing infrastructure.

Its docs say:

Provides a JavaScript API to broadcast text editor configuration changes.

Filter implementations may listen to the drupalEditorFeatureAdded,
drupalEditorFeatureRemoved, and drupalEditorFeatureRemoved events on document
to automatically adjust their settings based on the editor configuration.

Mea culpa

I wrote this, in #1894644: Unidirectional editor configuration -> filter settings syncing, over 9 years ago. It was how we made the UX for configuring CKEditor 4 somewhat decent: whenever you changed the CKEditor 4 toolbar (added buttons) or changed plugin settings (e.g. configured more styles for StylesCombo), it'd automatically update the filter_html filter configuration, to keep it in sync.

I already knew it was not great. It was a necessary evil. The follow-up work to make it decent (#2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11) never happened.

The reason it's all in JS is simply because for CKEditor 4, it is impossible to know which HTML tags, attributes and attribute values a particular CKEditor capability needs (requires) or (optionally) supports. So … for CKEditor 4, we had a hidden CKEditor 4 instance right in the configuration UI, to query the capabilities "live". That's how we were able to keep filter_html in sync. Imperfectly (there are many bug reports around this, for example #2649546: [upstream] Alignment/justify buttons not appearing for CKEditor, <p class="text-align-left text-align-center text-align-right text-align-justify"> not being allowed automatically), but … better than nothing.

With CKEditor 5, we do know on the server side which HTML tags/attributes/attribute values are supported. This was critical to be able to have an upgrade path at all.

Which means all that infrastructure is unused in Drupal core.

That leaves one key question: does contrib use it at all? Let's find out:

Text Editors triggering

1. addedFeature → 0 results
2. removedFeature → 0 results
3. modifiedFeature → 0 results

Conclusion: not a single text editor besides core's CKEditor 4 supported this.

Text Filters reacting to Text Editors

1. drupalEditorFeatureAdded → 1 result: linkit
2. drupalEditorFeatureRemoved → 1 result: linkit
3. drupalEditorFeatureModified → 0 results

Conclusion: only one.

Recommendation

Based on this, I would recommend a pragmatic but unusual course of action:

1. Commit the fix since the work has been done, to 9.4, 9.5, 10.0 and 10.1
2. But only commit the test to 9.4 and 9.5 because only there can it be tested, since CKEditor 4 does not exist in Drupal >=10
3. Repurpose #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11 to add deprecation warnings to these for Drupal 9.5, 10.0 and 10.1, to indicate these will be removed from Drupal 10.2

Result: the problem is fixed for CKEditor 4 for as long as that is around (9.4, 9.5, and in contrib during 2023 for 10.0 and 10.1), but not longer (by the time Drupal 10.2 is out on December 13, 2023, CKEditor 4 will not be supported anymore, or will about to be unsupported).

Alternatively, we could remove these in Drupal 10.0 and 10.1 already and require https://www.drupal.org/project/ckeditor to provide a BC layer for them; that way Drupal core can get rid of them sooner.

Tangentially related to #81: #3231341: Deprecate EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 10.1 for removal in Drupal 11.

I don't think we should remove these in 10.2 - even if ckeditor4 is unsupported, it's not impossible that someone has it installed and updates to 10.2. Should either be 10.0 or 11.0, but seems like less work to remove in 11.0 - it'll just be dead code for a bit longer but saves re-implementing for ckeditor4.

That works too.

Repurposed #2567801 — see #2567801-17: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11.

What do you think about recommendations 1 & 2?

Agree with @catch, we should keep this in 10.x and remove in 11, same as any other deprecated code. Whether we like it or not, some users will be living with CKEditor 4 until the end of life of Drupal 10.

Also agree with committing the fix + test to 9.x and the fix only to 10.x, as we can't write a test without an additional test harness, and even then it would be to support an unused feature. The deprecation can then be committed to 10.1.x or later.

Yes agreed with #1 and #2 from #81 about skipping the test coverage in 10.x

Agreed with #83 on.

Reuploading #68 for 9.5.x and the fix only from #79 for 10.x.

@group legacy to the rescue?

Thank you @Spokje - as we are only adding @group legacy to 9.5.x and the test is still present without it in 10.x I think this is OK and the simplest solution to getting this done.

Slightly tweaking the test added on 9.5.x so it is easier to keep things in sync between 10.x and 9.5.x

Here's an interdiff...

Committed and pushed 2aa0817d69 to 10.1.x and 79e1e2a8da to 10.0.x. Thanks!
Committed 52cc95b and pushed to 9.5.x. Thanks!

Discussed with @longwave. We agreed to backport this to 9.4.x

Committed 7928ea7 and pushed to 9.4.x. Thanks!