ice: Fix localfileio file:/// & --force-no-copy handling

Author: shyiko

ice-rest-catalog/src/main/java/com/altinity/ice/rest/catalog/internal/config/Config.java

@@ -22,7 +22,6 @@
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
-import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
@@ -284,14 +283,8 @@ public void putNotNullOrEmpty(String key, String value) {
     }
 
     if (warehouse.startsWith("file://")) {
-      if (!m.containsKey(LocalFileIO.LOCALFILEIO_PROP_BASEDIR)) {
-        // FIXME: wrong thing to do if warehouse is absolute
-        m.put(LocalFileIO.LOCALFILEIO_PROP_BASEDIR, new File(".").getAbsolutePath());
-      }
-      var warehouseLocation =
-          Strings.removePrefix(m.get(CatalogProperties.WAREHOUSE_LOCATION), "file://");
-      File d = Paths.get(m.get(LocalFileIO.LOCALFILEIO_PROP_BASEDIR), warehouseLocation).toFile();
-      ;
+      String workdir = LocalFileIO.resolveWorkdir(warehouse, localFileIOBaseDir);
+      File d = LocalFileIO.resolveWarehousePath(warehouse, workdir).toFile();
       // TODO: move it out of here; toIcebergConfig() should not have side-effects
       if (!d.isDirectory() && !d.mkdirs()) {
         throw new IOException("Unable to create " + d);

ice/src/main/java/com/altinity/ice/cli/internal/config/Config.java

@@ -31,8 +31,11 @@ public record Config(
     @JsonPropertyDescription(
             "/path/to/dir where to store downloaded files when `ice insert`ing from http:// & https://")
         String httpCacheDir,
+    @JsonPropertyDescription("Path to warehouse, e.g. s3://..., file://...") String warehouse,
     @JsonPropertyDescription("/path/to/dir to serve as a root for warehouse=file://...")
         String localFileIOBaseDir,
+    @JsonPropertyDescription("Locations outside the warehouse that ice is allowed to access")
+        String[] localFileIOAllowAccess,
     @JsonPropertyDescription("Settings for warehouse=s3://...") S3 s3,
     @JsonPropertyDescription(
             "(experimental) Iceberg properties (see https://iceberg.apache.org/javadoc/1.8.1/"
@@ -102,7 +105,13 @@ public void putNotNullOrEmpty(String key, String value) {
       }
     }
 
+    m.putNotNullOrEmpty(LocalFileIO.LOCALFILEIO_PROP_WAREHOUSE, warehouse);
     m.putNotNullOrEmpty(LocalFileIO.LOCALFILEIO_PROP_BASEDIR, localFileIOBaseDir);
+    if (localFileIOAllowAccess != null) {
+      m.putNotNullOrEmpty(
+          LocalFileIO.LOCALFILEIO_PROP_ALLOWACCESS, String.join(",", localFileIOAllowAccess));
+    }
+
     m.putNotNullOrEmpty(OPTION_HTTP_CACHE, httpCacheDir);
 
     if (icebergProperties != null) {

ice/src/main/java/com/altinity/ice/internal/iceberg/io/LocalFileIO.java

@@ -16,11 +16,14 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import javax.annotation.Nullable;
 import org.apache.iceberg.CatalogProperties;
 import org.apache.iceberg.exceptions.RuntimeIOException;
 import org.apache.iceberg.io.BulkDeletionFailureException;
@@ -32,12 +35,15 @@
 
 public class LocalFileIO implements DelegateFileIO {
 
-  // current workdir by default
+  public static final String LOCALFILEIO_PROP_WAREHOUSE = "localfileio.warehouse";
+
   public static final String LOCALFILEIO_PROP_BASEDIR = "localfileio.basedir";
-  public static final String LOCALFILEIO_PROP_WAREHOUSE =
-      "localfileio.warehouse"; // e.g. file://path/to/warehouse/root/relative/to/localfileio.basedir
-  private Path basePath;
+  public static final String LOCALFILEIO_PROP_ALLOWACCESS = "localfileio.allowaccess";
+
+  private String workDir;
+  private Path warehousePath;
   private String locationPrefix;
+  private List<Path> allowAccess = List.of(); // TODO: replace with trie
 
   @Override
   public void initialize(Map<String, String> properties) {
@@ -51,40 +57,104 @@ public void initialize(Map<String, String> properties) {
         warehouse.startsWith("file://"),
         "\"%s\" must start with file://",
         LOCALFILEIO_PROP_WAREHOUSE);
-    String baseDir = properties.getOrDefault(LOCALFILEIO_PROP_BASEDIR, "");
-    if (baseDir.isEmpty()) {
-      baseDir = System.getProperty("user.dir");
-    }
-    if (!baseDir.isEmpty()) {
-      baseDir = Strings.removeSuffix(baseDir, "/") + "/";
-    }
-    String base = baseDir + Strings.removeSuffix(Strings.removePrefix(warehouse, "file://"), "/");
-    if (!Files.isDirectory(Paths.get(base))) {
+    this.workDir = resolveWorkdir(warehouse, properties.get(LOCALFILEIO_PROP_BASEDIR));
+    Path warehousePath = resolveWarehousePath(warehouse, workDir);
+    if (!Files.isDirectory(warehousePath)) {
       throw new IllegalArgumentException(
-          String.format("\"%s\" must point to an existing directory", LOCALFILEIO_PROP_WAREHOUSE));
+          String.format("\"%s\" must point to an existing directory", warehousePath));
     }
-    Path basePath;
     try {
-      basePath = Paths.get(base).toRealPath();
+      this.warehousePath = warehousePath.toRealPath();
     } catch (IOException e) {
       throw new RuntimeIOException(e);
     }
-    // TODO: do no allow dir that contain subfolders other than data/metadata (unless force flag is
-    // used)
+    var x = Strings.removeSuffix(Strings.removePrefix(warehouse, "file://"), "/");
+    this.locationPrefix = "file://" + (!x.isEmpty() ? x + "/" : "");
+    this.allowAccess =
+        Arrays.stream(properties.getOrDefault(LOCALFILEIO_PROP_ALLOWACCESS, "").split(","))
+            .map(s -> Strings.removePrefix(s.trim(), "file://"))
+            .filter(s -> !s.isEmpty())
+            .map(Paths::get)
+            .toList();
+  }
+
+  /**
+   * resolveWorkdir returns workdir based on whether warehouse is absolute or relative, e.g. <code>
+   * resolveWorkdir("/foo/bar", null) -> "/"
+   * resolveWorkdir("foo/bar", "/") -> "/"
+   * resolveWorkdir("foo/bar", "/baz") -> "/baz"
+   * resolveWorkdir("foo/bar", null) -> "$PWD"
+   * </code>
+   */
+  public static String resolveWorkdir(String fileWarehouse, @Nullable String warehouseBaseDir) {
+    String baseDir = Objects.requireNonNullElse(warehouseBaseDir, "");
+    if (baseDir.isEmpty()) {
+      baseDir =
+          Strings.removePrefix(fileWarehouse, "file://").startsWith("/")
+              ? "/"
+              : System.getProperty("user.dir") /* workdir */;
+    }
+    return baseDir;
+  }
+
+  /**
+   * resolveBasePath returns warehouse's absolute path, e.g. <code>
+   * resolveBasePath("/foo/bar", "/") -> "/foo/bar"
+   * resolveBasePath("foo/bar", "/") -> "/foo/bar"
+   * resolveBasePath("foo/bar", "/baz") -> "/baz/foo/bar"
+   * resolveBasePath("foo/bar", "$PWD") -> "$PWD/foo/bar"
+   * </code>
+   */
+  public static Path resolveWarehousePath(String fileWarehouse, String workDir) {
+    Path basePath =
+        Paths.get(
+                Strings.removeSuffix(workDir, "/")
+                    + "/"
+                    + Strings.removeSuffix(Strings.removePrefix(fileWarehouse, "file://"), "/"))
+            .toAbsolutePath();
+    // TODO: do no allow dir that contain subfolders other than data/metadata
+    //       (unless force flag is used)
     if ("/".equals(basePath.toString())) {
       throw new IllegalArgumentException(
           String.format("\"%s\" cannot point to /", LOCALFILEIO_PROP_WAREHOUSE));
     }
-    this.basePath = basePath;
-    var x = Strings.removeSuffix(Strings.removePrefix(warehouse, "file://"), "/");
-    this.locationPrefix = "file://" + (!x.isEmpty() ? x + "/" : "");
+    return basePath;
   }
 
+  // TODO: refactor (resolve + all resolve* above); this feels way more complicated that it needs to
+  // be
   private Path resolve(String userPath) {
     String relativeToBase = Strings.removePrefix(userPath, this.locationPrefix);
-    Path resolved = basePath.resolve(relativeToBase).normalize().toAbsolutePath();
-    if (!resolved.startsWith(basePath)) {
-      throw new SecurityException(String.format("Access outside \"%s\" is not allowed", resolved));
+    if (relativeToBase.startsWith("file://")) {
+      // userPath is not relative to locationPrefix (expected when --force-no-copy is used)
+      // check if it's explicitly whitelisted by the user
+      relativeToBase = Strings.removePrefix(relativeToBase, "file://");
+      if (allowAccess.isEmpty()) {
+        throw new SecurityException(
+            String.format(
+                "Access outside \"%s\" is not allowed: \"%s\" (add `localFileIOAllowAccess: [\"/dir\"]` to .ice.yaml to whitelist)",
+                warehousePath, relativeToBase));
+      }
+      Path resolved;
+      if (relativeToBase.startsWith("/")) {
+        resolved = Paths.get(relativeToBase).toAbsolutePath();
+      } else {
+        resolved = Paths.get(workDir).resolve(relativeToBase).toAbsolutePath();
+      }
+      if (allowAccess.stream().noneMatch(resolved::startsWith)) {
+        throw new SecurityException(
+            String.format(
+                "Access outside \"%s\" (plus \"%s\") is not allowed: \"%s\"",
+                warehousePath,
+                String.join("\", \"", allowAccess.stream().map(Path::toString).toList()),
+                relativeToBase));
+      }
+      return resolved;
+    }
+    Path resolved = warehousePath.resolve(relativeToBase).normalize().toAbsolutePath();
+    if (!resolved.startsWith(warehousePath)) {
+      throw new SecurityException(
+          String.format("Access outside \"%s\" is not allowed: \"%s\"", warehousePath, resolved));
     }
     return resolved;
   }
@@ -107,7 +177,7 @@ public void deleteFile(String path) {
   }
 
   private String location(Path path) {
-    return locationPrefix + basePath.relativize(path);
+    return locationPrefix + warehousePath.relativize(path);
   }
 
   @Override