GROOVY-12278: Escape markup attribute values and refuse names which are not names

MarkupTemplateEngine writes element text through escapeXml, so a template
author can reasonably read the engine as treating the values it is given as
data. The attribute path did not hold up that reading. A value was escaped
only for the quote character configured as the delimiter, and an attribute
name was written exactly as it arrived.

Escape a value for the delimiter, as before, and additionally for the
ampersand and the angle brackets, which are not well formed inside an
attribute value whichever quote surrounds it. The other quote character is
neither unsafe nor ill formed there, so it is left as written and output is
unchanged for every value that was already well formed.

Refuse an attribute name which is not a name. A name has no escaped form:
escaping one produces a different name rather than a safe version of the
same one, so a name arriving from data is checked and rejected instead. This
is the half with teeth, since a map key such as

    x='1' onmouseover='alert(1)'

was previously written out and introduced attributes of its own.

Doing so surfaced that xmlDeclaration passed " encoding" as an attribute
name, using a leading space as a separator; the space is now written
separately and the name is a name.

escapeQuotes had no remaining caller and is removed.

Behaviour change worth a release note: a template which places an ampersand
or an angle bracket in an attribute value now emits it escaped, and one
which builds attribute names from data will fail rather than emit markup
whose shape the data chose.

Author: paulk-asert

subprojects/groovy-templates/src/main/groovy/groovy/text/markup/BaseTemplate.java

