Merge pull request #350 from ArnasDon/fix/ghsa-fg5p-2qc3-jmxr-db-authz

fix(security): patch DB-layer authz bypasses (GHSA-fg5p-2qc3-jmxr)

supabase/migrations/031_fix_profiles_update_rls.sql

@@ -0,0 +1,90 @@
+-- ============================================================
+-- 031_fix_profiles_update_rls.sql — lock down privilege columns
+--                                    on profiles (GHSA-fg5p-2qc3-jmxr, C1)
+--
+-- The problem
+--
+--   The `profiles_update` RLS policy from migration 017 gates on
+--   `auth.uid() = user_id` only — it lets a user edit their *own*
+--   row, which is correct for self-service fields (full_name,
+--   avatar). But `account_role` and `account_id` also live on
+--   `profiles`, and they are the source of truth for
+--   `is_account_member()`. RLS constrains *which rows* you may
+--   update, not *which columns*, and no column-level GRANT or
+--   trigger guards them. So the normal `authenticated` browser
+--   client can self-serve a privilege escalation / tenant move:
+--
+--     -- viewer self-promotes to owner of the shared account
+--     UPDATE profiles SET account_role = 'owner' WHERE user_id = auth.uid();
+--     -- attacker relocates into a victim tenant
+--     UPDATE profiles SET account_id = '<victim>' WHERE user_id = auth.uid();
+--
+--   Both pass the WITH CHECK because `user_id` is unchanged.
+--
+-- The fix
+--
+--   A BEFORE UPDATE trigger that rejects any change to
+--   `account_role` / `account_id` when the caller is the
+--   `authenticated` role (the browser). The legitimate writers are
+--   unaffected:
+--     - handle_new_user + the 018/019 member/invitation RPCs are
+--       SECURITY DEFINER owned by `postgres`, so `current_user` is
+--       `postgres`, not `authenticated`.
+--     - the server backend runs as `service_role`.
+--   Self-service edits that leave both columns untouched (the
+--   IS DISTINCT FROM checks are false) also pass through freely.
+--
+--   Membership stays owned by the supervised RPCs (018/019), which
+--   is exactly the model migration 018's header describes.
+--
+-- NOTE FOR MAINTAINER
+--
+--   `current_user` is the reliable discriminator here because every
+--   sanctioned writer runs as postgres (DEFINER) or service_role,
+--   and PostgREST's browser clients run as `authenticated`. If you
+--   ever add a NON-definer RPC or a new role that must write these
+--   columns, extend the guard's role check accordingly. Validate in
+--   your own environment before relying on this (see the checks at
+--   the bottom); this migration was not run against a live database.
+-- ============================================================
+
+CREATE OR REPLACE FUNCTION public.enforce_profile_privilege_columns()
+RETURNS TRIGGER
+LANGUAGE plpgsql
+SET search_path = public
+AS $$
+BEGIN
+  IF (NEW.account_role IS DISTINCT FROM OLD.account_role
+      OR NEW.account_id IS DISTINCT FROM OLD.account_id)
+     AND current_user = 'authenticated'
+  THEN
+    RAISE EXCEPTION
+      'account_role and account_id cannot be changed directly; use the account member/invitation RPCs'
+      USING ERRCODE = 'insufficient_privilege';
+  END IF;
+  RETURN NEW;
+END;
+$$;
+
+ALTER FUNCTION public.enforce_profile_privilege_columns() OWNER TO postgres;
+
+DROP TRIGGER IF EXISTS enforce_profile_privilege_columns ON public.profiles;
+CREATE TRIGGER enforce_profile_privilege_columns
+  BEFORE UPDATE ON public.profiles
+  FOR EACH ROW EXECUTE FUNCTION public.enforce_profile_privilege_columns();
+
+-- ============================================================
+-- Manual validation (run against a live instance — no automated
+-- SQL test harness exists in this repo):
+--
+--   1. As a viewer/member JWT via PostgREST, both of these must
+--      return 42501 (insufficient_privilege):
+--        PATCH /rest/v1/profiles?user_id=eq.<self> { "account_role": "owner" }
+--        PATCH /rest/v1/profiles?user_id=eq.<self> { "account_id": "<other>" }
+--   2. A self-service edit that leaves both columns alone must
+--      still succeed:
+--        PATCH /rest/v1/profiles?user_id=eq.<self> { "full_name": "New Name" }
+--   3. The member/invitation RPCs (set_member_role,
+--      transfer_account_ownership, redeem_invitation) must still
+--      succeed — they run SECURITY DEFINER as postgres.
+-- ============================================================

supabase/migrations/032_fix_ai_knowledge_membership.sql

