From 72c4ddaefbcda6a77e866365267ba9de796364aa Mon Sep 17 00:00:00 2001 From: Yun Chan Date: Mon, 28 Sep 2026 00:54:01 +0900 Subject: [PATCH] fix(supabase): require team membership for team-scoped writes and pin row ownership --- ...0260927000038_team_scoped_write_checks.sql | 183 +++++++++++++ .../team-scoped-write-checks.integration.sql | 252 ++++++++++++++++++ 2 files changed, 435 insertions(+) create mode 100644 server/supabase/migrations/20260927000038_team_scoped_write_checks.sql create mode 100644 server/supabase/tests/team-scoped-write-checks.integration.sql diff --git a/server/supabase/migrations/20260927000038_team_scoped_write_checks.sql b/server/supabase/migrations/20260927000038_team_scoped_write_checks.sql new file mode 100644 index 0000000..d56d19b --- /dev/null +++ b/server/supabase/migrations/20260927000038_team_scoped_write_checks.sql @@ -0,0 +1,183 @@ +-- Team-scoped write checks for meetings, meeting_documents and knowledge. +-- +-- Two holes in the shipped policies: +-- +-- 1) Publishing into a team without being a member of it. +-- meetings_insert and knowledge_documents_insert only checked user_id. +-- meetings_update (20260411000001) and knowledge_documents_update +-- (20260410000002) had no WITH CHECK, so Postgres reused USING, which also +-- ignores team_id. Anyone who knew a team uuid (a removed member, or an +-- invitee who never joined) could INSERT a row with team_id = T, or move +-- one of their own rows into T. meetings_read then showed it to every T +-- member, and match_knowledge_chunks fed the chunks into every T member's +-- RAG answers. remove_team_member only deletes the team_members row, so +-- removal did not close this route. +-- +-- Now every write that leaves a row with team_id set requires the writer +-- to be a current member of that team. A removed member keeps full +-- control of their own rows (read, delete, and unsharing by setting +-- team_id back to NULL), but can no longer write content that the team +-- sees. knowledge_chunks_insert gets the same rule, so a removed member +-- cannot keep adding chunks to a document that is still shared. +-- +-- 2) Team admins taking over a member's rows. +-- The admin branch of meetings_update and meeting_documents_update passed +-- USING, and with no WITH CHECK the rewritten row passed the owner branch +-- once the admin set user_id to their own id. meetings_delete and +-- meeting_documents_delete_own are owner-only, so the admin could then +-- delete the member's meeting (cascading transcripts, memos and documents) +-- and the sync tombstone went to the admin instead of the owner. +-- +-- Ownership is now immutable for JWT callers: a BEFORE UPDATE trigger +-- rejects any change of user_id, and rejects a non-owner moving the row +-- to another team (meetings) or another meeting (meeting_documents). +-- Clearing team_id stays allowed so ON DELETE SET NULL from teams keeps +-- working. The WITH CHECK clauses also pin the owner branch: an owner can +-- only attach a document to a meeting they can see, the same rule as +-- meeting_documents_insert. +-- +-- Service-role and internal jobs (no auth.uid()) are not affected by the +-- triggers, and SECURITY DEFINER RPCs never write team_id or user_id on these +-- tables (the portability import forces team_id NULL). + +-- ── meetings ───────────────────────────────────────────────────────────── + +DROP POLICY IF EXISTS "meetings_insert" ON public.meetings; +CREATE POLICY "meetings_insert" ON public.meetings + FOR INSERT WITH CHECK ( + user_id = auth.uid() + AND (team_id IS NULL OR team_id IN (SELECT public.user_team_ids(auth.uid()))) + ); + +DROP POLICY IF EXISTS "meetings_update" ON public.meetings; +CREATE POLICY "meetings_update" ON public.meetings + FOR UPDATE + USING ( + user_id = auth.uid() + OR (team_id IS NOT NULL AND team_id IN (SELECT public.user_admin_team_ids(auth.uid()))) + ) + WITH CHECK ( + ( + user_id = auth.uid() + OR (team_id IS NOT NULL AND team_id IN (SELECT public.user_admin_team_ids(auth.uid()))) + ) + AND (team_id IS NULL OR team_id IN (SELECT public.user_team_ids(auth.uid()))) + ); + +CREATE OR REPLACE FUNCTION public.enforce_meeting_ownership_v1() +RETURNS trigger +LANGUAGE plpgsql +SET search_path = '' +AS $$ +DECLARE + actor_id uuid := auth.uid(); +BEGIN + IF actor_id IS NULL THEN + RETURN NEW; + END IF; + IF NEW.user_id IS DISTINCT FROM OLD.user_id THEN + RAISE EXCEPTION 'meeting_owner_immutable' USING ERRCODE = '42501'; + END IF; + IF NEW.team_id IS DISTINCT FROM OLD.team_id + AND NEW.team_id IS NOT NULL + AND OLD.user_id IS DISTINCT FROM actor_id THEN + RAISE EXCEPTION 'meeting_team_owner_only' USING ERRCODE = '42501'; + END IF; + RETURN NEW; +END; +$$; + +REVOKE ALL ON FUNCTION public.enforce_meeting_ownership_v1() + FROM PUBLIC, anon, authenticated; + +DROP TRIGGER IF EXISTS meetings_ownership_v1 ON public.meetings; +CREATE TRIGGER meetings_ownership_v1 +BEFORE UPDATE OF user_id, team_id +ON public.meetings +FOR EACH ROW EXECUTE FUNCTION public.enforce_meeting_ownership_v1(); + +-- ── meeting_documents ──────────────────────────────────────────────────── + +DROP POLICY IF EXISTS "meeting_documents_update" ON public.meeting_documents; +CREATE POLICY "meeting_documents_update" ON public.meeting_documents + FOR UPDATE + USING ( + user_id = auth.uid() + OR meeting_id IN ( + SELECT id FROM public.meetings + WHERE team_id IN (SELECT public.user_admin_team_ids(auth.uid())) + ) + ) + WITH CHECK ( + ( + user_id = auth.uid() + OR meeting_id IN ( + SELECT id FROM public.meetings + WHERE team_id IN (SELECT public.user_admin_team_ids(auth.uid())) + ) + ) + AND meeting_id IN ( + SELECT id FROM public.meetings + WHERE user_id = auth.uid() + OR (team_id IS NOT NULL AND team_id IN (SELECT public.user_team_ids(auth.uid()))) + ) + ); + +CREATE OR REPLACE FUNCTION public.enforce_meeting_document_ownership_v1() +RETURNS trigger +LANGUAGE plpgsql +SET search_path = '' +AS $$ +DECLARE + actor_id uuid := auth.uid(); +BEGIN + IF actor_id IS NULL THEN + RETURN NEW; + END IF; + IF NEW.user_id IS DISTINCT FROM OLD.user_id THEN + RAISE EXCEPTION 'meeting_document_owner_immutable' USING ERRCODE = '42501'; + END IF; + IF NEW.meeting_id IS DISTINCT FROM OLD.meeting_id + AND OLD.user_id IS DISTINCT FROM actor_id THEN + RAISE EXCEPTION 'meeting_document_meeting_owner_only' USING ERRCODE = '42501'; + END IF; + RETURN NEW; +END; +$$; + +REVOKE ALL ON FUNCTION public.enforce_meeting_document_ownership_v1() + FROM PUBLIC, anon, authenticated; + +DROP TRIGGER IF EXISTS meeting_documents_ownership_v1 ON public.meeting_documents; +CREATE TRIGGER meeting_documents_ownership_v1 +BEFORE UPDATE OF user_id, meeting_id +ON public.meeting_documents +FOR EACH ROW EXECUTE FUNCTION public.enforce_meeting_document_ownership_v1(); + +-- ── knowledge_documents / knowledge_chunks ─────────────────────────────── + +DROP POLICY IF EXISTS "knowledge_documents_insert" ON public.knowledge_documents; +CREATE POLICY "knowledge_documents_insert" ON public.knowledge_documents + FOR INSERT WITH CHECK ( + user_id = auth.uid() + AND (team_id IS NULL OR team_id IN (SELECT public.user_team_ids(auth.uid()))) + ); + +DROP POLICY IF EXISTS "knowledge_documents_update" ON public.knowledge_documents; +CREATE POLICY "knowledge_documents_update" ON public.knowledge_documents + FOR UPDATE + USING (user_id = auth.uid()) + WITH CHECK ( + user_id = auth.uid() + AND (team_id IS NULL OR team_id IN (SELECT public.user_team_ids(auth.uid()))) + ); + +DROP POLICY IF EXISTS "knowledge_chunks_insert" ON public.knowledge_chunks; +CREATE POLICY "knowledge_chunks_insert" ON public.knowledge_chunks + FOR INSERT WITH CHECK ( + document_id IN ( + SELECT id FROM public.knowledge_documents + WHERE user_id = auth.uid() + AND (team_id IS NULL OR team_id IN (SELECT public.user_team_ids(auth.uid()))) + ) + ); diff --git a/server/supabase/tests/team-scoped-write-checks.integration.sql b/server/supabase/tests/team-scoped-write-checks.integration.sql new file mode 100644 index 0000000..bac621b --- /dev/null +++ b/server/supabase/tests/team-scoped-write-checks.integration.sql @@ -0,0 +1,252 @@ +\set ON_ERROR_STOP on + +-- Regression for 20260927000038_team_scoped_write_checks.sql. +-- 1) A non-member (or removed member) cannot publish meetings, knowledge +-- documents or knowledge chunks into a team. +-- 2) A team admin cannot take over a member's meeting or meeting document by +-- rewriting user_id, nor move it to another team or meeting. + +BEGIN; + +CREATE OR REPLACE FUNCTION pg_temp.assert_true(condition boolean, message text) +RETURNS void +LANGUAGE plpgsql +AS $$ +BEGIN + IF condition IS NOT TRUE THEN + RAISE EXCEPTION 'assertion_failed: %', message; + END IF; +END; +$$; + +CREATE OR REPLACE FUNCTION pg_temp.act_as(uid uuid) +RETURNS void +LANGUAGE sql +AS $$ + SELECT set_config( + 'request.jwt.claims', + json_build_object('sub', uid, 'role', 'authenticated')::text, + true + ); +$$; + +-- Must be rejected with 42501. expected_message, when set, pins the exact +-- trigger error so an RLS failure cannot mask a missing trigger (or the +-- reverse). +CREATE OR REPLACE FUNCTION pg_temp.assert_denied(stmt text, label text, expected_message text DEFAULT NULL) +RETURNS void +LANGUAGE plpgsql +AS $$ +DECLARE + affected bigint; +BEGIN + EXECUTE stmt; + GET DIAGNOSTICS affected = ROW_COUNT; + RAISE EXCEPTION 'write_was_allowed: % (% rows)', label, affected; +EXCEPTION + WHEN insufficient_privilege THEN + IF expected_message IS NOT NULL AND SQLERRM <> expected_message THEN + RAISE EXCEPTION 'unexpected_denial: % got "%" expected "%"', label, SQLERRM, expected_message; + END IF; +END; +$$; + +-- ── fixtures (as postgres) ─────────────────────────────────────────────── + +INSERT INTO auth.users ( + id, aud, role, email, encrypted_password, email_confirmed_at, + raw_app_meta_data, raw_user_meta_data, created_at, updated_at +) VALUES + ( + '38000000-0000-4000-8000-000000000001', 'authenticated', 'authenticated', + 'scoped-admin@example.invalid', crypt('fixture-password', gen_salt('bf')), now(), + '{"provider":"email","providers":["email"]}'::jsonb, '{"name":"Admin"}'::jsonb, now(), now() + ), + ( + '38000000-0000-4000-8000-000000000002', 'authenticated', 'authenticated', + 'scoped-member@example.invalid', crypt('fixture-password', gen_salt('bf')), now(), + '{"provider":"email","providers":["email"]}'::jsonb, '{"name":"Member"}'::jsonb, now(), now() + ), + ( + '38000000-0000-4000-8000-000000000003', 'authenticated', 'authenticated', + 'scoped-removed@example.invalid', crypt('fixture-password', gen_salt('bf')), now(), + '{"provider":"email","providers":["email"]}'::jsonb, '{"name":"Removed"}'::jsonb, now(), now() + ); + +-- Team T (admin owns, member joined) and team T2 (admin only). +INSERT INTO public.teams (id, name, owner_id) VALUES + ('38100000-0000-4000-8000-00000000000a', 'Scoped Team', '38000000-0000-4000-8000-000000000001'), + ('38100000-0000-4000-8000-00000000000b', 'Other Team', '38000000-0000-4000-8000-000000000001'); +INSERT INTO public.team_members (team_id, user_id, role) VALUES + ('38100000-0000-4000-8000-00000000000a', '38000000-0000-4000-8000-000000000001', 'owner'), + ('38100000-0000-4000-8000-00000000000a', '38000000-0000-4000-8000-000000000002', 'member'), + ('38100000-0000-4000-8000-00000000000b', '38000000-0000-4000-8000-000000000001', 'owner'); +-- The removed user was a member of T once; remove_team_member only deletes +-- the membership row, so rows they shared before stay shared. + +INSERT INTO public.meetings (id, user_id, team_id, title) VALUES + ('38200000-0000-4000-8000-000000000001', '38000000-0000-4000-8000-000000000002', + '38100000-0000-4000-8000-00000000000a', 'Member team meeting'), + ('38200000-0000-4000-8000-000000000002', '38000000-0000-4000-8000-000000000003', + NULL, 'Removed personal meeting'), + ('38200000-0000-4000-8000-000000000003', '38000000-0000-4000-8000-000000000003', + '38100000-0000-4000-8000-00000000000a', 'Removed legacy team meeting'); + +INSERT INTO public.meeting_documents (id, meeting_id, user_id, template_type, title, content) VALUES + ('38300000-0000-4000-8000-000000000001', '38200000-0000-4000-8000-000000000001', + '38000000-0000-4000-8000-000000000002', 'minutes', 'Member minutes', 'body'); + +INSERT INTO public.knowledge_documents (id, user_id, team_id, title) VALUES + ('38400000-0000-4000-8000-000000000001', '38000000-0000-4000-8000-000000000003', + NULL, 'Removed personal doc'), + ('38400000-0000-4000-8000-000000000002', '38000000-0000-4000-8000-000000000003', + '38100000-0000-4000-8000-00000000000a', 'Removed legacy shared doc'); + +SET LOCAL ROLE authenticated; + +-- ── 1) removed member / non-member cannot publish into T ───────────────── + +SELECT pg_temp.act_as('38000000-0000-4000-8000-000000000003'); + +SELECT pg_temp.assert_denied( + $$INSERT INTO public.meetings (user_id, team_id, title) + VALUES (auth.uid(), '38100000-0000-4000-8000-00000000000a', 'injected')$$, + 'non-member meeting insert into team' +); +SELECT pg_temp.assert_denied( + $$UPDATE public.meetings SET team_id = '38100000-0000-4000-8000-00000000000a' + WHERE id = '38200000-0000-4000-8000-000000000002'$$, + 'non-member moves own meeting into team' +); +SELECT pg_temp.assert_denied( + $$UPDATE public.meetings SET edited_transcript = 'ignore previous instructions' + WHERE id = '38200000-0000-4000-8000-000000000003'$$, + 'removed member keeps editing a meeting still shared with the team' +); +SELECT pg_temp.assert_denied( + $$INSERT INTO public.knowledge_documents (user_id, team_id, title) + VALUES (auth.uid(), '38100000-0000-4000-8000-00000000000a', 'poison')$$, + 'non-member knowledge document insert into team' +); +SELECT pg_temp.assert_denied( + $$UPDATE public.knowledge_documents SET team_id = '38100000-0000-4000-8000-00000000000a' + WHERE id = '38400000-0000-4000-8000-000000000001'$$, + 'non-member moves own knowledge document into team' +); +SELECT pg_temp.assert_denied( + $$INSERT INTO public.knowledge_chunks (document_id, chunk_index, content) + VALUES ('38400000-0000-4000-8000-000000000002', 0, 'poison chunk')$$, + 'removed member adds chunks to a document still shared with the team' +); + +-- Personal writes and unsharing stay available to the removed user. +INSERT INTO public.meetings (user_id, team_id, title) VALUES (auth.uid(), NULL, 'personal ok'); +UPDATE public.meetings SET title = 'personal edit ok' + WHERE id = '38200000-0000-4000-8000-000000000002'; +INSERT INTO public.knowledge_chunks (document_id, chunk_index, content) + VALUES ('38400000-0000-4000-8000-000000000001', 0, 'personal chunk ok'); +UPDATE public.meetings SET team_id = NULL + WHERE id = '38200000-0000-4000-8000-000000000003'; +UPDATE public.knowledge_documents SET team_id = NULL + WHERE id = '38400000-0000-4000-8000-000000000002'; + +RESET ROLE; +SELECT pg_temp.assert_true( + (SELECT count(*) FROM public.meetings + WHERE user_id = '38000000-0000-4000-8000-000000000003' + AND team_id IS NOT NULL) = 0, + 'removed member was able to unshare and has no team meetings left' +); +SELECT pg_temp.assert_true( + (SELECT title FROM public.meetings WHERE id = '38200000-0000-4000-8000-000000000002') + = 'personal edit ok', + 'owner edits of personal meetings still apply' +); +SET LOCAL ROLE authenticated; + +-- A current member can still share into the team. +SELECT pg_temp.act_as('38000000-0000-4000-8000-000000000002'); +INSERT INTO public.meetings (user_id, team_id, title) + VALUES (auth.uid(), '38100000-0000-4000-8000-00000000000a', 'member share ok'); +INSERT INTO public.knowledge_documents (id, user_id, team_id, title) + VALUES ('38400000-0000-4000-8000-000000000003', auth.uid(), + '38100000-0000-4000-8000-00000000000a', 'member shared doc ok'); +INSERT INTO public.knowledge_chunks (document_id, chunk_index, content) + VALUES ('38400000-0000-4000-8000-000000000003', 0, 'member chunk ok'); +UPDATE public.meetings SET title = 'member edit ok' + WHERE id = '38200000-0000-4000-8000-000000000001'; + +-- A member cannot attach their document to a meeting they cannot see. +SELECT pg_temp.assert_denied( + $$UPDATE public.meeting_documents SET meeting_id = '38200000-0000-4000-8000-000000000002' + WHERE id = '38300000-0000-4000-8000-000000000001'$$, + 'owner attaches document to a stranger''s private meeting' +); + +-- ── 2) admin cannot take over a member's rows ──────────────────────────── + +SELECT pg_temp.act_as('38000000-0000-4000-8000-000000000001'); + +SELECT pg_temp.assert_denied( + $$UPDATE public.meetings SET user_id = auth.uid() + WHERE id = '38200000-0000-4000-8000-000000000001'$$, + 'admin rewrites member meeting owner', + 'meeting_owner_immutable' +); +SELECT pg_temp.assert_denied( + $$UPDATE public.meetings SET team_id = '38100000-0000-4000-8000-00000000000b' + WHERE id = '38200000-0000-4000-8000-000000000001'$$, + 'admin moves member meeting to another team', + 'meeting_team_owner_only' +); +SELECT pg_temp.assert_denied( + $$UPDATE public.meeting_documents SET user_id = auth.uid() + WHERE id = '38300000-0000-4000-8000-000000000001'$$, + 'admin rewrites member document owner', + 'meeting_document_owner_immutable' +); + +-- Admin edit rights on content are unchanged. +UPDATE public.meetings SET title = 'admin edit ok' + WHERE id = '38200000-0000-4000-8000-000000000001'; +UPDATE public.meeting_documents SET content = 'admin edit ok' + WHERE id = '38300000-0000-4000-8000-000000000001'; + +-- Owner-only delete still cannot reach the member's rows. +DELETE FROM public.meetings WHERE id = '38200000-0000-4000-8000-000000000001'; +DELETE FROM public.meeting_documents WHERE id = '38300000-0000-4000-8000-000000000001'; + +RESET ROLE; +SELECT pg_temp.assert_true( + (SELECT user_id FROM public.meetings WHERE id = '38200000-0000-4000-8000-000000000001') + = '38000000-0000-4000-8000-000000000002'::uuid, + 'member keeps ownership of the team meeting' +); +SELECT pg_temp.assert_true( + (SELECT title FROM public.meetings WHERE id = '38200000-0000-4000-8000-000000000001') + = 'admin edit ok', + 'admin content edit applied' +); +SELECT pg_temp.assert_true( + (SELECT user_id FROM public.meeting_documents WHERE id = '38300000-0000-4000-8000-000000000001') + = '38000000-0000-4000-8000-000000000002'::uuid, + 'member keeps ownership of the document' +); + +-- ── team deletion still clears team_id through ON DELETE SET NULL ──────── +-- The delete runs as postgres (the atomic team RPCs are SECURITY DEFINER) but +-- keeps the admin's JWT, so the ownership trigger sees a non-owner actor on +-- the cascaded UPDATE and must still let team_id go to NULL. + +SELECT pg_temp.act_as('38000000-0000-4000-8000-000000000001'); +DELETE FROM public.teams WHERE id = '38100000-0000-4000-8000-00000000000a'; +SELECT pg_temp.assert_true( + (SELECT team_id FROM public.meetings WHERE id = '38200000-0000-4000-8000-000000000001') IS NULL, + 'team delete cascades team_id to NULL on a member meeting' +); +SELECT pg_temp.assert_true( + (SELECT team_id FROM public.knowledge_documents WHERE id = '38400000-0000-4000-8000-000000000003') IS NULL, + 'team delete cascades team_id to NULL on a member knowledge document' +); + +ROLLBACK;