Skip to content

Commit

Permalink
improve express regex middleware path parsing (#3203)
Browse files Browse the repository at this point in the history
Co-authored-by: Dominik Krejcik <dominik.krejcik@gmail.com>
  • Loading branch information
tlhunter and domkck committed Jun 7, 2023
1 parent d2843c1 commit 9d8ae7e
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 1 deletion.
74 changes: 74 additions & 0 deletions packages/datadog-plugin-express/test/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,80 @@ describe('Plugin', () => {
})
})

it('long regex should not steal path', done => {
const app = express()

try {
app.use(/\/foo\/(bar|baz|bez)/, (req, res, next) => {
next()
})
} catch (err) {
// eslint-disable-next-line no-console
console.log('This version of Express (>4.0 <4.6) has broken support for regex routing. Skipping this test.')
this.skip && this.skip() // mocha allows dynamic skipping, tap does not
return done()
}

app.get('/foo/bar', (req, res) => {
res.status(200).send('')
})

getPort().then(port => {
agent
.use(traces => {
const spans = sort(traces[0])

expect(spans[0]).to.have.property('resource', 'GET /foo/bar')
})
.then(done)
.catch(done)

appListener = app.listen(port, 'localhost', () => {
axios
.get(`http://localhost:${port}/foo/bar`)
.catch(done)
})
})
})

it('long regex child of string router should not steal path', done => {
const app = express()
const router = express.Router()

try {
router.use(/\/(bar|baz|bez)/, (req, res, next) => {
next()
})
app.use('/foo', router)
} catch (err) {
// eslint-disable-next-line no-console
console.log('This version of Express (>4.0 <4.6) has broken support for regex routing. Skipping this test.')
this.skip && this.skip() // mocha allows dynamic skipping, tap does not
return done()
}

app.get('/foo/bar', (req, res) => {
res.status(200).send('')
})

getPort().then(port => {
agent
.use(traces => {
const spans = sort(traces[0])

expect(spans[0]).to.have.property('resource', 'GET /foo/bar')
})
.then(done)
.catch(done)

appListener = app.listen(port, 'localhost', () => {
axios
.get(`http://localhost:${port}/foo/bar`)
.catch(done)
})
})
})

it('should not lose the current path on next', done => {
const app = express()
const Router = express.Router
Expand Down
13 changes: 12 additions & 1 deletion packages/datadog-plugin-router/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ class RouterPlugin extends WebPlugin {
route = context.stack.join('')

// Longer route is more likely to be the actual route handler route.
if (route.length > context.route.length) {
if (isMoreSpecificThan(route, context.route)) {
context.route = route
}
} else {
Expand All @@ -148,4 +148,15 @@ class RouterPlugin extends WebPlugin {
}
}

function isMoreSpecificThan (routeA, routeB) {
if (!routeIsRegex(routeA) && routeIsRegex(routeB)) {
return true
}
return routeA.length > routeB.length
}

function routeIsRegex (route) {
return route.includes('(/') && route.includes('/)')
}

module.exports = RouterPlugin

0 comments on commit 9d8ae7e

Please sign in to comment.