Merge pull request #316 from JMR-dev/fix-reporting-pii-mainthread

fix(reporting): scrub PII from crash stack traces + move ReportStore scan off the main thread

app/src/androidTest/kotlin/org/libremail/ui/reporting/StartupCrashPromptTest.kt

@@ -11,6 +11,8 @@ import androidx.compose.ui.test.onNodeWithText
 import androidx.compose.ui.test.performClick
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
 import org.junit.After
 import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertNull
@@ -57,7 +59,9 @@ class StartupCrashPromptTest {
 
     private fun string(resId: Int) = composeTestRule.activity.getString(resId)
 
-    private fun store() = ReportStore(dir)
+    // Unconfined scope runs ReportStore's initial scan inline so a reopened store (simulating a
+    // relaunch) is readable synchronously, as before the scan moved off-thread (#296).
+    private fun store() = ReportStore(dir, CoroutineScope(Dispatchers.Unconfined))
 
     private fun crash(id: String, createdAt: Long) = DebugReport(
         id = id,

app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt

@@ -67,7 +67,8 @@ class DiagnosticsCollector @Inject constructor(
             androidSdkInt = Build.VERSION.SDK_INT,
             deviceManufacturer = Build.MANUFACTURER ?: "",
             deviceModel = Build.MODEL ?: "",
-            stackTrace = throwable?.stackTraceToString(),
+            // Scrub PII (server host:port, emails) out of the trace before it enters the report (#294).
+            stackTrace = throwable?.let { StackTraceScrubber.scrub(it.stackTraceToString()) },
             settings = settings,
             accounts = accounts,
             logs = logBuffer.snapshot().map { it.formatted() },

app/src/main/kotlin/org/libremail/reporting/ReportStore.kt

@@ -1,9 +1,13 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 package org.libremail.reporting
 
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
+import kotlinx.coroutines.launch
 import java.io.File
 
 /**
@@ -11,12 +15,30 @@ import java.io.File
  * Deliberately NOT Room-backed: a crash-time save must be simple and robust, independent of the
  * (possibly encrypted, possibly mid-migration) app database. Reports persist until the user submits
  * or discards them. [reports] is a snapshot that updates on every [save]/[delete].
+ *
+ * The store is an eager `@Singleton` dependency of `CrashReporter`, whose `install()` runs on the
+ * MAIN thread in `Application.onCreate()`. Listing + reading + JSON-parsing every stored report there
+ * would be main-thread disk I/O (#296), so the flow is seeded empty and the initial scan is dispatched
+ * to [scope] (IO by default). Reactive consumers (Problem Reports, the startup prompt) observe
+ * [reports] and update when the scan lands; the empty window is momentary. Writes ([save] etc.)
+ * re-scan synchronously so a crash-time save is never lost to the pending initial scan.
  */
-class ReportStore(private val directory: File) {
+class ReportStore(
+    private val directory: File,
+    scope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO),
+) {
     private val lock = Any()
-    private val _reports = MutableStateFlow(scan())
+    private val _reports = MutableStateFlow<List<DebugReport>>(emptyList())
     val reports: StateFlow<List<DebugReport>> = _reports.asStateFlow()
 
+    init {
+        scope.launch { refresh() }
+    }
+
+    private fun refresh() {
+        synchronized(lock) { _reports.value = scan() }
+    }
+
     fun save(report: DebugReport) {
         synchronized(lock) {
             directory.mkdirs()

app/src/main/kotlin/org/libremail/reporting/StackTraceScrubber.kt

@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+package org.libremail.reporting
+
+/**
+ * Removes personally-identifiable information from a throwable's stack trace before it is stored in
+ * or submitted as a [DebugReport]. LibreMail reports are a hard PII-free surface, but mail/network
+ * exceptions (Jakarta/Angus Mail, `java.net`) embed PII in their *message* text — a server hostname
+ * and, for auth failures, the account username/email — e.g.
+ *
+ * ```
+ * java.net.ConnectException: Failed to connect to imap.example.com/93.184.216.34:993
+ * ```
+ *
+ * A regex alone can't safely scrub this: a hostname like `imap.example.com` is indistinguishable
+ * from a dotted class name (`java.net.ConnectException`) or a frame's file (`Socket.java`). So the
+ * scrubber instead keeps the parts of a trace that carry no PII yet make a report useful — the
+ * exception *class* names and every stack *frame* (`at pkg.Class.method(File.kt:42)`) — and drops
+ * the free-text message from each exception header line, which is the only place a hostname or
+ * username appears. As defense-in-depth it then redacts any residual e-mail address or `host:port`
+ * token left on a wrapped/continuation message line. Frame lines are never touched, so a frame's
+ * `File.kt:42` is never mistaken for a `host:port`.
+ */
+object StackTraceScrubber {
+
+    private const val REDACTED = "[redacted]"
+
+    /** Exception header prefixes that precede the class name and must be preserved. */
+    private val KNOWN_PREFIXES = listOf("Caused by: ", "Suppressed: ")
+
+    /** `user@host.tld` anywhere in the text. */
+    private val EMAIL = Regex("""[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}""")
+
+    /**
+     * A dotted host name or IPv4 address followed by `:port`, including the `InetSocketAddress`
+     * "host/1.2.3.4:port" rendering. Applied only to non-frame lines, so it can never match a stack
+     * frame's `File.kt:42`.
+     */
+    private val HOST_PORT = Regex("""[A-Za-z0-9.-]+(?:/[0-9.]+)?:\d{2,5}""")
+
+    /** Scrubs [stackTrace] (the output of [Throwable.stackTraceToString]) of PII. */
+    fun scrub(stackTrace: String): String =
+        stackTrace.lineSequence().joinToString(separator = "\n", transform = ::scrubLine)
+
+    private fun scrubLine(line: String): String {
+        val trimmed = line.trimStart()
+        // Stack frames and the "... N more" elision carry only class/method/file/line — never PII.
+        if (trimmed.startsWith("at ") || trimmed.startsWith("... ")) return line
+        // Exception header line ("Type: message", possibly behind a "Caused by: "/"Suppressed: "
+        // prefix): drop the free-text message, then redact anything PII-shaped that remains.
+        return redact(dropMessage(line))
+    }
+
+    /** Keeps a header's class name(s) and any `Caused by:`/`Suppressed:` prefix, dropping the message. */
+    private fun dropMessage(line: String): String {
+        val indent = line.takeWhile(Char::isWhitespace)
+        val body = line.substring(indent.length)
+        val prefix = KNOWN_PREFIXES.firstOrNull(body::startsWith).orEmpty()
+        // Throwable renders headers as "<class>" or "<class>: <message>"; keep up to the first ": ".
+        val classOnly = body.substring(prefix.length).substringBefore(": ")
+        return indent + prefix + classOnly
+    }
+
+    private fun redact(line: String): String = line.replace(EMAIL, REDACTED).replace(HOST_PORT, REDACTED)
+}

app/src/test/kotlin/org/libremail/reporting/CrashReporterTest.kt

@@ -3,6 +3,8 @@ package org.libremail.reporting
 
 import io.mockk.every
 import io.mockk.mockk
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
 import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.TemporaryFolder
@@ -23,9 +25,13 @@ class CrashReporterTest {
     private val settingsRepository = mockk<SettingsRepository>()
     private val accountRepository = mockk<AccountRepository>()
 
+    // Unconfined scope runs ReportStore's initial scan inline so a reopened store is readable
+    // synchronously, as before the scan moved off-thread (#296).
+    private fun newStore() = ReportStore(tempFolder.root, CoroutineScope(Dispatchers.Unconfined))
+
     @Test
     fun `persisting a forced crash saves a report offered on next launch`() {
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
         val buffer = RingLogBuffer()
         val collector = DiagnosticsCollector(appVersion, settingsRepository, accountRepository, buffer)
         val reporter = CrashReporter(collector, store, buffer)
@@ -39,7 +45,7 @@ class CrashReporterTest {
         assertTrue(saved.logs.any { it.contains("Uncaught exception") })
 
         // Still available to a fresh store instance, simulating the next app launch.
-        val nextLaunch = ReportStore(tempFolder.root)
+        val nextLaunch = newStore()
         assertEquals(1, nextLaunch.reports.value.size)
         assertEquals(ReportKind.CRASH, nextLaunch.reports.value.single().kind)
     }
@@ -48,7 +54,7 @@ class CrashReporterTest {
     fun `capture only persists — it has no path to transmit`() {
         // CrashReporter is constructed without any submitter/scheduler, so a crash can only ever be
         // written to the local store. Nothing here can send data off the device.
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
         val buffer = RingLogBuffer()
         val collector = DiagnosticsCollector(appVersion, settingsRepository, accountRepository, buffer)
         val reporter = CrashReporter(collector, store, buffer)

app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt

@@ -15,6 +15,7 @@ import org.libremail.domain.model.MailSecurity
 import org.libremail.domain.model.ServerConfig
 import org.libremail.domain.repository.AccountRepository
 import kotlin.test.assertEquals
+import kotlin.test.assertFalse
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
 
@@ -38,7 +39,26 @@ class DiagnosticsCollectorTest {
         assertEquals(ReportKind.CRASH, report.kind)
         assertEquals("1.2.3", report.appVersionName)
         assertEquals(42L, report.appVersionCode)
-        assertTrue(report.stackTrace.orEmpty().contains("kaboom"))
+        // The trace is captured but scrubbed of message free-text (PII guard, #294): the exception
+        // class survives while the free-text message ("kaboom") is dropped.
+        assertTrue(report.stackTrace.orEmpty().contains("RuntimeException"))
+        assertFalse(report.stackTrace.orEmpty().contains("kaboom"))
+    }
+
+    @Test
+    fun `crash report scrubs server host, port and email from the stack trace message`() = runTest {
+        val boom = RuntimeException("Failed to connect to imap.example.com/93.184.216.34:993 for user@example.com")
+
+        val trace = collector.collectCrash(boom).stackTrace.orEmpty()
+
+        // No server host, IP, port or email leaks out of the captured trace message.
+        assertFalse(trace.contains("imap.example.com"))
+        assertFalse(trace.contains("93.184.216.34"))
+        assertFalse(trace.contains(":993"))
+        assertFalse(trace.contains("user@example.com"))
+        // Class + frame info survives so the report is still actionable.
+        assertTrue(trace.contains("RuntimeException"))
+        assertTrue(trace.contains("DiagnosticsCollectorTest"))
     }
 
     @Test

app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt

@@ -1,6 +1,11 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 package org.libremail.reporting
 
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.test.StandardTestDispatcher
+import kotlinx.coroutines.test.TestCoroutineScheduler
 import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.TemporaryFolder
@@ -9,11 +14,17 @@ import kotlin.test.assertEquals
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
 
+@OptIn(ExperimentalCoroutinesApi::class)
 class ReportStoreTest {
 
     @get:Rule
     val tempFolder = TemporaryFolder()
 
+    // Runs the initial scan inline (Unconfined dispatches eagerly on the calling thread) so these
+    // tests observe a fully-populated store immediately, as before the scan moved off-thread (#296).
+    // The dedicated `seeds empty` test below verifies the real off-thread, empty-seed behaviour.
+    private fun newStore() = ReportStore(tempFolder.root, CoroutineScope(Dispatchers.Unconfined))
+
     private fun report(id: String, createdAt: Long = 1L, kind: ReportKind = ReportKind.MANUAL) = DebugReport(
         id = id,
         createdAtMillis = createdAt,
@@ -31,7 +42,7 @@ class ReportStoreTest {
 
     @Test
     fun `save then find and list`() {
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
 
         store.save(report("a"))
 
@@ -41,7 +52,7 @@ class ReportStoreTest {
 
     @Test
     fun `lists newest first`() {
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
 
         store.save(report("old", createdAt = 1L))
         store.save(report("new", createdAt = 2L))
@@ -51,7 +62,7 @@ class ReportStoreTest {
 
     @Test
     fun `delete removes the report`() {
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
         store.save(report("a"))
 
         store.delete("a")
@@ -62,28 +73,28 @@ class ReportStoreTest {
 
     @Test
     fun `survives a fresh instance over the same directory (next launch)`() {
-        ReportStore(tempFolder.root).save(report("persisted"))
+        newStore().save(report("persisted"))
 
-        val reopened = ReportStore(tempFolder.root)
+        val reopened = newStore()
 
         assertEquals("persisted", reopened.find("persisted")?.id)
     }
 
     @Test
     fun `markSurfaced flags the report and persists across a fresh instance`() {
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
         store.save(report("a"))
 
         store.markSurfaced("a")
 
         assertTrue(store.find("a")!!.surfaced)
         // Survives a fresh instance over the same directory (the next launch reads it as surfaced).
-        assertTrue(ReportStore(tempFolder.root).find("a")!!.surfaced)
+        assertTrue(newStore().find("a")!!.surfaced)
     }
 
     @Test
     fun `markSurfaced is a no-op for a missing report`() {
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
 
         store.markSurfaced("missing")
 
@@ -93,7 +104,7 @@ class ReportStoreTest {
     @Test
     fun `ignores unparseable files`() {
         File(tempFolder.root, "garbage.json").writeText("not json at all")
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
 
         store.save(report("valid"))
 
@@ -102,7 +113,7 @@ class ReportStoreTest {
 
     @Test
     fun `purgeOlderThan deletes reports strictly older than the cutoff`() {
-        val store = ReportStore(tempFolder.root)
+        val store = newStore()
         store.save(report("old", createdAt = 1_000L))
         store.save(report("boundary", createdAt = 3_000L))
         store.save(report("recent", createdAt = 5_000L))
@@ -114,4 +125,22 @@ class ReportStoreTest {
         // A report exactly at the cutoff is kept (strictly-older purge); list stays newest-first.
         assertEquals(listOf("recent", "boundary"), store.reports.value.map { it.id })
     }
+
+    @Test
+    fun `seeds the flow empty and runs the initial scan off-thread`() {
+        // A report already on disk from a previous launch, written directly (not via the flow).
+        File(tempFolder.root, "persisted.json").writeText(report("persisted").toStorageJson())
+
+        // A StandardTestDispatcher queues the launched scan instead of running it inline, so the
+        // window between construction and the scan landing is observable.
+        val scheduler = TestCoroutineScheduler()
+        val store = ReportStore(tempFolder.root, CoroutineScope(StandardTestDispatcher(scheduler)))
+
+        // The constructor did NOT scan on the calling thread: the flow is seeded empty (#296).
+        assertTrue(store.reports.value.isEmpty())
+
+        // Running the queued work performs the scan off-thread and populates the flow.
+        scheduler.advanceUntilIdle()
+        assertEquals(listOf("persisted"), store.reports.value.map { it.id })
+    }
 }

app/src/test/kotlin/org/libremail/reporting/StackTraceScrubberTest.kt

@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+package org.libremail.reporting
+
+import org.junit.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlin.test.assertTrue
+
+class StackTraceScrubberTest {
+
+    @Test
+    fun `drops host, ip, port and email from a connect-exception message but keeps classes and frames`() {
+        val raw = listOf(
+            "java.net.ConnectException: Failed to connect to imap.example.com/93.184.216.34:993 for user@example.com",
+            "\tat org.libremail.mail.ImapClient.connect(ImapClient.kt:42)",
+            "\tat org.libremail.mail.ImapClient.open(ImapClient.kt:17)",
+            "Caused by: java.net.SocketException: Broken pipe to smtp.example.com:587",
+            "\tat java.base/sun.nio.ch.Net.connect(Net.java:579)",
+            "\t... 12 more",
+        ).joinToString("\n")
+
+        val scrubbed = StackTraceScrubber.scrub(raw)
+
+        // PII gone: hostnames, host:port tokens, the IP address and the email.
+        assertFalse(scrubbed.contains("imap.example.com"), scrubbed)
+        assertFalse(scrubbed.contains("smtp.example.com"), scrubbed)
+        assertFalse(scrubbed.contains("93.184.216.34"), scrubbed)
+        assertFalse(scrubbed.contains(":993"), scrubbed)
+        assertFalse(scrubbed.contains(":587"), scrubbed)
+        assertFalse(scrubbed.contains("user@example.com"), scrubbed)
+        assertFalse(scrubbed.contains("example"), scrubbed)
+
+        // Useful non-PII structure kept: exception class names and every frame (class/method/file/line).
+        assertTrue(scrubbed.contains("java.net.ConnectException"), scrubbed)
+        assertTrue(scrubbed.contains("Caused by: java.net.SocketException"), scrubbed)
+        assertTrue(scrubbed.contains("at org.libremail.mail.ImapClient.connect(ImapClient.kt:42)"), scrubbed)
+        // A frame's own File.java:line is preserved — it must not be mistaken for a host:port.
+        assertTrue(scrubbed.contains("at java.base/sun.nio.ch.Net.connect(Net.java:579)"), scrubbed)
+        assertTrue(scrubbed.contains("... 12 more"), scrubbed)
+    }
+
+    @Test
+    fun `redacts an email and host-port left on a non-header continuation line`() {
+        // Some mail libraries append detail on a continuation line with no "Type:" prefix, so the
+        // message-drop can't strip it structurally; the regex redaction must still catch the PII.
+        val raw = listOf(
+            "javax.mail.AuthenticationFailedException: LOGIN failed",
+            "\trejected user@example.org at imap.mail.example.org:143",
+            "\tat org.libremail.mail.ImapClient.login(ImapClient.kt:88)",
+        ).joinToString("\n")
+
+        val scrubbed = StackTraceScrubber.scrub(raw)
+
+        assertFalse(scrubbed.contains("user@example.org"), scrubbed)
+        assertFalse(scrubbed.contains("imap.mail.example.org"), scrubbed)
+        assertFalse(scrubbed.contains(":143"), scrubbed)
+        assertFalse(scrubbed.contains("example"), scrubbed)
+        assertTrue(scrubbed.contains("[redacted]"), scrubbed)
+        assertTrue(scrubbed.contains("javax.mail.AuthenticationFailedException"), scrubbed)
+        assertTrue(scrubbed.contains("at org.libremail.mail.ImapClient.login(ImapClient.kt:88)"), scrubbed)
+    }
+
+    @Test
+    fun `keeps a null-message exception and its frames verbatim`() {
+        // The common app-logic crash (no PII, no message) must survive untouched.
+        val raw = listOf(
+            "java.lang.NullPointerException",
+            "\tat org.libremail.MainActivity.onCreate(MainActivity.kt:10)",
+        ).joinToString("\n")
+
+        assertEquals(raw, StackTraceScrubber.scrub(raw))
+    }
+}

app/src/test/kotlin/org/libremail/ui/reporting/StartupReportViewModelTest.kt

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 package org.libremail.ui.reporting
 
+import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.launch
@@ -64,7 +65,9 @@ class StartupReportViewModelTest {
 
     private fun crash(id: String, createdAt: Long) = report(id, ReportKind.CRASH, createdAt)
 
-    private fun store() = ReportStore(tempFolder.root)
+    // Unconfined scope runs ReportStore's initial scan inline so a store built over an existing dir
+    // is readable synchronously, as before the scan moved off-thread (#296).
+    private fun store() = ReportStore(tempFolder.root, CoroutineScope(Dispatchers.Unconfined))
 
     private fun viewModel(store: ReportStore) = StartupReportViewModel(store, now = { now })