express icon indicating copy to clipboard operation
express copied to clipboard

Express 5.0.0 Route with regex not working

Open HarishGangula opened this issue 1 year ago • 10 comments

We have created simple express application with following code


const express = require('express')
const app = express()
const port = 4000

function logErrors(err, req, res, next) {
    console.error(err.stack)
    next(err)
}

function clientErrorHandler(err, req, res, next) {
    if (req.xhr) {
        res.status(500).send({ error: 'Something failed!' })
    } else {
        next(err)
    }
}
function errorHandler (err, req, res, next) {
   
    res.status(err.statusCode || 500)
    res.json({ message: err.message || "something wrong" })
}

app.get('/', async (req, res) => {
    await Promise.reject({"message":"dsfjh", statusCode: 400})
    res.status(200).send("asfas")
})

app.get('/[discussion|page]/:slug', async (req, res) => {
    throw {statusCode: 400, message: "harish"}
    res.status(200).send("asfas")
})


app.use(logErrors)
app.use(clientErrorHandler)
app.use(errorHandler)

app.listen(port, () => {
    console.log(`Example app listening on port ${port}`)
})

It is throwing following error


~/express5/node_modules/path-to-regexp/dist/index.js:136
        throw new TypeError(`Unexpected ${nextType} at ${index}, expected ${type}: ${DEBUG_URL}`);
        ^

TypeError: Unexpected [ at 1, expected END: https://git.new/pathToRegexpError
    at Iter.consume (~/express5/node_modules/path-to-regexp/dist/index.js:136:15)
    at consume (~/express5/node_modules/path-to-regexp/dist/index.js:193:16)
    at parse (~/express5/node_modules/path-to-regexp/dist/index.js:197:20)
    at ~/express5/node_modules/path-to-regexp/dist/index.js:308:74
    at Array.map (<anonymous>)
    at pathToRegexp (~/express5/node_modules/path-to-regexp/dist/index.js:308:25)
    at Object.match (~/express5/node_modules/path-to-regexp/dist/index.js:278:30)
    at matcher (~/express5/node_modules/router/lib/layer.js:83:23)
    at new Layer (~/express5/node_modules/router/lib/layer.js:90:62)
    at Function.route (~/express5/node_modules/router/index.js:421:17)

We identified that path to regexp node module is broken which is causing this issue.

Node.js v20.10.0 used

HarishGangula avatar Sep 11 '24 07:09 HarishGangula

I have seen a similar issue with express 5.0 - which seems have root cause in path-to-regexp module:

/some-repo/node_modules/path-to-regexp/dist/index.js:85 throw new TypeError(Missing parameter name at ${i}: ${DEBUG_URL}); ^

TypeError: Missing parameter name at 1: https://git.new/pathToRegexpError at name (/some-repo/node_modules/path-to-regexp/dist/index.js:85:19) at lexer (/some-repo/node_modules/path-to-regexp/dist/index.js:103:27) at lexer.next () at Iter.peek (/some-repo/node_modules/path-to-regexp/dist/index.js:119:38) at Iter.tryConsume (/some-repo/node_modules/path-to-regexp/dist/index.js:125:28) at Iter.text (/some-repo/node_modules/path-to-regexp/dist/index.js:141:30) at consume (/some-repo/node_modules/path-to-regexp/dist/index.js:166:29) at parse (/some-repo/node_modules/path-to-regexp/dist/index.js:197:20) at /some-repo/node_modules/path-to-regexp/dist/index.js:308:74 at Array.map ()

thomashohn avatar Sep 11 '24 07:09 thomashohn

Is it realtes to https://blakeembrey.com/posts/2024-09-web-redos/ and https://github.com/advisories/GHSA-9wv6-86v2-598j? But I don't understand, why the major version of "path-to-regexp" was updated when express@5 was released?

ex1st avatar Sep 11 '24 08:09 ex1st

I agree - seems "weird"..

thomashohn avatar Sep 11 '24 08:09 thomashohn

https://github.com/expressjs/express/issues/5936#issuecomment-2340380583

SherbekMavlonov avatar Sep 11 '24 10:09 SherbekMavlonov

We fully dropped support for regular expressions in strings for routes. You can check out the docs for path-to-regexp@8 in that repo. We will be putting together a migration guide for folks, but in this case you need to either use an array of paths or write your own regular expression (very discouraged).

So it would become this:

app.get(['/discussion/:slug', '/page/:slug'], async (req, res) => {
    throw {statusCode: 400, message: "harish"}
    res.status(200).send("asfas")
})

wesleytodd avatar Sep 11 '24 16:09 wesleytodd

@wesleytodd, sad to hear, we used regexp for data type routing.

app.get('/product/:page([0-9]+)', async (req, res) => {

ex1st avatar Sep 11 '24 16:09 ex1st

We will be recommending folks use a more robust input validation approach for this. If you want, here is the open api library I maintain: https://github.com/wesleytodd/express-openapi/

The reason for this is mainly security around DOS protection. There is no way to support path segment regular expression matching while also avoiding major performance regressions. See more here: https://blakeembrey.com/posts/2024-09-web-redos/

This is a 100% big win for the ecosystem longer term, as regular expression route matching was one of the worst decisions from the early days which has stuck around. So while we understand the pain this change will cause, we stand by this decision as the right thing for the ecosystem and all express users (even if you need to change your approach to this sort of routing).

wesleytodd avatar Sep 11 '24 17:09 wesleytodd

@HarishGangula I do want to include as an aside here that '/[discussion|page]/:slug' did not do what you expected in express 4 either, since [] is a regex character group, so this was matching /d/x for example.

Due to time constraints I haven't had the capacity to write a version that supports parsing of regex characters into something that can be guaranteed safe to avoid ReDoS, but that's not a forever situation. They've been reserved so it's possible to add it back again later.

@ex1st Totally understand the concern, it's my biggest let down too. For now you'd have to include it in the method and call next() when it doesn't match a number. This is both good and bad, since it'll motivate people to use better validation libaries for more complex regex cases instead of trying to cram it into the path itself, but obviously for something like yours it looks like a regression.

blakeembrey avatar Sep 11 '24 18:09 blakeembrey

@blakeembrey This is one of the example but we have routes which uses regex like at end all routes i want keep '*' as route and send specific response instead of express default one.

HarishGangula avatar Sep 12 '24 04:09 HarishGangula

If you want to use a regex it’s fine to write a regex directly, it’s just not supported in the string.

blakeembrey avatar Sep 12 '24 05:09 blakeembrey

The migration guide explicitly mentions using regex as a string, but I take it that's not correct seeing as the example provided doesn't work (/:foo(.*))?

https://expressjs.com/en/guide/migrating-5.html#path-syntax

The following changes have been made to how the path string is matched to an incoming request:

richardsimko avatar Sep 14 '24 08:09 richardsimko

https://github.com/expressjs/express/issues/5948#issuecomment-2350915953 This solution is working fine. Thanks @richardsimko @blakeembrey for the quick help.

HarishGangula avatar Sep 16 '24 10:09 HarishGangula

I take it that's not correct

Yes, that guide needs updating. Please submit a PR on https://github.com/expressjs/expressjs.com

wesleytodd avatar Sep 16 '24 13:09 wesleytodd