This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch feat/use-commons-xml in repository https://gitbox.apache.org/repos/asf/commons-configuration.git
commit 47425633db08ac9c268c9700f5aeaca760daab96 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Sun Aug 30 22:25:52 2026 +0200 Harden XML parsing via commons-xml Replace all direct JAXP factory instantiations (DocumentBuilderFactory, SAXParserFactory, TransformerFactory) with the secure factories from org.apache.commons:commons-xml. These factories enable FEATURE_SECURE_PROCESSING and install a non-removable entity-resolver floor on every parser they produce: external DTD, entity, schema and XInclude lookups that a caller-set resolver does not resolve are resolved to empty content instead of being fetched, and internal entity expansion is bounded, 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 commons-xml dependency (1.0.0-SNAPSHOT until its first release). - Route factory creation through SecureDocumentBuilderFactory, SecureSAXParserFactory and SecureTransformerFactory in XMLConfiguration, XMLDocumentHelper, XMLPropertiesConfiguration and XMLPropertyListConfiguration, plus the affected tests. - No explicit hardening of the source passed to XMLDocumentHelper.transform is needed: transformers created by SecureTransformerFactory rewrite their sources on every transform call. - XMLConfiguration keeps its DefaultEntityResolver contract (return null for unknown entities): a null return no longer lets the parser fetch the external resource, because the resolver floor resolves it to empty content instead. - Parse with EntityResolver2 handling disabled (http://xml.org/sax/features/use-entity-resolver2) when schema validation is enabled: the JDK does not mark schema documents supplied by an EntityResolver2 as resolver-created, so the accessExternalSchema check enabled by secure processing refuses them even when a caller-set resolver (such as CatalogResolver) resolves them locally. The plain EntityResolver path marks them correctly and keeps resolver-based schema validation working. - Run the CI build with -Puse-apache-snapshots (inherited from the org.apache:apache parent POM) so the commons-xml SNAPSHOT resolves. Assisted-By: Claude Opus 4.8 (1M context) <[email protected]> Assisted-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01NS6CoaDG2mfSNpy4Ukhvrn --- .github/workflows/maven.yml | 2 +- pom.xml | 5 +++++ .../apache/commons/configuration2/XMLConfiguration.java | 11 ++++++++++- .../apache/commons/configuration2/XMLDocumentHelper.java | 15 ++++----------- .../configuration2/XMLPropertiesConfiguration.java | 3 ++- .../plist/XMLPropertyListConfiguration.java | 3 ++- .../configuration2/TestBaseConfigurationXMLReader.java | 4 ++-- .../TestHierarchicalConfigurationXMLReader.java | 4 ++-- .../commons/configuration2/TestXMLConfiguration.java | 10 ++++++---- .../commons/configuration2/TestXMLDocumentHelper.java | 3 ++- .../configuration2/TestXMLPropertiesConfiguration.java | 8 +++++--- 11 files changed, 41 insertions(+), 27 deletions(-) diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 61791213a..99cebcac0 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -49,6 +49,6 @@ jobs: java-version: ${{ matrix.java }} cache: 'maven' - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress + run: mvn --errors --show-version --batch-mode --no-transfer-progress -Puse-apache-snapshots # For Java 11, you can be more strict: -DadditionalJOption=-Xdoclint/package:-org.apache.commons.configuration2.plist diff --git a/pom.xml b/pom.xml index 190a769ff..320159ba7 100644 --- a/pom.xml +++ b/pom.xml @@ -96,6 +96,11 @@ </site> </distributionManagement> <dependencies> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-xml</artifactId> + <version>1.0.0-SNAPSHOT</version> + </dependency> <dependency> <groupId>org.apache.commons</groupId> <artifactId>commons-lang3</artifactId> diff --git a/src/main/java/org/apache/commons/configuration2/XMLConfiguration.java b/src/main/java/org/apache/commons/configuration2/XMLConfiguration.java index fb97d08f3..eca89a537 100644 --- a/src/main/java/org/apache/commons/configuration2/XMLConfiguration.java +++ b/src/main/java/org/apache/commons/configuration2/XMLConfiguration.java @@ -53,6 +53,7 @@ import org.apache.commons.configuration2.tree.NodeTreeWalker; import org.apache.commons.configuration2.tree.ReferenceNodeHandler; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.mutable.MutableObject; +import org.apache.commons.xml.SecureDocumentBuilderFactory; import org.w3c.dom.Attr; import org.w3c.dom.CDATASection; import org.w3c.dom.Document; @@ -693,12 +694,20 @@ public class XMLConfiguration extends BaseHierarchicalConfiguration implements F if (getDocumentBuilder() != null) { return getDocumentBuilder(); } - final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + final DocumentBuilderFactory factory = SecureDocumentBuilderFactory.newInstance(); if (isValidating()) { factory.setValidating(true); if (isSchemaValidation()) { factory.setNamespaceAware(true); factory.setAttribute(JAXP_SCHEMA_LANGUAGE, W3C_XML_SCHEMA); + try { + // Due to a bug, the JDK fails to mark schema documents supplied by an EntityResolver2 as resolver-created, + // so the accessExternalSchema check is applied to the resolved source and denies access. + // Downgrading the resolver to a plain EntityResolver works around the check. + factory.setFeature("http://xml.org/sax/features/use-entity-resolver2";, false); + } catch (final ParserConfigurationException e) { + // SAX-specific feature: parsers that do not recognize it keep EntityResolver2 handling. + } } } diff --git a/src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java b/src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java index 3c570e086..62d2ec966 100644 --- a/src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java +++ b/src/main/java/org/apache/commons/configuration2/XMLDocumentHelper.java @@ -33,6 +33,8 @@ import javax.xml.transform.dom.DOMResult; import javax.xml.transform.dom.DOMSource; import org.apache.commons.configuration2.ex.ConfigurationException; +import org.apache.commons.xml.SecureDocumentBuilderFactory; +import org.apache.commons.xml.SecureTransformerFactory; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.Node; @@ -92,15 +94,6 @@ final class XMLDocumentHelper { } } - /** - * 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 @@ final class XMLDocumentHelper { * @return The {@code TransformerFactory} */ static TransformerFactory createTransformerFactory() { - return TransformerFactory.newInstance(); + return SecureTransformerFactory.newInstance(); } /** @@ -184,7 +177,7 @@ final class XMLDocumentHelper { * @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(SecureDocumentBuilderFactory.newInstance()).newDocument(); final Element rootElem = doc.createElement(rootElementName); doc.appendChild(rootElem); return new XMLDocumentHelper(doc, emptyElementMapping(), null, null); diff --git a/src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java b/src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java index d5b217e63..8ba9696bd 100644 --- a/src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java +++ b/src/main/java/org/apache/commons/configuration2/XMLPropertiesConfiguration.java @@ -32,6 +32,7 @@ import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.configuration2.io.FileLocator; import org.apache.commons.configuration2.io.FileLocatorAware; import org.apache.commons.text.StringEscapeUtils; +import org.apache.commons.xml.SecureSAXParserFactory; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.Node; @@ -221,7 +222,7 @@ public class XMLPropertiesConfiguration extends BaseConfiguration implements Fil @Override public void read(final Reader in) throws ConfigurationException { - final SAXParserFactory factory = SAXParserFactory.newInstance(); + final SAXParserFactory factory = SecureSAXParserFactory.newInstance(); 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 04b415cca..0fb394d1c 100644 --- a/src/main/java/org/apache/commons/configuration2/plist/XMLPropertyListConfiguration.java +++ b/src/main/java/org/apache/commons/configuration2/plist/XMLPropertyListConfiguration.java @@ -55,6 +55,7 @@ import org.apache.commons.configuration2.tree.ImmutableNode; import org.apache.commons.configuration2.tree.InMemoryNodeModel; import org.apache.commons.lang3.StringUtils; import org.apache.commons.text.StringEscapeUtils; +import org.apache.commons.xml.SecureSAXParserFactory; import org.xml.sax.Attributes; import org.xml.sax.EntityResolver; import org.xml.sax.InputSource; @@ -661,7 +662,7 @@ public class XMLPropertyListConfiguration extends BaseHierarchicalConfiguration // parse the file final XMLPropertyListHandler handler = new XMLPropertyListHandler(); try { - final SAXParserFactory factory = SAXParserFactory.newInstance(); + final SAXParserFactory factory = SecureSAXParserFactory.newInstance(); 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 6ce26a9a9..01c32ac43 100644 --- a/src/test/java/org/apache/commons/configuration2/TestBaseConfigurationXMLReader.java +++ b/src/test/java/org/apache/commons/configuration2/TestBaseConfigurationXMLReader.java @@ -27,11 +27,11 @@ import java.util.Arrays; 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; import org.apache.commons.jxpath.JXPathContext; +import org.apache.commons.xml.SecureTransformerFactory; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.w3c.dom.Document; @@ -80,7 +80,7 @@ public class TestBaseConfigurationXMLReader { 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 = SecureTransformerFactory.newInstance().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 ad801f7a8..875f44604 100644 --- a/src/test/java/org/apache/commons/configuration2/TestHierarchicalConfigurationXMLReader.java +++ b/src/test/java/org/apache/commons/configuration2/TestHierarchicalConfigurationXMLReader.java @@ -20,13 +20,13 @@ package org.apache.commons.configuration2; 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; import org.apache.commons.configuration2.io.FileHandler; import org.apache.commons.configuration2.tree.ImmutableNode; import org.apache.commons.jxpath.JXPathContext; +import org.apache.commons.xml.SecureTransformerFactory; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.w3c.dom.Document; @@ -53,7 +53,7 @@ public class TestHierarchicalConfigurationXMLReader { 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 = SecureTransformerFactory.newInstance().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 279216662..c3fb77ed2 100644 --- a/src/test/java/org/apache/commons/configuration2/TestXMLConfiguration.java +++ b/src/test/java/org/apache/commons/configuration2/TestXMLConfiguration.java @@ -64,6 +64,8 @@ import org.apache.commons.configuration2.tree.ImmutableNode; import org.apache.commons.configuration2.tree.NodeStructureHelper; import org.apache.commons.configuration2.tree.xpath.XPathExpressionEngine; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.xml.SecureDocumentBuilderFactory; +import org.apache.commons.xml.SecureTransformerFactory; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -155,7 +157,7 @@ public class TestXMLConfiguration { 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 = SecureTransformerFactory.newInstance(); factory.newTransformer().transform(source, result); // 4. Return the resulting byte array return bos.toByteArray(); @@ -184,7 +186,7 @@ public class TestXMLConfiguration { 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 = SecureDocumentBuilderFactory.newInstance().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 @@ public class TestXMLConfiguration { * @throws ParserConfigurationException if an error occurs */ private DocumentBuilder createValidatingDocBuilder() throws ParserConfigurationException { - final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + final DocumentBuilderFactory factory = SecureDocumentBuilderFactory.newInstance(); factory.setValidating(true); final DocumentBuilder builder = factory.newDocumentBuilder(); builder.setErrorHandler(new DefaultHandler() { @@ -252,7 +254,7 @@ public class TestXMLConfiguration { } private Document parseXml(final String xml) throws SAXException, IOException, ParserConfigurationException { - return DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8))); + return SecureDocumentBuilderFactory.newInstance().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 35cff0a22..50b6f3298 100644 --- a/src/test/java/org/apache/commons/configuration2/TestXMLDocumentHelper.java +++ b/src/test/java/org/apache/commons/configuration2/TestXMLDocumentHelper.java @@ -45,6 +45,7 @@ import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; import org.apache.commons.configuration2.ex.ConfigurationException; +import org.apache.commons.xml.SecureDocumentBuilderFactory; import org.junit.jupiter.api.Test; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -134,7 +135,7 @@ public class TestXMLDocumentHelper { * @return The parsed document */ private static Document loadDocument(final String name) throws IOException, SAXException, ParserConfigurationException { - final DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder(); + final DocumentBuilder builder = SecureDocumentBuilderFactory.newInstance().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 b7e83751d..8304161c0 100644 --- a/src/test/java/org/apache/commons/configuration2/TestXMLPropertiesConfiguration.java +++ b/src/test/java/org/apache/commons/configuration2/TestXMLPropertiesConfiguration.java @@ -35,6 +35,8 @@ import javax.xml.transform.stream.StreamResult; import org.apache.commons.configuration2.ex.ConfigurationException; import org.apache.commons.configuration2.io.FileHandler; +import org.apache.commons.xml.SecureDocumentBuilderFactory; +import org.apache.commons.xml.SecureTransformerFactory; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.w3c.dom.Document; @@ -72,7 +74,7 @@ public class TestXMLPropertiesConfiguration { assertThrows(NullPointerException.class, () -> new XMLPropertiesConfiguration(null)); // Normal case final URL location = ConfigurationAssert.getTestURL(TEST_PROPERTIES_FILE); - final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); + final DocumentBuilderFactory dbFactory = SecureDocumentBuilderFactory.newInstance(); 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 @@ public class TestXMLPropertiesConfiguration { final File saveFile = newFile("test2.properties.xml", tempFolder); // save as DOM into saveFile - final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); + final DocumentBuilderFactory dbFactory = SecureDocumentBuilderFactory.newInstance(); final DocumentBuilder dBuilder = dbFactory.newDocumentBuilder(); final Document document = dBuilder.newDocument(); conf.save(document, document); - final TransformerFactory tFactory = TransformerFactory.newInstance(); + final TransformerFactory tFactory = SecureTransformerFactory.newInstance(); final Transformer transformer = tFactory.newTransformer(); final DOMSource source = new DOMSource(document); final Result result = new StreamResult(saveFile);
