Follow-up to #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored
Problem/Motivation
It seems that #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored had some unexpected side effects.
Before that fix, only ckeditor plugins' required attributes were added to the allowed html field.
Now, required AND allowed attributes are added to the field so they can appear twice (once with the exclamation mark, once without).
As we weren't filtering on attributes lately I don't think that's a big security issue but that's:
- certainly a regression
- probably not what the filter is expecting
- ugly ;)
Current behavior:
Based on the text editor configuration, these tags have automatically been added:
<a href hreflang !href target> <img src alt height width data-entity-type data-entity-uuid data-align data-caption !data-entity-type !data-entity-uuid !src !data-align !data-caption class="align-left align-right">
.
Expected behavior:
Based on the text editor configuration, these tags have automatically been added:
<a href hreflang target> <img src alt height width data-entity-type data-entity-uuid data-align data-caption class="align-left align-right">
.
Proposed resolution
No idea.
Remaining tasks
Find out what's wrong. Fix it. Enjoy.
User interface changes
None.
API changes
None.
Data model changes
None.
RC target triage
This bugfix is not disruptive at all as it only affects a few lines of faulty JS already in RC1.
Comment | File | Size | Author |
---|---|---|---|
#50 | core-js-ckeditor-2585173-50.patch | 7.12 KB | Wim Leers |
Comments
Comment #2
Jelle_SIt seems the '!'-notation for attributes is only for the string format notation, not needed when using
CKEDITOR.style
objects. Patch attached ;-)Comment #3
nod_We could use .replace('!', '') here and get rid of the if/else no?
Comment #4
attiks CreditAttribution: attiks at Attiks commentedAFAIK an exclamation mark is allowed in a data-* attribute, so I think the safest is as it is in the patch.
Comment #5
nod_replace can take a regex, so if it's only about a
!
in the first position we can do that too..replace(/^!/, '');
Comment #6
attiks CreditAttribution: attiks at Attiks commentedTrue, either way is fine by me
Comment #7
nod_let's go with that then. less lines can't hurt.
Comment #8
Jelle_SComment #9
nod_Just fixing eslint errors. Works, no duplicate attributes in the list now.
Comment #10
DuaelFrComment #11
Wim LeersThis patch does indeed fix the
problem.But…
Prior to #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored, the act of merely browsing to
/admin/config/content/formats/manage/basic_html
resulted in no tags being added.With this follow-up patch for #2579979, we still see:
The things that are being added specifically are:
target
attribute on<a>
class="align-left align-right"
attribute + attribute values on<img>
Therefore, this is still a net regression.
Comment #12
Wim LeersIt seems the changes in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored are triggering a bug in CKEditor.
filter.filter_html.admin.js
's_calculateAutoAllowedTags()
is most definitely only looking at the required tags. So I added a bit of debug output there (console.log(featureName, featureRule);
) to see what's going on. Turns out CKEditor is returning['href', 'target']
as required attributes for<a>
. Even thoughtarget
is only an allowed attributes. It seems they have some sort of object leaking/object reference bug in their parsing of plugins'allowedContent
/requiredContent
and the mapping of that into their "CKEditor feature rules".I pinged the CKEditor team for their help.
Comment #13
DuaelFrUpdated
core/profiles/standard/config/install/filter.format.basic_html.yml
to be consistent with its enabled editor plugins on install.That makes browsing to the
/admin/config/content/formats/manage/basic_html
not resulting in added tags anymore.Changing the issue status to Needs Review only to trigger the testbot. As told in #12 we need more input from the CKEditor team.
Comment #14
Wim Leers#13: that's wrong. Those things should not be whitelisted.
This explains why you said #2578957: (regression) "Open in new window" checkbox does not work anymore was "magically fixed" by #2579979! You want
target
to be whitelisted. But it's only an allowed thing, not a required thing. If people want to allow that, they need to manually whitelisttarget
. The point is that we only whitelist tags and attributes that are essential ( ) for that editor feature to function.target
is not essential.Comment #15
DuaelFrSo, we might want to remove the "Open in a new window" checkbox from the link Dialog when the attribute is not whitelisted... or totally remove that checkbox and let contrib deal with it.
We cannot expose something to end users by default and assume they know they have to change something in an obscure part of the settings to get it working.
Comment #16
Wim LeersFirst: you're absolutely right of course.
We should only expose it if the text format allows it. Just like
EditorImageDialog
for example only shows the "Align" checkboxes if it's allowed:In the case of
<a target>
, we should do something like… but that probably belongs on #2578957: (regression) "Open in new window" checkbox does not work anymore.
Comment #17
DuaelFrI didn't know that thing existed!
So, why did we decide that data-align was so important it had to be enabled by default (in standard profile at least) but link target is not?
We should fix that checkbox for sure but I think the standard profile is the good place to have that kind of thing enabled by default. That last thing should probably live in a follow-up issue, though. ;) Yay ! Chaining issues !
So, to summarize:
I'm running out of time. Could you open that last follow-up and reopen the one I closed as duplicate, please?
Comment #18
Wim Leers#17: Correct! Done as requested: #2578957-7: (regression) "Open in new window" checkbox does not work anymore (for point 2) + #2590403: Remove "Open in new window" checkbox from EditorLinkDialog — Was: "Consider whitelisting <a>'s target attribute in the Standard install profile".
Comment #19
DuaelFrThanks a lot! I'll post there tonight or tomorrow.
Removing rc target triage tag while we don't have something that's commitable.
Comment #20
Reinmar CreditAttribution: Reinmar commentedThe patch for https://www.drupal.org/node/2579979 does not seem to be correct, because the code tries to extend one feature with another feature, while (if I remember correctly) some previous version of that patch tried to extend one allowedContent rules with another ones. The previous one made more sense.
The second thing is that using CKEDITOR.style to define allowedContent work, because this is a valid argument of CKEDITOR.filter.allow() but it has a special meaning. All properties (styles, attrs) of such style are treated as required, because only elements having all of these properties are considered matching that style. So I would rather recommend using the object notation of allowed content rules rather than CKEDITOR.style instances.
And the third thing is how to extend one allowed content ruleset with another ruleset. I guess you don't want this to happen:
allowedContent 1:
{ element: ‘a’, attributes: { ‘href’ } }
allowedContent 2:
{ element: ‘a’, attributes: { ‘target’ } }
If you now do:
CKEDITOR.tools.extend( aC1, aC2, true )
you will have:{ element: ‘a’, attributes: { ‘target’ } }
Because the whole attributes property of aC1 is overridden.
I guess that you want to merge attributes lists. For that a deep-extend would make more sense, but you can also write a custom extend() function if this will require some special logic.
Few more pieces from what I wrote Wim on Skype:
Comment #21
Jelle_SOk, so if I'm not mistaken, we can't use CKEDITOR.style for our use case. The next 'most easily extendable' would be the object format. But, from what I gather, in the object format, the attributes and styles use string notation:
Correct me if I'm wrong but I don't think we can do something like:
or
Correct?
Comment #22
Reinmar CreditAttribution: Reinmar commentedActually you can specify attributes in one string, an array or an object (values must be
true
): check these tests https://github.com/ckeditor/ckeditor-dev/blob/master/tests/core/filter/a... and this fragment of docs http://docs.ckeditor.com/#!/guide/dev_allowed_content_rules-section-spec...So if you'll use a purely object format, then doing a deep copy from one object to another would work as extending one style with another.
Comment #23
Jelle_SOk here's a new patch.
According to the docs I can't use the object format for requiredContent, it's either string format or
CKEDITOR.style
format. But it seemsCKEDITOR.style
is valid for requiredContent for our use case. @Reinmar can you confirm?The allowedContent has been converted to the object format.
Not sure if this is because of the patch (but I don't think so) or default behaviour from before the patch:
- Edit the restricted HTML format and select CKEDITOR as editor.
- When I add the image align plugin to the restricted HTML format, nothing is added to the allowed HTML filter.
- Save and configure it again, and under the allowed HTML filter I get the message: "Based on the text editor configuration, these tags have automatically been added:
<img data-entity-type data-entity-uuid src data-align data-caption>
."This means you can save the text format with plugins that will not work
I think this would be solved if the plugin.js files are loaded on the config page, regardless of whether or not the plugin is enabled (I have not tested this theory yet, but fact is that they are not loaded when they are not enabled, and are loaded when they aren't).
Just checking if this behaviour is a bug introduced by this patch or the patch in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored.
Comment #24
Wim LeersPinged Reinmar to confirm that. That sounds very strange/confusing :(
Before which patch? This one, or #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored?
That is already the case. That's how this is able to work at all. See
\Drupal\ckeditor\Plugin\Editor\CKEditor::settingsForm()
:So I'm not sure I completely comprehend what you're saying here.
Because even in case you go to
/admin/config/content/formats/manage/restricted_html
(which definitely has no image-related filters:<img>
is not even allowed) and then enable CKEditor for the very first time, then you see that/core/modules/ckeditor/js/plugins/drupallink/plugin.js
and/core/modules/ckeditor/js/plugins/drupalimage/plugin.js
.EDIT: oh, I see — despite all that, the hidden CKEditor instance still ends up respecting each Drupal CKE plugin's
isEnabled()
method. I attached an interdiff with the fix for that.Comment #25
Jelle_SI agree. BTW, here's the doc link where I found it:
http://docs.ckeditor.com/#!/api/CKEDITOR.filter.contentRule
Either one, if the behaviour has changed at all.
Yes, but the drupalimagecaption is not loaded I think, so I assume with the interdiff it is?
Should the interdiff be part of this patch or a separate issue?
Comment #26
Reinmar CreditAttribution: Reinmar commentedYes, this is true. There's a big difference between
allowedContent
(filter.allow()
) andrequiredContent
(filter.check()
), so both these parameters are handled by a totally different code. As I look on the code now, it might be doable to support object format in requiredContent, but that's mostly thanks to refactoring that we've done after first implementation offilter.check()
. Previously it would require a lot of code, so we didn't want to bloat the editor with it as it wasn't necessary so far.BTW. I thought about the required content in case of Drupal. As I mentioned in #20 it's used by the editor to figure out whether current filter's settings allow for enabling a given feature. For instance, an image feature may allow
img[src,alt,width,height](someclasses,here)
, but in fact to work it requires just thisimg[src,alt]
. So the two things are:img[*](*)
. Hence, the requiredContent will be satisfied no matter what some feature will add to it (I mean extending one features reqContent with another features reqContent).Comment #27
Reinmar CreditAttribution: Reinmar commentedI've been looking at https://www.drupal.org/files/issues/core-js-ckeditor-2585173-23.patch and I don't understand one thing – why do you need to split classes, styles and attributes like this:
If I understand that piece of code correctly widget definition is a definition of drupalimage, which is your plugin. Can't you define allowed content in an object format from the beginning and avoid that work on strings?
Comment #28
xjmThis might even be critical, but @alexpott and I agreed that this should be an rc target at least, assuming the eventual fix does not introduce unexpected disruption.
Comment #29
Wim LeersI worked on determining the correct priority for this issue. Clarified the title and moved to the right component. Next: move this patch forward.
If you follow these steps:
/admin/config/content/formats/manage/basic_html
allowed_html
setting is strongly polluted: it contains lots of exclamation mark-prefixed attributes. :( :( :( In particular, the<img>
tag gets all thedata-
attributes added again, but now prefixed with exclamation marks. The question is: does this break things?/node/add/article
. Note that everything still works perfectly. Look atdrupalSettings.editor.formats.basic_html.editorSettings.allowedContent.img.attributes
. Be utterly surprised that this actually does NOT contain any of the exclamation mark-prefixed attributes!FilterHtml::getHTMLRestrictions()
, notice that because this usesDOMDocument
to parse theallowed_html
setting, it turns out that it automatically ignores/skips the invalid (exclamation mark-prefixed) HTML attributes! Three hurrays forDOMDocument
!Conclusion: through sheer luck, this does not negatively impact the end user; it only negatively impacts:
filter.format.*.yml
filesIf it were just point 1, this would be normal. With point 2, I think this is major. If this also affected end users (i.e. people using CKEditor/text formats), then I think this would be critical.
Comment #30
Wim LeersFirst, a straight reroll of #23, with conflicts resolved. (This conflicted with #2589779: Uncaught TypeError: Cannot set property 'float' of undefined when switching text formats.)
Next: working on making that better, taking into account all the feedback from CKEditor lead developer @Reinmar.
Comment #31
Wim Leers#26:
This used to be true. But this changed shortly before we entered the RC phase. See #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default. Since then, no attributes are whitelisted by default. You need to explicitly whitelist all attributes you want to use on tags.
#27: I also don't understand that.
#28: Thanks!
Stay tuned.
Comment #32
Wim Leers#27: oh, it's because
image2
uses the string format, anddrupalimage
wants to extendimage2
.Comment #33
Wim LeersFixing one small oversight in #30's rebaing that caused a JS error.
Comment #34
Wim LeersSo the actual root cause of the problem here is that CKEditor plugins have poor extensibility. Drupal has a very strong need for multiple things extending the same original thing. For example, a form can be altered by multiple Drupal modules. And applied to CKEditor, that means that multiple Drupal modules need to be able to extend the functionality of the same CKEditor plugin, which then also means adding more required and/or allowed attributes.
Let's look at a concrete example. Let's say we have an "Crazy Image Effect" module. This module allows you to apply a crazy image effect to any image. To achieve this, it has a filter that looks at
<img>
tags with adata-crazy-image-effect
attribute. Hence this attribute needs to be allowed for this to work. Hence we need thedata-crazy-image-effect
attribute to be inrequiredContent
. The end result is then that the module can alter theEditorImageDialog
form, that form then is able to communicate the user-selected value fordata-crazy-image-effect
to CKEditor, and CKEditor doesn't strip it because the attribute is whitelisted in the text format'sallowedContent
, because it was marked as required for that plugin.The changes made in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored were an effort to make this kind of extensibility actually feasible without having to parse values in a CKEditor plugin's
requiredContent
andallowedContent
. If CKEditor would improve the extensibility out of the box, we wouldn't have to go through all this :) AFAICT, CKEditor currently only cares about one kind of extensibility: extend by subclassing. But subclassing only allows one thing to extend the base thing, and Drupal needs the ability for multiple things to extend it.Comment #35
Wim Leersdrupalimage
had to overrideimage2
:widgetDefinition.requiredContent = 'img[alt,src,width,height,data-entity-type,data-entity-uuid]';
drupalimpagecaption
had to overridedrupalimage
:widgetDefinition.requiredContent = 'img[alt,src,width,height,data-entity-type,data-entity-uuid,data-align,data-caption]';
This is exactly the "designed for single extensibility" problem described in more detail in #34.
allowedContent
and aCKEDITOR.style
instance forrequiredContent
, per #26.drupalimage
's needs" concerns, now doing those separately. This makes things much clearer. Also including ample documentation (including some that was lost in #2579979: Allow contrib to alter EditorImageDialog/EditorImageDialog and have custom attributes be stored) and justification, to help avoid developers having to go through this entire issue, because this is definitely far more confusing than it should be. And is hence a thing to improve in a future CKEditor release.drupallink
'sallowedContent
to includeclasses: {}
, just likedrupalimage
, so that it is just consistently extensible likedrupalimage
.Comment #36
Wim LeersThis fixes an
eslint
error that #23 introduced.Comment #37
Wim LeersComment #38
Wim Leers#23:
I replied with an explanation and suggested interdiff in #24. But my fix was wrong. Opened #2606496: Automatically allowed automatically attributes may be incomplete if they are for a CKEditor plugin that is enabled depending on a filter to fix it, and indicates possible solutions.
Hence this issue is now fully focused on just the issue title, and #36 fully fixes it. We need manual testing and confirmation from the CKEditor developers.
Comment #39
DuaelFrI did some manual testing on images and links, according to everything that @Wim Leers said since #29.
The patch from #36 answers all the needs exposed here.
I just have a tiny comment that does not affect the fix. (It's RTBC-able for me)
May we have a small comment that explain we have to rebuild the entire object because we do not have any setter? I have to read the CKEDITOR code to understand why this was needed.
-----
I plan to use that patch logic this week on both my D8 Editor File upload module and a client site that need to be able to add the title and target attributes on their links.
Comment #40
Wim LeersGlad to hear that you agree this answers all raised problems/needs!
Of course. Will do so in the next reroll.
Comment #41
Wim LeersDone.
Unassigning, because this is now blocked on the CKEditor team.
Comment #42
DuaelFr#41 is great. Thank you, again :)
That's RTBC for me, let's wait for the CKTeam update.
Comment #43
Reinmar CreditAttribution: Reinmar commentedwidgetDefinition.allowedContent
andwidgetDefinition.requiredContent
twice in drupalimage/plugin.js?Couldn't you set them once and comment which attributes come from image2 and which are Drupal specific?
Of course it won't cause any troubles if you set them twice, but code size grows :)
data-entity-*
attributes really required? Issn't requiring them breaking pasting content from outside the editor?AFAIU, images without those attributes will be filtered out.
data-align
a required attribute? Can't you have an unaligned captioned image?drupalunlink
command need theallowedContent
setting? It does not create anything.Other than that the patch looks great.
Comment #44
Wim LeersIt's possible to merge them, but IMO separating the concerns makes the code much easier to understand. It also means that when people want to extend
drupalimage
, they see an example right there of how to do it correctly.No, that attribute is not required to be present on every image. But the plugin/widget does require those attributes to be allowed; without them, it wouldn't be possible to upload images and track where they are being used. Note that the default text format (
) does have the filter enabled, which means that it is in fact impossible to hotlink images from other domains.So, it's actually kind of true that those attributes are required for images. But really, what this is meant to convey, is that for this plugin to function, it needs those attributes to be allowed. Is this the wrong way of defining that?
Also, there's nothing new here, it's been like this for years,
Related improvement targeted for Drupal 8.1.0: #2510394: [drupalImage] Setting to still allow linking to an image by URL even when uploads are enabled.
And vice versa, you can also have an aligned uncaptioned image. The problem here is that we actually want to express "it is required to have either of these attributes". The
drupalimagecaption
plugin should actually be split up intodrupalimagecaption
anddrupalimagealign
, and both should extenddrupalimage
. But that's something for 8.1, unless you feel that is essential?Hah! We've had this since the very issue where we introduced the
drupallink
plugin: #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms. This went unnoticed all that time, including by @wwalc, who reviewed that issue. Removed, thanks!Yay!
Awaiting your response!
Comment #45
Reinmar CreditAttribution: Reinmar commentedRegarding
data-entity-*
anddata-align
– it's up to you whether you want to make them required or not. Defining them as required will limit what the users can do. But if your UI is consistent with that (e.g. that it requires choosing some alignment if you chose caption), then it's totally OK.Comment #46
Wim LeersOkay, @Reinmar is clearly +1 to the overall changes here.
His concerns are mostly regarding pre-existing things, which are A) at least partially correct, B) out of scope to resolve here, C) are too late to change now anyway given we're approaching release. Let's improve those things in Drupal 8.1.x.
Therefore, per #39, #42, #43, #44 and #45, this is RTBC.
Comment #47
Wim Leers#44 + #45:
Opened separate issue for this, since that is a completely harmless thing (that should nevertheless be tested separately to ensure it doesn't cause regressions), and this issue is a very important regression. Then this issue can go in without the need for a new round of manual testing.
See #2607454: Remove allowedContent for drupalunlink command.
Comment #48
Wim LeersComment #49
alexpottAlso given the amount of change in this area atm a new set of manual testing would probably be good.
Comment #50
Wim Leers+1.
Here's a straight reroll. DuaelFr will be doing manual testing in an hour or so. I already did brief manual testing; still working as in #41 AFAICT.
Comment #51
attiks CreditAttribution: attiks at Attiks commentedI reviewed the patch (not tested) and it looks good, code is even more readable, great job!
Comment #52
DuaelFrI did a lot of manual testing with all Core's plugins and my own one.
All the Core plugins are perfectly working. No more extra buggy attributes added to the list on page load.
I had to update my module to be able to see the issue fixed with a custom plugin so we might need a quick change record.
You did a great job :)
Comment #53
Wim LeersThank you very much for your extensive testing!
I disagree we need a change record. No APIs are changed in this patch. What this patch does, is fixing Drupal's own CKEditor plugins to comply with CKEditor APIs. i.e. we are using CKEditor APIs incorrectly in HEAD, as CKEditor lead developer @Reinmar explained in #26.
The patch therefore merely fixes core, and documents why
allowedContent
andrequiredContent
use different formats:(That link points to #26.)
Therefore the change you made (http://cgit.drupalcode.org/editor_file/commit/?id=8238733e057f6eb1c72397...) is just another change to comply with CKEditor's limitations. That is in no way a Drupal API. You had updated your module's CKEditor plugin to reflect recent changes in core, and in doing so, you inherited core's bug. Any future Drupal modules including a CKEditor plugin will look at core and see everything explained and documented in the correct way and will hence not make the same mistake.
The real problem is that CKEditor doesn't validate the incoming data: if it would be throwing an exception, none of this would ever have happened. I opened a ticket for that: http://dev.ckeditor.com/ticket/13898.
Comment #54
webchickYAY! Thanks, all. Committed/pushed to 8.0.x. Thanks!
Comment #56
Wim LeersYay, this unblocked #2598070: [regression] CKEditor Link button does not show if HTML filtering is enabled!