@@ -0,0 +1,100 @@
+-- ============================================================
+-- 032_fix_ai_knowledge_membership.sql — stop cross-account KB
+--                                        reads (GHSA-fg5p-2qc3-jmxr, H2)
+--
+-- The problem
+--
+--   `match_ai_knowledge_fts` and `match_ai_knowledge_semantic`
+--   (migration 030) are SECURITY DEFINER, so they bypass RLS. They
+--   filter only on the caller-supplied `p_account_id` and never
+--   call `is_account_member()`, yet they are GRANTed to
+--   `authenticated`. The 030 header assumed only the service-role
+--   bot would call them, but any logged-in user can hit PostgREST
+--   directly with a foreign `p_account_id` and read another
+--   tenant's knowledge base:
+--
+--     POST /rest/v1/rpc/match_ai_knowledge_fts
+--       { "p_account_id": "<victim>", "p_query": "price",
+--         "p_match_count": 1000 }
+--
+-- The fix
+--
+--   Recreate both functions as SECURITY INVOKER — the only change
+--   is the security mode; the bodies are byte-for-byte the same.
+--   The existing SELECT policy
+--     ai_knowledge_chunks_select = is_account_member(account_id)
+--   then governs `authenticated` callers, so a foreign
+--   `p_account_id` returns zero rows, while the auto-reply bot
+--   (service_role) still bypasses RLS and works unchanged. This
+--   mirrors the deliberate SECURITY INVOKER choice in
+--   `filter_contacts_by_tags` (migration 025).
+--
+--   The legitimate draft path already passes the caller's *own*
+--   accountId (see src/lib/ai/knowledge.ts → retrieveKnowledge),
+--   so it keeps returning that account's chunks under RLS.
+--
+-- NOTE FOR MAINTAINER
+--
+--   This migration was not run against a live database. Validate
+--   the two checks at the bottom in your own environment. If you
+--   would rather keep these SECURITY DEFINER, the alternative is to
+--   add `AND (auth.role() = 'service_role' OR
+--   is_account_member(p_account_id))` to each WHERE clause instead.
+-- ============================================================
+
+-- Lexical: full-text rank. Body unchanged from migration 030 —
+-- only SECURITY DEFINER → SECURITY INVOKER differs.
+CREATE OR REPLACE FUNCTION public.match_ai_knowledge_fts(
+  p_account_id  uuid,
+  p_query       text,
+  p_match_count integer
+)
+RETURNS TABLE (id uuid, content text, rank real) AS $$
+  SELECT c.id,
+         c.content,
+         ts_rank(c.fts, plainto_tsquery('simple', p_query)) AS rank
+  FROM ai_knowledge_chunks c
+  WHERE c.account_id = p_account_id
+    AND c.fts @@ plainto_tsquery('simple', p_query)
+  ORDER BY rank DESC
+  LIMIT GREATEST(p_match_count, 0);
+$$ LANGUAGE sql STABLE SECURITY INVOKER SET search_path = public;
+
+-- Semantic: cosine distance. Body unchanged from migration 030 —
+-- only SECURITY DEFINER → SECURITY INVOKER differs.
+CREATE OR REPLACE FUNCTION public.match_ai_knowledge_semantic(
+  p_account_id      uuid,
+  p_query_embedding text,
+  p_match_count     integer
+)
+RETURNS TABLE (id uuid, content text, distance real) AS $$
+  SELECT c.id,
+         c.content,
+         (c.embedding <=> p_query_embedding::vector(1536)) AS distance
+  FROM ai_knowledge_chunks c
+  WHERE c.account_id = p_account_id
+    AND c.embedding IS NOT NULL
+  ORDER BY c.embedding <=> p_query_embedding::vector(1536)
+  LIMIT GREATEST(p_match_count, 0);
+$$ LANGUAGE sql STABLE SECURITY INVOKER SET search_path = public;
+
+-- Re-assert the EXECUTE grants (CREATE OR REPLACE preserves them,
+-- but keep them explicit and re-runnable — mirrors migration 030).
+REVOKE ALL ON FUNCTION public.match_ai_knowledge_fts(uuid, text, integer) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION public.match_ai_knowledge_fts(uuid, text, integer) TO authenticated, service_role;
+REVOKE ALL ON FUNCTION public.match_ai_knowledge_semantic(uuid, text, integer) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION public.match_ai_knowledge_semantic(uuid, text, integer) TO authenticated, service_role;
+
+-- ============================================================
+-- Manual validation (run against a live instance — no automated
+-- SQL test harness exists in this repo):
+--
+--   1. As a non-member JWT, calling either RPC with a foreign
+--      p_account_id must return zero rows:
+--        POST /rest/v1/rpc/match_ai_knowledge_fts
+--          { "p_account_id": "<other-account>", "p_query": "price",
+--            "p_match_count": 1000 }              -> []
+--   2. The draft flow (own accountId, authenticated) and the
+--      auto-reply bot (service_role) must still return the
+--      account's own chunks.
+-- ============================================================