From 4e88dfb7541ef1064f2e550c49fa04608dff232f Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Thu, 4 Dec 2025 09:52:31 -0500 Subject: [PATCH 1/2] Simplify XML FSP --- .../text/lookup/StringLookupFactory.java | 5 ++--- .../commons/text/lookup/XmlStringLookup.java | 19 ++++++------------- .../text/lookup/XmlStringLookupTest.java | 18 ------------------ 3 files changed, 8 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java b/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java index 863baa7f34..699b7deab7 100644 --- a/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java +++ b/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java @@ -1718,15 +1718,14 @@ public StringLookup xmlStringLookup(final Map factoryFeatures) *
  • {@code "com/domain/document.xml:/path/to/node"}
    • * *

    - * Secure processing is enabled by default and can be overridden with the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure - * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}. + * Secure processing is enabled by default and can be overridden with this constructor. *

    *

    * Using a {@link StringLookup} from the {@link StringLookupFactory} fenced by the current directory ({@code Paths.get("")}): *

    * * 
    -     * StringLookupFactory.INSTANCE.xmlStringLookup(map, Pathe.get("")).lookup("com/domain/document.xml:/path/to/node");
+     * StringLookupFactory.INSTANCE.xmlStringLookup(map, Path.get("")).lookup("com/domain/document.xml:/path/to/node");
      *
    *

    * To use a {@link StringLookup} fenced by the current directory, use: 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 85747eebc2..ee33441993 100644 --- a/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java +++ b/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java @@ -30,7 +30,6 @@ import javax.xml.xpath.XPathFactory; import org.apache.commons.lang3.StringUtils; -import org.apache.commons.lang3.SystemProperties; import org.w3c.dom.Document; /** @@ -42,8 +41,7 @@ *

  • {@code "com/domain/document.xml:/path/to/node"}
    • * *

    - * Secure processing is enabled by default and can be overridden with the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure - * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}. + * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}. *

    * * @since 1.5 @@ -72,14 +70,13 @@ final class XmlStringLookup extends AbstractPathFencedLookup { } /** - * Defines the singleton for this class with secure processing enabled. + * Defines the singleton for this class with secure processing enabled by default. + *

    + * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}. + *

    */ static final XmlStringLookup INSTANCE = new XmlStringLookup(DEFAULT_XML_FEATURES, DEFAULT_XPATH_FEATURES, (Path[]) null); - private static boolean isSecure() { - return SystemProperties.getBoolean(XmlStringLookup.class, "secure", () -> true); - } - /** * Defines XPath factory features. */ @@ -113,8 +110,7 @@ private static boolean isSecure() { *
  • {@code "com/domain/document.xml:/path/to/node"}
    • * *

    - * Secure processing is enabled by default. The secure boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}. The secure - * value in the key overrides instance settings given in the constructor. + * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}. *

    * * @param key the key to be looked up, may be null. @@ -130,7 +126,6 @@ public String lookup(final String key) { if (keyLen != KEY_PARTS_LEN) { throw IllegalArgumentExceptions.format("Bad XML key format '%s'; the expected format is 'DocumentPath:XPath'.", key); } - final boolean secure = isSecure(); final String documentPath = keys[0]; final String xpath = StringUtils.substringAfterLast(key, SPLIT_CH); final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); @@ -138,14 +133,12 @@ public String lookup(final String key) { for (final Entry p : xmlFactoryFeatures.entrySet()) { dbFactory.setFeature(p.getKey(), p.getValue()); } - 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(); for (final Entry p : xPathFactoryFeatures.entrySet()) { xpFactory.setFeature(p.getKey(), p.getValue()); } - xpFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, secure); return xpFactory.newXPath().evaluate(xpath, doc); } } catch (final Exception e) { 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 f0cc50ba96..b637b158c6 100644 --- a/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java +++ b/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java @@ -35,7 +35,6 @@ import org.apache.commons.lang3.StringUtils; import org.apache.commons.text.StringSubstitutor; import org.junit.jupiter.api.Test; -import org.junitpioneer.jupiter.SetSystemProperty; /** * Tests {@link XmlStringLookup}. @@ -69,7 +68,6 @@ void testExternalEntityOff() { } @Test - @SetSystemProperty(key = "XmlStringLookup.secure", value = "false") void testExternalEntityOn() { final String key = DOC_DIR + "document-entity-ref.xml:/document/content"; assertEquals(DATA, new XmlStringLookup(EMPTY_MAP, EMPTY_MAP).apply(key).trim()); @@ -83,27 +81,12 @@ void testInterpolatorExternalDtdOff() { + "document-external-dtd.xml:/document/content}")); } - @Test - @SetSystemProperty(key = "XmlStringLookup.secure", value = "false") - void testInterpolatorExternalDtdOn() { - final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertEquals("This is an external entity.", - stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}").trim()); - } - @Test void testInterpolatorExternalEntityOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}")); } - @Test - @SetSystemProperty(key = "XmlStringLookup.secure", value = "false") - void testInterpolatorExternalEntityOffOverride() { - final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertEquals(DATA, stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}").trim()); - } - @Test void testInterpolatorExternalEntityOn() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); @@ -111,7 +94,6 @@ void testInterpolatorExternalEntityOn() { } @Test - @SetSystemProperty(key = "XmlStringLookup.secure", value = "true") void testInterpolatorExternalEntityOnOverride() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); assertThrows(IllegalArgumentException.class, From abe5a097bd7f882c988d9380bc09f9bfa8e1232b Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Thu, 4 Dec 2025 10:16:35 -0500 Subject: [PATCH 2/2] Simplify XML FSP --- src/changes/changes.xml | 2 +- .../text/lookup/XmlStringLookupTest.java | 21 +++++++++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index f8912d2005..9a4b221ebc 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -53,7 +53,7 @@ The type attribute can be add,update,fix,remove. Fix Apache RAT plugin console warnings. Fix site XML to use version 2.0.0 XML schema. Removed unreachable threshold verification code in src/main/java/org/apache/commons/text/similarity #730. - Enable secure processing for the XML parser in XmlStringLookup #729. + Enable secure processing for the XML parser in XmlStringLookup in case the underlying JAXP implementation doesn't #729. Add experimental CycloneDX VEX file #683. Add Damerau-Levenshtein distance #687. 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 b637b158c6..7327bca7ef 100644 --- a/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java +++ b/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java @@ -35,6 +35,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.commons.text.StringSubstitutor; import org.junit.jupiter.api.Test; +import org.junitpioneer.jupiter.SetSystemProperty; /** * Tests {@link XmlStringLookup}. @@ -77,8 +78,14 @@ void testExternalEntityOn() { @Test void testInterpolatorExternalDtdOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR - + "document-external-dtd.xml:/document/content}")); + assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}")); + } + + @Test + @SetSystemProperty(key = "javax.xml.accessExternalDTD", value = "file") + void testInterpolatorExternalDtdOn() { + final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); + assertEquals("This is an external entity.", stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}").trim()); } @Test @@ -87,6 +94,13 @@ void testInterpolatorExternalEntityOff() { assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}")); } + @Test + @SetSystemProperty(key = "javax.xml.accessExternalDTD", value = "file") + void testInterpolatorExternalEntityOffOverride() { + final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); + assertEquals(DATA, stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}").trim()); + } + @Test void testInterpolatorExternalEntityOn() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); @@ -96,8 +110,7 @@ void testInterpolatorExternalEntityOn() { @Test void testInterpolatorExternalEntityOnOverride() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertThrows(IllegalArgumentException.class, - () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}")); + assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}")); } @Test