fix(instrumentation): wrap lazily defined fs.opendir on Node 20#9094
Merged
Conversation
Contributor
Overall package sizeSelf size: 6.4 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.2.0 | 104.26 kB | 843.44 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9094 +/- ##
=======================================
Coverage 93.69% 93.69%
=======================================
Files 889 889
Lines 50856 50858 +2
Branches 11830 11829 -1
=======================================
+ Hits 47647 47650 +3
+ Misses 3209 3208 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0b0f79c | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-06-27 15:21:26 Comparing candidate commit 0b0f79c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2246 metrics, 40 unstable metrics.
|
BridgeAR
force-pushed
the
BridgeAR/2026-06-26-fix-iast-opendir-node20
branch
2 times, most recently
from
June 26, 2026 17:55
effe9cf to
2535576
Compare
Node 20 defines `fs.opendir` / `fs.opendirSync` as lazy getter+setter accessor
properties that resolve the real function on first read. Handing such a property
to `shimmer.wrap` instrumented the getter, so the property access was traced
while the real call ran uninstrumented — IAST then saw no `opendir` operation
and reported no PATH_TRAVERSAL vulnerability. Node 18/22 define these as plain
data properties, which is why the gap was Node-20-only.
Extend `shimmer.wrap`'s existing `replaceGetter` option to cover the lazy
getter+setter case: resolve the value once through the getter and wrap that,
rather than re-implementing the resolution in the fs instrumentation. The
property keeps its original shape — a getter+setter pair stays a getter+setter
pair whose setter still materializes a writable data property on assignment, so
the descriptor remains observationally identical for a downstream consumer that
inspects or overwrites it on that Node.js version.
A getter+setter pair without `replaceGetter` keeps being wrapped in place — the
wrapper becomes the new getter and the original setter is left untouched, as the
`url` instrumentation relies on for the `URL.prototype` `host` / `hostname`
accessors. Only a setter-only property throws. Narrowing the guard to reject
every unguarded getter+setter pair would have thrown inside the `url` hook,
silently dropping that instrumentation and the AppSec / IAST coverage built on
it.
`fs.js` now passes `{ replaceGetter: true }` through its `wrap` / `massWrap`
helpers instead of carrying its own materialization helper.
BridgeAR
force-pushed
the
BridgeAR/2026-06-26-fix-iast-opendir-node20
branch
from
June 27, 2026 15:08
2535576 to
0b0f79c
Compare
BridgeAR
marked this pull request as ready for review
June 27, 2026 15:51
szegedi
approved these changes
Jun 30, 2026
| } | ||
|
|
||
| const original = descriptor.value ?? options?.replaceGetter ? target[name] : descriptor.get | ||
| const original = (descriptor.value ?? options?.replaceGetter) ? target[name] : descriptor.get |
Contributor
There was a problem hiding this comment.
This looks cosmetic, but I guess helps comprehension, so let's leave it.
dd-octo-sts Bot
pushed a commit
that referenced
this pull request
Jul 1, 2026
Node 20 defines `fs.opendir` / `fs.opendirSync` as lazy getter+setter accessor
properties that resolve the real function on first read. Handing such a property
to `shimmer.wrap` instrumented the getter, so the property access was traced
while the real call ran uninstrumented — IAST then saw no `opendir` operation
and reported no PATH_TRAVERSAL vulnerability. Node 18/22 define these as plain
data properties, which is why the gap was Node-20-only.
Extend `shimmer.wrap`'s existing `replaceGetter` option to cover the lazy
getter+setter case: resolve the value once through the getter and wrap that,
rather than re-implementing the resolution in the fs instrumentation. The
property keeps its original shape — a getter+setter pair stays a getter+setter
pair whose setter still materializes a writable data property on assignment, so
the descriptor remains observationally identical for a downstream consumer that
inspects or overwrites it on that Node.js version.
A getter+setter pair without `replaceGetter` keeps being wrapped in place — the
wrapper becomes the new getter and the original setter is left untouched, as the
`url` instrumentation relies on for the `URL.prototype` `host` / `hostname`
accessors. Only a setter-only property throws. Narrowing the guard to reject
every unguarded getter+setter pair would have thrown inside the `url` hook,
silently dropping that instrumentation and the AppSec / IAST coverage built on
it.
`fs.js` now passes `{ replaceGetter: true }` through its `wrap` / `massWrap`
helpers instead of carrying its own materialization helper.
rochdev
pushed a commit
that referenced
this pull request
Jul 2, 2026
Node 20 defines `fs.opendir` / `fs.opendirSync` as lazy getter+setter accessor
properties that resolve the real function on first read. Handing such a property
to `shimmer.wrap` instrumented the getter, so the property access was traced
while the real call ran uninstrumented — IAST then saw no `opendir` operation
and reported no PATH_TRAVERSAL vulnerability. Node 18/22 define these as plain
data properties, which is why the gap was Node-20-only.
Extend `shimmer.wrap`'s existing `replaceGetter` option to cover the lazy
getter+setter case: resolve the value once through the getter and wrap that,
rather than re-implementing the resolution in the fs instrumentation. The
property keeps its original shape — a getter+setter pair stays a getter+setter
pair whose setter still materializes a writable data property on assignment, so
the descriptor remains observationally identical for a downstream consumer that
inspects or overwrites it on that Node.js version.
A getter+setter pair without `replaceGetter` keeps being wrapped in place — the
wrapper becomes the new getter and the original setter is left untouched, as the
`url` instrumentation relies on for the `URL.prototype` `host` / `hostname`
accessors. Only a setter-only property throws. Narrowing the guard to reject
every unguarded getter+setter pair would have thrown inside the `url` hook,
silently dropping that instrumentation and the AppSec / IAST coverage built on
it.
`fs.js` now passes `{ replaceGetter: true }` through its `wrap` / `massWrap`
helpers instead of carrying its own materialization helper.
rochdev
pushed a commit
that referenced
this pull request
Jul 2, 2026
Node 20 defines `fs.opendir` / `fs.opendirSync` as lazy getter+setter accessor
properties that resolve the real function on first read. Handing such a property
to `shimmer.wrap` instrumented the getter, so the property access was traced
while the real call ran uninstrumented — IAST then saw no `opendir` operation
and reported no PATH_TRAVERSAL vulnerability. Node 18/22 define these as plain
data properties, which is why the gap was Node-20-only.
Extend `shimmer.wrap`'s existing `replaceGetter` option to cover the lazy
getter+setter case: resolve the value once through the getter and wrap that,
rather than re-implementing the resolution in the fs instrumentation. The
property keeps its original shape — a getter+setter pair stays a getter+setter
pair whose setter still materializes a writable data property on assignment, so
the descriptor remains observationally identical for a downstream consumer that
inspects or overwrites it on that Node.js version.
A getter+setter pair without `replaceGetter` keeps being wrapped in place — the
wrapper becomes the new getter and the original setter is left untouched, as the
`url` instrumentation relies on for the `URL.prototype` `host` / `hostname`
accessors. Only a setter-only property throws. Narrowing the guard to reject
every unguarded getter+setter pair would have thrown inside the `url` hook,
silently dropping that instrumentation and the AppSec / IAST coverage built on
it.
`fs.js` now passes `{ replaceGetter: true }` through its `wrap` / `massWrap`
helpers instead of carrying its own materialization helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Node 20,
fs.opendirandfs.opendirSyncare lazy get+set accessor properties: the getter resolves the real function (and self-replaces with a data property) on first read. The fs instrumentation mass-wrapped these names withshimmer.wrap, which received the getter and instrumented the property access (get opendir) instead of the method. The real call then ran uninstrumented,apm:fs:operation:startnever fired with the directory path, and IAST reported noPATH_TRAVERSALvulnerability (and RASP no LFI) forfs.opendir/opendirSyncrequests. Node 18 and 22 define both as plain data properties, so the operation was lost only on Node 20 — surfacing as an intermittent failure of thefs.opendir/fs.opendirSynccases inpath-traversal-analyzer.spec.js.shimmer.wrap'sreplaceGetteroption now also handles a getter+setter accessor: it resolves the value once through the getter and redefines the property as a wrapped data property, dropping the accessor. It previously threw for a get+set pair.replaceGetterwhen wrappingfsmethods, so the lazy accessors resolve to the real function. A no-op for the data properties on Node 18 and 22.Test plan
packages/datadog-shimmer/test/shimmer.spec.jsgains a case for the getter+setterreplaceGetterpath (asserts the property is redefined as a wrapped data property) and one for the getter+setter rejection without the option. The first fails on the pre-fix shimmer.packages/datadog-instrumentations/test/fs.spec.jsassertsfs.opendir/opendirSyncare wrapped as methods (not accessors) and thatapm:fs:operation:startpublishes the method's own operation and path. Both fail onmasterunder Node 20.packages/dd-trace/test/appsec/iast/analyzers/path-traversal-analyzer.spec.js— the opendir cases now pass deterministically (10 consecutive runs on Node 20).fs-plugin/lfispecs.