Merge next into onsend-next branch

CHANGELOG.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## TBD
+
+## Bug fixes
+
+* `redactedKeys` now correctly apply to metadata on Event breadcrumbs
+  [#1526](https://github.com/bugsnag/bugsnag-android/pull/1526)
+
 ## 5.15.0 (2021-11-04)
 
 ### Bug fixes

bugsnag-android-core/detekt-baseline.xml

@@ -13,7 +13,7 @@
     <ID>LongParameterList:DeviceDataCollector.kt$DeviceDataCollector$( private val connectivity: Connectivity, private val appContext: Context, resources: Resources, private val deviceId: String?, private val buildInfo: DeviceBuildInfo, private val dataDirectory: File, rootDetector: RootDetector, private val bgTaskService: BackgroundTaskService, private val logger: Logger )</ID>
     <ID>LongParameterList:DeviceWithState.kt$DeviceWithState$( buildInfo: DeviceBuildInfo, jailbroken: Boolean?, id: String?, locale: String?, totalMemory: Long?, runtimeVersions: MutableMap&lt;String, Any>, /** * The number of free bytes of storage available on the device */ var freeDisk: Long?, /** * The number of free bytes of memory available on the device */ var freeMemory: Long?, /** * The orientation of the device when the event occurred: either portrait or landscape */ var orientation: String?, /** * The timestamp on the device when the event occurred */ var time: Date? )</ID>
     <ID>LongParameterList:EventFilenameInfo.kt$EventFilenameInfo.Companion$( obj: Any, uuid: String = UUID.randomUUID().toString(), apiKey: String?, timestamp: Long = System.currentTimeMillis(), config: ImmutableConfig, isLaunching: Boolean? = null )</ID>
-    <ID>LongParameterList:EventInternal.kt$EventInternal$( apiKey: String, breadcrumbs: MutableList&lt;Breadcrumb> = mutableListOf(), discardClasses: Set&lt;String> = setOf(), errors: MutableList&lt;Error> = mutableListOf(), metadata: Metadata = Metadata(), originalError: Throwable? = null, projectPackages: Collection&lt;String> = setOf(), severityReason: SeverityReason = SeverityReason.newInstance(SeverityReason.REASON_HANDLED_EXCEPTION), threads: MutableList&lt;Thread> = mutableListOf(), user: User = User() )</ID>
+    <ID>LongParameterList:EventInternal.kt$EventInternal$( apiKey: String, breadcrumbs: MutableList&lt;Breadcrumb> = mutableListOf(), discardClasses: Set&lt;String> = setOf(), errors: MutableList&lt;Error> = mutableListOf(), metadata: Metadata = Metadata(), originalError: Throwable? = null, projectPackages: Collection&lt;String> = setOf(), severityReason: SeverityReason = SeverityReason.newInstance(SeverityReason.REASON_HANDLED_EXCEPTION), threads: MutableList&lt;Thread> = mutableListOf(), user: User = User(), redactionKeys: Set&lt;String>? = null )</ID>
     <ID>LongParameterList:EventStorageModule.kt$EventStorageModule$( contextModule: ContextModule, configModule: ConfigModule, dataCollectionModule: DataCollectionModule, bgTaskService: BackgroundTaskService, trackerModule: TrackerModule, systemServiceModule: SystemServiceModule, notifier: Notifier, callbackState: CallbackState )</ID>
     <ID>LongParameterList:NativeStackframe.kt$NativeStackframe$( /** * The name of the method that was being executed */ var method: String?, /** * The location of the source file */ var file: String?, /** * The line number within the source file this stackframe refers to */ var lineNumber: Number?, /** * The address of the instruction where the event occurred. */ var frameAddress: Long?, /** * The address of the function where the event occurred. */ var symbolAddress: Long?, /** * The address of the library where the event occurred. */ var loadAddress: Long?, /** * Whether this frame identifies the program counter */ var isPC: Boolean?, /** * The type of the error */ var type: ErrorType? = null )</ID>
     <ID>LongParameterList:StateEvent.kt$StateEvent.Install$( @JvmField val apiKey: String, @JvmField val autoDetectNdkCrashes: Boolean, @JvmField val appVersion: String?, @JvmField val buildUuid: String?, @JvmField val releaseStage: String?, @JvmField val lastRunInfoPath: String, @JvmField val consecutiveLaunchCrashes: Int, @JvmField val sendThreads: ThreadSendPolicy )</ID>
@@ -22,7 +22,6 @@
     <ID>MagicNumber:DefaultDelivery.kt$DefaultDelivery$429</ID>
     <ID>MagicNumber:DefaultDelivery.kt$DefaultDelivery$499</ID>
     <ID>MagicNumber:LastRunInfoStore.kt$LastRunInfoStore$3</ID>
-    <ID>MaxLineLength:EventSerializationTest.kt$EventSerializationTest.Companion$it.threads.add(Thread(5, "main", ThreadType.ANDROID, true, Thread.State.RUNNABLE, stacktrace, NoopLogger))</ID>
     <ID>MaxLineLength:LastRunInfo.kt$LastRunInfo$return "LastRunInfo(consecutiveLaunchCrashes=$consecutiveLaunchCrashes, crashed=$crashed, crashedDuringLaunch=$crashedDuringLaunch)"</ID>
     <ID>MaxLineLength:ThreadState.kt$ThreadState$Thread(thread.id, thread.name, ThreadType.ANDROID, errorThread, Thread.State.forThread(thread), stacktrace, logger)</ID>
     <ID>ProtectedMemberInFinalClass:ConfigInternal.kt$ConfigInternal$protected val plugins = HashSet&lt;Plugin>()</ID>
@@ -41,9 +40,6 @@
     <ID>SwallowedException:DeviceIdStore.kt$DeviceIdStore$catch (exc: OverlappingFileLockException) { Thread.sleep(FILE_LOCK_WAIT_MS) }</ID>
     <ID>SwallowedException:PluginClient.kt$PluginClient$catch (exc: ClassNotFoundException) { logger.d("Plugin '$clz' is not on the classpath - functionality will not be enabled.") null }</ID>
     <ID>UnnecessaryAbstractClass:DependencyModule.kt$DependencyModule</ID>
-    <ID>UnusedPrivateMember:BugsnagEventMapper.kt$BugsnagEventMapper$private fun String.toDate(): Date</ID>
-    <ID>UnusedPrivateMember:BugsnagEventMapper.kt$BugsnagEventMapper$private val logger: Logger</ID>
-    <ID>UnusedPrivateMember:MarshalledEventSource.kt$MarshalledEventSource$private val eventFile: File</ID>
     <ID>UnusedPrivateMember:ThreadStateTest.kt$ThreadStateTest$private val configuration = generateImmutableConfig()</ID>
   </CurrentIssues>
 </SmellBaseline>

bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java

@@ -20,6 +20,7 @@
 import kotlin.jvm.functions.Function2;
 
 import java.io.File;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
@@ -185,7 +186,7 @@ public Unit invoke(Boolean hasConnection, String networkState) {
         eventStore = eventStorageModule.getEventStore();
 
         deliveryDelegate = new DeliveryDelegate(logger, eventStore,
-                immutableConfig, breadcrumbState, callbackState, notifier, bgTaskService);
+                immutableConfig, callbackState, notifier, bgTaskService);
 
         // Install a default exception handler with this client
         exceptionHandler = new ExceptionHandler(this, logger);
@@ -741,9 +742,8 @@ void notifyInternal(@NonNull Event event,
                         @Nullable OnErrorCallback onError) {
         // set the redacted keys on the event as this
         // will not have been set for RN/Unity events
-        Set<String> redactedKeys = metadataState.getMetadata().getRedactedKeys();
-        Metadata eventMetadata = event.getImpl().getMetadata();
-        eventMetadata.setRedactedKeys(redactedKeys);
+        Collection<String> redactedKeys = metadataState.getMetadata().getRedactedKeys();
+        event.setRedactedKeys(redactedKeys);
 
         // get session for event
         Session currentSession = sessionTracker.getCurrentSession();
@@ -761,6 +761,9 @@ void notifyInternal(@NonNull Event event,
             return;
         }
 
+        // leave an error breadcrumb of this event - for the next event
+        leaveErrorBreadcrumb(event);
+
         deliveryDelegate.deliver(event);
     }
 
@@ -924,6 +927,24 @@ void leaveAutoBreadcrumb(@NonNull String message,
         }
     }
 
