From 62c82aef68e58764aea8bf6fddaf847772221144 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 21:54:20 +0000 Subject: [PATCH] fix(nip46): two batched-consent races found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent review of the concurrent-servicing changes found two real bugs (the quartz/authorizer concurrency core reviewed clean): - Notification TOCTOU. SignerConsentCoordinator did a non-atomic "if pending empty → cancel notification" in the resolving request's finally. A request arriving concurrently could post the shared full-screen-intent notification between another request's empty-check and its cancel, wiping the new request's only surface while backgrounded — it then sat unseen until the 120s timeout denied it. Add/show and remove/empty-check/cancel now run under one surfaceLock, so a live request's notification can't be cancelled out. - Batched selection re-seeded to all-selected on any pending-set change (fail-open). Because requests are serviced concurrently, the pending set changes under an open sheet; re-seeding silently re-checked deselected items and auto-checked newly-arrived requests, so "Allow selected" could grant ops the user deselected or never saw. Now seed once and reconcile incrementally (selected ∩ tokens): deselections survive and a new request is never auto-selected. Also default the batch "Remember" toggle off. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015FHr2mu5SiHwYNR7evYUuF --- .../consent/SignerConsentActivity.kt | 12 +++-- .../consent/SignerConsentCoordinator.kt | 53 ++++++++++++------- 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt index 679b21e994..b89c9de9a4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentActivity.kt @@ -374,10 +374,14 @@ private fun BatchedConsentDialog( ) { val maxHeight = LocalConfiguration.current.screenHeightDp.dp * 0.85f val tokens = pending.map { it.token }.toSet() - // Re-seed (all selected) whenever the set of pending tokens actually changes — keying on size alone - // would leave a newly-arrived request unselectable when another resolves in the same frame. - var selected by remember(tokens) { mutableStateOf(tokens) } - var rememberChoice by remember { mutableStateOf(true) } + // Seed all-selected ONCE for the initial batch the user opened. The signer services requests + // concurrently, so `tokens` can change under an open sheet; reconcile incrementally instead of + // re-seeding — drop resolved tokens but KEEP the user's deselections, and never auto-select a + // newly-arrived request. Otherwise a request landing (or resolving) mid-decision would silently + // re-check everything, and an "Allow selected" tap would grant ops the user deselected or never saw. + var selected by remember { mutableStateOf(tokens) } + LaunchedEffect(tokens) { selected = selected intersect tokens } + var rememberChoice by remember { mutableStateOf(false) } Dialog( onDismissRequest = onDismiss, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt index ed8e1f9c86..e2f68f31b1 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/connectedApps/consent/SignerConsentCoordinator.kt @@ -31,6 +31,9 @@ import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.update +import kotlinx.coroutines.flow.updateAndGet +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import java.util.UUID import java.util.concurrent.ConcurrentHashMap @@ -88,6 +91,12 @@ object SignerConsentCoordinator { // arrive) so concurrent requests don't each post their own. private val batchNotificationId = "nip46-signer-consent".hashCode() + // Guards the surface (post/cancel of the one shared notification) against the pending set so a + // concurrent arrival's post can't be clobbered by another request's teardown cancel. Without it, + // request A could read "pending now empty" and then cancel AFTER request B posted a fresh + // notification under the same id, leaving B with no UI while backgrounded (silent deny at timeout). + private val surfaceLock = Mutex() + suspend fun requestConsent( context: Context, info: SignerConsentInfo, @@ -95,33 +104,39 @@ object SignerConsentCoordinator { val token = UUID.randomUUID().toString() val deferred = CompletableDeferred() deferreds[token] = deferred - _pending.update { it + PendingConsent(token, info) } - // Fast path when Amethyst already owns the foreground: open the dialog directly. When the app - // is backgrounded this is silently dropped by Android 12+ BAL, so the full-screen-intent - // notification is what surfaces the prompt. Both are idempotent — the Activity is singleTop and - // observes [pending], and the notification uses a stable id, so concurrent requests just refresh - // the one prompt. Wrapped because a BAL-blocked launch can throw on some OEMs rather than no-op. - runCatching { - context.startActivity( - Intent(context, SignerConsentActivity::class.java) - .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_SINGLE_TOP), + surfaceLock.withLock { + _pending.update { it + PendingConsent(token, info) } + // Fast path when Amethyst already owns the foreground: open the dialog directly. When the app + // is backgrounded this is silently dropped by Android 12+ BAL, so the full-screen-intent + // notification is what surfaces the prompt. Both are idempotent — the Activity is singleTop and + // observes [pending], and the notification uses a stable id, so concurrent requests just + // refresh the one prompt. Wrapped because a BAL-blocked launch can throw rather than no-op. + runCatching { + context.startActivity( + Intent(context, SignerConsentActivity::class.java) + .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_SINGLE_TOP), + ) + } + SignerConsentNotifier.show( + context = context, + activityClass = SignerConsentActivity::class.java, + extraKey = EXTRA_TOKEN, + token = "nip46-signer-consent", + titleRes = R.string.nip46_signer_notif_sign_title, ) } - SignerConsentNotifier.show( - context = context, - activityClass = SignerConsentActivity::class.java, - extraKey = EXTRA_TOKEN, - token = "nip46-signer-consent", - titleRes = R.string.nip46_signer_notif_sign_title, - ) return try { deferred.await() } finally { deferreds.remove(token) - _pending.update { list -> list.filterNot { it.token == token } } - if (_pending.value.isEmpty()) SignerConsentNotifier.cancel(context, batchNotificationId) + surfaceLock.withLock { + // Remove + emptiness check + cancel are one critical section vs. another request's + // add + show, so a fresh notification is never cancelled out from under a live request. + val stillPending = _pending.updateAndGet { list -> list.filterNot { it.token == token } } + if (stillPending.isEmpty()) SignerConsentNotifier.cancel(context, batchNotificationId) + } } }