fix(api): allow scoped keys past cli warnings#2075
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a SECURITY DEFINER RPC ChangesCLI Warning Generation for Scoped API Keys
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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/20260507171200_skip_cli_warning_read_fatal_for_scoped_keys.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 |
riderx
force-pushed
the
codex/fix-cli-warning-scoped-api-keys
branch
from
acd095b to
ab517b5
Compare
![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: acd095b4a7
ℹ️ 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".
![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
🤖 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/20260507171200_skip_cli_warning_read_fatal_for_scoped_keys.sql`:
- Around line 34-39: The migration uses unqualified built-in functions
array_append and jsonb_build_object inside a SECURITY DEFINER function with
search_path = '', so update the calls to use the pg_catalog schema
(pg_catalog.array_append and pg_catalog.jsonb_build_object) to avoid relying on
search_path and prevent potential privilege escalation; change every occurrence
of array_append(...) and jsonb_build_object(...) in the migration to their
pg_catalog-qualified forms and keep the argument order/structure identical.
🪄 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: 0cecc63e-b959-468b-b5d9-b7902b3b18e7
📒 Files selected for processing (2)
supabase/migrations/20260507171200_skip_cli_warning_read_fatal_for_scoped_keys.sqltests/rbac-permissions.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4ae1bd553
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
adamsardo
left a comment
adamsardo
left a comment
There was a problem hiding this comment.
Found one authorization edge case worth tightening before merge.
get_organization_cli_warnings now returns an empty warning array whenever check_min_rights(... read ...) is false but the supplied API key exists and is not expired. That fixes the app-scoped upload case, but it also means any unrelated active API key can call this RPC for an org it has no read/upload relationship to and avoid the previous fatal API key does not have read access to this organization response.
The regression test covers a scoped key whose limited_to_orgs includes the target org, so it does not catch the unrelated-key path. A safer boundary would be to skip the fatal warning only when the key is non-expired and is plausibly scoped to the requested org, e.g. orgid = ANY(api_key.limited_to_orgs) or one of api_key.limited_to_apps resolves to an app owned by orgid; otherwise keep returning the fatal warning. That preserves app-scoped CI uploads without silently treating a wrong valid key as acceptable for this org.
|
I think this can accidentally remove the only storage-limit enforcement for app-scoped uploads. Could we either keep the storage-plan warning/check even when skipping the org-read warning, or add the same storage entitlement check to the upload/bundle creation path? A regression with storage over limit + app-scoped key + no org read would make this clear. |
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked the latest head (5f70410) and the authorization/plan boundary concerns are still present in the current SQL. get_organization_cli_warnings() returns early whenever org read is denied and the supplied API key merely exists and is not expired. It does not prove that the key is scoped to the requested org, nor that one of its app scopes belongs to that org.
That keeps the unrelated-active-key path open: a valid key for some other org can avoid the fatal API key does not have read access to this organization warning for this org. The new regression only covers the intended app-scoped key case, so it does not protect that boundary.
The same early return also still skips the storage-plan warning path. If the later bundle creation path is meant to be the storage entitlement gate, this PR should add/point to that enforcement and cover it; otherwise app-scoped uploads without org read can bypass the fatal storage warning generated here.
git diff --check origin/main...origin/pr-2075 passes locally, but I would keep this blocked until the early-return is limited to keys actually related to the requested org/app and the storage-limit behavior is covered by a regression or enforced later in the upload path.
4 participants
Summary (AI generated)
get_organization_cli_warningsso app-scoped API keys are not blocked by an org-level read warning gate.Motivation (AI generated)
The upload command calls the org CLI warning RPC before the actual app-scoped upload permission check. New API keys can be valid for app administration or upload without org-level read access, so the warning RPC was incorrectly failing before upload could use the correct permission path.
Business Impact (AI generated)
This restores bundle uploads for customers using scoped API keys in CI/CD while preserving existing plan and upload permission checks. It reduces false authorization failures for the new API key model.
Test Plan (AI generated)
bun lintbunx vitest run tests/rbac-permissions.test.tsbun run cli:build && vue-tsc --noEmitGenerated with AI
Summary by CodeRabbit
New Features
Tests