diff --git a/pom.xml b/pom.xml index 9f8e678457..42a20a8a6a 100644 --- a/pom.xml +++ b/pom.xml @@ -81,12 +81,38 @@ 0.99 0.97 + + + + central + Central Repository + https://repo.maven.apache.org/maven2 + + false + + + + + apache.commons.staging + Apache Commons Secure XML 1.0.0 release candidate + https://repository.apache.org/content/repositories/orgapachecommons-1962/ + + false + + + + org.apache.commons commons-lang3 ${commons.lang3.version} + + org.apache.commons + commons-secure-xml + 1.0.0 + org.junit.jupiter diff --git a/src/changes/changes.xml b/src/changes/changes.xml index b9beac2ede..019f2acb75 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,6 +45,7 @@ The type attribute can be add,update,fix,remove. + 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. Improve test coverage #732. TextStringBuilder.append(char[], int, int) uses wrong variable in exception message #735. StrBuilder.readFrom(Readable) exposes stale internal buffer to Readable parameter (#741). 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 624c7d01e2..8b69cd639f 100644 --- a/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java +++ b/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java @@ -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; /** @@ -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 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 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 e0fdf8abe7..97ab564436 100644 --- a/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java +++ b/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java @@ -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 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 862fdf812d..8c2309e1b0 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.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 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 @@ 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 @@ -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 @@ -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 @@ -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