Move the XmlStringLookup secure toggle to a system property

Author: garydgregory

pom.xml

@@ -92,6 +92,11 @@
       <artifactId>junit-jupiter</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.junit-pioneer</groupId>
+      <artifactId>junit-pioneer</artifactId>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <!-- Java 21 support, revisit for Mockito 5 -->
       <groupId>net.bytebuddy</groupId>

src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java

@@ -1618,19 +1618,17 @@ public StringLookup xmlEncoderStringLookup() {
      * if a lookup causes causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions.
      * </p>
      * <p>
-     * We looks up values in an XML document in the format {@code "[secure=(true|false):]DocumentPath:XPath"}.
+     * We looks up values in an XML document in the format {@code "DocumentPath:XPath"}.
      * </p>
      * <p>
      * For example:
      * </p>
      * <ul>
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=false:com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=true:com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * 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 the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure
+     * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}.
      * </p>
      * <p>
      * Using a {@link StringLookup} from the {@link StringLookupFactory}:
@@ -1664,19 +1662,17 @@ public StringLookup xmlStringLookup() {
      * if a lookup causes causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions.
      * </p>
      * <p>
-     * We looks up values in an XML document in the format {@code "[secure=(true|false):]DocumentPath:XPath"}.
+     * We looks up values in an XML document in the format {@code "]DocumentPath:XPath"}.
      * </p>
      * <p>
      * For example:
      * </p>
      * <ul>
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=false:com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=true:com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * 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 the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure
+     * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}.
      * </p>
      * <p>
      * Using a {@link StringLookup} from the {@link StringLookupFactory}:
@@ -1713,19 +1709,17 @@ public StringLookup xmlStringLookup(final Map<String, Boolean> factoryFeatures)
      * if a lookup causes causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions.
      * </p>
      * <p>
-     * We looks up values in an XML document in the format {@code "[secure=(true|false):]DocumentPath:XPath"}.
+     * We looks up values in an XML document in the format {@code "DocumentPath:XPath"}.
      * </p>
      * <p>
      * For example:
      * </p>
      * <ul>
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=false:com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=true:com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * 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 the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure
+     * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}.
      * </p>
      * <p>
      * Using a {@link StringLookup} from the {@link StringLookupFactory} fenced by the current directory ({@code Paths.get("")}):

src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java

