Problem/Motivation

yarn audit currently gives:

17 vulnerabilities found - Packages audited: 1278
Severity: 12 High | 5 Critical

Proposed resolution

yarn upgrade on all the branches.

Remaining tasks

#3266912: Review version constraints for production yarn dependencies should probably be fixed prior to updating 9.3.x and 9.2.x, to avoid unnecessary minor version updates for the deps.

User interface changes

N/A

API changes

TBD

Data model changes

N/A

Release notes snippet

Drupal core’s JavaScript development dependencies have been updated to the latest allowed minor and patch versions to address a few security issues in those dependencies. This should have minimal impact on contributed or custom code and CI workflows. Core developers should completely remove their node_modules directory and re-run yarn install from within the core/ directory.

CommentFileSizeAuthor
#18 yarn-3278163-15-9.2.x.patch94.43 KBxjm
#18 yarn-3278163-15-9.3.x.patch169.91 KBxjm
#18 yarn-3278163-15-9.4.x.patch91.22 KBxjm
#18 yarn-3278163-15-9.5.x.patch91.22 KBxjm
#16 cke-build-diff.txt7.85 KBlauriii
#15 yarn-3278163-15-10.0.x.patch247.33 KBxjm
#11 yarn-3278163-11-9.2.x.patch85.99 KBxjm
#11 yarn-3278163-11-9.3.x.patch169.33 KBxjm
#11 yarn-3278163-11-9.4.x.patch80.52 KBxjm
#11 yarn-3278163-11-9.5.x.patch80.52 KBxjm
#11 yarn-3278163-11-10.0.x.patch119.69 KBxjm
#8 core-yarn-up-3278163-8.9.2.x.patch40.73 KBnod_
#5 core-yarn-up-3278163-5.10.0.x.patch51.59 KBnod_
#5 core-yarn-up-3278163-5.9.5.x.patch38.45 KBnod_
#5 core-yarn-up-3278163-5.9.4.x.patch38.45 KBnod_
#5 core-yarn-up-3278163-5.9.3.x.patch103.19 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: yarn upgrade for latest security vulnerabilities » [P-1] yarn upgrade for latest security vulnerabilities
Status: Active » Postponed
Related issues: +#3266912: Review version constraints for production yarn dependencies

Postponing for now.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: [P-1] yarn upgrade for latest security vulnerabilities » yarn upgrade for latest security vulnerabilities
Status: Postponed » Active

#3266912: Review version constraints for production yarn dependencies is in.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented

do we really need 9.2 as well?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
FileSize
core-yarn-up-3278163-5.9.3.x.patch103.19 KB
core-yarn-up-3278163-5.9.4.x.patch38.45 KB
core-yarn-up-3278163-5.9.5.x.patch38.45 KB
core-yarn-up-3278163-5.10.0.x.patch51.59 KB
  • 9.3.x
    -- BEFORE
17 vulnerabilities found - Packages audited: 1341
Severity: 12 High | 5 Critical

-- UPDATE
yarn upgrade @babel/core minimist @ckeditor/ckeditor5-dev-utils eslint-plugin-import
info Direct dependencies
├─ @babel/core@7.17.10
├─ @ckeditor/ckeditor5-dev-utils@30.1.3
├─ eslint-plugin-import@2.26.0
├─ glob@7.2.0
└─ minimist@1.2.6

-- AFTER
11 vulnerabilities found - Packages audited: 1249
Severity: 11 High
  • 9.4.x
    -- BEFORE
17 vulnerabilities found - Packages audited: 1237
Severity: 11 High | 6 Critical

-- UPDATE
yarn upgrade @babel/core minimist @ckeditor/ckeditor5-dev-utils eslint-plugin-import
info Direct dependencies
├─ @babel/core@7.17.10
├─ @ckeditor/ckeditor5-dev-utils@30.1.3
├─ eslint-plugin-import@2.26.0
├─ glob@7.2.0
└─ minimist@1.2.6

-- AFTER
11 vulnerabilities found - Packages audited: 1209
Severity: 11 High
  • 9.5.x:
    -- BEFORE
