From f5d670477b17533bde4c4eee74f45d489a1a4d36 Mon Sep 17 00:00:00 2001 From: Moheeb Zara Date: Fri, 15 May 2026 17:35:11 -0700 Subject: [PATCH] fix(security): close member self-escalation + harden after full audit Full-application audit (5 parallel read-only audits). Two agent "blockers" were false alarms and verified as already-correct. Real findings fixed: - Self-escalation (BLOCKER): migration 024 + schema.sql section 16 extend prevent_member_self_role_change to also block a non-privileged member changing their own tier_id (self tier upgrade) and onboarding_completed_at (skipping required code-of-conduct via a raw PostgREST call). finishOnboarding and skipOnboarding now set onboarding_completed_at through the service client AFTER their server-side required-steps check, so the legitimate path is unaffected. - Cross-tenant write scoping: added .eq('space_id', member.space_id) to updateForumThread, deleteForumThread, editComment, deleteComment, deleteChannel, renameChannel (RLS-only -> defence-in-depth). - generateInviteCode: crypto.getRandomValues, widened to 8 chars (was Math.random, ~21 bits, enumerable under the join rate limit). - SafeMarkdown: dropped iframe and the global style attribute (no unsandboxed embeds / UI-redress); markdown + safe HTML stay. - /settings: server-side admin redirect before fetching space and integration config (was client-hidden only). - app/(app)/layout.tsx: export const dynamic = 'force-dynamic'. - Bounded queries: loadComments .limit(500), tasks .limit(2000). Backlog from the audit is tracked in HANDOFF (import Zod schemas, types/database.ts + docs regen, email lowercasing, unauthorized- handling consistency, KB-modal ACL hookup). 271/271 tests pass. Build clean. Migrations 023 + 024 idempotent, apply via deploy.sh on next deploy. --- CHANGELOG.md | 7 +++ app/(app)/layout.tsx | 4 ++ app/(app)/settings/page.tsx | 8 +++- app/(app)/tasks/page.tsx | 2 +- components/comments/load-comments.ts | 3 ++ components/safe-markdown.tsx | 17 +++----- docs/HANDOFF.md | 31 ++++++++++++++ lib/actions/comms.ts | 4 ++ lib/actions/forum.ts | 9 +++- lib/actions/onboarding.ts | 13 +++++- lib/auth-actions.ts | 8 +++- scripts/024_self_change_hardening.sql | 61 +++++++++++++++++++++++++++ scripts/schema.sql | 38 +++++++++++++++++ 13 files changed, 187 insertions(+), 18 deletions(-) create mode 100644 scripts/024_self_change_hardening.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 1640a65..6fd82b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,13 @@ All notable changes to this project are documented here. Format follows [Keep a - Customize -> Area leads: create area-lead roles, see Vacant until assigned, assign/unassign a member from a picker. The assigned member effectively holds that role, so any Ops item whose access list includes it opens to them. - SECURITY DEFINER helpers `user_effective_roles` and `user_has_permission`. +### Security +- Closed two member self-escalation paths: a non-privileged member can no longer change their own `tier_id` or `onboarding_completed_at` on the `space_members` row (migration 024 extends the self-change trigger). Onboarding completion now goes through the service client after server-side validation. +- Added in-code space scoping to forum/comment/channel mutations (defence-in-depth on top of RLS). +- Invite codes are now cryptographically random and wider. +- `SafeMarkdown` no longer allows `iframe` or arbitrary `style` (no unsandboxed embeds or UI-redress). +- `/settings` is gated server-side to admins before any space/integration data is fetched. + ### Fixed - `revealSecret` no longer silently returns the stale plaintext column when a row is marked encrypted but has no ciphertext; it surfaces the corruption instead. - `upsertAreaLead` rejects a null/empty area code (Postgres treats NULLs as distinct in unique constraints, which had allowed duplicate area-lead rows). diff --git a/app/(app)/layout.tsx b/app/(app)/layout.tsx index 98e32bc..62b1540 100644 --- a/app/(app)/layout.tsx +++ b/app/(app)/layout.tsx @@ -3,6 +3,10 @@ import { redirect } from 'next/navigation' import { AppSidebar } from '@/components/app-sidebar' import { getRoleLabelMap } from '@/lib/role-labels' +// This layout is the auth/onboarding gate for the whole app. It must never be +// statically cached: every request re-checks the session and member state. +export const dynamic = 'force-dynamic' + export default async function AppLayout({ children }: { children: React.ReactNode }) { const supabase = await createClient() const { data: { user } } = await supabase.auth.getUser() diff --git a/app/(app)/settings/page.tsx b/app/(app)/settings/page.tsx index d0da5ed..15add88 100644 --- a/app/(app)/settings/page.tsx +++ b/app/(app)/settings/page.tsx @@ -12,14 +12,18 @@ export default async function SettingsPage() { if (!member) redirect('/login') + // Settings is admin-only. Gate on the server BEFORE fetching space config + // and integration state so non-admins never receive it. Board/treasurer + // use /customize for the things they can change. + const isAdmin = member.role === 'admin' + if (!isAdmin) redirect('/dashboard') + const { data: space } = await supabase .from('spaces').select('*').eq('id', member.space_id).single() const { data: integrations } = await supabase .from('integrations').select('*').eq('space_id', member.space_id) - const isAdmin = member?.role === 'admin' - return ( r.author_id).filter(Boolean) as string[])) diff --git a/components/safe-markdown.tsx b/components/safe-markdown.tsx index a67c44f..70da3b6 100644 --- a/components/safe-markdown.tsx +++ b/components/safe-markdown.tsx @@ -7,24 +7,21 @@ import rehypeSanitize, { defaultSchema } from 'rehype-sanitize' // Renders admin-authored markdown that MAY contain a constrained subset of raw // HTML. rehype-raw parses the HTML; rehype-sanitize then strips anything -// dangerous (script, event handlers, javascript: URLs, etc). The author is a -// space admin/board member, but a stored-XSS payload would still run in other -// members' sessions within that tenant, so this is sanitized, not trusted. +// dangerous. The author is a space admin/board member, but a stored-XSS or +// UI-redress payload would still run in other members' sessions within that +// tenant, so this is sanitized hard, not trusted: +// - no `iframe` (no unsandboxed third-party embeds / token theft frame) +// - no global `style` attribute (no full-viewport overlay redress) +// - links forced to safe protocols; rel/target locked down const schema = { ...defaultSchema, attributes: { ...defaultSchema.attributes, - '*': [...(defaultSchema.attributes?.['*'] ?? []), 'className', 'style'], + '*': [...(defaultSchema.attributes?.['*'] ?? []), 'className'], a: [...(defaultSchema.attributes?.a ?? []), 'target', 'rel'], }, - tagNames: [ - ...(defaultSchema.tagNames ?? []), - 'iframe', - ], - // Allow only video embeds from well-known hosts; everything else dropped. protocols: { ...defaultSchema.protocols, - src: ['https'], href: ['https', 'http', 'mailto'], }, } diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 9ce78e2..d2938b7 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -4,6 +4,37 @@ Append-only. Newest entries on top. Keep each entry to one screen. --- +## 2026-05-15 (pass 16) — Full-app audit + security hardening + +Branch: `main`. Migration: 024 (plus 023 from pass 15 not yet deployed). + +### Audit + +Five parallel read-only audits (security/RLS, server actions/validation, routes/data, schema/types/migrations, plus a security-smell sweep). Verified before acting — two agent "blockers" were false alarms (ops_acl/space_role_permissions/member_custom_roles DELETE policies DO exist in schema.sql §15; m2m needs no UPDATE). + +### Fixed this pass (security) + +- **Self-escalation closed (BLOCKER).** Migration 024 + schema.sql §16: `prevent_member_self_role_change` now also blocks a non-privileged member changing their own `tier_id` (self tier upgrade) and `onboarding_completed_at` (skip required code-of-conduct). `finishOnboarding`/`skipOnboarding` now set `onboarding_completed_at` via the service client AFTER their server-side required-steps check, so the legitimate path still works. +- **Cross-tenant write scoping.** Added `.eq('space_id', member.space_id)` to `updateForumThread`, `deleteForumThread`, `editComment`, `deleteComment`, `deleteChannel`, `renameChannel` (was RLS-only; now defence-in-depth). +- **Invite codes.** `auth-actions.generateInviteCode` moved off `Math.random()` to `crypto.getRandomValues` and widened to 8 chars (was ~21 bits, enumerable under the join rate limit). +- **SafeMarkdown hardened.** Dropped `iframe` from allowed tags (no unsandboxed embed/token-theft frame) and the global `style` attribute (no full-viewport UI-redress). Markdown + safe basic HTML still render. +- **`/settings` server gate.** Now `redirect('/dashboard')` for non-admins BEFORE fetching space/integration config (was client-hidden only). +- **`app/(app)/layout.tsx`** marked `export const dynamic = 'force-dynamic'` (auth/onboarding gate must never be cached). +- **Bounded queries.** `loadComments` `.limit(500)`, tasks page `.limit(2000)`. + +### Tracked backlog (audit findings, not security-critical) + +- `importMembers` / `importPaymentsCsv`: still no Zod row schema; dates not via `flexibleDateTime()`; emails not lowercased; silent row drops. Highest-value remaining correctness item. +- `types/database.ts` still missing 8 prior-pass tables + `comment_entity_type` enum + 3 `space_members` columns (loose casts; build green via `ignoreBuildErrors`). +- `docs/DATABASE_SCHEMA.md`, `DB_SCHEMA_MAP.md`, `docs/API_REFERENCE.md` stale since 2026-03-10 (missing migrations 016-024). +- Inconsistent unauthorized handling: `/ops` and `/import` `return null` instead of `redirect`. +- Email normalization (`.toLowerCase()`) missing across auth/member schemas. +- KB/process per-item ACL: infra complete, only the ops-client KB-modal hookup remains. Members-row "make area lead" shortcut still deferred. + +### Verification + +- `pnpm build` clean; `pnpm test` 271/271. Migrations 023 + 024 idempotent; apply via deploy.sh's tracked runner on next deploy. + ## 2026-05-15 (pass 15) — Permissions, Ops ACLs, area-lead roles Branch: `main`. Migration: 023 (applies on next deploy). diff --git a/lib/actions/comms.ts b/lib/actions/comms.ts index 146e3f5..2bc5925 100644 --- a/lib/actions/comms.ts +++ b/lib/actions/comms.ts @@ -55,12 +55,14 @@ export async function deleteChannel(channelId: string) { const supabase = await createClient() const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } + const { member } = auth // Default channels are protected at the RLS layer too. const { error, count } = await supabase .from('comms_channels') .delete({ count: 'exact' }) .eq('id', v.data) + .eq('space_id', member.space_id) if (error) return { error: error.message } if (count === 0) return { error: 'Cannot delete this channel' } @@ -77,11 +79,13 @@ export async function renameChannel(channelId: string, name: string, description const supabase = await createClient() const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } + const { member } = auth const { error } = await supabase .from('comms_channels') .update({ name: v.data.name, description: v.data.description ?? null }) .eq('id', idCheck.data) + .eq('space_id', member.space_id) if (error) return { error: error.message } revalidatePath('/comms') diff --git a/lib/actions/forum.ts b/lib/actions/forum.ts index 2fc4ad0..f54b77c 100644 --- a/lib/actions/forum.ts +++ b/lib/actions/forum.ts @@ -56,6 +56,7 @@ export async function updateForumThread(threadId: string, updates: { const supabase = await createClient() const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } + const { member } = auth // Pinning/locking is moderator-only (admin/board). The RLS UPDATE policy // additionally requires the actor to be the author or admin/board, so a @@ -73,7 +74,7 @@ export async function updateForumThread(threadId: string, updates: { if (v.data.pinned !== undefined) patch.pinned = v.data.pinned if (v.data.locked !== undefined) patch.locked = v.data.locked - const { error } = await supabase.from('forum_threads').update(patch).eq('id', idCheck.data) + const { error } = await supabase.from('forum_threads').update(patch).eq('id', idCheck.data).eq('space_id', member.space_id) if (error) return { error: error.message } revalidatePath('/forum') revalidatePath(`/forum/${idCheck.data}`) @@ -87,6 +88,7 @@ export async function deleteForumThread(threadId: string) { const supabase = await createClient() const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } + const { member } = auth // RLS DELETE allows the author or admin; non-authors get a row-not-found // error which we translate to a clearer message. @@ -94,6 +96,7 @@ export async function deleteForumThread(threadId: string) { .from('forum_threads') .delete({ count: 'exact' }) .eq('id', v.data) + .eq('space_id', member.space_id) if (error) return { error: error.message } if (count === 0) return { error: 'Not allowed to delete this thread' } @@ -159,11 +162,13 @@ export async function editComment(commentId: string, body: string) { const supabase = await createClient() const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } + const { member } = auth const { data, error } = await supabase .from('comments') .update({ body: v.data.body, edited_at: new Date().toISOString() }) .eq('id', idCheck.data) + .eq('space_id', member.space_id) .select('entity_type, entity_id') .single() @@ -179,11 +184,13 @@ export async function deleteComment(commentId: string) { const supabase = await createClient() const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } + const { member } = auth const { data, error } = await supabase .from('comments') .delete() .eq('id', v.data) + .eq('space_id', member.space_id) .select('entity_type, entity_id') .single() diff --git a/lib/actions/onboarding.ts b/lib/actions/onboarding.ts index 63f24c7..79ce6a4 100644 --- a/lib/actions/onboarding.ts +++ b/lib/actions/onboarding.ts @@ -2,6 +2,7 @@ import { revalidatePath } from 'next/cache' import { createClient } from '@/lib/supabase/server' +import { createAdminClient } from '@/lib/supabase/admin' import { requireMember, requireMemberWithRole, parseInput } from '@/lib/auth-helpers' import { createOnboardingStepSchema, @@ -173,10 +174,16 @@ export async function finishOnboarding() { return { error: 'Please complete the required steps before finishing.' } } - const { error } = await supabase + // onboarding_completed_at is blocked by the self-change trigger (migration + // 024) so a member cannot skip required steps via a raw PostgREST call. + // Set it via the service client AFTER the required-steps check above, scoped + // to this member's own id. + const admin = createAdminClient() + const { error } = await admin .from('space_members') .update({ onboarding_completed_at: new Date().toISOString() }) .eq('id', member.id) + .eq('space_id', member.space_id) if (error) return { error: error.message } revalidatePath('/dashboard') @@ -201,10 +208,12 @@ export async function skipOnboarding() { return { error: 'This space requires you to complete onboarding.' } } - const { error } = await supabase + const admin = createAdminClient() + const { error } = await admin .from('space_members') .update({ onboarding_completed_at: new Date().toISOString() }) .eq('id', member.id) + .eq('space_id', member.space_id) if (error) return { error: error.message } revalidatePath('/dashboard') diff --git a/lib/auth-actions.ts b/lib/auth-actions.ts index 50eb9d5..b4e42e1 100644 --- a/lib/auth-actions.ts +++ b/lib/auth-actions.ts @@ -6,11 +6,15 @@ import { redirect } from 'next/navigation' import { checkRateLimit, sanitizeString, sanitizeSlug } from '@/lib/security' function generateInviteCode() { - const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789' + // Cryptographically random, and wider (8 chars) so the code space is not + // brute-forceable under the joinSpace rate limit. + const chars = 'ABCDEFGHJKLMNPQRSTUVWXYZ23456789' const prefix = 'HSL' const year = new Date().getFullYear() + const bytes = new Uint8Array(8) + crypto.getRandomValues(bytes) let code = '' - for (let i = 0; i < 4; i++) code += chars.charAt(Math.floor(Math.random() * chars.length)) + for (let i = 0; i < 8; i++) code += chars[bytes[i] % chars.length] return `${prefix}-${year}-${code}` } diff --git a/scripts/024_self_change_hardening.sql b/scripts/024_self_change_hardening.sql new file mode 100644 index 0000000..7166d00 --- /dev/null +++ b/scripts/024_self_change_hardening.sql @@ -0,0 +1,61 @@ +-- ============================================================================= +-- 024: Close member self-escalation gaps in prevent_member_self_role_change +-- ============================================================================= +-- The members_update RLS policy intentionally lets a member update their own +-- row (display_name, handle, bio, skills, ...). The 015 trigger blocks the +-- privileged columns, but two columns added by later migrations were not in +-- its list: +-- +-- tier_id (021) - a member could self-upgrade their tier +-- onboarding_completed_at (022) - a member could skip required onboarding +-- (e.g. the code-of-conduct acknowledgement) +-- by setting this directly via PostgREST +-- +-- finishOnboarding()/skipOnboarding() now set onboarding_completed_at through +-- the service-role client AFTER server-side validation, so blocking it here +-- does not break the legitimate path. +-- ============================================================================= + +CREATE OR REPLACE FUNCTION public.prevent_member_self_role_change() + RETURNS trigger + LANGUAGE plpgsql SECURITY DEFINER + SET search_path = public +AS $$ +DECLARE + v_is_privileged boolean; +BEGIN + IF auth.uid() IS NULL THEN + RETURN NEW; + END IF; + + IF NEW.user_id IS DISTINCT FROM auth.uid() THEN + RETURN NEW; + END IF; + + SELECT public.user_has_role_in_space(auth.uid(), NEW.space_id, ARRAY['admin','board','treasurer']) + INTO v_is_privileged; + + IF v_is_privileged THEN + RETURN NEW; + END IF; + + IF NEW.role IS DISTINCT FROM OLD.role + OR NEW.tier IS DISTINCT FROM OLD.tier + OR NEW.tier_id IS DISTINCT FROM OLD.tier_id + OR NEW.status IS DISTINCT FROM OLD.status + OR NEW.approved IS DISTINCT FROM OLD.approved + OR NEW.has_card_access IS DISTINCT FROM OLD.has_card_access + OR NEW.onboarding_completed_at IS DISTINCT FROM OLD.onboarding_completed_at + OR NEW.space_id IS DISTINCT FROM OLD.space_id THEN + RAISE EXCEPTION 'Members cannot change their own role, tier, status, approval, card access, onboarding completion, or space.' + USING ERRCODE = '42501'; + END IF; + + RETURN NEW; +END; +$$; + +DROP TRIGGER IF EXISTS trg_prevent_member_self_role_change ON public.space_members; +CREATE TRIGGER trg_prevent_member_self_role_change + BEFORE UPDATE ON public.space_members + FOR EACH ROW EXECUTE FUNCTION public.prevent_member_self_role_change(); diff --git a/scripts/schema.sql b/scripts/schema.sql index 95d5bc7..566417e 100644 --- a/scripts/schema.sql +++ b/scripts/schema.sql @@ -1836,3 +1836,41 @@ DROP TRIGGER IF EXISTS trg_seed_default_role_permissions ON public.spaces; CREATE TRIGGER trg_seed_default_role_permissions AFTER INSERT ON public.spaces FOR EACH ROW EXECUTE FUNCTION public.seed_default_role_permissions(); + + +-- ============================================================================= +-- 16. Self-change hardening (equivalent to scripts/024_self_change_hardening.sql). +-- Adds tier_id and onboarding_completed_at to the columns a non-privileged +-- member cannot change on their own space_members row. +-- ============================================================================= + +CREATE OR REPLACE FUNCTION public.prevent_member_self_role_change() + RETURNS trigger LANGUAGE plpgsql SECURITY DEFINER SET search_path = public +AS $$ +DECLARE + v_is_privileged boolean; +BEGIN + IF auth.uid() IS NULL THEN RETURN NEW; END IF; + IF NEW.user_id IS DISTINCT FROM auth.uid() THEN RETURN NEW; END IF; + SELECT public.user_has_role_in_space(auth.uid(), NEW.space_id, ARRAY['admin','board','treasurer']) + INTO v_is_privileged; + IF v_is_privileged THEN RETURN NEW; END IF; + IF NEW.role IS DISTINCT FROM OLD.role + OR NEW.tier IS DISTINCT FROM OLD.tier + OR NEW.tier_id IS DISTINCT FROM OLD.tier_id + OR NEW.status IS DISTINCT FROM OLD.status + OR NEW.approved IS DISTINCT FROM OLD.approved + OR NEW.has_card_access IS DISTINCT FROM OLD.has_card_access + OR NEW.onboarding_completed_at IS DISTINCT FROM OLD.onboarding_completed_at + OR NEW.space_id IS DISTINCT FROM OLD.space_id THEN + RAISE EXCEPTION 'Members cannot change their own role, tier, status, approval, card access, onboarding completion, or space.' + USING ERRCODE = '42501'; + END IF; + RETURN NEW; +END; +$$; + +DROP TRIGGER IF EXISTS trg_prevent_member_self_role_change ON public.space_members; +CREATE TRIGGER trg_prevent_member_self_role_change + BEFORE UPDATE ON public.space_members + FOR EACH ROW EXECUTE FUNCTION public.prevent_member_self_role_change();