docs(audit): record D13 closure of A-2026-07-16

Design note (permission-middleware-consolidation.md, status implemented),
closure-table + §3 rows for A-2026-07-16, the A-2026-07-07 amendment
recording the missed fifth site, the D13 decision row, and the settled
two-scope authorization contract in architecture/server.md. Backlog row
12 stays untouched: the auth-route sweep is deferred to a future D14.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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
 

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.