From 084e88c1ea7201e6e12a343ce28e17c77aeef37a Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 18 May 2018 10:34:21 +0100 Subject: [PATCH 1/8] test: Enforce non-nil api key Alter the API key annotation to Nonnull rather than Nullable. If the API key is set as nil, the value is ignored and a warning logged. --- Source/BugsnagConfiguration.h | 2 +- Source/BugsnagConfiguration.m | 15 +++++++++++++++ Tests/BugsnagConfigurationTests.m | 16 ++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/Source/BugsnagConfiguration.h b/Source/BugsnagConfiguration.h index 095281c5d..e600520b0 100644 --- a/Source/BugsnagConfiguration.h +++ b/Source/BugsnagConfiguration.h @@ -69,7 +69,7 @@ typedef NSDictionary *_Nullable (^BugsnagBeforeNotifyHook)( /** * The API key of a Bugsnag project */ -@property(readwrite, retain, nullable) NSString *apiKey; +@property(readwrite, retain, nonnull) NSString *apiKey; /** * The URL used to notify Bugsnag */ diff --git a/Source/BugsnagConfiguration.m b/Source/BugsnagConfiguration.m index 1e78ac7d7..e4eb21190 100644 --- a/Source/BugsnagConfiguration.m +++ b/Source/BugsnagConfiguration.m @@ -31,6 +31,7 @@ #import "BSG_RFC3339DateTool.h" #import "BugsnagUser.h" #import "BugsnagSessionTracker.h" +#import "BugsnagLogger.h" static NSString *const kHeaderApiPayloadVersion = @"Bugsnag-Payload-Version"; static NSString *const kHeaderApiKey = @"Bugsnag-Api-Key"; @@ -199,6 +200,20 @@ - (void)setAppVersion:(NSString *)newVersion { } } +@synthesize apiKey = _apiKey; + +- (NSString *)apiKey { + return _apiKey; +} + +- (void)setApiKey:(NSString *)apiKey { + if (apiKey != nil) { + _apiKey = apiKey; + } else { + bsg_log_err(@"Attempted to override non-null API key with nil - ignoring."); + } +} + @synthesize shouldAutoCaptureSessions = _shouldAutoCaptureSessions; - (BOOL)shouldAutoCaptureSessions { diff --git a/Tests/BugsnagConfigurationTests.m b/Tests/BugsnagConfigurationTests.m index 3bc751783..286e3fad3 100644 --- a/Tests/BugsnagConfigurationTests.m +++ b/Tests/BugsnagConfigurationTests.m @@ -99,4 +99,20 @@ - (void)testUser { } +- (void)testApiKeySetter { + BugsnagConfiguration *config = [BugsnagConfiguration new]; + XCTAssertEqualObjects(@"", config.apiKey); + + config.apiKey = @"test"; + XCTAssertEqualObjects(@"test", config.apiKey); + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wnonnull" + config.apiKey = nil; +#pragma clang diagnostic pop + + XCTAssertEqualObjects(@"test", config.apiKey); +} + + @end From 1200634e1987b80e9dd548badd30f10286dfb160 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 18 May 2018 10:51:05 +0100 Subject: [PATCH 2/8] feat: Gate Bugsnag calls with check to see if notifier initialised Gates all methods in Bugsnag.m so that they no-op and log a warning if the notifier has not been initialised. Also avoids initialising the notifier if the API key is not supplied. --- Source/Bugsnag.m | 166 ++++++++++++++++++++++++++++------------------- 1 file changed, 101 insertions(+), 65 deletions(-) diff --git a/Source/Bugsnag.m b/Source/Bugsnag.m index 22692ea41..1d2f1d35c 100644 --- a/Source/Bugsnag.m +++ b/Source/Bugsnag.m @@ -51,9 +51,13 @@ + (void)startBugsnagWithApiKey:(NSString *)apiKey { + (void)startBugsnagWithConfiguration:(BugsnagConfiguration *)configuration { @synchronized(self) { - bsg_g_bugsnag_notifier = - [[BugsnagNotifier alloc] initWithConfiguration:configuration]; - [bsg_g_bugsnag_notifier start]; + if (configuration != nil && configuration.apiKey != nil) { + bsg_g_bugsnag_notifier = + [[BugsnagNotifier alloc] initWithConfiguration:configuration]; + [bsg_g_bugsnag_notifier start]; + } else { + bsg_log_err(@"Bugsnag not initialized - a valid API key must be supplied."); + } } } @@ -73,75 +77,87 @@ + (BugsnagNotifier *)notifier { } + (void)notify:(NSException *)exception { - [self.notifier notifyException:exception - block:^(BugsnagCrashReport *_Nonnull report) { - report.depth += 2; - }]; + if ([self bugsnagStarted]) { + [self.notifier notifyException:exception + block:^(BugsnagCrashReport *_Nonnull report) { + report.depth += 2; + }]; + } } + (void)notify:(NSException *)exception block:(BugsnagNotifyBlock)block { - [[self notifier] notifyException:exception - block:^(BugsnagCrashReport *_Nonnull report) { - report.depth += 2; - - if (block) { - block(report); - } - }]; + if ([self bugsnagStarted]) { + [[self notifier] notifyException:exception + block:^(BugsnagCrashReport *_Nonnull report) { + report.depth += 2; + + if (block) { + block(report); + } + }]; + } } + (void)notifyError:(NSError *)error { - [self.notifier notifyError:error - block:^(BugsnagCrashReport *_Nonnull report) { - report.depth += 2; - }]; + if ([self bugsnagStarted]) { + [self.notifier notifyError:error + block:^(BugsnagCrashReport *_Nonnull report) { + report.depth += 2; + }]; + } } + (void)notifyError:(NSError *)error block:(BugsnagNotifyBlock)block { - [[self notifier] notifyError:error - block:^(BugsnagCrashReport *_Nonnull report) { - report.depth += 2; - - if (block) { - block(report); - } - }]; + if ([self bugsnagStarted]) { + [[self notifier] notifyError:error + block:^(BugsnagCrashReport *_Nonnull report) { + report.depth += 2; + + if (block) { + block(report); + } + }]; + } } + (void)notify:(NSException *)exception withData:(NSDictionary *)metaData { - - [[self notifier] - notifyException:exception - block:^(BugsnagCrashReport *_Nonnull report) { - report.depth += 2; - report.metaData = [metaData - BSG_mergedInto:[self.notifier.configuration - .metaData toDictionary]]; - }]; + if ([self bugsnagStarted]) { + [[self notifier] + notifyException:exception + block:^(BugsnagCrashReport *_Nonnull report) { + report.depth += 2; + report.metaData = [metaData + BSG_mergedInto:[self.notifier.configuration + .metaData toDictionary]]; + }]; + } } + (void)notify:(NSException *)exception withData:(NSDictionary *)metaData atSeverity:(NSString *)severity { - - [[self notifier] - notifyException:exception - atSeverity:BSGParseSeverity(severity) - block:^(BugsnagCrashReport *_Nonnull report) { - report.depth += 2; - report.metaData = [metaData - BSG_mergedInto:[self.notifier.configuration - .metaData toDictionary]]; - report.severity = BSGParseSeverity(severity); - }]; + if ([self bugsnagStarted]) { + [[self notifier] + notifyException:exception + atSeverity:BSGParseSeverity(severity) + block:^(BugsnagCrashReport *_Nonnull report) { + report.depth += 2; + report.metaData = [metaData + BSG_mergedInto:[self.notifier.configuration + .metaData toDictionary]]; + report.severity = BSGParseSeverity(severity); + }]; + } } + (void)internalClientNotify:(NSException *_Nonnull)exception withData:(NSDictionary *_Nullable)metaData block:(BugsnagNotifyBlock _Nullable)block { - [self.notifier internalClientNotify:exception - withData:metaData - block:block]; + if ([self bugsnagStarted]) { + [self.notifier internalClientNotify:exception + withData:metaData + block:block]; + } } + (void)addAttribute:(NSString *)attributeName @@ -171,31 +187,43 @@ + (BOOL)bugsnagStarted { } + (void)leaveBreadcrumbWithMessage:(NSString *)message { - [self leaveBreadcrumbWithBlock:^(BugsnagBreadcrumb *_Nonnull crumbs) { - crumbs.metadata = @{BSGKeyMessage : message}; - }]; + if ([self bugsnagStarted]) { + [self leaveBreadcrumbWithBlock:^(BugsnagBreadcrumb *_Nonnull crumbs) { + crumbs.metadata = @{BSGKeyMessage: message}; + }]; + } } + (void)leaveBreadcrumbWithBlock: (void (^_Nonnull)(BugsnagBreadcrumb *_Nonnull))block { - [self.notifier addBreadcrumbWithBlock:block]; + if ([self bugsnagStarted]) { + [self.notifier addBreadcrumbWithBlock:block]; + } } + (void)leaveBreadcrumbForNotificationName: (NSString *_Nonnull)notificationName { - [self.notifier crumbleNotification:notificationName]; + if ([self bugsnagStarted]) { + [self.notifier crumbleNotification:notificationName]; + } } + (void)setBreadcrumbCapacity:(NSUInteger)capacity { - self.notifier.configuration.breadcrumbs.capacity = capacity; + if ([self bugsnagStarted]) { + self.notifier.configuration.breadcrumbs.capacity = capacity; + } } + (void)clearBreadcrumbs { - [self.notifier clearBreadcrumbs]; + if ([self bugsnagStarted]) { + [self.notifier clearBreadcrumbs]; + } } + (void)startSession { - [self.notifier startSession]; + if ([self bugsnagStarted]) { + [self.notifier startSession]; + } } + (NSDateFormatter *)payloadDateFormatter { @@ -209,23 +237,31 @@ + (NSDateFormatter *)payloadDateFormatter { } + (void)setSuspendThreadsForUserReported:(BOOL)suspendThreadsForUserReported { - [[BSG_KSCrash sharedInstance] - setSuspendThreadsForUserReported:suspendThreadsForUserReported]; + if ([self bugsnagStarted]) { + [[BSG_KSCrash sharedInstance] + setSuspendThreadsForUserReported:suspendThreadsForUserReported]; + } } + (void)setReportWhenDebuggerIsAttached:(BOOL)reportWhenDebuggerIsAttached { - [[BSG_KSCrash sharedInstance] - setReportWhenDebuggerIsAttached:reportWhenDebuggerIsAttached]; + if ([self bugsnagStarted]) { + [[BSG_KSCrash sharedInstance] + setReportWhenDebuggerIsAttached:reportWhenDebuggerIsAttached]; + } } + (void)setThreadTracingEnabled:(BOOL)threadTracingEnabled { - [[BSG_KSCrash sharedInstance] setThreadTracingEnabled:threadTracingEnabled]; + if ([self bugsnagStarted]) { + [[BSG_KSCrash sharedInstance] setThreadTracingEnabled:threadTracingEnabled]; + } } + (void)setWriteBinaryImagesForUserReported: (BOOL)writeBinaryImagesForUserReported { - [[BSG_KSCrash sharedInstance] - setWriteBinaryImagesForUserReported:writeBinaryImagesForUserReported]; + if ([self bugsnagStarted]) { + [[BSG_KSCrash sharedInstance] + setWriteBinaryImagesForUserReported:writeBinaryImagesForUserReported]; + } } @end From 71032edd9cbffe5cf5f359c70ea380f59d09179f Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 18 May 2018 11:03:09 +0100 Subject: [PATCH 3/8] docs: Add changelog entry for requiring api key --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e99748e7f..8ab92ade9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ Changelog ========= +## 5.X.X (TBD) + +* Enforce requiring API key to initialise notifier [#280](https://github.com/bugsnag/bugsnag-cocoa/pull/280) + ## 5.15.5 (25 Apr 2018) ### Bug Fixes From ce966a66080b3a8ea26543641dd547124dde17a9 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Mon, 21 May 2018 09:41:56 +0100 Subject: [PATCH 4/8] test: Verify that configuration has a valid api key --- Source/Bugsnag.m | 2 +- Source/BugsnagConfiguration.h | 2 ++ Source/BugsnagConfiguration.m | 5 +++++ Tests/BugsnagConfigurationTests.m | 11 +++++++++++ 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/Source/Bugsnag.m b/Source/Bugsnag.m index 1d2f1d35c..d7ac8f62e 100644 --- a/Source/Bugsnag.m +++ b/Source/Bugsnag.m @@ -51,7 +51,7 @@ + (void)startBugsnagWithApiKey:(NSString *)apiKey { + (void)startBugsnagWithConfiguration:(BugsnagConfiguration *)configuration { @synchronized(self) { - if (configuration != nil && configuration.apiKey != nil) { + if ([configuration hasValidApiKey]) { bsg_g_bugsnag_notifier = [[BugsnagNotifier alloc] initWithConfiguration:configuration]; [bsg_g_bugsnag_notifier start]; diff --git a/Source/BugsnagConfiguration.h b/Source/BugsnagConfiguration.h index e600520b0..652beac22 100644 --- a/Source/BugsnagConfiguration.h +++ b/Source/BugsnagConfiguration.h @@ -193,4 +193,6 @@ BugsnagBreadcrumbs *breadcrumbs; @property(nullable) NSString *codeBundleId; @property(nullable) NSString *notifierType; +- (BOOL)hasValidApiKey; + @end diff --git a/Source/BugsnagConfiguration.m b/Source/BugsnagConfiguration.m index e4eb21190..fe771daa3 100644 --- a/Source/BugsnagConfiguration.m +++ b/Source/BugsnagConfiguration.m @@ -246,4 +246,9 @@ - (NSDictionary *)sessionApiHeaders { kHeaderApiSentAt: [BSG_RFC3339DateTool stringFromDate:[NSDate new]] }; } + +- (BOOL)hasValidApiKey { + return _apiKey != nil && ![@"" isEqualToString:_apiKey]; +} + @end diff --git a/Tests/BugsnagConfigurationTests.m b/Tests/BugsnagConfigurationTests.m index 286e3fad3..373bffbf2 100644 --- a/Tests/BugsnagConfigurationTests.m +++ b/Tests/BugsnagConfigurationTests.m @@ -114,5 +114,16 @@ - (void)testApiKeySetter { XCTAssertEqualObjects(@"test", config.apiKey); } +- (void)testHasValidApiKey { + BugsnagConfiguration *config = nil; + XCTAssertFalse([config hasValidApiKey]); + + config = [BugsnagConfiguration new]; + XCTAssertFalse([config hasValidApiKey]); + + config.apiKey = @"5adf89e0aaa"; + XCTAssertTrue([config hasValidApiKey]); +} + @end From 47a34260865dd0b97d2586662bbd967cd58034f2 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Tue, 22 May 2018 09:29:33 +0100 Subject: [PATCH 5/8] refactor: Tweak apiKey config in response to review feedback --- Source/BugsnagConfiguration.h | 2 +- Source/BugsnagConfiguration.m | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Source/BugsnagConfiguration.h b/Source/BugsnagConfiguration.h index 652beac22..a2c18d5d4 100644 --- a/Source/BugsnagConfiguration.h +++ b/Source/BugsnagConfiguration.h @@ -69,7 +69,7 @@ typedef NSDictionary *_Nullable (^BugsnagBeforeNotifyHook)( /** * The API key of a Bugsnag project */ -@property(readwrite, retain, nonnull) NSString *apiKey; +@property(readwrite, retain, nullable) NSString *apiKey; /** * The URL used to notify Bugsnag */ diff --git a/Source/BugsnagConfiguration.m b/Source/BugsnagConfiguration.m index fe771daa3..2bc23bfb2 100644 --- a/Source/BugsnagConfiguration.m +++ b/Source/BugsnagConfiguration.m @@ -207,7 +207,7 @@ - (NSString *)apiKey { } - (void)setApiKey:(NSString *)apiKey { - if (apiKey != nil) { + if ([apiKey length] > 0) { _apiKey = apiKey; } else { bsg_log_err(@"Attempted to override non-null API key with nil - ignoring."); @@ -248,7 +248,7 @@ - (NSDictionary *)sessionApiHeaders { } - (BOOL)hasValidApiKey { - return _apiKey != nil && ![@"" isEqualToString:_apiKey]; + return [_apiKey length] > 0; } @end From 2638a69ed90da56d84ef55df67a4e3e243f8314f Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Tue, 22 May 2018 09:48:03 +0100 Subject: [PATCH 6/8] fix: Add a boolean property to check when notifier initialised rather than checking null --- Source/Bugsnag.m | 2 +- Source/BugsnagNotifier.h | 1 + Source/BugsnagNotifier.m | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Source/Bugsnag.m b/Source/Bugsnag.m index d7ac8f62e..f28394c2f 100644 --- a/Source/Bugsnag.m +++ b/Source/Bugsnag.m @@ -177,7 +177,7 @@ + (void)clearTabWithName:(NSString *)tabName { } + (BOOL)bugsnagStarted { - if (self.notifier == nil) { + if (!self.notifier.started) { bsg_log_err(@"Ensure you have started Bugsnag with startWithApiKey: " @"before calling any other Bugsnag functions."); diff --git a/Source/BugsnagNotifier.h b/Source/BugsnagNotifier.h index cdf9a5d11..9b837c2b5 100644 --- a/Source/BugsnagNotifier.h +++ b/Source/BugsnagNotifier.h @@ -40,6 +40,7 @@ @property(nonatomic, readwrite, retain) NSLock *_Nonnull metaDataLock; @property(nonatomic) BSGConnectivity *_Nonnull networkReachable; +@property(nonatomic, readonly) BOOL started; - (instancetype _Nonnull)initWithConfiguration: (BugsnagConfiguration *_Nonnull)configuration; diff --git a/Source/BugsnagNotifier.m b/Source/BugsnagNotifier.m index 8687fb439..1eebb95d7 100644 --- a/Source/BugsnagNotifier.m +++ b/Source/BugsnagNotifier.m @@ -356,6 +356,7 @@ - (void)start { // notification not received in time on initial startup, so trigger manually [self willEnterForeground:self]; + _started = YES; } - (void)watchLifecycleEvents:(NSNotificationCenter *)center { From 1f6c4f3553d05d25a0e5bc5942d659febccdcb38 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Tue, 22 May 2018 09:49:32 +0100 Subject: [PATCH 7/8] fix(make started property atomic): --- Source/BugsnagNotifier.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/BugsnagNotifier.h b/Source/BugsnagNotifier.h index 9b837c2b5..a4a9b6122 100644 --- a/Source/BugsnagNotifier.h +++ b/Source/BugsnagNotifier.h @@ -40,7 +40,7 @@ @property(nonatomic, readwrite, retain) NSLock *_Nonnull metaDataLock; @property(nonatomic) BSGConnectivity *_Nonnull networkReachable; -@property(nonatomic, readonly) BOOL started; +@property(readonly) BOOL started; - (instancetype _Nonnull)initWithConfiguration: (BugsnagConfiguration *_Nonnull)configuration; From 8a91bf876271f8097f8ce96c428798aa5f034963 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Wed, 23 May 2018 20:46:23 -0700 Subject: [PATCH 8/8] Keep key-value observability support Since we already had it through synthesized properties, keeps it in case its used. --- Source/BugsnagConfiguration.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Source/BugsnagConfiguration.m b/Source/BugsnagConfiguration.m index 2bc23bfb2..bcd9ce382 100644 --- a/Source/BugsnagConfiguration.m +++ b/Source/BugsnagConfiguration.m @@ -208,7 +208,9 @@ - (NSString *)apiKey { - (void)setApiKey:(NSString *)apiKey { if ([apiKey length] > 0) { + [self willChangeValueForKey:NSStringFromSelector(@selector(apiKey))]; _apiKey = apiKey; + [self didChangeValueForKey:NSStringFromSelector(@selector(apiKey))]; } else { bsg_log_err(@"Attempted to override non-null API key with nil - ignoring."); }