Problem/Motivation

The current patch in #3231364: Add CKEditor 5 module to Drupal core adds ~390 new dependencies on top of the current ~300 core dependencies. The growing number of third party JavaScript dependencies make it unreasonable to evaluate and monitor each dependency individually. While most of the dependencies are used only in the development environment, many of them are used one way or another to build the application from source, meaning that they could impact code that is run in production environments.

Proposed resolution

Allow reviewing built assets by providing a build tool that doesn't minify the assets to reduce the risk of security issues being added. This allows reviewing changes to the minified build files with the following process:

  1. Create new branch for the development build
  2. Build files using development build and commit changes
  3. Go back to the development branch
  4. Apply patch
  5. Rebuild files using development build
  6. Compare against the branch create earlier

Remaining tasks

TBD

User interface changes

N/A

API changes

TBD

Data model changes

TBD

Release notes snippet

TBD

Issue fork drupal-3233491

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

lauriii created an issue. See original summary.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +challenging
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Title: Create plan for handling extended number of JavaScript dependencies » [policy, no patch] Create plan for handling extended number of JavaScript dependencies
Priority: Normal » Critical
Issue tags: +JavaScript
justafish’s picture

justafish
she/her
Primary language English
Location London, UK
commented

For keeping the dependency chain up-to-date we should consider requiring node.js as a dependency for building Drupal and manage it in the same way we manage composer dependencies i.e. when there's a security update for a composer dependency it's up to the site owner to update and re-build. Additionally, this would allow us to stop manually compiling and checking in our core JavaScript files. For a bit of history/context - this is something we discussed for the Admin UI initiative and Dries was at the time on-board with having node as a build-only dependency.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: [policy, no patch] Create plan for handling extended number of JavaScript dependencies » [policy, no patch] Create plan for handling extended number of unvetted JavaScript dependencies
droplet’s picture

droplet commented

If we can't figure out the final plan before CKEDITOR 5 commits, I think we should put them into `drupalCore/sub-dir` and build it from that instead.

As the same as the other packages (postcss-*) for a single new theme. We should not mess the Core's package.json

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Reviewing built assets to reduce the risk of security issues being added. This would require a review step with un-minified code to diff against. It would be similar to the way both ES6 and ES5 JS files are reviewable before commit.

Does this mean that we would review all 3rd party assets or only assets owned by us? This is specifically talking about ES6 and ES5 but after CKEditor 5, our build tooling goes beyond that. For this to be effective, wouldn't we have to review all of our assets, including 3rd party assets?

I'm wondering if reviewing assets is sufficient because there's always the option that a malicious library is targeting the developer itself. Here's an example of a real life use case where a popular npm package was compromised https://eslint.org/blog/2018/07/postmortem-for-malicious-package-publishes, and the attack was specifically targeting the developer.

For keeping the dependency chain up-to-date we should consider requiring node.js as a dependency for building Drupal and manage it in the same way we manage composer dependencies i.e. when there's a security update for a composer dependency it's up to the site owner to update and re-build.

We ship updated composer.json and composer.lock files for security vulnerabilities in PHP dependencies, like we do for JavaScript dependencies. See Drupal 9.2.2 for example. I talked to @effulgentsia because I wanted to understand why we are shipping security releases for security updates in our dependencies, when sites could most of the time simply run composer update. He pointed out that we need to ship Drupal release so that update manager is able to inform users of a security release. It is also needed to trigger an update of the tarball. On top of that he mentioned some specific edge cases with composer that aren't relevant to this because Drupal cannot be installed using yarn (not at least yet 😅). This means that even if we moved some of the responsibility to our users, we would still be ultimately responsible for managing the updates.

Additionally, this would allow us to stop manually compiling and checking in our core JavaScript files.

We could potentially remove the build process in Drupal 10 because we might be able to remove Babel from the stack, given that all of the browsers supported by Drupal 10 support ES6 syntax #3238497: What to do with assets build step?.

lauriii credited effulgentsia.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Crediting @effulgentsia for #8.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Discussed this issue with @xjm since she had raised this issue as a stable blocker for CKEditor 5. The CKEditor 5 upstream packages are packaged by CKEditor. The responsibility of ensuring the safety of the build tools used for generating those files is on upstream.

