Fix #2242 by upgrading express and its dependencies to newer major versions #2244
Conversation
…major versions
| @@ -529,7 +529,7 @@ describe('ExpressReceiver', function () { | |||
| // Act | |||
| const req = { body: { }, url: 'http://localhost/slack/oauth_redirect', method: 'GET' } as Request; | |||
| const resp = { send: () => { } } as Response; | |||
| (receiver.router as any).handle(req, resp); | |||
| (receiver.router as any).handle(req, resp, () => {}); | |||
There was a problem hiding this comment.
This is the only breaking change affecting our code so far.
| "express": "^4.16.4", | ||
| "path-to-regexp": "^6.2.1", | ||
| "express": "^5.0.0", | ||
| "path-to-regexp": "^8.1.0", |
There was a problem hiding this comment.
Just ugprading path-ot-regrexp does not resolve the npm-audit error yet. express v4.x still brings path-to-regexp v6
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2244 +/- ##
==========================================
+ Coverage 81.59% 87.78% +6.19%
==========================================
Files 19 19
Lines 1646 1646
Branches 464 464
==========================================
+ Hits 1343 1445 +102
Misses 194 194
+ Partials 109 7 -102 ☔ View full report in Codecov by Sentry. |
|
The tests with Node 12 and 14 fail, but if we include this express major upgrade in a major release, we can consider dropping these EOLed runtime versions too. |
|
Interesting, even after deleting Perhaps doing a small, focussed bolt v4 major release that includes these changes, dropping node 12, 14 and 16, and perhaps a few other small things (like moving to socket-mode v2) is worthwhile. I will investigate the scope of the breaking changes in new express shortly. |
|
I am able to reproduce the issue locally now - not sure why I wasn't before - but also can fix the failing tests by adding However, based on internal discussions, it seems like most of the team is leaning towards doing a smaller-scoped new major version release that includes express v5. Since Let's continue the discussion internally to agree on the scope for relevant changes to address the security vulnerability. In the mean time, I will work on a fork of this PR as a draft for, at least, some of the suggestions floated internally re: scope of changes. |
|
@filmaj When you come up with a complete pull request for v4, please feel free to close this one! |
|
Sounds good, thank you, will do. I have started working on a branch that forks from your work 🙇 |
|
Closing in favour of #2254 (which incorporates this PR 🙇 ) |
Summary
This pull request could resolve #2242 but it's not an ideal solution for us. This change forces existing ExpressReceiever users to migrate to Express v5 immediately. To reviewers: read #2242 for context.
Even if we don't do this, migrating path-to-regexp to v8 must be done as early as possible.
Requirements (place an
xin each[ ])