Skip to content

Commit 403a81d

Browse files
BridgeARbengl
authored andcommitted
Improve log safety (#5631)
The logger instrumentation is using a proxy. Calling Reflect.get with a receiver might actually cause issues at times. Instead, just access the target directly. That is easier and safer in this situation. On top, remove the special handling for Symbol.toStringTag. That was added for the testing library and should not have been there in the first place. As a drive-by fix, it also improves the performance a very tiny bit.
1 parent f29db63 commit 403a81d

File tree

2 files changed

+39
-22
lines changed

2 files changed

+39
-22
lines changed

packages/datadog-plugin-winston/test/index.spec.js

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const agent = require('../../dd-trace/test/plugins/agent')
55
const http = require('http')
66
const { expect } = require('chai')
77
const proxyquire = require('proxyquire').noPreserveCache()
8+
const { inspect } = require('util')
89

910
function createLogServer () {
1011
return new Promise((resolve, reject) => {
@@ -194,7 +195,6 @@ describe('Plugin', () => {
194195
span_id: span.context().toSpanId()
195196
}
196197
}
197-
198198
const error = new Error('boom')
199199

200200
tracer.scope().activate(span, () => {
@@ -203,14 +203,42 @@ describe('Plugin', () => {
203203
const index = semver.intersects(version, '>=3') ? 0 : 2
204204
const record = log.firstCall.args[index]
205205

206-
expect(record).to.be.an('error')
206+
expect(record).to.be.an.instanceof(Error)
207207
expect(error).to.not.have.property('dd')
208208
expect(spy).to.have.been.calledWithMatch(meta.dd)
209209
})
210210
expect(await logServer.logPromise).to.include(meta.dd)
211211
})
212212

213213
if (semver.intersects(version, '>=3')) {
214+
it('should support sets and getters', async () => {
215+
const meta = {
216+
dd: {
217+
trace_id: span.context().toTraceId(true),
218+
span_id: span.context().toSpanId()
219+
}
220+
}
221+
const set = new Set([1])
222+
Object.defineProperty(set, 'getter', {
223+
get () {
224+
return this.size
225+
},
226+
enumerable: true
227+
})
228+
229+
tracer.scope().activate(span, () => {
230+
winston.log('info', set)
231+
232+
const record = log.firstCall.args[0]
233+
234+
expect(record).to.be.an.instanceof(Set)
235+
expect(inspect(record)).to.match(/"getter":1,/)
236+
expect(set).to.not.have.property('dd')
237+
expect(spy).to.have.been.calledWithMatch(meta.dd)
238+
})
239+
expect(await logServer.logPromise).to.include(meta.dd)
240+
})
241+
214242
it('should add the trace identifiers when streaming', async () => {
215243
const logger = winston.createLogger({
216244
transports: [transport, httpTransport]

packages/dd-trace/src/plugins/log_plugin.js

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,31 +4,21 @@ const { LOG } = require('../../../../ext/formats')
44
const Plugin = require('./plugin')
55
const { storage } = require('../../../datadog-core')
66

7-
const hasOwn = (obj, prop) => Object.prototype.hasOwnProperty.call(obj, prop)
8-
97
function messageProxy (message, holder) {
108
return new Proxy(message, {
11-
get (target, p, receiver) {
12-
if (p === Symbol.toStringTag) {
13-
return Object.prototype.toString.call(target).slice(8, -1)
14-
}
15-
16-
if (shouldOverride(target, p)) {
9+
get (target, key) {
10+
if (shouldOverride(target, key)) {
1711
return holder.dd
1812
}
1913

20-
// This is a workaround for a V8 bug that surfaced in Node.js 22
21-
if (p === 'stack') {
22-
return target.stack
23-
}
24-
25-
return Reflect.get(target, p, receiver)
14+
return target[key]
2615
},
2716
ownKeys (target) {
2817
const ownKeys = Reflect.ownKeys(target)
29-
return hasOwn(target, 'dd') || !Reflect.isExtensible(target)
30-
? ownKeys
31-
: ['dd', ...ownKeys]
18+
if (!Object.hasOwn(target, 'dd') && Reflect.isExtensible(target)) {
19+
ownKeys.push('dd')
20+
}
21+
return ownKeys
3222
},
3323
getOwnPropertyDescriptor (target, p) {
3424
return Reflect.getOwnPropertyDescriptor(shouldOverride(target, p) ? holder : target, p)
@@ -37,16 +27,15 @@ function messageProxy (message, holder) {
3727
}
3828

3929
function shouldOverride (target, p) {
40-
return p === 'dd' && !Reflect.has(target, p) && Reflect.isExtensible(target)
30+
return p === 'dd' && !Object.hasOwn(target, p) && Reflect.isExtensible(target)
4131
}
4232

4333
module.exports = class LogPlugin extends Plugin {
4434
constructor (...args) {
4535
super(...args)
4636

4737
this.addSub(`apm:${this.constructor.id}:log`, (arg) => {
48-
const store = storage('legacy').getStore()
49-
const span = store && store.span
38+
const span = storage('legacy').getStore()?.span
5039

5140
// NOTE: This needs to run whether or not there is a span
5241
// so service, version, and env will always get injected.

0 commit comments

Comments
 (0)