diff --git a/packages/lnbits/src/__tests__/client.test.ts b/packages/lnbits/src/__tests__/client.test.ts index eaa8aa8..187e948 100644 --- a/packages/lnbits/src/__tests__/client.test.ts +++ b/packages/lnbits/src/__tests__/client.test.ts @@ -21,6 +21,7 @@ import { type NostrClient, } from '@bitSpire/nostr-client' import { LnbitsClient, isAuthenticServerEvent } from '../client.js' +import type { LnbitsPayment } from '../types.js' const KIND_LNBITS_RPC = 21000 @@ -288,4 +289,285 @@ describe('LnbitsClient.handleReply wiring', () => { // Our test installs its own resolve, so the entry remains; that's // fine, we only need to assert resolve fired with the right payload. }) + + it('does not poison the seenEventIds cache with a forged event', () => { + // This is the test scenario where the #49 guard's contribution + // actually shows up: ev.id is the dedup key for the client-global + // exact-replay cache. WITHOUT the guard, an attacker could publish + // a forged kind-21000 event with an attacker-chosen ev.id, and + // handleReply would add that id to `seenEventIds`. Later, a + // legitimate event with the same id (theoretical — attacker would + // need to predict it) would be silently dropped. WITH the guard, + // forged events never reach `seenEventIds.add()`. + // + // This is precisely the "ev.id load-bearing for dedup" scenario the + // bitspire review pointed at. The earlier "drops a forged event + // without resolving any pending RPC" test was indistinguishable + // from the MAC-failure path (both reject); this test is not, + // because MAC-failure would still let the event through to the + // ev.id cache if the guard were absent. + const { client, serverIdentity, triggerEvent } = setupClient() + + const attackerKey = generateSecretKey() + const forged = forgeEvent( + { + kind: KIND_LNBITS_RPC, + content: 'irrelevant', + tags: [], + created_at: Math.floor(Date.now() / 1000), + }, + serverIdentity.publicKey, + attackerKey, + ) + + triggerEvent(forged) + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((client as any).seenEventIds.size).toBe(0) + }) +}) + +/** + * Dedup tests for the subscribe_payments push path (issue #50). + * + * Two layers: + * 1. Client-global `seenEventIds` — catches exact-replay of the same + * kind-21000 event (same bytes, same Schnorr sig). Tested via + * "fires once when same event injected twice." + * 2. Per-subscription `seenPaymentHashes` — catches different events + * carrying the same `payment_hash` (server fan-out across relays, + * or a server-side resend with a fresh `created_at`). Tested via + * "fires once when two distinct ev.ids carry the same payment_hash." + * + * Both layers must hold for `dispenseCash()` to not double-fire in the + * state machine. + */ +describe('LnbitsClient subscribe-payments dedup (#50)', () => { + function makeMockNostr(): { + nostr: NostrClient + triggerEvent: (ev: NostrEvent) => void + } { + let captured: ((ev: NostrEvent) => void) | null = null + const nostr = { + subscribe( + _filters: unknown, + opts: { onEvent: (ev: NostrEvent) => void }, + ): string { + captured = opts.onEvent + return 'mock-sub-id' + }, + publish: async () => {}, + unsubscribe: async () => {}, + } as unknown as NostrClient + return { + nostr, + triggerEvent: (ev) => { + if (!captured) throw new Error('handleReply not wired yet') + captured(ev) + }, + } + } + + function makeIdentity(): MachineIdentity { + const sk = generateSecretKey() + return { privateKey: sk, publicKey: getPublicKey(sk), npub: '' } + } + + /** Pre-register a subscription with a counting `onPush`. */ + function preregisterSub( + client: LnbitsClient, + subId: string, + ): { received: LnbitsPayment[] } { + const received: LnbitsPayment[] = [] + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ;(client as any).subscriptions.set(subId, { + subscriptionId: subId, + requestId: 'test-req', + relaySubId: 'mock-sub', + onPush: (p: LnbitsPayment) => { + received.push(p) + }, + onClose: undefined, + seenPaymentHashes: new Set(), + }) + return { received } + } + + /** Build a server-signed kind-21000 push event for the given subscription. */ + function buildPushEvent(opts: { + serverIdentity: MachineIdentity + recipientPubkey: string + subscriptionId: string + paymentHash: string + createdAt?: number + }): NostrEvent { + const payment: LnbitsPayment = { + checking_id: opts.paymentHash, + payment_hash: opts.paymentHash, + wallet_id: 'wallet-x', + amount: 1000, + fee: 0, + bolt11: 'lnbc...', + payment_request: 'lnbc...', + status: 'success', + time: new Date().toISOString(), + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + } + const responsePayload = JSON.stringify({ + status: 'OK', + request_id: 'test-req', + subscription_id: opts.subscriptionId, + data: { payment }, + }) + const ciphertext = encryptContentV2( + opts.serverIdentity, + opts.recipientPubkey, + responsePayload, + ) + return finalizeEvent( + { + kind: KIND_LNBITS_RPC, + content: ciphertext, + tags: [['p', opts.recipientPubkey]], + created_at: opts.createdAt ?? Math.floor(Date.now() / 1000), + }, + opts.serverIdentity.privateKey, + ) + } + + it('fires onPush once when the same event is injected twice (exact-replay dedup)', () => { + const serverIdentity = makeIdentity() + const recipientIdentity = makeIdentity() + const { nostr, triggerEvent } = makeMockNostr() + const client = new LnbitsClient({ + serverPubkey: serverIdentity.publicKey, + relays: ['ws://test/'], + }) + client.initialize(nostr, recipientIdentity) + + const { received } = preregisterSub(client, 'sub-1') + + const ev = buildPushEvent({ + serverIdentity, + recipientPubkey: recipientIdentity.publicKey, + subscriptionId: 'sub-1', + paymentHash: 'hash-aaa', + }) + // Same bytes both times — same ev.id, same payment_hash. + triggerEvent(ev) + triggerEvent(ev) + + expect(received).toHaveLength(1) + expect(received[0]!.payment_hash).toBe('hash-aaa') + }) + + it('fires onPush once when two distinct ev.ids carry the same payment_hash', () => { + const serverIdentity = makeIdentity() + const recipientIdentity = makeIdentity() + const { nostr, triggerEvent } = makeMockNostr() + const client = new LnbitsClient({ + serverPubkey: serverIdentity.publicKey, + relays: ['ws://test/'], + }) + client.initialize(nostr, recipientIdentity) + + const { received } = preregisterSub(client, 'sub-1') + + // Two distinct events (different created_at → different ev.id) but + // same payment_hash. This models server fan-out across two relays + // or a server-side resend. payment_hash dedup must catch it. + const now = Math.floor(Date.now() / 1000) + const ev1 = buildPushEvent({ + serverIdentity, + recipientPubkey: recipientIdentity.publicKey, + subscriptionId: 'sub-1', + paymentHash: 'hash-bbb', + createdAt: now, + }) + const ev2 = buildPushEvent({ + serverIdentity, + recipientPubkey: recipientIdentity.publicKey, + subscriptionId: 'sub-1', + paymentHash: 'hash-bbb', + createdAt: now + 1, + }) + expect(ev1.id).not.toBe(ev2.id) // sanity: ev.id dedup would NOT catch this + triggerEvent(ev1) + triggerEvent(ev2) + + expect(received).toHaveLength(1) + expect(received[0]!.payment_hash).toBe('hash-bbb') + }) + + it('fires onPush for each distinct payment_hash (negative dedup case)', () => { + const serverIdentity = makeIdentity() + const recipientIdentity = makeIdentity() + const { nostr, triggerEvent } = makeMockNostr() + const client = new LnbitsClient({ + serverPubkey: serverIdentity.publicKey, + relays: ['ws://test/'], + }) + client.initialize(nostr, recipientIdentity) + + const { received } = preregisterSub(client, 'sub-1') + + const ev1 = buildPushEvent({ + serverIdentity, + recipientPubkey: recipientIdentity.publicKey, + subscriptionId: 'sub-1', + paymentHash: 'hash-1', + }) + const ev2 = buildPushEvent({ + serverIdentity, + recipientPubkey: recipientIdentity.publicKey, + subscriptionId: 'sub-1', + paymentHash: 'hash-2', + createdAt: Math.floor(Date.now() / 1000) + 2, + }) + triggerEvent(ev1) + triggerEvent(ev2) + + expect(received).toHaveLength(2) + expect(received.map((p) => p.payment_hash)).toEqual(['hash-1', 'hash-2']) + }) + + it('keeps dedup state per-subscription (one sub seeing a hash does not silence another)', () => { + const serverIdentity = makeIdentity() + const recipientIdentity = makeIdentity() + const { nostr, triggerEvent } = makeMockNostr() + const client = new LnbitsClient({ + serverPubkey: serverIdentity.publicKey, + relays: ['ws://test/'], + }) + client.initialize(nostr, recipientIdentity) + + const a = preregisterSub(client, 'sub-A') + const b = preregisterSub(client, 'sub-B') + + // Build a push event for each subscription, both with the same hash. + // Different ev.ids (different subscription_id payload → different + // ciphertext → different content → different ev.id). + const evA = buildPushEvent({ + serverIdentity, + recipientPubkey: recipientIdentity.publicKey, + subscriptionId: 'sub-A', + paymentHash: 'hash-shared', + }) + const evB = buildPushEvent({ + serverIdentity, + recipientPubkey: recipientIdentity.publicKey, + subscriptionId: 'sub-B', + paymentHash: 'hash-shared', + createdAt: Math.floor(Date.now() / 1000) + 1, + }) + triggerEvent(evA) + triggerEvent(evB) + + // Each subscription sees its own push exactly once. + expect(a.received).toHaveLength(1) + expect(b.received).toHaveLength(1) + expect(a.received[0]!.payment_hash).toBe('hash-shared') + expect(b.received[0]!.payment_hash).toBe('hash-shared') + }) }) diff --git a/packages/lnbits/src/client.ts b/packages/lnbits/src/client.ts index ae40019..0a18c44 100644 --- a/packages/lnbits/src/client.ts +++ b/packages/lnbits/src/client.ts @@ -93,8 +93,31 @@ interface ActiveSubscription { relaySubId: string onPush: PaymentPushCallback onClose?: SubscriptionCloseCallback + /** + * Hash-based dedup. The relay may rebroadcast a settlement push after + * reconnect; the server may also fan-out to multiple relays which then + * forward both back to us. Either case fires `onPush` twice for the + * same logical settlement — and once the state machine receives the + * second invocation it can drive a second `dispenseCash()`. We dedup + * by `payment_hash` (the only field guaranteed-unique per settlement) + * before invoking `onPush`. Scoped per-subscription so a different + * subscription seeing the same hash for an independent reason still + * fires its own callback. + */ + seenPaymentHashes: Set } +/** + * Soft cap on the client-global `seenEventIds` set (defence against + * unbounded growth on long-running ATMs). 1000 entries × 64 hex chars + * ≈ 64 kB — trivially small. FIFO eviction; Sets preserve insertion + * order in JS so `values().next()` gives the oldest entry. + */ +const SEEN_EVENT_IDS_MAX = 1000 + +/** Soft cap on per-subscription `seenPaymentHashes`. */ +const SEEN_PAYMENT_HASHES_MAX = 500 + export class LnbitsClient { private readonly config: Required private nostr: NostrClient | null = null @@ -106,6 +129,19 @@ export class LnbitsClient { >() private readonly subscriptions = new Map() private relaySubIdForReplies?: string + /** + * Client-global dedup on inbound event ids. Catches exact-replay (relay + * re-delivery of the same kind-21000 event after reconnect) before any + * decrypt / parse work. Load-bearing for replay safety only because + * `isAuthenticServerEvent` runs first: without the Schnorr verification + * in #49, an attacker could publish a forged event with an arbitrary + * `ev.id` and pre-poison this cache, causing later legitimate events + * with the same id to be silently dropped. With the guard, every entry + * in this set is by construction an event the configured server signed. + * + * Bounded by `SEEN_EVENT_IDS_MAX` with FIFO eviction. + */ + private readonly seenEventIds = new Set() constructor(config: LnbitsConfig) { this.config = { @@ -217,6 +253,7 @@ export class LnbitsClient { relaySubId: this.relaySubIdForReplies ?? '', onPush, onClose, + seenPaymentHashes: new Set(), } // We don't have subscriptionId yet — index by requestId temporarily. // The reply listener will move it under the real subscriptionId once @@ -484,6 +521,14 @@ export class LnbitsClient { private handleReply(ev: NostrEvent): void { if (!this.identity) return if (!isAuthenticServerEvent(ev, this.config.serverPubkey)) return + // Exact-replay dedup. Skip if we've already processed this event id. + // Safe to trust `ev.id` here because `isAuthenticServerEvent` just + // Schnorr-verified the event (`verifyEvent` recomputes the id and + // confirms it matches the signed pubkey + body). Without that + // guarantee an attacker could pre-poison this set with chosen ids. + if (this.seenEventIds.has(ev.id)) return + this.recordSeenEventId(ev.id) + let plaintext: string try { plaintext = decryptContentV2(this.identity, this.config.serverPubkey, ev.content) @@ -513,9 +558,38 @@ export class LnbitsClient { this.subscriptions.delete(subId) sub.onClose?.(data.reason ?? 'ttl') } else if (data.payment) { + // Per-subscription hash dedup. The same logical settlement can + // arrive twice via different ev.ids (different relays in the + // fan-out, or a server-side resend). The event-id dedup above + // doesn't catch those; payment_hash does. Scoped per-sub so a + // different subscription seeing the hash for its own reason + // still fires. + const hash = data.payment.payment_hash + if (hash && sub.seenPaymentHashes.has(hash)) return + if (hash) recordSeen(sub.seenPaymentHashes, hash, SEEN_PAYMENT_HASHES_MAX) sub.onPush(data.payment) } } } } + + /** Append `id` with FIFO eviction when the cap is reached. */ + private recordSeenEventId(id: string): void { + recordSeen(this.seenEventIds, id, SEEN_EVENT_IDS_MAX) + } +} + +/** + * Append to a bounded `Set` with FIFO eviction. Sets preserve + * insertion order in JS, so `values().next().value` returns the oldest + * entry. Factored out as a module-level helper so both the client-global + * `seenEventIds` cap and the per-subscription `seenPaymentHashes` cap + * share one implementation. + */ +function recordSeen(set: Set, value: string, max: number): void { + if (set.size >= max) { + const oldest = set.values().next().value + if (oldest !== undefined) set.delete(oldest) + } + set.add(value) }