fix(classes): treat duplicate attributes as a change

golang.org/x/net/html's tokenizer keeps only the first of repeated
attribute names, so FormatHTML's byte-preserving comparison saw
<a href="https://x" href="javascript:...">, <img src=ok
src="javascript:..."> and a repeated style as unchanged and stored the
raw bytes, and CheckHTML accepted them.

htmlTokens now scans each start tag's raw bytes for its attribute names,
following the tokenizer's own readTag rules, and reports the document as
changed when that list differs from the attributes the tokenizer kept.
Such input is stored as bluemonday's output, which keeps the first value
as browsers do, and CheckHTML refuses it. Input without repeated
attributes is still stored byte for byte. A differential run of 2M
random tags found no disagreement between the scan and the tokenizer.

classes/check_html_test.go

@@ -34,6 +34,11 @@ func TestCheckHTML(t *testing.T) {
 		{"form", `<form action="https://evil.test"><input type=password></form>`, false},
 		{"style element", `<style>p { color: red }</style>`, false},
 		{"comment", `<p>a</p><!-- hidden -->`, false},
+		{"duplicate href", `<p><a href="https://x" href="javascript:alert(1)">x</a></p>`, false},
+		{"duplicate src", `<img src=ok src="javascript:alert(1)">`, false},
+		{"duplicate style", `<p style="color: red" style="position: fixed">x</p>`, false},
+		{"duplicate of an allowed attribute", `<p class="a" class="a">x</p>`, false},
+		{"attribute names inside a value", `<p><a href="https://example.test/?src=a&amp;href=b">x</a></p>`, true},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {

classes/format_html.go

@@ -136,10 +136,12 @@ func CheckHTML(doc string) error {
 // sanitizeHTML returns doc unchanged when htmlPolicy keeps all of it, and the
 // policy's output with changed set otherwise. Keeping all of it means doc and
 // the policy output tokenize to the same HTML tokens, so an output that only
-// re-escapes text or attribute values is not a change.
+// re-escapes text or attribute values is not a change. A tag that repeats an
+// attribute is always a change: the tokenizer keeps only the first of them,
+// so the comparison would not see the others in the stored bytes.
 func sanitizeHTML(doc string) (string, bool) {
 	clean := htmlPolicy.Sanitize(doc)
-	if clean == doc || sameTokens(doc, clean) {
+	if sameTokens(doc, clean) {
 		return doc, false
 	}
 	return clean, true
@@ -147,12 +149,19 @@ func sanitizeHTML(doc string) (string, bool) {
 
 // sameTokens reports whether a and b tokenize to the same sequence of HTML
 // tokens: type, tag name or decoded text, and the set of decoded attributes.
+// It is false when either repeats an attribute in a tag.
 func sameTokens(a, b string) bool {
 	ta, oka := htmlTokens(a)
+	if !oka {
+		return false
+	}
 	tb, okb := htmlTokens(b)
-	return oka && okb && slices.Equal(ta, tb)
+	return okb && slices.Equal(ta, tb)
 }
 
+// htmlTokens tokenizes doc. It is not ok when the tokenizer stops on an
+// error other than EOF, or when a start tag's raw attributes differ from the
+// attributes the tokenizer kept, which happens when a name repeats.
 func htmlTokens(doc string) ([]string, bool) {
 	var out []string
 	z := xhtml.NewTokenizer(strings.NewReader(doc))
@@ -161,7 +170,21 @@ func htmlTokens(doc string) ([]string, bool) {
 		if tt == xhtml.ErrorToken {
 			return out, errors.Is(z.Err(), io.EOF)
 		}
+		var raw []string
+		if tt == xhtml.StartTagToken || tt == xhtml.SelfClosingTagToken {
+			raw = rawAttrNames(z.Raw())
+		}
 		t := z.Token()
+		if tt == xhtml.StartTagToken || tt == xhtml.SelfClosingTagToken {
+			if len(raw) != len(t.Attr) {
+				return out, false
+			}
+			for i, a := range t.Attr {
+				if raw[i] != a.Key {
+					return out, false
+				}
+			}
+		}
 		attrs := make([]string, 0, len(t.Attr))
 		for _, a := range t.Attr {
 			attrs = append(attrs, a.Namespace+":"+a.Key+"="+a.Val)
@@ -170,3 +193,108 @@ func htmlTokens(doc string) ([]string, bool) {
 		out = append(out, fmt.Sprintf("%d|%s|%q", tt, t.Data, attrs))
 	}
 }
+
+// rawAttrNames returns the ASCII-lowercased name of every attribute in the
+// raw start tag, repeats included, in source order. It scans the tag as
+// golang.org/x/net/html's tokenizer does (readTagName, readTagAttrKey and
+// readTagAttrVal), which drops every repeat of a name it has seen. A scan
+// that disagrees with the tokenizer only makes htmlTokens report a change.
+func rawAttrNames(raw []byte) []string {
+	i, n := 2, len(raw) // past '<' and the first byte of the tag name
+	skipSpace := func() {
+		for i < n && isHTMLSpace(raw[i]) {
+			i++
+		}
+	}
+	for i < n {
+		c := raw[i]
+		i++
+		if isHTMLSpace(c) {
+			break
+		}
+		if c == '/' || c == '>' {
+			i--
+			break
+		}
+	}
+	var names []string
+	skipSpace()
+	for i < n {
+		if raw[i] == '>' {
+			break
+		}
+		// Attribute name: an '=' that starts it is part of the name.
+		start, end := i, n
+		for i < n {
+			c := raw[i]
+			i++
+			if c == '=' && i == start+1 {
+				continue
+			}
+			if c == '=' || c == '/' || c == '>' || isHTMLSpace(c) {
+				i--
+				end = i
+				break
+			}
+		}
+		// Attribute value.
+		skipSpace()
+		if i < n {
+			c := raw[i]
+			i++
+			switch {
+			case c == '/':
+			case c != '=':
+				i--
+			default:
+				skipSpace()
+				if i < n {
+					q := raw[i]
+					i++
+					switch q {
+					case '>':
+						i--
+					case '"', '\'':
+						for i < n {
+							i++
+							if raw[i-1] == q {
+								break
+							}
+						}
+					default:
+						for i < n {
+							c := raw[i]
+							i++
+							if isHTMLSpace(c) {
+								break
+							}
+							if c == '>' {
+								i--
+								break
+							}
+						}
+					}
+				}
+			}
+		}
+		if end > start {
+			names = append(names, asciiLower(raw[start:end]))
+		}
+		skipSpace()
+	}
+	return names
+}
+
+func isHTMLSpace(c byte) bool {
+	return c == ' ' || c == '\n' || c == '\r' || c == '\t' || c == '\f'
+}
+
+func asciiLower(b []byte) string {
+	out := []byte(string(b))
+	for i, c := range out {
+		if 'A' <= c && c <= 'Z' {
+			out[i] = c + 'a' - 'A'
+		}
+	}
+	return string(out)
+}

classes/format_html_test.go

@@ -49,6 +49,9 @@ var unsafePayloads = []struct {
 	{"style element", "<style>body { background: url(https://evil.test/x) }</style>"},
 	{"link stylesheet", `<link rel="stylesheet" href="https://evil.test/x.css">`},
 	{"html comment", "<p>a</p><!-- <script>alert(1)</script> -->"},
+	{"duplicate href", `<p><a href="https://x" href="javascript:alert(1)">x</a></p>`},
+	{"duplicate src", `<img src=ok src="javascript:alert(1)">`},
+	{"duplicate style", `<p style="color: red" style="position: fixed; background: url(javascript:alert(1))">x</p>`},
 }
 
 // forbiddenElements must never appear in sanitized journal HTML.
@@ -159,6 +162,72 @@ func TestFormatHTMLSanitizesUnsafeHTML(t *testing.T) {
 	})
 }
 
+// TestFormatHTMLDuplicateAttributes pins W-2: the tokenizer keeps only the
+// first of repeated attributes, so input that repeats one is never stored
+// byte for byte. The sanitized output keeps the first value, as browsers do.
+func TestFormatHTMLDuplicateAttributes(t *testing.T) {
+	cases := []struct {
+		name, src, want string
+	}{
+		{"duplicate href", `<p><a href="https://x" href="javascript:alert(1)">x</a></p>`, `<p><a href="https://x">x</a></p>`},
+		{"duplicate href in other case", `<p><a HREF="https://x" href="javascript:alert(1)">x</a></p>`, `<p><a href="https://x">x</a></p>`},
+		{"duplicate src", `<img src=ok src="javascript:alert(1)">`, `<img src="ok">`},
+		{"duplicate style", `<p style="color: red" style="position: fixed">x</p>`, `<p style="color: red">x</p>`},
+		{"duplicate class", `<p class="a" class="b">x</p>`, `<p class="a">x</p>`},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			out, err := FormatHTML(tc.src)
+			if err != nil {
+				t.Fatalf("FormatHTML(%q): %v", tc.src, err)
+			}
+			if out != tc.want {
+				t.Fatalf("got %q, want %q", out, tc.want)
+			}
+		})
+	}
+}
+
+// TestRawAttrNames checks the raw attribute scan against the tokenizer: for
+// tags without repeats both list the same names in the same order.
+func TestRawAttrNames(t *testing.T) {
+	tags := []string{
+		`<p>`,
+		`<br/>`,
+		`<img src="a" alt='b' width=640>`,
+		`<img src="x"onerror="alert(1)">`,
+		`<svg/onload=alert(1)>`,
+		`<a href = "x" title=y/>`,
+		`<input disabled checked>`,
+		`<div =x a=b>`,
+		"<div\tA=1\nB='2'\fc=\"3\"\r>",
+		`<p title="a href=b" data-x='c src=d'>`,
+		`<a href=x/y title=z>`,
+		`<p a=">" b>`,
+		`<footer class="author-footer" itemscope itemtype="https://schema.org/Person">`,
+	}
+	for _, tag := range tags {
+		z := xhtml.NewTokenizer(strings.NewReader(tag))
+		tt := z.Next()
+		if tt != xhtml.StartTagToken && tt != xhtml.SelfClosingTagToken {
+			t.Fatalf("%q tokenized as %v", tag, tt)
+		}
+		got := rawAttrNames(z.Raw())
+		tok := z.Token()
+		want := make([]string, 0, len(tok.Attr))
+		for _, a := range tok.Attr {
+			want = append(want, a.Key)
+		}
+		if strings.Join(got, ",") != strings.Join(want, ",") {
+			t.Errorf("rawAttrNames(%q) = %q, tokenizer kept %q", tag, got, want)
+		}
+	}
+
+	if got := rawAttrNames([]byte(`<a HREF="x" href="y" Href=z title=t>`)); strings.Join(got, ",") != "href,href,href,title" {
+		t.Fatalf("repeats = %q", got)
+	}
+}
+
 // TestFormatHTMLKeepsSchemeLikeText pins WR-01: text that merely contains
 // "data:", "javascript:" or "vbscript:" is not a URL and passes unchanged.
 func TestFormatHTMLKeepsSchemeLikeText(t *testing.T) {