Crash after starting using version 4.21.0

[az-faro]

I just updated express to fix the latest reported CVEs, but the last version (4.21.0) is now causing crashes when loading express.

Crash happens when I call this:
const express = require("express")

Error is:

...\node_modules\path-to-regexp\index.js:68
  path = path.replace(
              ^

TypeError: path.replace is not a function
    at pathToRegexp (...\node_modules\path-to-regexp\index.js:68:15)
    at new Layer (...\node_modules\express\lib\router\layer.js:45:17)
    at Function.route (...\node_modules\express\lib\router\index.js:505:15)
    at app.<computed> [as put] (...\node_modules\express\lib\application.js:498:30)
    at Object.<anonymous> (...\js\index.js:69:9)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)

I realize this is not the express library, but express is the only user of path-to-regexp in my application, so it can't be any other culprit.

The exact same code on my side runs fine using for example express 4.19.2.

[NewEraCracker]

Seems to be caused by this - node_modules\path-to-regexp\index.js:68:15:

pillarjs/path-to-regexp@29b96b4

I would go into that module and change.

path = path.replace(

for:

path = String(path).replace(

My two cents.

PS: If this works good, you can make a patch using patch-package.

[NewEraCracker]

Can be reproduced if the argument of routing path is not a string.

For example, this crashes with the same stack trace: app.get(1, (req, res) => res.sendStatus(200));

My workaround works. So my advise is for you to check all routes and make sure they are strings.

The patch:

diff --git a/node_modules/path-to-regexp/index.js b/node_modules/path-to-regexp/index.js
index 1150335..52edd3e 100644
--- a/node_modules/path-to-regexp/index.js
+++ b/node_modules/path-to-regexp/index.js
@@ -65,7 +65,7 @@ function pathToRegexp(path, keys, options) {
     return new RegExp(path.join('|'), flags);
   }
 
-  path = path.replace(
+  path = String(path).replace(
     /\\.|(\/)?(\.)?:(\w+)(\(.*?\))?(\*)?(\?)?|[.*]|\/\(/g,
     function (match, slash, format, key, capture, star, optional, offset) {
       pos = offset + match.length;

Edit after close: These routes/layers were getting ignored. The route was getting garbled to symbols of function. At least it did not crash...

[wesleytodd]

cc @blakeembrey let me know if the PR looks like the solution you want to go with and I can prep the express release.

[blakeembrey]

@az-faro what are you using as input if it’s not a string, array, or regex?

This library shouldn’t be trying to coerce non-strings since that will hide important bugs in the users code. For example, I can’t think of any native object that could coerce and still match a real route.

If you have a legitimate use-case for something using this that isn’t a bug, I’d be happy to patch this back, but it wasn’t a part of the original API contract even if it may have worked accidentally.

[az-faro]

@blakeembrey I don't even get far enough to use any input, it crashes directly when I call const express = require("express").

[az-faro]

Actually I revoke my previous statement, I get the crash here:

app.put(express.raw({ type: '*/*' })); (where app is let app = express();)

[alewitt2]

we are also running into this, and it appears to be because we are defining routes without a path at all, which should be allowed as defined in the docs Middleware callback function examples, specifically we are doing things like:

app.put(function (req, res, next) { next() })
app.post(function (req, res, next) { next() })
app.patch(function (req, res, next) { next() })
app.delete(function (req, res, next) { next() })

[blakeembrey]

So that's specific to .use, and isn't supported in regular methods. Did you test these ever worked? If they were passed to path-to-regexp and generated routes it was just the route function ..., which doesn't seem like it would have worked before.

[blakeembrey]

Oh wait, it's in your screenshot, this seems like a regression in Express and not path-to-regexp. Trying to coerce this in path-to-regexp would have hidden this bug.

[alewitt2]

yes this has worked for us for a long time.. and if you read that last sentence it explicitly says it should work for all app.METHODs

Even though the examples are for app.use(), they are also valid for app.use(), app.METHOD(), and app.all()

i agree this should be handled in express before it gets to path-to-regexp

[blakeembrey]

@wesleytodd Looks like it may be a regression in Express, I have a short call to jump to now but can investigate it afterward.

[wesleytodd]

I am very surprised this is not covered in the tests. I also have a few meetings starting in 25min, but I will try before then to get a test (and maybe fix) opened. If we dont have a PR to land in the next hour though, I will be on some work meetings and have trouble releasing this until later today.

EDIT: yep it is wild this was not covered in tests for this long. It was as easy as this to reproduce

      it('should support ' + method.toUpperCase() + ' without a path', function(done){
        if (method === 'query' && shouldSkipQuery(process.versions.node)) {
          this.skip()
        }
        var app = express();

        app[method](function(req, res){
          res.send(method)
        });

        request(app)
        [method]('/foo')
        .expect(200, done)
      })

[wesleytodd]

The same test in express@4.19.2 and I get a 404 instead of a throw. Were these routes layers actually being matched in the way you were using them in the past?