[ Demo ] @uppy/companion: express 5.0.0 compatibility

[maximilianschmid]

[socket-security[bot]]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/[email protected]	environment, filesystem	0	172 kB	vitaly
npm/[email protected]	None	0	11.1 kB	juliangruber
npm/[email protected]	None	0	124 kB	mathias
npm/[email protected]	None	0	7.5 kB	jonschlinkert
npm/[email protected]	None	0	6.93 kB	doowb
npm/[email protected]	None	0	8.69 kB	sindresorhus
npm/[email protected]	environment	0	176 kB	acemarke
npm/[email protected]	None	0	6.96 kB	sindresorhus
npm/[email protected]	None	0	90.4 kB	typescript-bot
npm/[email protected]	None	0	14.8 kB	isaacs

🚮 Removed packages: npm/@types/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

[github-actions[bot]]

Diff output files

diff --git a/packages/@uppy/companion/lib/companion.js b/packages/@uppy/companion/lib/companion.js
index 055c7ec..b05770a 100644
--- a/packages/@uppy/companion/lib/companion.js
+++ b/packages/@uppy/companion/lib/companion.js
@@ -89,7 +89,7 @@ module.exports.app = (optionsArg = {}) => {
   // Making `POST` request to the `/connect/:provider/:override?` route requires a form body parser middleware:
   // See https://github.com/simov/grant#dynamic-http
   app.use(
-    "/connect/:oauthProvider/:override?",
+    "/connect/:oauthProvider/:override",
     express.urlencoded({ extended: false }),
     getCredentialsOverrideMiddleware(providers, options),
   );
@@ -103,7 +103,7 @@ module.exports.app = (optionsArg = {}) => {
   });
   app.use(middlewares.cors(options));
   // add uppy options to the request object so it can be accessed by subsequent handlers.
-  app.use("*", middlewares.getCompanionMiddleware(options));
+  app.use(/(.*)/, middlewares.getCompanionMiddleware(options));
   app.use("/s3", s3(options.s3));
   if (options.enableUrlEndpoint) {
     app.use("/url", url());
@@ -175,7 +175,7 @@ module.exports.app = (optionsArg = {}) => {
     middlewares.hasSimpleAuthProvider,
     controllers.simpleAuth,
   );
-  app.get("/:providerName/list/:id?", middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list);
+  app.get("/:providerName/list/:id", middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list);
   // backwards compat:
   app.get("/search/:providerName/list", middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list);
   app.post(

[Murderlon]

Hi, are you sure that there are no breaking changes for people integrating Companion into their own Express 4.x or people making custom providers?

If that would break some cases, this can't be merged as we won't do a another major for a while.

[Kelketek]

@mifi @maximilianschmid Just wanted to bump this here-- projects I'm working with which use companion have been complaining about security vulnerabilities from the old package versions for a while and I would feel a lot better getting this merged in. If it's not something you have the resources to work on now, is there something I can do to help this along?

[maximilianschmid]

@Kelketek I‘ll have a look next week if we‘ll work on this.

[Murderlon]

projects I'm working with which use companion have been complaining about security vulnerabilities

Which vulnerabilities? Note that warnings does not equal that Companion is actually vulnerable. If there are any security issues, that's a separate issue to resolve, not something to rush a major release for (unless there's no other way).

[Kelketek]

@Murderlon Here's the output of npm audit:

❯ npm audit                                                                                                                                                        ─╯
# npm audit report

body-parser  <1.20.3
Severity: high
body-parser vulnerable to denial of service when url encoding is enabled - https://github.com/advisories/GHSA-qwcr-r2fm-qrc7
fix available via `npm audit fix --force`
Will install @uppy/[email protected], which is a breaking change
node_modules/express/node_modules/body-parser
  express  <=4.21.1 || 5.0.0-alpha.1 - 5.0.0
  Depends on vulnerable versions of body-parser
  Depends on vulnerable versions of cookie
  Depends on vulnerable versions of path-to-regexp
  Depends on vulnerable versions of send
  Depends on vulnerable versions of serve-static
  node_modules/express
    @uppy/companion  *
    Depends on vulnerable versions of cookie-parser
    Depends on vulnerable versions of express
    Depends on vulnerable versions of express-session
    Depends on vulnerable versions of grant
    node_modules/@uppy/companion

cookie  <0.7.0
cookie accepts cookie name, path, and domain with out of bounds characters - https://github.com/advisories/GHSA-pxg6-pf52-xh8x
fix available via `npm audit fix --force`
Will install @uppy/[email protected], which is a breaking change
node_modules/cookie
node_modules/express-session/node_modules/cookie
node_modules/express/node_modules/cookie
node_modules/grant/node_modules/cookie
  cookie-parser  1.0.1 - 1.4.6
  Depends on vulnerable versions of cookie
  node_modules/cookie-parser
  express-session  1.0.1 - 1.18.0
  Depends on vulnerable versions of cookie
  node_modules/express-session
  grant  >=5.3.0
  Depends on vulnerable versions of cookie
  node_modules/grant


path-to-regexp  <=0.1.11
Severity: high
path-to-regexp outputs backtracking regular expressions - https://github.com/advisories/GHSA-9wv6-86v2-598j
Unpatched `path-to-regexp` ReDoS in 0.1.x - https://github.com/advisories/GHSA-rhx6-c78j-4q9w
fix available via `npm audit fix --force`
Will install @uppy/[email protected], which is a breaking change
node_modules/path-to-regexp

send  <0.19.0
send vulnerable to template injection that can lead to XSS - https://github.com/advisories/GHSA-m6fv-jmcg-4jfg
fix available via `npm audit fix --force`
Will install @uppy/[email protected], which is a breaking change
node_modules/send
  serve-static  <=1.16.0
  Depends on vulnerable versions of send
  node_modules/serve-static

These warnings have appeared for four months or so, and they've been pretty concerning to me since they seem plausibly like they would affect companion. But if you're able to verify that companion is not affected, that would do a lot to assuage my fears. I agree that major version shouldn't be rushed, though in this case even if the major version is just 'we bump to the latest version of express and any other changes are minimal', that doesn't seem like rushing, it just seems like following semantic versioning and making sure security is up to date.

If there are other changes that are going into the next version bump that aren't well-vetted, that would give me more pause, or if you expect marketing pushes alongside major version numbers (which might happen with a project like Express itself).

An alternative would be to modify this PR to better verify backwards compatibility, in which case a major version update would not be needed. It might be done by adding in some conditionals checking between versions. It doesn't seem like the changes included in this PR are especially massive, so that looks like a plausible route-- to me, at least.

[maximilianschmid]

I'm not able to guarantee compatibility of a [email protected] companion version that works in a [email protected] app. So I have created a repo which you can use to use a [email protected] companion app version in your v5 express app.

Just use it in your package.json like this:

"@uppy/companion": "github:planmanager/uppycompanion#5.4.0",

It works for us in our production app. Hope this helps if you want to do the same.

[maximilianschmid]

As express@5 is not the default yet I think it make sense for uppy to stay on express@4 and upgrade to express@5 along with the next major version. To bridge the time you can use our repo.

[Murderlon]

@Kelketek resolved here: #5582. Will release it today.

[Kelketek]

Thank you, @Murderlon !