+    private void leaveErrorBreadcrumb(@NonNull Event event) {
+        // Add a breadcrumb for this event occurring
+        List<Error> errors = event.getErrors();
+
+        if (errors.size() > 0) {
+            String errorClass = errors.get(0).getErrorClass();
+            String message = errors.get(0).getErrorMessage();
+
+            Map<String, Object> data = new HashMap<>();
+            data.put("errorClass", errorClass);
+            data.put("message", message);
+            data.put("unhandled", String.valueOf(event.isUnhandled()));
+            data.put("severity", event.getSeverity().toString());
+            breadcrumbState.add(new Breadcrumb(errorClass,
+                    BreadcrumbType.ERROR, data, new Date(), logger));
+        }
+    }
+
     /**
      * Retrieves information about the last launch of the application, if it has been run before.
      *

bugsnag-android-core/src/main/java/com/bugsnag/android/DeliveryDelegate.java

@@ -18,22 +18,19 @@ class DeliveryDelegate extends BaseObservable {
     final Logger logger;
     private final EventStore eventStore;
     private final ImmutableConfig immutableConfig;
-    final BreadcrumbState breadcrumbState;
     private final Notifier notifier;
     private final CallbackState callbackState;
     final BackgroundTaskService backgroundTaskService;
 
     DeliveryDelegate(Logger logger,
                      EventStore eventStore,
                      ImmutableConfig immutableConfig,
-                     BreadcrumbState breadcrumbState,
                      CallbackState callbackState,
                      Notifier notifier,
                      BackgroundTaskService backgroundTaskService) {
         this.logger = logger;
         this.eventStore = eventStore;
         this.immutableConfig = immutableConfig;
-        this.breadcrumbState = breadcrumbState;
         this.callbackState = callbackState;
         this.notifier = notifier;
         this.backgroundTaskService = backgroundTaskService;
@@ -95,13 +92,11 @@ DeliveryStatus deliverPayloadInternal(@NonNull EventPayload payload, @NonNull Ev
         switch (deliveryStatus) {
             case DELIVERED:
                 logger.i("Sent 1 new event to Bugsnag");
-                leaveErrorBreadcrumb(event);
                 break;
             case UNDELIVERED:
                 logger.w("Could not send event(s) to Bugsnag,"
                         + " saving to disk to send later");
                 cacheEvent(event, false);
-                leaveErrorBreadcrumb(event);
                 break;
             case FAILURE:
                 logger.w("Problem sending event to Bugsnag");
@@ -118,22 +113,4 @@ private void cacheEvent(@NonNull Event event, boolean attemptSend) {
             eventStore.flushAsync();
         }
     }
-
-    private void leaveErrorBreadcrumb(@NonNull Event event) {
-        // Add a breadcrumb for this event occurring
-        List<Error> errors = event.getErrors();
-
-        if (errors.size() > 0) {
-            String errorClass = errors.get(0).getErrorClass();
-            String message = errors.get(0).getErrorMessage();
-
-            Map<String, Object> data = new HashMap<>();
-            data.put("errorClass", errorClass);
-            data.put("message", message);
-            data.put("unhandled", String.valueOf(event.isUnhandled()));
-            data.put("severity", event.getSeverity().toString());
-            breadcrumbState.add(new Breadcrumb(errorClass,
-                    BreadcrumbType.ERROR, data, new Date(), logger));
-        }
-    }
 }

bugsnag-android-core/src/main/java/com/bugsnag/android/Event.java

@@ -6,8 +6,10 @@
 import androidx.annotation.Nullable;
 
 import java.io.IOException;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * An Event object represents a Throwable captured by Bugsnag and is available as a parameter on
@@ -348,4 +350,8 @@ void setSession(@Nullable Session session) {
     EventInternal getImpl() {
         return impl;
     }
+
+    void setRedactedKeys(Collection<String> redactedKeys) {
+        impl.setRedactedKeys(redactedKeys);
+    }
 }

bugsnag-android-core/src/main/java/com/bugsnag/android/EventInternal.kt

@@ -24,7 +24,8 @@ internal class EventInternal : JsonStream.Streamable, MetadataAware, UserAware {
         config.projectPackages,
         severityReason,
         ThreadState(originalError, severityReason.unhandled, config).threads,
-        User()
+        User(),
+        config.redactedKeys.toSet()
     )
 
     internal constructor(
@@ -37,7 +38,8 @@ internal class EventInternal : JsonStream.Streamable, MetadataAware, UserAware {
         projectPackages: Collection<String> = setOf(),
         severityReason: SeverityReason = SeverityReason.newInstance(SeverityReason.REASON_HANDLED_EXCEPTION),
         threads: MutableList<Thread> = mutableListOf(),
-        user: User = User()
+        user: User = User(),
+        redactionKeys: Set<String>? = null
     ) {
         this.apiKey = apiKey
         this.breadcrumbs = breadcrumbs
@@ -49,6 +51,10 @@ internal class EventInternal : JsonStream.Streamable, MetadataAware, UserAware {
         this.severityReason = severityReason
         this.threads = threads
         this.userImpl = user
+
+        redactionKeys?.let {
+            this.redactedKeys = it
+        }
     }
 
     val originalError: Throwable?
@@ -58,6 +64,8 @@ internal class EventInternal : JsonStream.Streamable, MetadataAware, UserAware {
     private val discardClasses: Set<String>
     internal var projectPackages: Collection<String>
 
+    private val jsonStreamer: ObjectJsonStreamer = ObjectJsonStreamer()
+
     @JvmField
     internal var session: Session? = null
 
@@ -82,6 +90,13 @@ internal class EventInternal : JsonStream.Streamable, MetadataAware, UserAware {
     var groupingHash: String? = null
     var context: String? = null
 
+    var redactedKeys: Collection<String>
+        get() = jsonStreamer.redactedKeys
+        set(value) {
+            jsonStreamer.redactedKeys = value.toSet()
+            metadata.redactedKeys = value.toSet()
+        }
+
     /**
      * @return user information associated with this Event
      */