17 vulnerabilities found - Packages audited: 1237
Severity: 11 High | 6 Critical

-- UPDATE
yarn upgrade @babel/core minimist @ckeditor/ckeditor5-dev-utils eslint-plugin-import
info Direct dependencies
├─ @babel/core@7.17.10
├─ @ckeditor/ckeditor5-dev-utils@30.1.3
├─ eslint-plugin-import@2.26.0
├─ glob@7.2.0
└─ minimist@1.2.6

-- AFTER
11 vulnerabilities found - Packages audited: 1209
Severity: 11 High
  • 10.x:
    -- BEFORE
4 vulnerabilities found - Packages audited: 1061
Severity: 4 Critical

-- UPDATE
yarn upgrade @babel/core minimist @ckeditor/ckeditor5-dev-utils eslint-plugin-import
info Direct dependencies
├─ @babel/core@7.17.10
├─ @ckeditor/ckeditor5-dev-utils@30.1.3
├─ eslint-plugin-import@2.26.0
└─ glob@7.2.0

-- AFTER
0 vulnerabilities found - Packages audited: 1068

All that's left in the 9.x branch is the nightwatch update which leaves the 11 high severity issues.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
Status: Active » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

I think we need 9.2.x too since it's supported until June.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
FileSize
core-yarn-up-3278163-8.9.2.x.patch40.73 KB
  • 9.2.x
    -- BEFORE
16 vulnerabilities found - Packages audited: 893
Severity: 11 High | 5 Critical

-- UPDATE
yarn upgrade @babel/core minimist eslint-plugin-import
info Direct dependencies
├─ @babel/core@7.17.10
├─ eslint-plugin-import@2.26.0
└─ minimist@1.2.6

-- AFTER
11 vulnerabilities found - Packages audited: 933
Severity: 11 High
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Is there a reason we are not just doing yarn upgrade; yarn vendor-upgrade? The whole point of #3266912: Review version constraints for production yarn dependencies is to be able to use yarn upgrade safely when there is a security release. Doing a full yarn upgrade gets rid of all the vulnerabilities for me on most of the branches.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Needs work

NW for #9; @lauriii also agreed we can do the full ugprade.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs work » Needs review
FileSize
yarn-3278163-11-10.0.x.patch119.69 KB
yarn-3278163-11-9.5.x.patch80.52 KB
yarn-3278163-11-9.4.x.patch80.52 KB
yarn-3278163-11-9.3.x.patch169.33 KB
yarn-3278163-11-9.2.x.patch85.99 KB

Steps for patches:

  1. cd core; rm -rf node_modules; yarn install
  2. yarn audit
  3. yarn upgrade
  4. yarn audit
  5. yarn vendor-update

10.0.x

Before

4 vulnerabilities found - Packages audited: 1061
Severity: 4 Critical

After

0 vulnerabilities found - Packages audited: 1040

9.5.x

Before

17 vulnerabilities found - Packages audited: 1237
Severity: 11 High | 6 Critical

After

0 vulnerabilities found - Packages audited: 1139

9.4.x

Before

17 vulnerabilities found - Packages audited: 1237
Severity: 11 High | 6 Critical

After

0 vulnerabilities found - Packages audited: 1139

9.3.x

Before

17 vulnerabilities found - Packages audited: 1341
Severity: 12 High | 5 Critical

After

0 vulnerabilities found - Packages audited: 1208

9.2.x

Before

16 vulnerabilities found - Packages audited: 893
Severity: 11 High | 5 Critical

After

0 vulnerabilities found - Packages audited: 893
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

It looks like for 9.4.x we could still update shepherd.js, sortablejs, and tabbable which have new minor updates. Should we do this for beta or next alpha, or should we try to update these for the alpha1? This is unrelated to this issue but noticed as I was reviewing this.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs followup

@lauriii Good find. I would file separate issues for those. If they happen to land within the next 24h or so we can include them in alpha1; otherwise, we can update them between alpha1 and beta1.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work
Issue tags: -Needs followup

Filed a follow-up which we could work on after this has been committed: #3278786: Update production JavaScript dependencies to latest minors.

