Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
yarn audit gives:
[ayrton:core | Sat 12:44:37] $ yarn audit
yarn audit v1.22.4
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ high │ netmask npm package vulnerable to octal input data │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ netmask │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=2.0.1 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ nightwatch │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ nightwatch > proxy-agent > pac-proxy-agent > pac-resolver > │
│ │ netmask │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://www.npmjs.com/advisories/1658 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ low │ Prototype Pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ ini │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >1.3.6 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ stylelint │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ stylelint > global-modules > global-prefix > ini │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://www.npmjs.com/advisories/1589 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ low │ Prototype Pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ ini │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >1.3.6 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ stylelint-no-browser-hacks │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ stylelint-no-browser-hacks > stylelint > global-modules > │
│ │ global-prefix > ini │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://www.npmjs.com/advisories/1589 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ high │ Prototype Pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ y18n │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=5.0.5||>=4.0.1 <5.0.0||>=3.2.2 <4.0.0 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ nightwatch │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ nightwatch > mocha > yargs > y18n │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://www.npmjs.com/advisories/1654 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ high │ Prototype Pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ y18n │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=5.0.5||>=4.0.1 <5.0.0||>=3.2.2 <4.0.0 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ nightwatch │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ nightwatch > mocha > yargs-unparser > yargs > y18n │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://www.npmjs.com/advisories/1654 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ low │ Prototype Pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ yargs-parser │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=13.1.2 <14.0.0 || >=15.0.1 <16.0.0 || >=18.1.2 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ stylelint-no-browser-hacks │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ stylelint-no-browser-hacks > stylelint > meow > yargs-parser │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://www.npmjs.com/advisories/1500 │
└───────────────┴──────────────────────────────────────────────────────────────┘
6 vulnerabilities found - Packages audited: 1106
Severity: 3 Low | 3 HighProposed resolution
Update them, including increasing our constraint for Nightwatch.
Release notes snippet
Drupal core's development dependency on the Nightwatch npm package has been increased from 1.2.1 to 1.6.3 and all locked versions of dependencies have been updated to address security issues in these dependencies.
The minimum version of node.js for 8.9.x development has been increased to version 10.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 3211810-25-9.0.x.patch | 281.61 KB | Spokje |
| #25 | reroll_diff_14_25-9.0.txt | 2.94 KB | Spokje |
| #25 | reroll_diff_14_25-8.9.x.txt | 2.7 KB | Spokje |
| #25 | 3211810-25-8.9.x.patch | 140.43 KB | Spokje |
| #25 | reroll_diff_14_25-9.1-x-9.2.x-9.3.x.txt | 2.15 KB | Spokje |
Comments
Comment #2
xjmComment #3
Kristen PolThanks for the issue and patches.
nightwatchfrom^1.2.1to^1.6.3core/package.jsonandcore/yarn.lockfilescomposer updateafter patching and these additional files were modified:Comment #4
Kristen PolAfter patching, here's what I see for
yarn audit:9.2
9.1
9.0
8.9
Comment #5
SpokjeOn the remaining
yargs-parserproblem: @mherchel seemed to found a way to fix that here: #3144854-6: Remove stylelint-no-browser-hacksComment #6
SpokjeComment #7
Kristen PolThanks @Spokje. I seem to have trouble with that patch in #3144854: Remove stylelint-no-browser-hacks. Is it working for you?
Comment #8
xjmYeah, let's leave the outstanding issue with
yargs-parserout of scope here since it predates the more serious vulns we're trying to correct.Comment #9
xjm@Kristen Pol BTW, did you do a yarn install in the core directory after applying the patch and before each
yarn audit? Otherwise it will have stale deps from the last branch you looked at. (It could be that I just forgot about the URL issue when reauditing 8.9.x as well.)Comment #10
Kristen PolThanks for the tip! Here's what I get with running
yarn install --forcebeforeyarn audit. For 8.9 and 9.0, I don't see an issue open forurl-regexbeing replaced withurl-regex-safelike what's mentioned in the advisory: https://www.npmjs.com/advisories/1550.9.2
9.1
9.0
8.9
Comment #11
Amber Himes MatzDrupal 8.9.x
Testing patch yarn-3211810-2-8_9_x.patch.
I'm getting the same output as @Kristen Pol for
yarn auditin #10. I did some digging and experimenting and found that upgrading postcss-custom-properties to version 10.0.0 upgrades postcss-values-parser to 4.0.0 which upgrades is-url-superb to 4.0.0, which is the earliest major version of is-url-superb that removes the dependency to the insecure url-regex package.Attached is a diff of core/yarn.lock after running in the core/ directory:
yarn upgrade postcss-custom-properties@^10.0.0...which was run after applying the patch and running yarn install.
After upgrading postcss-custom-properties to ^10.0.0, here's the output of
yarn auditon the 8.9.x site:As you can see, the 2 HIGH vulnerabilities for url-regex are gone, just the 2 LOW ones, which I understand are being addressed in #3144854: Remove stylelint-no-browser-hacks.
I haven't tested the 3 other patches yet, as I got caught up in the one for 8.9.x.
Comment #12
Kristen PolMoving back to needs work based on #11.
Comment #13
Amber Himes MatzNote: I didn't see the need to run a composer update after applying the patches since the patches only modify core/package.json and core/yarn.lock, so I didn't get any of the modified files mentioned in #3 (Step #3).
TL;DR: Mostly, just the same results as Kristen in #10.
9.2.x
1. Downloaded and installed Drupal via 9.2.x branch (and did a git pull to get latest changes)
2. Applied patch in #2 (yarn-3211810-2-9_2_x.patch)
3. From core directory, ran
yarn install4. Via yarn.lock, verified nightwatch is at version 1.6.3
5. From core directory, ran
yarn auditOutput of yarn audit:
I think this is being addressed in #3144854: Remove stylelint-no-browser-hacks, so I call this a pass.
Drupal 9.1.x
1. Downloaded and installed Drupal via 9.1.x branch (and did a git pull to get latest changes)
2. Applied patch in #2 (yarn-3211810-2-9_1_x.patch)
3. From core directory, ran
yarn install4. Via yarn.lock, verified nightwatch is at version 1.6.3
5. From core directory, ran
yarn auditOutput of yarn audit:
Also pass for same reason as above.
Drupal 9.0.x
1. Downloaded and installed Drupal via 9.0.x branch (and did a git pull to get latest changes)
2. Applied patch in #2 (yarn-3211810-2-9_0_x.patch)
3. From core directory, ran
yarn install4. Via yarn.lock, verified nightwatch is at version 1.6.3
5. From core directory, ran
yarn auditOutput of yarn audit:
Not a pass.
Drupal 8.9.x
1. Downloaded and installed Drupal via 8.9.x branch (and did a git pull to get latest changes)
2. Applied patch in #2 (yarn-3211810-2-8_9_x.patch)
3. From core directory, ran
yarn install4. Via yarn.lock, verified nightwatch is at version 1.6.3
5. From core directory, ran
yarn auditOutput of yarn audit:
Again, not a pass.
Comment #14
alexpottI'm working on this.
Comment #15
alexpottHere's patches that resolve the url-regex security issues on 9.0.x and 8.9.x. I've also executed:
yarn run build && yarn run lint:css && yarn run lint:core-js-passingto make sure the builds are up-to-date with the packages. This has resulted in some js changes on 9.1, 9.2 and 9.3 - they can all use the same patch. On 8.9.x and 9.0.x this has also result in some CSS fixes. See the interdiffs for this.Comment #16
lauriiiCan we upgrade
postcss-custom-propertiesto 10.0.0 given that it drops support for versions of Node.js that Drupal 8.9.x supports? If we do that, we should do it explicitly by mentioning it in the release notes, and changing the Node.js version requirement in the package.json.Comment #17
alexpottIf we don't upgrade it we have
I think given these are core build tools we should update.
Comment #18
lauriiiI'm not against updating if the release managers are fine with that. If we increase the node.js version requirement, we should make it explicitly by updating the minimum node.js version in the package.json, and potentially mentioning in the release notes.
Comment #19
alexpottBumping the Node dependency in 8.9.x - there's no way around fixing the security issues in #17 without this. FWIW Node 10 appears to not be supported at this point either - https://nodejs.org/en/about/releases/
The other patches in #14 are fine because the minimum Node version on 9.x is 12.
Comment #20
catchSince it's only a build tool, I think it's OK to update it. Or at least - given a choice between the leaving security issue and updating, we should go for updating. Untagging for release manager review.
Comment #21
lauriiiI think in that case this is ready. I manually reviewed all three of the patches, and tested them manually by running the same commands listed in #15.
Comment #22
alexpottWhen committing it is possible that the caniuse db might need to be updated - there's a patch that does this on #3212177: Update caniuse-lite as it is outdated
There is a work-around -it's slightly messed up but there you go...
Comment #23
catchI went ahead and committed #3212177: Update caniuse-lite as it is outdated, but that means this needs a re-roll for yarn.lock...
Comment #24
SpokjeRe-rolling as we speak
Comment #25
SpokjeComment #26
Kristen PolThanks for the updated patches. I tested against 9.2, 9.1, 9.0, and 8.9:
9.2
9.1
9.0
8.9
Comment #27
SpokjeThanks for the (lightning speed) review @Kristen Pol
Seeing that the
yargs-parserissue is going to be dealt with elsewhere:Thus spoketh @xjm in #8
shouldn't this be
RTBC?Comment #28
Spokje(Reverting unwanted status change)
Comment #29
Kristen PolEven though
yarn auditis looking good, unfortunately I'm not comfortable marking RTBC because there are a lot of changes to files other thancore/yarn.lockandcore/package.jsonthat I'm unclear about.Comment #30
alexpott@Kristen Pol those changes come from running
yarn run build- every time we make changes to the dev tools we need to run that command to transpile JS and process CSS.Comment #31
Kristen Pol@alexpott Thanks for the clarification. I assumed it was something like that but didn't want to move forward without making sure it was expected. Marking RTBC based on the
yarn auditresults and thatyargs-parserwill be handled in #3144854: Remove stylelint-no-browser-hacks.Comment #32
Amber Himes Matz+1 RTBC
I tested the patches in #25 and got the same results for
yarn auditas @Kristen Pol in #26.I also tested
yarn test:nightwatch --tag corein all versions and confirmed that nightwatch tests would run after the upgrade to 1.6.3. (The tests did run; there were some test failures, but nightwatch did run.)Comment #33
catchWhen I try to commit the patch, core's pre-commit hooks are failing on the following:
Is this because only transpilation has failed and not the file (and if so a bug in the pre-commit hooks)?
It would be good to get this patch in before the patch releases, so ideas appreciated. It's possible to commit with the pre-commit hooks disabled of course, but would be good to understand what's going on.
Comment #34
SpokjeTook inspiration from #3118726-35: Upgrade to js.cookie 3 I did a
yarn install && yarn run build && yarn run lint:css && yarn run lint:core-js-passing && yarn run build:json all of the patches (note the addedyarn run build:js). That didn't change any files.My theory:
Since the JavaScript changes are only in parts of the js that is not present in the es6 counterparts, there's nothing to transpilate.
cross-envcheck sees that the original js has changed, checks the es6 counterpart, sees it has no changes and goesCLUNKComment #39
catch#34 seems like a good explanation. Since I am about 99.9% sure it is cross-env that is at fault here, I've gone ahead and committed this bypassing the pre-commit hooks. Let's open a follow-up to see if we can make cross-env a bit more forgiving.
Comment #41
catchFor the follow-up - @alexpott suggested I needed to run
npx browserslist@latest --update-dbThat didn't work by itself, but the following did:
(revert the commit)
1. rm -rf node_modules
2. npx browserslist@latest --update-db
3. yarn install
4. git reset --hard
5. Apply and commit patch
Doing this every commit check seems like overkill, but seems to be a cruft issue as opposed to an actual bug in cross-env.
Comment #42
SpokjeDo we also need an 8.9.x release notes addition about upping the minimum node.js version?
Thus spoketh@lauriii in #18.
Comment #43
catchAdded to the release notes snippet, and tagging.
Comment #44
SpokjeTips hat @catch and silently changes the release notes tag.
Comment #45
longwaveThis patch updated the cspell dictionaries and caused a spelling issue elsewhere in core, because only changed files are spellchecked, but that doesn't help when the dictionary itself changes: #3212521: [backport] cspell dislikes identifer in core/modules/views/src/Plugin/views/filter/FilterPluginBase.php and will fail any patch touching that file
Maybe the commit check script needs to run cspell across all of core if yarn.lock is updated?
Comment #47
xjm