From 5e2ddf336685e1f3b7a34fd4de87c73b41a8ae37 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 5 Aug 2021 13:02:36 -0700 Subject: [PATCH] fix(utils): Fix false-positive circular references when normalizing `Event` objects (#3864) When `captureException` is passed a non-`Error` object, we do our best to extract as much data as we can from that object. (That's what leads to the much-maligned "Non-Error exception [or promise rejection] captured with keys x, y, and z" error message.) A common case in which this occurs is when code of the form `Promise.reject(someEvent)` runs - common enough, in fact, that we handle `Event` objects separately. Specifically, we capture the event's `type`, `target` (the element which caused the event), and `currentTarget` (the element with the event listener on it) properties (none of which are enumerable), along with anything else on the event which _is_ enumerable. For most events, that "anything else" includes only one property: `isTrusted`, a boolean indicating whether or not the event is the result of a user action. For a long time, though, `isTrusted` has been showing up not as a boolean but as `[Circular ~]`. It turns out that's because when we try to grab the enumerable property values, we end up grabbing the entire event instead. (It's only shown up in the `isTrusted` value, but only because that's the only enumerable property on most `Event`s.) This fixes that, and adds a test for pulling data out of `Event` objects. --- packages/utils/src/object.ts | 10 ++-- packages/utils/test/object.test.ts | 74 +++++++++++++++++++++--------- packages/utils/test/testutils.ts | 13 ++++++ 3 files changed, 72 insertions(+), 25 deletions(-) create mode 100644 packages/utils/test/testutils.ts diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index 14156e9b1372..c06ea5877ddc 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -108,9 +108,11 @@ function getWalkSource( [key: string]: any; } = {}; + // Accessing event attributes can throw (see https://github.com/getsentry/sentry-javascript/issues/768 and + // https://github.com/getsentry/sentry-javascript/issues/838), but accessing `type` hasn't been wrapped in a + // try-catch in at least two years and no one's complained, so that's likely not an issue anymore source.type = event.type; - // Accessing event.target can throw (see getsentry/raven-js#838, #768) try { source.target = isElement(event.target) ? htmlTreeAsString(event.target) @@ -131,9 +133,9 @@ function getWalkSource( source.detail = event.detail; } - for (const i in event) { - if (Object.prototype.hasOwnProperty.call(event, i)) { - source[i] = event; + for (const attr in event) { + if (Object.prototype.hasOwnProperty.call(event, attr)) { + source[attr] = event[attr]; } } diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 2dfbe452fe15..674f665e6349 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -1,4 +1,10 @@ +/** + * @jest-environment jsdom + */ + +import * as isModule from '../src/is'; import { dropUndefinedKeys, extractExceptionKeysForMessage, fill, normalize, urlEncode } from '../src/object'; +import { testOnlyIfNodeVersionAtLeast } from './testutils'; describe('fill()', () => { test('wraps a method by calling a replacement function on it', () => { @@ -119,28 +125,54 @@ describe('normalize()', () => { }); }); - test('extracts extra properties from error objects', () => { - const obj = new Error('Wubba Lubba Dub Dub') as any; - obj.reason = new TypeError("I'm pickle Riiick!"); - obj.extra = 'some extra prop'; - - obj.stack = 'x'; - obj.reason.stack = 'x'; - - // IE 10/11 - delete obj.description; - delete obj.reason.description; - - expect(normalize(obj)).toEqual({ - message: 'Wubba Lubba Dub Dub', - name: 'Error', - stack: 'x', - reason: { - message: "I'm pickle Riiick!", - name: 'TypeError', + describe('getWalkSource()', () => { + test('extracts extra properties from error objects', () => { + const obj = new Error('Wubba Lubba Dub Dub') as any; + obj.reason = new TypeError("I'm pickle Riiick!"); + obj.extra = 'some extra prop'; + + obj.stack = 'x'; + obj.reason.stack = 'x'; + + // IE 10/11 + delete obj.description; + delete obj.reason.description; + + expect(normalize(obj)).toEqual({ + message: 'Wubba Lubba Dub Dub', + name: 'Error', stack: 'x', - }, - extra: 'some extra prop', + reason: { + message: "I'm pickle Riiick!", + name: 'TypeError', + stack: 'x', + }, + extra: 'some extra prop', + }); + }); + + testOnlyIfNodeVersionAtLeast(8)('extracts data from `Event` objects', () => { + const isElement = jest.spyOn(isModule, 'isElement').mockReturnValue(true); + const getAttribute = () => undefined; + + const parkElement = { tagName: 'PARK', getAttribute }; + const treeElement = { tagName: 'TREE', parentNode: parkElement, getAttribute }; + const squirrelElement = { tagName: 'SQUIRREL', parentNode: treeElement, getAttribute }; + + const chaseEvent = new Event('chase'); + Object.defineProperty(chaseEvent, 'target', { value: squirrelElement }); + Object.defineProperty(chaseEvent, 'currentTarget', { value: parkElement }); + Object.defineProperty(chaseEvent, 'wagging', { value: true, enumerable: false }); + + expect(normalize(chaseEvent)).toEqual({ + currentTarget: 'park', + isTrusted: false, + target: 'park > tree > squirrel', + type: 'chase', + // notice that `wagging` isn't included because it's not enumerable and not one of the ones we specifically extract + }); + + isElement.mockRestore(); }); }); diff --git a/packages/utils/test/testutils.ts b/packages/utils/test/testutils.ts new file mode 100644 index 000000000000..6130ee7ca365 --- /dev/null +++ b/packages/utils/test/testutils.ts @@ -0,0 +1,13 @@ +export const testOnlyIfNodeVersionAtLeast = (minVersion: number): jest.It => { + const currentNodeVersion = process.env.NODE_VERSION; + + try { + if (Number(currentNodeVersion?.split('.')[0]) < minVersion) { + return it.skip; + } + } catch (oO) { + // we can't tell, so err on the side of running the test + } + + return it; +};