diff --git a/.cursor/rules/marchat.mdc b/.cursor/rules/marchat.mdc index 7a8c40b..87efbb7 100644 --- a/.cursor/rules/marchat.mdc +++ b/.cursor/rules/marchat.mdc @@ -95,7 +95,7 @@ Prefer this repo (`go.mod`, `ARCHITECTURE.md`, `PROTOCOL.md`, skills) as source - **Startup**: validated before serve - at least one admin, non-empty admin key, valid listen port; **admin names** trimmed, lowercased, case-insensitive **dedupe** (`cmd/server`, extracted validation helpers) - **Hub**: per-channel routing, DMs, typing/read receipts/reactions; outbound client messages channel-stamped from membership (`stampClientChannel`); outbound `sender` stamped from authenticated session (`stampSenderTimedOutbound`); **reserved usernames** so handshake cannot double-book a name before registration - **WebSocket**: **serialized writes** per connection (avoid concurrent write + control-frame panics) -- **SQLite**: **WAL** enabled when backend is SQLite; in-process `:backup` uses **VACUUM INTO** (SQLite only; quote paths safely when SQL embeds paths). Postgres/MySQL use native backup tools. +- **SQLite**: WAL and related pragmas via DSN on every connection (`_busy_timeout`, `_journal_mode=WAL`, …) plus `SetMaxOpenConns(1)` / `SetMaxIdleConns(1)` in `InitDB` - not one-shot `PRAGMA` `Exec` with the default pool; in-process `:backup` uses **VACUUM INTO** (SQLite only; quote paths safely when SQL embeds paths). Postgres/MySQL use native backup tools. - **Web admin** (`server/admin_web.go`, `server/admin_web.html`): session cookies; **`MARCHAT_SESSION_SECRET`** preferred, **`MARCHAT_JWT_SECRET`** deprecated alias; `config.GenerateSessionSecret()` when unset; **login rate limiting** per IP; **CSRF** on mutating routes - **Interactive server config** (`server/config_ui.go`): can generate session secret when saving config - **Health**: metrics and health HTTP endpoints (`server/health.go`) diff --git a/.cursor/skills/README.md b/.cursor/skills/README.md index 6534618..39c90ac 100644 --- a/.cursor/skills/README.md +++ b/.cursor/skills/README.md @@ -29,7 +29,8 @@ For Cursor, dependencies, or platform behavior not defined in this repo, verify Update domain skills when shipped behavior changes. Recent fixes on `main` (or in flight): -- Reconnect backoff advances on failure (not reset each `Init()`); channel stamping on server outbound messages +- SQLite `InitDB`: DSN per-connection pragmas (`busy_timeout`, WAL) + `MaxOpenConns(1)` / `MaxIdleConns(1)`; do not one-shot `PRAGMA` with the default pool ([#118](https://github.com/Cod-e-Codes/marchat/issues/118)) +- Kick/ban self-target rejection and online-only kick (`ErrKickNotConnected` for offline targets; `BanUser` offline-capable) - Client transcript notices: negative `message_id` classified by content; scoped to active channel - URL click: OSC 8 hyperlinks on wrapped segments (Lip Gloss v2); manual click fallback remains unreliable for wrapped long URLs; copy/paste when needed ([#103](https://github.com/Cod-e-Codes/marchat/issues/103)) - Charm v2: `charm.land/*/v2`, `tea.View` + `KeyPressMsg`, overlay scroll/input routing in `scroll_input.go` diff --git a/.cursor/skills/database-marchat/SKILL.md b/.cursor/skills/database-marchat/SKILL.md index ba366a0..032160b 100644 --- a/.cursor/skills/database-marchat/SKILL.md +++ b/.cursor/skills/database-marchat/SKILL.md @@ -20,7 +20,13 @@ Runtime backend via `MARCHAT_DB_PATH`: SQLite (default), PostgreSQL, or MySQL. D - Parameterized queries only; no string-concatenated user input. - Every schema or query change must work on all three dialects (or use `db_dialect.go` helpers). -- SQLite: WAL when backend is SQLite; quote paths safely in `VACUUM INTO` and similar. +- SQLite (`InitDB` only): + - Put connection pragmas in the DSN so every pooled connection gets them (`_busy_timeout=5000`, `_journal_mode=WAL`, `_synchronous=NORMAL`, plus `_pragma` for cache/temp). Join with `?` or `&` if the path already has a query (`appendSQLiteDSNPragmas`). + - After `Ping`, set `SetMaxOpenConns(1)` and `SetMaxIdleConns(1)`. Do **not** leave the default multi-connection `database/sql` pool on SQLite. + - Do **not** rely on one-shot `Exec("PRAGMA ...")` after open for settings that must stick on every connection (that was the #118 `SQLITE_BUSY` failure mode). + - Verify after open: `busy_timeout > 0`; for file-backed DBs, `journal_mode` is `wal`. In-memory (`:memory:` / `mode=memory`) requires busy_timeout only. + - Quote paths safely in `VACUUM INTO` and similar. +- Postgres/MySQL: leave pool defaults alone (do not force `MaxOpenConns(1)`). - MySQL: DSN via `mysql:` or `mysql://`; `mysql.Config` with `parseTime=true`; indexed text rules for search. - Postgres: boolean columns need dialect boolean literals, not `= 0` / `= 1`. @@ -32,8 +38,8 @@ Include messages plus durable state: reactions, read receipts, `user_message_sta | Level | Where | |-------|--------| -| Unit / integration | In-memory or temp SQLite in `server/*_test.go` | -| CI smoke | `server/db_ci_smoke_test.go` with `MARCHAT_CI_POSTGRES_URL`, `MARCHAT_CI_MYSQL_URL` | +| Unit / integration | In-memory or temp SQLite in `server/*_test.go` (`db_test.go` covers DSN join, file WAL, `:memory:`, concurrent inserts) | +| CI smoke | `server/db_ci_smoke_test.go` with `MARCHAT_CI_POSTGRES_URL`, `MARCHAT_CI_MYSQL_URL` (also asserts pool is not forced to 1) | | Handlers | Visible replay SQL (`GetRecentMessagesForUser`), search, pin toggle | Locally, CI smoke tests skip without env vars. See `testing-marchat` skill. diff --git a/.cursor/skills/debugging-marchat/SKILL.md b/.cursor/skills/debugging-marchat/SKILL.md index d6aa081..cdcd418 100644 --- a/.cursor/skills/debugging-marchat/SKILL.md +++ b/.cursor/skills/debugging-marchat/SKILL.md @@ -37,6 +37,7 @@ Implementation: `internal/doctor/`. DB dialect and DSN shape checks live there a | Postgres boolean errors | Dialect boolean helpers in `server/db_dialect.go` (`:search`, pin toggle) | | MySQL time parsing | `mysql.Config` with `parseTime=true` in `InitDB` | | SQLite path vs remote DSN | `MARCHAT_DB_PATH`; `mysql:` / `postgres:` prefixes for driver detection | +| SQLite `SQLITE_BUSY` / missing messages under load | Confirm `InitDB` DSN pragmas (`_busy_timeout`, WAL) and `MaxOpenConns(1)`; not one-shot `PRAGMA` alone ([#118](https://github.com/Cod-e-Codes/marchat/issues/118)) | | Plugin disable race | `StopPlugin` waits for stdout/stderr readers (`plugin/host`) | | Rate limit | `server/loadverify_ratelimit_test.go` constants match `client.go` read pump | diff --git a/.cursor/skills/server-marchat/SKILL.md b/.cursor/skills/server-marchat/SKILL.md index aadbccd..483b40d 100644 --- a/.cursor/skills/server-marchat/SKILL.md +++ b/.cursor/skills/server-marchat/SKILL.md @@ -23,7 +23,7 @@ App entry: `cmd/server/main.go`. Library: `server/` (hub, client, handlers, db, - Per-channel routing, DMs, typing, read receipts, reactions. - Outbound client messages are channel-stamped from hub membership (`stampClientChannel`); client-supplied `channel` values are ignored for routing. -- All outbound/persist paths stamp `sender` from the authenticated session (`stampSenderTimedOutbound`); NUL bytes in persistable `content` are rejected before insert. +- All outbound/persist paths stamp `sender` from the authenticated session (`stampSenderTimedOutbound`); NUL bytes in persistable `content` are rejected before insert; empty or whitespace-only plaintext on `text` / `dm` / `edit` is rejected when `encrypted` is false (encrypted opaque ciphertext is never treated as empty). - Reserved usernames during handshake (no double-book before registration). - Serialized writes per connection (`client.go`). - File uploads: `SetReadLimit` uses `websocketReadLimit` (max of policy `fileMessageReadLimit` wire size and a **32 MiB** DoS ceiling) so modest oversize is fully read; declared/payload checks send a System reply and `continue`. `ErrReadLimit` (above the ceiling) logs rejection only - gorilla already sent empty close **1009**, so a System enqueue cannot flush. @@ -32,8 +32,8 @@ App entry: `cmd/server/main.go`. Library: `server/` (hub, client, handlers, db, ## Admin -- TUI: `admin_panel.go`, `config_ui.go` (Charm v2: `tea.View`, `KeyPressMsg`, bubbles setters). Admin panel enables `MouseModeCellMotion` and routes `MouseWheelMsg` for scrollable tabs and user/plugin tables. -- Web: `admin_web.go`, `admin_web.html`; `MARCHAT_SESSION_SECRET` (preferred), `MARCHAT_JWT_SECRET` deprecated; CSRF on mutating routes; login rate limit per IP. +- TUI: `admin_panel.go`, `config_ui.go` (Charm v2: `tea.View`, `KeyPressMsg`, bubbles setters). Admin panel enables `MouseModeCellMotion` and routes `MouseWheelMsg` for scrollable tabs and user/plugin tables. Kick/ban cmds claim success only when hub `KickUser`/`BanUser` return nil (self-target, not-connected kick, and permanently-banned kick errors map to failed action messages). `KickUser` is online-only; offline kicks return `ErrKickNotConnected`. +- Web: `admin_web.go`, `admin_web.html`; `MARCHAT_SESSION_SECRET` (preferred), `MARCHAT_JWT_SECRET` deprecated; CSRF on mutating routes; login rate limit per IP. User kick/ban actions return `success: false` with a message when the hub rejects the target. - Trusted proxies: `MARCHAT_TRUSTED_PROXIES` for forwarded client IP. ## Security diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b1651a5..bb2929b 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -90,7 +90,7 @@ The server is a standalone HTTP/WebSocket server application that provides real- #### Core Structures - **`Hub`**: Central message routing system managing client connections, message broadcasting, channel management, and user state; tracks reserved usernames so handshake cannot double-book the same name before a client is registered. All sends to `client.send` use non-blocking `select/default` to prevent deadlocks when a client's write buffer is full; stalled clients are dropped or the message is logged and skipped. **Text** messages fan out to plugins in a **separate goroutine** so plugin IPC never blocks the hub’s broadcast loop. -- **`Client`**: Individual WebSocket connection handler with read/write pumps and command processing. The `writePump` goroutine is started **before** history replay on connect so the send channel always has a consumer. Outbound client messages are channel-stamped from hub membership (`stampClientChannel`) so spoofed `channel` values cannot cross rooms; `sender` is stamped from the authenticated session (`stampSenderTimedOutbound`) on persist and broadcast paths. NUL bytes in persistable `content` are rejected before insert (Postgres rejects NUL in TEXT; SQLite accepts it). Failed inserts reply to the sender and are not broadcast. `handleCommand` sends `Unknown command` to non-admins for unrecognized `:` tokens; built-in admin-only commands still return an admin-privilege notice. Admins get `Unknown command` for unrecognized built-ins. +- **`Client`**: Individual WebSocket connection handler with read/write pumps and command processing. The `writePump` goroutine is started **before** history replay on connect so the send channel always has a consumer. Outbound client messages are channel-stamped from hub membership (`stampClientChannel`) so spoofed `channel` values cannot cross rooms; `sender` is stamped from the authenticated session (`stampSenderTimedOutbound`) on persist and broadcast paths. NUL bytes in persistable `content` are rejected before insert (Postgres rejects NUL in TEXT; SQLite accepts it). Empty or whitespace-only plaintext on `text`, `dm`, and `edit` is rejected when `encrypted` is false (System reply; no insert or broadcast); when `encrypted` is true the server treats `content` as opaque and does not apply the empty-plaintext check. Failed inserts reply to the sender and are not broadcast. `handleCommand` sends `Unknown command` to non-admins for unrecognized `:` tokens; built-in admin-only commands still return an admin-privilege notice. Admins get `Unknown command` for unrecognized built-ins. - **`AdminPanel`**: Terminal-based administrative interface for server management - **`WebAdminServer`**: Web-based administrative interface with session authentication - **`HealthChecker`**: System health monitoring with metrics collection @@ -103,7 +103,7 @@ The server is a standalone HTTP/WebSocket server application that provides real- - Direct message routing between specific users - Message editing, deletion, pinning, and search - Typing indicator, reaction, and read receipt broadcasting (channel-scoped when `channel` is set after stamping) -- User management including ban, kick, and allow operations (ban/kick state is committed under `banMutex`, then the lock is released before the actual disconnect to avoid holding the mutex across a channel send) +- User management including ban, kick, and allow operations (ban/kick state is committed under `banMutex`, then the lock is released before the actual disconnect to avoid holding the mutex across a channel send). `KickUser` and `BanUser` return errors; self-targets are rejected case-insensitively before any ban state is written, and callers (chat commands, admin TUI, web admin) claim success only when `err == nil`. `KickUser` is online-only (disconnect plus 24h temporary ban when the target has an active connection); offline moderation uses `BanUser` / `:ban` - Plugin command execution and management - Database backup via `:backup` and admin panels: **SQLite only** (`VACUUM INTO` with quoted paths). Postgres and MySQL deployments receive a clear error directing operators to native backup tools for `MARCHAT_DB_PATH`. - System metrics collection and health monitoring @@ -261,7 +261,7 @@ Client: WebSocket Receive → Decrypt → Display - MySQL DSN (`mysql:` / `mysql://`) - Schema creation and upsert/insert-ignore SQL are dialect-aware; message query helpers in `server/db_dialect.go` emit Postgres `TRUE`/`FALSE` or SQLite/MySQL `1`/`0` for boolean columns as needed. - Placeholder rebinding keeps shared query callsites portable across backends. -- SQLite-specific optimizations (for example WAL mode) are applied only when the selected backend is SQLite. +- SQLite-specific optimizations are applied only when the selected backend is SQLite: per-connection DSN pragmas (`_busy_timeout`, `_journal_mode=WAL`, `_synchronous`, cache/temp store) plus `SetMaxOpenConns(1)` / `SetMaxIdleConns(1)`. Do not rely on one-shot `PRAGMA` `Exec` after `Open` for settings that must stick on every pooled connection. - Durable state includes: - message history - reactions @@ -372,14 +372,15 @@ CREATE TABLE read_receipts ( ### Key Features - **Backend Selection**: `MARCHAT_DB_PATH` chooses SQLite/PostgreSQL/MySQL at runtime -- **WAL Mode (SQLite only)**: Write-Ahead Logging for better concurrency and crash recovery when SQLite is selected +- **WAL Mode (SQLite only)**: Write-Ahead Logging via DSN `_journal_mode=WAL` on every connection (file-backed DBs); verified after open. In-memory DSNs require `busy_timeout` only (WAL may not stick). +- **SQLite pool**: `MaxOpenConns(1)` and `MaxIdleConns(1)` so writers share one connection; Postgres/MySQL keep driver/pool defaults - **SQLite Database Files**: `marchat.db` (main), `marchat.db-wal` (write-ahead log), `marchat.db-shm` (shared memory) - **Message ID Tracking**: Sequential message IDs for user state management - **Encryption Support**: Binary storage for encrypted message data - **Performance Indexes**: Optimized queries for message retrieval and user state - **Message Cap**: Automatic cleanup maintaining 1000 most recent messages - **Ban History**: Comprehensive tracking of user moderation actions -- **Performance Tuning**: Backend-aware optimizations (SQLite pragmas when SQLite is selected) +- **Performance Tuning**: SQLite DSN per-connection pragmas and single-conn pool when SQLite is selected; Postgres/MySQL unchanged ## Administrative Interfaces @@ -455,12 +456,13 @@ The web-based interface (`admin_web.html`, embedded via `go:embed`) provides the ### Database Optimization -- **WAL Mode (SQLite only)**: Write-Ahead Logging enabled for improved concurrency and performance +- **WAL Mode (SQLite only)**: Write-Ahead Logging via per-connection DSN pragmas (not one-shot `PRAGMA` after open) +- **SQLite single-conn pool**: `MaxOpenConns(1)` / `MaxIdleConns(1)` with `_busy_timeout=5000` to avoid `SQLITE_BUSY` under concurrent WebSocket writers - **Indexed Queries**: Performance indexes on frequently queried columns - **Batch Operations**: Efficient bulk message operations - **Connection Reuse**: Persistent database connections - **Query Optimization**: Prepared statements for common operations -- **Performance Tuning**: SQLite-specific pragmas are applied only on SQLite; Postgres/MySQL use driver/backend defaults +- **Performance Tuning**: SQLite DSN pragmas + single-conn pool only on SQLite; Postgres/MySQL use driver/backend defaults - **Backup Considerations**: SQLite WAL mode creates additional files; backups may miss recent uncommitted data if taken while server is running ## Development Patterns diff --git a/CHANGELOG.md b/CHANGELOG.md index 28a3d12..fd8b7b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ Narrative notes by release. Per-file binaries and assets: [GitHub releases](http On **`main`** only; not part of the latest tagged release until you tag and publish. Compare against the current tag on [GitHub releases](https://github.com/Cod-e-Codes/marchat/releases). +- **Server**: **Fix:** `:kick` / `:ban` (and admin TUI / web user actions) reject self-targets case-insensitively and return clear errors instead of disconnecting the admin and writing a 24h ban; `KickUser` / `BanUser` return errors so callers claim success only on `nil`; kicking an already permanently banned user returns an error without claiming success ([#115](https://github.com/Cod-e-Codes/marchat/issues/115)). +- **Server**: **Fix:** `:kick` (and admin TUI / web kick actions) are online-only: `KickUser` requires an active WebSocket connection, disconnects the target, and applies a 24h temporary ban; offline or never-connected users return `ErrKickNotConnected` with no `tempKicks` entry or `ban_history` row; `:ban` / `BanUser` remain offline-capable ([#116](https://github.com/Cod-e-Codes/marchat/issues/116)). +- **Server**: **Fix:** reject empty or whitespace-only plaintext on `text`, `dm`, and `edit` when `encrypted` is false (System reply, no persist/broadcast); encrypted opaque `content` is not treated as empty ([#117](https://github.com/Cod-e-Codes/marchat/issues/117)). +- **Server**: **Fix:** SQLite `InitDB` applies `busy_timeout` / WAL / related pragmas via the DSN on every connection and sets `MaxOpenConns(1)` / `MaxIdleConns(1)`, so concurrent inserts no longer fail with `SQLITE_BUSY` from one-shot `PRAGMA` + the default `database/sql` pool ([#118](https://github.com/Cod-e-Codes/marchat/issues/118)). + ## v1.3.4 **Released 2026-08-03.** Since **[v1.3.3](https://github.com/Cod-e-Codes/marchat/releases/tag/v1.3.3)**; compare [`v1.3.3...v1.3.4`](https://github.com/Cod-e-Codes/marchat/compare/v1.3.3...v1.3.4). Commits: **`git log v1.3.3..v1.3.4 --oneline`**. diff --git a/PROTOCOL.md b/PROTOCOL.md index 554c5e8..2d91bd3 100644 --- a/PROTOCOL.md +++ b/PROTOCOL.md @@ -74,7 +74,7 @@ Optional fields (`recipient`, `reaction`, etc.) are omitted from JSON when unset #### Fields - `sender` (string): Username of the sender. -- `content` (string): Message text. Empty if type is `file`. For `search`, carries the query string. +- `content` (string): Message text. Empty if type is `file`. For `search`, carries the query string. For unencrypted `text`, `dm`, and `edit`, the server rejects empty or whitespace-only `content` with a private System `text` reply (connection stays open; nothing is persisted or broadcast). When `encrypted` is `true`, `content` is opaque ciphertext and is not checked for emptiness. - `created_at` (string): RFC3339 timestamp. - `type` (string): Core types include `"text"`, `"file"`, and `"admin_command"`. See [Extended Message Types](#extended-message-types) for additional values. - `file` (object, optional): Present only when `type` is `"file"`. @@ -175,6 +175,7 @@ The server stores and relays opaque `content` (and encrypted file blobs) without - Broadcasts updated user list. - On message send: - Persists eligible messages to the configured SQL backend selected by `MARCHAT_DB_PATH` (SQLite path, PostgreSQL DSN, or MySQL DSN). + - Rejects unencrypted `text` / `dm` / `edit` with empty or whitespace-only `content` (System reply to sender; no persist or broadcast). Encrypted payloads are not emptiness-checked. - Delivers to all connected clients **or** only to members of a channel when `channel` is non-empty and `sender` is not `System` (see [Channels](#channels)). Direct messages use a separate path (sender and recipient only). - Reactions, read receipts, and last channel per user may be persisted server-side and replayed to reconnecting clients. - DM unread counters and DM thread hide/archive state are client-side UI state in the reference TUI, not server protocol fields. The reference client stores this local state under its client config directory. Opening a DM thread marks that thread read immediately in the reference client. diff --git a/README.md b/README.md index 7a8634e..8bcbd31 100644 --- a/README.md +++ b/README.md @@ -310,7 +310,7 @@ Run **`./marchat-client -doctor`** or **`./marchat-server -doctor`** for a text | Command | Description | Hotkey | |---------|-------------|--------| | `:ban ` | Permanent ban | `Ctrl+B` (with user selected) | -| `:kick ` | 24h temporary ban | `Ctrl+K` (with user selected) | +| `:kick ` | Disconnect online user + 24h temp ban | `Ctrl+K` (with user selected) | | `:unban ` | Remove permanent ban | `Ctrl+Shift+B` | | `:allow ` | Override kick early | `Ctrl+Shift+A` | | `:forcedisconnect ` | Force disconnect user | `Ctrl+F` (with user selected) | @@ -652,12 +652,13 @@ See [PLUGIN_ECOSYSTEM.md](PLUGIN_ECOSYSTEM.md) for the development guide and [ma ## Moderation System -**Temporary Kicks (24 hours):** -- `:kick ` or `Ctrl+K` for temporary discipline +**Temporary Kicks (24 hours, online only):** +- `:kick ` or `Ctrl+K` disconnects a connected user and applies a 24h temporary ban +- Fails when the user is not connected (use `:ban` for offline moderation) - Auto-allowed after 24 hours, or override early with `:allow` - Ideal for cooling-off periods -**Permanent Bans (indefinite):** +**Permanent Bans (offline-capable):** - `:ban ` or `Ctrl+B` for serious violations - Remains until manual `:unban` or `Ctrl+Shift+B` - Ideal for persistent troublemakers diff --git a/TESTING.md b/TESTING.md index c70384f..8e15e9d 100644 --- a/TESTING.md +++ b/TESTING.md @@ -62,22 +62,24 @@ Some client behavior is only verifiable in a real terminal emulator with mouse r | `cmd/server/main_test.go` | Server main function and startup | Flag parsing, configuration validation, TLS setup, admin normalization, `validateStartupConfig`, deprecated flags | | `cmd/server/subprocess_doctor_test.go` | Server binary smoke | `go run ./cmd/server -doctor` / `-doctor-json` subprocess (covers `main` early exits) | | `server/handlers_test.go` | Server-side request handling | Database operations, message insertion, visible handshake replay (`GetRecentMessagesForUser`), reconnect replay, DM limit under DM noise, IP extraction | -| `server/hub_test.go` | WebSocket hub management | User bans, kicks, connection management, non-blocking send verification | +| `server/hub_test.go` | WebSocket hub management | User bans, online-only kicks, connection management, non-blocking send verification | | `server/loadverify_ratelimit_test.go` | WebSocket read-pump rate limit | Window, burst (20), and cooldown behavior (same constants as `client.go`) | | `server/loadverify_bench_test.go` | Hub broadcast benchmarks (optional) | Channel vs system-wide fan-out, parallel senders, JSON marshal baseline; see [Optional hub load benchmarks](#optional-hub-load-benchmarks-server) | | `server/integration_test.go` | End-to-end workflows | Message flow, ban flow, WebSocket handshake replay on reconnect (`TestIntegrationWebSocketHandshakeReplayOnReconnect`), concurrent operations | | `server/admin_web_test.go` | Admin web interface | HTTP endpoints, authentication, admin panel functionality | | `server/config_ui_test.go` | Server configuration UI | Configuration management, environment handling | | `server/admin_panel_test.go` | Admin panel functionality | Admin-specific operations and controls | -| `server/db_test.go` | Database operations | Database initialization, schema setup | +| `server/db_test.go` | Database operations | `InitDB` SQLite DSN pragmas (`busy_timeout`, WAL), `:memory:`, query-join `&`, single-conn pool, short concurrent inserts, schema smoke | | `server/db_dialect_test.go` | SQL dialect helpers | DSN detection, Postgres placeholder rebinding, boolean SQL literals, MySQL DSN via `mysql.Config` | -| `server/db_ci_smoke_test.go` | CI DB smoke | Postgres/MySQL `InitDB`, `CreateSchema`, core tables, visible handshake replay query, search and pin SQL (env-gated) | +| `server/db_ci_smoke_test.go` | CI DB smoke | Postgres/MySQL `InitDB`, `CreateSchema`, core tables, pool not forced to 1, visible handshake replay query, search and pin SQL (env-gated) | | `server/message_state_test.go` | Durable reactions | Reaction persistence and replay helpers | | `server/config_test.go` | Server configuration | Server configuration logic and validation | | `server/client_test.go` | Server client management | WebSocket client initialization, message handling, admin operations, unknown admin command system reply (`TestHandleCommandUnknownAdminSendsSystemReply`), channel stamping (`TestStampClientChannelOverwritesSpoofedChannel`), non-admin unknown vs admin-only command replies | | `server/client_sender_spoof_test.go` | Sender identity enforcement | Integration tests that wire `sender` is ignored on text/file paths (`stampSenderTimedOutbound`) | | `server/client_file_limit_test.go` | File size limits | `fileMessageReadLimit` / `websocketReadLimit` (32 MiB DoS ceiling); System reply before close for modest wire oversize; connection stays usable after reject | | `server/client_nullbyte_test.go` | NUL content validation | `contentContainsNUL`, SQLite NUL insert baseline, integration reject-no-broadcast | +| `server/message_validate_test.go` | Plaintext empty helper | `plaintextContentEmpty` table-driven cases (empty, whitespace, encrypted opaque) | +| `server/client_empty_content_test.go` | Empty plaintext rejection | Integration reject for text/DM/edit; encrypted opaque accept; command path still works | | `server/health_test.go` | Server health monitoring | Health checks, system metrics, HTTP endpoints, concurrent access | | `plugin/sdk/plugin_test.go` | Plugin SDK | Message types, extended fields (channel, encrypted, message_id, recipient, edited), JSON serialization, omitempty validation, backwards-compat unknown-field handling | | `plugin/sdk/stdio_test.go` | Plugin SDK stdio | `HandlePluginRequest` / `RunIO` (init, message, command, shutdown), EOF handling | diff --git a/server/admin_panel.go b/server/admin_panel.go index f9a8c36..b7296be 100644 --- a/server/admin_panel.go +++ b/server/admin_panel.go @@ -2,6 +2,7 @@ package server import ( "database/sql" + "errors" "fmt" "log" "os" @@ -1617,7 +1618,13 @@ func (ap *AdminPanel) adminUsername() string { func (ap *AdminPanel) banUser(username string) tea.Cmd { return func() tea.Msg { - ap.hub.BanUser(username, ap.adminUsername()) + if err := ap.hub.BanUser(username, ap.adminUsername()); err != nil { + msg := fmt.Sprintf("ERROR: Failed to ban '%s': %v", username, err) + if errors.Is(err, ErrAdminSelfTarget) { + msg = "ERROR: You cannot ban yourself" + } + return actionMsg{success: false, message: msg} + } return actionMsg{ success: true, message: fmt.Sprintf("User '%s' has been banned", username), @@ -1643,7 +1650,17 @@ func (ap *AdminPanel) unbanUser(username string) tea.Cmd { func (ap *AdminPanel) kickUser(username string) tea.Cmd { return func() tea.Msg { - ap.hub.KickUser(username, ap.adminUsername()) + if err := ap.hub.KickUser(username, ap.adminUsername()); err != nil { + msg := fmt.Sprintf("ERROR: Failed to kick '%s': %v", username, err) + if errors.Is(err, ErrAdminSelfTarget) { + msg = "ERROR: You cannot kick yourself" + } else if errors.Is(err, ErrKickPermanentlyBanned) { + msg = fmt.Sprintf("ERROR: Cannot kick '%s': user is permanently banned", username) + } else if errors.Is(err, ErrKickNotConnected) { + msg = fmt.Sprintf("ERROR: Cannot kick '%s': user is not connected", username) + } + return actionMsg{success: false, message: msg} + } return actionMsg{ success: true, message: fmt.Sprintf("User '%s' has been kicked (24h)", username), diff --git a/server/admin_web.go b/server/admin_web.go index 4db40a1..e495772 100644 --- a/server/admin_web.go +++ b/server/admin_web.go @@ -9,6 +9,7 @@ import ( "encoding/base64" "encoding/hex" "encoding/json" + "errors" "fmt" "log" "net/http" @@ -594,9 +595,17 @@ func (w *WebAdminServer) handleUserAction(rw http.ResponseWriter, r *http.Reques switch req.Action { case "ban": - w.hub.BanUser(req.Username, "web-admin") - message = fmt.Sprintf("User '%s' has been banned", req.Username) - success = true + if err := w.hub.BanUser(req.Username, "web-admin"); err != nil { + success = false + if errors.Is(err, ErrAdminSelfTarget) { + message = "You cannot ban yourself" + } else { + message = fmt.Sprintf("Failed to ban '%s': %v", req.Username, err) + } + } else { + message = fmt.Sprintf("User '%s' has been banned", req.Username) + success = true + } case "unban": success = w.hub.UnbanUser(req.Username, "web-admin") if success { @@ -605,9 +614,21 @@ func (w *WebAdminServer) handleUserAction(rw http.ResponseWriter, r *http.Reques message = fmt.Sprintf("User '%s' was not found in ban list", req.Username) } case "kick": - w.hub.KickUser(req.Username, "web-admin") - message = fmt.Sprintf("User '%s' has been kicked (24h)", req.Username) - success = true + if err := w.hub.KickUser(req.Username, "web-admin"); err != nil { + success = false + if errors.Is(err, ErrAdminSelfTarget) { + message = "You cannot kick yourself" + } else if errors.Is(err, ErrKickPermanentlyBanned) { + message = fmt.Sprintf("Cannot kick '%s': user is permanently banned", req.Username) + } else if errors.Is(err, ErrKickNotConnected) { + message = fmt.Sprintf("Cannot kick '%s': user is not connected", req.Username) + } else { + message = fmt.Sprintf("Failed to kick '%s': %v", req.Username, err) + } + } else { + message = fmt.Sprintf("User '%s' has been kicked (24h)", req.Username) + success = true + } case "allow": success = w.hub.AllowUser(req.Username, "web-admin") if success { diff --git a/server/client.go b/server/client.go index 9a83d62..dafc44c 100644 --- a/server/client.go +++ b/server/client.go @@ -175,6 +175,15 @@ func (c *Client) readPump() { } continue } + if plaintextContentEmpty(msg.Content, msg.Encrypted) { + c.send <- shared.Message{ + Sender: "System", + Content: "Message not sent: empty content", + CreatedAt: time.Now(), + Type: shared.TextMessage, + } + continue + } if err := EditMessage(c.db, msg.MessageID, c.username, msg.Content, msg.Encrypted); err != nil { c.send <- shared.Message{ Sender: "System", @@ -233,6 +242,15 @@ func (c *Client) readPump() { } continue } + if plaintextContentEmpty(msg.Content, msg.Encrypted) { + c.send <- shared.Message{ + Sender: "System", + Content: "Message not sent: empty content", + CreatedAt: time.Now(), + Type: shared.TextMessage, + } + continue + } c.stampSenderTimedOutbound(&msg) msgID, err := InsertMessage(c.db, msg) if err != nil { @@ -406,6 +424,15 @@ func (c *Client) readPump() { } continue } + if plaintextContentEmpty(msg.Content, msg.Encrypted) { + c.send <- shared.Message{ + Sender: "System", + Content: "Message not sent: empty content", + CreatedAt: time.Now(), + Type: shared.TextMessage, + } + continue + } c.stampSenderTimedOutbound(&msg) if msg.Type == "" || msg.Type == shared.TextMessage { msgID, err := InsertMessage(c.db, msg) @@ -638,7 +665,23 @@ func (c *Client) handleCommand(command string) { } return } - c.hub.KickUser(targetUsername, c.username) + if err := c.hub.KickUser(targetUsername, c.username); err != nil { + content := "Failed to kick user: " + err.Error() + if errors.Is(err, ErrAdminSelfTarget) { + content = "You cannot kick yourself." + } else if errors.Is(err, ErrKickPermanentlyBanned) { + content = "Cannot kick '" + targetUsername + "': user is permanently banned." + } else if errors.Is(err, ErrKickNotConnected) { + content = "Cannot kick '" + targetUsername + "': user is not connected." + } + c.send <- shared.Message{ + Sender: "System", + Content: content, + CreatedAt: time.Now(), + Type: shared.TextMessage, + } + return + } c.send <- shared.Message{ Sender: "System", Content: "User '" + targetUsername + "' has been kicked (24 hour temporary ban).", @@ -666,7 +709,19 @@ func (c *Client) handleCommand(command string) { } return } - c.hub.BanUser(targetUsername, c.username) + if err := c.hub.BanUser(targetUsername, c.username); err != nil { + content := "Failed to ban user: " + err.Error() + if errors.Is(err, ErrAdminSelfTarget) { + content = "You cannot ban yourself." + } + c.send <- shared.Message{ + Sender: "System", + Content: content, + CreatedAt: time.Now(), + Type: shared.TextMessage, + } + return + } c.send <- shared.Message{ Sender: "System", Content: "User '" + targetUsername + "' has been permanently banned.", diff --git a/server/client_empty_content_test.go b/server/client_empty_content_test.go new file mode 100644 index 0000000..d4cae40 --- /dev/null +++ b/server/client_empty_content_test.go @@ -0,0 +1,357 @@ +package server + +import ( + "net/http/httptest" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/Cod-e-Codes/marchat/shared" + "github.com/gorilla/websocket" +) + +// readSystemReplyContaining reads until a System message containing substr arrives. +// Any ReadJSON error ends the search (gorilla treats read errors, including +// deadlines, as permanent - do not retry after err). +func readSystemReplyContaining(t *testing.T, conn *websocket.Conn, substr string, timeout time.Duration) bool { + t.Helper() + _ = conn.SetReadDeadline(time.Now().Add(timeout)) + for { + var msg shared.Message + if err := conn.ReadJSON(&msg); err != nil { + return false + } + if msg.Sender == "System" && strings.Contains(msg.Content, substr) { + return true + } + } +} + +func TestIntegrationEmptyTextRejectedNoBroadcast(t *testing.T) { + tdir := t.TempDir() + dbPath := filepath.Join(tdir, "test.db") + db, err := InitDB(dbPath) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub(tdir, tdir, "", db) + go hub.Run() + + handler := ServeWs(hub, db, nil, "admin-key", false, 10<<20, dbPath) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + + dial := func(username string) *websocket.Conn { + t.Helper() + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("dial: %v", err) + } + if err := conn.WriteJSON(shared.Handshake{Username: username}); err != nil { + t.Fatalf("handshake: %v", err) + } + return conn + } + + listener := dial("emptyListen1") + defer listener.Close() + drainWSJSONUntilIdle(listener, 200*time.Millisecond) + + sender := dial("emptySend1") + defer sender.Close() + // Do not drain sender: deadline errors are permanent on gorilla Conn, and we + // still need to read the System reject reply below. + time.Sleep(100 * time.Millisecond) + drainWSJSONUntilIdle(listener, 200*time.Millisecond) + + bad := shared.Message{ + Content: " ", + Type: shared.TextMessage, + } + if err := sender.WriteJSON(bad); err != nil { + t.Fatalf("send: %v", err) + } + + if !readSystemReplyContaining(t, sender, "empty content", time.Second) { + t.Fatal("expected System reply about empty content") + } + + var count int + if err := db.QueryRow(`SELECT COUNT(*) FROM messages WHERE TRIM(content) = '' OR content IS NULL`).Scan(&count); err != nil { + t.Fatalf("query: %v", err) + } + if count != 0 { + t.Fatalf("empty content must not be persisted, got %d rows", count) + } + + _ = listener.SetReadDeadline(time.Now().Add(400 * time.Millisecond)) + var peerMsg shared.Message + if err := listener.ReadJSON(&peerMsg); err == nil && peerMsg.Sender == "emptySend1" { + t.Fatal("peer should not receive sender's empty message") + } +} + +func TestIntegrationEmptyDMRejected(t *testing.T) { + tdir := t.TempDir() + dbPath := filepath.Join(tdir, "test.db") + db, err := InitDB(dbPath) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub(tdir, tdir, "", db) + go hub.Run() + + handler := ServeWs(hub, db, nil, "admin-key", false, 10<<20, dbPath) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + + dial := func(username string) *websocket.Conn { + t.Helper() + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("dial: %v", err) + } + if err := conn.WriteJSON(shared.Handshake{Username: username}); err != nil { + t.Fatalf("handshake: %v", err) + } + return conn + } + + peer := dial("emptyDMPeer") + defer peer.Close() + drainWSJSONUntilIdle(peer, 200*time.Millisecond) + + sender := dial("emptyDMSend") + defer sender.Close() + time.Sleep(100 * time.Millisecond) + drainWSJSONUntilIdle(peer, 200*time.Millisecond) + + bad := shared.Message{ + Content: "", + Type: shared.DirectMessage, + Recipient: "emptyDMPeer", + } + if err := sender.WriteJSON(bad); err != nil { + t.Fatalf("send: %v", err) + } + + if !readSystemReplyContaining(t, sender, "empty content", time.Second) { + t.Fatal("expected System reply about empty content") + } + + var count int + if err := db.QueryRow(`SELECT COUNT(*) FROM messages WHERE recipient = ?`, "emptyDMPeer").Scan(&count); err != nil { + t.Fatalf("query: %v", err) + } + if count != 0 { + t.Fatalf("empty DM must not be persisted, got %d rows", count) + } + + _ = peer.SetReadDeadline(time.Now().Add(400 * time.Millisecond)) + var peerMsg shared.Message + if err := peer.ReadJSON(&peerMsg); err == nil && peerMsg.Sender == "emptyDMSend" { + t.Fatal("peer should not receive rejected empty DM") + } +} + +func TestIntegrationEmptyEditRejected(t *testing.T) { + tdir := t.TempDir() + dbPath := filepath.Join(tdir, "test.db") + db, err := InitDB(dbPath) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub(tdir, tdir, "", db) + go hub.Run() + + handler := ServeWs(hub, db, nil, "admin-key", false, 10<<20, dbPath) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.WriteJSON(shared.Handshake{Username: "emptyEdit1"}); err != nil { + t.Fatalf("handshake: %v", err) + } + time.Sleep(100 * time.Millisecond) + + orig := shared.Message{ + Content: "keep-me", + Type: shared.TextMessage, + } + if err := conn.WriteJSON(orig); err != nil { + t.Fatalf("send orig: %v", err) + } + // Wait for persist without draining (deadline would poison this Conn). + deadline := time.Now().Add(time.Second) + var msgID int64 + for time.Now().Before(deadline) { + err := db.QueryRow(`SELECT message_id FROM messages WHERE content = ?`, "keep-me").Scan(&msgID) + if err == nil && msgID > 0 { + break + } + time.Sleep(20 * time.Millisecond) + } + if msgID <= 0 { + t.Fatal("lookup message_id: original message not persisted") + } + + edit := shared.Message{ + Content: " ", + Type: shared.EditMessageType, + MessageID: msgID, + } + if err := conn.WriteJSON(edit); err != nil { + t.Fatalf("send edit: %v", err) + } + + if !readSystemReplyContaining(t, conn, "empty content", time.Second) { + t.Fatal("expected System reply about empty content") + } + + var content string + if err := db.QueryRow(`SELECT content FROM messages WHERE message_id = ?`, msgID).Scan(&content); err != nil { + t.Fatalf("query: %v", err) + } + if content != "keep-me" { + t.Fatalf("edit must not replace content, got %q", content) + } +} + +func TestIntegrationEncryptedOpaqueTextAccepted(t *testing.T) { + tdir := t.TempDir() + dbPath := filepath.Join(tdir, "test.db") + db, err := InitDB(dbPath) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub(tdir, tdir, "", db) + go hub.Run() + + handler := ServeWs(hub, db, nil, "admin-key", false, 10<<20, dbPath) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.WriteJSON(shared.Handshake{Username: "emptyEnc1"}); err != nil { + t.Fatalf("handshake: %v", err) + } + time.Sleep(100 * time.Millisecond) + + const blob = "bm9uY2UxMjM0NTY3ODkwYWJjZGVmZ2hpamtsbW5vcA==" + msg := shared.Message{ + Content: blob, + Type: shared.TextMessage, + Encrypted: true, + } + if err := conn.WriteJSON(msg); err != nil { + t.Fatalf("send: %v", err) + } + + _ = conn.SetReadDeadline(time.Now().Add(time.Second)) + var gotEcho bool + for { + var got shared.Message + if err := conn.ReadJSON(&got); err != nil { + break + } + if got.Sender == "System" && strings.Contains(got.Content, "empty content") { + t.Fatal("encrypted opaque content must not be rejected as empty") + } + if got.Content == blob && got.Encrypted { + gotEcho = true + break + } + } + if !gotEcho { + t.Fatal("expected encrypted message echo/broadcast") + } + + var count int + if err := db.QueryRow(`SELECT COUNT(*) FROM messages WHERE content = ?`, blob).Scan(&count); err != nil { + t.Fatalf("query: %v", err) + } + if count != 1 { + t.Fatalf("encrypted opaque content should be persisted, got %d rows", count) + } +} + +func TestIntegrationCommandPathStillWorksWithEmptyCheck(t *testing.T) { + tdir := t.TempDir() + dbPath := filepath.Join(tdir, "test.db") + db, err := InitDB(dbPath) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub(tdir, tdir, "", db) + go hub.Run() + + handler := ServeWs(hub, db, nil, "admin-key", false, 10<<20, dbPath) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.WriteJSON(shared.Handshake{Username: "emptyCmd1"}); err != nil { + t.Fatalf("handshake: %v", err) + } + time.Sleep(100 * time.Millisecond) + + cmd := shared.Message{ + Content: ":hello", + Type: shared.TextMessage, + } + if err := conn.WriteJSON(cmd); err != nil { + t.Fatalf("send: %v", err) + } + + _ = conn.SetReadDeadline(time.Now().Add(time.Second)) + for { + var got shared.Message + if err := conn.ReadJSON(&got); err != nil { + t.Fatal("expected System reply from command path") + } + if got.Sender == "System" && strings.Contains(got.Content, "empty content") { + t.Fatal("command path must not hit empty-content reject") + } + if got.Sender == "System" && strings.TrimSpace(got.Content) != "" { + return + } + } +} diff --git a/server/db.go b/server/db.go index 020c2e6..975b08f 100644 --- a/server/db.go +++ b/server/db.go @@ -12,6 +12,11 @@ import ( _ "modernc.org/sqlite" ) +// sqliteConnPragmas are applied on every new SQLite connection via the DSN +// (modernc.org/sqlite v1.55.0 shorthands + _pragma). One-shot PRAGMA Exec after +// Open only affects the connection that ran it, so these must live in the DSN. +const sqliteConnPragmas = "_busy_timeout=5000&_journal_mode=WAL&_synchronous=NORMAL&_pragma=cache_size(10000)&_pragma=temp_store(MEMORY)" + func detectDriver(conn string) (string, DBDialect, string) { v := strings.TrimSpace(conn) switch { @@ -41,6 +46,56 @@ func prepareMySQLDSN(dsn string) (string, error) { return cfg.FormatDSN(), nil } +// appendSQLiteDSNPragmas joins per-connection PRAGMA query params onto a SQLite +// path or DSN, using & when a query string is already present. +func appendSQLiteDSNPragmas(path string) string { + if strings.Contains(path, "?") { + return path + "&" + sqliteConnPragmas + } + return path + "?" + sqliteConnPragmas +} + +// isSQLiteMemoryDSN reports whether the SQLite DSN is an in-memory database +// where WAL may not stick (PRAGMA journal_mode often remains "memory"). +func isSQLiteMemoryDSN(dsn string) bool { + base := dsn + if i := strings.Index(dsn, "?"); i >= 0 { + base = dsn[:i] + } + base = strings.TrimSpace(base) + if base == ":memory:" || strings.EqualFold(base, "file::memory:") { + return true + } + q := "" + if i := strings.Index(dsn, "?"); i >= 0 { + q = strings.ToLower(dsn[i+1:]) + } + return strings.Contains(q, "mode=memory") +} + +func verifySQLitePragmas(db *sql.DB, dsn string) error { + var busyTimeout int + if err := db.QueryRow("PRAGMA busy_timeout;").Scan(&busyTimeout); err != nil { + return fmt.Errorf("verify PRAGMA busy_timeout: %w", err) + } + if busyTimeout <= 0 { + return fmt.Errorf("PRAGMA busy_timeout = %d, want > 0", busyTimeout) + } + + if isSQLiteMemoryDSN(dsn) { + return nil + } + + var journalMode string + if err := db.QueryRow("PRAGMA journal_mode;").Scan(&journalMode); err != nil { + return fmt.Errorf("verify PRAGMA journal_mode: %w", err) + } + if !strings.EqualFold(journalMode, "wal") { + return fmt.Errorf("PRAGMA journal_mode = %q, want wal", journalMode) + } + return nil +} + func InitDB(conn string) (*sql.DB, error) { driver, dialect, dsn := detectDriver(conn) if dialect == DialectMySQL { @@ -51,6 +106,10 @@ func InitDB(conn string) (*sql.DB, error) { } } + if dialect == DialectSQLite { + dsn = appendSQLiteDSNPragmas(dsn) + } + db, err := sql.Open(driver, dsn) if err != nil { return nil, fmt.Errorf("failed to open %s database: %w", dialect, err) @@ -65,36 +124,16 @@ func InitDB(conn string) (*sql.DB, error) { setDBDialect(db, dialect) if dialect == DialectSQLite { - // Enable WAL mode for better concurrency and performance - _, err = db.Exec("PRAGMA journal_mode=WAL;") - if err != nil { - log.Printf("Warning: Could not enable WAL mode: %v", err) - } else { - // Verify WAL mode was actually enabled - var journalMode string - err = db.QueryRow("PRAGMA journal_mode;").Scan(&journalMode) - if err != nil { - log.Printf("Warning: Could not verify journal mode: %v", err) - } else { - log.Printf("Database journal mode set to %s for improved concurrency", journalMode) - } - } + // Single connection: SQLite does not benefit from a multi-conn pool and + // concurrent writers on separate connections race for the write lock. + db.SetMaxOpenConns(1) + db.SetMaxIdleConns(1) - // Set additional performance optimizations - _, err = db.Exec("PRAGMA synchronous=NORMAL;") - if err != nil { - log.Printf("Warning: Could not set synchronous mode: %v", err) - } - - _, err = db.Exec("PRAGMA cache_size=10000;") - if err != nil { - log.Printf("Warning: Could not set cache size: %v", err) - } - - _, err = db.Exec("PRAGMA temp_store=MEMORY;") - if err != nil { - log.Printf("Warning: Could not set temp store: %v", err) + if err := verifySQLitePragmas(db, dsn); err != nil { + db.Close() + return nil, fmt.Errorf("sqlite pragma verification failed: %w", err) } + log.Printf("SQLite connected with per-connection pragmas (busy_timeout, WAL for file DBs) and MaxOpenConns=1") } return db, nil diff --git a/server/db_ci_smoke_test.go b/server/db_ci_smoke_test.go index 0dbe910..0554434 100644 --- a/server/db_ci_smoke_test.go +++ b/server/db_ci_smoke_test.go @@ -27,6 +27,10 @@ func TestPostgresInitDBAndSchemaSmoke(t *testing.T) { if getDBDialect(db) != DialectPostgres { t.Fatalf("dialect = %v, want postgres", getDBDialect(db)) } + // SQLite-only: InitDB must not force MaxOpenConns(1) on remote backends. + if db.Stats().MaxOpenConnections == 1 { + t.Fatal("postgres MaxOpenConnections unexpectedly 1 (SQLite-only pool limit)") + } CreateSchema(db) assertCISmokeTables(t, db, "postgres") @@ -48,6 +52,10 @@ func TestMySQLInitDBAndSchemaSmoke(t *testing.T) { if getDBDialect(db) != DialectMySQL { t.Fatalf("dialect = %v, want mysql", getDBDialect(db)) } + // SQLite-only: InitDB must not force MaxOpenConns(1) on remote backends. + if db.Stats().MaxOpenConnections == 1 { + t.Fatal("mysql MaxOpenConnections unexpectedly 1 (SQLite-only pool limit)") + } CreateSchema(db) assertCISmokeTables(t, db, "mysql") diff --git a/server/db_test.go b/server/db_test.go index e6ed365..a09d9ed 100644 --- a/server/db_test.go +++ b/server/db_test.go @@ -2,9 +2,34 @@ package server import ( "path/filepath" + "strings" + "sync" "testing" + "time" + + "github.com/Cod-e-Codes/marchat/shared" ) +func TestAppendSQLiteDSNPragmas(t *testing.T) { + t.Parallel() + + plain := appendSQLiteDSNPragmas("/tmp/chat.db") + if !strings.HasPrefix(plain, "/tmp/chat.db?") { + t.Fatalf("plain path DSN = %q, want ?-joined query", plain) + } + if !strings.Contains(plain, "_busy_timeout=5000") { + t.Fatalf("plain path missing busy_timeout: %q", plain) + } + + withQuery := appendSQLiteDSNPragmas("file:test.db?mode=rwc") + if !strings.HasPrefix(withQuery, "file:test.db?mode=rwc&") { + t.Fatalf("query path DSN = %q, want &-joined pragmas", withQuery) + } + if strings.Contains(withQuery, "?mode=rwc?") { + t.Fatalf("double ? in DSN: %q", withQuery) + } +} + func TestInitDBAndSchema(t *testing.T) { tdir := t.TempDir() dbPath := filepath.Join(tdir, "test.db") @@ -37,3 +62,152 @@ func TestInitDBAndSchema(t *testing.T) { t.Fatalf("user_message_state table not created") } } + +func TestInitDBSQLiteFilePragmasAndPool(t *testing.T) { + tdir := t.TempDir() + dbPath := filepath.Join(tdir, "pragmas.db") + db, err := InitDB(dbPath) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + + var busyTimeout int + if err := db.QueryRow("PRAGMA busy_timeout;").Scan(&busyTimeout); err != nil { + t.Fatalf("PRAGMA busy_timeout: %v", err) + } + if busyTimeout <= 0 { + t.Fatalf("busy_timeout = %d, want > 0", busyTimeout) + } + + var journalMode string + if err := db.QueryRow("PRAGMA journal_mode;").Scan(&journalMode); err != nil { + t.Fatalf("PRAGMA journal_mode: %v", err) + } + if !strings.EqualFold(journalMode, "wal") { + t.Fatalf("journal_mode = %q, want wal", journalMode) + } + + stats := db.Stats() + if stats.MaxOpenConnections != 1 { + t.Fatalf("MaxOpenConnections = %d, want 1", stats.MaxOpenConnections) + } +} + +func TestInitDBSQLiteMemory(t *testing.T) { + db, err := InitDB(":memory:") + if err != nil { + t.Fatalf("InitDB(:memory:): %v", err) + } + defer db.Close() + + var busyTimeout int + if err := db.QueryRow("PRAGMA busy_timeout;").Scan(&busyTimeout); err != nil { + t.Fatalf("PRAGMA busy_timeout: %v", err) + } + if busyTimeout <= 0 { + t.Fatalf("busy_timeout = %d, want > 0", busyTimeout) + } + + CreateSchema(db) + var n int + if err := db.QueryRow("SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='messages'").Scan(&n); err != nil { + t.Fatalf("messages table: %v", err) + } + if n == 0 { + t.Fatal("messages table not created") + } +} + +func TestInitDBSQLitePathWithExistingQuery(t *testing.T) { + tdir := t.TempDir() + // modernc accepts file: URIs; mode=rwc creates the file if missing. + path := filepath.ToSlash(filepath.Join(tdir, "query.db")) + dsn := "file:" + path + "?mode=rwc" + db, err := InitDB(dsn) + if err != nil { + t.Fatalf("InitDB(%q): %v", dsn, err) + } + defer db.Close() + + var busyTimeout int + if err := db.QueryRow("PRAGMA busy_timeout;").Scan(&busyTimeout); err != nil { + t.Fatalf("PRAGMA busy_timeout: %v", err) + } + if busyTimeout <= 0 { + t.Fatalf("busy_timeout = %d, want > 0", busyTimeout) + } + + var journalMode string + if err := db.QueryRow("PRAGMA journal_mode;").Scan(&journalMode); err != nil { + t.Fatalf("PRAGMA journal_mode: %v", err) + } + if !strings.EqualFold(journalMode, "wal") { + t.Fatalf("journal_mode = %q, want wal", journalMode) + } +} + +func TestIsSQLiteMemoryDSN(t *testing.T) { + t.Parallel() + cases := []struct { + dsn string + want bool + }{ + {":memory:", true}, + {":memory:?_busy_timeout=5000", true}, + {"file::memory:", true}, + {"file:memdb?mode=memory", true}, + {"/tmp/chat.db", false}, + {"file:/tmp/chat.db?mode=rwc", false}, + } + for _, tc := range cases { + if got := isSQLiteMemoryDSN(tc.dsn); got != tc.want { + t.Errorf("isSQLiteMemoryDSN(%q) = %v, want %v", tc.dsn, got, tc.want) + } + } +} + +func TestInitDBSQLiteConcurrentInserts(t *testing.T) { + tdir := t.TempDir() + db, err := InitDB(filepath.Join(tdir, "contention.db")) + if err != nil { + t.Fatalf("InitDB: %v", err) + } + defer db.Close() + CreateSchema(db) + + const goroutines = 8 + const perG = 20 + var wg sync.WaitGroup + errCh := make(chan error, goroutines*perG) + for g := 0; g < goroutines; g++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + for i := 0; i < perG; i++ { + _, err := InsertMessage(db, shared.Message{ + Sender: "user", + Content: "msg", + CreatedAt: time.Now(), + Channel: "general", + }) + if err != nil { + errCh <- err + } + } + }(g) + } + wg.Wait() + close(errCh) + for err := range errCh { + t.Fatalf("concurrent insert: %v", err) + } + + var n int + if err := db.QueryRow(`SELECT COUNT(*) FROM messages`).Scan(&n); err != nil { + t.Fatalf("count: %v", err) + } + if n != goroutines*perG { + t.Fatalf("message count = %d, want %d", n, goroutines*perG) + } +} diff --git a/server/hub.go b/server/hub.go index d1cd904..9deef77 100644 --- a/server/hub.go +++ b/server/hub.go @@ -2,6 +2,7 @@ package server import ( "database/sql" + "errors" "log" "strings" "sync" @@ -11,6 +12,17 @@ import ( "github.com/Cod-e-Codes/marchat/shared" ) +// Sentinel errors for KickUser / BanUser so callers can map clear replies +// and claim success only when err == nil. +var ( + // ErrAdminSelfTarget is returned when an admin tries to kick or ban themselves. + ErrAdminSelfTarget = errors.New("cannot kick or ban yourself") + // ErrKickPermanentlyBanned is returned when KickUser targets a permanently banned user. + ErrKickPermanentlyBanned = errors.New("cannot kick a permanently banned user") + // ErrKickNotConnected is returned when KickUser targets a user with no active connection. + ErrKickNotConnected = errors.New("user is not connected") +) + type Hub struct { clients map[*Client]bool usernames map[string]struct{} @@ -84,7 +96,13 @@ func (h *Hub) ReleaseUsername(username string) { // The ban state is recorded under banMutex, then the lock is released before // kicking the connected client so that a blocked send channel cannot hold // banMutex and stall all other ban/kick callers. -func (h *Hub) BanUser(username string, adminUsername string) { +// Returns ErrAdminSelfTarget when username matches adminUsername (case-insensitive). +// Offline bans are still allowed when the target is a different user. +func (h *Hub) BanUser(username string, adminUsername string) error { + if strings.EqualFold(username, adminUsername) { + return ErrAdminSelfTarget + } + h.banMutex.Lock() lowerUsername := strings.ToLower(username) @@ -119,6 +137,7 @@ func (h *Hub) BanUser(username string, adminUsername string) { h.banMutex.Unlock() h.kickUser(username, "You have been permanently banned by an administrator") + return nil } // UnbanUser removes a user from the ban list @@ -180,8 +199,34 @@ func (h *Hub) IsUserBanned(username string) bool { return false } -// kickUser forcibly disconnects a user by username. +// disconnectClient forcibly disconnects a connected client. // Uses a non-blocking send so the caller never stalls on a full channel. +func (h *Hub) disconnectClient(target *Client, reason string) { + if target == nil { + return + } + + username := target.username + log.Printf("[ADMIN] Kicking user '%s' (IP: %s) - Reason: %s", username, target.ipAddr, reason) + + kickMsg := shared.Message{ + Sender: "System", + Content: "You have been kicked by an administrator: " + reason, + CreatedAt: time.Now(), + Type: shared.TextMessage, + } + select { + case target.send <- kickMsg: + default: + log.Printf("[ADMIN] Could not deliver kick message to %s (send buffer full)", username) + } + + if target.conn != nil { + target.conn.Close() + } +} + +// kickUser forcibly disconnects a user by username. func (h *Hub) kickUser(username string, reason string) { h.clientsMutex.RLock() var target *Client @@ -198,27 +243,37 @@ func (h *Hub) kickUser(username string, reason string) { return } - log.Printf("[ADMIN] Kicking user '%s' (IP: %s) - Reason: %s", username, target.ipAddr, reason) + h.disconnectClient(target, reason) +} - kickMsg := shared.Message{ - Sender: "System", - Content: "You have been kicked by an administrator: " + reason, - CreatedAt: time.Now(), - Type: shared.TextMessage, +// KickUser disconnects a connected user and temporarily bans them for 24 hours. +// The target must have an active WebSocket connection; offline users are not +// kicked or temp-banned (use BanUser for offline moderation). +// Like BanUser, banMutex is released before the disconnect to avoid holding the +// lock across a potentially blocking channel send. +// Returns ErrAdminSelfTarget when username matches adminUsername (case-insensitive), +// checked before any tempKicks write. Returns ErrKickNotConnected when the user is +// not connected. Returns ErrKickPermanentlyBanned when the target is already +// permanently banned. +func (h *Hub) KickUser(username string, adminUsername string) error { + if strings.EqualFold(username, adminUsername) { + return ErrAdminSelfTarget } - select { - case target.send <- kickMsg: - default: - log.Printf("[ADMIN] Could not deliver kick message to %s (send buffer full)", username) + + h.clientsMutex.RLock() + var target *Client + for client := range h.clients { + if strings.EqualFold(client.username, username) { + target = client + break + } } + h.clientsMutex.RUnlock() - target.conn.Close() -} + if target == nil { + return ErrKickNotConnected + } -// KickUser temporarily bans a user for 24 hours. -// Like BanUser, the lock is released before the actual disconnect to avoid -// holding banMutex across a potentially blocking channel send. -func (h *Hub) KickUser(username string, adminUsername string) { h.banMutex.Lock() lowerUsername := strings.ToLower(username) @@ -227,7 +282,7 @@ func (h *Hub) KickUser(username string, adminUsername string) { if _, isPermanentlyBanned := h.bans[lowerUsername]; isPermanentlyBanned { h.banMutex.Unlock() log.Printf("[ADMIN] Cannot kick '%s' - user is permanently banned", username) - return + return ErrKickPermanentlyBanned } // Add to temporary kicks for 24 hours @@ -257,7 +312,8 @@ func (h *Hub) KickUser(username string, adminUsername string) { h.banMutex.Unlock() - h.kickUser(username, "You have been kicked by an administrator (24 hour temporary ban)") + h.disconnectClient(target, "You have been kicked by an administrator (24 hour temporary ban)") + return nil } // AllowUser removes a user from temporary kick list (override early) diff --git a/server/hub_test.go b/server/hub_test.go index 10f155c..760692c 100644 --- a/server/hub_test.go +++ b/server/hub_test.go @@ -2,6 +2,7 @@ package server import ( "database/sql" + "errors" "sort" "strings" "testing" @@ -11,6 +12,23 @@ import ( _ "modernc.org/sqlite" ) +func registerTestClient(hub *Hub, username string) *Client { + client := &Client{ + username: username, + send: make(chan interface{}, 10), + } + hub.clientsMutex.Lock() + hub.clients[client] = true + hub.clientsMutex.Unlock() + return client +} + +func countBanHistoryRows(db *sql.DB, username string) (int, error) { + var count int + err := db.QueryRow(`SELECT COUNT(*) FROM ban_history WHERE username = ?`, strings.ToLower(username)).Scan(&count) + return count, err +} + func TestNewHub(t *testing.T) { // Create a test database db, err := sql.Open("sqlite", ":memory:") @@ -79,7 +97,9 @@ func TestHubBanUser(t *testing.T) { adminUsername := "admin" // Test banning a user - hub.BanUser(username, adminUsername) + if err := hub.BanUser(username, adminUsername); err != nil { + t.Fatalf("BanUser returned unexpected error: %v", err) + } // Check if user is banned if !hub.IsUserBanned(username) { @@ -114,7 +134,9 @@ func TestHubUnbanUser(t *testing.T) { adminUsername := "admin" // First ban the user - hub.BanUser(username, adminUsername) + if err := hub.BanUser(username, adminUsername); err != nil { + t.Fatalf("BanUser returned unexpected error: %v", err) + } if !hub.IsUserBanned(username) { t.Error("User should be banned") } @@ -144,26 +166,186 @@ func TestHubKickUser(t *testing.T) { } defer db.Close() - // Create schema for database operations CreateSchema(db) hub := NewHub("./plugins", "./data", "http://registry.example.com", db) username := "testuser" adminUsername := "admin" + registerTestClient(hub, username) - // Test kicking a user - hub.KickUser(username, adminUsername) + if err := hub.KickUser(username, adminUsername); err != nil { + t.Fatalf("KickUser returned unexpected error: %v", err) + } - // Check if user is kicked (temporarily banned) if !hub.IsUserBanned(username) { t.Error("User should be kicked") } - // Check case insensitive if !hub.IsUserBanned(strings.ToUpper(username)) { t.Error("Kick should be case insensitive") } + + hub.banMutex.RLock() + kickExpiry, exists := hub.tempKicks[strings.ToLower(username)] + hub.banMutex.RUnlock() + if !exists { + t.Fatal("KickUser should write tempKicks for connected user") + } + until := time.Until(kickExpiry) + if until < 23*time.Hour || until > 24*time.Hour { + t.Errorf("kick expiry should be about 24h from now, got remaining %v", until) + } + + rows, err := countBanHistoryRows(db, username) + if err != nil { + t.Fatalf("countBanHistoryRows: %v", err) + } + if rows != 1 { + t.Errorf("kick should record one ban_history row, got %d", rows) + } +} + +func TestHubKickUserOfflineReturnsError(t *testing.T) { + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("Failed to open test database: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub("./plugins", "./data", "http://registry.example.com", db) + username := "offlineuser" + adminUsername := "admin" + + err = hub.KickUser(username, adminUsername) + if !errors.Is(err, ErrKickNotConnected) { + t.Fatalf("KickUser offline: got %v, want ErrKickNotConnected", err) + } + + if hub.IsUserBanned(username) { + t.Error("offline kick must not ban user") + } + + hub.banMutex.RLock() + _, inTemp := hub.tempKicks[strings.ToLower(username)] + hub.banMutex.RUnlock() + if inTemp { + t.Error("offline kick must not write tempKicks") + } + + rows, err := countBanHistoryRows(db, username) + if err != nil { + t.Fatalf("countBanHistoryRows: %v", err) + } + if rows != 0 { + t.Errorf("offline kick must not record ban_history, got %d rows", rows) + } +} + +func TestHubKickUserCaseInsensitiveOnlineMatch(t *testing.T) { + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("Failed to open test database: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub("./plugins", "./data", "http://registry.example.com", db) + registerTestClient(hub, "TestUser") + adminUsername := "admin" + + if err := hub.KickUser("testuser", adminUsername); err != nil { + t.Fatalf("KickUser case-insensitive match: %v", err) + } + if !hub.IsUserBanned("TESTUSER") { + t.Error("kick should ban case-insensitive username") + } +} + +func TestHubRejectsSelfKickAndBan(t *testing.T) { + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("Failed to open test database: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub("./plugins", "./data", "http://registry.example.com", db) + adminUsername := "Alice" + + cases := []string{"Alice", "alice", "ALICE", "aLiCe"} + for _, target := range cases { + err := hub.KickUser(target, adminUsername) + if !errors.Is(err, ErrAdminSelfTarget) { + t.Fatalf("KickUser(%q, %q): got %v, want ErrAdminSelfTarget", target, adminUsername, err) + } + hub.banMutex.RLock() + _, inTemp := hub.tempKicks[strings.ToLower(target)] + _, inBans := hub.bans[strings.ToLower(target)] + hub.banMutex.RUnlock() + if inTemp { + t.Fatalf("self-kick %q must not write tempKicks", target) + } + if inBans { + t.Fatalf("self-kick %q must not write permanent bans", target) + } + if hub.IsUserBanned(target) { + t.Fatalf("self-kick %q must leave user unbanned", target) + } + + err = hub.BanUser(target, adminUsername) + if !errors.Is(err, ErrAdminSelfTarget) { + t.Fatalf("BanUser(%q, %q): got %v, want ErrAdminSelfTarget", target, adminUsername, err) + } + hub.banMutex.RLock() + _, inTemp = hub.tempKicks[strings.ToLower(target)] + _, inBans = hub.bans[strings.ToLower(target)] + hub.banMutex.RUnlock() + if inTemp { + t.Fatalf("self-ban %q must not write tempKicks", target) + } + if inBans { + t.Fatalf("self-ban %q must not write permanent bans", target) + } + if hub.IsUserBanned(target) { + t.Fatalf("self-ban %q must leave user unbanned", target) + } + } +} + +func TestHubKickPermanentlyBannedReturnsError(t *testing.T) { + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("Failed to open test database: %v", err) + } + defer db.Close() + CreateSchema(db) + + hub := NewHub("./plugins", "./data", "http://registry.example.com", db) + username := "banneduser" + adminUsername := "admin" + registerTestClient(hub, username) + + if err := hub.BanUser(username, adminUsername); err != nil { + t.Fatalf("BanUser returned unexpected error: %v", err) + } + + err = hub.KickUser(username, adminUsername) + if !errors.Is(err, ErrKickPermanentlyBanned) { + t.Fatalf("KickUser of permanently banned user: got %v, want ErrKickPermanentlyBanned", err) + } + + hub.banMutex.RLock() + _, inTemp := hub.tempKicks[strings.ToLower(username)] + _, inBans := hub.bans[strings.ToLower(username)] + hub.banMutex.RUnlock() + if inTemp { + t.Error("kick of permanently banned user must not write tempKicks") + } + if !inBans { + t.Error("permanent ban must remain after failed kick") + } } func TestHubAllowUser(t *testing.T) { @@ -180,9 +362,12 @@ func TestHubAllowUser(t *testing.T) { username := "testuser" adminUsername := "admin" + registerTestClient(hub, username) // First kick the user - hub.KickUser(username, adminUsername) + if err := hub.KickUser(username, adminUsername); err != nil { + t.Fatalf("KickUser returned unexpected error: %v", err) + } if !hub.IsUserBanned(username) { t.Error("User should be kicked") } @@ -219,21 +404,29 @@ func TestHubBanOverridesKick(t *testing.T) { username := "testuser" adminUsername := "admin" + registerTestClient(hub, username) // First kick the user - hub.KickUser(username, adminUsername) + if err := hub.KickUser(username, adminUsername); err != nil { + t.Fatalf("KickUser returned unexpected error: %v", err) + } if !hub.IsUserBanned(username) { t.Error("User should be kicked") } // Now ban the user (should override kick) - hub.BanUser(username, adminUsername) + if err := hub.BanUser(username, adminUsername); err != nil { + t.Fatalf("BanUser returned unexpected error: %v", err) + } if !hub.IsUserBanned(username) { t.Error("User should be banned") } // Try to kick a permanently banned user (should not work) - hub.KickUser(username, adminUsername) + err = hub.KickUser(username, adminUsername) + if !errors.Is(err, ErrKickPermanentlyBanned) { + t.Fatalf("KickUser of permanently banned user: got %v, want ErrKickPermanentlyBanned", err) + } if !hub.IsUserBanned(username) { t.Error("Permanently banned user should remain banned") } @@ -254,8 +447,11 @@ func TestHubCleanupExpiredBans(t *testing.T) { username := "testuser" adminUsername := "admin" - // Kick a user (24 hour temporary ban) - hub.KickUser(username, adminUsername) + // Kick a user (24 hour temporary ban) via connected client + registerTestClient(hub, username) + if err := hub.KickUser(username, adminUsername); err != nil { + t.Fatalf("KickUser returned unexpected error: %v", err) + } if !hub.IsUserBanned(username) { t.Error("User should be kicked") } @@ -331,7 +527,9 @@ func TestHubBanCaseInsensitive(t *testing.T) { adminUsername := "admin" // Ban user with mixed case - hub.BanUser(username, adminUsername) + if err := hub.BanUser(username, adminUsername); err != nil { + t.Fatalf("BanUser returned unexpected error: %v", err) + } // Test various case combinations testCases := []string{ @@ -365,7 +563,9 @@ func TestHubMultipleBansAndKicks(t *testing.T) { // Ban multiple users for _, user := range users { - hub.BanUser(user, adminUsername) + if err := hub.BanUser(user, adminUsername); err != nil { + t.Fatalf("BanUser(%s) returned unexpected error: %v", user, err) + } } // Check all users are banned @@ -408,23 +608,31 @@ func TestHubConcurrentBanOperations(t *testing.T) { username := "testuser" adminUsername := "admin" + registerTestClient(hub, username) + registerTestClient(hub, username+"2") // Test concurrent ban/unban operations done := make(chan bool, 2) - // Goroutine 1: Ban and unban user + // Goroutine 1: Ban, kick, and unban/allow user go func() { for i := 0; i < 100; i++ { - hub.BanUser(username, adminUsername) + _ = hub.BanUser(username, adminUsername) + _ = hub.KickUser(username, adminUsername) hub.UnbanUser(username, adminUsername) + hub.AllowUser(username, adminUsername) } done <- true }() - // Goroutine 2: Check if user is banned + // Goroutine 2: Check if user is banned and alternate kick/ban go func() { for i := 0; i < 100; i++ { hub.IsUserBanned(username) + _ = hub.KickUser(username+"2", adminUsername) + _ = hub.BanUser(username+"3", adminUsername) + hub.UnbanUser(username+"3", adminUsername) + hub.AllowUser(username+"2", adminUsername) } done <- true }() @@ -434,7 +642,7 @@ func TestHubConcurrentBanOperations(t *testing.T) { <-done // Final state should be consistent - // The user should not be banned after the unban in the first goroutine + // The user should not be banned after the unban/allow in the first goroutine if hub.IsUserBanned(username) { t.Error("User should not be banned after concurrent operations") } @@ -487,7 +695,7 @@ func TestKickUserNonBlocking(t *testing.T) { client := &Client{ username: "victim", send: make(chan interface{}, 1), - conn: nil, // conn.Close will be skipped via nil check in test + conn: nil, // nil-safe Close in kickUser } client.send <- "filler" @@ -495,14 +703,12 @@ func TestKickUserNonBlocking(t *testing.T) { hub.clients[client] = true hub.clientsMutex.Unlock() - // kickUser must not block even though the buffer is full. + // kickUser must not block even though the buffer is full, and must not + // panic when conn is nil. done := make(chan struct{}) go func() { - defer func() { - _ = recover() // conn is nil in test; tolerate nil pointer in conn.Close - close(done) - }() hub.kickUser("victim", "test") + close(done) }() select { diff --git a/server/integration_test.go b/server/integration_test.go index 6af8c0c..6bef536 100644 --- a/server/integration_test.go +++ b/server/integration_test.go @@ -91,7 +91,9 @@ func TestIntegrationUserBanFlow(t *testing.T) { } // Ban the user - hub.BanUser(username, adminUsername) + if err := hub.BanUser(username, adminUsername); err != nil { + t.Fatalf("BanUser returned unexpected error: %v", err) + } if !hub.IsUserBanned(username) { t.Error("User should be banned after BanUser") } @@ -111,8 +113,11 @@ func TestIntegrationUserBanFlow(t *testing.T) { t.Error("User should not be banned after UnbanUser") } - // Test kick flow - hub.KickUser(username, adminUsername) + // Test kick flow (requires connected user) + registerTestClient(hub, username) + if err := hub.KickUser(username, adminUsername); err != nil { + t.Fatalf("KickUser returned unexpected error: %v", err) + } if !hub.IsUserBanned(username) { t.Error("User should be kicked after KickUser") } @@ -392,8 +397,10 @@ func TestIntegrationConcurrentOperations(t *testing.T) { go func(id int) { defer banWg.Done() username := fmt.Sprintf("user%d", id) - hub.BanUser(username, "admin") + _ = hub.BanUser(username, "admin") hub.UnbanUser(username, "admin") + _ = hub.KickUser(username, "admin") + hub.AllowUser(username, "admin") }(i) } diff --git a/server/message_validate.go b/server/message_validate.go index 91301a9..d84dc7a 100644 --- a/server/message_validate.go +++ b/server/message_validate.go @@ -7,3 +7,12 @@ import "strings" func contentContainsNUL(s string) bool { return strings.Contains(s, "\x00") } + +// plaintextContentEmpty reports whether unencrypted text/dm/edit content is empty or +// whitespace-only. Encrypted payloads are opaque and are never treated as empty here. +func plaintextContentEmpty(content string, encrypted bool) bool { + if encrypted { + return false + } + return strings.TrimSpace(content) == "" +} diff --git a/server/message_validate_test.go b/server/message_validate_test.go new file mode 100644 index 0000000..cd75100 --- /dev/null +++ b/server/message_validate_test.go @@ -0,0 +1,26 @@ +package server + +import "testing" + +func TestPlaintextContentEmpty(t *testing.T) { + tests := []struct { + name string + content string + encrypted bool + want bool + }{ + {name: "empty", content: "", encrypted: false, want: true}, + {name: "whitespace", content: " \t\n ", encrypted: false, want: true}, + {name: "non-empty", content: "hello", encrypted: false, want: false}, + {name: "encrypted empty opaque", content: "", encrypted: true, want: false}, + {name: "encrypted whitespace opaque", content: " ", encrypted: true, want: false}, + {name: "encrypted non-empty blob", content: "Y2lwaGVydGV4dA==", encrypted: true, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := plaintextContentEmpty(tt.content, tt.encrypted); got != tt.want { + t.Fatalf("plaintextContentEmpty(%q, %v) = %v, want %v", tt.content, tt.encrypted, got, tt.want) + } + }) + } +}