Harden metrics RPCs and enforce migration policy#1748
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request adds RBAC-protected SQL functions for per-app and global metrics (with date-range overloads, cache seeding, and cycle-derived ranges) and updates AGENTS.md to require CLI-based migration creation via Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant getAppMetrics as get_app_metrics(org_id, start_date, end_date)
participant AccessControl as check_min_rights & org validation
participant Cache as app_metrics_cache
participant Seeder as seed_get_app_metrics_caches
participant Parser as jsonb_to_recordset
Client->>getAppMetrics: Request metrics
getAppMetrics->>AccessControl: Verify role & org
alt access denied or org missing
AccessControl-->>getAppMetrics: deny / empty result
getAppMetrics-->>Client: return no rows
else access granted
AccessControl-->>getAppMetrics: proceed
getAppMetrics->>Cache: fetch cache entry
alt cache miss or stale
Cache-->>getAppMetrics: no entry
getAppMetrics->>Seeder: seed cache
Seeder-->>Cache: populate cache
else cache hit
Cache-->>getAppMetrics: cache JSONB
end
getAppMetrics->>Parser: parse JSONB -> rows
Parser-->>getAppMetrics: metric rows
getAppMetrics-->>Client: return sorted metrics
end
sequenceDiagram
actor Client
participant getGlobal as get_global_metrics(org_id, start_date, end_date)
participant getApp as get_app_metrics(org_id, start_date, end_date)
participant Aggregator as Aggregation by date
Client->>getGlobal: Request global metrics
getGlobal->>getApp: fetch per-app metrics
getApp-->>getGlobal: per-app rows
getGlobal->>Aggregator: sum metrics grouped by date
Aggregator-->>getGlobal: aggregated rows
getGlobal-->>Client: return global metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85c1f805b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fail bigint, | ||
| install bigint, | ||
| uninstall bigint | ||
| ) LANGUAGE plpgsql STABLE |
There was a problem hiding this comment.
CREATE OR REPLACE FUNCTION without SECURITY DEFINER changes this overload to security-invoker, so callers now execute the cache read/refresh logic with their own role. In this repo, public.app_metrics_cache is protected by a deny-all RLS policy and public.seed_get_app_metrics_caches(...) execution is revoked from API roles, so get_app_metrics(org_id, start_date, end_date) can return empty data or error for normal RPC callers instead of serving metrics; this regression then propagates to get_global_metrics(...) which depends on it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 269-275: Remove the repeated duplicate bullet points in the
migration guidance section so only one "Use CLI for migrations. Never manually
create migration files." line remains; edit the AGENTS.md content block
containing the migration bullets (the set that currently contains four identical
lines) and delete the extra three duplicates, leaving a single, correctly
formatted bullet about using the CLI for migrations.
In `@supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql`:
- Around line 17-18: The functions declaring "LANGUAGE plpgsql STABLE"
incorrectly promise no DB writes but call seed_get_app_metrics_caches(), which
performs INSERT ... ON CONFLICT DO UPDATE on app_metrics_cache; update the
function declarations for both overloads (the multi-parameter and
single-parameter variants that call seed_get_app_metrics_caches()) to remove the
STABLE qualifier or explicitly mark them VOLATILE so the planner knows they can
modify the database and the behavior is correct.
- Around line 41-52: The get_app_metrics functions are running as SECURITY
INVOKER so the RLS "Deny all" on app_metrics_cache causes the SELECT INTO
cache_entry (lines referencing app_metrics_cache) to return no rows and always
reseed; change both overloads of get_app_metrics (the 3-parameter overload that
calls seed_get_app_metrics_caches(org_id, start_date, end_date) and the
1-parameter overload) to be declared SECURITY DEFINER, leaving the existing
check_min_rights() authorization checks intact so access control remains
enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df53e4ef-a508-4a08-8d5d-422c5f14ac40
📒 Files selected for processing (2)
AGENTS.mdsupabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql
AGENTS.md
Outdated
| - If a migration file is newly created and not yet committed, it may be modified. | ||
| - Keep amending the same migration file while the pull request is still open. | ||
| - Never create a second migration file for the same schema change set. If not yet merged, do not split work into multiple migration files for one change. | ||
| - Use CLI for migrations. Never manually create migration files. | ||
| - Use CLI for migrations. Never manually create migration files. | ||
| - Use CLI for migrations. Never manually create migration files. | ||
| - Use CLI for migrations. Never manually create migration files. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate lines.
Lines 272–275 repeat the identical instruction four times. One occurrence is sufficient.
✂️ Proposed fix
- Keep amending the same migration file while the pull request is still open.
- Never create a second migration file for the same schema change set. If not yet merged, do not split work into multiple migration files for one change.
- Use CLI for migrations. Never manually create migration files.
-- Use CLI for migrations. Never manually create migration files.
-- Use CLI for migrations. Never manually create migration files.
-- Use CLI for migrations. Never manually create migration files.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - If a migration file is newly created and not yet committed, it may be modified. | |
| - Keep amending the same migration file while the pull request is still open. | |
| - Never create a second migration file for the same schema change set. If not yet merged, do not split work into multiple migration files for one change. | |
| - Use CLI for migrations. Never manually create migration files. | |
| - Use CLI for migrations. Never manually create migration files. | |
| - Use CLI for migrations. Never manually create migration files. | |
| - Use CLI for migrations. Never manually create migration files. | |
| - If a migration file is newly created and not yet committed, it may be modified. | |
| - Keep amending the same migration file while the pull request is still open. | |
| - Never create a second migration file for the same schema change set. If not yet merged, do not split work into multiple migration files for one change. | |
| - Use CLI for migrations. Never manually create migration files. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 269 - 275, Remove the repeated duplicate bullet
points in the migration guidance section so only one "Use CLI for migrations.
Never manually create migration files." line remains; edit the AGENTS.md content
block containing the migration bullets (the set that currently contains four
identical lines) and delete the extra three duplicates, leaving a single,
correctly formatted bullet about using the CLI for migrations.
supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql (2)
17-18:⚠️ Potential issue | 🟠 MajorRemove
STABLEfrom bothget_app_metricsoverloads.Line 17 and Line 105 promise PostgreSQL these functions are read-only, but the call path can reach
public.seed_get_app_metrics_caches(...), which upsertspublic.app_metrics_cache. These overloads should beVOLATILE(or just omit the qualifier) so the planner does not assume no writes.Suggested fix
-) LANGUAGE plpgsql STABLE +) LANGUAGE plpgsql SET search_path TO '' AS $function$Apply the same change to both overloads.
#!/bin/bash # Verify that the seeded cache helper writes and that both overloads are marked STABLE. rg -n --type sql -C3 'CREATE OR REPLACE FUNCTION public\.seed_get_app_metrics_caches|INSERT INTO public\.app_metrics_cache|ON CONFLICT \(org_id\)' supabase/migrations rg -n --type sql -C2 'CREATE OR REPLACE FUNCTION public\.get_app_metrics|LANGUAGE plpgsql STABLE|seed_get_app_metrics_caches' supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sqlAlso applies to: 105-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql` around lines 17 - 18, The two get_app_metrics function overloads are declared STABLE but can call public.seed_get_app_metrics_caches(...) which upserts public.app_metrics_cache, so update both CREATE OR REPLACE FUNCTION public.get_app_metrics declarations to not promise read-only behavior: remove the STABLE qualifier (or replace it with VOLATILE) for both overloads so the planner doesn't assume no writes; ensure you change both occurrences that reference get_app_metrics and leave seed_get_app_metrics_caches and app_metrics_cache unchanged.
17-18:⚠️ Potential issue | 🟠 MajorRun the cache-reading overloads as
SECURITY DEFINER.If
public.app_metrics_cachestill has the deny-all RLS policy, theSELECT ... INTO cache_entryon Line 45 will never see cached rows for anon/authenticated callers because these functions are stillSECURITY INVOKER. That turns every lookup into a reseed path and defeats the cache. Keep the explicitpublic.check_min_rights(...)guard, but execute the cache read as the function owner.Suggested fix
-) LANGUAGE plpgsql STABLE +) LANGUAGE plpgsql +SECURITY DEFINER SET search_path TO '' AS $function$Apply the same change to both
public.get_app_metricsoverloads.#!/bin/bash # Verify current RLS around app_metrics_cache and the security mode of the recreated functions. rg -n --type sql -C4 'ON public\.app_metrics_cache|CREATE POLICY|ROW LEVEL SECURITY|Deny all' supabase/migrations rg -n --type sql -C2 'CREATE OR REPLACE FUNCTION public\.get_app_metrics|SECURITY DEFINER|SELECT \*' supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sqlAlso applies to: 45-55, 105-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql` around lines 17 - 18, The functions public.get_app_metrics (both overloads) are currently SECURITY INVOKER so RLS prevents cache reads; change each function to SECURITY DEFINER so the SELECT ... INTO cache_entry can see rows in public.app_metrics_cache while retaining the explicit public.check_min_rights(...) guard inside the function; update the CREATE OR REPLACE FUNCTION declarations for both overloads to include SECURITY DEFINER (ensuring the function owner is the intended role) and keep the rest of the body (cache read, reseed logic and check_min_rights call) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql`:
- Around line 18-86: The unqualified built-ins (NULLIF, current_setting, NOW,
jsonb_to_recordset) must be schema-qualified when search_path is set to empty;
update their usages in this function (and the other three functions mentioned)
to call pg_catalog.NULLIF, pg_catalog.current_setting, pg_catalog.now (or
pg_catalog.transaction_timestamp() if preferred), and
pg_catalog.jsonb_to_recordset respectively so the calls resolve correctly; keep
all references to public functions like public.seed_get_app_metrics_caches,
public.check_min_rights, and public.get_identity_org_allowed as-is and only
change the built-in function identifiers to their pg_catalog-qualified names.
---
Duplicate comments:
In `@supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql`:
- Around line 17-18: The two get_app_metrics function overloads are declared
STABLE but can call public.seed_get_app_metrics_caches(...) which upserts
public.app_metrics_cache, so update both CREATE OR REPLACE FUNCTION
public.get_app_metrics declarations to not promise read-only behavior: remove
the STABLE qualifier (or replace it with VOLATILE) for both overloads so the
planner doesn't assume no writes; ensure you change both occurrences that
reference get_app_metrics and leave seed_get_app_metrics_caches and
app_metrics_cache unchanged.
- Around line 17-18: The functions public.get_app_metrics (both overloads) are
currently SECURITY INVOKER so RLS prevents cache reads; change each function to
SECURITY DEFINER so the SELECT ... INTO cache_entry can see rows in
public.app_metrics_cache while retaining the explicit
public.check_min_rights(...) guard inside the function; update the CREATE OR
REPLACE FUNCTION declarations for both overloads to include SECURITY DEFINER
(ensuring the function owner is the intended role) and keep the rest of the body
(cache read, reseed logic and check_min_rights call) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b3c9131-5c49-4d0c-b359-71c64f9dd5b9
📒 Files selected for processing (1)
supabase/migrations/20260308121758_fix_get_app_global_metrics_rbac.sql
riderx
force-pushed
the
riderx/metrics-oracle-check
branch
from
c9dddee to
867410a
Compare
|
1 participant
Summary
This PR fixes cross-tenant disclosure risk by adding org-level read checks to all
get_app_metricsandget_global_metricsRPC overloads while keeping signatures unchanged.It also updates AGENTS.md to mandate CLI-only migration creation (
bunx supabase migration new) and single-migration iteration before merge.Test plan
bun lint.Screenshots
Checklist
bun run lint:backend && bun run lint.accordingly.
my tests
Summary by CodeRabbit
New Features
Documentation