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>

4 files changed, 141 insertions, 137 deletions

diff --git a/components/script/dom/bindings/xmlname.rs b/components/script/dom/bindings/xmlname.rs
index 6a859f2e5eb..fa72e7c4c92 100644
--- a/components/script/dom/bindings/xmlname.rs
+++ b/components/script/dom/bindings/xmlname.rs
@@ -6,17 +6,91 @@
 
 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;
 
+/// Check if an element name is valid. See <http://www.w3.org/TR/xml/#NT-Name>
+/// for details.
+fn is_valid_start(c: char) -> bool {
+    matches!(c, ':' |
+        'A'..='Z' |
+        '_' |
+        'a'..='z' |
+        '\u{C0}'..='\u{D6}' |
+        '\u{D8}'..='\u{F6}' |
+        '\u{F8}'..='\u{2FF}' |
+        '\u{370}'..='\u{37D}' |
+        '\u{37F}'..='\u{1FFF}' |
+        '\u{200C}'..='\u{200D}' |
+        '\u{2070}'..='\u{218F}' |
+        '\u{2C00}'..='\u{2FEF}' |
+        '\u{3001}'..='\u{D7FF}' |
+        '\u{F900}'..='\u{FDCF}' |
+        '\u{FDF0}'..='\u{FFFD}' |
+        '\u{10000}'..='\u{EFFFF}')
+}
+
+fn is_valid_continuation(c: char) -> bool {
+    is_valid_start(c) ||
+        matches!(c,
+            '-' |
+            '.' |
+            '0'..='9' |
+            '\u{B7}' |
+            '\u{300}'..='\u{36F}' |
+            '\u{203F}'..='\u{2040}')
+}
+
 /// 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(()),
+///
+/// On success, this returns a tuple `(prefix, local name)`.
+pub(crate) fn validate_and_extract_qualified_name(
+    qualified_name: &str,
+) -> Fallible<(Option<&str>, &str)> {
+    if qualified_name.is_empty() {
+        // 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 == ':' {
+            if colon_offset.is_some() {
+                // Qualified names must not contain more than one colon
+                return Err(Error::InvalidCharacter);
+            }
+            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));
+    };
+
+    let (prefix, local_name) = qualified_name.split_at(colon_offset);
+    let local_name = &local_name[1..]; // Remove the colon
+
+    if prefix.is_empty() || local_name.is_empty() {
+        // Neither prefix nor local name can be empty
+        return Err(Error::InvalidCharacter);
+    }
+
+    Ok((Some(prefix), local_name))
 }
 
 /// Validate a namespace and qualified name and extract their parts.
@@ -25,139 +99,52 @@ pub(crate) fn validate_and_extract(
     namespace: Option<DOMString>,
     qualified_name: &str,
 ) -> Fallible<(Namespace, Option<Prefix>, LocalName)> {
-    // Step 1.
+    // Step 1. If namespace is the empty string, then set it to null.
     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());
+    // 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(':'));
 
-        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) {
+    match (namespace, prefix) {
         (ns!(), Some(_)) => {
-            // Step 6.
+            // 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.
+            // 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.
+            // 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.
+            // 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.
+            // Step 10. Return namespace, prefix, and localName.
             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' |
-            '_' |
-            'a'..='z' |
-            '\u{C0}'..='\u{D6}' |
-            '\u{D8}'..='\u{F6}' |
-            '\u{F8}'..='\u{2FF}' |
-            '\u{370}'..='\u{37D}' |
-            '\u{37F}'..='\u{1FFF}' |
-            '\u{200C}'..='\u{200D}' |
-            '\u{2070}'..='\u{218F}' |
-            '\u{2C00}'..='\u{2FEF}' |
-            '\u{3001}'..='\u{D7FF}' |
-            '\u{F900}'..='\u{FDCF}' |
-            '\u{FDF0}'..='\u{FFFD}' |
-            '\u{10000}'..='\u{EFFFF}')
-    }
-
-    fn is_valid_continuation(c: char) -> bool {
-        is_valid_start(c) ||
-            matches!(c,
-                '-' |
-                '.' |
-                '0'..='9' |
-                '\u{B7}' |
-                '\u{300}'..='\u{36F}' |
-                '\u{203F}'..='\u{2040}')
-    }
-
+pub(crate) fn matches_name_production(name: &str) -> bool {
     let mut iter = name.chars();
-    let mut non_qname_colons = false;
-    let mut seen_colon = false;
-    let mut last = match iter.next() {
-        None => return XMLName::Invalid,
-        Some(c) => {
-            if !is_valid_start(c) {
-                return XMLName::Invalid;
-            }
-            if c == ':' {
-                non_qname_colons = true;
-            }
-            c
-        },
-    };
-
-    for c in iter {
-        if !is_valid_continuation(c) {
-            return XMLName::Invalid;
-        }
-        if c == ':' {
-            if seen_colon {
-                non_qname_colons = true;
-            } else {
-                seen_colon = true;
-            }
-        }
-        last = c
-    }
-
-    if last == ':' {
-        non_qname_colons = true
-    }
 
-    if non_qname_colons {
-        XMLName::Name
-    } else {
-        XMLName::QName
+    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.
diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs
index f7bc139aac5..f5b69f4588d 100644
--- a/components/script/dom/document.rs
+++ b/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.
-        if xml_name_type(&target) == Invalid {
+        // Step 1. If target does not match the Name production, then throw an "InvalidCharacterError" DOMException.
+        if !matches_name_production(&target) {
             return Err(Error::InvalidCharacter);
         }
 
diff --git a/components/script/dom/domimplementation.rs b/components/script/dom/domimplementation.rs
index c263ba940af..af68edfc825 100644
--- a/components/script/dom/domimplementation.rs
+++ b/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 {
diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs
index 9783b65b9ce..c9e4318e263 100644
--- a/components/script/dom/element.rs
+++ b/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.
-        if xml_name_type(&name) == Invalid {
+        // Step 1. If qualifiedName does not match the Name production in XML,
+        // then throw an "InvalidCharacterError" DOMException.
+        if !matches_name_production(&name) {
             return Err(Error::InvalidCharacter);
         }