Skip to content

Commit

Permalink
feat: filter uncaught exceptions
Browse files Browse the repository at this point in the history
We want to filter all exception - that is ones that are uncaught as well
as ones that are imperatively sent through CrashReportService

That requires attaching filter to out Thread.defaultExceptionHandler chain

- extract filter into service, use it from crash report service
- implement default exception handler handling, init/reinit as needed
- concrete types for the handlers and public uninstall/install for testability
  • Loading branch information
mikehardy committed Nov 30, 2024
1 parent d9cc880 commit e68c8fc
Show file tree
Hide file tree
Showing 6 changed files with 162 additions and 30 deletions.
33 changes: 33 additions & 0 deletions AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/ACRATest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ import com.ichi2.anki.CrashReportService
import com.ichi2.anki.CrashReportService.FEEDBACK_REPORT_ALWAYS
import com.ichi2.anki.CrashReportService.FEEDBACK_REPORT_ASK
import com.ichi2.anki.R
import com.ichi2.anki.analytics.UsageAnalytics
import com.ichi2.anki.logging.ProductionCrashReportingTree
import com.ichi2.anki.preferences.sharedPrefs
import com.ichi2.anki.servicelayer.ThrowableFilterService
import com.ichi2.anki.testutil.GrantStoragePermission
import org.acra.ACRA
import org.acra.builder.ReportBuilder
Expand Down Expand Up @@ -230,6 +232,37 @@ class ACRATest : InstrumentedTest() {
assertToastMessage(R.string.feedback_auto_toast_text)
}

@Test
fun verifyExceptionHandlerChain() {
// contains assumptions about ordering in ACRA, ThrowableFilter and UsageAnalytics
// making sure they are correct is vital though, so we will accept the need to change
// this test if you re-order them
var firstExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
assertThat("First handler is ThrowableFilterService", firstExceptionHandler is ThrowableFilterService.FilteringExceptionHandler)
ThrowableFilterService.unInstallDefaultExceptionHandler()
var secondExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
assertThat("Second handler is AnalyticsLoggingExceptionHandler", secondExceptionHandler is UsageAnalytics.AnalyticsLoggingExceptionHandler)
UsageAnalytics.unInstallDefaultExceptionHandler()
var thirdExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
assertThat("Third handler is neither Analytics nor ThrowableFilter", thirdExceptionHandler !is UsageAnalytics.AnalyticsLoggingExceptionHandler && thirdExceptionHandler !is ThrowableFilterService.FilteringExceptionHandler)

// chain them again
UsageAnalytics.installDefaultExceptionHandler()
ThrowableFilterService.installDefaultExceptionHandler()

// reinitialize things and make sure they came through correctly again
CrashReportService.onPreferenceChanged(app!!.applicationContext, FEEDBACK_REPORT_ASK)
firstExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
assertThat("First handler is ThrowableFilterService", firstExceptionHandler is ThrowableFilterService.FilteringExceptionHandler)
ThrowableFilterService.unInstallDefaultExceptionHandler()
secondExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
Timber.i("Second handler is a %s", secondExceptionHandler)
assertThat("Second handler is AnalyticsLoggingExceptionHandler", secondExceptionHandler is UsageAnalytics.AnalyticsLoggingExceptionHandler)
UsageAnalytics.unInstallDefaultExceptionHandler()
thirdExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
assertThat("Third handler is neither Analytics nor ThrowableFilter", thirdExceptionHandler !is UsageAnalytics.AnalyticsLoggingExceptionHandler && thirdExceptionHandler !is ThrowableFilterService.FilteringExceptionHandler)
}

private fun setAcraReportingMode(feedbackReportAlways: String) {
CrashReportService.setAcraReportingMode(feedbackReportAlways)
}
Expand Down
4 changes: 4 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.kt
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import com.ichi2.anki.logging.RobolectricDebugTree
import com.ichi2.anki.preferences.SharedPreferencesProvider
import com.ichi2.anki.preferences.sharedPrefs
import com.ichi2.anki.servicelayer.DebugInfoService
import com.ichi2.anki.servicelayer.ThrowableFilterService
import com.ichi2.anki.services.BootService
import com.ichi2.anki.services.NotificationService
import com.ichi2.anki.ui.dialogs.ActivityAgnosticDialogs
Expand Down Expand Up @@ -146,6 +147,9 @@ open class AnkiDroidApp : Application(), Configuration.Provider, ChangeManager.S
UsageAnalytics.setDryRun(true)
}

