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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 539dedc391
ℹ️ 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: 45e3f74dec
ℹ️ 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".
💡 Codex Reviewcapgo/supabase/schemas/prod.sql Line 18016 in 7ebf397  Align app_versions update policy in prod schema
The cleanup migration now guards ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Followed up on the remaining standalone findings:
Validation passed locally for the focused org/API-key tests and pgTAP policy test, and the latest CI run is green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18cad6d46b
ℹ️ 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".
💡 Codex Reviewcapgo/supabase/schemas/prod.sql Lines 19659 to 19660 in 6932d77 Preserve get_user_id anon grants in the schema dump
The new migration explicitly grants ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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. |
🧪 Builder onboarding TUI preview — ✅ passed▶ Open the interactive HTML report (zoomable journey tree + cast playback) Commit: 7420610 · Job summary with the result table |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77480e5841
ℹ️ 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. |
|
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. |
|
2 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