A contact who reinstalled could never be messaged again
🔴 **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 <noreply@anthropic.com>
This commit is contained in:
Paul Hitt
co-authored by
Claude Opus 5
Claude Opus 5
parent
3c66d3e9bd
commit
cda64d8c2e
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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<AccountsClient.Failure.BadCredentials> {
|
||||
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<AccountsClient.Failure.SessionExpired> {
|
||||
client().parameters("someone")
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user