diff --git a/src/components/sharing/DiagramSharingModal.tsx b/src/components/sharing/DiagramSharingModal.tsx index 0af60aa..fca58be 100644 --- a/src/components/sharing/DiagramSharingModal.tsx +++ b/src/components/sharing/DiagramSharingModal.tsx @@ -1,6 +1,6 @@ import { useEffect, useState } from 'react' import type { CollaboratorPermission } from '../../data/DiagramCollaboratorRepository' -import { UnknownUsernameError } from '../../data/DiagramCollaboratorRepository' +import { SelfCollaboratorError, UnknownUsernameError } from '../../data/DiagramCollaboratorRepository' import { useDiagramCollaboratorStore } from '../../state/diagramCollaboratorStore' import { useProjectStore } from '../../state/projectStore' import Modal from '../common/Modal' @@ -36,7 +36,11 @@ export default function DiagramSharingModal({ onClose }: { onClose: () => void } await add(diagramId, trimmed, permission) setUsername('') } catch (err) { - setError(err instanceof UnknownUsernameError ? err.message : 'Could not add that person. Try again.') + setError( + err instanceof UnknownUsernameError || err instanceof SelfCollaboratorError + ? err.message + : 'Could not add that person. Try again.', + ) } finally { setBusy(false) } diff --git a/src/data/DiagramCollaboratorRepository.ts b/src/data/DiagramCollaboratorRepository.ts index 9df2267..18f470b 100644 --- a/src/data/DiagramCollaboratorRepository.ts +++ b/src/data/DiagramCollaboratorRepository.ts @@ -11,6 +11,11 @@ export interface DiagramCollaborator { * not a system failure. */ export class UnknownUsernameError extends Error {} +/** Thrown by `add` when the resolved username is the diagram owner's own + * account — RLS blocks this at the database level too (the real + * enforcement), this is just a friendlier message than the raw 42501. */ +export class SelfCollaboratorError extends Error {} + /** Storage abstraction for a diagram's collaborator list, per * organized-ideas.md §8: the owner shares with specific people by * username, choosing view or edit access per person. RLS restricts diff --git a/src/data/SupabaseDiagramCollaboratorRepository.ts b/src/data/SupabaseDiagramCollaboratorRepository.ts index 1925fe7..9d33c55 100644 --- a/src/data/SupabaseDiagramCollaboratorRepository.ts +++ b/src/data/SupabaseDiagramCollaboratorRepository.ts @@ -3,7 +3,7 @@ import type { DiagramCollaborator, DiagramCollaboratorRepository, } from './DiagramCollaboratorRepository' -import { UnknownUsernameError } from './DiagramCollaboratorRepository' +import { SelfCollaboratorError, UnknownUsernameError } from './DiagramCollaboratorRepository' import { supabase } from './supabaseClient' interface CollaboratorRow { @@ -43,6 +43,12 @@ export class SupabaseDiagramCollaboratorRepository implements DiagramCollaborato if (!userId) { throw new UnknownUsernameError(`No account found for username "${username}".`) } + const { + data: { user: currentUser }, + } = await supabase.auth.getUser() + if (currentUser && userId === currentUser.id) { + throw new SelfCollaboratorError("You already have full access to your own diagram — you can't share it with yourself.") + } const { error } = await supabase.from('diagram_collaborators').insert({ diagram_id: diagramId, user_id: userId, permission }) if (error) { console.error('Failed to add collaborator in Supabase', error) diff --git a/supabase/migrations/20260914000000_prevent_self_collaborator.sql b/supabase/migrations/20260914000000_prevent_self_collaborator.sql new file mode 100644 index 0000000..5c04544 --- /dev/null +++ b/supabase/migrations/20260914000000_prevent_self_collaborator.sql @@ -0,0 +1,29 @@ +-- Prevents a diagram's owner from ending up as their own collaborator row +-- (found via manual testing: the UI let you "share" a diagram with +-- yourself). Ownership already implies full access, so a self-collaborator +-- row is never meaningful — enforced here, not just in the client, since +-- the client-side check alone wouldn't stop a Super Admin's "add on behalf +-- of the owner" path or any other direct API access from creating one. + +drop policy "diagram_collaborators_insert" on public.diagram_collaborators; + +create policy "diagram_collaborators_insert" on public.diagram_collaborators for insert + with check ( + (public.is_super_admin() or public.diagram_owner_id(diagram_id) = auth.uid()) + and user_id != public.diagram_owner_id(diagram_id) + ); + +-- The existing update policy had no WITH CHECK at all (only USING) — added +-- here too, defensively, in case user_id (part of the primary key) is ever +-- changed via UPDATE rather than delete+insert. +drop policy "diagram_collaborators_update" on public.diagram_collaborators; + +create policy "diagram_collaborators_update" on public.diagram_collaborators for update + using ( + public.is_super_admin() + or public.diagram_owner_id(diagram_id) = auth.uid() + ) + with check ( + (public.is_super_admin() or public.diagram_owner_id(diagram_id) = auth.uid()) + and user_id != public.diagram_owner_id(diagram_id) + ); diff --git a/supabase/tests/rls.sql b/supabase/tests/rls.sql index d80b804..9ed0466 100644 --- a/supabase/tests/rls.sql +++ b/supabase/tests/rls.sql @@ -25,7 +25,7 @@ begin; create extension if not exists pgtap with schema extensions; -select plan(52); +select plan(53); -- ---------------------------------------------------------------------- -- Fixtures (as postgres — RLS does not apply) @@ -138,6 +138,13 @@ select throws_ok( select set_config('request.jwt.claim.sub', '11111111-1111-1111-1111-111111111111', true); +select throws_ok( + $$ insert into public.diagram_collaborators (diagram_id, user_id, permission) + values ('b0000000-0000-0000-0000-000000000001', '11111111-1111-1111-1111-111111111111', 'edit') $$, + '42501'::char(5), null, + 'alice (owner) cannot add herself as a collaborator on her own diagram' +); + select lives_ok( $$ insert into public.diagram_collaborators (diagram_id, user_id, permission) values ('b0000000-0000-0000-0000-000000000001', '22222222-2222-2222-2222-222222222222', 'view') $$,