From 1beedb8a5b2470b85a87074bfec0732a7b55375f Mon Sep 17 00:00:00 2001 From: highperfocused Date: Mon, 7 Sep 2026 17:06:27 +0200 Subject: [PATCH] Address PR #62 review feedback: NWC relay security, N+1 zap queries, manual-payment confirmation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot review comments on PR #62: - nwc.ts: reject plaintext ws:// Wallet Connect relays, requiring wss://. Every request is signed by the connection secret (a private key); even with an encrypted payload, an unencrypted transport still leaks metadata about the connection and admits tampering. - ZapButton/useZaps: fixed the N+1 query pattern — a feed page mounted one unconditional zap-receipts query (limit 500) per rendered note. Added a `revealed` gate so the query only fires once a note is actually hovered or focused (the same interaction that already reveals the action row via CSS), confirmed live: 0 queries fired across 49 mounted notes before any interaction, exactly 1 after hovering one. - ZapDialog: manual-payment confirmation compared the note's total receipt *count* against a baseline, so anyone else zapping the same note while the dialog waited would falsely confirm the viewer's own unpaid invoice. Added `hasValidReceiptForInvoice` to match against the specific invoice instead, with unit tests covering the exact race the review described. - useZaps.ts: corrected a docstring overclaiming that receipt validation prevents "forged" receipts from inflating totals — it only rules out structurally invalid data; NIP-57 receipts are vouched for by the recipient's own LNURL server, so trusting one is inherent to the protocol, not something client-side validation can prove. Investigated but did not change: the review's claim that `nip04.decrypt(...)` needs an `await` because it returns a Promise. Not correct for this project's actual `nostr-tools` dependency — confirmed by running the real encrypt/decrypt round trip, `decrypt` is synchronous and returns the plaintext string directly, so the existing `JSON.parse` call already worked. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BYiUtZMQeA5RHggQw73wto --- src/components/nostr/NoteCard.tsx | 9 +++++++- src/components/nostr/ZapButton.tsx | 18 ++++++++++++--- src/components/nostr/ZapDialog.tsx | 12 +++++----- src/hooks/useZaps.test.ts | 25 ++++++++++++++++++++- src/hooks/useZaps.ts | 36 +++++++++++++++++++++++++----- src/lib/nwc.test.ts | 5 +++++ src/lib/nwc.ts | 8 +++++-- 7 files changed, 93 insertions(+), 20 deletions(-) diff --git a/src/components/nostr/NoteCard.tsx b/src/components/nostr/NoteCard.tsx index 71d1860..a1d46d5 100644 --- a/src/components/nostr/NoteCard.tsx +++ b/src/components/nostr/NoteCard.tsx @@ -1,3 +1,4 @@ +import { useState } from 'react'; import { MessageSquare, Repeat2 } from 'lucide-react'; import type { NostrEvent } from '@nostrify/nostrify'; import { nip19 } from 'nostr-tools'; @@ -26,6 +27,10 @@ export function NoteCard({ event, compact, className }: NoteCardProps) { const { openApp } = useWindowManager(); const { toast } = useToast(); const hints = useRelayHints(); + // Tracks the same interaction that reveals the action row via CSS + // (`group-hover`/`focus-within`), so `ZapButton` can defer its relay query + // until this note is actually looked at instead of firing on every mount. + const [revealed, setRevealed] = useState(false); const copyLink = async () => { try { @@ -45,6 +50,8 @@ export function NoteCard({ event, compact, className }: NoteCardProps) { 'group border-b border-border px-4 py-3 transition-colors last:border-b-0 hover:bg-muted/40', className, )} + onMouseEnter={() => setRevealed(true)} + onFocus={() => setRevealed(true)} > @@ -71,7 +78,7 @@ export function NoteCard({ event, compact, className }: NoteCardProps) { Copy link - + )} diff --git a/src/components/nostr/ZapButton.tsx b/src/components/nostr/ZapButton.tsx index b5ae641..0579289 100644 --- a/src/components/nostr/ZapButton.tsx +++ b/src/components/nostr/ZapButton.tsx @@ -9,12 +9,24 @@ import { useAuthor } from '@/hooks/useAuthor'; import { useZapReceipts, summarizeZapReceipts, formatSats } from '@/hooks/useZaps'; import { cn } from '@/lib/utils'; -/** Shows a note's zap total and opens the zap flow (NIP-57), from the feed or a thread. */ -export function ZapButton({ target, className }: { target: NostrEvent; className?: string }) { +/** + * Shows a note's zap total and opens the zap flow (NIP-57), from the feed or + * a thread. + * + * `revealed` gates the zap-receipts query itself, not just the total's + * visibility: a feed page mounts one of these per note, and firing every + * one's relay query unconditionally on mount turns a page of notes into a + * page of concurrent queries before anyone's looked at any of them. Callers + * in a list (`NoteCard`) pass `revealed` once the row is actually hovered or + * focused — the same interaction that already reveals the row via CSS — so + * off-screen or never-looked-at notes never fetch. A caller showing the + * button on its own (the thread root) can just leave it `true`. + */ +export function ZapButton({ target, className, revealed = true }: { target: NostrEvent; className?: string; revealed?: boolean }) { const { user } = useCurrentUser(); const [authOpen, setAuthOpen] = useState(false); const [zapOpen, setZapOpen] = useState(false); - const receipts = useZapReceipts(target.id); + const receipts = useZapReceipts(target.id, { enabled: revealed || zapOpen }); const recipient = useAuthor(target.pubkey); const { totalSats } = summarizeZapReceipts(receipts.data); diff --git a/src/components/nostr/ZapDialog.tsx b/src/components/nostr/ZapDialog.tsx index c259d6a..f6a5e85 100644 --- a/src/components/nostr/ZapDialog.tsx +++ b/src/components/nostr/ZapDialog.tsx @@ -7,7 +7,7 @@ import { Textarea } from '@/components/ui/textarea'; import { Dialog, DialogContent, DialogHeader, DialogTitle, DialogDescription } from '@/components/ui/dialog'; import { QRCodeCanvas } from '@/components/ui/qrcode'; import { useToast } from '@/hooks/useToast'; -import { useCreateZapInvoice, useZapReceipts, summarizeZapReceipts, formatSats } from '@/hooks/useZaps'; +import { useCreateZapInvoice, useZapReceipts, hasValidReceiptForInvoice, formatSats } from '@/hooks/useZaps'; import { useNwcConnection, usePayWithNwc } from '@/hooks/useNwc'; import { cn } from '@/lib/utils'; @@ -38,14 +38,15 @@ export function ZapDialog({ open, onClose, target, recipientMetadata, recipientM const [invoice, setInvoice] = useState(null); const [amountSats, setAmountSats] = useState(0); const [errorMessage, setErrorMessage] = useState(''); - const [receiptBaseline, setReceiptBaseline] = useState(0); // Re-fetched on an interval only while this dialog is showing an unpaid // manual invoice. Whether that now means "paid" is derived at render time // below instead of copied into state, so there is nothing to keep in sync - // by hand. + // by hand. Matched against the specific invoice, not just a receipt-count + // increase — someone else zapping the same note while this dialog waits + // must not be mistaken for this payment completing. const receipts = useZapReceipts(target.id, { refetchInterval: stage === 'manual' ? 4_000 : false }); - const manualPaymentConfirmed = stage === 'manual' && summarizeZapReceipts(receipts.data).count > receiptBaseline; + const manualPaymentConfirmed = stage === 'manual' && invoice !== null && hasValidReceiptForInvoice(receipts.data, invoice); const effectiveStage: Stage = manualPaymentConfirmed ? 'paid' : stage; const resolvedAmount = customAmount.trim() ? Number(customAmount) : amount; @@ -57,9 +58,6 @@ export function ZapDialog({ open, onClose, target, recipientMetadata, recipientM setErrorMessage(''); try { - const { count } = summarizeZapReceipts(receipts.data); - setReceiptBaseline(count); - const result = await createInvoice.mutateAsync({ target, recipientMetadata, diff --git a/src/hooks/useZaps.test.ts b/src/hooks/useZaps.test.ts index 1f94b46..45ba95a 100644 --- a/src/hooks/useZaps.test.ts +++ b/src/hooks/useZaps.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from 'vitest'; import { finalizeEvent, generateSecretKey, getPublicKey, nip57 } from 'nostr-tools'; import type { NostrEvent } from '@nostrify/nostrify'; -import { summarizeZapReceipts, formatSats } from './useZaps'; +import { summarizeZapReceipts, hasValidReceiptForInvoice, formatSats } from './useZaps'; const targetId = 'a'.repeat(64); // A syntactically valid-enough bolt11 for getSatoshisAmountFromBolt11: "lnbc" @@ -87,3 +87,26 @@ describe('summarizeZapReceipts', () => { expect(summary.count).toBe(1); }); }); + +describe('hasValidReceiptForInvoice', () => { + const MY_INVOICE = `lnbc210n1${'q'.repeat(45)}`; + + it('returns false for no receipts', () => { + expect(hasValidReceiptForInvoice(undefined, MY_INVOICE)).toBe(false); + }); + + it('matches a valid receipt paying the exact invoice', () => { + const mine = receipt({ id: '1', tags: [['e', targetId], ['bolt11', MY_INVOICE], ['description', signedZapRequest(21_000).json]] }); + expect(hasValidReceiptForInvoice([mine], MY_INVOICE)).toBe(true); + }); + + it('ignores a receipt for a different invoice — someone else zapping the same note must not confirm this payment', () => { + const someoneElses = receipt({ id: '1' }); // uses the default FAKE_BOLT11, not MY_INVOICE + expect(hasValidReceiptForInvoice([someoneElses], MY_INVOICE)).toBe(false); + }); + + it('ignores a receipt matching the invoice but with an invalid description', () => { + const forged = receipt({ id: '1', tags: [['e', targetId], ['bolt11', MY_INVOICE], ['description', 'not json']] }); + expect(hasValidReceiptForInvoice([forged], MY_INVOICE)).toBe(false); + }); +}); diff --git a/src/hooks/useZaps.ts b/src/hooks/useZaps.ts index 9c9e24a..45a7c94 100644 --- a/src/hooks/useZaps.ts +++ b/src/hooks/useZaps.ts @@ -12,13 +12,19 @@ function zapReceiptsQueryKey(eventId: string) { return ['nostr', 'zap-receipts', eventId] as const; } -/** NIP-57 zap receipts (kind 9735) referencing `eventId`. */ -export function useZapReceipts(eventId: string | undefined, opts?: { refetchInterval?: number | false }) { +/** + * NIP-57 zap receipts (kind 9735) referencing `eventId`. Pass `enabled: false` + * until the caller actually needs the total — a note list mounts one of these + * per row, and firing all of them unconditionally turns a page of notes into + * a page of relay queries (an N+1 pattern) before anyone's looked at any of + * them. + */ +export function useZapReceipts(eventId: string | undefined, opts?: { refetchInterval?: number | false; enabled?: boolean }) { const { nostr } = useNostr(); return useQuery({ queryKey: zapReceiptsQueryKey(eventId ?? ''), - enabled: Boolean(eventId), + enabled: Boolean(eventId) && (opts?.enabled ?? true), queryFn: async ({ signal }) => { const events = await nostr.query( [{ kinds: [ZAP_RECEIPT_KIND], '#e': [eventId!], limit: 500 }], @@ -39,9 +45,12 @@ export interface ZapSummary { /** * Sums zap receipts defensively: a receipt only counts if it carries a * `bolt11` invoice and a `description` whose embedded zap request is a - * well-formed, signature-valid Nostr event. Anything else — a malformed or - * forged receipt a relay happens to serve back — is dropped rather than - * inflating the total. + * well-formed, signature-valid Nostr event. That rules out garbage a relay + * happens to serve back — malformed data, or a request signature that + * doesn't check out — but it is not proof any payment actually happened. + * NIP-57 receipts are published by the recipient's own LNURL server, so + * trusting that a receipt means "paid" is inherent to the protocol; this + * validation only keeps structurally-invalid noise out of the total. */ export function summarizeZapReceipts(events: NostrEvent[] | undefined): ZapSummary { const seen = new Set(); @@ -67,6 +76,21 @@ export function summarizeZapReceipts(events: NostrEvent[] | undefined): ZapSumma return { totalSats, count }; } +/** + * True if any signature-valid receipt among `events` pays exactly `invoice`. + * Used to confirm a specific manual payment — checking whether the note's + * receipt *count* went up is not enough, since anyone else zapping the same + * note while the payer is waiting would also bump the count and falsely + * confirm a still-unpaid invoice. + */ +export function hasValidReceiptForInvoice(events: NostrEvent[] | undefined, invoice: string): boolean { + return (events ?? []).some((receipt) => { + const bolt11 = tagValue(receipt, 'bolt11'); + const description = tagValue(receipt, 'description'); + return bolt11 === invoice && Boolean(description) && nip57.validateZapRequest(description!) === null; + }); +} + /** "1.2K" for 1234 — compact, locale-aware, and never wraps a note's action row. */ export function formatSats(sats: number): string { return new Intl.NumberFormat(undefined, { notation: 'compact', maximumFractionDigits: 1 }).format(sats); diff --git a/src/lib/nwc.test.ts b/src/lib/nwc.test.ts index 6387439..108dd3f 100644 --- a/src/lib/nwc.test.ts +++ b/src/lib/nwc.test.ts @@ -39,6 +39,11 @@ describe('parseNwcUri', () => { expect(() => parseNwcUri(uri)).toThrow(); }); + it('rejects a plaintext ws:// relay', () => { + const uri = `nostr+walletconnect://${PUBKEY}?relay=ws%3A%2F%2Frelay.example.com&secret=${SECRET}`; + expect(() => parseNwcUri(uri)).toThrow(); + }); + it('lowercases hex fields', () => { const uri = `nostr+walletconnect://${PUBKEY.toUpperCase()}?relay=wss%3A%2F%2Frelay.example.com&secret=${SECRET.toUpperCase()}`; const connection = parseNwcUri(uri); diff --git a/src/lib/nwc.ts b/src/lib/nwc.ts index 03bed13..0c88d3c 100644 --- a/src/lib/nwc.ts +++ b/src/lib/nwc.ts @@ -37,8 +37,12 @@ export function parseNwcUri(uri: string): NwcConnection { if (!HEX_64.test(parsed.pubkey) || !HEX_64.test(parsed.secret)) { throw new Error('That Wallet Connect link is missing a valid key.'); } - if (!parsed.relay.startsWith('wss://') && !parsed.relay.startsWith('ws://')) { - throw new Error('That Wallet Connect link has an invalid relay.'); + // wss:// only: every request to the wallet is signed by `secret` (a private + // key) and, even though the payload is encrypted, an unencrypted `ws://` + // transport still leaks metadata about the connection (who you're paying, + // when, how often) to anyone on the network path, and admits tampering. + if (!parsed.relay.startsWith('wss://')) { + throw new Error('That Wallet Connect link must use a wss:// relay.'); } return { pubkey: parsed.pubkey.toLowerCase(), relay: parsed.relay, secret: parsed.secret.toLowerCase() };