fix(utils): Try-catch monkeypatching to handle frozen objects/functions

Author: mydea

packages/browser/test/unit/index.test.ts

@@ -1,4 +1,5 @@
 import { getReportDialogEndpoint, SDK_VERSION } from '@sentry/core';
+import { WrappedFunction } from '@sentry/types';
 import * as utils from '@sentry/utils';
 
 import type { Event } from '../../src';
@@ -391,4 +392,14 @@ describe('wrap()', () => {
 
     expect(result2).toBe(42);
   });
+
+  it('should ignore frozen functions', () => {
+    const func = Object.freeze(() => 42);
+
+    // eslint-disable-next-line deprecation/deprecation
+    wrap(func);
+
+    expect(func()).toBe(42);
+    expect((func as WrappedFunction).__sentry_wrapped__).toBeUndefined();
+  });
 });

packages/utils/src/object.ts

@@ -28,12 +28,7 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa
   // Make sure it's a function first, as we need to attach an empty prototype for `defineProperties` to work
   // otherwise it'll throw "TypeError: Object.defineProperties called on non-object"
   if (typeof wrapped === 'function') {
-    try {
-      markFunctionWrapped(wrapped, original);
-    } catch (_Oo) {
-      // This can throw if multiple fill happens on a global object like XMLHttpRequest
-      // Fixes https://github.com/getsentry/sentry-javascript/issues/2043
-    }
+    markFunctionWrapped(wrapped, original);
   }
 
   source[name] = wrapped;
@@ -47,12 +42,14 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa
  * @param value The value to which to set the property
  */
 export function addNonEnumerableProperty(obj: { [key: string]: unknown }, name: string, value: unknown): void {
-  Object.defineProperty(obj, name, {
-    // enumerable: false, // the default, so we can save on bundle size by not explicitly setting it
-    value: value,
-    writable: true,
-    configurable: true,
-  });
+  try {
+    Object.defineProperty(obj, name, {
+      // enumerable: false, // the default, so we can save on bundle size by not explicitly setting it
+      value: value,
+      writable: true,
+      configurable: true,
+    });
+  } catch (o_O) {} // eslint-disable-line no-empty
 }
 
 /**
@@ -63,9 +60,11 @@ export function addNonEnumerableProperty(obj: { [key: string]: unknown }, name:
  * @param original the original function that gets wrapped
  */
 export function markFunctionWrapped(wrapped: WrappedFunction, original: WrappedFunction): void {
-  const proto = original.prototype || {};
-  wrapped.prototype = original.prototype = proto;
-  addNonEnumerableProperty(wrapped, '__sentry_original__', original);
+  try {
+    const proto = original.prototype || {};
+    wrapped.prototype = original.prototype = proto;
+    addNonEnumerableProperty(wrapped, '__sentry_original__', original);
+  } catch (o_O) {} // eslint-disable-line no-empty
 }
 
 /**

packages/utils/test/object.test.ts

@@ -2,7 +2,16 @@
  * @jest-environment jsdom
  */
 
-import { dropUndefinedKeys, extractExceptionKeysForMessage, fill, objectify, urlEncode } from '../src/object';
+import { WrappedFunction } from '@sentry/types';
+import {
+  dropUndefinedKeys,
+  extractExceptionKeysForMessage,
+  fill,
+  objectify,
+  urlEncode,
+  addNonEnumerableProperty,
+  markFunctionWrapped,
+} from '../src/object';
 import { testOnlyIfNodeVersionAtLeast } from './testutils';
 
 describe('fill()', () => {
@@ -315,3 +324,95 @@ describe('objectify()', () => {
     expect(objectifiedNonPrimtive).toBe(notAPrimitive);
   });
 });
+
+describe('addNonEnumerableProperty', () => {
+  it('works with a plain object', () => {
+    const obj: { foo?: string } = {};
+    addNonEnumerableProperty(obj, 'foo', 'bar');
+    expect(obj.foo).toBe('bar');
+  });
+
+  it('works with a class', () => {
+    class MyClass {
+      public foo?: string;
+    }
+    const obj = new MyClass();
+    addNonEnumerableProperty(obj as any, 'foo', 'bar');
+    expect(obj.foo).toBe('bar');
+  });
+
+  it('works with a function', () => {
+    const func = jest.fn();
+    addNonEnumerableProperty(func as any, 'foo', 'bar');
+    expect((func as any).foo).toBe('bar');
+    func();
+    expect(func).toHaveBeenCalledTimes(1);
+  });
+
+  it('works with an existing property object', () => {
+    const obj = { foo: 'before' };
+    addNonEnumerableProperty(obj, 'foo', 'bar');
+    expect(obj.foo).toBe('bar');
+  });
+
+  it('works with an existing readonly property object', () => {
+    const obj = { foo: 'before' };
+
+    Object.defineProperty(obj, 'foo', {
+      value: 'defined',
+      writable: false,
+    });
+
+    addNonEnumerableProperty(obj, 'foo', 'bar');
+    expect(obj.foo).toBe('bar');
+  });
+
+  it('does not error with a frozen object', () => {
+    const obj = Object.freeze({ foo: 'before' });
+
+    addNonEnumerableProperty(obj, 'foo', 'bar');
+    expect(obj.foo).toBe('before');
+  });
+});
+
+describe('markFunctionWrapped', () => {
+  it('works with a function', () => {
+    const originalFunc = jest.fn();
+    const wrappedFunc = jest.fn();
+    markFunctionWrapped(wrappedFunc, originalFunc);
+
+    expect((wrappedFunc as WrappedFunction).__sentry_original__).toBe(originalFunc);
+
+    wrappedFunc();
+
+    expect(wrappedFunc).toHaveBeenCalledTimes(1);
+    expect(originalFunc).not.toHaveBeenCalled();
+  });
+
+  it('works with a frozen original function', () => {
+    const originalFunc = Object.freeze(jest.fn());
+    const wrappedFunc = jest.fn();
+    markFunctionWrapped(wrappedFunc, originalFunc);
+
+    expect((wrappedFunc as WrappedFunction).__sentry_original__).toBe(originalFunc);
+
+    wrappedFunc();
+
+    expect(wrappedFunc).toHaveBeenCalledTimes(1);
+    expect(originalFunc).not.toHaveBeenCalled();
+  });
+
+  it('works with a frozen wrapped function', () => {
+    const originalFunc = Object.freeze(jest.fn());
+    const wrappedFunc = Object.freeze(jest.fn());
+    markFunctionWrapped(wrappedFunc, originalFunc);
+
+    // Skips adding the property, but also doesn't error
+    expect((wrappedFunc as WrappedFunction).__sentry_original__).toBe(undefined);
+
+    wrappedFunc();
+
+    expect(wrappedFunc).toHaveBeenCalledTimes(1);
+    expect(originalFunc).not.toHaveBeenCalled();
+  });
+});