fix(debugger): redact sensitive data in log probe messages - #9704
fix(debugger): redact sensitive data in log probe messages#9704p-datadog wants to merge 5 commits into
Conversation
Log probe template segments rendered EL expression results with
util.inspect and no redaction, so a template referencing an object or
map (e.g. {user}, {params}) interpolated every property/entry verbatim.
Sensitive identifiers (password, api_key, session, ...) leaked in
plaintext to the debugger intake, even though the snapshot path redacts
them.
Redact values whose key matches a redacted identifier before inspecting,
inside the code evaluated in the paused process. Objects and maps whose
own keys are all non-redacted are inspected unchanged, preserving the
existing message format.
Signed-off-by: ddsign <ddsign@bsdpower.com>
p-datadog
added
semver-patch
AI Generated
Overall package sizeSelf size: 7.85 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.3 | 125.43 kB | 441.68 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 #9704 +/- ##
==========================================
- Coverage 98.51% 98.46% -0.06%
==========================================
Files 963 963
Lines 135796 135877 +81
Branches 11984 11815 -169
==========================================
+ Hits 133785 133791 +6
- Misses 2011 2086 +75 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:
|
Signed-off-by: ddsign <ddsign@bsdpower.com>
|
@codex review |
…ient index.js now requires ./snapshot/redaction at load, which transitively requires ../config (worker-only). The onPause spec must pass the already config-stubbed redaction module to proxyquire so the real config, which reads null workerData in unit tests, is not loaded. Signed-off-by: ddsign <ddsign@bsdpower.com>
There was a problem hiding this comment.
Pull request overview
This PR updates Dynamic Instrumentation log-probe template rendering so that interpolated values (especially objects and Maps) honor the same sensitive-identifier redaction behavior already applied to snapshot capture, preventing plaintext secrets from being emitted in probe messages.
Changes:
- Adds a redaction-aware
$dd_inspectSegmentserializer injected into the paused-process evaluation code used for template segment rendering. - Updates template segment compilation to use
$dd_inspectSegmentinstead of directutil.inspectcalls. - Extends the debugger redaction integration test target app and adds an integration test asserting object/
Mapmessage redaction.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/dd-trace/test/debugger/devtools_client/condition.spec.js | Updates unit test expectations to use $dd_inspectSegment in compiled segment output. |
| packages/dd-trace/src/debugger/devtools_client/index.js | Injects redaction-aware segment inspection helpers into the paused-frame evaluation prelude. |
| packages/dd-trace/src/debugger/devtools_client/condition.js | Switches segment rendering to $dd_inspectSegment for consistent redaction behavior. |
| integration-tests/debugger/target-app/redact.js | Adds an object and Map containing a redacted key to exercise message redaction. |
| integration-tests/debugger/redact.spec.js | Adds an integration test verifying log-probe message redaction for object and Map interpolation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff7537cca8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenchmarksBenchmark execution time: 2026-08-05 20:48:50 Comparing candidate commit c3d58c7 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2321 metrics, 37 unstable metrics.
|
Address review comment: $dd_isRedactedIdentifier only handled string keys, so
symbol property names and symbol Map keys (e.g. Symbol('password')) were
interpolated verbatim into log probe messages while the snapshot path redacts
them via normalizeName(name, isSymbol).
- index.js: normalize symbol keys the same way as snapshot/redaction.js (strip
the Symbol() wrapper) before checking the redacted-identifier set
- target-app/redact.js: add a Symbol('password') key to obj and map
- redact.spec.js: assert the symbol-keyed value is redacted
Verified: eslint clean, debugger unit suite (618 passing), redact integration
test (3 passing).
Signed-off-by: ddsign <ddsign@bsdpower.com>
Addresses four review comments on the message-path redactor: - Detect Maps across realms: use util.types.isMap instead of instanceof, so a Map from node:vm is redacted rather than printed verbatim. - Bound the Map scan: only the first maxArrayLength entries are rendered, so scan at most that many keys and rebuild only when a rendered key is redacted, keeping the common large-Map case constant-time on the paused thread. - Render Proxy segments as [Proxy]: enumerating a proxy via Reflect.ownKeys runs its traps (customer code) in the paused frame, and letting util.inspect render the target would leak a proxy-wrapped secret. - Redact direct references: compileSegments threads the terminal identifier of a direct reference to $dd_inspectSegment, which redacts by name like the snapshot path. Computed/dynamic terminals have no static name and fall through. Tests: condition.spec.js identifier threading; redact.spec.js direct-ref, cross-realm Map, Proxy, and large-Map-window cases + target-app locals; template.spec.js proxy assertion updated to [Proxy]. Verified: eslint clean; debugger unit suite (619 passing); redact + template integration tests (11 passing). Signed-off-by: ddsign <ddsign@bsdpower.com>
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
integration-tests/debugger/redact.spec.js:84
- Expected object rendering should tolerate the bracketed vs non-bracketed symbol property key format across Node majors; otherwise this strict equality assertion will be version-fragile.
assert.strictEqual(
message,
"obj={ username: 'alice', password: '[redacted]', Symbol(password): '[redacted]' };" +
"map=Map(3) { 'username' => 'alice', 'password' => '[redacted]', Symbol(password) => '[redacted]' }"
)
packages/dd-trace/src/debugger/devtools_client/index.js:59
- The comment here says bounding the scan keeps the large-Map case constant-time, but when a redacted key is within the rendered window the code still clones the entire Map (O(n) in Map size). This is misleading and can understate pause-time cost for very large Maps; please clarify the comment (and consider whether an additional cap is needed).
// Only the first maxArrayLength entries are rendered, so scanning past that
// window cannot affect the output. Bounding the scan keeps the common
// large-Map case constant-time on the paused application thread.
integration-tests/debugger/redact.spec.js:6
- This spec currently hard-codes the symbol-key rendering shape (
Symbol(password): ...). Node’sutil.inspectoutput for symbol property keys differs across Node majors (see template.spec’s handling ofSymbol(nodejs.util.inspect.custom)), so this test may fail on older majors unless it gates on the runtime version.
This issue also appears on line 80 of the same file.
'use strict'
const assert = require('node:assert/strict')
const { once } = require('node:events')
const { assertObjectContains } = require('../helpers')
const { setup } = require('./utils')
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3d58c7be5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const key of value.keys()) { | ||
| if ($dd_index++ >= $dd_max) break; |
There was a problem hiding this comment.
On Node.js 18, util.inspect does not apply maxArrayLength to Maps—the existing expectation in integration-tests/debugger/template.spec.js explicitly renders all five entries on that runtime. Consequently, a Map whose fourth or later key is password passes this three-entry scan unchanged and emits the sensitive value. The redaction window must follow the runtime's actual Map rendering behavior rather than assuming the configured cap applies.
AGENTS.md reference: AGENTS.md:L179-L185
Useful? React with 👍 / 👎.
| maxStringLength: 8 * 1024, | ||
| breakLength: Infinity | ||
| }; | ||
| const $dd_redactedIdentifiers = new Set(${JSON.stringify([...REDACTED_IDENTIFIERS])}); |
There was a problem hiding this comment.
When the paused function has a local or parameter named Set, this unqualified constructor resolves to that application binding; for example, const Set = 'shadow' makes the entire setup throw TypeError: Set is not a constructor before any template result is produced. In that context every evaluated log probe at the pause is sent with an empty message, so the setup should obtain the intrinsic from an explicit global reference rather than the call frame's lexical scope.
Useful? React with 👍 / 👎.
| if ($dd_index++ >= $dd_max) break; | ||
| if ($dd_isRedactedIdentifier(key)) { $dd_hasRenderedRedactedKey = true; break; } | ||
| } | ||
| if (!$dd_hasRenderedRedactedKey) return value; |
There was a problem hiding this comment.
When a Map also has a sensitive own property, such as map.password = 'shh!', this branch examines only Map entry keys and returns the original value if none of the first entries are sensitive. util.inspect renders that property as Map(0) { password: 'shh!' }, so interpolating the Map still leaks it; Map values need their own string/symbol properties checked as well as their entries.
Useful? React with 👍 / 👎.
| const redacted = new Map(); | ||
| for (const [key, val] of value) redacted.set(key, $dd_isRedactedIdentifier(key) ? '[redacted]' : val); |
There was a problem hiding this comment.
The new bounded pre-scan still falls through to a full copy when a sensitive key occurs near the front: a million-entry Map whose first key is password causes this loop to iterate and allocate all million entries while the application thread is paused, although util.inspect renders only three on affected runtimes. This redacted-near-front case preserves the whole-Map pause cost from the earlier implementation, so redaction should avoid materializing entries outside the rendered window.
AGENTS.md reference: AGENTS.md:L194-L200
Useful? React with 👍 / 👎.
| return false; | ||
| }; | ||
| const $dd_redactSegmentValue = (value) => { | ||
| if (value === null || typeof value !== 'object') return value; |
There was a problem hiding this comment.
When a template interpolates a function with a sensitive own property, such as handler.password = 'shh!', this early return skips redaction because functions have typeof value === 'function'. util.inspect then renders [Function: handler] { password: 'shh!' }, so function-valued configuration objects can still leak secrets; functions need the same own-property redaction path as objects.
Useful? React with 👍 / 👎.
| const $dd_max = $dd_segmentInspectOptions.maxArrayLength; | ||
| let $dd_index = 0; | ||
| let $dd_hasRenderedRedactedKey = false; | ||
| for (const key of value.keys()) { |
There was a problem hiding this comment.
When an interpolated value is a Map subclass that overrides keys(), this call dispatches into application code while the frame is paused. An override that mutates state or throws therefore changes the application or turns the segment into an evaluation error, whereas util.inspect renders the same Map without calling the override; iterate with the intrinsic Map.prototype.keys instead.
Useful? React with 👍 / 👎.
What does this PR do?
Makes the Dynamic Instrumentation log-probe message path honor the same redaction the snapshot path applies.
Log probe template segments were rendered with
util.inspect($dd_inspect) and no redaction. A template that references an object or map (e.g.{user},{params}) interpolated every own property / entry verbatim, so sensitive identifiers (password,api_key,session, …) were emitted in plaintext to the debugger intake — even though the snapshot path (snapshot/processor.js) already redacts them.The fix injects a redaction-aware segment serializer into the code evaluated in the paused process:
$dd_redactSegmentValuereplaces a property/entry value with[redacted]when its key matches a redacted identifier, for plain objects, class instances, andMaps;util.inspectuntouched, so the existing message format (including[Object],[Getter], circular/proxy rendering) is preserved;Motivation
APMSP-3552 audited across all tracers - Node is the only one lacking redaction besides Ruby.
Additional Notes
Ruby fix: DataDog/dd-trace-rb#6155