From ff15246c7b4a378b7af894039bb146ea5dedc676 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 14:37:31 +0000 Subject: [PATCH] fix: harden the title/favicon link preview fallback Audit fixes for the compact link preview: - Stop reading and icon <link>s once <body> starts. </head> is optional, and without it an inline SVG's <title> ("Close", "Menu") was taken as the page title. - Never cut a long <title> inside a surrogate pair. - Match the HTML mime type case-insensitively: UrlPreview stores the server's spelling, so "Text/HTML" pages lost their icon. - Honour data: icons. An inline data:image icon (up to 64 KB) is used as is; `href="data:,"` means "no favicon", so /favicon.ico is no longer requested for those sites. - Remember icon URLs the server refused (HTTP errors only, not network errors) so a missing /favicon.ico is not re-requested every time its card scrolls back into view. - Keep the open-in-browser callback stable across recompositions. - Benchmark rows for the title/icon parse path: no measurable cost on the heavy head, ~1.5 us on a small one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KdPp7UiETpzv8jQ5qfAFqp --- .../commons/preview/MetaTagsParser.kt | 26 ++++++++++++-- .../amethyst/commons/preview/UrlInfoItem.kt | 16 ++++++++- .../preview/TitleAndIconFallbackTest.kt | 34 +++++++++++++++++++ .../preview/UrlInfoItemTextPreviewTest.kt | 22 ++++++++++++ .../prodbench/MetaTagsParserBenchmark.kt | 13 +++++++ .../commons/ui/components/UrlPreviewCard.kt | 24 +++++++++++-- 6 files changed, 129 insertions(+), 6 deletions(-) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/MetaTagsParser.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/MetaTagsParser.kt index 6fefdf4dc2..5f4bd4cd4b 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/MetaTagsParser.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/MetaTagsParser.kt @@ -58,6 +58,7 @@ object MetaTagsParser { private const val META = "meta" private const val LINK = "link" private const val HEAD = "head" + private const val BODY = "body" private const val ICON = "icon" private const val REL = "rel" private const val CONTENT = "content" @@ -88,6 +89,12 @@ object MetaTagsParser { /** The `</head>` that ends the interesting part of the document. */ HEAD_END, + /** + * A `<body>` start tag. `</head>` is optional, so on a page that omits it this is the only + * sign the head is over. + */ + BODY_START, + /** A closed `<title>`: its text is [TagScanner.textStart]..<[TagScanner.textEnd]. */ TITLE, @@ -111,26 +118,34 @@ object MetaTagsParser { ): Sequence<MetaTag> = sequence { val s = TagScanner(input) + // Meta tags past an implicit end of head are still read, as they always were. The + // title and icons are not: past `<body>`, a `<title>` is an inline SVG's tooltip + // ("Close", "Menu"), not the page's name. + var inBody = false while (!s.exhausted()) { when (s.nextTag()) { TagKind.HEAD_END -> { break } + TagKind.BODY_START -> { + inBody = true + } + TagKind.META -> { val attrs = parseAttrs(input, s.attrsStart, s.attrsEnd) ?: continue yield(MetaTag(attrs)) } TagKind.TITLE -> { - if (includeTitleAndIcons) { + if (includeTitleAndIcons && !inBody) { val text = titleText(input, s.textStart, s.textEnd) if (text.isNotEmpty()) yield(MetaTag(mapOf(CONTENT to text), HeadElement.TITLE)) } } TagKind.LINK -> { - if (includeTitleAndIcons) { + if (includeTitleAndIcons && !inBody) { val attrs = parseAttrs(input, s.attrsStart, s.attrsEnd) ?: continue if (isIconRel(attrs[REL])) yield(MetaTag(attrs, HeadElement.LINK)) } @@ -160,7 +175,11 @@ object MetaTagsParser { ): String { val raw = input.substring(from, to) val decoded = if (raw.indexOf('&') < 0) raw else raw.replace(Attrs.RE_CHAR_REF, Attrs.Companion::replaceCharRefs) - return decoded.replace(WHITESPACE_RUN, " ").trim().take(MAX_TITLE_LENGTH) + val title = decoded.replace(WHITESPACE_RUN, " ").trim() + if (title.length <= MAX_TITLE_LENGTH) return title + // Never end on half of a surrogate pair: a lone high surrogate renders as tofu. + val cut = if (title[MAX_TITLE_LENGTH - 1].isHighSurrogate()) MAX_TITLE_LENGTH - 1 else MAX_TITLE_LENGTH + return title.substring(0, cut) } private class TagScanner( @@ -314,6 +333,7 @@ object MetaTagsParser { when (nameEnd - nameStart) { META.length -> { if (nameIs(nameStart, nameEnd, META)) return TagKind.META + if (nameIs(nameStart, nameEnd, BODY)) return TagKind.BODY_START // Most `<link>`s are stylesheets and preloads; checking the raw span for `icon` // keeps their attributes from ever being parsed into a map. if (nameIs(nameStart, nameEnd, LINK) && spanContains(attrsStart, attrsEnd, ICON)) return TagKind.LINK diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItem.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItem.kt index 00b24466cf..4977a6cd67 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItem.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItem.kt @@ -71,10 +71,20 @@ class UrlInfoItem( /** * The page's icon, absolute: the one it declared, else `/favicon.ico` at its origin -- the * same guess every browser makes, since most sites that declare nothing still serve one. + * + * A `data:` icon is used as-is when it is an image (the image loader decodes those inline), + * and taken at its word when it is not: `href="data:,"` is how a site tells browsers it has no + * favicon, so guessing `/favicon.ico` there is a request that is known to miss. + * * Null for a non-HTML URL (an image or a video is its own preview) or an unresolvable one. */ val iconUrlFullPath: String? = - if (mimeType.startsWith("text/html")) resolve(icon) ?: resolve(DEFAULT_FAVICON) else null + when { + !mimeType.startsWith("text/html", ignoreCase = true) -> null + icon.startsWith("data:image/", ignoreCase = true) -> if (icon.length <= MAX_INLINE_ICON_LENGTH) icon else resolve(DEFAULT_FAVICON) + icon.startsWith("data:", ignoreCase = true) -> null + else -> resolve(icon) ?: resolve(DEFAULT_FAVICON) + } /** * The declared `og:video`, absolute, but only when the declaration holds up: a [videoType] the @@ -149,5 +159,9 @@ class UrlInfoItem( companion object { private const val DEFAULT_FAVICON = "/favicon.ico" + + // Previews are cached, so an inline icon is held for as long as its card is. Real inline + // favicons are a few KB; anything far past that is not worth keeping in memory. + private const val MAX_INLINE_ICON_LENGTH = 64 * 1024 } } diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/preview/TitleAndIconFallbackTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/preview/TitleAndIconFallbackTest.kt index d0f9db83cc..4822fceaa4 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/preview/TitleAndIconFallbackTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/preview/TitleAndIconFallbackTest.kt @@ -176,4 +176,38 @@ class TitleAndIconFallbackTest { assertEquals("", info.icon) } + + @Test + fun aBodyTitleIsIgnoredWhenTheHeadIsNeverClosed() { + // `</head>` is optional: `<body>` closes the head implicitly. An inline SVG's <title> + // ("Close", "Menu") is the classic thing to then mistake for the page title. + val info = extract("""<head><meta name="description" content="D"><body><svg><title>Close""") + + assertEquals("", info.title) + assertEquals("D", info.description) + } + + @Test + fun aBodyIconLinkIsIgnoredWhenTheHeadIsNeverClosed() { + val info = extract("""T""") + + assertEquals("T", info.title) + assertEquals("", info.icon) + } + + @Test + fun aTitleWithAttributesIsRead() { + val info = extract("""Helmet""") + + assertEquals("Helmet", info.title) + } + + @Test + fun aLongTitleIsNotCutInsideASurrogatePair() { + // 299 ASCII chars then an emoji: a plain take(300) keeps only its high surrogate. + val info = extract("" + "a".repeat(299) + "\uD83D\uDE00 tail") + + assertTrue(info.title.isNotEmpty()) + assertTrue(!info.title.last().isHighSurrogate(), "title ends in a dangling high surrogate") + } } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItemTextPreviewTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItemTextPreviewTest.kt index eaeeeb96a4..5998927fbe 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItemTextPreviewTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/preview/UrlInfoItemTextPreviewTest.kt @@ -84,4 +84,26 @@ class UrlInfoItemTextPreviewTest { fun nonHtmlUrlsHaveNoIcon() { assertNull(page(url = "https://example.com/a.png", image = "https://example.com/a.png", mimeType = "image/png").iconUrlFullPath) } + + @Test + fun anUppercaseHtmlMimeStillGetsAnIcon() { + // UrlPreview stores MediaType.toString(), which keeps the server's spelling. + assertEquals( + "https://example.nsite.lol/favicon.ico", + page(mimeType = "Text/HTML; charset=UTF-8").iconUrlFullPath, + ) + } + + @Test + fun anEmptyDataIconMeansTheSiteHasNoIcon() { + // `` is the common way to tell browsers not to request + // /favicon.ico. Requesting it anyway is a guaranteed miss. + assertNull(page(icon = "data:,").iconUrlFullPath) + } + + @Test + fun anInlineImageIconIsUsedAsIs() { + val inline = "data:image/png;base64,iVBORw0KGgo=" + assertEquals(inline, page(icon = inline).iconUrlFullPath) + } } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/prodbench/MetaTagsParserBenchmark.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/prodbench/MetaTagsParserBenchmark.kt index 228498fdf1..2a2d0f70db 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/prodbench/MetaTagsParserBenchmark.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/prodbench/MetaTagsParserBenchmark.kt @@ -160,6 +160,19 @@ class MetaTagsParserBenchmark { } bench("no , whole doc", open, 2_000) { MetaTagsParser.parse(it).count() } + // What production runs (HtmlParser asks for the and icon <link>s too). Should sit + // on top of the rows above: the 60 preload links never reach the attribute parser. + bench("spa head, +title/icons", spa, 50_000) { MetaTagsParser.parse(it, includeTitleAndIcons = true).count() } + bench("spa head, og: + fallbacks", spa, 50_000) { + OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(it, includeTitleAndIcons = true)).title.length + } + bench("heavy head, +title/icons", heavy, 2_000) { MetaTagsParser.parse(it, includeTitleAndIcons = true).count() } + bench("no </head>, +title/icons", open, 2_000) { MetaTagsParser.parse(it, includeTitleAndIcons = true).count() } + + val spaFallbacks = OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(spa, includeTitleAndIcons = true)) + assertEquals("Example — Your Network. Your Rules.", spaFallbacks.title) + assertEquals("/favicon.svg", spaFallbacks.icon) + assertTrue(MetaTagsParser.parse(open).count() >= 64) } } diff --git a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/components/UrlPreviewCard.kt b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/components/UrlPreviewCard.kt index 59265eb13d..66853c6bac 100644 --- a/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/components/UrlPreviewCard.kt +++ b/commonsUI/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/ui/components/UrlPreviewCard.kt @@ -20,6 +20,7 @@ */ package com.vitorpamplona.amethyst.commons.ui.components +import androidx.collection.LruCache import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.background import androidx.compose.foundation.combinedClickable @@ -49,6 +50,7 @@ import androidx.compose.ui.platform.LocalUriHandler import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import coil3.compose.AsyncImage +import coil3.network.HttpException import com.vitorpamplona.amethyst.commons.icons.symbols.Icon import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.commons.preview.UrlInfoItem @@ -130,7 +132,15 @@ fun UrlPreviewCard( // Only meaningful when the card's own tap does something else (e.g. opening the comment // thread); otherwise it would duplicate the card's open-in-browser tap. - val onOpenInBrowser: (() -> Unit)? = onCardClick?.let { { runCatching { uri.openUri(url) } } } + val hasCardClick = onCardClick != null + val onOpenInBrowser: (() -> Unit)? = + remember(hasCardClick, url, uri) { + if (hasCardClick) { + { runCatching { uri.openUri(url) } } + } else { + null + } + } // A page with no picture to lead with -- most often one with no OpenGraph at all, previewed // from its `<title>`, meta description and favicon -- gets the short, wide card. Painting the @@ -251,6 +261,13 @@ private fun CompactUrlPreviewCard( } } +/** + * Icon URLs the server refused (404 and the like) in this process. The image loader caches + * successes but not failures, so without this a site with no `/favicon.ico` would be asked for it + * again -- and 404 again -- every time its card scrolled back into view. + */ +private val failedIconUrls = LruCache<String, Unit>(200) + /** * The site's icon in a fixed square. A generic globe sits there until the icon actually loads, * so a site whose favicon is missing (the `/favicon.ico` guess is only a guess) or undecodable @@ -278,13 +295,16 @@ private fun UrlPreviewIcon( ) } - if (iconUrl != null) { + if (iconUrl != null && failedIconUrls[iconUrl] == null) { AsyncImage( model = iconUrl, contentDescription = contentDescription, contentScale = ContentScale.Fit, modifier = UrlPreviewIconImageModifier, onSuccess = { loaded = true }, + // Only a server's answer is remembered. A network error (offline, a timeout) says + // nothing about the icon and must be retried once the connection is back. + onError = { if (it.result.throwable is HttpException) failedIconUrls.put(iconUrl, Unit) }, ) } }