Restrict org status RPCs to authenticated members#1751
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:
📝 WalkthroughWalkthroughAdds three SECURITY DEFINER PostgreSQL functions in public: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (anon / authenticated / service_role)
participant RPC as RPC (is_paying_org / is_trial_org / is_onboarding_needed_org)
participant Rights as public.check_min_rights
participant Orgs as public.orgs / public.stripe_info
Caller->>RPC: CALL <function>(orgid)
RPC->>Rights: check_min_rights(caller_role, orgid)
Rights-->>RPC: allowed / denied
alt allowed
RPC->>Orgs: SELECT org row -> customer_id, status, trial_at
Orgs-->>RPC: stripe fields / org presence
RPC-->>Caller: boolean / integer (days remaining) / onboarding boolean
else denied
RPC-->>Caller: false / 0 / false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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/20260313121928_fix-onboarding-needed-org-nonexistent.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: supabase/tests/20_test_org_management_functions.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: supabase/tests/06_org_functions.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 |
riderx
force-pushed
the
riderx/fix-rpc-billing-status
branch
from
c6c0200 to
bc00e88
Compare
![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
🤖 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/20260308203352_restrict-org-status-rpc-access.sql`:
- Around line 53-55: The SELECT in function public.is_trial_org (using
stripe_info.trial_at, stripe_info.customer_id and orgs.customer_id via orgid)
can return NULL when no stripe_info row or trial_at is NULL; change the return
expression to guarantee a non-null integer—wrap the computed GREATEST(...)
result in COALESCE(..., 0) or fetch into a local integer variable and return 0
when NULL—so the function always returns a non-null integer as declared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4b0ac10-fb4b-4a1b-bc7a-25a42c1fe259
📒 Files selected for processing (1)
supabase/migrations/20260308203352_restrict-org-status-rpc-access.sql
riderx
force-pushed
the
riderx/fix-rpc-billing-status
branch
from
e7eec85 to
f740ccb
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
supabase/tests/20_test_org_management_functions.sql (1)
276-277: Scope service-role auth to only the assertions that need it.
tests.authenticate_as_service_role()here appears to stay active for later sections. Add a nearbytests.clear_authentication()after the restricted RPC checks so downstream tests keep their intended context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/20_test_org_management_functions.sql` around lines 276 - 277, The call to tests.authenticate_as_service_role() is left active for subsequent tests; after the restricted RPC checks complete, call tests.clear_authentication() to restore normal auth context. Locate the block that runs tests.authenticate_as_service_role() before the restricted RPC assertions and insert a tests.clear_authentication() immediately after those assertions (i.e., after the restricted RPC checks) so downstream tests run with the intended context.
🤖 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/20260313121928_fix-onboarding-needed-org-nonexistent.sql`:
- Around line 1-17: This new migration duplicates work for the same feature;
remove this separate migration and fold the CREATE OR REPLACE FUNCTION
"public"."is_onboarding_needed_org" definition into the existing in-flight
migration for the feature so there's a single editable migration file,
preserving the function signature, LANGUAGE plpgsql, SECURITY DEFINER, SET
"search_path" = '' and the logic (EXISTS check against public.orgs, NOT
public.is_onboarded_org, and public.is_trial_org = 0); ensure the combined
migration keeps the same idempotent CREATE OR REPLACE FUNCTION statement and any
comments about avoiding org existence disclosure so ordering/drift is avoided.
- Around line 7-15: Revoke EXECUTE on the UUID-parameter functions
is_onboarding_needed_org(uuid), is_paying_org(uuid), and is_trial_org(uuid) from
anon and authenticated and make them service_role-only, and add new public
no-parameter wrapper functions (e.g., is_onboarding_needed_org(),
is_paying_org(), is_trial_org()) that perform caller identity/membership checks
and then call the internal uuid versions with the target org id; update usages
(such as the CHECK in the RETURN block that calls
is_onboarding_needed_org(is_onboarding_needed_org.orgid)) to call the new
wrapper where appropriate or, if the org id must be explicit, perform the
membership check in the wrapper and expose a safe public API that avoids
granting direct EXECUTE on the uuid-parameter functions.
---
Nitpick comments:
In `@supabase/tests/20_test_org_management_functions.sql`:
- Around line 276-277: The call to tests.authenticate_as_service_role() is left
active for subsequent tests; after the restricted RPC checks complete, call
tests.clear_authentication() to restore normal auth context. Locate the block
that runs tests.authenticate_as_service_role() before the restricted RPC
assertions and insert a tests.clear_authentication() immediately after those
assertions (i.e., after the restricted RPC checks) so downstream tests run with
the intended context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6a4f1da-531d-4a90-99c5-6d8e4a679fe6
📒 Files selected for processing (5)
supabase/migrations/20260308203352_restrict-org-status-rpc-access.sqlsupabase/migrations/20260313121928_fix-onboarding-needed-org-nonexistent.sqlsupabase/tests/06_org_functions.sqlsupabase/tests/20_test_org_management_functions.sqlsupabase/tests/46_test_org_status_rpcs.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- supabase/tests/06_org_functions.sql
- supabase/tests/46_test_org_status_rpcs.sql
| -- Keep onboarding-needed checks false for missing org IDs to avoid org existence disclosure. | ||
| CREATE OR REPLACE FUNCTION "public"."is_onboarding_needed_org"("orgid" "uuid") RETURNS boolean | ||
| LANGUAGE "plpgsql" SECURITY DEFINER | ||
| SET "search_path" = '' | ||
| AS $$ | ||
| BEGIN | ||
| RETURN ( | ||
| EXISTS ( | ||
| SELECT 1 FROM public.orgs | ||
| WHERE id = is_onboarding_needed_org.orgid | ||
| ) | ||
| AND | ||
| NOT public.is_onboarded_org(is_onboarding_needed_org.orgid) | ||
| AND public.is_trial_org(is_onboarding_needed_org.orgid) = 0 | ||
| ); | ||
| END; | ||
| $$; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Please fold this into the existing in-flight migration for this feature.
This introduces a second migration for the same feature scope; keep a single editable migration until ship to avoid split ordering and drift.
As per coding guidelines: "When a feature requires schema changes, create a single migration file with the 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/20260313121928_fix-onboarding-needed-org-nonexistent.sql`
around lines 1 - 17, This new migration duplicates work for the same feature;
remove this separate migration and fold the CREATE OR REPLACE FUNCTION
"public"."is_onboarding_needed_org" definition into the existing in-flight
migration for the feature so there's a single editable migration file,
preserving the function signature, LANGUAGE plpgsql, SECURITY DEFINER, SET
"search_path" = '' and the logic (EXISTS check against public.orgs, NOT
public.is_onboarded_org, and public.is_trial_org = 0); ensure the combined
migration keeps the same idempotent CREATE OR REPLACE FUNCTION statement and any
comments about avoiding org existence disclosure so ordering/drift is avoided.
| RETURN ( | ||
| EXISTS ( | ||
| SELECT 1 FROM public.orgs | ||
| WHERE id = is_onboarding_needed_org.orgid | ||
| ) | ||
| AND | ||
| NOT public.is_onboarded_org(is_onboarding_needed_org.orgid) | ||
| AND public.is_trial_org(is_onboarding_needed_org.orgid) = 0 | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find function + privilege statements for is_onboarding_needed_org:"
rg -n -C2 'CREATE OR REPLACE FUNCTION "public"\."is_onboarding_needed_org"|REVOKE ALL ON FUNCTION "public"\."is_onboarding_needed_org"|GRANT EXECUTE ON FUNCTION "public"\."is_onboarding_needed_org"' supabase/migrations
echo
echo "Find tests covering anon/non-member behavior for is_onboarding_needed_org:"
rg -n -C2 'is_onboarding_needed_org\(' supabase/testsRepository: Cap-go/capgo
Length of output: 2066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full function body for is_onboarding_needed_org (current) ==="
sed -n '2,15p' supabase/migrations/20260313121928_fix-onboarding-needed-org-nonexistent.sql
echo
echo "=== Check for GRANT/REVOKE statements on is_onboarding_needed_org ==="
rg -n 'GRANT|REVOKE' supabase/migrations/20260313121928_fix-onboarding-needed-org-nonexistent.sql
echo
echo "=== is_paying_org function definition and grants ==="
rg -n -A20 'CREATE OR REPLACE FUNCTION "public"\."is_paying_org"' supabase/migrations | head -50
rg -n 'GRANT.*is_paying_org|REVOKE.*is_paying_org' supabase/migrations
echo
echo "=== is_trial_org function definition and grants ==="
rg -n -A20 'CREATE OR REPLACE FUNCTION "public"\."is_trial_org"' supabase/migrations | head -50
rg -n 'GRANT.*is_trial_org|REVOKE.*is_trial_org' supabase/migrations
echo
echo "=== Check if wrapper functions exist (no-arg versions) ==="
rg -n 'CREATE OR REPLACE FUNCTION "public"\."is_onboarding_needed_org"\s*\(\s*\)' supabase/migrations
rg -n 'CREATE OR REPLACE FUNCTION "public"\."is_paying_org"\s*\(\s*\)' supabase/migrations
rg -n 'CREATE OR REPLACE FUNCTION "public"\."is_trial_org"\s*\(\s*\)' supabase/migrationsRepository: Cap-go/capgo
Length of output: 621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check base migration for GRANT/REVOKE on these functions ==="
rg -n 'GRANT.*is_onboarding_needed_org|GRANT.*is_paying_org|GRANT.*is_trial_org' supabase/migrations/20250530233128_base.sql
echo
echo "=== Full is_paying_org function (base) ==="
rg -n -A15 'CREATE OR REPLACE FUNCTION "public"\."is_paying_org"' supabase/migrations/20250530233128_base.sql | head -40
echo
echo "=== Full is_trial_org function (base) ==="
rg -n -A15 'CREATE OR REPLACE FUNCTION "public"\."is_trial_org"' supabase/migrations/20250530233128_base.sql | head -40
echo
echo "=== Full is_onboarded_org function (base) ==="
rg -n -A15 'CREATE OR REPLACE FUNCTION "public"\."is_onboarded_org"' supabase/migrations/20250530233128_base.sql | head -40
echo
echo "=== Check for check_min_rights usage in these functions ==="
rg -n -B5 -A20 'check_min_rights' supabase/migrations/20250530233128_base.sql | grep -A20 'is_paying_org\|is_trial_org\|is_onboarding_needed_org'Repository: Cap-go/capgo
Length of output: 2992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if wrapper functions (no-arg versions) exist ==="
rg -n 'CREATE OR REPLACE FUNCTION "public"\."is_onboarding_needed_org"\s*\(\s*\) RETURNS' supabase/migrations
rg -n 'CREATE OR REPLACE FUNCTION "public"\."is_paying_org"\s*\(\s*\) RETURNS' supabase/migrations
rg -n 'CREATE OR REPLACE FUNCTION "public"\."is_trial_org"\s*\(\s*\) RETURNS' supabase/migrations
echo
echo "=== Verify information disclosure risk: test if anon can call the uuid function ==="
rg -n -B3 -A10 'is_onboarding_needed_org.*anon\|anon.*is_onboarding_needed_org' supabase/testsRepository: Cap-go/capgo
Length of output: 114
Revoke EXECUTE on these uuid-parameter functions from anon and authenticated roles.
The is_onboarding_needed_org(uuid), is_paying_org(uuid), and is_trial_org(uuid) functions are currently GRANT ALL ... TO anon/authenticated, allowing any user to query org existence and trial status via boolean outcomes. These uuid-parameter functions should be service_role-only for internal lookups. Create public wrapper functions with no parameters that call the inner uuid versions; the wrappers perform caller identity/membership checks and become the public-facing API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260313121928_fix-onboarding-needed-org-nonexistent.sql`
around lines 7 - 15, Revoke EXECUTE on the UUID-parameter functions
is_onboarding_needed_org(uuid), is_paying_org(uuid), and is_trial_org(uuid) from
anon and authenticated and make them service_role-only, and add new public
no-parameter wrapper functions (e.g., is_onboarding_needed_org(),
is_paying_org(), is_trial_org()) that perform caller identity/membership checks
and then call the internal uuid versions with the target org id; update usages
(such as the CHECK in the RETURN block that calls
is_onboarding_needed_org(is_onboarding_needed_org.orgid)) to call the new
wrapper where appropriate or, if the org id must be explicit, perform the
membership check in the wrapper and expose a safe public API that avoids
granting direct EXECUTE on the uuid-parameter functions.
1 participant
Summary (AI generated)
is_onboarding_needed_orgSQL test expectations with current contract after recent org function hardening changes.Motivation (AI generated)
Latest CI failures showed an expectation mismatch for non-existent org handling in onboarding checks.
Business Impact (AI generated)
Ensures CI reliability and keeps SQL test coverage consistent with current function behavior, preventing false negatives during automated validation.
Test Plan (AI generated)
bun lint:backend.e7eec8533toriderx/fix-rpc-billing-status.Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Security
Tests