fix: restore bundle upload for API keys with app-only RBAC bindings#2079
fix: restore bundle upload for API keys with app-only RBAC bindings#2079
Conversation
Two bugs prevented OTA uploads via API keys while bundle list still worked: 1. **null owner_org (23502)**: The BEFORE INSERT trigger on app_versions called get_user_main_org_id_by_app_id which, since migration 20260203150000, runs auth checks that fail under PostgREST's authenticator role (no auth.uid(), auth.role()='anon'). The trigger fires after RLS has already validated access, so auth re-checks are redundant. Replaced with a minimal SECURITY DEFINER helper get_owner_org_by_app_id_internal that simply resolves the owning org from the apps table. 2. **no read access**: API keys created with only app-level role bindings (e.g. app_uploader) lack org_member and therefore org.read, which get_organization_cli_warnings requires. Users always receive org_member alongside app-level roles; API keys must respect the same invariant. post.ts now auto-adds an org_member binding for each org that has app-level bindings but no explicit org-level binding. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a SECURITY DEFINER helper to resolve an app's owner_org for triggers, updates the auto_owner_org_by_app_id trigger to use it (with an app_id-change guard and privilege hardening), and enriches API key binding creation to auto-add org-scoped ChangesBindings and App Ownership
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)
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.1.0)supabase/migrations/20260508122137_fix_app_versions_trigger_owner_org.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: Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🧹 Nitpick comments (1)
supabase/migrations/20260508122137_fix_app_versions_trigger_owner_org.sql (1)
15-21: Add the required execution-model and EXPLAIN evidence for this helper.This function now sits on the
app_versionswrite path, but the supplied PR notes only cover functional verification. Please add where it runs, how often it runs, expected cardinality/indexes, andEXPLAIN (ANALYZE, BUFFERS)for the worst-case lookup.As per coding guidelines, “Every PostgreSQL function must be proven to scale before shipping; document execution model (where it runs, how often, which roles, cardinality, and indexes) and provide EXPLAIN (ANALYZE, BUFFERS) output for worst-case scenarios in PR notes”.
🤖 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/migrations/20260508122137_fix_app_versions_trigger_owner_org.sql` around lines 15 - 21, Document execution model and worst‑case query evidence for the helper function get_owner_org_by_app_id_internal: update the PR notes to state where it runs (the app_versions write path/trigger), how often (per app_versions write), which role/privileges run it (SECURITY DEFINER caller context), expected cardinality (number of rows in public.apps and expected selectivity of app_id) and required indexes (ensure public.apps has a unique/index on app_id); then run and paste EXPLAIN (ANALYZE, BUFFERS) for the worst‑case lookup against public.apps (large table, no useful cache) and include any follow‑ups (e.g., create an index on apps.app_id or change query to use indexed column) so reviewers can confirm the function scales.
🤖 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/20260508122137_fix_app_versions_trigger_owner_org.sql`:
- Around line 23-24: The trigger function auto_owner_org_by_app_id currently
runs as SECURITY INVOKER and cannot call get_owner_org_by_app_id_internal
(EXECUTE revoked from PUBLIC); change the trigger function definition to be
SECURITY DEFINER so it runs with the postgres role and can call the internal
helper, i.e., alter or recreate function auto_owner_org_by_app_id(...) with
SECURITY DEFINER (keeping owner as postgres) so the trigger executes with
elevated privileges when invoking get_owner_org_by_app_id_internal.
---
Nitpick comments:
In `@supabase/migrations/20260508122137_fix_app_versions_trigger_owner_org.sql`:
- Around line 15-21: Document execution model and worst‑case query evidence for
the helper function get_owner_org_by_app_id_internal: update the PR notes to
state where it runs (the app_versions write path/trigger), how often (per
app_versions write), which role/privileges run it (SECURITY DEFINER caller
context), expected cardinality (number of rows in public.apps and expected
selectivity of app_id) and required indexes (ensure public.apps has a
unique/index on app_id); then run and paste EXPLAIN (ANALYZE, BUFFERS) for the
worst‑case lookup against public.apps (large table, no useful cache) and include
any follow‑ups (e.g., create an index on apps.app_id or change query to use
indexed column) so reviewers can confirm the function scales.
🪄 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: 5a53993c-b6af-48b7-b31e-519436ad1a87
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
supabase/functions/_backend/public/apikey/post.tssupabase/migrations/20260508122137_fix_app_versions_trigger_owner_org.sqlsupabase/schemas/prod.sql
The trigger function was SECURITY INVOKER, so it ran under the caller's role (authenticated/anon) and could not call get_owner_org_by_app_id_internal after EXECUTE was revoked from PUBLIC. Making the trigger SECURITY DEFINER lets it run as postgres (the owner), which can call the internal helper without any grants to unprivileged roles. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
2 participants
Summary
Bug 1 — null owner_org (23502): The
BEFORE INSERTtrigger onapp_versionscalledget_user_main_org_id_by_app_idwhich, since migration20260203150000, runs auth checks that fail under PostgREST'sauthenticatorrole (auth.uid() = NULL,auth.role() = 'anon'). The trigger fires after RLS has already validated the insert, so re-checking auth is redundant and causesowner_orgto be set toNULL. Fix: new minimal SECURITY DEFINER helperget_owner_org_by_app_id_internalthat resolvesowner_orgfromappswithout auth logic.Bug 2 — no read access:
get_organization_cli_warningsrequiresorg.read. API keys created with only app-level bindings (e.g.app_uploader) don't haveorg_memberand therefore lackorg.read. For users,org_member + app_uploaderare always created together — API keys must respect the same invariant. Fix:post.tsnow auto-inserts anorg_memberbinding for each org that has app-level bindings but no explicit org-level binding.Both
bundle list(unaffected) andbundle upload(broken) used the same API keys, confirming auth was valid but upload-specific paths were broken.Test plan
app_uploaderbinding (no org role selected in the UI)role_bindingsnow includes anorg_memberentry for the orgcapgo bundle uploadwith that key → no "no read access" errorcapgo bundle uploadwith any valid key →app_versions.owner_orgis never NULLcapgo bundle list→ still worksorg_memberneed to be recreated by affected users🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Security