From 6a516d287e3bf012e50340f3f47d4627e5cec5d0 Mon Sep 17 00:00:00 2001 From: nerdsane Date: Wed, 22 Jul 2026 00:22:58 -0500 Subject: [PATCH 1/2] Decode XML entity references when reading CSDL attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `attr_str` returned `Attribute::value` verbatim, which is the raw, still-escaped byte range. Any CSDL containing `&`, `"` or a numeric character reference in an attribute therefore parsed into the typed model as the literal escape text rather than the character it denotes. This is load-bearing for the emitter escaping that follows: with the emitter escaping correctly and the parser not unescaping, every parse/emit/parse cycle would add a layer (`&` -> `&` -> `&amp;`). The two changes are only correct together. No existing behaviour changes for the current corpus — no CSDL fixture in the repository contains an entity reference — so this commit is a no-op for the suite and is separated purely to keep the shared read-path change reviewable on its own. Refs ARN-237. Co-Authored-By: Claude Opus 4.8 --- crates/temper-spec/src/csdl/parser/xml.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/temper-spec/src/csdl/parser/xml.rs b/crates/temper-spec/src/csdl/parser/xml.rs index b8dc630f4..880611eff 100644 --- a/crates/temper-spec/src/csdl/parser/xml.rs +++ b/crates/temper-spec/src/csdl/parser/xml.rs @@ -38,12 +38,18 @@ pub(super) fn local_name_end(element: &BytesEnd) -> String { full.rsplit(':').next().unwrap_or(full).to_string() } +/// Read an attribute, decoding XML entity and character references. +/// +/// `Attribute::value` is the raw, still-escaped bytes. Returning those directly +/// would surface `&` and ` ` as literal text and make parse/emit/parse +/// double-escape, so the value is unescaped here. pub(super) fn attr_str(element: &BytesStart, name: &str) -> Option { element .attributes() .flatten() .find(|attribute| std::str::from_utf8(attribute.key.as_ref()).unwrap_or("") == name) - .and_then(|attribute| String::from_utf8(attribute.value.to_vec()).ok()) + .and_then(|attribute| attribute.unescape_value().ok()) + .map(|value| value.into_owned()) } pub(super) fn required_attr(element: &BytesStart, name: &str) -> Result { From a5e80530f43f89faa5e8cb405f83a56c4c2b627a Mon Sep 17 00:00:00 2001 From: nerdsane Date: Wed, 22 Jul 2026 00:23:38 -0500 Subject: [PATCH 2/2] Escape every attribute value emitted in CSDL metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `emit_csdl_xml` applied `xml_escape` to only 4 of roughly 30 interpolation sites. Names, types, namespaces, DefaultValue, navigation-binding Path/Target, action/function references and annotation terms were all written into attributes verbatim. Those values are agent- and user-influenced, so a `"` in an identifier closed the attribute and let the remainder of the value inject arbitrary markup — the same defect class as ARN-172. A property named Name"/> --- crates/temper-spec/src/csdl/emit.rs | 230 ++++++++------------ crates/temper-spec/src/csdl/emit_test.rs | 261 +++++++++++++++++++++++ 2 files changed, 353 insertions(+), 138 deletions(-) create mode 100644 crates/temper-spec/src/csdl/emit_test.rs diff --git a/crates/temper-spec/src/csdl/emit.rs b/crates/temper-spec/src/csdl/emit.rs index 1ea494e9e..a5a2d69ae 100644 --- a/crates/temper-spec/src/csdl/emit.rs +++ b/crates/temper-spec/src/csdl/emit.rs @@ -10,7 +10,7 @@ pub fn emit_csdl_xml(doc: &CsdlDocument) -> String { let mut out = String::from("\n"); out.push_str(&format!( "\n", - doc.version + xml_escape(&doc.version) )); out.push_str(" \n"); @@ -26,7 +26,7 @@ pub fn emit_csdl_xml(doc: &CsdlDocument) -> String { fn emit_schema(out: &mut String, schema: &Schema) { out.push_str(&format!( " \n", - schema.namespace + xml_escape(&schema.namespace) )); for term in &schema.terms { @@ -54,10 +54,11 @@ fn emit_schema(out: &mut String, schema: &Schema) { fn emit_term(out: &mut String, term: &Term) { out.push_str(&format!( " \n", et.name)); + out.push_str(&format!( + " \n", + xml_escape(&et.name) + )); for member in &et.members { if let Some(val) = member.value { out.push_str(&format!( " \n", - member.name, val + xml_escape(&member.name), + val )); } else { - out.push_str(&format!(" \n", member.name)); + out.push_str(&format!( + " \n", + xml_escape(&member.name) + )); } } out.push_str(" \n"); @@ -84,17 +92,23 @@ fn emit_entity_type(out: &mut String, et: &EntityType) { if et.has_stream { out.push_str(&format!( " \n", - et.name + xml_escape(&et.name) )); } else { - out.push_str(&format!(" \n", et.name)); + out.push_str(&format!( + " \n", + xml_escape(&et.name) + )); } // Key if !et.key_properties.is_empty() { out.push_str(" \n"); for key in &et.key_properties { - out.push_str(&format!(" \n")); + out.push_str(&format!( + " \n", + xml_escape(key) + )); } out.push_str(" \n"); } @@ -120,13 +134,14 @@ fn emit_entity_type(out: &mut String, et: &EntityType) { fn emit_property(out: &mut String, prop: &Property) { out.push_str(&format!( " \n", - rc.property, rc.referenced_property + xml_escape(&rc.property), + xml_escape(&rc.referenced_property) )); } out.push_str(" \n"); @@ -170,7 +187,10 @@ fn emit_action(out: &mut String, action: &Action) { || action.return_type.is_some() || !action.annotations.is_empty(); - out.push_str(&format!(" \n"); } fn emit_return_type(out: &mut String, rt: &ReturnType) { - out.push_str(&format!(" \n", - ai.name, ai.action + xml_escape(&ai.name), + xml_escape(&ai.action) )); } for fi in &container.function_imports { out.push_str(&format!( " \n", - fi.name, fi.function + xml_escape(&fi.name), + xml_escape(&fi.function) )); } out.push_str(" \n"); @@ -283,17 +312,20 @@ fn emit_entity_set(out: &mut String, es: &EntitySet) { if es.navigation_bindings.is_empty() { out.push_str(&format!( " \n", - es.name, es.entity_type + xml_escape(&es.name), + xml_escape(&es.entity_type) )); } else { out.push_str(&format!( " \n", - es.name, es.entity_type + xml_escape(&es.name), + xml_escape(&es.entity_type) )); for nb in &es.navigation_bindings { out.push_str(&format!( " \n", - nb.path, nb.target + xml_escape(&nb.path), + xml_escape(&nb.target) )); } out.push_str(" \n"); @@ -302,34 +334,35 @@ fn emit_entity_set(out: &mut String, es: &EntitySet) { fn emit_annotation(out: &mut String, ann: &Annotation, indent: usize) { let pad: String = " ".repeat(indent); + let term = xml_escape(&ann.term); match &ann.value { AnnotationValue::String(s) => { out.push_str(&format!( "{pad}\n", - ann.term, + term, xml_escape(s) )); } AnnotationValue::Float(f) => { out.push_str(&format!( "{pad}\n", - ann.term + term )); } AnnotationValue::Bool(b) => { out.push_str(&format!( "{pad}\n", - ann.term + term )); } AnnotationValue::Int(i) => { out.push_str(&format!( "{pad}\n", - ann.term + term )); } AnnotationValue::Collection(items) => { - out.push_str(&format!("{pad}\n", ann.term)); + out.push_str(&format!("{pad}\n")); out.push_str(&format!("{pad} \n")); for item in items { out.push_str(&format!("{pad} {}\n", xml_escape(item))); @@ -338,11 +371,12 @@ fn emit_annotation(out: &mut String, ann: &Annotation, indent: usize) { out.push_str(&format!("{pad}\n")); } AnnotationValue::Record(map) => { - out.push_str(&format!("{pad}\n", ann.term)); + out.push_str(&format!("{pad}\n")); out.push_str(&format!("{pad} \n")); for (k, v) in map { out.push_str(&format!( - "{pad} \n", + "{pad} \n", + xml_escape(k), xml_escape(v) )); } @@ -353,113 +387,33 @@ fn emit_annotation(out: &mut String, ann: &Annotation, indent: usize) { } /// Escape XML special characters in attribute/text values. +/// +/// Every value interpolated into emitted CSDL must pass through this function — +/// identifiers included. Names, types, and references are agent- or +/// user-influenced, so an unescaped `"` there closes the attribute and lets the +/// value inject arbitrary markup. +/// +/// Tab, newline, and carriage return are escaped as character references +/// because XML attribute-value normalization would otherwise replace them with +/// spaces, silently changing the value on the way back in. fn xml_escape(s: &str) -> String { - s.replace('&', "&") - .replace('<', "<") - .replace('>', ">") - .replace('"', """) - .replace('\'', "'") -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::csdl::parse_csdl; - - #[test] - fn emit_round_trips_minimal_csdl() { - let xml = r#" - - - - - - - - - - - - - - "#; - - let doc = parse_csdl(xml).unwrap(); - let emitted = emit_csdl_xml(&doc); - - // Parse the emitted XML back and verify structure is preserved. - let doc2 = parse_csdl(&emitted).expect("emitted XML should re-parse"); - assert_eq!(doc2.version, "4.0"); - assert_eq!(doc2.schemas.len(), 1); - let schema = &doc2.schemas[0]; - assert_eq!(schema.namespace, "Test"); - assert_eq!(schema.entity_types.len(), 1); - assert_eq!(schema.entity_types[0].name, "Widget"); - assert_eq!(schema.entity_types[0].key_properties, vec!["Id"]); - assert_eq!(schema.entity_types[0].properties.len(), 2); - assert_eq!(schema.entity_containers.len(), 1); - assert_eq!(schema.entity_containers[0].entity_sets.len(), 1); - assert_eq!( - schema.entity_containers[0].entity_sets[0].entity_type, - "Test.Widget" - ); - } - - #[test] - fn emit_round_trips_has_stream() { - let xml = r#" - - - - - - - - - - - - - - - "#; - - let doc = parse_csdl(xml).unwrap(); - let schema = &doc.schemas[0]; - - let media = schema.entity_type("MediaFile").unwrap(); - assert!(media.has_stream, "MediaFile should have has_stream=true"); - - let regular = schema.entity_type("RegularEntity").unwrap(); - assert!( - !regular.has_stream, - "RegularEntity should have has_stream=false" - ); - - // Round-trip - let emitted = emit_csdl_xml(&doc); - let doc2 = parse_csdl(&emitted).unwrap(); - let schema2 = &doc2.schemas[0]; - - assert!(schema2.entity_type("MediaFile").unwrap().has_stream); - assert!(!schema2.entity_type("RegularEntity").unwrap().has_stream); - } - - #[test] - fn emit_round_trips_reference_csdl() { - let xml = include_str!("../../../../test-fixtures/specs/model.csdl.xml"); - let doc = parse_csdl(xml).unwrap(); - let emitted = emit_csdl_xml(&doc); - - let doc2 = parse_csdl(&emitted).expect("emitted reference CSDL should re-parse"); - assert_eq!(doc2.schemas.len(), doc.schemas.len()); - - // Verify entity types are preserved. - for (s1, s2) in doc.schemas.iter().zip(doc2.schemas.iter()) { - assert_eq!(s1.namespace, s2.namespace); - assert_eq!(s1.entity_types.len(), s2.entity_types.len()); - assert_eq!(s1.actions.len(), s2.actions.len()); - assert_eq!(s1.entity_containers.len(), s2.entity_containers.len()); + let mut out = String::with_capacity(s.len()); + for ch in s.chars() { + match ch { + '&' => out.push_str("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + '\'' => out.push_str("'"), + '\t' => out.push_str(" "), + '\n' => out.push_str(" "), + '\r' => out.push_str(" "), + _ => out.push(ch), } } + out } + +#[cfg(test)] +#[path = "emit_test.rs"] +mod tests; diff --git a/crates/temper-spec/src/csdl/emit_test.rs b/crates/temper-spec/src/csdl/emit_test.rs new file mode 100644 index 000000000..1880fd3a2 --- /dev/null +++ b/crates/temper-spec/src/csdl/emit_test.rs @@ -0,0 +1,261 @@ +use super::*; +use crate::csdl::parse_csdl; + +#[test] +fn emit_round_trips_minimal_csdl() { + let xml = r#" + + + + + + + + + + + + + + "#; + + let doc = parse_csdl(xml).unwrap(); + let emitted = emit_csdl_xml(&doc); + + // Parse the emitted XML back and verify structure is preserved. + let doc2 = parse_csdl(&emitted).expect("emitted XML should re-parse"); + assert_eq!(doc2.version, "4.0"); + assert_eq!(doc2.schemas.len(), 1); + let schema = &doc2.schemas[0]; + assert_eq!(schema.namespace, "Test"); + assert_eq!(schema.entity_types.len(), 1); + assert_eq!(schema.entity_types[0].name, "Widget"); + assert_eq!(schema.entity_types[0].key_properties, vec!["Id"]); + assert_eq!(schema.entity_types[0].properties.len(), 2); + assert_eq!(schema.entity_containers.len(), 1); + assert_eq!(schema.entity_containers[0].entity_sets.len(), 1); + assert_eq!( + schema.entity_containers[0].entity_sets[0].entity_type, + "Test.Widget" + ); +} + +#[test] +fn emit_round_trips_has_stream() { + let xml = r#" + + + + + + + + + + + + + + + "#; + + let doc = parse_csdl(xml).unwrap(); + let schema = &doc.schemas[0]; + + let media = schema.entity_type("MediaFile").unwrap(); + assert!(media.has_stream, "MediaFile should have has_stream=true"); + + let regular = schema.entity_type("RegularEntity").unwrap(); + assert!( + !regular.has_stream, + "RegularEntity should have has_stream=false" + ); + + // Round-trip + let emitted = emit_csdl_xml(&doc); + let doc2 = parse_csdl(&emitted).unwrap(); + let schema2 = &doc2.schemas[0]; + + assert!(schema2.entity_type("MediaFile").unwrap().has_stream); + assert!(!schema2.entity_type("RegularEntity").unwrap().has_stream); +} + +/// A property name carrying a quote must not be able to close the attribute +/// and inject markup of its own. +#[test] +fn adversarial_identifiers_do_not_inject_markup() { + let doc = CsdlDocument { + version: "4.0".to_string(), + schemas: vec![Schema { + namespace: "Ns\">".to_string()], + properties: vec![Property { + name: "Name\"/>".to_string()), + }], + has_stream: false, + }], + enum_types: Vec::new(), + actions: Vec::new(), + functions: Vec::new(), + entity_containers: vec![EntityContainer { + name: "Svc\">".to_string(), + entity_type: "Test.Widget\">".to_string(), + navigation_bindings: vec![NavigationBinding { + path: "Path\">".to_string(), + target: "Target\">".to_string(), + }], + }], + action_imports: vec![ActionImport { + name: "DoIt\">".to_string(), + action: "Test.DoIt\">".to_string(), + }], + function_imports: vec![FunctionImport { + name: "GetIt\">".to_string(), + function: "Test.GetIt\">".to_string(), + }], + }], + terms: Vec::new(), + }], + }; + + let emitted = emit_csdl_xml(&doc); + // The adversarial substrings may legitimately appear *escaped* inside an + // attribute value; what must never appear is live markup. + assert!( + !emitted.contains(""), + "namespace escaped its attribute:\n{emitted}" + ); + assert!( + !emitted.contains("