feat(cli-warnings): fail fast when CLI < 7.107.0 hits the PR #2282 RBAC bug#2302
feat(cli-warnings): fail fast when CLI < 7.107.0 hits the PR #2282 RBAC bug#2302WcaleNieWolny wants to merge 1 commit into
Conversation
#2282 RBAC bug Adds a fail-fast warning in get_organization_cli_warnings that fires when ALL of these are true for the caller: 1. API key is RBAC v2 (apikeys.mode IS NULL). 2. Key has limited_to_apps set. 3. Org has use_new_rbac = true. 4. CLI version parses as < 7.107.0. These are the exact conditions that trigger the appid-passthrough bug fixed in #2282. Without this warning, affected users see the misleading "Plan upgrade required for upload" billing error and waste time chasing plan/Stripe state when the real issue is RBAC denying the call because the CLI didn't thread appid through the plan check. checkRemoteCliMessages runs BEFORE checkPlanValidUpload in the upload flow (cli/src/bundle/upload.ts), so the new fatal warning replaces the useless billing error with one that names the bug, the upgrade target (@capgo/cli@7.107.0), and the workaround (drop limited_to_apps on the key) - the same workaround documented in the wiki triage runbook. Scope deliberately narrowed to RBAC v2 keys per request - legacy mode keys ('all','read','write','upload') with limited_to_apps would hit the same bug, but we'd rather start tight and broaden later if needed. Test (tests/cli-warning-rbac-appid-bug.test.ts) covers the full condition matrix: fires for the bug case, silent on the cutoff, above the cutoff, legacy mode, no app restriction, use_new_rbac=false, and unparseable version strings (dev/next builds).
📝 WalkthroughWalkthroughThis PR introduces a database RPC function to detect and warn CLI users about a specific RBAC v2 key incompatibility bug affecting versions below 7.107.0, along with a comprehensive regression test suite that validates the warning fires under the correct API key and organization conditions. ChangesRBAC CLI version bug warning
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (version < 7.107.0)
participant AuthClient as Supabase Client
participant RPC as get_organization_cli_warnings RPC
participant AuthDB as Auth DB
participant OrgDB as Org DB
CLI->>AuthClient: call RPC with orgid, cli_version
AuthClient->>RPC: execute (SECURITY DEFINER)
RPC->>AuthDB: check API key org read permission
AuthDB-->>RPC: org read permission granted
RPC->>OrgDB: check org RBAC v2 enabled, app-restricted key, org paying
OrgDB-->>RPC: conditions match RBAC bug criteria
RPC->>RPC: parse cli_version X.Y.Z, compare < 7.107.0
RPC-->>AuthClient: return fatal warning array containing RBAC bug message
AuthClient-->>CLI: display fatal warning to user
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.2.1)supabase/migrations/20260520040202_cli_warning_rbac_appid_bug.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 |
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
🧹 Nitpick comments (1)
supabase/migrations/20260520040202_cli_warning_rbac_appid_bug.sql (1)
41-41: ⚡ Quick winRemove dead code:
PERFORM cli_version;is a no-op.This line executes
PERFORMon a parameter reference, which evaluates the expression and discards the result. Sincecli_versionis already a function parameter, this does nothing useful.♻️ Proposed fix
BEGIN - PERFORM cli_version; - has_org_read := public.cli_check_permission(🤖 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/20260520040202_cli_warning_rbac_appid_bug.sql` at line 41, The statement PERFORM cli_version; is a no-op because cli_version is already a function parameter; remove that line from the migration so the function no longer evaluates-and-discards the parameter. Locate the PERFORM cli_version; statement in the migration and delete it (leave any surrounding logic intact) so the code no longer executes the redundant PERFORM on the cli_version parameter.
🤖 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/20260520040202_cli_warning_rbac_appid_bug.sql`:
- Around line 111-113: Update the comment text in the migration SQL where the
phrase "Unparseable versions (empty string, "dev","next")" appears: change the
word "Unparseable" to the standard spelling "Unparsable" so the comment reads
"Unparsable versions (empty string, "dev", "next")". Locate and edit that
comment block (the multi-line comment containing "Parse leading X.Y.Z.") to
apply the correction.
In `@tests/cli-warning-rbac-appid-bug.test.ts`:
- Line 295: The test description string in the it() block currently uses
"unparseable" but should use "unparsable"; update the test name in the it('does
NOT fire on unparseable CLI version strings (dev/next builds)') to use
"unparsable" so it reads it('does NOT fire on unparsable CLI version strings
(dev/next builds)') to fix the typo and keep spelling consistent with the
migration file.
- Line 143: The test suite title in the describe.skipIf call ("CLI warning: RBAC
v2 + limited_to_apps + old CLI (PR `#2282`)") starts with an uppercase letter
which violates the lint rule; update the string passed to describe.skipIf to
start with a lowercase character (e.g., change "CLI warning..." to "cli
warning...") so the describe title begins with lowercase, leaving the rest of
the text unchanged and keeping the same describe.skipIf invocation.
---
Nitpick comments:
In `@supabase/migrations/20260520040202_cli_warning_rbac_appid_bug.sql`:
- Line 41: The statement PERFORM cli_version; is a no-op because cli_version is
already a function parameter; remove that line from the migration so the
function no longer evaluates-and-discards the parameter. Locate the PERFORM
cli_version; statement in the migration and delete it (leave any surrounding
logic intact) so the code no longer executes the redundant PERFORM on the
cli_version parameter.
🪄 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: 93c9f4fc-0c13-4ed1-a009-cc2811fd6f95
📒 Files selected for processing (2)
supabase/migrations/20260520040202_cli_warning_rbac_appid_bug.sqltests/cli-warning-rbac-appid-bug.test.ts
| -- Parse leading X.Y.Z. Unparseable versions (empty string, "dev", | ||
| -- "next") fall through without firing the warning - safer to be | ||
| -- silent than to nag on non-release builds. |
There was a problem hiding this comment.
Minor typo: "Unparseable" → "Unparsable".
Static analysis flagged the spelling. The standard form is "unparsable".
📝 Proposed fix
- -- Parse leading X.Y.Z. Unparseable versions (empty string, "dev",
+ -- Parse leading X.Y.Z. Unparsable versions (empty string, "dev",📝 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.
| -- Parse leading X.Y.Z. Unparseable versions (empty string, "dev", | |
| -- "next") fall through without firing the warning - safer to be | |
| -- silent than to nag on non-release builds. | |
| -- Parse leading X.Y.Z. Unparsable versions (empty string, "dev", | |
| -- "next") fall through without firing the warning - safer to be | |
| -- silent than to nag on non-release builds. |
🧰 Tools
🪛 GitHub Check: Run tests
[warning] 111-111:
"Unparseable" should be "Unparsable".
🤖 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/20260520040202_cli_warning_rbac_appid_bug.sql` around
lines 111 - 113, Update the comment text in the migration SQL where the phrase
"Unparseable versions (empty string, "dev","next")" appears: change the word
"Unparseable" to the standard spelling "Unparsable" so the comment reads
"Unparsable versions (empty string, "dev", "next")". Locate and edit that
comment block (the multi-line comment containing "Parse leading X.Y.Z.") to
apply the correction.
| await serviceRoleSupabase.from('apikeys').delete().eq('id', handle.id) | ||
| } | ||
|
|
||
| describe.skipIf(USE_CLOUDFLARE_WORKERS)('CLI warning: RBAC v2 + limited_to_apps + old CLI (PR #2282)', () => { |
There was a problem hiding this comment.
ESLint: describe title should begin with lowercase.
Per project linting rules, test suite descriptions should start with a lowercase letter.
📝 Proposed fix
-describe.skipIf(USE_CLOUDFLARE_WORKERS)('CLI warning: RBAC v2 + limited_to_apps + old CLI (PR `#2282`)', () => {
+describe.skipIf(USE_CLOUDFLARE_WORKERS)('cli warning: RBAC v2 + limited_to_apps + old CLI (PR `#2282`)', () => {📝 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.
| describe.skipIf(USE_CLOUDFLARE_WORKERS)('CLI warning: RBAC v2 + limited_to_apps + old CLI (PR #2282)', () => { | |
| describe.skipIf(USE_CLOUDFLARE_WORKERS)('cli warning: RBAC v2 + limited_to_apps + old CLI (PR `#2282`)', () => { |
🧰 Tools
🪛 ESLint
[error] 143-143: describes should begin with lowercase
(test/prefer-lowercase-title)
🤖 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/cli-warning-rbac-appid-bug.test.ts` at line 143, The test suite title
in the describe.skipIf call ("CLI warning: RBAC v2 + limited_to_apps + old CLI
(PR `#2282`)") starts with an uppercase letter which violates the lint rule;
update the string passed to describe.skipIf to start with a lowercase character
(e.g., change "CLI warning..." to "cli warning...") so the describe title begins
with lowercase, leaving the rest of the text unchanged and keeping the same
describe.skipIf invocation.
| } | ||
| }) | ||
|
|
||
| it('does NOT fire on unparseable CLI version strings (dev/next builds)', async () => { |
There was a problem hiding this comment.
Minor typo: "unparseable" → "unparsable".
Static analysis flagged this—same spelling issue as in the migration file.
📝 Proposed fix
- it('does NOT fire on unparseable CLI version strings (dev/next builds)', async () => {
+ it('does NOT fire on unparsable CLI version strings (dev/next builds)', async () => {📝 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.
| it('does NOT fire on unparseable CLI version strings (dev/next builds)', async () => { | |
| it('does NOT fire on unparsable CLI version strings (dev/next builds)', async () => { |
🧰 Tools
🪛 GitHub Check: Run tests
[warning] 295-295:
"unparseable" should be "unparsable".
🤖 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/cli-warning-rbac-appid-bug.test.ts` at line 295, The test description
string in the it() block currently uses "unparseable" but should use
"unparsable"; update the test name in the it('does NOT fire on unparseable CLI
version strings (dev/next builds)') to use "unparsable" so it reads it('does NOT
fire on unparsable CLI version strings (dev/next builds)') to fix the typo and
keep spelling consistent with the migration file.
|
|
Closing in favor of #2279. After tracing through #2279's migration carefully, the bug this PR warns about is structurally fixed by #2279 — not just patched. The relevant chain:
For an old CLI (< 7.107.0) calling
So the bug can no longer manifest on the old CLI's call path. The 3-arg overload from #2282 stays harmless. The warning this PR adds becomes:
There's a separate concern about a privilege-widening side effect of #2279's migration (previously app-restricted keys gain |
1 participant
Summary
Follow-up to #2282. That PR fixed the appid-passthrough bug on the server side and shipped the CLI fix in
@capgo/cli@7.107.0. This PR adds a fail-fast fatal CLI warning for users still running older CLIs whose API keys would trigger the bug, so they see an actionable message instead of the misleading"Plan upgrade required for upload"billing error.The warning fires only when ALL are true
apikeys.mode IS NULLwith role_bindings).limited_to_appsset (non-empty).use_new_rbac = true.cli_versionparses asX.Y.ZAND sits below7.107.0.These are the exact preconditions for the appid-passthrough denial in
rbac_check_permission_direct. If any are false, the warning stays silent — including unparseable version strings likedev/next(we'd rather not nag on local builds).Why fatal and why now
checkRemoteCliMessagesruns beforecheckPlanValidUploadincli/src/bundle/upload.ts:846vs:850. A fatal warning throws and stops the upload before the misleading billing error can fire.Error: Plan upgrade required for upload+ an auto-opened plans page in the browser. Users waste time inspecting Stripe / their plan when the real issue is RBAC denying for app-context reasons.@capgo/cli@7.107.0), and the workaround (droplimited_to_appson the key, leaving only the org restriction).Why scoped to RBAC v2 keys only
Legacy keys (
mode IN ('read','write','upload','all')) withlimited_to_appson ause_new_rbacorg also hit the bug. I narrowed scope to RBAC v2 keys per the request — start tight, broaden later if support reports the same misclassification for legacy keys. Easy to extend the condition.How the version check works
Postgres
int[]compares lexicographically, so[7,106,0] < [7,107,0]and[7,107,0] < [7,107,0]is false. Pre-release suffixes (7.107.0-beta.1) parse to[7,107,0]and fall the right side of the cutoff.What the user sees
Before (on, say,
@capgo/cli@7.106.0):(browser opens the plans page — completely unrelated to the actual problem)
After:
Test plan
tests/cli-warning-rbac-appid-bug.test.tscovers the full condition matrix end-to-end via PostgREST (anon +capgkeyheader — same path the CLI takes):bun typecheck— cleanbun run cli:lint/cli:typecheck/cli:build— cleanbun run supabase:with-env -- bunx vitest run tests/cli-warning-rbac-appid-bug.test.ts— 7/7 passingOut of scope
Summary by CodeRabbit
New Features