diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 65812357614..e4a41751bee 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -8,7 +8,6 @@ const detectedSpecificEndpoints = {} let templateHtml = blockedTemplates.html let templateJson = blockedTemplates.json let templateGraphqlJson = blockedTemplates.graphqlJson -let blockingConfiguration const specificBlockingTypes = { GRAPHQL: 'graphql' @@ -22,13 +21,13 @@ function addSpecificEndpoint (method, url, type) { detectedSpecificEndpoints[getSpecificKey(method, url)] = type } -function getBlockWithRedirectData (rootSpan) { - let statusCode = blockingConfiguration.parameters.status_code +function getBlockWithRedirectData (rootSpan, actionParameters) { + let statusCode = actionParameters.status_code if (!statusCode || statusCode < 300 || statusCode >= 400) { statusCode = 303 } const headers = { - Location: blockingConfiguration.parameters.location + Location: actionParameters.location } rootSpan.addTags({ @@ -48,7 +47,7 @@ function getSpecificBlockingData (type) { } } -function getBlockWithContentData (req, specificType, rootSpan) { +function getBlockWithContentData (req, specificType, rootSpan, actionParameters) { let type let body let statusCode @@ -64,7 +63,7 @@ function getBlockWithContentData (req, specificType, rootSpan) { // parse the Accept header, ex: Accept: text/html, application/xhtml+xml, application/xml;q=0.9, */*;q=0.8 const accept = req.headers.accept?.split(',').map((str) => str.split(';', 1)[0].trim()) - if (!blockingConfiguration || blockingConfiguration.parameters.type === 'auto') { + if (!actionParameters || actionParameters.type === 'auto') { if (accept?.includes('text/html') && !accept.includes('application/json')) { type = 'text/html; charset=utf-8' body = templateHtml @@ -73,7 +72,7 @@ function getBlockWithContentData (req, specificType, rootSpan) { body = templateJson } } else { - if (blockingConfiguration.parameters.type === 'html') { + if (actionParameters.type === 'html') { type = 'text/html; charset=utf-8' body = templateHtml } else { @@ -83,11 +82,7 @@ function getBlockWithContentData (req, specificType, rootSpan) { } } - if (blockingConfiguration?.type === 'block_request' && blockingConfiguration.parameters.status_code) { - statusCode = blockingConfiguration.parameters.status_code - } else { - statusCode = 403 - } + statusCode = actionParameters?.status_code || 403 const headers = { 'Content-Type': type, @@ -101,21 +96,22 @@ function getBlockWithContentData (req, specificType, rootSpan) { return { body, statusCode, headers } } -function getBlockingData (req, specificType, rootSpan) { - if (blockingConfiguration?.type === 'redirect_request' && blockingConfiguration.parameters.location) { - return getBlockWithRedirectData(rootSpan) +function getBlockingData (req, specificType, rootSpan, actionParameters) { + if (actionParameters?.location) { + return getBlockWithRedirectData(rootSpan, actionParameters) } else { - return getBlockWithContentData(req, specificType, rootSpan) + return getBlockWithContentData(req, specificType, rootSpan, actionParameters) } } -function block (req, res, rootSpan, abortController, type) { +// Is anybody using type argument? +function block (req, res, rootSpan, abortController, type, actionParameters) { if (res.headersSent) { log.warn('Cannot send blocking response when headers have already been sent') return } - const { body, headers, statusCode } = getBlockingData(req, type, rootSpan) + const { body, headers, statusCode } = getBlockingData(req, type, rootSpan, actionParameters) res.writeHead(statusCode, headers).end(body) @@ -142,15 +138,10 @@ function setTemplates (config) { } } -function updateBlockingConfiguration (newBlockingConfiguration) { - blockingConfiguration = newBlockingConfiguration -} - module.exports = { addSpecificEndpoint, block, specificBlockingTypes, getBlockingData, - setTemplates, - updateBlockingConfiguration + setTemplates } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index d6b17ea49b1..b5c373908cf 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -223,8 +223,12 @@ function onPassportVerify ({ credentials, user }) { function handleResults (actions, req, res, rootSpan, abortController) { if (!actions || !req || !res || !rootSpan || !abortController) return - if (actions.includes('block')) { - block(req, res, rootSpan, abortController) + if (actions.block_request) { + block(req, res, rootSpan, abortController, null, actions.block_request) + } + + if (actions.redirect_request) { + block(req, res, rootSpan, abortController, null, actions.redirect_request) } } diff --git a/packages/dd-trace/src/appsec/rule_manager.js b/packages/dd-trace/src/appsec/rule_manager.js index 3cbef3597e3..5078ed40853 100644 --- a/packages/dd-trace/src/appsec/rule_manager.js +++ b/packages/dd-trace/src/appsec/rule_manager.js @@ -20,10 +20,6 @@ function loadRules (config) { : require('./recommended.json') waf.init(defaultRules, config) - - if (defaultRules.actions) { - blocking.updateBlockingConfiguration(defaultRules.actions.find(action => action.id === 'block')) - } } function updateWafFromRC ({ toUnapply, toApply, toModify }) { @@ -95,6 +91,7 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { } if (file && file.actions && file.actions.length) { + batchConfiguration = true newActions.set(id, file.actions) } @@ -112,7 +109,9 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { newRuleset || newRulesOverride.modified || newExclusions.modified || - newCustomRules.modified) { + newCustomRules.modified || + newActions.modified + ) { const payload = newRuleset || {} if (newRulesData.modified) { @@ -127,6 +126,9 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { if (newCustomRules.modified) { payload.custom_rules = concatArrays(newCustomRules) } + if (newActions.modified) { + payload.actions = concatArrays(newActions) + } try { waf.update(payload) @@ -146,6 +148,9 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { if (newCustomRules.modified) { appliedCustomRules = newCustomRules } + if (newActions.modified) { + appliedActions = newActions + } } catch (err) { newApplyState = ERROR newApplyError = err.toString() @@ -156,11 +161,6 @@ function updateWafFromRC ({ toUnapply, toApply, toModify }) { config.apply_state = newApplyState if (newApplyError) config.apply_error = newApplyError } - - if (newActions.modified) { - blocking.updateBlockingConfiguration(concatArrays(newActions).find(action => action.id === 'block')) - appliedActions = newActions - } } // A Map with a new prop `modified`, a bool that indicates if the Map was modified @@ -242,7 +242,6 @@ function copyRulesData (rulesData) { function clearAllRules () { waf.destroy() - blocking.updateBlockingConfiguration(undefined) defaultRules = undefined diff --git a/packages/dd-trace/src/appsec/sdk/user_blocking.js b/packages/dd-trace/src/appsec/sdk/user_blocking.js index 0c385b8202d..4ecbdec074b 100644 --- a/packages/dd-trace/src/appsec/sdk/user_blocking.js +++ b/packages/dd-trace/src/appsec/sdk/user_blocking.js @@ -13,7 +13,7 @@ function isUserBlocked (user) { if (!actions) return false - return actions.includes('block') + return Object.keys(actions).includes('block_request') || Object.keys(actions).includes('redirect_request') } function checkUserAndSetUser (tracer, user) { diff --git a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js index 7c7f99fba3e..d238017db67 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -60,7 +60,9 @@ class WAFContextWrapper { this.addressesToSkip = newAddressesToSkip const ruleTriggered = !!result.events?.length - const blockTriggered = result.actions?.includes('block') + + const blockTriggered = result.actions && (Object.keys(result.actions).includes('block_request') || + Object.keys(result.actions).includes('redirect_request')) Reporter.reportMetrics({ duration: result.totalRuntime / 1e3, diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 600ce92772c..0a4b2d0c217 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -149,18 +149,14 @@ describe('blocking', () => { } it('should block with default html template and custom status', () => { - updateBlockingConfiguration({ - id: 'block', - type: 'block_request', - parameters: { - status_code: 401, - type: 'auto' - } - }) + const actionParameters = { + status_code: 401, + type: 'auto' + } req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan) + block(req, res, rootSpan, null, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) @@ -168,18 +164,14 @@ describe('blocking', () => { it('should block with default json template and custom status ' + 'when type is forced to json and accept is html', () => { - updateBlockingConfiguration({ - id: 'block', - type: 'block_request', - parameters: { - status_code: 401, - type: 'json' - } - }) + const actionParameters = { + status_code: 401, + type: 'json' + } req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan) + block(req, res, rootSpan, null, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) @@ -187,35 +179,27 @@ describe('blocking', () => { it('should block with default html template and custom status ' + 'when type is forced to html and accept is html', () => { - updateBlockingConfiguration({ - id: 'block', - type: 'block_request', - parameters: { - status_code: 401, - type: 'html' - } - }) + const actionParameters = { + status_code: 401, + type: 'html' + } req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan) + block(req, res, rootSpan, null, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with default json template and custom status', () => { - updateBlockingConfiguration({ - id: 'block', - type: 'block_request', - parameters: { - status_code: 401, - type: 'auto' - } - }) + const actionParameters = { + status_code: 401, + type: 'auto' + } setTemplates(config) - block(req, res, rootSpan) + block(req, res, rootSpan, null, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) @@ -223,17 +207,13 @@ describe('blocking', () => { it('should block with default json template and custom status ' + 'when type is forced to json and accept is not defined', () => { - updateBlockingConfiguration({ - id: 'block', - type: 'block_request', - parameters: { - status_code: 401, - type: 'json' - } - }) + const actionParameters = { + status_code: 401, + type: 'json' + } setTemplates(config) - block(req, res, rootSpan) + block(req, res, rootSpan, null, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) @@ -241,34 +221,26 @@ describe('blocking', () => { it('should block with default html template and custom status ' + 'when type is forced to html and accept is not defined', () => { - updateBlockingConfiguration({ - id: 'block', - type: 'block_request', - parameters: { - status_code: 401, - type: 'html' - } - }) + const actionParameters = { + status_code: 401, + type: 'html' + } setTemplates(config) - block(req, res, rootSpan) + block(req, res, rootSpan, null, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with custom redirect', () => { - updateBlockingConfiguration({ - id: 'block', - type: 'redirect_request', - parameters: { - status_code: 301, - location: '/you-have-been-blocked' - } - }) + const actionParameters = { + status_code: 301, + location: '/you-have-been-blocked' + } setTemplates(config) - block(req, res, rootSpan) + block(req, res, rootSpan, null, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWithExactly(301, { Location: '/you-have-been-blocked' diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index febd54244a6..6f12e88871f 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -24,6 +24,14 @@ const { storage } = require('../../../datadog-core') const telemetryMetrics = require('../../src/telemetry/metrics') const addresses = require('../../src/appsec/addresses') +const resultActions = { + block_request: { + status_code: '401', + type: 'auto', + grpc_status_code: '10' + } +} + describe('AppSec Index', () => { let config let AppSec @@ -662,7 +670,7 @@ describe('AppSec Index', () => { it('Should block when it is detected as attack', () => { const body = { key: 'value' } req.body = body - sinon.stub(waf, 'run').returns(['block']) + sinon.stub(waf, 'run').returns(resultActions) bodyParser.publish({ req, res, body, abortController }) @@ -704,7 +712,7 @@ describe('AppSec Index', () => { it('Should block when it is detected as attack', () => { const cookies = { key: 'value' } - sinon.stub(waf, 'run').returns(['block']) + sinon.stub(waf, 'run').returns(resultActions) cookieParser.publish({ req, res, abortController, cookies }) @@ -748,7 +756,7 @@ describe('AppSec Index', () => { it('Should block when it is detected as attack', () => { const query = { key: 'value' } req.query = query - sinon.stub(waf, 'run').returns(['block']) + sinon.stub(waf, 'run').returns(resultActions) queryParser.publish({ req, res, query, abortController }) diff --git a/packages/dd-trace/test/appsec/rule_manager.spec.js b/packages/dd-trace/test/appsec/rule_manager.spec.js index e7daca53341..ca50617fbb2 100644 --- a/packages/dd-trace/test/appsec/rule_manager.spec.js +++ b/packages/dd-trace/test/appsec/rule_manager.spec.js @@ -20,8 +20,6 @@ describe('AppSec Rule Manager', () => { sinon.stub(waf, 'init').callThrough() sinon.stub(waf, 'destroy').callThrough() sinon.stub(waf, 'update').callThrough() - - sinon.stub(blocking, 'updateBlockingConfiguration').callThrough() }) afterEach(() => { @@ -34,22 +32,6 @@ describe('AppSec Rule Manager', () => { loadRules(config.appsec) expect(waf.init).to.have.been.calledOnceWithExactly(rules, config.appsec) - expect(blocking.updateBlockingConfiguration).not.to.have.been.called - }) - - 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.updateBlockingConfiguration).to.have.been.calledOnceWithExactly({ - id: 'block', - otherParam: 'other' - }) }) it('should throw if null/undefined are passed', () => { @@ -69,7 +51,6 @@ describe('AppSec Rule Manager', () => { clearAllRules() expect(waf.destroy).to.have.been.calledOnce - expect(blocking.updateBlockingConfiguration).to.have.been.calledOnceWithExactly(undefined) }) }) @@ -527,13 +508,7 @@ describe('AppSec Rule Manager', () => { ] updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) - - expect(waf.update).not.to.have.been.called - expect(blocking.updateBlockingConfiguration).to.have.been.calledOnceWithExactly( - { - id: 'block', - otherParam: 'other' - }) + expect(waf.update).to.have.been.calledOnceWithExactly(asm) }) it('should unapply blocking actions', () => { @@ -557,8 +532,6 @@ describe('AppSec Rule Manager', () => { } ] updateWafFromRC({ toUnapply: [], toApply, toModify: [] }) - // reset counters - blocking.updateBlockingConfiguration.reset() const toUnapply = [ { @@ -569,8 +542,7 @@ describe('AppSec Rule Manager', () => { updateWafFromRC({ toUnapply, toApply: [], toModify: [] }) - expect(waf.update).not.to.have.been.called - expect(blocking.updateBlockingConfiguration).to.have.been.calledOnceWithExactly(undefined) + expect(waf.update).to.have.been.calledOnceWithExactly(asm) }) it('should ignore other properties', () => { diff --git a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js index 3072b57122b..d7c51d2418d 100644 --- a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js +++ b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js @@ -11,6 +11,14 @@ const path = require('path') const waf = require('../../../src/appsec/waf') const { USER_ID } = require('../../../src/appsec/addresses') +const resultActions = { + block_request: { + status_code: '401', + type: 'auto', + grpc_status_code: '10' + } +} + describe('user_blocking', () => { describe('Internal API', () => { const req = { protocol: 'https' } @@ -21,8 +29,8 @@ describe('user_blocking', () => { before(() => { const runStub = sinon.stub(waf, 'run') - runStub.withArgs({ persistent: { [USER_ID]: 'user' } }).returns(['block']) - runStub.withArgs({ persistent: { [USER_ID]: 'gooduser' } }).returns(['']) + runStub.withArgs({ persistent: { [USER_ID]: 'user' } }).returns(resultActions) + runStub.withArgs({ persistent: { [USER_ID]: 'gooduser' } }).returns(undefined) }) beforeEach(() => {