Share: stage attachments instead of sending on selection, and resolve only content:// URIs

app/src/main/java/org/thoughtcrime/securesms/MediaPreviewActivity.kt

@@ -150,6 +150,9 @@ class MediaPreviewActivity : ScreenLockActionBarActivity(),
     @Inject
     lateinit var mediaDatabase: MediaDatabase
 
+    @Inject
+    lateinit var shareIntentTokenStore: ShareIntentTokenStore
+
     override val applyDefaultWindowInsets: Boolean
         get() = false
 
@@ -488,6 +491,12 @@ class MediaPreviewActivity : ScreenLockActionBarActivity(),
             )
             composeIntent.setAction(Intent.ACTION_SEND)
             composeIntent.putExtra(Intent.EXTRA_STREAM, mediaItem.uri)
+            // ShareActivity passes one of our own attachment URIs along untouched only for the exact
+            // URIs a token vouches for; without this it would have nothing to read.
+            composeIntent.putExtra(
+                ShareActivity.EXTRA_SHARE_TOKEN,
+                shareIntentTokenStore.mint(authorisedUris = setOf(mediaItem.uri))
+            )
             composeIntent.setType(mediaItem.mimeType)
             startActivity(composeIntent)
         }

app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt

@@ -2,12 +2,14 @@ package org.thoughtcrime.securesms
 
 import android.content.BroadcastReceiver
 import android.content.ClipData
+import android.content.ContentResolver
 import android.content.Context
 import android.content.Intent
 import android.content.IntentFilter
 import android.net.Uri
 import android.os.Bundle
 import androidx.annotation.IdRes
+import androidx.annotation.VisibleForTesting
 import androidx.core.content.ContextCompat
 import androidx.core.content.IntentCompat
 import androidx.fragment.app.Fragment
@@ -21,6 +23,7 @@ import org.session.libsignal.utilities.Log
 import org.thoughtcrime.securesms.auth.LoginStateRepository
 import org.thoughtcrime.securesms.home.HomeActivity
 import org.thoughtcrime.securesms.migration.DatabaseMigrationManager
+import org.thoughtcrime.securesms.mms.PartAuthority
 import org.thoughtcrime.securesms.migration.DatabaseMigrationStateActivity
 import org.thoughtcrime.securesms.onboarding.landing.LandingActivity
 import org.thoughtcrime.securesms.service.KeyCachingService
@@ -345,14 +348,36 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
 
     private suspend fun copyFileToCache(uri: Uri, filename: String): Uri? = withContext(Dispatchers.IO) {
         try {
+            // A URI grant is the only thing that makes the sender's content readable to us, and only
+            // content:// carries one. openInputStream also accepts file:// and android.resource://,
+            // both of which it opens as this app with nothing consulted - and this runs before the
+            // user has authenticated, so it must not be able to reach anything of ours.
+            if (ContentResolver.SCHEME_CONTENT != uri.scheme) {
+                Log.w(TAG, "Refusing to cache a shared URI that carries no content grant - aborting.")
+                return@withContext null
+            }
+
+            // Our own providers answer us regardless of being unexported, and our FileProvider's
+            // configured roots include this very cache directory - so without this the copy below
+            // would read our own data back for the sender, still before they have authenticated.
+            if (PartAuthority.isLocalUri(uri) || FileProviderUtil.AUTHORITY == uri.authority) {
+                Log.w(TAG, "Refusing to cache a shared URI that names one of our own providers - aborting.")
+                return@withContext null
+            }
+
+            val cacheFilename = cacheFilenameFrom(filename)
+            if (cacheFilename == null) {
+                Log.w(TAG, "Shared content did not provide a usable filename - aborting.")
+                return@withContext null
+            }
+
             val inputStream = contentResolver.openInputStream(uri)
             if (inputStream == null) {
                 Log.w(TAG, "Could not open input stream to cache shared content - aborting.")
                 return@withContext null
             }
 
-            // Create a File in your cache directory using the retrieved name
-            val tempFile = File(cacheDir, filename)
+            val tempFile = File(cacheDir, cacheFilename)
             inputStream.use { input ->
                 FileOutputStream(tempFile).use { output ->
                     input.copyTo(output)
@@ -413,4 +438,16 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
             clearKeyReceiver = null
         }
     }
-}
+}
+
+/**
+ * Reduces a sending app's `OpenableColumns.DISPLAY_NAME` to a name that can only land directly in the
+ * directory it is joined to, or null when nothing usable is left of it.
+ *
+ * The display name reaches us verbatim from the sending app and is not a path segment until it is
+ * made one: joined as given it lets "../" out of the directory, and the two relative names survive
+ * the reduction still naming a directory rather than a file.
+ */
+@VisibleForTesting
+internal fun cacheFilenameFrom(displayName: String): String? =
+    File(displayName).name.takeUnless { it.isEmpty() || it == "." || it == ".." }

