docs(audit): correct CRITICAL #4 closure — Dispatch has a live hub caller

The closure rationale for audit finding #4 claimed in five places that
nothing in the server calls EventSink.Dispatch. That is disprovable by
grep: ws/hub.go:1034 calls Dispatch on every broadcast message, and
api/router.go:134-139 wires h.pluginSink whenever plugins are enabled.
The call site is pre-existing on main, not introduced by this branch.

Restate the closure on the claim the evidence actually supports:
Dispatch has exactly one caller outside the plugin package's tests
(ws/hub.go, on the hub's broadcast goroutine under seqMu), but its loop
body invokes no guest code and no production code calls Subscribe, so
the subscriber set is always empty and no guest code executes on the
event path. Finding #4 stays closed; the reason changes.

Also warn on Subscribe that adding the first production caller turns
Dispatch's loop live on the hub's hot path, and note in the SECURITY
GATE that the call site already exists so wiring delivery is not a new
integration.

Corrected in: plugin/host_events.go (Dispatch + Subscribe comments),
plugin/audit_closure_test.go, docs/audit-2026-04-07.md (row 4 and the
structural-mitigation paragraph), docs/audit-2026-07-19.md §1 row,
docs/plans/audit-2026-07-19-decisions.md D11.

Comments and docs only — no behaviour change.
This commit is contained in:
J3vb
2026-07-20 14:36:24 +02:00
parent 4ab01c39df
commit 605f97051a
5 changed files with 42 additions and 20 deletions
+10 -6
View File
@@ -22,18 +22,22 @@ Verified in code: `config.DefaultConfig()` sets `Plugins.Enabled: false` and
**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*` / `EventSink.Dispatch` have no callers outside the
`plugin` package's own tests. 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.
`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 | **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 (not reachable)** — there is no event-delivery path to rate-limit. `EventSink.Dispatch` invokes no guest code in either build (the loop body is inert) and nothing in the server calls it: the WS hub never dispatches to the sink, and no `on_event` host wiring exists. A plugin cannot slow the hub by handling events slowly because it never handles one. Recorded as a gate rather than left silent: the SECURITY GATE comment on `Server/plugin/host_events.go:90-105` 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. `TestEventDeliveryHasNoGuestPath` fails if delivery appears without that review |
| 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 |
+1 -1
View File
@@ -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) |
|---------|-----|--------------------|------------------------------|
| 15 | CRITICAL | Plugin governance (timeout, storage isolation, ACL, event rate limit, HTTP exfiltration) | **Re-verified 2026-07-20 (P3)** — 14 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 as not reachable — `EventSink.Dispatch` invokes no guest code and has no callers, with a SECURITY GATE comment requiring the rate limit in whatever change wires delivery; #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 |
| 15 | CRITICAL | Plugin governance (timeout, storage isolation, ACL, event rate limit, HTTP exfiltration) | **Re-verified 2026-07-20 (P3)** — 14 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` |
+1 -1
View File
@@ -24,7 +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`). |
| 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, ~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