What's in the scope here is to ensure that we can ensure safety of the build tools used for building Drupals custom plugins. We agreed that the MVP for CKEditor 5 could be to provide a development webpack mode that doesn't minify the build files, and to document how to review changes to the build files using that.

hooroomoo opened merge request !2024

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

hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Status: Active » Needs review

If user runs yarn build:ckeditor5 --mode=development the files will not be minified.

I added "build:ckeditor5 --dev": "yarn build:ckeditor5 --mode=development" in core/package.json to have a shortcut command but I am not sure why it's not working. I ran yarn install and yarn run vendor-update but still would not work.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: [policy, no patch] Create plan for handling extended number of unvetted JavaScript dependencies » Create plan for reviewing changes in 3rd party JavaScript dependencies
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks great! Thank you @hooroomoo!

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs release manager review

@lauriii asked for my feedback on this issue, so tagging for RM review to make sure we do that. :)

xjm’s picture

xjm
she/her
Primary language English
commented

For keeping the dependency chain up-to-date we should consider requiring node.js as a dependency for building Drupal and manage it in the same way we manage composer dependencies i.e. when there's a security update for a composer dependency it's up to the site owner to update and re-build.

Actually, this is 100% not the policy for Composer dependencies. If there is an upstream release for a Composer dependency that is exploitable through Drupal, we issue a security release increasing the constraint. If it is not exploitable through Drupal, we increase the lockfile versions in a non-security patch release. The same should be applied to the yarn dependencies that build our vendor and production assets.

I haven't had a chance to read through everything yet, but just wanted to get that correction in there in case it affects the proposed resolution.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs documentation updates
xjm’s picture

xjm
she/her
Primary language English
commented

Can folks who worked on the issue either mark threads resolved, or put in a thumbs-up emoji if they think the thread is resolved?

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs release manager review +Needs followup

I tested this manually with:

  1. git checkout -b old f093c32e5cd^

    (The commit hash right before the CKEditor 5 update.)
     

  2. Applying and committing the change from this issue.
  3. cd core; rm -rf node_modules; yarn install

  4. yarn build:ckeditor5-dev

  5. git commit -am "Uniminified built assets."

  6. git checkout -b new f093c32e5cd

    (The CKEditor 5 update itself.)
     

  7. Applying and committing the change from this issue.
  8. cd core; rm -rf node_modules; yarn install

  9. yarn build:ckeditor5-dev

  10. git commit -am "Uniminified built assets."

  11. git diff old -- core/modules/ckeditor5/js

Seems to work really well!

We'll need to work out similar solutions for other areas (e.g. built vendor assets where we're not just copying around a minified version, Project Browser, etc.) but this seems perfectly sufficient to the needs of CKEditor 5 itself and making sure our dev dependency tree isn't inserting anything malicious into our built assets. Tagging "needs followup" to discuss other and future cases.

Before this goes in, we need an update to https://www.drupal.org/about/core/policies/core-change-policies/frontend... to explain how to use these new commands (like the instructions in the IS, but with sample CLI commands like I provided in my manual testing steps.

Thanks everyone!

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Create plan for reviewing changes in 3rd party JavaScript dependencies » Create process for reviewing changes in 3rd party JavaScript dependencies
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed b825f91 and pushed to 10.0.x. Thanks!
Committed 91ebf8e and pushed to 9.4.x. Thanks!

  • alexpott committed b825f91 on 10.0.x 
    Issue #3233491 by hooroomoo, xjm, lauriii, nod_, justafish, droplet,...

  • alexpott committed 91ebf8e on 9.4.x 
    Issue #3233491 by hooroomoo, xjm, lauriii, nod_, justafish, droplet,...
hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Issue tags: -Needs documentation updates

Docs are updated, I am removing the docs needed tag: https://www.drupal.org/about/core/policies/core-change-policies/frontend...

Added two things:
1. Reviewing changes to CKEditor 5 build files (the instructions how to use the command)

2. Un-minify CKEditor 5 JavaScript files with development mode (the actual commands listed at the bottom of the handbook)

Status: Fixed » Closed (fixed)

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