From 24089317a7b4be19de12275867a51ec5718ce3dd Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 3 Oct 2024 09:33:45 +0200 Subject: [PATCH 1/7] add support to api security sampling --- docs/test.ts | 1 - index.d.ts | 7 +- .../src/appsec/api_security_sampler.js | 54 +++----- .../src/appsec/api_security_sampler_cache.js | 53 +++++++ packages/dd-trace/src/appsec/index.js | 9 +- .../src/appsec/remote_config/capabilities.js | 1 - .../src/appsec/remote_config/index.js | 7 - packages/dd-trace/src/config.js | 7 +- .../test/appsec/api_security_sampler.spec.js | 92 +++++++------ .../appsec/api_security_sampler_cache.spec.js | 65 +++++++++ .../appsec/index.sequelize.plugin.spec.js | 2 +- packages/dd-trace/test/appsec/index.spec.js | 116 +++++++--------- .../test/appsec/remote_config/index.spec.js | 129 ------------------ .../test/appsec/response_blocking.spec.js | 5 +- packages/dd-trace/test/config.spec.js | 50 ++----- 15 files changed, 257 insertions(+), 341 deletions(-) create mode 100644 packages/dd-trace/src/appsec/api_security_sampler_cache.js create mode 100644 packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js diff --git a/docs/test.ts b/docs/test.ts index e948e4ff4dd..e6d28d04db0 100644 --- a/docs/test.ts +++ b/docs/test.ts @@ -115,7 +115,6 @@ tracer.init({ }, apiSecurity: { enabled: true, - requestSampling: 1.0 }, rasp: { enabled: true diff --git a/index.d.ts b/index.d.ts index bc17ef2dad5..a5de0c987e5 100644 --- a/index.d.ts +++ b/index.d.ts @@ -656,7 +656,7 @@ declare namespace tracer { mode?: 'safe' | 'extended' | 'disabled' }, /** - * Configuration for Api Security sampling + * Configuration for Api Security */ apiSecurity?: { /** Whether to enable Api Security. @@ -664,11 +664,6 @@ declare namespace tracer { */ enabled?: boolean, - /** Controls the request sampling rate (between 0 and 1) in which Api Security is triggered. - * The value will be coerced back if it's outside of the 0-1 range. - * @default 0.1 - */ - requestSampling?: number }, /** * Configuration for RASP diff --git a/packages/dd-trace/src/appsec/api_security_sampler.js b/packages/dd-trace/src/appsec/api_security_sampler.js index 68bd896af7e..6baaed4f87e 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -1,61 +1,51 @@ 'use strict' -const log = require('../log') +const ApiSecuritySamplerCache = require('./api_security_sampler_cache') +const web = require('../plugins/util/web') +const { USER_KEEP, AUTO_KEEP } = require('../../../../ext/priority') let enabled -let requestSampling - -const sampledRequests = new WeakSet() +let sampledRequests function configure ({ apiSecurity }) { enabled = apiSecurity.enabled - setRequestSampling(apiSecurity.requestSampling) + sampledRequests = new ApiSecuritySamplerCache(apiSecurity.sampleDelay) } function disable () { enabled = false + sampledRequests?.clear() } -function setRequestSampling (sampling) { - requestSampling = parseRequestSampling(sampling) -} - -function parseRequestSampling (requestSampling) { - let parsed = parseFloat(requestSampling) - - if (isNaN(parsed)) { - log.warn(`Incorrect API Security request sampling value: ${requestSampling}`) +function sampleRequest (req, res) { + if (!enabled) return false - parsed = 0 - } else { - parsed = Math.min(1, Math.max(0, parsed)) - } + const rootSpan = web.root(req) + if (!rootSpan) return false - return parsed -} + const priority = getSpanPriority(rootSpan) -function sampleRequest (req) { - if (!enabled || !requestSampling) { + if (priority !== AUTO_KEEP && priority !== USER_KEEP) { return false } - const shouldSample = Math.random() <= requestSampling + const key = sampledRequests.computeKey(req, res) + const isSampled = sampledRequests.isSampled(key) - if (shouldSample) { - sampledRequests.add(req) - } + if (isSampled) return false + + sampledRequests.set(key) - return shouldSample + return true } -function isSampled (req) { - return sampledRequests.has(req) +function getSpanPriority (span) { + const spanContext = span.context?.() + return spanContext._sampling?.priority // default ?? } module.exports = { configure, disable, - setRequestSampling, - sampleRequest, - isSampled + sampleRequest } diff --git a/packages/dd-trace/src/appsec/api_security_sampler_cache.js b/packages/dd-trace/src/appsec/api_security_sampler_cache.js new file mode 100644 index 00000000000..4c328c43428 --- /dev/null +++ b/packages/dd-trace/src/appsec/api_security_sampler_cache.js @@ -0,0 +1,53 @@ +'use strict' + +const crypto = require('node:crypto') +const log = require('../log') + +const MAX_SIZE = 4096 +const DEFAULT_DELAY = 30 // 30s + +class ApiSecuritySamplerCache extends Map { + constructor (delay) { + super() + this.delay = this._parseSampleDelay(delay) + } + + _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 + } + } + + computeKey (req, res) { + const route = req.url + const method = req.method.toLowerCase() + const statusCode = res.statusCode + const str = route + statusCode + method + return crypto.createHash('md5').update(str).digest('hex') + } + + isSampled (key) { + if (!super.has(key)) { + return false + } + const previous = super.get(key) + return Date.now() - previous < (this.delay * 1000) + } + + set (key) { + if (super.has(key)) { + super.delete(key) + } + + super.set(key, Date.now()) + if (super.size > MAX_SIZE) { + const oldestKey = super.keys().next().value + super.delete(oldestKey) + } + } +} + +module.exports = ApiSecuritySamplerCache diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 10e63ebd2de..56b645dbcbc 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -104,10 +104,6 @@ function incomingHttpStartTranslator ({ req, res, abortController }) { persistent[addresses.HTTP_CLIENT_IP] = clientIp } - if (apiSecuritySampler.sampleRequest(req)) { - persistent[addresses.WAF_CONTEXT_PROCESSOR] = { 'extract-schema': true } - } - const actions = waf.run({ persistent }, req) handleResults(actions, req, res, rootSpan, abortController) @@ -136,6 +132,10 @@ function incomingHttpEndTranslator ({ req, res }) { persistent[addresses.HTTP_INCOMING_QUERY] = req.query } + if (apiSecuritySampler.sampleRequest(req, res)) { + persistent[addresses.WAF_CONTEXT_PROCESSOR] = { 'extract-schema': true } + } + if (Object.keys(persistent).length) { waf.run({ persistent }, req) } @@ -202,7 +202,6 @@ function onRequestCookieParser ({ req, res, abortController, cookies }) { function onResponseBody ({ req, body }) { if (!body || typeof body !== 'object') return - if (!apiSecuritySampler.isSampled(req)) return // we don't support blocking at this point, so no results needed waf.run({ diff --git a/packages/dd-trace/src/appsec/remote_config/capabilities.js b/packages/dd-trace/src/appsec/remote_config/capabilities.js index f42d7358203..4c2fb8f8ff8 100644 --- a/packages/dd-trace/src/appsec/remote_config/capabilities.js +++ b/packages/dd-trace/src/appsec/remote_config/capabilities.js @@ -11,7 +11,6 @@ module.exports = { ASM_CUSTOM_RULES: 1n << 8n, ASM_CUSTOM_BLOCKING_RESPONSE: 1n << 9n, ASM_TRUSTED_IPS: 1n << 10n, - ASM_API_SECURITY_SAMPLE_RATE: 1n << 11n, APM_TRACING_SAMPLE_RATE: 1n << 12n, APM_TRACING_LOGS_INJECTION: 1n << 13n, APM_TRACING_HTTP_HEADER_TAGS: 1n << 14n, diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index b63b3690102..64007da69df 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -4,7 +4,6 @@ const Activation = require('../activation') const RemoteConfigManager = require('./manager') const RemoteConfigCapabilities = require('./capabilities') -const apiSecuritySampler = require('../api_security_sampler') let rc @@ -24,18 +23,12 @@ function enable (config, appsec) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_ACTIVATION, true) } - if (config.appsec.apiSecurity?.enabled) { - rc.updateCapabilities(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) - } - rc.setProductHandler('ASM_FEATURES', (action, rcConfig) => { if (!rcConfig) return if (activation === Activation.ONECLICK) { enableOrDisableAppsec(action, rcConfig, config, appsec) } - - apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) }) } diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index dc5bb524d1a..05e69568cb6 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -444,7 +444,7 @@ class Config { const defaults = setHiddenProperty(this, '_defaults', {}) this._setValue(defaults, 'appsec.apiSecurity.enabled', true) - this._setValue(defaults, 'appsec.apiSecurity.requestSampling', 0.1) + this._setValue(defaults, 'appsec.apiSecurity.sampleDelay', 30) this._setValue(defaults, 'appsec.blockedTemplateGraphql', undefined) this._setValue(defaults, 'appsec.blockedTemplateHtml', undefined) this._setValue(defaults, 'appsec.blockedTemplateJson', undefined) @@ -555,7 +555,7 @@ class Config { AWS_LAMBDA_FUNCTION_NAME, DD_AGENT_HOST, DD_API_SECURITY_ENABLED, - DD_API_SECURITY_REQUEST_SAMPLE_RATE, + DD_API_SECURITY_SAMPLE_DELAY, DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, DD_APPSEC_ENABLED, DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON, @@ -671,7 +671,7 @@ class Config { DD_API_SECURITY_ENABLED && isTrue(DD_API_SECURITY_ENABLED), DD_EXPERIMENTAL_API_SECURITY_ENABLED && isTrue(DD_EXPERIMENTAL_API_SECURITY_ENABLED) )) - this._setUnit(env, 'appsec.apiSecurity.requestSampling', DD_API_SECURITY_REQUEST_SAMPLE_RATE) + this._setValue(env, 'appsec.apiSecurity.sampleDelay', maybeFloat(DD_API_SECURITY_SAMPLE_DELAY)) this._setValue(env, 'appsec.blockedTemplateGraphql', maybeFile(DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON)) this._setValue(env, 'appsec.blockedTemplateHtml', maybeFile(DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML)) this._envUnprocessed['appsec.blockedTemplateHtml'] = DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML @@ -838,7 +838,6 @@ class Config { tagger.add(tags, options.tags) this._setBoolean(opts, 'appsec.apiSecurity.enabled', options.appsec.apiSecurity?.enabled) - this._setUnit(opts, 'appsec.apiSecurity.requestSampling', options.appsec.apiSecurity?.requestSampling) this._setValue(opts, 'appsec.blockedTemplateGraphql', maybeFile(options.appsec.blockedTemplateGraphql)) this._setValue(opts, 'appsec.blockedTemplateHtml', maybeFile(options.appsec.blockedTemplateHtml)) this._optsUnprocessed['appsec.blockedTemplateHtml'] = options.appsec.blockedTemplateHtml 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 5a69af05a5c..08b4355084a 100644 --- a/packages/dd-trace/test/appsec/api_security_sampler.spec.js +++ b/packages/dd-trace/test/appsec/api_security_sampler.spec.js @@ -1,71 +1,75 @@ 'use strict' -const apiSecuritySampler = require('../../src/appsec/api_security_sampler') +const proxyquire = require('proxyquire') -describe('Api Security Sampler', () => { - let config +describe('API Security Sampler', () => { + let apiSecuritySampler, webStub, clock beforeEach(() => { - config = { - apiSecurity: { - enabled: true, - requestSampling: 1 - } - } - - sinon.stub(Math, 'random').returns(0.3) + webStub = { root: sinon.stub() } + clock = sinon.useFakeTimers(Date.now()) + + apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { + '../plugins/util/web': webStub + }) }) - afterEach(sinon.restore) + afterEach(() => { + clock.restore() + }) describe('sampleRequest', () => { - it('should sample request if enabled and sampling 1', () => { - apiSecuritySampler.configure(config) - - expect(apiSecuritySampler.sampleRequest({})).to.true + beforeEach(() => { + apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) }) - it('should not sample request if enabled and sampling 0', () => { - config.apiSecurity.requestSampling = 0 - apiSecuritySampler.configure(config) - - expect(apiSecuritySampler.sampleRequest({})).to.false + it('should return false if not enabled', () => { + apiSecuritySampler.disable() + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) - it('should sample request if enabled and sampling greater than random', () => { - config.apiSecurity.requestSampling = 0.5 - - apiSecuritySampler.configure(config) - - expect(apiSecuritySampler.sampleRequest({})).to.true + it('should return false if no root span', () => { + webStub.root.returns(null) + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) - it('should not sample request if enabled and sampling less than random', () => { - config.apiSecurity.requestSampling = 0.1 - - apiSecuritySampler.configure(config) - - expect(apiSecuritySampler.sampleRequest()).to.false + it('should return true and put request in cache if priority is AUTO_KEEP', () => { + const rootSpan = { context: () => ({ _sampling: { priority: 2 } }) } + webStub.root.returns(rootSpan) + const req = { url: '/test', method: 'GET' } + const res = { statusCode: 200 } + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true }) - it('should not sample request if incorrect config value', () => { - config.apiSecurity.requestSampling = NaN - - apiSecuritySampler.configure(config) + it('should return true and put request in cache if priority is USER_KEEP', () => { + const rootSpan = { context: () => ({ _sampling: { priority: 1 } }) } + webStub.root.returns(rootSpan) + const req = { url: '/test', method: 'GET' } + const res = { statusCode: 200 } + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + }) - expect(apiSecuritySampler.sampleRequest()).to.false + it('should return false if priority is neither AUTO_KEEP nor USER_KEEP', () => { + const rootSpan = { context: () => ({ _sampling: { priority: 0 } }) } + webStub.root.returns(rootSpan) + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) + }) - it('should sample request according to the config', () => { - config.apiSecurity.requestSampling = 1 + describe('disable', () => { + it('should set enabled to false and clear the cache', () => { + const req = { url: '/test', method: 'GET' } + const res = { statusCode: 200 } - apiSecuritySampler.configure(config) + const rootSpan = { context: () => ({ _sampling: { priority: 2 } }) } + webStub.root.returns(rootSpan) - expect(apiSecuritySampler.sampleRequest({})).to.true + apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true - apiSecuritySampler.setRequestSampling(0) + apiSecuritySampler.disable() - expect(apiSecuritySampler.sampleRequest()).to.false + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) }) }) diff --git a/packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js b/packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js new file mode 100644 index 00000000000..868705cd458 --- /dev/null +++ b/packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js @@ -0,0 +1,65 @@ +'use strict' + +const ApiSecuritySamplerCache = require('../../src/appsec/api_security_sampler_cache') + +describe('ApiSecuritySamplerCache', () => { + let cache + let clock + + beforeEach(() => { + cache = new ApiSecuritySamplerCache() + clock = sinon.useFakeTimers(Date.now()) + }) + + afterEach(() => { + clock.restore() + }) + + describe('Standard behavior with default delay', () => { + it('should not sample when first seen', () => { + const req = { url: '/test', method: 'GET' } + const res = { status_code: 200 } + const key = cache.computeKey(req, res) + expect(cache.isSampled(key)).to.be.false + }) + + it('should sample within 30 seconds of first seen', () => { + const req = { url: '/test', method: 'GET' } + const res = { status_code: 200 } + const key = cache.computeKey(req, res) + cache.set(key) + clock.tick(29000) + expect(cache.isSampled(key)).to.be.true + }) + + it('should not sample after 30 seconds', () => { + const req = { url: '/test', method: 'GET' } + const res = { status_code: 200 } + const key = cache.computeKey(req, res) + cache.set(key) + clock.tick(31000) + expect(cache.isSampled(key)).to.be.false + }) + }) + + describe('Max size behavior', () => { + it('should remove oldest entry when max size is exceeded', () => { + const baseReq = { method: 'GET' } + const baseRes = { status_code: 200 } + + for (let i = 0; i < 4097; i++) { + const req = { ...baseReq, url: `test${i}` } + const key = cache.computeKey(req, baseRes) + cache.set(key) + } + + expect(cache.size).to.equal(4096) + + const firstKey = cache.computeKey({ ...baseReq, url: 'test0' }, baseRes) + expect(cache.isSampled(firstKey)).to.be.false + + const lastKey = cache.computeKey({ ...baseReq, url: 'test4096' }, baseRes) + expect(cache.isSampled(lastKey)).to.be.true + }) + }) +}) diff --git a/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js b/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js index d444b82ec5e..49442e361b2 100644 --- a/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js @@ -21,7 +21,7 @@ describe('sequelize', () => { rules: path.join(__dirname, 'express-rules.json'), apiSecurity: { enabled: true, - requestSampling: 1 + sampleDelay: 10 } } })) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index b8a41d840b5..c83852750fc 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -66,7 +66,7 @@ describe('AppSec Index', function () { }, apiSecurity: { enabled: false, - requestSampling: 0 + sampleDelay: 10 }, rasp: { enabled: true @@ -102,9 +102,10 @@ describe('AppSec Index', function () { disable: sinon.stub() } - apiSecuritySampler = require('../../src/appsec/api_security_sampler') + apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { + '../plugins/util/web': web + }) sinon.spy(apiSecuritySampler, 'sampleRequest') - sinon.spy(apiSecuritySampler, 'isSampled') rasp = { enable: sinon.stub(), @@ -466,45 +467,10 @@ describe('AppSec Index', function () { web.root.returns(rootSpan) }) - it('should not trigger schema extraction with sampling disabled', () => { - config.appsec.apiSecurity = { - enabled: true, - requestSampling: 0 - } - - AppSec.enable(config) - - const req = { - url: '/path', - headers: { - 'user-agent': 'Arachni', - host: 'localhost', - cookie: 'a=1;b=2' - }, - method: 'POST', - socket: { - remoteAddress: '127.0.0.1', - remotePort: 8080 - } - } - const res = {} - - AppSec.incomingHttpStartTranslator({ req, res }) - - expect(waf.run).to.have.been.calledOnceWithExactly({ - persistent: { - 'server.request.uri.raw': '/path', - 'server.request.headers.no_cookies': { 'user-agent': 'Arachni', host: 'localhost' }, - 'server.request.method': 'POST', - 'http.client_ip': '127.0.0.1' - } - }, req) - }) - it('should not trigger schema extraction with feature disabled', () => { config.appsec.apiSecurity = { enabled: false, - requestSampling: 1 + sampleDelay: 1 } AppSec.enable(config) @@ -520,18 +486,34 @@ describe('AppSec Index', function () { socket: { remoteAddress: '127.0.0.1', remotePort: 8080 + }, + body: { + a: '1' + }, + query: { + b: '2' + }, + route: { + path: '/path/:c' } } - const res = {} + const res = { + getHeaders: () => ({ + 'content-type': 'application/json', + 'content-lenght': 42 + }), + statusCode: 201 + } - AppSec.incomingHttpStartTranslator({ req, res }) + web.patch(req) + + sinon.stub(Reporter, 'finishRequest') + AppSec.incomingHttpEndTranslator({ req, res }) expect(waf.run).to.have.been.calledOnceWithExactly({ persistent: { - 'server.request.uri.raw': '/path', - 'server.request.headers.no_cookies': { 'user-agent': 'Arachni', host: 'localhost' }, - 'server.request.method': 'POST', - 'http.client_ip': '127.0.0.1' + 'server.request.body': { a: '1' }, + 'server.request.query': { b: '2' } } }, req) }) @@ -539,7 +521,7 @@ describe('AppSec Index', function () { it('should trigger schema extraction with sampling enabled', () => { config.appsec.apiSecurity = { enabled: true, - requestSampling: 1 + sampleDelay: 1 } AppSec.enable(config) @@ -548,25 +530,34 @@ describe('AppSec Index', function () { url: '/path', headers: { 'user-agent': 'Arachni', - host: 'localhost', - cookie: 'a=1;b=2' + host: 'localhost' }, method: 'POST', socket: { remoteAddress: '127.0.0.1', remotePort: 8080 + }, + body: { + a: '1' } } - const res = {} + const res = { + getHeaders: () => ({ + 'content-type': 'application/json', + 'content-lenght': 42 + }), + statusCode: 201 + } - AppSec.incomingHttpStartTranslator({ req, res }) + const rootSpan = { context: () => ({ _sampling: { priority: 2 } }) } + + web.root.returns(rootSpan) + + AppSec.incomingHttpEndTranslator({ req, res }) expect(waf.run).to.have.been.calledOnceWithExactly({ persistent: { - 'server.request.uri.raw': '/path', - 'server.request.headers.no_cookies': { 'user-agent': 'Arachni', host: 'localhost' }, - 'server.request.method': 'POST', - 'http.client_ip': '127.0.0.1', + 'server.request.body': { a: '1' }, 'waf.context.processor': { 'extract-schema': true } } }, req) @@ -576,7 +567,7 @@ describe('AppSec Index', function () { beforeEach(() => { config.appsec.apiSecurity = { enabled: true, - requestSampling: 1 + sampleDelay: 1 } AppSec.enable(config) }) @@ -589,28 +580,15 @@ describe('AppSec Index', function () { responseBody.publish({ req: {}, body: 'string' }) responseBody.publish({ req: {}, body: null }) - expect(apiSecuritySampler.isSampled).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.isSampled = apiSecuritySampler.isSampled.instantiateFake(() => false) - const req = {} - - responseBody.publish({ req, body: {} }) - - expect(apiSecuritySampler.isSampled).to.have.been.calledOnceWith(req) expect(waf.run).to.not.been.called }) - it('should call to the waf if it is a sampled request', () => { - apiSecuritySampler.isSampled = apiSecuritySampler.isSampled.instantiateFake(() => true) + it('should call to the waf if body is an object', () => { const req = {} const body = {} responseBody.publish({ req, body }) - expect(apiSecuritySampler.isSampled).to.have.been.calledOnceWith(req) expect(waf.run).to.been.calledOnceWith({ persistent: { [addresses.HTTP_INCOMING_RESPONSE_BODY]: body diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index fd923c9a92b..e3fe644687f 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -9,7 +9,6 @@ let RemoteConfigManager let RuleManager let appsec let remoteConfig -let apiSecuritySampler describe('Remote Config index', () => { beforeEach(() => { @@ -33,11 +32,6 @@ describe('Remote Config index', () => { updateWafFromRC: sinon.stub() } - apiSecuritySampler = { - configure: sinon.stub(), - setRequestSampling: sinon.stub() - } - appsec = { enable: sinon.spy(), disable: sinon.spy() @@ -46,7 +40,6 @@ describe('Remote Config index', () => { remoteConfig = proxyquire('../src/appsec/remote_config', { './manager': RemoteConfigManager, '../rule_manager': RuleManager, - '../api_security_sampler': apiSecuritySampler, '..': appsec }) }) @@ -84,28 +77,6 @@ describe('Remote Config index', () => { expect(rc.setProductHandler).to.not.have.been.called }) - it('should listen ASM_API_SECURITY_SAMPLE_RATE when appsec.enabled=undefined and appSecurity.enabled=true', () => { - config.appsec = { enabled: undefined, apiSecurity: { enabled: true } } - - remoteConfig.enable(config) - - expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) - expect(rc.updateCapabilities) - .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_ACTIVATION, true) - expect(rc.updateCapabilities) - .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) - }) - - it('should listen ASM_API_SECURITY_SAMPLE_RATE when appsec.enabled=true and appSecurity.enabled=true', () => { - config.appsec = { enabled: true, apiSecurity: { enabled: true } } - - remoteConfig.enable(config) - - expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) - expect(rc.updateCapabilities) - .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) - }) - describe('ASM_FEATURES remote config listener', () => { let listener @@ -142,106 +113,6 @@ describe('Remote Config index', () => { expect(appsec.disable).to.not.have.been.called }) }) - - describe('API Security Request Sampling', () => { - describe('OneClick', () => { - let listener - - beforeEach(() => { - config = { - appsec: { - enabled: undefined, - apiSecurity: { - requestSampling: 0.1 - } - } - } - - remoteConfig.enable(config) - - listener = rc.setProductHandler.firstCall.args[1] - }) - - it('should update apiSecuritySampler config', () => { - listener('apply', { - api_security: { - request_sample_rate: 0.5 - } - }) - - expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0.5) - }) - - it('should update apiSecuritySampler config and disable it', () => { - listener('apply', { - api_security: { - request_sample_rate: 0 - } - }) - - expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0) - }) - - it('should not update apiSecuritySampler config with values greater than 1', () => { - listener('apply', { - api_security: { - request_sample_rate: 5 - } - }) - - expect(apiSecuritySampler.configure).to.not.be.called - }) - - it('should not update apiSecuritySampler config with values less than 0', () => { - listener('apply', { - api_security: { - request_sample_rate: -0.4 - } - }) - - expect(apiSecuritySampler.configure).to.not.be.called - }) - - it('should not update apiSecuritySampler config with incorrect values', () => { - listener('apply', { - api_security: { - request_sample_rate: 'not_a_number' - } - }) - - expect(apiSecuritySampler.configure).to.not.be.called - }) - }) - - describe('Enabled', () => { - let listener - - beforeEach(() => { - config = { - appsec: { - enabled: true, - apiSecurity: { - requestSampling: 0.1 - } - } - } - - remoteConfig.enable(config) - - listener = rc.setProductHandler.firstCall.args[1] - }) - - it('should update config apiSecurity.requestSampling property value', () => { - listener('apply', { - api_security: { - request_sample_rate: 0.5 - } - }) - - expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0.5) - }) - }) - }) }) describe('enableWafUpdate', () => { diff --git a/packages/dd-trace/test/appsec/response_blocking.spec.js b/packages/dd-trace/test/appsec/response_blocking.spec.js index 2868a42b05b..fc003e4cf27 100644 --- a/packages/dd-trace/test/appsec/response_blocking.spec.js +++ b/packages/dd-trace/test/appsec/response_blocking.spec.js @@ -52,7 +52,10 @@ describe('HTTP Response Blocking', () => { appsec.enable(new Config({ appsec: { enabled: true, - rules: path.join(__dirname, 'response_blocking_rules.json') + rules: path.join(__dirname, 'response_blocking_rules.json'), + apiSecurity: { + enabled: false + } } })) }) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index ca4d8b142d3..8648d1bfedd 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -248,7 +248,7 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) expect(config).to.have.nested.property('appsec.eventTracking.mode', 'safe') expect(config).to.have.nested.property('appsec.apiSecurity.enabled', true) - expect(config).to.have.nested.property('appsec.apiSecurity.requestSampling', 0.1) + expect(config).to.have.nested.property('appsec.apiSecurity.sampleDelay', 30) expect(config).to.have.nested.property('appsec.sca.enabled', null) expect(config).to.have.nested.property('appsec.standalone.enabled', undefined) expect(config).to.have.nested.property('remoteConfig.enabled', true) @@ -485,7 +485,7 @@ describe('Config', () => { process.env.DD_PROFILING_ENABLED = 'true' process.env.DD_INJECTION_ENABLED = 'profiler' process.env.DD_API_SECURITY_ENABLED = 'true' - process.env.DD_API_SECURITY_REQUEST_SAMPLE_RATE = 1 + process.env.DD_API_SECURITY_SAMPLE_DELAY = '25' process.env.DD_INSTRUMENTATION_INSTALL_ID = '68e75c48-57ca-4a12-adfc-575c4b05fcbe' process.env.DD_INSTRUMENTATION_INSTALL_TYPE = 'k8s_single_step' process.env.DD_INSTRUMENTATION_INSTALL_TIME = '1703188212' @@ -565,7 +565,7 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) expect(config).to.have.nested.property('appsec.eventTracking.mode', 'extended') expect(config).to.have.nested.property('appsec.apiSecurity.enabled', true) - expect(config).to.have.nested.property('appsec.apiSecurity.requestSampling', 1) + expect(config).to.have.nested.property('appsec.apiSecurity.sampleDelay', 25) expect(config).to.have.nested.property('appsec.sca.enabled', true) expect(config).to.have.nested.property('appsec.standalone.enabled', true) expect(config).to.have.nested.property('remoteConfig.enabled', false) @@ -1078,7 +1078,7 @@ describe('Config', () => { process.env.DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON = BLOCKED_TEMPLATE_JSON_PATH // json and html here process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = 'disabled' process.env.DD_API_SECURITY_ENABLED = 'false' - process.env.DD_API_SECURITY_REQUEST_SAMPLE_RATE = 0.5 + process.env.DD_API_SECURITY_SAMPLE_DELAY = '10' process.env.DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS = 11 process.env.DD_IAST_ENABLED = 'false' process.env.DD_IAST_REDACTION_NAME_PATTERN = 'name_pattern_to_be_overriden_by_options' @@ -1141,8 +1141,7 @@ describe('Config', () => { mode: 'safe' }, apiSecurity: { - enabled: true, - requestSampling: 1.0 + enabled: true }, rasp: { enabled: false @@ -1211,7 +1210,7 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) expect(config).to.have.nested.property('appsec.eventTracking.mode', 'safe') expect(config).to.have.nested.property('appsec.apiSecurity.enabled', true) - expect(config).to.have.nested.property('appsec.apiSecurity.requestSampling', 1.0) + expect(config).to.have.nested.property('appsec.apiSecurity.sampleDelay', 10) expect(config).to.have.nested.property('remoteConfig.pollInterval', 42) expect(config).to.have.nested.property('iast.enabled', true) expect(config).to.have.nested.property('iast.requestSampling', 30) @@ -1239,8 +1238,7 @@ describe('Config', () => { mode: 'disabled' }, apiSecurity: { - enabled: true, - requestSampling: 1.0 + enabled: true }, rasp: { enabled: false @@ -1272,8 +1270,7 @@ describe('Config', () => { mode: 'safe' }, apiSecurity: { - enabled: false, - requestSampling: 0.5 + enabled: false }, rasp: { enabled: true @@ -1309,7 +1306,7 @@ describe('Config', () => { }, apiSecurity: { enabled: true, - requestSampling: 1.0 + sampleDelay: 30 }, sca: { enabled: null @@ -1990,35 +1987,6 @@ describe('Config', () => { }) }) - it('should sanitize values for API Security sampling between 0 and 1', () => { - expect(new Config({ - appsec: { - apiSecurity: { - enabled: true, - requestSampling: 5 - } - } - })).to.have.nested.property('appsec.apiSecurity.requestSampling', 1) - - expect(new Config({ - appsec: { - apiSecurity: { - enabled: true, - requestSampling: -5 - } - } - })).to.have.nested.property('appsec.apiSecurity.requestSampling', 0) - - expect(new Config({ - appsec: { - apiSecurity: { - enabled: true, - requestSampling: 0.1 - } - } - })).to.have.nested.property('appsec.apiSecurity.requestSampling', 0.1) - }) - context('payload tagging', () => { let env From 0871f0e614594eb664997b91840ac62c4e945c9f Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 3 Oct 2024 14:25:32 +0200 Subject: [PATCH 2/7] fix express plugin schema extraction --- packages/dd-trace/test/appsec/index.express.plugin.spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 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 e8b0d4a50e4..886c20d97d6 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -104,9 +104,9 @@ withVersions('express', 'express', version => { appsec.disable() }) - describe('with requestSampling 1.0', () => { + describe('with sample delay 10', () => { beforeEach(() => { - config.appsec.apiSecurity.requestSampling = 1.0 + config.appsec.apiSecurity.sampleDelay = 10 appsec.enable(config) }) @@ -170,7 +170,7 @@ withVersions('express', 'express', version => { }) it('should not get the schema', async () => { - config.appsec.apiSecurity.requestSampling = 0 + config.appsec.apiSecurity.enabled = false appsec.enable(config) const res = await axios.post(`http://localhost:${port}/`, { key: 'value' }) From c1e0f45ec5019552bd31fc37d61a6bc9fdef9551 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 3 Oct 2024 16:39:29 +0200 Subject: [PATCH 3/7] use priority simpler to get span priority --- .../src/appsec/api_security_sampler.js | 16 ++-- .../test/appsec/api_security_sampler.spec.js | 87 +++++++++---------- packages/dd-trace/test/appsec/index.spec.js | 15 +++- 3 files changed, 59 insertions(+), 59 deletions(-) diff --git a/packages/dd-trace/src/appsec/api_security_sampler.js b/packages/dd-trace/src/appsec/api_security_sampler.js index 6baaed4f87e..8c4b8fdc226 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -1,11 +1,12 @@ 'use strict' const ApiSecuritySamplerCache = require('./api_security_sampler_cache') +const PrioritySampler = require('../priority_sampler') const web = require('../plugins/util/web') -const { USER_KEEP, AUTO_KEEP } = require('../../../../ext/priority') let enabled let sampledRequests +const prioritySampler = new PrioritySampler() function configure ({ apiSecurity }) { enabled = apiSecurity.enabled @@ -23,27 +24,22 @@ function sampleRequest (req, res) { const rootSpan = web.root(req) if (!rootSpan) return false - const priority = getSpanPriority(rootSpan) + const isSampled = prioritySampler.isSampled(rootSpan) - if (priority !== AUTO_KEEP && priority !== USER_KEEP) { + if (!isSampled) { return false } const key = sampledRequests.computeKey(req, res) - const isSampled = sampledRequests.isSampled(key) + const alreadySampled = sampledRequests.isSampled(key) - if (isSampled) return false + if (alreadySampled) return false sampledRequests.set(key) return true } -function getSpanPriority (span) { - const spanContext = span.context?.() - return spanContext._sampling?.priority // default ?? -} - module.exports = { configure, disable, 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 08b4355084a..abcee98f3ae 100644 --- a/packages/dd-trace/test/appsec/api_security_sampler.spec.js +++ b/packages/dd-trace/test/appsec/api_security_sampler.spec.js @@ -3,73 +3,68 @@ const proxyquire = require('proxyquire') describe('API Security Sampler', () => { - let apiSecuritySampler, webStub, clock + let apiSecuritySampler, webStub, clock, sampler, span beforeEach(() => { webStub = { root: sinon.stub() } clock = sinon.useFakeTimers(Date.now()) + sampler = sinon.stub().returns({ + isSampled: sinon.stub() + }) + apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { - '../plugins/util/web': webStub + '../plugins/util/web': webStub, + '../priority_sampler': sampler }) + + apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) + + span = { + context: sinon.stub().returns({}) + } }) afterEach(() => { clock.restore() }) - describe('sampleRequest', () => { - beforeEach(() => { - apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) - }) - - it('should return false if not enabled', () => { - apiSecuritySampler.disable() - expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false - }) - - it('should return false if no root span', () => { - webStub.root.returns(null) - expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false - }) + it('should return false if not enabled', () => { + apiSecuritySampler.disable() + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false + }) - it('should return true and put request in cache if priority is AUTO_KEEP', () => { - const rootSpan = { context: () => ({ _sampling: { priority: 2 } }) } - webStub.root.returns(rootSpan) - const req = { url: '/test', method: 'GET' } - const res = { statusCode: 200 } - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true - }) + it('should return false if no root span', () => { + webStub.root.returns(null) + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false + }) - it('should return true and put request in cache if priority is USER_KEEP', () => { - const rootSpan = { context: () => ({ _sampling: { priority: 1 } }) } - webStub.root.returns(rootSpan) - const req = { url: '/test', method: 'GET' } - const res = { statusCode: 200 } - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true - }) + it('should return true and put request in cache if priority is AUTO_KEEP or USER_KEEP', () => { + webStub.root.returns(span) + sampler().isSampled.returns(true) + const req = { url: '/test', method: 'GET' } + const res = { statusCode: 200 } + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + }) - it('should return false if priority is neither AUTO_KEEP nor USER_KEEP', () => { - const rootSpan = { context: () => ({ _sampling: { priority: 0 } }) } - webStub.root.returns(rootSpan) - expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false - }) + it('should return false if priority is neither AUTO_KEEP nor USER_KEEP', () => { + webStub.root.returns(span) + sampler().isSampled.returns(false) + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) - describe('disable', () => { - it('should set enabled to false and clear the cache', () => { - const req = { url: '/test', method: 'GET' } - const res = { statusCode: 200 } + it('should set enabled to false and clear the cache', () => { + const req = { url: '/test', method: 'GET' } + const res = { statusCode: 200 } - const rootSpan = { context: () => ({ _sampling: { priority: 2 } }) } - webStub.root.returns(rootSpan) + webStub.root.returns(span) + sampler().isSampled.returns(true) - apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) - expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true - apiSecuritySampler.disable() + apiSecuritySampler.disable() - expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false - }) + expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) }) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index c83852750fc..13f6b197f0c 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -46,6 +46,7 @@ describe('AppSec Index', function () { let graphql let apiSecuritySampler let rasp + let sampler const RULES = { rules: [{ a: 1 }] } @@ -78,6 +79,10 @@ describe('AppSec Index', function () { root: sinon.stub() } + sampler = sinon.stub().returns({ + isSampled: sinon.stub() + }) + blocking = { setTemplates: sinon.stub() } @@ -103,7 +108,8 @@ describe('AppSec Index', function () { } apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { - '../plugins/util/web': web + '../plugins/util/web': web, + '../priority_sampler': sampler }) sinon.spy(apiSecuritySampler, 'sampleRequest') @@ -549,9 +555,12 @@ describe('AppSec Index', function () { statusCode: 201 } - const rootSpan = { context: () => ({ _sampling: { priority: 2 } }) } + const span = { + context: sinon.stub().returns({}) + } - web.root.returns(rootSpan) + web.root.returns(span) + sampler().isSampled.returns(true) AppSec.incomingHttpEndTranslator({ req, res }) From 200a1775e2c4b041ee2facb4b193153789ee21cf Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 7 Oct 2024 09:24:31 +0200 Subject: [PATCH 4/7] use lru cache package --- .../src/appsec/api_security_sampler.js | 42 ++++++++++-- .../src/appsec/api_security_sampler_cache.js | 53 --------------- .../test/appsec/api_security_sampler.spec.js | 56 +++++++++++----- .../appsec/api_security_sampler_cache.spec.js | 65 ------------------- 4 files changed, 76 insertions(+), 140 deletions(-) delete mode 100644 packages/dd-trace/src/appsec/api_security_sampler_cache.js delete mode 100644 packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js diff --git a/packages/dd-trace/src/appsec/api_security_sampler.js b/packages/dd-trace/src/appsec/api_security_sampler.js index 8c4b8fdc226..d53bd1d85a5 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -1,16 +1,23 @@ 'use strict' -const ApiSecuritySamplerCache = require('./api_security_sampler_cache') +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 MAX_SIZE = 4096 +const DEFAULT_DELAY = 30 // 30s let enabled let sampledRequests -const prioritySampler = new PrioritySampler() +let prioritySampler function configure ({ apiSecurity }) { enabled = apiSecurity.enabled - sampledRequests = new ApiSecuritySamplerCache(apiSecurity.sampleDelay) + const ttl = parseSampleDelay(apiSecurity.sampleDelay) * 1000 + sampledRequests = new LRUCache({ max: MAX_SIZE, ttl }) + prioritySampler = new PrioritySampler() } function disable () { @@ -30,8 +37,8 @@ function sampleRequest (req, res) { return false } - const key = sampledRequests.computeKey(req, res) - const alreadySampled = sampledRequests.isSampled(key) + const key = computeKey(req, res) + const alreadySampled = sampledRequests.has(key) if (alreadySampled) return false @@ -40,8 +47,31 @@ function sampleRequest (req, res) { return true } +function isSampled (req, res) { + const key = computeKey(req, res) + return !!sampledRequests.has(key) +} + +function computeKey (req, res) { + const route = req.url + const method = req.method.toLowerCase() + const statusCode = res.statusCode + const str = route + statusCode + method + return crypto.createHash('md5').update(str).digest('hex') +} + +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 + } +} + module.exports = { configure, disable, - sampleRequest + sampleRequest, + isSampled } diff --git a/packages/dd-trace/src/appsec/api_security_sampler_cache.js b/packages/dd-trace/src/appsec/api_security_sampler_cache.js deleted file mode 100644 index 4c328c43428..00000000000 --- a/packages/dd-trace/src/appsec/api_security_sampler_cache.js +++ /dev/null @@ -1,53 +0,0 @@ -'use strict' - -const crypto = require('node:crypto') -const log = require('../log') - -const MAX_SIZE = 4096 -const DEFAULT_DELAY = 30 // 30s - -class ApiSecuritySamplerCache extends Map { - constructor (delay) { - super() - this.delay = this._parseSampleDelay(delay) - } - - _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 - } - } - - computeKey (req, res) { - const route = req.url - const method = req.method.toLowerCase() - const statusCode = res.statusCode - const str = route + statusCode + method - return crypto.createHash('md5').update(str).digest('hex') - } - - isSampled (key) { - if (!super.has(key)) { - return false - } - const previous = super.get(key) - return Date.now() - previous < (this.delay * 1000) - } - - set (key) { - if (super.has(key)) { - super.delete(key) - } - - super.set(key, Date.now()) - if (super.size > MAX_SIZE) { - const oldestKey = super.keys().next().value - super.delete(oldestKey) - } - } -} - -module.exports = ApiSecuritySamplerCache 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 abcee98f3ae..f2d0b0da7f6 100644 --- a/packages/dd-trace/test/appsec/api_security_sampler.spec.js +++ b/packages/dd-trace/test/appsec/api_security_sampler.spec.js @@ -1,13 +1,17 @@ 'use strict' +const { performance } = require('node:perf_hooks') const proxyquire = require('proxyquire') describe('API Security Sampler', () => { - let apiSecuritySampler, webStub, clock, sampler, span + const req = { url: '/test', method: 'GET' } + const res = { statusCode: 200 } + let apiSecuritySampler, performanceNowStub, webStub, sampler, span beforeEach(() => { + performanceNowStub = sinon.stub(performance, 'now').returns(0) + webStub = { root: sinon.stub() } - clock = sinon.useFakeTimers(Date.now()) sampler = sinon.stub().returns({ isSampled: sinon.stub() @@ -18,15 +22,21 @@ describe('API Security Sampler', () => { '../priority_sampler': sampler }) - apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) + apiSecuritySampler.configure({ apiSecurity: { enabled: true } }) span = { context: sinon.stub().returns({}) } + + webStub.root.returns(span) + sampler().isSampled.returns(true) + + performanceNowStub.returns(performance.now() + 1) }) afterEach(() => { - clock.restore() + performanceNowStub.restore() + apiSecuritySampler.disable() }) it('should return false if not enabled', () => { @@ -40,31 +50,45 @@ describe('API Security Sampler', () => { }) it('should return true and put request in cache if priority is AUTO_KEEP or USER_KEEP', () => { - webStub.root.returns(span) - sampler().isSampled.returns(true) - const req = { url: '/test', method: 'GET' } - const res = { statusCode: 200 } + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + }) + + it('should not sample before 30 seconds', () => { + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + performanceNowStub.returns(performance.now() + 25000) + + expect(apiSecuritySampler.sampleRequest(req, res)).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 + + 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', () => { - webStub.root.returns(span) sampler().isSampled.returns(false) expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false }) - it('should set enabled to false and clear the cache', () => { - const req = { url: '/test', method: 'GET' } - const res = { statusCode: 200 } + 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, url: `/test${i}` }, res)).to.be.true + } - webStub.root.returns(span) - sampler().isSampled.returns(true) + expect(apiSecuritySampler.isSampled({ method, url: '/test0' }, res)).to.be.false + expect(apiSecuritySampler.isSampled({ method, url: '/test4096' }, res)).to.be.true + }) - apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) + it('should set enabled to false and clear the cache', () => { expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true apiSecuritySampler.disable() - expect(apiSecuritySampler.sampleRequest({}, {})).to.be.false + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.false }) }) diff --git a/packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js b/packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js deleted file mode 100644 index 868705cd458..00000000000 --- a/packages/dd-trace/test/appsec/api_security_sampler_cache.spec.js +++ /dev/null @@ -1,65 +0,0 @@ -'use strict' - -const ApiSecuritySamplerCache = require('../../src/appsec/api_security_sampler_cache') - -describe('ApiSecuritySamplerCache', () => { - let cache - let clock - - beforeEach(() => { - cache = new ApiSecuritySamplerCache() - clock = sinon.useFakeTimers(Date.now()) - }) - - afterEach(() => { - clock.restore() - }) - - describe('Standard behavior with default delay', () => { - it('should not sample when first seen', () => { - const req = { url: '/test', method: 'GET' } - const res = { status_code: 200 } - const key = cache.computeKey(req, res) - expect(cache.isSampled(key)).to.be.false - }) - - it('should sample within 30 seconds of first seen', () => { - const req = { url: '/test', method: 'GET' } - const res = { status_code: 200 } - const key = cache.computeKey(req, res) - cache.set(key) - clock.tick(29000) - expect(cache.isSampled(key)).to.be.true - }) - - it('should not sample after 30 seconds', () => { - const req = { url: '/test', method: 'GET' } - const res = { status_code: 200 } - const key = cache.computeKey(req, res) - cache.set(key) - clock.tick(31000) - expect(cache.isSampled(key)).to.be.false - }) - }) - - describe('Max size behavior', () => { - it('should remove oldest entry when max size is exceeded', () => { - const baseReq = { method: 'GET' } - const baseRes = { status_code: 200 } - - for (let i = 0; i < 4097; i++) { - const req = { ...baseReq, url: `test${i}` } - const key = cache.computeKey(req, baseRes) - cache.set(key) - } - - expect(cache.size).to.equal(4096) - - const firstKey = cache.computeKey({ ...baseReq, url: 'test0' }, baseRes) - expect(cache.isSampled(firstKey)).to.be.false - - const lastKey = cache.computeKey({ ...baseReq, url: 'test4096' }, baseRes) - expect(cache.isSampled(lastKey)).to.be.true - }) - }) -}) From 7d0ace06f603ff5962a989ccf0239b6dab0745ba Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 7 Oct 2024 14:01:10 +0200 Subject: [PATCH 5/7] use route path instead of url --- .../src/appsec/api_security_sampler.js | 2 +- packages/dd-trace/src/appsec/index.js | 2 +- .../test/appsec/api_security_sampler.spec.js | 39 ++++++++++++++++--- packages/dd-trace/test/appsec/index.spec.js | 4 +- 4 files changed, 39 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/src/appsec/api_security_sampler.js b/packages/dd-trace/src/appsec/api_security_sampler.js index d53bd1d85a5..e8f1d834ed4 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -53,7 +53,7 @@ function isSampled (req, res) { } function computeKey (req, res) { - const route = req.url + const route = req.route.path const method = req.method.toLowerCase() const statusCode = res.statusCode const str = route + statusCode + method diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 56b645dbcbc..8655b767285 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 (apiSecuritySampler.sampleRequest(req, res)) { + if (req.route && typeof req.route.path === 'string' && apiSecuritySampler.sampleRequest(req, res)) { persistent[addresses.WAF_CONTEXT_PROCESSOR] = { 'extract-schema': true } } 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 f2d0b0da7f6..11e5bc61730 100644 --- a/packages/dd-trace/test/appsec/api_security_sampler.spec.js +++ b/packages/dd-trace/test/appsec/api_security_sampler.spec.js @@ -4,7 +4,7 @@ const { performance } = require('node:perf_hooks') const proxyquire = require('proxyquire') describe('API Security Sampler', () => { - const req = { url: '/test', method: 'GET' } + const req = { route: { path: '/test' }, method: 'GET' } const res = { statusCode: 200 } let apiSecuritySampler, performanceNowStub, webStub, sampler, span @@ -77,11 +77,10 @@ describe('API Security Sampler', () => { 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, url: `/test${i}` }, res)).to.be.true + expect(apiSecuritySampler.sampleRequest({ method, route: { path: `/test${i}` } }, res)).to.be.true } - - expect(apiSecuritySampler.isSampled({ method, url: '/test0' }, res)).to.be.false - expect(apiSecuritySampler.isSampled({ method, url: '/test4096' }, res)).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 }) it('should set enabled to false and clear the cache', () => { @@ -91,4 +90,34 @@ describe('API Security Sampler', () => { 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 + }) + + 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 + expect(apiSecuritySampler.isSampled(getReq, res)).to.be.true + expect(apiSecuritySampler.isSampled(postReq, res)).to.be.true + }) + + it('should create different keys for different status codes', () => { + 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.isSampled(req, res200)).to.be.true + expect(apiSecuritySampler.isSampled(req, res404)).to.be.true + }) }) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 13f6b197f0c..18937c6db38 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -533,7 +533,9 @@ describe('AppSec Index', function () { AppSec.enable(config) const req = { - url: '/path', + route: { + path: '/path' + }, headers: { 'user-agent': 'Arachni', host: 'localhost' From 4ebff2457001eda2a7e80702f991f865fe969db3 Mon Sep 17 00:00:00 2001 From: ishabi Date: Tue, 8 Oct 2024 09:59:25 +0200 Subject: [PATCH 6/7] use route.path or url to generate the key --- .../datadog-instrumentations/src/express.js | 2 +- .../src/appsec/api_security_sampler.js | 22 +++++- packages/dd-trace/src/appsec/index.js | 5 +- .../test/appsec/api_security_sampler.spec.js | 73 +++++++++++++++++-- packages/dd-trace/test/appsec/index.spec.js | 24 +++++- 5 files changed, 111 insertions(+), 15 deletions(-) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index d3113821364..97803268ebd 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -28,7 +28,7 @@ function wrapResponseJson (json) { obj = arguments[1] } - responseJsonChannel.publish({ req: this.req, body: obj }) + responseJsonChannel.publish({ req: this.req, res: this.res, 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 e8f1d834ed4..6d9cf10739a 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -5,6 +5,7 @@ const LRUCache = require('lru-cache') const PrioritySampler = require('../priority_sampler') const web = require('../plugins/util/web') const log = require('../log') +const { USER_KEEP, AUTO_KEEP, AUTO_REJECT, USER_REJECT } = require('../../../../ext/priority') const MAX_SIZE = 4096 const DEFAULT_DELAY = 30 // 30s @@ -31,12 +32,26 @@ function sampleRequest (req, res) { const rootSpan = web.root(req) if (!rootSpan) return false + const priority = getSpanPriority(rootSpan) + + if (priority === AUTO_REJECT || priority === USER_REJECT) { + return false + } + + if (priority === AUTO_KEEP || priority === USER_KEEP) { + return sample(req, res) + } + const isSampled = prioritySampler.isSampled(rootSpan) if (!isSampled) { return false } + return sample(req, res) +} + +function sample (req, res) { const key = computeKey(req, res) const alreadySampled = sampledRequests.has(key) @@ -53,7 +68,7 @@ function isSampled (req, res) { } function computeKey (req, res) { - const route = req.route.path + const route = req.route?.path || req.url const method = req.method.toLowerCase() const statusCode = res.statusCode const str = route + statusCode + method @@ -69,6 +84,11 @@ function parseSampleDelay (delay) { } } +function getSpanPriority (span) { + const spanContext = span.context?.() + return spanContext._sampling?.priority +} + module.exports = { configure, disable, diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 8655b767285..ae33d2745a8 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)) { 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.isSampled(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 11e5bc61730..865a0ba3605 100644 --- a/packages/dd-trace/test/appsec/api_security_sampler.spec.js +++ b/packages/dd-trace/test/appsec/api_security_sampler.spec.js @@ -2,6 +2,7 @@ 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' } @@ -25,7 +26,9 @@ describe('API Security Sampler', () => { apiSecuritySampler.configure({ apiSecurity: { enabled: true } }) span = { - context: sinon.stub().returns({}) + context: sinon.stub().returns({ + _sampling: { priority: AUTO_KEEP } + }) } webStub.root.returns(span) @@ -49,10 +52,37 @@ 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 } }) + sampler().isSampled.returns(false) expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true }) + it('should sample for USER_KEEP priority without checking prioritySampler', () => { + span.context.returns({ _sampling: { priority: USER_KEEP } }) + sampler().isSampled.returns(false) + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + }) + + it('should use prioritySampler for undefined priority', () => { + span.context.returns({ _sampling: { priority: undefined } }) + sampler().isSampled.returns(true) + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true + + sampler().isSampled.returns(false) + expect(apiSecuritySampler.sampleRequest(req, res)).to.be.false + }) + it('should not sample before 30 seconds', () => { expect(apiSecuritySampler.sampleRequest(req, res)).to.be.true performanceNowStub.returns(performance.now() + 25000) @@ -69,11 +99,6 @@ describe('API Security Sampler', () => { 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 - }) - it('should remove oldest entry when max size is exceeded', () => { const method = req.method for (let i = 0; i < 4097; i++) { @@ -120,4 +145,38 @@ describe('API Security Sampler', () => { expect(apiSecuritySampler.isSampled(req, res200)).to.be.true expect(apiSecuritySampler.isSampled(req, res404)).to.be.true }) + + it('should use route.path when available', () => { + const reqWithRoute = { + route: { path: '/users/:id' }, + url: '/users/123', + method: 'GET' + } + + apiSecuritySampler.sampleRequest(reqWithRoute, res) + expect(apiSecuritySampler.isSampled(reqWithRoute, res)).to.be.true + + const reqWithDifferentUrl = { + route: { path: '/users/:id' }, + url: '/users/456', + method: 'GET' + } + expect(apiSecuritySampler.isSampled(reqWithDifferentUrl, res)).to.be.true + }) + + it('should fall back to url when route.path is not available', () => { + const reqWithUrl = { + url: '/users/123', + method: 'GET' + } + + apiSecuritySampler.sampleRequest(reqWithUrl, res) + expect(apiSecuritySampler.isSampled(reqWithUrl, res)).to.be.true + + const reqWithDifferentUrl = { + url: '/users/456', + method: 'GET' + } + expect(apiSecuritySampler.isSampled(reqWithDifferentUrl, res)).to.be.false + }) }) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 18937c6db38..bd0144e7050 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -112,6 +112,7 @@ describe('AppSec Index', function () { '../priority_sampler': sampler }) sinon.spy(apiSecuritySampler, 'sampleRequest') + sinon.spy(apiSecuritySampler, 'isSampled') rasp = { enable: sinon.stub(), @@ -588,18 +589,33 @@ describe('AppSec Index', function () { }) it('should not do anything if body is not an object', () => { - responseBody.publish({ req: {}, body: 'string' }) - responseBody.publish({ req: {}, body: null }) + responseBody.publish({ req: {}, res: {}, body: 'string' }) + responseBody.publish({ req: {}, res: {}, body: null }) + expect(apiSecuritySampler.isSampled).to.not.been.called expect(waf.run).to.not.been.called }) - it('should call to the waf if body is an object', () => { + it('should not call to the waf if it is not a sampled request', () => { + apiSecuritySampler.isSampled = apiSecuritySampler.isSampled.instantiateFake(() => false) const req = {} + const res = {} + + responseBody.publish({ req, res, body: {} }) + + expect(apiSecuritySampler.isSampled).to.have.been.calledOnceWith(req) + expect(waf.run).to.not.been.called + }) + + it('should call to the waf if it is a sampled request', () => { + apiSecuritySampler.isSampled = apiSecuritySampler.isSampled.instantiateFake(() => true) + const req = {} + const res = {} const body = {} - responseBody.publish({ req, body }) + responseBody.publish({ req, res, body }) + expect(apiSecuritySampler.isSampled).to.have.been.calledOnceWith(req, res) expect(waf.run).to.been.calledOnceWith({ persistent: { [addresses.HTTP_INCOMING_RESPONSE_BODY]: body From 832800a3d31741032d180aa1a7efcce0274fac50 Mon Sep 17 00:00:00 2001 From: Igor Unanua Date: Wed, 9 Oct 2024 14:59:23 +0200 Subject: [PATCH 7/7] call sampleRequest in onResponseBody and incomingHttpEndTranslator --- .../datadog-instrumentations/src/express.js | 2 +- .../src/appsec/api_security_sampler.js | 23 ++++++++----------- packages/dd-trace/src/appsec/index.js | 4 ++-- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index 97803268ebd..34ece9d1376 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -28,7 +28,7 @@ function wrapResponseJson (json) { obj = arguments[1] } - responseJsonChannel.publish({ req: this.req, res: this.res, 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 6d9cf10739a..9d3c7ea824a 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -2,23 +2,20 @@ 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 { USER_KEEP, AUTO_KEEP, AUTO_REJECT, USER_REJECT } = require('../../../../ext/priority') +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() } function disable () { @@ -26,8 +23,8 @@ function disable () { sampledRequests?.clear() } -function sampleRequest (req, res) { - if (!enabled) return false +function sampleRequest (req, res, forceSample) { + if (!enabled || this.isSampled(req, res)) return false const rootSpan = web.root(req) if (!rootSpan) return false @@ -38,17 +35,15 @@ function sampleRequest (req, res) { return false } - if (priority === AUTO_KEEP || priority === USER_KEEP) { - return sample(req, res) + if (!priority && !rootSpan._prioritySampler?.isSampled(rootSpan)) { + return false } - const isSampled = prioritySampler.isSampled(rootSpan) - - if (!isSampled) { - return false + if (forceSample) { + sample(req, res) } - return sample(req, res) + return true } function sample (req, res) { @@ -70,7 +65,7 @@ function isSampled (req, res) { function computeKey (req, res) { const route = req.route?.path || req.url const method = req.method.toLowerCase() - const statusCode = res.statusCode + const statusCode = res.statusCode === 304 ? 200 : res.statusCode const str = route + statusCode + method return crypto.createHash('md5').update(str).digest('hex') } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index ae33d2745a8..3ea67feda71 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 (apiSecuritySampler.sampleRequest(req, res)) { + if (apiSecuritySampler.sampleRequest(req, res, true)) { persistent[addresses.WAF_CONTEXT_PROCESSOR] = { 'extract-schema': true } } @@ -202,7 +202,7 @@ function onRequestCookieParser ({ req, res, abortController, cookies }) { function onResponseBody ({ req, res, body }) { if (!body || typeof body !== 'object') return - if (!apiSecuritySampler.isSampled(req, res)) return + if (!apiSecuritySampler.sampleRequest(req, res)) return // we don't support blocking at this point, so no results needed waf.run({