From a3712749433c1ad536f4faf9a94520fabc9c791e Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Mon, 14 Oct 2024 13:15:26 +0200 Subject: [PATCH 1/4] fix: setContext correctly processes not plain JS object --- CHANGELOG.md | 2 + .../io/sentry/react/RNSentryModuleImpl.java | 15 ++- ios/RNSentry.mm | 10 +- src/js/wrapper.ts | 45 ++++++- test/wrapper.test.ts | 116 ++++++++++++++++++ 5 files changed, 179 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88edc6e3e..b2845a4d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ - Handles error with string cause ([#4163](https://github.com/getsentry/sentry-react-native/pull/4163)) - Use `appLaunchedInForeground` to determine invalid app start data on Android ([#4146](https://github.com/getsentry/sentry-react-native/pull/4146)) - Upload source maps for all release variants on Android (not only the last found) ([#4125](https://github.com/getsentry/sentry-react-native/pull/4125)) +- 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)) ### Dependencies diff --git a/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java index 6103b3b76..ecb41f166 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 d2254aa5c..a04733e7b 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 ed1bd7d1f..b123f6d75 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 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**'); }, /** @@ -439,7 +456,8 @@ export const NATIVE: SentryNativeWrapper = { * @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 not passed to native SDK, because it contains non-serializable values', e); + } + + if (normalizedContext) { + return RNSentry.setContext(key, normalizedContext); + } else { + return RNSentry.setContext(key, { error: '**non-serializable**' }); + } }, /** diff --git a/test/wrapper.test.ts b/test/wrapper.test.ts index 2e06ed5c8..457343f9d 100644 --- a/test/wrapper.test.ts +++ b/test/wrapper.test.ts @@ -623,4 +623,120 @@ 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); + }); + + 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 ~]' }); + }); + }); }); From 9ed26d0182408875ac296fcf9553ba4d5e0686dd Mon Sep 17 00:00:00 2001 From: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:16:36 +0200 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Antonis Lilis --- src/js/wrapper.ts | 4 ++-- test/wrapper.test.ts | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/js/wrapper.ts b/src/js/wrapper.ts index b123f6d75..d976cd017 100644 --- a/src/js/wrapper.ts +++ b/src/js/wrapper.ts @@ -409,7 +409,7 @@ export const NATIVE: SentryNativeWrapper = { const normalizedExtra = normalize(extra); stringifiedExtra = JSON.stringify(normalizedExtra); } catch (e) { - logger.error('Extra not passed to native SDK, because it contains non-stringifiable values', e); + logger.error('Extra for key ${key} not passed to native SDK, because it contains non-stringifiable values', e); } if (typeof stringifiedExtra === 'string') { @@ -473,7 +473,7 @@ export const NATIVE: SentryNativeWrapper = { try { normalizedContext = convertToNormalizedObject(context); } catch (e) { - logger.error('Context not passed to native SDK, because it contains non-serializable values', e); + logger.error('Context for key ${key} not passed to native SDK, because it contains non-serializable values', e); } if (normalizedContext) { diff --git a/test/wrapper.test.ts b/test/wrapper.test.ts index 457343f9d..e66cb4f64 100644 --- a/test/wrapper.test.ts +++ b/test/wrapper.test.ts @@ -724,6 +724,7 @@ describe('Tests Native Wrapper', () => { 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', () => { @@ -737,6 +738,7 @@ describe('Tests Native Wrapper', () => { circular.self = circular; NATIVE.setContext('key', circular); expect(RNSentry.setContext).toHaveBeenCalledWith('key', { self: '[Circular ~]' }); + expect(RNSentry.setContext).toHaveBeenCalledOnce(); }); }); }); From 718ad0712f181ac324a59f47b1db52e3d57e8c1f Mon Sep 17 00:00:00 2001 From: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:19:14 +0200 Subject: [PATCH 3/4] Update wrapper.ts --- src/js/wrapper.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/js/wrapper.ts b/src/js/wrapper.ts index d976cd017..ac9915d7c 100644 --- a/src/js/wrapper.ts +++ b/src/js/wrapper.ts @@ -452,7 +452,7 @@ 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 */ @@ -477,9 +477,9 @@ export const NATIVE: SentryNativeWrapper = { } if (normalizedContext) { - return RNSentry.setContext(key, normalizedContext); + RNSentry.setContext(key, normalizedContext); } else { - return RNSentry.setContext(key, { error: '**non-serializable**' }); + RNSentry.setContext(key, { error: '**non-serializable**' }); } }, From 6fd081329532c64f2b7954e925e468845ea0e991 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Date: Tue, 15 Oct 2024 10:50:51 +0200 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0967ba5f8..87fd11a5f 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 @@ -13,8 +16,6 @@ - Handles error with string cause ([#4163](https://github.com/getsentry/sentry-react-native/pull/4163)) - Use `appLaunchedInForeground` to determine invalid app start data on Android ([#4146](https://github.com/getsentry/sentry-react-native/pull/4146)) - Upload source maps for all release variants on Android (not only the last found) ([#4125](https://github.com/getsentry/sentry-react-native/pull/4125)) -- 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)) ### Dependencies