fix: Improve security of socket.io transport layer#894
Merged
Conversation
This implements several tests for the data leaking scenarios discussed in #889
This addresses inauthentic clients receiving a sync triggered by a later authentic client’s sync (as described in #889).
Both `send` and `sendAll` use loops to iterate over the registered clients and emit events. Using `sendAll` would emit an exponential number of events if multiple clients were connected with the same `playerID`.
A provisional client is a client that hasn’t yet been authenticated by the game master. Adding it to `clientInfo` could be risky becase it could receive payloads intended for a specific playerID before it has been authenticated for that playerID. With this approach, the provisional client is sandboxed until it is authenticated.
This prevents a race condition whereby if an inauthentic client synced just before an authentic client, but its authentication process took longer than the authentic client’s sync, it could receive the authentic client’s sync payload.
Memory overhead for a bunch of empty Sets is probably marginal, but it seems like these should be removed nonetheless.
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.
Closes #889
SocketIO#clientInfountil after they have been authenticated.There is still no expiration of client connections, so hypothetically the transport is still vulnerable to a client that keeps open its connection after leaving a credentialed match that someone else then joins (see #889 for discussion). The only way to avoid that currently would be to re-authenticate each client when sending anything to them, which would add significant overhead to what is supposed to be a lightweight transport layer. I think ultimately there should be a way for the lobby API to ping the transport and say that a specific playerID’s credentials have expired at which point clients with that playerID could be re-authenticated or removed.