Merge #7736

7736:  Closes #7711 and #7721: Improve file picker selection.  r=csadilek,Mugurell a=Amejia481
Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>

components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt

@@ -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()
     }
 }
 

components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt

@@ -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()
     }
 }
 

components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/prompt/GeckoPromptDelegate.kt

@@ -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()
     }
 }
 

components/feature/prompts/build.gradle

@@ -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'

components/feature/prompts/src/androidTest/java/mozilla/components/feature/prompts/file/OnDeviceFilePickerTest.kt

+/* 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)
+    }
+}

components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptContainer.kt

@@ -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)
+    }
 }

components/feature/prompts/src/main/java/mozilla/components/feature/prompts/file/FilePicker.kt

@@ -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()
+}

components/support/ktx/build.gradle

@@ -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

components/support/ktx/src/androidTest/java/mozilla/components/support/ktx/android/net/OnDeviceUriKtTest.kt

+/* 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))
+    }
+}

components/support/ktx/src/main/java/mozilla/components/support/ktx/android/net/Uri.kt

@@ -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
+    }
+}

components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt

@@ -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)
+}

components/support/ktx/src/test/java/mozilla/components/support/ktx/kotlin/StringTest.kt

@@ -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())
+    }
 }

taskcluster/ci/test/kind.yml

@@ -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'