+ * 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 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 3feb4fe5e0..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:
+ *
+ *
+ * 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,10 +1639,6 @@ public StringLookup xmlEncoderStringLookup() {
*
- * 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)}.
- *
- *
* Using a {@link StringLookup} from the {@link StringLookupFactory}:
*
*
@@ -1652,14 +1660,25 @@ 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;
}
/**
* 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:
+ *
+ *
+ * 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"}.
@@ -1671,10 +1690,6 @@ public StringLookup xmlStringLookup() {
*
- * 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)}.
- *
- *
* Using a {@link StringLookup} from the {@link StringLookupFactory}:
*
*
@@ -1705,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"}.
@@ -1718,9 +1744,6 @@ 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.
- *
- *
* 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 624c7d01e2..1fb3f0c65a 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 @@
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 @@
* {@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 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
@@ -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 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 and has no fence.
*
- * 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.
*
*/
- 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 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");
}
/**
@@ -110,7 +109,14 @@ 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.
+ * - 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.
@@ -128,14 +134,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();
+ 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 = 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 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..51f1e6d7f0
--- /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 e0fdf8abe7..67ab3cb804 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;
@@ -33,7 +36,6 @@
import org.junit.jupiter.api.Test;
import org.junitpioneer.jupiter.DefaultLocale;
-import org.junitpioneer.jupiter.SetSystemProperty;
/**
* Tests {@link StringLookupFactory}.
@@ -291,16 +293,56 @@ 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() {
- 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 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 862fdf812d..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,19 +23,21 @@
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 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 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 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);
@@ -62,62 +99,80 @@ 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() {
- 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/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 @@
-
-
-
-
-