fix(db): bind public permission oracle subjects#2150
Closed
hieu-lee wants to merge 2 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR hardens the ChangesPermission Oracle Security Hardening
Possibly Related PRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
SpeedyArt
reviewed
May 11, 2026
SpeedyArt
left a comment
SpeedyArt
left a comment
There was a problem hiding this comment.
One residual logging concern on this security fix: the new public subject-binding denials still persist raw stable identifiers in pg_log payloads (user_id, auth_uid, and apikey_user_id on the mismatch branches).
Because check_min_rights remains callable by public roles, a mismatched request can now cause retained logs to contain both the requested user subject and the caller/API-key subject. For the same #1667-style cleanup that is moving user/org IDs to booleans/counts elsewhere, I would keep these diagnostics metadata-only: for example hasUserId, hasAuthUid, hasApikeySubject, subjectMatchesAuth, subjectMatchesApikey, plus the deny reason, rather than storing the raw UUIDs.
That should preserve the useful debugging signal for the oracle-binding fix without reintroducing linkable user identifiers into the retained denial logs.
![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: 3
🧹 Nitpick comments (2)
supabase/migrations/20260511024500_bind_permission_oracle_subjects.sql (1)
196-220: ⚡ Quick winAlso revoke from
anonas defensive hardening.The search found no evidence of prior direct
GRANT EXECUTE ... TO "anon"for either function in the migration chain. However, adding"anon"to theREVOKEstatement remains a reasonable defensive practice: if ananongrant existed (from external sources or historical upgrades),REVOKE ... FROM PUBLICwould not remove it. Including"anon"in the revocation is low-effort and closes that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260511024500_bind_permission_oracle_subjects.sql` around lines 196 - 220, Update the REVOKE statements for the functions rbac_check_permission_direct and rbac_check_permission_direct_no_password_policy to also revoke from the anon role (i.e., change FROM PUBLIC, "authenticated" to FROM PUBLIC, "authenticated", "anon") so any existing anon grants are explicitly removed as defensive hardening.supabase/tests/49_test_apikey_oracle_rpc_permissions.sql (1)
193-235: ⚡ Quick winConsider adding anon role checks for RBAC direct helpers to match the test coverage pattern.
The
check_min_rights_legacyhelper (lines 160-191) comprehensively tests all three roles (anon, authenticated, service_role), but the RBAC direct permission helpers only test authenticated and service_role. For consistency and to explicitly verify the PR's objective of removing public EXECUTE from all direct helpers, consider adding anon role checks for:
public.rbac_check_permission_directpublic.rbac_check_permission_direct_no_password_policyBoth should assert
falsefor the anon role.📋 Proposed additions for complete coverage
Add these assertions after line 159 (before the existing authenticated checks):
SELECT is( has_function_privilege( 'anon'::name, 'public.rbac_check_permission_direct(text, uuid, uuid, character varying, bigint, text)'::regprocedure, 'EXECUTE' ), false, 'anon role has no execute privilege on direct RBAC helper' ); SELECT is( has_function_privilege( 'anon'::name, 'public.rbac_check_permission_direct_no_password_policy(text, uuid, uuid, character varying, bigint, text)'::regprocedure, 'EXECUTE' ), false, 'anon role has no execute privilege on direct RBAC no-policy helper' );Then update the plan count from 38 to 40.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/tests/49_test_apikey_oracle_rpc_permissions.sql` around lines 193 - 235, Tests for the RBAC direct helper functions are missing anon-role assertions: add two assertions that verify has_function_privilege('anon', 'public.rbac_check_permission_direct(...)'::regprocedure, 'EXECUTE') returns false and has_function_privilege('anon', 'public.rbac_check_permission_direct_no_password_policy(...)'::regprocedure, 'EXECUTE') returns false (use the same argument signature as the existing authenticated checks) placed before the current authenticated-role checks, and increment the TAP plan count from 38 to 40; target the functions rbac_check_permission_direct and rbac_check_permission_direct_no_password_policy in your edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260511024500_bind_permission_oracle_subjects.sql`:
- Around line 3-169: Add the required execution-model and worst-case plan
evidence for the check_min_rights function: describe where it runs, expected
invocation frequency, effective roles (who calls it), cardinality assumptions,
and which indexes support lookups on public.apps.owner_org, public.channels.id
and public.orgs.id; then attach EXPLAIN (ANALYZE, BUFFERS) output for
representative worst-case executions (org-level, app-level, channel-level)
including calls that exercise app lookup, channel lookup and org
2FA/password-policy checks and the rbac paths (rbac_is_enabled_for_org,
rbac_check_permission_direct), and include notes about which inputs (user_id
NULL vs non-NULL, apikey present) produce the worst cost and why.
- Around line 75-108: The branch only validates v_apikey when user_id IS
DISTINCT FROM auth.uid(), allowing a request where auth.uid() == user_id but an
attached API key belongs to a different user to slip through; always validate
the API key subject. Move the SELECT FROM public.find_apikey_by_value(v_apikey)
(and the assignment to v_apikey_user_id) out of the user_id IS DISTINCT FROM
auth.uid() branch so it runs whenever v_apikey IS NOT NULL, and if
v_apikey_user_id IS NULL or v_apikey_user_id IS DISTINCT FROM user_id then
PERFORM the appropriate pg_log (reuse the
CHECK_MIN_RIGHTS_APIKEY_SUBJECT_MISMATCH message) and RETURN false; keep the
existing user-id-vs-auth.uid() logging/deny logic for non-apikey cases and
ensure rbac_check_permission_direct(..., v_apikey) will only be reached when the
apikey subject equals user_id.
- Around line 52-58: Reject mixed channel/org scopes early: before deriving
v_effective_org_id from channels (the IF block referencing v_effective_org_id
and channel_id and the SELECT from public.channels), add a guard that if
channel_id IS NOT NULL and (org_id IS NOT NULL OR app_id IS NOT NULL) then look
up the channel's owner_org (from public.channels where id = channel_id) and
compare it against the provided org_id (or the org owning the provided app_id);
if they differ, RAISE EXCEPTION / RETURN an error to reject the mixed-scope
request. Ensure the check runs prior to any later org-gated 2FA/password/RBAC
logic so authorization cannot mix org A's policies with channel B's membership.
---
Nitpick comments:
In `@supabase/migrations/20260511024500_bind_permission_oracle_subjects.sql`:
- Around line 196-220: Update the REVOKE statements for the functions
rbac_check_permission_direct and rbac_check_permission_direct_no_password_policy
to also revoke from the anon role (i.e., change FROM PUBLIC, "authenticated" to
FROM PUBLIC, "authenticated", "anon") so any existing anon grants are explicitly
removed as defensive hardening.
In `@supabase/tests/49_test_apikey_oracle_rpc_permissions.sql`:
- Around line 193-235: Tests for the RBAC direct helper functions are missing
anon-role assertions: add two assertions that verify
has_function_privilege('anon',
'public.rbac_check_permission_direct(...)'::regprocedure, 'EXECUTE') returns
false and has_function_privilege('anon',
'public.rbac_check_permission_direct_no_password_policy(...)'::regprocedure,
'EXECUTE') returns false (use the same argument signature as the existing
authenticated checks) placed before the current authenticated-role checks, and
increment the TAP plan count from 38 to 40; target the functions
rbac_check_permission_direct and rbac_check_permission_direct_no_password_policy
in your edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67417ab2-2085-4145-a908-e9cd40022a98
📒 Files selected for processing (4)
supabase/migrations/20260511024500_bind_permission_oracle_subjects.sqlsupabase/schemas/prod.sqlsupabase/tests/19_test_identity_functions.sqlsupabase/tests/49_test_apikey_oracle_rpc_permissions.sql
Comment on lines
+3
to
+169
| CREATE OR REPLACE FUNCTION "public"."check_min_rights"( | ||
| "min_right" "public"."user_min_right", | ||
| "user_id" "uuid", | ||
| "org_id" "uuid", | ||
| "app_id" character varying, | ||
| "channel_id" bigint | ||
| ) RETURNS boolean | ||
| LANGUAGE "plpgsql" SECURITY DEFINER | ||
| SET "search_path" TO '' | ||
| AS $$ | ||
| DECLARE | ||
| v_perm text; | ||
| v_scope text; | ||
| v_apikey text; | ||
| v_apikey_user_id uuid; | ||
| v_request_role text; | ||
| v_use_rbac boolean; | ||
| v_effective_org_id uuid := org_id; | ||
| v_app_owner_org uuid; | ||
| v_org_enforcing_2fa boolean; | ||
| v_password_policy_ok boolean; | ||
| BEGIN | ||
| -- Existing apps are always authorized in the app owner's org scope. | ||
| -- Keep nonexistent apps on the caller org so API handlers can still return their | ||
| -- own not-found errors after a valid org-level check. | ||
| IF app_id IS NOT NULL THEN | ||
| SELECT owner_org INTO v_app_owner_org | ||
| FROM public.apps | ||
| WHERE public.apps.app_id = check_min_rights.app_id | ||
| LIMIT 1; | ||
|
|
||
| IF v_app_owner_org IS NOT NULL THEN | ||
| IF v_effective_org_id IS NOT NULL AND v_effective_org_id IS DISTINCT FROM v_app_owner_org THEN | ||
| PERFORM public.pg_log('deny: CHECK_MIN_RIGHTS_APP_ORG_MISMATCH', jsonb_build_object( | ||
| 'has_org_id', v_effective_org_id IS NOT NULL, | ||
| 'has_app_owner_org', v_app_owner_org IS NOT NULL, | ||
| 'has_app_id', app_id IS NOT NULL, | ||
| 'has_channel_id', channel_id IS NOT NULL, | ||
| 'min_right', min_right::text, | ||
| 'has_user_id', user_id IS NOT NULL, | ||
| 'org_matches_app_owner', v_effective_org_id IS NOT DISTINCT FROM v_app_owner_org | ||
| )); | ||
| RETURN false; | ||
| END IF; | ||
|
|
||
| v_effective_org_id := v_app_owner_org; | ||
| END IF; | ||
| END IF; | ||
|
|
||
| -- Derive org from channel when not provided to honor org-level flag and scoping. | ||
| IF v_effective_org_id IS NULL AND channel_id IS NOT NULL THEN | ||
| SELECT owner_org INTO v_effective_org_id | ||
| FROM public.channels | ||
| WHERE public.channels.id = channel_id | ||
| LIMIT 1; | ||
| END IF; | ||
|
|
||
| SELECT public.get_apikey_header() INTO v_apikey; | ||
| v_request_role := public.current_request_role(); | ||
|
|
||
| IF v_request_role IN ('anon', 'authenticated') THEN | ||
| IF user_id IS NULL THEN | ||
| IF v_apikey IS NULL THEN | ||
| PERFORM public.pg_log('deny: CHECK_MIN_RIGHTS_UNBOUND_PUBLIC_SUBJECT', jsonb_build_object( | ||
| 'has_org_id', COALESCE(org_id, v_effective_org_id) IS NOT NULL, | ||
| 'has_app_id', app_id IS NOT NULL, | ||
| 'has_channel_id', channel_id IS NOT NULL, | ||
| 'min_right', min_right::text, | ||
| 'request_role', v_request_role | ||
| )); | ||
| RETURN false; | ||
| END IF; | ||
| ELSIF user_id IS DISTINCT FROM auth.uid() THEN | ||
| IF v_apikey IS NULL THEN | ||
| PERFORM public.pg_log('deny: CHECK_MIN_RIGHTS_PUBLIC_SUBJECT_MISMATCH', jsonb_build_object( | ||
| 'has_org_id', COALESCE(org_id, v_effective_org_id) IS NOT NULL, | ||
| 'has_app_id', app_id IS NOT NULL, | ||
| 'has_channel_id', channel_id IS NOT NULL, | ||
| 'min_right', min_right::text, | ||
| 'has_user_id', user_id IS NOT NULL, | ||
| 'has_auth_uid', auth.uid() IS NOT NULL, | ||
| 'subject_matches_auth', user_id IS NOT DISTINCT FROM auth.uid(), | ||
| 'request_role', v_request_role | ||
| )); | ||
| RETURN false; | ||
| END IF; | ||
|
|
||
| SELECT found_key.user_id INTO v_apikey_user_id | ||
| FROM public.find_apikey_by_value(v_apikey) AS found_key | ||
| LIMIT 1; | ||
|
|
||
| IF v_apikey_user_id IS NULL OR v_apikey_user_id IS DISTINCT FROM user_id THEN | ||
| PERFORM public.pg_log('deny: CHECK_MIN_RIGHTS_APIKEY_SUBJECT_MISMATCH', jsonb_build_object( | ||
| 'has_org_id', COALESCE(org_id, v_effective_org_id) IS NOT NULL, | ||
| 'has_app_id', app_id IS NOT NULL, | ||
| 'has_channel_id', channel_id IS NOT NULL, | ||
| 'min_right', min_right::text, | ||
| 'has_user_id', user_id IS NOT NULL, | ||
| 'has_auth_uid', auth.uid() IS NOT NULL, | ||
| 'request_role', v_request_role, | ||
| 'has_apikey_subject', v_apikey_user_id IS NOT NULL, | ||
| 'subject_matches_auth', user_id IS NOT DISTINCT FROM auth.uid(), | ||
| 'subject_matches_apikey', user_id IS NOT DISTINCT FROM v_apikey_user_id | ||
| )); | ||
| RETURN false; | ||
| END IF; | ||
| END IF; | ||
| END IF; | ||
|
|
||
| -- RBAC-managed API keys have apikeys.mode = NULL, so get_identity_org_appid() | ||
| -- returns NULL and rbac_check_permission_direct() must resolve the key before | ||
| -- org identity gates can be evaluated. | ||
| IF v_effective_org_id IS NOT NULL AND NOT (v_apikey IS NOT NULL AND user_id IS NULL) THEN | ||
| SELECT enforcing_2fa INTO v_org_enforcing_2fa | ||
| FROM public.orgs | ||
| WHERE id = v_effective_org_id; | ||
|
|
||
| IF v_org_enforcing_2fa = true AND (user_id IS NULL OR NOT public.has_2fa_enabled(user_id)) THEN | ||
| PERFORM public.pg_log('deny: CHECK_MIN_RIGHTS_2FA_ENFORCEMENT', jsonb_build_object( | ||
| 'has_org_id', COALESCE(org_id, v_effective_org_id) IS NOT NULL, | ||
| 'has_app_id', app_id IS NOT NULL, | ||
| 'has_channel_id', channel_id IS NOT NULL, | ||
| 'min_right', min_right::text, | ||
| 'has_user_id', user_id IS NOT NULL | ||
| )); | ||
| RETURN false; | ||
| END IF; | ||
|
|
||
| v_password_policy_ok := public.user_meets_password_policy(user_id, v_effective_org_id); | ||
| IF v_password_policy_ok = false THEN | ||
| PERFORM public.pg_log('deny: CHECK_MIN_RIGHTS_PASSWORD_POLICY_ENFORCEMENT', jsonb_build_object( | ||
| 'has_org_id', COALESCE(org_id, v_effective_org_id) IS NOT NULL, | ||
| 'has_app_id', app_id IS NOT NULL, | ||
| 'has_channel_id', channel_id IS NOT NULL, | ||
| 'min_right', min_right::text, | ||
| 'has_user_id', user_id IS NOT NULL | ||
| )); | ||
| RETURN false; | ||
| END IF; | ||
| END IF; | ||
|
|
||
| v_use_rbac := public.rbac_is_enabled_for_org(v_effective_org_id); | ||
| IF NOT v_use_rbac THEN | ||
| RETURN public.check_min_rights_legacy(min_right, user_id, COALESCE(org_id, v_effective_org_id), app_id, channel_id); | ||
| END IF; | ||
|
|
||
| IF channel_id IS NOT NULL THEN | ||
| v_scope := public.rbac_scope_channel(); | ||
| ELSIF app_id IS NOT NULL THEN | ||
| v_scope := public.rbac_scope_app(); | ||
| ELSE | ||
| v_scope := public.rbac_scope_org(); | ||
| END IF; | ||
|
|
||
| v_perm := public.rbac_permission_for_legacy(min_right, v_scope); | ||
|
|
||
| -- Keep RLS authorization semantics aligned with explicit RBAC checks. In | ||
| -- particular, an API key with direct role bindings must be evaluated as the | ||
| -- API-key principal and must not inherit broader owner-user permissions. | ||
| RETURN public.rbac_check_permission_direct( | ||
| v_perm, | ||
| user_id, | ||
| v_effective_org_id, | ||
| app_id, | ||
| channel_id, | ||
| v_apikey | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Please attach the required execution-model and plan evidence for this function.
This helper now adds multiple lookups (apps, channels, orgs) plus several security checks on an authorization path, but the PR notes only include the pgTAP plan count. Before merge, please add the execution model and worst-case EXPLAIN (ANALYZE, BUFFERS) for org/app/channel invocations.
As per coding guidelines, Every PostgreSQL function must be proven to scale before shipping; document execution model (where it runs, how often, which roles, cardinality, and indexes) and provide EXPLAIN (ANALYZE, BUFFERS) output for worst-case scenarios in PR notes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260511024500_bind_permission_oracle_subjects.sql`
around lines 3 - 169, Add the required execution-model and worst-case plan
evidence for the check_min_rights function: describe where it runs, expected
invocation frequency, effective roles (who calls it), cardinality assumptions,
and which indexes support lookups on public.apps.owner_org, public.channels.id
and public.orgs.id; then attach EXPLAIN (ANALYZE, BUFFERS) output for
representative worst-case executions (org-level, app-level, channel-level)
including calls that exercise app lookup, channel lookup and org
2FA/password-policy checks and the rbac paths (rbac_is_enabled_for_org,
rbac_check_permission_direct), and include notes about which inputs (user_id
NULL vs non-NULL, apikey present) produce the worst cost and why.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
|
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
2 participants
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
check_min_rights(min_right, user_id, ...)calls to the request JWT subject or to the user resolved from the requestcapgkeycheck_min_rightsgrant used by CLI/RLS app-list compatibility, including RBAC-managed API-key flows whereuser_idis intentionallyNULLcheck_min_rights_legacy,rbac_check_permission_direct, andrbac_check_permission_direct_no_password_policy/claim #1667
Motivation
The public SQL surface still exposed subject-taking permission helpers. An anonymous caller could ask whether an arbitrary known user UUID has a right on an org/app/channel through
check_min_rights, and an authenticated caller had direct execute on RBAC helper functions that accept an arbitraryp_user_id. Those helpers should remain available to trusted internal code and self-bound wrappers, but public calls should not act as membership or permission oracles for arbitrary subjects.Test Plan
git diff --check49_test_apikey_oracle_rpc_permissions.sqlhas 42 planned assertions and 42 assertion callsnpx eslint supabase/functions/_backend/public/organization/audit.tsChecklist