Fix AppSec manual blocking parameters (#4606)

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

@@ -9,6 +9,8 @@ let templateHtml = blockedTemplates.html
 let templateJson = blockedTemplates.json
 let templateGraphqlJson = blockedTemplates.graphqlJson
 
+let defaultBlockingActionParameters
+
 const responseBlockedSet = new WeakSet()
 
 const specificBlockingTypes = {
@@ -23,7 +25,7 @@ function addSpecificEndpoint (method, url, type) {
   detectedSpecificEndpoints[getSpecificKey(method, url)] = type
 }
 
-function getBlockWithRedirectData (rootSpan, actionParameters) {
+function getBlockWithRedirectData (actionParameters) {
   let statusCode = actionParameters.status_code
   if (!statusCode || statusCode < 300 || statusCode >= 400) {
     statusCode = 303
@@ -32,10 +34,6 @@ function getBlockWithRedirectData (rootSpan, actionParameters) {
     Location: actionParameters.location
   }
 
-  rootSpan.addTags({
-    'appsec.blocked': 'true'
-  })
-
   return { headers, statusCode }
 }
 
@@ -49,7 +47,7 @@ function getSpecificBlockingData (type) {
   }
 }
 
-function getBlockWithContentData (req, specificType, rootSpan, actionParameters) {
+function getBlockWithContentData (req, specificType, actionParameters) {
   let type
   let body
 
@@ -90,28 +88,28 @@ function getBlockWithContentData (req, specificType, rootSpan, actionParameters)
     'Content-Length': Buffer.byteLength(body)
   }
 
-  rootSpan.addTags({
-    'appsec.blocked': 'true'
-  })
-
   return { body, statusCode, headers }
 }
 
-function getBlockingData (req, specificType, rootSpan, actionParameters) {
+function getBlockingData (req, specificType, actionParameters) {
   if (actionParameters?.location) {
-    return getBlockWithRedirectData(rootSpan, actionParameters)
+    return getBlockWithRedirectData(actionParameters)
   } else {
-    return getBlockWithContentData(req, specificType, rootSpan, actionParameters)
+    return getBlockWithContentData(req, specificType, actionParameters)
   }
 }
 
-function block (req, res, rootSpan, abortController, actionParameters) {
+function block (req, res, rootSpan, abortController, actionParameters = defaultBlockingActionParameters) {
   if (res.headersSent) {
     log.warn('Cannot send blocking response when headers have already been sent')
     return
   }
 
-  const { body, headers, statusCode } = getBlockingData(req, null, rootSpan, actionParameters)
+  const { body, headers, statusCode } = getBlockingData(req, null, actionParameters)
+
+  rootSpan.addTags({
+    'appsec.blocked': 'true'
+  })
 
   for (const headerName of res.getHeaderNames()) {
     res.removeHeader(headerName)
@@ -125,7 +123,8 @@ function block (req, res, rootSpan, abortController, actionParameters) {
 }
 
 function getBlockingAction (actions) {
-  return actions?.block_request || actions?.redirect_request
+  // waf only returns one action, but it prioritizes redirect over block
+  return actions?.redirect_request || actions?.block_request
 }
 
 function setTemplates (config) {
@@ -152,12 +151,19 @@ function isBlocked (res) {
   return responseBlockedSet.has(res)
 }
 
+function setDefaultBlockingActionParameters (actions) {
+  const blockAction = actions?.find(action => action.id === 'block')
+
+  defaultBlockingActionParameters = blockAction?.parameters
+}
+
 module.exports = {
   addSpecificEndpoint,
   block,
   specificBlockingTypes,
   getBlockingData,
   getBlockingAction,
   setTemplates,
-  isBlocked
+  isBlocked,
+  setDefaultBlockingActionParameters
 }

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

@@ -94,11 +94,13 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) {
     const rootSpan = web.root(req)
     if (!rootSpan) return
 
-    const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, rootSpan, requestData.wafAction)
+    const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, requestData.wafAction)
     abortData.statusCode = blockingData.statusCode
     abortData.headers = blockingData.headers
     abortData.message = blockingData.body
 
+    rootSpan.setTag('appsec.blocked', 'true')
+
     abortController?.abort()
   }
 

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

@@ -4,6 +4,8 @@ const fs = require('fs')
 const waf = require('./waf')
 const { ACKNOWLEDGED, ERROR } = require('./remote_config/apply_states')
 
+const blocking = require('./blocking')
+
 let defaultRules
 
 let appliedRulesData = new Map()
@@ -19,6 +21,8 @@ function loadRules (config) {
     : require('./recommended.json')
 
   waf.init(defaultRules, config)
+
+  blocking.setDefaultBlockingActionParameters(defaultRules?.actions)
 }
 
 function updateWafFromRC ({ toUnapply, toApply, toModify }) {
@@ -141,6 +145,8 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) {
       }
       if (newActions.modified) {
         appliedActions = newActions
+
+        blocking.setDefaultBlockingActionParameters(concatArrays(newActions))
       }
     } catch (err) {
       newApplyState = ERROR
@@ -242,6 +248,8 @@ function clearAllRules () {
   appliedExclusions.clear()
   appliedCustomRules.clear()
   appliedActions.clear()
+
+  blocking.setDefaultBlockingActionParameters(undefined)
 }
 
 module.exports = {

packages/dd-trace/test/appsec/blocking-actions-rules.json

@@ -33,7 +33,11 @@
   "actions": [
     {
       "id": "block",
-      "otherParam": "other"
+      "otherParam": "other",
+      "parameters": {
+        "location": "/error",
+        "status_code": 302
+      }
     },
     {
       "id": "otherId",

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

@@ -220,12 +220,15 @@ describe('GraphQL', () => {
         grpc_status_code: '10'
       }
 
+      const rootSpan = { setTag: sinon.stub() }
+
       const abortController = context.abortController
 
       sinon.stub(waf, 'run').returns({
         block_request: blockParameters
       })
-      sinon.stub(web, 'root').returns({})
+
+      sinon.stub(web, 'root').returns(rootSpan)
 
       startGraphqlResolve.publish({ context, resolverInfo })
 
@@ -240,7 +243,9 @@ describe('GraphQL', () => {
       const abortData = {}
       apolloChannel.asyncEnd.publish({ abortController, abortData })
 
-      expect(blocking.getBlockingData).to.have.been.calledOnceWithExactly(req, 'graphql', {}, blockParameters)
+      expect(blocking.getBlockingData).to.have.been.calledOnceWithExactly(req, 'graphql', blockParameters)
+
+      expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true')
     })
   })
 })

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

@@ -1,11 +1,14 @@
 'use strict'
 
+const path = require('path')
+const fs = require('fs')
 const { loadRules, clearAllRules, updateWafFromRC } = require('../../src/appsec/rule_manager')
 const Config = require('../../src/config')
 const { ACKNOWLEDGED } = require('../../src/appsec/remote_config/apply_states')
 
 const rules = require('../../src/appsec/recommended.json')
 const waf = require('../../src/appsec/waf')
+const blocking = require('../../src/appsec/blocking')
 
 describe('AppSec Rule Manager', () => {
   let config
@@ -14,9 +17,11 @@ describe('AppSec Rule Manager', () => {
     clearAllRules()
     config = new Config()
 
-    sinon.stub(waf, 'init').callThrough()
-    sinon.stub(waf, 'destroy').callThrough()
-    sinon.stub(waf, 'update').callThrough()
+    sinon.stub(waf, 'init')
+    sinon.stub(waf, 'destroy')
+    sinon.stub(waf, 'update')
+
+    sinon.stub(blocking, 'setDefaultBlockingActionParameters')
   })
 
   afterEach(() => {
@@ -39,15 +44,30 @@ describe('AppSec Rule Manager', () => {
       config.appsec.rules = './bad-formatted-rules.json'
       expect(() => { loadRules(config.appsec) }).to.throw()
     })
+
+    it('should call updateBlockingConfiguration with proper params', () => {
+      const rulesPath = path.join(__dirname, './blocking-actions-rules.json')
+      const testRules = JSON.parse(fs.readFileSync(rulesPath))
+
+      config.appsec.rules = rulesPath
+
+      loadRules(config.appsec)
+
+      expect(waf.init).to.have.been.calledOnceWithExactly(testRules, config.appsec)
+      expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly(testRules.actions)
+    })
   })
 
   describe('clearAllRules', () => {
     it('should call clear method on all applied rules', () => {
       loadRules(config.appsec)
       expect(waf.init).to.have.been.calledOnce
 
+      blocking.setDefaultBlockingActionParameters.resetHistory()
+
       clearAllRules()
       expect(waf.destroy).to.have.been.calledOnce
+      expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly(undefined)
     })
   })
 
@@ -483,29 +503,62 @@ describe('AppSec Rule Manager', () => {
       })
 
       it('should apply blocking actions', () => {
-        const asm = {
-          actions: [
-            {
-              id: 'block',
-              otherParam: 'other'
-            },
-            {
-              id: 'otherId',
-              moreParams: 'more'
-            }
-          ]
-        }
-
         const toApply = [
           {
             product: 'ASM',
             id: '1',
-            file: asm
+            file: {
+              actions: [
+                {
+                  id: 'notblock',
+                  parameters: {
+                    location: '/notfound',
+                    status_code: 404
+                  }
+                }
+              ]
+            }
+          },
+          {
+            product: 'ASM',
+            id: '2',
+            file: {
+              actions: [
+                {
+                  id: 'block',
+                  parameters: {
+                    location: '/redirected',
+                    status_code: 302
+                  }
+                }
+              ]
+            }
           }
         ]
 
         updateWafFromRC({ toUnapply: [], toApply, toModify: [] })
-        expect(waf.update).to.have.been.calledOnceWithExactly(asm)
+
+        const expectedPayload = {
+          actions: [
+            {
+              id: 'notblock',
+              parameters: {
+                location: '/notfound',
+                status_code: 404
+              }
+            },
+            {
+              id: 'block',
+              parameters: {
+                location: '/redirected',
+                status_code: 302
+              }
+            }
+          ]
+        }
+
+        expect(waf.update).to.have.been.calledOnceWithExactly(expectedPayload)
+        expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly(expectedPayload.actions)
       })
 
       it('should unapply blocking actions', () => {
@@ -530,6 +583,11 @@ describe('AppSec Rule Manager', () => {
         ]
         updateWafFromRC({ toUnapply: [], toApply, toModify: [] })
 
+        expect(waf.update).to.have.been.calledOnceWithExactly(asm)
+        expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly(asm.actions)
+
+        sinon.resetHistory()
+
         const toUnapply = [
           {
             product: 'ASM',
@@ -539,7 +597,8 @@ describe('AppSec Rule Manager', () => {
 
         updateWafFromRC({ toUnapply, toApply: [], toModify: [] })
 
-        expect(waf.update).to.have.been.calledOnceWithExactly(asm)
+        expect(waf.update).to.have.been.calledOnceWithExactly({ actions: [] })
+        expect(blocking.setDefaultBlockingActionParameters).to.have.been.calledOnceWithExactly([])
       })
 
       it('should ignore other properties', () => {