From f51fe6860c778de9bd75bc15c4c30d1c0f80658e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 May 2022 09:39:11 +0000 Subject: [PATCH 1/8] fix(core): Normalize trace context --- packages/core/src/baseclient.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 3f365f111ed9..b298324eb804 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -506,6 +506,12 @@ export abstract class BaseClient implements Client { if (event.contexts && event.contexts.trace) { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access normalized.contexts.trace = event.contexts.trace; + + // event.contexts.trace.data may contain circular/dangerous data so we need to normalize it + if (event.contexts.trace.data) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + normalized.contexts.trace.data = normalize(event.contexts.trace.data, depth, maxBreadth); + } } normalized.sdkProcessingMetadata = { ...normalized.sdkProcessingMetadata, baseClientNormalized: true }; From 6720cca0dc36a644dd1e9778f82c498fa9682ccc Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 May 2022 14:08:29 +0000 Subject: [PATCH 2/8] Normalize free form data in spans --- packages/core/src/baseclient.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index b298324eb804..a32410fe629d 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -476,7 +476,7 @@ export abstract class BaseClient implements Client { return null; } - const normalized = { + const normalized: Event = { ...event, ...(event.breadcrumbs && { breadcrumbs: event.breadcrumbs.map(b => ({ @@ -496,6 +496,7 @@ export abstract class BaseClient implements Client { extra: normalize(event.extra, depth, maxBreadth), }), }; + // event.contexts.trace stores information about a Transaction. Similarly, // event.spans[] stores information about child Spans. Given that a // Transaction is conceptually a Span, normalization should apply to both @@ -504,16 +505,24 @@ export abstract class BaseClient implements Client { // so this block overwrites the normalized event to add back the original // Transaction information prior to normalization. if (event.contexts && event.contexts.trace) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + normalized.contexts = {}; normalized.contexts.trace = event.contexts.trace; // event.contexts.trace.data may contain circular/dangerous data so we need to normalize it if (event.contexts.trace.data) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access normalized.contexts.trace.data = normalize(event.contexts.trace.data, depth, maxBreadth); } } + // event.spans[].data may contain circular/dangerous data so we need to normalize it + if (event.spans) { + normalized.spans = event.spans.map(span => { + // We cannot use the spread operator on span here because that overwrites the `toJSON` method + span.data = normalize(span.data, depth, maxBreadth); + return span; + }); + } + normalized.sdkProcessingMetadata = { ...normalized.sdkProcessingMetadata, baseClientNormalized: true }; return normalized; From 37691cdcdfb4fe469fb3803c234ed0216b26411c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 May 2022 14:24:22 +0000 Subject: [PATCH 3/8] Add integration tests --- .../startTransaction/circular_data/subject.js | 11 ++++++++ .../startTransaction/circular_data/test.ts | 26 +++++++++++++++++++ .../public-api/startTransaction/init.js | 1 + 3 files changed, 38 insertions(+) create mode 100644 packages/integration-tests/suites/public-api/startTransaction/circular_data/subject.js create mode 100644 packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts diff --git a/packages/integration-tests/suites/public-api/startTransaction/circular_data/subject.js b/packages/integration-tests/suites/public-api/startTransaction/circular_data/subject.js new file mode 100644 index 000000000000..50f8cef000be --- /dev/null +++ b/packages/integration-tests/suites/public-api/startTransaction/circular_data/subject.js @@ -0,0 +1,11 @@ +const chicken = {}; +const egg = { contains: chicken }; +chicken.lays = egg; + +const circularObject = chicken; + +const transaction = Sentry.startTransaction({ name: 'circular_object_test_transaction', data: circularObject }); +const span = transaction.startChild({ op: 'circular_object_test_span', data: circularObject }); + +span.finish(); +transaction.finish(); diff --git a/packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts b/packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts new file mode 100644 index 000000000000..a716c0239b3d --- /dev/null +++ b/packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts @@ -0,0 +1,26 @@ +import { expect } from '@playwright/test'; +import { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('should be able to handle circular data', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.type).toBe('transaction'); + expect(eventData.transaction).toBe('circular_object_test_transaction'); + + expect(eventData.contexts).toMatchObject({ + trace: { + data: { lays: { contains: { lays: { contains: '[Circular ~]' } } } }, + }, + }); + + expect(eventData?.spans?.[0]).toMatchObject({ + data: { lays: { contains: '[Circular ~]' } }, + op: 'circular_object_test_span', + }); + + await new Promise(resolve => setTimeout(resolve, 2000)); +}); diff --git a/packages/integration-tests/suites/public-api/startTransaction/init.js b/packages/integration-tests/suites/public-api/startTransaction/init.js index b326cc489bde..0aadc7c39b84 100644 --- a/packages/integration-tests/suites/public-api/startTransaction/init.js +++ b/packages/integration-tests/suites/public-api/startTransaction/init.js @@ -7,4 +7,5 @@ window.Sentry = Sentry; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', tracesSampleRate: 1.0, + normalizeDepth: 10, }); From 2ff633716089952fe376131f01e44494f95a60a6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 May 2022 14:27:59 +0000 Subject: [PATCH 4/8] Fix tests --- packages/core/src/baseclient.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index a32410fe629d..d9bf66a5dfb1 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -518,7 +518,9 @@ export abstract class BaseClient implements Client { if (event.spans) { normalized.spans = event.spans.map(span => { // We cannot use the spread operator on span here because that overwrites the `toJSON` method - span.data = normalize(span.data, depth, maxBreadth); + if (span.data) { + span.data = normalize(span.data, depth, maxBreadth); + } return span; }); } From febd6cd9c3b4c8fb8e781c7e91d4964eb9dee699 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 1 Jun 2022 07:27:00 +0000 Subject: [PATCH 5/8] Add documentation --- packages/core/src/baseclient.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index d9bf66a5dfb1..ef5635f98541 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -517,7 +517,7 @@ export abstract class BaseClient implements Client { // event.spans[].data may contain circular/dangerous data so we need to normalize it if (event.spans) { normalized.spans = event.spans.map(span => { - // We cannot use the spread operator on span here because that overwrites the `toJSON` method + // We cannot use the spread operator here because `toJSON` on `span` is non-enumerable if (span.data) { span.data = normalize(span.data, depth, maxBreadth); } From 7ec680ffbb8253eaffe50e861446fd02ead19316 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 1 Jun 2022 09:44:31 +0000 Subject: [PATCH 6/8] Fix bug in `dropUndefinedKeys` that kept references to original data --- .../suites/public-api/startTransaction/circular_data/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts b/packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts index a716c0239b3d..7ad0ec532fb7 100644 --- a/packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts +++ b/packages/integration-tests/suites/public-api/startTransaction/circular_data/test.ts @@ -13,7 +13,7 @@ sentryTest('should be able to handle circular data', async ({ getLocalTestPath, expect(eventData.contexts).toMatchObject({ trace: { - data: { lays: { contains: { lays: { contains: '[Circular ~]' } } } }, + data: { lays: { contains: '[Circular ~]' } }, }, }); From 0256842391505da97317e2c345f456ceea82b26f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 3 Jun 2022 07:57:23 +0000 Subject: [PATCH 7/8] Undo changes to `dropUndefinedKeys` --- packages/utils/src/object.ts | 62 +++++++++++------------------- packages/utils/test/object.test.ts | 36 ++++++++--------- 2 files changed, 37 insertions(+), 61 deletions(-) diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index 5b789b29060d..5957214ecc86 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -4,6 +4,7 @@ import { WrappedFunction } from '@sentry/types'; import { htmlTreeAsString } from './browser'; import { isElement, isError, isEvent, isInstanceOf, isPlainObject, isPrimitive } from './is'; +import { memoBuilder, MemoFunc } from './memo'; import { truncate } from './string'; /** @@ -203,61 +204,42 @@ export function extractExceptionKeysForMessage(exception: Record(inputValue: T): T { - // This map keeps track of what already visited nodes map to. - // Our Set - based memoBuilder doesn't work here because we want to the output object to have the same circular - // references as the input object. - const memoizationMap = new Map(); - +export function dropUndefinedKeys(val: T): T { // This function just proxies `_dropUndefinedKeys` to keep the `memoBuilder` out of this function's API - return _dropUndefinedKeys(inputValue, memoizationMap); + return _dropUndefinedKeys(val, memoBuilder()); } -function _dropUndefinedKeys(inputValue: T, memoizationMap: Map): T { - if (isPlainObject(inputValue)) { - // If this node has already been visited due to a circular reference, return the object it was mapped to in the new object - const memoVal = memoizationMap.get(inputValue); - if (memoVal !== undefined) { - return memoVal as T; - } - - const returnValue: { [key: string]: any } = {}; - // Store the mapping of this value in case we visit it again, in case of circular data - memoizationMap.set(inputValue, returnValue); +function _dropUndefinedKeys(val: T, memo: MemoFunc): T { + const [memoize] = memo; // we don't need unmemoize because we don't need to visit nodes twice - for (const key of Object.keys(inputValue)) { - if (typeof inputValue[key] !== 'undefined') { - returnValue[key] = _dropUndefinedKeys(inputValue[key], memoizationMap); + if (isPlainObject(val)) { + if (memoize(val)) { + return val; + } + const rv: { [key: string]: any } = {}; + for (const key of Object.keys(val)) { + if (typeof val[key] !== 'undefined') { + rv[key] = _dropUndefinedKeys(val[key], memo); } } - - return returnValue as T; + return rv as T; } - if (Array.isArray(inputValue)) { - // If this node has already been visited due to a circular reference, return the array it was mapped to in the new object - const memoVal = memoizationMap.get(inputValue); - if (memoVal !== undefined) { - return memoVal as T; + if (Array.isArray(val)) { + if (memoize(val)) { + return val; } - - const returnValue: unknown[] = []; - // Store the mapping of this value in case we visit it again, in case of circular data - memoizationMap.set(inputValue, returnValue); - - inputValue.forEach((item: unknown) => { - returnValue.push(_dropUndefinedKeys(item, memoizationMap)); - }); - - return returnValue as unknown as T; + return (val as any[]).map(item => { + return _dropUndefinedKeys(item, memo); + }) as any; } - return inputValue; + return val; } /** diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 3e5d8eb03e36..0b889c8a92f5 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -200,34 +200,28 @@ describe('dropUndefinedKeys()', () => { }); }); - test('should not throw on objects with circular reference', () => { - const chicken: any = { + test('objects with circular reference', () => { + const dog: any = { food: undefined, }; - const egg = { - edges: undefined, - contains: chicken, + const human = { + brain: undefined, + pets: dog, }; - chicken.lays = egg; - - const droppedChicken = dropUndefinedKeys(chicken); - - // Removes undefined keys - expect(Object.keys(droppedChicken)).toEqual(['lays']); - expect(Object.keys(droppedChicken.lays)).toEqual(['contains']); - - // Returns new object - expect(chicken === droppedChicken).toBe(false); - expect(chicken.lays === droppedChicken.lays).toBe(false); + const rat = { + scares: human, + weight: '4kg', + }; - // Returns new references within objects - expect(chicken === droppedChicken.lays.contains).toBe(false); - expect(egg === droppedChicken.lays.contains.lays).toBe(false); + dog.chases = rat; - // Keeps circular reference - expect(droppedChicken.lays.contains === droppedChicken).toBe(true); + expect(dropUndefinedKeys(human)).toStrictEqual({ + pets: { + chases: rat, + }, + }); }); test('arrays with circular reference', () => { From 4d6a9198d475f583e0ffc4fbf50c484d000d543d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 3 Jun 2022 12:00:22 +0000 Subject: [PATCH 8/8] Fix rebasing stuff --- packages/utils/src/object.ts | 62 +++++++++++++++++++----------- packages/utils/test/object.test.ts | 36 +++++++++-------- 2 files changed, 61 insertions(+), 37 deletions(-) diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index 5957214ecc86..5b789b29060d 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -4,7 +4,6 @@ import { WrappedFunction } from '@sentry/types'; import { htmlTreeAsString } from './browser'; import { isElement, isError, isEvent, isInstanceOf, isPlainObject, isPrimitive } from './is'; -import { memoBuilder, MemoFunc } from './memo'; import { truncate } from './string'; /** @@ -204,42 +203,61 @@ export function extractExceptionKeysForMessage(exception: Record(val: T): T { +export function dropUndefinedKeys(inputValue: T): T { + // This map keeps track of what already visited nodes map to. + // Our Set - based memoBuilder doesn't work here because we want to the output object to have the same circular + // references as the input object. + const memoizationMap = new Map(); + // This function just proxies `_dropUndefinedKeys` to keep the `memoBuilder` out of this function's API - return _dropUndefinedKeys(val, memoBuilder()); + return _dropUndefinedKeys(inputValue, memoizationMap); } -function _dropUndefinedKeys(val: T, memo: MemoFunc): T { - const [memoize] = memo; // we don't need unmemoize because we don't need to visit nodes twice - - if (isPlainObject(val)) { - if (memoize(val)) { - return val; +function _dropUndefinedKeys(inputValue: T, memoizationMap: Map): T { + if (isPlainObject(inputValue)) { + // If this node has already been visited due to a circular reference, return the object it was mapped to in the new object + const memoVal = memoizationMap.get(inputValue); + if (memoVal !== undefined) { + return memoVal as T; } - const rv: { [key: string]: any } = {}; - for (const key of Object.keys(val)) { - if (typeof val[key] !== 'undefined') { - rv[key] = _dropUndefinedKeys(val[key], memo); + + const returnValue: { [key: string]: any } = {}; + // Store the mapping of this value in case we visit it again, in case of circular data + memoizationMap.set(inputValue, returnValue); + + for (const key of Object.keys(inputValue)) { + if (typeof inputValue[key] !== 'undefined') { + returnValue[key] = _dropUndefinedKeys(inputValue[key], memoizationMap); } } - return rv as T; + + return returnValue as T; } - if (Array.isArray(val)) { - if (memoize(val)) { - return val; + if (Array.isArray(inputValue)) { + // If this node has already been visited due to a circular reference, return the array it was mapped to in the new object + const memoVal = memoizationMap.get(inputValue); + if (memoVal !== undefined) { + return memoVal as T; } - return (val as any[]).map(item => { - return _dropUndefinedKeys(item, memo); - }) as any; + + const returnValue: unknown[] = []; + // Store the mapping of this value in case we visit it again, in case of circular data + memoizationMap.set(inputValue, returnValue); + + inputValue.forEach((item: unknown) => { + returnValue.push(_dropUndefinedKeys(item, memoizationMap)); + }); + + return returnValue as unknown as T; } - return val; + return inputValue; } /** diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 0b889c8a92f5..3e5d8eb03e36 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -200,28 +200,34 @@ describe('dropUndefinedKeys()', () => { }); }); - test('objects with circular reference', () => { - const dog: any = { + test('should not throw on objects with circular reference', () => { + const chicken: any = { food: undefined, }; - const human = { - brain: undefined, - pets: dog, + const egg = { + edges: undefined, + contains: chicken, }; - const rat = { - scares: human, - weight: '4kg', - }; + chicken.lays = egg; - dog.chases = rat; + const droppedChicken = dropUndefinedKeys(chicken); - expect(dropUndefinedKeys(human)).toStrictEqual({ - pets: { - chases: rat, - }, - }); + // Removes undefined keys + expect(Object.keys(droppedChicken)).toEqual(['lays']); + expect(Object.keys(droppedChicken.lays)).toEqual(['contains']); + + // Returns new object + expect(chicken === droppedChicken).toBe(false); + expect(chicken.lays === droppedChicken.lays).toBe(false); + + // Returns new references within objects + expect(chicken === droppedChicken.lays.contains).toBe(false); + expect(egg === droppedChicken.lays.contains.lays).toBe(false); + + // Keeps circular reference + expect(droppedChicken.lays.contains === droppedChicken).toBe(true); }); test('arrays with circular reference', () => {