Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
15 changes: 13 additions & 2 deletions android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand Down
10 changes: 9 additions & 1 deletion ios/RNSentry.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}];
}

Expand Down
45 changes: 39 additions & 6 deletions src/js/wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
}

RNSentry.setExtra(key, stringifiedExtra);
if (typeof stringifiedExtra === 'string') {
return RNSentry.setExtra(key, stringifiedExtra);
}
return RNSentry.setExtra(key, '**non-stringifiable**');
},

/**
Expand Down Expand Up @@ -439,15 +456,31 @@ 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 {
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
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 not passed to native SDK, because it contains non-serializable values', e);
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
}

if (normalizedContext) {
return RNSentry.setContext(key, normalizedContext);
} else {
return RNSentry.setContext(key, { error: '**non-serializable**' });
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
}
},

/**
Expand Down
116 changes: 116 additions & 0 deletions test/wrapper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
});

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 ~]' });
krystofwoldrich marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
Loading