diff --git a/index.d.ts b/index.d.ts index 2c69ead50b..ded2f13d23 100644 --- a/index.d.ts +++ b/index.d.ts @@ -660,7 +660,7 @@ declare namespace tracer { */ apiSecurity?: { /** Whether to enable Api Security. - * @default false + * @default true */ enabled?: boolean, diff --git a/integration-tests/standalone-asm.spec.js b/integration-tests/standalone-asm.spec.js index d57a96f738..28ade4e1c1 100644 --- a/integration-tests/standalone-asm.spec.js +++ b/integration-tests/standalone-asm.spec.js @@ -103,7 +103,7 @@ describe('Standalone ASM', () => { assert.notProperty(meta, 'manual.keep') assert.notProperty(meta, '_dd.p.appsec') - assert.propertyVal(metrics, '_sampling_priority_v1', 1) + assert.propertyVal(metrics, '_sampling_priority_v1', 0) assert.propertyVal(metrics, '_dd.apm.enabled', 0) assertDrop(payload[2][0]) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index c47feef246..22571e7b82 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -29,7 +29,7 @@ function wrapResponseJson (json) { obj = arguments[1] } - responseJsonChannel.publish({ req: this.req, body: obj }) + responseJsonChannel.publish({ req: this.req, res: this, body: obj }) } return json.apply(this, arguments) diff --git a/packages/dd-trace/src/appsec/api_security_sampler.js b/packages/dd-trace/src/appsec/api_security_sampler.js index e8f1d834ed..db4efecbb6 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -1,23 +1,20 @@ 'use strict' -const crypto = require('node:crypto') const LRUCache = require('lru-cache') -const PrioritySampler = require('../priority_sampler') const web = require('../plugins/util/web') const log = require('../log') +const { AUTO_REJECT, USER_REJECT } = require('../../../../ext/priority') const MAX_SIZE = 4096 const DEFAULT_DELAY = 30 // 30s let enabled let sampledRequests -let prioritySampler function configure ({ apiSecurity }) { enabled = apiSecurity.enabled - const ttl = parseSampleDelay(apiSecurity.sampleDelay) * 1000 - sampledRequests = new LRUCache({ max: MAX_SIZE, ttl }) - prioritySampler = new PrioritySampler() + const delay = apiSecurity.sampleDelay || DEFAULT_DELAY + sampledRequests = new LRUCache({ max: MAX_SIZE, ttl: delay * 1000 }) } function disable () { @@ -25,48 +22,61 @@ function disable () { sampledRequests?.clear() } -function sampleRequest (req, res) { +function sampleRequest (req, res, force = false) { if (!enabled) return false + if (this.isSampled(req, res)) return false const rootSpan = web.root(req) if (!rootSpan) return false - const isSampled = prioritySampler.isSampled(rootSpan) + const priority = getSpanPriority(rootSpan) - if (!isSampled) { + if (priority === AUTO_REJECT || priority === USER_REJECT) { return false } - const key = computeKey(req, res) - const alreadySampled = sampledRequests.has(key) + if (!priority && !rootSpan._prioritySampler?.isSampled(rootSpan)) { + return false + } - if (alreadySampled) return false + if (force) { + return sample(req, res) + } - sampledRequests.set(key) + return true +} +function sample (req, res) { + const key = computeKey(req, res) + if (!key) return false + if (sampledRequests.has(key)) return false + + sampledRequests.set(key) return true } function isSampled (req, res) { const key = computeKey(req, res) - return !!sampledRequests.has(key) + if (!key) return false + + return sampledRequests.has(key) } function computeKey (req, res) { - const route = req.route.path - const method = req.method.toLowerCase() - const statusCode = res.statusCode - const str = route + statusCode + method - return crypto.createHash('md5').update(str).digest('hex') -} + const route = web.getContext(req).paths.join('') || '' + const method = req.method + const status = res.statusCode -function parseSampleDelay (delay) { - if (typeof delay === 'number' && Number.isFinite(delay) && delay > 0) { - return delay - } else { - log.warn('Invalid delay value. Delay must be a positive number.') - return DEFAULT_DELAY + if (!method || !status) { + log.warn('Unsupported groupkey for API security') + return null } + return method + route + status +} + +function getSpanPriority (span) { + const spanContext = span.context?.() + return spanContext._sampling?.priority } module.exports = { diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 8655b76728..3ea67feda7 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -132,7 +132,7 @@ function incomingHttpEndTranslator ({ req, res }) { persistent[addresses.HTTP_INCOMING_QUERY] = req.query } - if (req.route && typeof req.route.path === 'string' && apiSecuritySampler.sampleRequest(req, res)) { + if (apiSecuritySampler.sampleRequest(req, res, true)) { persistent[addresses.WAF_CONTEXT_PROCESSOR] = { 'extract-schema': true } } @@ -200,8 +200,9 @@ function onRequestCookieParser ({ req, res, abortController, cookies }) { handleResults(results, req, res, rootSpan, abortController) } -function onResponseBody ({ req, body }) { +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({ diff --git a/packages/dd-trace/test/appsec/api_security_sampler.spec.js b/packages/dd-trace/test/appsec/api_security_sampler.spec.js index 11e5bc6173..f8eefb25e9 100644 --- a/packages/dd-trace/test/appsec/api_security_sampler.spec.js +++ b/packages/dd-trace/test/appsec/api_security_sampler.spec.js @@ -2,34 +2,38 @@ const { performance } = require('node:perf_hooks') const proxyquire = require('proxyquire') +const { USER_KEEP, AUTO_KEEP, AUTO_REJECT, USER_REJECT } = require('../../../../ext/priority') describe('API Security Sampler', () => { const req = { route: { path: '/test' }, method: 'GET' } const res = { statusCode: 200 } - let apiSecuritySampler, performanceNowStub, webStub, sampler, span + let apiSecuritySampler, performanceNowStub, webStub, span beforeEach(() => { performanceNowStub = sinon.stub(performance, 'now').returns(0) - webStub = { root: sinon.stub() } - - sampler = sinon.stub().returns({ - isSampled: sinon.stub() - }) + webStub = { + root: sinon.stub(), + getContext: sinon.stub(), + _prioritySampler: { + isSampled: sinon.stub() + } + } apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { - '../plugins/util/web': webStub, - '../priority_sampler': sampler + '../plugins/util/web': webStub }) apiSecuritySampler.configure({ apiSecurity: { enabled: true } }) span = { - context: sinon.stub().returns({}) + context: sinon.stub().returns({ + _sampling: { priority: AUTO_KEEP } + }) } webStub.root.returns(span) - sampler().isSampled.returns(true) + webStub.getContext.returns({ paths: ['path'] }) performanceNowStub.returns(performance.now() + 1) }) @@ -49,64 +53,67 @@ describe('API Security Sampler', () => { expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) - it('should return true and put request in cache if priority is AUTO_KEEP or USER_KEEP', () => { + it('should return false for AUTO_REJECT priority', () => { + span.context.returns({ _sampling: { priority: AUTO_REJECT } }) + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.false + }) + + it('should return false for USER_REJECT priority', () => { + span.context.returns({ _sampling: { priority: USER_REJECT } }) + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.false + }) + + it('should sample for AUTO_KEEP priority without checking prioritySampler', () => { + span.context.returns({ _sampling: { priority: AUTO_KEEP } }) expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true }) - it('should not sample before 30 seconds', () => { + it('should sample for USER_KEEP priority without checking prioritySampler', () => { + span.context.returns({ _sampling: { priority: USER_KEEP } }) expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + }) + + it('should not sample before 30 seconds', () => { + expect(apiSecuritySampler.sampleRequest(req, res, true)).to.be.true performanceNowStub.returns(performance.now() + 25000) - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.false + expect(apiSecuritySampler.sampleRequest(req, res, true)).to.be.false expect(apiSecuritySampler.isSampled(req, res)).to.be.true }) it('should sample after 30 seconds', () => { - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + expect(apiSecuritySampler.sampleRequest(req, res, true)).to.be.true performanceNowStub.returns(performance.now() + 35000) - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true - }) - - it('should return false if priority is neither AUTO_KEEP nor USER_KEEP', () => { - sampler().isSampled.returns(false) - expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false + expect(apiSecuritySampler.sampleRequest(req, res, true)).to.be.true }) it('should remove oldest entry when max size is exceeded', () => { - const method = req.method for (let i = 0; i < 4097; i++) { - expect(apiSecuritySampler.sampleRequest({ method, route: { path: `/test${i}` } }, res)).to.be.true + const path = `/test${i}` + webStub.getContext.returns({ paths: [path] }) + expect(apiSecuritySampler.sampleRequest(req, res, true)).to.be.true } - expect(apiSecuritySampler.isSampled({ method, route: { path: '/test0' } }, res)).to.be.false - expect(apiSecuritySampler.isSampled({ method, route: { path: '/test4096' } }, res)).to.be.true + webStub.getContext.returns({ paths: ['/test0'] }) + expect(apiSecuritySampler.isSampled(req, res)).to.be.false + webStub.getContext.returns({ paths: ['/test4096'] }) + expect(apiSecuritySampler.isSampled(req, res)).to.be.true }) it('should set enabled to false and clear the cache', () => { - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + expect(apiSecuritySampler.sampleRequest(req, res, true)).to.be.true apiSecuritySampler.disable() - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.false - }) - - it('should create different keys for different URLs', () => { - const req1 = { route: { path: '/test1' }, method: 'GET' } - const req2 = { route: { path: '/test2' }, method: 'GET' } - - expect(apiSecuritySampler.sampleRequest(req1, res)).to.be.true - expect(apiSecuritySampler.sampleRequest(req2, res)).to.be.true - expect(apiSecuritySampler.isSampled(req1, res)).to.be.true - expect(apiSecuritySampler.isSampled(req2, res)).to.be.true + expect(apiSecuritySampler.sampleRequest(req, res, true)).to.be.false }) it('should create different keys for different methods', () => { - const getReq = { route: { path: '/test1' }, method: 'GET' } - const postReq = { route: { path: '/test1' }, method: 'POST' } - - expect(apiSecuritySampler.sampleRequest(getReq, res)).to.be.true - expect(apiSecuritySampler.sampleRequest(postReq, res)).to.be.true + const getReq = { method: 'GET' } + const postReq = { method: 'POST' } + expect(apiSecuritySampler.sampleRequest(getReq, res, true)).to.be.true + expect(apiSecuritySampler.sampleRequest(postReq, res, true)).to.be.true expect(apiSecuritySampler.isSampled(getReq, res)).to.be.true expect(apiSecuritySampler.isSampled(postReq, res)).to.be.true }) @@ -115,9 +122,14 @@ describe('API Security Sampler', () => { const res200 = { statusCode: 200 } const res404 = { statusCode: 404 } - expect(apiSecuritySampler.sampleRequest(req, res200)).to.be.true - expect(apiSecuritySampler.sampleRequest(req, res404)).to.be.true + expect(apiSecuritySampler.sampleRequest(req, res200, true)).to.be.true + expect(apiSecuritySampler.sampleRequest(req, res404, true)).to.be.true expect(apiSecuritySampler.isSampled(req, res200)).to.be.true expect(apiSecuritySampler.isSampled(req, res404)).to.be.true }) + + it('should not sample when method or statusCode is not available', () => { + expect(apiSecuritySampler.sampleRequest(req, {}, true)).to.be.false + expect(apiSecuritySampler.sampleRequest({}, res, true)).to.be.false + }) }) 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 886c20d97d..64fe35368d 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -41,7 +41,7 @@ withVersions('express', 'express', version => { }) app.post('/json', (req, res) => { - res.jsonp({ jsonResKey: 'jsonResValue' }) + res.json({ jsonResKey: 'jsonResValue' }) }) server = app.listen(port, () => { diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 40fb41079a..d39059b060 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -46,7 +46,6 @@ describe('AppSec Index', function () { let graphql let apiSecuritySampler let rasp - let sampler const RULES = { rules: [{ a: 1 }] } @@ -76,13 +75,13 @@ describe('AppSec Index', function () { } web = { - root: sinon.stub() + root: sinon.stub(), + getContext: sinon.stub(), + _prioritySampler: { + isSampled: sinon.stub() + } } - sampler = sinon.stub().returns({ - isSampled: sinon.stub() - }) - blocking = { setTemplates: sinon.stub() } @@ -108,8 +107,7 @@ describe('AppSec Index', function () { } apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { - '../plugins/util/web': web, - '../priority_sampler': sampler + '../plugins/util/web': web }) sinon.spy(apiSecuritySampler, 'sampleRequest') @@ -471,6 +469,7 @@ describe('AppSec Index', function () { } web.root.returns(rootSpan) + web.getContext.returns({ paths: ['path'] }) }) it('should not trigger schema extraction with feature disabled', () => { @@ -558,11 +557,15 @@ describe('AppSec Index', function () { } const span = { - context: sinon.stub().returns({}) + context: sinon.stub().returns({ + _sampling: { + priority: 1 + } + }) } web.root.returns(span) - sampler().isSampled.returns(true) + web._prioritySampler.isSampled.returns(true) AppSec.incomingHttpEndTranslator({ req, res }) @@ -580,6 +583,7 @@ describe('AppSec Index', function () { enabled: true, sampleDelay: 1 } + AppSec.enable(config) }) @@ -591,15 +595,30 @@ describe('AppSec Index', function () { responseBody.publish({ req: {}, body: 'string' }) responseBody.publish({ req: {}, body: null }) + expect(apiSecuritySampler.sampleRequest).to.not.been.called + expect(waf.run).to.not.been.called + }) + + it('should not call to the waf if it is not a sampled request', () => { + apiSecuritySampler.sampleRequest = apiSecuritySampler.sampleRequest.instantiateFake(() => false) + const req = {} + const res = {} + + responseBody.publish({ req, res, body: {} }) + + expect(apiSecuritySampler.sampleRequest).to.have.been.calledOnceWith(req, res) expect(waf.run).to.not.been.called }) - it('should call to the waf if body is an object', () => { + it('should call to the waf if it is a sampled request', () => { + apiSecuritySampler.sampleRequest = apiSecuritySampler.sampleRequest.instantiateFake(() => true) const req = {} + const res = {} const body = {} - responseBody.publish({ req, body }) + responseBody.publish({ req, res, body }) + expect(apiSecuritySampler.sampleRequest).to.have.been.calledOnceWith(req, res) expect(waf.run).to.been.calledOnceWith({ persistent: { [addresses.HTTP_INCOMING_RESPONSE_BODY]: body