Skip to content

Commit a1de51d

Browse files
trentmtrentm
authored
fix(hapi): limit which event data fields are included in captured APM error object for Hapi 'log' and 'request' error events (#4504)
Closes: #4503
1 parent b053ddc commit a1de51d

File tree

3 files changed

+131
-46
lines changed

3 files changed

+131
-46
lines changed

‎CHANGELOG.asciidoc‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,34 @@ See the <<upgrade-to-v4>> guide.
4444
[float]
4545
===== Bug fixes
4646
47+
* Change how `@hapi/hapi` instrumentation includes additional data when
48+
capturing an error for Hapi `log` and `request` Server events to avoid
49+
possible capture of large amounts of data, that could lead to latency issues
50+
and high memory usage. Some data that may have been captured before will
51+
*no longer* be captured. ({issues}4503[#4503])
52+
+
53+
The `@hapi/hapi` instrumentation will capture an APM error whenever a
54+
Hapi `log` or `request` server event (https://hapi.dev/api/#server.events) with
55+
the "error" tag is emitted, e.g. when a Hapi server responds with an HTTP 500
56+
error. Before this change, any and all properties on the logged Error or data
57+
would be included in the APM error data sent to APM server (in the
58+
`error.custom` field). This could cause a surprise for applications that attach
59+
(sometimes large) data to the internal server Error for other purposes (e.g.
60+
application error handling).
61+
+
62+
The expected surprise case is when a deeply-nested object is added as a
63+
property to the event data. To protect against serializing these, the Hapi
64+
instrumentation will only serialize event data properties that are "simple"
65+
types (boolean, string, number, Date), other types (Array, object, Buffer, etc.)
66+
will *not* be captured. This is similar behavior as is used for the
67+
`captureAttributes` option to <<apm-capture-error,`apm.captureError()`>>
68+
for the same purpose.
69+
+
70+
In addition, the updated Hapi instrumentation will no longer capture to
71+
`error.custom` when the emitted data is an `Error` instance, because this was a
72+
duplication of the `Error` properties already being captured to the
73+
`error.exception.attributes` field.
74+
4775
[float]
4876
===== Chores
4977

‎lib/instrumentation/modules/@hapi/hapi.js‎

Lines changed: 81 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,69 @@ var shimmer = require('../../shimmer');
1212

1313
var onPreAuthSym = Symbol('ElasticAPMOnPreAuth');
1414

15+
// Collect simple data a Hapi `event.data` object, typically from a Hapi
16+
// 'log' or 'request' server event (https://hapi.dev/api/#server.events). This
17+
// limits to including simple property values (bool, string, number, Date) to
18+
// limit the possibility of accidentally capturing huge data in `captureError`
19+
// below.
20+
//
21+
// This implementation is based on lib/errors.js#attributesFromErr.
22+
function simpleDataFromEventData(agent, eventData) {
23+
try {
24+
let simpleRepr = simpleReprFromVal(eventData);
25+
if (simpleRepr !== undefined) {
26+
return simpleRepr;
27+
}
28+
29+
let n = 0;
30+
const attrs = {};
31+
const keys = Object.keys(eventData);
32+
for (let i = 0; i < keys.length; i++) {
33+
const key = keys[i];
34+
let val = eventData[key];
35+
simpleRepr = simpleReprFromVal(val);
36+
if (simpleRepr) {
37+
attrs[key] = simpleRepr;
38+
n++;
39+
}
40+
}
41+
return n ? attrs : undefined;
42+
} catch (err) {
43+
agent.logger.trace(
44+
'hapi: could not gather simple attrs from event data: ' + err.message,
45+
);
46+
}
47+
}
48+
49+
// If `val` is a "simple" type (bool, string, number, Date), then return a
50+
// reasonable value to represent it in a JSON serialization. Otherwise, return
51+
// undefined.
52+
function simpleReprFromVal(val) {
53+
switch (typeof val) {
54+
case 'boolean':
55+
case 'string':
56+
case 'number':
57+
break;
58+
case 'object':
59+
// Ignore all objects except Dates.
60+
if (
61+
val === null ||
62+
typeof val.toISOString !== 'function' ||
63+
typeof val.getTime !== 'function'
64+
) {
65+
return;
66+
} else if (Number.isNaN(val.getTime())) {
67+
val = 'Invalid Date'; // calling toISOString() on invalid dates throws
68+
} else {
69+
val = val.toISOString();
70+
}
71+
break;
72+
default:
73+
return;
74+
}
75+
return val;
76+
}
77+
1578
module.exports = function (hapi, agent, { version, enabled }) {
1679
if (!enabled) {
1780
return hapi;
@@ -64,23 +127,28 @@ module.exports = function (hapi, agent, { version, enabled }) {
64127
return;
65128
}
66129

67-
// TODO: Find better location to put this than custom
68-
var payload = {
130+
// Hapi 'log' and 'request' events (https://hapi.dev/api/#server.events)
131+
// have `event.error`, `event.data`, or neither.
132+
// `agent.captureError` requires an Error instance or string for its first
133+
// arg: bias to getting that, then any other data add to `opts.custom.data`.
134+
const info = event.error || event.data;
135+
let errOrStr, data;
136+
if (info instanceof Error || typeof info === 'string') {
137+
errOrStr = info;
138+
} else if (info) {
139+
data = simpleDataFromEventData(agent, info);
140+
}
141+
if (!errOrStr) {
142+
errOrStr = 'hapi server emitted a "' + type + '" event tagged "error"';
143+
}
144+
145+
agent.captureError(errOrStr, {
69146
custom: {
70147
tags: event.tags,
71-
internals: event.internals,
72-
// Moved from data to error in hapi 17
73-
data: event.data || event.error,
148+
data,
74149
},
75150
request: req && req.raw && req.raw.req,
76-
};
77-
78-
var err = payload.custom.data;
79-
if (!(err instanceof Error) && typeof err !== 'string') {
80-
err = 'hapi server emitted a ' + type + ' event tagged error';
81-
}
82-
83-
agent.captureError(err, payload);
151+
});
84152
}
85153

86154
function onPreAuth(request, reply) {

‎test/instrumentation/modules/hapi/hapi.test.js‎

Lines changed: 22 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ test('connectionless server error logging with Error', function (t) {
151151
return;
152152
}
153153

154-
t.plan(6);
154+
t.plan(5);
155155

156156
var customError = new Error('custom error');
157157

@@ -163,8 +163,7 @@ test('connectionless server error logging with Error', function (t) {
163163
t.strictEqual(err, customError);
164164
t.ok(opts.custom);
165165
t.deepEqual(opts.custom.tags, ['error']);
166-
t.false(opts.custom.internals);
167-
t.ok(opts.custom.data instanceof Error);
166+
t.strictEqual(opts.custom.data, undefined);
168167
};
169168

170169
var server = makeServer();
@@ -182,7 +181,7 @@ test('connectionless server error logging with String', function (t) {
182181
return;
183182
}
184183

185-
t.plan(6);
184+
t.plan(5);
186185

187186
var customError = 'custom error';
188187

@@ -194,8 +193,7 @@ test('connectionless server error logging with String', function (t) {
194193
t.strictEqual(err, customError);
195194
t.ok(opts.custom);
196195
t.deepEqual(opts.custom.tags, ['error']);
197-
t.false(opts.custom.internals);
198-
t.ok(typeof opts.custom.data === 'string');
196+
t.strictEqual(opts.custom.data, undefined);
199197
};
200198

201199
var server = makeServer();
@@ -213,7 +211,7 @@ test('connectionless server error logging with Object', function (t) {
213211
return;
214212
}
215213

216-
t.plan(6);
214+
t.plan(5);
217215

218216
var customError = {
219217
error: 'I forgot to turn this into an actual Error',
@@ -224,10 +222,9 @@ test('connectionless server error logging with Object', function (t) {
224222
agent.captureError = function (err, opts) {
225223
server.stop(noop);
226224

227-
t.strictEqual(err, 'hapi server emitted a log event tagged error');
225+
t.strictEqual(err, 'hapi server emitted a "log" event tagged "error"');
228226
t.ok(opts.custom);
229227
t.deepEqual(opts.custom.tags, ['error']);
230-
t.false(opts.custom.internals);
231228
t.deepEqual(opts.custom.data, customError);
232229
};
233230

@@ -240,7 +237,7 @@ test('connectionless server error logging with Object', function (t) {
240237
});
241238

242239
test('server error logging with Error', function (t) {
243-
t.plan(6);
240+
t.plan(5);
244241

245242
var customError = new Error('custom error');
246243

@@ -252,8 +249,7 @@ test('server error logging with Error', function (t) {
252249
t.strictEqual(err, customError);
253250
t.ok(opts.custom);
254251
t.deepEqual(opts.custom.tags, ['error']);
255-
t.false(opts.custom.internals);
256-
t.ok(opts.custom.data instanceof Error);
252+
t.strictEqual(opts.custom.data, undefined);
257253
};
258254

259255
var server = startServer(function (err) {
@@ -264,7 +260,7 @@ test('server error logging with Error', function (t) {
264260
});
265261

266262
test('server error logging with Error does not affect event tags', function (t) {
267-
t.plan(8);
263+
t.plan(7);
268264

269265
var customError = new Error('custom error');
270266

@@ -276,8 +272,7 @@ test('server error logging with Error does not affect event tags', function (t)
276272
t.strictEqual(err, customError);
277273
t.ok(opts.custom);
278274
t.deepEqual(opts.custom.tags, ['error']);
279-
t.false(opts.custom.internals);
280-
t.ok(opts.custom.data instanceof Error);
275+
t.strictEqual(opts.custom.data, undefined);
281276
};
282277

283278
var server = makeServer();
@@ -299,7 +294,7 @@ test('server error logging with Error does not affect event tags', function (t)
299294
});
300295

301296
test('server error logging with String', function (t) {
302-
t.plan(6);
297+
t.plan(5);
303298

304299
var customError = 'custom error';
305300

@@ -311,8 +306,7 @@ test('server error logging with String', function (t) {
311306
t.strictEqual(err, customError);
312307
t.ok(opts.custom);
313308
t.deepEqual(opts.custom.tags, ['error']);
314-
t.false(opts.custom.internals);
315-
t.ok(typeof opts.custom.data === 'string');
309+
t.strictEqual(opts.custom.data, undefined);
316310
};
317311

318312
var server = startServer(function (err) {
@@ -323,7 +317,7 @@ test('server error logging with String', function (t) {
323317
});
324318

325319
test('server error logging with Object', function (t) {
326-
t.plan(6);
320+
t.plan(5);
327321

328322
var customError = {
329323
error: 'I forgot to turn this into an actual Error',
@@ -334,10 +328,9 @@ test('server error logging with Object', function (t) {
334328
agent.captureError = function (err, opts) {
335329
server.stop(noop);
336330

337-
t.strictEqual(err, 'hapi server emitted a log event tagged error');
331+
t.strictEqual(err, 'hapi server emitted a "log" event tagged "error"');
338332
t.ok(opts.custom);
339333
t.deepEqual(opts.custom.tags, ['error']);
340-
t.false(opts.custom.internals);
341334
t.deepEqual(opts.custom.data, customError);
342335
};
343336

@@ -349,7 +342,7 @@ test('server error logging with Object', function (t) {
349342
});
350343

351344
test('request error logging with Error', function (t) {
352-
t.plan(13);
345+
t.plan(12);
353346

354347
var customError = new Error('custom error');
355348

@@ -364,8 +357,7 @@ test('request error logging with Error', function (t) {
364357
t.ok(opts.custom);
365358
t.ok(opts.request);
366359
t.deepEqual(opts.custom.tags, ['error']);
367-
t.false(opts.custom.internals);
368-
t.ok(opts.custom.data instanceof Error);
360+
t.strictEqual(opts.custom.data, undefined);
369361
};
370362

371363
var server = makeServer();
@@ -394,7 +386,7 @@ test('request error logging with Error', function (t) {
394386
});
395387

396388
test('request error logging with Error does not affect event tags', function (t) {
397-
t.plan(15);
389+
t.plan(14);
398390

399391
var customError = new Error('custom error');
400392

@@ -409,8 +401,7 @@ test('request error logging with Error does not affect event tags', function (t)
409401
t.ok(opts.custom);
410402
t.ok(opts.request);
411403
t.deepEqual(opts.custom.tags, ['elastic-apm', 'error']);
412-
t.false(opts.custom.internals);
413-
t.ok(opts.custom.data instanceof Error);
404+
t.strictEqual(opts.custom.data, undefined);
414405
};
415406

416407
var server = makeServer();
@@ -450,7 +441,7 @@ test('request error logging with Error does not affect event tags', function (t)
450441
});
451442

452443
test('request error logging with String', function (t) {
453-
t.plan(13);
444+
t.plan(12);
454445

455446
var customError = 'custom error';
456447

@@ -465,8 +456,7 @@ test('request error logging with String', function (t) {
465456
t.ok(opts.custom);
466457
t.ok(opts.request);
467458
t.deepEqual(opts.custom.tags, ['error']);
468-
t.false(opts.custom.internals);
469-
t.ok(typeof opts.custom.data === 'string');
459+
t.strictEqual(opts.custom.data, undefined);
470460
};
471461

472462
var server = makeServer();
@@ -495,7 +485,7 @@ test('request error logging with String', function (t) {
495485
});
496486

497487
test('request error logging with Object', function (t) {
498-
t.plan(13);
488+
t.plan(12);
499489

500490
var customError = {
501491
error: 'I forgot to turn this into an actual Error',
@@ -508,11 +498,10 @@ test('request error logging with Object', function (t) {
508498
});
509499

510500
agent.captureError = function (err, opts) {
511-
t.strictEqual(err, 'hapi server emitted a request event tagged error');
501+
t.strictEqual(err, 'hapi server emitted a "request" event tagged "error"');
512502
t.ok(opts.custom);
513503
t.ok(opts.request);
514504
t.deepEqual(opts.custom.tags, ['error']);
515-
t.false(opts.custom.internals);
516505
t.deepEqual(opts.custom.data, customError);
517506
};
518507

0 commit comments

Comments
 (0)