diff --git a/CHANGELOG.md b/CHANGELOG.md index d5bf8d0bca..71743a9e19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ### TBD +### Fixed + +- (react-native) Handle circular references in metadata before it's sent to the native notifier layer [#1673](https://github.com/bugsnag/bugsnag-js/pull/1673) + ### Changed - (react-native) Update bugsnag-android to v5.19.2 - New APIs to support forthcoming feature flag and experiment functionality. For more information, please see https://docs.bugsnag.com/product/features-experiments. diff --git a/packages/core/lib/derecursify.js b/packages/core/lib/derecursify.js new file mode 100644 index 0000000000..9c8938eba8 --- /dev/null +++ b/packages/core/lib/derecursify.js @@ -0,0 +1,82 @@ +const isArray = require('./es-utils/is-array') + +const isSafeLiteral = (obj) => ( + typeof obj === 'string' || obj instanceof String || + typeof obj === 'number' || obj instanceof Number || + typeof obj === 'boolean' || obj instanceof Boolean +) + +const isError = o => ( + o instanceof Error || /^\[object (Error|(Dom)?Exception)]$/.test(Object.prototype.toString.call(o)) +) + +const throwsMessage = err => '[Throws: ' + (err ? err.message : '?') + ']' + +const safelyGetProp = (obj, propName) => { + try { + return obj[propName] + } catch (err) { + return throwsMessage(err) + } +} + +/** + * Similar to `safe-json-stringify` this function rebuilds an object graph without any circular references. + * This requirement is different to `JSON.parse(safeJsonStringify(data))` in three key ways: + * - `toJSON` methods are not called + * - there is no redaction or fixed depth limit + * + * @param data the value to be made safe for the ReactNative bridge + * @returns a safe version of the given `data` + */ +module.exports = function (data) { + const seen = [] + + const visit = (obj) => { + if (obj === null || obj === undefined) return obj + + if (isSafeLiteral(obj)) { + return obj + } + + if (isError(obj)) { + return visit({ name: obj.name, message: obj.message }) + } + + if (obj instanceof Date) { + return obj.toISOString() + } + + if (seen.includes(obj)) { + // circular references are replaced and marked + return '[Circular]' + } + + // handle arrays, and all iterable non-array types (such as Set) + if (isArray(obj) || obj[Symbol.iterator]) { + seen.push(obj) + const safeArray = [] + try { + for (const value of obj) { + safeArray.push(visit(value)) + } + } catch (err) { + // if retrieving the Iterator fails + return throwsMessage(err) + } + seen.pop() + return safeArray + } + + seen.push(obj) + const safeObj = {} + for (const propName in obj) { + safeObj[propName] = visit(safelyGetProp(obj, propName)) + } + seen.pop() + + return safeObj + } + + return visit(data) +} diff --git a/packages/core/lib/test/derecursify.test.ts b/packages/core/lib/test/derecursify.test.ts new file mode 100644 index 0000000000..5e6cf12ae0 --- /dev/null +++ b/packages/core/lib/test/derecursify.test.ts @@ -0,0 +1,126 @@ +import derecursift from '../derecursify' + +describe('delivery: react native makeSafe', () => { + it('leaves simple types intact', () => { + const symbol = Symbol('symbol_field') + const date = new Date() + const data: any = { + string: 'hello', + number: -15.321, + bool: true, + date, + array: [ + 1, 2, 3, + 'string', + { nestedObject: true } + ], + nestedData: { + string: 'hello', + number: -15.321, + bool: true + }, + [symbol]: 'some value', + map: new Map([['key', 'value']]), + _null: null, + _undefined: undefined + } + + const result = derecursift(data) + + /* eslint-disable-next-line @typescript-eslint/no-dynamic-delete */ + delete data[symbol] // we don't copy Symbol keys over + expect(result).toStrictEqual({ + ...data, + // dates are converted to ISO strings + date: date.toISOString(), + // maps iterate as arrays of arrays + map: [ + ['key', 'value'] + ] + }) + }) + + describe('handles errors', () => { + it('when reading properties', () => { + const object: any = {} + Object.defineProperty(object, 'badProperty', { + get () { + throw new Error('failure') + }, + enumerable: true + }) + + const result = derecursift(object) + expect(result).toStrictEqual({ badProperty: '[Throws: failure]' }) + }) + + it('when they are properties', () => { + const value = { errorProp: new Error('something wrong') } + const result = derecursift(value) + expect(result).toStrictEqual({ errorProp: { name: 'Error', message: 'something wrong' } }) + }) + }) + + describe('handles circular references', () => { + it('when directly in objects', () => { + const object: { self?: any } = {} + object.self = object + + const result = derecursift(object) + expect(result).toStrictEqual({ self: '[Circular]' }) + }) + + it('when nested in objects', () => { + const outer: any = { + inner: {} + } + + outer.inner.parent = outer + + const result = derecursift(outer) + expect(result).toStrictEqual({ inner: { parent: '[Circular]' } }) + }) + + it('when in arrays', () => { + const array: any[] = [{}, {}] + array[0].circularRef = array + + const result = derecursift(array) + expect(result).toStrictEqual([{ circularRef: '[Circular]' }, {}]) + }) + + it('when in non-array iterables', () => { + const object: any = {} + const values = new Set() + values.add(object) + + object.container = values + + const result = derecursift(values) + expect(result).toStrictEqual([{ container: '[Circular]' }]) + }) + + it('when nested in objects within arrays', () => { + const metaData: any = { + from: 'javascript' + } + + // ensure that circular references are safely handled + metaData.circle = metaData + + const array = [{ + someObject: metaData + }] + + const result = derecursift(array) + expect(result).toStrictEqual([ + { + someObject: { + from: 'javascript', + circle: '[Circular]' + } + } + ]) + }) + }) +}) diff --git a/packages/delivery-react-native/delivery.js b/packages/delivery-react-native/delivery.js index 0a34d16c1c..044fa48afd 100644 --- a/packages/delivery-react-native/delivery.js +++ b/packages/delivery-react-native/delivery.js @@ -1,3 +1,5 @@ +const derecursify = require('@bugsnag/core/lib/derecursify') + module.exports = (client, NativeClient) => ({ sendEvent: (payload, cb = () => {}) => { const event = payload.events[0] @@ -17,10 +19,10 @@ module.exports = (client, NativeClient) => ({ app: event.app, device: event.device, threads: event.threads, - breadcrumbs: event.breadcrumbs, + breadcrumbs: derecursify(event.breadcrumbs), context: event.context, user: event._user, - metadata: event._metadata, + metadata: derecursify(event._metadata), groupingHash: event.groupingHash, apiKey: event.apiKey, featureFlags: event.toJSON().featureFlags, diff --git a/packages/delivery-react-native/test/delivery.test.ts b/packages/delivery-react-native/test/delivery.test.ts index 8b86555626..6abf877e41 100644 --- a/packages/delivery-react-native/test/delivery.test.ts +++ b/packages/delivery-react-native/test/delivery.test.ts @@ -46,8 +46,16 @@ describe('delivery: react native', () => { } } const c = new Client({ apiKey: 'api_key' }) + + const metaData: any = { + from: 'javascript' + } + + // ensure that circular references in metadata are safely handled + metaData.circle = metaData + c._setDelivery(client => delivery(client, NativeClient)) - c.leaveBreadcrumb('hi') + c.leaveBreadcrumb('hi', metaData, 'state') c.setContext('test screen') c.setUser('123') c.notify(new Error('oh no'), (e) => { @@ -65,6 +73,10 @@ describe('delivery: react native', () => { expect(sent[0].threads).toEqual([]) expect(sent[0].breadcrumbs.length).toBe(1) expect(sent[0].breadcrumbs[0].message).toBe('hi') + expect(sent[0].breadcrumbs[0].metadata).toStrictEqual({ + from: 'javascript', + circle: '[Circular]' + }) expect(sent[0].context).toBe('test screen') expect(sent[0].user).toEqual({ id: '123', email: undefined, name: undefined }) expect(sent[0].metadata).toEqual({}) diff --git a/packages/plugin-react-native-client-sync/client-sync.js b/packages/plugin-react-native-client-sync/client-sync.js index d2498a3eb7..5bb026bff3 100644 --- a/packages/plugin-react-native-client-sync/client-sync.js +++ b/packages/plugin-react-native-client-sync/client-sync.js @@ -1,4 +1,5 @@ const { DeviceEventEmitter, NativeEventEmitter, NativeModules, Platform } = require('react-native') +const derecursify = require('@bugsnag/core/lib/derecursify') module.exports = (NativeClient) => ({ load: (client) => { @@ -7,7 +8,7 @@ module.exports = (NativeClient) => ({ // to JSON() method doesn't get called before passing the object over the // bridge. This happens in the remote debugger and means the "message" // property is incorrectly named "name" - NativeClient.leaveBreadcrumb({ ...breadcrumb }) + NativeClient.leaveBreadcrumb(derecursify(breadcrumb)) }, true) const origSetUser = client.setUser @@ -28,9 +29,9 @@ module.exports = (NativeClient) => ({ client.addMetadata = function (section, key, value) { const ret = origAddMetadata.apply(this, arguments) if (typeof key === 'object') { - NativeClient.addMetadata(section, key) + NativeClient.addMetadata(section, derecursify(key)) } else { - NativeClient.addMetadata(section, { [key]: value }) + NativeClient.addMetadata(section, { [key]: derecursify(value) }) } return ret } diff --git a/test/react-native/features/fixtures/app/scenario_js/app/scenarios/BreadcrumbsJsManualScenario.js b/test/react-native/features/fixtures/app/scenario_js/app/scenarios/BreadcrumbsJsManualScenario.js index ab9ac32d30..1e4543894d 100644 --- a/test/react-native/features/fixtures/app/scenario_js/app/scenarios/BreadcrumbsJsManualScenario.js +++ b/test/react-native/features/fixtures/app/scenario_js/app/scenarios/BreadcrumbsJsManualScenario.js @@ -6,6 +6,10 @@ export class BreadcrumbsJsManualScenario extends Scenario { const metaData = { from: 'javascript' } + + // ensure that circular references are safely handled + metaData.circle = metaData + Bugsnag.leaveBreadcrumb('oh crumbs', metaData, 'state') Bugsnag.notify(new Error('BreadcrumbsJsManualScenario')) } diff --git a/test/react-native/features/fixtures/app/scenario_js/app/scenarios/MetadataJsScenario.js b/test/react-native/features/fixtures/app/scenario_js/app/scenarios/MetadataJsScenario.js index afcee417bd..198da2e3a2 100644 --- a/test/react-native/features/fixtures/app/scenario_js/app/scenarios/MetadataJsScenario.js +++ b/test/react-native/features/fixtures/app/scenario_js/app/scenarios/MetadataJsScenario.js @@ -13,8 +13,13 @@ export class MetadataJsScenario extends Scenario { } run () { + const recursiveMetadata = {} + recursiveMetadata.data = 'some valid data' + recursiveMetadata.circle = recursiveMetadata + Bugsnag.addMetadata('jsdata', 'some_more_data', 'set via client') Bugsnag.addMetadata('jsdata', 'redacted_data', 'not present') + Bugsnag.addMetadata('jsdata', 'recursive', recursiveMetadata) Bugsnag.notify(new Error('MetadataJsScenario'), (event) => { event.addMetadata('jsdata', 'even_more_data', 'set via event') event.addMetadata('jsarraydata', 'items', ['a', 'b', 'c']) diff --git a/test/react-native/features/fixtures/expected_breadcrumbs/JsManualScenario.json b/test/react-native/features/fixtures/expected_breadcrumbs/JsManualScenario.json index e90f8012b6..7c9246f4f1 100644 --- a/test/react-native/features/fixtures/expected_breadcrumbs/JsManualScenario.json +++ b/test/react-native/features/fixtures/expected_breadcrumbs/JsManualScenario.json @@ -3,6 +3,7 @@ "name": "oh crumbs", "timestamp": "^\\d{4}\\-\\d{2}\\-\\d{2}T\\d{2}:\\d{2}:[\\d\\.]+Z?$", "metaData": { - "from": "javascript" + "from": "javascript", + "circle": "[Circular]" } -} \ No newline at end of file +} diff --git a/test/react-native/features/metadata.feature b/test/react-native/features/metadata.feature index 5ebb95b7f4..fb098da2d0 100644 --- a/test/react-native/features/metadata.feature +++ b/test/react-native/features/metadata.feature @@ -9,6 +9,8 @@ Scenario: Setting metadata (JS) And the event "metaData.jsdata.some_more_data" equals "set via client" And the event "metaData.jsdata.even_more_data" equals "set via event" And the event "metaData.jsdata.redacted_data" equals "[REDACTED]" + And the event "metaData.jsdata.recursive.data" equals "some valid data" + And the event "metaData.jsdata.recursive.circle" equals "[Circular]" And the error payload field "events.0.metaData.jsarraydata.items" is an array with 3 elements Scenario: Setting metadata (native handled)