Security/correctness audit 2026-08-02: verified findings from fresh-eyes review of payments + Cat-forget surfaces (ranked)

[catomean]

Two independent fresh-eyes reviews (payments/wallets: #534 #536 #544 #556; Cat forget: #542 #545 + open #555) produced these verified findings. The criticals are already fixed (#561: L402 receipt binding / self-attestation / preimage settlement + entity-wallets embed; #562: tool-path profile clearing; #555 carries its own blockers as a PR comment). This issue tracks the rest, ranked.

Important — payments

1. Recipient-side invoice-spam limiter missing on GET /api/v1/pay/... and POST /api/v1/payments/public — both mint a real invoice via the recipient's LNURL/NWC relay per request. rateLimitTipRecipient (src/lib/rate-limit.ts:214) was built for exactly this and is only applied to tips/lnurlp. An attacker can get a seller rate-limited/banned by their own wallet provider and litter orphan intents.
2. Rate-limit key is the leftmost X-Forwarded-For hop (client-controlled; Caddy appends). Every keyed limiter using x-forwarded-for.split(',')[0] is bypassable with a random header. Fix once in a shared clientIpKey() (also closes the 3-way duplication of IP-derivation: v1/pay route, payments/public route, rate-limit.ts).
3. Acknowledge as notification cannon: POST /api/v1/payments/public/{id} acknowledge + feat(wallets): lock down wallet reads + notify recipients of payment claims #544's recipient notification → free intents (finding 1) fan plausible "someone paid you" cards into a seller's confirmation queue = ship-goods-for-no-money social-engineering primitive. Needs per-recipient limiting + queue hygiene.
4. Settlement side-effects are at-most-once with no compensating job: claimPaidTransition flips to paid FIRST; a crash before order/inventory/notify/webhook loses them forever (refresh short-circuits on paid; cron won't re-pick). Add a side_effects_at marker + sweep for paid AND side_effects_at IS NULL.
5. WALLET_CLIENT_COLUMNS defined twice with no drift guard (src/config/database-tables.ts vs migration 20260802120000 GRANT list; Postgres does NOT auto-grant later ADD COLUMNs). Next wallets column = silent 42501s (the exact failure fix(payments): bind L402 receipts to their entity, settle on preimage, never trust self-attestation #561 just fixed once). Needs a drift test like refactor(pricing): one SSOT, machine-guarded — tier map, upsell copy, SQL drift test #550's pricing SQL test, plus a lint/grep gate forbidding wallets(*)/select('*') on WALLETS.
6. Migration 20260802120000's stated goal not achieved: GET /api/wallets?profile_id=<any> (withOptionalAuth) serves address_or_xpub to anonymous callers via the admin client — an xpub reveals the owner's entire address history. Drop it from PUBLIC_WALLET_FIELDS (keep lightning_address) or amend the migration's claim.
7. L402 verify branch deliberately unlimited yet drives an outbound NWC/LNURL/mempool call per request on non-terminal intents — one valid token buys unbounded outbound traffic. Cheap per-token limiter.

Important — Cat forget (beyond #555's blockers)

8. Store failures indistinguishable from "no match": both stores swallow DB errors into {deleted:[], notFound: wanted} → half-done deletions reported as success, and outright query failures reported as "nothing matched". Add an explicit failed channel to ForgetResult/ProfileRemovalResult; handler returns success:false with an honest "couldn't reach your memories".
9. Raw substring containment: a 4-char fact like "work" deletes anything containing network/framework (MIN_FORGET_FRAGMENT_CHARS=4, no word boundary). Word-boundary the containment branch. (Coordinate with fix(cat): forget_memories actually deletes + skip guaranteed-413 Groq round-trips #555 — same file.)
10. Silent cap: facts 11+ are sliced off and never reported in notFound (profile path has NO cap — another drift). Report over-cap facts.
11. stillUnknown compares raw vs trimmed facts — facts with padding or <4 chars silently vanish from the "no match" list.
12. forget_memories is the only irreversible destructive verb with riskLevel low + no confirmation while ~18 lesser actions require confirmation; tool path has no deterministic deletion listing (depends on LLM compliance). Consider confirmation or a guaranteed receipt event.
13. pruneIfNeeded delete not user-scoped (memory.ts ~:406) — safe today under RLS-scoped client, becomes cross-user the day it runs under service role. Add .eq('user_id', userId) for pattern consistency.

Suggestions

14. fix(payments): settle through the admin client — RLS silently voided recipient confirmation #534 left refreshPaymentStatus/reconcilePaymentIntent/resolveSellerWallet taking a supabase client they no longer use — signatures advertise caller-scoped behavior that no longer exists.
15. apiPaymentRequired embeds the bearer token with no Cache-Control: no-store (only payment response without it).
16. fix(payments): publication gate on authenticated payment initiation #536's visibility gate needs a per-entity-type test matrix (a USING (true) policy makes it a no-op; a missing anon policy 400s every payment) and is duplicated across the anon and key/OIDC paths with only one load-bearing per path.
17. Test gaps flagged: no handler-level test for forget_memories exec_action; embeddings floor (0.45/0.75) never executed by any test; INCIDENT_CORPUS lacks innocent memories sharing generic stems (adding "Has strong cooking skills" breaks the over-match test — intentionally).

What the reviews confirmed is SOLID (for the record)

• fix(payments): settle through the admin client — RLS silently voided recipient confirmation #534 settlement authorization: every admin-client call authorizes first; claimPaidTransition conditional-update is genuinely atomic — no double-spend path found.
• No wallet/invoice enumeration via the L402 token lookup (hash + uniform error).
• Forget cross-user scoping sound (RLS + explicit user_id on all user-facing deletes).
• nwc_connection_uri never reaches Cat context (mapped to has_nwc boolean under service role).

[catomean]

Finding 6 (public wallet listing serves address_or_xpub to anonymous callers) — verified still live and fixed in #743.

Confirmed on production before fixing: one active wallet had a real 111-char zpub readable via GET /api/wallets?profile_id=<uuid> with no auth, and the public profile rendered it labelled "Extended Public Key" with a copy button.

Fix redacts extended keys for non-owners, classifying by value rather than the user-set wallet_type label; plain addresses still pass through. /api/wallets is the only non-owner read path for that column, so this closes the class.

Findings 1–5 and 7–17 remain open.