diff --git a/app/src/main/kotlin/org/libremail/mail/GraphSender.kt b/app/src/main/kotlin/org/libremail/mail/GraphSender.kt index a183458..bbe3762 100644 --- a/app/src/main/kotlin/org/libremail/mail/GraphSender.kt +++ b/app/src/main/kotlin/org/libremail/mail/GraphSender.kt @@ -7,6 +7,7 @@ import kotlinx.coroutines.withContext import org.json.JSONArray import org.json.JSONObject import org.libremail.domain.model.OutgoingMessage +import org.libremail.reporting.AppLog import java.io.IOException import java.net.HttpURLConnection import java.net.URL @@ -34,6 +35,14 @@ class GraphSender @Inject constructor() { message: OutgoingMessage, attachments: List = emptyList(), ) = withContext(Dispatchers.IO) { + // Guard before any attachment is read into memory: Graph sendMail carries attachment bytes inline + // (base64) in a single ~4 MB request, so an oversized file would blow that request limit and risk + // an OOM from readBytes(). Fail with mayHaveSent=false so the outbox falls back to SMTP, which + // streams attachments and handles far larger files (#298). + attachments.firstOrNull { it.file.length() > MAX_ATTACHMENT_BYTES }?.let { + AppLog.w(TAG, "Attachment over Graph sendMail size limit; not sending via Graph") + throw GraphSendException("Attachment exceeds the Graph sendMail size limit", mayHaveSent = false) + } val payload = buildSendMailPayload(message, attachments) val connection = (URL(SEND_MAIL_URL).openConnection() as HttpURLConnection).apply { requestMethod = "POST" @@ -76,8 +85,13 @@ class GraphSender @Inject constructor() { } private companion object { + const val TAG = "GraphSender" const val SEND_MAIL_URL = "https://graph.microsoft.com/v1.0/me/sendMail" const val TIMEOUT_MS = 15_000 + + // Per-file ceiling kept below Graph sendMail's ~4 MB whole-request cap, so one attachment can never + // exceed the request limit or OOM when read into the base64 payload; larger files fall back to SMTP. + const val MAX_ATTACHMENT_BYTES = 3L * 1024 * 1024 const val HTTP_OK_MIN = 200 const val HTTP_OK_MAX = 299 const val ERROR_BODY_LIMIT = 500 diff --git a/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt b/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt index 495368a..0c77768 100644 --- a/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt +++ b/app/src/main/kotlin/org/libremail/mail/HtmlToText.kt @@ -12,23 +12,29 @@ package org.libremail.mail */ object HtmlToText { + // Hoisted out of convert() so each pattern is compiled once, not four times per call — convert() + // runs once per fetched HTML body during sync, so this is a hot path (#298). + private val SCRIPT_STYLE = Regex("(?is)<(script|style)\\b[^>]*>.*?") + private val LIST_ITEM = Regex("(?i)]*>") private val BLOCK_BREAK = Regex( "(?i)]*>|", ) - private val LIST_ITEM = Regex("(?i)]*>") + private val TAG = Regex("<[^>]*>") + private val SPACES_AND_TABS = Regex("[ \\t]+") + private val BLANK_LINES = Regex("\n{3,}") fun convert(html: String): String { var s = html // Drop script/style contents outright so their text never leaks into the output. - s = s.replace(Regex("(?is)<(script|style)\\b[^>]*>.*?"), "") + s = SCRIPT_STYLE.replace(s, "") s = LIST_ITEM.replace(s, "\n• ") s = BLOCK_BREAK.replace(s, "\n") - s = s.replace(Regex("<[^>]*>"), "") + s = TAG.replace(s, "") s = decodeEntities(s) // Collapse runs of spaces/tabs, then trim trailing spaces and cap consecutive blank lines. - s = s.replace(Regex("[ \\t]+"), " ") + s = SPACES_AND_TABS.replace(s, " ") s = s.lineSequence().joinToString("\n") { it.trim() } - s = s.replace(Regex("\n{3,}"), "\n\n") + s = BLANK_LINES.replace(s, "\n\n") return s.trim() } diff --git a/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt b/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt index 2581525..6fdf54a 100644 --- a/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt +++ b/app/src/main/kotlin/org/libremail/reporting/DiagnosticsCollector.kt @@ -99,14 +99,24 @@ private fun providerLabel(account: Account): String = when (account.authType) { AuthType.PASSWORD_IMAP -> imapProviderLabel(account.imap.host) } +/** + * Buckets an IMAP host to a coarse provider by matching brand tokens at DNS-label boundaries rather + * than as raw substrings, so a custom domain that merely contains a brand name — e.g. + * `mail.notgmail.example` — is no longer mislabeled (here it would have read as Gmail) (#298). The + * short, common tokens (`me`/`mac`/`live`) match only as a registrable-domain suffix, never as a bare + * label, so an innocent `me.company.example` doesn't read as iCloud either. + */ private fun imapProviderLabel(host: String): String { val h = host.lowercase() + val labels = h.split('.') + fun hasLabel(vararg brands: String) = brands.any { it in labels } + fun hasDomain(vararg domains: String) = domains.any { h == it || h.endsWith(".$it") } return when { - "gmail" in h || "googlemail" in h -> "Gmail" - "yahoo" in h -> "Yahoo" - "icloud" in h || "me.com" in h || "mac.com" in h -> "iCloud" - "outlook" in h || "office365" in h || "hotmail" in h || "live.com" in h -> "Outlook" - "aol" in h -> "AOL" + hasLabel("gmail", "googlemail") -> "Gmail" + hasLabel("yahoo") -> "Yahoo" + hasLabel("icloud") || hasDomain("me.com", "mac.com") -> "iCloud" + hasLabel("outlook", "office365", "hotmail") || hasDomain("live.com") -> "Outlook" + hasLabel("aol") -> "AOL" else -> "Other" } } diff --git a/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt b/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt index 3fe7b67..d4cfa63 100644 --- a/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt +++ b/app/src/main/kotlin/org/libremail/reporting/ReportStore.kt @@ -9,6 +9,9 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch import java.io.File +import java.nio.file.AtomicMoveNotSupportedException +import java.nio.file.Files +import java.nio.file.StandardCopyOption /** * File-backed store of pending [DebugReport]s — one JSON file per report under [directory]. @@ -52,7 +55,7 @@ class ReportStore( synchronized(lock) { val serialized = serializeForDisk(report) ?: return directory.mkdirs() - File(directory, fileName(report.id)).writeText(serialized) + writeAtomically(File(directory, fileName(report.id)), serialized) _reports.value = scan() } } @@ -69,7 +72,7 @@ class ReportStore( val report = _reports.value.firstOrNull { it.id == id } ?: return if (report.surfaced) return val serialized = serializeForDisk(report.copy(surfaced = true)) ?: return - File(directory, fileName(id)).writeText(serialized) + writeAtomically(File(directory, fileName(id)), serialized) _reports.value = scan() } } @@ -135,10 +138,38 @@ class ReportStore( return runCatching { DebugReport.fromStorageJson(json) }.getOrNull() } + /** + * Writes [content] to [target] via a temp file + atomic rename, so a process death mid-write — the + * crash path that saves a report while the app is dying — can never leave a torn `.json` that [scan] + * would fail to parse and silently drop (#298). The temp file uses a non-`.json` suffix so [scan] + * ignores it (and any orphan left by an interrupted write), and the rename replaces an existing file + * (the [markSurfaced] rewrite) atomically. Falls back to a plain replace on the rare filesystem + * without atomic rename — still safer than an in-place truncate-then-write. + */ + private fun writeAtomically(target: File, content: String) { + val tmp = File(directory, target.name + TMP_SUFFIX) + tmp.writeText(content) + try { + Files.move( + tmp.toPath(), + target.toPath(), + StandardCopyOption.ATOMIC_MOVE, + StandardCopyOption.REPLACE_EXISTING, + ) + } catch (e: AtomicMoveNotSupportedException) { + AppLog.w(TAG, "Atomic report write unsupported here; falling back to a non-atomic replace", e) + Files.move(tmp.toPath(), target.toPath(), StandardCopyOption.REPLACE_EXISTING) + } + } + private fun fileName(id: String) = "$id$SUFFIX" private companion object { const val SUFFIX = ".json" + + // Suffix for the write-and-rename temp file. Deliberately NOT ending in [SUFFIX] so scan() never + // treats a half-written or orphaned temp as a report (#298). + const val TMP_SUFFIX = ".tmp" const val TAG = "ReportStore" /** diff --git a/app/src/main/kotlin/org/libremail/richtext/RichText.kt b/app/src/main/kotlin/org/libremail/richtext/RichText.kt index 4da265e..23029ad 100644 --- a/app/src/main/kotlin/org/libremail/richtext/RichText.kt +++ b/app/src/main/kotlin/org/libremail/richtext/RichText.kt @@ -109,11 +109,17 @@ internal fun lineMarker(line: String): String? = when { */ internal fun mergeSameValueSpans(spans: List): List { val merged = ArrayList() + // Index of the right-most merged run for each style value. Spans are processed in ascending start + // order, and runs of one value stay non-overlapping with strictly increasing ends, so only that + // value's last run can touch the next span — track it directly instead of re-scanning `merged` for + // every span (the old O(n^2) indexOfLast). The produced list is byte-for-byte identical (#298). + val lastRunByStyle = HashMap() for (span in spans.sortedWith(compareBy({ it.start }, { it.end }))) { - val i = merged.indexOfLast { it.style == span.style && span.start <= it.end } - if (i >= 0) { + val i = lastRunByStyle[span.style] + if (i != null && span.start <= merged[i].end) { merged[i] = merged[i].copy(end = maxOf(merged[i].end, span.end)) } else { + lastRunByStyle[span.style] = merged.size merged.add(span) } } diff --git a/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt b/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt index ed67e10..65bccf9 100644 --- a/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt +++ b/app/src/main/kotlin/org/libremail/richtext/RichTextEditing.kt @@ -33,10 +33,15 @@ object RichTextEditing { return content.copy(spans = (otherKinds + updated).sortedBy { it.start }) } - /** Links [[start], [end]) to [url], replacing any links that overlap the range. */ + /** + * Links [[start], [end]) to [url]. A link that only partially overlaps the range keeps its + * non-overlapping remainder — the same split [toggleStyle]/[subtractRange] does for spans — instead + * of being dropped whole, so relinking part of a longer link no longer silently un-links the rest + * of it (#298). A link fully inside the range is replaced outright. + */ fun applyLink(content: RichTextContent, start: Int, end: Int, url: String): RichTextContent { if (start >= end || url.isBlank()) return content - val kept = content.links.filter { it.end <= start || it.start >= end } + val kept = subtractLinkRange(content.links, start, end) return content.copy(links = (kept + RichLink(start, end, url)).sortedBy { it.start }) } @@ -221,6 +226,17 @@ private fun subtractRange(spans: List, start: Int, end: Int): List, start: Int, end: Int): List = links.flatMap { link -> + when { + link.end <= start || link.start >= end -> listOf(link) + else -> buildList { + if (link.start < start) add(link.copy(end = start)) + if (link.end > end) add(link.copy(start = end)) + } + } +} + // --- block marker helpers --- private val ORDERED = Regex("^\\d+\\. ") diff --git a/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt b/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt index dbcb6de..c4c02ea 100644 --- a/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/GraphSenderSendTest.kt @@ -1,14 +1,20 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.mail +import io.mockk.every +import io.mockk.mockkStatic +import io.mockk.unmockkAll import kotlinx.coroutines.test.runTest import org.junit.After +import org.junit.Before import org.junit.Test import org.libremail.domain.model.OutgoingMessage import java.io.ByteArrayOutputStream +import java.io.File import java.io.IOException import java.io.InputStream import java.io.OutputStream +import java.io.RandomAccessFile import java.net.HttpURLConnection import java.net.URL import java.net.URLConnection @@ -18,6 +24,7 @@ import java.util.concurrent.atomic.AtomicReference import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue /** @@ -30,8 +37,19 @@ import kotlin.test.assertTrue */ class GraphSenderSendTest { + @Before + fun setUp() { + // send() now breadcrumbs through AppLog on the oversized-attachment guard; android.util.Log is a + // no-op stub under plain JVM tests, so mock it (fully qualified, so this file never imports it). + mockkStatic(android.util.Log::class) + every { android.util.Log.w(any(), any()) } returns 0 + } + @After - fun tearDown() = armed.set(null) + fun tearDown() { + armed.set(null) + unmockkAll() + } private val message = OutgoingMessage(accountId = "outlook:me@x.com", to = "bob@example.org", subject = "Hi", body = "Body") @@ -84,6 +102,26 @@ class GraphSenderSendTest { assertFalse(ex.mayHaveSent, "the request never reached Graph, so a retry is safe") } + @Test + fun `an oversized attachment fails safe-to-fall-back before opening a connection`() = runTest { + val last = arm() // armed, but the guard must trip before any connection is opened + val big = File.createTempFile("graph-big", ".bin") + try { + // 4 MiB, over the 3 MiB per-file cap. setLength allocates the size without writing the bytes, + // so the guard (which reads file.length()) trips without the test materializing 4 MiB. + RandomAccessFile(big, "rw").use { it.setLength(4L * 1024 * 1024) } + + val ex = assertFailsWith { + GraphSender().send("token", message, listOf(SendableAttachment(big))) + } + + assertFalse(ex.mayHaveSent, "oversized never reached Graph, so SMTP fallback is safe") + assertNull(last.get(), "the guard must trip before any connection is opened") + } finally { + big.delete() + } + } + @Test fun `GraphSendException carries its message, flag and cause`() { val cause = IOException("boom") diff --git a/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt b/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt index 859d2fa..2f75273 100644 --- a/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/DiagnosticsCollectorTest.kt @@ -169,6 +169,26 @@ class DiagnosticsCollectorTest { ) } + @Test + fun `provider label matches brand tokens at label boundaries, not as substrings`() = runTest { + every { settingsRepository.settings } returns flowOf(AppSettings()) + // Each custom host merely CONTAINS a brand name inside a longer DNS label; the old substring match + // mislabeled them (notgmail→Gmail, yahooligans→Yahoo, me.company→iCloud, kaolin→AOL). They must all + // bucket to "Other" now (#298). + every { accountRepository.observeAccounts() } returns flowOf( + listOf( + account("1@x", AuthType.PASSWORD_IMAP, "mail.notgmail.example"), + account("2@x", AuthType.PASSWORD_IMAP, "imap.yahooligans.example"), + account("3@x", AuthType.PASSWORD_IMAP, "me.company.example"), + account("4@x", AuthType.PASSWORD_IMAP, "kaolin.example"), + ), + ) + + val report = collector.collectManual() + + assertEquals(List(4) { "Other (PASSWORD_IMAP)" }, report.accounts) + } + private fun account(email: String, authType: AuthType, imapHost: String) = Account( id = "id:$email", email = email, diff --git a/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt b/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt index 9becacd..9f9c885 100644 --- a/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/ReportStoreTest.kt @@ -111,6 +111,29 @@ class ReportStoreTest { assertEquals(listOf("valid"), store.reports.value.map { it.id }) } + @Test + fun `save leaves no temporary file behind (atomic write renames it into place)`() { + val store = newStore() + + store.save(report("a")) + + // The write-and-rename temp must not linger: only the final ".json" remains on disk (#298). + assertEquals(listOf("a.json"), tempFolder.root.listFiles()?.map { it.name }.orEmpty()) + } + + @Test + fun `a stray temp file from an interrupted write is never scanned as a report`() { + // A process death mid-write leaves a ".json.tmp" file, never a torn ".json". scan() filters on + // ".json", so the orphan is ignored and a valid report saved alongside still lists cleanly — the + // old in-place write could instead leave a truncated ".json" that scan() silently dropped (#298). + File(tempFolder.root, "torn.json.tmp").writeText("{ half-written") + val store = newStore() + + store.save(report("valid")) + + assertEquals(listOf("valid"), store.reports.value.map { it.id }) + } + @Test fun `purgeOlderThan deletes reports strictly older than the cutoff`() { val store = newStore() diff --git a/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt b/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt index 52553e4..e7707e1 100644 --- a/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt +++ b/app/src/test/kotlin/org/libremail/richtext/RichTextEditingTest.kt @@ -301,18 +301,53 @@ class RichTextEditingTest { } @Test - fun `applyLink keeps links wholly outside the range and replaces overlapping ones`() { + fun `applyLink keeps links outside the range and splits partial overlaps, keeping the remainder`() { val content = RichTextContent( "0123456789", links = listOf(RichLink(0, 2, "a"), RichLink(3, 6, "b"), RichLink(7, 9, "c")), ) val result = RichTextEditing.applyLink(content, 4, 7, "http://new") assertEquals( - listOf(RichLink(0, 2, "a"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")), + // "a" is wholly outside; "b" (3,6) overlaps [4,7) so only its (3,4) remainder survives (it is + // no longer dropped whole); the new link takes [4,7); "c" starts at the range end, kept whole. + listOf(RichLink(0, 2, "a"), RichLink(3, 4, "b"), RichLink(4, 7, "http://new"), RichLink(7, 9, "c")), result.links.sortedBy { it.start }, ) } + @Test + fun `applyLink over the middle of a link relinks the middle and keeps both surrounding remainders`() { + val content = RichTextContent("0123456789", links = listOf(RichLink(0, 8, "old"))) + val result = RichTextEditing.applyLink(content, 3, 5, "new") + assertEquals( + listOf(RichLink(0, 3, "old"), RichLink(3, 5, "new"), RichLink(5, 8, "old")), + result.links.sortedBy { it.start }, + ) + } + + // --- mergeSameValueSpans (shared merge used by toggleStyle and the HTML parser) --- + + @Test + fun `mergeSameValueSpans coalesces touching and overlapping runs of the same value only`() { + val merged = mergeSameValueSpans( + listOf( + RichSpan(5, 8, RichStyle.Bold), // out of order, and overlaps the (3,6) run below + RichSpan(0, 3, RichStyle.Bold), + RichSpan(3, 6, RichStyle.Bold), // touches (0,3) and overlaps (5,8) → one 0..8 run + RichSpan(0, 4, RichStyle.Italic), // a different value never folds into the Bold run + RichSpan(10, 12, RichStyle.Bold), // a gap breaks the run into a fresh one + ), + ) + assertEquals( + listOf( + RichSpan(0, 8, RichStyle.Bold), + RichSpan(0, 4, RichStyle.Italic), + RichSpan(10, 12, RichStyle.Bold), + ), + merged, + ) + } + // --- styleAt / isStyled caret edges --- @Test