xml5ever: compare prefix and local name when looking for duplicate attributes

[FadeHack]

Fixes #775

<root xml:lang="en" lang="en"/> was reported as Duplicate attribute, but <root lang="en" xml:lang="en"/> parsed fine.

Why

finish_attribute compared the raw attribute name it had just read against the local names of the attributes already on the tag:

let current_attr_name = self.current_attr_name.borrow();
let name = &current_attr_name[..];
self.current_tag_attrs
    .borrow()
    .iter()
    .any(|a| &*a.name.local == name)

The stored attributes have been through process_qname by then, so xml:lang is sitting there as prefix xml with local name lang. Reading a plain lang right after it matches that local name and gets rejected.

That is also where the order dependence comes from. With lang first, the raw name xml:lang is compared against the local name lang, they do not match, and both attributes survive.

Namespaces in XML section 6.3 says two attributes are the same only when their expanded names are the same. The xml prefix is bound to http://www.w3.org/XML/1998/namespace by definition per section 3, whether or not it is declared, so xml:lang and lang have different expanded names and both orderings should parse cleanly. Section 6.3 lists that exact shape as legal, in its <good a="1" n1:a="2" /> example. It also affects the WPT test html/rendering/non-replaced-elements/tables/table-align-float.xhtml, whose root element carries both.

The fix

Run process_qname first, then compare the prefix along with the local name. Real duplicates still get reported, including two attributes that share a prefix.

Worth being clear about where this check sits. The tokenizer has no namespace bindings, they are resolved later, so it can only catch names that are identical. The case where two different prefixes are bound to the same namespace URI is a real duplicate under section 6.3, and that one is already handled in the tree builder by bind_attr_qname and check_duplicate_attr, which resolve the prefix and compare (Namespace, LocalName). So the split is that the tokenizer catches identical names and the tree builder catches everything that needs bindings. This PR only corrects the tokenizer half.

Testing

Added unit tests in xml5ever/src/tokenizer/mod.rs for both orderings from the issue, for real duplicates with and without a prefix, and one recording that the tokenizer deliberately stays quiet about two different prefixes since it cannot resolve them. The first two fail without the change.

cargo test --all, cargo fmt --all -- --check and cargo clippy --all-features --all-targets are clean.

[Ygg01]

Think the issue here is that you don't have an expanded name. To quote the spec:

[Definition: An expanded name is a pair consisting of a namespace name and a local name. ]

[Definition: For a name N in a namespace identified by a URI I, the namespace name is I. For a name N that is not in a namespace, the namespace name has no value.]

So in your case you have a

<root 
   xml:lang = "a"
   lang = "b"
/>

Because XML namespace with name xml isn't defined (more on that later), these two become following expanded names

<root
    lang = "a"
    lang = "b"
/>

So the error is the intended behavior.

What your PR allows is for someone to write following element <root xxx:lang = "en" xyz:lang = "de" zzz:lang = "fr">

When xxx, xyz, and zzz aren't defined namespaces they names xxx:lang, xyz:lang and zzz:lang are just their local name (lang) and hence duplicates.

What can change the situation in your case xml namespace and xmlns namespaces are the default or ever present. This suggests that XML Tokenizer needs to be namespace aware.

EDIT:

Look at following

For example, each of the bad empty-element tags is illegal in the following:

<!-- http://www.w3.org is bound to n1 and n2 -->
<x xmlns:n1="http://www.w3.org" 
  xmlns:n2="http://www.w3.org" >
 <bad a="1"     a="2" />
 <bad n1:a="1"  n2:a="2" />
</x>```

This is identical to <root a:name="1" b:name="2"/> test case. That said it seems like an edge case a lot of validators miss.

Because XML namespace Do not care about Prefixes. Prefixes are links to XML namespace and XML namespace determines what expanded name is.

[Ygg01]

The way I see it you can solve this option in several ways. But do we care about XML Namespaces or not?

If we do, then proper XMLNS-aware tokenizer sounds like the right solution.

If we want to unblock a special use case or de-facto use case, then your changes are okish, but the tree builder will need to catch these errors instead.

If we don't need mixed XMLNS attributes in a node, then PR is not needed.

[FadeHack]

Thanks for digging into this, the section 6.3 point is the right thing to be checking. I went and read it again along with section 3, and I think the two cases you raised land differently.

On xml:lang vs lang: the xml prefix is not undefined. Section 3 says it explicitly:

The prefix xml is by definition bound to the namespace name http://www.w3.org/XML/1998/namespace. It MAY, but need not, be declared, and MUST NOT be bound to any other namespace name.

So xml:lang expands to {http://www.w3.org/XML/1998/namespace}lang whether or not anyone declared it, and plain lang expands to lang in no namespace. Different expanded names, so not duplicates. Section 6.3 actually spells this exact shape out in its legal example:

<good a="1"     n1:a="2" />

That is the same pattern as lang plus xml:lang. And you can see it in what we build today, with the PR applied:

input:  <root xml:lang="en" lang="en"/>
errors: []
<root> attrs: ["{http://www.w3.org/XML/1998/namespace}lang=en", "{}lang=en"]

Two distinct expanded names on the element, which is what the reporter was after. The old behaviour rejected that in one order and accepted it in the other, so it was not enforcing 6.3, it was just comparing a raw name against an already split local name.

On n1:a and n2:a pointing at the same URI, you are right that those are duplicates and that prefixes alone cannot tell you. But that check already exists and it is in the tree builder, bind_attr_qname and check_duplicate_attr, which resolve the prefix and then key a HashSet on (Namespace, LocalName). Running your example from the spec through the parser:

input:  <x xmlns:n1="http://www.w3.org" xmlns:n2="http://www.w3.org"><bad n1:a="1" n2:a="2"/></x>
<x> attrs: []
  <bad> attrs: ["{http://www.w3.org}a=1"]

Only one a survives, so the duplicate is caught. Undeclared prefixes end up in the same place:

input:  <root a:name="1" b:name="2"/>
errors: ["No appropriate namespace found", "No appropriate namespace found"]
<root> attrs: ["{}name=1"]

Both fail to resolve, both land in no namespace, and the second is dropped as a duplicate.

So to answer your three options directly, I think we are already on the middle one and have been for a while. The split is that the tokenizer catches names that are literally identical, and the tree builder catches everything that needs bindings. This PR only fixes the tokenizer half, which was comparing the wrong two things and rejecting names it had no business rejecting.

You did make me realise one of my tests was overclaiming. I had different_prefixes_are_distinct asserting no error for <root a:name="1" b:name="2"/>, which reads like I am saying those two are definitely distinct, and as you point out that depends entirely on the bindings. I have renamed it to different_prefixes_are_left_to_the_tree_builder and the comment now says the tokenizer is deliberately saying nothing either way, which is what it is actually testing. Pushed that up.

One thing I noticed while checking this that is worth a separate issue if you agree: the tree builder drops the duplicate attribute silently, no parse_error. Section 6.3 makes it a namespace constraint violation, so I would have expected an error alongside the drop. I did not touch it here since it is outside what this PR is about.