Commit 3ea28238 authored by Arturo Mejia's avatar Arturo Mejia
Browse files

Closes #7711 and #7721: Improve file picker selection.

parent 84d7a0f7
......@@ -18,6 +18,7 @@ import mozilla.components.concept.engine.prompt.PromptRequest.MultipleChoice
import mozilla.components.concept.engine.prompt.PromptRequest.SingleChoice
import mozilla.components.concept.engine.prompt.ShareData
import mozilla.components.support.base.log.logger.Logger
import mozilla.components.support.ktx.kotlin.sanitizeFileName
import mozilla.components.support.ktx.kotlin.toDate
import org.mozilla.geckoview.AllowOrDeny
import org.mozilla.geckoview.GeckoResult
......@@ -558,7 +559,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe
cursor.moveToFirst()
fileName = cursor.getString(nameIndex)
}
return fileName
return fileName.sanitizeFileName()
}
}
......
......@@ -18,6 +18,7 @@ import mozilla.components.concept.engine.prompt.PromptRequest.MultipleChoice
import mozilla.components.concept.engine.prompt.PromptRequest.SingleChoice
import mozilla.components.concept.engine.prompt.ShareData
import mozilla.components.support.base.log.logger.Logger
import mozilla.components.support.ktx.kotlin.sanitizeFileName
import mozilla.components.support.ktx.kotlin.toDate
import org.mozilla.geckoview.AllowOrDeny
import org.mozilla.geckoview.GeckoResult
......@@ -558,7 +559,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe
cursor.moveToFirst()
fileName = cursor.getString(nameIndex)
}
return fileName
return fileName.sanitizeFileName()
}
}
......
......@@ -18,6 +18,7 @@ import mozilla.components.concept.engine.prompt.PromptRequest.MultipleChoice
import mozilla.components.concept.engine.prompt.PromptRequest.SingleChoice
import mozilla.components.concept.engine.prompt.ShareData
import mozilla.components.support.base.log.logger.Logger
import mozilla.components.support.ktx.kotlin.sanitizeFileName
import mozilla.components.support.ktx.kotlin.toDate
import org.mozilla.geckoview.AllowOrDeny
import org.mozilla.geckoview.GeckoResult
......@@ -558,7 +559,7 @@ internal class GeckoPromptDelegate(private val geckoEngineSession: GeckoEngineSe
cursor.moveToFirst()
fileName = cursor.getString(nameIndex)
}
return fileName
return fileName.sanitizeFileName()
}
}
......
......@@ -11,6 +11,7 @@ android {
defaultConfig {
minSdkVersion config.minSdkVersion
targetSdkVersion config.targetSdkVersion
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
}
buildTypes {
......@@ -47,6 +48,10 @@ dependencies {
testImplementation Dependencies.testing_mockito
testImplementation project(':support-test')
testImplementation project(':support-test-libstate')
androidTestImplementation project(':support-android-test')
androidTestImplementation Dependencies.androidx_test_core
androidTestImplementation Dependencies.androidx_test_runner
}
apply from: '../../../publish.gradle'
......
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.feature.prompts.file
import android.content.ClipData
import android.content.Context
import android.content.Intent
import androidx.core.net.toUri
import androidx.test.core.app.ApplicationProvider
import mozilla.components.browser.state.store.BrowserStore
import mozilla.components.concept.engine.prompt.PromptRequest
import mozilla.components.feature.prompts.PromptContainer
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
class OnDeviceFilePickerTest {
private val context: Context
get() = ApplicationProvider.getApplicationContext()
private val badUri1 = "file:///data/user/0/${context.packageName}/any_directory/file.text".toUri()
private val badUri2 = "file:///data/data/${context.packageName}/any_directory/file.text".toUri()
private val goodUri = "file:///data/directory/${context.packageName}/any_directory/file.text".toUri()
@Test
fun removeUrisUnderPrivateAppDir() {
val uris = arrayOf(badUri1, badUri2, goodUri)
val filterUris = uris.removeUrisUnderPrivateAppDir(context)
assertEquals(1, filterUris.size)
assertTrue(filterUris.contains(goodUri))
}
@Test
fun unsafeUrisWillNotBeSelected() {
val promptContainer = PromptContainer.TestPromptContainer(context)
val filePicker = FilePicker(promptContainer, BrowserStore()) { }
var onDismissWasExecuted = false
var onSingleFileSelectedWasExecuted = false
var onMultipleFilesSelectedWasExecuted = false
val filePickerRequest = PromptRequest.File(
arrayOf(""),
isMultipleFilesSelection = true,
onDismiss = { onDismissWasExecuted = true },
onSingleFileSelected = { _, _ -> onSingleFileSelectedWasExecuted = true },
onMultipleFilesSelected = { _, _ -> onMultipleFilesSelectedWasExecuted = true }
)
val intent = Intent()
intent.clipData = ClipData("", arrayOf(), ClipData.Item(badUri1))
filePicker.handleFilePickerIntentResult(intent, filePickerRequest)
assertTrue(onDismissWasExecuted)
assertFalse(onSingleFileSelectedWasExecuted)
assertFalse(onMultipleFilesSelectedWasExecuted)
}
@Test
fun safeUrisWillBeSelected() {
val promptContainer = PromptContainer.TestPromptContainer(context)
val filePicker = FilePicker(promptContainer, BrowserStore()) { }
var urisWereSelected = false
var onDismissWasExecuted = false
var onSingleFileSelectedWasExecuted = false
val filePickerRequest = PromptRequest.File(
arrayOf(""),
isMultipleFilesSelection = true,
onDismiss = { onDismissWasExecuted = true },
onSingleFileSelected = { _, _ -> onSingleFileSelectedWasExecuted = true },
onMultipleFilesSelected = { _, uris -> urisWereSelected = uris.isNotEmpty() }
)
val intent = Intent()
intent.clipData = ClipData("", arrayOf(), ClipData.Item(goodUri))
filePicker.handleFilePickerIntentResult(intent, filePickerRequest)
assertTrue(urisWereSelected)
assertFalse(onDismissWasExecuted)
assertFalse(onSingleFileSelectedWasExecuted)
}
@Test
fun unsafeUriWillNotBeSelected() {
val promptContainer = PromptContainer.TestPromptContainer(context)
val filePicker = FilePicker(promptContainer, BrowserStore()) { }
var onDismissWasExecuted = false
var onSingleFileSelectedWasExecuted = false
var onMultipleFilesSelectedWasExecuted = false
val filePickerRequest = PromptRequest.File(
arrayOf(""),
onDismiss = { onDismissWasExecuted = true },
onSingleFileSelected = { _, _ -> onSingleFileSelectedWasExecuted = true },
onMultipleFilesSelected = { _, _ -> onMultipleFilesSelectedWasExecuted = true }
)
val intent = Intent()
intent.data = badUri1
filePicker.handleFilePickerIntentResult(intent, filePickerRequest)
assertTrue(onDismissWasExecuted)
assertFalse(onSingleFileSelectedWasExecuted)
assertFalse(onMultipleFilesSelectedWasExecuted)
}
@Test
fun safeUriWillBeSelected() {
val promptContainer = PromptContainer.TestPromptContainer(context)
val filePicker = FilePicker(promptContainer, BrowserStore()) { }
var onDismissWasExecuted = false
var onSingleFileSelectedWasExecuted = false
var onMultipleFilesSelectedWasExecuted = false
val filePickerRequest = PromptRequest.File(
arrayOf(""),
onDismiss = { onDismissWasExecuted = true },
onSingleFileSelected = { _, _ -> onSingleFileSelectedWasExecuted = true },
onMultipleFilesSelected = { _, _ -> onMultipleFilesSelectedWasExecuted = true }
)
val intent = Intent()
intent.data = goodUri
filePicker.handleFilePickerIntentResult(intent, filePickerRequest)
assertTrue(onSingleFileSelectedWasExecuted)
assertFalse(onMultipleFilesSelectedWasExecuted)
assertFalse(onDismissWasExecuted)
}
}
......@@ -7,6 +7,7 @@ package mozilla.components.feature.prompts
import android.content.Context
import android.content.Intent
import androidx.annotation.StringRes
import androidx.annotation.VisibleForTesting
/**
* Wrapper to hold shared functionality between activities and fragments for [PromptFeature].
......@@ -51,4 +52,10 @@ internal sealed class PromptContainer {
override fun getString(res: Int) = fragment.getString(res)
}
@VisibleForTesting
internal class TestPromptContainer(override val context: Context) : PromptContainer() {
override fun startActivityForResult(intent: Intent, code: Int) = Unit
override fun getString(res: Int) = context.getString(res)
}
}
......@@ -6,6 +6,7 @@ package mozilla.components.feature.prompts.file
import android.app.Activity
import android.app.Activity.RESULT_OK
import android.content.Context
import android.content.Intent
import android.content.Intent.EXTRA_INITIAL_INTENTS
import android.content.pm.PackageManager.PERMISSION_GRANTED
......@@ -22,6 +23,7 @@ import mozilla.components.feature.prompts.consumePromptFrom
import mozilla.components.support.base.feature.OnNeedToRequestPermissions
import mozilla.components.support.base.feature.PermissionsFeature
import mozilla.components.support.ktx.android.content.isPermissionGranted
import mozilla.components.support.ktx.android.net.isUnderPrivateAppDirectory
/**
* @property container The [Activity] or [Fragment] which hosts the file picker.
......@@ -159,13 +161,24 @@ internal class FilePicker(
if (intent?.clipData != null && request.isMultipleFilesSelection) {
intent.clipData?.run {
val uris = Array<Uri>(itemCount) { index -> getItemAt(index).uri }
request.onMultipleFilesSelected(container.context, uris)
// We want to verify that we are not exposing any private data
val sanitizedUris = uris.removeUrisUnderPrivateAppDir(container.context)
if (sanitizedUris.isEmpty()) {
request.onDismiss()
} else {
request.onMultipleFilesSelected(container.context, sanitizedUris)
}
}
} else {
val uri = intent?.data ?: captureUri
uri?.let {
request.onSingleFileSelected(container.context, it)
} ?: request.onDismiss
// We want to verify that we are not exposing any private data
if (!it.isUnderPrivateAppDirectory(container.context)) {
request.onSingleFileSelected(container.context, it)
} else {
request.onDismiss()
}
} ?: request.onDismiss()
}
}
......@@ -176,3 +189,7 @@ internal class FilePicker(
const val FILE_PICKER_ACTIVITY_REQUEST_CODE = 7113
}
}
internal fun Array<Uri>.removeUrisUnderPrivateAppDir(context: Context): Array<Uri> {
return this.filter { !it.isUnderPrivateAppDirectory(context) }.toTypedArray()
}
......@@ -41,6 +41,10 @@ dependencies {
testImplementation project(':support-test')
androidTestImplementation project(':support-android-test')
androidTestImplementation Dependencies.androidx_test_core
androidTestImplementation Dependencies.androidx_test_runner
testImplementation Dependencies.androidx_test_core
testImplementation Dependencies.androidx_test_junit
testImplementation Dependencies.kotlin_reflect
......
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.support.ktx.android.net
import android.content.Context
import androidx.core.net.toUri
import androidx.test.core.app.ApplicationProvider
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
class OnDeviceUriKtTest {
private val context: Context
get() = ApplicationProvider.getApplicationContext()
@Test
fun isUnderPrivateAppDirectory() {
var uri = "file:///data/user/0/${context.packageName}/any_directory/file.text".toUri()
assertTrue(uri.isUnderPrivateAppDirectory(context))
uri = "file:///data/data/${context.packageName}/any_directory/file.text".toUri()
assertTrue(uri.isUnderPrivateAppDirectory(context))
uri = "file:///data/directory/${context.packageName}/any_directory/file.text".toUri()
assertFalse(uri.isUnderPrivateAppDirectory(context))
}
}
......@@ -4,7 +4,12 @@
package mozilla.components.support.ktx.android.net
import android.content.ContentResolver
import android.content.Context
import android.net.Uri
import android.os.Build
import java.io.File
import java.io.IOException
private val commonPrefixes = listOf("www.", "mobile.", "m.")
......@@ -53,3 +58,30 @@ fun Uri.sameSchemeAndHostAs(other: Uri) = scheme == other.scheme && host == othe
* https://html.spec.whatwg.org/multipage/origin.html#same-origin
*/
fun Uri.sameOriginAs(other: Uri) = sameSchemeAndHostAs(other) && port == other.port
/**
* Indicate if the [this] uri is under the application private directory.
*/
fun Uri.isUnderPrivateAppDirectory(context: Context): Boolean {
return when (this.scheme) {
ContentResolver.SCHEME_FILE -> {
try {
val uriPath = path ?: return true
val uriCanonicalPath = File(uriPath).canonicalPath
val dataDirCanonicalPath = File(context.applicationInfo.dataDir).canonicalPath
if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.Q) {
uriCanonicalPath.startsWith(dataDirCanonicalPath)
} else {
// We have to do this manual check on early builds of Android 11
// as symlink didn't resolve from /data/user/ to data/data
// we have to revise this again once Android 11 is out
// https://github.com/mozilla-mobile/android-components/issues/7750
uriCanonicalPath.startsWith("/data/data") || uriCanonicalPath.startsWith("/data/user")
}
} catch (e: IOException) {
true
}
}
else -> false
}
}
......@@ -7,6 +7,7 @@
package mozilla.components.support.ktx.kotlin
import mozilla.components.support.utils.URLStringUtils
import java.io.File
import java.net.MalformedURLException
import java.net.URL
import java.security.MessageDigest
......@@ -136,3 +137,11 @@ fun String.isSameOriginAs(other: String): Boolean {
fun String.sanitizeURL(): String {
return this.trim()
}
/**
* Remove any unwanted character from string containing file name.
* For example for an input of "/../../../../../../directory/file.txt" you will get "file.txt"
*/
fun String.sanitizeFileName(): String {
return this.substringAfterLast(File.separatorChar)
}
......@@ -169,4 +169,16 @@ class StringTest {
assertFalse("https://mozilla.org".isResourceUrl())
assertFalse("http://mozilla.org".isResourceUrl())
}
@Test
fun sanitizeFileName() {
var file = "/../../../../../../../../../../directory/file.txt"
val fileName = "file.txt"
assertEquals(fileName, file.sanitizeFileName())
file = "/root/directory/file.txt"
assertEquals(fileName, file.sanitizeFileName())
}
}
......@@ -111,4 +111,18 @@ jobs:
post-gradlew:
- ['automation/taskcluster/androidTest/ui-test.sh', 'feature-logins', 'arm', '1']
treeherder:
symbol: 'unit-logins'
\ No newline at end of file
symbol: 'unit-logins'
android-feature-prompts:
description: 'Run unit tests on device for feature prompts'
run:
post-gradlew:
- ['automation/taskcluster/androidTest/ui-test.sh', 'feature-prompts', 'arm', '1']
treeherder:
symbol: 'unit-feature-prompts'
android-support-ktx:
description: 'Run unit tests on device for support ktx'
run:
post-gradlew:
- ['automation/taskcluster/androidTest/ui-test.sh', 'support-ktx', 'arm', '1']
treeherder:
symbol: 'unit-support-ktx'
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment