Share: stage attachments instead of sending on selection, and resolve only content:// URIs - #2222
Merged
Merged
Conversation
…e attachments - ShareActivity resolves only content:// URIs, and not ones naming our own providers; other schemes are refused rather than opened directly. - The share destination travels as a token minted by DirectShareService and MediaPreviewActivity rather than as a parcelled Address in the Intent, and an Address in the Intent is no longer read. Direct share is unchanged on the API levels where it still runs: ChooserTargetService stopped being consulted at API 31, so that is 26-30. - The filename a shared file is cached under is reduced to a single path segment before being joined to cacheDir, and the app-lock cache path resolves content:// URIs only. - An attachment that is shared in, or picked as a document or a GIF, now waits in the input bar with send armed instead of being sent the moment it is chosen, which the document picker had a TODO asking for. Picking from the library or the camera is unaffected - it already went through the editor.
…heir URIs Follows review on the previous commit. - A staged attachment had no way to be seen or sent: the input bar drew nothing for it, the send button only appeared once there was text, and sendMessage never consulted the attachment manager. A shared or picked file was therefore discarded. It now shows in the input bar with its name, size and a control to drop it again, the send button arms for an attachment as well as for text, and send includes it along with any message typed alongside. - A share token now names the URIs it speaks for rather than merely existing. The chooser merges a direct-share target's extras into the sender's own Intent, so a token minted here can arrive attached to URIs chosen by another app. - Our own FileProvider is refused on the same footing as the attachment and blob providers, on both the share and app-lock paths; its configured roots include the cache directory and external storage. - Name the blob a Giphy result is written to. Without one it is named "null" verbatim, which the attachment then carries to the recipient.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Share: stage attachments instead of sending on selection, and resolve only content:// URIs
Changes in the share-intent and attachment path. The first is the only user-visible one.
Attachments are staged rather than sent on selection
Sharing a document into Session, or picking one with + → File, sent it the moment a conversation
was chosen — no preview, no chance to add a message, no confirmation. It now waits in the input bar
with its name, size and a control to drop it again, and sends when you say so, with whatever message
you typed alongside it.
ConversationActivityV2had a TODO asking for exactly this.GIFs picked from Giphy behave the same way, since they shared the listener. Picking from the library
and taking a photo are unchanged — those already went through the media editor.
This needed three pieces that were not there before: the input bar had nothing to draw a staged
attachment with, the send button only appeared once there was text, and
sendMessagedid not consultthe attachment manager. So
AttachmentDraftViewjoins the quote and link-preview drafts in the inputbar's additional-content container, the send button arms for a staged attachment as well as for text,
and send includes it.
This is the part worth arguing about, and it is a product decision rather than a tidy-up.
The share path resolves
content://URIs onlyShareActivityis exported, so the URIs in the Intent it receives come from whichever app invoked theshare sheet.
ContentResolver.openInputStreamaccepts three schemes —content://,file://andandroid.resource://— and onlycontent://involves a URI grant; the other two it opens directly asus. The share path wants the granted case, so it resolves that one and declines the rest. The copy the
app-lock path makes of incoming shared files does the same.
Nothing we ship sends us anything else.
MediaPreviewActivity.forward()isShareActivity's onlyin-app caller and passes a
content://attachment URI, and a sending app targeting API 24 or abovegets
FileUriExposedExceptionin its own process for afile://extra.URIs naming our own providers are passed along only for an Intent we built
ShareViewModelhands a URI matchingPartAuthority.isLocalUristraight to the attachment managerrather than resolving it. That is right for
MediaPreviewActivity.forward(), which forwards anattachment we already hold, and is not right for an Intent that arrived from outside — our own
providers answer us whether or not they are exported, so such a URI resolves against our own data.
That branch is now taken only for the exact URIs a token was minted for, rather than for any Intent
carrying a token: the system chooser merges a direct-share target's extras into the sender's Intent,
so a token we minted can arrive attached to URIs another app chose. The resolve path declines these
URIs too, and our own FileProvider is refused on the same footing — its configured roots include the
cache directory and external storage.
The share destination travels as a token
DirectShareServiceput the targetAddressinto theChooserTargetbundle andShareViewModelreadit back out of the incoming Intent, so the destination was carried in the Intent itself. It now mints
an opaque token per target and holds
token -> Addressin memory;ShareViewModelresolves the tokenand no longer reads an
Addressfrom the Intent.MediaPreviewActivity.forward()mints a token forthe URI it is forwarding and no destination, so it still reaches the contact picker.
Direct share behaves exactly as before. Worth knowing for scope:
ChooserTargetServicewas deprecatedat API 30 and Android stopped consulting it at API 31, so with
minSdk = 26this code path is live onAPI 26–30 only. Migrating to
ShortcutManagersharing shortcuts would restore direct share on 12+, butthat is its own piece of work.
Cached share filenames are reduced to a single path segment
The name the app-lock path caches a shared file under is the sending app's
OpenableColumns.DISPLAY_NAMEverbatim. That is a display name, not a path segment, so it is now reduced to its last segment before
being joined to
cacheDir, and a name that reduces to nothing usable (.,.., empty) is declined.Fixed at the call site rather than in
FilenameUtils.getFilenameFromUri, whose many other callers wanta display name and not a path segment.
A Giphy result is given a filename
GiphyActivitywrote the blob without one, so the attachment was namednullverbatim — and carriedthat to the recipient. Invisible until now, because a GIF renders as an image rather than by name; the
input bar draft is what surfaced it.
Two decisions a reviewer may want to push back on
ShareActivitytwice — once before
routeApplicationStatesends it to the lock screen, once from the Intent the lockscreen replays — and each instance resolves the Intent independently, so a single-use token would be
spent before the user ever authenticates. Retention is capped at 512 entries, oldest first, instead.
Testing
20 unit tests across
ShareViewModelTest,ShareIntentTokenStoreTestandShareIntentCacheFilenameTest. Full suite: 320 tests, 0 failures, 0 skipped.The staging behaviour cannot be unit tested here —
ConversationActivityV2is not constructible in theJVM suite — so it was exercised on an emulator (API 37,
playdebug), each step confirmed on screen: