From f2534323dac48685351952911c0be5b7ba2ca27d Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Tue, 19 May 2026 21:35:49 -0700 Subject: [PATCH] security: enforce https on dues_payment_methods.url at the data layer The url renders to members as a clickable href. https was enforced only in the admin Zod action, but RLS lets an admin/board write the row directly via PostgREST, bypassing Zod, with no DB constraint -- a malicious admin could store a javascript: URL and XSS members who click. - Migration 050: CHECK (url ~* '^https://') so the DB is the source of truth; integration-tested that the service path cannot store javascript:/http:. - Member-read action filters with isSafeDuesUrl (defense-in-depth). --- DB_SCHEMA_MAP.md | 1 + docs/DATABASE_SCHEMA.md | 1 + docs/HANDOFF.md | 3 +++ integration/dues-payment-methods.test.ts | 18 +++++++++++++++ lib/actions/dues-payments.ts | 7 +++++- scripts/050_dues_payment_methods_https.sql | 27 ++++++++++++++++++++++ scripts/schema.sql | 6 ++++- 7 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 scripts/050_dues_payment_methods_https.sql diff --git a/DB_SCHEMA_MAP.md b/DB_SCHEMA_MAP.md index e94c403..7285074 100644 --- a/DB_SCHEMA_MAP.md +++ b/DB_SCHEMA_MAP.md @@ -415,6 +415,7 @@ Default channels created by trigger on space INSERT: `general`, `announcements`, | 047 | `members_with_permission(sid, perm)` SECURITY DEFINER, set-returning inverse of `user_has_permission` (same current/late gate, same admin shortcut + `space_role_permissions`/`user_effective_roles` fallback). Lets Phase 4 fan-outs (e.g. `form_submission_admin`) resolve permission holders in one query. No new table/column/policy | | 048 | `notification_preferences` (Phase 5): per-member opt-out of muteable categories (`bookings, classes, forms, admin_alerts`); `billing` never muteable/stored. Cols `space_id, member_id, category, enabled`; PK `(space_id, member_id, category)`. Opt-out: no row = enabled. RLS on, NO client policy (default-deny); dispatcher reads via service client and marks muted rows `skipped`; `/me` view + toggle via validated service-client actions. `touch_updated_at` trigger | | 049 | `dues_payment_methods`: admin-configured external dues links. Cols `id, space_id, platform (payment_platform), url, instructions, is_active, sort_order`; `UNIQUE (space_id, platform)`. One pay-here URL per platform (PayPal/Zeffy/Venmo); members click out, treasurer reconciles manually (platform pre-typed). RLS: SELECT any space member, write admin/board. `url` validated https-only at the action boundary. `touch_updated_at` trigger | +| 050 | CHECK `dues_payment_methods_url_https` (`url ~* '^https://'`): the url renders as a member-clickable href and RLS lets admins write directly past the app Zod check, so the DB enforces https (no stored `javascript:`/`http:` XSS). Member-read action also re-filters with `isSafeDuesUrl`. Idempotent DO-block | --- diff --git a/docs/DATABASE_SCHEMA.md b/docs/DATABASE_SCHEMA.md index 3e2dbd8..8e1c21d 100644 --- a/docs/DATABASE_SCHEMA.md +++ b/docs/DATABASE_SCHEMA.md @@ -730,6 +730,7 @@ migration is mirrored as a section in it. Tables/columns added: | 047 | `members_with_permission(sid, perm)` SECURITY DEFINER, `search_path=public`. Inverted, set-returning form of `user_has_permission`: returns `member_id` for every member of `sid` who holds `perm` (same current/late status gate as 046, same admin shortcut, same `space_role_permissions` + `user_effective_roles` fallback). Lets the Phase 4 notification fan-outs (e.g. `form_submission_admin`) find permission holders in one query instead of N round-trips. EXECUTE is locked: `REVOKE EXECUTE FROM PUBLIC`, `GRANT EXECUTE TO service_role` so the set-returning shape cannot be used by a PostgREST client to enumerate "who has permission X in space Y" across spaces (the rationale for not locking `user_has_permission` is that RLS policies depend on anon/authenticated being able to call it). Additive; existing `user_has_permission` callers unchanged. No new table/column/policy | | 048 | `notification_preferences` (Product spine Phase 5): per-member opt-out of muteable categories. Columns `space_id, member_id, category, enabled`, PK `(space_id, member_id, category)`. Opt-out model: absence of a row = enabled. Only the four muteable categories (`bookings, classes, forms, admin_alerts`) are ever stored; `billing` (dues renewed/failed/lapsed) is membership-critical and never muteable. RLS enabled, NO client policy (default-deny): the dispatcher reads via the service client and marks a muted outbox row `skipped`; the `/me` self-view + toggle write both go through validated service-client actions, same convention as `notifications` / `member_billing`. `touch_updated_at` trigger. No new permission code | | 049 | `dues_payment_methods`: admin-configured external dues payment links. Columns `id, space_id, platform (payment_platform), url, instructions, is_active, sort_order`, `UNIQUE (space_id, platform)`. One external pay-here URL per platform (PayPal/Zeffy/Venmo); members click out to pay dues off-platform and a treasurer reconciles manually via the existing payments flow (the platform tag pre-types that reconcile). Link configuration only, no automated payment record. RLS: SELECT = any space member (they render the buttons); INSERT/UPDATE/DELETE = admin/board (`user_has_role_in_space`). `url` is validated absolute-https at the server-action boundary. `touch_updated_at` trigger. No new permission code | +| 050 | `dues_payment_methods.url` gains a CHECK constraint (`dues_payment_methods_url_https`, `url ~* '^https://'`). The url is rendered to members as a clickable href; RLS lets an admin/board write the row directly via PostgREST, bypassing the app Zod check, so the DB enforces https as the source of truth (a `javascript:`/`http:` value can never be stored). The member-read action also re-validates with `isSafeDuesUrl` (defense-in-depth). Idempotent DO-block add-if-missing. No new table/column/policy | New enum: `comment_entity_type` = `forum_thread | proposal | incident | policy`. diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 97ccc7e..e4c5ec4 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -31,6 +31,9 @@ Branch `main`. Same session as pass-57; addressed three follow-up requests the u - Made `isSafeDuesUrl` load-bearing (the Zod url field now refines on it, removing the duplicate inline https check). Verified ChipInput limits match the profile Zod schema (skills/interests 40×60, willing_to 20×60, affiliations 50×200). Confirmed `getMyBilling`'s only real caller is `/me` (shape change from `null` to always-object is safe). External links render `target=_blank rel=noopener noreferrer`; url validated absolute-https so no javascript:/data: scheme. ### Post-deploy audit follow-up (LOCAL, NOT yet deployed) + +**Third ultrathink-audit finding (migration 050): stored-XSS defense gap on the dues url.** `dues_payment_methods.url` is rendered to members as a clickable ``. The https requirement was enforced ONLY in the admin Zod action, but RLS lets an admin/board write the row directly via PostgREST (public anon key + their JWT), bypassing Zod, and the table had no url constraint. A malicious admin could store `javascript:...` and XSS any member who clicks. Fixed two ways: (a) migration 050 adds a CHECK `url ~* '^https://'` (the DB is now the source of truth; integration-tested that the service path can't store `javascript:`/`http:`), and (b) the member-read action filters with `isSafeDuesUrl` (defense-in-depth, immediate protection for current prod). Admin-gated and prod has no rows yet, so no live exploitation window, but it escalated "space admin" to "XSS any member" and is closed now. + Second ultrathink audit after the `ab8c22a` deploy found NO deploy-blocking bug. Re-read the dispatcher, dues actions, and dues card with fresh eyes: URL is Zod-validated absolute-https; the `delete` platform flows through a parameterized `.eq` filter (double-gated by the space pin + RLS); writes/reads are scoped to the authenticated member; billing is never muted (logic + test). Then hardened + polished (all local, post-`ab8c22a`): - **Integration coverage added (now 37, was 31).** `integration/dues-payment-methods.test.ts` (4: admin insert + member read + non-admin write denied + cross-space read denied + cross-space write denied + 046 unverified-admin gate) and `integration/notification-prefs-dispatch.test.ts` (2: muted category -> `skipped` while billing always sends; no-pref-row -> sends). The dispatcher skip was unexercised in prod (cron gated off), so this is its first real exercise. All 37 pass vs local Postgres. - **Harness bug fixed.** `integration/db.ts` `rowsAsUser` ran `begin; set; select; commit;` in one `psql -c`, which prints only the LAST statement's result, so the SELECT output was swallowed (returned `[]`). It was never used before; now the SELECT is the last statement (session `set`, no trailing `commit`). diff --git a/integration/dues-payment-methods.test.ts b/integration/dues-payment-methods.test.ts index 5763caa..a89de05 100644 --- a/integration/dues-payment-methods.test.ts +++ b/integration/dues-payment-methods.test.ts @@ -78,6 +78,24 @@ d('dues_payment_methods RLS', () => { expect(r.ok).toBe(false) }) + it('the url CHECK rejects non-https schemes even via the service path (050)', () => { + const sid = space() + // service path bypasses RLS + the app Zod check, so the DB CHECK is the + // last line of defense against a stored javascript:/http: href. + const jsUrl = execService( + `insert into dues_payment_methods (space_id, platform, url) values ('${sid}','paypal','javascript:alert(1)');`, + ) + expect(jsUrl.ok).toBe(false) + const httpUrl = execService( + `insert into dues_payment_methods (space_id, platform, url) values ('${sid}','venmo','http://insecure');`, + ) + expect(httpUrl.ok).toBe(false) + const httpsUrl = execService( + `insert into dues_payment_methods (space_id, platform, url) values ('${sid}','zeffy','https://ok.example');`, + ) + expect(httpsUrl.ok).toBe(true) + }) + it('an unverified member cannot write even with admin role (046 status gate)', () => { const sid = space() const pending = seedMember(sid, { role: 'admin', status: 'unverified' }) diff --git a/lib/actions/dues-payments.ts b/lib/actions/dues-payments.ts index 4c109ff..7da6513 100644 --- a/lib/actions/dues-payments.ts +++ b/lib/actions/dues-payments.ts @@ -5,6 +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' export type DuesPaymentMethod = { platform: string @@ -32,7 +33,11 @@ export async function listActiveDuesPaymentMethods(): Promise< .eq('is_active', true) .order('sort_order', { ascending: true }) - return { data: (data ?? []).map(toMethod) } + // Defense-in-depth: only render absolute-https links. The DB CHECK and the + // admin Zod schema already enforce this, but a member's browser must never + // be handed an unsafe href, so re-validate at the boundary that feeds the + // clickable card and drop anything that is not a safe https URL. + return { data: (data ?? []).map(toMethod).filter(m => isSafeDuesUrl(m.url)) } } // Admin-facing: every configured method (active + inactive) for the management diff --git a/scripts/050_dues_payment_methods_https.sql b/scripts/050_dues_payment_methods_https.sql new file mode 100644 index 0000000..b030bcd --- /dev/null +++ b/scripts/050_dues_payment_methods_https.sql @@ -0,0 +1,27 @@ +-- ============================================================================= +-- 050: dues_payment_methods.url must be an absolute https URL (CHECK constraint) +-- ============================================================================= +-- The url is rendered to MEMBERS as a clickable . The admin Zod schema +-- (duesPaymentMethodSchema / isSafeDuesUrl) enforces https, but RLS lets an +-- admin/board write the row directly via PostgREST, bypassing the app-layer +-- validation. Without a data-layer constraint a malicious admin could store a +-- `javascript:` URL and XSS members who click it. Enforce https at the DB so +-- the schema is the single source of truth for what can ever be stored. +-- +-- Case-insensitive prefix match (`~*`) so `HTTPS://` (which new URL() accepts) +-- is allowed too; the member-read action additionally re-validates with +-- isSafeDuesUrl. Existing prod rows: none yet, so the constraint adds cleanly. +-- +-- Idempotent: only adds the constraint if it is not already present. Apply +-- as-is (Supabase SQL editor / psql); re-runs are no-ops. +-- ============================================================================= + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'dues_payment_methods_url_https' + ) THEN + ALTER TABLE public.dues_payment_methods + ADD CONSTRAINT dues_payment_methods_url_https CHECK (url ~* '^https://'); + END IF; +END $$; diff --git a/scripts/schema.sql b/scripts/schema.sql index 6ce53fb..2ef8a2e 100644 --- a/scripts/schema.sql +++ b/scripts/schema.sql @@ -2811,6 +2811,9 @@ CREATE TRIGGER trg_notification_prefs_touch -- Link configuration only, no automated payment record. RLS: SELECT = any -- space member (they render the buttons); writes admin/board. url is validated -- https-only at the server-action boundary. +-- url must be an absolute https URL: it is rendered to members as a clickable +-- href, and RLS lets admins write the row directly (bypassing the app's Zod +-- check), so the data layer is the source of truth (migration 050). CREATE TABLE IF NOT EXISTS public.dues_payment_methods ( id uuid PRIMARY KEY DEFAULT uuid_generate_v4(), space_id uuid NOT NULL REFERENCES public.spaces(id) ON DELETE CASCADE, @@ -2821,7 +2824,8 @@ CREATE TABLE IF NOT EXISTS public.dues_payment_methods ( sort_order integer NOT NULL DEFAULT 0, created_at timestamptz NOT NULL DEFAULT now(), updated_at timestamptz NOT NULL DEFAULT now(), - UNIQUE (space_id, platform) + UNIQUE (space_id, platform), + CONSTRAINT dues_payment_methods_url_https CHECK (url ~* '^https://') ); CREATE INDEX IF NOT EXISTS idx_dues_payment_methods_space ON public.dues_payment_methods (space_id);