From 93674d249cabfb274046e92ca938c7e786b83cad Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Sat, 29 Aug 2026 17:37:28 -0400 Subject: [PATCH] Blocking someone showed an error, and could be undone by their next message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by finally running the banner I shipped in 0.3 instead of trusting that it compiled. `blocked()` read `user_id` from rows the relay emits as `{id, name, slug}` — the buddy-list shape, not the `user_id` shape the §8 routes use. It threw `No value for user_id`. Tapping Block worked on the server and then showed the user an error, which on a safety action is the worst outcome available: it succeeded and the screen said otherwise. The reason it survived is the reason it was dangerous. The block list is empty until somebody blocks their first person, so the map never ran and every test passed. The first block anybody performed broke it. 🔴 And it did not stop at a message. `adoptStrangers` read the block list with `.getOrDefault(emptyList())`, so the throw became "nobody is blocked" — and the next message from someone you had just blocked auto-added them back to your contacts, defeating the exact property the code comment above it claims to hold. That now fails closed: if the block list cannot be established, nothing is adopted and the next poll tries again. A stranger appearing a few seconds late is cosmetic; re-adding somebody who blocked their way out of a conversation is not. The new test uses a NON-EMPTY list, because that is the case that was missing — an empty-list assertion looks like coverage and exercises no parsing. Confirmed by reintroducing the bug: the populated case fails, the empty case still passes. Co-Authored-By: Claude Opus 4.8 --- app/build.gradle.kts | 4 +- .../messenger/app/MessengerViewModel.kt | 9 ++- .../kotlin/io/flamenet/client/MessengerApi.kt | 16 ++++- .../io/flamenet/client/BlockListShapeTest.kt | 68 +++++++++++++++++++ 4 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 client/src/test/kotlin/io/flamenet/client/BlockListShapeTest.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 8525e6e..71ed5cf 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -33,8 +33,8 @@ android { // ⚠️ Android also refuses to install an APK whose `versionCode` is // lower than the installed one, so a code that goes backwards makes a // release uninstallable for everybody already on the higher number. - versionCode = 3 - versionName = "0.3" + versionCode = 4 + versionName = "0.4" testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" } 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 5817c8c..25a3e47 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/MessengerViewModel.kt @@ -1279,7 +1279,14 @@ class MessengerViewModel(app: Application) : AndroidViewModel(app) { adopting += strangers viewModelScope.launch(Dispatchers.IO) { var added = false - val blocked = runCatching { current.api.blocked() }.getOrDefault(emptyList()) + // 🔴 **Fail closed.** This used to default to "nobody is blocked" + // when the call failed, which is the wrong direction for a safety + // check: a parse bug in the block list silently became "adopt the + // person you blocked". If we cannot establish who is blocked, we + // adopt nobody and try again on the next poll — a stranger showing + // up a few seconds late is a cosmetic problem; re-adding somebody + // who blocked their way out of a conversation is not. + val blocked = runCatching { current.api.blocked() }.getOrNull() ?: return@launch for (id in strangers) { runCatching { if (id in blocked) return@runCatching diff --git a/client/src/main/kotlin/io/flamenet/client/MessengerApi.kt b/client/src/main/kotlin/io/flamenet/client/MessengerApi.kt index 8335d9f..934f2be 100644 --- a/client/src/main/kotlin/io/flamenet/client/MessengerApi.kt +++ b/client/src/main/kotlin/io/flamenet/client/MessengerApi.kt @@ -184,9 +184,23 @@ class MessengerApi( fun markAllNotificationsRead() = post("notifications/read_all", JSONObject()).let { } + /** + * The ids this account has blocked. + * + * 🔴 **`id`, not `user_id`.** This route answers `{id, name, slug}` — the + * same shape as the buddy list and NOT the `user_id` shape the §8 routes + * use. Reading `user_id` here threw `No value for user_id`, and the reason + * it survived so long is the reason it was dangerous: the list is empty + * until somebody blocks their first person, so the map never ran and every + * test passed. The first block anybody performed broke it. + * + * ⚠️ It threw where a caller was defaulting to "nobody is blocked", which + * turned a field-name typo into a blocked person being auto-added back to + * your contacts. See `adoptStrangers`. + */ fun blocked(): List { val list = get("blocks").optJSONArray("blocked") ?: JSONArray() - return (0 until list.length()).map { list.getJSONObject(it).getLong("user_id") } + return (0 until list.length()).map { list.getJSONObject(it).getLong("id") } } fun setBlocked(userId: Long, blocked: Boolean) { diff --git a/client/src/test/kotlin/io/flamenet/client/BlockListShapeTest.kt b/client/src/test/kotlin/io/flamenet/client/BlockListShapeTest.kt new file mode 100644 index 0000000..1f288d7 --- /dev/null +++ b/client/src/test/kotlin/io/flamenet/client/BlockListShapeTest.kt @@ -0,0 +1,68 @@ +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.assertEquals +import kotlin.test.assertTrue + +/** + * The block list's wire shape, parsed from a NON-EMPTY list. + * + * 🔴 **This is the test that was missing, and its absence had teeth.** The + * client read `user_id` from rows the relay emits as `{id, name, slug}` — the + * buddy-list shape, not the `user_id` shape the §8 routes use. It threw + * `No value for user_id`, and it survived because the only thing ever asserted + * was an EMPTY block list: with no rows, the map never runs and the parser is + * never exercised. Everything passed until the first person blocked somebody. + * + * ⚠️ So the case that matters is a list with something in it. An empty-list + * assertion here would be worse than none, because it looks like coverage. + */ +class BlockListShapeTest { + + private lateinit var server: MockWebServer + + @BeforeTest fun start() { server = MockWebServer().also { it.start() } } + @AfterTest fun stop() { server.shutdown() } + + private fun api() = MessengerApi( + server.url("/").toString().trimEnd('/'), "someone", "app-password") + + private fun serve(body: String) { + server.enqueue(MockResponse().setResponseCode(200).setBody(body)) + } + + @Test + fun `a populated block list parses, using id rather than user_id`() { + // Exactly what the relay emits — see BlockListResponse in Wire.swift + // and the conformance assertion "the block list carries id, name and + // slug". + serve("""{"blocked":[ + {"id": 42, "name": "Spam Lord", "slug": "spamlord"}, + {"id": 77, "name": "Someone Else", "slug": "someone-else"} + ]}""") + assertEquals(listOf(42L, 77L), api().blocked()) + } + + @Test + fun `an empty block list is empty rather than an error`() { + serve("""{"blocked":[]}""") + assertTrue(api().blocked().isEmpty()) + } + + /** + * ⚠️ A row shaped like the §8 routes must NOT quietly parse. If the relay + * ever changed shape, silently reading a different field would hand back + * an under-populated block list — and an under-populated block list is a + * blocked person becoming visible again. + */ + @Test + fun `a row without id is a failure, not a silent gap in the list`() { + serve("""{"blocked":[{"user_id": 42, "name": "Spam Lord"}]}""") + val outcome = runCatching { api().blocked() } + assertTrue(outcome.isFailure, "a row with no `id` must not parse to something") + } +}