diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 46d5d78eb1ae..1b58ad5c318f 100644 --- a/packages/core/src/baseclient.ts +++ b/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 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 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( diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 64218d8ff97e..f29607e5ac94 100644 --- a/packages/core/test/lib/base.test.ts +++ b/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 }); diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 23b4fab2d65f..f7792070f400 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/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; } }); diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index a40ac5d01547..164e98950c61 100644 --- a/packages/utils/src/misc.ts +++ b/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; +} diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index fe78d6c2765c..9f5161ea9764 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -398,3 +398,40 @@ export function dropUndefinedKeys(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; +} diff --git a/packages/utils/test/misc.test.ts b/packages/utils/test/misc.test.ts index 440e14ddb1fd..7f013122bcc5 100644 --- a/packages/utils/test/misc.test.ts +++ b/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); + }); +}); diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 674f665e6349..dffa23e22408 100644 --- a/packages/utils/test/object.test.ts +++ b/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); + }); +});