Simplify XML FSP (#731)

* Simplify XML FSP

* Simplify XML FSP

Author: garydgregory

src/changes/changes.xml

@@ -53,7 +53,7 @@ The <action> type attribute can be add,update,fix,remove.
     <action type="fix" dev="ggregory" due-to="Gary Gregory">Fix Apache RAT plugin console warnings.</action>
     <action type="fix" dev="ggregory" due-to="Gary Gregory">Fix site XML to use version 2.0.0 XML schema.</action>
     <action type="fix" dev="ggregory" due-to="Michael Hausegger">Removed unreachable threshold verification code in src/main/java/org/apache/commons/text/similarity #730.</action>
-    <action type="fix" dev="ggregory" due-to="김민재, Gary Gregory">Enable secure processing for the XML parser in XmlStringLookup #729.</action>
+    <action type="fix" dev="ggregory" due-to="김민재, Gary Gregory, Piotr Karwasz">Enable secure processing for the XML parser in XmlStringLookup in case the underlying JAXP implementation doesn't #729.</action>
     <!-- ADD -->
     <action type="add" dev="ggregory" due-to="Piotr P. Karwasz, Gary Gregory">Add experimental CycloneDX VEX file #683.</action>
     <action type="add" dev="ggregory" due-to="LorgeN, Gary Gregory" issue="TEXT-235">Add Damerau-Levenshtein distance #687.</action>

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

@@ -1718,15 +1718,14 @@ public StringLookup xmlStringLookup(final Map<String, Boolean> factoryFeatures)
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * 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.
      * </p>
      * <p>
      * Using a {@link StringLookup} from the {@link StringLookupFactory} fenced by the current directory ({@code Paths.get("")}):
      * </p>
      *
      * <pre>
-     * 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");
      * </pre>
      * <p>
      * To use a {@link StringLookup} fenced by the current directory, use:

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 @@
  * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
  * </ul>
  * <p>
- * 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...)}.
  * </p>
  *
  * @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.
+     * <p>
+     * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}.
+     * </p>
      */
     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() {
      * <li>{@code "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 {@link StringLookupFactory#xmlStringLookup(Map, Path...)}.
      * </p>
      *
      * @param key the key to be looked up, may be null.
@@ -130,22 +126,19 @@ 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();
         try {
             for (final Entry<String, Boolean> 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<String, Boolean> 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) {

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

@@ -69,7 +69,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());
@@ -79,16 +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 = "XmlStringLookup.secure", value = "false")
+    @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());
+        assertEquals("This is an external entity.", stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}").trim());
     }
 
     @Test
@@ -98,7 +95,7 @@ void testInterpolatorExternalEntityOff() {
     }
 
     @Test
-    @SetSystemProperty(key = "XmlStringLookup.secure", value = "false")
+    @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());
@@ -111,11 +108,9 @@ void testInterpolatorExternalEntityOn() {
     }
 
     @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}"));
+        assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"));
     }
 
     @Test