Automate testing for XMLUtils factory methods

checkstyle.xml

@@ -11,17 +11,32 @@
     <module name="TreeWalker">
         <!--
         <module name="TodoComment">-->
-            <!-- The (?i) below means Case Insensitive -->
+        <!-- The (?i) below means Case Insensitive -->
         <!--<property name="format" value="(?i)FIXME"/>
 -->
-  <module name="RegexpSinglelineJava">
+        <module name="RegexpSinglelineJava">
-      <property name="format" value="org\.jetbrains\.annotations\.NotNull"/>
+            <property name="format" value="org\.jetbrains\.annotations\.NotNull"/>
-  </module>
+        </module>
-  <module name="RegexpSinglelineJava">
+        <module name="RegexpSinglelineJava">
-      <property name="format" value="org\.jetbrains\.annotations\.Nullable"/>
+            <property name="format" value="org\.jetbrains\.annotations\.Nullable"/>
-  </module>
+        </module>
-  <module name="RegexpSinglelineJava">
+        <module name="RegexpSinglelineJava">
-      <property name="format" value="org\.jetbrains\.annotations\.\*"/>
+            <property name="format" value="org\.jetbrains\.annotations\.\*"/>
-  </module>
+        </module>
-</module>
+    </module>
+    <module name="RegexpMultiline">
+        <property name="id" value="transformerFactoryNewInstance"/>
+        <property name="matchAcrossLines" value="true"/>
+        <property name="format" value="TransformerFactory\.newInstance\("/>
+    </module>
+    <module name="RegexpMultiline">
+        <property name="id" value="documentBuilderFactoryNewInstance"/>
+        <property name="matchAcrossLines" value="true"/>
+        <property name="format" value="DocumentBuilderFactory\.newInstance\("/>
+    </module>
+    <module name="RegexpMultiline">
+        <property name="id" value="saxParserFactoryNewInstance"/>
+        <property name="matchAcrossLines" value="true"/>
+        <property name="format" value="SAXParserFactory\.newInstance\("/>
+    </module>
 </module>

checkstyle_suppressions.xml

@@ -4,5 +4,5 @@
         "https://checkstyle.org/dtds/suppressions_1_2.dtd">
 
 <suppressions>
-  
+
 </suppressions>

org.hl7.fhir.utilities/checkstyle_suppressions.xml

@@ -4,5 +4,7 @@
         "https://checkstyle.org/dtds/suppressions_1_2.dtd">
 
 <suppressions>
-
+    <suppress id="transformerFactoryNewInstance" files="/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java" lines="508"/>
+    <suppress id="documentBuilderFactoryNewInstance" files="/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java" lines="515"/>
+    <suppress id="saxParserFactoryNewInstance" files="/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java" lines="522"/>
 </suppressions>

org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/xml/XMLUtil.java

@@ -522,6 +522,7 @@ public class XMLUtil {
     final SAXParserFactory spf = SAXParserFactory.newInstance();
     spf.setFeature(SAX_FEATURES_EXTERNAL_GENERAL_ENTITIES, false);
     spf.setFeature(APACHE_XML_FEATURES_DISALLOW_DOCTYPE_DECL, true);
+
     return spf;
   }
 

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/xml/XMLUtilTests.java

@@ -0,0 +1,79 @@
+package org.hl7.fhir.utilities.xml;
+
+import net.sf.saxon.trans.XPathException;
+import org.hl7.fhir.utilities.filesystem.ManagedFileAccess;
+import org.junit.Test;
+import org.w3c.dom.Document;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.SAXParseException;
+import org.xml.sax.XMLReader;
+
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.parsers.SAXParserFactory;
+import javax.xml.transform.*;
+import javax.xml.transform.dom.DOMResult;
+import javax.xml.transform.dom.DOMSource;
+import javax.xml.transform.sax.SAXSource;
+import javax.xml.transform.stream.StreamResult;
+import javax.xml.transform.stream.StreamSource;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.StringWriter;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class XMLUtilTests {
+  @Test
+  public void testDocumentBuilderThrowsExceptionForExternalEntity() throws ParserConfigurationException, IOException {
+    DocumentBuilderFactory factory = XMLUtil.newXXEProtectedDocumentBuilderFactory();
+    DocumentBuilder safeBuilder = factory.newDocumentBuilder();
+
+    File file = ManagedFileAccess.file("src/test/resources/xml/evil-resource.xml");
+
+    SAXParseException e = assertThrows(SAXParseException.class, ()-> safeBuilder.parse(file));
+    assertThat(e.getMessage()).contains("DOCTYPE is disallowed");
+  }
+
+  @Test
+  public void testTransformerFactoryThrowsExceptionForExternalEntity() throws ParserConfigurationException, IOException, SAXException, TransformerException {
+    DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+    DocumentBuilder safeBuilder = factory.newDocumentBuilder();
+
+    File file = ManagedFileAccess.file("src/test/resources/xml/resource.xml");
+
+    Document document = safeBuilder.parse(file);
+
+    StringWriter sw = new StringWriter();
+    TransformerFactory tf = XMLUtil.newXXEProtectedTransformerFactory();
+
+    File templateFile = ManagedFileAccess.file("src/test/resources/xml/evil-transform.xslt");
+    Source xsltSource = new StreamSource(templateFile);
+    Transformer transformer = tf.newTransformer(xsltSource);
+    transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no");
+    transformer.setOutputProperty(OutputKeys.METHOD, "xml");
+    transformer.setOutputProperty(OutputKeys.INDENT, "yes");
+    transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-8");
+
+    XPathException e = assertThrows(XPathException.class, () -> transformer.transform(new DOMSource(document), new StreamResult(sw)));
+    assertThat(e.getMessage()).contains("URIs using protocol file are not permitted");
+  }
+
+  @Test
+  public void testSaxParserFactoryThrowsExceptionForExternalEntity() throws ParserConfigurationException, IOException, SAXException, TransformerException {
+
+    SAXParserFactory spf = XMLUtil.newXXEProtectedSaxParserFactory();
+    spf.setNamespaceAware(true);
+    spf.setValidating(false);
+    XMLReader xmlReader = spf.newSAXParser().getXMLReader();
+
+    File templateFile = ManagedFileAccess.file("src/test/resources/xml/evil-resource.xml");
+
+    SAXParseException e = assertThrows(SAXParseException.class, () -> xmlReader.parse(new StreamSource(templateFile).getSystemId()));
+    assertThat(e.getMessage()).contains("DOCTYPE is disallowed");
+  }
+}

org.hl7.fhir.utilities/src/test/resources/xml/evil-resource.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE foo [<!ENTITY example SYSTEM "jackpot.xml"> ]>
+<Patient xmlns="http://hl7.org/fhir">
+    <id value="example"/>
+    <text> <status value="generated"/> &example;</text>
+</Patient>

org.hl7.fhir.utilities/src/test/resources/xml/evil-transform.xslt

@@ -0,0 +1,6 @@
+<xsl:stylesheet version="1.0"
+                xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
+    <xsl:template match="/">
+        Evil: <xsl:value-of select="document('jackpot.xml')" />
+    </xsl:template>
+</xsl:stylesheet>

org.hl7.fhir.utilities/src/test/resources/xml/jackpot.xml

@@ -0,0 +1 @@
+<stuff>stuff</stuff>

org.hl7.fhir.utilities/src/test/resources/xml/resource.xml

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<Patient xmlns="http://hl7.org/fhir">
+    <id value="example"/>
+</Patient>