Repository navigation
Every member's access token reached every other member in server:clients (GRYT-1239) - #194
Merged
Merged
Conversation
Two guests join through the real handshake. Then the test searches everything each of them was sent for the other's access token and grytUserId, and for secret-named fields in server:clients, server:details.clients and members:list. It also puts someone in voice with a camera, a screen share and a DM call, and covers the per-recipient path server:clients takes when a channel is gated. This fails on main. Both events copy the whole connection record, so every member gets every other member's token, grytUserId and permissions. Co-Authored-By: Claude Opus 5 <[email protected]>
server:clients and server:details.clients each copied the whole connection record for every member and sent it to every other member. The record holds accessToken, grytUserId and permissions, so every verified socket got every other member's live access token. That token passes requireAuth as its owner for fifteen minutes, and token:refresh renews it. publicClientRecord names the sixteen fields another member may see, and publicClientList keys them by socket id and blanks a voice channel the recipient can't see. Both broadcasts build from it, so neither can carry a field that isn't on the list. The fields are the ones the web and mobile clients read: the voice, camera, screen share, mute and deafen state, plus serverUserId and nickname. A client still learns its own permissions and role from server:details.server_info, and its own token from server:joined and token:refreshed. None of that moved. members:list was already built from a separate allowlist and carries no token; the new property scan covers it too and it stays clean. voice:latency:update, voice:call:members and the plugin message bus send no member records. Consumers checked before trimming: web and desktop client, mobile, the bot and SDK, the addon and server plugin APIs, and the socket docs. Nothing read accessToken, grytUserId, permissions, latencyStats, color, status, lastSeen or activity off these records. Co-Authored-By: Claude Opus 5 <[email protected]>
sivert-io
force-pushed
the
claude/GRYT-1239-client-list-fields
branch
from
September 15, 2026 20:55
0dda3a1 to
eb1229d
Compare
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
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.
Review-required path
This changes
packages/server/src/socket/**, the media plane's socket layer. It touches how member state is broadcast, so it needs a read of the whole diff before it merges.The leak
emitClientsNowinsrc/socket/utils/clients.tsandsendServerDetailsinsrc/socket/utils/server.tseach spread the whole live connection record into their payload (server:clients, and theclientsfield ofserver:details). That record carriesaccessToken,grytUserIdandpermissionsalongside the presence fields, so every verified socket received every other connected member's live access token, the owner's included when they are online. The token authenticates as its owner for fifteen minutes, andtoken:refreshrenews it, so a captured one keeps working well past that window.Confirmed on a throwaway server built from this branch: before the fix, two guests each received the other's
server:joinedtoken inside theirserver:clientsandserver:details.clients. After the fix, neither appears.The PR body keeps the mechanism at this level on purpose, since the repository is public.
The fix
publicClientRecordnames the sixteen fields another member is allowed to see, andpublicClientListbuilds the socket-id-keyed map from it, dropping unregisteredtemp_connections and blanking a voice channel the recipient cannot see (the existing scoped-channel masking, unchanged). Both broadcasts now build frompublicClientList, so neither can carry a field that is not on the list.The sixteen fields are the union of what the web/desktop and mobile clients actually read:
serverUserId,nickname,isMuted,isDeafened,isServerMuted,isServerDeafened,isAFK,streamID,hasJoinedChannel,voiceChannelId,isConnectedToVoice,cameraEnabled,cameraStreamID,screenShareEnabled,screenShareVideoStreamID,screenShareAudioStreamID.A client still learns its own permissions and role from
server:details.server_info, and its own access token fromserver:joined/token:refreshed. Neither path was touched, and a test asserts each member still receives its own state, token and standing.Consumers checked before trimming
registerServerSocketEvents.ts,serverView.tsx, voice components): reads the sixteen fields above off the map. Reads its ownserverUserIdand the two server-mute flags off its own entry only. Does not readaccessToken,grytUserId,permissions,latencyStats,color,status,lastSeenoractivityoff these records; those come frommembers:list,server_info,voice:latency:updateor the stored token.src/voice/shares.ts,VoiceSheet.tsx): reads a subset:serverUserId,nickname,voiceChannelId,screenShareEnabled,screenShareVideoStreamID,cameraEnabled,cameraStreamID. All kept.server:clientslistener and no SDK type for these records.server:details.clients: no client reads it at all today; it is kept and trimmed for parity.server-api.mdxlistsserver:clientsandserver:detailsby name only, with no field list, so no docs change is needed.Other emits audited
members:listis already built from its own allowlist (buildMemberList) and carries no token; its identity field is a keyed HMAC, notgrytUserId. The new property scan covers it and it is clean.voice:latency:updatesends the sender's own latency to their call peers,voice:call:memberssends server-user-ids only, and the plugin message bus sends no member records. None carry a token.The admin list events (
server:bans,server:roles,server:members:invites,server:audit,server:joinRequests) return some identity fields by design and are gated behindmanage_*/view_*permissions, so they are out of scope here.Tests
clientRecordLeaks.test.ts: two guests join through the real handshake; a recursive property scan asserts that no emitted payload carriesaccessToken,grytUserId,permissions,latencyStatsor an IP, and that neither guest's token orgrytUserIdreaches the other. Covers a member in voice with a camera, a screen share and a DM call, and the per-recipient path a gated channel takes. Fails onorigin/main, passes here.publicClientList.test.ts: unit-tests the allowlist (exact field set, no spread, no secret, channel masking) and a source check that assertsserver:clientsis emitted from one file and both broadcasts build frompublicClientList. The source check reads the builder, not just the call site.Both were run against
origin/main'sclients.ts/server.tsto confirm they fail without the fix. Mutating any emit back to the raw record, or adding a secret to the allowlist, turns them red.CI-equivalent locally:
yarn test(1334),yarn test:examples(9),npx tsc --noEmit,yarn build,npx eslint .(0 errors; 5 pre-existing warnings in untouched files),node scripts/check-comment-length.mjs.The client e2e suite (
yarn e2e, 18 tests) passed against a server image built from this branch (GRYT_E2E_SERVER_IMAGE).Sibling fix and release
server#192 (GRYT-1238), the sibling security fix on this socket layer, has since merged to
main. This branch is rebased on top of it (and #193), so both fixes are in the same line and should go out in the same release. The rebased diff is the four files below and stays clear of #192's recipient-selection changes.Token invalidation (recommended, not done here)
A token that leaked cannot be recalled, and within the bug's lifetime an attacker could roll a captured access token forward indefinitely by refreshing inside each fifteen-minute window. So an upgrade that only stops new leaks still leaves captured tokens live.
I recommend bumping
server_config.token_versiononce after deploying this. Refresh tokens were not exposed by this bug (they only ever go to the owning socket viaserver:joined/token:refreshed), so a legitimate client holding one re-mints a fresh access token at the new version automatically, while an attacker holding only a captured access token is turned away atrequireAuthand attoken:refresh. The cost is a one-time revoke-then-refresh cycle for everyone connected at upgrade.I have not implemented a mass logout. Tracked as GRYT-1259: bump
token_versionby hand when deploying this, or add a one-shot that fires exactly once on the upgrade. If you want the automatic version, say so and it goes in its own PR, with care that it fires once rather than on every boot.🤖 Generated with Claude Code