From 7d7b8adc496e83edcac2089f1c1ec98235f2ed8c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 21 Jul 2023 08:57:58 +0200 Subject: [PATCH] fix(utils): Truncate aggregate exception values (LinkedErrors) (#8593) Aggregate exceptions' `exception.value` strings weren't truncated correctly in our event preparation pipeline. The tricky part here is that the `LinkedErrors` integration adds the linked/child exceptions to the event in an event processor which is applied only after we truncate the original event. Hence, we need to also truncate the values in the event processor itself. --- .../captureException/linkedErrrors/subject.js | 13 ++++ .../captureException/linkedErrrors/test.ts | 41 +++++++++++ .../browser/src/integrations/linkederrors.ts | 4 +- .../node/src/integrations/linkederrors.ts | 5 +- packages/utils/src/aggregate-errors.ts | 37 +++++++--- packages/utils/test/aggregate-errors.test.ts | 68 ++++++++++++++++--- 6 files changed, 147 insertions(+), 21 deletions(-) create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/test.ts diff --git a/packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/subject.js new file mode 100644 index 000000000000..f7eb81412b13 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/subject.js @@ -0,0 +1,13 @@ +const wat = new Error(`This is a very long message that should be truncated and will be, +this is a very long message that should be truncated and will be, +this is a very long message that should be truncated and will be, +this is a very long message that should be truncated and will be, +this is a very long message that should be truncated and will be`); + +wat.cause = new Error(`This is a very long message that should be truncated and hopefully will be, +this is a very long message that should be truncated and hopefully will be, +this is a very long message that should be truncated and hopefully will be, +this is a very long message that should be truncated and hopefully will be, +this is a very long message that should be truncated and hopefully will be,`); + +Sentry.captureException(wat); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/test.ts new file mode 100644 index 000000000000..b78ae5e12525 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/linkedErrrors/test.ts @@ -0,0 +1,41 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should capture a linked error with messages', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(2); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'Error', + value: `This is a very long message that should be truncated and hopefully will be, +this is a very long message that should be truncated and hopefully will be, +this is a very long message that should be truncated and hopefully will be, +this is a very long me...`, + mechanism: { + type: 'chained', + handled: true, + }, + stacktrace: { + frames: expect.any(Array), + }, + }); + expect(eventData.exception?.values?.[1]).toMatchObject({ + type: 'Error', + value: `This is a very long message that should be truncated and will be, +this is a very long message that should be truncated and will be, +this is a very long message that should be truncated and will be, +this is a very long message that should be truncated...`, + mechanism: { + type: 'generic', + handled: true, + }, + stacktrace: { + frames: expect.any(Array), + }, + }); +}); diff --git a/packages/browser/src/integrations/linkederrors.ts b/packages/browser/src/integrations/linkederrors.ts index 08581fecb9b3..709ba8228107 100644 --- a/packages/browser/src/integrations/linkederrors.ts +++ b/packages/browser/src/integrations/linkederrors.ts @@ -54,9 +54,11 @@ export class LinkedErrors implements Integration { return event; } + const options = client.getOptions(); applyAggregateErrorsToEvent( exceptionFromError, - client.getOptions().stackParser, + options.stackParser, + options.maxValueLength, self._key, self._limit, event, diff --git a/packages/node/src/integrations/linkederrors.ts b/packages/node/src/integrations/linkederrors.ts index d6a130ce5d54..610a376f640a 100644 --- a/packages/node/src/integrations/linkederrors.ts +++ b/packages/node/src/integrations/linkederrors.ts @@ -50,9 +50,12 @@ export class LinkedErrors implements Integration { return event; } + const options = client.getOptions(); + applyAggregateErrorsToEvent( exceptionFromError, - client.getOptions().stackParser, + options.stackParser, + options.maxValueLength, self._key, self._limit, event, diff --git a/packages/utils/src/aggregate-errors.ts b/packages/utils/src/aggregate-errors.ts index 1dcf9b1628ef..4547203b4fd2 100644 --- a/packages/utils/src/aggregate-errors.ts +++ b/packages/utils/src/aggregate-errors.ts @@ -1,6 +1,7 @@ import type { Event, EventHint, Exception, ExtendedError, StackParser } from '@sentry/types'; import { isInstanceOf } from './is'; +import { truncate } from './string'; /** * Creates exceptions inside `event.exception.values` for errors that are nested on properties based on the `key` parameter. @@ -8,6 +9,7 @@ import { isInstanceOf } from './is'; export function applyAggregateErrorsToEvent( exceptionFromErrorImplementation: (stackParser: StackParser, ex: Error) => Exception, parser: StackParser, + maxValueLimit: number = 250, key: string, limit: number, event: Event, @@ -23,15 +25,18 @@ export function applyAggregateErrorsToEvent( // We only create exception grouping if there is an exception in the event. if (originalException) { - event.exception.values = aggregateExceptionsFromError( - exceptionFromErrorImplementation, - parser, - limit, - hint.originalException as ExtendedError, - key, - event.exception.values, - originalException, - 0, + event.exception.values = truncateAggregateExceptions( + aggregateExceptionsFromError( + exceptionFromErrorImplementation, + parser, + limit, + hint.originalException as ExtendedError, + key, + event.exception.values, + originalException, + 0, + ), + maxValueLimit, ); } } @@ -123,3 +128,17 @@ function applyExceptionGroupFieldsForChildException( parent_id: parentId, }; } + +/** + * Truncate the message (exception.value) of all exceptions in the event. + * Because this event processor is ran after `applyClientOptions`, + * we need to truncate the message of the added exceptions here. + */ +function truncateAggregateExceptions(exceptions: Exception[], maxValueLength: number): Exception[] { + return exceptions.map(exception => { + if (exception.value) { + exception.value = truncate(exception.value, maxValueLength); + } + return exception; + }); +} diff --git a/packages/utils/test/aggregate-errors.test.ts b/packages/utils/test/aggregate-errors.test.ts index 9a42bba12858..66b0f3fcfdb1 100644 --- a/packages/utils/test/aggregate-errors.test.ts +++ b/packages/utils/test/aggregate-errors.test.ts @@ -11,7 +11,7 @@ describe('applyAggregateErrorsToEvent()', () => { test('should not do anything if event does not contain an exception', () => { const event: Event = { exception: undefined }; const eventHint: EventHint = { originalException: new Error() }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, 'cause', 100, event, eventHint); // no changes expect(event).toStrictEqual({ exception: undefined }); @@ -20,7 +20,7 @@ describe('applyAggregateErrorsToEvent()', () => { test('should not do anything if event does not contain exception values', () => { const event: Event = { exception: { values: undefined } }; const eventHint: EventHint = { originalException: new Error() }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, 'cause', 100, event, eventHint); // no changes expect(event).toStrictEqual({ exception: { values: undefined } }); @@ -28,7 +28,7 @@ describe('applyAggregateErrorsToEvent()', () => { test('should not do anything if event does not contain an event hint', () => { const event: Event = { exception: { values: [] } }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, undefined); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, 'cause', 100, event, undefined); // no changes expect(event).toStrictEqual({ exception: { values: [] } }); @@ -37,7 +37,7 @@ describe('applyAggregateErrorsToEvent()', () => { test('should not do anything if the event hint does not contain an original exception', () => { const event: Event = { exception: { values: [] } }; const eventHint: EventHint = { originalException: undefined }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, 'cause', 100, event, eventHint); // no changes expect(event).toStrictEqual({ exception: { values: [] } }); @@ -52,7 +52,7 @@ describe('applyAggregateErrorsToEvent()', () => { const event: Event = { exception: { values: [exceptionFromError(stackParser, originalException)] } }; const eventHint: EventHint = { originalException }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, key, 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, key, 100, event, eventHint); expect(event).toStrictEqual({ exception: { values: [ @@ -97,7 +97,7 @@ describe('applyAggregateErrorsToEvent()', () => { const event: Event = { exception: { values: [exceptionFromError(stackParser, originalException)] } }; const eventHint: EventHint = { originalException }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, 'cause', 100, event, eventHint); // no changes expect(event).toStrictEqual({ exception: { values: [exceptionFromError(stackParser, originalException)] } }); @@ -116,7 +116,7 @@ describe('applyAggregateErrorsToEvent()', () => { } const eventHint: EventHint = { originalException }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, key, 5, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, key, 5, event, eventHint); // 6 -> one for original exception + 5 linked expect(event.exception?.values).toHaveLength(5 + 1); @@ -140,7 +140,7 @@ describe('applyAggregateErrorsToEvent()', () => { const event: Event = { exception: { values: [exceptionFromError(stackParser, fakeAggregateError)] } }; const eventHint: EventHint = { originalException: fakeAggregateError }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, 'cause', 100, event, eventHint); expect(event.exception?.values?.[event.exception.values.length - 1].mechanism?.type).toBe('instrument'); }); @@ -155,7 +155,7 @@ describe('applyAggregateErrorsToEvent()', () => { const event: Event = { exception: { values: [exceptionFromError(stackParser, fakeAggregateError1)] } }; const eventHint: EventHint = { originalException: fakeAggregateError1 }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, 'cause', 100, event, eventHint); expect(event).toStrictEqual({ exception: { values: [ @@ -234,7 +234,7 @@ describe('applyAggregateErrorsToEvent()', () => { const event: Event = { exception: { values: [exceptionFromError(stackParser, originalException)] } }; const eventHint: EventHint = { originalException }; - applyAggregateErrorsToEvent(exceptionFromError, stackParser, key, 100, event, eventHint); + applyAggregateErrorsToEvent(exceptionFromError, stackParser, undefined, key, 100, event, eventHint); expect(event).toStrictEqual({ exception: { values: [ @@ -272,4 +272,52 @@ describe('applyAggregateErrorsToEvent()', () => { }, }); }); + + test('should truncate the exception values if they exceed the `maxValueLength` option', () => { + const originalException: ExtendedError = new Error('Root Error with long message'); + originalException.cause = new Error('Nested Error 1 with longer message'); + originalException.cause.cause = new Error('Nested Error 2 with longer message with longer message'); + + const event: Event = { exception: { values: [exceptionFromError(stackParser, originalException)] } }; + const eventHint: EventHint = { originalException }; + + const maxValueLength = 15; + applyAggregateErrorsToEvent(exceptionFromError, stackParser, maxValueLength, 'cause', 10, event, eventHint); + expect(event).toStrictEqual({ + exception: { + values: [ + { + value: 'Nested Error 2 ...', + mechanism: { + exception_id: 2, + handled: true, + parent_id: 1, + source: 'cause', + type: 'chained', + }, + }, + { + value: 'Nested Error 1 ...', + mechanism: { + exception_id: 1, + handled: true, + parent_id: 0, + is_exception_group: true, + source: 'cause', + type: 'chained', + }, + }, + { + value: 'Root Error with...', + mechanism: { + exception_id: 0, + handled: true, + is_exception_group: true, + type: 'instrument', + }, + }, + ], + }, + }); + }); });