Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
I'm unable to limit the values that can be entered for an attribute, e.g. class with CKEditor 5
Steps to reproduce
- Install Drupal
- Enable CKEditor5
- Create a new filter format and enable CKEditor5 for said format
- Turn on the limit HTML filter
- Attempt to edit the allowed HTML and note it says you need to edit it in the 'source editing' section
- In the source editing section, allow specifying a class on an element, e.g.
<a class="button">
- Blur the source editing field and note it updates the allowed HTML field
- At this point, everything is looking like it did in CKEditor 4 (except for the missing styles combo, but that's blocked on upstream)
- Now go and create some content with
<a class="button">
in it and note CKEditor strips out the class attribute. - Go back and edit the filter format and change it from
<a class="button">
to<a class>
instead. - Save and edit your content and note it now allows
<a class="button>
but it also allows<a class="use-ajax">
too
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#31 | 3259493-31.patch | 11.65 KB | Wim Leers |
| |||
#31 | interdiff.txt | 2.38 KB | Wim Leers |
#24 | 3259493-24.patch | 11.18 KB | Wim Leers |
| |||
#24 | interdiff.txt | 1.08 KB | Wim Leers |
#23 | 3259493-23.patch | 11.19 KB | Wim Leers |
Comments
Comment #2
Wim LeersRelated: 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 allowed to configure, which we brought to Drupal core in ("ACF" for short)#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 #3238333: Roadmap to CKEditor 5 stable in Drupal 9 the parent issue, so I suspect you would.
. Is that intentional? Would you consider this a bug, or in other words, a stable-blocking regression that prevents us from moving to CKEditor 5? You did makeComment #3
larowlanI think there are security implications so yes I think it's major
Some classes could lead to defacement
Comment #4
Wim LeersAlright, marking
and tagging .P.S.: I personally agree, but was especially interested in your perspective as a core committer 😊
Comment #5
Wim LeersPer 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.
Comment #6
Wim LeersI get a PHP assertion error if follow your steps to reproduce:
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: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.
Comment #7
larowlanYeah I had asserts off
Comment #8
Wim LeersAlright. 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, includingclass="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:<a>
(links are pretty special)<blockquote>
nor<p>
(block quotes are not very special)<h6>
(headings are "medium special")<cite>
which is not provided by any CKE5 plugin, but allowed thanks to GHS!<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.
Comment #9
Wim LeersDigging 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'sjson_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.
Comment #10
Wim LeersNote 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.Comment #11
Wim LeersComment #12
Wim LeersWe 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.
Comment #14
Wim Leers#3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object is in!
Rerolling #11.
Comment #15
lauriiiShould we add integration test for this too to confirm that the configuration is working?
Comment #16
Wim LeersIMHO 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 :)
Comment #18
lauriiiThe 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.
Comment #19
Wim LeersFair enough — better to remove all doubt.
Comment #20
Wim LeersThis will get validation in #3260857: Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values.
Comment #21
Wim LeersComment #22
Wim LeersHere's the test coverage that @lauriii requested. It will fail. Because the unit test coverage was not complete enough.
Comment #23
Wim LeersHere'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.
Comment #24
Wim LeersFix.
Comment #27
lauriiiTest coverage looks good ✨
Glad that we discovered #22 before committing this 😅
Comment #28
bnjmnmJust some nits!
Add a doc describing what is happening here.
This bit should get a doc
I'm guessing this is a copypaste artifact since this isn't testing image stuff.
This works fine with plain old Stark as the default theme.
I think this is another copypaste artifact
GOOD TEST
Comment #29
Wim LeersComment #30
Wim LeersPer #3260853-13: [GHS] Partial wildcard attributes (<foo data-*>, <foo *-bar-*>, <foo *-bar>) and attribute values (<h2 id="jump-*">) not yet supported.
Comment #31
Wim LeersAddressed all points in #28.
Comment #32
lauriii🛳 🚀 💥
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.
Comment #33
Wim LeersThis now also blocks #3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal.
Comment #37
bnjmnmCommitted 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!