Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block reports sent to ACRA #17402

Merged
merged 5 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .idea/dictionaries/usernames.xml
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
<w>quiro91</w>
<w>tarekkma</w>
<w>vianey</w>
<w>voczi</w>
<w>zanki</w>
</words>
</dictionary>
Expand Down
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
4 changes: 4 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ 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 org.acra.ACRA
Expand Down Expand Up @@ -281,6 +282,8 @@ object CrashReportService {
}
}
if (FEEDBACK_REPORT_NEVER != reportMode) {
if (ThrowableFilterService.shouldDiscardThrowable(e)) return

ACRA.errorReporter.putCustomData("origin", origin ?: "")
ACRA.errorReporter.putCustomData("additionalInfo", additionalInfo ?: "")
ACRA.errorReporter.handleException(e)
Expand Down Expand Up @@ -319,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)
david-allison marked this conversation as resolved.
Show resolved Hide resolved
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,118 @@
/****************************************************************************************
* 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 com.ichi2.anki.exception.StorageAccessException
import net.ankiweb.rsdroid.exceptions.BackendNetworkException
import net.ankiweb.rsdroid.exceptions.BackendSyncException
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 {
// Note that an exception may have a nested BackendSyncException,
// so we check if it is safe from PII despite also filtering by type
return exceptionIsUnwanted(t) || !t.safeFromPII()
}

// There are few exception types that are common, but are unwanted in
// our analytics or crash report service because they are not actionable
fun exceptionIsUnwanted(t: Throwable): Boolean {
Timber.v("exceptionIsUnwanted - examining %s", t.javaClass.simpleName)
when (t) {
is BackendNetworkException -> return true
is BackendSyncException -> return true
is StorageAccessException -> return true
}
Timber.v("exceptionIsUnwanted - exception was wanted")
return false
}

/**
* 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
@@ -0,0 +1,65 @@
/*
* Copyright (c) 2024 voczi <[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

import androidx.test.ext.junit.runners.AndroidJUnit4
import anki.backend.BackendError
import com.ichi2.anki.exception.StorageAccessException
import com.ichi2.anki.servicelayer.ThrowableFilterService
import com.ichi2.anki.servicelayer.ThrowableFilterService.safeFromPII
import com.ichi2.testutils.JvmTest
import net.ankiweb.rsdroid.exceptions.BackendDeckIsFilteredException
import net.ankiweb.rsdroid.exceptions.BackendNetworkException
import net.ankiweb.rsdroid.exceptions.BackendSyncException
import net.ankiweb.rsdroid.exceptions.BackendSyncException.BackendSyncServerMessageException
import org.junit.Test
import org.junit.runner.RunWith
import kotlin.test.assertFalse
import kotlin.test.assertTrue

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

@Test
fun `Normal exceptions are flagged as PII-safe`() {
val exception = BackendDeckIsFilteredException(BackendError.newBuilder().build())
assertTrue(exception.safeFromPII(), "Exception reported as safe from PII")
}

@Test
fun `BackendSyncServerMessage exceptions are flagged as PII-unsafe`() {
val exception1 = BackendSyncServerMessageException(BackendError.newBuilder().build())
assertFalse(exception1.safeFromPII(), "Exception reported as not safe from PII")

val exception2 = Exception("", Exception("", exception1))
assertFalse(exception2.safeFromPII(), "Nested exception reported as not safe from PII")
}

@Test
fun `exceptions are discarded correctly by type`() {
// regular exceptions should go through
assertFalse(ThrowableFilterService.shouldDiscardThrowable(Exception("wanted")))

// exceptions of known unwanted types should not go through
val exception1 = BackendNetworkException(BackendError.newBuilder().build())
assertTrue(ThrowableFilterService.shouldDiscardThrowable(exception1))
val exception2 = BackendSyncException(BackendError.newBuilder().build())
assertTrue(ThrowableFilterService.shouldDiscardThrowable(exception2))
val exception3 = StorageAccessException("test exception")
assertTrue(ThrowableFilterService.shouldDiscardThrowable(exception3))
}
}
3 changes: 3 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ subprojects {
// tell backend to avoid rollover time, and disable interval fuzzing
it.environment("ANKI_TEST_MODE", "1")

it.maxHeapSize = "2g"
it.minHeapSize = "1g"

it.useJUnitPlatform()
it.testLogging {
events("failed", "skipped")
Expand Down