Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces comprehensive org-scoped authentication checks for metrics and plan-related RPC endpoints through a database migration, adds owner_org fallback resolution logic in backend triggers, and provides extensive test coverage validating authentication enforcement across anonymous, authenticated, API key, and service role callers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as RPC Handler
participant Auth as JWT/Session
participant DB as Database
alt Service Role
Client->>RPC: Call RPC with service_role JWT
RPC->>Auth: Extract role from JWT claims
Auth-->>RPC: role = service_role
RPC->>DB: Execute query (bypass rights check)
DB-->>RPC: Return data
else Authenticated User
Client->>RPC: Call RPC with user JWT
RPC->>Auth: Extract role & session_user
Auth-->>RPC: role = authenticated
RPC->>DB: Check user rights via check_min_rights()
alt User has required rights
DB-->>RPC: Rights granted
RPC->>DB: Execute query
DB-->>RPC: Return data
else User lacks rights
DB-->>RPC: NO_RIGHTS error
RPC-->>Client: Return error
end
else Anonymous
Client->>RPC: Call RPC without auth
RPC->>Auth: No JWT/session detected
Auth-->>RPC: role = anon
RPC-->>Client: Return NO_RIGHTS error
end
RPC-->>Client: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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. 🔧 SQLFluff (4.0.4)supabase/migrations/20260309000518_fix-unauth-rpc-ci.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: 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: 1f574e90b5
ℹ️ 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".
| cycle_start timestamptz; | ||
| cycle_end timestamptz; | ||
| BEGIN | ||
| IF session_user <> 'service_role' AND NOT public.check_min_rights( |
There was a problem hiding this comment.
This guard relies on session_user to detect service-role bypass, but these RPCs are invoked from backend code through supabaseAdmin(...) (for example in supabase/functions/_backend/triggers/cron_email.ts and cron_stat_app.ts) where PostgREST auth is conveyed by JWT role claims, not by changing session_user. In that context the bypass does not trigger, get_identity_org_allowed(...) resolves to NULL (no auth.uid()/capgkey), check_min_rights(...) returns false, and the RPC raises NO_RIGHTS, breaking internal cron/statistics flows that call get_cycle_info_org/related functions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sql (1)
91-93: Qualify the ordinarypg_catalogcalls in these empty-search_pathfunctions.
search_path = ''is set correctly, but ordinary built-ins likeNOW(),date_trunc(), andjsonb_to_recordset()are still unqualified in the new bodies. Please prefix those withpg_catalog.for consistency with the repo SQL convention. As per coding guidelines, "Always set an emptysearch_path = ''in every PostgreSQL function and use fully qualified names for all references".Also applies to: 112-124, 292-297, 340-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql around lines 91 - 93, Several functions use an empty search_path but call unqualified built-ins (e.g., NOW(), date_trunc(), jsonb_to_recordset()); update each occurrence to use pg_catalog-qualified names so they work with search_path = ''. Find usages in the shown block (the conditional using NOW() and the assignment calling public.seed_get_app_metrics_caches) and the other mentioned blocks (lines 112-124, 292-297, 340-355) and replace NOW(), date_trunc(), jsonb_to_recordset() (and any other ordinary built-ins) with pg_catalog.NOW(), pg_catalog.date_trunc(), pg_catalog.jsonb_to_recordset() respectively, leaving calls to public.* functions (like public.seed_get_app_metrics_caches) 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/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql:
- Around line 22-30: The guard that checks for service-role is using
session_user which won't reflect PostgREST JWT role switching; update the
conditional checks to use current_user instead of session_user in all RPC auth
guards (e.g., the IF block that calls public.check_min_rights with
get_app_metrics and related checks), and make the same replacement for the other
occurrences referenced (lines/groups around 62-70, 145-153, 181-189, 219-227,
244-252, 278-286, 330-338, 399-407) so the service_role bypass works correctly
under PostgREST.
---
Nitpick comments:
In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql:
- Around line 91-93: Several functions use an empty search_path but call
unqualified built-ins (e.g., NOW(), date_trunc(), jsonb_to_recordset()); update
each occurrence to use pg_catalog-qualified names so they work with search_path
= ''. Find usages in the shown block (the conditional using NOW() and the
assignment calling public.seed_get_app_metrics_caches) and the other mentioned
blocks (lines 112-124, 292-297, 340-355) and replace NOW(), date_trunc(),
jsonb_to_recordset() (and any other ordinary built-ins) with pg_catalog.NOW(),
pg_catalog.date_trunc(), pg_catalog.jsonb_to_recordset() respectively, leaving
calls to public.* functions (like public.seed_get_app_metrics_caches) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a040bf5-2518-4a25-8c1e-ec200838dec8
📒 Files selected for processing (1)
supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sql
supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sql
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql:
- Line 1: The migration filename uses a nonstandard timestamp
("03082026,093259PM_fix-unauth-rpc-auth-checks.sql") which will break numeric
ordering; create a properly timestamped Supabase migration instead (use the CLI:
bunx supabase migration new <feature_slug> to generate a file named
YYYYMMDDHHMMSS_<slug>.sql), move the SQL from the existing file into that new
file, remove the old misnamed file, and continue editing the new CLI-generated
migration (keep a single migration file per feature as described).
- Around line 38-42: The cycle end is currently cast to date directly and
becomes inclusive, which pulls in the first day of the next cycle; update all
call sites that pass cycle_end as a date (e.g., where RETURN QUERY SELECT * FROM
public.get_app_metrics(org_id, cycle_start::date, cycle_end::date) and any other
calls using cycle_start/cycle_end) to subtract one day before casting—use either
(cycle_end::date - 1) or (cycle_end - INTERVAL '1 day')::date when passing the
end date so downstream BETWEEN p_start_date AND p_end_date remains correct;
locate uses by the variables cycle_start and cycle_end and the functions
get_cycle_info_org and get_app_metrics and apply the same change at each call
site mentioned in the review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8884db6-5ca0-470a-ba4b-5abf499d33bf
📒 Files selected for processing (1)
supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sql
| @@ -0,0 +1,492 @@ | |||
| -- Fix: add org-scoped auth checks on sensitive plan/usage RPCs and lock down cleanup RPC | |||
Contributor
There was a problem hiding this comment.
Use a normal Supabase migration timestamp here.
03082026,093259PM_... will not sort with the repo’s numeric migration sequence, so this change can be applied out of order on fresh environments. Please rename it to a standard CLI-generated YYYYMMDDHHMMSS_<slug>.sql migration before merge.
As per coding guidelines, "When a feature requires schema changes, create a single migration file with Supabase CLI (bunx supabase migration new <feature_slug>) and keep editing that file until the feature ships."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql at line
1, The migration filename uses a nonstandard timestamp
("03082026,093259PM_fix-unauth-rpc-auth-checks.sql") which will break numeric
ordering; create a properly timestamped Supabase migration instead (use the CLI:
bunx supabase migration new <feature_slug> to generate a file named
YYYYMMDDHHMMSS_<slug>.sql), move the SQL from the existing file into that new
file, remove the old misnamed file, and continue editing the new CLI-generated
migration (keep a single migration file per feature as described).
Comment on lines
+38
to
+42
| SELECT subscription_anchor_start, subscription_anchor_end | ||
| INTO cycle_start, cycle_end | ||
| FROM public.get_cycle_info_org(org_id); | ||
|
|
||
| RETURN QUERY SELECT * FROM public.get_app_metrics(org_id, cycle_start::date, cycle_end::date); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how downstream metric functions interpret end_date / cycle_end.
rg -n -C3 --glob '*.sql' \
"CREATE OR REPLACE FUNCTION public\.(get_total_metrics|seed_get_app_metrics_caches)|BETWEEN[[:space:]].*(end_date|cycle_end)|<=[[:space:]].*(end_date|cycle_end)|<[[:space:]].*(end_date|cycle_end)" \
supabase/migrationsRepository: Cap-go/capgo
Length of output: 10290
🏁 Script executed:
cat -n supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sql | head -50Repository: Cap-go/capgo
Length of output: 1782
🏁 Script executed:
rg -n "get_cycle_info_org" supabase/migrations --glob "*.sql" -A 15 | head -100Repository: Cap-go/capgo
Length of output: 10474
🏁 Script executed:
rg -n "get_app_metrics" supabase/migrations --glob "*.sql" -B 3 -A 10 | head -150Repository: Cap-go/capgo
Length of output: 14222
🏁 Script executed:
rg -n "CREATE.*FUNCTION.*get_cycle_info_org" supabase/migrations --glob "*.sql" -A 30Repository: Cap-go/capgo
Length of output: 6178
🏁 Script executed:
sed -n '177,178p; 403,411p' supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sqlRepository: Cap-go/capgo
Length of output: 551
Fix end-date boundary overflow in cycle calculations by subtracting one day before casting to date.
The get_cycle_info_org() function computes end_date := start_date + INTERVAL '1 MONTH', creating a timestamp at the start of the next cycle (e.g., 2026-04-01 00:00:00). This is semantically an exclusive boundary. However, casting to date (cycle_end::date) and passing to downstream functions that use BETWEEN p_start_date AND p_end_date (inclusive on both ends) causes metrics from the first day of the next cycle to be included in the current cycle.
Change to (cycle_end::date - 1) or (cycle_end - INTERVAL '1 day')::date at lines 42, 177-178, and 403-411.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql around
lines 38 - 42, The cycle end is currently cast to date directly and becomes
inclusive, which pulls in the first day of the next cycle; update all call sites
that pass cycle_end as a date (e.g., where RETURN QUERY SELECT * FROM
public.get_app_metrics(org_id, cycle_start::date, cycle_end::date) and any other
calls using cycle_start/cycle_end) to subtract one day before casting—use either
(cycle_end::date - 1) or (cycle_end - INTERVAL '1 day')::date when passing the
end date so downstream BETWEEN p_start_date AND p_end_date remains correct;
locate uses by the variables cycle_start and cycle_end and the functions
get_cycle_info_org and get_app_metrics and apply the same change at each call
site mentioned in the review.
Contributor
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 `@tests/plan-rpc-auth.test.ts`:
- Around line 102-115: The test matrix currently only checks anon vs
service_role; add two authenticated paths per rpc: call the RPC with an
authenticated non-member client (e.g., create or reuse a helper like
getAuthenticatedClient(nonMember) or a JWT-backed client) and assert the
returned error message contains rpc.anonError ?? 'NO_RIGHTS', and call the RPC
with an authenticated member/API-key client (e.g.,
getAuthenticatedClient(member) or an API-key-backed client) and assert error is
null; reuse rpc.label, rpc.name and rpc.params in both new it.concurrent tests
so each RPC is exercised for authenticated non-member (expect NO_RIGHTS) and
authenticated member/API-key (expect success).
- Around line 77-82: The test that calls delete_old_deleted_versions() mutates
shared DB state and must be isolated: remove that case from the it.concurrent()
matrix and create a new serial test (use it() not it.concurrent()) that seeds
its own test-owned rows, invokes delete_old_deleted_versions(), asserts
outcomes, and then cleans up those seeded rows; apply the same change for the
duplicate case referenced around lines 103-114 so both mutation tests use
dedicated seed data and run serially to avoid interfering with read-only
concurrent tests.
- Around line 6-7: Export the already-normalized SUPABASE_BASE_URL and
SUPABASE_ANON_KEY from test-utils.ts and stop hard-reading env vars in
tests/plan-rpc-auth.test.ts; specifically remove the duplicate
normalizeLocalhostUrl()/env.SUPABASE_URL logic and in getAnonClient() (and the
block around lines 85-99) import and use SUPABASE_BASE_URL and SUPABASE_ANON_KEY
from test-utils.ts so both getAnonClient() and getSupabaseClient() use the same
normalized base URL and anon key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7c5c842-7442-4441-aa87-ce06843d8f45
📒 Files selected for processing (1)
tests/plan-rpc-auth.test.ts
riderx
force-pushed
the
riderx/fix-unauth-rpc
branch
from
f015e83 to
725bf8c
Compare
riderx
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sql (2)
42-42:⚠️ Potential issue | 🟠 MajorFix cycle end boundary overflow.
The
cycle_endfromget_cycle_info_org()represents an exclusive boundary (start of next month), but casting to::dateand using withBETWEEN(inclusive) may include metrics from the next cycle's first day. Use(cycle_end::date - 1)or adjust downstream queries.This applies to all cycle_end usages at lines 42, 178, and 408.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql at line 42, The cycle_end value from get_cycle_info_org() is an exclusive boundary but is being cast to ::date and used inclusively, which can include the next cycle's first day; update all usages (e.g., the RETURN QUERY calling public.get_app_metrics and other places referencing cycle_end) to use an exclusive-to-inclusive conversion such as (cycle_end::date - 1) or change the downstream predicate to use a strict less-than (date < cycle_end::date) so that metrics for the next cycle’s first day are excluded.
1-1:⚠️ Potential issue | 🟠 MajorUse standard Supabase migration timestamp format.
The filename
03082026,093259PM_fix-unauth-rpc-auth-checks.sqlwon't sort correctly with the repo's numeric migration sequence. Rename toYYYYMMDDHHMMSS_<slug>.sqlformat using the Supabase CLI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql at line 1, The migration filename 03082026,093259PM_fix-unauth-rpc-auth-checks.sql uses a non-standard timestamp and will mis-sort; rename the file to Supabase's numeric migration format YYYYMMDDHHMMSS_<slug>.sql (e.g., generate a proper timestamped name) and create the migration via the Supabase CLI so the filename and internal migration metadata match; update any references to the old name if present and ensure the new filename retains the descriptive slug like fix-unauth-rpc-auth-checks.tests/plan-rpc-auth.test.ts (2)
102-115:⚠️ Potential issue | 🟠 MajorAdd authenticated non-member/member test paths.
The current matrix only tests "anon is denied" and "service_role is allowed". It doesn't cover JWT-backed
authenticatedcallers (API keys and signed-in users). Add at least one authenticated non-member case expectingNO_RIGHTSand one authorized member case expecting success.Based on learnings: "API keys authenticate through the
authenticatedrole via JWT, not theanonrole."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plan-rpc-auth.test.ts` around lines 102 - 115, Extend the existing RPC auth matrix to cover JWT-backed "authenticated" callers by adding two extra concurrent tests for each rpc in the loop: one that uses an authenticated non-member client (create a JWT-backed client instead of getAnonClient(), e.g., a helper that returns a client authenticated via API key or a signed-in user) and asserts the error message contains rpc.anonError ?? 'NO_RIGHTS', and another that uses an authenticated member client (create or sign-in a test user and ensure they are a member for this RPC) and asserts the RPC error is null; use the same rpc.name and rpc.params in the calls and reuse getSupabaseClient() semantics for service-role comparisons when implementing the authenticated helpers.
77-82:⚠️ Potential issue | 🟠 MajorIsolate
delete_old_deleted_versionsfrom concurrent test matrix.This case mutates shared DB state but runs under
it.concurrent()alongside read-only RPCs. This makes the suite order-dependent. Move this RPC to a separate serial test with dedicated seed data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plan-rpc-auth.test.ts` around lines 77 - 82, The test entry for the RPC labeled 'delete_old_deleted_versions' mutates shared DB state and must be removed from the concurrent test matrix and placed into its own serial test with dedicated seed data; remove the object with name 'delete_old_deleted_versions' from the test matrix (where the list of RPC cases is defined) and add a new standalone test (use it(...) not it.concurrent(...)) that seeds the DB with the specific state needed, invokes the RPC 'delete_old_deleted_versions', asserts the expected anonError 'permission denied' (or other expectations), and ensures DB cleanup/reset before/after the test so it cannot affect other concurrent read-only RPC tests.
🧹 Nitpick comments (2)
supabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sql (1)
23-26: Consider using consistent service-role detection across migrations.This migration uses a simpler detection pattern (
auth.jwt() ->> 'role'+session_user) compared to the more comprehensive pattern in20260309000518_fix-unauth-rpc-ci.sqlwhich also checkscurrent_setting('request.jwt.claim.role')andauth.role(). For consistency and robustness, consider aligning these patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql around lines 23 - 26, The v_is_service_role assignment uses a limited detection pattern; update the expression that sets v_is_service_role to match the more robust approach used elsewhere by checking auth.jwt() ->> 'role', current_setting('request.jwt.claim.role', true), and auth.role() in addition to session_user (and treating nulls safely), so replace the existing OR conditions in the v_is_service_role calculation with a combined check that mirrors the logic in 20260309000518_fix-unauth-rpc-ci.sql (use auth.jwt(), current_setting('request.jwt.claim.role', true), and auth.role() plus the session_user comparison) to ensure consistent service-role detection across migrations.tests/on-version-update-owner-org-fallback.test.ts (1)
21-50: Useit.concurrent()for parallel test execution.Per coding guidelines, tests should use
it.concurrent()instead ofit()to maximize parallelism. Since this test creates unique seed data withrandomUUID(), it's safe to run concurrently.♻️ Proposed change
- it('uses deleted_apps fallback when apps lookup is missing owner_org', async () => { + it.concurrent('uses deleted_apps fallback when apps lookup is missing owner_org', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/on-version-update-owner-org-fallback.test.ts` around lines 21 - 50, The test currently uses it(...) which prevents parallel execution; update the test declaration in tests/on-version-update-owner-org-fallback.test.ts to use it.concurrent('uses deleted_apps fallback when apps lookup is missing owner_org', async () => { ... }) so Jest will run it in parallel — ensure the async callback and use of randomUUID()-seeded data (appId/versionId) remain unchanged and that any setup/teardown in this test (executeSQL inserts/deletes) is still isolated.
🤖 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/functions/_backend/triggers/cron_clear_versions.ts`:
- Line 9: Add a missing type import for Context used by the function
resolveOwnerOrg: import the Context type from the Hono package (use a type-only
import like in on_version_update.ts) near the top alongside other imports so the
signature async function resolveOwnerOrg(c: Context, appId: string | null):
Promise<string | null> compiles without TS2304.
In `@supabase/migrations/20260309000518_fix-unauth-rpc-ci.sql`:
- Around line 43-47: The downstream call to public.get_app_metrics is passing
cycle_end as an inclusive date which pulls metrics from the next cycle; update
the RETURN QUERY call that currently uses cycle_start::date, cycle_end::date to
use cycle_start::date, (cycle_end::date - 1) so the exclusive boundary from
public.get_cycle_info_org(org_id) is preserved, and make the same adjustment at
the other occurrence later in the file (the second call that passes
p_start_date/p_end_date into generate_series or get_app_metrics around the later
block referenced in the comment).
---
Duplicate comments:
In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql:
- Line 42: The cycle_end value from get_cycle_info_org() is an exclusive
boundary but is being cast to ::date and used inclusively, which can include the
next cycle's first day; update all usages (e.g., the RETURN QUERY calling
public.get_app_metrics and other places referencing cycle_end) to use an
exclusive-to-inclusive conversion such as (cycle_end::date - 1) or change the
downstream predicate to use a strict less-than (date < cycle_end::date) so that
metrics for the next cycle’s first day are excluded.
- Line 1: The migration filename
03082026,093259PM_fix-unauth-rpc-auth-checks.sql uses a non-standard timestamp
and will mis-sort; rename the file to Supabase's numeric migration format
YYYYMMDDHHMMSS_<slug>.sql (e.g., generate a proper timestamped name) and create
the migration via the Supabase CLI so the filename and internal migration
metadata match; update any references to the old name if present and ensure the
new filename retains the descriptive slug like fix-unauth-rpc-auth-checks.
In `@tests/plan-rpc-auth.test.ts`:
- Around line 102-115: Extend the existing RPC auth matrix to cover JWT-backed
"authenticated" callers by adding two extra concurrent tests for each rpc in the
loop: one that uses an authenticated non-member client (create a JWT-backed
client instead of getAnonClient(), e.g., a helper that returns a client
authenticated via API key or a signed-in user) and asserts the error message
contains rpc.anonError ?? 'NO_RIGHTS', and another that uses an authenticated
member client (create or sign-in a test user and ensure they are a member for
this RPC) and asserts the RPC error is null; use the same rpc.name and
rpc.params in the calls and reuse getSupabaseClient() semantics for service-role
comparisons when implementing the authenticated helpers.
- Around line 77-82: The test entry for the RPC labeled
'delete_old_deleted_versions' mutates shared DB state and must be removed from
the concurrent test matrix and placed into its own serial test with dedicated
seed data; remove the object with name 'delete_old_deleted_versions' from the
test matrix (where the list of RPC cases is defined) and add a new standalone
test (use it(...) not it.concurrent(...)) that seeds the DB with the specific
state needed, invokes the RPC 'delete_old_deleted_versions', asserts the
expected anonError 'permission denied' (or other expectations), and ensures DB
cleanup/reset before/after the test so it cannot affect other concurrent
read-only RPC tests.
---
Nitpick comments:
In `@supabase/migrations/03082026`,093259PM_fix-unauth-rpc-auth-checks.sql:
- Around line 23-26: The v_is_service_role assignment uses a limited detection
pattern; update the expression that sets v_is_service_role to match the more
robust approach used elsewhere by checking auth.jwt() ->> 'role',
current_setting('request.jwt.claim.role', true), and auth.role() in addition to
session_user (and treating nulls safely), so replace the existing OR conditions
in the v_is_service_role calculation with a combined check that mirrors the
logic in 20260309000518_fix-unauth-rpc-ci.sql (use auth.jwt(),
current_setting('request.jwt.claim.role', true), and auth.role() plus the
session_user comparison) to ensure consistent service-role detection across
migrations.
In `@tests/on-version-update-owner-org-fallback.test.ts`:
- Around line 21-50: The test currently uses it(...) which prevents parallel
execution; update the test declaration in
tests/on-version-update-owner-org-fallback.test.ts to use it.concurrent('uses
deleted_apps fallback when apps lookup is missing owner_org', async () => { ...
}) so Jest will run it in parallel — ensure the async callback and use of
randomUUID()-seeded data (appId/versionId) remain unchanged and that any
setup/teardown in this test (executeSQL inserts/deletes) is still isolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8adb8040-3b6f-4856-9db0-c27140a4930c
📒 Files selected for processing (6)
supabase/functions/_backend/triggers/cron_clear_versions.tssupabase/functions/_backend/triggers/on_version_update.tssupabase/migrations/03082026,093259PM_fix-unauth-rpc-auth-checks.sqlsupabase/migrations/20260309000518_fix-unauth-rpc-ci.sqltests/on-version-update-owner-org-fallback.test.tstests/plan-rpc-auth.test.ts
riderx
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/20260309000518_fix-unauth-rpc-ci.sql`:
- Around line 1-529: This new migration duplicates the same feature changes
already present in the existing migration
20260308000000_fix_metrics_plan_rpcs_auth; fold its SQL for functions like
get_app_metrics, get_global_metrics, get_current_plan_max_org,
get_current_plan_name_org, get_cycle_info_org, get_plan_usage_percent_detailed
and the REVOKE/GRANT for delete_old_deleted_versions into that single migration
instead of adding 20260309000518_fix-unauth-rpc-ci.sql; to fix, remove the extra
migration file and copy/merge the new PL/pgSQL blocks (and their SECURITY
DEFINER/permission changes and service-role JWT checks) into the existing
migration file, resolving any duplicates so each function is declared once and
preserving all auth checks and GRANT/REVOKE statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d6ce880-ef64-4b27-a135-7b2049e1f43c
📒 Files selected for processing (4)
supabase/functions/_backend/triggers/cron_clear_versions.tssupabase/migrations/20260309000518_fix-unauth-rpc-ci.sqltests/plan-rpc-auth.test.tstests/test-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/plan-rpc-auth.test.ts
Comment on lines
+1
to
+529
| -- Fix: add org-scoped auth checks on sensitive plan/usage RPCs and lock down cleanup RPC | ||
| -- Add service-role detection by JWT role claim for backend caller contexts. | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_app_metrics(org_id uuid) | ||
| RETURNS TABLE ( | ||
| app_id character varying, | ||
| date date, | ||
| mau bigint, | ||
| storage bigint, | ||
| bandwidth bigint, | ||
| build_time_unit bigint, | ||
| get bigint, | ||
| fail bigint, | ||
| install bigint, | ||
| uninstall bigint | ||
| ) LANGUAGE plpgsql STABLE | ||
| SET | ||
| search_path = '' AS $$ | ||
| DECLARE | ||
| cycle_start timestamptz; | ||
| cycle_end timestamptz; | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_app_metrics.org_id), | ||
| get_app_metrics.org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| SELECT subscription_anchor_start, subscription_anchor_end | ||
| INTO cycle_start, cycle_end | ||
| FROM public.get_cycle_info_org(org_id); | ||
|
|
||
| RETURN QUERY SELECT * FROM public.get_app_metrics(org_id, cycle_start::date, (cycle_end::date - 1)); | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_app_metrics( | ||
| org_id uuid, | ||
| start_date date, | ||
| end_date date | ||
| ) RETURNS TABLE ( | ||
| app_id character varying, | ||
| date date, | ||
| mau bigint, | ||
| storage bigint, | ||
| bandwidth bigint, | ||
| build_time_unit bigint, | ||
| get bigint, | ||
| fail bigint, | ||
| install bigint, | ||
| uninstall bigint | ||
| ) LANGUAGE plpgsql SECURITY DEFINER | ||
| SET | ||
| search_path = '' AS $$ | ||
| DECLARE | ||
| cache_entry public.app_metrics_cache%ROWTYPE; | ||
| org_exists boolean; | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_app_metrics.org_id), | ||
| get_app_metrics.org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| SELECT EXISTS ( | ||
| SELECT 1 | ||
| FROM public.orgs | ||
| WHERE id = get_app_metrics.org_id | ||
| ) INTO org_exists; | ||
|
|
||
| IF NOT org_exists THEN | ||
| RETURN; | ||
| END IF; | ||
|
|
||
| SELECT * | ||
| INTO cache_entry | ||
| FROM public.app_metrics_cache | ||
| WHERE org_id = get_app_metrics.org_id; | ||
|
|
||
| IF cache_entry.id IS NULL | ||
| OR cache_entry.start_date IS DISTINCT FROM get_app_metrics.start_date | ||
| OR cache_entry.end_date IS DISTINCT FROM get_app_metrics.end_date | ||
| OR cache_entry.cached_at IS NULL | ||
| OR cache_entry.cached_at < (NOW() - interval '5 minutes') | ||
| THEN | ||
| cache_entry := public.seed_get_app_metrics_caches(get_app_metrics.org_id, get_app_metrics.start_date, get_app_metrics.end_date); | ||
| END IF; | ||
|
|
||
| IF cache_entry.response IS NULL THEN | ||
| RETURN; | ||
| END IF; | ||
|
|
||
| RETURN QUERY | ||
| SELECT | ||
| metrics.app_id, | ||
| metrics.date, | ||
| metrics.mau, | ||
| metrics.storage, | ||
| metrics.bandwidth, | ||
| metrics.build_time_unit, | ||
| metrics.get, | ||
| metrics.fail, | ||
| metrics.install, | ||
| metrics.uninstall | ||
| FROM jsonb_to_recordset(cache_entry.response) AS metrics( | ||
| app_id character varying, | ||
| date date, | ||
| mau bigint, | ||
| storage bigint, | ||
| bandwidth bigint, | ||
| build_time_unit bigint, | ||
| get bigint, | ||
| fail bigint, | ||
| install bigint, | ||
| uninstall bigint | ||
| ) | ||
| ORDER BY metrics.app_id, metrics.date; | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_global_metrics(org_id uuid) | ||
| RETURNS TABLE ( | ||
| date date, | ||
| mau bigint, | ||
| storage bigint, | ||
| bandwidth bigint, | ||
| get bigint, | ||
| fail bigint, | ||
| install bigint, | ||
| uninstall bigint | ||
| ) LANGUAGE plpgsql | ||
| SET | ||
| search_path = '' AS $$ | ||
| DECLARE | ||
| cycle_start timestamptz; | ||
| cycle_end timestamptz; | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_global_metrics.org_id), | ||
| get_global_metrics.org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| SELECT subscription_anchor_start, subscription_anchor_end | ||
| INTO cycle_start, cycle_end | ||
| FROM public.get_cycle_info_org(org_id); | ||
|
|
||
| RETURN QUERY | ||
| SELECT * FROM public.get_global_metrics(org_id, cycle_start::date, (cycle_end::date - 1)); | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_global_metrics( | ||
| org_id uuid, | ||
| start_date date, | ||
| end_date date | ||
| ) RETURNS TABLE ( | ||
| date date, | ||
| mau bigint, | ||
| storage bigint, | ||
| bandwidth bigint, | ||
| get bigint, | ||
| fail bigint, | ||
| install bigint, | ||
| uninstall bigint | ||
| ) LANGUAGE plpgsql | ||
| SET | ||
| search_path = '' AS $$ | ||
| DECLARE | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_global_metrics.org_id), | ||
| get_global_metrics.org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| RETURN QUERY | ||
| SELECT | ||
| metrics.date, | ||
| SUM(metrics.mau)::bigint AS mau, | ||
| SUM(metrics.storage)::bigint AS storage, | ||
| SUM(metrics.bandwidth)::bigint AS bandwidth, | ||
| SUM(metrics.get)::bigint AS get, | ||
| SUM(metrics.fail)::bigint AS fail, | ||
| SUM(metrics.install)::bigint AS install, | ||
| SUM(metrics.uninstall)::bigint AS uninstall | ||
| FROM | ||
| public.get_app_metrics(org_id, start_date, end_date) AS metrics | ||
| GROUP BY | ||
| metrics.date | ||
| ORDER BY | ||
| metrics.date; | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_current_plan_max_org(orgid uuid) | ||
| RETURNS TABLE ( | ||
| mau bigint, | ||
| bandwidth bigint, | ||
| storage bigint, | ||
| build_time_unit bigint | ||
| ) LANGUAGE plpgsql STABLE SECURITY DEFINER | ||
| SET search_path = '' AS $$ | ||
| DECLARE | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_current_plan_max_org.orgid), | ||
| get_current_plan_max_org.orgid, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| RETURN QUERY | ||
| SELECT p.mau, p.bandwidth, p.storage, p.build_time_unit | ||
| FROM public.orgs o | ||
| JOIN public.stripe_info si ON o.customer_id = si.customer_id | ||
| JOIN public.plans p ON si.product_id = p.stripe_id | ||
| WHERE o.id = orgid; | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_current_plan_name_org(orgid uuid) | ||
| RETURNS character varying | ||
| LANGUAGE plpgsql | ||
| SET | ||
| search_path = '' AS $$ | ||
| DECLARE | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_current_plan_name_org.orgid), | ||
| get_current_plan_name_org.orgid, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| RETURN | ||
| (SELECT name | ||
| FROM public.plans | ||
| WHERE stripe_id=(SELECT product_id | ||
| FROM public.stripe_info | ||
| WHERE customer_id=(SELECT customer_id FROM public.orgs WHERE id=orgid) | ||
| )); | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_cycle_info_org(orgid uuid) | ||
| RETURNS TABLE ( | ||
| subscription_anchor_start timestamp with time zone, | ||
| subscription_anchor_end timestamp with time zone | ||
| ) LANGUAGE plpgsql | ||
| SET | ||
| search_path = '' AS $$ | ||
| DECLARE | ||
| customer_id_var text; | ||
| stripe_info_row public.stripe_info%ROWTYPE; | ||
| anchor_day INTERVAL; | ||
| start_date timestamp with time zone; | ||
| end_date timestamp with time zone; | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_cycle_info_org.orgid), | ||
| get_cycle_info_org.orgid, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| SELECT customer_id INTO customer_id_var FROM public.orgs WHERE id = orgid; | ||
|
|
||
| SELECT * INTO stripe_info_row FROM public.stripe_info WHERE customer_id = customer_id_var; | ||
|
|
||
| anchor_day := COALESCE(stripe_info_row.subscription_anchor_start - date_trunc('MONTH', stripe_info_row.subscription_anchor_start), '0 DAYS'::INTERVAL); | ||
|
|
||
| IF anchor_day > NOW() - date_trunc('MONTH', NOW()) THEN | ||
| start_date := date_trunc('MONTH', NOW() - INTERVAL '1 MONTH') + anchor_day; | ||
| ELSE | ||
| start_date := date_trunc('MONTH', NOW()) + anchor_day; | ||
| END IF; | ||
|
|
||
| end_date := start_date + INTERVAL '1 MONTH'; | ||
|
|
||
| RETURN QUERY | ||
| SELECT start_date, end_date; | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_plan_usage_percent_detailed(orgid uuid) | ||
| RETURNS TABLE ( | ||
| total_percent double precision, | ||
| mau_percent double precision, | ||
| bandwidth_percent double precision, | ||
| storage_percent double precision, | ||
| build_time_percent double precision | ||
| ) LANGUAGE plpgsql STABLE SECURITY DEFINER | ||
| SET search_path = '' AS $$ | ||
| DECLARE | ||
| v_start_date date; | ||
| v_end_date date; | ||
| v_plan_mau bigint; | ||
| v_plan_bandwidth bigint; | ||
| v_plan_storage bigint; | ||
| v_plan_build_time bigint; | ||
| v_anchor_day INTERVAL; | ||
| total_stats RECORD; | ||
| percent_mau double precision; | ||
| percent_bandwidth double precision; | ||
| percent_storage double precision; | ||
| percent_build_time double precision; | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_plan_usage_percent_detailed.orgid), | ||
| get_plan_usage_percent_detailed.orgid, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| SELECT | ||
| COALESCE(si.subscription_anchor_start - date_trunc('MONTH', si.subscription_anchor_start), '0 DAYS'::INTERVAL), | ||
| p.mau, | ||
| p.bandwidth, | ||
| p.storage, | ||
| p.build_time_unit | ||
| INTO v_anchor_day, v_plan_mau, v_plan_bandwidth, v_plan_storage, v_plan_build_time | ||
| FROM public.orgs o | ||
| LEFT JOIN public.stripe_info si ON o.customer_id = si.customer_id | ||
| LEFT JOIN public.plans p ON si.product_id = p.stripe_id | ||
| WHERE o.id = orgid; | ||
|
|
||
| IF v_anchor_day > NOW() - date_trunc('MONTH', NOW()) THEN | ||
| v_start_date := (date_trunc('MONTH', NOW() - INTERVAL '1 MONTH') + v_anchor_day)::date; | ||
| ELSE | ||
| v_start_date := (date_trunc('MONTH', NOW()) + v_anchor_day)::date; | ||
| END IF; | ||
| v_end_date := (v_start_date + INTERVAL '1 MONTH')::date; | ||
|
|
||
| SELECT * INTO total_stats | ||
| FROM public.get_total_metrics(orgid, v_start_date, v_end_date); | ||
|
|
||
| percent_mau := public.convert_number_to_percent(total_stats.mau, v_plan_mau); | ||
| percent_bandwidth := public.convert_number_to_percent(total_stats.bandwidth, v_plan_bandwidth); | ||
| percent_storage := public.convert_number_to_percent(total_stats.storage, v_plan_storage); | ||
| percent_build_time := public.convert_number_to_percent(total_stats.build_time_unit, v_plan_build_time); | ||
|
|
||
| RETURN QUERY SELECT | ||
| GREATEST(percent_mau, percent_bandwidth, percent_storage, percent_build_time), | ||
| percent_mau, | ||
| percent_bandwidth, | ||
| percent_storage, | ||
| percent_build_time; | ||
| END; | ||
| $$; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.get_plan_usage_percent_detailed( | ||
| orgid uuid, | ||
| cycle_start date, | ||
| cycle_end date | ||
| ) RETURNS TABLE ( | ||
| total_percent double precision, | ||
| mau_percent double precision, | ||
| bandwidth_percent double precision, | ||
| storage_percent double precision, | ||
| build_time_percent double precision | ||
| ) LANGUAGE plpgsql STABLE SECURITY DEFINER | ||
| SET search_path = '' AS $$ | ||
| DECLARE | ||
| v_plan_mau bigint; | ||
| v_plan_bandwidth bigint; | ||
| v_plan_storage bigint; | ||
| v_plan_build_time bigint; | ||
| total_stats RECORD; | ||
| percent_mau double precision; | ||
| percent_bandwidth double precision; | ||
| percent_storage double precision; | ||
| percent_build_time double precision; | ||
| v_is_service_role boolean; | ||
| v_request_role text; | ||
| BEGIN | ||
| v_request_role := NULLIF(current_setting('request.jwt.claim.role', true), ''); | ||
| v_is_service_role := ( | ||
| (v_request_role = 'service_role') | ||
| OR ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT auth.role()) = 'service_role') | ||
| OR ((SELECT session_user) IS NOT DISTINCT FROM 'postgres') | ||
| ); | ||
|
|
||
| IF NOT v_is_service_role AND NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, | ||
| public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], get_plan_usage_percent_detailed.orgid), | ||
| get_plan_usage_percent_detailed.orgid, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) THEN | ||
| RAISE EXCEPTION 'NO_RIGHTS'; | ||
| END IF; | ||
|
|
||
| SELECT p.mau, p.bandwidth, p.storage, p.build_time_unit | ||
| INTO v_plan_mau, v_plan_bandwidth, v_plan_storage, v_plan_build_time | ||
| FROM public.orgs o | ||
| JOIN public.stripe_info si ON o.customer_id = si.customer_id | ||
| JOIN public.plans p ON si.product_id = p.stripe_id | ||
| WHERE o.id = orgid; | ||
|
|
||
| SELECT * INTO total_stats | ||
| FROM public.get_total_metrics(orgid, cycle_start, cycle_end); | ||
|
|
||
| percent_mau := public.convert_number_to_percent(total_stats.mau, v_plan_mau); | ||
| percent_bandwidth := public.convert_number_to_percent(total_stats.bandwidth, v_plan_bandwidth); | ||
| percent_storage := public.convert_number_to_percent(total_stats.storage, v_plan_storage); | ||
| percent_build_time := public.convert_number_to_percent(total_stats.build_time_unit, v_plan_build_time); | ||
|
|
||
| RETURN QUERY SELECT | ||
| GREATEST(percent_mau, percent_bandwidth, percent_storage, percent_build_time), | ||
| percent_mau, | ||
| percent_bandwidth, | ||
| percent_storage, | ||
| percent_build_time; | ||
| END; | ||
| $$; | ||
|
|
||
| REVOKE EXECUTE ON FUNCTION public.delete_old_deleted_versions() FROM public; | ||
| REVOKE EXECUTE ON FUNCTION public.delete_old_deleted_versions() FROM anon, authenticated; | ||
| GRANT EXECUTE ON FUNCTION public.delete_old_deleted_versions() TO service_role; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fold this into the existing PR migration.
The PR already has 20260308000000_fix_metrics_plan_rpcs_auth.sql, so adding 20260309000518_fix-unauth-rpc-ci.sql creates a second unshipped migration for the same feature. Please merge this delta back into the existing migration before release.
As per coding guidelines, "When a feature requires schema changes, create a single migration file with Supabase CLI (bunx supabase migration new <feature_slug>) and keep editing that file until the feature ships; never edit previously committed migrations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260309000518_fix-unauth-rpc-ci.sql` around lines 1 -
529, This new migration duplicates the same feature changes already present in
the existing migration 20260308000000_fix_metrics_plan_rpcs_auth; fold its SQL
for functions like get_app_metrics, get_global_metrics,
get_current_plan_max_org, get_current_plan_name_org, get_cycle_info_org,
get_plan_usage_percent_detailed and the REVOKE/GRANT for
delete_old_deleted_versions into that single migration instead of adding
20260309000518_fix-unauth-rpc-ci.sql; to fix, remove the extra migration file
and copy/merge the new PL/pgSQL blocks (and their SECURITY DEFINER/permission
changes and service-role JWT checks) into the existing migration file, resolving
any duplicates so each function is declared once and preserving all auth checks
and GRANT/REVOKE statements.
|
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.
Summary (AI generated)
Secured all sensitive plan/usage RPC paths with org-aware rights checks plus service-role compatible bypass logic, fixed cycle-end boundary handling for metrics windows, and removed the old malformed migration filename in favor of a CLI-style migration name.
Migrating fallback handling was updated so
on_version_updateandcron_clear_versionsresolveowner_orgfrom deleted apps when missing before upsertingapp_versions_meta.Cleanup rights were tightened by revoking
delete_old_deleted_versionsfrom public roles and granting it only toservice_role.Tests now reuse normalized Supabase constants from
test-utils.ts, cover anon/authenticated/service role/api-key access for RPCs, and isolate the mutating cleanup RPC with dedicated fixture data.Test plan (AI generated)
bun lint,bun lint:backend, andbunx eslint tests/plan-rpc-auth.test.tsand confirmed they passed after fixes.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests