aiolabs/bitspire
9
0
Fork 0
Code Issues 52 Pull requests 2 Projects Releases Packages Wiki Activity Actions

security: hash-based dedup on subscribe_payments push callbacks #50

New issue
Closed
opened 2026-06-13 22:03:02 +00:00 by padreug · 1 comment
padreug commented 2026-06-13 22:03:02 +00:00
Owner
Copy link

Migrated from aiolabs/lamassu-next#50 — opened by @padreug on 2026-05-26.\n\n## Problem

LnbitsClient.subscribePayments callbacks fire once per inbound push event from the relay. There is no de-duplication by payment_hash on the ATM side. If the relay rebroadcasts a settlement push (post-reconnect re-delivery, second relay push, or buggy server-side fan-out), watchInvoice's callback (apps/machine/src/services/lightning.ts:1006-1011) and the LNURL-withdraw session callback (apps/machine/src/services/lightning.ts:~720-731) will fire twice for the same settlement.

Whether the customer actually re-dispenses depends on state-machine idempotency that the cross-codebase audit did not fully trace. Even if the state machine is idempotent today, the callback layer should provide its own dedup primitive — both as defence in depth and because S3 (settlement receipts) will piggyback on the same callback path and needs hash-based dedup to verify each receipt only once.

Code sites

  • apps/machine/src/services/lightning.ts:watchInvoice — subscribePayments filtered by payment_hash. Callback fires onPaymentCallback(push.preimage) without dedup.
  • apps/machine/src/services/lightning.ts:generateLnurlWithdraw — subscribePayments filtered by {tag: 'withdraw', link_id}. Same callback shape, same gap.

Fix

Maintain a small bounded Set<string> (or LRU) of seen payment_hash values per-subscription. Skip the callback invocation if the hash is already present.

const seenHashes = new Set<string>()
const subId = await lnbits.subscribePayments(
  walletId,
  { /* filter */ },
  (push) => {
    if (push.payment_hash && seenHashes.has(push.payment_hash)) return
    if (push.payment_hash) seenHashes.add(push.payment_hash)
    // existing handler
  },
)

Prune on unsubscribe() to bound memory.

Acceptance

  • Two pushes with the same payment_hash on the same subscription invoke the callback exactly once.
  • Pushes with different hashes are unaffected.
  • Cleanup on unsubscribe drops the seen-set.
  • LNURL-withdraw session also wraps its callback in dedup.

Why now

  • Blocks shipping S3 (NIP-57-style settlement receipts) in aiolabs/satmachineadmin#17 cleanly — that work subscribes to receipt events on the same relay and needs each receipt processed once.
  • Pairs naturally with the reply-sig-verify work (filed alongside) since both touch LnbitsClient's relay reply handling.

References

  • Cross-codebase review 2026-05-26, coherence agent finding #10 + security agent finding #4.
  • Estimated effort: 2h.
