fix: enforce org scope on organization members endpoint#1866
fix: enforce org scope on organization members endpoint#1866
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a policy-aware API-key authorization gate to the organization members GET handler: the handler now computes an effective API key (preferring request auth), checks org policy via a Supabase client before creating the RLS-backed client, and surfaces a specific expiring-key error path when applicable. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Func as MembersHandler
participant Policy as ApiKeyPolicyChecker
participant SBPolicy as Supabase (policy lookup)
participant SB as Supabase (RLS RPC)
Client->>Func: GET /organization/members (apikey header / auth)
Func->>Func: derive effectiveApikey = auth?.apikey ?? apikey
Func->>SBPolicy: create client with effectiveApikey.key
Func->>Policy: apikeyHasOrgRightWithPolicy(effectiveApikey, orgId)
alt policy valid
Func->>SB: create RLS client with effectiveApikey.key
Func->>SB: call get_org_members RPC (user_id = effectiveApikey.user_id)
SB-->>Func: members data
Func-->>Client: 200 + members
else requires expiring key
Policy-->>Func: error = org_requires_expiring_key
Func-->>Client: 401 org_requires_expiring_key
else policy invalid/other
Policy-->>Func: invalid
Func-->>Client: 400 cannot_access_organization
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 7485addb1c
ℹ️ 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".
![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)
tests/organization-api.test.ts (1)
225-234: Pin the bypass precondition in this regression.This still passes if
USER_IDever loses access toORG_ID, which would turn it into a generic “non-member gets 400” test instead of the scoped-key bypass case. Add a control request that proves the same principal can readORG_IDwithout thelimited_to_orgsrestriction before asserting the scoped key is blocked.Suggested tweak
it.concurrent('rejects GET /organization/members outside limited_to_orgs scope', async () => { + const ownerResponse = await fetch(`${BASE_URL}/organization/members?orgId=${ORG_ID}`, { + headers, + method: 'GET', + }) + expect(ownerResponse.status).toBe(200) + const response = await fetch(`${BASE_URL}/organization/members?orgId=${ORG_ID}`, { headers: { ...readOnlyHeaders, capgkey: readOnlyKey }, method: 'GET', })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/organization-api.test.ts` around lines 225 - 234, Add a preceding control request that proves the same principal can read ORG_ID without the scoped key: before the existing scoped-key fetch, call fetch(`${BASE_URL}/organization/members?orgId=${ORG_ID}`) using the principal's normal auth headers (i.e., omit the capgkey/readOnlyKey or use the non-limited headers used elsewhere in tests), assert a successful status (e.g., 200) and that the payload contains expected members; then keep the current scoped-key request (headers: { ...readOnlyHeaders, capgkey: readOnlyKey }) and assert it returns 400 with error 'cannot_access_organization' so the test proves the failure is due to limited_to_orgs scope rather than a general loss of access.
🤖 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/public/organization/members/get.ts`:
- Around line 47-53: The org-policy check currently treats missing/errored
policy lookups as success; update the lookup logic in checkApikeyMeetsOrgPolicy
(and any helper used by apikeyHasOrgRightWithPolicy) so that if the DB query
returns no rows or an error occurs it returns { valid: false, error:
'org_policy_lookup_failed' } (or similar) instead of { valid: true }; ensure
apikeyHasOrgRightWithPolicy surfaces that failure so the caller (the
get_org_members flow) will throw the 401/deny branch rather than allowing access
when the policy lookup fails.
---
Nitpick comments:
In `@tests/organization-api.test.ts`:
- Around line 225-234: Add a preceding control request that proves the same
principal can read ORG_ID without the scoped key: before the existing scoped-key
fetch, call fetch(`${BASE_URL}/organization/members?orgId=${ORG_ID}`) using the
principal's normal auth headers (i.e., omit the capgkey/readOnlyKey or use the
non-limited headers used elsewhere in tests), assert a successful status (e.g.,
200) and that the payload contains expected members; then keep the current
scoped-key request (headers: { ...readOnlyHeaders, capgkey: readOnlyKey }) and
assert it returns 400 with error 'cannot_access_organization' so the test proves
the failure is due to limited_to_orgs scope rather than a general loss of
access.
🪄 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: 9ef516b6-dad4-4ef3-8c6a-3aa677f62a2d
📒 Files selected for processing (2)
supabase/functions/_backend/public/organization/members/get.tstests/organization-api.test.ts
|
Pushed fixes for org members scope enforcement and policy lookup fail-closed, plus added a control test for unrestricted access. Commit: e7e9042. Test run: PR: #1866 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/public/organization/members/get.ts (1)
58-62: Inconsistent apikey reference in RPC call.The policy check and Supabase client use
effectiveApikey, but the RPC call usesapikey.user_id(the original parameter). While these should be identical when set by middleware, using different references is inconsistent and could cause subtle issues if the code evolves.♻️ Suggested fix for consistency
const { data, error } = await supabase .rpc('get_org_members', { - user_id: apikey.user_id, + user_id: effectiveApikey.user_id, guild_id: body.orgId, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/organization/members/get.ts` around lines 58 - 62, The RPC call to get_org_members is using apikey.user_id while the rest of the function and policy checks use effectiveApikey; change the RPC parameter to use effectiveApikey.user_id for consistency (update the call on supabase.rpc('get_org_members', { user_id: ..., guild_id: body.orgId }) to pass effectiveApikey.user_id) so all checks and the Supabase call reference the same apikey object.
🤖 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/functions/_backend/public/organization/members/get.ts`:
- Around line 58-62: The RPC call to get_org_members is using apikey.user_id
while the rest of the function and policy checks use effectiveApikey; change the
RPC parameter to use effectiveApikey.user_id for consistency (update the call on
supabase.rpc('get_org_members', { user_id: ..., guild_id: body.orgId }) to pass
effectiveApikey.user_id) so all checks and the Supabase call reference the same
apikey object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cf59f02-fb4e-4b34-b22b-ff9c9daef450
📒 Files selected for processing (3)
supabase/functions/_backend/public/organization/members/get.tssupabase/functions/_backend/utils/supabase.tstests/organization-api.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/organization-api.test.ts
|
PR #1866 I pushed a one-line fix in supabase/functions/_backend/public/organization/members/get.ts so get_org_members uses effectiveApikey.user_id. The remaining CI failure is in tests/audit-logs.test.ts (expected 200 vs 400 and changed_fields missing comment), which looks unrelated to the org-members diff and needs a separate audit-logs investigation. |
|
1 participant
Summary (AI generated)
limited_to_orgs+ org API key policy checks onGET /organization/members/organizationscope enforcement behaviorMotivation (AI generated)
A draft security advisory showed that an org-limited API key could still read another organization's member list when the key owner had broader access. The endpoint already used RBAC, but it skipped the explicit API-key org-scope check used by sibling handlers.
Business Impact (AI generated)
This closes an authorization bypass that exposed organization membership metadata across org boundaries, which improves safe API key delegation and reduces customer-facing security risk.
Test Plan (AI generated)
bun run lint:backendbun run supabase:with-env -- bunx vitest run tests/organization-api.test.tsbun run test:backendGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests