DB security hardening: gate leaking RPCs, repair CSV upload, schema-drift columns#793
Merged
Merged
Conversation
…ge holes) get_users_with_auth() and get_platform_health_stats() were authenticated- executable with zero authorization — any logged-in user could dump every user's email + auth metadata. Both now gated with is_admin()/is_super_admin(), matching sibling admin RPCs. update_user_last_seen() constrained to self-or- admin; dead task-reminder RPCs, two CRM analytics RPCs, and the cron-only refresh_crm_coach_engagement had authenticated EXECUTE revoked. Verified each against app call sites; anon never had access. Applied to prod via MCP. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…write, reconcile type with table uploadStatsCSV wrote upload_batch_id (a column that never existed on baseball_player_stats) on every row → every CSV upload failed the insert. Removed the legacy write (import_run_id is the canonical linkage). 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; BaseballPlayerStats type now names the 6 real source_*/ import_run_id columns it was missing. Table and type now match exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
…cord Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Qodo is busy workingCheck back in a few minutes. Qodo's code review agents are on it. |
|
Updates to Preview Branch (fix/db-security-hardening-2026-07-08) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
Summary by CodeRabbit
WalkthroughThis PR adds two Supabase migrations gating SECURITY DEFINER RPCs (admin telemetry/PII functions and user last_seen update) and revoking execute grants on several ungated RPCs, adds missing columns to baseball_player_stats, replaces upload_batch_id with new source/import provenance fields in the BaseballPlayerStats type and CSV insert logic, and adds a forensic audit document. ChangesDB Security Gating and Audit
Baseball Stats Provenance and Schema Fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_platform_health_stats
participant is_super_admin_is_admin
participant pg_catalog
Caller->>get_platform_health_stats: call RPC
get_platform_health_stats->>is_super_admin_is_admin: check authorization
is_super_admin_is_admin-->>get_platform_health_stats: allowed / Forbidden 42501
get_platform_health_stats->>pg_catalog: query pg_stat_activity, pg_database_size, pg_class
pg_catalog-->>get_platform_health_stats: counts and table sizes
get_platform_health_stats-->>Caller: health stats table
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 🔧 ESLint
src/lib/types/index.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Comment |
coderabbitai
Bot
added
database
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/types/index.ts`:
- Around line 179-187: The provenance fields in the type definitions currently
use bare string types instead of the file’s existing literal-union pattern, so
tighten `source_trust_level`, `source_visibility`, and `source_match_tier` in
`Baseball...` type declarations to dedicated union aliases declared near the
other enum-like types in this file. Add symbols such as
`BaseballSourceTrustLevel`, `BaseballSourceVisibility`, and a matching
`BaseballSourceMatchTier` if the value set is fixed, then update the interface
fields to use those aliases (with nullability preserved where needed). Verify
the allowed values against the DB constraints before naming the union members.
- Around line 179-187: `source_visibility` in `src/lib/types/index.ts` is typed
too strictly and should allow null to match the database schema. Update the
`baseball_player_stats` type definition so `source_visibility` is `string |
null`, alongside the existing nullable provenance fields like
`source_trust_level` and `import_run_id`; no other fields in this block need to
change.
In `@supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql`:
- Around line 29-42: The REVOKE statements in the migration only remove EXECUTE
from authenticated and anon, leaving the default PUBLIC path open on these
definer RPCs. Update each REVOKE for public.get_pending_task_reminders,
public.mark_task_reminder_sent, public.get_crm_click_destinations,
public.get_crm_template_performance, and public.refresh_crm_coach_engagement to
also revoke PUBLIC, while preserving access for the service_role/admin caller
used by the cron and CRM admin surfaces.
- Around line 8-24: Harden public.update_user_last_seen by tightening its
authorization check and grants. In the update_user_last_seen function, replace
the target_user_id comparison with IS DISTINCT FROM so NULL auth.uid() values
cannot bypass the forbidden branch, and keep the public.is_admin() exception
path intact. Also update the migration’s GRANT/REVOKE section to revoke EXECUTE
from PUBLIC in addition to anon and authenticated, matching the other SECURITY
DEFINER hardening patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 451ccfe2-58ae-4638-978c-a291c7f5925e
⛔ Files ignored due to path filters (1)
src/lib/types/database.tsis excluded by!src/lib/types/database.ts
📒 Files selected for processing (6)
docs/audits/DB_FORENSIC_AUDIT_2026-07-08.mdsrc/app/baseball/actions/stats.tssrc/lib/types/index.tssupabase/migrations/20260708020000_gate_admin_telemetry_rpcs_pii.sqlsupabase/migrations/20260708021000_gate_ungated_definer_rpcs.sqlsupabase/migrations/20260708022000_baseball_player_stats_real_stat_columns.sql
💤 Files with no reviewable changes (1)
- src/app/baseball/actions/stats.ts
Comment on lines
+179
to
+187
| // Source-trust / import provenance (added by the source-trust + import-run | ||
| // migrations; import_run_id supersedes the removed legacy upload_batch_id). | ||
| source_trust_level: string | null; | ||
| source_visibility: string; | ||
| source_match_confidence: number | null; | ||
| source_match_tier: string | null; | ||
| source_external_id: string | null; | ||
| import_run_id: string | null; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tighten source_trust_level / source_visibility / source_match_tier to literal unions instead of string.
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 bare string, 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:
export type BaseballSourceTrustLevel =
| 'official' | 'verified_vendor' | 'coach_reviewed' | 'player_submitted' | 'unverified' | 'inferred';
export type BaseballSourceVisibility = 'staff_only' | 'player_visible' | 'restricted';then apply them to source_trust_level: BaseballSourceTrustLevel | null; and source_visibility: BaseballSourceVisibility; (and similarly for source_match_tier if its value set is fixed). Confirm the exact permitted values against the corresponding CHECK constraints before naming the union members.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/types/index.ts` around lines 179 - 187, The provenance fields in the
type definitions currently use bare string types instead of the file’s existing
literal-union pattern, so tighten `source_trust_level`, `source_visibility`, and
`source_match_tier` in `Baseball...` type declarations to dedicated union
aliases declared near the other enum-like types in this file. Add symbols such
as `BaseballSourceTrustLevel`, `BaseballSourceVisibility`, and a matching
`BaseballSourceMatchTier` if the value set is fixed, then update the interface
fields to use those aliases (with nullability preserved where needed). Verify
the allowed values against the DB constraints before naming the union members.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nullability + defaults for the new provenance columns and confirm write-path coverage.
grep -n "source_visibility\|import_run_id\|source_trust_level" src/app/baseball/actions/stats.ts
rg -n "ADD COLUMN.*source_visibility|ADD COLUMN.*import_run_id" supabase/migrationsRepository: njrini99-code/helmv3
Length of output: 1726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== stats.ts relevant insert/update path =="
sed -n '430,620p' src/app/baseball/actions/stats.ts
echo
echo "== migration for source_visibility =="
sed -n '1,140p' supabase/migrations/20260624000460_baseball_import_registry_load_bearing.sql
echo
echo "== migration for import_run_id on baseball_player_stats =="
sed -n '1,120p' supabase/migrations/20260701007000_baseball_player_stats_import_run_id.sql
echo
echo "== source_visibility / import_run_id in generated DB types =="
rg -n "baseball_player_stats.*source_visibility|baseball_player_stats.*import_run_id|source_visibility:|import_run_id:" src/lib/types/database.tsRepository: njrini99-code/helmv3
Length of output: 16149
Make source_visibility nullable in src/lib/types/index.ts:179-187.
supabase/migrations/20260624000460_baseball_import_registry_load_bearing.sql:48-60 allows baseball_player_stats.source_visibility to be NULL and treats NULL as the legacy/manual value. This type should be string | null; import_run_id is already nullable and doesn’t need a change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/types/index.ts` around lines 179 - 187, `source_visibility` in
`src/lib/types/index.ts` is typed too strictly and should allow null to match
the database schema. Update the `baseball_player_stats` type definition so
`source_visibility` is `string | null`, alongside the existing nullable
provenance fields like `source_trust_level` and `import_run_id`; no other fields
in this block need to change.
Comment on lines
+8
to
+24
| 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$; |
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 public.update_user_last_seen authz and grants.
supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql:15should useIS DISTINCT FROMinstead of<>;auth.uid()is NULL for anon/service_role, so the current guard can skip the exception.supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql:29-42should mirror the other hardening migrations and revokeEXECUTEfromPUBLICtoo, not justanon/authenticated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql` around
lines 8 - 24, Harden public.update_user_last_seen by tightening its
authorization check and grants. In the update_user_last_seen function, replace
the target_user_id comparison with IS DISTINCT FROM so NULL auth.uid() values
cannot bypass the forbidden branch, and keep the public.is_admin() exception
path intact. Also update the migration’s GRANT/REVOKE section to revoke EXECUTE
from PUBLIC in addition to anon and authenticated, matching the other SECURITY
DEFINER hardening patterns.
Comment on lines
+29
to
+42
| 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; |
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 PUBLIC too supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql:29-42 only removes authenticated/anon; the default PUBLIC EXECUTE path on these functions stays open. Add PUBLIC to each REVOKE (keep service_role if the cron/admin caller still needs it).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260708021000_gate_ungated_definer_rpcs.sql` around
lines 29 - 42, The REVOKE statements in the migration only remove EXECUTE from
authenticated and anon, leaving the default PUBLIC path open on these definer
RPCs. Update each REVOKE for public.get_pending_task_reminders,
public.mark_task_reminder_sent, public.get_crm_click_destinations,
public.get_crm_template_performance, and public.refresh_crm_coach_engagement to
also revoke PUBLIC, while preserving access for the service_role/admin caller
used by the cron and CRM admin surfaces.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up to the 2026-07-08 forensic DB audit (
docs/audits/DB_FORENSIC_AUDIT_2026-07-08.md). Independent of PR #792. DB changes already applied to prod via MCP; these migrations are the idempotent repo record.Fixed
get_users_with_auth()andget_platform_health_stats()wereSECURITY DEFINER+ authenticated-EXECUTE with no authorization check — any logged-in user could dump every user's email + auth metadata, or read DB/connection telemetry. Now gated withis_admin()/is_super_admin()(still callable by admins, never anon), matching the sibling admin RPCs.update_user_last_seen(could overwrite any user's timestamp → self-or-admin); deadget_pending_task_reminders/mark_task_reminder_sent, CRM analyticsget_crm_click_destinations/get_crm_template_performance, and cron-onlyrefresh_crm_coach_engagementhad authenticated EXECUTE revoked. Each verified against app call sites first.uploadStatsCSVwroteupload_batch_id— a column that never existed — so every upload failed the insert. Removed the legacy write; added the 7 real stat columns the type declared but the table lacked (additive/nullable); reconciled theBaseballPlayerStatstype with the table exactly.Verified
tsc0 · lint 0 · ratchet 0 · unit 4462 · business 4536 · types-drift 0 · schema-invariants 0. Every gated function re-queried post-apply: gates present, admins retain access, anon has none.Deferred to you (documented, NOT changed)
The course-library cross-tenant write policies (
golf_course_tee_holes/golf_course_tees/golf_courseswithUSING true) let any authenticated user edit any school's course data. This may be intentional (crowd-sourced, soft-delete "grows-from-saves" library). I did not touch it — it's a live golf feature and a product-design call. Decision needed: wiki model (add an audit trigger) vs owner-scoped (tighten policies). See the audit doc §Deferred.🤖 Generated with Claude Code
https://claude.ai/code/session_01LGqvhig8njW38q9PAwDXgu