Crash after starting using version 4.21.0 about express HOT 42 OPEN

In a more positive note, this helped uncover bugs in application code. So if you are landing on this issue with a similar problem, take a look at the behaviors you are expecting out of requests which hit these middleware and spend some time ensuring your application code is working as you intend. Sorry the docs were wrong and that we introduced a throw where it used to silently noop, but IMO this is as good an outcome as we could hope for.

from express.

but IMO this is as good an outcome as we could hope for.

No, this is the worst. This is why people start pinning modules and don't get potentially important CVE fixes, because libraries keep breaking with minor upgrades. This is nonsense. Add a warning, keep it as it was, increase major, but do not break with minor, please.

from express.

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

from express.

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

from express.

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)
      })

from express.

From looking over the various places that resulted in this bug, I believe this was never intended to work and the documentation is wrong. As a result, I'm working through these steps and you should remove these lines of code from your application since they didn't do anything before:

1. Introduce a clearer error in path-to-regexp
2. Remove this documentation from expressjs.com (which is under app.use, I believe it was a mistaken clarification)

If you want this code that previously did not run, to run, you can set the route to * as mentioned here: #5955 (comment)

from express.

sounds good 👍 thanks for digging into this everyone.

from express.

As it was documented on this way therefore I assume thousands of developers are using it on this way in their apps. Then a minor release breaks it and most likely thousands of apps. I understand the reasoning behind the change but it seem to be a major breaking change and should have not be in a minor release..

from express.

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() })

from express.

fwiw: to work around this issue we just updated our code to this and all still seems to be working

  app.put('*', _.validate, _.jsonOnly);
  app.post('*', _.validate, _.jsonOnly);
  app.patch('*', _.validate, _.jsonOnly);
  app.delete('*', _.validate, _.jsonOnly);

from express.

Hello,

Rather than patching a lib, #5975 describes a similar issue with a proposed fix.

The way the app binds routes to a controller causes the bug. Small patch in middlewaring resolves the case.

from express.

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

from express.

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

from express.

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

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

from express.

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.

from express.

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.

from express.

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

from express.

The same test in [email protected] 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?

from express.

I guess what I should ask is, can you provide a functional reproduction of the how you expect this to work?

from express.

Anyone who is in this thread who is using this form of the api.

from express.

