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.

Author: lobsterkatie

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];
       }
     }
 

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();
     });
   });
 

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;
+};