Repository: jhy/jsoup (java)

Issue:
Escape `<` and `>` when serializing attribute values
The spec has been [updated](https://github.com/whatwg/html/pull/6362) to help protect against mutation XSS.

See https://bughunters.google.com/blog/5038742869770240/escaping-and-in-attributes-how-it-helps-protect-against-mutation-xss

Pull request:
Escape `<` and `>` when serializing attribute values
Fixes #2337

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index 980a8d82..a01e11d9 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -14,6 +14,7 @@
 * Added `NodeVisitor#traverse(Node)` to simplify node traversal calls (vs. importing `NodeTraversor`).
 * The HTML parser now allows the specific text-data type (Data, RcData) to be customized for known tags. (Previously, that was only supported on custom tags.) [#2326](https://github.com/jhy/jsoup/issues/2326).
 * Added `Connection#readFully()` as a replacement for `Connection#bufferUp()` with an explicit IOException. Similarly, added `Connection#readBody()` over `Connection#body()`. Deprecated `Connection#bufferUp()`. [#2327](https://github.com/jhy/jsoup/pull/2327) 
+* When serializing HTML, the `<` and `>` characters are now escaped in attributes. This helps prevent a class of mutation XSS attacks. [#2337](https://github.com/jhy/jsoup/pull/2337) 
 
 ### Bug Fixes
 * The contents of a `script` in a `svg` foreign context should be parsed as script data, not text. [#2320](https://github.com/jhy/jsoup/issues/2320)
diff --git a/src/main/java/org/jsoup/nodes/Entities.java b/src/main/java/org/jsoup/nodes/Entities.java
index ad379a74..b0b451c0 100644
--- a/src/main/java/org/jsoup/nodes/Entities.java
+++ b/src/main/java/org/jsoup/nodes/Entities.java
@@ -247,12 +247,10 @@ public class Entities {
                     appendNbsp(accum, escapeMode);
                     break;
                 case '<':
-                    // escape when in character data or when in a xml attribute val or XML syntax; not needed in html attr val
-                    appendLt(accum, options, escapeMode, syntax);
+                    accum.append("&lt;");
                     break;
                 case '>':
-                    if ((options & ForText) != 0) accum.append("&gt;");
-                    else accum.append(c);
+                    accum.append("&gt;");
                     break;
                 case '"':
                     if ((options & ForAttribute) != 0) accum.append("&quot;");
@@ -291,11 +289,6 @@ public class Entities {
         else accum.append("&#xa0;");
     }
 
-    private static void appendLt(QuietAppendable accum, int options, EscapeMode escapeMode, Syntax syntax) {
-        if ((options & ForText) != 0 || escapeMode == EscapeMode.xhtml || syntax == Syntax.xml) accum.append("&lt;");
-        else accum.append('<'); // no need to escape < when in an HTML attribute
-    }
-
     private static void appendApos(QuietAppendable accum, int options, EscapeMode escapeMode) {
         if ((options & ForAttribute) != 0 && (options & ForText) != 0) {
             if (escapeMode == EscapeMode.xhtml) accum.append("&#x27;");


Test patch:
diff --git a/src/test/java/org/jsoup/nodes/AttributeTest.java b/src/test/java/org/jsoup/nodes/AttributeTest.java
index a233fcf3..4cff3205 100644
--- a/src/test/java/org/jsoup/nodes/AttributeTest.java
+++ b/src/test/java/org/jsoup/nodes/AttributeTest.java
@@ -16,6 +16,12 @@ public class AttributeTest {
         assertEquals(attr.html(), attr.toString());
     }
 
+    @Test
+    public void htmlWithLtAndGtInValue() {
+        Attribute attr = new Attribute("key", "<value>");
+        assertEquals("key=\"&lt;value&gt;\"", attr.html());
+    }
+
     @Test public void testWithSupplementaryCharacterInAttributeKeyAndValue() {
         String s = new String(Character.toChars(135361));
         Attribute attr = new Attribute(s, "A" + s + "B");
diff --git a/src/test/java/org/jsoup/nodes/DocumentTest.java b/src/test/java/org/jsoup/nodes/DocumentTest.java
index ea9a8a20..6fc6080f 100644
--- a/src/test/java/org/jsoup/nodes/DocumentTest.java
+++ b/src/test/java/org/jsoup/nodes/DocumentTest.java
@@ -167,7 +167,7 @@ public class DocumentTest {
                 "<html>\n" +
                 " <head></head>\n" +
                 " <body>\n" +
-                "  <img async checked src=\"&amp;<>&quot;\">&lt;&gt;&amp;\"<foo></foo>bar\n" + // html won't include self-closing
+                "  <img async checked src=\"&amp;&lt;&gt;&quot;\">&lt;&gt;&amp;\"<foo></foo>bar\n" + // html won't include self-closing
                 " </body>\n" +
                 "</html>", doc.html());
 
@@ -176,7 +176,7 @@ public class DocumentTest {
                 "<html>\n" +
                 " <head></head>\n" +
                 " <body>\n" +
-                "  <img async=\"\" checked=\"checked\" src=\"&amp;&lt;>&quot;\" />&lt;&gt;&amp;\"<foo />bar\n" + // xml will
+                "  <img async=\"\" checked=\"checked\" src=\"&amp;&lt;&gt;&quot;\" />&lt;&gt;&amp;\"<foo />bar\n" + // xml will
                 " </body>\n" +
                 "</html>", doc.html());
     }
diff --git a/src/test/java/org/jsoup/nodes/EntitiesTest.java b/src/test/java/org/jsoup/nodes/EntitiesTest.java
index 25226423..6180e27c 100644
--- a/src/test/java/org/jsoup/nodes/EntitiesTest.java
+++ b/src/test/java/org/jsoup/nodes/EntitiesTest.java
@@ -152,19 +152,18 @@ public class EntitiesTest {
         assertEquals(string, Entities.unescape(string));
     }
 
-    @Test public void escapesGtInXmlAttributesButNotInHtml() {
-        // https://github.com/jhy/jsoup/issues/528 - < is OK in HTML attribute values, but not in XML
-
+    @Test public void alwaysEscapeLtAndGtInAttributeValues() {
+        // https://github.com/jhy/jsoup/issues/2337
 
         String docHtml = "<a title='<p>One</p>'>One</a>";
         Document doc = Jsoup.parse(docHtml);
         Element element = doc.select("a").first();
 
         doc.outputSettings().escapeMode(base);
-        assertEquals("<a title=\"<p>One</p>\">One</a>", element.outerHtml());
+        assertEquals("<a title=\"&lt;p&gt;One&lt;/p&gt;\">One</a>", element.outerHtml());
 
         doc.outputSettings().escapeMode(xhtml);
-        assertEquals("<a title=\"&lt;p>One&lt;/p>\">One</a>", element.outerHtml());
+        assertEquals("<a title=\"&lt;p&gt;One&lt;/p&gt;\">One</a>", element.outerHtml());
     }
 
     @Test public void controlCharactersAreEscaped() {
diff --git a/src/test/java/org/jsoup/parser/XmlTreeBuilderTest.java b/src/test/java/org/jsoup/parser/XmlTreeBuilderTest.java
index b4ee3feb..dcba7986 100644
--- a/src/test/java/org/jsoup/parser/XmlTreeBuilderTest.java
+++ b/src/test/java/org/jsoup/parser/XmlTreeBuilderTest.java
@@ -4,7 +4,6 @@ import org.jsoup.Jsoup;
 import org.jsoup.TextUtil;
 import org.jsoup.nodes.*;
 import org.jsoup.select.Elements;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import java.io.File;
@@ -256,20 +255,17 @@ public class XmlTreeBuilderTest {
 
     @Test public void xmlParserEnablesXmlOutputAndEscapes() {
         // Test that when using the XML parser, the output mode and escape mode default to XHTML entities
-        // https://github.com/jhy/jsoup/issues/1420
-        Document doc = Jsoup.parse("<p one='&lt;two&gt;&copy'>Three</p>", "", Parser.xmlParser());
+        Document doc = Jsoup.parse("<root/>", "", Parser.xmlParser());
         assertEquals(doc.outputSettings().syntax(), Syntax.xml);
         assertEquals(doc.outputSettings().escapeMode(), Entities.EscapeMode.xhtml);
-        assertEquals("<p one=\"&lt;two>©\">Three</p>", doc.html()); // only the < should be escaped
     }
 
-    @Test public void xmlSyntaxEscapesLtInAttributes() {
-        // Regardless of the entity escape mode, make sure < is escaped in attributes when in XML
-        Document doc = Jsoup.parse("<p one='&lt;two&gt;&copy'>Three</p>", "", Parser.xmlParser());
+    @Test public void xmlSyntaxAlwaysEscapesLtAndGtInAttributeValues() {
+        // https://github.com/jhy/jsoup/issues/2337
+        Document doc = Jsoup.parse("<p one='&lt;two&gt;'>Three</p>", "", Parser.xmlParser());
         doc.outputSettings().escapeMode(Entities.EscapeMode.extended);
-        doc.outputSettings().charset("ascii"); // to make sure &copy; is output
         assertEquals(doc.outputSettings().syntax(), Syntax.xml);
-        assertEquals("<p one=\"&lt;two>&copy;\">Three</p>", doc.html());
+        assertEquals("<p one=\"&lt;two&gt;\">Three</p>", doc.html());
     }
 
     @Test void xmlOutputCorrectsInvalidAttributeNames() {
@@ -372,7 +368,6 @@ public class XmlTreeBuilderTest {
         // https://github.com/jhy/jsoup/issues/1947
         String xml = "<x><?xmlDeclaration att1=\"value1\" att2=\"&lt;val2>\"?></x>";
         Document doc = Jsoup.parse(xml, Parser.xmlParser());
-        assertEquals(xml, doc.html());
         XmlDeclaration decl = (XmlDeclaration) doc.expectFirst("x").childNode(0);
         assertEquals("<val2>", decl.attr("att2"));
     }