-
Notifications
You must be signed in to change notification settings - Fork 384
Use brace-expansion >2.0.1 #3065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the brace-expansion
dependency to require version >= 2.0.1, which appears to address a security vulnerability by updating the regular expression pattern used in the brace expansion logic.
- Updates package.json to add explicit
brace-expansion
dependency with minimum version 2.0.1 - Updates compiled JavaScript files to reflect the new brace-expansion implementation with improved regex pattern
- Fixes a potential ReDoS (Regular Expression Denial of Service) vulnerability in the comma-matching pattern
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated no comments.
File | Description |
---|---|
package.json | Adds explicit brace-expansion dependency with version constraint >=2.0.1 |
lib/*.js (multiple files) | Generated JavaScript code reflecting the updated brace-expansion package with security fix |
Files not reviewed (1)
- package-lock.json: Language not supported
package.json
Outdated
@@ -52,7 +52,8 @@ | |||
"path": "^0.12.7", | |||
"semver": "^7.7.2", | |||
"uuid": "^11.1.0", | |||
"zlib": "^1.0.5" | |||
"zlib": "^1.0.5", | |||
"brace-expansion": ">=2.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I believe the dependencies are in alphabetical order, so it would be good if this addition maintained that order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't depend on brace-expansion ourselves, right? Perhaps best to try to update the dependency that pulls in the outdated library version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, that dependency comes from https://www.npmjs.com/package/@actions/glob, which is stuck at 0.5.0 from a year ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ask the actions folk to update their dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking better, that dependency points to brace-expansion < 2.0.0, which is not problematic. The only problematic ones are 2.0.0
(that we don't have) and 2.0.1
. I tried retracing where that comes from, but in the end just resorted to an explicit override of that specific version
package-lock.json
Outdated
"node_modules/minimatch/node_modules/brace-expansion": { | ||
"version": "1.1.12", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we're still pulling in v1.1.12 here — if we want to override this I think we need to modify the overrides
property in package.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either that or update all the dependencies that rely on old versions of brace-expansion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, if we can update minimatch
, that's better. We've had situations before where we've wanted to bump a transient dependency before the direct dependency had bumped it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, now I see why dependabot didn't manage to do this update. I'll get back to it on Monday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(notice I didn't tell which Monday 😜 😅 on it now)
7b474e5
to
f11caf4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at the lockfile and we're still pulling in v1.1.11 of brace-expansion
— does that need updating too?
No, I double checked and the versions that need updating are just 2.0.0 (which we don't use) and 2.0.1. Versions <2.0.0 are ok. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist