router param instrumentation

packages/datadog-instrumentations/src/router.js

@@ -179,10 +179,9 @@ addHook({ name: 'router', versions: ['>=1'] }, Router => {
   return Router
 })
 
-function createWrapLayerMethod () {
-  // TODO: create a dedicate channel for this
-  const processParamsStartCh = channel('datadog:express:process_params:start')
+const processParamsStartCh = channel('datadog:express:process_params:start')
 
+function createWrapLayerMethod () {
   return function wrapMethod (original) {
     return function wrappedHandleRequest (req, res, next) {
       if (processParamsStartCh.hasSubscribers) {
@@ -191,8 +190,7 @@ function createWrapLayerMethod () {
         processParamsStartCh.publish({
           req,
           res,
-          abortController,
-          params: req?.params
+          abortController
         })
         if (abortController.signal.aborted) return
       }
@@ -209,4 +207,33 @@ addHook({
   return Layer
 })
 
+function wrapProcessParamsMethod (original) {
+  return function wrappedProcessParams (name, fn) {
+    const wrappedFn = function wrappedParamCallback (req, res, next, param) {
+      if (processParamsStartCh.hasSubscribers) {
+        const abortController = new AbortController()
+
+        processParamsStartCh.publish({
+          req,
+          res,
+          abortController
+        })
+
+        if (abortController.signal.aborted) return
+      }
+
+      return fn(req, res, next, param, name)
+    }
+
+    return original.call(this, name, wrappedFn)
+  }
+}
+
+addHook({
+  name: 'router', versions: ['>=2']
+}, router => {
+  shimmer.wrap(router.prototype, 'param', wrapProcessParamsMethod)
+  return router
+})
+
 module.exports = { createWrapRouterMethod }

packages/dd-trace/src/appsec/channels.js

@@ -19,6 +19,7 @@ module.exports = {
   nextBodyParsed: dc.channel('apm:next:body-parsed'),
   nextQueryParsed: dc.channel('apm:next:query-parsed'),
   expressProcessParams: dc.channel('datadog:express:process_params:start'),
+  routerParam: dc.channel('datadog:router:param:start'),
   responseBody: dc.channel('datadog:express:response:json:start'),
   responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'),
   httpClientRequestStart: dc.channel('apm:http:client:request:start'),

packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js

@@ -78,6 +78,15 @@ class TaintTrackingPlugin extends SourceIastPlugin {
       }
     )
 
+    this.addSub(
+      { channelName: 'datadog:router:param:start', tag: HTTP_REQUEST_PATH_PARAM },
+      ({ req }) => {
+        if (req && req.params !== null && typeof req.params === 'object') {
+          this._taintTrackingHandler(HTTP_REQUEST_PATH_PARAM, req, 'params')
+        }
+      }
+    )
+
     this.addSub(
       { channelName: 'datadog:query:read:finish', tag: HTTP_REQUEST_QUERY },
       ({ query }) => {

packages/dd-trace/src/appsec/index.js

@@ -16,7 +16,8 @@ const {
   expressProcessParams,
   responseBody,
   responseWriteHead,
-  responseSetHeader
+  responseSetHeader,
+  routerParam
 } = require('./channels')
 const waf = require('./waf')
 const addresses = require('./addresses')
@@ -67,6 +68,7 @@ function enable (_config) {
     nextBodyParsed.subscribe(onRequestBodyParsed)
     nextQueryParsed.subscribe(onRequestQueryParsed)
     expressProcessParams.subscribe(onRequestProcessParams)
+    routerParam.subscribe(onRequestProcessParams)
     responseBody.subscribe(onResponseBody)
     responseWriteHead.subscribe(onResponseWriteHead)
     responseSetHeader.subscribe(onResponseSetHeader)
@@ -311,6 +313,7 @@ function disable () {
   if (nextBodyParsed.hasSubscribers) nextBodyParsed.unsubscribe(onRequestBodyParsed)
   if (nextQueryParsed.hasSubscribers) nextQueryParsed.unsubscribe(onRequestQueryParsed)
   if (expressProcessParams.hasSubscribers) expressProcessParams.unsubscribe(onRequestProcessParams)
+  if (routerParam.hasSubscribers) routerParam.unsubscribe(onRequestProcessParams)
   if (responseBody.hasSubscribers) responseBody.unsubscribe(onResponseBody)
   if (responseWriteHead.hasSubscribers) responseWriteHead.unsubscribe(onResponseWriteHead)
   if (responseSetHeader.hasSubscribers) responseSetHeader.unsubscribe(onResponseSetHeader)

packages/dd-trace/test/appsec/iast/taint-tracking/plugin.spec.js

@@ -16,6 +16,7 @@ const queryParseFinishChannel = dc.channel('datadog:qs:parse:finish')
 const bodyParserFinishChannel = dc.channel('datadog:body-parser:read:finish')
 const cookieParseFinishCh = dc.channel('datadog:cookie:parse:finish')
 const processParamsStartCh = dc.channel('datadog:express:process_params:start')
+const routerParamStartCh = dc.channel('datadog:router:param:start')
 
 describe('IAST Taint tracking plugin', () => {
   let taintTrackingPlugin
@@ -42,16 +43,18 @@ describe('IAST Taint tracking plugin', () => {
   })
 
   it('Should subscribe to body parser, qs, cookie and process_params channel', () => {
-    expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(9)
+    expect(taintTrackingPlugin._subscriptions).to.have.lengthOf(11)
     expect(taintTrackingPlugin._subscriptions[0]._channel.name).to.equals('datadog:body-parser:read:finish')
     expect(taintTrackingPlugin._subscriptions[1]._channel.name).to.equals('datadog:multer:read:finish')
     expect(taintTrackingPlugin._subscriptions[2]._channel.name).to.equals('datadog:qs:parse:finish')
     expect(taintTrackingPlugin._subscriptions[3]._channel.name).to.equals('apm:express:middleware:next')
     expect(taintTrackingPlugin._subscriptions[4]._channel.name).to.equals('datadog:cookie:parse:finish')
     expect(taintTrackingPlugin._subscriptions[5]._channel.name).to.equals('datadog:express:process_params:start')
-    expect(taintTrackingPlugin._subscriptions[6]._channel.name).to.equals('apm:graphql:resolve:start')
-    expect(taintTrackingPlugin._subscriptions[7]._channel.name).to.equals('datadog:url:parse:finish')
-    expect(taintTrackingPlugin._subscriptions[8]._channel.name).to.equals('datadog:url:getter:finish')
+    expect(taintTrackingPlugin._subscriptions[6]._channel.name).to.equals('datadog:router:param:start')
+    expect(taintTrackingPlugin._subscriptions[7]._channel.name).to.equals('datadog:query:read:finish')
+    expect(taintTrackingPlugin._subscriptions[8]._channel.name).to.equals('apm:graphql:resolve:start')
+    expect(taintTrackingPlugin._subscriptions[9]._channel.name).to.equals('datadog:url:parse:finish')
+    expect(taintTrackingPlugin._subscriptions[10]._channel.name).to.equals('datadog:url:getter:finish')
   })
 
   describe('taint sources', () => {
@@ -209,7 +212,7 @@ describe('IAST Taint tracking plugin', () => {
       )
     })
 
-    it('Should taint request params when process params event is published', () => {
+    it('Should taint request params when process params event is published with processParamsStartCh', () => {
       const req = {
         params: {
           parameter1: 'tainted1'
@@ -224,6 +227,21 @@ describe('IAST Taint tracking plugin', () => {
       )
     })
 
+    it('Should taint request params when process params event is published with routerParamStartCh', () => {
+      const req = {
+        params: {
+          parameter1: 'tainted1'
+        }
+      }
+
+      routerParamStartCh.publish({ req })
+      expect(taintTrackingOperations.taintObject).to.be.calledOnceWith(
+        iastContext,
+        req.params,
+        HTTP_REQUEST_PATH_PARAM
+      )
+    })
+
     it('Should not taint request params when process params event is published with non params request', () => {
       const req = {}
 

packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.express.plugin.spec.js

@@ -80,11 +80,11 @@ describe('Path params sourcing with express', () => {
   let appListener
 
   withVersions('express', 'express', version => {
-    const checkParamIsTaintedAndNext = (req, res, next, param) => {
+    const checkParamIsTaintedAndNext = (req, res, next, param, name) => {
       const store = storage.getStore()
       const iastContext = iastContextFunctions.getIastContext(store)
 
-      const pathParamValue = param
+      const pathParamValue = name ? req.params[name] : req.params
       const isParameterTainted = isTainted(iastContext, pathParamValue)
       expect(isParameterTainted).to.be.true
       const taintedParameterValueRanges = getRanges(iastContext, pathParamValue)
@@ -185,8 +185,7 @@ describe('Path params sourcing with express', () => {
       })
     })
 
-    // to be fixed
-    it.skip('should taint path param on router.params callback', function (done) {
+    it('should taint path param on router.params callback', function (done) {
       const app = express()
 
       app.use('/:parameter1/:parameter2', (req, res) => {
@@ -206,20 +205,15 @@ describe('Path params sourcing with express', () => {
       })
     })
 
-    // to be fixed
-    it.skip('should taint path param on router.params callback with custom implementation', function (done) {
+    it('should taint path param on router.params callback with custom implementation', function (done) {
       const app = express()
 
       app.use('/:parameter1/:parameter2', (req, res) => {
         res.status(200).send()
       })
 
-      app.param((param, option) => {
-        return checkParamIsTaintedAndNext
-      })
-
-      app.param('parameter1')
-      app.param('parameter2')
+      app.param('parameter1', checkParamIsTaintedAndNext)
+      app.param('parameter2', checkParamIsTaintedAndNext)
 
       appListener = app.listen(0, 'localhost', () => {
         const port = appListener.address().port

packages/dd-trace/test/appsec/index.spec.js

@@ -15,6 +15,7 @@ const {
   nextBodyParsed,
   nextQueryParsed,
   expressProcessParams,
+  routerParam,
   responseBody,
   responseWriteHead,
   responseSetHeader
@@ -175,6 +176,7 @@ describe('AppSec Index', function () {
       expect(nextBodyParsed.hasSubscribers).to.be.false
       expect(nextQueryParsed.hasSubscribers).to.be.false
       expect(expressProcessParams.hasSubscribers).to.be.false
+      expect(routerParam.hasSubscribers).to.be.false
       expect(responseWriteHead.hasSubscribers).to.be.false
       expect(responseSetHeader.hasSubscribers).to.be.false
 
@@ -187,6 +189,7 @@ describe('AppSec Index', function () {
       expect(nextBodyParsed.hasSubscribers).to.be.true
       expect(nextQueryParsed.hasSubscribers).to.be.true
       expect(expressProcessParams.hasSubscribers).to.be.true
+      expect(routerParam.hasSubscribers).to.be.true
       expect(responseWriteHead.hasSubscribers).to.be.true
       expect(responseSetHeader.hasSubscribers).to.be.true
     })
@@ -268,6 +271,7 @@ describe('AppSec Index', function () {
       expect(nextBodyParsed.hasSubscribers).to.be.false
       expect(nextQueryParsed.hasSubscribers).to.be.false
       expect(expressProcessParams.hasSubscribers).to.be.false
+      expect(routerParam.hasSubscribers).to.be.false
       expect(responseWriteHead.hasSubscribers).to.be.false
       expect(responseSetHeader.hasSubscribers).to.be.false
     })