From 2b3230870375322f9cd3044ed6b65681402d19f4 Mon Sep 17 00:00:00 2001 From: Arnas Donauskas Date: Wed, 8 Jul 2026 09:10:07 +0300 Subject: [PATCH] fix(security): patch DB-layer authz bypasses (GHSA-fg5p-2qc3-jmxr) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1 (Critical): the profiles_update RLS policy gates only on user_id, leaving account_role/account_id — the source of truth for is_account_member() — freely writable by the row's own user. A browser-client UPDATE could self-promote a viewer to owner or move into another tenant. Add a BEFORE UPDATE trigger that rejects changes to those columns when current_user = 'authenticated'; the SECURITY DEFINER member/invitation RPCs (postgres) and the service_role backend are unaffected. H2 (High): match_ai_knowledge_fts/_semantic were SECURITY DEFINER, filtered only on the caller-supplied p_account_id with no membership check, and were granted to authenticated — letting any logged-in user read another account's knowledge base via a foreign p_account_id. Recreate both as SECURITY INVOKER so the existing ai_knowledge_chunks_select RLS policy applies; the service_role auto-reply bot still bypasses RLS. Co-Authored-By: Claude Opus 4.8 --- .../031_fix_profiles_update_rls.sql | 90 ++++++++++++++++ .../032_fix_ai_knowledge_membership.sql | 100 ++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 supabase/migrations/031_fix_profiles_update_rls.sql create mode 100644 supabase/migrations/032_fix_ai_knowledge_membership.sql diff --git a/supabase/migrations/031_fix_profiles_update_rls.sql b/supabase/migrations/031_fix_profiles_update_rls.sql new file mode 100644 index 0000000..131ab94 --- /dev/null +++ b/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 = '' 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. { "account_role": "owner" } +-- PATCH /rest/v1/profiles?user_id=eq. { "account_id": "" } +-- 2. A self-service edit that leaves both columns alone must +-- still succeed: +-- PATCH /rest/v1/profiles?user_id=eq. { "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. +-- ============================================================ diff --git a/supabase/migrations/032_fix_ai_knowledge_membership.sql b/supabase/migrations/032_fix_ai_knowledge_membership.sql new file mode 100644 index 0000000..f23c185 --- /dev/null +++ b/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": "", "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": "", "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. +-- ============================================================