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

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 <noreply@anthropic.com>
This commit is contained in:
Arnas Donauskas 2026-07-08 09:10:07 +03:00
parent f90563b35c
commit 2b32308703
2 changed files with 190 additions and 0 deletions

View File

@ -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.
-- ============================================================

View File

@ -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.
-- ============================================================