fix(rbac): enforce API key expiration policy in permission checks#2130
fix(rbac): enforce API key expiration policy in permission checks#2130teixr12 wants to merge 6 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds two DB functions enforcing API-key expiration, max-expiration-window, org/app scoping, and 2FA before RBAC or legacy permission checks; updates backend util and webhook handling for max-expiration errors; tests validate the org toggle and overlong-key rejection. ChangesAPI-Key Expiration and Scoping Enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/20260511013000_enforce_rbac_apikey_expiration_policy.sql`:
- Around line 516-579: The _no_password_policy variant diverges from
rbac_check_permission_direct by not applying channel_permission_overrides,
allowing explicit channel denies to be bypassed; update the logic around
rbac_has_permission calls (both the apikey branch using
public.rbac_principal_apikey() and the user branch using
public.rbac_principal_user()) to run the same channel_permission_overrides
resolution as rbac_check_permission_direct does (i.e. after computing v_allowed
from rbac_has_permission, also consult/apply
channel_permission_overrides/group-based overrides for p_channel_id), and ensure
the logging paths (pg_log calls) reflect the override result; mirror the
override application that exists in rbac_check_permission_direct so channel.*
checks behave identically in the _no_password_policy path.
In `@tests/rbac-permissions.test.ts`:
- Around line 750-756: This test mutates the shared ORG_ID fixture via a direct
UPDATE; instead create a test-specific org/app pair and use that id instead of
ORG_ID: add a seeded org (name prefixed with the test file or feature, e.g.,
"rbac-permissions.test...") and associated app via the existing test helper or
DB insert helper, capture the returned org_id/app_id, then run the UPDATE query
against that new org_id (replace references to ORG_ID and the UPDATE block
around the query call), and ensure the rest of the test (including the similar
block at lines 797-801) uses the new ids so shared fixtures are never mutated
during parallel runs.
🪄 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: 8f922600-9ea1-4c0e-9658-8aa7845dce8d
📒 Files selected for processing (2)
supabase/migrations/20260511013000_enforce_rbac_apikey_expiration_policy.sqltests/rbac-permissions.test.ts
teixr12
force-pushed
the
bounty/rbac-apikey-expiration-policy
branch
2 times, most recently
from
0e404ee to
5b984bb
Compare
teixr12
force-pushed
the
bounty/rbac-apikey-expiration-policy
branch
from
5b984bb to
c0ac762
Compare
|
Follow-up after the latest push:
CI is green on the latest commit: backend tests, Playwright, benchmarks, dead-code, Socket, Sonar, and CodeRabbit all passed. |
SpeedyArt
left a comment
SpeedyArt
left a comment
There was a problem hiding this comment.
I think the runtime policy check still only covers the require_apikey_expiration half of the org API-key expiration policy.
The migration reads only orgs.require_apikey_expiration inside both direct RBAC functions and rejects only expires_at IS NULL. But the existing create/update trigger also enforces orgs.max_apikey_expiration_days: if an org later sets or lowers that max, an already-issued key with expires_at far beyond the new max will keep passing rbac_check_permission_direct() until its old expiration date.
That leaves the same retroactive-policy gap this PR is closing for non-expiring keys, just for long-lived expiring keys. I would load max_apikey_expiration_days alongside require_apikey_expiration and deny when v_api_key.expires_at > now() + make_interval(days => max_apikey_expiration_days). A regression can mirror the new policy-flip test: create a scoped key expiring well beyond the future max, flip max_apikey_expiration_days to a shorter value, and assert both direct RBAC functions deny it.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/utils/supabase.ts (1)
314-314: 💤 Low valueClarify why the unused
_supabaseparameter is retained.The parameter is now ignored (indicated by the
_prefix) since the function always usessupabaseAdmin(c)for the policy lookup. If this is kept for backward compatibility with existing callers, consider adding a JSDoc comment explaining that the parameter is deprecated and ignored. If backward compatibility is not required, consider removing it in a follow-up to simplify the API.🤖 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/functions/_backend/utils/supabase.ts` at line 314, The parameter `_supabase: SupabaseClient<Database>` is unused because the function always uses supabaseAdmin(c) for policy lookup; either remove the parameter in a follow-up to simplify the API or add a concise JSDoc on the parameter (and function) stating it is deprecated/ignored and kept only for backward compatibility so callers understand it has no effect; update the function signature comment to reference `_supabase` and supabaseAdmin(c) to make the intent explicit.
🤖 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.
Nitpick comments:
In `@supabase/functions/_backend/utils/supabase.ts`:
- Line 314: The parameter `_supabase: SupabaseClient<Database>` is unused
because the function always uses supabaseAdmin(c) for policy lookup; either
remove the parameter in a follow-up to simplify the API or add a concise JSDoc
on the parameter (and function) stating it is deprecated/ignored and kept only
for backward compatibility so callers understand it has no effect; update the
function signature comment to reference `_supabase` and supabaseAdmin(c) to make
the intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d3c1e04-54e1-45da-b100-bcfbb696aff7
📒 Files selected for processing (3)
supabase/functions/_backend/utils/supabase.tssupabase/migrations/20260511013000_enforce_rbac_apikey_expiration_policy.sqltests/rbac-permissions.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/rbac-permissions.test.ts
- supabase/migrations/20260511013000_enforce_rbac_apikey_expiration_policy.sql
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
One remaining policy surface looks missed: the TS org-policy helper still only enforces require_apikey_expiration, not the max-expiration half that this PR now adds to the SQL RBAC checks.
Evidence: apikeyHasOrgRightWithPolicy() now deliberately calls checkApikeyMeetsOrgPolicy() via supabaseAdmin(c), but that helper still selects only require_apikey_expiration and returns valid once the key has any expires_at. Callers such as the webhook permission path (assertWebhookOrgPolicy() before hasOrgRightApikey()/check_min_rights) rely on this helper as the API-key org-policy gate, so an org-scoped key whose expires_at is beyond a newly lowered max_apikey_expiration_days can still pass those non-rbac_check_permission_direct() surfaces.
Could we load max_apikey_expiration_days in checkApikeyMeetsOrgPolicy() and mirror the same expires_at > now + max denial used in the migration? A small regression through one helper caller (webhooks or org audit) would cover the gap that the new rbac-permissions tests do not exercise.
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 `@supabase/functions/_backend/utils/supabase.ts`:
- Around line 1715-1720: The max-expiration check currently compares
key.expires_at against a max date anchored to new Date(), which allows overly
long keys to become valid later; change the anchor to the key issuance time by
using key.created_at (e.g., parse key.created_at into a Date, add
org.max_apikey_expiration_days to that Date to compute maxDate) and then compare
new Date(key.expires_at) > maxDate, returning { valid: false, error:
'expiration_exceeds_max' } when it exceeds; ensure you handle missing/invalid
key.created_at the same way you handle other date fields.
🪄 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: ec830fe5-b332-4b7c-90ea-78ca23e43a47
📒 Files selected for processing (3)
supabase/functions/_backend/public/webhooks/index.tssupabase/functions/_backend/utils/supabase.tstests/webhooks-apikey-policy.test.ts
|
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked the latest head (7f7523f) against the policy gaps raised earlier. The current patch now enforces both halves of the org API-key expiration policy at runtime: non-expiring keys are denied when required, and overlong keys are compared against the key's created_at + max_apikey_expiration_days window in both the SQL RBAC functions and the TS org-policy helper.
I also checked the no-password direct path and the new regressions: the channel deny override case is covered, the overlong-key RBAC flip test covers both direct functions, and webhook listing now returns expiration_exceeds_max through the TS policy helper. GitHub checks are green, merge state is clean, and git diff --check origin/main...origin/pr-2130 passes locally. No remaining blocker from the issues I reviewed.
|
The runtime denial for 1. In
So if an admin sets The TS counterpart in
Two ways to reconcile:
Either way worth a regression test that sets 2. Date arithmetic uses local-timezone JS on the TS side, UTC interval on the SQL side
Supabase Edge Functions normally run in UTC so this is largely theoretical today, but the safer formulation is timezone-agnostic:
...and add a unit test that pins the system timezone to something non-UTC (e.g. 3. Minor: If Happy to send a tiny follow-up PR with the test cases if (1) reaches consensus. |
5 participants
Summary
Fixes an authorization gap where an API key created without
expires_atcould keep passing RBAC direct permission checks after its org later enablesrequire_apikey_expiration.Root Cause
The API-key create/update trigger enforces expiration policy for new writes, but
rbac_check_permission_direct()andrbac_check_permission_direct_no_password_policy()only checked whether a key was expired. They did not reject existing non-expiring keys when the target org starts requiring expiration.Fix
require_apikey_expiration = true.RBAC_CHECK_PERM_APIKEY_EXPIRATION_REQUIREDusing key id/org/app/channel metadata only, without logging secret key material.Test plan
git diff --checkbash scripts/check-supabase-migration-order.shtests/rbac-permissions.test.ts.bun run supabase:with-env -- bunx vitest run tests/rbac-permissions.test.ts, but this checkout does not havebunor installed project dependencies. GitHub CI is expected to run the backend suite for the pushed branch.Screenshots
N/A. This is a backend SQL authorization change with regression test coverage and no frontend/CLI UI changes.
Checklist
bun run lint:backend && bun run lint.Refs #1667
/claim #1667
Summary by CodeRabbit
New Features
Tests