fix: setContext correctly processes non-plain JS object (#4168)

krystofwoldrich · antonis · web-flow · commit ac41368cedb5 · 2024-10-15T08:53:44.000Z
Co-authored-by: Antonis Lilis &lt;antonis.lilis@gmail.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,9 @@
 ### Fixes
 
 - TimetoTisplay correctly warns about not supporting the new React Native architecture ([#4160](https://github.com/getsentry/sentry-react-native/pull/4160))
+- Native Wrapper method `setContext` ensures only values convertible to NativeMap are passed ([#4168](https://github.com/getsentry/sentry-react-native/pull/4168))
+- Native Wrapper method `setExtra` ensures only stringified values are passed ([#4168](https://github.com/getsentry/sentry-react-native/pull/4168))
+- `setContext('key', null)` removes the key value also from platform context ([#4168](https://github.com/getsentry/sentry-react-native/pull/4168))
 
 ## 5.34.0
 
diff --git a/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
@@ -639,18 +639,29 @@ public void clearBreadcrumbs() {
     }
 
     public void setExtra(String key, String extra) {
+        if (key == null || extra == null) {
+            logger.log(SentryLevel.ERROR, "RNSentry.setExtra called with null key or value, can't change extra.");
+            return;
+        }
+
         Sentry.configureScope(scope -> {
             scope.setExtra(key, extra);
         });
     }
 
     public void setContext(final String key, final ReadableMap context) {
-        if (key == null || context == null) {
+        if (key == null) {
+            logger.log(SentryLevel.ERROR, "RNSentry.setContext called with null key, can't change context.");
             return;
         }
+
         Sentry.configureScope(scope -> {
-            final HashMap<String, Object> contextHashMap = context.toHashMap();
+            if (context == null) {
+                scope.removeContexts(key);
+                return;
+            }
 
+            final HashMap<String, Object> contextHashMap = context.toHashMap();
             scope.setContexts(key, contextHashMap);
         });
     }
diff --git a/ios/RNSentry.mm b/ios/RNSentry.mm
@@ -587,8 +587,16 @@ - (NSDictionary*) fetchNativeStackFramesBy: (NSArray<NSNumber*>*)instructionsAdd
                   context:(NSDictionary *)context
 )
 {
+    if (key == nil) {
+        return;
+    }
+
     [SentrySDK configureScope:^(SentryScope * _Nonnull scope) {
-        [scope setContextValue:context forKey:key];
+        if (context == nil) {
+            [scope removeContextForKey:key];
+        } else {
+            [scope setContextValue:context forKey:key];
+        }
     }];
 }
 
diff --git a/src/js/wrapper.ts b/src/js/wrapper.ts
@@ -28,6 +28,7 @@ import type { NativeAndroidProfileEvent, NativeProfileEvent } from './profiling/
 import type { MobileReplayOptions } from './replay/mobilereplay';
 import type { RequiredKeysUser } from './user';
 import { isTurboModuleEnabled } from './utils/environment';
+import { convertToNormalizedObject } from './utils/normalize';
 import { ReactNativeLibraries } from './utils/rnlibraries';
 import { base64StringFromByteArray, utf8ToBytes } from './vendor';
 
@@ -84,7 +85,8 @@ interface SentryNativeWrapper {
   enableNativeFramesTracking(): void;
 
   addBreadcrumb(breadcrumb: Breadcrumb): void;
-  setContext(key: string, context: { [key: string]: unknown } | null): void;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  setContext(key: string, context: { [key: string]: any } | null): void;
   clearBreadcrumbs(): void;
   setExtra(key: string, extra: unknown): void;
   setUser(user: User | null): void;
@@ -395,10 +397,25 @@ export const NATIVE: SentryNativeWrapper = {
       throw this._NativeClientError;
     }
 
-    // we stringify the extra as native only takes in strings.
-    const stringifiedExtra = typeof extra === 'string' ? extra : JSON.stringify(extra);
+    if (typeof extra === 'string') {
+      return RNSentry.setExtra(key, extra);
+    }
+    if (typeof extra === 'undefined') {
+      return RNSentry.setExtra(key, 'undefined');
+    }
+
+    let stringifiedExtra: string | undefined;
+    try {
+      const normalizedExtra = normalize(extra);
+      stringifiedExtra = JSON.stringify(normalizedExtra);
+    } catch (e) {
+      logger.error('Extra for key ${key} not passed to native SDK, because it contains non-stringifiable values', e);
+    }
 
-    RNSentry.setExtra(key, stringifiedExtra);
+    if (typeof stringifiedExtra === 'string') {
+      return RNSentry.setExtra(key, stringifiedExtra);
+    }
+    return RNSentry.setExtra(key, '**non-stringifiable**');
   },
 
   /**
@@ -435,19 +452,35 @@ export const NATIVE: SentryNativeWrapper = {
   },
 
   /**
-   * Sets context on the native scope. Not implemented in Android yet.
+   * Sets context on the native scope.
    * @param key string
    * @param context key-value map
    */
-  setContext(key: string, context: { [key: string]: unknown } | null): void {
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  setContext(key: string, context: { [key: string]: any } | null): void {
     if (!this.enableNative) {
       return;
     }
     if (!this._isModuleLoaded(RNSentry)) {
       throw this._NativeClientError;
     }
 
-    RNSentry.setContext(key, context !== null ? normalize(context) : null);
+    if (context === null) {
+      return RNSentry.setContext(key, null);
+    }
+
+    let normalizedContext: Record<string, unknown> | undefined;
+    try {
+      normalizedContext = convertToNormalizedObject(context);
+    } catch (e) {
+      logger.error('Context for key ${key} not passed to native SDK, because it contains non-serializable values', e);
+    }
+
+    if (normalizedContext) {
+      RNSentry.setContext(key, normalizedContext);
+    } else {
+      RNSentry.setContext(key, { error: '**non-serializable**' });
+    }
   },
 
   /**
diff --git a/test/wrapper.test.ts b/test/wrapper.test.ts
@@ -623,4 +623,122 @@ describe('Tests Native Wrapper', () => {
       expect(NATIVE.stopProfiling()).toBe(null);
     });
   });
+
+  describe('setExtra', () => {
+    test('passes string value to native method', () => {
+      NATIVE.setExtra('key', 'string value');
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', 'string value');
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+
+    test('stringifies number value before passing to native method', () => {
+      NATIVE.setExtra('key', 42);
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', '42');
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+
+    test('stringifies boolean value before passing to native method', () => {
+      NATIVE.setExtra('key', true);
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', 'true');
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+
+    test('stringifies object value before passing to native method', () => {
+      const obj = { foo: 'bar', baz: 123 };
+      NATIVE.setExtra('key', obj);
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', JSON.stringify(obj));
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+
+    test('stringifies array value before passing to native method', () => {
+      const arr = [1, 'two', { three: 3 }];
+      NATIVE.setExtra('key', arr);
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', JSON.stringify(arr));
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+
+    test('handles null value by stringifying', () => {
+      NATIVE.setExtra('key', null);
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', 'null');
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+
+    test('handles undefined value by stringifying', () => {
+      NATIVE.setExtra('key', undefined);
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', 'undefined');
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+
+    test('handles non-serializable value by stringifying', () => {
+      const circular: { self?: unknown } = {};
+      circular.self = circular;
+      NATIVE.setExtra('key', circular);
+      expect(RNSentry.setExtra).toHaveBeenCalledWith('key', '{"self":"[Circular ~]"}');
+      expect(RNSentry.setExtra).toHaveBeenCalledOnce();
+    });
+  });
+
+  describe('setContext', () => {
+    test('passes plain JS object to native method', () => {
+      const context = { foo: 'bar', baz: 123 };
+      NATIVE.setContext('key', context);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', context);
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('converts non-plain JS object to plain object before passing to native method', () => {
+      class TestClass {
+        prop = 'value';
+      }
+      const context = new TestClass();
+      NATIVE.setContext('key', context);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', { prop: 'value' });
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('converts array to object with "value" key before passing to native method', () => {
+      const context = [1, 'two', { three: 3 }];
+      NATIVE.setContext('key', context);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', { value: [1, 'two', { three: 3 }] });
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('converts string primitive to object with "value" key before passing to native method', () => {
+      NATIVE.setContext('key', 'string value' as unknown as object);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', { value: 'string value' });
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('converts number primitive to object with "value" key before passing to native method', () => {
+      NATIVE.setContext('key', 42 as unknown as object);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', { value: 42 });
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('converts boolean primitive to object with "value" key before passing to native method', () => {
+      NATIVE.setContext('key', true as unknown as object);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', { value: true });
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('handles null value by passing null to native method', () => {
+      NATIVE.setContext('key', null);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', null);
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('handles undefined value by converting to object with "value" key', () => {
+      NATIVE.setContext('key', undefined as unknown as object);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', { value: undefined });
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+
+    test('handles non-serializable value by converting to normalized object', () => {
+      const circular: { self?: unknown } = {};
+      circular.self = circular;
+      NATIVE.setContext('key', circular);
+      expect(RNSentry.setContext).toHaveBeenCalledWith('key', { self: '[Circular ~]' });
+      expect(RNSentry.setContext).toHaveBeenCalledOnce();
+    });
+  });
 });

Original file line number	Diff line number	Diff line change
`@@ -587,8 +587,16 @@ - (NSDictionary) fetchNativeStackFramesBy: (NSArray<NSNumber>*)instructionsAdd`
`587`	`587`	`context:(NSDictionary *)context`
`588`	`588`	`)`
`589`	`589`	`{`
	`590`	`+ if (key == nil) {`
	`591`	`+ return;`
	`592`	`+ }`
	`593`	`+`
`590`	`594`	`[SentrySDK configureScope:^(SentryScope * _Nonnull scope) {`
`591`		`- [scope setContextValue:context forKey:key];`
	`595`	`+ if (context == nil) {`
	`596`	`+ [scope removeContextForKey:key];`
	`597`	`+ } else {`
	`598`	`+ [scope setContextValue:context forKey:key];`
	`599`	`+ }`
`592`	`600`	`}];`
`593`	`601`	`}`
`594`	`602`