dsmiley commented on code in PR #4193:
URL: https://github.com/apache/solr/pull/4193#discussion_r2898798253
##########
solr/core/src/java/org/apache/solr/core/ConfigSetService.java:
##########
@@ -46,6 +48,32 @@ public abstract class ConfigSetService {
public static final String UPLOAD_FILENAME_EXCLUDE_REGEX = "^\\..*$";
public static final Pattern UPLOAD_FILENAME_EXCLUDE_PATTERN =
Pattern.compile(UPLOAD_FILENAME_EXCLUDE_REGEX);
+
+ public static final String FORBIDDEN_FILE_TYPES_PROP =
"solr.configset.forbidden.file.types";
+ public static final Set<String> DEFAULT_FORBIDDEN_FILE_TYPES =
+ Set.of("class", "java", "jar", "tgz", "zip", "tar", "gz");
+
+ private static final Set<String> USE_FORBIDDEN_FILE_TYPES;
+
+ static {
+ String userForbiddenFileTypes =
EnvUtils.getProperty(FORBIDDEN_FILE_TYPES_PROP);
+ USE_FORBIDDEN_FILE_TYPES =
+ StrUtils.isNullOrEmpty(userForbiddenFileTypes)
+ ? DEFAULT_FORBIDDEN_FILE_TYPES
+ : Set.of(userForbiddenFileTypes.split(","));
+ }
Review Comment:
this is now much simpler -- no lazy initialization with double-checked
locking. Wasn't necessary!
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -155,7 +154,7 @@ public void uploadConfig(String configName, Path source)
throws IOException {
public void uploadFileToConfig(
String configName, String fileName, byte[] data, boolean
overwriteOnExists)
throws IOException {
- if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(fileName)) {
Review Comment:
this line (and really the very definition of that method in a ZK specific
class) irritated the hell out of me, prompting me to do this PR
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkMaintenanceUtils.java:
##########
@@ -443,13 +433,9 @@ public static void downloadFromZK(SolrZkClient zkClient,
String zkPath, Path fil
if (children.size() == 0) {
// If we didn't copy data down, then we also didn't create the file.
But we still need a
// marker on the local disk so create an empty file.
- if (isFileForbiddenInConfigSets(zkPath)) {
Review Comment:
I don't see why this was here. This method doesn't even say "configSet" in
it, but lets pretend it did. So nonetheless, what risk does this stop? I
think it's sufficient to prevent ConfigSetService from adding "forbidden" files
to its storage (usually ZK). If such a file got there already, then it's
either by design (sys-admin knows what they are doing) or if bad actors can get
stuff into ZK then you've already lost.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