> _Migrated from [aiolabs/lamassu-next#50](https://git.atitlan.io/aiolabs/lamassu-next/issues/50) — opened by @padreug on 2026-05-26._\n\n## Problem `LnbitsClient.subscribePayments` callbacks fire once per inbound push event from the relay. There is **no de-duplication by `payment_hash` on the ATM side**. If the relay rebroadcasts a settlement push (post-reconnect re-delivery, second relay push, or buggy server-side fan-out), `watchInvoice`'s callback (`apps/machine/src/services/lightning.ts:1006-1011`) and the LNURL-withdraw session callback (`apps/machine/src/services/lightning.ts:~720-731`) will fire twice for the same settlement. Whether the customer actually re-dispenses depends on state-machine idempotency that the cross-codebase audit did not fully trace. Even if the state machine is idempotent today, the callback layer should provide its own dedup primitive — both as defence in depth and because **S3 (settlement receipts)** will piggyback on the same callback path and needs hash-based dedup to verify each receipt only once. ## Code sites - `apps/machine/src/services/lightning.ts:watchInvoice` — `subscribePayments` filtered by `payment_hash`. Callback fires `onPaymentCallback(push.preimage)` without dedup. - `apps/machine/src/services/lightning.ts:generateLnurlWithdraw` — `subscribePayments` filtered by `{tag: 'withdraw', link_id}`. Same callback shape, same gap. ## Fix Maintain a small bounded `Set<string>` (or LRU) of seen `payment_hash` values per-subscription. Skip the callback invocation if the hash is already present. ```ts const seenHashes = new Set<string>() const subId = await lnbits.subscribePayments( walletId, { /* filter */ }, (push) => { if (push.payment_hash && seenHashes.has(push.payment_hash)) return if (push.payment_hash) seenHashes.add(push.payment_hash) // existing handler }, ) ``` Prune on `unsubscribe()` to bound memory. ## Acceptance - [ ] Two pushes with the same `payment_hash` on the same subscription invoke the callback exactly once. - [ ] Pushes with different hashes are unaffected. - [ ] Cleanup on unsubscribe drops the seen-set. - [ ] LNURL-withdraw session also wraps its callback in dedup. ## Why now - Blocks shipping S3 (NIP-57-style settlement receipts) in `aiolabs/satmachineadmin#17` cleanly — that work subscribes to receipt events on the same relay and needs each receipt processed once. - Pairs naturally with the reply-sig-verify work (filed alongside) since both touch `LnbitsClient`'s relay reply handling. ## References - Cross-codebase review 2026-05-26, coherence agent finding #10 + security agent finding #4. - Estimated effort: 2h.
padreug commented 2026-06-13 22:05:08 +00:00
Author
Owner
Copy link

@padreug commented on 2026-05-26 (lamassu-next#50):

Shipped on dev at commit 8c4be01. Builds on 0dcbe44 (#49): with ev.id Schnorr-verified by the guard, it's now safe as a client-global dedup key.

Implementation — two tiers:

  1. seenEventIds: Set<string> on LnbitsClient (FIFO-capped at 1000). Checked in handleReply right after the #49 guard. Catches exact-replay (same bytes re-delivered post-reconnect, etc.).
  2. seenPaymentHashes: Set<string> per ActiveSubscription (FIFO-capped at 500). Checked at the push-dispatch site. Catches logical duplicates — server fan-out across two relays produces different ev.ids carrying the same payment_hash.

Shared recordSeen() helper for FIFO eviction (Sets preserve insertion order in JS, so values().next() gives the oldest).

Tests — 11/11 passing, 5 new for #50:

  • does not poison the seenEventIds cache with a forged event — this is the test that depends on #49's guard. A refactor that drops isAuthenticServerEvent would let the forged event's id into seenEventIds. Bitspire session's call: this is where the guard's contribution becomes load-bearing, not in the original "drops a forged event without resolving any pending RPC" test (which was indistinguishable from the MAC-failure path).
  • Exact-replay: same event injected twice → onPush fires once via seenEventIds.
  • Distinct ev.ids with same payment_hash → onPush fires once via per-sub seenPaymentHashes. Sanity-asserts ev1.id !== ev2.id to prove ev.id dedup wouldn't catch this case.
  • Distinct payment_hashes → fires twice (negative dedup).
  • Per-sub isolation: same hash on two subs → both fire (state is per-sub).

Why two tiers, not one:

  • ev.id dedup alone misses server fan-out (different ev.ids for same logical settlement).
  • payment_hash dedup alone misses exact-replay before the decrypt cost is paid (cheap rejection at the event-id layer keeps the hot path tight).
  • Both layers together handle both classes; the test for "two distinct ev.ids with same payment_hash" proves the ev.id layer correctly doesn't fire while the hash layer does.

Why this matters in production: without dedup, a relay rebroadcast of a settlement push invokes 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.

Closing.

> _@padreug commented on 2026-05-26 ([lamassu-next#50](https://git.atitlan.io/aiolabs/lamassu-next/issues/50#issuecomment-1126)):_ Shipped on `dev` at commit `8c4be01`. Builds on `0dcbe44` (#49): with `ev.id` Schnorr-verified by the guard, it's now safe as a client-global dedup key. **Implementation — two tiers:** 1. **`seenEventIds: Set<string>`** on `LnbitsClient` (FIFO-capped at 1000). Checked in `handleReply` right after the `#49` guard. Catches exact-replay (same bytes re-delivered post-reconnect, etc.). 2. **`seenPaymentHashes: Set<string>`** per `ActiveSubscription` (FIFO-capped at 500). Checked at the push-dispatch site. Catches logical duplicates — server fan-out across two relays produces different `ev.id`s carrying the same `payment_hash`. Shared `recordSeen()` helper for FIFO eviction (Sets preserve insertion order in JS, so `values().next()` gives the oldest). **Tests — 11/11 passing**, 5 new for #50: - `does not poison the seenEventIds cache with a forged event` — **this is the test that depends on #49's guard.** A refactor that drops `isAuthenticServerEvent` would let the forged event's id into `seenEventIds`. Bitspire session's call: this is where the guard's contribution becomes load-bearing, not in the original "drops a forged event without resolving any pending RPC" test (which was indistinguishable from the MAC-failure path). - Exact-replay: same event injected twice → `onPush` fires once via `seenEventIds`. - Distinct ev.ids with same `payment_hash` → `onPush` fires once via per-sub `seenPaymentHashes`. Sanity-asserts `ev1.id !== ev2.id` to prove ev.id dedup *wouldn't* catch this case. - Distinct payment_hashes → fires twice (negative dedup). - Per-sub isolation: same hash on two subs → both fire (state is per-sub). **Why two tiers, not one:** - ev.id dedup alone misses server fan-out (different ev.ids for same logical settlement). - payment_hash dedup alone misses exact-replay before the decrypt cost is paid (cheap rejection at the event-id layer keeps the hot path tight). - Both layers together handle both classes; the test for "two distinct ev.ids with same payment_hash" proves the ev.id layer correctly *doesn't* fire while the hash layer does. **Why this matters in production:** without dedup, a relay rebroadcast of a settlement push invokes `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. Closing.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
dev
feat/rpi4-target
feat/rpi5-apex-hal
fix/sintra-timezone
wip/rpi4-sketch
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
aiolabs/bitspire#50
No description provided.