Merge branch 'main' into fix-sqlcipher-native-lib-load

This commit is contained in:
Jason Ross
2026-07-03 09:25:38 -05:00
committed by GitHub
7 changed files with 141 additions and 5 deletions
@@ -111,9 +111,10 @@ internal fun buildSendMailPayload(message: OutgoingMessage, attachments: List<Se
.put("name", attachment.file.name)
.put("contentBytes", Base64.getEncoder().encodeToString(attachment.file.readBytes()))
// An inline image the HTML references as `cid:contentId` — Graph renders it in the body
// rather than listing it as a downloadable attachment.
// rather than listing it as a downloadable attachment. The content id is sanitized of
// control characters before it enters the payload (issue #204, defense-in-depth).
if (attachment.isInlineImage) {
obj.put("isInline", true).put("contentId", attachment.contentId)
obj.put("isInline", true).put("contentId", sanitizeContentId(attachment.contentId))
}
items.put(obj)
}
@@ -14,3 +14,12 @@ data class SendableAttachment(val file: File, val contentId: String? = null, val
/** True only for a genuine inline image (inline flag set and a content id to reference it by). */
val isInlineImage: Boolean get() = isInline && contentId != null
}
/**
* Strips CR/LF and other ISO control characters from a [SendableAttachment.contentId] before it is
* emitted as a MIME `Content-ID` header ([SmtpSender]) or a Graph JSON field ([GraphSender]), so a
* contentId that ever became attacker-influenced could not inject a header line. Today's ids are
* app-generated (`img-<uuid>@libremail`) and contain none, so this is a no-op for them — it is
* defense-in-depth against a future change letting external values reach [contentId] (issue #204).
*/
internal fun sanitizeContentId(raw: String?): String = raw.orEmpty().filterNot { it.isISOControl() }
@@ -121,7 +121,9 @@ class SmtpSender @Inject constructor() {
/** An inline image part: attached bytes, a `Content-ID` the HTML's `cid:` matches, inline disposition. */
private fun inlinePart(attachment: SendableAttachment): MimeBodyPart = MimeBodyPart().apply {
attachFile(attachment.file)
contentID = "<${attachment.contentId}>"
// Sanitize before it becomes a header value: a CR/LF in the content id would otherwise inject
// additional MIME header lines into this part (issue #204, defense-in-depth).
contentID = "<${sanitizeContentId(attachment.contentId)}>"
setDisposition(MimeBodyPart.INLINE)
}
@@ -27,7 +27,7 @@ object RichTextHtml {
/** The `font-family`/`font-size` declarations of the base-style wrapper (possibly empty). */
private fun baseCss(base: RichBaseStyle): String = listOfNotNull(
base.fontCss?.let { "font-family:$it" },
base.fontCss?.let(::safeFontFamily)?.let { "font-family:$it" },
base.fontSizePt?.let { "font-size:${it}pt" },
).joinToString(";")
@@ -197,7 +197,8 @@ private fun appendRun(sb: StringBuilder, content: RichTextContent, a: Int, b: In
/** Merges the parameterized styles active on a run into one CSS declaration list (maybe empty). */
private fun inlineCss(styles: List<RichStyle>): String {
val parts = ArrayList<String>()
styles.firstNotNullOfOrNull { it as? RichStyle.FontFamily }?.let { parts.add("font-family:${it.css}") }
styles.firstNotNullOfOrNull { it as? RichStyle.FontFamily }?.css?.let(::safeFontFamily)
?.let { parts.add("font-family:$it") }
styles.firstNotNullOfOrNull { it as? RichStyle.FontSize }?.let { parts.add("font-size:${it.pt}pt") }
styles.firstNotNullOfOrNull { it as? RichStyle.FontColor }?.let { parts.add("color:${cssColor(it.argb)}") }
styles.firstNotNullOfOrNull { it as? RichStyle.Highlight }
@@ -215,3 +216,16 @@ internal fun cssColor(argb: Int): String = "#" + (argb and RGB_MASK).toString(HE
internal fun escape(s: String): String = s.replace("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;")
internal fun escapeAttr(s: String): String = escape(s).replace("\"", "&quot;")
/** The characters a font-family stack legitimately uses: letters, digits, spaces, commas, quotes, etc. */
private val SAFE_FONT_FAMILY = Regex("[A-Za-z0-9 ,._'\"-]*")
/**
* Emits a `font-family` value only when every character is one a real font stack uses, so a value
* that ever carried CSS metacharacters (notably `;` or `:`) can't inject a sibling declaration into
* the raw `style` attribute — an unrecognized value is dropped rather than emitted (issue #205,
* defense-in-depth: not reachable today, since the picker only offers the fixed `FontRegistry` stacks
* and reply/forward flattens sender HTML first). Every bundled `FontRegistry` stack is within this
* set, so the built-in fonts round-trip unchanged.
*/
private fun safeFontFamily(css: String): String? = css.takeIf { SAFE_FONT_FAMILY.matches(it) }
@@ -127,4 +127,21 @@ class GraphSenderTest {
image.delete()
}
}
@Test
fun `an inline image contentId is stripped of control characters in the payload`() {
val image = File.createTempFile("graph-inline", ".png").apply { writeText("PNGDATA") }
try {
val message = message(to = "a@x.com").copy(bodyHtml = "<p><img src=\"cid:logo@libremail\"></p>")
// A crafted content id with CR/LF and a control char; only the printable text must remain.
val inline = SendableAttachment(image, contentId = "logo@libremail\r\nevil", isInline = true)
val contentId = JSONObject(buildSendMailPayload(message, listOf(inline)))
.getJSONObject("message").getJSONArray("attachments").getJSONObject(0)
.getString("contentId")
assertEquals("logo@libremailevil", contentId)
assertFalse('\r' in contentId || '\n' in contentId, "contentId=$contentId")
} finally {
image.delete()
}
}
}
@@ -4,6 +4,8 @@ package org.libremail.mail
import com.icegreen.greenmail.util.GreenMail
import com.icegreen.greenmail.util.GreenMailUtil
import com.icegreen.greenmail.util.ServerSetupTest
import jakarta.mail.Multipart
import jakarta.mail.Part
import kotlinx.coroutines.test.runTest
import org.junit.After
import org.junit.Before
@@ -13,6 +15,7 @@ import org.libremail.domain.model.OutgoingMessage
import org.libremail.domain.model.SmtpParams
import java.io.File
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNull
import kotlin.test.assertTrue
@@ -166,6 +169,62 @@ class SmtpSenderTest {
image.delete()
}
@Test
fun `a Content-ID carrying CRLF cannot inject a MIME header line`() = runTest {
val image = File.createTempFile("libremail-inline", ".png").apply { writeText("PNGDATA") }
val params = SmtpParams(
host = "127.0.0.1",
port = greenMail.smtp.port,
security = MailSecurity.NONE,
username = "sender@example.org",
secret = "secret",
useXoauth2 = false,
)
sender.send(
params = params,
from = "sender@example.org",
message = OutgoingMessage(
accountId = "x",
to = "bob@example.org",
subject = "Inline injection",
body = "See image",
bodyHtml = "<p>See <img src=\"cid:logo@libremail\"></p>",
),
// A crafted content id that, unsanitized, would break out of the Content-ID header and add
// its own `X-Injected` header line to the inline part.
attachments = listOf(
SendableAttachment(image, contentId = "logo@libremail\r\nX-Injected: evil", isInline = true),
),
)
greenMail.waitForIncomingEmail(1)
val received = greenMail.receivedMessages.single()
// The CR/LF is stripped, so the crafted text never becomes its own header on any MIME part...
assertFalse(hasHeaderAnywhere(received, "X-Injected"), "Content-ID CR/LF must not inject a header")
// ...and the Content-ID that is emitted stays on a single line.
val cid = contentIdAnywhere(received)
assertTrue(cid != null && '\r' !in cid && '\n' !in cid, "Content-ID must be a single line: $cid")
image.delete()
}
/** True if [part] or any nested MIME part carries a header named [name]. */
private fun hasHeaderAnywhere(part: Part, name: String): Boolean {
if (!part.getHeader(name).isNullOrEmpty()) return true
val content = runCatching { part.content }.getOrNull()
return content is Multipart && (0 until content.count).any { hasHeaderAnywhere(content.getBodyPart(it), name) }
}
/** The first `Content-ID` header found on [part] or any nested MIME part, or null. */
private fun contentIdAnywhere(part: Part): String? {
part.getHeader("Content-ID")?.firstOrNull()?.let { return it }
val content = runCatching { part.content }.getOrNull()
if (content is Multipart) {
for (i in 0 until content.count) contentIdAnywhere(content.getBodyPart(i))?.let { return it }
}
return null
}
@Test
fun `send delivers a message with an attachment`() = runTest {
val file = File.createTempFile("libremail-report", ".txt").apply { writeText("quarterly numbers") }
@@ -254,6 +254,40 @@ class RichTextHtmlTest {
assertRoundTrips(content)
}
@Test
fun `an unsafe inline font-family value is dropped instead of injected raw (issue 205)`() {
val content = RichTextContent(
text = "x",
spans = listOf(
RichSpan(0, 1, RichStyle.FontFamily("Arial; color:red")),
RichSpan(0, 1, RichStyle.Bold),
),
)
val html = RichTextHtml.toHtml(content)
// The smuggled sibling declaration must never reach the emitted style attribute...
assertFalse(html.contains("color:red"), html)
assertFalse(html.contains("Arial; color:red"), html)
assertFalse(html.contains("font-family"), html)
// ...but the run's other styling is unaffected.
assertTrue(html.contains("<b>x</b>"), html)
}
@Test
fun `an unsafe base-style font-family is dropped from the wrapper (issue 205)`() {
val content = RichTextContent("hi", baseStyle = RichBaseStyle(fontCss = "Arial: red;}", fontSizePt = 12))
val html = RichTextHtml.toHtml(content)
assertFalse(html.contains("font-family"), html)
// The safe font-size declaration in the same wrapper still emits.
assertTrue(html.contains("font-size:12pt"), html)
}
@Test
fun `a registry font stack with quotes and commas still emits its font-family (issue 205)`() {
// The bundled FontRegistry stacks are all within the safe set, so built-in fonts are unaffected.
val content = RichTextContent("x", spans = listOf(RichSpan(0, 1, RichStyle.FontFamily("'Inter', sans-serif"))))
assertTrue(RichTextHtml.toHtml(content).contains("font-family:'Inter', sans-serif"))
}
@Test
fun `base style wrapper adds no stray text or newlines`() {
val restored = RichTextHtml.fromHtml("<div style=\"font-size:12pt\"><p>a<br>b</p></div>")