This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-text.git
The following commit(s) were added to refs/heads/master by this push:
new ae3d36b8 Harden XML parsing via commons-secure-xml (#766)
ae3d36b8 is described below
commit ae3d36b8cea5d1fa5eaf3bd28f07bb111d1890e0
Author: Piotr P. Karwasz <[email protected]>
AuthorDate: Wed Sep 9 15:43:13 2026 +0200
Harden XML parsing via commons-secure-xml (#766)
* 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 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MHgnMnGWHQoH2zD2jFdoMT
* 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 <[email protected]>
Claude-Session: https://claude.ai/code/session_0167e29ScPEdfzJnEFm95imK
* Bump Apache Commons Secure XML from 1.0.0-SNAPSHOT to 1.0.0
* 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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Y7VfssZLkK11bBY2kVyyY4
* Add missing tests
* 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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt
* 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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt
* 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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt
* 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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QoqFgJRAAHAHhvJTfDRfUt
---------
Co-authored-by: Gary Gregory <[email protected]>
---
pom.xml | 5 +
src/changes/changes.xml | 3 +
.../org/apache/commons/text/lookup/PathFence.java | 11 +-
.../commons/text/lookup/PathFenceResolver.java | 107 ++++++++++++++
.../commons/text/lookup/StringLookupFactory.java | 59 +++++---
.../commons/text/lookup/XmlStringLookup.java | 67 +++++----
.../commons/text/lookup/PathFenceResolverTest.java | 164 +++++++++++++++++++++
.../text/lookup/StringLookupFactoryTest.java | 54 ++++++-
.../commons/text/lookup/XmlStringLookupTest.java | 109 ++++++++++----
.../document.dtd} | 5 +-
.../documents}/document-entity-ref.xml | 6 +-
.../documents}/document-external-dtd.xml | 4 +-
.../documents/document-remote-entity.xml} | 6 +-
.../text => XmlStringLookup}/xml-entity.txt | 0
.../apache/commons/text/document-external-dtd.dtd | 20 ---
15 files changed, 512 insertions(+), 108 deletions(-)
diff --git a/pom.xml b/pom.xml
index 150a58e9..7e37f37b 100644
--- a/pom.xml
+++ b/pom.xml
@@ -87,6 +87,11 @@
<artifactId>commons-lang3</artifactId>
<version>${commons.lang3.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-secure-xml</artifactId>
+ <version>1.0.0</version>
+ </dependency>
<!-- testing -->
<dependency>
<groupId>org.junit.jupiter</groupId>
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 043e37d2..c456633a 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -72,7 +72,10 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="ggregory" due-to="Javid Khan, Gary Gregory">Fix
ArrayIndexOutOfBoundsException in DnsStringLookup for '|' key (#765).</action>
<action type="fix" dev="ggregory" due-to="Weiki, Gary Gregory">Compare
characters by value in DamerauLevenshteinDistance (#770).</action>
<action type="fix" dev="ggregory" due-to="Jeff Lenamon, Gary
Gregory">StringMatcher.isMatch(CharSequence, int, int, int) forwards bufferEnd
as bufferStart (#768).</action>
+ <action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary
Gregory">XmlStringLookup resolves relative system identifiers against the XML
document, instead of the current working directory.</action>
<!-- ADD -->
+ <action type="add" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary
Gregory">XmlStringLookup creates its XML parser and XPath factories through
Commons Secure XML.</action>
+ <action type="add" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary
Gregory">XmlStringLookup resolves external DTD subsets and entities from its
Path fences.</action>
<!-- UPDATE -->
<action type="update" dev="ggregory" due-to="Gary Gregory">Bump
org.apache.commons:commons-parent from 93 to 105.</action>
<action type="update" dev="ggregory" due-to="Gary Gregory">Bump the
level of test coverage checks.</action>
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 0bd884a5..c914fa1d 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 @@ final class PathFence {
*/
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 @@ final class PathFence {
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 00000000..3458017d
--- /dev/null
+++ b/src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java
@@ -0,0 +1,107 @@
+/*
+ * 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.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}.
+ * <p>
+ * 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.
+ * </p>
+ *
+ * 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 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 3feb4fe5..af41f0cc 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.BiFunction;
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 final class StringLookupFactory {
/**
* Returns an XML StringLookup instance.
* <p>
- * 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.
+ * </p>
+ * <p>
+ * If this factory was built using {@link Builder#setFences}, then the
string lookup is <strong>fenced</strong> and additional features are available:
+ * </p>
+ * <ul>
+ * <li>External DTD subsets and entities are enabled.</li>
+ * <li>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}.</li>
+ * </ul>
+ * <p>
+ * Every resource inside the fences is considered trusted, so fence only
directories whose contents you control.
* </p>
* <p>
* We looks up values in an XML document in the format {@code
"DocumentPath:XPath"}.
@@ -1627,10 +1639,6 @@ public final class StringLookupFactory {
* <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)}.
- * </p>
- * <p>
* Using a {@link StringLookup} from the {@link StringLookupFactory}:
* </p>
*
@@ -1652,14 +1660,25 @@ public final class StringLookupFactory {
* @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;
}
/**
* Returns an XML StringLookup instance.
* <p>
- * 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.
+ * </p>
+ * <p>
+ * If this factory was built using {@link Builder#setFences}, then the
string lookup is <strong>fenced</strong> and additional features are available:
+ * </p>
+ * <ul>
+ * <li>External DTD subsets and entities are enabled.</li>
+ * <li>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}.</li>
+ * </ul>
+ * <p>
+ * Every resource inside the fences is considered trusted, so fence only
directories whose contents you control.
* </p>
* <p>
* We looks up values in an XML document in the format {@code
"]DocumentPath:XPath"}.
@@ -1671,10 +1690,6 @@ public final class StringLookupFactory {
* <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)}.
- * </p>
- * <p>
* Using a {@link StringLookup} from the {@link StringLookupFactory}:
* </p>
*
@@ -1705,8 +1720,19 @@ public final class StringLookupFactory {
/**
* Returns a fenced XML StringLookup instance.
* <p>
- * 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.
+ * </p>
+ * <p>
+ * If the {@code fences} argument is not empty, then the string lookup is
<strong>fenced</strong> and additional features are available:
+ * </p>
+ * <ul>
+ * <li>External DTD subsets and entities are enabled.</li>
+ * <li>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}.</li>
+ * </ul>
+ * <p>
+ * Every resource inside the fences is considered trusted, so fence only
directories whose contents you control.
* </p>
* <p>
* We looks up values in an XML document in the format {@code
"DocumentPath:XPath"}.
@@ -1718,9 +1744,6 @@ public final class StringLookupFactory {
* <li>{@code "com/domain/document.xml:/path/to/node"}</li>
* </ul>
* <p>
- * 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>
*
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 624c7d01..1fb3f0c6 100644
--- a/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java
+++ b/src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java
@@ -20,16 +20,19 @@ package org.apache.commons.text.lookup;
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;
import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
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;
/**
@@ -41,7 +44,19 @@ import org.w3c.dom.Document;
* <li>{@code "com/domain/document.xml:/path/to/node"}</li>
* </ul>
* <p>
- * 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)}.
+ * </p>
+ * <p>
+ * Documents are parsed through Apache Commons Secure XML, which secures two
separate aspects:
+ * </p>
+ * <ul>
+ * <li>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.</li>
+ * <li>External resource fetching, that is external DTD subsets and external
entities, is ignored by default and can <strong>not</strong> be enabled via the
+ * feature map. To enable external resource fetching, provide a non-empty list
of <strong>fences</strong>.</li>
+ * </ul>
+ * <p>
+ * Every resource inside the fences is considered trusted, so fence only
directories whose contents you control.
* </p>
*
* @since 1.5
@@ -54,28 +69,12 @@ final class XmlStringLookup extends
AbstractPathFencedLookup {
private static final int KEY_PARTS_LEN = 2;
/**
- * Defines default XPath factory features.
- */
- static final Map<String, Boolean> DEFAULT_XPATH_FEATURES;
-
- /**
- * Defines default XML factory features.
- */
- static final Map<String, Boolean> 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 and has no fence.
* <p>
- * 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 and fences; without any fence, external resources are ignored.
* </p>
*/
- 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.
@@ -98,7 +97,7 @@ final class XmlStringLookup extends AbstractPathFencedLookup {
XmlStringLookup(final Map<String, Boolean> xmlFactoryFeatures, final
Map<String, Boolean> xPathFactoryFeatures, final Path... fences) {
super(fences);
this.xmlFactoryFeatures = Objects.requireNonNull(xmlFactoryFeatures,
"xmlFactoryFeatures");
- this.xPathFactoryFeatures =
Objects.requireNonNull(xPathFactoryFeatures, "xPathFfactoryFeatures");
+ this.xPathFactoryFeatures =
Objects.requireNonNull(xPathFactoryFeatures, "xPathFactoryFeatures");
}
/**
@@ -110,7 +109,14 @@ final class XmlStringLookup extends
AbstractPathFencedLookup {
* <li>{@code "com/domain/document.xml:/path/to/node"}</li>
* </ul>
* <p>
- * 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:
+ * </p>
+ * <ul>
+ * <li>Processing limits are governed by {@link
XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default.</li>
+ * <li>External DTD subsets and external entities are resolved only from
the fences guarding this lookup if these are not empty.</li>
+ * </ul>
+ * <p>
+ * Every resource inside the fences is considered trusted, so fence only
directories whose contents you control.
* </p>
*
* @param key The key to be looked up, may be null.
@@ -128,14 +134,21 @@ final class XmlStringLookup extends
AbstractPathFencedLookup {
}
final String documentPath = keys[0];
final String xpath = StringUtils.substringAfterLast(key, SPLIT_CH);
- final DocumentBuilderFactory dbFactory =
DocumentBuilderFactory.newInstance();
+ final DocumentBuilderFactory dbFactory =
SecureDocumentBuilderFactory.newInstance();
try {
for (final Entry<String, Boolean> 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 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<String, Boolean> 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 00000000..51f1e6d7
--- /dev/null
+++ b/src/test/java/org/apache/commons/text/lookup/PathFenceResolverTest.java
@@ -0,0 +1,164 @@
+/*
+ * 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.net.URISyntaxException;
+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 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));
+ }
+
+ @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());
+ }
+
+ @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());
+ }
+}
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 e0fdf8ab..67ab3cb8 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.assertSame;
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;
@@ -33,7 +36,6 @@ import javax.xml.XMLConstants;
import org.junit.jupiter.api.Test;
import org.junitpioneer.jupiter.DefaultLocale;
-import org.junitpioneer.jupiter.SetSystemProperty;
/**
* Tests {@link StringLookupFactory}.
@@ -291,16 +293,56 @@ class StringLookupFactoryTest {
XmlStringLookupTest.assertLookup(stringLookupFactory.xmlStringLookup(new
HashMap<>()));
}
+ @Test
+ void testXmlStringLookupEmptyPaths() {
+
XmlStringLookupTest.assertLookup(StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP,
new Path[0]));
+ }
+
@Test
void testXmlStringLookupExternalEntityOff() {
- assertThrows(IllegalArgumentException.class,
- () ->
StringLookupFactory.INSTANCE.xmlStringLookup().apply(XmlStringLookupTest.DOC_DIR
+ "document-entity-ref.xml:/document/content"));
+ XmlStringLookupTest.assertDoesNotLeak(
+ () ->
StringLookupFactory.INSTANCE.xmlStringLookup().apply(XmlStringLookupTest.FENCE_DOCS
+ "document-entity-ref.xml:/document/content"),
+ XmlStringLookupTest.DATA);
}
@Test
- @SetSystemProperty(key = "XmlStringLookup.secure", value = "false")
void testXmlStringLookupExternalEntityOn() {
- final String key = XmlStringLookupTest.DOC_DIR +
"document-entity-ref.xml:/document/content";
- assertEquals(XmlStringLookupTest.DATA,
StringLookupFactory.INSTANCE.xmlStringLookup(XmlStringLookupTest.EMPTY_MAP).apply(key).trim());
+ // 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);
+ 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<String, Boolean> 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));
+ }
+
}
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 862fdf81..13c3ac24 100644
--- a/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java
+++ b/src/test/java/org/apache/commons/text/lookup/XmlStringLookupTest.java
@@ -23,19 +23,21 @@ 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 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.Map;
+import java.util.function.Supplier;
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.Test;
-import org.junitpioneer.jupiter.SetSystemProperty;
/**
* Tests {@link XmlStringLookup}.
@@ -44,11 +46,46 @@ class XmlStringLookupTest {
static final String DATA = "Hello World!";
static final Map<String, Boolean> 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
+ */
+ static void assertDoesNotLeak(final Supplier<String> lookup, final String
external) {
+ final String result = lookup.get();
+ assertNotNull(result, "lookup returned null");
+ 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<String, StringLookup> 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<String> 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);
@@ -62,62 +99,80 @@ class XmlStringLookupTest {
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() {
- assertThrows(IllegalArgumentException.class,
- () -> new
XmlStringLookup(XmlStringLookup.DEFAULT_XML_FEATURES, EMPTY_MAP).apply(DOC_DIR
+ "document-entity-ref.xml:/document/content"));
+ assertDoesNotLeak(
+ () -> new XmlStringLookup(EMPTY_MAP,
EMPTY_MAP).apply(FENCE_DOCS + "document-entity-ref.xml:/document/content"),
DATA);
}
@Test
- 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());
+ 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();
- assertThrows(IllegalArgumentException.class, () ->
stringSubstitutor.replace("${xml:" + DOC_DIR +
"document-external-dtd.xml:/document/content}"));
+ 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")
- 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();
- assertThrows(IllegalArgumentException.class, () ->
stringSubstitutor.replace("${xml:" + DOC_DIR +
"document-entity-ref.xml:/document/content}"));
+ 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")
- 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();
- assertThrows(IllegalArgumentException.class, () ->
stringSubstitutor.replace("${xml:" + DOC_DIR +
"document-entity-ref.xml:/document/content}"));
+ assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" +
FENCE_DOCS + "document-external-dtd.xml:/document/content}"), DTD_DATA);
}
@Test
- void testInterpolatorExternalEntityOnOverride() {
+ void testInterpolatorExternalEntityOff() {
final StringSubstitutor stringSubstitutor =
StringSubstitutor.createInterpolator();
- assertThrows(IllegalArgumentException.class, () ->
stringSubstitutor.replace("${xml:" + DOC_DIR +
"document-entity-ref.xml:/document/content}"));
+ assertDoesNotLeak(() -> stringSubstitutor.replace("${xml:" +
FENCE_DOCS + "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
diff --git a/src/test/resources/org/apache/commons/text/xml-entity.txt
b/src/test/resources/XmlStringLookup/document.dtd
similarity index 84%
copy from src/test/resources/org/apache/commons/text/xml-entity.txt
copy to src/test/resources/XmlStringLookup/document.dtd
index 4f2fa758..f652db2d 100644
--- a/src/test/resources/org/apache/commons/text/xml-entity.txt
+++ b/src/test/resources/XmlStringLookup/document.dtd
@@ -14,4 +14,7 @@
See the License for the specific language governing permissions and
limitations under the License.
-->
-Hello World!
+<!ENTITY externalEntity "This is an external entity.">
+<!ELEMENT document (title, content)>
+<!ELEMENT title (#PCDATA)>
+<!ELEMENT content (#PCDATA)>
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%
copy from src/test/resources/org/apache/commons/text/document-entity-ref.xml
copy to src/test/resources/XmlStringLookup/documents/document-entity-ref.xml
index bfd36098..37050d62 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.
-->
<!DOCTYPE document [
- <!ENTITY ext SYSTEM
"src/test/resources/org/apache/commons/text/xml-entity.txt">
+ <!ENTITY ext SYSTEM "../xml-entity.txt">
]>
<document>
- <title>Example of an External Entity</title>
+ <title>External entity in the parent directory</title>
<content>
- &ext;
+ &ext;
</content>
</document>
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 158f5d83..12fb1345 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.
-->
-<!DOCTYPE document SYSTEM
"src/test/resources/org/apache/commons/text/document-external-dtd.dtd">
+<!DOCTYPE document SYSTEM "../document.dtd">
<document>
- <title>Example of an External Entity</title>
+ <title>External DTD subset in the parent directory</title>
<content>
&externalEntity;
</content>
diff --git a/src/test/resources/org/apache/commons/text/document-entity-ref.xml
b/src/test/resources/XmlStringLookup/documents/document-remote-entity.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-remote-entity.xml
index bfd36098..98bc5aa2 100644
--- a/src/test/resources/org/apache/commons/text/document-entity-ref.xml
+++ b/src/test/resources/XmlStringLookup/documents/document-remote-entity.xml
@@ -16,11 +16,11 @@
limitations under the License.
-->
<!DOCTYPE document [
- <!ENTITY ext SYSTEM
"src/test/resources/org/apache/commons/text/xml-entity.txt">
+ <!ENTITY ext SYSTEM "http://localhost:1/secret.txt";>
]>
<document>
- <title>Example of an External Entity</title>
+ <title>Remote external entity</title>
<content>
- &ext;
+ &ext;
</content>
</document>
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 dbc9ca74..00000000
--- a/src/test/resources/org/apache/commons/text/document-external-dtd.dtd
+++ /dev/null
@@ -1,20 +0,0 @@
-<!--
- 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.
--->
-<!ENTITY externalEntity "This is an external entity.">
-<!ELEMENT document (title, content)>
-<!ELEMENT title (#PCDATA)>
-<!ELEMENT content (#PCDATA)>