@@ -109,7 +124,8 @@ internal class EventInternal : JsonStream.Streamable, MetadataAware, UserAware {
     }
 
     @Throws(IOException::class)
-    override fun toStream(writer: JsonStream) {
+    override fun toStream(parentWriter: JsonStream) {
+        val writer = JsonStream(parentWriter, jsonStreamer)
         // Write error basics
         writer.beginObject()
         writer.name("context").value(context)

bugsnag-android-core/src/main/java/com/bugsnag/android/JsonStream.java

@@ -33,6 +33,13 @@ public JsonStream(@NonNull Writer out) {
         objectJsonStreamer = new ObjectJsonStreamer();
     }
 
+    JsonStream(@NonNull JsonStream stream, @NonNull ObjectJsonStreamer streamer) {
+        super(stream.out);
+        setSerializeNulls(stream.getSerializeNulls());
+        this.out = stream.out;
+        this.objectJsonStreamer = streamer;
+    }
+
     // Allow chaining name().value()
     @NonNull
     public JsonStream name(@Nullable String name) throws IOException {

bugsnag-android-core/src/test/java/com/bugsnag/android/ClientFacadeTest.java

@@ -1,5 +1,6 @@
 package com.bugsnag.android;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.any;
@@ -18,6 +19,7 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 
@@ -517,4 +519,41 @@ public void onStateChange(@NotNull StateEvent event) {
         verify(launchCrashTracker, times(1)).removeObserver(observer);
     }
 
+    @Test
+    public void notifyLeavesErrorBreadcrumb() {
+        client.notify(new RuntimeException("Whoops!"));
+
+        ArgumentCaptor<Breadcrumb> breadcrumbCaptor = ArgumentCaptor.forClass(Breadcrumb.class);
+        verify(breadcrumbState).add(breadcrumbCaptor.capture());
+        Breadcrumb breadcrumb = breadcrumbCaptor.getValue();
+
+        assertEquals(BreadcrumbType.ERROR, breadcrumb.getType());
+        assertEquals("java.lang.RuntimeException", breadcrumb.getMessage());
+        assertEquals("java.lang.RuntimeException", breadcrumb.getMetadata().get("errorClass"));
+        assertEquals("Whoops!", breadcrumb.getMetadata().get("message"));
+        assertEquals("false", breadcrumb.getMetadata().get("unhandled"));
+        assertEquals("WARNING", breadcrumb.getMetadata().get("severity"));
+    }
+
+    @Test
+    public void notifyUnhandledLeavesErrorBreadcrumb() {
+        client.notifyUnhandledException(
+                new RuntimeException("Whoops!"),
+                new Metadata(),
+                SeverityReason.REASON_UNHANDLED_EXCEPTION,
+                null
+        );
+
+        ArgumentCaptor<Breadcrumb> breadcrumbCaptor = ArgumentCaptor.forClass(Breadcrumb.class);
+        verify(breadcrumbState).add(breadcrumbCaptor.capture());
+        Breadcrumb breadcrumb = breadcrumbCaptor.getValue();
+
+        assertEquals(BreadcrumbType.ERROR, breadcrumb.getType());
+        assertEquals("java.lang.RuntimeException", breadcrumb.getMessage());
+        assertEquals("java.lang.RuntimeException", breadcrumb.getMetadata().get("errorClass"));
+        assertEquals("Whoops!", breadcrumb.getMetadata().get("message"));
+        assertEquals("true", breadcrumb.getMetadata().get("unhandled"));
+        assertEquals("ERROR", breadcrumb.getMetadata().get("severity"));
+    }
+
 }

bugsnag-android-core/src/test/java/com/bugsnag/android/DeliveryDelegateTest.kt

@@ -21,7 +21,6 @@ internal class DeliveryDelegateTest {
     private val notifier = Notifier()
     val config = generateImmutableConfig()
     val callbackState = CallbackState()
-    val breadcrumbState = BreadcrumbState(50, callbackState, NoopLogger)
     private val logger = InterceptingLogger()
     lateinit var deliveryDelegate: DeliveryDelegate
     val handledState = SeverityReason.newInstance(
@@ -36,7 +35,6 @@ internal class DeliveryDelegateTest {
                 logger,
                 eventStore,
                 config,
-                breadcrumbState,
                 callbackState,
                 notifier,
                 BackgroundTaskService()
@@ -112,14 +110,6 @@ internal class DeliveryDelegateTest {
         val status = deliveryDelegate.deliverPayloadInternal(eventPayload, event)
         assertEquals(DeliveryStatus.DELIVERED, status)
         assertEquals("Sent 1 new event to Bugsnag", logger.msg)
-
-        val breadcrumb = requireNotNull(breadcrumbState.copy().first())
-        assertEquals(BreadcrumbType.ERROR, breadcrumb.type)
-        assertEquals("java.lang.RuntimeException", breadcrumb.message)
-        assertEquals("java.lang.RuntimeException", breadcrumb.metadata!!["errorClass"])
-        assertEquals("Whoops!", breadcrumb.metadata!!["message"])
-        assertEquals("true", breadcrumb.metadata!!["unhandled"])
-        assertEquals("ERROR", breadcrumb.metadata!!["severity"])
     }
 
     private class InterceptingLogger : Logger {

bugsnag-android-core/src/test/java/com/bugsnag/android/EventRedactionTest.kt

@@ -11,26 +11,44 @@ internal class EventRedactionTest {
 
     @Test
     fun testEventRedaction() {
+        val event = Event(
+            null,
+            generateImmutableConfig()
+                .copy(redactedKeys = setOf("password", "changeme")),
+            SeverityReason.newInstance(SeverityReason.REASON_HANDLED_EXCEPTION),
+            NoopLogger
+        )
+
+        testEventRedaction(event, "event_redaction.json")
+    }
+
+    @Test
+    fun testDefaultEventRedaction() {
         val event = Event(
             null,
             generateImmutableConfig(),
             SeverityReason.newInstance(SeverityReason.REASON_HANDLED_EXCEPTION),
             NoopLogger
         )
+
+        testEventRedaction(event, "event_default_redaction.json")
+    }
+
+    private fun testEventRedaction(event: Event, jsonFixture: String) {
         event.app = generateAppWithState()
         event.device = generateDeviceWithState()
 
         event.addMetadata("app", "password", "foo")
         event.addMetadata("device", "password", "bar")
         event.impl.metadata.addMetadata("baz", "password", "hunter2")
-        val metadata = mutableMapOf<String, Any?>(Pair("password", "whoops"))
+        val metadata = mutableMapOf<String, Any?>(Pair("changeme", "whoops"))
         event.breadcrumbs = listOf(Breadcrumb("Whoops", BreadcrumbType.LOG, metadata, Date(0), NoopLogger))
         event.threads.clear()
         event.device.cpuAbi = emptyArray()
 
         val writer = StringWriter()
         val stream = JsonStream(writer)
         event.toStream(stream)
-        validateJson("event_redaction.json", writer.toString())
+        validateJson(jsonFixture, writer.toString())
     }
 }