Skip to content

Commit 07cfffb

Browse files
mpretty-cyrompretty-cyro
authored
Merge pull request #2222 from session-foundation/fix/share-intent-uri-handling
Share: stage attachments instead of sending on selection, and resolve only content:// URIs
2 parents 34fd2ba + 21ca884 commit 07cfffb

15 files changed

Lines changed: 728 additions & 36 deletions

File tree

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,9 @@ class MediaPreviewActivity : ScreenLockActionBarActivity(),
150150
@Inject
151151
lateinit var mediaDatabase: MediaDatabase
152152

153+
@Inject
154+
lateinit var shareIntentTokenStore: ShareIntentTokenStore
155+
153156
override val applyDefaultWindowInsets: Boolean
154157
get() = false
155158

@@ -488,6 +491,12 @@ class MediaPreviewActivity : ScreenLockActionBarActivity(),
488491
)
489492
composeIntent.setAction(Intent.ACTION_SEND)
490493
composeIntent.putExtra(Intent.EXTRA_STREAM, mediaItem.uri)
494+
// ShareActivity passes one of our own attachment URIs along untouched only for the exact
495+
// URIs a token vouches for; without this it would have nothing to read.
496+
composeIntent.putExtra(
497+
ShareActivity.EXTRA_SHARE_TOKEN,
498+
shareIntentTokenStore.mint(authorisedUris = setOf(mediaItem.uri))
499+
)
491500
composeIntent.setType(mediaItem.mimeType)
492501
startActivity(composeIntent)
493502
}

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

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@ package org.thoughtcrime.securesms
22

33
import android.content.BroadcastReceiver
44
import android.content.ClipData
5+
import android.content.ContentResolver
56
import android.content.Context
67
import android.content.Intent
78
import android.content.IntentFilter
89
import android.net.Uri
910
import android.os.Bundle
1011
import androidx.annotation.IdRes
12+
import androidx.annotation.VisibleForTesting
1113
import androidx.core.content.ContextCompat
1214
import androidx.core.content.IntentCompat
1315
import androidx.fragment.app.Fragment
@@ -21,6 +23,7 @@ import org.session.libsignal.utilities.Log
2123
import org.thoughtcrime.securesms.auth.LoginStateRepository
2224
import org.thoughtcrime.securesms.home.HomeActivity
2325
import org.thoughtcrime.securesms.migration.DatabaseMigrationManager
26+
import org.thoughtcrime.securesms.mms.PartAuthority
2427
import org.thoughtcrime.securesms.migration.DatabaseMigrationStateActivity
2528
import org.thoughtcrime.securesms.onboarding.landing.LandingActivity
2629
import org.thoughtcrime.securesms.service.KeyCachingService
@@ -345,14 +348,36 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
345348

