Ensure that qualified-name segments start with a valid start character (#35530)

* Add spec comments to various methods

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>

* Ensure that qualified-name segments start with a valid start character

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>

* Update WPT expectations

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>

---------

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>

components/script/dom/bindings/xmlname.rs

@@ -6,89 +6,11 @@
 
 use html5ever::{namespace_url, ns, LocalName, Namespace, Prefix};
 
-use crate::dom::bindings::error::{Error, ErrorResult, Fallible};
+use crate::dom::bindings::error::{Error, Fallible};
 use crate::dom::bindings::str::DOMString;
 
-/// Validate a qualified name. See <https://dom.spec.whatwg.org/#validate> for details.
-pub(crate) fn validate_qualified_name(qualified_name: &str) -> ErrorResult {
-    // Step 2.
-    match xml_name_type(qualified_name) {
-        XMLName::Invalid => Err(Error::InvalidCharacter),
-        XMLName::Name => Err(Error::InvalidCharacter), // see whatwg/dom#671
-        XMLName::QName => Ok(()),
-    }
-}
-
-/// Validate a namespace and qualified name and extract their parts.
-/// See <https://dom.spec.whatwg.org/#validate-and-extract> for details.
-pub(crate) fn validate_and_extract(
-    namespace: Option<DOMString>,
-    qualified_name: &str,
-) -> Fallible<(Namespace, Option<Prefix>, LocalName)> {
-    // Step 1.
-    let namespace = namespace_from_domstring(namespace);
-
-    // Step 2.
-    validate_qualified_name(qualified_name)?;
-
-    let colon = ':';
-
-    // Step 5.
-    let mut parts = qualified_name.splitn(2, colon);
-
-    let (maybe_prefix, local_name) = {
-        let maybe_prefix = parts.next();
-        let maybe_local_name = parts.next();
-
-        debug_assert!(parts.next().is_none());
-
-        if let Some(local_name) = maybe_local_name {
-            debug_assert!(!maybe_prefix.unwrap().is_empty());
-
-            (maybe_prefix, local_name)
-        } else {
-            (None, maybe_prefix.unwrap())
-        }
-    };
-
-    debug_assert!(!local_name.contains(colon));
-
-    match (namespace, maybe_prefix) {
-        (ns!(), Some(_)) => {
-            // Step 6.
-            Err(Error::Namespace)
-        },
-        (ref ns, Some("xml")) if ns != &ns!(xml) => {
-            // Step 7.
-            Err(Error::Namespace)
-        },
-        (ref ns, p) if ns != &ns!(xmlns) && (qualified_name == "xmlns" || p == Some("xmlns")) => {
-            // Step 8.
-            Err(Error::Namespace)
-        },
-        (ns!(xmlns), p) if qualified_name != "xmlns" && p != Some("xmlns") => {
-            // Step 9.
-            Err(Error::Namespace)
-        },
-        (ns, p) => {
-            // Step 10.
-            Ok((ns, p.map(Prefix::from), LocalName::from(local_name)))
-        },
-    }
-}
-
-/// Results of `xml_name_type`.
-#[derive(PartialEq)]
-#[allow(missing_docs)]
-pub(crate) enum XMLName {
-    QName,
-    Name,
-    Invalid,
-}
-
 /// Check if an element name is valid. See <http://www.w3.org/TR/xml/#NT-Name>
 /// for details.
-pub(crate) fn xml_name_type(name: &str) -> XMLName {
 fn is_valid_start(c: char) -> bool {
     matches!(c, ':' |
         'A'..='Z' |
@@ -119,47 +41,112 @@ pub(crate) fn xml_name_type(name: &str) -> XMLName {
             '\u{203F}'..='\u{2040}')
 }
 
-    let mut iter = name.chars();
+/// Validate a qualified name. See <https://dom.spec.whatwg.org/#validate> for details.
-    let mut non_qname_colons = false;
+///
-    let mut seen_colon = false;
+/// On success, this returns a tuple `(prefix, local name)`.
-    let mut last = match iter.next() {
+pub(crate) fn validate_and_extract_qualified_name(
-        None => return XMLName::Invalid,
+    qualified_name: &str,
-        Some(c) => {
+) -> Fallible<(Option<&str>, &str)> {
-            if !is_valid_start(c) {
+    if qualified_name.is_empty() {
-                return XMLName::Invalid;
+        // Qualified names must not be empty
+        return Err(Error::InvalidCharacter);
     }
+    let mut colon_offset = None;
+    let mut at_start_of_name = true;
+
+    for (byte_position, c) in qualified_name.char_indices() {
         if c == ':' {
-                non_qname_colons = true;
+            if colon_offset.is_some() {
+                // Qualified names must not contain more than one colon
+                return Err(Error::InvalidCharacter);
             }
-            c
+            colon_offset = Some(byte_position);
-        },
+            at_start_of_name = true;
+            continue;
+        }
+
+        if at_start_of_name {
+            if !is_valid_start(c) {
+                // Name segments must begin with a valid start character
+                return Err(Error::InvalidCharacter);
+            }
+            at_start_of_name = false;
+        } else if !is_valid_continuation(c) {
+            // Name segments must consist of valid characters
+            return Err(Error::InvalidCharacter);
+        }
+    }
+
+    let Some(colon_offset) = colon_offset else {
+        // Simple case: there is no prefix
+        return Ok((None, qualified_name));
     };
 
-    for c in iter {
+    let (prefix, local_name) = qualified_name.split_at(colon_offset);
-        if !is_valid_continuation(c) {
+    let local_name = &local_name[1..]; // Remove the colon
-            return XMLName::Invalid;
+
-        }
+    if prefix.is_empty() || local_name.is_empty() {
-        if c == ':' {
+        // Neither prefix nor local name can be empty
-            if seen_colon {
+        return Err(Error::InvalidCharacter);
-                non_qname_colons = true;
-            } else {
-                seen_colon = true;
-            }
-        }
-        last = c
     }
 
-    if last == ':' {
+    Ok((Some(prefix), local_name))
-        non_qname_colons = true
 }
 
-    if non_qname_colons {
+/// Validate a namespace and qualified name and extract their parts.
-        XMLName::Name
+/// See <https://dom.spec.whatwg.org/#validate-and-extract> for details.
-    } else {
+pub(crate) fn validate_and_extract(
-        XMLName::QName
+    namespace: Option<DOMString>,
+    qualified_name: &str,
+) -> Fallible<(Namespace, Option<Prefix>, LocalName)> {
+    // Step 1. If namespace is the empty string, then set it to null.
+    let namespace = namespace_from_domstring(namespace);
+
+    // Step 2. Validate qualifiedName.
+    // Step 3. Let prefix be null.
+    // Step 4. Let localName be qualifiedName.
+    // Step 5. If qualifiedName contains a U+003A (:):
+    // NOTE: validate_and_extract_qualified_name does all of these things for us, because
+    // it's easier to do them together
+    let (prefix, local_name) = validate_and_extract_qualified_name(qualified_name)?;
+    debug_assert!(!local_name.contains(':'));
+
+    match (namespace, prefix) {
+        (ns!(), Some(_)) => {
+            // Step 6. If prefix is non-null and namespace is null, then throw a "NamespaceError" DOMException.
+            Err(Error::Namespace)
+        },
+        (ref ns, Some("xml")) if ns != &ns!(xml) => {
+            // Step 7. If prefix is "xml" and namespace is not the XML namespace,
+            // then throw a "NamespaceError" DOMException.
+            Err(Error::Namespace)
+        },
+        (ref ns, p) if ns != &ns!(xmlns) && (qualified_name == "xmlns" || p == Some("xmlns")) => {
+            // Step 8. If either qualifiedName or prefix is "xmlns" and namespace is not the XMLNS namespace,
+            // then throw a "NamespaceError" DOMException.
+            Err(Error::Namespace)
+        },
+        (ns!(xmlns), p) if qualified_name != "xmlns" && p != Some("xmlns") => {
+            // Step 9. If namespace is the XMLNS namespace and neither qualifiedName nor prefix is "xmlns",
+            // then throw a "NamespaceError" DOMException.
+            Err(Error::Namespace)
+        },
+        (ns, p) => {
+            // Step 10. Return namespace, prefix, and localName.
+            Ok((ns, p.map(Prefix::from), LocalName::from(local_name)))
+        },
     }
 }
 
+pub(crate) fn matches_name_production(name: &str) -> bool {
+    let mut iter = name.chars();
+
+    if iter.next().is_none_or(|c| !is_valid_start(c)) {
+        return false;
+    }
+    iter.all(is_valid_continuation)
+}
+
 /// Convert a possibly-null URL to a namespace.
 ///
 /// If the URL is None, returns the empty namespace.

components/script/dom/document.rs

@@ -115,9 +115,8 @@ use crate::dom::bindings::str::{DOMString, USVString};
 use crate::dom::bindings::trace::{HashMapTracedValues, NoTrace};
 #[cfg(feature = "webgpu")]
 use crate::dom::bindings::weakref::WeakRef;
-use crate::dom::bindings::xmlname::XMLName::Invalid;
 use crate::dom::bindings::xmlname::{
-    namespace_from_domstring, validate_and_extract, xml_name_type,
+    matches_name_production, namespace_from_domstring, validate_and_extract,
 };
 use crate::dom::cdatasection::CDATASection;
 use crate::dom::clipboardevent::ClipboardEvent;
@@ -4835,17 +4834,19 @@ impl DocumentMethods<crate::DomTypeHolder> for Document {
         self.get_element_by_id(&Atom::from(id))
     }
 
-    // https://dom.spec.whatwg.org/#dom-document-createelement
+    /// <https://dom.spec.whatwg.org/#dom-document-createelement>
     fn CreateElement(
         &self,
         mut local_name: DOMString,
         options: StringOrElementCreationOptions,
         can_gc: CanGc,
     ) -> Fallible<DomRoot<Element>> {
-        if xml_name_type(&local_name) == Invalid {
+        // Step 1. If localName does not match the Name production, then throw an "InvalidCharacterError" DOMException.
+        if !matches_name_production(&local_name) {
             debug!("Not a valid element name");
             return Err(Error::InvalidCharacter);
         }
+
         if self.is_html_document {
             local_name.make_ascii_lowercase();
         }
@@ -4874,7 +4875,7 @@ impl DocumentMethods<crate::DomTypeHolder> for Document {
         ))
     }
 
-    // https://dom.spec.whatwg.org/#dom-document-createelementns
+    /// <https://dom.spec.whatwg.org/#dom-document-createelementns>
     fn CreateElementNS(
         &self,
         namespace: Option<DOMString>,
@@ -4882,7 +4883,12 @@ impl DocumentMethods<crate::DomTypeHolder> for Document {
         options: StringOrElementCreationOptions,
         can_gc: CanGc,
     ) -> Fallible<DomRoot<Element>> {
+        // Step 1. Let namespace, prefix, and localName be the result of passing namespace and qualifiedName
+        // to validate and extract.
         let (namespace, prefix, local_name) = validate_and_extract(namespace, &qualified_name)?;
+
+        // Step 2. Let is be null.
+        // Step 3. If options is a dictionary and options["is"] exists, then set is to it.
         let name = QualName::new(prefix, namespace, local_name);
         let is = match options {
             StringOrElementCreationOptions::String(_) => None,
@@ -4890,6 +4896,8 @@ impl DocumentMethods<crate::DomTypeHolder> for Document {
                 options.is.as_ref().map(|is| LocalName::from(&**is))
             },
         };
+
+        // Step 4. Return the result of creating an element given document, localName, namespace, prefix, is, and true.
         Ok(Element::create(
             name,
             is,
@@ -4901,9 +4909,11 @@ impl DocumentMethods<crate::DomTypeHolder> for Document {
         ))
     }
 
-    // https://dom.spec.whatwg.org/#dom-document-createattribute
+    /// <https://dom.spec.whatwg.org/#dom-document-createattribute>
     fn CreateAttribute(&self, mut local_name: DOMString, can_gc: CanGc) -> Fallible<DomRoot<Attr>> {
-        if xml_name_type(&local_name) == Invalid {
+        // Step 1. If localName does not match the Name production in XML,
+        // then throw an "InvalidCharacterError" DOMException.
+        if !matches_name_production(&local_name) {
             debug!("Not a valid element name");
             return Err(Error::InvalidCharacter);
         }
@@ -4989,8 +4999,8 @@ impl DocumentMethods<crate::DomTypeHolder> for Document {
         data: DOMString,
         can_gc: CanGc,
     ) -> Fallible<DomRoot<ProcessingInstruction>> {
-        // Step 1.
+        // Step 1. If target does not match the Name production, then throw an "InvalidCharacterError" DOMException.
-        if xml_name_type(&target) == Invalid {
+        if !matches_name_production(&target) {
             return Err(Error::InvalidCharacter);
         }
 

components/script/dom/domimplementation.rs

@@ -18,7 +18,9 @@ use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::reflector::{reflect_dom_object, Reflector};
 use crate::dom::bindings::root::{Dom, DomRoot};
 use crate::dom::bindings::str::DOMString;
-use crate::dom::bindings::xmlname::{namespace_from_domstring, validate_qualified_name};
+use crate::dom::bindings::xmlname::{
+    namespace_from_domstring, validate_and_extract_qualified_name,
+};
 use crate::dom::document::{Document, DocumentSource, HasBrowsingContext, IsHTMLDocument};
 use crate::dom::documenttype::DocumentType;
 use crate::dom::htmlbodyelement::HTMLBodyElement;
@@ -57,7 +59,7 @@ impl DOMImplementation {
 
 // https://dom.spec.whatwg.org/#domimplementation
 impl DOMImplementationMethods<crate::DomTypeHolder> for DOMImplementation {
-    // https://dom.spec.whatwg.org/#dom-domimplementation-createdocumenttype
+    /// <https://dom.spec.whatwg.org/#dom-domimplementation-createdocumenttype>
     fn CreateDocumentType(
         &self,
         qualified_name: DOMString,
@@ -65,7 +67,9 @@ impl DOMImplementationMethods<crate::DomTypeHolder> for DOMImplementation {
         sysid: DOMString,
         can_gc: CanGc,
     ) -> Fallible<DomRoot<DocumentType>> {
-        validate_qualified_name(&qualified_name)?;
+        // Step 1. Validate qualifiedName.
+        validate_and_extract_qualified_name(&qualified_name)?;
+
         Ok(DocumentType::new(
             qualified_name,
             Some(pubid),
@@ -75,7 +79,7 @@ impl DOMImplementationMethods<crate::DomTypeHolder> for DOMImplementation {
         ))
     }
 
-    // https://dom.spec.whatwg.org/#dom-domimplementation-createdocument
+    /// <https://dom.spec.whatwg.org/#dom-domimplementation-createdocument>
     fn CreateDocument(
         &self,
         maybe_namespace: Option<DOMString>,
@@ -107,7 +111,10 @@ impl DOMImplementationMethods<crate::DomTypeHolder> for DOMImplementation {
             loader,
             Some(self.document.insecure_requests_policy()),
         );
-        // Step 2-3.
+
+        // Step 2. Let element be null.
+        // Step 3. If qualifiedName is not the empty string, then set element to the result of running
+        // the internal createElementNS steps, given document, namespace, qualifiedName, and an empty dictionary.
         let maybe_elem = if qname.is_empty() {
             None
         } else {

components/script/dom/element.rs

@@ -88,9 +88,8 @@ use crate::dom::bindings::refcounted::{Trusted, TrustedPromise};
 use crate::dom::bindings::reflector::DomObject;
 use crate::dom::bindings::root::{Dom, DomRoot, LayoutDom, MutNullableDom};
 use crate::dom::bindings::str::{DOMString, USVString};
-use crate::dom::bindings::xmlname::XMLName::Invalid;
 use crate::dom::bindings::xmlname::{
-    namespace_from_domstring, validate_and_extract, xml_name_type,
+    matches_name_production, namespace_from_domstring, validate_and_extract,
 };
 use crate::dom::characterdata::CharacterData;
 use crate::dom::create::create_element;
@@ -1658,7 +1657,7 @@ impl Element {
         can_gc: CanGc,
     ) -> ErrorResult {
         // Step 1.
-        if let Invalid = xml_name_type(&name) {
+        if !matches_name_production(&name) {
             return Err(Error::InvalidCharacter);
         }
 
@@ -2309,7 +2308,7 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         can_gc: CanGc,
     ) -> Fallible<bool> {
         // Step 1.
-        if xml_name_type(&name) == Invalid {
+        if !matches_name_production(&name) {
             return Err(Error::InvalidCharacter);
         }
 
@@ -2349,10 +2348,11 @@ impl ElementMethods<crate::DomTypeHolder> for Element {
         }
     }
 
-    // https://dom.spec.whatwg.org/#dom-element-setattribute
+    /// <https://dom.spec.whatwg.org/#dom-element-setattribute>
     fn SetAttribute(&self, name: DOMString, value: DOMString, can_gc: CanGc) -> ErrorResult {
-        // Step 1.
+        // Step 1. If qualifiedName does not match the Name production in XML,
-        if xml_name_type(&name) == Invalid {
+        // then throw an "InvalidCharacterError" DOMException.
+        if !matches_name_production(&name) {
             return Err(Error::InvalidCharacter);
         }
 

tests/wpt/meta/dom/nodes/DOMImplementation-createDocument.html.ini

@@ -1,6 +0,0 @@
-[DOMImplementation-createDocument.html]
-  [createDocument test: "http://example.com/","a:0",null,"INVALID_CHARACTER_ERR"]
-    expected: FAIL
-
-  [createDocument test: "http://example.com/","a:̀",null,"INVALID_CHARACTER_ERR"]
-    expected: FAIL

tests/wpt/meta/dom/nodes/Document-createElementNS.html.ini

@@ -1,18 +0,0 @@
-[Document-createElementNS.html]
-  [createElementNS test in HTML document: "http://example.com/","a:0","INVALID_CHARACTER_ERR"]
-    expected: FAIL
-
-  [createElementNS test in XML document: "http://example.com/","a:0","INVALID_CHARACTER_ERR"]
-    expected: FAIL
-
-  [createElementNS test in XHTML document: "http://example.com/","a:0","INVALID_CHARACTER_ERR"]
-    expected: FAIL
-
-  [createElementNS test in HTML document: "http://example.com/","a:̀","INVALID_CHARACTER_ERR"]
-    expected: FAIL
-
-  [createElementNS test in XML document: "http://example.com/","a:̀","INVALID_CHARACTER_ERR"]
-    expected: FAIL
-
-  [createElementNS test in XHTML document: "http://example.com/","a:̀","INVALID_CHARACTER_ERR"]
-    expected: FAIL