fix(api): prevent cross-org event broadcasts#1802
Conversation
📝 WalkthroughWalkthroughAdds org-level access checks to event broadcasting: when Changes
Sequence DiagramsequenceDiagram
participant Client
participant Backend as Backend/Events
participant Auth as Auth System
Client->>Backend: POST /events (body, notifyConsole, user_id?)
Backend->>Backend: Parse body, extract requestedOrgId (if notifyConsole)
alt requestedOrgId provided
Backend->>Auth: Verify access (hasOrgRightApikey or hasOrgRight) for requestedOrgId
alt Access granted
Auth-->>Backend: OK
Backend->>Backend: Use requestedOrgId for notifyConsole flow
Backend-->>Client: 200 OK (event processed)
else Access denied
Auth-->>Backend: Forbidden
Backend-->>Client: 403 Forbidden
end
else No requestedOrgId
Backend->>Backend: Use authenticated userId as orgId
Backend-->>Client: 200 OK (event processed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: f60fcb52d0
ℹ️ 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".
| const requestedOrgId = typeof body.user_id === 'string' && body.user_id.length > 0 ? body.user_id : undefined | ||
|
|
||
| if (requestedOrgId && !(await canAccessRequestedOrg(c, requestedOrgId))) | ||
| return c.json({ error: 'Forbidden' }, 403) |
There was a problem hiding this comment.
The new guard treats every non-empty body.user_id as an organization ID and rejects the request unless hasOrgRight* passes, but existing callers still use user_id as a user identifier (for example sendEvent in src/modules/auth.ts sends main.auth?.id on login). With this change, those requests now return 403 because a user UUID is not an org UUID, so login/event analytics are silently dropped for authenticated users. Restrict this org-access check to org-scoped flows (e.g. notifyConsole/org-targeted events) or otherwise distinguish org IDs from user IDs before enforcing org authorization.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/events.test.ts (1)
82-107:⚠️ Potential issue | 🟠 MajorAdd the missing JWT authorized-org success test.
Line 90 adds only the JWT forbidden case, while Line 84 still skips the JWT happy path. This leaves a gap where a regression that rejects all JWT org-targeted requests would still pass this suite.
Suggested test addition
+ it('allows jwt broadcasts for an authorized org', async () => { + const authHeaders = await getAuthHeaders() + const response = await fetch(`${BASE_URL}/private/events`, { + method: 'POST', + headers: authHeaders, + body: JSON.stringify({ + channel: 'test', + event: 'test_event', + description: 'Testing event tracking', + notifyConsole: true, + user_id: ORG_ID, + }), + }) + + const data = await response.json() as { status: string } + expect(response.status).toBe(200) + expect(data.status).toBe('ok') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events.test.ts` around lines 82 - 107, Add a new test that verifies JWT-authenticated requests succeed when targeting the token's own org: use getAuthHeaders() to obtain authHeaders, POST to `${BASE_URL}/private/events` with a body similar to the forbidden test but set user_id to the token's org (or omit user_id so the server uses the JWT's org), then assert response.status is 200 and the response body indicates success (e.g., contains the created event id or no error). Place this alongside the existing jwt tests (referencing getAuthHeaders, BASE_URL, and the /private/events route) so the suite covers both the FOREIGN_ORG_ID forbidden case and a positive authorized-org case.
🧹 Nitpick comments (1)
tests/events.test.ts (1)
42-107: Useit.concurrent()for the newly added tests.Line 42, Line 62, and Line 90 introduce new
it()tests instead ofit.concurrent(), which is inconsistent with the test concurrency rule for this repo.Suggested refactor
- it('rejects apikey attempts to broadcast events to a foreign org', async () => { + it.concurrent('rejects apikey attempts to broadcast events to a foreign org', async () => { - it('allows apikey broadcasts for an authorized org', async () => { + it.concurrent('allows apikey broadcasts for an authorized org', async () => { - it('rejects jwt attempts to broadcast events to a foreign org', async () => { + it.concurrent('rejects jwt attempts to broadcast events to a foreign org', async () => {As per coding guidelines: "ALL TEST FILES RUN IN PARALLEL; use
it.concurrent()instead ofit()to maximize parallelism; create dedicated seed data when tests modify shared resources."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events.test.ts` around lines 42 - 107, Three tests use plain it() instead of the repo-required it.concurrent(): replace the three it(...) blocks whose descriptions are "rejects apikey attempts to broadcast events to a foreign org", "allows apikey broadcasts for an authorized org", and "rejects jwt attempts to broadcast events to a foreign org" with it.concurrent(...) so they run in parallel; while doing so ensure any shared-resource modification in those tests is backed by dedicated seed/setup/teardown (create isolated test data or mocks) to avoid cross-test interference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/events.test.ts`:
- Around line 82-107: Add a new test that verifies JWT-authenticated requests
succeed when targeting the token's own org: use getAuthHeaders() to obtain
authHeaders, POST to `${BASE_URL}/private/events` with a body similar to the
forbidden test but set user_id to the token's org (or omit user_id so the server
uses the JWT's org), then assert response.status is 200 and the response body
indicates success (e.g., contains the created event id or no error). Place this
alongside the existing jwt tests (referencing getAuthHeaders, BASE_URL, and the
/private/events route) so the suite covers both the FOREIGN_ORG_ID forbidden
case and a positive authorized-org case.
---
Nitpick comments:
In `@tests/events.test.ts`:
- Around line 42-107: Three tests use plain it() instead of the repo-required
it.concurrent(): replace the three it(...) blocks whose descriptions are
"rejects apikey attempts to broadcast events to a foreign org", "allows apikey
broadcasts for an authorized org", and "rejects jwt attempts to broadcast events
to a foreign org" with it.concurrent(...) so they run in parallel; while doing
so ensure any shared-resource modification in those tests is backed by dedicated
seed/setup/teardown (create isolated test data or mocks) to avoid cross-test
interference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2635e65-83b4-4f80-a3cf-ae60fd793fca
📒 Files selected for processing (2)
supabase/functions/_backend/private/events.tstests/events.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/events.test.ts (1)
42-80: Tests correctly validate cross-org authorization boundaries for API keys.Both rejection (foreign org → 403) and allowance (authorized org → 200) paths are covered with appropriate assertions.
Consider using
it.concurrent()for these tests to align with the parallelism guideline, since they don't modify shared database state.As per coding guidelines: "ALL TEST FILES RUN IN PARALLEL; use
it.concurrent()instead ofit()to maximize parallelism."♻️ Optional refactor to use concurrent tests
- it('rejects apikey attempts to broadcast events to a foreign org', async () => { + it.concurrent('rejects apikey attempts to broadcast events to a foreign org', async () => {- it('allows apikey broadcasts for an authorized org', async () => { + it.concurrent('allows apikey broadcasts for an authorized org', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events.test.ts` around lines 42 - 80, Convert the two synchronous tests "rejects apikey attempts to broadcast events to a foreign org" and "allows apikey broadcasts for an authorized org" to run in parallel by replacing the top-level it() calls with it.concurrent(), keeping all headers, request bodies and assertions unchanged; update both test declarations in tests/events.test.ts so they use it.concurrent() to follow the guideline that all tests run in parallel and ensure there are no shared DB mutations in these specific cases before making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/events.test.ts`:
- Around line 42-80: Convert the two synchronous tests "rejects apikey attempts
to broadcast events to a foreign org" and "allows apikey broadcasts for an
authorized org" to run in parallel by replacing the top-level it() calls with
it.concurrent(), keeping all headers, request bodies and assertions unchanged;
update both test declarations in tests/events.test.ts so they use
it.concurrent() to follow the guideline that all tests run in parallel and
ensure there are no shared DB mutations in these specific cases before making
the change.
|
1 participant
Summary (AI generated)
/private/eventsbefore using them for realtime or analyticsnotifyConsoleand tracking requests for orgs the caller cannot readMotivation (AI generated)
The
/private/eventshandler trustedbody.user_idas an org identifier and used it to route realtime broadcasts and analytics side effects. That allowed callers to target organizations they do not belong to.Business Impact (AI generated)
This closes a cross-org integrity issue that could inject fake operational activity into another customer's console and analytics. It reduces support risk and protects tenant isolation.
Test Plan (AI generated)
bunx eslint supabase/functions/_backend/private/events.ts tests/events.test.tsbun run lint:backendbun run typecheckbun run supabase:with-env -- bunx vitest run tests/events.test.tsbun run test:allGenerated with AI
Summary by CodeRabbit