@@ -168,7 +168,8 @@ public BaseTemplate xmlDeclaration() throws IOException {
         out.write("<?xml ");
         writeAttribute("version", "1.0");
         if (configuration.getDeclarationEncoding() != null) {
-            writeAttribute(" encoding", configuration.getDeclarationEncoding());
+            out.write(' ');
+            writeAttribute("encoding", configuration.getDeclarationEncoding());
         }
         out.write("?>");
         out.write(configuration.getNewLineString());
@@ -213,13 +214,56 @@ public BaseTemplate pi(Map<?, ?> attrs) throws IOException {
     }
 
     private void writeAttribute(String attName, String value) throws IOException {
+        checkAttributeName(attName);
         out.write(attName);
         out.write("=");
         writeQt();
-        out.write(escapeQuotes(value));
+        out.write(escapeAttributeValue(value));
         writeQt();
     }
 
+    /**
+     * Escapes an attribute value. The delimiter in use would otherwise end the value; the
+     * ampersand and less-than are not well formed inside an attribute value; and the
+     * greater-than, though well formed there, is escaped too so a value is treated exactly as
+     * element text is. The other quote character, being neither the delimiter nor ill formed,
+     * is left as written.
+     *
+     * @param str the attribute value
+     * @return the escaped value
+     */
+    private String escapeAttributeValue(final String str) {
+        String quote = configuration.isUseDoubleQuotes() ? "\"" : "'";
+        String escape = configuration.isUseDoubleQuotes() ? "&quot;" : "&apos;";
+        // The ampersand goes first so the entities introduced after it are not re-escaped.
+        return str.replace("&", "&amp;")
+                .replace("<", "&lt;")
+                .replace(">", "&gt;")
+                .replace(quote, escape);
+    }
+
+    /**
+     * Rejects an attribute name which is not a name.
+     * <p>
+     * A name has no escaped form: escaping one yields a different name rather than a safe
+     * version of the same one, and writing it unaltered lets a name taken from data introduce
+     * further attributes of its own. So an unusable name is refused instead.
+     *
+     * @param attName the attribute name to check
+     * @throws IllegalArgumentException if the name cannot be written as an attribute name
+     */
+    private static void checkAttributeName(final String attName) {
+        boolean usable = attName != null && !attName.isEmpty()
+                && (Character.isLetter(attName.charAt(0)) || attName.charAt(0) == '_' || attName.charAt(0) == ':');
+        for (int i = 1; usable && i < attName.length(); i += 1) {
+            char c = attName.charAt(i);
+            usable = Character.isLetterOrDigit(c) || c == '-' || c == '_' || c == '.' || c == ':';
+        }
+        if (!usable) {
+            throw new IllegalArgumentException("Invalid markup attribute name: " + attName);
+        }
+    }
+
     private void writeQt() throws IOException {
         if (configuration.isUseDoubleQuotes()) {
             out.write('"');
@@ -235,11 +279,6 @@ private void writeIndent() throws IOException {
         }
     }
 
-    private String escapeQuotes(String str) {
-        String quote = configuration.isUseDoubleQuotes() ? "\"" : "'";
-        String escape = configuration.isUseDoubleQuotes() ? "&quot;" : "&apos;";
-        return str.replace(quote, escape);
-    }
 
     /**
      * This is the main method responsible for writing a tag and its attributes.

subprojects/groovy-templates/src/test/groovy/groovy/text/MarkupTemplateEngineTest.groovy

@@ -273,6 +273,72 @@ final class MarkupTemplateEngineTest {
         assert rendered.toString() == '<html><a href=\'foo.html\'>Link text</a><tagWithQuote attr=\'fo&apos;o\'/></html>'
     }
 
+    // GROOVY-12278: an attribute value is data, as element text is. yield() escapes all five XML
+    // metacharacters; the attribute path escaped only the delimiter in use, so an ampersand or a
+    // less-than in a value produced markup that is not well formed, and a greater-than was left
+    // unescaped where element text escapes it.
+    @Test
+    void testAttributeValuesAreEscapedLikeElementText() {
+        def engine = new MarkupTemplateEngine(new TemplateConfiguration())
+        def template = engine.createTemplate '''
+            html {
+                div(title: value)
+            }
+        '''
+        StringWriter rendered = new StringWriter()
+        template.make([value: 'a & b < c > d']).writeTo(rendered)
+        assert rendered.toString() == "<html><div title='a &amp; b &lt; c &gt; d'/></html>"
+    }
+
+    // The delimiter still cannot end the value, and the other quote is neither unsafe nor
+    // ill-formed inside one, so it is left as written.
+    @Test
+    void testAttributeValueCannotEndItsOwnAttribute() {
+        def engine = new MarkupTemplateEngine(new TemplateConfiguration())
+        def template = engine.createTemplate '''
+            html {
+                div(title: value)
+            }
+        '''
+        StringWriter rendered = new StringWriter()
+        template.make([value: "x' onmouseover='alert(1)"]).writeTo(rendered)
+        assert !rendered.toString().contains("onmouseover='alert")
+        assert rendered.toString().contains('&apos;')
+    }
+
+    // GROOVY-12278: a name has no escaped form, so a name taken from data which is not a name is
+    // refused rather than written out to introduce attributes of its own.
+    @Test
+    void testAttributeNameTakenFromDataMustBeAName() {
+        def engine = new MarkupTemplateEngine(new TemplateConfiguration())
+        def template = engine.createTemplate '''
+            html {
+                div(attrs)
+            }
+        '''
+        StringWriter rendered = new StringWriter()
+        def err = shouldFail(IllegalArgumentException) {
+            template.make([attrs: ["x='1' onmouseover='alert(1)'": 'y']]).writeTo(rendered)
+        }
+        assert err.message.contains('Invalid markup attribute name')
+    }
+
+    @Test
+    void testOrdinaryAttributeNamesStillRender() {
+        def engine = new MarkupTemplateEngine(new TemplateConfiguration())
+        def template = engine.createTemplate '''
+            html {
+                div('data-id': 1, 'xlink:href': 'a', _private: 2, 'a.b': 3)
+            }
+        '''
+        StringWriter rendered = new StringWriter()
+        template.make().writeTo(rendered)
+        assert rendered.toString().contains("data-id='1'")
+        assert rendered.toString().contains("xlink:href='a'")
+        assert rendered.toString().contains("_private='2'")
+        assert rendered.toString().contains("a.b='3'")
+    }
+
     @Test
     void testTagsWithAttributesAndDoubleQuotes() {
         def engine = new MarkupTemplateEngine(new TemplateConfiguration())