Skip to content

Commit 3913137

Browse files
CarlesDDCarlesDD
authored
feat(iast): Improve callsite processing on IAST vuln location computing (#7509)
* Improve callsite processing on IAST vuln location computing * Simplify excluding conditional * Specify array types in getCallSiteFramesForLocation JSDoc
1 parent e51c0ec commit 3913137

File tree

3 files changed

+62
-72
lines changed

3 files changed

+62
-72
lines changed

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
'use strict'
22

33
const { storage } = require('../../../../../datadog-core')
4-
const { getNonDDCallSiteFrames } = require('../path-line')
4+
const { getCallSiteFramesForLocation } = require('../path-line')
55
const { getIastContext, getIastStackTraceId } = require('../iast-context')
66
const overheadController = require('../overhead-controller')
77
const { SinkIastPlugin } = require('../iast-plugin')
@@ -35,9 +35,8 @@ class Analyzer extends SinkIastPlugin {
3535

3636
_reportEvidence (value, context, evidence) {
3737
const callSiteFrames = getVulnerabilityCallSiteFrames()
38-
const nonDDCallSiteFrames = getNonDDCallSiteFrames(callSiteFrames, this._getExcludedPaths())
39-
40-
const location = this._getLocation(value, nonDDCallSiteFrames)
38+
const frames = getCallSiteFramesForLocation(callSiteFrames, this._getExcludedPaths())
39+
const location = this._getLocation(value, frames)
4140

4241
if (!this._isExcluded(location)) {
4342
const originalLocation = this._getOriginalLocation(location)
@@ -51,7 +50,7 @@ class Analyzer extends SinkIastPlugin {
5150
stackId
5251
)
5352

54-
addVulnerability(context, vulnerability, nonDDCallSiteFrames)
53+
addVulnerability(context, vulnerability, frames)
5554
}
5655
}
5756

‎packages/dd-trace/src/appsec/iast/path-line.js‎

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -8,29 +8,40 @@ const { getOriginalPathAndLineFromSourceMap } = require('./taint-tracking/rewrit
88
const pathLine = {
99
getNodeModulesPaths,
1010
getRelativePath,
11-
getNonDDCallSiteFrames,
11+
getCallSiteFramesForLocation,
1212
ddBasePath, // Exported only for test purposes
1313
}
1414

1515
const EXCLUDED_PATHS = [
1616
path.join(path.sep, 'node_modules', 'dc-polyfill'),
1717
]
18-
const EXCLUDED_PATH_PREFIXES = [
19-
'node:diagnostics_channel',
20-
'diagnostics_channel',
21-
'node:child_process',
22-
'child_process',
23-
'node:async_hooks',
24-
'async_hooks',
25-
'node:internal/async_local_storage',
26-
]
2718

28-
function getNonDDCallSiteFrames (callSiteFrames, externallyExcludedPaths) {
19+
/**
20+
* Processes and filters call site frames to find the best location for a vulnerability.
21+
* Returns client frames if available, otherwise falls back to all processed frames.
22+
* Excludes dd-trace frames and all Node.js built-in/internal modules (node:*).
23+
* @param {CallSiteFrame[]} callSiteFrames
24+
* @param {string[]} externallyExcludedPaths
25+
* @returns {CallSiteFrame[]} Client frames if available, otherwise all processed frames
26+
*
27+
* @typedef {object} CallSiteFrame
28+
* @property {number} id
29+
* @property {string} file - Original file path
30+
* @property {number} line
31+
* @property {number} column
32+
* @property {string} function
33+
* @property {string} class_name
34+
* @property {boolean} isNative
35+
* @property {string} [path] - Relative path, added during processing
36+
* @property {boolean} [isInternal] - Whether the frame is internal, added during processing
37+
*/
38+
function getCallSiteFramesForLocation (callSiteFrames, externallyExcludedPaths) {
2939
if (!callSiteFrames) {
3040
return []
3141
}
3242

33-
const result = []
43+
const allFrames = []
44+
const clientFrames = []
3445

3546
for (const callsite of callSiteFrames) {
3647
let filepath = callsite.file?.startsWith('file://') ? fileURLToPath(callsite.file) : callsite.file
@@ -47,18 +58,22 @@ function getNonDDCallSiteFrames (callSiteFrames, externallyExcludedPaths) {
4758
callsite.column = column
4859
}
4960

50-
if (
51-
!isExcluded(callsite, externallyExcludedPaths) &&
52-
(!filepath.includes(pathLine.ddBasePath) || globalThis.__DD_ESBUILD_IAST_WITH_NO_SM)
53-
) {
61+
if (filepath) {
5462
callsite.path = getRelativePath(filepath)
5563
callsite.isInternal = !path.isAbsolute(filepath)
5664

57-
result.push(callsite)
65+
allFrames.push(callsite)
66+
67+
if (
68+
!isExcluded(callsite, externallyExcludedPaths) &&
69+
(!filepath.includes(pathLine.ddBasePath) || globalThis.__DD_ESBUILD_IAST_WITH_NO_SM)
70+
) {
71+
clientFrames.push(callsite)
72+
}
5873
}
5974
}
6075

61-
return result
76+
return clientFrames.length > 0 ? clientFrames : allFrames
6277
}
6378

6479
function getRelativePath (filepath) {
@@ -67,10 +82,12 @@ function getRelativePath (filepath) {
6782

6883
function isExcluded (callsite, externallyExcludedPaths) {
6984
if (callsite.isNative) return true
85+
7086
const filename = globalThis.__DD_ESBUILD_IAST_WITH_SM ? callsite.path : callsite.file
71-
if (!filename) {
87+
if (!filename || filename.startsWith('node:')) {
7288
return true
7389
}
90+
7491
let excludedPaths = EXCLUDED_PATHS
7592
if (externallyExcludedPaths) {
7693
excludedPaths = [...excludedPaths, ...externallyExcludedPaths]
@@ -82,12 +99,6 @@ function isExcluded (callsite, externallyExcludedPaths) {
8299
}
83100
}
84101

85-
for (const EXCLUDED_PATH_PREFIX of EXCLUDED_PATH_PREFIXES) {
86-
if (filename.indexOf(EXCLUDED_PATH_PREFIX) === 0) {
87-
return true
88-
}
89-
}
90-
91102
return false
92103
}
93104

‎packages/dd-trace/test/appsec/iast/path-line.spec.js‎

Lines changed: 22 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ const path = require('path')
66

77
const proxyquire = require('proxyquire')
88

9-
const { getCallsiteFrames } = require('../../../src/appsec/stack_trace')
109
class CallSiteMock {
1110
constructor (fileName, lineNumber, columnNumber = 0) {
1211
this.file = fileName
@@ -26,10 +25,12 @@ describe('path-line', function () {
2625
const firstSep = tmpdir.indexOf(path.sep)
2726
const rootPath = tmpdir.slice(0, firstSep + 1)
2827

29-
const DIAGNOSTICS_CHANNEL_PATHS = [
28+
const EXCLUDED_TEST_PATHS = [
3029
path.join(rootPath, 'path', 'node_modules', 'dc-polyfill'),
3130
'node:diagnostics_channel',
32-
'diagnostics_channel',
31+
'node:fs',
32+
'node:events',
33+
'node:internal/async_local_storage',
3334
]
3435
let mockPath, pathLine, mockProcess
3536

@@ -42,20 +43,20 @@ describe('path-line', function () {
4243
})
4344
})
4445

45-
describe('getNonDDCallSiteFrames', () => {
46+
describe('getCallSiteFramesForLocation', () => {
4647
describe('does not fail', () => {
4748
it('with null parameter', () => {
48-
const result = pathLine.getNonDDCallSiteFrames(null)
49+
const result = pathLine.getCallSiteFramesForLocation(null)
4950
assert.deepStrictEqual(result, [])
5051
})
5152

5253
it('with empty list parameter', () => {
53-
const result = pathLine.getNonDDCallSiteFrames([])
54+
const result = pathLine.getCallSiteFramesForLocation([])
5455
assert.deepStrictEqual(result, [])
5556
})
5657

5758
it('without parameter', () => {
58-
const result = pathLine.getNonDDCallSiteFrames()
59+
const result = pathLine.getCallSiteFramesForLocation()
5960
assert.deepStrictEqual(result, [])
6061
})
6162
})
@@ -91,7 +92,7 @@ describe('path-line', function () {
9192
callsites.push(new CallSiteMock(firstFileOutOfDD, 13, 42))
9293
callsites.push(new CallSiteMock(secondFileOutOfDD, 20, 15))
9394

94-
const results = pathLine.getNonDDCallSiteFrames(callsites)
95+
const results = pathLine.getCallSiteFramesForLocation(callsites)
9596

9697
assert.strictEqual(results.length, 2)
9798

@@ -104,17 +105,19 @@ describe('path-line', function () {
104105
assert.strictEqual(results[1].column, 15)
105106
})
106107

107-
it('should return an empty array when all stack frames are in dd trace', () => {
108+
it('should fallback to all processed frames when all stack frames are in dd trace', () => {
108109
const callsites = []
109110
callsites.push(new CallSiteMock(PATH_AND_LINE_PATH, PATH_AND_LINE_LINE))
110111
callsites.push(new CallSiteMock(path.join(DD_BASE_PATH, 'other', 'file', 'in', 'dd.js'), 89))
111112
callsites.push(new CallSiteMock(path.join(DD_BASE_PATH, 'another', 'file', 'in', 'dd.js'), 5))
112113

113-
const results = pathLine.getNonDDCallSiteFrames(callsites)
114-
assert.deepStrictEqual(results, [])
114+
const results = pathLine.getCallSiteFramesForLocation(callsites)
115+
116+
assert.strictEqual(results.length, 3)
117+
assert.ok(results.every(r => r.path && typeof r.isInternal === 'boolean'))
115118
})
116119

117-
DIAGNOSTICS_CHANNEL_PATHS.forEach((dcPath) => {
120+
EXCLUDED_TEST_PATHS.forEach((dcPath) => {
118121
it(`should exclude ${dcPath} from the results`, () => {
119122
const callsites = []
120123
const expectedFilePath = path.join('first', 'file', 'out', 'of', 'dd.js')
@@ -125,7 +128,7 @@ describe('path-line', function () {
125128
callsites.push(new CallSiteMock(dcPath, 25))
126129
callsites.push(new CallSiteMock(firstFileOutOfDD, 13, 42))
127130

128-
const results = pathLine.getNonDDCallSiteFrames(callsites)
131+
const results = pathLine.getCallSiteFramesForLocation(callsites)
129132
assert.strictEqual(results.length, 1)
130133

131134
assert.strictEqual(results[0].path, expectedFilePath)
@@ -167,7 +170,7 @@ describe('path-line', function () {
167170
callsites.push(new CallSiteMock(firstFileOutOfDD, 13, 42))
168171
callsites.push(new CallSiteMock(secondFileOutOfDD, 20, 15))
169172

170-
const results = pathLine.getNonDDCallSiteFrames(callsites)
173+
const results = pathLine.getCallSiteFramesForLocation(callsites)
171174
assert.strictEqual(results.length, 2)
172175

173176
assert.strictEqual(results[0].path, expectedFilePaths[0])
@@ -230,7 +233,7 @@ describe('path-line', function () {
230233
callsites.push(new CallSiteMock(path.join(PROJECT_PATH, bundleOutFile), 3))
231234
callsites.push(new CallSiteMock(path.join(PROJECT_PATH, bundleOutFile), 4, 71))
232235

233-
const results = pathLine.getNonDDCallSiteFrames(callsites)
236+
const results = pathLine.getCallSiteFramesForLocation(callsites)
234237
assert.strictEqual(results.length, 2)
235238

236239
assert.strictEqual(results[0].path, 'file.js')
@@ -275,7 +278,7 @@ describe('path-line', function () {
275278
callsites.push(new CallSiteMock(path.join(OUT_BUILD_PATH, bundleOutFile), 3, 14))
276279
callsites.push(new CallSiteMock(path.join(OUT_BUILD_PATH, bundleOutFile), 2, 71))
277280

278-
const results = pathLine.getNonDDCallSiteFrames(callsites)
281+
const results = pathLine.getCallSiteFramesForLocation(callsites)
279282
assert.strictEqual(results.length, 4)
280283

281284
assert.strictEqual(results[0].path, bundleOutFile)
@@ -297,35 +300,12 @@ describe('path-line', function () {
297300
})
298301

299302
describe('getNodeModulesPaths', () => {
300-
function getCallSiteInfo () {
301-
const previousPrepareStackTrace = Error.prepareStackTrace
302-
const previousStackTraceLimit = Error.stackTraceLimit
303-
let callsiteList
304-
Error.stackTraceLimit = 100
305-
Error.prepareStackTrace = function (_, callsites) {
306-
callsiteList = callsites
307-
}
308-
const e = new Error()
309-
e.stack
310-
Error.prepareStackTrace = previousPrepareStackTrace
311-
Error.stackTraceLimit = previousStackTraceLimit
312-
313-
return callsiteList
314-
}
315-
316303
it('should handle windows paths correctly', () => {
317-
const basePath = pathLine.ddBasePath
318-
pathLine.ddBasePath = path.join('test', 'base', 'path')
319-
320-
const list = getCallsiteFrames(32, getCallSiteInfo, getCallSiteInfo)
321-
const firstNonDDPath = pathLine.getNonDDCallSiteFrames(list)[0]
322-
323-
const expectedPath = path.join('node_modules', firstNonDDPath.path)
324-
const nodeModulesPaths = pathLine.getNodeModulesPaths(firstNonDDPath.path)
304+
const testPath = 'some/nested/path/file.js'
305+
const expectedPath = path.join('node_modules', 'some', 'nested', 'path', 'file.js')
306+
const nodeModulesPaths = pathLine.getNodeModulesPaths(testPath)
325307

326308
assert.strictEqual(nodeModulesPaths[0], expectedPath)
327-
328-
pathLine.ddBasePath = basePath
329309
})
330310

331311
it('should convert / to \\ in windows platforms', () => {

0 commit comments

Comments
 (0)