fix: serialize circular inputs without throwing

[Krisztiaan]

:loudspeaker: Type of change

• [x] Bugfix

:scroll: Description

Added a replacer for some JSON.serialize calls, to avoid unprotected throws due to circular constructs.

:bulb: Motivation and Context

Solves a potential crash when data from userland is circular. Resolves #1477

:green_heart: How did you test it?

Added unit test

:pencil: Checklist

• [x] I reviewed submitted code
• [x] I added tests to verify changes
• [x] All tests passing
• [x] No breaking changes

:crystal_ball: Next steps

🚢 If you could also advise where to put the replacer creator function, that'd be nice.

[marandaneto]

@Krisztiaan thanks for doing this. I will let @jennmueng and @AbhiPrasad review this since they own the JS know-how.

[Krisztiaan]

@Krisztiaan thanks for doing this. I will let @jennmueng and @AbhiPrasad review this since they own the JS know-how.

No problems, this is very close to what I ended up doing before we pass anything to sentry in our project.

[marandaneto]

@Krisztiaan btw:

/home/runner/work/sentry-react-native/sentry-react-native/test/wrapper.test.ts 576:0 error Parsing error: Declaration or statement expected

[AbhiPrasad]

We have the walk utility in the @sentry/utils package which does similar, we can probably just use that instead of a custom replacer: https://github.com/getsentry/sentry-javascript/blob/fcd97c44b6e9f38c6aa9abeee06368c88ab08ea5/packages/utils/src/object.ts#L312

See how it's used here: https://github.com/getsentry/sentry-javascript/blob/fcd97c44b6e9f38c6aa9abeee06368c88ab08ea5/packages/utils/src/object.ts#L360-L379

[github-actions[bot]]

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[marandaneto]

We have the walk utility in the @sentry/utils package which does similar, we can probably just use that instead of a custom replacer: getsentry/sentry-javascript@fcd97c4/packages/utils/src/object.ts#L312

See how it's used here: getsentry/sentry-javascript@fcd97c4/packages/utils/src/object.ts#L360-L379

@Krisztiaan would you like to update this PR to use the Sentry's utils method?

[Krisztiaan]

Thanks, I'll do a little bit of PR housekeeping this weekend.

[Krisztiaan]

@marandaneto the change is done, also saw a place to bring in normalize :)

[bruno-garcia]

Lining seems to have Failed:

Ideally we'd automate this as CI failures due to linting can be frustrating. But could you please fix this one?

/home/runner/work/sentry-react-native/sentry-react-native/src/js/wrapper.ts 9 2:1 error Run autofix to sort these imports! simple-import-sort/sort 10

11 /home/runner/work/sentry-react-native/sentry-react-native/test/wrapper.test.ts 12 576:0 error Parsing error: Declaration or statement expected 13

14 ✖ 2 problems (2 errors, 0 warnings) 15 1 error and 0 warnings potentially fixable with the --fix option.

[marandaneto]

@Krisztiaan lint is still unhappy.

11 /home/runner/work/sentry-react-native/sentry-react-native/test/wrapper.test.ts 12 576:0 error Parsing error: Declaration or statement expected 13

[Krisztiaan]

@marandaneto tests are failing with walk, due to different parsing logic than what's expected here. (serializeValue) Pushed so you can see the fail cases too. Advise on how you'd want to reconcile, please. Either test expectations are now wrong, or serializeValue does too much in walk to use here.

For something maybe a bit simpler, something like cycle.js could be used to decycle objects instead, so walk or a replacer would not be needed here.

[marandaneto]

@marandaneto tests are failing with walk, due to different parsing logic than what's expected here. (serializeValue) Pushed so you can see the fail cases too. Advise on how you'd want to reconcile, please. Either test expectations are now wrong, or serializeValue does too much in walk to use here.

For a more thorough example, something like cycle.js could be used to decycle objects, so a replacer would not be needed here.

mmm, Maybe @AbhiPrasad or @jennmueng can help here.

[Krisztiaan]

Do let me know what's up. Imo this could be somewhat important to deal with in a timely manner, as it can cause unexpected crashes, related to a crash detection lib, and that is not that cool. However easy to fix.

[marandaneto]

Do let me know what's up. Imo this could be somewhat important to deal with in a timely manner, as it can cause unexpected crashes, related to a crash detection lib, and that is not that cool. However easy to fix.

@AbhiPrasad @bruno-garcia @jennmueng ping, in case you all can help us to unblock this.

[marandaneto]

@Krisztiaan after checking this one again, I believe the problem is with the ReactNavigation class. https://github.com/getsentry/sentry-react-native/blob/main/src/js/tracing/reactnavigation.ts#L209-L216

The params should be normalized, since the Event itself, when sendEvent gets called, the Event has been already normalized by the BaseClient, https://github.com/getsentry/sentry-javascript/blob/master/packages/core/src/baseclient.ts#L385-L435

So your changes won't fix the original issue anyway.

There's also this: https://github.com/getsentry/sentry-javascript/blob/6.17.9/packages/core/src/baseclient.ts#L426-L434

[Krisztiaan]

This is not a react-navigation only issue. Pass a circular object to any of these calls, or just try the added tests from this PR without the functional modifications of this PR to see sentry breaking with JSON.stringify failing.

So your changes won't fix the original issue anyway.

According to the tests, this is a fix, but walk causes side-effects that you'd need to make a decision on, which version to use, or walk to be discarded, and another way of de-cycling used.

[marandaneto]

This is not a react-navigation only issue. Pass a circular object to any of these calls, or just try the added tests from this PR without the functional modifications of this PR to see sentry breaking with JSON.stringify failing.

So your changes won't fix the original issue anyway.

According to the tests, this is a fix, but walk causes side-effects that you'd need to make a decision on, which version to use, or walk to be discarded, and another way of de-cycling used.

I agree it's not a react-navigation only, but please take a look at https://github.com/getsentry/sentry-javascript/blob/master/packages/core/src/baseclient.ts?rgh-link-date=2022-03-21T13%3A25%3A54Z#L385-L435 The wrapper class gets the Event already normalized, so no need to do it again. I'd rather not patch the wrapper class anyway, if any of those methods need normalization, it should be done here -> https://github.com/getsentry/sentry-react-native/blob/main/src/js/scope.ts

So we normalize them even before calling the wrapper most likely.

[marandaneto]

A few things have changed since then, this should have been fixed, please let us know if not with the latest versions.