fix(core): Prevent exception recapturing

packages/core/src/baseclient.ts

@@ -13,6 +13,7 @@ import {
   Transport,
 } from '@sentry/types';
 import {
+  checkOrSetAlreadyCaught,
   dateTimestampInSeconds,
   Dsn,
   isPlainObject,
@@ -29,6 +30,8 @@ import {
 import { Backend, BackendClass } from './basebackend';
 import { IntegrationIndex, setupIntegrations } from './integration';
 
+const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured.";
+
 /**
  * Base implementation for all JavaScript SDK clients.
  *
@@ -101,6 +104,12 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
    */
   // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
   public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined {
+    // ensure we haven't captured this very object before
+    if (checkOrSetAlreadyCaught(exception)) {
+      logger.log(ALREADY_SEEN_ERROR);
+      return;
+    }
+
     let eventId: string | undefined = hint && hint.event_id;
 
     this._process(
@@ -140,6 +149,12 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
    * @inheritDoc
    */
   public captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined {
+    // ensure we haven't captured this very object before
+    if (hint?.originalException && checkOrSetAlreadyCaught(hint.originalException)) {
+      logger.log(ALREADY_SEEN_ERROR);
+      return;
+    }
+
     let eventId: string | undefined = hint && hint.event_id;
 
     this._process(

packages/core/test/lib/base.test.ts

@@ -12,6 +12,9 @@ const PUBLIC_DSN = 'https://username@domain/123';
 // eslint-disable-next-line no-var
 declare var global: any;
 
+const backendEventFromException = jest.spyOn(TestBackend.prototype, 'eventFromException');
+const clientProcess = jest.spyOn(TestClient.prototype as any, '_process');
+
 jest.mock('@sentry/utils', () => {
   const original = jest.requireActual('@sentry/utils');
   return {
@@ -57,7 +60,7 @@ describe('BaseClient', () => {
   });
 
   afterEach(() => {
-    jest.restoreAllMocks();
+    jest.clearAllMocks();
   });
 
   describe('constructor() / getDsn()', () => {
@@ -249,6 +252,30 @@ describe('BaseClient', () => {
         }),
       );
     });
+
+    test.each([
+      ['`Error` instance', new Error('Will I get caught twice?')],
+      ['plain object', { 'Will I': 'get caught twice?' }],
+      ['primitive wrapper', new String('Will I get caught twice?')],
+      // Primitives aren't tested directly here because they need to be wrapped with `objectify` *before*  being passed
+      // to `captureException` (which is how we'd end up with a primitive wrapper as tested above) in order for the
+      // already-seen check to work . Any primitive which is passed without being wrapped will be captured each time it
+      // is encountered, so this test doesn't apply.
+    ])("doesn't capture the same exception twice - %s", (_name: string, thrown: any) => {
+      const client = new TestClient({ dsn: PUBLIC_DSN });
+
+      expect(thrown.__sentry_captured__).toBeUndefined();
+
+      client.captureException(thrown);
+
+      expect(thrown.__sentry_captured__).toBe(true);
+      expect(backendEventFromException).toHaveBeenCalledTimes(1);
+
+      client.captureException(thrown);
+
+      // `captureException` should bail right away this second time around and not get as far as calling this again
+      expect(backendEventFromException).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe('captureMessage', () => {
@@ -325,6 +352,35 @@ describe('BaseClient', () => {
       expect(TestBackend.instance!.event).toBeUndefined();
     });
 
+    test.each([
+      ['`Error` instance', new Error('Will I get caught twice?')],
+      ['plain object', { 'Will I': 'get caught twice?' }],
+      ['primitive wrapper', new String('Will I get caught twice?')],
+      // Primitives aren't tested directly here because they need to be wrapped with `objectify` *before*  being passed
+      // to `captureEvent` (which is how we'd end up with a primitive wrapper as tested above) in order for the
+      // already-seen check to work . Any primitive which is passed without being wrapped will be captured each time it
+      // is encountered, so this test doesn't apply.
+    ])("doesn't capture an event wrapping the same exception twice - %s", (_name: string, thrown: any) => {
+      // Note: this is the same test as in `describe(captureException)`, except with the exception already wrapped in a
+      // hint and accompanying an event. Duplicated here because some methods skip `captureException` and go straight to
+      // `captureEvent`.
+      const client = new TestClient({ dsn: PUBLIC_DSN });
+      const event = { exception: { values: [{ type: 'Error', message: 'Will I get caught twice?' }] } };
+      const hint = { originalException: thrown };
+
+      expect(thrown.__sentry_captured__).toBeUndefined();
+
+      client.captureEvent(event, hint);
+
+      expect(thrown.__sentry_captured__).toBe(true);
+      expect(clientProcess).toHaveBeenCalledTimes(1);
+
+      client.captureEvent(event, hint);
+
+      // `captureEvent` should bail right away this second time around and not get as far as calling this again
+      expect(clientProcess).toHaveBeenCalledTimes(1);
+    });
+
     test('sends an event', () => {
       expect.assertions(2);
       const client = new TestClient({ dsn: PUBLIC_DSN });
@@ -798,7 +854,7 @@ describe('BaseClient', () => {
       expect(TestBackend.instance!.event).toBeUndefined();
     });
 
-    test('calls beforeSend gets an access to a hint as a second argument', () => {
+    test('beforeSend gets access to a hint as a second argument', () => {
       expect.assertions(2);
       const beforeSend = jest.fn((event, hint) => ({ ...event, data: hint.data }));
       const client = new TestClient({ dsn: PUBLIC_DSN, beforeSend });

packages/nextjs/src/utils/withSentry.ts

@@ -1,7 +1,7 @@
 import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node';
 import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing';
 import { Transaction } from '@sentry/types';
-import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils';
+import { addExceptionMechanism, isString, logger, objectify, stripUrlQueryAndFragment } from '@sentry/utils';
 import * as domain from 'domain';
 import { NextApiHandler, NextApiResponse } from 'next';
 
@@ -76,6 +76,12 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
       try {
         return await origHandler(req, res);
       } catch (e) {
+        // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can
+        // store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced
+        // to take, it can happen that the same thrown object gets caught in two different ways, and flagging it is a
+        // way to prevent it from actually being reported twice.)
+        const objectifiedErr = objectify(e);
+
         if (currentScope) {
           currentScope.addEventProcessor(event => {
             addExceptionMechanism(event, {
@@ -88,9 +94,14 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
             });
             return event;
           });
-          captureException(e);
+
+          captureException(objectifiedErr);
         }
-        throw e;
+
+        // We rethrow here so that nextjs can do with the error whatever it would normally do. (Sometimes "whatever it
+        // would normally do" is to allow the error to bubble up to the global handlers - another reason we need to mark
+        // the error as already having been captured.)
+        throw objectifiedErr;
       }
     });
 

packages/utils/src/misc.ts

@@ -237,3 +237,43 @@ export function stripUrlQueryAndFragment(urlPath: string): string {
   // eslint-disable-next-line no-useless-escape
   return urlPath.split(/[\?#]/, 1)[0];
 }
+
+/**
+ * Checks whether or not we've already captured the given exception (note: not an identical exception - the very object
+ * in question), and marks it captured if not.
+ *
+ * This is useful because it's possible for an error to get captured by more than one mechanism. After we intercept and
+ * record an error, we rethrow it (assuming we've intercepted it before it's reached the top-level global handlers), so
+ * that we don't interfere with whatever effects the error might have had were the SDK not there. At that point, because
+ * the error has been rethrown, it's possible for it to bubble up to some other code we've instrumented. If it's not
+ * caught after that, it will bubble all the way up to the global handlers (which of course we also instrument). This
+ * function helps us ensure that even if we encounter the same error more than once, we only record it the first time we
+ * see it.
+ *
+ * Note: It will ignore primitives (always return `false` and not mark them as seen), as properties can't be set on
+ * them. {@link: Object.objectify} can be used on exceptions to convert any that are primitives into their equivalent
+ * object wrapper forms so that this check will always work. However, because we need to flag the exact object which
+ * will get rethrown, and because that rethrowing happens outside of the event processing pipeline, the objectification
+ * must be done before the exception captured.
+ *
+ * @param A thrown exception to check or flag as having been seen
+ * @returns `true` if the exception has already been captured, `false` if not (with the side effect of marking it seen)
+ */
+export function checkOrSetAlreadyCaught(exception: unknown): boolean {
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+  if ((exception as any)?.__sentry_captured__) {
+    return true;
+  }
+
+  try {
+    // set it this way rather than by assignment so that it's not ennumerable and therefore isn't recorded by the
+    // `ExtraErrorData` integration
+    Object.defineProperty(exception, '__sentry_captured__', {
+      value: true,
+    });
+  } catch (err) {
+    // `exception` is a primitive, so we can't mark it seen
+  }
+
+  return false;
+}

packages/utils/src/object.ts

@@ -398,3 +398,40 @@ export function dropUndefinedKeys<T>(val: T): T {
 
   return val;
 }
+
+/**
+ * Ensure that something is an object.
+ *
+ * Turns `undefined` and `null` into `String`s and all other primitives into instances of their respective wrapper
+ * classes (String, Boolean, Number, etc.). Acts as the identity function on non-primitives.
+ *
+ * @param wat The subject of the objectification
+ * @returns A version of `wat` which can safely be used with `Object` class methods
+ */
+export function objectify(wat: unknown): typeof Object {
+  let objectified;
+  switch (true) {
+    case wat === undefined || wat === null:
+      objectified = new String(wat);
+      break;
+
+    // Though symbols and bigints do have wrapper classes (`Symbol` and `BigInt`, respectively), for whatever reason
+    // those classes don't have constructors which can be used with the `new` keyword. We therefore need to cast each as
+    // an object in order to wrap it.
+    case typeof wat === 'symbol' || typeof wat === 'bigint':
+      objectified = Object(wat);
+      break;
+
+    // this will catch the remaining primitives: `String`, `Number`, and `Boolean`
+    case isPrimitive(wat):
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+      objectified = new (wat as any).constructor(wat);
+      break;
+
+    // by process of elimination, at this point we know that `wat` must already be an object
+    default:
+      objectified = wat;
+      break;
+  }
+  return objectified;
+}

packages/utils/test/misc.test.ts

@@ -3,6 +3,7 @@ import { Event, Mechanism, StackFrame } from '@sentry/types';
 import {
   addContextToFrame,
   addExceptionMechanism,
+  checkOrSetAlreadyCaught,
   getEventDescription,
   parseRetryAfterHeader,
   stripUrlQueryAndFragment,
@@ -288,3 +289,33 @@ describe('addExceptionMechanism', () => {
     });
   });
 });
+
+describe('checkOrSetAlreadyCaught()', () => {
+  describe('ignores primitives', () => {
+    it.each([
+      ['undefined', undefined],
+      ['null', null],
+      ['number', 1231],
+      ['boolean', true],
+      ['string', 'Dogs are great!'],
+    ])('%s', (_case: string, exception: unknown): void => {
+      // in this case, "ignore" just means reporting them as unseen without actually doing anything to them (which of
+      // course it can't anyway, because primitives are immutable)
+      expect(checkOrSetAlreadyCaught(exception)).toBe(false);
+    });
+  });
+
+  it("recognizes exceptions it's seen before", () => {
+    // `exception` can be any object - an `Error`, a class instance, or a plain object
+    const exception = { message: 'Oh, no! Charlie ate the flip-flops! :-(', __sentry_captured__: true };
+
+    expect(checkOrSetAlreadyCaught(exception)).toBe(true);
+  });
+
+  it('recognizes new exceptions as new and marks them as seen', () => {
+    const exception = { message: 'Oh, no! Charlie ate the flip-flops! :-(' };
+
+    expect(checkOrSetAlreadyCaught(exception)).toBe(false);
+    expect((exception as any).__sentry_captured__).toBe(true);
+  });
+});

packages/utils/test/object.test.ts

@@ -3,7 +3,14 @@
  */
 
 import * as isModule from '../src/is';
-import { dropUndefinedKeys, extractExceptionKeysForMessage, fill, normalize, urlEncode } from '../src/object';
+import {
+  dropUndefinedKeys,
+  extractExceptionKeysForMessage,
+  fill,
+  normalize,
+  objectify,
+  urlEncode,
+} from '../src/object';
 import { testOnlyIfNodeVersionAtLeast } from './testutils';
 
 describe('fill()', () => {
@@ -638,3 +645,59 @@ describe('dropUndefinedKeys()', () => {
     });
   });
 });
+
+describe('objectify()', () => {
+  describe('stringifies nullish values', () => {
+    it.each([
+      ['undefined', undefined],
+      ['null', null],
+    ])('%s', (stringifiedValue, origValue): void => {
+      const objectifiedNullish = objectify(origValue);
+
+      expect(objectifiedNullish).toEqual(expect.any(String));
+      expect(objectifiedNullish.valueOf()).toEqual(stringifiedValue);
+    });
+  });
+
+  describe('wraps other primitives with their respective object wrapper classes', () => {
+    // TODO: There's currently a bug in Jest - if you give it the `Boolean` class, it runs `typeof received ===
+    // 'boolean'` but not `received instanceof Boolean` (the way it correctly does for other primitive wrappers, like
+    // `Number` and `String). (See https://github.com/facebook/jest/pull/11976.) Once that is fixed and we upgrade jest,
+    // we can comment the test below back in. (The tests for symbols and bigints are working only because our current
+    // version of jest is sufficiently old that they're not even considered in the relevant check and just fall to the
+    // default `instanceof` check jest uses for all unknown classes.)
+
+    it.each([
+      ['number', Number, 1121],
+      ['string', String, 'Dogs are great!'],
+      // ["boolean", Boolean, true],
+      ['symbol', Symbol, Symbol('Maisey')],
+    ])('%s', (_caseName, wrapperClass, primitive) => {
+      const objectifiedPrimitive = objectify(primitive);
+
+      expect(objectifiedPrimitive).toEqual(expect.any(wrapperClass));
+      expect(objectifiedPrimitive.valueOf()).toEqual(primitive);
+    });
+
+    // `BigInt` doesn't exist in Node < 10, so we test it separately here.
+    testOnlyIfNodeVersionAtLeast(10)('bigint', () => {
+      // Hack to get around the fact that literal bigints cause a syntax error in older versions of Node, so the
+      // assignment needs to not even be parsed as code in those versions
+      let bigintPrimitive;
+      eval('bigintPrimitive = 1231n;');
+
+      const objectifiedBigInt = objectify(bigintPrimitive);
+
+      expect(objectifiedBigInt).toEqual(expect.any(BigInt));
+      expect(objectifiedBigInt.valueOf()).toEqual(bigintPrimitive);
+    });
+  });
+
+  it('leaves objects alone', () => {
+    const notAPrimitive = new Object();
+    const objectifiedNonPrimtive = objectify(notAPrimitive);
+
+    // `.toBe()` tests on identity, so this shows no wrapping has occurred
+    expect(objectifiedNonPrimtive).toBe(notAPrimitive);
+  });
+});