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 &lt;$1&gt; 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

Command icon 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

star-szr created an issue. See original summary.

star-szr’s picture

Issue summary: View changes
star-szr’s picture

I 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:

  1. Keep XSS filtering and document this as a known limitation (we could also switch filtering to use Standard::filterXss())
  2. Remove XSS filtering to allow for more use cases and acknowledge that there are many ways that security can go wrong when it comes to configuring text filters and formats beyond the scope of this module.

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.

jstoller’s picture

Title: Rework XSS filtering implementation for more use cases » XSS filtering breaks replacements with regex token in tag name
Category: Task » Bug report

This 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.

star-szr’s picture

Assigned: star-szr » Unassigned
Issue summary: View changes

Thanks 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!

star-szr’s picture

Version: 1.0.x-dev » 1.1.x-dev
Assigned: Unassigned » star-szr

Working on this.

star-szr’s picture

Title: XSS filtering breaks replacements with regex token in tag name » XSS filtering breaks replacements with tokens that look like HTML tags
star-szr’s picture

Title: XSS filtering breaks replacements with tokens that look like HTML tags » XSS filtering breaks replacements with regex token in tag name

  • star-szr committed 961b1acc on 1.1.x
    fix: #3476210 Remove XSS filtering of replacement strings
    
    Editing...
star-szr’s picture

Assigned: star-szr » Unassigned
Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

star-szr’s picture

Noting that this is part of the 1.1.1 release. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.