Problem/Motivation
As of this writing we run the "replacement text" through Drupal's Xss::filterAdmin().
The problem is that this prevents legitimate use cases such as replacements that look like <$1> or <$1 $2>. The first example would end up as <$1> and end up breaking pasted content in usual ways.
Note that the XSS filtering was removed in 8ffe587 but reverted afterward. The removal was never in a release, only in the dev version.
Steps to reproduce
1. Install the CKEditor 5 Paste Filter module
2. Add or edit a text format (/admin/config/content/formats)
3. Set the Text editor of the text format to CKEditor 5
4. Under CKEditor 5 plugin settings select the Paste filter vertical tab
5. Enable the plugin by checking the Filter pasted content checkbox
6. (optional) Customize the filters if needed. If you are customizing the filters, please provide details below.
7. Save the text format: Scroll to the bottom and click Save configuration
8. Add a new node using the configured text format (/node/add)
9. Paste the rich content into the editor
Search expression: <(strong|em) class="remove">
Replacement: <$1>
Proposed resolution
Remove XSS filtering. Editing text formats and filters is already an administrative permission.
Remaining tasks
- Remove filtering
- Add some automated tests to cover these use cases
User interface changes
None
API changes
TBD, should be none
Data model changes
None
Issue fork ckeditor5_paste_filter-3476210
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
star-szrComment #3
star-szrI found Drupal\editor\EditorXssFilter\Standard::filterXss() and was hoping that would solve this use case, but unfortunately it does not.
I'm reluctant to roll our own XSS filtering for this use case, in part because it still wouldn't be bulletproof. For example:
Search expression:
<(em)>.*(bed).*<\/em>Replacement:
<$1$2>Content:
<em>go to bed</em>Result:
<embed>I think we have two main options:
Standard::filterXss())I need to examine the bigger picture more to see if there are other protections/mitigations that might allow us to justify removing XSS filtering the replacement text.
Comment #4
jstollerThis was driving me crazy for hours! I'm changing this to a bug report, since it breaks a core feature of the module, and updating the title to be more explicit. If a true fix is far out, then I'd suggest at least documenting the issue, to save others some frustration.
Given the potential uses for this module, my inclination would be to get rid of the XSS filtering. You can implement an "Edit CKEditor paste filters" permission, to allow for some security without crippling the module's functionality.
For now I'm working around the problem by capturing the opening and closing tags of any HTML element in the search, so they're included in the replacement tokens. So, a rule like this:
search: '<(\/?)h1>'
replace: '<$1h2>'
...changes to this:
search: '(<|<\/)h1(>)'
replace: '$1h2$2'
This masks the HTML from Xss::filterAdmin(). It won't fix every use case, but it's meeting my needs for now.
Comment #5
star-szrThanks for the bump and additional information @jstoller (and sorry for the trouble)!
Since editing text formats and filters is already behind an administrative permission, at this point I'm happy to open this up (remove the filtering). I might get to this soon, but unassigning in case someone wants to at least get the ball rolling. Thanks!
Comment #6
star-szrWorking on this.
Comment #7
star-szrComment #8
star-szrComment #11
star-szrComment #13
star-szrNoting that this is part of the 1.1.1 release. Thanks!