From fe4e881df65d7e1b716a73b1adc0e4a8893ac89d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 22:03:49 +0000 Subject: [PATCH] =?UTF-8?q?fix(nip46):=20audit=20fixes=20=E2=80=94=20data?= =?UTF-8?q?=20race,=20cancellation,=20write=20amplification?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from an audit of the signer, all verified against the code: - Data race: NostrConnectSignerService deduped request ids inside onEvent, which the relay pool invokes CONCURRENTLY from each relay's socket thread (PoolRequests dispatches listeners outside its lock). Two relays delivering the same subscription could mutate the LinkedHashSet at once → race / CME. Move dedup into the single consumer coroutine; onEvent now only does the thread-safe channel send. - Swallowed cancellation: broad `catch (Exception)` around suspend calls in the processor, the service's decrypt + publish, and connectViaNostrConnect caught CancellationException too, breaking structured cancellation when the service restarts. Rethrow it first (matching the AccountCacheState convention). - Write amplification: the ledger wrote last-used to that client's DataStore file on EVERY authorized request (unthrottled, unlike the relay-auth store). Coalesce to at most one write per client per 60s in the authorizer. - Redundant resubscribe: the enable/relays collector lacked distinctUntilChanged, so a duplicate inbox-relay emission tore the subscription down and re-opened it on every relay for nothing. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015FHr2mu5SiHwYNR7evYUuF --- .../model/nip46Signer/Nip46SignerState.kt | 7 +++++ .../signers/Nip46PermissionAuthorizer.kt | 31 +++++++++++++++++-- .../server/BunkerRequestProcessor.kt | 3 ++ .../server/NostrConnectSignerService.kt | 31 +++++++++++++------ 4 files changed, 60 insertions(+), 12 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt index 234e93821d..651af1abeb 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip46Signer/Nip46SignerState.kt @@ -37,6 +37,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.server.BunkerRequestProcessor import com.vitorpamplona.quartz.nip46RemoteSigner.server.NostrConnectSignerService import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.RandomInstance +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow @@ -44,6 +45,7 @@ import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch @@ -121,6 +123,9 @@ class Nip46SignerState( scope.launch(Dispatchers.IO) { combine(settings.nip46SignerEnabled, listeningRelays) { enabled, relays -> enabled to relays } + // Inbox/relay StateFlows can re-emit an identical set; without this every duplicate + // would tear the subscription down and re-open it on every relay for no reason. + .distinctUntilChanged() .collectLatest { (enabled, relays) -> if (!enabled) return@collectLatest if (!signer.isWriteable()) { @@ -207,6 +212,8 @@ class Nip46SignerState( setEnabled(true) ConnectResult.Connected(offer.clientPubKey, offer.name) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Log.w("NIP46Signer") { "nostrconnect pairing failed: ${e.message}" } ConnectResult.Failed(e.message ?: "unknown error") diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/napplet/signers/Nip46PermissionAuthorizer.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/napplet/signers/Nip46PermissionAuthorizer.kt index bf7cc7c6f7..e83e07eb66 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/napplet/signers/Nip46PermissionAuthorizer.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/napplet/signers/Nip46PermissionAuthorizer.kt @@ -20,6 +20,8 @@ */ package com.vitorpamplona.amethyst.commons.napplet.signers +import com.vitorpamplona.amethyst.commons.util.KmpLock +import com.vitorpamplona.amethyst.commons.util.withLock import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestConnect @@ -30,6 +32,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestNip44Encrypt import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequestSign import com.vitorpamplona.quartz.nip46RemoteSigner.server.Nip46ConnectDecision import com.vitorpamplona.quartz.nip46RemoteSigner.server.Nip46RequestAuthorizer +import com.vitorpamplona.quartz.utils.TimeUtils /** * Bridges the NIP-46 signer core to Amethyst's shared "Connected Apps" trust @@ -67,9 +70,30 @@ class Nip46PermissionAuthorizer( /** Invoked after a successful connect so the host can persist display metadata (name/url/image). */ val onConnected: (suspend (clientPubKey: HexKey, request: BunkerRequestConnect) -> Unit)? = null, ) : Nip46RequestAuthorizer { + // A high-throughput client can authorize many signs per second; last-used is display-only, + // so coalesce the DataStore write to at most one per client per LAST_USED_THROTTLE_SECS + // instead of writing the whole per-client preferences file on every request. + private val lastUsedThrottle = mutableMapOf() + private val throttleLock = KmpLock() + /** The ledger coordinate for [clientPubKey] under this account. */ fun coordinateFor(clientPubKey: HexKey): String = coordinateFor(signerPubKey, clientPubKey) + private suspend fun touchLastUsed(coordinate: String) { + val now = TimeUtils.now() + val shouldWrite = + throttleLock.withLock { + val previous = lastUsedThrottle[coordinate] ?: 0L + if (now - previous >= LAST_USED_THROTTLE_SECS) { + lastUsedThrottle[coordinate] = now + true + } else { + false + } + } + if (shouldWrite) ledger.updateLastUsed(coordinate, now) + } + override suspend fun onConnect( clientPubKey: HexKey, request: BunkerRequestConnect, @@ -82,7 +106,7 @@ class Nip46PermissionAuthorizer( if (!ledger.hasPolicy(coordinate)) { ledger.setPolicy(coordinate, defaultPolicyOnConnect) } - ledger.updateLastUsed(coordinate) + touchLastUsed(coordinate) onConnected?.invoke(clientPubKey, request) // Echo the offered secret when present (the client validates it); otherwise ack. @@ -100,7 +124,7 @@ class Nip46PermissionAuthorizer( val op = request.toSignerOp() ?: return true val coordinate = coordinateFor(clientPubKey) val allowed = ledger.decide(coordinate, op) == NostrOpDecision.ALLOW - if (allowed) ledger.updateLastUsed(coordinate) + if (allowed) touchLastUsed(coordinate) return allowed } @@ -115,6 +139,9 @@ class Nip46PermissionAuthorizer( private const val ACK = "ack" + /** Minimum seconds between persisted last-used updates for one client (write-coalescing). */ + private const val LAST_USED_THROTTLE_SECS = 60L + /** * The Connected-Apps ledger coordinate for a NIP-46 client, namespaced by * the user's signer so the same client paired with two accounts on one diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt index efb7454bd3..f86e3c984c 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/BunkerRequestProcessor.kt @@ -44,6 +44,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponseGetRelays import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponsePong import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponsePublicKey import com.vitorpamplona.quartz.nip46RemoteSigner.ReadWrite +import kotlinx.coroutines.CancellationException /** * The signer/bunker side of NIP-46: turns a decrypted [BunkerRequest] from a @@ -130,6 +131,8 @@ class BunkerRequestProcessor( else -> BunkerResponseError(request.id, "unsupported method: ${request.method}") } } + } catch (e: CancellationException) { + throw e } catch (e: Exception) { BunkerResponseError(request.id, "${e::class.simpleName}: ${e.message}") } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt index 41317dad27..af42c53a26 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip46RemoteSigner/server/NostrConnectSignerService.kt @@ -31,6 +31,7 @@ import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerRequest import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerResponseError import com.vitorpamplona.quartz.nip46RemoteSigner.NostrConnectEvent import com.vitorpamplona.quartz.utils.Log +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.channels.Channel.Factory.UNLIMITED @@ -76,8 +77,6 @@ class NostrConnectSignerService( val self = signer.pubKey val events = Channel(UNLIMITED) - // Insertion-ordered so the oldest id can be evicted once the cap is hit. - val seen = LinkedHashSet() val subId = newSubId() val listener = object : SubscriptionListener { @@ -87,23 +86,31 @@ class NostrConnectSignerService( relay: NormalizedRelayUrl, forFilters: List?, ) { - if (event is NostrConnectEvent && event.verifiedRecipientPubKey() == self && seen.add(event.id)) { - if (seen.size > seenCap) { - seen.iterator().let { - it.next() - it.remove() - } - } + // onEvent is invoked CONCURRENTLY from each relay's socket thread, so it must + // touch no shared mutable state here — the (thread-safe) channel send is all it + // does; dedup happens in the single-threaded consumer below. + if (event is NostrConnectEvent && event.verifiedRecipientPubKey() == self) { events.trySend(event) } } } + // Insertion-ordered dedup, confined to this one consumer coroutine (never the relay threads); + // evicts the oldest id past the cap so a long-lived signer can't grow it without bound. + val seen = LinkedHashSet() val filter = Filter(kinds = listOf(NostrConnectEvent.KIND), tags = mapOf("p" to listOf(self))) client.subscribe(subId, relays.associateWith { listOf(filter) }, listener) try { while (true) { - handle(events.receive()) + val event = events.receive() + if (!seen.add(event.id)) continue // same request already handled (arrived on another relay) + if (seen.size > seenCap) { + seen.iterator().let { + it.next() + it.remove() + } + } + handle(event) } } finally { client.unsubscribe(subId) @@ -116,6 +123,8 @@ class NostrConnectSignerService( val request = try { event.decryptMessage(signer) as? BunkerRequest ?: return + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Log.w("NIP46Signer") { "could not decrypt request ${event.id.take(8)}: ${e.message}" } return @@ -128,6 +137,8 @@ class NostrConnectSignerService( try { val reply = NostrConnectEvent.create(response, client, signer) this.client.publish(reply, relays) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Log.w("NIP46Signer") { "failed to send reply for ${request.method}: ${e.message}" } }