From cb5c52f5e6929d028d755f1b3cb7d4f202910cf1 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 14 Nov 2026 17:27:03 -0500 Subject: [PATCH] fix: validate XML attribute identities and preflight GIF frame limits --- crates/engine/src/cmd.rs | 2 + crates/engine/src/cmd/place/docx.rs | 7 ++- crates/engine/src/cmd/variablecmds/library.rs | 15 ++--- crates/engine/src/cmd/xmlattrs.rs | 55 +++++++++++++++++++ crates/engine/src/tests/textimport.rs | 2 + crates/engine/src/tests/variables.rs | 14 +++++ crates/trace/src/raster.rs | 5 ++ crates/trace/src/tests.rs | 1 + 8 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 crates/engine/src/cmd/xmlattrs.rs diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index 99b8e1ecdd..575909a7bd 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -97,6 +97,8 @@ pub(crate) mod symboledit; pub mod symbollib; pub(crate) mod tabs; mod textedit; +mod xmlattrs; + pub use textedit::effective_kerning; pub mod textstyles; pub(crate) mod textwrap; diff --git a/crates/engine/src/cmd/place/docx.rs b/crates/engine/src/cmd/place/docx.rs index 9fd06106a5..804e3fd6ae 100644 --- a/crates/engine/src/cmd/place/docx.rs +++ b/crates/engine/src/cmd/place/docx.rs @@ -167,10 +167,13 @@ fn parse(xml: &str, name: &str, cmd: &str) -> Result { ..Options::default() } .strict(); - opts.parse(xml.as_bytes()).map_err(|e| match e { + let doc = opts.parse(xml.as_bytes()).map_err(|e| match e { craft_xml::Error::TooDeep(_) => bad(cmd, format!("`{name}` in the Word document nests elements more than {MAX_NESTING} deep")), _ => bad(cmd, format!("`{name}` in the Word document is not well-formed XML: {e}")), - }) + })?; + crate::cmd::xmlattrs::validate(xml.as_bytes(), opts.limits) + .map_err(|e| bad(cmd, format!("`{name}` in the Word document is not well-formed XML: {e}")))?; + Ok(doc) } /// The part that the first relationship of part `source` (`""`: the package) whose type ends in diff --git a/crates/engine/src/cmd/variablecmds/library.rs b/crates/engine/src/cmd/variablecmds/library.rs index 6708a51630..44f298053e 100644 --- a/crates/engine/src/cmd/variablecmds/library.rs +++ b/crates/engine/src/cmd/variablecmds/library.rs @@ -199,7 +199,7 @@ impl Library { fn parse_library(xml: &str, cmd: &str) -> Result { let xml = without_doctype(xml, cmd)?; let opts = craft_xml::Options { - names: craft_xml::Names::Local, + names: craft_xml::Names::AsWritten, limits: craft_xml::Limits { max_depth: MAX_LIBRARY_NESTING, max_nodes: MAX_LIBRARY_NODES as usize, ..craft_xml::Limits::DEFAULT }, ..craft_xml::Options::default() } @@ -208,12 +208,13 @@ fn parse_library(xml: &str, cmd: &str) -> Result { craft_xml::Error::TooDeep(_) => bad(cmd, format!("the variable library nests elements more than {MAX_LIBRARY_NESTING} deep")), _ => bad(cmd, format!("the variable library is not well-formed XML: {e}")), })?; - let Some(root) = std::iter::once(&doc).chain(doc.descendants()).find(|n| n.is("variableSets")) else { + crate::cmd::xmlattrs::validate(xml.as_bytes(), opts.limits).map_err(|e| bad(cmd, format!("the variable library is not well-formed XML: {e}")))?; + let Some(root) = std::iter::once(&doc).chain(doc.descendants()).find(|n| n.is(craft_xml::Local("variableSets"))) else { return Err(bad(cmd, "the file holds no `variableSets` element")); }; - let sets: Vec<&craft_xml::El> = root.children("variableSet").collect(); + let sets: Vec<&craft_xml::El> = root.children(craft_xml::Local("variableSet")).collect(); let (mut text, mut other, mut declared, mut refs) = (Vec::new(), Vec::new(), BTreeSet::new(), Vec::new()); - for var in sets.iter().flat_map(|s| s.children("variables")).flat_map(|v| v.children("variable")) { + for var in sets.iter().flat_map(|s| s.children(craft_xml::Local("variables"))).flat_map(|v| v.children(craft_xml::Local("variable"))) { let name = named(var.attr("varName"), "varName", cmd)?; if !declared.insert(name.clone()) { return Err(bad(cmd, format!("variable `{name}` is declared twice"))); @@ -239,7 +240,7 @@ fn parse_library(xml: &str, cmd: &str) -> Result { } let text_names: BTreeSet<&str> = text.iter().map(String::as_str).collect(); let (mut datasets, mut seen) = (Vec::new(), BTreeSet::new()); - for row in sets.iter().flat_map(|s| s.children("sampleDataSets")).flat_map(|d| d.children("sampleDataSet")) { + for row in sets.iter().flat_map(|s| s.children(craft_xml::Local("sampleDataSets"))).flat_map(|d| d.children(craft_xml::Local("sampleDataSet"))) { let name = named(row.attr("dataSetName"), "dataSetName", cmd)?; if !seen.insert(name.clone()) { return Err(bad(cmd, format!("data set `{name}` is declared twice"))); @@ -249,7 +250,7 @@ fn parse_library(xml: &str, cmd: &str) -> Result { } let (mut values, mut held) = (BTreeMap::new(), BTreeSet::new()); for el in row.elements() { - let var = el.name.as_str(); + let var = el.local(); if !declared.contains(var) { return Err(bad(cmd, format!("data set `{name}` holds a value for `{var}`, which the library doesn't declare"))); } @@ -259,7 +260,7 @@ fn parse_library(xml: &str, cmd: &str) -> Result { if !text_names.contains(var) { continue; } - let paras: Vec = el.children("p").map(craft_xml::El::deep_text).collect(); + let paras: Vec = el.children(craft_xml::Local("p")).map(craft_xml::El::deep_text).collect(); if !paras.is_empty() { values.insert(var.to_string(), DataValue::Text(paras.join("\n"))); } diff --git a/crates/engine/src/cmd/xmlattrs.rs b/crates/engine/src/cmd/xmlattrs.rs new file mode 100644 index 0000000000..6dd2578fae --- /dev/null +++ b/crates/engine/src/cmd/xmlattrs.rs @@ -0,0 +1,55 @@ +//! Attribute uniqueness checks missing from the pinned shared XML reader's strict mode. +//! +//! Temporary consumer guard: parsing remains in craft-xml. Move this invariant into the +//! shared strict reader and remove this second event pass when that dependency is updated. + +/// Check names before canonical/local-name rewriting can collapse distinct attributes. +/// The event reader and namespace scope use the same caller-supplied resource caps. +pub(super) fn validate(xml: &[u8], limits: craft_xml::Limits) -> Result<(), String> { + use std::collections::BTreeSet; + + let opts = craft_xml::Options { limits, doctype: craft_xml::Doctype::Reject, ..Default::default() }.strict(); + let mut events = opts.events(xml); + let mut bindings: Vec<(String, String)> = Vec::new(); + let mut scopes = Vec::new(); + while let Some(event) = events.next().map_err(|e| e.to_string())? { + match event { + craft_xml::Event::Start { attrs, .. } => { + scopes.push(bindings.len()); + let mut raw = BTreeSet::new(); + for (key, value) in &attrs { + if !raw.insert(key.as_str()) { + return Err(format!("duplicate attribute `{key}`")); + } + if let Some(prefix) = key.strip_prefix("xmlns:") { + if bindings.len() >= limits.max_attrs { + return Err("too many in-scope namespace declarations".into()); + } + bindings.push((prefix.to_string(), value.clone())); + } + } + let mut expanded = BTreeSet::new(); + for (key, _) in &attrs { + if key == "xmlns" || key.starts_with("xmlns:") { + continue; + } + // Default namespaces do not apply to attributes. Preserve undeclared + // prefixes for standalone SVG library fragments, as the importer did. + let name = match key.split_once(':') { + Some(("xml", local)) => ("http://www.w3.org/XML/1998/namespace", local), + Some((prefix, local)) => { + bindings.iter().rev().find(|(p, _)| p == prefix).map_or(("", key.as_str()), |(_, uri)| (uri.as_str(), local)) + } + None => ("", key.as_str()), + }; + if !expanded.insert(name) { + return Err(format!("duplicate expanded attribute `{key}`")); + } + } + } + craft_xml::Event::End { .. } => bindings.truncate(scopes.pop().unwrap_or(0)), + _ => {} + } + } + Ok(()) +} diff --git a/crates/engine/src/tests/textimport.rs b/crates/engine/src/tests/textimport.rs index 88f613fe7a..cc89a8261b 100644 --- a/crates/engine/src/tests/textimport.rs +++ b/crates/engine/src/tests/textimport.rs @@ -471,6 +471,8 @@ fn malformed_docx_fails_cleanly() { for xml in [ format!(r#"partial text"#), document("A&missing;B"), + document("text"), + document("text"), ] { let e = place(&mut s, "a.docx", &store(&[("word/document.xml", xml.as_bytes())])).unwrap_err(); assert!(e.contains("not well-formed XML"), "{e}"); diff --git a/crates/engine/src/tests/variables.rs b/crates/engine/src/tests/variables.rs index 5539377dbe..5358a5e84e 100644 --- a/crates/engine/src/tests/variables.rs +++ b/crates/engine/src/tests/variables.rs @@ -2481,6 +2481,9 @@ fn malformed_variable_libraries_are_refused() { ("not well-formed XML", "".to_string()), ("not well-formed XML", ok.replace("

