From cf7fd9d9141fcfad93c2dc701bf1db84a8e3bc1d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 22:38:47 +0000 Subject: [PATCH] fix(nip42): don't sign one challenge twice, and name the wall either way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of the NIP-42 fetch work turned up one real defect, one inconsistency of its own making, and one allocation worth removing. **A slow signature was signed twice.** reauthenticateIfAuthRequired coalesces a burst of `auth-required:` CLOSEDs by skipping while an AUTH is in flight, but it only asked hasFinishedAllAuths(), which knows about AUTHs already SENT. A signature is not instantaneous, and the ordinary sequence — relay challenges at connect, the first REQ is refused before the signature comes back — leaves the watcher empty, so the guard passed and a second signing pass started for the same challenge. On a NIP-55 or NIP-46 signer that window is a user-facing prompt, so the user got a second one; it also put two threads into saveAuthSubmission's non-atomic check-then-put at once, which can let both AUTHs onto the wire. isSigning() is the half the guard was missing. The sibling tests never saw it because they run on Dispatchers.Unconfined, which completes the signature inline; the new test uses a real dispatcher and a signer that takes time, and fails 2-vs-1 without the fix. **An auth wall is now named whether or not we waited for it.** fetchAllPages already reported End.AUTH_REQUIRED unconditionally while fetchAllWithHooks only wrote `auth-refused:` when pendingOnAuthRequired was on, so authRefusedRelays() silently missed every client with no responder attached — exactly the configuration the reason exists to describe. What the relay said does not depend on whether anyone was there to answer it. **authSuccessMarks() replaces a per-relay associateWith.** The old form allocated one entry per relay on every fetch, auth-gated or not, to record a value the lookup already defaults to. It now records only relays that have already authenticated (near-empty in practice) and returns emptyMap() when nothing answers AUTH at all — this runs per fetch on fan-outs of thousands. Also stops IAuthStatus.NO_AUTH_STATE handing out a MutableStateFlow upcast to StateFlow, which any caller could cast back and mutate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01J8u58RJgTEGkbZH8ygqR35 --- .../client/accessories/NostrClientCountExt.kt | 3 +- .../NostrClientFetchAllWithHooksExt.kt | 26 +++++--- .../accessories/NostrClientFetchFirstExt.kt | 4 +- .../relay/client/auth/AuthOutcome.kt | 24 ++++++++ .../relay/client/auth/RelayAuthenticator.kt | 12 +++- .../RelayAuthenticatorReauthOnClosedTest.kt | 59 +++++++++++++++++++ .../accessories/Nip42AuthGatedFetchTest.kt | 12 ++-- 7 files changed, 123 insertions(+), 17 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt index 1e6b1a05f2..3a1e7d76ae 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientCountExt.kt @@ -24,6 +24,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient import com.vitorpamplona.quartz.nip01Core.relay.client.auth.AuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.DEFAULT_AUTH_GRACE_MS import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMark +import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMarks import com.vitorpamplona.quartz.nip01Core.relay.client.auth.awaitAuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.hasAuthResponder import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.RelayConnectionListener @@ -202,7 +203,7 @@ suspend fun INostrClient.count( try { addConnectionListener(listener) - val authMarks = if (pendingOnAuthRequired) filters.keys.associateWith { authSuccessMark(it) } else emptyMap() + val authMarks = if (pendingOnAuthRequired) authSuccessMarks(filters.keys) else emptyMap() filters.forEach { (relay, filterList) -> val subId = newSubId() subIdToRelay[subId] = relay diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt index ef221707bb..b6739312cd 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchAllWithHooksExt.kt @@ -24,7 +24,7 @@ import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient import com.vitorpamplona.quartz.nip01Core.relay.client.auth.AuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.DEFAULT_AUTH_GRACE_MS -import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMark +import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMarks import com.vitorpamplona.quartz.nip01Core.relay.client.auth.awaitAuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.hasAuthResponder import com.vitorpamplona.quartz.nip01Core.relay.client.reqs.SubscriptionListener @@ -169,12 +169,22 @@ suspend fun INostrClient.fetchAllWithHooks( relay: NormalizedRelayUrl, forFilters: List?, ) { - // Keep the relay pending on an auth-required refusal: the authenticator answers the - // challenge and re-fires this subscription, so the post-auth events still arrive. - // Hand it to the resolver, which ends the relay as auth-refused if the challenge - // does not work out — the refusal stays bounded by the AUTH, not by the timeout. - if (pendingOnAuthRequired && MachineReadablePrefix.parse(message) == MachineReadablePrefix.AUTH_REQUIRED) { - authRefusalChannel.trySend(relay to message) + if (MachineReadablePrefix.parse(message) == MachineReadablePrefix.AUTH_REQUIRED) { + // Keep the relay pending: the authenticator answers the challenge and re-fires + // this subscription, so the post-auth events still arrive. The resolver ends it + // as auth-refused if the challenge does not work out — bounded by the AUTH, not + // by the timeout. + if (pendingOnAuthRequired) { + authRefusalChannel.trySend(relay to message) + return + } + // Not waiting — but still NAME the wall. What the relay said does not depend on + // whether we chose to answer it, and a caller reading `doneOut` wants to know it + // gave up on an auth wall rather than on a policy refusal it can do nothing + // about. This is what [fetchAllPages] already does with End.AUTH_REQUIRED, and + // leaving it as a plain `closed:` here is what would make + // [authRefusedRelays] silently miss every no-responder client. + doneChannel.trySend(relay to "$DONE_REASON_AUTH_REFUSED:$message") return } doneChannel.trySend(relay to "closed:$message") @@ -192,7 +202,7 @@ suspend fun INostrClient.fetchAllWithHooks( // resolver compares against these: an AUTH that lands after this point is one that // re-sent our subscription, whereas a connection that was already authenticated and // still refused us is being gated for a reason no further waiting fixes. - val authMarks = if (pendingOnAuthRequired) filters.keys.associateWith { authSuccessMark(it) } else emptyMap() + val authMarks = if (pendingOnAuthRequired) authSuccessMarks(filters.keys) else emptyMap() val collected = mutableListOf>() // One conflated token, armed by a delay()-based watchdog, ends the fetch // at the wall-clock ceiling. delay() keeps the cap on the coroutine clock diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt index 5b9ee79bad..e1faf08b42 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/NostrClientFetchFirstExt.kt @@ -24,7 +24,7 @@ import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip01Core.relay.client.INostrClient import com.vitorpamplona.quartz.nip01Core.relay.client.auth.AuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.DEFAULT_AUTH_GRACE_MS -import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMark +import com.vitorpamplona.quartz.nip01Core.relay.client.auth.authSuccessMarks import com.vitorpamplona.quartz.nip01Core.relay.client.auth.awaitAuthOutcome import com.vitorpamplona.quartz.nip01Core.relay.client.auth.hasAuthResponder import com.vitorpamplona.quartz.nip01Core.relay.client.reqs.SubscriptionListener @@ -160,7 +160,7 @@ suspend fun INostrClient.fetchFirst( } // Read before the REQ goes out — see [awaitAuthOutcome]'s `since`. - val authMarks = if (pendingOnAuthRequired) filters.keys.associateWith { authSuccessMark(it) } else emptyMap() + val authMarks = if (pendingOnAuthRequired) authSuccessMarks(filters.keys) else emptyMap() var result: Event? = null try { diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt index 20eb72ec8e..0d464e3c55 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/AuthOutcome.kt @@ -68,6 +68,30 @@ fun INostrClient.hasAuthResponder(): Boolean = authResponders().isNotEmpty() */ fun INostrClient.authSuccessMark(relay: NormalizedRelayUrl): Int = authResponders().successMark(relay) +/** + * The marks for a whole fan-out, as a map to be read with `marks[relay] ?: 0`. + * + * Only relays that have ALREADY authenticated get an entry: a relay whose mark is zero is + * indistinguishable from an absent one, and at fan-out sizes where this matters (a router + * sweeping thousands of urls per fetch) nearly every relay is at zero when the REQ goes out. + * Building the full map instead would allocate one entry per relay on every fetch, auth-gated + * or not, to record a value the lookup already defaults to. + * + * Returns an empty map when nothing answers AUTH here, so a caller pays nothing at all. + */ +fun INostrClient.authSuccessMarks(relays: Collection): Map { + val responders = authResponders() + if (responders.isEmpty()) return emptyMap() + var marks: MutableMap? = null + for (relay in relays) { + val mark = responders.successMark(relay) + if (mark != 0) { + (marks ?: HashMap().also { marks = it })[relay] = mark + } + } + return marks ?: emptyMap() +} + /** * Suspends until this relay's NIP-42 challenge resolves one way or the other, for a * caller whose REQ (or COUNT) just came back `CLOSED auth-required:`. diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt index daaed8a1cd..90e62f2991 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticator.kt @@ -80,7 +80,7 @@ interface IAuthStatus { * Shared so the default [authStateFlow] getter allocates nothing — it is read on * every fetch that touches an auth-gated relay. */ - val NO_AUTH_STATE: StateFlow> = MutableStateFlow(persistentMapOf()) + val NO_AUTH_STATE: StateFlow> = MutableStateFlow(persistentMapOf()).asStateFlow() } } @@ -228,7 +228,15 @@ class RelayAuthenticator( // re-hit an external (NIP-55) signer for every ledger-ALLOW account. Skip while an AUTH is // still in flight — the OK of the one we already sent runs [checkAuthResults] → syncFilters, // which re-drives the refused REQ; if it's still refused, that fresh CLOSED re-auths then. - if (!status.hasFinishedAllAuths()) return + // + // BOTH halves of "in flight" have to be checked. [hasFinishedAllAuths] only knows about AUTHs + // already SENT, and a signature is not instantaneous: the relay challenges at connect and + // refuses the first REQ before the signature comes back, so during that window the watcher is + // empty, this guard used to pass, and a second signing pass started for the same challenge — + // a second user-facing prompt on a NIP-55/NIP-46 signer, and two threads racing + // [RelayAuthStatus.saveAuthSubmission]'s check-then-put. See + // RelayAuthenticatorReauthOnClosedTest.aSlowSignatureIsNotSignedTwiceForOneChallenge. + if (!status.hasFinishedAllAuths() || status.isSigning()) return val challenge = status.lastChallenge() ?: return authenticate(relay, challenge, interactive = false) } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt index e8ae39933b..e4da2deaa8 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/auth/RelayAuthenticatorReauthOnClosedTest.kt @@ -36,6 +36,7 @@ import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.runBlocking import kotlin.test.Test import kotlin.test.assertEquals @@ -270,4 +271,62 @@ class RelayAuthenticatorReauthOnClosedTest { assertTrue(relay.sent.isEmpty(), "Without a stored challenge there is nothing to re-auth with") assertFalse(relay.sent.any { it is AuthCmd }) } + + /** + * A signature is not instantaneous, and the coalescing guard used to be blind to that. + * + * The relay challenges at connect and refuses the first REQ with `auth-required:` before + * the signature comes back — the ordinary case, not a corner one. [reauthenticateIfAuthRequired] + * checks [RelayAuthStatus.hasFinishedAllAuths], which only knows about AUTHs already SENT, so + * while the first signature is still being produced the watcher is empty, the guard passes, + * and a second signing pass starts for the very same challenge. + * + * That is the thing the guard's own comment says it exists to prevent — "Re-signing on each + * would re-hit an external (NIP-55) signer" — and for a NIP-55 or NIP-46 signer the whole gap + * is a user-facing prompt, so the cost is a SECOND prompt for one connection. It also puts two + * threads into [RelayAuthStatus.saveAuthSubmission]'s non-atomic check-then-put at once, which + * can let both AUTHs onto the wire. + * + * Runs on a real dispatcher with a signer that takes time; the sibling tests use + * [Dispatchers.Unconfined], which completes the signature inline and hides this entirely. + */ + @Test + fun aSlowSignatureIsNotSignedTwiceForOneChallenge() = + runBlocking { + val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val signer = NostrSignerInternal(KeyPair()) + val signCalls = + java.util.concurrent.atomic + .AtomicInteger(0) + + val client = CapturingClient() + RelayAuthenticator( + client = client, + scope = scope, + signWithAllLoggedInUsers = { _, template, _ -> + signCalls.incrementAndGet() + // Stands in for the signer round-trip — a NIP-55 prompt, a bunker hop. + kotlinx.coroutines.delay(300) + listOf(signer.sign(template)) + }, + ) + val listener = client.captured ?: error("RelayAuthenticator did not register a listener") + val relay = FakeRelayClient(NormalizedRelayUrl("wss://relay.example/")) + + listener.onConnecting(relay) + listener.onIncomingMessage(relay, "", AuthMessage("chal-1")) + // The REQ we already had open is refused while that signature is still being produced. + listener.onIncomingMessage( + relay, + "", + ClosedMessage("sub", MachineReadablePrefix.AUTH_REQUIRED.format("authenticate first")), + ) + + // Let both signing passes finish, whether one or two were started. + kotlinx.coroutines.delay(1_000) + + assertEquals(1, signCalls.get(), "one challenge must cost exactly one signature, not one per refusal") + assertEquals(1, authedPubKeys(relay).size, "and exactly one AUTH reaches the wire") + scope.cancel() + } } diff --git a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt index bc955fb98c..90c11a0a07 100644 --- a/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt +++ b/quartz/src/jvmTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/Nip42AuthGatedFetchTest.kt @@ -171,7 +171,10 @@ class Nip42AuthGatedFetchTest { } assertEquals(0, collected.size) - assertTrue(doneOut[relay]?.startsWith("closed:") == true, "was ${doneOut[relay]}") + // Named even though we never waited: what the relay said does not depend on whether + // anyone was there to answer it, so `authRefusedRelays()` sees a no-responder client + // exactly as it sees a declining one. + assertEquals(setOf(relay), doneOut.authRefusedRelays(), "was ${doneOut[relay]}") assertTrue(elapsed.inWholeMilliseconds < 1_000, "no waiting when nobody can answer; was $elapsed") } } @@ -200,9 +203,10 @@ class Nip42AuthGatedFetchTest { doneOut = doneOut, ) { _, _ -> true } - assertTrue( - doneOut[relay]?.startsWith("closed:") == true, - "opting out must end the relay on the CLOSED itself; was ${doneOut[relay]}", + assertEquals( + setOf(relay), + doneOut.authRefusedRelays(), + "opting out ends the relay on the CLOSED itself, but still names the wall; was ${doneOut[relay]}", ) } }