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-text.git
commit 68e8cc393f638c63798cd01a38deb47b8b6b4e22 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Mon Aug 31 15:54:18 2026 +0200 Harden XML parsing via commons-secure-xml Create XmlStringLookup's document builder and XPath factories through org.apache.commons:commons-secure-xml. The secure factories enable FEATURE_SECURE_PROCESSING and install a non-removable entity-resolver floor on every parser they produce: external DTD and entity lookups are resolved to empty content instead of being fetched, and internal entity expansion is bounded, regardless of the JAXP implementation on the classpath. Changes: - Add the commons-secure-xml dependency (1.0.0-SNAPSHOT until its first release). - Route factory creation through SecureDocumentBuilderFactory and SecureXPathFactory in XmlStringLookup on the default path. The documented XmlStringLookup.secure=false system property keeps its meaning: that path still uses the plain JAXP factories, so external entity resolution can be restored where it is wanted. - Adapt the secure-path tests to the secure contract: a parser may either reject a document with an external reference or parse it with the reference resolved to empty content; the tests now assert that the external content does not leak into the result instead of expecting one fixed failure mode. - Run the CI and CodeQL builds with -Puse-apache-snapshots (inherited from the org.apache:apache parent POM) so the commons-secure-xml SNAPSHOT resolves; CodeQL's autobuild receives the profile through MAVEN_ARGS. Assisted-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MHgnMnGWHQoH2zD2jFdoMT --- .github/workflows/codeql-analysis.yml | 2 ++ .github/workflows/maven.yml | 2 +- pom.xml | 5 ++++ src/changes/changes.xml | 1 + .../commons/text/lookup/XmlStringLookup.java | 7 ++++-- .../text/lookup/StringLookupFactoryTest.java | 5 ++-- .../commons/text/lookup/XmlStringLookupTest.java | 29 ++++++++++++++++------ 7 files changed, 38 insertions(+), 13 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index b47769e0..82099bd2 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -69,6 +69,8 @@ jobs: # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild uses: github/codeql-action/autobuild@fe4161a26a8629af62121b670040955b330f9af2 # v4.31.6 + env: + MAVEN_ARGS: -Puse-apache-snapshots # âšī¸ Command-line programs to run using the OS shell. # đ https://git.io/JvXDl diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index d2aef55f..4063d072 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -44,4 +44,4 @@ jobs: java-version: ${{ matrix.java }} cache: 'maven' - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Dpolyglot.engine.WarnInterpreterOnly=false + run: mvn --errors --show-version --batch-mode --no-transfer-progress -Dpolyglot.engine.WarnInterpreterOnly=false -Puse-apache-snapshots diff --git a/pom.xml b/pom.xml index bcab4f57..f61b1f74 100644 --- a/pom.xml +++ b/pom.xml @@ -86,6 +86,11 @@ <artifactId>commons-lang3</artifactId> <version>${commons.lang3.version}</version> </dependency> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-secure-xml</artifactId> + <version>1.0.0-SNAPSHOT</version> + </dependency> <!-- testing --> <dependency> <groupId>org.junit.jupiter</groupId> diff --git a/src/changes/changes.xml b/src/changes/changes.xml index f8912d20..bd0bb8dd 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -47,6 +47,7 @@ The <action> type attribute can be add,update,fix,remove. <body> <release version="1.15.0" date="YYYY-MM-DD" description="Release 1.15.0. This is a feature and maintenance release. Java 8 or later is required."> <!-- FIX --> + <action type="fix" dev="pkarwasz">XmlStringLookup creates its XML parser and XPath factories through org.apache.commons:commons-secure-xml; the XmlStringLookup.secure=false system property still restores the previous behavior.</action> <action type="fix" dev="ggregory" due-to="Gary Gregory">Fix exception message typo in XmlStringLookup.XmlStringLookup(Map, Path...).</action> <action type="fix" dev="ggregory" due-to="Pierre Post, Sumit Bera, Alex Herbert, Gary Gregory" issue="TEXT-236">Inserting at the end of a TextStringBuilder throws a StringIndexOutOfBoundsException.</action> <action type="fix" dev="ggregory" due-to="Michael Hausegger">Fix TextStringBuilderTest.testAppendToCharBuffer() to use proper argument type #724.</action> diff --git a/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java b/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java index 85747eeb..7978d644 100644 --- a/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java +++ b/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java @@ -31,6 +31,8 @@ import javax.xml.xpath.XPathFactory; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.SystemProperties; +import org.apache.commons.xml.secure.SecureDocumentBuilderFactory; +import org.apache.commons.xml.secure.SecureXPathFactory; import org.w3c.dom.Document; /** @@ -133,7 +135,8 @@ final class XmlStringLookup extends AbstractPathFencedLookup { final boolean secure = isSecure(); final String documentPath = keys[0]; final String xpath = StringUtils.substringAfterLast(key, SPLIT_CH); - final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); + // The secure factory installs a non-removable resolver floor; the documented opt-out keeps a plain factory. + final DocumentBuilderFactory dbFactory = secure ? SecureDocumentBuilderFactory.newInstance() : DocumentBuilderFactory.newInstance(); try { for (final Entry<String, Boolean> p : xmlFactoryFeatures.entrySet()) { dbFactory.setFeature(p.getKey(), p.getValue()); @@ -141,7 +144,7 @@ final class XmlStringLookup extends AbstractPathFencedLookup { dbFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, secure); try (InputStream inputStream = Files.newInputStream(getPath(documentPath))) { final Document doc = dbFactory.newDocumentBuilder().parse(inputStream); - final XPathFactory xpFactory = XPathFactory.newInstance(); + final XPathFactory xpFactory = secure ? SecureXPathFactory.newInstance() : XPathFactory.newInstance(); for (final Entry<String, Boolean> p : xPathFactoryFeatures.entrySet()) { xpFactory.setFeature(p.getKey(), p.getValue()); } diff --git a/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java b/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java index bdc6cac1..a1ade0bc 100644 --- a/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java +++ b/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java @@ -268,8 +268,9 @@ class StringLookupFactoryTest { @Test void testXmlStringLookupExternalEntityOff() { - assertThrows(IllegalArgumentException.class, - () -> StringLookupFactory.INSTANCE.xmlStringLookup().apply(XmlStringLookupTest.DOC_DIR + "document-entity-ref.xml:/document/content")); + XmlStringLookupTest.assertBlocksOrDoesNotLeak( + () -> StringLookupFactory.INSTANCE.xmlStringLookup().apply(XmlStringLookupTest.DOC_DIR + "document-entity-ref.xml:/document/content"), + XmlStringLookupTest.DATA); } @Test diff --git a/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java b/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java index f0cc50ba..a34e7bb4 100644 --- a/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java +++ b/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java @@ -29,6 +29,7 @@ import java.nio.file.Paths; import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.function.Supplier; import javax.xml.XMLConstants; @@ -50,6 +51,19 @@ class XmlStringLookupTest { private static final String DOC_RELATIVE = DOC_DIR + "document.xml"; private static final String DOC_ROOT = "/document.xml"; + /** + * Asserts the secure contract for an external reference: the parser either rejects the document or parses it + * with the reference resolved to empty content, but the external content never appears in the result. + */ + static void assertBlocksOrDoesNotLeak(final Supplier<String> lookup, final String external) { + try { + final String result = lookup.get(); + assertFalse(result != null && result.contains(external), () -> "external content leaked: " + result); + } catch (final IllegalArgumentException e) { + // the parser rejected the external reference outright + } + } + static void assertLookup(final StringLookup xmlStringLookup) { assertNotNull(xmlStringLookup); assertInstanceOf(XmlStringLookup.class, xmlStringLookup); @@ -64,8 +78,8 @@ class XmlStringLookupTest { @Test void testExternalEntityOff() { - assertThrows(IllegalArgumentException.class, - () -> new XmlStringLookup(XmlStringLookup.DEFAULT_XML_FEATURES, EMPTY_MAP).apply(DOC_DIR + "document-entity-ref.xml:/document/content")); + assertBlocksOrDoesNotLeak(() -> new XmlStringLookup(XmlStringLookup.DEFAULT_XML_FEATURES, EMPTY_MAP).apply(DOC_DIR + "document-entity-ref.xml:/document/content"), + DATA); } @Test @@ -79,8 +93,8 @@ class XmlStringLookupTest { @Test void testInterpolatorExternalDtdOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR - + "document-external-dtd.xml:/document/content}")); + assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}"), + "This is an external entity."); } @Test @@ -94,7 +108,7 @@ class XmlStringLookupTest { @Test void testInterpolatorExternalEntityOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}")); + assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); } @Test @@ -107,15 +121,14 @@ class XmlStringLookupTest { @Test void testInterpolatorExternalEntityOn() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}")); + assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); } @Test @SetSystemProperty(key = "XmlStringLookup.secure", value = "true") void testInterpolatorExternalEntityOnOverride() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertThrows(IllegalArgumentException.class, - () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}")); + assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); } @Test
