From 17b17eb1b34ef516efe9f85989bfc69b00c4127f Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:48:47 +0200 Subject: [PATCH] fix(security): close all 13 findings from the 2026-07-28 server scan, plus dependabot rollup (#1264) * fix(admin): reject banned users in admin auth (F1) adminAuthMiddleware accepted a Bearer token on session validity plus the ADMINISTRATOR bit alone and never consulted ban state, so a ban never revoked admin-panel access. Adds the auth.IsEffectivelyBanned guard that api.AuthMiddleware already uses, at both admin credential-resolution points. Co-Authored-By: Claude Opus 5 (1M context) * fix(ws): gate the voice-channel text subscription on READ_MESSAGES (F2) registerNow subscribed any client with voice state to that channel's text-message topic regardless of READ_MESSAGES. The handshake's already-computed readable-channel set is now passed into registerNow and the subscription only happens when the voice channel is in it, preserving authorized reconnect delivery. Co-Authored-By: Claude Opus 5 (1M context) * fix(service): require READ_MESSAGES to delete messages (F4) The non-DM delete gate checked MANAGE_MESSAGES without READ_MESSAGES, so a role locked out of a private channel could still delete every message in it. Requires ReadMessages alongside ManageMessages (and alongside SendMessages on the author path) and derives the mod flag from that same gate. Co-Authored-By: Claude Opus 5 (1M context) * fix(service): require READ_MESSAGES alongside MANAGE_MESSAGES in SetMessagePinned (F8) Pin/unpin checked only MANAGE_MESSAGES, so a role denied READ on a private channel could still pin and unpin its messages. Co-Authored-By: Claude Opus 5 (1M context) * fix(service): enforce the DM block at every DM interaction sink (F5) The DM block was only checked on send, leaving edit, reactions, pins and typing as bypasses. One shared requireDMNotBlocked is now called from all of them. Co-Authored-By: Claude Opus 5 (1M context) * fix(ws): re-check CONNECT_VOICE when minting a refreshed LiveKit token (F6) voice_token_refresh re-minted a LiveKit token without re-checking CONNECT_VOICE, so a revoked permission kept working for the life of the session. The permission is now re-checked where the token is minted, and a 60s sweep evicts participants whose permission was revoked. Co-Authored-By: Claude Opus 5 (1M context) * fix(ws): rate-limit voice_e2ee_offer after validation, keyed on server state (F7) The limiter key was built from unvalidated client input, letting an attacker grow the limiter map without bound. The limiter now runs after validation and keys on (sender, voiceChannelID), never on client input. Co-Authored-By: Claude Opus 5 (1M context) * fix(ws): deliver voice_state/voice_leave only to roles that may read the channel (F9) Voice state of private channels was broadcast to every connected client, leaking channel membership. All 11 emit sites now route through one READ-filtered fan-out, channel-tagged so replay filters too. Co-Authored-By: Claude Opus 5 (1M context) * fix(api): redact the LiveKit access token from proxy dial-failure logs (F10) A dial failure wrote the LiveKit access-token JWT into the server log via the URL in the error. redactKey now runs on the error before it reaches slog. Co-Authored-By: Claude Opus 5 (1M context) * fix(auth): reserve the [deleted-N] username namespace (F11, F12) The tombstone username namespace used by account deletion was freely registrable, letting a user impersonate a deleted account. The namespace is now reserved at validation, and DeleteAccount retries with a random suffix on collision. Co-Authored-By: Claude Opus 5 (1M context) * fix(api): strip Unicode format characters from upload filenames (F13) The attachment filename sanitizer stripped control characters but not unicode.Cf, allowing bidi-override extension spoofing. Cf is now stripped alongside controls and foreign path separators are cut. Co-Authored-By: Claude Opus 5 (1M context) * fix(api): reserve the login attempt before the bcrypt compare (F3) The per-username lockout was a read-only IsLockedOut check followed by a failure recorded only after the ~250ms bcrypt compare, so N concurrent requests all passed the stale check before any of them recorded a failure. The per-username cap is the only cross-IP brute-force defence (the middleware limits per IP), so a distributed burst landed N guesses per 15-minute window instead of 10. Both counters are now reserved atomically with limiter.Allow before the compare, and the lockout decision moves to the read-only limiter.Check so the reservation is not double-counted. The limits are sized at threshold+1, which leaves the sequential accepted-input set byte-identical to the previous behaviour: failures 1-10 still land, the 10th still trips the lockout, and the account owner's correct password on attempt 10 still returns 200. Sizing at threshold instead would make 9 cheap wrong guesses convert the victim's own correct password into a 15-minute lockout - the regression that got two earlier attempts at this fix rejected, now pinned by a boundary test. Deliberately scoped to handleLogin. The report also suggested widening to the password-confirmation endpoints, but those are authenticated, share a single pw_confirm_fail key across the TOTP endpoints, and widening there is what got the first attempt rejected. Co-Authored-By: Claude Opus 5 (1M context) * chore(deps): bump five Rust dependencies in /Client/tauri-client/src-tauri Rolls up dependabot #1259, #1260, #1261, #1262 and #1263: tauri-build 2.5.6 -> 2.6.3 tauri-plugin-fs 2.4.5 -> 2.5.1 tauri-plugin-http 2.5.7 -> 2.5.9 tauri-plugin-store 2.4.2 -> 2.4.4 webpki-roots 1.0.6 -> 1.0.9 All five are lockfile-only; the manifest constraints already permitted the new versions. The five PRs each rewrote overlapping regions of the same Cargo.lock and so could not be merged independently, so the lockfile was regenerated with cargo update --precise for each crate instead. The combined result is smaller than the sum of the five diffs because they share transitive updates. Verified with cargo check --locked --all-targets (exit 0). Co-Authored-By: Claude Opus 5 (1M context) * chore(deps): bump typescript-eslint from 8.58.0 to 8.65.0 in /Client/tauri-client Dependabot #1258. 8.65.0 improves @typescript-eslint/no-unnecessary-type-assertion, which surfaces four assertions that were already redundant and now fail the lint gate. They are removed here rather than in a follow-up so no commit in this branch leaves `npm run lint` red: UserBar.ts / members.store.ts "online" as UserStatus -> "online" (the receiver already accepts the literal) media.ts drops `as RequestInit` on a literal that is already assignable LoginForm.ts drops `as { message: unknown }` made redundant by the `"message" in err` narrowing All four are the rule's own autofix. Verified: npm run typecheck, npm run lint, npm run format:check all clean, and the unit suite is 3572/3572 green across 129 files. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- Client/tauri-client/package-lock.json | 130 ++++++------ Client/tauri-client/package.json | 2 +- Client/tauri-client/src-tauri/Cargo.lock | 156 +++++++-------- Client/tauri-client/src/components/UserBar.ts | 2 +- .../src/components/message-list/media.ts | 2 +- .../src/pages/connect-page/LoginForm.ts | 2 +- .../tauri-client/src/stores/members.store.ts | 2 +- Server/admin/logstream.go | 5 + Server/admin/middleware.go | 8 + Server/admin/middleware_coverage_test.go | 25 +++ Server/api/auth_handler.go | 31 ++- Server/api/auth_handler_test.go | 90 +++++++++ Server/api/channel_handler_test.go | 10 + Server/api/livekit_proxy.go | 12 +- Server/api/livekit_proxy_test.go | 40 ++++ Server/api/upload_handler.go | 25 ++- Server/api/upload_handler_test.go | 71 +++++++ Server/auth/helpers.go | 11 + Server/auth/helpers_test.go | 33 +++ Server/db/account.go | 68 +++++-- Server/db/account_test.go | 40 ++++ Server/service/channel.go | 6 + Server/service/channel_test.go | 38 ++++ Server/service/message.go | 74 +++++-- Server/service/message_test.go | 189 ++++++++++++++++++ Server/ws/coverage_boost_test.go | 138 ++++++++++++- Server/ws/emit.go | 11 +- Server/ws/emit_test.go | 25 +-- Server/ws/event.go | 21 +- Server/ws/export_test.go | 11 +- Server/ws/handler_v2_voice_e2ee_offer_test.go | 50 +++++ Server/ws/handler_v2_voice_token_test.go | 122 +++++++++-- Server/ws/handlers.go | 8 +- Server/ws/hub.go | 131 +++++++++++- Server/ws/hub_test.go | 59 +++++- Server/ws/livekit_webhook.go | 6 +- Server/ws/serve.go | 21 +- Server/ws/voice_controls.go | 5 +- Server/ws/voice_e2ee.go | 42 ++-- Server/ws/voice_join.go | 20 +- Server/ws/voice_leave.go | 2 +- Server/ws/ws_integration_test.go | 138 +++++++++++++ 42 files changed, 1608 insertions(+), 274 deletions(-) diff --git a/Client/tauri-client/package-lock.json b/Client/tauri-client/package-lock.json index df750a5a..6e158748 100644 --- a/Client/tauri-client/package-lock.json +++ b/Client/tauri-client/package-lock.json @@ -36,7 +36,7 @@ "oxlint": "^1.76.0", "prettier": "^3.9.6", "typescript": "^5.7", - "typescript-eslint": "^8.58.0", + "typescript-eslint": "^8.65.0", "vite": "^6", "vitest": "^3" } @@ -4045,17 +4045,17 @@ "license": "MIT" }, "node_modules/@typescript-eslint/eslint-plugin": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.58.0.tgz", - "integrity": "sha512-RLkVSiNuUP1C2ROIWfqX+YcUfLaSnxGE/8M+Y57lopVwg9VTYYfhuz15Yf1IzCKgZj6/rIbYTmJCUSqr76r0Wg==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.65.0.tgz", + "integrity": "sha512-IEgob78X12rHpUmtcwFsXhZdVGJtwTVP8FiCLZkR6GlYVrl2PcuB+KhCE5BlVC/eQpQnu8WXRtkHZuPar+gCRA==", "dev": true, "license": "MIT", "dependencies": { "@eslint-community/regexpp": "^4.12.2", - "@typescript-eslint/scope-manager": "8.58.0", - "@typescript-eslint/type-utils": "8.58.0", - "@typescript-eslint/utils": "8.58.0", - "@typescript-eslint/visitor-keys": "8.58.0", + "@typescript-eslint/scope-manager": "8.65.0", + "@typescript-eslint/type-utils": "8.65.0", + "@typescript-eslint/utils": "8.65.0", + "@typescript-eslint/visitor-keys": "8.65.0", "ignore": "^7.0.5", "natural-compare": "^1.4.0", "ts-api-utils": "^2.5.0" @@ -4068,15 +4068,15 @@ "url": "https://opencollective.com/typescript-eslint" }, "peerDependencies": { - "@typescript-eslint/parser": "^8.58.0", + "@typescript-eslint/parser": "^8.65.0", "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/eslint-plugin/node_modules/ignore": { - "version": "7.0.5", - "resolved": "https://registry.npmjs.org/ignore/-/ignore-7.0.5.tgz", - "integrity": "sha512-Hs59xBNfUIunMFgWAbGX5cq6893IbWg4KnrjbYwX3tx0ztorVgTDA6B2sxf8ejHJ4wz8BqGUMYlnzNBer5NvGg==", + "version": "7.0.6", + "resolved": "https://registry.npmjs.org/ignore/-/ignore-7.0.6.tgz", + "integrity": "sha512-BAg6QkE8W+TuQLrrw0Ugr7HegXduRuuj8/ti2kSOc+jz1dmx8/WNcjr6XGnq5YpDWxFwwaavqD0+jIUOKelTsw==", "dev": true, "license": "MIT", "engines": { @@ -4084,16 +4084,16 @@ } }, "node_modules/@typescript-eslint/parser": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-8.58.0.tgz", - "integrity": "sha512-rLoGZIf9afaRBYsPUMtvkDWykwXwUPL60HebR4JgTI8mxfFe2cQTu3AGitANp4b9B2QlVru6WzjgB2IzJKiCSA==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-8.65.0.tgz", + "integrity": "sha512-CZ4nMxWwgu1HEEFNkeaCptra9QCtkmKdgf3sWh1rl1trIhmxLilgTV4cwcbQ4wemnT4sWQN8CaKOmdYx+g2gMA==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/scope-manager": "8.58.0", - "@typescript-eslint/types": "8.58.0", - "@typescript-eslint/typescript-estree": "8.58.0", - "@typescript-eslint/visitor-keys": "8.58.0", + "@typescript-eslint/scope-manager": "8.65.0", + "@typescript-eslint/types": "8.65.0", + "@typescript-eslint/typescript-estree": "8.65.0", + "@typescript-eslint/visitor-keys": "8.65.0", "debug": "^4.4.3" }, "engines": { @@ -4109,14 +4109,14 @@ } }, "node_modules/@typescript-eslint/project-service": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/project-service/-/project-service-8.58.0.tgz", - "integrity": "sha512-8Q/wBPWLQP1j16NxoPNIKpDZFMaxl7yWIoqXWYeWO+Bbd2mjgvoF0dxP2jKZg5+x49rgKdf7Ck473M8PC3V9lg==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/project-service/-/project-service-8.65.0.tgz", + "integrity": "sha512-SxnPhbTsGahizDgbu7oqFH/xVtzIqMd/s+WtnSxNxJZJpLbdT5IPdzg8EZxO3+PoKahXmwJLeNQOpKJb3/bi7Q==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/tsconfig-utils": "^8.58.0", - "@typescript-eslint/types": "^8.58.0", + "@typescript-eslint/tsconfig-utils": "^8.65.0", + "@typescript-eslint/types": "^8.65.0", "debug": "^4.4.3" }, "engines": { @@ -4131,14 +4131,14 @@ } }, "node_modules/@typescript-eslint/scope-manager": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/scope-manager/-/scope-manager-8.58.0.tgz", - "integrity": "sha512-W1Lur1oF50FxSnNdGp3Vs6P+yBRSmZiw4IIjEeYxd8UQJwhUF0gDgDD/W/Tgmh73mxgEU3qX0Bzdl/NGuSPEpQ==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/scope-manager/-/scope-manager-8.65.0.tgz", + "integrity": "sha512-Esbl8OSYiVxBokYgWPf7VVWg/BE798wXhimnn9ML9Pt5qoDf8bfQlgjlKXR/k98+AcNzlLKYrpCcrcuZ9DZLgg==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/types": "8.58.0", - "@typescript-eslint/visitor-keys": "8.58.0" + "@typescript-eslint/types": "8.65.0", + "@typescript-eslint/visitor-keys": "8.65.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -4149,9 +4149,9 @@ } }, "node_modules/@typescript-eslint/tsconfig-utils": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/tsconfig-utils/-/tsconfig-utils-8.58.0.tgz", - "integrity": "sha512-doNSZEVJsWEu4htiVC+PR6NpM+pa+a4ClH9INRWOWCUzMst/VA9c4gXq92F8GUD1rwhNvRLkgjfYtFXegXQF7A==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/tsconfig-utils/-/tsconfig-utils-8.65.0.tgz", + "integrity": "sha512-j6GzGqCiRdA7Qhur2VVmKZAkBLfnHFQfx4TaJGL9RMveZqCo48jSHHO0DTgizEnGhtWnqmbtCUSrqSkdiY/0Hg==", "dev": true, "license": "MIT", "engines": { @@ -4166,15 +4166,15 @@ } }, "node_modules/@typescript-eslint/type-utils": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/type-utils/-/type-utils-8.58.0.tgz", - "integrity": "sha512-aGsCQImkDIqMyx1u4PrVlbi/krmDsQUs4zAcCV6M7yPcPev+RqVlndsJy9kJ8TLihW9TZ0kbDAzctpLn5o+lOg==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/type-utils/-/type-utils-8.65.0.tgz", + "integrity": "sha512-YjaZ7PRI5qY7ax2L3PbvX0rRyGtipAReCWs0mhhDBHjH/vl0g0BonaGXrKdKpMbIIsMIwDgbk/xzkBTyAltS5g==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/types": "8.58.0", - "@typescript-eslint/typescript-estree": "8.58.0", - "@typescript-eslint/utils": "8.58.0", + "@typescript-eslint/types": "8.65.0", + "@typescript-eslint/typescript-estree": "8.65.0", + "@typescript-eslint/utils": "8.65.0", "debug": "^4.4.3", "ts-api-utils": "^2.5.0" }, @@ -4191,9 +4191,9 @@ } }, "node_modules/@typescript-eslint/types": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/types/-/types-8.58.0.tgz", - "integrity": "sha512-O9CjxypDT89fbHxRfETNoAnHj/i6IpRK0CvbVN3qibxlLdo5p5hcLmUuCCrHMpxiWSwKyI8mCP7qRNYuOJ0Uww==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/types/-/types-8.65.0.tgz", + "integrity": "sha512-JSSwWNy+H0E/01jJEM+hrX6N0OFDzFzeIhHFSAS01tlVaevpG8cFyYRPhS5yjGOvBUx3sqQHVMjCL1CAZZMxBg==", "dev": true, "license": "MIT", "engines": { @@ -4205,16 +4205,16 @@ } }, "node_modules/@typescript-eslint/typescript-estree": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/typescript-estree/-/typescript-estree-8.58.0.tgz", - "integrity": "sha512-7vv5UWbHqew/dvs+D3e1RvLv1v2eeZ9txRHPnEEBUgSNLx5ghdzjHa0sgLWYVKssH+lYmV0JaWdoubo0ncGYLA==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/typescript-estree/-/typescript-estree-8.65.0.tgz", + "integrity": "sha512-JboAE2swaYt4tb1fHhHTABE2K+OLy09XfcTbhnk4Pw96f9dd2e9iYsJ28gBggHlo5z5x1rkyWvcPoTuNTd4oGg==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/project-service": "8.58.0", - "@typescript-eslint/tsconfig-utils": "8.58.0", - "@typescript-eslint/types": "8.58.0", - "@typescript-eslint/visitor-keys": "8.58.0", + "@typescript-eslint/project-service": "8.65.0", + "@typescript-eslint/tsconfig-utils": "8.65.0", + "@typescript-eslint/types": "8.65.0", + "@typescript-eslint/visitor-keys": "8.65.0", "debug": "^4.4.3", "minimatch": "^10.2.2", "semver": "^7.7.3", @@ -4233,16 +4233,16 @@ } }, "node_modules/@typescript-eslint/utils": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/utils/-/utils-8.58.0.tgz", - "integrity": "sha512-RfeSqcFeHMHlAWzt4TBjWOAtoW9lnsAGiP3GbaX9uVgTYYrMbVnGONEfUCiSss+xMHFl+eHZiipmA8WkQ7FuNA==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/utils/-/utils-8.65.0.tgz", + "integrity": "sha512-gXiwIHsYreboxeJucHKPvgwl7dXt50mF8s1/c00cP/WoVTyWKFdtfhRWwZiXYFU5H2O8vVoSLNrexFZjYS/SGA==", "dev": true, "license": "MIT", "dependencies": { "@eslint-community/eslint-utils": "^4.9.1", - "@typescript-eslint/scope-manager": "8.58.0", - "@typescript-eslint/types": "8.58.0", - "@typescript-eslint/typescript-estree": "8.58.0" + "@typescript-eslint/scope-manager": "8.65.0", + "@typescript-eslint/types": "8.65.0", + "@typescript-eslint/typescript-estree": "8.65.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -4257,13 +4257,13 @@ } }, "node_modules/@typescript-eslint/visitor-keys": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/visitor-keys/-/visitor-keys-8.58.0.tgz", - "integrity": "sha512-XJ9UD9+bbDo4a4epraTwG3TsNPeiB9aShrUneAVXy8q4LuwowN+qu89/6ByLMINqvIMeI9H9hOHQtg/ijrYXzQ==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/visitor-keys/-/visitor-keys-8.65.0.tgz", + "integrity": "sha512-8C71BQkGjiMmXtop7pHVJu1l2NNShFdkCyD6a2ezzs5vU/L3LRtb69EtcteFwz0mYMPzIgOw0n6OV4VBUWZd7A==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/types": "8.58.0", + "@typescript-eslint/types": "8.65.0", "eslint-visitor-keys": "^5.0.0" }, "engines": { @@ -7810,16 +7810,16 @@ } }, "node_modules/typescript-eslint": { - "version": "8.58.0", - "resolved": "https://registry.npmjs.org/typescript-eslint/-/typescript-eslint-8.58.0.tgz", - "integrity": "sha512-e2TQzKfaI85fO+F3QywtX+tCTsu/D3WW5LVU6nz8hTFKFZ8yBJ6mSYRpXqdR3mFjPWmO0eWsTa5f+UpAOe/FMA==", + "version": "8.65.0", + "resolved": "https://registry.npmjs.org/typescript-eslint/-/typescript-eslint-8.65.0.tgz", + "integrity": "sha512-/ggrHAwyjENDusvyxbuqxAC2dTnZg/Z8F+fgQtYIz+L6n/9HfSlEZcFGV/NsMNa6CkGk0xUjUAFwC0vHOflvIA==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/eslint-plugin": "8.58.0", - "@typescript-eslint/parser": "8.58.0", - "@typescript-eslint/typescript-estree": "8.58.0", - "@typescript-eslint/utils": "8.58.0" + "@typescript-eslint/eslint-plugin": "8.65.0", + "@typescript-eslint/parser": "8.65.0", + "@typescript-eslint/typescript-estree": "8.65.0", + "@typescript-eslint/utils": "8.65.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" diff --git a/Client/tauri-client/package.json b/Client/tauri-client/package.json index 48dabf84..e64c63b7 100644 --- a/Client/tauri-client/package.json +++ b/Client/tauri-client/package.json @@ -45,7 +45,7 @@ "oxlint": "^1.76.0", "prettier": "^3.9.6", "typescript": "^5.7", - "typescript-eslint": "^8.58.0", + "typescript-eslint": "^8.65.0", "vite": "^6", "vitest": "^3" }, diff --git a/Client/tauri-client/src-tauri/Cargo.lock b/Client/tauri-client/src-tauri/Cargo.lock index e93371e8..24d0e694 100644 --- a/Client/tauri-client/src-tauri/Cargo.lock +++ b/Client/tauri-client/src-tauri/Cargo.lock @@ -708,24 +708,6 @@ dependencies = [ "version_check", ] -[[package]] -name = "cookie_store" -version = "0.21.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2eac901828f88a5241ee0600950ab981148a18f2f756900ffba1b125ca6a3ef9" -dependencies = [ - "cookie", - "document-features", - "idna", - "log", - "publicsuffix", - "serde", - "serde_derive", - "serde_json", - "time", - "url", -] - [[package]] name = "cookie_store" version = "0.22.1" @@ -913,14 +895,20 @@ dependencies = [ [[package]] name = "ctor" -version = "0.2.9" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "32a2785755761f3ddc1492979ce1e48d2c00d09311c39e4466429188f3dd6501" +checksum = "352d39c2f7bef1d6ad73db6f5160efcaed66d94ef8c6c573a8410c00bf909a98" dependencies = [ - "quote", - "syn 2.0.117", + "ctor-proc-macro", + "dtor", ] +[[package]] +name = "ctor-proc-macro" +version = "0.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52560adf09603e58c9a7ee1fe1dcb95a16927b17c127f0ac02d6e768a0e25bc1" + [[package]] name = "darling" version = "0.23.0" @@ -1197,6 +1185,21 @@ dependencies = [ "dtoa", ] +[[package]] +name = "dtor" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f1057d6c64987086ff8ed0fd3fbf377a6b7d205cc7715868cd401705f715cbe4" +dependencies = [ + "dtor-proc-macro", +] + +[[package]] +name = "dtor-proc-macro" +version = "0.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f678cf4a922c215c63e0de95eb1ff08a958a81d47e485cf9da1e27bf6305cfa5" + [[package]] name = "dunce" version = "1.0.5" @@ -2056,7 +2059,7 @@ dependencies = [ "tokio", "tokio-rustls", "tower-service", - "webpki-roots 1.0.6", + "webpki-roots 1.0.9", ] [[package]] @@ -2081,7 +2084,7 @@ dependencies = [ "tokio", "tower-service", "tracing", - "windows-registry 0.6.1", + "windows-registry", ] [[package]] @@ -3039,7 +3042,7 @@ dependencies = [ "tokio-rustls", "tokio-tungstenite", "url", - "webpki-roots 1.0.6", + "webpki-roots 1.0.9", "windows 0.58.0", ] @@ -3187,7 +3190,6 @@ version = "0.11.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1fd6780a80ae0c52cc120a26a1a42c1ae51b247a253e4e06113d23d2c2edd078" dependencies = [ - "phf_macros 0.11.3", "phf_shared 0.11.3", ] @@ -3286,19 +3288,6 @@ dependencies = [ "syn 1.0.109", ] -[[package]] -name = "phf_macros" -version = "0.11.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f84ac04429c13a7ff43785d75ad27569f2951ce0ffd30a3321230db2fc727216" -dependencies = [ - "phf_generator 0.11.3", - "phf_shared 0.11.3", - "proc-macro2", - "quote", - "syn 2.0.117", -] - [[package]] name = "phf_macros" version = "0.13.1" @@ -3900,7 +3889,7 @@ dependencies = [ "base64 0.22.1", "bytes", "cookie", - "cookie_store 0.22.1", + "cookie_store", "encoding_rs", "futures-core", "h2", @@ -3931,7 +3920,7 @@ dependencies = [ "wasm-bindgen", "wasm-bindgen-futures", "web-sys", - "webpki-roots 1.0.6", + "webpki-roots 1.0.9", ] [[package]] @@ -4885,9 +4874,9 @@ dependencies = [ [[package]] name = "tauri-build" -version = "2.5.6" +version = "2.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4bbc990d1dbf57a8e1c7fa2327f2a614d8b757805603c1b9ba5c81bade09fd4d" +checksum = "bc9ce40b16101cb6ea63d3e221567affd1c3a9205f95d7bc574941a10636b632" dependencies = [ "anyhow", "cargo_toml", @@ -4901,7 +4890,6 @@ dependencies = [ "serde_json", "tauri-utils", "tauri-winres", - "toml 0.9.12+spec-1.1.0", "walkdir", ] @@ -4994,7 +4982,7 @@ dependencies = [ "thiserror 2.0.18", "tracing", "url", - "windows-registry 0.5.3", + "windows-registry", "windows-result 0.3.4", ] @@ -5018,13 +5006,15 @@ dependencies = [ [[package]] name = "tauri-plugin-fs" -version = "2.4.5" +version = "2.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed390cc669f937afeb8b28032ce837bac8ea023d975a2e207375ec05afaf1804" +checksum = "b7ecc274121aca0c036a2b42d1cbe83d368d348f54e0bb8a735c2b1548e8f371" dependencies = [ "anyhow", "dunce", "glob", + "log", + "objc2-foundation", "percent-encoding", "schemars 0.8.22", "serde", @@ -5034,18 +5024,18 @@ dependencies = [ "tauri-plugin", "tauri-utils", "thiserror 2.0.18", - "toml 0.9.12+spec-1.1.0", + "toml 1.0.7+spec-1.1.0", "url", ] [[package]] name = "tauri-plugin-http" -version = "2.5.7" +version = "2.5.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d8f069451c4e87e7e2636b7f065a4c52866c4ce5e60e2d53fa1038edb6d184dc" +checksum = "b5bd512048e1985b7ec78f96d99083e2ddaf7e0d906b2b63c44ce5bb8b894067" dependencies = [ "bytes", - "cookie_store 0.21.1", + "cookie_store", "data-url", "http", "regex", @@ -5153,9 +5143,9 @@ dependencies = [ [[package]] name = "tauri-plugin-store" -version = "2.4.2" +version = "2.4.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5ca1a8ff83c269b115e98726ffc13f9e548a10161544a92ad121d6d0a96e16ea" +checksum = "6708afbe549f176b712066e71648ba8fafba20789453718260c7ca356733cb0c" dependencies = [ "dunce", "serde", @@ -5289,14 +5279,15 @@ dependencies = [ [[package]] name = "tauri-utils" -version = "2.8.3" +version = "2.9.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "219a1f983a2af3653f75b5747f76733b0da7ff03069c7a41901a5eb3ace4557d" +checksum = "3e176a18e67764923c4f1ce66f25ae4abe5f688384d5eb1a0fa6c77f3d90f887" dependencies = [ "anyhow", "brotli", "cargo_metadata", "ctor", + "dom_query", "dunce", "glob", "html5ever 0.29.1", @@ -5306,7 +5297,8 @@ dependencies = [ "kuchikiki", "log", "memchr", - "phf 0.11.3", + "phf 0.13.1", + "plist", "proc-macro2", "quote", "regex", @@ -5318,7 +5310,7 @@ dependencies = [ "serde_with", "swift-rs", "thiserror 2.0.18", - "toml 0.9.12+spec-1.1.0", + "toml 1.0.7+spec-1.1.0", "url", "urlpattern", "uuid", @@ -5603,6 +5595,21 @@ dependencies = [ "winnow 0.7.15", ] +[[package]] +name = "toml" +version = "1.0.7+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dd28d57d8a6f6e458bc0b8784f8fdcc4b99a437936056fa122cb234f18656a96" +dependencies = [ + "indexmap 2.13.0", + "serde_core", + "serde_spanned 1.0.4", + "toml_datetime 1.0.1+spec-1.1.0", + "toml_parser", + "toml_writer", + "winnow 1.0.0", +] + [[package]] name = "toml_datetime" version = "0.6.3" @@ -6243,14 +6250,14 @@ version = "0.26.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "521bc38abb08001b01866da9f51eb7c5d647a19260e00054a8c7fd5f9e57f7a9" dependencies = [ - "webpki-roots 1.0.6", + "webpki-roots 1.0.9", ] [[package]] name = "webpki-roots" -version = "1.0.6" +version = "1.0.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22cfaf3c063993ff62e73cb4311efde4db1efb31ab78a3e5c457939ad5cc0bed" +checksum = "7dcd9d09a39985f5344844e66b0c530a33843579125f23e21e9f0f220850f22a" dependencies = [ "rustls-pki-types", ] @@ -6492,17 +6499,6 @@ dependencies = [ "windows-strings 0.4.2", ] -[[package]] -name = "windows-registry" -version = "0.6.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "02752bf7fbdcce7f2a27a742f798510f3e5ad88dbe84871e5168e2120c3d5720" -dependencies = [ - "windows-link 0.2.1", - "windows-result 0.4.1", - "windows-strings 0.5.1", -] - [[package]] name = "windows-result" version = "0.2.0" @@ -6521,15 +6517,6 @@ dependencies = [ "windows-link 0.1.3", ] -[[package]] -name = "windows-result" -version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5" -dependencies = [ - "windows-link 0.2.1", -] - [[package]] name = "windows-strings" version = "0.1.0" @@ -6549,15 +6536,6 @@ dependencies = [ "windows-link 0.1.3", ] -[[package]] -name = "windows-strings" -version = "0.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7837d08f69c77cf6b07689544538e017c1bfcf57e34b4c0ff58e6c2cd3b37091" -dependencies = [ - "windows-link 0.2.1", -] - [[package]] name = "windows-sys" version = "0.45.0" diff --git a/Client/tauri-client/src/components/UserBar.ts b/Client/tauri-client/src/components/UserBar.ts index 47341f33..34c32c17 100644 --- a/Client/tauri-client/src/components/UserBar.ts +++ b/Client/tauri-client/src/components/UserBar.ts @@ -82,7 +82,7 @@ export function createUserBar(options?: UserBarOptions): MountableComponent { }; statusPicker = createStatusPicker({ - currentStatus: "online" as UserStatus, + currentStatus: "online", onStatusChange: (status: UserStatus) => { const ws = options?.ws; if (ws !== null && ws !== undefined && canSetStatus()) { diff --git a/Client/tauri-client/src/components/message-list/media.ts b/Client/tauri-client/src/components/message-list/media.ts index 5b6f774a..351ae20b 100644 --- a/Client/tauri-client/src/components/message-list/media.ts +++ b/Client/tauri-client/src/components/message-list/media.ts @@ -143,7 +143,7 @@ export function renderYouTubeEmbed(videoId: string, originalUrl: string): HTMLDi const oembedUrl = `https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=${encodeURIComponent(videoId)}&format=json`; tauriFetch(oembedUrl, { signal: AbortSignal.timeout(5000), - } as RequestInit) + }) .then((res) => (res.ok ? (res.json() as Promise<{ title?: string } | null>) : null)) .then((data) => { if (generation !== mediaCacheGeneration) { diff --git a/Client/tauri-client/src/pages/connect-page/LoginForm.ts b/Client/tauri-client/src/pages/connect-page/LoginForm.ts index ae599a16..293ccecd 100644 --- a/Client/tauri-client/src/pages/connect-page/LoginForm.ts +++ b/Client/tauri-client/src/pages/connect-page/LoginForm.ts @@ -596,7 +596,7 @@ export function createLoginForm(opts: LoginFormOptions): LoginFormApi { } else if (typeof err === "string") { message = err; } else if (err !== null && typeof err === "object" && "message" in err) { - message = String((err as { message: unknown }).message); + message = String(err.message); } else { message = String(err); } diff --git a/Client/tauri-client/src/stores/members.store.ts b/Client/tauri-client/src/stores/members.store.ts index 9eaf5606..695f4785 100644 --- a/Client/tauri-client/src/stores/members.store.ts +++ b/Client/tauri-client/src/stores/members.store.ts @@ -72,7 +72,7 @@ export function addMember(payload: MemberJoinPayload): void { username: payload.user.username, avatar: payload.user.avatar, role: payload.user.role, - status: "online" as UserStatus, + status: "online", identityPublicKey: payload.user.identity_public_key ?? null, }); return { ...prev, members: next }; diff --git a/Server/admin/logstream.go b/Server/admin/logstream.go index 097fccbe..6d6588c6 100644 --- a/Server/admin/logstream.go +++ b/Server/admin/logstream.go @@ -362,6 +362,11 @@ func handleLogStream(database *db.DB, ringBuf *RingBuffer) http.HandlerFunc { if userErr != nil || user == nil { return false } + // A ban mid-stream must cut the stream, same as adminAuthMiddleware + // rejects a banned user on the request path. + if auth.IsEffectivelyBanned(user) { + return false + } role, roleErr := database.GetRoleByID(ctx, user.RoleID) if roleErr != nil || role == nil { return false diff --git a/Server/admin/middleware.go b/Server/admin/middleware.go index 11a9b105..e8442d85 100644 --- a/Server/admin/middleware.go +++ b/Server/admin/middleware.go @@ -48,6 +48,14 @@ func adminAuthMiddleware(database *db.DB) func(http.Handler) http.Handler { return } + // Reject effectively-banned users before any further processing, as + // api.AuthMiddleware does: a ban must revoke admin-panel access + // immediately, not only once the session expires. + if auth.IsEffectivelyBanned(user) { + writeErr(w, http.StatusForbidden, "FORBIDDEN", "your account has been suspended") + return + } + role, err := database.GetRoleByID(r.Context(), user.RoleID) if err != nil || role == nil { writeErr(w, http.StatusUnauthorized, "UNAUTHORIZED", "role not found") diff --git a/Server/admin/middleware_coverage_test.go b/Server/admin/middleware_coverage_test.go index c76cbe8d..ab9fc599 100644 --- a/Server/admin/middleware_coverage_test.go +++ b/Server/admin/middleware_coverage_test.go @@ -49,6 +49,31 @@ func TestAdminAuthMiddleware_ExpiredSession(t *testing.T) { } } +// TestAdminAuthMiddleware_BannedAdmin verifies that a banned administrator's +// still-valid session is rejected with 403 — a ban must revoke admin-panel +// access immediately, not only when the session expires. +func TestAdminAuthMiddleware_BannedAdmin(t *testing.T) { + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database)) + token := createAdminUser(t, database) + + if w := doRequest(t, handler, http.MethodGet, "/stats", token, nil); w.Code != http.StatusOK { + t.Fatalf("pre-ban status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + + if _, err := database.ExecContext(context.Background(), + `UPDATE users SET banned = 1 WHERE username = 'adminuser'`, + ); err != nil { + t.Fatalf("UPDATE users banned: %v", err) + } + + w := doRequest(t, handler, http.MethodGet, "/stats", token, nil) + + if w.Code != http.StatusForbidden { + t.Errorf("banned admin status = %d, want 403; body: %s", w.Code, w.Body.String()) + } +} + // TestAdminAuthMiddleware_MissingBearer verifies that a request with no // Authorization header returns 401. func TestAdminAuthMiddleware_MissingBearer(t *testing.T) { diff --git a/Server/api/auth_handler.go b/Server/api/auth_handler.go index 1392e01d..666b9ccb 100644 --- a/Server/api/auth_handler.go +++ b/Server/api/auth_handler.go @@ -323,6 +323,26 @@ func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth. failKey := "login_fail:" + ip userFailKey := "login_user_fail:" + unameKey + // F3: atomically reserve this attempt BEFORE the bcrypt compare. The + // read-only IsLockedOut gates above are check-then-act: N concurrent + // requests all pass them before any failure is recorded below, so the + // per-username cap — the only cross-IP brute-force defence — bound + // only sequential attackers. Allow records the attempt under the + // limiter's lock, capping a concurrent burst at the same budget a + // sequential attacker gets. Sized at threshold+1 so the sequential + // accepted-input set is unchanged: failures 1–10 still land, the 10th + // still trips the lockout (via the Check below), and a correct + // password on attempt 10 still succeeds — successful logins reset + // both counters. The reservation sits after the DB-error return above + // so a transient DB outage still does not consume attempts. + if !limiter.Allow(failKey, loginFailureThreshold+1, loginFailureWindow) || + !limiter.Allow(userFailKey, loginUserFailureThreshold+1, loginUserFailureWindow) { + writeJSON(w, http.StatusTooManyRequests, errorResponse{ + Error: "RATE_LIMITED", + Message: "account temporarily locked due to too many failed attempts", + }) + return + } // Always run the password check — with an empty hash when the user does // not exist. auth.CheckPassword performs a dummy bcrypt comparison for an // empty hash, so bcrypt executes on every path and response time stays @@ -334,12 +354,15 @@ func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth. storedHash = user.PasswordHash } if !auth.CheckPassword(storedHash, req.Password) { - // Track failures per-IP; lockout on threshold. - if !limiter.Allow(failKey, loginFailureThreshold, loginFailureWindow) { + // The attempt was already recorded atomically up-front (F3); here + // only decide the lockouts, at the same boundary as before: the + // 10th in-window failure locks the key. Check is read-only, so + // the reservation is not double-counted. + if !limiter.Check(failKey, loginFailureThreshold+1, loginFailureWindow) { limiter.Lockout(r.Context(), lockKey, loginLockoutDuration) } - // BUG-110: Track failures per-username; lockout on threshold. - if !limiter.Allow(userFailKey, loginUserFailureThreshold, loginUserFailureWindow) { + // BUG-110: per-username lockout on threshold. + if !limiter.Check(userFailKey, loginUserFailureThreshold+1, loginUserFailureWindow) { limiter.Lockout(r.Context(), userLockKey, loginUserLockoutDuration) } slog.Info("login failed", "ip", ip, "username_len", len(req.Username)) diff --git a/Server/api/auth_handler_test.go b/Server/api/auth_handler_test.go index 9720e74f..74245ec2 100644 --- a/Server/api/auth_handler_test.go +++ b/Server/api/auth_handler_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "sync" "testing" "testing/fstest" "time" @@ -444,6 +445,95 @@ func TestLogin_UsernameLockoutIgnoresCasing(t *testing.T) { } } +// TestLogin_ConcurrentBurstCannotExceedUsernameBudget locks F3: the +// per-username failure cap (the only cross-IP brute-force defence) must bind +// concurrent attackers, not just sequential ones. Before the fix the lockout +// was a read-only check followed by a post-bcrypt record, so N concurrent +// requests all passed the stale check and landed N guesses; a distributed +// burst must now land at most the same 10-attempt budget a sequential +// attacker gets. +func TestLogin_ConcurrentBurstCannotExceedUsernameBudget(t *testing.T) { + database := newAuthTestDB(t) + limiter := auth.NewRateLimiter() + router := buildAuthRouter(database, limiter) + + hash, _ := auth.HashPassword("correctPass1") + _, _ = database.CreateUser(context.Background(), "bursttarget", hash, 4) + + const burst = 40 + start := make(chan struct{}) + codes := make([]int, burst) + var wg sync.WaitGroup + for i := range burst { + wg.Go(func() { + raw, _ := json.Marshal(map[string]string{ + "username": "bursttarget", + "password": "wrongpassword", + }) + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/login", bytes.NewReader(raw)) + req.Header.Set("Content-Type", "application/json") + // Unique IP per request so only the per-username limiter binds. + req.RemoteAddr = fmt.Sprintf("203.0.113.%d:9999", i+1) + rr := httptest.NewRecorder() + <-start + router.ServeHTTP(rr, req) + codes[i] = rr.Code + }) + } + close(start) + wg.Wait() + + landed, limited := 0, 0 + for i, code := range codes { + switch code { + case http.StatusUnauthorized: + landed++ + case http.StatusTooManyRequests: + limited++ + default: + t.Fatalf("request %d: unexpected status %d", i, code) + } + } + // Sequential budget is 10 landed guesses (9 recorded + the one that trips + // the lockout); a concurrent burst must not exceed it. + if landed > 10 { + t.Fatalf("concurrent burst landed %d password guesses (%d rate-limited), want at most 10", landed, limited) + } +} + +// TestLogin_NineFailuresThenCorrectPasswordSucceeds pins the sequential +// accepted-input boundary that the F3 fix must not move: after 9 wrong +// guesses the account owner's correct password still logs in (attempt 10 is +// inside the budget). A fix that reserves attempts pre-compare at the +// original threshold would return 429 here and hand attackers a 9-request +// victim lockout. +func TestLogin_NineFailuresThenCorrectPasswordSucceeds(t *testing.T) { + database := newAuthTestDB(t) + limiter := auth.NewRateLimiter() + router := buildAuthRouter(database, limiter) + + hash, _ := auth.HashPassword("correctPass1") + _, _ = database.CreateUser(context.Background(), "boundaryuser", hash, 4) + + for i := 0; i < 9; i++ { + rr := postJSONFromIP(t, router, "/api/v1/auth/login", map[string]string{ + "username": "boundaryuser", + "password": "wrongpassword", + }, fmt.Sprintf("198.51.100.%d", i+1)) + if rr.Code != http.StatusUnauthorized { + t.Fatalf("setup attempt %d status = %d, want 401; body = %s", i+1, rr.Code, rr.Body.String()) + } + } + + rr := postJSONFromIP(t, router, "/api/v1/auth/login", map[string]string{ + "username": "boundaryuser", + "password": "correctPass1", + }, "198.51.100.250") + if rr.Code != http.StatusOK { + t.Fatalf("correct password on attempt 10 status = %d, want 200; body = %s", rr.Code, rr.Body.String()) + } +} + func TestLogin_SuccessResetsUsernameFailureCounter(t *testing.T) { database := newAuthTestDB(t) limiter := auth.NewRateLimiter() diff --git a/Server/api/channel_handler_test.go b/Server/api/channel_handler_test.go index e9334f5d..3d8ea400 100644 --- a/Server/api/channel_handler_test.go +++ b/Server/api/channel_handler_test.go @@ -690,6 +690,16 @@ func newPinTestDB(t *testing.T) *db.DB { opened_at TEXT NOT NULL DEFAULT (datetime('now')), PRIMARY KEY (user_id, channel_id) ); + -- Mirrors migration 012. DM pin authorization consults the block list + -- (a blocked user must not mutate the blocker's pins), so this fixture + -- needs the table or the lookup fails closed with a 500. + CREATE TABLE IF NOT EXISTS user_blocks ( + blocker_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + blocked_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + PRIMARY KEY (blocker_id, blocked_id), + CHECK (blocker_id != blocked_id) + ); `) if err != nil { t.Fatalf("create dm_participants: %v", err) diff --git a/Server/api/livekit_proxy.go b/Server/api/livekit_proxy.go index 85908bbf..4bc11614 100644 --- a/Server/api/livekit_proxy.go +++ b/Server/api/livekit_proxy.go @@ -141,7 +141,17 @@ func proxyWebSocket(w http.ResponseWriter, r *http.Request, target *url.URL, all defer dialResp.Body.Close() //nolint:errcheck // best-effort close } if err != nil { - slog.Warn("livekit proxy: backend dial failed", "host", backendURL.Host, "path", backendURL.Path, "err", err) + // websocket.Dial wraps a *url.Error, which embeds the full request URL — + // including the access_token query parameter, a live LiveKit room-join + // JWT. Anyone with log access (stdout, a shipper, or the admin panel's + // live view, whose ring buffer captures DEBUG+ regardless of the + // configured stdout level) could replay it inside its 5-minute TTL as + // the victim's participant identity. Strip the credential before the + // error reaches slog: the raw query blob first, so an encoded form is + // caught too, then the decoded token. + safeErr := redactKey(err.Error(), backendURL.RawQuery) + safeErr = redactKey(safeErr, backendURL.Query().Get("access_token")) + slog.Warn("livekit proxy: backend dial failed", "host", backendURL.Host, "path", backendURL.Path, "err", safeErr) writeJSON(w, http.StatusBadGateway, errorResponse{ Error: "BAD_GATEWAY", Message: "backend unavailable", diff --git a/Server/api/livekit_proxy_test.go b/Server/api/livekit_proxy_test.go index 3c005f15..19ed2a6e 100644 --- a/Server/api/livekit_proxy_test.go +++ b/Server/api/livekit_proxy_test.go @@ -1,8 +1,11 @@ package api import ( + "bytes" + "log/slog" "net/http" "net/http/httptest" + "strings" "testing" ) @@ -224,6 +227,43 @@ func TestLiveKitProxy_DoesNotBlockUserMetrics(t *testing.T) { } } +// TestProxyWebSocket_DialFailureDoesNotLogAccessToken locks the credential out +// of the log stream: websocket.Dial wraps a *url.Error carrying the full +// backend URL, so a dial failure (LiveKit down, restarting, refused) used to +// print the caller's live room-join JWT at Warn level — replayable inside its +// 5-minute TTL by anyone reading stdout or the admin panel's log ring buffer. +func TestProxyWebSocket_DialFailureDoesNotLogAccessToken(t *testing.T) { + const token = "eyJhbGciOiJIUzI1NiJ9.SECRET-LIVEKIT-JWT-PAYLOAD.c2lnbmF0dXJl" + + var logs bytes.Buffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + // Port 1 on loopback refuses connections, which is the exact branch where + // the wrapped *url.Error carries the query string. + proxy := NewLiveKitProxy("http://127.0.0.1:1", []string{"*"}) + r := httptest.NewRequest("GET", "/rtc?access_token="+token, nil) + r.Header.Set("Connection", "Upgrade") + r.Header.Set("Upgrade", "websocket") + w := httptest.NewRecorder() + proxy.ServeHTTP(w, r) + + if w.Code != http.StatusBadGateway { + t.Fatalf("expected 502 when the backend refuses the dial, got %d", w.Code) + } + out := logs.String() + if out == "" { + t.Fatal("expected the dial failure to be logged at all") + } + if strings.Contains(out, token) { + t.Fatalf("the LiveKit access token leaked into the log stream:\n%s", out) + } + if !strings.Contains(out, "backend dial failed") { + t.Errorf("the failure must still be diagnosable, got:\n%s", out) + } +} + func TestLiveKitProxy_InvalidURL_FallsBackToLocalhost(t *testing.T) { // Should not panic with invalid URL proxy := NewLiveKitProxy("://invalid", []string{"*"}) diff --git a/Server/api/upload_handler.go b/Server/api/upload_handler.go index 132c31a7..b27648cf 100644 --- a/Server/api/upload_handler.go +++ b/Server/api/upload_handler.go @@ -14,6 +14,7 @@ import ( "path/filepath" "strings" "time" + "unicode" "github.com/go-chi/chi/v5" "github.com/google/uuid" @@ -35,17 +36,31 @@ type uploadResponse struct { Height *int `json:"height,omitempty"` } -// sanitizeUploadFilename cleans an upload filename: strips control characters, -// removes path separators, and truncates to a safe length. +// sanitizeUploadFilename cleans an upload filename: strips control and +// invisible formatting characters, removes path separators, and truncates to a +// safe length. func sanitizeUploadFilename(name string) string { // Strip path components — use only the base name. name = filepath.Base(name) - // Remove control characters. + // filepath.Base only understands the *server* OS's separator, so a + // backslash survives on a Linux server and is then a path separator on the + // victim's Windows client, where the name is pre-filled into a save dialog. + if i := strings.LastIndexByte(name, '\\'); i >= 0 { + name = name[i+1:] + } + // Remove control characters and invisible formatting characters. var sb strings.Builder for _, r := range name { - if r >= 32 && r != 127 { // exclude control chars and DEL - sb.WriteRune(r) + // unicode.Cf covers the bidi overrides (U+202A–U+202E, U+2066–U+2069): + // invisible characters that reorder how the name renders, so an + // attachment can display a harmless-looking extension to every other + // member of the channel while really being an executable script — and + // the same string is what the native save dialog pre-fills. This is the + // rule auth.ValidateUsername already applies to usernames. + if unicode.IsControl(r) || unicode.In(r, unicode.Cf) { + continue } + sb.WriteRune(r) } name = strings.TrimSpace(sb.String()) // Truncate to 255 characters (filesystem limit). diff --git a/Server/api/upload_handler_test.go b/Server/api/upload_handler_test.go index 74587cd7..e3623d50 100644 --- a/Server/api/upload_handler_test.go +++ b/Server/api/upload_handler_test.go @@ -618,6 +618,77 @@ func TestUpload_SanitizesReservedFilenameToUnnamed(t *testing.T) { } } +// TestUpload_StripsBidiOverrideAndForeignSeparator locks the two gaps in +// sanitizeUploadFilename. The sanitizer filtered ASCII control bytes only, so +// U+202E RIGHT-TO-LEFT OVERRIDE survived into attachments.filename and was +// reflected to every other member of the channel — and into the native save +// dialog the client pre-fills — making a script display as though it ended in +// ".txt". Separately, filepath.Base only strips the server OS's separator, so a +// backslash survived on a Linux server and is a path separator on the victim's +// Windows client. +func TestUpload_StripsBidiOverrideAndForeignSeparator(t *testing.T) { + // Escaped rather than embedded: a literal U+202E would reorder this source + // file in every editor and terminal that renders it — which is the whole + // primitive under test. + const rtlOverride = "\u202e" + + cases := []struct { + name string + upload string + wantName string + }{ + { + name: "bidi override removed", + upload: "Q3_Report" + rtlOverride + "txt.bat", + wantName: "Q3_Reporttxt.bat", + }, + { + name: "other invisible formatting characters removed", + upload: "in\u200bvoice\u2066.pdf", // ZERO WIDTH SPACE, LEFT-TO-RIGHT ISOLATE + wantName: "invoice.pdf", + }, + { + name: "backslash path stripped regardless of server OS", + upload: `..\..\Windows\evil.bat`, + wantName: "evil.bat", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + database := newUploadTestDB(t) + store := newUploadTestStorage(t) + router := buildUploadRouter(database, store, nil) + token := uploadCreateToken(t, database, "bidi"+strings.ReplaceAll(tc.name, " ", ""), 1) + + rr := doUpload(t, router, token, "file", tc.upload, []byte("@echo off\r\n")) + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body: %s", rr.Code, rr.Body.String()) + } + var resp map[string]any + if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + got, _ := resp["filename"].(string) + if got != tc.wantName { + t.Errorf("filename = %q, want %q", got, tc.wantName) + } + + // The stored record must match — it is what every other client renders. + att, err := database.GetAttachmentByID(context.Background(), resp["id"].(string)) + if err != nil || att == nil { + t.Fatalf("GetAttachmentByID: %v", err) + } + if att.Filename != tc.wantName { + t.Errorf("DB filename = %q, want %q", att.Filename, tc.wantName) + } + if strings.ContainsAny(att.Filename, "\\/") { + t.Errorf("stored filename %q still contains a path separator", att.Filename) + } + }) + } +} + func TestUpload_SuccessfulUploadCreatesDBRecord(t *testing.T) { database := newUploadTestDB(t) store := newUploadTestStorage(t) diff --git a/Server/auth/helpers.go b/Server/auth/helpers.go index ff1379e0..3c344833 100644 --- a/Server/auth/helpers.go +++ b/Server/auth/helpers.go @@ -13,10 +13,21 @@ import ( // ValidateUsername checks that a (pre-trimmed) username meets naming rules: // - Length 2-32 runes (after trim) // - Only printable characters (no control chars, no zero-width chars) +// - Not inside the "[deleted-…]" namespace reserved for anonymised accounts // // Returns a descriptive error on failure, nil on success. func ValidateUsername(username string) error { username = strings.TrimSpace(username) + // db.DeleteAccount anonymises an account by renaming it to "[deleted-]", + // and users.username is UNIQUE COLLATE NOCASE. With the namespace + // unreserved, anyone could rename themselves to "[deleted-]" and + // make the victim's own account deletion fail on the unique index for as + // long as they held the name. Reserve the whole namespace, not just the + // exact "[deleted-]" form, so the collision-fallback names + // DeleteAccount generates are unavailable too. + if lower := strings.ToLower(username); strings.HasPrefix(lower, "[deleted-") && strings.HasSuffix(lower, "]") { + return fmt.Errorf("username is reserved") + } n := len([]rune(username)) if n < minUsernameLength { return fmt.Errorf("username must be at least %d characters", minUsernameLength) diff --git a/Server/auth/helpers_test.go b/Server/auth/helpers_test.go index 69eb5646..e71f217a 100644 --- a/Server/auth/helpers_test.go +++ b/Server/auth/helpers_test.go @@ -327,6 +327,39 @@ func TestValidateUsername_ValidNames(t *testing.T) { } } +// TestValidateUsername_ReservedDeletedNamespace locks the anonymisation +// namespace shut. db.DeleteAccount renames a deleted account to +// "[deleted-]" in a UNIQUE COLLATE NOCASE column, so while the name was +// registrable a member could take a chosen victim's and make every subsequent +// account-deletion attempt fail on the unique index. +func TestValidateUsername_ReservedDeletedNamespace(t *testing.T) { + reserved := []string{ + "[deleted-42]", // the exact anonymised form + "[DELETED-42]", // UNIQUE is COLLATE NOCASE, so case must not help + "[Deleted-1]", // mixed case + "[deleted-42-a3f19c]", // the collision-fallback form + "[deleted-]", // the namespace, not just the numeric form + } + for _, name := range reserved { + if err := auth.ValidateUsername(name); err == nil { + t.Errorf("ValidateUsername(%q) = nil, want error for reserved namespace", name) + } + } + + // Names that merely resemble it stay available. + allowed := []string{ + "[deleted", // no closing bracket + "deleted-42]", // no opening bracket + "[not-deleted-1]", // different namespace + "[cool]", // ordinary bracketed name + } + for _, name := range allowed { + if err := auth.ValidateUsername(name); err != nil { + t.Errorf("ValidateUsername(%q) = %v, want nil", name, err) + } + } +} + func TestValidateUsername_TooShort(t *testing.T) { cases := []string{ "", // empty diff --git a/Server/db/account.go b/Server/db/account.go index 1604baef..fad44fab 100644 --- a/Server/db/account.go +++ b/Server/db/account.go @@ -2,6 +2,9 @@ package db import ( "context" + "crypto/rand" + "database/sql" + "encoding/hex" "fmt" "strings" ) @@ -120,20 +123,15 @@ func (d *DB) DeleteAccount(ctx context.Context, userID int64) error { } // ── Anonymise user row ─────────────────────────────────────────────── - anonUsername := fmt.Sprintf("[deleted-%d]", userID) - if _, err := tx.ExecContext(ctx, - `UPDATE users - SET username = ?, - password = '', - avatar = NULL, - totp_secret = NULL, - status = 'offline', - banned = 1, - ban_reason = 'account deleted' - WHERE id = ?`, - anonUsername, userID, - ); err != nil { - return fmt.Errorf("DeleteAccount anonymise: %w", err) + // users.username is UNIQUE COLLATE NOCASE, so a third party holding + // "[deleted-]" would make this UPDATE fail, roll the whole + // transaction back, and deny the victim their own account deletion + // indefinitely. auth.ValidateUsername now reserves that namespace, but the + // erasure path must not depend on it: on a collision, fall back to a random + // suffix and retry. A SQLite constraint violation rolls back the statement, + // not the enclosing transaction, so retrying in place is safe. + if err := anonymiseUser(ctx, tx, userID); err != nil { + return err } if err := tx.Commit(); err != nil { @@ -141,3 +139,45 @@ func (d *DB) DeleteAccount(ctx context.Context, userID int64) error { } return nil } + +// anonymiseUserAttempts is how many names anonymiseUser will try: the canonical +// "[deleted-]" plus randomly suffixed variants. Exhausting it means the +// generator collided repeatedly, which is not something an attacker can force. +const anonymiseUserAttempts = 4 + +// anonymiseUser strips the user's credentials and personal fields and renames +// the row out of the way. The first candidate is the canonical +// "[deleted-]"; if that name is taken, later candidates append a random +// suffix so no third party can pin the account in place by squatting a +// predictable string. +func anonymiseUser(ctx context.Context, tx *sql.Tx, userID int64) error { + const anonymise = `UPDATE users + SET username = ?, + password = '', + avatar = NULL, + totp_secret = NULL, + status = 'offline', + banned = 1, + ban_reason = 'account deleted' + WHERE id = ?` + + var lastErr error + for attempt := range anonymiseUserAttempts { + name := fmt.Sprintf("[deleted-%d]", userID) + if attempt > 0 { + suffix := make([]byte, 6) + if _, err := rand.Read(suffix); err != nil { + return fmt.Errorf("DeleteAccount anonymise suffix: %w", err) + } + name = fmt.Sprintf("[deleted-%d-%s]", userID, hex.EncodeToString(suffix)) + } + _, lastErr = tx.ExecContext(ctx, anonymise, name, userID) + if lastErr == nil { + return nil + } + if !IsUniqueConstraintError(lastErr) { + return fmt.Errorf("DeleteAccount anonymise: %w", lastErr) + } + } + return fmt.Errorf("DeleteAccount anonymise: %w", lastErr) +} diff --git a/Server/db/account_test.go b/Server/db/account_test.go index e0654d6d..1481c73e 100644 --- a/Server/db/account_test.go +++ b/Server/db/account_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "testing" "github.com/owncord/server/db" @@ -190,6 +191,45 @@ func TestDeleteAccount_NonexistentUser(t *testing.T) { } } +// TestDeleteAccount_SquattedAnonNameStillDeletes locks the erasure path against +// a targeted denial of service: users.username is UNIQUE COLLATE NOCASE, so an +// attacker who renamed themselves to the victim's "[deleted-]" made the +// anonymising UPDATE fail, rolled the whole transaction back, and left the +// victim permanently unable to delete their own account. auth.ValidateUsername +// now reserves the namespace, but DeleteAccount must survive a squatted name on +// its own — including one already in the database from before the rule existed. +func TestDeleteAccount_SquattedAnonNameStillDeletes(t *testing.T) { + database := openMigratedMemory(t) + victimID := seedUser(t, database, "victim") + squatterID := seedUser(t, database, "squatter") + + squatted := fmt.Sprintf("[deleted-%d]", victimID) + if _, err := database.ExecContext(context.Background(), + "UPDATE users SET username = ? WHERE id = ?", squatted, squatterID, + ); err != nil { + t.Fatalf("squat username: %v", err) + } + + if err := database.DeleteAccount(context.Background(), victimID); err != nil { + t.Fatalf("DeleteAccount must not be blockable by a squatted name: %v", err) + } + + victim, err := database.GetUserByID(context.Background(), victimID) + if err != nil || victim == nil { + t.Fatalf("GetUserByID after delete: %v", err) + } + if strings.EqualFold(victim.Username, squatted) { + t.Fatalf("victim kept the squatted name %q", victim.Username) + } + if !strings.HasPrefix(victim.Username, fmt.Sprintf("[deleted-%d-", victimID)) { + t.Errorf("Username = %q, want a suffixed [deleted-%d-…] fallback", victim.Username, victimID) + } + // The erasure itself must still have happened. + if !victim.Banned || victim.PasswordHash != "" { + t.Errorf("account not anonymised: banned=%v password=%q", victim.Banned, victim.PasswordHash) + } +} + // ─── Helper ────────────────────────────────────────────────────────────────── func setRole(t *testing.T, database *db.DB, userID, roleID int64) { diff --git a/Server/service/channel.go b/Server/service/channel.go index 03f39320..f9a669f6 100644 --- a/Server/service/channel.go +++ b/Server/service/channel.go @@ -120,6 +120,12 @@ func (s *ChannelService) HandleTyping(ctx context.Context, userID, channelID int if dmErr != nil || !ok { return nil, nil //nolint:nilerr // typing indicators are best-effort; errors silently dropped } + // A blocked user must not be able to keep poking the blocker with + // typing indicators. Same gate as the other DM sinks; silently dropped + // here because typing is best-effort. + if blkErr := requireDMNotBlocked(ctx, s.st, userID, channelID); blkErr != nil { + return nil, nil //nolint:nilerr // best-effort: a blocked or unreadable DM emits nothing + } } else if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages) { return nil, nil // silent drop } diff --git a/Server/service/channel_test.go b/Server/service/channel_test.go index e135297c..cf9c5711 100644 --- a/Server/service/channel_test.go +++ b/Server/service/channel_test.go @@ -40,3 +40,41 @@ func TestListVisibleChannels_OverrideFetchErrorFailsClosed(t *testing.T) { t.Fatalf("ListVisibleChannels returned %d channels on override fetch failure, want none", len(got)) } } + +// TestHandleTyping_BlockedInDMEmitsNothing completes the DM-block sweep: a +// blocked user could still drive a repeatable typing indicator at the blocker, +// because HandleTyping authorized on DM participation alone. Typing is +// best-effort, so the refusal is a silent nil rather than an error. +func TestHandleTyping_BlockedInDMEmitsNothing(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages, + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUser(t, database, &db.User{ID: 2, Username: "bob"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedUserRole(t, database, 2, permissions.MemberRoleID) + seedChannel(t, database, &db.Channel{ID: 50, Name: "dm-1-2", Type: "dm"}) + seedDMParticipant(t, database, 50, 1) + seedDMParticipant(t, database, 50, 2) + + svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database))) + + ch, err := svc.HandleTyping(context.Background(), 1, 50, nil) + if err != nil || ch == nil { + t.Fatalf("unblocked DM typing must resolve the channel: ch=%v err=%v", ch, err) + } + + seedBlock(t, database, 2, 1) // bob blocks alice + + ch, err = svc.HandleTyping(context.Background(), 1, 50, nil) + if err != nil { + t.Fatalf("typing is best-effort, expected a silent drop, got err=%v", err) + } + if ch != nil { + t.Fatal("blocked user must not produce a typing broadcast") + } +} diff --git a/Server/service/message.go b/Server/service/message.go index e2e4a749..01c2bdc9 100644 --- a/Server/service/message.go +++ b/Server/service/message.go @@ -290,6 +290,9 @@ func (s *MessageService) EditMessage(ctx context.Context, userID, msgID int64, r if dmErr != nil || !ok { return nil, fmt.Errorf("%w: cannot edit this message", ErrForbidden) } + if blkErr := requireDMNotBlocked(ctx, s.st, userID, msg.ChannelID); blkErr != nil { + return nil, blkErr + } } else if !s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.SendMessages) { return nil, fmt.Errorf("%w: cannot edit this message", ErrForbidden) } @@ -353,21 +356,29 @@ func (s *MessageService) DeleteMessage(ctx context.Context, userID, msgID int64) ch, chErr := s.st.GetChannel(ctx, msg.ChannelID) isDM := chErr == nil && ch != nil && ch.Type == "dm" + var isMod bool if isDM { ok, dmErr := s.st.IsDMParticipant(ctx, userID, msg.ChannelID) if dmErr != nil || !ok { return nil, fmt.Errorf("%w: cannot delete this message", ErrForbidden) } } else { + // Require READ_MESSAGES alongside MANAGE_MESSAGES (and alongside + // SEND_MESSAGES on the author path) so a role explicitly denied access to + // a channel cannot delete messages in it. Mirrors handleReaction and + // checkSendPermission, which both require ReadMessages for non-DM channels. isMsgOwner := msg.UserID == userID - canManage := s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.ManageMessages) - canDelete := canManage || (isMsgOwner && s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.SendMessages)) + canManage := s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.ReadMessages|permissions.ManageMessages) + canDelete := canManage || (isMsgOwner && s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.ReadMessages|permissions.SendMessages)) if !canDelete { return nil, fmt.Errorf("%w: cannot delete this message", ErrForbidden) } + // db.DeleteMessage skips the ownership check when ismod is true, so the + // moderation flag must reuse the decision made above rather than + // re-checking MANAGE_MESSAGES without READ_MESSAGES. + isMod = canManage } - isMod := !isDM && s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.ManageMessages) if err := s.st.DeleteMessage(ctx, msgID, userID, isMod); err != nil { return nil, fmt.Errorf("%w: cannot delete this message", ErrForbidden) } @@ -446,6 +457,9 @@ func (s *MessageService) handleReaction(ctx context.Context, userID, msgID int64 if dmErr != nil || !ok { return nil, fmt.Errorf("%w: not a DM participant", ErrBadRequest) } + if blkErr := requireDMNotBlocked(ctx, s.st, userID, msg.ChannelID); blkErr != nil { + return nil, blkErr + } } else if !s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.ReadMessages|permissions.AddReactions) { // Require READ_MESSAGES in addition to ADD_REACTIONS so a user cannot // react in a channel they cannot read. Mirrors checkSendPermission, @@ -618,7 +632,14 @@ func (s *MessageService) SetMessagePinned(ctx context.Context, userID, channelID if err != nil || !ok { return fmt.Errorf("%w: access denied", ErrNotFound) } - } else if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ManageMessages) { + if blkErr := requireDMNotBlocked(ctx, s.st, userID, channelID); blkErr != nil { + return blkErr + } + } else if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages|permissions.ManageMessages) { + // Require READ_MESSAGES alongside MANAGE_MESSAGES so a role locked out + // of a private channel cannot mutate its pins — the admin panel's + // "Can access" toggle denies READ_MESSAGES|CONNECT_VOICE and leaves + // MANAGE_MESSAGES intact. Mirrors handleReaction and checkSendPermission. return fmt.Errorf("%w: missing MANAGE_MESSAGES permission", ErrForbidden) } // Verify message belongs to this channel. @@ -698,17 +719,7 @@ func (s *MessageService) checkSendPermission(ctx context.Context, userID, channe if !ok { return fmt.Errorf("%w: not a participant in this DM", ErrForbidden) } - recipient, err := s.st.GetDMRecipient(ctx, channelID, userID) - if err == nil && recipient != nil { - blocked, blkErr := s.st.IsEitherBlocked(ctx, userID, recipient.ID) - if blkErr != nil { - return fmt.Errorf("%w: failed to check block status: %v", ErrInternal, blkErr) - } - if blocked { - return fmt.Errorf("%w: cannot send messages — user is blocked", ErrBlocked) - } - } - return nil + return requireDMNotBlocked(ctx, s.st, userID, channelID) } if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages|permissions.SendMessages) { return fmt.Errorf("%w: missing SEND_MESSAGES permission", ErrForbidden) @@ -721,6 +732,39 @@ func (s *MessageService) checkSendPermission(ctx context.Context, userID, channe return nil } +// requireDMNotBlocked reports ErrBlocked when userID and the other participant +// of DM channelID have blocked each other in either direction. +// +// It is the single block-check implementation, called from every DM +// interaction sink — send, edit, react, pin and typing. Enforcing it on the +// send path alone left a blocked user an open channel to the blocker: editing +// an already-sent message fans MessageEditedDMEvent out to every participant, +// so arbitrary new text still reached the person who blocked them, and +// reactions and typing indicators did the same. +// +// Callers keep their own IsDMParticipant check. Its failure mode is +// deliberately different per sink (ErrForbidden for edit, ErrBadRequest for +// reactions, ErrNotFound for pins so a foreign DM's existence stays hidden) +// and flattening them here would change client-visible status codes. +// +// A GetDMRecipient lookup failure or a DM with no other participant is treated +// as "not blocked", carrying over the posture the send path has always had +// rather than newly failing closed on all five sinks at once. +func requireDMNotBlocked(ctx context.Context, st Store, userID, channelID int64) error { + recipient, err := st.GetDMRecipient(ctx, channelID, userID) + if err != nil || recipient == nil { + return nil //nolint:nilerr // carries over checkSendPermission's posture: a lookup failure or a DM with no other participant is not a block + } + blocked, blkErr := st.IsEitherBlocked(ctx, userID, recipient.ID) + if blkErr != nil { + return fmt.Errorf("%w: failed to check block status: %v", ErrInternal, blkErr) + } + if blocked { + return fmt.Errorf("%w: user is blocked", ErrBlocked) + } + return nil +} + // sanitizeContent validates and sanitizes message content. func sanitizeContent(raw string, allowEmpty bool) (string, error) { if len(raw) > maxMessageLen*4 { diff --git a/Server/service/message_test.go b/Server/service/message_test.go index ca61b003..2a303a7b 100644 --- a/Server/service/message_test.go +++ b/Server/service/message_test.go @@ -484,6 +484,65 @@ func TestDeleteMessage_ModCanDeleteOthersMessage(t *testing.T) { } } +// TestDeleteMessage_DeniedReadCannotDelete locks the channel-lockout invariant: +// when an admin unchecks "Can access" for a role, the panel writes +// deny = READ_MESSAGES|CONNECT_VOICE and leaves MANAGE_MESSAGES intact, so the +// delete gate must also require READ_MESSAGES — otherwise a moderator excluded +// from a private channel could soft-delete every message in it by enumerating +// message IDs. Mirrors api/channel_authz_test.go's denyReadMessages helper. +func TestDeleteMessage_DeniedReadCannotDelete(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.ModeratorRoleID, + Name: "moderator", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.ManageMessages, + Position: 10, + }) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages, + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUser(t, database, &db.User{ID: 2, Username: "mod_bob"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedUserRole(t, database, 2, permissions.ModeratorRoleID) + seedChannel(t, database, &db.Channel{ID: 10, Name: "staff-private", Type: "text"}) + + permSvc := NewPermissionService(database, permissions.NewChecker(database)) + svc := NewMessageService(database, permSvc, nil) + + sent, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", Content: "private discussion", + }) + if err != nil { + t.Fatalf("send: %v", err) + } + + // Admin unchecks "Can access" for both roles: READ_MESSAGES and + // CONNECT_VOICE are denied, MANAGE_MESSAGES survives the deny mask. + denyPrivate := permissions.ReadMessages | permissions.ConnectVoice + seedChannelOverride(t, database, permissions.ModeratorRoleID, 10, 0, denyPrivate) + seedChannelOverride(t, database, permissions.MemberRoleID, 10, 0, denyPrivate) + permSvc.InvalidateChannel(10) + + if _, err := svc.DeleteMessage(context.Background(), 2, sent.MessageID); !errors.Is(err, ErrForbidden) { + t.Fatalf("moderator denied READ_MESSAGES must not delete: got %v", err) + } + if _, err := svc.DeleteMessage(context.Background(), 1, sent.MessageID); !errors.Is(err, ErrForbidden) { + t.Fatalf("author denied READ_MESSAGES must not delete: got %v", err) + } + + msg, err := database.GetMessage(context.Background(), sent.MessageID) + if err != nil || msg == nil { + t.Fatalf("GetMessage: %v", err) + } + if msg.Deleted { + t.Fatal("message must survive delete attempts from a locked-out role") + } +} + func TestDeleteMessage_InvalidMessageID(t *testing.T) { svc, _ := newTestMessageService(t) @@ -515,3 +574,133 @@ func TestSendMessage_HTMLSanitized(t *testing.T) { t.Fatal("expected safe text to remain in content") } } + +// TestSetMessagePinned_DeniedReadCannotPin locks the same channel-lockout +// invariant as TestDeleteMessage_DeniedReadCannotDelete, on the pin sink: an +// admin unchecking "Can access" writes deny = READ_MESSAGES|CONNECT_VOICE and +// leaves MANAGE_MESSAGES intact, so the pin gate must require READ_MESSAGES +// too — otherwise a locked-out moderator can mutate the pin list the real +// members see, and use the success/not-found split as a message-ID oracle. +func TestSetMessagePinned_DeniedReadCannotPin(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.ModeratorRoleID, + Name: "moderator", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.ManageMessages, + Position: 10, + }) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages, + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUser(t, database, &db.User{ID: 2, Username: "mod_bob"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedUserRole(t, database, 2, permissions.ModeratorRoleID) + seedChannel(t, database, &db.Channel{ID: 10, Name: "staff-private", Type: "text"}) + + permSvc := NewPermissionService(database, permissions.NewChecker(database)) + svc := NewMessageService(database, permSvc, nil) + + sent, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", Content: "announcement", + }) + if err != nil { + t.Fatalf("send: %v", err) + } + + // While the moderator can still read the channel, pinning works. + if err := svc.SetMessagePinned(context.Background(), 2, 10, sent.MessageID, true); err != nil { + t.Fatalf("moderator with READ_MESSAGES must be able to pin: %v", err) + } + + denyPrivate := permissions.ReadMessages | permissions.ConnectVoice + seedChannelOverride(t, database, permissions.ModeratorRoleID, 10, 0, denyPrivate) + permSvc.InvalidateChannel(10) + + if err := svc.SetMessagePinned(context.Background(), 2, 10, sent.MessageID, false); !errors.Is(err, ErrForbidden) { + t.Fatalf("moderator denied READ_MESSAGES must not unpin: got %v", err) + } + // The existence oracle is closed with it: an id that is not in this channel + // is refused by the same permission check, not by a distinguishable + // not-found answer. + if err := svc.SetMessagePinned(context.Background(), 2, 10, sent.MessageID+999, true); !errors.Is(err, ErrForbidden) { + t.Fatalf("denied role must not learn which message ids exist: got %v", err) + } + + pinned, err := database.GetPinnedMessages(context.Background(), 10, 1) + if err != nil { + t.Fatalf("GetPinnedMessages: %v", err) + } + if len(pinned) != 1 { + t.Fatalf("pin state must survive the locked-out unpin attempt, got %d pinned", len(pinned)) + } +} + +// TestDMBlock_EnforcedOnEveryInteractionSink locks the block invariant across +// all DM verbs, not just send. Blocking used to be checked only in +// checkSendPermission, so a blocked user kept a live channel to the blocker: +// editing an already-sent message fans chat_edited out to both participants, so +// arbitrary new text still arrived, and reactions and pins did the same. +func TestDMBlock_EnforcedOnEveryInteractionSink(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AddReactions, + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUser(t, database, &db.User{ID: 2, Username: "bob"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedUserRole(t, database, 2, permissions.MemberRoleID) + seedChannel(t, database, &db.Channel{ID: 50, Name: "dm-1-2", Type: "dm"}) + seedDMParticipant(t, database, 50, 1) + seedDMParticipant(t, database, 50, 2) + + permSvc := NewPermissionService(database, permissions.NewChecker(database)) + svc := NewMessageService(database, permSvc, nil) + + sent, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 50, UserID: 1, Username: "alice", Content: "hi bob", + }) + if err != nil { + t.Fatalf("send before block: %v", err) + } + + seedBlock(t, database, 2, 1) // bob blocks alice + + // Send — the one path that was already enforced. + if _, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 50, UserID: 1, Username: "alice", Content: "let me back in", + }); !errors.Is(err, ErrBlocked) { + t.Fatalf("blocked send must be refused: got %v", err) + } + + // Edit — the finding's primary sink. + if _, err := svc.EditMessage(context.Background(), 1, sent.MessageID, "abusive replacement"); !errors.Is(err, ErrBlocked) { + t.Fatalf("blocked edit must be refused: got %v", err) + } + msg, err := database.GetMessage(context.Background(), sent.MessageID) + if err != nil || msg == nil { + t.Fatalf("GetMessage: %v", err) + } + if msg.Content != "hi bob" { + t.Fatalf("content must be unchanged after a blocked edit, got %q", msg.Content) + } + + // Reactions and pins are the same class of repeatable notification. + if _, err := svc.AddReaction(context.Background(), 1, sent.MessageID, "👋"); !errors.Is(err, ErrBlocked) { + t.Fatalf("blocked reaction must be refused: got %v", err) + } + if err := svc.SetMessagePinned(context.Background(), 1, 50, sent.MessageID, true); !errors.Is(err, ErrBlocked) { + t.Fatalf("blocked pin must be refused: got %v", err) + } + + // The block is symmetric, matching the pre-existing send-path semantics. + if _, err := svc.EditMessage(context.Background(), 2, sent.MessageID, "bob edits"); !errors.Is(err, ErrBlocked) { + t.Fatalf("blocker is equally refused, matching IsEitherBlocked: got %v", err) + } +} diff --git a/Server/ws/coverage_boost_test.go b/Server/ws/coverage_boost_test.go index f93112c3..6cb541b0 100644 --- a/Server/ws/coverage_boost_test.go +++ b/Server/ws/coverage_boost_test.go @@ -15,6 +15,7 @@ import ( "github.com/owncord/server/auth" "github.com/owncord/server/config" "github.com/owncord/server/db" + "github.com/owncord/server/permissions" "github.com/owncord/server/service" "github.com/owncord/server/ws" ) @@ -1452,6 +1453,78 @@ func TestHandleVoiceJoin_FullFlow(t *testing.T) { } } +// TestVoiceState_NotDeliveredToRolesDeniedRead locks the visibility invariant +// for voice metadata: voice_state / voice_leave used to go out via +// BroadcastToAll, so every authenticated client learned the membership and +// camera/mute state of voice channels that channel_overrides hides from their +// role — even though the ready payload deliberately filters them out. A member +// who can read the channel must still receive them. +func TestVoiceState_NotDeliveredToRolesDeniedRead(t *testing.T) { + hub, database := newCoverageHub(t) + joiner := seedCoverageOwner(t, database, "vs-joiner") + vcID := seedVoiceChannel(t, database, "vs-private-vc") + + // Two plain members (role 4). One is locked out of the channel with the + // override the admin panel writes when "Can access" is unchecked. + newMember := func(name string) *db.User { + t.Helper() + if _, err := database.CreateUser(context.Background(), name, "hash", 4); err != nil { + t.Fatalf("CreateUser %s: %v", name, err) + } + u, err := database.GetUserByUsername(context.Background(), name) + if err != nil || u == nil { + t.Fatalf("GetUserByUsername %s: %v", name, err) + } + return u + } + insider := newMember("vs-insider") + outsiderRole := int64(3) // Moderator: a distinct role so the deny is role-scoped + outsider := newMember("vs-outsider") + if _, err := database.ExecContext(context.Background(), + `UPDATE users SET role_id = ? WHERE id = ?`, outsiderRole, outsider.ID, + ); err != nil { + t.Fatalf("reassign outsider role: %v", err) + } + if err := database.UpsertChannelOverride(context.Background(), vcID, outsiderRole, 0, + permissions.ReadMessages|permissions.ConnectVoice); err != nil { + t.Fatalf("UpsertChannelOverride: %v", err) + } + + insiderSend := make(chan []byte, 64) + outsiderSend := make(chan []byte, 64) + hub.Register(ws.NewTestClientWithUser(hub, insider, 0, insiderSend)) + hub.Register(ws.NewTestClientWithUser(hub, outsider, 0, outsiderSend)) + + joinerSend := make(chan []byte, 64) + jc := ws.NewTestClientWithUser(hub, joiner, 0, joinerSend) + hub.Register(jc) + time.Sleep(30 * time.Millisecond) + + raw, _ := json.Marshal(map[string]any{ + "type": "voice_join", + "payload": map[string]any{"channel_id": vcID}, + }) + hub.HandleMessageForTest(jc, raw) + time.Sleep(150 * time.Millisecond) + + countVoiceState := func(ch <-chan []byte) int { + n := 0 + for _, msg := range drainChanTimeout(ch, 300*time.Millisecond) { + var env map[string]any + if json.Unmarshal(msg, &env) == nil && env["type"] == "voice_state" { + n++ + } + } + return n + } + if got := countVoiceState(insiderSend); got == 0 { + t.Error("a member who may READ the channel must still receive voice_state") + } + if got := countVoiceState(outsiderSend); got != 0 { + t.Errorf("a role denied READ received %d voice_state events, want 0", got) + } +} + func TestHandleVoiceJoin_AlreadyInSameChannel(t *testing.T) { hub, database := newCoverageHub(t) user := seedCoverageOwner(t, database, "vj-same-user") @@ -2407,9 +2480,14 @@ func TestHandleVoiceTokenRefresh_InVoice_ReturnsToken(t *testing.T) { } func TestHandleVoiceTokenRefresh_NilUser(t *testing.T) { - hub, _ := newCoverageHub(t) + hub, database := newCoverageHub(t) + // The client deliberately carries no *db.User — that is what this test + // covers — but the row must exist so the CONNECT_VOICE re-check can resolve + // a role. Without it the handler stops at FORBIDDEN and never reaches the + // missing-voice-state branch under test. + user := seedCoverageOwner(t, database, "vtr-nil-user") send := make(chan []byte, 16) - c := ws.NewTestClient(hub, 1, send) + c := ws.NewTestClient(hub, user.ID, send) hub.Register(c) time.Sleep(20 * time.Millisecond) @@ -2693,6 +2771,62 @@ func TestSweepStaleVoiceStates_MismatchedChannelIsGhost(t *testing.T) { } } +// TestSweepStaleVoiceStates_EvictsRevokedConnectVoice locks the revocation half +// of the voice-permission invariant: nothing in ws re-validated CONNECT_VOICE +// for a connection that stays open, so stripping the bit blocked future joins +// but left the offender in the room. The sweep must now evict them — DB row +// gone and the client's own voice state cleared. +func TestSweepStaleVoiceStates_EvictsRevokedConnectVoice(t *testing.T) { + hub, database := newCoverageHub(t) + // Member role (id 4), not Owner: admins bypass every channel check. + if _, err := database.CreateUser(context.Background(), "sweep-revoked", "hash", 4); err != nil { + t.Fatalf("CreateUser: %v", err) + } + user, err := database.GetUserByUsername(context.Background(), "sweep-revoked") + if err != nil || user == nil { + t.Fatalf("GetUserByUsername: %v", err) + } + vcID := seedVoiceChannel(t, database, "sweep-revoked-vc") + + send := make(chan []byte, 64) + c := ws.NewTestClientWithUser(hub, user, 0, send) + hub.Register(c) + time.Sleep(20 * time.Millisecond) + + if joinErr := database.JoinVoiceChannel(context.Background(), user.ID, vcID); joinErr != nil { + t.Fatalf("JoinVoiceChannel: %v", joinErr) + } + vs, err := database.GetVoiceState(context.Background(), user.ID) + if err != nil || vs == nil { + t.Fatalf("GetVoiceState after join: %v", err) + } + ws.SetClientVoiceStateForTest(c, vcID, vs.JoinedAt) + + // Still permitted → the sweep leaves them alone. + hub.SweepStaleVoiceStatesForTest() + time.Sleep(100 * time.Millisecond) + if state, _ := database.GetVoiceState(context.Background(), user.ID); state == nil { + t.Fatal("a permitted participant must survive the sweep") + } + + // Moderator revokes CONNECT_VOICE on this channel for the Member role. + if permErr := database.UpsertChannelOverride( + context.Background(), vcID, 4, 0, permissions.ConnectVoice, + ); permErr != nil { + t.Fatalf("UpsertChannelOverride: %v", permErr) + } + + hub.SweepStaleVoiceStatesForTest() + time.Sleep(200 * time.Millisecond) + + if state, _ := database.GetVoiceState(context.Background(), user.ID); state != nil { + t.Error("revoked participant's voice state must be deleted by the sweep") + } + if chID := ws.GetClientVoiceChIDForTest(c); chID != 0 { + t.Errorf("revoked participant's client voice state must be cleared, got channel %d", chID) + } +} + // ─── BroadcastToChannel / BroadcastToAll full-channel path ────────────────── func TestBroadcastToChannel_DropsWhenFull(t *testing.T) { diff --git a/Server/ws/emit.go b/Server/ws/emit.go index bf8ab1cf..db59d858 100644 --- a/Server/ws/emit.go +++ b/Server/ws/emit.go @@ -1,12 +1,14 @@ package ws import ( + "context" "fmt" "log/slog" ) // EmitEvents routes typed events to the appropriate broadcast methods. -// Called from readPump goroutines after a V2 handler returns. +// Called from readPump goroutines after a V2 handler returns. ctx carries the +// dispatching connection's cancellation for the routes that hit the database. // // CRITICAL ordering: SequencedDMEvent MUST be checked before ChannelEvent // because DM events implement both interfaces. The SequencedDMEvent path @@ -15,7 +17,7 @@ import ( // VoiceChannelEvent MUST be checked before ExcludeSenderEvent because voice // events implement a superset of ExcludeSender semantics but target by voice // channel membership rather than channel focus. -func (h *Hub) EmitEvents(events []Event) { +func (h *Hub) EmitEvents(ctx context.Context, events []Event) { for _, ev := range events { switch e := ev.(type) { case SequencedDMEvent: @@ -33,6 +35,11 @@ func (h *Hub) EmitEvents(events []Event) { h.SendToUserHigh(e.TargetUserID(), e.Payload()) case ChannelEvent: h.BroadcastToChannel(e.ChannelID(), e.Payload()) + case VoiceVisibilityEvent: + // Server-wide, but never to a client that cannot read the channel. + // ctx is threaded from the dispatching connection so the audience + // lookup dies with it rather than outliving the request. + h.broadcastVoiceEvent(ctx, e.VisibleChannelID(), e.Payload()) case BroadcastAllEvent: // Check concrete type: presence is low-priority, others are normal. if _, isPresence := ev.(PresenceEvent); isPresence { diff --git a/Server/ws/emit_test.go b/Server/ws/emit_test.go index 09fe4774..f8e1fdd6 100644 --- a/Server/ws/emit_test.go +++ b/Server/ws/emit_test.go @@ -1,6 +1,7 @@ package ws import ( + "context" "testing" "time" ) @@ -166,7 +167,7 @@ func TestEmitEvents_ChannelEvent_CallsBroadcastToChannel(t *testing.T) { go h.Run() defer h.Stop() - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) // Give the hub loop time to deliver. msgs := drainChan(send1, 100*time.Millisecond) @@ -183,7 +184,7 @@ func TestEmitEvents_ExcludeSenderEvent(t *testing.T) { payload := []byte(`{"type":"typing"}`) events := []Event{stubExcludeSenderEvent{channelID: 42, excludeUserID: 1, payload: payload}} - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) // broadcastExcludeLow is synchronous — check immediately. senderMsgs := drainChan(sendSender, 50*time.Millisecond) @@ -210,7 +211,7 @@ func TestEmitEvents_SequencedDMEvent(t *testing.T) { payload: payload, }} - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) msgs1 := drainChan(send1, 50*time.Millisecond) msgs2 := drainChan(send2, 50*time.Millisecond) @@ -235,7 +236,7 @@ func TestEmitEvents_UserTargetedEvent(t *testing.T) { payload := []byte(`{"type":"targeted"}`) events := []Event{stubUserTargetedEvent{targetUserID: 2, payload: payload}} - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) msgs1 := drainChan(send1, 50*time.Millisecond) msgs2 := drainChan(send2, 50*time.Millisecond) @@ -260,7 +261,7 @@ func TestEmitEvents_BroadcastAllEvent(t *testing.T) { go h.Run() defer h.Stop() - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) msgs1 := drainChan(send1, 100*time.Millisecond) msgs2 := drainChan(send2, 100*time.Millisecond) @@ -286,7 +287,7 @@ func TestEmitEvents_VoiceChannelEvent(t *testing.T) { payload: payload, }} - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) senderMsgs := drainChan(sendSender, 50*time.Millisecond) otherMsgs := drainChan(sendOther, 50*time.Millisecond) @@ -319,7 +320,7 @@ func TestEmitEvents_VoiceChannelGuardedEvent(t *testing.T) { payload: payload, }} - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) targetMsgs := drainChan(sendTarget, 50*time.Millisecond) otherMsgs := drainChan(sendOther, 50*time.Millisecond) @@ -348,7 +349,7 @@ func TestEmitEvents_VoiceChannelGuardedEvent_TargetNotInChannel(t *testing.T) { payload: payload, }} - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) targetMsgs := drainChan(sendTarget, 50*time.Millisecond) if len(targetMsgs) != 0 { @@ -361,8 +362,8 @@ func TestEmitEvents_EmptyEvents_NoOp(t *testing.T) { _ = registerEmitTestClient(h, 1, 42) // Should not panic or block. - h.EmitEvents(nil) - h.EmitEvents([]Event{}) + h.EmitEvents(context.Background(), nil) + h.EmitEvents(context.Background(), []Event{}) } func TestEmitEvents_MixedEventTypes_AllRouted(t *testing.T) { @@ -379,7 +380,7 @@ func TestEmitEvents_MixedEventTypes_AllRouted(t *testing.T) { stubUserTargetedEvent{targetUserID: 2, payload: []byte(`{"e":2}`)}, } - h.EmitEvents(events) + h.EmitEvents(context.Background(), events) chMsgs := drainChan(sendCh, 100*time.Millisecond) targetMsgs := drainChan(sendTarget, 100*time.Millisecond) @@ -397,5 +398,5 @@ func TestEmitEvents_UnknownType_LogsWarning(t *testing.T) { _ = registerEmitTestClient(h, 1, 42) // Should not panic; logs a warning (we verify no crash, not log content). - h.EmitEvents([]Event{stubUnknownEvent{}}) + h.EmitEvents(context.Background(), []Event{stubUnknownEvent{}}) } diff --git a/Server/ws/event.go b/Server/ws/event.go index c62bd1a9..59267310 100644 --- a/Server/ws/event.go +++ b/Server/ws/event.go @@ -98,6 +98,16 @@ type BroadcastAllEvent interface { Payload() []byte } +// VoiceVisibilityEvent routes to Hub.broadcastVoiceEvent: server-wide in scope, +// but delivered only to clients whose role may READ the named channel, and +// tagged with it so reconnect replay filters it the same way. MUST be checked +// before BroadcastAllEvent, which it would otherwise satisfy. +type VoiceVisibilityEvent interface { + Event + VisibleChannelID() int64 + Payload() []byte +} + // VoiceChannelEvent routes to Hub.sendToVoiceChannelExcept (ephemeral, // targets voice channel participants excluding sender). type VoiceChannelEvent interface { @@ -256,13 +266,16 @@ func (e ReactionDMEvent) ParticipantIDs() []int64 { } func (e ReactionDMEvent) Payload() []byte { return e.payload } -// VoiceStateEvent is a voice state broadcast to all connected clients. +// VoiceStateEvent is a voice state update fanned out to the clients whose role +// may READ the voice channel it describes. Satisfies VoiceVisibilityEvent. type VoiceStateEvent struct { - payload []byte + voiceChannelID int64 + payload []byte } -func (e VoiceStateEvent) EventType() string { return MsgTypeVoiceState } -func (e VoiceStateEvent) Payload() []byte { return e.payload } +func (e VoiceStateEvent) EventType() string { return MsgTypeVoiceState } +func (e VoiceStateEvent) VisibleChannelID() int64 { return e.voiceChannelID } +func (e VoiceStateEvent) Payload() []byte { return e.payload } // PluginBroadcastEvent is a plugin slash-command result broadcast to a channel // (sequenced, replayable). Emitted by the chat_command handler after the diff --git a/Server/ws/export_test.go b/Server/ws/export_test.go index 4e3ae9f8..fefa4eea 100644 --- a/Server/ws/export_test.go +++ b/Server/ws/export_test.go @@ -273,9 +273,16 @@ func WsToHTTPForTest(wsURL string) string { } // RegisterNowForTest exposes registerNow for external tests so clients are -// visible immediately (no channel round-trip through hub.Run). +// visible immediately (no channel round-trip through hub.Run). No channels are +// readable, matching the hub-loop registration path. func (h *Hub) RegisterNowForTest(c *Client) { - h.registerNow(c) + h.registerNow(c, nil) +} + +// RegisterNowWithReadableForTest exposes registerNow with an explicit +// READ_MESSAGES channel set, as the handshake paths in serve.go supply it. +func (h *Hub) RegisterNowWithReadableForTest(c *Client, readableChannelIDs map[int64]bool) { + h.registerNow(c, readableChannelIDs) } // ClearVoiceStateForTest exposes clearVoiceState for external tests. diff --git a/Server/ws/handler_v2_voice_e2ee_offer_test.go b/Server/ws/handler_v2_voice_e2ee_offer_test.go index eee3843d..da76014d 100644 --- a/Server/ws/handler_v2_voice_e2ee_offer_test.go +++ b/Server/ws/handler_v2_voice_e2ee_offer_test.go @@ -93,6 +93,56 @@ func TestVoiceE2EEOfferV2_RotationBurstNotRateLimited(t *testing.T) { } } +// TestVoiceE2EEOfferV2_RejectedOffersAllocateNoLimiterState locks the fix for +// the unbounded-map defect: the limiter key interpolated the client-supplied +// target_user_id and ran before every validation, so one authenticated socket — +// not in voice, not a key holder — could insert a fresh entry into the shared +// process-wide RateLimiter on every frame. Entries live ~20 minutes, so the +// spray was a memory-exhaustion lever against the whole server. +func TestVoiceE2EEOfferV2_RejectedOffersAllocateNoLimiterState(t *testing.T) { + limiter := auth.NewRateLimiter() + + // (a) Not in a voice channel at all — the cheapest rejection. + deps := offerDeps(false) + deps.Limiter = limiter + info := ClientInfo{UserID: 1, VoiceChannelID: 0} + for target := int64(1); target <= 500; target++ { + cmd := VoiceE2EEOfferCmd{userID: 1, targetUserID: target, encryptedKey: validEncKey, iv: validIV} + if result := handleVoiceE2EEOfferV2(context.Background(), cmd, info, deps); result.Error == nil { + t.Fatalf("offer from a client not in voice must be rejected (target %d)", target) + } + } + if windows, _ := limiter.Len(); windows != 0 { + t.Fatalf("a client not in voice allocated %d limiter entries, want 0", windows) + } + + // (b) In voice but not the key holder — rejected later, still allocates nothing. + info = ClientInfo{UserID: 1, VoiceChannelID: 100} + for target := int64(1); target <= 500; target++ { + cmd := VoiceE2EEOfferCmd{userID: 1, targetUserID: target, encryptedKey: validEncKey, iv: validIV} + if result := handleVoiceE2EEOfferV2(context.Background(), cmd, info, deps); result.Error == nil { + t.Fatalf("offer from a non-key-holder must be rejected (target %d)", target) + } + } + if windows, _ := limiter.Len(); windows != 0 { + t.Fatalf("a non-key-holder allocated %d limiter entries, want 0", windows) + } + + // (c) A real key holder is still budgeted, and its entries are bounded by + // the channel-keyed outer budget rather than by attacker-chosen target ids. + holderDeps := offerDeps(true) + holderDeps.Limiter = limiter + for target := int64(1); target <= 500; target++ { + cmd := VoiceE2EEOfferCmd{userID: 1, targetUserID: target, encryptedKey: validEncKey, iv: validIV} + handleVoiceE2EEOfferV2(context.Background(), cmd, info, holderDeps) + } + windows, _ := limiter.Len() + if windows > voiceE2EEOfferRateLimit+1 { + t.Fatalf("key holder spraying 500 target ids allocated %d limiter entries, want at most %d", + windows, voiceE2EEOfferRateLimit+1) + } +} + func TestVoiceE2EEOfferV2_NotInVoiceChannel(t *testing.T) { deps := offerDeps(true) cmd := VoiceE2EEOfferCmd{userID: 1, targetUserID: 2, encryptedKey: validEncKey, iv: validIV} diff --git a/Server/ws/handler_v2_voice_token_test.go b/Server/ws/handler_v2_voice_token_test.go index a4cd5d6e..a7d65b0a 100644 --- a/Server/ws/handler_v2_voice_token_test.go +++ b/Server/ws/handler_v2_voice_token_test.go @@ -6,6 +6,8 @@ import ( "testing" "github.com/owncord/server/auth" + "github.com/owncord/server/db" + "github.com/owncord/server/permissions" ) // ── mocks ────────────────────────────────────────────────────────────────────── @@ -33,16 +35,65 @@ func (m *mockKeyHolder) IsVoiceKeyHolder(_, _ int64) bool { return m.isHolder } // ── tests ────────────────────────────────────────────────────────────────────── -func tokenRefreshDeps() VoiceDeps { +// tokenRefreshDeps wires a real in-memory DB because the handler now re-checks +// CONNECT_VOICE before minting a token: user 1 holds a voice-only role (READ + +// CONNECT_VOICE, no SPEAK/VIDEO/SCREEN_SHARE) on voice channel 100. +func tokenRefreshDeps(t *testing.T) VoiceDeps { + t.Helper() + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + if migErr := db.Migrate(database); migErr != nil { + t.Fatalf("Migrate: %v", migErr) + } + t.Cleanup(func() { _ = database.Close() }) + + seedVoiceOnlyRole(t, database, voiceOnlyRoleID, permissions.ReadMessages|permissions.ConnectVoice) + seedTokenRefreshUser(t, database, 1, voiceOnlyRoleID) + if _, execErr := database.ExecContext(context.Background(), + `INSERT INTO channels (id, name, type, position) VALUES (100, 'voice-100', 'voice', 0)`, + ); execErr != nil { + t.Fatalf("seed channel: %v", execErr) + } + return VoiceDeps{ - Limiter: auth.NewRateLimiter(), - TokenGen: &mockTokenGen{token: "jwt-test-token", url: "ws://lk:7880"}, - KeyHolder: &mockKeyHolder{isHolder: true}, + DB: database, + Permissions: permissions.NewChecker(database), + Limiter: auth.NewRateLimiter(), + TokenGen: &mockTokenGen{token: "jwt-test-token", url: "ws://lk:7880"}, + KeyHolder: &mockKeyHolder{isHolder: true}, + } +} + +// voiceOnlyRoleID is a fixed id well clear of the migration-seeded defaults. +const voiceOnlyRoleID = 900 + +func seedVoiceOnlyRole(t *testing.T, database *db.DB, roleID, perms int64) { + t.Helper() + if _, err := database.ExecContext(context.Background(), + `INSERT INTO roles (id, name, permissions, position, is_default) + VALUES (?, 'voice-only', ?, 5, 0) + ON CONFLICT(id) DO UPDATE SET permissions = excluded.permissions`, + roleID, perms, + ); err != nil { + t.Fatalf("seed role: %v", err) + } +} + +func seedTokenRefreshUser(t *testing.T, database *db.DB, userID, roleID int64) { + t.Helper() + if _, err := database.ExecContext(context.Background(), + `INSERT INTO users (id, username, password, role_id) VALUES (?, 'alice', '', ?) + ON CONFLICT(id) DO UPDATE SET role_id = excluded.role_id`, + userID, roleID, + ); err != nil { + t.Fatalf("seed user: %v", err) } } func TestVoiceTokenRefreshV2_HappyPath(t *testing.T) { - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{ UserID: 1, @@ -70,7 +121,7 @@ func TestVoiceTokenRefreshV2_HappyPath(t *testing.T) { } func TestVoiceTokenRefreshV2_NotInVoice(t *testing.T) { - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{UserID: 1, VoiceChannelID: 0} @@ -89,7 +140,7 @@ func TestVoiceTokenRefreshV2_NotInVoice(t *testing.T) { } func TestVoiceTokenRefreshV2_RateLimited(t *testing.T) { - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{UserID: 1, Username: "alice", VoiceChannelID: 100, VoiceJoinToken: "t"} @@ -111,7 +162,7 @@ func TestVoiceTokenRefreshV2_RateLimited(t *testing.T) { } func TestVoiceTokenRefreshV2_TokenGenNil(t *testing.T) { - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) deps.TokenGen = nil cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{UserID: 1, VoiceChannelID: 100, VoiceJoinToken: "t"} @@ -131,7 +182,7 @@ func TestVoiceTokenRefreshV2_TokenGenNil(t *testing.T) { } func TestVoiceTokenRefreshV2_GenerateTokenError(t *testing.T) { - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) deps.TokenGen = &mockTokenGen{err: context.DeadlineExceeded, url: "ws://lk:7880"} cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{UserID: 1, Username: "alice", VoiceChannelID: 100, VoiceJoinToken: "t"} @@ -151,7 +202,7 @@ func TestVoiceTokenRefreshV2_GenerateTokenError(t *testing.T) { } func TestVoiceTokenRefreshV2_IsKeyHolderReflectedInReply(t *testing.T) { - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) deps.KeyHolder = &mockKeyHolder{isHolder: false} cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{UserID: 1, Username: "alice", VoiceChannelID: 100, VoiceJoinToken: "t"} @@ -175,7 +226,7 @@ func TestVoiceTokenRefreshV2_IsKeyHolderReflectedInReply(t *testing.T) { } func TestVoiceTokenRefreshV2_NoEvents(t *testing.T) { - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{UserID: 1, Username: "alice", VoiceChannelID: 100, VoiceJoinToken: "t"} @@ -187,13 +238,13 @@ func TestVoiceTokenRefreshV2_NoEvents(t *testing.T) { } func TestVoiceTokenRefreshV2_PermissionsPassedToTokenGen(t *testing.T) { - // Use a capturing mock to verify permissions are forwarded. + // Use a capturing mock to verify permissions are forwarded. The fixture role + // holds CONNECT_VOICE (so the token is minted at all) but none of + // SPEAK_VOICE / USE_VIDEO / SHARE_SCREEN, so each publish grant must be + // false while subscribe stays unconditionally true. captureMock := &capturingTokenGen{token: "jwt", url: "ws://lk"} - deps := tokenRefreshDeps() + deps := tokenRefreshDeps(t) deps.TokenGen = captureMock - // No Permissions or DB set → hasPerm returns false for all. - deps.Permissions = nil - deps.DB = nil cmd := VoiceTokenRefreshCmd{userID: 1} info := ClientInfo{UserID: 1, Username: "alice", VoiceChannelID: 100, VoiceJoinToken: "t"} @@ -219,6 +270,45 @@ func TestVoiceTokenRefreshV2_PermissionsPassedToTokenGen(t *testing.T) { } } +// TestVoiceTokenRefreshV2_RevokedConnectVoiceRefusedAndEvicts locks the +// revocation invariant: voice_join was the only place CONNECT_VOICE was ever +// checked, so a user stripped of it mid-session could keep re-minting LiveKit +// room-join grants (one per 60s, CanSubscribe=true) for a channel they are no +// longer allowed in. The refusal must also evict, or the live SFU session +// simply outlives the permission. +func TestVoiceTokenRefreshV2_RevokedConnectVoiceRefusedAndEvicts(t *testing.T) { + deps := tokenRefreshDeps(t) + cmd := VoiceTokenRefreshCmd{userID: 1} + info := ClientInfo{UserID: 1, Username: "alice", VoiceChannelID: 100, VoiceJoinToken: "t"} + + // Still authorized: a token is issued. + if result := handleVoiceTokenRefreshV2(context.Background(), cmd, info, deps); result.Error != nil { + t.Fatalf("authorized refresh must succeed: %v", result.Error) + } + + // A moderator strips CONNECT_VOICE from the role. + seedVoiceOnlyRole(t, deps.DB, voiceOnlyRoleID, permissions.ReadMessages) + deps.Limiter = auth.NewRateLimiter() // clear the 1-per-60s budget for this second call + + result := handleVoiceTokenRefreshV2(context.Background(), cmd, info, deps) + if result.Error == nil { + t.Fatal("revoked CONNECT_VOICE must not mint a fresh SFU token") + } + ce, ok := result.Error.(ClientError) + if !ok { + t.Fatalf("expected ClientError, got %T", result.Error) + } + if ce.Code != ErrCodeForbidden { + t.Errorf("expected code %q, got %q", ErrCodeForbidden, ce.Code) + } + if result.Reply != nil { + t.Error("no voice token may accompany the refusal") + } + if !result.LeaveVoice { + t.Error("refusal must also evict the live voice session") + } +} + // capturingTokenGen records the arguments passed to GenerateToken. type capturingTokenGen struct { token string diff --git a/Server/ws/handlers.go b/Server/ws/handlers.go index 47dfd206..8150e7e9 100644 --- a/Server/ws/handlers.go +++ b/Server/ws/handlers.go @@ -145,6 +145,12 @@ func (h *Hub) handleMessage(c *Client, raw []byte) { reqLog.Error("ws handler internal error", "err", result.Error) c.sendMsg(buildErrorMsgWithID(ErrCodeInternal, "internal error", env.ID)) } + // A rejection may still need to evict: voice_token_refresh returns + // LeaveVoice alongside its error when CONNECT_VOICE was revoked, so the + // user is removed from the SFU rather than merely denied a new token. + if result.LeaveVoice { + h.handleVoiceLeave(c.ctx, c) + } return } @@ -182,7 +188,7 @@ func (h *Hub) handleMessage(c *Client, raw []byte) { c.sendMsg(result.Reply) } if len(result.Events) > 0 { - h.EmitEvents(result.Events) + h.EmitEvents(c.ctx, result.Events) } // Voice join/leave hand off to the hub-internal routines (also called // un-throttled on disconnect/switch). handleVoiceJoin re-reads channel_id diff --git a/Server/ws/hub.go b/Server/ws/hub.go index 749ae46b..28fb1424 100644 --- a/Server/ws/hub.go +++ b/Server/ws/hub.go @@ -23,6 +23,12 @@ import ( type broadcastMsg struct { channelID int64 // 0 = send to all connected clients msg []byte + // recipients, when non-nil, replaces topic fan-out with direct delivery to + // exactly these user IDs. Used by voice_state/voice_leave: they are global + // in scope (every sidebar shows them) but must not disclose a channel the + // recipient's role may not READ, and the audience is resolved off the hub + // goroutine so deliverBroadcast stays free of permission queries. + recipients []int64 } // Hub manages all active WebSocket clients and routes messages between them. @@ -295,7 +301,10 @@ func (h *Hub) Run() { return case ev := <-h.clientEvents: if ev.add { - h.registerNow(ev.c) + // No handshake permission set on this path (and no DB + // call allowed on the hub goroutine) — nil denies the + // inherited voice-channel subscription. + h.registerNow(ev.c, nil) } else { h.unregisterNow(ev.c) } @@ -392,7 +401,7 @@ func (h *Hub) CleanupVoiceForChannel(channelID int64) { // Broadcast voice_leave for each participant. for _, vs := range states { - h.BroadcastToAll(buildVoiceLeave(channelID, vs.UserID)) + h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, vs.UserID)) } } @@ -430,7 +439,12 @@ type clientEvent struct { add bool } -func (h *Hub) registerNow(c *Client) { +// registerNow adds c to the hub and subscribes it to its topics. +// +// readableChannelIDs is the set of channels the user holds READ_MESSAGES on, +// as computed by the handshake (serve.go). It gates the inherited voice-channel +// subscription only; a nil set denies it (fail closed). +func (h *Hub) registerNow(c *Client, readableChannelIDs map[int64]bool) { h.mu.Lock() if old, exists := h.clients[c.userID]; exists { oldVoiceChID, oldVoiceJoinToken := old.clearVoiceState() @@ -468,10 +482,12 @@ func (h *Hub) registerNow(c *Client) { if chID := c.getChannelID(); chID != 0 { h.pubsub.Subscribe(c, ChannelTopic(chID)) } - // If the client is already in a voice channel (e.g. reconnect or test setup), - // subscribe to that channel's topic so voice-scoped and channel-scoped - // broadcasts reach them. - if voiceChID := c.getVoiceChID(); voiceChID != 0 { + // If the client is already in a voice channel (e.g. reconnect), re-subscribe + // to that channel's topic so the message stream keeps flowing without a new + // channel_focus. Voice membership is gated on CONNECT_VOICE alone, so it must + // not by itself grant a channel's message stream: subscribe only when the + // handshake confirmed READ_MESSAGES on that channel. + if voiceChID := c.getVoiceChID(); voiceChID != 0 && readableChannelIDs[voiceChID] { h.pubsub.Subscribe(c, ChannelTopic(voiceChID)) } } @@ -515,6 +531,70 @@ func (h *Hub) BroadcastToAll(msg []byte) { } } +// broadcastVoiceEvent enqueues a voice_state / voice_leave message for the +// connected clients whose current role may READ channelID. +// +// These events used to go out via BroadcastToAll, which handed every +// authenticated client the membership and camera/mute state of voice channels +// that channel_overrides hides from their role — while the equivalent read path +// (buildReady) deliberately filters voice states to readable channels. Tagging +// the event with its real channel id also makes reconnect replay filter it, +// where a channelID of 0 was replayed unconditionally. +// +// The audience is resolved here, on the caller's goroutine, so the hub's +// dispatch loop never blocks on permission lookups. +func (h *Hub) broadcastVoiceEvent(ctx context.Context, channelID int64, msg []byte) { + bm := broadcastMsg{ + channelID: channelID, + msg: msg, + recipients: h.voiceEventAudience(ctx, channelID), + } + select { + case h.broadcast <- bm: + default: + h.broadcastDrops.Add(1) + slog.Warn("hub: broadcast channel full, dropping voice event", + "channel_id", channelID, "msg_len", len(msg)) + } +} + +// voiceEventAudience returns the connected user IDs whose current role may READ +// channelID. Always non-nil, so an empty result means "deliver to nobody" +// rather than "no filter". Roles are resolved per client (an admin may have +// reassigned one mid-session) and the channel verdict is memoised per role, so +// the cost is one role lookup per connected client plus one override lookup per +// distinct role. Fails closed: a client whose role cannot be resolved is left +// out. Mirrors RefreshChannelVisibility, which resolves visibility the same way. +func (h *Hub) voiceEventAudience(ctx context.Context, channelID int64) []int64 { + h.mu.RLock() + userIDs := make([]int64, 0, len(h.clients)) + for uid := range h.clients { + userIDs = append(userIDs, uid) + } + h.mu.RUnlock() + + audience := make([]int64, 0, len(userIDs)) + if h.db == nil || h.permChecker == nil { + return audience + } + visibleByRole := make(map[int64]bool) + for _, uid := range userIDs { + role, err := h.db.GetRoleForUser(ctx, uid) + if err != nil || role == nil { + continue + } + visible, ok := visibleByRole[role.ID] + if !ok { + visible = h.permChecker.HasChannelPerm(ctx, role.Permissions, role.ID, channelID, permissions.ReadMessages) + visibleByRole[role.ID] = visible + } + if visible { + audience = append(audience, uid) + } + } + return audience +} + // BroadcastServerRestart sends a server_restart message to all connected clients. // reason describes why the server is restarting (e.g., "update"). // delaySeconds tells clients how long until the server actually shuts down. @@ -970,6 +1050,31 @@ func (h *Hub) sweepStaleVoiceStates() { } // Hub run-loop sweeper — no request tie. ctx := context.Background() + + // Revocation must evict a live session, not merely block the next join. + // Nothing else in ws re-validates voice permissions for a connection that + // stays open, so a user stripped of CONNECT_VOICE kept their SFU session + // until they disconnected. Checked once a minute, and only for the handful + // of clients actually in voice. + h.mu.RLock() + inVoice := make([]*Client, 0, len(h.clients)) + for _, c := range h.clients { + if c.getVoiceChID() != 0 { + inVoice = append(inVoice, c) + } + } + h.mu.RUnlock() + for _, c := range inVoice { + chID := c.getVoiceChID() + if chID == 0 || h.hasChannelPerm(ctx, c, chID, permissions.ConnectVoice) { + continue + } + slog.Warn("sweepStaleVoiceStates: evicting participant whose CONNECT_VOICE was revoked", + "user_id", c.userID, "channel_id", chID) + c.sendMsg(buildErrorMsg(ErrCodeForbidden, "missing CONNECT_VOICE permission")) + h.handleVoiceLeave(ctx, c) + } + allStates, err := h.db.GetAllVoiceStates(ctx) if err != nil { slog.Warn("sweepStaleVoiceStates: GetAllVoiceStates failed", "err", err) @@ -1012,7 +1117,7 @@ func (h *Hub) sweepStaleVoiceStates() { } slog.Warn("sweepStaleVoiceStates: removed ghost voice state", "user_id", s.userID, "channel_id", s.channelID) - h.BroadcastToAll(buildVoiceLeave(s.channelID, s.userID)) + h.broadcastVoiceEvent(ctx, s.channelID, buildVoiceLeave(s.channelID, s.userID)) if h.livekit != nil { _ = h.livekit.RemoveParticipant(ctx, s.channelID, s.userID, s.joinedAt) } @@ -1046,10 +1151,16 @@ func (h *Hub) deliverBroadcast(bm broadcastMsg) { sink.Dispatch(context.Background(), eventType, msg) } - if bm.channelID == 0 { + switch { + case bm.recipients != nil: + // Visibility-filtered fan-out: the audience was resolved by the caller. + for _, userID := range bm.recipients { + h.SendToUser(userID, msg) + } + case bm.channelID == 0: // Global broadcast — deliver to every connected client. h.pubsub.PublishGlobal(msg) - } else { + default: // Channel-scoped broadcast — deliver to subscribers of the channel topic. topic := ChannelTopic(bm.channelID) if !h.topicLimiter.Allow(topic) { diff --git a/Server/ws/hub_test.go b/Server/ws/hub_test.go index d728c101..fbdbf672 100644 --- a/Server/ws/hub_test.go +++ b/Server/ws/hub_test.go @@ -261,7 +261,11 @@ func TestHub_BroadcastToChannel_SkipsUnfocusedClient(t *testing.T) { assertNotReceived(t, s2, "unfocused client must NOT receive channel broadcast") } -func TestHub_BroadcastToChannel_DeliversToVoiceClient(t *testing.T) { +// Voice membership is gated on CONNECT_VOICE only, so it must never on its own +// subscribe a client to a channel's message stream — that route requires +// READ_MESSAGES (channel_focus). Registration without a READ_MESSAGES set must +// therefore deliver nothing. +func TestHub_BroadcastToChannel_NotDeliveredOnVoiceMembershipAlone(t *testing.T) { hub, database := newTestHub(t) go hub.Run() defer hub.Stop() @@ -280,7 +284,58 @@ func TestHub_BroadcastToChannel_DeliversToVoiceClient(t *testing.T) { hub.BroadcastToChannel(chID, msg) time.Sleep(20 * time.Millisecond) - assertReceived(t, s1, msg, "voice client should receive channel broadcast") + assertNotReceived(t, s1, "voice membership alone must NOT deliver the channel message stream") +} + +// The inherited voice-channel subscription follows the handshake's +// READ_MESSAGES set: a reconnecting client that may read the channel keeps live +// delivery (it never re-sends channel_focus), one that may not gets nothing. +func TestHub_RegisterNow_VoiceChannelSubscriptionFollowsReadPermission(t *testing.T) { + tests := []struct { + name string + slug string + readable func(chID int64) map[int64]bool + want bool + }{ + { + name: "READ_MESSAGES on the voice channel keeps the stream", + slug: "readable", + readable: func(chID int64) map[int64]bool { return map[int64]bool{chID: true} }, + want: true, + }, + { + name: "READ_MESSAGES only elsewhere denies the stream", + slug: "unreadable", + readable: func(chID int64) map[int64]bool { return map[int64]bool{chID + 1000: true} }, + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + hub, database := newTestHub(t) + go hub.Run() + defer hub.Stop() + + chID := seedTestChannel(t, database, "voice-text-"+tc.slug) + u1 := seedTestUser(t, database, "voiceuser-"+tc.slug) + + s1 := make(chan []byte, 4) + c1 := ws.NewTestClient(hub, u1, s1) // channelID == 0 (no channel_focus yet) + ws.SetClientVoiceChID(c1, chID) + hub.RegisterNowWithReadableForTest(c1, tc.readable(chID)) + + msg := []byte(`{"type":"chat_message","payload":{"content":"hello"}}`) + hub.BroadcastToChannel(chID, msg) + time.Sleep(20 * time.Millisecond) + + if tc.want { + assertReceived(t, s1, msg, "voice client with READ_MESSAGES") + } else { + assertNotReceived(t, s1, "voice client without READ_MESSAGES") + } + }) + } } func TestHub_BroadcastToAll_StillDeliversToUnfocusedClient(t *testing.T) { diff --git a/Server/ws/livekit_webhook.go b/Server/ws/livekit_webhook.go index 12baad35..c44f4633 100644 --- a/Server/ws/livekit_webhook.go +++ b/Server/ws/livekit_webhook.go @@ -202,7 +202,7 @@ func (h *Hub) handleWebhookParticipantLeft(ctx context.Context, event *livekit.W } } - h.BroadcastToAll(buildVoiceLeave(channelID, userID)) + h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, userID)) slog.Info("livekit webhook: cleaned up stale voice state", "user_id", userID, "channel_id", channelID) @@ -215,7 +215,7 @@ func (h *Hub) handleWebhookParticipantLeft(ctx context.Context, event *livekit.W slog.Error("livekit webhook: LeaveVoiceChannelIfMatch failed (stale DB row)", "error", dbErr, "user_id", userID, "channel_id", channelID) } else if deleted { - h.BroadcastToAll(buildVoiceLeave(channelID, userID)) + h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, userID)) slog.Info("livekit webhook: cleaned stale DB voice row after reconnect", "user_id", userID, "channel_id", channelID) } @@ -228,7 +228,7 @@ func (h *Hub) handleWebhookParticipantLeft(ctx context.Context, event *livekit.W slog.Error("livekit webhook: LeaveVoiceChannelIfMatch failed (client gone)", "error", dbErr, "user_id", userID, "channel_id", channelID) } else if deleted { - h.BroadcastToAll(buildVoiceLeave(channelID, userID)) + h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, userID)) } } } diff --git a/Server/ws/serve.go b/Server/ws/serve.go index 089cbfcf..c9d0ff87 100644 --- a/Server/ws/serve.go +++ b/Server/ws/serve.go @@ -173,7 +173,7 @@ func (h *Hub) handleReconnect( // the write window are queued in the client's send buffer instead of // being lost (BUG-123). writePump hasn't started yet, so queued messages // will be drained once the pumps begin. - h.registerNow(c) + h.registerNow(c, allowedChannelIDs) // Replay succeeded — send auth_ok then missed events. The replay tier // is included in the payload so the client can attribute reconnect @@ -261,7 +261,7 @@ func (h *Hub) handleFreshConnect( if _, delErr := database.LeaveVoiceChannelIfMatch(ctx, c.userID, vs.ChannelID, vs.JoinedAt); delErr != nil { slog.Warn("ws fresh connect: LeaveVoiceChannelIfMatch failed", "err", delErr) } - h.BroadcastToAll(buildVoiceLeave(vs.ChannelID, c.userID)) + h.broadcastVoiceEvent(ctx, vs.ChannelID, buildVoiceLeave(vs.ChannelID, c.userID)) if h.livekit != nil { // BUG-089: Capture stale join token so the goroutine only removes // the exact stale participant. The identity includes joinedAt, so @@ -299,7 +299,22 @@ func (h *Hub) handleFreshConnect( // the write window are queued in the client's send buffer instead of // being lost (BUG-123). writePump hasn't started yet, so queued messages // will be drained once the pumps begin. - h.registerNow(c) + // + // Only the replay-failure fallback (lastSeq > 0) can inherit voice state + // from the previous connection, so that is the only case where registerNow + // needs the read-permission set. Fail closed on error: nil denies the + // inherited voice-channel subscription. + var allowedChannelIDs map[int64]bool + if c.lastSeq > 0 { + allowed, allowedErr := h.computeAllowedChannels(ctx, database, c.user) + if allowedErr != nil { + slog.Warn("ws handleFreshConnect: computeAllowedChannels failed, skipping voice channel subscription", + "user_id", c.userID, "err", allowedErr) + } else { + allowedChannelIDs = allowed + } + } + h.registerNow(c, allowedChannelIDs) // Fresh connection or replay fallback: full auth_ok + ready flow. slog.Info("ws sending auth_ok", "user_id", c.userID, "username", c.user.Username, "role", c.roleName) diff --git a/Server/ws/voice_controls.go b/Server/ws/voice_controls.go index bc1ada87..c944b60c 100644 --- a/Server/ws/voice_controls.go +++ b/Server/ws/voice_controls.go @@ -152,5 +152,8 @@ func voiceStateBroadcast(ctx context.Context, d VoiceDeps, userID int64) Result if state == nil { return Result{} // not in voice — nothing to broadcast } - return Result{Events: []Event{VoiceStateEvent{payload: buildVoiceState(*state)}}} + return Result{Events: []Event{VoiceStateEvent{ + voiceChannelID: state.ChannelID, + payload: buildVoiceState(*state), + }}} } diff --git a/Server/ws/voice_e2ee.go b/Server/ws/voice_e2ee.go index c3ad5956..6367db55 100644 --- a/Server/ws/voice_e2ee.go +++ b/Server/ws/voice_e2ee.go @@ -15,6 +15,11 @@ import ( const ( voiceE2EERateLimit = 5 voiceE2EEWindow = time.Second + // voiceE2EEOfferRateLimit budgets a whole key rotation, which is a burst of + // one offer per peer fired on join/leave and on the periodic re-key. It is + // therefore sized well above any realistic voice channel rather than at + // voiceE2EERateLimit, which suits announces (one frame per rotation). + voiceE2EEOfferRateLimit = 64 ) // validateBase64Loose checks that s is valid padded (StdEncoding) or unpadded @@ -165,20 +170,6 @@ func handleVoiceE2EEOfferV2(_ context.Context, cmd Command, info ClientInfo, dep offerCmd := cmd.(VoiceE2EEOfferCmd) voiceChID := info.VoiceChannelID - // A legitimate rotation is a burst of one offer per peer (fired on - // join/leave and the periodic re-key), so the budget must not depend on - // channel size — keyed per sender alone, the 6th+ peer's offer was - // silently rate-limited and that peer could never decrypt audio again. - // Keying per (sender, target) admits any single rotation regardless of - // participant count while still capping repeated offers at one victim — - // the abuse this limit exists for, since an offer can force the target to - // re-key or disconnect. Cross-target spray stays bounded per victim and - // requires holding key-holder status in that channel. - ratKey := fmt.Sprintf("voice_e2ee_offer:%d:%d", info.UserID, offerCmd.TargetUserID()) - if d.Limiter != nil && !d.Limiter.Allow(ratKey, voiceE2EERateLimit, voiceE2EEWindow) { - return Result{Error: ClientError{Code: ErrCodeRateLimited, Message: "too many e2ee offers"}} - } - if voiceChID == 0 { return Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "not in a voice channel"}} } @@ -207,6 +198,29 @@ func handleVoiceE2EEOfferV2(_ context.Context, cmd Command, info ClientInfo, dep return Result{Error: ClientError{Code: ErrCodeNotKeyHolder, Message: "only the key holder may send key offers"}} } + // Rate limit last, and let no unvalidated client input allocate limiter + // state. RateLimiter.Allow inserts a live map entry per distinct key, reaped + // only ~20 minutes later, and the key used to interpolate target_user_id — + // attacker-controlled JSON — before any check ran. One authenticated socket + // that was neither in voice nor a key holder could therefore spray unbounded + // entries into the process-wide limiter until the server ran out of memory. + // + // The outer budget is keyed on (sender, voice channel), server-held state + // only, and sized for a whole rotation. Passing it is now the precondition + // for creating any per-target entry, so limiter growth is bounded by real, + // permission-checked voice joins rather than by attacker-chosen integers. + ratKey := fmt.Sprintf("voice_e2ee_offer:%d:%d", info.UserID, voiceChID) + if d.Limiter != nil && !d.Limiter.Allow(ratKey, voiceE2EEOfferRateLimit, voiceE2EEWindow) { + return Result{Error: ClientError{Code: ErrCodeRateLimited, Message: "too many e2ee offers"}} + } + // Inner budget keeps the W1-2 per-victim cap: an offer can force the target + // to re-key or disconnect, so repeated offers at one peer stay capped even + // though a full rotation across many peers passes. + targetKey := fmt.Sprintf("voice_e2ee_offer:%d:%d:%d", info.UserID, voiceChID, targetUserID) + if d.Limiter != nil && !d.Limiter.Allow(targetKey, voiceE2EERateLimit, voiceE2EEWindow) { + return Result{Error: ClientError{Code: ErrCodeRateLimited, Message: "too many e2ee offers"}} + } + msg := buildVoiceE2EEOffer(info.UserID, encKey, iv) return Result{ Events: []Event{VoiceE2EEOfferGuardedEvent{ diff --git a/Server/ws/voice_join.go b/Server/ws/voice_join.go index f5a147b4..2da58f7c 100644 --- a/Server/ws/voice_join.go +++ b/Server/ws/voice_join.go @@ -198,8 +198,8 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe // Update key holder map now that this client's voice state is set. h.updateKeyHolder(channelID) - // Broadcast the joiner's state to all connected clients. - h.BroadcastToAll(buildVoiceState(*state)) + // Broadcast the joiner's state to the clients allowed to see this channel. + h.broadcastVoiceEvent(ctx, channelID, buildVoiceState(*state)) // Send existing channel voice states to the joiner. existing, err := h.db.GetChannelVoiceStates(ctx, channelID) @@ -270,6 +270,20 @@ func handleVoiceTokenRefreshV2(ctx context.Context, cmd Command, info ClientInfo return Result{Error: ClientError{Code: ErrCodeInternal, Message: "voice not configured"}} } + // Re-check CONNECT_VOICE where the credential is minted. The channel comes + // from the client's own session state, and voice_join (voice_join.go:61) was + // the only place this bit was ever checked — so a user whose CONNECT_VOICE + // was revoked mid-session kept minting fresh SFU room-join grants. Refusing + // alone would leave the live session in place, so the refusal also evicts: + // LeaveVoice runs handleVoiceLeave, which clears the client's voice state, + // deletes the voice_states row and removes the LiveKit participant. + if !hasPerm(ctx, d.DB, d.Permissions, userID, channelID, permissions.ConnectVoice) { + return Result{ + Error: ClientError{Code: ErrCodeForbidden, Message: "missing CONNECT_VOICE permission"}, + LeaveVoice: true, + } + } + canPublish := hasPerm(ctx, d.DB, d.Permissions, userID, channelID, permissions.SpeakVoice) canSubscribe := true canVideo := hasPerm(ctx, d.DB, d.Permissions, userID, channelID, permissions.UseVideo) @@ -315,6 +329,6 @@ func (h *Hub) rollbackVoiceJoin(ctx context.Context, c *Client, channelID int64, "user_id", c.userID, "channel_id", channelID) } if broadcast { - h.BroadcastToAll(buildVoiceLeave(channelID, c.userID)) + h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, c.userID)) } } diff --git a/Server/ws/voice_leave.go b/Server/ws/voice_leave.go index 481b0a76..901cfc68 100644 --- a/Server/ws/voice_leave.go +++ b/Server/ws/voice_leave.go @@ -35,7 +35,7 @@ func (h *Hub) handleVoiceLeave(ctx context.Context, c *Client) { c.sendMsg(buildErrorMsg(ErrCodeInternal, "voice leave failed — please rejoin if issues persist")) } - h.BroadcastToAll(buildVoiceLeave(oldChID, c.userID)) + h.broadcastVoiceEvent(ctx, oldChID, buildVoiceLeave(oldChID, c.userID)) // Re-elect key holder now that this user has left the channel. h.updateKeyHolder(oldChID) diff --git a/Server/ws/ws_integration_test.go b/Server/ws/ws_integration_test.go index 456c6761..47e13e5a 100644 --- a/Server/ws/ws_integration_test.go +++ b/Server/ws/ws_integration_test.go @@ -671,6 +671,144 @@ func TestServeWS_Reconnect_PreservesVoiceState(t *testing.T) { } } +// TestServeWS_Reconnect_AuthorizedVoiceClientKeepsChannelStream verifies the +// authorized half of the voice-subscription gate end to end: the reconnect +// handshake passes the user's READ_MESSAGES set to registerNow, so a user who +// may read the channel they are in voice on keeps live message delivery without +// re-sending channel_focus (the desktop client does not re-send it on auth_ok). +func TestServeWS_Reconnect_AuthorizedVoiceClientKeepsChannelStream(t *testing.T) { + database := openServeTestDB(t) + limiter := auth.NewRateLimiter() + hub := ws.NewHub(database, limiter, nil) + go hub.Run() + defer hub.Stop() + + // roleID 1 = Owner: holds READ_MESSAGES on every channel. + userID, err := database.CreateUser(context.Background(), "ws-voice-read-allowed", "hash", 1) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + token, err := auth.GenerateToken() + if err != nil { + t.Fatalf("GenerateToken: %v", err) + } + tokenHash := auth.HashToken(token) + if _, err := database.CreateSession(context.Background(), userID, tokenHash, "test", "127.0.0.1"); err != nil { + t.Fatalf("CreateSession: %v", err) + } + + chID, err := database.CreateChannel(context.Background(), "voice-read-allowed", "text", "", "", 0) + if err != nil { + t.Fatalf("CreateChannel: %v", err) + } + + handler := ws.ServeWS(hub, database, []string{"*"}) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + dialAndAuth := func(lastSeq uint64) *websocket.Conn { + t.Helper() + conn, dialResp, dialErr := websocket.Dial(ctx, wsURL, nil) + if dialResp != nil && dialResp.Body != nil { + dialResp.Body.Close() + } + if dialErr != nil { + t.Fatalf("websocket.Dial: %v", dialErr) + } + authMsg := map[string]any{ + "type": "auth", + "payload": map[string]any{"token": token, "last_seq": lastSeq}, + } + raw, marshalErr := json.Marshal(authMsg) + if marshalErr != nil { + t.Fatalf("marshal auth: %v", marshalErr) + } + if writeErr := conn.Write(ctx, websocket.MessageText, raw); writeErr != nil { + t.Fatalf("write auth: %v", writeErr) + } + // Read auth_ok + first following message. + for i := 0; i < 2; i++ { + if _, _, readErr := conn.Read(ctx); readErr != nil { + t.Fatalf("read handshake message %d: %v", i, readErr) + } + } + return conn + } + + conn1 := dialAndAuth(0) + defer func() { _ = conn1.Close(websocket.StatusNormalClosure, "") }() + + var originalClient *ws.Client + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + originalClient = hub.GetClient(userID) + if originalClient != nil { + break + } + time.Sleep(20 * time.Millisecond) + } + if originalClient == nil { + t.Fatal("expected first client to be registered") + } + + // Join voice on the channel AFTER conn1 is established (see the + // PreservesVoiceState test: setting it earlier is cleaned up on connect). + if err := database.JoinVoiceChannel(context.Background(), userID, chID); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + vs, err := database.GetVoiceState(context.Background(), userID) + if err != nil || vs == nil { + t.Fatalf("GetVoiceState: %v", err) + } + ws.SetClientVoiceStateForTest(originalClient, chID, vs.JoinedAt) + + // Reconnect (lastSeq > 0) — voice state transfers to the replacement client, + // which has no focused channel of its own. + conn2 := dialAndAuth(2) + defer func() { _ = conn2.Close(websocket.StatusNormalClosure, "") }() + + var replacementClient *ws.Client + deadline = time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + replacementClient = hub.GetClient(userID) + if replacementClient != nil && ws.GetClientVoiceChIDForTest(replacementClient) == chID { + break + } + time.Sleep(20 * time.Millisecond) + } + if replacementClient == nil || ws.GetClientVoiceChIDForTest(replacementClient) != chID { + t.Fatal("expected replacement client with transferred voice state") + } + // The topic subscription lands just after the client enters the hub map. + time.Sleep(100 * time.Millisecond) + + hub.BroadcastToChannel(chID, []byte(`{"type":"chat_message","payload":{"content":"still-visible"}}`)) + + readDeadline := time.Now().Add(3 * time.Second) + for { + if time.Now().After(readDeadline) { + t.Fatal("authorized voice client stopped receiving the channel message stream after reconnect") + } + readCtx, readCancel := context.WithTimeout(ctx, 500*time.Millisecond) + _, raw, readErr := conn2.Read(readCtx) + readCancel() + if readErr != nil { + continue + } + var msg map[string]any + if err := json.Unmarshal(raw, &msg); err != nil { + continue + } + if msg["type"] == "chat_message" { + return + } + } +} + // TestServeWS_FreshReconnect_CleansStaleVoiceState verifies that when a user // presses F5 (fresh connection, lastSeq = 0) while in voice, the server: // 1. cleans the DB voice_state row before building ready