Harden XML parsing via copernik-xml-factory

pom.xml

@@ -96,6 +96,11 @@
     </site>
   </distributionManagement>
   <dependencies>
+    <dependency>
+      <groupId>eu.copernik</groupId>
+      <artifactId>copernik-xml-factory</artifactId>
+      <version>0.1.1</version>
+    </dependency>
     <dependency>
       <groupId>org.apache.commons</groupId>
       <artifactId>commons-lang3</artifactId>

src/main/java/org/apache/commons/configuration2/XMLConfiguration.java

@@ -67,6 +67,8 @@
 import org.xml.sax.SAXParseException;
 import org.xml.sax.helpers.DefaultHandler;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * <p>
  * A specialized hierarchical configuration class that is able to parse XML documents.
@@ -526,7 +528,17 @@ private static boolean shouldTrim(final Element element, final boolean currentTr
     private boolean schemaValidation;
 
     /** The EntityResolver to use. */
-    private EntityResolver entityResolver = new DefaultEntityResolver();
+    private EntityResolver entityResolver = new DefaultEntityResolver() {
+        @Override
+        public InputSource resolveEntity(String publicId, String systemId) throws SAXException {
+            InputSource inputSource = super.resolveEntity(publicId, systemId);
+            if (inputSource != null) {
+                return inputSource;
+            }
+            throw new SAXException(String.format(
+                    "Refused to resolve external entity with public ID [%s] and system ID [%s]: no local resource is registered for it.", publicId, systemId));
+        }
+    };
 
     /** The current file locator. */
     private FileLocator locator;
@@ -693,7 +705,7 @@ protected DocumentBuilder createDocumentBuilder() throws ParserConfigurationExce
         if (getDocumentBuilder() != null) {
             return getDocumentBuilder();
         }
-        final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory();
         if (isValidating()) {
             factory.setValidating(true);
             if (isSchemaValidation()) {

src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java

@@ -38,6 +38,8 @@
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * <p>
  * An internally used helper class for dealing with XML documents.
@@ -92,15 +94,6 @@ static DocumentBuilder createDocumentBuilder(final DocumentBuilderFactory factor
         }
     }
 
-    /**
-     * Creates a new {@code DocumentBuilderFactory} instance.
-     *
-     * @return the new factory object
-     */
-    private static DocumentBuilderFactory createDocumentBuilderFactory() {
-        return DocumentBuilderFactory.newInstance();
-    }
-
     /**
      * Creates the element mapping for the specified documents. For each node in the source document an entry is created
      * pointing to the corresponding node in the destination object.
@@ -163,7 +156,7 @@ static Transformer createTransformer(final TransformerFactory factory) throws Co
      * @return the {@code TransformerFactory}
      */
     static TransformerFactory createTransformerFactory() {
-        return TransformerFactory.newInstance();
+        return XmlFactories.newTransformerFactory();
     }
 
     /**
@@ -184,7 +177,7 @@ private static Map<Node, Node> emptyElementMapping() {
      * @throws ConfigurationException if an error occurs when creating the document
      */
     public static XMLDocumentHelper forNewDocument(final String rootElementName) throws ConfigurationException {
-        final Document doc = createDocumentBuilder(createDocumentBuilderFactory()).newDocument();
+        final Document doc = createDocumentBuilder(XmlFactories.newDocumentBuilderFactory()).newDocument();
         final Element rootElem = doc.createElement(rootElementName);
         doc.appendChild(rootElem);
         return new XMLDocumentHelper(doc, emptyElementMapping(), null, null);
@@ -230,7 +223,7 @@ public static XMLDocumentHelper forSourceDocument(final Document srcDoc) throws
      */
     public static void transform(final Transformer transformer, final Source source, final Result result) throws ConfigurationException {
         try {
-            transformer.transform(source, result);
+            transformer.transform(XmlFactories.harden(source), result);
         } catch (final TransformerException tex) {
             throw new ConfigurationException(tex);
         }

src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java

@@ -41,6 +41,8 @@
 import org.xml.sax.XMLReader;
 import org.xml.sax.helpers.DefaultHandler;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * This configuration implements the XML properties format introduced in Java, see
  * https://docs.oracle.com/javase/8/docs/api/java/util/Properties.html. An XML properties file looks like this:
@@ -221,7 +223,7 @@ public void load(final Element element) throws ConfigurationException {
 
     @Override
     public void read(final Reader in) throws ConfigurationException {
-        final SAXParserFactory factory = SAXParserFactory.newInstance();
+        final SAXParserFactory factory = XmlFactories.newSAXParserFactory();
         factory.setNamespaceAware(false);
         factory.setValidating(true);
         try {

src/main/java/org/apache/commons/configuration2/plist/XMLPropertyListConfiguration.java

@@ -62,6 +62,8 @@
 import org.xml.sax.XMLReader;
 import org.xml.sax.helpers.DefaultHandler;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * Property list file (plist) in XML FORMAT as used by macOS X (http://www.apple.com/DTDs/PropertyList-1.0.dtd). This
  * configuration doesn't support the binary FORMAT used in OS X 10.4.
@@ -651,7 +653,7 @@ public void read(final Reader in) throws ConfigurationException {
         // parse the file
         final XMLPropertyListHandler handler = new XMLPropertyListHandler();
         try {
-            final SAXParserFactory factory = SAXParserFactory.newInstance();
+            final SAXParserFactory factory = XmlFactories.newSAXParserFactory();
             factory.setValidating(true);
             final XMLReader xmlReader = factory.newSAXParser().getXMLReader();
             xmlReader.setEntityResolver(resolver);

src/test/java/org/apache/commons/configuration2/TestBaseConfigurationXMLReader.java

@@ -27,7 +27,6 @@
 import java.util.Iterator;
 
 import javax.xml.transform.Transformer;
-import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMResult;
 import javax.xml.transform.sax.SAXSource;
 
@@ -40,6 +39,8 @@
 import org.xml.sax.SAXException;
 import org.xml.sax.helpers.DefaultHandler;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * Test class for BaseConfigurationXMLReader.
  */
@@ -80,7 +81,7 @@ private void check(final JXPathContext ctx, final String path, final String[] va
     private void checkDocument(final BaseConfigurationXMLReader creader, final String rootName) throws Exception {
         final SAXSource source = new SAXSource(creader, new InputSource());
         final DOMResult result = new DOMResult();
-        final Transformer trans = TransformerFactory.newInstance().newTransformer();
+        final Transformer trans = XmlFactories.newTransformerFactory().newTransformer();
         trans.transform(source, result);
         final Node root = ((Document) result.getNode()).getDocumentElement();
         final JXPathContext ctx = JXPathContext.newContext(root);

src/test/java/org/apache/commons/configuration2/TestHierarchicalConfigurationXMLReader.java

@@ -20,7 +20,6 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import javax.xml.transform.Transformer;
-import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMResult;
 import javax.xml.transform.sax.SAXSource;
 
@@ -33,6 +32,8 @@
 import org.w3c.dom.Node;
 import org.xml.sax.InputSource;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * Test class for HierarchicalConfigurationXMLReader.
  */
@@ -53,7 +54,7 @@ public void setUp() throws Exception {
     void testParse() throws Exception {
         final SAXSource source = new SAXSource(parser, new InputSource());
         final DOMResult result = new DOMResult();
-        final Transformer trans = TransformerFactory.newInstance().newTransformer();
+        final Transformer trans = XmlFactories.newTransformerFactory().newTransformer();
         trans.transform(source, result);
         final Node root = ((Document) result.getNode()).getDocumentElement();
         final JXPathContext ctx = JXPathContext.newContext(root);

src/test/java/org/apache/commons/configuration2/TestXMLConfiguration.java

@@ -74,6 +74,8 @@
 import org.xml.sax.SAXParseException;
 import org.xml.sax.helpers.DefaultHandler;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * test for loading and saving XML properties files
  */
@@ -155,7 +157,7 @@ private static byte[] nodeToByteArray(final Node node) throws TransformerExcepti
         final Source source = new DOMSource(node);
         final ByteArrayOutputStream bos = new ByteArrayOutputStream();
         final Result result = new StreamResult(bos);
-        final TransformerFactory factory = TransformerFactory.newInstance();
+        final TransformerFactory factory = XmlFactories.newTransformerFactory();
         factory.newTransformer().transform(source, result);
         // 4. Return the resulting byte array
         return bos.toByteArray();
@@ -184,7 +186,7 @@ private Element buildDomElementFixture() throws SAXException, IOException, Parse
 
     private Node buildDomNodeFixture() throws SAXException, IOException, ParserConfigurationException {
         final String content = "<configuration><test attr=\"x\">1</test></configuration>";
-        final Node document = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new ByteArrayInputStream(content.getBytes()));
+        final Node document = XmlFactories.newDocumentBuilderFactory().newDocumentBuilder().parse(new ByteArrayInputStream(content.getBytes()));
         final Node node = document.getFirstChild().getFirstChild(); // <test>
         assertEquals("test", node.getNodeName()); // sanity check
         return node;
@@ -239,7 +241,7 @@ private void checkSaveDelimiterParsingDisabled(final String key) throws Configur
      * @throws ParserConfigurationException if an error occurs
      */
     private DocumentBuilder createValidatingDocBuilder() throws ParserConfigurationException {
-        final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory();
         factory.setValidating(true);
         final DocumentBuilder builder = factory.newDocumentBuilder();
         builder.setErrorHandler(new DefaultHandler() {
@@ -252,7 +254,7 @@ public void error(final SAXParseException ex) throws SAXException {
     }
 
     private Document parseXml(final String xml) throws SAXException, IOException, ParserConfigurationException {
-        return DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)));
+        return XmlFactories.newDocumentBuilderFactory().newDocumentBuilder().parse(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)));
     }
 
     /**

src/test/java/org/apache/commons/configuration2/TestXMLDocumentHelper.java

@@ -53,6 +53,8 @@
 import org.w3c.dom.Text;
 import org.xml.sax.SAXException;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * Test class for {@code XMLDocumentHelper}.
  */
@@ -134,7 +136,7 @@ private static Document loadDocument() throws ParserConfigurationException, IOEx
      * @return the parsed document
      */
     private static Document loadDocument(final String name) throws IOException, SAXException, ParserConfigurationException {
-        final DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
+        final DocumentBuilder builder = XmlFactories.newDocumentBuilderFactory().newDocumentBuilder();
         return builder.parse(ConfigurationAssert.getTestFile(name));
     }
 

src/test/java/org/apache/commons/configuration2/TestXMLPropertiesConfiguration.java

@@ -40,6 +40,8 @@
 import org.w3c.dom.Document;
 import org.xml.sax.InputSource;
 
+import eu.copernik.xml.factory.XmlFactories;
+
 /**
  * Test class for {@code XMLPropertiesConfiguration}.
  */
@@ -72,7 +74,7 @@ void testDOMLoad() throws Exception {
         assertThrows(NullPointerException.class, () -> new XMLPropertiesConfiguration(null));
         // Normal case
         final URL location = ConfigurationAssert.getTestURL(TEST_PROPERTIES_FILE);
-        final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
+        final DocumentBuilderFactory dbFactory = XmlFactories.newDocumentBuilderFactory();
         final DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
         dBuilder.setEntityResolver((publicId, systemId) -> new InputSource(getClass().getClassLoader().getResourceAsStream("properties.dtd")));
         final File file = new File(location.toURI());
@@ -101,11 +103,11 @@ void testDOMSave() throws Exception {
         final File saveFile = newFile("test2.properties.xml", tempFolder);
 
         // save as DOM into saveFile
-        final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
+        final DocumentBuilderFactory dbFactory = XmlFactories.newDocumentBuilderFactory();
         final DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
         final Document document = dBuilder.newDocument();
         conf.save(document, document);
-        final TransformerFactory tFactory = TransformerFactory.newInstance();
+        final TransformerFactory tFactory = XmlFactories.newTransformerFactory();
         final Transformer transformer = tFactory.newTransformer();
         final DOMSource source = new DOMSource(document);
         final Result result = new StreamResult(saveFile);

src/test/java/org/apache/commons/configuration2/builder/combined/TestMultiFileConfigurationBuilder.java

@@ -29,6 +29,7 @@
 import java.util.Collection;
 import java.util.Collections;
 
+import org.apache.commons.configuration2.ConfigurationAssert;
 import org.apache.commons.configuration2.ConfigurationLookup;
 import org.apache.commons.configuration2.DynamicCombinedConfiguration;
 import org.apache.commons.configuration2.HierarchicalConfiguration;
@@ -49,6 +50,7 @@
 import org.apache.commons.configuration2.ex.ConfigurationException;
 import org.apache.commons.configuration2.interpol.ConfigurationInterpolator;
 import org.apache.commons.configuration2.interpol.DefaultLookups;
+import org.apache.commons.configuration2.resolver.CatalogResolver;
 import org.apache.commons.configuration2.tree.ExpressionEngine;
 import org.apache.commons.configuration2.tree.xpath.XPathExpressionEngine;
 import org.junit.jupiter.api.Test;
@@ -334,8 +336,13 @@ void testRemoveBuilderListenerOnReset() throws ConfigurationException {
      */
     @Test
     void testSchemaValidationError() {
+        // The testMultiConfiguration_2001.xml configuration references its schema through an absolute https URI.
+        // The hardened parser does not fetch external resources,
+        // so register the local testMultiConfiguration.xsd for that system URI via an XML catalog.
+        final CatalogResolver resolver = new CatalogResolver();
+        resolver.setCatalogFiles(ConfigurationAssert.getTestFile("catalog.xml").getAbsolutePath());
         final MultiFileConfigurationBuilder<XMLConfiguration> builder = createTestBuilder(
-            new XMLBuilderParametersImpl().setValidating(true).setSchemaValidation(true));
+            new XMLBuilderParametersImpl().setValidating(true).setSchemaValidation(true).setEntityResolver(resolver));
         switchToConfig("2001");
         final ConfigurationException ex = assertThrows(ConfigurationException.class, builder::getConfiguration);
         Throwable cause = ex.getCause();