fix(signer): publish nostrconnect response to all URI relays - #3
Merged
Conversation
Previously Clave only published the connect response to parsedURI.relays.first. Clients (nostr-tools BunkerSigner.fromURI, NDK, etc.) subscribe on every relay listed in the nostrconnect URI — if the chosen relay drops the ephemeral kind:24133 event, the handshake silently times out even though Clave's activity log shows 'signed'. Now publishes the same signed event to every connected relay in parallel and fetches client follow-ups from all of them.
…handshake LightSigner.handleRequest hardcoded relay.powr.build as the response relay. That's correct for bunker:// (proxy subscribes there → APNs → NSE), but broke nostrconnect:// follow-up RPCs: get_public_key, switch_relays, etc. went to relay.powr.build while the client was still subscribed on its URI relays and never saw the response. Thread an optional responseRelays: [LightRelay] through handleRequest and sendErrorResponse. When provided (from handleNostrConnect's already-connected URI relays), publish to all of them in parallel, best-effort. When nil (NSE/bunker path), keep original behavior. This lets switch_relays reach the client on its URI relays, the client then switches to relay.powr.build, and future RPCs flow the bunker path.
…lity Project sets SWIFT_DEFAULT_ACTOR_ISOLATION=MainActor, which inferred LightRelay.init as @MainActor-bound. That caused a Swift 6 warning when the new AppState multi-relay helpers create LightRelay instances inside TaskGroup closures (off-main). LightRelay is already @unchecked Sendable so marking init nonisolated is consistent. Async methods already auto-hop and don't need the annotation.
LightRelay.connect races ws.sendPing with a Task.sleep timeout. When the sleep wins, group.cancelAll() is called — but withCheckedThrowingContinuation does not cooperate with Swift cancellation, so the ping callback never fires, the continuation leaks, and the TaskGroup can never return. Same pattern in publishEvent and fetchEvents with ws.receive(). Symptom before this fix: if a relay accepts the WebSocket handshake but stays silent (e.g. nostr.wine waiting for NIP-42 AUTH before sending pong), the containing handleNostrConnect hangs forever on the approval sheet's 'Connecting…' spinner. Pre-existing bug, but the Task 2 multi-relay change makes it trigger reliably for any URI containing a silent relay. Fix: force ws.cancel on timeout so pending callbacks resolve with an error. Extracted receiveWithTimeout helper for the two receive() call sites.
Previous fix called ws.cancel() on timeout to force the sendPing callback
to fire so the continuation could complete. But URLSessionWebSocketTask
can invoke the sendPing callback more than once when cancel happens
shortly after a pong arrives — once with success, then again with the
abort error from cancel. Resuming a CheckedContinuation twice is a
fatal error ("SWIFT TASK CONTINUATION MISUSE").
Fix: wrap the callback body in a thread-safe one-shot guard so any
subsequent invocations are silently dropped.…an run Previously handleNostrConnect broke out of its retry loop as soon as one client reply arrived (handshakeComplete=true) and then disconnected. But NDK's and nostr-tools' NIP-46 clients send a sequence of RPCs after the initial connect response: connect(ack) → get_public_key → switch_relays. Clave was processing only the first of these before disconnecting, stranding the client on the URI relays instead of letting it migrate to relay.powr.build via switch_relays. All subsequent sign_event RPCs then went to relays the proxy doesn't watch, and signing silently failed. Fix: after a reply is seen, suppress further republish of the connect response (handshakeComplete guards that) but keep running the retry loop's listen+fetch windows for the full ~15s budget. Dedupe events across iterations via a persistent seenEventIds set. Result: NDK's switch_relays call now receives the ['wss://relay.powr.build'] response and migrates its RPC layer accordingly, so future sign_events flow through the normal bunker path (proxy → APNs → NSE).
DocNR
marked this pull request as ready for review
April 18, 2026 17:49
DocNR added a commit
that referenced
this pull request
May 2, 2026
Brainstorm review of design-system.md against shipped code surfaced 9 inconsistencies + 1 anti-pattern still present. Fixed everything in one batch so the next TestFlight archive carries it all. Code: - HomeView: drop .padding(.bottom, 8) on SlimIdentityBar invocation — slim banner owns its outer bottom padding (12pt); stacking another 8pt on top was double-counting (review #4) - HomeView: drop .padding(.bottom, 8) inside statsRow — listSectionSpacing(0) carries the gap to Connected Clients; the residual padding kept the visible gap excessive after polish round 2 (review #2) - AccountDetailView: avatarLarge letter fallback opacity 0.25 → 0.22 to align with SlimIdentityBar's 0.22 (review #3) - ConnectSheet: add .presentationBackground(Color(.systemGroupedBackground)) — was the last sheet still defaulting to translucent (review #9) - ApprovalSheet: rename @State capExceeded → showConnectionCapAlert for naming convention parity with HomeView (review #7) design-system.md: - New "Cross-platform applicability" section at the top — clarifies what carries directly to clave.casa web companion (color tokens, displayLabel rule, identity-vs-functional zone philosophy, avatar treatments, copy patterns, anti-patterns) vs what's iOS-only (SwiftUI modifiers, haptics, sheet/toolbar conventions) - §3 Typography: corrected initial-letter font scale — AvatarView uses size*0.35 mono (pubkey) or size*0.4 proportional (name); was wrongly documented as a single 0.37 (review #1) - §4 Avatars: added Treatment Selection Rule table (B on neutral bg, C on saturated theme gradient) + clarified 1-vs-2 letter behavior (review #5, #6) - §4 Sizing scale: expanded table to include initial font + border thickness per slot, with the ~5% border scaling rule (review #8) - §5 Spacing: explicit "single source of truth" note on slim banner bottom padding; new "Stats row" subsection capturing the ultraThinMaterial-on-small-cards-OK rule (review gap #10, #4) - §6 HomeView gradient: documented palette[0] defensive fallback when currentAccount is nil (review #13) - §7 Patterns: new "State variable naming" subsection with the showCapAlert / showAccountCapAlert / showConnectionCapAlert convention (review #7 doc side) - §11 Anti-patterns: audit-point note that ConnectSheet was the last surface to acquire .presentationBackground (review #9 doc side) Build green on iOS Simulator 26.4. pbxproj still 41 — assumes user hasn't yet archived 41; bump to 42 if needed before re-archive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DocNR added a commit
that referenced
this pull request
Jun 7, 2026
Closes a spec-compliance gap surfaced by external code review of the TypeScript port (same bug existed verbatim in the Swift port). Spec algorithm step 4 says: before MAC verify, fail if the embedded kind/scope on the wire don't match the caller-supplied expected kind/scope. Our implementation skipped this check on the assumption that MAC verify covered it — that's only half right. Why MAC verify alone isn't sufficient: - The MAC is computed over `nonce || u32_be(kind) || u32_be(scope_len) || scope || chacha20_ct`. - At encrypt time, the encryptor uses (kind, scope) consistent with what they signed in to the wire's embedded fields. - On decrypt, our implementation computes the verification MAC using the CALLER's `context.kind / context.scope`, NOT the embedded `parts.kind / parts.scope` from `Ciphertext.decode`. - For a legitimate wire, caller's context == embedded == encryptor's choice, so MAC matches and decrypt succeeds. - For a wire whose embedded kind/scope bytes are tampered IN TRANSIT but whose MAC tag is left intact, our MAC computation still uses the (untampered) caller context. If that context matches what the encryptor originally signed in, MAC matches and decrypt succeeds — silently accepting a tampered wire. The exploit surface in current usage is narrow because we never expose `parts.kind / parts.scope` from the public decrypt API (the caller already knows kind+scope; they passed them as context). But it's a real spec deviation and weakens the "embedded context is authenticated" property the v3 design claims. Fix: explicit `parts.kind == context.kind` check + constant-time `parts.scope == context.scope` bytes equality, before MAC verify. Both branches throw `.decryptionFailed` (same case as MAC failure) so a network observer can't oracle which mismatch tripped it. Constant-time helper duplicated inline rather than imported from Encryption.swift because cross-file private exposure for a 12-line helper adds little. Two new XCTest cases in NIP44v3Tests.swift verify rejection: - testDecryptRejectsTamperedEmbeddedKind — flips byte 68 of a valid wire (low byte of u32 kind), MAC untouched, expects .decryptionFailed. - testDecryptRejectsTamperedEmbeddedScope — flips byte 73 (first scope byte) of a valid non-empty-scope wire, MAC untouched, expects .decryptionFailed. Both would have silently SUCCEEDED before this commit. Build bumped 92 → 93. Companion Spectr-side fix lands in DocNR/spectr@feat/nip44v3-port in a separate commit covering the same defect + #2/#3/#4 from the same review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Fixes multiple bugs in Clave's
nostrconnect://handshake that caused login to silently fail with compliant multi-relay clients (plebsvszombies.cc, zap.cooking, any nostr-toolsBunkerSigner.fromURIuser with a non-relay.powr.buildURI), plus two pre-existing concurrency bugs inLightRelaythat caused hangs and crashes.Nothing changes in the bunker:// flow or the NSE/APNs pipeline.
Commits (8)
feat(signer): add parallel multi-relay helpers for nostrconnect handshake—connectToRelays/publishEventToRelays/fetchEventsFromRelaysonAppState, parallelize over URI relays withwithTaskGroup. Unit tests for empty-input fast paths and unreachable-URL behavior.fix(signer): publish nostrconnect response to all URI relays— replaceparsedURI.relays.firstwith publish-to-all; activity log "signed" if any relay accepts, "error" if all fail, "Could not connect to any relay" if zero connect.chore: bump build to 10fix(signer): publish RPC responses to URI relays during nostrconnect handshake— threadresponseRelays: [LightRelay]throughLightSigner.handleRequestso follow-up RPC responses (get_public_key, switch_relays, etc.) land where the client is listening. Falls back toSharedConstants.relayURLfor the bunker/NSE path.fix(relay): mark LightRelay.init as nonisolated for Swift 6 compatibility— project hasSWIFT_DEFAULT_ACTOR_ISOLATION=MainActor, which inferredinitas MainActor-bound;nonisolated initlets TaskGroup closures construct it off-main.fix(relay): cancel WebSocket on timeout to prevent sendPing/receive hang— previously, if a relay accepted the WebSocket but never responded to ping (e.g., nostr.wine's NIP-42 AUTH wait), thesendPingcontinuation leaked andwithThrowingTaskGroupcouldn't return. Timeout now callsws.cancel()to force the callback to fire.fix(relay): guard sendPing continuation against double-resume—URLSessionWebSocketTask.sendPingcan invoke its callback more than once when cancel happens shortly after pong; added thread-safe one-shot guard to preventSWIFT TASK CONTINUATION MISUSEcrash.fix(signer): keep listening after first client RPC so switch_relays can run— was breaking out of the retry loop onhandshakeComplete=trueand disconnecting, which stranded clients mid-handshake beforeswitch_relayscould migrate them torelay.powr.build. Now continues listening for the full ~15s window; handshakeComplete only suppresses republish.Device verification
Known limitations (not regressions; will address in follow-ups)
switch_relaysto migrate torelay.powr.build. Neither plebsvszombies.cc (older bundled nostr-tools that predates fromURI's switchRelays call) nor zap.cooking (custom wrapper that bypasses NDK's blockUntilReady) actually does this today. Tracked as a follow-up: either encourage upstream fixes or add proxy-per-client-relay subscriptions on the Dell (spawned task).connectRPC doesn't validateparams[0](remote-signer-pubkey). Secret check still gates, so not a security issue; hardening tracked as backlog.resultfield. Spec shows{id, result, error}; we send{id, error}. Most clients don't care; backlog.describeis a non-standard extension method. Harmless; documented for awareness.Test plan
Merge
Recommend squash-merge to consolidate the 8-commit history into a single
feat(signer): nostrconnect multi-relay handshakecommit on main.🤖 Generated with Claude Code