Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA database migration introduces two overloaded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
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 |
![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: c098cf8482
ℹ️ 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".
| v_user_id := public.get_identity('{read,upload,write,all}'::public.key_mode[]); | ||
| v_is_service_role := ( | ||
| ((SELECT auth.jwt() ->> 'role') = 'service_role') | ||
| OR ((SELECT current_user) IS NOT DISTINCT FROM 'postgres') |
There was a problem hiding this comment.
current_user bypass from RPC authorization
This function is SECURITY DEFINER and owned by postgres, so current_user resolves to postgres while it runs; that makes v_is_service_role always true and skips the check_min_rights authorization path for normal callers. As a result, any role with execute permission on get_org_members can query members for arbitrary org IDs, and the same current_user guard is repeated in the 2-argument overload below, leaving both entry points effectively unchecked.
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: 5
🧹 Nitpick comments (2)
supabase/migrations/20260224000000_fix_org_member_rpc_access.sql (2)
90-90: UseUNION ALL— the two result sets are disjoint and deduplication is needlessly expensive.The
org_usersrows (with realaidvalues) andtmp_usersrows (with syntheticaidvalues) can never produce identical tuples, soUNION's implicit dedup scan is pure overhead.♻️ Proposed fix
- UNION + UNION ALL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260224000000_fix_org_member_rpc_access.sql` at line 90, Replace the standalone "UNION" between the two SELECTs combining org_users and tmp_users with "UNION ALL" because the result sets are disjoint; update the SQL in the migration that constructs the combined result from org_users and tmp_users to use UNION ALL to avoid the unnecessary deduplication overhead.
131-141: Double identity resolution —get_identity()andget_identity_org_allowed()are called separately.
v_user_id(line 131) is resolved viaget_identity()and used only for theIS NULLshort-circuit, whilecheck_min_rights(line 141) re-resolves the identity viaget_identity_org_allowed(). If an org-scoped API key yields a different identity from the generic one, the NULL guard operates on a different UUID than the one passed tocheck_min_rights, creating a subtle mismatch. Passv_user_iddirectly tocheck_min_rightsand resolve identity only once usingget_identity_org_allowed().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260224000000_fix_org_member_rpc_access.sql` around lines 131 - 141, The code resolves identity twice causing a mismatch: replace the separate call to public.get_identity() with a single resolution using public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[], check_org_members_password_policy.org_id) and store that UUID in v_user_id, then pass v_user_id into public.check_min_rights(...) instead of re-calling get_identity_org_allowed(); ensure the NULL guard checks this same v_user_id and remove the duplicate identity call so get_identity_org_allowed is invoked only once.
🤖 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/20260224000000_fix_org_member_rpc_access.sql`:
- Line 93: The synthetic aid calculation using ((SELECT COALESCE(MAX(id), 0)
FROM public.org_users) + tmp.id)::bigint is non-deterministic and can collide;
change the approach in this migration/query to produce stable out-of-band IDs
instead of adding tmp.id to MAX(id). Replace the MAX-based formula for aid with
a stable scheme such as a large negative offset (e.g., -1 * tmp.id) or expose
tmp.id together with an is_tmp discriminator so callers use (tmp.id, is_tmp)
instead of treating aid as a persistent key; update any references to aid,
tmp.id, public.org_users/org_users.id and consumers that expect stable IDs
accordingly.
- Around line 111-112: The two-argument overload get_org_members("user_id" uuid,
"guild_id" uuid) is an internal implementation and must not be granted EXECUTE
to the authenticated role; remove the GRANT EXECUTE ... TO "authenticated" for
that overload and leave the GRANT for "service_role" intact, and ensure only the
wrapper/no-arg (and any one-arg) get_org_members wrapper functions that perform
auth checks are granted to "authenticated" instead; update the migration so only
the wrapper function names/overloads intended for end users receive EXECUTE for
"authenticated".
- Around line 137-148: Add a denial audit log call before the exception in
check_org_members_password_policy: when the branch that raises RAISE EXCEPTION
'NO_RIGHTS' is taken (inside check of v_is_service_role / v_user_id), call
public.pg_log with the same denial message and contextual parameters used by the
get_org_members overloads (e.g., 'deny: NO_RIGHTS', the requesting user id
variable v_user_id, the org id from check_org_members_password_policy.org_id,
the rpc endpoint name and/or 'rpc' context) immediately before raising the
exception so unauthorized attempts are recorded.
- Line 89: The condition AND public.is_member_of_org(users.id, o.org_id) is
redundant because the query already joins org_users o to users (users.id =
o.user_id) and filters o.org_id = get_org_members.guild_id; remove the
is_member_of_org(...) predicate so the membership check relies on the existing
join (refer to the is_member_of_org function, the org_users alias o, users table
and get_org_members.guild_id to locate the clause).
- Around line 20-23: The v_is_service_role check uses CURRENT_USER which, in
these SECURITY DEFINER functions, is always the function owner and makes auth
checks ineffective; update the v_is_service_role assignment to use SELECT
session_user instead of SELECT current_user in all three functions that declare
v_is_service_role (the blocks where v_is_service_role is computed and then used
in the IF NOT v_is_service_role checks). Also remove the GRANT EXECUTE ON
FUNCTION get_org_members(user_id, guild_id) TO authenticated so the inner
function with user-supplied parameters is not directly callable by authenticated
users. Ensure both changes are applied consistently across the migration.
---
Nitpick comments:
In `@supabase/migrations/20260224000000_fix_org_member_rpc_access.sql`:
- Line 90: Replace the standalone "UNION" between the two SELECTs combining
org_users and tmp_users with "UNION ALL" because the result sets are disjoint;
update the SQL in the migration that constructs the combined result from
org_users and tmp_users to use UNION ALL to avoid the unnecessary deduplication
overhead.
- Around line 131-141: The code resolves identity twice causing a mismatch:
replace the separate call to public.get_identity() with a single resolution
using
public.get_identity_org_allowed('{read,upload,write,all}'::public.key_mode[],
check_org_members_password_policy.org_id) and store that UUID in v_user_id, then
pass v_user_id into public.check_min_rights(...) instead of re-calling
get_identity_org_allowed(); ensure the NULL guard checks this same v_user_id and
remove the duplicate identity call so get_identity_org_allowed is invoked only
once.
supabase/migrations/20260224000000_fix_org_member_rpc_access.sql
Outdated
Show resolved
Hide resolved
supabase/migrations/20260224000000_fix_org_member_rpc_access.sql
Outdated
Show resolved
Hide resolved
supabase/migrations/20260224000000_fix_org_member_rpc_access.sql
Outdated
Show resolved
Hide resolved
|
1 participant
Summary (AI generated)
current_userchecks withsession_user-based service-role/admin bypass logic in all affected SECURITY DEFINER functions.get_org_members(user_id, guild_id)and made temporary invitationaidvalues stable.authenticatedfor the two-argument innerget_org_membersoverload.NO_RIGHTSaudit logging incheck_org_members_password_policybefore denial.Motivation (AI generated)
current_usersemantics under SECURITY DEFINER.Business Impact (AI generated)
Test Plan (AI generated)
bun lint:backend.sqlfluff lint --dialect postgres supabase/migrations/20260224000000_fix_org_member_rpc_access.sql(tooling run; reports repository-wide SQL style constraints for context).get_org_membersorcheck_org_members_password_policywithoutNO_RIGHTS.service_role/postgrespaths still function for internal jobs/administrative callers as intended.aidvalues and non-stable collision-prone arithmetic is removed.authenticatedcan only execute the one-argument wrapperget_org_memberspath, and not the inner(user_id, guild_id)overload.Generated with AI