// Last in the UncaughtExceptionHandlers chain is our filter service
ThrowableFilterService.initialize()

applicationScope.launch {
Timber.i(DebugInfoService.getDebugInfo(this@AnkiDroidApp))
}
Expand Down
21 changes: 3 additions & 18 deletions AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ import com.ichi2.anki.analytics.UsageAnalytics.sendAnalyticsException
import com.ichi2.anki.exception.ManuallyReportedException
import com.ichi2.anki.exception.UserSubmittedException
import com.ichi2.anki.preferences.sharedPrefs
import com.ichi2.anki.servicelayer.ThrowableFilterService
import com.ichi2.libanki.utils.TimeManager
import com.ichi2.utils.WebViewDebugging.setDataDirectorySuffix
import net.ankiweb.rsdroid.exceptions.BackendSyncException.BackendSyncServerMessageException
import org.acra.ACRA
import org.acra.ReportField
import org.acra.config.CoreConfigurationBuilder
Expand Down Expand Up @@ -282,30 +282,14 @@ object CrashReportService {
}
}
if (FEEDBACK_REPORT_NEVER != reportMode) {
if (!e.safeFromPII()) return
if (ThrowableFilterService.shouldDiscardThrowable(e)) return

ACRA.errorReporter.putCustomData("origin", origin ?: "")
ACRA.errorReporter.putCustomData("additionalInfo", additionalInfo ?: "")
ACRA.errorReporter.handleException(e)
}
}

/**
* Checks if the [Throwable] is safe from Personally Identifiable Information (PII)
* @return `false` if the [Throwable] contains PII, otherwise `true`
*/
fun Throwable.safeFromPII(): Boolean {
if (this.containsPIINonRecursive()) return false
return this.cause?.safeFromPII() != false
}

private fun Throwable.containsPIINonRecursive(): Boolean {
// BackendSyncServerMessage may contain PII and we do not want this leaked to ACRA.
// Related: https://github.com/ankidroid/Anki-Android/issues/17392
// and also https://github.com/ankitects/anki/commit/ba1f5f4
return this is BackendSyncServerMessageException
}

fun isProperServiceProcess(): Boolean {
return ACRA.isACRASenderServiceProcess()
}
Expand Down Expand Up @@ -338,6 +322,7 @@ object CrashReportService {
deleteACRALimiterData(ctx)
// We also need to re-chain our UncaughtExceptionHandlers
UsageAnalytics.reInitialize()
ThrowableFilterService.reInitialize()
}

