From 08290dc85f1fa68887b03628f61a14ec955e5680 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 7 Jul 2026 23:18:40 -0500 Subject: [PATCH] fix(mail): gracefully fall back when the IMAP server lacks UIDPLUS The targeted UID EXPUNGE (IMAPFolder.expunge(Message[])) from #295/#318 throws "UID EXPUNGE not supported" on a server without the UIDPLUS extension, which broke delete/move entirely on rare self-hosted/legacy IMAP servers (#319). expungeTargeted now probes UIDPLUS from the folder's own already-open protocol (via IMAPFolder.doCommand, so it never opens a second connection + LOGIN and keeps the one-connection-per-batch invariant of #125/#295). With UIDPLUS it still uses the targeted UID EXPUNGE. Without it, it falls back to a plain, untargeted EXPUNGE only when provably safe: the messages we just flagged are the only \Deleted ones in the folder. When unrelated \Deleted mail is present a plain EXPUNGE would destroy it, and there is no UIDPLUS-free way to expunge a single UID, so we refuse and fail loud, preserving the #295 "never touch unrelated \Deleted mail" invariant. PII-free AppLog breadcrumbs record the fallback decision. Covered by GreenMail unit tests for both branches (with UIDPLUS via the default probe; without via an injected capability seam, since GreenMail always advertises UIDPLUS). Closes #319 --- .../kotlin/org/libremail/mail/ImapClient.kt | 89 +++++++++++++++++-- .../org/libremail/mail/ImapClientTest.kt | 71 +++++++++++++++ 2 files changed, 151 insertions(+), 9 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt index 2ab1570..4a49550 100644 --- a/app/src/main/kotlin/org/libremail/mail/ImapClient.kt +++ b/app/src/main/kotlin/org/libremail/mail/ImapClient.kt @@ -5,6 +5,7 @@ import jakarta.mail.FetchProfile import jakarta.mail.Flags import jakarta.mail.Folder import jakarta.mail.Message +import jakarta.mail.MessagingException import jakarta.mail.Multipart import jakarta.mail.Part import jakarta.mail.Session @@ -103,6 +104,16 @@ data class ReplyContext( class ImapClient internal constructor( private val reuseConnections: Boolean, private val reuseIdleTimeoutMillis: Long = DEFAULT_REUSE_IDLE_TIMEOUT_MS, + /** + * Reports whether the folder's already-open IMAP connection advertises the UIDPLUS extension (RFC + * 4315), which gates the targeted `UID EXPUNGE` vs. the plain-EXPUNGE fallback in [expungeTargeted] + * (issue #319). Reads the capability from the folder's own protocol so it never opens a second + * connection + LOGIN mid-operation — preserving the one-connection-per-batch invariant of issues + * #125/#295. Defaults to the real probe ([probeUidPlusCapability]); the internal constructor lets a + * test inject a fixed value, because GreenMail always advertises UIDPLUS and so cannot exercise the + * no-UIDPLUS fallback path on its own. + */ + private val supportsUidPlus: (IMAPFolder) -> Boolean = ::probeUidPlusCapability, ) { /** @@ -432,17 +443,61 @@ class ImapClient internal constructor( } /** - * Permanently removes exactly [messages] — which the caller has already flagged `\Deleted` — with a - * **targeted** UID EXPUNGE (RFC 4315, [IMAPFolder.expunge]). Deliberately never the untargeted - * [Folder.expunge], which expunges *every* `\Deleted`-flagged message in [mailbox] — including ones - * a second client, Gmail, or a partial earlier move left flagged — the data-loss bug in issue #295. - * Every folder this client opens is an [IMAPFolder], so the cast holds in production and under - * GreenMail. The server must advertise UIDPLUS (Gmail, Outlook, and GreenMail do); on one that does - * not, Angus raises "UID EXPUNGE not supported" here rather than silently falling back to the - * unrelated-mail-destroying untargeted expunge — a loud failure is the correct, safe outcome. + * Permanently removes exactly [messages] — which the caller has already flagged `\Deleted` — using a + * **targeted** UID EXPUNGE (RFC 4315, [IMAPFolder.expunge]) whenever the server advertises UIDPLUS + * (Gmail, Outlook, and GreenMail do). That never touches other `\Deleted`-flagged mail in [mailbox] + * — ones a second client, Gmail, or a partial earlier move left flagged — the data-loss bug in + * issue #295. + * + * On a server **without** UIDPLUS the targeted UID EXPUNGE throws `UID EXPUNGE not supported`, which + * used to break delete/move entirely (issue #319). There we fall back to a plain, untargeted + * [Folder.expunge] — but **only** when it is provably safe, i.e. the messages we just flagged are the + * *only* `\Deleted` ones in [mailbox] ([countForeignDeletedMessages] is 0). A plain EXPUNGE removes + * **every** `\Deleted` message, so when unrelated `\Deleted` mail is present we refuse and fail loud + * rather than destroy mail the user never selected: there is no UIDPLUS-free way to expunge one + * specific UID while sparing the others (that capability is exactly what UIDPLUS provides), so + * preserving the issue-#295 invariant — never touch unrelated `\Deleted` mail — wins over completing + * the delete. Every folder this client opens is an [IMAPFolder], so the cast holds in production and + * under GreenMail. */ private fun expungeTargeted(mailbox: Folder, messages: Array) { - (mailbox as IMAPFolder).expunge(messages) + val imapFolder = mailbox as IMAPFolder + if (supportsUidPlus(imapFolder)) { + imapFolder.expunge(messages) + return + } + val foreignDeleted = countForeignDeletedMessages(imapFolder, messages) + if (foreignDeleted == 0) { + AppLog.i(EXPUNGE_TAG, "server lacks UIDPLUS; no other \\Deleted mail present, using safe plain EXPUNGE") + imapFolder.expunge() + } else { + AppLog.w( + EXPUNGE_TAG, + "server lacks UIDPLUS and $foreignDeleted other \\Deleted message(s) present; " + + "refusing plain EXPUNGE so unrelated mail is not destroyed", + ) + throw MessagingException( + "Cannot honor a targeted expunge: the server does not support UIDPLUS and other deleted " + + "messages are present, so a plain EXPUNGE would remove mail that was not selected", + ) + } + } + + /** + * Counts messages in [mailbox] flagged `\Deleted` that are **not** among [targets] (the ones the + * caller just flagged). Used only on the no-UIDPLUS fallback in [expungeTargeted] to decide whether a + * plain EXPUNGE is safe — it is safe exactly when this is 0. Fetches FLAGS + UID for the whole folder, + * acceptable on this rare legacy-server path. + */ + private fun countForeignDeletedMessages(mailbox: IMAPFolder, targets: Array): Int { + val targetUids = targets.mapTo(HashSet()) { mailbox.getUID(it) } + val all = mailbox.messages + val profile = FetchProfile().apply { + add(FetchProfile.Item.FLAGS) + add(UIDFolder.FetchProfileItem.UID) + } + mailbox.fetch(all, profile) + return all.count { it.isSet(Flags.Flag.DELETED) && mailbox.getUID(it) !in targetUids } } /** @@ -719,6 +774,7 @@ class ImapClient internal constructor( const val TIMEOUT_MS = "15000" const val TAG = "LibreMailIdle" const val PERF_TAG = "ImapPerf" + const val EXPUNGE_TAG = "ImapExpunge" const val NANOS_PER_MS = 1_000_000L // A reused connection unused for this long is idle-evicted (issue #357 Part 2): long enough to @@ -728,6 +784,21 @@ class ImapClient internal constructor( } } +/** The IMAP UIDPLUS extension name (RFC 4315); its presence is what makes a targeted `UID EXPUNGE` legal. */ +private const val CAP_UIDPLUS = "UIDPLUS" + +/** + * Whether [folder]'s already-open connection advertises the IMAP UIDPLUS extension (RFC 4315) — the + * capability a targeted `UID EXPUNGE` requires. Reads it from the folder's own protocol via + * [IMAPFolder.doCommand] (the capabilities parsed at LOGIN; no extra round trip), reusing the open + * connection rather than borrowing a fresh store protocol — which would open a second connection + LOGIN + * mid-batch and defeat the reuse invariant of issue #125. Any read failure degrades to `false`, so the + * caller takes the safe plain-EXPUNGE fallback. The production default of [ImapClient.supportsUidPlus]. + */ +private fun probeUidPlusCapability(folder: IMAPFolder): Boolean = runCatching { + folder.doCommand { protocol -> protocol.hasCapability(CAP_UIDPLUS) } as? Boolean +}.getOrNull() ?: false + /** * True when [part] is a user-facing downloadable attachment: its `Content-Disposition` is * `attachment`, OR it has a filename but no `Content-ID` header. A part with a filename AND a diff --git a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt index 5c23358..113be5f 100644 --- a/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/ImapClientTest.kt @@ -364,6 +364,77 @@ class ImapClientTest { assertTrue(archive.contains("Move me"), "archive=$archive") } + // --- issue #319: graceful fallback when the server lacks the UIDPLUS extension --- + // + // GreenMail always advertises UIDPLUS, so the "with UIDPLUS" case (a targeted UID EXPUNGE that spares + // unrelated \Deleted mail) is exercised by the default-client delete/move tests above. To drive the + // "without UIDPLUS" branch against the same real server, these tests inject a capability probe that + // reports no UIDPLUS while every EXPUNGE below still runs for real against GreenMail. + + private val noUidPlusClient = ImapClient(reuseConnections = false, supportsUidPlus = { false }) + + @Test + fun `deleteMessages falls back to a safe plain expunge when the server lacks UIDPLUS`() = runTest { + val buffer = RingLogBuffer() + AppLog.install(buffer) + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Target", "delete this") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Keep", "no flag at all") + greenMail.waitForIncomingEmail(2) + val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject } + + // No other \Deleted mail is present, so a plain EXPUNGE removes only the target — and must not throw. + noUidPlusClient.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("Target").uid)) + + val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + assertFalse(remaining.contains("Target"), "the target must be expunged via the fallback, remaining=$remaining") + assertTrue(remaining.contains("Keep"), "an unflagged message must survive, remaining=$remaining") + assertTrue( + buffer.snapshot().any { it.message.contains("using safe plain EXPUNGE") }, + "the no-UIDPLUS fallback decision must be logged, messages=${buffer.snapshot().map { it.message }}", + ) + } + + @Test + fun `deleteMessages without UIDPLUS refuses to expunge when other Deleted mail is present`() = runTest { + val buffer = RingLogBuffer() + AppLog.install(buffer) + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Target", "delete this") + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Bystander", "flagged elsewhere") + greenMail.waitForIncomingEmail(2) + val bySubject = client.fetchRecent(params(), "INBOX", limit = 50).associateBy { it.subject } + // A second client left "Bystander" flagged \Deleted but un-expunged; a plain EXPUNGE would drop it. + client.setFlag(params(), "INBOX", bySubject.getValue("Bystander").uid, Flags.Flag.DELETED, value = true) + + // Without UIDPLUS there is no way to expunge only "Target" while sparing "Bystander": refuse loudly + // rather than destroy mail the user never selected (preserves the issue #295 invariant). + assertFailsWith { + noUidPlusClient.deleteMessages(params(), "INBOX", listOf(bySubject.getValue("Target").uid)) + } + + val remaining = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + assertTrue(remaining.contains("Target"), "the refused target must NOT be expunged, remaining=$remaining") + assertTrue(remaining.contains("Bystander"), "unrelated \\Deleted mail must survive, remaining=$remaining") + assertTrue( + buffer.snapshot().any { it.message.contains("refusing plain EXPUNGE") }, + "the refusal must be logged, messages=${buffer.snapshot().map { it.message }}", + ) + } + + @Test + fun `moveMessages falls back to a safe plain expunge when the server lacks UIDPLUS`() = runTest { + GreenMailUtil.sendTextEmailTest("alice@example.org", "bob@example.org", "Move me", "relocate this") + greenMail.waitForIncomingEmail(1) + appendMessage("Archive", "carol@example.org", "Seed", "Creates the Archive folder") + val uid = client.fetchRecent(params(), "INBOX", limit = 50).first { it.subject == "Move me" }.uid + + noUidPlusClient.moveMessages(params(), "INBOX", listOf(uid), "Archive") + + val inbox = client.fetchRecent(params(), "INBOX", limit = 50).map { it.subject } + val archive = client.fetchRecent(params(), "Archive", limit = 50).map { it.subject } + assertFalse(inbox.contains("Move me"), "the moved message must leave the source via the fallback, inbox=$inbox") + assertTrue(archive.contains("Move me"), "archive=$archive") + } + @Test fun `fetchRecent returns empty for an empty folder`() = runTest { createFolder("Empty") -- 2.47.3