minidom: Ensure there is no colon in name when creating element

Signed-off-by: Maxime “pep” Buquet <pep@bouah.net>

minidom-rs/src/element.rs

@@ -123,6 +123,14 @@ impl PartialEq for Element {
     }
 }
 
+fn ensure_no_prefix<S: AsRef<str>>(s: &S) -> Result<()> {
+    let name_parts = s.as_ref().split(':').collect::<Vec<&str>>();
+    match name_parts.len() {
+        1 => Ok(()),
+        _ => Err(Error::InvalidElement),
+    }
+}
+
 impl Element {
     fn new<P: Into<Prefixes>>(
         name: String,
@@ -132,8 +140,8 @@ impl Element {
         attributes: BTreeMap<String, String>,
         children: Vec<Node>,
     ) -> Element {
-        // TODO: Check that "name" doesn't contain ":". We've stopped accepting the "prefix:local"
+        ensure_no_prefix(&name).unwrap();
-        // format.
+        // TODO: Return Result<Element> instead.
         Element {
             name,
             namespace,
@@ -863,7 +871,7 @@ fn build_element<R: BufRead>(reader: &EventReader<R>, event: &BytesStart, prefix
         }
     };
 
-    let element = Element::new(
+    Ok(Element::new(
         name,
         namespace.clone(),
         // Note that this will always be Some(_) as we can't distinguish between the None case and
@@ -872,8 +880,7 @@ fn build_element<R: BufRead>(reader: &EventReader<R>, event: &BytesStart, prefix
         local_prefixes,
         attributes,
         Vec::new()
-    );
+    ))
-    Ok(element)
 }
 
 /// An iterator over references to child elements of an `Element`.

minidom-rs/src/error.rs

@@ -32,7 +32,7 @@ pub enum Error {
     /// An error which is returned when an element is closed when it shouldn't be
     InvalidElementClosed,
 
-    /// An error which is returned when an elemet's name contains more than one colon
+    /// An error which is returned when an elemet's name contains more colons than permitted
     InvalidElement,
 
     /// An error which is returned when an element being serialized doesn't contain a prefix

minidom-rs/src/tests.rs

@@ -77,8 +77,13 @@ fn test_real_data() {
         .append(correction)
         .build();
     let stream = Element::builder("stream", "http://etherx.jabber.org/streams")
-        .prefix(Some(String::from("stream")), "http://etherx.jabber.org/streams").unwrap()
+        .prefix(
-        .prefix(None, "jabber:client").unwrap()
+            Some(String::from("stream")),
+            "http://etherx.jabber.org/streams",
+        )
+        .unwrap()
+        .prefix(None, "jabber:client")
+        .unwrap()
         .append(message)
         .build();
     println!("{}", String::from(&stream));
@@ -109,8 +114,13 @@ fn test_real_data() {
         .append(pubsub)
         .build();
     let stream = Element::builder("stream", "http://etherx.jabber.org/streams")
-        .prefix(Some(String::from("stream")), "http://etherx.jabber.org/streams").unwrap()
+        .prefix(
-        .prefix(None, "jabber:client").unwrap()
+            Some(String::from("stream")),
+            "http://etherx.jabber.org/streams",
+        )
+        .unwrap()
+        .prefix(None, "jabber:client")
+        .unwrap()
         .append(iq)
         .build();
 
@@ -141,8 +151,10 @@ fn writer_with_decl_works() {
 #[test]
 fn writer_with_prefix() {
     let root = Element::builder("root", "ns1")
-        .prefix(Some(String::from("p1")), "ns1").unwrap()
+        .prefix(Some(String::from("p1")), "ns1")
-        .prefix(None, "ns2").unwrap()
+        .unwrap()
+        .prefix(None, "ns2")
+        .unwrap()
         .build();
     assert_eq!(String::from(&root),
         r#"<p1:root xmlns="ns2" xmlns:p1="ns1"/>"#,
@@ -161,18 +173,15 @@ fn writer_no_prefix_namespace() {
 #[test]
 fn writer_no_prefix_namespace_child() {
     let child = Element::builder("child", "ns1").build();
-    let root = Element::builder("root", "ns1")
+    let root = Element::builder("root", "ns1").append(child).build();
-        .append(child)
-        .build();
     // TODO: Same remark as `writer_no_prefix_namespace`.
     assert_eq!(String::from(&root), r#"<root xmlns="ns1"><child/></root>"#);
 
     let child = Element::builder("child", "ns2")
-        .prefix(None, "ns3").unwrap()
+        .prefix(None, "ns3")
-        .build();
+        .unwrap()
-    let root = Element::builder("root", "ns1")
-        .append(child)
         .build();
+    let root = Element::builder("root", "ns1").append(child).build();
     // TODO: Same remark as `writer_no_prefix_namespace`.
     assert_eq!(String::from(&root), r#"<root xmlns="ns1"><ns0:child xmlns:ns0="ns2" xmlns="ns3"/></root>"#);
 }
@@ -181,7 +190,8 @@ fn writer_no_prefix_namespace_child() {
 fn writer_prefix_namespace_child() {
     let child = Element::builder("child", "ns1").build();
     let root = Element::builder("root", "ns1")
-        .prefix(Some(String::from("p1")), "ns1").unwrap()
+        .prefix(Some(String::from("p1")), "ns1")
+        .unwrap()
         .append(child)
         .build();
     assert_eq!(String::from(&root), r#"<p1:root xmlns:p1="ns1"><p1:child/></p1:root>"#);
@@ -193,8 +203,10 @@ fn writer_with_prefix_deduplicate() {
         // .prefix(Some(String::from("p1")), "ns1")
         .build();
     let root = Element::builder("root", "ns1")
-        .prefix(Some(String::from("p1")), "ns1").unwrap()
+        .prefix(Some(String::from("p1")), "ns1")
-        .prefix(None, "ns2").unwrap()
+        .unwrap()
+        .prefix(None, "ns2")
+        .unwrap()
         .append(child)
         .build();
     assert_eq!(String::from(&root),
@@ -202,22 +214,20 @@ fn writer_with_prefix_deduplicate() {
     );
 
     // Ensure descendants don't just reuse ancestors' prefixes that have been shadowed in between
-    let grandchild = Element::builder("grandchild", "ns1")
+    let grandchild = Element::builder("grandchild", "ns1").build();
-        .build();
+    let child = Element::builder("child", "ns2").append(grandchild).build();
-    let child = Element::builder("child", "ns2")
+    let root = Element::builder("root", "ns1").append(child).build();
-        .append(grandchild)
+    assert_eq!(
-        .build();
+        String::from(&root),
-    let root = Element::builder("root", "ns1")
-        .append(child)
-        .build();
-    assert_eq!(String::from(&root),
         r#"<root xmlns="ns1"><child xmlns="ns2"><grandchild xmlns="ns1"/></child></root>"#,
     );
 }
 
 #[test]
 fn writer_escapes_attributes() {
-    let root = Element::builder("root", "ns1").attr("a", "\"Air\" quotes").build();
+    let root = Element::builder("root", "ns1")
+        .attr("a", "\"Air\" quotes")
+        .build();
     let mut writer = Vec::new();
     {
         root.write_to(&mut writer).unwrap();