/**
Expand Down
29 changes: 19 additions & 10 deletions AnkiDroid/src/main/java/com/ichi2/anki/analytics/UsageAnalytics.kt
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,19 @@ object UsageAnalytics {
private var sAnalyticsTrackingId: String? = null
private var sAnalyticsSamplePercentage = -1

@FunctionalInterface
fun interface AnalyticsLoggingExceptionHandler : Thread.UncaughtExceptionHandler

var uncaughtExceptionHandler = AnalyticsLoggingExceptionHandler {
thread: Thread?, throwable: Throwable ->
sendAnalyticsException(throwable, true)
if (thread == null) {
Timber.w("unexpected: thread was null")
return@AnalyticsLoggingExceptionHandler
}
sOriginalUncaughtExceptionHandler!!.uncaughtException(thread, throwable)
}

/**
* Initialize the analytics provider - must be called prior to sending anything.
* Usage after that is static
Expand Down Expand Up @@ -118,23 +131,19 @@ object UsageAnalytics {
* We want to send an analytics hit on any exception, then chain to other handlers (e.g., ACRA)
*/
@Synchronized
private fun installDefaultExceptionHandler() {
@VisibleForTesting
fun installDefaultExceptionHandler() {
sOriginalUncaughtExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
Thread.setDefaultUncaughtExceptionHandler { thread: Thread?, throwable: Throwable ->
sendAnalyticsException(throwable, true)
if (thread == null) {
Timber.w("unexpected: thread was null")
return@setDefaultUncaughtExceptionHandler
}
sOriginalUncaughtExceptionHandler!!.uncaughtException(thread, throwable)
}
Timber.d("Chaining to uncaughtExceptionHandler (%s)", sOriginalUncaughtExceptionHandler)
Thread.setDefaultUncaughtExceptionHandler(uncaughtExceptionHandler)
}

/**
* Reset the default exception handler
*/
@Synchronized
private fun unInstallDefaultExceptionHandler() {
@VisibleForTesting
fun unInstallDefaultExceptionHandler() {
Thread.setDefaultUncaughtExceptionHandler(sOriginalUncaughtExceptionHandler)
sOriginalUncaughtExceptionHandler = null
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/****************************************************************************************
* Copyright (c) 2024 voczi <[email protected]>
* Copyright (c) 2024 Mike Hardy <[email protected]> *
* *
* This program is free software; you can redistribute it and/or modify it under *
* the terms of the GNU General Public License as published by the Free Software *
* Foundation; either version 3 of the License, or (at your option) any later *
* version. *
* *
* This program is distributed in the hope that it will be useful, but WITHOUT ANY *
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A *
* PARTICULAR PURPOSE. See the GNU General Public License for more details. *
* *
* You should have received a copy of the GNU General Public License along with *
* this program. If not, see <http://www.gnu.org/licenses/>. *
****************************************************************************************/
package com.ichi2.anki.servicelayer

import androidx.annotation.VisibleForTesting
import net.ankiweb.rsdroid.exceptions.BackendSyncException.BackendSyncServerMessageException
import timber.log.Timber

object ThrowableFilterService {

@FunctionalInterface
fun interface FilteringExceptionHandler : Thread.UncaughtExceptionHandler

@VisibleForTesting
var originalUncaughtExceptionHandler: Thread.UncaughtExceptionHandler? = null

var uncaughtExceptionHandler = FilteringExceptionHandler {
thread: Thread?, throwable: Throwable ->
if (thread == null) {
Timber.w("unexpected: thread was null")
return@FilteringExceptionHandler
}
if (shouldDiscardThrowable(throwable)) {
Timber.i("discarding throwable")
return@FilteringExceptionHandler
}
originalUncaughtExceptionHandler?.uncaughtException(thread, throwable)
}

fun initialize() {
Timber.i("initialize()")
installDefaultExceptionHandler()
}

/**
* We want to filter any exceptions with PII, then chain to other handlers
*/
@Synchronized
@VisibleForTesting
fun installDefaultExceptionHandler() {
originalUncaughtExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
Timber.d("Chaining to uncaughtExceptionHandler (%s)", originalUncaughtExceptionHandler)
Thread.setDefaultUncaughtExceptionHandler(uncaughtExceptionHandler)
}

/**
* Reset the default exception handler
*/
@Synchronized
@VisibleForTesting
fun unInstallDefaultExceptionHandler() {
Thread.setDefaultUncaughtExceptionHandler(originalUncaughtExceptionHandler)
originalUncaughtExceptionHandler = null
}

/**
* Re-Initialize the throwable filter
*/
@Synchronized
fun reInitialize() {
// send any pending async hits, re-chain default exception handlers and re-init
Timber.i("reInitialize()")
unInstallDefaultExceptionHandler()
initialize()
}

fun shouldDiscardThrowable(t: Throwable): Boolean {
return !t.safeFromPII()
}

/**
* Checks if the [Throwable] is safe from Personally Identifiable Information (PII)
* @return `false` if the [Throwable] contains PII, otherwise `true`
*/
fun Throwable.safeFromPII(): Boolean {
if (this.containsPIINonRecursive()) return false
return this.cause?.safeFromPII() != false
}

private fun Throwable.containsPIINonRecursive(): Boolean {
// BackendSyncServerMessage may contain PII and we do not want this leaked to ACRA.
// Related: https://github.com/ankidroid/Anki-Android/issues/17392
// and also https://github.com/ankitects/anki/commit/ba1f5f4
return this is BackendSyncServerMessageException
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package com.ichi2.anki

import androidx.test.ext.junit.runners.AndroidJUnit4
import anki.backend.BackendError
import com.ichi2.anki.CrashReportService.safeFromPII
import com.ichi2.anki.servicelayer.ThrowableFilterService.safeFromPII
import com.ichi2.testutils.JvmTest
import net.ankiweb.rsdroid.exceptions.BackendDeckIsFilteredException
import net.ankiweb.rsdroid.exceptions.BackendSyncException.BackendSyncServerMessageException
Expand All @@ -28,7 +28,8 @@ import kotlin.test.assertFalse
import kotlin.test.assertTrue

@RunWith(AndroidJUnit4::class)
class CrashReportServiceTest : JvmTest() {
class ThrowableFilterServiceTest : JvmTest() {

@Test
fun `Normal exceptions are flagged as PII-safe`() {
val exception = BackendDeckIsFilteredException(BackendError.newBuilder().build())
Expand Down

0 comments on commit e68c8fc

Please sign in to comment.