From 7d7fef7b90b47ba55845a921964b067cc2d60401 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 07:09:33 -0500 Subject: [PATCH] chore(security): whitelist font-family on HTML emit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RichTextHtml.inlineCss and baseCss interpolated the font-family CSS value into the emitted style attribute raw. The whole attribute is escaped (escapeAttr), so a value can't break out of style="…" or inject a tag, and it isn't reachable today (the picker only offers the fixed FontRegistry stacks; reply/forward flattens sender HTML to plaintext first) — but a non-registry value would let `;`/`:` inject a sibling CSS declaration inside the attribute. Constrain the emitted font-family to a safe charset (the characters a real font stack uses — letters, digits, spaces, commas, quotes, hyphens, periods, underscores), dropping anything else instead of emitting it raw. All seven bundled FontRegistry stacks are within this set, so the built-in fonts are unaffected. RichTextHtml stays a pure module (no dependency on the UI-layer FontRegistry), so the charset restriction is the layer-clean form of the whitelist. Test: a RichStyle.FontFamily("Arial; color:red") no longer appears raw in the emitted HTML (inline and base-style), while a registry stack with quotes/commas still emits. Closes #205 Co-Authored-By: Claude Opus 4.8 --- .../org/libremail/richtext/RichTextHtml.kt | 18 ++++++++-- .../libremail/richtext/RichTextHtmlTest.kt | 34 +++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt b/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt index 7553f0e..ea18904 100644 --- a/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt +++ b/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt @@ -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): String { val parts = ArrayList() - 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("&", "&").replace("<", "<").replace(">", ">") internal fun escapeAttr(s: String): String = escape(s).replace("\"", """) + +/** 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) } diff --git a/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt b/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt index 79bbecf..e5d6bbb 100644 --- a/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt +++ b/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt @@ -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("x"), 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("

a
b

")