Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(hapi): limit which event data fields are included in captured APM error object for Hapi 'log' and 'request' error events #4504

Merged
merged 4 commits into from
Mar 13, 2025
Merged

fix(hapi): limit which event data fields are included in captured APM error object for Hapi 'log' and 'request' error events #4504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
825bcac
fix(hapi): limit which event data fields are included in captured APM…
trentm Mar 11, 2025
be7ce0f
fix t.plan()s in tests; move part of logic to simpleReprFromVal per r…
trentm Mar 12, 2025
4ee4492
slightly nicer logic per review feedback
trentm Mar 12, 2025
1f157db
try to clarify the user impact in changelog
trentm Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,34 @@ See the <<upgrade-to-v4>> guide.
[float]
===== Bug fixes

* Change how `@hapi/hapi` instrumentation includes additional data when
capturing an error for Hapi `log` and `request` Server events to avoid
possible capture of large amounts of data, that could lead to latency issues
and high memory usage. Some data that may have been captured before will
*no longer* be captured. ({issues}4503[#4503])
+
The `@hapi/hapi` instrumentation will capture an APM error whenever a
Hapi `log` or `request` server event (https://hapi.dev/api/#server.events) with
the "error" tag is emitted, e.g. when a Hapi server responds with an HTTP 500
error. Before this change, any and all properties on the logged Error or data
would be included in the APM error data sent to APM server (in the
`error.custom` field). This could cause a surprise for applications that attach
(sometimes large) data to the internal server Error for other purposes (e.g.
application error handling).
+
The expected surprise case is when a deeply-nested object is added as a
property to the event data. To protect against serializing these, the Hapi
instrumentation will only serialize event data properties that are "simple"
types (boolean, string, number, Date), other types (Array, object, Buffer, etc.)
will *not* be captured. This is similar behavior as is used for the
`captureAttributes` option to <<apm-capture-error,`apm.captureError()`>>
for the same purpose.
+
In addition, the updated Hapi instrumentation will no longer capture to
`error.custom` when the emitted data is an `Error` instance, because this was a
duplication of the `Error` properties already being captured to the
`error.exception.attributes` field.

[float]
===== Chores

Expand Down
94 changes: 81 additions & 13 deletions lib/instrumentation/modules/@hapi/hapi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,69 @@ var shimmer = require('../../shimmer');

var onPreAuthSym = Symbol('ElasticAPMOnPreAuth');

// Collect simple data a Hapi `event.data` object, typically from a Hapi
// 'log' or 'request' server event (https://hapi.dev/api/#server.events). This
// limits to including simple property values (bool, string, number, Date) to
// limit the possibility of accidentally capturing huge data in `captureError`
// below.
//
// This implementation is based on lib/errors.js#attributesFromErr.
function simpleDataFromEventData(agent, eventData) {
try {
let simpleRepr = simpleReprFromVal(eventData);
if (simpleRepr !== undefined) {
return simpleRepr;
}

let n = 0;
const attrs = {};
const keys = Object.keys(eventData);
for (let i = 0; i < keys.length; i++) {
const key = keys[i];
let val = eventData[key];
simpleRepr = simpleReprFromVal(val);
if (simpleRepr) {
attrs[key] = simpleRepr;
n++;
}
}
return n ? attrs : undefined;
} catch (err) {
agent.logger.trace(
'hapi: could not gather simple attrs from event data: ' + err.message,
);
}
}

// If `val` is a "simple" type (bool, string, number, Date), then return a
// reasonable value to represent it in a JSON serialization. Otherwise, return
// undefined.
function simpleReprFromVal(val) {
switch (typeof val) {
case 'boolean':
case 'string':
case 'number':
break;
case 'object':
// Ignore all objects except Dates.
if (
val === null ||
typeof val.toISOString !== 'function' ||
typeof val.getTime !== 'function'
) {
return;
} else if (Number.isNaN(val.getTime())) {
val = 'Invalid Date'; // calling toISOString() on invalid dates throws
} else {
val = val.toISOString();
}
break;
default:
return;
}
return val;
}

module.exports = function (hapi, agent, { version, enabled }) {
if (!enabled) {
return hapi;
Expand Down Expand Up @@ -64,23 +127,28 @@ module.exports = function (hapi, agent, { version, enabled }) {
return;
}

// TODO: Find better location to put this than custom
var payload = {
// Hapi 'log' and 'request' events (https://hapi.dev/api/#server.events)
// have `event.error`, `event.data`, or neither.
// `agent.captureError` requires an Error instance or string for its first
// arg: bias to getting that, then any other data add to `opts.custom.data`.
const info = event.error || event.data;
let errOrStr, data;
if (info instanceof Error || typeof info === 'string') {
errOrStr = info;
} else if (info) {
data = simpleDataFromEventData(agent, info);
}
if (!errOrStr) {
errOrStr = 'hapi server emitted a "' + type + '" event tagged "error"';
}

agent.captureError(errOrStr, {
custom: {
tags: event.tags,
internals: event.internals,
// Moved from data to error in hapi 17
data: event.data || event.error,
data,
},
request: req && req.raw && req.raw.req,
};

var err = payload.custom.data;
if (!(err instanceof Error) && typeof err !== 'string') {
err = 'hapi server emitted a ' + type + ' event tagged error';
}

agent.captureError(err, payload);
});
}

function onPreAuth(request, reply) {
Expand Down
55 changes: 22 additions & 33 deletions test/instrumentation/modules/hapi/hapi.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ test('connectionless server error logging with Error', function (t) {
return;
}

t.plan(6);
t.plan(5);

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

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

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

t.plan(6);
t.plan(5);

var customError = 'custom error';

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

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

t.plan(6);
t.plan(5);

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

t.strictEqual(err, 'hapi server emitted a log event tagged error');
t.strictEqual(err, 'hapi server emitted a "log" event tagged "error"');
t.ok(opts.custom);
t.deepEqual(opts.custom.tags, ['error']);
t.false(opts.custom.internals);
t.deepEqual(opts.custom.data, customError);
};

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

test('server error logging with Error', function (t) {
t.plan(6);
t.plan(5);

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

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

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

test('server error logging with Error does not affect event tags', function (t) {
t.plan(8);
t.plan(7);

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

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

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

test('server error logging with String', function (t) {
t.plan(6);
t.plan(5);

var customError = 'custom error';

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

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

test('server error logging with Object', function (t) {
t.plan(6);
t.plan(5);

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

t.strictEqual(err, 'hapi server emitted a log event tagged error');
t.strictEqual(err, 'hapi server emitted a "log" event tagged "error"');
t.ok(opts.custom);
t.deepEqual(opts.custom.tags, ['error']);
t.false(opts.custom.internals);
t.deepEqual(opts.custom.data, customError);
};

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

test('request error logging with Error', function (t) {
t.plan(13);
t.plan(12);

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

Expand All @@ -364,8 +357,7 @@ test('request error logging with Error', function (t) {
t.ok(opts.custom);
t.ok(opts.request);
t.deepEqual(opts.custom.tags, ['error']);
t.false(opts.custom.internals);
t.ok(opts.custom.data instanceof Error);
t.strictEqual(opts.custom.data, undefined);
};

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

test('request error logging with Error does not affect event tags', function (t) {
t.plan(15);
t.plan(14);

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

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

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

test('request error logging with String', function (t) {
t.plan(13);
t.plan(12);

var customError = 'custom error';

Expand All @@ -465,8 +456,7 @@ test('request error logging with String', function (t) {
t.ok(opts.custom);
t.ok(opts.request);
t.deepEqual(opts.custom.tags, ['error']);
t.false(opts.custom.internals);
t.ok(typeof opts.custom.data === 'string');
t.strictEqual(opts.custom.data, undefined);
};

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

test('request error logging with Object', function (t) {
t.plan(13);
t.plan(12);

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

agent.captureError = function (err, opts) {
t.strictEqual(err, 'hapi server emitted a request event tagged error');
t.strictEqual(err, 'hapi server emitted a "request" event tagged "error"');
t.ok(opts.custom);
t.ok(opts.request);
t.deepEqual(opts.custom.tags, ['error']);
t.false(opts.custom.internals);
t.deepEqual(opts.custom.data, customError);
};

Expand Down
Loading