-
Notifications
You must be signed in to change notification settings - Fork 37
fix: Remove router dependency as it comes from Express that is marked as peerDependency now
#69
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
Conversation
|
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: npm/[email protected], npm/[email protected] |
Megapixel99
left a comment
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.
Merging this will get rid of this error message:
# npm audit report
path-to-regexp <=0.1.11
Severity: high
Unpatched `path-to-regexp` ReDoS in 0.1.x - https://github.com/advisories/GHSA-rhx6-c78j-4q9w
path-to-regexp outputs backtracking regular expressions - https://github.com/advisories/GHSA-9wv6-86v2-598j
No fix available
node_modules/router/node_modules/path-to-regexp
router 1.0.0-beta.1 - 2.0.0-beta.2
Depends on vulnerable versions of path-to-regexp
node_modules/router
@wesleytodd/openapi *
Depends on vulnerable versions of router
node_modules/@wesleytodd/openapi
3 high severity vulnerabilities
Some issues need review, and may require choosing
a different dependency.
Remove `router` dependency and mark `express` as peer dependency. This way we get rid of the security warnings when we npm install this package. We also allow the use of the Router that comes with whatever express version the host application has.
6b6c7a9 to
26b6513
Compare
c323332 to
c2f4cf9
Compare
|
OMG thank you for this! I have not reviewed it yet, but I literally opened a tab in my terminal with all the right files to fix this last week and just never got back to it. I will fully review it in the morning, but just wanted to drop a note of appreciation before then. EDIT: Oh wait, took a first glance and am not sure this is enough. The tests are failing because of some regexp changes, which is what I had been looking at last week. I know this is a good change, but I am not sure it will be enough to support both latest versions of 4.x and also 5. |
|
I guess I should add, for the vuln report, we obviously have to open those CVE's, but it only impacts a subset of route formats and none of the most common ones. So while it is important to update, as long as you read the CVE and ensure your app is not vulnerable to the ReDOS then you should be fine on your express version. If you want to read about the security issues we discovered while getting the project back on it's feet you can read about that here: https://expressjs.com/2024/10/22/security-audit-milestone-achievement.html |
|
The failing tests should be fixed by PR #66 |
router dependencyrouter dependency as it comes from Express that is marked as peerDependency now
|
@wesleytodd friendly ping on this one, including the test fix in #66 |
|
Upgrading to Express v5 is a larger effort, and requires us to maintain 2 branches for this project. The reasons why I think it requires us to have 2 branches:
cc @wesleytodd for comments I will close this PR for now. |
Remove
routerdependency and markexpressas peer dependency. This way we get rid of the security warnings when we npm install this package. We also allow the use of the Router that comes with whatever express version the host application has.