JMR-dev/LibreMail
1
0
Fork 0
Code Issues 40 Pull Requests 2 Actions Packages Projects Releases 2 Wiki Activity

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

Browse Source 
A crash report captured throwable.stackTraceToString() verbatim, so mail/network
exceptions (Jakarta Mail, java.net) could embed server host:port tokens and account
emails/usernames in the report's stackTrace field — violating the PII-free-reports
constraint. Add StackTraceScrubber, applied in DiagnosticsCollector before the trace
enters toSubmissionPayload()/toStorageJson(): it keeps the non-PII value (exception
class names + every frame's class/method/file/line) and drops each header line's
free-text message (where hostnames/usernames live), then redacts any residual email
or host:port left on a wrapped continuation line. Frame lines are untouched, so a
frame's File.kt:42 is never mistaken for a host:port.

ReportStore did MutableStateFlow(scan()) in its constructor — a dir list + read +
JSON-parse of every stored report. As an eager @Singleton dep of CrashReporter, whose
install() runs on the MAIN thread in Application.onCreate(), this was main-thread disk
I/O that grows with the 30-day retention. Seed the flow empty and dispatch the initial
scan to an injectable scope (Dispatchers.IO by default); reactive consumers update when
it lands, and writes still re-scan synchronously so a crash-time save is never lost.

Tests: StackTraceScrubberTest (host/ip/port/email dropped from a ConnectException +
auth-failure trace while classes/frames survive; regex redaction of a continuation
line; null-message trace preserved verbatim); DiagnosticsCollector end-to-end scrub
test; ReportStore empty-seed + off-thread populate via a StandardTestDispatcher. Store
constructions in existing tests use an Unconfined scope to keep their synchronous
reopen semantics.

Closes #294
Closes #296

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
JMR-dev
2026-07-04 02:18:10 -05:00
co-authored by Claude Opus 4.8
parent 7966efde2f
commit 005f8a3aca
9 changed files with 241 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/test/kotlin/org/libremail/reporting/CrashReporterTest.kt
+9 -3
View File
@@ -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
+21 -1
View File
@@ -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
+39 -10
View File
@@ -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
+73
View File
@@ -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
+4 -1
View File
@@ -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 })