From 35acc09121fc1490ed14a00b57eaf378ce390dc6 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:16:01 +0200 Subject: [PATCH] fix(ws): filter channel metadata broadcasts by READ_MESSAGES (F9) channel_create and channel_update were handed to BroadcastToAll and enqueued with channelID 0, so the full channel payload -- name, topic and category of a channel that channel_overrides hides from the recipient's role -- went to every connected client and was replayed unconditionally from the ring buffer. Both now resolve an audience through the same READ_MESSAGES helper the voice path uses and enqueue under the real channel id, which filters live delivery and both replay tiers by one mechanism. channel_delete stays unfiltered by design: the row is already gone, so a check there would strand the channel in the sidebar of users who saw it via a positive override. Verified by a panel of agents; a base-revert control fails on both the live leak and the replay leak, while the pre-existing broadcast tests pass unmodified. Co-Authored-By: Claude Opus 5 (1M context) --- Server/ws/hub.go | 41 ++++++++--- Server/ws/hub_channel_meta_test.go | 112 +++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 8 deletions(-) create mode 100644 Server/ws/hub_channel_meta_test.go diff --git a/Server/ws/hub.go b/Server/ws/hub.go index f3281f00..1307e27e 100644 --- a/Server/ws/hub.go +++ b/Server/ws/hub.go @@ -544,28 +544,36 @@ func (h *Hub) BroadcastToAll(msg []byte) { // The audience is resolved here, on the caller's goroutine, so the hub's // dispatch loop never blocks on permission lookups. func (h *Hub) broadcastVoiceEvent(ctx context.Context, channelID int64, msg []byte) { + h.broadcastChannelScoped(ctx, channelID, msg, "voice event") +} + +// broadcastChannelScoped enqueues msg for exactly the connected clients whose +// current role may READ channelID, tagged with that channel id so reconnect +// replay filters it too (EventsSinceFiltered replays a channelID of 0 +// unconditionally). kind only labels the drop warning. +func (h *Hub) broadcastChannelScoped(ctx context.Context, channelID int64, msg []byte, kind string) { bm := broadcastMsg{ channelID: channelID, msg: msg, - recipients: h.voiceEventAudience(ctx, channelID), + recipients: h.channelReadAudience(ctx, channelID), } select { case h.broadcast <- bm: default: h.broadcastDrops.Add(1) - slog.Warn("hub: broadcast channel full, dropping voice event", + slog.Warn("hub: broadcast channel full, dropping "+kind, "channel_id", channelID, "msg_len", len(msg)) } } -// voiceEventAudience returns the connected user IDs whose current role may READ +// channelReadAudience returns the connected user IDs whose current role may READ // channelID. Always non-nil, so an empty result means "deliver to nobody" // rather than "no filter". Roles are resolved per client (an admin may have // reassigned one mid-session) and the channel verdict is memoised per role, so // the cost is one role lookup per connected client plus one override lookup per // distinct role. Fails closed: a client whose role cannot be resolved is left // out. Mirrors RefreshChannelVisibility, which resolves visibility the same way. -func (h *Hub) voiceEventAudience(ctx context.Context, channelID int64) []int64 { +func (h *Hub) channelReadAudience(ctx context.Context, channelID int64) []int64 { h.mu.RLock() userIDs := make([]int64, 0, len(h.clients)) for uid := range h.clients { @@ -602,17 +610,34 @@ func (h *Hub) BroadcastServerRestart(reason string, delaySeconds int) { h.BroadcastToAll(buildServerRestartMsg(reason, delaySeconds)) } -// BroadcastChannelCreate sends a channel_create message to all connected clients. +// BroadcastChannelCreate sends a channel_create message to the connected +// clients whose current role may READ ch. It used to go out via BroadcastToAll, +// which handed every authenticated client the name, category and topic of a +// channel that channel_overrides hides from their role — metadata the ready +// payload (buildReady/VisibleChannelIDs) deliberately withholds. +// +// The admin HubBroadcaster interface carries no context, so — like +// RefreshChannelVisibility — the audience is resolved against Background: the +// fan-out must complete regardless of the triggering request. func (h *Hub) BroadcastChannelCreate(ch *db.Channel) { - h.BroadcastToAll(buildChannelCreate(ch)) + h.broadcastChannelScoped(context.Background(), ch.ID, buildChannelCreate(ch), "channel_create") } -// BroadcastChannelUpdate sends a channel_update message to all connected clients. +// BroadcastChannelUpdate sends a channel_update message to the connected +// clients whose current role may READ ch. Same disclosure as +// BroadcastChannelCreate; same filtered fan-out. func (h *Hub) BroadcastChannelUpdate(ch *db.Channel) { - h.BroadcastToAll(buildChannelUpdate(ch)) + h.broadcastChannelScoped(context.Background(), ch.ID, buildChannelUpdate(ch), "channel_update") } // BroadcastChannelDelete sends a channel_delete message to all connected clients. +// +// Deliberately unfiltered: the payload is the bare channel id, with none of the +// metadata create/update carry, and by the time the admin handler calls this the +// channel row — and with it the ON DELETE CASCADE'd channel_overrides — is +// already gone, so a permission check here would answer from base role perms +// and could drop the delete for exactly the users who saw the channel via a +// positive override, stranding it in their sidebar. func (h *Hub) BroadcastChannelDelete(channelID int64) { h.BroadcastToAll(buildChannelDelete(channelID)) } diff --git a/Server/ws/hub_channel_meta_test.go b/Server/ws/hub_channel_meta_test.go new file mode 100644 index 00000000..f66381ad --- /dev/null +++ b/Server/ws/hub_channel_meta_test.go @@ -0,0 +1,112 @@ +package ws_test + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/owncord/server/db" + "github.com/owncord/server/permissions" + "github.com/owncord/server/ws" +) + +// countChannelMetaFor counts channel_create / channel_update events for a +// specific channel id in a batch of raw WS frames. +func countChannelMetaFor(msgs [][]byte, channelID int64) int { + n := 0 + for _, m := range msgs { + var env struct { + Type string `json:"type"` + Payload struct { + ID int64 `json:"id"` + } `json:"payload"` + } + if json.Unmarshal(m, &env) != nil { + continue + } + if env.Payload.ID != channelID { + continue + } + if env.Type == "channel_create" || env.Type == "channel_update" { + n++ + } + } + return n +} + +// TestChannelMetadata_NotDeliveredToRolesDeniedRead locks the visibility +// invariant for channel metadata: channel_create / channel_update used to go +// out via BroadcastToAll, so every authenticated client learned the name, +// category and topic of a channel that channel_overrides hides from their role — +// live, and again on reconnect, since an event stored under channelID 0 is +// replayed unconditionally. A role that may READ the channel must still receive +// both, live and on replay. +func TestChannelMetadata_NotDeliveredToRolesDeniedRead(t *testing.T) { + hub, database := newHandlerHub(t) + + pubID := seedTestChannel(t, database, "chmeta-general") + privID := seedTestChannel(t, database, "chmeta-leadership") + + insider := seedMemberUser(t, database, "chmeta-insider") // role 4: base READ_MESSAGES + outsider := seedModUser(t, database, "chmeta-outsider") // role 3: denied READ below + if err := database.UpsertChannelOverride( + context.Background(), privID, outsider.RoleID, 0, permissions.ReadMessages, + ); err != nil { + t.Fatalf("UpsertChannelOverride: %v", err) + } + + insiderSend := make(chan []byte, 64) + outsiderSend := make(chan []byte, 64) + hub.Register(ws.NewTestClientWithUser(hub, insider, 0, insiderSend)) + hub.Register(ws.NewTestClientWithUser(hub, outsider, 0, outsiderSend)) + time.Sleep(30 * time.Millisecond) + + pub := &db.Channel{ID: pubID, Name: "chmeta-general", Type: "text", Category: "Text"} + priv := &db.Channel{ + ID: privID, Name: "chmeta-leadership", Type: "text", + Category: "Staff", Topic: "acquisition talks", + } + hub.BroadcastChannelCreate(pub) + hub.BroadcastChannelUpdate(pub) + hub.BroadcastChannelCreate(priv) + hub.BroadcastChannelUpdate(priv) + time.Sleep(150 * time.Millisecond) + + // ── live delivery ───────────────────────────────────────────────────────── + insiderLive := drainChanTimeout(insiderSend, 200*time.Millisecond) + outsiderLive := drainChanTimeout(outsiderSend, 200*time.Millisecond) + + if got := countChannelMetaFor(insiderLive, privID); got != 2 { + t.Errorf("insider received %d channel_create/channel_update for the private channel, want 2", got) + } + // Positive control: the outsider is connected and receiving, so a zero count + // on the private channel is filtering and not a broken delivery path. + if got := countChannelMetaFor(outsiderLive, pubID); got != 2 { + t.Errorf("outsider received %d channel_create/channel_update for the readable channel, want 2", got) + } + if got := countChannelMetaFor(outsiderLive, privID); got != 0 { + t.Errorf("a role denied READ received %d channel metadata events for the private channel, want 0", got) + } + + // ── reconnect replay ────────────────────────────────────────────────────── + oldest := hub.ReplayBuffer().OldestSeq() + if oldest == 0 { + t.Fatal("replay buffer recorded no channel events (oldest seq is 0)") + } + replayFor := func(u *db.User) [][]byte { + t.Helper() + allowed, err := hub.ComputeAllowedChannelsForTest(database, u) + if err != nil { + t.Fatalf("ComputeAllowedChannelsForTest: %v", err) + } + return hub.ReplayBuffer().EventsSinceFiltered(oldest+1, allowed) + } + + if got := countChannelMetaFor(replayFor(insider), privID); got != 2 { + t.Errorf("insider replay contained %d channel metadata events for the private channel, want 2", got) + } + if got := countChannelMetaFor(replayFor(outsider), privID); got != 0 { + t.Errorf("replay leaked %d channel metadata events for the private channel to a role denied READ, want 0", got) + } +}