From a94e4f8d19a98a62ce393a4cd5cce127f461dbbe Mon Sep 17 00:00:00 2001 From: Conor Bronsdon <120674402+conorbronsdon@users.noreply.github.com> Date: Sun, 5 Jul 2026 23:16:16 -0700 Subject: [PATCH] fix: harden against confirmed review findings Three verified hardening fixes to the liberal XML/feed parser, each with a regression test that fails on the pre-fix source. 1. Missing no longer drops the whole feed (feed.mojo). A dropped end tag between two items, or before the closing , silently parsed to zero items. Now flush the open item both when a new item/entry starts while one is open and whenever the item element leaves the stack via an ancestor close. feedparser recovers both; so do we. 2. Numeric character references to forbidden XML scalars (NUL, C0 controls, surrogates, out-of-range) are mapped to U+FFFD instead of emitted verbatim (xml_parser.mojo). A raw NUL/control byte can truncate or corrupt a downstream consumer. Strict mode raises. 3. Bounded the element-name stack (_MAX_DEPTH = 1024) (feed.mojo). An adversarial document with thousands of unclosed start tags grew the stack without bound and made every liberal end-tag reverse scan O(depth) -> O(n^2). Bounding the stack bounds both; parse time is now linear. Co-Authored-By: Claude --- src/feed/feed.mojo | 38 ++++++++++++++++++++++----- src/feed/xml_parser.mojo | 30 ++++++++++++++++++++++ test/test_feed.mojo | 54 +++++++++++++++++++++++++++++++++++++++ test/test_xml_parser.mojo | 15 +++++++++++ 4 files changed, 131 insertions(+), 6 deletions(-) diff --git a/src/feed/feed.mojo b/src/feed/feed.mojo index a22e518..06bd985 100644 --- a/src/feed/feed.mojo +++ b/src/feed/feed.mojo @@ -37,6 +37,13 @@ from feed.xml_parser import ( EVENT_EOF, ) +# Cap on the element-name stack. Real feeds nest only a handful deep; an +# adversarial document with thousands of unclosed start tags would +# otherwise grow the stack without bound (memory DoS) and make every +# liberal end-tag reverse scan O(depth), i.e. O(n^2) over the document. +# Bounding the stack bounds both. Chosen far above any legitimate feed. +comptime _MAX_DEPTH = 1024 + def _canonical_ns_prefix(uri: String) -> String: """Canonical prefix for a known namespace URI, or "" if unknown.""" @@ -150,7 +157,11 @@ def parse_feed(var source: String, *, strict: Bool = False) raises -> Feed: var canon = _canon_name(event.name, ns_map) event.name = canon^ ref name = event.name - stack.append(name.copy()) + # Refuse to grow past the cap — see _MAX_DEPTH. Beyond it the + # document is pathological, not a feed, and we trade fidelity + # for a hard bound on memory and per-tag scan cost. + if len(stack) < _MAX_DEPTH: + stack.append(name.copy()) var depth = len(stack) if depth == 1: if name == "rss": @@ -173,7 +184,13 @@ def parse_feed(var source: String, *, strict: Bool = False) raises -> Feed: # Hello world foo survives intact. if depth <= _field_depth(feed.kind, in_item, item_depth): text = String() - if not in_item and (name == "item" or name == "entry"): + if name == "item" or name == "entry": + # A new item/entry opening while one is still open means + # the previous item's was omitted. Flush it before + # starting the next so one missing end tag can't silently + # drop the whole feed (feedparser recovers both). + if in_item: + feed.items.append(item^) in_item = True item_depth = depth item = FeedItem() @@ -251,10 +268,10 @@ def parse_feed(var source: String, *, strict: Bool = False) raises -> Feed: if in_item: if name == "item" or name == "entry": - if effective_depth == item_depth: - feed.items.append(item^) - item = FeedItem() - in_item = False + # Flushing is handled uniformly below, keyed on the + # item element leaving the stack — so a missing + # closed by an ancestor flushes on the same path. + pass elif effective_depth == item_depth + 1: _assign_item_field( item, @@ -297,6 +314,15 @@ def parse_feed(var source: String, *, strict: Bool = False) raises -> Feed: elif name == "subtitle": _set_if_empty(feed.description, value) + # Flush a still-open item whenever its start element leaves the + # stack: its own (normal close) or an ancestor closing + # over an item whose was omitted (liberal recovery). + if in_item and effective_depth <= item_depth: + feed.items.append(item^) + item = FeedItem() + in_item = False + pub_date_authoritative = False + stack.shrink(idx) # Clear accumulation once a field-level element closes; a # nested child closing keeps the running text. diff --git a/src/feed/xml_parser.mojo b/src/feed/xml_parser.mojo index 795c2b9..42f903f 100644 --- a/src/feed/xml_parser.mojo +++ b/src/feed/xml_parser.mojo @@ -31,6 +31,26 @@ def _is_space(b: UInt8) -> Bool: return b == 0x20 or b == 0x09 or b == 0x0A or b == 0x0D +def _is_valid_xml_char(cp: Int) -> Bool: + """XML 1.0 Char production: the scalars a document may contain. + + Excludes NUL and the C0 control block (except tab, LF, CR), + surrogates, and out-of-range codepoints. A numeric character + reference to a forbidden scalar (e.g. `�`) must not be emitted + verbatim — a raw NUL/control byte can truncate or corrupt whatever + consumes the decoded text. + """ + if cp == 0x09 or cp == 0x0A or cp == 0x0D: + return True + if cp >= 0x20 and cp <= 0xD7FF: + return True + if cp >= 0xE000 and cp <= 0xFFFD: + return True + if cp >= 0x10000 and cp <= 0x10FFFF: + return True + return False + + def _append_codepoint(mut out: String, cp_in: Int): """UTF-8 encode a Unicode scalar value and append it to `out`. @@ -492,6 +512,16 @@ struct XmlPullParser(Copyable, Movable): out += String(StringSlice(unsafe_from_utf8=bytes[start:end])) out += String(";") return out^ + if not _is_valid_xml_char(cp): + # Well-formed number, forbidden target (NUL, a C0 control, + # a surrogate, or out of range). Strict rejects it; liberal + # substitutes U+FFFD rather than emit a raw control byte. + if self.strict: + raise self._strict_error( + "character reference to a codepoint XML forbids", + self.pos, + ) + cp = 0xFFFD _append_codepoint(out, cp) return out^ var name = String(StringSlice(unsafe_from_utf8=bytes[start:end])) diff --git a/test/test_feed.mojo b/test/test_feed.mojo index b3fd836..e5ebc68 100644 --- a/test/test_feed.mojo +++ b/test/test_feed.mojo @@ -128,6 +128,60 @@ def test_stray_end_tag_ignored() raises: assert_equal(feed.items[0].title, "One") +def test_missing_mid_item_close_recovers_both() raises: + # A dropped between two items must not swallow the feed: the + # next flushes the still-open one. feedparser recovers both. + var source: String = """T + Oneg1 + Twog2 + """ + var feed = parse_feed(source^) + assert_equal(len(feed.items), 2) + assert_equal(feed.items[0].title, "One") + assert_equal(feed.items[0].guid, "g1") + assert_equal(feed.items[1].title, "Two") + assert_equal(feed.items[1].guid, "g2") + + +def test_missing_last_item_close_recovers_via_ancestor() raises: + # The final is dropped; the enclosing closes over + # it. The item must still be flushed rather than silently discarded. + var source: String = """T + Onlyg9 + """ + var feed = parse_feed(source^) + assert_equal(len(feed.items), 1) + assert_equal(feed.items[0].title, "Only") + assert_equal(feed.items[0].guid, "g9") + + +def test_missing_entry_close_recovers_atom() raises: + # Same recovery for Atom : a dropped before the next. + var source: String = """ + Aa1 + Bb1 + """ + var feed = parse_feed(source^) + assert_equal(len(feed.items), 2) + assert_equal(feed.items[0].title, "A") + assert_equal(feed.items[1].title, "B") + + +def test_deep_unclosed_nesting_is_bounded() raises: + # Thousands of unclosed start tags plus stray end tags must not grow + # the name-stack without bound or make end-tag matching quadratic; + # parsing must complete and yield no bogus items. + var hostile = String('') + for i in range(4000): # well past _MAX_DEPTH (1024) + hostile += "" + for _ in range(4000): + hostile += "" + hostile += "" + var feed = parse_feed(hostile^) + assert_equal(feed.kind, String(KIND_RSS)) + assert_equal(len(feed.items), 0) + + def test_mixed_content_text_survives() raises: # Text before, inside, and after nested children must all be kept. var source: String = """ diff --git a/test/test_xml_parser.mojo b/test/test_xml_parser.mojo index 9b6f7a1..73aab57 100644 --- a/test/test_xml_parser.mojo +++ b/test/test_xml_parser.mojo @@ -80,6 +80,16 @@ def test_out_of_range_codepoint_becomes_replacement() raises: assert_equal(events[1].text, "��") +def test_forbidden_control_char_ref_becomes_replacement() raises: + # NUL and C0 controls are well-formed numeric refs but forbidden XML + # characters; they must decode to U+FFFD, never raw control bytes. + var events = _events("a�bcd") + assert_equal(events[1].text, "a�b�c�d") + # Tab/LF/CR remain legal control characters and pass through. + var ok = _events(" ") + assert_equal(ok[1].text, "\t\n\r") + + def test_entities_in_attributes() raises: var events = _events('') assert_equal(events[0].attrs["title"], "a & b") @@ -261,6 +271,11 @@ def test_strict_bare_ampersand() raises: _strict_events("fish & chips") +def test_strict_forbidden_char_ref() raises: + with assert_raises(contains="XML forbids"): + _strict_events("�") + + def test_strict_error_reports_location() raises: with assert_raises(contains="line 3"): _strict_events("\n\nx\n")