Fixed: Improve file path validation to work with mounted directories

File.getCanonicalPath() resolves all symlinks in a path. OS-level mount points (Docker bind mounts, EFS) appear as symlinks. When a subdirectory like runtime/ is mounted, a file's canonical path resolves to the mount target (e.g. /mnt/efs/data/file.pdf), which no longer shares a prefix with the canonical path of ofbiz.home (/opt/ofbiz)

(cherry picked from commit b3eadf1)

Author: jacopoc

applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java

@@ -37,6 +37,7 @@
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 import java.util.Comparator;
 import java.util.HashMap;
@@ -473,12 +474,24 @@ public static String buildRequestPrefix(Delegator delegator, Locale locale, Stri
 
     /**
      * Checks that the given file is within the provided context root directory.
+     * Uses a dual-check strategy to support EFS/Docker mount points:
+     * 1. Canonical paths (resolves symlinks on both sides) — works for non-mounted paths.
+     * 2. Normalized absolute paths (collapses ".." without following symlinks) — fallback for
+     *    when contextRoot or a subdirectory inside it is a mount point, causing canonical paths
+     *    to diverge. Path traversal via ".." is still blocked by the normalization step.
      */
     static void checkContextFileBoundary(File file, String contextRoot) throws GeneralException {
         try {
             String canonicalAllowed = new File(contextRoot).getCanonicalPath();
             String canonicalFilePath = file.getCanonicalPath();
-            if (!canonicalFilePath.startsWith(canonicalAllowed + File.separator)) {
+            boolean passesCanonical = canonicalFilePath.startsWith(canonicalAllowed + File.separator)
+                    || canonicalFilePath.equals(canonicalAllowed);
+
+            Path normalizedAllowed = Path.of(contextRoot).toAbsolutePath().normalize();
+            Path normalizedFilePath = file.toPath().toAbsolutePath().normalize();
+            boolean passesNormalized = normalizedFilePath.startsWith(normalizedAllowed);
+
+            if (!passesCanonical && !passesNormalized) {
                 throw new GeneralException("Access to file denied: path resolves outside of the allowed directory");
             }
         } catch (IOException e) {

framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java

@@ -174,16 +174,19 @@ public static void checkLocalFileAllowList(File file) throws GeneralException {
     }
 
     /**
-     * Checks that the given file is within the OFBiz home directory and within one of the
-     * subdirectories listed in {@code content.data.ofbiz.file.allowed.paths} (security.properties).
+     * Checks that the given file is within one of the subdirectories listed in
+     * {@code content.data.ofbiz.file.allowed.paths} (security.properties), relative to the OFBiz
+     * home directory. The check uses canonical paths (resolving symlinks on both sides), so
+     * EFS/Docker volume mounts on allowed subdirectories are handled correctly. A preliminary
+     * "must be under ofbiz.home" canonical check is intentionally absent: when an allowed
+     * subdirectory (e.g. {@code runtime/}) is a mount point, the file's canonical path diverges
+     * from {@code canonicalHome}, but the per-allowed-path comparison below still passes because
+     * it resolves both sides through the mount. Path traversal via {@code ../} is still blocked.
      */
     public static void checkOfbizFileAllowList(File file) throws GeneralException {
         try {
             String canonicalHome = new File(System.getProperty("ofbiz.home")).getCanonicalPath();
             String canonicalFilePath = file.getCanonicalPath();
-            if (!canonicalFilePath.startsWith(canonicalHome + File.separator)) {
-                throw new GeneralException("Access to file denied: path resolves outside of the OFBiz home directory");
-            }
             String allowedPathsStr = UtilProperties.getPropertyValue("security",
                     "content.data.ofbiz.file.allowed.paths", "applications/,themes/,plugins/,runtime/");
             if (UtilValidate.isNotEmpty(allowedPathsStr)) {

framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java

@@ -20,13 +20,95 @@
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.Arrays;
+import java.util.Comparator;
 import java.util.List;
 
+import org.apache.ofbiz.base.util.GeneralException;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 
 public class SecurityUtilTest {
+
+    private Path tempHome;
+    private Path tempExternal;
+    private String previousOfbizHome;
+
+    @Before
+    public void setUpTempDirs() throws Exception {
+        tempHome = Files.createTempDirectory("ofbiz-home-test");
+        tempExternal = Files.createTempDirectory("ofbiz-ext-test");
+        previousOfbizHome = System.getProperty("ofbiz.home");
+        System.setProperty("ofbiz.home", tempHome.toString());
+    }
+
+    @After
+    public void tearDownTempDirs() throws Exception {
+        if (previousOfbizHome != null) {
+            System.setProperty("ofbiz.home", previousOfbizHome);
+        } else {
+            System.clearProperty("ofbiz.home");
+        }
+        deleteDirRecursively(tempHome);
+        deleteDirRecursively(tempExternal);
+    }
+
+    private static void deleteDirRecursively(Path dir) throws Exception {
+        if (dir != null && Files.exists(dir)) {
+            Files.walk(dir).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+        }
+    }
+
+    @Test
+    public void checkOfbizFileAllowListAcceptsFileInAllowedDir() throws Exception {
+        Path runtimeDir = Files.createDirectory(tempHome.resolve("runtime"));
+        Path file = Files.createTempFile(runtimeDir, "upload", ".dat");
+        SecurityUtil.checkOfbizFileAllowList(file.toFile());
+    }
+
+    @Test
+    public void checkOfbizFileAllowListRejectsFileOutsideAllowedDir() throws Exception {
+        Path forbiddenDir = Files.createDirectory(tempHome.resolve("forbidden"));
+        Path file = Files.createTempFile(forbiddenDir, "upload", ".dat");
+        try {
+            SecurityUtil.checkOfbizFileAllowList(file.toFile());
+            fail("Expected GeneralException for file outside allowed dirs");
+        } catch (GeneralException e) {
+            assertTrue(e.getMessage().contains("not within an allowed directory"));
+        }
+    }
+
+    @Test
+    public void checkOfbizFileAllowListAcceptsFileUnderSymlinkedAllowedDir() throws Exception {
+        // Simulates an EFS/Docker volume mount: runtime/ is a symlink to an external directory.
+        // Prior to the fix, the home-boundary canonical check incorrectly rejected this because
+        // the file's canonical path diverged from canonicalHome.
+        Path runtimeLink = tempHome.resolve("runtime");
+        Files.createSymbolicLink(runtimeLink, tempExternal);
+        Path file = Files.createTempFile(tempExternal, "upload", ".dat");
+        SecurityUtil.checkOfbizFileAllowList(file.toFile());
+    }
+
+    @Test
+    public void checkOfbizFileAllowListRejectsPathTraversal() throws Exception {
+        Path runtimeDir = Files.createDirectory(tempHome.resolve("runtime"));
+        // Construct a path that tries to escape via ".." — getCanonicalPath resolves this.
+        File traversalFile = new File(runtimeDir.toFile(), "../../etc/passwd");
+        try {
+            SecurityUtil.checkOfbizFileAllowList(traversalFile);
+            fail("Expected GeneralException for path traversal attempt");
+        } catch (GeneralException e) {
+            assertTrue(e.getMessage().contains("not within an allowed directory"));
+        }
+    }
+
+
     @Test
     public void basicAdminPermissionTesting() {
         List<String> adminPermissions = Arrays.asList("PARTYMGR", "EXAMPLE", "ACCTG_PREF");