fix: authorize org-scoped console event broadcasts#1818
Conversation
|
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:
📝 WalkthroughWalkthroughReplaced a local org-access helper with RBAC enforcement: when Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Function
participant RBAC as "RBAC / checkPermission"
participant Broadcaster as "Console Broadcaster"
Client->>Function: POST /private/events (body with notifyConsole, user_id, tags)
alt notifyConsole true
Function->>RBAC: checkPermission('org.read', {orgId: user_id})
RBAC-->>Function: allow / deny
alt allow
Function->>Broadcaster: broadcast console event (orgId, tags)
Broadcaster-->>Function: ack
Function-->>Client: 200 OK
else deny
Function-->>Client: error cannot_access_organization (403)
end
else notifyConsole false
Function->>Broadcaster: regular broadcast
Broadcaster-->>Function: ack
Function-->>Client: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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)
tests/events.test.ts (1)
125-164: Add regression coverage for missing org ID guard.The new backend path also rejects
notifyConsolerequests withoutuser_id(missing_org_id), but this branch isn’t tested yet. Adding one test will lock the guard in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events.test.ts` around lines 125 - 164, Add a regression test that submits a POST to the /private/events endpoint with notifyConsole: true but omits user_id and assert the response is HTTP 400 with error 'missing_org_id'; update tests/events.test.ts by adding a new it(...) (similar to the existing "rejects console event broadcast for a foreign org" case) that sends the same payload fields (channel, event, description, notifyConsole: true) but without user_id and verifies response.status === 400 and response.json().error === 'missing_org_id' to lock the org ID guard in place.
🤖 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/events.test.ts`:
- Around line 125-164: Update the two test cases in tests/events.test.ts from
synchronous it(...) to parallel it.concurrent(...): change the 'broadcasts
console event for an authorized org' and 'rejects console event broadcast for a
foreign org' blocks to use it.concurrent so they run in parallel with the test
suite; ensure the same test bodies, names, and assertions remain unchanged but
replace the it symbol with it.concurrent to comply with the project's parallel
test policy.
---
Nitpick comments:
In `@tests/events.test.ts`:
- Around line 125-164: Add a regression test that submits a POST to the
/private/events endpoint with notifyConsole: true but omits user_id and assert
the response is HTTP 400 with error 'missing_org_id'; update
tests/events.test.ts by adding a new it(...) (similar to the existing "rejects
console event broadcast for a foreign org" case) that sends the same payload
fields (channel, event, description, notifyConsole: true) but without user_id
and verifies response.status === 400 and response.json().error ===
'missing_org_id' to lock the org ID guard in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ebd4d07-3929-4081-9db5-962131f5bf7c
📒 Files selected for processing (2)
supabase/functions/_backend/private/events.tstests/events.test.ts
|
CI blocker: the failing |
|
CI still fails on the shared |
|
Run tests fails in |
|
PR #1818: the failing test is |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/private/events.ts (1)
45-50: Consolidate org authorization to a single path fornotifyConsole.There are now two org-access checks for the same flow (precheck at Line 36 and RBAC at Lines 48-50), which can return different error formats/statuses and duplicate permission I/O. A single gate will keep behavior deterministic.
♻️ Suggested consolidation
- if (requestedOrgId && !(await canAccessRequestedOrg(c, requestedOrgId))) - return c.json({ error: 'Forbidden' }, 403) // notifyConsole: broadcast to Supabase Realtime only, skip all tracking if (body.notifyConsole) { - if (!body.user_id) { + if (!requestedOrgId) { throw simpleError('missing_org_id', 'Missing org ID for console notification') } - if (!(await checkPermission(c, 'org.read', { orgId: body.user_id }))) { - throw simpleError('cannot_access_organization', 'You can\'t access this organization', { org_id: body.user_id }) + if (!(await checkPermission(c, 'org.read', { orgId: requestedOrgId }))) { + throw simpleError('cannot_access_organization', 'You can\'t access this organization', { org_id: requestedOrgId }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/events.ts` around lines 45 - 50, The notifyConsole flow currently performs two org-access checks (a precheck and an RBAC check using checkPermission), causing duplicated permission I/O and inconsistent error responses; consolidate to a single gate by removing the redundant precheck and keeping one canonical check: first assert body.user_id exists, then call checkPermission(c, 'org.read', { orgId: body.user_id }) and on failure throw a single consistent simpleError (e.g., 'cannot_access_organization') so all authorization failures follow the same path and format (update the function notifyConsole to use only this single presence+RBAC check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/private/events.ts`:
- Around line 45-50: The notifyConsole flow currently performs two org-access
checks (a precheck and an RBAC check using checkPermission), causing duplicated
permission I/O and inconsistent error responses; consolidate to a single gate by
removing the redundant precheck and keeping one canonical check: first assert
body.user_id exists, then call checkPermission(c, 'org.read', { orgId:
body.user_id }) and on failure throw a single consistent simpleError (e.g.,
'cannot_access_organization') so all authorization failures follow the same path
and format (update the function notifyConsole to use only this single
presence+RBAC check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1567ce87-263c-4ab2-8c41-be7b95eb8e5a
📒 Files selected for processing (2)
supabase/functions/_backend/private/events.tstests/events.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/events.test.ts
![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: f76c97ebb9
ℹ️ 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".
|
Pulled in origin/main and resolved the merge in |
|
@riderx We just need you. Thank you for the pull request. We just need you to reply or fix your pull request according to the AI comments. When the AI reviewer is done and the build passes in the CI, we will merge your pull request. |
|
CI is still red on https://github.com/Cap-go/capgo/actions/runs/23273358364/job/67670834123. The job stops in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cea5914232
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/private/events.ts`:
- Around line 34-35: The 403 branch that currently returns an ad-hoc JSON should
use the shared error helper: instead of returning c.json({ error: 'Forbidden' },
403) after the checkPermission call (see checkPermission and requestedOrgId),
return the standardized error shape via c.json(simpleError('Forbidden', 403))
(or quickError if your codebase prefers quickError signature); add the needed
import for simpleError/quickError and replace the ad-hoc response so all backend
endpoints share the same error shape and status handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b574034-ca78-4f6c-a1e2-ad675baa4f57
📒 Files selected for processing (1)
supabase/functions/_backend/private/events.ts
|
Merged main and resolved the conflict in supabase/functions/_backend/private/events.ts by reintroducing the ResolveTrackingUserId/CanAccessRequestedOrg helpers and aligning the notifyConsole flow with the upstream logic before pushing the branch. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63b8a723b5
ℹ️ 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".
|
1 participant
Summary (AI generated)
/private/eventsconsole broadcasts before usingbody.user_idas a realtime targetnotifyConsolerequests for organizations the caller cannot accessMotivation (AI generated)
The oldest valid triage advisory reports that
/private/eventstrusts a user-controlled org identifier for realtime broadcasts. That allows cross-organization event injection into the CLI feed unless the supplied org is authorized first.Business Impact (AI generated)
This closes a high-severity integrity issue that could let one tenant inject fake deployment activity into another tenant's dashboard and poison org-scoped analytics, reducing incident noise and support risk.
Test Plan (AI generated)
bun lint supabase/functions/_backend/private/events.ts tests/events.test.tsbun run supabase:with-env -- bun test tests/events.test.tsbun run test:backendGenerated with AI
Summary by CodeRabbit