security(lnbits): two-tier hash dedup on subscribe-payments push callbacks

Closes aiolabs/lamassu-next#50. Builds on #49 (commit 0dcbe44):
isAuthenticServerEvent now Schnorr-verifies inbound events, so ev.id
is by construction the id of a server-signed event and can be used
as a dedup key without risk of attacker pre-poisoning.

Two layers:

1. Client-global `seenEventIds` (Set<string>, FIFO cap 1000) in
   `LnbitsClient.handleReply`. Skips events whose id has been seen.
   Catches exact-replay — relay re-delivers the same bytes after
   reconnect, or any other source that emits a bit-identical event.
   Without #49's guard, an attacker could pre-poison this set with
   chosen ids; with the guard, every entry is a server-signed event.

2. Per-subscription `seenPaymentHashes` (Set<string>, FIFO cap 500)
   on `ActiveSubscription`. Skips `onPush` invocations whose payment
   hash has been seen on the same subscription. Catches logical
   duplicates — server fan-out across two relays produces two
   different ev.ids carrying the same payment_hash, which the
   client-global ev.id layer can't dedup but the per-sub hash layer
   does. Scoped per-sub so independent subscriptions seeing the same
   hash for their own reasons still fire.

Why this matters in production: without dedup, a relay rebroadcast of
a settlement push would invoke `onPush` twice. The state machine's
`watchInvoice` callback resolves on the first push and unsubscribes,
but a race between the second push and the unsubscribe round-trip
could land a second `dispenseCash()` for the same cash-out — customer
walks away with double the cash. Per-sub `payment_hash` is the only
field guaranteed-unique per settlement (the customer's payment hash
is fixed at invoice creation), so it's the right dedup key.

5 new tests:
- forged event doesn't poison `seenEventIds` (proves guard + dedup
  interact: a refactor that removes either #49's guard or this
  patch's `seenEventIds.add()` ordering would fail this test)
- exact-replay: same event injected twice, callback fires once
- distinct ev.ids with same payment_hash, callback fires once
  (per-sub hash dedup; ev.id dedup doesn't apply)
- distinct payment_hashes fire normally (negative dedup case)
- per-sub isolation: same hash on two subs fires both callbacks

Total: 11/11 lnbits package tests pass. Workspace typecheck clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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<string>(),
+    })
+    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')
+  })
 })

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<string>
 }
 
+/**
+ * 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<LnbitsConfig>
   private nostr: NostrClient | null = null
@@ -106,6 +129,19 @@ export class LnbitsClient {
   >()
   private readonly subscriptions = new Map<string, ActiveSubscription>()
   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<string>()
 
   constructor(config: LnbitsConfig) {
     this.config = {
@@ -217,6 +253,7 @@ export class LnbitsClient {
       relaySubId: this.relaySubIdForReplies ?? '',
       onPush,
       onClose,
+      seenPaymentHashes: new Set<string>(),
     }
     // 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<string>` 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<string>, value: string, max: number): void {
+  if (set.size >= max) {
+    const oldest = set.values().next().value
+    if (oldest !== undefined) set.delete(oldest)
+  }
+  set.add(value)
 }