fix: ignore empty api key from react native #237

fractalwrench · 2018-05-08T13:34:06Z

Goal

Prevents crashes when a nil API key is present, by guarding against calling any code that uses an API key.

Design

This approach avoids crashes in the case that the Notifier has not been configured correctly, and maintains the existing API design.

Changeset

Checks on the length of the API key throughout the codebase.

Tests

Manual tests on React Native with no API key set in either the native/JS layer. JS error + auto session tracking were tested.

Discussion

Alternative Approaches

Throwing an exception in start was discussed but not pursued, due to concerns over unexpectedly crashing production applications due to a change in behaviour. Adding an error parameter to start was also discussed, which would be set if the API key was not present.

My main concern with this change is that it's not particularly intuitive that the API key can be null, and that we may not be warning users strongly enough that something is wrong with how they've configured Bugsnag.

I'm also concerned that it would be easy for a future developer to make a change that inadvertently accesses the API key and assumes it is non-null - which is an easy enough assumption to make as this is documented within the notifier spec and is the case in most of the other notifiers.

The concerns above aren't pre-requisites for this change so IMO we can pursue them at a later date, if we decide to change our public API to allow this.

Review

For the submitter, initial self-review:

Commented on code changes inline explain the reasoning behind the approach
Reviewed the test cases added for completeness and possible points for discussion
A changelog entry was added for the goal of this pull request
Check the scope of the changeset - is everything in the diff required for the pull request?
This pull request is ready for:
- Initial review of the intended approach, not yet feature complete
- Structural review of the classes, functions, and properties modified
- Final review

For the pull request reviewer(s), this changeset has been reviewed for:

Consistency across platforms for structures or concepts added or modified
Consistency between the changeset and the goal stated above
Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
Usage friction - is the proposed change in usage cumbersome or complicated?
Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
Concurrency concerns - if components are accessed asynchronously, what issues will arise
Thoroughness of added tests and any missing edge cases
Idiomatic use of the language

Ignores empty api keys when initialising react native, preferring to use the existing config.apiKey instead

martin308

One question around the nil check

martin308 · 2018-05-08T19:56:01Z

cocoa/BugsnagReactNative.m

