Restrict audit_logs to authenticated users#1572
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. 📝 WalkthroughWalkthroughReplaces API-key based access with JWT-authenticated access for audit logs: DB RLS now requires authenticated super_admin via Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: b18befa158
ℹ️ 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".
| REVOKE ALL ON TABLE "public"."audit_logs" FROM "anon"; | ||
| REVOKE ALL ON SEQUENCE "public"."audit_logs_id_seq" FROM "anon"; | ||
|
|
||
| DROP POLICY IF EXISTS "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs"; | ||
| DROP POLICY IF EXISTS "Allow select for auth (super_admin+)" ON "public"."audit_logs"; | ||
|
|
||
| CREATE POLICY "Allow select for auth (super_admin+)" ON "public"."audit_logs" | ||
| FOR SELECT TO "authenticated" | ||
| USING ( |
There was a problem hiding this comment.
Revoking anon and limiting the SELECT policy to authenticated makes all API-key requests to audit_logs fail, because supabaseApikey(...) creates a client with the anon key and relies on RLS to authorize via capgkey (see supabase/functions/_backend/public/organization/audit.ts using supabaseApikey(...).from('audit_logs')). With this change, auth.uid() is null for those calls, so org audit endpoints that are currently API-key authenticated will start returning permission errors. If the intent is to keep API-key access, the policy needs to include anon (or a separate policy for capgkey-based access); otherwise, the backend endpoint must be updated to use authenticated/service-role access instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request attempts to optimize audit_logs Row Level Security (RLS) by restricting access to authenticated users only. However, the changes introduce critical breaking changes to the existing API.
Changes:
- Revokes all anon role access to
audit_logstable and its sequence - Replaces the existing RLS policy with one that only allows authenticated role access using
auth.uid()-based super_admin checks - Removes support for API key-based audit log access
| REVOKE ALL ON TABLE "public"."audit_logs" FROM "anon"; | ||
| REVOKE ALL ON SEQUENCE "public"."audit_logs_id_seq" FROM "anon"; |
There was a problem hiding this comment.
This change breaks API key access to audit logs. The endpoint /organization/audit uses supabaseApikey() which creates a Supabase client with the anon role (see supabase/functions/_backend/utils/supabase.ts:93). The old policy allowed anon role access via get_identity_org_allowed(), but revoking ALL permissions from anon means API keys can no longer query audit logs. The new policy restricts access to the authenticated role only, which API keys don't use.
This will cause all audit log tests in tests/audit-logs.test.ts to fail, as they use API key authentication (Authorization header with APIKEY_TEST_ALL). Additionally, this breaks the documented API functionality for customers using API keys to retrieve audit logs via the /organization/audit endpoint.
| -- Restrict audit_logs access to authenticated users only and fail fast for anon | ||
| -- to avoid expensive RLS evaluation on unauthenticated requests. | ||
|
|
||
| REVOKE ALL ON TABLE "public"."audit_logs" FROM "anon"; | ||
| REVOKE ALL ON SEQUENCE "public"."audit_logs_id_seq" FROM "anon"; | ||
|
|
||
| DROP POLICY IF EXISTS "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs"; | ||
| DROP POLICY IF EXISTS "Allow select for auth (super_admin+)" ON "public"."audit_logs"; | ||
|
|
||
| CREATE POLICY "Allow select for auth (super_admin+)" ON "public"."audit_logs" | ||
| FOR SELECT TO "authenticated" | ||
| USING ( | ||
| (SELECT public.check_min_rights( | ||
| 'super_admin'::public.user_min_right, | ||
| auth_check.uid, | ||
| org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) | ||
| FROM (SELECT auth.uid() AS uid) AS auth_check) |
There was a problem hiding this comment.
The new policy uses a subquery with auth.uid() to call check_min_rights. However, this pattern is inconsistent with the existing audit_logs policy (see supabase/migrations/20251226125240_audit_log.sql:177-191) which used get_identity_org_allowed() to support both authenticated users AND API keys.
If the intent is to restrict to authenticated users only (excluding API keys), the correct approach would be:
- Keep the anon role grant if API keys should have access, OR
- Update the backend endpoint to use JWT authentication instead of API keys for audit logs
The PR description states "audit logs are not meant to be accessible via API keys", but this contradicts the existing implementation which explicitly supports API keys via the /organization/audit endpoint with the middlewareKey middleware.
| -- Restrict audit_logs access to authenticated users only and fail fast for anon | |
| -- to avoid expensive RLS evaluation on unauthenticated requests. | |
| REVOKE ALL ON TABLE "public"."audit_logs" FROM "anon"; | |
| REVOKE ALL ON SEQUENCE "public"."audit_logs_id_seq" FROM "anon"; | |
| DROP POLICY IF EXISTS "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs"; | |
| DROP POLICY IF EXISTS "Allow select for auth (super_admin+)" ON "public"."audit_logs"; | |
| CREATE POLICY "Allow select for auth (super_admin+)" ON "public"."audit_logs" | |
| FOR SELECT TO "authenticated" | |
| USING ( | |
| (SELECT public.check_min_rights( | |
| 'super_admin'::public.user_min_right, | |
| auth_check.uid, | |
| org_id, | |
| NULL::character varying, | |
| NULL::bigint | |
| ) | |
| FROM (SELECT auth.uid() AS uid) AS auth_check) | |
| -- Optimize audit_logs RLS while preserving access for both authenticated users | |
| -- and API keys (anon role), relying on get_identity_org_allowed() to resolve | |
| -- the caller identity in a unified way. | |
| -- NOTE: Do not revoke anon privileges here; existing grants are required so | |
| -- API-key-based access (middlewareKey) can still reach the table and rely | |
| -- on RLS for authorization. | |
| DROP POLICY IF EXISTS "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs"; | |
| DROP POLICY IF EXISTS "Allow select for auth (super_admin+)" ON "public"."audit_logs"; | |
| CREATE POLICY "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs" | |
| FOR SELECT TO "authenticated", "anon" | |
| USING ( | |
| public.check_min_rights( | |
| 'super_admin'::public.user_min_right, | |
| public.get_identity_org_allowed(org_id), | |
| org_id, | |
| NULL::character varying, | |
| NULL::bigint | |
| ) |
| REVOKE ALL ON SEQUENCE "public"."audit_logs_id_seq" FROM "anon"; | ||
|
|
||
| DROP POLICY IF EXISTS "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs"; | ||
| DROP POLICY IF EXISTS "Allow select for auth (super_admin+)" ON "public"."audit_logs"; |
There was a problem hiding this comment.
The policy drops both the old policies but only creates one new policy. The migration drops two policies:
- "Allow select for auth, api keys (super_admin+)" (the original broad policy)
- "Allow select for auth (super_admin+)" (possibly added in a later migration)
However, searching through migrations shows that only the first policy "Allow select for auth, api keys (super_admin+)" was created in 20251226125240_audit_log.sql. The second policy name doesn't exist in any prior migrations. This DROP statement for a non-existent policy is harmless (due to IF EXISTS) but indicates unclear understanding of the current state.
| DROP POLICY IF EXISTS "Allow select for auth (super_admin+)" ON "public"."audit_logs"; |
| -- Restrict audit_logs access to authenticated users only and fail fast for anon | ||
| -- to avoid expensive RLS evaluation on unauthenticated requests. | ||
|
|
||
| REVOKE ALL ON TABLE "public"."audit_logs" FROM "anon"; | ||
| REVOKE ALL ON SEQUENCE "public"."audit_logs_id_seq" FROM "anon"; | ||
|
|
||
| DROP POLICY IF EXISTS "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs"; | ||
| DROP POLICY IF EXISTS "Allow select for auth (super_admin+)" ON "public"."audit_logs"; | ||
|
|
||
| CREATE POLICY "Allow select for auth (super_admin+)" ON "public"."audit_logs" | ||
| FOR SELECT TO "authenticated" | ||
| USING ( | ||
| (SELECT public.check_min_rights( | ||
| 'super_admin'::public.user_min_right, | ||
| auth_check.uid, | ||
| org_id, | ||
| NULL::character varying, | ||
| NULL::bigint | ||
| ) | ||
| FROM (SELECT auth.uid() AS uid) AS auth_check) | ||
| ); |
There was a problem hiding this comment.
The migration lacks test coverage updates. The existing tests in tests/audit-logs.test.ts use API key authentication and will fail after this migration. No corresponding changes to the test file or backend endpoint are included in this PR to accommodate the new authentication requirement. The PR test plan only mentions "bun lint:backend", which doesn't catch this runtime breaking change.
riderx
force-pushed
the
riderx/audit-logs-rls
branch
from
a0d9354 to
84231b9
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: 4
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/public/organization/audit.ts`:
- Around line 40-42: The code calls hasOrgRight(c, body.orgId, auth.userId,
'super_admin') which uses the admin SDK and bypasses RLS; replace this with a
rights check using an auth-scoped client so RLS applies (e.g., call the RPC or
permission check via supabaseWithAuth(c, auth).rpc(...) or equivalent
auth-scoped method), passing body.orgId and the required role ('super_admin')
and throw the same simpleError if the RPC result denies access; update the logic
around hasOrgRight, auth, and c to use supabaseWithAuth instead of the admin
client.
- Around line 28-38: The getAuditLogs guard currently only checks auth.userId;
require the request to be authenticated via JWT by checking
c.get('auth')?.authType === 'jwt' in addition to auth.userId (i.e., ensure auth
exists, auth.userId is present, and auth.authType === 'jwt'); if that check
fails, throw the same simpleError('not_authorized', 'Not authorized') to prevent
apikey-based access (refer to the auth variable retrieved in getAuditLogs and
enforce auth.authType === 'jwt' before proceeding or calling supabaseWithAuth).
In `@tests/audit-logs.test.ts`:
- Line 5: Tests for bundle create/update/delete are currently passing JWT auth
(authHeaders) and should exercise API-key auth; update those test cases that
call the /bundle endpoints to use API-key headers instead of getAuthHeaders()
while keeping JWTs (getAuthHeaders()) for /organization/audit checks.
Concretely, in the tests that call fetchWithRetry('/bundle...') replace the auth
header argument (the variable that holds getAuthHeaders()) with the API-key
header variable (create or reuse the existing API-key header constant/source
from your test-utils or Supabase client), ensuring the /organization/audit calls
still use getAuthHeaders(); apply the same change to the other /bundle call
blocks referenced (around the other ranges).
In `@tests/test-utils.ts`:
- Around line 124-159: getAuthHeaders currently caches a rejected promise when
sign-in fails, preventing retries; wrap the async initialization inside a
try/catch so that on any thrown error you clear authHeadersPromise (and
cachedAuthHeaders) before re-throwing, and only set cachedAuthHeaders on
success; specifically update the inner async IIFE used to populate
authHeadersPromise in getAuthHeaders so failures reset authHeadersPromise =
undefined (and cachedAuthHeaders = undefined) to allow subsequent calls to
retry.
84231b9 to
9517c0d
Compare
riderx
force-pushed
the
riderx/audit-logs-rls
branch
from
a28de64 to
c434654
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/tests/40_test_audit_log_apikey.sql (1)
141-188:⚠️ Potential issue | 🟡 MinorAvoid hardcoded API key triggering secret scanning.
Gitleaks flagged the literal key. If CI runs secret scanning, consider fetching the fixture key from a helper/seed reference or adding an allowlist annotation for this test-only value.
🤖 Fix all issues with AI agents
In `@tests/audit-logs.test.ts`:
- Line 5: The named imports in the test import statement are out of the
ESLint-expected order; reorder the named imports so apiKeyHeaders appears before
getSupabaseClient in the import list (e.g., place apiKeyHeaders just prior to
getSupabaseClient among BASE_URL, fetchWithRetry, getAuthHeaders, apiKeyHeaders,
getSupabaseClient, ORG_ID, TEST_EMAIL, USER_ID) to satisfy the lint rule without
changing any identifiers.
🧹 Nitpick comments (1)
tests/audit-logs.test.ts (1)
71-339: Considerit.concurrent()for parallel test execution.This file can run faster and align with the parallel-test guideline by using
it.concurrent()for the test cases.As per coding guidelines: Design tests for parallel execution; use `it.concurrent()` instead of `it()` to run tests in parallel within the same file.🔁 Example change (apply to all tests in this file)
- it('get audit logs for organization', async () => { + it.concurrent('get audit logs for organization', async () => {
| import { z } from 'zod' | ||
|
|
||
| import { BASE_URL, fetchWithRetry, getSupabaseClient, headers, ORG_ID as SEED_ORG_ID, TEST_EMAIL, USER_ID } from './test-utils.ts' | ||
| import { BASE_URL, fetchWithRetry, getAuthHeaders, getSupabaseClient, headers as apiKeyHeaders, ORG_ID as SEED_ORG_ID, TEST_EMAIL, USER_ID } from './test-utils.ts' |
There was a problem hiding this comment.
Fix named import order to satisfy ESLint.
The lint rule expects apiKeyHeaders before getSupabaseClient.
🧹 Proposed fix
-import { BASE_URL, fetchWithRetry, getAuthHeaders, getSupabaseClient, headers as apiKeyHeaders, ORG_ID as SEED_ORG_ID, TEST_EMAIL, USER_ID } from './test-utils.ts'
+import { BASE_URL, fetchWithRetry, getAuthHeaders, headers as apiKeyHeaders, getSupabaseClient, ORG_ID as SEED_ORG_ID, TEST_EMAIL, USER_ID } from './test-utils.ts'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { BASE_URL, fetchWithRetry, getAuthHeaders, getSupabaseClient, headers as apiKeyHeaders, ORG_ID as SEED_ORG_ID, TEST_EMAIL, USER_ID } from './test-utils.ts' | |
| import { BASE_URL, fetchWithRetry, getAuthHeaders, headers as apiKeyHeaders, getSupabaseClient, ORG_ID as SEED_ORG_ID, TEST_EMAIL, USER_ID } from './test-utils.ts' |
🧰 Tools
🪛 ESLint
[error] 5-5: Expected "apiKeyHeaders" to come before "getSupabaseClient".
(perfectionist/sort-named-imports)
🤖 Prompt for AI Agents
In `@tests/audit-logs.test.ts` at line 5, The named imports in the test import
statement are out of the ESLint-expected order; reorder the named imports so
apiKeyHeaders appears before getSupabaseClient in the import list (e.g., place
apiKeyHeaders just prior to getSupabaseClient among BASE_URL, fetchWithRetry,
getAuthHeaders, apiKeyHeaders, getSupabaseClient, ORG_ID, TEST_EMAIL, USER_ID)
to satisfy the lint rule without changing any identifiers.
|
|
/tip $70 @Judel777 thank for finding it |
|
🎉🎈 @Judel777 has been awarded $70 by Capgo! 🎈🎊 |
Summary (AI generated)
Motivation (AI generated)
Business Impact (AI generated)
Test plan (AI generated)
Screenshots
Checklist
.
accordingly.
my tests
Generated with AI
Summary by CodeRabbit
Chores
Bug Fixes
Tests
New Features
Refactor