From d8790087b4ff0bf7654333f99a2f83e468de76cd Mon Sep 17 00:00:00 2001 From: Padreug Date: Sat, 27 Jun 2026 12:03:27 +0200 Subject: [PATCH 1/4] fix(admin): wait for a connected relay before the boot DM, not a fixed sleep (#48) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The boot-time admin notification (notifyAdminsOfNewConnection) spun up a throwaway RelayPool to two external public relays, slept a fixed 2500ms, then published. Those relays are often slow to connect, so on the aio-demo deploy the publish failed on boot ("relay not connected: wss://blastr.f7z.xyz; …") — caught and logged, non-fatal, but noisy every boot. Poll pool.connectedCount() > 0 up to an 8s cap instead: send as soon as a relay is up, and still best-effort — fall through and let dmUser log the failure if none connect in time. Refs: #48, #45 --- src/daemon/admin/index.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/daemon/admin/index.ts b/src/daemon/admin/index.ts index a54ad60..36764bb 100644 --- a/src/daemon/admin/index.ts +++ b/src/daemon/admin/index.ts @@ -127,8 +127,15 @@ class AdminInterface { const sk = secretKeyBytes(this.adminNsec); const pool = new RelayPool(['wss://blastr.f7z.xyz', 'wss://nostr.mutinywallet.com'], {}); pool.start(); - // Give the connections a moment to come up before publishing. - await new Promise((r) => setTimeout(r, 2500)); + // Wait until at least one relay is actually connected (capped), rather + // than a fixed sleep — these external public relays can be slow to come + // up, and a too-short fixed wait made the DM publish fail on boot. Still + // best-effort: if none connect in time we fall through and dmUser logs + // the publish failure without affecting the daemon. (#48) + const deadline = Date.now() + 8000; + while (pool.connectedCount() === 0 && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 100)); + } for (const npub of this.npubs || []) { await dmUser(sk, npub, `nsecBunker has started; use ${connectionString} to connect to it and unlock your key(s)`, pool); From 434817c899152adb4b044c7a6c60b01651f482d9 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sat, 27 Jun 2026 12:22:24 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(admin):=20harden=20the=20boot=20DM=20?= =?UTF-8?q?=E2=80=94=20guard=20teardown=20+=20unhandled=20rejection=20(rev?= =?UTF-8?q?iew=20CS-3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds the review's CS-3 finding into the boot-DM fix: - notifyAdminsOfNewConnection wraps the publish loop in try/finally so a throw mid-loop can't leak the throwaway pool's reconnect loops + sockets for the process lifetime. - The constructor's fire-and-forget call now .catch()es (both the notify and the config() it chains off), so a boot-DM failure can't surface as an unhandled rejection — process-terminating under Node defaults. - dmUser guards its whole body: nip19.decode/nip04.encrypt ran *before* the old publish-only try, so a malformed admin npub (passes startsWith, fails the bech32 checksum) threw past the caller. Now best-effort, never throws. Refs: #48, review CS-3 --- src/daemon/admin/index.ts | 20 +++++++++++++++----- src/utils/dm-user.ts | 32 ++++++++++++++++++-------------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/daemon/admin/index.ts b/src/daemon/admin/index.ts index 36764bb..569e1c3 100644 --- a/src/daemon/admin/index.ts +++ b/src/daemon/admin/index.ts @@ -110,9 +110,13 @@ class AdminInterface { this.config().then((config) => { if (config.admin?.notifyAdminsOnBoot) { - this.notifyAdminsOfNewConnection(connectionString); + // .catch so a boot-DM failure can't surface as an unhandled + // rejection (process-terminating under Node defaults). #48 / CS-3. + this.notifyAdminsOfNewConnection(connectionString).catch((e) => + console.log('notifyAdminsOfNewConnection failed:', e?.message ?? e), + ); } - }); + }).catch((e) => console.log('config() failed during admin init:', e?.message ?? e)); } public async config(): Promise { @@ -137,10 +141,16 @@ class AdminInterface { await new Promise((r) => setTimeout(r, 100)); } - for (const npub of this.npubs || []) { - await dmUser(sk, npub, `nsecBunker has started; use ${connectionString} to connect to it and unlock your key(s)`, pool); + // try/finally so a throw mid-loop can't leak the pool's reconnect loops + + // sockets for the process lifetime (#48 / review CS-3). dmUser itself is + // now fully guarded, but keep the finally as belt-and-suspenders. + try { + for (const npub of this.npubs || []) { + await dmUser(sk, npub, `nsecBunker has started; use ${connectionString} to connect to it and unlock your key(s)`, pool); + } + } finally { + pool.stop(); } - pool.stop(); } /** diff --git a/src/utils/dm-user.ts b/src/utils/dm-user.ts index db3dc06..34c6add 100644 --- a/src/utils/dm-user.ts +++ b/src/utils/dm-user.ts @@ -12,22 +12,26 @@ export async function dmUser( content: string, pool: RelayPool, ): Promise { - const recipientHex = recipient.startsWith("npub1") - ? (nip19.decode(recipient).data as string) - : recipient; - const ciphertext = nip04.encrypt(sk, recipientHex, content); - const event = finalizeEvent( - { - kind: 4, - created_at: Math.floor(Date.now() / 1000), - tags: [["p", recipientHex]], - content: ciphertext, - }, - sk, - ); + // Guard the whole thing: nip19.decode throws on a malformed npub (passes the + // startsWith check but fails the bech32 checksum), and that previously threw + // *before* the publish try, escaping the caller. Best-effort — never throw. + // (review CS-3) try { + const recipientHex = recipient.startsWith("npub1") + ? (nip19.decode(recipient).data as string) + : recipient; + const ciphertext = nip04.encrypt(sk, recipientHex, content); + const event = finalizeEvent( + { + kind: 4, + created_at: Math.floor(Date.now() / 1000), + tags: [["p", recipientHex]], + content: ciphertext, + }, + sk, + ); await pool.publish(event); } catch (e) { - console.log(e); + console.log('dmUser failed for', recipient, '-', (e as any)?.message ?? e); } } From 1c16a2a4b7b1292d09183f9000b46106e640d53e Mon Sep 17 00:00:00 2001 From: Padreug Date: Sat, 27 Jun 2026 12:27:36 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(transport):=20harden=20RelayPool=20?= =?UTF-8?q?=E2=80=94=20connect=20timeout,=20stop-race,=20cross-relay=20ded?= =?UTF-8?q?up?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds three medium findings from the transport review into the RelayPool: - RP-4: connectOnce used the static Relay.connect(), which silently DROPS a {timeout} option in nostr-tools 2.20.0, so a black-holed TCP connect (SYN accepted, never upgraded) stalled that relay's loop for the OS socket timeout (minutes) with no retry. Now constructs the Relay and calls the instance .connect({ timeout: 5000 }), which honours the timeout → prompt reject + backoff. - RP-2: connectOnce didn't re-check `stopped` after the await. If stop() ran while a connect was in flight, the resolved socket re-armed subscriptions on a relay we meant to abandon, leaked the socket, and hung connectLoop (its promise never resolved because onclose never fired). Now drops the socket cleanly and resolves if stopped mid-connect. - RP-1: no cross-relay event dedup — a kind:24133 request published to N relays (the normal NIP-46 pattern), or re-delivered after a reconnect, drove the daemon handler + recordSigning N times, making rate caps bind ~N× tighter (fails closed, not open). Added a bounded (4000-id, ≈LRU) pool-wide seen-set; onevent fires at most once per event id. Closes the CS-4 replay vector too. Test: tests/relay-pool.test.ts asserts a duplicate event id is delivered once. relay 3 / nip46 1 / admin 2 green; daemon bundles; tsc at baseline. Refs: transport review RP-1/RP-2/RP-4, CS-4; #42 --- src/daemon/lib/relay-pool.ts | 61 +++++++++++++++++++++++++++++++++--- tests/relay-pool.test.ts | 32 +++++++++++++++++++ 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/daemon/lib/relay-pool.ts b/src/daemon/lib/relay-pool.ts index 6ca0c89..cb5f120 100644 --- a/src/daemon/lib/relay-pool.ts +++ b/src/daemon/lib/relay-pool.ts @@ -56,6 +56,8 @@ interface ActiveSub { const RECONNECT_BASE_MS = 1_000; const RECONNECT_CAP_MS = 10_000; // match #20's cap — cheap to retry a LAN relay +const CONNECT_TIMEOUT_MS = 5_000; // bound a black-holed TCP connect (review RP-4) +const SEEN_EVENT_CAP = 4_000; // bounded cross-relay event-id dedup set (review RP-1) /** * A single relay connection that owns its (re)connect loop. On every successful @@ -78,6 +80,9 @@ class ManagedRelay { public readonly url: string, private readonly registry: Map, private readonly log: (...args: any[]) => void, + /** Pool-wide first-seen gate: true the first time an event id is seen + * across ALL relays, false on a duplicate. (review RP-1) */ + private readonly markSeen: (id: string) => boolean, ) {} start(): void { @@ -122,7 +127,12 @@ class ManagedRelay { this.active.get(id)?.close(); try { const sub = this.relay.subscribe(s.filters, { - onevent: (e: Event) => s.onevent(e), + // Dedup across relays (and across reconnect re-deliveries): a + // kind:24133 request published to N relays must drive the daemon + // handler / recordSigning ONCE, not N times (review RP-1 / CS-4). + onevent: (e: Event) => { + if (this.markSeen(e.id)) s.onevent(e); + }, oneose: () => s.oneose?.(), }); this.active.set(id, sub); @@ -179,16 +189,39 @@ class ManagedRelay { private connectOnce(): Promise { return new Promise((resolve) => { void (async () => { + // enableReconnect:false — WE own reconnect, not nostr-tools. let relay: Relay; try { - // enableReconnect:false — WE own reconnect, not nostr-tools. - relay = await Relay.connect(this.url, { enableReconnect: false }); + relay = new Relay(this.url, { enableReconnect: false }); + } catch (e: any) { + this.log("relay construct failed:", e?.message ?? e); + resolve(false); + return; + } + try { + // Use the instance .connect({timeout}) rather than the static + // Relay.connect(), which silently DROPS a timeout option in + // nostr-tools 2.20.0. Without it a black-holed TCP connect + // (SYN accepted, never upgraded) stalls this loop for the OS + // socket timeout (minutes) with no retry (review RP-4). + await relay.connect({ timeout: CONNECT_TIMEOUT_MS }); } catch (e: any) { this.log("connect failed:", e?.message ?? e); + try { relay.close(); } catch { /* ignore */ } resolve(false); return; } + // If stop() ran while the connect was in flight, drop this socket + // cleanly — otherwise we'd re-arm subscriptions on a relay we mean + // to abandon, leak the socket, and hang connectLoop (its promise + // never resolves because onclose never fires). (review RP-2) + if (this.stopped) { + try { relay.close(); } catch { /* ignore */ } + resolve(true); + return; + } + this.relay = relay; this.connected = true; this.lastConnectedAt = Date.now(); @@ -265,6 +298,9 @@ export class RelayPool { private counter = 0; private heartbeatTimer: ReturnType | undefined; private lastHeartbeat = 0; + /** Insertion-ordered (≈LRU) bounded set of event ids already delivered to a + * subscription callback — the cross-relay/replay dedup gate (review RP-1). */ + private readonly seen: Set = new Set(); constructor( public readonly relayUrls: string[], @@ -273,12 +309,27 @@ export class RelayPool { this.log = opts.log ?? (() => {}); this.relays = relayUrls.map( (url) => - new ManagedRelay(url, this.registry, (...a: any[]) => - this.log(`[relay:${url}]`, ...a), + new ManagedRelay( + url, + this.registry, + (...a: any[]) => this.log(`[relay:${url}]`, ...a), + (id) => this.markSeen(id), ), ); } + /** Returns true the first time `id` is seen pool-wide, false on a duplicate; + * evicts the oldest id past the cap so this never grows unbounded. */ + private markSeen(id: string): boolean { + if (this.seen.has(id)) return false; + this.seen.add(id); + if (this.seen.size > SEEN_EVENT_CAP) { + const oldest = this.seen.values().next().value; + if (oldest !== undefined) this.seen.delete(oldest); + } + return true; + } + /** Start every relay's connect loop + (optionally) the sleep/wake heartbeat. */ start(): void { for (const r of this.relays) r.start(); diff --git a/tests/relay-pool.test.ts b/tests/relay-pool.test.ts index 148f144..5dbc76b 100644 --- a/tests/relay-pool.test.ts +++ b/tests/relay-pool.test.ts @@ -102,3 +102,35 @@ test("RelayPool.healthy() is false until the registry is subscribed on the wire pool.stop(); await relay.stop(); }); + +/** + * RP-1: a NIP-46 request published to multiple relays (or re-delivered after a + * reconnect) must drive the subscription callback ONCE — otherwise the daemon + * signs N times and over-counts rate caps. The pool dedups by event id. + */ +test("RelayPool delivers each event id at most once (#RP-1)", async () => { + const relay = new MockRelay(); + await relay.start(); + + const received: string[] = []; + const pool = new RelayPool([relay.url], { log: () => {} }); + pool.start(); + await pool.subscribeAwaitingEose([{ kinds: [24133], "#p": [PUBKEY] }], (e) => + received.push(e.id), + ); + + const ev = makeEvent(); + relay.inject(ev); + relay.inject(ev); // same id again (simulates a second relay / a replay) + await waitFor(() => received.includes(ev.id)); + await new Promise((r) => setTimeout(r, 200)); // give a 2nd delivery a chance + + assert.equal( + received.filter((id) => id === ev.id).length, + 1, + "a duplicate event id is delivered to the callback only once", + ); + + pool.stop(); + await relay.stop(); +}); From edf1ddc7da7e81ea033ec74e0537e0b4395e3105 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sat, 27 Jun 2026 13:08:37 +0200 Subject: [PATCH 4/4] =?UTF-8?q?fix(admin):=20clear=20pending=20callbacks?= =?UTF-8?q?=20on=20ACL=20timeout/response=20=E2=80=94=20stop=20the=20leak?= =?UTF-8?q?=20(review=20AD-1/CS-2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit requestPermission's 10s approval timeout called resolve(undefined) but never clearPending, and a single admin's response cleared only its own id. So every timed-out approval, and (with multiple admin npubs) the non-responding admins' entries, leaked permanently in transport.pending — each retaining its closure over remotePubkey/keyName/method/the serialized sign_event payload, so growth tracked signing traffic. Scope: the admin-DM auth path (only when no web baseUrl is set). - Collect every issued request id; a single `finish()` resolves once and clears ALL of them, on both timeout and the first response. - A `settled` latch makes late/duplicate responses no-ops — which also stops a late 'always' approval from running allowAllRequestsFromKey after the request already resolved (review AD-2). - Guard nip19.decode so a malformed admin npub skips that admin instead of throwing through the loop. - Defense-in-depth: bound transport.pending at 1000 (evict oldest) so any other un-cleared path can't grow it without limit. tsc at baseline; daemon bundles; admin + nip46 suites green. Refs: transport review AD-1/CS-2/AD-2; #42 --- src/daemon/admin/index.ts | 33 +++++++++++++++++++++++++++------ src/daemon/nip46/transport.ts | 8 ++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/daemon/admin/index.ts b/src/daemon/admin/index.ts index 569e1c3..e19aaa7 100644 --- a/src/daemon/admin/index.ts +++ b/src/daemon/admin/index.ts @@ -414,13 +414,33 @@ class AdminInterface { return new Promise((resolve) => { console.log(`requesting permission for`, keyName, { remotePubkey, method }); + const ids: string[] = []; + let settled = false; + // Resolve once and ALWAYS clear every pending sendRequest callback — + // on timeout AND on the first admin response. Previously the timeout + // cleared nothing and a single admin's response cleared only its own + // id, so every timed-out request and (with multiple admins) the + // non-responding admins' entries leaked in transport.pending for the + // process lifetime (review AD-1/CS-2). The `settled` latch also stops + // a late approval from acting after the request resolved (review AD-2). + const finish = (value: boolean | undefined) => { + if (settled) return; + settled = true; + for (const id of ids) this.transport.clearPending(id); + resolve(value); + }; + // If an admin doesn't respond within 10 seconds, report timeout. - setTimeout(() => { - resolve(undefined); - }, 10000); + setTimeout(() => finish(undefined), 10000); for (const npub of this.npubs) { - const adminPubkey = nip19.decode(npub).data as string; + let adminPubkey: string; + try { + adminPubkey = nip19.decode(npub).data as string; + } catch { + console.log(`skipping malformed admin npub: ${npub}`); + continue; + } const params = JSON.stringify({ keyName, remotePubkey, @@ -436,17 +456,18 @@ class AdminInterface { 'nip44', NIP46_ADMIN_RESPONSE_KIND, (res) => { - this.transport.clearPending(id); + if (settled) return; // ignore late / duplicate responses this.requestPermissionResponse( remotePubkey, keyName, method, param, - resolve, + finish, res ); } ); + ids.push(id); } }); } diff --git a/src/daemon/nip46/transport.ts b/src/daemon/nip46/transport.ts index b787cee..2a08a5f 100644 --- a/src/daemon/nip46/transport.ts +++ b/src/daemon/nip46/transport.ts @@ -132,6 +132,14 @@ export class Nip46Transport { ): string { const id = Math.random().toString(36).substring(2, 12); this.pending.set(id, cb); + // Defense-in-depth bound: callers (admin requestPermission) clear pending + // entries on resolve/timeout, but cap the map so any un-cleared path can't + // grow it without limit — evict the oldest (it would time out anyway). + // (review AD-1/CS-2) + if (this.pending.size > 1000) { + const oldest = this.pending.keys().next().value; + if (oldest !== undefined) this.pending.delete(oldest); + } const content = this.encrypt(remotePubkey, JSON.stringify({ id, method, params }), encryption); const event = finalizeEvent( { kind, created_at: Math.floor(Date.now() / 1000), tags: [["p", remotePubkey]], content },