Skip to content

Commit

Permalink
Merge pull request #6363 from grzesiek2010/disable_security_exception
Browse files Browse the repository at this point in the history
Log error instead of throwing security exception
  • Loading branch information
seadowg committed Aug 23, 2024
1 parent 951f44e commit e40663c
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 55 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +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 absoluteFilePath =
if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath

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"
)
}
return absoluteFilePath
}
}
Original file line number Diff line number Diff line change
@@ -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"))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_DATABASE_VE
import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_TABLE_NAME
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.db.sqlite.CursorExt.foldAndClose
import org.odk.collect.db.sqlite.DatabaseConnection
import org.odk.collect.db.sqlite.SQLiteDatabaseExt.delete
Expand Down Expand Up @@ -120,11 +121,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)
)
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 0 additions & 14 deletions shared/src/main/java/org/odk/collect/shared/PathUtils.kt
Original file line number Diff line number Diff line change
@@ -1,26 +1,12 @@
package org.odk.collect.shared

import java.io.File

object PathUtils {

@JvmStatic
fun getRelativeFilePath(dirPath: String, filePath: String): String {
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(), "_")
Expand Down
36 changes: 0 additions & 36 deletions shared/src/test/java/org/odk/collect/shared/PathUtilsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down

0 comments on commit e40663c

Please sign in to comment.