docs(plans): remediation plan for security-hardening review regressions

Adds docs/plans/security-hardening-remediation.md capturing the code-review
findings against fix/security-hardening-review and sequencing the fixes into
three waves (availability/backend-breaking, behavioral regressions, cleanup).
Each item names root cause, right-altitude fix, files, and verification.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs/plans/security-hardening-remediation.md

@@ -0,0 +1,285 @@
+# Plan: Remediate security-hardening review regressions
+
+**Status:** design only, not implemented
+**Owner:** TBD
+**Tracks:** code review of branch `fix/security-hardening-review` (2026-07-17)
+**Estimated effort:** 2–4 focused days
+
+## Why
+
+The `fix/security-hardening-review` branch lands a broad, well-intentioned
+security pass (login timing fix, TOTP TOCTOU, X-Forwarded-For spoofing,
+LiveKit webhook signature binding, plugin SSRF/DNS-rebinding, WASM CPU
+budgets, attachment IDOR, ban authorization, WAF chunked-body inspection,
+update-binary re-verification, and several rate limits). A recall-oriented
+code review confirmed that most of it is sound, but that several of the
+hardening changes introduced new correctness/availability regressions —
+verified against the source, not just the diff.
+
+This plan sequences the fixes. It is ordered by severity: availability- and
+backend-breaking bugs first, then behavioral regressions, then cleanup. Each
+item names the root cause, the fix at the right altitude, the files, and the
+verification that must pass. Every new fix ships with tests — several of the
+regressions below slipped through precisely because the new security code had
+no coverage (repo rule: "Target 80%+ coverage; TDD is the expected workflow").
+
+## Non-goals
+
+- Not a redesign of the plugin runtime, the rate limiter, or the permission
+  model — each fix stays local and generalizes only where a bandaid would
+  otherwise recur.
+- Not reverting the hardening. The security intent of every change is kept;
+  only the regressions are corrected.
+- Not implementing the Postgres backend beyond what is needed to stop the
+  attachment path from hard-erroring (see W1-3).
+
+## Wave 1 — Availability & backend-breaking (HIGH)
+
+### W1-1. Plugin CPU budget must not permanently brick the module
+- **File:** `Server/plugin/sandbox_wazero.go` (~line 224, `invokeCommand`)
+- **Root cause:** the runtime is built `WithCloseOnContextDone(true)`
+  (line 72). The new per-call `context.WithTimeout` wraps `allocate`,
+  `command_dispatch`, and `deallocate`, so an expired deadline *closes the
+  module*. `inst.module` is only cleared by `platformDeactivate`, so nothing
+  re-instantiates it — one over-budget command bricks the plugin for all
+  users until admin disable/enable or restart. The budget is wall-clock,
+  floored at 100 ms, so any host HTTP call (`httpTimeout` = 10 s) trips it.
+- **Fix:** after a `context.DeadlineExceeded` overrun, mark the instance so
+  the next dispatch re-instantiates the module (re-run `activateWithRuntime`
+  lazily), or reset `inst.module = nil` and re-activate on demand. Separately,
+  scope the deadline to the guest CPU call only — do **not** count host-call
+  time (host functions should run under the parent ctx, or the budget clock
+  should pause during host calls). Reconsider the floor so legitimate work
+  isn't killed.
+- **Verify:** new test (build tag `wazero`) that (a) a command exceeding the
+  budget returns the budget error *and* a subsequent command on the same
+  plugin still succeeds; (b) a command performing a host HTTP call within
+  `httpTimeout` is not killed by the CPU budget.
+
+### W1-2. E2EE key rotation drops peers in 7+ participant calls
+- **Files:** `Server/ws/voice_e2ee.go` (~line 151);
+  `Client/tauri-client/src/lib/livekitSession.ts` (~lines 1317-1349)
+- **Root cause:** the `voice_e2ee_offer` limit is 5/sec, but the key holder
+  loops over every peer sending one offer each, back-to-back, with no pacing
+  and no retry on `RATE_LIMITED`. With 6+ peers, offers to the 6th+ peer are
+  rejected and silently dropped — those peers never get the rotated key and
+  their audio never decrypts. Fires on join/leave and the 5-minute periodic
+  rotation.
+- **Fix (choose one, prefer server-side):**
+  - Server: exempt the fan-out relay from the tight per-message cap — rate
+    limit the *rotation event* (one budget per rotation) rather than each
+    per-peer offer; or scale the limit to channel size.
+  - Client: add bounded pacing + retry/backoff on `RATE_LIMITED` so all peers
+    eventually receive the offer.
+  - Preferred: server distinguishes "offer burst that belongs to one
+    rotation" from spam, so a single user still can't spam unrelated offers.
+- **Verify:** integration test with 8 voice participants confirming every peer
+  receives the rotated key after a join/leave and after a periodic rotation.
+
+### W1-3. Attachment-ownership check breaks Postgres and isn't atomic
+- **Files:** `Server/service/message.go` (~line 183);
+  `Server/db/queries/*attachment*.sql` + regenerate `dbgen`/`pgdbgen`;
+  `Server/store/postgres.go`, `Server/store/sqlite.go`
+- **Root cause:** the new per-attachment `GetAttachmentByID` loop (a) calls a
+  method that returns `ErrPostgresNotImplemented` on `PostgresStore`
+  (`postgres.go:498`), so every attachment-bearing `SendMessage` hard-errors
+  on Postgres; (b) is a check-then-link TOCTOU (the race pattern this PR fixes
+  elsewhere); (c) is an N+1 query on the hot send path; (d) rejects legacy
+  `uploader_id IS NULL` rows and already-linked attachments on retry (W2-4).
+- **Fix (right altitude):** delete the loop. Enforce ownership atomically in
+  the shared link query — add `AND uploader_id = ?` (and keep
+  `AND message_id IS NULL`) to `LinkAttachmentsToMessage`, and return
+  `ErrForbidden` when `RowsAffected != len(AttachmentIDs)`. Edit the SQL in
+  `Server/db/queries/` and run `make sqlc-generate` (never hand-edit `dbgen`/
+  `pgdbgen`); update the SQLite + Postgres migrations as a pair. This makes
+  the check atomic, one query, and backend-agnostic, and removes the need for
+  the `MemStore.GetAttachmentByID` `(nil,nil)` contortion.
+- **Verify:** service tests (SQLite *and* a Postgres path or a store fake that
+  implements the link semantics) covering: own unlinked attachment links;
+  another user's attachment is refused; nonexistent id is skipped; already
+  linked id is refused; `RowsAffected` mismatch → no message persisted.
+
+### W1-4. Ban authorization guards dead code
+- **Files:** `Server/admin/handlers_users.go` (`handlePatchUser`, ~line 112);
+  `Server/service/moderation.go`
+- **Root cause:** `requireBanAuthority` (BAN_MEMBERS + role hierarchy) is
+  wired only into `ModerationService.BanUser`/`UnbanUser`, which have no
+  production callers. The live ban path is `handlePatchUser`, which runs
+  `UPDATE users SET banned = 1 ...` directly with no BAN_MEMBERS/hierarchy
+  check, so any admin-panel actor can ban an equal- or higher-ranked user
+  (including the owner).
+- **Fix:** route `handlePatchUser`'s ban/unban branch through
+  `ModerationService.BanUser`/`UnbanUser` (so the new authorization actually
+  runs), or lift `requireBanAuthority` into the handler. Keep the
+  admin-IP/admin-auth perimeter; add the permission + hierarchy check on top.
+  Move the target-existence check *after* authorization so a caller without
+  BAN_MEMBERS can't enumerate user ids via NotFound-vs-Forbidden.
+- **Verify:** handler test — actor without BAN_MEMBERS is refused; actor of
+  equal/lower rank than target is refused; owner-rank target can't be banned
+  by a lower rank; authorized actor succeeds.
+
+## Wave 2 — Behavioral regressions (MED-HIGH → MED)
+
+### W2-1. Client-update rate limiter shares the auth bucket
+- **File:** `Server/api/router.go` (~line 257)
+- **Root cause:** it uses the empty-prefix `RateLimitMiddleware` on the shared
+  `limiter`, colliding per-IP with `verifyTOTP`, the sensitive endpoints, and
+  profile/password. A 30/min auto-poll drains the shared per-IP budget →
+  spurious 429s on 2FA and password change.
+- **Fix:** give the client-update route a dedicated key prefix via
+  `rateLimitMiddlewareWithPrefix(limiter, "client_update:", ...)`, mirroring
+  the LiveKit proxy's `"livekit_proxy:"` prefix (`router.go:192`).
+- **Verify:** test that hammering `/client-update` to its limit does not 429 a
+  subsequent `verify-totp`/password request from the same IP.
+
+### W2-2. ChangePassword reports failure after the password is committed
+- **Files:** `Server/service/user.go` (~line 60); caller
+  `Server/api/profile_handler.go` (~line 231)
+- **Root cause:** `UpdateUserPassword` commits first; if `DeleteOtherSessions`
+  then errors, the function returns `ErrInternal`, the caller emits 500, and
+  the `password_change` audit entry is skipped. The user is told it failed
+  while the new password is live; retrying with the old password fails and can
+  trip the password-confirm lockout.
+- **Fix:** treat session-revocation failure as a partial success, not a total
+  failure. Log + audit the password change, return success with a
+  `sessions_revoked`/warning signal the client can surface, or perform a
+  bounded compensating retry of `DeleteOtherSessions`. Do not report a 5xx for
+  an already-committed change; always write the audit row.
+- **Verify:** test that when `DeleteOtherSessions` errors, the audit row is
+  still written and the handler does not return a 5xx that implies the
+  password is unchanged.
+
+### W2-3. Plugin activation via RegisterCommand breaks in-place upgrades
+- **Files:** `Server/plugin/sandbox_wazero.go` (~line 140);
+  `Server/plugin/host_commands.go` (~line 36);
+  `Server/plugin/registry.go` (`installFromDisk`, `InstallFromZip`)
+- **Root cause:** `RegisterCommand` refuses when `existing != inst` by
+  *pointer*, but `installFromDisk` replaces `r.plugins[id]`/`r.byName` with a
+  fresh `*Instance` without clearing the old command bindings. Re-installing an
+  enabled plugin leaves stale bindings that block re-registration; dispatch
+  keeps routing to the orphaned old module until restart. The old
+  `r.commands[cmd] = inst` overwrote unconditionally.
+- **Fix:** compare ownership by plugin identity (name/id), not instance
+  pointer — allow the same plugin to re-bind its own command — and/or clear a
+  plugin's stale command bindings during reinstall/deactivation before
+  re-activation. Preserve the cross-plugin hijack protection (a *different*
+  plugin still can't claim an owned command).
+- **Verify:** test that upgrading an enabled plugin in place rebinds its
+  commands and dispatch routes to the new module; a different plugin claiming
+  an owned command is still refused.
+
+### W2-4. Attachment check rejects legit retries and legacy uploads
+- **File:** `Server/service/message.go` (~line 191)
+- **Root cause:** `att.MessageID != nil → ErrForbidden` means a client retry of
+  a send whose first attempt already linked the attachment can never succeed;
+  the same branch rejects legacy `uploader_id IS NULL` rows.
+- **Fix:** subsumed by W1-3 — the atomic link UPDATE skips already-linked/
+  non-owned rows instead of failing the whole send. Decide explicit policy for
+  legacy NULL-uploader rows (migrate/backfill vs. treat as unowned).
+- **Verify:** covered by W1-3 tests (already-linked id → skipped, not fatal).
+
+### W2-5. XFF right-to-left walk collapses/spoofs on broad trusted CIDRs
+- **File:** `Server/api/middleware.go` (~line 227, `clientIPWithProxies`)
+- **Root cause:** the walk skips every entry inside `trustedCIDRs`. With a
+  broad config (e.g. `trusted_proxies: 10.0.0.0/8` covering LAN clients), the
+  real client entry is skipped, the loop exhausts, and it falls back to the
+  proxy's own `RemoteAddr` — collapsing all clients into one lockout bucket
+  (one user's failed logins lock out everyone), or letting a client at a
+  trusted IP forge the key.
+- **Fix:** when the walk exhausts without a non-trusted candidate, return the
+  left-most *valid* XFF entry (the furthest-upstream client) rather than
+  `RemoteAddr`, so distinct clients keep distinct keys. Document that
+  `trusted_proxies` should list only proxy hops, and validate config on
+  startup. Pre-parse `trustedCIDRs` into `[]*net.IPNet` once (see W3-3).
+- **Verify:** test with `trusted_proxies=10.0.0.0/8`, proxy at `10.0.0.2`,
+  two LAN clients `10.5.1.7` / `10.5.1.8` behind it → distinct keys; a spoofed
+  leftmost entry from an untrusted RemoteAddr is ignored.
+
+### W2-6. SSRF-hardened dialer loses multi-address fallback
+- **File:** `Server/plugin/host_http.go` (~line 115)
+- **Root cause:** after validating every resolved IP, it dials only `ips[0]`,
+  dropping Happy-Eyeballs/next-record fallback. An allowlisted dual-stack or
+  round-robin host whose first record is down now hard-fails.
+- **Fix:** loop over the vetted IPs and try each until one connects, keeping
+  the "dial only vetted concrete IPs" property. Also remove the redundant
+  `rejectPrivateAddrs` pre-resolve at line 68 (keep the cheap `hostAllowed`
+  allowlist) since the dial-time resolve+validate is authoritative — saves a
+  second DNS round trip (W3-2).
+- **Verify:** test that a host resolving to `[unreachable, reachable]` still
+  connects via the reachable vetted address; a host resolving to a private IP
+  is still refused.
+
+### W2-7. Plugin-broadcast gate omits the block check
+- **Files:** `Server/ws/handlers_command.go` (~line 107);
+  `Server/permissions/checker.go` (~line 79)
+- **Root cause:** `requireChannelBroadcastAccess` routes through
+  `RequireChannelAccess`, whose DM branch checks only participant membership,
+  while the real send path (`checkSendPermission`) also enforces
+  `IsEitherBlocked`. A blocked user's plugin broadcast can reach the person who
+  blocked them. It also issues a raw `GetRoleByID` per broadcast, bypassing the
+  service-layer permission cache.
+- **Fix:** route the broadcast gate through the shared service-layer send check
+  (`MessageService.checkSendPermission` or an extracted equivalent) so DM
+  block, slow-mode, and future policy apply uniformly and the perm cache is
+  used. Avoids a fourth hand-rolled permission helper on `Hub`.
+- **Verify:** test that a blocked user's plugin broadcast into a DM is refused.
+
+## Wave 3 — Cleanup, efficiency, hardening depth (LOW-MED)
+
+### W3-1. Updater text-asset cache: add coalescing + negative caching
+- **File:** `Server/updater/updater.go` (~line 729, `FetchTextAssetCached`)
+- **Fix:** guard refresh with `golang.org/x/sync/singleflight` so a TTL-expiry
+  burst issues one outbound fetch; briefly cache errors so an upstream outage
+  doesn't re-fetch on every request. Evict superseded keys.
+- **Verify:** concurrent cold-cache test issues exactly one upstream fetch.
+
+### W3-2. De-duplicate the update binary hashing
+- **File:** `Server/admin/update_handlers.go` (~line 166, `fileSHA256`)
+- **Fix:** `fileSHA256` duplicates `updater.VerifyChecksum`'s hashing body and
+  re-reads the just-verified binary. Export one hashing helper from the updater
+  package (or have `DownloadAndVerify` return the checksum it already computed)
+  and reuse it for the TOCTOU snapshot.
+
+### W3-3. Update TOCTOU guard depth + XFF CIDR pre-parsing
+- **Files:** `Server/admin/update_handlers.go` (~line 117);
+  `Server/api/middleware.go` (`isTrustedProxy`)
+- **Fix:** the re-verify narrows but does not close the swap window (verify by
+  path, then rename+spawn by path). Real closure needs fd-based verification or
+  `O_EXCL` staging in the updater package. Separately, `isTrustedProxy`
+  re-parses every CIDR string per call on the request hot path — pre-parse into
+  `[]*net.IPNet` once at middleware construction.
+
+### W3-4. Cache-Control header contradiction
+- **File:** `Server/api/upload_handler.go` (~line 309) + test at
+  `upload_handler_test.go` (~line 733)
+- **Fix:** `private, max-age=31536000, no-cache` is self-contradictory —
+  `no-cache` forces revalidation, so the year-long `max-age` is dead weight.
+  Use `private, no-cache` and update the test assertion.
+
+### W3-5. Restore test coverage lost to the MemStore change
+- **File:** `Server/store/memstore.go` (~line 693)
+- **Fix:** subsumed by W1-3 (atomic link removes the need for the `(nil,nil)`
+  stub). If MemStore keeps attachment stubs, ensure the ownership behavior is
+  covered by a store that actually tracks `uploader_id`/`message_id` so the
+  IDOR guard cannot silently regress in tests.
+
+## Sequencing
+
+1. **W1** first (availability/backend-breaking). W1-3 unblocks W2-4 and W3-5.
+2. **W2** next. W2-5 pairs with the CIDR pre-parse in W3-3. W2-6 pairs with the
+   redundant-resolve removal.
+3. **W3** last, or fold each item into the related Wave-1/2 change.
+
+## Cross-cutting requirements
+
+- **Tests:** every fix ships with tests (repo rule: 80%+ coverage, TDD). Add
+  the missing coverage for the *existing* new security code too:
+  `requireBanAuthority`, `FetchTextAssetCached`, `requireChannelBroadcastAccess`,
+  fail-closed `DecryptTOTPSecret`.
+- **Build-tag matrix:** W1-1/W2-3 touch `//go:build wazero` code — verify the
+  default, `otel`, `wazero`, and `otel,wazero` variants all still build.
+- **Two backends:** W1-3 changes queries — edit `Server/db/queries/` + both
+  migration trees and run `make sqlc-generate`; do not hand-edit `dbgen`/
+  `pgdbgen`. CI runs `make sqlc-verify`.
+- **CI gates:** `go test -race`, `-tags deadlock`, `golangci-lint run`,
+  `govulncheck ./...` must pass before merge.