fix: pass appid through plan-check RPC so RBAC keys can upload#2282
Conversation
The CLI's checkPlanValidUpload calls is_allowed_action_org_action without app_id, so check_min_rights -> rbac_check_permission_direct receives p_app_id = NULL. For an API key with limited_to_apps set (typical for RBAC-managed keys), the app-scope restriction in rbac_check_permission_direct denies the call because array_length(limited_to_apps, 1) > 0 AND v_effective_app_id IS NULL. The CLI surfaces this RBAC denial as "Plan upgrade required for upload" even though the org's plan is healthy. This change adds a 3-arg overload to both is_allowed_action_org_action and is_paying_and_good_plan_org_action that accepts appid and threads it through check_min_rights, then updates the CLI upload path to pass appid when it has one. The existing 2-arg signatures are preserved so older clients continue to work. Reproduced via app@matsuri-tech.com / tech.matsuri.cleaning with an RBAC-managed "codemagic" key (mode=NULL, limited_to_apps=[that app]).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR extends plan validation to accept an optional app-scope context. A new migration adds 3-argument overloads to ChangesPlan validation app-scope context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/utils.ts (1)
846-855:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle RPC errors before treating this as a bad plan.
If this new RPC fails,
validPlanis falsy and the user still gets “Plan upgrade required for upload”. That keeps permission/migration failures indistinguishable from actual billing failures.🔧 Proposed fix
- const { data: validPlan } = await supabase.rpc('is_allowed_action_org_action', { orgid: orgId, actions: ['storage'], appid: appId }) + const { data: validPlan, error: validPlanError } = await supabase.rpc('is_allowed_action_org_action', { + orgid: orgId, + actions: ['storage'], + appid: appId, + }) + if (validPlanError) { + const message = `Cannot validate upload plan: ${formatError(validPlanError)}` + log.error(message) + throw new Error(message) + } if (!validPlan) {As per coding guidelines: "For user-visible error messages, format errors with
formatError(...)instead of dumping raw exceptions when possible."🤖 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 `@cli/src/utils.ts` around lines 846 - 855, The RPC call to supabase.rpc('is_allowed_action_org_action', ...) ignores RPC errors and treats any falsy validPlan as a billing issue; change the destructuring to capture the RPC error (e.g., { data: validPlan, error }) and handle it first: if error exists log and throw the formatted error using formatError(error) so permission/migration failures surface correctly, otherwise proceed to check if !validPlan and keep the existing user-facing log.error/redirect behavior; update references around the supabase.rpc call, validPlan, and the user-facing log.error to reflect this flow.
🤖 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 `@cli/src/types/supabase.types.ts`:
- Around line 3789-3804: The RPC overload for is_paying_and_good_plan_org_action
is out of sync with the DB change that added an appid variant (similar to
is_allowed_action_org_action); update the RPC typings to add the overload that
accepts Args { actions: Database["public"]["Enums"]["action_type"][], appid:
string | null, orgid: string } and Returns: boolean for
is_paying_and_good_plan_org_action (match the pattern used by
is_allowed_action_org_action), then regenerate the Supabase TypeScript schema by
running the project type generation command (bun types) so the file and DB
surface stay in sync.
In `@supabase/migrations/20260518071442_plan_check_passthrough_appid.sql`:
- Line 25: In the SECURITY DEFINER function in this migration
(plan_check_passthrough_appid.sql) fully qualify the PostgreSQL built-ins:
replace uses of current_setting and now with pg_catalog.current_setting and
pg_catalog.now respectively, and ensure the function sets an empty search_path;
update any call sites inside the function that reference current_setting or now
to use the pg_catalog-qualified names to comply with the repo rule and prevent
SQL injection.
- Around line 125-139: The migration currently REVOKEs ALL privileges for anon
on the new 3-argument overload of public.is_allowed_action_org_action("orgid"
uuid, "actions" public.action_type [], "appid" character varying), which
prevents CLI/API-key RPCs (that run as anon) from reaching the wrapper; restore
access by adding a GRANT EXECUTE ON FUNCTION
public.is_allowed_action_org_action("orgid" uuid, "actions" public.action_type
[], "appid" character varying) TO anon so the CLI path can call the overload
before its body runs.
---
Outside diff comments:
In `@cli/src/utils.ts`:
- Around line 846-855: The RPC call to
supabase.rpc('is_allowed_action_org_action', ...) ignores RPC errors and treats
any falsy validPlan as a billing issue; change the destructuring to capture the
RPC error (e.g., { data: validPlan, error }) and handle it first: if error
exists log and throw the formatted error using formatError(error) so
permission/migration failures surface correctly, otherwise proceed to check if
!validPlan and keep the existing user-facing log.error/redirect behavior; update
references around the supabase.rpc call, validPlan, and the user-facing
log.error to reflect this flow.
🪄 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: 7f3f15fd-feb8-4ec9-ade0-c399cfc2abd3
📒 Files selected for processing (3)
cli/src/types/supabase.types.tscli/src/utils.tssupabase/migrations/20260518071442_plan_check_passthrough_appid.sql
| result boolean; | ||
| has_credits boolean; | ||
| BEGIN | ||
| SELECT current_setting('role', true) INTO caller_role; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Fully qualify the pg_catalog calls in this SECURITY DEFINER function.
current_setting on Line 25 and now on Line 54 are still unqualified. This repo’s migration rule requires fully qualified references inside PostgreSQL functions.
🔧 Proposed fix
- SELECT current_setting('role', true) INTO caller_role;
+ SELECT pg_catalog.current_setting('role', true) INTO caller_role;
...
- SELECT (si.trial_at > now()) OR (si.status = 'succeeded' AND NOT (
+ SELECT (si.trial_at > pg_catalog.now()) OR (si.status = 'succeeded' AND NOT (As per coding guidelines: "Every PostgreSQL function must set an empty search_path and use fully qualified names for all references to prevent SQL injection vulnerabilities."
Also applies to: 54-54
🤖 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/20260518071442_plan_check_passthrough_appid.sql` at line
25, In the SECURITY DEFINER function in this migration
(plan_check_passthrough_appid.sql) fully qualify the PostgreSQL built-ins:
replace uses of current_setting and now with pg_catalog.current_setting and
pg_catalog.now respectively, and ensure the function sets an empty search_path;
update any call sites inside the function that reference current_setting or now
to use the pg_catalog-qualified names to comply with the repo rule and prevent
SQL injection.
Addresses CodeRabbit review feedback on the appid passthrough change: 1. GRANT EXECUTE ... TO anon on the new 3-arg is_allowed_action_org_action overload. The CLI authenticates per-call via the capgkey header and connects with the Supabase anon key, so PostgREST sets role = anon. Without this grant the upload path would 403 before the function body even runs. The existing 2-arg overload already grants to anon. 2. Capture and surface RPC errors in checkPlanValidUpload. Previously a permission/migration failure was indistinguishable from a real billing failure - both surfaced as "Plan upgrade required for upload". Now non-billing errors throw with formatError() so they can be diagnosed. 3. Mirror the appid overload in the supabase.types.ts entry for is_paying_and_good_plan_org_action to match the DB-level signature. Not called directly from the CLI, but keeps the types accurate. Skipped: CodeRabbit also suggested pg_catalog-qualifying current_setting and now(). No repo-wide rule for this; matches the existing is_paying_and_good_plan_org_action style. With SET search_path = '' the pg_catalog schema is still implicitly first in the resolution order.
|
Triaged CodeRabbit's review and addressed in
On #4: there's no explicit repo rule for CI re-running on |
Confirmed empirically by spinning up local supabase, applying the migration, and running bunx supabase gen types typescript --local: the generator does NOT emit the 3-arg overload for is_allowed_action_org_action or is_paying_and_good_plan_org_action, even though both overloads exist in the DB and PostgREST routes to each correctly at runtime. (Curiously, is_allowed_capgkey - which has the same overload shape - does get both variants emitted. Looks like a gen-types limitation we cannot easily fix from the migration.) Since the next auto-sync (chore(auto-sync): update supabase schema and generated types) would just revert any manual overload edit, this commit: 1. Restores cli/src/types/supabase.types.ts to match what the generator produces (no manual overloads). 2. Casts the 3-arg args object to `never` at the call site so TypeScript accepts it. The runtime call still routes to the 3-arg DB function via PostgREST. This decouples the CLI from gen-types' overload-handling quirk and keeps the file auto-sync-stable.
|
Follow-up on types: I started a local Supabase, applied all migrations including the new one, and ran
Looks like a Since the repo's
This keeps the build green regardless of what the auto-sync does next, and isolates the gen-types quirk to one well-commented line of code. The DB migration is unchanged — the 3-arg overload is the source of truth. Lint / typecheck / build / tests all green locally. |
Three assertions, all running against PostgREST as anon with a capgkey
header (the same path the CLI takes for uploads):
1. The 2-arg is_allowed_action_org_action call still denies when the
key has limited_to_apps set and use_new_rbac is on for the org.
This is the original bug repro - the CLI's checkPlanValidUpload
called this variant and tripped the RBAC app-scope restriction.
2. The new 3-arg overload with a matching appid returns true. This is
the fix - threading appid into check_min_rights gives RBAC the
context it needs to satisfy the limited_to_apps check.
3. The 3-arg overload with a *non-matching* appid (different app in
the same org) still denies. This guards the safety invariant: a
key restricted to app A still cannot be used to authorize uploads
for app B, regardless of org membership.
Test setup notes:
- The apikeys_force_server_key BEFORE trigger overrides whatever we
pass in for `key` with a server-generated UUID. We read the actual
plaintext back from INSERT ... RETURNING (which executes before the
DEFERRABLE INITIALLY DEFERRED apikeys_strip_plain_key_for_hashed
trigger runs at commit), so we hold the real key to send via the
capgkey header.
- Two same-org apps prove that the rejection in test #3 comes from
limited_to_apps, not from the cross-org guard in check_min_rights.
Verified the test actually fails when the migration is rolled back:
PostgREST returns PGRST202 ("Could not find the function
public.is_allowed_action_org_action(actions, appid, orgid)") for the
two 3-arg cases. With the migration applied, all three pass.
|
Added a regression test in
Validation that the test is actually testing the fixI rolled the migration back locally (dropped the 3-arg overload) and re-ran the test. PostgREST returned Test setup gotcha worth notingThe Local: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/plan-check-appid-passthrough.test.ts (1)
248-286: ⚡ Quick winUse
it.concurrentfor all three test cases in this file.Line 248, Line 261, and Line 273 use
it(...); switch them toit.concurrent(...)to match the test parallelization rule for.test.tsfiles.Suggested diff
- it('denies the 2-arg call (regression of the original bug)', async () => { + it.concurrent('denies the 2-arg call (regression of the original bug)', async () => { @@ - it('allows the 3-arg call when appid matches limited_to_apps (the fix)', async () => { + it.concurrent('allows the 3-arg call when appid matches limited_to_apps (the fix)', async () => { @@ - it('still denies the 3-arg call when appid does not match limited_to_apps (safety invariant)', async () => { + it.concurrent('still denies the 3-arg call when appid does not match limited_to_apps (safety invariant)', async () => {As per coding guidelines:
tests/**/*.test.ts: “Design all tests for parallel execution across files; use it.concurrent() instead of it() to run tests in parallel within the same file for faster CI/CD”.🤖 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 `@tests/plan-check-appid-passthrough.test.ts` around lines 248 - 286, Change the three synchronous test declarations to run concurrently: replace it(...) with it.concurrent(...) for the tests whose descriptions are "denies the 2-arg call (regression of the original bug)", "allows the 3-arg call when appid matches limited_to_apps (the fix)", and "still denies the 3-arg call when appid does not match limited_to_apps (safety invariant)"; update each test declaration in the file tests/plan-check-appid-passthrough.test.ts so the calls to createCapgkeyClient(...) and subsequent rpc(...) assertions run under it.concurrent to comply with the tests/**/*.test.ts parallelization guideline.
🤖 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 `@tests/plan-check-appid-passthrough.test.ts`:
- Around line 248-286: Change the three synchronous test declarations to run
concurrently: replace it(...) with it.concurrent(...) for the tests whose
descriptions are "denies the 2-arg call (regression of the original bug)",
"allows the 3-arg call when appid matches limited_to_apps (the fix)", and "still
denies the 3-arg call when appid does not match limited_to_apps (safety
invariant)"; update each test declaration in the file
tests/plan-check-appid-passthrough.test.ts so the calls to
createCapgkeyClient(...) and subsequent rpc(...) assertions run under
it.concurrent to comply with the tests/**/*.test.ts parallelization guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99574c97-4871-4d6f-ae5b-1c7b8641976d
📒 Files selected for processing (1)
tests/plan-check-appid-passthrough.test.ts
Supabase generated types mark apps.id, apikeys.rbac_id, and apikeys.key as nullable, so assigning their RETURNING values to local string variables tripped vue-tsc in CI (TS2322). Adds explicit null checks at each insert site - they double as documentation that the test's invariants depend on those columns being populated, and fail loudly with a clear message if a future schema change makes them nullable in practice.
|
2 participants
Summary
appidparameter) tois_allowed_action_org_actionandis_paying_and_good_plan_org_action, threadingappidintocheck_min_rights.checkPlanValidUploadto passappid(it already received it but was discarding it).Bug
The CLI's
checkPlanValidUploadcallsis_allowed_action_org_action(orgid, ['storage'])without anapp_id. The call chain reachesrbac_check_permission_directwithp_app_id = NULL. For an API key withlimited_to_appsset (typical for RBAC-managed keys), the app-scope restriction block:…denies because
array_length > 0 AND v_effective_app_id IS NULL. The CLI surfaces this RBAC denial as the misleading"Plan upgrade required for upload"error, even though the org's plan is perfectly healthy.Safety invariant preserved
A key with
limited_to_apps = [A]still cannot perform app-B operations:rbac_check_permission_directstill enforceslimited_to_appswhenever anapp_idis supplied. Org-scope reads now correctly thread the caller'sapp_id(when known) so the restriction has the context it needs.Backward compatibility
Both
is_allowed_action_org_action(orgid, actions)andis_paying_and_good_plan_org_action(orgid, actions)keep their existing 2-arg signatures. The new 3-arg overloads sit alongside them, so older deployed clients keep working unchanged.Workaround for affected users on old CLI
Recreate the API key without
limited_to_apps(scope by org only), or remove the app restriction on the existing key. This unblocks them until they upgrade.Test plan
bun run cli:lint— cleanbun run cli:typecheck— cleanbun run cli:build— succeedsbun run cli:test— 13/13 passinglimited_to_appsset to confirm the upload now succeedslimited_to_apps = [A]is still denied if attempting to use it for app B's upload (app.upload_bundlepath remains restricted)Out of scope
Other call-sites of
is_allowed_action_org_action/is_allowed_action_org(bandwidth, MAU, dashboard reads) may have the same trap. Worth a follow-up audit, but not bundled here to keep this PR surgical.Summary by CodeRabbit
Bug Fixes
Improvements
Tests