@@ -215,7 +215,11 @@ + (void)startWithConfiguration:(BugsnagConfiguration *)config {
    NSString *appVersion = [RCTConvert NSString:options[@"appVersion"]];
    NSString *codeBundleId = [RCTConvert NSString:options[@"codeBundleId"]];
    BugsnagConfiguration* config = [Bugsnag bugsnagStarted] ? [Bugsnag configuration] : [BugsnagConfiguration new];
-    config.apiKey = apiKey;
+
+    if (apiKey != nil && apiKey.length > 0) {


do we need to check for nil here? It seems like on line 208 we just check the length

No, if the value is nil, then length is coerced to 0.

kattrali · 2018-05-10T05:31:05Z

How can / will this change be tested?

fractalwrench · 2018-05-10T07:56:42Z

@kattrali I think in the short-term we should test this manually, and longer-term via mazerunner when that's setup.

fractalwrench · 2018-05-11T13:12:01Z

@kattrali I've updated the PR and the description - I think this is ready for a structural review now. The vendored Cocoa code will also need patching into the main bugsnag-cocoa repo.

kattrali · 2018-05-15T21:11:15Z

cocoa/BugsnagReactNative.m

@@ -215,7 +215,11 @@ + (void)startWithConfiguration:(BugsnagConfiguration *)config {
    NSString *appVersion = [RCTConvert NSString:options[@"appVersion"]];
    NSString *codeBundleId = [RCTConvert NSString:options[@"codeBundleId"]];
    BugsnagConfiguration* config = [Bugsnag bugsnagStarted] ? [Bugsnag configuration] : [BugsnagConfiguration new];
-    config.apiKey = apiKey;
+
+    if (apiKey.length > 0) {


This looks like the right solution on the bugsnag-react-native side

kattrali · 2018-05-15T21:44:36Z

cocoa/vendor/bugsnag-cocoa/Source/BugsnagSessionTracker.m

@@ -96,7 +96,7 @@ - (void)send {
        }
        BugsnagSessionTrackingPayload *payload = [[BugsnagSessionTrackingPayload alloc] initWithSessions:sessions];

-        if (payload.sessions.count > 0) {
+        if (payload.sessions.count > 0 && self.config.apiKey.length > 0) {


This check (and the one in getBodyFromReports) should instead be much earlier in the process of sending requests. Calls to the public interface (in Bugsnag.h) and automatic events from lifecycle callbacks should check if API key is nil and no-op if that is the case. +[Bugsnag bugsnagStarted] could potentially be beefed up a bit to check both whether notifier and API key are nil.

You shouldnt be able to set the apikey to nil using a config accessor, and if you initialize with nil it should no op and not set the notifier static variable?

Well yeah, API key should be not be nullable and initializing with a configuration object should require an API key.

fractalwrench · 2018-05-16T09:13:36Z

@kattrali @snmaynard I'm a bit unclear on what further actions need to be taken to get this PR to a mergeable state. From the feedback so far, it's suggested that we either:

(A) Prevent initialisation of the Notifier entirely when an API key is not supplied, by not instantiating anything.
(B) Add a check to every public method in Bugsnag in bugsnag-cocoa and BugsnagReactNative in bugsnag-react-native, that no-ops if the notifier has not been initialised, and also add checks to any automatic events (session tracking, lifecycle, etc).

Questions for option A would include:

What should we do if the API key is configured for the native layer but not React Native (et vice versa)?
Throwing exceptions in Obj-C is generally discouraged, but we need to warn the user the notifier has been misconfigured - is logging a warning sufficient here?
How we document the behaviour change for anyone using the notifiers in this unintended way.

Questions for option B would include:

Would we log a warning or simply no-op?
How would we prevent against future developers forgetting that the API key can be null (for instance, if they mistakenly add a public method that assumes the API key is a valid value). I think that this would be very easy to do accidentally considering that most of the other notifiers enforce a valid API key.

kattrali · 2018-05-18T05:51:36Z

(A) Prevent initialisation of the Notifier entirely when an API key is not supplied, by not instantiating anything.

Correct, the notifier should not be instantiated when API key is nil.

What should we do if the API key is configured for the native layer but not React Native (et vice versa)?

JS- and native-layer reports are delivered through the native layer so then it doesn't matter if the JS layer is initialized with no API key, as it is shared from the existing client/notifier on the native side. In the other direction, if an API key is provided to the JS layer but not the native layer, JS initialization passes the key to startWithOptions on the native side and it should override nil (or an existing value).

Throwing exceptions in Obj-C is generally discouraged, but we need to warn the user the notifier has been misconfigured - is logging a warning sufficient here?

A warning and allowing the application to continue without Bugsnag activated will suffice.

How we document the behaviour change for anyone using the notifiers in this unintended way.

Since setting the API key from either the native layer or the JS layer works, the only unintended way is not setting an API key at all. No additional documentation should be required for this case.

(B) Add a check to every public method in Bugsnag in bugsnag-cocoa and BugsnagReactNative in bugsnag-react-native, that no-ops if the notifier has not been initialised, and also add checks to any automatic events (session tracking, lifecycle, etc).

For most methods in Bugsnag.h this is already the case - bugsnagStarted gates functionality preventing methods from being called without the notifier being initialized and issues a warning. Adding the check to automatic events is a natural next step, as well as ensuring the rest of the public interface of Bugsnag.h is covered.

Would we log a warning or simply no-op?

Log a warning, in line with existing behavior.

How would we prevent against future developers forgetting that the API key can be null (for instance, if they mistakenly add a public method that assumes the API key is a valid value). I think that this would be very easy to do accidentally considering that most of the other notifiers enforce a valid API key.

Stronger style, review, and linting guidelines. For example, having a general rule for not putting nullable values into dictionary literals unchecked, even when that value should not be nil.

Separate from those concerns, I want to point out that this changeset was prompted by initializing with an non-nil API key but then later setting it to nil. There should be a guard against that scenario. Setting API key to nil should be a no-op which warns.

fractalwrench · 2018-05-18T11:00:02Z

@kattrali thanks for the comprehensive response. I have updated this PR so that it guards the necessary calls with bugsnagStarted, and made similar alterations in bugsnag-cocoa: bugsnag/bugsnag-cocoa#280

I believe this should resolve this issue and #214 as the notifier won't initialise if the API key is nil, and will NOP on public methods if this is the case. Do we still want to leave in nil checks deeper within the notifier (e.g. in BugsnagSessionTracker)?

We will of course need to update our vendored Cocoa code assuming the changeset in #280 is ok - I've left this task out for now, pending review.

snmaynard · 2018-05-19T18:27:22Z

Yeah - I think this works. We should ensure that the vendored code is the same as in bugsnag-cocoa so those changes should be undone.

fractalwrench · 2018-05-21T08:48:32Z

I have updated the vendored code so that there aren't any changes. This PR is now blocked until bugsnag/bugsnag-cocoa#280 is approved.

$fractalwrench$

$@fractalwrench$

fix: ignore empty api key from react native

4816169

Ignores empty api keys when initialising react native, preferring to use the existing config.apiKey instead

$@fractalwrench$ fractalwrench requested a review from a team May 8, 2018 13:34

martin308 reviewed May 8, 2018

View reviewed changes

fractalwrench added 2 commits May 11, 2018 13:53

$@fractalwrench$

fix: guard against api key by avoiding error/session paths

3ec0fb8

$@fractalwrench$

Merge branch 'master' into api-key-plist-fix

3f74b34

$@fractalwrench$ fractalwrench requested a review from a team May 11, 2018 16:12

kattrali suggested changes May 15, 2018

View reviewed changes

$@fractalwrench$

Merge branch 'master' into api-key-plist-fix

f47fdd5

$@fractalwrench$ fractalwrench mentioned this pull request May 18, 2018

Enforce requiring API key to initialise notifier bugsnag/bugsnag-cocoa#280

Merged

15 tasks

$@fractalwrench$

fix: Check bugsnag has started on all React Native Cocoa methods

593920d

$@fractalwrench$

remove unnecessary nil checks from vendored cocoa code

84ab0de

$@fractalwrench$ fractalwrench requested a review from kattrali May 21, 2018 08:48

kattrali approved these changes May 22, 2018

View reviewed changes

$@fractalwrench$

Merge branch 'master' into api-key-plist-fix

83cf568

$@fractalwrench$ fractalwrench merged commit 741dcca into master May 24, 2018

$@fractalwrench$ fractalwrench deleted the api-key-plist-fix branch May 24, 2018 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ignore empty api key from react native #237

fix: ignore empty api key from react native #237

$@fractalwrench$ fractalwrench commented May 8, 2018 •

edited

Loading

martin308 left a comment

martin308 May 8, 2018

kattrali May 8, 2018

kattrali commented May 10, 2018

fractalwrench commented May 10, 2018

fractalwrench commented May 11, 2018

kattrali May 15, 2018

kattrali May 15, 2018

snmaynard May 15, 2018

kattrali May 15, 2018

fractalwrench commented May 16, 2018

kattrali commented May 18, 2018

fractalwrench commented May 18, 2018

snmaynard commented May 19, 2018

fractalwrench commented May 21, 2018

fix: ignore empty api key from react native #237

fix: ignore empty api key from react native #237

Conversation

fractalwrench commented May 8, 2018 • edited Loading

Goal

Design

Changeset

Tests

Discussion

Alternative Approaches

Review

martin308 left a comment

Choose a reason for hiding this comment

martin308 May 8, 2018

Choose a reason for hiding this comment

kattrali May 8, 2018

Choose a reason for hiding this comment

kattrali commented May 10, 2018

fractalwrench commented May 10, 2018

fractalwrench commented May 11, 2018

kattrali May 15, 2018

Choose a reason for hiding this comment

kattrali May 15, 2018

Choose a reason for hiding this comment

snmaynard May 15, 2018

Choose a reason for hiding this comment

kattrali May 15, 2018

Choose a reason for hiding this comment

fractalwrench commented May 16, 2018

kattrali commented May 18, 2018

fractalwrench commented May 18, 2018

snmaynard commented May 19, 2018

fractalwrench commented May 21, 2018

$@fractalwrench$ fractalwrench commented May 8, 2018 •

edited

Loading