docs(audit): close plugin CRITICALs 1-4, accept #5 as residual risk

P3 item 4. Each of the five plugin CRITICALs in audit-2026-04-07.md was
re-verified against the current Server/plugin/ code rather than the tracker:

- #1 invokeCommand timeout — CLOSED. Per-call CPU budget (manifest →
  config → 100ms floor) + WithCloseOnContextDone + lazy re-instantiation.
  Landed in PR #1182 (7b178ff, b13adf2); pinned by the W1-1 test.
- #2 storage key isolation — CLOSED. The premise did not hold: the namespace
  is the caller's Instance.ID and plugin_kv PRIMARY KEY (plugin_id, key).
- #3 per-command ACL — CLOSED by the manifest `commands` ACL in 3d2dd19.
- #4 event rate limit — CLOSED as not reachable: EventSink.Dispatch invokes
  no guest code and has zero callers; the requirement is recorded as a
  SECURITY GATE at the point delivery would be wired.
- #5 HTTP exfiltration — OPEN, accepted residual risk. An allowlisted host is
  by definition a permitted destination; closing it needs egress content
  policy and per-plugin allowlists, i.e. a runtime redesign, out of scope
  for P3.

Because #5 stays open the standing rule fires as written: plugins ship
default-disabled at the beta gate. Re-verified in config.DefaultConfig() —
Plugins.Enabled false, HTTPAllowlist empty. Also records the structural
mitigation covering #2/#4/#5: no host imports are wired into the wazero
runtime, so command_dispatch and list_commands are the only guest-reachable
entry points today.

Mirrors the outcome in the §1 carried-over row of audit-2026-07-19.md,
records decision D11 in plans/audit-2026-07-19-decisions.md, and notes in
plans/slash-commands.md which slice of its manifest design already landed.
This commit is contained in:
J3vb
2026-07-20 14:10:15 +02:00
parent 3d2dd19001
commit 4ab01c39df
4 changed files with 43 additions and 11 deletions
+3 -2
View File
@@ -1,8 +1,8 @@
# Audit 2026-07-19 — Maintainer Decisions
**Date decided:** 2026-07-19 (D1D8); 2026-07-20 (D9D10)
**Date decided:** 2026-07-19 (D1D8); 2026-07-20 (D9D11)
**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.
**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.
**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
@@ -24,6 +24,7 @@ here (and the audit's closure table) as items land.
| D8 | What to implement first | backlog §6 | **Greenlit now: Protocol codegen (D4) + the quick-wins batch**`LogAudit` error handling (`admin/handlers_backup.go`), contradictory upload `Cache-Control` (`upload_handler.go`), hub inline settings SQL through the data layer (`ws/hub.go`), Hub constructor cleanup (required collaborators into `NewHub`). | **Implemented 2026-07-19** (all four quick wins + D4). Hub cleanup shipped as: race fix — `eventPersister`/`eventStore`/`pluginSink` are now atomic (they were plain fields written by `main.go` after `NewRouter` had already started `Run`); remaining pre-Run setters now reject late calls with an error log instead of racing silently. Note discovered during the work: the discarded-`LogAudit` pattern is repo-wide (23 call sites) — the two tracker-flagged backup handlers are fixed; whether best-effort audit writes stay the convention elsewhere needs a policy decision. |
| D9 | Channel-visibility unification (rule duplicated across ~4 "must mirror" sites) | A-2026-07-07 / backlog 3 | **Greenlit 2026-07-20 — implement**: funnel all four sites through the existing `permissions.Checker` predicate + one filter helper; add a REST/WS agreement test. See [channel-visibility-unification.md](channel-visibility-unification.md). | **Implemented 2026-07-20**`permissions.Checker.VisibleChannelIDs` + `ChannelRef`; `ListVisibleChannels`, `buildReady`, `computeAllowedChannels` delegate; `RefreshChannelVisibility` uses `HasChannelPerm`. REST/WS agreement test asserts all three sites yield the identical non-DM set. |
| 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 as not reachable — `EventSink.Dispatch` invokes no guest code and has zero callers; rather than build a limiter for a path that does not exist, the requirement is recorded as a SECURITY GATE comment at the exact place someone would wire delivery. #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, ~12 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`). |
## Suggested sequencing
+8
View File
@@ -109,6 +109,14 @@ Plugin manifests gain a `commands` block. The manifest is the source of
truth for the per-command schema; the runtime never trusts what the plugin
says at dispatch time. Example:
> **Partially landed 2026-07-20** (audit-2026-04-07 CRITICAL #3): the
> *name-only* slice of this block exists today — `plugin.json` accepts
> `"commands": [{"name": "kick"}]` and `Registry.RegisterCommand` refuses any
> command the manifest did not declare, so `list_commands` can no longer bind
> names behind the admin's back. `description` / `options` /
> `default_member_permissions` below are still design-only; unknown keys parse
> and are ignored, so manifests written against the full schema already load.
```json
{
"name": "moderation-tools",