Merge pull request #1199 from J3vb/docs/plugin-critical-closure

feat(plugin): close audit plugin CRITICALs — per-command ACL, storage/event/HTTP dispositions

docs/audit-2026-04-07.md

@@ -5,20 +5,40 @@
 
 ---
 
-## Finding closure status (maintained; last updated 2026-07-18)
+## Finding closure status (maintained; last updated 2026-07-20)
 
 Every CRITICAL/HIGH below must end with a closing commit link or an explicit
 mitigation before the beta gate. Standing rule: any plugin CRITICAL still
-OPEN at the beta gate → plugins ship default-disabled (they already default
-to `plugins.enabled: false`).
+OPEN at the beta gate → plugins ship default-disabled.
+
+**Rule status 2026-07-20:** finding #5 is closed as *accepted residual risk*,
+not fixed, so the rule fires — plugins ship default-disabled at beta.
+Verified in code: `config.DefaultConfig()` sets `Plugins.Enabled: false` and
+`Plugins.HTTPAllowlist: []string{}` (`Server/config/config.go:206-212`), and
+`hostAllowed` denies every host against an empty allowlist
+(`Server/plugin/host_http.go:140-158`, pinned by
+`TestEmptyAllowlistDeniesEveryHost`).
+
+**Structural mitigation covering #2, #4 and #5:** no host imports are wired
+into the wazero runtime. `activateWithRuntime` instantiates guest modules with
+WASI preview-1 only (`Server/plugin/sandbox_wazero.go:69-124`), and
+`HTTPDo` / `Storage*` have no callers outside the `plugin` package's own
+tests. `EventSink.Dispatch` is the exception — `Server/ws/hub.go:1034` calls
+it on every broadcast when plugins are enabled — but its loop body is inert
+and nothing outside tests calls `EventSink.Subscribe`, so it iterates over an
+empty subscriber set and reaches no guest code (see #4). The only
+guest-reachable entry points today are `command_dispatch` (via the WS
+`chat_command` handler) and `list_commands` at activation. Wiring those host
+imports is what makes #5 exploitable at all and is the point at which #4's
+rate limit must exist.
 
 | # | Sev | Finding | Status |
 |---|-----|---------|--------|
-| 1 | CRITICAL | Plugin `invokeCommand` has no timeout | IN PROGRESS — CPU budget added on `fix/security-hardening-review`; regression fix (module bricking, W1-1) required before merge |
-| 2 | CRITICAL | Plugin storage has no per-plugin key isolation | OPEN — verify/close in P3 |
-| 3 | CRITICAL | Plugin per-command ACL missing (auto-registration) | OPEN — verify/close in P3 |
-| 4 | CRITICAL | No rate limit on event delivery to plugins | OPEN — verify/close in P3 |
-| 5 | CRITICAL | Plugin HTTP capability allows data exfiltration to allowlisted hosts | OPEN — partially mitigated by SSRF hardening + allowlist; document residual risk in P3 |
+| 1 | CRITICAL | Plugin `invokeCommand` has no timeout | **CLOSED 2026-07-20** — verified in code. Every guest call (`allocate` / `command_dispatch` / `deallocate`) runs under a per-invocation deadline: `budgetMs` = manifest `resources.cpu_budget_ms` → `plugins.cpu_budget_ms` → hard 100 ms floor, applied via `context.WithTimeout` (`Server/plugin/sandbox_wazero.go:257-268`). The runtime is built `WithCloseOnContextDone(true)` (`sandbox_wazero.go:69-73`) so an expired deadline interrupts a runaway guest (`for {}`), and `releaseClosedModule` (`sandbox_wazero.go:326-338`) drops the closed module so the next dispatch re-instantiates lazily instead of bricking the plugin (regression W1-1). Landed in PR #1182 (`0f58ddd` budget, `2111976` W1-1). Pinned by `TestWazeroCPUBudgetOverrunDoesNotBrickPlugin` (`sandbox_wazero_test.go:271`) |
+| 2 | CRITICAL | Plugin storage has no per-plugin key isolation | **CLOSED 2026-07-20** — the finding's premise does not hold against the code. Isolation is structural, not a check that can be skipped: every `Storage*` call passes the caller's `Instance.ID` as the namespace and exposes no parameter by which a caller — let alone a guest module — could name another plugin's namespace (`Server/plugin/host_storage.go:26-73`), and `plugin_kv PRIMARY KEY (plugin_id, key)` (`Server/migrations/015_plugins.sql:13-18`) makes the same split the storage layout. Every query filters on `plugin_id` (`Server/db/plugin_queries.go:87-135`). This PR adds `TestStorageKeysIsolatedPerPlugin` pinning it (same key from two plugins does not collide; scan/delete do not cross namespaces) plus the missing key-size cap the file's doc comment already promised |
+| 3 | CRITICAL | Plugin per-command ACL missing (auto-registration) | **CLOSED 2026-07-20 (this PR)** — the manifest is now the per-command ACL. `plugin.json` gains a `commands` block; `RegisterCommand` refuses any name the manifest did not declare (`Server/plugin/host_commands.go:31-45`, `ErrCommandNotDeclared`), which is the single choke point both `list_commands` auto-registration (`sandbox_wazero.go:146-153`) and direct registration route through. A guest can therefore no longer widen its own command surface, and an admin can see the full command list before enabling. Declared names are validated to the dispatcher's canonical form, deduplicated, and capped at 64 (`manifest.go:207-233`). Cross-plugin hijack was already refused and stays refused. Pinned by `TestRegisterCommandRequiresManifestDeclaration` + `TestManifestCommandsValidation` |
+| 4 | CRITICAL | No rate limit on event delivery to plugins | **CLOSED 2026-07-20 (no guest code on the event path)** — there is no guest delivery to rate-limit. Note what *is* wired, so this is not mistaken for an absent call site: `EventSink.Dispatch` has exactly one caller outside the `plugin` package's tests — `Server/ws/hub.go:1034`, invoked on **every** broadcast message whenever an operator enables plugins (`Server/api/router.go:134-139` sets `h.pluginSink` when the registry is non-nil), on the hub's broadcast goroutine while `seqMu` is held. What makes the finding unreachable is one level down: `Dispatch`'s loop body invokes no guest code in either build (it touches no `inst.module`), and no production code calls `EventSink.Subscribe` — only tests — so `subs` is empty and the loop never iterates. A plugin cannot slow the hub by handling events slowly because no plugin ever handles one. Recorded as a gate rather than left silent: the SECURITY GATE comment on `Server/plugin/host_events.go` requires the per-plugin rate limit, the `invokeCommand` CPU deadline, and off-hub-goroutine delivery to land *in the same change* that wires guest delivery — and flags that the hot call site already exists, so wiring is a one-line `Subscribe` away, not a new integration. `TestEventDeliveryHasNoGuestPath` fails if delivery appears without that review |
+| 5 | CRITICAL | Plugin HTTP capability allows data exfiltration to allowlisted hosts | **OPEN — accepted residual risk (2026-07-20)**. Not fixable by hardening: an allowlisted host is by definition a permitted destination, so a plugin holding `http` can POST anything it can read to it. Closing it properly needs egress content policy (per-plugin request/response body inspection, byte budgets, per-plugin allowlists instead of one server-wide list) — a plugin-runtime redesign, not a patch. Standing mitigations, all verified in code: (a) `plugins.enabled` defaults false; (b) `plugins.http_allowlist` defaults empty and an empty allowlist denies every host, so the capability is inert until an operator names a destination; (c) the manifest must declare `http`, which is visible to the admin before enabling; (d) no host import is wired, so guest code cannot call `HTTPDo` at all today; (e) SSRF hardening (allowlist dot-boundary matching, guarded dial that vets every resolved IP before connecting, redirect re-checks, 5 MiB response cap) confines reach to public allowlisted hosts. Residual risk accepted for alpha/beta: an operator who both enables plugins and allowlists a host trusts the plugins they install with data those plugins can read |
 | 6 | HIGH | `Server/store/` untested | SUPERSEDED — `store/` package is being removed in P4 (single data layer); tests move to in-memory SQLite |
 | 7 | HIGH | Client `src/lib`/`src/stores` <10% unit coverage | CLOSED since audit — large vitest suite exists (113 files); suite health tracked in P2 |
 | 8 | HIGH | Unpinned critical npm packages | OPEN — review in P2 |
@@ -373,6 +393,13 @@ Auth flow, channels, messages, DMs, health/reconnect, UI overlays, voice control
 
 ### CRITICAL Issues
 
+> **Closure status (2026-07-20):** the table below is the original 2026-04-07
+> record and is kept verbatim. Current state lives in the closure table at the
+> top of this document — findings 1–4 are closed; #5 (HTTP exfiltration to an
+> allowlisted host) is accepted residual risk, which keeps plugins
+> default-disabled at the beta gate. Line numbers below refer to the audited
+> tree, not today's.
+
 | SEVERITY | File:Line | Finding |
 |----------|-----------|---------|
 | **CRITICAL** | `Server/plugin/sandbox_wazero.go:162,211` | `invokeCommand` has **no timeout** — a looping plugin hangs the goroutine indefinitely |

