[GitHub] [solr] dsmiley commented on a change in pull request #593: SOLR-15950 Create SOLR_HOME/{filestore,userfiles} lazily on first use

[GitBox <gi...@apache.org>]

dsmiley commented on a change in pull request #593:
URL: https://github.com/apache/solr/pull/593#discussion_r798934722



##########
File path: solr/core/src/java/org/apache/solr/handler/CatStream.java
##########
@@ -98,7 +98,11 @@ public void setStreamContext(StreamContext context) {
 
     this.chroot = core.getCoreContainer().getUserFilesPath();
     if (! Files.exists(chroot)) {
-      throw new IllegalStateException(chroot + " directory used to load files must exist but could not be found!");
+      try {

Review comment:
       Please no; see JIRA.

##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -94,8 +94,18 @@ private static Path _getRealPath(String path, Path solrHome) {
     if (!path.isEmpty() && path.charAt(0) != File.separatorChar) {
       path = File.separator + path;
     }
-    // Use concat because path might start with a slash and be incorrectly interpreted as absolute
-    return solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY + path);
+    if (path.startsWith("\\\\")) { // Windows absolute UNC
+      throw new SolrException(BAD_REQUEST, "Illegal path " + path);
+    }
+    while (path.startsWith("/")) { // Trim all leading slashes
+      path = path.substring(1);
+    }
+    var finalPath = getPackageStoreDirPath(solrHome).resolve(path);
+    // Guard against path traversal by asserting final path is sub path of filestore
+    if (finalPath.normalize().startsWith(getPackageStoreDirPath(solrHome).normalize())) {

Review comment:
       Is normalization necessary here?

##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -572,7 +582,16 @@ private void ensurePackageStoreDir(Path solrHome) {
   }
 
   public static Path getPackageStoreDirPath(Path solrHome) {
-    return solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
+    var path = solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
+    if (!Files.exists(path)) {
+      try {
+        Files.createDirectories(path);
+        log.info("Created filestore folder {}", path);
+      } catch (IOException e) {
+        throw new SolrException(SERVER_ERROR, "Faild creating 'filestore' folder in SOLR_HOME. Check permissions");

Review comment:
       There might be a permissions issue; this isn't a problem per-se... can't we just return null and ultimately just disable the package store?  Log something too... I'd just do INFO level.




-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org