From f7dd3414cdf12582cd571de58060347ba57e1ce8 Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Mon, 3 Aug 2026 16:01:41 -0300 Subject: [PATCH] Persist the share data watermark so a restored conversation can't replay a share. --- .../thoughtcrime/securesms/MainActivity.kt | 4 - .../conversation/drafts/DraftRepository.kt | 76 ++++++---- .../conversation/drafts/DraftViewModel.kt | 4 +- .../conversation/v2/ConversationActivity.kt | 2 - .../conversation/v2/ConversationFragment.kt | 6 +- .../v2/ShareDataTimestampViewModel.kt | 39 ----- .../securesms/keyvalue/MiscellaneousValues.kt | 11 ++ .../drafts/DraftRepositoryTest.kt | 136 ++++++++++++++++++ 8 files changed, 197 insertions(+), 81 deletions(-) delete mode 100644 app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ShareDataTimestampViewModel.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepositoryTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/MainActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/MainActivity.kt index 6a5386fa8c..61b5063f0d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/MainActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/MainActivity.kt @@ -134,7 +134,6 @@ import org.thoughtcrime.securesms.components.voice.VoiceNoteMediaControllerOwner import org.thoughtcrime.securesms.conversation.ConversationIntents import org.thoughtcrime.securesms.conversation.NewConversationActivity import org.thoughtcrime.securesms.conversation.v2.MotionEventRelay -import org.thoughtcrime.securesms.conversation.v2.ShareDataTimestampViewModel import org.thoughtcrime.securesms.conversationlist.ConversationListArchiveFragment import org.thoughtcrime.securesms.conversationlist.ConversationListFragment import org.thoughtcrime.securesms.conversationlist.RelinkDevicesReminderBottomSheetFragment @@ -257,7 +256,6 @@ class MainActivity : private val toolbarViewModel: MainToolbarViewModel by viewModels() private val toolbarCallback = ToolbarCallback() - private val shareDataTimestampViewModel: ShareDataTimestampViewModel by viewModels() private val motionEventRelay: MotionEventRelay by viewModels() @@ -382,8 +380,6 @@ class MainActivity : } } - shareDataTimestampViewModel.setTimestampFromActivityCreation(savedInstanceState, intent) - setContent { val mainToolbarState by toolbarViewModel.state.collectAsStateWithLifecycle() val megaphone by mainNavigationViewModel.megaphone.collectAsStateWithLifecycle() diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepository.kt index 7ac7524064..48bae19001 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepository.kt @@ -33,6 +33,7 @@ import org.thoughtcrime.securesms.database.model.databaseprotos.BodyRangeList import org.thoughtcrime.securesms.database.withAttachments import org.thoughtcrime.securesms.dependencies.AppDependencies import org.thoughtcrime.securesms.keyboard.KeyboardUtil +import org.thoughtcrime.securesms.keyvalue.SignalStore import org.thoughtcrime.securesms.mms.GifSlide import org.thoughtcrime.securesms.mms.ImageSlide import org.thoughtcrime.securesms.mms.PartAuthority @@ -60,8 +61,8 @@ class DraftRepository( val TAG = Log.tag(DraftRepository::class.java) } - fun getShareOrDraftData(lastShareDataTimestamp: Long): Maybe> { - return MaybeCompat.fromCallable { getShareOrDraftDataInternal(lastShareDataTimestamp) } + fun getShareOrDraftData(): Maybe> { + return MaybeCompat.fromCallable { getShareOrDraftDataInternal() } .observeOn(Schedulers.io()) } @@ -72,16 +73,21 @@ class DraftRepository( * Note: Voice note drafts are handled differently and via the [DraftViewModel.state] */ @Suppress("ConvertTwoComparisonsToRangeCheck") - private fun getShareOrDraftDataInternal(lastShareDataTimestamp: Long): Pair? { + private fun getShareOrDraftDataInternal(): Pair? { val sharedDataTimestamp: Long = conversationArguments?.shareDataTimestamp ?: -1 + val lastShareDataTimestamp: Long = SignalStore.misc.lastProcessedShareDataTimestamp Log.d(TAG, "Shared this data at $sharedDataTimestamp and last processed share data at $lastShareDataTimestamp") if (sharedDataTimestamp > 0 && sharedDataTimestamp <= lastShareDataTimestamp) { - Log.d(TAG, "Already processed this share data. Skipping.") - return null + Log.d(TAG, "Already processed this share data. Falling back to database drafts.") + return loadDatabaseDraftData() } else { Log.d(TAG, "Have not processed this share data. Proceeding.") } + if (sharedDataTimestamp > 0) { + SignalStore.misc.lastProcessedShareDataTimestamp = sharedDataTimestamp + } + val shareText = conversationArguments?.draftText val shareMedia = conversationArguments?.draftMedia val shareContentType = conversationArguments?.draftContentType @@ -127,36 +133,48 @@ class DraftRepository( } if (conversationArguments?.canInitializeFromDatabase() == true) { - val (drafts, updatedText) = loadDraftsInternal(conversationArguments.threadId) - - val draftText: CharSequence? = drafts.firstOrNull { it.type == DraftTable.Draft.TEXT }?.let { updatedText ?: it.value } - - val messageEdit: ConversationMessage? = drafts.firstOrNull { it.type == DraftTable.Draft.MESSAGE_EDIT }?.let { loadDraftMessageEditInternal(it.value) } - if (messageEdit != null) { - return ShareOrDraftData.SetEditMessage(messageEdit, draftText, clearQuote = drafts.none { it.type == DraftTable.Draft.QUOTE }) to drafts - } - - val location: SignalPlace? = drafts.firstOrNull { it.type == DraftTable.Draft.LOCATION }?.let { SignalPlace.deserialize(it.value) } - if (location != null) { - return ShareOrDraftData.SetLocation(location, draftText) to drafts - } - - val quote: ConversationMessage? = drafts.firstOrNull { it.type == DraftTable.Draft.QUOTE }?.let { loadDraftQuoteInternal(it.value) } - if (quote != null) { - return ShareOrDraftData.SetQuote(quote, draftText) to drafts - } - - if (draftText != null) { - return ShareOrDraftData.SetText(draftText) to drafts - } - - return null to drafts + return loadDatabaseDraftData() } // no share or draft return null } + /** + * Loads the drafts stored for this conversation, ignoring any share payload in [conversationArguments]. + */ + private fun loadDatabaseDraftData(): Pair? { + val threadId: Long = conversationArguments?.threadId ?: -1 + if (threadId <= 0) { + return null + } + + val (drafts, updatedText) = loadDraftsInternal(threadId) + + val draftText: CharSequence? = drafts.firstOrNull { it.type == DraftTable.Draft.TEXT }?.let { updatedText ?: it.value } + + val messageEdit: ConversationMessage? = drafts.firstOrNull { it.type == DraftTable.Draft.MESSAGE_EDIT }?.let { loadDraftMessageEditInternal(it.value) } + if (messageEdit != null) { + return ShareOrDraftData.SetEditMessage(messageEdit, draftText, clearQuote = drafts.none { it.type == DraftTable.Draft.QUOTE }) to drafts + } + + val location: SignalPlace? = drafts.firstOrNull { it.type == DraftTable.Draft.LOCATION }?.let { SignalPlace.deserialize(it.value) } + if (location != null) { + return ShareOrDraftData.SetLocation(location, draftText) to drafts + } + + val quote: ConversationMessage? = drafts.firstOrNull { it.type == DraftTable.Draft.QUOTE }?.let { loadDraftQuoteInternal(it.value) } + if (quote != null) { + return ShareOrDraftData.SetQuote(quote, draftText) to drafts + } + + if (draftText != null) { + return ShareOrDraftData.SetText(draftText) to drafts + } + + return null to drafts + } + fun deleteVoiceNoteDraftData(draft: DraftTable.Draft?) { if (draft != null) { SignalExecutors.BOUNDED.execute { diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftViewModel.kt index a706e53ff9..01cd1fbaef 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/drafts/DraftViewModel.kt @@ -132,8 +132,8 @@ class DraftViewModel @JvmOverloads constructor( return newState } - fun loadShareOrDraftData(lastShareDataTimestamp: Long): Maybe { - return repository.getShareOrDraftData(lastShareDataTimestamp) + fun loadShareOrDraftData(): Maybe { + return repository.getShareOrDraftData() .doOnSuccess { (_, drafts) -> if (drafts != null) { store.update { saveDraftsIfChanged(it, it.copyAndSetDrafts(drafts = drafts)) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivity.kt index bcc4d7e877..2f428a0d88 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivity.kt @@ -51,7 +51,6 @@ open class ConversationActivity : PassphraseRequiredActivity(), VoiceNoteMediaCo override val googlePayResultPublisher: Subject = PublishSubject.create() private val motionEventRelay: MotionEventRelay by viewModels() - private val shareDataTimestampViewModel: ShareDataTimestampViewModel by viewModels() override fun onPreCreate() { theme.onCreate(this) @@ -79,7 +78,6 @@ open class ConversationActivity : PassphraseRequiredActivity(), VoiceNoteMediaCo transitionDebouncer.publish { supportStartPostponedEnterTransition() } window.requestFeature(Window.FEATURE_ACTIVITY_TRANSITIONS) - shareDataTimestampViewModel.setTimestampFromActivityCreation(savedInstanceState, intent) setContentView(R.layout.fragment_container) if (savedInstanceState == null) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt index 09d4f6c815..0b3aafbb4c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationFragment.kt @@ -540,8 +540,6 @@ class ConversationFragment : InlineQueryViewModelV2(conversationRecipientRepository) } - private val shareDataTimestampViewModel: ShareDataTimestampViewModel by activityViewModels() - private val mainNavigationViewModel: MainNavigationViewModel by activityViewModels { MainNavigationViewModel.Factory() } private val inlineQueryController: InlineQueryResultsControllerV2 by lazy { @@ -1450,7 +1448,7 @@ class ConversationFragment : .inputReadyState .take(1) .flatMapMaybe { inputReadyState -> - draftViewModel.loadShareOrDraftData(shareDataTimestampViewModel.timestamp) + draftViewModel.loadShareOrDraftData() .map { inputReadyState to it } } .subscribeBy { (inputReadyState, data) -> handleShareOrDraftData(inputReadyState, data) } @@ -2165,8 +2163,6 @@ class ConversationFragment : } private fun handleShareOrDraftData(inputReadyState: InputReadyState, data: ShareOrDraftData) { - shareDataTimestampViewModel.setTimestampFromConversationArgs(args) - if (inputReadyState.isAnnouncementGroup == true && inputReadyState.isAdmin == false) { Toast.makeText(requireContext(), R.string.MultiselectForwardFragment__only_admins_can_send_messages_to_this_group, Toast.LENGTH_SHORT).show() draftViewModel.clearDraft() diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ShareDataTimestampViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ShareDataTimestampViewModel.kt deleted file mode 100644 index a2cb7dde54..0000000000 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ShareDataTimestampViewModel.kt +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Copyright 2023 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.thoughtcrime.securesms.conversation.v2 - -import android.content.Intent -import android.os.Bundle -import androidx.lifecycle.SavedStateHandle -import androidx.lifecycle.ViewModel -import org.thoughtcrime.securesms.conversation.ConversationArgs -import org.thoughtcrime.securesms.util.delegate - -/** - * Hold the last share timestamp in an activity scoped view model for sharing between - * the activity and fragments. - */ -class ShareDataTimestampViewModel( - savedStateHandle: SavedStateHandle -) : ViewModel() { - - companion object { - private const val TIMESTAMP = "timestamp" - } - - var timestamp: Long by savedStateHandle.delegate(TIMESTAMP, -1L) - private set - - fun setTimestampFromActivityCreation(savedInstanceState: Bundle?, intent: Intent) { - if (savedInstanceState == null && intent.flags and Intent.FLAG_ACTIVITY_LAUNCHED_FROM_HISTORY != 0) { - timestamp = System.currentTimeMillis() - } - } - - fun setTimestampFromConversationArgs(args: ConversationArgs) { - timestamp = args.shareDataTimestamp - } -} diff --git a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.kt b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.kt index c9afb24813..fa6a52f214 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.kt @@ -55,6 +55,7 @@ class MiscellaneousValues internal constructor(store: KeyValueStore) : SignalSto private const val LAST_SYNC_MESSAGE_SEEN_TIME_MS = "misc.last_sync_message_seen_time" private const val LAST_APPLIED_PNI_CHANGE_SERVER_TIMESTAMP = "misc.last_applied_pni_change_server_timestamp" private const val LAST_MISSING_PLAY_SERVICES_FCM_VERIFICATION_TIME = "misc.last_missing_play_services_fcm_verification_time" + private const val LAST_PROCESSED_SHARE_DATA_TIMESTAMP = "misc.last_processed_share_data_timestamp" } public override fun onFirstEverAppLaunch() { @@ -360,4 +361,14 @@ class MiscellaneousValues internal constructor(store: KeyValueStore) : SignalSto * The last time we tried to get an FCM token for a user reporting missing Play Services. */ var lastMissingPlayServicesFcmVerificationTime: Long by longValue(LAST_MISSING_PLAY_SERVICES_FCM_VERIFICATION_TIME, 0) + + /** + * High-water mark of the most recently consumed share payload, as set by + * [org.thoughtcrime.securesms.conversation.ConversationArgs.shareDataTimestamp]. Any share whose timestamp is at or + * below this has already been handed off to the conversation and must never be replayed. + * + * This has to outlive the navigation entry carrying the payload, which is persisted and can be restored long after + * the activity that consumed it is gone. + */ + var lastProcessedShareDataTimestamp: Long by longValue(LAST_PROCESSED_SHARE_DATA_TIMESTAMP, -1) } diff --git a/app/src/test/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepositoryTest.kt b/app/src/test/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepositoryTest.kt new file mode 100644 index 0000000000..bed846eedd --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/conversation/drafts/DraftRepositoryTest.kt @@ -0,0 +1,136 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.conversation.drafts + +import android.app.Application +import io.mockk.every +import io.mockk.mockk +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.signal.core.util.logging.Log +import org.thoughtcrime.securesms.conversation.ConversationArgs +import org.thoughtcrime.securesms.conversation.ConversationIntents.ConversationScreenType +import org.thoughtcrime.securesms.database.DraftTable +import org.thoughtcrime.securesms.database.ThreadTable +import org.thoughtcrime.securesms.keyvalue.SignalStore +import org.thoughtcrime.securesms.recipients.RecipientId +import org.thoughtcrime.securesms.testutil.DirectExecutor +import org.thoughtcrime.securesms.testutil.SignalStoreRule +import org.thoughtcrime.securesms.testutil.SystemOutLogger + +/** + * Verifies that a share payload is handed to the conversation exactly once, using the durable marker in + * [org.thoughtcrime.securesms.keyvalue.MiscellaneousValues.lastProcessedShareDataTimestamp]. + */ +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, application = Application::class) +class DraftRepositoryTest { + + companion object { + private const val THREAD_ID = 1L + } + + @get:Rule + val signalStore = SignalStoreRule() + + private lateinit var draftTable: DraftTable + + @Before + fun setUp() { + Log.initialize(SystemOutLogger()) + + draftTable = mockk(relaxed = true) + every { draftTable.getDrafts(any()) } returns DraftTable.Drafts() + } + + @Test + fun `unprocessed share is delivered and advances the marker`() { + val result = repository(shareDataTimestamp = 1000L, draftText = "shared text").load() + + assertEquals(DraftRepository.ShareOrDraftData.SetText("shared text"), result?.first) + assertEquals(1000L, SignalStore.misc.lastProcessedShareDataTimestamp) + } + + @Test + fun `share is not delivered a second time`() { + SignalStore.misc.lastProcessedShareDataTimestamp = 1000L + + val result = repository(shareDataTimestamp = 1000L, draftText = "shared text").load() + + assertNull(result?.first) + assertEquals(1000L, SignalStore.misc.lastProcessedShareDataTimestamp) + } + + /** AND-9817: a restored navigation entry can carry a share that is older than the last one we consumed. */ + @Test + fun `share older than the marker is not delivered`() { + SignalStore.misc.lastProcessedShareDataTimestamp = 2000L + + val result = repository(shareDataTimestamp = 1000L, draftText = "shared text").load() + + assertNull(result?.first) + assertEquals(2000L, SignalStore.misc.lastProcessedShareDataTimestamp) + } + + @Test + fun `skipped share still loads database drafts`() { + SignalStore.misc.lastProcessedShareDataTimestamp = 1000L + every { draftTable.getDrafts(THREAD_ID) } returns DraftTable.Drafts(listOf(DraftTable.Draft(DraftTable.Draft.TEXT, "saved draft"))) + + val result = repository(shareDataTimestamp = 1000L, draftText = "shared text").load() + + assertEquals(DraftRepository.ShareOrDraftData.SetText("saved draft"), result?.first) + } + + @Test + fun `opening a conversation without share data does not reset the marker`() { + SignalStore.misc.lastProcessedShareDataTimestamp = 2000L + + repository(shareDataTimestamp = -1L, draftText = null).load() + + assertEquals(2000L, SignalStore.misc.lastProcessedShareDataTimestamp) + } + + private fun DraftRepository.load(): Pair? { + return getShareOrDraftData().blockingGet() + } + + private fun repository(shareDataTimestamp: Long, draftText: String?): DraftRepository { + return DraftRepository( + context = mockk(relaxed = true), + threadTable = mockk(relaxed = true), + draftTable = draftTable, + saveDraftsExecutor = DirectExecutor(), + conversationArguments = conversationArgs(shareDataTimestamp, draftText) + ) + } + + private fun conversationArgs(shareDataTimestamp: Long, draftText: String?): ConversationArgs { + return ConversationArgs( + recipientId = RecipientId.from(1L), + threadId = THREAD_ID, + draftText = draftText, + draftMedia = null, + draftContentType = null, + media = null, + stickerLocator = null, + isBorderless = false, + distributionType = ThreadTable.DistributionTypes.DEFAULT, + startingPosition = -1, + isFirstTimeInSelfCreatedGroup = false, + isWithSearchOpen = false, + giftBadge = null, + shareDataTimestamp = shareDataTimestamp, + conversationScreenType = ConversationScreenType.NORMAL + ) + } +}