diff --git a/Server/plugin/audit_closure_test.go b/Server/plugin/audit_closure_test.go index 744b15c1..01d8e963 100644 --- a/Server/plugin/audit_closure_test.go +++ b/Server/plugin/audit_closure_test.go @@ -162,12 +162,14 @@ func TestStorageRejectsOversizedKeyAndValue(t *testing.T) { } } -// TestEventDeliveryHasNoGuestPath locks finding #4. The finding (a plugin -// slowing the server by handling events slowly) is not reachable today: -// Subscribe requires the capability and Dispatch invokes no guest code in -// either build. If this test has to change because Dispatch grew a real -// delivery path, that change must also bring the per-plugin rate limit — see -// the SECURITY GATE comment on EventSink.Dispatch. +// TestEventDeliveryHasNoGuestPath locks finding #4. Dispatch is called on the +// hub's broadcast path (ws/hub.go) whenever plugins are enabled, but the +// finding (a plugin slowing the server by handling events slowly) is not +// reachable today: Subscribe requires the capability and has no production +// callers, and Dispatch invokes no guest code in either build. If this test +// has to change because Dispatch grew a real delivery path, that change must +// also bring the per-plugin rate limit — see the SECURITY GATE comment on +// EventSink.Dispatch. func TestEventDeliveryHasNoGuestPath(t *testing.T) { sink := NewEventSink() noCap := &Instance{ID: 1, Manifest: &Manifest{Name: "nocap"}} diff --git a/Server/plugin/host_events.go b/Server/plugin/host_events.go index 8857af5a..22dc6b0e 100644 --- a/Server/plugin/host_events.go +++ b/Server/plugin/host_events.go @@ -58,6 +58,11 @@ func (s *EventSink) Emit(channelID int64, payload []byte) { // Subscribe binds inst to topic. Multiple plugins may subscribe to the same // topic — events fan out to every subscriber. +// +// No production code calls Subscribe today (only this package's tests), so +// subs is always empty at runtime and Dispatch's loop never iterates. The +// first caller added here turns Dispatch's loop live on the hub's broadcast +// path — see the SECURITY GATE comment on Dispatch before adding one. func (s *EventSink) Subscribe(topic string, inst *Instance) error { if !inst.Manifest.HasCapability(CapEvents) { return ErrCapabilityNotGranted @@ -90,17 +95,28 @@ func (s *EventSink) UnsubscribeAll(inst *Instance) { // Dispatch invokes every subscriber's on_event for topic. // // SECURITY GATE (audit 2026-04-07 finding #4 — "no rate limit on event -// delivery to plugins"). Guest delivery is NOT implemented in either build: -// the loop below touches no module, and nothing in the server calls Dispatch, -// so a plugin cannot slow the hub by handling events slowly. Wiring the -// guest call is what makes the finding real, so whoever does it must land, in -// the same change: +// delivery to plugins"). Read this before adding anything to the loop below. +// +// Dispatch already has a production caller: ws/hub.go calls it on every +// broadcast message when an operator has enabled plugins (api/router.go wires +// h.pluginSink whenever the registry is non-nil). That call site runs on the +// hub's broadcast goroutine while seqMu is held, so anything this function +// does is on the hub's hot path and must not block or re-enter the hub. +// +// Guest delivery is nonetheless NOT implemented in either build: the loop +// below touches no module, and no production code calls Subscribe (only this +// package's tests), so subs is empty and the loop never iterates. No guest +// code executes on the event path today — that, not an absent call site, is +// why a plugin cannot currently slow the hub by handling events slowly. +// +// Wiring guest delivery is what makes the finding real, so whoever does it +// must land, in the same change: // // - a per-plugin delivery rate limit (drop, never block the caller), and // - the same per-call CPU-budget deadline invokeCommand applies // (sandbox_wazero.go), and // - delivery off the hub's broadcast goroutine so a slow guest cannot -// backpressure fan-out to WS clients. +// backpressure fan-out to WS clients or extend the seqMu hold. // // Until then this stays inert on purpose. func (s *EventSink) Dispatch(ctx context.Context, topic string, payload []byte) { diff --git a/docs/audit-2026-04-07.md b/docs/audit-2026-04-07.md index 4bf6fb7e..50ec17c5 100644 --- a/docs/audit-2026-04-07.md +++ b/docs/audit-2026-04-07.md @@ -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 | diff --git a/docs/audit-2026-07-19.md b/docs/audit-2026-07-19.md index ccece922..d4b60ecd 100644 --- a/docs/audit-2026-07-19.md +++ b/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) | **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 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 | +| 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` | diff --git a/docs/plans/audit-2026-07-19-decisions.md b/docs/plans/audit-2026-07-19-decisions.md index 543fc3e1..a5565677 100644 --- a/docs/plans/audit-2026-07-19-decisions.md +++ b/docs/plans/audit-2026-07-19-decisions.md @@ -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, ~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`). | +| 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