Skip to content

Commit d2f8faf

Browse files
dsmileyCopilot
dsmiley
and
Copilot
authored
Refactor: move isFileForbiddenInConfigSets (#4193)
CLI configSet upload no longer blocks certain file types. The user of the CLI tool must already have permissions to touch ZooKeeper. Enforcement of such remains at Solr's ConfigSet HTTP endpoint. Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>
1 parent 9b0309d commit d2f8faf

9 files changed

Lines changed: 71 additions & 57 deletions

File tree

‎changelog/unreleased/move-configset-methods.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
title: CLI configSet upload no longer blocks certain file types. The user of the CLI tool must already have permissions to touch ZooKeeper. Enforcement of such remains at Solr's ConfigSet HTTP endpoint.
2+
type: other
3+
authors:
4+
- name: David Smiley
5+
links:
6+
- name: PR#4193
7+
url: https://github.com/apache/solr/pull/4193

‎solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,11 @@
1818

1919
import java.io.IOException;
2020
import java.lang.invoke.MethodHandles;
21+
import java.nio.file.FileVisitResult;
22+
import java.nio.file.Files;
2123
import java.nio.file.Path;
24+
import java.nio.file.SimpleFileVisitor;
25+
import java.nio.file.attribute.BasicFileAttributes;
2226
import java.util.ArrayList;
2327
import java.util.Collections;
2428
import java.util.List;
@@ -168,6 +172,22 @@ public void copyConfig(String fromConfig, String toConfig) throws IOException {
168172

169173
@Override
170174
public void uploadConfig(String configName, Path dir) throws IOException {
175+
Files.walkFileTree(
176+
dir,
177+
new SimpleFileVisitor<>() {
178+
@Override
179+
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
180+
String filename = file.getFileName().toString();
181+
if (ConfigSetService.isFileForbiddenInConfigSets(filename)) {
182+
throw new SolrException(
183+
SolrException.ErrorCode.BAD_REQUEST,
184+
"The file type provided for upload, '"
185+
+ filename
186+
+ "', is forbidden for use in uploading configsets.");
187+
}
188+
return FileVisitResult.CONTINUE;
189+
}
190+
});
171191
zkClient.uploadToZK(
172192
dir, CONFIGS_ZKNODE + "/" + configName, ConfigSetService.UPLOAD_FILENAME_EXCLUDE_PATTERN);
173193
}
@@ -178,7 +198,7 @@ public void uploadFileToConfig(
178198
throws IOException {
179199
String filePath = CONFIGS_ZKNODE + "/" + configName + "/" + fileName;
180200
try {
181-
if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(fileName)) {
201+
if (ConfigSetService.isFileForbiddenInConfigSets(fileName)) {
182202
throw new SolrException(
183203
SolrException.ErrorCode.BAD_REQUEST,
184204
"The file type provided for upload, '"
@@ -324,11 +344,11 @@ private void copyConfigDirFromZk(String fromZkPath, String toZkPath) throws IOEx
324344

325345
private void copyData(String fromZkFilePath, String toZkFilePath)
326346
throws KeeperException, InterruptedException {
327-
if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(fromZkFilePath)) {
347+
if (ConfigSetService.isFileForbiddenInConfigSets(fromZkFilePath)) {
328348
log.warn(
329349
"Skipping copy of file in ZK, as the source file is a forbidden type: {}",
330350
fromZkFilePath);
331-
} else if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(toZkFilePath)) {
351+
} else if (ConfigSetService.isFileForbiddenInConfigSets(toZkFilePath)) {
332352
log.warn(
333353
"Skipping download of file from ZK, as the target file is a forbidden type: {}",
334354
toZkFilePath);

‎solr/core/src/java/org/apache/solr/core/ConfigSetService.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,15 @@
2626
import java.util.List;
2727
import java.util.Locale;
2828
import java.util.Map;
29+
import java.util.Set;
2930
import java.util.regex.Pattern;
3031
import org.apache.solr.cloud.ZkConfigSetService;
3132
import org.apache.solr.cloud.ZkController;
3233
import org.apache.solr.common.ConfigNode;
3334
import org.apache.solr.common.SolrException;
3435
import org.apache.solr.common.util.EnvUtils;
3536
import org.apache.solr.common.util.NamedList;
37+
import org.apache.solr.common.util.StrUtils;
3638
import org.apache.solr.handler.admin.ConfigSetsHandler;
3739
import org.apache.solr.schema.IndexSchema;
3840
import org.apache.solr.schema.IndexSchemaFactory;
@@ -46,6 +48,32 @@ public abstract class ConfigSetService {
4648
public static final String UPLOAD_FILENAME_EXCLUDE_REGEX = "^\\..*$";
4749
public static final Pattern UPLOAD_FILENAME_EXCLUDE_PATTERN =
4850
Pattern.compile(UPLOAD_FILENAME_EXCLUDE_REGEX);
51+
52+
public static final String FORBIDDEN_FILE_TYPES_PROP = "solr.configset.forbidden.file.types";
53+
public static final Set<String> DEFAULT_FORBIDDEN_FILE_TYPES =
54+
Set.of("class", "java", "jar", "tgz", "zip", "tar", "gz");
55+
56+
private static final Set<String> USE_FORBIDDEN_FILE_TYPES;
57+
58+
static {
59+
String userForbiddenFileTypes = EnvUtils.getProperty(FORBIDDEN_FILE_TYPES_PROP);
60+
USE_FORBIDDEN_FILE_TYPES =
61+
StrUtils.isNullOrEmpty(userForbiddenFileTypes)
62+
? DEFAULT_FORBIDDEN_FILE_TYPES
63+
: Set.of(userForbiddenFileTypes.split(","));
64+
}
65+
66+
/**
67+
* Determine if a file path is forbidden for use in configsets based on its file extension.
68+
*
69+
* @param filePath the file path or name to check
70+
* @return true if the file extension is among the forbidden types
71+
*/
72+
public static boolean isFileForbiddenInConfigSets(String filePath) {
73+
int lastDot = filePath.lastIndexOf('.');
74+
return lastDot >= 0 && USE_FORBIDDEN_FILE_TYPES.contains(filePath.substring(lastDot + 1));
75+
}
76+
4977
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
5078

5179
public static ConfigSetService createConfigSetService(CoreContainer coreContainer) {

‎solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import java.util.stream.Collectors;
3636
import java.util.stream.Stream;
3737
import org.apache.solr.common.SolrException;
38-
import org.apache.solr.common.cloud.ZkMaintenanceUtils;
3938
import org.apache.solr.common.util.Utils;
4039
import org.apache.solr.util.FileTypeMagicUtil;
4140
import org.apache.solr.util.FileUtils;
@@ -155,7 +154,7 @@ public void uploadConfig(String configName, Path source) throws IOException {
155154
public void uploadFileToConfig(
156155
String configName, String fileName, byte[] data, boolean overwriteOnExists)
157156
throws IOException {
158-
if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(fileName)) {
157+
if (ConfigSetService.isFileForbiddenInConfigSets(fileName)) {
159158
throw new SolrException(
160159
SolrException.ErrorCode.BAD_REQUEST,
161160
"The file type provided for upload, '"
@@ -234,7 +233,7 @@ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs)
234233
@Override
235234
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
236235
throws IOException {
237-
if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(file.getFileName().toString())) {
236+
if (ConfigSetService.isFileForbiddenInConfigSets(file.getFileName().toString())) {
238237
log.warn(
239238
"Not including uploading file to config, as it is a forbidden type: {}",
240239
file.getFileName());

‎solr/core/src/java/org/apache/solr/core/backup/BackupManager.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import org.apache.solr.common.cloud.ClusterState;
3838
import org.apache.solr.common.cloud.DocCollection;
3939
import org.apache.solr.common.cloud.SolrZkClient;
40-
import org.apache.solr.common.cloud.ZkMaintenanceUtils;
4140
import org.apache.solr.common.cloud.ZkStateReader;
4241
import org.apache.solr.common.util.Utils;
4342
import org.apache.solr.core.ConfigSetService;
@@ -344,7 +343,7 @@ private void downloadConfigToRepo(ConfigSetService configSetService, String conf
344343
// checking for '/' is correct for a directory since ConfigSetService#getAllConfigFiles
345344
// always separates file paths with '/'
346345
if (!filePath.endsWith("/")) {
347-
if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(filePath)) {
346+
if (ConfigSetService.isFileForbiddenInConfigSets(filePath)) {
348347
log.warn(
349348
"Not including zookeeper file in backup, as it is a forbidden type: {}", filePath);
350349
} else {
@@ -385,7 +384,7 @@ private void uploadConfigToSolrCloud(
385384
switch (t) {
386385
case FILE:
387386
{
388-
if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(filePath)) {
387+
if (ConfigSetService.isFileForbiddenInConfigSets(filePath)) {
389388
log.warn(
390389
"Not including zookeeper file in restore, as it is a forbidden type: {}", file);
391390
} else {

‎solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import org.apache.solr.client.api.endpoint.ConfigsetsApi;
3131
import org.apache.solr.client.api.model.SolrJerseyResponse;
3232
import org.apache.solr.common.SolrException;
33-
import org.apache.solr.common.cloud.ZkMaintenanceUtils;
3433
import org.apache.solr.core.ConfigSetService;
3534
import org.apache.solr.core.CoreContainer;
3635
import org.apache.solr.jersey.PermissionName;
@@ -129,7 +128,7 @@ public SolrJerseyResponse uploadConfigSetFile(
129128
throw new SolrException(
130129
SolrException.ErrorCode.BAD_REQUEST,
131130
"The file path provided for upload, '" + singleFilePath + "', is not valid.");
132-
} else if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(fixedSingleFilePath)
131+
} else if (ConfigSetService.isFileForbiddenInConfigSets(fixedSingleFilePath)
133132
|| FileTypeMagicUtil.isFileForbiddenInConfigset(data)) {
134133
throw new SolrException(
135134
SolrException.ErrorCode.BAD_REQUEST,

‎solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@
8484
import org.apache.solr.common.SolrException;
8585
import org.apache.solr.common.SolrInputDocument;
8686
import org.apache.solr.common.cloud.SolrZkClient;
87-
import org.apache.solr.common.cloud.ZkMaintenanceUtils;
8887
import org.apache.solr.common.params.CollectionParams.CollectionAction;
8988
import org.apache.solr.common.params.ConfigSetParams;
9089
import org.apache.solr.common.params.ConfigSetParams.ConfigSetAction;
@@ -583,7 +582,7 @@ public void testOverwriteWithForbiddenFiles(boolean v2) throws Exception {
583582
assertEquals(
584583
400, uploadConfigSet(configsetName, configsetSuffix, null, true, false, v2, true, false));
585584

586-
for (String fileEnding : ZkMaintenanceUtils.DEFAULT_FORBIDDEN_FILE_TYPES) {
585+
for (String fileEnding : ConfigSetService.DEFAULT_FORBIDDEN_FILE_TYPES) {
587586
String f = configPath + "/test." + fileEnding;
588587
assertFalse(
589588
"Expecting file " + f + " to not exist, because it has a forbidden file type",
@@ -804,7 +803,7 @@ public void testSingleFileForbiddenType(boolean v2) throws Exception {
804803
.withTimeout(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
805804
.withConnTimeOut(45000, TimeUnit.MILLISECONDS)
806805
.build()) {
807-
for (String fileType : ZkMaintenanceUtils.DEFAULT_FORBIDDEN_FILE_TYPES) {
806+
for (String fileType : ConfigSetService.DEFAULT_FORBIDDEN_FILE_TYPES) {
808807
ignoreException("is forbidden for use in configSets");
809808
assertEquals(
810809
"Can't upload a configset file with a forbidden type: " + fileType,
@@ -1385,7 +1384,7 @@ private static void zipWithForbiddenEndings(Path fileOrDirectory, Path zipfile)
13851384
try (ZipOutputStream zout = new ZipOutputStream(out)) {
13861385
if (Files.isRegularFile(fileOrDirectory)) {
13871386
// Create entries with given file, one for each forbidden endding
1388-
for (String fileType : ZkMaintenanceUtils.DEFAULT_FORBIDDEN_FILE_TYPES) {
1387+
for (String fileType : ConfigSetService.DEFAULT_FORBIDDEN_FILE_TYPES) {
13891388
zout.putNextEntry(new ZipEntry("test." + fileType));
13901389

13911390
try (InputStream in = Files.newInputStream(fileOrDirectory)) {

‎solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkMaintenanceUtils.java‎

Lines changed: 3 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,9 @@
2929
import java.util.Comparator;
3030
import java.util.List;
3131
import java.util.Locale;
32-
import java.util.Set;
3332
import java.util.function.Predicate;
3433
import java.util.regex.Pattern;
3534
import org.apache.solr.client.solrj.SolrServerException;
36-
import org.apache.solr.common.SolrException;
37-
import org.apache.solr.common.util.EnvUtils;
3835
import org.apache.solr.common.util.StrUtils;
3936
import org.apache.zookeeper.CreateMode;
4037
import org.apache.zookeeper.KeeperException;
@@ -345,13 +342,6 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
345342
filenameExclusions);
346343
return FileVisitResult.CONTINUE;
347344
}
348-
if (isFileForbiddenInConfigSets(filename)) {
349-
throw new SolrException(
350-
SolrException.ErrorCode.BAD_REQUEST,
351-
"The file type provided for upload, '"
352-
+ filename
353-
+ "', is forbidden for use in uploading configsets.");
354-
}
355345
// TODO: Cannot check MAGIC header for file since FileTypeGuesser is in core
356346
String zkNode = createZkNodeName(zkPath, rootPath, file);
357347
try {
@@ -443,13 +433,9 @@ public static void downloadFromZK(SolrZkClient zkClient, String zkPath, Path fil
443433
if (children.size() == 0) {
444434
// If we didn't copy data down, then we also didn't create the file. But we still need a
445435
// marker on the local disk so create an empty file.
446-
if (isFileForbiddenInConfigSets(zkPath)) {
447-
log.warn("Skipping download of file from ZK, as it is a forbidden type: {}", zkPath);
448-
} else {
449-
// TODO: Cannot check MAGIC header for file since FileTypeGuesser is in core
450-
if (copyDataDown(zkClient, zkPath, file) == 0) {
451-
Files.createFile(file);
452-
}
436+
// TODO: Cannot check MAGIC header for file since FileTypeGuesser is in core
437+
if (copyDataDown(zkClient, zkPath, file) == 0) {
438+
Files.createFile(file);
453439
}
454440
} else {
455441
Files.createDirectories(file); // Make parent dir.
@@ -581,29 +567,6 @@ public static String createZkNodeName(String zkRoot, Path root, Path file) {
581567
return ret;
582568
}
583569

584-
public static final String FORBIDDEN_FILE_TYPES_PROP = "solr.configset.forbidden.file.types";
585-
public static final Set<String> DEFAULT_FORBIDDEN_FILE_TYPES =
586-
Set.of("class", "java", "jar", "tgz", "zip", "tar", "gz");
587-
private static volatile Set<String> USE_FORBIDDEN_FILE_TYPES = null;
588-
589-
public static boolean isFileForbiddenInConfigSets(String filePath) {
590-
// Try to set the forbidden file types just once, since it is set by SysProp/EnvVar
591-
if (USE_FORBIDDEN_FILE_TYPES == null) {
592-
synchronized (DEFAULT_FORBIDDEN_FILE_TYPES) {
593-
if (USE_FORBIDDEN_FILE_TYPES == null) {
594-
String userForbiddenFileTypes = EnvUtils.getProperty(FORBIDDEN_FILE_TYPES_PROP);
595-
if (StrUtils.isNullOrEmpty(userForbiddenFileTypes)) {
596-
USE_FORBIDDEN_FILE_TYPES = DEFAULT_FORBIDDEN_FILE_TYPES;
597-
} else {
598-
USE_FORBIDDEN_FILE_TYPES = Set.of(userForbiddenFileTypes.split(","));
599-
}
600-
}
601-
}
602-
}
603-
int lastDot = filePath.lastIndexOf('.');
604-
return lastDot >= 0 && USE_FORBIDDEN_FILE_TYPES.contains(filePath.substring(lastDot + 1));
605-
}
606-
607570
/**
608571
* Create a persistent znode with no data if it does not already exist
609572
*

‎solr/solrj-zookeeper/src/test/org/apache/solr/common/cloud/TestZkConfigSetService.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,15 +131,15 @@ public void testUploadConfig() throws IOException {
131131
Files.write(tempConfig.resolve("file1"), overwritten);
132132
configSetService.uploadConfig("testconfig", tempConfig);
133133

134-
assertEquals(2, configSetService.listConfigs().size());
134+
assertEquals(1, configSetService.listConfigs().size());
135135
Path download2 = createTempDir("download2");
136136
configSetService.downloadConfig("testconfig", download2);
137137
byte[] checkdata2 = Files.readAllBytes(download2.resolve("file1"));
138138
assertArrayEquals(overwritten, checkdata2);
139139

140140
// uploading same files to a new name creates a new config
141141
configSetService.uploadConfig("config2", tempConfig);
142-
assertEquals(3, configSetService.listConfigs().size());
142+
assertEquals(2, configSetService.listConfigs().size());
143143

144144
// Test copying a config works in both flavors
145145
configSetService.copyConfig("config2", "config2copy");

0 commit comments

Comments
 (0)