From 4a587a7d4d06e9059ec02e850e5c32abcc4b9a1c Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Fri, 4 Sep 2026 13:43:39 -0400 Subject: [PATCH 1/3] Zero key material after use (ios#39, Android half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stretched master key in AccountKdf is an intermediate nobody needs after the HKDF expansion, and it is the most valuable thing in that file: every other account secret is derived from it. It now lives inside `SecureBytes.withSecret`, which wipes it however the block leaves โ€” including out of a throw, because a derivation that failed half-way is exactly where material gets forgotten. ๐Ÿ”ด The Android guarantee is WEAKER than the iOS one, and PARITY.md says ๐ŸŸก rather than rounding it up to โœ…. iOS has `memset_s`, specified not to be optimised away, and does not page application memory to disk. Neither holds on the JVM: a compacting collector can relocate a ByteArray and leave the bytes at the old address untouched, Android has had compressed swap on by default for years, and `Arrays.fill` is only "not eliminated in practice" rather than guaranteed. There is no way to pin a Java array, so that is a ceiling rather than a gap to close later. So the claim is written narrowly: a derived key is not left sitting in the heap after use. Not "the key is gone". Spending the value of this feature on a stronger claim than it supports would be the worst possible trade. โš ๏ธ `signInSecrets` does BOTH expansions inside the block. Wiping between them would derive the account seed from thirty-two zero bytes โ€” a perfectly consistent, perfectly wrong identity, on every device, silently. That is a far worse failure than the leak this narrows, and `signInSecrets` had no pinned vector of its own, so a test compares the pair-path against the individual derivations. Proven: moving the wipe one line earlier turns it red. โš ๏ธ The limit is asserted rather than left to prose โ€” a copy taken before a wipe survives it, and that is a passing test so nobody reads `wipe` as stronger than it is. 10 unit tests. App builds, every JVM suite green including the pinned KDF vectors. Co-Authored-By: Claude Opus 5 --- PARITY.md | 15 +- .../main/kotlin/io/flamenet/e2e/AccountKdf.kt | 38 +++-- .../kotlin/io/flamenet/e2e/SecureBytes.kt | 78 ++++++++++ .../kotlin/io/flamenet/e2e/SecureBytesTest.kt | 140 ++++++++++++++++++ 4 files changed, 256 insertions(+), 15 deletions(-) create mode 100644 engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt create mode 100644 engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt diff --git a/PARITY.md b/PARITY.md index 7ba7ccd..ff3b21d 100644 --- a/PARITY.md +++ b/PARITY.md @@ -160,7 +160,7 @@ being the one that leaks. |---|:--:|:--:|---| | Back off on repeated app-lock attempts | โœ… | โœ… | ios#41 โ€” 3 free attempts, then 30s doubling to a 5-minute ceiling | | Handle a wrong clock | โœ… | โœ… | ios#45 โ€” skew learned free from the `Date` header; expiry takes the LATER of device and relay | -| Zero key material after use | โœ… | โšช | ios#39 | +| Zero key material after use | โœ… | ๐ŸŸก | ios#39 โ€” done, but the JVM guarantee is **weaker than iOS**; see below | | Pin the relay's identity | โœ… | โšช | ios#19 | โš ๏ธ **The back-off is not a defence of the data.** It slows a person holding an @@ -185,6 +185,19 @@ the UI thread, so the two fields are `@Volatile` and updated together under a lock. A torn read of "skew from now, timestamp from an hour ago" would age out an observation just made, or trust one that had not been. +๐Ÿ”ด **Zeroing is where Android cannot match iOS, and the row says ๐ŸŸก for that +reason rather than being rounded up to โœ….** iOS has `memset_s`, specified not to +be optimised away, and iOS does not page application memory to disk. Neither +holds here: a compacting collector can relocate a `ByteArray` and leave the +bytes at the old address untouched, Android has had compressed swap on by +default for years, and `Arrays.fill` is merely "not eliminated in practice" +rather than guaranteed. There is no way to pin a Java array, so this is a +ceiling on what any implementation here can offer โ€” not a gap to close later. + +The claim Android supports is therefore narrower and is written that way in +`SecureBytes`: **a derived key is not left sitting in the heap after use.** Not +"the key is gone". + ๐Ÿ”ด One divergence from iOS, and it is a bug the Swift version cannot have. iOS computes `30 * pow(2, steps)` in a `Double`, which saturates to infinity and still caps correctly. Kotlin shifting a `Long` does not: `30L shl 60` wraps diff --git a/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt b/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt index dc5b87b..2b075a5 100644 --- a/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt +++ b/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt @@ -133,12 +133,17 @@ object AccountKdf { context: String, length: Int = 32, params: Params = Params.PBKDF2_V1, - ): ByteArray = Hkdf.derive( - ikm = master(passphrase, salt, params), - salt = ByteArray(0), - info = context.toByteArray(Charsets.UTF_8), - length = length, - ) + ): ByteArray = SecureBytes.withSecret(master(passphrase, salt, params)) { ikm -> + // ios#39 โ€” the stretched master is an intermediate nobody needs after + // the expansion, and it is the most valuable thing in this file: every + // other secret is derived from it. Wiped however this leaves. + Hkdf.derive( + ikm = ikm, + salt = ByteArray(0), + info = context.toByteArray(Charsets.UTF_8), + length = length, + ) + } /** What gets sent to the account service. */ fun authSecret(passphrase: String, salt: ByteArray, params: Params = Params.PBKDF2_V1): ByteArray = @@ -179,14 +184,19 @@ object AccountKdf { salt: ByteArray, params: Params = Params.PBKDF2_V1, ): Pair { - val ikm = master(passphrase, salt, params) - fun expand(context: String) = Hkdf.derive( - ikm = ikm, - salt = ByteArray(0), - info = context.toByteArray(Charsets.UTF_8), - length = 32, - ) - return expand(Context.ACCOUNT_AUTH) to expand(Context.ACCOUNT_IDENTITY) + return SecureBytes.withSecret(master(passphrase, salt, params)) { ikm -> + fun expand(context: String) = Hkdf.derive( + ikm = ikm, + salt = ByteArray(0), + info = context.toByteArray(Charsets.UTF_8), + length = 32, + ) + // โš ๏ธ BOTH expansions happen inside the block. Returning the pair and + // wiping afterwards would be the same thing; expanding after the + // wipe would derive two keys from thirty-two zero bytes, which is + // the failure mode this shape makes impossible to write. + expand(Context.ACCOUNT_AUTH) to expand(Context.ACCOUNT_IDENTITY) + } } /** diff --git a/engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt b/engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt new file mode 100644 index 0000000..381e99d --- /dev/null +++ b/engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt @@ -0,0 +1,78 @@ +package io.flamenet.e2e + +import java.util.Arrays + +/** + * Zeroing key material once it has been used (ios#39). + * + * ๐Ÿ”ด **This narrows a window. It does not remove the material, and claiming + * otherwise would spend the whole value of the feature on a lie.** What it buys + * is that a derived key does not sit in the process for the lifetime of the app + * after the one moment it was needed โ€” which matters for a heap dump, a crash + * report, or a debugger attached later. It buys nothing against somebody + * reading memory *while* the secret is in use. + * + * ๐Ÿ”ด **The JVM guarantee is weaker than the iOS one, and the difference is not + * a detail.** iOS has `memset_s`, which is specified not to be optimised away, + * and iOS does not page application memory to disk. Neither holds here: + * + * - **A compacting garbage collector moves objects.** A `ByteArray` can be + * relocated between allocation and wiping, and the bytes at the old address + * are not cleared by the move. Zeroing reaches the array you are holding; it + * cannot reach a copy the runtime made on its own. There is no way to pin a + * Java array, so this is a ceiling on what any implementation here can offer. + * - **Android has compressed swap.** zram has been on by default for years, so + * a page holding key material can reach a backing store โ€” unlike iOS, where + * the platform does not page application memory out at all. + * - **There is no `memset_s`.** `Arrays.fill` on an array that escapes the + * method is not eliminated in practice, because the JIT cannot prove the + * write is dead. "Not in practice" is a weaker statement than the C standard + * makes, and it is the honest one. + * + * โš ๏ธ **A Kotlin `String` cannot be zeroed**, same as Swift and for the same + * reasons: immutable, freely copied, and possibly interned. A passphrase that + * arrived as a `String` is beyond reach. The right response is to turn it into + * bytes early, wipe those, and not pretend about the original โ€” which is why + * `AccountKdf.master` already calls `PBEKeySpec.clearPassword()` rather than + * trying to do anything about the `String` it was handed. + * + * So the claim this type supports, precisely: **a derived key is not left + * sitting in the heap after use.** Not "the key is gone". + */ +object SecureBytes { + + /** + * Zero this array in place. + * + * โš ๏ธ Reaches THIS array only. Anything that copied it beforehand keeps an + * untouched copy, and the runtime may have made copies nobody asked for. + */ + fun wipe(bytes: ByteArray?) { + if (bytes == null || bytes.isEmpty()) return + Arrays.fill(bytes, 0) + } + + /** + * Run [body] with key material and wipe it however [body] leaves. + * + * โš ๏ธ Wipes on the way out of a throw as well. A derivation that failed + * half-way is exactly the case where the material is most likely to be + * forgotten โ€” which is why this is `try/finally` rather than a wipe at the + * end of a happy path. + */ + inline fun withSecret(secret: ByteArray, body: (ByteArray) -> T): T = + try { + body(secret) + } finally { + wipe(secret) + } + + /** + * A copy of [source], for callers that genuinely need one. + * + * ๐Ÿ”ด The copy is ordinary memory and this object stops being responsible + * for it. Every use of this is a decision to leave a second copy behind, + * and it is spelled out rather than implicit for that reason. + */ + fun copyOf(source: ByteArray): ByteArray = source.copyOf() +} diff --git a/engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt b/engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt new file mode 100644 index 0000000..4c89295 --- /dev/null +++ b/engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt @@ -0,0 +1,140 @@ +package io.flamenet.e2e + +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Zeroing key material once it has been used (ios#39, Android half). + * + * ๐Ÿ”ด **The claim being tested is narrow, and the tests are shaped to keep it + * narrow.** What this buys is that a derived key is not left sitting in the + * heap after the one moment it was needed. It is not "the key is gone" โ€” a + * compacting collector can have moved the array, Android has compressed swap, + * and there is no `memset_s` on the JVM. `SecureBytes` says all of that in + * prose; these assert only what is actually true. + * + * ๐Ÿ”ด **The most valuable test here is that the derived keys did not change.** + * Wiping an intermediate one line too early derives every account secret from + * thirty-two zero bytes โ€” which produces a perfectly consistent, perfectly + * wrong identity, silently, on every device. That is a far worse failure than + * the leak this feature exists to narrow, so it is checked first. + */ +class SecureBytesTest { + + private val passphrase = "a-long-enough-passphrase" + private val salt = ByteArray(16) { it.toByte() } + + // ---- The failure that would matter most ----------------------------- + + /** + * ๐Ÿ”ด The pinned vectors already cover `authSecret` and `accountKeySeed`. + * This covers the shape wiping could break without moving a vector: that + * the two secrets from one stretch still match the two derived separately. + * If the wipe landed between the expansions, the second would be derived + * from zeroes and this diverges. + */ + @Test + fun `wiping the intermediate did not change what is derived`() { + val (auth, account) = AccountKdf.signInSecrets(passphrase, salt) + assertContentEquals(AccountKdf.authSecret(passphrase, salt), auth) + assertContentEquals(AccountKdf.accountKeySeed(passphrase, salt), account) + } + + /** + * โš ๏ธ And neither is zeroes, which is what "derived from a wiped buffer" + * would look like. A test that only compared the two paths would pass if + * BOTH were wrong in the same way. + */ + @Test + fun `no derived secret is a run of zeroes`() { + val (auth, account) = AccountKdf.signInSecrets(passphrase, salt) + assertFalse(auth.all { it == 0.toByte() }, "the auth secret is all zeroes") + assertFalse(account.all { it == 0.toByte() }, "the account seed is all zeroes") + assertFalse( + AccountKdf.identityKey(passphrase, salt).all { it == 0.toByte() }, + "the identity key is all zeroes", + ) + } + + /** Derivation stays deterministic across calls, wiping or not. */ + @Test + fun `derivation is still deterministic`() { + assertContentEquals( + AccountKdf.identityKey(passphrase, salt), + AccountKdf.identityKey(passphrase, salt), + ) + } + + /** Different contexts still separate, so the wipe did not collapse them. */ + @Test + fun `the contexts still produce different secrets`() { + val auth = AccountKdf.authSecret(passphrase, salt) + val identity = AccountKdf.identityKey(passphrase, salt) + val account = AccountKdf.accountKeySeed(passphrase, salt) + assertFalse(auth.contentEquals(identity)) + assertFalse(auth.contentEquals(account)) + assertFalse(identity.contentEquals(account)) + } + + // ---- The wiping itself ---------------------------------------------- + + @Test + fun `wipe zeroes the array it is given`() { + val secret = ByteArray(32) { 0x41 } + SecureBytes.wipe(secret) + assertTrue(secret.all { it == 0.toByte() }) + } + + @Test + fun `wiping nothing is not an error`() { + SecureBytes.wipe(null) + SecureBytes.wipe(ByteArray(0)) + } + + @Test + fun `withSecret wipes on the way out`() { + val secret = ByteArray(32) { 0x41 } + val seen = SecureBytes.withSecret(secret) { it.copyOf() } + assertTrue(seen.all { it == 0x41.toByte() }, "the body saw a wiped buffer") + assertTrue(secret.all { it == 0.toByte() }, "the buffer was not wiped") + } + + /** + * ๐Ÿ”ด **The case the `finally` exists for.** A derivation that failed + * half-way is exactly where the material gets forgotten, so the wipe has to + * survive a throw โ€” not sit at the end of a happy path. + */ + @Test + fun `withSecret wipes when the body throws`() { + val secret = ByteArray(32) { 0x41 } + assertFailsWith { + SecureBytes.withSecret(secret) { error("derivation failed half-way") } + } + assertTrue(secret.all { it == 0.toByte() }, "a throw left the material behind") + } + + /** + * โš ๏ธ **The honest limit, asserted rather than left to the prose.** A copy + * taken before the wipe is untouched by it. This is not a bug to fix โ€” a + * `ByteArray` cannot reach copies of itself โ€” and stating it as a passing + * test stops anybody reading `wipe` as stronger than it is. + */ + @Test + fun `a copy taken beforehand survives the wipe`() { + val secret = ByteArray(32) { 0x41 } + val copy = SecureBytes.copyOf(secret) + SecureBytes.wipe(secret) + assertTrue(secret.all { it == 0.toByte() }) + assertTrue(copy.all { it == 0x41.toByte() }, + "the copy was wiped too, which would mean these share a buffer") + } + + @Test + fun `withSecret returns what the body returned`() { + assertEquals(7, SecureBytes.withSecret(ByteArray(4)) { 7 }) + } +} -- 2.54.0 From ab8ff658b23078e8aae557dabbc320d5cfcea426 Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Fri, 4 Sep 2026 13:46:57 -0400 Subject: [PATCH 2/3] Pin the relay's identity, not just its TLS (ios#19, Android half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ๐Ÿ”ด This was not a missing feature on Android. It was a wrong conclusion. `Trust.verifyHeadSignature` already pinned `issuerPublicKey` the first time a signed log head arrived. When the key later changed, the head signature simply stopped verifying โ€” and the only conclusion available at that point was `KeyLogForkException`. So a relay key rotation somebody performed on purpose was reported to the user as the relay showing two different histories, with `forkDetected` set. Those need different responses. A fork means nothing the relay says can be trusted and the client should stop. A rotation means somebody ran a command, or somebody else is answering on that address, and only a person can tell which. Reporting the second as the first is alarming and unactionable โ€” and an alarm people cannot act on is one they learn to dismiss, which costs the real fork detection its credibility as well. `RelayIdentityPin` now decides what the published key MEANS before it is used, with the same four verdicts as iOS, and a change raises `RelayIdentityChangedException` โ€” deliberately NOT a `KeyLogForkException`, and without setting `forkDetected`, because the log may be perfectly consistent. โš ๏ธ A relay that publishes no key is a weaker deployment, not an attack, and an unparseable key is broken rather than hostile. Both read as "not published" rather than as a change: refusing to talk to an older self-hosted build would be a worse failure than the one this prevents. โš ๏ธ The warning names both possibilities and asserts neither, because nothing in the client can tell a rotation from an impostor. Two things found while writing the tests. `RelayClient.host` already existed, so the one I added was a duplicate declaration โ€” removed rather than renamed. And the compiler folded `changed !is KeyLogForkException` to a constant when the value was typed concretely, which is the compiler proving the separation statically and also an assertion testing nothing; it is now held as the sealed base, which is the check a caller's `when` actually performs. 9 unit tests. App builds, every JVM suite green. Co-Authored-By: Claude Opus 5 --- PARITY.md | 16 +- .../io/flamenet/client/RelayIdentityPin.kt | 112 ++++++++++++ .../main/kotlin/io/flamenet/client/Trust.kt | 40 ++++- .../flamenet/client/RelayIdentityPinTest.kt | 167 ++++++++++++++++++ 4 files changed, 326 insertions(+), 9 deletions(-) create mode 100644 client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt create mode 100644 client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt diff --git a/PARITY.md b/PARITY.md index ff3b21d..e6de01c 100644 --- a/PARITY.md +++ b/PARITY.md @@ -161,7 +161,7 @@ being the one that leaks. | Back off on repeated app-lock attempts | โœ… | โœ… | ios#41 โ€” 3 free attempts, then 30s doubling to a 5-minute ceiling | | Handle a wrong clock | โœ… | โœ… | ios#45 โ€” skew learned free from the `Date` header; expiry takes the LATER of device and relay | | Zero key material after use | โœ… | ๐ŸŸก | ios#39 โ€” done, but the JVM guarantee is **weaker than iOS**; see below | -| Pin the relay's identity | โœ… | โšช | ios#19 | +| Pin the relay's identity | โœ… | โœ… | ios#19 โ€” Android already pinned the key and drew the **wrong conclusion** from a change; see below | โš ๏ธ **The back-off is not a defence of the data.** It slows a person holding an unlocked phone. The vault and the ratchet keys are protected by the Android @@ -185,6 +185,20 @@ the UI thread, so the two fields are `@Volatile` and updated together under a lock. A torn read of "skew from now, timestamp from an hour ago" would age out an observation just made, or trust one that had not been. +๐Ÿ”ด **The relay pin was not a missing feature on Android โ€” it was a wrong +conclusion.** `Trust.verifyHeadSignature` already pinned `issuerPublicKey` the +first time a signed log head arrived. When the key later changed, the signature +stopped verifying and the only conclusion available at that point was +`KeyLogForkException` โ€” so a rotation somebody performed on purpose was reported +to the user as *the relay showing two different histories*. + +Those need different responses. A fork means nothing the relay says can be +trusted. A rotation means somebody ran a command, or somebody else is answering +on that address, and only a person can tell which. `RelayIdentityChangedException` +is deliberately **not** a `KeyLogForkException`, and `forkDetected` is not set โ€” +an alarm people cannot act on is one they learn to dismiss, which would cost the +real fork detection its credibility too. + ๐Ÿ”ด **Zeroing is where Android cannot match iOS, and the row says ๐ŸŸก for that reason rather than being rounded up to โœ….** iOS has `memset_s`, specified not to be optimised away, and iOS does not page application memory to disk. Neither diff --git a/client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt b/client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt new file mode 100644 index 0000000..38229af --- /dev/null +++ b/client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt @@ -0,0 +1,112 @@ +package io.flamenet.client + +/** + * Which relay this is, beyond which certificate it presented (ios#19). + * + * ๐Ÿ”ด **TLS answers "is this host who DNS says it is". It does not answer "is + * this the same relay I have been talking to".** A mis-issued certificate, a + * hijacked DNS record, or a hostile party who obtains the domain all produce a + * server with a perfectly valid certificate for the right name โ€” and every one + * of them is a different relay wearing the same address. + * + * The relay publishes an issuer public key in its descriptor. Pinning that on + * first contact turns "the certificate is valid" into "this is the same relay", + * which is a materially stronger statement and the one that matters. + * + * ๐Ÿ”ด **Android already pinned this key and drew the wrong conclusion from it.** + * `Trust.verifyHeadSignature` pins `issuerPublicKey` the first time a signed log + * head arrives, and if the key later changes the signature simply fails to + * verify โ€” so the client raises `KeyLogForkException`, sets `forkDetected`, and + * tells somebody the relay's key log does not add up. + * + * A rotated relay key is not a forked log. They need different responses: a + * fork means the relay is showing two different histories and nothing it says + * can be trusted; a rotation means somebody ran a command, or somebody else is + * answering on that address. Reporting the second as the first is both alarming + * and unactionable, and an alarm people cannot act on is one they learn to + * dismiss โ€” which costs the real fork detection its credibility too. + * + * โš ๏ธ **A relay that publishes no key is a weaker deployment, not an attack.** + * Somebody self-hosting an older build has nothing to pin, and refusing to talk + * to them would be a worse failure than the one this prevents. That case is + * distinguished from a key that CHANGED, which is not weakness. + * + * โš ๏ธ A change is surfaced, never silently accepted and never silently refused. + * The same shape as a contact's identity changing: stop, say so, and let a + * person decide โ€” because a legitimate rotation exists and an app that cannot + * survive one is an app that gets uninstalled. + */ +object RelayIdentityPin { + + /** What a descriptor's key means against what was pinned before. */ + sealed interface Verdict { + /** Nothing pinned and a key published: pin it. */ + data class FirstSight(val offered: ByteArray) : Verdict { + override fun equals(other: Any?) = + other is FirstSight && offered.contentEquals(other.offered) + override fun hashCode() = offered.contentHashCode() + } + + /** The same relay as before. */ + data object Unchanged : Verdict + + /** This deployment publishes no key. Nothing to check. */ + data object NotPublished : Verdict + + /** + * ๐Ÿ”ด A different key than the one pinned. Either a deliberate rotation + * or a different server answering on this address, and nothing here can + * tell those apart โ€” only a person can. + */ + data class Changed(val pinned: ByteArray, val offered: ByteArray) : Verdict { + override fun equals(other: Any?) = other is Changed && + pinned.contentEquals(other.pinned) && offered.contentEquals(other.offered) + override fun hashCode() = 31 * pinned.contentHashCode() + offered.contentHashCode() + } + } + + /** + * Compare what the relay just published against what is pinned. + * + * โš ๏ธ An empty or unparseable key counts as "not published" rather than as a + * change. A relay that answers with rubbish is broken, and treating broken + * as hostile would raise an alarm nobody can act on. + */ + fun check(offeredBase64: String?, pinned: ByteArray?): Verdict { + val offered = offeredBase64 + ?.takeIf { it.isNotEmpty() } + ?.let { runCatching { Wire.unb64(it) }.getOrNull() } + ?.takeIf { it.isNotEmpty() } + ?: return Verdict.NotPublished + if (pinned == null || pinned.isEmpty()) return Verdict.FirstSight(offered) + return if (offered.contentEquals(pinned)) Verdict.Unchanged + else Verdict.Changed(pinned = pinned, offered = offered) + } + + /** + * What to tell somebody when it changed. + * + * ๐Ÿ”ด Names the two possibilities and does not pretend to know which. An + * alarm that asserts an attack when a rotation is equally likely trains + * people to dismiss alarms. + */ + fun warning(host: String): String = + "The server at $host is presenting a different identity than the one " + + "this app recorded the first time it connected. That happens when the " + + "server's keys are rotated on purpose โ€” and it also happens when " + + "something else is answering in its place. Nothing here can tell those " + + "apart. If you were not expecting a change, stop and check with " + + "whoever runs it before signing in again." +} + +/** + * The relay is not the one this app pinned. + * + * ๐Ÿ”ด **Deliberately NOT a [KeyLogForkException].** A fork means the relay is + * showing two different histories and nothing it says can be trusted. This means + * the server's identity key is not the one recorded โ€” a rotation somebody + * performed, or somebody else's server. The remedies are different, so the + * exceptions are different. + */ +class RelayIdentityChangedException(val host: String) : + RelayException(RelayIdentityPin.warning(host)) diff --git a/client/src/main/kotlin/io/flamenet/client/Trust.kt b/client/src/main/kotlin/io/flamenet/client/Trust.kt index 3d48183..ba4d977 100644 --- a/client/src/main/kotlin/io/flamenet/client/Trust.kt +++ b/client/src/main/kotlin/io/flamenet/client/Trust.kt @@ -612,21 +612,45 @@ class Trust( private fun verifyHeadSignature(head: Head, signatureB64: String) { if (signatureB64.isEmpty()) return val signature = runCatching { Wire.unb64(signatureB64) }.getOrNull() ?: return - var issuer = vault.keyLogIssuerPin() - if (issuer == null) { - val published = runCatching { - Wire.unb64(relay.relayDescriptor().optString("issuerPublicKey")) - }.getOrNull() - if (published == null || published.isEmpty()) return - vault.saveKeyLogIssuerPin(published) - issuer = published + + // ios#19 โ€” decide what the relay's published identity MEANS before + // using it, rather than inferring it from a signature that failed. + // + // ๐Ÿ”ด This is the bug the pin previously hid. A rotated relay key made + // the signature stop verifying, and the only conclusion available at + // that point was "fork" โ€” so a command somebody ran on purpose was + // reported as the relay showing two different histories. Different + // situations, different remedies, and an alarm nobody can act on is one + // people learn to dismiss, which costs real fork detection too. + val offered = runCatching { + relay.relayDescriptor().optString("issuerPublicKey") + }.getOrNull() + val issuer = when (val verdict = RelayIdentityPin.check(offered, vault.keyLogIssuerPin())) { + is RelayIdentityPin.Verdict.NotPublished -> + // Nothing published and nothing pinned: a weaker deployment, + // not an attack. Nothing to check the signature against. + vault.keyLogIssuerPin() ?: return + is RelayIdentityPin.Verdict.FirstSight -> { + vault.saveKeyLogIssuerPin(verdict.offered) + verdict.offered + } + is RelayIdentityPin.Verdict.Unchanged -> vault.keyLogIssuerPin() ?: return + is RelayIdentityPin.Verdict.Changed -> + // โš ๏ธ NOT a fork, and `forkDetected` is deliberately not set. + // The log may be perfectly consistent; it is the server's + // identity that is not the one recorded. + throw RelayIdentityChangedException(relayHostForPin()) } + if (!KeyLog.verifyHead(head.size, head.root, signature, issuer)) { forkDetected = true throw KeyLogForkException("log head signature does not verify under the pinned key") } } + /** The host to name in an identity-change warning. */ + private fun relayHostForPin(): String = relay.host + private fun consistent(from: Int, to: Int, oldRoot: String, newRoot: String): Boolean { val c = relay.logConsistency(from.toLong(), to.toLong()) return KeyLog.verifyConsistency( diff --git a/client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt b/client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt new file mode 100644 index 0000000..173c583 --- /dev/null +++ b/client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt @@ -0,0 +1,167 @@ +package io.flamenet.client + +import java.util.Base64 +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertIs +import kotlin.test.assertTrue + +/** + * Pinning the relay's identity (ios#19, Android half). + * + * ๐Ÿ”ด **The finding this closes is not a missing feature โ€” it is a wrong + * conclusion.** Android already pinned `issuerPublicKey`, inside + * `Trust.verifyHeadSignature`. When the key later changed, the head signature + * stopped verifying and the only conclusion available at that point was + * `KeyLogForkException` โ€” so a rotation somebody performed on purpose was + * reported to the user as the relay showing two different histories. + * + * Those need different responses. A fork means nothing the relay says can be + * trusted. A rotation means somebody ran a command, or somebody else is + * answering on that address, and a person has to decide which. Reporting the + * second as the first is alarming and unactionable, and an alarm people cannot + * act on is one they learn to dismiss โ€” which costs the real fork detection its + * credibility too. + */ +class RelayIdentityPinTest { + + private fun key(byte: Int) = ByteArray(32) { byte.toByte() } + private fun b64(bytes: ByteArray): String = Base64.getEncoder().encodeToString(bytes) + + // ---- The four verdicts ---------------------------------------------- + + @Test + fun `a published key with nothing pinned is first sight`() { + val offered = key(1) + val verdict = RelayIdentityPin.check(b64(offered), pinned = null) + assertEquals(RelayIdentityPin.Verdict.FirstSight(offered), verdict) + } + + @Test + fun `the same key is unchanged`() { + assertEquals( + RelayIdentityPin.Verdict.Unchanged, + RelayIdentityPin.check(b64(key(1)), pinned = key(1)), + ) + } + + /** + * ๐Ÿ”ด The case the whole file exists for, and the one that used to be + * reported as a forked log. + */ + @Test + fun `a different key is a change, carrying both halves`() { + val verdict = RelayIdentityPin.check(b64(key(2)), pinned = key(1)) + val changed = assertIs(verdict) + assertTrue(changed.pinned.contentEquals(key(1))) + assertTrue(changed.offered.contentEquals(key(2))) + } + + /** + * โš ๏ธ **A relay that publishes no key is a weaker deployment, not an + * attack.** Somebody self-hosting an older build has nothing to pin, and + * refusing to talk to them would be a worse failure than the one this + * prevents. + */ + @Test + fun `a deployment that publishes nothing is not an attack`() { + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check(null, pinned = key(1)), + ) + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check("", pinned = key(1)), + ) + } + + /** + * โš ๏ธ **Broken is not hostile.** A relay answering with rubbish counts as + * "not published" rather than as a change โ€” treating a malformed field as + * an attack would raise an alarm nobody can act on, on a deployment that is + * simply misconfigured. + */ + @Test + fun `an unparseable key reads as not published, not as a change`() { + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check("!!! not base64 !!!", pinned = key(1)), + ) + // Valid base64 of nothing is still nothing. + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check(b64(ByteArray(0)), pinned = key(1)), + ) + } + + /** An empty pin is no pin: first sight, not a change against zero bytes. */ + @Test + fun `an empty pin is treated as no pin`() { + val offered = key(3) + assertEquals( + RelayIdentityPin.Verdict.FirstSight(offered), + RelayIdentityPin.check(b64(offered), pinned = ByteArray(0)), + ) + } + + // ---- What somebody is told ------------------------------------------ + + /** + * ๐Ÿ”ด The warning names BOTH possibilities and asserts neither. An alarm that + * claims an attack when a rotation is equally likely trains people to + * dismiss alarms โ€” and the next one may be real. + */ + @Test + fun `the warning does not pretend to know which it is`() { + val text = RelayIdentityPin.warning("relay.example.com") + assertTrue(text.contains("relay.example.com"), text) + assertTrue(text.contains("rotated on purpose"), text) + assertTrue(text.contains("something else is answering"), text) + assertTrue(text.contains("Nothing here can tell those apart"), text) + } + + /** + * ๐Ÿ”ด **The exception is not a fork exception, and that separation is the + * point of this change.** A caller catching `KeyLogForkException` to decide + * "the log is untrustworthy" must not be handed a rotation. + */ + @Test + fun `an identity change is not reported as a key-log fork`() { + // โš ๏ธ Held as the sealed base, deliberately. Typed as the concrete class + // the compiler folds `!is KeyLogForkException` to a constant and warns โ€” + // which is the compiler proving the point statically, and also means the + // assertion would test nothing. Through the base type it is the runtime + // check a caller's `when` actually performs. + val thrown: RelayException = RelayIdentityChangedException("relay.example.com") + assertTrue( + thrown !is KeyLogForkException, + "a rotated relay key is being reported as a forked log", + ) + val identity = assertIs(thrown) + assertEquals("relay.example.com", identity.host) + assertTrue(identity.message!!.contains("rotated on purpose")) + } + + // ---- Shape ----------------------------------------------------------- + + /** + * โš ๏ธ `ByteArray` has reference equality by default, so the verdicts carry + * hand-written `equals`. Without them every comparison in this suite would + * be false and the tests would have been written around it. + */ + @Test + fun `verdicts compare by content, not by reference`() { + assertEquals( + RelayIdentityPin.Verdict.FirstSight(key(1)), + RelayIdentityPin.Verdict.FirstSight(key(1)), + ) + assertEquals( + RelayIdentityPin.Verdict.Changed(key(1), key(2)), + RelayIdentityPin.Verdict.Changed(key(1), key(2)), + ) + assertTrue( + RelayIdentityPin.Verdict.Changed(key(1), key(2)) != + RelayIdentityPin.Verdict.Changed(key(1), key(3)), + ) + } +} -- 2.54.0 From e0420a00c6b62d3772ef0a1f19326cbf199076e7 Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Fri, 4 Sep 2026 13:53:13 -0400 Subject: [PATCH 3/3] =?UTF-8?q?Apply=20inbound=20edits=20and=20pins=20inst?= =?UTF-8?q?ead=20of=20filing=20them=20as=20messages=20(=C2=A76.15/#6.16/#6?= =?UTF-8?q?.19)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ๐Ÿ”ด This was an interop bug, not a missing feature. The engine has parsed `editOf`, `pins` and `fwd` for as long as they have existed, and it exposes `isRenderable` โ€” "does this belong in a thread" โ€” as the single answer. `Session`'s inbound loop never consulted it. So three things arrived from the iPhone app and were filed as ordinary bubbles. An edit carries its new wording in `text`, so it drew as a DUPLICATE beside the typo it was meant to replace โ€” a correction that made the thread worse than the mistake did. A pin carries only an id, so it drew as a blank bubble. And a forward was drawn as the sender's own words. Worst of the three is the one nobody had hit yet: a control envelope from a NEWER peer carries `ctl` and a kind this build has never heard of, and would have drawn a bubble full of JSON in somebody's conversation. That is precisely the failure `isControl` was written to prevent, defeated by nobody calling it โ€” and it would have shipped to every older Android install the first time a control kind was added on iOS. The guard is `isRenderable` rather than a list of kinds this build knows, because an allowlist is only correct until the other platform adds one. What is applied now: - an edit rewrites the message it names and stamps `edited`, so a bubble can say so โ€” silently replacing the text is worse than the typo, because the reader cannot tell a correction from their own misremembering; - a pin is stored per thread, sealed in the vault like the transcript, as an ID and never a copy: a copy goes stale against an edit and outlives a deletion; - `fwd` reaches the transcript so second-hand words are not drawn as the sender's own โ€” THAT it was forwarded, never from whom. ๐Ÿ”ด An inbound edit is applied only when it comes from the person whose message it is. Without that check any contact sharing a session could rewrite anybody's words in the reader's transcript โ€” a better attack than sending a message of their own, because the words would carry somebody else's name. โš ๏ธ Clearing a thread clears its pin. A banner pointing at a message that was just cleared reads as the clear not having worked. โš ๏ธ `fwd` is read as both a boolean and a number, for the same reason `isControl` accepts both: `org.json`'s `optBoolean` does not coerce a number, and that is how this class of bug shipped once already. 9 unit tests. App builds, every JVM suite green. The three PARITY rows stay ๐ŸŸก: the inbound half is done, originating each still needs the UI. Co-Authored-By: Claude Opus 5 --- PARITY.md | 32 +++- .../io/flamenet/messenger/app/Session.kt | 152 +++++++++++++++++- .../messenger/app/InboundControlTest.kt | 138 ++++++++++++++++ .../src/main/kotlin/io/flamenet/e2e/Vault.kt | 31 +++- 4 files changed, 344 insertions(+), 9 deletions(-) create mode 100644 app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt diff --git a/PARITY.md b/PARITY.md index e6de01c..970fdfa 100644 --- a/PARITY.md +++ b/PARITY.md @@ -94,15 +94,15 @@ From `E2E/E2EInbox.swift`, which is the real feature list. | History at rest | โœ… encrypted, keyed by person not device | | Session restore across restarts | โœ… credential and device both survive | | Reactions | โœ… transcript carries a `mid`; long-press gives a five-emoji row, one emoji per person | -| Edit a sent message (ยง6.15) | ๐ŸŸก codec back, and the `mid` it was blocked on now exists โ€” applying an inbound edit and offering the control are UI work | +| Edit a sent message (ยง6.15) | ๐ŸŸก inbound edits now APPLY to the message they name; offering the control is UI work | | Drafts that survive a relaunch | โœ… sealed in the vault, not SharedPreferences | -| Forwarding (ยง6.16) | ๐ŸŸก codec back; originating needs the UI | +| Forwarding (ยง6.16) | ๐ŸŸก `fwd` now reaches the transcript so second-hand words are not drawn as the sender's own; originating needs the UI | | Group message ids and quotes (ยง6.9) | โœ… transcript stores both; sent and queued copies share one id | | Mentions (ยง6.18) | โœ… codec, an `@` picker in the group composer, and ids recorded at pick time rather than re-derived from the text | | Per-message timers (ยง6.17) | โœ… combining rule and per-message pruning; composer control in progress | | Search inside messages | โœ… scanned, never indexed | | Search inside messages | โœ… scanned, never indexed โ€” no plaintext index at rest | -| Pinned message (ยง6.19) | ๐ŸŸก codec back; the banner and the control are UI work | +| Pinned message (ยง6.19) | ๐ŸŸก inbound pins now stored per thread, sealed in the vault; the banner and the control are UI work | | Photos | โœ… send via the system picker, render inline | | Voice notes (record + play) | โœ… AAC/MP4, tap to record, tap to send | | Stories, and seen-state | โœ… sent per-contact, expired bytes deleted | @@ -222,6 +222,32 @@ somebody is grinding attempts. The steps are clamped before the shift, and --- +## Inbound control envelopes + +๐Ÿ”ด **The app never asked the engine whether an envelope belonged in a thread.** +`Content.isRenderable` has existed since ยง6.15 and `Session`'s inbound loop did +not consult it, so three things arrived from the iPhone app and were filed as +ordinary bubbles: + +- an **edit** carries its new wording in `text`, so it drew as a duplicate + beside the typo it was meant to replace โ€” a correction that made the thread + worse; +- a **pin** carries only an id, so it drew as a blank bubble; +- and any **control envelope from a newer peer** would have drawn a bubble full + of JSON โ€” the failure `isControl` was written to prevent, defeated by nobody + calling it. + +All three now branch before filing, and the last is guarded by `isRenderable` +itself rather than by a list of kinds this build happens to know: an allowlist is +only correct until the other platform adds one. + +โš ๏ธ An inbound edit is applied only when it comes from the person whose message +it is. Without that check any contact sharing a session could rewrite anybody's +words in the reader's transcript โ€” a better attack than sending a message of +their own, because the words would carry somebody else's name. + +--- + ## Test safety | Feature | iOS | Android | Note | diff --git a/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt b/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt index e8925e3..3309158 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt @@ -116,6 +116,21 @@ class Session( * to know anything had been dropped. */ val about: Content.Reference? = null, + /** + * Second-hand words (ยง6.16). + * + * โš ๏ธ THAT it was forwarded, never from whom. + */ + val forwarded: Boolean = false, + /** + * When this message was last edited (ยง6.15), or null if never. + * + * ๐Ÿ”ด **Kept so the bubble can say "edited".** A correction that silently + * replaces the text is worse than the typo: the reader has no way to + * know the words changed after they read them, and no way to tell a + * correction from their own misremembering. + */ + val editedAt: Long? = null, /** * A timer this one message carries, in seconds (ยง6.17); 0 for none. * @@ -163,6 +178,12 @@ class Session( link = m.optJSONObject("lnk")?.let { Content.LinkPreview.fromJson(it) }, messageId = m.optString("mid").takeIf { it.isNotEmpty() }, about = m.optJSONObject("re")?.let { Content.Reference.fromJson(it) }, + // โš ๏ธ Both shapes, for the same reason `Content.isForwarded` + // accepts both: `org.json`'s `optBoolean` does not coerce a + // number, so reading only that would miss a `"fwd":1` written + // by any other engine. + forwarded = m.optBoolean("fwd", false) || m.optInt("fwd", 0) != 0, + editedAt = m.optLong("edited", 0L).takeIf { it > 0L }, lifetimeSeconds = m.optInt("secs", 0), ) } @@ -184,6 +205,15 @@ class Session( about: Content.Reference? = null, /** A timer this one message carries, in seconds (ยง6.17); 0 for none. */ lifetimeSeconds: Int = 0, + /** + * Whether the sender is passing on somebody else's words (ยง6.16). + * + * โš ๏ธ Says THAT it was forwarded and never from whom โ€” the point of a + * forward is that the original author does not travel with it. This + * exists so second-hand words are not shown as the sender's own, which + * is a different thing from attribution. + */ + forwarded: Boolean = false, ) { vault.append(threadId, JSONObject() .put("author", author) @@ -201,10 +231,10 @@ class Session( } .also { if (attachment != null) it.put("kind", attachment.kind).put("att", attachment.toJson()) } .also { if (link != null) it.put("lnk", link.toJson()) } - // โš ๏ธ Written only when present. A null on every message would grow - // the transcript for nothing and make "no id" indistinguishable - // from "an id of nothing". - .also { if (messageId != null) it.put("mid", messageId) } + // โš ๏ธ Written only when true, for the same reason `secs` is. A false + // on every message grows the transcript for a flag that is almost + // always absent. + .also { if (forwarded) it.put("fwd", true) } .also { if (about != null) it.put("re", about.toJson()) }) // ๐Ÿ”ด A reply arriving cancels the reminder, and it is done HERE because @@ -627,6 +657,70 @@ class Session( return hit } + /** + * Apply an inbound edit to the message it names (ยง6.15). + * + * ๐Ÿ”ด **An edit is never a bubble of its own.** It changes one that is + * already there, or it does nothing. Android had the codec and no + * application, so an edit from the iPhone app arrived carrying the new + * wording in `text` and was filed as an ORDINARY MESSAGE โ€” a correction + * showing up as a duplicate beside the typo it was meant to replace, which + * is worse than the typo. + * + * ๐Ÿ”ด **Only the author may edit their own message.** Without this check any + * contact sharing a session could rewrite anybody's words in the reader's + * transcript, which is a far better attack than sending a message of their + * own: the words would carry somebody else's name. + * + * Returns whether a message was found and changed. + */ + fun applyEdit( + threadId: String, + messageId: String, + author: Long, + newText: String, + at: Long = System.currentTimeMillis(), + ): Boolean { + val stored = vault.history(threadId) + var hit = false + for (i in 0 until stored.length()) { + val row = stored.getJSONObject(i) + if (row.optString("mid") != messageId) continue + // ๐Ÿ”ด The author check. `mine` is this device's own message; an + // inbound edit must come from the person whose message it is. + val wroteIt = if (row.optBoolean("mine")) author == userId + else row.optLong("author") == author + if (!wroteIt) break + row.put("text", newText).put("edited", at) + hit = true + break + } + if (hit) vault.replaceHistory(threadId, stored) + return hit + } + + /** + * The message pinned in a thread, or null (ยง6.19). + * + * โš ๏ธ An id, never a copy. A stored copy would go stale the moment the + * message was edited, and would survive the message being deleted โ€” a + * banner quoting words that are no longer in the conversation. + */ + fun pinnedMessage(threadId: String): String? = + vault.pinnedMessage(threadId) + + /** + * Pin a message, or take the pin down with an empty [messageId]. + * + * โš ๏ธ **The last pin wins and there is only ever one.** A thread with a list + * of pins is a second inbox; ยง6.19 is one banner, and two people pinning at + * once resolve by whoever's envelope arrived last โ€” the same rule the + * transcript already uses for everything else. + */ + fun applyPin(threadId: String, messageId: String) { + vault.savePinnedMessage(threadId, messageId.takeIf { it.isNotBlank() }) + } + /** * React to somebody's message, or take it back with an empty [emoji]. * @@ -924,6 +1018,37 @@ class Session( if (story.isActive(now)) incomingStories += story continue } + // ๐Ÿ”ด **An EDIT (ยง6.15) changes a message; it is never one.** + // Android had the codec and no application, so an edit from the + // iPhone app fell straight through this loop carrying its new + // wording in `text` and was filed as an ordinary bubble โ€” a + // correction arriving as a duplicate beside the typo it was meant + // to replace, which is worse than the typo. + message.content.editOf?.let { editing -> + val gid = if (message.fromAddress != null) null + else message.content.groupId + val thread = gid?.let(Vault::threadForGroup) + ?: message.fromAddress?.let(Vault::threadForAddress) + ?: Vault.threadForUser(message.fromUser) + if (applyEdit(thread, editing, message.fromUser, + message.content.text.orEmpty())) { + touched += thread + } + continue + } + // ๐Ÿ”ด **A PIN (ยง6.19) is an id, not a bubble.** Same story: without + // this it was filed as a message whose text was empty, so a pin + // from the other app drew a blank bubble in the conversation. + message.content.pins?.let { pinned -> + val gid = if (message.fromAddress != null) null + else message.content.groupId + val thread = gid?.let(Vault::threadForGroup) + ?: message.fromAddress?.let(Vault::threadForAddress) + ?: Vault.threadForUser(message.fromUser) + applyPin(thread, pinned) + touched += thread + continue + } // A reaction to a MESSAGE. Folded onto the message it is about, // never filed as one of its own โ€” a thread where every thumbs-up // is a bubble is a thread nobody can read. @@ -997,6 +1122,19 @@ class Session( // worse, match by coincidence. Groups do not federate yet, so a // remote sender's group claim is ignored and the message files in // their own thread. + // ๐Ÿ”ด **The guard that stops this list needing to be complete.** + // Every branch above names one kind of non-message. `isRenderable` + // is the engine's own answer to "does this belong in a thread", and + // it is false for reactions, call signals, edits, pins and โ€” the + // case that matters most โ€” ANY control envelope from a newer peer, + // which carries `ctl` and a kind this build has never heard of. + // + // Without it, a control kind added on iOS ships a bubble full of + // JSON into somebody's conversation on every Android install that + // predates it. The engine has had `isRenderable` since ยง6.15; the + // app simply never consulted it. + if (!message.content.isRenderable) continue + val gid = if (message.fromAddress != null) null else message.content.groupId if (gid != null) { val known = groupRoster(gid) @@ -1046,7 +1184,11 @@ class Session( // side. messageId = message.content.messageId, about = message.content.about, - lifetimeSeconds = message.content.lifetimeSeconds ?: 0) + lifetimeSeconds = message.content.lifetimeSeconds ?: 0, + // ยง6.16 โ€” so second-hand words are not drawn as the sender's + // own. The engine has read `fwd` off the wire since forwarding + // shipped; the transcript was where it stopped. + forwarded = message.content.isForwarded) touched += thread } diff --git a/app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt b/app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt new file mode 100644 index 0000000..5b0e628 --- /dev/null +++ b/app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt @@ -0,0 +1,138 @@ +package io.flamenet.messenger.app + +import io.flamenet.e2e.Content +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Edits, pins and unknown control envelopes must never become bubbles + * (ios#6.15/#6.16/#6.19, Android half). + * + * ๐Ÿ”ด **The bug this closes was an interop bug, not a missing feature.** The + * engine has parsed `editOf`, `pins` and `fwd` for as long as they have + * existed, and it exposes `isRenderable` โ€” "does this belong in a thread" โ€” as + * the single answer. `Session`'s inbound loop never consulted it. + * + * So an edit from the iPhone app arrived carrying its new wording in `text` and + * was filed as an ORDINARY MESSAGE: a correction drawn as a duplicate beside the + * typo it was meant to replace. A pin was filed as a bubble whose text was + * empty. And any control envelope from a newer peer โ€” which carries `ctl` and a + * kind this build has never heard of โ€” would have drawn a bubble full of JSON + * in somebody's conversation. + * + * โš ๏ธ These test the ENGINE's classification, which is what the app now branches + * on. The app-side application (`applyEdit`, `applyPin`) needs a `Vault` and a + * keystore, so it is covered on device in `FeatureUseTest`; what is checkable + * here is the decision that used to be missing. + */ +class InboundControlTest { + + private fun parse(json: String): Content = Content.parse(json)!! + + // ---- What must not be drawn ----------------------------------------- + + /** + * ๐Ÿ”ด The case that shipped a duplicate bubble. An edit carries the new + * wording, so it looks exactly like a message to anything that does not ask. + */ + @Test + fun `an edit is not renderable, even though it carries text`() { + val edit = parse(Content.edit("the corrected wording", of = "mid-1")) + assertTrue(edit.editOf == "mid-1", "the codec no longer reads editOf") + assertTrue(edit.text?.isNotEmpty() == true, "an edit carries its new text") + assertFalse(edit.isRenderable, "an edit would be filed as a message") + } + + /** A pin names an id and has nothing to show. */ + @Test + fun `a pin is not renderable`() { + val pin = parse(Content.pin("mid-1")) + assertTrue(pin.pins == "mid-1") + assertFalse(pin.isRenderable, "a pin would be filed as an empty bubble") + } + + /** Taking a pin down is the same envelope with an empty id. */ + @Test + fun `taking a pin down is also not renderable`() { + val unpin = parse(Content.pin("")) + assertTrue(unpin.pins == "", "an empty id is how a pin is removed") + assertFalse(unpin.isRenderable) + } + + /** + * ๐Ÿ”ด **The case an allowlist cannot cover, and the reason the app branches + * on `isRenderable` rather than on a list of kinds it knows.** A control + * envelope from a NEWER peer has a kind nothing here has heard of. Keying on + * known kinds ships a bubble full of JSON to every older install the first + * time a new control message is added on the other platform. + */ + @Test + fun `a control envelope from a newer peer is not renderable`() { + val fromTheFuture = parse( + """{"fnc":1,"kind":"something-invented-later","ctl":1,"text":"{}"}""" + ) + assertTrue(fromTheFuture.isControl, "the ctl flag was not honoured") + assertFalse(fromTheFuture.isRenderable, + "an unknown control kind would be drawn as a message") + } + + /** + * โš ๏ธ Both shapes of the flag. `org.json`'s `optBoolean` does not coerce a + * number, so an engine sending `"ctl":1` rather than `"ctl":true` would slip + * past a reader that checked only one โ€” which is how this class of bug got + * shipped once already. + */ + @Test + fun `the control flag is honoured as a number and as a boolean`() { + assertFalse(parse("""{"fnc":1,"kind":"x","ctl":1,"text":"{}"}""").isRenderable) + assertFalse(parse("""{"fnc":1,"kind":"x","ctl":true,"text":"{}"}""").isRenderable) + } + + // ---- What must still be drawn --------------------------------------- + + /** + * ๐Ÿ”ด The guard must not swallow ordinary messages. A `continue` in the wrong + * place is a conversation that silently stops receiving. + */ + @Test + fun `an ordinary message is still renderable`() { + val text = parse(Content.text("hello", messageId = "mid-1")) + assertTrue(text.isRenderable) + } + + @Test + fun `a reply is still renderable`() { + val reply = parse( + Content.text("answering", messageId = "mid-2", + about = Content.Reference("mid-1", 7L, "the question")), + ) + assertTrue(reply.isRenderable) + assertTrue(reply.about?.messageId == "mid-1") + } + + /** + * ๐Ÿ”ด **A forward IS a message.** It is second-hand words, not a control + * envelope โ€” the flag changes how it is drawn, not whether it is. + */ + @Test + fun `a forwarded message is renderable, and says so`() { + val forwarded = parse( + Content.text("passed along", messageId = "mid-3", forwarded = true), + ) + assertTrue(forwarded.isRenderable, "a forward was treated as control") + assertTrue(forwarded.isForwarded, "the fwd flag was lost") + } + + /** + * โš ๏ธ And the same both-shapes rule for `fwd`, for the same reason. An engine + * writing `1` where this one writes `true` would have its forwards drawn as + * the sender's own words. + */ + @Test + fun `the forwarded flag is honoured as a number and as a boolean`() { + assertTrue(parse("""{"fnc":1,"kind":"text","fwd":1,"text":"x"}""").isForwarded) + assertTrue(parse("""{"fnc":1,"kind":"text","fwd":true,"text":"x"}""").isForwarded) + assertFalse(parse("""{"fnc":1,"kind":"text","text":"x"}""").isForwarded) + } +} diff --git a/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt b/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt index 72f7caf..e61e5cc 100644 --- a/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt +++ b/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt @@ -1658,7 +1658,36 @@ class Vault(private val storage: Storage, private val key: SecretKey) { * make the next message from that person undecryptable โ€” a "clear chat" * that quietly breaks the chat. */ - fun forgetHistory(threadId: String) = storage.delete(historyName(threadId)) + fun forgetHistory(threadId: String) { + storage.delete(historyName(threadId)) + // โš ๏ธ The pin goes with the transcript. A banner pointing at a message + // that was just cleared is a reference to nothing, and it would survive + // "clear this conversation" โ€” which reads as the clear not having + // worked. + storage.delete(pinnedName(threadId)) + } + + // -- Pinned message (ยง6.19) ---------------------------------------------- + + private fun pinnedName(threadId: String) = "pinned-$threadId" + + /** + * The message pinned in a thread, or null. + * + * ๐Ÿ”ด **Encrypted at rest like the transcript, not a preference.** Which + * message somebody pinned in which conversation is exactly the shape of + * metadata the vault exists to keep off a stolen phone โ€” and it is stored + * as an ID, never a copy, so it cannot go stale against an edit or outlive + * a deletion. + */ + fun pinnedMessage(threadId: String): String? = + readJson(pinnedName(threadId))?.optString("mid")?.takeIf { it.isNotEmpty() } + + /** Pin a message, or clear the pin with null. */ + fun savePinnedMessage(threadId: String, messageId: String?) { + if (messageId.isNullOrEmpty()) storage.delete(pinnedName(threadId)) + else writeJson(pinnedName(threadId), JSONObject().put("mid", messageId)) + } /** * Delete one line from a thread's history (delete for me). -- 2.54.0