Conversation
📝 WalkthroughWalkthroughThis pull request adds row-level security (RLS) policies to the Changes
Sequence DiagramsequenceDiagram
actor Client as Client/API Caller
participant RLS as RLS Policy<br/>(webhooks/deliveries)
participant GetKey as get_apikey_header()
participant GetIdent as get_identity_org_allowed()
participant CheckRights as check_min_rights()
participant DB as Database
Client->>RLS: Query with API key header or auth context
RLS->>GetKey: Retrieve API key from header
alt API Key Present
GetKey-->>RLS: API key found
RLS->>GetIdent: Validate identity for org scope
GetIdent-->>RLS: Identity or null
else No API Key
GetKey-->>RLS: null
RLS->>GetIdent: Use auth.uid()
GetIdent-->>RLS: User identity
end
RLS->>CheckRights: Verify admin rights for identity & org
CheckRights-->>RLS: Permission granted/denied
alt Permission Granted
RLS->>DB: Allow query execution
DB-->>Client: Return results
else Permission Denied
RLS->>Client: Block query
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.0.4)supabase/migrations/20260308235013_webhook-org-scope-api-key-org-check.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: 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: c2bd8ba605
ℹ️ 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".
| WHEN public.get_apikey_header() IS NOT NULL | ||
| THEN public.get_identity_org_allowed( | ||
| '{all,write,upload}'::public.key_mode[], |
There was a problem hiding this comment.
This branch is intended to make capgkey authoritative when both auth methods are present, but it still calls get_identity_org_allowed(...), which immediately returns auth.uid() whenever a JWT exists (see supabase/schemas/prod.sql around the early IF auth_uid IS NOT NULL THEN RETURN auth_uid). In mixed-auth requests, org-limited API key scope is therefore still bypassed by the user session, so the hardening in this migration does not actually prevent the cross-org access scenario it targets.
Useful? React with 👍 / 👎.
| CASE | ||
| WHEN public.get_apikey_header() IS NOT NULL | ||
| THEN public.get_identity_org_allowed( | ||
| '{all,write,upload}'::public.key_mode[], |
There was a problem hiding this comment.
This SELECT policy requires 'read' rights but only allows API key modes '{all,write,upload}', so read-only keys can no longer resolve an identity and are denied even for in-scope org rows. Previous webhook-delivery policies accepted read mode, so this change regresses existing read-only integrations that query delivery records with read-scoped keys.
Useful? React with 👍 / 👎.
|
![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
🧹 Nitpick comments (2)
supabase/migrations/20260308235013_webhook-org-scope-api-key-org-check.sql (1)
21-28: Use(SELECT auth.uid())in each fallback branch.The
ELSE auth.uid()branch here, and the repeated branches below, misses the repo’s required RLS pattern. Please switch those fallbacks toELSE (SELECT auth.uid()).♻️ Minimal fix
- ELSE auth.uid() + ELSE (SELECT auth.uid())As per coding guidelines, "Call
auth.uid()only once in RLS policies using a subquery pattern instead of calling it multiple times".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260308235013_webhook-org-scope-api-key-org-check.sql` around lines 21 - 28, The CASE fallback branches use auth.uid() directly which violates the RLS pattern; update each ELSE branch (the one returning auth.uid() in the CASE blocks around public.get_apikey_header() / public.get_identity_org_allowed(...) and any repeated similar CASEs) to return the UID via a subquery: replace ELSE auth.uid() with ELSE (SELECT auth.uid()) so the policy calls auth.uid() exactly once per the repo convention; ensure you update every identical fallback occurrence in this SQL snippet.tests/hashed-apikey-rls.test.ts (1)
573-614: Seed this suite per test so it can useit.concurrent().These cases share one API key, one webhook, and one delivery row from
beforeAll, and Line 637 mutates that shared resource. That keeps the suite order-dependent and blocks the repo’s required parallel-test pattern. Move setup/cleanup into per-test fixtures and switch the cases toit.concurrent().As per coding guidelines, "Design all tests for parallel execution; use
it.concurrent()instead ofit()to maximize parallelism; create dedicated seed data for tests that modify shared resources to avoid conflicts".Also applies to: 616-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hashed-apikey-rls.test.ts` around lines 573 - 614, The tests currently share mutable seeded state created in beforeAll/afterAll (limitedKey, webhookId, deliveryId via createHashedApiKey, pool.query inserts, and deleteApiKey) which prevents safe use of it.concurrent(); change the suite to create and tear down those resources per test (use beforeEach/afterEach or seed inside each test) so each test gets its own limitedKey, webhook row and webhook_delivery row, and then convert the relevant tests to it.concurrent() so they can run in parallel without mutating shared state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hashed-apikey-rls.test.ts`:
- Line 568: The describe block title "Webhook and webhook_delivery RLS with
API-key org scope precedence" must be lowercased to satisfy
test/prefer-lowercase-title; update the string passed to the describe call (the
describe that currently begins with "Webhook and webhook_delivery RLS with
API-key org scope precedence") to an all-lowercase version (e.g., "webhook and
webhook_delivery rls with api-key org scope precedence") so the suite name
conforms to the lowercase rule.
- Around line 635-649: The test uses limitedKey (scoped to 0000...0000) so the
UPDATE is rejected before Postgres reaches the WITH CHECK; modify the test to
create or reuse a second API key scoped to ORG_ID_RLS (e.g., limitedKeyOrg or
limitedKeyForRls) and call execWithAuthAndCapgkey with that key to attempt to
update the row's org_id to ORG_ID_2, then assert rowCount is 0 and org_id
remains ORG_ID_RLS; keep the existing variables deliveryId, ORG_ID_RLS and
ORG_ID_2 and only change which key is used for the failing UPDATE so the WITH
CHECK path in the migration is exercised.
---
Nitpick comments:
In `@supabase/migrations/20260308235013_webhook-org-scope-api-key-org-check.sql`:
- Around line 21-28: The CASE fallback branches use auth.uid() directly which
violates the RLS pattern; update each ELSE branch (the one returning auth.uid()
in the CASE blocks around public.get_apikey_header() /
public.get_identity_org_allowed(...) and any repeated similar CASEs) to return
the UID via a subquery: replace ELSE auth.uid() with ELSE (SELECT auth.uid()) so
the policy calls auth.uid() exactly once per the repo convention; ensure you
update every identical fallback occurrence in this SQL snippet.
In `@tests/hashed-apikey-rls.test.ts`:
- Around line 573-614: The tests currently share mutable seeded state created in
beforeAll/afterAll (limitedKey, webhookId, deliveryId via createHashedApiKey,
pool.query inserts, and deleteApiKey) which prevents safe use of
it.concurrent(); change the suite to create and tear down those resources per
test (use beforeEach/afterEach or seed inside each test) so each test gets its
own limitedKey, webhook row and webhook_delivery row, and then convert the
relevant tests to it.concurrent() so they can run in parallel without mutating
shared state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7585ba63-1f1c-4a3d-a646-61ade945f7c0
📒 Files selected for processing (2)
supabase/migrations/20260308235013_webhook-org-scope-api-key-org-check.sqltests/hashed-apikey-rls.test.ts
| }) | ||
| }) | ||
|
|
||
| describe('Webhook and webhook_delivery RLS with API-key org scope precedence', () => { |
There was a problem hiding this comment.
Lowercase this describe title.
test/prefer-lowercase-title will fail on the current suite name.
🧰 Tools
🪛 ESLint
[error] 568-568: describes should begin with lowercase
(test/prefer-lowercase-title)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hashed-apikey-rls.test.ts` at line 568, The describe block title
"Webhook and webhook_delivery RLS with API-key org scope precedence" must be
lowercased to satisfy test/prefer-lowercase-title; update the string passed to
the describe call (the describe that currently begins with "Webhook and
webhook_delivery RLS with API-key org scope precedence") to an all-lowercase
version (e.g., "webhook and webhook_delivery rls with api-key org scope
precedence") so the suite name conforms to the lowercase rule.
| it('prevents webhook_delivery org_id changes when update payload org_id is unauthorized', async () => { | ||
| const updatedRows = await execWithAuthAndCapgkey( | ||
| 'UPDATE public.webhook_deliveries SET org_id = $1 WHERE id = $2', | ||
| USER_ID_RLS, | ||
| limitedKey.key, | ||
| [ORG_ID_2, deliveryId], | ||
| ) | ||
|
|
||
| const { rows } = await pool.query( | ||
| 'SELECT org_id FROM public.webhook_deliveries WHERE id = $1', | ||
| [deliveryId], | ||
| ) | ||
|
|
||
| expect(updatedRows.rowCount).toBe(0) | ||
| expect(rows[0].org_id).toBe(ORG_ID_RLS) |
There was a problem hiding this comment.
This regression never reaches the new update WITH CHECK.
Because limitedKey is scoped to 00000000-0000-0000-0000-000000000000 while the row still belongs to ORG_ID_RLS, Postgres rejects the statement on the existing row first. So the WITH CHECK block in supabase/migrations/20260308235013_webhook-org-scope-api-key-org-check.sql at Lines 180-195 is not exercised here, and a bug there would still pass this test.
Use a second key limited to ORG_ID_RLS for this case, then try to move the row to ORG_ID_2.
🧪 Minimal coverage fix
describe('Webhook and webhook_delivery RLS with API-key org scope precedence', () => {
let limitedKey: { id: number, key: string, key_hash: string }
+ let scopedKey: { id: number, key: string, key_hash: string }
let webhookId: string
let deliveryId: string
beforeAll(async () => {
limitedKey = await createHashedApiKey('rls-webhook-org-scope-key', 'all')
+ scopedKey = await createHashedApiKey('rls-webhook-org-scope-update-key', 'all')
await pool.query(
`UPDATE public.apikeys SET limited_to_orgs = $1 WHERE id = $2`,
[['00000000-0000-0000-0000-000000000000'], limitedKey.id],
)
+ await pool.query(
+ `UPDATE public.apikeys SET limited_to_orgs = $1 WHERE id = $2`,
+ [[ORG_ID_RLS], scopedKey.id],
+ )
...
})
afterAll(async () => {
...
await deleteApiKey(limitedKey.id)
+ await deleteApiKey(scopedKey.id)
})
it('prevents webhook_delivery org_id changes when update payload org_id is unauthorized', async () => {
const updatedRows = await execWithAuthAndCapgkey(
'UPDATE public.webhook_deliveries SET org_id = $1 WHERE id = $2',
USER_ID_RLS,
- limitedKey.key,
+ scopedKey.key,
[ORG_ID_2, deliveryId],
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hashed-apikey-rls.test.ts` around lines 635 - 649, The test uses
limitedKey (scoped to 0000...0000) so the UPDATE is rejected before Postgres
reaches the WITH CHECK; modify the test to create or reuse a second API key
scoped to ORG_ID_RLS (e.g., limitedKeyOrg or limitedKeyForRls) and call
execWithAuthAndCapgkey with that key to attempt to update the row's org_id to
ORG_ID_2, then assert rowCount is 0 and org_id remains ORG_ID_RLS; keep the
existing variables deliveryId, ORG_ID_RLS and ORG_ID_2 and only change which key
is used for the failing UPDATE so the WITH CHECK path in the migration is
exercised.
1 participant
Summary (AI generated)
20260308235013_webhook-org-scope-api-key-org-check.sqlto enforce API-key-first authorization on webhook and webhook_delivery RLS paths, including org-scoped checks and safe update validation.tests/hashed-apikey-rls.test.tsfor webhook read precedence and unauthorizedwebhook_deliveries.org_idupdates when both user auth and an out-of-scope key are present.Motivation (AI generated)
capgkeyheader is supplied.Business Impact (AI generated)
Test Plan (AI generated)
bun lint:backendbun test tests/hashed-apikey-rls.test.ts(targeted regression)Summary by CodeRabbit
Release Notes
New Features
Tests