fix(api): harden API key management auth#2403
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:
📝 WalkthroughWalkthroughReplaces legacy key-mode/min-rights with RBAC-only checks. Updates middleware, handlers, SQL policies/migrations, generated types, seeds, CLI permission flow, frontend role handling, and tests/docs. Centralizes API-key management with JWT-only operations and ownership scoping. Removes obsolete functions/enums and aligns RLS assertions. ChangesEnd-to-end RBAC unification across services
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
2d98712 to
0ceea8a
Compare
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
0ceea8a to
7489bf9
Compare
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
7489bf9 to
15a2742
Compare
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
15a2742 to
0f07372
Compare
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
0f07372 to
6d7e2b2
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/tests/26_test_rls_policies.sql (1)
377-393: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert the new deny policies are actually restrictive.
This only proves the three policy names exist. If a future migration recreates any of them as
PERMISSIVE, the test still passes while the deny becomes ineffective against the existing owner allow-policies. Please addpg_policiesassertions forpermissive = 'RESTRICTIVE'and the expectedroles/cmdon these three new policies.As per coding guidelines: Add explicit deny policies for operations that must be impossible for user-facing roles, using RESTRICTIVE policies instead of relying on implicit deny.
🤖 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/26_test_rls_policies.sql` around lines 377 - 393, The test currently only checks that policy names exist for table apikeys via policies_are but does not verify they are restrictive; add assertions that query pg_policies for the three deny policies (e.g., 'Deny anon delete on apikeys', 'Deny anon select on apikeys', 'Deny anon update on apikeys') and assert permissive = 'RESTRICTIVE' and that the role(s) and cmd columns match the expected values for each policy; locate this near the existing policies_are call for apikeys and add one assertion per deny policy checking pg_policies.permissive, pg_policies.roles and pg_policies.cmd to ensure the denies are actually restrictive.
🤖 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/functions/_backend/public/apikey/auth.ts`:
- Around line 18-21: The code reads auth.authType without guarding for missing
auth; change the declaration to allow undefined (e.g., const auth =
c.get('auth') as AuthInfo | undefined) and update the guard to check for missing
auth first (if (!auth || auth.authType !== 'jwt' || !auth.userId) { ... }), and
when building the quickError payload use optional chaining for the authType
field (auth?.authType) so a missing auth produces the intended 401 instead of a
500; keep the same quickError call, errorCode, action and moreInfo variables.
In `@tests/apikeys-expiration.test.ts`:
- Around line 807-812: The test block removed an endpoint-level check, leaving
only direct Supabase/RLS reads (expectApiKeyCannotReadBaseOrg and
expectApiKeyCanReadBaseOrg); restore one HTTP-path assertion against the /apikey
endpoint in this block so middleware/header-parsing is exercised—add a single
assertion that the expiredKeyValue is rejected when calling the /apikey HTTP
route (alongside the RLS helper) and keep the validKeyValue HTTP-path check in
the other test block, referencing the existing helpers for RLS reads and the
/apikey route to locate where to add it.
In `@tests/apikeys.test.ts`:
- Around line 1030-1077: The test "plain key cannot update apikeys table
directly through RLS" leaves the created API key if an assertion fails; wrap the
cleanup DELETE call in a finally block so the seeded key is always removed.
Specifically, after creating the key (createResponse / createData), move the
fetch DELETE for `/apikey/${createData.id}` into a finally section that runs
regardless of test success, keeping the existing authHeaders and preserving the
rest of the assertions in the try block; ensure createData.id is available in
the finally (declare it in the outer scope if needed) so cleanup always
executes.
---
Outside diff comments:
In `@supabase/tests/26_test_rls_policies.sql`:
- Around line 377-393: The test currently only checks that policy names exist
for table apikeys via policies_are but does not verify they are restrictive; add
assertions that query pg_policies for the three deny policies (e.g., 'Deny anon
delete on apikeys', 'Deny anon select on apikeys', 'Deny anon update on
apikeys') and assert permissive = 'RESTRICTIVE' and that the role(s) and cmd
columns match the expected values for each policy; locate this near the existing
policies_are call for apikeys and add one assertion per deny policy checking
pg_policies.permissive, pg_policies.roles and pg_policies.cmd to ensure the
denies are actually restrictive.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d385eca-b762-452d-a898-93a462ad60c4
📒 Files selected for processing (10)
cli/src/types/supabase.types.tssupabase/functions/_backend/public/apikey/auth.tssupabase/functions/_backend/public/apikey/delete.tssupabase/functions/_backend/public/apikey/get.tssupabase/functions/_backend/public/apikey/put.tssupabase/functions/_backend/public/apikey/scope.tssupabase/migrations/20260603120805_deny_apikey_management_from_api_key_auth.sqlsupabase/tests/26_test_rls_policies.sqltests/apikeys-expiration.test.tstests/apikeys.test.ts
💤 Files with no reviewable changes (1)
- supabase/functions/_backend/public/apikey/scope.ts
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
6d7e2b2 to
ed6303b
Compare
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
ed6303b to
a8fc38e
Compare
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
a8fc38e to
3991392
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cli/src/types/supabase.types.ts`:
- Around line 3037-3045: The RPC type for app_versions_has_app_permission
incorrectly requires both p_apikey and p_user_id as non-null strings; update the
SQL RPC signature so the inactive auth parameter is nullable (make p_apikey OR
p_user_id NULLABLE/optional in the function definition for
app_versions_has_app_permission), deploy the migration, then regenerate the
TypeScript types (run the project type generation command, e.g. `bun types`) so
the generated supabase.types.ts reflects p_apikey and p_user_id as
nullable/optional and callers no longer need unsafe casts or dummy values.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81ad7474-945a-4192-9133-1c57dfd028ca
📒 Files selected for processing (10)
cli/src/types/supabase.types.tssupabase/functions/_backend/public/apikey/auth.tssupabase/functions/_backend/public/apikey/delete.tssupabase/functions/_backend/public/apikey/get.tssupabase/functions/_backend/public/apikey/put.tssupabase/functions/_backend/public/apikey/scope.tssupabase/migrations/20260603120805_deny_apikey_management_from_api_key_auth.sqlsupabase/tests/26_test_rls_policies.sqltests/apikeys-expiration.test.tstests/apikeys.test.ts
💤 Files with no reviewable changes (1)
- supabase/functions/_backend/public/apikey/scope.ts
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
3991392 to
d1b7c51
Compare
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
d1b7c51 to
fea72cb
Compare
![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: fea72cb875
ℹ️ 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".
riderx
force-pushed
the
codex/rbac-apikey-management-hardening
branch
from
fea72cb to
507af66
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Hostile Review — RBAC + API-key hardeningAdversarial multi-engine review (CodeRabbit + Codex + a 10-agent Claude reviewer fleet), merged and deduped. Base:
🔴 Critical1. Privilege escalation via direct
|
| # | Location | Issue |
|---|---|---|
| 5 | public/organization/audit.ts:~65 |
Gates on org.delete, but org.read_audit exists (rbac.ts:43) and the RLS function audit_logs_allowed_orgs() uses rbac_perm_org_read_audit(). A read_audit-only auditor is blocked at the API; a delete-only caller passes the API but gets RLS-filtered to empty. Contract mismatch in both directions — use org.read_audit. |
| 6 | …190328…sql org_users backfill CASE |
The org-scope branch maps invite_super_admin / invite_admin, but the app_id / channel_id branches omit invite_write / invite_upload, so those users fall through to app_reader / channel_reader (silent downgrade for anyone mid-invite at app/channel scope). |
| 7 | public/apikey/get.ts:~46–79 |
List/get use supabaseAdmin(c) + .select('*') for API-key callers, returning the key column (plaintext for non-hashed keys) in the response — defeating the new anon-deny RLS policy. Scoped to the caller's own keys (.eq('user_id', …)), so not cross-user, but key material should be masked/omitted from responses. |
| 8 | cli/src/bundle/upload.ts:~1530 |
When the key lacks channel RBAC permission, the code only log.warns and then proceeds to emit "App Uploaded" and report success. Server-side denial is correct, but the CLI silently turns "upload + set channel" into "upload only" — --channel users believe the channel was updated. |
| 9 | public/apikey/scope.ts:~26–30 |
isValidApiKeyIdFormat accepts only UUID / numeric, but selectOwnedApiKeyByIdentifier supports .eq('key', id) lookups. Legacy non-UUID key values would 400 before reaching the lookup. Impact depends on whether non-UUID keys exist in prod. |
| 10 | public/bundle/set_channel.ts:~60 (unverified) |
checkPermission (which acquires a second pool connection) is now called inside an open BEGIN transaction that already holds a connection → possible pool-starvation deadlock under load. Move the check before BEGIN. |
🟢 Low / Minor
cli/src/channel/delete.ts:~39—successIfNotFoundreturnstruebefore the permission check, but no deletion happens for a missing channel → existence-oracle only, not an auth bypass.src/pages/settings/organization/Members.vue:~608—console.logleaks invited email + role (PII) in production.src/stores/organization.ts:~620—console.logleaks org ID, full org object, and user ID indeleteOrganization.src/pages/ApiKeys.vue:~638/src/stores/organization.ts:~75(unverified) — owner-role priority /roleHasOrgRankedge cases in client-side gating (display-only; server is authoritative).
Refuted / discarded (investigated, not real)
- "
group_members_update/groups_updatelackWITH CHECK→ cross-org move" — Postgres reusesUSINGasWITH CHECKon UPDATE, so the neworg_id/group_idis re-validated. Not exploitable. - "API keys without RBAC bindings fail open to the JWT path" — refuted on inspection; the genuine, narrower issue is folded into Critical Launch paid offers #1.
- "
middlewareKeyallows unauthenticated access to any route" — authentication still happens; the real issue is the authorization decoupling (High #3). - Several
accept_invitationatomicity/overwrite + apikey TOCTOU claims — overlapping duplicates; the real defect is the silent failure (High Display download stats from live update in capgo app #2).
Coverage notes
- Automated adversarial verification was rate-limited; 11 highest-impact findings were hand-verified, the rest (esp. items marked unverified) deserve a maintainer look.
- The 43 pgTAP security tests (
supabase/tests/*.sql) and regenerated schema dumps were excluded from CodeRabbit (file-count cap) — not independently reviewed here.
Engines: CodeRabbit · Codex · Claude reviewer fleet (security / database / typescript / silent-failure / correctness agents). Report-only — no code was modified.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?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: cd9e395297
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f19fb3d516
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c844bbb31
ℹ️ 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".
…agement-hardening # Conflicts: # graphify-out/graph.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b60ad205c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba13a5dc05
ℹ️ 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".
| throw new Error(msg) | ||
| } | ||
| } | ||
| if (!(await checkAppExists(supabase, appid))) { |
There was a problem hiding this comment.
When a key/user is scoped only to a channel (for example the seeded channel_admin role), channel set and channel delete now pass the channel id into hasCliPermission, but this shared helper still calls checkAppExists() first. That RPC is gated by app.read with no channel id, so channel-scoped callers fail here with “App ... does not exist” before their channel.update_settings/channel.delete permission can be evaluated.
Useful? React with 👍 / 👎.
| CREATE POLICY "Allow RBAC channel_devices insert" | ||
| ON public.channel_devices | ||
| FOR INSERT | ||
| TO authenticated |
There was a problem hiding this comment.
Fresh evidence after the route-level forced-device permission fix: API-key callers that pass channel.manage_forced_devices still write through supabaseWithAuth, which uses the anon role for capgkey auth, but this INSERT policy is only TO authenticated. Creating a new forced-device override through /private/channel_device with a valid channel-scoped API key therefore fails under RLS even though the handler already authorized the channel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c1465f596
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…agement-hardening # Conflicts: # graphify-out/GRAPH_REPORT.md # graphify-out/graph.json
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
3 participants
Summary (AI generated)
middlewareV2(['all', 'write']); handlers now authenticate first and enforce authorization through RBAC checks, API route guards, and RLS.use_new_rbacruntime gating while keeping the compatibility output field and old SQL compatibility helpers./apikeymanagement: broad non-limitedallAPI keys retain sibling key update/delete compatibility, while limited scoped keys are filtered out and all API keys are blocked from key creation, self-mutation, and role-binding replacement.orgsare closed,use_new_rbacis forced true, rollback cannot disable RBAC, andcli organization setnow callsPUT /organizationinstead of writingorgsdirectly.password_policy_configso moving CLI org settings to the API route does not drop the password-policy feature.Motivation (AI generated)
RBAC is now the authoritative authorization model. Leaving old key-mode write labels, app-side RBAC feature flags, and CLI direct org writes made the active security boundary ambiguous and could preserve upgrade/downgrade paths that no longer match the product model. This PR removes those stale surfaces while preserving intentional compatibility for old API-key read paths and old org payload shape.
Fix Justification (AI generated)
['all', 'write']route declarations are no longer the security boundary and were misleading./apikeyread/list compatibility: keeping API-key reads avoids breaking old compatible callers; scoped keys still use the existing limited-scope filter.orgsUPDATE is now authenticated-only and named-RBAC guarded; API-key org settings writes must go through the backend route wheremiddlewareV2,checkPermission, and API-key org policy checks run before the service-role write.cli organization setno longer relies on direct Supabaseorgs.update(...), so it does not need the old write RLS policy to stay open.min_lengthbounded to 6..72, matching the DB constraint.rbac_enable_for_orgremains service-role callable for old automation, but always returns RBAC enabled;rbac_rollback_orgis retained but cannot disable RBAC or delete bindings.Business Impact (AI generated)
This reduces the chance of API-key self-escalation, leaked-key damage, or accidental reactivation of legacy write behavior, while avoiding a breaking change for compatible API-key read/list callers and current CLI org-settings workflows.
Test Plan (AI generated)
bun lint:backendbun typecheck:backendbun run cli:typecheck && bun run typecheck:backend && bun run typecheck:frontendbun run cli:checkbun run supabase:db:resetPGSSLMODE=disable bunx supabase test db --db-url postgresql://postgres:postgres@127.0.0.1:60642/postgres supabase/tests/00-supabase_test_helpers.sql supabase/tests/26_test_rls_policies.sql supabase/tests/48_test_rbac_admin_rpc_execute_grants.sqlbun test --timeout 60000 tests/events.test.ts tests/apikeys.test.ts tests/apikeys-expiration.test.ts tests/organization-api.test.ts tests/audit-logs.test.ts tests/password-policy.test.tsbunx vitest run tests/organization-put-stripe-sync.unit.test.tsGenerated with AI
Summary by CodeRabbit
New Features
Improvements