app/src/main/java/org/thoughtcrime/securesms/ShareActivity.kt

@@ -32,7 +32,7 @@ class ShareActivity : FullComposeScreenLockActivity() {
     private val viewModel: ShareViewModel by viewModels()
 
     companion object {
-        const val EXTRA_ADDRESS = "address"
+        const val EXTRA_SHARE_TOKEN = "share_token"
     }
 
 

app/src/main/java/org/thoughtcrime/securesms/ShareIntentTokenStore.kt

@@ -0,0 +1,66 @@
+package org.thoughtcrime.securesms
+
+import android.net.Uri
+import org.session.libsession.utilities.Address
+import java.security.SecureRandom
+import java.util.Base64
+import javax.inject.Inject
+import javax.inject.Singleton
+
+/**
+ * Issues opaque tokens that mark a share Intent as one this app built itself, and carries the
+ * conversation such an Intent should open.
+ *
+ * `ShareActivity` is exported, so the Intent it receives is composed by whichever app invoked the
+ * share sheet. A token stands in for the destination because it means nothing outside this process,
+ * where the table that resolves it lives.
+ *
+ * A token names the URIs it speaks for rather than merely existing, because the two do not arrive
+ * together: the system chooser merges a direct-share target's extras into the *sender's* Intent, so
+ * a token minted here can reach us alongside URIs chosen by another app.
+ */
+@Singleton
+class ShareIntentTokenStore @Inject constructor() {
+
+    /**
+     * Present only for a token this store issued. A null [address] means "no destination chosen",
+     * and [authorisedUris] is the exact set of our own URIs the Intent carrying it may pass along -
+     * usually empty.
+     */
+    class Minted(val address: Address?, val authorisedUris: Set<Uri>) {
+        fun authorises(uri: Uri): Boolean = uri in authorisedUris
+    }
+
+    private val random = SecureRandom()
+
+    private val issued = LinkedHashMap<String, Minted>()
+
+    @JvmOverloads
+    @Synchronized
+    fun mint(address: Address? = null, authorisedUris: Set<Uri> = emptySet()): String {
+        // A chooser refresh mints one token per conversation, so retention is capped rather than
+        // left to grow with however many times the share sheet has been opened this process.
+        while (issued.size >= MAX_RETAINED) {
+            issued.remove(issued.keys.first())
+        }
+
+        val token = Base64.getUrlEncoder().withoutPadding()
+            .encodeToString(ByteArray(TOKEN_BYTES).also(random::nextBytes))
+
+        issued[token] = Minted(address, authorisedUris)
+        return token
+    }
+
+    /**
+     * Resolution deliberately does not retire the token: one user action can create `ShareActivity`
+     * twice - once before app lock routes it away, once from the Intent the lock screen replays -
+     * and each instance resolves the Intent independently.
+     */
+    @Synchronized
+    fun resolve(token: String?): Minted? = token?.let(issued::get)
+
+    private companion object {
+        private const val TOKEN_BYTES = 32
+        private const val MAX_RETAINED = 512
+    }
+}

app/src/main/java/org/thoughtcrime/securesms/ShareViewModel.kt

@@ -1,10 +1,11 @@
 package org.thoughtcrime.securesms
 
+import android.content.ContentResolver
 import android.content.Context
 import android.content.Intent
 import android.net.Uri
 import android.provider.OpenableColumns
-import androidx.core.content.IntentCompat
+import androidx.annotation.VisibleForTesting
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.viewModelScope
 import dagger.hilt.android.lifecycle.HiltViewModel
@@ -35,9 +36,9 @@ import org.thoughtcrime.securesms.mms.PartAuthority
 import org.thoughtcrime.securesms.providers.BlobUtils
 import org.thoughtcrime.securesms.repository.ConversationRepository
 import org.thoughtcrime.securesms.util.AvatarUIData
+import org.thoughtcrime.securesms.util.FileProviderUtil
 import org.thoughtcrime.securesms.util.AvatarUtils
 import org.thoughtcrime.securesms.util.MediaUtil
-import java.io.FileInputStream
 import java.io.IOException
 import javax.inject.Inject
 
@@ -46,6 +47,7 @@ class ShareViewModel @Inject constructor(
     @ApplicationContext private val context: Context,
     private val avatarUtils: AvatarUtils,
     private val deprecationManager: LegacyGroupDeprecationManager,
+    private val shareIntentTokenStore: ShareIntentTokenStore,
     conversationRepository: ConversationRepository,
 ): ViewModel(){
 
@@ -55,6 +57,8 @@ class ShareViewModel @Inject constructor(
     private var resolvedPlaintext: CharSequence? = null
     private var mimeType: String? = null
     private var isPassingAlongMedia = false
+    private var minted: ShareIntentTokenStore.Minted? = null
+    private var shareDestination: Address? = null
 
     // Input: The search query
     private val mutableSearchQuery = MutableStateFlow("")
@@ -144,6 +148,10 @@ class ShareViewModel @Inject constructor(
         mimeType = null
         isPassingAlongMedia = false
 
+        val minted = shareIntentTokenStore.resolve(intent.getStringExtra(ShareActivity.EXTRA_SHARE_TOKEN))
+        this.minted = minted
+        shareDestination = minted?.address
+
         val action = intent.action
         val type = intent.type
         val incomingUris = ArrayList<Uri>()
@@ -176,50 +184,79 @@ class ShareViewModel @Inject constructor(
         isPassingAlongMedia = false
         mimeType = getMimeType(uris.firstOrNull(), type)
 
-        if (uris.isNotEmpty() && uris.all { PartAuthority.isLocalUri(it) }) {
+        // A URI naming one of our own providers is passed to the attachment manager verbatim, which
+        // reads it as us - so it resolves to the viewer's own message history rather than to anything
+        // the sender holds. Only the exact URIs a token was minted for may take that route: holding a
+        // token is not enough, because the chooser merges our direct-share extras into the sender's
+        // own Intent, so a valid token can arrive alongside URIs we never vouched for.
+        if (minted != null && uris.isNotEmpty() && uris.all { minted.authorises(it) }) {
             isPassingAlongMedia = true
             resolvedExtras = uris
-            handleResolvedMedia(intent)
+            handleResolvedMedia()
         } else if (
             uris.isEmpty() &&
             charSequenceExtra != null &&
             (mimeType?.startsWith("text/") == true)
         ) {
             resolvedPlaintext = charSequenceExtra
-            handleResolvedMedia(intent)
+            handleResolvedMedia()
         } else if (uris.isNotEmpty()) {
             _uiState.update { it.copy(showLoader = true) }
-            resolveMedia(intent, uris)
+            resolveMedia(uris)
         } else {
             _uiState.update { it.copy(showLoader = false) }
         }
     }
 
-    private fun handleResolvedMedia(intent: Intent) {
-        val address = IntentCompat.getParcelableExtra(intent, ShareActivity.EXTRA_ADDRESS, Address::class.java)
+    private fun handleResolvedMedia() {
+        val address = shareDestination
         if (address is Address.Conversable) {
             createConversation(address)
         } else {
             _uiState.update { it.copy(showLoader = false) }
         }
     }
 
-    private fun resolveMedia(intent: Intent, uris: List<Uri>){
+    private fun resolveMedia(uris: List<Uri>){
         viewModelScope.launch(Dispatchers.Default){
             resolvedExtras = uris.mapNotNull { processSingleUri(it) }
-            handleResolvedMedia(intent)
+            handleResolvedMedia()
+        }
+    }
+
+    /**
+     * Whether a URI offered by whoever sent the share Intent may be opened on their behalf.
+     */
+    @VisibleForTesting
+    internal fun canReadSharedUri(uri: Uri): Boolean {
+        // A URI grant is what makes the sender's content readable to us, and only content:// carries
+        // one. openInputStream also accepts file:// and android.resource://, both of which it opens
+        // as this app with nothing consulted, so anything this app can reach would be readable by
+        // whoever sent the Intent.
+        if (ContentResolver.SCHEME_CONTENT != uri.scheme) {
+            Log.w(TAG, "Refusing a shared URI that carries no content grant.")
+            return false
+        }
+
+        // Our own providers answer us whether or not they are exported, so these resolve to our own
+        // data rather than to anything the sender holds. That covers the attachment and blob
+        // providers, and equally our FileProvider, whose configured roots include the cache
+        // directory and external storage.
+        if (PartAuthority.isLocalUri(uri) || FileProviderUtil.AUTHORITY == uri.authority) {
+            Log.w(TAG, "Refusing a shared URI that names one of our own providers.")
+            return false
         }
+
+        return true
     }
 
     private fun processSingleUri(uri: Uri): Uri? {
         try {
             Log.i(TAG, "Resolving URI: " + uri.toString() + " - " + uri.path)
 
-            val inputStream = if ("file" == uri.scheme) {
-                FileInputStream(uri.path)
-            } else {
-                context.contentResolver.openInputStream(uri)
-            }
+            if (!canReadSharedUri(uri)) return null
+
+            val inputStream = context.contentResolver.openInputStream(uri)
 
             if (inputStream == null) {
                 Log.w(TAG, "Failed to create input stream during ShareActivity - bailing.")

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt

@@ -1109,9 +1109,9 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
             } else {
                 prepMediaForSending(mediaURI, mediaType).addListener(object : ListenableFuture.Listener<Boolean> {
 
-                    override fun onSuccess(result: Boolean?) {
-                        sendAttachments(attachmentManager.buildSlideDeck().asAttachments(), null)
-                    }
+                    // Nothing to do on success: prepMediaForSending stages the attachment in the input
+                    // bar, and the send is the user's to make once they can see what they shared.
+                    override fun onSuccess(result: Boolean?) {}
 
                     override fun onFailure(e: ExecutionException?) {
                         Toast.makeText(this@ConversationActivityV2, R.string.attachmentsErrorLoad, Toast.LENGTH_LONG).show()
@@ -2370,7 +2370,14 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
 
         viewModel.beforeSendMessage()
 
-        if (binding.inputBar.linkPreview != null || binding.inputBar.quote != null) {
+        if (attachmentManager.isAttachmentPresent()) {
+            sendAttachments(
+                attachmentManager.buildSlideDeck().asAttachments(),
+                getMessageBody(),
+                binding.inputBar.quote,
+                binding.inputBar.linkPreview
+            )
+        } else if (binding.inputBar.linkPreview != null || binding.inputBar.quote != null) {
             sendAttachments(listOf(), getMessageBody(), binding.inputBar.quote, binding.inputBar.linkPreview)
         } else {
             sendTextOnlyMessage()
@@ -2612,7 +2619,16 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
         )
     }
 
-    override fun onAttachmentChanged() { /* Do nothing */ }
+    override fun onAttachmentChanged() {
+        val slide = attachmentManager.getSlide()
+        if (slide != null) binding.inputBar.showAttachmentDraft(glide, slide)
+        else binding.inputBar.clearAttachmentDraft()
+    }
+
+    override fun cancelAttachmentDraft() {
+        attachmentManager.clear()
+        if (isShowingAttachmentOptions) { toggleAttachmentOptions() }
+    }
 
     override fun onRequestPermissionsResult(requestCode: Int, permissions: Array<out String>, grantResults: IntArray) {
         super.onRequestPermissionsResult(requestCode, permissions, grantResults)
@@ -2633,18 +2649,12 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
 
                 // If the attachment was too large or MediaConstraints.isSatisfied failed for some
                 // other reason then we reset the attachment manager & shown buttons then bail..
+                // Otherwise it is left staged in the input bar, so that the user can see what they
+                // picked, add a message to it, and choose to send.
                 if (!result) {
                     attachmentManager.clear()
                     if (isShowingAttachmentOptions) { toggleAttachmentOptions() }
-                    return
                 }
-
-                // ..otherwise we can attempt to send the attachment(s).
-                // Note: The only multi-attachment message type is when sending images - all others
-                // attempt send the attachment immediately upon file selection.
-                sendAttachments(attachmentManager.buildSlideDeck().asAttachments(), null)
-                //todo: The current system sends the document the moment it has been selected, without text (body is set to null above) - We will want to fix this and allow the user to add text with a document AND be able to confirm before sending
-                //todo: Simply setting body to getMessageBody() above isn't good enough as it doesn't give the user a chance to confirm their message before sending it.
             }
 
             override fun onFailure(e: ExecutionException?) {