From c57506ba71535fd650e9b6f8779e0c3283bfeeb5 Mon Sep 17 00:00:00 2001 From: Adam Date: Fri, 23 Oct 2026 03:01:40 +0000 Subject: [PATCH 1/3] Harden Supabase grants, de-duplicate share views and index foreign keys (T15) SC-04: get_shared_item counts each viewer once per share per UTC day, keyed by the signed-in user or the forwarded client IP, hashed with the share and the day. SC-08: revoke client writes on user_roles, subscriptions, user_promos and admin_grants, and all client access on promo_codes and daily_usage_counters, one migration per table. Every writer is the service role or a SECURITY DEFINER function. DB-05: index 25 unindexed foreign keys. Destructive changes (legacy policy drops, the staging-only function drop, duplicate indexes, a primary key) are proposals in supabase/proposed/ and never run. scripts/agent-stack.sh strips every auth email template block from the overlay config, since the confirmation template broke `start`. Co-Authored-By: Claude Opus 5.5 --- scripts/agent-stack.sh | 4 +- supabase/README.md | 2 + .../20261023060000_share_view_dedup.sql | 78 ++++++++++++++++ ...060100_revoke_client_writes_user_roles.sql | 6 ++ ...200_revoke_client_writes_subscriptions.sql | 5 ++ ...60300_revoke_client_writes_user_promos.sql | 5 ++ ...0400_revoke_client_writes_admin_grants.sql | 4 + ...60500_revoke_client_access_promo_codes.sql | 4 + ...oke_client_access_daily_usage_counters.sql | 4 + .../20261023060700_index_foreign_keys.sql | 37 ++++++++ .../20261023060000_drop_legacy_policies.sql | 44 ++++++++++ ...20261023060100_drop_staging_auto_admin.sql | 13 +++ .../20261023060200_drop_duplicate_indexes.sql | 49 +++++++++++ ...0300_promo_redeem_failures_primary_key.sql | 12 +++ supabase/proposed/README.md | 5 ++ supabase/tests/admin_tables_rls_test.sql | 4 +- .../server_written_tables_grants_test.sql | 76 ++++++++++++++++ supabase/tests/share_views_test.sql | 88 +++++++++++++++++++ supabase/tests/user_roles_rls_test.sql | 4 +- 19 files changed, 440 insertions(+), 4 deletions(-) create mode 100644 supabase/migrations/20261023060000_share_view_dedup.sql create mode 100644 supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql create mode 100644 supabase/migrations/20261023060200_revoke_client_writes_subscriptions.sql create mode 100644 supabase/migrations/20261023060300_revoke_client_writes_user_promos.sql create mode 100644 supabase/migrations/20261023060400_revoke_client_writes_admin_grants.sql create mode 100644 supabase/migrations/20261023060500_revoke_client_access_promo_codes.sql create mode 100644 supabase/migrations/20261023060600_revoke_client_access_daily_usage_counters.sql create mode 100644 supabase/migrations/20261023060700_index_foreign_keys.sql create mode 100644 supabase/proposed/20261023060000_drop_legacy_policies.sql create mode 100644 supabase/proposed/20261023060100_drop_staging_auto_admin.sql create mode 100644 supabase/proposed/20261023060200_drop_duplicate_indexes.sql create mode 100644 supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql create mode 100644 supabase/proposed/README.md create mode 100644 supabase/tests/server_written_tables_grants_test.sql create mode 100644 supabase/tests/share_views_test.sql diff --git a/scripts/agent-stack.sh b/scripts/agent-stack.sh index 6a0a77e32..a63b68bf0 100755 --- a/scripts/agent-stack.sh +++ b/scripts/agent-stack.sh @@ -10,7 +10,7 @@ # project it starts is an overlay directory: symlinks to this checkout's # migrations, tests, and templates, a copy of its functions (the stack does # not find functions behind a symlink), and a copy of config.toml without the -# magic-link template block. Run `reset` after changing migrations and +# auth email template blocks. Run `reset` after changing migrations and # `serve` again after changing functions. # # usage: scripts/agent-stack.sh [--name NAME] start|env|reset|test|advisors|psql|serve|stop|destroy @@ -57,7 +57,7 @@ prepare_overlay() { if [ -L "$dir/supabase/functions" ]; then rm "$dir/supabase/functions"; fi mkdir -p "$dir/supabase/functions" rsync -a --delete --exclude .env "$root/supabase/functions/" "$dir/supabase/functions/" - awk '/^\[/ { skip = ($0 == "[auth.email.template.magic_link]") } !skip' \ + awk '/^\[/ { skip = ($0 ~ /^\[auth\.email\.template\./) } !skip' \ "$root/supabase/config.toml" > "$dir/supabase/config.toml" } diff --git a/supabase/README.md b/supabase/README.md index a15ce3384..80fa5b5a0 100644 --- a/supabase/README.md +++ b/supabase/README.md @@ -22,6 +22,7 @@ supabase/functions/ Deno Edge Functions supabase/functions/_shared shared auth, validation, prompts, research, and cost logic supabase/migrations/ ordered database history supabase/rollbacks/ down scripts for data migrations; never applied by db push +supabase/proposed/ destructive SQL awaiting authorization; never applied by db push supabase/tests/ pgTAP database and policy tests ``` @@ -108,6 +109,7 @@ Do not use wildcard origins to hide a failed browser test. Before protected stag Migration filenames define execution order. Never edit an applied migration to change production history; add a new forward migration. When creating a table, sequence or function in `public`, grant only the Data API roles and operations it needs in the same migration, alongside RLS and its policies. Since `20261016120000_security_preship_hardening.sql`, tables and sequences that `postgres` creates in `public` grant `anon` and `authenticated` nothing by default, so a missing grant shows up as `permission denied`, not as an open table. Functions still get Postgres's global `PUBLIC EXECUTE`, so every new function needs its own `REVOKE ... FROM PUBLIC`. `supabase/tests/client_grants_test.sql` checks both. +Anything destructive (dropping a policy, function, index or column, deleting rows, or a constraint existing rows might break) goes in `supabase/proposed/` instead, with a header saying what it removes and what covers it; see [proposed/README.md](proposed/README.md). Local gate: diff --git a/supabase/migrations/20261023060000_share_view_dedup.sql b/supabase/migrations/20261023060000_share_view_dedup.sql new file mode 100644 index 000000000..d9b61d721 --- /dev/null +++ b/supabase/migrations/20261023060000_share_view_dedup.sql @@ -0,0 +1,78 @@ +-- SC-04: get_shared_item counted every call, so reloading a share link +-- inflated its view count. Count each viewer at most once per share per UTC +-- day instead. The share page and the function's signature and result stay +-- the same. +-- +-- A viewer is the signed-in user, otherwise the client IP the API gateway +-- forwards. The stored key is a SHA-256 of share, day and viewer, so rows +-- cannot be matched across shares or days, and rows from earlier days are +-- deleted as soon as a new view is counted. This stops reload inflation; it +-- is not abuse protection: a client that varies its IP still counts again. + +CREATE TABLE public.share_views ( + share_id uuid NOT NULL REFERENCES public.shared_items(id) ON DELETE CASCADE, + viewed_on date NOT NULL, + viewer_key bytea NOT NULL, + PRIMARY KEY (share_id, viewed_on, viewer_key) +); +CREATE INDEX share_views_viewed_on_idx ON public.share_views (viewed_on); +ALTER TABLE public.share_views ENABLE ROW LEVEL SECURITY; +-- Only get_shared_item (SECURITY DEFINER) touches this table. +REVOKE ALL ON TABLE public.share_views FROM PUBLIC, anon, authenticated; + +CREATE OR REPLACE FUNCTION public.get_shared_item(p_share_code text) +RETURNS TABLE (id uuid, share_code text, item_type text, item_data jsonb, title text, created_at timestamptz, view_count integer) +LANGUAGE plpgsql SECURITY DEFINER SET search_path = '' AS $$ +#variable_conflict use_column +DECLARE + v_item public.shared_items%ROWTYPE; + v_today date := (now() AT TIME ZONE 'UTC')::date; + v_headers jsonb; + v_viewer text; +BEGIN + SELECT * INTO v_item FROM public.shared_items AS s + WHERE s.share_code = p_share_code + AND (s.expires_at IS NULL OR s.expires_at > now()); + IF NOT FOUND THEN RETURN; END IF; + + v_viewer := 'user:' || (SELECT auth.uid())::text; + IF v_viewer IS NULL THEN + -- PostgREST sets request.headers; it is unset for direct SQL callers, + -- which then share one 'unknown' viewer per share per day. + BEGIN + v_headers := nullif(current_setting('request.headers', true), '')::jsonb; + EXCEPTION WHEN others THEN + v_headers := NULL; + END; + -- cf-connecting-ip is set by the edge and cannot be supplied by the + -- client; the first x-forwarded-for entry is the fallback. + v_viewer := 'ip:' || coalesce( + nullif(btrim(v_headers ->> 'cf-connecting-ip'), ''), + nullif(btrim(split_part(v_headers ->> 'x-forwarded-for', ',', 1)), ''), + nullif(btrim(v_headers ->> 'x-real-ip'), ''), + 'unknown' + ); + END IF; + + INSERT INTO public.share_views (share_id, viewed_on, viewer_key) + VALUES ( + v_item.id, + v_today, + sha256(convert_to(v_item.id::text || ':' || v_today::text || ':' || v_viewer, 'UTF8')) + ) + ON CONFLICT DO NOTHING; + + IF FOUND THEN + UPDATE public.shared_items AS s + SET view_count = s.view_count + 1 + WHERE s.id = v_item.id + RETURNING s.view_count INTO v_item.view_count; + DELETE FROM public.share_views AS v WHERE v.viewed_on < v_today; + END IF; + + RETURN QUERY SELECT v_item.id, v_item.share_code, v_item.item_type, + v_item.item_data, v_item.title, v_item.created_at, v_item.view_count; +END; +$$; +REVOKE ALL ON FUNCTION public.get_shared_item(text) FROM PUBLIC; +GRANT EXECUTE ON FUNCTION public.get_shared_item(text) TO anon, authenticated; diff --git a/supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql b/supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql new file mode 100644 index 000000000..a3089a651 --- /dev/null +++ b/supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql @@ -0,0 +1,6 @@ +-- SC-08: roles are granted by Edge Functions (service role) and +-- handle_new_user (SECURITY DEFINER). Clients only read their own row +-- (SettingsPage). Without write grants, a permissive policy left on a hosted +-- project (DB-03) can no longer let a user change their own role. +REVOKE INSERT, UPDATE, DELETE ON TABLE public.user_roles FROM anon, authenticated; +REVOKE SELECT ON TABLE public.user_roles FROM anon; diff --git a/supabase/migrations/20261023060200_revoke_client_writes_subscriptions.sql b/supabase/migrations/20261023060200_revoke_client_writes_subscriptions.sql new file mode 100644 index 000000000..51a79e39e --- /dev/null +++ b/supabase/migrations/20261023060200_revoke_client_writes_subscriptions.sql @@ -0,0 +1,5 @@ +-- SC-08: subscriptions are written only by the Stripe webhook and checkout +-- Edge Functions (service role). Clients read their own rows +-- (use-subscription.ts, settings-store.ts). +REVOKE INSERT, UPDATE, DELETE ON TABLE public.subscriptions FROM anon, authenticated; +REVOKE SELECT ON TABLE public.subscriptions FROM anon; diff --git a/supabase/migrations/20261023060300_revoke_client_writes_user_promos.sql b/supabase/migrations/20261023060300_revoke_client_writes_user_promos.sql new file mode 100644 index 000000000..2d6ac075f --- /dev/null +++ b/supabase/migrations/20261023060300_revoke_client_writes_user_promos.sql @@ -0,0 +1,5 @@ +-- SC-08: redemptions are written by redeem_promo_code_atomic (SECURITY +-- DEFINER) and read by create-checkout (service role). Signed-in clients keep +-- read access to their own rows. +REVOKE INSERT, UPDATE, DELETE ON TABLE public.user_promos FROM anon, authenticated; +REVOKE SELECT ON TABLE public.user_promos FROM anon; diff --git a/supabase/migrations/20261023060400_revoke_client_writes_admin_grants.sql b/supabase/migrations/20261023060400_revoke_client_writes_admin_grants.sql new file mode 100644 index 000000000..db1ec7da9 --- /dev/null +++ b/supabase/migrations/20261023060400_revoke_client_writes_admin_grants.sql @@ -0,0 +1,4 @@ +-- SC-08: admin grants are written only by the admin Edge Function (service +-- role). Signed-in clients keep the admin-only read policy. +REVOKE INSERT, UPDATE, DELETE ON TABLE public.admin_grants FROM anon, authenticated; +REVOKE SELECT ON TABLE public.admin_grants FROM anon; diff --git a/supabase/migrations/20261023060500_revoke_client_access_promo_codes.sql b/supabase/migrations/20261023060500_revoke_client_access_promo_codes.sql new file mode 100644 index 000000000..7563cfafe --- /dev/null +++ b/supabase/migrations/20261023060500_revoke_client_access_promo_codes.sql @@ -0,0 +1,4 @@ +-- SC-08: promo codes are read and written by the admin and create-checkout +-- Edge Functions (service role) and redeem_promo_code_atomic (SECURITY +-- DEFINER). No client reads them, and RLS has no policy for them. +REVOKE ALL ON TABLE public.promo_codes FROM anon, authenticated; diff --git a/supabase/migrations/20261023060600_revoke_client_access_daily_usage_counters.sql b/supabase/migrations/20261023060600_revoke_client_access_daily_usage_counters.sql new file mode 100644 index 000000000..ff09db3de --- /dev/null +++ b/supabase/migrations/20261023060600_revoke_client_access_daily_usage_counters.sql @@ -0,0 +1,4 @@ +-- SC-08: usage counters are written by the quota RPCs (service role) and read +-- by clients only through ai_usage_today() (SECURITY DEFINER). RLS has no +-- policy for them. +REVOKE ALL ON TABLE public.daily_usage_counters FROM anon, authenticated; diff --git a/supabase/migrations/20261023060700_index_foreign_keys.sql b/supabase/migrations/20261023060700_index_foreign_keys.sql new file mode 100644 index 000000000..b81a56657 --- /dev/null +++ b/supabase/migrations/20261023060700_index_foreign_keys.sql @@ -0,0 +1,37 @@ +-- DB-05: cover foreign keys that had no index starting with their columns. +-- Without one, deleting a user (auth.users cascades and SET NULLs), a source, +-- a promo code, a share or a workspace document scans the whole child table. +-- Skipped: global_prompts, promo_codes and curated_sources created_by, which +-- reference admin authors on tables with a handful of rows. +-- +-- Plain CREATE INDEX: the CLI runs each migration file in a transaction, so +-- CONCURRENTLY is not available. Each build briefly blocks writes to its +-- table; every table here is small or, for the hidden workspace feature, +-- empty in production. + +-- squawk-ignore-file require-concurrent-index-creation + +CREATE INDEX IF NOT EXISTS source_files_source_id_idx ON public.source_files (source_id); +CREATE INDEX IF NOT EXISTS source_files_user_id_idx ON public.source_files (user_id); +CREATE INDEX IF NOT EXISTS api_keys_user_id_idx ON public.api_keys (user_id); +CREATE INDEX IF NOT EXISTS user_promos_promo_code_id_idx ON public.user_promos (promo_code_id); +CREATE INDEX IF NOT EXISTS shared_items_created_by_idx ON public.shared_items (created_by); +CREATE INDEX IF NOT EXISTS demo_access_codes_created_by_idx ON public.demo_access_codes (created_by); + +CREATE INDEX IF NOT EXISTS workspace_audit_events_version_id_idx ON public.workspace_audit_events (version_id); +CREATE INDEX IF NOT EXISTS workspace_document_state_document_idx ON private.workspace_document_state (workspace_id, document_id); +CREATE INDEX IF NOT EXISTS workspace_document_updates_author_id_idx ON private.workspace_document_updates (author_id); +CREATE INDEX IF NOT EXISTS workspace_document_updates_document_idx ON private.workspace_document_updates (workspace_id, document_id); +CREATE INDEX IF NOT EXISTS workspace_versions_created_by_idx ON public.workspace_versions (created_by); +CREATE INDEX IF NOT EXISTS workspace_proposals_author_id_idx ON public.workspace_proposals (author_id); +CREATE INDEX IF NOT EXISTS workspace_comments_author_id_idx ON public.workspace_comments (author_id); +CREATE INDEX IF NOT EXISTS workspace_comments_resolved_by_idx ON public.workspace_comments (resolved_by); +CREATE INDEX IF NOT EXISTS workspace_comment_replies_author_id_idx ON public.workspace_comment_replies (author_id); +CREATE INDEX IF NOT EXISTS workspace_comment_replies_workspace_id_idx ON public.workspace_comment_replies (workspace_id); +CREATE INDEX IF NOT EXISTS workspace_notifications_actor_id_idx ON public.workspace_notifications (actor_id); +CREATE INDEX IF NOT EXISTS workspace_notifications_document_idx ON public.workspace_notifications (workspace_id, document_id); +CREATE INDEX IF NOT EXISTS workspace_sources_created_by_idx ON public.workspace_sources (created_by); +CREATE INDEX IF NOT EXISTS workspace_threads_created_by_idx ON public.workspace_threads (created_by); +CREATE INDEX IF NOT EXISTS workspace_thread_messages_author_id_idx ON public.workspace_thread_messages (author_id); +CREATE INDEX IF NOT EXISTS workspace_thread_messages_thread_idx ON public.workspace_thread_messages (workspace_id, thread_id); +CREATE INDEX IF NOT EXISTS workspace_artifacts_document_idx ON public.workspace_artifacts (workspace_id, document_id); diff --git a/supabase/proposed/20261023060000_drop_legacy_policies.sql b/supabase/proposed/20261023060000_drop_legacy_policies.sql new file mode 100644 index 000000000..934e0e609 --- /dev/null +++ b/supabase/proposed/20261023060000_drop_legacy_policies.sql @@ -0,0 +1,44 @@ +-- DB-03 (proposed, destructive): drop policies that no migration created. +-- +-- The QA run on 22 Oct 2026 found policies on production that are not in +-- staging or in supabase/migrations, including duplicate permissive ones on +-- shared_items, user_roles and ai_requests, and two auth_rls_initplan +-- warnings on shared_items. The repo intends exactly these policies: +-- +-- shared_items none. Clients have no table grants; reads go through +-- get_shared_item(code) and writes through the share Edge +-- Function (20260819000000_security_hardening.sql). +-- user_roles "user_roles_select_own" (SELECT own row). +-- ai_requests "ai_requests_select_own" (SELECT own rows). +-- +-- Every other policy on these tables is inert: the clients' table grants +-- (and, from 20261023060100, no write grant on user_roles) already block +-- what those policies would allow. Dropping them removes the advisor warnings +-- and the confusion. The policy names were in the QA snapshot, which is not +-- in the repo, so this drops by exclusion and prints each definition first. +-- Run on production only after checking the NOTICE output in a dry run +-- (BEGIN; ...; ROLLBACK;). + +DO $$ +DECLARE + p record; +BEGIN + FOR p IN + SELECT pol.polname, cls.relname, + pg_get_expr(pol.polqual, pol.polrelid) AS using_expr, + pg_get_expr(pol.polwithcheck, pol.polrelid) AS check_expr + FROM pg_policy AS pol + JOIN pg_class AS cls ON cls.oid = pol.polrelid + WHERE cls.relnamespace = 'public'::regnamespace + AND ( + cls.relname = 'shared_items' + OR (cls.relname = 'user_roles' AND pol.polname <> 'user_roles_select_own') + OR (cls.relname = 'ai_requests' AND pol.polname <> 'ai_requests_select_own') + ) + LOOP + RAISE NOTICE 'dropping policy % on %: USING (%) WITH CHECK (%)', + p.polname, p.relname, p.using_expr, p.check_expr; + EXECUTE format('DROP POLICY IF EXISTS %I ON public.%I', p.polname, p.relname); + END LOOP; +END; +$$; diff --git a/supabase/proposed/20261023060100_drop_staging_auto_admin.sql b/supabase/proposed/20261023060100_drop_staging_auto_admin.sql new file mode 100644 index 000000000..f61c5e199 --- /dev/null +++ b/supabase/proposed/20261023060100_drop_staging_auto_admin.sql @@ -0,0 +1,13 @@ +-- DB-04 (proposed, destructive, STAGING ONLY: brifeyypjauyusmkptkx). +-- +-- staging_auto_admin() is left over from a trigger that was dropped by hand +-- on staging. It does not exist on production or anywhere in +-- supabase/migrations, so it must never be added to migrations. Run this by +-- hand in the staging SQL editor. Check first that no trigger still uses it: +-- +-- SELECT tgname, tgrelid::regclass FROM pg_trigger t +-- JOIN pg_proc p ON p.oid = t.tgfoid WHERE p.proname = 'staging_auto_admin'; +-- +-- That query must return no rows; without CASCADE the drop fails otherwise. + +DROP FUNCTION IF EXISTS public.staging_auto_admin(); diff --git a/supabase/proposed/20261023060200_drop_duplicate_indexes.sql b/supabase/proposed/20261023060200_drop_duplicate_indexes.sql new file mode 100644 index 000000000..1dcc336a1 --- /dev/null +++ b/supabase/proposed/20261023060200_drop_duplicate_indexes.sql @@ -0,0 +1,49 @@ +-- DB-05 (proposed, destructive): drop indexes that duplicate another index +-- on the same table and column. +-- +-- subscriptions idx_subscriptions_stripe_customer duplicates +-- idx_subscriptions_stripe_customer_id (both plain +-- btree on stripe_customer_id); the QA advisor finding. +-- subscriptions idx_subscriptions_stripe_sub duplicates the unique +-- constraint index subscriptions_stripe_subscription_id_key. +-- promo_codes idx_promo_codes_code duplicates promo_codes_code_key. +-- global_prompts idx_global_prompts_feature duplicates global_prompts_feature_key. +-- user_roles idx_user_roles_user_id duplicates user_roles_user_id_key. +-- demo_access_codes demo_access_codes_email_idx duplicates demo_access_codes_email_key. +-- +-- Each drop runs only if the index that stays exists and covers exactly the +-- same columns, so a project whose indexes differ keeps both. Run it outside +-- peak hours: DROP INDEX briefly locks its table. + +DO $$ +DECLARE + pair record; +BEGIN + FOR pair IN + SELECT * FROM (VALUES + ('idx_subscriptions_stripe_customer', 'idx_subscriptions_stripe_customer_id'), + ('idx_subscriptions_stripe_sub', 'subscriptions_stripe_subscription_id_key'), + ('idx_promo_codes_code', 'promo_codes_code_key'), + ('idx_global_prompts_feature', 'global_prompts_feature_key'), + ('idx_user_roles_user_id', 'user_roles_user_id_key'), + ('demo_access_codes_email_idx', 'demo_access_codes_email_key') + ) AS v(dropped, kept) + LOOP + IF EXISTS ( + SELECT 1 + FROM pg_index AS d + JOIN pg_index AS k ON k.indrelid = d.indrelid + WHERE d.indexrelid = to_regclass('public.' || pair.dropped) + AND k.indexrelid = to_regclass('public.' || pair.kept) + AND d.indkey::text = k.indkey::text + AND d.indexprs IS NULL AND k.indexprs IS NULL + AND d.indpred IS NULL AND k.indpred IS NULL + ) THEN + RAISE NOTICE 'dropping % (kept: %)', pair.dropped, pair.kept; + EXECUTE format('DROP INDEX IF EXISTS public.%I', pair.dropped); + ELSE + RAISE NOTICE 'keeping %: % is missing or covers other columns', pair.dropped, pair.kept; + END IF; + END LOOP; +END; +$$; diff --git a/supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql b/supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql new file mode 100644 index 000000000..17b5d8ebb --- /dev/null +++ b/supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql @@ -0,0 +1,12 @@ +-- DB-05 (proposed): give private.promo_redeem_failures a primary key. +-- +-- It is the one table without a primary key. Its natural columns +-- (user_id, failed_at) can repeat when two failures share a timestamp, so a +-- key on them could fail on existing rows. A surrogate identity column cannot +-- fail: it fills every existing row. redeem_promo_code_atomic inserts only +-- user_id, so it needs no change. The table is rewritten while it holds an +-- ACCESS EXCLUSIVE lock; it holds only failed promo attempts, pruned per user +-- after a day, so that is brief. + +ALTER TABLE private.promo_redeem_failures + ADD COLUMN IF NOT EXISTS id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY; diff --git a/supabase/proposed/README.md b/supabase/proposed/README.md new file mode 100644 index 000000000..61d82233a --- /dev/null +++ b/supabase/proposed/README.md @@ -0,0 +1,5 @@ +# Proposed destructive SQL + +**Status:** Proposals only. Nothing in this directory is applied by `supabase db push`, `db reset` or CI. + +Each file here drops or constrains something that already exists on a hosted project, so it needs Adam's explicit authorization for each environment. To apply one, check the target project first, read the file's header, and run it by hand in the SQL editor (or copy it into `supabase/migrations/` under a new timestamp if it should become history). Every statement is guarded with `IF EXISTS` or a catalog check, so running a file twice changes nothing the second time. diff --git a/supabase/tests/admin_tables_rls_test.sql b/supabase/tests/admin_tables_rls_test.sql index bc9e22826..85e568902 100644 --- a/supabase/tests/admin_tables_rls_test.sql +++ b/supabase/tests/admin_tables_rls_test.sql @@ -118,8 +118,10 @@ RESET ROLE; SET LOCAL ROLE anon; SELECT set_config('request.jwt.claim.sub', '', true); SELECT set_config('request.jwt.claim.role', 'anon', true); -SELECT is_empty( +SELECT throws_ok( $$SELECT 1 FROM public.admin_grants$$, + '42501', + NULL, 'anonymous callers cannot read the grant log' ); SELECT throws_ok( diff --git a/supabase/tests/server_written_tables_grants_test.sql b/supabase/tests/server_written_tables_grants_test.sql new file mode 100644 index 000000000..107b0c2c5 --- /dev/null +++ b/supabase/tests/server_written_tables_grants_test.sql @@ -0,0 +1,76 @@ +BEGIN; +SELECT plan(6); + +-- Returns how many rows a statement changed; a privilege or RLS rejection changes none. +CREATE FUNCTION pg_temp.rows_changed(statement text) +RETURNS bigint LANGUAGE plpgsql AS $$ +DECLARE changed bigint; +BEGIN + EXECUTE statement; + GET DIAGNOSTICS changed = ROW_COUNT; + RETURN changed; +EXCEPTION WHEN insufficient_privilege THEN + RETURN 0; +END; +$$; + +-- SC-08: only the service role and SECURITY DEFINER functions write these tables. +SELECT is_empty( + $$SELECT t || ':' || r || ':' || p + FROM unnest(ARRAY['public.user_roles', 'public.subscriptions', 'public.user_promos', + 'public.admin_grants', 'public.promo_codes', 'public.daily_usage_counters']) AS t, + unnest(ARRAY['anon', 'authenticated']) AS r, + unnest(ARRAY['INSERT', 'UPDATE', 'DELETE']) AS p + WHERE has_table_privilege(r, t, p)$$, + 'clients cannot write roles, subscriptions, promos, admin grants or usage counters' +); +SELECT is_empty( + $$SELECT t || ':' || r + FROM unnest(ARRAY['public.user_roles', 'public.subscriptions', 'public.user_promos', + 'public.admin_grants', 'public.promo_codes', 'public.daily_usage_counters']) AS t, + unnest(ARRAY['anon']) AS r + WHERE has_table_privilege(r, t, 'SELECT') + UNION ALL + SELECT t FROM unnest(ARRAY['public.promo_codes', 'public.daily_usage_counters']) AS t + WHERE has_table_privilege('authenticated', t, 'SELECT')$$, + 'anon reads none of them, and signed-in users cannot read promo codes or usage counters directly' +); +SELECT ok( + (SELECT bool_and(has_table_privilege('service_role', t, p)) + FROM unnest(ARRAY['public.user_roles', 'public.subscriptions', 'public.user_promos', + 'public.admin_grants', 'public.promo_codes', 'public.daily_usage_counters']) AS t, + unnest(ARRAY['SELECT', 'INSERT', 'UPDATE', 'DELETE']) AS p), + 'the service role keeps every privilege on them' +); + +INSERT INTO auth.users (id) VALUES ('e3333333-3333-4333-8333-333333333333'); +INSERT INTO public.user_roles (user_id, role) VALUES ('e3333333-3333-4333-8333-333333333333', 'user') +ON CONFLICT (user_id) DO UPDATE SET role = 'user'; +-- Hosted projects carry permissive policies that no migration created (DB-03). +-- Without write grants, even a policy like this one must not let a user write. +CREATE POLICY pgtap_legacy_user_roles_all ON public.user_roles + FOR ALL USING (user_id = auth.uid()) WITH CHECK (user_id = auth.uid()); + +SET LOCAL ROLE authenticated; +SELECT set_config('request.jwt.claim.sub', 'e3333333-3333-4333-8333-333333333333', true); +SELECT set_config('request.jwt.claim.role', 'authenticated', true); +SELECT is( + (SELECT role::text FROM public.user_roles), + 'user', + 'a signed-in user still reads their own role' +); +SELECT is( + pg_temp.rows_changed($$UPDATE public.user_roles SET role = 'admin' + WHERE user_id = 'e3333333-3333-4333-8333-333333333333'$$), + 0::bigint, + 'a signed-in user cannot make themselves an admin' +); +RESET ROLE; +SELECT is( + (SELECT role::text FROM public.user_roles WHERE user_id = 'e3333333-3333-4333-8333-333333333333'), + 'user', + 'the role is unchanged' +); + +SELECT * FROM finish(); +ROLLBACK; diff --git a/supabase/tests/share_views_test.sql b/supabase/tests/share_views_test.sql new file mode 100644 index 000000000..00474c47e --- /dev/null +++ b/supabase/tests/share_views_test.sql @@ -0,0 +1,88 @@ +BEGIN; +SELECT plan(9); + +-- get_shared_item counts each viewer once per share per UTC day (SC-04). +INSERT INTO auth.users (id) VALUES + ('e1111111-1111-4111-8111-111111111111'), + ('e2222222-2222-4222-8222-222222222222'); +INSERT INTO public.shared_items (share_code, item_type, item_data, title, created_by) VALUES + ('pgtap-views-a', 'case', '{}', 'Share A', 'e1111111-1111-4111-8111-111111111111'), + ('pgtap-views-b', 'case', '{}', 'Share B', 'e1111111-1111-4111-8111-111111111111'); + +CREATE FUNCTION pg_temp.views(p_code text) RETURNS integer LANGUAGE sql AS $$ + SELECT view_count FROM public.shared_items WHERE share_code = p_code +$$; + +SET LOCAL ROLE anon; +SELECT set_config('request.jwt.claim.sub', '', true); +SELECT set_config('request.jwt.claim.role', 'anon', true); +SELECT set_config('request.headers', '{"x-forwarded-for": "203.0.113.7, 10.0.0.1"}', true); +SELECT is( + (SELECT view_count FROM public.get_shared_item('pgtap-views-a')), + 1, + 'the first anonymous view counts' +); +SELECT is( + (SELECT view_count FROM public.get_shared_item('pgtap-views-a')), + 1, + 'reloading from the same address does not count again, and still returns the share' +); +SELECT set_config('request.headers', '{"x-forwarded-for": "198.51.100.4"}', true); +SELECT is( + (SELECT view_count FROM public.get_shared_item('pgtap-views-a')), + 2, + 'a viewer from another address counts' +); +-- The edge sets cf-connecting-ip; a client cannot dodge it by varying x-forwarded-for. +SELECT set_config('request.headers', '{"cf-connecting-ip": "192.0.2.9", "x-forwarded-for": "1.1.1.1"}', true); +SELECT public.get_shared_item('pgtap-views-a'); +SELECT set_config('request.headers', '{"cf-connecting-ip": "192.0.2.9", "x-forwarded-for": "2.2.2.2"}', true); +SELECT is( + (SELECT view_count FROM public.get_shared_item('pgtap-views-a')), + 3, + 'cf-connecting-ip identifies the viewer ahead of x-forwarded-for' +); +SELECT is( + (SELECT view_count FROM public.get_shared_item('pgtap-views-b')), + 1, + 'a viewer already counted on one share still counts on another' +); +SELECT throws_ok( + $$SELECT * FROM public.share_views$$, + '42501', + NULL, + 'anonymous callers cannot read view records' +); +RESET ROLE; + +SET LOCAL ROLE authenticated; +SELECT set_config('request.jwt.claim.sub', 'e2222222-2222-4222-8222-222222222222', true); +SELECT set_config('request.jwt.claim.role', 'authenticated', true); +SELECT set_config('request.headers', '{"x-forwarded-for": "203.0.113.50"}', true); +SELECT public.get_shared_item('pgtap-views-a'); +SELECT set_config('request.headers', '{"x-forwarded-for": "203.0.113.51"}', true); +SELECT is( + (SELECT view_count FROM public.get_shared_item('pgtap-views-a')), + 4, + 'a signed-in viewer counts once, whatever address they reload from' +); +SELECT throws_ok( + $$INSERT INTO public.share_views (share_id, viewed_on, viewer_key) + SELECT id, current_date, '\x00' FROM public.get_shared_item('pgtap-views-b')$$, + '42501', + NULL, + 'signed-in callers cannot write view records' +); +RESET ROLE; + +-- Without request headers (direct SQL callers) the lookup still works. +SELECT set_config('request.headers', '', true); +SELECT set_config('request.jwt.claim.sub', '', true); +SELECT is( + (SELECT title FROM public.get_shared_item('pgtap-views-b')), + 'Share B', + 'a caller without request headers still reads the share' +); + +SELECT * FROM finish(); +ROLLBACK; diff --git a/supabase/tests/user_roles_rls_test.sql b/supabase/tests/user_roles_rls_test.sql index cfe13a7ed..7f4ba9d3e 100644 --- a/supabase/tests/user_roles_rls_test.sql +++ b/supabase/tests/user_roles_rls_test.sql @@ -110,8 +110,10 @@ RESET ROLE; SET LOCAL ROLE anon; SELECT set_config('request.jwt.claim.sub', '', true); SELECT set_config('request.jwt.claim.role', 'anon', true); -SELECT is_empty( +SELECT throws_ok( $$SELECT 1 FROM public.user_roles$$, + '42501', + NULL, 'anonymous callers see no roles' ); SELECT throws_ok( From 4ca5f0c9290011cadefbf85b84d8a5f88e3c81ef Mon Sep 17 00:00:00 2001 From: Adam Date: Fri, 23 Oct 2026 03:17:09 +0000 Subject: [PATCH 2/3] Address T15 review round 1 - Revoke anon SELECT on ai_requests, so a legacy permissive SELECT policy cannot expose usage rows to the anon key. - The proposed legacy-policy drop stops if user_roles_select_own or ai_requests_select_own is missing, and its header no longer calls SELECT policies inert. - Test pruning of earlier-day share views; check column-level write grants. - Duplicate-index proposal also compares uniqueness, validity, operator classes and collations. - Skip the two composite workspace document FK indexes the primary keys already serve; fix comments on lock duration, the viewer hash and who writes user_roles. Co-Authored-By: Claude Opus 5.5 --- .../20261023060000_share_view_dedup.sql | 17 +++++++----- ...060100_revoke_client_writes_user_roles.sql | 4 +-- .../20261023060700_index_foreign_keys.sql | 13 ++++----- ..._revoke_client_select_ai_requests_anon.sql | 5 ++++ .../20261023060000_drop_legacy_policies.sql | 27 ++++++++++++++----- ...20261023060100_drop_staging_auto_admin.sql | 2 ++ .../20261023060200_drop_duplicate_indexes.sql | 10 +++++-- ...0300_promo_redeem_failures_primary_key.sql | 4 ++- .../server_written_tables_grants_test.sql | 19 ++++++++----- supabase/tests/share_views_test.sql | 16 ++++++++--- 10 files changed, 82 insertions(+), 35 deletions(-) create mode 100644 supabase/migrations/20261023060800_revoke_client_select_ai_requests_anon.sql diff --git a/supabase/migrations/20261023060000_share_view_dedup.sql b/supabase/migrations/20261023060000_share_view_dedup.sql index d9b61d721..989cc31bc 100644 --- a/supabase/migrations/20261023060000_share_view_dedup.sql +++ b/supabase/migrations/20261023060000_share_view_dedup.sql @@ -4,10 +4,13 @@ -- the same. -- -- A viewer is the signed-in user, otherwise the client IP the API gateway --- forwards. The stored key is a SHA-256 of share, day and viewer, so rows --- cannot be matched across shares or days, and rows from earlier days are --- deleted as soon as a new view is counted. This stops reload inflation; it --- is not abuse protection: a client that varies its IP still counts again. +-- forwards. The stored key is a SHA-256 of share, day and viewer. It keeps +-- raw IPs out of the table, but it is not anonymous: with no secret in it, +-- an IPv4 address can be recovered by trying every address. Only server +-- roles can read the table, and rows from earlier days are deleted as soon +-- as a new view is counted. This stops reload inflation; it is not abuse +-- protection: a client that varies its IP still counts again, and viewers +-- behind one NAT address count once. CREATE TABLE public.share_views ( share_id uuid NOT NULL REFERENCES public.shared_items(id) ON DELETE CASCADE, @@ -44,8 +47,8 @@ BEGIN EXCEPTION WHEN others THEN v_headers := NULL; END; - -- cf-connecting-ip is set by the edge and cannot be supplied by the - -- client; the first x-forwarded-for entry is the fallback. + -- On hosted Supabase, Cloudflare sets cf-connecting-ip and overwrites any + -- client value; the first x-forwarded-for entry is the fallback. v_viewer := 'ip:' || coalesce( nullif(btrim(v_headers ->> 'cf-connecting-ip'), ''), nullif(btrim(split_part(v_headers ->> 'x-forwarded-for', ',', 1)), ''), @@ -58,7 +61,7 @@ BEGIN VALUES ( v_item.id, v_today, - sha256(convert_to(v_item.id::text || ':' || v_today::text || ':' || v_viewer, 'UTF8')) + sha256(convert_to(v_item.id::text || ':' || to_char(v_today, 'YYYY-MM-DD') || ':' || v_viewer, 'UTF8')) ) ON CONFLICT DO NOTHING; diff --git a/supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql b/supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql index a3089a651..c1a33710e 100644 --- a/supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql +++ b/supabase/migrations/20261023060100_revoke_client_writes_user_roles.sql @@ -1,5 +1,5 @@ --- SC-08: roles are granted by Edge Functions (service role) and --- handle_new_user (SECURITY DEFINER). Clients only read their own row +-- SC-08: roles are written only by the admin and demo-code-login Edge +-- Functions (service role). Clients only read their own row -- (SettingsPage). Without write grants, a permissive policy left on a hosted -- project (DB-03) can no longer let a user change their own role. REVOKE INSERT, UPDATE, DELETE ON TABLE public.user_roles FROM anon, authenticated; diff --git a/supabase/migrations/20261023060700_index_foreign_keys.sql b/supabase/migrations/20261023060700_index_foreign_keys.sql index b81a56657..ae21e6741 100644 --- a/supabase/migrations/20261023060700_index_foreign_keys.sql +++ b/supabase/migrations/20261023060700_index_foreign_keys.sql @@ -2,12 +2,15 @@ -- Without one, deleting a user (auth.users cascades and SET NULLs), a source, -- a promo code, a share or a workspace document scans the whole child table. -- Skipped: global_prompts, promo_codes and curated_sources created_by, which --- reference admin authors on tables with a handful of rows. +-- reference admin authors on tables with a handful of rows, and the +-- (workspace_id, document_id) keys of private.workspace_document_state and +-- private.workspace_document_updates, whose primary keys already lead with +-- document_id (the advisor still lists those two). -- -- Plain CREATE INDEX: the CLI runs each migration file in a transaction, so --- CONCURRENTLY is not available. Each build briefly blocks writes to its --- table; every table here is small or, for the hidden workspace feature, --- empty in production. +-- CONCURRENTLY is not available. Each build blocks writes to its table until +-- the whole file commits; every table here is small or, for the hidden +-- workspace feature, empty in production. -- squawk-ignore-file require-concurrent-index-creation @@ -19,9 +22,7 @@ CREATE INDEX IF NOT EXISTS shared_items_created_by_idx ON public.shared_items (c CREATE INDEX IF NOT EXISTS demo_access_codes_created_by_idx ON public.demo_access_codes (created_by); CREATE INDEX IF NOT EXISTS workspace_audit_events_version_id_idx ON public.workspace_audit_events (version_id); -CREATE INDEX IF NOT EXISTS workspace_document_state_document_idx ON private.workspace_document_state (workspace_id, document_id); CREATE INDEX IF NOT EXISTS workspace_document_updates_author_id_idx ON private.workspace_document_updates (author_id); -CREATE INDEX IF NOT EXISTS workspace_document_updates_document_idx ON private.workspace_document_updates (workspace_id, document_id); CREATE INDEX IF NOT EXISTS workspace_versions_created_by_idx ON public.workspace_versions (created_by); CREATE INDEX IF NOT EXISTS workspace_proposals_author_id_idx ON public.workspace_proposals (author_id); CREATE INDEX IF NOT EXISTS workspace_comments_author_id_idx ON public.workspace_comments (author_id); diff --git a/supabase/migrations/20261023060800_revoke_client_select_ai_requests_anon.sql b/supabase/migrations/20261023060800_revoke_client_select_ai_requests_anon.sql new file mode 100644 index 000000000..9f138ed4e --- /dev/null +++ b/supabase/migrations/20261023060800_revoke_client_select_ai_requests_anon.sql @@ -0,0 +1,5 @@ +-- SC-08: ai_requests predates the closed default privileges, so anon kept +-- SELECT. No signed-out client reads it (use-subscription.ts runs signed in), +-- and a permissive SELECT policy left on a hosted project (DB-03) would +-- otherwise expose usage rows to anyone with the anon key. +REVOKE SELECT ON TABLE public.ai_requests FROM anon; diff --git a/supabase/proposed/20261023060000_drop_legacy_policies.sql b/supabase/proposed/20261023060000_drop_legacy_policies.sql index 934e0e609..95da0d6e9 100644 --- a/supabase/proposed/20261023060000_drop_legacy_policies.sql +++ b/supabase/proposed/20261023060000_drop_legacy_policies.sql @@ -11,18 +11,31 @@ -- user_roles "user_roles_select_own" (SELECT own row). -- ai_requests "ai_requests_select_own" (SELECT own rows). -- --- Every other policy on these tables is inert: the clients' table grants --- (and, from 20261023060100, no write grant on user_roles) already block --- what those policies would allow. Dropping them removes the advisor warnings --- and the confusion. The policy names were in the QA snapshot, which is not --- in the repo, so this drops by exclusion and prints each definition first. --- Run on production only after checking the NOTICE output in a dry run --- (BEGIN; ...; ROLLBACK;). +-- Write policies on these tables are inert: clients hold no INSERT, UPDATE +-- or DELETE grant on them (user_roles from 20261023060100), and no grant at +-- all on shared_items. SELECT policies are not: signed-in users keep SELECT +-- on user_roles and ai_requests, so a legacy permissive SELECT policy there +-- is live until this runs (anon lost SELECT in 20261023060100 and +-- 20261023060800). Read the NOTICE lines with that in mind. +-- +-- The policy names were in the QA snapshot, which is not in the repo, so +-- this drops by exclusion and prints each definition first. It stops before +-- dropping anything if a policy it means to keep is missing, since dropping +-- the rest would then leave signed-in users unable to read their own role +-- (SettingsPage and the curated_sources admin policies depend on it). Do a +-- dry run first (BEGIN; ...; ROLLBACK;). DO $$ DECLARE p record; BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' + AND tablename = 'user_roles' AND policyname = 'user_roles_select_own') + OR NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' + AND tablename = 'ai_requests' AND policyname = 'ai_requests_select_own') THEN + RAISE EXCEPTION 'user_roles_select_own or ai_requests_select_own is missing; recreate it before dropping the others'; + END IF; + FOR p IN SELECT pol.polname, cls.relname, pg_get_expr(pol.polqual, pol.polrelid) AS using_expr, diff --git a/supabase/proposed/20261023060100_drop_staging_auto_admin.sql b/supabase/proposed/20261023060100_drop_staging_auto_admin.sql index f61c5e199..a205f0146 100644 --- a/supabase/proposed/20261023060100_drop_staging_auto_admin.sql +++ b/supabase/proposed/20261023060100_drop_staging_auto_admin.sql @@ -9,5 +9,7 @@ -- JOIN pg_proc p ON p.oid = t.tgfoid WHERE p.proname = 'staging_auto_admin'; -- -- That query must return no rows; without CASCADE the drop fails otherwise. +-- `\df public.staging_auto_admin` should show it with no arguments; if it +-- takes any, add them to the DROP below, or IF EXISTS skips it silently. DROP FUNCTION IF EXISTS public.staging_auto_admin(); diff --git a/supabase/proposed/20261023060200_drop_duplicate_indexes.sql b/supabase/proposed/20261023060200_drop_duplicate_indexes.sql index 1dcc336a1..20fba4394 100644 --- a/supabase/proposed/20261023060200_drop_duplicate_indexes.sql +++ b/supabase/proposed/20261023060200_drop_duplicate_indexes.sql @@ -11,8 +11,10 @@ -- user_roles idx_user_roles_user_id duplicates user_roles_user_id_key. -- demo_access_codes demo_access_codes_email_idx duplicates demo_access_codes_email_key. -- --- Each drop runs only if the index that stays exists and covers exactly the --- same columns, so a project whose indexes differ keeps both. Run it outside +-- Each drop runs only if the index that stays exists, is valid, is unique +-- whenever the dropped one is, and has the same columns, operator classes, +-- collations and no expression or predicate. A project whose indexes differ +-- keeps both. Run it outside -- peak hours: DROP INDEX briefly locks its table. DO $$ @@ -35,7 +37,11 @@ BEGIN JOIN pg_index AS k ON k.indrelid = d.indrelid WHERE d.indexrelid = to_regclass('public.' || pair.dropped) AND k.indexrelid = to_regclass('public.' || pair.kept) + AND k.indisvalid + AND (NOT d.indisunique OR k.indisunique) AND d.indkey::text = k.indkey::text + AND d.indclass::text = k.indclass::text + AND d.indcollation::text = k.indcollation::text AND d.indexprs IS NULL AND k.indexprs IS NULL AND d.indpred IS NULL AND k.indpred IS NULL ) THEN diff --git a/supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql b/supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql index 17b5d8ebb..f79ecc16c 100644 --- a/supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql +++ b/supabase/proposed/20261023060300_promo_redeem_failures_primary_key.sql @@ -6,7 +6,9 @@ -- fail: it fills every existing row. redeem_promo_code_atomic inserts only -- user_id, so it needs no change. The table is rewritten while it holds an -- ACCESS EXCLUSIVE lock; it holds only failed promo attempts, pruned per user --- after a day, so that is brief. +-- after a day, so that is brief. If the table already has an id column, +-- IF NOT EXISTS skips the whole statement, primary key included; check with +-- `\d private.promo_redeem_failures` first. ALTER TABLE private.promo_redeem_failures ADD COLUMN IF NOT EXISTS id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY; diff --git a/supabase/tests/server_written_tables_grants_test.sql b/supabase/tests/server_written_tables_grants_test.sql index 107b0c2c5..7f459a326 100644 --- a/supabase/tests/server_written_tables_grants_test.sql +++ b/supabase/tests/server_written_tables_grants_test.sql @@ -20,20 +20,27 @@ SELECT is_empty( FROM unnest(ARRAY['public.user_roles', 'public.subscriptions', 'public.user_promos', 'public.admin_grants', 'public.promo_codes', 'public.daily_usage_counters']) AS t, unnest(ARRAY['anon', 'authenticated']) AS r, - unnest(ARRAY['INSERT', 'UPDATE', 'DELETE']) AS p - WHERE has_table_privilege(r, t, p)$$, - 'clients cannot write roles, subscriptions, promos, admin grants or usage counters' + unnest(ARRAY['INSERT', 'UPDATE']) AS p + WHERE has_any_column_privilege(r, t, p) + UNION ALL + SELECT t || ':' || r || ':DELETE' + FROM unnest(ARRAY['public.user_roles', 'public.subscriptions', 'public.user_promos', + 'public.admin_grants', 'public.promo_codes', 'public.daily_usage_counters']) AS t, + unnest(ARRAY['anon', 'authenticated']) AS r + WHERE has_table_privilege(r, t, 'DELETE')$$, + 'clients cannot write roles, subscriptions, promos, admin grants or usage counters, even one column' ); SELECT is_empty( $$SELECT t || ':' || r FROM unnest(ARRAY['public.user_roles', 'public.subscriptions', 'public.user_promos', - 'public.admin_grants', 'public.promo_codes', 'public.daily_usage_counters']) AS t, + 'public.admin_grants', 'public.promo_codes', 'public.daily_usage_counters', + 'public.ai_requests']) AS t, unnest(ARRAY['anon']) AS r - WHERE has_table_privilege(r, t, 'SELECT') + WHERE has_any_column_privilege(r, t, 'SELECT') UNION ALL SELECT t FROM unnest(ARRAY['public.promo_codes', 'public.daily_usage_counters']) AS t WHERE has_table_privilege('authenticated', t, 'SELECT')$$, - 'anon reads none of them, and signed-in users cannot read promo codes or usage counters directly' + 'anon reads none of them (nor AI request history), and signed-in users cannot read promo codes or usage counters directly' ); SELECT ok( (SELECT bool_and(has_table_privilege('service_role', t, p)) diff --git a/supabase/tests/share_views_test.sql b/supabase/tests/share_views_test.sql index 00474c47e..1beae3dba 100644 --- a/supabase/tests/share_views_test.sql +++ b/supabase/tests/share_views_test.sql @@ -1,5 +1,5 @@ BEGIN; -SELECT plan(9); +SELECT plan(10); -- get_shared_item counts each viewer once per share per UTC day (SC-04). INSERT INTO auth.users (id) VALUES @@ -9,9 +9,10 @@ INSERT INTO public.shared_items (share_code, item_type, item_data, title, create ('pgtap-views-a', 'case', '{}', 'Share A', 'e1111111-1111-4111-8111-111111111111'), ('pgtap-views-b', 'case', '{}', 'Share B', 'e1111111-1111-4111-8111-111111111111'); -CREATE FUNCTION pg_temp.views(p_code text) RETURNS integer LANGUAGE sql AS $$ - SELECT view_count FROM public.shared_items WHERE share_code = p_code -$$; +-- A record from an earlier day must be pruned by the next counted view. +INSERT INTO public.share_views (share_id, viewed_on, viewer_key) +SELECT id, (now() AT TIME ZONE 'UTC')::date - 1, '\x01'::bytea +FROM public.shared_items WHERE share_code = 'pgtap-views-b'; SET LOCAL ROLE anon; SELECT set_config('request.jwt.claim.sub', '', true); @@ -75,6 +76,13 @@ SELECT throws_ok( ); RESET ROLE; +SELECT is( + (SELECT count(*) FROM public.share_views + WHERE viewed_on < (now() AT TIME ZONE 'UTC')::date), + 0::bigint, + 'records from earlier days are deleted once a new view is counted' +); + -- Without request headers (direct SQL callers) the lookup still works. SELECT set_config('request.headers', '', true); SELECT set_config('request.jwt.claim.sub', '', true); From 5c8144b1f48aa9ed914d3dcff38c9e9f7a09d75c Mon Sep 17 00:00:00 2001 From: Adam Date: Fri, 23 Oct 2026 03:30:43 +0000 Subject: [PATCH 3/3] Address T15 review round 2 - get_shared_item deletes at most 500 earlier-day view records per counted view, so the first view after midnight stays fast. - The proposed legacy-policy drop also checks that the kept policies read only the caller's own rows, and prints them. - The grants test catches column-level SELECT grants too. Co-Authored-By: Claude Opus 5.5 --- .../20261023060000_share_view_dedup.sql | 14 ++++++--- .../20261023060000_drop_legacy_policies.sql | 31 +++++++++++++------ .../server_written_tables_grants_test.sql | 2 +- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/supabase/migrations/20261023060000_share_view_dedup.sql b/supabase/migrations/20261023060000_share_view_dedup.sql index 989cc31bc..c674ad31f 100644 --- a/supabase/migrations/20261023060000_share_view_dedup.sql +++ b/supabase/migrations/20261023060000_share_view_dedup.sql @@ -1,14 +1,15 @@ -- SC-04: get_shared_item counted every call, so reloading a share link -- inflated its view count. Count each viewer at most once per share per UTC -- day instead. The share page and the function's signature and result stay --- the same. +-- the same. The number shown on the share page now means distinct viewers +-- per day, not page loads. -- -- A viewer is the signed-in user, otherwise the client IP the API gateway -- forwards. The stored key is a SHA-256 of share, day and viewer. It keeps -- raw IPs out of the table, but it is not anonymous: with no secret in it, -- an IPv4 address can be recovered by trying every address. Only server --- roles can read the table, and rows from earlier days are deleted as soon --- as a new view is counted. This stops reload inflation; it is not abuse +-- roles can read the table, and each counted view deletes up to 500 rows +-- from earlier days. This stops reload inflation; it is not abuse -- protection: a client that varies its IP still counts again, and viewers -- behind one NAT address count once. @@ -70,7 +71,12 @@ BEGIN SET view_count = s.view_count + 1 WHERE s.id = v_item.id RETURNING s.view_count INTO v_item.view_count; - DELETE FROM public.share_views AS v WHERE v.viewed_on < v_today; + -- Bounded, so a busy day never makes one viewer's request slow; the + -- next counted views delete the rest. + DELETE FROM public.share_views AS v + WHERE v.ctid = ANY (ARRAY( + SELECT o.ctid FROM public.share_views AS o WHERE o.viewed_on < v_today LIMIT 500 + )); END IF; RETURN QUERY SELECT v_item.id, v_item.share_code, v_item.item_type, diff --git a/supabase/proposed/20261023060000_drop_legacy_policies.sql b/supabase/proposed/20261023060000_drop_legacy_policies.sql index 95da0d6e9..1e4906e86 100644 --- a/supabase/proposed/20261023060000_drop_legacy_policies.sql +++ b/supabase/proposed/20261023060000_drop_legacy_policies.sql @@ -20,21 +20,32 @@ -- -- The policy names were in the QA snapshot, which is not in the repo, so -- this drops by exclusion and prints each definition first. It stops before --- dropping anything if a policy it means to keep is missing, since dropping --- the rest would then leave signed-in users unable to read their own role --- (SettingsPage and the curated_sources admin policies depend on it). Do a --- dry run first (BEGIN; ...; ROLLBACK;). +-- dropping anything unless both kept policies exist and read only the +-- caller's own rows: without user_roles_select_own, signed-in users could no +-- longer read their own role (SettingsPage and the curated_sources admin +-- policies depend on it), and a drifted copy would keep the leak this fixes. +-- Do a dry run first (BEGIN; ...; ROLLBACK;). DO $$ DECLARE p record; BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' - AND tablename = 'user_roles' AND policyname = 'user_roles_select_own') - OR NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' - AND tablename = 'ai_requests' AND policyname = 'ai_requests_select_own') THEN - RAISE EXCEPTION 'user_roles_select_own or ai_requests_select_own is missing; recreate it before dropping the others'; - END IF; + -- The kept policies must exist and read only the caller's own rows, as + -- 20260819000000 and 20261016120000 define them. + FOR p IN + SELECT k.tablename, k.policyname, pol.cmd, pol.permissive, pol.qual + FROM (VALUES ('user_roles', 'user_roles_select_own'), + ('ai_requests', 'ai_requests_select_own')) AS k(tablename, policyname) + LEFT JOIN pg_policies AS pol + ON pol.schemaname = 'public' AND pol.tablename = k.tablename AND pol.policyname = k.policyname + LOOP + RAISE NOTICE 'keeping policy % on %: % % USING (%)', p.policyname, p.tablename, p.permissive, p.cmd, p.qual; + IF p.cmd IS DISTINCT FROM 'SELECT' + OR p.qual IS DISTINCT FROM '(user_id = ( SELECT auth.uid() AS uid))' THEN + RAISE EXCEPTION 'policy % on % is missing or not own-rows SELECT; fix it before dropping the others', + p.policyname, p.tablename; + END IF; + END LOOP; FOR p IN SELECT pol.polname, cls.relname, diff --git a/supabase/tests/server_written_tables_grants_test.sql b/supabase/tests/server_written_tables_grants_test.sql index 7f459a326..102bbe94e 100644 --- a/supabase/tests/server_written_tables_grants_test.sql +++ b/supabase/tests/server_written_tables_grants_test.sql @@ -39,7 +39,7 @@ SELECT is_empty( WHERE has_any_column_privilege(r, t, 'SELECT') UNION ALL SELECT t FROM unnest(ARRAY['public.promo_codes', 'public.daily_usage_counters']) AS t - WHERE has_table_privilege('authenticated', t, 'SELECT')$$, + WHERE has_any_column_privilege('authenticated', t, 'SELECT')$$, 'anon reads none of them (nor AI request history), and signed-in users cannot read promo codes or usage counters directly' ); SELECT ok(