From cda64d8c2e8e87ab940f2a294a96d0d8fdfbbffa Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Wed, 26 Aug 2026 07:09:08 -0400 Subject: [PATCH] A contact who reinstalled could never be messaged again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🔴 **The chat header said "keys unchanged since you met" while every send was refused with "identity keys changed".** Found by driving two real apps: the iOS side relaunched with -FNMSGResetState, which is a genuinely new device with new keys, and Android correctly refused to send to it. Correct — and then there was no way out. `Trust.state()` could return UNKNOWN, VERIFIED or PINNED and nothing else. CHANGED was computed inside `record()` while a send was in flight and thrown away, because `Vault.Trust` had nowhere to remember it. So: - the header kept reporting the OLD pinned keys as unchanged, - the "Accept new keys" button, which only renders for CHANGED, never appeared, - `acceptIdentityChange()` was unreachable from the UI, - and the contact stayed unmessageable for ever, with the app insisting nothing was wrong. That is precisely what a tester does on day one: reinstall, and discover their friend can no longer reach them. `Vault.Trust` now carries `changed`, `record()` persists the detection without adopting the offered keys and drops the verified tick (whatever was compared in person was compared against the old key), `state()` reports it, and accepting clears it. Keys that match again also clear it, so a transient wrong answer does not strand somebody behind a banner for ever. Verified live on the emulator: the banner appears with its full text, Accept new keys is reachable, and the header returns to "keys unchanged" afterwards. Four tests in TrustTest; three of them fail against the old `state()`. --- 🔴 **An expired session told the user their passphrase was wrong.** Hit immediately afterwards: a send failed with "That handle or passphrase isn't right", ten hours after a successful sign-in, because AccountsClient mapped EVERY 401 and 403 to BadCredentials. The natural reading is that the password stopped working or was changed by someone else, and the natural response is to change a password that was never wrong. Only `sessions` and `accounts` actually present a passphrase, so only those can report one as wrong. Everything else now raises SessionExpired, which the app states as what it is. Two tests cover both directions. Co-Authored-By: Claude Opus 5 --- app/src/main/AndroidManifest.xml | 2 + .../messenger/app/MessengerViewModel.kt | 3 + .../res/drawable/ic_launcher_foreground.xml | 27 +++++++++ .../res/drawable/ic_launcher_monochrome.xml | 19 ++++++ .../res/mipmap-anydpi-v26/ic_launcher.xml | 6 ++ .../mipmap-anydpi-v26/ic_launcher_round.xml | 6 ++ .../res/values/ic_launcher_background.xml | 5 ++ .../io/flamenet/client/AccountsClient.kt | 27 ++++++++- .../main/kotlin/io/flamenet/client/Trust.kt | 35 ++++++++++- .../client/AccountsClientStatusTest.kt | 54 +++++++++++++++++ .../kotlin/io/flamenet/client/TrustTest.kt | 58 +++++++++++++++++++ .../src/main/kotlin/io/flamenet/e2e/Vault.kt | 21 ++++++- 12 files changed, 257 insertions(+), 6 deletions(-) create mode 100644 app/src/main/res/drawable/ic_launcher_foreground.xml create mode 100644 app/src/main/res/drawable/ic_launcher_monochrome.xml create mode 100644 app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml create mode 100644 app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml create mode 100644 app/src/main/res/values/ic_launcher_background.xml create mode 100644 client/src/test/kotlin/io/flamenet/client/AccountsClientStatusTest.kt diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 3d0de58..3205b75 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -17,6 +17,8 @@ diff --git a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt index ed3fe0d..25278b8 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt @@ -1165,6 +1165,9 @@ class MessengerViewModel(app: Application) : AndroidViewModel(app) { */ private fun describe(failure: Throwable): String = when (failure) { is AccountsClient.Failure.BadCredentials -> "That handle or passphrase isn't right." + is AccountsClient.Failure.SessionExpired -> + "Your session expired. Sign out and back in to keep going." + is AccountsClient.Failure.HandleTaken -> "That handle is already taken." is AccountsClient.Failure.RateLimited -> "Too many attempts. Wait a few minutes." is AccountsClient.Failure.Transport -> failure.message.orEmpty() diff --git a/app/src/main/res/drawable/ic_launcher_foreground.xml b/app/src/main/res/drawable/ic_launcher_foreground.xml new file mode 100644 index 0000000..28fe24d --- /dev/null +++ b/app/src/main/res/drawable/ic_launcher_foreground.xml @@ -0,0 +1,27 @@ + + + + + + + + diff --git a/app/src/main/res/drawable/ic_launcher_monochrome.xml b/app/src/main/res/drawable/ic_launcher_monochrome.xml new file mode 100644 index 0000000..8570b52 --- /dev/null +++ b/app/src/main/res/drawable/ic_launcher_monochrome.xml @@ -0,0 +1,19 @@ + + + + + + + diff --git a/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml b/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml new file mode 100644 index 0000000..c78bee3 --- /dev/null +++ b/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml b/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml new file mode 100644 index 0000000..c78bee3 --- /dev/null +++ b/app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml @@ -0,0 +1,6 @@ + + + + + + diff --git a/app/src/main/res/values/ic_launcher_background.xml b/app/src/main/res/values/ic_launcher_background.xml new file mode 100644 index 0000000..4b9e5bc --- /dev/null +++ b/app/src/main/res/values/ic_launcher_background.xml @@ -0,0 +1,5 @@ + + + + #121A2E + diff --git a/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt b/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt index 3a0b81e..7af3234 100644 --- a/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt +++ b/client/src/main/kotlin/io/flamenet/client/AccountsClient.kt @@ -56,6 +56,19 @@ class AccountsClient( sealed class Failure(message: String) : Exception(message) { /** Wrong handle and wrong passphrase are the same answer, on purpose. */ object BadCredentials : Failure("that handle or passphrase isn't right") + + /** + * The session is no longer accepted — expired, or revoked elsewhere. + * + * 🔴 **Not the same as a wrong passphrase, and saying so matters.** + * Every 401 and 403 used to become [BadCredentials], so a session that + * simply aged out told the user "that handle or passphrase isn't + * right" — in the middle of sending a message, hours after they signed + * in successfully. The natural reading is that their password stopped + * working or was changed by someone else, and the natural response is + * to change a password that was never wrong. + */ + object SessionExpired : Failure("your session has expired — sign in again") object HandleTaken : Failure("that handle is already taken") object RateLimited : Failure("too many attempts — wait a few minutes") class Service(val status: Int) : Failure("the account service returned $status") @@ -114,10 +127,13 @@ class AccountsClient( .url(base.newBuilder().addPathSegments(path).build()) .post(body.toString().toRequestBody(JSON)) .header("Accept", "application/json") - .build() + .build(), + // Only these two actually present a passphrase, so only these two can + // meaningfully report one as wrong. + carriesCredentials = path == "sessions" || path == "accounts", ) - private fun send(request: Request): JSONObject { + private fun send(request: Request, carriesCredentials: Boolean = false): JSONObject { val response = try { http.newCall(request).execute() } catch (e: Exception) { @@ -131,7 +147,12 @@ class AccountsClient( } catch (_: Exception) { throw Failure.Transport("unreadable response") } - 401, 403 -> throw Failure.BadCredentials + // ⚠️ The SAME status means different things depending on what + // was sent. Rejecting a passphrase is BadCredentials; refusing + // a token the user cannot see is an expired session. + 401, 403 -> + throw if (carriesCredentials) Failure.BadCredentials + else Failure.SessionExpired 409 -> throw Failure.HandleTaken 429 -> throw Failure.RateLimited else -> throw Failure.Service(it.code) diff --git a/client/src/main/kotlin/io/flamenet/client/Trust.kt b/client/src/main/kotlin/io/flamenet/client/Trust.kt index 40e19c1..f90fbd0 100644 --- a/client/src/main/kotlin/io/flamenet/client/Trust.kt +++ b/client/src/main/kotlin/io/flamenet/client/Trust.kt @@ -128,6 +128,7 @@ class Trust( // The fnid is derived from the account key, so a new one means a new // account key — a different person, however the relay presents it. if (fnid != null && existing?.fnid != null && existing.fnid != fnid) { + markChanged(userId, existing) return IdentityTrust.CHANGED } @@ -144,7 +145,15 @@ class Trust( // pass, and that is precisely the attack: the victim's real devices // still work, so nothing looks wrong, while a device the attacker // controls receives a copy of everything. - if (!encoded.all { existing.identities.contains(it) }) return IdentityTrust.CHANGED + if (!encoded.all { existing.identities.contains(it) }) { + markChanged(userId, existing) + return IdentityTrust.CHANGED + } + // Back to matching: an earlier scare that resolved itself should not + // leave the contact stuck behind an Accept button for ever. + if (existing.changed) { + vault.saveTrust(userId, existing.copy(changed = false)) + } return if (existing.verified) IdentityTrust.VERIFIED else IdentityTrust.PINNED } @@ -197,6 +206,13 @@ class Trust( /** Trust as it stands, for UI that has not triggered a fetch. */ fun state(userId: Long): IdentityTrust { val t = vault.loadTrust(userId) ?: return IdentityTrust.UNKNOWN + // 🔴 CHANGED first, and it is persisted rather than recomputed. This + // function used to be unable to return it at all: CHANGED was worked + // out inside `record()` during a send and then dropped, so the header + // said "keys unchanged since you met" while every send was refused for + // exactly that reason — and the Accept button, which only shows for + // CHANGED, could never appear. + if (t.changed) return IdentityTrust.CHANGED if (t.identities.isEmpty()) return IdentityTrust.UNKNOWN return if (t.verified) IdentityTrust.VERIFIED else IdentityTrust.PINNED } @@ -239,10 +255,25 @@ class Trust( ?: vault.loadTrust(userId)?.fnid vault.saveTrust( userId, - Vault.Trust(offered.map(Wire::b64), verified = false, fnid = address), + // changed = false: this IS the acceptance. Leaving it set would + // re-raise the banner on the very keys the user just approved. + Vault.Trust(offered.map(Wire::b64), verified = false, fnid = address, + changed = false), ) } + /** + * Remember that something other than the pinned keys was offered. + * + * ⚠️ The pinned `identities` are left exactly as they were, and `verified` + * is dropped: whatever was checked in person was checked against the OLD + * key, so the tick cannot carry over to a key nobody has compared. + */ + private fun markChanged(userId: Long, existing: Vault.Trust) { + if (existing.changed && !existing.verified) return + vault.saveTrust(userId, existing.copy(changed = true, verified = false)) + } + /** Forget a person entirely — used when they are removed as a contact. */ fun forget(userId: Long) = vault.forgetTrust(userId) diff --git a/client/src/test/kotlin/io/flamenet/client/AccountsClientStatusTest.kt b/client/src/test/kotlin/io/flamenet/client/AccountsClientStatusTest.kt new file mode 100644 index 0000000..22c5702 --- /dev/null +++ b/client/src/test/kotlin/io/flamenet/client/AccountsClientStatusTest.kt @@ -0,0 +1,54 @@ +package io.flamenet.client + +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertFailsWith + +/** + * What a 401 from the account service is allowed to mean. + * + * 🔴 Guards a bug found by driving the app: every 401 and 403 became + * [AccountsClient.Failure.BadCredentials], so a session that simply aged out + * told the user "that handle or passphrase isn't right" — mid-send, hours + * after a successful sign-in. The obvious reading is that the password stopped + * working, and the obvious response is to change one that was never wrong. + */ +class AccountsClientStatusTest { + + private lateinit var server: MockWebServer + + @BeforeTest fun start() { server = MockWebServer().also { it.start() } } + @AfterTest fun stop() = server.shutdown() + + private fun client() = AccountsClient(server.url("/").toString()) + + private fun unauthorized() = MockResponse().setResponseCode(401) + .setHeader("Content-Type", "application/json") + .setBody("""{"error":true,"reason":"unauthorized"}""") + + @Test + fun `a rejected passphrase is reported as bad credentials`() { + // The params lookup succeeds; the session request is what gets refused. + server.enqueue(MockResponse().setResponseCode(200) + .setHeader("Content-Type", "application/json") + .setBody("""{"kdfSalt":"AAAAAAAAAAAAAAAAAAAAAA==","kdfIterations":1,"kdfID":1}""")) + server.enqueue(unauthorized()) + + assertFailsWith { + client().signIn("someone", "wrong-passphrase") + } + } + + @Test + fun `a refused token is an expired session, not a wrong passphrase`() { + server.enqueue(unauthorized()) + // Any authenticated call that does NOT present a passphrase. Telling + // this user their passphrase is wrong would be a lie. + assertFailsWith { + client().parameters("someone") + } + } +} diff --git a/client/src/test/kotlin/io/flamenet/client/TrustTest.kt b/client/src/test/kotlin/io/flamenet/client/TrustTest.kt index 63ced60..c55985f 100644 --- a/client/src/test/kotlin/io/flamenet/client/TrustTest.kt +++ b/client/src/test/kotlin/io/flamenet/client/TrustTest.kt @@ -52,6 +52,64 @@ class TrustTest { private fun key(seed: Byte) = ByteArray(32) { seed } + // -- A detected change has to SURVIVE ------------------------------------ + + /** + * 🔴 The bug these guard, found by driving two real apps: `record()` worked + * out CHANGED during a send and then threw it away, because nothing stored + * it. `state()` could only answer UNKNOWN, VERIFIED or PINNED — so the chat + * header went on saying "keys unchanged since you met" while every send was + * refused with "identity keys changed", and the "Accept new keys" button, + * which only renders for CHANGED, could never appear. A contact who + * reinstalled was unmessageable for ever with no way out. + */ + @Test + fun `a detected key change is still CHANGED when asked again later`() { + val t = trust() + t.record(7, listOf(key(1))) + assertEquals(IdentityTrust.PINNED, t.state(7)) + + assertEquals(IdentityTrust.CHANGED, t.record(7, listOf(key(2)))) + // The point: ASK AGAIN. This is what the UI does, and it used to be + // told everything was fine. + assertEquals(IdentityTrust.CHANGED, t.state(7), + "the change was detected and then forgotten") + } + + @Test + fun `the pinned keys are kept, not replaced, by a detected change`() { + val t = trust() + t.record(7, listOf(key(1))) + t.record(7, listOf(key(2))) + // Still the ORIGINAL key. Detecting a substitution must not adopt it. + assertEquals(listOf(Wire.b64(key(1))), vault.loadTrust(7)?.identities) + } + + @Test + fun `a change drops the verified tick`() { + val t = trust() + t.record(7, listOf(key(1))) + t.setVerified(7, true) + assertEquals(IdentityTrust.VERIFIED, t.state(7)) + + t.record(7, listOf(key(2))) + // Whatever was compared in person was compared against the OLD key. + assertEquals(IdentityTrust.CHANGED, t.state(7)) + assertFalse(vault.loadTrust(7)!!.verified) + } + + @Test + fun `keys that match again clear the banner`() { + val t = trust() + t.record(7, listOf(key(1))) + t.record(7, listOf(key(2))) + assertEquals(IdentityTrust.CHANGED, t.state(7)) + + // A transient wrong answer should not strand the contact for ever. + assertEquals(IdentityTrust.PINNED, t.record(7, listOf(key(1)))) + assertEquals(IdentityTrust.PINNED, t.state(7)) + } + // -- Pinning ------------------------------------------------------------- @Test diff --git a/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt b/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt index 0f8b463..75525bb 100644 --- a/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt +++ b/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt @@ -219,6 +219,23 @@ class Vault(private val storage: Storage, private val key: SecretKey) { * purpose, so the client cannot tell them apart either. */ val fnid: String? = null, + + /** + * A key change was detected and has NOT been accepted yet. + * + * 🔴 **Without this the detection is thrown away.** `record()` computes + * CHANGED while a send is in flight and returns it; nothing persisted + * it, so `state()` could only ever answer UNKNOWN, VERIFIED or PINNED. + * The chat header therefore went on saying "keys unchanged since you + * met" while every send failed with "identity keys changed", and the + * "Accept new keys" button — which only renders for CHANGED — could + * never appear. A contact who reinstalled became permanently + * unmessageable, with the UI insisting nothing was wrong. + * + * ⚠️ The pinned `identities` stay as they were. This flag records that + * something ELSE was offered, not that we believe it. + */ + val changed: Boolean = false, ) fun loadTrust(userId: Long): Trust? = readJson(trustName(userId))?.let { o -> @@ -227,13 +244,15 @@ class Vault(private val storage: Storage, private val key: SecretKey) { identities = (0 until a.length()).map { a.getString(it) }, verified = o.optBoolean("verified", false), fnid = o.optString("fnid").ifEmpty { null }, + changed = o.optBoolean("changed", false), ) } fun saveTrust(userId: Long, trust: Trust) = writeJson(trustName(userId), JSONObject() .put("identities", org.json.JSONArray(trust.identities)) .put("verified", trust.verified) - .put("fnid", trust.fnid ?: JSONObject.NULL)) + .put("fnid", trust.fnid ?: JSONObject.NULL) + .put("changed", trust.changed)) fun forgetTrust(userId: Long) = storage.delete(trustName(userId))