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.
2026-05-15 17:35:11 -07:00
parent 9447b5ee80
commit f5d670477b
13 changed files with 187 additions and 18 deletions
@@ -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).
@@ -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()
@@ -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 (
    <SettingsClient
      space={space}
@@ -17,7 +17,7 @@ export default async function TasksPage() {
  if (!spaceId) return null

  const [{ data: tasks }, { data: members }, { data: areasRaw }] = await Promise.all([
-    supabase.from('tasks').select('*').eq('space_id', spaceId).order('created_at', { ascending: false }),
+    supabase.from('tasks').select('*').eq('space_id', spaceId).order('created_at', { ascending: false }).limit(2000),
    supabase.from('space_members').select('id, display_name, user_id').eq('space_id', spaceId).in('status', ['current', 'late']),
    supabase
      .from('space_areas')
@@ -14,6 +14,9 @@ export async function loadComments(
    .eq('entity_type', entityType)
    .eq('entity_id', entityId)
    .order('created_at', { ascending: true })
+    // Bound the payload: a thread with thousands of comments must not be able
+    // to hang the page. Newest 500 is plenty for the current UI.
+    .limit(500)

  const rows = data ?? []
  const authorIds = Array.from(new Set(rows.map(r => r.author_id).filter(Boolean) as string[]))
@@ -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'],
  },
 }
@@ -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).
@@ -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')
@@ -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()

@@ -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')
@@ -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}`
 }

@@ -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();
@@ -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();