diff --git a/integration-tests/debugger/target-app/template.js b/integration-tests/debugger/target-app/template.js index f1c43cefb24..acd40f22677 100644 --- a/integration-tests/debugger/target-app/template.js +++ b/integration-tests/debugger/target-app/template.js @@ -24,6 +24,8 @@ fastify.get('/:name', function (request) { const emptyArr = [] const arr = [{ a: 1 }, 2, 3, 4, 5] const emptyObj = {} + const maxObj = { a: 1, b: 2, c: 3, d: 4, e: 5 } + Object.defineProperty(maxObj, 'hidden', { value: 6 }) const obj = { foo: { baz: 42, @@ -35,6 +37,8 @@ fastify.get('/:name', function (request) { get baz () { return 'This is a getter!' }, + qux: 42, + quux: false, [inspect.custom] () { return 'This is a custom inspect!' }, @@ -44,8 +48,31 @@ fastify.get('/:name', function (request) { return 'This is a proxy!' }, }) + const objectWithProxyPrototype = Object.create(new Proxy({}, { + getPrototypeOf () { + throw new Error('Proxy prototype trap should not run') + }, + })) + Object.defineProperties(objectWithProxyPrototype, { + a: { value: 1, enumerable: true }, + b: { value: 2, enumerable: true }, + c: { value: 3, enumerable: true }, + d: { value: 4, enumerable: true }, + e: { value: 5, enumerable: true }, + f: { value: 6, enumerable: true }, + }) + const sideEffectfulObject = { + a: 1, + b: 2, + get [Symbol.toStringTag] () { + throw new Error('Symbol.toStringTag getter should not run') + }, + [Symbol('extra')]: 4, + } const circular = {} circular.circular = circular + const wideCircular = { circular: undefined, a: 1, b: 2, c: 3, d: 4, e: 5 } + wideCircular.circular = wideCircular const ins = new CustomClass() const p = Promise.resolve(42) const arrowFn = () => {} @@ -80,6 +107,10 @@ class CustomClass { constructor () { this.c = 3 + this.d = 4 + this.e = 5 + this.f = 6 + this.g = 7 } get [Symbol.toStringTag] () { diff --git a/integration-tests/debugger/template.spec.js b/integration-tests/debugger/template.spec.js index 92481f42b4f..9282035a57c 100644 --- a/integration-tests/debugger/template.spec.js +++ b/integration-tests/debugger/template.spec.js @@ -48,27 +48,22 @@ describe('Dynamic Instrumentation', function () { assert.strictEqual(messages.shift(), '[]') assert.strictEqual(messages.shift(), '[ [Object], 2, 3, ... 2 more items ]') assert.strictEqual(messages.shift(), '{}') + assert.strictEqual(messages.shift(), '{ a: 1, b: 2, c: 3, d: 4, e: 5 }') const obj = messages.shift() - let expectedObjectShape = '{ ' + - 'foo: [Object], ' + - 'bar: true, ' + - 'baz: [Getter], ' + - (NODE_MAJOR >= 24 - ? 'Symbol(nodejs.util.inspect.custom): [Function: [nodejs.util.inspect.custom]] ' - : '[Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]] ') + + const expectedObjectShape = '{ ' + + 'foo: [Object], bar: true, baz: [Getter], qux: 42, quux: false, ... 1 more property ' + '}' assert.strictEqual(obj, expectedObjectShape) - if (NODE_MAJOR >= 26) { - // A proxy should be stringified to the wrapped object plus the proxy type in newer Node.js versions - expectedObjectShape = `Proxy(${expectedObjectShape})` - } - assert.strictEqual(messages.shift(), expectedObjectShape) + assert.strictEqual(messages.shift(), '[Proxy]') + assert.strictEqual(messages.shift(), '{ a: 1, b: 2, c: 3, d: 4, e: 5, ... 1 more property }') + assert.strictEqual(messages.shift(), '[Value omitted: inspection may execute user code]') assert.strictEqual(messages.shift(), ' { circular: [Circular *1] }') + assert.strictEqual( + messages.shift(), + ' { circular: [Circular *1], a: 1, b: 2, c: 3, d: 4, ... 1 more property }' + ) assert.strictEqual(messages.shift(), '[class CustomClass]') - // Notice execution of `Symbol.toStringTag` getter (`foo`). There's nothing we can do about it when using - // `util.inspect`, but it has not been considered a big side-effects issue, as anyone implementing this - // function is doing so with the explicit intent of modifying the string representation of instances. - assert.strictEqual(messages.shift(), 'CustomClass [foo] { b: 2, c: 3 }') + assert.strictEqual(messages.shift(), '{ b: 2, c: 3, d: 4, e: 5, f: 6, ... 1 more property }') if (NODE_MAJOR >= 24) { assert.strictEqual(messages.shift(), 'Promise { 42 }') } else { @@ -83,18 +78,8 @@ describe('Dynamic Instrumentation', function () { } assert.strictEqual(messages.shift(), '[Function: arrowFn]') assert.strictEqual(messages.shift(), '[Function: fn]') - assert.strictEqual( - messages.shift(), - NODE_MAJOR > 18 - ? 'Set(5) { 1, 2, 3, ... 2 more items }' - : 'Set(5) { 1, 2, 3, 4, 5 }' - ) - assert.strictEqual( - messages.shift(), - NODE_MAJOR > 18 - ? 'Map(5) { 1 => 2, 3 => 4, 5 => 6, ... 2 more items }' - : 'Map(5) { 1 => 2, 3 => 4, 5 => 6, 7 => 8, 9 => 10 }' - ) + assert.strictEqual(messages.shift(), 'Set(5) { 1, 2, 3, ... 2 more items }') + assert.strictEqual(messages.shift(), 'Map(5) { 1 => 2, 3 => 4, 5 => 6, ... 2 more items }') assert.strictEqual(messages.shift(), 'WeakSet { }') assert.strictEqual(messages.shift(), 'WeakMap { }') assert.strictEqual(messages.shift(), 'Buffer(6) [Uint8Array] [ 102, 111, 111, ... 3 more items ]') @@ -138,12 +123,20 @@ describe('Dynamic Instrumentation', function () { { str: ';' }, { dsl: 'emptyObj', json: { ref: 'emptyObj' } }, { str: ';' }, + { dsl: 'maxObj', json: { ref: 'maxObj' } }, + { str: ';' }, { dsl: 'obj', json: { ref: 'obj' } }, { str: ';' }, { dsl: 'proxy', json: { ref: 'proxy' } }, { str: ';' }, + { dsl: 'objectWithProxyPrototype', json: { ref: 'objectWithProxyPrototype' } }, + { str: ';' }, + { dsl: 'sideEffectfulObject', json: { ref: 'sideEffectfulObject' } }, + { str: ';' }, { dsl: 'circular', json: { ref: 'circular' } }, { str: ';' }, + { dsl: 'wideCircular', json: { ref: 'wideCircular' } }, + { str: ';' }, { dsl: 'CustomClass', json: { ref: 'CustomClass' } }, { str: ';' }, { dsl: 'ins', json: { ref: 'ins' } }, diff --git a/packages/dd-trace/src/debugger/constants.js b/packages/dd-trace/src/debugger/constants.js index 322713fb290..ccbf307d278 100644 --- a/packages/dd-trace/src/debugger/constants.js +++ b/packages/dd-trace/src/debugger/constants.js @@ -4,4 +4,5 @@ module.exports = { DEBUGGER_DIAGNOSTICS_V1: '/debugger/v1/diagnostics', DEBUGGER_INPUT_V1: '/debugger/v1/input', DEBUGGER_INPUT_V2: '/debugger/v2/input', + INSPECT_SEGMENT_GLOBAL_PROPERTY: 'debuggerInspectSegment', } diff --git a/packages/dd-trace/src/debugger/devtools_client/condition.js b/packages/dd-trace/src/debugger/devtools_client/condition.js index fa574726e9d..7b8859899aa 100644 --- a/packages/dd-trace/src/debugger/devtools_client/condition.js +++ b/packages/dd-trace/src/debugger/devtools_client/condition.js @@ -55,7 +55,7 @@ function compileSegments (segments) { ? `(() => { try { const result = ${compile(json)} - return typeof result === 'string' ? result : $dd_inspect(result, $dd_segmentInspectOptions) + return typeof result === 'string' ? result : $dd_inspectSegment(result) } catch (e) { return { expr: ${JSON.stringify(dsl)}, message: \`\${e.name}: \${e.message}\` } } diff --git a/packages/dd-trace/src/debugger/devtools_client/index.js b/packages/dd-trace/src/debugger/devtools_client/index.js index 643fd04bbe4..7f07e134b6c 100644 --- a/packages/dd-trace/src/debugger/devtools_client/index.js +++ b/packages/dd-trace/src/debugger/devtools_client/index.js @@ -4,6 +4,7 @@ const { randomUUID } = require('crypto') const { workerData: { probeSamplerBuffer } } = require('worker_threads') const { version } = require('../../../../../package.json') const processTags = require('../../process-tags') +const { INSPECT_SEGMENT_GLOBAL_PROPERTY } = require('../constants') const { MAX_SAMPLED_PROBES_PER_PAUSE, SAMPLED_PROBE_COUNT_INDEX, @@ -23,16 +24,8 @@ require('./remote_config') /** @typedef {import('node:inspector').Debugger.EvaluateOnCallFrameReturnType} EvaluateOnCallFrameResult */ -const templateExpressionSetupCode = ` - const $dd_inspect = global.require('node:util').inspect; - const $dd_segmentInspectOptions = { - depth: 0, - customInspect: false, - maxArrayLength: 3, - maxStringLength: 8 * 1024, - breakLength: Infinity - }; -` +const templateExpressionSetupCode = 'const $dd_inspectSegment = ' + + `globalThis[Symbol.for('dd-trace')][${JSON.stringify(INSPECT_SEGMENT_GLOBAL_PROPERTY)}];` // Expression to run on a call frame of the paused thread to get its active trace and span id. const getDDTagsExpression = `(() => { diff --git a/packages/dd-trace/src/debugger/index.js b/packages/dd-trace/src/debugger/index.js index 6a987bfe493..13224321675 100644 --- a/packages/dd-trace/src/debugger/index.js +++ b/packages/dd-trace/src/debugger/index.js @@ -7,7 +7,7 @@ const { Worker, MessageChannel, threadId: parentThreadId } = require('worker_thr const log = require('../log') const { fetchAgentInfo } = require('../agent/info') const getDebuggerConfig = require('./config') -const { DEBUGGER_DIAGNOSTICS_V1, DEBUGGER_INPUT_V2 } = require('./constants') +const { DEBUGGER_DIAGNOSTICS_V1, DEBUGGER_INPUT_V2, INSPECT_SEGMENT_GLOBAL_PROPERTY } = require('./constants') const { installProbeSampler, uninstallProbeSampler } = require('./probe_sampler') /** @@ -64,7 +64,9 @@ function start (config, rcInstance) { const logChannel = new MessageChannel() configChannel = new MessageChannel() - globalThis[Symbol.for('dd-trace')].utilTypes = types + const debuggerGlobals = globalThis[Symbol.for('dd-trace')] + debuggerGlobals.utilTypes = types + debuggerGlobals[INSPECT_SEGMENT_GLOBAL_PROPERTY] = require('./inspect-segment') const probeSamplerBuffer = installProbeSampler() diff --git a/packages/dd-trace/src/debugger/inspect-segment.js b/packages/dd-trace/src/debugger/inspect-segment.js new file mode 100644 index 00000000000..04bd14573c4 --- /dev/null +++ b/packages/dd-trace/src/debugger/inspect-segment.js @@ -0,0 +1,102 @@ +'use strict' + +const { inspect, types } = require('node:util') + +/** @typedef {NonNullable>} PropertyDescriptor */ + +const maxProperties = 5 +const segmentInspectOptions = { + depth: 0, + customInspect: false, + maxArrayLength: 3, + maxStringLength: 8 * 1024, + breakLength: Infinity, +} + +module.exports = inspectSegment + +/** + * Inspect a dynamic-instrumentation template value without invoking user code. + * Unlike collections, `util.inspect` has no option for limiting the number of object properties, so this function + * truncates objects before inspecting them. + * + * @param {unknown} value + * @returns {string} + */ +function inspectSegment (value) { + if (value === null || (typeof value !== 'object' && typeof value !== 'function')) { + return inspect(value, segmentInspectOptions) + } + if (types.isProxy(value)) return '[Proxy]' + if ( + Array.isArray(value) || + types.isTypedArray(value) || + types.isAnyArrayBuffer(value) || + types.isDataView(value) || + types.isMap(value) || + types.isSet(value) || + types.isWeakMap(value) || + types.isWeakSet(value) || + types.isMapIterator(value) || + types.isSetIterator(value) + ) { + return inspect(value, segmentInspectOptions) + } + + /** @type {(string | symbol)[]} */ + const keys = Object.keys(value) + let propertyCount = keys.length + const symbols = Object.getOwnPropertySymbols(value) + for (let i = 0; i < symbols.length; i++) { + if (Object.getOwnPropertyDescriptor(value, symbols[i])?.enumerable === true) { + propertyCount++ + if (keys.length < maxProperties) keys.push(symbols[i]) + } + } + + if (propertyCount <= maxProperties) { + // TODO: Decide whether allowing util.inspect to invoke Symbol.toStringTag getters is acceptable. If it is, + // remove inspectionCanRunUserCode and the related omission paths. + if (inspectionCanRunUserCode(value)) { + return '[Value omitted: inspection may execute user code]' + } + return inspect(value, segmentInspectOptions) + } + + const truncated = {} + for (let i = 0; i < maxProperties; i++) { + const descriptor = /** @type {PropertyDescriptor} */ (Object.getOwnPropertyDescriptor(value, keys[i])) + if ( + (keys[i] === Symbol.toStringTag && descriptor.get !== undefined) || + (descriptor.value !== value && inspectionCanRunUserCode(descriptor.value)) + ) { + return '[Value omitted: inspection may execute user code]' + } + if (descriptor.value === value) descriptor.value = truncated + Object.defineProperty(truncated, keys[i], descriptor) + } + + const omitted = propertyCount - maxProperties + const inspected = inspect(truncated, segmentInspectOptions) + return `${inspected.slice(0, -2)}, ... ${omitted} more ${omitted === 1 ? 'property' : 'properties'} }` +} + +/** + * Determine whether inspecting a value could invoke a proxy trap or toStringTag getter. + * + * @param {unknown} value + * @returns {boolean} + */ +function inspectionCanRunUserCode (value) { + const type = typeof value + if (value === null || (type !== 'object' && type !== 'function')) return false + if (types.isProxy(value)) return true + + let current = value + while (current !== null) { + if (Object.getOwnPropertyDescriptor(current, Symbol.toStringTag)?.get !== undefined) return true + current = Object.getPrototypeOf(current) + if (types.isProxy(current)) return true + } + return false +} diff --git a/packages/dd-trace/test/debugger/devtools_client/condition.spec.js b/packages/dd-trace/test/debugger/devtools_client/condition.spec.js index 8899adbbc4b..31af0db29b0 100644 --- a/packages/dd-trace/test/debugger/devtools_client/condition.spec.js +++ b/packages/dd-trace/test/debugger/devtools_client/condition.spec.js @@ -126,7 +126,7 @@ describe('Expression language', function () { `[(() => { try { const result = foo - return typeof result === 'string' ? result : $dd_inspect(result, $dd_segmentInspectOptions) + return typeof result === 'string' ? result : $dd_inspectSegment(result) } catch (e) { return { expr: "foo", message: \`\${e.name}: \${e.message}\` } } @@ -146,7 +146,7 @@ describe('Expression language', function () { `["foo: ",(() => { try { const result = foo - return typeof result === 'string' ? result : $dd_inspect(result, $dd_segmentInspectOptions) + return typeof result === 'string' ? result : $dd_inspectSegment(result) } catch (e) { return { expr: "foo", message: \`\${e.name}: \${e.message}\` } } diff --git a/packages/dd-trace/test/debugger/devtools_client/inspect-segment.spec.js b/packages/dd-trace/test/debugger/devtools_client/inspect-segment.spec.js new file mode 100644 index 00000000000..41ce1d9e24e --- /dev/null +++ b/packages/dd-trace/test/debugger/devtools_client/inspect-segment.spec.js @@ -0,0 +1,83 @@ +'use strict' + +const assert = require('node:assert/strict') +const { inspect } = require('node:util') + +const { describe, it } = require('mocha') + +require('../../setup/mocha') + +const inspectSegment = require('../../../src/debugger/inspect-segment') + +describe('inspectSegment', function () { + it('limits collections and enumerable object properties', function () { + const fiveProperties = { a: 1, b: 2, c: 3, d: 4, e: 5 } + Object.defineProperty(fiveProperties, 'hidden', { value: 6 }) + const sixProperties = { ...fiveProperties, f: 6 } + + assert.strictEqual(inspectSegment(42), '42') + assert.strictEqual(inspectSegment([1, 2, 3, 4]), '[ 1, 2, 3, ... 1 more item ]') + assert.strictEqual(inspectSegment(fiveProperties), '{ a: 1, b: 2, c: 3, d: 4, e: 5 }') + assert.strictEqual( + inspectSegment(sixProperties), + '{ a: 1, b: 2, c: 3, d: 4, e: 5, ... 1 more property }' + ) + }) + + it('does not invoke proxy traps', function () { + const proxy = new Proxy({}, { + ownKeys () { + throw new Error('Proxy trap should not run') + }, + }) + const objectWithProxyPrototype = Object.create(new Proxy({}, { + getPrototypeOf () { + throw new Error('Proxy prototype trap should not run') + }, + })) + Object.assign(objectWithProxyPrototype, { a: 1, b: 2, c: 3, d: 4, e: 5, f: 6 }) + + assert.strictEqual(inspectSegment(proxy), '[Proxy]') + assert.strictEqual( + inspectSegment(objectWithProxyPrototype), + '{ a: 1, b: 2, c: 3, d: 4, e: 5, ... 1 more property }' + ) + }) + + it('does not invoke Symbol.toStringTag getters or custom inspection functions', function () { + let customInspectCalled = false + const value = { + get [Symbol.toStringTag] () { + throw new Error('Symbol.toStringTag getter should not run') + }, + [inspect.custom] () { + customInspectCalled = true + return 'custom' + }, + } + + assert.strictEqual(inspectSegment(value), '[Value omitted: inspection may execute user code]') + assert.strictEqual(customInspectCalled, false) + }) + + it('omits wide objects containing values whose inspection may run user code', function () { + const sideEffectfulValue = { + get [Symbol.toStringTag] () { + throw new Error('Symbol.toStringTag getter should not run') + }, + } + const value = { a: sideEffectfulValue, b: 2, c: 3, d: 4, e: 5, f: 6 } + + assert.strictEqual(inspectSegment(value), '[Value omitted: inspection may execute user code]') + }) + + it('preserves circular references when truncating objects', function () { + const value = { circular: undefined, a: 1, b: 2, c: 3, d: 4, e: 5 } + value.circular = value + + assert.strictEqual( + inspectSegment(value), + ' { circular: [Circular *1], a: 1, b: 2, c: 3, d: 4, ... 1 more property }' + ) + }) +})