From b083c7dad3228843611252db0bb91a9f52687ff2 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 24 Aug 2026 12:09:13 +1000 Subject: [PATCH 1/3] App lock: cover the app instead of destroying it, and let debug builds lock at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects that only show up together. - asked for ACCESS_SESSION_SECRETS without the ${authority_postfix} the declaration carries, so on debug builds — the only build type with a non-empty postfix — the app did not hold the permission its own CLEAR_KEY_EVENT broadcast requires and the broadcast was silently dropped. App lock has therefore never auto-locked anything in a debug build, which is why this was invisible locally. - onMasterSecretCleared() finished the activity when the app was not visible. Every ScreenLockActionBarActivity in the process received the broadcast, so the whole task emptied — including a conversation waiting on a picker result. Attaching a File with app lock on dropped the user out of Session and discarded the file with no error. The lock is now presented over the activity and onStart re-checks on return, so the task and its pending result survive. Nothing was traded away to do this: the timeout stays 0, masterSecret is a sentinel Object rather than a key so finishing evicted nothing, and BaseActionBarActivity already applies FLAG_SECURE in onPause, so a backgrounded conversation was never in the recents thumbnail whether or not it was finished. Cold start while locked still routes-and-finishes via next_intent; only the return path changed. Verified on a Pixel 6 / API 34 emulator: attach File with the lock on now returns to the same conversation behind the lock screen and sends the file; Home-then-reopen and a force-stop cold start both still demand authentication. --- app/src/main/AndroidManifest.xml | 2 +- .../securesms/ScreenLockActionBarActivity.kt | 39 ++++++++++++++++--- .../securesms/ScreenLockActivity.kt | 11 ++---- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 3d3ce25040..2a6f7bb80b 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -66,7 +66,7 @@ - + diff --git a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt index 9952eb3359..afa56e9de7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt @@ -80,6 +80,7 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { } private var clearKeyReceiver: BroadcastReceiver? = null + private var hasStarted = false @Inject lateinit var loginStateRepository: LoginStateRepository @@ -95,9 +96,7 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { super.onCreate(savedInstanceState) - val locked = KeyCachingService.isLocked(this) && isScreenLockEnabled(this) && - loginStateRepository.peekLoginState() != null - routeApplicationState(locked) + routeApplicationState(isScreenLocked()) if (!isFinishing) { initializeClearKeyReceiver() @@ -107,6 +106,34 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { protected open fun onCreate(savedInstanceState: Bundle?, ready: Boolean) {} + override fun onStart() { + super.onStart() + + // Only re-locks an activity that is coming BACK to the foreground; a first start while locked + // is already handled by onCreate's routing. The distinction matters because the lock can be + // cleared while this activity is merely stopped, which happens on any excursion to another app + // - a file picker, the camera, the share sheet. Covering it here rather than destroying it is + // what lets such an excursion return to the state it left, including a pending + // onActivityResult. + val isReturningToForeground = hasStarted + hasStarted = true + + if (isReturningToForeground && isScreenLocked()) presentScreenLock() + } + + private fun isScreenLocked(): Boolean = + KeyCachingService.isLocked(this) && isScreenLockEnabled(this) && + loginStateRepository.peekLoginState() != null + + // Covers this activity with the lock screen WITHOUT finishing it, so the task and any pending + // activity result survive. No "next_intent" is supplied because there is nothing to route back to + // - this activity is still underneath, and ScreenLockActivity finishes itself on success to reveal + // it. ScreenLockActivity is singleInstancePerTask, so concurrent calls from several live activities + // reuse the one instance. + private fun presentScreenLock() { + startActivity(Intent(this, ScreenLockActivity::class.java)) + } + override fun onPause() { Log.i(TAG, "onPause()") super.onPause() @@ -120,8 +147,10 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { fun onMasterSecretCleared() { Log.i(TAG, "onMasterSecretCleared()") - if (appVisibilityManager.isAppVisible.value) routeApplicationState(true) - else finish() + + // Nothing to cover while backgrounded, and finishing would discard whatever the user was in + // the middle of - onStart presents the lock when they come back instead. + if (appVisibilityManager.isAppVisible.value) presentScreenLock() } protected fun initFragment(@IdRes target: Int, fragment: T): T? { diff --git a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt index 771bdfe9dc..7249956804 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt @@ -281,13 +281,10 @@ class ScreenLockActivity : BaseActionBarActivity() { keyCachingService?.setMasterSecret(Any()) // The 'nextIntent' will take us to the MainActivity if this is a standard unlock, or it will - // take us to the ShareActivity if this is an external share. - val nextIntent = intent.getParcelableExtra("next_intent") - if (nextIntent == null) { - Log.w(TAG, "Got a null nextIntent - cannot proceed.") - } else { - startActivity(nextIntent) - } + // take us to the ShareActivity if this is an external share. It is absent when this lock was + // presented OVER a still-live activity (see ScreenLockActionBarActivity.presentScreenLock), in + // which case finishing is all that is needed to return to it. + intent.getParcelableExtra("next_intent")?.let(::startActivity) finish() } From c09c285c5ad5785bdbbbd705c1c530fd313444ef Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 24 Aug 2026 15:17:56 +1000 Subject: [PATCH 2/3] =?UTF-8?q?App=20lock:=20address=20review=20=E2=80=94?= =?UTF-8?q?=20cancel=20must=20leave=20the=20app,=20and=20survive=20a=20rec?= =?UTF-8?q?reation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review on this branch. - Dismissing the lock without authenticating revealed the activity underneath, whose onStart presented the lock straight back: a loop escapable only via Home, and one that spun with no user input at all during a biometric lockout, flashing the covered content on each pass. Every non-authenticating exit now leaves the app. Note ScreenLockActivity is singleInstancePerTask, so it occupies its OWN task above the app's — backgrounding just this task reveals the app again, so it goes to the launcher. Back is routed through the same path; it previously bypassed it entirely. - A configuration change while stopped recreated the activity, and onCreate's route-and-finish path then discarded the pending picker result — the reported bug, one step removed. hasStarted is now saved/restored and the locked route is suppressed on a recreation, so onStart covers it instead. - onMasterSecretCleared now applies the same isScreenLocked() guard onStart uses. KeyCachingService.onDestroy broadcasts CLEAR_KEY_EVENT whether or not app lock is on, and ScreenLockActivity only prompts when the setting is on — so without the guard that broadcast could raise a lock screen with no way to authenticate past it. - onNewIntent no longer adopts an intent that carries no next_intent, which would discard the destination a routed unlock still has to reach. - Restored the warning for a next_intent that has genuinely gone missing, distinguished from the expected-absent case by an explicit extra rather than by null alone. Verified on a Pixel 6 / API 34 emulator: rotating the device while the picker is open now keeps the attachment (onCreate logs has_started=true, the locked route is skipped, and the file still sends); Back from a lock covering a live conversation lands on the launcher with exactly one lock screen created. --- .../securesms/ScreenLockActionBarActivity.kt | 45 +++++++++--- .../securesms/ScreenLockActivity.kt | 68 ++++++++++++++++--- 2 files changed, 95 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt index afa56e9de7..4aae429d06 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActionBarActivity.kt @@ -46,6 +46,8 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { private const val STATE_WELCOME_SCREEN = 3 private const val STATE_DATABASE_MIGRATE = 4 // This is different from STATE_UPGRADE_DATABASE as it is used to migrate database in a whole rather than the internal db schema upgrades + private const val KEY_HAS_STARTED = "has_started" + private fun getStateName(state: Int): String { return when (state) { STATE_NORMAL -> "STATE_NORMAL" @@ -81,6 +83,7 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { private var clearKeyReceiver: BroadcastReceiver? = null private var hasStarted = false + private var wasRecreated = false @Inject lateinit var loginStateRepository: LoginStateRepository @@ -96,6 +99,11 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { super.onCreate(savedInstanceState) + // Captured once, before routing: routeApplicationState is a coroutine, so reading hasStarted + // from inside it would race with onStart setting it and misread a cold start as a recreation. + wasRecreated = (savedInstanceState != null) + hasStarted = savedInstanceState?.getBoolean(KEY_HAS_STARTED) == true + routeApplicationState(isScreenLocked()) if (!isFinishing) { @@ -106,6 +114,11 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { protected open fun onCreate(savedInstanceState: Bundle?, ready: Boolean) {} + override fun onSaveInstanceState(outState: Bundle) { + super.onSaveInstanceState(outState) + outState.putBoolean(KEY_HAS_STARTED, hasStarted) + } + override fun onStart() { super.onStart() @@ -125,13 +138,19 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { KeyCachingService.isLocked(this) && isScreenLockEnabled(this) && loginStateRepository.peekLoginState() != null - // Covers this activity with the lock screen WITHOUT finishing it, so the task and any pending + // Puts the lock screen in front of this activity WITHOUT finishing it, so the task and any pending // activity result survive. No "next_intent" is supplied because there is nothing to route back to - // - this activity is still underneath, and ScreenLockActivity finishes itself on success to reveal - // it. ScreenLockActivity is singleInstancePerTask, so concurrent calls from several live activities - // reuse the one instance. + // - this activity is still here, and ScreenLockActivity finishes itself on success to reveal it. + // + // ScreenLockActivity is singleInstancePerTask, so it occupies its own task ABOVE this one rather + // than stacking in it: concurrent calls from several live activities reuse the one instance, and + // dismissing it without authenticating reveals this activity again - which is why it has to send + // the user to the launcher instead (see ScreenLockActivity.leaveAppLocked). private fun presentScreenLock() { - startActivity(Intent(this, ScreenLockActivity::class.java)) + startActivity( + Intent(this, ScreenLockActivity::class.java) + .putExtra(ScreenLockActivity.EXTRA_PRESENTED_OVER, true) + ) } override fun onPause() { @@ -148,9 +167,14 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { fun onMasterSecretCleared() { Log.i(TAG, "onMasterSecretCleared()") - // Nothing to cover while backgrounded, and finishing would discard whatever the user was in - // the middle of - onStart presents the lock when they come back instead. - if (appVisibilityManager.isAppVisible.value) presentScreenLock() + // KeyCachingService.onDestroy broadcasts this whether or not app lock is enabled, so the same + // isScreenLocked() check onStart uses is needed here too - without it the broadcast can raise a + // lock screen that never prompts for anything, because ScreenLockActivity only starts an + // authentication when the setting is on. + // + // Nothing to cover while backgrounded either, and finishing would discard whatever the user was + // in the middle of - onStart presents the lock when they come back instead. + if (appVisibilityManager.isAppVisible.value && isScreenLocked()) presentScreenLock() } protected fun initFragment(@IdRes target: Int, fragment: T): T? { @@ -194,7 +218,10 @@ abstract class ScreenLockActionBarActivity : BaseActionBarActivity() { Log.i(TAG, "routeApplicationState() - ${getStateName(state)}") return when (state) { - STATE_SCREEN_LOCKED -> getScreenUnlockIntent() // Note: This is a suspend function + // Routing finishes this activity, which on a recreation would discard a pending activity + // result - a picker excursion survives a rotation only if the activity waiting on it does. + // A recreated instance is covered by onStart instead, via the restored hasStarted. + STATE_SCREEN_LOCKED -> if (wasRecreated) null else getScreenUnlockIntent() // Note: This is a suspend function STATE_UPGRADE_DATABASE -> getUpgradeDatabaseIntent() STATE_WELCOME_SCREEN -> getWelcomeIntent() STATE_DATABASE_MIGRATE -> getRoutedIntent(DatabaseMigrationStateActivity::class.java, getConversationListIntent()) diff --git a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt index 7249956804..9194752d0d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/ScreenLockActivity.kt @@ -30,6 +30,7 @@ import android.view.animation.TranslateAnimation import android.widget.ImageView import android.widget.TextView import android.widget.Toast +import androidx.activity.OnBackPressedCallback import androidx.biometric.BiometricPrompt import androidx.biometric.BiometricManager import androidx.core.content.ContextCompat @@ -47,6 +48,17 @@ import org.thoughtcrime.securesms.service.KeyCachingService import org.thoughtcrime.securesms.service.KeyCachingService.KeySetBinder class ScreenLockActivity : BaseActionBarActivity() { + companion object { + /** + * Marks a lock that was presented over a still-live activity, and which therefore carries no + * [EXTRA_NEXT_INTENT]. Without it there is no way to tell that case apart from a routed unlock + * whose destination has gone missing, and the second one has to stay reportable. + */ + const val EXTRA_PRESENTED_OVER: String = "presented_over" + + private const val EXTRA_NEXT_INTENT: String = "next_intent" + } + private val TAG: String = ScreenLockActivity::class.java.simpleName private lateinit var fingerprintPrompt: ImageView @@ -74,6 +86,14 @@ class ScreenLockActivity : BaseActionBarActivity() { setContentView(R.layout.screen_lock_activity) initializeResources() + // Back must leave the app, not just dismiss the lock: the activity underneath is still alive and + // would re-present it immediately. + onBackPressedDispatcher.addCallback(this, object : OnBackPressedCallback(true) { + override fun handleOnBackPressed() { + leaveAppLocked() + } + }) + // Start and bind to the KeyCachingService instance. val bindIntent = Intent(this, KeyCachingService::class.java) startService(bindIntent) @@ -101,14 +121,14 @@ class ScreenLockActivity : BaseActionBarActivity() { BiometricPrompt.ERROR_NEGATIVE_BUTTON, BiometricPrompt.ERROR_USER_CANCELED -> { onAuthenticationFailed() - finish() + leaveAppLocked() } // User made 5 incorrect biometric login attempts so they get a timeout // Note: The SYSTEM provides the localised error "Too many attempts. Try again later.". BiometricPrompt.ERROR_LOCKOUT -> { Toast.makeText(context, errString, Toast.LENGTH_SHORT).show() - finish() + leaveAppLocked() } // User made a large number of incorrect biometric login attempts and Android disabled @@ -116,12 +136,12 @@ class ScreenLockActivity : BaseActionBarActivity() { // Note: The SYSTEM provides the localised error "Too many attempts. Fingerprint sensor disabled." BiometricPrompt.ERROR_LOCKOUT_PERMANENT -> { Toast.makeText(context, errString, Toast.LENGTH_SHORT).show() - finish() + leaveAppLocked() } else -> { Log.w(TAG, "Unhandled authentication error: $errorCode $errString") - finish() + leaveAppLocked() } } } @@ -219,7 +239,11 @@ class ScreenLockActivity : BaseActionBarActivity() { override fun onNewIntent(intent: Intent) { super.onNewIntent(intent) - setIntent(intent) + + // A lock presented over a live activity carries no "next_intent". Adopting such an intent would + // discard the destination a routed unlock still has to reach - the activity that routed here has + // already finished itself, so nothing else remembers where to go. + if (intent.hasExtra(EXTRA_NEXT_INTENT)) setIntent(intent) } public override fun onActivityResult(requestCode: Int, resultcode: Int, data: Intent?) { @@ -276,15 +300,41 @@ class ScreenLockActivity : BaseActionBarActivity() { ?.start() } + // Declining the unlock has to leave the app, not merely finish this activity. The activity being + // locked is still alive (ScreenLockActionBarActivity.presentScreenLock) and finishing alone reveals + // it, whereupon its onStart presents the lock straight back - a loop with no way out, which during a + // biometric lockout spins with no user input at all and flashes the covered content each time. + // + // Backgrounding this task is not enough either: launchMode is singleInstancePerTask, so this lives + // in its OWN task and the app's task sits beneath it. Going to the launcher is what backgrounds + // both. Returning to the app afterwards presents the lock again, which is correct. + private fun leaveAppLocked() { + startActivity( + Intent(Intent.ACTION_MAIN) + .addCategory(Intent.CATEGORY_HOME) + .setFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + ) + finish() + } + private fun handleAuthenticated() { authenticated = true keyCachingService?.setMasterSecret(Any()) // The 'nextIntent' will take us to the MainActivity if this is a standard unlock, or it will - // take us to the ShareActivity if this is an external share. It is absent when this lock was - // presented OVER a still-live activity (see ScreenLockActionBarActivity.presentScreenLock), in - // which case finishing is all that is needed to return to it. - intent.getParcelableExtra("next_intent")?.let(::startActivity) + // take us to the ShareActivity if this is an external share. + val nextIntent = intent.getParcelableExtra(EXTRA_NEXT_INTENT) + + when { + nextIntent != null -> startActivity(nextIntent) + + // Presented over a still-live activity, so there is nothing to route to and finishing + // reveals it again (see ScreenLockActionBarActivity.presentScreenLock). + intent.getBooleanExtra(EXTRA_PRESENTED_OVER, false) -> {} + + // Routed here, but the destination is gone: unlocking will land on nothing. + else -> Log.w(TAG, "Got a null nextIntent - cannot proceed.") + } finish() } From e0a1d830420a3c1b58487bbbf34c3b9d1fc061ad Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 24 Aug 2026 17:25:11 +1000 Subject: [PATCH 3/3] App lock: expire immediately when there is no timeout, instead of via an alarm Backgrounding the app and coming straight back did not lock it. With a zero timeout startTimeoutIfAppropriate still went through AlarmManager, which batches non-wakeup alarms - the "immediate" expiry was consistently arriving ~5s later, and onAppForegrounded cancels the pending alarm on the way back in. So any trip shorter than the batching delay cancelled the lock before it ever happened, and since the timeout is always 0 today (no setting exposes it) that is every quick app switch. Deliver the expiry directly in that case. The service is necessarily foregrounded here, because this is only reached with a secret set, so sending the existing PendingIntent is allowed; it routes through the same onStartCommand path the alarm used, with a fallback to the old behaviour if the PendingIntent has been cancelled. Non-zero timeouts are unchanged. Measured on a Pixel 6 / API 34 emulator: handleClearKey now runs 4ms after "App is no longer visible" rather than ~5s, and a two-second round trip out of the app and back locks. The picker case still works with the tighter timing - the file is attached and sent after unlocking - and this was verified with a fingerprint enrolled, so the BiometricPrompt path rather than the device-credential fallback. --- .../securesms/service/KeyCachingService.java | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java b/app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java index 434bfa1b8b..0fd27f29ae 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java +++ b/app/src/main/java/org/thoughtcrime/securesms/service/KeyCachingService.java @@ -220,10 +220,29 @@ private static void startTimeoutIfAppropriate(@NonNull Context context) { if (!TextSecurePreferences.isPasswordDisabled(context)) timeoutMillis = TimeUnit.MINUTES.toMillis(passphraseTimeoutMinutes); else timeoutMillis = TimeUnit.SECONDS.toMillis(screenLockTimeoutSeconds); + PendingIntent expirationIntent = buildExpirationPendingIntent(context); + + // With no timeout there is nothing to schedule, and scheduling anyway is what left the lock + // unreliable: AlarmManager batches non-wakeup alarms, so an alarm for "now" was still arriving + // seconds later, and onAppForegrounded cancels it on the way back in. A quick trip to another app + // and straight back therefore never locked at all. Deliver it directly instead - the service is + // necessarily foregrounded here, because we only get this far with a secret set. + if (timeoutMillis <= 0) { + Log.i(TAG, "No timeout, expiring immediately"); + + try { + expirationIntent.send(); + } catch (PendingIntent.CanceledException e) { + Log.w(TAG, "Immediate expiry was cancelled, falling back to an alarm", e); + ServiceUtil.getAlarmManager(context).set(AlarmManager.ELAPSED_REALTIME, SystemClock.elapsedRealtime(), expirationIntent); + } + + return; + } + Log.i(TAG, "Starting timeout: " + timeoutMillis); - AlarmManager alarmManager = ServiceUtil.getAlarmManager(context); - PendingIntent expirationIntent = buildExpirationPendingIntent(context); + AlarmManager alarmManager = ServiceUtil.getAlarmManager(context); alarmManager.cancel(expirationIntent); alarmManager.set(AlarmManager.ELAPSED_REALTIME, SystemClock.elapsedRealtime() + timeoutMillis, expirationIntent);