fix: enforce room membership on the client-to-client relay - #26
Open
sidmorizon wants to merge 5 commits into
Open
fix: enforce room membership on the client-to-client relay#26sidmorizon wants to merge 5 commits into
sidmorizon wants to merge 5 commits into
Conversation
`socket.to(roomId).emit()` is a delivery operator: it resolves the membership of the *recipients* and never checks the sender's. The two c2c relay paths passed a client-supplied roomId straight into it, so any connected socket that knew a roomId could inject client-to-client calls into a room it had never joined - without occupying a room slot, without triggering the `room-full` alert, and invisibly to both real devices. That bypasses the invariant the pairing flow relies on: influencing a pairing session should require taking one of the room's two slots, which is precisely what makes it observable to the user (the real device can no longer join, so they re-pair with a fresh room). Reproduced against the built server: with A and B paired (2/2, so join is refused with CONNECTION_REJECTED and the member list holds only A and B), a third socket that never joined still delivered `cancelTransfer` to both. Membership is authoritative in RoomManager and already enforced by leaveRoom, getRoomUsers and startTransfer; the relay path simply never consulted it, because JsBridgeBase is a transport abstraction with no notion of rooms and the bridge was not given the RoomManager. Hand it the RoomManager and check the sender in checkC2cEnvelope, which both the request and the response path share. Impact was availability only - never confidentiality. Payloads are encrypted with a key derived on the clients from the pairing code and an ECDHE exchange, so an injected payload cannot be decrypted by a peer and real traffic cannot be read by an injector. Also documents that the server-generated room key is not part of the end-to-end encryption scheme: the client never consumes it and this server never encrypts with it. Reviewers have read it as key escrow, so the intent is now stated at each site it surfaces.
getRoomUsers answered differently depending on whether a room existed: a missing room returned [], while a room the caller was not in threw. Any connected socket could tell the two apart, which turns the method into an unauthenticated room existence oracle. It is also on the rate limit whitelist, so it could be probed at line speed - measured at ~1250 probes/s on a single connection, roughly 3700x cheaper than probing via joinRoom, which is rate limited and costs a room slot. isUserInRoom() already reports false for a missing room, so both cases now fail through the same branch with an identical code and message. Room existence is no longer observable to a non-member. The error is ROOM_NOT_FOUND rather than SOCKET_NOT_IN_ROOM because clients map error codes, not messages: shipped clients already translate ROOM_NOT_FOUND into a localized "invalid pairing code" prompt, whereas SOCKET_NOT_IN_ROOM falls through to a raw English string in a toast. Compatibility with shipped clients was checked call site by call site. A caller that is in the room still gets the same response as before, and a member's room always exists (a room is only deleted when its last member leaves), so the changed branches are reachable only once the caller is no longer a member - in practice only after the 1 hour room timeout. There, app UI previously rendered a silently empty peer list and now surfaces a localized error, and the CLI's 1s room-users poll already ignores failures (transfer-receiver-adapter.ts), so its pairing loop is unaffected. Note this does not make room ids unobservable in general: joinRoom still distinguishes ROOM_NOT_FOUND from CONNECTION_REJECTED. That path stays rate limited, consumes a room slot and raises `room-full` on the paired devices, so probing through it remains slow and visible by design.
getRoomUsers was on the rate limit whitelist, so it could be called at line speed - measured at ~1250 calls/s on one connection. It was exempt because the CLI polls it once per second while pairing, and the default 3s window would reject two of every three polls. Give it its own 800ms window instead. The CLI's 1s poll is unaffected and the ceiling drops to ~1.25 calls/s per connection, three orders of magnitude lower. The window has to sit meaningfully below the caller's polling interval. setInterval fixes the interval at which requests are *sent*; network latency shifts each request by roughly the same amount, so it cancels out of the gap the server observes, and only jitter moves that gap - in both directions. A 1000ms window against a 1000ms poll therefore sits exactly on the threshold rather than safely above it: measured over localhost, where latency is under a millisecond and stable, timer drift alone still pushed one poll in 30 below 1000ms and into a rejection. At 800ms the same run passes 30 of 30. pruneRateLimitState() now expires each entry against its own window rather than the default. It only ever drops entries whose window has passed, so a per-method window longer than the default would otherwise be reclaimed while still live - which is precisely the rate limit reset that function exists to prevent.
The server emitted room-full, user-left and start-transfer, but never user-joined - despite it being declared in IServerToClientEvents since the beginning. A peer that needed to know someone had entered the room had no push to wait on, so the CLI polls getRoomUsers once per second throughout pairing. That poll is the sole reason getRoomUsers needs a rate limit window as loose as 800ms. The app never needed the event: its pairing is passive, and the joining side immediately calls verifyPairingCode over the c2c channel, so the arrival of that call already means "the peer is here". Only a peer that wants to react before pairing verification completes - the CLI, which shows a "device connected, verifying" step - has to ask. Emitted from the joining socket rather than the server, so the joiner is excluded, and only after join() resolves, so a receiver that immediately calls getRoomUsers sees the membership the event describes. The already-in-room early return does not emit: no new member appeared. Shipped clients listen for none of this; Socket.IO drops events with no handler, so this is inert until the CLI is changed to consume it. The 800ms window on getRoomUsers therefore has to stay until the CLI's poll is actually gone. Also corrects the user-joined / user-left declarations to include roomId. user-left has always been emitted with it - a client multiplexes several rooms over one socket and needs it to tell which room an event refers to - and the app's handler already reads it. Declaration only; no behavior change.
The table is for legitimate high-frequency callers a shipped client already depends on, not an opt-out from rate limiting. Absent methods fall back to RATE_LIMIT_INTERVAL_MS, and that default is deliberate: a newly added method is limited without anyone having to remember to limit it. Without this stated, the table reads like a quieter whitelist, and the fail-safe it preserves is easy to erode one entry at a time. Adding an entry now requires naming the caller, its frequency, and why the default cannot serve it. Entries are compatibility shims with a lifetime, not permanent exemptions - the getRoomUsers entry records that it goes away with the CLI's poll once that consumes user-joined.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prompted by an external security report claiming the server-returned room
key defeats E2EE. That claim does not hold — the key is never consumed by
any client and this server never encrypts with it — but auditing it turned
up two real gaps and one piece of missing infrastructure.
What changed
1. The c2c relay never checked the sender's membership (
943a287)socket.to(roomId).emit()resolves the membership of the recipients andnever checks the sender's. Both relay paths passed a client-supplied roomId
straight into it, so any connected socket that knew a roomId could inject
client-to-client calls into a room it had never joined — without taking a
room slot, without raising
room-full, and invisibly to both real devices.That bypassed the invariant the pairing flow relies on: influencing a
session should require occupying one of the two slots, which is exactly
what makes it observable (the real device can no longer join, so the user
re-pairs with a fresh room).
Reproduced against the built server: with A and B paired 2/2 — join refused
with
CONNECTION_REJECTED, member list holding only A and B — a thirdsocket that never joined still delivered
cancelTransferto both.Impact was availability only, never confidentiality: payloads are encrypted
with a key the clients derive from the pairing code and an ECDHE exchange
this server is not party to.
2.
getRoomUsersleaked room existence (8d6a778)It returned
[]for a missing room but threw for a room the caller was notin, so any socket could tell the two apart — an unauthenticated room
existence oracle, and one exempt from rate limiting, measured at ~1250
probes/s. Both cases now fail identically through
isUserInRoom().Uses
ROOM_NOT_FOUNDrather thanSOCKET_NOT_IN_ROOMbecause clients maperror codes, not messages: shipped clients already translate the former
into a localized prompt, while the latter surfaces a raw English string.
3.
getRoomUsersis rate limited instead of exempt (cbea719)It was whitelisted because the CLI polls it once per second while pairing.
It now has its own 800ms window: the CLI's poll is unaffected and the
ceiling drops to ~1.25 calls/s per connection.
The window must sit meaningfully below the caller's poll interval —
setIntervalfixes when requests are sent, and latency cancels out ofthe gap the server sees, so only jitter moves it, in both directions. A
1000ms window against a 1000ms poll sits exactly on the threshold: over
localhost, timer drift alone still pushed one poll in 30 into a rejection.
At 800ms the same run passes 30 of 30.
pruneRateLimitState()now expires each entry against its own window, so aper-method window longer than the default cannot be reclaimed while live —
precisely the rate limit reset that function exists to prevent.
4. The server now emits
user-joined(912bdc9)Declared in
IServerToClientEventssince the beginning but never emitted,which is why the CLI polls to notice a peer arriving. Emitted from the
joining socket (so the joiner is excluded) after
join()resolves (so themembership it describes is already in effect).
Inert for now: no shipped client registers a handler, and Socket.IO drops
events with none. The 800ms window has to stay until the CLI's poll is
actually gone.
5. Documentation (
943a287,b0bda66)The room key's semantics are now stated at each site it surfaces — clients
never consume it, this server never encrypts with it, and real protection
comes from a key derived on the clients. Reviewers have read it as key
escrow; the intent should not require an audit to recover.
The per-method rate limit table now states its contract: it is for
legitimate high-frequency callers, not an opt-out. Absent methods fall back
to the default, and that default is the point — a new method is limited
without anyone having to remember to limit it.
Verification
Every commit was built and run against
smoke.ts+crash-logging.ts(all pass), plus targeted checks against the built server:
unaffected
method name the server does not implement), whitelist still exempt,
getRoomUserslimited at 100ms and passing at 900msuser-joinedreaches existing members only, with the joiner excluded, andmembership is already visible to a receiver that immediately calls
getRoomUsersFollow-ups in the client repo
user-joinedinstead of pollinggetRoomUsers(
transfer-receiver-adapter.ts). Once shipped, thegetRoomUsersentryin the rate limit table can be deleted.
cancelTransferon the client c2c API has no guard at all — it does notcompare roomId, nor check whether pairing was verified. It should use the
existing
checkIsVerifiedRoomId(), as shouldchangeTransferDirection.That check can only live on the client: this server does not know, and
should not know, whether pairing succeeded.
🤖 Generated with Claude Code