From 7169386437680fa1a55300fe6c189e0f690b8348 Mon Sep 17 00:00:00 2001 From: Sivert Date: Mon, 28 Sep 2026 16:10:12 +0200 Subject: [PATCH] A removed MLS device stays removed (GRYT-1555) Removing a device used to delete its row, and nothing else stopped it. If it was still signed in, its next sync said registered: false, the driver published fresh KeyPackages under the same id, and whoever sent next added it back to the DM. So removing a lost phone that was still online did nothing for long. mls:device:remove now writes the id to mls_removed_devices as well. From then on every mls:* call from that device id gets device_removed: sync, publish, claim, commit, send and welcome:ack. touchMlsDevice refuses it too, so a publish racing the removal can't bring the row back. Nothing it published is left to claim, since its KeyPackages go with the removal and it can't publish more. A new device id registers as usual, so setting the app up again makes a new device. The rows are kept for good. Any window we'd pick is one a phone left in a drawer can wait out, and a row is a few dozen bytes. There's no other per-device limit to hit either. A guest merged into an account brings its removed ids along, like it brings its devices. Co-Authored-By: Claude Opus 5.5 --- src/db/sqlite/connection.ts | 9 +++++++++ src/db/sqlite/mergeGuest.test.ts | 1 + src/db/sqlite/mls.test.ts | 15 +++++++++++++++ src/db/sqlite/mls.ts | 20 ++++++++++++++++---- src/socket/handlers/mls.test.ts | 26 ++++++++++++++++++++++++++ src/socket/handlers/mls.ts | 12 +++++++++++- 6 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/db/sqlite/connection.ts b/src/db/sqlite/connection.ts index 6a674565..6487f5b2 100644 --- a/src/db/sqlite/connection.ts +++ b/src/db/sqlite/connection.ts @@ -590,6 +590,15 @@ function createSchema(d: DatabaseSync): void { PRIMARY KEY (server_user_id, device_id) ); + -- Devices their owner removed (GRYT-1555). Kept for good: a phone in a drawer can + -- come back after any window we'd pick, and a row is a few dozen bytes. + CREATE TABLE IF NOT EXISTS mls_removed_devices ( + server_user_id TEXT NOT NULL, + device_id TEXT NOT NULL, + removed_at TEXT NOT NULL, + PRIMARY KEY (server_user_id, device_id) + ); + -- data goes to NULL when a package is handed out. The row stays until the -- retention sweep, so a Welcome naming the ref can still find its device. CREATE TABLE IF NOT EXISTS mls_key_packages ( diff --git a/src/db/sqlite/mergeGuest.test.ts b/src/db/sqlite/mergeGuest.test.ts index d9907aba..3e97019f 100644 --- a/src/db/sqlite/mergeGuest.test.ts +++ b/src/db/sqlite/mergeGuest.test.ts @@ -217,6 +217,7 @@ describe("merging a guest into an account that is already a member", () => { "users.server_user_id", "roles.server_user_id", "conversation_members.server_user_id", "mentions.server_user_id", "refresh_tokens.server_user_id", "mls_devices.server_user_id", "mls_key_packages.server_user_id", "mls_welcomes.server_user_id", + "mls_removed_devices.server_user_id", ]); const listed = new Set(AUTHOR_COLUMNS.map(([t, c]) => `${t}.${c}`)); const tables = db.prepare(`SELECT name FROM sqlite_master WHERE type = 'table'`).all() as { name: string }[]; diff --git a/src/db/sqlite/mls.test.ts b/src/db/sqlite/mls.test.ts index ea0adda2..2623c340 100644 --- a/src/db/sqlite/mls.test.ts +++ b/src/db/sqlite/mls.test.ts @@ -25,6 +25,7 @@ import { MLS_MAX_KEY_PACKAGES, mlsKeyPackageOwner, oldestMlsSeq, + isRemovedMlsDevice, removeMlsDevice, sweepMls, touchMlsDevice, @@ -80,6 +81,20 @@ describe("devices", () => { assert.deepEqual(listMlsDevices([me]), []); assert.equal(mlsKeyPackageOwner("ref-removed"), null); }); + + it("never lets a removed device id back in, and remembers it through a guest merge", async () => { + const guest = await upsertUser("mls-removed-guest", "Guest"); + const account = await upsertUser("mls-removed-account", "Account"); + touchMlsDevice(guest.server_user_id, "lost-phone"); + removeMlsDevice(guest.server_user_id, "lost-phone"); + + assert.equal(isRemovedMlsDevice(guest.server_user_id, "lost-phone"), true); + assert.equal(touchMlsDevice(guest.server_user_id, "lost-phone"), "device_removed"); + assert.equal(touchMlsDevice(guest.server_user_id, "new-phone"), "ok"); + + assert.ok(mergeGuestIntoAccount("mls-removed-guest", "mls-removed-account")); + assert.equal(touchMlsDevice(account.server_user_id, "lost-phone"), "device_removed"); + }); }); describe("KeyPackages", () => { diff --git a/src/db/sqlite/mls.ts b/src/db/sqlite/mls.ts index 9c57a61f..dc5a481c 100644 --- a/src/db/sqlite/mls.ts +++ b/src/db/sqlite/mls.ts @@ -99,14 +99,15 @@ function rowToEntry(r: Record): MlsLogEntry { // ── Devices ───────────────────────────────────────────────────────────── -/** A new device past the cap is refused; a known one only has last_seen_at moved. */ +/** A new device past the cap is refused, and so is a removed one; a known one only has last_seen_at moved. */ export function touchMlsDevice( serverUserId: string, deviceId: string, now = new Date(), -): "ok" | "too_many_devices" { +): "ok" | "too_many_devices" | "device_removed" { return inTransaction(() => { const db = getSqliteDb(); + if (isRemovedMlsDevice(serverUserId, deviceId)) return "device_removed"; const at = toIso(now); const known = db .prepare(`UPDATE mls_devices SET last_seen_at = ? WHERE server_user_id = ? AND device_id = ?`) @@ -148,10 +149,19 @@ export function listMlsDevices(serverUserIds: string[]): MlsDevice[] { })); } -/** Its KeyPackages and waiting Welcomes go with it, since nothing can use them now. */ -export function removeMlsDevice(serverUserId: string, deviceId: string): boolean { +export function isRemovedMlsDevice(serverUserId: string, deviceId: string): boolean { + return !!getSqliteDb() + .prepare(`SELECT 1 FROM mls_removed_devices WHERE server_user_id = ? AND device_id = ?`) + .get(serverUserId, deviceId); +} + +/** Its KeyPackages and waiting Welcomes go with it, and the id can never register again. */ +export function removeMlsDevice(serverUserId: string, deviceId: string, now = new Date()): boolean { return inTransaction(() => { const db = getSqliteDb(); + db.prepare( + `INSERT OR IGNORE INTO mls_removed_devices (server_user_id, device_id, removed_at) VALUES (?, ?, ?)`, + ).run(serverUserId, deviceId, toIso(now)); const gone = db .prepare(`DELETE FROM mls_devices WHERE server_user_id = ? AND device_id = ?`) .run(serverUserId, deviceId); @@ -169,6 +179,8 @@ export function carryMlsDevicesForward(db: DatabaseSync, from: string, to: strin AND device_id IN (SELECT device_id FROM mls_devices WHERE server_user_id = ?)`, ).run(from, to); db.prepare(`UPDATE mls_devices SET server_user_id = ? WHERE server_user_id = ?`).run(to, from); + db.prepare(`UPDATE OR IGNORE mls_removed_devices SET server_user_id = ? WHERE server_user_id = ?`).run(to, from); + db.prepare(`DELETE FROM mls_removed_devices WHERE server_user_id = ?`).run(from); db.prepare(`UPDATE mls_key_packages SET server_user_id = ? WHERE server_user_id = ?`).run(to, from); db.prepare(`UPDATE mls_welcomes SET server_user_id = ? WHERE server_user_id = ?`).run(to, from); } diff --git a/src/socket/handlers/mls.test.ts b/src/socket/handlers/mls.test.ts index e7fbba23..a0d36b10 100644 --- a/src/socket/handlers/mls.test.ts +++ b/src/socket/handlers/mls.test.ts @@ -437,6 +437,32 @@ describe("KeyPackages", () => { assert.equal((await publish(carol, await makeDevice("carol-5", 1))).ok, true, "removing one frees its slot"); }); + it("won't take a removed device back, though a new one is fine (GRYT-1555)", async () => { + const hana = await connect("Hana"); + const ivar = await connect("Ivar"); + const conv = (await openDirectConversation(hana.serverUserId, ivar.serverUserId)).conversation_id; + const laptop = await makeDevice("hana-laptop", 1); + const phone = await makeDevice("hana-phone", 2); + const ivarPhone = await makeDevice("ivar-phone", 1); + assert.equal((await publish(hana, laptop)).ok, true); + assert.equal((await publish(hana, phone)).ok, true); + assert.equal((await publish(ivar, ivarPhone)).ok, true); + + assert.equal((await hana.call("mls:device:remove", { deviceId: phone.id })).ok, true); + // The phone is still signed in and does what the driver does on reconnect. + assert.equal((await hana.call("mls:sync", { deviceId: phone.id })).error, "device_removed"); + assert.equal((await publish(hana, await makeDevice("hana-phone", 2))).error, "device_removed"); + const own = await hana.call("mls:keypackages:claim", { conversationId: conv, deviceId: phone.id }); + assert.equal(own.error, "device_removed"); + + const claimed = await ivar.call("mls:keypackages:claim", { conversationId: conv, deviceId: ivarPhone.id }); + assert.deepEqual((claimed.keyPackages as { deviceId: string }[]).map((k) => k.deviceId), [laptop.id]); + const listed = await ivar.call("mls:devices", { conversationId: conv }); + assert.equal((listed.devices as { deviceId: string }[]).some((d) => d.deviceId === phone.id), false); + + assert.equal((await publish(hana, await makeDevice("hana-phone-again", 1))).ok, true, "setting it up again is a new device"); + }); + it("are handed out once each, then the last-resort one", async () => { const eve = await connect("Eve"); const frank = await connect("Frank"); diff --git a/src/socket/handlers/mls.ts b/src/socket/handlers/mls.ts index 97222788..d7c1f237 100644 --- a/src/socket/handlers/mls.ts +++ b/src/socket/handlers/mls.ts @@ -20,6 +20,7 @@ import { getUserByServerId, insertMessage, isMlsDevice, + isRemovedMlsDevice, listConversationsForUser, listMlsDevices, listMlsGroupsForMember, @@ -167,6 +168,10 @@ export function registerMlsHandlers(ctx: HandlerContext): EventHandlerMap { ack(fail("invalid_device", "deviceId has to be 1 to 64 letters, digits, - or _.")); return false; } + if (isRemovedMlsDevice(auth.tokenPayload.serverUserId, deviceId)) { + ack(fail("device_removed", "You removed this device from encrypted messages here.")); + return false; + } if (mustExist && !isMlsDevice(auth.tokenPayload.serverUserId, deviceId)) { ack(fail("unknown_device", "Publish KeyPackages from this device first.")); return false; @@ -345,7 +350,12 @@ export function registerMlsHandlers(ctx: HandlerContext): EventHandlerMap { } const isNew = !isMlsDevice(self, payload.deviceId); - if (touchMlsDevice(self, payload.deviceId) === "too_many_devices") { + const touched = touchMlsDevice(self, payload.deviceId); + if (touched === "device_removed") { + ack(fail("device_removed", "You removed this device from encrypted messages here.")); + return; + } + if (touched === "too_many_devices") { ack(fail("too_many_devices", "You have five devices using encrypted messages here. Remove one first.")); return; }