Problem/Motivation
CKEditor 4 always disallows on*
attributes within CKEditor 4. We should implement similar measures in CKEditor 5 to prevent self XSS.
Proposed resolution
Make Drupal\ckeditor5\HTMLRestrictions
disallow on*
and style
attributes.
Remaining tasks
Postponed on #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss)
TestsValidation constraint- Review
User interface changes
A validation error will show up in the CKE5 admin UI if you attempt to configure Source Editing to explicitly allow one of these insecure attributes.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#24 | 3228691-24-93x.patch | 11.38 KB | bnjmnm |
#22 | 3228691-22.patch | 11.51 KB | Wim Leers |
#10 | 3228691-10-PoC-do-not-test.patch | 3.2 KB | Wim Leers |
Issue fork drupal-3228691
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Wim LeersI think there's actually two parts:
sourceEditing
to allowon*
,style
or any of the tags in\Drupal\editor\EditorXssFilter\Standard
. This will require just a validation constraint, nothing needs to change inHTMLRestrictionsUtilities
AFAICT.to disallow
style
andon*
and …Or … what is more likely … I am missing something. Because what I'm saying is very different from what's the proposed resolution in the IS :P
Comment #3
Wim LeersComment #4
lauriiiFor feature parity with CKEditor 4, we don't have to do the second step. See
Drupal\ckeditor\Plugin\CKEditorPlugin\Internal::generateACFSettings
which confirms that CKEditor 4 is configured to disallow certain attributes only when HTML restrictor exists. CKEditor 4 full HTML text formats allow using bothstyle
andon*
attributes. For that reason full HTML text formats are documented as unsafe in various different places including https://www.drupal.org/docs/core-modules-and-themes/core-modules/filter-....Comment #5
Wim LeersAh, good point!
But those special tags would still get stripped by the text editor XSS filtering…Did a deep dive, and confirmed that @lauriii's comment in #4 is accurate! See\editor_filter_xss()
, which contains this:Perfect!
Comment #6
Wim LeersComment #7
lauriiiComment #8
Wim LeersThis should wait for #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object to finish; that will make this simpler.
Comment #10
Wim Leers#3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object is in!
Re-reading this, my analysis in #2 is kinda wrong. The second point is indeed not necessary like @lauriii pointed out in #4. Because CKEditor 5 does not able to restrict things, because it only is able to allow extra things! I see I already realized this in #5 :P
So what we need is something like this.
Related: #3260857: Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values.
Comment #12
Wim LeersTests added in https://git.drupalcode.org/project/drupal/-/merge_requests/1933/diffs?co...
Logic also implemented. Tests should pass. Ready for review! 😊
Comment #13
Wim LeersComment #14
Wim LeersHaving just reviewed #3261600: Update to CKEditor5 v32.0.0, I was reminded of #3245950: [upstream] <script> tag support in GHS.
Which makes me wonder if we should make this behave differently than what the issue originally described:
FilterFormat::getHtmlRestrictions() === FALSE
), don't do this validation, but instead disallow configuring anything at all — everything is already allowed anyway, so no need to configure explicit additional tags/attributeson formats with HTML restrictions, do perform this validation … but also disallowWe should allow anything the user wants, unless it is going to be stripped by the filters, which brings me to my next point:<script>
and<style>
. But then where does it stop — perhaps also<iframe>
and<embed>
should be forbidden?style
andon*
), use the restrictions that the text format itself provides!\Drupal\filter\Plugin\Filter\FilterHtml::getHTMLRestrictions()
contains:That is declaring "disallow the
style
andon*
attributes" already! We should probably just interpret that?!Automatically interpreting the text format's HTML restrictions would currently result in
InvalidArgumentException: "*" is not a valid HTML tag name. in Drupal\ckeditor5\HTMLRestrictions::validateAllowedRestrictionsPhase1() (line 113 of core/modules/ckeditor5/src/HTMLRestrictions.php).
though, so I'd like confirmation first that we indeed want to do this. Because it requires removing this:and hence we'd need to do #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) first.
Comment #15
lauriiiIt seems like the ideal solution would be to interpret the wildcard tags. Should we mark this as postponed on #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) then?
Comment #16
Wim Leers+1. I'll start that.
Comment #17
catchComment #18
Wim Leers#3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) landed! Time to update this. Let's first rebase.
Comment #19
Wim Leers#14 has now been implemented! 🤓👍 Ready for review again!
Comment #20
Wim LeersAnd that was the very final piece, a
@todo
referring to #3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal, also is done — because #3260869 landed a while ago :)Comment #21
nod_Added a comment for a small detail, other than that it makes sense, test look good, and works in manual testing too.
Comment #22
Wim LeersCommit d75fb448 as patch, should apply to all 3 branches.
Comment #23
Wim Leers(The
9.3.x
failures are expected: they happen because #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) has not yet been cherry-picked to9.3.x
.)Comment #24
bnjmnmComment #28
bnjmnmCommitted to 10.0.x and cherry-picked to 9.4.x and 9.3.x (9.3 backport since CKEditor5 is experimental and making a module more secure oughta go anywhere it can).