346349
private suspend fun copyFileToCache(uri: Uri, filename: String): Uri? = withContext(Dispatchers.IO) {
347350
try {
351+
// A URI grant is the only thing that makes the sender's content readable to us, and only
352+
// content:// carries one. openInputStream also accepts file:// and android.resource://,
353+
// both of which it opens as this app with nothing consulted - and this runs before the
354+
// user has authenticated, so it must not be able to reach anything of ours.
355+
if (ContentResolver.SCHEME_CONTENT != uri.scheme) {
356+
Log.w(TAG, "Refusing to cache a shared URI that carries no content grant - aborting.")
357+
return@withContext null
358+
}
359+
360+
// Our own providers answer us regardless of being unexported, and our FileProvider's
361+
// configured roots include this very cache directory - so without this the copy below
362+
// would read our own data back for the sender, still before they have authenticated.
363+
if (PartAuthority.isLocalUri(uri) || FileProviderUtil.AUTHORITY == uri.authority) {
364+
Log.w(TAG, "Refusing to cache a shared URI that names one of our own providers - aborting.")
365+
return@withContext null
366+
}
367+
368+
val cacheFilename = cacheFilenameFrom(filename)
369+
if (cacheFilename == null) {
370+
Log.w(TAG, "Shared content did not provide a usable filename - aborting.")
371+
return@withContext null
372+
}
373+
348374
val inputStream = contentResolver.openInputStream(uri)
349375
if (inputStream == null) {
350376
Log.w(TAG, "Could not open input stream to cache shared content - aborting.")
351377
return@withContext null
352378
}
353379

354-
// Create a File in your cache directory using the retrieved name
355-
val tempFile = File(cacheDir, filename)
380+
val tempFile = File(cacheDir, cacheFilename)
356381
inputStream.use { input ->
357382
FileOutputStream(tempFile).use { output ->
358383
input.copyTo(output)
@@ -413,4 +438,16 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() {
413438
clearKeyReceiver = null
414439
}
415440
}
416-
}
441+
}
442+
443+
/**
444+
* Reduces a sending app's `OpenableColumns.DISPLAY_NAME` to a name that can only land directly in the
445+
* directory it is joined to, or null when nothing usable is left of it.
446+
*
447+
* The display name reaches us verbatim from the sending app and is not a path segment until it is
448+
* made one: joined as given it lets "../" out of the directory, and the two relative names survive
449+
* the reduction still naming a directory rather than a file.
450+
*/
451+
@VisibleForTesting
452+
internal fun cacheFilenameFrom(displayName: String): String? =
453+
File(displayName).name.takeUnless { it.isEmpty() || it == "." || it == ".." }

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ class ShareActivity : FullComposeScreenLockActivity() {
3232
private val viewModel: ShareViewModel by viewModels()
3333

3434
companion object {
35-
const val EXTRA_ADDRESS = "address"
35+
const val EXTRA_SHARE_TOKEN = "share_token"
3636
}
3737

3838

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package org.thoughtcrime.securesms
2+
3+
import android.net.Uri
4+
import org.session.libsession.utilities.Address
5+
import java.security.SecureRandom
6+
import java.util.Base64
7+
import javax.inject.Inject
8+
import javax.inject.Singleton
9+
10+
/**
11+
* Issues opaque tokens that mark a share Intent as one this app built itself, and carries the
12+
* conversation such an Intent should open.
13+
*
14+
* `ShareActivity` is exported, so the Intent it receives is composed by whichever app invoked the
15+
* share sheet. A token stands in for the destination because it means nothing outside this process,
16+
* where the table that resolves it lives.
17+
*
18+
* A token names the URIs it speaks for rather than merely existing, because the two do not arrive
19+
* together: the system chooser merges a direct-share target's extras into the *sender's* Intent, so
20+
* a token minted here can reach us alongside URIs chosen by another app.
21+
*/
22+
@Singleton
23+
class ShareIntentTokenStore @Inject constructor() {
24+
25+
/**
26+
* Present only for a token this store issued. A null [address] means "no destination chosen",
27+
* and [authorisedUris] is the exact set of our own URIs the Intent carrying it may pass along -
28+
* usually empty.
29+
*/
30+
class Minted(val address: Address?, val authorisedUris: Set<Uri>) {
31+
fun authorises(uri: Uri): Boolean = uri in authorisedUris
32+
}
33+
34+
private val random = SecureRandom()
35+
36+
private val issued = LinkedHashMap<String, Minted>()
37+
38+
@JvmOverloads
39+
@Synchronized
40+
fun mint(address: Address? = null, authorisedUris: Set<Uri> = emptySet()): String {
41+
// A chooser refresh mints one token per conversation, so retention is capped rather than
42+
// left to grow with however many times the share sheet has been opened this process.
43+
while (issued.size >= MAX_RETAINED) {
44+
issued.remove(issued.keys.first())
45+
}
46+
47+
val token = Base64.getUrlEncoder().withoutPadding()
48+
.encodeToString(ByteArray(TOKEN_BYTES).also(random::nextBytes))
49+
50+
issued[token] = Minted(address, authorisedUris)
51+
return token
52+
}
53+
54+
/**
55+
* Resolution deliberately does not retire the token: one user action can create `ShareActivity`
56+
* twice - once before app lock routes it away, once from the Intent the lock screen replays -
57+
* and each instance resolves the Intent independently.
58+
*/
59+
@Synchronized
60+
fun resolve(token: String?): Minted? = token?.let(issued::get)
61+
62+
private companion object {
63+
private const val TOKEN_BYTES = 32
64+
private const val MAX_RETAINED = 512
65+
}
66+
}

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

Lines changed: 52 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
package org.thoughtcrime.securesms
22

3+
import android.content.ContentResolver
34
import android.content.Context
45
import android.content.Intent
56
import android.net.Uri
67
import android.provider.OpenableColumns
7-
import androidx.core.content.IntentCompat
8+
import androidx.annotation.VisibleForTesting
89
import androidx.lifecycle.ViewModel
910
import androidx.lifecycle.viewModelScope
1011
import dagger.hilt.android.lifecycle.HiltViewModel
@@ -35,9 +36,9 @@ import org.thoughtcrime.securesms.mms.PartAuthority
3536
import org.thoughtcrime.securesms.providers.BlobUtils
3637
import org.thoughtcrime.securesms.repository.ConversationRepository
3738
import org.thoughtcrime.securesms.util.AvatarUIData
39+
import org.thoughtcrime.securesms.util.FileProviderUtil
3840
import org.thoughtcrime.securesms.util.AvatarUtils
3941
import org.thoughtcrime.securesms.util.MediaUtil
40-
import java.io.FileInputStream
4142
import java.io.IOException
4243
import javax.inject.Inject
4344

@@ -46,6 +47,7 @@ class ShareViewModel @Inject constructor(
4647
@ApplicationContext private val context: Context,
4748
private val avatarUtils: AvatarUtils,
4849
private val deprecationManager: LegacyGroupDeprecationManager,
50+
private val shareIntentTokenStore: ShareIntentTokenStore,
4951
conversationRepository: ConversationRepository,
5052
): ViewModel(){
5153

@@ -55,6 +57,8 @@ class ShareViewModel @Inject constructor(
5557
private var resolvedPlaintext: CharSequence? = null
5658
private var mimeType: String? = null
5759
private var isPassingAlongMedia = false
60+
private var minted: ShareIntentTokenStore.Minted? = null
61+
private var shareDestination: Address? = null
5862

5963
// Input: The search query
6064
private val mutableSearchQuery = MutableStateFlow("")
@@ -144,6 +148,10 @@ class ShareViewModel @Inject constructor(
144148
mimeType = null
145149
isPassingAlongMedia = false
146150

151+
val minted = shareIntentTokenStore.resolve(intent.getStringExtra(ShareActivity.EXTRA_SHARE_TOKEN))
152+
this.minted = minted
153+
shareDestination = minted?.address
154+
147155
val action = intent.action
148156
val type = intent.type
149157
val incomingUris = ArrayList<Uri>()
@@ -176,50 +184,79 @@ class ShareViewModel @Inject constructor(
176184
isPassingAlongMedia = false
177185
mimeType = getMimeType(uris.firstOrNull(), type)
178186

179-
if (uris.isNotEmpty() && uris.all { PartAuthority.isLocalUri(it) }) {
187+
// A URI naming one of our own providers is passed to the attachment manager verbatim, which
188+
// reads it as us - so it resolves to the viewer's own message history rather than to anything
189+
// the sender holds. Only the exact URIs a token was minted for may take that route: holding a
190+
// token is not enough, because the chooser merges our direct-share extras into the sender's
191+
// own Intent, so a valid token can arrive alongside URIs we never vouched for.
192+
if (minted != null && uris.isNotEmpty() && uris.all { minted.authorises(it) }) {
180193
isPassingAlongMedia = true
181194
resolvedExtras = uris
182-
handleResolvedMedia(intent)
195+
handleResolvedMedia()
183196
} else if (
184197
uris.isEmpty() &&
185198
charSequenceExtra != null &&
186199
(mimeType?.startsWith("text/") == true)
187200
) {
188201
resolvedPlaintext = charSequenceExtra
189-
handleResolvedMedia(intent)
202+
handleResolvedMedia()
190203
} else if (uris.isNotEmpty()) {
191204
_uiState.update { it.copy(showLoader = true) }
192-
resolveMedia(intent, uris)
205+
resolveMedia(uris)
193206
} else {
194207
_uiState.update { it.copy(showLoader = false) }
195208
}
196209
}
197210

198-
private fun handleResolvedMedia(intent: Intent) {
199-
val address = IntentCompat.getParcelableExtra(intent, ShareActivity.EXTRA_ADDRESS, Address::class.java)
211+
private fun handleResolvedMedia() {
212+
val address = shareDestination
200213
if (address is Address.Conversable) {
201214
createConversation(address)
202215
} else {
203216
_uiState.update { it.copy(showLoader = false) }
204217
}
205218
}
206219

207-
private fun resolveMedia(intent: Intent, uris: List<Uri>){
220+
private fun resolveMedia(uris: List<Uri>){
208221
viewModelScope.launch(Dispatchers.Default){
209222
resolvedExtras = uris.mapNotNull { processSingleUri(it) }
210-
handleResolvedMedia(intent)
223+
handleResolvedMedia()
224+
}
225+
}
226+
227+
/**
228+
* Whether a URI offered by whoever sent the share Intent may be opened on their behalf.
229+
*/
230+
@VisibleForTesting
231+
internal fun canReadSharedUri(uri: Uri): Boolean {
232+
// A URI grant is what makes the sender's content readable to us, and only content:// carries
233+
// one. openInputStream also accepts file:// and android.resource://, both of which it opens
234+
// as this app with nothing consulted, so anything this app can reach would be readable by
235+
// whoever sent the Intent.
236+
if (ContentResolver.SCHEME_CONTENT != uri.scheme) {
237+
Log.w(TAG, "Refusing a shared URI that carries no content grant.")
238+
return false
239+
}
240+
241+
// Our own providers answer us whether or not they are exported, so these resolve to our own
242+
// data rather than to anything the sender holds. That covers the attachment and blob
243+
// providers, and equally our FileProvider, whose configured roots include the cache
244+
// directory and external storage.
245+
if (PartAuthority.isLocalUri(uri) || FileProviderUtil.AUTHORITY == uri.authority) {
246+
Log.w(TAG, "Refusing a shared URI that names one of our own providers.")
247+
return false
211248
}
249+
250+
return true
212251
}
213252

214253
private fun processSingleUri(uri: Uri): Uri? {
215254
try {
216255
Log.i(TAG, "Resolving URI: " + uri.toString() + " - " + uri.path)
217256

218-
val inputStream = if ("file" == uri.scheme) {
219-
FileInputStream(uri.path)
220-
} else {
221-
context.contentResolver.openInputStream(uri)
222-
}
257+
if (!canReadSharedUri(uri)) return null
258+
259+
val inputStream = context.contentResolver.openInputStream(uri)
223260

224261
if (inputStream == null) {
225262
Log.w(TAG, "Failed to create input stream during ShareActivity - bailing.")

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

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,9 +1109,9 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
11091109
} else {
11101110
prepMediaForSending(mediaURI, mediaType).addListener(object : ListenableFuture.Listener<Boolean> {
11111111

1112-
override fun onSuccess(result: Boolean?) {
1113-
sendAttachments(attachmentManager.buildSlideDeck().asAttachments(), null)
1114-
}
1112+
// Nothing to do on success: prepMediaForSending stages the attachment in the input
1113+
// bar, and the send is the user's to make once they can see what they shared.
1114+
override fun onSuccess(result: Boolean?) {}
11151115

11161116
override fun onFailure(e: ExecutionException?) {
11171117
Toast.makeText(this@ConversationActivityV2, R.string.attachmentsErrorLoad, Toast.LENGTH_LONG).show()
@@ -2370,7 +2370,14 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
23702370

23712371
viewModel.beforeSendMessage()
23722372

2373-
if (binding.inputBar.linkPreview != null || binding.inputBar.quote != null) {
2373+
if (attachmentManager.isAttachmentPresent()) {
2374+
sendAttachments(
2375+
attachmentManager.buildSlideDeck().asAttachments(),
2376+
getMessageBody(),
2377+
binding.inputBar.quote,
2378+
binding.inputBar.linkPreview
2379+
)
2380+
} else if (binding.inputBar.linkPreview != null || binding.inputBar.quote != null) {
23742381
sendAttachments(listOf(), getMessageBody(), binding.inputBar.quote, binding.inputBar.linkPreview)
23752382
} else {
23762383
sendTextOnlyMessage()
@@ -2612,7 +2619,16 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
26122619
)
26132620
}
26142621

2615-
override fun onAttachmentChanged() { /* Do nothing */ }
2622+
override fun onAttachmentChanged() {
2623+
val slide = attachmentManager.getSlide()
2624+
if (slide != null) binding.inputBar.showAttachmentDraft(glide, slide)
2625+
else binding.inputBar.clearAttachmentDraft()
2626+
}
2627+
2628+
override fun cancelAttachmentDraft() {
2629+
attachmentManager.clear()
2630+
if (isShowingAttachmentOptions) { toggleAttachmentOptions() }
2631+
}
26162632

26172633
override fun onRequestPermissionsResult(requestCode: Int, permissions: Array<out String>, grantResults: IntArray) {
26182634
super.onRequestPermissionsResult(requestCode, permissions, grantResults)
@@ -2633,18 +2649,12 @@ class ConversationActivityV2 : ScreenLockActionBarActivity(), InputBarDelegate,
26332649

26342650
// If the attachment was too large or MediaConstraints.isSatisfied failed for some
26352651
// other reason then we reset the attachment manager & shown buttons then bail..
2652+
// Otherwise it is left staged in the input bar, so that the user can see what they
2653+
// picked, add a message to it, and choose to send.
26362654
if (!result) {
26372655
attachmentManager.clear()
26382656
if (isShowingAttachmentOptions) { toggleAttachmentOptions() }
2639-
return
26402657
}
2641-
2642-
// ..otherwise we can attempt to send the attachment(s).
2643-
// Note: The only multi-attachment message type is when sending images - all others
2644-
// attempt send the attachment immediately upon file selection.
2645-
sendAttachments(attachmentManager.buildSlideDeck().asAttachments(), null)
2646-
//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
2647-
//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.
26482658
}
26492659

26502660
override fun onFailure(e: ExecutionException?) {

0 commit comments

Comments
 (0)