Skip to content

Removing hook for express v5 router #6272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+66 −42
Open

Removing hook for express v5 router #6272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e48abd
removing hook for express v5 router
crysmags Aug 12, 2025
25c6637
separating v4 router instrumentation to its own addhook
crysmags Aug 29, 2025
58f6859
add router subscription for code origin
crysmags Aug 29, 2025
e2e6ee4
fixinf appsec tests to include router plugin on agent load
crysmags Sep 4, 2025
d03b10b
updating appsec -lfi to also block requests for router middleware
crysmags Sep 4, 2025
2ef48b6
removing mistake
crysmags Sep 4, 2025
06991b7
asm tests now check for right amount of traces - only router middlewa…
crysmags Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions integration-tests/appsec/standalone-asm.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ describe('Standalone ASM', () => {
assert.strictEqual(payload.length, 1)
assert.isArray(payload[0])

// express.request + 4 middlewares
assert.strictEqual(payload[0].length, 5)
// express.request + router.middleware x 2
assert.strictEqual(payload[0].length, 3)

assertKeep(payload[0][0])
})
Expand All @@ -95,7 +95,7 @@ describe('Standalone ASM', () => {
} else {
const fifthReq = payload[0]
assert.isArray(fifthReq)
assert.strictEqual(fifthReq.length, 5)
assert.strictEqual(fifthReq.length, 3)

const { meta, metrics } = fifthReq[0]
assert.notProperty(meta, 'manual.keep')
Expand Down Expand Up @@ -288,7 +288,7 @@ describe('Standalone ASM', () => {
} else {
const fifthReq = payload[0]
assert.isArray(fifthReq)
assert.strictEqual(fifthReq.length, 5)
assert.strictEqual(fifthReq.length, 3)
assertKeep(fifthReq[0])
}
}, 40000, 2)
Expand Down Expand Up @@ -330,8 +330,8 @@ describe('Standalone ASM', () => {
assert.strictEqual(payload.length, 1)
assert.isArray(payload[0])

// express.request + 4 middlewares
assert.strictEqual(payload[0].length, 5)
// express.request + router.middleware x 2
assert.strictEqual(payload[0].length, 3)

const { meta, metrics } = payload[0][0]
assert.property(meta, '_dd.iast.json') // WEAK_HASH and XCONTENTTYPE_HEADER_MISSING reported
Expand All @@ -350,8 +350,8 @@ describe('Standalone ASM', () => {
assert.strictEqual(payload.length, 1)
assert.isArray(payload[0])

// express.request + 4 middlewares
assert.strictEqual(payload[0].length, 5)
// express.request + router.middleware x 2
assert.strictEqual(payload[0].length, 3)

const { meta, metrics } = payload[0][0]
assert.property(meta, '_dd.appsec.json') // crs-942-100 triggered
Expand Down
10 changes: 3 additions & 7 deletions packages/datadog-instrumentations/src/express.js
View file
Open in desktop
Copy link
Contributor

@CarlesDD CarlesDD Aug 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're trying to do here, but I believe there's an issue with this change.

By removing the addHook blocks that scoped the wrapping logic to specific Express versions, we're now applying the same wrapping logic to express.Router regardless of whether it's Express 4 or 5. This isn't safe: in Express 5, Router is a constructor and the wrapping should happen on its prototype, while in Express 4 it's not, so wrapping it directly is necessary.

Applying the same wrap to both versions lead to runtime errors, as seen in the tests.

If the goal is to avoid wrapping express.Router entirely in Express 5, we should retain the version-specific addHook blocks and simply omit the wrapping for Router in the >=5.0.0 block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarlesDD I believe I addressed your issue, I have added in an addHook block for express v4 with a comment as well. This is will make it more explicit that for v4 we are wrapping the router and in v5 we will fall to the router instrumentation to wrap the calls.

Original file line number Diff line number Diff line change
Expand Up @@ -67,20 +67,16 @@ addHook({ name: 'express', versions: ['>=4'] }, express => {
return express
})

// Express 5 does not rely on router in the same way as v4 and should not be instrumented anymore.
// It would otherwise produce spans for router and express, and so duplicating them.
// We now fall back to router instrumentation
addHook({ name: 'express', versions: ['4'] }, express => {
shimmer.wrap(express.Router, 'use', wrapRouterMethod)
shimmer.wrap(express.Router, 'route', wrapRouterMethod)

return express
})

addHook({ name: 'express', versions: ['>=5.0.0'] }, express => {
shimmer.wrap(express.Router.prototype, 'use', wrapRouterMethod)
shimmer.wrap(express.Router.prototype, 'route', wrapRouterMethod)

return express
})

const queryParserReadCh = channel('datadog:query:read:finish')

function publishQueryParsedAndNext (req, res, next) {
Expand Down
10 changes: 10 additions & 0 deletions packages/datadog-plugin-express/src/code_origin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ class ExpressCodeOriginForSpansPlugin extends Plugin {
if (layerTags.has(layer)) return
layerTags.set(layer, entryTags(topOfStackFunc))
})
this.addSub('apm:router:middleware:enter', ({ req, layer }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.addSub('apm:router:middleware:enter', ({ req, layer }) => {
this.addSub('apm:router:middleware:enter', ({ req, layer }) => {

const tags = layerTags.get(layer)
if (!tags) return
web.getContext(req).span?.addTags(tags)
})

this.addSub('apm:router:route:added', ({ topOfStackFunc, layer }) => {
if (layerTags.has(layer)) return
layerTags.set(layer, entryTags(topOfStackFunc))
})
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/datadog-plugin-express/test/code_origin.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('Plugin', () => {
const config = { codeOriginForSpans: { enabled: false } }

describe(`with tracer config ${JSON.stringify(config)}`, () => {
before(() => agent.load(['express', 'http'], [{}, {}, { client: false }], config))
before(() => agent.load(['express', 'http', 'router'], [{}, { client: false }, {}], config))

after(() => agent.close({ ritmReset: false, wipe: true }))

Expand Down Expand Up @@ -55,7 +55,7 @@ describe('Plugin', () => {

for (const config of configs) {
describe(`with tracer config ${JSON.stringify(config)}`, () => {
before(() => agent.load(['express', 'http'], [{}, {}, { client: false }], config))
before(() => agent.load(['express', 'http', 'router'], [{}, { client: false }, {}], config))

after(() => agent.close({ ritmReset: false, wipe: true }))

Expand Down
54 changes: 33 additions & 21 deletions packages/datadog-plugin-express/test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('Plugin', () => {

describe('without http', () => {
before(() => {
return agent.load('express', { client: false })
return agent.load(['express', 'router'], [{ client: false }, {}])
})

after(() => {
Expand Down Expand Up @@ -94,7 +94,7 @@ describe('Plugin', () => {

describe('without configuration', () => {
before(() => {
return agent.load(['express', 'http'], [{}, { client: false }])
return agent.load(['express', 'http', 'router'], [{}, { client: false }, {}])
})

after(() => {
Expand Down Expand Up @@ -230,6 +230,9 @@ describe('Plugin', () => {
const spans = sort(traces[0])
const isExpress4 = semver.intersects(version, '<5.0.0')
let index = 0
const whichMiddleware = semver.intersects(version, '<5.0.0')
? 'express'
: 'router'
Comment on lines +233 to +235
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this senver.intersect 2 lines above.

Suggested change
const whichMiddleware = semver.intersects(version, '<5.0.0')
? 'express'
: 'router'
const whichMiddleware = isExpress4 ? 'express' : 'router'


const rootSpan = spans[index++]
expect(rootSpan).to.have.property('resource', 'GET /app/user/:id')
Expand All @@ -251,31 +254,31 @@ describe('Plugin', () => {
}

expect(spans[index]).to.have.property('resource', 'named')
expect(spans[index]).to.have.property('name', 'express.middleware')
expect(spans[index]).to.have.property('name', `${whichMiddleware}.middleware`)
expect(spans[index].parent_id.toString()).to.equal(rootSpan.span_id.toString())
expect(spans[index].meta).to.have.property('component', 'express')
expect(spans[index].meta).to.have.property('component', whichMiddleware)
index++

expect(spans[index]).to.have.property('resource', 'router')
expect(spans[index]).to.have.property('name', 'express.middleware')
expect(spans[index]).to.have.property('name', `${whichMiddleware}.middleware`)
expect(spans[index].parent_id.toString()).to.equal(rootSpan.span_id.toString())
expect(spans[index].meta).to.have.property('component', 'express')
expect(spans[index].meta).to.have.property('component', whichMiddleware)
index++

if (isExpress4) {
expect(spans[index].resource).to.match(/^bound\s.*$/)
} else {
expect(spans[index]).to.have.property('resource', 'handle')
}
expect(spans[index]).to.have.property('name', 'express.middleware')
expect(spans[index]).to.have.property('name', `${whichMiddleware}.middleware`)
expect(spans[index].parent_id.toString()).to.equal(spans[index - 1].span_id.toString())
expect(spans[index].meta).to.have.property('component', 'express')
expect(spans[index].meta).to.have.property('component', whichMiddleware)
index++

expect(spans[index]).to.have.property('resource', '<anonymous>')
expect(spans[index]).to.have.property('name', 'express.middleware')
expect(spans[index]).to.have.property('name', `${whichMiddleware}.middleware`)
expect(spans[index].parent_id.toString()).to.equal(spans[index - 1].span_id.toString())
expect(spans[index].meta).to.have.property('component', 'express')
expect(spans[index].meta).to.have.property('component', whichMiddleware)

expect(index).to.equal(spans.length - 1)
})
Expand Down Expand Up @@ -314,13 +317,16 @@ describe('Plugin', () => {
const spans = sort(traces[0])

const breakingSpanIndex = semver.intersects(version, '<5.0.0') ? 3 : 1
const whichMiddleware = semver.intersects(version, '<5.0.0')
? 'express'
: 'router'

expect(spans[0]).to.have.property('resource', 'GET /user/:id')
expect(spans[0]).to.have.property('name', 'express.request')
expect(spans[0].meta).to.have.property('component', 'express')
expect(spans[breakingSpanIndex]).to.have.property('resource', 'breaking')
expect(spans[breakingSpanIndex]).to.have.property('name', 'express.middleware')
expect(spans[breakingSpanIndex].meta).to.have.property('component', 'express')
expect(spans[breakingSpanIndex]).to.have.property('name', `${whichMiddleware}.middleware`)
expect(spans[breakingSpanIndex].meta).to.have.property('component', whichMiddleware)
})
.then(done)
.catch(done)
Expand Down Expand Up @@ -359,12 +365,15 @@ describe('Plugin', () => {
.assertSomeTraces(traces => {
const spans = sort(traces[0])
const errorSpanIndex = semver.intersects(version, '<5.0.0') ? 4 : 2
const whichMiddleware = semver.intersects(version, '<5.0.0')
? 'express'
: 'router'

expect(spans[0]).to.have.property('name', 'express.request')
expect(spans[errorSpanIndex]).to.have.property('name', 'express.middleware')
expect(spans[errorSpanIndex]).to.have.property('name', `${whichMiddleware}.middleware`)
expect(spans[errorSpanIndex].meta).to.have.property(ERROR_TYPE, error.name)
expect(spans[0].meta).to.have.property('component', 'express')
expect(spans[errorSpanIndex].meta).to.have.property('component', 'express')
expect(spans[errorSpanIndex].meta).to.have.property('component', whichMiddleware)
})
.then(done)
.catch(done)
Expand Down Expand Up @@ -1176,6 +1185,9 @@ describe('Plugin', () => {
.assertSomeTraces(traces => {
const spans = sort(traces[0])
const secondErrorIndex = spans.length - 2
const whichMiddleware = semver.intersects(version, '<5.0.0')
? 'express'
: 'router'

expect(spans[0]).to.have.property('error', 1)
expect(spans[0].meta).to.have.property(ERROR_TYPE, error.name)
Expand All @@ -1186,7 +1198,7 @@ describe('Plugin', () => {
expect(spans[secondErrorIndex].meta).to.have.property(ERROR_TYPE, error.name)
expect(spans[secondErrorIndex].meta).to.have.property(ERROR_MESSAGE, error.message)
expect(spans[secondErrorIndex].meta).to.have.property(ERROR_STACK, error.stack)
expect(spans[secondErrorIndex].meta).to.have.property('component', 'express')
expect(spans[secondErrorIndex].meta).to.have.property('component', whichMiddleware)
})
.then(done)
.catch(done)
Expand Down Expand Up @@ -1448,12 +1460,12 @@ describe('Plugin', () => {

describe('with configuration', () => {
before(() => {
return agent.load(['express', 'http'], [{
return agent.load(['express', 'http', 'router'], [{
service: 'custom',
validateStatus: code => code < 400,
headers: ['User-Agent'],
blocklist: ['/health']
}, { client: false }])
}, { client: false }, {}])
})

after(() => {
Expand Down Expand Up @@ -1576,9 +1588,9 @@ describe('Plugin', () => {

describe('with configuration for middleware disabled', () => {
before(() => {
return agent.load(['express', 'http'], [{
return agent.load(['express', 'http', 'router'], [{
middleware: false
}, { client: false }])
}, { client: false }, { middleware: false }])
})

after(() => {
Expand All @@ -1594,8 +1606,8 @@ describe('Plugin', () => {

let span

app.use((req, res, next) => {
span = tracer.scope().active()
app.use(async (req, res, next) => {
span = await tracer.scope().active()
next()
})

Expand Down
7 changes: 5 additions & 2 deletions packages/datadog-plugin-express/test/integration-test/client.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ describe('esm', () => {
describe('with DD_TRACE_MIDDLEWARE_TRACING_ENABLED unset', () => {
it('is instrumented', async () => {
proc = await spawnPluginIntegrationTestProc(sandbox.folder, 'server.mjs', agent.port)
const numberOfSpans = semver.intersects(version, '<5.0.0') ? 4 : 3
const numberOfSpans = semver.intersects(version, '<5.0.0') ? 4 : 2
const whichMiddleware = semver.intersects(version, '<5.0.0')
? 'express'
: 'router'

return curlAndAssertMessage(agent, proc, ({ headers, payload }) => {
assert.propertyVal(headers, 'host', `127.0.0.1:${agent.port}`)
Expand All @@ -48,7 +51,7 @@ describe('esm', () => {
assert.isArray(payload[0])
assert.strictEqual(payload[0].length, numberOfSpans)
assert.propertyVal(payload[0][0], 'name', 'express.request')
assert.propertyVal(payload[0][1], 'name', 'express.middleware')
assert.propertyVal(payload[0][1], 'name', `${whichMiddleware}.middleware`)
})
}).timeout(50000)
})
Expand Down
1 change: 1 addition & 0 deletions packages/dd-trace/src/appsec/channels.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ module.exports = {
responseSetHeader: dc.channel('datadog:http:server:response:set-header:start'),
responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'),
routerParam: dc.channel('datadog:router:param:start'),
routerMiddlewareError: dc.channel('apm:router:middleware:error'),
setCookieChannel: dc.channel('datadog:iast:set-cookie'),
setUncaughtExceptionCaptureCallbackStart: dc.channel('datadog:process:setUncaughtExceptionCaptureCallback:start'),
startGraphqlResolve: dc.channel('datadog:graphql:resolver:start'),
Expand Down
4 changes: 3 additions & 1 deletion packages/dd-trace/src/appsec/rasp/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ const web = require('../../plugins/util/web')
const {
setUncaughtExceptionCaptureCallbackStart,
expressMiddlewareError,
fastifyMiddlewareError
fastifyMiddlewareError,
routerMiddlewareError
} = require('../channels')
const { block, registerBlockDelegation, isBlocked } = require('../blocking')
const ssrf = require('./ssrf')
Expand Down Expand Up @@ -117,6 +118,7 @@ function enable (config) {

expressMiddlewareError.subscribe(blockOnDatadogRaspAbortError)
fastifyMiddlewareError.subscribe(blockOnDatadogRaspAbortError)
routerMiddlewareError.subscribe(blockOnDatadogRaspAbortError)
}

function disable () {
Expand Down
2 changes: 1 addition & 1 deletion packages/dd-trace/test/appsec/rasp/command_injection.express.plugin.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ describe('RASP - command_injection', () => {
}

before(() => {
return agent.load(['express', 'http', 'child_process'], { client: false })
return agent.load(['express', 'http', 'child_process', 'router'], { client: false })
})

before((done) => {
Expand Down
Loading