From b3466e8daf9cd7ee326921d484fd8f964bcb6097 Mon Sep 17 00:00:00 2001 From: Voczi Date: Sat, 9 Nov 2024 11:55:33 +0100 Subject: [PATCH] Filter/block reports sent to ACRA --- .idea/dictionaries/usernames.xml | 1 + .../main/java/com/ichi2/anki/AnkiDroidApp.kt | 2 +- .../java/com/ichi2/anki/CrashReportService.kt | 87 ++++++++++++++++--- .../com/ichi2/anki/CrashReportServiceTest.kt | 66 ++++++++++++++ 4 files changed, 145 insertions(+), 11 deletions(-) create mode 100644 AnkiDroid/src/test/java/com/ichi2/anki/CrashReportServiceTest.kt diff --git a/.idea/dictionaries/usernames.xml b/.idea/dictionaries/usernames.xml index 55dafdb672bf..24e5841bf6f9 100644 --- a/.idea/dictionaries/usernames.xml +++ b/.idea/dictionaries/usernames.xml @@ -83,6 +83,7 @@ quiro91 tarekkma vianey + voczi zanki diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.kt index bf91fa6f3ac3..489f85b928c9 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.kt @@ -124,7 +124,7 @@ open class AnkiDroidApp : Application(), Configuration.Provider, ChangeManager.S // Ensures any change is propagated to widgets ChangeManager.subscribe(this) - CrashReportService.initialize(this) + CrashReportService.initialize(this, preferences) val logType = LogType.value when (logType) { LogType.DEBUG -> Timber.plant(DebugTree()) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt index 0cc97f963b53..bcbca8079b77 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt @@ -18,6 +18,7 @@ package com.ichi2.anki import android.app.Application import android.content.Context import android.content.SharedPreferences +import android.util.Patterns import androidx.annotation.StringRes import androidx.annotation.VisibleForTesting import androidx.core.content.edit @@ -28,9 +29,11 @@ import com.ichi2.anki.analytics.UsageAnalytics import com.ichi2.anki.analytics.UsageAnalytics.sendAnalyticsException import com.ichi2.anki.exception.ManuallyReportedException import com.ichi2.anki.exception.UserSubmittedException +import com.ichi2.anki.preferences.get import com.ichi2.anki.preferences.sharedPrefs 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 @@ -73,9 +76,13 @@ object CrashReportService { lateinit var acraCoreConfigBuilder: CoreConfigurationBuilder private set private lateinit var mApplication: Application + + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + var mPreferences: SharedPreferences? = null private const val WEBVIEW_VER_NAME = "WEBVIEW_VER_NAME" private const val MIN_INTERVAL_MS = 60000 - private const val EXCEPTION_MESSAGE = "Exception report sent by user manually. See: 'Comment/USER_COMMENT'" + private const val EXCEPTION_MESSAGE = + "Exception report sent by user manually. See: 'Comment/USER_COMMENT'" private enum class ToastType(@StringRes private val toastMessageRes: Int) { AUTO_TOAST(R.string.feedback_auto_toast_text), @@ -160,8 +167,9 @@ object CrashReportService { * data directory. Analytics falls back to a sensible default if this is not set. */ @JvmStatic - fun initialize(application: Application) { + fun initialize(application: Application, preferences: SharedPreferences) { mApplication = application + mPreferences = preferences // FIXME ACRA needs to reinitialize after language is changed, but with the new language // this is difficult because the Application (AnkiDroidApp) does not change it's baseContext // perhaps a solution could be to change AnkiDroidApp to have a context wrapper that it sets @@ -257,34 +265,93 @@ object CrashReportService { } /** Used when we don't have an exception to throw, but we know something is wrong and want to diagnose it */ - fun sendExceptionReport(message: String?, origin: String?) { + fun sendExceptionReport(message: String?, origin: String?): Boolean = sendExceptionReport(ManuallyReportedException(message), origin, null) - } - fun sendExceptionReport(e: Throwable, origin: String?) { + fun sendExceptionReport(e: Throwable, origin: String?): Boolean = sendExceptionReport(e, origin, null) - } - fun sendExceptionReport(e: Throwable, origin: String?, additionalInfo: String?) { + fun sendExceptionReport(e: Throwable, origin: String?, additionalInfo: String?): Boolean = sendExceptionReport(e, origin, additionalInfo, false) - } - fun sendExceptionReport(e: Throwable, origin: String?, additionalInfo: String?, onlyIfSilent: Boolean) { + fun sendExceptionReport( + e: Throwable, + origin: String?, + additionalInfo: String?, + onlyIfSilent: Boolean + ): Boolean { sendAnalyticsException(e, false) AnkiDroidApp.sentExceptionReportHack = true val reportMode = mApplication.applicationContext.sharedPrefs() .getString(FEEDBACK_REPORT_KEY, FEEDBACK_REPORT_ASK) + if (onlyIfSilent) { if (FEEDBACK_REPORT_ALWAYS != reportMode) { Timber.i("sendExceptionReport - onlyIfSilent true, but ACRA is not 'always accept'. Skipping report send.") - return + return false } } + if (FEEDBACK_REPORT_NEVER != reportMode) { + if (processException(e) == null) { + return false + } + ACRA.errorReporter.putCustomData("origin", origin ?: "") ACRA.errorReporter.putCustomData("additionalInfo", additionalInfo ?: "") ACRA.errorReporter.handleException(e) + return true } + + return false + } + + fun Throwable.replaceMessage(newMessage: String?): Throwable = + Throwable(newMessage, this.cause) + + private val exceptionRules: List<(Throwable) -> Throwable?> = listOf( + { e -> + // BackendSyncServerMessage may contain PII and we do not want this leaked to ACRA. + // If the exception is of a cause deemed to be associated with a message containing PII, + // then do not send it at all to retain user privacy. + // Related: https://github.com/ankidroid/Anki-Android/issues/17392 + // and also https://github.com/ankitects/anki/commit/ba1f5f4 + if (e.cause is BackendSyncServerMessageException) null else e + }, + { e -> + + // Make sure we filter out any email addresses which may be leaked by the sync server. + e.replaceMessage( + Patterns.EMAIL_ADDRESS.toRegex().replace(e.message.orEmpty(), "-EMAIL-") + ) + }, + { e -> + // Filter out the current username in the message (if it exists) as it is considered PII. + e.replaceMessage( + e.message.orEmpty() + .replace(mPreferences?.get(SyncPreferences.USERNAME).toString(), "-USERNAME-") + ) + } + ) + + // Apply each and every rule in exceptionRules on the original exception. + // This means that after this method call, we can get an exception that has the username and + // email of the user filtered out so it doesn't get sent to ACRA as is. + // If the method returns null, that means we do not send a report to ACRA at all as we have + // decided to block this based on some arbitrary rule(s) enforced on the exception's contents. + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + fun processException(e: Throwable): Throwable? { + var newE: Throwable? = e + + for (rule in exceptionRules) { + newE = rule.invoke(newE!!) + + if (newE == null) { + return null + } + } + + return newE } fun isProperServiceProcess(): Boolean { diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/CrashReportServiceTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/CrashReportServiceTest.kt new file mode 100644 index 000000000000..715d4885783c --- /dev/null +++ b/AnkiDroid/src/test/java/com/ichi2/anki/CrashReportServiceTest.kt @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2024 voczi + * + * 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 . + */ + +package com.ichi2.anki + +import androidx.core.content.edit +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.ichi2.anki.CrashReportService.processException +import com.ichi2.anki.preferences.sharedPrefs +import net.ankiweb.rsdroid.exceptions.BackendDeckIsFilteredException +import net.ankiweb.rsdroid.exceptions.BackendSyncException.BackendSyncServerMessageException +import org.hamcrest.MatcherAssert.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.mock +import kotlin.Throwable +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNull + +@RunWith(AndroidJUnit4::class) +class CrashReportServiceTest : RobolectricTest() { + @Test + fun `Normal exceptions are untouched`() { + val message = "This is a test message which should not be filtered nor blocked" + val e = Throwable(message, mock(BackendDeckIsFilteredException::class.java)) + val processed = processException(e) + + assertNotNull(processed, "Returned exception should not be null") + assertEquals(processed.message!!, message, "Returned exception's message should stay the same") + } + + @Test + fun `Backend sync server messages are blocked`() { + assertNull(processException(Throwable("test", mock(BackendSyncServerMessageException::class.java))), "Returned exception is null") + } + + @Test + fun `Emails are filtered out`() { + val msg = processException(Throwable("Lorem ipsum 123 email@domain.tld Lorem ipsum."))!!.message!! + assertThat("Exception message does not contain the email", !msg.contains("email@domain.tld")) + } + + @Test + fun `Username is filtered out`() { + val preferences = targetContext.sharedPrefs() + preferences.edit() { putString(SyncPreferences.USERNAME, "MyUser") } + CrashReportService.mPreferences = preferences + + val msg = processException(Throwable("Lorem ipsum 123 MyUser Lorem ipsum."))!!.message!! + assertThat("Exception message not contain the username", !msg.contains("MyUser")) + } +}