From de7640a00688cff31470e44c3e43285ed7beb244 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Fri, 13 Nov 2026 21:11:56 -0500 Subject: [PATCH] feat(xml): preserve strict end-tag parsing as an option --- crates/dav/src/lib.rs | 9 ++++- crates/docx/src/xml.rs | 1 + crates/idml/src/part.rs | 10 ++++- crates/materialx/src/lib.rs | 9 ++++- crates/odf/src/text/tests_more/lossless.rs | 10 ++++- crates/odf/src/text/xml.rs | 10 ++++- crates/xml/src/lib.rs | 13 +++++- crates/xml/src/read.rs | 46 ++++++++++++++++----- crates/xml/tests/hostile.rs | 47 +++++++++++++++++++++- 9 files changed, 134 insertions(+), 21 deletions(-) diff --git a/crates/dav/src/lib.rs b/crates/dav/src/lib.rs index 87040feae..db137a050 100644 --- a/crates/dav/src/lib.rs +++ b/crates/dav/src/lib.rs @@ -74,7 +74,14 @@ pub fn parse_multistatus(bytes: &[u8]) -> Result { if !text.chars().all(craft_xml::is_xml_char) { return Err(Error::Characters); } - let limits = Limits { max_input: MAX_BYTES, max_depth: MAX_DEPTH, max_nodes: MAX_NODES, max_attrs: MAX_ATTRS, max_attrs_total: MAX_ATTRS_TOTAL }; + let limits = Limits { + max_input: MAX_BYTES, + max_depth: MAX_DEPTH, + max_nodes: MAX_NODES, + max_elements: MAX_NODES, + max_attrs: MAX_ATTRS, + max_attrs_total: MAX_ATTRS_TOTAL, + }; let options = Options { doctype: Doctype::Reject, ..Options::canonical(&CANONICAL).strict().with_limits(limits) }; let root = options.parse(bytes).map_err(Error::Xml)?; if root.name != "d:multistatus" { diff --git a/crates/docx/src/xml.rs b/crates/docx/src/xml.rs index ff953f43d..bb86755d9 100644 --- a/crates/docx/src/xml.rs +++ b/crates/docx/src/xml.rs @@ -373,6 +373,7 @@ fn parse_with(bytes: &[u8], extra: &[(&'static str, &str)], strict: bool) -> Res max_depth: MAX_DEPTH + 1, // Text stretches count as nodes too: two for each element. max_nodes: 2 * MAX_ELEMENTS, + max_elements: MAX_ELEMENTS, max_attrs: MAX_ATTRS, max_attrs_total: 2 * MAX_ELEMENTS, }; diff --git a/crates/idml/src/part.rs b/crates/idml/src/part.rs index f55ce4197..e8fd72d6b 100644 --- a/crates/idml/src/part.rs +++ b/crates/idml/src/part.rs @@ -4,8 +4,14 @@ use craft_xml::{DECL_STANDALONE, El, Local, Node, Options}; use crate::{DOM_VERSION, PACKAGING_NS}; -pub(crate) const XML_LIMITS: craft_xml::Limits = - craft_xml::Limits { max_input: 512 << 20, max_depth: 256, max_nodes: 8_000_000, max_attrs: 1024, max_attrs_total: 32_000_000 }; +pub(crate) const XML_LIMITS: craft_xml::Limits = craft_xml::Limits { + max_input: 512 << 20, + max_depth: 256, + max_nodes: 8_000_000, + max_elements: 8_000_000, + max_attrs: 1024, + max_attrs_total: 32_000_000, +}; /// The XML limits a part is read with: 512 MiB of input, 256 levels of nesting (InDesign's /// packages stay under a few dozen), 8,000,000 nodes and 32,000,000 attributes in one part. diff --git a/crates/materialx/src/lib.rs b/crates/materialx/src/lib.rs index d60db3eef..72afc8ca1 100644 --- a/crates/materialx/src/lib.rs +++ b/crates/materialx/src/lib.rs @@ -183,7 +183,14 @@ const OPENPBR_WRITABLE: &[(&str, InputKind)] = &[ impl Document { /// Parses a MaterialX document while enforcing size, nesting and node-count limits. pub fn parse(bytes: &[u8]) -> Result { - let limits = Limits { max_input: MAX_BYTES, max_depth: MAX_DEPTH, max_nodes: MAX_NODES, max_attrs: 256, max_attrs_total: 400_000 }; + let limits = Limits { + max_input: MAX_BYTES, + max_depth: MAX_DEPTH, + max_nodes: MAX_NODES, + max_elements: MAX_NODES, + max_attrs: 256, + max_attrs_total: 400_000, + }; let options = Options { doctype: Doctype::Reject, unknown_entity: UnknownEntity::Error, diff --git a/crates/odf/src/text/tests_more/lossless.rs b/crates/odf/src/text/tests_more/lossless.rs index 645c0b918..7d05e3491 100644 --- a/crates/odf/src/text/tests_more/lossless.rs +++ b/crates/odf/src/text/tests_more/lossless.rs @@ -345,8 +345,14 @@ const SAMPLES: [Sample; 18] = [ /// Whether `xml` is well-formed: every element closed, in order, once. fn well_formed(xml: &[u8]) -> bool { - let limits = - craft_xml::Limits { max_input: usize::MAX, max_depth: usize::MAX, max_nodes: usize::MAX, max_attrs: usize::MAX, max_attrs_total: usize::MAX }; + let limits = craft_xml::Limits { + max_input: usize::MAX, + max_depth: usize::MAX, + max_nodes: usize::MAX, + max_elements: usize::MAX, + max_attrs: usize::MAX, + max_attrs_total: usize::MAX, + }; craft_xml::Options::default().strict().with_limits(limits).parse(xml).is_ok() } diff --git a/crates/odf/src/text/xml.rs b/crates/odf/src/text/xml.rs index ef5a0e0d1..d96ee4917 100644 --- a/crates/odf/src/text/xml.rs +++ b/crates/odf/src/text/xml.rs @@ -45,8 +45,14 @@ pub(super) fn events(xml: &[u8], mut f: impl FnMut(Ev)) { pub(super) fn events_at(xml: &[u8], mut f: impl FnMut(Ev, (usize, usize))) { // Names and attributes as written; no limit but the input's own size (each part is at most a // package entry), and at most 128 attributes kept per element. - let limits = - craft_xml::Limits { max_input: usize::MAX, max_depth: usize::MAX, max_nodes: usize::MAX, max_attrs: usize::MAX, max_attrs_total: usize::MAX }; + let limits = craft_xml::Limits { + max_input: usize::MAX, + max_depth: usize::MAX, + max_nodes: usize::MAX, + max_elements: usize::MAX, + max_attrs: usize::MAX, + max_attrs_total: usize::MAX, + }; let opts = craft_xml::Options::default().with_limits(limits); let mut events = opts.events(xml); // Malformed XML ends the stream. diff --git a/crates/xml/src/lib.rs b/crates/xml/src/lib.rs index b953690bf..5d3d40f27 100644 --- a/crates/xml/src/lib.rs +++ b/crates/xml/src/lib.rs @@ -49,6 +49,9 @@ pub enum Error { /// The tree would hold more than [`Limits::max_nodes`] nodes. #[error("the XML holds more than {0} nodes")] TooManyNodes(usize), + /// The tree would hold more than [`Limits::max_elements`] elements. + #[error("the XML holds more than {0} elements")] + TooManyElements(usize), /// One start tag has more than [`Limits::max_attrs`] attributes. #[error("an XML element has more than {0} attributes")] TooManyAttrs(usize), @@ -76,6 +79,14 @@ pub enum Error { impl Error { /// Was a limit exceeded (as opposed to the markup being wrong)? pub fn is_limit(&self) -> bool { - matches!(self, Error::TooLarge(_) | Error::TooDeep(_) | Error::TooManyNodes(_) | Error::TooManyAttrs(_) | Error::TooManyAttrsTotal(_)) + matches!( + self, + Error::TooLarge(_) + | Error::TooDeep(_) + | Error::TooManyNodes(_) + | Error::TooManyElements(_) + | Error::TooManyAttrs(_) + | Error::TooManyAttrsTotal(_) + ) } } diff --git a/crates/xml/src/read.rs b/crates/xml/src/read.rs index e155099e8..a4cfdcd36 100644 --- a/crates/xml/src/read.rs +++ b/crates/xml/src/read.rs @@ -25,6 +25,10 @@ pub struct Limits { /// 2,000,000: a tree node costs about 72 bytes before its strings, and text lies between /// elements at most, so this is about a million elements of an indented document. pub max_nodes: usize, + /// Most elements kept in a tree at once, independent of text nodes. Elements handed to a + /// streaming callback leave the count with everything inside them. Default unlimited; the + /// node limit already bounds element count for callers using default limits. + pub max_elements: usize, /// Most attributes on one start tag, namespace declarations and malformed ones included. /// With [`Names::Canonical`] it is also the most namespace declarations in scope at once. /// Default 1024. @@ -38,8 +42,14 @@ pub struct Limits { } impl Limits { - pub const DEFAULT: Limits = - Limits { max_input: 256 * 1024 * 1024, max_depth: 256, max_nodes: 2_000_000, max_attrs: 1024, max_attrs_total: 4_000_000 }; + pub const DEFAULT: Limits = Limits { + max_input: 256 * 1024 * 1024, + max_depth: 256, + max_nodes: 2_000_000, + max_elements: usize::MAX, + max_attrs: 1024, + max_attrs_total: 4_000_000, + }; } impl Default for Limits { @@ -174,6 +184,11 @@ pub struct Options<'a> { /// malformed attribute is skipped, and bytes that are not UTF-8 become U+FFFD. Strict: each /// of those is an error, as are a second root element and text outside the root. pub strict: bool, + /// Require closing tags to match their opening tags, and reject unclosed elements at EOF, + /// while keeping the other lenient behaviors (including stopping after the first root). + /// This is useful when a format historically accepted a single-root prefix but rejected + /// malformed nesting. + pub strict_end_tags: bool, pub unknown_entity: UnknownEntity, pub doctype: Doctype, /// Keep CDATA sections as [`Node::CData`] instead of joining them to the text around them. @@ -197,6 +212,7 @@ impl Default for Options<'_> { names: Names::AsWritten, ns_decls: true, strict: false, + strict_end_tags: false, unknown_entity: UnknownEntity::Drop, doctype: Doctype::Ignore, cdata: false, @@ -271,7 +287,7 @@ impl<'a> Options<'a> { } let text = to_utf8(bytes); let mut events = Events::new(*self, &text, false); - let mut tree = Builder { opts: self, split, stack: Vec::new(), marks: Vec::new(), kept: 0, attrs: 0, root: None, at: 0 }; + let mut tree = Builder { opts: self, split, stack: Vec::new(), marks: Vec::new(), kept: 0, elements: 0, attrs: 0, root: None, at: 0 }; let mut err = tree.run(&mut events, &mut each).err(); // What was open when the read stopped goes into its parent as it is. while let Some(el) = tree.stack.pop() { @@ -443,9 +459,9 @@ impl<'o, 'i> Events<'o, 'i> { } Q::End(e) => { let Some(top) = self.open.last() else { - return if self.opts.strict { self.syntax("an end tag has no start tag") } else { Ok(None) }; + return if self.opts.strict || self.opts.strict_end_tags { self.syntax("an end tag has no start tag") } else { Ok(None) }; }; - if self.opts.strict && top.raw != e.name().as_ref() { + if (self.opts.strict || self.opts.strict_end_tags) && top.raw != e.name().as_ref() { return self.syntax(format!("the end tag `{}` does not match its start tag", String::from_utf8_lossy(e.name().as_ref()))); } self.end() @@ -485,7 +501,11 @@ impl<'o, 'i> Events<'o, 'i> { } Q::Decl(_) => return Ok(None), Q::Eof => { - return if self.opts.strict && !self.open.is_empty() { self.syntax("an element is not closed") } else { Ok(Some(None)) }; + return if (self.opts.strict || self.opts.strict_end_tags) && !self.open.is_empty() { + self.syntax("an element is not closed") + } else { + Ok(Some(None)) + }; } }; Ok(Some(Some(event))) @@ -614,7 +634,7 @@ impl<'o, 'i> Events<'o, 'i> { let keep_name = self.end_names || canonical.is_some_and(|c| c.element.is_some()); self.open.push(Open { scope, - raw: if self.opts.strict { raw.as_ref().to_vec() } else { Vec::new() }, + raw: if self.opts.strict || self.opts.strict_end_tags { raw.as_ref().to_vec() } else { Vec::new() }, name: if keep_name { name.clone() } else { String::new() }, }); Ok(Event::Start { name, attrs: out }) @@ -626,8 +646,9 @@ struct Builder<'b, 'o> { split: &'b [&'b str], stack: Vec, /// The node and attribute counts when each open element started. - marks: Vec<(usize, usize)>, + marks: Vec<(usize, usize, usize)>, kept: usize, + elements: usize, /// Attributes in the elements built so far and not handed out. attrs: usize, root: Option, @@ -686,7 +707,11 @@ impl Builder<'_, '_> { if self.root.is_some() { return Err(Error::Syntax { at: self.at, msg: "more than one root element".into() }.into()); } - self.marks.push((self.kept, self.attrs)); + self.marks.push((self.kept, self.attrs, self.elements)); + self.elements = self.elements.saturating_add(1); + if self.elements > self.opts.limits.max_elements { + return Err(Error::TooManyElements(self.opts.limits.max_elements).into()); + } self.attrs = self.attrs.saturating_add(attrs.len()); if self.attrs > self.opts.limits.max_attrs_total { return Err(Error::TooManyAttrsTotal(self.opts.limits.max_attrs_total).into()); @@ -710,7 +735,7 @@ impl Builder<'_, '_> { /// Attaches a complete element to its parent, hands it out, or makes it the root. fn finish>(&mut self, mut el: El, each: &mut impl FnMut(El) -> Result<(), E>) -> Result<(), E> { - let (mark, attrs_mark) = self.marks.pop().unwrap_or((0, 0)); + let (mark, attrs_mark, elements_mark) = self.marks.pop().unwrap_or((0, 0, 0)); if self.opts.drop_blank_text && el.children.iter().any(|n| matches!(n, Node::El(_))) { let is_text = |n: &Node| matches!(n, Node::Text(_)); if el.children.iter().all(|n| !is_text(n) || matches!(n, Node::Text(t) if t.trim().is_empty())) { @@ -727,6 +752,7 @@ impl Builder<'_, '_> { each(el)?; // The element and everything inside it have left the tree. self.kept = mark; + self.elements = elements_mark; self.attrs = attrs_mark; } else { self.push(Node::El(el))?; diff --git a/crates/xml/tests/hostile.rs b/crates/xml/tests/hostile.rs index 8535d5835..2d387519b 100644 --- a/crates/xml/tests/hostile.rs +++ b/crates/xml/tests/hostile.rs @@ -22,14 +22,40 @@ fn text_len(e: &El) -> usize { fn the_defaults_are_the_documented_ones() { assert_eq!( Limits::default(), - Limits { max_input: 256 * 1024 * 1024, max_depth: 256, max_nodes: 2_000_000, max_attrs: 1024, max_attrs_total: 4_000_000 } + Limits { + max_input: 256 * 1024 * 1024, + max_depth: 256, + max_nodes: 2_000_000, + max_elements: usize::MAX, + max_attrs: 1024, + max_attrs_total: 4_000_000 + } ); - for e in [Error::TooLarge(1), Error::TooDeep(1), Error::TooManyNodes(1), Error::TooManyAttrs(1), Error::TooManyAttrsTotal(1)] { + for e in [ + Error::TooLarge(1), + Error::TooDeep(1), + Error::TooManyNodes(1), + Error::TooManyElements(1), + Error::TooManyAttrs(1), + Error::TooManyAttrsTotal(1), + ] { assert!(e.is_limit(), "{e}"); } assert!(!Error::NoRoot.is_limit() && !Error::Doctype.is_limit()); } +#[test] +fn strict_end_tags_keep_first_root_and_entity_compatibility() { + let opts = Options { strict_end_tags: true, unknown_entity: UnknownEntity::Literal, doctype: Doctype::Ignore, ..Options::default() }; + let root = opts.parse(b"]>&unknown;").unwrap(); + assert_eq!(root.name, "a"); + assert_eq!(root.text().as_ref(), "&unknown;"); + + for malformed in [b"".as_slice(), b"".as_slice(), b"".as_slice()] { + assert!(opts.parse(malformed).is_err(), "{}", String::from_utf8_lossy(malformed)); + } +} + #[test] fn a_depth_bomb_is_refused_at_the_limit() { let limits = Limits { max_depth: 8, ..Limits::DEFAULT }; @@ -412,6 +438,23 @@ fn a_node_count_charges_a_budget_shared_by_several_parts() { assert_eq!(short.parse(part.as_bytes()), Err(Error::TooManyNodes(200))); } +#[test] +fn element_limits_are_independent_and_streamed_rows_release_them() { + let limited = Options::default().with_limits(Limits { max_elements: 2, ..Limits::DEFAULT }); + assert_eq!(limited.parse(b""), Err(Error::TooManyElements(2))); + + let streaming = Options::default().with_limits(Limits { max_elements: 3, ..Limits::DEFAULT }); + let mut rows = 0; + let root = streaming + .parse_streaming(b"", &["row"], |_| { + rows += 1; + Ok::<(), Error>(()) + }) + .unwrap(); + assert_eq!(rows, 2); + assert!(root.children.is_empty()); +} + /// A part that cannot be read to its end still gives what came before the failure. #[test] fn a_failed_streaming_read_returns_what_it_had() {