Repository navigation
[Security][P1] Fix cross-tenant drive metadata broadcast (#560) - #565
Conversation
Replace global broadcast with per-user broadcasts to prevent cross-tenant
information leakage. Drive create/update/delete events are now only sent
to users who have access to the drive (owner + members).
Changes:
- Add getDriveRecipientUserIds() helper to get owner + members
- Update broadcastDriveEvent() to require recipientUserIds parameter
- Broadcast to user:${userId}:drives channel instead of global:drives
- Remove join_global_drives/leave_global_drives handlers from realtime
- Update useGlobalDriveSocket to only join user-specific channel
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThe PR transitions drive event broadcasting from a global channel model to a targeted per-user channel model. It introduces recipient-aware broadcasting by adding a new Changes
Sequence Diagram(s)sequenceDiagram
actor User1
actor User2
participant Client as Client/Web App
participant API as Drive API<br/>(apps/web)
participant DB as Database
participant WS as WebSocket<br/>Broadcaster
participant RT as Realtime Server<br/>(Socket.io)
rect rgba(200, 150, 100, 0.5)
Note over User1,RT: Old Flow: Global Channel Broadcast
User1->>Client: Update drive
Client->>API: PATCH /drives/[id]
API->>DB: Update drive
DB-->>API: Success
API->>WS: broadcastDriveEvent(payload)
WS->>RT: Emit to global:drives
RT-->>User1: Event received
RT-->>User2: Event received (unintended)
end
rect rgba(100, 150, 200, 0.5)
Note over User1,RT: New Flow: Per-User Channel Broadcast
User1->>Client: Update drive
Client->>API: PATCH /drives/[id]
API->>DB: Update drive
DB-->>API: Success
API->>DB: getDriveRecipientUserIds(driveId)
DB-->>API: [User1_ID, User2_ID]
API->>WS: broadcastDriveEvent(payload, [User1_ID, User2_ID])
WS->>RT: Emit to user:User1_ID:drives
WS->>RT: Emit to user:User2_ID:drives
RT-->>User1: Event received
RT-->>User2: Event received (authorized)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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)
apps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts (1)
504-524: 🛠️ Refactor suggestion | 🟠 MajorTests don't assert that
broadcastDriveEventreceives the recipient user IDs.The core security invariant of this PR is that events are delivered only to authorized recipients. The boundary obligation tests verify
broadcastDriveEventwas called but don't assert the second argument (recipientUserIds). Consider strengthening the assertion:- expect(broadcastDriveEvent).toHaveBeenCalled(); + expect(broadcastDriveEvent).toHaveBeenCalledWith( + expect.objectContaining({ driveId: mockDriveId, event: 'updated' }), + ['user-123', 'user-456'] + );The same applies to the DELETE boundary test at Line 732.
🧹 Nitpick comments (3)
apps/web/src/hooks/useGlobalDriveSocket.ts (2)
17-18: Stale comment: still references "global drives channel".This comment should be updated to reflect the new user-specific channel semantics.
- // Track if we've joined the global drives channel + // Track if we've joined the user-specific drives channel
76-76: Stale comment: references "both global and user-specific".There is no global channel anymore. Update to avoid confusion.
- // Listen for drive events (both global and user-specific) + // Listen for drive events on user-specific channelapps/web/src/lib/websocket/socket-utils.ts (1)
200-214:Promise.allwill short-circuit on the first rejected fetch, skipping remaining recipients.If one broadcast request fails (e.g., network blip for one user's channel), the remaining users won't receive the event. Consider
Promise.allSettledto ensure best-effort delivery to all recipients, then log any individual failures.♻️ Suggested improvement
- await Promise.all( - recipientUserIds.map(async (userId) => { + const results = await Promise.allSettled( + recipientUserIds.map((userId) => { const requestBody = JSON.stringify({ channelId: `user:${userId}:drives`, event: `drive:${payload.operation}`, payload, }); - await fetch(`${realtimeUrl}/api/broadcast`, { + return fetch(`${realtimeUrl}/api/broadcast`, { method: 'POST', headers: createSignedBroadcastHeaders(requestBody), body: requestBody, }); }) ); + + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + realtimeLogger.warn('Some drive event broadcasts failed', { + operation: payload.operation, + failedCount: failures.length, + totalCount: recipientUserIds.length, + }); + }
- Update stale comments in useGlobalDriveSocket.ts referencing "global drives" - Strengthen test assertions in route.test.ts to verify recipient userIds - Replace Promise.all with Promise.allSettled for resilient broadcasting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addressed CodeRabbit Review FeedbackThank you @coderabbitai for the thorough review! All suggestions have been addressed in commit 21389f3: ✅ Major: Test Assertions for Recipient UserIdsUpdated both PATCH and DELETE boundary tests in expect(broadcastDriveEvent).toHaveBeenCalledWith(
expect.objectContaining({ driveId: mockDriveId, event: 'updated' }),
['user-123', 'user-456']
);✅ Nitpick: Stale Comments in
|
|
Tip For best results, initiate chat on the files or code changes.
Excellent work addressing all the feedback:
The security fix looks solid, and your thorough testing gives good confidence in the implementation. Great job on this P1 security issue! 🚀 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
broadcastDriveEvent()sent toglobal:drivesroom with no authorization checkuser:${userId}:driveschannelsChanges
getDriveRecipientUserIds()helper that returns owner + all drive membersbroadcastDriveEvent()signature to requirerecipientUserIdsparameteruser:${userId}:driveschannel instead ofglobal:drivesjoin_global_drivesandleave_global_driveshandlers from realtime serviceuseGlobalDriveSockethook to only join user-specific channelFiles Modified
packages/lib/src/services/drive-member-service.tsapps/web/src/lib/websocket/socket-utils.tsapps/web/src/app/api/drives/route.ts,apps/web/src/app/api/drives/[driveId]/route.ts, + 5 moreapps/realtime/src/index.tsapps/web/src/hooks/useGlobalDriveSocket.tsapps/web/src/lib/ai/tools/drive-tools.ts,apps/web/src/lib/ai/tools/page-write-tools.tsTest plan
Scalability Note
Current approach: O(members) broadcasts per drive event. Acceptable for typical drive sizes (< 100 members).
Future org-layer optimization: Single broadcast to
org:${orgId}:drivesroom with O(1) broadcast.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes