Skip to content

Harden XML parsing via commons-secure-xml #766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ppkarwasz wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Harden XML parsing via commons-secure-xml #766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f758b18
Harden XML parsing via commons-secure-xml
ppkarwasz Aug 31, 2026
ac4f28f
Use the Commons Secure XML 1.0.0 release candidate
ppkarwasz Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,38 @@
<commons.jacoco.lineRatio>0.99</commons.jacoco.lineRatio>
<commons.jacoco.complexityRatio>0.97</commons.jacoco.complexityRatio>
</properties>
<repositories>
<!-- Repeat the super POM's Central so it is queried before the staging repository. -->
<repository>
<id>central</id>
<name>Central Repository</name>
<url>https://repo.maven.apache.org/maven2</url>
<snapshots>
<enabled>false</enabled>
</snapshots>
</repository>
<!-- TODO: remove once Commons Secure XML 1.0.0 is released. -->
<repository>
<id>apache.commons.staging</id>
<name>Apache Commons Secure XML 1.0.0 release candidate</name>
<url>https://repository.apache.org/content/repositories/orgapachecommons-1962/</url>
<snapshots>
<enabled>false</enabled>
</snapshots>
</repository>
</repositories>

<dependencies>
<dependency>
<groupId>org.apache.commons</groupId>
<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</version>
</dependency>
<!-- testing -->
<dependency>
<groupId>org.junit.jupiter</groupId>
Expand Down
1 change: 1 addition & 0 deletions src/changes/changes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ The <action> type attribute can be add,update,fix,remove.
<body>
<release version="1.15.1" date="YYYY-MM-DD" description="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; feature maps without secure processing and the javax.xml.accessExternalDTD system property still restore the previous behavior.</action>
<action type="fix" dev="ggregory" due-to="Dominik Stadler, Gary Gregory">Improve test coverage #732.</action>
<action type="fix" dev="ggregory" issue="TEXT-239" due-to="Dominik Stadler, Gary Gregory">TextStringBuilder.append(char[], int, int) uses wrong variable in exception message #735.</action>
<action type="fix" dev="ggregory" due-to="Omkhar Arasaratnam, Gary Gregory">StrBuilder.readFrom(Readable) exposes stale internal buffer to Readable parameter (#741).</action>
Expand Down
13 changes: 11 additions & 2 deletions src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import javax.xml.xpath.XPathFactory;

import org.apache.commons.lang3.StringUtils;
import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
import org.apache.commons.xml.secure.SecureXPathFactory;
import org.w3c.dom.Document;

/**
Expand Down Expand Up @@ -128,14 +130,21 @@ public String lookup(final String key) {
}
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 that ignores the JAXP access properties,
// so the documented opt-outs keep a plain factory: a feature map without secure processing, or the
// standard javax.xml.accessExternalDTD system property re-allowing external access.
final boolean secure = Boolean.TRUE.equals(xmlFactoryFeatures.get(XMLConstants.FEATURE_SECURE_PROCESSING))
&& StringUtils.isEmpty(System.getProperty("javax.xml.accessExternalDTD"));
final DocumentBuilderFactory dbFactory = secure ? SecureDocumentBuilderFactory.newInstance() : DocumentBuilderFactory.newInstance();
try {
for (final Entry<String, Boolean> p : xmlFactoryFeatures.entrySet()) {
dbFactory.setFeature(p.getKey(), p.getValue());
}
try (InputStream inputStream = Files.newInputStream(getPath(documentPath))) {
final Document doc = dbFactory.newDocumentBuilder().parse(inputStream);
final XPathFactory xpFactory = XPathFactory.newInstance();
final XPathFactory xpFactory = Boolean.TRUE.equals(xPathFactoryFeatures.get(XMLConstants.FEATURE_SECURE_PROCESSING))
? SecureXPathFactory.newInstance()
: XPathFactory.newInstance();
for (final Entry<String, Boolean> p : xPathFactoryFeatures.entrySet()) {
xpFactory.setFeature(p.getKey(), p.getValue());
}
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,9 @@ void testXmlStringLookup() {

@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
Expand Down
27 changes: 21 additions & 6 deletions src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Supplier;

import javax.xml.XMLConstants;

Expand All @@ -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);
Expand All @@ -64,8 +78,8 @@ void testBadXPath() {

@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
Expand All @@ -78,7 +92,8 @@ void testExternalEntityOn() {
@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
Expand All @@ -91,7 +106,7 @@ void testInterpolatorExternalDtdOn() {
@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
Expand All @@ -104,13 +119,13 @@ void testInterpolatorExternalEntityOffOverride() {
@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
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
Expand Down
Loading