From e75732ce6826b6d9b75ce4dbfe20e0d51c8ab117 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 26 Oct 2026 20:34:54 +0000 Subject: [PATCH] Lock down SECURITY DEFINER functions, privileged columns and login redirects - Revoke 11 internal SECURITY DEFINER functions (call-analysis claim, cron cleanups, system chat messages, agent join with owner id, effective plan, monthly usage, autoblog bump, backup-host promotion, auth trigger) from anon/authenticated; service role keeps them. - start_media_session: only the room's host may publish (any signed-in user could make themselves current host of any room). - New functions are no longer executable by PUBLIC/anon/authenticated by default; browser RPCs need an explicit GRANT. - Pin privileged columns behind the update-own policies: participant role, kind, agent owner, backup host, un-leave; session channel/org/team, creator and notify/recording stamps; profile email and created_at. Drop the direct session_participants INSERT policy (joins go through join_session, which enforces plan seat caps). - Login and auth callback redirects rejected '//host' but not '/\\host', which the URL parser reads as another origin. Shared sameOriginPath() now rejects backslashes and control characters and checks the origin. Migration applied by hand on dev2 (backup taken first). Co-Authored-By: Claude Opus 5.5 --- apps/web/src/app/auth/callback/route.test.ts | 2 + apps/web/src/app/auth/callback/route.ts | 7 +- .../src/components/auth/LoginForm.test.tsx | 1 + apps/web/src/components/auth/LoginForm.tsx | 7 +- apps/web/src/lib/safe-redirect.test.ts | 25 +++ apps/web/src/lib/safe-redirect.ts | 20 ++ .../20261026230000_secdef_lockdown.sql | 212 ++++++++++++++++++ 7 files changed, 264 insertions(+), 10 deletions(-) create mode 100644 apps/web/src/lib/safe-redirect.test.ts create mode 100644 apps/web/src/lib/safe-redirect.ts create mode 100644 supabase/migrations/20261026230000_secdef_lockdown.sql diff --git a/apps/web/src/app/auth/callback/route.test.ts b/apps/web/src/app/auth/callback/route.test.ts index 1bc0af03..4a12dbe3 100644 --- a/apps/web/src/app/auth/callback/route.test.ts +++ b/apps/web/src/app/auth/callback/route.test.ts @@ -11,6 +11,8 @@ describe('safeNextPath', () => { it('falls back for external callback targets', () => { expect(safeNextPath('https://evil.example/phish')).toBe('/dashboard'); expect(safeNextPath('//evil.example/phish')).toBe('/dashboard'); + expect(safeNextPath('/\\evil.example/phish')).toBe('/dashboard'); + expect(safeNextPath('/\t/evil.example/phish')).toBe('/dashboard'); }); it('falls back for missing or relative callback targets', () => { diff --git a/apps/web/src/app/auth/callback/route.ts b/apps/web/src/app/auth/callback/route.ts index 1c3a6bfd..bb5bb161 100644 --- a/apps/web/src/app/auth/callback/route.ts +++ b/apps/web/src/app/auth/callback/route.ts @@ -1,6 +1,7 @@ /* eslint-disable @typescript-eslint/no-deprecated */ import { createServerClient, type CookieOptions } from '@supabase/ssr'; import { NextResponse, type NextRequest } from 'next/server'; +import { sameOriginPath } from '@/lib/safe-redirect'; import { exchangeCodeAndPersist } from '@/lib/supabase/exchange-code'; import { INVITE_COOKIE, cookieFrom, onSignedIn, refFromRequest } from '@/lib/credits'; @@ -27,11 +28,7 @@ async function afterSignIn( } export function safeNextPath(next: string | null): string { - if (!next || !next.startsWith('/') || next.startsWith('//')) { - return '/dashboard'; - } - - return next; + return sameOriginPath(next) ?? '/dashboard'; } export async function GET(request: NextRequest) { diff --git a/apps/web/src/components/auth/LoginForm.test.tsx b/apps/web/src/components/auth/LoginForm.test.tsx index f226e151..251a7819 100644 --- a/apps/web/src/components/auth/LoginForm.test.tsx +++ b/apps/web/src/components/auth/LoginForm.test.tsx @@ -27,6 +27,7 @@ describe('safeRedirectPath', () => { expect(safeRedirectPath(null)).toBe('/dashboard'); expect(safeRedirectPath('https://evil.example/phish')).toBe('/dashboard'); expect(safeRedirectPath('//evil.example/phish')).toBe('/dashboard'); + expect(safeRedirectPath('/\\evil.example/phish')).toBe('/dashboard'); expect(safeRedirectPath('dashboard')).toBe('/dashboard'); }); }); diff --git a/apps/web/src/components/auth/LoginForm.tsx b/apps/web/src/components/auth/LoginForm.tsx index e3b87247..b167ea5a 100644 --- a/apps/web/src/components/auth/LoginForm.tsx +++ b/apps/web/src/components/auth/LoginForm.tsx @@ -7,13 +7,10 @@ import Link from 'next/link'; import { Mail, Lock, Eye, EyeOff, Loader2, AlertCircle, Key } from 'lucide-react'; import { useTranslations } from 'next-intl'; import { trackLogin } from '@/lib/analytics'; +import { sameOriginPath } from '@/lib/safe-redirect'; export function safeRedirectPath(redirect: string | null): string { - if (!redirect || !redirect.startsWith('/') || redirect.startsWith('//')) { - return '/dashboard'; - } - - return redirect; + return sameOriginPath(redirect) ?? '/dashboard'; } export function LoginForm() { diff --git a/apps/web/src/lib/safe-redirect.test.ts b/apps/web/src/lib/safe-redirect.test.ts new file mode 100644 index 00000000..face09da --- /dev/null +++ b/apps/web/src/lib/safe-redirect.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it } from 'vitest'; +import { sameOriginPath } from './safe-redirect'; + +describe('sameOriginPath', () => { + it('keeps same-site paths', () => { + expect(sameOriginPath('/dashboard')).toBe('/dashboard'); + expect(sameOriginPath('/r/abc?x=1#k=2')).toBe('/r/abc?x=1#k=2'); + }); + + it('rejects anything that can leave the site', () => { + for (const bad of [ + null, + '', + 'https://evil.com', + '//evil.com', + '/\\evil.com', + '/\\/evil.com', + '/\t/evil.com', + '/\n/evil.com', + 'javascript:alert(1)', + ]) { + expect(sameOriginPath(bad)).toBeNull(); + } + }); +}); diff --git a/apps/web/src/lib/safe-redirect.ts b/apps/web/src/lib/safe-redirect.ts new file mode 100644 index 00000000..f9ac2b6e --- /dev/null +++ b/apps/web/src/lib/safe-redirect.ts @@ -0,0 +1,20 @@ +const BASE = 'https://same-origin.invalid'; + +/** + * A redirect target that stays on this site, or null. Accepts only a path + * ("/dashboard?x=1"). Rejects protocol-relative "//host", backslash forms + * ("/\\host", which browsers and the URL parser read as "//host") and control + * characters (tabs and newlines are stripped by the URL parser, so "/\t/host" + * also becomes "//host"). + */ +export function sameOriginPath(target: string | null | undefined): string | null { + if (!target || !target.startsWith('/') || target.startsWith('//')) return null; + // eslint-disable-next-line no-control-regex + if (/[\\\u0000-\u001f\u007f]/.test(target)) return null; + try { + if (new URL(target, BASE).origin !== BASE) return null; + } catch { + return null; + } + return target; +} diff --git a/supabase/migrations/20261026230000_secdef_lockdown.sql b/supabase/migrations/20261026230000_secdef_lockdown.sql new file mode 100644 index 00000000..01918ae1 --- /dev/null +++ b/supabase/migrations/20261026230000_secdef_lockdown.sql @@ -0,0 +1,212 @@ +-- Lock down SECURITY DEFINER functions and privileged columns (2026-10-26 audit). +-- +-- Supabase grants EXECUTE to PUBLIC on every new function, so each SECURITY +-- DEFINER function in public was callable with the anon key. This migration: +-- 1. revokes the internal ones (cron, triggers, service-role routes) from +-- PUBLIC/anon/authenticated; +-- 2. adds a host check to start_media_session (any signed-in user could make +-- themselves current host of any room); +-- 3. stops new functions from being executable by PUBLIC/anon/authenticated +-- by default: a NEW function the browser calls (or an RLS policy uses) +-- now needs an explicit GRANT EXECUTE ... TO anon, authenticated; +-- 4. pins privileged columns that "update own row" policies let users write: +-- session_participants (role, kind, agent owner, backup host, un-leave), +-- sessions (channel/org/team, creator, notify/recording stamps), profiles +-- (email, created_at); and drops the direct INSERT policy on +-- session_participants (joins go through join_session, which enforces +-- the plan's seat caps). + +-- 1. Internal functions: server (service role), cron or triggers only. +revoke execute on function public.auto_promote_backup_host(uuid) from public, anon, authenticated; +revoke execute on function public.bump_autoblog_integration(uuid) from public, anon, authenticated; +revoke execute on function public.claim_call_analysis() from public, anon, authenticated; +revoke execute on function public.cleanup_expired_rooms() from public, anon, authenticated; +revoke execute on function public.mark_stale_participants() from public, anon, authenticated; +revoke execute on function public.prune_meeting_reminders() from public, anon, authenticated; +revoke execute on function public.get_user_monthly_usage(uuid, integer, integer) from public, anon, authenticated; +revoke execute on function public.join_session_as_agent(text, text, text, uuid) from public, anon, authenticated; +revoke execute on function public.send_system_message(uuid, text, text) from public, anon, authenticated; +revoke execute on function public.user_effective_plan(uuid) from public, anon, authenticated; +revoke execute on function public.handle_new_user() from public, anon, authenticated; + +grant execute on function public.auto_promote_backup_host(uuid) to service_role; +grant execute on function public.bump_autoblog_integration(uuid) to service_role; +grant execute on function public.claim_call_analysis() to service_role; +grant execute on function public.cleanup_expired_rooms() to service_role; +grant execute on function public.mark_stale_participants() to service_role; +grant execute on function public.prune_meeting_reminders() to service_role; +grant execute on function public.get_user_monthly_usage(uuid, integer, integer) to service_role; +grant execute on function public.join_session_as_agent(text, text, text, uuid) to service_role; +grant execute on function public.send_system_message(uuid, text, text) to service_role; +grant execute on function public.user_effective_plan(uuid) to service_role; + +-- 2. start_media_session: only the room's host may publish and take current host. +create or replace function public.start_media_session( + p_room_id uuid, + p_mode session_mode default 'p2p'::session_mode, + p_capture_source jsonb default null::jsonb, + p_sfu_endpoint text default null::text, + p_sfu_room_id text default null::text +) +returns media_sessions +language plpgsql +security definer +set search_path to 'public' +as $function$ +declare + v_session public.sessions; + v_media_session public.media_sessions; +begin + if auth.uid() is null then + raise exception 'Authentication required'; + end if; + + select * into v_session + from public.sessions + where id = p_room_id and status != 'ended'; + + if v_session is null then + raise exception 'Room not found or has ended'; + end if; + + if not ( + auth.uid() in (v_session.creator_id, v_session.host_user_id, v_session.current_host_id) + or exists ( + select 1 from public.session_participants sp + where sp.session_id = p_room_id and sp.user_id = auth.uid() + and sp.role = 'host' and sp.left_at is null + ) + ) then + raise exception 'Only the host can start sharing in this room'; + end if; + + update public.media_sessions + set status = 'ended', ended_at = now() + where room_id = p_room_id + and publisher_id = auth.uid() + and status = 'active'; + + insert into public.media_sessions (room_id, publisher_id, mode, capture_source, sfu_endpoint, sfu_room_id) + values (p_room_id, auth.uid(), p_mode, p_capture_source, p_sfu_endpoint, p_sfu_room_id) + returning * into v_media_session; + + update public.sessions + set current_host_id = auth.uid(), host_last_seen_at = now(), status = 'active', mode = p_mode + where id = p_room_id; + + return v_media_session; +end; +$function$; + +-- The 3-argument overload delegates so both carry the same check. +create or replace function public.start_media_session( + p_room_id uuid, + p_mode session_mode default 'p2p'::session_mode, + p_capture_source jsonb default null::jsonb +) +returns media_sessions +language sql +security invoker +set search_path to 'public' +as $function$ + select public.start_media_session(p_room_id, p_mode, p_capture_source, null::text, null::text); +$function$; + +-- 3. New functions are not executable by PUBLIC/anon/authenticated by default. +alter default privileges for role postgres revoke execute on functions from public; +alter default privileges for role postgres in schema public revoke execute on functions from public, anon, authenticated; + +-- 4a. session_participants: no direct inserts (join_session enforces seat caps). +drop policy if exists "Users can join sessions" on public.session_participants; + +create or replace function public.session_participants_protect() +returns trigger +language plpgsql +set search_path to 'public' +as $function$ +begin + if current_user not in ('anon', 'authenticated') then + return new; + end if; + new.id := old.id; + new.session_id := old.session_id; + new.user_id := old.user_id; + new.kind := old.kind; + new.agent_owner_id := old.agent_owner_id; + new.agent_client := old.agent_client; + new.joined_at := old.joined_at; + if old.left_at is not null then + new.left_at := old.left_at; + end if; + if not exists ( + select 1 from public.sessions s + where s.id = old.session_id + and auth.uid() in (s.host_user_id, s.current_host_id, s.creator_id) + ) then + new.role := old.role; + new.is_backup_host := old.is_backup_host; + end if; + return new; +end; +$function$; + +drop trigger if exists session_participants_protect on public.session_participants; +create trigger session_participants_protect + before update on public.session_participants + for each row execute function public.session_participants_protect(); + +-- 4b. sessions: the host may edit their room, not re-home it or reset stamps. +create or replace function public.sessions_protect() +returns trigger +language plpgsql +set search_path to 'public' +as $function$ +begin + if current_user not in ('anon', 'authenticated') then + return new; + end if; + new.id := old.id; + new.creator_id := old.creator_id; + new.created_at := old.created_at; + new.channel_id := old.channel_id; + new.org_id := old.org_id; + new.team_id := old.team_id; + new.live_notified_at := old.live_notified_at; + new.recording_started_at := old.recording_started_at; + new.restream_egress_id := old.restream_egress_id; + return new; +end; +$function$; + +drop trigger if exists sessions_protect on public.sessions; +create trigger sessions_protect + before update on public.sessions + for each row execute function public.sessions_protect(); + +-- 4c. profiles: plan columns are pinned by profiles_protect_plan; also pin +-- email (written by the service-role profile API) and created_at. +create or replace function public.profiles_protect_identity() +returns trigger +language plpgsql +set search_path to 'public' +as $function$ +begin + if current_user in ('anon', 'authenticated') then + new.email := old.email; + new.created_at := old.created_at; + end if; + return new; +end; +$function$; + +drop trigger if exists profiles_protect_identity on public.profiles; +create trigger profiles_protect_identity + before update on public.profiles + for each row execute function public.profiles_protect_identity(); + +-- Trigger functions are never RPCs. +revoke execute on function public.session_participants_protect() from public, anon, authenticated; +revoke execute on function public.sessions_protect() from public, anon, authenticated; +revoke execute on function public.profiles_protect_identity() from public, anon, authenticated; + +notify pgrst, 'reload schema';