From 0cb9bb2906b0e427df3ba128da4ad731cace6109 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 4 Jul 2026 21:12:59 -0500 Subject: [PATCH] refactor(data): route DB/keystore logging through AppLog (#327) Migrates the DB/keystore area's raw android.util.Log calls to AppLog so key-invalidation and DB-conversion breadcrumbs land in the process RingLogBuffer (and thus a user-reviewed debug report) even in release builds, where Log.d is otherwise stripped from Logcat only. - DatabaseKeyCipher: 4 auth-bound-key decision points (encrypt retry, isInvalidated's three branches) now log via AppLog.d(tag, msg, e). - DatabaseEncryption.migrate: adds an AppLog.i "converting local cache database (targetEncrypted=...)" breadcrumb at the start, alongside the existing "converted" completion line now routed through AppLog.d. - AccountDataMigrator: the "moved account tables into the account database: $present" breadcrumb (table names only) now routed through AppLog.d. No PII or key material is logged; table-name sets and boolean flags only. Adds instrumented tests (DatabaseKeyCipher is device-only and behavior-preserving, so no new test there) asserting the breadcrumbs land in a RingLogBuffer and never contain the seeded email, secret, or passphrase. Part of #324. Co-Authored-By: Claude Opus 4.8 --- .../data/local/AccountDataMigratorTest.kt | 23 +++++++++++ .../data/local/DatabaseEncryptionTest.kt | 38 +++++++++++++++++++ .../data/local/AccountDataMigrator.kt | 4 +- .../data/local/DatabaseEncryption.kt | 5 ++- .../data/security/DatabaseKeyCipher.kt | 10 ++--- 5 files changed, 71 insertions(+), 9 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index a53c863..8db0847 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -13,6 +13,7 @@ import kotlinx.coroutines.runBlocking import org.json.JSONObject import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue @@ -21,6 +22,8 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremail.data.local.entity.CredentialEntity +import org.libremail.reporting.AppLog +import org.libremail.reporting.RingLogBuffer /** * The one-time move performed by [AccountDataMigrator] (issue #111): copying accounts / credentials / @@ -142,6 +145,26 @@ class AccountDataMigratorTest { } } + @Test + fun copyEmitsANonPiiAppLogBreadcrumbNamingOnlyTheMovedTables() = runBlocking { + seedVersion14Cache() + val buffer = RingLogBuffer() + AppLog.install(buffer) + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + val entry = buffer.snapshot() + .single { it.message.startsWith("moved account tables into the account database") } + assertEquals("the migration breadcrumb is a debug line", 'D', entry.level) + listOf("accounts", "credentials", "account_settings", "signatures").forEach { table -> + assertTrue("breadcrumb must name the moved table $table", entry.message.contains(table)) + } + // The breadcrumb carries only table names — never the seeded email, secret, or passphrase. + assertFalse(entry.message.contains("ada@example.org")) + assertFalse(entry.message.contains("sealed-secret")) + assertFalse(entry.message.contains(passphrase)) + } + @Test fun reRunningTheCopyIsIdempotentAndKeepsLaterEdits() = runBlocking { seedVersion14Cache() diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt index 53609ee..a4b8c30 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/DatabaseEncryptionTest.kt @@ -17,6 +17,8 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.libremail.data.local.entity.MessageEntity +import org.libremail.reporting.AppLog +import org.libremail.reporting.RingLogBuffer import java.io.File /** @@ -150,6 +152,42 @@ class DatabaseEncryptionTest { assertEquals("Room's schema version must survive the plaintext -> encrypted conversion", 19, version) } + @Test + fun conversionEmitsNonPiiAppLogBreadcrumbs() = runBlocking { + val buffer = RingLogBuffer() + AppLog.install(buffer) + + // The seeded row carries an email address so the PII assertions below are meaningful. + openPlaintext().apply { + messageDao().insertNew(listOf(message("acct:1"))) + close() + } + + DatabaseEncryption.ensureEncrypted(dbFile, passphrase) + val afterEncrypt = buffer.snapshot() + val converting = afterEncrypt.single { it.message.startsWith("converting local cache database") } + assertEquals("the start breadcrumb is informational", 'I', converting.level) + assertEquals("converting local cache database (targetEncrypted=true)", converting.message) + val convertedAfterEncrypt = afterEncrypt.single { it.message == "local cache database converted" } + assertEquals('D', convertedAfterEncrypt.level) + + // Converting back to plaintext logs the same pair with the flag flipped. + buffer.clear() + DatabaseEncryption.ensurePlaintext(dbFile, passphrase) + val afterDecrypt = buffer.snapshot() + assertTrue( + afterDecrypt.any { it.message == "converting local cache database (targetEncrypted=false)" }, + ) + assertTrue(afterDecrypt.any { it.message == "local cache database converted" }) + + // Neither conversion's breadcrumbs may leak the passphrase, the on-disk path, or account PII. + (afterEncrypt + afterDecrypt).forEach { entry -> + assertFalse("must not leak the passphrase", entry.message.contains(passphrase)) + assertFalse("must not leak the db file path", entry.message.contains(dbFile.absolutePath)) + assertFalse("must not leak the seeded email", entry.message.contains("ada@example.org")) + } + } + private fun openPlaintext(): LibreMailDatabase = Room.databaseBuilder(context, LibreMailDatabase::class.java, dbName).build() diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt index a1be640..74769b1 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -2,7 +2,6 @@ package org.libremail.data.local import android.content.Context -import android.util.Log import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.booleanPreferencesKey @@ -15,6 +14,7 @@ import kotlinx.coroutines.withContext import net.zetetic.database.sqlcipher.SQLiteDatabase import org.libremail.data.security.DatabaseKeyStore import org.libremail.data.settings.SettingsRepository +import org.libremail.reporting.AppLog import java.io.File import javax.inject.Inject import javax.inject.Singleton @@ -181,7 +181,7 @@ class AccountDataMigrator @Inject constructor( "(SELECT COUNT(*) FROM `accounts` AS ranked WHERE ranked.`email` < `accounts`.`email`)", ) } - Log.d(TAG, "moved account tables into the account database: $present") + AppLog.d(TAG, "moved account tables into the account database: $present") } finally { db.rawExecSQL("DETACH DATABASE cache;") } diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt index 6694a2c..7122f26 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.local -import android.util.Log import net.zetetic.database.sqlcipher.SQLiteDatabase +import org.libremail.reporting.AppLog import java.io.File /** @@ -40,6 +40,7 @@ object DatabaseEncryption { * tables but not that pragma, and a reset version would make Room attempt a bogus migration. */ private fun migrate(dbFile: File, sourcePassphrase: String, targetPassphrase: String) { + AppLog.i(TAG, "converting local cache database (targetEncrypted=${targetPassphrase.isNotEmpty()})") ensureNativeLibraryLoaded() val dir = dbFile.parentFile ?: error("database file has no parent directory") val tmp = File(dir, dbFile.name + ".migrate").apply { delete() } @@ -88,7 +89,7 @@ object DatabaseEncryption { tmp.copyTo(dbFile, overwrite = true) tmp.delete() } - Log.d(TAG, "local cache database converted") + AppLog.d(TAG, "local cache database converted") } private fun startsWithSqliteHeader(dbFile: File): Boolean { diff --git a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt index 41fe348..1d51852 100644 --- a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt +++ b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt @@ -5,7 +5,7 @@ import android.os.Build import android.security.keystore.KeyGenParameterSpec import android.security.keystore.KeyPermanentlyInvalidatedException import android.security.keystore.UserNotAuthenticatedException -import android.util.Log +import org.libremail.reporting.AppLog import javax.inject.Inject import javax.inject.Singleton @@ -43,7 +43,7 @@ class DatabaseKeyCipher @Inject constructor() : override fun encrypt(plaintext: String): String = try { super.encrypt(plaintext) } catch (e: KeyPermanentlyInvalidatedException) { - Log.d(TAG, "replacing invalidated auth-bound key before sealing", e) + AppLog.d(TAG, "replacing invalidated auth-bound key before sealing", e) deleteKey() super.encrypt(plaintext) } @@ -60,16 +60,16 @@ class DatabaseKeyCipher @Inject constructor() : initEncryptCipher(key) false } catch (e: KeyPermanentlyInvalidatedException) { - Log.d(TAG, "auth-bound database key invalidated", e) + AppLog.d(TAG, "auth-bound database key invalidated", e) true } catch (e: UserNotAuthenticatedException) { // Valid key, just outside its time-bound auth window — not invalidated. - Log.d(TAG, "auth-bound key outside its auth window; not invalidated", e) + AppLog.d(TAG, "auth-bound key outside its auth window; not invalidated", e) false } catch (e: Exception) { // Never let a validity probe crash the foreground pass; a real decrypt later surfaces any // genuine problem. Treat an unknown probe failure as "not invalidated" (don't wipe). - Log.d(TAG, "auth-bound key validity probe failed; treating as valid", e) + AppLog.d(TAG, "auth-bound key validity probe failed; treating as valid", e) false } } -- 2.47.3