From b3d4cd924b759fad548384a634665bdc8e2c37f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 17:08:21 +0000 Subject: [PATCH 1/3] fix: stop HTML comments and script bodies from hiding og: meta tags MetaTagsParser's scanner treated `` like an element and ran the attribute quote tracker over its text. A comment holding an odd number of quote characters -- an apostrophe in "we don't" is enough -- left the scanner inside a phantom quoted attribute value, so every tag that followed was swallowed until the next matching quote character. brainstorm.world hits this: its head opens with a theme comment containing `don't`, `'dark'` and `'system'` (five apostrophes), and the scanner only resurfaced at the apostrophe in `manifest's`, several comments later. The whole og: block sat in between, so the parser saw 4 meta tags instead of 22 and none of them og:*. With no title/description/image, UrlInfoItem.fetchComplete() is false, UrlCachedPreviewer stores Empty and the note renders a bare link. Comments are now skipped to `-->`, declarations and processing instructions (``, ``) to the next `>` without quote tracking, and script/style bodies to their end tag -- `for (i = 0; i < n; i++)` and quotes in JS strings are raw text, not markup, and can hide the same way. Also guards a peek() past the end of a body truncated right after a `/`. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FumxeDJPEgPqX8mz3xkM6b --- .../commons/preview/MetaTagsParser.kt | 48 ++++- .../preview/MetaTagsParserCommentTest.kt | 168 ++++++++++++++++++ 2 files changed, 213 insertions(+), 3 deletions(-) create mode 100644 commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/preview/MetaTagsParserCommentTest.kt 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 77092cadb2..6293f8c4f4 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 @@ -33,6 +33,7 @@ data class MetaTag( } object MetaTagsParser { + private val TAG_NAME = Regex("""[0-9a-zA-Z]+""") private val NON_ATTR_NAME_CHARS = setOf(Char(0x0), '"', '\'', '>', '/') private val NON_UNQUOTED_ATTR_VALUE_CHARS = setOf('"', '\'', '=', '>', '<', '`') @@ -81,10 +82,42 @@ object MetaTagsParser { this.skipWhile { it.isWhitespace() } } + private fun skipComment() { + val end = input.indexOf("-->", p) + p = if (end < 0) input.length else end + 3 + } + + private fun skipToTagEnd() { + skipWhile { it != '>' } + if (!exhausted()) consume() + } + + /** Leaves [p] on the ``, `` and `` are not element markup, so the + // attribute-quote tracking below must not run over them. A comment holding an odd + // number of quote characters -- an apostrophe in "we don't", a lone `"` -- would + // otherwise leave the scanner inside a phantom quoted attribute value and make it + // swallow every tag that follows, until the next matching quote character. That is + // enough to hide a page's whole `` block from the preview. + if (peek() == '!' || peek() == '?') { + if (input.startsWith("!--", p)) { + skipComment() + } else { + skipToTagEnd() + } + return null + } // read tag name val isEnd = peek() == '/' @@ -105,7 +138,7 @@ object MetaTagsParser { val c = consume() when { // `/>` out of quote -> end of tag - quote == null && c == '/' && peek() == '>' -> { + quote == null && c == '/' && !exhausted() && peek() == '>' -> { consume() break } @@ -129,11 +162,20 @@ object MetaTagsParser { val attrsEnd = p - 1 val name = input.slice(nameStart.. + | + | + | + | + """.trimMargin() + + val metaTags = MetaTagsParser.parse(input).toList() + + assertEquals(2, metaTags.size) + assertEquals("Brainstorm", metaTags[0].attr("content")) + assertEquals("https://example.com/og-image.png", metaTags[1].attr("content")) + } + + @Test + fun metaTagsInsideCommentsAreNotParsed() { + val input = + """ + | + | + | + | + """.trimMargin() + + val metaTags = MetaTagsParser.parse(input).toList() + + assertEquals(1, metaTags.size) + assertEquals("Real Title", metaTags[0].attr("content")) + } + + @Test + fun scriptBodyDoesNotSwallowFollowingMetaTags() { + val input = + """ + | + | + | + | + """.trimMargin() + + val metaTags = MetaTagsParser.parse(input).toList() + + assertEquals(1, metaTags.size) + assertEquals("Real Title", metaTags[0].attr("content")) + } + + @Test + fun styleBodyDoesNotSwallowFollowingMetaTags() { + val input = + """ + | + | + | + | + """.trimMargin() + + val metaTags = MetaTagsParser.parse(input).toList() + + assertEquals(1, metaTags.size) + assertEquals("Real Title", metaTags[0].attr("content")) + } + + @Test + fun doctypeAndProcessingInstructionsAreSkipped() { + val input = + """ + | + | + | + | + | + """.trimMargin() + + val metaTags = MetaTagsParser.parse(input).toList() + + assertEquals(1, metaTags.size) + assertEquals("Real Title", metaTags[0].attr("content")) + } + + @Test + fun unterminatedCommentEndsTheDocument() { + val input = + """ + | + | + | + | + | Brainstorm - Web of Trust for Nostr + | + | + | + | + |
+ | + """.trimMargin() + + val info = OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(input)) + + assertEquals("Brainstorm - Your Network. Your Rules.", info.title) + assertEquals("The decentralized Web of Trust layer for Nostr.", info.description) + assertEquals("https://brainstorm.nosfabrica.com/og-image.png", info.image) + } +} From 39797b2191683d8761bedfac83288fb5fa283f4a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 19:58:08 +0000 Subject: [PATCH 2/3] perf: drop the regex and the per-tag allocations from the meta scan The tag-name check was a Regex match over a freshly cut substring, run for every `<` in the document. Both are gone: names are compared in place against the only four that matter (meta, head, script, style), ASCII-case-folded with `code or 0x20`, so a non-meta tag now costs zero allocations -- no substring, no Matcher, no RawTag. nextTag() reports a TagKind and leaves the attribute span as two indices; only a real `` gets read, and parseAttrs() reads that span straight out of the document instead of a copy of it. The rest of the scan got the same treatment: - `indexOf('<')` / `indexOf('>')` / `indexOf("-->")` instead of char-at-a-time predicate loops -- these are intrinsified and vectorized on the JVM. - `Set.contains` for the attribute character classes boxed a Char per character of every meta tag; they are `when` branches now. - one Pair, one Result and one lambda per attribute (`runCatching { add(Pair) }`) became a boolean-returning add -- a duplicate attribute no longer throws. - `toImmutableMap()` rebuilt a persistent map for every meta tag; the Attrs builder is discarded at freeze(), so its own map is already private. - the character-reference Regex only runs on values that contain an `&`. Measured on a comment-free head, where this and the previous implementation do identical work (same JVM, both warmed, `plainHead` corpus): 1.1 KB head, 10 metas 12.5 us -> 5.1 us ( 88 -> 215 MB/s) 28 KB head, 204 metas 267 us -> 116 us (105 -> 242 MB/s) MetaTagsParserBenchmark joins the prodbench suite as the guard, on corpora shaped like a Vite SPA head and a CMS head buried in analytics scripts: any site we preview picks the input, so the scan has to stay linear in it. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FumxeDJPEgPqX8mz3xkM6b --- .../commons/preview/MetaTagsParser.kt | 302 ++++++++++-------- .../prodbench/MetaTagsParserBenchmark.kt | 165 ++++++++++ 2 files changed, 340 insertions(+), 127 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/prodbench/MetaTagsParserBenchmark.kt 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 6293f8c4f4..baa7bc7568 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 @@ -21,7 +21,6 @@ package com.vitorpamplona.amethyst.commons.preview import com.vitorpamplona.amethyst.commons.util.codePointToChars -import kotlinx.collections.immutable.toImmutableMap data class MetaTag( private val attrs: Map, @@ -32,10 +31,37 @@ data class MetaTag( fun attr(name: String): String = attrs[name.lowercase()] ?: "" } +/** + * Extracts `` tags out of a (possibly partial) HTML document. + * + * This runs on every link preview, over bytes straight off the network, so the scan touches each + * character once and allocates nothing until an actual `` shows up: `<` is found with + * [String.indexOf], tag names are compared in place against the four names that matter, and only a + * meta tag's attribute span is ever handed to [parseAttrs]. + */ object MetaTagsParser { - private val TAG_NAME = Regex("""[0-9a-zA-Z]+""") - private val NON_ATTR_NAME_CHARS = setOf(Char(0x0), '"', '\'', '>', '/') - private val NON_UNQUOTED_ATTR_VALUE_CHARS = setOf('"', '\'', '=', '>', '<', '`') + private const val NO_QUOTE = ' ' + + private const val META = "meta" + private const val HEAD = "head" + private const val SCRIPT = "script" + private const val STYLE = "style" + + private const val SCRIPT_END = "` start tag: its attribute span is [TagScanner.attrsStart]..<[TagScanner.attrsEnd]. */ + META, + + /** The `` that ends the interesting part of the document. */ + HEAD_END, + + /** Anything else: other elements, comments, declarations, unparseable markup. */ + OTHER, + } /** * Lazily parse a partial HTML document and extract meta tags. @@ -44,141 +70,167 @@ object MetaTagsParser { sequence { val s = TagScanner(input) while (!s.exhausted()) { - val t = s.nextTag() ?: continue - if (t.name == "head" && t.isEnd) { - break - } - if (t.name == "meta") { - val attrs = parseAttrs(t.attrPart) ?: continue + val kind = s.nextTag() + if (kind == TagKind.HEAD_END) break + if (kind == TagKind.META) { + val attrs = parseAttrs(input, s.attrsStart, s.attrsEnd) ?: continue yield(MetaTag(attrs)) } } } - private data class RawTag( - val isEnd: Boolean, - val name: String, - val attrPart: String, - ) - private class TagScanner( private val input: String, ) { + private val length = input.length private var p = 0 - fun exhausted(): Boolean = p >= input.length + /** Attribute span of the tag [nextTag] last reported as [TagKind.META]. */ + var attrsStart = 0 + private set + var attrsEnd = 0 + private set - private fun peek(): Char = input[p] + fun exhausted(): Boolean = p >= length - private fun consume(): Char = input[p++] - - private fun skipWhile(pred: (Char) -> Boolean) { - while (!this.exhausted() && pred(this.peek())) { - this.consume() + /** + * True when `input[from..", p) - p = if (end < 0) input.length else end + 3 + val end = input.indexOf(COMMENT_END, p) + p = if (end < 0) length else end + COMMENT_END.length } private fun skipToTagEnd() { - skipWhile { it != '>' } - if (!exhausted()) consume() + val end = input.indexOf('>', p) + p = if (end < 0) length else end + 1 } /** Leaves [p] on the `= length) return TagKind.OTHER - // ``, `` and `` are not element markup, so the + // ``, `` and `` are not element markup, so the // attribute-quote tracking below must not run over them. A comment holding an odd // number of quote characters -- an apostrophe in "we don't", a lone `"` -- would // otherwise leave the scanner inside a phantom quoted attribute value and make it // swallow every tag that follows, until the next matching quote character. That is // enough to hide a page's whole `` block from the preview. - if (peek() == '!' || peek() == '?') { - if (input.startsWith("!--", p)) { + val first = input[p] + if (first == '!' || first == '?') { + if (input.startsWith(COMMENT_START, p)) { skipComment() } else { skipToTagEnd() } - return null + return TagKind.OTHER } - // read tag name - val isEnd = peek() == '/' - if (isEnd) { - consume() - } + // read the tag name + val isEnd = first == '/' + if (isEnd) p++ val nameStart = p - skipWhile { !it.isWhitespace() && it != '>' } + while (p < length && !input[p].isWhitespace() && input[p] != '>') p++ val nameEnd = p - // seek to start of attrs part - skipSpaces() - val attrsStart = p + // seek to the start of the attrs part + while (p < length && input[p].isWhitespace()) p++ + attrsStart = p - // skip until end of tag - var quote: Char? = null - while (!exhausted()) { - val c = consume() - when { - // `/>` out of quote -> end of tag - quote == null && c == '/' && !exhausted() && peek() == '>' -> { - consume() + // skip to the end of the tag, tracking quoted values so a `>` inside one doesn't end it + var i = p + var quote = NO_QUOTE + while (i < length) { + val c = input[i] + if (quote == NO_QUOTE) { + // `>` or `/>` out of quote -> end of tag + if (c == '>') { + i++ break } - - // `>` out of quote -> end of tag - quote == null && c == '>' -> { + if (c == '/' && i + 1 < length && input[i + 1] == '>') { + i += 2 break } - - // entering quote - quote == null && (c == '\'' || c == '"') -> { - quote = c - } - - // leaving quote - quote != null && c == quote -> { - quote = null - } + if (c == '"' || c == '\'') quote = c + } else if (c == quote) { + quote = NO_QUOTE } + i++ } - val attrsEnd = p - 1 + p = i + attrsEnd = i - 1 - val name = input.slice(nameStart..` because `Set.contains` boxes + // the char, once per attribute character of every meta tag. + private fun isNonAttrNameChar(c: Char): Boolean = + when (c) { + '\u0000', '"', '\'', '>', '/' -> true + else -> false + } + + private fun isNonUnquotedAttrValueChar(c: Char): Boolean = + when (c) { + '"', '\'', '=', '>', '<', '`' -> true + else -> false + } + // map of HTML element attribute name to its value, with additional logics: // - attribute names are matched in a case-insensitive manner // - attribute names never duplicate @@ -258,16 +310,20 @@ object MetaTagsParser { private val attrs = mutableMapOf() - fun add(attr: Pair) { - val name = attr.first.lowercase() - if (attrs.containsKey(name)) { - throw IllegalArgumentException("duplicated attribute name: $name") - } - val value = attr.second.replace(RE_CHAR_REF, Companion::replaceCharRefs) - attrs += Pair(name, value) + /** Adds an attribute, returning false if that name was already set (the first value wins). */ + fun add( + name: String, + value: String, + ): Boolean { + val key = name.lowercase() + if (attrs.containsKey(key)) return false + // Resolving character references is the expensive half of an attribute and almost no + // value has an `&` in it, so the scan for one pays for itself. + attrs[key] = if (value.indexOf('&') < 0) value else value.replace(RE_CHAR_REF, Companion::replaceCharRefs) + return true } - fun freeze(): Map = attrs.toImmutableMap() + fun freeze(): Map = attrs } private enum class State { @@ -278,16 +334,22 @@ object MetaTagsParser { SPACE, } - private fun parseAttrs(input: String): Map? { + /** Parses the attributes of a single tag, held in `input[from..? { val attrs = Attrs() var state = State.NAME - var nameBegin = 0 - var nameEnd = 0 - var valueBegin = 0 - var valueQuote: Char? = null + var nameBegin = from + var nameEnd = from + var valueBegin = from + var valueQuote = NO_QUOTE - input.forEachIndexed { i, c -> + for (i in from.. { when { @@ -301,7 +363,7 @@ object MetaTagsParser { state = State.BEFORE_EQ } - NON_ATTR_NAME_CHARS.contains(c) || c.isISOControl() || !c.isDefined() -> { + isNonAttrNameChar(c) || c.isISOControl() || !c.isDefined() -> { return null } } @@ -317,7 +379,7 @@ object MetaTagsParser { else -> { // if it is expecting = but gets another name, starts another property - runCatching { attrs.add(Pair(input.slice(nameBegin.. { valueBegin = i - valueQuote = null + valueQuote = NO_QUOTE state = State.VALUE } } } State.VALUE -> { - var attr: Pair? = null - if (valueQuote != null) { - if (c == valueQuote) { - attr = - Pair( - input.slice(nameBegin.. { - attr = - Pair( - input.slice(nameBegin.. valueEnd = i - i == input.length - 1 -> { - attr = - Pair( - input.slice(nameBegin.. valueEnd = i + 1 - NON_UNQUOTED_ATTR_VALUE_CHARS.contains(c) -> { - return null - } + isNonUnquotedAttrValueChar(c) -> return null } } - if (attr != null) { - runCatching { attrs.add(attr) }.getOrNull() ?: return null + if (valueEnd >= 0) { + val added = + attrs.add( + input.substring(nameBegin, nameEnd), + input.substring(valueBegin, valueEnd), + ) + if (!added) return null state = State.SPACE } } 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 new file mode 100644 index 0000000000..228498fdf1 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/prodbench/MetaTagsParserBenchmark.kt @@ -0,0 +1,165 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.prodbench + +import com.vitorpamplona.amethyst.commons.preview.MetaTagsParser +import com.vitorpamplona.amethyst.commons.preview.OpenGraphParser +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Measures the `` scan behind every link preview. + * + * Every URL in a rendered note can reach [MetaTagsParser] with a whole HTML document in hand, so + * the scan runs on user-visible paths with attacker-shaped input (any site can serve a 1 MB head). + * The numbers below are the guard against that: the parser must stay linear and allocation-light, + * scanning at hundreds of MB/s rather than degrading with page size. + * + * Deterministic and offline. Prints ns/op and MB/s; the assertions only check that the scan still + * finds the right tags, never wall time (CI machines vary). + */ +class MetaTagsParserBenchmark { + companion object { + /** The shape of a Vite/React SPA head: theme comment, inline script, then the og: block. */ + fun spaHead(): String = + """ + + + + + + + + Example - A Site + + + + + + + + + + + +
+ + """.trimIndent() + + /** + * A news/CMS head: [tags] worth of meta+link noise, analytics scripts, JSON-LD and + * boilerplate comments, with the og: block near the end -- the worst realistic ordering. + */ + fun heavyHead(tags: Int): String { + val sb = StringBuilder(64 * 1024) + sb.append("\n\n\n") + sb.append("\n") + repeat(tags) { i -> + sb.append("\n") + sb.append("\n") + sb.append("\n") + sb.append("\n") + sb.append("\n") + } + sb.append("\n") + sb.append("\n") + sb.append("\n") + sb.append("\n") + sb.append("\n\n") + // Body the parser must never reach: it stops at . + repeat(tags * 40) { i -> sb.append("

Paragraph $i with markup and \"quotes\".

\n") } + sb.append("\n\n") + return sb.toString() + } + + /** Same content, but with no `` to stop at -- the scan runs over the whole document. */ + fun unterminatedHead(tags: Int): String = heavyHead(tags).replace("", "") + + fun bench( + label: String, + input: String, + reps: Int, + op: (String) -> Int, + ) { + repeat(maxOf(reps / 4, 2)) { op(input) } // warmup + val t0 = System.nanoTime() + var sink = 0 + repeat(reps) { sink += op(input) } + val ns = (System.nanoTime() - t0) / reps + val mbps = input.length.toDouble() / ns * 1000.0 // bytes/ns -> MB/s + println( + String.format( + "%-34s %9d B %9d ns/op %8.1f MB/s (hits=%d)", + label, + input.length, + ns, + mbps, + sink / reps, + ), + ) + } + } + + @Test + fun metaScans() { + val spa = spaHead() + val heavy = heavyHead(60) + val open = unterminatedHead(60) + + // correctness first: a benchmark that finds nothing measures nothing + val spaInfo = OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(spa)) + assertEquals("Example — Your Network. Your Rules.", spaInfo.title) + assertEquals("https://example.com/og-image.png", spaInfo.image) + + val heavyInfo = OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(heavy)) + assertEquals("The Headline", heavyInfo.title) + assertEquals("https://example.com/lead.jpg", heavyInfo.image) + + // the body after is never scanned + assertEquals(1 + 60 + 3, MetaTagsParser.parse(heavy).count()) + + // Note: the two "heavy head" rows report MB/s over the whole document, of which only the + // ~46 KB head is actually scanned -- the scan stops at . The last row is the same + // page with no , i.e. what a hostile server can force us to read end to end. + println("MetaTagsParser") + bench("spa head, all tags", spa, 50_000) { MetaTagsParser.parse(it).count() } + bench("spa head, og: extraction", spa, 50_000) { + OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(it)).title.length + } + bench("heavy head, to ", heavy, 2_000) { MetaTagsParser.parse(it).count() } + bench("heavy head, og: extraction", heavy, 2_000) { + OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(it)).title.length + } + bench("no , whole doc", open, 2_000) { MetaTagsParser.parse(it).count() } + + assertTrue(MetaTagsParser.parse(open).count() >= 64) + } +} From a7209a70b6a909c57b5c495df3b98f3ab9d4a303 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 20:16:12 +0000 Subject: [PATCH 3/3] test: cover the meta-tag variants the suite never saw, and fix title/textarea Reviewing what the tests actually reach turned up one live defect and a suite that mostly did not run. The defect is the comment bug again, in the elements whose content is text rather than markup. `5 < 6, that's math` is ordinary HTML: the `<` opened a phantom tag, the apostrophe opened a phantom attribute value, and every tag after it -- the whole og: block -- was swallowed. Same for ` + | + | + """.trimMargin() + + val metaTags = MetaTagsParser.parse(input).toList() + + assertEquals(1, metaTags.size) + assertEquals("Real Title", metaTags[0].attr("content")) + } + + @Test + fun aSelfClosedScriptStillOpensRawText() { + // Deliberate, and what a browser does: `/` on a script start tag is ignored, so everything + // up to `` is script data. A page written this way shows nothing after it either. + // Pinned so that "fixing" it never turns a JS string into an og: tag. + val input = + """ + | + |