From 1cd8d764ab4ffb42ce6d1cec82e620ed845cf75b Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 11 Oct 2024 08:38:39 +0200 Subject: [PATCH 01/12] Path Parameters blocking --- .../datadog-instrumentations/src/express.js | 12 +- packages/dd-trace/src/appsec/channels.js | 1 + packages/dd-trace/src/appsec/index.js | 23 +- .../dd-trace/test/appsec/express-rules.json | 25 ++ .../test/appsec/index.express.plugin.spec.js | 376 +++++++++++++----- packages/dd-trace/test/appsec/index.spec.js | 8 +- 6 files changed, 341 insertions(+), 104 deletions(-) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index c47feef2468..57c8b9309db 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -106,7 +106,17 @@ const wrapProcessParamsMethod = (requestPositionInArguments) => { return (original) => { return function () { if (processParamsStartCh.hasSubscribers) { - processParamsStartCh.publish({ req: arguments[requestPositionInArguments] }) + const req = arguments[requestPositionInArguments] + const abortController = new AbortController() + + processParamsStartCh.publish({ + req, + res: req?.res, + abortController, + params: req?.params + }) + + if (abortController.signal.aborted) return } return original.apply(this, arguments) diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index 729f4da0334..6bdaf7b1df3 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -18,6 +18,7 @@ module.exports = { nextBodyParsed: dc.channel('apm:next:body-parsed'), nextQueryParsed: dc.channel('apm:next:query-parsed'), responseBody: dc.channel('datadog:express:response:json:start'), + expressProcessParams: dc.channel('datadog:express:process_params:start'), responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'), httpClientRequestStart: dc.channel('apm:http:client:request:start'), responseSetHeader: dc.channel('datadog:http:server:response:set-header:start'), diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 4fea5ead514..7bffeb7080c 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -6,6 +6,7 @@ const remoteConfig = require('./remote_config') const { bodyParser, cookieParser, + expressProcessParams, incomingHttpRequestStart, incomingHttpRequestEnd, passportVerify, @@ -64,6 +65,7 @@ function enable (_config) { responseBody.subscribe(onResponseBody) responseWriteHead.subscribe(onResponseWriteHead) responseSetHeader.subscribe(onResponseSetHeader) + expressProcessParams.subscribe(onRequestProcessParams) if (_config.appsec.eventTracking.enabled) { passportVerify.subscribe(onPassportVerify) @@ -122,11 +124,6 @@ function incomingHttpEndTranslator ({ req, res }) { persistent[addresses.HTTP_INCOMING_BODY] = req.body } - // TODO: temporary express instrumentation, will use express plugin later - if (req.params !== null && typeof req.params === 'object') { - persistent[addresses.HTTP_INCOMING_PARAMS] = req.params - } - // we need to keep this to support other cookie parsers if (req.cookies !== null && typeof req.cookies === 'object') { persistent[addresses.HTTP_INCOMING_COOKIES] = req.cookies @@ -262,6 +259,21 @@ function onResponseSetHeader ({ res, abortController }) { } } +function onRequestProcessParams ({ req, res, abortController, params }) { + const rootSpan = web.root(req) + if (!rootSpan) return + + if (!params || typeof params !== 'object' || !Object.keys(params).length) return + + const results = waf.run({ + persistent: { + [addresses.HTTP_INCOMING_PARAMS]: params + } + }, req) + + handleResults(results, req, res, rootSpan, abortController) +} + function handleResults (actions, req, res, rootSpan, abortController) { if (!actions || !req || !res || !rootSpan || !abortController) return @@ -297,6 +309,7 @@ function disable () { if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify) if (responseWriteHead.hasSubscribers) responseWriteHead.unsubscribe(onResponseWriteHead) if (responseSetHeader.hasSubscribers) responseSetHeader.unsubscribe(onResponseSetHeader) + if (expressProcessParams.hasSubscribers) expressProcessParams.unsubscribe(onRequestProcessParams) } module.exports = { diff --git a/packages/dd-trace/test/appsec/express-rules.json b/packages/dd-trace/test/appsec/express-rules.json index 8c5dfeaba31..e8dd910bd02 100644 --- a/packages/dd-trace/test/appsec/express-rules.json +++ b/packages/dd-trace/test/appsec/express-rules.json @@ -28,6 +28,31 @@ ], "transformers": ["lowercase"], "on_match": ["block"] + }, + { + "id": "test-rule-id-2", + "name": "test-rule-name-2", + "tags": { + "type": "security_scanner", + "category": "attack_attempt" + }, + "conditions": [ + { + "parameters": { + "inputs": [ + { + "address": "server.request.path_params" + } + ], + "list": [ + "testattack" + ] + }, + "operator": "phrase_match" + } + ], + "transformers": ["lowercase"], + "on_match": ["block"] } ] } diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index e8b0d4a50e4..983ec800697 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -9,41 +9,45 @@ const { json } = require('../../src/appsec/blocked_templates') const zlib = require('zlib') withVersions('express', 'express', version => { - describe('Suspicious request blocking - query', () => { - let port, server, requestBody + describe('Suspicious request blocking - path parameters', () => { + let port, server, paramCallbackSpy before(() => { - return agent.load(['express', 'http'], { client: false }) + return agent.load(['express', 'http'], {client: false}) }) before((done) => { const express = require('../../../../versions/express').get() - const bodyParser = require('../../../../versions/body-parser').get() const app = express() - app.use(bodyParser.json()) - app.get('/', (req, res) => { - requestBody() - res.end('DONE') + app.get('/multiple-path-params/:parameter1/:parameter2', (req, res) => { + res.send('DONE') }) - app.post('/', (req, res) => { + const nestedRouter = express.Router({ mergeParams: true }) + nestedRouter.get('/:nestedDuplicatedParameter', (req, res) => { res.send('DONE') }) - app.post('/sendjson', (req, res) => { - res.send({ sendResKey: 'sendResValue' }) - }) + app.use('/nested/:nestedDuplicatedParameter', nestedRouter) - app.post('/jsonp', (req, res) => { - res.jsonp({ jsonpResKey: 'jsonpResValue' }) + app.get('/callback-path-param/:callbackedParameter', (req, res) => { + res.send('DONE') }) - app.post('/json', (req, res) => { - res.jsonp({ jsonResKey: 'jsonResValue' }) + const paramCallback = (req, res, next) => { + next() + } + + paramCallbackSpy = sinon.spy(paramCallback) + + app.param(() => { + return paramCallbackSpy }) + app.param('callbackedParameter') + server = app.listen(port, () => { port = server.address().port done() @@ -52,138 +56,322 @@ withVersions('express', 'express', version => { after(() => { server.close() - return agent.close({ ritmReset: false }) + return agent.close({ritmReset: false}) + }) + + beforeEach(async () => { + appsec.enable(new Config({ + appsec: { + enabled: true, + rules: path.join(__dirname, 'express-rules.json') + } + })) + }) + + afterEach(() => { + appsec.disable() + sinon.reset() }) - describe('Blocking', () => { - beforeEach(async () => { - requestBody = sinon.stub() - appsec.enable(new Config({ appsec: { enabled: true, rules: path.join(__dirname, 'express-rules.json') } })) + describe('route with multiple path parameters', () => { + it('should not block the request when attack is not detected', async () => { + const res = await axios.get(`http://localhost:${port}/multiple-path-params/safe_param/safe_param`) + expect(res.data).to.be.equal('DONE') + }) + + it('should block the request when attack is detected', async () => { + try { + await axios.get(`http://localhost:${port}/multiple-path-params/testattack/testattack`) + + 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)) + } }) - afterEach(() => { - appsec.disable() + it('should block the request when attack is detected', async () => { + try { + await axios.get(`http://localhost:${port}/multiple-path-params/testattack/safe_param`) + + 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)) + } }) - it('should not block the request without an attack', async () => { - const res = await axios.get(`http://localhost:${port}/?key=value`) + it('should block the request when attack is detected', async () => { + try { + await axios.get(`http://localhost:${port}/multiple-path-params/safe_param/testattack`) + + 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).to.be.calledOnce + describe('nested routers', () => { + it('should not block the request when attack is not detected', async () => { + const res = await axios.get(`http://localhost:${port}/nested/safe_param/safe_param`) expect(res.data).to.be.equal('DONE') }) it('should block the request when attack is detected', async () => { try { - await axios.get(`http://localhost:${port}/?key=testattack`) + await axios.get(`http://localhost:${port}/nested/safe_param/testattack`) + + 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)) + } + }) + + it('should block the request when attack is detected', async () => { + try { + await axios.get(`http://localhost:${port}/nested/testattack/safe_param`) + + 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)) + } + }) + + it('should block the request when attack is detected', async () => { + try { + await axios.get(`http://localhost:${port}/nested/testattack/testattack`) 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 } }) }) - describe('Api Security', () => { - let config + describe('path parameter callback', () => { + it('should not block the request when attack is not detected', async () => { + const res = await axios.get(`http://localhost:${port}/callback-path-param/safe_param`) + expect(res.data).to.be.equal('DONE') + expect(paramCallbackSpy).to.be.calledOnce + }) - beforeEach(() => { - config = new Config({ - appsec: { - enabled: true, - rules: path.join(__dirname, 'api_security_rules.json'), - apiSecurity: { - enabled: true - } - } - }) + it('should block the request when attack is detected', async () => { + try { + await axios.get(`http://localhost:${port}/callback-path-param/testattack`) + + 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(paramCallbackSpy).to.not.be.called + } }) + }) + }) + + describe('Suspicious request blocking - query', () => { + let port, server, requestBody + + before(() => { + return agent.load(['express', 'http'], {client: false}) + }) - afterEach(() => { - appsec.disable() + before((done) => { + const express = require('../../../../versions/express').get() + + const app = express() + + app.get('/', (req, res) => { + requestBody() + res.end('DONE') }) - describe('with requestSampling 1.0', () => { - beforeEach(() => { - config.appsec.apiSecurity.requestSampling = 1.0 - appsec.enable(config) - }) + server = app.listen(port, () => { + port = server.address().port + done() + }) + }) + + after(() => { + server.close() + return agent.close({ritmReset: false}) + }) - function formatSchema (body) { - return zlib.gzipSync(JSON.stringify(body)).toString('base64') + beforeEach(async () => { + requestBody = sinon.stub() + appsec.enable(new Config({ + appsec: { + enabled: true, + rules: path.join(__dirname, 'express-rules.json') } + })) + }) - it('should get the request body schema', async () => { - const expectedRequestBodySchema = formatSchema([{ key: [8] }]) - const res = await axios.post(`http://localhost:${port}/`, { key: 'value' }) + afterEach(() => { + appsec.disable() + }) - await agent.use((traces) => { - const span = traces[0][0] - expect(span.meta).to.haveOwnProperty('_dd.appsec.s.req.body') - expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.res.body') - expect(span.meta['_dd.appsec.s.req.body']).to.be.equal(expectedRequestBodySchema) - }) + it('should not block the request without an attack', async () => { + const res = await axios.get(`http://localhost:${port}/?key=value`) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.equal('DONE') - }) + expect(requestBody).to.be.calledOnce + expect(res.data).to.be.equal('DONE') + }) - it('should get the response body schema with res.send method with object', async () => { - const expectedResponseBodySchema = formatSchema([{ sendResKey: [8] }]) - const res = await axios.post(`http://localhost:${port}/sendjson`, { key: 'value' }) + it('should block the request when attack is detected', async () => { + try { + await axios.get(`http://localhost:${port}/?key=testattack`) - await agent.use((traces) => { - const span = traces[0][0] - expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) - }) + 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 + } + }) + }) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.deep.equal({ sendResKey: 'sendResValue' }) - }) + describe('Api Security', () => { + let config, port, server - it('should get the response body schema with res.json method', async () => { - const expectedResponseBodySchema = formatSchema([{ jsonResKey: [8] }]) - const res = await axios.post(`http://localhost:${port}/json`, { key: 'value' }) + before(() => { + return agent.load(['express', 'http'], {client: false}) + }) - await agent.use((traces) => { - const span = traces[0][0] - expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) - }) + before((done) => { + const express = require('../../../../versions/express').get() + const bodyParser = require('../../../../versions/body-parser').get() - expect(res.status).to.be.equal(200) - expect(res.data).to.be.deep.equal({ jsonResKey: 'jsonResValue' }) - }) + const app = express() + app.use(bodyParser.json()) + + app.post('/', (req, res) => { + res.send('DONE') + }) - it('should get the response body schema with res.jsonp method', async () => { - const expectedResponseBodySchema = formatSchema([{ jsonpResKey: [8] }]) - const res = await axios.post(`http://localhost:${port}/jsonp`, { key: 'value' }) + app.post('/sendjson', (req, res) => { + res.send({ sendResKey: 'sendResValue' }) + }) - await agent.use((traces) => { - const span = traces[0][0] - expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) - }) + app.post('/jsonp', (req, res) => { + res.jsonp({ jsonpResKey: 'jsonpResValue' }) + }) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.deep.equal({ jsonpResKey: 'jsonpResValue' }) - }) + app.post('/json', (req, res) => { + res.jsonp({ jsonResKey: 'jsonResValue' }) }) - it('should not get the schema', async () => { - config.appsec.apiSecurity.requestSampling = 0 + server = app.listen(port, () => { + port = server.address().port + done() + }) + }) + + after(() => { + server.close() + return agent.close({ritmReset: false}) + }) + + beforeEach(() => { + config = new Config({ + appsec: { + enabled: true, + rules: path.join(__dirname, 'api_security_rules.json'), + apiSecurity: { + enabled: true + } + } + }) + }) + + afterEach(() => { + appsec.disable() + }) + + describe('with requestSampling 1.0', () => { + beforeEach(() => { + config.appsec.apiSecurity.requestSampling = 1.0 appsec.enable(config) + }) + + function formatSchema (body) { + return zlib.gzipSync(JSON.stringify(body)).toString('base64') + } + it('should get the request body schema', async () => { + const expectedRequestBodySchema = formatSchema([{ key: [8] }]) const res = await axios.post(`http://localhost:${port}/`, { key: 'value' }) await agent.use((traces) => { const span = traces[0][0] - expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.req.body') + expect(span.meta).to.haveOwnProperty('_dd.appsec.s.req.body') expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.res.body') + expect(span.meta['_dd.appsec.s.req.body']).to.be.equal(expectedRequestBodySchema) }) expect(res.status).to.be.equal(200) expect(res.data).to.be.equal('DONE') }) + + it('should get the response body schema with res.send method with object', async () => { + const expectedResponseBodySchema = formatSchema([{ sendResKey: [8] }]) + const res = await axios.post(`http://localhost:${port}/sendjson`, { key: 'value' }) + + await agent.use((traces) => { + const span = traces[0][0] + expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) + }) + + expect(res.status).to.be.equal(200) + expect(res.data).to.be.deep.equal({ sendResKey: 'sendResValue' }) + }) + + it('should get the response body schema with res.json method', async () => { + const expectedResponseBodySchema = formatSchema([{ jsonResKey: [8] }]) + const res = await axios.post(`http://localhost:${port}/json`, { key: 'value' }) + + await agent.use((traces) => { + const span = traces[0][0] + expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) + }) + + expect(res.status).to.be.equal(200) + expect(res.data).to.be.deep.equal({ jsonResKey: 'jsonResValue' }) + }) + + it('should get the response body schema with res.jsonp method', async () => { + const expectedResponseBodySchema = formatSchema([{ jsonpResKey: [8] }]) + const res = await axios.post(`http://localhost:${port}/jsonp`, { key: 'value' }) + + await agent.use((traces) => { + const span = traces[0][0] + expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) + }) + + expect(res.status).to.be.equal(200) + expect(res.data).to.be.deep.equal({ jsonpResKey: 'jsonpResValue' }) + }) + }) + + it('should not get the schema', async () => { + config.appsec.apiSecurity.requestSampling = 0 + appsec.enable(config) + + const res = await axios.post(`http://localhost:${port}/`, { key: 'value' }) + + await agent.use((traces) => { + const span = traces[0][0] + expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.req.body') + expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.res.body') + }) + + expect(res.status).to.be.equal(200) + expect(res.data).to.be.equal('DONE') }) }) }) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index bb0b994b0d2..8722b9c07fa 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -8,6 +8,7 @@ const appsec = require('../../src/appsec') const { bodyParser, cookieParser, + expressProcessParams, incomingHttpRequestStart, incomingHttpRequestEnd, queryParser, @@ -169,6 +170,7 @@ describe('AppSec Index', function () { it('should subscribe to blockable channels', () => { expect(bodyParser.hasSubscribers).to.be.false expect(cookieParser.hasSubscribers).to.be.false + expect(expressProcessParams.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false @@ -180,6 +182,7 @@ describe('AppSec Index', function () { expect(bodyParser.hasSubscribers).to.be.true expect(cookieParser.hasSubscribers).to.be.true + expect(expressProcessParams.hasSubscribers).to.be.true expect(queryParser.hasSubscribers).to.be.true expect(nextBodyParsed.hasSubscribers).to.be.true expect(nextQueryParsed.hasSubscribers).to.be.true @@ -260,6 +263,7 @@ describe('AppSec Index', function () { expect(bodyParser.hasSubscribers).to.be.false expect(cookieParser.hasSubscribers).to.be.false + expect(expressProcessParams.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false @@ -430,9 +434,6 @@ describe('AppSec Index', function () { route: { path: '/path/:c' }, - params: { - c: '3' - }, cookies: { d: '4', e: '5' @@ -454,7 +455,6 @@ describe('AppSec Index', function () { expect(waf.run).to.have.been.calledOnceWithExactly({ persistent: { 'server.request.body': { a: '1' }, - 'server.request.path_params': { c: '3' }, 'server.request.cookies': { d: '4', e: '5' }, 'server.request.query': { b: '2' } } From b849955cdcd3f68fe2f24f4b81339343f5989ecf Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 11 Oct 2024 08:51:25 +0200 Subject: [PATCH 02/12] Lint --- .../test/appsec/index.express.plugin.spec.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index 983ec800697..66271d5cd7d 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -13,7 +13,7 @@ withVersions('express', 'express', version => { let port, server, paramCallbackSpy before(() => { - return agent.load(['express', 'http'], {client: false}) + return agent.load(['express', 'http'], { client: false }) }) before((done) => { @@ -56,7 +56,7 @@ withVersions('express', 'express', version => { after(() => { server.close() - return agent.close({ritmReset: false}) + return agent.close({ ritmReset: false }) }) beforeEach(async () => { @@ -178,7 +178,7 @@ withVersions('express', 'express', version => { let port, server, requestBody before(() => { - return agent.load(['express', 'http'], {client: false}) + return agent.load(['express', 'http'], { client: false }) }) before((done) => { @@ -199,7 +199,7 @@ withVersions('express', 'express', version => { after(() => { server.close() - return agent.close({ritmReset: false}) + return agent.close({ ritmReset: false }) }) beforeEach(async () => { @@ -240,7 +240,7 @@ withVersions('express', 'express', version => { let config, port, server before(() => { - return agent.load(['express', 'http'], {client: false}) + return agent.load(['express', 'http'], { client: false }) }) before((done) => { @@ -274,7 +274,7 @@ withVersions('express', 'express', version => { after(() => { server.close() - return agent.close({ritmReset: false}) + return agent.close({ ritmReset: false }) }) beforeEach(() => { From bf7d4e669bffd2e033226df4889558bbbf2f775e Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 11 Oct 2024 09:02:25 +0200 Subject: [PATCH 03/12] Change expect to assert in SRB tests --- .../test/appsec/index.express.plugin.spec.js | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index 66271d5cd7d..4fc31ce8488 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -1,6 +1,7 @@ 'use strict' const axios = require('axios') +const { assert } = require('chai') const path = require('path') const agent = require('../plugins/agent') const appsec = require('../../src/appsec') @@ -76,7 +77,8 @@ withVersions('express', 'express', version => { describe('route with multiple path parameters', () => { it('should not block the request when attack is not detected', async () => { const res = await axios.get(`http://localhost:${port}/multiple-path-params/safe_param/safe_param`) - expect(res.data).to.be.equal('DONE') + assert.equal(res.status, 200) + assert.equal(res.data, 'DONE') }) it('should block the request when attack is detected', async () => { @@ -85,8 +87,8 @@ withVersions('express', 'express', version => { 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)) + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) } }) @@ -96,8 +98,8 @@ withVersions('express', 'express', version => { 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)) + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) } }) @@ -107,8 +109,8 @@ withVersions('express', 'express', version => { 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)) + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) } }) }) @@ -116,7 +118,8 @@ withVersions('express', 'express', version => { describe('nested routers', () => { it('should not block the request when attack is not detected', async () => { const res = await axios.get(`http://localhost:${port}/nested/safe_param/safe_param`) - expect(res.data).to.be.equal('DONE') + assert.equal(res.status, 200) + assert.equal(res.data, 'DONE') }) it('should block the request when attack is detected', async () => { @@ -125,8 +128,8 @@ withVersions('express', 'express', version => { 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)) + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) } }) @@ -136,8 +139,8 @@ withVersions('express', 'express', version => { 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)) + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) } }) @@ -147,8 +150,8 @@ withVersions('express', 'express', version => { 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)) + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) } }) }) @@ -156,8 +159,9 @@ withVersions('express', 'express', version => { describe('path parameter callback', () => { it('should not block the request when attack is not detected', async () => { const res = await axios.get(`http://localhost:${port}/callback-path-param/safe_param`) - expect(res.data).to.be.equal('DONE') - expect(paramCallbackSpy).to.be.calledOnce + assert.equal(res.status, 200) + assert.equal(res.data, 'DONE') + sinon.assert.calledOnce(paramCallbackSpy) }) it('should block the request when attack is detected', async () => { @@ -166,9 +170,9 @@ withVersions('express', 'express', version => { 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(paramCallbackSpy).to.not.be.called + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) + sinon.assert.notCalled(paramCallbackSpy) } }) }) @@ -219,8 +223,8 @@ withVersions('express', 'express', version => { it('should not block the request without an attack', async () => { const res = await axios.get(`http://localhost:${port}/?key=value`) - expect(requestBody).to.be.calledOnce - expect(res.data).to.be.equal('DONE') + sinon.assert.calledOnce(requestBody) + assert.equal(res.data, 'DONE') }) it('should block the request when attack is detected', async () => { @@ -229,9 +233,9 @@ withVersions('express', 'express', version => { 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 + assert.equal(e.response.status, 403) + assert.deepEqual(e.response.data, JSON.parse(json)) + sinon.assert.notCalled(requestBody) } }) }) From ac5fc5ce4192b19803d254e1024dc420446d18e1 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 11 Oct 2024 09:10:19 +0200 Subject: [PATCH 04/12] Change expect to assert in API Sec tests --- .../test/appsec/index.express.plugin.spec.js | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index 4fc31ce8488..67b0d69b990 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -223,8 +223,9 @@ withVersions('express', 'express', version => { it('should not block the request without an attack', async () => { const res = await axios.get(`http://localhost:${port}/?key=value`) - sinon.assert.calledOnce(requestBody) + assert.equal(res.status, 200) assert.equal(res.data, 'DONE') + sinon.assert.calledOnce(requestBody) }) it('should block the request when attack is detected', async () => { @@ -313,13 +314,13 @@ withVersions('express', 'express', version => { await agent.use((traces) => { const span = traces[0][0] - expect(span.meta).to.haveOwnProperty('_dd.appsec.s.req.body') - expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.res.body') - expect(span.meta['_dd.appsec.s.req.body']).to.be.equal(expectedRequestBodySchema) + assert.property(span.meta, '_dd.appsec.s.req.body') + assert.notProperty(span.meta, '_dd.appsec.s.res.body') + assert.equal(span.meta['_dd.appsec.s.req.body'], expectedRequestBodySchema) }) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.equal('DONE') + assert.equal(res.status, 200) + assert.equal(res.data, 'DONE') }) it('should get the response body schema with res.send method with object', async () => { @@ -328,11 +329,11 @@ withVersions('express', 'express', version => { await agent.use((traces) => { const span = traces[0][0] - expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) + assert.equal(span.meta['_dd.appsec.s.res.body'], expectedResponseBodySchema) }) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.deep.equal({ sendResKey: 'sendResValue' }) + assert.equal(res.status, 200) + assert.deepEqual(res.data, { sendResKey: 'sendResValue' }) }) it('should get the response body schema with res.json method', async () => { @@ -341,11 +342,11 @@ withVersions('express', 'express', version => { await agent.use((traces) => { const span = traces[0][0] - expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) + assert.equal(span.meta['_dd.appsec.s.res.body'], expectedResponseBodySchema) }) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.deep.equal({ jsonResKey: 'jsonResValue' }) + assert.equal(res.status, 200) + assert.deepEqual(res.data, { jsonResKey: 'jsonResValue' }) }) it('should get the response body schema with res.jsonp method', async () => { @@ -354,11 +355,11 @@ withVersions('express', 'express', version => { await agent.use((traces) => { const span = traces[0][0] - expect(span.meta['_dd.appsec.s.res.body']).to.be.equal(expectedResponseBodySchema) + assert.equal(span.meta['_dd.appsec.s.res.body'], expectedResponseBodySchema) }) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.deep.equal({ jsonpResKey: 'jsonpResValue' }) + assert.equal(res.status, 200) + assert.deepEqual(res.data, { jsonpResKey: 'jsonpResValue' }) }) }) @@ -370,12 +371,12 @@ withVersions('express', 'express', version => { await agent.use((traces) => { const span = traces[0][0] - expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.req.body') - expect(span.meta).not.to.haveOwnProperty('_dd.appsec.s.res.body') + assert.notProperty(span.meta, '_dd.appsec.s.req.body') + assert.notProperty(span.meta, '_dd.appsec.s.res.body') }) - expect(res.status).to.be.equal(200) - expect(res.data).to.be.equal('DONE') + assert.equal(res.status, 200) + assert.equal(res.data, 'DONE') }) }) }) From 4dc37bdf87c174e2f59bc8dff576cf28219be660 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Fri, 11 Oct 2024 09:11:38 +0200 Subject: [PATCH 05/12] Improve test naming --- .../test/appsec/index.express.plugin.spec.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index 67b0d69b990..dc1e5d0acc3 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -81,7 +81,7 @@ withVersions('express', 'express', version => { assert.equal(res.data, 'DONE') }) - it('should block the request when attack is detected', async () => { + it('should block the request when attack is detected in both parameters', async () => { try { await axios.get(`http://localhost:${port}/multiple-path-params/testattack/testattack`) @@ -92,7 +92,7 @@ withVersions('express', 'express', version => { } }) - it('should block the request when attack is detected', async () => { + it('should block the request when attack is detected in the first parameter', async () => { try { await axios.get(`http://localhost:${port}/multiple-path-params/testattack/safe_param`) @@ -103,7 +103,7 @@ withVersions('express', 'express', version => { } }) - it('should block the request when attack is detected', async () => { + it('should block the request when attack is detected in the second parameter', async () => { try { await axios.get(`http://localhost:${port}/multiple-path-params/safe_param/testattack`) @@ -122,7 +122,7 @@ withVersions('express', 'express', version => { assert.equal(res.data, 'DONE') }) - it('should block the request when attack is detected', async () => { + it('should block the request when attack is detected in the nested paremeter', async () => { try { await axios.get(`http://localhost:${port}/nested/safe_param/testattack`) @@ -133,7 +133,7 @@ withVersions('express', 'express', version => { } }) - it('should block the request when attack is detected', async () => { + it('should block the request when attack is detected in the parent paremeter', async () => { try { await axios.get(`http://localhost:${port}/nested/testattack/safe_param`) @@ -144,7 +144,7 @@ withVersions('express', 'express', version => { } }) - it('should block the request when attack is detected', async () => { + it('should block the request when attack is detected both parameters', async () => { try { await axios.get(`http://localhost:${port}/nested/testattack/testattack`) From 506a2bd784beacdf38bf6cc78522590005a44934 Mon Sep 17 00:00:00 2001 From: Carles Capell <107924659+CarlesDD@users.noreply.github.com> Date: Mon, 14 Oct 2024 09:16:22 +0200 Subject: [PATCH 06/12] Correct spacing in tests Co-authored-by: Ugaitz Urien --- packages/dd-trace/test/appsec/index.express.plugin.spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index dc1e5d0acc3..e1e45e14099 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -77,6 +77,7 @@ withVersions('express', 'express', version => { describe('route with multiple path parameters', () => { it('should not block the request when attack is not detected', async () => { const res = await axios.get(`http://localhost:${port}/multiple-path-params/safe_param/safe_param`) + assert.equal(res.status, 200) assert.equal(res.data, 'DONE') }) @@ -118,6 +119,7 @@ withVersions('express', 'express', version => { describe('nested routers', () => { it('should not block the request when attack is not detected', async () => { const res = await axios.get(`http://localhost:${port}/nested/safe_param/safe_param`) + assert.equal(res.status, 200) assert.equal(res.data, 'DONE') }) @@ -310,6 +312,7 @@ withVersions('express', 'express', version => { it('should get the request body schema', async () => { const expectedRequestBodySchema = formatSchema([{ key: [8] }]) + const res = await axios.post(`http://localhost:${port}/`, { key: 'value' }) await agent.use((traces) => { From b5b549a333ccc0214795083642fae4a3d3dc964f Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Mon, 14 Oct 2024 09:28:48 +0200 Subject: [PATCH 07/12] Keep consistency of order in appsec channels --- packages/dd-trace/src/appsec/channels.js | 2 +- packages/dd-trace/src/appsec/index.js | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index 6bdaf7b1df3..3081ed9974a 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -17,8 +17,8 @@ module.exports = { setCookieChannel: dc.channel('datadog:iast:set-cookie'), nextBodyParsed: dc.channel('apm:next:body-parsed'), nextQueryParsed: dc.channel('apm:next:query-parsed'), - responseBody: dc.channel('datadog:express:response:json:start'), expressProcessParams: dc.channel('datadog:express:process_params:start'), + responseBody: dc.channel('datadog:express:response:json:start'), responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'), httpClientRequestStart: dc.channel('apm:http:client:request:start'), responseSetHeader: dc.channel('datadog:http:server:response:set-header:start'), diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 7bffeb7080c..ad25bd3d9c6 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -6,13 +6,13 @@ const remoteConfig = require('./remote_config') const { bodyParser, cookieParser, - expressProcessParams, incomingHttpRequestStart, incomingHttpRequestEnd, passportVerify, queryParser, nextBodyParsed, nextQueryParsed, + expressProcessParams, responseBody, responseWriteHead, responseSetHeader @@ -55,17 +55,17 @@ function enable (_config) { apiSecuritySampler.configure(_config.appsec) + bodyParser.subscribe(onRequestBodyParsed) + cookieParser.subscribe(onRequestCookieParser) incomingHttpRequestStart.subscribe(incomingHttpStartTranslator) incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator) - bodyParser.subscribe(onRequestBodyParsed) + queryParser.subscribe(onRequestQueryParsed) nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) - queryParser.subscribe(onRequestQueryParsed) - cookieParser.subscribe(onRequestCookieParser) + expressProcessParams.subscribe(onRequestProcessParams) responseBody.subscribe(onResponseBody) responseWriteHead.subscribe(onResponseWriteHead) responseSetHeader.subscribe(onResponseSetHeader) - expressProcessParams.subscribe(onRequestProcessParams) if (_config.appsec.eventTracking.enabled) { passportVerify.subscribe(onPassportVerify) @@ -299,17 +299,17 @@ function disable () { // Channel#unsubscribe() is undefined for non active channels if (bodyParser.hasSubscribers) bodyParser.unsubscribe(onRequestBodyParsed) + if (cookieParser.hasSubscribers) cookieParser.unsubscribe(onRequestCookieParser) if (incomingHttpRequestStart.hasSubscribers) incomingHttpRequestStart.unsubscribe(incomingHttpStartTranslator) if (incomingHttpRequestEnd.hasSubscribers) incomingHttpRequestEnd.unsubscribe(incomingHttpEndTranslator) + if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify) if (queryParser.hasSubscribers) queryParser.unsubscribe(onRequestQueryParsed) if (nextBodyParsed.hasSubscribers) nextBodyParsed.unsubscribe(onRequestBodyParsed) if (nextQueryParsed.hasSubscribers) nextQueryParsed.unsubscribe(onRequestQueryParsed) - if (cookieParser.hasSubscribers) cookieParser.unsubscribe(onRequestCookieParser) + if (expressProcessParams.hasSubscribers) expressProcessParams.unsubscribe(onRequestProcessParams) if (responseBody.hasSubscribers) responseBody.unsubscribe(onResponseBody) - if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify) if (responseWriteHead.hasSubscribers) responseWriteHead.unsubscribe(onResponseWriteHead) if (responseSetHeader.hasSubscribers) responseSetHeader.unsubscribe(onResponseSetHeader) - if (expressProcessParams.hasSubscribers) expressProcessParams.unsubscribe(onRequestProcessParams) } module.exports = { From 0e85c9493d4f6e0a7b14b2fde6997c754d55a785 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Mon, 14 Oct 2024 09:33:16 +0200 Subject: [PATCH 08/12] Better wrap fn naming in express instrumentation --- packages/datadog-instrumentations/src/express.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index 57c8b9309db..1d8645e9af1 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -103,8 +103,8 @@ addHook({ const processParamsStartCh = channel('datadog:express:process_params:start') const wrapProcessParamsMethod = (requestPositionInArguments) => { - return (original) => { - return function () { + return function wrapProcessParams (original) { + return function wrappedProcessParams () { if (processParamsStartCh.hasSubscribers) { const req = arguments[requestPositionInArguments] const abortController = new AbortController() From 80cebffed1820487f284897f5556af5264fdd289 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Mon, 14 Oct 2024 09:43:08 +0200 Subject: [PATCH 09/12] Keep consistency of order in appsec channels handlers --- packages/dd-trace/src/appsec/index.js | 96 +++++++++++++-------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index ad25bd3d9c6..f3656e459e8 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -31,6 +31,8 @@ const { storage } = require('../../../datadog-core') const graphql = require('./graphql') const rasp = require('./rasp') +const responseAnalyzedSet = new WeakSet() + let isEnabled = false let config @@ -81,6 +83,41 @@ function enable (_config) { } } +function onRequestBodyParsed ({ req, res, body, abortController }) { + if (body === undefined || body === null) return + + if (!req) { + const store = storage.getStore() + req = store?.req + } + + const rootSpan = web.root(req) + if (!rootSpan) return + + const results = waf.run({ + persistent: { + [addresses.HTTP_INCOMING_BODY]: body + } + }, req) + + handleResults(results, req, res, rootSpan, abortController) +} + +function onRequestCookieParser ({ req, res, abortController, cookies }) { + if (!cookies || typeof cookies !== 'object') return + + const rootSpan = web.root(req) + if (!rootSpan) return + + const results = waf.run({ + persistent: { + [addresses.HTTP_INCOMING_COOKIES]: cookies + } + }, req) + + handleResults(results, req, res, rootSpan, abortController) +} + function incomingHttpStartTranslator ({ req, res, abortController }) { const rootSpan = web.root(req) if (!rootSpan) return @@ -142,24 +179,16 @@ function incomingHttpEndTranslator ({ req, res }) { Reporter.finishRequest(req, res) } -function onRequestBodyParsed ({ req, res, body, abortController }) { - if (body === undefined || body === null) return +function onPassportVerify ({ credentials, user }) { + const store = storage.getStore() + const rootSpan = store?.req && web.root(store.req) - if (!req) { - const store = storage.getStore() - req = store?.req + if (!rootSpan) { + log.warn('No rootSpan found in onPassportVerify') + return } - const rootSpan = web.root(req) - if (!rootSpan) return - - const results = waf.run({ - persistent: { - [addresses.HTTP_INCOMING_BODY]: body - } - }, req) - - handleResults(results, req, res, rootSpan, abortController) + passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode) } function onRequestQueryParsed ({ req, res, query, abortController }) { @@ -182,15 +211,15 @@ function onRequestQueryParsed ({ req, res, query, abortController }) { handleResults(results, req, res, rootSpan, abortController) } -function onRequestCookieParser ({ req, res, abortController, cookies }) { - if (!cookies || typeof cookies !== 'object') return - +function onRequestProcessParams ({ req, res, abortController, params }) { const rootSpan = web.root(req) if (!rootSpan) return + if (!params || typeof params !== 'object' || !Object.keys(params).length) return + const results = waf.run({ persistent: { - [addresses.HTTP_INCOMING_COOKIES]: cookies + [addresses.HTTP_INCOMING_PARAMS]: params } }, req) @@ -209,20 +238,6 @@ function onResponseBody ({ req, body }) { }, req) } -function onPassportVerify ({ credentials, user }) { - const store = storage.getStore() - const rootSpan = store?.req && web.root(store.req) - - if (!rootSpan) { - log.warn('No rootSpan found in onPassportVerify') - return - } - - passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode) -} - -const responseAnalyzedSet = new WeakSet() - function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) { // avoid "write after end" error if (isBlocked(res)) { @@ -259,21 +274,6 @@ function onResponseSetHeader ({ res, abortController }) { } } -function onRequestProcessParams ({ req, res, abortController, params }) { - const rootSpan = web.root(req) - if (!rootSpan) return - - if (!params || typeof params !== 'object' || !Object.keys(params).length) return - - const results = waf.run({ - persistent: { - [addresses.HTTP_INCOMING_PARAMS]: params - } - }, req) - - handleResults(results, req, res, rootSpan, abortController) -} - function handleResults (actions, req, res, rootSpan, abortController) { if (!actions || !req || !res || !rootSpan || !abortController) return From c545063ee0ea6f9fbb8fb076cbe4a09bac712bbe Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Mon, 14 Oct 2024 15:53:48 +0200 Subject: [PATCH 10/12] Keep consistency of order in appsec channels handlers - test --- packages/dd-trace/test/appsec/index.spec.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 8722b9c07fa..4b8c6c0438c 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -8,13 +8,13 @@ const appsec = require('../../src/appsec') const { bodyParser, cookieParser, - expressProcessParams, incomingHttpRequestStart, incomingHttpRequestEnd, + passportVerify, queryParser, nextBodyParsed, nextQueryParsed, - passportVerify, + expressProcessParams, responseBody, responseWriteHead, responseSetHeader @@ -170,11 +170,11 @@ describe('AppSec Index', function () { it('should subscribe to blockable channels', () => { expect(bodyParser.hasSubscribers).to.be.false expect(cookieParser.hasSubscribers).to.be.false - expect(expressProcessParams.hasSubscribers).to.be.false + expect(passportVerify.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false - expect(passportVerify.hasSubscribers).to.be.false + expect(expressProcessParams.hasSubscribers).to.be.false expect(responseWriteHead.hasSubscribers).to.be.false expect(responseSetHeader.hasSubscribers).to.be.false @@ -182,11 +182,11 @@ describe('AppSec Index', function () { expect(bodyParser.hasSubscribers).to.be.true expect(cookieParser.hasSubscribers).to.be.true - expect(expressProcessParams.hasSubscribers).to.be.true + expect(passportVerify.hasSubscribers).to.be.true expect(queryParser.hasSubscribers).to.be.true expect(nextBodyParsed.hasSubscribers).to.be.true expect(nextQueryParsed.hasSubscribers).to.be.true - expect(passportVerify.hasSubscribers).to.be.true + expect(expressProcessParams.hasSubscribers).to.be.true expect(responseWriteHead.hasSubscribers).to.be.true expect(responseSetHeader.hasSubscribers).to.be.true }) @@ -263,11 +263,11 @@ describe('AppSec Index', function () { expect(bodyParser.hasSubscribers).to.be.false expect(cookieParser.hasSubscribers).to.be.false - expect(expressProcessParams.hasSubscribers).to.be.false + expect(passportVerify.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false - expect(passportVerify.hasSubscribers).to.be.false + expect(expressProcessParams.hasSubscribers).to.be.false expect(responseWriteHead.hasSubscribers).to.be.false expect(responseSetHeader.hasSubscribers).to.be.false }) From 88a344887052aed0c690fa1fd3825560f275e5d5 Mon Sep 17 00:00:00 2001 From: CarlesDD Date: Tue, 15 Oct 2024 17:14:15 +0200 Subject: [PATCH 11/12] Refactor express plugin test - use axios.create and getPort --- .../test/appsec/index.express.plugin.spec.js | 67 ++++++++++--------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index e1e45e14099..c38d496623b 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -1,7 +1,8 @@ 'use strict' -const axios = require('axios') +const Axios = require('axios') const { assert } = require('chai') +const getPort = require('get-port') const path = require('path') const agent = require('../plugins/agent') const appsec = require('../../src/appsec') @@ -11,7 +12,7 @@ const zlib = require('zlib') withVersions('express', 'express', version => { describe('Suspicious request blocking - path parameters', () => { - let port, server, paramCallbackSpy + let server, paramCallbackSpy, axios before(() => { return agent.load(['express', 'http'], { client: false }) @@ -49,9 +50,11 @@ withVersions('express', 'express', version => { app.param('callbackedParameter') - server = app.listen(port, () => { - port = server.address().port - done() + getPort().then((port) => { + server = app.listen(port, () => { + axios = Axios.create({ baseURL: `http://localhost:${port}` }) + done() + }) }) }) @@ -76,7 +79,7 @@ withVersions('express', 'express', version => { describe('route with multiple path parameters', () => { it('should not block the request when attack is not detected', async () => { - const res = await axios.get(`http://localhost:${port}/multiple-path-params/safe_param/safe_param`) + const res = await axios.get('/multiple-path-params/safe_param/safe_param') assert.equal(res.status, 200) assert.equal(res.data, 'DONE') @@ -84,7 +87,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected in both parameters', async () => { try { - await axios.get(`http://localhost:${port}/multiple-path-params/testattack/testattack`) + await axios.get('/multiple-path-params/testattack/testattack') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -95,7 +98,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected in the first parameter', async () => { try { - await axios.get(`http://localhost:${port}/multiple-path-params/testattack/safe_param`) + await axios.get('/multiple-path-params/testattack/safe_param') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -106,7 +109,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected in the second parameter', async () => { try { - await axios.get(`http://localhost:${port}/multiple-path-params/safe_param/testattack`) + await axios.get('/multiple-path-params/safe_param/testattack') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -118,7 +121,7 @@ withVersions('express', 'express', version => { describe('nested routers', () => { it('should not block the request when attack is not detected', async () => { - const res = await axios.get(`http://localhost:${port}/nested/safe_param/safe_param`) + const res = await axios.get('/nested/safe_param/safe_param') assert.equal(res.status, 200) assert.equal(res.data, 'DONE') @@ -126,7 +129,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected in the nested paremeter', async () => { try { - await axios.get(`http://localhost:${port}/nested/safe_param/testattack`) + await axios.get('/nested/safe_param/testattack') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -137,7 +140,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected in the parent paremeter', async () => { try { - await axios.get(`http://localhost:${port}/nested/testattack/safe_param`) + await axios.get('/nested/testattack/safe_param') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -148,7 +151,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected both parameters', async () => { try { - await axios.get(`http://localhost:${port}/nested/testattack/testattack`) + await axios.get('/nested/testattack/testattack') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -160,7 +163,7 @@ withVersions('express', 'express', version => { describe('path parameter callback', () => { it('should not block the request when attack is not detected', async () => { - const res = await axios.get(`http://localhost:${port}/callback-path-param/safe_param`) + const res = await axios.get('/callback-path-param/safe_param') assert.equal(res.status, 200) assert.equal(res.data, 'DONE') sinon.assert.calledOnce(paramCallbackSpy) @@ -168,7 +171,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected', async () => { try { - await axios.get(`http://localhost:${port}/callback-path-param/testattack`) + await axios.get('/callback-path-param/testattack') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -181,7 +184,7 @@ withVersions('express', 'express', version => { }) describe('Suspicious request blocking - query', () => { - let port, server, requestBody + let server, requestBody, axios before(() => { return agent.load(['express', 'http'], { client: false }) @@ -197,9 +200,11 @@ withVersions('express', 'express', version => { res.end('DONE') }) - server = app.listen(port, () => { - port = server.address().port - done() + getPort().then((port) => { + server = app.listen(port, () => { + axios = Axios.create({ baseURL: `http://localhost:${port}` }) + done() + }) }) }) @@ -223,7 +228,7 @@ withVersions('express', 'express', version => { }) it('should not block the request without an attack', async () => { - const res = await axios.get(`http://localhost:${port}/?key=value`) + const res = await axios.get('/?key=value') assert.equal(res.status, 200) assert.equal(res.data, 'DONE') @@ -232,7 +237,7 @@ withVersions('express', 'express', version => { it('should block the request when attack is detected', async () => { try { - await axios.get(`http://localhost:${port}/?key=testattack`) + await axios.get('/?key=testattack') return Promise.reject(new Error('Request should not return 200')) } catch (e) { @@ -244,7 +249,7 @@ withVersions('express', 'express', version => { }) describe('Api Security', () => { - let config, port, server + let config, server, axios before(() => { return agent.load(['express', 'http'], { client: false }) @@ -273,9 +278,11 @@ withVersions('express', 'express', version => { res.jsonp({ jsonResKey: 'jsonResValue' }) }) - server = app.listen(port, () => { - port = server.address().port - done() + getPort().then((port) => { + server = app.listen(port, () => { + axios = Axios.create({ baseURL: `http://localhost:${port}` }) + done() + }) }) }) @@ -313,7 +320,7 @@ withVersions('express', 'express', version => { it('should get the request body schema', async () => { const expectedRequestBodySchema = formatSchema([{ key: [8] }]) - const res = await axios.post(`http://localhost:${port}/`, { key: 'value' }) + const res = await axios.post('/', { key: 'value' }) await agent.use((traces) => { const span = traces[0][0] @@ -328,7 +335,7 @@ withVersions('express', 'express', version => { it('should get the response body schema with res.send method with object', async () => { const expectedResponseBodySchema = formatSchema([{ sendResKey: [8] }]) - const res = await axios.post(`http://localhost:${port}/sendjson`, { key: 'value' }) + const res = await axios.post('/sendjson', { key: 'value' }) await agent.use((traces) => { const span = traces[0][0] @@ -341,7 +348,7 @@ withVersions('express', 'express', version => { it('should get the response body schema with res.json method', async () => { const expectedResponseBodySchema = formatSchema([{ jsonResKey: [8] }]) - const res = await axios.post(`http://localhost:${port}/json`, { key: 'value' }) + const res = await axios.post('/json', { key: 'value' }) await agent.use((traces) => { const span = traces[0][0] @@ -354,7 +361,7 @@ withVersions('express', 'express', version => { it('should get the response body schema with res.jsonp method', async () => { const expectedResponseBodySchema = formatSchema([{ jsonpResKey: [8] }]) - const res = await axios.post(`http://localhost:${port}/jsonp`, { key: 'value' }) + const res = await axios.post('/jsonp', { key: 'value' }) await agent.use((traces) => { const span = traces[0][0] @@ -370,7 +377,7 @@ withVersions('express', 'express', version => { config.appsec.apiSecurity.requestSampling = 0 appsec.enable(config) - const res = await axios.post(`http://localhost:${port}/`, { key: 'value' }) + const res = await axios.post('/', { key: 'value' }) await agent.use((traces) => { const span = traces[0][0] From 522edcb29fcd94eb6aad05c8f777885c7648ffb6 Mon Sep 17 00:00:00 2001 From: Carles Capell <107924659+CarlesDD@users.noreply.github.com> Date: Tue, 15 Oct 2024 17:17:41 +0200 Subject: [PATCH 12/12] Update packages/datadog-instrumentations/src/express.js Co-authored-by: simon-id --- packages/datadog-instrumentations/src/express.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index 1d8645e9af1..b093eab7830 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -102,7 +102,7 @@ addHook({ }) const processParamsStartCh = channel('datadog:express:process_params:start') -const wrapProcessParamsMethod = (requestPositionInArguments) => { +function wrapProcessParamsMethod (requestPositionInArguments) { return function wrapProcessParams (original) { return function wrappedProcessParams () { if (processParamsStartCh.hasSubscribers) {