Replace attribute invalid characters with _, vs stripping

jhy · jhy · commit 6cbe7e494fe8 · 2024-07-10T11:03:20.000+10:00
Fixes #2143
diff --git a/CHANGES.md b/CHANGES.md
@@ -20,6 +20,9 @@
 * Updated the `button` tag configuration to include a space between multiple button elements in the `Element.text()`
   method. [2105](https://github.com/jhy/jsoup/issues/2105)
 * Added support for the `ns|*` all elements in namespace Selector. [1811](https://github.com/jhy/jsoup/issues/1811)
+* When normalising attribute names during serialization, invalid characters are now replaced with `_`, vs being
+  stripped. This should make the process clearer, and generally prevent an invalid attribute name being coerced
+  unexpectedly. [2143](https://github.com/jhy/jsoup/issues/2143)
 
 ### Changes
 
diff --git a/src/main/java/org/jsoup/nodes/Attribute.java b/src/main/java/org/jsoup/nodes/Attribute.java
@@ -174,18 +174,23 @@ static void htmlNoValidate(String key, @Nullable String val, Appendable accum, D
     }
 
     private static final Pattern xmlKeyValid = Pattern.compile("[a-zA-Z_:][-a-zA-Z0-9_:.]*");
-    private static final Pattern xmlKeyReplace = Pattern.compile("[^-a-zA-Z0-9_:.]");
+    private static final Pattern xmlKeyReplace = Pattern.compile("[^-a-zA-Z0-9_:.]+");
     private static final Pattern htmlKeyValid = Pattern.compile("[^\\x00-\\x1f\\x7f-\\x9f \"'/=]+");
-    private static final Pattern htmlKeyReplace = Pattern.compile("[\\x00-\\x1f\\x7f-\\x9f \"'/=]");
+    private static final Pattern htmlKeyReplace = Pattern.compile("[\\x00-\\x1f\\x7f-\\x9f \"'/=]+");
 
+    /**
+     * Get a valid attribute key for the given syntax. If the key is not valid, it will be coerced into a valid key.
+     * @param key the original attribute key
+     * @param syntax HTML or XML
+     * @return the original key if it's valid; a key with invalid characters replaced with "_" otherwise; or null if a valid key could not be created.
+     */
     @Nullable public static String getValidKey(String key, Syntax syntax) {
-        // we consider HTML attributes to always be valid. XML checks key validity
         if (syntax == Syntax.xml && !xmlKeyValid.matcher(key).matches()) {
-            key = xmlKeyReplace.matcher(key).replaceAll("");
+            key = xmlKeyReplace.matcher(key).replaceAll("_");
             return xmlKeyValid.matcher(key).matches() ? key : null; // null if could not be coerced
         }
         else if (syntax == Syntax.html && !htmlKeyValid.matcher(key).matches()) {
-            key = htmlKeyReplace.matcher(key).replaceAll("");
+            key = htmlKeyReplace.matcher(key).replaceAll("_");
             return htmlKeyValid.matcher(key).matches() ? key : null; // null if could not be coerced
         }
         return key;
diff --git a/src/test/java/org/jsoup/helper/W3CDomTest.java b/src/test/java/org/jsoup/helper/W3CDomTest.java
@@ -151,7 +151,7 @@ public void handlesInvalidAttributeNames() {
 
         Document w3Doc = W3CDom.convert(jsoupDoc);
         String xml = W3CDom.asString(w3Doc, W3CDom.OutputXml());
-        assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?><html xmlns=\"http://www.w3.org/1999/xhtml\"><head/><body name=\"\" style=\"color: red\"/></html>", xml);
+        assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?><html xmlns=\"http://www.w3.org/1999/xhtml\"><head/><body _=\"\" name_=\"\" style=\"color: red\"/></html>", xml);
     }
 
     @Test
@@ -175,7 +175,7 @@ public void xmlInputDocMaintainsHtmlAttributeNames() {
 
         Document w3Doc = W3CDom.convert(jsoupDoc);
         String out = W3CDom.asString(w3Doc, W3CDom.OutputHtml());
-        String expected = "<!DOCTYPE html SYSTEM \"about:legacy-compat\"><html xmlns=\"http://www.w3.org/1999/xhtml\"><head><META http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\"></head><body><p hnh=\"2\">unicode attr names coerced</p></body></html>";
+        String expected = "<!DOCTYPE html SYSTEM \"about:legacy-compat\"><html xmlns=\"http://www.w3.org/1999/xhtml\"><head><META http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\"></head><body><p h_nh=\"2\">unicode attr names coerced</p></body></html>";
         assertEquals(expected, TextUtil.stripNewlines(out));
     }
 
diff --git a/src/test/java/org/jsoup/parser/HtmlParserTest.java b/src/test/java/org/jsoup/parser/HtmlParserTest.java
@@ -85,7 +85,7 @@ private static Stream<Arguments> dupeAttributeData() {
         // NOTE: per spec this should be the test case. but impacts too many ppl
         // assertEquals("<p =a>One<a <p>Something</a></p>\n<a <p>Else</a>", doc.body().html());
 
-        assertEquals("<p a>One<a></a></p><p><a>Something</a></p><a>Else</a>", TextUtil.stripNewlines(doc.body().html()));
+        assertEquals("<p _a>One<a></a></p><p><a>Something</a></p><a>Else</a>", TextUtil.stripNewlines(doc.body().html()));
 
         doc = Jsoup.parse("<p .....>");
         assertEquals("<p .....></p>", doc.body().html());
@@ -1522,7 +1522,7 @@ private boolean didAddElements(String input) {
         assertEquals(Document.OutputSettings.Syntax.html, doc.outputSettings().syntax());
 
         String out = doc.body().outerHtml();
-        assertEquals("<body style=\"color: red\" name>\n <div></div>\n</body>", out);
+        assertEquals("<body style=\"color: red\" _ name_>\n <div _></div>\n</body>", out);
     }
 
     @Test void templateInHead() {
diff --git a/src/test/java/org/jsoup/parser/XmlTreeBuilderTest.java b/src/test/java/org/jsoup/parser/XmlTreeBuilderTest.java
@@ -296,7 +296,7 @@ public void handlesLTinScript() {
         assertEquals(Syntax.xml, doc.outputSettings().syntax());
 
         String out = doc.html();
-        assertEquals("<body style=\"color: red\" name=\"\"><div></div></body>", out);
+        assertEquals("<body style=\"color: red\" _=\"\" name_=\"\"><div _=\"\"></div></body>", out);
     }
 
     @Test void customTagsAreFlyweights() {

Original file line number	Diff line number	Diff line change
`@@ -151,7 +151,7 @@ public void handlesInvalidAttributeNames() {`
`151`	`151`
`152`	`152`	`Document w3Doc = W3CDom.convert(jsoupDoc);`
`153`	`153`	`String xml = W3CDom.asString(w3Doc, W3CDom.OutputXml());`
`154`		`- assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?><html xmlns=\"http://www.w3.org/1999/xhtml\"><head/><body name=\"\" style=\"color: red\"/></html>", xml);`
	`154`	`+ assertEquals("<?xml version=\"1.0\" encoding=\"UTF-8\"?><html xmlns=\"http://www.w3.org/1999/xhtml\"><head/><body _=\"\" name_=\"\" style=\"color: red\"/></html>", xml);`
`155`	`155`	`}`
`156`	`156`
`157`	`157`	`@Test`
`@@ -175,7 +175,7 @@ public void xmlInputDocMaintainsHtmlAttributeNames() {`
`175`	`175`
`176`	`176`	`Document w3Doc = W3CDom.convert(jsoupDoc);`
`177`	`177`	`String out = W3CDom.asString(w3Doc, W3CDom.OutputHtml());`
`178`		`- String expected = "<!DOCTYPE html SYSTEM \"about:legacy-compat\"><html xmlns=\"http://www.w3.org/1999/xhtml\"><head><META http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\"></head><body><p hnh=\"2\">unicode attr names coerced</p></body></html>";`
	`178`	`+ String expected = "<!DOCTYPE html SYSTEM \"about:legacy-compat\"><html xmlns=\"http://www.w3.org/1999/xhtml\"><head><META http-equiv=\"Content-Type\" content=\"text/html; charset=UTF-8\"></head><body><p h_nh=\"2\">unicode attr names coerced</p></body></html>";`
`179`	`179`	`assertEquals(expected, TextUtil.stripNewlines(out));`
`180`	`180`	`}`
`181`	`181`
Original file line number	Diff line number	Diff line change
`@@ -296,7 +296,7 @@ public void handlesLTinScript() {`
`296`	`296`	`assertEquals(Syntax.xml, doc.outputSettings().syntax());`
`297`	`297`
`298`	`298`	`String out = doc.html();`
`299`		`- assertEquals("<body style=\"color: red\" name=\"\"><div></div></body>", out);`
	`299`	`+ assertEquals("<body style=\"color: red\" _=\"\" name_=\"\"><div _=\"\"></div></body>", out);`
`300`	`300`	`}`
`301`	`301`
`302`	`302`	`@Test void customTagsAreFlyweights() {`