Merge pull request #6357 from seadowg/symlink-support

Support symlink `dirPath` in `getAbsoluteFilePath`
getodk · Aug 23, 2024 · 951f44e · 951f44e
1 parent db209a7
commit 951f44e
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 4 deletions.
diff --git a/shared/src/main/java/org/odk/collect/shared/PathUtils.kt b/shared/src/main/java/org/odk/collect/shared/PathUtils.kt
@@ -11,12 +11,13 @@ object PathUtils {
 
     @JvmStatic
     fun getAbsoluteFilePath(dirPath: String, filePath: String): String {
-        val absolutePath = if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath
+        val absolutePath =
+            if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath
 
-        if (File(absolutePath).canonicalPath.startsWith(dirPath)) {
+        if (File(absolutePath).canonicalPath.startsWith(File(dirPath).canonicalPath)) {
             return absolutePath
         } else {
-            throw SecurityException("Invalid path: $absolutePath")
+            throw SecurityException("Contact support@getodk.org. Attempt to access file outside of Collect directory: $absolutePath")
         }
     }
 

diff --git a/shared/src/main/java/org/odk/collect/shared/TempFiles.kt b/shared/src/main/java/org/odk/collect/shared/TempFiles.kt
@@ -84,7 +84,7 @@ object TempFiles {
     }
 
     private fun getTempDir(): File {
-        val tmpDir = File(System.getProperty("java.io.tmpdir", "."), " org.odk.collect.shared.TempFiles")
+        val tmpDir = File(System.getProperty("java.io.tmpdir", "."), "org.odk.collect.shared.TempFiles")
         if (!tmpDir.exists()) {
             tmpDir.mkdir()
         }

diff --git a/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt b/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt
@@ -3,6 +3,7 @@ package org.odk.collect.shared
 import org.hamcrest.MatcherAssert.assertThat
 import org.hamcrest.Matchers.equalTo
 import org.junit.Test
+import java.io.File
 
 class PathUtilsTest {
 
@@ -29,6 +30,17 @@ class PathUtilsTest {
         PathUtils.getAbsoluteFilePath("/root/dir", "../tmp/file")
     }
 
+    @Test
+    fun `getAbsoluteFilePath() works when dirPath is not canonical`() {
+        val tempDir = TempFiles.createTempDir()
+        val nonCanonicalPath =
+            tempDir.canonicalPath + File.separator + ".." + File.separator + tempDir.name
+        assertThat(File(nonCanonicalPath).canonicalPath, equalTo(tempDir.canonicalPath))
+
+        val path = PathUtils.getAbsoluteFilePath(nonCanonicalPath, "file")
+        assertThat(path, equalTo(nonCanonicalPath + File.separator + "file"))
+    }
+
     @Test
     fun `getRelativeFilePath() returns filePath with dirPath removed`() {
         val path = PathUtils.getRelativeFilePath("/root/dir", "/root/dir/file")