From 6ed7931b8bdac418716e4792ff89ddb793b25c77 Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Mon, 7 Sep 2026 17:10:52 -0400 Subject: [PATCH] Read the contact's address from the directory, and name the refusals The send path fetches full bundles only when a device needs a session established, so it does not drain one-time prekeys on every message. The contact's address rides with those bundles -- so every later send had no address, computed the `local:` key-log subject instead of `fnid:`, matched no leaf, and refused its own send. flamenet-relay!82 puts the address on the directory; this reads it. Two bugs the wiring exposed: * `rememberFnid` silently overwrote a DIFFERENT address, the one thing its own doc comment says it must never do. An fnid is derived from the account key, so a new address is a different account, not a new device. It went unnoticed while only the once-per-conversation bundle path fed it; the directory feeds it every send. It marks CHANGED now. * `guardIdentity` read the pinned address and passed it back into the comparison against itself, so "a different address is a different person" could never fire on a send. It compares the served one now. And the copy: only `IdentityChangedException` was named, so `KeyNotLoggedException` and `KeyLogForkException` reached the screen as "Something went wrong reaching that server." -- sending a person to check their signal for a decision this phone had made. Also: the call screen painted a flat `colorScheme.background` rectangle over the app's gradient, which read as a blue box around the call area. Co-Authored-By: Claude Opus 5 --- .../io/flamenet/messenger/app/CallScreen.kt | 8 ++++- .../messenger/app/MessengerViewModel.kt | 20 +++++++++++ .../io/flamenet/client/Conversations.kt | 31 ++++++++++++----- .../kotlin/io/flamenet/client/RelayClient.kt | 25 +++++++++++++- .../main/kotlin/io/flamenet/client/Trust.kt | 10 ++++++ .../kotlin/io/flamenet/client/TrustTest.kt | 33 +++++++++++++++++++ 6 files changed, 116 insertions(+), 11 deletions(-) diff --git a/app/src/main/kotlin/io/flamenet/messenger/app/CallScreen.kt b/app/src/main/kotlin/io/flamenet/messenger/app/CallScreen.kt index de55e3e..a983831 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/CallScreen.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/CallScreen.kt @@ -63,7 +63,13 @@ fun CallScreenBody( Column( modifier = Modifier .fillMaxSize() - .background(MaterialTheme.colorScheme.background) + // ⚠️ The app's ground, not `colorScheme.background`. That token is + // a flat blue, and painting it full-screen here stamped a solid + // rectangle over the gradient every other screen shows — which + // read as "a blue box around the call area". The brush is used + // rather than transparency so this composable still stands on its + // own in the screenshot harness. + .background(groundBrush()) .padding(horizontal = Kit.margin) .testTag("call-screen"), horizontalAlignment = Alignment.CenterHorizontally, 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 9456c32..ac1e4b5 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt @@ -5,6 +5,8 @@ import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import io.flamenet.client.ChainRpc import io.flamenet.client.IdentityChangedException +import io.flamenet.client.KeyLogForkException +import io.flamenet.client.KeyNotLoggedException import io.flamenet.client.DeviceClock import io.flamenet.client.AccountsClient import io.flamenet.client.RelayClient @@ -3890,6 +3892,24 @@ internal fun describeFailure(failure: Throwable): String = when (failure) { is IdentityChangedException -> "Their security code changed. Open the conversation to review it " + "before sending anything else." + // 🔴 **The other two refusals reached the generic network line.** + // Only `IdentityChangedException` was named, and the send path throws + // these just as often — a key the log does not account for is the + // failure a person actually hits after a contact reinstalls. "Something + // went wrong reaching that server" sent them to check their signal for a + // decision this phone made, and the server was answering fine. + is KeyNotLoggedException -> if (failure.hadEntries) { + "This contact's device is offering a key that doesn't match what " + + "the server has on record for it. Open the conversation to " + + "review their security code before sending anything else." + } else { + "This contact's device is offering a key the server never recorded. " + + "Nothing was sent. Check their security code with them directly." + } + is KeyLogForkException -> + "This server's record of published keys doesn't add up, so nothing " + + "was sent. That can mean tampering \u2014 check this contact's " + + "security code with them another way before continuing." is Vault.Corrupt -> "This phone's stored keys could not be read." // 🔴 **Anything not named above used to reach the screen verbatim.** diff --git a/client/src/main/kotlin/io/flamenet/client/Conversations.kt b/client/src/main/kotlin/io/flamenet/client/Conversations.kt index 7146c25..6b43c9e 100644 --- a/client/src/main/kotlin/io/flamenet/client/Conversations.kt +++ b/client/src/main/kotlin/io/flamenet/client/Conversations.kt @@ -612,14 +612,21 @@ class Conversations( val theirs = bundles.filter { it.deviceId != identity.deviceId } if (theirs.isEmpty()) return - // The pinned address is passed back in so a CHANGED address is caught - // here too, not only a changed key. + // ⚠️ **The address compared has to be the one just SERVED.** This + // read the pin and passed it straight back into the comparison, which + // compares it against itself — so the "a different address is a + // different person" rule could never fire on a send. It reads as + // working code and tests as working code; only a relay actually + // swapping an address would have shown it. val pinned = trust.pinnedFnid(toUserId) - if (trust.record(toUserId, theirs.map { it.identitySigning }, pinned) + if (trust.record(toUserId, theirs.map { it.identitySigning }, servedFnid ?: pinned) == IdentityTrust.CHANGED ) { throw IdentityChangedException(toUserId) } + // Only once the keys are accepted — an address learned alongside keys we + // are about to refuse is not one to pin. + servedFnid?.let { trust.rememberFnid(toUserId, it) } // The address just served wins over the pinned one: a device that // registered a moment ago is logged under whatever the account claims // NOW, and the pin may predate the claim. @@ -664,7 +671,8 @@ class Conversations( // "Too many attempts". The directory costs nothing; full bundles are // fetched only when some device has no session yet, which is the one // moment X3DH actually needs them. - val directory = relay.listDevices(toUserId) + val served = relay.listDirectory(toUserId) + val directory = served.devices val stubs = (0 until directory.length()).map { i -> val o = directory.getJSONObject(i) PeerBundle( @@ -700,13 +708,18 @@ class Conversations( val needsEstablish = stubs.any { it.deviceId != identity.deviceId && existingSession(it.deviceId) == null } - // ⚠️ The address comes back with the bundles and is REMEMBERED. The - // directory route serves no fnid, so a later send that needs no bundle - // would otherwise be unable to check the log again. - var servedFnid: String? = null + // 🔴 **The directory serves the address too, and it has to.** A key + // log leaf is written under `fnid:` once an account has claimed an + // address; a verifier that does not know the address computes + // `local:/`, matches no leaf, and refuses the send as an + // unlogged key — reported as "something went wrong reaching that + // server". Bundles are fetched only when a session needs establishing, + // so every later send saw the directory alone; falling back to the pin + // failed outright for a contact pinned before addresses existed. + var servedFnid: String? = served.fnid val bundles = if (!needsEstablish) stubs else { val keys = relay.fetchKeys(toUserId) - servedFnid = keys.fnid + servedFnid = keys.fnid ?: servedFnid keys.fnid?.let { trust.rememberFnid(toUserId, it) } val full = keys.bundles.associateBy { it.deviceId } // Union: every device the directory knows, carrying a full bundle diff --git a/client/src/main/kotlin/io/flamenet/client/RelayClient.kt b/client/src/main/kotlin/io/flamenet/client/RelayClient.kt index 9f63ec7..a77591f 100644 --- a/client/src/main/kotlin/io/flamenet/client/RelayClient.kt +++ b/client/src/main/kotlin/io/flamenet/client/RelayClient.kt @@ -201,7 +201,30 @@ class RelayClient( /** `GET /e2e/devices/{user}` — who a user is, costing no prekey. */ fun listDevices(userId: Long): JSONArray = - getJson("/e2e/devices/$userId").optJSONArray("devices") ?: JSONArray() + listDirectory(userId).devices + + /** + * The directory, with the account's address alongside it. + * + * ⚠️ The address is what makes the key log checkable: a leaf is written + * under `fnid:` once an account has claimed one, and a verifier that does + * not know it computes the wrong subject and refuses the send. The send + * path deliberately fetches full bundles only when a session needs + * establishing, so on every other message this is the only place the + * address can come from. + */ + fun listDirectory(userId: Long): Directory { + val o = getJson("/e2e/devices/$userId") + return Directory( + devices = o.optJSONArray("devices") ?: JSONArray(), + // Absent on a relay older than this field, which is a relay whose + // accounts have no addresses either — null is then correct, not a + // degraded answer. + fnid = o.optString("fnid").takeIf { it.isNotEmpty() }, + ) + } + + data class Directory(val devices: JSONArray, val fnid: String?) /** * `DELETE /e2e/devices/{id}` — remove one of the ACCOUNT'S OWN devices. diff --git a/client/src/main/kotlin/io/flamenet/client/Trust.kt b/client/src/main/kotlin/io/flamenet/client/Trust.kt index ba4d977..2e07367 100644 --- a/client/src/main/kotlin/io/flamenet/client/Trust.kt +++ b/client/src/main/kotlin/io/flamenet/client/Trust.kt @@ -259,6 +259,16 @@ class Trust( fun rememberFnid(userId: Long, fnid: String) { val existing = vault.loadTrust(userId) ?: return if (existing.fnid == fnid) return + // 🔴 **The paragraph above was documentation, not behaviour.** This + // overwrote a DIFFERENT address in place — the one thing it says it must + // never do — and it went unnoticed while only the bundle route carried + // an address, because that route is reached once per conversation. The + // directory carries it on every send, so a silent replacement here + // would erase the change the pin exists to notice. + if (existing.fnid != null) { + markChanged(userId, existing) + return + } vault.saveTrust(userId, existing.copy(fnid = fnid)) } diff --git a/client/src/test/kotlin/io/flamenet/client/TrustTest.kt b/client/src/test/kotlin/io/flamenet/client/TrustTest.kt index c060de4..cb88b6b 100644 --- a/client/src/test/kotlin/io/flamenet/client/TrustTest.kt +++ b/client/src/test/kotlin/io/flamenet/client/TrustTest.kt @@ -95,6 +95,39 @@ class TrustTest { assertEquals(listOf(Wire.b64(key(1))), vault.loadTrust(7)?.identities) } + /** + * 🔴 **The doc comment on `rememberFnid` said this and the code did the + * opposite.** It overwrote a different address in place — the one thing an + * address pin exists to prevent, since an fnid is derived from the account + * key, so a new address is not a new device but a different account. + * + * ⚠️ It went unnoticed while only the bundle route carried an address: + * that route is reached once per conversation, when a session is first + * established. The directory carries it on every send. + */ + @Test + fun `a different address is a change, not a quiet replacement`() { + val t = trust() + t.record(7, listOf(key(1)), fnid = "fnid:first") + t.rememberFnid(7, "fnid:second") + + assertEquals("fnid:first", vault.loadTrust(7)?.fnid, + "the pinned address was replaced instead of challenged") + assertEquals(IdentityTrust.CHANGED, t.state(7)) + } + + /** An address learned for the first time is simply pinned. */ + @Test + fun `an address is adopted when none was pinned`() { + val t = trust() + t.record(7, listOf(key(1))) + assertNull(vault.loadTrust(7)?.fnid) + + t.rememberFnid(7, "fnid:first") + assertEquals("fnid:first", vault.loadTrust(7)?.fnid) + assertEquals(IdentityTrust.PINNED, t.state(7)) + } + @Test fun `a change drops the verified tick`() { val t = trust()