Skip to content
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

Incompatible with path-to-regex ≥ 6 #4136

Closed
guimard opened this issue Dec 19, 2019 · 13 comments
Closed

Incompatible with path-to-regex ≥ 6 #4136

guimard opened this issue Dec 19, 2019 · 13 comments

Comments

@guimard
Copy link

guimard commented Dec 19, 2019

Hi,

express is not compatible with recent path-to-regex. The fix is simple but then test fail:

diff --git a/lib/router/layer.js b/lib/router/layer.js
index 4dc8e86..cc96d56 100644
--- a/lib/router/layer.js
+++ b/lib/router/layer.js
@@ -13,7 +13,7 @@
  * @private
  */

-var pathRegexp = require('path-to-regexp');
+const { pathToRegexp } = require('path-to-regexp');
 var debug = require('debug')('express:router:layer');

 /**
@@ -42,7 +42,7 @@ function Layer(path, options, fn) {
   this.name = fn.name || '<anonymous>';
   this.params = undefined;
   this.path = undefined;
-  this.regexp = pathRegexp(path, this.keys = [], opts);
+  this.regexp = pathToRegexp(path, this.keys = [], opts);

   // set fast path flags
   this.regexp.fast_star = path === '*'

Cheers,
Xavier

@dougwilson
Copy link
Contributor

Yes, we know. The new version has a lot of changes that happened which breaks existing rout definitions. We are working to upgrade it in express 5 breaking release in this PR: pillarjs/router#42

@dougwilson
Copy link
Contributor

If you are looking to use the new route syntax, we will have it out soon enough in a new express 5 pre release. Express 4 will continue to use the older path to regexp as we have set in our package.json as not to break existing express 4 apps 👍

@guimard
Copy link
Author

guimard commented Dec 19, 2019

@dougwilson: hi, I'm Debian Developer and try to update express in Debian. I'll wait for express 5 to update it 😉

NB: express is currently broken in Debian testing

@dougwilson
Copy link
Contributor

If you're looking to use express, you can find the install instructions here: https://github.com/expressjs/express/blob/master/Readme.md#installation

@guimard
Copy link
Author

guimard commented Dec 19, 2019

@dougwilson : I know that. For security reason, Debian does not accepts sub-library embedding. Then express has to use Debian version for each of its dependencies. That's why I filed this issue.

@dougwilson
Copy link
Contributor

Ok, gotcha. So just a note that existing express 4 apps will just break if the wrong version of path to regex is loaded for them. Upgrading the lib in express won't fix app the apps using express as they would still be broken and they would have to use the new route syntax where applicable.

@wesleytodd
Copy link
Member

@guimard why is the Debian package for express trying to use a different version of path-to-regexp than what ships with express?

@guimard
Copy link
Author

guimard commented Dec 20, 2019

Because there is only one path-to-regexp, and updated at least each time a vulnerability is discovered. Here it was updated because some other modules needs it

@guimard
Copy link
Author

guimard commented Dec 20, 2019

Note that I'm talking about Debian testing/unstable. When a Debian stable is published, there is no more any library update except for security reasons (or significant bug)

@dougwilson
Copy link
Contributor

I'm not sure what that means. There isn't a vulnerability against path-to-regexp that I've seen before, at least with the version we're using. Just because other modules may depend on other versions of path-to-regexp is not really relevant in the Node.js/npm ecosystem since the way packages are managed allows different modules to declare their own version ranges of modules.

But, ultimately, path-to-regexp 6 will break existing express 4 apps. So if you're saying that Debian would require express to use path-to-regexp 6, even if we did that, apps written to work on express 4 would cease to function properly...

@guimard
Copy link
Author

guimard commented Dec 20, 2019

Yes, it will break existing app for Debian testing users, but not for Debian stable users 😉

@guimard
Copy link
Author

guimard commented Dec 20, 2019

Node and Go have a really different eco-system than other languages. They allow to use an embedded outdated library while other don't.

@wesleytodd
Copy link
Member

I still fail to see how something we would change would help anyone using Debian packages to install node modules. And I don’t think this is a use case we should ever support, this just should not be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants