From f758b18bc4e8c0203eb501618d803c46cb04d81a Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 31 Aug 2026 15:54:18 +0200 Subject: [PATCH 1/9] Harden XML parsing via commons-secure-xml Create XmlStringLookup's document builder and XPath factories through org.apache.commons:commons-secure-xml. The secure factories enable FEATURE_SECURE_PROCESSING and install a non-removable entity-resolver floor on every parser they produce: external DTD and entity lookups are resolved to empty content instead of being fetched, and internal entity expansion is bounded, regardless of the JAXP implementation on the classpath. Changes: - Add the commons-secure-xml dependency (1.0.0-SNAPSHOT until its first release). - Route factory creation through SecureDocumentBuilderFactory and SecureXPathFactory in XmlStringLookup when the instance's feature map enables secure processing. The documented opt-outs keep their meaning: feature maps without secure processing, and the standard javax.xml.accessExternalDTD system property (which the secure factory's resolver floor would otherwise ignore), keep using the plain JAXP factories, so external entity resolution can be restored where it is wanted. - Adapt the secure-path tests to the secure contract: a parser may either reject a document with an external reference or parse it with the reference resolved to empty content; the tests now assert that the external content does not leak into the result instead of expecting one fixed failure mode. - Run the CI and CodeQL builds with -Puse-apache-snapshots (inherited from the org.apache:apache parent POM) so the commons-secure-xml SNAPSHOT resolves; CodeQL's autobuild receives the profile through MAVEN_ARGS. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MHgnMnGWHQoH2zD2jFdoMT --- .github/workflows/codeql-analysis.yml | 2 ++ .github/workflows/maven.yml | 2 +- pom.xml | 5 ++++ src/changes/changes.xml | 1 + .../commons/text/lookup/XmlStringLookup.java | 13 +++++++-- .../text/lookup/StringLookupFactoryTest.java | 5 ++-- .../text/lookup/XmlStringLookupTest.java | 27 ++++++++++++++----- 7 files changed, 44 insertions(+), 11 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index eaf7ff5225..9a2a536446 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -70,6 +70,8 @@ jobs: # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild uses: github/codeql-action/autobuild@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 + env: + MAVEN_ARGS: -Puse-apache-snapshots # ℹī¸ Command-line programs to run using the OS shell. # 📚 https://git.io/JvXDl diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 5a8b24254a..4f58b6e53d 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -49,4 +49,4 @@ jobs: java-version: ${{ matrix.java }} cache: 'maven' - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Dpolyglot.engine.WarnInterpreterOnly=false + run: mvn --errors --show-version --batch-mode --no-transfer-progress -Dpolyglot.engine.WarnInterpreterOnly=false -Puse-apache-snapshots diff --git a/pom.xml b/pom.xml index 9f8e678457..649e0f71a1 100644 --- a/pom.xml +++ b/pom.xml @@ -87,6 +87,11 @@ commons-lang3 ${commons.lang3.version} + + org.apache.commons + commons-secure-xml + 1.0.0-SNAPSHOT + 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 From ac4f28fc0d2e43928893f864e3baac3efb0a8486 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 3 Sep 2026 07:04:11 +0200 Subject: [PATCH 2/9] Use the Commons Secure XML 1.0.0 release candidate Bump org.apache.commons:commons-secure-xml from 1.0.0-SNAPSHOT to 1.0.0 and add the temporary staging repository https://repository.apache.org/content/repositories/orgapachecommons-1962/ after Central, so the vote gets downstream CI results. Drop the -Puse-apache-snapshots profile from the CI workflows, which the release version no longer needs. Remove the staging repository once 1.0.0 is released. Assisted-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0167e29ScPEdfzJnEFm95imK --- .github/workflows/codeql-analysis.yml | 2 -- .github/workflows/maven.yml | 2 +- pom.xml | 23 ++++++++++++++++++++++- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 9a2a536446..eaf7ff5225 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -70,8 +70,6 @@ jobs: # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild uses: github/codeql-action/autobuild@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 - env: - MAVEN_ARGS: -Puse-apache-snapshots # ℹī¸ Command-line programs to run using the OS shell. # 📚 https://git.io/JvXDl diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 4f58b6e53d..5a8b24254a 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -49,4 +49,4 @@ jobs: java-version: ${{ matrix.java }} cache: 'maven' - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Dpolyglot.engine.WarnInterpreterOnly=false -Puse-apache-snapshots + run: mvn --errors --show-version --batch-mode --no-transfer-progress -Dpolyglot.engine.WarnInterpreterOnly=false diff --git a/pom.xml b/pom.xml index 649e0f71a1..42a20a8a6a 100644 --- a/pom.xml +++ b/pom.xml @@ -81,6 +81,27 @@ 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 @@ -90,7 +111,7 @@ org.apache.commons commons-secure-xml - 1.0.0-SNAPSHOT + 1.0.0 From d4205be7a1570025ded1fb7182c5ed293a3b4cc9 Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Sun, 6 Sep 2026 08:39:46 -0400 Subject: [PATCH 3/9] Bump Apache Commons Secure XML from 1.0.0-SNAPSHOT to 1.0.0 --- pom.xml | 21 --------------------- src/changes/changes.xml | 2 +- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/pom.xml b/pom.xml index 42a20a8a6a..774be7549f 100644 --- a/pom.xml +++ b/pom.xml @@ -81,27 +81,6 @@ 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 diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 019f2acb75..4e1dca4c28 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,7 +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. + 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). From bc9c20447a11dfbde21defdcc7713a62a0ba7ee8 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sun, 6 Sep 2026 20:00:19 +0200 Subject: [PATCH 4/9] Drop the insecure XmlStringLookup fallback Commons Secure XML blocks external resource fetching with an entity resolver rather than with parser features, so the opt-outs the previous commit documented never worked: neither a feature map without secure processing nor the javax.xml.accessExternalDTD system property reaches the resolver floor. Always create the factories through SecureDocumentBuilderFactory and SecureXPathFactory instead. Changes: - Remove the plain-JAXP fallback from XmlStringLookup.lookup(String), along with the DEFAULT_XML_FEATURES and DEFAULT_XPATH_FEATURES maps that only existed to select it. - Document the two aspects Commons Secure XML secures separately: FEATURE_SECURE_PROCESSING governs processing limits and remains settable, while external DTD subsets and entities are blocked outright and cannot be re-enabled. - Remove the references to the "XmlStringLookup.secure" system property, which was never introduced. - Disable the tests that expect an external entity to resolve, and tighten the leak assertion now that the parser no longer rejects such documents. - Shorten the changelog entry and state the behavior change. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Y7VfssZLkK11bBY2kVyyY4 --- src/changes/changes.xml | 2 +- .../text/lookup/StringLookupFactory.java | 19 ++++--- .../commons/text/lookup/XmlStringLookup.java | 51 +++++++------------ .../text/lookup/StringLookupFactoryTest.java | 6 +-- .../text/lookup/XmlStringLookupTest.java | 32 ++++++------ 5 files changed, 51 insertions(+), 59 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 4e1dca4c28..4811aaa1ce 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,7 +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. + XmlStringLookup creates its XML parser and XPath factories through Commons Secure XML; external DTD subsets and entities are no longer resolved and cannot be re-enabled. 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/StringLookupFactory.java b/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java index 3feb4fe5e0..918689104c 100644 --- a/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java +++ b/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java @@ -1627,8 +1627,10 @@ public StringLookup xmlEncoderStringLookup() { *
  • {@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)}. + * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects. Processing limits, such as the number of entity expansions, + * come from {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default and can be turned off through the factory features. + * External DTD subsets and external entities are blocked by an entity resolver rather than by a feature, so neither a feature nor a JAXP + * {@code javax.xml.accessExternal*} property can re-enable them. *

    *

    * Using a {@link StringLookup} from the {@link StringLookupFactory}: @@ -1652,7 +1654,7 @@ public StringLookup xmlEncoderStringLookup() { * @since 1.5 */ public StringLookup xmlStringLookup() { - return fences != null ? xmlStringLookup(XmlStringLookup.DEFAULT_XPATH_FEATURES, fences) : XmlStringLookup.INSTANCE; + return fences != null ? xmlStringLookup(Collections.emptyMap(), fences) : XmlStringLookup.INSTANCE; } /** @@ -1671,8 +1673,10 @@ public StringLookup xmlStringLookup() { *

  • {@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)}. + * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects. Processing limits, such as the number of entity expansions, + * come from {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default and can be turned off through the factory features. + * External DTD subsets and external entities are blocked by an entity resolver rather than by a feature, so neither a feature nor a JAXP + * {@code javax.xml.accessExternal*} property can re-enable them. *

    *

    * Using a {@link StringLookup} from the {@link StringLookupFactory}: @@ -1718,7 +1722,10 @@ 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 this constructor. + * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects. Processing limits, such as the number of entity expansions, + * come from {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default and can be turned off through the factory features. + * External DTD subsets and external entities are blocked by an entity resolver rather than by a feature, so neither a feature nor a JAXP + * {@code javax.xml.accessExternal*} property can re-enable them. *

    *

    * Using a {@link StringLookup} from the {@link StringLookupFactory} fenced by the current directory ({@code Paths.get("")}): 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 8b69cd639f..de88e5e7ab 100644 --- a/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java +++ b/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java @@ -20,7 +20,7 @@ import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; -import java.util.HashMap; +import java.util.Collections; import java.util.Map; import java.util.Map.Entry; import java.util.Objects; @@ -43,8 +43,17 @@ *

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

    - * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}. + * DOM parser and XPath factory features can be set with {@link StringLookupFactory#xmlStringLookup(Map)}. *

    + *

    + * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects: + *

    + *
      + *
    • Processing limits, such as the number of entity expansions, come from {@link XMLConstants#FEATURE_SECURE_PROCESSING}. That feature is enabled by + * default, and the feature maps above can turn it off.
      • + *
    • External resource fetching, that is external DTD subsets and external entities, is blocked by an entity resolver rather than by a feature. Neither a + * feature nor a JAXP {@code javax.xml.accessExternal*} property can re-enable it.
      • + *
    * * @since 1.5 */ @@ -56,28 +65,12 @@ final class XmlStringLookup extends AbstractPathFencedLookup { private static final int KEY_PARTS_LEN = 2; /** - * Defines default XPath factory features. - */ - static final Map DEFAULT_XPATH_FEATURES; - - /** - * Defines default XML factory features. - */ - static final Map DEFAULT_XML_FEATURES; - static { - DEFAULT_XPATH_FEATURES = new HashMap<>(1); - DEFAULT_XPATH_FEATURES.put(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE); - 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 by default. + * Defines the singleton for this class, which sets no parser or XPath factory feature. *

    - * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}. + * Use {@link StringLookupFactory#xmlStringLookup(Map, Path...)} to set features; external resource resolution is off anyway. *

    */ - static final XmlStringLookup INSTANCE = new XmlStringLookup(DEFAULT_XML_FEATURES, DEFAULT_XPATH_FEATURES, (Path[]) null); + static final XmlStringLookup INSTANCE = new XmlStringLookup(Collections.emptyMap(), Collections.emptyMap(), (Path[]) null); /** * Defines XPath factory features. @@ -100,7 +93,7 @@ final class XmlStringLookup extends AbstractPathFencedLookup { XmlStringLookup(final Map xmlFactoryFeatures, final Map xPathFactoryFeatures, final Path... fences) { super(fences); this.xmlFactoryFeatures = Objects.requireNonNull(xmlFactoryFeatures, "xmlFactoryFeatures"); - this.xPathFactoryFeatures = Objects.requireNonNull(xPathFactoryFeatures, "xPathFfactoryFeatures"); + this.xPathFactoryFeatures = Objects.requireNonNull(xPathFactoryFeatures, "xPathFactoryFeatures"); } /** @@ -112,7 +105,8 @@ final class XmlStringLookup extends AbstractPathFencedLookup { *
  • {@code "com/domain/document.xml:/path/to/node"}
    • * *

    - * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}. + * The document is parsed through Apache Commons Secure XML: processing limits are governed by {@link XMLConstants#FEATURE_SECURE_PROCESSING}, which is + * enabled by default, while external DTD subsets and external entities are blocked outright and cannot be re-enabled. *

    * * @param key The key to be looked up, may be null. @@ -130,21 +124,14 @@ public String lookup(final String key) { } final String documentPath = keys[0]; final String xpath = StringUtils.substringAfterLast(key, SPLIT_CH); - // 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(); + final DocumentBuilderFactory dbFactory = SecureDocumentBuilderFactory.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 = Boolean.TRUE.equals(xPathFactoryFeatures.get(XMLConstants.FEATURE_SECURE_PROCESSING)) - ? SecureXPathFactory.newInstance() - : XPathFactory.newInstance(); + final XPathFactory xpFactory = SecureXPathFactory.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 97ab564436..c4ce464494 100644 --- a/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java +++ b/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java @@ -31,9 +31,9 @@ import javax.xml.XMLConstants; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.DefaultLocale; -import org.junitpioneer.jupiter.SetSystemProperty; /** * Tests {@link StringLookupFactory}. @@ -293,13 +293,13 @@ void testXmlStringLookup() { @Test void testXmlStringLookupExternalEntityOff() { - XmlStringLookupTest.assertBlocksOrDoesNotLeak( + XmlStringLookupTest.assertDoesNotLeak( () -> StringLookupFactory.INSTANCE.xmlStringLookup().apply(XmlStringLookupTest.DOC_DIR + "document-entity-ref.xml:/document/content"), XmlStringLookupTest.DATA); } @Test - @SetSystemProperty(key = "XmlStringLookup.secure", value = "false") + @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") 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()); 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 8c2309e1b0..e8e02f5e04 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.Disabled; import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.SetSystemProperty; @@ -52,16 +53,12 @@ class XmlStringLookupTest { 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. + * Asserts external content does not leak */ - 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 assertDoesNotLeak(final Supplier lookup, final String external) { + final String result = lookup.get(); + assertNotNull(result, "lookup returned null"); + assertFalse(result.contains(external), () -> "external content leaked: " + result); } static void assertLookup(final StringLookup xmlStringLookup) { @@ -78,26 +75,27 @@ void testBadXPath() { @Test void testExternalEntityOff() { - assertBlocksOrDoesNotLeak( - () -> new XmlStringLookup(XmlStringLookup.DEFAULT_XML_FEATURES, EMPTY_MAP).apply(DOC_DIR + "document-entity-ref.xml:/document/content"), DATA); + assertDoesNotLeak( + () -> new XmlStringLookup(EMPTY_MAP, EMPTY_MAP).apply(DOC_DIR + "document-entity-ref.xml:/document/content"), DATA); } @Test + @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") void testExternalEntityOn() { final String key = DOC_DIR + "document-entity-ref.xml:/document/content"; assertEquals(DATA, new XmlStringLookup(EMPTY_MAP, EMPTY_MAP).apply(key).trim()); - assertEquals(DATA, new XmlStringLookup(EMPTY_MAP, XmlStringLookup.DEFAULT_XPATH_FEATURES).apply(key).trim()); } @Test void testInterpolatorExternalDtdOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}"), + assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}"), "This is an external entity."); } @Test @SetSystemProperty(key = "javax.xml.accessExternalDTD", value = "file") + @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") 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()); @@ -106,11 +104,12 @@ void testInterpolatorExternalDtdOn() { @Test void testInterpolatorExternalEntityOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); + assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); } @Test @SetSystemProperty(key = "javax.xml.accessExternalDTD", value = "file") + @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") void testInterpolatorExternalEntityOffOverride() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); assertEquals(DATA, stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}").trim()); @@ -119,20 +118,19 @@ void testInterpolatorExternalEntityOffOverride() { @Test void testInterpolatorExternalEntityOn() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); + assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); } @Test void testInterpolatorExternalEntityOnOverride() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertBlocksOrDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); + assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); } @Test void testInterpolatorSecureOnBla() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); assertThrows(IllegalArgumentException.class, () -> stringSubstitutor.replace("${xml:" + DOC_DIR + "bla.xml:/document/content}")); - // Using XmlStringLookup.secure=false allows the BLA to occur. } @Test From 121b9df5b7d747f2949e70c92d01f44066c551e9 Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Tue, 8 Sep 2026 16:41:32 -0400 Subject: [PATCH 5/9] Add missing tests --- .../text/lookup/StringLookupFactoryTest.java | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) 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 c4ce464494..0547349dfd 100644 --- a/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java +++ b/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java @@ -22,6 +22,9 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Locale; @@ -291,6 +294,11 @@ void testXmlStringLookup() { XmlStringLookupTest.assertLookup(stringLookupFactory.xmlStringLookup(new HashMap<>())); } + @Test + void testXmlStringLookupEmptyPaths() { + XmlStringLookupTest.assertLookup(StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP, new Path[0])); + } + @Test void testXmlStringLookupExternalEntityOff() { XmlStringLookupTest.assertDoesNotLeak( @@ -304,4 +312,36 @@ 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()); } + @Test + void testXmlStringLookupMultiplePaths() { + final Path documentPath = Paths.get(XmlStringLookupTest.DOC_DIR); + final Path otherPath = Paths.get("src/main"); + XmlStringLookupTest.assertLookup(StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP, otherPath, documentPath)); + XmlStringLookupTest.assertLookup(StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP, documentPath, otherPath)); + } + + @Test + void testXmlStringLookupNullFeatures() { + assertThrows(NullPointerException.class, () -> StringLookupFactory.INSTANCE.xmlStringLookup(null, Paths.get(XmlStringLookupTest.DOC_DIR))); + } + + @Test + void testXmlStringLookupNullPaths() { + XmlStringLookupTest.assertLookup(StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP, (Path[]) null)); + } + + @Test + void testXmlStringLookupOutsidePaths() { + final StringLookup lookup = StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP, Paths.get("src/main")); + assertThrows(IllegalArgumentException.class, () -> lookup.apply(XmlStringLookupTest.DOC_DIR + "document.xml:/root/path/to/node")); + } + + @Test + void testXmlStringLookupPaths() { + final Path documentPath = Paths.get(XmlStringLookupTest.DOC_DIR); + final Map features = Collections.singletonMap(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE); + XmlStringLookupTest.assertLookup(StringLookupFactory.INSTANCE.xmlStringLookup(features, documentPath)); + XmlStringLookupTest.assertLookup(StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP, documentPath)); + } + } From 3014afc4b625a44e845d10e6dd154ec37ae830a5 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 9 Sep 2026 09:22:52 +0200 Subject: [PATCH 6/9] Fence external XML references in XmlStringLookup Commons Secure XML blocks external DTD subsets and entities with a non-removable entity-resolver floor, and lets a caller opt specific resources back in by installing a resolver of their own. Reuse the Path fences the lookup already accepts for that: a caller who declared trusted roots for the document gets its follow-up resources resolved from those same roots, and from nowhere else. Changes: - Add PathFenceResolver, a package-private EntityResolver that resolves a system identifier only when it lands inside the fence and refuses anything else, including any non-file reference. XmlStringLookup installs it only when the fence has roots, so an unfenced lookup resolves no external resource at all. - Parse with the document's own URI, so relative system identifiers resolve against the document as XML requires, rather than against the process working directory. - Add PathFence.isEmpty() to tell an empty fence from a real allow. - Reword the XmlStringLookup and StringLookupFactory Javadoc: secure processing covers two separate aspects, the FEATURE_SECURE_PROCESSING limits that callers can still turn off and the external references that only a fence can opt back in. Drop the references to the XmlStringLookup.secure system property, which was never implemented. - Move the external-reference fixtures to src/test/resources/XmlStringLookup, with the referenced files in the root and the documents that reference them one directory below, so one pair of files covers both the allowed and the refused case depending on where the fence points. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt --- src/changes/changes.xml | 3 +- .../apache/commons/text/lookup/PathFence.java | 11 +- .../text/lookup/PathFenceResolver.java | 108 +++++++++++++ .../text/lookup/StringLookupFactory.java | 64 +++++--- .../commons/text/lookup/XmlStringLookup.java | 33 +++- .../text/lookup/PathFenceResolverTest.java | 147 ++++++++++++++++++ .../text/lookup/StringLookupFactoryTest.java | 11 +- .../text/lookup/XmlStringLookupTest.java | 100 ++++++++---- .../resources/XmlStringLookup/document.dtd | 20 +++ .../documents}/document-entity-ref.xml | 6 +- .../documents}/document-external-dtd.xml | 4 +- .../documents/document-remote-entity.xml | 26 ++++ .../text => XmlStringLookup}/xml-entity.txt | 0 .../commons/text/document-external-dtd.dtd | 20 --- 14 files changed, 460 insertions(+), 93 deletions(-) create mode 100644 src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java create mode 100644 src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java create mode 100644 src/test/resources/XmlStringLookup/document.dtd rename src/test/resources/{org/apache/commons/text => XmlStringLookup/documents}/document-entity-ref.xml (86%) rename src/test/resources/{org/apache/commons/text => XmlStringLookup/documents}/document-external-dtd.xml (86%) create mode 100644 src/test/resources/XmlStringLookup/documents/document-remote-entity.xml rename src/test/resources/{org/apache/commons/text => XmlStringLookup}/xml-entity.txt (100%) delete mode 100644 src/test/resources/org/apache/commons/text/document-external-dtd.dtd diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 4811aaa1ce..a90a87ab9b 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,7 +45,6 @@ The type attribute can be add,update,fix,remove. - XmlStringLookup creates its XML parser and XPath factories through Commons Secure XML; external DTD subsets and entities are no longer resolved and cannot be re-enabled. 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). @@ -72,6 +71,8 @@ The type attribute can be add,update,fix,remove. StringSubstitutorReader can now substitute variables with a suffix longer than one characters (#764). Fix ArrayIndexOutOfBoundsException in DnsStringLookup for '|' key (#765). + XmlStringLookup creates its XML parser and XPath factories through Commons Secure XML. + XmlStringLookup resolves external DTD subsets and entities from its Path fences. Bump org.apache.commons:commons-parent from 93 to 104. Bump the level of test coverage checks. diff --git a/src/main/java/org/apache/commons/text/lookup/PathFence.java b/src/main/java/org/apache/commons/text/lookup/PathFence.java index 0bd884a5ac..c914fa1def 100644 --- a/src/main/java/org/apache/commons/text/lookup/PathFence.java +++ b/src/main/java/org/apache/commons/text/lookup/PathFence.java @@ -94,7 +94,7 @@ private PathFence(final Builder builder) { */ Path apply(final String fileName) { final Path path = Paths.get(fileName); - if (roots.isEmpty()) { + if (isEmpty()) { return path; } final Path pathAbs = normalize(path); @@ -105,6 +105,15 @@ Path apply(final String fileName) { throw new IllegalArgumentException(String.format("[%s] -> [%s] not in the fence %s", fileName, pathAbs, roots)); } + /** + * Tests whether this fence has no roots, in which case {@link #apply(String)} lets every path through. + * + * @return whether this fence has no roots. + */ + boolean isEmpty() { + return roots.isEmpty(); + } + private Path normalize(final Path path) { return path.toAbsolutePath().normalize(); } diff --git a/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java b/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java new file mode 100644 index 0000000000..1154871f32 --- /dev/null +++ b/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache license, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the license for the specific language governing permissions and + * limitations under the license. + */ + +package org.apache.commons.text.lookup; + +import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.nio.file.FileSystemNotFoundException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Objects; + +import org.apache.commons.lang3.StringUtils; +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; + +/** + * Resolves the external resources of an XML document from within a {@link PathFence}. + *

    + * A document fetched from a fence may legitimately reference a follow-up resource, an external DTD subset or an external entity, living in the same roots. + *

    + * + * Keep package-private. + */ +final class PathFenceResolver implements EntityResolver { + + /** + * Converts a system identifier to a local Path. + * + * @param systemId An absolute 'file:' URI, may be null. + * @return A Path, or null if the system identifier is empty. + * @throws SAXException If the system identifier is not a valid `file:` URI. + */ + private static Path toPath(final String systemId) throws SAXException { + if (StringUtils.isEmpty(systemId)) { + return null; + } + try { + final URI uri = new URI(systemId); + if (!"file".equals(uri.getScheme())) { + throw new SAXParseException("Failed to read external document '" + systemId + "', because only 'file' access is allowed.", null, systemId, -1, + -1); + } + return Paths.get(uri); + } catch (final URISyntaxException | IllegalArgumentException | FileSystemNotFoundException e) { + throw new SAXParseException("Failed to read external document '" + systemId + "'.", null, systemId, -1, -1, e); + } + } + + /** + * A fence is made of Paths guarding Path resolution. + */ + private final PathFence fence; + + /** + * Constructs a new instance. + * + * @param fence The fence guarding Path resolution. + */ + PathFenceResolver(final PathFence fence) { + this.fence = Objects.requireNonNull(fence, "fence"); + } + + /** + * Resolves an external resource, opting it in when it resolves within our fence. + * + * @param publicId The public identifier, may be null. + * @param systemId The system identifier, already absolutized by the caller, may be null. + * @return An InputSource on the resource. + * @throws SAXException if the system identifier names a file outside our fence. + * @throws IOException if the resource cannot be read. + */ + @Override + public InputSource resolveEntity(final String publicId, final String systemId) throws SAXException, IOException { + final Path path = toPath(systemId); + if (path == null) { + return null; + } + final Path fenced; + try { + fenced = fence.apply(path.toString()); + } catch (final IllegalArgumentException e) { + throw new SAXException(e); + } + final InputSource inputSource = new InputSource(Files.newInputStream(fenced)); + inputSource.setPublicId(publicId); + inputSource.setSystemId(systemId); + return inputSource; + } +} 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 918689104c..af41f0cc1c 100644 --- a/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java +++ b/src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java @@ -30,6 +30,7 @@ import java.util.function.Function; import java.util.function.Supplier; +import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.xpath.XPathFactory; @@ -1614,8 +1615,19 @@ public StringLookup xmlEncoderStringLookup() { /** * Returns an XML StringLookup instance. *

    - * If this factory was built using {@link Builder#setFences(Path...)}, then the string lookup is fenced and will throw an {@link IllegalArgumentException} - * if a lookup causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions. + * XML files are parsed using Commons Secure XML, which enables the {@link XMLConstants#FEATURE_SECURE_PROCESSING} processing limits and ignores external + * DTD subsets and entities by default. + *

    + *

    + * If this factory was built using {@link Builder#setFences}, then the string lookup is fenced and additional features are available: + *

    + *
      + *
    • External DTD subsets and entities are enabled.
      • + *
    • The document and any external DTD subset or entity it references are read from within those fences, and a path resolving outside them throws an + * {@link IllegalArgumentException}.
      • + *
    + *

    + * Every resource inside the fences is considered trusted, so fence only directories whose contents you control. *

    *

    * We looks up values in an XML document in the format {@code "DocumentPath:XPath"}. @@ -1627,12 +1639,6 @@ public StringLookup xmlEncoderStringLookup() { *

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

    - * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects. Processing limits, such as the number of entity expansions, - * come from {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default and can be turned off through the factory features. - * External DTD subsets and external entities are blocked by an entity resolver rather than by a feature, so neither a feature nor a JAXP - * {@code javax.xml.accessExternal*} property can re-enable them. - *

    - *

    * Using a {@link StringLookup} from the {@link StringLookupFactory}: *

    * @@ -1660,8 +1666,19 @@ public StringLookup xmlStringLookup() { /** * Returns an XML StringLookup instance. *

    - * If this factory was built using {@link Builder#setFences(Path...)}, then the string lookup is fenced and will throw an {@link IllegalArgumentException} - * if a lookup causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions. + * XML files are parsed using Commons Secure XML, which enables the {@link XMLConstants#FEATURE_SECURE_PROCESSING} processing limits, which + * {@code factoryFeatures} can turn back off, and ignores external DTD subsets and entities by default. + *

    + *

    + * If this factory was built using {@link Builder#setFences}, then the string lookup is fenced and additional features are available: + *

    + *
      + *
    • External DTD subsets and entities are enabled.
      • + *
    • The document and any external DTD subset or entity it references are read from within those fences, and a path resolving outside them throws an + * {@link IllegalArgumentException}.
      • + *
    + *

    + * Every resource inside the fences is considered trusted, so fence only directories whose contents you control. *

    *

    * We looks up values in an XML document in the format {@code "]DocumentPath:XPath"}. @@ -1673,12 +1690,6 @@ public StringLookup xmlStringLookup() { *

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

    - * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects. Processing limits, such as the number of entity expansions, - * come from {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default and can be turned off through the factory features. - * External DTD subsets and external entities are blocked by an entity resolver rather than by a feature, so neither a feature nor a JAXP - * {@code javax.xml.accessExternal*} property can re-enable them. - *

    - *

    * Using a {@link StringLookup} from the {@link StringLookupFactory}: *

    * @@ -1709,8 +1720,19 @@ public StringLookup xmlStringLookup(final Map factoryFeatures) /** * Returns a fenced XML StringLookup instance. *

    - * If this factory was built using {@link Builder#setFences(Path...)}, then the string lookup is fenced and will throw an {@link IllegalArgumentException} - * if a lookup causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions. + * XML files are parsed using Commons Secure XML, which enables the {@link XMLConstants#FEATURE_SECURE_PROCESSING} processing limits, which + * {@code factoryFeatures} can turn back off, and ignores external DTD subsets and entities by default. + *

    + *

    + * If the {@code fences} argument is not empty, then the string lookup is fenced and additional features are available: + *

    + *
      + *
    • External DTD subsets and entities are enabled.
      • + *
    • The document and any external DTD subset or entity it references are read from within those fences, and a path resolving outside them throws an + * {@link IllegalArgumentException}.
      • + *
    + *

    + * Every resource inside the fences is considered trusted, so fence only directories whose contents you control. *

    *

    * We looks up values in an XML document in the format {@code "DocumentPath:XPath"}. @@ -1722,12 +1744,6 @@ public StringLookup xmlStringLookup(final Map factoryFeatures) *

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

    - * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects. Processing limits, such as the number of entity expansions, - * come from {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default and can be turned off through the factory features. - * External DTD subsets and external entities are blocked by an entity resolver rather than by a feature, so neither a feature nor a JAXP - * {@code javax.xml.accessExternal*} property can re-enable them. - *

    - *

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

    * 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 de88e5e7ab..1fb3f0c65a 100644 --- a/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java +++ b/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java @@ -26,6 +26,7 @@ import java.util.Objects; import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.xpath.XPathFactory; @@ -51,9 +52,12 @@ *
      *
    • Processing limits, such as the number of entity expansions, come from {@link XMLConstants#FEATURE_SECURE_PROCESSING}. That feature is enabled by * default, and the feature maps above can turn it off.
      • - *
    • External resource fetching, that is external DTD subsets and external entities, is blocked by an entity resolver rather than by a feature. Neither a - * feature nor a JAXP {@code javax.xml.accessExternal*} property can re-enable it.
      • + *
    • External resource fetching, that is external DTD subsets and external entities, is ignored by default and can not be enabled via the + * feature map. To enable external resource fetching, provide a non-empty list of fences.
      • *
    + *

    + * Every resource inside the fences is considered trusted, so fence only directories whose contents you control. + *

    * * @since 1.5 */ @@ -65,9 +69,9 @@ final class XmlStringLookup extends AbstractPathFencedLookup { private static final int KEY_PARTS_LEN = 2; /** - * Defines the singleton for this class, which sets no parser or XPath factory feature. + * Defines the singleton for this class, which sets no parser or XPath factory feature and has no fence. *

    - * Use {@link StringLookupFactory#xmlStringLookup(Map, Path...)} to set features; external resource resolution is off anyway. + * Use {@link StringLookupFactory#xmlStringLookup(Map, Path...)} to set features and fences; without any fence, external resources are ignored. *

    */ static final XmlStringLookup INSTANCE = new XmlStringLookup(Collections.emptyMap(), Collections.emptyMap(), (Path[]) null); @@ -105,8 +109,14 @@ final class XmlStringLookup extends AbstractPathFencedLookup { *
  • {@code "com/domain/document.xml:/path/to/node"}
    • * *

    - * The document is parsed through Apache Commons Secure XML: processing limits are governed by {@link XMLConstants#FEATURE_SECURE_PROCESSING}, which is - * enabled by default, while external DTD subsets and external entities are blocked outright and cannot be re-enabled. + * The document is parsed through Apache Commons Secure XML: + *

    + *
      + *
    • Processing limits are governed by {@link XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default.
      • + *
    • External DTD subsets and external entities are resolved only from the fences guarding this lookup if these are not empty.
      • + *
    + *

    + * Every resource inside the fences is considered trusted, so fence only directories whose contents you control. *

    * * @param key The key to be looked up, may be null. @@ -129,8 +139,15 @@ public String lookup(final String key) { 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 Path documentFile = getPath(documentPath); + try (InputStream inputStream = Files.newInputStream(documentFile)) { + final DocumentBuilder documentBuilder = dbFactory.newDocumentBuilder(); + // If the fence is not empty, opt-in follow-up resources fetched from the fence. + if (!fence.isEmpty()) { + documentBuilder.setEntityResolver(new PathFenceResolver(fence)); + } + // Parsing with the document's own URI gives relative system identifiers a base URI to resolve against, as XML requires. + final Document doc = documentBuilder.parse(inputStream, documentFile.toUri().toString()); final XPathFactory xpFactory = SecureXPathFactory.newInstance(); for (final Entry p : xPathFactoryFeatures.entrySet()) { xpFactory.setFeature(p.getKey(), p.getValue()); diff --git a/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java b/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java new file mode 100644 index 0000000000..91c75b3728 --- /dev/null +++ b/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache license, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the license for the specific language governing permissions and + * limitations under the license. + */ + +package org.apache.commons.text.lookup; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +import org.apache.commons.lang3.StringUtils; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +/** + * Tests {@link PathFenceResolver}. + */ +class PathFenceResolverTest { + + private static final String DATA = "Hello World!"; + private static final Path CURRENT_PATH = Paths.get(StringUtils.EMPTY); // NOT "." + + /** + * Reads an InputSource byte stream as UTF-8. + */ + private static String read(final InputSource inputSource) throws IOException { + final ByteArrayOutputStream out = new ByteArrayOutputStream(); + try (InputStream in = inputSource.getByteStream()) { + final byte[] buffer = new byte[1024]; + for (int len = in.read(buffer); len != -1; len = in.read(buffer)) { + out.write(buffer, 0, len); + } + } + return new String(out.toByteArray(), StandardCharsets.UTF_8); + } + + private static PathFenceResolver resolver(final Path... roots) { + return new PathFenceResolver(PathFence.builder().setRoots(roots).get()); + } + + /** + * Writes a file and gives back its path relative to the working directory. + */ + private static Path write(final Path path, final String content) throws IOException { + Files.createDirectories(path.getParent()); + Files.write(path, content.getBytes(StandardCharsets.UTF_8)); + return CURRENT_PATH.toAbsolutePath().relativize(path.toAbsolutePath()); + } + + @Test + void testAbsentFileInFence(@TempDir final Path tempDir) { + assertThrows(IOException.class, () -> resolver(tempDir).resolveEntity(null, tempDir.resolve("absent.txt").toUri().toString())); + } + + @Test + void testDrivePathIsRefused(@TempDir final Path tempDir) { + // A bare Windows drive path is not a 'file:' URI, so it is refused like any other non-file system identifier. + assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "C:/does-not-matter.txt")); + } + + @Test + void testEmptySystemId(@TempDir final Path tempDir) throws Exception { + assertNull(resolver(tempDir).resolveEntity(null, StringUtils.EMPTY)); + } + + @Test + void testFileUrlInFence(@TempDir final Path tempDir) throws Exception { + final Path target = write(tempDir.resolve("entity.txt"), DATA); + final InputSource inputSource = resolver(tempDir).resolveEntity("publicId", target.toUri().toString()); + assertNotNull(inputSource); + assertEquals("publicId", inputSource.getPublicId()); + assertEquals(DATA, read(inputSource)); + } + + @Test + void testFileUrlOutsideFence(@TempDir final Path tempDir) throws Exception { + final Path target = write(tempDir.resolve("out/entity.txt"), DATA); + final SAXException e = assertThrows(SAXException.class, () -> resolver(tempDir.resolve("in")).resolveEntity(null, target.toUri().toString())); + assertInstanceOf(IllegalArgumentException.class, e.getCause()); + } + + @Test + void testNullFence() { + assertThrows(NullPointerException.class, () -> new PathFenceResolver(null)); + } + + @Test + void testNullSystemId(@TempDir final Path tempDir) throws Exception { + assertNull(resolver(tempDir).resolveEntity(null, null)); + } + + @Test + void testPercentEncodedFileUrlInFence(@TempDir final Path tempDir) throws Exception { + final Path target = write(tempDir.resolve("na me.txt"), DATA); + final InputSource inputSource = resolver(tempDir).resolveEntity(null, target.toUri().toString()); + assertNotNull(inputSource); + assertEquals(DATA, read(inputSource)); + } + + @Test + void testRelativeSystemIdIsRefused(@TempDir final Path tempDir) throws Exception { + // XmlStringLookup parses with the document's own URI, so a relative system identifier always reaches us absolutized. + final Path target = write(tempDir.resolve("entity.txt"), DATA); + assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, target.toString().replace('\\', '/'))); + } + + @Test + void testRemoteSystemIdIsRefused(@TempDir final Path tempDir) { + // A remote reference names no path, so the fence can never opt it in. + assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "http://localhost:1/entity.txt")); + assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "https://localhost:1/entity.txt")); + assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "ftp://localhost:1/entity.txt")); + assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "jar:file:/lib.jar!/entity.txt")); + } + + @Test + void testSystemIdIsEchoed(@TempDir final Path tempDir) throws Exception { + final Path target = write(tempDir.resolve("entity.txt"), DATA); + final String systemId = target.toUri().toString(); + assertEquals(systemId, resolver(tempDir).resolveEntity(null, systemId).getSystemId()); + } +} 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 0547349dfd..67ab3cb804 100644 --- a/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java +++ b/src/test/java/org/apache/commons/text/lookup/StringLookupFactoryTest.java @@ -34,7 +34,6 @@ import javax.xml.XMLConstants; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.DefaultLocale; @@ -302,16 +301,18 @@ void testXmlStringLookupEmptyPaths() { @Test void testXmlStringLookupExternalEntityOff() { XmlStringLookupTest.assertDoesNotLeak( - () -> StringLookupFactory.INSTANCE.xmlStringLookup().apply(XmlStringLookupTest.DOC_DIR + "document-entity-ref.xml:/document/content"), + () -> StringLookupFactory.INSTANCE.xmlStringLookup().apply(XmlStringLookupTest.FENCE_DOCS + "document-entity-ref.xml:/document/content"), XmlStringLookupTest.DATA); } @Test - @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") 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()); + // A fence opts the external entity in: it resolves inside the fence, one directory above the document. + final String key = XmlStringLookupTest.FENCE_DOCS + "document-entity-ref.xml:/document/content"; + final StringLookup lookup = StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP, XmlStringLookupTest.FENCE_ROOT); + assertEquals(XmlStringLookupTest.DATA, lookup.apply(key).trim()); } + @Test void testXmlStringLookupMultiplePaths() { final Path documentPath = Paths.get(XmlStringLookupTest.DOC_DIR); 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 e8e02f5e04..13c3ac2485 100644 --- a/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java +++ b/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java @@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.file.Path; import java.nio.file.Paths; @@ -34,10 +35,9 @@ import javax.xml.XMLConstants; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.commons.text.StringSubstitutor; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; -import org.junitpioneer.jupiter.SetSystemProperty; /** * Tests {@link XmlStringLookup}. @@ -46,11 +46,18 @@ class XmlStringLookupTest { static final String DATA = "Hello World!"; static final Map EMPTY_MAP = Collections.emptyMap(); + static final String DOC_DIR = "src/test/resources/org/apache/commons/text/"; private static final Path CURRENT_PATH = Paths.get(StringUtils.EMPTY); // NOT "." private static final Path ABSENT_PATH = Paths.get("does not exist at all"); - static final String DOC_DIR = "src/test/resources/org/apache/commons/text/"; private static final String DOC_RELATIVE = DOC_DIR + "document.xml"; private static final String DOC_ROOT = "/document.xml"; + private static final String DTD_DATA = "This is an external entity."; + /** Holds the files the fixture documents reference. */ + private static final String FENCE_DIR = "src/test/resources/XmlStringLookup/"; + static final Path FENCE_ROOT = Paths.get(FENCE_DIR); + /** Holds the documents themselves, one directory below the files they reference. */ + static final String FENCE_DOCS = FENCE_DIR + "documents/"; + private static final Path FENCE_DOCS_PATH = Paths.get(FENCE_DOCS); /** * Asserts external content does not leak @@ -61,6 +68,25 @@ static void assertDoesNotLeak(final Supplier lookup, final String extern assertFalse(result.contains(external), () -> "external content leaked: " + result); } + /** + * Builds a substitutor whose {@code xml} lookup is fenced. {@link StringSubstitutor#createInterpolator()} shares one + * {@link InterpolatorStringLookup} instance JVM-wide, so its lookup map must not be mutated here. + */ + private static StringSubstitutor fencedInterpolator(final Path... fences) { + final Map stringLookupMap = new HashMap<>(1); + stringLookupMap.put(StringLookupFactory.KEY_XML, StringLookupFactory.INSTANCE.xmlStringLookup(EMPTY_MAP, fences)); + return new StringSubstitutor(StringLookupFactory.INSTANCE.interpolatorStringLookup(stringLookupMap, null, false)); + } + + /** + * Asserts the lookup is refused because {@code fileName}, not the document itself, resolves outside the fence. + */ + static void assertRefusesOutsideFence(final Supplier lookup, final String fileName) { + final IllegalArgumentException e = assertThrows(IllegalArgumentException.class, lookup::get); + final String message = ExceptionUtils.getRootCauseMessage(e); + assertTrue(message.contains(fileName) && message.contains("not in the fence"), () -> "unexpected refusal: " + message); + } + static void assertLookup(final StringLookup xmlStringLookup) { assertNotNull(xmlStringLookup); assertInstanceOf(XmlStringLookup.class, xmlStringLookup); @@ -73,58 +99,74 @@ void testBadXPath() { assertThrows(IllegalArgumentException.class, () -> XmlStringLookup.INSTANCE.apply("docName")); } + @Test + void testExternalDtdOff() { + assertDoesNotLeak( + () -> new XmlStringLookup(EMPTY_MAP, EMPTY_MAP).apply(FENCE_DOCS + "document-external-dtd.xml:/document/content"), DTD_DATA); + } + @Test void testExternalEntityOff() { assertDoesNotLeak( - () -> new XmlStringLookup(EMPTY_MAP, EMPTY_MAP).apply(DOC_DIR + "document-entity-ref.xml:/document/content"), DATA); + () -> new XmlStringLookup(EMPTY_MAP, EMPTY_MAP).apply(FENCE_DOCS + "document-entity-ref.xml:/document/content"), DATA); } @Test - @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") - void testExternalEntityOn() { - final String key = DOC_DIR + "document-entity-ref.xml:/document/content"; - assertEquals(DATA, new XmlStringLookup(EMPTY_MAP, EMPTY_MAP).apply(key).trim()); + void testFenceAllowsExternalDtd() { + // The fence covers the document and the DTD it references in the parent directory. + assertEquals(DTD_DATA, + new XmlStringLookup(EMPTY_MAP, EMPTY_MAP, FENCE_ROOT).apply(FENCE_DOCS + "document-external-dtd.xml:/document/content").trim()); } @Test - void testInterpolatorExternalDtdOff() { - final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-external-dtd.xml:/document/content}"), - "This is an external entity."); + void testFenceAllowsExternalEntity() { + // The fence covers the document and the entity it references in the parent directory. + assertEquals(DATA, new XmlStringLookup(EMPTY_MAP, EMPTY_MAP, FENCE_ROOT).apply(FENCE_DOCS + "document-entity-ref.xml:/document/content").trim()); } @Test - @SetSystemProperty(key = "javax.xml.accessExternalDTD", value = "file") - @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") - 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()); + void testFenceBlocksExternalDtdOutsideFence() { + // The fence covers the document only, so its DTD in the parent directory is out of reach. + final XmlStringLookup lookup = new XmlStringLookup(EMPTY_MAP, EMPTY_MAP, FENCE_DOCS_PATH); + assertRefusesOutsideFence(() -> lookup.apply(FENCE_DOCS + "document-external-dtd.xml:/document/content"), "document.dtd"); } @Test - void testInterpolatorExternalEntityOff() { - final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); + void testFenceBlocksExternalEntityOutsideFence() { + // The fence covers the document only, so its entity in the parent directory is out of reach. + final XmlStringLookup lookup = new XmlStringLookup(EMPTY_MAP, EMPTY_MAP, FENCE_DOCS_PATH); + assertRefusesOutsideFence(() -> lookup.apply(FENCE_DOCS + "document-entity-ref.xml:/document/content"), "xml-entity.txt"); } @Test - @SetSystemProperty(key = "javax.xml.accessExternalDTD", value = "file") - @Disabled("External entities are blocked by Commons Secure XML through an entity resolver and can no longer be re-enabled.") - void testInterpolatorExternalEntityOffOverride() { - final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertEquals(DATA, stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}").trim()); + void testFenceBlocksRemoteEntity() { + // A remote reference names no path, so no fence can ever opt it in. + final XmlStringLookup lookup = new XmlStringLookup(EMPTY_MAP, EMPTY_MAP, FENCE_ROOT); + assertThrows(IllegalArgumentException.class, () -> lookup.apply(FENCE_DOCS + "document-remote-entity.xml:/document/content")); + } + + @Test + void testFencedInterpolatorExternalDtdOn() { + final StringSubstitutor stringSubstitutor = fencedInterpolator(FENCE_ROOT); + assertEquals(DTD_DATA, stringSubstitutor.replace("${xml:" + FENCE_DOCS + "document-external-dtd.xml:/document/content}").trim()); } @Test - void testInterpolatorExternalEntityOn() { + void testFencedInterpolatorExternalEntityOn() { + final StringSubstitutor stringSubstitutor = fencedInterpolator(FENCE_ROOT); + assertEquals(DATA, stringSubstitutor.replace("${xml:" + FENCE_DOCS + "document-entity-ref.xml:/document/content}").trim()); + } + + @Test + void testInterpolatorExternalDtdOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); + assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + FENCE_DOCS + "document-external-dtd.xml:/document/content}"), DTD_DATA); } @Test - void testInterpolatorExternalEntityOnOverride() { + void testInterpolatorExternalEntityOff() { final StringSubstitutor stringSubstitutor = StringSubstitutor.createInterpolator(); - assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + DOC_DIR + "document-entity-ref.xml:/document/content}"), DATA); + assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" + FENCE_DOCS + "document-entity-ref.xml:/document/content}"), DATA); } @Test diff --git a/src/test/resources/XmlStringLookup/document.dtd b/src/test/resources/XmlStringLookup/document.dtd new file mode 100644 index 0000000000..f652db2d9e --- /dev/null +++ b/src/test/resources/XmlStringLookup/document.dtd @@ -0,0 +1,20 @@ + + + + + diff --git a/src/test/resources/org/apache/commons/text/document-entity-ref.xml b/src/test/resources/XmlStringLookup/documents/document-entity-ref.xml similarity index 86% rename from src/test/resources/org/apache/commons/text/document-entity-ref.xml rename to src/test/resources/XmlStringLookup/documents/document-entity-ref.xml index bfd3609802..37050d6264 100644 --- a/src/test/resources/org/apache/commons/text/document-entity-ref.xml +++ b/src/test/resources/XmlStringLookup/documents/document-entity-ref.xml @@ -16,11 +16,11 @@ limitations under the License. --> + ]> - Example of an External Entity + External entity in the parent directory - &ext; + &ext; diff --git a/src/test/resources/org/apache/commons/text/document-external-dtd.xml b/src/test/resources/XmlStringLookup/documents/document-external-dtd.xml similarity index 86% rename from src/test/resources/org/apache/commons/text/document-external-dtd.xml rename to src/test/resources/XmlStringLookup/documents/document-external-dtd.xml index 158f5d8336..12fb1345ac 100644 --- a/src/test/resources/org/apache/commons/text/document-external-dtd.xml +++ b/src/test/resources/XmlStringLookup/documents/document-external-dtd.xml @@ -15,9 +15,9 @@ See the License for the specific language governing permissions and limitations under the License. --> - + - Example of an External Entity + External DTD subset in the parent directory &externalEntity; diff --git a/src/test/resources/XmlStringLookup/documents/document-remote-entity.xml b/src/test/resources/XmlStringLookup/documents/document-remote-entity.xml new file mode 100644 index 0000000000..98bc5aa2bd --- /dev/null +++ b/src/test/resources/XmlStringLookup/documents/document-remote-entity.xml @@ -0,0 +1,26 @@ + + + +]> + + Remote external entity + + &ext; + + diff --git a/src/test/resources/org/apache/commons/text/xml-entity.txt b/src/test/resources/XmlStringLookup/xml-entity.txt similarity index 100% rename from src/test/resources/org/apache/commons/text/xml-entity.txt rename to src/test/resources/XmlStringLookup/xml-entity.txt diff --git a/src/test/resources/org/apache/commons/text/document-external-dtd.dtd b/src/test/resources/org/apache/commons/text/document-external-dtd.dtd deleted file mode 100644 index dbc9ca746b..0000000000 --- a/src/test/resources/org/apache/commons/text/document-external-dtd.dtd +++ /dev/null @@ -1,20 +0,0 @@ - - - - - From 21c0bd98bd99cbe39e878acbfa5f7af7f39bed16 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 9 Sep 2026 09:31:57 +0200 Subject: [PATCH 7/9] Add a changelog entry for the base URI fix XmlStringLookup used to parse through DocumentBuilder.parse(InputStream), which supplies no base URI, so the parser expanded a relative system identifier against the process working directory rather than against the document. That defect shipped in 1.5 and was reachable through StringLookupFactory.xmlStringLookup(Map) with a feature map that left external references enabled, so record it as a fix of its own rather than folding it into the Commons Secure XML entries. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt --- src/changes/changes.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index a90a87ab9b..36a9e7c4e8 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -70,6 +70,7 @@ The type attribute can be add,update,fix,remove. TextStringBuilder.lastIndexOf("") and StrBuilder.lastIndexOf("") return incorrect index for empty string (size - 1 instead of size) (#763). StringSubstitutorReader can now substitute variables with a suffix longer than one characters (#764). Fix ArrayIndexOutOfBoundsException in DnsStringLookup for '|' key (#765). + XmlStringLookup resolves relative system identifiers against the XML document, instead of the current working directory. XmlStringLookup creates its XML parser and XPath factories through Commons Secure XML. XmlStringLookup resolves external DTD subsets and entities from its Path fences. From b1eca27f90185d49ca7c14ef69cb4bacb6b3fc68 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 9 Sep 2026 09:41:48 +0200 Subject: [PATCH 8/9] Cover the malformed system identifier branch of PathFenceResolver PathFenceResolver.toPath catches URISyntaxException, IllegalArgumentException and FileSystemNotFoundException, but every refusal the tests exercised went through the preceding scheme check instead, leaving that catch untested. - testMalformedSystemIdIsRefused covers the URISyntaxException: an unencoded space, the realistic way a system identifier goes malformed. - testUnconvertibleFileUrlIsRefused covers the IllegalArgumentException from Paths.get on a 'file:' URI that parses and passes the scheme check but names no path: an opaque one, and one carrying a fragment. Both assert on the cause, so a change that made the scheme check reject these earlier would not silently keep the tests green. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt --- .../text/lookup/PathFenceResolverTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java b/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java index 91c75b3728..51f1e6d7f0 100644 --- a/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java +++ b/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java @@ -26,6 +26,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -104,6 +105,13 @@ void testFileUrlOutsideFence(@TempDir final Path tempDir) throws Exception { assertInstanceOf(IllegalArgumentException.class, e.getCause()); } + @Test + void testMalformedSystemIdIsRefused(@TempDir final Path tempDir) { + // An unencoded space is illegal in a URI, so the identifier never becomes a path. + final SAXException e = assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "file:/a b/entity.txt")); + assertInstanceOf(URISyntaxException.class, e.getCause()); + } + @Test void testNullFence() { assertThrows(NullPointerException.class, () -> new PathFenceResolver(null)); @@ -144,4 +152,13 @@ void testSystemIdIsEchoed(@TempDir final Path tempDir) throws Exception { final String systemId = target.toUri().toString(); assertEquals(systemId, resolver(tempDir).resolveEntity(null, systemId).getSystemId()); } + + @Test + void testUnconvertibleFileUrlIsRefused(@TempDir final Path tempDir) { + // A well-formed 'file:' URI that names no path: opaque, and carrying a fragment. + final SAXException opaque = assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "file:entity.txt")); + assertInstanceOf(IllegalArgumentException.class, opaque.getCause()); + final SAXException fragment = assertThrows(SAXException.class, () -> resolver(tempDir).resolveEntity(null, "file:/entity.txt#frag")); + assertInstanceOf(IllegalArgumentException.class, fragment.getCause()); + } } From 3f797a6eb903605408180ad58dac41833bf141de Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 9 Sep 2026 09:55:51 +0200 Subject: [PATCH 9/9] Drop the unreachable FileSystemNotFoundException catch Paths.get(URI) raises FileSystemNotFoundException only for a scheme whose provider is not installed, and PathFenceResolver.toPath rejects every scheme but 'file' before it gets there, so that catch type could never fire. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt --- .../java/org/apache/commons/text/lookup/PathFenceResolver.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java b/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java index 1154871f32..3458017d60 100644 --- a/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java +++ b/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java @@ -20,7 +20,6 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; -import java.nio.file.FileSystemNotFoundException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -60,7 +59,7 @@ private static Path toPath(final String systemId) throws SAXException { -1); } return Paths.get(uri); - } catch (final URISyntaxException | IllegalArgumentException | FileSystemNotFoundException e) { + } catch (final URISyntaxException | IllegalArgumentException e) { throw new SAXParseException("Failed to read external document '" + systemId + "'.", null, systemId, -1, -1, e); } }