x

", "

A&missing;B

")), ("not well-formed XML", ok.replace("", "")), + ("not well-formed XML", ok.replace("varName=\"Name\"", "varName=\"Name\" varName=\"Other\"")), + ("not well-formed XML", ok.replace("".to_string()), ("declares no variables", "".to_string()), ("is declared twice", library(&[("Name", "textcontent"), ("Name", "textcontent")], &[])), @@ -3174,3 +3177,14 @@ fn an_svg_without_readable_variable_data_opens_its_art() { assert_eq!(vectorcraft_testkit::svg_shapes(&vectorcraft_svg::export(d, &Default::default())), 1, "the art opens"); } } + +#[test] +fn variable_library_attributes_keep_their_namespace() { + let mut s = session(); + let xml = library(&[("Name", "textcontent")], &[("A", &[("Name", &["x"])])]) + .replace("varName=\"Name\"", "xmlns:m=\"urn:metadata\" m:varName=\"Other\" varName=\"Name\""); + let loaded = s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap(); + assert_eq!(loaded["variables"], json!(["Name"])); + let sets = s.execute("dataset.list", &json!({})).unwrap(); + assert_eq!(sets["datasets"][0]["values"], json!({"Name": {"text": "x"}})); +} diff --git a/crates/trace/src/raster.rs b/crates/trace/src/raster.rs index 10a7918ab7..492b3b4eed 100644 --- a/crates/trace/src/raster.rs +++ b/crates/trace/src/raster.rs @@ -35,6 +35,11 @@ pub const MAX_SOURCE_PIXELS: u64 = 192 << 20; pub(crate) fn trace_header(bytes: &[u8], max_pixels: u64, max_alloc: u64) -> Result, TraceError> { use craft_image_codecs::{ChannelLayout, Format, Limits, PngDecodeRows, RasterColorType, RasterInspection}; let decode_err = |e: craft_image_codecs::CodecError| TraceError::Decode(e.to_string()); + // Refuse a GIF frame larger than its logical screen before inspection's + // plausibility check can turn the caller's size refusal into a decode error. + if gif_first_frame_pixels(bytes).is_some_and(|pixels| pixels > max_pixels) { + return Err(TraceError::TooLarge { width: 0, height: 0, max: max_pixels >> 20 }); + } if craft_image_codecs::detect(bytes) != Some(Format::Png) { return craft_image_codecs::inspect_raster(bytes).map_err(decode_err); } diff --git a/crates/trace/src/tests.rs b/crates/trace/src/tests.rs index c6a16f9170..85d423a6f4 100644 --- a/crates/trace/src/tests.rs +++ b/crates/trace/src/tests.rs @@ -3973,6 +3973,7 @@ fn a_gif_frame_larger_than_its_screen_is_refused_before_decoding() { gif.extend([2, 1, 0, 0, 0x3B]); assert!(matches!(Raster::decode(&gif), Err(TraceError::TooLarge { .. })), "{:?}", Raster::decode(&gif).err()); assert!(matches!(Raster::decode_to_fit(&gif), Err(TraceError::TooLarge { .. }))); + assert!(matches!(decode_alpha(&gif), Err(TraceError::TooLarge { .. }))); } // Synthetic PNG chunks for transparency/orientation regression fixtures.