docs/audit-2026-07-19.md

@@ -49,7 +49,7 @@ audit's table; details stay in [audit-2026-04-07.md](audit-2026-04-07.md).
 
 | Prior # | Sev | Finding (one-line) | Re-verification (2026-07-19) |
 |---------|-----|--------------------|------------------------------|
-| 1–5 | CRITICAL | Plugin governance (timeout, storage isolation, ACL, event rate limit, HTTP exfiltration) | Unchanged since prior closure table; plugins still default-disabled (`plugins.enabled: false`), which is the standing mitigation |
+| 1–5 | CRITICAL | Plugin governance (timeout, storage isolation, ACL, event rate limit, HTTP exfiltration) | **Re-verified 2026-07-20 (P3)** — 1–4 closed, #5 accepted residual risk; full per-finding detail (with file:line and pinning tests) stays in [audit-2026-04-07.md](audit-2026-04-07.md). #1 closed by the CPU budget + lazy re-instantiation in PR #1182; #2 closed as structural — the KV namespace is the caller's `Instance.ID` and `plugin_kv PRIMARY KEY (plugin_id, key)`, no parameter can name another plugin's namespace; #3 closed by a new manifest `commands` ACL that `RegisterCommand` enforces, so `list_commands` can no longer bind undeclared names; #4 closed because no guest code runs on the event path — `EventSink.Dispatch` *is* called from `Server/ws/hub.go:1034` on every broadcast when plugins are enabled, but its loop body invokes no guest code and no production code calls `EventSink.Subscribe`, so it iterates an empty subscriber set; a SECURITY GATE comment on `Dispatch` requires the rate limit, CPU deadline and off-hub-goroutine delivery in whatever change wires real delivery, and warns that the hot call site already exists; #5 is not fixable by hardening (an allowlisted host *is* a permitted destination) and is accepted for alpha/beta. Standing mitigation unchanged and re-verified: `plugins.enabled: false` and an empty `plugins.http_allowlist` by default, plus no host imports wired into the wazero runtime |
 | 6 | HIGH | `Server/store/` untested | **RESOLVED 2026-07-19 (D3)** — the `store/` package is deleted rather than tested. `SQLiteStore` was a pure pass-through to `*db.DB`; its event/plugin methods moved into `db` (`event_queries.go`, `plugin_queries.go`). Consumers now depend on narrow interfaces `*db.DB` satisfies (`service.Store`, `ws.EventStore`, `plugin.PluginStore`), and the former `MemStore`-based unit tests run against a real in-memory SQLite `db` — so the code paths that were untested through the seam are now exercised directly |
 | 7 | HIGH | Client unit coverage | Suite is large (157 test files) and green; flipping `client-tests` to blocking is backlog #10 — see A-2026-07-04 |
 | 9 | MEDIUM | auth_handler bypasses service layer | **Confirmed open** — `Server/api/router.go:101` passes `database *db.DB` to `MountAuthRoutes` while sibling mounts receive `svc` |

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–D10)
+**Date decided:** 2026-07-19 (D1–D8); 2026-07-20 (D9–D11)
 **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 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`). |
 
 ## Suggested sequencing
 

docs/plans/slash-commands.md

@@ -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",