Skip to content

Commit 00161cd

Browse files
committed
fix(debugger): limit and safely stringify template log message values
Debugger log templates previously used util.inspect directly, which could stringify objects with unbounded property counts and invoke user code via prototype-chain proxy traps and Symbol.toStringTag getters. Add inspectSegment to cap enumerable object properties at five, omit values whose inspection may execute user code, and render direct proxies as [Proxy] for predictable output. Wire the helper into the devtools client via a dd-trace global so template expressions use the safe formatter.
1 parent 525555b commit 00161cd

9 files changed

Lines changed: 248 additions & 43 deletions

File tree

integration-tests/debugger/target-app/template.js

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ fastify.get('/:name', function (request) {
2424
const emptyArr = []
2525
const arr = [{ a: 1 }, 2, 3, 4, 5]
2626
const emptyObj = {}
27+
const maxObj = { a: 1, b: 2, c: 3, d: 4, e: 5 }
28+
Object.defineProperty(maxObj, 'hidden', { value: 6 })
2729
const obj = {
2830
foo: {
2931
baz: 42,
@@ -35,6 +37,8 @@ fastify.get('/:name', function (request) {
3537
get baz () {
3638
return 'This is a getter!'
3739
},
40+
qux: 42,
41+
quux: false,
3842
[inspect.custom] () {
3943
return 'This is a custom inspect!'
4044
},
@@ -44,8 +48,31 @@ fastify.get('/:name', function (request) {
4448
return 'This is a proxy!'
4549
},
4650
})
51+
const objectWithProxyPrototype = Object.create(new Proxy({}, {
52+
getPrototypeOf () {
53+
throw new Error('Proxy prototype trap should not run')
54+
},
55+
}))
56+
Object.defineProperties(objectWithProxyPrototype, {
57+
a: { value: 1, enumerable: true },
58+
b: { value: 2, enumerable: true },
59+
c: { value: 3, enumerable: true },
60+
d: { value: 4, enumerable: true },
61+
e: { value: 5, enumerable: true },
62+
f: { value: 6, enumerable: true },
63+
})
64+
const sideEffectfulObject = {
65+
a: 1,
66+
b: 2,
67+
get [Symbol.toStringTag] () {
68+
throw new Error('Symbol.toStringTag getter should not run')
69+
},
70+
[Symbol('extra')]: 4,
71+
}
4772
const circular = {}
4873
circular.circular = circular
74+
const wideCircular = { circular: undefined, a: 1, b: 2, c: 3, d: 4, e: 5 }
75+
wideCircular.circular = wideCircular
4976
const ins = new CustomClass()
5077
const p = Promise.resolve(42)
5178
const arrowFn = () => {}
@@ -80,6 +107,10 @@ class CustomClass {
80107

81108
constructor () {
82109
this.c = 3
110+
this.d = 4
111+
this.e = 5
112+
this.f = 6
113+
this.g = 7
83114
}
84115

85116
get [Symbol.toStringTag] () {

integration-tests/debugger/template.spec.js

Lines changed: 21 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -48,27 +48,22 @@ describe('Dynamic Instrumentation', function () {
4848
assert.strictEqual(messages.shift(), '[]')
4949
assert.strictEqual(messages.shift(), '[ [Object], 2, 3, ... 2 more items ]')
5050
assert.strictEqual(messages.shift(), '{}')
51+
assert.strictEqual(messages.shift(), '{ a: 1, b: 2, c: 3, d: 4, e: 5 }')
5152
const obj = messages.shift()
52-
let expectedObjectShape = '{ ' +
53-
'foo: [Object], ' +
54-
'bar: true, ' +
55-
'baz: [Getter], ' +
56-
(NODE_MAJOR >= 24
57-
? 'Symbol(nodejs.util.inspect.custom): [Function: [nodejs.util.inspect.custom]] '
58-
: '[Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]] ') +
53+
const expectedObjectShape = '{ ' +
54+
'foo: [Object], bar: true, baz: [Getter], qux: 42, quux: false, ... 1 more property ' +
5955
'}'
6056
assert.strictEqual(obj, expectedObjectShape)
61-
if (NODE_MAJOR >= 26) {
62-
// A proxy should be stringified to the wrapped object plus the proxy type in newer Node.js versions
63-
expectedObjectShape = `Proxy(${expectedObjectShape})`
64-
}
65-
assert.strictEqual(messages.shift(), expectedObjectShape)
57+
assert.strictEqual(messages.shift(), '[Proxy]')
58+
assert.strictEqual(messages.shift(), '{ a: 1, b: 2, c: 3, d: 4, e: 5, ... 1 more property }')
59+
assert.strictEqual(messages.shift(), '[Value omitted: inspection may execute user code]')
6660
assert.strictEqual(messages.shift(), '<ref *1> { circular: [Circular *1] }')
61+
assert.strictEqual(
62+
messages.shift(),
63+
'<ref *1> { circular: [Circular *1], a: 1, b: 2, c: 3, d: 4, ... 1 more property }'
64+
)
6765
assert.strictEqual(messages.shift(), '[class CustomClass]')
68-
// Notice execution of `Symbol.toStringTag` getter (`foo`). There's nothing we can do about it when using
69-
// `util.inspect`, but it has not been considered a big side-effects issue, as anyone implementing this
70-
// function is doing so with the explicit intent of modifying the string representation of instances.
71-
assert.strictEqual(messages.shift(), 'CustomClass [foo] { b: 2, c: 3 }')
66+
assert.strictEqual(messages.shift(), '{ b: 2, c: 3, d: 4, e: 5, f: 6, ... 1 more property }')
7267
if (NODE_MAJOR >= 24) {
7368
assert.strictEqual(messages.shift(), 'Promise { 42 }')
7469
} else {
@@ -83,18 +78,8 @@ describe('Dynamic Instrumentation', function () {
8378
}
8479
assert.strictEqual(messages.shift(), '[Function: arrowFn]')
8580
assert.strictEqual(messages.shift(), '[Function: fn]')
86-
assert.strictEqual(
87-
messages.shift(),
88-
NODE_MAJOR > 18
89-
? 'Set(5) { 1, 2, 3, ... 2 more items }'
90-
: 'Set(5) { 1, 2, 3, 4, 5 }'
91-
)
92-
assert.strictEqual(
93-
messages.shift(),
94-
NODE_MAJOR > 18
95-
? 'Map(5) { 1 => 2, 3 => 4, 5 => 6, ... 2 more items }'
96-
: 'Map(5) { 1 => 2, 3 => 4, 5 => 6, 7 => 8, 9 => 10 }'
97-
)
81+
assert.strictEqual(messages.shift(), 'Set(5) { 1, 2, 3, ... 2 more items }')
82+
assert.strictEqual(messages.shift(), 'Map(5) { 1 => 2, 3 => 4, 5 => 6, ... 2 more items }')
9883
assert.strictEqual(messages.shift(), 'WeakSet { <items unknown> }')
9984
assert.strictEqual(messages.shift(), 'WeakMap { <items unknown> }')
10085
assert.strictEqual(messages.shift(), 'Buffer(6) [Uint8Array] [ 102, 111, 111, ... 3 more items ]')
@@ -138,12 +123,20 @@ describe('Dynamic Instrumentation', function () {
138123
{ str: ';' },
139124
{ dsl: 'emptyObj', json: { ref: 'emptyObj' } },
140125
{ str: ';' },
126+
{ dsl: 'maxObj', json: { ref: 'maxObj' } },
127+
{ str: ';' },
141128
{ dsl: 'obj', json: { ref: 'obj' } },
142129
{ str: ';' },
143130
{ dsl: 'proxy', json: { ref: 'proxy' } },
144131
{ str: ';' },
132+
{ dsl: 'objectWithProxyPrototype', json: { ref: 'objectWithProxyPrototype' } },
133+
{ str: ';' },
134+
{ dsl: 'sideEffectfulObject', json: { ref: 'sideEffectfulObject' } },
135+
{ str: ';' },
145136
{ dsl: 'circular', json: { ref: 'circular' } },
146137
{ str: ';' },
138+
{ dsl: 'wideCircular', json: { ref: 'wideCircular' } },
139+
{ str: ';' },
147140
{ dsl: 'CustomClass', json: { ref: 'CustomClass' } },
148141
{ str: ';' },
149142
{ dsl: 'ins', json: { ref: 'ins' } },

packages/dd-trace/src/debugger/constants.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,5 @@ module.exports = {
44
DEBUGGER_DIAGNOSTICS_V1: '/debugger/v1/diagnostics',
55
DEBUGGER_INPUT_V1: '/debugger/v1/input',
66
DEBUGGER_INPUT_V2: '/debugger/v2/input',
7+
INSPECT_SEGMENT_GLOBAL_PROPERTY: 'debuggerInspectSegment',
78
}

packages/dd-trace/src/debugger/devtools_client/condition.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ function compileSegments (segments) {
5555
? `(() => {
5656
try {
5757
const result = ${compile(json)}
58-
return typeof result === 'string' ? result : $dd_inspect(result, $dd_segmentInspectOptions)
58+
return typeof result === 'string' ? result : $dd_inspectSegment(result)
5959
} catch (e) {
6060
return { expr: ${JSON.stringify(dsl)}, message: \`\${e.name}: \${e.message}\` }
6161
}

packages/dd-trace/src/debugger/devtools_client/index.js

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const { randomUUID } = require('crypto')
44
const { workerData: { probeSamplerBuffer } } = require('worker_threads')
55
const { version } = require('../../../../../package.json')
66
const processTags = require('../../process-tags')
7+
const { INSPECT_SEGMENT_GLOBAL_PROPERTY } = require('../constants')
78
const {
89
MAX_SAMPLED_PROBES_PER_PAUSE,
910
SAMPLED_PROBE_COUNT_INDEX,
@@ -23,16 +24,8 @@ require('./remote_config')
2324

2425
/** @typedef {import('node:inspector').Debugger.EvaluateOnCallFrameReturnType} EvaluateOnCallFrameResult */
2526

26-
const templateExpressionSetupCode = `
27-
const $dd_inspect = global.require('node:util').inspect;
28-
const $dd_segmentInspectOptions = {
29-
depth: 0,
30-
customInspect: false,
31-
maxArrayLength: 3,
32-
maxStringLength: 8 * 1024,
33-
breakLength: Infinity
34-
};
35-
`
27+
const templateExpressionSetupCode = 'const $dd_inspectSegment = ' +
28+
`globalThis[Symbol.for('dd-trace')][${JSON.stringify(INSPECT_SEGMENT_GLOBAL_PROPERTY)}];`
3629

3730
// Expression to run on a call frame of the paused thread to get its active trace and span id.
3831
const getDDTagsExpression = `(() => {

packages/dd-trace/src/debugger/index.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ const { Worker, MessageChannel, threadId: parentThreadId } = require('worker_thr
77
const log = require('../log')
88
const { fetchAgentInfo } = require('../agent/info')
99
const getDebuggerConfig = require('./config')
10-
const { DEBUGGER_DIAGNOSTICS_V1, DEBUGGER_INPUT_V2 } = require('./constants')
10+
const { DEBUGGER_DIAGNOSTICS_V1, DEBUGGER_INPUT_V2, INSPECT_SEGMENT_GLOBAL_PROPERTY } = require('./constants')
1111
const { installProbeSampler, uninstallProbeSampler } = require('./probe_sampler')
1212

1313
/**
@@ -64,7 +64,9 @@ function start (config, rcInstance) {
6464
const logChannel = new MessageChannel()
6565
configChannel = new MessageChannel()
6666

67-
globalThis[Symbol.for('dd-trace')].utilTypes = types
67+
const debuggerGlobals = globalThis[Symbol.for('dd-trace')]
68+
debuggerGlobals.utilTypes = types
69+
debuggerGlobals[INSPECT_SEGMENT_GLOBAL_PROPERTY] = require('./inspect-segment')
6870

6971
const probeSamplerBuffer = installProbeSampler()
7072

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
'use strict'
2+
3+
const { inspect, types } = require('node:util')
4+
5+
/** @typedef {NonNullable<ReturnType<typeof globalThis.Object.getOwnPropertyDescriptor>>} PropertyDescriptor */
6+
7+
const maxProperties = 5
8+
const segmentInspectOptions = {
9+
depth: 0,
10+
customInspect: false,
11+
maxArrayLength: 3,
12+
maxStringLength: 8 * 1024,
13+
breakLength: Infinity,
14+
}
15+
16+
module.exports = inspectSegment
17+
18+
/**
19+
* Inspect a dynamic-instrumentation template value without invoking user code.
20+
* Unlike collections, `util.inspect` has no option for limiting the number of object properties, so this function
21+
* truncates objects before inspecting them.
22+
*
23+
* @param {unknown} value
24+
* @returns {string}
25+
*/
26+
function inspectSegment (value) {
27+
if (value === null || (typeof value !== 'object' && typeof value !== 'function')) {
28+
return inspect(value, segmentInspectOptions)
29+
}
30+
if (types.isProxy(value)) return '[Proxy]'
31+
if (
32+
Array.isArray(value) ||
33+
types.isTypedArray(value) ||
34+
types.isAnyArrayBuffer(value) ||
35+
types.isDataView(value) ||
36+
types.isMap(value) ||
37+
types.isSet(value) ||
38+
types.isWeakMap(value) ||
39+
types.isWeakSet(value) ||
40+
types.isMapIterator(value) ||
41+
types.isSetIterator(value)
42+
) {
43+
return inspect(value, segmentInspectOptions)
44+
}
45+
46+
/** @type {(string | symbol)[]} */
47+
const keys = Object.keys(value)
48+
let propertyCount = keys.length
49+
const symbols = Object.getOwnPropertySymbols(value)
50+
for (let i = 0; i < symbols.length; i++) {
51+
if (Object.getOwnPropertyDescriptor(value, symbols[i])?.enumerable === true) {
52+
propertyCount++
53+
if (keys.length < maxProperties) keys.push(symbols[i])
54+
}
55+
}
56+
57+
if (propertyCount <= maxProperties) {
58+
// TODO: Decide whether allowing util.inspect to invoke Symbol.toStringTag getters is acceptable. If it is,
59+
// remove inspectionCanRunUserCode and the related omission paths.
60+
if (inspectionCanRunUserCode(value)) {
61+
return '[Value omitted: inspection may execute user code]'
62+
}
63+
return inspect(value, segmentInspectOptions)
64+
}
65+
66+
const truncated = {}
67+
for (let i = 0; i < maxProperties; i++) {
68+
const descriptor = /** @type {PropertyDescriptor} */ (Object.getOwnPropertyDescriptor(value, keys[i]))
69+
if (
70+
(keys[i] === Symbol.toStringTag && descriptor.get !== undefined) ||
71+
(descriptor.value !== value && inspectionCanRunUserCode(descriptor.value))
72+
) {
73+
return '[Value omitted: inspection may execute user code]'
74+
}
75+
if (descriptor.value === value) descriptor.value = truncated
76+
Object.defineProperty(truncated, keys[i], descriptor)
77+
}
78+
79+
const omitted = propertyCount - maxProperties
80+
const inspected = inspect(truncated, segmentInspectOptions)
81+
return `${inspected.slice(0, -2)}, ... ${omitted} more ${omitted === 1 ? 'property' : 'properties'} }`
82+
}
83+
84+
/**
85+
* Determine whether inspecting a value could invoke a proxy trap or toStringTag getter.
86+
*
87+
* @param {unknown} value
88+
* @returns {boolean}
89+
*/
90+
function inspectionCanRunUserCode (value) {
91+
const type = typeof value
92+
if (value === null || (type !== 'object' && type !== 'function')) return false
93+
if (types.isProxy(value)) return true
94+
95+
let current = value
96+
while (current !== null) {
97+
if (Object.getOwnPropertyDescriptor(current, Symbol.toStringTag)?.get !== undefined) return true
98+
current = Object.getPrototypeOf(current)
99+
if (types.isProxy(current)) return true
100+
}
101+
return false
102+
}

packages/dd-trace/test/debugger/devtools_client/condition.spec.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ describe('Expression language', function () {
126126
`[(() => {
127127
try {
128128
const result = foo
129-
return typeof result === 'string' ? result : $dd_inspect(result, $dd_segmentInspectOptions)
129+
return typeof result === 'string' ? result : $dd_inspectSegment(result)
130130
} catch (e) {
131131
return { expr: "foo", message: \`\${e.name}: \${e.message}\` }
132132
}
@@ -146,7 +146,7 @@ describe('Expression language', function () {
146146
`["foo: ",(() => {
147147
try {
148148
const result = foo
149-
return typeof result === 'string' ? result : $dd_inspect(result, $dd_segmentInspectOptions)
149+
return typeof result === 'string' ? result : $dd_inspectSegment(result)
150150
} catch (e) {
151151
return { expr: "foo", message: \`\${e.name}: \${e.message}\` }
152152
}

0 commit comments

Comments
 (0)