From 1a1fbf8d7f3ce830aa161ce4fca45761785a0e97 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 4 Jul 2026 20:23:38 -0500 Subject: [PATCH] =?UTF-8?q?feat(logging):=20AppLog=20seam=20=E2=80=94=20re?= =?UTF-8?q?cord=20scrubbed=20throwables=20+=20accountLogRef=20(#325)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add throwable-recording overloads to AppLog.d/w and make AppLog.e record the throwable it is given: the throwable's stack trace is scrubbed via the existing StackTraceScrubber (exception class names + frames kept; host/email-bearing exception messages stripped) and appended to the buffered log line, so a throwable can reach a user-reviewed DebugReport without leaking PII. The existing no-throwable overloads are unchanged. Add accountLogRef(accountId): a short, stable, non-reversible reference (scheme prefix + truncated SHA-256 of the id) so downstream logging can identify an account without logging the raw Account.id, which embeds the email. Foundation for the #324 debug-logging strangler epic; consumed by #326–#330. Co-Authored-By: Claude Opus 4.8 --- .../org/libremail/reporting/AccountLogRef.kt | 39 ++++++++++ .../kotlin/org/libremail/reporting/AppLog.kt | 28 +++++++- .../libremail/reporting/AccountLogRefTest.kt | 72 +++++++++++++++++++ .../org/libremail/reporting/AppLogTest.kt | 59 ++++++++++++++- .../reporting/AppLogUninstalledTest.kt | 11 ++- 5 files changed, 203 insertions(+), 6 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/reporting/AccountLogRef.kt create mode 100644 app/src/test/kotlin/org/libremail/reporting/AccountLogRefTest.kt diff --git a/app/src/main/kotlin/org/libremail/reporting/AccountLogRef.kt b/app/src/main/kotlin/org/libremail/reporting/AccountLogRef.kt new file mode 100644 index 0000000..ab94bbf --- /dev/null +++ b/app/src/main/kotlin/org/libremail/reporting/AccountLogRef.kt @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.reporting + +import java.security.MessageDigest + +/** + * A short, stable, non-reversible reference for an account — safe to write to logs and a [DebugReport]. + * + * [Account.id][org.libremail.domain.model.Account.id] embeds the raw email address (e.g. + * `"outlook:user@domain.com"` or `"imap:user@domain.com"`), so it is PII and must **never** be logged + * directly. This returns the id's scheme prefix followed by a truncated SHA-256 of the whole id — e.g. + * `"outlook:a1b2c3"` — which is: + * + * - **stable**: the same id always maps to the same reference (a hash, not a random token), so lines + * for one account correlate across a session and across reports; + * - **non-reversible**: a truncated one-way hash cannot be turned back into the email; + * - **non-PII**: the scheme (`outlook`/`imap`) names the auth kind, not the user, and the hex hash + * contains no `@`, domain, or local part. If the part before the first `:` is not a bare token + * (e.g. an id that is itself an address), it is replaced with [GENERIC_SCHEME] so no address can + * leak through the prefix. + */ +fun accountLogRef(accountId: String): String { + val candidate = accountId.substringBefore(':', missingDelimiterValue = "") + val scheme = candidate.takeIf(::isBareScheme) ?: GENERIC_SCHEME + return "$scheme:${sha256Hex(accountId).take(REF_HASH_LENGTH)}" +} + +/** Prefix used when an id has no scheme, or one that could itself carry PII (an `@`, a dot, …). */ +private const val GENERIC_SCHEME = "acct" + +/** Hex characters of the SHA-256 kept in the reference; short but collision-safe for a device's few accounts. */ +private const val REF_HASH_LENGTH = 6 + +/** A bare scheme token is letters/digits only, so an address (with `@`/`.`) can never pass as a scheme. */ +private fun isBareScheme(text: String): Boolean = text.isNotEmpty() && text.all(Char::isLetterOrDigit) + +private fun sha256Hex(input: String): String = MessageDigest.getInstance("SHA-256") + .digest(input.toByteArray(Charsets.UTF_8)) + .joinToString("") { "%02x".format(it) } diff --git a/app/src/main/kotlin/org/libremail/reporting/AppLog.kt b/app/src/main/kotlin/org/libremail/reporting/AppLog.kt index 74a2b7f..e48b20b 100644 --- a/app/src/main/kotlin/org/libremail/reporting/AppLog.kt +++ b/app/src/main/kotlin/org/libremail/reporting/AppLog.kt @@ -8,6 +8,12 @@ import android.util.Log * be attached to a user-reviewed [DebugReport]. Call [install] once at startup. Never pass PII * (email addresses, message content, credentials) to these methods — the buffer can end up in a * report the user reviews and may submit. + * + * The throwable-carrying [d]/[w]/[e] overloads forward the throwable to Logcat as usual, but record a + * **PII-scrubbed** rendering of its stack trace into the buffer via [StackTraceScrubber]: exception + * class names and stack frames are kept, while exception *messages* — which can embed a server host or + * an account email (e.g. a `ConnectException` or an auth failure) — are stripped. To refer to an + * account in a log line, log [accountLogRef]`(account.id)` rather than the raw id, which is PII. */ object AppLog { @Volatile @@ -22,6 +28,11 @@ object AppLog { buffer?.record('D', tag, message) } + fun d(tag: String, message: String, throwable: Throwable?) { + Log.d(tag, message, throwable) + buffer?.record('D', tag, bufferLine(message, throwable)) + } + fun i(tag: String, message: String) { Log.i(tag, message) buffer?.record('I', tag, message) @@ -32,8 +43,23 @@ object AppLog { buffer?.record('W', tag, message) } + fun w(tag: String, message: String, throwable: Throwable?) { + Log.w(tag, message, throwable) + buffer?.record('W', tag, bufferLine(message, throwable)) + } + fun e(tag: String, message: String, throwable: Throwable? = null) { Log.e(tag, message, throwable) - buffer?.record('E', tag, message) + buffer?.record('E', tag, bufferLine(message, throwable)) + } + + /** + * The buffer text for a log call: the caller [message] alone, or — when a [throwable] is present — + * the message followed by the throwable's **scrubbed** stack trace (exception class names and stack + * frames kept; host/email-bearing exception messages stripped by [StackTraceScrubber]). + */ + private fun bufferLine(message: String, throwable: Throwable?): String { + if (throwable == null) return message + return "$message\n" + StackTraceScrubber.scrub(throwable.stackTraceToString()) } } diff --git a/app/src/test/kotlin/org/libremail/reporting/AccountLogRefTest.kt b/app/src/test/kotlin/org/libremail/reporting/AccountLogRefTest.kt new file mode 100644 index 0000000..d03d035 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/reporting/AccountLogRefTest.kt @@ -0,0 +1,72 @@ +// 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.assertNotEquals +import kotlin.test.assertTrue + +/** + * [accountLogRef] turns a PII-bearing `Account.id` (which embeds the raw email) into a short, stable, + * non-reversible reference that is safe to write to logs and a [DebugReport]. + */ +class AccountLogRefTest { + + @Test + fun `is deterministic and stable for the same id`() { + val id = "outlook:user@example.com" + assertEquals(accountLogRef(id), accountLogRef(id)) + } + + @Test + fun `differs across different ids`() { + assertNotEquals( + accountLogRef("outlook:user@example.com"), + accountLogRef("outlook:other@example.com"), + ) + // Same address under a different scheme is a different account, so it must map to a different ref. + assertNotEquals( + accountLogRef("outlook:user@example.com"), + accountLogRef("imap:user@example.com"), + ) + } + + @Test + fun `contains neither the email nor its domain and is not the raw id`() { + val id = "imap:Alice.Smith@example.com" + + val ref = accountLogRef(id) + + assertNotEquals(id, ref) + assertFalse(ref.contains("@"), ref) + assertFalse(ref.contains("Alice.Smith"), ref) + assertFalse(ref.contains("example.com"), ref) + assertFalse(ref.contains("example"), ref) + } + + @Test + fun `keeps the non-PII scheme prefix so refs stay readable`() { + assertTrue(accountLogRef("outlook:user@example.com").startsWith("outlook:"), "outlook prefix") + assertTrue(accountLogRef("imap:user@example.com").startsWith("imap:"), "imap prefix") + } + + @Test + fun `falls back to a generic prefix for an id with no scheme, never leaking the address`() { + val ref = accountLogRef("user@example.com") + + assertTrue(ref.startsWith("acct:"), ref) + assertFalse(ref.contains("@"), ref) + assertFalse(ref.contains("example"), ref) + } + + @Test + fun `never treats an address-shaped prefix as the scheme`() { + // If the part before the first ':' is itself an address, it must not surface as the prefix. + val ref = accountLogRef("user@example.com:143") + + assertTrue(ref.startsWith("acct:"), ref) + assertFalse(ref.contains("@"), ref) + assertFalse(ref.contains("example"), ref) + } +} diff --git a/app/src/test/kotlin/org/libremail/reporting/AppLogTest.kt b/app/src/test/kotlin/org/libremail/reporting/AppLogTest.kt index 10a0696..9094c4d 100644 --- a/app/src/test/kotlin/org/libremail/reporting/AppLogTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/AppLogTest.kt @@ -9,7 +9,10 @@ import io.mockk.verify import org.junit.After import org.junit.Before import org.junit.Test +import java.net.ConnectException import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue /** * [AppLog] mirrors every Logcat call into the process [RingLogBuffer] so recent activity can be @@ -24,8 +27,10 @@ class AppLogTest { fun setUp() { mockkStatic(Log::class) every { Log.d(any(), any()) } returns 0 + every { Log.d(any(), any(), any()) } returns 0 every { Log.i(any(), any()) } returns 0 every { Log.w(any(), any()) } returns 0 + every { Log.w(any(), any(), any()) } returns 0 every { Log.e(any(), any(), any()) } returns 0 AppLog.install(buffer) } @@ -39,15 +44,14 @@ class AppLogTest { AppLog.i("Tag", "info line") AppLog.w("Tag", "warn line") AppLog.e("Tag", "error line") - AppLog.e("Tag", "error with cause", IllegalStateException("boom")) val snapshot = buffer.snapshot() assertEquals( - listOf('D', 'I', 'W', 'E', 'E'), + listOf('D', 'I', 'W', 'E'), snapshot.map { it.level }, ) assertEquals( - listOf("debug line", "info line", "warn line", "error line", "error with cause"), + listOf("debug line", "info line", "warn line", "error line"), snapshot.map { it.message }, ) verify { Log.d("Tag", "debug line") } @@ -55,4 +59,53 @@ class AppLogTest { verify { Log.w("Tag", "warn line") } verify { Log.e("Tag", "error line", null) } } + + @Test + fun `e with a throwable records the scrubbed trace - class and frames kept, host and email stripped`() { + val cause = ConnectException("Failed to connect to imap.example.com/93.184.216.34:993 for user@example.com") + + AppLog.e("Net", "connect failed", cause) + + val entry = buffer.snapshot().single() + assertEquals('E', entry.level) + // The caller message stays, and the throwable's class name + a frame are kept for usefulness. + assertTrue(entry.message.startsWith("connect failed\n"), entry.message) + assertTrue(entry.message.contains("java.net.ConnectException"), entry.message) + assertTrue(entry.message.contains("at org.libremail.reporting.AppLogTest"), entry.message) + // PII embedded in the exception message is gone: host, ip, port and email. + assertFalse(entry.message.contains("imap.example.com"), entry.message) + assertFalse(entry.message.contains("93.184.216.34"), entry.message) + assertFalse(entry.message.contains(":993"), entry.message) + assertFalse(entry.message.contains("user@example.com"), entry.message) + assertFalse(entry.message.contains("example"), entry.message) + verify { Log.e("Net", "connect failed", cause) } + } + + @Test + fun `w and d throwable overloads record the scrubbed trace and forward the throwable to Logcat`() { + val cause = ConnectException("auth failed for user@example.org at imap.mail.example.org:143") + + AppLog.w("Net", "warn cause", cause) + AppLog.d("Net", "debug cause", cause) + + val snapshot = buffer.snapshot() + assertEquals(listOf('W', 'D'), snapshot.map { it.level }) + assertEquals(listOf("warn cause", "debug cause"), snapshot.map { it.message.substringBefore('\n') }) + snapshot.forEach { entry -> + assertTrue(entry.message.contains("java.net.ConnectException"), entry.message) + assertFalse(entry.message.contains("user@example.org"), entry.message) + assertFalse(entry.message.contains("imap.mail.example.org"), entry.message) + assertFalse(entry.message.contains(":143"), entry.message) + assertFalse(entry.message.contains("example"), entry.message) + } + verify { Log.w("Net", "warn cause", cause) } + verify { Log.d("Net", "debug cause", cause) } + } + + @Test + fun `a null throwable records the message alone`() { + AppLog.w("Net", "just a warning", null) + + assertEquals("just a warning", buffer.snapshot().single().message) + } } diff --git a/app/src/test/kotlin/org/libremail/reporting/AppLogUninstalledTest.kt b/app/src/test/kotlin/org/libremail/reporting/AppLogUninstalledTest.kt index 0b02f01..4972f80 100644 --- a/app/src/test/kotlin/org/libremail/reporting/AppLogUninstalledTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/AppLogUninstalledTest.kt @@ -23,8 +23,10 @@ class AppLogUninstalledTest { fun setUp() { mockkStatic(Log::class) every { Log.d(any(), any()) } returns 0 + every { Log.d(any(), any(), any()) } returns 0 every { Log.i(any(), any()) } returns 0 every { Log.w(any(), any()) } returns 0 + every { Log.w(any(), any(), any()) } returns 0 every { Log.e(any(), any(), any()) } returns 0 AppLog::class.java.getDeclaredField("buffer").apply { isAccessible = true }.set(AppLog, null) } @@ -34,16 +36,21 @@ class AppLogUninstalledTest { @Test fun `logging before a buffer is installed only forwards to Logcat`() { + val cause = IllegalStateException("boom") AppLog.d("Tag", "debug") + AppLog.d("Tag", "debug with cause", cause) AppLog.i("Tag", "info") AppLog.w("Tag", "warn") + AppLog.w("Tag", "warn with cause", cause) AppLog.e("Tag", "error") - AppLog.e("Tag", "error with cause", IllegalStateException("boom")) + AppLog.e("Tag", "error with cause", cause) verify { Log.d("Tag", "debug") } + verify { Log.d("Tag", "debug with cause", cause) } verify { Log.i("Tag", "info") } verify { Log.w("Tag", "warn") } + verify { Log.w("Tag", "warn with cause", cause) } verify { Log.e("Tag", "error", null) } - verify { Log.e("Tag", "error with cause", any()) } + verify { Log.e("Tag", "error with cause", cause) } } }