[ Demo ] @uppy/companion: express 5.0.0 compatibility #5457

maximilianschmid · 2024-09-12T09:54:41Z

No description provided.

socket-security · 2024-09-12T09:55:53Z

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 · 2024-09-12T09:56:06Z

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 · 2024-09-19T08:56:10Z

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.

mifi

I think this would be a breaking change and so needs a new major release of Companion

mifi · 2024-10-20T19:00:49Z

packages/@uppy/companion/src/companion.js

@@ -103,7 +103,7 @@ module.exports.app = (optionsArg = {}) => {
  // override provider credentials at request time
  // 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?', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options))
+  app.use('/connect/:oauthProvider/:override', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options))


looking at https://expressjs.com/en/guide/migrating-5.html#path-syntax i think wee need this instead?

Suggested change

app.use('/connect/:oauthProvider/:override', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options))

app.use('/connect/:oauthProvider{/:override}', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options))

source: https://www.npmjs.com/package/path-to-regexp#optional

mifi · 2024-10-20T19:10:38Z

packages/@uppy/companion/src/companion.js

@@ -117,7 +117,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))


I thought this should be

Suggested change

app.use(/(.*)/, middlewares.getCompanionMiddleware(options))

app.use(middlewares.getCompanionMiddleware(options))

mifi · 2024-10-20T19:11:04Z

packages/@uppy/companion/src/companion.js

@@ -132,7 +132,7 @@ module.exports.app = (optionsArg = {}) => {

  app.post('/:providerName/simple-auth', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, 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)


ditto

Suggested change

app.get('/:providerName/list/:id', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list)

app.get('/:providerName/list{/:id}', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list)

Kelketek · 2024-12-30T22:49:41Z

@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 · 2025-01-01T16:19:41Z

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

Murderlon · 2025-01-06T09:03:38Z

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 · 2025-01-06T16:46:03Z

@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.

… 5.0.1

maximilianschmid · 2025-01-08T10:42:36Z

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 · 2025-01-08T10:45:46Z

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 · 2025-01-08T12:29:32Z

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

Kelketek · 2025-01-08T14:43:17Z

Thank you, @Murderlon !

express 5.0.0 migration

730316e

github-actions bot added the pending end-to-end tests label Sep 12, 2024

- remove added body-parser dep from package.json

3350fc8

maximilianschmid changed the title ~~[ Demo ] express 5.0.0 compatibility~~ [ Demo ] @uppy/companion: express 5.0.0 compatibility Sep 12, 2024

Murderlon requested a review from mifi October 15, 2024 08:08

mifi reviewed Oct 20, 2024

View reviewed changes

update @uppy/companion

d261bf0

maximilianschmid added 2 commits January 8, 2025 11:15

Merge branch 'main' into express-5.0.0

b472594

packages/@uppy/companion/package.json - upgrade express from 5.0.0 to…

5bda199

… 5.0.1

maximilianschmid closed this Jan 8, 2025

Murderlon mentioned this pull request Jan 8, 2025

@uppy/companion: upgrade express & express-session #5582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ Demo ] @uppy/companion: express 5.0.0 compatibility #5457

[ Demo ] @uppy/companion: express 5.0.0 compatibility #5457

maximilianschmid commented Sep 12, 2024

socket-security bot commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

Murderlon commented Sep 19, 2024

mifi left a comment

mifi Oct 20, 2024

mifi Oct 20, 2024

mifi Oct 20, 2024

Kelketek commented Dec 30, 2024

maximilianschmid commented Jan 1, 2025

Murderlon commented Jan 6, 2025

Kelketek commented Jan 6, 2025

maximilianschmid commented Jan 8, 2025 •

edited

Loading

maximilianschmid commented Jan 8, 2025

Murderlon commented Jan 8, 2025

Kelketek commented Jan 8, 2025

	app.use('/connect/:oauthProvider/:override', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options))
	app.use('/connect/:oauthProvider{/:override}', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options))

	app.use(/(.*)/, middlewares.getCompanionMiddleware(options))
	app.use(middlewares.getCompanionMiddleware(options))

	app.get('/:providerName/list/:id', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list)
	app.get('/:providerName/list{/:id}', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list)

[ Demo ] @uppy/companion: express 5.0.0 compatibility #5457

[ Demo ] @uppy/companion: express 5.0.0 compatibility #5457

Conversation

maximilianschmid commented Sep 12, 2024

socket-security bot commented Sep 12, 2024 • edited Loading

github-actions bot commented Sep 12, 2024 • edited Loading

Murderlon commented Sep 19, 2024

mifi left a comment

Choose a reason for hiding this comment

mifi Oct 20, 2024

Choose a reason for hiding this comment

mifi Oct 20, 2024

Choose a reason for hiding this comment

mifi Oct 20, 2024

Choose a reason for hiding this comment

Kelketek commented Dec 30, 2024

maximilianschmid commented Jan 1, 2025

Murderlon commented Jan 6, 2025

Kelketek commented Jan 6, 2025

maximilianschmid commented Jan 8, 2025 • edited Loading

maximilianschmid commented Jan 8, 2025

Murderlon commented Jan 8, 2025

Kelketek commented Jan 8, 2025

socket-security bot commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

maximilianschmid commented Jan 8, 2025 •

edited

Loading