From f1bae44afdfad537a40be22216456fb5b46bf461 Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Sun, 17 May 2026 05:06:02 -0700 Subject: [PATCH] fix(notifications): P2 extreme-audit P1 fixes before deploy Independent audit verdict was NOT SAFE; all findings fixed: - dispatcher Idempotency-Key is now per-attempt (${id}:${attempts}), not row id alone, so a transient-failure response cached by Resend cannot permanently suppress every retry and silently lose an email. - enqueueDues is fully try/catch wrapped so a notifications-table or render failure can never throw into / wedge the Stripe money path. - dues_lapsed dedupe falls back to the subscription id when periodEnd is null (canceled/deleted subs), so future lapses no longer collapse to one key and get suppressed. New test; suite 499. - documented the dispatcher's once/min concurrency assumption. --- __tests__/notifications-logic.test.ts | 25 +++++++ app/api/cron/notifications/route.ts | 20 ++++-- app/api/stripe/webhook/[space]/route.ts | 89 +++++++++++++++---------- docs/HANDOFF.md | 17 +++++ lib/notifications-logic.ts | 18 ++++- 5 files changed, 125 insertions(+), 44 deletions(-) diff --git a/__tests__/notifications-logic.test.ts b/__tests__/notifications-logic.test.ts index 5b4ee0d..913a435 100644 --- a/__tests__/notifications-logic.test.ts +++ b/__tests__/notifications-logic.test.ts @@ -47,6 +47,31 @@ describe('duesDedupeKey', () => { expect(a).toBe('dues_lapsed:m1:2026-06-01') expect(a).not.toBe(b) }) + it('lapse falls back to subscription id when periodEnd is null', () => { + // canceled/deleted subs carry no item period; without the fallback every + // lapse would collapse to dues_lapsed:m1 and suppress future notices. + const k = duesDedupeKey('dues_lapsed', { + memberId: 'm1', + periodEnd: null, + subscriptionId: 'sub_A', + }) + expect(k).toBe('dues_lapsed:m1:sub_A') + // A comeback subscription has a new id, so a later lapse stays distinct. + const k2 = duesDedupeKey('dues_lapsed', { + memberId: 'm1', + periodEnd: null, + subscriptionId: 'sub_B', + }) + expect(k2).not.toBe(k) + // periodEnd still wins when present (stable per billing period). + expect( + duesDedupeKey('dues_lapsed', { + memberId: 'm1', + periodEnd: '2026-06-01', + subscriptionId: 'sub_A', + }), + ).toBe('dues_lapsed:m1:2026-06-01') + }) }) describe('formatMoney', () => { diff --git a/app/api/cron/notifications/route.ts b/app/api/cron/notifications/route.ts index f9519bd..da26087 100644 --- a/app/api/cron/notifications/route.ts +++ b/app/api/cron/notifications/route.ts @@ -4,10 +4,14 @@ // (cron has none) so proxy.ts whitelists /api/cron; trust is the CRON_SECRET // shared-secret header, compared in constant time. // -// Re-entrancy: a row is updated immediately after its own send, and sendEmail -// passes the row id as Resend's Idempotency-Key, so an overlapping run cannot -// double-send. Transient failures (retryable) stay pending for the next -// minute until the attempt budget is exhausted; permanent ones go to 'failed'. +// Re-entrancy: there is no row-level claim/lock, so two overlapping runs can +// scan the same pending rows. This is safe because (a) the crontab fires once +// a minute and a run drains <=20 rows in ~5s, so overlap is rare, and (b) +// sendEmail passes a per-attempt Idempotency-Key (`${id}:${attempts}`): +// concurrent runs of the SAME attempt dedupe at Resend, while the next +// minute's retry of a transiently-failed row is a deliberately fresh send. +// Transient failures stay pending until the attempt budget is exhausted; +// permanent ones go to 'failed'. import { NextRequest, NextResponse } from 'next/server' import { timingSafeEqual } from 'node:crypto' import { createAdminClient } from '@/lib/supabase/admin' @@ -63,7 +67,13 @@ export async function POST(req: NextRequest) { subject: row.subject as string, html: row.body_html as string, text: row.body_text as string, - idempotencyKey: row.id as string, + // Per-attempt, NOT row.id alone: Resend dedupes 24h on key+payload and + // its docs do not guarantee a failed response is excluded from that + // cache. A stable key would let a cached transient failure permanently + // suppress every later retry (email silently lost). Tying the key to + // the attempt number means concurrent runs of the SAME attempt still + // dedupe, but the next minute's real retry is a fresh send. + idempotencyKey: `${row.id}:${(row.attempts as number) ?? 0}`, }) if (res.ok) { diff --git a/app/api/stripe/webhook/[space]/route.ts b/app/api/stripe/webhook/[space]/route.ts index 0d8b03d..c4d0f39 100644 --- a/app/api/stripe/webhook/[space]/route.ts +++ b/app/api/stripe/webhook/[space]/route.ts @@ -88,6 +88,11 @@ export async function POST( // inline (keeps this money path fast + retry-safe); the dispatcher cron // sends. ignoreDuplicates + the (space_id, dedupe_key) unique index make a // Stripe event replay (new event id, same invoice/period) a no-op. + // + // BEST-EFFORT: the entire body is wrapped so a notifications-table or + // render failure can NEVER throw into the money path. The Stripe ledger / + // member_billing / status writes must finalize even if email infra is + // down; a missed enqueue is acceptable, a wedged money path is not. async function enqueueDues( type: DuesNotificationType, args: { @@ -97,45 +102,55 @@ export async function POST( currency?: string | null periodEnd?: string | null invoiceId?: string | null + subscriptionId?: string | null }, ): Promise { - if (!args.memberId) return - const { data: mem } = await admin - .from('space_members') - .select('email, display_name') - .eq('id', args.memberId) - .eq('space_id', spaceId) - .maybeSingle() - const recipient = ((mem?.email as string | null) || args.fallbackEmail || '').trim() - if (!recipient) return - const { subject, html, text } = renderDuesEmail({ - type, - spaceName: await getSpaceName(), - memberName: (mem?.display_name as string | null) ?? null, - amount: args.amount ?? null, - currency: args.currency ?? null, - periodEnd: args.periodEnd ?? null, - manageUrl, - }) - await admin.from('notifications').upsert( - { - space_id: spaceId, - member_id: args.memberId, + try { + if (!args.memberId) return + const { data: mem } = await admin + .from('space_members') + .select('email, display_name') + .eq('id', args.memberId) + .eq('space_id', spaceId) + .maybeSingle() + const recipient = ((mem?.email as string | null) || args.fallbackEmail || '').trim() + if (!recipient) return + const { subject, html, text } = renderDuesEmail({ type, - channel: 'email', - recipient, - subject, - body_html: html, - body_text: text, - status: 'pending', - dedupe_key: duesDedupeKey(type, { - invoiceId: args.invoiceId, - memberId: args.memberId, - periodEnd: args.periodEnd, - }), - }, - { onConflict: 'space_id,dedupe_key', ignoreDuplicates: true }, - ) + spaceName: await getSpaceName(), + memberName: (mem?.display_name as string | null) ?? null, + amount: args.amount ?? null, + currency: args.currency ?? null, + periodEnd: args.periodEnd ?? null, + manageUrl, + }) + const { error: enqErr } = await admin.from('notifications').upsert( + { + space_id: spaceId, + member_id: args.memberId, + type, + channel: 'email', + recipient, + subject, + body_html: html, + body_text: text, + status: 'pending', + dedupe_key: duesDedupeKey(type, { + invoiceId: args.invoiceId, + memberId: args.memberId, + periodEnd: args.periodEnd, + subscriptionId: args.subscriptionId, + }), + }, + { onConflict: 'space_id,dedupe_key', ignoreDuplicates: true }, + ) + if (enqErr) console.error(`[stripe webhook] enqueue ${type} failed:`, enqErr.message) + } catch (e) { + console.error( + `[stripe webhook] enqueue ${type} threw:`, + e instanceof Error ? e.message : e, + ) + } } // Resolve our member: prefer metadata (set on the subscription + session), @@ -211,7 +226,7 @@ export async function POST( // Dues lapsed past grace -> one "marked late" email per lapsed period // (dedupe is member + periodEnd, so next cycle can lapse again). if (desired === 'late') { - await enqueueDues('dues_lapsed', { memberId, periodEnd }) + await enqueueDues('dues_lapsed', { memberId, periodEnd, subscriptionId: sub.id }) } } diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index a102663..3b035d3 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -4,6 +4,23 @@ Append-only. Newest entries on top. Keep each entry to one screen. --- +## 2026-05-17 (pass 46) — Phase 2 EXTREME AUDIT: NOT-SAFE verdict, P1s fixed (LOCAL, still awaiting deploy) + +Branch `main`. Owner asked for an extreme audit before deploy. Ran an independent code audit + self-audit + web re-verify of Resend idempotency semantics. Verdict was **NOT SAFE as-is**; all findings fixed. Suite 499, build clean. + +### Findings -> fixes (commit pending in this pass) +- **P1 (P0 blast radius) dispatcher idempotency**: row id alone as Resend Idempotency-Key; Resend's docs do not guarantee a failed response is excluded from the 24h key cache, so a cached transient failure could permanently suppress every retry (email silently lost). Fixed: key is now per-attempt `${row.id}:${attempts}` (concurrent same-attempt runs still dedupe; next-minute retry is a fresh send). `app/api/cron/notifications/route.ts`. +- **P1 enqueue not isolated from money path**: a persistent notifications-table/render failure could 500 the Stripe webhook on every retry and stall ledger finalization. Fixed: `enqueueDues` is fully wrapped (try/catch + non-throwing error log) so email infra can never wedge the money path. `app/api/stripe/webhook/[space]/route.ts`. +- **P1 `dues_lapsed` null-periodEnd collapse**: canceled/deleted subs carry no item period; key collapsed to `dues_lapsed:` and suppressed every future lapse notice. Fixed: fall back to subscription id (`duesDedupeKey`), threaded `subscriptionId` through `enqueueDues`. New test (499 total). +- **P2 dispatcher concurrency**: no row-level lock; documented the once/min + ~5s-drain assumption and why per-attempt key makes it safe. +- Audit PASS (no change needed): Phase-1 webhook guarantees intact, multi-tenant isolation, RLS + member self-view, HTML escaping, CRON_SECRET constant-time, migration idempotent + mirror exact, fresh-clone fails safe. + +### State +- **Still not deployed.** All P1s fixed and gated; awaiting the deploy decision. Local commits ahead of origin: pass-44 docs (dc76a86), the 5 Phase 2 commits, and this audit-fix + pass-46. +- Post-deploy provisioning (owner, Droplet): set `RESEND_API_KEY`, `EMAIL_FROM` (verified domain + SPF/DKIM), `CRON_SECRET`; add the once-a-minute crontab line (docs/DEPLOYMENT.md). Inert until then. + +--- + ## 2026-05-17 (pass 45) — Product spine Phase 2: transactional notifications (LOCAL, awaiting one reviewed deploy) Branch `main`. Built Phase 2 dues-lifecycle email end to end, gated + committed per sub-phase. NOT deployed (ASK pending). Suite 498, build clean. diff --git a/lib/notifications-logic.ts b/lib/notifications-logic.ts index 01f8bac..4cc972d 100644 --- a/lib/notifications-logic.ts +++ b/lib/notifications-logic.ts @@ -37,12 +37,26 @@ export function notificationDedupeKey( // Renewal + payment-failed are keyed by the Stripe invoice (one mail per // invoice outcome). Lapse is keyed by member + the period it lapsed for, so a // member can lapse again next cycle but not be re-mailed for the same one. +// periodEnd can be absent (canceled/deleted subscriptions carry no item +// current_period_end); without a fallback every lapse would collapse to +// `dues_lapsed:` and suppress every future lapse notice. Fall back to +// the subscription id (a new comeback subscription has a new id, so distinct +// lapses stay distinct). export function duesDedupeKey( type: DuesNotificationType, - ref: { invoiceId?: string | null; memberId?: string | null; periodEnd?: string | null }, + ref: { + invoiceId?: string | null + memberId?: string | null + periodEnd?: string | null + subscriptionId?: string | null + }, ): string { if (type === 'dues_lapsed') { - return notificationDedupeKey(['dues_lapsed', ref.memberId, ref.periodEnd]) + return notificationDedupeKey([ + 'dues_lapsed', + ref.memberId, + ref.periodEnd || ref.subscriptionId, + ]) } return notificationDedupeKey([type, ref.invoiceId]) }