I wonder if this was a bug in the documentation instead of Express, I suspect it's never actually worked before since the path generated wouldn't have started with / (as it'd be String(function () {})).

That said, it's possible to "fix" this, but that might change the behavior of applications since they weren't actually working before and now they'd start working.

from express.

Yeah I suspect the same. The old behavior looks like it would never match these middleware and would then just pass on to following matched layers. So if we made it now match, that would be a breaking change. I need more information from the folks using this to help us decide what is the right thing to do next.

from express.

I'm doing this to be able to parse xml, it now fails on the second to last line (has worked before):

const express = require("express")
const bodyParser = require("body-parser")

require('body-parser-xml')(bodyParser)

// Create the express app
let app = express();

// Setup how to parse data
app.put(express.raw({ type: '*/*' }));  // <--- crash here
app.use(bodyParser.xml());

from express.

I get that we need to fix the new throw, but based on the test and investigating the code here I dont think express.raw would have been called. Can you create a full code example which I can run on [email protected] which shows that the body is populated and that body-parser-xml is not the thing reading the raw body? We dont maintain that library, so I have no idea what it could be doing.

from express.

@az-faro I think we understand that, but if you commented out app.put(express.raw({ type: '*/*' })) the application probably works, in which case I'd be inclined to say either the documentation is wrong and fix that, or Express is wrong and fix that instead. Either way, I think this exposed a bug in your code and isn't a bug in the libraries.

How do you expect app.put(function () {}) to work? Is it just that you want to run that one middleware on PUT only?

@wesleytodd I'm inclined to fix this in Express and make it work, since it was documented to work, and the fact that it may break existing users relying it being broken isn't a breaking change.

from express.

@blakeembrey ok, I removed app.put(express.raw({ type: '*/*' })); and indeed the program still works fine without it. It was written a while ago, so I can't say for certain why I thought it was needed, apparently it isn't.

from express.

yes i believe the intention is the middleware get applied to all requests to the specified METHOD. So updating express to start matching correctly seems the right approach. This would be a bug fix, not a breaking change since thats what the document says it should be doing.

from express.

@alewitt2 We agree, but your application will start doing something it didn't do before, which could break your application. For example, if you change app.put to app.use and it doesn't work, us fixing this will break your application. Would this be a bad outcome for you?

from express.

yes understood. Not sure you want my opinion here 😆 as i am a bit of a purist when it comes to semver.. but since you asked, I don't think it matters if its a bad outcome for me. I think the feature is a good one and it should be fixed to work properly. and bug fixes arent breaking changes.

from express.

bug fixes arent breaking changes

I wish this was the case, but we have many times (like this one) had cases where what we considered a bug fix broke folks, and we always try to follow strict semver unless it is a critical security issue.

from express.

do what you think is best, but i would just fix it in 4x

from express.

also the "breaking change" is that people's code would actually be getting called. I don't think thats a bad end result

from express.

also the "breaking change" is that people's code would actually be getting called. I don't think thats a bad end result

it certainly CAN be. when you put it that way, this seems like a strong argument for not changing behavior until a semver major

from express.

I will also push my test which proves this should error, and we can merge that after we update to the version with the new error.

from express.

Ha, even better is that .get without a path is an alias for getting a setting. These variadic apis are so bad. We will be removing all of these going in some future major. This particular one I even tried to fix back in 2018 but it never landed.

from express.

Yep, this would have errored in early 4.x releases the same way it does today, it looks like I inadvertently introduced coercion in pillarjs/path-to-regexp@943ceb7. It's possible someone saw it not erroring and assumed it worked, and updated the docs as a result.

from express.

As it was documented on this way therefore I assume thousands of developers are using it on this way in their apps.

The old behavior meant the code did not run. It literally did not work. So “thousands of developers” never tested their code? You can just delete the lines that have this error and it works exactly the same.

Add a warning, keep it as it was, increase major, but do not break with minor, please.

In this world, what do you see as breaking? If we “fixed” the bug and made it work as documented, it might break much more because your code never ran before. And since it has never run, it probably isn’t being tested, so that could be much worse to run untested code for the first time than to expose an error.

Arguably it could keep silently failing but that isn’t good for anyone either. Code you write should work and should run as expected.

from express.

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.

from express.

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

from express.

@wesleytodd who are you asking?

from express.

It does depend.

Did it ever work on v4? If it didn't, I would recommend against fixing, just prevent the crash and emit a warning.

Fix on v5 please, or ensure it works okay, and make a note on release as a breaking change.

Nice work. My two cents.

from express.

Yep, this would have errored in early 4.x releases the same way it does today, it looks like I inadvertently introduced coercion in pillarjs/path-to-regexp@943ceb7. It's possible someone saw it not erroring and assumed it worked, and updated the docs as a result.

« committed 10 years ago » Let that sink in.

but IMO this is as good an outcome as we could hope for.

No, this is the worst. This is why people start pinning modules and don't get potentially important CVE fixes, because libraries keep breaking with minor upgrades. This is nonsense. Add a warning, keep it as it was, increase major, but do not break with minor, please.

I've been doing a variety of things, based on each project working, or going kaboom, or being so big that I cannot check everything.

1. Patch it (as is) #5955 (comment)
2. npm 8+ overrides

"overrides": {
  "encodeurl": "~2.0.0",
  "path-to-regexp": "^0.1.11"
},

See: pillarjs/send#240 & #5956 and you may want to add the following patch at your discretion:

diff --git a/node_modules/path-to-regexp/index.js b/node_modules/path-to-regexp/index.js
index 750c2bf..480fd9a 100644
--- a/node_modules/path-to-regexp/index.js
+++ b/node_modules/path-to-regexp/index.js
@@ -65,11 +65,11 @@ function pathToRegexp(path, keys, options) {
     return new RegExp(path.join('|'), flags);
   }
 
-  if (typeof path !== 'string') {
+  if (strict && typeof path !== 'string') {
     throw new TypeError('path must be a string, array of strings, or regular expression');
   }
 
-  path = path.replace(
+  path = String(path).replace(
     /\\.|(\/)?(\.)?:(\w+)(\(.*?\))?(\*)?(\?)?|[.*]|\/\(/g,
     function (match, slash, format, key, capture, star, optional, offset) {
       pos = offset + match.length;

My two cents.

from express.