From d6cc57af5647632be736466c178c6ff9271594f1 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:18:08 +0200 Subject: [PATCH] fix(ws): require DM participation to join a DM voice room (F11) The voice gate authorized a client-supplied channel_id with role bits only, and DM channels carry no channel_overrides rows, so any member's base CONNECT_VOICE bit minted a LiveKit RoomJoin and CanSubscribe token for any DM. Both voice entry points now go through a gate that re-runs the old role predicate and additionally requires DM participation, delegating that rule to the existing permissions.Checker.RequireChannelAccess rather than adding a second implementation of it. Verified by a panel of agents; a negative control of the base tree plus only the new test file fails both non-participant tests with a LiveKit room token issued for a DM the user is not a participant of, while both participant tests pass on base and patched alike. Co-Authored-By: Claude Opus 5 (1M context) --- Server/ws/deps.go | 53 +++++++++++ Server/ws/handlers.go | 15 ++- Server/ws/voice_dm_access_test.go | 147 ++++++++++++++++++++++++++++++ Server/ws/voice_join.go | 10 +- 4 files changed, 218 insertions(+), 7 deletions(-) create mode 100644 Server/ws/voice_dm_access_test.go diff --git a/Server/ws/deps.go b/Server/ws/deps.go index f99a17e9..fd6e2886 100644 --- a/Server/ws/deps.go +++ b/Server/ws/deps.go @@ -129,6 +129,59 @@ func hasPerm(ctx context.Context, database *db.DB, perms *permissions.Checker, u return perms.HasChannelPerm(ctx, role.Permissions, role.ID, channelID, perm) } +// hasChannelAccess is the gate to use when the channel id comes from the client: +// it is hasPerm plus the channel-type branch that role bits cannot express. +// +// A DM channel carries no channel_overrides rows, so a default Member's base +// bits satisfy hasPerm for ANY dm channel id — including a conversation the +// caller is not part of. permissions.Checker.RequireChannelAccess is the shared +// definition of channel access (service.PermissionService.RequireChannelAccess +// mirrors it for the REST/service paths) and supplies the IsDMParticipant +// branch, so the DM membership rule keeps exactly one implementation. Group DMs +// need no special case: dm_participants holds one row per participant and +// IsDMParticipant is a lookup on (user_id, channel_id). +// +// The role bit is still required on top, which RequireChannelAccess waives for +// DMs. Voice has always demanded CONNECT_VOICE and sweepStaleVoiceStates keeps +// re-checking it per role for every live participant, so keeping it here means +// this check can only ever narrow access — never hand someone a grant the old +// role-only check refused, and never let the sweeper evict a client the join +// gate admitted. +// +// Blocking is deliberately not consulted here: it is the message paths' rule +// (service.requireDMNotBlocked), it is two-party only, and a blocked user is +// still a participant, so it is orthogonal to the non-participant hole this +// closes. +func hasChannelAccess(ctx context.Context, database *db.DB, perms *permissions.Checker, userID, channelID, perm int64) bool { + if database == nil || perms == nil { + return false + } + role, err := database.GetRoleForUser(ctx, userID) + if err != nil || role == nil { + return false + } + if !perms.HasChannelPerm(ctx, role.Permissions, role.ID, channelID, perm) { + return false + } + ch, err := database.GetChannel(ctx, channelID) + if err != nil { + // Fail closed: an unknown type would silently take the non-DM path. + slog.Error("ws: hasChannelAccess GetChannel failed, denying", + "user_id", userID, "channel_id", channelID, "err", err) + return false + } + channelType := "" + if ch != nil { + channelType = ch.Type + } + // A missing channel row leaves channelType empty, i.e. the role verdict + // above stands: there is no DM there to join, and callers keep reporting a + // deleted channel the way they always have. For every non-DM type this call + // just re-runs the role check above; the repeated lookup is the price of one + // shared definition of the rule, on a per-user rate-limited path. + return perms.RequireChannelAccess(ctx, userID, role.Permissions, role.ID, channelType, channelID, perm) == nil +} + // ── V2 handler type ───────────────────────────────────────────────────────── // HandlerV2 is the function signature for new-style (pure-ish) handlers. diff --git a/Server/ws/handlers.go b/Server/ws/handlers.go index 8150e7e9..6b8a30bf 100644 --- a/Server/ws/handlers.go +++ b/Server/ws/handlers.go @@ -218,11 +218,16 @@ func (h *Hub) hasChannelPerm(ctx context.Context, c *Client, channelID int64, pe return h.permChecker.HasChannelPerm(ctx, role.Permissions, role.ID, channelID, perm) } -// requireChannelPerm checks whether the client has the given permission on the -// channel. If not, it sends a FORBIDDEN error to the client and returns false. -// The permLabel should be the human-readable permission name (e.g. "SEND_MESSAGES"). -func (h *Hub) requireChannelPerm(ctx context.Context, c *Client, channelID int64, perm int64, permLabel string) bool { - if h.hasChannelPerm(ctx, c, channelID, perm) { +// requireChannelAccess checks whether the client may act on the channel with the +// given permission. If not, it sends a FORBIDDEN error to the client and returns +// false. The permLabel should be the human-readable permission name (e.g. +// "SEND_MESSAGES"). +// +// Unlike hasChannelPerm it is channel-type aware (see hasChannelAccess), which +// is what a channel id taken straight from a client frame requires: role bits +// alone let any member through to a DM they are not a participant of. +func (h *Hub) requireChannelAccess(ctx context.Context, c *Client, channelID int64, perm int64, permLabel string) bool { + if hasChannelAccess(ctx, h.db, h.permChecker, c.userID, channelID, perm) { return true } slog.Warn("ws permission denied", "user_id", c.userID, "channel_id", channelID, "perm", permLabel) diff --git a/Server/ws/voice_dm_access_test.go b/Server/ws/voice_dm_access_test.go new file mode 100644 index 00000000..3ad44db0 --- /dev/null +++ b/Server/ws/voice_dm_access_test.go @@ -0,0 +1,147 @@ +package ws_test + +import ( + "context" + "testing" + "time" + + "github.com/owncord/server/ws" +) + +// F11: voice_join and voice_token_refresh authorized the client-supplied channel +// id with a role-only permission check. DM channels carry no channel_overrides, +// so a default Member's base CONNECT_VOICE bit satisfied that check for ANY dm +// channel id and the server minted a LiveKit RoomJoin+CanSubscribe token for a +// conversation the caller is not part of (and then fed them the other +// participants' voice_e2ee_announce keys). Both entry points must consult DM +// membership; both must still work for a genuine participant. + +// assertNoVoiceToken asserts that no LiveKit token reached the client and that a +// FORBIDDEN error did. +func assertNoVoiceToken(t *testing.T, msgs [][]byte) { + t.Helper() + for _, m := range msgs { + if extractType(t, m) == "voice_token" { + t.Fatal("a LiveKit room token was issued for a DM the user is not a participant of") + } + } + found := false + for _, m := range msgs { + if extractCode(t, m) == "FORBIDDEN" { + found = true + break + } + } + if !found { + t.Error("expected a FORBIDDEN error for the non-participant") + } +} + +func hasVoiceToken(t *testing.T, msgs [][]byte) bool { + t.Helper() + for _, m := range msgs { + if extractType(t, m) == "voice_token" { + return true + } + } + return false +} + +func TestVoiceJoin_DMNonParticipant_GetsNoTokenAndNoVoiceState(t *testing.T) { + hub, database := newVoiceHub(t) + alice := seedMemberUser(t, database, "dmvoice-alice") + bob := seedMemberUser(t, database, "dmvoice-bob") + mallory := seedMemberUser(t, database, "dmvoice-mallory") + dmID := seedDMChannel(t, database, alice.ID, bob.ID) + + send := make(chan []byte, 32) + c := ws.NewTestClientWithUser(hub, mallory, 0, send) + hub.Register(c) + time.Sleep(20 * time.Millisecond) + + hub.HandleMessageForTest(c, voiceJoinMsg(dmID)) + time.Sleep(50 * time.Millisecond) + + assertNoVoiceToken(t, drainChanTimeout(send, 200*time.Millisecond)) + + state, err := database.GetVoiceState(context.Background(), mallory.ID) + if err != nil { + t.Fatalf("GetVoiceState: %v", err) + } + if state != nil { + t.Fatalf("non-participant was persisted into the DM's voice channel (%d)", state.ChannelID) + } +} + +func TestVoiceJoin_DMParticipant_StillJoins(t *testing.T) { + hub, database := newVoiceHub(t) + alice := seedMemberUser(t, database, "dmvoice-ok-alice") + bob := seedMemberUser(t, database, "dmvoice-ok-bob") + dmID := seedDMChannel(t, database, alice.ID, bob.ID) + + send := make(chan []byte, 32) + c := ws.NewTestClientWithUser(hub, alice, 0, send) + hub.Register(c) + time.Sleep(20 * time.Millisecond) + + hub.HandleMessageForTest(c, voiceJoinMsg(dmID)) + time.Sleep(50 * time.Millisecond) + + if !hasVoiceToken(t, drainChanTimeout(send, 200*time.Millisecond)) { + t.Error("a DM participant must still receive a voice token for their own DM") + } + + state, err := database.GetVoiceState(context.Background(), alice.ID) + if err != nil { + t.Fatalf("GetVoiceState: %v", err) + } + if state == nil || state.ChannelID != dmID { + t.Fatalf("participant voice state = %+v, want channel %d", state, dmID) + } +} + +func TestVoiceTokenRefresh_DMNonParticipant_Refused(t *testing.T) { + hub, database := newVoiceHub(t) + alice := seedMemberUser(t, database, "dmrefresh-alice") + bob := seedMemberUser(t, database, "dmrefresh-bob") + mallory := seedMemberUser(t, database, "dmrefresh-mallory") + dmID := seedDMChannel(t, database, alice.ID, bob.ID) + + send := make(chan []byte, 32) + c := ws.NewTestClientWithUser(hub, mallory, 0, send) + hub.Register(c) + time.Sleep(20 * time.Millisecond) + + // Second entry point: the refresh mints a token from the session's own voice + // channel id, so it must re-run the same membership check rather than trust + // that a join once passed. + ws.SetVoiceChIDForTest(c, dmID) + + hub.HandleMessageForTest(c, voiceTokenRefreshMsg()) + time.Sleep(50 * time.Millisecond) + + assertNoVoiceToken(t, drainChanTimeout(send, 200*time.Millisecond)) +} + +func TestVoiceTokenRefresh_DMParticipant_StillRefreshes(t *testing.T) { + hub, database := newVoiceHub(t) + alice := seedMemberUser(t, database, "dmrefresh-ok-alice") + bob := seedMemberUser(t, database, "dmrefresh-ok-bob") + dmID := seedDMChannel(t, database, alice.ID, bob.ID) + + send := make(chan []byte, 32) + c := ws.NewTestClientWithUser(hub, alice, 0, send) + hub.Register(c) + time.Sleep(20 * time.Millisecond) + + hub.HandleMessageForTest(c, voiceJoinMsg(dmID)) + time.Sleep(50 * time.Millisecond) + drainChanBuf(send) + + hub.HandleMessageForTest(c, voiceTokenRefreshMsg()) + time.Sleep(50 * time.Millisecond) + + if !hasVoiceToken(t, drainChanTimeout(send, 200*time.Millisecond)) { + t.Error("a DM participant must still be able to refresh their voice token") + } +} diff --git a/Server/ws/voice_join.go b/Server/ws/voice_join.go index 2da58f7c..b3e36e36 100644 --- a/Server/ws/voice_join.go +++ b/Server/ws/voice_join.go @@ -58,7 +58,10 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe return } - if !h.requireChannelPerm(ctx, c, channelID, permissions.ConnectVoice, "CONNECT_VOICE") { + // channel_id is attacker-controlled, so the gate must be channel-TYPE aware: + // a role-only check passes for any DM channel id (DMs have no overrides), and + // the token minted below carries RoomJoin+CanSubscribe for that DM's room. + if !h.requireChannelAccess(ctx, c, channelID, permissions.ConnectVoice, "CONNECT_VOICE") { return } @@ -277,7 +280,10 @@ func handleVoiceTokenRefreshV2(ctx context.Context, cmd Command, info ClientInfo // alone would leave the live session in place, so the refusal also evicts: // LeaveVoice runs handleVoiceLeave, which clears the client's voice state, // deletes the voice_states row and removes the LiveKit participant. - if !hasPerm(ctx, d.DB, d.Permissions, userID, channelID, permissions.ConnectVoice) { + // Channel-type aware, like the voice_join gate: this mints the same + // RoomJoin+CanSubscribe credential, so a role-only check here would keep + // re-issuing one for a DM the user is not a participant of. + if !hasChannelAccess(ctx, d.DB, d.Permissions, userID, channelID, permissions.ConnectVoice) { return Result{ Error: ClientError{Code: ErrCodeForbidden, Message: "missing CONNECT_VOICE permission"}, LeaveVoice: true,