From ace8aaf9f928556afe2793a1330cd178ec34c0ab Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 18 Jun 2026 09:49:13 +0200 Subject: [PATCH 1/2] Harden XML parsing via copernik-xml-factory Replace all direct JAXP factory instantiations (DocumentBuilderFactory, SAXParserFactory, TransformerFactory) with the hardened factories from eu.copernik:copernik-xml-factory 0.1.1. These factories block external DTD and entity fetching and bound internal entity expansion, regardless of the JAXP implementation on the classpath. Hardening the parsing of a configuration file is admittedly not necessary: configuration files are normally trusted. This limits the side-effects if a user (against advice) decides to parse untrusted configuration files. Changes: - Add the copernik-xml-factory dependency. - Route factory creation through XmlFactories in XMLConfiguration, XMLDocumentHelper, XMLPropertiesConfiguration and XMLPropertyListConfiguration, plus the affected tests. - Harden the source passed to XMLDocumentHelper.transform. - XMLConfiguration's entity resolver now throws a SAXException for unregistered entities instead of returning null. Returning null would let the parser fall back to fetching the external resource and would override the deny-all resolver the hardening factories install on some implementations. This is done in an anonymous DefaultEntityResolver subclass local to XMLConfiguration, leaving the public DefaultEntityResolver contract (return null for unknown entities) unchanged. Assisted-By: Claude Opus 4.8 (1M context) --- pom.xml | 5 +++++ .../configuration2/XMLConfiguration.java | 16 ++++++++++++++-- .../configuration2/XMLDocumentHelper.java | 17 +++++------------ .../XMLPropertiesConfiguration.java | 4 +++- .../plist/XMLPropertyListConfiguration.java | 4 +++- .../TestBaseConfigurationXMLReader.java | 5 +++-- .../TestHierarchicalConfigurationXMLReader.java | 5 +++-- .../configuration2/TestXMLConfiguration.java | 10 ++++++---- .../configuration2/TestXMLDocumentHelper.java | 4 +++- .../TestXMLPropertiesConfiguration.java | 8 +++++--- 10 files changed, 50 insertions(+), 28 deletions(-) diff --git a/pom.xml b/pom.xml index 555508c079..b454cf1675 100644 --- a/pom.xml +++ b/pom.xml @@ -96,6 +96,11 @@ + + eu.copernik + copernik-xml-factory + 0.1.1 + org.apache.commons commons-lang3 diff --git a/src/main/java/org/apache/commons/configuration2/XMLConfiguration.java b/src/main/java/org/apache/commons/configuration2/XMLConfiguration.java index 0cb9d73651..dccdf6beff 100644 --- a/src/main/java/org/apache/commons/configuration2/XMLConfiguration.java +++ b/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; + /** *

* 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()) { diff --git a/src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java b/src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java index 95584f6c2a..97cd527d57 100644 --- a/src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java +++ b/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; + /** *

* 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 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); } diff --git a/src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java b/src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java index daa7b22af5..81eb545273 100644 --- a/src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java +++ b/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 { diff --git a/src/main/java/org/apache/commons/configuration2/plist/XMLPropertyListConfiguration.java b/src/main/java/org/apache/commons/configuration2/plist/XMLPropertyListConfiguration.java index 2520e6258c..3af84720c8 100644 --- a/src/main/java/org/apache/commons/configuration2/plist/XMLPropertyListConfiguration.java +++ b/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); diff --git a/src/test/java/org/apache/commons/configuration2/TestBaseConfigurationXMLReader.java b/src/test/java/org/apache/commons/configuration2/TestBaseConfigurationXMLReader.java index cbdc85a2f2..7315603fb2 100644 --- a/src/test/java/org/apache/commons/configuration2/TestBaseConfigurationXMLReader.java +++ b/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); diff --git a/src/test/java/org/apache/commons/configuration2/TestHierarchicalConfigurationXMLReader.java b/src/test/java/org/apache/commons/configuration2/TestHierarchicalConfigurationXMLReader.java index ad801f7a87..53e73dfd82 100644 --- a/src/test/java/org/apache/commons/configuration2/TestHierarchicalConfigurationXMLReader.java +++ b/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); diff --git a/src/test/java/org/apache/commons/configuration2/TestXMLConfiguration.java b/src/test/java/org/apache/commons/configuration2/TestXMLConfiguration.java index efbbffa9ee..7c4b5d5a1c 100644 --- a/src/test/java/org/apache/commons/configuration2/TestXMLConfiguration.java +++ b/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 = "1"; - 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(); // 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))); } /** diff --git a/src/test/java/org/apache/commons/configuration2/TestXMLDocumentHelper.java b/src/test/java/org/apache/commons/configuration2/TestXMLDocumentHelper.java index 2e0f410c63..b2b83e1f0f 100644 --- a/src/test/java/org/apache/commons/configuration2/TestXMLDocumentHelper.java +++ b/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)); } diff --git a/src/test/java/org/apache/commons/configuration2/TestXMLPropertiesConfiguration.java b/src/test/java/org/apache/commons/configuration2/TestXMLPropertiesConfiguration.java index 0db1ba2cef..c998103674 100644 --- a/src/test/java/org/apache/commons/configuration2/TestXMLPropertiesConfiguration.java +++ b/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); From 2ae32064c062bd335ec99bfb70b236079ddf8987 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 18 Jun 2026 10:52:50 +0200 Subject: [PATCH 2/2] Resolve schema locally in TestMultiFileConfigurationBuilder The 2001 test configuration references its schema through an absolute https URI (xsi:noNamespaceSchemaLocation). The hardened parser does not fetch external resources, so testSchemaValidationError no longer reached the intended schema validation error; the entity resolver refused the remote fetch first. Register the local testMultiConfiguration.xsd for that system URI via an XML catalog (CatalogResolver pointing at the existing catalog.xml, which already rewrites https://commons.apache.org/ to the local test resources) and set it as the builder's entity resolver. The schema now loads locally, so the expected SAXParseException validation error is raised again. Assisted-By: Claude Opus 4.8 (1M context) --- .../combined/TestMultiFileConfigurationBuilder.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/apache/commons/configuration2/builder/combined/TestMultiFileConfigurationBuilder.java b/src/test/java/org/apache/commons/configuration2/builder/combined/TestMultiFileConfigurationBuilder.java index d6563154c3..c370c47641 100644 --- a/src/test/java/org/apache/commons/configuration2/builder/combined/TestMultiFileConfigurationBuilder.java +++ b/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 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();