fix(nip46): two batched-consent races found in review

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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015FHr2mu5SiHwYNR7evYUuF
2026-08-10 08:27:04 +00:00 · 2026-07-17 15:18:36 +00:00
parent fd5556609f
commit 62c82aef68
2 changed files with 42 additions and 23 deletions
@@ -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,
@@ -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<SignerOpGrant>()
        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)
+            }
        }
    }