From 36afa7ea2c499fb9103a5aadf1169e9873e4cf29 Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Wed, 20 May 2026 00:33:08 -0700 Subject: [PATCH] audit: remove duplicate Stripe integration card + tighten input validation Deep audit of the recent change set (passes 56-60): - Remove the generic Integrations-tab Stripe card. saveIntegration overwrites integrations.config wholesale, so configuring Stripe there clobbered the dues config (mode/prices/grace_days) that the Dues tab manages, and couldn't actually set up dues. Stripe is now configured in one place (the Dues tab). PayPal/Zeffy/Venmo sync cards unchanged. - deleteDuesPaymentMethod validates platform against DUES_LINK_PLATFORMS instead of passing a raw client string into the enum column. - markNotificationsRead uses a strict uuid regex for the id filter. PGRST201 class characterized + confirmed contained: PostgREST treats a 2-FK table as an ambiguous junction only when its PK covers both FKs; the only such table now is space_member_custom_roles (a legitimate M2M). Guarded by integration/auth-embed.test.ts. --- app/(app)/settings/settings-client.tsx | 21 +++++---------------- docs/HANDOFF.md | 20 ++++++++++++++++++++ lib/actions/dues-payments.ts | 6 +++++- lib/actions/notifications.ts | 4 +++- 4 files changed, 33 insertions(+), 18 deletions(-) diff --git a/app/(app)/settings/settings-client.tsx b/app/(app)/settings/settings-client.tsx index e65176a..222b7c6 100644 --- a/app/(app)/settings/settings-client.tsx +++ b/app/(app)/settings/settings-client.tsx @@ -71,22 +71,11 @@ const INTEGRATIONS_CONFIG = [ ], docs: 'https://developer.venmo.com/', }, - { - platform: 'stripe', - name: 'Stripe', - description: 'Accept card payments and subscriptions', - icon: ( - - - - ), - fields: [ - { key: 'publishable_key', label: 'Publishable Key', type: 'text', placeholder: 'pk_test_...' }, - { key: 'secret_key', label: 'Secret Key', type: 'password', placeholder: 'sk_test_...' }, - { key: 'webhook_secret', label: 'Webhook Secret', type: 'password', placeholder: 'whsec_...' }, - ], - docs: 'https://stripe.com/docs/api', - }, + // Stripe is intentionally NOT listed here. Stripe recurring dues is + // configured in the dedicated Dues tab (saveStripeSettings: mode, tier->price + // map, grace days, vaulted keys). The generic saveIntegration path OVERWRITES + // integrations.config with a different shape, which would clobber the dues + // config (drop prices/mode/grace_days). One Stripe config UI, in Dues. ] export default function SettingsClient({ space, isAdmin, integrations, currentRole }: Props) { diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index f61d4e0..d92642f 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -4,6 +4,26 @@ Append-only. Newest entries on top. Keep each entry to one screen. --- +## 2026-05-20 (pass 61): Deep audit + fixes (LOCAL, NOT deployed) + +Branch `main`. Deep audit of the app + the recent change set (passes 56-60), an independent subagent review of the new code, and fixes for what was real. Suite 594 unit / 40 integration / build green. + +### Systemic PGRST201 finding (root cause fully characterized, class contained) +The outage class is now precisely understood. PostgREST treats a 2-foreign-key table as an ambiguous many-to-many **junction only when its PRIMARY KEY covers both FK columns** (the classic join-table shape). That is why `notification_preferences` (PK `space_id, member_id, category`) broke the `space_members <-> spaces` embed, while the **23 other tables that FK to both** `space_members` and `spaces` (notifications, payments, member_billing, classes, equipment, ...) do **not** — they have surrogate `id` PKs. Verified by querying `pg_constraint`/`pg_index`: the only remaining composite-PK 2-FK junction is `space_member_custom_roles` (space_members <-> space_custom_roles), which is a **legitimate** M2M (no competing direct relationship, so no ambiguity). The auth embed is guarded by `integration/auth-embed.test.ts`. Conclusion: the class is contained; the rule to remember is in DATABASE_SCHEMA 052 and the ARCHITECTURE gotcha note. + +### Fixed +- **Stripe config footgun (real bug).** The Integrations tab had a generic "Stripe" card writing via `saveIntegration`, which **overwrites** `integrations.config` wholesale -- it would clobber the dues config's `mode`/`prices`/`grace_days` (managed by the Dues-tab `saveStripeSettings`) and could not actually configure dues. Removed the Stripe entry from `INTEGRATIONS_CONFIG`; Stripe is now configured in exactly one place (the Dues tab). Integrations keeps PayPal/Zeffy/Venmo (separate sync surface). +- **Boundary validation** (subagent P1/P2): `deleteDuesPaymentMethod` now validates `platform` against `DUES_LINK_PLATFORMS` (was passing a raw client string into the enum column); `markNotificationsRead` uses a strict uuid regex for the optional id filter. + +### Reviewed, no change needed +- Independent subagent review of notifications/dues/inbox/me/settings: **no P0**. Self-views correctly scoped to the caller's own `member_id`; no cross-member/cross-space path; inbox renders `body_text` (never `body_html`) so no XSS; dues links double-validated https before render; cron auth constant-time; cron prefs fail open (correct for a billing-critical mailer). +- Webhook signing secret is shown pre-populated to admins on `/settings` (subagent P2): left as-is -- webhook secrets are meant to be retrievable, the page is admin-only. + +### Open / next +- NOT deployed: the Stripe-card removal + the two validation fixes are prod code, committed locally. Awaiting user go. (No migration.) + +--- + ## 2026-05-20 (pass 60): /settings "Dues" tab (LOCAL, NOT deployed) Branch `main`. The Stripe-dues config + external-payment-links panels were floating below the settings tab bar (rendered by page.tsx outside SettingsClient, shown regardless of the active tab). Moved both into a new **Dues** tab inside SettingsClient (tabs are now Space / Integrations / Dues / Webhooks); the panels render as standard `bg-card rounded border` tab cards instead of `border-t` page sections. page.tsx now renders only SettingsClient. UI-only, no schema/actions change. Build + 594 unit green. NOT deployed. diff --git a/lib/actions/dues-payments.ts b/lib/actions/dues-payments.ts index 7da6513..d41e8cf 100644 --- a/lib/actions/dues-payments.ts +++ b/lib/actions/dues-payments.ts @@ -5,7 +5,7 @@ import { createClient } from '@/lib/supabase/server' import { requireMember, requireMemberWithRole, parseInput } from '@/lib/auth-helpers' import { ADMIN_ROLES } from '@/lib/permissions' import { duesPaymentMethodSchema } from '@/lib/validations' -import { isSafeDuesUrl } from '@/lib/dues-payments-logic' +import { isSafeDuesUrl, isDuesLinkPlatform } from '@/lib/dues-payments-logic' export type DuesPaymentMethod = { platform: string @@ -100,6 +100,10 @@ export async function deleteDuesPaymentMethod(input: { if (!auth.ok) return { error: auth.error } const { member } = auth + // Validate at the boundary (same as the upsert path) instead of passing a + // raw client string into the enum column. + if (!isDuesLinkPlatform(input.platform)) return { error: 'Invalid platform' } + const { error } = await supabase .from('dues_payment_methods') .delete() diff --git a/lib/actions/notifications.ts b/lib/actions/notifications.ts index 6b6e1ba..e144823 100644 --- a/lib/actions/notifications.ts +++ b/lib/actions/notifications.ts @@ -77,7 +77,9 @@ export async function markNotificationsRead( if (input.ids && input.ids.length > 0) { // Defend the uuid column against a malformed client id (a non-uuid would // error the cast). Only well-formed ids pass; an empty result is a no-op. - const ids = input.ids.filter(id => /^[0-9a-f-]{36}$/i.test(id)) + const ids = input.ids.filter(id => + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(id), + ) if (ids.length === 0) return { ok: true } q = q.in('id', ids) }