diff --git a/docs/audit-2026-04-07.md b/docs/audit-2026-04-07.md index 83195976..4bf6fb7e 100644 --- a/docs/audit-2026-04-07.md +++ b/docs/audit-2026-04-07.md @@ -5,20 +5,36 @@ --- -## 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*` / `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. | # | 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 (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 | +| 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 +389,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 | diff --git a/docs/audit-2026-07-19.md b/docs/audit-2026-07-19.md index 5cc37f12..ccece922 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) | 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 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 | | 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 261bb43d..543fc3e1 100644 --- a/docs/plans/audit-2026-07-19-decisions.md +++ b/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 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`). | ## Suggested sequencing diff --git a/docs/plans/slash-commands.md b/docs/plans/slash-commands.md index 0fa564d2..e1068026 100644 --- a/docs/plans/slash-commands.md +++ b/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",