Security: Update npm dependencies to fix issues reported by audit #67708
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gziolo
added
npm Packages
gziolo
force-pushed
the
update/npm-audit-fix
branch
from
7cbb119 to
0e9bf77
Compare
|
Size Change: -5 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
gziolo
force-pushed
the
update/npm-audit-fix
branch
from
9fe2c94 to
6a8744b
Compare
gziolo
commented
Dec 9, 2024
| @@ -599,7 +588,7 @@ async function runPackagesRelease( config, customMessages ) { | |||
| await Promise.all( | |||
| temporaryFolders | |||
| .filter( ( tempDir ) => fs.existsSync( tempDir ) ) | |||
| .map( ( tempDir ) => rimraf( tempDir, rethrow ) ) | |||
| .map( ( tempDir ) => rimraf( tempDir ) ) | |||
There was a problem hiding this comment.
I'm not entirely sure if this is exactly the same handling, so I would appreciate some verification. The updated version of rimraf is Promise-based now, so it seems correct to me.
There was a problem hiding this comment.
I don't see why Promise.all was there before, but it makes sense now if there are promises being used.
Executed `npm audit fix` and manually resolved a few issues that required manual intervention
gziolo
force-pushed
the
update/npm-audit-fix
branch
from
6a8744b to
d61e77e
Compare
gziolo
requested review from
ObliviousHarmony,
t-hamano,
ntwb,
nerrad,
ajitbohra,
ryanwelcher and
kevin940726
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @galbus, @rohjay, @leup, @hueitan. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
By the way, I removed the automated update from |
sirreal
approved these changes
Dec 9, 2024
| @@ -599,7 +588,7 @@ async function runPackagesRelease( config, customMessages ) { | |||
| await Promise.all( | |||
| temporaryFolders | |||
| .filter( ( tempDir ) => fs.existsSync( tempDir ) ) | |||
| .map( ( tempDir ) => rimraf( tempDir, rethrow ) ) | |||
| .map( ( tempDir ) => rimraf( tempDir ) ) | |||
There was a problem hiding this comment.
I don't see why Promise.all was there before, but it makes sense now if there are promises being used.
Comment on lines
+5
to
+11
| @@ -7,5 +7,7 @@ export = lighthouse; | ||
| * @property {import('./index.js')['snapshot']} snapshot | ||
| */ | ||
| /** @type {import('./index.js')['default'] & ExportType} */ | ||
| +// Otherwise TS is confused when using ES types in CJS. | ||
| +// @ts-ignore | ||
| declare const lighthouse: typeof import("./index.js")['default']; | ||
| declare const lighthouse: typeof import("./index.js")["default"] & ExportType; |
There was a problem hiding this comment.
This patch was still necessary? That's too bad… it would be nice to dig into why it's needed and try to address that.
There was a problem hiding this comment.
Yes, I initially removed the old patch completely, but I had to re-apply it to the new version 😞
|
@swissspidy, can you double-check the |
Done 👍 Pushed a small config change for LH. |
|
Flaky tests detected in 2f8fd0c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12232912333
|
|
I think this rimraf upgrade is causing problems on windows, you can see it here. I'll investigate. |
|
Sorry I couldn't test it on Windows. Perhaps the https://github.com/isaacs/rimraf/blob/main/README.md
Update: The following should fix the issue, but I'm curious why the problem didn't occur on Mac OS and Linux: diff --git a/package.json b/package.json
index dbf69043c5..5ebdcd29bb 100644
--- a/package.json
+++ b/package.json
@@ -181,7 +181,7 @@
"postbuild:packages": " npm run --if-present --workspaces build:wp",
"build:plugin-zip": "bash ./bin/build-plugin-zip.sh",
"clean:package-types": "tsc --build --clean && rimraf \"./packages/*/build-types\"",
- "clean:packages": "rimraf \"./packages/*/@(build|build-module|build-wp|build-style)\"",
+ "clean:packages": "rimraf --glob \"./packages/*/@(build|build-module|build-wp|build-style)\"",
"component-usage-stats": "node ./node_modules/react-scanner/bin/react-scanner -c ./react-scanner.config.js",
"dev": "cross-env NODE_ENV=development npm run build:packages && concurrently \"wp-scripts start\" \"npm run dev:packages\"",
"dev:packages": "cross-env NODE_ENV=development concurrently \"node ./bin/packages/watch.js\" \"tsc --build --watch\"", |
|
Fix in #67829. |
I believe the shells were handling the glob expansion which I also attempt to address in #67829. I also think that other systems are more permissive with characters allowed in paths so may not error, for example I can make a directory named |
yogeshbhutkar
pushed a commit
to yogeshbhutkar/gutenberg
that referenced
this pull request
Dec 18, 2024
…rdPress#67708) * Security: Update npm dependencies to fix issues reported by audit Executed `npm audit fix` and manually resolved a few issues that required manual intervention * Update changelog entries * Update Lighthouse defaults --------- Co-authored-by: Pascal Birchler <[email protected]> Unlinked contributors: galbus, rohjay, leup, hueitan. Co-authored-by: gziolo <[email protected]> Co-authored-by: swissspidy <[email protected]> Co-authored-by: sirreal <[email protected]> Co-authored-by: stein2nd <[email protected]> Co-authored-by: kjroelke <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #56069 (updates
jest-dev-server).Fixes #54895 (updates
lighthouse).Fixes #62002 (updates
puppeteer-core).Fixes #63771.
Supersedes and fixes #65488.
Why?
npm auditreports several vulnerabilities. Some of them can be fixed withnpm audit fix.How?
Executed
npm audit fixand manually resolved a few issues that required manual intervention. After this PR there are still issues reported:It should help when installing development tools like
@wordpress/scripts, where all reported issues should be addressed.The biggest concern I had was with
rimrafas it had several breaking changes that forced updates in the code:I didn't update to the latest version v6, because it requires node 20 or higher, and WordPress packages still allow node 18, which is in maintenance mode until April 2025.
Testing Instructions
All CI checks should pass. In the development process, there were some issues reported due to breaking changes in
rimraf. The rest should be less problematic, but these type of changes are usually difficult to test as they mostly impact dev tooling that doesn't have so detailed testing coverage. There should be zero impact on production code.