The steps in #11 are missing a step to run yarn build to ensure that any changes in the build tooling are reflected in the build files. At least some of the branches have changes as a result of these changes.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 9.2.x-dev » 10.0.x-dev
Status: Needs work » Needs review
FileSize
yarn-3278163-15-10.0.x.patch247.33 KB

Not sure what happened to my comment... new steps:

  1. cd core
  2. rm -rf node_modules; yarn install; yarn upgrade; yarn vendor-update; yarn build; yarn lint:css; yarn lint:core-js-passing
  3. yarn spellcheck:make-drupal-dict
  4. yarn audit

Here's the 10.0.x patch; others forthcoming.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
cke-build-diff.txt7.85 KB

Went through the changes to the build files and all looks fine. Didn't research what was triggering the changes. Posted diff in case someone else is curious and has time to do that.

The last submitted patch, 11: yarn-3278163-11-9.4.x.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
FileSize
yarn-3278163-15-9.5.x.patch91.22 KB
yarn-3278163-15-9.4.x.patch91.22 KB
yarn-3278163-15-9.3.x.patch169.91 KB
yarn-3278163-15-9.2.x.patch94.43 KB
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/core/misc/cspell/dictionary.txt
@@ -25,6 +25,8 @@ alternatif
+analyse
+analysed

@@ -233,6 +232,7 @@ codesniffer
+colour

@@ -1099,6 +1093,7 @@ readmore
+realise

Looks like we're going to have to do some more de-Britifying here.... will be a followup.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

I went through all of the patches and verified that patches were created correctly with the following steps:

git apply --index <patch>
yarn install
yarn upgrade
yarn build
yarn audit
yarn vendor-update
git diff
yarn lint:css
yarn lint:core-js-passing

Except for 9.2.x I didn't run yarn vendor-update since it's a new command in 9.3.x.

The last submitted patch, 18: yarn-3278163-15-9.3.x.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented

ah sorry didn't make the connection between the 2 issues, went in that issue with my brain off :)

Thanks for picking that up

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Committed da0e89b and pushed to 10.0.x. Thanks!
Committed c0bad15 and pushed to 9.5.x. Thanks!
Committed ccd1565 and pushed to 9.4.x. Thanks!

Re the 9.3.x and 9.2.x builds I'm confused as to why 9.2.x is changing CSS and removing -moz-appearance but 9.3.x is not? It feels right that we're not change CSS in 9.3.x and wrong that we are in 9.2.x

  • alexpott committed da0e89b on 10.0.x 
    Issue #3278163 by xjm, nod_, lauriii: yarn upgrade for latest security...

  • alexpott committed c0bad15 on 9.5.x 
    Issue #3278163 by xjm, nod_, lauriii: yarn upgrade for latest security...

  • alexpott committed ccd1565 on 9.4.x 
    Issue #3278163 by xjm, nod_, lauriii: yarn upgrade for latest security...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

That should have happened in #3262573: Update our yarn dev dependencies to the extent allowed by current constraints for 9.2.x too but I'm not sure why it didn't happen 🤔 Maybe the updates for different branches were made at different time or with different commands that led into small differences between the branches?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 10.0.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Ah I see somehow 9.3.x got ahead of everyone else in #3262573: Update our yarn dev dependencies to the extent allowed by current constraints - funky.

Committed 46277de and pushed to 9.3.x. Thanks!
Committed 7f65d32 and pushed to 9.2.x. Thanks!

  • alexpott committed 7f65d32 on 9.2.x 
    Issue #3278163 by xjm, nod_, lauriii: yarn upgrade for latest security...

  • alexpott committed 46277de on 9.3.x 
    Issue #3278163 by xjm, nod_, lauriii: yarn upgrade for latest security...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Thanks for #16, I was very surprised that CKEditor 5 files had changed, but #16 put my mind at ease 👍🙏

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

The file in #16 was generated with the tooling and instructions created in #3233491: Create process for reviewing changes in 3rd party JavaScript dependencies by @hooroomoo ☺️

Status: Fixed » Closed (fixed)

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