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).
This commit is contained in:
Moheeb Zara
@@ -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 |
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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`.
|
||||
|
||||
|
||||
@@ -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 `<a href>`. 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`).
|
||||
|
||||
@@ -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' })
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 <a href>. 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 $$;
|
||||
+5
-1
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user