Conversation
|
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)
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}
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
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 | |||
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).
| 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); |
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.
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
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
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
# Conflicts: # supabase/functions/_backend/triggers/cron_clear_versions.ts
|
CI triage: I tested one migration-level fix locally and it did not hold: the suite still failed with 22 auth regressions, so I reverted that candidate instead of pushing something misleading. This branch needs a broader auth/permission pass on the affected plan/metrics RPCs rather than a small patch. |
|
CI still fails for the same service-role auth reason as the newer RPC-hardening branches. The |
|
Run tests fails in |
|
CI still fails on #1752 in Run tests. The failing device flow is still returning 404/429 instead of 200, and the broader log also shows shared backend issues like |
|
PR #1752: CI is failing in |
|
PR #1752 is failing in because RPC auth behavior no longer matches the expected anon and non-member access paths. That is a targeted regression, but it still needs deeper auth-policy review, so I did not try to paper over it with a quick patch. |
|
CI is still red on https://github.com/Cap-go/capgo/actions/runs/23273331981/job/67670747101. The main failure I isolated is |
|
CI is failing in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eac199b6d
ℹ️ 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".
| ) THEN | ||
| RETURN; |
There was a problem hiding this comment.
The auth hardening in 20260308121758_fix_get_app_global_metrics_rbac.sql changed get_app_metrics/get_global_metrics to raise NO_RIGHTS, and tests/plan-rpc-auth.test.ts now asserts that contract. This later migration runs afterwards and replaces the guard with a bare RETURN here (and again in get_global_metrics at lines 218-226), so a fresh supabase db reset still gives anon/non-member callers an empty result set instead of an auth error.
Useful? React with 👍 / 👎.
| CREATE OR REPLACE FUNCTION public.get_current_plan_name_org(orgid uuid) | ||
| RETURNS character varying | ||
| LANGUAGE plpgsql SECURITY DEFINER | ||
| SET | ||
| search_path = '' AS $$ |
There was a problem hiding this comment.
On a clean database these definitions are overwritten immediately by supabase/migrations/20260316220423_harden_plan_usage_org_rpc_access.sql, which recreates get_current_plan_name_org (lines 4-55), get_cycle_info_org (57-133), and both get_plan_usage_percent_detailed overloads (135-318). None of the new request.jwt.claim.role bypass or NO_RIGHTS behavior in this file survives the migration sequence, so the backend caller fix and the new auth assertions only work on manually patched databases, not on bootstrapped environments.
Useful? React with 👍 / 👎.
| const { data: deletedApp, error: deletedError } = await supabaseAdmin(c) | ||
| .from('deleted_apps') | ||
| .select('owner_org') | ||
| .eq('app_id', record.app_id) | ||
| .maybeSingle() |
There was a problem hiding this comment.
This fallback assumes deleted_apps has at most one row per app_id, but the schema only enforces uniqueness on (app_id, owner_org) (supabase/migrations/20250530233128_base.sql:3882). If an app id is reused by a different org, .maybeSingle() will start erroring with multiple matches and resolveOwnerOrg() falls back to null, so app_versions_meta upserts are skipped again for the deleted-app path this patch is trying to repair.
Useful? React with 👍 / 👎.
|
See PR #1752. I tried to harden the plan RPC auth path, but the focused suite still fails broadly: anon/non-member calls are not consistently raising |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec3e629d2e
ℹ️ 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".
| it.concurrent(`allows api key for ${rpc.label}`, async () => { | ||
| const client = getApiKeyClient(APIKEY_TEST_ALL) | ||
| const { error } = await client.rpc(rpc.name as never, rpc.params as never) | ||
| expect(error).toBeNull() |
There was a problem hiding this comment.
These new allows api key assertions will fail for several entries in rpcCases: PostgREST still executes capgkey requests as the anon role, but get_current_plan_max_org revokes anon in supabase/migrations/20260313104400_fix_get_current_plan_max_org_access_cli.sql:59-63, and get_current_plan_name_org, get_cycle_info_org, and both get_plan_usage_percent_detailed overloads do the same in supabase/migrations/20260316220423_harden_plan_usage_org_rpc_access.sql:50-53, 128-131, 236-239, and 332-335. In those cases client.rpc(...) returns permission denied for function ... before the new check_min_rights logic can run, so the API-key path added by this commit is still unusable.
Useful? React with 👍 / 👎.
|
CI is still blocked on #1752. The failing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30abb8b8d9
ℹ️ 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".
| ) THEN | ||
| RETURN NULL; | ||
| RAISE EXCEPTION 'NO_RIGHTS'; |
There was a problem hiding this comment.
get_current_plan_name_org used to degrade to NULL when the org was missing or no longer readable, which our client helpers convert to 'Solo' (src/services/supabase.ts) and several pages call without a surrounding try/catch (src/pages/settings/organization/Usage.vue, Plans.vue). Raising NO_RIGHTS here turns those same stale-org / membership-revoked cases into thrown exceptions, so a deleted org or lagging org switch can now break the settings UI and backend helpers instead of falling back to the existing default.
Useful? React with 👍 / 👎.
| REVOKE ALL ON FUNCTION public.get_current_plan_name_org(uuid) FROM PUBLIC; | ||
| REVOKE ALL ON FUNCTION public.get_current_plan_name_org(uuid) FROM anon; | ||
| GRANT EXECUTE ON FUNCTION public.get_current_plan_name_org(uuid) TO anon; |
There was a problem hiding this comment.
Granting anon execute on these plan RPCs changes anonymous calls from a 42501 permission denied failure into an in-function NO_RIGHTS error. After a clean reset, the existing suites still assert the old contract in tests/plan-usage-org-rpc-access.test.ts and supabase/tests/08_plan_functions.sql, so this commit leaves backend CI red even before the new plan-rpc-auth coverage runs.
Useful? React with 👍 / 👎.
|
PR: #1752 I checked the unresolved review threads here. The current blockers span migration ordering and behavior that is overwritten later in the migration chain, so a branch-local patch would not survive a clean reset. The auth/NO_RIGHTS contract and the plan-usage RPC fixes need to be moved into the last owning migration and reconciled with the current test expectations before this can be fixed safely. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0c9d1c1ef
ℹ️ 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".
| NULL::bigint | ||
| ) THEN | ||
| RETURN; | ||
| RAISE EXCEPTION 'NO_RIGHTS'; |
There was a problem hiding this comment.
get_cycle_info_org non-throwing for foreign orgs
Raising NO_RIGHTS here breaks get_orgs_v6(userid): that RPC still materializes SELECT public.get_cycle_info_org(o.id) AS f, o.* FROM public.orgs AS o before joining back to the caller’s memberships (supabase/migrations/20251222140030_rbac_system.sql:1937-1940). For any authenticated/API-key caller who is not a member of every org, the first inaccessible org now aborts the whole query, and /public/app/get already depends on rpc('get_orgs_v6') to list accessible orgs (supabase/functions/_backend/public/app/get.ts:79-84).
Useful? React with 👍 / 👎.
| RETURNS void AS $$ | ||
| BEGIN | ||
| perform set_config('role', 'anon', true); | ||
| EXECUTE 'SET LOCAL ROLE anon'; |
There was a problem hiding this comment.
clear_authentication
This changes the actual SQL role to anon, but tests.authenticate_as(...) still only updates JWT GUCs, not SET LOCAL ROLE authenticated. Existing pgTAP files like supabase/tests/07_auth_functions.sql:16-22 call clear_authentication(); authenticate_as('test_user'); is_platform_admin(), and is_platform_admin() is executable only by authenticated/service_role (supabase/migrations/20260311164503_split_is_admin_platform_admin_and_rls.sql:43-46). After this change those tests keep running as anon, so they start failing with permission denied and no longer exercise authenticated-role behavior.
Useful? React with 👍 / 👎.
|
CI is blocked by repo-wide lint failures in src/components/dashboard/InviteTeammateModal.vue:25 and src/pages/onboarding/organization.vue:613-614, which are unrelated to this branch. The unresolved review threads still cover plan RPC migration ordering, owner_org fallback, and authenticated-role behavior, so this still needs a broader migration-chain pass. #1752 |
|
CI is blocked by repo-wide lint failures in #1752. The failing run stops in () and (). I did not patch this branch because the failures are outside the PR diff and need a repo-wide lint fix or rebase. |
|
Correction: CI is blocked by repo-wide lint failures in #1752. The failing run stops in |
|
Status update for #1752: I checked the current CI and review state. This PR still has 11 unresolved review threads, and the latest That is not a safe quick fix in this pass, so I am leaving the branch unchanged rather than introducing a partial repair. |
|
Thanks for the contribution. This PR looks like a low-signal or template-style submission, which is hard to review and maintain. Please submit a focused PR with clear problem context, concrete diffs, and a short validation plan. |
1 participant
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