@@ -30,36 +30,30 @@
 import javax.xml.xpath.XPathFactory;
 
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.SystemProperties;
 import org.w3c.dom.Document;
 
 /**
- * Looks up values in an XML document in the format {@code "[secure=(true|false):]DocumentPath:XPath"}.
+ * Looks up values in an XML document in the format {@code "DocumentPath:XPath"}.
  * <p>
  * For example:
  * </p>
  * <ul>
  * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
- * <li>{@code "secure=false:com/domain/document.xml:/path/to/node"}</li>
- * <li>{@code "secure=true:com/domain/document.xml:/path/to/node"}</li>
  * </ul>
  * <p>
- * 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 the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure
+ * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}.
  * </p>
  *
  * @since 1.5
  */
 final class XmlStringLookup extends AbstractPathFencedLookup {
 
     /**
-     * Minimum number of key parts.
+     * The number of key parts.
      */
-    private static final int KEY_PARTS_MIN = 2;
-
-    /**
-     * Minimum number of key parts.
-     */
-    private static final int KEY_PARTS_MAX = 3;
+    private static final int KEY_PARTS_LEN = 2;
 
     /**
      * Defines default XPath factory features.
@@ -76,11 +70,16 @@ final class XmlStringLookup extends AbstractPathFencedLookup {
         DEFAULT_XML_FEATURES = new HashMap<>(1);
         DEFAULT_XML_FEATURES.put(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
     }
+
     /**
      * Defines the singleton for this class with secure processing enabled.
      */
     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.
      */
@@ -106,14 +105,12 @@ final class XmlStringLookup extends AbstractPathFencedLookup {
     }
 
     /**
-     * Looks up a value for the key in the format {@code "[secure=(true|false):]DocumentPath:XPath"}.
+     * Looks up a value for the key in the format {@code "DocumentPath:XPath"}.
      * <p>
      * For example:
      * </p>
      * <ul>
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=false:com/domain/document.xml:/path/to/node"}</li>
-     * <li>{@code "secure=true:com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
      * Secure processing is enabled by default. The secure boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}. The secure
@@ -130,12 +127,11 @@ public String lookup(final String key) {
         }
         final String[] keys = key.split(SPLIT_STR);
         final int keyLen = keys.length;
-        if (keyLen != KEY_PARTS_MIN && keyLen != KEY_PARTS_MAX) {
-            throw IllegalArgumentExceptions.format("Bad XML key format '%s'; the expected format is [secure=(true|false):]DocumentPath:XPath.", key);
+        if (keyLen != KEY_PARTS_LEN) {
+            throw IllegalArgumentExceptions.format("Bad XML key format '%s'; the expected format is 'DocumentPath:XPath'.", key);
         }
-        final boolean isKeySecure = keyLen == KEY_PARTS_MAX;
-        final Boolean secure = isKeySecure ? parseSecureKey(keys, key) : null;
-        final String documentPath = isKeySecure ? keys[1] : keys[0];
+        final Boolean secure = isSecure();
+        final String documentPath = keys[0];
         final String xpath = StringUtils.substringAfterLast(key, SPLIT_CH);
         final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
         try {
@@ -160,12 +156,4 @@ public String lookup(final String key) {
             throw new IllegalArgumentException(e);
         }
     }
-
-    private Boolean parseSecureKey(final String[] args, final String key) {
-        final String[] secParts = args[0].split("=");
-        if (secParts.length != 2 && !Objects.equals(secParts[0], "secure")) {
-            throw IllegalArgumentExceptions.format("Bad XML key format '%s'; the expected format is [secure=(true|false):]DocumentPath:XPath.", key);
-        }
-        return Boolean.valueOf(secParts[1]);
-    }
 }

src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java

@@ -32,6 +32,7 @@
 import javax.xml.XMLConstants;
 
 import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
 
 /**
  * Tests {@link StringLookupFactory}.
@@ -272,6 +273,7 @@ void testXmlStringLookupExternalEntityOff() {
     }
 
     @Test
+    @SetSystemProperty(key = "XmlStringLookup.secure", value = "false")
     void testXmlStringLookupExternalEntityOn() {
         final String key = XmlStringLookupTest.DOC_DIR + "document-entity-ref.xml:/document/content";
         assertEquals(XmlStringLookupTest.DATA, StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP).apply(key).trim());

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}.
@@ -68,6 +69,7 @@ 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());
@@ -81,9 +83,10 @@ void testInterpolatorExternalEntityOff() {
     }
 
     @Test
+    @SetSystemProperty(key = "XmlStringLookup.secure", value = "false")
     void testInterpolatorExternalEntityOffOverride() {
         final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator();
-        assertEquals(DATA, stringSubstitutor.replace("${xml:secure=false:" + DOC_DIR + "document-entity-ref.xml:/document/content}").trim());
+        assertEquals(DATA, stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}").trim());
     }
 
     @Test
@@ -93,18 +96,18 @@ void testInterpolatorExternalEntityOn() {
     }
 
     @Test
+    @SetSystemProperty(key = "XmlStringLookup.secure", value = "true")
     void testInterpolatorExternalEntityOnOverride() {
         final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator();
         assertThrows(IllegalArgumentException.class,
-                () -> stringSubstitutor.replace("${xml:secure=true:" + DOC_DIR + "document-entity-ref.xml:/document/content}"));
+                () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"));
     }
 
     @Test
     void testInterpolatorSecureOnBla() {
         final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator();
         assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "bla.xml:/document/content}"));
-        assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:secure=true:" + DOC_DIR + "bla.xml:/document/content}"));
-        // Using secure=false allows the BLA to occur.
+        // Using XmlStringLookup.secure=false allows the BLA to occur.
     }
 
     @Test