Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7509 +/- ##
==========================================
- Coverage 80.24% 80.24% -0.01%
==========================================
Files 733 733
Lines 31663 31662 -1
==========================================
- Hits 25409 25407 -2
- Misses 6254 6255 +1 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overall package sizeSelf size: 4.84 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 816.75 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
Overall LGTM, just left some minor suggestions
| if (filename.startsWith('node:')) { | ||
| return true |
There was a problem hiding this comment.
Ideally, this is moved up into the !filename if statement with an OR. That way there is no additional work done while checking for that.
| } | ||
|
|
||
| return result | ||
| return clientFrames.length > 0 ? clientFrames : allFrames |
There was a problem hiding this comment.
Do we need to copy the frames for allFrames? Would it not be fine to just return callSiteFrames?
| return clientFrames.length > 0 ? clientFrames : allFrames | |
| return clientFrames.length > 0 ? clientFrames : callSiteFrames |
There was a problem hiding this comment.
We can't return callSiteFrames directly because allFrames is a subset of processed frames that have valid file information (only contains frames with a valid filepath - line 61 filter). Frames without filepath are excluded and wouldn't have the path/isInternal properties added.
| * @param {Array} callSiteFrames | ||
| * @param {Array} externallyExcludedPaths | ||
| * @returns {Array} Client frames if available, otherwise all processed frames |
There was a problem hiding this comment.
Nit: define the content of the arrays. That could be defined in the Node.js types already
| const frames = getCallSiteFramesForLocation(callSiteFrames, this._getExcludedPaths()) | ||
| const location = this._getLocation(value, frames) |
There was a problem hiding this comment.
Nit: are these underscored properties really meant as internally private? A customer would normally not get hold of them, if I am not mistaken, while we seem to use them across classes.
I am just commenting about it because it would be great to make truly internal methods truly private and keep others accessible (as long as they are not directly accessible for customers).
There was a problem hiding this comment.
No, they're not meant as internally private. _getLocation is overridden by several subclasses (e.g., CookieAnalyzer, HardcodedBaseAnalyzer, MissingHeaderAnalyzer) and some call super._getLocation()
There was a problem hiding this comment.
In that case we should not use the underscore, right?
There was a problem hiding this comment.
The underscore prefix here acts as "protected" in OOP terms: not for external/customer use, but accessible to internal subclasses. This is a consistent pattern across the codebase, not just in appsec:
- Profiling: Profiler defines _collect(), ServerlessProfiler overrides it and calls super._collect()
- Exporters: Writer base class calls
this._sendPayload(), implemented by AgentWriter, CoverageWriter, DynamicInstrumentationLogsWriter, and others
Changing this naming convention across the codebase would be a larger refactor outside the scope of this PR.
There was a problem hiding this comment.
I understand why it was used but we are currently moving away from that in most code.
Thus, when we touch something, it would be great to change it as well. In this particular case making them non-underscored would in my opinion be the way to go.
I marked it as nit, so it is not something that I expect to be done as part of the PR, while it would be nice.
CarlesDD
force-pushed
the
ccapell/iast-vuln-location-detection-improve-exculded
branch
from
ccf3e08 to
03854ff
Compare
BenchmarksBenchmark execution time: 2026-02-27 06:29:16 Comparing candidate commit 48d988d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 232 metrics, 28 unstable metrics. |
CarlesDD
deleted the
ccapell/iast-vuln-location-detection-improve-exculded
branch
…ng (#7509) * Improve callsite processing on IAST vuln location computing * Simplify excluding conditional * Specify array types in getCallSiteFramesForLocation JSDoc
…ng (#7509) * Improve callsite processing on IAST vuln location computing * Simplify excluding conditional * Specify array types in getCallSiteFramesForLocation JSDoc
3 participants
What does this PR do?
Simplifies IAST stack trace filtering by replacing the manually maintained
EXCLUDED_PATH_PREFIXESlist withfilename.startsWith('node:')to automatically exclude all Node.js built-in modules. Also adds fallback to all processed frames when no client frames are found.Motivation
Since dd-trace v5 only supports Node.js >= 18, and all built-in modules have the node: prefix in stack traces, we can exclude them automatically. This eliminates maintenance burden when new Node.js modules appear in stack traces.
APPSEC-61435