From 67fdd18d7ecc1acd6932d68e4e31324da34eb7c8 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Sat, 29 Aug 2026 09:07:04 +0200 Subject: [PATCH] feat(b2-5): one permission predicate per security property (#1440) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(b2-5): canonical permission predicates One value-taking predicate per security property in Server/permissions: CanViewChannel, CanAdmitSession (= view), CanSendMessage, CanType (= send), CanJoinVoice, CanModerateVoice, all over a Subject the caller resolves (role bits, both override layers, channel flags, DM state). Checker now resolves a Subject and asks it, so HasChannelPerm, HasChannelPermBatch and VisibleChannelIDs are the same rule rather than three copies. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * fix(b2-5): send sites delegate to CanSendMessage (S-01) checkSendPermission, HandleTyping, the ready payload's can_send and the composer refresh all ask permissions.CanSendMessage over a resolved Subject (PermissionService.Subject / ws subjectFor). Typing now follows the post policy: a read-only member, an announcement reader without MANAGE_MESSAGES, an archived channel, a blocked or non-participant DM user emit nothing. Parity tables run each site against the predicate over the same fixture, in both the cached-service and bare-hub branches. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * refactor(b2-5): view sites delegate to CanViewChannel/CanAdmitSession (S-12) HandleChannelFocus and the post-Subscribe revalidation (applySetChannelID) ask permissions.CanAdmitSession; channelReadAudience and RefreshChannelVisibility ask CanViewChannel — all over a Subject resolved by subjectFor in either the cached-service or bare-hub branch, so no ws path mirrors the visibility rule by hand any more. hasPermChecked is gone with its last caller. Parity tables per site, both branches, every override layer plus an archived channel. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * refactor(b2-5): voice join sites delegate to CanJoinVoice voice_join, voice_token_refresh, the destination of a moderator move and the stale-voice sweep all ask permissions.CanJoinVoice over the subject the new ws channelSubject resolves (role bits, both override layers, channel flags, DM membership and block state); joinDenial maps a refusal to the frame each reason always produced. hasChannelAccess, hasChannelAccessLive and Hub.requireChannelAccess are gone with their last callers. The sweep now re-runs the whole join rule (a deleted or archived channel, a lost DM membership or a new block evict too, not only a lost CONNECT_VOICE bit), and the token refresh refuses a deleted channel. Parity tables cover the shared resolver, the join gate and the sweep in both branches. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * fix(b2-5): voice moderation delegates to CanModerateVoice (SEC-02 server half) voiceModTarget decides with permissions.CanModerateVoice over the actor's subject in the target's channel: effective MUTE_MEMBERS there (a role-layer or user-layer deny now holds), READ_MESSAGES so a hidden room cannot be moderated, and DM membership for a DM call. The base-bit check stays as an early rejection only, keeping FORBIDDEN ahead of the voice-state lookup. Locked by a table over both override layers, a hidden channel and the Administrator bypass, through the real voice_mod_mute path; the deafen-race fixtures gain the Checker the gate now needs. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * docs(b2-5): evidence block, inventory and closed rows Record the B2-5 evidence (pre-squash SHAs, before/after inventory, the SEC-02 READ decision, the residue that leaves the authz-chokepoint rule with B3 item 15) in the plan, mark the step done, and flip S-01, S-12 and the server half of SEC-02 to resolved/superseded in the issue register. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * fix(b2-5): CanJoinVoice refuses an archived DM call too The old voice_join gate refused every archived channel regardless of type, and the admin PATCH accepts archived for a DM; the predicate's DM branch returned before consulting the flag, so join, token refresh and the sweep would have let an evicted participant back into an archived call. Archive is now checked after membership and block for both channel kinds (Codex P2 on #1440), pinned in the predicate table. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * docs(b2-5): record the Codex P2 fix in the evidence block Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu --------- Co-authored-by: Claude Fable 5 --- Server/permissions/checker.go | 49 +- Server/permissions/predicates.go | 166 +++++++ Server/permissions/predicates_test.go | 149 +++++++ Server/service/channel.go | 49 +- Server/service/message_perms.go | 78 ++-- Server/service/permission.go | 18 + Server/service/predicate_parity_test.go | 166 +++++++ Server/ws/can_send_test.go | 11 + Server/ws/coverage_voice_lifecycle_test.go | 10 +- Server/ws/deps.go | 164 +++---- Server/ws/handlers.go | 53 +-- Server/ws/hub_broadcast.go | 136 ++---- Server/ws/hub_sweep.go | 45 +- Server/ws/predicate_parity_internal_test.go | 419 ++++++++++++++++++ Server/ws/serve_ready.go | 43 +- Server/ws/voice_join.go | 95 ++-- Server/ws/voice_moderation.go | 63 ++- .../ws/voice_moderation_deafen_race_test.go | 5 +- Server/ws/voice_moderation_overrides_test.go | 72 +++ .../b2-protocol-trust-compat-2026-08-28.md | 116 ++++- .../repo-health-issue-register-2026-08-23.md | 50 +-- 21 files changed, 1482 insertions(+), 475 deletions(-) create mode 100644 Server/permissions/predicates.go create mode 100644 Server/permissions/predicates_test.go create mode 100644 Server/service/predicate_parity_test.go create mode 100644 Server/ws/predicate_parity_internal_test.go create mode 100644 Server/ws/voice_moderation_overrides_test.go diff --git a/Server/permissions/checker.go b/Server/permissions/checker.go index 75195bbe..f6e17428 100644 --- a/Server/permissions/checker.go +++ b/Server/permissions/checker.go @@ -67,22 +67,38 @@ func NewChecker(db DB) *Checker { // userID may be 0 for a check that is genuinely role-only (no member in hand); // the per-user layer is then skipped rather than queried for a nonexistent id. func (ck *Checker) HasChannelPerm(ctx context.Context, rolePerms int64, roleID, userID, channelID, perm int64) bool { - if HasAdmin(rolePerms) { - return true - } - allow, deny, err := ck.db.GetChannelPermissions(ctx, channelID, roleID) + s, err := ck.Subject(ctx, rolePerms, roleID, userID, channelID) if err != nil { return false } - o := ChannelOverride{Allow: allow, Deny: deny} + return s.Has(perm) +} + +// Subject resolves both override layers for the member in channelID, live +// from the database, and returns them with the role bits as a Subject for the +// value-taking predicates (CanViewChannel and friends). The channel's flags +// and any DM state are the caller's to fill in. Administrator skips the fetch +// — overrides never change its verdict. A lookup failure is returned, not +// collapsed, so error-aware callers (the voice sweep) can tell a transient +// read failure from a denial; HasChannelPerm collapses it to false. +func (ck *Checker) Subject(ctx context.Context, rolePerms int64, roleID, userID, channelID int64) (Subject, error) { + s := Subject{RolePerms: rolePerms} + if HasAdmin(rolePerms) { + return s, nil + } + allow, deny, err := ck.db.GetChannelPermissions(ctx, channelID, roleID) + if err != nil { + return Subject{}, err + } + s.Override = ChannelOverride{Allow: allow, Deny: deny} if userID != 0 { uAllow, uDeny, uErr := ck.db.GetUserChannelPermissions(ctx, channelID, userID) if uErr != nil { - return false + return Subject{}, uErr } - o.UserAllow, o.UserDeny = uAllow, uDeny + s.Override.UserAllow, s.Override.UserDeny = uAllow, uDeny } - return EffectiveChannelPerms(rolePerms, o)&perm == perm + return s, nil } // HasChannelPermBatch reports whether the member has the given permission on @@ -91,11 +107,7 @@ func (ck *Checker) HasChannelPerm(ctx context.Context, rolePerms int64, roleID, // channels in bulk. The zero-value ChannelOverride (no entry in map) is correct // -- it means no override exists at either layer. func (ck *Checker) HasChannelPermBatch(rolePerms int64, overrides map[int64]ChannelOverride, channelID, perm int64) bool { - if HasAdmin(rolePerms) { - return true - } - o := overrides[channelID] // zero value when no override exists - return EffectiveChannelPerms(rolePerms, o)&perm == perm + return Subject{RolePerms: rolePerms, Override: overrides[channelID]}.Has(perm) } // VisibleChannelIDs returns the set of non-DM channel IDs the member @@ -113,13 +125,10 @@ func (ck *Checker) VisibleChannelIDs(rolePerms int64, channels []ChannelRef, ove if ch.Type == "dm" { continue } - // Archived channels are hidden from every client surface (admins - // included) — they stay manageable from the admin panel, which lists - // channels without this predicate. - if ch.Archived { - continue - } - if ck.HasChannelPermBatch(rolePerms, overrides, ch.ID, ReadMessages) { + // CanViewChannel hides archived channels from every client surface + // (admins included) — they stay manageable from the admin panel, + // which lists channels without this predicate. + if CanViewChannel(Subject{RolePerms: rolePerms, Override: overrides[ch.ID], Channel: ch}) == nil { visible[ch.ID] = true } } diff --git a/Server/permissions/predicates.go b/Server/permissions/predicates.go new file mode 100644 index 00000000..41f28dd1 --- /dev/null +++ b/Server/permissions/predicates.go @@ -0,0 +1,166 @@ +package permissions + +import ( + "errors" + "fmt" +) + +// One predicate per security property (B2-5). Each is a pure function over a +// Subject the caller has already resolved — no context, no store — so a call +// site cannot drift by re-deriving half the rule: ready's can_send, the +// composer refresh, typing, the send path and the plugin gate all ask +// CanSendMessage the same question. A nil result is "allowed"; a non-nil one +// is a sentinel from this package (or wraps ErrPermissionDenied with the +// missing bit's name) so each call site keeps mapping denials to its own +// status codes and messages. + +// ErrArchived is returned for a write to, or a client surface for, an archived +// channel. History stays readable, so read paths do not consult it. +var ErrArchived = errors.New("channel is archived") + +// ErrBlocked is returned when either DM party has blocked the other. +var ErrBlocked = errors.New("user is blocked") + +// ErrNotVoiceChannel is returned by CanJoinVoice for a channel type that has +// no voice room (text, announcement). +var ErrNotVoiceChannel = errors.New("not a voice channel") + +// Subject is everything a channel predicate consults: the actor's role bits, +// both override layers for the one channel in question, the channel's flags, +// and — for a DM — the membership and block state the caller looked up. The +// zero value is a subject with no role and no membership, which every +// predicate refuses. +type Subject struct { + RolePerms int64 + Override ChannelOverride + Channel ChannelRef // Type and Archived; ID is not consulted + // DMParticipant and DMBlocked are consulted only when Channel.Type is + // "dm". Group DMs pass DMBlocked=false — blocks are enforced at group + // creation, not per message (service.requireDMNotBlocked). + DMParticipant bool + DMBlocked bool +} + +// Has reports whether the subject's effective permission in the channel holds +// every bit of perm. Administrator bypasses both override layers; a zero perm +// is never held. This is the single value-taking bit predicate — Checker and +// PermissionService resolve a Subject and ask it. +func (s Subject) Has(perm int64) bool { + return HasAdmin(s.RolePerms) || HasPerm(EffectiveChannelPerms(s.RolePerms, s.Override), perm) +} + +// missing wraps ErrPermissionDenied with the bit a caller would name in its +// own FORBIDDEN message. For a multi-bit perm the named bit is the last one +// the caller listed as the "reason" (SEND for READ|SEND). +func missing(named int64) error { + return fmt.Errorf("%w: missing %s", ErrPermissionDenied, Name(named)) +} + +// dmMember refuses a non-participant. Every DM rule starts here and nothing +// about the role bypasses it — an administrator is not in someone else's DM. +func dmMember(s Subject) error { + if !s.DMParticipant { + return ErrNotDMParticipant + } + return nil +} + +// CanViewChannel is visibility: what the sidebar lists, the ready payload +// carries, reconnect replay filters to, and channel-scoped fan-out reaches. +// Permission is checked before the archive flag so an unauthorized caller +// learns nothing about the channel from the error; archived channels are then +// hidden from everyone, admins included (they stay manageable from the admin +// panel, which does not use this predicate). +func CanViewChannel(s Subject) error { + if s.Channel.Type == "dm" { + return dmMember(s) + } + if !s.Has(ReadMessages) { + return missing(ReadMessages) + } + if s.Channel.Archived { + return ErrArchived + } + return nil +} + +// CanAdmitSession decides whether a live socket may attach to a channel's +// stream (channel_focus, the post-Subscribe revalidation). It is +// CanViewChannel by definition: a session sees exactly what the user sees. +func CanAdmitSession(s Subject) error { return CanViewChannel(s) } + +// CanSendMessage is the post policy: READ and SEND in the channel, MANAGE on +// top for announcement channels, never into an archive; for a DM, membership +// and no block in either direction. SendMessage, EditMessage, CanPost, the +// ready payload's can_send, the composer refresh and typing all delegate here. +func CanSendMessage(s Subject) error { + if s.Channel.Type == "dm" { + if err := dmMember(s); err != nil { + return err + } + if s.DMBlocked { + return ErrBlocked + } + return nil + } + if !s.Has(ReadMessages | SendMessages) { + return missing(SendMessages) + } + if s.Channel.Type == "announcement" && !s.Has(ManageMessages) { + return missing(ManageMessages) + } + if s.Channel.Archived { + return ErrArchived + } + return nil +} + +// CanType is CanSendMessage (S-01): a typing indicator announces a post, so a +// member who cannot post cannot announce one. +func CanType(s Subject) error { return CanSendMessage(s) } + +// CanJoinVoice gates the LiveKit credential: CONNECT_VOICE in the channel +// (required for DM calls too — the role bit was always demanded on top of +// membership, so this can only ever narrow), a channel that has a room, for +// a DM membership plus no block, and no archive for either kind — the admin +// PATCH accepts `archived` for any channel type, and an evicted participant +// must not rejoin the archived room. Applies at join, at token refresh, to +// the target of a moderator move, and in the stale-voice sweep. +func CanJoinVoice(s Subject) error { + if !s.Has(ConnectVoice) { + return missing(ConnectVoice) + } + switch s.Channel.Type { + case "dm": + if err := dmMember(s); err != nil { + return err + } + if s.DMBlocked { + return ErrBlocked + } + case "voice": + default: + return ErrNotVoiceChannel + } + if s.Channel.Archived { + return ErrArchived + } + return nil +} + +// CanModerateVoice is the actor's authority in the TARGET's channel: +// effective MUTE_MEMBERS there (so a per-channel deny holds — SEC-02), and +// READ_MESSAGES so a moderator can only act in a room they can see; a DM call +// additionally requires the actor to be a participant. Rank is not a +// permission and stays with the caller (the strict-outranks check). +func CanModerateVoice(s Subject) error { + if s.Channel.Type == "dm" { + if err := dmMember(s); err != nil { + return err + } + } + if !s.Has(ReadMessages | MuteMembers) { + return missing(MuteMembers) + } + return nil +} diff --git a/Server/permissions/predicates_test.go b/Server/permissions/predicates_test.go new file mode 100644 index 00000000..bfe5799b --- /dev/null +++ b/Server/permissions/predicates_test.go @@ -0,0 +1,149 @@ +package permissions + +import ( + "errors" + "testing" +) + +// Each predicate is one security property. The tables below are the property +// stated as cases; every call site that used to hand-roll a copy of the rule +// now delegates here and carries a parity test against these verdicts. + +const ( + memberBits = ReadMessages | SendMessages | ConnectVoice + modBits = memberBits | ManageMessages | MuteMembers +) + +func text(archived bool) ChannelRef { return ChannelRef{Type: "text", Archived: archived} } +func announcement() ChannelRef { return ChannelRef{Type: "announcement"} } +func voice(archived bool) ChannelRef { return ChannelRef{Type: "voice", Archived: archived} } +func dm() ChannelRef { return ChannelRef{Type: "dm"} } +func deny(bits int64) ChannelOverride { return ChannelOverride{Deny: bits} } +func userDeny(bits int64) ChannelOverride { return ChannelOverride{UserDeny: bits} } +func allow(bits int64) ChannelOverride { return ChannelOverride{Allow: bits} } +func userAllow(bits int64) ChannelOverride { return ChannelOverride{UserAllow: bits} } + +type predicateCase struct { + name string + s Subject + want error // nil = allowed; otherwise errors.Is(got, want) +} + +func runPredicate(t *testing.T, name string, fn func(Subject) error, cases []predicateCase) { + t.Helper() + for _, c := range cases { + got := fn(c.s) + if c.want == nil && got != nil { + t.Errorf("%s/%s: want allowed, got %v", name, c.name, got) + } + if c.want != nil && !errors.Is(got, c.want) { + t.Errorf("%s/%s: want %v, got %v", name, c.name, c.want, got) + } + } +} + +func TestCanViewChannel(t *testing.T) { + runPredicate(t, "CanViewChannel", CanViewChannel, []predicateCase{ + {"member reads text", Subject{RolePerms: memberBits, Channel: text(false)}, nil}, + {"no role fails closed", Subject{Channel: text(false)}, ErrPermissionDenied}, + {"role deny READ hides", Subject{RolePerms: memberBits, Override: deny(ReadMessages), Channel: text(false)}, ErrPermissionDenied}, + {"user deny READ hides", Subject{RolePerms: memberBits, Override: userDeny(ReadMessages), Channel: text(false)}, ErrPermissionDenied}, + {"user allow beats role deny", Subject{RolePerms: memberBits, Override: ChannelOverride{Deny: ReadMessages, UserAllow: ReadMessages}, Channel: text(false)}, nil}, + {"admin bypasses deny", Subject{RolePerms: Administrator, Override: deny(ReadMessages), Channel: text(false)}, nil}, + {"archived hidden from members", Subject{RolePerms: memberBits, Channel: text(true)}, ErrArchived}, + {"archived hidden from admins", Subject{RolePerms: Administrator, Channel: text(true)}, ErrArchived}, + {"unauthorized never learns archived", Subject{Channel: text(true)}, ErrPermissionDenied}, + {"dm participant sees", Subject{Channel: dm(), DMParticipant: true}, nil}, + {"dm non-participant blind, even admin", Subject{RolePerms: Administrator, Channel: dm()}, ErrNotDMParticipant}, + {"dm ignores block", Subject{Channel: dm(), DMParticipant: true, DMBlocked: true}, nil}, + }) +} + +func TestCanSendMessage(t *testing.T) { + cases := []predicateCase{ + {"member posts in text", Subject{RolePerms: memberBits, Channel: text(false)}, nil}, + {"reader without SEND refused", Subject{RolePerms: ReadMessages, Channel: text(false)}, ErrPermissionDenied}, + {"SEND without READ refused", Subject{RolePerms: SendMessages, Channel: text(false)}, ErrPermissionDenied}, + {"role deny SEND refused", Subject{RolePerms: memberBits, Override: deny(SendMessages), Channel: text(false)}, ErrPermissionDenied}, + {"user deny SEND refused", Subject{RolePerms: memberBits, Override: userDeny(SendMessages), Channel: text(false)}, ErrPermissionDenied}, + {"announcement needs MANAGE", Subject{RolePerms: memberBits, Channel: announcement()}, ErrPermissionDenied}, + {"moderator posts in announcement", Subject{RolePerms: modBits, Channel: announcement()}, nil}, + {"override allow MANAGE enables announcement", Subject{RolePerms: memberBits, Override: allow(ManageMessages), Channel: announcement()}, nil}, + {"user allow MANAGE enables announcement", Subject{RolePerms: memberBits, Override: userAllow(ManageMessages), Channel: announcement()}, nil}, + {"admin bypasses on announcement", Subject{RolePerms: Administrator, Channel: announcement()}, nil}, + {"archived refuses members", Subject{RolePerms: memberBits, Channel: text(true)}, ErrArchived}, + {"archived refuses admins", Subject{RolePerms: Administrator, Channel: text(true)}, ErrArchived}, + {"unauthorized never learns archived", Subject{RolePerms: ReadMessages, Channel: text(true)}, ErrPermissionDenied}, + {"dm participant posts without role bits", Subject{Channel: dm(), DMParticipant: true}, nil}, + {"dm non-participant refused", Subject{RolePerms: Administrator, Channel: dm()}, ErrNotDMParticipant}, + {"dm blocked refused", Subject{Channel: dm(), DMParticipant: true, DMBlocked: true}, ErrBlocked}, + } + runPredicate(t, "CanSendMessage", CanSendMessage, cases) + // CanType is CanSendMessage by definition (S-01): a typing indicator is + // the first half of a post, so it answers to the same rule. + runPredicate(t, "CanType", CanType, cases) +} + +func TestCanAdmitSession(t *testing.T) { + // Session admission (channel_focus / topic subscribe) is visibility. + cases := []predicateCase{ + {"member admitted", Subject{RolePerms: memberBits, Channel: text(false)}, nil}, + {"archived refused", Subject{RolePerms: memberBits, Channel: text(true)}, ErrArchived}, + {"dm participant admitted", Subject{Channel: dm(), DMParticipant: true}, nil}, + {"dm non-participant refused", Subject{Channel: dm()}, ErrNotDMParticipant}, + } + runPredicate(t, "CanAdmitSession", CanAdmitSession, cases) + runPredicate(t, "CanViewChannel(parity)", CanViewChannel, cases) +} + +func TestCanJoinVoice(t *testing.T) { + runPredicate(t, "CanJoinVoice", CanJoinVoice, []predicateCase{ + {"member joins voice", Subject{RolePerms: memberBits, Channel: voice(false)}, nil}, + {"no CONNECT refused", Subject{RolePerms: ReadMessages, Channel: voice(false)}, ErrPermissionDenied}, + {"role deny CONNECT refused", Subject{RolePerms: memberBits, Override: deny(ConnectVoice), Channel: voice(false)}, ErrPermissionDenied}, + {"user deny CONNECT refused", Subject{RolePerms: memberBits, Override: userDeny(ConnectVoice), Channel: voice(false)}, ErrPermissionDenied}, + {"admin bypasses deny", Subject{RolePerms: Administrator, Override: deny(ConnectVoice), Channel: voice(false)}, nil}, + {"text channel is not voice", Subject{RolePerms: memberBits, Channel: text(false)}, ErrNotVoiceChannel}, + {"archived voice refused", Subject{RolePerms: memberBits, Channel: voice(true)}, ErrArchived}, + {"unauthorized never learns archived", Subject{RolePerms: ReadMessages, Channel: voice(true)}, ErrPermissionDenied}, + {"dm call needs CONNECT bit too", Subject{Channel: dm(), DMParticipant: true}, ErrPermissionDenied}, + {"dm participant with CONNECT joins", Subject{RolePerms: ConnectVoice, Channel: dm(), DMParticipant: true}, nil}, + {"dm non-participant refused", Subject{RolePerms: Administrator, Channel: dm()}, ErrNotDMParticipant}, + {"dm blocked refused", Subject{RolePerms: ConnectVoice, Channel: dm(), DMParticipant: true, DMBlocked: true}, ErrBlocked}, + // The admin PATCH accepts archived for any channel type, and the old + // voice_join gate refused every archived channel — an evicted + // participant must not rejoin an archived DM call (Codex P2, #1440). + {"archived dm call refused", Subject{RolePerms: ConnectVoice, Channel: ChannelRef{Type: "dm", Archived: true}, DMParticipant: true}, ErrArchived}, + {"archived dm non-participant learns nothing", Subject{RolePerms: ConnectVoice, Channel: ChannelRef{Type: "dm", Archived: true}}, ErrNotDMParticipant}, + }) +} + +func TestCanModerateVoice(t *testing.T) { + runPredicate(t, "CanModerateVoice", CanModerateVoice, []predicateCase{ + {"moderator with MUTE in channel", Subject{RolePerms: modBits, Channel: voice(false)}, nil}, + {"no MUTE refused", Subject{RolePerms: memberBits, Channel: voice(false)}, ErrPermissionDenied}, + {"role deny MUTE in this channel refused", Subject{RolePerms: modBits, Override: deny(MuteMembers), Channel: voice(false)}, ErrPermissionDenied}, + {"user deny MUTE in this channel refused", Subject{RolePerms: modBits, Override: userDeny(MuteMembers), Channel: voice(false)}, ErrPermissionDenied}, + {"channel hidden from moderator refused", Subject{RolePerms: modBits, Override: deny(ReadMessages), Channel: voice(false)}, ErrPermissionDenied}, + {"admin bypasses deny", Subject{RolePerms: Administrator, Override: deny(MuteMembers), Channel: voice(false)}, nil}, + {"dm call needs actor membership", Subject{RolePerms: modBits, Channel: dm()}, ErrNotDMParticipant}, + {"dm participant moderator allowed", Subject{RolePerms: modBits, Channel: dm(), DMParticipant: true}, nil}, + {"dm participant without MUTE refused", Subject{RolePerms: memberBits, Channel: dm(), DMParticipant: true}, ErrPermissionDenied}, + }) +} + +// TestSubjectHas pins the one generic predicate every other one is built on: +// Administrator bypasses overrides, everything else is the resolved two-layer +// mask, and a zero perm is never held (matching HasPerm). +func TestSubjectHas(t *testing.T) { + s := Subject{RolePerms: memberBits, Override: ChannelOverride{Deny: SendMessages, UserAllow: ManageMessages}} + if !s.Has(ReadMessages) || s.Has(SendMessages) || !s.Has(ManageMessages) || s.Has(ReadMessages|SendMessages) { + t.Fatal("Has must apply both override layers and be ALL-of") + } + if s.Has(0) { + t.Fatal("zero perm is never held") + } + if !(Subject{RolePerms: Administrator, Override: deny(AllPerms)}).Has(ManageServer) { + t.Fatal("Administrator bypasses overrides") + } +} diff --git a/Server/service/channel.go b/Server/service/channel.go index 32c91f6d..1f0d4775 100644 --- a/Server/service/channel.go +++ b/Server/service/channel.go @@ -118,19 +118,14 @@ func (s *ChannelService) HandleTyping(ctx context.Context, userID, channelID int return nil, nil //nolint:nilerr // typing indicators are best-effort; errors silently dropped } - if ch.Type == "dm" { - ok, dmErr := s.st.IsDMParticipant(ctx, userID, channelID) - if dmErr != nil || !ok { - return nil, nil //nolint:nilerr // typing indicators are best-effort; errors silently dropped - } - // A blocked user must not be able to keep poking the blocker with - // typing indicators. Same gate as the other DM sinks; silently dropped - // here because typing is best-effort. - if blkErr := requireDMNotBlocked(ctx, s.st, userID, channelID); blkErr != nil { - return nil, nil //nolint:nilerr // best-effort: a blocked or unreadable DM emits nothing - } - } else if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages) { - return nil, nil // silent drop + // A typing indicator announces a post, so it answers to the post policy + // (permissions.CanType is CanSendMessage — S-01): a read-only member, an + // announcement reader without MANAGE_MESSAGES, an archived channel, a + // blocked or non-participant DM user all emit nothing. Silent, because + // typing is best-effort. + sub, subErr := channelSubject(ctx, s.st, s.perms, userID, ch, true) + if subErr != nil || permissions.CanType(sub) != nil { + return nil, nil //nolint:nilerr // best-effort: a denial or a DM lookup failure emits nothing } // Per-user-per-channel rate limit. Built only now that the channel is @@ -247,24 +242,18 @@ func (s *ChannelService) HandleChannelFocus(ctx context.Context, userID, channel return nil, fmt.Errorf("%w: channel not found", ErrNotFound) } - switch { - case ch.Type == "dm": - ok, err := s.st.IsDMParticipant(ctx, userID, channelID) - if err != nil || !ok { - return nil, fmt.Errorf("%w: access denied", ErrForbidden) - } - case !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages): + // Session admission is permissions.CanAdmitSession — visibility, the same + // predicate behind ListVisibleChannels, the ready payload and reconnect + // replay — so a socket that still holds an id it can no longer see (or + // an archived channel, OC-0070) cannot resubscribe to the live topic or + // advance its read state. channel_focus and mark_read share this one + // service call, so the gate closes both at once. + sub, subErr := channelSubject(ctx, s.st, s.perms, userID, ch, false) + if subErr != nil { return nil, fmt.Errorf("%w: access denied", ErrForbidden) - case ch.Archived: - // Archived channels are hidden from every other client surface - // (ListVisibleChannels, ready payload, reconnect replay, voice join — - // see permissions.Checker.VisibleChannelIDs and ws/voice_join.go). - // HasChannelPerm alone doesn't know about the archive flag, so without - // this a socket that still held the id could resubscribe to the live - // topic and advance its own read state on a channel reconnect replay - // then filters back out. channel_focus and mark_read share this one - // service call, so the guard closes both at once (OC-0070). - return nil, fmt.Errorf("%w: channel is archived", ErrForbidden) + } + if err := permissions.CanAdmitSession(sub); err != nil { + return nil, fmt.Errorf("%w: %v", ErrForbidden, err) } // Mark channel as read. latestID == 0 (no undeleted messages) still diff --git a/Server/service/message_perms.go b/Server/service/message_perms.go index 307fe677..92c3efa1 100644 --- a/Server/service/message_perms.go +++ b/Server/service/message_perms.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "github.com/J3vb/OwnCord/Server/db" @@ -69,36 +70,63 @@ func (s *MessageService) CanPost(ctx context.Context, userID, channelID int64) e return s.checkSendPermission(ctx, userID, ch) } -// checkSendPermission validates send permission for ch. Announcement channels -// are readable by anyone with READ_MESSAGES but only postable by users with -// MANAGE_MESSAGES (posting is restricted to moderators/admins); all other -// non-DM channels require SEND_MESSAGES. Also enforces requireChannelWritable, -// so every caller — SendMessage, EditMessage, CanPost — refuses an archived -// channel without re-implementing that check itself. +// checkSendPermission is permissions.CanSendMessage over the resolved subject: +// READ|SEND in the channel, MANAGE_MESSAGES on top for announcement channels, +// never into an archive; DM membership and no block. Every caller — +// SendMessage, EditMessage, CanPost, and typing via ChannelService — asks that +// one predicate, so none can drift (S-01, S-12). func (s *MessageService) checkSendPermission(ctx context.Context, userID int64, ch *db.Channel) error { - isDM := ch.Type == "dm" - if isDM { - ok, err := s.st.IsDMParticipant(ctx, userID, ch.ID) - if err != nil { - return fmt.Errorf("%w: failed to check DM participation: %v", ErrInternal, err) - } - if !ok { - return fmt.Errorf("%w: not a participant in this DM", ErrForbidden) - } - return requireDMNotBlocked(ctx, s.st, userID, ch.ID) - } - if err := requireChannelWritable(ch); err != nil { + sub, err := channelSubject(ctx, s.st, s.perms, userID, ch, true) + if err != nil { return err } - if !s.perms.HasChannelPerm(ctx, userID, ch.ID, permissions.ReadMessages|permissions.SendMessages) { - return fmt.Errorf("%w: missing SEND_MESSAGES permission", ErrForbidden) + return denial(permissions.CanSendMessage(sub)) +} + +// channelSubject resolves what the channel predicates need for userID in ch: +// role bits and both override layers from the permission cache (a lookup +// failure or a missing role yields no bits — fail closed, as HasChannelPerm +// always has), the channel's flags, and for a DM its membership and, when +// withBlock is set, the two-party block state. The only error is a DM lookup +// failure, wrapped as ErrInternal; callers keep their own posture toward it +// (SendMessage reports it, typing drops silently). +func channelSubject(ctx context.Context, st Store, perms *PermissionService, userID int64, ch *db.Channel, withBlock bool) (permissions.Subject, error) { + sub, err := perms.Subject(ctx, userID, ch.ID) + if err != nil { + sub = permissions.Subject{} } - // Announcement channels: posting is restricted to users who can manage - // messages, even though everyone with READ_MESSAGES can view them. - if ch.Type == "announcement" && !s.perms.HasChannelPerm(ctx, userID, ch.ID, permissions.ManageMessages) { - return fmt.Errorf("%w: announcement channels require MANAGE_MESSAGES to post", ErrForbidden) + sub.Channel = permissions.ChannelRef{ID: ch.ID, Type: ch.Type, Archived: ch.Archived} + if ch.Type != "dm" { + return sub, nil + } + ok, dmErr := st.IsDMParticipant(ctx, userID, ch.ID) + if dmErr != nil { + return sub, fmt.Errorf("%w: failed to check DM participation: %v", ErrInternal, dmErr) + } + sub.DMParticipant = ok + if ok && withBlock { + switch blkErr := requireDMNotBlocked(ctx, st, userID, ch.ID); { + case errors.Is(blkErr, ErrBlocked): + sub.DMBlocked = true + case blkErr != nil: + return sub, blkErr + } + } + return sub, nil +} + +// denial maps a predicate verdict onto the service's error kinds: a block is +// ErrBlocked (its own client-visible code), every other refusal ErrForbidden +// carrying the predicate's reason. +func denial(err error) error { + switch { + case err == nil: + return nil + case errors.Is(err, permissions.ErrBlocked): + return fmt.Errorf("%w: user is blocked", ErrBlocked) + default: + return fmt.Errorf("%w: %v", ErrForbidden, err) } - return nil } // requireChannelWritable refuses a write against an archived non-DM channel. diff --git a/Server/service/permission.go b/Server/service/permission.go index e804bd41..5602d792 100644 --- a/Server/service/permission.go +++ b/Server/service/permission.go @@ -96,6 +96,24 @@ func (s *PermissionService) HasChannelPermChecked(ctx context.Context, userID, c return s.checker.HasChannelPermBatch(cp.rolePerms, cp.overrides, channelID, perm), nil } +// Subject resolves the user's role bits and both override layers for +// channelID from the per-user cache, as a permissions.Subject for the +// value-taking predicates (CanSendMessage and friends). Channel flags and DM +// state are the caller's to fill in. A missing role row yields the zero +// Subject (no bits — every predicate refuses it) with a nil error; a store +// failure is returned so callers choose between failing closed and +// reporting it. +func (s *PermissionService) Subject(ctx context.Context, userID, channelID int64) (permissions.Subject, error) { + cp, err := s.getOrPopulate(ctx, userID) + if err != nil { + return permissions.Subject{}, err + } + if cp == nil { + return permissions.Subject{}, nil + } + return permissions.Subject{RolePerms: cp.rolePerms, Override: cp.overrides[channelID]}, nil +} + // RequireChannelAccess checks whether the user can access the channel with // the given permission. For DM channels it verifies participant membership. // For regular channels it uses cached role-based permission checks. diff --git a/Server/service/predicate_parity_test.go b/Server/service/predicate_parity_test.go new file mode 100644 index 00000000..8771459c --- /dev/null +++ b/Server/service/predicate_parity_test.go @@ -0,0 +1,166 @@ +package service + +import ( + "context" + "errors" + "testing" + + "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/permissions" +) + +// B2-5 parity tables: every service call site that decides a security +// property is run against the canonical permissions predicate over the same +// fixture, so the two can never disagree. The fixture covers each input the +// predicates consult — role bits, both override layers, channel type and +// archive flag, DM membership and blocks. + +const ( + parityRoleMember = int64(20) // READ|SEND + parityRoleReader = int64(21) // READ only + parityRoleMod = int64(22) // READ|SEND|MANAGE_MESSAGES + parityRoleAdmin = int64(23) // ADMINISTRATOR only + + parityUserMember = int64(1) + parityUserReader = int64(2) + parityUserMod = int64(3) + parityUserAdmin = int64(4) + parityUserBob = int64(5) // member; DM partner + parityUserEve = int64(6) // member; blocked by Bob + + parityChanText = int64(10) + parityChanAnnouncement = int64(11) + parityChanArchived = int64(12) + parityChanRoleDeny = int64(13) // role override denies SEND to member role + parityChanUserDeny = int64(14) // per-user override denies READ to the member user + parityChanUserAllow = int64(15) // per-user override grants MANAGE_MESSAGES to the member user; announcement + parityChanDM = int64(50) // member <-> bob + parityChanDMBlocked = int64(51) // eve <-> bob, bob blocks eve + parityChanMissing = int64(999) +) + +var parityUsers = []int64{parityUserMember, parityUserReader, parityUserMod, parityUserAdmin, parityUserBob, parityUserEve} + +var parityChannels = []int64{ + parityChanText, parityChanAnnouncement, parityChanArchived, parityChanRoleDeny, + parityChanUserDeny, parityChanUserAllow, parityChanDM, parityChanDMBlocked, parityChanMissing, +} + +func newParityDB(t *testing.T) *db.DB { + t.Helper() + database := newTestDB(t) + seedRole(t, database, &db.Role{ID: parityRoleMember, Name: "p-member", Permissions: permissions.ReadMessages | permissions.SendMessages, Position: 1}) + seedRole(t, database, &db.Role{ID: parityRoleReader, Name: "p-reader", Permissions: permissions.ReadMessages, Position: 1}) + seedRole(t, database, &db.Role{ID: parityRoleMod, Name: "p-mod", Permissions: permissions.ReadMessages | permissions.SendMessages | permissions.ManageMessages, Position: 2}) + seedRole(t, database, &db.Role{ID: parityRoleAdmin, Name: "p-admin", Permissions: permissions.Administrator, Position: 3}) + for uid, rid := range map[int64]int64{ + parityUserMember: parityRoleMember, parityUserReader: parityRoleReader, parityUserMod: parityRoleMod, + parityUserAdmin: parityRoleAdmin, parityUserBob: parityRoleMember, parityUserEve: parityRoleMember, + } { + seedUser(t, database, &db.User{ID: uid, Username: seedUsername(uid)}) + seedUserRole(t, database, uid, rid) + } + seedChannel(t, database, &db.Channel{ID: parityChanText, Name: "text", Type: "text"}) + seedChannel(t, database, &db.Channel{ID: parityChanAnnouncement, Name: "news", Type: "announcement"}) + seedChannel(t, database, &db.Channel{ID: parityChanArchived, Name: "old", Type: "text"}) + if _, err := database.ExecContext(context.Background(), `UPDATE channels SET archived = 1 WHERE id = ?`, parityChanArchived); err != nil { + t.Fatalf("archive: %v", err) + } + seedChannel(t, database, &db.Channel{ID: parityChanRoleDeny, Name: "role-deny", Type: "text"}) + seedChannelOverride(t, database, parityRoleMember, parityChanRoleDeny, 0, permissions.SendMessages) + seedChannel(t, database, &db.Channel{ID: parityChanUserDeny, Name: "user-deny", Type: "text"}) + seedChannelUserOverride(t, database, parityUserMember, parityChanUserDeny, 0, permissions.ReadMessages) + seedChannel(t, database, &db.Channel{ID: parityChanUserAllow, Name: "user-allow", Type: "announcement"}) + seedChannelUserOverride(t, database, parityUserMember, parityChanUserAllow, permissions.ManageMessages, 0) + seedChannel(t, database, &db.Channel{ID: parityChanDM, Name: "dm", Type: "dm"}) + seedDMParticipant(t, database, parityChanDM, parityUserMember) + seedDMParticipant(t, database, parityChanDM, parityUserBob) + seedChannel(t, database, &db.Channel{ID: parityChanDMBlocked, Name: "dm-blocked", Type: "dm"}) + seedDMParticipant(t, database, parityChanDMBlocked, parityUserEve) + seedDMParticipant(t, database, parityChanDMBlocked, parityUserBob) + seedBlock(t, database, parityUserBob, parityUserEve) + return database +} + +// parityWant is the canonical verdict for (user, channel) from the predicate +// over a Subject the test resolves itself — independently of the call site. +func parityWant(t *testing.T, database *db.DB, perms *PermissionService, pred func(permissions.Subject) error, userID, channelID int64) (allowed bool, verdict error) { + t.Helper() + ch, err := database.GetChannel(context.Background(), channelID) + if err != nil { + t.Fatalf("GetChannel(%d): %v", channelID, err) + } + if ch == nil { + return false, ErrNotFound + } + sub, err := channelSubject(context.Background(), database, perms, userID, ch, true) + if err != nil { + t.Fatalf("channelSubject(%d,%d): %v", userID, channelID, err) + } + verdict = pred(sub) + return verdict == nil, verdict +} + +// TestSendPolicyParity: CanPost (the send path) and HandleTyping (S-01) agree +// with CanSendMessage for every (user, channel) in the fixture, including the +// kind of refusal. +func TestSendPolicyParity(t *testing.T) { + database := newParityDB(t) + perms := NewPermissionService(database, permissions.NewChecker(database)) + msgSvc := NewMessageService(database, perms, nil) + chSvc := NewChannelService(database, perms) + ctx := context.Background() + + for _, uid := range parityUsers { + for _, cid := range parityChannels { + wantOK, want := parityWant(t, database, perms, permissions.CanSendMessage, uid, cid) + + got := msgSvc.CanPost(ctx, uid, cid) + if (got == nil) != wantOK { + t.Errorf("CanPost(user=%d, chan=%d) = %v, predicate says %v", uid, cid, got, want) + } + switch { + case errors.Is(want, permissions.ErrBlocked) && !errors.Is(got, ErrBlocked): + t.Errorf("CanPost(user=%d, chan=%d) = %v, want ErrBlocked", uid, cid, got) + case errors.Is(want, ErrNotFound) && !errors.Is(got, ErrNotFound): + t.Errorf("CanPost(user=%d, chan=%d) = %v, want ErrNotFound", uid, cid, got) + case want != nil && !errors.Is(want, permissions.ErrBlocked) && !errors.Is(want, ErrNotFound) && !errors.Is(got, ErrForbidden): + t.Errorf("CanPost(user=%d, chan=%d) = %v, want ErrForbidden", uid, cid, got) + } + + ch, err := chSvc.HandleTyping(ctx, uid, cid, nil) + if err != nil { + t.Errorf("HandleTyping(user=%d, chan=%d) errored: %v", uid, cid, err) + } + if (ch != nil) != wantOK { + t.Errorf("HandleTyping(user=%d, chan=%d) emits=%v, but CanSendMessage says %v (S-01: typing must follow the send policy)", uid, cid, ch != nil, want) + } + } + } +} + +// TestViewPolicyParity: HandleChannelFocus (session admission: channel_focus +// and mark_read) agrees with CanAdmitSession for every (user, channel). +func TestViewPolicyParity(t *testing.T) { + database := newParityDB(t) + perms := NewPermissionService(database, permissions.NewChecker(database)) + chSvc := NewChannelService(database, perms) + ctx := context.Background() + + for _, uid := range parityUsers { + for _, cid := range parityChannels { + wantOK, want := parityWant(t, database, perms, permissions.CanAdmitSession, uid, cid) + + _, got := chSvc.HandleChannelFocus(ctx, uid, cid) + if (got == nil) != wantOK { + t.Errorf("HandleChannelFocus(user=%d, chan=%d) = %v, predicate says %v", uid, cid, got, want) + } + switch { + case errors.Is(want, ErrNotFound) && !errors.Is(got, ErrNotFound): + t.Errorf("HandleChannelFocus(user=%d, chan=%d) = %v, want ErrNotFound", uid, cid, got) + case want != nil && !errors.Is(want, ErrNotFound) && !errors.Is(got, ErrForbidden): + t.Errorf("HandleChannelFocus(user=%d, chan=%d) = %v, want ErrForbidden", uid, cid, got) + } + } + } +} diff --git a/Server/ws/can_send_test.go b/Server/ws/can_send_test.go index ece56f13..9447c65c 100644 --- a/Server/ws/can_send_test.go +++ b/Server/ws/can_send_test.go @@ -38,6 +38,17 @@ func TestChannelCanSend(t *testing.T) { if got := channelCanSend(c.role, c.o, c.ctype); got != c.want { t.Errorf("%s: channelCanSend = %v, want %v", c.name, got, c.want) } + // B2-5 parity: the affordance is the canonical send predicate. + var bits int64 + if c.role != nil { + bits = c.role.Permissions + } + want := permissions.CanSendMessage(permissions.Subject{ + RolePerms: bits, Override: permOverride(c.o), Channel: permissions.ChannelRef{Type: c.ctype}, + }) == nil + if want != c.want { + t.Errorf("%s: CanSendMessage = %v, channelCanSend table says %v", c.name, want, c.want) + } } } diff --git a/Server/ws/coverage_voice_lifecycle_test.go b/Server/ws/coverage_voice_lifecycle_test.go index 2e166314..c5abd19a 100644 --- a/Server/ws/coverage_voice_lifecycle_test.go +++ b/Server/ws/coverage_voice_lifecycle_test.go @@ -67,16 +67,18 @@ func TestHandleVoiceTokenRefresh_InVoice_ReturnsToken(t *testing.T) { func TestHandleVoiceTokenRefresh_NilUser(t *testing.T) { hub, database := newCoverageHub(t) // The client deliberately carries no *db.User — that is what this test - // covers — but the row must exist so the CONNECT_VOICE re-check can resolve - // a role. Without it the handler stops at FORBIDDEN and never reaches the - // missing-voice-state branch under test. + // covers — but the user row and the channel row must exist so the join + // gate the refresh re-runs (permissions.CanJoinVoice) can resolve a role + // and a channel. Without them the handler stops at FORBIDDEN and never + // reaches the missing-voice-state branch under test. user := seedCoverageOwner(t, database, "vtr-nil-user") + chID := seedVoiceChannel(t, database, "vtr-nil-user-chan") send := make(chan []byte, 16) c := ws.NewTestClient(hub, user.ID, send) hub.Register(c) waitRegistered(t, hub, c) - ws.SetVoiceChIDForTest(c, 42) + ws.SetVoiceChIDForTest(c, chID) hub.HandleMessageForTest(c, voiceTokenRefreshMsg()) diff --git a/Server/ws/deps.go b/Server/ws/deps.go index 3598e2d7..cb6e9cd1 100644 --- a/Server/ws/deps.go +++ b/Server/ws/deps.go @@ -2,6 +2,7 @@ package ws import ( "context" + "errors" "log/slog" "github.com/J3vb/OwnCord/Server/auth" @@ -178,125 +179,82 @@ func hasPerm(ctx context.Context, database *db.DB, perms *permissions.Checker, p return perms.HasChannelPerm(ctx, role.Permissions, role.ID, userID, channelID, perm) } -// hasPermChecked is hasPerm's error-preserving counterpart: it distinguishes -// "the role/override lookup failed" (err != nil, verdict meaningless) from -// "the lookup answered and the bit is absent" (false, nil error). hasPerm and -// hasChannelAccess both collapse that distinction to a fail-closed false, -// which is the correct posture for every gate that sends FORBIDDEN on denial -// (requireChannelAccess, requirePerm, and friends) — do not route those -// through this helper. It exists for a caller like applySetChannelID's -// post-Subscribe revalidation (OC-0266), which documents that a transient -// lookup error must NOT be treated as a denial: unwinding on a DB hiccup -// would silently kill the channel's live message stream with no error frame -// ever sent to the client. -func hasPermChecked(ctx context.Context, database *db.DB, perms *permissions.Checker, permSvc *service.PermissionService, userID, channelID, perm int64) (bool, error) { +// subjectFor resolves userID's role bits and both override layers for +// channelID — from the PermissionService cache when one is wired, else live +// through the Checker — as a permissions.Subject for the value-taking +// predicates (CanSendMessage, CanJoinVoice, ...). Channel flags and DM state +// stay the caller's to fill in. A lookup failure is returned, not collapsed +// (hasPerm and hasChannelAccess collapse it to a fail-closed false, the right +// posture for every gate that sends FORBIDDEN on denial), so an error-aware +// caller like applySetChannelID's post-Subscribe revalidation (OC-0266) can +// tell a transient read failure from a denial; a missing role row is the +// zero Subject, which every predicate refuses. +func subjectFor(ctx context.Context, database *db.DB, perms *permissions.Checker, permSvc *service.PermissionService, userID, channelID int64) (permissions.Subject, error) { if permSvc != nil { - return permSvc.HasChannelPermChecked(ctx, userID, channelID, perm) + return permSvc.Subject(ctx, userID, channelID) } if database == nil || perms == nil { - return false, nil + return permissions.Subject{}, nil } role, err := database.GetRoleForUser(ctx, userID) if err != nil { - return false, err + return permissions.Subject{}, err } if role == nil { - return false, nil + return permissions.Subject{}, nil } - return perms.HasChannelPerm(ctx, role.Permissions, role.ID, userID, channelID, perm), nil + return perms.Subject(ctx, role.Permissions, role.ID, userID, channelID) } -// 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. -// -// With a PermissionService the role-bit gate is answered from its per-user -// cache (the channel-type lookup and the DM membership check stay live — -// dm_participants rows are membership, not permission, state and are never -// cached). Both branches enforce the same rule: role bit required on top, DM -// membership via the single shared IsDMParticipant definition. -func hasChannelAccess(ctx context.Context, database *db.DB, perms *permissions.Checker, permSvc *service.PermissionService, userID, channelID, perm int64) bool { - if database == nil { - return false - } - if permSvc == nil { - return hasChannelAccessLive(ctx, database, perms, userID, channelID, perm) - } - if !permSvc.HasChannelPerm(ctx, userID, 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 - } - // A missing channel row takes the non-DM branch, 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. - if ch == nil || ch.Type != "dm" { - return true - } - // DM: for "dm" the service's RequireChannelAccess is exactly the - // IsDMParticipant membership rule (it waives the role check, which was - // already enforced above). - return permSvc.RequireChannelAccess(ctx, userID, ch.Type, channelID, perm) == nil +// subjectFor is the hub-wired form of the package-level subjectFor. +func (h *Hub) subjectFor(ctx context.Context, userID, channelID int64) (permissions.Subject, error) { + return subjectFor(ctx, h.db, h.permChecker, h.perms, userID, channelID) } -// hasChannelAccessLive is the uncached hasChannelAccess path, kept verbatim for -// hubs and deps constructed without a PermissionService (bare test fixtures). -func hasChannelAccessLive(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, userID, channelID, perm) { - return false - } - ch, err := database.GetChannel(ctx, channelID) +// channelSubject is subjectFor plus the channel's flags and, for a DM, the +// membership and (withBlock) two-party block state — everything CanJoinVoice +// and CanModerateVoice consult. Membership and blocks are always read live +// (dm_participants rows are membership, not permission, state and are never +// cached). An error is a lookup failure, never a denial; callers decide +// whether that fails closed. +func channelSubject(ctx context.Context, database *db.DB, perms *permissions.Checker, permSvc *service.PermissionService, userID int64, ch *db.Channel, withBlock bool) (permissions.Subject, error) { + sub, err := subjectFor(ctx, database, perms, permSvc, userID, ch.ID) 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 + return permissions.Subject{}, err } - // A missing channel row takes the non-DM branch, 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. - if ch == nil || ch.Type != "dm" { - // For every non-DM type, RequireChannelAccess is defined as exactly the - // HasChannelPerm call already made above, so re-invoking it would only - // repeat the same override lookup. The role verdict is the answer. - return true + sub.Channel = channelRef(ch) + if ch.Type != "dm" || database == nil { + return sub, nil + } + ok, err := database.IsDMParticipant(ctx, userID, ch.ID) + if err != nil { + return permissions.Subject{}, err + } + sub.DMParticipant = ok + if ok && withBlock { + switch err := service.RequireDMNotBlocked(ctx, database, userID, ch.ID); { + case errors.Is(err, service.ErrBlocked): + sub.DMBlocked = true + case err != nil: + return permissions.Subject{}, err + } + } + return sub, nil +} + +// joinDenial maps a CanJoinVoice refusal to the error frame the voice_join +// gate has always sent for that reason. +func joinDenial(err error) ClientError { + switch { + case errors.Is(err, permissions.ErrNotVoiceChannel): + return ClientError{Code: ErrCodeBadRequest, Message: "not a voice channel"} + case errors.Is(err, permissions.ErrArchived): + return ClientError{Code: ErrCodeBadRequest, Message: "channel is archived"} + case errors.Is(err, permissions.ErrBlocked): + return ClientError{Code: ErrCodeForbidden, Message: "cannot join voice: blocked"} + default: + return ClientError{Code: ErrCodeForbidden, Message: "missing CONNECT_VOICE permission"} } - // DM: the role bit above stays required on top; the membership rule keeps - // its single shared definition in RequireChannelAccess (IsDMParticipant), - // which waives the role check for DMs. - return perms.RequireChannelAccess(ctx, userID, role.Permissions, role.ID, ch.Type, channelID, perm) == nil } // ── V2 handler type ───────────────────────────────────────────────────────── diff --git a/Server/ws/handlers.go b/Server/ws/handlers.go index 91df62f3..4c613bb3 100644 --- a/Server/ws/handlers.go +++ b/Server/ws/handlers.go @@ -271,33 +271,33 @@ func (h *Hub) applySetChannelID(c *Client, newChID int64) { return } h.pubsub.Subscribe(c, ChannelTopic(newChID)) - // The re-validation mirrors HandleChannelFocus's admission gate: DMs are - // participant-gated (the READ role bit is deliberately waived), non-DMs - // need READ_MESSAGES, and a deleted channel is a denial. A transient - // lookup error is NOT a denial — the recheck exists to catch a concrete + // The re-validation is permissions.CanAdmitSession — the same predicate + // as HandleChannelFocus's admission gate (S-12): DMs are participant-gated + // (the READ role bit is deliberately waived), non-DMs need READ_MESSAGES + // and no archive, and a deleted channel is a denial. A transient lookup + // error is NOT a denial (OC-0266) — the recheck exists to catch a concrete // revoke in the Subscribe race window, the sweeps stay authoritative, and // unwinding on error would turn any DB hiccup into a silently dead - // message stream with no error frame sent to the client. + // message stream with no error frame sent to the client; subjectFor and + // the DM lookup both report a failure instead of collapsing it. ch, chErr := h.db.GetChannel(c.ctx, newChID) if chErr != nil { return } - if ch != nil && ch.Type == "dm" { - if ok, dmErr := h.db.IsDMParticipant(c.ctx, c.userID, newChID); dmErr != nil || ok { + if ch != nil { + sub, subErr := h.subjectFor(c.ctx, c.userID, newChID) + if subErr != nil { return } - } else if ch != nil && !ch.Archived { - // hasPermChecked, not hasChannelAccess: ch is already in hand and known - // non-DM here, so the role/override bit is the whole answer (a second - // hasChannelAccess-internal GetChannel would only repeat the read - // above), and unlike hasChannelAccess it reports a lookup failure - // instead of collapsing it to a denial — required by the "transient - // lookup error is NOT a denial" contract documented above (OC-0266). - allowed, permErr := hasPermChecked(c.ctx, h.db, h.permChecker, h.perms, c.userID, newChID, permissions.ReadMessages) - if permErr != nil { - return + sub.Channel = channelRef(ch) + if ch.Type == "dm" { + ok, dmErr := h.db.IsDMParticipant(c.ctx, c.userID, newChID) + if dmErr != nil { + return + } + sub.DMParticipant = ok } - if allowed { + if permissions.CanAdmitSession(sub) == nil { return } } @@ -333,23 +333,6 @@ func (h *Hub) hasChannelPerm(ctx context.Context, c *Client, channelID int64, pe return h.permChecker.HasChannelPerm(ctx, role.Permissions, role.ID, c.userID, 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, h.perms, c.userID, channelID, perm) { - return true - } - slog.Warn("ws permission denied", "user_id", c.userID, "channel_id", channelID, "perm", permLabel) - c.sendMsg(buildErrorMsg(ErrCodeForbidden, "missing "+permLabel+" permission")) - return false -} - // broadcastExcludeLow sends a message at low priority to all clients in the // sender's channel EXCEPT the sender. Messages sent via this function are NOT // stored in the replay ring buffer — they are ephemeral. This is correct for diff --git a/Server/ws/hub_broadcast.go b/Server/ws/hub_broadcast.go index 1646051e..13e3fac3 100644 --- a/Server/ws/hub_broadcast.go +++ b/Server/ws/hub_broadcast.go @@ -225,6 +225,7 @@ func (h *Hub) channelReadAudienceImpl(ctx context.Context, channelID int64, igno // events to the whole server. Resolve the DM's real audience (its // participants, intersected with who is actually connected) instead, // mirroring the IsDMParticipant membership rule hasChannelAccess uses. + var ref permissions.ChannelRef if h.db != nil { ch, err := h.db.GetChannel(ctx, channelID) if err != nil { @@ -244,43 +245,36 @@ func (h *Hub) channelReadAudienceImpl(ctx context.Context, channelID int64, igno if ch == nil { return []int64{} } - // Archived channels are hidden from every client regardless of - // permissions, mirroring RefreshChannelVisibility and VisibleChannelIDs. - // Without this, an admin edit to an archived channel (or a voice - // teardown inside one) fans out straight to every connected user whose - // base role holds READ_MESSAGES, none of whom have the channel in their - // ready payload or sidebar. ignoreArchived opts a caller out of this - // specific check only — see channelReadAudienceIgnoringArchived. - if ch.Archived && !ignoreArchived { - return []int64{} - } if ch.Type == "dm" { return h.channelReadAudienceDM(ctx, channelID, userIDs) } + ref = channelRef(ch) + // CanViewChannel hides an archived channel from everyone, mirroring + // RefreshChannelVisibility and VisibleChannelIDs: without that, an + // admin edit to an archived channel (or a voice teardown inside one) + // would fan out to every connected user whose base role holds + // READ_MESSAGES, none of whom have the channel in their sidebar. + // ignoreArchived resolves the pre-archival audience instead — see + // channelReadAudienceIgnoringArchived. + if ignoreArchived { + ref.Archived = false + } } - audience := make([]int64, 0, len(userIDs)) - if h.perms != nil { - for _, uid := range userIDs { - if h.perms.HasChannelPerm(ctx, uid, channelID, permissions.ReadMessages) { - audience = append(audience, uid) - } - } - return audience - } - if h.db == nil || h.permChecker == nil { - return audience - } // Resolved per USER, not memoised per role: channel_user_overrides is the // last layer of the resolution order, so two members of the same role can // legitimately disagree about one channel and a per-role memo would hand - // one of them the other's verdict. + // one of them the other's verdict. The verdict is CanViewChannel over + // subjectFor (cached service or live checker); an unresolvable user is + // left out. + audience := make([]int64, 0, len(userIDs)) for _, uid := range userIDs { - role, err := h.db.GetRoleForUser(ctx, uid) - if err != nil || role == nil { + sub, err := h.subjectFor(ctx, uid, channelID) + if err != nil { continue } - if h.permChecker.HasChannelPerm(ctx, role.Permissions, role.ID, uid, channelID, permissions.ReadMessages) { + sub.Channel = ref + if permissions.CanViewChannel(sub) == nil { audience = append(audience, uid) } } @@ -399,56 +393,33 @@ func (h *Hub) RefreshChannelVisibility(ch *db.Channel) { // the targeted re-sync must complete regardless of the triggering request. ctx := context.Background() - // Visibility is resolved per user. With a PermissionService it comes from - // the per-user cache — safe because the admin handlers invalidate - // (InvalidateAll on override change, InvalidateUser on role change) before - // calling into the hub, so the lookups below repopulate from post-change - // data; the 30s TTL is only a backstop and the F6 gen-counter guard keeps - // a racing populate from caching stale rows. Without a service (bare test - // hubs) each client is resolved live. + // Visibility is CanViewChannel — the single predicate shared with + // buildReady / REST ListVisibleChannels — resolved per user from their + // CURRENT role (c.user is a connect-time snapshot). With a + // PermissionService the subject comes from the per-user cache — safe + // because the admin handlers invalidate (InvalidateAll on override + // change, InvalidateUser on role change) before calling into the hub, so + // the lookups below repopulate from post-change data; the 30s TTL is only + // a backstop and the F6 gen-counter guard keeps a racing populate from + // caching stale rows. Without a service (bare test hubs) each client is + // resolved live. Fails closed: an unresolvable role loses visibility + // rather than keeping a stale grant. // // Deliberately NOT memoised per role: channel_user_overrides is the last // layer of the resolution order, so two members of the same role can // legitimately disagree about one channel — exactly the case a per-user // override edit creates, and exactly the fan-out this function targets. - userVisible := func(userID, roleID int64) bool { - role, err := h.db.GetRoleByID(ctx, roleID) - if err != nil || role == nil { - return false - } - // Single visibility predicate shared with buildReady / REST - // ListVisibleChannels; the checker fails closed on a lookup error - // and bypasses for admins, matching the other sites exactly. - return h.permChecker.HasChannelPerm(ctx, role.Permissions, roleID, userID, ch.ID, permissions.ReadMessages) - } - for _, c := range clients { if c.user == nil { continue } - var visible bool - switch { - case ch.Archived: - // Archived channels are hidden from every client regardless of - // permissions, mirroring VisibleChannelIDs. - visible = false - case h.perms != nil: - // The service resolves the user's CURRENT role internally (c.user - // is a connect-time snapshot), failing closed — an unresolvable - // role loses visibility rather than keeping a stale grant. - visible = h.perms.HasChannelPerm(ctx, c.user.ID, ch.ID, permissions.ReadMessages) - default: - // c.user is a connect-time snapshot; an admin may have changed the - // user's role mid-session, so resolve the current role from the DB. - // Fail closed: on error send nothing rather than mis-target. - fresh, err := h.db.GetUserByID(ctx, c.user.ID) - if err != nil || fresh == nil { - slog.Warn("hub: RefreshChannelVisibility could not resolve user role", - "user_id", c.user.ID, "err", err) - continue - } - visible = userVisible(fresh.ID, fresh.RoleID) + sub, err := h.subjectFor(ctx, c.user.ID, ch.ID) + if err != nil { + slog.Warn("hub: RefreshChannelVisibility could not resolve permissions, revoking", + "user_id", c.user.ID, "channel_id", ch.ID, "err", err) } + sub.Channel = channelRef(ch) + visible := err == nil && permissions.CanViewChannel(sub) == nil if refreshChannelVisibilityRaceHook != nil { refreshChannelVisibilityRaceHook(c.user.ID) @@ -475,7 +446,7 @@ func (h *Hub) RefreshChannelVisibility(ch *db.Channel) { // Addressed per client so it can carry this recipient's own // can_send verdict — the whole point of this fan-out is that a // permission change just made those verdicts diverge. - live.sendMsg(buildChannelCreateFor(ch, h.refreshChannelVisibilityCanSend(ctx, ch, c.user.ID, c.user.RoleID))) + live.sendMsg(buildChannelCreateFor(ch, h.refreshChannelVisibilityCanSend(ctx, ch, c.user.ID))) continue } live.sendMsg(buildChannelDelete(ch.ID)) @@ -496,33 +467,22 @@ func (h *Hub) RefreshChannelVisibility(ch *db.Channel) { h.bumpVisibilityWatermark() } -// refreshChannelVisibilityCanSend mirrors channelCanSend (serve_ready.go) — the value the ready -// payload ships per channel — but expressed as per-user permission checks -// so it works in both the service and bare-hub branches without needing a -// resolved *db.Role. HasChannelPerm already bypasses for admins and fails -// closed on a lookup error, matching channelCanSend's own admin shortcut. +// refreshChannelVisibilityCanSend is the can_send verdict the ready payload +// ships per channel (channelCanSend), recomputed for one live user from their +// CURRENT role: permissions.CanSendMessage over the subject subjectFor +// resolves in either the service or the bare-hub branch, failing closed on a +// lookup error (S-12). // // Without this, can_send is only ever computed at connect time, so a role // edit or override edit leaves every connected client's composer stuck on // its stale connect-time verdict until the socket is rebuilt. -func (h *Hub) refreshChannelVisibilityCanSend(ctx context.Context, ch *db.Channel, userID, roleID int64) bool { - has := func(perm int64) bool { - if h.perms != nil { - return h.perms.HasChannelPerm(ctx, userID, ch.ID, perm) - } - role, err := h.db.GetRoleByID(ctx, roleID) - if err != nil || role == nil { - return false - } - return h.permChecker.HasChannelPerm(ctx, role.Permissions, roleID, userID, ch.ID, perm) - } - if !has(permissions.ReadMessages) || !has(permissions.SendMessages) { +func (h *Hub) refreshChannelVisibilityCanSend(ctx context.Context, ch *db.Channel, userID int64) bool { + sub, err := h.subjectFor(ctx, userID, ch.ID) + if err != nil { return false } - if ch.Type == "announcement" { - return has(permissions.ManageMessages) - } - return true + sub.Channel = channelRef(ch) + return permissions.CanSendMessage(sub) == nil } // RefreshAllChannelVisibility re-runs RefreshChannelVisibility for every diff --git a/Server/ws/hub_sweep.go b/Server/ws/hub_sweep.go index 43719234..09d1dfdd 100644 --- a/Server/ws/hub_sweep.go +++ b/Server/ws/hub_sweep.go @@ -182,10 +182,10 @@ func (h *Hub) sweepStaleVoiceEvictRevoked(ctx context.Context) { if chID == 0 { continue } - allowed, err := h.hasChannelPermChecked(ctx, c.userID, chID, permissions.ConnectVoice) + allowed, err := h.voiceStillAllowed(ctx, c.userID, chID) if err != nil { // A transient read failure (I/O error, lock contention, a - // maintenance window) is not a revocation — hasChannelPerm and + // maintenance window) is not a revocation — hasChannelAccess and // permissions.Checker.HasChannelPerm both collapse any DB error // to "denied", which would otherwise evict every in-voice // participant on one bad read. Skip this client this tick; the @@ -317,40 +317,29 @@ func (h *Hub) sweepStaleVoiceStates() { // tests use this hook to reproduce it deterministically. var sweepStaleVoiceJoinRaceHook func(userID, channelID int64, joinedAt string) -// hasChannelPermChecked is hasChannelPerm's error-aware counterpart: it -// distinguishes a genuine permission denial (role missing, or the effective -// permission bits don't include perm) from a DB read failure, by inlining the -// same resolution hasChannelPerm/permissions.Checker.HasChannelPerm perform — -// both of which collapse any error into "denied", indistinguishable from a -// real revocation. sweepStaleVoiceStates needs that distinction: unlike a -// handler answering one client's request, it evicts a live voice session on -// "denied", so a transient read failure must not be treated as a revocation. -func (h *Hub) hasChannelPermChecked(ctx context.Context, userID, channelID int64, perm int64) (allowed bool, err error) { - role, err := h.db.GetRoleForUser(ctx, userID) +// voiceStillAllowed is the sweep's error-aware re-run of the join gate: it +// distinguishes a genuine refusal (permissions.CanJoinVoice over the live +// subject — the bit revoked, the channel archived or deleted, DM membership +// or block state changed) from a DB read failure. sweepStaleVoiceStates +// needs that distinction: unlike a handler answering one client's request, +// it evicts a live voice session on "denied", so a transient read failure +// must not be treated as a revocation. Deliberately read live, never through +// the cached PermissionService: this is the last-line backstop, and staying +// authoritative for a change that somehow bypassed the invalidation hooks is +// worth the handful of reads a minute it costs for the clients in voice. +func (h *Hub) voiceStillAllowed(ctx context.Context, userID, channelID int64) (allowed bool, err error) { + ch, err := h.db.GetChannel(ctx, channelID) if err != nil { return false, err } - if role == nil { - // No role row is a genuine deny, not an error — mirrors - // hasChannelPerm's role == nil case. + if ch == nil { return false, nil } - if permissions.HasAdmin(role.Permissions) { - return true, nil - } - allow, deny, err := h.db.GetChannelPermissions(ctx, channelID, role.ID) + sub, err := channelSubject(ctx, h.db, h.permChecker, nil, userID, ch, true) if err != nil { return false, err } - o := permissions.ChannelOverride{Allow: allow, Deny: deny} - if userID != 0 { - uAllow, uDeny, uErr := h.db.GetUserChannelPermissions(ctx, channelID, userID) - if uErr != nil { - return false, uErr - } - o.UserAllow, o.UserDeny = uAllow, uDeny - } - return permissions.EffectiveChannelPerms(role.Permissions, o)&perm == perm, nil + return permissions.CanJoinVoice(sub) == nil, nil } // cleanupVoiceRaceClearHook, when non-nil, runs immediately before diff --git a/Server/ws/predicate_parity_internal_test.go b/Server/ws/predicate_parity_internal_test.go new file mode 100644 index 00000000..0cf9eeaa --- /dev/null +++ b/Server/ws/predicate_parity_internal_test.go @@ -0,0 +1,419 @@ +package ws + +import ( + "context" + "encoding/json" + "fmt" + "testing" + "time" + + "github.com/J3vb/OwnCord/Server/auth" + "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/permissions" + "github.com/J3vb/OwnCord/Server/service" +) + +// B2-5 parity tables for the ws call sites that decide a security property: +// each site is run against the canonical permissions predicate over the same +// fixture, in both the PermissionService-wired and the bare-hub branch, so +// the two resolution paths and the rule can never disagree (S-12). + +// parityOverrideCases are the override layers every ws parity table walks: +// each one flips a bit the predicates consult. +var parityOverrideCases = []struct { + name string + allow, deny int64 // role layer + uAllow int64 // user layer + uDeny int64 +}{ + {"no override", 0, 0, 0, 0}, + {"role deny SEND", 0, permissions.SendMessages, 0, 0}, + {"role deny READ", 0, permissions.ReadMessages, 0, 0}, + {"role deny CONNECT", 0, permissions.ConnectVoice, 0, 0}, + {"role deny MUTE", 0, permissions.MuteMembers, 0, 0}, + {"role allow MANAGE", permissions.ManageMessages, 0, 0, 0}, + {"user deny SEND", 0, 0, 0, permissions.SendMessages}, + {"user deny READ", 0, 0, 0, permissions.ReadMessages}, + {"user deny MUTE", 0, 0, 0, permissions.MuteMembers}, + {"user allow MANAGE", 0, 0, permissions.ManageMessages, 0}, + {"user allow beats role deny", 0, permissions.SendMessages, permissions.SendMessages, 0}, +} + +// setParityOverrides installs one case's layers for (role, user) on the +// channel and drops the permission cache so the service branch re-reads. +func setParityOverrides(t *testing.T, database *db.DB, permSvc *service.PermissionService, chID, roleID, userID int64, c struct { + name string + allow, deny int64 + uAllow int64 + uDeny int64 +}, +) { + t.Helper() + ctx := context.Background() + if err := database.UpsertChannelOverride(ctx, chID, roleID, c.allow, c.deny); err != nil { + t.Fatalf("%s: UpsertChannelOverride: %v", c.name, err) + } + if err := database.UpsertChannelUserOverride(ctx, chID, userID, c.uAllow, c.uDeny); err != nil { + t.Fatalf("%s: UpsertChannelUserOverride: %v", c.name, err) + } + permSvc.InvalidateAll() +} + +// paritySubject resolves the subject the way the test wants it, straight from +// the Checker, so the site under test is compared against an independent +// resolution rather than its own. +func paritySubject(t *testing.T, database *db.DB, userID int64, ch *db.Channel) permissions.Subject { + t.Helper() + ctx := context.Background() + role, err := database.GetRoleForUser(ctx, userID) + if err != nil || role == nil { + t.Fatalf("GetRoleForUser(%d): %v", userID, err) + } + sub, err := permissions.NewChecker(database).Subject(ctx, role.Permissions, role.ID, userID, ch.ID) + if err != nil { + t.Fatalf("Checker.Subject: %v", err) + } + sub.Channel = channelRef(ch) + return sub +} + +// TestRefreshChannelVisibilityCanSend_Parity: the composer refresh verdict is +// CanSendMessage in both branches, for text and announcement channels. +func TestRefreshChannelVisibilityCanSend_Parity(t *testing.T) { + ctx := context.Background() + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "refresh-parity-user") + textID := mustCreateVoiceChannel(t, database, "refresh-parity-text") + if _, err := database.ExecContext(ctx, `UPDATE channels SET type = 'text' WHERE id = ?`, textID); err != nil { + t.Fatalf("retype: %v", err) + } + newsID := mustCreateVoiceChannel(t, database, "refresh-parity-news") + if _, err := database.ExecContext(ctx, `UPDATE channels SET type = 'announcement' WHERE id = ?`, newsID); err != nil { + t.Fatalf("retype: %v", err) + } + + h := NewHub(database, auth.NewRateLimiter(), nil) + permSvc := service.NewPermissionService(database, h.permChecker) + + for _, chID := range []int64{textID, newsID} { + ch, err := database.GetChannel(ctx, chID) + if err != nil || ch == nil { + t.Fatalf("GetChannel(%d): %v", chID, err) + } + for _, c := range parityOverrideCases { + setParityOverrides(t, database, permSvc, chID, harvestVoiceRoleID, uid, c) + want := permissions.CanSendMessage(paritySubject(t, database, uid, ch)) == nil + + h.perms = nil + if got := h.refreshChannelVisibilityCanSend(ctx, ch, uid); got != want { + t.Errorf("%s/%s bare hub: refreshChannelVisibilityCanSend = %v, CanSendMessage = %v", ch.Type, c.name, got, want) + } + h.perms = permSvc + if got := h.refreshChannelVisibilityCanSend(ctx, ch, uid); got != want { + t.Errorf("%s/%s service: refreshChannelVisibilityCanSend = %v, CanSendMessage = %v", ch.Type, c.name, got, want) + } + } + } +} + +// viewParityFixture is a bare hub with one registered client on a text +// channel plus a second, archived channel, shared by the view-property +// parity tables below. +type viewParityFixture struct { + h *Hub + database *db.DB + permSvc *service.PermissionService + user *db.User + textID int64 + oldID int64 // archived + client *Client + send chan []byte +} + +func newViewParityFixture(t *testing.T) *viewParityFixture { + t.Helper() + ctx := context.Background() + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "view-parity-user") + textID := mustCreateVoiceChannel(t, database, "view-parity-text") + oldID := mustCreateVoiceChannel(t, database, "view-parity-old") + if _, err := database.ExecContext(ctx, `UPDATE channels SET type = 'text' WHERE id IN (?, ?)`, textID, oldID); err != nil { + t.Fatalf("retype: %v", err) + } + if _, err := database.ExecContext(ctx, `UPDATE channels SET archived = 1 WHERE id = ?`, oldID); err != nil { + t.Fatalf("archive: %v", err) + } + h := NewHub(database, auth.NewRateLimiter(), nil) + user, err := database.GetUserByID(ctx, uid) + if err != nil || user == nil { + t.Fatalf("GetUserByID: %v", err) + } + send := make(chan []byte, 64) + c := NewTestClientWithUser(h, user, textID, send) + h.RegisterNowForTest(c) + return &viewParityFixture{ + h: h, database: database, permSvc: service.NewPermissionService(database, h.permChecker), + user: user, textID: textID, oldID: oldID, client: c, send: send, + } +} + +func (f *viewParityFixture) channel(t *testing.T, id int64) *db.Channel { + t.Helper() + ch, err := f.database.GetChannel(context.Background(), id) + if err != nil || ch == nil { + t.Fatalf("GetChannel(%d): %v", id, err) + } + return ch +} + +// eachBranch runs fn once with the bare hub and once with the cached +// PermissionService wired, labelling the branch. +func (f *viewParityFixture) eachBranch(fn func(branch string)) { + f.h.perms = nil + fn("bare") + f.h.perms = f.permSvc + fn("service") +} + +// TestApplySetChannelID_Parity: the post-Subscribe revalidation keeps the +// subscription exactly when CanAdmitSession allows it — for every override +// layer, and for an archived channel. +func TestApplySetChannelID_Parity(t *testing.T) { + f := newViewParityFixture(t) + for _, chID := range []int64{f.textID, f.oldID} { + ch := f.channel(t, chID) + for _, c := range parityOverrideCases { + setParityOverrides(t, f.database, f.permSvc, chID, harvestVoiceRoleID, f.user.ID, c) + want := permissions.CanAdmitSession(paritySubject(t, f.database, f.user.ID, ch)) == nil + f.eachBranch(func(branch string) { + f.h.applySetChannelID(f.client, 0) // a same-channel focus is a no-op; refocus from scratch + f.h.applySetChannelID(f.client, chID) + if got := f.h.SubscribedToChannelTopicForTest(f.client, chID); got != want { + t.Errorf("chan=%d/%s/%s: subscribed = %v, CanAdmitSession = %v", chID, c.name, branch, got, want) + } + }) + } + } +} + +// TestChannelReadAudience_Parity: a connected user is in a channel's read +// audience exactly when CanViewChannel allows it. +func TestChannelReadAudience_Parity(t *testing.T) { + f := newViewParityFixture(t) + ctx := context.Background() + for _, chID := range []int64{f.textID, f.oldID} { + ch := f.channel(t, chID) + for _, c := range parityOverrideCases { + setParityOverrides(t, f.database, f.permSvc, chID, harvestVoiceRoleID, f.user.ID, c) + want := permissions.CanViewChannel(paritySubject(t, f.database, f.user.ID, ch)) == nil + f.eachBranch(func(branch string) { + got := false + for _, uid := range f.h.channelReadAudience(ctx, chID) { + if uid == f.user.ID { + got = true + } + } + if got != want { + t.Errorf("chan=%d/%s/%s: in audience = %v, CanViewChannel = %v", chID, c.name, branch, got, want) + } + }) + } + } +} + +// voiceParityFixture adds to the view fixture a voice channel, an archived +// voice channel, and a DM with a second user (optionally blocking). +type voiceParityFixture struct { + *viewParityFixture + voiceID, oldVoiceID, dmID int64 + other int64 +} + +func newVoiceParityFixture(t *testing.T) *voiceParityFixture { + t.Helper() + f := newViewParityFixture(t) + ctx := context.Background() + voiceID := mustCreateVoiceChannel(t, f.database, "voice-parity") + oldVoiceID := mustCreateVoiceChannel(t, f.database, "voice-parity-old") + if _, err := f.database.ExecContext(ctx, `UPDATE channels SET archived = 1 WHERE id = ?`, oldVoiceID); err != nil { + t.Fatalf("archive: %v", err) + } + other := seedHarvestVoiceUser(t, f.database, "voice-parity-other") + res, err := f.database.ExecContext(ctx, `INSERT INTO channels (name, type, position) VALUES ('dm-parity', 'dm', 0)`) + if err != nil { + t.Fatalf("insert dm: %v", err) + } + dmID, _ := res.LastInsertId() + for _, uid := range []int64{f.user.ID, other} { + if _, err := f.database.ExecContext(ctx, `INSERT INTO dm_participants (channel_id, user_id) VALUES (?, ?)`, dmID, uid); err != nil { + t.Fatalf("insert dm participant: %v", err) + } + } + return &voiceParityFixture{viewParityFixture: f, voiceID: voiceID, oldVoiceID: oldVoiceID, dmID: dmID, other: other} +} + +// setBlocked makes other block the fixture user (or clears the block). +func (f *voiceParityFixture) setBlocked(t *testing.T, blocked bool) { + t.Helper() + ctx := context.Background() + if _, err := f.database.ExecContext(ctx, `DELETE FROM user_blocks WHERE blocker_id = ? AND blocked_id = ?`, f.other, f.user.ID); err != nil { + t.Fatalf("clear block: %v", err) + } + if blocked { + if _, err := f.database.ExecContext(ctx, `INSERT INTO user_blocks (blocker_id, blocked_id) VALUES (?, ?)`, f.other, f.user.ID); err != nil { + t.Fatalf("insert block: %v", err) + } + } +} + +// joinWant is the independent CanJoinVoice verdict for the fixture user on +// ch: Checker-resolved bits plus DM state read straight from the tables. +func (f *voiceParityFixture) joinWant(t *testing.T, ch *db.Channel, blocked bool) error { + t.Helper() + sub := paritySubject(t, f.database, f.user.ID, ch) + if ch.Type == "dm" { + sub.DMParticipant = true + sub.DMBlocked = blocked + } + return permissions.CanJoinVoice(sub) +} + +// TestChannelSubject_Parity: the shared resolver every voice site feeds the +// predicates agrees with an independent resolution in both branches, +// including DM membership and block state. +func TestChannelSubject_Parity(t *testing.T) { + f := newVoiceParityFixture(t) + ctx := context.Background() + for _, chID := range []int64{f.voiceID, f.oldVoiceID, f.dmID} { + ch := f.channel(t, chID) + for _, blocked := range []bool{false, true} { + f.setBlocked(t, blocked) + for _, c := range parityOverrideCases { + setParityOverrides(t, f.database, f.permSvc, chID, harvestVoiceRoleID, f.user.ID, c) + want := paritySubject(t, f.database, f.user.ID, ch) + if ch.Type == "dm" { + want.DMParticipant = true + want.DMBlocked = blocked + } + f.eachBranch(func(branch string) { + got, err := channelSubject(ctx, f.database, f.h.permChecker, f.h.perms, f.user.ID, ch, true) + if err != nil { + t.Fatalf("chan=%d/%s/%s: channelSubject: %v", chID, c.name, branch, err) + } + if got != want { + t.Errorf("chan=%d/blocked=%v/%s/%s: channelSubject = %+v, want %+v", chID, blocked, c.name, branch, got, want) + } + }) + } + } + } +} + +// TestVoiceJoinPrecheck_Parity: the voice_join gate refuses exactly when +// CanJoinVoice refuses, with the frame joinDenial maps that reason to; when +// the predicate allows, the only refusal left is the fixture having no +// LiveKit (VOICE_ERROR), which proves the gate was passed. +func TestVoiceJoinPrecheck_Parity(t *testing.T) { + f := newVoiceParityFixture(t) + f.h.limiter = nil // the table would trip the per-user join limit + ctx := context.Background() + for _, chID := range []int64{f.voiceID, f.oldVoiceID, f.textID, f.dmID} { + ch := f.channel(t, chID) + for _, blocked := range []bool{false, true} { + f.setBlocked(t, blocked) + for _, c := range parityOverrideCases { + setParityOverrides(t, f.database, f.permSvc, chID, harvestVoiceRoleID, f.user.ID, c) + want := f.joinWant(t, ch, blocked) + f.eachBranch(func(branch string) { + for len(f.send) > 0 { + <-f.send + } + _, _, ok := f.h.voiceJoinPrecheck(ctx, f.client, json.RawMessage(fmt.Sprintf(`{"channel_id":%d}`, chID))) + if ok { + t.Fatalf("chan=%d/%s/%s: precheck passed with no LiveKit configured", chID, c.name, branch) + } + var env struct { + Payload struct { + Code string `json:"code"` + } `json:"payload"` + } + select { + case raw := <-f.send: + if err := json.Unmarshal(raw, &env); err != nil { + t.Fatalf("unmarshal: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatalf("chan=%d/%s/%s: no error frame", chID, c.name, branch) + } + wantCode := ErrCodeVoiceError // allowed: refused only by the missing LiveKit + if want != nil { + wantCode = joinDenial(want).Code + } + if env.Payload.Code != wantCode { + t.Errorf("chan=%d/blocked=%v/%s/%s: frame %s, CanJoinVoice says %v (want %s)", chID, blocked, c.name, branch, env.Payload.Code, want, wantCode) + } + }) + } + } + } +} + +// TestVoiceStillAllowed_Parity: the sweep's re-check is CanJoinVoice over the +// live subject, so it evicts exactly what the join gate would now refuse. +func TestVoiceStillAllowed_Parity(t *testing.T) { + f := newVoiceParityFixture(t) + ctx := context.Background() + for _, chID := range []int64{f.voiceID, f.oldVoiceID, f.dmID} { + ch := f.channel(t, chID) + for _, blocked := range []bool{false, true} { + f.setBlocked(t, blocked) + for _, c := range parityOverrideCases { + setParityOverrides(t, f.database, f.permSvc, chID, harvestVoiceRoleID, f.user.ID, c) + want := f.joinWant(t, ch, blocked) == nil + got, err := f.h.voiceStillAllowed(ctx, f.user.ID, chID) + if err != nil { + t.Fatalf("chan=%d/%s: voiceStillAllowed: %v", chID, c.name, err) + } + if got != want { + t.Errorf("chan=%d/blocked=%v/%s: voiceStillAllowed = %v, CanJoinVoice = %v", chID, blocked, c.name, got, want) + } + } + } + } + if got, err := f.h.voiceStillAllowed(ctx, f.user.ID, 999999); err != nil || got { + t.Errorf("deleted channel: allowed=%v err=%v, want a refusal with no error", got, err) + } +} + +// TestRefreshChannelVisibility_Parity: the fan-out sends channel_create +// exactly when CanViewChannel allows and channel_delete otherwise. +func TestRefreshChannelVisibility_Parity(t *testing.T) { + f := newViewParityFixture(t) + for _, chID := range []int64{f.textID, f.oldID} { + ch := f.channel(t, chID) + for _, c := range parityOverrideCases { + setParityOverrides(t, f.database, f.permSvc, chID, harvestVoiceRoleID, f.user.ID, c) + want := MsgTypeChannelDelete + if permissions.CanViewChannel(paritySubject(t, f.database, f.user.ID, ch)) == nil { + want = MsgTypeChannelCreate + } + f.eachBranch(func(branch string) { + f.h.RefreshChannelVisibility(ch) + var env struct { + Type string `json:"type"` + } + select { + case raw := <-f.send: + if err := json.Unmarshal(raw, &env); err != nil { + t.Fatalf("unmarshal: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatalf("chan=%d/%s/%s: no frame from RefreshChannelVisibility", chID, c.name, branch) + } + if env.Type != want { + t.Errorf("chan=%d/%s/%s: got %s, CanViewChannel says %s", chID, c.name, branch, env.Type, want) + } + }) + } + } +} diff --git a/Server/ws/serve_ready.go b/Server/ws/serve_ready.go index 5a676cff..53a56367 100644 --- a/Server/ws/serve_ready.go +++ b/Server/ws/serve_ready.go @@ -125,38 +125,35 @@ func channelRefs(channels []db.Channel) []permissions.ChannelRef { func permOverrides(overrides map[int64]db.ChannelOverride) map[int64]permissions.ChannelOverride { out := make(map[int64]permissions.ChannelOverride, len(overrides)) for id, o := range overrides { - out[id] = permissions.ChannelOverride{ - Allow: o.Allow, - Deny: o.Deny, - UserAllow: o.UserAllow, - UserDeny: o.UserDeny, - } + out[id] = permOverride(o) } return out } // channelCanSend reports whether a user with the given role and per-channel -// override may post in a channel of chanType. It mirrors the non-DM branch of -// MessageService.checkSendPermission so the client can pre-disable the composer -// without a round-trip; the server still enforces the rule authoritatively. +// override may post in a channel of chanType — the ready payload's can_send +// affordance, so the client can pre-disable the composer without a +// round-trip. It is permissions.CanSendMessage, the same predicate the send +// path enforces, so the affordance cannot drift from the rule (S-12). func channelCanSend(role *db.Role, o db.ChannelOverride, chanType string) bool { if role == nil { return false } - if permissions.HasAdmin(role.Permissions) { - return true - } - eff := permissions.EffectiveChannelPerms(role.Permissions, permissions.ChannelOverride{ - Allow: o.Allow, Deny: o.Deny, UserAllow: o.UserAllow, UserDeny: o.UserDeny, - }) - need := permissions.ReadMessages | permissions.SendMessages - if eff&need != need { - return false - } - if chanType == "announcement" { - return eff&permissions.ManageMessages == permissions.ManageMessages - } - return true + return permissions.CanSendMessage(permissions.Subject{ + RolePerms: role.Permissions, + Override: permOverride(o), + Channel: permissions.ChannelRef{Type: chanType}, + }) == nil +} + +// channelRef maps one db channel to the predicates' db-agnostic ChannelRef. +func channelRef(ch *db.Channel) permissions.ChannelRef { + return permissions.ChannelRef{ID: ch.ID, Type: ch.Type, Archived: ch.Archived} +} + +// permOverride maps one db override (both layers) to the checker's type. +func permOverride(o db.ChannelOverride) permissions.ChannelOverride { + return permissions.ChannelOverride{Allow: o.Allow, Deny: o.Deny, UserAllow: o.UserAllow, UserDeny: o.UserDeny} } // readyVisibleChannels resolves the channels the user may see for the ready diff --git a/Server/ws/voice_join.go b/Server/ws/voice_join.go index 262e7873..5f8114f9 100644 --- a/Server/ws/voice_join.go +++ b/Server/ws/voice_join.go @@ -10,7 +10,6 @@ import ( "github.com/J3vb/OwnCord/Server/auth" "github.com/J3vb/OwnCord/Server/db" "github.com/J3vb/OwnCord/Server/permissions" - "github.com/J3vb/OwnCord/Server/service" ) // Voice join/leave rate limits. voice_join and voice_leave each fan out a @@ -99,13 +98,6 @@ func (h *Hub) voiceJoinPrecheck(ctx context.Context, c *Client, payload json.Raw return 0, nil, false } - // 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 0, nil, false - } - // Validate the target channel exists before any state changes (leaving // the current voice channel, persisting join, etc.). ch, err := h.db.GetChannel(ctx, channelID) @@ -114,39 +106,30 @@ func (h *Hub) voiceJoinPrecheck(ctx context.Context, c *Client, payload json.Raw return 0, nil, false } - // channel_id is attacker-controlled and requireChannelAccess above only - // gates CONNECT_VOICE, which says nothing about channel type — a text or - // announcement channel would otherwise accept a join, persist a - // voice_states row, mint a LiveKit room and broadcast voice_state for a - // channel the UI can never render or moderate. 'dm' stays allowed: DM and - // group voice calls join through this same handler. - if ch.Type != "voice" && ch.Type != "dm" { - c.sendMsg(buildErrorMsg(ErrCodeBadRequest, "not a voice channel")) + // channel_id is attacker-controlled, so the gate is + // permissions.CanJoinVoice over the channel-TYPE-aware subject: the + // CONNECT_VOICE bit (a role-only check passes for any DM id — DMs have no + // overrides — and the token minted below carries RoomJoin+CanSubscribe + // for that room), a channel that has a room (a text channel would + // otherwise persist a voice_states row and mint a LiveKit room the UI can + // never render or moderate; DM and group calls join through this same + // handler), no archive (a caller still holding the id of a channel nobody + // can see must not join its room; the archive transition also evicts + // whoever is inside), and for a DM membership plus no block (blocking + // never touches dm_participants, so membership alone would let a blocked + // user into the blocker's call — same rule as every other DM sink, + // service.requireDMNotBlocked, group DMs exempt). The same predicate + // gates the token refresh and a moderator move's destination. + sub, subErr := channelSubject(ctx, h.db, h.permChecker, h.perms, c.userID, ch, true) + if subErr != nil { + slog.Error("ws voice_join: permission lookup failed, denying", "user_id", c.userID, "channel_id", channelID, "err", subErr) + c.sendMsg(buildErrorMsg(ErrCodeInternal, "permission check failed")) return 0, nil, false } - - // A blocked user is still a DM participant — blocking never touches - // dm_participants (service/block.go), so the CONNECT_VOICE + IsDMParticipant - // gate above passes them straight through into the blocker's DM voice room. - // Every other 1:1-DM interaction sink (send, edit, react, pin, typing, - // call_ring) already routes through this same check - // (service.requireDMNotBlocked); voice was the one gap. Group DMs are - // exempt inside it, matching every other sink. h.db satisfies - // service.Store directly, so no MessageService wiring is needed here. - if ch.Type == "dm" { - if err := service.RequireDMNotBlocked(ctx, h.db, c.userID, channelID); err != nil { - c.sendMsg(buildErrorMsg(ErrCodeForbidden, "cannot join voice: blocked")) - return 0, nil, false - } - } - - // Archived channels are hidden from every client and their voice states are - // dropped from `ready`, but `archived` was consulted only by the visibility - // predicate — so a caller still holding the id could join the room of a - // channel nobody can see or moderate. Refuse the join outright; the sibling - // archive transition also evicts whoever is already inside. - if ch.Archived { - c.sendMsg(buildErrorMsg(ErrCodeBadRequest, "channel is archived")) + if joinErr := permissions.CanJoinVoice(sub); joinErr != nil { + slog.Warn("ws voice_join refused", "user_id", c.userID, "channel_id", channelID, "reason", joinErr) + refusal := joinDenial(joinErr) + c.sendMsg(buildErrorMsg(refusal.Code, refusal.Message)) return 0, nil, false } @@ -581,35 +564,33 @@ func handleVoiceTokenRefreshV2(ctx context.Context, cmd Command, info ClientInfo return Result{Error: ClientError{Code: ErrCodeInternal, Message: "voice not configured"}} } - // Re-check CONNECT_VOICE where the credential is minted. The channel comes - // from the client's own session state, and voice_join (voice_join.go:61) was - // the only place this bit was ever checked — so a user whose CONNECT_VOICE - // was revoked mid-session kept minting fresh SFU room-join grants. Refusing + // Re-run the join gate (permissions.CanJoinVoice, exactly as voice_join + // applies it) where the credential is minted. The channel comes from the + // client's own session state, and voice_join used to be the only place + // the bit was checked — so a user whose CONNECT_VOICE was revoked + // mid-session kept minting fresh SFU room-join grants, and a block imposed + // mid-session (OC-0018) kept re-issuing one for the blocker's DM. Refusing // 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. - // 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, d.PermSvc, userID, channelID, permissions.ConnectVoice) { + // deletes the voice_states row and removes the LiveKit participant. Fails + // closed: a deleted channel or a lookup failure is a refusal too. + ch, chErr := d.DB.GetChannel(ctx, channelID) + if chErr != nil || ch == nil { return Result{ Error: ClientError{Code: ErrCodeForbidden, Message: "missing CONNECT_VOICE permission"}, LeaveVoice: true, } } - - // Same block gate as voice_join (voice_join.go, OC-0018): a block imposed - // mid-session must not let the refresh keep minting a fresh SFU credential - // for a DM the other participant has since blocked. RequireDMNotBlocked is - // a safe no-op for a non-DM channelID (no dm_participants row to match), so - // this needs no channel-type fetch of its own. d.DB satisfies service.Store - // directly. - if err := service.RequireDMNotBlocked(ctx, d.DB, userID, channelID); err != nil { + sub, subErr := channelSubject(ctx, d.DB, d.Permissions, d.PermSvc, userID, ch, true) + if subErr != nil { return Result{ - Error: ClientError{Code: ErrCodeForbidden, Message: "cannot refresh voice token: blocked"}, + Error: ClientError{Code: ErrCodeForbidden, Message: "missing CONNECT_VOICE permission"}, LeaveVoice: true, } } + if joinErr := permissions.CanJoinVoice(sub); joinErr != nil { + return Result{Error: joinDenial(joinErr), LeaveVoice: true} + } // With a PermissionService these three are cache hits after the gate above // populated the user's entry — the refresh drops from ~9 DB reads to at diff --git a/Server/ws/voice_moderation.go b/Server/ws/voice_moderation.go index a145108a..312ce55c 100644 --- a/Server/ws/voice_moderation.go +++ b/Server/ws/voice_moderation.go @@ -2,6 +2,7 @@ package ws import ( "context" + "errors" "fmt" "log/slog" @@ -85,26 +86,36 @@ func voiceModTarget(ctx context.Context, d VoiceDeps, actorID, targetID int64) ( return nil, &Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "user is not in a voice channel"}} } - // MUTE_MEMBERS authorizes moderating server voice channels, not a private - // DM call the actor happens not to be part of — voice_mod_kick and friends - // carry no channel id from the client, so without this a moderator could - // reach into any two users' DM call by targeting a user id alone. Refused - // with the exact same shape as "target not in voice" so the actor learns - // nothing about a DM call they are not in. + // The decision is permissions.CanModerateVoice over the actor's subject in + // the TARGET's channel: effective MUTE_MEMBERS there, so a role-layer or + // user-layer deny on that channel holds (SEC-02), READ_MESSAGES so a room + // hidden from the actor cannot be moderated, and for a DM call the actor's + // own membership — voice_mod_kick and friends carry no channel id from + // the client, so without that a moderator could reach into any two users' + // DM call by targeting a user id alone. The DM refusal keeps the exact + // shape of "target not in voice" so the actor learns nothing about a call + // they are not in. The base-bit check above is only an early rejection + // (it never admits): it keeps FORBIDDEN ahead of the voice-state lookup + // for actors with no MUTE_MEMBERS at all, which also means a channel + // allow cannot grant the bit to a role whose base lacks it. ch, err := d.DB.GetChannel(ctx, state.ChannelID) if err != nil { slog.Error("ws voiceModTarget GetChannel", "err", err, "channel_id", state.ChannelID) return nil, &Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to read channel"}} } - if ch != nil && ch.Type == "dm" { - participant, err := d.DB.IsDMParticipant(ctx, actorID, state.ChannelID) - if err != nil { - slog.Error("ws voiceModTarget IsDMParticipant", "err", err, "channel_id", state.ChannelID) - return nil, &Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to verify DM membership"}} - } - if !participant { - return nil, &Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "user is not in a voice channel"}} - } + if ch == nil { + return nil, &Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "user is not in a voice channel"}} + } + sub, subErr := channelSubject(ctx, d.DB, d.Permissions, d.PermSvc, actorID, ch, false) + if subErr != nil { + slog.Error("ws voiceModTarget channelSubject", "err", subErr, "channel_id", state.ChannelID) + return nil, &Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to verify channel access"}} + } + switch modErr := permissions.CanModerateVoice(sub); { + case errors.Is(modErr, permissions.ErrNotDMParticipant): + return nil, &Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "user is not in a voice channel"}} + case modErr != nil: + return nil, &Result{Error: ClientError{Code: ErrCodeForbidden, Message: "missing MUTE_MEMBERS permission"}} } return state, nil @@ -416,16 +427,20 @@ func handleVoiceModMoveV2(ctx context.Context, cmd Command, info ClientInfo, dep if dest.Type != "voice" { return Result{Error: ClientError{Code: ErrCodeBadRequest, Message: "destination is not a voice channel"}} } - // The re-join this move hands off to (handleVoiceJoin) refuses an - // archived channel outright; check it here too, or the pre-flight commits - // the destructive half of the move for a re-join guaranteed to bounce. - if dest.Archived { - return Result{Error: ClientError{Code: ErrCodeBadRequest, Message: "channel is archived"}} + // The destination is gated on the TARGET's access with the same predicate + // the re-join this move hands off to (handleVoiceJoin) will apply — + // permissions.CanJoinVoice — so a move can neither place someone in a + // channel they could not join themselves nor commit the destructive half + // of the move for a re-join guaranteed to bounce (an archived channel). + targetSub, subErr := channelSubject(ctx, d.DB, d.Permissions, d.PermSvc, c.TargetID(), dest, false) + if subErr != nil { + slog.Error("ws handleVoiceModMoveV2 channelSubject", "err", subErr, "channel_id", c.ToChannelID()) + return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to check destination access"}} } - // The destination is gated on the TARGET's access, not the moderator's: - // a move must not become a way to place someone in a channel they could - // not join themselves. - if !hasChannelAccess(ctx, d.DB, d.Permissions, d.PermSvc, c.TargetID(), c.ToChannelID(), permissions.ConnectVoice) { + switch joinErr := permissions.CanJoinVoice(targetSub); { + case errors.Is(joinErr, permissions.ErrArchived): + return Result{Error: ClientError{Code: ErrCodeBadRequest, Message: "channel is archived"}} + case joinErr != nil: return Result{Error: ClientError{ Code: ErrCodeForbidden, Message: "user cannot connect to that voice channel", diff --git a/Server/ws/voice_moderation_deafen_race_test.go b/Server/ws/voice_moderation_deafen_race_test.go index d4d1a09b..a54d5c83 100644 --- a/Server/ws/voice_moderation_deafen_race_test.go +++ b/Server/ws/voice_moderation_deafen_race_test.go @@ -23,6 +23,7 @@ import ( "testing" "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/permissions" ) // deafenRaceRoleAdmin / deafenRaceRoleMember reuse the default seeded roles @@ -98,7 +99,7 @@ func TestVoiceModDeafen_RollbackFollowsTargetChannelMove(t *testing.T) { cmd := VoiceModDeafenCmd{userID: actorID, channelID: chanA, targetID: targetID, deafened: true} info := ClientInfo{UserID: actorID} - deps := VoiceDeps{DB: database} + deps := VoiceDeps{DB: database, Permissions: permissions.NewChecker(database)} result := handleVoiceModDeafenV2(ctx, cmd, info, deps) @@ -181,7 +182,7 @@ func TestVoiceModDeafen_UndeafenRollbackDoesNotApplyOnUnauthorizedChannel(t *tes // test above. cmd := VoiceModDeafenCmd{userID: actorID, channelID: chanA, targetID: targetID, deafened: false} info := ClientInfo{UserID: actorID} - deps := VoiceDeps{DB: database} + deps := VoiceDeps{DB: database, Permissions: permissions.NewChecker(database)} result := handleVoiceModDeafenV2(ctx, cmd, info, deps) diff --git a/Server/ws/voice_moderation_overrides_test.go b/Server/ws/voice_moderation_overrides_test.go new file mode 100644 index 00000000..4ea8c3ab --- /dev/null +++ b/Server/ws/voice_moderation_overrides_test.go @@ -0,0 +1,72 @@ +package ws_test + +import ( + "context" + "testing" + + "github.com/J3vb/OwnCord/Server/permissions" + "github.com/J3vb/OwnCord/Server/ws" +) + +// TestVoiceMod_ChannelOverridesApply locks SEC-02's server half: the actor's +// authority is their EFFECTIVE permission in the target's channel +// (permissions.CanModerateVoice), so a role-layer or user-layer deny of +// MUTE_MEMBERS on that channel refuses the action even though the base role +// holds the bit, and a channel the actor cannot see (READ_MESSAGES denied) +// cannot be moderated either. Administrator keeps its bypass. +func TestVoiceMod_ChannelOverridesApply(t *testing.T) { + cases := []struct { + name string + actorRole int // 2 Admin: MUTE_MEMBERS without ADMINISTRATOR; 1 Owner: ADMINISTRATOR + roleDeny int64 + userDeny int64 + wantCode string // "" = allowed (target ends up server-muted) + }{ + {"no override: allowed", 2, 0, 0, ""}, + {"role deny MUTE_MEMBERS in this channel", 2, permissions.MuteMembers, 0, "FORBIDDEN"}, + {"user deny MUTE_MEMBERS in this channel", 2, 0, permissions.MuteMembers, "FORBIDDEN"}, + {"role deny READ_MESSAGES: hidden channel", 2, permissions.ReadMessages, 0, "FORBIDDEN"}, + {"administrator bypasses the deny", 1, permissions.MuteMembers, permissions.MuteMembers, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hub, database := newVoiceModHub(t) + chanID := seedVoiceChan(t, database, "vc-override") + actor := seedVoiceUserWithRole(t, database, "mod-override", tc.actorRole) + target := seedVoiceUserWithRole(t, database, "target-override", 4) // Member + ctx := context.Background() + if err := database.UpsertChannelOverride(ctx, chanID, int64(tc.actorRole), 0, tc.roleDeny); err != nil { + t.Fatalf("UpsertChannelOverride: %v", err) + } + if err := database.UpsertChannelUserOverride(ctx, chanID, actor.ID, 0, tc.userDeny); err != nil { + t.Fatalf("UpsertChannelUserOverride: %v", err) + } + + joinVoice(t, hub, target, chanID) + + send := make(chan []byte, 16) + c := ws.NewTestClientWithUser(hub, actor, chanID, send) + hub.Register(c) + waitRegistered(t, hub, c) + + hub.HandleMessageForTest(c, voiceModMuteMsg(chanID, target.ID, true)) + + state, err := database.GetVoiceState(ctx, target.ID) + if err != nil || state == nil { + t.Fatalf("GetVoiceState: state=%v err=%v", state, err) + } + if tc.wantCode == "" { + if !state.ServerMuted { + t.Fatal("expected the target to be server muted") + } + return + } + if code := receiveErrorCode(send, waitTimeout); code != tc.wantCode { + t.Fatalf("error code = %q, want %s", code, tc.wantCode) + } + if state.ServerMuted { + t.Fatal("target must not be server muted after a refused action") + } + }) + } +} diff --git a/docs/plans/b2-protocol-trust-compat-2026-08-28.md b/docs/plans/b2-protocol-trust-compat-2026-08-28.md index 60eca8df..36ae88cd 100644 --- a/docs/plans/b2-protocol-trust-compat-2026-08-28.md +++ b/docs/plans/b2-protocol-trust-compat-2026-08-28.md @@ -5,7 +5,7 @@ `v1.2.0-alpha.4` — claims verified at `64d2e108`; the branch was rebased onto `dd7ed091` (#1432) before merge **Status:** in progress — entry gate 1 of 3 met at draft time (see below); B2-0, -B2-1 and B2-8 landed 2026-08-28, B2-2 (with B2-3 and B2-4 folded in) on 2026-08-29 (evidence in their sections); B2-5 is next. +B2-1 and B2-8 landed 2026-08-28, B2-2 (with B2-3 and B2-4 folded in) and B2-5 on 2026-08-29 (evidence in their sections); B2-6 and B2-7 are next. Update this line, not only the step table, when a step lands. Primary inputs: @@ -41,7 +41,7 @@ one to one and a half weeks with agents working steps in parallel. | **B2-2** | Protocol epoch and negotiation — **DONE 2026-08-29 (slim; absorbs B2-3, B2-4)** | 1 day | serialized, after B2-8 | | **B2-3** | Server-first updates through the signed manifest — folded into B2-2 | ½ day | after B2-2 | | **B2-4** | Compatibility matrix — folded into B2-2 | ½ day | after B2-2 | -| **B2-5** | One permission predicate per security property | 1–2 days | serialized | +| **B2-5** | One permission predicate per security property — **DONE 2026-08-29 (PR #1440)** | 1–2 days | serialized | | **B2-6** | Safe audit coverage | ½ day | B2-1, B2-7 | | **B2-7** | Trust model, absence proofs, plugin boundary | 1 day | B2-1, B2-6 | | **B2-8** | The nine B2-tagged findings | 1 day | before B2-2 | @@ -339,6 +339,100 @@ half stands on those two tests. one. If residual calls remain with a reason, record them in HP-2 and leave the rule to B3 (roadmap B3 item 15). +**Evidence, 2026-08-29** — branch `feat/b2-5-permission-predicates` from +`dev` `9c9b8be6`; PR #1440 to `dev`. HP-2 question 5 cites this block. + +- Pre-squash SHAs, one commit per property: `00761523` (predicates + + `Checker` delegation), `94aba833` (send — S-01), `0271cbbe` (view / + session admission — S-12), `802101a0` (voice join), `aeee37e8` (voice + moderation — SEC-02 server half). +- Predicates (`Server/permissions/predicates.go`), each pure over a + `Subject` (role bits, both override layers, channel flags, DM membership + and block state): `CanViewChannel`, `CanAdmitSession` (= view), + `CanSendMessage`, `CanType` (= send), `CanJoinVoice`, `CanModerateVoice`; + `Subject.Has` is the one value-taking bit predicate `Checker` and + `PermissionService` route through. Refusals are sentinels + (`ErrPermissionDenied` + bit name, `ErrArchived`, `ErrBlocked`, + `ErrNotDMParticipant`, `ErrNotVoiceChannel`) so each site keeps its own + status codes. Permission is checked before the archive flag everywhere, so + an unauthorized caller learns nothing from the error. +- Parity tables (site vs predicate over the same fixture, both the + cached-service and bare-hub branch, every override layer): + `Server/service/predicate_parity_test.go` (`CanPost`, `HandleTyping`, + `HandleChannelFocus`) and `Server/ws/predicate_parity_internal_test.go` + (`channelCanSend`, `refreshChannelVisibilityCanSend`, `applySetChannelID`, + `channelReadAudience`, `RefreshChannelVisibility`, `channelSubject`, + `voiceJoinPrecheck`, `voiceStillAllowed`). Red before delegation: S-01 (19 + typing rows) and SEC-02 (`TestVoiceMod_ChannelOverridesApply`, three deny + rows); every other site already agreed with its predicate. +- Decision recorded for SEC-02's open question ("READ_MESSAGES or + CONNECT_VOICE?"): `CanModerateVoice` requires effective `READ_MESSAGES` + + `MUTE_MEMBERS` in the target's channel — a moderator acts only where they + can see. The base-bit `HasServerPerm` check stays as an early rejection + (never admits), which keeps FORBIDDEN ahead of the voice-state lookup and + means a channel allow cannot grant `MUTE_MEMBERS` to a base role lacking it. +- Inventory, step 1 grep plus the hand-rolled sites, before → after: + + | Site (before) | Property | After | + | ------------------------------------------------------------------- | --------------- | --------------------------------------------- | + | `permissions/checker.go` HasChannelPerm / Batch / VisibleChannelIDs | view (bit) | `Subject.Has` / `CanViewChannel` | + | `service/message_perms.go:93-100` checkSendPermission | send | `CanSendMessage` | + | `service/channel.go:132` HandleTyping (READ only — S-01) | type | `CanType` | + | `service/channel.go:256` HandleChannelFocus | admit | `CanAdmitSession` | + | `ws/serve_ready.go:149-157` channelCanSend | send | `CanSendMessage` | + | `ws/hub_broadcast.go:519-523` refreshChannelVisibilityCanSend | send | `CanSendMessage` | + | `ws/hub_broadcast.go:265,283` channelReadAudience | view | `CanViewChannel` | + | `ws/hub_broadcast.go:422-439` RefreshChannelVisibility | view | `CanViewChannel` | + | `ws/handlers.go:296` applySetChannelID (hasPermChecked) | admit | `CanAdmitSession`; helper deleted | + | `ws/voice_join.go:105-151` voiceJoinPrecheck (requireChannelAccess) | join | `CanJoinVoice`; helper deleted | + | `ws/voice_join.go:594-612` handleVoiceTokenRefreshV2 | join | `CanJoinVoice` | + | `ws/voice_moderation.go:416-433` move destination | join | `CanJoinVoice` | + | `ws/hub_sweep.go:353` hasChannelPermChecked (EffectiveChannelPerms) | join (bit only) | `CanJoinVoice` (whole rule, error-aware) | + | `ws/voice_moderation.go:64` voiceModTarget (HasServerPerm — SEC-02) | moderate | `CanModerateVoice` + base-bit early rejection | + | `ws/deps.go` hasChannelAccess / hasChannelAccessLive | join/admit glue | deleted (`channelSubject` + predicates) | + +- Residue after migration (direct bit-helper calls outside + `Server/permissions`, non-test), each with its reason — so step 5's + condition is not met and the `authz-chokepoint` rule stays with B3 item + 15, consistent with the 2026-08-18 measurement that dropped it (1 hit in + `api/`, a false positive; 30 widened, 87% legitimate): + - Server-scoped permissions with no channel — `HasServerPerm` in + `api/middleware.go:200`, `admin/middleware.go:109`, `service/emoji.go:95`, + `service/moderation.go:51`, `service/role.go:82`, and `HasAnyPerm` + (`AdminPerimeter`) in `admin/middleware.go:84`. These ARE the canonical + server-wide predicate; there is no channel to resolve a `Subject` for. + - `HasAdmin` as a fetch short-circuit (skip the override query for admins) + in `service/channel.go:59`, `service/message_perms.go:25`, + `service/permission.go:224`, `ws/serve.go:780`, `ws/serve_ready.go:169`, + `ws/voice_join.go:355`; as an authorization input in + `admin/handlers_channel_perms.go:95,325`, `admin/logstream.go:452`, + `api/upload_handler.go:404`, `service/role.go:104` (role hierarchy — the + measurement's "no `Outranks`" class). + - `& permissions.AllPerms` masks on admin input (`admin/handlers_channel_perms.go:131-358`, + `service/role.go:210,307`) — sanitisation, not a decision. + - `service/mentions.go:262-266,302-304` — the bulk @everyone reader walk + resolves the role layer per role and the user layer as a set difference; + the owner declined the mechanical `HasPerm` conversion on 2026-08-18 + (memory `owncord-invariant-rule-measurement-2026-08-18`). + - `ws/voice_moderation.go:65` — the base-bit early rejection described + above. +- Behaviour deltas beyond the three findings, all narrowing: the stale-voice + sweep re-runs the whole join rule (deleted/archived channel, lost DM + membership, new block evict too); the token refresh refuses a deleted + channel; the bare-hub `RefreshChannelVisibility` branch fails closed on a + lookup error like the service branch always did. Two fixtures needed + completing, assertions untouched: the deafen-race `VoiceDeps` gain a + `Checker`, and `TestHandleVoiceTokenRefresh_NilUser` seeds the channel it + refreshes. +- Gates at `aeee37e8`, from `Server/`: four build-tag variants, `go vet`, + `go test -race ./...`, `go test -tags deadlock ./ws/`, `golangci-lint run` + — all exit 0, run before each of the five commits. +- Codex review on #1440 (P2): `CanJoinVoice`'s DM branch returned before the + archive flag, while the old `voiceJoinPrecheck` refused every archived + channel and the admin PATCH accepts `archived` for a DM. Fixed in + `fdd2a3ff` (archive checked after membership and block for both kinds, + pinned in the predicate table), same gate green; thread resolved. + ## B2-6 — Safe audit coverage 1. Enumerate the security-sensitive mutations: credential and TOTP changes, @@ -465,15 +559,15 @@ The seven local reports in `docs/security-findings/` (gitignored, never committed; the directory-to-row mapping lives in its local README) and where each goes: -| Public row | Owner phase | Acceptance test lives | Lands with | -| ---------- | ------------------------------- | ---------------------------------------------------------------------------------------- | --------------------------------- | -| S-01 | **B2** | beside the report until B2-5 merges | B2-5 | -| SEC-02 | **B2** (server half) | beside the report until B2-5 merges | B2-5; UI half in B5 | -| C-09 | **B2** (contract) / B7 (client) | beside the report | contract in B2-7 docs; code in B7 | -| SEC-03 | B2 if small, else **B5** | beside the report | B2-9 or B5 item 11 | -| SEC-01 | **B4** | private GitHub advisory (owner creates it) | B4 | -| SEC-04 | **B3/B6** | private GitHub advisory (owner creates it) | B6 | -| OC-0324 | **B4** | beside the report; no advisory — the tracked ledger already carries this finding in full | B4 | +| Public row | Owner phase | Acceptance test lives | Lands with | +| ---------- | ------------------------------- | ---------------------------------------------------------------------------------------- | ------------------------------------- | +| S-01 | **B2** | landed with B2-5 (`Server/service/predicate_parity_test.go`) | B2-5 (PR #1440) — done | +| SEC-02 | **B2** (server half) | landed with B2-5 (`Server/ws/voice_moderation_overrides_test.go`) | B2-5 (PR #1440) — done; UI half in B5 | +| C-09 | **B2** (contract) / B7 (client) | beside the report | contract in B2-7 docs; code in B7 | +| SEC-03 | B2 if small, else **B5** | beside the report | B2-9 or B5 item 11 | +| SEC-01 | **B4** | private GitHub advisory (owner creates it) | B4 | +| SEC-04 | **B3/B6** | private GitHub advisory (owner creates it) | B6 | +| OC-0324 | **B4** | beside the report; no advisory — the tracked ledger already carries this finding in full | B4 | An acceptance test demonstrates the defect, so it is exploit detail: it stays local until its fix lands, then lands publicly in the same PR. The two diff --git a/docs/plans/repo-health-issue-register-2026-08-23.md b/docs/plans/repo-health-issue-register-2026-08-23.md index acb9a90d..040cb2ce 100644 --- a/docs/plans/repo-health-issue-register-2026-08-23.md +++ b/docs/plans/repo-health-issue-register-2026-08-23.md @@ -149,12 +149,12 @@ This register carries only non-sensitive security properties and opaque remediation families; an apparently related engineering row is not evidence that any private report is fixed. -| ID | Pri | State | Opaque remediation family | Phase | Public closure evidence | -| ------ | --: | --------- | --------------------------------------------------------- | ----- | ------------------------------------------------------------------------------------------------------------------------------------------------- | -| SEC-01 | P1 | confirmed | Atomic concurrent password-confirmation admission. | B4 | One server-owned admission decision, bounded concurrent attempts, and race/load regression coverage. | -| SEC-02 | P1 | confirmed | Effective channel-level voice moderation permissions. | B5 | Voice moderation delegates to the same effective-permission policy as the authoritative channel action, with override and denial tests. | -| SEC-03 | P1 | confirmed | Bounded per-response and aggregate preview/media reads. | B2/B5 | Streaming limits are enforced before buffering; aggregate memory/concurrency budgets, timeout, cancellation, and adversarial boundary tests pass. | -| SEC-04 | P1 | confirmed | Durable per-user/server storage quotas and disk headroom. | B3/B6 | Transaction-safe quotas cover files and cumulative storage; low-disk behavior fails safely and is exercised by restart/concurrency tests. | +| ID | Pri | State | Opaque remediation family | Phase | Public closure evidence | +| ------ | --: | ------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----- | ------------------------------------------------------------------------------------------------------------------------------------------------- | +| SEC-01 | P1 | confirmed | Atomic concurrent password-confirmation admission. | B4 | One server-owned admission decision, bounded concurrent attempts, and race/load regression coverage. | +| SEC-02 | P1 | resolved/superseded | Effective channel-level voice moderation permissions. Server half landed in B2-5 (PR #1440): voice moderation decides on the effective permission in the target channel, with override and denial tests. | B5 | UI half only: the moderation controls surface effective permissions (B5 item 11). | +| SEC-03 | P1 | confirmed | Bounded per-response and aggregate preview/media reads. | B2/B5 | Streaming limits are enforced before buffering; aggregate memory/concurrency budgets, timeout, cancellation, and adversarial boundary tests pass. | +| SEC-04 | P1 | confirmed | Durable per-user/server storage quotas and disk headroom. | B3/B6 | Transaction-safe quotas cover files and cumulative storage; low-disk behavior fails safely and is exercised by restart/concurrency tests. | ## Client engineering issues @@ -184,25 +184,25 @@ not recounted here. ## Server engineering issues -| ID | Pri | State | Issue and evidence | Phase | Closure evidence | -| ---- | --: | --------- | ----------------------------------------------------------------------------------------------------------------------------------- | ------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| S-01 | P1 | confirmed | Typing currently checks a weaker permission than posting. | B2/B3 | Typing delegates to the same send-policy predicate; denial, announcement, and override tests prevent drift. | -| S-02 | P1 | confirmed | Invite create/revoke are privileged mutations without the audit coverage used by sibling mutation families. | B4/B5 | Successful create/revoke produce safe, non-secret audit events; failure behavior is tested. | -| S-03 | P2 | confirmed | Admin channel name/topic/category validation lacks one explicit rune/normalization contract. | B3/B5 | Shared limits cover admin and user writers; boundary tests count runes, not bytes. | -| S-04 | P2 | confirmed | Sibling admin channel lookups expose inconsistent DM/not-found response contracts. | B3 | One non-DM resolution policy and response contract covers both paths. | -| S-05 | P2 | confirmed | Repository-wide Go formatting is not a required gate. | B1 | Tree is formatted and a fast required gate fails future drift. | -| S-06 | P2 | confirmed | Server coverage is uploaded without a global or core-package regression floor; current aggregate is 74.6%. | B3/B10 | Documented baseline/exclusions and ratcheted global/core thresholds. | -| S-07 | P2 | confirmed | Thousands of tests and 17 fuzz targets exist, but there are no Go benchmarks for hub/replay, permission, DB, or fan-out hot paths. | B6/B10 | Stable microbenchmarks and reference load baselines cover the highest-risk paths. | -| S-08 | P2 | confirmed | Large lifecycle/hub/serve files remain structural hotspots. | B3 | Cohesive extractions preserve lifecycle, locking, race, and deadlock invariants. | -| S-09 | P2 | confirmed | API/admin/WebSocket layers still contain many direct database call sites. | B3 | Each use moves behind a narrow service/store seam or is documented as an intentional transaction/composition boundary. | -| S-10 | P2 | confirmed | Auth routes still consume raw database ownership and are the first intended S-09 migration slice. | B3/B4 | Tested AuthService/narrow interfaces preserve enumeration and sentinel-error behavior. | -| S-11 | P2 | confirmed | Hub construction uses post-construction collaborator setters, leaving required wiring temporally coupled to `Run`. | B3 | Required collaborators are validated constructor/options inputs; only genuinely dynamic dependencies remain mutable. | -| S-12 | P2 | confirmed | Ready/refresh/WebSocket paths mirror message send-permission policy by hand. | B3 | All paths delegate to one value-taking predicate with parity tests. | -| S-13 | P2 | confirmed | Durable TOTP used-code and partial-auth persister work remains incomplete. | B4 | Hash-only persistence, expiry, restart, and failure-mode tests land without persisting sliding rate-limit windows. | -| S-14 | P1 | confirmed | Load tooling exists, but no supported capacity result is published for the approved 250 users / 100 connections / 25 voice profile. | B6/B10 | Reproducible report states hardware/software, CPU, memory, DB waits, p95/p99 latency, and pass/fail thresholds. | -| S-15 | P3 | verify | `voice_speakers` and `member_leave` remain reserved protocol entries with no production emit site. | B2 | Compatibility review removes unused entries before the epoch freeze or explicitly reserves and fixtures them; schema, generated types, docs, and tests agree. | -| S-16 | P3 | verify | Voice key-holder TOCTOU hardening remains a documented follow-up without a demonstrated contract failure. | B2/B3 | Threat-model review either records why outer checks suffice or adds an in-function recheck and race-focused private test. | -| S-17 | P3 | watch | Vulnerability tooling found no reachable Go advisory, while non-called/unmaintained upstream paths remain. | B6/B10 | Dependency path is monitored, compatible fixes are applied, and reachable-symbol scanning remains required. | +| ID | Pri | State | Issue and evidence | Phase | Closure evidence | +| ---- | --: | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| S-01 | P1 | resolved/superseded | Typing currently checks a weaker permission than posting. Landed in B2-5 (PR #1440): typing delegates to the send-policy predicate; denial, announcement, archive and override tests prevent drift. | B2/B3 | No separate action; the parity table locks it. | +| S-02 | P1 | confirmed | Invite create/revoke are privileged mutations without the audit coverage used by sibling mutation families. | B4/B5 | Successful create/revoke produce safe, non-secret audit events; failure behavior is tested. | +| S-03 | P2 | confirmed | Admin channel name/topic/category validation lacks one explicit rune/normalization contract. | B3/B5 | Shared limits cover admin and user writers; boundary tests count runes, not bytes. | +| S-04 | P2 | confirmed | Sibling admin channel lookups expose inconsistent DM/not-found response contracts. | B3 | One non-DM resolution policy and response contract covers both paths. | +| S-05 | P2 | confirmed | Repository-wide Go formatting is not a required gate. | B1 | Tree is formatted and a fast required gate fails future drift. | +| S-06 | P2 | confirmed | Server coverage is uploaded without a global or core-package regression floor; current aggregate is 74.6%. | B3/B10 | Documented baseline/exclusions and ratcheted global/core thresholds. | +| S-07 | P2 | confirmed | Thousands of tests and 17 fuzz targets exist, but there are no Go benchmarks for hub/replay, permission, DB, or fan-out hot paths. | B6/B10 | Stable microbenchmarks and reference load baselines cover the highest-risk paths. | +| S-08 | P2 | confirmed | Large lifecycle/hub/serve files remain structural hotspots. | B3 | Cohesive extractions preserve lifecycle, locking, race, and deadlock invariants. | +| S-09 | P2 | confirmed | API/admin/WebSocket layers still contain many direct database call sites. | B3 | Each use moves behind a narrow service/store seam or is documented as an intentional transaction/composition boundary. | +| S-10 | P2 | confirmed | Auth routes still consume raw database ownership and are the first intended S-09 migration slice. | B3/B4 | Tested AuthService/narrow interfaces preserve enumeration and sentinel-error behavior. | +| S-11 | P2 | confirmed | Hub construction uses post-construction collaborator setters, leaving required wiring temporally coupled to `Run`. | B3 | Required collaborators are validated constructor/options inputs; only genuinely dynamic dependencies remain mutable. | +| S-12 | P2 | resolved/superseded | Ready/refresh/WebSocket paths mirror message send-permission policy by hand. Landed in B2-5 (PR #1440): all paths delegate to one value-taking predicate with parity tests in both resolution branches. | B3 | No separate action; the authz-chokepoint invariant rule remains B3 item 15. | +| S-13 | P2 | confirmed | Durable TOTP used-code and partial-auth persister work remains incomplete. | B4 | Hash-only persistence, expiry, restart, and failure-mode tests land without persisting sliding rate-limit windows. | +| S-14 | P1 | confirmed | Load tooling exists, but no supported capacity result is published for the approved 250 users / 100 connections / 25 voice profile. | B6/B10 | Reproducible report states hardware/software, CPU, memory, DB waits, p95/p99 latency, and pass/fail thresholds. | +| S-15 | P3 | verify | `voice_speakers` and `member_leave` remain reserved protocol entries with no production emit site. | B2 | Compatibility review removes unused entries before the epoch freeze or explicitly reserves and fixtures them; schema, generated types, docs, and tests agree. | +| S-16 | P3 | verify | Voice key-holder TOCTOU hardening remains a documented follow-up without a demonstrated contract failure. | B2/B3 | Threat-model review either records why outer checks suffice or adds an in-function recheck and race-focused private test. | +| S-17 | P3 | watch | Vulnerability tooling found no reachable Go advisory, while non-called/unmaintained upstream paths remain. | B6/B10 | Dependency path is monitored, compatible fixes are applied, and reachable-symbol scanning remains required. | ## Repository, CI, documentation, and supply chain