mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
fix: 40 correctness fixes from the 2026-08-19 bug hunt (#1392)
* fix(identity): 1 defect(s) (OC-0151)
* fix(ws): 1 defect(s) (OC-0152)
* fix(admin): 1 defect(s) (OC-0153)
* fix(admin): 1 defect(s) (OC-0154)
* fix(voice): 2 defect(s) (OC-0155, OC-0167)
Replace distributeRoomKey's per-call offer counter with an instance-level
sliding-window budget shared by every voice_e2ee_offer send path.
- OC-0155: back-to-back rotations (the second run immediately by
drainPendingRotationOrArmTimer) each got a fresh pacing budget, so their
combined sends could exceed the server's single per-second cap.
- OC-0167: handleAnnounceInner's drain-time offer send bypassed pacing
entirely, letting a key holder joining a large ongoing call burst every
queued announce's offer unpaced.
The shared budget is reset in clearState() since the server's limit is
scoped per (sender, channel).
* fix(client): 1 defect(s) (OC-0156)
createPresenceSender dropped a queued custom_status when a later plain
status change superseded the pending retry. The retry now carries the
last committed custom_status forward.
* fix(client): 2 defect(s) (OC-0160, OC-0163)
OC-0160: exempt the handshake frames (ready, auth_ok) from the ws message
size limit and run the guard after parsing. A 'ready' frame grows unbounded
with member/channel/DM counts and carries no seq, so dropping it left the
client on empty stores with no error and no recovery path.
OC-0163: bracket a bare IPv6 host when building the wss:// URL so the
authority parses, and collapse bracketed/bare IPv6 literals to the same
cert_store_key so one server is not pinned (and user-confirmed) twice.
* fix(voice): 1 defect(s) (OC-0162)
updatePttKey armed the Rust poller when a PTT key was bound mid-call but
never applied the gate. The poller only emits 'ptt-state' on a press/release
transition, so an idle key produced no event and the already-published mic
stayed hot until the user's first physical press+release. Mirror the join-time
gate computation in updatePttKey, guarded on being in a call, polling actually
being live, and the mic not already being gated.
* fix(client): 1 defect(s) (OC-0164)
* fix(plugin): 1 defect(s) (OC-0165)
scanPluginDirectory now skips a malformed plugin subdirectory and joins its
error instead of aborting the whole scan, and LoadAll logs-and-continues so
one bad plugin directory cannot disable every other plugin.
* fix(ws): 1 defect(s) (OC-0166)
Route PresenceSelfEvent onto the owner's normal-priority queue instead of
letting it fall through to the UserTargetedEvent high-priority case, so a
user's own presence frames all share one FIFO and cannot be delivered out
of order relative to the visible presence_update path.
* fix(db): 1 defect(s) (OC-0168)
* fix(client): 1 defect(s) (OC-0169)
* fix(client): 1 defect(s) (OC-0171)
addMessage appended a broadcast at the tail even when trailing optimistic
rows were still unreconciled, so a message that committed while our own
send was in flight ended up ordered behind the row confirmSend later
stamped with a higher server id/timestamp. Insert before the trailing
unreconciled run instead.
* fix(voice): 1 defect(s) (OC-0172)
* fix(client): 1 defect(s) (OC-0174)
* fix(ws): 1 defect(s) (OC-0175)
* fix(client): 1 defect(s) (OC-0177)
* fix(client): 1 defect(s) (OC-0178)
* fix(voice): 1 defect(s) (OC-0179)
Undeafening no longer sends a voice_mute{muted:false} the server will
refuse while a moderator-imposed mute stands, matching the localServerMuted
guard already present in onMuteToggle.
* fix(client): 1 defect(s) (OC-0182)
* fix(plugin): 1 defect(s) (OC-0183)
* fix(client): 1 defect(s) (OC-0184)
Treat a trailing underscore as an emphasis delimiter, not part of the URL,
when scanning for the end of an autolinked URL.
* fix(client): 1 defect(s) (OC-0185)
Reveal .msg-actions-bar on .message:focus-within, not only on hover, so
keyboard users can see the per-message action buttons they Tab into
instead of activating them at opacity: 0.
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
* fix(client): 1 defect(s) (OC-0186)
* fix(client): 1 defect(s) (OC-0187)
The Add Server modal validated addresses with its own narrower regex that
never gained IPv6 support when api.ts's validator did, so an IPv6 server
could be logged into but never saved as a profile. Extract the validator
into src/lib/hostValidation.ts and use it from both call sites.
* fix(client): 1 defect(s) (OC-0189)
DM sidebar rows dropped mention counts entirely and the header total
excluded muted conversations outright, so a direct mention in a muted DM
was invisible. Render a mention badge that outranks the plain unread
badge, and count a muted channel's mentionCount toward the header total.
* fix(client): 1 defect(s) (OC-0190)
* fix(client): 1 defect(s) (OC-0191)
* fix(client): 2 defect(s) (OC-0157, OC-0176)
* fix(client): 1 defect(s) (OC-0161)
confirmTotp answers 401 for a wrong enrollment code while the session is still valid; firing the global onUnauthorized sink signed the user out and deleted their stored credential. Opt that one call out via a skipUnauthorized flag on doFetch.
* fix(admin): 1 defect(s) (OC-0173)
* fix(identity): 1 defect(s) (OC-0180)
* fix(admin): archived channel PATCH skips voice eviction and fan-out (OC-0158)
handlePatchChannel commits the AdminUpdateChannel write, then re-reads the
channel to drive voice eviction and the visibility fan-out. When that
post-commit re-read failed, the handler returned early: the archive was
durable but connected clients were never told and voice members were never
evicted, leaving users talking in a channel that no longer exists for them.
Drive the post-commit work off the values already in hand rather than
abandoning it when the re-read fails.
Adds SetPatchChannelPostCommitHook so the test can land a cancellation in
that exact window deterministically instead of racing wall-clock timing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
* fix(admin): role changes commit with no client ever notified (OC-0170)
broadcastRoles derived its context from the inbound *http.Request, so the
roles_update fan-out was tied to the request lifetime. A role create,
update, or delete could commit to the database and then broadcast nothing
once that request context was done, leaving every connected client on a
stale role list until the next full resync.
Decouple the fan-out from the request context so the broadcast follows the
commit rather than the caller.
Adds BroadcastRolesForTest to reach broadcastRoles from the external test
package.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
* fix(client): username rename stomps the profile card header (OC-0188)
The account profile card's header is a resolveDisplayName() slot, but the
username-rename save path wrote the raw username straight into it. A user
with a display name set would see the header switch from their display
name to their new username after a rename, disagreeing with every other
surface that renders the same identity.
Resolve the header through the same display-name path the initial render
uses, so a rename updates the username field without touching the header.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
* fix(client): settings overlay never focuses when mounted already-open (OC-0181)
mount() synced initial state — including the show() that calls
focusDialog() — before appending root to the container. .focus() on a
still-detached subtree is a silent no-op, so a caller that mounts while
uiStore.settingsOpen is already true (ConnectPage's lazy first-open path)
got a visible overlay whose focus trap never captured focus: keyboard
users landed outside the dialog with Tab escaping to the page behind it.
Attach root before syncing initial state so focusDialog() runs against a
connected subtree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
* chore: satisfy the CI gates for this fix batch
The fix batch's own commits left three CI gates red. Nothing here changes
behaviour; every edit is a lint, type, or formatting correction to code
this batch introduced.
golangci-lint:
- OC-0153 and OC-0173 replaced the last two uses of admin's setupSanitizer,
and OC-0151 the last use of api's sanitizer, leaving both package-level
bluemonday vars unused. Remove them along with the now-unused imports,
and reword the comments that named them so they still explain why the
fixpoint sanitizer is the right one without pointing at deleted symbols.
- Modernize the new handshake-deadline test's loop to range-over-int.
tsc --noEmit:
- jsdom ships no types and @types/jsdom is not a dependency, so declare the
surface the new admin-panel test uses, following src/types/jitsi-rnnoise.d.ts.
- Narrow the last-call lookup instead of indexing under
noUncheckedIndexedAccess, with an explicit failure message.
- membersStore.setState replaces whole state, so the presence-sender mocks
must supply typingUsers.
prettier: reformat the five files this batch touched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
* chore(ledger): record the 2026-08-19 hunt and its fixes
Adds the 41 findings confirmed by the 2026-08-19 hunt and marks the 40
fixed on this branch, each with its commit, the test that pins it, and
revertProof "pass".
"pass" means an independent check, not the fixing agent's self-report:
every commit had its source diff reverted against the working tree, its
own test re-run and required to FAIL, then the source restored and the
test required to PASS. Commits whose tests live inline in Rust
#[cfg(test)] blocks were proven the same way at hunk level, splicing the
pre-fix source onto the post-fix test module.
OC-0159 is recorded as a duplicate of OC-0152: the flow-reconnect and
flow-message lenses independently found the same unbounded handshake
write and proposed the same helper over the same call sites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
* test(e2e): make the voice-roster join fixture self-consistent
The voice-widget join test emitted a voice_state for user_id 4 claiming
username "newvoiceuser", but id 4 is "member2" in MOCK_MEMBERS_MULTI_ROLE.
A real server never sends a voice_state whose username disagrees with the
member record for that id, and the same file's VOICE_STATE_EVENT already
pairs id 1 with "testuser" correctly — this one event was the outlier.
The contradiction was invisible while the roster rendered the payload's
raw username. OC-0177 makes it resolve identity through membersStore so a
nickname shows the same in voice as everywhere else, at which point the
fixture's own inconsistency surfaced as a failure.
Send id 4's real username and assert on it. The test still covers what it
did before — a genuine join by a user not previously in voice, asserted by
name and by roster count.
Verified against the app unchanged: with the old fixture the spec fails
1/5 (matching CI), with this one it passes 5/5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M6gVN2JM5wrduhkNaFCxdK
---------
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -1,8 +1,11 @@
|
||||
package admin_test
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/admin"
|
||||
@@ -67,3 +70,60 @@ func TestAdminAPI_PatchChannel_UnarchiveDoesNotCleanVoice(t *testing.T) {
|
||||
t.Fatalf("CleanupVoiceForChannel calls = %v, want none on unarchive", hub.voiceCleanupIDs)
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0158: handlePatchChannel commits AdminUpdateChannel (which can set
|
||||
// archived=1) and only afterwards re-reads the row with
|
||||
// database.GetChannel(r.Context(), id). That read is still bound to the
|
||||
// admin's own request context, so a caller cancellation arriving right after
|
||||
// the commit (tab close, network blip) makes the re-read fail and the
|
||||
// handler return early — never calling hub.CleanupVoiceForChannel nor
|
||||
// hub.RefreshChannelVisibility, even though the archive already committed.
|
||||
// Live voice participants of an archived voice channel are then stuck with a
|
||||
// voice_states row, a VoiceTopic subscription and a LiveKit session in a room
|
||||
// nothing shows any more, and no sweep recovers them.
|
||||
//
|
||||
// This reproduces the race deterministically by cancelling the request
|
||||
// context from a hook that fires synchronously right after the
|
||||
// AdminUpdateChannel commit — exactly the window the repro describes a
|
||||
// browser abort landing in — instead of relying on wall-clock timing.
|
||||
func TestAdminAPI_PatchChannel_ArchiveSurvivesContextCancelAfterCommit(t *testing.T) {
|
||||
database := openAdminTestDB(t)
|
||||
hub := &mockHub{}
|
||||
handler := admin.NewAdminAPI(database, "1.0.0", hub, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
|
||||
token := createAdminUser(t, database)
|
||||
|
||||
chID, _ := database.AdminCreateChannel(context.Background(), "archive-cancel-race", "voice", "", "", 0)
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
restore := admin.SetPatchChannelPostCommitHook(func() {
|
||||
cancel()
|
||||
})
|
||||
defer restore()
|
||||
|
||||
body, _ := json.Marshal(map[string]any{"archived": true})
|
||||
req := httptest.NewRequest(http.MethodPatch, "/channels/"+itoa(chID), bytes.NewReader(body))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
req = req.WithContext(ctx)
|
||||
w := httptest.NewRecorder()
|
||||
handler.ServeHTTP(w, req)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200 (patch must survive a caller cancellation that arrives after the archive already committed); body: %s", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
ch, err := database.GetChannel(context.Background(), chID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetChannel: %v", err)
|
||||
}
|
||||
if ch == nil || !ch.Archived {
|
||||
t.Fatalf("channel %d must be archived after a reported-successful patch: %+v", chID, ch)
|
||||
}
|
||||
|
||||
if len(hub.voiceCleanupIDs) != 1 || hub.voiceCleanupIDs[0] != chID {
|
||||
t.Errorf("CleanupVoiceForChannel calls = %v, want exactly [%d]", hub.voiceCleanupIDs, chID)
|
||||
}
|
||||
if len(hub.visibilityRefreshes) != 1 {
|
||||
t.Errorf("RefreshChannelVisibility calls = %d, want 1", len(hub.visibilityRefreshes))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,7 +1,10 @@
|
||||
package admin
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"time"
|
||||
@@ -10,6 +13,14 @@ import (
|
||||
"github.com/owncord/server/db"
|
||||
)
|
||||
|
||||
// BroadcastRolesForTest exposes broadcastRoles for external tests. It builds
|
||||
// a bare *http.Request carrying ctx, since broadcastRoles's only use of its
|
||||
// *http.Request argument is r.Context().
|
||||
func BroadcastRolesForTest(ctx context.Context, database *db.DB, hub HubBroadcaster) {
|
||||
r := httptest.NewRequest(http.MethodPost, "/roles", nil).WithContext(ctx)
|
||||
broadcastRoles(r, database, hub)
|
||||
}
|
||||
|
||||
// CaptureSetupLimiter installs h so the next NewAdminAPI call reports the
|
||||
// *auth.RateLimiter it creates for the /setup endpoint. NewAdminAPI returns
|
||||
// only an http.Handler, so this is the only way tests can reach that limiter
|
||||
@@ -37,6 +48,17 @@ func SetSetupLimiterReapTiming(interval, maxWindow time.Duration) (restore func(
|
||||
// at a temp dir. Lives here so it stays out of the production binary.
|
||||
func SetBackupBaseDir(dir string) { backupBaseDir = dir }
|
||||
|
||||
// SetPatchChannelPostCommitHook installs h to run synchronously right after
|
||||
// handlePatchChannel's AdminUpdateChannel commit, before the post-commit
|
||||
// re-read and hub fan-out — the only way to deterministically land a caller
|
||||
// cancellation in that exact window (OC-0158) instead of racing wall-clock
|
||||
// timing.
|
||||
func SetPatchChannelPostCommitHook(h func()) (restore func()) {
|
||||
prev := patchChannelPostCommitHook
|
||||
patchChannelPostCommitHook = h
|
||||
return func() { patchChannelPostCommitHook = prev }
|
||||
}
|
||||
|
||||
// StubCopyBackup swaps the restore path's file-copy hook so tests can inject
|
||||
// mid-copy failures that pass the pre-copy integrity gate. CopyBackupForTest
|
||||
// is the real implementation, for stubs that only want to fail once.
|
||||
|
||||
@@ -196,6 +196,14 @@ func nsfwAuditSuffix(before, after bool) string {
|
||||
return " (unmarked NSFW)"
|
||||
}
|
||||
|
||||
// patchChannelPostCommitHook, when non-nil, runs synchronously right after
|
||||
// handlePatchChannel's AdminUpdateChannel commit, before the post-commit
|
||||
// re-read and hub fan-out. It exists so tests can deterministically simulate
|
||||
// a caller cancellation (browser tab close, network blip) landing in that
|
||||
// exact window — the race OC-0158 is about — instead of relying on
|
||||
// wall-clock timing to hit it.
|
||||
var patchChannelPostCommitHook func()
|
||||
|
||||
func handlePatchChannel(database *db.DB, hub HubBroadcaster) http.HandlerFunc {
|
||||
return func(w http.ResponseWriter, r *http.Request) {
|
||||
existing := getAdminChannel(database, w, r)
|
||||
@@ -241,12 +249,27 @@ func handlePatchChannel(database *db.DB, hub HubBroadcaster) http.HandlerFunc {
|
||||
return
|
||||
}
|
||||
|
||||
// From here on the update has already committed. If the admin's
|
||||
// browser goes away in this window (tab close, navigation, network
|
||||
// blip), r.Context() cancels, and a GetChannel re-read that still
|
||||
// used it would fail with context.Canceled — 500ing while leaving
|
||||
// the commit (including a fresh archived=1) unbroadcast, its voice
|
||||
// eviction and visibility fan-out skipped, and every connected
|
||||
// client still showing the stale state until it reconnects
|
||||
// (OC-0158). Run the rest of the handler on an uncancellable tail,
|
||||
// matching handleDeleteChannel's delCtx (OC-0010).
|
||||
tail := context.WithoutCancel(r.Context())
|
||||
|
||||
if patchChannelPostCommitHook != nil {
|
||||
patchChannelPostCommitHook()
|
||||
}
|
||||
|
||||
actor := actorFromContext(r)
|
||||
slog.Info("channel updated", "actor_id", actor, "channel_id", id, "name", req.Name, "nsfw", req.NSFW)
|
||||
db.WriteAudit(context.WithoutCancel(r.Context()), database, actor, "channel_update", "channel", id,
|
||||
db.WriteAudit(tail, database, actor, "channel_update", "channel", id,
|
||||
fmt.Sprintf("updated #%s%s", req.Name, nsfwAuditSuffix(existing.NSFW, req.NSFW)))
|
||||
|
||||
updated, err := database.GetChannel(r.Context(), id)
|
||||
updated, err := database.GetChannel(tail, id)
|
||||
if err != nil || updated == nil {
|
||||
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to fetch updated channel")
|
||||
return
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package admin
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"log/slog"
|
||||
@@ -255,11 +256,17 @@ func invalidateUsers(permInvalidator PermissionInvalidator, userIDs []int64) {
|
||||
// broadcastRoles re-reads the role list and pushes it to every client. Re-read
|
||||
// rather than patched locally so the broadcast always reflects committed state,
|
||||
// including any concurrent change.
|
||||
//
|
||||
// Called after the mutation has already committed, so the caller's request
|
||||
// context may be canceled by the time this runs (client aborted, deadline
|
||||
// fired) -- context.WithoutCancel detaches the re-read from that, matching
|
||||
// broadcastEmojiSet in api/emoji_handler.go and broadcastDMOpen in
|
||||
// api/dm_handler.go.
|
||||
func broadcastRoles(r *http.Request, database *db.DB, hub HubBroadcaster) {
|
||||
if hub == nil || database == nil {
|
||||
return
|
||||
}
|
||||
list, err := database.ListRoles(r.Context())
|
||||
list, err := database.ListRoles(context.WithoutCancel(r.Context()))
|
||||
if err != nil {
|
||||
// The mutation already committed; clients converge on their next
|
||||
// reconnect rather than seeing a failed request.
|
||||
|
||||
@@ -405,6 +405,33 @@ func TestAdminAPI_ReorderRoles_NormalizesAndBroadcasts(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// ─── broadcast fan-out must survive request cancellation ────────────────────
|
||||
|
||||
// TestBroadcastRoles_SurvivesCanceledRequestContext pins OC-0170:
|
||||
// broadcastRoles re-reads the role list with r.Context() AFTER the mutation
|
||||
// (create/update/delete/reorder) has already committed. If the admin's
|
||||
// request is aborted (tab closed, deadline fired) in that window, the re-read
|
||||
// must not ride the same now-canceled context, or the roles_update broadcast
|
||||
// is silently skipped and every connected client keeps the stale role list.
|
||||
// This mirrors OC-0139's fix for broadcastEmojiSet in api/emoji_handler.go
|
||||
// and the analogous fix for broadcastDMOpen in api/dm_handler.go.
|
||||
func TestBroadcastRoles_SurvivesCanceledRequestContext(t *testing.T) {
|
||||
database := openAdminTestDB(t)
|
||||
hub := &mockHub{}
|
||||
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
cancel() // the request was already aborted by the time the commit lands
|
||||
|
||||
admin.BroadcastRolesForTest(ctx, database, hub)
|
||||
|
||||
if len(hub.rolesUpdates) != 1 {
|
||||
t.Fatalf("roles_update broadcasts = %d, want 1 (fan-out must survive a canceled request context)", len(hub.rolesUpdates))
|
||||
}
|
||||
if len(hub.rolesUpdates[0]) != 3 {
|
||||
t.Errorf("broadcast carried %d roles, want the seeded 3", len(hub.rolesUpdates[0]))
|
||||
}
|
||||
}
|
||||
|
||||
func TestAdminAPI_ReorderRoles_PartialListRefused(t *testing.T) {
|
||||
database := openAdminTestDB(t)
|
||||
handler, hub, _, token := newRolesHandler(t, database)
|
||||
|
||||
@@ -11,15 +11,12 @@ import (
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/microcosm-cc/bluemonday"
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/config"
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/service"
|
||||
)
|
||||
|
||||
// setupSanitizer strips all HTML from user input during setup.
|
||||
var setupSanitizer = bluemonday.StrictPolicy()
|
||||
|
||||
// ownerRoleID is the role ID assigned to the first user (Owner).
|
||||
const ownerRoleID = 1
|
||||
|
||||
@@ -172,7 +169,15 @@ func setupPrecheck(w http.ResponseWriter, r *http.Request, limiter *auth.RateLim
|
||||
return req, "", false
|
||||
}
|
||||
|
||||
req.Username = strings.TrimSpace(setupSanitizer.Sanitize(req.Username))
|
||||
// Use the fixpoint sanitizer (service.SanitizeText), not a bare
|
||||
// bluemonday.StrictPolicy().Sanitize call: bluemonday's bare Sanitize HTML-escapes
|
||||
// survivors (' -> ', & -> &, " -> "), so a name like "O'Brien"
|
||||
// would be stored as "O'Brien" — different from what handleLogin
|
||||
// looks up later (which only trims), permanently locking the Owner out
|
||||
// of their own account. Mirrors the registration path (auth_handler.go)
|
||||
// and the profile-rename path (profile_handler.go), which canonicalize
|
||||
// usernames the same way. See service.SanitizeText's doc comment.
|
||||
req.Username = strings.TrimSpace(service.SanitizeText(req.Username))
|
||||
if req.Username == "" || req.Password == "" {
|
||||
writeErr(w, http.StatusBadRequest, "BAD_REQUEST", "username and password are required")
|
||||
return req, "", false
|
||||
|
||||
@@ -98,6 +98,44 @@ func TestSetup_CreatesOwner(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetup_UsernameNotHTMLEscaped pins OC-0153: handleSetup must not persist
|
||||
// an HTML-escaped owner username. setupPrecheck canonicalized the username
|
||||
// with a bare bluemonday.StrictPolicy().Sanitize call, which HTML-escapes
|
||||
// survivors (' -> ', & -> &, " -> "), so a name like "O'Brien"
|
||||
// was stored as "O'Brien" — different from what the owner typed and from
|
||||
// what handleLogin looks up later (which only trims). That permanently locks
|
||||
// the Owner out of their own account. Mirrors the already-fixed sibling in
|
||||
// Server/api/auth_handler_test.go (TestRegister_UsernameNotHTMLEscaped).
|
||||
func TestSetup_UsernameNotHTMLEscaped(t *testing.T) {
|
||||
database := openAdminTestDB(t)
|
||||
handler := admin.NewAdminAPI(database, "1.0.0", nil, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
|
||||
|
||||
rr := doRequest(t, handler, "POST", "/setup", "", map[string]string{
|
||||
"username": "O'Brien",
|
||||
"password": "SecurePass123!",
|
||||
})
|
||||
if rr.Code != http.StatusCreated {
|
||||
t.Fatalf("POST /setup = %d, want 201; body=%s", rr.Code, rr.Body.String())
|
||||
}
|
||||
|
||||
var resp struct {
|
||||
Username string `json:"username"`
|
||||
}
|
||||
if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
|
||||
t.Fatalf("unmarshal: %v", err)
|
||||
}
|
||||
if resp.Username != "O'Brien" {
|
||||
t.Errorf("setup response username = %q, want %q (must not be HTML-escaped)", resp.Username, "O'Brien")
|
||||
}
|
||||
|
||||
// The username handleLogin will look up (raw, only trimmed) must match
|
||||
// what setup stored, or the owner is locked out of their own account.
|
||||
stored, err := database.GetUserByUsername(context.Background(), "O'Brien")
|
||||
if err != nil || stored == nil {
|
||||
t.Fatalf("GetUserByUsername(%q) = (%v, %v), want a match", "O'Brien", stored, err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSetup_BlockedAfterFirstUser(t *testing.T) {
|
||||
database := openAdminTestDB(t)
|
||||
handler := admin.NewAdminAPI(database, "1.0.0", nil, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
|
||||
|
||||
@@ -9,6 +9,7 @@ import (
|
||||
|
||||
"github.com/owncord/server/config"
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/service"
|
||||
)
|
||||
|
||||
// ─── SetupOptions ────────────────────────────────────────────────────────────
|
||||
@@ -99,9 +100,17 @@ func validateWizard(wr *setupWizardRequest) error {
|
||||
|
||||
// wizardValidateIdentity checks and normalises the settings-table fields the
|
||||
// server reads live: the display name and the message of the day.
|
||||
//
|
||||
// It uses the fixpoint sanitizer (service.SanitizeText), not a bare
|
||||
// bluemonday.StrictPolicy().Sanitize call: bluemonday's bare Sanitize HTML-escapes
|
||||
// survivors (' -> ', & -> &, " -> "), which would store these
|
||||
// fields differently from how the admin Settings page's handlePatchSettings
|
||||
// stores the exact same keys (no sanitizer at all). See setup_handler.go's
|
||||
// identical treatment of the username field, and service.SanitizeText's doc
|
||||
// comment.
|
||||
func wizardValidateIdentity(wr *setupWizardRequest) error {
|
||||
if wr.ServerName != nil {
|
||||
name := strings.TrimSpace(setupSanitizer.Sanitize(*wr.ServerName))
|
||||
name := strings.TrimSpace(service.SanitizeText(*wr.ServerName))
|
||||
if name == "" {
|
||||
return fmt.Errorf("server_name cannot be empty")
|
||||
}
|
||||
@@ -111,7 +120,7 @@ func wizardValidateIdentity(wr *setupWizardRequest) error {
|
||||
*wr.ServerName = name
|
||||
}
|
||||
if wr.Motd != nil {
|
||||
motd := strings.TrimSpace(setupSanitizer.Sanitize(*wr.Motd))
|
||||
motd := strings.TrimSpace(service.SanitizeText(*wr.Motd))
|
||||
if len(motd) > maxMotdLen {
|
||||
return fmt.Errorf("motd must be at most %d characters", maxMotdLen)
|
||||
}
|
||||
|
||||
@@ -215,6 +215,41 @@ func TestSetupWizard_NoRestartWhenValuesMatchRunning(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetupWizard_IdentityFieldsStoredRawNotEscaped pins OC-0173: the wizard
|
||||
// must store server_name/motd the same way handlePatchSettings does later
|
||||
// (raw survivors, not HTML-entity-escaped), so a name set at first run and
|
||||
// the identical name set afterwards through the admin Settings page produce
|
||||
// the same stored value. Before the fix, wizardValidateIdentity ran these
|
||||
// fields through the bare bluemonday sanitizer, which HTML-escapes
|
||||
// survivors (' -> ', " -> ", & -> &) — see service.SanitizeText's
|
||||
// doc comment, which the setup_handler.go username path already follows for
|
||||
// exactly this reason.
|
||||
func TestSetupWizard_IdentityFieldsStoredRawNotEscaped(t *testing.T) {
|
||||
database := openAdminTestDB(t)
|
||||
cfgPath := filepath.Join(t.TempDir(), "config.yaml")
|
||||
restarted := make(chan string, 1)
|
||||
handler := wizardHandler(t, database, cfgPath, restarted)
|
||||
|
||||
rr := doRequest(t, handler, "POST", "/setup", "", map[string]any{
|
||||
"username": "owner",
|
||||
"password": "SecurePass123!",
|
||||
"wizard": map[string]any{
|
||||
"server_name": "Bob's Place",
|
||||
"motd": `Say "hi" & relax`,
|
||||
},
|
||||
})
|
||||
if rr.Code != http.StatusCreated {
|
||||
t.Fatalf("POST /setup = %d, want 201; body=%s", rr.Code, rr.Body.String())
|
||||
}
|
||||
|
||||
if got, want := getSetting(t, database, "server_name"), "Bob's Place"; got != want {
|
||||
t.Errorf("server_name = %q, want %q (stored HTML-escaped instead of raw)", got, want)
|
||||
}
|
||||
if got, want := getSetting(t, database, "motd"), `Say "hi" & relax`; got != want {
|
||||
t.Errorf("motd = %q, want %q (stored HTML-escaped instead of raw)", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSetupWizard_InvalidValuesRejectBeforeAccountCreation(t *testing.T) {
|
||||
cases := map[string]map[string]any{
|
||||
"port too low": {"port": 0},
|
||||
|
||||
@@ -1158,22 +1158,35 @@ async function clearPermOverride(){
|
||||
async function saveChannelPerms(){
|
||||
const pc=state.permChannel;if(!pc)return;
|
||||
try{
|
||||
/* Quick toggles first: same masks this panel has always written. */
|
||||
/* Quick toggles first: same masks this panel has always written. Track
|
||||
which roles this loop actually wrote — the override matrix below reads
|
||||
its radios from the pre-save snapshot, so if its target is one of
|
||||
these roles that snapshot is already stale and must not be trusted. */
|
||||
const touchedRoles=new Set();
|
||||
for(const role of pc.roles){
|
||||
if((role.permissions&ADMIN_BIT)!==0)continue;
|
||||
const box=document.getElementById('permRole'+role.role_id);
|
||||
if(!box)continue;
|
||||
const wasHidden=(role.deny&0x2)!==0;
|
||||
if(!box.checked)await api('PUT','/channels/'+pc.id+'/permissions/'+role.role_id,{allow:0,deny:DENY_PRIVATE});
|
||||
else if(wasHidden)await api('DELETE','/channels/'+pc.id+'/permissions/'+role.role_id);
|
||||
if(!box.checked){await api('PUT','/channels/'+pc.id+'/permissions/'+role.role_id,{allow:0,deny:DENY_PRIVATE});touchedRoles.add(role.role_id)}
|
||||
else if(wasHidden){await api('DELETE','/channels/'+pc.id+'/permissions/'+role.role_id);touchedRoles.add(role.role_id)}
|
||||
}
|
||||
/* Then the matrix, if a target is selected. An all-inherit row is a delete:
|
||||
storing (0,0) would leave a row that resolves to nothing. */
|
||||
const path=permTargetPath();
|
||||
if(path){
|
||||
const masks=collectOverrideMasks();
|
||||
if(masks.allow===0&&masks.deny===0)await api('DELETE',path);
|
||||
else await api('PUT',path,masks);
|
||||
/* Then the matrix, if a target is selected — unless the quick-toggle loop
|
||||
above just wrote that exact role's override row. Its radios reflect
|
||||
state from before that write, so collecting them now would silently
|
||||
undo the toggle (e.g. write back an all-inherit row that DELETEs what
|
||||
was just PUT). An all-inherit row is itself a delete: storing (0,0)
|
||||
would leave a row that resolves to nothing. */
|
||||
const sel=document.getElementById('permTarget');
|
||||
const targetVal=sel?sel.value:'';
|
||||
const targetIsTouchedRole=targetVal.charAt(0)==='r'&&touchedRoles.has(parseInt(targetVal.slice(2),10));
|
||||
if(!targetIsTouchedRole){
|
||||
const path=permTargetPath();
|
||||
if(path){
|
||||
const masks=collectOverrideMasks();
|
||||
if(masks.allow===0&&masks.deny===0)await api('DELETE',path);
|
||||
else await api('PUT',path,masks);
|
||||
}
|
||||
}
|
||||
closeModal();showToast('Channel permissions updated');renderContent();
|
||||
}catch(e){showToast(e.message,'error')}
|
||||
|
||||
@@ -12,16 +12,12 @@ import (
|
||||
"unicode/utf8"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/microcosm-cc/bluemonday"
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
"github.com/owncord/server/service"
|
||||
)
|
||||
|
||||
// sanitizer strips all HTML from user-supplied strings before storage.
|
||||
var sanitizer = bluemonday.StrictPolicy()
|
||||
|
||||
// maxLoginUsernameLen bounds the username accepted by handleLogin, mirroring
|
||||
// auth.ValidateUsername's 32-rune cap on registered usernames. Enforced
|
||||
// before the value is ever used to build a RateLimiter map key — see the
|
||||
@@ -276,8 +272,25 @@ func registerReadRequest(w http.ResponseWriter, r *http.Request) (registerReques
|
||||
return req, false
|
||||
}
|
||||
|
||||
// F: use the fixpoint sanitizer (service.SanitizeText), not the bare
|
||||
// sanitizer.Sanitize below — Sanitize's output is always HTML-escaped
|
||||
// OC-0151: bound the raw field before it ever reaches the fixpoint
|
||||
// sanitizer below. sanitizeToFixpoint's cost is quadratic in input
|
||||
// length (nested HTML entities force roughly one extra pass per two
|
||||
// nesting levels), so an unauthenticated caller could otherwise pin a
|
||||
// core for minutes with one oversized username, all before
|
||||
// auth.ValidateUsername's 32-rune cap ever runs. This is a cheap
|
||||
// byte-length pre-check — *4 still admits any legitimate 32-rune UTF-8
|
||||
// username — mirroring sanitizeContent's raw-length bound in
|
||||
// service/message.go and loginReadRequest's username bound below.
|
||||
if len(req.Username) > maxLoginUsernameLen*4 {
|
||||
writeJSON(w, http.StatusBadRequest, errorResponse{
|
||||
Error: "INVALID_INPUT",
|
||||
Message: "username is too long",
|
||||
})
|
||||
return req, false
|
||||
}
|
||||
|
||||
// F: use the fixpoint sanitizer (service.SanitizeText), not a bare
|
||||
// bluemonday.StrictPolicy().Sanitize call — Sanitize's output is always HTML-escaped
|
||||
// (' -> ', & -> &, " -> "), so a plain call here would store
|
||||
// a different string than what handleLogin looks up (which only
|
||||
// trims), permanently locking out any username containing one of
|
||||
|
||||
@@ -1215,6 +1215,47 @@ func TestLogin_OversizedUsernameRejectedBeforeRateLimiterKey(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0151: registerReadRequest ran the fixpoint sanitizer
|
||||
// (service.SanitizeText) over the raw username *before* auth.ValidateUsername
|
||||
// applies its 32-rune cap. The sanitizer loops sanitizePass to a fixpoint,
|
||||
// and nested HTML entities force roughly one extra pass per two nesting
|
||||
// levels, so the cost is quadratic in the attacker-controlled field length.
|
||||
// A 16 KB adversarial username measurably takes ~200ms to sanitize on this
|
||||
// tree (measured up to ~3.4s at 64 KB) — all of it spent before any bound on
|
||||
// the field is applied, and unauthenticated. The fix must reject an
|
||||
// oversized username on a cheap byte-length check *before* sanitizing, so
|
||||
// the rejection is near-instant regardless of payload size.
|
||||
func TestRegister_OversizedUsernameRejectedBeforeSanitizing(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
limiter := auth.NewRateLimiter()
|
||||
router := buildAuthRouter(database, limiter)
|
||||
|
||||
// Adversarial nested-entity payload (16 KB) — see service.sanitizeToFixpoint's
|
||||
// doc comment for why this shape is quadratic to sanitize.
|
||||
hugeUsername := "&" + strings.Repeat("amp;", 4000) + "lt;"
|
||||
|
||||
start := time.Now()
|
||||
rr := postJSON(t, router, "/api/v1/auth/register", map[string]string{
|
||||
"username": hugeUsername,
|
||||
"password": "securePass1",
|
||||
"invite_code": "whatever",
|
||||
})
|
||||
elapsed := time.Since(start)
|
||||
|
||||
if rr.Code != http.StatusBadRequest {
|
||||
t.Errorf("Register oversized username status = %d, want 400; body = %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
|
||||
// A guard that runs before sanitizing rejects in well under a
|
||||
// millisecond; the pre-fix code spends ~200ms in sanitizeToFixpoint on
|
||||
// this payload before it ever reaches auth.ValidateUsername's length
|
||||
// check. 150ms gives generous margin over noise while still being far
|
||||
// below the unguarded cost.
|
||||
if elapsed > 150*time.Millisecond {
|
||||
t.Errorf("Register oversized username took %v, want well under 150ms (raw field must be bounded before sanitizing, not after)", elapsed)
|
||||
}
|
||||
}
|
||||
|
||||
// ─── Rate limiting integration test ──────────────────────────────────────────
|
||||
|
||||
func TestRegister_RateLimit(t *testing.T) {
|
||||
|
||||
@@ -176,8 +176,21 @@ func handleUpdateProfile(svc *service.Services, broadcaster ProfileBroadcaster)
|
||||
return
|
||||
}
|
||||
|
||||
// Use the fixpoint sanitizer (service.SanitizeText), not the bare
|
||||
// sanitizer.Sanitize below — Sanitize's output is always
|
||||
// OC-0151: bound the raw field before it ever reaches the fixpoint
|
||||
// sanitizer below, for the same reason as the register path
|
||||
// (auth_handler.go's registerReadRequest) — sanitizeToFixpoint's
|
||||
// cost is quadratic in input length, and nothing bounds this field
|
||||
// before it runs. This is a cheap byte-length pre-check — *4 still
|
||||
// admits any legitimate 32-rune UTF-8 username.
|
||||
if len(req.Username) > maxLoginUsernameLen*4 {
|
||||
writeJSON(w, http.StatusBadRequest, errorResponse{
|
||||
Error: "INVALID_INPUT", Message: "username is too long",
|
||||
})
|
||||
return
|
||||
}
|
||||
|
||||
// Use the fixpoint sanitizer (service.SanitizeText), not a bare
|
||||
// bluemonday.StrictPolicy().Sanitize call — Sanitize's output is always
|
||||
// HTML-escaped, so a plain apostrophe would be persisted as '
|
||||
// and login (which never re-escapes) would look the account up
|
||||
// under a name that no longer matches. See service.SanitizeText's
|
||||
@@ -197,9 +210,14 @@ func handleUpdateProfile(svc *service.Services, broadcaster ProfileBroadcaster)
|
||||
return
|
||||
}
|
||||
|
||||
// Sanitize and validate avatar if provided.
|
||||
// Sanitize and validate avatar if provided. Use the fixpoint
|
||||
// sanitizer (service.SanitizeText), not a bare
|
||||
// bluemonday.StrictPolicy().Sanitize call — Sanitize's output is always HTML-escaped, so a URL with more
|
||||
// than one query parameter would have its "&" separators rewritten
|
||||
// to "&" and be persisted (and served) broken. Same reasoning as
|
||||
// the username path above.
|
||||
if req.Avatar != nil {
|
||||
trimmed := strings.TrimSpace(sanitizer.Sanitize(*req.Avatar))
|
||||
trimmed := strings.TrimSpace(service.SanitizeText(*req.Avatar))
|
||||
if err := validateAvatarURL(trimmed); err != nil {
|
||||
writeJSON(w, http.StatusBadRequest, errorResponse{
|
||||
Error: "INVALID_INPUT", Message: err.Error(),
|
||||
|
||||
@@ -164,6 +164,79 @@ func TestUpdateProfile_UsernameWithApostropheIsNotEscaped(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0151: handleUpdateProfile is the same call in the same order as
|
||||
// registerReadRequest — service.SanitizeText (the fixpoint sanitizer) runs
|
||||
// on the raw username before auth.ValidateUsername's 32-rune cap. Since
|
||||
// sanitizeToFixpoint's cost is quadratic in input length, an authenticated
|
||||
// caller can still pin a core for hundreds of milliseconds (and much longer
|
||||
// at larger sizes) with one PATCH before any bound is applied. The fix must
|
||||
// reject an oversized username on a cheap byte-length check before
|
||||
// sanitizing, so the rejection is near-instant regardless of payload size.
|
||||
func TestUpdateProfile_OversizedUsernameRejectedBeforeSanitizing(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
router := buildProfileRouter(database)
|
||||
token := profileCreateToken(t, database, "patchvictim", 4)
|
||||
|
||||
// Adversarial nested-entity payload (16 KB) — see service.sanitizeToFixpoint's
|
||||
// doc comment for why this shape is quadratic to sanitize.
|
||||
hugeUsername := "&" + strings.Repeat("amp;", 4000) + "lt;"
|
||||
|
||||
start := time.Now()
|
||||
rr := patchJSON(t, router, "/api/v1/users/me", token, map[string]string{
|
||||
"username": hugeUsername,
|
||||
})
|
||||
elapsed := time.Since(start)
|
||||
|
||||
if rr.Code != http.StatusBadRequest {
|
||||
t.Errorf("UpdateProfile oversized username status = %d, want 400; body = %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
|
||||
// A guard that runs before sanitizing rejects in well under a
|
||||
// millisecond; the pre-fix code spends ~200ms in sanitizeToFixpoint on
|
||||
// this payload before it ever reaches auth.ValidateUsername's length
|
||||
// check. 150ms gives generous margin over noise while still being far
|
||||
// below the unguarded cost.
|
||||
if elapsed > 150*time.Millisecond {
|
||||
t.Errorf("UpdateProfile oversized username took %v, want well under 150ms (raw field must be bounded before sanitizing, not after)", elapsed)
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0180: the avatar branch must canonicalize with the same fixpoint
|
||||
// sanitizer (service.SanitizeText) as the username path above it, not the
|
||||
// bare bluemonday sanitizer.Sanitize — Sanitize's output is always
|
||||
// HTML-escaped, so a legitimate avatar URL with more than one query
|
||||
// parameter gets its "&" separators rewritten to "&" and is persisted
|
||||
// (and later served to every client) as a broken URL.
|
||||
func TestUpdateProfile_AvatarQueryStringIsNotEscaped(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
router := buildProfileRouter(database)
|
||||
token := profileCreateToken(t, database, "avatarqsuser", 4)
|
||||
|
||||
const avatarURL = "https://www.gravatar.com/avatar/abc?s=256&d=identicon"
|
||||
|
||||
rr := patchJSON(t, router, "/api/v1/users/me", token, map[string]string{
|
||||
"username": "avatarqsuser",
|
||||
"avatar": avatarURL,
|
||||
})
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200; body = %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
|
||||
var resp map[string]any
|
||||
_ = json.NewDecoder(rr.Body).Decode(&resp)
|
||||
if resp["avatar"] != avatarURL {
|
||||
t.Errorf("avatar = %v, want %q (must not be HTML-escaped)", resp["avatar"], avatarURL)
|
||||
}
|
||||
|
||||
u, err := database.GetUserByUsername(context.Background(), "avatarqsuser")
|
||||
if err != nil || u == nil {
|
||||
t.Fatalf("GetUserByUsername: %v, %v", u, err)
|
||||
}
|
||||
if u.Avatar == nil || *u.Avatar != avatarURL {
|
||||
t.Errorf("stored avatar = %v, want %q (must not be HTML-escaped)", u.Avatar, avatarURL)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUpdateProfile_UsernameTaken(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
router := buildProfileRouter(database)
|
||||
|
||||
@@ -112,9 +112,17 @@ func (d *DB) PluginKVDelete(ctx context.Context, pluginID int64, key string) err
|
||||
}
|
||||
|
||||
func (d *DB) PluginKVScan(ctx context.Context, pluginID int64, prefix string, limit int) (map[string][]byte, error) {
|
||||
// A BINARY prefix comparison, not LIKE: LIKE treats '_'/'%' in prefix as
|
||||
// wildcards and is ASCII-case-insensitive by default, which disagrees
|
||||
// with the exact `key = ?` match used by PluginKVGet/Set/Delete on this
|
||||
// same table. `key >= ?` keeps the (plugin_id, key) primary-key index
|
||||
// usable for the seek; substr(key, 1, length(?)) = ? compares under the
|
||||
// column's default BINARY collation, so no wildcards and no case-folding.
|
||||
rows, err := d.reader.QueryContext(ctx,
|
||||
`SELECT key, value FROM plugin_kv WHERE plugin_id = ? AND key LIKE ? ORDER BY key LIMIT ?`,
|
||||
pluginID, prefix+"%", limit,
|
||||
`SELECT key, value FROM plugin_kv
|
||||
WHERE plugin_id = ? AND key >= ? AND substr(key, 1, length(?)) = ?
|
||||
ORDER BY key LIMIT ?`,
|
||||
pluginID, prefix, prefix, prefix, limit,
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("PluginKVScan: %w", err)
|
||||
|
||||
@@ -321,6 +321,54 @@ func TestPluginKVScan(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestPluginKVScan_IsBinaryPrefixMatch pins that PluginKVScan is an exact,
|
||||
// case-sensitive BINARY prefix match — matching the exact-match semantics of
|
||||
// PluginKVGet/Set/Delete on the same table — rather than a SQL LIKE pattern
|
||||
// match, where '_' and '%' are wildcards and matching is ASCII
|
||||
// case-insensitive.
|
||||
func TestPluginKVScan_IsBinaryPrefixMatch(t *testing.T) {
|
||||
database := newMigratedTestDB(t)
|
||||
ctx := context.Background()
|
||||
id := installTestPlugin(t, database, "hello")
|
||||
|
||||
for k, v := range map[string]string{
|
||||
"cfg_a": "underscore-match",
|
||||
"cfgXa": "not-a-prefix-match",
|
||||
"Key1": "capital-key",
|
||||
"key1": "lowercase-key",
|
||||
} {
|
||||
if err := database.PluginKVSet(ctx, id, k, []byte(v)); err != nil {
|
||||
t.Fatalf("PluginKVSet(%s): %v", k, err)
|
||||
}
|
||||
}
|
||||
|
||||
// '_' in the prefix must be a literal underscore, not a LIKE
|
||||
// single-character wildcard, so "cfgXa" must not be returned.
|
||||
underscoreScan, err := database.PluginKVScan(ctx, id, "cfg_", 100)
|
||||
if err != nil {
|
||||
t.Fatalf("PluginKVScan cfg_: %v", err)
|
||||
}
|
||||
if _, ok := underscoreScan["cfgXa"]; ok {
|
||||
t.Errorf("PluginKVScan(%q) = %v; '_' matched any character like a LIKE wildcard, but PluginKVGet treats \"cfg_a\" and \"cfgXa\" as distinct keys", "cfg_", underscoreScan)
|
||||
}
|
||||
if len(underscoreScan) != 1 || !bytes.Equal(underscoreScan["cfg_a"], []byte("underscore-match")) {
|
||||
t.Errorf("PluginKVScan(%q) = %v, want exactly {cfg_a: underscore-match}", "cfg_", underscoreScan)
|
||||
}
|
||||
|
||||
// Matching must be case-sensitive (BINARY), matching key = ? on the same
|
||||
// table, so scanning "Key" must not return "key1".
|
||||
caseScan, err := database.PluginKVScan(ctx, id, "Key", 100)
|
||||
if err != nil {
|
||||
t.Fatalf("PluginKVScan Key: %v", err)
|
||||
}
|
||||
if _, ok := caseScan["key1"]; ok {
|
||||
t.Errorf("PluginKVScan(%q) = %v; LIKE's ASCII case-insensitivity matched \"key1\", but PluginKVGet/Delete treat \"Key1\" and \"key1\" as distinct keys", "Key", caseScan)
|
||||
}
|
||||
if len(caseScan) != 1 || !bytes.Equal(caseScan["Key1"], []byte("capital-key")) {
|
||||
t.Errorf("PluginKVScan(%q) = %v, want exactly {Key1: capital-key}", "Key", caseScan)
|
||||
}
|
||||
}
|
||||
|
||||
func TestInstallPlugin_ReinstallReturnsCorrectID_AfterOtherWrites(t *testing.T) {
|
||||
database := openMigratedMemory(t)
|
||||
ctx := context.Background()
|
||||
|
||||
+24
-9
@@ -18,6 +18,7 @@ package plugin
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
@@ -30,8 +31,15 @@ type foundPlugin struct {
|
||||
}
|
||||
|
||||
// scanPluginDirectory walks dir non-recursively and parses plugin.json from
|
||||
// every immediate subdirectory. Returns on the first error encountered;
|
||||
// partial results are not returned alongside errors.
|
||||
// every immediate subdirectory. A per-plugin failure (malformed manifest,
|
||||
// missing or symlinked entrypoint, a stray symlink anywhere in that plugin's
|
||||
// tree) is recorded and that one subdirectory is skipped — it does not stop
|
||||
// the scan. The returned error is non-nil whenever at least one subdirectory
|
||||
// was skipped, joining every such failure, but `found` still holds every
|
||||
// plugin that scanned cleanly. Callers that need the scan to be all-or-
|
||||
// nothing should check the returned error before using `found`; LoadAll
|
||||
// deliberately does not, so one bad plugin directory cannot disable every
|
||||
// other plugin (OC-0165).
|
||||
func scanPluginDirectory(dir string) ([]foundPlugin, error) {
|
||||
if dir == "" {
|
||||
return nil, nil
|
||||
@@ -45,6 +53,7 @@ func scanPluginDirectory(dir string) ([]foundPlugin, error) {
|
||||
return nil, err
|
||||
}
|
||||
var found []foundPlugin
|
||||
var scanErr error
|
||||
for _, e := range entries {
|
||||
if !e.IsDir() {
|
||||
continue
|
||||
@@ -54,7 +63,8 @@ func scanPluginDirectory(dir string) ([]foundPlugin, error) {
|
||||
// Prefer plugin.toml (wazero build) over plugin.json.
|
||||
manifest, ok, tomlErr := tryLoadPluginTOML(pluginDir)
|
||||
if tomlErr != nil {
|
||||
return nil, fmt.Errorf("plugin %q: %w", e.Name(), tomlErr)
|
||||
scanErr = errors.Join(scanErr, fmt.Errorf("plugin %q: %w", e.Name(), tomlErr))
|
||||
continue
|
||||
}
|
||||
if !ok {
|
||||
// Fall back to plugin.json.
|
||||
@@ -64,12 +74,14 @@ func scanPluginDirectory(dir string) ([]foundPlugin, error) {
|
||||
if os.IsNotExist(rdErr) {
|
||||
continue
|
||||
}
|
||||
return nil, fmt.Errorf("plugin %q: read plugin.json: %w", e.Name(), rdErr)
|
||||
scanErr = errors.Join(scanErr, fmt.Errorf("plugin %q: read plugin.json: %w", e.Name(), rdErr))
|
||||
continue
|
||||
}
|
||||
var parseErr error
|
||||
manifest, parseErr = ParseManifest(raw)
|
||||
if parseErr != nil {
|
||||
return nil, fmt.Errorf("plugin %q: %w", e.Name(), parseErr)
|
||||
scanErr = errors.Join(scanErr, fmt.Errorf("plugin %q: %w", e.Name(), parseErr))
|
||||
continue
|
||||
}
|
||||
}
|
||||
// Reject any symlinks anywhere in the plugin directory tree. The asset
|
||||
@@ -80,13 +92,16 @@ func scanPluginDirectory(dir string) ([]foundPlugin, error) {
|
||||
// check below so a symlink is detected instead of followed, even
|
||||
// when its target is a valid .wasm file.
|
||||
if err := rejectSymlinksUnder(pluginDir); err != nil {
|
||||
return nil, fmt.Errorf("plugin %q: %w", e.Name(), err)
|
||||
scanErr = errors.Join(scanErr, fmt.Errorf("plugin %q: %w", e.Name(), err))
|
||||
continue
|
||||
}
|
||||
wasmPath := filepath.Join(pluginDir, manifest.Entrypoint)
|
||||
if info, statErr := os.Lstat(wasmPath); statErr != nil {
|
||||
return nil, fmt.Errorf("plugin %q: missing entrypoint %s: %w", e.Name(), manifest.Entrypoint, statErr)
|
||||
scanErr = errors.Join(scanErr, fmt.Errorf("plugin %q: missing entrypoint %s: %w", e.Name(), manifest.Entrypoint, statErr))
|
||||
continue
|
||||
} else if info.Mode()&os.ModeSymlink != 0 {
|
||||
return nil, fmt.Errorf("plugin %q: entrypoint %s is a symlink", e.Name(), manifest.Entrypoint)
|
||||
scanErr = errors.Join(scanErr, fmt.Errorf("plugin %q: entrypoint %s is a symlink", e.Name(), manifest.Entrypoint))
|
||||
continue
|
||||
}
|
||||
found = append(found, foundPlugin{
|
||||
Manifest: manifest,
|
||||
@@ -94,7 +109,7 @@ func scanPluginDirectory(dir string) ([]foundPlugin, error) {
|
||||
WASMPath: wasmPath,
|
||||
})
|
||||
}
|
||||
return found, nil
|
||||
return found, scanErr
|
||||
}
|
||||
|
||||
// rejectSymlinksUnder walks root and returns an error if any entry is a
|
||||
|
||||
@@ -65,6 +65,47 @@ func TestRejectSymlinksUnderFindsNestedSymlink(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0165: a single malformed plugin directory must not blank out every
|
||||
// other, otherwise-valid plugin in the scan. scanPluginDirectory should skip
|
||||
// the bad directory (recording its error) and still return the good one.
|
||||
func TestScanPluginDirectory_SkipsBadPluginButReturnsGood(t *testing.T) {
|
||||
root := t.TempDir()
|
||||
|
||||
// Good plugin: valid plugin.json + matching .wasm entrypoint.
|
||||
goodDir := filepath.Join(root, "hello")
|
||||
if err := os.MkdirAll(goodDir, 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
goodManifest := []byte(`{"name":"hello","version":"1.0.0","entrypoint":"hello.wasm"}`)
|
||||
if err := os.WriteFile(filepath.Join(goodDir, "plugin.json"), goodManifest, 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := os.WriteFile(filepath.Join(goodDir, "hello.wasm"), []byte("\x00asm"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// Broken plugin: malformed JSON (trailing comma).
|
||||
brokenDir := filepath.Join(root, "broken")
|
||||
if err := os.MkdirAll(brokenDir, 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
brokenManifest := []byte(`{"name":"broken","version":"1.0.0","entrypoint":"broken.wasm",}`)
|
||||
if err := os.WriteFile(filepath.Join(brokenDir, "plugin.json"), brokenManifest, 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
found, err := scanPluginDirectory(root)
|
||||
if err == nil {
|
||||
t.Fatal("expected scanPluginDirectory to report an error for the broken plugin")
|
||||
}
|
||||
if len(found) != 1 {
|
||||
t.Fatalf("scanPluginDirectory returned %d plugins, want 1 (the good one survived alongside the reported error); got %+v", len(found), found)
|
||||
}
|
||||
if found[0].Manifest.Name != "hello" {
|
||||
t.Fatalf("scanPluginDirectory returned plugin %q, want \"hello\"", found[0].Manifest.Name)
|
||||
}
|
||||
}
|
||||
|
||||
func TestScanPluginDirectoryRejectsSymlinkEntrypoint(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("symlink creation requires elevated privileges on Windows")
|
||||
|
||||
@@ -168,9 +168,14 @@ func (r *Registry) LoadAll(ctx context.Context) error {
|
||||
}
|
||||
}
|
||||
}
|
||||
// scanPluginDirectory reports a non-nil error whenever at least one
|
||||
// plugin subdirectory failed to parse, but it still returns every
|
||||
// plugin that scanned cleanly in `manifests`. Log-and-continue here
|
||||
// rather than aborting: one malformed plugin directory must not take
|
||||
// every other, otherwise-valid plugin down with it (OC-0165).
|
||||
manifests, err := scanPluginDirectory(r.cfg.Directory)
|
||||
if err != nil {
|
||||
return fmt.Errorf("plugin: scan %q: %w", r.cfg.Directory, err)
|
||||
slog.Warn("plugin: some plugin directories failed to scan and were skipped", "dir", r.cfg.Directory, "err", err)
|
||||
}
|
||||
for _, found := range manifests {
|
||||
if err := r.installFromDisk(ctx, found); err != nil {
|
||||
|
||||
@@ -114,6 +114,45 @@ func TestRegistry_LoadAll_RegistersDiscoveredPlugins(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// OC-0165: one malformed plugin directory must not blank the whole registry.
|
||||
// LoadAll must still install and activate every good plugin alongside a
|
||||
// broken one, matching installFromDisk's own per-plugin-failure policy a few
|
||||
// lines below (a bad plugin there just gets `slog.Warn` + `continue`).
|
||||
func TestRegistry_LoadAll_InstallsGoodPluginsDespiteOneBadDirectory(t *testing.T) {
|
||||
r, store, dir := newRegistryWithDir(t)
|
||||
ctx := context.Background()
|
||||
writePluginDir(t, dir, "alpha", simpleManifest("alpha"))
|
||||
|
||||
// "broken" has a plugin.json that fails to parse — malformed JSON.
|
||||
brokenDir := filepath.Join(dir, "broken")
|
||||
if err := os.MkdirAll(brokenDir, 0o750); err != nil {
|
||||
t.Fatalf("mkdir %s: %v", brokenDir, err)
|
||||
}
|
||||
if err := os.WriteFile(filepath.Join(brokenDir, "plugin.json"), []byte(`{"name":"broken",}`), 0o600); err != nil {
|
||||
t.Fatalf("write plugin.json: %v", err)
|
||||
}
|
||||
|
||||
if err := r.LoadAll(ctx); err != nil {
|
||||
t.Fatalf("LoadAll: %v — a malformed plugin directory must not fail the whole load", err)
|
||||
}
|
||||
|
||||
list := r.List()
|
||||
if len(list) != 1 {
|
||||
t.Fatalf("List() has %d entries after LoadAll, want 1 (alpha installed despite broken's failure); got %+v", len(list), list)
|
||||
}
|
||||
if list[0].Manifest.Name != "alpha" {
|
||||
t.Errorf("List()[0].Manifest.Name = %q, want \"alpha\"", list[0].Manifest.Name)
|
||||
}
|
||||
|
||||
rows, err := store.ListPlugins(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("ListPlugins: %v", err)
|
||||
}
|
||||
if len(rows) != 1 {
|
||||
t.Errorf("store has %d rows, want 1 — the good plugin must still be persisted", len(rows))
|
||||
}
|
||||
}
|
||||
|
||||
func TestRegistry_LoadAll_RemovesStaleStagingDirs(t *testing.T) {
|
||||
r, _, dir := newRegistryWithDir(t)
|
||||
|
||||
|
||||
@@ -84,7 +84,18 @@ func platformInit(cfg Config) (any, func(context.Context) error, error) {
|
||||
if memMB <= 0 {
|
||||
memMB = 64 // default 64 MiB per plugin runtime
|
||||
}
|
||||
memPages := uint32(memMB) * 1024 * 1024 / wazeroPageBytes
|
||||
// Compute the byte count in 64-bit before dividing down to pages: doing
|
||||
// the multiplication in uint32 wraps at 4 GiB, so a configured
|
||||
// max_memory_mb at or above 4096 would silently truncate (or zero out)
|
||||
// the limit actually installed. wazero's own ceiling is 65536 pages
|
||||
// (4 GiB, wasm32's addressable maximum; WithMemoryLimitPages panics
|
||||
// above it), so clamp to that after computing in 64-bit.
|
||||
const wazeroMaxPages = 65536
|
||||
pages := uint64(memMB) * 1024 * 1024 / wazeroPageBytes
|
||||
if pages > wazeroMaxPages {
|
||||
pages = wazeroMaxPages
|
||||
}
|
||||
memPages := uint32(pages)
|
||||
|
||||
rt := wazero.NewRuntimeWithConfig(ctx,
|
||||
wazero.NewRuntimeConfig().
|
||||
|
||||
@@ -26,6 +26,8 @@ import (
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"github.com/tetratelabs/wazero"
|
||||
)
|
||||
|
||||
// addWASM is the bytes of a minimal (module (func (export "add") ... )).
|
||||
@@ -499,3 +501,40 @@ func TestWazeroDeactivateClosesCompiledModule(t *testing.T) {
|
||||
t.Error("deactivate leaked the CompiledModule — re-activation cycles retain every compile")
|
||||
}
|
||||
}
|
||||
|
||||
// memoryWASM is a minimal module containing only a memory section declaring
|
||||
// `(memory 1)` — a single required page, no export needed. wazero validates
|
||||
// a module's declared memory against the runtime's configured page limit at
|
||||
// CompileModule time (internal/wasm.Memory.Validate), so this is enough to
|
||||
// observe the effective page limit platformInit installed.
|
||||
var memoryWASM = []byte{
|
||||
0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00, // magic, version
|
||||
0x05, 0x03, 0x01, 0x00, 0x01, // memory section: 1 memory, min-only, min=1 page
|
||||
}
|
||||
|
||||
// TestPlatformInitPageCountDoesNotOverflowUint32 pins OC-0183:
|
||||
// `uint32(memMB) * 1024 * 1024 / wazeroPageBytes` computed the byte count in
|
||||
// uint32 before dividing, so it wraps at 4 GiB. A MaxMemoryMB of exactly 4096
|
||||
// (4 GiB) wraps the byte count to 0, so WithMemoryLimitPages(0) is installed
|
||||
// and every plugin whose WASM declares a memory section fails to compile —
|
||||
// even though 4096 MiB is a legitimate, in-range request (wazero's own
|
||||
// ceiling is 65536 pages = 4 GiB, i.e. exactly this value is allowed).
|
||||
func TestPlatformInitPageCountDoesNotOverflowUint32(t *testing.T) {
|
||||
platform, closeFn, err := platformInit(Config{MaxMemoryMB: 4096})
|
||||
if err != nil {
|
||||
t.Fatalf("platformInit: %v", err)
|
||||
}
|
||||
t.Cleanup(func() { _ = closeFn(context.Background()) })
|
||||
|
||||
rt, ok := platform.(wazero.Runtime)
|
||||
if !ok || rt == nil {
|
||||
t.Fatal("platformInit did not return a usable wazero.Runtime")
|
||||
}
|
||||
|
||||
ctx := context.Background()
|
||||
if _, err := rt.CompileModule(ctx, memoryWASM); err != nil {
|
||||
t.Fatalf("CompileModule with MaxMemoryMB=4096 should succeed (4096 MiB = 65536 pages, "+
|
||||
"wazero's own ceiling) but got: %v — the byte-count math overflowed uint32 and wrapped "+
|
||||
"the effective memory limit to (near) zero pages", err)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -59,6 +59,23 @@ func (h *Hub) EmitEvents(ctx context.Context, events []Event) {
|
||||
// Low priority: typing indicators are ephemeral.
|
||||
h.broadcastExcludeLow(e.ChannelID(), e.ExcludeUserID(), e.Payload())
|
||||
}
|
||||
case PresenceSelfEvent:
|
||||
// Normal priority, NOT the UserTargetedEvent default below (which
|
||||
// PresenceSelfEvent also satisfies — this case must stay ordered
|
||||
// before it so the type switch picks this one). Every other
|
||||
// source of this same user's own presence — the visible
|
||||
// presence_update path (PresenceEvent -> BroadcastToAll) and the
|
||||
// connect/disconnect coalescer's private half
|
||||
// (BroadcastPresence -> h.SendToUser) — already shares the
|
||||
// normal-priority queue. Routing this one through
|
||||
// h.SendToUserHigh instead split one user's own presence across
|
||||
// two per-client FIFOs with different drain order: writePump
|
||||
// always drains high strictly before normal, so a newer
|
||||
// invisible self-frame on high could be delivered before an
|
||||
// older visible-status frame still sitting on normal, leaving
|
||||
// the owner's own client on a stale status — the same hazard
|
||||
// OC-0003/OC-0214 fixed for the "others" half of presence.
|
||||
h.SendToUser(e.TargetUserID(), e.Payload())
|
||||
case UserTargetedEvent:
|
||||
// High priority: targeted events (DM opens, mentions).
|
||||
// dm_channel_open is unsequenced and targeted, so replay can never
|
||||
|
||||
@@ -0,0 +1,70 @@
|
||||
package ws
|
||||
|
||||
// emit_presence_self_priority_test.go — regression test for OC-0166: the
|
||||
// private half of an invisible user's presence (PresenceSelfEvent) satisfies
|
||||
// UserTargetedEvent, so EmitEvents routed it through h.SendToUserHigh onto the
|
||||
// HIGH-priority queue, while every other source of that same user's own
|
||||
// presence — the visible presence_update path (PresenceEvent -> BroadcastToAll)
|
||||
// and the connect/disconnect coalescer's private half
|
||||
// (BroadcastPresence -> h.SendToUser) — shares the NORMAL-priority queue.
|
||||
// writePump always drains high strictly before normal (serve_pumps.go), so a
|
||||
// newer invisible self-frame queued on high can reach the socket ahead of an
|
||||
// older visible-status frame still sitting on normal, leaving the owner's own
|
||||
// client showing a stale status. This is the same split-FIFO hazard OC-0003 /
|
||||
// OC-0214 fixed for the "others" half of presence; this pins the self half.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// TestEmitEvents_PresenceSelfEvent_UsesNormalPriorityQueue pins the fix: a
|
||||
// PresenceSelfEvent, routed through EmitEvents, must land on the owner's
|
||||
// normal-priority queue — the same FIFO every other source of that user's own
|
||||
// presence uses — never the high-priority queue.
|
||||
//
|
||||
// Before the fix, PresenceSelfEvent fell through to the UserTargetedEvent
|
||||
// case in emit.go and was sent via h.SendToUserHigh, so this test observes
|
||||
// the frame on c.sendHigh instead of c.send, and fails.
|
||||
func TestEmitEvents_PresenceSelfEvent_UsesNormalPriorityQueue(t *testing.T) {
|
||||
h := newEmitTestHub()
|
||||
|
||||
// Built directly (not via the emit_test.go helpers) so send and sendHigh
|
||||
// are DISTINCT channels — the shared-channel helpers in export_test.go
|
||||
// unify them "for test observability" and would mask exactly the
|
||||
// queue-split this test needs to detect.
|
||||
owner := &Client{
|
||||
hub: h,
|
||||
ctx: context.Background(),
|
||||
userID: 1,
|
||||
send: make(chan []byte, 8),
|
||||
sendHigh: make(chan []byte, 8),
|
||||
sendLow: make(chan []byte, 8),
|
||||
}
|
||||
h.clients[1] = owner
|
||||
|
||||
payload := []byte(`{"type":"presence","user_id":1,"status":"invisible"}`)
|
||||
h.EmitEvents(context.Background(), []Event{
|
||||
PresenceSelfEvent{targetUserID: 1, payload: payload},
|
||||
})
|
||||
|
||||
normalMsgs := drainChan(owner.send, 200*time.Millisecond)
|
||||
highMsgs := drainChan(owner.sendHigh, 50*time.Millisecond)
|
||||
|
||||
if len(normalMsgs) != 1 {
|
||||
t.Errorf("expected the private half of an invisible presence change on "+
|
||||
"the owner's normal-priority queue (same FIFO as the visible "+
|
||||
"presence_update path and the connect/disconnect coalescer's "+
|
||||
"private half), got %d normal messages, %d high messages",
|
||||
len(normalMsgs), len(highMsgs))
|
||||
}
|
||||
if len(highMsgs) != 0 {
|
||||
t.Errorf("invisible presence's private half must not go out on the "+
|
||||
"high-priority queue: writePump drains high strictly before "+
|
||||
"normal, so a newer self-frame queued there can be delivered "+
|
||||
"before an older visible-status frame still sitting on normal, "+
|
||||
"leaving the owner's own view stale; got %d high messages",
|
||||
len(highMsgs))
|
||||
}
|
||||
}
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/owncord/server/db"
|
||||
"github.com/owncord/server/permissions"
|
||||
"github.com/owncord/server/ws"
|
||||
)
|
||||
@@ -169,3 +170,42 @@ func TestApplySetChannelID_TransientLookupError_KeepsFocus(t *testing.T) {
|
||||
t.Errorf("client channelID = %d, want %d (focus must survive a transient lookup error)", got, ch)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplySetChannelID_ArchivedChannel_Unwinds pins OC-0175: the applier's
|
||||
// re-validation mirrors HandleChannelFocus's DM-participant and
|
||||
// READ_MESSAGES legs but never looks at ch.Archived, even though
|
||||
// HandleChannelFocus itself refuses an archived channel (service/channel.go,
|
||||
// OC-0070) and archiving is exactly the kind of visibility change the
|
||||
// revalidation exists to catch (OC-0024). A channel archived in the window
|
||||
// between the admission gate and the Subscribe call must not leave the
|
||||
// socket subscribed and focused forever, matching the deleted-channel case
|
||||
// above.
|
||||
func TestApplySetChannelID_ArchivedChannel_Unwinds(t *testing.T) {
|
||||
hub, database := newHandlerHub(t)
|
||||
user := seedMemberUser(t, database, "focus-archived-user")
|
||||
ch := seedTestChannel(t, database, "focus-archived-chan")
|
||||
|
||||
send := make(chan []byte, 16)
|
||||
c := ws.NewTestClientWithUser(hub, user, 0, send)
|
||||
hub.Register(c)
|
||||
waitRegistered(t, hub, c)
|
||||
|
||||
// Simulate the admin's archive having already committed before the
|
||||
// applier runs: the admission gate (HandleChannelFocus) ran and passed
|
||||
// before this, exactly as in the OC-0024 revoke-race test above.
|
||||
if err := database.AdminUpdateChannel(context.Background(), ch, db.ChannelUpdate{
|
||||
Name: "focus-archived-chan",
|
||||
Archived: true,
|
||||
}); err != nil {
|
||||
t.Fatalf("AdminUpdateChannel: %v", err)
|
||||
}
|
||||
|
||||
hub.ApplySetChannelIDForTest(c, ch)
|
||||
|
||||
if hub.SubscribedToChannelTopicForTest(c, ch) {
|
||||
t.Error("client must not stay subscribed to an archived channel's topic")
|
||||
}
|
||||
if got := ws.ClientChannelIDForTest(c); got != 0 {
|
||||
t.Errorf("client channelID = %d, want 0 after focusing a channel archived mid-race", got)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -262,7 +262,7 @@ func (h *Hub) applySetChannelID(c *Client, newChID int64) {
|
||||
if ok, dmErr := h.db.IsDMParticipant(c.ctx, c.userID, newChID); dmErr != nil || ok {
|
||||
return
|
||||
}
|
||||
} else if ch != nil && hasChannelAccess(c.ctx, h.db, h.permChecker, h.perms, c.userID, newChID, permissions.ReadMessages) {
|
||||
} else if ch != nil && !ch.Archived && hasChannelAccess(c.ctx, h.db, h.permChecker, h.perms, c.userID, newChID, permissions.ReadMessages) {
|
||||
return
|
||||
}
|
||||
h.pubsub.Unsubscribe(c, ChannelTopic(newChID))
|
||||
|
||||
@@ -0,0 +1,143 @@
|
||||
package ws
|
||||
|
||||
// oc_0172_voice_join_getchannelvoicestates_error_test.go — regression test
|
||||
// for finding OC-0172.
|
||||
//
|
||||
// voiceJoinComplete had already subscribed the joiner to the voice topic,
|
||||
// re-elected the key holder, and broadcast the joiner's own voice_state to
|
||||
// every client that can see the channel by the time it reads back the
|
||||
// channel's existing participants via GetChannelVoiceStates. That read is
|
||||
// also the ONLY place the server ever relays an existing participant's
|
||||
// stored ECDH public key (voice_e2ee_announce) to a joiner. When the read
|
||||
// failed, the old code just `return`ed: no error frame, no rollback of the
|
||||
// voice_states row it had already committed, no compensating voice_leave for
|
||||
// the voice_state it had already broadcast, and the client's in-memory
|
||||
// voiceChID stayed set even though the join never finished. The joiner was
|
||||
// left half-joined and silent, guaranteed to fail the E2EE key exchange with
|
||||
// whoever was already in the channel and time out ~15s later with no
|
||||
// explanation.
|
||||
//
|
||||
// This reuses voiceJoinPostTokenRaceHook (already test-only plumbing for
|
||||
// OC-0008) to fault-inject exactly the failure this finding describes: it
|
||||
// fires after the token round trip completes and before voiceJoinComplete's
|
||||
// GetChannelVoiceStates call, so everything up to and including the joiner's
|
||||
// own voice_state broadcast has already happened by the time the DB read
|
||||
// that this finding is about fails.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/config"
|
||||
"github.com/owncord/server/db"
|
||||
)
|
||||
|
||||
// TestVoiceJoin_GetChannelVoiceStatesError_RollsBackAndNotifiesClient pins
|
||||
// OC-0172: a GetChannelVoiceStates failure inside voiceJoinComplete must not
|
||||
// leave the joiner silently half-joined. It must roll back the DB row it
|
||||
// already committed and tell the client the join failed, the same way every
|
||||
// other post-commit failure in this handler already does (BUG-088's
|
||||
// rollbackVoiceJoin, OC-0008's token-supersession guard).
|
||||
func TestVoiceJoin_GetChannelVoiceStatesError_RollsBackAndNotifiesClient(t *testing.T) {
|
||||
database := newHarvestVoiceDB(t)
|
||||
uid := seedHarvestVoiceUser(t, database, "join-0172-victim")
|
||||
chID := mustCreateVoiceChannel(t, database, "voice-join-0172")
|
||||
|
||||
lk, err := NewLiveKitClient(&config.VoiceConfig{
|
||||
LiveKitAPIKey: "test-api-key-0172",
|
||||
LiveKitAPISecret: "test-api-secret-0172-xyz",
|
||||
LiveKitURL: "ws://127.0.0.1:1", // never dialed: GenerateToken is local
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("NewLiveKitClient: %v", err)
|
||||
}
|
||||
|
||||
h := NewHub(database, auth.NewRateLimiter(), nil)
|
||||
h.SetLiveKit(lk)
|
||||
|
||||
send := make(chan []byte, 8)
|
||||
c := NewTestClient(h, uid, send)
|
||||
c.user = &db.User{ID: uid, Username: "join-0172-victim"}
|
||||
h.mu.Lock()
|
||||
h.clients[uid] = c
|
||||
h.mu.Unlock()
|
||||
|
||||
// Fault-inject the GetChannelVoiceStates call inside voiceJoinComplete.
|
||||
// This hook fires after GenerateToken succeeds and before the token is
|
||||
// handed to the client — i.e. strictly before voiceJoinComplete runs, so
|
||||
// by the time GetChannelVoiceStates executes, the `users` table it joins
|
||||
// against is gone and it returns an error. Nothing between the hook and
|
||||
// that call touches the DB (subscribe, updateKeyHolder, and the joiner's
|
||||
// own voice_state broadcast are all in-memory), so this does not perturb
|
||||
// any earlier step.
|
||||
//
|
||||
// Renaming (not dropping) `users` is deliberate: with foreign keys
|
||||
// enabled, SQLite's DROP TABLE performs an implicit cascading DELETE
|
||||
// through any FK referencing the dropped table before removing it (see
|
||||
// https://www.sqlite.org/lang_droptable.html), which would delete the
|
||||
// joiner's own voice_states row as a side effect of the fault injection
|
||||
// itself — masking whether the handler's own rollback logic is what
|
||||
// cleaned it up. A rename breaks the same JOIN without touching any row.
|
||||
var hookRan bool
|
||||
voiceJoinPostTokenRaceHook = func(client *Client) {
|
||||
hookRan = true
|
||||
if _, err := database.ExecContext(context.Background(), `ALTER TABLE users RENAME TO users_bak_0172`); err != nil {
|
||||
t.Fatalf("hook: rename users: %v", err)
|
||||
}
|
||||
}
|
||||
defer func() { voiceJoinPostTokenRaceHook = nil }()
|
||||
|
||||
payload, _ := json.Marshal(map[string]any{"channel_id": chID})
|
||||
h.handleVoiceJoin(context.Background(), c, json.RawMessage(payload))
|
||||
|
||||
if !hookRan {
|
||||
t.Fatal("voiceJoinPostTokenRaceHook never fired — test setup is broken, not exercising the join path")
|
||||
}
|
||||
|
||||
msgs := drainChan(send, 200*time.Millisecond)
|
||||
|
||||
var gotError bool
|
||||
for _, m := range msgs {
|
||||
var env struct {
|
||||
Type string `json:"type"`
|
||||
Payload struct {
|
||||
Code string `json:"code"`
|
||||
Message string `json:"message"`
|
||||
} `json:"payload"`
|
||||
}
|
||||
if err := json.Unmarshal(m, &env); err != nil {
|
||||
continue
|
||||
}
|
||||
if env.Type == MsgTypeError {
|
||||
gotError = true
|
||||
if env.Payload.Code != ErrCodeInternal {
|
||||
t.Errorf("error frame code = %q, want %q", env.Payload.Code, ErrCodeInternal)
|
||||
}
|
||||
}
|
||||
}
|
||||
if !gotError {
|
||||
t.Error("client received no error frame after GetChannelVoiceStates failed mid-join — the join silently half-completed with no explanation")
|
||||
}
|
||||
|
||||
// The client's in-memory voice state must be cleared, not left pointing
|
||||
// at a join the server gave up on partway through.
|
||||
if gotCh := c.getVoiceChID(); gotCh != 0 {
|
||||
t.Errorf("client voiceChID = %d after GetChannelVoiceStates failed mid-join, want 0 (rolled back)", gotCh)
|
||||
}
|
||||
|
||||
// The voice_states row committed earlier in the handler must not survive
|
||||
// a join that never finished. Query without the `users` JOIN so the
|
||||
// dropped table (a fault-injection artifact, not part of the finding)
|
||||
// does not itself break this check.
|
||||
var count int
|
||||
if err := database.QueryRowContext(context.Background(),
|
||||
`SELECT COUNT(*) FROM voice_states WHERE user_id = ?`, uid).Scan(&count); err != nil {
|
||||
t.Fatalf("count voice_states: %v", err)
|
||||
}
|
||||
if count != 0 {
|
||||
t.Errorf("voice_states row for user %d still present after a join that failed mid-completion, want it rolled back", uid)
|
||||
}
|
||||
}
|
||||
+23
-6
@@ -32,6 +32,24 @@ const (
|
||||
maxColdReplay = 5000
|
||||
)
|
||||
|
||||
// handshakeWrite writes one handshake-phase message (auth_ok, ready, or a
|
||||
// replay event) under writeTimeout, instead of the bare ctx every caller here
|
||||
// otherwise has on hand.
|
||||
//
|
||||
// Every handshake write runs against ctx = r.Context() from ServeWS.
|
||||
// websocket.Accept hijacks the connection, which stops net/http's own
|
||||
// mechanism for cancelling that context on client disconnect, so without this
|
||||
// wrapper ctx is never cancelled while the handler is blocked inside
|
||||
// conn.Write — a peer that stops reading (or whose receive window closes)
|
||||
// pins the write, the handler goroutine, and the socket forever (OC-0152).
|
||||
// writePumpWrite (serve_pumps.go) already bounds its writes the same way;
|
||||
// this brings the handshake writes in serve.go up to the same guarantee.
|
||||
func handshakeWrite(ctx context.Context, conn *websocket.Conn, msg []byte) error {
|
||||
wCtx, cancel := context.WithTimeout(ctx, writeTimeout)
|
||||
defer cancel()
|
||||
return conn.Write(wCtx, websocket.MessageText, msg)
|
||||
}
|
||||
|
||||
// ServeWS upgrades an HTTP connection to WebSocket, performs in-band auth,
|
||||
// then drives the client's read/write loops.
|
||||
// Do not wrap with AuthMiddleware — WS does its own auth.
|
||||
@@ -510,14 +528,14 @@ func (h *Hub) reconnectWriteReplay(
|
||||
// is included in the payload so the client can attribute reconnect
|
||||
// behaviour without separate metric scraping.
|
||||
slog.Info("ws sending auth_ok (reconnect)", "user_id", c.userID, "username", c.user.Username, "role", c.roleName, "replay_source", replaySource)
|
||||
if err := conn.Write(ctx, websocket.MessageText, h.buildAuthOK(ctx, c.user, c.roleName, replaySource)); err != nil {
|
||||
if err := handshakeWrite(ctx, conn, h.buildAuthOK(ctx, c.user, c.roleName, replaySource)); err != nil {
|
||||
slog.Warn("ws: failed to send auth_ok (reconnect)", "user_id", c.userID, "err", err)
|
||||
h.unregisterFailedHandshake(ctx, c)
|
||||
_ = conn.Close(websocket.StatusInternalError, "handshake failed")
|
||||
return false
|
||||
}
|
||||
for _, evt := range events {
|
||||
if err := conn.Write(ctx, websocket.MessageText, evt); err != nil {
|
||||
if err := handshakeWrite(ctx, conn, evt); err != nil {
|
||||
slog.Warn("ws: failed to send replay event", "user_id", c.userID, "err", err)
|
||||
h.unregisterFailedHandshake(ctx, c)
|
||||
_ = conn.Close(websocket.StatusInternalError, "handshake failed")
|
||||
@@ -745,7 +763,7 @@ func (h *Hub) handleFreshConnect(
|
||||
|
||||
// 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)
|
||||
if err := conn.Write(ctx, websocket.MessageText, h.buildAuthOK(ctx, c.user, c.roleName, "none")); err != nil {
|
||||
if err := handshakeWrite(ctx, conn, h.buildAuthOK(ctx, c.user, c.roleName, "none")); err != nil {
|
||||
slog.Warn("ws: failed to send auth_ok", "user_id", c.userID, "err", err)
|
||||
h.unregisterFailedHandshake(ctx, c)
|
||||
_ = conn.Close(websocket.StatusInternalError, "handshake failed")
|
||||
@@ -753,7 +771,7 @@ func (h *Hub) handleFreshConnect(
|
||||
}
|
||||
if ready, readyErr := h.buildReady(ctx, database, c.userID, userRole); readyErr == nil {
|
||||
slog.Info("ws sending ready payload", "user_id", c.userID, "payload_bytes", len(ready))
|
||||
if err := conn.Write(ctx, websocket.MessageText, ready); err != nil {
|
||||
if err := handshakeWrite(ctx, conn, ready); err != nil {
|
||||
slog.Warn("ws: failed to send ready payload", "user_id", c.userID, "err", err)
|
||||
h.unregisterFailedHandshake(ctx, c)
|
||||
_ = conn.Close(websocket.StatusInternalError, "handshake failed")
|
||||
@@ -761,8 +779,7 @@ func (h *Hub) handleFreshConnect(
|
||||
}
|
||||
} else {
|
||||
slog.Error("buildReady failed", "user_id", c.userID, "err", readyErr)
|
||||
_ = conn.Write(ctx, websocket.MessageText,
|
||||
buildErrorMsg(ErrCodeInternal, "failed to build ready payload"))
|
||||
_ = handshakeWrite(ctx, conn, buildErrorMsg(ErrCodeInternal, "failed to build ready payload"))
|
||||
h.unregisterFailedHandshake(ctx, c)
|
||||
_ = conn.Close(websocket.StatusInternalError, "failed to build ready payload")
|
||||
return readyErr
|
||||
|
||||
@@ -0,0 +1,200 @@
|
||||
package ws_test
|
||||
|
||||
// serve_handshake_write_deadline_test.go pins OC-0152: every handshake write
|
||||
// in serve.go (auth_ok / ready / reconnect replay) is issued with the bare
|
||||
// r.Context() instead of a bounded write context. websocket.Accept hijacks the
|
||||
// connection, which stops net/http's own read loop from ever cancelling that
|
||||
// context, so coder/websocket's Conn.Write blocks on the underlying socket
|
||||
// write forever once a stalled peer's receive window and the server's send
|
||||
// buffer fill — pinning the handler goroutine, the client's slot in the hub,
|
||||
// and the file descriptor for good.
|
||||
//
|
||||
// The test shrinks both sides' TCP socket buffers to the kernel minimum (so
|
||||
// a bounded amount of unread traffic is enough to make the write block, no
|
||||
// megabyte-scale burst required) and seeds enough members that the ready
|
||||
// payload alone comfortably exceeds that minimum. It then dials, completes
|
||||
// auth, and never reads another byte. Registration happens before the
|
||||
// handshake writes (serve.go), so hub.ClientCount() drops back to 0 only once
|
||||
// the blocked write returns — with a deadline, that happens once writeTimeout
|
||||
// elapses; without one, it never happens and the poll loop below times out.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/coder/websocket"
|
||||
|
||||
"github.com/owncord/server/auth"
|
||||
"github.com/owncord/server/ws"
|
||||
)
|
||||
|
||||
// tinySendBufListener wraps a net.Listener and shrinks SO_SNDBUF on every
|
||||
// accepted connection to the kernel minimum, so the server's handshake writes
|
||||
// cannot buffer their way past a peer that stops reading.
|
||||
type tinySendBufListener struct {
|
||||
net.Listener
|
||||
}
|
||||
|
||||
func (l *tinySendBufListener) Accept() (net.Conn, error) {
|
||||
c, err := l.Listener.Accept()
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if tc, ok := c.(*net.TCPConn); ok {
|
||||
_ = tc.SetWriteBuffer(1) // kernel clamps this up to its own floor
|
||||
}
|
||||
return c, nil
|
||||
}
|
||||
|
||||
// TestServeWS_HandshakeWrite_TimesOutOnStalledPeer pins OC-0152. It fails
|
||||
// against the pre-fix code and passes once every handshake write is wrapped
|
||||
// in a bounded write context.
|
||||
//
|
||||
// The two buffer-shrinking tricks below (tinySendBufListener +
|
||||
// SetReadBuffer) don't produce a truly infinite block in this sandbox's
|
||||
// network stack — TCP window mechanics still let bytes trickle through
|
||||
// eventually even though nothing ever calls Read. What they reliably produce
|
||||
// is a large, measurable slowdown: a ~500KB ready payload measured well
|
||||
// north of 30s to complete against the pre-fix code in this environment,
|
||||
// against a fixed writeTimeout of 10s. So instead of asserting "never
|
||||
// returns", the test asserts the behavior the fix is actually supposed to
|
||||
// guarantee: the handshake resolves (success or failure) within
|
||||
// writeTimeout-plus-margin. That holds post-fix regardless of payload size
|
||||
// (the AfterFunc-driven close fires at the deadline no matter how much data
|
||||
// is still queued) and fails pre-fix for any payload large enough to still
|
||||
// be in flight at that point — which the member count below is sized well
|
||||
// past, for margin.
|
||||
func TestServeWS_HandshakeWrite_TimesOutOnStalledPeer(t *testing.T) {
|
||||
database := openServeTestDB(t)
|
||||
limiter := auth.NewRateLimiter()
|
||||
hub := ws.NewHub(database, limiter, nil)
|
||||
go hub.Run()
|
||||
defer hub.Stop()
|
||||
|
||||
// Seed enough members that the ready payload takes long enough to
|
||||
// trickle through the shrunk buffers below that it is still in flight
|
||||
// well past writeTimeout — see the function doc for why this needs to
|
||||
// be "slow enough to still be running at the deadline", not "infinite".
|
||||
for i := range 4000 {
|
||||
if _, err := database.CreateUser(context.Background(), fmt.Sprintf("bulk-member-%d", i), "hash", 1); err != nil {
|
||||
t.Fatalf("CreateUser(bulk-member-%d): %v", i, err)
|
||||
}
|
||||
}
|
||||
|
||||
// Seed the connecting user's own session.
|
||||
userID, err := database.CreateUser(context.Background(), "stalled-peer-user", "hash", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
token, err := auth.GenerateToken()
|
||||
if err != nil {
|
||||
t.Fatalf("GenerateToken: %v", err)
|
||||
}
|
||||
if _, err := database.CreateSession(context.Background(), userID, auth.HashToken(token), "test", "127.0.0.1"); err != nil {
|
||||
t.Fatalf("CreateSession: %v", err)
|
||||
}
|
||||
|
||||
handler := ws.ServeWS(hub, database, []string{"*"}, 0)
|
||||
|
||||
ln, err := net.Listen("tcp", "127.0.0.1:0")
|
||||
if err != nil {
|
||||
t.Fatalf("net.Listen: %v", err)
|
||||
}
|
||||
srv := httptest.NewUnstartedServer(handler)
|
||||
_ = srv.Listener.Close()
|
||||
srv.Listener = &tinySendBufListener{Listener: ln}
|
||||
srv.Start()
|
||||
defer srv.Close()
|
||||
|
||||
wsURL := "ws" + strings.TrimPrefix(srv.URL, "http")
|
||||
|
||||
// Custom HTTP client that shrinks SO_RCVBUF on the dial connection to the
|
||||
// kernel minimum, so this "peer" advertises a tiny receive window once it
|
||||
// stops draining it.
|
||||
dialer := &net.Dialer{}
|
||||
httpClient := &http.Client{
|
||||
Transport: &http.Transport{
|
||||
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
|
||||
c, dialErr := dialer.DialContext(ctx, network, addr)
|
||||
if dialErr != nil {
|
||||
return nil, dialErr
|
||||
}
|
||||
if tc, ok := c.(*net.TCPConn); ok {
|
||||
_ = tc.SetReadBuffer(1) // kernel clamps this up to its own floor
|
||||
}
|
||||
return c, nil
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
dialCtx, dialCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||
defer dialCancel()
|
||||
conn, resp, err := websocket.Dial(dialCtx, wsURL, &websocket.DialOptions{HTTPClient: httpClient})
|
||||
if resp != nil && resp.Body != nil {
|
||||
defer func() { _ = resp.Body.Close() }()
|
||||
}
|
||||
if err != nil {
|
||||
t.Fatalf("websocket.Dial: %v", err)
|
||||
}
|
||||
defer func() { _ = conn.Close(websocket.StatusInternalError, "test done") }()
|
||||
|
||||
authMsg := map[string]any{
|
||||
"type": "auth",
|
||||
"payload": map[string]string{"token": token},
|
||||
}
|
||||
raw, _ := json.Marshal(authMsg)
|
||||
authCtx, authCancel := context.WithTimeout(context.Background(), 5*time.Second)
|
||||
defer authCancel()
|
||||
if err := conn.Write(authCtx, websocket.MessageText, raw); err != nil {
|
||||
t.Fatalf("write auth: %v", err)
|
||||
}
|
||||
|
||||
// Read auth_ok (small — writes/reads quickly regardless of the buffer
|
||||
// shrinking below) so that by the time we start the stall phase,
|
||||
// registration has DEFINITELY already happened (registerNow runs before
|
||||
// any handshake write — serve.go). Without this, polling ClientCount()
|
||||
// immediately after sending auth races the server's own goroutine
|
||||
// scheduling: an early poll can observe ClientCount()==0 simply because
|
||||
// registration hasn't happened *yet*, producing a false pass unrelated to
|
||||
// OC-0152 on both the buggy and the fixed code.
|
||||
readCtx, readCancel := context.WithTimeout(context.Background(), 8*time.Second)
|
||||
defer readCancel()
|
||||
_, authOKRaw, err := conn.Read(readCtx)
|
||||
if err != nil {
|
||||
t.Fatalf("read auth_ok: %v", err)
|
||||
}
|
||||
var authOK struct {
|
||||
Type string `json:"type"`
|
||||
}
|
||||
if err := json.Unmarshal(authOKRaw, &authOK); err != nil || authOK.Type != "auth_ok" {
|
||||
t.Fatalf("expected auth_ok, got %q (unmarshal err %v)", authOKRaw, err)
|
||||
}
|
||||
if hub.ClientCount() != 1 {
|
||||
t.Fatalf("ClientCount = %d right after auth_ok, want 1 (registration happens before this write)", hub.ClientCount())
|
||||
}
|
||||
|
||||
// From here on the test deliberately never reads another frame — this is
|
||||
// the stalled peer. The next handshake write is the ready payload; a
|
||||
// ClientCount of 1 below just means that write is still in flight, and
|
||||
// can only fall back to 0 once it returns (success or, post-fix, timeout)
|
||||
// and the failed-handshake teardown runs.
|
||||
deadline := time.Now().Add(15 * time.Second)
|
||||
for time.Now().Before(deadline) {
|
||||
if hub.ClientCount() == 0 {
|
||||
return // handshake write returned (timed out) and cleaned up — fixed.
|
||||
}
|
||||
time.Sleep(100 * time.Millisecond)
|
||||
}
|
||||
|
||||
t.Fatalf("OC-0152: handshake write to a stalled peer did not resolve within %v of writeTimeout margin — "+
|
||||
"ClientCount is still %d, meaning the write is bound to the bare request "+
|
||||
"context (never cancelled while the handler blocks in it) instead of a "+
|
||||
"writeTimeout-bounded one", 15*time.Second, hub.ClientCount())
|
||||
}
|
||||
@@ -477,9 +477,22 @@ func (h *Hub) voiceJoinComplete(ctx context.Context, c *Client, ch *db.Channel,
|
||||
h.broadcastVoiceEvent(ctx, channelID, buildVoiceState(*state))
|
||||
|
||||
// Send existing channel voice states to the joiner.
|
||||
//
|
||||
// OC-0172: this read is the ONLY place the server ever relays an existing
|
||||
// participant's stored ECDH public key (voice_e2ee_announce) to a joiner
|
||||
// — mid-call peers never counter-announce, they only answer an offer. A
|
||||
// swallowed error here used to just `return`, leaving the joiner's own
|
||||
// voice_state already broadcast to everyone (above) but the joiner
|
||||
// itself blind to who else is in the channel and unable to complete the
|
||||
// E2EE key exchange: it times out ~15s later with no explanation. Treat
|
||||
// this the same as every other post-commit failure in this handler
|
||||
// (rollbackVoiceJoin + an error frame), broadcasting the compensating
|
||||
// voice_leave for the voice_state that already went out.
|
||||
existing, err := h.db.GetChannelVoiceStates(ctx, channelID)
|
||||
if err != nil {
|
||||
slog.Error("ws handleVoiceJoin GetChannelVoiceStates", "err", err)
|
||||
h.rollbackVoiceJoin(ctx, c, channelID, state.JoinedAt, true)
|
||||
c.sendMsg(buildErrorMsg(ErrCodeInternal, "failed to join voice channel"))
|
||||
return
|
||||
}
|
||||
for _, vs := range existing {
|
||||
|
||||
Reference in New Issue
Block a user