diff --git a/docs/architecture/server.md b/docs/architecture/server.md index c094d497..bb58f306 100644 --- a/docs/architecture/server.md +++ b/docs/architecture/server.md @@ -119,10 +119,16 @@ sequenceDiagram chi's `middleware.RealIP` is deliberately omitted — client IP is resolved via `clientIPWithProxies` against configured trusted proxies instead, so spoofed `X-Real-IP`/`X-Forwarded-For` headers are not trusted by default. Authentication -is bearer-token (SHA-256-hashed opaque tokens); authorization is enforced -inconsistently — sometimes as `RequirePermission` middleware at mount time, -sometimes in-handler through `svc.Permissions` (an audit finding). The shaded -region marks the two documented bypass paths of the domain layer. +is bearer-token (SHA-256-hashed opaque tokens); authorization is enforced at two +deliberate scopes (D13): `RequirePermission` middleware gates the two +channel-less routes on server-wide role permissions via +`permissions.HasServerPerm` (channel overrides deliberately not consulted — a +per-channel allow must never open a server-wide gate), while anything +channel-scoped is checked in the service layer through `svc.Permissions` / +`permissions.Checker`, which resolves overrides and fails closed if they cannot +be fetched. The shaded region marks the two documented bypass paths of the +domain layer. **Source of truth:** `Server/api/router.go`, `Server/api/middleware.go`, -`Server/api/auth_handler.go`, `Server/admin/middleware.go`, `Server/service/`. +`Server/api/auth_handler.go`, `Server/admin/middleware.go`, +`Server/permissions/`, `Server/service/`. diff --git a/docs/audit-2026-07-19.md b/docs/audit-2026-07-19.md index d4b60ecd..7b0f100a 100644 --- a/docs/audit-2026-07-19.md +++ b/docs/audit-2026-07-19.md @@ -20,7 +20,7 @@ accepted-risk note before the beta gate. MEDIUMs are folded into the backlog | A-2026-07-04 | HIGH | Client unit test suite "KNOWN RED" and non-blocking in CI; E2E never gated | CLOSED 2026-07-20 — the "KNOWN RED" premise was stale: suite verified green on `main` (3261/3261, 114 files) and the annotations in both CLAUDE.md files + `ci.yml` corrected to "green, must stay green". Root cause of the false premise: on Node 22+, native Web Storage shadows jsdom's `localStorage`, failing ~478 unrelated tests locally; with `NODE_OPTIONS=--no-experimental-webstorage` (CI pins Node 20) the suite is fully green. Documented in the client CLAUDE.md and the `ci-check` skill so it is not re-misdiagnosed. Flipping `client-tests` to blocking + adding the nightly Playwright gate remain tracked as backlog #10 | | A-2026-07-05 | MEDIUM | Dead sqlc layer: `Server/db/dbgen/` (~3.5k LOC) generated + CI-verified but imported by nothing | RESOLVED 2026-07-19 — `dbgen` wired into `db.DB`; 97 methods across all domains delegate to it (no longer dead). Remaining raw queries (variable IN / FTS / tx) tracked in [plans/sqlc-adoption.md](plans/sqlc-adoption.md) | | A-2026-07-06 | MEDIUM | Three coexisting DB-access styles (raw `*db.DB` in api/admin/ws, `store.Store` under service, dead dbgen) | RESOLVED 2026-07-19 — collapsed to a single sqlc-backed `db` package: dbgen wired in (D2) and the `store` seam deleted (D3). The service layer depends on a narrow `service.Store` interface `*db.DB` satisfies; ws/plugin similarly. Broadening service-only access above the remaining direct-`db` handlers is the residual layering work (A-2026-07-06 backlog item 12) | -| A-2026-07-07 | MEDIUM | Channel-visibility logic duplicated across ~4 sites with "must mirror" comments | RESOLVED 2026-07-20 (D9) — all four sites (REST `ListVisibleChannels`, ws `buildReady`, replay `computeAllowedChannels`, hub `RefreshChannelVisibility`) route through one `permissions.Checker` predicate (`VisibleChannelIDs` / `HasChannelPerm`); REST/WS agreement test added | +| A-2026-07-07 | MEDIUM | Channel-visibility logic duplicated across ~4 sites with "must mirror" comments | RESOLVED 2026-07-20 (D9) — all four sites (REST `ListVisibleChannels`, ws `buildReady`, replay `computeAllowedChannels`, hub `RefreshChannelVisibility`) route through one `permissions.Checker` predicate (`VisibleChannelIDs` / `HasChannelPerm`); REST/WS agreement test added. 2026-07-23 (D13): a missed fifth copy (`MessageService.GetAccessibleChannelIDs`) was found re-inlining the rule and now delegates to `VisibleChannelIDs` | | A-2026-07-08 | MEDIUM | Protocol constants on both sides claim generation from `docs/protocol-schema.json`, which does not exist in the repo | CLOSED 2026-07-19 — codegen implemented: `docs/protocol-schema.json` + `Server/scripts/genprotocol` + `make protocol-verify` CI gate | | A-2026-07-09 | MEDIUM | Dual V1+V2 WS dispatch (strangler-fig) still live; two parsers/registries to keep in sync | RESOLVED 2026-07-20 (D10) — the 3 remaining V1 types (`chat_command`, `voice_join`, `voice_leave`) ported to typed V2 handlers; the V1 registry + fallback path deleted. `handleMessage` has a single dispatch generation. Server-internal only, no wire change | | A-2026-07-10 | MEDIUM | `api.NewRouter` god-constructor: builds services, hub, LiveKit, updater, admin, plugins; spawns goroutines; mounts everything | OPEN | @@ -29,6 +29,7 @@ accepted-risk note before the beta gate. MEDIUMs are folded into the backlog | A-2026-07-13 | LOW | Dead schema: `sounds` table survives soundboard removal (correction 2026-07-19: `audit_log_v6` is only a transient rename inside migration 003, not a coexisting table) | OPEN | | A-2026-07-14 | LOW | Scattered client constants (`#5865F2` ×18, `localhost:8443` ×3); 64 timer call sites with manual lifecycle | OPEN | | A-2026-07-15 | LOW | `docs/plans/security-hardening-remediation.md` partly stale (references deleted `store/postgres.go`) | OPEN | +| A-2026-07-16 | HIGH | Server-wide permission rule hand-rolled at 2 sites (`RequirePermission` raw any-of bit test; `ModerationService`); channel-level `deny` silently dropped — and cached for 30s — when the override fetch errors, at 2 of 5 sites | RESOLVED 2026-07-23 (D13) — `permissions.HasServerPerm` now owns the server-scoped rule (both sites collapse onto it; multi-bit masks are all-of); both override-fetch sites fail closed (`getOrPopulate` skips the fetch for admins, denies and caches nothing on error; `ListVisibleChannels` returns `ErrInternal`); the fifth D9 site (`GetAccessibleChannelIDs`) routes through `VisibleChannelIDs`. Locked by failing-first tests. See [plans/permission-middleware-consolidation.md](plans/permission-middleware-consolidation.md) | --- @@ -119,12 +120,13 @@ tooling). The findings are about the seams that grew around that design. |----|-----|------|----------|---------|----------------|--------| | A-2026-07-05 | MEDIUM | Data layer | `Server/db/dbgen/` (~3.5k LOC), `sqlc.yaml`, CI `sqlc-verify` job | sqlc output is generated, version-pinned, CI-verified — and imported by nothing. Hand-written raw SQL in `Server/db/*_queries.go` is what runs. | Decide the Phase-A question: adopt dbgen inside `db.DB` method bodies, or delete `dbgen/` + `queries/` + the CI job. Either ends the illusion of a second data layer. | S | | A-2026-07-06 | MEDIUM | Layering | All `Mount*Routes` signatures take `database *db.DB` alongside `svc` (`Server/api/*_handler.go`); `Server/admin` handlers take `*db.DB` | ~359 direct `database.*` calls above the store seam; three access styles coexist. The abstraction exists but cannot be relied on (e.g. for a future backend swap or for test doubles). | Consolidate incrementally: new handlers service-only; migrate one mount per PR, starting with auth (prior #9). | L | -| A-2026-07-07 | MEDIUM | Correctness risk | `Server/ws/serve.go` (`buildReady`, `computeAllowedChannels`), `Server/ws/hub.go` (`RefreshChannelVisibility`), REST `handleListChannels` | Channel-visibility filtering implemented ~4× with comments instructing they "must mirror" each other. The recent private-channel fixes (e.g. `2bfe6d6`) show this is actively churning — a drift between copies is an information-disclosure bug waiting to happen. | **Resolved 2026-07-20 (D9)** — added `permissions.Checker.VisibleChannelIDs`; all four sites route through it (`RefreshChannelVisibility` uses the single-channel `HasChannelPerm`); REST/WS agreement test added. | M | +| A-2026-07-07 | MEDIUM | Correctness risk | `Server/ws/serve.go` (`buildReady`, `computeAllowedChannels`), `Server/ws/hub.go` (`RefreshChannelVisibility`), REST `handleListChannels` | Channel-visibility filtering implemented ~4× with comments instructing they "must mirror" each other. The recent private-channel fixes (e.g. `2bfe6d6`) show this is actively churning — a drift between copies is an information-disclosure bug waiting to happen. | **Resolved 2026-07-20 (D9)** — added `permissions.Checker.VisibleChannelIDs`; all four sites route through it (`RefreshChannelVisibility` uses the single-channel `HasChannelPerm`); REST/WS agreement test added. **2026-07-23 (D13):** a missed fifth copy (`MessageService.GetAccessibleChannelIDs`, admin bypass + dm-skip + raw READ mask) found and routed through `VisibleChannelIDs`. | M | | A-2026-07-08 | MEDIUM | Protocol integrity | `Server/ws/message_types.go` header comment; `Client/…/src/lib/protocolTypes.ts` header + "Extensions (not in protocol-schema.json…)" comments | Both sides claim `docs/protocol-schema.json` is the generated single source of truth. The file does not exist; the two constant sets are maintained by hand and have already grown divergent "extension" entries. | Either commit a real `protocol-schema.json` + generator (best: also emits protocol.md tables), or delete the claim and add a cross-language equality test over the two constant sets. | M | | A-2026-07-09 | MEDIUM | Real-time | `Server/ws/handlers.go` (`handleMessage` V2-then-V1 fallback), dual registration in `NewHub` (`Server/ws/hub.go`) | Strangler-fig V1+V2 dispatch is live: two parsers (lenient/strict), two registries, per-type duplication. | **Resolved 2026-07-20 (D10)** — ported the last 3 V1 types (`chat_command`, `voice_join`, `voice_leave`) to typed V2 handlers and deleted the V1 registry + `handleMessage` fallback; a parity guard test locks the single dispatch path shut. | M/L | | A-2026-07-10 | MEDIUM | Composition | `Server/api/router.go:34` (`NewRouter`, ~278 lines) | God-constructor builds rate limiter, TOTP key, storage, services, hub, LiveKit client+process, updater, admin + plugin handlers; spawns goroutines; returns a cleanup closure covering only one of them. Hard to test wiring in isolation; lifecycle ownership is implicit. | Split construction (a `Deps`/`App` struct built in `main.go`) from route mounting (`NewRouter(deps)`); return a composite `io.Closer`. | M | | A-2026-07-11 | MEDIUM | Real-time | `Server/ws/hub.go` (`SetLiveKit`, `SetEventPersister`, `SetPluginRegistry`, …) | Hub is a mega-object wired post-construction via setters that "must be called before Run" — temporal coupling; a missed setter is a nil-deref at runtime, not a compile error. | Move required collaborators into `NewHub` params (or an options struct validated before `Run`). Full Hub decomposition is a separate, larger effort (backlog 12). | S (constructor) / L (decomposition) | | — | MEDIUM | Layering | `Server/ws/hub.go:182` (`refreshSettingsLocked`) | Hub runs inline `SELECT value FROM settings WHERE key='server_name'` instead of using `SettingsStore` — the only raw SQL in the real-time layer. | **Fixed 2026-07-19** — now uses `db.GetSetting`; full consolidation folds into A-2026-07-06. | S | +| A-2026-07-16 | HIGH | Correctness risk | `Server/api/middleware.go` (`RequirePermission`), `Server/service/moderation.go:38`, `Server/service/permission.go` (`getOrPopulate`), `Server/service/channel.go` (`ListVisibleChannels`), `Server/service/message.go` (`GetAccessibleChannelIDs`) | Found 2026-07-21 pulling on the D9 thread. (1) The server-wide rule (admin bypass + bit test) had no owner: two sites hand-rolled it, `RequirePermission` as any-of (`&perm != 0`) where `HasPerm` is all-of — identical for today's single-bit constants, silently divergent for any multi-bit mask. (2) On `GetAllChannelPermissionsForRole` error, two of five sites substituted an *empty override map*: every `deny` for that role evaporates, and `PermissionService` caches the degraded snapshot for `permCacheTTL` (30s) across `HasChannelPerm`'s ~25 callers. The three sibling sites fail closed on the identical error. | **Resolved 2026-07-23 (D13)** — `permissions.HasServerPerm` (admin bypass, all-of) owns the server rule; both fetch sites fail closed (admin skip + deny-and-cache-nothing / `ErrInternal`, `slog.Error` at the fail point); `HasChannelPerm` delegates to `Checker.HasChannelPermBatch`; `GetAccessibleChannelIDs` delegates to `VisibleChannelIDs`. See [plans/permission-middleware-consolidation.md](plans/permission-middleware-consolidation.md). | M | | — | LOW | Scaling posture | `Server/auth/ratelimit.go` (documented), in-memory pub/sub + ring buffer, process-local TOTP replay | Single-instance coupling is structural and *documented* — this is a deliberate design, not a bug. Recorded here so the constraint stays visible ([architecture/system-overview.md D8](architecture/system-overview.md)). | No action now; revisit only if multi-instance ever becomes a goal. | — | | A-2026-07-13 | LOW | Schema hygiene | `sounds` table (`Server/migrations/001`), `audit_log` + `audit_log_v6` (`003`) | Dead/duplicated schema: soundboard was removed but its table remains; two audit-log tables coexist after the 003 rebuild. | Add a cleanup migration (drop `sounds`, finish the audit_log consolidation) next time a migration ships anyway. | S | diff --git a/docs/plans/audit-2026-07-19-decisions.md b/docs/plans/audit-2026-07-19-decisions.md index b66652d4..5266cb9e 100644 --- a/docs/plans/audit-2026-07-19-decisions.md +++ b/docs/plans/audit-2026-07-19-decisions.md @@ -1,8 +1,8 @@ # Audit 2026-07-19 — Maintainer Decisions -**Date decided:** 2026-07-19 (D1–D8); 2026-07-20 (D9–D11) +**Date decided:** 2026-07-19 (D1–D8); 2026-07-20 (D9–D12); 2026-07-21 (D13) **Decided by:** J3vb -**Status:** decisions recorded; greenlit items (D4, D7, D8) implemented 2026-07-19 — see per-row Status. **2026-07-20:** backlog items 3 and 11 (D9, D10) implemented — channel-visibility unified through `permissions.Checker`; V2 dispatch migration finished and V1 deleted. **2026-07-20 (P3):** the five plugin CRITICALs carried over from audit-2026-04-07 dispositioned (D11) — four closed, one accepted as residual risk, which keeps plugins default-disabled at the beta gate. +**Status:** decisions recorded; greenlit items (D4, D7, D8) implemented 2026-07-19 — see per-row Status. **2026-07-20:** backlog items 3 and 11 (D9, D10) implemented — channel-visibility unified through `permissions.Checker`; V2 dispatch migration finished and V1 deleted. **2026-07-20 (P3):** the five plugin CRITICALs carried over from audit-2026-04-07 dispositioned (D11) — four closed, one accepted as residual risk, which keeps plugins default-disabled at the beta gate. **2026-07-23:** D13 implemented — server-scoped permission rule unified in `permissions.HasServerPerm`; override-fetch errors fail closed instead of dropping (and caching the loss of) every channel `deny`; the fifth D9 site routed through the `Checker`. **Source:** decision points raised by [docs/audit-2026-07-19.md](../audit-2026-07-19.md) This document records the maintainer's answers to the open decision points from @@ -26,6 +26,7 @@ here (and the audit's closure table) as items land. | D10 | Finish the V2 dispatch migration; delete V1 | A-2026-07-09 / backlog 11 | **Greenlit 2026-07-20 — implement**: port the 3 remaining V1 types (`chat_command`, `voice_join`, `voice_leave`) to V2, then delete the V1 registry + fallback path. Server-internal only, no wire change. See [v2-dispatch-migration.md](v2-dispatch-migration.md). | **Implemented 2026-07-20** — the 3 types ported to typed V2 handlers (voice join/leave hand off to the hub routines via new `Result.JoinVoice`/`LeaveVoice` appliers); V1 registry + `handleMessage` fallback deleted; a constructor↔handler parity guard test locks it shut. No wire change. | | D11 | Disposition of the five plugin CRITICALs from audit-2026-04-07 (§1 carried-over row) | prior #1–#5 | **Close what the code already closes; fix the one cheap real gap; accept the one that hardening cannot fix.** Verified each against `Server/plugin/` rather than the tracker: #1 (no `invokeCommand` timeout) closed by PR #1182 — per-call CPU budget with a 100 ms floor plus `WithCloseOnContextDone` and lazy re-instantiation so an overrun does not brick the plugin. #2 (storage key isolation) closed as structural — the namespace is the caller's `Instance.ID` and `plugin_kv PRIMARY KEY (plugin_id, key)`; no parameter exists by which a plugin could name another's namespace, so the finding's premise was wrong. #3 (per-command ACL) was a **real gap** and is fixed here: the manifest gains a `commands` block and `RegisterCommand` refuses undeclared names, so `list_commands` can no longer widen a plugin's command surface behind the admin's back. #4 (event rate limit) closed because no guest code executes on the event path — precisely: `EventSink.Dispatch` has exactly one caller outside the plugin package's tests (`Server/ws/hub.go:1034`, on every broadcast when plugins are enabled, on the hub goroutine under `seqMu`), but its loop body invokes no guest code and no production code calls `EventSink.Subscribe`, so the subscriber set is always empty. Rather than build a limiter for guest calls that do not happen, the requirement is recorded as a SECURITY GATE comment on `Dispatch` and `Subscribe` — the exact places someone would wire delivery — including the warning that the hot call site already exists and sits under the hub's `seqMu`. #5 (HTTP exfiltration to an allowlisted host) **stays open as accepted residual risk** — an allowlisted host is by definition permitted, so closing it needs egress content policy and per-plugin allowlists (a runtime redesign, ~1–2 weeks), explicitly out of scope for P3. | **Implemented 2026-07-20** — closure tables in [audit-2026-04-07.md](../audit-2026-04-07.md) and the §1 row of [audit-2026-07-19.md](../audit-2026-07-19.md) updated; manifest `commands` ACL + key-size cap + five pinning tests landed (`Server/plugin/audit_closure_test.go`). Because #5 remains open, the standing rule fires as written: **plugins ship default-disabled at the beta gate** — re-verified in `config.DefaultConfig()` (`Plugins.Enabled: false`, empty `HTTPAllowlist`). | | D12 | Who supplies the GIF (Klipy) API key | P3 item 1 / A-2026-07-02 family | **Decided 2026-07-20 — per-operator key, feature default-off.** The key previously shipped inside the client bundle via `VITE_KLIPY_API_KEY`. That is not a sharing arrangement but a disclosure: Vite inlines the value verbatim, so anyone who downloaded the client could extract the maintainer's key and use it for any purpose, with the maintainer carrying the quota, abuse and terms-of-service exposure. The key is now server-side only (`gif.api_key` / `OWNCORD_GIF_API_KEY`). **Alternatives considered and rejected:** a project-hosted proxy holding the maintainer's key (preserves zero-config GIFs and keeps revoke/rate-limit control, but introduces a hard central dependency into a self-hosted product, puts every server's search queries through maintainer infrastructure, and leaves the maintainer paying the quota), and a hybrid falling back to that proxy when unconfigured (same objections, opt-out only). **Consequence accepted:** each operator requests their own key at partner.klipy.com; fresh installs have GIFs off and the client shows "GIFs are not enabled on this server". Discoverability is handled in the README feature list and a quick-start section rather than by defaulting the feature on. | **Implemented 2026-07-20** — server proxy + default-off contract in #1198; `VITE_KLIPY_API_KEY` deleted from source and from all three release build jobs. Old key rotation is a maintainer action, sequenced after the new path is verified working. | +| D13 | Server-wide permission rule hand-rolled outside `permissions`; channel `deny` dropped — and cached — on override-fetch error | A-2026-07-16 | **Greenlit 2026-07-21 — implement**: add `permissions.HasServerPerm` (admin bypass OR all-of bit test, four lines, no DB) and collapse `RequirePermission` + `ModerationService.requireBanPermission` onto it; fail closed at both override-fetch sites; delegate `PermissionService.HasChannelPerm` and `GetAccessibleChannelIDs` to the `Checker`. Explicitly **not** making `RequirePermission` channel-aware — neither of its routes has a channel id, and a per-channel allow must never open a server-wide gate. Auth-route direct-db sweep (backlog row 12 / A-2026-07-06) deferred to a future D14; row 12 unchanged by this work. See [permission-middleware-consolidation.md](permission-middleware-consolidation.md). | **Implemented 2026-07-23** — `HasServerPerm` owns the rule (multi-bit masks now all-of); both fetch sites fail closed with `slog.Error` (nothing cached on error, so the next request retries); fifth D9 site (`GetAccessibleChannelIDs`) routes through `VisibleChannelIDs`; `AuthMiddleware` gains the dangling-role nil guard (401). Locked by failing-first deny tests plus 403 locks on both `RequirePermission` routes. | ## Suggested sequencing diff --git a/docs/plans/permission-middleware-consolidation.md b/docs/plans/permission-middleware-consolidation.md new file mode 100644 index 00000000..76ea754c --- /dev/null +++ b/docs/plans/permission-middleware-consolidation.md @@ -0,0 +1,127 @@ +# Permission-Middleware Consolidation (audit finding A-2026-07-16) — Design + +**Status:** implemented 2026-07-23 (D13) +**Decision:** D13 in [audit-2026-07-19-decisions.md](audit-2026-07-19-decisions.md) +**Closes:** audit finding A-2026-07-16 (new, HIGH). Closes **none** of backlog §6 +item 12's findings — A-2026-07-06, A-2026-07-10 and A-2026-07-11 all remain +exactly as recorded; row 12 stays unstruck and unannotated. + +## Problem + +Two separate defects in the same rule, found by pulling on the same thread. + +**1. The server-wide rule has no home.** `api.RequirePermission` (`middleware.go:112`) +does an admin bypass and then a raw `role.Permissions&perm == 0` bit test. +`service.ModerationService` (`moderation.go:38`) writes the same rule as +`!HasAdmin(p) && !HasPerm(p, BanMembers)`. The `permissions` package exposes +`HasPerm`, `HasAdmin`, `EffectivePerms` and four **channel-scoped** `Checker` +methods — nothing combining the admin bypass with a channel-less bit, so both +sites hand-rolled it. The raw test is also any-of (`&perm != 0`) where `HasPerm` +is all-of: identical for the single-bit constants both call sites pass today, +silently divergent for any future multi-bit mask. + +**2. A channel-level `deny` is genuinely not honoured — one layer down.** +`PermissionService.getOrPopulate` (`permission.go:145-149`) and +`ChannelService.ListVisibleChannels` (`channel.go:58-61`) substitute an *empty +override map* when `GetAllChannelPermissionsForRole` errors. Every `deny` bit for +that role evaporates, and `PermissionService` then **caches** the degraded +snapshot for `permCacheTTL` (30s), across `HasChannelPerm`'s ~25 callers: message +reads, pins, attachment serving, WS. Meanwhile `permissions.Checker` +(`checker.go:60-63`), `MessageService.GetAccessibleChannelIDs` +(`message.go:643-646`) and `ws.buildReady` (`serve.go:622-624`) all fail *closed* +on the identical error. Two of five sites dissent, and they are the cached ones. + +D9 also declared `VisibleChannelIDs` the single visibility predicate; it missed a +fifth site — `GetAccessibleChannelIDs` still re-inlines admin bypass + dm-skip + +`EffectivePerms` + a raw READ mask (`message.go:639-666`). + +## Approach — one server-scoped predicate, and fail closed on override load + +1. Add `permissions.HasServerPerm(rolePerms, perm int64) bool` — four lines, + `HasAdmin(rolePerms) || HasPerm(rolePerms, perm)`, no DB, no interface. + `RequirePermission` and `ModerationService.BanUser` both collapse onto it. + `RequirePermission`'s signature is unchanged, so nothing needs replumbing. +2. Fail closed at both override-fetch sites. `getOrPopulate` first **skips the + fetch entirely for admins** (mirroring `channel.go:57` and `serve.go:619` — + they bypass every channel check anyway), then `return nil` on error, caching + nothing so the next request retries. `ListVisibleChannels` returns + `ErrInternal`. Both log `slog.Error` at the fail point. +3. Delete the two remaining copies of the channel rule. + `PermissionService.HasChannelPerm` delegates to the `Checker` it already holds + (`HasChannelPermBatch`) once `cachedPerms.overrides` carries + `permissions.ChannelOverride` — converted once at populate time by the + existing `permOverrides` helper (`channel.go:86`, same package, no adapter + needed). `GetAccessibleChannelIDs` calls `VisibleChannelIDs`, making D9's + closure true rather than aspirational. + +Routing `RequirePermission` through the `Checker` is explicitly **not** the fix — +see Non-goals. + +## Files touched + +- `Server/permissions/permissions.go` — add `HasServerPerm`. +- `Server/api/middleware.go` — `RequirePermission` uses it; `AuthMiddleware` + gains the missing `|| role == nil` (`GetRoleByID` returns `(nil, nil)` for a + missing row, so a dangling `role_id` puts a typed-nil `*db.Role` in ctx today; + `admin/middleware.go:52` already checks). +- `Server/service/moderation.go` — second server-scoped site collapses. +- `Server/service/permission.go` — admin skip + fail closed in `getOrPopulate`; + `HasChannelPerm` delegates; `cachedPerms.overrides` retyped. +- `Server/service/channel.go` — `ListVisibleChannels` fails closed. +- `Server/service/message.go` — `GetAccessibleChannelIDs` delegates to + `VisibleChannelIDs`. +- Docs: `docs/audit-2026-07-19.md` — new §1 + §3 rows for A-2026-07-16 + (RESOLVED 2026-07-23 (D13)); amend the A-2026-07-07 rows to record the missed + fifth site; row 12 untouched. `docs/plans/audit-2026-07-19-decisions.md` — D13 + row + status clause. `docs/architecture/server.md` — D3 prose (§"enforced + inconsistently") and the source-of-truth list. + +## Test plan + +- `TestHasChannelPerm_OverrideFetchErrorDenies` and + `TestListVisibleChannels_OverrideFetchErrorFailsClosed` — a `Store` double + embedding `*db.DB` (the `pwStore` pattern, `user_test.go:16`) that fails only + `GetAllChannelPermissionsForRole`, over a real seeded `deny`. **Both fail on + today's code**; they are the headline locks. +- `TestHasServerPerm` — table-driven, pins the all-of contract and the admin + bypass at the layer that owns the rule. +- `TestRequirePermission_MultiBitRequiresAllBits` — the any-of → all-of + tightening is the only semantic change to the middleware; nothing else fails if + someone reverts to `&perm != 0`. +- `TestCreateInvite_ChannelAllowOverrideDoesNotGrant` — a channel override + granting `MANAGE_INVITES` must not open a server-wide route. Reachable state: + `admin/handlers_channel_perms.go:100` masks with `AllPerms`, which permits it. +- `TestDiagnosticsConnectivity_MemberForbidden` and + `TestAuthMiddleware_DanglingRoleUnauthorized` — the second `RequirePermission` + route has no 403 lock at all today, and the nil-role guard is a 403→401 flip + that must not ship untested. +- Existing deny locks stay green untouched and are the regression net: + `channel_authz_test.go:94/110`, `channel_handler_test.go:788`, + `upload_handler_test.go:1222`, `permission_test.go:50`, `can_send_test.go:34`. + +## Non-goals + +- **Making `RequirePermission` channel-aware.** Neither route has a channel, chi + cannot hand a `r.Use` middleware a `{id}` declared on its own mux (v5.2.5 + `mux.go:513`), `GET /api/v1/files/{id}` could never use it (its channel id + comes from the DB row), and ws has no HTTP middleware — so it would be a + *second* enforcement point for a rule the `Checker` owns. `channelID=0` would + issue a query whose right-looking answer is an accident of `ErrNoRows` + handling (`db/channel_queries.go:140`), not a design. +- **The auth-route DB sweep (item 12 / A-2026-07-06).** `AuthMiddleware` has 20 + call sites and the auth handlers ride on raw db sentinels (`handleLogin`'s + enumeration defence needs `GetUserByUsername`'s `(nil, nil)`; + `ErrLastAdmin`→403; `IsUniqueConstraintError`→400). **D14**, first slice: a + `service.AuthService` behind `AuthMiddleware`, which also deletes the + `database *db.DB` parameter from the five Mount funcs that feed it nothing + else. That is when row 12 earns `PARTIAL`, not this PR. +- **A source-scanning guard test** for raw bit patterns — a homegrown regexp + lint with a known ceiling, catching what the two new tests plus review already + catch. Revisit as a `golangci-lint` rule if it recurs. +- **`ws.channelCanSend`** (`serve.go:583-590`) — the last hand-rolled copy. It + holds an override *value*, not a map, so reducing it needs a one-entry map + allocation on the ready hot path or a new value-taking predicate. Disclosed + deliberately rather than fixed; separate PR. +- No `(bool, error)` permission signatures (`ws/deps.go:86-90`'s + INTERNAL-vs-FORBIDDEN precedent is right but is a ~25-site change), no + rate-limiter reordering, no `IsOwnerRole` deletion, no 403 body change.