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") + } +}