Skip to content

Commit

Permalink
use route.path or url to generate the key
Browse files Browse the repository at this point in the history
  • Loading branch information
IlyasShabi committed Oct 11, 2024
1 parent 55d4c01 commit 3ecd200
Show file tree
Hide file tree
Showing 8 changed files with 129 additions and 87 deletions.
2 changes: 1 addition & 1 deletion index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ declare namespace tracer {
*/
apiSecurity?: {
/** Whether to enable Api Security.
* @default false
* @default true
*/
enabled?: boolean,

Expand Down
2 changes: 1 addition & 1 deletion integration-tests/standalone-asm.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
2 changes: 1 addition & 1 deletion packages/datadog-instrumentations/src/express.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
62 changes: 36 additions & 26 deletions packages/dd-trace/src/appsec/api_security_sampler.js
Original file line number Diff line number Diff line change
@@ -1,72 +1,82 @@
'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 () {
enabled = false
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 = {
Expand Down
5 changes: 3 additions & 2 deletions packages/dd-trace/src/appsec/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
}

Expand Down Expand Up @@ -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({
Expand Down
98 changes: 55 additions & 43 deletions packages/dd-trace/test/appsec/api_security_sampler.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
Expand All @@ -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
})
Expand All @@ -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
})
})
2 changes: 1 addition & 1 deletion packages/dd-trace/test/appsec/index.express.plugin.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, () => {
Expand Down
Loading

0 comments on commit 3ecd200

Please sign in to comment.