App lock: cover the app instead of destroying it, and let debug builds lock at all - #2180
Open
mpretty-cyro wants to merge 3 commits into
Open
App lock: cover the app instead of destroying it, and let debug builds lock at all#2180mpretty-cyro wants to merge 3 commits into
mpretty-cyro wants to merge 3 commits into
Conversation
…s lock at all
Two defects that only show up together.
- <uses-permission> asked for ACCESS_SESSION_SECRETS without the ${authority_postfix}
the <permission> 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.
…ecreation 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.
… 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.
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.
The symptom
With app lock enabled, attaching a File to a message drops the user out of Session and the file is
never sent — no error, no toast, no failed-message bubble.
Two defects, and they only show together
1.
<uses-permission>didn't match the<permission>declaration.AndroidManifest.xmldeclares the permission with the build's postfix but asked for it without one:KeyCachingService.handleClearKeybroadcasts with"…ACCESS_SESSION_SECRETS" + BuildConfig.AUTHORITY_POSTFIX, so on any build with a non-empty postfix the app does not hold thepermission its own broadcast requires and the broadcast is dropped in silence. Only
debugsets apostfix (
release/qause""), so app lock has never auto-locked anything in a debug build — asecurity feature that silently does nothing in the build developers run all day, and the reason the
defect below was invisible locally.
2. Clearing the key destroyed the task.
onMasterSecretCleared()calledfinish()when the app was not visible. EveryScreenLockActionBarActivityin the process receives that broadcast, so the whole task emptied —including a conversation waiting on a picker result, whose
onActivityResultthen had nowhere to go.Opening the SAF picker stops the process (
ApplicationContextobservesProcessLifecycleOwner), andthe screen-lock timeout is
0—getScreenLockTimeoutdefaults to0, the only writer in the treealso writes
0, and no setting exposes it — so the alarm fires while the picker is open, every time.The lock is now presented in front of the activity and
onStartre-checks on return, so theactivity and its pending result survive.
Scope of that claim: this preserves a direct return to the task — coming back from a picker,
camera or share sheet, which is the case attachment flows depend on. It does not make a conversation
survive re-entry from the launcher icon:
HomeActivityislaunchMode="singleTask"and the launcheralias targets it, so that clears everything above Home regardless of this change.
Nothing was traded away for this
0— "lock as soon as I leave" is the right posture, and raising it would tradesecurity to hide this
masterSecretis a sentinelObject, not a key ("only indicates if the app was unlocked or not"),so finishing evicted no cryptographic material
BaseActionBarActivityalready appliesFLAG_SECUREinonPausefor every activity, so abackgrounded conversation was never in the recents thumbnail whether or not it was finished
Second commit: review fixes
onStartpresented the lock straight back — a loop escapable only via Home, and one that spun withno user input during a biometric lockout. Every non-authenticating exit now leaves the app.
Worth knowing for anyone touching this:
ScreenLockActivityissingleInstancePerTask, so it is theroot of its own task above the app's, not stacked inside it —
moveTaskToBack(true)thereforereveals the app again and does not work here. Back is routed through the same path; it previously
bypassed the error handling entirely.
onCreate's route-and-finishpath then discarded the pending result.
hasStartedis saved/restored and the locked route issuppressed on a recreation, so
onStartcovers it instead.onMasterSecretClearednow applies the sameisScreenLocked()guard asonStart:KeyCachingService.onDestroybroadcasts whether or not app lock is on, andScreenLockActivityonlyprompts when the setting is on, so without it that broadcast could raise a lock screen with no way
past it.
onNewIntentno longer adopts an intent carrying nonext_intent, which would discard thedestination a routed unlock still has to reach.
next_intentthat has genuinely gone missing, distinguished from theexpected-absent case by an explicit extra rather than by null alone.
Third commit: the zero timeout was never immediate
Backgrounding the app and coming straight back did not lock it, and this predates the whole PR.
startTimeoutIfAppropriatescheduled even a zero timeout throughAlarmManager, which batchesnon-wakeup alarms — measured delivery for an alarm set to "now" was ~5 seconds — and
onAppForegroundedcancels the pending alarm on the way back in. Any excursion shorter than thebatching delay therefore cancelled the lock before it happened, which with a timeout that is always 0
means every quick app switch:
Zero timeouts now send the existing expiry
PendingIntentdirectly instead of scheduling it. That isnot a background service start — the branch is only reachable with a secret set, so
foregroundService()has already run — and it falls back to the old alarm if thePendingIntenthasbeen cancelled. Non-zero timeouts are unchanged.
handleClearKeynow runs 4ms after backgroundingrather than ~5s.
This makes the lock noticeably stricter than the behaviour anyone has actually been using, because
until now the alarm delay was quietly forgiving every short trip. That is what a 0 timeout means and
what the setting promises, but if it proves too aggressive in practice the answer is the timeout
setting this code has never exposed, not an alarm that happens to be slow.
Testing
Pixel 6 / API 34 emulator with a device PIN,
websiteDebug— which is itself the check on defect 1,since that variant previously never received the broadcast:
Unlocking returns to the same conversation with the file marked Sent. Also checked:
a two-second round trip out of the app and back now locks (the case the alarm delay used to swallow); Home then
reopen still demands authentication; a force-stop cold start still routes through
STATE_SCREEN_LOCKED;rotating the device while the picker is open keeps the attachment (
onCreatelogshas_started=true,the locked route is skipped, the file still sends); and Back from a lock covering a live conversation
lands on the launcher having created exactly one lock screen.
Not covered by that testing
ERROR_LOCKOUTcase. Everything above was re-run with a fingerprintenrolled, so the
BiometricPromptpath and itsonAuthenticationErrorarms do now execute ratherthan falling back to
createConfirmDeviceCredentialIntent— worth checking withadb shell dumpsys biometric | grep -i eligiblebefore trusting any app-lock test result, since abare emulator reports
Ineligible … :7and never runs that branch at all. Deliberately triggering afive-failed-attempts lockout is still untested.
before, since the activity was already finished pre-PR, but not the same property as the picker case.
onStart, but the activity beneath also runsonResume, which clearsFLAG_SECURE. Not observed, and not verified frame-by-frame. If it shows up,the answer is to blank content while locked rather than to go back to destroying the task.