From 63bc0ebe69cc3cc92f4174231efbb5166fa8bb9c Mon Sep 17 00:00:00 2001 From: Yun Chan Date: Mon, 28 Sep 2026 02:16:21 +0900 Subject: [PATCH] fix(db): lock increment_daily_usage and admin analytics RPCs to service_role --- .../20260929000001_function_acl_hardening.sql | 200 +++++++++++++ .../tests/function-acl.integration.sql | 283 ++++++++++++++++++ 2 files changed, 483 insertions(+) create mode 100644 server/supabase/migrations/20260929000001_function_acl_hardening.sql create mode 100644 server/supabase/tests/function-acl.integration.sql diff --git a/server/supabase/migrations/20260929000001_function_acl_hardening.sql b/server/supabase/migrations/20260929000001_function_acl_hardening.sql new file mode 100644 index 0000000..e419cd0 --- /dev/null +++ b/server/supabase/migrations/20260929000001_function_acl_hardening.sql @@ -0,0 +1,200 @@ +-- ============================================================================ +-- 20260929000001_function_acl_hardening.sql +-- +-- Function execution ACL hardening. +-- +-- Background +-- Supabase's default privileges give every function that postgres creates in +-- schema public a *direct* EXECUTE grant to anon, authenticated and +-- service_role. `REVOKE ... FROM PUBLIC` therefore does not lock anon out. +-- Three early migrations only revoked PUBLIC (and sometimes authenticated): +-- * 20260409000003 increment_daily_usage -> anon could add any (negative) +-- amount to any user's daily_usage, wiping or exhausting quota. +-- * 20260413000004 admin_usage_by_feature / admin_top_users / admin_dau -> +-- SECURITY DEFINER bodies with no role predicate, callable by anon and +-- every signed-in user; admin_top_users leaks user ids and names. +-- +-- Policy (single place; enforced by tests/function-acl.integration.sql) +-- * No SECURITY DEFINER function in schema public is executable by anon. +-- * SECURITY DEFINER trigger functions are executable by nobody but the +-- owner and service_role (triggers do not check EXECUTE when they fire). +-- * authenticated may execute a SECURITY DEFINER function only if it is on +-- the reviewed allowlist in tests/function-acl.integration.sql. +-- * Every SECURITY DEFINER function pins search_path. +-- New migrations keep writing their own +-- REVOKE ALL ON FUNCTION ... FROM PUBLIC, anon[, authenticated]; +-- GRANT EXECUTE ON FUNCTION ... TO ; +-- and add authenticated RPCs to the test allowlist. An omission is caught by +-- the local integration test (it is not run by CI yet). +-- +-- Why default privileges are NOT changed here +-- A deny-by-default `ALTER DEFAULT PRIVILEGES` only works for anon if it is +-- the schema-less (global) form, because anon inherits the built-in PUBLIC +-- EXECUTE default. The global form also strips PUBLIC EXECUTE from pg_temp +-- helpers created by postgres, which breaks every integration test that +-- calls pg_temp.assert_true()/act_as() after SET ROLE authenticated, and it +-- would silently change the ACL of functions created by later migrations +-- that are being written against the current defaults. The schema-scoped +-- form alone is a no-op for anon. The sweep below plus the catalog test give +-- the same guarantee without those side effects. +-- ============================================================================ + +-- ---------------------------------------------------------------------------- +-- 1. Sweep: apply the policy to every existing SECURITY DEFINER function in +-- schema public. authenticated/service_role access is preserved exactly +-- (except for trigger functions, which cannot be called directly anyway); +-- only anon/PUBLIC access is removed. +-- ---------------------------------------------------------------------------- +DO $$ +DECLARE + fn record; + target text; + keep_authenticated boolean; + keep_service_role boolean; +BEGIN + FOR fn IN + SELECT p.oid, + n.nspname, + p.proname, + pg_get_function_identity_arguments(p.oid) AS identity_args, + p.prorettype = 'trigger'::regtype AS is_trigger + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' + AND p.prosecdef + LOOP + target := format('%I.%I(%s)', fn.nspname, fn.proname, fn.identity_args); + keep_authenticated := NOT fn.is_trigger + AND has_function_privilege('authenticated', fn.oid, 'EXECUTE'); + keep_service_role := has_function_privilege('service_role', fn.oid, 'EXECUTE'); + + EXECUTE format('REVOKE ALL ON ROUTINE %s FROM PUBLIC, anon', target); + IF fn.is_trigger THEN + EXECUTE format('REVOKE ALL ON ROUTINE %s FROM authenticated', target); + END IF; + + -- Re-grant access that previously flowed only through PUBLIC. + IF keep_authenticated THEN + EXECUTE format('GRANT EXECUTE ON ROUTINE %s TO authenticated', target); + END IF; + IF keep_service_role THEN + EXECUTE format('GRANT EXECUTE ON ROUTINE %s TO service_role', target); + END IF; + END LOOP; +END; +$$; + +-- ---------------------------------------------------------------------------- +-- 2. increment_daily_usage: service_role only, and the counter can no longer +-- go below zero. Negative amounts stay allowed for the service-role refund +-- path (functions/_shared/embedding-quota.ts releases reserved units with a +-- negative amount), but the stored total is clamped at 0. +-- ---------------------------------------------------------------------------- +CREATE OR REPLACE FUNCTION public.increment_daily_usage( + p_user_id uuid, + p_feature text, + p_amount integer DEFAULT 1 +) +RETURNS integer +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path = '' +AS $$ +DECLARE + v_new_count integer; +BEGIN + IF p_user_id IS NULL OR p_feature IS NULL OR p_amount IS NULL THEN + RAISE EXCEPTION 'increment_daily_usage: arguments must not be null' + USING ERRCODE = '22004'; + END IF; + + INSERT INTO public.daily_usage (user_id, date, feature, count) + VALUES (p_user_id, CURRENT_DATE, p_feature, greatest(p_amount, 0)) + ON CONFLICT (user_id, date, feature) + DO UPDATE SET count = greatest(public.daily_usage.count + p_amount, 0) + RETURNING count INTO v_new_count; + + RETURN v_new_count; +END; +$$; + +REVOKE ALL ON FUNCTION public.increment_daily_usage(uuid, text, integer) + FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.increment_daily_usage(uuid, text, integer) + TO service_role; + +-- ---------------------------------------------------------------------------- +-- 3. daily_usage.count is a usage counter; negative totals are never valid. +-- Legitimate decrements (quota reservation release) already clamp with +-- greatest(count - 1, 0). Any negative row can only come from the abuse +-- above, so it is normalised to 0 before the constraint is validated. +-- ---------------------------------------------------------------------------- +UPDATE public.daily_usage + SET count = 0 + WHERE count < 0; + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 + FROM pg_constraint + WHERE conrelid = 'public.daily_usage'::regclass + AND conname = 'daily_usage_count_nonnegative' + ) THEN + ALTER TABLE public.daily_usage + ADD CONSTRAINT daily_usage_count_nonnegative CHECK (count >= 0); + END IF; +END; +$$; + +-- ---------------------------------------------------------------------------- +-- 4. Platform analytics RPCs: service_role only. Their bodies have no role +-- predicate and run as definer (RLS bypassed), so no end-user role may call +-- them. The admin console reads Supabase with the service-role key, like +-- the other admin_*_v1 RPCs. +-- ---------------------------------------------------------------------------- +REVOKE ALL ON FUNCTION public.admin_usage_by_feature(date, date) + FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.admin_usage_by_feature(date, date) + TO service_role; + +REVOKE ALL ON FUNCTION public.admin_top_users(date, date, integer) + FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.admin_top_users(date, date, integer) + TO service_role; + +REVOKE ALL ON FUNCTION public.admin_dau(date, date) + FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.admin_dau(date, date) + TO service_role; + +COMMENT ON FUNCTION public.increment_daily_usage(uuid, text, integer) IS + 'service_role only. Adds p_amount to today''s usage counter, clamped at 0.'; +COMMENT ON FUNCTION public.admin_usage_by_feature(date, date) IS + 'service_role only. Platform analytics; no caller role predicate in body.'; +COMMENT ON FUNCTION public.admin_top_users(date, date, integer) IS + 'service_role only. Returns user ids and names; no caller role predicate in body.'; +COMMENT ON FUNCTION public.admin_dau(date, date) IS + 'service_role only. Platform analytics; no caller role predicate in body.'; + +-- ---------------------------------------------------------------------------- +-- 5. Self-check: fail the migration if any public SECURITY DEFINER function is +-- still executable by anon. +-- ---------------------------------------------------------------------------- +DO $$ +DECLARE + offenders text; +BEGIN + SELECT string_agg(p.oid::regprocedure::text, ', ' ORDER BY p.oid::regprocedure::text) + INTO offenders + FROM pg_proc p + WHERE p.pronamespace = 'public'::regnamespace + AND p.prosecdef + AND has_function_privilege('anon', p.oid, 'EXECUTE'); + + IF offenders IS NOT NULL THEN + RAISE EXCEPTION 'function_acl_hardening: anon can still execute SECURITY DEFINER functions: %', + offenders; + END IF; +END; +$$; diff --git a/server/supabase/tests/function-acl.integration.sql b/server/supabase/tests/function-acl.integration.sql new file mode 100644 index 0000000..b341c37 --- /dev/null +++ b/server/supabase/tests/function-acl.integration.sql @@ -0,0 +1,283 @@ +\set ON_ERROR_STOP on + +-- Function execution ACL policy (see migrations/20260929000001_function_acl_hardening.sql). +-- +-- * Every SECURITY DEFINER function in schema public pins search_path. +-- * anon cannot execute any SECURITY DEFINER function in schema public. +-- * SECURITY DEFINER trigger functions are not executable by authenticated. +-- * authenticated can execute a SECURITY DEFINER function only if it is on +-- the reviewed allowlist below. Adding an authenticated RPC means adding +-- it here on purpose; a forgotten REVOKE fails this test. +-- * increment_daily_usage and the admin analytics RPCs are service_role only, +-- and daily_usage.count can never go negative. +-- +-- Local only: psql against the local Supabase stack. Runs in a transaction and +-- rolls back. + +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; +$$; + +-- Reviewed allowlist: SECURITY DEFINER functions that signed-in users may call +-- directly. Each one must derive the caller from auth.uid() (or enforce its own +-- role check) inside the body. +CREATE TEMP TABLE authenticated_definer_allowlist (proname text PRIMARY KEY) ON COMMIT DROP; +INSERT INTO authenticated_definer_allowlist (proname) VALUES + ('accept_team_invite'), + ('admin_act_on_content_report_v1'), + ('admin_list_content_reports_v1'), + ('bootstrap_custom_instructions'), + ('bootstrap_user_templates_v1'), + ('cancel_team_invite'), + ('create_team'), + ('create_team_activity'), + ('create_team_invite'), + ('create_user_template_v1'), + ('delete_user_template_v1'), + ('export_account_portability'), + ('list_team_invites'), + ('list_team_members'), + ('mobile_add_memo_tag_v1'), + ('mobile_begin_meeting_processing'), + ('mobile_begin_meeting_recording'), + ('mobile_cancel_meeting_recording'), + ('mobile_complete_meeting_processing'), + ('mobile_create_meeting_workspace_v2'), + ('mobile_fail_meeting_recording'), + ('mobile_list_memo_tags_v1'), + ('mobile_mark_meeting_processing_failure'), + ('mobile_queue_meeting_recording'), + ('mobile_remove_memo_tag_v1'), + ('mobile_rename_memo_tag_v1'), + ('mobile_search_memos_v1'), + ('purge_revoked_device'), + ('register_push_registration'), + ('remove_team_member'), + ('reorder_custom_instruction'), + ('reserve_push_dispatch'), + ('resolve_team_invite_recipient'), + ('restore_account_portability'), + ('revoke_device'), + ('select_user_template_v1'), + ('set_active_custom_instruction'), + ('sync_delete_user_template_v1'), + ('sync_set_builtin_instruction_prompt_v1'), + ('sync_upsert_user_template_v1'), + ('unregister_current_device'), + ('unregister_push_registration'), + ('update_team_member_role'), + ('update_user_template_v1'), + ('user_admin_team_ids'), + ('user_team_ids'); + +-- Offender queries, shared by the detector self-check and the real checks. +CREATE OR REPLACE FUNCTION pg_temp.definer_functions_executable_by(p_role text) +RETURNS text +LANGUAGE sql +STABLE +AS $$ + SELECT string_agg(p.oid::regprocedure::text, ', ' ORDER BY p.oid::regprocedure::text) + FROM pg_proc p + WHERE p.pronamespace = 'public'::regnamespace + AND p.prosecdef + AND has_function_privilege(p_role, p.oid, 'EXECUTE'); +$$; + +-- --------------------------------------------------------------------------- +-- 0. Detector self-check: a new SECURITY DEFINER function without an explicit +-- REVOKE is callable by anon under the platform defaults, and the offender +-- query must report it. This proves a forgotten REVOKE is caught. +-- --------------------------------------------------------------------------- +CREATE FUNCTION public.zz_function_acl_probe_v1() +RETURNS integer +LANGUAGE sql +SECURITY DEFINER +SET search_path = '' +AS 'SELECT 1'; + +SELECT pg_temp.assert_true( + position('zz_function_acl_probe_v1()' IN coalesce(pg_temp.definer_functions_executable_by('anon'), '')) > 0, + 'offender query must detect a SECURITY DEFINER function missing its REVOKE' +); + +DROP FUNCTION public.zz_function_acl_probe_v1(); + +-- --------------------------------------------------------------------------- +-- 1. Catalog policy. +-- --------------------------------------------------------------------------- +DO $$ +DECLARE + offenders text; +BEGIN + SELECT string_agg(p.oid::regprocedure::text, ', ' ORDER BY p.oid::regprocedure::text) + INTO offenders + FROM pg_proc p + WHERE p.pronamespace = 'public'::regnamespace + AND p.prosecdef + AND NOT EXISTS ( + SELECT 1 FROM unnest(coalesce(p.proconfig, ARRAY[]::text[])) AS cfg(setting) + WHERE cfg.setting LIKE 'search_path=%' + ); + PERFORM pg_temp.assert_true( + offenders IS NULL, + 'SECURITY DEFINER functions without a pinned search_path: ' || coalesce(offenders, '') + ); +END; +$$; + +DO $$ +DECLARE + offenders text := pg_temp.definer_functions_executable_by('anon'); +BEGIN + PERFORM pg_temp.assert_true( + offenders IS NULL, + 'anon can execute SECURITY DEFINER functions: ' || coalesce(offenders, '') + ); +END; +$$; + +DO $$ +DECLARE + offenders text; +BEGIN + SELECT string_agg(p.oid::regprocedure::text, ', ' ORDER BY p.oid::regprocedure::text) + INTO offenders + FROM pg_proc p + WHERE p.pronamespace = 'public'::regnamespace + AND p.prosecdef + AND p.prorettype = 'trigger'::regtype + AND has_function_privilege('authenticated', p.oid, 'EXECUTE'); + PERFORM pg_temp.assert_true( + offenders IS NULL, + 'authenticated can execute SECURITY DEFINER trigger functions: ' || coalesce(offenders, '') + ); +END; +$$; + +DO $$ +DECLARE + offenders text; +BEGIN + SELECT string_agg(p.oid::regprocedure::text, ', ' ORDER BY p.oid::regprocedure::text) + INTO offenders + FROM pg_proc p + WHERE p.pronamespace = 'public'::regnamespace + AND p.prosecdef + AND has_function_privilege('authenticated', p.oid, 'EXECUTE') + AND NOT EXISTS ( + SELECT 1 FROM authenticated_definer_allowlist a WHERE a.proname = p.proname + ); + PERFORM pg_temp.assert_true( + offenders IS NULL, + 'authenticated can execute SECURITY DEFINER functions outside the reviewed allowlist: ' + || coalesce(offenders, '') + ); +END; +$$; + +-- --------------------------------------------------------------------------- +-- 2. Service-role-only functions. +-- --------------------------------------------------------------------------- +DO $$ +DECLARE + fn text; +BEGIN + FOREACH fn IN ARRAY ARRAY[ + 'public.increment_daily_usage(uuid,text,integer)', + 'public.admin_usage_by_feature(date,date)', + 'public.admin_top_users(date,date,integer)', + 'public.admin_dau(date,date)' + ] LOOP + PERFORM pg_temp.assert_true( + NOT has_function_privilege('anon', fn, 'EXECUTE'), + 'anon must not execute ' || fn + ); + PERFORM pg_temp.assert_true( + NOT has_function_privilege('authenticated', fn, 'EXECUTE'), + 'authenticated must not execute ' || fn + ); + PERFORM pg_temp.assert_true( + has_function_privilege('service_role', fn, 'EXECUTE'), + 'service_role must execute ' || fn + ); + END LOOP; +END; +$$; + +-- --------------------------------------------------------------------------- +-- 3. daily_usage counter integrity. +-- --------------------------------------------------------------------------- +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 ( + 'a1000000-0000-4000-8000-000000000001', 'authenticated', 'authenticated', + 'function-acl-one@example.invalid', crypt('fixture-password', gen_salt('bf')), now(), + '{"provider":"email","providers":["email"]}'::jsonb, '{}'::jsonb, now(), now() +); + +SELECT pg_temp.assert_true( + EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conrelid = 'public.daily_usage'::regclass + AND conname = 'daily_usage_count_nonnegative' + AND convalidated + ), + 'daily_usage.count has a validated non-negative CHECK' +); + +SELECT pg_temp.assert_true( + public.increment_daily_usage('a1000000-0000-4000-8000-000000000001', 'acl_probe', 5) = 5, + 'service path increments the counter' +); +SELECT pg_temp.assert_true( + public.increment_daily_usage('a1000000-0000-4000-8000-000000000001', 'acl_probe', -2) = 3, + 'service path refunds with a negative amount' +); +SELECT pg_temp.assert_true( + public.increment_daily_usage('a1000000-0000-4000-8000-000000000001', 'acl_probe', -1000000) = 0, + 'a refund larger than the counter clamps at zero' +); +SELECT pg_temp.assert_true( + public.increment_daily_usage('a1000000-0000-4000-8000-000000000001', 'acl_probe_new', -7) = 0, + 'a negative first write stores zero' +); + +DO $$ +BEGIN + PERFORM public.increment_daily_usage('a1000000-0000-4000-8000-000000000001', 'acl_probe', NULL); + RAISE EXCEPTION 'assertion_failed: NULL amount was accepted'; +EXCEPTION + WHEN null_value_not_allowed THEN + NULL; +END; +$$; + +DO $$ +BEGIN + UPDATE public.daily_usage + SET count = -1 + WHERE user_id = 'a1000000-0000-4000-8000-000000000001' + AND feature = 'acl_probe'; + RAISE EXCEPTION 'assertion_failed: negative daily_usage.count was stored'; +EXCEPTION + WHEN check_violation THEN + NULL; +END; +$$; + +-- Call-time rejection for anon/authenticated is what PostgREST enforces via +-- the EXECUTE privilege asserted in section 2. It is intentionally not probed +-- with SET ROLE + a PL/pgSQL EXCEPTION handler here: on the local Supabase +-- image that combination segfaults the backend and restarts the database. + +ROLLBACK;