From a40cc2d2d40f01321a09792fcf2efcdbaba17c79 Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Sun, 30 Aug 2026 09:13:16 -0400 Subject: [PATCH] Measure Android coverage, and round-trip the vault MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JaCoCo was not configured on either JVM module, so the Android numbers were unknown rather than good. :engine and :client are plain Kotlin/JVM precisely so they can be measured without an emulator; now they are. Baseline: engine 84.9% lines, client 47.3%. Vault was the outlier in engine at 53%, and its uncovered half was almost entirely save/load pairs — credentials, trust pins, the delivery key, the key-log pins, the roster, avatars, per-line deletion. That is not an arbitrary gap. It is the exact shape of a bug that has already shipped in this app twice. `requireKeyLog` was computed, held in memory and never written down, so a security control the UI offered silently reverted on every relaunch. `Trust.changed` says the same thing in its own doc comment: returned by `record()`, persisted by nothing, so `state()` could never answer CHANGED, the "Accept new keys" button could never render, and a contact who reinstalled became permanently unmessageable while the header insisted nothing had changed. Neither was hard to write. Both were invisible because a save and a load that disagree still compile and only differ after a restart, which no test performed. Every test here writes with one Vault and reads with a second built over the same bytes, because that is the only arrangement in which the two halves are forced to agree. Includes the AAD binding the class doc promises: a trust record copied into another contact's slot must not open, or someone with file access could attribute a stranger's keys to someone you trust. Vault 53.0% -> 84.9% lines, engine 84.9% -> 92.3%. Co-Authored-By: Claude Opus 4.8 --- client/build.gradle.kts | 10 + engine/build.gradle.kts | 10 + .../io/flamenet/e2e/VaultPersistenceTest.kt | 342 ++++++++++++++++++ 3 files changed, 362 insertions(+) create mode 100644 engine/src/test/kotlin/io/flamenet/e2e/VaultPersistenceTest.kt diff --git a/client/build.gradle.kts b/client/build.gradle.kts index 43d33a3..d791939 100644 --- a/client/build.gradle.kts +++ b/client/build.gradle.kts @@ -6,6 +6,7 @@ // OkHttp works identically on both. plugins { kotlin("jvm") + jacoco } repositories { mavenCentral() } @@ -38,3 +39,12 @@ tasks.test { useJUnitPlatform() testLogging { events("passed", "failed", "skipped") } } + +// Coverage for this module. `:engine` and `:client` are plain JVM libraries +// precisely so they can be measured and tested without an emulator, so there is +// no reason for their numbers to be a guess. +tasks.withType().configureEach { finalizedBy(tasks.named("jacocoTestReport")) } +tasks.named("jacocoTestReport") { + dependsOn(tasks.named("test")) + reports { xml.required.set(true); html.required.set(true) } +} diff --git a/engine/build.gradle.kts b/engine/build.gradle.kts index f291900..7623a47 100644 --- a/engine/build.gradle.kts +++ b/engine/build.gradle.kts @@ -1,5 +1,6 @@ plugins { kotlin("jvm") + jacoco } repositories { mavenCentral() } @@ -58,3 +59,12 @@ tasks.test { showStandardStreams = true } } + +// Coverage for this module. `:engine` and `:client` are plain JVM libraries +// precisely so they can be measured and tested without an emulator, so there is +// no reason for their numbers to be a guess. +tasks.withType().configureEach { finalizedBy(tasks.named("jacocoTestReport")) } +tasks.named("jacocoTestReport") { + dependsOn(tasks.named("test")) + reports { xml.required.set(true); html.required.set(true) } +} diff --git a/engine/src/test/kotlin/io/flamenet/e2e/VaultPersistenceTest.kt b/engine/src/test/kotlin/io/flamenet/e2e/VaultPersistenceTest.kt new file mode 100644 index 0000000..4c1d376 --- /dev/null +++ b/engine/src/test/kotlin/io/flamenet/e2e/VaultPersistenceTest.kt @@ -0,0 +1,342 @@ +package io.flamenet.e2e + +import org.json.JSONArray +import org.json.JSONObject +import javax.crypto.KeyGenerator +import javax.crypto.SecretKey +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Round trips for everything the vault remembers. + * + * 🔴 **This class of bug has already shipped here once.** `requireKeyLog` was + * computed, held in memory, and never written down, so a security control the + * UI offered silently reverted on every relaunch. `Trust.changed` carries the + * same warning in its own doc comment: it was returned by `record()` and + * persisted by nothing, so a contact who reinstalled became permanently + * unmessageable while the header insisted nothing had changed. + * + * Neither was a hard bug to write. Both were invisible because a save and a + * load that disagree still compile, still run, and only differ after a restart + * — which no test performed. Every test below writes with one `Vault` instance + * and reads with a second one built over the same bytes, because that is the + * only arrangement in which the two halves have to agree. + */ +class VaultPersistenceTest { + + private class MapStorage : Vault.Storage { + val records = LinkedHashMap() + override fun read(name: String) = records[name] + override fun write(name: String, bytes: ByteArray) { records[name] = bytes } + override fun delete(name: String) { records.remove(name) } + override fun names() = records.keys.toList() + } + + private fun key(): SecretKey = + KeyGenerator.getInstance("AES").apply { init(256) }.generateKey() + + /** Write with one instance, read with another — a restart, in effect. */ + private fun roundTrip(write: (Vault) -> Unit, read: (Vault) -> T): T { + val storage = MapStorage(); val secret = key() + write(Vault(storage, secret)) + return read(Vault(storage, secret)) + } + + // -- Credential ---------------------------------------------------------- + + @Test + fun `a credential survives a restart`() { + val original = Vault.Credential( + relayBaseUrl = "https://relay.example", username = "ada", + appPassword = "hunter2", userId = 42, fnid = "ada@example") + val restored = roundTrip({ it.saveCredential(original) }, { it.loadCredential() }) + assertEquals(original, restored) + } + + /** + * A credential written before addresses existed is still a valid + * credential and must not fail to load. + */ + @Test + fun `a credential with no address round-trips as null rather than empty`() { + val restored = roundTrip({ + it.saveCredential(Vault.Credential( + "https://relay.example", "ada", "pw", 42, fnid = null)) + }, { it.loadCredential() }) + assertNull(restored!!.fnid) + } + + @Test + fun `no credential reads back as null, not as a blank one`() { + assertNull(Vault(MapStorage(), key()).loadCredential()) + } + + // -- Trust --------------------------------------------------------------- + + @Test + fun `a trust record survives a restart`() { + val original = Vault.Trust( + identities = listOf("ik-one", "ik-two"), verified = true, + fnid = "bob@example", changed = false) + val restored = roundTrip({ it.saveTrust(7, original) }, { it.loadTrust(7) }) + assertEquals(original, restored) + } + + /** + * 🔴 The flag whose absence made reinstalled contacts unmessageable. If + * this does not persist, `state()` can never answer CHANGED, so the + * "Accept new keys" button never renders and the only route back is + * clearing app data. + */ + @Test + fun `a detected key change is still there after a restart`() { + val restored = roundTrip({ + it.saveTrust(7, Vault.Trust(listOf("ik-old"), verified = false, changed = true)) + }, { it.loadTrust(7) }) + assertTrue(restored!!.changed) + // and the pinned identities are unchanged: the flag records that + // something else was OFFERED, not that we believe it. + assertEquals(listOf("ik-old"), restored.identities) + } + + @Test + fun `trust with no address round-trips as null`() { + val restored = roundTrip({ + it.saveTrust(7, Vault.Trust(listOf("ik"), verified = false, fnid = null)) + }, { it.loadTrust(7) }) + assertNull(restored!!.fnid) + } + + @Test + fun `trust is per contact`() { + val storage = MapStorage(); val secret = key() + val v = Vault(storage, secret) + v.saveTrust(1, Vault.Trust(listOf("ik-1"), verified = true)) + v.saveTrust(2, Vault.Trust(listOf("ik-2"), verified = false)) + val reread = Vault(storage, secret) + assertEquals(listOf("ik-1"), reread.loadTrust(1)!!.identities) + assertEquals(listOf("ik-2"), reread.loadTrust(2)!!.identities) + assertTrue(reread.loadTrust(1)!!.verified) + assertTrue(!reread.loadTrust(2)!!.verified) + } + + @Test + fun `forgetting a contact's trust removes it and leaves others alone`() { + val storage = MapStorage(); val secret = key() + val v = Vault(storage, secret) + v.saveTrust(1, Vault.Trust(listOf("ik-1"), verified = true)) + v.saveTrust(2, Vault.Trust(listOf("ik-2"), verified = true)) + v.forgetTrust(1) + val reread = Vault(storage, secret) + assertNull(reread.loadTrust(1)) + assertEquals(listOf("ik-2"), reread.loadTrust(2)!!.identities) + } + + /** + * 🔴 **Every record's name is bound into the AAD**, so a blob is not + * portable between slots. Without that, someone with file access could + * paste one contact's pinned keys into another contact's slot and the app + * would accept them — attributing a stranger's identity to someone you + * trust. + */ + @Test + fun `a trust record moved into another contact's slot does not open`() { + val storage = MapStorage(); val secret = key() + val v = Vault(storage, secret) + v.saveTrust(1, Vault.Trust(listOf("ik-victim"), verified = true)) + v.saveTrust(2, Vault.Trust(listOf("ik-other"), verified = false)) + + val slotOne = storage.names().first { it.endsWith("/1") || it.endsWith("1") } + val slotTwo = storage.names().first { it != slotOne } + storage.records[slotTwo] = storage.records[slotOne]!!.copyOf() + + assertFailsWith { Vault(storage, secret).loadTrust(2) } + } + + // -- Sealed sender delivery key ----------------------------------------- + + @Test + fun `a delivery key survives a restart with its peers`() { + val mine = ByteArray(32) { 9 } + val restored = roundTrip({ + it.saveDelivery(Vault.Delivery( + key = mine, published = true, + peers = mapOf(5L to ByteArray(32) { 1 }, 6L to ByteArray(32) { 2 }), + toldPeers = setOf(5L))) + }, { it.loadDelivery() })!! + + assertContentEquals(mine, restored.key) + assertTrue(restored.published) + assertEquals(setOf(5L, 6L), restored.peers.keys) + assertContentEquals(ByteArray(32) { 1 }, restored.peers[5L]) + assertEquals(setOf(5L), restored.toldPeers) + } + + /** + * `published` is the relay's knowledge, not ours. Losing it across a + * restart means either re-publishing forever or believing a key is + * reachable when the relay has never seen its digest. + */ + @Test + fun `an unpublished delivery key does not come back as published`() { + val restored = roundTrip({ + it.saveDelivery(Vault.Delivery(ByteArray(32) { 3 }, published = false)) + }, { it.loadDelivery() })!! + assertTrue(!restored.published) + } + + // -- Key transparency pins ---------------------------------------------- + + /** + * The pin is what makes the log append-only from this device's point of + * view: a head smaller than the one we already saw is a rewind. A pin that + * does not survive a restart makes every launch trust whatever it is + * handed first. + */ + @Test + fun `a key log pin survives a restart`() { + val restored = roundTrip({ + it.saveKeyLogPin(Vault.KeyLogPin(size = 128, root = "cm9vdA==")) + }, { it.loadKeyLogPin() })!! + assertEquals(128, restored.size) + assertEquals("cm9vdA==", restored.root) + } + + @Test + fun `the pinned issuer key survives a restart`() { + val issuer = ByteArray(32) { it.toByte() } + val restored = roundTrip({ it.saveKeyLogIssuerPin(issuer) }, + { it.keyLogIssuerPin() }) + assertContentEquals(issuer, restored) + } + + @Test + fun `no issuer pin reads back as null`() { + assertNull(Vault(MapStorage(), key()).keyLogIssuerPin()) + } + + // -- Roster -------------------------------------------------------------- + + @Test + fun `a roster survives a restart with its off-server flag`() { + val items = JSONArray().put(JSONObject().put("id", 1).put("name", "ada")) + val (readItems, offServer) = roundTrip({ it.saveRoster(items, offServer = true) }, + { it.loadRoster() }) + assertEquals(1, readItems.length()) + assertEquals("ada", readItems.getJSONObject(0).getString("name")) + assertTrue(offServer) + } + + @Test + fun `an absent roster reads back empty rather than null`() { + val (items, offServer) = Vault(MapStorage(), key()).loadRoster() + assertEquals(0, items.length()) + assertTrue(!offServer) + } + + // -- Avatars ------------------------------------------------------------- + + private fun ref(id: String = "blob-1") = Attachment.Ref( + kind = Attachment.IMAGE, id = id, key = ByteArray(32) { 4 }, + digest = ByteArray(32) { 5 }, bytes = 100, mime = "image/jpeg", + width = 64, height = 64) + + @Test + fun `a contact's avatar survives a restart`() { + val restored = roundTrip({ it.saveAvatar(11, ref()) }, { it.avatarFor(11) })!! + assertEquals("blob-1", restored.id) + assertContentEquals(ByteArray(32) { 4 }, restored.key) + } + + @Test + fun `clearing a contact's avatar is remembered`() { + val storage = MapStorage(); val secret = key() + Vault(storage, secret).saveAvatar(11, ref()) + Vault(storage, secret).saveAvatar(11, null) + assertNull(Vault(storage, secret).avatarFor(11)) + } + + @Test + fun `my own avatar survives a restart`() { + val restored = roundTrip({ it.saveMyAvatar(ref("mine")) }, { it.myAvatar() })!! + assertEquals("mine", restored.id) + } + + /** + * Whether a peer already has our avatar is what stops us re-sending it on + * every message. Forgetting it across a restart turns a one-time upload + * into a per-launch one. + */ + @Test + fun `who we have already shared an avatar with is remembered`() { + val restored = roundTrip({ it.markAvatarShared(11, "blob-1") }, + { it.avatarSharedWith(11) }) + assertEquals("blob-1", restored) + } + + // -- History deletion ---------------------------------------------------- + + private fun line(at: Long, mine: Boolean, text: String) = + JSONObject().put("at", at).put("mine", mine).put("text", text) + + @Test + fun `deleting one line leaves the rest of the thread intact`() { + val storage = MapStorage(); val secret = key() + val v = Vault(storage, secret) + v.replaceHistory("t", JSONArray() + .put(line(1, true, "first")) + .put(line(2, false, "second")) + .put(line(3, true, "third"))) + + Vault(storage, secret).deleteLine("t", at = 2, mine = false, text = "second") + + val kept = Vault(storage, secret).history("t") + assertEquals(2, kept.length()) + assertEquals("first", kept.getJSONObject(0).getString("text")) + assertEquals("third", kept.getJSONObject(1).getString("text")) + } + + /** + * Keyed by timestamp AND side AND text, so two messages in the same + * millisecond cannot take each other down. + */ + @Test + fun `deleting is precise about which line it removes`() { + val storage = MapStorage(); val secret = key() + Vault(storage, secret).replaceHistory("t", JSONArray() + .put(line(5, true, "same ms mine")) + .put(line(5, false, "same ms theirs")) + .put(line(5, true, "same ms mine too"))) + + Vault(storage, secret).deleteLine("t", at = 5, mine = true, text = "same ms mine") + + val kept = Vault(storage, secret).history("t") + assertEquals(2, kept.length()) + assertEquals("same ms theirs", kept.getJSONObject(0).getString("text")) + assertEquals("same ms mine too", kept.getJSONObject(1).getString("text")) + } + + @Test + fun `deleting a line that is not there changes nothing`() { + val storage = MapStorage(); val secret = key() + Vault(storage, secret).replaceHistory("t", JSONArray().put(line(1, true, "only"))) + Vault(storage, secret).deleteLine("t", at = 99, mine = true, text = "absent") + assertEquals(1, Vault(storage, secret).history("t").length()) + } + + /** "Deleted means deleted" — the record goes, not a tombstone. */ + @Test + fun `forgetting a thread removes its record entirely`() { + val storage = MapStorage(); val secret = key() + Vault(storage, secret).replaceHistory("t", JSONArray().put(line(1, true, "hi"))) + val before = storage.names().size + Vault(storage, secret).forgetHistory("t") + assertEquals(before - 1, storage.names().size) + assertEquals(0, Vault(storage, secret).history("t").length()) + } +}