security: bound room creation and reclaim empty rooms (DoS)#34
Conversation
`ChatServer` created a room entry for every new `ROOM_JOIN` name and never removed one: `handleRoomLeave` dropped only the caller and `removeSession` dropped the user from every member set without deleting empty room keys. The map therefore grew monotonically for the process lifetime, and with no rate limiting a single client could spam unique room names to exhaust memory and force an N-way `ROOM_ADDED` broadcast per join. In the default no-`--accounts` mode this was unauthenticated. - Reclaim a non-default room as soon as its last member leaves or disconnects, and announce it with the new `ROOM_REMOVED` frame plus a `ChatMessage.roomRemoved` factory. `general` is never reclaimed. - Handle `ROOM_REMOVED` in the console client and the Swing controller/model so room lists do not go stale. - Replace the hardcoded 1,000-room constant with `ChatServerConfig.maxRooms` (default 256, CLI `--max-rooms`); an over-cap `ROOM_JOIN` for a new room is answered with `ERROR` instead of creating it. - Add per-connection token buckets (`RateLimitConfig`, `TokenBucket`) for inbound frames and for room creation; exhausted buckets reply `ERROR` rather than dropping the connection. Membership, the cap check, and reclamation run under a dedicated rooms monitor so the cap is exact and a room cannot be reclaimed between a joiner observing it and joining it. That monitor performs no I/O and is never nested inside the sessions monitor: `removeSession` now announces `USER_REMOVED`/`ROOM_REMOVED` after releasing the sessions monitor, so the two locks have no cycle. Tests cover reclamation on last leave and on disconnect, the broadcast, the general-room exemption, the room cap, and both token buckets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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: b5a35dd41a
ℹ️ 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".
| ROOM_JOIN, | ||
| ROOM_LEAVE, | ||
| ROOM_ADDED, | ||
| ROOM_REMOVED, |
There was a problem hiding this comment.
When a mixed-version deployment has any older v1 client connected, reclaiming a room now broadcasts ROOM_REMOVED with the still-current protocol version 1. Those clients do not have this enum value, so ChatProtocol.decode fails before their version check can produce an explicit unsupported-version error, and the client drops the connection whenever another user empties a room. Please either increment the protocol version for this new frame or avoid sending it to protocol-v1 clients.
Useful? React with 👍 / 👎.
| synchronized (roomsMonitor) { | ||
| roomNames = List.copyOf(roomMembers.keySet()); | ||
| } | ||
| for (String roomName : roomNames) { | ||
| connection.send(ChatMessage.roomAdded(roomName)); |
There was a problem hiding this comment.
When a client is connecting while the last member leaves a room, the new connection is already in sessions, so announceRoomRemoved can send ROOM_REMOVED to it after this snapshot is taken but before the loop sends the stale ROOM_ADDED. The client then applies remove-before-add and keeps a room that no longer exists until it tries to use it. Please order the initial room list with removal broadcasts, or revalidate/suppress stale room additions before sending them.
Useful? React with 👍 / 👎.
1 participant
Fixes the remaining denial-of-service issue found in the security audit. The unbounded pre-handshake frame read was already fixed upstream and is untouched here.
The bug. Room entries were never reclaimed:
handleRoomLeaveremoved only the caller andremoveSessionremoved the user from every member set but never deleted the room key, soroomMembersgrew monotonically for the process lifetime. There was no rate limiting, so one client could spam uniqueROOM_JOINnames to leak memory and force an N-wayROOM_ADDEDbroadcast per join. In the default no---accountsmode this is unauthenticated.ROOM_REMOVEDframe.generalis never reclaimed.maxRooms, default 256, CLI--max-rooms. The previous hardcoded 1000 was bypassable viajoinRoom'scomputeIfAbsent.ERRORrather than disconnecting.roomsMonitormakes the cap exact; no I/O under the monitor and the two locks are acyclic../gradlew clean check→ BUILD SUCCESSFUL, 105 tests, 0 failures; checkstyle, spotless, spotbugs and coverage all green.PROTOCOL_VERSIONstays 1, but an old client build receivingROOM_REMOVEDwill drop the connection. In a mixed-version deployment, upgrade clients before servers.🤖 Generated with Claude Code