From ac41368cedb5f4e78bd1019e9a859ea193c3fb1a Mon Sep 17 00:00:00 2001 From: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Date: Tue, 15 Oct 2024 10:53:44 +0200 Subject: [PATCH] fix: setContext correctly processes non-plain JS object (#4168) Co-authored-by: Antonis Lilis --- CHANGELOG.md | 3 + .../io/sentry/react/RNSentryModuleImpl.java | 15 ++- ios/RNSentry.mm | 10 +- src/js/wrapper.ts | 47 +++++-- test/wrapper.test.ts | 118 ++++++++++++++++++ 5 files changed, 183 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d837a3037d..87fd11a5f8 100644 --- 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 index 6103b3b76f..ecb41f1665 100644 --- 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 contextHashMap = context.toHashMap(); + if (context == null) { + scope.removeContexts(key); + return; + } + final HashMap contextHashMap = context.toHashMap(); scope.setContexts(key, contextHashMap); }); } diff --git a/ios/RNSentry.mm b/ios/RNSentry.mm index d2254aa5c5..a04733e7b5 100644 --- a/ios/RNSentry.mm +++ b/ios/RNSentry.mm @@ -587,8 +587,16 @@ - (NSDictionary*) fetchNativeStackFramesBy: (NSArray*)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 index ed1bd7d1f0..ac9915d7c8 100644 --- 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,11 +452,12 @@ 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; } @@ -447,7 +465,22 @@ export const NATIVE: SentryNativeWrapper = { throw this._NativeClientError; } - RNSentry.setContext(key, context !== null ? normalize(context) : null); + if (context === null) { + return RNSentry.setContext(key, null); + } + + let normalizedContext: Record | 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 index 2e06ed5c8b..e66cb4f64e 100644 --- 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(); + }); + }); });