From e0420a00c6b62d3772ef0a1f19326cbf199076e7 Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Fri, 4 Sep 2026 13:53:13 -0400 Subject: [PATCH] =?UTF-8?q?Apply=20inbound=20edits=20and=20pins=20instead?= =?UTF-8?q?=20of=20filing=20them=20as=20messages=20(=C2=A76.15/#6.16/#6.19?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ๐Ÿ”ด This was an interop bug, not a missing feature. The engine has parsed `editOf`, `pins` and `fwd` for as long as they have existed, and it exposes `isRenderable` โ€” "does this belong in a thread" โ€” as the single answer. `Session`'s inbound loop never consulted it. So three things arrived from the iPhone app and were filed as ordinary bubbles. An edit carries its new wording in `text`, so it drew as a DUPLICATE beside the typo it was meant to replace โ€” a correction that made the thread worse than the mistake did. A pin carries only an id, so it drew as a blank bubble. And a forward was drawn as the sender's own words. Worst of the three is the one nobody had hit yet: a control envelope from a NEWER peer carries `ctl` and a kind this build has never heard of, and would have drawn a bubble full of JSON in somebody's conversation. That is precisely the failure `isControl` was written to prevent, defeated by nobody calling it โ€” and it would have shipped to every older Android install the first time a control kind was added on iOS. The guard is `isRenderable` rather than a list of kinds this build knows, because an allowlist is only correct until the other platform adds one. What is applied now: - an edit rewrites the message it names and stamps `edited`, so a bubble can say so โ€” silently replacing the text is worse than the typo, because the reader cannot tell a correction from their own misremembering; - a pin is stored per thread, sealed in the vault like the transcript, as an ID and never a copy: a copy goes stale against an edit and outlives a deletion; - `fwd` reaches the transcript so second-hand words are not drawn as the sender's own โ€” THAT it was forwarded, never from whom. ๐Ÿ”ด An inbound edit is applied only when it comes from the person whose message it is. Without that check any contact sharing a session could rewrite anybody's words in the reader's transcript โ€” a better attack than sending a message of their own, because the words would carry somebody else's name. โš ๏ธ Clearing a thread clears its pin. A banner pointing at a message that was just cleared reads as the clear not having worked. โš ๏ธ `fwd` is read as both a boolean and a number, for the same reason `isControl` accepts both: `org.json`'s `optBoolean` does not coerce a number, and that is how this class of bug shipped once already. 9 unit tests. App builds, every JVM suite green. The three PARITY rows stay ๐ŸŸก: the inbound half is done, originating each still needs the UI. Co-Authored-By: Claude Opus 5 --- PARITY.md | 32 +++- .../io/flamenet/messenger/app/Session.kt | 152 +++++++++++++++++- .../messenger/app/InboundControlTest.kt | 138 ++++++++++++++++ .../src/main/kotlin/io/flamenet/e2e/Vault.kt | 31 +++- 4 files changed, 344 insertions(+), 9 deletions(-) create mode 100644 app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt diff --git a/PARITY.md b/PARITY.md index e6de01c..970fdfa 100644 --- a/PARITY.md +++ b/PARITY.md @@ -94,15 +94,15 @@ From `E2E/E2EInbox.swift`, which is the real feature list. | History at rest | โœ… encrypted, keyed by person not device | | Session restore across restarts | โœ… credential and device both survive | | Reactions | โœ… transcript carries a `mid`; long-press gives a five-emoji row, one emoji per person | -| Edit a sent message (ยง6.15) | ๐ŸŸก codec back, and the `mid` it was blocked on now exists โ€” applying an inbound edit and offering the control are UI work | +| Edit a sent message (ยง6.15) | ๐ŸŸก inbound edits now APPLY to the message they name; offering the control is UI work | | Drafts that survive a relaunch | โœ… sealed in the vault, not SharedPreferences | -| Forwarding (ยง6.16) | ๐ŸŸก codec back; originating needs the UI | +| Forwarding (ยง6.16) | ๐ŸŸก `fwd` now reaches the transcript so second-hand words are not drawn as the sender's own; originating needs the UI | | Group message ids and quotes (ยง6.9) | โœ… transcript stores both; sent and queued copies share one id | | Mentions (ยง6.18) | โœ… codec, an `@` picker in the group composer, and ids recorded at pick time rather than re-derived from the text | | Per-message timers (ยง6.17) | โœ… combining rule and per-message pruning; composer control in progress | | Search inside messages | โœ… scanned, never indexed | | Search inside messages | โœ… scanned, never indexed โ€” no plaintext index at rest | -| Pinned message (ยง6.19) | ๐ŸŸก codec back; the banner and the control are UI work | +| Pinned message (ยง6.19) | ๐ŸŸก inbound pins now stored per thread, sealed in the vault; the banner and the control are UI work | | Photos | โœ… send via the system picker, render inline | | Voice notes (record + play) | โœ… AAC/MP4, tap to record, tap to send | | Stories, and seen-state | โœ… sent per-contact, expired bytes deleted | @@ -222,6 +222,32 @@ somebody is grinding attempts. The steps are clamped before the shift, and --- +## Inbound control envelopes + +๐Ÿ”ด **The app never asked the engine whether an envelope belonged in a thread.** +`Content.isRenderable` has existed since ยง6.15 and `Session`'s inbound loop did +not consult it, so three things arrived from the iPhone app and were filed as +ordinary bubbles: + +- an **edit** carries its new wording in `text`, so it drew as a duplicate + beside the typo it was meant to replace โ€” a correction that made the thread + worse; +- a **pin** carries only an id, so it drew as a blank bubble; +- and any **control envelope from a newer peer** would have drawn a bubble full + of JSON โ€” the failure `isControl` was written to prevent, defeated by nobody + calling it. + +All three now branch before filing, and the last is guarded by `isRenderable` +itself rather than by a list of kinds this build happens to know: an allowlist is +only correct until the other platform adds one. + +โš ๏ธ An inbound edit is applied only when it comes from the person whose message +it is. Without that check any contact sharing a session could rewrite anybody's +words in the reader's transcript โ€” a better attack than sending a message of +their own, because the words would carry somebody else's name. + +--- + ## Test safety | Feature | iOS | Android | Note | diff --git a/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt b/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt index e8925e3..3309158 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/Session.kt @@ -116,6 +116,21 @@ class Session( * to know anything had been dropped. */ val about: Content.Reference? = null, + /** + * Second-hand words (ยง6.16). + * + * โš ๏ธ THAT it was forwarded, never from whom. + */ + val forwarded: Boolean = false, + /** + * When this message was last edited (ยง6.15), or null if never. + * + * ๐Ÿ”ด **Kept so the bubble can say "edited".** A correction that silently + * replaces the text is worse than the typo: the reader has no way to + * know the words changed after they read them, and no way to tell a + * correction from their own misremembering. + */ + val editedAt: Long? = null, /** * A timer this one message carries, in seconds (ยง6.17); 0 for none. * @@ -163,6 +178,12 @@ class Session( link = m.optJSONObject("lnk")?.let { Content.LinkPreview.fromJson(it) }, messageId = m.optString("mid").takeIf { it.isNotEmpty() }, about = m.optJSONObject("re")?.let { Content.Reference.fromJson(it) }, + // โš ๏ธ Both shapes, for the same reason `Content.isForwarded` + // accepts both: `org.json`'s `optBoolean` does not coerce a + // number, so reading only that would miss a `"fwd":1` written + // by any other engine. + forwarded = m.optBoolean("fwd", false) || m.optInt("fwd", 0) != 0, + editedAt = m.optLong("edited", 0L).takeIf { it > 0L }, lifetimeSeconds = m.optInt("secs", 0), ) } @@ -184,6 +205,15 @@ class Session( about: Content.Reference? = null, /** A timer this one message carries, in seconds (ยง6.17); 0 for none. */ lifetimeSeconds: Int = 0, + /** + * Whether the sender is passing on somebody else's words (ยง6.16). + * + * โš ๏ธ Says THAT it was forwarded and never from whom โ€” the point of a + * forward is that the original author does not travel with it. This + * exists so second-hand words are not shown as the sender's own, which + * is a different thing from attribution. + */ + forwarded: Boolean = false, ) { vault.append(threadId, JSONObject() .put("author", author) @@ -201,10 +231,10 @@ class Session( } .also { if (attachment != null) it.put("kind", attachment.kind).put("att", attachment.toJson()) } .also { if (link != null) it.put("lnk", link.toJson()) } - // โš ๏ธ Written only when present. A null on every message would grow - // the transcript for nothing and make "no id" indistinguishable - // from "an id of nothing". - .also { if (messageId != null) it.put("mid", messageId) } + // โš ๏ธ Written only when true, for the same reason `secs` is. A false + // on every message grows the transcript for a flag that is almost + // always absent. + .also { if (forwarded) it.put("fwd", true) } .also { if (about != null) it.put("re", about.toJson()) }) // ๐Ÿ”ด A reply arriving cancels the reminder, and it is done HERE because @@ -627,6 +657,70 @@ class Session( return hit } + /** + * Apply an inbound edit to the message it names (ยง6.15). + * + * ๐Ÿ”ด **An edit is never a bubble of its own.** It changes one that is + * already there, or it does nothing. Android had the codec and no + * application, so an edit from the iPhone app arrived carrying the new + * wording in `text` and was filed as an ORDINARY MESSAGE โ€” a correction + * showing up as a duplicate beside the typo it was meant to replace, which + * is worse than the typo. + * + * ๐Ÿ”ด **Only the author may edit their own message.** Without this check any + * contact sharing a session could rewrite anybody's words in the reader's + * transcript, which is a far better attack than sending a message of their + * own: the words would carry somebody else's name. + * + * Returns whether a message was found and changed. + */ + fun applyEdit( + threadId: String, + messageId: String, + author: Long, + newText: String, + at: Long = System.currentTimeMillis(), + ): Boolean { + val stored = vault.history(threadId) + var hit = false + for (i in 0 until stored.length()) { + val row = stored.getJSONObject(i) + if (row.optString("mid") != messageId) continue + // ๐Ÿ”ด The author check. `mine` is this device's own message; an + // inbound edit must come from the person whose message it is. + val wroteIt = if (row.optBoolean("mine")) author == userId + else row.optLong("author") == author + if (!wroteIt) break + row.put("text", newText).put("edited", at) + hit = true + break + } + if (hit) vault.replaceHistory(threadId, stored) + return hit + } + + /** + * The message pinned in a thread, or null (ยง6.19). + * + * โš ๏ธ An id, never a copy. A stored copy would go stale the moment the + * message was edited, and would survive the message being deleted โ€” a + * banner quoting words that are no longer in the conversation. + */ + fun pinnedMessage(threadId: String): String? = + vault.pinnedMessage(threadId) + + /** + * Pin a message, or take the pin down with an empty [messageId]. + * + * โš ๏ธ **The last pin wins and there is only ever one.** A thread with a list + * of pins is a second inbox; ยง6.19 is one banner, and two people pinning at + * once resolve by whoever's envelope arrived last โ€” the same rule the + * transcript already uses for everything else. + */ + fun applyPin(threadId: String, messageId: String) { + vault.savePinnedMessage(threadId, messageId.takeIf { it.isNotBlank() }) + } + /** * React to somebody's message, or take it back with an empty [emoji]. * @@ -924,6 +1018,37 @@ class Session( if (story.isActive(now)) incomingStories += story continue } + // ๐Ÿ”ด **An EDIT (ยง6.15) changes a message; it is never one.** + // Android had the codec and no application, so an edit from the + // iPhone app fell straight through this loop carrying its new + // wording in `text` and was filed as an ordinary bubble โ€” a + // correction arriving as a duplicate beside the typo it was meant + // to replace, which is worse than the typo. + message.content.editOf?.let { editing -> + val gid = if (message.fromAddress != null) null + else message.content.groupId + val thread = gid?.let(Vault::threadForGroup) + ?: message.fromAddress?.let(Vault::threadForAddress) + ?: Vault.threadForUser(message.fromUser) + if (applyEdit(thread, editing, message.fromUser, + message.content.text.orEmpty())) { + touched += thread + } + continue + } + // ๐Ÿ”ด **A PIN (ยง6.19) is an id, not a bubble.** Same story: without + // this it was filed as a message whose text was empty, so a pin + // from the other app drew a blank bubble in the conversation. + message.content.pins?.let { pinned -> + val gid = if (message.fromAddress != null) null + else message.content.groupId + val thread = gid?.let(Vault::threadForGroup) + ?: message.fromAddress?.let(Vault::threadForAddress) + ?: Vault.threadForUser(message.fromUser) + applyPin(thread, pinned) + touched += thread + continue + } // A reaction to a MESSAGE. Folded onto the message it is about, // never filed as one of its own โ€” a thread where every thumbs-up // is a bubble is a thread nobody can read. @@ -997,6 +1122,19 @@ class Session( // worse, match by coincidence. Groups do not federate yet, so a // remote sender's group claim is ignored and the message files in // their own thread. + // ๐Ÿ”ด **The guard that stops this list needing to be complete.** + // Every branch above names one kind of non-message. `isRenderable` + // is the engine's own answer to "does this belong in a thread", and + // it is false for reactions, call signals, edits, pins and โ€” the + // case that matters most โ€” ANY control envelope from a newer peer, + // which carries `ctl` and a kind this build has never heard of. + // + // Without it, a control kind added on iOS ships a bubble full of + // JSON into somebody's conversation on every Android install that + // predates it. The engine has had `isRenderable` since ยง6.15; the + // app simply never consulted it. + if (!message.content.isRenderable) continue + val gid = if (message.fromAddress != null) null else message.content.groupId if (gid != null) { val known = groupRoster(gid) @@ -1046,7 +1184,11 @@ class Session( // side. messageId = message.content.messageId, about = message.content.about, - lifetimeSeconds = message.content.lifetimeSeconds ?: 0) + lifetimeSeconds = message.content.lifetimeSeconds ?: 0, + // ยง6.16 โ€” so second-hand words are not drawn as the sender's + // own. The engine has read `fwd` off the wire since forwarding + // shipped; the transcript was where it stopped. + forwarded = message.content.isForwarded) touched += thread } diff --git a/app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt b/app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt new file mode 100644 index 0000000..5b0e628 --- /dev/null +++ b/app/src/test/kotlin/io/flamenet/messenger/app/InboundControlTest.kt @@ -0,0 +1,138 @@ +package io.flamenet.messenger.app + +import io.flamenet.e2e.Content +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Edits, pins and unknown control envelopes must never become bubbles + * (ios#6.15/#6.16/#6.19, Android half). + * + * ๐Ÿ”ด **The bug this closes was an interop bug, not a missing feature.** The + * engine has parsed `editOf`, `pins` and `fwd` for as long as they have + * existed, and it exposes `isRenderable` โ€” "does this belong in a thread" โ€” as + * the single answer. `Session`'s inbound loop never consulted it. + * + * So an edit from the iPhone app arrived carrying its new wording in `text` and + * was filed as an ORDINARY MESSAGE: a correction drawn as a duplicate beside the + * typo it was meant to replace. A pin was filed as a bubble whose text was + * empty. And any control envelope from a newer peer โ€” which carries `ctl` and a + * kind this build has never heard of โ€” would have drawn a bubble full of JSON + * in somebody's conversation. + * + * โš ๏ธ These test the ENGINE's classification, which is what the app now branches + * on. The app-side application (`applyEdit`, `applyPin`) needs a `Vault` and a + * keystore, so it is covered on device in `FeatureUseTest`; what is checkable + * here is the decision that used to be missing. + */ +class InboundControlTest { + + private fun parse(json: String): Content = Content.parse(json)!! + + // ---- What must not be drawn ----------------------------------------- + + /** + * ๐Ÿ”ด The case that shipped a duplicate bubble. An edit carries the new + * wording, so it looks exactly like a message to anything that does not ask. + */ + @Test + fun `an edit is not renderable, even though it carries text`() { + val edit = parse(Content.edit("the corrected wording", of = "mid-1")) + assertTrue(edit.editOf == "mid-1", "the codec no longer reads editOf") + assertTrue(edit.text?.isNotEmpty() == true, "an edit carries its new text") + assertFalse(edit.isRenderable, "an edit would be filed as a message") + } + + /** A pin names an id and has nothing to show. */ + @Test + fun `a pin is not renderable`() { + val pin = parse(Content.pin("mid-1")) + assertTrue(pin.pins == "mid-1") + assertFalse(pin.isRenderable, "a pin would be filed as an empty bubble") + } + + /** Taking a pin down is the same envelope with an empty id. */ + @Test + fun `taking a pin down is also not renderable`() { + val unpin = parse(Content.pin("")) + assertTrue(unpin.pins == "", "an empty id is how a pin is removed") + assertFalse(unpin.isRenderable) + } + + /** + * ๐Ÿ”ด **The case an allowlist cannot cover, and the reason the app branches + * on `isRenderable` rather than on a list of kinds it knows.** A control + * envelope from a NEWER peer has a kind nothing here has heard of. Keying on + * known kinds ships a bubble full of JSON to every older install the first + * time a new control message is added on the other platform. + */ + @Test + fun `a control envelope from a newer peer is not renderable`() { + val fromTheFuture = parse( + """{"fnc":1,"kind":"something-invented-later","ctl":1,"text":"{}"}""" + ) + assertTrue(fromTheFuture.isControl, "the ctl flag was not honoured") + assertFalse(fromTheFuture.isRenderable, + "an unknown control kind would be drawn as a message") + } + + /** + * โš ๏ธ Both shapes of the flag. `org.json`'s `optBoolean` does not coerce a + * number, so an engine sending `"ctl":1` rather than `"ctl":true` would slip + * past a reader that checked only one โ€” which is how this class of bug got + * shipped once already. + */ + @Test + fun `the control flag is honoured as a number and as a boolean`() { + assertFalse(parse("""{"fnc":1,"kind":"x","ctl":1,"text":"{}"}""").isRenderable) + assertFalse(parse("""{"fnc":1,"kind":"x","ctl":true,"text":"{}"}""").isRenderable) + } + + // ---- What must still be drawn --------------------------------------- + + /** + * ๐Ÿ”ด The guard must not swallow ordinary messages. A `continue` in the wrong + * place is a conversation that silently stops receiving. + */ + @Test + fun `an ordinary message is still renderable`() { + val text = parse(Content.text("hello", messageId = "mid-1")) + assertTrue(text.isRenderable) + } + + @Test + fun `a reply is still renderable`() { + val reply = parse( + Content.text("answering", messageId = "mid-2", + about = Content.Reference("mid-1", 7L, "the question")), + ) + assertTrue(reply.isRenderable) + assertTrue(reply.about?.messageId == "mid-1") + } + + /** + * ๐Ÿ”ด **A forward IS a message.** It is second-hand words, not a control + * envelope โ€” the flag changes how it is drawn, not whether it is. + */ + @Test + fun `a forwarded message is renderable, and says so`() { + val forwarded = parse( + Content.text("passed along", messageId = "mid-3", forwarded = true), + ) + assertTrue(forwarded.isRenderable, "a forward was treated as control") + assertTrue(forwarded.isForwarded, "the fwd flag was lost") + } + + /** + * โš ๏ธ And the same both-shapes rule for `fwd`, for the same reason. An engine + * writing `1` where this one writes `true` would have its forwards drawn as + * the sender's own words. + */ + @Test + fun `the forwarded flag is honoured as a number and as a boolean`() { + assertTrue(parse("""{"fnc":1,"kind":"text","fwd":1,"text":"x"}""").isForwarded) + assertTrue(parse("""{"fnc":1,"kind":"text","fwd":true,"text":"x"}""").isForwarded) + assertFalse(parse("""{"fnc":1,"kind":"text","text":"x"}""").isForwarded) + } +} diff --git a/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt b/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt index 72f7caf..e61e5cc 100644 --- a/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt +++ b/engine/src/main/kotlin/io/flamenet/e2e/Vault.kt @@ -1658,7 +1658,36 @@ class Vault(private val storage: Storage, private val key: SecretKey) { * make the next message from that person undecryptable โ€” a "clear chat" * that quietly breaks the chat. */ - fun forgetHistory(threadId: String) = storage.delete(historyName(threadId)) + fun forgetHistory(threadId: String) { + storage.delete(historyName(threadId)) + // โš ๏ธ The pin goes with the transcript. A banner pointing at a message + // that was just cleared is a reference to nothing, and it would survive + // "clear this conversation" โ€” which reads as the clear not having + // worked. + storage.delete(pinnedName(threadId)) + } + + // -- Pinned message (ยง6.19) ---------------------------------------------- + + private fun pinnedName(threadId: String) = "pinned-$threadId" + + /** + * The message pinned in a thread, or null. + * + * ๐Ÿ”ด **Encrypted at rest like the transcript, not a preference.** Which + * message somebody pinned in which conversation is exactly the shape of + * metadata the vault exists to keep off a stolen phone โ€” and it is stored + * as an ID, never a copy, so it cannot go stale against an edit or outlive + * a deletion. + */ + fun pinnedMessage(threadId: String): String? = + readJson(pinnedName(threadId))?.optString("mid")?.takeIf { it.isNotEmpty() } + + /** Pin a message, or clear the pin with null. */ + fun savePinnedMessage(threadId: String, messageId: String?) { + if (messageId.isNullOrEmpty()) storage.delete(pinnedName(threadId)) + else writeJson(pinnedName(threadId), JSONObject().put("mid", messageId)) + } /** * Delete one line from a thread's history (delete for me).