fix(core): Prevent exception recapturing (#4067)

When we instrument code to catch exceptions, once we've recorded the error we rethrow it, so that whatever would have happened without us still happens. Sometimes, that means that we'll see the same exception (as in, literally the same instance of `Error`) again, because it will hit some other code we've instrumented.

The clearest example of this is us catching an exception in, say, an event handler, recording it, rethrowing it, and it then hitting our wrapped global `onerror`. We already handle that particular case, but the nextjs SDK has introduced other places where this happens, which our current solution doesn't handle.

(Specifically, because we instrument the code in different (but overlapping) ways to compensate for differences between Vercel and non-Vercel environments, in non-Vercel environments we can end up seeing errors get caught twice.)

This prevents us from capturing the same error twice by flagging the object as seen and bailing if we encounter an error with the flag set. Since people do all sorts of crazy things, and throw primitives (especially strings), it also accounts for that case, by wrapping any primitive it encounters with its corresponding wrapper class. Finally, it takes advantage of this change in `@sentry/nextjs`'s `withSentry` wrapper, to prevent the cases mentioned above.

(Note: This is a different case than the `Dedupe` integration is meant to handle, which is two copies of the same error in a row. In this case, though the thing which gets thrown is the same the first and second times we see it, the stacktrace to get to it being captured is different, since different instrumentation is involved each time.)

Author: lobsterkatie

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