[codex] Fix create_device org scope advisory#1997
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request hardens authorization for app-scoped operations by adding explicit owner organization validation at the API handler level and enhancing the database-level Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as API Handler<br/>(create_device)
participant DB as Database<br/>(check_min_rights)
participant RBAC as RBAC Engine
participant Result as Authorization<br/>Result
Client->>Handler: POST /private/create_device<br/>(org_id, app_id)
Handler->>DB: Query app by app_id<br/>(returns app + owner_org)
DB-->>Handler: app {owner_org, ...}
Handler->>Handler: Check: app.owner_org<br/>== request.org_id?
alt Org Mismatch
Handler-->>Client: 401 not_authorized
else Org Match
Handler->>DB: check_min_rights(...)<br/>for app scope
DB->>DB: Resolve effective org<br/>from app/channel/org
DB->>DB: Enforce 2FA & password<br/>policy checks
alt Legacy Auth Path
DB->>DB: Check user min_right
else RBAC Path
DB->>RBAC: Map min_right→permission
RBAC->>RBAC: Verify user principal<br/>has permission
alt Permission Denied
DB->>DB: Validate API-key<br/>org/app scoping
RBAC-->>DB: Grant/Deny via<br/>API-key principal
end
end
DB-->>Handler: Authorization result
alt Authorized
Handler->>Handler: createStatsDevices()
Handler-->>Client: 200 {status: ok}
else Denied
Handler-->>Client: 401 not_authorized
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
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)
Review rate limit: 0/5 reviews remaining, refill in 2 minutes and 11 seconds. Comment |
![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: 055bf15140
ℹ️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/migrations/20260430145518_enforce_check_min_rights_app_org_scope.sql (1)
139-140: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRe-assert
EXECUTEprivileges in this migration.This replaces a callable
SECURITY DEFINERfunction, but the migration still leaves function permissions implicit. Please add explicitREVOKE ALL ... FROM PUBLICand the intendedGRANT EXECUTE ...statements here so the privilege boundary is declared in the migration itself.As per coding guidelines, "Apply explicit REVOKE ALL and GRANT statements for function permissioning; start from deny-by-default and grant only required roles; set OWNER explicitly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260430145518_enforce_check_min_rights_app_org_scope.sql` around lines 139 - 140, Add explicit function permissioning for the replaced SECURITY DEFINER routine: after altering owner of the public.check_min_rights(min_right,user_id,org_id,app_id,channel_id) function, add a REVOKE ALL ON FUNCTION "public"."check_min_rights"("public"."user_min_right","uuid","uuid","character varying","bigint") FROM PUBLIC; then add the intended GRANT EXECUTE ON FUNCTION "public"."check_min_rights"(...) TO the specific role(s) that should call it (e.g., authenticated_service_role or postgres as applicable). Ensure the REVOKE uses the exact signature and the GRANT uses the same signature so the migration explicitly declares the deny-by-default boundary.
🧹 Nitpick comments (1)
tests/private-error-cases.test.ts (1)
62-118: ⚡ Quick winUse
it.concurrent()for these new cases.These additions are independent and already use unique IDs, so they can follow the repo’s test rule and run concurrently.
As per coding guidelines, "Design all tests for parallel execution across files; use it.concurrent() instead of it() to maximize parallelism within test files."Suggested fix
- it('should reject app/org scope mismatch in permission checks', async () => { + it.concurrent('should reject app/org scope mismatch in permission checks', async () => { // ... }) - it('should return 401 when org_id does not own the app', async () => { + it.concurrent('should return 401 when org_id does not own the app', async () => { // ... }) - it('should allow device creation when org_id owns the app', async () => { + it.concurrent('should allow device creation when org_id owns the app', async () => { // ... })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/private-error-cases.test.ts` around lines 62 - 118, Replace the three synchronous tests that use it(...) with concurrent tests by changing them to it.concurrent(...): the tests titled "should reject app/org scope mismatch in permission checks", "should return 401 when org_id does not own the app", and "should allow device creation when org_id owns the app"; this keeps the same test bodies and unique IDs while enabling parallel execution per the repo guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/private-error-cases.test.ts`:
- Around line 93-97: The method chain on the Supabase query is failing ESLint
due to broken chain formatting; reformat the call so each chained method starts
on its own line with the leading dot aligned (e.g.
getSupabaseClient().from('devices').select('device_id') followed by each
.eq('app_id', APPNAME), .eq('device_id', deviceId) and finally .maybeSingle() on
its own line) and ensure the entire expression ends with a semicolon; update the
lines around getSupabaseClient().from('devices').select and .maybeSingle to
follow the project's chain-formatting style.
---
Outside diff comments:
In
`@supabase/migrations/20260430145518_enforce_check_min_rights_app_org_scope.sql`:
- Around line 139-140: Add explicit function permissioning for the replaced
SECURITY DEFINER routine: after altering owner of the
public.check_min_rights(min_right,user_id,org_id,app_id,channel_id) function,
add a REVOKE ALL ON FUNCTION
"public"."check_min_rights"("public"."user_min_right","uuid","uuid","character
varying","bigint") FROM PUBLIC; then add the intended GRANT EXECUTE ON FUNCTION
"public"."check_min_rights"(...) TO the specific role(s) that should call it
(e.g., authenticated_service_role or postgres as applicable). Ensure the REVOKE
uses the exact signature and the GRANT uses the same signature so the migration
explicitly declares the deny-by-default boundary.
---
Nitpick comments:
In `@tests/private-error-cases.test.ts`:
- Around line 62-118: Replace the three synchronous tests that use it(...) with
concurrent tests by changing them to it.concurrent(...): the tests titled
"should reject app/org scope mismatch in permission checks", "should return 401
when org_id does not own the app", and "should allow device creation when org_id
owns the app"; this keeps the same test bodies and unique IDs while enabling
parallel execution per the repo guideline.
🪄 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: ad4a3caa-6c9a-445d-bf49-35b08262220a
📒 Files selected for processing (3)
supabase/functions/_backend/private/create_device.tssupabase/migrations/20260430145518_enforce_check_min_rights_app_org_scope.sqltests/private-error-cases.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: babf917405
ℹ️ 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".
riderx
force-pushed
the
codex/fix-ghsa-create-device-org-scope
branch
from
7c6d1f7 to
a58c0e7
Compare
|
1 participant
Summary (AI generated)
/private/create_devicebefore device state is written.public.check_min_rights()so existing app-scoped checks reject a foreignorg_id.Motivation (AI generated)
GHSA-mhrc-qhq8-872f showed that a caller could supply an org they control while targeting an app owned by another org they can access, allowing device creation with an inconsistent authorization scope.
Business Impact (AI generated)
This closes an authenticated authorization flaw in device telemetry creation and protects app/org scope integrity for Capgo customers.
Test Plan (AI generated)
bun lintbun lint:backendbun run supabase:with-env -- bunx vitest run tests/private-error-cases.test.tsbun test:backendGenerated with AI
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests