Problem/Motivation

https://github.com/ckeditor/ckeditor5/releases/tag/v46.0.3 has been released, fixing an XSS vulnerability.

From reading the release notes, it looks as if Drupal core is not directly vulnerable - we don't enable the HTML Embed plugin in core, and we also don't use ViewRawElement as far as I can tell. However, it would be good to get a second (or third) set of eyes to confirm this is really the case.

Drupal 11.x/11.2/10.6/10.5 are all on 45.2.1, so we need to update to 45.2.2 once this is available.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3544581-nr-bot_2pho7q41.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3544581

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:

About issue forks

Comments

catch created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Update to ckeditor 45.2.2 (once available) » Update to CKEditor5 v45.2.2

https://github.com/ckeditor/ckeditor5/releases/tag/v45.2.2 is out.

catch’s picture

catch
he/him
Primary language English
commented

The ckeditor team has confirmed that the vulnerability was introduced after 44.0.0 was tagged, so we don't need to do anything for 11.1 and 10.4.

godotislate made their first commit to this issue’s fork.

godotislate opened merge request !13140

godotislate’s picture

godotislate
he/him
commented

Put up MR 13140.

Looks like ckeditor/essentials didn't get updated, which I'm guessing is because of this in core/scripts/js/assets/ckeditor5Files.js:

const ckeditor5PluginMapping = {
  'block-quote': 'blockquote',
  essentials: 'internal',
  'basic-styles': 'basic',
};

...

let library = pluginName.replace(/-./g, (match) => match[1].toUpperCase());
      // Special case for Drupal implementation.
      if (ckeditor5PluginMapping.hasOwnProperty(pluginName)) {
        library = ckeditor5PluginMapping[pluginName];
      }
      if (library === 'ckeditor5') {
        folder = 'ckeditor5/ckeditor5-dll';
      } else {
        library = `ckeditor5.${library}`;
      }
      fileList.push({
        pack,
        library,
        folder,
        files: buildFiles.map((absolutePath) => ({
          from: absolutePath.replace(`${ckeditor5package}/`, ''),
          to: absolutePath.replace(`${ckeditor5package}/build/`, ''),
        })),
      });

Looking at why essentials has special handling.

godotislate’s picture

godotislate
he/him
commented

OK, looks like in https://git.drupalcode.org/project/drupal/-/commit/aa2f8c3 the library was renamed from ckeditor/internal to ckeditor/essentials, and the ckeditor5Files.js script was not updated to match. Pushed a commit to the MR to address.

Note that the essentials.js file itself has not changed upstream.

godotislate’s picture

godotislate
he/him
commented
Status: Active » Needs review

Tests are green.

godotislate opened merge request !13143

godotislate opened merge request !13144

godotislate opened merge request !13145

godotislate’s picture

godotislate
he/him
commented

Opened MRs for 10.5.x, 10.6.x, and 11.2.x. Investigating 11.2.x MR test failure that isn't failing on 11.x.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3544581-nr-bot_2pho7q41.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review
Issue tags: +no-needs-review-bot

OK, bumping up ScriptBytes to 171060 fixes AssetAggregationAcrossPagesTest::testFrontAndRecipesPagesAuthenticated test, but I'm not sure why the aggregated script size would increase on the test pages, since CKEditor isn't on any of those pages AFAICT.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

That is odd but I think we should go ahead here and open a new issue just to double check it's unrelated.

Everything else looks straightforwards so moving to RTBC.

godotislate’s picture

godotislate
he/him
commented

OK, just checked and looks like the latest 11.2.x daily failed on the same test: https://git.drupalcode.org/project/drupal/-/jobs/6402181.

nod_ closed merge request !13140

  • nod_ committed 9f46196c on 11.x 
    Issue #3544581 by catch, longwave, godotislate: Update to CKEditor5 v45....

  • nod_ committed 90fc517a on 11.2.x 
    Issue #3544581 by catch, longwave, godotislate: Update to CKEditor5 v45....

nod_ closed merge request !13144

nod_ closed merge request !13143

  • nod_ committed 6a2c9c3e on 10.6.x 
    Issue #3544581 by catch, longwave, godotislate: Update to CKEditor5 v45....

nod_ closed merge request !13145

  • nod_ committed 5cd64465 on 10.5.x 
    Issue #3544581 by catch, longwave, godotislate: Update to CKEditor5 v45....
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 9f46196 and pushed to 11.x. Thanks!
Committed 90fc517 and pushed to 11.2.x. Thanks!
Committed 6a2c9c3 and pushed to 10.6.x. Thanks!
Committed 5cd6446 and pushed to 10.5.x. Thanks!

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

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

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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