You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ds...@apache.org on 2026/09/25 14:07:40 UTC
(solr) 05/08: SOLR-18226: Validate replication file names resolve within core config/tlog directory
This is an automated email from the ASF dual-hosted git repository.
dsmiley pushed a commit to branch branch_9_11
in repository https://gitbox.apache.org/repos/asf/solr.git
commit cc701d6e2f85e6437cc6b427a819d5d1c64c4ef9
Author: Mark Robert Miller <ma...@gmail.com>
AuthorDate: Wed Sep 23 01:36:19 2026 -0400
SOLR-18226: Validate replication file names resolve within core config/tlog directory
Replication file fetching now treats both "/" and "\" as path separators
when validating a file name, and requires config/tlog file names to
resolve within the core's config or tlog directory.
Co-Authored-By: David Smiley <ds...@apache.org>
Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
(cherry picked from commit e044bf20b405b8ca985730be3553d77d1fc8ff31)
(cherry picked from commit 40e0e8d9e00b0553f5cc64bfbd8d4ed8526f1d27)
(cherry picked from commit 3ed9b5dbb7e203aa708012acc95c3d41bb873758)
---
...SOLR-18226-replication-file-name-validation.yml | 12 ++
.../solr/handler/admin/api/ReplicationAPIBase.java | 47 +++--
.../src/java/org/apache/solr/util/FileUtils.java | 24 +++
.../handler/admin/api/CoreReplicationAPITest.java | 205 ++++++++++++++++++++-
4 files changed, 274 insertions(+), 14 deletions(-)
diff --git a/changelog/unreleased/SOLR-18226-replication-file-name-validation.yml b/changelog/unreleased/SOLR-18226-replication-file-name-validation.yml
new file mode 100644
index 00000000000..9024d0fab68
--- /dev/null
+++ b/changelog/unreleased/SOLR-18226-replication-file-name-validation.yml
@@ -0,0 +1,12 @@
+title: >-
+ Replication file fetching treats both "/" and "\" as path separators when
+ validating a file name, and config/tlog file names must resolve within the
+ core's config or tlog directory.
+type: changed
+authors:
+ - name: Mark Robert Miller
+links:
+ - name: SOLR-18226
+ url: https://issues.apache.org/jira/browse/SOLR-18226
+important_notes:
+ - "A `filecontent` request to `/solr/{core}/replication` with `dirType=cf` or `dirType=tlogFile` must name a file that resolves inside the core's config or tlog directory; otherwise it is rejected."
diff --git a/solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java b/solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java
index 69aab484ec0..eedd80fa051 100644
--- a/solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java
+++ b/solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java
@@ -26,8 +26,8 @@ import java.lang.invoke.MethodHandles;
import java.nio.ByteBuffer;
import java.nio.channels.SeekableByteChannel;
import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
import java.nio.file.Path;
-import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
@@ -58,6 +58,7 @@ import org.apache.solr.core.SolrCore;
import org.apache.solr.handler.ReplicationHandler;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.FileUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -290,13 +291,10 @@ public abstract class ReplicationAPIBase extends JerseyResource {
switch (dirType) {
case CONF_FILE_SHORT:
- cfileName = file;
+ cfileName = fileName;
break;
case TLOG_FILE:
- tlogFileName = file;
- break;
- default:
- fileName = file;
+ tlogFileName = fileName;
break;
}
@@ -319,7 +317,15 @@ public abstract class ReplicationAPIBase extends JerseyResource {
// Throw exception on directory traversal attempts
protected String validateFilenameOrError(String fileName) {
if (fileName != null) {
- Path filePath = Paths.get(fileName);
+ // Treat both '/' and '\' as separators so that ".." components are detected on
+ // every platform. The original fileName is returned; initFile() enforces that it
+ // resolves within the intended directory.
+ Path filePath;
+ try {
+ filePath = Path.of(FileUtils.normalizeToOsPathSeparator(fileName));
+ } catch (InvalidPathException e) {
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Invalid file name");
+ }
filePath.forEach(
subpath -> {
if ("..".equals(subpath.toString())) {
@@ -556,8 +562,10 @@ public abstract class ReplicationAPIBase extends JerseyResource {
@Override
protected Path initFile() {
- // if it is a tlog file read from tlog directory
- return Path.of(solrCore.getUpdateHandler().getUpdateLog().getTlogDir(), tlogFileName);
+ return resolveWithinOrForbidden(
+ Path.of(solrCore.getUpdateHandler().getUpdateLog().getTlogDir()),
+ tlogFileName,
+ "tlog directory");
}
}
@@ -577,9 +585,26 @@ public abstract class ReplicationAPIBase extends JerseyResource {
@Override
protected Path initFile() {
- // if it is a conf file read from config directory
- return solrCore.getResourceLoader().getConfigPath().resolve(cfileName);
+ return resolveWithinOrForbidden(
+ solrCore.getResourceLoader().getConfigPath(), cfileName, "core config directory");
+ }
+ }
+
+ /**
+ * Resolves {@code child} against {@code parent}, throwing a FORBIDDEN {@link SolrException} if
+ * the result lies outside {@code parent}.
+ */
+ private static Path resolveWithinOrForbidden(
+ Path parent, String child, String parentDescription) {
+ if (child == null) {
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "File name required");
+ }
+ Path resolved = parent.resolve(child);
+ if (!FileUtils.isPathAChildOfParent(parent, resolved)) {
+ throw new SolrException(
+ SolrException.ErrorCode.FORBIDDEN, "File path must be within the " + parentDescription);
}
+ return resolved.normalize();
}
private void reportErrorOnResponse(
diff --git a/solr/core/src/java/org/apache/solr/util/FileUtils.java b/solr/core/src/java/org/apache/solr/util/FileUtils.java
index b027d1e4d77..4b7d3094f43 100644
--- a/solr/core/src/java/org/apache/solr/util/FileUtils.java
+++ b/solr/core/src/java/org/apache/solr/util/FileUtils.java
@@ -23,6 +23,7 @@ import java.io.FileOutputStream;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.nio.channels.FileChannel;
+import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import org.apache.commons.io.FileExistsException;
@@ -127,4 +128,27 @@ public class FileUtils {
return normalizedChild.startsWith(normalizedParent)
&& !normalizedChild.equals(normalizedParent);
}
+
+ /**
+ * Rewrites {@code path} so every path separator matches the current filesystem's separator.
+ *
+ * <p>Both {@code /} and {@code \} are replaced with {@link FileSystems#getDefault()}'s separator.
+ * This is useful before iterating {@link Path} components, since on POSIX a {@code \} is
+ * otherwise part of a name rather than a separator, so {@code \..\conf\x} would be a single
+ * component.
+ *
+ * <p>Caveat: on non-Windows platforms this reinterprets any filename that legitimately contains a
+ * backslash as a path with additional components. Callers that treat backslash as a valid
+ * filename character on POSIX must not use this method.
+ *
+ * @param path the path to normalize
+ * @return path normalized with the filesystem's default separator, or {@code null} if {@code
+ * path} was {@code null}
+ */
+ public static String normalizeToOsPathSeparator(String path) {
+ if (path == null) return null;
+ path = path.replace("/", FileSystems.getDefault().getSeparator());
+ path = path.replace("\\", FileSystems.getDefault().getSeparator());
+ return path;
+ }
}
diff --git a/solr/core/src/test/org/apache/solr/handler/admin/api/CoreReplicationAPITest.java b/solr/core/src/test/org/apache/solr/handler/admin/api/CoreReplicationAPITest.java
index be05cedbbc1..682784c6d8b 100644
--- a/solr/core/src/test/org/apache/solr/handler/admin/api/CoreReplicationAPITest.java
+++ b/solr/core/src/test/org/apache/solr/handler/admin/api/CoreReplicationAPITest.java
@@ -22,14 +22,17 @@ import static org.mockito.Mockito.when;
import io.opentracing.noop.NoopSpan;
import java.io.IOException;
+import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import org.apache.lucene.util.Constants;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.api.model.FileListResponse;
import org.apache.solr.client.api.model.FileMetaData;
import org.apache.solr.client.api.model.IndexVersionResponse;
+import org.apache.solr.common.SolrException;
import org.apache.solr.core.SolrCore;
import org.apache.solr.core.SolrResourceLoader;
import org.apache.solr.handler.ReplicationHandler;
@@ -37,6 +40,7 @@ import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.response.SolrQueryResponse;
import org.apache.solr.update.UpdateHandler;
import org.apache.solr.update.UpdateLog;
+import org.junit.Assume;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
@@ -98,6 +102,183 @@ public class CoreReplicationAPITest extends SolrTestCaseJ4 {
assertTrue(actual instanceof ReplicationAPIBase.LocalFsConfFileStream);
}
+ @Test
+ public void testValidateFilenameRejectsBackslashTraversal() {
+ // Backslash is a separator for validation on every platform, not just Windows.
+ for (String dirType : new String[] {"cf", "tlogFile"}) {
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ "Expected FORBIDDEN for dirType=" + dirType,
+ () ->
+ coreReplicationAPI.doFetchFile(
+ "\\..\\conf\\solrconfig.xml", dirType, null, null, false, false, 0, null));
+ assertEquals(
+ "Wrong error code for dirType=" + dirType,
+ SolrException.ErrorCode.FORBIDDEN.code,
+ ex.code());
+ }
+ }
+
+ @Test
+ public void testValidateFilenameRejectsTraversalVariants() {
+ // Mixed separators, across both conf-file and tlog-file dirTypes.
+ String[] inputs = {
+ "\\..\\conf\\solrconfig.xml",
+ "/..\\conf\\solrconfig.xml",
+ "\\../conf/solrconfig.xml",
+ "foo\\..\\..\\other\\file.txt",
+ "..\\other\\file.txt",
+ };
+ for (String dirType : new String[] {"cf", "tlogFile"}) {
+ for (String input : inputs) {
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ "Expected FORBIDDEN for dirType=" + dirType + " input=" + input,
+ () ->
+ coreReplicationAPI.doFetchFile(
+ input, dirType, null, null, false, false, 0, null));
+ assertEquals(
+ "Wrong error code for dirType=" + dirType + " input=" + input,
+ SolrException.ErrorCode.FORBIDDEN.code,
+ ex.code());
+ }
+ }
+ }
+
+ @Test
+ public void testValidateFilenameRejectsAbsolutePath() {
+ // On POSIX, '/abs/file.txt' is absolute; validateFilenameOrError must reject it
+ // via its isAbsolute() branch. Windows treats a leading '/' as drive-relative
+ // (not absolute), so on Windows the containment check in initFile carries the
+ // load instead; this assertion targets the POSIX code path explicitly.
+ Assume.assumeFalse(Constants.WINDOWS);
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ () ->
+ coreReplicationAPI.doFetchFile(
+ "/abs/file.txt", "cf", null, null, false, false, 0, null));
+ assertEquals(SolrException.ErrorCode.FORBIDDEN.code, ex.code());
+ }
+
+ @Test
+ public void testValidateFilenameRejectsWindowsAbsolutePath() {
+ // Symmetric to testValidateFilenameRejectsAbsolutePath: on Windows,
+ // 'C:\Windows\...' IS absolute and must be rejected by
+ // validateFilenameOrError's isAbsolute() branch. POSIX treats 'C:\...' as a
+ // relative path with a literal ':' character, so Path.of won't mark it
+ // absolute there; this assertion is therefore gated to Windows only.
+ Assume.assumeTrue(Constants.WINDOWS);
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ () ->
+ coreReplicationAPI.doFetchFile(
+ "C:\\Windows\\System32\\drivers\\etc\\hosts",
+ "cf",
+ null,
+ null,
+ false,
+ false,
+ 0,
+ null));
+ assertEquals(SolrException.ErrorCode.FORBIDDEN.code, ex.code());
+ }
+
+ @Test
+ public void testValidateFilenameRejectsInvalidPath() {
+ // A NUL character is rejected by Path.of on all platforms; ensure the
+ // InvalidPathException is translated into a BAD_REQUEST SolrException
+ // (malformed input, not access-denied) rather than bubbling as an
+ // unhandled runtime error.
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ () ->
+ coreReplicationAPI.doFetchFile(
+ "foo\u0000bar", "cf", null, null, false, false, 0, null));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code());
+ }
+
+ @Test
+ public void testValidNestedPathAccepted() throws IOException {
+ // Positive test: legal nested relative paths (e.g. 'lang/en.txt') must not
+ // be rejected by validateFilenameOrError OR by the isPathAChildOfParent
+ // containment check in LocalFs{Conf,Tlog}FileStream.initFile(). Guards
+ // against those checks over-rejecting.
+ // Also exercises the normalize() applied to the resolved path by
+ // resolveWithinOrForbidden; the on-disk file must still be readable.
+ Path configPath = mockCore.getResourceLoader().getConfigPath();
+ Files.createDirectories(configPath.resolve("lang"));
+ Path cfFile = configPath.resolve("lang").resolve("en.txt");
+ Files.writeString(cfFile, "hello");
+ Path tlogDir = Path.of(mockCore.getUpdateHandler().getUpdateLog().getTlogDir());
+ Files.createDirectories(tlogDir.resolve("nested"));
+ Path tlogFile = tlogDir.resolve("nested").resolve("tlog.0");
+ Files.writeString(tlogFile, "hello");
+
+ ReplicationAPIBase.DirectoryFileStream cfStream =
+ coreReplicationAPI.doFetchFile("lang/en.txt", "cf", null, null, false, false, 0, null);
+ assertTrue(cfStream instanceof ReplicationAPIBase.LocalFsConfFileStream);
+ assertTrue("Expected readable config file at " + cfFile, Files.isReadable(cfFile));
+ assertEquals(cfFile.normalize(), configPath.resolve("lang/en.txt").normalize());
+
+ ReplicationAPIBase.DirectoryFileStream tlogStream =
+ coreReplicationAPI.doFetchFile(
+ "nested/tlog.0", "tlogFile", null, null, false, false, 0, null);
+ assertTrue(tlogStream instanceof ReplicationAPIBase.LocalFsTlogFileStream);
+ assertTrue("Expected readable tlog file at " + tlogFile, Files.isReadable(tlogFile));
+ assertEquals(tlogFile.normalize(), tlogDir.resolve("nested/tlog.0").normalize());
+ }
+
+ @Test
+ public void testDoFetchFileRejectsNullFilename() {
+ // A null file argument for the LocalFs{Conf,Tlog}FileStream paths
+ // must surface as a clean 4xx SolrException rather than an NPE bubbling out
+ // of Path.resolve(null). Guards the null-child branch of
+ // resolveWithinOrForbidden for both dirType=cf and dirType=tlogFile.
+ for (String dirType : new String[] {"cf", "tlogFile"}) {
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ "Expected 4xx for dirType=" + dirType,
+ () ->
+ coreReplicationAPI.doFetchFile(null, dirType, null, null, false, false, 0, null));
+ assertTrue(
+ "Expected a 4xx error code for dirType=" + dirType + ", got " + ex.code(),
+ ex.code() >= 400 && ex.code() < 500);
+ }
+ }
+
+ @Test
+ public void testInitFileGuardCatchesPathEscapingConfigDir() {
+ // Directly exercise LocalFsConfFileStream.initFile()'s isPathAChildOfParent guard,
+ // independent of validateFilenameOrError.
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ () ->
+ ((CoreReplicationAPIMock) coreReplicationAPI)
+ .newUnvalidatedConfFileStream("../../other/file.txt"));
+ assertEquals(SolrException.ErrorCode.FORBIDDEN.code, ex.code());
+ }
+
+ @Test
+ public void testInitFileGuardCatchesPathEscapingTlogDir() {
+ // Symmetric to testInitFileGuardCatchesPathEscapingConfigDir: ensures the
+ // LocalFsTlogFileStream.initFile() containment check rejects paths that
+ // resolve outside the tlog directory, independent of validateFilenameOrError.
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ () ->
+ ((CoreReplicationAPIMock) coreReplicationAPI)
+ .newUnvalidatedTlogFileStream("../../other/file.txt"));
+ assertEquals(SolrException.ErrorCode.FORBIDDEN.code, ex.code());
+ }
+
private void setUpMocks() throws IOException {
mockCore = mock(SolrCore.class);
mockReplicationHandler = mock(ReplicationHandler.class);
@@ -106,15 +287,15 @@ public class CoreReplicationAPITest extends SolrTestCaseJ4 {
UpdateHandler mockUpdateHandler = mock(UpdateHandler.class);
UpdateLog mockUpdateLog = mock(UpdateLog.class);
when(mockUpdateHandler.getUpdateLog()).thenReturn(mockUpdateLog);
- when(mockUpdateLog.getTlogDir()).thenReturn("ignore");
+ when(mockUpdateLog.getTlogDir()).thenReturn(createTempDir("coreReplicationTlog").toString());
// Mocks for LocalFsConfFileStream
SolrResourceLoader mockSolrResourceLoader = mock(SolrResourceLoader.class);
- Path mockPath = mock(Path.class);
+ Path configPath = createTempDir("coreReplicationConf");
when(mockCore.getRequestHandler(ReplicationHandler.PATH)).thenReturn(mockReplicationHandler);
when(mockCore.getUpdateHandler()).thenReturn(mockUpdateHandler);
when(mockCore.getResourceLoader()).thenReturn(mockSolrResourceLoader);
- when(mockSolrResourceLoader.getConfigPath()).thenReturn(mockPath);
+ when(mockSolrResourceLoader.getConfigPath()).thenReturn(configPath);
}
private static class CoreReplicationAPIMock extends CoreReplication {
@@ -129,5 +310,23 @@ public class CoreReplicationAPITest extends SolrTestCaseJ4 {
filesResponse.fileList = new ArrayList<>(fileMetaData);
return filesResponse;
}
+
+ ReplicationAPIBase.LocalFsConfFileStream newUnvalidatedConfFileStream(String cfname) {
+ return new LocalFsConfFileStream(cfname, "cf", null, null, false, false, 0, null) {
+ @Override
+ protected String validateFilenameOrError(String fileName) {
+ return fileName;
+ }
+ };
+ }
+
+ ReplicationAPIBase.LocalFsTlogFileStream newUnvalidatedTlogFileStream(String tlogname) {
+ return new LocalFsTlogFileStream(tlogname, "tlogFile", null, null, false, false, 0, null) {
+ @Override
+ protected String validateFilenameOrError(String fileName) {
+ return fileName;
+ }
+ };
+ }
}
}