Skip to content

Commit d19f3b0

Browse files
CarlesDDiunanua
CarlesDD
and
iunanua
authored
Fix IAST standalone sampling priority propagation (#4927)
* WIP * Disable vuln deduplication in OCE test * Test vuln deduplication on the fly * Skip vuln dedup in multiple sends test * Fix lint issues * Remove multiple send test * Move on the fly span creation for vulns out of req to addVulnerability method * Move finish out-of-request span * Update packages/dd-trace/src/appsec/iast/vulnerability-reporter.js Co-authored-by: Igor Unanua <[email protected]> --------- Co-authored-by: Igor Unanua <[email protected]>
1 parent b456550 commit d19f3b0

File tree

3 files changed

+62
-66
lines changed

3 files changed

+62
-66
lines changed

‎packages/dd-trace/src/appsec/iast/vulnerability-reporter.js

+32-37
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,36 @@ let resetVulnerabilityCacheTimer
1717
let deduplicationEnabled = true
1818

1919
function addVulnerability (iastContext, vulnerability) {
20-
if (vulnerability && vulnerability.evidence && vulnerability.type &&
21-
vulnerability.location) {
22-
if (iastContext && iastContext.rootSpan) {
20+
if (vulnerability?.evidence && vulnerability?.type && vulnerability?.location) {
21+
if (deduplicationEnabled && isDuplicatedVulnerability(vulnerability)) return
22+
23+
VULNERABILITY_HASHES.set(`${vulnerability.type}${vulnerability.hash}`, true)
24+
25+
let span = iastContext?.rootSpan
26+
27+
if (!span && tracer) {
28+
span = tracer.startSpan('vulnerability', {
29+
type: 'vulnerability'
30+
})
31+
32+
vulnerability.location.spanId = span.context().toSpanId()
33+
34+
span.addTags({
35+
[IAST_ENABLED_TAG_KEY]: 1
36+
})
37+
}
38+
39+
if (!span) return
40+
41+
keepTrace(span, SAMPLING_MECHANISM_APPSEC)
42+
standalone.sample(span)
43+
44+
if (iastContext?.rootSpan) {
2345
iastContext[VULNERABILITIES_KEY] = iastContext[VULNERABILITIES_KEY] || []
2446
iastContext[VULNERABILITIES_KEY].push(vulnerability)
2547
} else {
26-
sendVulnerabilities([vulnerability])
48+
sendVulnerabilities([vulnerability], span)
49+
span.finish()
2750
}
2851
}
2952
}
@@ -34,36 +57,17 @@ function isValidVulnerability (vulnerability) {
3457
vulnerability.location && vulnerability.location.spanId
3558
}
3659

37-
function sendVulnerabilities (vulnerabilities, rootSpan) {
60+
function sendVulnerabilities (vulnerabilities, span) {
3861
if (vulnerabilities && vulnerabilities.length) {
39-
let span = rootSpan
40-
if (!span && tracer) {
41-
span = tracer.startSpan('vulnerability', {
42-
type: 'vulnerability'
43-
})
44-
vulnerabilities.forEach((vulnerability) => {
45-
vulnerability.location.spanId = span.context().toSpanId()
46-
})
47-
span.addTags({
48-
[IAST_ENABLED_TAG_KEY]: 1
49-
})
50-
}
51-
5262
if (span && span.addTags) {
53-
const validAndDedupVulnerabilities = deduplicateVulnerabilities(vulnerabilities).filter(isValidVulnerability)
54-
const jsonToSend = vulnerabilitiesFormatter.toJson(validAndDedupVulnerabilities)
63+
const validatedVulnerabilities = vulnerabilities.filter(isValidVulnerability)
64+
const jsonToSend = vulnerabilitiesFormatter.toJson(validatedVulnerabilities)
5565

5666
if (jsonToSend.vulnerabilities.length > 0) {
5767
const tags = {}
5868
// TODO: Store this outside of the span and set the tag in the exporter.
5969
tags[IAST_JSON_TAG_KEY] = JSON.stringify(jsonToSend)
6070
span.addTags(tags)
61-
62-
keepTrace(span, SAMPLING_MECHANISM_APPSEC)
63-
64-
standalone.sample(span)
65-
66-
if (!rootSpan) span.finish()
6771
}
6872
}
6973
}
@@ -86,17 +90,8 @@ function stopClearCacheTimer () {
8690
}
8791
}
8892

89-
function deduplicateVulnerabilities (vulnerabilities) {
90-
if (!deduplicationEnabled) return vulnerabilities
91-
const deduplicated = vulnerabilities.filter((vulnerability) => {
92-
const key = `${vulnerability.type}${vulnerability.hash}`
93-
if (!VULNERABILITY_HASHES.get(key)) {
94-
VULNERABILITY_HASHES.set(key, true)
95-
return true
96-
}
97-
return false
98-
})
99-
return deduplicated
93+
function isDuplicatedVulnerability (vulnerability) {
94+
return VULNERABILITY_HASHES.get(`${vulnerability.type}${vulnerability.hash}`)
10095
}
10196

10297
function start (config, _tracer) {

‎packages/dd-trace/test/appsec/iast/overhead-controller.spec.js

+4-6
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,8 @@ describe('Overhead controller', () => {
331331
iast: {
332332
enabled: true,
333333
requestSampling: 100,
334-
maxConcurrentRequests: 2
334+
maxConcurrentRequests: 2,
335+
deduplicationEnabled: false
335336
}
336337
}
337338
})
@@ -365,15 +366,13 @@ describe('Overhead controller', () => {
365366
} else if (url === SECOND_REQUEST) {
366367
setImmediate(() => {
367368
requestResolvers[FIRST_REQUEST]()
368-
vulnerabilityReporter.clearCache()
369369
})
370370
}
371371
})
372372
testRequestEventEmitter.on(TEST_REQUEST_FINISHED, (url) => {
373373
if (url === FIRST_REQUEST) {
374374
setImmediate(() => {
375375
requestResolvers[SECOND_REQUEST]()
376-
vulnerabilityReporter.clearCache()
377376
})
378377
}
379378
})
@@ -388,7 +387,8 @@ describe('Overhead controller', () => {
388387
iast: {
389388
enabled: true,
390389
requestSampling: 100,
391-
maxConcurrentRequests: 2
390+
maxConcurrentRequests: 2,
391+
deduplicationEnabled: false
392392
}
393393
}
394394
})
@@ -435,7 +435,6 @@ describe('Overhead controller', () => {
435435
requestResolvers[FIRST_REQUEST]()
436436
} else if (url === FIFTH_REQUEST) {
437437
requestResolvers[SECOND_REQUEST]()
438-
vulnerabilityReporter.clearCache()
439438
}
440439
})
441440
testRequestEventEmitter.on(TEST_REQUEST_FINISHED, (url) => {
@@ -444,7 +443,6 @@ describe('Overhead controller', () => {
444443
axios.get(`http://localhost:${serverConfig.port}${FIFTH_REQUEST}`).then().catch(done)
445444
} else if (url === SECOND_REQUEST) {
446445
setImmediate(() => {
447-
vulnerabilityReporter.clearCache()
448446
requestResolvers[THIRD_REQUEST]()
449447
requestResolvers[FOURTH_REQUEST]()
450448
requestResolvers[FIFTH_REQUEST]()

‎packages/dd-trace/test/appsec/iast/vulnerability-reporter.spec.js

+26-23
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,14 @@ describe('vulnerability-reporter', () => {
5252
expect(iastContext.vulnerabilities).to.be.an('array')
5353
})
5454

55-
it('should add multiple vulnerabilities', () => {
55+
it('should deduplicate same vulnerabilities', () => {
5656
addVulnerability(iastContext,
5757
vulnerabilityAnalyzer._createVulnerability('INSECURE_HASHING', { value: 'sha1' }, -555))
5858
addVulnerability(iastContext,
5959
vulnerabilityAnalyzer._createVulnerability('INSECURE_HASHING', { value: 'sha1' }, 888))
6060
addVulnerability(iastContext,
6161
vulnerabilityAnalyzer._createVulnerability('INSECURE_HASHING', { value: 'sha1' }, 123))
62-
expect(iastContext.vulnerabilities).to.have.length(3)
62+
expect(iastContext.vulnerabilities).to.have.length(1)
6363
})
6464

6565
it('should add in the context evidence properties', () => {
@@ -260,7 +260,12 @@ describe('vulnerability-reporter', () => {
260260
'[{"value":"SELECT id FROM u WHERE email = \'"},{"value":"[email protected]","source":1},{"value":"\';"}]},' +
261261
'"location":{"spanId":888,"path":"filename.js","line":99}}]}'
262262
})
263-
expect(prioritySampler.setPriority).to.have.been.calledOnceWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
263+
264+
expect(prioritySampler.setPriority).to.have.been.calledTwice
265+
expect(prioritySampler.setPriority.firstCall)
266+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
267+
expect(prioritySampler.setPriority.secondCall)
268+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
264269
})
265270

266271
it('should send multiple vulnerabilities with same tainted source', () => {
@@ -313,7 +318,12 @@ describe('vulnerability-reporter', () => {
313318
'[{"value":"UPDATE u SET name=\'"},{"value":"joe","source":0},{"value":"\' WHERE id=1;"}]},' +
314319
'"location":{"spanId":888,"path":"filename.js","line":99}}]}'
315320
})
316-
expect(prioritySampler.setPriority).to.have.been.calledOnceWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
321+
322+
expect(prioritySampler.setPriority).to.have.been.calledTwice
323+
expect(prioritySampler.setPriority.firstCall)
324+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
325+
expect(prioritySampler.setPriority.secondCall)
326+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
317327
})
318328

319329
it('should send once with multiple vulnerabilities', () => {
@@ -334,7 +344,13 @@ describe('vulnerability-reporter', () => {
334344
'{"type":"INSECURE_HASHING","hash":1755238473,"evidence":{"value":"md5"},' +
335345
'"location":{"spanId":-5,"path":"/path/to/file3.js","line":3}}]}'
336346
})
337-
expect(prioritySampler.setPriority).to.have.been.calledOnceWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
347+
expect(prioritySampler.setPriority).to.have.been.calledThrice
348+
expect(prioritySampler.setPriority.firstCall)
349+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
350+
expect(prioritySampler.setPriority.secondCall)
351+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
352+
expect(prioritySampler.setPriority.thirdCall)
353+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
338354
})
339355

340356
it('should send once vulnerability with one vulnerability', () => {
@@ -366,23 +382,6 @@ describe('vulnerability-reporter', () => {
366382
expect(prioritySampler.setPriority).to.have.been.calledOnceWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
367383
})
368384

369-
it('should not send duplicated vulnerabilities in multiple sends', () => {
370-
const iastContext = { rootSpan: span }
371-
addVulnerability(iastContext,
372-
vulnerabilityAnalyzer._createVulnerability('INSECURE_HASHING', { value: 'sha1' }, 888,
373-
{ path: 'filename.js', line: 88 }))
374-
addVulnerability(iastContext,
375-
vulnerabilityAnalyzer._createVulnerability('INSECURE_HASHING', { value: 'sha1' }, 888,
376-
{ path: 'filename.js', line: 88 }))
377-
sendVulnerabilities(iastContext.vulnerabilities, span)
378-
sendVulnerabilities(iastContext.vulnerabilities, span)
379-
expect(span.addTags).to.have.been.calledOnceWithExactly({
380-
'_dd.iast.json': '{"sources":[],"vulnerabilities":[{"type":"INSECURE_HASHING","hash":3410512691,' +
381-
'"evidence":{"value":"sha1"},"location":{"spanId":888,"path":"filename.js","line":88}}]}'
382-
})
383-
expect(prioritySampler.setPriority).to.have.been.calledOnceWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
384-
})
385-
386385
it('should not deduplicate vulnerabilities if not enabled', () => {
387386
start({
388387
iast: {
@@ -401,7 +400,11 @@ describe('vulnerability-reporter', () => {
401400
'{"type":"INSECURE_HASHING","hash":3410512691,"evidence":{"value":"sha1"},"location":' +
402401
'{"spanId":888,"path":"filename.js","line":88}}]}'
403402
})
404-
expect(prioritySampler.setPriority).to.have.been.calledOnceWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
403+
expect(prioritySampler.setPriority).to.have.been.calledTwice
404+
expect(prioritySampler.setPriority.firstCall)
405+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
406+
expect(prioritySampler.setPriority.secondCall)
407+
.to.have.been.calledWithExactly(span, USER_KEEP, SAMPLING_MECHANISM_APPSEC)
405408
})
406409

407410
it('should add _dd.p.appsec trace tag with standalone enabled', () => {

0 commit comments

Comments
 (0)