Prevent I/O when checking UNC paths against --root-dir (#1466)

Test on [windows] please

pkl-core/src/main/java/org/pkl/core/SecurityManager.java

@@ -44,6 +44,24 @@ public interface SecurityManager {
    */
   void checkResolveResource(URI resource) throws SecurityManagerException;
 
+  /**
+   * Resolves the given {@code file:} URI to a secure, symlink-free path that has been verified to
+   * be within the root directory (if one is configured). The returned path can be opened with
+   * {@link java.nio.file.LinkOption#NOFOLLOW_LINKS}.
+   *
+   * <p>Returns {@code null} for non-{@code file:} URIs or if no root directory is configured.
+   *
+   * @param uri the URI to resolve
+   * @param isResource denotes if uri belongs to a resource (otherwise, a module)
+   * @return the resolved, symlink-free path under root directory, or {@code null}
+   * @throws SecurityManagerException if the resolved path is not within the root directory
+   * @throws IOException if the path cannot be resolved
+   */
+  default @Nullable Path resolveSecurePath(URI uri, boolean isResource)
+      throws SecurityManagerException, IOException {
+    return null;
+  }
+
   /**
    * Resolves the given {@code file:} URI to a secure, symlink-free path that has been verified to
    * be within the root directory (if one is configured). The returned path can be opened with
@@ -57,6 +75,6 @@ public interface SecurityManager {
    * @throws IOException if the path cannot be resolved
    */
   default @Nullable Path resolveSecurePath(URI uri) throws SecurityManagerException, IOException {
-    return null;
+    return resolveSecurePath(uri, false);
   }
 }

pkl-core/src/main/java/org/pkl/core/SecurityManagers.java

@@ -173,14 +173,20 @@ public final class SecurityManagers {
     }
 
     @Override
-    public @Nullable Path resolveSecurePath(URI uri) throws SecurityManagerException, IOException {
+    public @Nullable Path resolveSecurePath(URI uri, boolean isResource)
-      if (rootDir == null || !uri.isAbsolute() || !uri.getScheme().equals("file")) {
+        throws SecurityManagerException, IOException {
+      if (rootDir == null
+          || !uri.isAbsolute()
+          || !uri.getScheme().equals("file")
+          || (uri.getAuthority() != null && !uri.getAuthority().isEmpty())) {
         return null;
       }
       var path = Path.of(uri);
       var realPath = path.toRealPath();
       if (!realPath.startsWith(rootDir)) {
-        throw new SecurityManagerException(ErrorMessages.create("modulePastRootDir", uri, rootDir));
+        var errorMessageKey = isResource ? "resourcePastRootDir" : "modulePastRootDir";
+        var message = ErrorMessages.create(errorMessageKey, uri, rootDir);
+        throw new SecurityManagerException(message);
       }
       return realPath;
     }
@@ -225,6 +231,16 @@ public final class SecurityManagers {
       if (rootDir == null || !checkUri.getScheme().equals("file")) return;
 
       var path = Path.of(checkUri);
+
+      // uri represents a UNC path if authority is non-null
+      // so treat this like a potentially redirected HTTP read:
+      // check if both the given and real paths are under rootDir
+      if (checkUri.getAuthority() != null && !checkUri.getAuthority().isEmpty()) {
+        doCheckIsUnderRootDir(path.normalize(), uri, isResource);
+      }
+      // if given path is under rootDir, do I/O to determine if the real path is under the root dir
+      // this can result in a nasty timeout (~20s) in Files.exists if the server doesn't respond :(
+
       if (Files.exists(path)) {
         try {
           path = path.toRealPath();
@@ -237,6 +253,12 @@ public final class SecurityManagers {
         path = path.normalize();
       }
 
+      doCheckIsUnderRootDir(path, uri, isResource);
+    }
+
+    private void doCheckIsUnderRootDir(Path path, URI uri, boolean isResource)
+        throws SecurityManagerException {
+      assert rootDir != null;
       if (!path.startsWith(rootDir)) {
         var errorMessageKey = isResource ? "resourcePastRootDir" : "modulePastRootDir";
         var message = ErrorMessages.create(errorMessageKey, uri, rootDir);

pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java

@@ -262,7 +262,7 @@ public final class ResourceReaders {
       // Use resolveSecurePath to get a symlink-free path verified under rootDir.
       var securityManager = VmContext.get(null).getSecurityManager();
       try {
-        var securePath = securityManager.resolveSecurePath(uri);
+        var securePath = securityManager.resolveSecurePath(uri, true);
         if (securePath != null) {
           try (var in = Files.newInputStream(securePath, LinkOption.NOFOLLOW_LINKS)) {
             var content = in.readAllBytes();

pkl-core/src/test/kotlin/org/pkl/core/EvaluatorTest.kt

@@ -22,6 +22,7 @@ import java.nio.file.FileSystems
 import java.nio.file.Files
 import java.nio.file.Path
 import java.util.*
+import java.util.concurrent.TimeUnit
 import java.util.regex.Pattern
 import kotlin.io.path.createParentDirectories
 import kotlin.io.path.writeText
@@ -29,7 +30,10 @@ import org.assertj.core.api.Assertions.assertThat
 import org.assertj.core.api.Assertions.assertThatCode
 import org.junit.jupiter.api.AfterAll
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
 import org.junit.jupiter.api.assertThrows
+import org.junit.jupiter.api.condition.EnabledOnOs
+import org.junit.jupiter.api.condition.OS
 import org.junit.jupiter.api.io.TempDir
 import org.pkl.commons.createTempFile
 import org.pkl.commons.test.PackageServer
@@ -540,6 +544,28 @@ class EvaluatorTest {
     }
   }
 
+  @Test
+  @EnabledOnOs(OS.WINDOWS)
+  @Timeout(1, unit = TimeUnit.SECONDS)
+  fun `root dir check happens without any UNC or SMB access`() {
+    val evaluator =
+      with(EvaluatorBuilder.preconfigured()) {
+        rootDir = Path.of("/tmp/test")
+        build()
+      }
+    // this uses a TEST-NET-1 IP which has no server running in order to force a timeout-driven
+    // failure (takes ~20s)
+    // root dir check failure should prevent any I/O and fail fast instead of hitting the timeout
+    val exc =
+      assertThrows<PklException> {
+        evaluator.evaluate(text("result = import(\"file://192.0.2.1/share/nope.pkl\")"))
+      }
+    assertThat(exc)
+      .hasMessageContaining(
+        "Refusing to load module `file://192.0.2.1/share/nope.pkl` because it is not within the root directory (`--root-dir`)."
+      )
+  }
+
   private fun checkModule(module: PModule) {
     assertThat(module.properties.size).isEqualTo(2)
     assertThat(module.getProperty("name")).isEqualTo("pigeon")