DB security hardening: gate leaking RPCs, repair CSV upload, schema-drift columns #793
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # Production Database Forensic Audit — 2026-07-08 | ||
|
|
||
| Shared GolfHelm + BaseballHelm Supabase project. 5 read-only auditors + Supabase | ||
| advisors. Overall grade: **C+** — structurally messy, operationally small, **zero | ||
| data corruption**. Security/hygiene debt, not integrity failure. | ||
|
|
||
| ## Dimension grades | ||
|
|
||
| | Dimension | Grade | Verdict | | ||
| |---|---|---| | ||
| | RLS coverage | B− | 100% of 260 tables have RLS. One cross-tenant write hole (course library). | | ||
| | SECURITY DEFINER fns | C→**fixed** | Anon clean; 2 fns leaked all user emails to any authed user. **Gated 2026-07-08.** | | ||
| | Data quality | C | Zero orphans/corruption; E2E still writes into prod (`organizations` 50% junk). | | ||
| | Schema truth | C+ | 8/8 recent migrations verified applied; 27 prod migrations have no repo file; drift bugs. | | ||
| | Performance | B+ | 591 MB, 9/60 conns, cached. 694 perf advisories ~all cosmetic at this scale. | | ||
|
|
||
| ## Fixed in this PR (applied to prod + repo migrations) | ||
|
|
||
| - **[P0] PII leak** — `get_users_with_auth()` / `get_platform_health_stats()` were | ||
| SECURITY DEFINER + authenticated-EXECUTE with no gate; any logged-in user could | ||
| dump every user's email + auth metadata. Now `is_admin()/is_super_admin()`-gated | ||
| (migration `20260708020000`). | ||
| - **[P1] Ungated RPC cluster** — `update_user_last_seen` (could overwrite any user's | ||
| timestamp) now self-or-admin; dead RPCs (`get_pending_task_reminders`, | ||
| `mark_task_reminder_sent`), CRM analytics (`get_crm_click_destinations`, | ||
| `get_crm_template_performance`), and `refresh_crm_coach_engagement` (cron uses | ||
| service_role) had authenticated EXECUTE revoked (migration `20260708021000`). | ||
| - **[P0] CSV stat upload broken** — `uploadStatsCSV` wrote `upload_batch_id` (a | ||
| column that never existed) on every insert → every upload failed. Removed the | ||
| legacy write; added the 7 real stat columns the type declared but the table | ||
| lacked (`caught_stealing`, `sacrifice_bunts`, `runs_allowed`, `pitches_thrown`, | ||
| `strikes_thrown`, `launch_angle`, `spin_rate`) additively so the DB matches the | ||
| type; type now names the 6 real `source_*`/`import_run_id` columns it was | ||
| missing (migration `20260708022000`). | ||
|
|
||
| ## Deferred — needs your decision (NOT changed) | ||
|
|
||
| - **[P1] Course-library cross-tenant writes.** `golf_course_tee_holes` (ALL, | ||
| `USING true`), `golf_course_tees` (UPDATE, `USING true`), `golf_courses` (UPDATE, | ||
| `auth.uid() IS NOT NULL`) let any authenticated user edit/delete any school's | ||
| course data. **This may be intentional** — the library is a soft-delete, | ||
| crowd-sourced "grows-from-saves" wiki. I did not tighten it because it's a live | ||
| golf product and the open-edit model is plausibly by design. **Decision needed:** | ||
| is cross-school course editing intended (wiki model → add a server-side audit | ||
| trigger so edits are always logged) or not (→ scope writes to owner/admin)? | ||
| - Security-definer `*_public` views expose a platform-wide coach directory to any | ||
| authed user (no PII); fine at 10 teams, add `is_public` opt-in before scaling. | ||
| - 27 orphan migrations (recorded in prod, no repo file); `schema_migrations` | ||
| version/filename mismatches + 6 double-recorded — a future replay-from-scratch | ||
| hazard, not a live bug. | ||
| - `admin_events` + `error_logs` = 80% of DB size, ~1,550 rows/day, no retention. | ||
| - E2E suite writes into the shared prod DB (an "E2E Test University" org was created | ||
| 6 days before this audit). See the separate-E2E-project recommendation. | ||
|
|
||
| ## Healthy (verified, don't worry) | ||
|
|
||
| RLS on every table. Zero FK orphans, zero childless conversations, zero | ||
| impossible/negative stats, zero future-dated rows. 160-game E2E purge held. | ||
| Graveyard tables dormant. The scary advisor counts (199 unindexed FKs, 259 unused | ||
| indexes) waste 9.6 MB total and sit on tiny tables — cosmetic at this scale. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| -- SECURITY (2026-07-08 forensic audit): get_users_with_auth() and | ||
| -- get_platform_health_stats() are SECURITY DEFINER + EXECUTE-to-authenticated | ||
| -- but had NO authorization check — any logged-in user could dump every user's | ||
| -- email + auth metadata, or read platform DB/connection telemetry. Prepend the | ||
| -- same is_super_admin()/is_admin() gate the sibling admin RPCs | ||
| -- (get_admin_dashboard_rollup, get_audit_log_recent) already use. Bodies are | ||
| -- otherwise byte-identical to the pre-existing definitions. Idempotent | ||
| -- (CREATE OR REPLACE). Applied to prod via MCP apply_migration on 2026-07-08. | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_users_with_auth() | ||
| RETURNS json | ||
| LANGUAGE plpgsql | ||
| STABLE SECURITY DEFINER | ||
| SET search_path TO 'public' | ||
| AS $function$ | ||
| DECLARE | ||
| result JSON; | ||
| BEGIN | ||
| IF NOT (public.is_super_admin() OR public.is_admin()) THEN | ||
| RAISE EXCEPTION 'Forbidden' USING ERRCODE = '42501'; | ||
| END IF; | ||
|
|
||
| SELECT COALESCE(json_agg(u), '[]'::json) INTO result | ||
| FROM ( | ||
| SELECT | ||
| pu.id, | ||
| pu.email, | ||
| pu.role, | ||
| pu.created_at, | ||
| pu.last_seen, | ||
| au.last_sign_in_at, | ||
| au.email_confirmed_at, | ||
| au.created_at as auth_created_at | ||
| FROM users pu | ||
| LEFT JOIN auth.users au ON au.id = pu.id | ||
| ORDER BY pu.created_at DESC | ||
| ) u; | ||
|
|
||
| RETURN result; | ||
| END; | ||
| $function$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_platform_health_stats() | ||
| RETURNS TABLE(active_users_1h integer, active_users_24h integer, active_users_7d integer, active_users_30d integer, active_sessions integer, total_sessions integer, total_auth_users integer, users_signed_in_today integer, users_never_signed_in integer, db_size_bytes bigint, largest_tables jsonb, active_connections integer, idle_connections integer) | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path TO 'public', 'pg_catalog' | ||
| AS $function$ | ||
| DECLARE | ||
| v_1h TIMESTAMPTZ := NOW() - INTERVAL '1 hour'; | ||
| v_24h TIMESTAMPTZ := NOW() - INTERVAL '24 hours'; | ||
| v_7d TIMESTAMPTZ := NOW() - INTERVAL '7 days'; | ||
| v_30d TIMESTAMPTZ := NOW() - INTERVAL '30 days'; | ||
| v_today TIMESTAMPTZ := DATE_TRUNC('day', NOW()); | ||
| v_largest jsonb; | ||
| BEGIN | ||
| IF NOT (public.is_super_admin() OR public.is_admin()) THEN | ||
| RAISE EXCEPTION 'Forbidden' USING ERRCODE = '42501'; | ||
| END IF; | ||
|
|
||
| SELECT COALESCE( | ||
| jsonb_agg(t ORDER BY (t->>'size_bytes')::bigint DESC), | ||
| '[]'::jsonb | ||
| ) | ||
| INTO v_largest | ||
| FROM ( | ||
| SELECT jsonb_build_object( | ||
| 'table_name', n.nspname || '.' || c.relname, | ||
| 'size_bytes', pg_total_relation_size(c.oid), | ||
| 'row_count', GREATEST(c.reltuples::bigint, 0) | ||
| ) AS t | ||
| FROM pg_class c | ||
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| WHERE c.relkind = 'r' | ||
| AND n.nspname NOT IN ('pg_catalog', 'information_schema', 'pg_toast') | ||
| AND n.nspname NOT LIKE 'pg_temp_%' | ||
| AND n.nspname NOT LIKE 'pg_toast_temp_%' | ||
| ORDER BY pg_total_relation_size(c.oid) DESC | ||
| LIMIT 5 | ||
| ) sub; | ||
|
|
||
| RETURN QUERY SELECT | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen >= v_1h), | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen >= v_24h), | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen >= v_7d), | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen >= v_30d), | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen >= v_1h), | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen IS NOT NULL), | ||
| (SELECT COUNT(*)::INTEGER FROM users), | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen >= v_today), | ||
| (SELECT COUNT(*)::INTEGER FROM users WHERE last_seen IS NULL), | ||
| (SELECT pg_database_size(current_database())::BIGINT), | ||
| COALESCE(v_largest, '[]'::jsonb), | ||
| (SELECT COUNT(*)::INTEGER FROM pg_stat_activity WHERE state = 'active' AND datname = current_database()), | ||
| (SELECT COUNT(*)::INTEGER FROM pg_stat_activity WHERE state = 'idle' AND datname = current_database()); | ||
| END; | ||
| $function$; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| -- SECURITY (2026-07-08 forensic audit): tighten ungated SECURITY DEFINER RPCs. | ||
| -- Each verified individually for safe blast radius (app call sites checked). | ||
| -- Idempotent. Applied to prod via MCP apply_migration on 2026-07-08. | ||
|
|
||
| -- (1) update_user_last_seen: was UPDATE-any-user-by-uuid. The only app caller | ||
| -- (components/admin/LastSeenUpdater.tsx) always passes the CALLER's own id. | ||
| -- Guard so a user can only touch their own row (admins may touch any). | ||
| CREATE OR REPLACE FUNCTION public.update_user_last_seen(target_user_id uuid) | ||
| RETURNS void | ||
| LANGUAGE plpgsql | ||
| SECURITY DEFINER | ||
| SET search_path TO 'public' | ||
| AS $function$ | ||
| BEGIN | ||
| IF target_user_id <> auth.uid() AND NOT public.is_admin() THEN | ||
| RAISE EXCEPTION 'Forbidden' USING ERRCODE = '42501'; | ||
| END IF; | ||
|
|
||
| UPDATE users | ||
| SET last_seen = NOW() | ||
| WHERE id = target_user_id | ||
| AND (last_seen IS NULL OR last_seen < NOW() - INTERVAL '5 minutes'); | ||
| END; | ||
| $function$; | ||
|
Comment on lines
+8
to
+24
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -nP "update_user_last_seen|FUNCTION public\.is_admin\b|GRANT EXECUTE .*update_user_last_seen" supabase/migrationsRepository: njrini99-code/helmv3 Length of output: 1279 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,220p' supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql
printf '\n---\n'
rg -n "CREATE OR REPLACE FUNCTION public\.is_admin|CREATE OR REPLACE FUNCTION .*is_admin" supabase/migrationsRepository: njrini99-code/helmv3 Length of output: 2426 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'MIGRATION FILE:\n'
cat -n supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql | sed -n '1,120p'
printf '\nBASELINE is_admin DEF:\n'
sed -n '3853,3895p' supabase/migrations/20260527000000_prod_public_baseline.sql
printf '\nBASELINE update_user_last_seen GRANTS:\n'
sed -n '20905,20920p' supabase/migrations/20260527000000_prod_public_baseline.sqlRepository: njrini99-code/helmv3 Length of output: 4738 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "REVOKE EXECUTE ON FUNCTION .*FROM PUBLIC|ALTER DEFAULT PRIVILEGES.*FUNCTIONS|REVOKE .*EXECUTE.*FUNCTIONS.*PUBLIC" supabase/migrationsRepository: njrini99-code/helmv3 Length of output: 2326 Harden
🤖 Prompt for AI Agents |
||
|
|
||
| -- (2) Dead RPCs (zero app call sites; auditor-confirmed unreachable): revoke | ||
| -- EXECUTE from authenticated/anon. A future service_role/cron caller would | ||
| -- bypass these grants anyway. | ||
| REVOKE EXECUTE ON FUNCTION public.get_pending_task_reminders() FROM authenticated, anon; | ||
| REVOKE EXECUTE ON FUNCTION public.mark_task_reminder_sent(uuid) FROM authenticated, anon; | ||
|
|
||
| -- (3) CRM analytics RPCs: zero app call sites, and every sibling CRM analytics | ||
| -- RPC is admin-gated. Revoke authenticated/anon EXECUTE (admin surfaces call | ||
| -- CRM analytics via the service_role admin client). | ||
| REVOKE EXECUTE ON FUNCTION public.get_crm_click_destinations(text, integer) FROM authenticated, anon; | ||
| REVOKE EXECUTE ON FUNCTION public.get_crm_template_performance(text) FROM authenticated, anon; | ||
|
|
||
| -- (4) refresh_crm_coach_engagement: only caller is the cron route, which uses | ||
| -- the service_role admin client (src/app/api/cron/refresh-engagement/route.ts | ||
| -- -> createAdminClient). Revoke authenticated to remove the shared-DB DoS | ||
| -- surface (a materialized-view refresh any user could otherwise trigger). | ||
| REVOKE EXECUTE ON FUNCTION public.refresh_crm_coach_engagement() FROM authenticated, anon; | ||
|
Comment on lines
+29
to
+42
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
for fn in get_pending_task_reminders mark_task_reminder_sent get_crm_click_destinations get_crm_template_performance refresh_crm_coach_engagement; do
echo "== $fn =="
rg -nP "FUNCTION public\.$fn\b" supabase/migrations
doneRepository: njrini99-code/helmv3 Length of output: 1131 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== target migration =="
sed -n '1,140p' supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql
echo
echo "== search for PUBLIC revokes/default privileges =="
rg -n "REVOKE EXECUTE ON FUNCTION .* FROM PUBLIC|ALTER DEFAULT PRIVILEGES.*FUNCTIONS|ALTER DEFAULT PRIVILEGES.*EXECUTE" supabase/migrations
echo
echo "== search for the five RPC definitions and any explicit grants/revokes =="
rg -n "get_pending_task_reminders|mark_task_reminder_sent|get_crm_click_destinations|get_crm_template_performance|refresh_crm_coach_engagement" supabase/migrationsRepository: njrini99-code/helmv3 Length of output: 10100 Revoke 🤖 Prompt for AI Agents |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| -- Schema drift repair (2026-07-08 audit): the BaseballPlayerStats TS type | ||
| -- declares 7 real baseball stats that were never added to the live table, so | ||
| -- the UI renders them as always-null and any write naming them 400s. These are | ||
| -- all legitimate stats the app already surfaces (launch_angle/spin_rate on | ||
| -- metrics cards, caught_stealing etc. on box scores). Additive + nullable = | ||
| -- safe (no rewrite, no default). Makes the table match the type. The 8th | ||
| -- phantom field, upload_batch_id, is a legacy dup of the existing import_run_id | ||
| -- and is NOT added — its write is removed in the same PR. Applied to prod | ||
| -- via MCP apply_migration on 2026-07-08. | ||
| ALTER TABLE public.baseball_player_stats | ||
| ADD COLUMN IF NOT EXISTS caught_stealing integer, | ||
| ADD COLUMN IF NOT EXISTS sacrifice_bunts integer, | ||
| ADD COLUMN IF NOT EXISTS runs_allowed integer, | ||
| ADD COLUMN IF NOT EXISTS pitches_thrown integer, | ||
| ADD COLUMN IF NOT EXISTS strikes_thrown integer, | ||
| ADD COLUMN IF NOT EXISTS launch_angle numeric, | ||
| ADD COLUMN IF NOT EXISTS spin_rate numeric; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tighten
source_trust_level/source_visibility/source_match_tierto literal unions instead ofstring.This file already models every other enum-like DB column as a literal union (
BaseballStatSource,BaseballUploadStatus,BaseballTrend,BaseballInsightStatus, etc.) declared right above this interface. The three new provenance fields regress that convention by using barestring, which loses compile-time protection against typos ('staff_only'vs'staff_Only') and silently accepts any string a caller passes.Suggested fix, matching the existing style in this file:
then apply them to
source_trust_level: BaseballSourceTrustLevel | null;andsource_visibility: BaseballSourceVisibility;(and similarly forsource_match_tierif its value set is fixed). Confirm the exact permitted values against the corresponding CHECK constraints before naming the union members.🤖 Prompt for AI Agents
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: njrini99-code/helmv3
Length of output: 1726
🏁 Script executed:
Repository: njrini99-code/helmv3
Length of output: 16149
Make
source_visibilitynullable insrc/lib/types/index.ts:179-187.supabase/migrations/20260624000460_baseball_import_registry_load_bearing.sql:48-60allowsbaseball_player_stats.source_visibilityto beNULLand treatsNULLas the legacy/manual value. This type should bestring | null;import_run_idis already nullable and doesn’t need a change.🤖 Prompt for AI Agents