GROOVY-12103 part2: XML markup builders don't reject PI/comment terminators in all output paths

Author: paulk-asert

subprojects/groovy-xml/src/main/groovy/groovy/xml/StreamingDOMBuilder.groovy

@@ -33,6 +33,7 @@ class StreamingDOMBuilder extends AbstractStreamingBuilder {
     def defaultNamespaceStack = ['']
     /** Closure backing {@code mkp.comment} for DOM comment node creation. */
     def commentClosure = {doc, pendingNamespaces, namespaces, namespaceSpecificTags, prefix, attrs, body, dom ->
+        checkCommentText(body)
         def comment = dom.document.createComment(body)
         if (comment != null) {
             dom.element.appendChild(comment)
@@ -41,10 +42,12 @@ class StreamingDOMBuilder extends AbstractStreamingBuilder {
     /** Closure backing {@code mkp.pi} for DOM processing-instruction node creation. */
     def piClosure = {doc, pendingNamespaces, namespaces, namespaceSpecificTags, prefix, attrs, body, dom ->
         attrs.each {target, instruction ->
+            checkProcessingInstruction(target)
             def pi = null
             if (instruction instanceof Map) {
                 pi = dom.document.createProcessingInstruction(target, toMapStringClosure(instruction))
             } else {
+                checkProcessingInstruction(instruction)
                 pi = dom.document.createProcessingInstruction(target, instruction)
             }
             if (pi != null) {

subprojects/groovy-xml/src/main/groovy/groovy/xml/StreamingMarkupBuilder.groovy

@@ -71,6 +71,7 @@ class StreamingMarkupBuilder extends AbstractStreamingBuilder {
      * Invoked by calling <code>mkp.comment</code>
      */
     def commentClosure = {doc, pendingNamespaces, namespaces, namespaceSpecificTags, prefix, attrs, body, out ->
+        checkCommentText(body)
         out.unescaped() << '<!--'
         out.escaped() << body
         out.unescaped() << '-->'
@@ -81,6 +82,7 @@ class StreamingMarkupBuilder extends AbstractStreamingBuilder {
      */
     def piClosure = {doc, pendingNamespaces, namespaces, namespaceSpecificTags, prefix, attrs, body, out ->
         attrs.each {target, instruction ->
+            checkProcessingInstruction(target)
             out.unescaped() << '<?'
             if (instruction instanceof Map) {
                 out.unescaped() << target
@@ -89,6 +91,7 @@ class StreamingMarkupBuilder extends AbstractStreamingBuilder {
                     valueStr.contains('\'') || (useDoubleQuotes && !valueStr.contains('"'))
                 }
             } else {
+                checkProcessingInstruction(instruction)
                 out.unescaped() << "$target $instruction"
             }
             out.unescaped() << '?>'

subprojects/groovy-xml/src/main/groovy/groovy/xml/streamingmarkupsupport/AbstractStreamingBuilder.groovy

@@ -89,6 +89,32 @@ class AbstractStreamingBuilder {
         buf.toString()
     }
 
+    /**
+     * Rejects a processing-instruction target or data fragment that would prematurely
+     * close the PI. The {@code ?>} sequence ends a PI and cannot be entity-escaped, so
+     * content holding it would inject sibling markup; such content is rejected instead.
+     */
+    def checkProcessingInstruction = { part ->
+        if (part?.toString()?.contains('?>')) {
+            throw new GroovyRuntimeException("Processing instruction target/data must not contain '?>': ${part}")
+        }
+        part
+    }
+
+    /**
+     * Rejects comment text that would prematurely close the comment. Comment content
+     * cannot contain {@code --} or end with {@code -} (which would form {@code --} against
+     * the closing delimiter), and no escaping is legal inside a comment, so such text is
+     * rejected instead.
+     */
+    def checkCommentText = { text ->
+        def value = text?.toString()
+        if (value != null && (value.contains('--') || value.endsWith('-'))) {
+            throw new GroovyRuntimeException("XML comment text must not contain '--' or end with '-': ${text}")
+        }
+        text
+    }
+
     /** Registry of built-in {@code mkp} helper tags shared by concrete streaming builders. */
     def specialTags = ['declareNamespace':namespaceSetupClosure,
                            'declareAlias':aliasSetupClosure,

subprojects/groovy-xml/src/test/groovy/groovy/xml/MarkupBuilderInjectionTest.groovy

@@ -62,4 +62,68 @@ final class MarkupBuilderInjectionTest {
         def result = smb.bind { mkp.pi('xml-stylesheet': [href: 'style.css', type: 'text/css']); root('x') }.toString()
         assertTrue(result.contains('<?xml-stylesheet') && result.contains('style.css') && result.contains('?>'))
     }
+
+    @Test
+    void streamingProcessingInstructionRejectsTerminatorInTarget() {
+        def smb = new StreamingMarkupBuilder()
+        assertThrows(GroovyRuntimeException) {
+            smb.bind { mkp.pi('x?><injected/>': [href: 'a']); root('x') }.toString()
+        }
+    }
+
+    @Test
+    void streamingProcessingInstructionRejectsTerminatorInNonMapData() {
+        def smb = new StreamingMarkupBuilder()
+        assertThrows(GroovyRuntimeException) {
+            smb.bind { mkp.pi('target': 'data?><injected/>'); root('x') }.toString()
+        }
+    }
+
+    @Test
+    void streamingProcessingInstructionKeepsPlainNonMapData() {
+        def smb = new StreamingMarkupBuilder()
+        def result = smb.bind { mkp.pi('target': 'plain data'); root('x') }.toString()
+        assertTrue(result.contains('<?target plain data?>'))
+    }
+
+    @Test
+    void streamingCommentRejectsCommentTerminator() {
+        def smb = new StreamingMarkupBuilder()
+        assertThrows(GroovyRuntimeException) {
+            smb.bind { mkp.comment('legit --> <injected/> rest'); root('x') }.toString()
+        }
+        assertThrows(GroovyRuntimeException) {
+            smb.bind { mkp.comment('a -- b'); root('x') }.toString()
+        }
+        assertThrows(GroovyRuntimeException) {
+            smb.bind { mkp.comment('ends with dash-'); root('x') }.toString()
+        }
+    }
+
+    @Test
+    void streamingCommentKeepsPlainText() {
+        def smb = new StreamingMarkupBuilder()
+        def result = smb.bind { mkp.comment('plain text'); root('x') }.toString()
+        assertTrue(result.contains('<!--plain text-->'))
+    }
+
+    @Test
+    void streamingDomProcessingInstructionRejectsTerminator() {
+        assertThrows(GroovyRuntimeException) {
+            new StreamingDOMBuilder().bind { mkp.pi('xml-stylesheet': [href: 'a?><injected/>']); root('x') }()
+        }
+        assertThrows(GroovyRuntimeException) {
+            new StreamingDOMBuilder().bind { mkp.pi('x?><injected/>': [href: 'a']); root('x') }()
+        }
+        assertThrows(GroovyRuntimeException) {
+            new StreamingDOMBuilder().bind { mkp.pi('target': 'data?><injected/>'); root('x') }()
+        }
+    }
+
+    @Test
+    void streamingDomCommentRejectsCommentTerminator() {
+        assertThrows(GroovyRuntimeException) {
+            new StreamingDOMBuilder().bind { mkp.comment('a -- b'); root('x') }()
+        }
+    }
 }