From 8f05e2813bf24faf00606ea79104fd7b4a533d4f Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Wed, 21 Aug 2024 12:07:19 +0200 Subject: [PATCH 1/3] Moved getAbsoluteFilePath to androidshared --- .../collect/androidshared/utils/PathUtils.kt | 17 +++++ .../android/database/DatabaseObjectMapper.kt | 2 +- .../DatabaseSavepointsRepository.kt | 5 +- .../fastexternalitemset/ItemsetDbAdapter.java | 4 +- .../download/ServerFormDownloaderTest.java | 2 +- .../java/org/odk/collect/shared/PathUtils.kt | 14 ---- .../org/odk/collect/shared/PathUtilsTest.kt | 68 +++++++++---------- 7 files changed, 59 insertions(+), 53 deletions(-) create mode 100644 androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt diff --git a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt new file mode 100644 index 00000000000..1f20205bf03 --- /dev/null +++ b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt @@ -0,0 +1,17 @@ +package org.odk.collect.androidshared.utils + +import java.io.File + +object PathUtils { + @JvmStatic + fun getAbsoluteFilePath(dirPath: String, filePath: String): String { + val absolutePath = + if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath + + if (File(absolutePath).canonicalPath.startsWith(File(dirPath).canonicalPath)) { + return absolutePath + } else { + throw SecurityException("Contact support@getodk.org. Attempt to access file outside of Collect directory: $absolutePath") + } + } +} diff --git a/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt b/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt index 5183d7ca90e..8015bcae310 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt +++ b/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt @@ -5,9 +5,9 @@ import android.database.Cursor import android.provider.BaseColumns import org.odk.collect.android.database.forms.DatabaseFormColumns import org.odk.collect.android.database.instances.DatabaseInstanceColumns +import org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath import org.odk.collect.forms.Form import org.odk.collect.forms.instances.Instance -import org.odk.collect.shared.PathUtils.getAbsoluteFilePath import org.odk.collect.shared.PathUtils.getRelativeFilePath import java.lang.Boolean diff --git a/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt b/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt index f71be767631..e134be02cb1 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt +++ b/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt @@ -10,6 +10,7 @@ import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_DATABASE_NA import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_DATABASE_VERSION import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.FORM_DB_ID import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.INSTANCE_DB_ID +import org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath import org.odk.collect.forms.savepoints.Savepoint import org.odk.collect.forms.savepoints.SavepointsRepository import org.odk.collect.shared.PathUtils @@ -129,11 +130,11 @@ class DatabaseSavepointsRepository( return Savepoint( cursor.getLong(formDbIdColumnIndex), if (cursor.isNull(instanceDbIdColumnIndex)) null else cursor.getLong(instanceDbIdColumnIndex), - PathUtils.getAbsoluteFilePath( + getAbsoluteFilePath( cachePath, cursor.getString(savepointFilePathColumnIndex) ), - PathUtils.getAbsoluteFilePath( + getAbsoluteFilePath( instancesPath, cursor.getString(instanceDirPathColumnIndex) ) diff --git a/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java b/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java index a45736fa8b4..7393d4f004f 100644 --- a/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java +++ b/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java @@ -1,5 +1,7 @@ package org.odk.collect.android.fastexternalitemset; +import static org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath; + import android.content.ContentValues; import android.database.Cursor; import android.database.SQLException; @@ -189,7 +191,7 @@ public void delete(String path) { if (c != null) { if (c.getCount() == 1) { c.moveToFirst(); - String table = getMd5FromString(PathUtils.getAbsoluteFilePath(storagePathProvider.getOdkDirPath(StorageSubdirectory.FORMS), c.getString(c.getColumnIndex(KEY_PATH)))); + String table = getMd5FromString(getAbsoluteFilePath(storagePathProvider.getOdkDirPath(StorageSubdirectory.FORMS), c.getString(c.getColumnIndex(KEY_PATH)))); db.execSQL("DROP TABLE IF EXISTS " + DATABASE_TABLE + table); } c.close(); diff --git a/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java b/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java index eb01f7f8fba..c3b41b4baf3 100644 --- a/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java +++ b/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java @@ -12,9 +12,9 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.odk.collect.android.utilities.FileUtils.read; +import static org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath; import static org.odk.collect.formstest.FormUtils.buildForm; import static org.odk.collect.formstest.FormUtils.createXFormBody; -import static org.odk.collect.shared.PathUtils.getAbsoluteFilePath; import static java.util.Arrays.asList; import static java.util.Arrays.stream; import static java.util.stream.Collectors.toList; diff --git a/shared/src/main/java/org/odk/collect/shared/PathUtils.kt b/shared/src/main/java/org/odk/collect/shared/PathUtils.kt index 66de93ac3d1..1f4ba227cc8 100644 --- a/shared/src/main/java/org/odk/collect/shared/PathUtils.kt +++ b/shared/src/main/java/org/odk/collect/shared/PathUtils.kt @@ -1,7 +1,5 @@ package org.odk.collect.shared -import java.io.File - object PathUtils { @JvmStatic @@ -9,18 +7,6 @@ object PathUtils { return if (filePath.startsWith(dirPath)) filePath.substring(dirPath.length + 1) else filePath } - @JvmStatic - fun getAbsoluteFilePath(dirPath: String, filePath: String): String { - val absolutePath = - if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath - - if (File(absolutePath).canonicalPath.startsWith(File(dirPath).canonicalPath)) { - return absolutePath - } else { - throw SecurityException("Contact support@getodk.org. Attempt to access file outside of Collect directory: $absolutePath") - } - } - // https://stackoverflow.com/questions/2679699/what-characters-allowed-in-file-names-on-android @JvmStatic fun getPathSafeFileName(fileName: String) = fileName.replace("[\"*/:<>?\\\\|]".toRegex(), "_") diff --git a/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt b/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt index 006b48f9472..b9f66174409 100644 --- a/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt +++ b/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt @@ -3,43 +3,43 @@ package org.odk.collect.shared import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.equalTo import org.junit.Test -import java.io.File +// import java.io.File class PathUtilsTest { - @Test - fun `getAbsoluteFilePath() returns filePath prepended with dirPath`() { - val path = PathUtils.getAbsoluteFilePath("/anotherRoot/anotherDir", "root/dir/file") - assertThat(path, equalTo("/anotherRoot/anotherDir/root/dir/file")) - } - - @Test - fun `getAbsoluteFilePath() returns valid path when filePath does not start with seperator`() { - val path = PathUtils.getAbsoluteFilePath("/root/dir", "file") - assertThat(path, equalTo("/root/dir/file")) - } - - @Test - fun `getAbsoluteFilePath() returns filePath when it starts with dirPath`() { - val path = PathUtils.getAbsoluteFilePath("/root/dir", "/root/dir/file") - assertThat(path, equalTo("/root/dir/file")) - } - - @Test(expected = SecurityException::class) - fun `getAbsoluteFilePath() throws SecurityException when filePath is outside the dirPath`() { - 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 `getAbsoluteFilePath() returns filePath prepended with dirPath`() { +// val path = PathUtils.getAbsoluteFilePath("/anotherRoot/anotherDir", "root/dir/file") +// assertThat(path, equalTo("/anotherRoot/anotherDir/root/dir/file")) +// } +// +// @Test +// fun `getAbsoluteFilePath() returns valid path when filePath does not start with seperator`() { +// val path = PathUtils.getAbsoluteFilePath("/root/dir", "file") +// assertThat(path, equalTo("/root/dir/file")) +// } +// +// @Test +// fun `getAbsoluteFilePath() returns filePath when it starts with dirPath`() { +// val path = PathUtils.getAbsoluteFilePath("/root/dir", "/root/dir/file") +// assertThat(path, equalTo("/root/dir/file")) +// } +// +// @Test(expected = SecurityException::class) +// fun `getAbsoluteFilePath() throws SecurityException when filePath is outside the dirPath`() { +// 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`() { From f9377c42ca43ebd1f4ad243393aa2d448ce996de Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Wed, 21 Aug 2024 13:05:53 +0200 Subject: [PATCH 2/3] Log error instead of throwing exception --- .../collect/androidshared/utils/PathUtils.kt | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt index 1f20205bf03..a2ed621597a 100644 --- a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt +++ b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt @@ -1,17 +1,26 @@ package org.odk.collect.androidshared.utils +import timber.log.Timber import java.io.File object PathUtils { @JvmStatic fun getAbsoluteFilePath(dirPath: String, filePath: String): String { - val absolutePath = + val absoluteFilePath = if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath - if (File(absolutePath).canonicalPath.startsWith(File(dirPath).canonicalPath)) { - return absolutePath - } else { - throw SecurityException("Contact support@getodk.org. Attempt to access file outside of Collect directory: $absolutePath") + val canonicalAbsoluteFilePath = File(absoluteFilePath).canonicalPath + val canonicalDirPath = File(dirPath).canonicalPath + if (!canonicalAbsoluteFilePath.startsWith(canonicalDirPath)) { + Timber.e( + "Attempt to access file outside of Collect directory:\n" + + "dirPath: $dirPath\n" + + "filePath: $filePath\n" + + "absoluteFilePath: $absoluteFilePath\n" + + "canonicalAbsoluteFilePath: $canonicalAbsoluteFilePath\n" + + "canonicalDirPath: $canonicalDirPath\n" + ) } + return absoluteFilePath } } From e3f3d60c49b0e81fe40f851baaeeba08c0d998e6 Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Wed, 21 Aug 2024 16:19:09 +0200 Subject: [PATCH 3/3] Moved tests to the androidshared module --- .../collect/androidshared/utils/PathUtils.kt | 2 +- .../androidshared/utils/PathUtilsTest.kt | 38 +++++++++++++++++++ .../org/odk/collect/shared/PathUtilsTest.kt | 36 ------------------ 3 files changed, 39 insertions(+), 37 deletions(-) create mode 100644 androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt diff --git a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt index a2ed621597a..2f2be8c598c 100644 --- a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt +++ b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt @@ -18,7 +18,7 @@ object PathUtils { "filePath: $filePath\n" + "absoluteFilePath: $absoluteFilePath\n" + "canonicalAbsoluteFilePath: $canonicalAbsoluteFilePath\n" + - "canonicalDirPath: $canonicalDirPath\n" + "canonicalDirPath: $canonicalDirPath" ) } return absoluteFilePath diff --git a/androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt b/androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt new file mode 100644 index 00000000000..472b285027d --- /dev/null +++ b/androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt @@ -0,0 +1,38 @@ +package org.odk.collect.androidshared.utils + +import org.hamcrest.MatcherAssert.assertThat +import org.hamcrest.Matchers.equalTo +import org.junit.Test +import org.odk.collect.shared.TempFiles +import java.io.File + +class PathUtilsTest { + @Test + fun `getAbsoluteFilePath() returns filePath prepended with dirPath`() { + val path = PathUtils.getAbsoluteFilePath("/anotherRoot/anotherDir", "root/dir/file") + assertThat(path, equalTo("/anotherRoot/anotherDir/root/dir/file")) + } + + @Test + fun `getAbsoluteFilePath() returns valid path when filePath does not start with seperator`() { + val path = PathUtils.getAbsoluteFilePath("/root/dir", "file") + assertThat(path, equalTo("/root/dir/file")) + } + + @Test + fun `getAbsoluteFilePath() returns filePath when it starts with dirPath`() { + val path = PathUtils.getAbsoluteFilePath("/root/dir", "/root/dir/file") + assertThat(path, equalTo("/root/dir/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")) + } +} diff --git a/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt b/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt index b9f66174409..b7c2dede84b 100644 --- a/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt +++ b/shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt @@ -3,44 +3,8 @@ 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 { - -// @Test -// fun `getAbsoluteFilePath() returns filePath prepended with dirPath`() { -// val path = PathUtils.getAbsoluteFilePath("/anotherRoot/anotherDir", "root/dir/file") -// assertThat(path, equalTo("/anotherRoot/anotherDir/root/dir/file")) -// } -// -// @Test -// fun `getAbsoluteFilePath() returns valid path when filePath does not start with seperator`() { -// val path = PathUtils.getAbsoluteFilePath("/root/dir", "file") -// assertThat(path, equalTo("/root/dir/file")) -// } -// -// @Test -// fun `getAbsoluteFilePath() returns filePath when it starts with dirPath`() { -// val path = PathUtils.getAbsoluteFilePath("/root/dir", "/root/dir/file") -// assertThat(path, equalTo("/root/dir/file")) -// } -// -// @Test(expected = SecurityException::class) -// fun `getAbsoluteFilePath() throws SecurityException when filePath is outside the dirPath`() { -// 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")