fix: harden remaining helper RPC authz#1810
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA migration hardens 11 public helper RPCs by removing anonymous/public EXECUTE, adding SECURITY DEFINER or STABLE definitions, and embedding caller-aware identity and rights checks (via Changes
Sequence DiagramsequenceDiagram
actor Client
participant RPC as Helper RPC (SECURITY DEFINER)
participant Identity as get_identity_org_allowed
participant Rights as check_min_rights
participant DB as Database
Client->>RPC: Call helper (e.g., is_paying_and_good_plan_org(org_id))
RPC->>RPC: Read caller context (current_setting('role') / session_user)
RPC->>Identity: Resolve identity / membership for caller & org
Identity->>DB: Query org, user_org_members, stripe_info, plans
DB-->>Identity: Identity record / org data
Identity-->>RPC: Identity result
RPC->>Rights: Check minimum rights for requested operation
Rights->>DB: Validate membership/role/ownership
DB-->>Rights: Rights decision
Rights-->>RPC: Authorization pass/fail
alt Authorization Passes
RPC->>DB: Run main logic (plans, dates, usage, storage sums)
DB-->>RPC: Result (boolean / numeric)
RPC-->>Client: Return result
else Authorization Fails
RPC-->>Client: NULL / access denied
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 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/tests/08_plan_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 |
![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: bde227a450
ℹ️ 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".
| SELECT public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], is_member_of_org.org_id) | ||
| INTO caller_id; | ||
|
|
||
| IF caller_id IS NULL OR caller_id <> is_member_of_org.user_id OR NOT public.check_min_rights( | ||
| 'read'::public.user_min_right, |
There was a problem hiding this comment.
is_member_of_org now derives the caller exclusively from get_identity_org_allowed(...) and rejects when that identity does not match user_id, which breaks existing anon RPC flows that pass API keys as function arguments instead of the capgkey header. In particular, public.get_org_owner_id(apikey, app_id) still computes real_user_id from its apikey parameter and calls is_member_of_org(real_user_id, org_id); for anonymous PostgREST requests without a capgkey header, this new guard returns false even for valid keys, causing NO_RIGHTS for previously valid calls.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
supabase/migrations/20260317020451_secure_remaining_helper_rpcs.sql (1)
447-452: Consider no-argument client wrappers for self-only RPCs.
get_user_main_org_id(uuid)andis_account_disabled(uuid)are self-only by behavior, but their client-facing signatures still accept arbitraryuuidinput. A()wrapper for client usage (internally usingauth.uid()) plus a service-onlyuuidoverload tightens API boundaries and removes unnecessary caller-controlled parameters.♻️ Suggested pattern
- GRANT EXECUTE ON FUNCTION "public"."get_user_main_org_id"("user_id" "uuid") TO "authenticated"; + GRANT EXECUTE ON FUNCTION "public"."get_user_main_org_id"() TO "authenticated"; + GRANT EXECUTE ON FUNCTION "public"."get_user_main_org_id"("user_id" "uuid") TO "service_role"; + CREATE OR REPLACE FUNCTION "public"."get_user_main_org_id"() RETURNS "uuid" + LANGUAGE "plpgsql" SECURITY DEFINER + SET "search_path" TO '' + AS $$ + BEGIN + RETURN public.get_user_main_org_id(auth.uid()); + END; + $$; - GRANT EXECUTE ON FUNCTION "public"."is_account_disabled"("user_id" "uuid") TO "authenticated"; + GRANT EXECUTE ON FUNCTION "public"."is_account_disabled"() TO "authenticated"; + GRANT EXECUTE ON FUNCTION "public"."is_account_disabled"("user_id" "uuid") TO "service_role"; + CREATE OR REPLACE FUNCTION "public"."is_account_disabled"() RETURNS boolean + LANGUAGE "plpgsql" SECURITY DEFINER + SET "search_path" TO '' + AS $$ + BEGIN + RETURN public.is_account_disabled(auth.uid()); + END; + $$;As per coding guidelines: "For PostgreSQL functions with admin/platform-RBAC checks, define one service-role-only
uuidoverload for internal lookups and one user-context()overload for client usage".Also applies to: 453-485, 537-542, 543-567
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260317020451_secure_remaining_helper_rpcs.sql` around lines 447 - 452, The RPCs get_user_main_org_id(uuid) and is_account_disabled(uuid) currently accept caller-controlled UUIDs but are intended to be self-only; add a no-argument client wrapper for each (e.g., get_user_main_org_id() and is_account_disabled()) that internally calls auth.uid() and delegates to the service-role uuid overload, and keep the existing uuid overload as service-role-only for internal lookups; then adjust grants so the no-arg overloads are EXECUTE-able by "authenticated" (and not by PUBLIC/anon) while the uuid overloads are EXECUTE-able only by "service_role" (revoking PUBLIC/anon as needed).supabase/tests/47_test_helper_rpc_authz.sql (1)
3-135: Add coverage for the remaining hardened RPCs to fully close the loop.This suite currently validates a subset of the hardened helpers. Please add explicit authz tests for
is_onboarded_org,is_onboarding_needed_org, andis_org_yearly(authorized, foreign-org, and anonymous contexts) so all newly hardened RPCs are regression-protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/47_test_helper_rpc_authz.sql` around lines 3 - 135, Add explicit authz tests for is_onboarded_org, is_onboarding_needed_org, and is_org_yearly by mirroring the existing patterns: for each RPC, authenticate as test_admin (tests.authenticate_as('test_admin')) and assert the admin can read the actual boolean/metric (use is(...) or ok(...) similar to is_canceled_org/get_total_storage_size_org), then authenticate as test_user and assert a foreign-org user receives the safe non-disclosing value (false or 0 as appropriate, like the is_canceled_org/get_total_storage_size_org checks), and finally clear authentication (tests.clear_authentication()) and assert anonymous calls are non-disclosing—either expect a safe default (false/0) or use throws_ok(...) with SQLSTATE '42501' if the function should be blocked; use tests.get_supabase_uid('test_admin') and the UUID '22dbad8a-b885-4309-9b3b-a09f8460fb6d' to target the org in each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/migrations/20260317020451_secure_remaining_helper_rpcs.sql`:
- Around line 447-452: The RPCs get_user_main_org_id(uuid) and
is_account_disabled(uuid) currently accept caller-controlled UUIDs but are
intended to be self-only; add a no-argument client wrapper for each (e.g.,
get_user_main_org_id() and is_account_disabled()) that internally calls
auth.uid() and delegates to the service-role uuid overload, and keep the
existing uuid overload as service-role-only for internal lookups; then adjust
grants so the no-arg overloads are EXECUTE-able by "authenticated" (and not by
PUBLIC/anon) while the uuid overloads are EXECUTE-able only by "service_role"
(revoking PUBLIC/anon as needed).
In `@supabase/tests/47_test_helper_rpc_authz.sql`:
- Around line 3-135: Add explicit authz tests for is_onboarded_org,
is_onboarding_needed_org, and is_org_yearly by mirroring the existing patterns:
for each RPC, authenticate as test_admin (tests.authenticate_as('test_admin'))
and assert the admin can read the actual boolean/metric (use is(...) or ok(...)
similar to is_canceled_org/get_total_storage_size_org), then authenticate as
test_user and assert a foreign-org user receives the safe non-disclosing value
(false or 0 as appropriate, like the is_canceled_org/get_total_storage_size_org
checks), and finally clear authentication (tests.clear_authentication()) and
assert anonymous calls are non-disclosing—either expect a safe default (false/0)
or use throws_ok(...) with SQLSTATE '42501' if the function should be blocked;
use tests.get_supabase_uid('test_admin') and the UUID
'22dbad8a-b885-4309-9b3b-a09f8460fb6d' to target the org in each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3801b595-e4cc-47a0-b4f0-ef2444e09171
📒 Files selected for processing (2)
supabase/migrations/20260317020451_secure_remaining_helper_rpcs.sqlsupabase/tests/47_test_helper_rpc_authz.sql
|
1 participant
Summary (AI generated)
Motivation (AI generated)
The oldest open security advisory in triage identified helper RPCs that still exposed org billing/storage state and user relationship signals without sufficient caller-aware authorization. This closes that remaining surface using the same authz pattern already adopted by earlier hardening migrations.
Business Impact (AI generated)
This reduces security risk around tenant isolation and billing metadata exposure, and it resolves an externally reported advisory with a validated patch. It also lowers the chance of follow-on exploit chains built from org-state and membership oracles.
Test Plan (AI generated)
bun run supabase:db:resetnode_modules/supabase/bin/supabase test db supabase/tests/00-supabase_test_helpers.sql supabase/tests/47_test_helper_rpc_authz.sql --workdir .context/supabase-worktrees/b74947c2bun run supabase:with-env -- bunx vitest run tests/upsert-version-meta-rpc.test.ts tests/build_time_tracking.test.ts tests/get-identity-apikey-only-rpc.test.tsbun run test:backendGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests