expressjs
Crash after starting using version 4.21.0
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.
Seems to be caused by this - node_modules\path-to-regexp\index.js:68:15:
https://github.com/pillarjs/path-to-regexp/commit/29b96b4a1de52824e1ca0f49a701183cc4ed476f
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.
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...
cc @blakeembrey let me know if the PR looks like the solution you want to go with and I can prep the express release.
@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.
@blakeembrey I don't even get far enough to use any input, it crashes directly when I call const express = require("express").
Actually I revoke my previous statement, I get the crash here:
app.put(express.raw({ type: '*/*' })); (where app is let app = 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() })
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.
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.
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
@wesleytodd Looks like it may be a regression in Express, I have a short call to jump to now but can investigate it afterward.
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)
})
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?
I guess what I should ask is, can you provide a functional reproduction of the how you expect this to work?
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.
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.
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());
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.
@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.
@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.
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.
@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?
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.
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.
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.
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);
also the "breaking change" is that people's code would actually be getting called. I don't think thats a bad end result