From 2b2d58abc133d9ed05b2953b076a2cfa99261903 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Sat, 29 Aug 2026 11:11:37 +0200 Subject: [PATCH] feat(b2-6): safe audit coverage (S-02) (#1441) * docs(b2-6): enumerate the security-sensitive mutations and their audit coverage Step 1 of B2-6: the mutation inventory crossed with the 43 non-test Audit( call sites at 67fdd18d, recorded in the plan's evidence block. Invite create/revoke (S-02) and plugin install/uninstall have no audit row; no timeout mutation exists (kick is force_logout / voice_mod_kick). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * added new doc * docs(audit): update changelog and security documentation to include admin panel actions in audit log * feat(b2-6): audit every security-sensitive mutation, invite and plugin rows first (S-02) Step 2 of B2-6. TestAuditCoverage_* in service, api and admin drive each mutation from the plan's inventory against a fake db.AuditStore (Server/db/audittest) and assert the expected action arrives. Red before this commit on exactly four rows: invite_create, invite_revoke, plugin_install, plugin_uninstall. - InviteService writes invite_create / invite_revoke naming the invite by id, never by code; RevokeInvite now takes the actor, threaded from the handler. A failed revoke writes nothing (test). - The plugin admin handler takes a db.Auditor and writes plugin_install / plugin_uninstall against the RequireAdminAuth principal (admin.ActorIDFromContext, exported for that). - docs/security.md lists the four new actions; CHANGELOG under Unreleased. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * test(b2-6): denylist over the recorded audit detail corpus Step 3 of B2-6. Each TestAuditCoverage_* table now ends with a subtest that runs audittest.AssertSafeDetails over every entry its rows recorded: a shape denylist (bcrypt/argon2 hashes, password=/token=/secret=/ recovery-code key-value leaks, otpauth URIs, Bearer credentials) plus the fixture's own secrets (raw tokens, passwords and hashes, TOTP secrets and codes, invite codes, message bodies). audittest_test.go proves each class bites and that ordinary details pass. Zero hits on the corpus at HEAD, so no call site needed changing. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * docs(b2-6): record pre-squash SHAs, red/green and denylist evidence Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * fix(b2-6): audit committed invites on a canceled request; 404 unknown plugin uninstall Codex P2s on #1441, both test-first: - CreateInvite read the invite back on the request context, so a cancel after the insert committed returned an error and skipped invite_create. The read-back and audit now run on context.WithoutCancel, like the password-change tail. TestCreateInvite_AuditSurvivesCanceledLookup. - Registry.UninstallPlugin is idempotent on an unknown id, so the handler wrote plugin_uninstall for plugins that never existed. It now checks the row first: 404 and no audit. TestPluginsHandlerUninstallUnknownID. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu * docs(b2-6): record the Codex review outcome on #1441 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu --------- Co-authored-by: Claude Fable 5 --- CHANGELOG.md | 6 + Server/admin/audit_coverage_test.go | 171 +++++++++++++++ Server/admin/helpers.go | 11 +- Server/api/audit_coverage_plugin_test.go | 101 +++++++++ Server/api/audit_coverage_test.go | 104 +++++++++ Server/api/invite_handler.go | 9 +- Server/api/plugins_handler.go | 50 ++++- Server/api/plugins_handler_test.go | 20 +- Server/api/router.go | 2 +- Server/db/audittest/audittest.go | 112 ++++++++++ Server/db/audittest/audittest_test.go | 47 +++++ Server/service/audit_coverage_test.go | 198 ++++++++++++++++++ Server/service/invite.go | 14 +- .../b2-protocol-trust-compat-2026-08-28.md | 82 +++++++- .../repo-health-issue-register-2026-08-23.md | 38 ++-- docs/security.md | 2 +- 16 files changed, 928 insertions(+), 39 deletions(-) create mode 100644 Server/admin/audit_coverage_test.go create mode 100644 Server/api/audit_coverage_plugin_test.go create mode 100644 Server/api/audit_coverage_test.go create mode 100644 Server/db/audittest/audittest.go create mode 100644 Server/db/audittest/audittest_test.go create mode 100644 Server/service/audit_coverage_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d2360396..98d47cec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,12 @@ protocol now carries a version number. reaches clients once the server runs it. Releases that do not change the protocol are offered as before. +### Admin panel + +- Creating or revoking an invite, and installing or uninstalling a plugin, now + show up in the audit log. Invite entries name the invite by id, never by + code. + ### Repository - `protocol/schema.json` declares `protocol_epoch`; `npm run generate` emits it diff --git a/Server/admin/audit_coverage_test.go b/Server/admin/audit_coverage_test.go new file mode 100644 index 00000000..46c666a9 --- /dev/null +++ b/Server/admin/audit_coverage_test.go @@ -0,0 +1,171 @@ +package admin_test + +import ( + "context" + "encoding/json" + "net/http" + "path/filepath" + "testing" + + "github.com/J3vb/OwnCord/Server/admin" + "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/db/audittest" + "github.com/J3vb/OwnCord/Server/permissions" +) + +// TestAuditCoverage_AdminMutations is the B2-6 audit table for the +// admin-owned security-sensitive mutations: channel permission edits (role +// and user layer), API-token create/revoke, settings changes and the setup +// wizard's config write. The closing subtest runs the detail denylist over +// the recorded corpus (plan docs/plans/b2-protocol-trust-compat-2026-08-28.md +// § B2-6). +func TestAuditCoverage_AdminMutations(t *testing.T) { + + // fixture returns a handler, an owner token and a channel id, with the + // recorder installed after seeding. + fixture := func(t *testing.T) (http.Handler, *db.DB, string, int64) { + t.Helper() + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, &mockPermInvalidator{}, + newTestModService(database), newTestRoleService(database)) + token := createAdminUser(t, database) + chID, err := database.CreateChannel(context.Background(), "secret", "text", "", "", 0) + if err != nil { + t.Fatalf("CreateChannel: %v", err) + } + return handler, database, token, chID + } + rows := []struct { + name string + action string + run func(t *testing.T) (*audittest.Recorder, []string) + }{ + {"channel role perms set", "channel_perms_update", func(t *testing.T) (*audittest.Recorder, []string) { + handler, database, token, chID := fixture(t) + rec := audittest.Install(t, database) + w := doRequest(t, handler, http.MethodPut, "/channels/"+itoa(chID)+"/permissions/3", token, + map[string]any{"allow": 0, "deny": permissions.ReadMessages}) + if w.Code != http.StatusOK { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + return rec, nil + }}, + {"channel role perms clear", "channel_perms_clear", func(t *testing.T) (*audittest.Recorder, []string) { + handler, database, token, chID := fixture(t) + if w := doRequest(t, handler, http.MethodPut, "/channels/"+itoa(chID)+"/permissions/3", token, + map[string]any{"allow": 0, "deny": permissions.ReadMessages}); w.Code != http.StatusOK { + t.Fatalf("seed override: status = %d; body = %s", w.Code, w.Body.String()) + } + rec := audittest.Install(t, database) + w := doRequest(t, handler, http.MethodDelete, "/channels/"+itoa(chID)+"/permissions/3", token, nil) + if w.Code != http.StatusNoContent && w.Code != http.StatusOK { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + return rec, nil + }}, + {"channel user perms set", "channel_user_perms_update", func(t *testing.T) (*audittest.Recorder, []string) { + handler, database, token, chID := fixture(t) + target := seedOverrideTarget(t, database, "override-target") + rec := audittest.Install(t, database) + w := doRequest(t, handler, http.MethodPut, "/channels/"+itoa(chID)+"/user-permissions/"+itoa(target), token, + map[string]any{"allow": permissions.ReadMessages, "deny": permissions.SendMessages}) + if w.Code != http.StatusOK { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + return rec, nil + }}, + {"channel user perms clear", "channel_user_perms_clear", func(t *testing.T) (*audittest.Recorder, []string) { + handler, database, token, chID := fixture(t) + target := seedOverrideTarget(t, database, "override-target") + if w := doRequest(t, handler, http.MethodPut, "/channels/"+itoa(chID)+"/user-permissions/"+itoa(target), token, + map[string]any{"allow": permissions.ReadMessages, "deny": permissions.SendMessages}); w.Code != http.StatusOK { + t.Fatalf("seed override: status = %d; body = %s", w.Code, w.Body.String()) + } + rec := audittest.Install(t, database) + w := doRequest(t, handler, http.MethodDelete, "/channels/"+itoa(chID)+"/user-permissions/"+itoa(target), token, nil) + if w.Code != http.StatusNoContent && w.Code != http.StatusOK { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + return rec, nil + }}, + {"api token create", "api_token_create", func(t *testing.T) (*audittest.Recorder, []string) { + handler, database, token, _ := fixture(t) + rec := audittest.Install(t, database) + w := doRequest(t, handler, http.MethodPost, "/tokens", token, + map[string]any{"label": "ci bot", "username": "adminuser"}) + if w.Code != http.StatusCreated { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + var resp struct { + Token string `json:"token"` + } + _ = json.Unmarshal(w.Body.Bytes(), &resp) + return rec, []string{resp.Token, token} + }}, + {"api token revoke", "api_token_revoke", func(t *testing.T) (*audittest.Recorder, []string) { + handler, database, token, _ := fixture(t) + w := doRequest(t, handler, http.MethodPost, "/tokens", token, + map[string]any{"label": "ci bot", "username": "adminuser"}) + if w.Code != http.StatusCreated { + t.Fatalf("seed token: status = %d; body = %s", w.Code, w.Body.String()) + } + var resp struct { + ID int64 `json:"id"` + Token string `json:"token"` + } + _ = json.Unmarshal(w.Body.Bytes(), &resp) + rec := audittest.Install(t, database) + if w := doRequest(t, handler, http.MethodDelete, "/tokens/"+itoa(resp.ID), token, nil); w.Code != http.StatusNoContent { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + return rec, []string{resp.Token, token} + }}, + {"setting change", "setting_change", func(t *testing.T) (*audittest.Recorder, []string) { + handler, database, token, _ := fixture(t) + rec := audittest.Install(t, database) + w := doRequest(t, handler, http.MethodPatch, "/settings", token, + map[string]string{"motd": "welcome 4d0d1405"}) + if w.Code != http.StatusOK { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + return rec, []string{"welcome 4d0d1405", token} + }}, + {"config write (setup wizard)", "config_write", func(t *testing.T) (*audittest.Recorder, []string) { + database := openAdminTestDB(t) + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + handler := wizardHandler(t, database, cfgPath, make(chan string, 1)) + rec := audittest.Install(t, database) + const password = "SecurePass123!" + w := doRequest(t, handler, http.MethodPost, "/setup", "", map[string]any{ + "username": "owner", + "password": password, + "wizard": map[string]any{"server_name": "Audit Server"}, + }) + if w.Code != http.StatusCreated { + t.Fatalf("status = %d; body = %s", w.Code, w.Body.String()) + } + var resp struct { + Token string `json:"token"` + } + _ = json.Unmarshal(w.Body.Bytes(), &resp) + return rec, []string{password, resp.Token} + }}, + } + + var corpus []db.AuditEntry + var secrets []string + for _, row := range rows { + t.Run(row.name, func(t *testing.T) { + rec, s := row.run(t) + rec.Wait(t, row.action) + corpus = append(corpus, rec.Entries()...) + secrets = append(secrets, s...) + }) + } + t.Run("detail denylist", func(t *testing.T) { + if len(corpus) == 0 { + t.Fatal("no audit entries recorded") + } + audittest.AssertSafeDetails(t, corpus, secrets...) + }) +} diff --git a/Server/admin/helpers.go b/Server/admin/helpers.go index 06c2b4e2..0e2f6712 100644 --- a/Server/admin/helpers.go +++ b/Server/admin/helpers.go @@ -1,6 +1,7 @@ package admin import ( + "context" "encoding/json" "log/slog" "net/http" @@ -58,7 +59,15 @@ func queryInt(r *http.Request, key string, defaultVal, minVal, maxVal int) int { // context by adminAuthMiddleware. Returns 0 if called outside that middleware // (should not happen in production). func actorFromContext(r *http.Request) int64 { - user, ok := r.Context().Value(adminUserKey).(*db.User) + return ActorIDFromContext(r.Context()) +} + +// ActorIDFromContext returns the admin principal's user ID that +// RequireAdminAuth stored in ctx, or 0 outside that middleware. Exported for +// handlers mounted behind RequireAdminAuth from other packages (the plugin +// admin surface in api) so their audit rows name the real actor. +func ActorIDFromContext(ctx context.Context) int64 { + user, ok := ctx.Value(adminUserKey).(*db.User) if !ok || user == nil { return 0 } diff --git a/Server/api/audit_coverage_plugin_test.go b/Server/api/audit_coverage_plugin_test.go new file mode 100644 index 00000000..d315b3bf --- /dev/null +++ b/Server/api/audit_coverage_plugin_test.go @@ -0,0 +1,101 @@ +package api + +import ( + "context" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/db/audittest" +) + +// TestAuditCoverage_PluginLifecycle is the plugin half of the B2-6 audit +// table: install and uninstall each emit an audit entry, and neither detail +// carries anything from the archive beyond the plugin name. +func TestAuditCoverage_PluginLifecycle(t *testing.T) { + install := func(t *testing.T) (http.Handler, *db.DB, int64) { + t.Helper() + reg, mem := newTestPluginRegistryWithStore(t) + h := NewPluginAdminHandler(reg, mem, mem) + body, contentType := buildZipUpload(t, validPluginZip(t)) + req := httptest.NewRequest("POST", "/install", body) + req.Header.Set("Content-Type", contentType) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusCreated { + t.Fatalf("install: status = %d; body = %s", rec.Code, rec.Body.String()) + } + row, err := mem.GetPluginByName(context.Background(), "hello") + if err != nil || row == nil { + t.Fatalf("GetPluginByName: %v", err) + } + return h, mem, row.ID + } + + rows := []struct { + name string + action string + run func(t *testing.T) *audittest.Recorder + }{ + {"plugin install", "plugin_install", func(t *testing.T) *audittest.Recorder { + reg, mem := newTestPluginRegistryWithStore(t) + h := NewPluginAdminHandler(reg, mem, mem) + rec := audittest.Install(t, mem) + body, contentType := buildZipUpload(t, validPluginZip(t)) + req := httptest.NewRequest("POST", "/install", body) + req.Header.Set("Content-Type", contentType) + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("install: status = %d; body = %s", w.Code, w.Body.String()) + } + return rec + }}, + {"plugin uninstall", "plugin_uninstall", func(t *testing.T) *audittest.Recorder { + h, mem, id := install(t) + rec := audittest.Install(t, mem) + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("DELETE", "/"+strconv.FormatInt(id, 10), nil)) + if w.Code != http.StatusNoContent { + t.Fatalf("uninstall: status = %d; body = %s", w.Code, w.Body.String()) + } + return rec + }}, + } + + var corpus []db.AuditEntry + for _, row := range rows { + t.Run(row.name, func(t *testing.T) { + rec := row.run(t) + rec.Wait(t, row.action) + corpus = append(corpus, rec.Entries()...) + }) + } + t.Run("detail denylist", func(t *testing.T) { + if len(corpus) == 0 { + t.Fatal("no audit entries recorded") + } + audittest.AssertSafeDetails(t, corpus) + }) +} + +// TestPluginsHandlerUninstallUnknownID pins Codex's P2 on #1441: the registry +// treats an unknown id as an idempotent no-op, so the handler must answer 404 +// and write no plugin_uninstall row for a plugin that never existed. +func TestPluginsHandlerUninstallUnknownID(t *testing.T) { + reg, mem := newTestPluginRegistryWithStore(t) + h := NewPluginAdminHandler(reg, mem, mem) + rec := audittest.Install(t, mem) + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest("DELETE", "/999", nil)) + if w.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404; body = %s", w.Code, w.Body.String()) + } + time.Sleep(20 * time.Millisecond) + if got := rec.Entries(); len(got) != 0 { + t.Fatalf("unknown plugin must not audit; got %v", got) + } +} diff --git a/Server/api/audit_coverage_test.go b/Server/api/audit_coverage_test.go new file mode 100644 index 00000000..28082d86 --- /dev/null +++ b/Server/api/audit_coverage_test.go @@ -0,0 +1,104 @@ +package api_test + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + "github.com/J3vb/OwnCord/Server/auth" + "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/db/audittest" +) + +// TestAuditCoverage_APIMutations is the B2-6 audit table for the +// api-owned security-sensitive mutations (TOTP enrolment and removal, +// account self-deletion). The plugin lifecycle rows live in +// audit_coverage_plugin_test.go because their fixtures are package-internal. +// The closing subtest runs the detail denylist over the recorded corpus +// (plan docs/plans/b2-protocol-trust-compat-2026-08-28.md § B2-6). +func TestAuditCoverage_APIMutations(t *testing.T) { + const password = "Password1!" + + // enrolTOTP runs enable+confirm for token and returns the TOTP secret + // and the confirmation code, both fixture secrets for the denylist. + enrolTOTP := func(t *testing.T, router http.Handler, token string) (secret, code string) { + t.Helper() + rr := postJSONWithToken(t, router, "/api/v1/users/me/totp/enable", token, + map[string]string{"password": password}) + if rr.Code != http.StatusOK { + t.Fatalf("enable: status = %d; body = %s", rr.Code, rr.Body.String()) + } + var enableResp map[string]any + _ = json.NewDecoder(rr.Body).Decode(&enableResp) + secret = extractSecretFromURI(t, enableResp["qr_uri"].(string)) + code, _ = auth.GenerateTOTPCode(secret, time.Now().UTC()) + rr = postJSONWithToken(t, router, "/api/v1/users/me/totp/confirm", token, + map[string]string{"password": password, "code": code}) + if rr.Code != http.StatusNoContent { + t.Fatalf("confirm: status = %d; body = %s", rr.Code, rr.Body.String()) + } + return secret, code + } + + rows := []struct { + name string + action string + run func(t *testing.T) (*audittest.Recorder, []string) + }{ + {"totp enable", "totp_enabled", func(t *testing.T) (*audittest.Recorder, []string) { + database := newAuthTestDB(t) + router := buildAuthRouter(database, auth.NewRateLimiter()) + token := loginAndGetToken(t, router, database, "totpenable", 4) + rec := audittest.Install(t, database) + secret, code := enrolTOTP(t, router, token) + return rec, []string{password, token, secret, code} + }}, + {"totp disable", "totp_disabled", func(t *testing.T) (*audittest.Recorder, []string) { + database := newAuthTestDB(t) + router := buildAuthRouter(database, auth.NewRateLimiter()) + token := loginAndGetToken(t, router, database, "totpdisable", 4) + secret, code := enrolTOTP(t, router, token) + rec := audittest.Install(t, database) + rr := deleteWithToken(t, router, "/api/v1/users/me/totp", token, + map[string]string{"password": password}) + if rr.Code != http.StatusNoContent { + t.Fatalf("disable: status = %d; body = %s", rr.Code, rr.Body.String()) + } + return rec, []string{password, token, secret, code} + }}, + {"account delete", "account_deleted", func(t *testing.T) (*audittest.Recorder, []string) { + database := newAuthTestDB(t) + router := buildAuthRouter(database, auth.NewRateLimiter()) + hash, _ := auth.HashPassword(password) + uid, _ := database.CreateUser(context.Background(), "selfdelete", hash, 4) + token, _ := auth.GenerateToken() + _, _ = database.CreateSession(context.Background(), uid, auth.HashToken(token), "test", "127.0.0.1") + rec := audittest.Install(t, database) + rr := deleteJSONWithToken(t, router, "/api/v1/auth/account", token, + map[string]string{"password": password}) + if rr.Code != http.StatusNoContent { + t.Fatalf("delete account: status = %d; body = %s", rr.Code, rr.Body.String()) + } + return rec, []string{password, hash, token} + }}, + } + + var corpus []db.AuditEntry + var secrets []string + for _, row := range rows { + t.Run(row.name, func(t *testing.T) { + rec, s := row.run(t) + rec.Wait(t, row.action) + corpus = append(corpus, rec.Entries()...) + secrets = append(secrets, s...) + }) + } + t.Run("detail denylist", func(t *testing.T) { + if len(corpus) == 0 { + t.Fatal("no audit entries recorded") + } + audittest.AssertSafeDetails(t, corpus, secrets...) + }) +} diff --git a/Server/api/invite_handler.go b/Server/api/invite_handler.go index abb5aa79..4edc18eb 100644 --- a/Server/api/invite_handler.go +++ b/Server/api/invite_handler.go @@ -102,8 +102,15 @@ func handleListInvites(svc *service.Services) http.HandlerFunc { // handleRevokeInvite processes DELETE /api/v1/invites/:code. func handleRevokeInvite(svc *service.Services) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { + user, ok := r.Context().Value(UserKey).(*db.User) + if !ok || user == nil { + writeJSON(w, http.StatusUnauthorized, errorResponse{ + Error: "UNAUTHORIZED", Message: "not authenticated", + }) + return + } code := chi.URLParam(r, "code") - if err := svc.Invites.RevokeInvite(r.Context(), code); err != nil { + if err := svc.Invites.RevokeInvite(r.Context(), user.ID, code); err != nil { writeServiceError(r.Context(), w, err) return } diff --git a/Server/api/plugins_handler.go b/Server/api/plugins_handler.go index 147481f0..659c91f5 100644 --- a/Server/api/plugins_handler.go +++ b/Server/api/plugins_handler.go @@ -7,12 +7,17 @@ package api import ( + "context" + "database/sql" + "errors" "io" "log/slog" "net/http" "strconv" "strings" + "github.com/J3vb/OwnCord/Server/admin" + "github.com/J3vb/OwnCord/Server/db" "github.com/J3vb/OwnCord/Server/plugin" "github.com/go-chi/chi/v5" ) @@ -26,13 +31,14 @@ const maxPluginUploadBytes = 16 * 1024 * 1024 type PluginAdminHandler struct { registry *plugin.Registry store plugin.PluginStore + audit db.Auditor // nil disables audit writes (unit tests) } // NewPluginAdminHandler builds an http.Handler that the router can mount. // Pass a nil registry when plugin support is disabled — the handler then // reports an empty list and 503 on lifecycle calls. -func NewPluginAdminHandler(registry *plugin.Registry, st plugin.PluginStore) http.Handler { - h := &PluginAdminHandler{registry: registry, store: st} +func NewPluginAdminHandler(registry *plugin.Registry, st plugin.PluginStore, audit db.Auditor) http.Handler { + h := &PluginAdminHandler{registry: registry, store: st, audit: audit} r := chi.NewRouter() r.Get("/", h.list) r.Post("/install", h.install) @@ -104,6 +110,7 @@ func (h *PluginAdminHandler) install(w http.ResponseWriter, r *http.Request) { }) return } + h.writeAudit(r, "plugin_install", h.installedID(r.Context(), name), name) writeJSON(w, http.StatusCreated, map[string]any{"name": name}) } @@ -170,14 +177,53 @@ func (h *PluginAdminHandler) uninstall(w http.ResponseWriter, r *http.Request) { http.Error(w, "plugin runtime disabled", http.StatusServiceUnavailable) return } + // Registry.UninstallPlugin is idempotent on an unknown id, so check the + // row here: a stale or repeated delete must answer 404 and must not + // record a plugin_uninstall that never happened. + if h.store != nil { + if _, err := h.store.GetPlugin(r.Context(), id); err != nil { + if errors.Is(err, sql.ErrNoRows) { + http.Error(w, "plugin not found", http.StatusNotFound) + return + } + slog.Error("plugin lookup failed", "id", id, "error", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } if err := h.registry.UninstallPlugin(r.Context(), id); err != nil { slog.Error("plugin uninstall failed", "id", id, "error", err) http.Error(w, "internal server error", http.StatusInternalServerError) return } + h.writeAudit(r, "plugin_uninstall", id, "") w.WriteHeader(http.StatusNoContent) } +// writeAudit records a plugin lifecycle mutation (B2-6) against the admin +// principal RequireAdminAuth put on the request. A nil auditor (unit tests +// that only exercise the HTTP surface) records nothing. +func (h *PluginAdminHandler) writeAudit(r *http.Request, action string, pluginID int64, detail string) { + if h.audit == nil { + return + } + db.WriteAudit(context.WithoutCancel(r.Context()), h.audit, admin.ActorIDFromContext(r.Context()), + action, "plugin", pluginID, detail) +} + +// installedID resolves a freshly installed plugin's row id for its audit +// entry; 0 when the store cannot answer (the name in detail still identifies it). +func (h *PluginAdminHandler) installedID(ctx context.Context, name string) int64 { + if h.store == nil { + return 0 + } + row, err := h.store.GetPluginByName(ctx, name) + if err != nil || row == nil { + return 0 + } + return row.ID +} + // pluginRuntimeState reports whether lifecycle calls will work, for the // X-Plugin-Runtime response header. A nil registry means plugin support is // compiled/configured off and every lifecycle endpoint answers 503. diff --git a/Server/api/plugins_handler_test.go b/Server/api/plugins_handler_test.go index 27fa3dd3..53abf4ca 100644 --- a/Server/api/plugins_handler_test.go +++ b/Server/api/plugins_handler_test.go @@ -40,7 +40,7 @@ func openPluginTestDB(t *testing.T) *db.DB { } func TestPluginsHandlerListEmptyWhenRegistryNil(t *testing.T) { - h := NewPluginAdminHandler(nil, nil) + h := NewPluginAdminHandler(nil, nil, nil) req := httptest.NewRequest("GET", "/", nil) rec := httptest.NewRecorder() h.ServeHTTP(rec, req) @@ -53,7 +53,7 @@ func TestPluginsHandlerListEmptyWhenRegistryNil(t *testing.T) { } func TestPluginsHandlerInstallRejectsWhenRegistryNil(t *testing.T) { - h := NewPluginAdminHandler(nil, nil) + h := NewPluginAdminHandler(nil, nil, nil) body, contentType := buildZipUpload(t, validPluginZip(t)) req := httptest.NewRequest("POST", "/install", body) req.Header.Set("Content-Type", contentType) @@ -66,7 +66,7 @@ func TestPluginsHandlerInstallRejectsWhenRegistryNil(t *testing.T) { func TestPluginsHandlerInstallRejectsNonZipContentType(t *testing.T) { reg := newTestPluginRegistry(t) - h := NewPluginAdminHandler(reg, nil) + h := NewPluginAdminHandler(reg, nil, nil) // Build a multipart body whose file part is labelled as text/plain. var buf bytes.Buffer @@ -94,7 +94,7 @@ func TestPluginsHandlerInstallRejectsNonZipContentType(t *testing.T) { func TestPluginsHandlerInstallRejectsNonZipMagic(t *testing.T) { reg := newTestPluginRegistry(t) - h := NewPluginAdminHandler(reg, nil) + h := NewPluginAdminHandler(reg, nil, nil) body, contentType := buildZipUpload(t, []byte("this is definitely not a zip")) req := httptest.NewRequest("POST", "/install", body) @@ -111,7 +111,7 @@ func TestPluginsHandlerInstallHappyPath(t *testing.T) { mem := openPluginTestDB(t) // Wire the store into the handler so /list can show the new row. The // registry already writes via its own PluginStore. - h := NewPluginAdminHandler(reg, mem) + h := NewPluginAdminHandler(reg, mem, nil) body, contentType := buildZipUpload(t, validPluginZip(t)) req := httptest.NewRequest("POST", "/install", body) req.Header.Set("Content-Type", contentType) @@ -128,14 +128,14 @@ func TestPluginsHandlerInstallHappyPath(t *testing.T) { // The admin panel's empty state distinguishes "no plugins installed" from // "the runtime is off", which it can only do from this header. func TestPluginsHandlerListReportsRuntimeState(t *testing.T) { - off := NewPluginAdminHandler(nil, nil) + off := NewPluginAdminHandler(nil, nil, nil) rec := httptest.NewRecorder() off.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil)) if got := rec.Header().Get("X-Plugin-Runtime"); got != "disabled" { t.Fatalf("nil registry: X-Plugin-Runtime = %q, want %q", got, "disabled") } - on := NewPluginAdminHandler(newTestPluginRegistry(t), openPluginTestDB(t)) + on := NewPluginAdminHandler(newTestPluginRegistry(t), openPluginTestDB(t), nil) rec = httptest.NewRecorder() on.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil)) if got := rec.Header().Get("X-Plugin-Runtime"); got != "enabled" { @@ -147,7 +147,7 @@ func TestPluginsHandlerListReportsRuntimeState(t *testing.T) { // Go field names and every column renders empty. func TestPluginsHandlerListUsesSnakeCaseJSON(t *testing.T) { reg, mem := newTestPluginRegistryWithStore(t) - h := NewPluginAdminHandler(reg, mem) + h := NewPluginAdminHandler(reg, mem, nil) body, contentType := buildZipUpload(t, validPluginZip(t)) req := httptest.NewRequest("POST", "/install", body) @@ -178,7 +178,7 @@ func TestPluginsHandlerListUsesSnakeCaseJSON(t *testing.T) { } func TestPluginsHandlerEnableDisableUninstallReturn503WhenRegistryNil(t *testing.T) { - h := NewPluginAdminHandler(nil, nil) + h := NewPluginAdminHandler(nil, nil, nil) for _, tc := range []struct{ method, path string }{ {"POST", "/1/enable"}, {"POST", "/1/disable"}, @@ -195,7 +195,7 @@ func TestPluginsHandlerEnableDisableUninstallReturn503WhenRegistryNil(t *testing func TestPluginsHandlerLifecycleInvalidID(t *testing.T) { reg := newTestPluginRegistry(t) - h := NewPluginAdminHandler(reg, nil) + h := NewPluginAdminHandler(reg, nil, nil) req := httptest.NewRequest("POST", "/not-an-int/enable", nil) rec := httptest.NewRecorder() h.ServeHTTP(rec, req) diff --git a/Server/api/router.go b/Server/api/router.go index 6631861e..a39b5da9 100644 --- a/Server/api/router.go +++ b/Server/api/router.go @@ -181,7 +181,7 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri // which case lifecycle calls return 503 and list returns []). r.Group(func(r chi.Router) { r.Use(admin.RequireAdminAuth(database)) - r.Mount("/api/v1/admin/plugins", NewPluginAdminHandler(pluginRegistry, database)) + r.Mount("/api/v1/admin/plugins", NewPluginAdminHandler(pluginRegistry, database, database)) }) }) diff --git a/Server/db/audittest/audittest.go b/Server/db/audittest/audittest.go new file mode 100644 index 00000000..caf9fe4f --- /dev/null +++ b/Server/db/audittest/audittest.go @@ -0,0 +1,112 @@ +// Package audittest captures audit entries through a fake db.AuditStore so a +// test can assert which actions a mutation emitted and what its detail +// carried (B2-6). Install swaps the DB's audit path for an in-memory writer +// for the test's lifetime; every db.WriteAudit routed through that *DB — +// from api, admin, service or ws — lands in the recorder. +package audittest + +import ( + "context" + "regexp" + "strings" + "sync" + "testing" + "time" + + "github.com/J3vb/OwnCord/Server/db" +) + +// Recorder is a db.AuditStore that keeps every persisted entry in memory. +type Recorder struct { + mu sync.Mutex + entries []db.AuditEntry +} + +// Install routes every audit write through d into a fresh Recorder until the +// test ends. Call it before the mutation under test, after the fixture is +// seeded (seeding may legitimately write audits you do not want to assert). +func Install(t testing.TB, d *db.DB) *Recorder { + t.Helper() + rec := &Recorder{} + // batchSize 1: each entry flushes as soon as the runner receives it. + w := db.NewAuditWriter(rec, 256, 1, time.Millisecond) + w.Start(context.Background()) + d.SetAuditWriter(w) + t.Cleanup(func() { + d.SetAuditWriter(nil) + w.Stop(context.Background()) + }) + return rec +} + +// PersistAudits implements db.AuditStore. +func (r *Recorder) PersistAudits(_ context.Context, entries []db.AuditEntry) (int, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.entries = append(r.entries, entries...) + return len(entries), nil +} + +// Entries returns a snapshot of everything recorded so far. +func (r *Recorder) Entries() []db.AuditEntry { + r.mu.Lock() + defer r.mu.Unlock() + return append([]db.AuditEntry(nil), r.entries...) +} + +// Wait returns the first recorded entry with the given action, polling until +// the asynchronous writer has flushed it. It fails the test after five +// seconds, listing the actions that did arrive. +func (r *Recorder) Wait(t testing.TB, action string) db.AuditEntry { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for { + var got []string + for _, e := range r.Entries() { + if e.Action == action { + return e + } + got = append(got, e.Action) + } + if time.Now().After(deadline) { + t.Fatalf("no %q audit entry recorded; recorded actions: %v", action, got) + return db.AuditEntry{} + } + time.Sleep(time.Millisecond) + } +} + +// detailDenylist is the shape half of the B2-6 denylist: patterns that a +// safe detail string never contains regardless of the fixture's values. +var detailDenylist = []*regexp.Regexp{ + regexp.MustCompile(`\$2[aby]\$\d\d\$`), // bcrypt hash + regexp.MustCompile(`\$argon2`), // argon2 hash + // key=value / key: value leaks of a credential field. + regexp.MustCompile(`(?i)\b(password|passwd|token|secret|recovery[_ ]?codes?|totp)\s*[:=]\s*\S`), + regexp.MustCompile(`(?i)^otpauth://`), + regexp.MustCompile(`(?i)\bBearer\s+\S`), +} + +// AssertSafeDetails fails the test if any recorded detail matches the shape +// denylist or contains one of the fixture's known secrets — the raw tokens, +// passwords and hashes, TOTP secrets, invite codes and message bodies the +// calling table used. Fix a hit at the audit call site, never by widening +// this list's exceptions. +func AssertSafeDetails(t testing.TB, entries []db.AuditEntry, secrets ...string) { + t.Helper() + for _, e := range entries { + for _, re := range detailDenylist { + if re.MatchString(e.Detail) { + t.Errorf("audit %q detail matches denylist %q: %q", e.Action, re, e.Detail) + } + } + for _, s := range secrets { + if s == "" { + continue + } + if strings.Contains(e.Detail, s) { + t.Errorf("audit %q detail carries a fixture secret: %q", e.Action, e.Detail) + } + } + } +} diff --git a/Server/db/audittest/audittest_test.go b/Server/db/audittest/audittest_test.go new file mode 100644 index 00000000..7d3fa38a --- /dev/null +++ b/Server/db/audittest/audittest_test.go @@ -0,0 +1,47 @@ +package audittest + +import ( + "testing" + + "github.com/J3vb/OwnCord/Server/db" +) + +// failCounter records Errorf calls so the denylist can be proven to bite. +type failCounter struct { + testing.TB + errs int +} + +func (f *failCounter) Helper() {} +func (f *failCounter) Errorf(string, ...any) { f.errs++ } +func (f *failCounter) Fatalf(s string, a ...any) { f.errs++ } + +// TestAssertSafeDetails_Bites proves each denylist class rejects a detail +// that carries it and that a plain detail passes — a denylist that cannot +// fail proves nothing about the corpus. +func TestAssertSafeDetails_Bites(t *testing.T) { + bad := []db.AuditEntry{ + {Action: "a", Detail: "hash $2a$12$abcdefghijklmnopqrstuv"}, + {Action: "b", Detail: "rotated password=hunter2"}, + {Action: "c", Detail: "Token: abc"}, + {Action: "d", Detail: "otpauth://totp/x?secret=Y"}, + {Action: "e", Detail: "Bearer eyJ"}, + {Action: "f", Detail: "message body: the fixture secret"}, + } + for _, e := range bad { + fc := &failCounter{TB: t} + AssertSafeDetails(fc, []db.AuditEntry{e}, "the fixture secret") + if fc.errs == 0 { + t.Errorf("denylist let %q through: %q", e.Action, e.Detail) + } + } + fc := &failCounter{TB: t} + AssertSafeDetails(fc, []db.AuditEntry{ + {Action: "ok", Detail: "password changed"}, + {Action: "ok", Detail: "max_uses=5 expires_in_hours=24"}, + {Action: "ok", Detail: "set overrides for role mod on #general (allow=0x1 deny=0x2)"}, + }, "the fixture secret") + if fc.errs != 0 { + t.Errorf("denylist rejected safe details: %d errors", fc.errs) + } +} diff --git a/Server/service/audit_coverage_test.go b/Server/service/audit_coverage_test.go new file mode 100644 index 00000000..dfc66f5e --- /dev/null +++ b/Server/service/audit_coverage_test.go @@ -0,0 +1,198 @@ +package service + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/J3vb/OwnCord/Server/auth" + "github.com/J3vb/OwnCord/Server/db" + "github.com/J3vb/OwnCord/Server/db/audittest" + "github.com/J3vb/OwnCord/Server/permissions" +) + +// TestAuditCoverage_ServiceMutations is the B2-6 audit table for the +// service-owned security-sensitive mutations: each row performs one mutation +// against a fake db.AuditStore and asserts the expected action arrives. The +// closing subtest runs the detail denylist over everything the rows recorded +// (plan docs/plans/b2-protocol-trust-compat-2026-08-28.md § B2-6). +func TestAuditCoverage_ServiceMutations(t *testing.T) { + rows := []struct { + name string + action string + // run seeds its fixture, installs the recorder, performs the mutation + // and returns the recorder plus every secret value the fixture used. + run func(t *testing.T) (*audittest.Recorder, []string) + }{ + {"invite create", "invite_create", func(t *testing.T) (*audittest.Recorder, []string) { + _, database := newTestModerationService(t) + rec := audittest.Install(t, database) + inv, err := NewInviteService(database).CreateInvite(context.Background(), 1, 5, 24) + if err != nil { + t.Fatalf("CreateInvite: %v", err) + } + return rec, []string{inv.Code} + }}, + {"invite revoke", "invite_revoke", func(t *testing.T) (*audittest.Recorder, []string) { + _, database := newTestModerationService(t) + svc := NewInviteService(database) + inv, err := svc.CreateInvite(context.Background(), 1, 5, 24) + if err != nil { + t.Fatalf("CreateInvite: %v", err) + } + rec := audittest.Install(t, database) + if err := svc.RevokeInvite(context.Background(), 1, inv.Code); err != nil { + t.Fatalf("RevokeInvite: %v", err) + } + return rec, []string{inv.Code} + }}, + {"ban", "user_ban", func(t *testing.T) (*audittest.Recorder, []string) { + svc, database := newTestModerationService(t) + rec := audittest.Install(t, database) + if err := svc.BanUser(context.Background(), 1, 4, "spam", nil); err != nil { + t.Fatalf("BanUser: %v", err) + } + return rec, nil + }}, + {"unban", "user_unban", func(t *testing.T) (*audittest.Recorder, []string) { + svc, database := newTestModerationService(t) + if err := svc.BanUser(context.Background(), 1, 4, "spam", nil); err != nil { + t.Fatalf("BanUser: %v", err) + } + rec := audittest.Install(t, database) + if err := svc.UnbanUser(context.Background(), 1, 4); err != nil { + t.Fatalf("UnbanUser: %v", err) + } + return rec, nil + }}, + {"kick (force logout)", "force_logout", func(t *testing.T) (*audittest.Recorder, []string) { + svc, database := newTestModerationService(t) + rec := audittest.Install(t, database) + if err := svc.ForceLogout(context.Background(), 1, 4); err != nil { + t.Fatalf("ForceLogout: %v", err) + } + return rec, nil + }}, + {"role assignment", "role_change", func(t *testing.T) (*audittest.Recorder, []string) { + svc, database := newTestRoleService(t) + rec := audittest.Install(t, database) + if _, err := svc.ChangeUserRole(context.Background(), 1, 4, 3); err != nil { + t.Fatalf("ChangeUserRole: %v", err) + } + return rec, nil + }}, + {"password change", "password_change", func(t *testing.T) (*audittest.Recorder, []string) { + _, database := newTestModerationService(t) + hash, _ := auth.HashPassword("NewPassw0rd!") + rec := audittest.Install(t, database) + if _, err := NewUserService(database).ChangePassword(context.Background(), 4, hash, 0); err != nil { + t.Fatalf("ChangePassword: %v", err) + } + return rec, []string{"NewPassw0rd!", hash} + }}, + {"session revoke", "session_revoke", func(t *testing.T) (*audittest.Recorder, []string) { + _, database := newTestModerationService(t) + raw, _ := auth.GenerateToken() + hash := auth.HashToken(raw) + sid, err := database.CreateSession(context.Background(), 4, hash, "test", "127.0.0.1") + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + rec := audittest.Install(t, database) + if err := NewUserService(database).RevokeSession(context.Background(), 4, sid); err != nil { + t.Fatalf("RevokeSession: %v", err) + } + return rec, []string{raw, hash} + }}, + {"message delete", "message_delete", func(t *testing.T) (*audittest.Recorder, []string) { + svc, database := newTestMessageService(t) + const body = "secret message body 4d0d1405" + res, err := svc.SendMessage(context.Background(), SendMessageParams{ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: body}) + if err != nil { + t.Fatalf("SendMessage: %v", err) + } + rec := audittest.Install(t, database) + if _, err := svc.DeleteMessage(context.Background(), 1, res.MessageID); err != nil { + t.Fatalf("DeleteMessage: %v", err) + } + return rec, []string{body} + }}, + {"message purge", "message_purge", func(t *testing.T) (*audittest.Recorder, []string) { + svc, database := newTestMessageService(t) + seedRole(t, database, &db.Role{ID: permissions.MemberRoleID, Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.ManageMessages, Position: 1}) + const body = "purged message body 4d0d1405" + if _, err := svc.SendMessage(context.Background(), SendMessageParams{ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: body}); err != nil { + t.Fatalf("SendMessage: %v", err) + } + rec := audittest.Install(t, database) + if _, err := svc.PurgeMessages(context.Background(), 1, 10, 10, 0); err != nil { + t.Fatalf("PurgeMessages: %v", err) + } + return rec, []string{body} + }}, + } + + var corpus []db.AuditEntry + var secrets []string + for _, row := range rows { + t.Run(row.name, func(t *testing.T) { + rec, s := row.run(t) + rec.Wait(t, row.action) + corpus = append(corpus, rec.Entries()...) + secrets = append(secrets, s...) + }) + } + t.Run("detail denylist", func(t *testing.T) { + if len(corpus) == 0 { + t.Fatal("no audit entries recorded") + } + audittest.AssertSafeDetails(t, corpus, secrets...) + }) +} + +// TestAuditCoverage_InviteRevokeFailureEmitsNothing pins S-02's failure half: +// a revoke that finds no invite is NotFound and writes no audit row. +func TestAuditCoverage_InviteRevokeFailureEmitsNothing(t *testing.T) { + _, database := newTestModerationService(t) + rec := audittest.Install(t, database) + err := NewInviteService(database).RevokeInvite(context.Background(), 1, "no-such-code") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("RevokeInvite unknown code: want ErrNotFound, got %v", err) + } + time.Sleep(20 * time.Millisecond) + if got := rec.Entries(); len(got) != 0 { + t.Fatalf("failed revoke must not audit; got %v", got) + } +} + +// cancelAfterCreateStore cancels the request context the moment the insert +// has committed, modelling a client that drops the connection mid-request. +type cancelAfterCreateStore struct { + *db.DB + cancel context.CancelFunc +} + +func (s *cancelAfterCreateStore) CreateInvite(ctx context.Context, createdBy int64, maxUses int, expiresAt *time.Time) (string, error) { + code, err := s.DB.CreateInvite(ctx, createdBy, maxUses, expiresAt) + s.cancel() + return code, err +} + +// TestCreateInvite_AuditSurvivesCanceledLookup pins Codex's P2 on #1441: once +// the invite row is committed, a canceled request context must neither turn +// the creation into an error nor skip its audit row. +func TestCreateInvite_AuditSurvivesCanceledLookup(t *testing.T) { + _, database := newTestModerationService(t) + rec := audittest.Install(t, database) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + inv, err := NewInviteService(&cancelAfterCreateStore{DB: database, cancel: cancel}).CreateInvite(ctx, 1, 5, 24) + if err != nil { + t.Fatalf("committed invite must not fail on a canceled lookup: %v", err) + } + if e := rec.Wait(t, "invite_create"); e.TargetID != inv.ID { + t.Fatalf("invite_create target = %d, want %d", e.TargetID, inv.ID) + } +} diff --git a/Server/service/invite.go b/Server/service/invite.go index 90ed6680..55198f2b 100644 --- a/Server/service/invite.go +++ b/Server/service/invite.go @@ -53,10 +53,17 @@ func (s *InviteService) CreateInvite(ctx context.Context, createdBy int64, maxUs return nil, fmt.Errorf("%w: failed to create invite: %v", ErrInternal, err) } - invite, err := s.st.GetInvite(ctx, code) + // The invite is committed from here on: a request canceled during the + // read-back must neither fail the creation nor skip its audit row. + tailCtx := context.WithoutCancel(ctx) + invite, err := s.st.GetInvite(tailCtx, code) if err != nil || invite == nil { return nil, fmt.Errorf("%w: failed to retrieve invite: %v", ErrInternal, err) } + // S-02: the row names the invite by id, never by code; the code is the + // credential. + db.WriteAudit(tailCtx, s.st, createdBy, "invite_create", "invite", invite.ID, + fmt.Sprintf("max_uses=%d expires_in_hours=%d", maxUses, expiresInHours)) return invite, nil } @@ -69,8 +76,8 @@ func (s *InviteService) ListInvites(ctx context.Context) ([]*db.Invite, error) { return invites, nil } -// RevokeInvite revokes an invite by code. -func (s *InviteService) RevokeInvite(ctx context.Context, code string) error { +// RevokeInvite revokes an invite by code on behalf of actorID. +func (s *InviteService) RevokeInvite(ctx context.Context, actorID int64, code string) error { invite, err := s.st.GetInvite(ctx, code) if err != nil || invite == nil { return fmt.Errorf("%w: invite not found", ErrNotFound) @@ -78,5 +85,6 @@ func (s *InviteService) RevokeInvite(ctx context.Context, code string) error { if err := s.st.RevokeInvite(ctx, code); err != nil { return fmt.Errorf("%w: failed to revoke invite: %v", ErrInternal, err) } + db.WriteAudit(context.WithoutCancel(ctx), s.st, actorID, "invite_revoke", "invite", invite.ID, "") return nil } diff --git a/docs/plans/b2-protocol-trust-compat-2026-08-28.md b/docs/plans/b2-protocol-trust-compat-2026-08-28.md index 36ae88cd..45e9ce8f 100644 --- a/docs/plans/b2-protocol-trust-compat-2026-08-28.md +++ b/docs/plans/b2-protocol-trust-compat-2026-08-28.md @@ -5,7 +5,7 @@ `v1.2.0-alpha.4` — claims verified at `64d2e108`; the branch was rebased onto `dd7ed091` (#1432) before merge **Status:** in progress — entry gate 1 of 3 met at draft time (see below); B2-0, -B2-1 and B2-8 landed 2026-08-28, B2-2 (with B2-3 and B2-4 folded in) and B2-5 on 2026-08-29 (evidence in their sections); B2-6 and B2-7 are next. +B2-1 and B2-8 landed 2026-08-28, B2-2 (with B2-3 and B2-4 folded in) and B2-5 on 2026-08-29 (evidence in their sections); B2-6 landed 2026-08-29 (PR #1441); B2-7 is next. Update this line, not only the step table, when a step lands. Primary inputs: @@ -446,6 +446,86 @@ half stands on those two tests. secret, or message body — a denylist over the recorded corpus. Fix any hit at the call site, never by loosening the list. +**Evidence, 2026-08-29** — branch `feat/b2-6-audit-coverage` from `dev` +`67fdd18d`; PR #1441 to `dev`. HP-2 cites this block. + +- Step 1 — the mutation inventory, crossed with the 43 non-test `Audit(` call + sites at `67fdd18d` (`WriteAudit`, `LogAudit`, `EnqueueAudit`). "Before" is + whether the mutation wrote an audit row at that SHA; "table" names the B2-6 + test that now asserts it (`TestAuditCoverage_*` in `service`, `api` and + `admin`, each over a fake `db.AuditStore` from `Server/db/audittest`). + + | Mutation | Handler | Action | Before | Table | + | ------------------------- | -------------------------------------------------------------- | -------------------------------------------------------- | ------ | -------------------------------------------------------------- | + | Password change | `service/user.go` `ChangePassword` | `password_change` | yes | service | + | Session revoke | `service/user.go` `RevokeSession` | `session_revoke` | yes | service | + | TOTP enrol | `api/totp_handler.go` confirm | `totp_enabled` | yes | api | + | TOTP disable | `api/totp_handler.go` disable | `totp_disabled` | yes | api | + | Role assignment | `service/moderation.go` `ChangeUserRole` | `role_change` | yes | service | + | Invite create | `service/invite.go` `CreateInvite` | `invite_create` | **no** | service — added (S-02) | + | Invite revoke | `service/invite.go` `RevokeInvite` | `invite_revoke` | **no** | service — added (S-02); actor threaded from the handler | + | Ban / unban | `service/moderation.go` `BanUser` / `UnbanUser` | `user_ban` / `user_unban` | yes | service | + | Kick (sessions) | `service/moderation.go` `ForceLogout` | `force_logout` | yes | service | + | Kick (voice) | `ws/voice_moderation.go` `handleVoiceModKick` | `voice_mod_kick` | yes | existing `TestVoiceMod_Kick_RemovesFromVoiceAndNotifiesTarget` | + | Timeout | — no timeout mutation exists on the server | — | n/a | — | + | Channel role overrides | `admin/handlers_channel_perms.go` put / delete | `channel_perms_update` / `channel_perms_clear` | yes | admin | + | Channel user overrides | `admin/handlers_channel_perms.go` put / delete (user layer) | `channel_user_perms_update` / `channel_user_perms_clear` | yes | admin | + | TLS / config change | `admin/setup_handler.go` `setupApplyWizard` | `config_write` (with `server_setup`) | yes | admin | + | Settings change | `admin/handlers_settings.go` `handlePatchSettings` | `setting_change` | yes | admin | + | API token create / revoke | `admin/handlers_tokens.go` (`token_cli.go` shares the actions) | `api_token_create` / `api_token_revoke` | yes | admin | + | Plugin install | `api/plugins_handler.go` `install` | `plugin_install` | **no** | api — added | + | Plugin uninstall | `api/plugins_handler.go` `uninstall` | `plugin_uninstall` | **no** | api — added | + | Account deletion | `api/auth_handler.go` delete account | `account_deleted` | yes | api | + | Message deletion | `service/message_crud.go` `DeleteMessage` | `message_delete` | yes | service | + | Message purge | `service/message_purge.go` `PurgeMessages` | `message_purge` | yes | service | + + Call sites outside the security-sensitive list (channel CRUD, emoji, + profile, identity key, backups, login/logout/register, `ws_connect`, the + other three voice moderation actions) keep their existing rows and are not + in the table; the denylist in step 3 does not run over them. + +- Pre-squash SHAs, one commit per step: `ea914e66` (step 1, the table + above), `a06499f2` (step 2, tables + the four audit calls), `6193a709` + (step 3, denylist + its self-test). `474ec74c` and `cbbf41c1` are the + register/CHANGELOG/security.md edits, committed from outside the session + while the step-2 gate ran; content unchanged, kept as-is. +- Step 2 — fixture: `Server/db/audittest` installs a `db.AuditWriter` over a + recording `AuditStore` via `SetAuditWriter`, so every `WriteAudit` through + the test's `*db.DB` lands in memory regardless of package. Tables: + `TestAuditCoverage_ServiceMutations` (10 rows), `TestAuditCoverage_APIMutations` + (3), `TestAuditCoverage_PluginLifecycle` (2, package-internal fixtures), + `TestAuditCoverage_AdminMutations` (8). Red at `ea914e66` + tests on exactly + the four rows the table predicts — `invite_create`, `invite_revoke`, + `plugin_install`, `plugin_uninstall` (each `no "" audit entry +recorded; recorded actions: []`); every other row green before any + production change. Green after the four calls. `RevokeInvite` gained the + actor parameter (threaded from `handleRevokeInvite`); the plugin handler + gained a `db.Auditor` and `admin.ActorIDFromContext` was exported so its + rows name the `RequireAdminAuth` principal. S-02's failure half: + `TestAuditCoverage_InviteRevokeFailureEmitsNothing`. +- Step 3 — `audittest.AssertSafeDetails` runs over the union corpus each + table recorded: shape denylist (bcrypt/argon2 hashes, `password=` / + `token=` / `secret=` / recovery-code key-value leaks, `otpauth://`, + `Bearer `) plus every fixture secret the rows return (raw session and API + tokens and their hashes, passwords, TOTP secrets and codes, invite codes, + message bodies, the setup password). `TestAssertSafeDetails_Bites` proves + each class rejects and ordinary details pass. Zero hits on the corpus at + `6193a709`; no call site changed. +- Gates from `Server/` before each commit: four build-tag variants, `go vet`, + `go test -race ./...`, `go test -tags deadlock ./ws/`, `golangci-lint run` + (one `contextcheck` round: hoisted `ctx` in the tables, inlined), + `sqlc generate` and `genprotocol` drift — all exit 0. Docs commits: + `npm run check:docs`, `npm run check:hygiene`. +- Closes S-02 (register: resolved/superseded). Ledger untouched. +- Codex review on #1441, two P2s, both fixed test-first in `aadd911b`, same + gate green: `CreateInvite` read the invite back on the request context, so + a cancel after the committed insert failed the call and skipped + `invite_create` — read-back and audit now run on `context.WithoutCancel` + (`TestCreateInvite_AuditSurvivesCanceledLookup`); and + `Registry.UninstallPlugin` is idempotent on an unknown id, so the handler + audited uninstalls that never happened — it now checks the row first and + answers 404 with no audit (`TestPluginsHandlerUninstallUnknownID`). + ## B2-7 — Trust model, absence proofs, plugin boundary (documents) Runs in parallel with B2-1 and B2-6. diff --git a/docs/plans/repo-health-issue-register-2026-08-23.md b/docs/plans/repo-health-issue-register-2026-08-23.md index 040cb2ce..b53ad78f 100644 --- a/docs/plans/repo-health-issue-register-2026-08-23.md +++ b/docs/plans/repo-health-issue-register-2026-08-23.md @@ -184,25 +184,25 @@ not recounted here. ## Server engineering issues -| ID | Pri | State | Issue and evidence | Phase | Closure evidence | -| ---- | --: | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| S-01 | P1 | resolved/superseded | Typing currently checks a weaker permission than posting. Landed in B2-5 (PR #1440): typing delegates to the send-policy predicate; denial, announcement, archive and override tests prevent drift. | B2/B3 | No separate action; the parity table locks it. | -| S-02 | P1 | confirmed | Invite create/revoke are privileged mutations without the audit coverage used by sibling mutation families. | B4/B5 | Successful create/revoke produce safe, non-secret audit events; failure behavior is tested. | -| S-03 | P2 | confirmed | Admin channel name/topic/category validation lacks one explicit rune/normalization contract. | B3/B5 | Shared limits cover admin and user writers; boundary tests count runes, not bytes. | -| S-04 | P2 | confirmed | Sibling admin channel lookups expose inconsistent DM/not-found response contracts. | B3 | One non-DM resolution policy and response contract covers both paths. | -| S-05 | P2 | confirmed | Repository-wide Go formatting is not a required gate. | B1 | Tree is formatted and a fast required gate fails future drift. | -| S-06 | P2 | confirmed | Server coverage is uploaded without a global or core-package regression floor; current aggregate is 74.6%. | B3/B10 | Documented baseline/exclusions and ratcheted global/core thresholds. | -| S-07 | P2 | confirmed | Thousands of tests and 17 fuzz targets exist, but there are no Go benchmarks for hub/replay, permission, DB, or fan-out hot paths. | B6/B10 | Stable microbenchmarks and reference load baselines cover the highest-risk paths. | -| S-08 | P2 | confirmed | Large lifecycle/hub/serve files remain structural hotspots. | B3 | Cohesive extractions preserve lifecycle, locking, race, and deadlock invariants. | -| S-09 | P2 | confirmed | API/admin/WebSocket layers still contain many direct database call sites. | B3 | Each use moves behind a narrow service/store seam or is documented as an intentional transaction/composition boundary. | -| S-10 | P2 | confirmed | Auth routes still consume raw database ownership and are the first intended S-09 migration slice. | B3/B4 | Tested AuthService/narrow interfaces preserve enumeration and sentinel-error behavior. | -| S-11 | P2 | confirmed | Hub construction uses post-construction collaborator setters, leaving required wiring temporally coupled to `Run`. | B3 | Required collaborators are validated constructor/options inputs; only genuinely dynamic dependencies remain mutable. | -| S-12 | P2 | resolved/superseded | Ready/refresh/WebSocket paths mirror message send-permission policy by hand. Landed in B2-5 (PR #1440): all paths delegate to one value-taking predicate with parity tests in both resolution branches. | B3 | No separate action; the authz-chokepoint invariant rule remains B3 item 15. | -| S-13 | P2 | confirmed | Durable TOTP used-code and partial-auth persister work remains incomplete. | B4 | Hash-only persistence, expiry, restart, and failure-mode tests land without persisting sliding rate-limit windows. | -| S-14 | P1 | confirmed | Load tooling exists, but no supported capacity result is published for the approved 250 users / 100 connections / 25 voice profile. | B6/B10 | Reproducible report states hardware/software, CPU, memory, DB waits, p95/p99 latency, and pass/fail thresholds. | -| S-15 | P3 | verify | `voice_speakers` and `member_leave` remain reserved protocol entries with no production emit site. | B2 | Compatibility review removes unused entries before the epoch freeze or explicitly reserves and fixtures them; schema, generated types, docs, and tests agree. | -| S-16 | P3 | verify | Voice key-holder TOCTOU hardening remains a documented follow-up without a demonstrated contract failure. | B2/B3 | Threat-model review either records why outer checks suffice or adds an in-function recheck and race-focused private test. | -| S-17 | P3 | watch | Vulnerability tooling found no reachable Go advisory, while non-called/unmaintained upstream paths remain. | B6/B10 | Dependency path is monitored, compatible fixes are applied, and reachable-symbol scanning remains required. | +| ID | Pri | State | Issue and evidence | Phase | Closure evidence | +| ---- | --: | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| S-01 | P1 | resolved/superseded | Typing currently checks a weaker permission than posting. Landed in B2-5 (PR #1440): typing delegates to the send-policy predicate; denial, announcement, archive and override tests prevent drift. | B2/B3 | No separate action; the parity table locks it. | +| S-02 | P1 | resolved/superseded | Invite create/revoke are privileged mutations without the audit coverage used by sibling mutation families. Landed in B2-6: `invite_create` / `invite_revoke` rows name the invite by id, never by code; the audit table and detail denylist lock every security-sensitive mutation. | B2 | No separate action; `TestAuditCoverage_*` (service/api/admin) and the S-02 failure test lock it. | +| S-03 | P2 | confirmed | Admin channel name/topic/category validation lacks one explicit rune/normalization contract. | B3/B5 | Shared limits cover admin and user writers; boundary tests count runes, not bytes. | +| S-04 | P2 | confirmed | Sibling admin channel lookups expose inconsistent DM/not-found response contracts. | B3 | One non-DM resolution policy and response contract covers both paths. | +| S-05 | P2 | confirmed | Repository-wide Go formatting is not a required gate. | B1 | Tree is formatted and a fast required gate fails future drift. | +| S-06 | P2 | confirmed | Server coverage is uploaded without a global or core-package regression floor; current aggregate is 74.6%. | B3/B10 | Documented baseline/exclusions and ratcheted global/core thresholds. | +| S-07 | P2 | confirmed | Thousands of tests and 17 fuzz targets exist, but there are no Go benchmarks for hub/replay, permission, DB, or fan-out hot paths. | B6/B10 | Stable microbenchmarks and reference load baselines cover the highest-risk paths. | +| S-08 | P2 | confirmed | Large lifecycle/hub/serve files remain structural hotspots. | B3 | Cohesive extractions preserve lifecycle, locking, race, and deadlock invariants. | +| S-09 | P2 | confirmed | API/admin/WebSocket layers still contain many direct database call sites. | B3 | Each use moves behind a narrow service/store seam or is documented as an intentional transaction/composition boundary. | +| S-10 | P2 | confirmed | Auth routes still consume raw database ownership and are the first intended S-09 migration slice. | B3/B4 | Tested AuthService/narrow interfaces preserve enumeration and sentinel-error behavior. | +| S-11 | P2 | confirmed | Hub construction uses post-construction collaborator setters, leaving required wiring temporally coupled to `Run`. | B3 | Required collaborators are validated constructor/options inputs; only genuinely dynamic dependencies remain mutable. | +| S-12 | P2 | resolved/superseded | Ready/refresh/WebSocket paths mirror message send-permission policy by hand. Landed in B2-5 (PR #1440): all paths delegate to one value-taking predicate with parity tests in both resolution branches. | B3 | No separate action; the authz-chokepoint invariant rule remains B3 item 15. | +| S-13 | P2 | confirmed | Durable TOTP used-code and partial-auth persister work remains incomplete. | B4 | Hash-only persistence, expiry, restart, and failure-mode tests land without persisting sliding rate-limit windows. | +| S-14 | P1 | confirmed | Load tooling exists, but no supported capacity result is published for the approved 250 users / 100 connections / 25 voice profile. | B6/B10 | Reproducible report states hardware/software, CPU, memory, DB waits, p95/p99 latency, and pass/fail thresholds. | +| S-15 | P3 | verify | `voice_speakers` and `member_leave` remain reserved protocol entries with no production emit site. | B2 | Compatibility review removes unused entries before the epoch freeze or explicitly reserves and fixtures them; schema, generated types, docs, and tests agree. | +| S-16 | P3 | verify | Voice key-holder TOCTOU hardening remains a documented follow-up without a demonstrated contract failure. | B2/B3 | Threat-model review either records why outer checks suffice or adds an in-function recheck and race-focused private test. | +| S-17 | P3 | watch | Vulnerability tooling found no reachable Go advisory, while non-called/unmaintained upstream paths remain. | B6/B10 | Dependency path is monitored, compatible fixes are applied, and reachable-symbol scanning remains required. | ## Repository, CI, documentation, and supply chain diff --git a/docs/security.md b/docs/security.md index dfdd509d..3083e420 100644 --- a/docs/security.md +++ b/docs/security.md @@ -54,7 +54,7 @@ Security-relevant actions are recorded in the `audit_log` table with actor, acti - **Auth:** `user_register`, `user_login`, `user_logout`, `login_blocked_banned`, `account_deleted`, `password_change`, `session_revoke` - **2FA:** `totp_enabled`, `totp_verified`, `totp_disabled` -- **Admin:** `role_change`, `role_create`, `role_update`, `role_delete`, `role_reorder`, `user_ban`, `user_unban`, `force_logout`, `setting_change`, `server_setup`, `api_token_create`, `api_token_revoke`, `config_write` +- **Admin:** `role_change`, `role_create`, `role_update`, `role_delete`, `role_reorder`, `user_ban`, `user_unban`, `force_logout`, `setting_change`, `server_setup`, `api_token_create`, `api_token_revoke`, `config_write`, `invite_create`, `invite_revoke`, `plugin_install`, `plugin_uninstall` - **Content:** `channel_create`, `channel_update`, `channel_delete`, `channel_perms_update`, `channel_perms_clear`, `channel_user_perms_update`, `channel_user_perms_clear`, `message_delete`, `message_purge`, `emoji_create`, `emoji_delete` - **Profile:** `profile_update`, `identity_key_update` - **Ops:** `backup_create`, `backup_delete`, `backup_restore`, `ws_connect`