Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis migration hardens API-key-scoped authorization for webhooks and webhook_deliveries by dropping existing RLS policies and recreating them with conditional routing: API-key identity checks via get_apikey_header, get_identity_org_allowed, and check_min_rights when an API key is present, otherwise falling back to user ID authorization across all CRUD operations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Policy as RLS Policy Engine
participant KeyFunc as get_apikey_header()
participant OrgFunc as get_identity_org_allowed()
participant RightsFunc as check_min_rights()
participant Fallback as User ID Auth
Client->>Policy: Request webhooks/deliveries (with/without API key)
Policy->>KeyFunc: Check for API key header
alt API Key Present
KeyFunc-->>Policy: API key found
Policy->>OrgFunc: Validate org scope
OrgFunc-->>Policy: Scope valid?
alt Scope Valid
Policy->>RightsFunc: Check operation rights
RightsFunc-->>Policy: Rights granted?
alt Rights Sufficient
Policy-->>Client: ✓ Access allowed
else
Policy-->>Client: ✗ Access denied
end
else
Policy-->>Client: ✗ Org mismatch
end
else No API Key
KeyFunc-->>Policy: No API key
Policy->>Fallback: Route to user ID auth
Fallback-->>Policy: User authorized?
alt User Valid
Policy-->>Client: ✓ Access allowed
else
Policy-->>Client: ✗ Access denied
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
riderx
changed the title
riderx
force-pushed
the
riderx/fix-webhook-org-scope
branch
from
3c11624 to
a1be230
Compare
riderx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260308091000_restrict_webhook_key_modes.sql`:
- Around line 138-153: The UPDATE policy "Allow admin to update
webhook_deliveries" on public.webhook_deliveries is missing a WITH CHECK clause
so new values (e.g., org_id) aren't validated; add a WITH CHECK that mirrors the
USING condition but evaluates the proposed row (use NEW.org_id or equivalent)
and calls public.check_min_rights(...) with
public.get_identity_org_allowed('{all,write,upload}'::public.key_mode[],
NEW.org_id) so updates are authorized against the target org; reference the
existing webhooks UPDATE policy for the exact predicate structure and use the
same functions public.check_min_rights and public.get_identity_org_allowed.
- Around line 1-11: The migration 20260308091000_restrict_webhook_key_modes.sql
references the function public.get_identity_org_allowed() which doesn't exist
until migration 20260309091500_enforce_api_key_app_scoped_org_access.sql,
causing the migration to fail; fix by ensuring the function exists before this
policy migration — either rename 20260308091000 to a timestamp after
20260309091500 so it runs later, or merge the two migrations into one file with
the CREATE FUNCTION public.get_identity_org_allowed(...) placed before the ALTER
POLICY/GRANT statements that reference it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 992b48c6-5fb1-44ad-9ea8-5c4b6c78ed88
📒 Files selected for processing (2)
supabase/migrations/20260308091000_restrict_webhook_key_modes.sqlsupabase/migrations/20260309091500_enforce_api_key_app_scoped_org_access.sql
Comment on lines
+1
to
+11
| -- ============================================================================= | ||
| -- Migration: Restrict webhook table access to write-capable API keys | ||
| -- | ||
| -- Direct REST queries with read-only API keys can currently resolve identity via | ||
| -- get_identity_org_allowed() because its key mode allow-list includes `read`. | ||
| -- That enables webhook secret reads from public webhooks table. | ||
| -- | ||
| -- This migration tightens webhook and webhook_deliveries policies so only | ||
| -- all/upload/write-capable keys can satisfy the identity checks for any | ||
| -- write/read operation and the same org-scoped admin checks apply. | ||
| -- ============================================================================= |
Contributor
There was a problem hiding this comment.
Critical: Migration order causes failure — function does not exist yet.
This migration (timestamp 20260308091000) calls public.get_identity_org_allowed(), but that function is created in 20260309091500_enforce_api_key_app_scoped_org_access.sql (timestamp 20260309091500). Since migrations run in timestamp order, this policy migration executes first and will fail with a "function does not exist" error.
🔧 Suggested fix
Rename this migration file to have a timestamp after the function migration:
- 20260308091000_restrict_webhook_key_modes.sql
+ 20260309092000_restrict_webhook_key_modes.sqlOr alternatively, merge both migrations into a single file with the function defined before the policies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260308091000_restrict_webhook_key_modes.sql` around
lines 1 - 11, The migration 20260308091000_restrict_webhook_key_modes.sql
references the function public.get_identity_org_allowed() which doesn't exist
until migration 20260309091500_enforce_api_key_app_scoped_org_access.sql,
causing the migration to fail; fix by ensuring the function exists before this
policy migration — either rename 20260308091000 to a timestamp after
20260309091500 so it runs later, or merge the two migrations into one file with
the CREATE FUNCTION public.get_identity_org_allowed(...) placed before the ALTER
POLICY/GRANT statements that reference it.
Comment on lines
+138
to
+153
| CREATE POLICY "Allow admin to update webhook_deliveries" | ||
| ON public.webhook_deliveries | ||
| FOR UPDATE | ||
| TO authenticated, anon | ||
| USING ( | ||
| public.check_min_rights( | ||
| 'admin'::public.user_min_right, | ||
| public.get_identity_org_allowed( | ||
| '{all,write,upload}'::public.key_mode[], | ||
| org_id | ||
| ), | ||
| org_id, | ||
| null::character varying, | ||
| null::bigint | ||
| ) | ||
| ); |
Contributor
There was a problem hiding this comment.
Missing WITH CHECK clause on UPDATE policy allows potential org_id manipulation.
Unlike the webhooks UPDATE policy (lines 53-80), this policy only has USING without WITH CHECK. A user with access to the current row could potentially update org_id to an unauthorized organization since the new values aren't validated.
🔧 Suggested fix
CREATE POLICY "Allow admin to update webhook_deliveries"
ON public.webhook_deliveries
FOR UPDATE
TO authenticated, anon
USING (
public.check_min_rights(
'admin'::public.user_min_right,
public.get_identity_org_allowed(
'{all,write,upload}'::public.key_mode[],
org_id
),
org_id,
null::character varying,
- null::bigint
+ null::bigint
)
+)
+WITH CHECK (
+ public.check_min_rights(
+ 'admin'::public.user_min_right,
+ public.get_identity_org_allowed(
+ '{all,write,upload}'::public.key_mode[],
+ org_id
+ ),
+ org_id,
+ null::character varying,
+ null::bigint
+ )
);📝 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.
Suggested change
| CREATE POLICY "Allow admin to update webhook_deliveries" | |
| ON public.webhook_deliveries | |
| FOR UPDATE | |
| TO authenticated, anon | |
| USING ( | |
| public.check_min_rights( | |
| 'admin'::public.user_min_right, | |
| public.get_identity_org_allowed( | |
| '{all,write,upload}'::public.key_mode[], | |
| org_id | |
| ), | |
| org_id, | |
| null::character varying, | |
| null::bigint | |
| ) | |
| ); | |
| CREATE POLICY "Allow admin to update webhook_deliveries" | |
| ON public.webhook_deliveries | |
| FOR UPDATE | |
| TO authenticated, anon | |
| USING ( | |
| public.check_min_rights( | |
| 'admin'::public.user_min_right, | |
| public.get_identity_org_allowed( | |
| '{all,write,upload}'::public.key_mode[], | |
| org_id | |
| ), | |
| org_id, | |
| null::character varying, | |
| null::bigint | |
| ) | |
| ) | |
| WITH CHECK ( | |
| public.check_min_rights( | |
| 'admin'::public.user_min_right, | |
| public.get_identity_org_allowed( | |
| '{all,write,upload}'::public.key_mode[], | |
| org_id | |
| ), | |
| org_id, | |
| null::character varying, | |
| null::bigint | |
| ) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260308091000_restrict_webhook_key_modes.sql` around
lines 138 - 153, The UPDATE policy "Allow admin to update webhook_deliveries" on
public.webhook_deliveries is missing a WITH CHECK clause so new values (e.g.,
org_id) aren't validated; add a WITH CHECK that mirrors the USING condition but
evaluates the proposed row (use NEW.org_id or equivalent) and calls
public.check_min_rights(...) with
public.get_identity_org_allowed('{all,write,upload}'::public.key_mode[],
NEW.org_id) so updates are authorized against the target org; reference the
existing webhooks UPDATE policy for the exact predicate structure and use the
same functions public.check_min_rights and public.get_identity_org_allowed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260308180224_webhook-api-key-org-scope-hardening.sql`:
- Around line 23-24: The SELECT allowlist for API keys mistakenly includes the
"read" mode; update the calls to public.get_identity_org_allowed that pass
'{read,upload,write,all}'::public.key_mode[] (seen around the
webhook/webhook_deliveries allowlist) to remove "read" so they pass
'{upload,write,all}'::public.key_mode[] instead; apply the same change to the
other occurrence referenced (lines near the second call to
public.get_identity_org_allowed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dba9de6b-5484-47ab-b7d8-79a06c9d080b
📒 Files selected for processing (1)
supabase/migrations/20260308180224_webhook-api-key-org-scope-hardening.sql
supabase/migrations/20260308180224_webhook-api-key-org-scope-hardening.sql
Outdated
Show resolved
Hide resolved
riderx
changed the title
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)