From a2100ff53e2f05cad62fe7413edfe44ca9870a32 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 20 Feb 2025 14:20:48 +0100 Subject: [PATCH 01/56] update native appsec --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 28194e8557a..63244cc9a86 100644 --- a/package.json +++ b/package.json @@ -83,7 +83,7 @@ }, "dependencies": { "@datadog/libdatadog": "^0.4.0", - "@datadog/native-appsec": "8.4.0", + "@datadog/native-appsec": "8.5.0", "@datadog/native-iast-rewriter": "2.8.0", "@datadog/native-iast-taint-tracking": "3.3.0", "@datadog/native-metrics": "^3.1.0", diff --git a/yarn.lock b/yarn.lock index 218b3a7c05b..238de084d27 100644 --- a/yarn.lock +++ b/yarn.lock @@ -406,10 +406,10 @@ resolved "https://registry.yarnpkg.com/@datadog/libdatadog/-/libdatadog-0.4.0.tgz#aeeea02973f663b555ad9ac30c4015a31d561598" integrity sha512-kGZfFVmQInzt6J4FFGrqMbrDvOxqwk3WqhAreS6n9b/De+iMVy/NMu3V7uKsY5zAvz+uQw0liDJm3ZDVH/MVVw== -"@datadog/native-appsec@8.4.0": - version "8.4.0" - resolved "https://registry.yarnpkg.com/@datadog/native-appsec/-/native-appsec-8.4.0.tgz#5c44d949ff8f40a94c334554db79c1c470653bae" - integrity sha512-LC47AnpVLpQFEUOP/nIIs+i0wLb8XYO+et3ACaJlHa2YJM3asR4KZTqQjDQNy08PTAUbVvYWKwfSR1qVsU/BeA== +"@datadog/native-appsec@8.5.0": + version "8.5.0" + resolved "https://registry.yarnpkg.com/@datadog/native-appsec/-/native-appsec-8.5.0.tgz#cf4eea74a07085a0dc9f3e98c130736b38cd61c9" + integrity sha512-95y+fm7jd+3iknzuu57pWEPw9fcK9uSBCPiB4kSPHszHu3bESlZM553tc4ANsz+X3gMkYGVg2pgSydG77nSDJw== dependencies: node-gyp-build "^3.9.0" From 6a55c87357ec4e5915c449e8f6bfb96273639087 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 20 Feb 2025 14:21:41 +0100 Subject: [PATCH 02/56] report appsec block failed --- packages/dd-trace/src/appsec/blocking.js | 39 +++++++++++-------- .../dd-trace/test/appsec/blocking.spec.js | 12 +++++- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 733b982a811..e96f4f16d02 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -100,29 +100,36 @@ function getBlockingData (req, specificType, actionParameters) { } function block (req, res, rootSpan, abortController, actionParameters = defaultBlockingActionParameters) { - if (res.headersSent) { - log.warn('[ASM] Cannot send blocking response when headers have already been sent') - return - } + try { + if (res.headersSent) { + log.warn('[ASM] Cannot send blocking response when headers have already been sent') - const { body, headers, statusCode } = getBlockingData(req, null, actionParameters) + throw new Error('Headers have already been sent') + } - rootSpan.addTags({ - 'appsec.blocked': 'true' - }) + const { body, headers, statusCode } = getBlockingData(req, null, actionParameters) - for (const headerName of res.getHeaderNames()) { - res.removeHeader(headerName) - } + for (const headerName of res.getHeaderNames()) { + res.removeHeader(headerName) + } + + res.writeHead(statusCode, headers) - res.writeHead(statusCode, headers) + // this is needed to call the original end method, since express-session replaces it + res.constructor.prototype.end.call(res, body) - // this is needed to call the original end method, since express-session replaces it - res.constructor.prototype.end.call(res, body) + responseBlockedSet.add(res) - responseBlockedSet.add(res) + rootSpan.addTags({ + 'appsec.blocked': 'true' + }) - abortController?.abort() + abortController?.abort() + } catch (err) { + rootSpan.addTags({ + '_dd.appsec.block.failed': 1 + }) + } } function getBlockingAction (actions) { diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 1a809410694..2514dedbf7a 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -62,7 +62,7 @@ describe('blocking', () => { expect(log.warn).to.have.been .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') - expect(rootSpan.addTags).to.not.have.been.called + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ '_dd.appsec.block.failed': 1 }) expect(res.setHeader).to.not.have.been.called expect(res.constructor.prototype.end).to.not.have.been.called }) @@ -130,6 +130,16 @@ describe('blocking', () => { }) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly('jsonBody') }) + + it('should set block failed tag when express res end throws an error', () => { + res.constructor.prototype.end.throws(new Error('End failed')) + + block(req, res, rootSpan) + + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ '_dd.appsec.block.failed': 1 }) + expect(res.writeHead).to.have.been.calledOnce + expect(res.constructor.prototype.end).to.have.been.calledOnce + }) }) describe('block with default templates', () => { From bc78fc294102eabb93287a42406698d3cf6b54ea Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 20 Feb 2025 17:25:28 +0100 Subject: [PATCH 03/56] report truncation tags --- packages/dd-trace/src/appsec/blocking.js | 4 +- packages/dd-trace/src/appsec/reporter.js | 16 ++++++ .../src/appsec/waf/waf_context_wrapper.js | 24 +++++++- .../appsec/index.body-parser.plugin.spec.js | 41 ++++++++++++++ .../dd-trace/test/appsec/reporter.spec.js | 21 +++++++ .../appsec/waf/waf_context_wrapper.spec.js | 55 +++++++++++++++++++ 6 files changed, 156 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index e96f4f16d02..799d74efd07 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -120,11 +120,11 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB responseBlockedSet.add(res) + abortController.abort() + rootSpan.addTags({ 'appsec.blocked': 'true' }) - - abortController?.abort() } catch (err) { rootSpan.addTags({ '_dd.appsec.block.failed': 1 diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 8f16a1a513a..a1628fd109c 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -114,6 +114,22 @@ function reportMetrics (metrics, raspRule) { } else { updateWafRequestsMetricTags(metrics, store.req) } + + reportTruncationMetrics(rootSpan, metrics) +} + +function reportTruncationMetrics (rootSpan, metrics) { + if (metrics.maxTruncatedString) { + rootSpan.setTag('_dd.appsec.truncated.string_length', metrics.maxTruncatedString) + } + + if (metrics.maxTruncatedContainerSize) { + rootSpan.setTag('_dd.appsec.truncated.container_size', metrics.maxTruncatedContainerSize) + } + + if (metrics.maxTruncatedContainerDepth) { + rootSpan.setTag('_dd.appsec.truncated.container_depth', metrics.maxTruncatedContainerDepth) + } } function reportAttack (attackData) { 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 1561bd1d0d0..1a6a6979a06 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -17,8 +17,8 @@ class WAFContextWrapper { this.wafTimeout = wafTimeout this.wafVersion = wafVersion this.rulesVersion = rulesVersion - this.addressesToSkip = new Set() this.knownAddresses = knownAddresses + this.addressesToSkip = new Set() this.cachedUserIdActions = new Map() } @@ -80,8 +80,27 @@ class WAFContextWrapper { try { const start = process.hrtime.bigint() + const metrics = { + rulesVersion: this.rulesVersion, + wafVersion: this.wafVersion + } + const result = this.ddwafContext.run(payload, this.wafTimeout) + if (result) { + if (result.metrics?.maxTruncatedString) { + metrics.maxTruncatedString = result.metrics.maxTruncatedString + } + + if (result.metrics?.maxTruncatedContainerSize) { + metrics.maxTruncatedContainerSize = result.metrics.maxTruncatedContainerSize + } + + if (result.metrics?.maxTruncatedContainerDepth) { + metrics.maxTruncatedContainerDepth = result.metrics.maxTruncatedContainerDepth + } + } + const end = process.hrtime.bigint() this.addressesToSkip = newAddressesToSkip @@ -97,12 +116,11 @@ class WAFContextWrapper { } Reporter.reportMetrics({ + ...metrics, duration: result.totalRuntime / 1e3, durationExt: parseInt(end - start) / 1e3, - rulesVersion: this.rulesVersion, ruleTriggered, blockTriggered, - wafVersion: this.wafVersion, wafTimeout: result.timeout }, raspRule) diff --git a/packages/dd-trace/test/appsec/index.body-parser.plugin.spec.js b/packages/dd-trace/test/appsec/index.body-parser.plugin.spec.js index 458a69ee97d..f418f5562e1 100644 --- a/packages/dd-trace/test/appsec/index.body-parser.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.body-parser.plugin.spec.js @@ -64,5 +64,46 @@ withVersions('body-parser', 'body-parser', version => { expect(requestBody).not.to.be.called } }) + + it('should block the request when attack is detected and report truncation metrics', async () => { + try { + const longValue = 'testattack'.repeat(500) + + const largeObject = {} + for (let i = 0; i < 300; ++i) { + largeObject[`key${i}`] = `value${i}` + } + + const deepObject = createNestedObject(25, { value: 'a' }) + + const complexPayload = { + deepObject, + longValue, + largeObject + } + + await axios.post(`http://localhost:${port}/`, { complexPayload }) + + return Promise.reject(new Error('Request should not return 200')) + } catch (e) { + expect(e.response.status).to.be.equals(403) + expect(e.response.data).to.be.deep.equal(JSON.parse(json)) + expect(requestBody).not.to.be.called + + await agent.use((traces) => { + const span = traces[0][0] + expect(span.metrics['_dd.appsec.truncated.string_length']).to.equal(5000) + expect(span.metrics['_dd.appsec.truncated.container_size']).to.equal(300) + expect(span.metrics['_dd.appsec.truncated.container_depth']).to.equal(20) + }) + } + }) }) }) + +const createNestedObject = (n, obj) => { + if (n > 0) { + return { a: createNestedObject(n - 1, obj) } + } + return obj +} diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index 08ad31d2fb8..ac8dc25929c 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -182,6 +182,27 @@ describe('reporter', () => { expect(telemetry.updateRaspRequestsMetricTags).to.not.have.been.called }) + it('should set max truncation string length metric if set', () => { + Reporter.reportMetrics({ maxTruncatedString: 300 }) + + expect(web.root).to.have.been.calledOnceWithExactly(req) + expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.truncated.string_length', 300) + }) + + it('should set max truncation container size metric if set', () => { + Reporter.reportMetrics({ maxTruncatedContainerSize: 200 }) + + expect(web.root).to.have.been.calledOnceWithExactly(req) + expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.truncated.container_size', 200) + }) + + it('should set max truncation container depth metric if set', () => { + Reporter.reportMetrics({ maxTruncatedContainerDepth: 100 }) + + expect(web.root).to.have.been.calledOnceWithExactly(req) + expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.truncated.container_depth', 100) + }) + it('should call updateWafRequestsMetricTags', () => { const metrics = { rulesVersion: '1.2.3' } const store = storage('legacy').getStore() diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index 436f6c093d4..b168734559e 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -154,4 +154,59 @@ describe('WAFContextWrapper', () => { sinon.assert.calledOnceWithExactly(log.warn, '[ASM] Calling run on a disposed context') }) }) + + describe('Truncation handling', () => { + let ddwafContext, wafContextWrapper, Reporter, WAFContextWrapper + + beforeEach(() => { + ddwafContext = { + run: sinon.stub(), + disposed: false + } + + Reporter = { + reportMetrics: sinon.stub(), + reportAttack: sinon.stub(), + reportDerivatives: sinon.stub() + } + + WAFContextWrapper = proxyquire('../../../src/appsec/waf/waf_context_wrapper', { + '../reporter': Reporter + }) + + wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) + }) + + it('Should report truncations', () => { + const payload = { + persistent: { + [addresses.HTTP_INCOMING_QUERY]: { + value: 'test' + } + } + } + + const wafResult = { + metrics: { + maxTruncatedString: 6000, + maxTruncatedContainerSize: 400, + maxTruncatedContainerDepth: 20 + }, + totalRuntime: 100, + events: [], + actions: [], + timeout: false, + derivatives: {} + } + ddwafContext.run.returns(wafResult) + + wafContextWrapper.run(payload) + const reportedMetricsCall = Reporter.reportMetrics.firstCall + expect(reportedMetricsCall.args[0]).to.include({ + maxTruncatedString: 6000, + maxTruncatedContainerSize: 400, + maxTruncatedContainerDepth: 20 + }) + }) + }) }) From 4580619011a9f86c2b371bb63375ab5be1203d94 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 12:34:24 +0100 Subject: [PATCH 04/56] add error blocking log --- packages/dd-trace/src/appsec/blocking.js | 2 ++ .../src/appsec/waf/waf_context_wrapper.js | 18 ++++++++---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 799d74efd07..792eb0b8438 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -129,6 +129,8 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB rootSpan.addTags({ '_dd.appsec.block.failed': 1 }) + + log.error('[ASM] Blocking error', err) } } 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 1a6a6979a06..1b77fe1d407 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -87,18 +87,16 @@ class WAFContextWrapper { const result = this.ddwafContext.run(payload, this.wafTimeout) - if (result) { - if (result.metrics?.maxTruncatedString) { - metrics.maxTruncatedString = result.metrics.maxTruncatedString - } + if (result?.metrics?.maxTruncatedString) { + metrics.maxTruncatedString = result.metrics.maxTruncatedString + } - if (result.metrics?.maxTruncatedContainerSize) { - metrics.maxTruncatedContainerSize = result.metrics.maxTruncatedContainerSize - } + if (result?.metrics?.maxTruncatedContainerSize) { + metrics.maxTruncatedContainerSize = result.metrics.maxTruncatedContainerSize + } - if (result.metrics?.maxTruncatedContainerDepth) { - metrics.maxTruncatedContainerDepth = result.metrics.maxTruncatedContainerDepth - } + if (result?.metrics?.maxTruncatedContainerDepth) { + metrics.maxTruncatedContainerDepth = result.metrics.maxTruncatedContainerDepth } const end = process.hrtime.bigint() From 4fd5bcf70c600dfb7e7a4e069d689d9823cd9d89 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 14:01:25 +0100 Subject: [PATCH 05/56] failed graphql blocking tag --- packages/dd-trace/src/appsec/graphql.js | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index 3d2603c0e33..a61fc89035d 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -7,6 +7,7 @@ const { getBlockingData, getBlockingAction } = require('./blocking') +const log = require('../log') const waf = require('./waf') const addresses = require('./addresses') const web = require('../plugins/util/web') @@ -94,14 +95,22 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { const rootSpan = web.root(req) if (!rootSpan) return - const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, requestData.wafAction) - abortData.statusCode = blockingData.statusCode - abortData.headers = blockingData.headers - abortData.message = blockingData.body + try { + 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() - abortController?.abort() + rootSpan.setTag('appsec.blocked', 'true') + } catch (err) { + rootSpan.addTags({ + '_dd.appsec.block.failed': 1 + }) + + log.error('[ASM] Blocking error', err) + } } graphqlRequestData.delete(req) From 747211f8b49bf4fa43f4256c5a29e6e1eb2ab221 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 18:59:04 +0100 Subject: [PATCH 06/56] report after block --- packages/dd-trace/src/appsec/blocking.js | 38 +++++++------------ packages/dd-trace/src/appsec/index.js | 36 +++++++++++++++--- .../src/appsec/rasp/command_injection.js | 2 +- packages/dd-trace/src/appsec/rasp/lfi.js | 2 +- .../dd-trace/src/appsec/rasp/sql_injection.js | 2 +- packages/dd-trace/src/appsec/rasp/ssrf.js | 2 +- packages/dd-trace/src/appsec/rasp/utils.js | 20 ++++++++-- .../src/appsec/waf/waf_context_wrapper.js | 22 +++-------- 8 files changed, 69 insertions(+), 55 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 792eb0b8438..5598bb89255 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -100,38 +100,26 @@ function getBlockingData (req, specificType, actionParameters) { } function block (req, res, rootSpan, abortController, actionParameters = defaultBlockingActionParameters) { - try { - if (res.headersSent) { - log.warn('[ASM] Cannot send blocking response when headers have already been sent') + if (res.headersSent) { + log.warn('[ASM] Cannot send blocking response when headers have already been sent') - throw new Error('Headers have already been sent') - } - - const { body, headers, statusCode } = getBlockingData(req, null, actionParameters) - - for (const headerName of res.getHeaderNames()) { - res.removeHeader(headerName) - } + throw new Error('Headers have already been sent') + } - res.writeHead(statusCode, headers) + const { body, headers, statusCode } = getBlockingData(req, null, actionParameters) - // this is needed to call the original end method, since express-session replaces it - res.constructor.prototype.end.call(res, body) + for (const headerName of res.getHeaderNames()) { + res.removeHeader(headerName) + } - responseBlockedSet.add(res) + res.writeHead(statusCode, headers) - abortController.abort() + // this is needed to call the original end method, since express-session replaces it + res.constructor.prototype.end.call(res, body) - rootSpan.addTags({ - 'appsec.blocked': 'true' - }) - } catch (err) { - rootSpan.addTags({ - '_dd.appsec.block.failed': 1 - }) + responseBlockedSet.add(res) - log.error('[ASM] Blocking error', err) - } + abortController.abort() } function getBlockingAction (actions) { diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index cc7eef68008..8fb14faa1df 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -34,6 +34,7 @@ const UserTracking = require('./user_tracking') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') const rasp = require('./rasp') +const { reportMetrics } = require('./reporter') const responseAnalyzedSet = new WeakSet() @@ -317,13 +318,38 @@ function onResponseSetHeader ({ res, abortController }) { } } -function handleResults (actions, req, res, rootSpan, abortController) { - if (!actions || !req || !res || !rootSpan || !abortController) return +function handleResults (wafResults, req, res, rootSpan, abortController) { + const { metrics, durationExt, result } = wafResults - const blockingAction = getBlockingAction(actions) - if (blockingAction) { - block(req, res, rootSpan, abortController, blockingAction) + const ruleTriggered = !!result.events?.length + const blockTriggered = getBlockingAction(wafResults?.actions) + + const canBlock = wafResults?.actions && req && res && rootSpan && abortController + + if (canBlock && blockTriggered) { + try { + block(req, res, rootSpan, abortController, blockTriggered) + + rootSpan.addTags({ + 'appsec.blocked': 'true' + }) + } catch (err) { + rootSpan.addTags({ + '_dd.appsec.block.failed': 1 + }) + + log.error('[ASM] Blocking error', err) + } } + + reportMetrics({ + ...metrics, + durationExt, + duration: result.totalRuntime / 1e3, + ruleTriggered, + blockTriggered, + wafTimeout: result.timeout + }, null) } function disable () { diff --git a/packages/dd-trace/src/appsec/rasp/command_injection.js b/packages/dd-trace/src/appsec/rasp/command_injection.js index eea0af5d22d..d403fcf36d4 100644 --- a/packages/dd-trace/src/appsec/rasp/command_injection.js +++ b/packages/dd-trace/src/appsec/rasp/command_injection.js @@ -47,7 +47,7 @@ function analyzeCommandInjection ({ file, fileArgs, shell, abortController }) { const result = waf.run({ ephemeral }, req, raspRule) const res = store?.res - handleResult(result, req, res, abortController, config) + handleResult(result, req, res, abortController, config, raspRule) } module.exports = { diff --git a/packages/dd-trace/src/appsec/rasp/lfi.js b/packages/dd-trace/src/appsec/rasp/lfi.js index 87c82175ac1..4065638de9c 100644 --- a/packages/dd-trace/src/appsec/rasp/lfi.js +++ b/packages/dd-trace/src/appsec/rasp/lfi.js @@ -61,7 +61,7 @@ function analyzeLfi (ctx) { const raspRule = { type: RULE_TYPES.LFI } const result = waf.run({ ephemeral }, req, raspRule) - handleResult(result, req, res, ctx.abortController, config) + handleResult(result, req, res, ctx.abortController, config, raspRule) }) } diff --git a/packages/dd-trace/src/appsec/rasp/sql_injection.js b/packages/dd-trace/src/appsec/rasp/sql_injection.js index 8da179bcfe8..ccafb300193 100644 --- a/packages/dd-trace/src/appsec/rasp/sql_injection.js +++ b/packages/dd-trace/src/appsec/rasp/sql_injection.js @@ -76,7 +76,7 @@ function analyzeSqlInjection (query, dbSystem, abortController) { const result = waf.run({ ephemeral }, req, raspRule) - handleResult(result, req, res, abortController, config) + handleResult(result, req, res, abortController, config, raspRule) } function hasInputAddress (payload) { diff --git a/packages/dd-trace/src/appsec/rasp/ssrf.js b/packages/dd-trace/src/appsec/rasp/ssrf.js index d0d75f16c60..90128fb4799 100644 --- a/packages/dd-trace/src/appsec/rasp/ssrf.js +++ b/packages/dd-trace/src/appsec/rasp/ssrf.js @@ -34,7 +34,7 @@ function analyzeSsrf (ctx) { const result = waf.run({ ephemeral }, req, raspRule) const res = store?.res - handleResult(result, req, res, ctx.abortController, config) + handleResult(result, req, res, ctx.abortController, config, raspRule) } module.exports = { enable, disable } diff --git a/packages/dd-trace/src/appsec/rasp/utils.js b/packages/dd-trace/src/appsec/rasp/utils.js index 17875c48c7b..7d5c23aece7 100644 --- a/packages/dd-trace/src/appsec/rasp/utils.js +++ b/packages/dd-trace/src/appsec/rasp/utils.js @@ -3,6 +3,7 @@ const web = require('../../plugins/util/web') const { getCallsiteFrames, reportStackTrace, canReportStackTrace } = require('../stack_trace') const { getBlockingAction } = require('../blocking') +const { reportMetrics } = require('../reporter') const log = require('../../log') const abortOnUncaughtException = process.execArgv?.includes('--abort-on-uncaught-exception') @@ -28,8 +29,9 @@ class DatadogRaspAbortError extends Error { } } -function handleResult (actions, req, res, abortController, config) { - const generateStackTraceAction = actions?.generate_stack +function handleResult (wafResults, req, res, abortController, config, raspRule) { + const { result, metrics, durationExt } = wafResults + const generateStackTraceAction = result.actions?.generate_stack const { enabled, maxDepth, maxStackTraces } = config.appsec.stackTrace @@ -45,11 +47,21 @@ function handleResult (actions, req, res, abortController, config) { ) } + const ruleTriggered = !!result?.events?.length + const blockingAction = getBlockingAction(result?.actions) + + reportMetrics({ + ...metrics, + durationExt, + duration: result.totalRuntime / 1e3, + ruleTriggered, + blockingAction, + wafTimeout: result.timeout + }, raspRule) + if (!abortController || abortOnUncaughtException) return - const blockingAction = getBlockingAction(actions) if (blockingAction) { - const rootSpan = web.root(req) // Should block only in express if (rootSpan?.context()._name === 'express.request') { const abortError = new DatadogRaspAbortError(req, res, blockingAction) 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 1b77fe1d407..bcf266fd052 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -1,7 +1,6 @@ 'use strict' const log = require('../../log') -const Reporter = require('../reporter') const addresses = require('../addresses') const { getBlockingAction } = require('../blocking') const { wafRunFinished } = require('../channels') @@ -113,26 +112,15 @@ class WAFContextWrapper { this.setUserIdCache(userId, result) } - Reporter.reportMetrics({ - ...metrics, - duration: result.totalRuntime / 1e3, - durationExt: parseInt(end - start) / 1e3, - ruleTriggered, - blockTriggered, - wafTimeout: result.timeout - }, raspRule) - - if (ruleTriggered) { - Reporter.reportAttack(JSON.stringify(result.events)) - } - - Reporter.reportDerivatives(result.derivatives) - if (wafRunFinished.hasSubscribers) { wafRunFinished.publish({ payload }) } - return result.actions + return { + result, + metrics, + durationExt: parseInt(end - start) / 1e3 + } } catch (err) { log.error('[ASM] Error while running the AppSec WAF', err) } From 57e7a4ae3885d67eee19dff25ed15698bcfed8fc Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 19:05:54 +0100 Subject: [PATCH 07/56] add waf timeout span tag --- packages/dd-trace/src/appsec/reporter.js | 5 +++++ packages/dd-trace/src/appsec/telemetry/index.js | 3 ++- packages/dd-trace/src/appsec/telemetry/waf.js | 6 +++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index a1628fd109c..8e5b33b5f16 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -210,6 +210,7 @@ function finishRequest (req, res) { } const metrics = getRequestMetrics(req) + if (metrics?.duration) { rootSpan.setTag('_dd.appsec.waf.duration', metrics.duration) } @@ -218,6 +219,10 @@ function finishRequest (req, res) { rootSpan.setTag('_dd.appsec.waf.duration_ext', metrics.durationExt) } + if (metrics?.wafTimeouts) { + rootSpan.setTag('_dd.appsec.waf.timeouts', metrics.wafTimeouts) + } + if (metrics?.raspDuration) { rootSpan.setTag('_dd.appsec.rasp.duration', metrics.raspDuration) } diff --git a/packages/dd-trace/src/appsec/telemetry/index.js b/packages/dd-trace/src/appsec/telemetry/index.js index 7dbf52799fc..4ad857688c8 100644 --- a/packages/dd-trace/src/appsec/telemetry/index.js +++ b/packages/dd-trace/src/appsec/telemetry/index.js @@ -30,7 +30,8 @@ function newStore () { durationExt: 0, raspDuration: 0, raspDurationExt: 0, - raspEvalCount: 0 + raspEvalCount: 0, + wafTimeouts: 0 } } } diff --git a/packages/dd-trace/src/appsec/telemetry/waf.js b/packages/dd-trace/src/appsec/telemetry/waf.js index c19b41a48c7..055448a819b 100644 --- a/packages/dd-trace/src/appsec/telemetry/waf.js +++ b/packages/dd-trace/src/appsec/telemetry/waf.js @@ -7,9 +7,13 @@ const appsecMetrics = telemetryMetrics.manager.namespace('appsec') const DD_TELEMETRY_WAF_RESULT_TAGS = Symbol('_dd.appsec.telemetry.waf.result.tags') -function addWafRequestMetrics (store, { duration, durationExt }) { +function addWafRequestMetrics (store, { duration, durationExt, wafTimeout }) { store[DD_TELEMETRY_REQUEST_METRICS].duration += duration || 0 store[DD_TELEMETRY_REQUEST_METRICS].durationExt += durationExt || 0 + + if (wafTimeout) { + store[DD_TELEMETRY_REQUEST_METRICS].wafTimeouts++ + } } function trackWafDurations ({ duration, durationExt }, versionsTags) { From b182a4158f51429f26030fef027c5f2d5c727d86 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 19:08:19 +0100 Subject: [PATCH 08/56] add rasp timeout span tag --- packages/dd-trace/src/appsec/reporter.js | 4 ++++ packages/dd-trace/src/appsec/telemetry/index.js | 3 ++- packages/dd-trace/src/appsec/telemetry/rasp.js | 6 +++++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 8e5b33b5f16..aa93c287842 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -235,6 +235,10 @@ function finishRequest (req, res) { rootSpan.setTag('_dd.appsec.rasp.rule.eval', metrics.raspEvalCount) } + if (metrics?.raspTimeouts) { + rootSpan.setTag('_dd.appsec.rasp.timeout', metrics.raspTimeouts) + } + incrementWafRequestsMetric(req) // collect some headers even when no attack is detected diff --git a/packages/dd-trace/src/appsec/telemetry/index.js b/packages/dd-trace/src/appsec/telemetry/index.js index 4ad857688c8..e05452bb905 100644 --- a/packages/dd-trace/src/appsec/telemetry/index.js +++ b/packages/dd-trace/src/appsec/telemetry/index.js @@ -31,7 +31,8 @@ function newStore () { raspDuration: 0, raspDurationExt: 0, raspEvalCount: 0, - wafTimeouts: 0 + wafTimeouts: 0, + raspTimeouts: 0 } } } diff --git a/packages/dd-trace/src/appsec/telemetry/rasp.js b/packages/dd-trace/src/appsec/telemetry/rasp.js index 5d7f0c5e632..639542c8b11 100644 --- a/packages/dd-trace/src/appsec/telemetry/rasp.js +++ b/packages/dd-trace/src/appsec/telemetry/rasp.js @@ -5,10 +5,14 @@ const { DD_TELEMETRY_REQUEST_METRICS } = require('./common') const appsecMetrics = telemetryMetrics.manager.namespace('appsec') -function addRaspRequestMetrics (store, { duration, durationExt }) { +function addRaspRequestMetrics (store, { duration, durationExt, wafTimeout }) { store[DD_TELEMETRY_REQUEST_METRICS].raspDuration += duration || 0 store[DD_TELEMETRY_REQUEST_METRICS].raspDurationExt += durationExt || 0 store[DD_TELEMETRY_REQUEST_METRICS].raspEvalCount++ + + if (wafTimeout) { + store[DD_TELEMETRY_REQUEST_METRICS].raspTimeouts++ + } } function trackRaspMetrics (metrics, raspRule) { From 1631647193640f7a08a1780188106ef04215b8d7 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 19:18:42 +0100 Subject: [PATCH 09/56] add waf error tag --- packages/dd-trace/src/appsec/reporter.js | 4 ++ .../dd-trace/src/appsec/telemetry/index.js | 3 +- packages/dd-trace/src/appsec/telemetry/waf.js | 9 ++- .../src/appsec/waf/waf_context_wrapper.js | 71 ++++++++++++------- 4 files changed, 58 insertions(+), 29 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index aa93c287842..2b1d849b16b 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -219,6 +219,10 @@ function finishRequest (req, res) { rootSpan.setTag('_dd.appsec.waf.duration_ext', metrics.durationExt) } + if (metrics?.wafErrorCode) { + rootSpan.setTag('_dd.appsec.waf.error', metrics.wafErrorCode) + } + if (metrics?.wafTimeouts) { rootSpan.setTag('_dd.appsec.waf.timeouts', metrics.wafTimeouts) } diff --git a/packages/dd-trace/src/appsec/telemetry/index.js b/packages/dd-trace/src/appsec/telemetry/index.js index e05452bb905..3f0128fc161 100644 --- a/packages/dd-trace/src/appsec/telemetry/index.js +++ b/packages/dd-trace/src/appsec/telemetry/index.js @@ -32,7 +32,8 @@ function newStore () { raspDurationExt: 0, raspEvalCount: 0, wafTimeouts: 0, - raspTimeouts: 0 + raspTimeouts: 0, + wafErrorCode: null } } } diff --git a/packages/dd-trace/src/appsec/telemetry/waf.js b/packages/dd-trace/src/appsec/telemetry/waf.js index 055448a819b..4cbf0fa0946 100644 --- a/packages/dd-trace/src/appsec/telemetry/waf.js +++ b/packages/dd-trace/src/appsec/telemetry/waf.js @@ -7,13 +7,20 @@ const appsecMetrics = telemetryMetrics.manager.namespace('appsec') const DD_TELEMETRY_WAF_RESULT_TAGS = Symbol('_dd.appsec.telemetry.waf.result.tags') -function addWafRequestMetrics (store, { duration, durationExt, wafTimeout }) { +function addWafRequestMetrics (store, { duration, durationExt, wafTimeout, errorCode }) { store[DD_TELEMETRY_REQUEST_METRICS].duration += duration || 0 store[DD_TELEMETRY_REQUEST_METRICS].durationExt += durationExt || 0 if (wafTimeout) { store[DD_TELEMETRY_REQUEST_METRICS].wafTimeouts++ } + + if (errorCode) { + store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode = Math.max( + errorCode, + store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode ?? errorCode + ) + } } function trackWafDurations ({ duration, durationExt }, versionsTags) { 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 bcf266fd052..b64ce33e418 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -76,53 +76,70 @@ class WAFContextWrapper { if (!payloadHasData) return - try { - const start = process.hrtime.bigint() + const start = process.hrtime.bigint() - const metrics = { - rulesVersion: this.rulesVersion, - wafVersion: this.wafVersion - } + const metrics = { + rulesVersion: this.rulesVersion, + wafVersion: this.wafVersion + } - const result = this.ddwafContext.run(payload, this.wafTimeout) + const result = this.ddwafContext.run(payload, this.wafTimeout) + + if (!result) { + // Binding or other waf unexpected errors + metrics.errorCode = -127 + } else { + if (typeof result.errorCode === 'number' && result.errorCode < 0) { + metrics.errorCode = result.errorCode + } - if (result?.metrics?.maxTruncatedString) { + if (result.metrics?.maxTruncatedString) { metrics.maxTruncatedString = result.metrics.maxTruncatedString } - if (result?.metrics?.maxTruncatedContainerSize) { + if (result.metrics?.maxTruncatedContainerSize) { metrics.maxTruncatedContainerSize = result.metrics.maxTruncatedContainerSize } - if (result?.metrics?.maxTruncatedContainerDepth) { + if (result.metrics?.maxTruncatedContainerDepth) { metrics.maxTruncatedContainerDepth = result.metrics.maxTruncatedContainerDepth } + } - const end = process.hrtime.bigint() + const end = process.hrtime.bigint() - this.addressesToSkip = newAddressesToSkip + this.addressesToSkip = newAddressesToSkip - const ruleTriggered = !!result.events?.length + const ruleTriggered = !!result?.events?.length - const blockTriggered = !!getBlockingAction(result.actions) + const blockTriggered = !!getBlockingAction(result?.actions) - // SPECIAL CASE FOR USER_ID - // TODO: make this universal - if (userId && ruleTriggered && blockTriggered) { - this.setUserIdCache(userId, result) - } + // SPECIAL CASE FOR USER_ID + // TODO: make this universal + if (userId && ruleTriggered && blockTriggered) { + this.setUserIdCache(userId, result) + } - if (wafRunFinished.hasSubscribers) { - wafRunFinished.publish({ payload }) - } + if (wafRunFinished.hasSubscribers) { + wafRunFinished.publish({ payload }) + } - return { - result, - metrics, - durationExt: parseInt(end - start) / 1e3 - } + return { + result, + metrics, + durationExt: parseInt(end - start) / 1e3 + } + } + + runWaf (payload) { + try { + const result = this.ddwafContext.run(payload, this.wafTimeout) + + return result } catch (err) { log.error('[ASM] Error while running the AppSec WAF', err) + + return null } } From bfd0e2c81b8473fbf235386f05e3bf28a4eebcfe Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 19:20:58 +0100 Subject: [PATCH 10/56] add rasp error tag --- packages/dd-trace/src/appsec/reporter.js | 4 ++++ packages/dd-trace/src/appsec/telemetry/index.js | 3 ++- packages/dd-trace/src/appsec/telemetry/rasp.js | 9 ++++++++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 2b1d849b16b..07ec65ff73a 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -243,6 +243,10 @@ function finishRequest (req, res) { rootSpan.setTag('_dd.appsec.rasp.timeout', metrics.raspTimeouts) } + if (metrics?.raspErrorCode) { + rootSpan.setTag('_dd.appsec.rasp.error', metrics.raspErrorCode) + } + incrementWafRequestsMetric(req) // collect some headers even when no attack is detected diff --git a/packages/dd-trace/src/appsec/telemetry/index.js b/packages/dd-trace/src/appsec/telemetry/index.js index 3f0128fc161..1a0141c2068 100644 --- a/packages/dd-trace/src/appsec/telemetry/index.js +++ b/packages/dd-trace/src/appsec/telemetry/index.js @@ -33,7 +33,8 @@ function newStore () { raspEvalCount: 0, wafTimeouts: 0, raspTimeouts: 0, - wafErrorCode: null + wafErrorCode: null, + raspErrorCode: null } } } diff --git a/packages/dd-trace/src/appsec/telemetry/rasp.js b/packages/dd-trace/src/appsec/telemetry/rasp.js index 639542c8b11..e6fe008a26e 100644 --- a/packages/dd-trace/src/appsec/telemetry/rasp.js +++ b/packages/dd-trace/src/appsec/telemetry/rasp.js @@ -5,7 +5,7 @@ const { DD_TELEMETRY_REQUEST_METRICS } = require('./common') const appsecMetrics = telemetryMetrics.manager.namespace('appsec') -function addRaspRequestMetrics (store, { duration, durationExt, wafTimeout }) { +function addRaspRequestMetrics (store, { duration, durationExt, wafTimeout, errorCode }) { store[DD_TELEMETRY_REQUEST_METRICS].raspDuration += duration || 0 store[DD_TELEMETRY_REQUEST_METRICS].raspDurationExt += durationExt || 0 store[DD_TELEMETRY_REQUEST_METRICS].raspEvalCount++ @@ -13,6 +13,13 @@ function addRaspRequestMetrics (store, { duration, durationExt, wafTimeout }) { if (wafTimeout) { store[DD_TELEMETRY_REQUEST_METRICS].raspTimeouts++ } + + if (errorCode) { + store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode = Math.max( + errorCode, + store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode ?? errorCode + ) + } } function trackRaspMetrics (metrics, raspRule) { From bc3f2458efb66fcd427dc1308dd69b0c803e7b5d Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 19:25:57 +0100 Subject: [PATCH 11/56] remove raspRule from waf run call --- packages/dd-trace/src/appsec/rasp/command_injection.js | 2 +- packages/dd-trace/src/appsec/rasp/lfi.js | 2 +- packages/dd-trace/src/appsec/rasp/sql_injection.js | 2 +- packages/dd-trace/src/appsec/rasp/ssrf.js | 2 +- packages/dd-trace/src/appsec/waf/index.js | 4 ++-- packages/dd-trace/src/appsec/waf/waf_context_wrapper.js | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp/command_injection.js b/packages/dd-trace/src/appsec/rasp/command_injection.js index d403fcf36d4..47139a3bce4 100644 --- a/packages/dd-trace/src/appsec/rasp/command_injection.js +++ b/packages/dd-trace/src/appsec/rasp/command_injection.js @@ -44,7 +44,7 @@ function analyzeCommandInjection ({ file, fileArgs, shell, abortController }) { raspRule.variant = 'exec' } - const result = waf.run({ ephemeral }, req, raspRule) + const result = waf.run({ ephemeral }, req) const res = store?.res handleResult(result, req, res, abortController, config, raspRule) diff --git a/packages/dd-trace/src/appsec/rasp/lfi.js b/packages/dd-trace/src/appsec/rasp/lfi.js index 4065638de9c..867ae03a8f5 100644 --- a/packages/dd-trace/src/appsec/rasp/lfi.js +++ b/packages/dd-trace/src/appsec/rasp/lfi.js @@ -60,7 +60,7 @@ function analyzeLfi (ctx) { const raspRule = { type: RULE_TYPES.LFI } - const result = waf.run({ ephemeral }, req, raspRule) + const result = waf.run({ ephemeral }, req) handleResult(result, req, res, ctx.abortController, config, raspRule) }) } diff --git a/packages/dd-trace/src/appsec/rasp/sql_injection.js b/packages/dd-trace/src/appsec/rasp/sql_injection.js index ccafb300193..7c6ea8275ec 100644 --- a/packages/dd-trace/src/appsec/rasp/sql_injection.js +++ b/packages/dd-trace/src/appsec/rasp/sql_injection.js @@ -74,7 +74,7 @@ function analyzeSqlInjection (query, dbSystem, abortController) { const raspRule = { type: RULE_TYPES.SQL_INJECTION } - const result = waf.run({ ephemeral }, req, raspRule) + const result = waf.run({ ephemeral }, req) handleResult(result, req, res, abortController, config, raspRule) } diff --git a/packages/dd-trace/src/appsec/rasp/ssrf.js b/packages/dd-trace/src/appsec/rasp/ssrf.js index 90128fb4799..d24b857f526 100644 --- a/packages/dd-trace/src/appsec/rasp/ssrf.js +++ b/packages/dd-trace/src/appsec/rasp/ssrf.js @@ -31,7 +31,7 @@ function analyzeSsrf (ctx) { const raspRule = { type: RULE_TYPES.SSRF } - const result = waf.run({ ephemeral }, req, raspRule) + const result = waf.run({ ephemeral }, req) const res = store?.res handleResult(result, req, res, ctx.abortController, config, raspRule) diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index b025a123f46..596cbb9a17e 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -46,7 +46,7 @@ function update (newRules) { } } -function run (data, req, raspRule) { +function run (data, req) { if (!req) { const store = storage('legacy').getStore() if (!store || !store.req) { @@ -59,7 +59,7 @@ function run (data, req, raspRule) { const wafContext = waf.wafManager.getWAFContext(req) - return wafContext.run(data, raspRule) + return wafContext.run(data) } function disposeContext (req) { 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 b64ce33e418..6f1d3232764 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -21,7 +21,7 @@ class WAFContextWrapper { this.cachedUserIdActions = new Map() } - run ({ persistent, ephemeral }, raspRule) { + run ({ persistent, ephemeral }) { if (this.ddwafContext.disposed) { log.warn('[ASM] Calling run on a disposed context') return From 21a301abc6e7cdcbde737604b0eb2183e435ecc1 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Feb 2025 19:52:21 +0100 Subject: [PATCH 12/56] add waf wrapper tests --- .../src/appsec/waf/waf_context_wrapper.js | 7 ++ .../dd-trace/test/appsec/waf/index.spec.js | 92 ++++--------------- 2 files changed, 23 insertions(+), 76 deletions(-) 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 6f1d3232764..046917ba355 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -4,6 +4,7 @@ const log = require('../../log') const addresses = require('../addresses') const { getBlockingAction } = require('../blocking') const { wafRunFinished } = require('../channels') +const Reporter = require('../reporter') // TODO: remove once ephemeral addresses are implemented const preventDuplicateAddresses = new Set([ @@ -124,6 +125,12 @@ class WAFContextWrapper { wafRunFinished.publish({ payload }) } + if (ruleTriggered) { + Reporter.reportAttack(JSON.stringify(result.events)) + } + + Reporter.reportDerivatives(result.derivatives) + return { result, metrics, diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index 33c0bfbb3a3..474e3a32257 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -107,19 +107,7 @@ describe('WAF Manager', () => { }) describe('run', () => { - it('should call wafManager.run with raspRuleType', () => { - const run = sinon.stub() - WAFManager.prototype.getWAFContext = sinon.stub().returns({ run }) - waf.init(rules, config.appsec) - - const payload = { persistent: { 'server.io.net.url': 'http://example.com' } } - const req = {} - waf.run(payload, req, 'ssrf') - - expect(run).to.be.calledOnceWithExactly(payload, 'ssrf') - }) - - it('should call wafManager.run without raspRuleType', () => { + it('should call wafManager.run', () => { const run = sinon.stub() WAFManager.prototype.getWAFContext = sinon.stub().returns({ run }) waf.init(rules, config.appsec) @@ -128,7 +116,7 @@ describe('WAF Manager', () => { const req = {} waf.run(payload, req) - expect(run).to.be.calledOnceWithExactly(payload, undefined) + expect(run).to.be.calledOnceWithExactly(payload) }) }) @@ -290,66 +278,6 @@ describe('WAF Manager', () => { expect(Reporter.reportAttack).to.be.calledOnceWithExactly('["ATTACK DATA"]') }) - it('should report if rule is triggered', () => { - const result = { - totalRuntime: 1, - durationExt: 1, - events: ['ruleTriggered'] - } - - ddwafContext.run.returns(result) - const params = { - persistent: { - 'server.request.headers.no_cookies': { header: 'value' } - } - } - - wafContextWrapper.run(params) - - expect(Reporter.reportMetrics).to.be.calledOnce - - const reportMetricsArg = Reporter.reportMetrics.firstCall.args[0] - expect(reportMetricsArg.ruleTriggered).to.be.true - }) - - it('should report raspRuleType', () => { - const result = { - totalRuntime: 1, - durationExt: 1 - } - - ddwafContext.run.returns(result) - const params = { - persistent: { - 'server.request.headers.no_cookies': { header: 'value' } - } - } - - wafContextWrapper.run(params, 'rule_type') - - expect(Reporter.reportMetrics).to.be.calledOnce - expect(Reporter.reportMetrics.firstCall.args[1]).to.be.equal('rule_type') - }) - - it('should not report raspRuleType when it is not provided', () => { - const result = { - totalRuntime: 1, - durationExt: 1 - } - - ddwafContext.run.returns(result) - const params = { - persistent: { - 'server.request.headers.no_cookies': { header: 'value' } - } - } - - wafContextWrapper.run(params) - - expect(Reporter.reportMetrics).to.be.calledOnce - expect(Reporter.reportMetrics.firstCall.args[1]).to.be.equal(undefined) - }) - it('should not report attack when ddwafContext does not return events', () => { ddwafContext.run.returns({ totalRuntime: 1, durationExt: 1 }) const params = { @@ -376,7 +304,7 @@ describe('WAF Manager', () => { expect(Reporter.reportAttack).not.to.be.called }) - it('should return the actions', () => { + it('should return the results with metrics and durationExt', () => { const actions = ['block'] ddwafContext.run.returns({ totalRuntime: 1, durationExt: 1, events: [], actions }) @@ -388,7 +316,19 @@ describe('WAF Manager', () => { const result = wafContextWrapper.run(params) - expect(result).to.be.equals(actions) + expect(result.result).to.deep.equal({ + totalRuntime: 1, + durationExt: 1, + events: [], + actions: ['block'] + }) + + expect(result.metrics).to.deep.equal({ + rulesVersion: '1.0.0', + wafVersion: undefined + }) + + expect(result.durationExt).to.be.greaterThan(0) }) it('should report schemas when ddwafContext returns schemas in the derivatives', () => { From eaea97b634ceb1b2d6cc06588b24fc20729d33d5 Mon Sep 17 00:00:00 2001 From: ishabi Date: Tue, 25 Feb 2025 17:01:21 +0100 Subject: [PATCH 13/56] report metrics after blocking --- packages/dd-trace/src/appsec/blocking.js | 4 +- packages/dd-trace/src/appsec/graphql.js | 25 +++-- packages/dd-trace/src/appsec/index.js | 30 +++-- packages/dd-trace/src/appsec/rasp/index.js | 8 +- packages/dd-trace/src/appsec/rasp/utils.js | 20 ++-- packages/dd-trace/src/appsec/reporter.js | 4 + packages/dd-trace/src/appsec/sdk/set_user.js | 6 +- .../dd-trace/src/appsec/sdk/track_event.js | 7 +- .../dd-trace/src/appsec/sdk/user_blocking.js | 26 ++++- .../src/appsec/waf/waf_context_wrapper.js | 38 +++++-- .../dd-trace/test/appsec/blocking.spec.js | 57 ++++------ packages/dd-trace/test/appsec/graphql.spec.js | 9 +- packages/dd-trace/test/appsec/index.spec.js | 18 ++- .../appsec/rasp/command_injection.spec.js | 22 ++-- .../dd-trace/test/appsec/rasp/lfi.spec.js | 9 +- .../test/appsec/rasp/sql_injection.spec.js | 4 +- .../dd-trace/test/appsec/rasp/ssrf.spec.js | 2 +- .../dd-trace/test/appsec/rasp/utils.spec.js | 66 ++++++++--- .../dd-trace/test/appsec/reporter.spec.js | 104 +++++++++++++----- .../test/appsec/sdk/user_blocking.spec.js | 20 ++-- .../dd-trace/test/appsec/waf/index.spec.js | 16 +-- .../appsec/waf/waf_context_wrapper.spec.js | 55 --------- 22 files changed, 311 insertions(+), 239 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 5598bb89255..6b4a1be5cd2 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -99,7 +99,7 @@ function getBlockingData (req, specificType, actionParameters) { } } -function block (req, res, rootSpan, abortController, actionParameters = defaultBlockingActionParameters) { +function block (req, res, abortController, actionParameters = defaultBlockingActionParameters) { if (res.headersSent) { log.warn('[ASM] Cannot send blocking response when headers have already been sent') @@ -119,7 +119,7 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB responseBlockedSet.add(res) - abortController.abort() + abortController?.abort() } function getBlockingAction (actions) { diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index a61fc89035d..f47411fdb3e 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -7,6 +7,7 @@ const { getBlockingData, getBlockingAction } = require('./blocking') +const { reportMetrics } = require('./reporter') const log = require('../log') const waf = require('./waf') const addresses = require('./addresses') @@ -37,14 +38,18 @@ function onGraphqlStartResolve ({ context, resolverInfo }) { if (!resolverInfo || typeof resolverInfo !== 'object') return - const actions = waf.run({ ephemeral: { [addresses.HTTP_INCOMING_GRAPHQL_RESOLVER]: resolverInfo } }, req) - const blockingAction = getBlockingAction(actions) - if (blockingAction) { - const requestData = graphqlRequestData.get(req) - if (requestData?.isInGraphqlRequest) { - requestData.blocked = true - requestData.wafAction = blockingAction - context?.abortController?.abort() + const wafResults = waf.run({ ephemeral: { [addresses.HTTP_INCOMING_GRAPHQL_RESOLVER]: resolverInfo } }, req) + + if (wafResults) { + const blockingAction = getBlockingAction(wafResults.actions) + + if (blockingAction) { + const requestData = graphqlRequestData.get(req) + if (requestData?.isInGraphqlRequest) { + requestData.blocked = true + requestData.wafResults = wafResults + context?.abortController?.abort() + } } } } @@ -96,7 +101,7 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { if (!rootSpan) return try { - const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, requestData.wafAction) + const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, requestData.wafResults.actions) abortData.statusCode = blockingData.statusCode abortData.headers = blockingData.headers abortData.message = blockingData.body @@ -104,6 +109,8 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { abortController.abort() rootSpan.setTag('appsec.blocked', 'true') + + reportMetrics(requestData.wafResults.metrics, null) } catch (err) { rootSpan.addTags({ '_dd.appsec.block.failed': 1 diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 8fb14faa1df..0dd7401840d 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -150,9 +150,9 @@ function incomingHttpStartTranslator ({ req, res, abortController }) { persistent[addresses.HTTP_CLIENT_IP] = clientIp } - const actions = waf.run({ persistent }, req) + const results = waf.run({ persistent }, req) - handleResults(actions, req, res, rootSpan, abortController) + handleResults(results, req, res, rootSpan, abortController) } function incomingHttpEndTranslator ({ req, res }) { @@ -274,12 +274,14 @@ function onResponseBody ({ req, res, body }) { if (!body || typeof body !== 'object') return if (!apiSecuritySampler.sampleRequest(req, res)) return - // we don't support blocking at this point, so no results needed - waf.run({ + // we don't support blocking at this point, so no results needed only reporting + const results = waf.run({ persistent: { [addresses.HTTP_INCOMING_RESPONSE_BODY]: body } }, req) + + reportMetrics(results?.metrics, null) } function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) { @@ -319,16 +321,17 @@ function onResponseSetHeader ({ res, abortController }) { } function handleResults (wafResults, req, res, rootSpan, abortController) { - const { metrics, durationExt, result } = wafResults + if (!wafResults) return + + const { actions, metrics } = wafResults - const ruleTriggered = !!result.events?.length - const blockTriggered = getBlockingAction(wafResults?.actions) + const blockTriggered = getBlockingAction(actions) - const canBlock = wafResults?.actions && req && res && rootSpan && abortController + const canBlock = actions && req && res && rootSpan && abortController if (canBlock && blockTriggered) { try { - block(req, res, rootSpan, abortController, blockTriggered) + block(req, res, abortController, blockTriggered) rootSpan.addTags({ 'appsec.blocked': 'true' @@ -342,14 +345,7 @@ function handleResults (wafResults, req, res, rootSpan, abortController) { } } - reportMetrics({ - ...metrics, - durationExt, - duration: result.totalRuntime / 1e3, - ruleTriggered, - blockTriggered, - wafTimeout: result.timeout - }, null) + reportMetrics(metrics, null) } function disable () { diff --git a/packages/dd-trace/src/appsec/rasp/index.js b/packages/dd-trace/src/appsec/rasp/index.js index 4a65518495d..1ac944d027b 100644 --- a/packages/dd-trace/src/appsec/rasp/index.js +++ b/packages/dd-trace/src/appsec/rasp/index.js @@ -86,7 +86,13 @@ function blockOnDatadogRaspAbortError ({ error }) { const { req, res, blockingAction } = abortError if (!isBlocked(res)) { - block(req, res, web.root(req), null, blockingAction) + block(req, res, null, blockingAction) + + const rootSpan = web.root(req) + + rootSpan?.addTags({ + 'appsec.blocked': 'true' + }) } return true diff --git a/packages/dd-trace/src/appsec/rasp/utils.js b/packages/dd-trace/src/appsec/rasp/utils.js index 7d5c23aece7..0ae76724ab7 100644 --- a/packages/dd-trace/src/appsec/rasp/utils.js +++ b/packages/dd-trace/src/appsec/rasp/utils.js @@ -30,8 +30,11 @@ class DatadogRaspAbortError extends Error { } function handleResult (wafResults, req, res, abortController, config, raspRule) { - const { result, metrics, durationExt } = wafResults - const generateStackTraceAction = result.actions?.generate_stack + if (!wafResults) return + + const { actions, metrics } = wafResults + + const generateStackTraceAction = actions?.generate_stack const { enabled, maxDepth, maxStackTraces } = config.appsec.stackTrace @@ -47,20 +50,11 @@ function handleResult (wafResults, req, res, abortController, config, raspRule) ) } - const ruleTriggered = !!result?.events?.length - const blockingAction = getBlockingAction(result?.actions) - - reportMetrics({ - ...metrics, - durationExt, - duration: result.totalRuntime / 1e3, - ruleTriggered, - blockingAction, - wafTimeout: result.timeout - }, raspRule) + reportMetrics(metrics, raspRule) if (!abortController || abortOnUncaughtException) return + const blockingAction = getBlockingAction(actions) if (blockingAction) { // Should block only in express if (rootSpan?.context()._name === 'express.request') { diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 07ec65ff73a..34a7ca6e976 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -102,13 +102,17 @@ function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}) { } function reportMetrics (metrics, raspRule) { + if (!metrics) return + const store = storage('legacy').getStore() const rootSpan = store?.req && web.root(store.req) + if (!rootSpan) return if (metrics.rulesVersion) { rootSpan.setTag('_dd.appsec.event_rules.version', metrics.rulesVersion) } + if (raspRule) { updateRaspRequestsMetricTags(metrics, store.req, raspRule) } else { diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index bc7b0281b03..93f78336cd6 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -4,6 +4,7 @@ const { getRootSpan } = require('./utils') const log = require('../../log') const waf = require('../waf') const addresses = require('../addresses') +const { reportMetrics } = require('../reporter') function setUserTags (user, rootSpan) { for (const k of Object.keys(user)) { @@ -34,7 +35,10 @@ function setUser (tracer, user) { persistent[addresses.USER_SESSION_ID] = user.session_id } - waf.run({ persistent }) + const wafResults = waf.run({ persistent }) + if (!wafResults) return + + reportMetrics(wafResults.metrics, null) } module.exports = { diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index a04f596bbc3..4c94670d5a2 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -8,6 +8,7 @@ const waf = require('../waf') const { SAMPLING_MECHANISM_APPSEC } = require('../../constants') const { keepTrace } = require('../../priority_sampler') const addresses = require('../addresses') +const { reportMetrics } = require('../reporter') function trackUserLoginSuccessEvent (tracer, user, metadata) { // TODO: better user check here and in _setUser() ? @@ -96,7 +97,11 @@ function runWaf (eventName, user) { persistent[addresses.USER_LOGIN] = '' + user.login } - waf.run({ persistent }) + const wafResults = waf.run({ persistent }) + + if (!wafResults) return + + reportMetrics(wafResults, null) } module.exports = { diff --git a/packages/dd-trace/src/appsec/sdk/user_blocking.js b/packages/dd-trace/src/appsec/sdk/user_blocking.js index e0000ba1ac9..125d34239cf 100644 --- a/packages/dd-trace/src/appsec/sdk/user_blocking.js +++ b/packages/dd-trace/src/appsec/sdk/user_blocking.js @@ -7,10 +7,18 @@ const { block, getBlockingAction } = require('../blocking') const { storage } = require('../../../../datadog-core') const { setUserTags } = require('./set_user') const log = require('../../log') +const { reportMetrics } = require('../reporter') function isUserBlocked (user) { - const actions = waf.run({ persistent: { [USER_ID]: user.id } }) - return !!getBlockingAction(actions) + const wafResults = waf.run({ persistent: { [USER_ID]: user.id } }) + + if (!wafResults) return + + const blockTriggered = !!getBlockingAction(wafResults.actions) + + reportMetrics(wafResults.metrics, null) + + return blockTriggered } function checkUserAndSetUser (tracer, user) { @@ -52,9 +60,19 @@ function blockRequest (tracer, req, res) { return false } - block(req, res, rootSpan) + try { + block(req, res) + + rootSpan.setTag('appsec.blocked', 'true') - return true + return true + } catch (err) { + rootSpan.setTag('_dd.appsec.block.failed', 1) + + log.error('[ASM] Blocking error', err) + + return false + } } module.exports = { 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 046917ba355..6cfff5e7394 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -34,7 +34,9 @@ class WAFContextWrapper { if (userId) { const cachedAction = this.cachedUserIdActions.get(userId) if (cachedAction) { - return cachedAction + return { + actions: cachedAction + } } } @@ -81,14 +83,28 @@ class WAFContextWrapper { const metrics = { rulesVersion: this.rulesVersion, - wafVersion: this.wafVersion + wafVersion: this.wafVersion, + wafTimeout: false, + duration: 0, + blockTriggered: false, + ruleTriggered: false } const result = this.ddwafContext.run(payload, this.wafTimeout) + this.addressesToSkip = newAddressesToSkip + + const end = process.hrtime.bigint() + metrics.durationExt = parseInt(end - start) / 1e3 + if (!result) { // Binding or other waf unexpected errors metrics.errorCode = -127 + + return { + actions: null, + metrics + } } else { if (typeof result.errorCode === 'number' && result.errorCode < 0) { metrics.errorCode = result.errorCode @@ -107,13 +123,9 @@ class WAFContextWrapper { } } - const end = process.hrtime.bigint() - - this.addressesToSkip = newAddressesToSkip - - const ruleTriggered = !!result?.events?.length + const ruleTriggered = !!result.events?.length - const blockTriggered = !!getBlockingAction(result?.actions) + const blockTriggered = !!getBlockingAction(result.actions) // SPECIAL CASE FOR USER_ID // TODO: make this universal @@ -131,10 +143,14 @@ class WAFContextWrapper { Reporter.reportDerivatives(result.derivatives) + metrics.duration = result.totalRuntime / 1e3 + metrics.blockTriggered = blockTriggered + metrics.ruleTriggered = ruleTriggered + metrics.wafTimeout = result.timeout + return { - result, - metrics, - durationExt: parseInt(end - start) / 1e3 + actions: result.actions, + metrics } } diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 2514dedbf7a..64251e01c74 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -15,7 +15,7 @@ describe('blocking', () => { let log let block, setTemplates - let req, res, rootSpan + let req, res beforeEach(() => { log = { @@ -45,10 +45,6 @@ describe('blocking', () => { } } } - - rootSpan = { - addTags: sinon.stub() - } }) describe('block', () => { @@ -58,20 +54,23 @@ describe('blocking', () => { it('should log warn and not send blocking response when headers have already been sent', () => { res.headersSent = true - block(req, res, rootSpan) + + try { + block(req, res) + } catch (error) { + expect(error.message).to.equal('Headers have already been sent') + } expect(log.warn).to.have.been .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ '_dd.appsec.block.failed': 1 }) expect(res.setHeader).to.not.have.been.called expect(res.constructor.prototype.end).to.not.have.been.called }) it('should send blocking response with html type if present in the headers', () => { req.headers.accept = 'text/html' - block(req, res, rootSpan) + block(req, res) - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'text/html; charset=utf-8', 'Content-Length': 12 @@ -81,9 +80,8 @@ describe('blocking', () => { it('should send blocking response with json type if present in the headers in priority', () => { req.headers.accept = 'text/html, application/xhtml+xml, application/xml;q=0.9, */*;q=0.8, application/json' - block(req, res, rootSpan) + block(req, res) - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -92,9 +90,8 @@ describe('blocking', () => { }) it('should send blocking response with json type if neither html or json is present in the headers', () => { - block(req, res, rootSpan) + block(req, res) - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -104,9 +101,8 @@ describe('blocking', () => { it('should send blocking response and call abortController if passed in arguments', () => { const abortController = new AbortController() - block(req, res, rootSpan, abortController) + block(req, res, abortController) - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -118,9 +114,8 @@ describe('blocking', () => { it('should remove all headers before sending blocking response', () => { res.getHeaderNames.returns(['header1', 'header2']) - block(req, res, rootSpan) + block(req, res) - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.removeHeader).to.have.been.calledTwice expect(res.removeHeader.firstCall).to.have.been.calledWithExactly('header1') expect(res.removeHeader.secondCall).to.have.been.calledWithExactly('header2') @@ -130,16 +125,6 @@ describe('blocking', () => { }) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly('jsonBody') }) - - it('should set block failed tag when express res end throws an error', () => { - res.constructor.prototype.end.throws(new Error('End failed')) - - block(req, res, rootSpan) - - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ '_dd.appsec.block.failed': 1 }) - expect(res.writeHead).to.have.been.calledOnce - expect(res.constructor.prototype.end).to.have.been.calledOnce - }) }) describe('block with default templates', () => { @@ -154,7 +139,7 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan) + block(req, res) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) @@ -162,7 +147,7 @@ describe('blocking', () => { it('should block with default json template', () => { setTemplates(config) - block(req, res, rootSpan) + block(req, res) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) @@ -184,7 +169,7 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + block(req, res, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) @@ -199,7 +184,7 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + block(req, res, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) @@ -214,7 +199,7 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + block(req, res, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) @@ -227,7 +212,7 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + block(req, res, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) @@ -241,7 +226,7 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + block(req, res, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) @@ -255,7 +240,7 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + block(req, res, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) @@ -268,7 +253,7 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + block(req, res, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWithExactly(301, { Location: '/you-have-been-blocked' diff --git a/packages/dd-trace/test/appsec/graphql.spec.js b/packages/dd-trace/test/appsec/graphql.spec.js index 308103fad87..08201f7c497 100644 --- a/packages/dd-trace/test/appsec/graphql.spec.js +++ b/packages/dd-trace/test/appsec/graphql.spec.js @@ -225,7 +225,10 @@ describe('GraphQL', () => { const abortController = context.abortController sinon.stub(waf, 'run').returns({ - block_request: blockParameters + actions: { + block_request: blockParameters + }, + metrics: {} }) sinon.stub(web, 'root').returns(rootSpan) @@ -243,7 +246,9 @@ describe('GraphQL', () => { const abortData = {} apolloChannel.asyncEnd.publish({ abortController, abortData }) - expect(blocking.getBlockingData).to.have.been.calledOnceWithExactly(req, 'graphql', blockParameters) + // [TODO] The implementation of this was not correct? + expect(blocking.getBlockingData).to.have.been.calledOnceWithExactly( + req, 'graphql', { block_request: blockParameters }) expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') }) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 09cca9aad5f..134f277db44 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -32,10 +32,16 @@ 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' + actions: { + block_request: { + status_code: '401', + type: 'auto', + grpc_status_code: '10' + } + }, + metrics: { + rulesVersion: '1.0.0', + wafVersion: '1.0.0' } } @@ -834,7 +840,7 @@ describe('AppSec Index', function () { passportVerify.publish(payload) - expect(storage('legacy').getStore).to.have.been.calledOnce + expect(storage('legacy').getStore).to.have.been.calledTwice expect(web.root).to.have.been.calledOnceWithExactly(req) expect(UserTracking.trackLogin).to.have.been.calledOnceWithExactly( payload.framework, @@ -912,7 +918,7 @@ describe('AppSec Index', function () { passportUser.publish(payload) - expect(storage('legacy').getStore).to.have.been.calledOnce + expect(storage('legacy').getStore).to.have.been.calledTwice expect(web.root).to.have.been.calledOnceWithExactly(req) expect(UserTracking.trackUser).to.have.been.calledOnceWithExactly( payload.user, diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.spec.js index eae70e05256..e27d7273fe7 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.spec.js @@ -106,9 +106,7 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.SHELL_COMMAND]: 'cmd' } - sinon.assert.calledOnceWithExactly( - waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'shell' } - ) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should analyze command_injection with arguments', () => { @@ -123,9 +121,7 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.SHELL_COMMAND]: ['cmd', 'arg0', 'arg1'] } - sinon.assert.calledOnceWithExactly( - waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'shell' } - ) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should call handleResult', () => { @@ -134,12 +130,13 @@ describe('RASP - command_injection.js', () => { const wafResult = { waf: 'waf' } const req = { req: 'req' } const res = { res: 'res' } + const rule = { type: 'command_injection', variant: 'shell' } waf.run.returns(wafResult) legacyStorage.getStore.returns({ req, res }) start.publish(ctx) - sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config) + sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config, rule) }) }) @@ -155,9 +152,7 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.EXEC_COMMAND]: ['ls'] } - sinon.assert.calledOnceWithExactly( - waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'exec' } - ) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should analyze command injection with arguments', () => { @@ -172,9 +167,7 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.EXEC_COMMAND]: ['ls', '-la', '/tmp'] } - sinon.assert.calledOnceWithExactly( - waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'exec' } - ) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should call handleResult', () => { @@ -183,12 +176,13 @@ describe('RASP - command_injection.js', () => { const wafResult = { waf: 'waf' } const req = { req: 'req' } const res = { res: 'res' } + const rule = { type: 'command_injection', variant: 'exec' } waf.run.returns(wafResult) legacyStorage.getStore.returns({ req, res }) start.publish(ctx) - sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config) + sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config, rule) }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/lfi.spec.js b/packages/dd-trace/test/appsec/rasp/lfi.spec.js index b21c6473103..0a180596303 100644 --- a/packages/dd-trace/test/appsec/rasp/lfi.spec.js +++ b/packages/dd-trace/test/appsec/rasp/lfi.spec.js @@ -15,7 +15,12 @@ describe('RASP - lfi.js', () => { } waf = { - run: sinon.stub() + run: sinon.stub().returns({ + result: { + totalRuntime: 30, + timeout: false + } + }) } web = { @@ -109,7 +114,7 @@ describe('RASP - lfi.js', () => { fsOperationStart.publish(ctx) const ephemeral = { [FS_OPERATION_PATH]: path } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'lfi' }) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should NOT analyze lfi for child fs operations', () => { diff --git a/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js b/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js index 39b56c22d5e..2a3b641eb26 100644 --- a/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js +++ b/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js @@ -55,7 +55,7 @@ describe('RASP - sql_injection', () => { [addresses.DB_STATEMENT]: 'SELECT 1', [addresses.DB_SYSTEM]: 'postgresql' } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'sql_injection' }) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should not analyze sql injection if rasp is disabled', () => { @@ -126,7 +126,7 @@ describe('RASP - sql_injection', () => { [addresses.DB_STATEMENT]: 'SELECT 1', [addresses.DB_SYSTEM]: 'mysql' } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'sql_injection' }) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should not analyze sql injection if rasp is disabled', () => { diff --git a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js index 7c301e8e517..4089cc38e76 100644 --- a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js +++ b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js @@ -52,7 +52,7 @@ describe('RASP - ssrf.js', () => { httpClientRequestStart.publish(ctx) const ephemeral = { [addresses.HTTP_OUTGOING_URL]: 'http://example.com' } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'ssrf' }) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) }) it('should not analyze ssrf if rasp is disabled', () => { diff --git a/packages/dd-trace/test/appsec/rasp/utils.spec.js b/packages/dd-trace/test/appsec/rasp/utils.spec.js index 6a74c07444d..e8679c363ac 100644 --- a/packages/dd-trace/test/appsec/rasp/utils.spec.js +++ b/packages/dd-trace/test/appsec/rasp/utils.spec.js @@ -35,15 +35,21 @@ describe('RASP - utils.js', () => { const req = {} const rootSpan = {} const stackId = 'test_stack_id' - const result = { - generate_stack: { - stack_id: stackId + const results = { + actions: { + generate_stack: { + stack_id: stackId + } + }, + metrics: { + totalRuntime: 50, + timeout: true } } web.root.returns(rootSpan) - utils.handleResult(result, req, undefined, undefined, config) + utils.handleResult(results, req, undefined, undefined, config, { type: 'type' }) sinon.assert.calledOnceWithExactly(stackTrace.reportStackTrace, rootSpan, stackId, sinon.match.array) }) @@ -56,47 +62,71 @@ describe('RASP - utils.js', () => { } } } - const result = { - generate_stack: { - stack_id: 'stackId' + const results = { + actions: { + generate_stack: { + stack_id: 'stackId' + } + }, + metrics: { + totalRuntime: 50, + timeout: true } } web.root.returns(rootSpan) - utils.handleResult(result, req, undefined, undefined, config) + utils.handleResult(results, req, undefined, undefined, config, { type: 'type' }) sinon.assert.notCalled(stackTrace.reportStackTrace) }) it('should not report stack trace when rootSpan is null', () => { const req = {} - const result = { - generate_stack: { - stack_id: 'stackId' + const results = { + actions: { + generate_stack: { + stack_id: 'stackId' + } + }, + metrics: { + totalRuntime: 50, + timeout: true } } web.root.returns(null) - utils.handleResult(result, req, undefined, undefined, config) + utils.handleResult(results, req, undefined, undefined, config) sinon.assert.notCalled(stackTrace.reportStackTrace) }) it('should not report stack trace when no action is present in waf result', () => { const req = {} - const result = {} + const result = { + metrics: { + totalRuntime: 50, + timeout: true + } + } - utils.handleResult(result, req, undefined, undefined, config) + utils.handleResult(result, req, undefined, undefined, config, { type: 'type' }) sinon.assert.notCalled(stackTrace.reportStackTrace) }) it('should not report stack trace when stack trace reporting is disabled', () => { const req = {} - const result = { - generate_stack: { - stack_id: 'stackId' + const results = { + actions: { + generate_stack: { + stack_id: 'stackId' + } + }, + metrics: { + totalRuntime: 50, + timeout: true } } + const config = { appsec: { stackTrace: { @@ -107,7 +137,7 @@ describe('RASP - utils.js', () => { } } - utils.handleResult(result, req, undefined, undefined, config) + utils.handleResult(results, req, undefined, undefined, config, { type: 'type' }) sinon.assert.notCalled(stackTrace.reportStackTrace) }) }) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index ac8dc25929c..4bc4403a07f 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -149,15 +149,28 @@ describe('reporter', () => { }) it('should do nothing when passed incomplete objects', () => { - web.root.returns(null) - Reporter.reportMetrics({}) expect(span.setTag).not.to.have.been.called }) - it('should set duration metrics if set', () => { - const metrics = { duration: 1337 } + it('should do nothing when rootSpan is not available', () => { + web.root.returns(null) + + const metrics = { + durationExt: 42, + totalRuntime: 1337000 + } + + Reporter.reportMetrics(metrics) + + expect(telemetry.updateWafRequestsMetricTags).not.to.have.been.called + expect(telemetry.updateRaspRequestsMetricTags).not.to.have.been.called + }) + + it('should set duration metrics from result.totalRuntime', () => { + const metrics = { durationExt: 42, totalRuntime: 1337000 } + Reporter.reportMetrics(metrics) expect(web.root).to.have.been.calledOnceWithExactly(req) @@ -166,7 +179,8 @@ describe('reporter', () => { }) it('should set ext duration metrics if set', () => { - const metrics = { durationExt: 42 } + const metrics = { durationExt: 42, totalRuntime: 0 } + Reporter.reportMetrics(metrics) expect(web.root).to.have.been.calledOnceWithExactly(req) @@ -175,48 +189,86 @@ describe('reporter', () => { }) it('should set rulesVersion if set', () => { - Reporter.reportMetrics({ rulesVersion: '1.2.3' }) + const metrics = { + rulesVersion: '1.2.3', + durationExt: 0, + totalRuntime: 0 + } + + Reporter.reportMetrics(metrics) expect(web.root).to.have.been.calledOnceWithExactly(req) expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.event_rules.version', '1.2.3') - expect(telemetry.updateRaspRequestsMetricTags).to.not.have.been.called }) - it('should set max truncation string length metric if set', () => { - Reporter.reportMetrics({ maxTruncatedString: 300 }) + it('should set blockTriggered when provided', () => { + const metrics = { + durationExt: 0, + totalRuntime: 0, + blockTriggered: true + } - expect(web.root).to.have.been.calledOnceWithExactly(req) - expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.truncated.string_length', 300) + Reporter.reportMetrics(metrics, null) + + expect(telemetry.updateWafRequestsMetricTags).to.have.been.calledOnceWithExactly(metrics, req) }) - it('should set max truncation container size metric if set', () => { - Reporter.reportMetrics({ maxTruncatedContainerSize: 200 }) + it('should set wafTimeout when result has timeout', () => { + const metrics = { + durationExt: 0, + totalRuntime: 0, + timeout: true + } - expect(web.root).to.have.been.calledOnceWithExactly(req) - expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.truncated.container_size', 200) + Reporter.reportMetrics(metrics) + + expect(telemetry.updateWafRequestsMetricTags).to.have.been.calledOnceWithExactly(metrics, req) }) - it('should set max truncation container depth metric if set', () => { - Reporter.reportMetrics({ maxTruncatedContainerDepth: 100 }) + it('should set max truncation string length metric if set', () => { + const metrics = { + maxTruncatedString: 300, + durationExt: 0, + totalRuntime: 0 + } - expect(web.root).to.have.been.calledOnceWithExactly(req) - expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.truncated.container_depth', 100) + Reporter.reportMetrics(metrics) + + expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.truncated.string_length', 300) }) - it('should call updateWafRequestsMetricTags', () => { - const metrics = { rulesVersion: '1.2.3' } - const store = storage('legacy').getStore() + it('should set max truncation container size metric if set', () => { + const metrics = { + maxTruncatedContainerSize: 200, + durationExt: 0, + totalRuntime: 0 + } Reporter.reportMetrics(metrics) - expect(telemetry.updateWafRequestsMetricTags).to.have.been.calledOnceWithExactly(metrics, store.req) - expect(telemetry.updateRaspRequestsMetricTags).to.not.have.been.called + expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.truncated.container_size', 200) + }) + + it('should set max truncation container depth metric if set', () => { + const metrics = { + maxTruncatedContainerDepth: 100, + durationExt: 0, + totalRuntime: 0 + } + + Reporter.reportMetrics(metrics) + + expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.truncated.container_depth', 100) }) it('should call updateRaspRequestsMetricTags when raspRule is provided', () => { - const metrics = { rulesVersion: '1.2.3' } - const store = storage('legacy').getStore() + const metrics = { + rulesVersion: '1.2.3', + durationExt: 0, + totalRuntime: 0 + } + const store = storage('legacy').getStore() const raspRule = { type: 'rule_type', variant: 'rule_variant' } Reporter.reportMetrics(metrics, raspRule) 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 e43e5ffd972..65ae39d094f 100644 --- a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js +++ b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js @@ -12,11 +12,14 @@ const { USER_ID } = require('../../../src/appsec/addresses') const blocking = require('../../../src/appsec/blocking') const resultActions = { - block_request: { - status_code: '401', - type: 'auto', - grpc_status_code: '10' - } + actions: { + block_request: { + status_code: '401', + type: 'auto', + grpc_status_code: '10' + } + }, + metrics: {} } describe('user_blocking', () => { @@ -115,7 +118,7 @@ describe('user_blocking', () => { const ret = userBlocking.blockRequest(tracer) expect(ret).to.be.true expect(legacyStorage.getStore).to.have.been.calledOnce - expect(block).to.be.calledOnceWithExactly(req, res, rootSpan) + expect(block).to.be.calledOnceWithExactly(req, res) }) it('should log warning when req or res is not available', () => { @@ -144,7 +147,7 @@ describe('user_blocking', () => { const ret = userBlocking.blockRequest(tracer, req, res) expect(ret).to.be.true expect(log.warn).to.not.have.been.called - expect(block).to.have.been.calledOnceWithExactly(req, res, rootSpan) + expect(block).to.have.been.calledOnceWithExactly(req, res) }) }) }) @@ -286,11 +289,12 @@ describe('user_blocking', () => { controller = (req, res) => { res.end() const ret = tracer.appsec.blockRequest() - expect(ret).to.be.true + expect(ret).to.be.false } agent.use(traces => { expect(traces[0][0].meta).to.not.have.property('appsec.blocked', 'true') expect(traces[0][0].meta).to.have.property('http.status_code', '200') + expect(traces[0][0].metrics).to.not.have.property('_dd.appsec.block.failed', 1) }).then(done).catch(done) axios.get(`http://localhost:${port}/`) }) diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index 474e3a32257..bafe4464dd8 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -49,7 +49,6 @@ describe('WAF Manager', () => { waf.destroy() sinon.stub(Reporter.metricsQueue, 'set') - sinon.stub(Reporter, 'reportMetrics') sinon.stub(Reporter, 'reportAttack') sinon.stub(Reporter, 'reportWafUpdate') sinon.stub(Reporter, 'reportDerivatives') @@ -316,19 +315,16 @@ describe('WAF Manager', () => { const result = wafContextWrapper.run(params) - expect(result.result).to.deep.equal({ - totalRuntime: 1, - durationExt: 1, - events: [], - actions: ['block'] - }) + expect(result.actions).to.deep.equal(['block']) - expect(result.metrics).to.deep.equal({ + expect(result.metrics).to.include({ rulesVersion: '1.0.0', - wafVersion: undefined + duration: 0.001, + blockTriggered: false, + ruleTriggered: false }) - expect(result.durationExt).to.be.greaterThan(0) + expect(result.metrics.durationExt).to.be.greaterThan(0) }) it('should report schemas when ddwafContext returns schemas in the derivatives', () => { diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index b168734559e..436f6c093d4 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -154,59 +154,4 @@ describe('WAFContextWrapper', () => { sinon.assert.calledOnceWithExactly(log.warn, '[ASM] Calling run on a disposed context') }) }) - - describe('Truncation handling', () => { - let ddwafContext, wafContextWrapper, Reporter, WAFContextWrapper - - beforeEach(() => { - ddwafContext = { - run: sinon.stub(), - disposed: false - } - - Reporter = { - reportMetrics: sinon.stub(), - reportAttack: sinon.stub(), - reportDerivatives: sinon.stub() - } - - WAFContextWrapper = proxyquire('../../../src/appsec/waf/waf_context_wrapper', { - '../reporter': Reporter - }) - - wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) - }) - - it('Should report truncations', () => { - const payload = { - persistent: { - [addresses.HTTP_INCOMING_QUERY]: { - value: 'test' - } - } - } - - const wafResult = { - metrics: { - maxTruncatedString: 6000, - maxTruncatedContainerSize: 400, - maxTruncatedContainerDepth: 20 - }, - totalRuntime: 100, - events: [], - actions: [], - timeout: false, - derivatives: {} - } - ddwafContext.run.returns(wafResult) - - wafContextWrapper.run(payload) - const reportedMetricsCall = Reporter.reportMetrics.firstCall - expect(reportedMetricsCall.args[0]).to.include({ - maxTruncatedString: 6000, - maxTruncatedContainerSize: 400, - maxTruncatedContainerDepth: 20 - }) - }) - }) }) From 6ed1996476af3f6b8ae7e7c2bdd950a9ad8e2948 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 26 Feb 2025 10:25:55 +0100 Subject: [PATCH 14/56] call runWaf --- packages/dd-trace/src/appsec/waf/waf_context_wrapper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6cfff5e7394..d039f09081d 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -90,7 +90,7 @@ class WAFContextWrapper { ruleTriggered: false } - const result = this.ddwafContext.run(payload, this.wafTimeout) + const result = this.runWaf(payload, this.wafTimeout) this.addressesToSkip = newAddressesToSkip From 2794d7008908d73753e17279ab91cb08e4900081 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 26 Feb 2025 14:10:23 +0100 Subject: [PATCH 15/56] rasp timout test --- .../test/appsec/rasp/waf-timeout.spec.js | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js diff --git a/packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js b/packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js new file mode 100644 index 00000000000..0e8cb0c8319 --- /dev/null +++ b/packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js @@ -0,0 +1,66 @@ +'use strict' + +const { createSandbox, FakeAgent, spawnProc } = require('../../../../../integration-tests/helpers') +const getPort = require('get-port') +const path = require('path') +const Axios = require('axios') +const { assert } = require('chai') + +describe('RASP - Waf timeout', () => { + let axios, sandbox, cwd, appPort, appFile, agent, proc + + before(async function () { + this.timeout(process.platform === 'win32' ? 90000 : 30000) + + sandbox = await createSandbox( + ['express'], + false, + [ + path.join(__dirname, 'resources'), + path.join(__dirname, '..', '..', '..', 'src', 'appsec', 'recommended.json') + ] + ) + + appPort = await getPort() + cwd = sandbox.folder + appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') + + axios = Axios.create({ + baseURL: `http://localhost:${appPort}` + }) + }) + + after(async function () { + this.timeout(60000) + await sandbox.remove() + }) + + beforeEach(async () => { + agent = await new FakeAgent().start() + proc = await spawnProc(appFile, { + cwd, + env: { + DD_TRACE_AGENT_PORT: agent.port, + APP_PORT: appPort, + DD_APPSEC_ENABLED: 'true', + DD_APPSEC_RASP_ENABLED: 'true', + DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, + DD_APPSEC_WAF_TIMEOUT: 1, + DD_APPSEC_RULES: path.join(cwd, 'recommended.json') + } + }) + }) + + afterEach(async () => { + proc.kill() + await agent.stop() + }) + + it('Should not block since waf will timeout', async () => { + await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + await agent.assertMessageReceived(({ payload }) => { + assert.property(payload[0][0].metrics, '_dd.appsec.rasp.timeout') + assert.equal(payload[0][0].metrics['_dd.appsec.rasp.timeout'], 1) + }) + }) +}) From 882120c9b8f309a20a4e45d3ca3b3330eb3b434f Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 26 Feb 2025 14:50:32 +0100 Subject: [PATCH 16/56] add waf/rasp errors and timeout --- .../test/appsec/rasp/waf-timeout.spec.js | 66 --------- .../test/appsec/waf-rasp-errors.spec.js | 131 ++++++++++++++++++ 2 files changed, 131 insertions(+), 66 deletions(-) delete mode 100644 packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js create mode 100644 packages/dd-trace/test/appsec/waf-rasp-errors.spec.js diff --git a/packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js b/packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js deleted file mode 100644 index 0e8cb0c8319..00000000000 --- a/packages/dd-trace/test/appsec/rasp/waf-timeout.spec.js +++ /dev/null @@ -1,66 +0,0 @@ -'use strict' - -const { createSandbox, FakeAgent, spawnProc } = require('../../../../../integration-tests/helpers') -const getPort = require('get-port') -const path = require('path') -const Axios = require('axios') -const { assert } = require('chai') - -describe('RASP - Waf timeout', () => { - let axios, sandbox, cwd, appPort, appFile, agent, proc - - before(async function () { - this.timeout(process.platform === 'win32' ? 90000 : 30000) - - sandbox = await createSandbox( - ['express'], - false, - [ - path.join(__dirname, 'resources'), - path.join(__dirname, '..', '..', '..', 'src', 'appsec', 'recommended.json') - ] - ) - - appPort = await getPort() - cwd = sandbox.folder - appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') - - axios = Axios.create({ - baseURL: `http://localhost:${appPort}` - }) - }) - - after(async function () { - this.timeout(60000) - await sandbox.remove() - }) - - beforeEach(async () => { - agent = await new FakeAgent().start() - proc = await spawnProc(appFile, { - cwd, - env: { - DD_TRACE_AGENT_PORT: agent.port, - APP_PORT: appPort, - DD_APPSEC_ENABLED: 'true', - DD_APPSEC_RASP_ENABLED: 'true', - DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, - DD_APPSEC_WAF_TIMEOUT: 1, - DD_APPSEC_RULES: path.join(cwd, 'recommended.json') - } - }) - }) - - afterEach(async () => { - proc.kill() - await agent.stop() - }) - - it('Should not block since waf will timeout', async () => { - await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') - await agent.assertMessageReceived(({ payload }) => { - assert.property(payload[0][0].metrics, '_dd.appsec.rasp.timeout') - assert.equal(payload[0][0].metrics['_dd.appsec.rasp.timeout'], 1) - }) - }) -}) diff --git a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js new file mode 100644 index 00000000000..4c52c7a35e4 --- /dev/null +++ b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js @@ -0,0 +1,131 @@ +'use strict' + +const { createSandbox, FakeAgent, spawnProc } = require('../../../../integration-tests/helpers') +const getPort = require('get-port') +const path = require('path') +const Axios = require('axios') +const { assert } = require('chai') + +describe.only('WAF/RASP - timeout', () => { + let axios, sandbox, cwd, appPort, appFile, agent, proc + + before(async function () { + this.timeout(process.platform === 'win32' ? 90000 : 30000) + + sandbox = await createSandbox( + ['express'], + false, + [ + path.join(__dirname, 'rasp', 'resources'), + path.join(__dirname, '..', '..', 'src', 'appsec', 'recommended.json') + ] + ) + + appPort = await getPort() + cwd = sandbox.folder + appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') + + axios = Axios.create({ + baseURL: `http://localhost:${appPort}` + }) + }) + + after(async function () { + this.timeout(60000) + await sandbox.remove() + }) + + beforeEach(async () => { + agent = await new FakeAgent().start() + proc = await spawnProc(appFile, { + cwd, + env: { + DD_TRACE_AGENT_PORT: agent.port, + APP_PORT: appPort, + DD_APPSEC_ENABLED: 'true', + DD_APPSEC_RASP_ENABLED: 'true', + DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, + DD_APPSEC_WAF_TIMEOUT: 1, + DD_APPSEC_RULES: path.join(cwd, 'recommended.json') + } + }) + }) + + afterEach(async () => { + proc.kill() + await agent.stop() + }) + + it('Should not block since waf will timeout', async () => { + await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + await agent.assertMessageReceived(({ payload }) => { + assert.property(payload[0][0].metrics, '_dd.appsec.rasp.timeout') + assert.equal(payload[0][0].metrics['_dd.appsec.rasp.timeout'], 1) + + assert.property(payload[0][0].metrics, '_dd.appsec.waf.timeouts') + assert(payload[0][0].metrics['_dd.appsec.waf.timeouts'] > 1) + }) + }) +}) + +describe.only('WAF/RASP - error', () => { + let axios, sandbox, cwd, appPort, appFile, agent, proc + + before(async function () { + this.timeout(process.platform === 'win32' ? 90000 : 30000) + + sandbox = await createSandbox( + ['express'], + false, + [ + path.join(__dirname, 'rasp', 'resources'), + path.join(__dirname, '..', '..', 'src', 'appsec', 'recommended.json') + ] + ) + + appPort = await getPort() + cwd = sandbox.folder + appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') + + axios = Axios.create({ + baseURL: `http://localhost:${appPort}` + }) + }) + + after(async function () { + this.timeout(60000) + await sandbox.remove() + }) + + beforeEach(async () => { + agent = await new FakeAgent().start() + proc = await spawnProc(appFile, { + cwd, + env: { + DD_TRACE_AGENT_PORT: agent.port, + APP_PORT: appPort, + DD_APPSEC_ENABLED: 'true', + DD_APPSEC_RASP_ENABLED: 'true', + DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, + DD_APPSEC_WAF_TIMEOUT: 0.1, + DD_APPSEC_RULES: path.join(cwd, 'recommended.json') + } + }) + }) + + afterEach(async () => { + proc.kill() + await agent.stop() + }) + + it('Should not block since waf will return error', async () => { + await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + await agent.assertMessageReceived(({ payload }) => { + assert.property(payload[0][0].metrics, '_dd.appsec.rasp.error') + assert.equal(payload[0][0].metrics['_dd.appsec.rasp.error'], -127) + + assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') + assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) + }) + }) +}) From 96849b553d5c8809fc82ef4c45b91fa74c6ba796 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 26 Feb 2025 15:32:28 +0100 Subject: [PATCH 17/56] report metrics after catch on graphql --- packages/dd-trace/src/appsec/graphql.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index f47411fdb3e..d5ccdbbd9e0 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -109,8 +109,6 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { abortController.abort() rootSpan.setTag('appsec.blocked', 'true') - - reportMetrics(requestData.wafResults.metrics, null) } catch (err) { rootSpan.addTags({ '_dd.appsec.block.failed': 1 @@ -118,6 +116,8 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { log.error('[ASM] Blocking error', err) } + + reportMetrics(requestData.wafResults.metrics, null) } graphqlRequestData.delete(req) From 4a9fc38bec6012a1ea7786ea8a24bb6d922cc36c Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 10:20:46 +0100 Subject: [PATCH 18/56] return false if waf result is not defined --- packages/dd-trace/src/appsec/sdk/user_blocking.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/user_blocking.js b/packages/dd-trace/src/appsec/sdk/user_blocking.js index 125d34239cf..02d5b52f48d 100644 --- a/packages/dd-trace/src/appsec/sdk/user_blocking.js +++ b/packages/dd-trace/src/appsec/sdk/user_blocking.js @@ -3,7 +3,7 @@ const { USER_ID } = require('../addresses') const waf = require('../waf') const { getRootSpan } = require('./utils') -const { block, getBlockingAction } = require('../blocking') +const { block } = require('../blocking') const { storage } = require('../../../../datadog-core') const { setUserTags } = require('./set_user') const log = require('../../log') @@ -11,14 +11,11 @@ const { reportMetrics } = require('../reporter') function isUserBlocked (user) { const wafResults = waf.run({ persistent: { [USER_ID]: user.id } }) - - if (!wafResults) return - - const blockTriggered = !!getBlockingAction(wafResults.actions) + if (!wafResults) return false reportMetrics(wafResults.metrics, null) - return blockTriggered + return wafResults.metrics.blockTriggered } function checkUserAndSetUser (tracer, user) { From b13b0f5624970eb98b8279d263de814d61e5f2ac Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 10:44:39 +0100 Subject: [PATCH 19/56] graphql blocking action --- packages/dd-trace/src/appsec/graphql.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index d5ccdbbd9e0..dc9f99e02de 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -101,7 +101,8 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { if (!rootSpan) return try { - const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, requestData.wafResults.actions) + const blockingAction = getBlockingAction(requestData.wafResults.actions) + const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, blockingAction) abortData.statusCode = blockingData.statusCode abortData.headers = blockingData.headers abortData.message = blockingData.body From 83c753ce0e41a7b6f57c98d335487b3262c51a7c Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 11:25:39 +0100 Subject: [PATCH 20/56] graphql report metrics --- packages/dd-trace/src/appsec/graphql.js | 16 +++++++--------- .../dd-trace/test/appsec/waf-rasp-errors.spec.js | 4 ++-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index dc9f99e02de..0cca09c0ae3 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -41,15 +41,13 @@ function onGraphqlStartResolve ({ context, resolverInfo }) { const wafResults = waf.run({ ephemeral: { [addresses.HTTP_INCOMING_GRAPHQL_RESOLVER]: resolverInfo } }, req) if (wafResults) { + const requestData = graphqlRequestData.get(req) + requestData.wafResults = wafResults const blockingAction = getBlockingAction(wafResults.actions) - if (blockingAction) { - const requestData = graphqlRequestData.get(req) - if (requestData?.isInGraphqlRequest) { - requestData.blocked = true - requestData.wafResults = wafResults - context?.abortController?.abort() - } + if (blockingAction && requestData?.isInGraphqlRequest) { + requestData.blocked = true + context?.abortController?.abort() } } } @@ -117,10 +115,10 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { log.error('[ASM] Blocking error', err) } - - reportMetrics(requestData.wafResults.metrics, null) } + reportMetrics(requestData.wafResults.metrics, null) + graphqlRequestData.delete(req) } diff --git a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js index 4c52c7a35e4..c93a2f60f2f 100644 --- a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js @@ -6,7 +6,7 @@ const path = require('path') const Axios = require('axios') const { assert } = require('chai') -describe.only('WAF/RASP - timeout', () => { +describe('WAF/RASP - timeout', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc before(async function () { @@ -68,7 +68,7 @@ describe.only('WAF/RASP - timeout', () => { }) }) -describe.only('WAF/RASP - error', () => { +describe('WAF/RASP - error', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc before(async function () { From b12a6e963e7b4222fd57140b891140f59cebdd2d Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 11:51:55 +0100 Subject: [PATCH 21/56] check if request data exist first --- packages/dd-trace/src/appsec/graphql.js | 6 +++--- packages/dd-trace/test/appsec/graphql.spec.js | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index 0cca09c0ae3..00eb2f200dc 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -39,13 +39,13 @@ function onGraphqlStartResolve ({ context, resolverInfo }) { if (!resolverInfo || typeof resolverInfo !== 'object') return const wafResults = waf.run({ ephemeral: { [addresses.HTTP_INCOMING_GRAPHQL_RESOLVER]: resolverInfo } }, req) + const requestData = graphqlRequestData.get(req) - if (wafResults) { - const requestData = graphqlRequestData.get(req) + if (wafResults && requestData) { requestData.wafResults = wafResults const blockingAction = getBlockingAction(wafResults.actions) - if (blockingAction && requestData?.isInGraphqlRequest) { + if (blockingAction && requestData.isInGraphqlRequest) { requestData.blocked = true context?.abortController?.abort() } diff --git a/packages/dd-trace/test/appsec/graphql.spec.js b/packages/dd-trace/test/appsec/graphql.spec.js index 08201f7c497..4632bf8d122 100644 --- a/packages/dd-trace/test/appsec/graphql.spec.js +++ b/packages/dd-trace/test/appsec/graphql.spec.js @@ -246,9 +246,8 @@ describe('GraphQL', () => { const abortData = {} apolloChannel.asyncEnd.publish({ abortController, abortData }) - // [TODO] The implementation of this was not correct? expect(blocking.getBlockingData).to.have.been.calledOnceWithExactly( - req, 'graphql', { block_request: blockParameters }) + req, 'graphql', blockParameters) expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') }) From 0e46435edc9bc2be5d6331c52b2e63eee337c690 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 12:10:38 +0100 Subject: [PATCH 22/56] user tracking blocking action --- packages/dd-trace/src/appsec/sdk/user_blocking.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/user_blocking.js b/packages/dd-trace/src/appsec/sdk/user_blocking.js index 02d5b52f48d..fd0b6dc6851 100644 --- a/packages/dd-trace/src/appsec/sdk/user_blocking.js +++ b/packages/dd-trace/src/appsec/sdk/user_blocking.js @@ -3,7 +3,7 @@ const { USER_ID } = require('../addresses') const waf = require('../waf') const { getRootSpan } = require('./utils') -const { block } = require('../blocking') +const { block, getBlockingAction } = require('../blocking') const { storage } = require('../../../../datadog-core') const { setUserTags } = require('./set_user') const log = require('../../log') @@ -11,11 +11,14 @@ const { reportMetrics } = require('../reporter') function isUserBlocked (user) { const wafResults = waf.run({ persistent: { [USER_ID]: user.id } }) + if (!wafResults) return false + const blockTriggered = !!getBlockingAction(wafResults.actions) + reportMetrics(wafResults.metrics, null) - return wafResults.metrics.blockTriggered + return blockTriggered } function checkUserAndSetUser (tracer, user) { From da299300124ba85a747fa8dc35331c30e51a34d6 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 12:23:10 +0100 Subject: [PATCH 23/56] waf and rasp timeout --- packages/dd-trace/test/appsec/waf-rasp-errors.spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js index c93a2f60f2f..fd88bf363bb 100644 --- a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js @@ -10,7 +10,7 @@ describe('WAF/RASP - timeout', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc before(async function () { - this.timeout(process.platform === 'win32' ? 90000 : 30000) + this.timeout(90000) sandbox = await createSandbox( ['express'], @@ -31,7 +31,7 @@ describe('WAF/RASP - timeout', () => { }) after(async function () { - this.timeout(60000) + this.timeout(90000) await sandbox.remove() }) @@ -72,7 +72,7 @@ describe('WAF/RASP - error', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc before(async function () { - this.timeout(process.platform === 'win32' ? 90000 : 30000) + this.timeout(90000) sandbox = await createSandbox( ['express'], @@ -93,7 +93,7 @@ describe('WAF/RASP - error', () => { }) after(async function () { - this.timeout(60000) + this.timeout(90000) await sandbox.remove() }) From 9af7e1d2c29bdfe84861b0a7d1b1cf3099816be8 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 14:47:39 +0100 Subject: [PATCH 24/56] fix sql injection tests --- .../appsec/rasp/resources/shi-app/index.js | 9 +++++ .../rasp/sql_injection.pg.plugin.spec.js | 8 ++--- .../test/appsec/waf-rasp-errors.spec.js | 36 +++++++++++++++---- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js index 133c57dfb2b..84e2a893f88 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js +++ b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js @@ -6,9 +6,12 @@ tracer.init({ }) const express = require('express') +const bodyParser = require('body-parser') const childProcess = require('child_process') const app = express() +app.use(bodyParser.json()); + const port = process.env.APP_PORT || 3000 app.get('/shi/execFileSync', async (req, res) => { @@ -53,6 +56,12 @@ app.get('/cmdi/execFileSync/out-of-express-scope', async (req, res) => { }) }) +app.post('/shi/execFileSync', async (req, res) => { + childProcess.execFileSync('ls', [req.query.dir], { shell: true }) + + res.end('OK') +}) + app.listen(port, () => { process.send({ port }) }) diff --git a/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js b/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js index 2d4dd779c17..5cd46780826 100644 --- a/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js @@ -219,7 +219,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 1) + assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 1) }) it('should call to waf twice for sql injection with two different queries in pg Pool', async () => { @@ -232,7 +232,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 2) + assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 2) }) it('should call to waf twice for sql injection and same query when input address is updated', async () => { @@ -254,7 +254,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 2) + assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 2) }) it('should call to waf once for sql injection and same query when input address is updated', async () => { @@ -276,7 +276,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 1) + assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 1) }) }) }) diff --git a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js index fd88bf363bb..ab79cf72f7a 100644 --- a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js @@ -10,7 +10,7 @@ describe('WAF/RASP - timeout', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc before(async function () { - this.timeout(90000) + this.timeout(process.platform === 'win32' ? 90000 : 30000) sandbox = await createSandbox( ['express'], @@ -31,7 +31,7 @@ describe('WAF/RASP - timeout', () => { }) after(async function () { - this.timeout(90000) + this.timeout(60000) await sandbox.remove() }) @@ -57,13 +57,28 @@ describe('WAF/RASP - timeout', () => { }) it('Should not block since waf will timeout', async () => { - await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + const longValue = 'testattack'.repeat(500) + + const largeObject = {} + for (let i = 0; i < 300; ++i) { + largeObject[`key${i}`] = `value${i}` + } + + const deepObject = createNestedObject(25, { value: 'a' }) + + const complexPayload = { + deepObject, + longValue, + largeObject + } + + await axios.post('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)', complexPayload) await agent.assertMessageReceived(({ payload }) => { assert.property(payload[0][0].metrics, '_dd.appsec.rasp.timeout') - assert.equal(payload[0][0].metrics['_dd.appsec.rasp.timeout'], 1) + assert(payload[0][0].metrics['_dd.appsec.rasp.timeout'] >= 1) assert.property(payload[0][0].metrics, '_dd.appsec.waf.timeouts') - assert(payload[0][0].metrics['_dd.appsec.waf.timeouts'] > 1) + assert(payload[0][0].metrics['_dd.appsec.waf.timeouts'] >= 1) }) }) }) @@ -72,7 +87,7 @@ describe('WAF/RASP - error', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc before(async function () { - this.timeout(90000) + this.timeout(process.platform === 'win32' ? 90000 : 30000) sandbox = await createSandbox( ['express'], @@ -93,7 +108,7 @@ describe('WAF/RASP - error', () => { }) after(async function () { - this.timeout(90000) + this.timeout(60000) await sandbox.remove() }) @@ -129,3 +144,10 @@ describe('WAF/RASP - error', () => { }) }) }) + +const createNestedObject = (n, obj) => { + if (n > 0) { + return { a: createNestedObject(n - 1, obj) } + } + return obj +} From 1ea5a8007e146ea06553083fbaa2d2ce7fe5aa8c Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 14:52:33 +0100 Subject: [PATCH 25/56] add comment for waf timeout --- packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js | 2 +- packages/dd-trace/test/appsec/waf-rasp-errors.spec.js | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js index 84e2a893f88..a7c5cbc5eb1 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js +++ b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js @@ -10,7 +10,7 @@ const bodyParser = require('body-parser') const childProcess = require('child_process') const app = express() -app.use(bodyParser.json()); +app.use(bodyParser.json()) const port = process.env.APP_PORT || 3000 diff --git a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js index ab79cf72f7a..e2b19c79d78 100644 --- a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js @@ -56,6 +56,7 @@ describe('WAF/RASP - timeout', () => { await agent.stop() }) + // This test could be Flaky since the WAF could return result before 1ms it('Should not block since waf will timeout', async () => { const longValue = 'testattack'.repeat(500) From fdd94b6fc7ae79e45b151462ee5b6944ac04d121 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 15:14:41 +0100 Subject: [PATCH 26/56] keep only waf error test --- .../appsec/rasp/resources/shi-app/index.js | 9 -- .../test/appsec/waf-rasp-errors.spec.js | 85 ------------------- 2 files changed, 94 deletions(-) diff --git a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js index a7c5cbc5eb1..133c57dfb2b 100644 --- a/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js +++ b/packages/dd-trace/test/appsec/rasp/resources/shi-app/index.js @@ -6,12 +6,9 @@ tracer.init({ }) const express = require('express') -const bodyParser = require('body-parser') const childProcess = require('child_process') const app = express() -app.use(bodyParser.json()) - const port = process.env.APP_PORT || 3000 app.get('/shi/execFileSync', async (req, res) => { @@ -56,12 +53,6 @@ app.get('/cmdi/execFileSync/out-of-express-scope', async (req, res) => { }) }) -app.post('/shi/execFileSync', async (req, res) => { - childProcess.execFileSync('ls', [req.query.dir], { shell: true }) - - res.end('OK') -}) - app.listen(port, () => { process.send({ port }) }) diff --git a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js index e2b19c79d78..8298be81c21 100644 --- a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js @@ -6,84 +6,6 @@ const path = require('path') const Axios = require('axios') const { assert } = require('chai') -describe('WAF/RASP - timeout', () => { - let axios, sandbox, cwd, appPort, appFile, agent, proc - - before(async function () { - this.timeout(process.platform === 'win32' ? 90000 : 30000) - - sandbox = await createSandbox( - ['express'], - false, - [ - path.join(__dirname, 'rasp', 'resources'), - path.join(__dirname, '..', '..', 'src', 'appsec', 'recommended.json') - ] - ) - - appPort = await getPort() - cwd = sandbox.folder - appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') - - axios = Axios.create({ - baseURL: `http://localhost:${appPort}` - }) - }) - - after(async function () { - this.timeout(60000) - await sandbox.remove() - }) - - beforeEach(async () => { - agent = await new FakeAgent().start() - proc = await spawnProc(appFile, { - cwd, - env: { - DD_TRACE_AGENT_PORT: agent.port, - APP_PORT: appPort, - DD_APPSEC_ENABLED: 'true', - DD_APPSEC_RASP_ENABLED: 'true', - DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, - DD_APPSEC_WAF_TIMEOUT: 1, - DD_APPSEC_RULES: path.join(cwd, 'recommended.json') - } - }) - }) - - afterEach(async () => { - proc.kill() - await agent.stop() - }) - - // This test could be Flaky since the WAF could return result before 1ms - it('Should not block since waf will timeout', async () => { - const longValue = 'testattack'.repeat(500) - - const largeObject = {} - for (let i = 0; i < 300; ++i) { - largeObject[`key${i}`] = `value${i}` - } - - const deepObject = createNestedObject(25, { value: 'a' }) - - const complexPayload = { - deepObject, - longValue, - largeObject - } - - await axios.post('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)', complexPayload) - await agent.assertMessageReceived(({ payload }) => { - assert.property(payload[0][0].metrics, '_dd.appsec.rasp.timeout') - assert(payload[0][0].metrics['_dd.appsec.rasp.timeout'] >= 1) - - assert.property(payload[0][0].metrics, '_dd.appsec.waf.timeouts') - assert(payload[0][0].metrics['_dd.appsec.waf.timeouts'] >= 1) - }) - }) -}) - describe('WAF/RASP - error', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc @@ -145,10 +67,3 @@ describe('WAF/RASP - error', () => { }) }) }) - -const createNestedObject = (n, obj) => { - if (n > 0) { - return { a: createNestedObject(n - 1, obj) } - } - return obj -} From d57e33d0eec7c18a265f38c1bf031590c09f0d5c Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 19:09:11 +0100 Subject: [PATCH 27/56] fallback to dummy BlockList for cypress 6 --- .../dd-trace/src/plugins/util/ip_extractor.js | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/plugins/util/ip_extractor.js b/packages/dd-trace/src/plugins/util/ip_extractor.js index 26a1bc50b3b..7a0b78082fb 100644 --- a/packages/dd-trace/src/plugins/util/ip_extractor.js +++ b/packages/dd-trace/src/plugins/util/ip_extractor.js @@ -1,6 +1,5 @@ 'use strict' -const { BlockList } = require('net') const net = require('net') const ipHeaderList = [ @@ -28,12 +27,25 @@ const privateCIDRs = [ 'fd00::/8' ] -const privateIPMatcher = new BlockList() -for (const cidr of privateCIDRs) { - const [address, prefix] = cidr.split('/') - privateIPMatcher.addSubnet(address, parseInt(prefix), net.isIPv6(address) ? 'ipv6' : 'ipv4') +let privateIPMatcher + +try { + const { BlockList } = require('net') + privateIPMatcher = new BlockList() + + for (const cidr of privateCIDRs) { + const [address, prefix] = cidr.split('/') + + privateIPMatcher.addSubnet(address, parseInt(prefix), net.isIPv6(address) ? 'ipv6' : 'ipv4') + } +} catch (e) { + // Fallback for older Node.js versions without BlockList + privateIPMatcher = { + check: () => false, + addSubnet: () => {} + } } function extractIp (config, req) { From 4a44a8616d8556b3bff7185772a12317a0e55cdc Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 27 Feb 2025 19:20:43 +0100 Subject: [PATCH 28/56] fix linter --- .../dd-trace/src/plugins/util/ip_extractor.js | 2 - .../test/appsec/waf-rasp-errors.spec.js | 69 ------------------- 2 files changed, 71 deletions(-) delete mode 100644 packages/dd-trace/test/appsec/waf-rasp-errors.spec.js diff --git a/packages/dd-trace/src/plugins/util/ip_extractor.js b/packages/dd-trace/src/plugins/util/ip_extractor.js index 7a0b78082fb..27a8a684161 100644 --- a/packages/dd-trace/src/plugins/util/ip_extractor.js +++ b/packages/dd-trace/src/plugins/util/ip_extractor.js @@ -27,8 +27,6 @@ const privateCIDRs = [ 'fd00::/8' ] - - let privateIPMatcher try { diff --git a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js b/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js deleted file mode 100644 index 8298be81c21..00000000000 --- a/packages/dd-trace/test/appsec/waf-rasp-errors.spec.js +++ /dev/null @@ -1,69 +0,0 @@ -'use strict' - -const { createSandbox, FakeAgent, spawnProc } = require('../../../../integration-tests/helpers') -const getPort = require('get-port') -const path = require('path') -const Axios = require('axios') -const { assert } = require('chai') - -describe('WAF/RASP - error', () => { - let axios, sandbox, cwd, appPort, appFile, agent, proc - - before(async function () { - this.timeout(process.platform === 'win32' ? 90000 : 30000) - - sandbox = await createSandbox( - ['express'], - false, - [ - path.join(__dirname, 'rasp', 'resources'), - path.join(__dirname, '..', '..', 'src', 'appsec', 'recommended.json') - ] - ) - - appPort = await getPort() - cwd = sandbox.folder - appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') - - axios = Axios.create({ - baseURL: `http://localhost:${appPort}` - }) - }) - - after(async function () { - this.timeout(60000) - await sandbox.remove() - }) - - beforeEach(async () => { - agent = await new FakeAgent().start() - proc = await spawnProc(appFile, { - cwd, - env: { - DD_TRACE_AGENT_PORT: agent.port, - APP_PORT: appPort, - DD_APPSEC_ENABLED: 'true', - DD_APPSEC_RASP_ENABLED: 'true', - DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, - DD_APPSEC_WAF_TIMEOUT: 0.1, - DD_APPSEC_RULES: path.join(cwd, 'recommended.json') - } - }) - }) - - afterEach(async () => { - proc.kill() - await agent.stop() - }) - - it('Should not block since waf will return error', async () => { - await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') - await agent.assertMessageReceived(({ payload }) => { - assert.property(payload[0][0].metrics, '_dd.appsec.rasp.error') - assert.equal(payload[0][0].metrics['_dd.appsec.rasp.error'], -127) - - assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') - assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) - }) - }) -}) From e51917cd89e83bb476d42075b7ac8864e3b5554c Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 28 Feb 2025 10:46:46 +0100 Subject: [PATCH 29/56] add waf error test --- packages/dd-trace/src/appsec/rasp/index.js | 19 ++++-- packages/dd-trace/test/appsec/graphql.spec.js | 3 +- .../dd-trace/test/appsec/rasp/lfi.spec.js | 2 +- .../dd-trace/test/appsec/waf-errors.spec.js | 64 +++++++++++++++++++ 4 files changed, 80 insertions(+), 8 deletions(-) create mode 100644 packages/dd-trace/test/appsec/waf-errors.spec.js diff --git a/packages/dd-trace/src/appsec/rasp/index.js b/packages/dd-trace/src/appsec/rasp/index.js index 1ac944d027b..02de270b661 100644 --- a/packages/dd-trace/src/appsec/rasp/index.js +++ b/packages/dd-trace/src/appsec/rasp/index.js @@ -7,6 +7,7 @@ const ssrf = require('./ssrf') const sqli = require('./sql_injection') const lfi = require('./lfi') const cmdi = require('./command_injection') +const log = require('../../log') const { DatadogRaspAbortError } = require('./utils') @@ -86,13 +87,21 @@ function blockOnDatadogRaspAbortError ({ error }) { const { req, res, blockingAction } = abortError if (!isBlocked(res)) { - block(req, res, null, blockingAction) - const rootSpan = web.root(req) - rootSpan?.addTags({ - 'appsec.blocked': 'true' - }) + try { + block(req, res, null, blockingAction) + + rootSpan?.addTags({ + 'appsec.blocked': 'true' + }) + } catch (err) { + rootSpan?.addTags({ + '_dd.appsec.block.failed': 1 + }) + + log.error('[ASM] Blocking error', err) + } } return true diff --git a/packages/dd-trace/test/appsec/graphql.spec.js b/packages/dd-trace/test/appsec/graphql.spec.js index 4632bf8d122..ea37f8ef504 100644 --- a/packages/dd-trace/test/appsec/graphql.spec.js +++ b/packages/dd-trace/test/appsec/graphql.spec.js @@ -246,8 +246,7 @@ 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') }) diff --git a/packages/dd-trace/test/appsec/rasp/lfi.spec.js b/packages/dd-trace/test/appsec/rasp/lfi.spec.js index 0a180596303..4353ffcc243 100644 --- a/packages/dd-trace/test/appsec/rasp/lfi.spec.js +++ b/packages/dd-trace/test/appsec/rasp/lfi.spec.js @@ -16,7 +16,7 @@ describe('RASP - lfi.js', () => { waf = { run: sinon.stub().returns({ - result: { + metrics: { totalRuntime: 30, timeout: false } diff --git a/packages/dd-trace/test/appsec/waf-errors.spec.js b/packages/dd-trace/test/appsec/waf-errors.spec.js new file mode 100644 index 00000000000..53e1fdda323 --- /dev/null +++ b/packages/dd-trace/test/appsec/waf-errors.spec.js @@ -0,0 +1,64 @@ +'use strict' + +const { createSandbox, FakeAgent, spawnProc } = require('../../../../integration-tests/helpers') +const getPort = require('get-port') +const path = require('path') +const Axios = require('axios') +const { assert } = require('chai') + +describe('WAF - error', () => { + let axios, sandbox, cwd, appPort, appFile, agent, proc + + before(async function () { + this.timeout(process.platform === 'win32' ? 90000 : 30000) + + sandbox = await createSandbox( + ['express'], + false, + [path.join(__dirname, 'resources')] + ) + + appPort = await getPort() + cwd = sandbox.folder + appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') + + axios = Axios.create({ + baseURL: `http://localhost:${appPort}` + }) + }) + + after(async function () { + this.timeout(60000) + await sandbox.remove() + }) + + beforeEach(async () => { + agent = await new FakeAgent().start() + proc = await spawnProc(appFile, { + cwd, + env: { + DD_TRACE_AGENT_PORT: agent.port, + DD_TRACE_DEBUG: 'true', + APP_PORT: appPort, + DD_APPSEC_ENABLED: 'true', + DD_APPSEC_RASP_ENABLED: 'true', + DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, + DD_APPSEC_WAF_TIMEOUT: 0.1, + DD_APPSEC_RULES: path.join(cwd, 'resources', 'rasp_rules.json') + } + }) + }) + + afterEach(async () => { + proc.kill() + await agent.stop() + }) + + it('Should not block since waf will return error', async () => { + await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + await agent.assertMessageReceived(({ payload }) => { + assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') + assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) + }) + }) +}) From 2a141b5b9fe44bd964bbbb68622b51d417db89a4 Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 28 Feb 2025 10:54:22 +0100 Subject: [PATCH 30/56] fix rasp resources path --- packages/dd-trace/test/appsec/waf-errors.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/waf-errors.spec.js b/packages/dd-trace/test/appsec/waf-errors.spec.js index 53e1fdda323..256b4c99a67 100644 --- a/packages/dd-trace/test/appsec/waf-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-errors.spec.js @@ -15,7 +15,7 @@ describe('WAF - error', () => { sandbox = await createSandbox( ['express'], false, - [path.join(__dirname, 'resources')] + [path.join(__dirname, 'rasp', 'resources')] ) appPort = await getPort() From a7617a344e27f2ea3e63e74b401c55b4c56faf13 Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 28 Feb 2025 11:15:06 +0100 Subject: [PATCH 31/56] fix waf error on windows --- .../dd-trace/test/appsec/waf-errors.spec.js | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/test/appsec/waf-errors.spec.js b/packages/dd-trace/test/appsec/waf-errors.spec.js index 256b4c99a67..e85f8d61893 100644 --- a/packages/dd-trace/test/appsec/waf-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-errors.spec.js @@ -55,10 +55,22 @@ describe('WAF - error', () => { }) it('Should not block since waf will return error', async () => { - await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') - await agent.assertMessageReceived(({ payload }) => { - assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') - assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) - }) + try { + await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') + + if (process.platform !== 'win32') { + await agent.assertMessageReceived(({ payload }) => { + assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') + assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) + }) + } + } catch (err) { + if (process.platform === 'win32') { + await agent.assertMessageReceived(({ payload }) => { + assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') + assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) + }) + } + } }) }) From 5c2df8e6d50285114617daf69613d3a642146446 Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 28 Feb 2025 11:35:38 +0100 Subject: [PATCH 32/56] fix waf error on windows path --- packages/dd-trace/test/appsec/waf-errors.spec.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/waf-errors.spec.js b/packages/dd-trace/test/appsec/waf-errors.spec.js index e85f8d61893..d40b75d63d9 100644 --- a/packages/dd-trace/test/appsec/waf-errors.spec.js +++ b/packages/dd-trace/test/appsec/waf-errors.spec.js @@ -6,7 +6,7 @@ const path = require('path') const Axios = require('axios') const { assert } = require('chai') -describe('WAF - error', () => { +describe.only('WAF - error', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc before(async function () { @@ -56,6 +56,10 @@ describe('WAF - error', () => { it('Should not block since waf will return error', async () => { try { + process.platform === 'win32' + ? '/shi/execFileSync?dir=type%20C:\\Windows\\System32\\drivers\\etc\\hosts' + : '/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)' + await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') if (process.platform !== 'win32') { @@ -70,6 +74,9 @@ describe('WAF - error', () => { assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) }) + } else { + // If we're on Linux and actually got an error, rethrow + throw err } } }) From b321418efa4cadc833f6693ead90fc0799b97c1f Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 28 Feb 2025 11:41:30 +0100 Subject: [PATCH 33/56] remove waf errors test file --- .../dd-trace/test/appsec/waf-errors.spec.js | 83 ------------------- 1 file changed, 83 deletions(-) delete mode 100644 packages/dd-trace/test/appsec/waf-errors.spec.js diff --git a/packages/dd-trace/test/appsec/waf-errors.spec.js b/packages/dd-trace/test/appsec/waf-errors.spec.js deleted file mode 100644 index d40b75d63d9..00000000000 --- a/packages/dd-trace/test/appsec/waf-errors.spec.js +++ /dev/null @@ -1,83 +0,0 @@ -'use strict' - -const { createSandbox, FakeAgent, spawnProc } = require('../../../../integration-tests/helpers') -const getPort = require('get-port') -const path = require('path') -const Axios = require('axios') -const { assert } = require('chai') - -describe.only('WAF - error', () => { - let axios, sandbox, cwd, appPort, appFile, agent, proc - - before(async function () { - this.timeout(process.platform === 'win32' ? 90000 : 30000) - - sandbox = await createSandbox( - ['express'], - false, - [path.join(__dirname, 'rasp', 'resources')] - ) - - appPort = await getPort() - cwd = sandbox.folder - appFile = path.join(cwd, 'resources', 'shi-app', 'index.js') - - axios = Axios.create({ - baseURL: `http://localhost:${appPort}` - }) - }) - - after(async function () { - this.timeout(60000) - await sandbox.remove() - }) - - beforeEach(async () => { - agent = await new FakeAgent().start() - proc = await spawnProc(appFile, { - cwd, - env: { - DD_TRACE_AGENT_PORT: agent.port, - DD_TRACE_DEBUG: 'true', - APP_PORT: appPort, - DD_APPSEC_ENABLED: 'true', - DD_APPSEC_RASP_ENABLED: 'true', - DD_TELEMETRY_HEARTBEAT_INTERVAL: 1, - DD_APPSEC_WAF_TIMEOUT: 0.1, - DD_APPSEC_RULES: path.join(cwd, 'resources', 'rasp_rules.json') - } - }) - }) - - afterEach(async () => { - proc.kill() - await agent.stop() - }) - - it('Should not block since waf will return error', async () => { - try { - process.platform === 'win32' - ? '/shi/execFileSync?dir=type%20C:\\Windows\\System32\\drivers\\etc\\hosts' - : '/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)' - - await axios.get('/shi/execFileSync?dir=$(cat /etc/passwd 1>%262 ; echo .)') - - if (process.platform !== 'win32') { - await agent.assertMessageReceived(({ payload }) => { - assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') - assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) - }) - } - } catch (err) { - if (process.platform === 'win32') { - await agent.assertMessageReceived(({ payload }) => { - assert.property(payload[0][0].metrics, '_dd.appsec.waf.error') - assert.equal(payload[0][0].metrics['_dd.appsec.waf.error'], -127) - }) - } else { - // If we're on Linux and actually got an error, rethrow - throw err - } - } - }) -}) From 327f239ade725b3c59bcae659b6ec84adc04090d Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 3 Mar 2025 11:08:38 +0100 Subject: [PATCH 34/56] report waf metrics on api security schema extraction --- packages/dd-trace/src/appsec/index.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 0dd7401840d..36c94d4d3a6 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -34,7 +34,6 @@ const UserTracking = require('./user_tracking') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') const rasp = require('./rasp') -const { reportMetrics } = require('./reporter') const responseAnalyzedSet = new WeakSet() @@ -180,7 +179,9 @@ function incomingHttpEndTranslator ({ req, res }) { } if (Object.keys(persistent).length) { - waf.run({ persistent }, req) + const wafResults = waf.run({ persistent }, req) + + Reporter.reportMetrics(wafResults?.metrics, null) } waf.disposeContext(req) @@ -281,7 +282,7 @@ function onResponseBody ({ req, res, body }) { } }, req) - reportMetrics(results?.metrics, null) + Reporter.reportMetrics(results?.metrics, null) } function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) { @@ -345,7 +346,7 @@ function handleResults (wafResults, req, res, rootSpan, abortController) { } } - reportMetrics(metrics, null) + Reporter.reportMetrics(metrics, null) } function disable () { From d0d72c583920379430fddce2d8382b390cac98d1 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 09:52:23 +0100 Subject: [PATCH 35/56] report metrics inside run waf --- packages/dd-trace/src/appsec/blocking.js | 38 +++--- packages/dd-trace/src/appsec/graphql.js | 24 ++-- packages/dd-trace/src/appsec/index.js | 43 ++----- .../src/appsec/rasp/command_injection.js | 4 +- packages/dd-trace/src/appsec/rasp/index.js | 16 +-- packages/dd-trace/src/appsec/rasp/lfi.js | 4 +- .../dd-trace/src/appsec/rasp/sql_injection.js | 4 +- packages/dd-trace/src/appsec/rasp/ssrf.js | 4 +- packages/dd-trace/src/appsec/rasp/utils.js | 9 +- packages/dd-trace/src/appsec/reporter.js | 2 - packages/dd-trace/src/appsec/sdk/set_user.js | 6 +- .../dd-trace/src/appsec/sdk/track_event.js | 7 +- packages/dd-trace/src/appsec/waf/index.js | 4 +- .../src/appsec/waf/waf_context_wrapper.js | 28 ++--- .../dd-trace/src/plugins/util/ip_extractor.js | 20 +-- .../dd-trace/test/appsec/blocking.spec.js | 117 ++++++++++-------- packages/dd-trace/test/appsec/graphql.spec.js | 5 +- packages/dd-trace/test/appsec/index.spec.js | 18 +-- .../appsec/rasp/command_injection.spec.js | 22 ++-- .../dd-trace/test/appsec/rasp/lfi.spec.js | 9 +- .../rasp/sql_injection.pg.plugin.spec.js | 8 +- .../test/appsec/rasp/sql_injection.spec.js | 4 +- .../dd-trace/test/appsec/rasp/ssrf.spec.js | 2 +- .../dd-trace/test/appsec/rasp/utils.spec.js | 66 +++------- .../dd-trace/test/appsec/reporter.spec.js | 2 + .../dd-trace/test/appsec/waf/index.spec.js | 90 ++++++++++++-- 26 files changed, 263 insertions(+), 293 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 6b4a1be5cd2..9583599c01e 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -99,27 +99,37 @@ function getBlockingData (req, specificType, actionParameters) { } } -function block (req, res, abortController, actionParameters = defaultBlockingActionParameters) { - if (res.headersSent) { - log.warn('[ASM] Cannot send blocking response when headers have already been sent') +function block (req, res, rootSpan, abortController, actionParameters = defaultBlockingActionParameters) { + try { + if (res.headersSent) { + log.warn('[ASM] Cannot send blocking response when headers have already been sent') - throw new Error('Headers have already been sent') - } + throw new Error('Headers have already been sent') + } - const { body, headers, statusCode } = getBlockingData(req, null, actionParameters) + const { body, headers, statusCode } = getBlockingData(req, null, actionParameters) - for (const headerName of res.getHeaderNames()) { - res.removeHeader(headerName) - } + for (const headerName of res.getHeaderNames()) { + res.removeHeader(headerName) + } - res.writeHead(statusCode, headers) + res.writeHead(statusCode, headers) - // this is needed to call the original end method, since express-session replaces it - res.constructor.prototype.end.call(res, body) + // this is needed to call the original end method, since express-session replaces it + res.constructor.prototype.end.call(res, body) - responseBlockedSet.add(res) + responseBlockedSet.add(res) + abortController?.abort() - abortController?.abort() + rootSpan.addTags({ + 'appsec.blocked': 'true' + }) + } catch (err) { + rootSpan?.addTags({ + '_dd.appsec.block.failed': 1 + }) + log.error('[ASM] Blocking error', err) + } } function getBlockingAction (actions) { diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index 00eb2f200dc..d2675e85673 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -7,7 +7,6 @@ const { getBlockingData, getBlockingAction } = require('./blocking') -const { reportMetrics } = require('./reporter') const log = require('../log') const waf = require('./waf') const addresses = require('./addresses') @@ -38,15 +37,13 @@ function onGraphqlStartResolve ({ context, resolverInfo }) { if (!resolverInfo || typeof resolverInfo !== 'object') return - const wafResults = waf.run({ ephemeral: { [addresses.HTTP_INCOMING_GRAPHQL_RESOLVER]: resolverInfo } }, req) - const requestData = graphqlRequestData.get(req) - - if (wafResults && requestData) { - requestData.wafResults = wafResults - const blockingAction = getBlockingAction(wafResults.actions) - - if (blockingAction && requestData.isInGraphqlRequest) { + const actions = waf.run({ ephemeral: { [addresses.HTTP_INCOMING_GRAPHQL_RESOLVER]: resolverInfo } }, req) + const blockingAction = getBlockingAction(actions) + if (blockingAction) { + const requestData = graphqlRequestData.get(req) + if (requestData?.isInGraphqlRequest) { requestData.blocked = true + requestData.wafAction = blockingAction context?.abortController?.abort() } } @@ -99,15 +96,14 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { if (!rootSpan) return try { - const blockingAction = getBlockingAction(requestData.wafResults.actions) - const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, blockingAction) + const blockingData = getBlockingData(req, specificBlockingTypes.GRAPHQL, requestData.wafAction) abortData.statusCode = blockingData.statusCode abortData.headers = blockingData.headers abortData.message = blockingData.body - abortController.abort() - rootSpan.setTag('appsec.blocked', 'true') + + abortController?.abort() } catch (err) { rootSpan.addTags({ '_dd.appsec.block.failed': 1 @@ -117,8 +113,6 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { } } - reportMetrics(requestData.wafResults.metrics, null) - graphqlRequestData.delete(req) } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 36c94d4d3a6..cc7eef68008 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -149,9 +149,9 @@ function incomingHttpStartTranslator ({ req, res, abortController }) { persistent[addresses.HTTP_CLIENT_IP] = clientIp } - const results = waf.run({ persistent }, req) + const actions = waf.run({ persistent }, req) - handleResults(results, req, res, rootSpan, abortController) + handleResults(actions, req, res, rootSpan, abortController) } function incomingHttpEndTranslator ({ req, res }) { @@ -179,9 +179,7 @@ function incomingHttpEndTranslator ({ req, res }) { } if (Object.keys(persistent).length) { - const wafResults = waf.run({ persistent }, req) - - Reporter.reportMetrics(wafResults?.metrics, null) + waf.run({ persistent }, req) } waf.disposeContext(req) @@ -275,14 +273,12 @@ function onResponseBody ({ req, res, body }) { if (!body || typeof body !== 'object') return if (!apiSecuritySampler.sampleRequest(req, res)) return - // we don't support blocking at this point, so no results needed only reporting - const results = waf.run({ + // we don't support blocking at this point, so no results needed + waf.run({ persistent: { [addresses.HTTP_INCOMING_RESPONSE_BODY]: body } }, req) - - Reporter.reportMetrics(results?.metrics, null) } function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) { @@ -321,32 +317,13 @@ function onResponseSetHeader ({ res, abortController }) { } } -function handleResults (wafResults, req, res, rootSpan, abortController) { - if (!wafResults) return - - const { actions, metrics } = wafResults - - const blockTriggered = getBlockingAction(actions) +function handleResults (actions, req, res, rootSpan, abortController) { + if (!actions || !req || !res || !rootSpan || !abortController) return - const canBlock = actions && req && res && rootSpan && abortController - - if (canBlock && blockTriggered) { - try { - block(req, res, abortController, blockTriggered) - - rootSpan.addTags({ - 'appsec.blocked': 'true' - }) - } catch (err) { - rootSpan.addTags({ - '_dd.appsec.block.failed': 1 - }) - - log.error('[ASM] Blocking error', err) - } + const blockingAction = getBlockingAction(actions) + if (blockingAction) { + block(req, res, rootSpan, abortController, blockingAction) } - - Reporter.reportMetrics(metrics, null) } function disable () { diff --git a/packages/dd-trace/src/appsec/rasp/command_injection.js b/packages/dd-trace/src/appsec/rasp/command_injection.js index 47139a3bce4..eea0af5d22d 100644 --- a/packages/dd-trace/src/appsec/rasp/command_injection.js +++ b/packages/dd-trace/src/appsec/rasp/command_injection.js @@ -44,10 +44,10 @@ function analyzeCommandInjection ({ file, fileArgs, shell, abortController }) { raspRule.variant = 'exec' } - const result = waf.run({ ephemeral }, req) + const result = waf.run({ ephemeral }, req, raspRule) const res = store?.res - handleResult(result, req, res, abortController, config, raspRule) + handleResult(result, req, res, abortController, config) } module.exports = { diff --git a/packages/dd-trace/src/appsec/rasp/index.js b/packages/dd-trace/src/appsec/rasp/index.js index 02de270b661..c00c6493487 100644 --- a/packages/dd-trace/src/appsec/rasp/index.js +++ b/packages/dd-trace/src/appsec/rasp/index.js @@ -87,21 +87,7 @@ function blockOnDatadogRaspAbortError ({ error }) { const { req, res, blockingAction } = abortError if (!isBlocked(res)) { - const rootSpan = web.root(req) - - try { - block(req, res, null, blockingAction) - - rootSpan?.addTags({ - 'appsec.blocked': 'true' - }) - } catch (err) { - rootSpan?.addTags({ - '_dd.appsec.block.failed': 1 - }) - - log.error('[ASM] Blocking error', err) - } + block(req, res, web.root(req), null, blockingAction) } return true diff --git a/packages/dd-trace/src/appsec/rasp/lfi.js b/packages/dd-trace/src/appsec/rasp/lfi.js index 867ae03a8f5..87c82175ac1 100644 --- a/packages/dd-trace/src/appsec/rasp/lfi.js +++ b/packages/dd-trace/src/appsec/rasp/lfi.js @@ -60,8 +60,8 @@ function analyzeLfi (ctx) { const raspRule = { type: RULE_TYPES.LFI } - const result = waf.run({ ephemeral }, req) - handleResult(result, req, res, ctx.abortController, config, raspRule) + const result = waf.run({ ephemeral }, req, raspRule) + handleResult(result, req, res, ctx.abortController, config) }) } diff --git a/packages/dd-trace/src/appsec/rasp/sql_injection.js b/packages/dd-trace/src/appsec/rasp/sql_injection.js index 7c6ea8275ec..8da179bcfe8 100644 --- a/packages/dd-trace/src/appsec/rasp/sql_injection.js +++ b/packages/dd-trace/src/appsec/rasp/sql_injection.js @@ -74,9 +74,9 @@ function analyzeSqlInjection (query, dbSystem, abortController) { const raspRule = { type: RULE_TYPES.SQL_INJECTION } - const result = waf.run({ ephemeral }, req) + const result = waf.run({ ephemeral }, req, raspRule) - handleResult(result, req, res, abortController, config, raspRule) + handleResult(result, req, res, abortController, config) } function hasInputAddress (payload) { diff --git a/packages/dd-trace/src/appsec/rasp/ssrf.js b/packages/dd-trace/src/appsec/rasp/ssrf.js index d24b857f526..d0d75f16c60 100644 --- a/packages/dd-trace/src/appsec/rasp/ssrf.js +++ b/packages/dd-trace/src/appsec/rasp/ssrf.js @@ -31,10 +31,10 @@ function analyzeSsrf (ctx) { const raspRule = { type: RULE_TYPES.SSRF } - const result = waf.run({ ephemeral }, req) + const result = waf.run({ ephemeral }, req, raspRule) const res = store?.res - handleResult(result, req, res, ctx.abortController, config, raspRule) + handleResult(result, req, res, ctx.abortController, config) } module.exports = { enable, disable } diff --git a/packages/dd-trace/src/appsec/rasp/utils.js b/packages/dd-trace/src/appsec/rasp/utils.js index 0ae76724ab7..f3426159786 100644 --- a/packages/dd-trace/src/appsec/rasp/utils.js +++ b/packages/dd-trace/src/appsec/rasp/utils.js @@ -3,7 +3,6 @@ const web = require('../../plugins/util/web') const { getCallsiteFrames, reportStackTrace, canReportStackTrace } = require('../stack_trace') const { getBlockingAction } = require('../blocking') -const { reportMetrics } = require('../reporter') const log = require('../../log') const abortOnUncaughtException = process.execArgv?.includes('--abort-on-uncaught-exception') @@ -29,11 +28,7 @@ class DatadogRaspAbortError extends Error { } } -function handleResult (wafResults, req, res, abortController, config, raspRule) { - if (!wafResults) return - - const { actions, metrics } = wafResults - +function handleResult (actions, req, res, abortController, config) { const generateStackTraceAction = actions?.generate_stack const { enabled, maxDepth, maxStackTraces } = config.appsec.stackTrace @@ -50,8 +45,6 @@ function handleResult (wafResults, req, res, abortController, config, raspRule) ) } - reportMetrics(metrics, raspRule) - if (!abortController || abortOnUncaughtException) return const blockingAction = getBlockingAction(actions) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 69a2760e4d5..308f978bf78 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -101,8 +101,6 @@ function reportWafInit (wafVersion, rulesVersion, diagnosticsRules = {}) { } function reportMetrics (metrics, raspRule) { - if (!metrics) return - const store = storage('legacy').getStore() const rootSpan = store?.req && web.root(store.req) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 93f78336cd6..bc7b0281b03 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -4,7 +4,6 @@ const { getRootSpan } = require('./utils') const log = require('../../log') const waf = require('../waf') const addresses = require('../addresses') -const { reportMetrics } = require('../reporter') function setUserTags (user, rootSpan) { for (const k of Object.keys(user)) { @@ -35,10 +34,7 @@ function setUser (tracer, user) { persistent[addresses.USER_SESSION_ID] = user.session_id } - const wafResults = waf.run({ persistent }) - if (!wafResults) return - - reportMetrics(wafResults.metrics, null) + waf.run({ persistent }) } module.exports = { diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index 30b506bfd6b..a54829d0a2b 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -6,7 +6,6 @@ const { setUserTags } = require('./set_user') const waf = require('../waf') const { keepTrace } = require('../../priority_sampler') const addresses = require('../addresses') -const { reportMetrics } = require('../reporter') const { ASM } = require('../../standalone/product') function trackUserLoginSuccessEvent (tracer, user, metadata) { @@ -95,11 +94,7 @@ function runWaf (eventName, user) { persistent[addresses.USER_LOGIN] = '' + user.login } - const wafResults = waf.run({ persistent }) - - if (!wafResults) return - - reportMetrics(wafResults, null) + waf.run({ persistent }) } module.exports = { diff --git a/packages/dd-trace/src/appsec/waf/index.js b/packages/dd-trace/src/appsec/waf/index.js index 596cbb9a17e..b025a123f46 100644 --- a/packages/dd-trace/src/appsec/waf/index.js +++ b/packages/dd-trace/src/appsec/waf/index.js @@ -46,7 +46,7 @@ function update (newRules) { } } -function run (data, req) { +function run (data, req, raspRule) { if (!req) { const store = storage('legacy').getStore() if (!store || !store.req) { @@ -59,7 +59,7 @@ function run (data, req) { const wafContext = waf.wafManager.getWAFContext(req) - return wafContext.run(data) + return wafContext.run(data, raspRule) } function disposeContext (req) { 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 d039f09081d..07a42413a43 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -22,7 +22,7 @@ class WAFContextWrapper { this.cachedUserIdActions = new Map() } - run ({ persistent, ephemeral }) { + run ({ persistent, ephemeral }, raspRule) { if (this.ddwafContext.disposed) { log.warn('[ASM] Calling run on a disposed context') return @@ -34,9 +34,7 @@ class WAFContextWrapper { if (userId) { const cachedAction = this.cachedUserIdActions.get(userId) if (cachedAction) { - return { - actions: cachedAction - } + return cachedAction } } @@ -101,10 +99,7 @@ class WAFContextWrapper { // Binding or other waf unexpected errors metrics.errorCode = -127 - return { - actions: null, - metrics - } + return null } else { if (typeof result.errorCode === 'number' && result.errorCode < 0) { metrics.errorCode = result.errorCode @@ -137,21 +132,20 @@ class WAFContextWrapper { wafRunFinished.publish({ payload }) } - if (ruleTriggered) { - Reporter.reportAttack(JSON.stringify(result.events)) - } - - Reporter.reportDerivatives(result.derivatives) - metrics.duration = result.totalRuntime / 1e3 metrics.blockTriggered = blockTriggered metrics.ruleTriggered = ruleTriggered metrics.wafTimeout = result.timeout - return { - actions: result.actions, - metrics + Reporter.reportMetrics(metrics, raspRule) + + if (ruleTriggered) { + Reporter.reportAttack(JSON.stringify(result.events)) } + + Reporter.reportDerivatives(result.derivatives) + + return result.actions } runWaf (payload) { diff --git a/packages/dd-trace/src/plugins/util/ip_extractor.js b/packages/dd-trace/src/plugins/util/ip_extractor.js index 27a8a684161..26a1bc50b3b 100644 --- a/packages/dd-trace/src/plugins/util/ip_extractor.js +++ b/packages/dd-trace/src/plugins/util/ip_extractor.js @@ -1,5 +1,6 @@ 'use strict' +const { BlockList } = require('net') const net = require('net') const ipHeaderList = [ @@ -27,23 +28,12 @@ const privateCIDRs = [ 'fd00::/8' ] -let privateIPMatcher +const privateIPMatcher = new BlockList() -try { - const { BlockList } = require('net') - privateIPMatcher = new BlockList() +for (const cidr of privateCIDRs) { + const [address, prefix] = cidr.split('/') - for (const cidr of privateCIDRs) { - const [address, prefix] = cidr.split('/') - - privateIPMatcher.addSubnet(address, parseInt(prefix), net.isIPv6(address) ? 'ipv6' : 'ipv4') - } -} catch (e) { - // Fallback for older Node.js versions without BlockList - privateIPMatcher = { - check: () => false, - addSubnet: () => {} - } + privateIPMatcher.addSubnet(address, parseInt(prefix), net.isIPv6(address) ? 'ipv6' : 'ipv4') } function extractIp (config, req) { diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 64251e01c74..9575e1cf470 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -15,7 +15,7 @@ describe('blocking', () => { let log let block, setTemplates - let req, res + let req, res, rootSpan beforeEach(() => { log = { @@ -45,6 +45,10 @@ describe('blocking', () => { } } } + + rootSpan = { + addTags: sinon.stub() + } }) describe('block', () => { @@ -54,7 +58,6 @@ describe('blocking', () => { it('should log warn and not send blocking response when headers have already been sent', () => { res.headersSent = true - try { block(req, res) } catch (error) { @@ -63,14 +66,16 @@ describe('blocking', () => { expect(log.warn).to.have.been .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') + expect(rootSpan.addTags).to.not.have.been.called expect(res.setHeader).to.not.have.been.called expect(res.constructor.prototype.end).to.not.have.been.called }) it('should send blocking response with html type if present in the headers', () => { req.headers.accept = 'text/html' - block(req, res) + block(req, res, rootSpan) + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'text/html; charset=utf-8', 'Content-Length': 12 @@ -80,8 +85,9 @@ describe('blocking', () => { it('should send blocking response with json type if present in the headers in priority', () => { req.headers.accept = 'text/html, application/xhtml+xml, application/xml;q=0.9, */*;q=0.8, application/json' - block(req, res) + block(req, res, rootSpan) + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -90,8 +96,9 @@ describe('blocking', () => { }) it('should send blocking response with json type if neither html or json is present in the headers', () => { - block(req, res) + block(req, res, rootSpan) + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -101,8 +108,9 @@ describe('blocking', () => { it('should send blocking response and call abortController if passed in arguments', () => { const abortController = new AbortController() - block(req, res, abortController) + block(req, res, rootSpan, abortController) + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -114,8 +122,9 @@ describe('blocking', () => { it('should remove all headers before sending blocking response', () => { res.getHeaderNames.returns(['header1', 'header2']) - block(req, res) + block(req, res, rootSpan) + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.removeHeader).to.have.been.calledTwice expect(res.removeHeader.firstCall).to.have.been.calledWithExactly('header1') expect(res.removeHeader.secondCall).to.have.been.calledWithExactly('header2') @@ -139,7 +148,7 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res) + block(req, res, rootSpan) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) @@ -147,7 +156,7 @@ describe('blocking', () => { it('should block with default json template', () => { setTemplates(config) - block(req, res) + block(req, res, rootSpan) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) @@ -169,41 +178,41 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, null, actionParameters) + block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with default json template and custom status ' + - 'when type is forced to json and accept is html', () => { - const actionParameters = { - status_code: 401, - type: 'json' - } - req.headers.accept = 'text/html' - setTemplates(config) + 'when type is forced to json and accept is html', () => { + const actionParameters = { + status_code: 401, + type: 'json' + } + req.headers.accept = 'text/html' + setTemplates(config) - block(req, res, null, actionParameters) + block(req, res, rootSpan, null, actionParameters) - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) - }) + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + }) it('should block with default html template and custom status ' + - 'when type is forced to html and accept is html', () => { - const actionParameters = { - status_code: 401, - type: 'html' - } - req.headers.accept = 'text/html' - setTemplates(config) + 'when type is forced to html and accept is html', () => { + const actionParameters = { + status_code: 401, + type: 'html' + } + req.headers.accept = 'text/html' + setTemplates(config) - block(req, res, null, actionParameters) + block(req, res, rootSpan, null, actionParameters) - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) - }) + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + }) it('should block with default json template and custom status', () => { const actionParameters = { @@ -212,39 +221,39 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, null, actionParameters) + block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) it('should block with default json template and custom status ' + - 'when type is forced to json and accept is not defined', () => { - const actionParameters = { - status_code: 401, - type: 'json' - } - setTemplates(config) + 'when type is forced to json and accept is not defined', () => { + const actionParameters = { + status_code: 401, + type: 'json' + } + setTemplates(config) - block(req, res, null, actionParameters) + block(req, res, rootSpan, null, actionParameters) - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) - }) + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + }) it('should block with default html template and custom status ' + - 'when type is forced to html and accept is not defined', () => { - const actionParameters = { - status_code: 401, - type: 'html' - } - setTemplates(config) + 'when type is forced to html and accept is not defined', () => { + const actionParameters = { + status_code: 401, + type: 'html' + } + setTemplates(config) - block(req, res, null, actionParameters) + block(req, res, rootSpan, null, actionParameters) - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) - }) + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + }) it('should block with custom redirect', () => { const actionParameters = { @@ -253,7 +262,7 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, null, actionParameters) + block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWithExactly(301, { Location: '/you-have-been-blocked' diff --git a/packages/dd-trace/test/appsec/graphql.spec.js b/packages/dd-trace/test/appsec/graphql.spec.js index ea37f8ef504..308103fad87 100644 --- a/packages/dd-trace/test/appsec/graphql.spec.js +++ b/packages/dd-trace/test/appsec/graphql.spec.js @@ -225,10 +225,7 @@ describe('GraphQL', () => { const abortController = context.abortController sinon.stub(waf, 'run').returns({ - actions: { - block_request: blockParameters - }, - metrics: {} + block_request: blockParameters }) sinon.stub(web, 'root').returns(rootSpan) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index a42fefc9435..0033d88f823 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -32,16 +32,10 @@ const telemetryMetrics = require('../../src/telemetry/metrics') const addresses = require('../../src/appsec/addresses') const resultActions = { - actions: { - block_request: { - status_code: '401', - type: 'auto', - grpc_status_code: '10' - } - }, - metrics: { - rulesVersion: '1.0.0', - wafVersion: '1.0.0' + block_request: { + status_code: '401', + type: 'auto', + grpc_status_code: '10' } } @@ -847,7 +841,7 @@ describe('AppSec Index', function () { passportVerify.publish(payload) - expect(storage('legacy').getStore).to.have.been.calledTwice + expect(storage('legacy').getStore).to.have.been.calledOnce expect(web.root).to.have.been.calledOnceWithExactly(req) expect(UserTracking.trackLogin).to.have.been.calledOnceWithExactly( payload.framework, @@ -925,7 +919,7 @@ describe('AppSec Index', function () { passportUser.publish(payload) - expect(storage('legacy').getStore).to.have.been.calledTwice + expect(storage('legacy').getStore).to.have.been.calledOnce expect(web.root).to.have.been.calledOnceWithExactly(req) expect(UserTracking.trackUser).to.have.been.calledOnceWithExactly( payload.user, diff --git a/packages/dd-trace/test/appsec/rasp/command_injection.spec.js b/packages/dd-trace/test/appsec/rasp/command_injection.spec.js index e27d7273fe7..eae70e05256 100644 --- a/packages/dd-trace/test/appsec/rasp/command_injection.spec.js +++ b/packages/dd-trace/test/appsec/rasp/command_injection.spec.js @@ -106,7 +106,9 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.SHELL_COMMAND]: 'cmd' } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly( + waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'shell' } + ) }) it('should analyze command_injection with arguments', () => { @@ -121,7 +123,9 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.SHELL_COMMAND]: ['cmd', 'arg0', 'arg1'] } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly( + waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'shell' } + ) }) it('should call handleResult', () => { @@ -130,13 +134,12 @@ describe('RASP - command_injection.js', () => { const wafResult = { waf: 'waf' } const req = { req: 'req' } const res = { res: 'res' } - const rule = { type: 'command_injection', variant: 'shell' } waf.run.returns(wafResult) legacyStorage.getStore.returns({ req, res }) start.publish(ctx) - sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config, rule) + sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config) }) }) @@ -152,7 +155,9 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.EXEC_COMMAND]: ['ls'] } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly( + waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'exec' } + ) }) it('should analyze command injection with arguments', () => { @@ -167,7 +172,9 @@ describe('RASP - command_injection.js', () => { start.publish(ctx) const ephemeral = { [addresses.EXEC_COMMAND]: ['ls', '-la', '/tmp'] } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly( + waf.run, { ephemeral }, req, { type: 'command_injection', variant: 'exec' } + ) }) it('should call handleResult', () => { @@ -176,13 +183,12 @@ describe('RASP - command_injection.js', () => { const wafResult = { waf: 'waf' } const req = { req: 'req' } const res = { res: 'res' } - const rule = { type: 'command_injection', variant: 'exec' } waf.run.returns(wafResult) legacyStorage.getStore.returns({ req, res }) start.publish(ctx) - sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config, rule) + sinon.assert.calledOnceWithExactly(utils.handleResult, wafResult, req, res, abortController, config) }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/lfi.spec.js b/packages/dd-trace/test/appsec/rasp/lfi.spec.js index 4353ffcc243..b21c6473103 100644 --- a/packages/dd-trace/test/appsec/rasp/lfi.spec.js +++ b/packages/dd-trace/test/appsec/rasp/lfi.spec.js @@ -15,12 +15,7 @@ describe('RASP - lfi.js', () => { } waf = { - run: sinon.stub().returns({ - metrics: { - totalRuntime: 30, - timeout: false - } - }) + run: sinon.stub() } web = { @@ -114,7 +109,7 @@ describe('RASP - lfi.js', () => { fsOperationStart.publish(ctx) const ephemeral = { [FS_OPERATION_PATH]: path } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'lfi' }) }) it('should NOT analyze lfi for child fs operations', () => { diff --git a/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js b/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js index 5cd46780826..2d4dd779c17 100644 --- a/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp/sql_injection.pg.plugin.spec.js @@ -219,7 +219,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 1) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 1) }) it('should call to waf twice for sql injection with two different queries in pg Pool', async () => { @@ -232,7 +232,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 2) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 2) }) it('should call to waf twice for sql injection and same query when input address is updated', async () => { @@ -254,7 +254,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 2) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 2) }) it('should call to waf once for sql injection and same query when input address is updated', async () => { @@ -276,7 +276,7 @@ describe('RASP - sql_injection', () => { await axios.get('/') - assert.equal(run.args.filter(arg => 'ephemeral' in arg[0]).length, 1) + assert.equal(run.args.filter(arg => arg[1]?.type === 'sql_injection').length, 1) }) }) }) diff --git a/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js b/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js index 2a3b641eb26..39b56c22d5e 100644 --- a/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js +++ b/packages/dd-trace/test/appsec/rasp/sql_injection.spec.js @@ -55,7 +55,7 @@ describe('RASP - sql_injection', () => { [addresses.DB_STATEMENT]: 'SELECT 1', [addresses.DB_SYSTEM]: 'postgresql' } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'sql_injection' }) }) it('should not analyze sql injection if rasp is disabled', () => { @@ -126,7 +126,7 @@ describe('RASP - sql_injection', () => { [addresses.DB_STATEMENT]: 'SELECT 1', [addresses.DB_SYSTEM]: 'mysql' } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'sql_injection' }) }) it('should not analyze sql injection if rasp is disabled', () => { diff --git a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js index 4089cc38e76..7c301e8e517 100644 --- a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js +++ b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js @@ -52,7 +52,7 @@ describe('RASP - ssrf.js', () => { httpClientRequestStart.publish(ctx) const ephemeral = { [addresses.HTTP_OUTGOING_URL]: 'http://example.com' } - sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req) + sinon.assert.calledOnceWithExactly(waf.run, { ephemeral }, req, { type: 'ssrf' }) }) it('should not analyze ssrf if rasp is disabled', () => { diff --git a/packages/dd-trace/test/appsec/rasp/utils.spec.js b/packages/dd-trace/test/appsec/rasp/utils.spec.js index e8679c363ac..6a74c07444d 100644 --- a/packages/dd-trace/test/appsec/rasp/utils.spec.js +++ b/packages/dd-trace/test/appsec/rasp/utils.spec.js @@ -35,21 +35,15 @@ describe('RASP - utils.js', () => { const req = {} const rootSpan = {} const stackId = 'test_stack_id' - const results = { - actions: { - generate_stack: { - stack_id: stackId - } - }, - metrics: { - totalRuntime: 50, - timeout: true + const result = { + generate_stack: { + stack_id: stackId } } web.root.returns(rootSpan) - utils.handleResult(results, req, undefined, undefined, config, { type: 'type' }) + utils.handleResult(result, req, undefined, undefined, config) sinon.assert.calledOnceWithExactly(stackTrace.reportStackTrace, rootSpan, stackId, sinon.match.array) }) @@ -62,71 +56,47 @@ describe('RASP - utils.js', () => { } } } - const results = { - actions: { - generate_stack: { - stack_id: 'stackId' - } - }, - metrics: { - totalRuntime: 50, - timeout: true + const result = { + generate_stack: { + stack_id: 'stackId' } } web.root.returns(rootSpan) - utils.handleResult(results, req, undefined, undefined, config, { type: 'type' }) + utils.handleResult(result, req, undefined, undefined, config) sinon.assert.notCalled(stackTrace.reportStackTrace) }) it('should not report stack trace when rootSpan is null', () => { const req = {} - const results = { - actions: { - generate_stack: { - stack_id: 'stackId' - } - }, - metrics: { - totalRuntime: 50, - timeout: true + const result = { + generate_stack: { + stack_id: 'stackId' } } web.root.returns(null) - utils.handleResult(results, req, undefined, undefined, config) + utils.handleResult(result, req, undefined, undefined, config) sinon.assert.notCalled(stackTrace.reportStackTrace) }) it('should not report stack trace when no action is present in waf result', () => { const req = {} - const result = { - metrics: { - totalRuntime: 50, - timeout: true - } - } + const result = {} - utils.handleResult(result, req, undefined, undefined, config, { type: 'type' }) + utils.handleResult(result, req, undefined, undefined, config) sinon.assert.notCalled(stackTrace.reportStackTrace) }) it('should not report stack trace when stack trace reporting is disabled', () => { const req = {} - const results = { - actions: { - generate_stack: { - stack_id: 'stackId' - } - }, - metrics: { - totalRuntime: 50, - timeout: true + const result = { + generate_stack: { + stack_id: 'stackId' } } - const config = { appsec: { stackTrace: { @@ -137,7 +107,7 @@ describe('RASP - utils.js', () => { } } - utils.handleResult(results, req, undefined, undefined, config, { type: 'type' }) + utils.handleResult(result, req, undefined, undefined, config) sinon.assert.notCalled(stackTrace.reportStackTrace) }) }) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index af3edc8f6b5..b2d9205143f 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -143,6 +143,8 @@ describe('reporter', () => { }) it('should do nothing when passed incomplete objects', () => { + web.root.returns(null) + Reporter.reportMetrics({}) expect(span.setTag).not.to.have.been.called diff --git a/packages/dd-trace/test/appsec/waf/index.spec.js b/packages/dd-trace/test/appsec/waf/index.spec.js index bafe4464dd8..33c0bfbb3a3 100644 --- a/packages/dd-trace/test/appsec/waf/index.spec.js +++ b/packages/dd-trace/test/appsec/waf/index.spec.js @@ -49,6 +49,7 @@ describe('WAF Manager', () => { waf.destroy() sinon.stub(Reporter.metricsQueue, 'set') + sinon.stub(Reporter, 'reportMetrics') sinon.stub(Reporter, 'reportAttack') sinon.stub(Reporter, 'reportWafUpdate') sinon.stub(Reporter, 'reportDerivatives') @@ -106,7 +107,19 @@ describe('WAF Manager', () => { }) describe('run', () => { - it('should call wafManager.run', () => { + it('should call wafManager.run with raspRuleType', () => { + const run = sinon.stub() + WAFManager.prototype.getWAFContext = sinon.stub().returns({ run }) + waf.init(rules, config.appsec) + + const payload = { persistent: { 'server.io.net.url': 'http://example.com' } } + const req = {} + waf.run(payload, req, 'ssrf') + + expect(run).to.be.calledOnceWithExactly(payload, 'ssrf') + }) + + it('should call wafManager.run without raspRuleType', () => { const run = sinon.stub() WAFManager.prototype.getWAFContext = sinon.stub().returns({ run }) waf.init(rules, config.appsec) @@ -115,7 +128,7 @@ describe('WAF Manager', () => { const req = {} waf.run(payload, req) - expect(run).to.be.calledOnceWithExactly(payload) + expect(run).to.be.calledOnceWithExactly(payload, undefined) }) }) @@ -277,6 +290,66 @@ describe('WAF Manager', () => { expect(Reporter.reportAttack).to.be.calledOnceWithExactly('["ATTACK DATA"]') }) + it('should report if rule is triggered', () => { + const result = { + totalRuntime: 1, + durationExt: 1, + events: ['ruleTriggered'] + } + + ddwafContext.run.returns(result) + const params = { + persistent: { + 'server.request.headers.no_cookies': { header: 'value' } + } + } + + wafContextWrapper.run(params) + + expect(Reporter.reportMetrics).to.be.calledOnce + + const reportMetricsArg = Reporter.reportMetrics.firstCall.args[0] + expect(reportMetricsArg.ruleTriggered).to.be.true + }) + + it('should report raspRuleType', () => { + const result = { + totalRuntime: 1, + durationExt: 1 + } + + ddwafContext.run.returns(result) + const params = { + persistent: { + 'server.request.headers.no_cookies': { header: 'value' } + } + } + + wafContextWrapper.run(params, 'rule_type') + + expect(Reporter.reportMetrics).to.be.calledOnce + expect(Reporter.reportMetrics.firstCall.args[1]).to.be.equal('rule_type') + }) + + it('should not report raspRuleType when it is not provided', () => { + const result = { + totalRuntime: 1, + durationExt: 1 + } + + ddwafContext.run.returns(result) + const params = { + persistent: { + 'server.request.headers.no_cookies': { header: 'value' } + } + } + + wafContextWrapper.run(params) + + expect(Reporter.reportMetrics).to.be.calledOnce + expect(Reporter.reportMetrics.firstCall.args[1]).to.be.equal(undefined) + }) + it('should not report attack when ddwafContext does not return events', () => { ddwafContext.run.returns({ totalRuntime: 1, durationExt: 1 }) const params = { @@ -303,7 +376,7 @@ describe('WAF Manager', () => { expect(Reporter.reportAttack).not.to.be.called }) - it('should return the results with metrics and durationExt', () => { + it('should return the actions', () => { const actions = ['block'] ddwafContext.run.returns({ totalRuntime: 1, durationExt: 1, events: [], actions }) @@ -315,16 +388,7 @@ describe('WAF Manager', () => { const result = wafContextWrapper.run(params) - expect(result.actions).to.deep.equal(['block']) - - expect(result.metrics).to.include({ - rulesVersion: '1.0.0', - duration: 0.001, - blockTriggered: false, - ruleTriggered: false - }) - - expect(result.metrics.durationExt).to.be.greaterThan(0) + expect(result).to.be.equals(actions) }) it('should report schemas when ddwafContext returns schemas in the derivatives', () => { From 2df48d286b31ed4dc7e3415287c4d94236ffdb5a Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 10:56:40 +0100 Subject: [PATCH 36/56] user blocking tests --- packages/dd-trace/src/appsec/blocking.js | 13 +++-- .../dd-trace/src/appsec/sdk/user_blocking.js | 26 ++-------- .../dd-trace/test/appsec/blocking.spec.js | 49 ++++++++++++------- .../test/appsec/sdk/user_blocking.spec.js | 21 ++++---- 4 files changed, 52 insertions(+), 57 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 9583599c01e..be6cfeb0381 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -100,6 +100,7 @@ function getBlockingData (req, specificType, actionParameters) { } function block (req, res, rootSpan, abortController, actionParameters = defaultBlockingActionParameters) { + let blocked try { if (res.headersSent) { log.warn('[ASM] Cannot send blocking response when headers have already been sent') @@ -118,17 +119,23 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB // this is needed to call the original end method, since express-session replaces it res.constructor.prototype.end.call(res, body) - responseBlockedSet.add(res) - abortController?.abort() - rootSpan.addTags({ 'appsec.blocked': 'true' }) + + blocked = true } catch (err) { rootSpan?.addTags({ '_dd.appsec.block.failed': 1 }) log.error('[ASM] Blocking error', err) + + blocked = false + } finally { + responseBlockedSet.add(res) + abortController?.abort() + + return blocked } } diff --git a/packages/dd-trace/src/appsec/sdk/user_blocking.js b/packages/dd-trace/src/appsec/sdk/user_blocking.js index fd0b6dc6851..a102d814cb2 100644 --- a/packages/dd-trace/src/appsec/sdk/user_blocking.js +++ b/packages/dd-trace/src/appsec/sdk/user_blocking.js @@ -7,18 +7,10 @@ const { block, getBlockingAction } = require('../blocking') const { storage } = require('../../../../datadog-core') const { setUserTags } = require('./set_user') const log = require('../../log') -const { reportMetrics } = require('../reporter') function isUserBlocked (user) { - const wafResults = waf.run({ persistent: { [USER_ID]: user.id } }) - - if (!wafResults) return false - - const blockTriggered = !!getBlockingAction(wafResults.actions) - - reportMetrics(wafResults.metrics, null) - - return blockTriggered + const actions = waf.run({ persistent: { [USER_ID]: user.id } }) + return !!getBlockingAction(actions) } function checkUserAndSetUser (tracer, user) { @@ -60,19 +52,7 @@ function blockRequest (tracer, req, res) { return false } - try { - block(req, res) - - rootSpan.setTag('appsec.blocked', 'true') - - return true - } catch (err) { - rootSpan.setTag('_dd.appsec.block.failed', 1) - - log.error('[ASM] Blocking error', err) - - return false - } + return block(req, res, rootSpan) } module.exports = { diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 9575e1cf470..c209690e61e 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -58,12 +58,9 @@ describe('blocking', () => { it('should log warn and not send blocking response when headers have already been sent', () => { res.headersSent = true - try { - block(req, res) - } catch (error) { - expect(error.message).to.equal('Headers have already been sent') - } + const blocked = block(req, res) + expect(blocked).to.be.false expect(log.warn).to.have.been .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') expect(rootSpan.addTags).to.not.have.been.called @@ -73,8 +70,9 @@ describe('blocking', () => { it('should send blocking response with html type if present in the headers', () => { req.headers.accept = 'text/html' - block(req, res, rootSpan) + const blocked = block(req, res, rootSpan) + expect(blocked).to.be.true expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'text/html; charset=utf-8', @@ -85,8 +83,9 @@ describe('blocking', () => { it('should send blocking response with json type if present in the headers in priority', () => { req.headers.accept = 'text/html, application/xhtml+xml, application/xml;q=0.9, */*;q=0.8, application/json' - block(req, res, rootSpan) + const blocked = block(req, res, rootSpan) + expect(blocked).to.be.true expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', @@ -96,8 +95,9 @@ describe('blocking', () => { }) it('should send blocking response with json type if neither html or json is present in the headers', () => { - block(req, res, rootSpan) + const blocked = block(req, res, rootSpan) + expect(blocked).to.be.true expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', @@ -108,8 +108,9 @@ describe('blocking', () => { it('should send blocking response and call abortController if passed in arguments', () => { const abortController = new AbortController() - block(req, res, rootSpan, abortController) + const blocked = block(req, res, rootSpan, abortController) + expect(blocked).to.be.true expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', @@ -122,8 +123,9 @@ describe('blocking', () => { it('should remove all headers before sending blocking response', () => { res.getHeaderNames.returns(['header1', 'header2']) - block(req, res, rootSpan) + const blocked = block(req, res, rootSpan) + expect(blocked).to.be.true expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.removeHeader).to.have.been.calledTwice expect(res.removeHeader.firstCall).to.have.been.calledWithExactly('header1') @@ -148,16 +150,18 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan) + const blocked = block(req, res, rootSpan) + expect(blocked).to.be.true expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with default json template', () => { setTemplates(config) - block(req, res, rootSpan) + const blocked = block(req, res, rootSpan) + expect(blocked).to.be.true expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) }) @@ -178,8 +182,9 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) @@ -193,8 +198,9 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) @@ -208,8 +214,9 @@ describe('blocking', () => { req.headers.accept = 'text/html' setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) @@ -221,8 +228,9 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) @@ -235,8 +243,9 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) @@ -249,8 +258,9 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) @@ -262,8 +272,9 @@ describe('blocking', () => { } setTemplates(config) - block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWithExactly(301, { Location: '/you-have-been-blocked' }) 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 65ae39d094f..1ff74461438 100644 --- a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js +++ b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js @@ -12,14 +12,11 @@ const { USER_ID } = require('../../../src/appsec/addresses') const blocking = require('../../../src/appsec/blocking') const resultActions = { - actions: { - block_request: { - status_code: '401', - type: 'auto', - grpc_status_code: '10' - } - }, - metrics: {} + block_request: { + status_code: '401', + type: 'auto', + grpc_status_code: '10' + } } describe('user_blocking', () => { @@ -45,7 +42,7 @@ describe('user_blocking', () => { } getRootSpan = sinon.stub().returns(rootSpan) - block = sinon.stub() + block = sinon.stub().returns(true) legacyStorage = { getStore: sinon.stub().returns({ req, res }) @@ -118,7 +115,7 @@ describe('user_blocking', () => { const ret = userBlocking.blockRequest(tracer) expect(ret).to.be.true expect(legacyStorage.getStore).to.have.been.calledOnce - expect(block).to.be.calledOnceWithExactly(req, res) + expect(block).to.be.calledOnceWithExactly(req, res, rootSpan) }) it('should log warning when req or res is not available', () => { @@ -147,7 +144,7 @@ describe('user_blocking', () => { const ret = userBlocking.blockRequest(tracer, req, res) expect(ret).to.be.true expect(log.warn).to.not.have.been.called - expect(block).to.have.been.calledOnceWithExactly(req, res) + expect(block).to.have.been.calledOnceWithExactly(req, res, rootSpan) }) }) }) @@ -294,7 +291,7 @@ describe('user_blocking', () => { agent.use(traces => { expect(traces[0][0].meta).to.not.have.property('appsec.blocked', 'true') expect(traces[0][0].meta).to.have.property('http.status_code', '200') - expect(traces[0][0].metrics).to.not.have.property('_dd.appsec.block.failed', 1) + expect(traces[0][0].metrics).to.have.property('_dd.appsec.block.failed', 1) }).then(done).catch(done) axios.get(`http://localhost:${port}/`) }) From 2560c74fa763711284bcc0dde83990bf5a40daa4 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 11:10:32 +0100 Subject: [PATCH 37/56] add reporter tests --- packages/dd-trace/src/appsec/rasp/index.js | 1 - .../src/appsec/waf/waf_context_wrapper.js | 2 +- .../dd-trace/test/appsec/blocking.spec.js | 8 +-- .../dd-trace/test/appsec/reporter.spec.js | 61 ++++++------------- 4 files changed, 25 insertions(+), 47 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp/index.js b/packages/dd-trace/src/appsec/rasp/index.js index c00c6493487..4a65518495d 100644 --- a/packages/dd-trace/src/appsec/rasp/index.js +++ b/packages/dd-trace/src/appsec/rasp/index.js @@ -7,7 +7,6 @@ const ssrf = require('./ssrf') const sqli = require('./sql_injection') const lfi = require('./lfi') const cmdi = require('./command_injection') -const log = require('../../log') const { DatadogRaspAbortError } = require('./utils') 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 07a42413a43..bbaf77fe4a9 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -1,10 +1,10 @@ 'use strict' const log = require('../../log') +const Reporter = require('../reporter') const addresses = require('../addresses') const { getBlockingAction } = require('../blocking') const { wafRunFinished } = require('../channels') -const Reporter = require('../reporter') // TODO: remove once ephemeral addresses are implemented const preventDuplicateAddresses = new Set([ diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index c209690e61e..88d0cb73904 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -200,7 +200,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) @@ -216,7 +216,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) @@ -245,7 +245,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) @@ -260,7 +260,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true + expect(blocked).to.be.true expect(res.writeHead).to.have.been.calledOnceWith(401) expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index b2d9205143f..ab3886f4723 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -153,10 +153,7 @@ describe('reporter', () => { it('should do nothing when rootSpan is not available', () => { web.root.returns(null) - const metrics = { - durationExt: 42, - totalRuntime: 1337000 - } + const metrics = { durationExt: 42, totalRuntime: 1337000 } Reporter.reportMetrics(metrics) @@ -174,8 +171,18 @@ describe('reporter', () => { expect(telemetry.updateRaspRequestsMetricTags).to.not.have.been.called }) + it('should call updateWafRequestsMetricTags', () => { + const metrics = { rulesVersion: '1.2.3' } + const store = storage('legacy').getStore() + + Reporter.reportMetrics(metrics) + + expect(telemetry.updateWafRequestsMetricTags).to.have.been.calledOnceWithExactly(metrics, store.req) + expect(telemetry.updateRaspRequestsMetricTags).to.not.have.been.called + }) + it('should set ext duration metrics if set', () => { - const metrics = { durationExt: 42, totalRuntime: 0 } + const metrics = { durationExt: 42 } Reporter.reportMetrics(metrics) @@ -185,24 +192,17 @@ describe('reporter', () => { }) it('should set rulesVersion if set', () => { - const metrics = { - rulesVersion: '1.2.3', - durationExt: 0, - totalRuntime: 0 - } + const metrics = { rulesVersion: '1.2.3' } Reporter.reportMetrics(metrics) expect(web.root).to.have.been.calledOnceWithExactly(req) expect(span.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.event_rules.version', '1.2.3') + expect(telemetry.updateRaspRequestsMetricTags).to.not.have.been.called }) it('should set blockTriggered when provided', () => { - const metrics = { - durationExt: 0, - totalRuntime: 0, - blockTriggered: true - } + const metrics = { blockTriggered: true } Reporter.reportMetrics(metrics, null) @@ -210,11 +210,7 @@ describe('reporter', () => { }) it('should set wafTimeout when result has timeout', () => { - const metrics = { - durationExt: 0, - totalRuntime: 0, - timeout: true - } + const metrics = { timeout: true } Reporter.reportMetrics(metrics) @@ -222,11 +218,7 @@ describe('reporter', () => { }) it('should set max truncation string length metric if set', () => { - const metrics = { - maxTruncatedString: 300, - durationExt: 0, - totalRuntime: 0 - } + const metrics = { maxTruncatedString: 300 } Reporter.reportMetrics(metrics) @@ -234,11 +226,7 @@ describe('reporter', () => { }) it('should set max truncation container size metric if set', () => { - const metrics = { - maxTruncatedContainerSize: 200, - durationExt: 0, - totalRuntime: 0 - } + const metrics = { maxTruncatedContainerSize: 200 } Reporter.reportMetrics(metrics) @@ -246,11 +234,7 @@ describe('reporter', () => { }) it('should set max truncation container depth metric if set', () => { - const metrics = { - maxTruncatedContainerDepth: 100, - durationExt: 0, - totalRuntime: 0 - } + const metrics = { maxTruncatedContainerDepth: 100 } Reporter.reportMetrics(metrics) @@ -258,12 +242,7 @@ describe('reporter', () => { }) it('should call updateRaspRequestsMetricTags when raspRule is provided', () => { - const metrics = { - rulesVersion: '1.2.3', - durationExt: 0, - totalRuntime: 0 - } - + const metrics = { rulesVersion: '1.2.3' } const store = storage('legacy').getStore() const raspRule = { type: 'rule_type', variant: 'rule_variant' } From 1e236f2417b87f6957c149bfc50176018085e905 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 11:34:31 +0100 Subject: [PATCH 38/56] linter --- packages/dd-trace/src/appsec/blocking.js | 10 +-- .../dd-trace/test/appsec/blocking.spec.js | 84 +++++++++---------- 2 files changed, 47 insertions(+), 47 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index be6cfeb0381..8b5a37310f5 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -123,6 +123,9 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB 'appsec.blocked': 'true' }) + responseBlockedSet.add(res) + abortController?.abort() + blocked = true } catch (err) { rootSpan?.addTags({ @@ -131,12 +134,9 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB log.error('[ASM] Blocking error', err) blocked = false - } finally { - responseBlockedSet.add(res) - abortController?.abort() - - return blocked } + + return blocked } function getBlockingAction (actions) { diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 88d0cb73904..50587fdb1be 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -191,35 +191,35 @@ describe('blocking', () => { it('should block with default json template and custom status ' + 'when type is forced to json and accept is html', () => { - const actionParameters = { - status_code: 401, - type: 'json' - } - req.headers.accept = 'text/html' - setTemplates(config) + const actionParameters = { + status_code: 401, + type: 'json' + } + req.headers.accept = 'text/html' + setTemplates(config) - const blocked = block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) - }) + expect(blocked).to.be.true + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + }) it('should block with default html template and custom status ' + 'when type is forced to html and accept is html', () => { - const actionParameters = { - status_code: 401, - type: 'html' - } - req.headers.accept = 'text/html' - setTemplates(config) + const actionParameters = { + status_code: 401, + type: 'html' + } + req.headers.accept = 'text/html' + setTemplates(config) - const blocked = block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) - }) + expect(blocked).to.be.true + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + }) it('should block with default json template and custom status', () => { const actionParameters = { @@ -237,33 +237,33 @@ describe('blocking', () => { it('should block with default json template and custom status ' + 'when type is forced to json and accept is not defined', () => { - const actionParameters = { - status_code: 401, - type: 'json' - } - setTemplates(config) + const actionParameters = { + status_code: 401, + type: 'json' + } + setTemplates(config) - const blocked = block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) - }) + expect(blocked).to.be.true + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + }) it('should block with default html template and custom status ' + 'when type is forced to html and accept is not defined', () => { - const actionParameters = { - status_code: 401, - type: 'html' - } - setTemplates(config) + const actionParameters = { + status_code: 401, + type: 'html' + } + setTemplates(config) - const blocked = block(req, res, rootSpan, null, actionParameters) + const blocked = block(req, res, rootSpan, null, actionParameters) - expect(blocked).to.be.true - expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) - }) + expect(blocked).to.be.true + expect(res.writeHead).to.have.been.calledOnceWith(401) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + }) it('should block with custom redirect', () => { const actionParameters = { From 32b629b4388208b94e1199d891f4b0003689f6c1 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 14:43:27 +0100 Subject: [PATCH 39/56] add test for block failure --- packages/dd-trace/test/appsec/blocking.spec.js | 4 ++-- packages/dd-trace/test/appsec/reporter.spec.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 50587fdb1be..b6b207ae962 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -58,12 +58,12 @@ describe('blocking', () => { it('should log warn and not send blocking response when headers have already been sent', () => { res.headersSent = true - const blocked = block(req, res) + const blocked = block(req, res, rootSpan) expect(blocked).to.be.false expect(log.warn).to.have.been .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') - expect(rootSpan.addTags).to.not.have.been.called + expect(rootSpan.addTags).to.have.been.called expect(res.setHeader).to.not.have.been.called expect(res.constructor.prototype.end).to.not.have.been.called }) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index ab3886f4723..55d70317660 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -183,7 +183,6 @@ describe('reporter', () => { it('should set ext duration metrics if set', () => { const metrics = { durationExt: 42 } - Reporter.reportMetrics(metrics) expect(web.root).to.have.been.calledOnceWithExactly(req) @@ -244,6 +243,7 @@ describe('reporter', () => { it('should call updateRaspRequestsMetricTags when raspRule is provided', () => { const metrics = { rulesVersion: '1.2.3' } const store = storage('legacy').getStore() + const raspRule = { type: 'rule_type', variant: 'rule_variant' } Reporter.reportMetrics(metrics, raspRule) From f05d5f4fcafd72d486873d16787bcdac6b3e51db Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 15:43:20 +0100 Subject: [PATCH 40/56] use settag instead of addtags --- packages/dd-trace/src/appsec/blocking.js | 15 ++++----------- packages/dd-trace/src/appsec/graphql.js | 4 +--- packages/dd-trace/src/appsec/reporter.js | 8 ++++---- packages/dd-trace/test/appsec/blocking.spec.js | 14 +++++++------- packages/dd-trace/test/appsec/index.spec.js | 1 + 5 files changed, 17 insertions(+), 25 deletions(-) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 8b5a37310f5..5b3dd290c00 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -100,7 +100,6 @@ function getBlockingData (req, specificType, actionParameters) { } function block (req, res, rootSpan, abortController, actionParameters = defaultBlockingActionParameters) { - let blocked try { if (res.headersSent) { log.warn('[ASM] Cannot send blocking response when headers have already been sent') @@ -119,24 +118,18 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB // this is needed to call the original end method, since express-session replaces it res.constructor.prototype.end.call(res, body) - rootSpan.addTags({ - 'appsec.blocked': 'true' - }) + rootSpan.setTag('appsec.blocked', 'true') responseBlockedSet.add(res) abortController?.abort() - blocked = true + return true } catch (err) { - rootSpan?.addTags({ - '_dd.appsec.block.failed': 1 - }) + rootSpan?.setTag('_dd.appsec.block.failed', 1) log.error('[ASM] Blocking error', err) - blocked = false + return false } - - return blocked } function getBlockingAction (actions) { diff --git a/packages/dd-trace/src/appsec/graphql.js b/packages/dd-trace/src/appsec/graphql.js index d2675e85673..2ff265e6282 100644 --- a/packages/dd-trace/src/appsec/graphql.js +++ b/packages/dd-trace/src/appsec/graphql.js @@ -105,9 +105,7 @@ function beforeWriteApolloGraphqlResponse ({ abortController, abortData }) { abortController?.abort() } catch (err) { - rootSpan.addTags({ - '_dd.appsec.block.failed': 1 - }) + rootSpan.setTag('_dd.appsec.block.failed', 1) log.error('[ASM] Blocking error', err) } diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 308f978bf78..8fa2795bb6b 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -232,10 +232,6 @@ function finishRequest (req, res) { rootSpan.setTag('_dd.appsec.rasp.duration_ext', metrics.raspDurationExt) } - if (metrics?.raspEvalCount) { - rootSpan.setTag('_dd.appsec.rasp.rule.eval', metrics.raspEvalCount) - } - if (metrics?.raspTimeouts) { rootSpan.setTag('_dd.appsec.rasp.timeout', metrics.raspTimeouts) } @@ -244,6 +240,10 @@ function finishRequest (req, res) { rootSpan.setTag('_dd.appsec.rasp.error', metrics.raspErrorCode) } + if (metrics?.raspEvalCount) { + rootSpan.setTag('_dd.appsec.rasp.rule.eval', metrics.raspEvalCount) + } + incrementWafRequestsMetric(req) // collect some headers even when no attack is detected diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index b6b207ae962..3b92f6e523e 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -47,7 +47,7 @@ describe('blocking', () => { } rootSpan = { - addTags: sinon.stub() + setTag: sinon.stub() } }) @@ -63,7 +63,7 @@ describe('blocking', () => { expect(blocked).to.be.false expect(log.warn).to.have.been .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') - expect(rootSpan.addTags).to.have.been.called + expect(rootSpan.setTag).to.have.been.called expect(res.setHeader).to.not.have.been.called expect(res.constructor.prototype.end).to.not.have.been.called }) @@ -73,7 +73,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan) expect(blocked).to.be.true - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'text/html; charset=utf-8', 'Content-Length': 12 @@ -86,7 +86,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan) expect(blocked).to.be.true - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true' ) expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -98,7 +98,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan) expect(blocked).to.be.true - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -111,7 +111,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan, abortController) expect(blocked).to.be.true - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -126,7 +126,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan) expect(blocked).to.be.true - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) expect(res.removeHeader).to.have.been.calledTwice expect(res.removeHeader.firstCall).to.have.been.calledWithExactly('header1') expect(res.removeHeader.secondCall).to.have.been.calledWithExactly('header2') diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 0033d88f823..7c79e4751a6 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -663,6 +663,7 @@ describe('AppSec Index', function () { rootSpan = { addTags: sinon.stub(), + setTag: sinon.stub(), _tags: {}, context: () => ({ _tags: rootSpan._tags }) } From f290905e4a5c502ccb6dbe011a6e60aea8999d03 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 15:45:56 +0100 Subject: [PATCH 41/56] metrics order --- packages/dd-trace/src/appsec/reporter.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/src/appsec/reporter.js b/packages/dd-trace/src/appsec/reporter.js index 8fa2795bb6b..9a0b7467c37 100644 --- a/packages/dd-trace/src/appsec/reporter.js +++ b/packages/dd-trace/src/appsec/reporter.js @@ -232,14 +232,14 @@ function finishRequest (req, res) { rootSpan.setTag('_dd.appsec.rasp.duration_ext', metrics.raspDurationExt) } - if (metrics?.raspTimeouts) { - rootSpan.setTag('_dd.appsec.rasp.timeout', metrics.raspTimeouts) - } - if (metrics?.raspErrorCode) { rootSpan.setTag('_dd.appsec.rasp.error', metrics.raspErrorCode) } + if (metrics?.raspTimeouts) { + rootSpan.setTag('_dd.appsec.rasp.timeout', metrics.raspTimeouts) + } + if (metrics?.raspEvalCount) { rootSpan.setTag('_dd.appsec.rasp.rule.eval', metrics.raspEvalCount) } From 1e2f50ad0cbd39461ef26bdabe71c4e251750fc5 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 5 Mar 2025 15:51:52 +0100 Subject: [PATCH 42/56] fix linter and test --- packages/dd-trace/test/appsec/blocking.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 3b92f6e523e..6f311054810 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -86,7 +86,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan) expect(blocked).to.be.true - expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true' ) + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { 'Content-Type': 'application/json', 'Content-Length': 8 @@ -126,7 +126,7 @@ describe('blocking', () => { const blocked = block(req, res, rootSpan) expect(blocked).to.be.true - expect(rootSpan.setTag).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') expect(res.removeHeader).to.have.been.calledTwice expect(res.removeHeader.firstCall).to.have.been.calledWithExactly('header1') expect(res.removeHeader.secondCall).to.have.been.calledWithExactly('header2') From e779cb2eaa807266f9f1250aec51ad66991f4f4a Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 09:19:47 +0100 Subject: [PATCH 43/56] remove return null --- packages/dd-trace/src/appsec/waf/waf_context_wrapper.js | 2 -- 1 file changed, 2 deletions(-) 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 bbaf77fe4a9..9c1941a17e9 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -98,8 +98,6 @@ class WAFContextWrapper { if (!result) { // Binding or other waf unexpected errors metrics.errorCode = -127 - - return null } else { if (typeof result.errorCode === 'number' && result.errorCode < 0) { metrics.errorCode = result.errorCode From 082cbf4e751aa1cfdfbf9e754c5d66a48d0aef79 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 09:36:27 +0100 Subject: [PATCH 44/56] add waf context run stubs --- .../appsec/waf/waf_context_wrapper.spec.js | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index 436f6c093d4..9bbe2c03eaa 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -6,15 +6,23 @@ const addresses = require('../../../src/appsec/addresses') const { wafRunFinished } = require('../../../src/appsec/channels') describe('WAFContextWrapper', () => { + let ddwafContext + + beforeEach(() => { + ddwafContext = { + run: sinon.stub().returns({ + events: {}, + derivatives: {}, + }) + } + }) + const knownAddresses = new Set([ addresses.HTTP_INCOMING_QUERY, addresses.HTTP_INCOMING_GRAPHQL_RESOLVER ]) it('Should send HTTP_INCOMING_QUERY only once', () => { - const ddwafContext = { - run: sinon.stub() - } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { @@ -30,9 +38,6 @@ describe('WAFContextWrapper', () => { }) it('Should send ephemeral addresses every time', () => { - const ddwafContext = { - run: sinon.stub() - } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { @@ -59,9 +64,6 @@ describe('WAFContextWrapper', () => { }) it('Should ignore run without known addresses', () => { - const ddwafContext = { - run: sinon.stub() - } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { @@ -79,9 +81,6 @@ describe('WAFContextWrapper', () => { }) it('should publish the payload in the dc channel', () => { - const ddwafContext = { - run: sinon.stub().returns([]) - } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { persistent: { @@ -111,7 +110,10 @@ describe('WAFContextWrapper', () => { } ddwafContext = { - run: sinon.stub() + run: sinon.stub().returns({ + events: {}, + derivatives: {}, + }) } const ProxiedWafContextWrapper = proxyquire('../../../src/appsec/waf/waf_context_wrapper', { From 29dc7aeef5c4ed4e56ecff011eb1eb742e1adf6b Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 09:48:20 +0100 Subject: [PATCH 45/56] waf error code readability --- packages/dd-trace/src/appsec/telemetry/rasp.js | 12 ++++++++---- packages/dd-trace/src/appsec/telemetry/waf.js | 12 ++++++++---- .../test/appsec/waf/waf_context_wrapper.spec.js | 4 ++-- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/src/appsec/telemetry/rasp.js b/packages/dd-trace/src/appsec/telemetry/rasp.js index e6fe008a26e..8fd6e80dbd7 100644 --- a/packages/dd-trace/src/appsec/telemetry/rasp.js +++ b/packages/dd-trace/src/appsec/telemetry/rasp.js @@ -15,10 +15,14 @@ function addRaspRequestMetrics (store, { duration, durationExt, wafTimeout, erro } if (errorCode) { - store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode = Math.max( - errorCode, - store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode ?? errorCode - ) + if (store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode) { + store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode = Math.max( + errorCode, + store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode + ) + } else { + store[DD_TELEMETRY_REQUEST_METRICS].raspErrorCode = errorCode + } } } diff --git a/packages/dd-trace/src/appsec/telemetry/waf.js b/packages/dd-trace/src/appsec/telemetry/waf.js index 4cbf0fa0946..31b4ecafec6 100644 --- a/packages/dd-trace/src/appsec/telemetry/waf.js +++ b/packages/dd-trace/src/appsec/telemetry/waf.js @@ -16,10 +16,14 @@ function addWafRequestMetrics (store, { duration, durationExt, wafTimeout, error } if (errorCode) { - store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode = Math.max( - errorCode, - store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode ?? errorCode - ) + if (store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode) { + store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode = Math.max( + errorCode, + store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode + ) + } else { + store[DD_TELEMETRY_REQUEST_METRICS].wafErrorCode = errorCode + } } } diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index 9bbe2c03eaa..8829670139e 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -12,7 +12,7 @@ describe('WAFContextWrapper', () => { ddwafContext = { run: sinon.stub().returns({ events: {}, - derivatives: {}, + derivatives: {} }) } }) @@ -112,7 +112,7 @@ describe('WAFContextWrapper', () => { ddwafContext = { run: sinon.stub().returns({ events: {}, - derivatives: {}, + derivatives: {} }) } From 64654410ff3a33a206cf033d8cf44cd5e6947215 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 09:55:38 +0100 Subject: [PATCH 46/56] fix timers --- packages/dd-trace/src/appsec/waf/waf_context_wrapper.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 9c1941a17e9..3ebb016f18f 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -77,7 +77,6 @@ class WAFContextWrapper { if (!payloadHasData) return - const start = process.hrtime.bigint() const metrics = { rulesVersion: this.rulesVersion, @@ -88,11 +87,12 @@ class WAFContextWrapper { ruleTriggered: false } - const result = this.runWaf(payload, this.wafTimeout) + const start = process.hrtime.bigint() - this.addressesToSkip = newAddressesToSkip + const result = this.runWaf(payload, this.wafTimeout) const end = process.hrtime.bigint() + metrics.durationExt = parseInt(end - start) / 1e3 if (!result) { @@ -116,6 +116,8 @@ class WAFContextWrapper { } } + this.addressesToSkip = newAddressesToSkip + const ruleTriggered = !!result.events?.length const blockTriggered = !!getBlockingAction(result.actions) From 3a4f212948fc9eda03529c82bb352c4156412303 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 10:44:50 +0100 Subject: [PATCH 47/56] report metrics on waf failure --- .../src/appsec/waf/waf_context_wrapper.js | 36 ++++++++++--------- packages/dd-trace/test/appsec/index.spec.js | 1 - 2 files changed, 19 insertions(+), 18 deletions(-) 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 3ebb016f18f..b747390bbdb 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -77,14 +77,18 @@ class WAFContextWrapper { if (!payloadHasData) return - const metrics = { rulesVersion: this.rulesVersion, wafVersion: this.wafVersion, wafTimeout: false, duration: 0, + durationExt: 0, blockTriggered: false, - ruleTriggered: false + ruleTriggered: false, + errorCode: null, + maxTruncatedString: null, + maxTruncatedString: null, + maxTruncatedString: null } const start = process.hrtime.bigint() @@ -95,25 +99,23 @@ class WAFContextWrapper { metrics.durationExt = parseInt(end - start) / 1e3 - if (!result) { - // Binding or other waf unexpected errors - metrics.errorCode = -127 - } else { - if (typeof result.errorCode === 'number' && result.errorCode < 0) { - metrics.errorCode = result.errorCode - } + if (!result || (typeof result.errorCode === 'number' && result.errorCode < 0)) { + metrics.errorCode = result ? result.errorCode : -127 - if (result.metrics?.maxTruncatedString) { - metrics.maxTruncatedString = result.metrics.maxTruncatedString + if (wafRunFinished.hasSubscribers) { + wafRunFinished.publish({ payload }) } + Reporter.reportMetrics(metrics, raspRule) - if (result.metrics?.maxTruncatedContainerSize) { - metrics.maxTruncatedContainerSize = result.metrics.maxTruncatedContainerSize - } + return + } - if (result.metrics?.maxTruncatedContainerDepth) { - metrics.maxTruncatedContainerDepth = result.metrics.maxTruncatedContainerDepth - } + if (result.metrics) { + const { maxTruncatedString, maxTruncatedContainerSize, maxTruncatedContainerDepth } = result.metrics + + if (maxTruncatedString) metrics.maxTruncatedString = maxTruncatedString + if (maxTruncatedContainerSize) metrics.maxTruncatedContainerSize = maxTruncatedContainerSize + if (maxTruncatedContainerDepth) metrics.maxTruncatedContainerDepth = maxTruncatedContainerDepth } this.addressesToSkip = newAddressesToSkip diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 7c79e4751a6..19f115a7340 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -662,7 +662,6 @@ describe('AppSec Index', function () { sinon.stub(waf, 'run') rootSpan = { - addTags: sinon.stub(), setTag: sinon.stub(), _tags: {}, context: () => ({ _tags: rootSpan._tags }) From e4a602351953f5efa690e746fd9800c6dfbd47c6 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 10:47:45 +0100 Subject: [PATCH 48/56] fix metrics --- packages/dd-trace/src/appsec/waf/waf_context_wrapper.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 b747390bbdb..4fd07ac41d2 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -87,8 +87,8 @@ class WAFContextWrapper { ruleTriggered: false, errorCode: null, maxTruncatedString: null, - maxTruncatedString: null, - maxTruncatedString: null + maxTruncatedContainerSize: null, + maxTruncatedContainerDepth: null } const start = process.hrtime.bigint() From d78502e6a086afe9d2ff645d3b59063dbe0cc530 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 12:00:01 +0100 Subject: [PATCH 49/56] remove run waf function --- .../src/appsec/waf/waf_context_wrapper.js | 88 ++++++++----------- .../appsec/waf/waf_context_wrapper.spec.js | 74 +++++++++++++--- 2 files changed, 99 insertions(+), 63 deletions(-) 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 4fd07ac41d2..07f127bf00d 100644 --- a/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js +++ b/packages/dd-trace/src/appsec/waf/waf_context_wrapper.js @@ -91,74 +91,64 @@ class WAFContextWrapper { maxTruncatedContainerDepth: null } - const start = process.hrtime.bigint() + try { + const start = process.hrtime.bigint() - const result = this.runWaf(payload, this.wafTimeout) + const result = this.ddwafContext.run(payload, this.wafTimeout) - const end = process.hrtime.bigint() + const end = process.hrtime.bigint() - metrics.durationExt = parseInt(end - start) / 1e3 + metrics.durationExt = parseInt(end - start) / 1e3 - if (!result || (typeof result.errorCode === 'number' && result.errorCode < 0)) { - metrics.errorCode = result ? result.errorCode : -127 + if (typeof result.errorCode === 'number' && result.errorCode < 0) { + const error = new Error('WAF code error') + error.errorCode = result.errorCode - if (wafRunFinished.hasSubscribers) { - wafRunFinished.publish({ payload }) + throw error } - Reporter.reportMetrics(metrics, raspRule) - - return - } - if (result.metrics) { - const { maxTruncatedString, maxTruncatedContainerSize, maxTruncatedContainerDepth } = result.metrics + if (result.metrics) { + const { maxTruncatedString, maxTruncatedContainerSize, maxTruncatedContainerDepth } = result.metrics - if (maxTruncatedString) metrics.maxTruncatedString = maxTruncatedString - if (maxTruncatedContainerSize) metrics.maxTruncatedContainerSize = maxTruncatedContainerSize - if (maxTruncatedContainerDepth) metrics.maxTruncatedContainerDepth = maxTruncatedContainerDepth - } - - this.addressesToSkip = newAddressesToSkip - - const ruleTriggered = !!result.events?.length - - const blockTriggered = !!getBlockingAction(result.actions) - - // SPECIAL CASE FOR USER_ID - // TODO: make this universal - if (userId && ruleTriggered && blockTriggered) { - this.setUserIdCache(userId, result) - } + if (maxTruncatedString) metrics.maxTruncatedString = maxTruncatedString + if (maxTruncatedContainerSize) metrics.maxTruncatedContainerSize = maxTruncatedContainerSize + if (maxTruncatedContainerDepth) metrics.maxTruncatedContainerDepth = maxTruncatedContainerDepth + } - if (wafRunFinished.hasSubscribers) { - wafRunFinished.publish({ payload }) - } + this.addressesToSkip = newAddressesToSkip - metrics.duration = result.totalRuntime / 1e3 - metrics.blockTriggered = blockTriggered - metrics.ruleTriggered = ruleTriggered - metrics.wafTimeout = result.timeout + const ruleTriggered = !!result.events?.length - Reporter.reportMetrics(metrics, raspRule) + const blockTriggered = !!getBlockingAction(result.actions) - if (ruleTriggered) { - Reporter.reportAttack(JSON.stringify(result.events)) - } + // SPECIAL CASE FOR USER_ID + // TODO: make this universal + if (userId && ruleTriggered && blockTriggered) { + this.setUserIdCache(userId, result) + } - Reporter.reportDerivatives(result.derivatives) + metrics.duration = result.totalRuntime / 1e3 + metrics.blockTriggered = blockTriggered + metrics.ruleTriggered = ruleTriggered + metrics.wafTimeout = result.timeout - return result.actions - } + if (ruleTriggered) { + Reporter.reportAttack(JSON.stringify(result.events)) + } - runWaf (payload) { - try { - const result = this.ddwafContext.run(payload, this.wafTimeout) + Reporter.reportDerivatives(result.derivatives) - return result + return result.actions } catch (err) { log.error('[ASM] Error while running the AppSec WAF', err) - return null + metrics.errorCode = err.errorCode ?? -127 + } finally { + if (wafRunFinished.hasSubscribers) { + wafRunFinished.publish({ payload }) + } + + Reporter.reportMetrics(metrics, raspRule) } } diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index 8829670139e..dc39c978f96 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -4,25 +4,50 @@ const proxyquire = require('proxyquire') const WAFContextWrapper = require('../../../src/appsec/waf/waf_context_wrapper') const addresses = require('../../../src/appsec/addresses') const { wafRunFinished } = require('../../../src/appsec/channels') +const Reporter = require('../../../src/appsec/reporter') describe('WAFContextWrapper', () => { - let ddwafContext + const knownAddresses = new Set([ + addresses.HTTP_INCOMING_QUERY, + addresses.HTTP_INCOMING_GRAPHQL_RESOLVER + ]) + + let reportMetricsStub - beforeEach(() => { - ddwafContext = { + before(() => { + reportMetricsStub = sinon.stub(Reporter, 'reportMetrics') + }) + + afterEach(() => { + reportMetricsStub.resetHistory() + }) + + it('Should send HTTP_INCOMING_QUERY only once', () => { + const ddwafContext = { run: sinon.stub().returns({ events: {}, derivatives: {} }) } - }) + const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) - const knownAddresses = new Set([ - addresses.HTTP_INCOMING_QUERY, - addresses.HTTP_INCOMING_GRAPHQL_RESOLVER - ]) + const payload = { + persistent: { + [addresses.HTTP_INCOMING_QUERY]: { key: 'value' } + } + } - it('Should send HTTP_INCOMING_QUERY only once', () => { + wafContextWrapper.run(payload) + wafContextWrapper.run(payload) + + expect(ddwafContext.run).to.have.been.calledOnceWithExactly(payload, 1000) + expect(reportMetricsStub).to.have.been.calledOnce + }) + + it('Should send HTTP_INCOMING_QUERY twice if waf run returns null', () => { + const ddwafContext = { + run: sinon.stub() + } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { @@ -34,10 +59,25 @@ describe('WAFContextWrapper', () => { wafContextWrapper.run(payload) wafContextWrapper.run(payload) - expect(ddwafContext.run).to.have.been.calledOnceWithExactly(payload, 1000) + expect(ddwafContext.run).to.have.been.calledTwice + expect(ddwafContext.run).to.have.been.calledWithExactly(payload, 1000) + + const firstCall = reportMetricsStub.getCall(0).args[0] + expect(firstCall).to.have.property('errorCode') + expect(firstCall.errorCode).to.equal(-127) + + const secondCall = reportMetricsStub.getCall(1).args[0] + expect(secondCall).to.have.property('errorCode') + expect(secondCall.errorCode).to.equal(-127) }) it('Should send ephemeral addresses every time', () => { + const ddwafContext = { + run: sinon.stub().returns({ + events: {}, + derivatives: {} + }) + } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { @@ -61,9 +101,13 @@ describe('WAFContextWrapper', () => { } } }, 1000) + expect(reportMetricsStub).to.have.been.calledTwice }) it('Should ignore run without known addresses', () => { + const ddwafContext = { + run: sinon.stub() + } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { @@ -81,6 +125,9 @@ describe('WAFContextWrapper', () => { }) it('should publish the payload in the dc channel', () => { + const ddwafContext = { + run: sinon.stub().returns([]) + } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) const payload = { persistent: { @@ -110,10 +157,7 @@ describe('WAFContextWrapper', () => { } ddwafContext = { - run: sinon.stub().returns({ - events: {}, - derivatives: {} - }) + run: sinon.stub() } const ProxiedWafContextWrapper = proxyquire('../../../src/appsec/waf/waf_context_wrapper', { @@ -139,6 +183,7 @@ describe('WAFContextWrapper', () => { wafContextWrapper.run(payload) sinon.assert.calledOnce(ddwafContext.run) + expect(reportMetricsStub).to.have.been.calledOnce }) it('Should not call run and log a warn if context is disposed', () => { @@ -154,6 +199,7 @@ describe('WAFContextWrapper', () => { sinon.assert.notCalled(ddwafContext.run) sinon.assert.calledOnceWithExactly(log.warn, '[ASM] Calling run on a disposed context') + expect(reportMetricsStub).to.not.have.been.called }) }) }) From 2ec60f7355bc5798d25377ab74671c5f8dedca31 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 14:55:24 +0100 Subject: [PATCH 50/56] add unit tests --- packages/dd-trace/test/appsec/graphql.spec.js | 67 ++++++++++++------- .../dd-trace/test/appsec/reporter.spec.js | 32 +++++++++ 2 files changed, 74 insertions(+), 25 deletions(-) diff --git a/packages/dd-trace/test/appsec/graphql.spec.js b/packages/dd-trace/test/appsec/graphql.spec.js index 308103fad87..7ae77f8a4d7 100644 --- a/packages/dd-trace/test/appsec/graphql.spec.js +++ b/packages/dd-trace/test/appsec/graphql.spec.js @@ -158,6 +158,16 @@ describe('GraphQL', () => { describe('block response', () => { const req = {} const res = {} + const resolverInfo = { + user: [{ id: '1234' }] + } + const blockParameters = { + status_code: '401', + type: 'auto', + grpc_status_code: '10' + } + + let context, rootSpan beforeEach(() => { sinon.stub(storage('legacy'), 'getStore').returns({ req, res }) @@ -165,6 +175,12 @@ describe('GraphQL', () => { graphql.enable() graphqlMiddlewareChannel.start.publish({ req, res }) apolloChannel.start.publish() + context = { + abortController: { + abort: sinon.stub() + } + } + rootSpan = { setTag: sinon.stub() } }) afterEach(() => { @@ -174,16 +190,6 @@ describe('GraphQL', () => { }) it('Should not call abort', () => { - const context = { - abortController: { - abort: sinon.stub() - } - } - - const resolverInfo = { - user: [{ id: '1234' }] - } - const abortController = {} sinon.stub(waf, 'run').returns(['']) @@ -204,23 +210,34 @@ describe('GraphQL', () => { }) it('Should call abort', () => { - const context = { - abortController: { - abort: sinon.stub() + const abortController = context.abortController + + sinon.stub(waf, 'run').returns({ + block_request: blockParameters + }) + + sinon.stub(web, 'root').returns(rootSpan) + + startGraphqlResolve.publish({ context, resolverInfo }) + + expect(waf.run).to.have.been.calledOnceWithExactly({ + ephemeral: { + [addresses.HTTP_INCOMING_GRAPHQL_RESOLVER]: resolverInfo } - } + }, {}) - const resolverInfo = { - user: [{ id: '1234' }] - } + expect(context.abortController.abort).to.have.been.called - const blockParameters = { - status_code: '401', - type: 'auto', - grpc_status_code: '10' - } + const abortData = {} + apolloChannel.asyncEnd.publish({ abortController, abortData }) + + expect(blocking.getBlockingData).to.have.been.calledOnceWithExactly(req, 'graphql', blockParameters) + + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') + }) - const rootSpan = { setTag: sinon.stub() } + it('Should fail to block', () => { + blocking.getBlockingData.returns(undefined) const abortController = context.abortController @@ -238,14 +255,14 @@ describe('GraphQL', () => { } }, {}) - expect(context.abortController.abort).to.have.been.called + expect(context.abortController.abort).to.have.been.calledOnce const abortData = {} apolloChannel.asyncEnd.publish({ abortController, abortData }) expect(blocking.getBlockingData).to.have.been.calledOnceWithExactly(req, 'graphql', blockParameters) - expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.block.failed', 1) }) }) }) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index 55d70317660..606a5269ada 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -703,6 +703,22 @@ describe('reporter', () => { expect(span.setTag).to.not.have.been.calledWith('_dd.appsec.rasp.rule.eval') }) + it('should set waf.timeouts tags if there are metrics stored', () => { + telemetry.getRequestMetrics.returns({ wafTimeouts: true }) + + Reporter.finishRequest({}, {}) + + expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.waf.timeouts', true) + }) + + it('should set waf error code tags if there are metrics stored', () => { + telemetry.getRequestMetrics.returns({ wafErrorCode: -1 }) + + Reporter.finishRequest({}, {}) + + expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.waf.error', -1) + }) + it('should set rasp.duration tags if there are metrics stored', () => { telemetry.getRequestMetrics.returns({ raspDuration: 123, raspDurationExt: 321, raspEvalCount: 3 }) @@ -715,6 +731,22 @@ describe('reporter', () => { expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.rasp.rule.eval', 3) }) + it('should set rasp.timeout tags if there are metrics stored', () => { + telemetry.getRequestMetrics.returns({ raspTimeouts: true }) + + Reporter.finishRequest({}, {}) + + expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.rasp.timeout', true) + }) + + it('should set rasp error code tags if there are metrics stored', () => { + telemetry.getRequestMetrics.returns({ raspErrorCode: -1 }) + + Reporter.finishRequest({}, {}) + + expect(span.setTag).to.have.been.calledWithExactly('_dd.appsec.rasp.error', -1) + }) + it('should keep span if there are metrics', () => { const req = {} From 14af79abd537a923084b0da4447ca133ce6c63b9 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 16:03:19 +0100 Subject: [PATCH 51/56] add telemetry tests --- .../test/appsec/telemetry/rasp.spec.js | 34 ++++++++++++++----- .../test/appsec/telemetry/waf.spec.js | 16 +++++++++ 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/packages/dd-trace/test/appsec/telemetry/rasp.spec.js b/packages/dd-trace/test/appsec/telemetry/rasp.spec.js index f12223b9a76..3640fb3e9d4 100644 --- a/packages/dd-trace/test/appsec/telemetry/rasp.spec.js +++ b/packages/dd-trace/test/appsec/telemetry/rasp.spec.js @@ -38,7 +38,7 @@ describe('Appsec Rasp Telemetry metrics', () => { appsecTelemetry.updateRaspRequestsMetricTags({ duration: 42, durationExt: 52 - }, req, 'rule-type') + }, req, { type: 'rule-type' }) expect(count).to.have.been.calledWith('rasp.rule.eval') expect(count).to.not.have.been.calledWith('rasp.timeout') @@ -51,7 +51,7 @@ describe('Appsec Rasp Telemetry metrics', () => { duration: 42, durationExt: 52, wafTimeout: true - }, req, 'rule-type') + }, req, { type: 'rule-type' }) expect(count).to.have.been.calledWith('rasp.rule.eval') expect(count).to.have.been.calledWith('rasp.timeout') @@ -64,7 +64,7 @@ describe('Appsec Rasp Telemetry metrics', () => { duration: 42, durationExt: 52, ruleTriggered: true - }, req, 'rule-type') + }, req, { type: 'rule-type' }) expect(count).to.have.been.calledWith('rasp.rule.match') expect(count).to.have.been.calledWith('rasp.rule.eval') @@ -76,12 +76,12 @@ describe('Appsec Rasp Telemetry metrics', () => { appsecTelemetry.updateRaspRequestsMetricTags({ duration: 42, durationExt: 52 - }, req, 'rule-type') + }, req, { type: 'rule-type' }) appsecTelemetry.updateRaspRequestsMetricTags({ duration: 24, durationExt: 25 - }, req, 'rule-type') + }, req, { type: 'rule-type' }) const { duration, @@ -97,6 +97,22 @@ describe('Appsec Rasp Telemetry metrics', () => { expect(raspDurationExt).to.be.eq(77) expect(raspEvalCount).to.be.eq(2) }) + + it('should increment raspTimeouts if wafTimeout is true', () => { + appsecTelemetry.updateRaspRequestsMetricTags({ wafTimeout: true }, req, { type: 'rule-type' }) + appsecTelemetry.updateRaspRequestsMetricTags({ wafTimeout: true }, req, { type: 'rule-type' }) + + const { raspTimeouts } = appsecTelemetry.getRequestMetrics(req) + expect(raspTimeouts).to.equal(2) + }) + + it('should keep the maximum raspErrorCode', () => { + appsecTelemetry.updateRaspRequestsMetricTags({ errorCode: -1 }, req, { type: 'rule-type' }) + appsecTelemetry.updateRaspRequestsMetricTags({ errorCode: -3 }, req, { type: 'rule-type' }) + + const { raspErrorCode } = appsecTelemetry.getRequestMetrics(req) + expect(raspErrorCode).to.equal(-1) + }) }) describe('incWafRequestsMetric', () => { @@ -115,7 +131,7 @@ describe('Appsec Rasp Telemetry metrics', () => { wafTimeout: true, wafVersion, rulesVersion - }, req, 'rule_type') + }, req, { type: 'rule-type' }) expect(count).to.have.not.been.calledWith('waf.requests') appsecTelemetry.incrementWafRequestsMetric(req) @@ -189,12 +205,12 @@ describe('Appsec Rasp Telemetry metrics', () => { appsecTelemetry.updateRaspRequestsMetricTags({ duration: 42, durationExt: 52 - }, req, 'rule_type') + }, req, { type: 'rule-type' }) appsecTelemetry.updateRaspRequestsMetricTags({ duration: 24, durationExt: 25 - }, req, 'rule_type') + }, req, { type: 'rule-type' }) const { raspDuration, raspDurationExt, raspEvalCount } = appsecTelemetry.getRequestMetrics(req) @@ -212,7 +228,7 @@ describe('Appsec Rasp Telemetry metrics', () => { appsecTelemetry.updateRaspRequestsMetricTags({ duration: 24, durationExt: 25 - }, req, 'rule_type') + }, req, { type: 'rule-type' }) expect(count).to.not.have.been.called expect(inc).to.not.have.been.called diff --git a/packages/dd-trace/test/appsec/telemetry/waf.spec.js b/packages/dd-trace/test/appsec/telemetry/waf.spec.js index ceaa31c0db2..3e16714c234 100644 --- a/packages/dd-trace/test/appsec/telemetry/waf.spec.js +++ b/packages/dd-trace/test/appsec/telemetry/waf.spec.js @@ -159,6 +159,22 @@ describe('Appsec Waf Telemetry metrics', () => { expect(duration).to.be.eq(66) expect(durationExt).to.be.eq(77) }) + + it('should increment wafTimeouts if wafTimeout is true', () => { + appsecTelemetry.updateWafRequestsMetricTags({ wafTimeout: true }, req) + appsecTelemetry.updateWafRequestsMetricTags({ wafTimeout: true }, req) + + const { wafTimeouts } = appsecTelemetry.getRequestMetrics(req) + expect(wafTimeouts).to.equal(2) + }) + + it('should keep the maximum wafErrorCode', () => { + appsecTelemetry.updateWafRequestsMetricTags({ errorCode: -1 }, req) + appsecTelemetry.updateWafRequestsMetricTags({ errorCode: -3 }, req) + + const { wafErrorCode } = appsecTelemetry.getRequestMetrics(req) + expect(wafErrorCode).to.equal(-1) + }) }) describe('incWafInitMetric', () => { From afe2e552b33d40ce6cbc74cf8993ea6c05bbfd5f Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 17:06:59 +0100 Subject: [PATCH 52/56] test message --- packages/dd-trace/test/appsec/blocking.spec.js | 2 +- packages/dd-trace/test/appsec/graphql.spec.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 6f311054810..692ef58b6ef 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -63,7 +63,7 @@ describe('blocking', () => { expect(blocked).to.be.false expect(log.warn).to.have.been .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') - expect(rootSpan.setTag).to.have.been.called + expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('_dd.appsec.block.failed', 1) expect(res.setHeader).to.not.have.been.called expect(res.constructor.prototype.end).to.not.have.been.called }) diff --git a/packages/dd-trace/test/appsec/graphql.spec.js b/packages/dd-trace/test/appsec/graphql.spec.js index 7ae77f8a4d7..ab030e3082b 100644 --- a/packages/dd-trace/test/appsec/graphql.spec.js +++ b/packages/dd-trace/test/appsec/graphql.spec.js @@ -236,7 +236,7 @@ describe('GraphQL', () => { expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('appsec.blocked', 'true') }) - it('Should fail to block', () => { + it('Should catch error when block fails', () => { blocking.getBlockingData.returns(undefined) const abortController = context.abortController @@ -255,7 +255,7 @@ describe('GraphQL', () => { } }, {}) - expect(context.abortController.abort).to.have.been.calledOnce + expect(abortController.abort).to.have.been.calledOnce const abortData = {} apolloChannel.asyncEnd.publish({ abortController, abortData }) From 3804308342466532b597629ff1611c4593893eae Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 17:14:24 +0100 Subject: [PATCH 53/56] replace total runtime with duration --- packages/dd-trace/test/appsec/reporter.spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/reporter.spec.js b/packages/dd-trace/test/appsec/reporter.spec.js index 606a5269ada..271a81e2725 100644 --- a/packages/dd-trace/test/appsec/reporter.spec.js +++ b/packages/dd-trace/test/appsec/reporter.spec.js @@ -153,7 +153,7 @@ describe('reporter', () => { it('should do nothing when rootSpan is not available', () => { web.root.returns(null) - const metrics = { durationExt: 42, totalRuntime: 1337000 } + const metrics = { durationExt: 42, duration: 1337000 } Reporter.reportMetrics(metrics) @@ -161,8 +161,8 @@ describe('reporter', () => { expect(telemetry.updateRaspRequestsMetricTags).not.to.have.been.called }) - it('should set duration metrics from result.totalRuntime', () => { - const metrics = { durationExt: 42, totalRuntime: 1337000 } + it('should set duration metric if set', () => { + const metrics = { duration: 1337000 } Reporter.reportMetrics(metrics) From bb3e807cd3cb6f83260d4820b7a228b95c9ff85e Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 18:29:02 +0100 Subject: [PATCH 54/56] reporter test --- .../appsec/waf/waf_context_wrapper.spec.js | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index dc39c978f96..efb75bfd0b3 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -12,14 +12,12 @@ describe('WAFContextWrapper', () => { addresses.HTTP_INCOMING_GRAPHQL_RESOLVER ]) - let reportMetricsStub - - before(() => { - reportMetricsStub = sinon.stub(Reporter, 'reportMetrics') + beforeEach(() => { + sinon.stub(Reporter, 'reportMetrics') }) afterEach(() => { - reportMetricsStub.resetHistory() + sinon.restore() }) it('Should send HTTP_INCOMING_QUERY only once', () => { @@ -41,7 +39,7 @@ describe('WAFContextWrapper', () => { wafContextWrapper.run(payload) expect(ddwafContext.run).to.have.been.calledOnceWithExactly(payload, 1000) - expect(reportMetricsStub).to.have.been.calledOnce + expect(Reporter.reportMetrics).to.have.been.calledOnce }) it('Should send HTTP_INCOMING_QUERY twice if waf run returns null', () => { @@ -62,11 +60,11 @@ describe('WAFContextWrapper', () => { expect(ddwafContext.run).to.have.been.calledTwice expect(ddwafContext.run).to.have.been.calledWithExactly(payload, 1000) - const firstCall = reportMetricsStub.getCall(0).args[0] + const firstCall = Reporter.reportMetrics.getCall(0).args[0] expect(firstCall).to.have.property('errorCode') expect(firstCall.errorCode).to.equal(-127) - const secondCall = reportMetricsStub.getCall(1).args[0] + const secondCall = Reporter.reportMetrics.getCall(1).args[0] expect(secondCall).to.have.property('errorCode') expect(secondCall.errorCode).to.equal(-127) }) @@ -101,7 +99,7 @@ describe('WAFContextWrapper', () => { } } }, 1000) - expect(reportMetricsStub).to.have.been.calledTwice + expect(Reporter.reportMetrics).to.have.been.calledTwice }) it('Should ignore run without known addresses', () => { @@ -121,7 +119,7 @@ describe('WAFContextWrapper', () => { wafContextWrapper.run(payload) - expect(ddwafContext.run).to.have.not.been.called + expect(ddwafContext.run).to.not.have.been.called }) it('should publish the payload in the dc channel', () => { @@ -183,7 +181,7 @@ describe('WAFContextWrapper', () => { wafContextWrapper.run(payload) sinon.assert.calledOnce(ddwafContext.run) - expect(reportMetricsStub).to.have.been.calledOnce + expect(Reporter.reportMetrics).to.have.been.calledOnce }) it('Should not call run and log a warn if context is disposed', () => { @@ -199,7 +197,7 @@ describe('WAFContextWrapper', () => { sinon.assert.notCalled(ddwafContext.run) sinon.assert.calledOnceWithExactly(log.warn, '[ASM] Calling run on a disposed context') - expect(reportMetricsStub).to.not.have.been.called + expect(Reporter.reportMetrics).to.not.have.been.called }) }) }) From e08217d6a33712878ef34cc874f93d99e93d3b8c Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 6 Mar 2025 18:34:42 +0100 Subject: [PATCH 55/56] test title --- packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index efb75bfd0b3..ac77c2c54d1 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -42,9 +42,9 @@ describe('WAFContextWrapper', () => { expect(Reporter.reportMetrics).to.have.been.calledOnce }) - it('Should send HTTP_INCOMING_QUERY twice if waf run returns null', () => { + it('Should send HTTP_INCOMING_QUERY twice if waf run fails', () => { const ddwafContext = { - run: sinon.stub() + run: sinon.stub().throws(new Error('test')) } const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) From ca6ae33d2fa11656a1569fe79a771039cac79f9a Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 7 Mar 2025 10:59:42 +0100 Subject: [PATCH 56/56] add more tests --- .../appsec/waf/waf_context_wrapper.spec.js | 85 +++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js index ac77c2c54d1..c2406221061 100644 --- a/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js +++ b/packages/dd-trace/test/appsec/waf/waf_context_wrapper.spec.js @@ -58,15 +58,13 @@ describe('WAFContextWrapper', () => { wafContextWrapper.run(payload) expect(ddwafContext.run).to.have.been.calledTwice - expect(ddwafContext.run).to.have.been.calledWithExactly(payload, 1000) + expect(ddwafContext.run).to.always.have.been.calledWithExactly(payload, 1000) const firstCall = Reporter.reportMetrics.getCall(0).args[0] - expect(firstCall).to.have.property('errorCode') - expect(firstCall.errorCode).to.equal(-127) + expect(firstCall).to.have.property('errorCode', -127) const secondCall = Reporter.reportMetrics.getCall(1).args[0] - expect(secondCall).to.have.property('errorCode') - expect(secondCall.errorCode).to.equal(-127) + expect(secondCall).to.have.property('errorCode', -127) }) it('Should send ephemeral addresses every time', () => { @@ -144,6 +142,83 @@ describe('WAFContextWrapper', () => { expect(finishedCallback).to.be.calledOnceWith({ payload }) }) + it('should report error code when the waf run fails', () => { + const ddwafContext = { + run: sinon.stub().returns({ errorCode: -2 }) + } + + const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) + + const payload = { + persistent: { + [addresses.HTTP_INCOMING_QUERY]: { key: 'value' } + } + } + + wafContextWrapper.run(payload) + + expect(Reporter.reportMetrics).to.have.been.calledOnce + const reportedMetrics = Reporter.reportMetrics.getCall(0).args[0] + + expect(reportedMetrics).to.include({ + rulesVersion: '1.8.0', + wafVersion: '1.14.0', + wafTimeout: false, + blockTriggered: false, + ruleTriggered: false, + errorCode: -2, + maxTruncatedString: null, + maxTruncatedContainerSize: null, + maxTruncatedContainerDepth: null + }) + }) + + it('should report truncation metrics, blockTriggered, and ruleTriggered on successful waf run', () => { + const ddwafContext = { + run: sinon.stub().returns({ + events: [{ rule_matches: [] }], + derivatives: [], + actions: { + redirect_request: { + status_code: 301 + } + }, + totalRuntime: 123456, + timeout: false, + metrics: { + maxTruncatedString: 5000, + maxTruncatedContainerSize: 300, + maxTruncatedContainerDepth: 20 + } + }) + } + + const wafContextWrapper = new WAFContextWrapper(ddwafContext, 1000, '1.14.0', '1.8.0', knownAddresses) + + const payload = { + persistent: { + [addresses.HTTP_INCOMING_QUERY]: { key: 'value' } + } + } + + wafContextWrapper.run(payload) + + expect(Reporter.reportMetrics).to.have.been.calledOnce + const reportedMetrics = Reporter.reportMetrics.getCall(0).args[0] + + expect(reportedMetrics).to.include({ + rulesVersion: '1.8.0', + wafVersion: '1.14.0', + wafTimeout: false, + blockTriggered: true, + ruleTriggered: true, + errorCode: null, + maxTruncatedString: 5000, + maxTruncatedContainerSize: 300, + maxTruncatedContainerDepth: 20 + }) + }) + describe('Disposal context check', () => { let log let ddwafContext