fix: correctness fixes from the 2026-08-20 bug hunt (#1398)

* fix(identity): 2 defect(s) (OC-0192, OC-0197)

OC-0192: bound raw display_name/about/avatar bytes before the quadratic
fixpoint sanitizer runs, in both the REST handler and UserService.UpdateProfile.

OC-0197: sanitize display_name before validateDisplayName so an
HTML-entity-encoded bidi override (e.g. "‮") can no longer pass
validation as ASCII and be decoded into the real character on the way to
storage.

* fix(ws): 1 defect(s) (OC-0196)

A transient DB error during WebSocket auth (session or user lookup) was
collapsed into the terminal auth_error frame, which the client treats as
non-recoverable: it stops reconnecting and clears stored credentials. A
sub-second SQLite hiccup therefore force-logged-out every reconnecting
client with a perfectly valid session. Send a non-terminal INTERNAL error
frame instead so normal backoff/reconnect retries.

* fix(api): 1 defect(s) (OC-0198)

* fix(ws): 1 defect(s) (OC-0200)

normalizeHostForCertCompare now unwraps a bracketed IPv6 literal after the
trailing-":443" strip and before lowercasing, matching tofu::cert_store_key's
normalization order. Without the unwrap, every cert-tofu host equality guard
took the "unrelated host" branch for bracketed-IPv6 servers.

* fix(api): 1 defect(s) (OC-0202)

* fix(admin): 1 defect(s) (OC-0203)

Channel permission override handlers applied requireGrantableOverride only
to the bits being written, so an all-zero PUT or a DELETE could clear a
deny bit the actor's own role does not hold — EffectivePerms =
(rolePerm &^ deny) | allow makes removing a deny an escalation. Both the
role-layer and per-user handlers now check the guard against the bits
already on the row.

* fix(client): 1 defect(s) (OC-0205)

* fix(client): 3 defect(s) (OC-0207, OC-0227, OC-0235)

* fix(client): 1 defect(s) (OC-0208)

* fix(voice): 3 defect(s) (OC-0209, OC-0212, OC-0213)

OC-0209: reject a replayed retired-key announce before verifyPeerAnnounce
runs, so the replay cannot overwrite the peer's displayed verification
status/session fingerprint with the retired key's before being rejected.

OC-0212: buffer an announce blocked as a TOFU pin mismatch and replay it
after a successful rePinPeerIdentity, so re-pinning actually restores the
peer for the live call instead of clearing the badge and leaving them
un-keyed (a mid-call peer never re-announces on its own).

OC-0213: skip retiring a departing peer's key when the local voice roster
still lists them as present — a rejoin announce published straight into
the send queue can overtake the buffered, stale voice_leave, and retiring
a still-live key would reject every later genuine re-announce as a replay.

* fix(ws): 1 defect(s) (OC-0211)

* fix(identity): 1 defect(s) (OC-0214)

The delete-account admin guard counted remaining admins with a raw
`banned = 0` filter, so an admin whose temporary ban had already lapsed
was treated as unusable. Use the shared notBannedClause, appended outside
the Sprintf format string because its strftime verbs (%Y, %H) would
otherwise be parsed as fmt directives.

* fix(client): 1 defect(s) (OC-0215)

* fix(voice): 1 defect(s) (OC-0216)

* fix(client): 1 defect(s) (OC-0217)

* fix(voice): 1 defect(s) (OC-0219)

rollbackVoiceJoin cleared the client's in-memory voiceChID but left its
VoiceTopic subscription in place, so a socket whose join failed after
voiceJoinComplete's Subscribe kept receiving that room's E2EE relays for
the rest of the connection. Use clearVoiceAndUnsubscribe instead, matching
every other path that takes a client out of voice while its WS stays up.

* fix(client): 2 defect(s) (OC-0220, OC-0224)

dmDisplayName: a group DM whose other members have all left keeps a live
is_group row, but the server leaves `recipient` zero-valued, so the empty
username fell through as a blank label. Fall back to a non-empty placeholder.

updateDmLastMessage: a queued chat_message redelivered for an id already
reflected in the `ready` snapshot double-counted the unread badge. Only
increment when the message id advances past lastMessageId.

* fix(client): 1 defect(s) (OC-0221)

Cap queued attachments at the server's 10-attachment limit in the message
composer. Past that the server rejects the whole chat_send frame as a
generic parse error, orphaning already-uploaded attachments; refusing
before the upload starts keeps composer state and the send in sync.

* fix(ws): 1 defect(s) (OC-0222)

handleReconnect built the resume auth_ok before applyConnectStatus settled
c.user.Status, so a resumed client was told its disconnect-time status
(routinely "offline") instead of the status it was coming online as.
Move applyConnectStatus ahead of reconnectWriteReplay, matching
handleFreshConnect's ordering.

* fix(mentions): 1 defect(s) (OC-0223)

* fix(admin): 1 defect(s) (OC-0225)

* fix(client): 1 defect(s) (OC-0226)

* fix(client): 1 defect(s) (OC-0228)

* fix(client): 1 defect(s) (OC-0230)

Route the Logs tab entry counter through renderLogEntries so every render path (filter change, Clear, Refresh, live entry) keeps the count in sync with the list.

* fix(voice): 1 defect(s) (OC-0231)

* fix(client): 1 defect(s) (OC-0232)

Reduce Motion toggle wrote the reduced-motion class directly, fighting the
OS-sync media-query listener that owns it when Sync with OS is on. Route the
side effect through syncOsMotionListener so whichever source owns the class
re-derives it.

* fix(client): 1 defect(s) (OC-0233)

notifyIncomingMessage titled the desktop notification with the raw
payload username, so the popup named the sender differently from the
message row it points at. Resolve the author the same way the message
list does (resolveAuthor over the live membersStore, then
resolveDisplayName).

* fix(client): 1 defect(s) (OC-0234)

* fix(client): 1 defect(s) (OC-0236)

* fix(ws): 1 defect(s) (OC-0237)

* fix(client): 4 defect(s) (OC-0193, OC-0201, OC-0204, OC-0218)

* fix(identity): 1 defect(s) (OC-0195)

Bound free-text profile fields by raw byte length before cleanText's
quadratic sanitizeToFixpoint pass runs, generalizing OC-0192's guard into
cleanTextBounded and applying it to HandlePresenceUpdate's custom_status,
SetCustomStatus, and group DM names.

* fix(dm): 1 defect(s) (OC-0199)

handleCreateDM now broadcasts dm_channel_open to the recipient when a 1:1 DM is newly created, matching handleCreateGroupDM. GetOrCreateDMChannel pre-seeds dm_open_state for both users, so the recipient's later OpenDM reported opened=false and nothing ever told them the DM existed.

* fix(voice): 1 defect(s) (OC-0206)

vad-worklet.js gate timing constants were copied from the setTimeout
fallback's ~16ms poll cadence, but AudioWorkletProcessor.process() runs
once per 128-sample render quantum (~2.667ms at the 48kHz AudioContext).
The mic gate therefore closed ~6x faster than intended (~32ms of silence
instead of ~200ms), with the startup grace and RMS post interval off by
the same factor. Scale the frame counts to render quanta.

* fix(client): 1 defect(s) (OC-0229)

* test(client): assert the real TOFU re-pin outcome and make the pin mock faithful

The e2e journey test asserted that "Trust New Key" makes the peer's verify
badge disappear. That is the behaviour OC-0212 identifies as the defect: a
mid-call peer never re-announces, so clearing the badge left the peer
un-keyed for the rest of the call with nothing on screen. Re-pinning now
replays the announce that was blocked as a mismatch and re-verifies it
against the pin just stored, so assert the peer actually lands verified.

The mock's store_identity_pin was a no-op recorder while get_identity_pin
served a static seed map, so the replayed announce re-read the stale pin and
re-failed — a mismatch the real keyring never produces. Back the pins with a
mutable map so a write is visible to the next read. The unreadable-store
(DC-08) and reject-keeps-blocked paths are unchanged and still pass.

* fix(dm): 1 defect(s) (OC-0194)

Add regression tests pinning the raw-byte bound on group DM names, for
both CreateGroupDM and RenameGroupDM.

The Server/service/dm.go source fix for OC-0194 already landed in
bdbd5ac (fix(identity): 1 defect(s) (OC-0195)), which generalized the
guard into cleanTextBounded and applied it to the group DM name paths
alongside the profile fields. This commit therefore carries the OC-0194
tests only; dm.go is unchanged.

Revert-proof: with dm.go restored to bdbd5ac^ (cleanText before the
rune-count check) both new tests fail — CreateGroupDM returns "recipient
not found" after 222ms and RenameGroupDM accepts the name after 251ms,
against a 150ms budget. With the fix in place both pass in 0.03s.

* fix(ws): 1 defect(s) (OC-0210)

* chore(findings): record the 2026-08-20 hunt's 46 findings as fixed

Appends OC-0192..OC-0237 from the 2026-08-20 converging hunt and marks each
fixed with its commit and the test that pins it. Pre-existing records are
byte-identical; nextId moves 192 -> 238 so the next hunt cannot collide with
these ids.

Every fix was independently revert-proofed: the commit's own source diff is
reverse-applied, its test must go red, and must return green once restored.
43 of 46 carry revertProof "pass" from that mechanical run. Three could not be
checked at file level and were proved by hand at hunk level instead, recorded
as "pass (hand-proved)": OC-0200, whose ws.ts edit no longer reverse-applies
because the merge kept main's equivalent implementation; OC-0215, whose Rust
tests live in-file under #[cfg(test)]; and OC-0194, which stacks on a helper
introduced by an earlier commit. No fix was found to rest on a vacuous test.

OC-0200 additionally carries a note: main fixed that same normalizer
independently while this branch was in flight, so the branch is no longer the
only thing closing it.

* docs: record the dm_channel_open emission on 1:1 DM creation

POST /api/v1/dms now emits dm_channel_open to the recipient when it creates a
channel (it previously emitted nothing on that path), so api.md states it the
way the sibling DM endpoints already state theirs.

The channels/members/DMs UX spec claimed the server broadcast the event "to
both parties" on this flow. That was never true — nothing was broadcast before,
and now only the recipient is sent it; the creator learns the channel from the
response body. This doc lists dispatcher.ts, dm.store.ts, ChannelSidebar.ts,
service/channel.go and dm.go among its sources of truth, all touched here, so
it is corrected in the same change per its maintenance rule.

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
J3vb
2026-08-20 20:45:30 +02:00
committed by GitHub
co-authored by Claude
parent d880b64d64
commit 5202e3fe1e
91 changed files with 6345 additions and 284 deletions
+50 -2
View File
@@ -138,7 +138,17 @@ func handlePutChannelPermission(database *db.DB, hub HubBroadcaster, permInvalid
}
// Escalation guard: a MANAGE_CHANNELS holder without ADMINISTRATOR
// cannot grant bits their own role lacks via a channel override.
if err := requireGrantableOverride(actorRole, allow, deny); err != nil {
// Checked against the union of the bits being written and the bits
// already present on the row: clearing an existing deny is also a
// grant (EffectivePerms = (rolePerm &^ deny) | allow), so writing an
// all-zero mask over a deny the actor's own role lacks must not slip
// past this guard just because the NEW mask alone is empty.
curAllow, curDeny, err := database.GetChannelPermissions(r.Context(), ch.ID, roleID)
if err != nil {
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to fetch channel permission")
return
}
if err := requireGrantableOverride(actorRole, curAllow|allow, curDeny|deny); err != nil {
writeErr(w, http.StatusForbidden, "FORBIDDEN", err.Error())
return
}
@@ -215,6 +225,21 @@ func handleDeleteChannelPermission(database *db.DB, hub HubBroadcaster, permInva
writeErr(w, http.StatusUnauthorized, "UNAUTHORIZED", "not authenticated")
return
}
// Escalation guard: deleting an override is a permission mutation with
// the same authority as writing one — removing a deny row restores
// exactly the access the PUT path refuses to grant (EffectivePerms =
// (rolePerm &^ deny) | allow) — so gate it identically to
// handlePutChannelPermission, checked against the bits the deleted row
// actually carries.
curAllow, curDeny, err := database.GetChannelPermissions(r.Context(), ch.ID, roleID)
if err != nil {
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to fetch channel permission")
return
}
if err := requireGrantableOverride(actorRole, curAllow, curDeny); err != nil {
writeErr(w, http.StatusForbidden, "FORBIDDEN", err.Error())
return
}
// Hierarchy guard: deleting an override is a permission mutation with the
// same authority as writing one (removing a deny row restores exactly the
// access the PUT path refuses to grant), so gate it identically to
@@ -339,7 +364,16 @@ func handlePutChannelUserPermission(database *db.DB, hub HubBroadcaster, permInv
}
// Escalation guard: a MANAGE_CHANNELS holder without ADMINISTRATOR
// cannot grant bits their own role lacks via a per-user override.
if err := requireGrantableOverride(actorRole, allow, deny); err != nil {
// Checked against the union of the bits being written and the bits
// already present on the row, same rationale as
// handlePutChannelPermission: clearing an existing deny is a grant, so
// an all-zero write must not bypass this guard.
curAllow, curDeny, err := database.GetUserChannelPermissions(r.Context(), ch.ID, user.ID)
if err != nil {
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to fetch channel user permission")
return
}
if err := requireGrantableOverride(actorRole, curAllow|allow, curDeny|deny); err != nil {
writeErr(w, http.StatusForbidden, "FORBIDDEN", err.Error())
return
}
@@ -392,6 +426,20 @@ func handleDeleteChannelUserPermission(database *db.DB, hub HubBroadcaster, perm
writeErr(w, http.StatusUnauthorized, "UNAUTHORIZED", "not authenticated")
return
}
// Escalation guard: clearing a per-user override restores exactly the
// access the PUT path refuses to grant (EffectivePerms = (rolePerm &^
// deny) | allow), so gate it identically to
// handlePutChannelUserPermission, checked against the bits the
// deleted row actually carries.
curAllow, curDeny, err := database.GetUserChannelPermissions(r.Context(), ch.ID, user.ID)
if err != nil {
writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to fetch channel user permission")
return
}
if err := requireGrantableOverride(actorRole, curAllow, curDeny); err != nil {
writeErr(w, http.StatusForbidden, "FORBIDDEN", err.Error())
return
}
// Hierarchy guard: clearing a higher-ranked member's override is the
// same authority as writing one, so gate it identically.
if !requireManageableUser(database, w, r, user, actorRole) {
@@ -431,3 +431,91 @@ func TestDeleteChannelPermission_UnknownRole(t *testing.T) {
t.Errorf("status = %d, want 404; body: %s", w.Code, w.Body.String())
}
}
// Clearing an override is a permission grant when it removes a deny bit the
// actor's own role does not hold: EffectivePerms = (rolePerm &^ deny) | allow,
// so wiping a deny row hands back exactly the access the PUT path refuses to
// grant (TestPutChannelPermission_ModeratorCannotEscalate). The DELETE
// handler must apply requireGrantableOverride to the override being REMOVED,
// not skip it just because the hierarchy guard alone passes.
func TestDeleteChannelPermission_EscalationGuard(t *testing.T) {
database := openAdminTestDB(t)
handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
// Helper role: low position, base permissions include MANAGE_MESSAGES.
if _, err := database.ExecContext(context.Background(),
`INSERT INTO roles (id, name, color, permissions, position, is_default)
VALUES (20, 'Helper', NULL, ?, 5, 0)`,
permissions.ManageMessages,
); err != nil {
t.Fatalf("seed Helper role: %v", err)
}
// Actor: MANAGE_CHANNELS holder without MANAGE_MESSAGES or ADMINISTRATOR,
// ranked above Helper so only the escalation guard is exercised.
_, modToken := createRoleUser(t, database, 10, "Moderator", permissions.ManageChannels, 70, "moduser")
chID, err := database.CreateChannel(context.Background(), "escalate-del", "text", "", "", 0)
if err != nil {
t.Fatalf("CreateChannel: %v", err)
}
if err := database.UpsertChannelOverride(context.Background(), chID, 20, 0, permissions.ManageMessages); err != nil {
t.Fatalf("UpsertChannelOverride: %v", err)
}
w := doRequest(t, handler, http.MethodDelete,
"/channels/"+itoa(chID)+"/permissions/20", modToken, nil)
if w.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403; body: %s", w.Code, w.Body.String())
}
allow, deny, err := database.GetChannelPermissions(context.Background(), chID, 20)
if err != nil {
t.Fatalf("GetChannelPermissions: %v", err)
}
if allow != 0 || deny != permissions.ManageMessages {
t.Errorf("override mutated by forbidden delete: (%#x, %#x)", allow, deny)
}
}
// A PUT with an all-zero mask that clears an existing deny bit the actor's
// own role does not hold is exactly as much an escalation as writing that
// bit directly (TestPutChannelPermission_ModeratorCannotEscalate): clearing a
// deny is a grant. requireGrantableOverride must see the bits being REMOVED
// by this write, not just the (trivially empty) bits being written.
func TestPutChannelPermission_ClearByZeroMaskEscalationGuard(t *testing.T) {
database := openAdminTestDB(t)
handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
if _, err := database.ExecContext(context.Background(),
`INSERT INTO roles (id, name, color, permissions, position, is_default)
VALUES (20, 'Helper', NULL, ?, 5, 0)`,
permissions.ManageMessages,
); err != nil {
t.Fatalf("seed Helper role: %v", err)
}
_, modToken := createRoleUser(t, database, 10, "Moderator", permissions.ManageChannels, 70, "moduser")
chID, err := database.CreateChannel(context.Background(), "escalate-zero", "text", "", "", 0)
if err != nil {
t.Fatalf("CreateChannel: %v", err)
}
if err := database.UpsertChannelOverride(context.Background(), chID, 20, 0, permissions.ManageMessages); err != nil {
t.Fatalf("UpsertChannelOverride: %v", err)
}
w := doRequest(t, handler, http.MethodPut,
"/channels/"+itoa(chID)+"/permissions/20", modToken,
map[string]any{"allow": 0, "deny": 0})
if w.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403; body: %s", w.Code, w.Body.String())
}
allow, deny, err := database.GetChannelPermissions(context.Background(), chID, 20)
if err != nil {
t.Fatalf("GetChannelPermissions: %v", err)
}
if allow != 0 || deny != permissions.ManageMessages {
t.Errorf("override mutated by forbidden zero-mask PUT: (%#x, %#x)", allow, deny)
}
}
@@ -382,3 +382,72 @@ func TestDeleteChannelUserPermission_ClearsOverride(t *testing.T) {
t.Errorf("second delete status = %d, want 204", w.Code)
}
}
// Clearing a per-user override is a permission grant when it removes a deny
// bit the actor's own role does not hold, exactly like the role-layer case
// (TestDeleteChannelPermission_EscalationGuard in handlers_channel_perms_test.go).
// The DELETE handler must apply requireGrantableOverride to the override
// being REMOVED, not skip the escalation guard because hierarchy alone
// passes.
func TestDeleteChannelUserPermission_EscalationGuard(t *testing.T) {
database := openAdminTestDB(t)
handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
// Actor: MANAGE_CHANNELS holder without MANAGE_MESSAGES or ADMINISTRATOR.
_, modToken := createRoleUser(t, database, 10, "Moderator", permissions.ManageChannels, 70, "moduser")
target := seedOverrideTarget(t, database, "escalate-del-target")
chID, err := database.CreateChannel(context.Background(), "escalate-del-user", "text", "", "", 0)
if err != nil {
t.Fatalf("CreateChannel: %v", err)
}
if err := database.UpsertChannelUserOverride(context.Background(), chID, target, 0, permissions.ManageMessages); err != nil {
t.Fatalf("UpsertChannelUserOverride: %v", err)
}
w := doRequest(t, handler, http.MethodDelete,
"/channels/"+itoa(chID)+"/user-permissions/"+itoa(target), modToken, nil)
if w.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403; body: %s", w.Code, w.Body.String())
}
allow, deny, err := database.GetUserChannelPermissions(context.Background(), chID, target)
if err != nil {
t.Fatalf("GetUserChannelPermissions: %v", err)
}
if allow != 0 || deny != permissions.ManageMessages {
t.Errorf("override mutated by forbidden delete: (%#x, %#x)", allow, deny)
}
}
// Same escalation, reached through a PUT that writes an all-zero mask: it
// still clears the existing deny bit, which is a grant
// (TestPutChannelPermission_ClearByZeroMaskEscalationGuard's per-user twin).
func TestPutChannelUserPermission_ClearByZeroMaskEscalationGuard(t *testing.T) {
database := openAdminTestDB(t)
handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database))
_, modToken := createRoleUser(t, database, 10, "Moderator", permissions.ManageChannels, 70, "moduser")
target := seedOverrideTarget(t, database, "escalate-zero-target")
chID, err := database.CreateChannel(context.Background(), "escalate-zero-user", "text", "", "", 0)
if err != nil {
t.Fatalf("CreateChannel: %v", err)
}
if err := database.UpsertChannelUserOverride(context.Background(), chID, target, 0, permissions.ManageMessages); err != nil {
t.Fatalf("UpsertChannelUserOverride: %v", err)
}
w := doRequest(t, handler, http.MethodPut,
"/channels/"+itoa(chID)+"/user-permissions/"+itoa(target), modToken,
map[string]any{"allow": 0, "deny": 0})
if w.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403; body: %s", w.Code, w.Body.String())
}
allow, deny, err := database.GetUserChannelPermissions(context.Background(), chID, target)
if err != nil {
t.Fatalf("GetUserChannelPermissions: %v", err)
}
if allow != 0 || deny != permissions.ManageMessages {
t.Errorf("override mutated by forbidden zero-mask PUT: (%#x, %#x)", allow, deny)
}
}
+12 -2
View File
@@ -3,6 +3,7 @@ package admin
import (
"context"
"errors"
"log/slog"
"net/http"
"github.com/owncord/server/auth"
@@ -53,9 +54,18 @@ func adminAuthMiddleware(database *db.DB) func(http.Handler) http.Handler {
writeErr(w, http.StatusUnauthorized, "UNAUTHORIZED", "user not found")
case errors.Is(err, auth.ErrRoleNotFound):
writeErr(w, http.StatusUnauthorized, "UNAUTHORIZED", "role not found")
default:
// ErrTokenNotFound or a wrapped DB error.
case errors.Is(err, auth.ErrTokenNotFound):
writeErr(w, http.StatusUnauthorized, "UNAUTHORIZED", "invalid or expired session")
default:
// A wrapped DB error, not one of the sentinels above (mirrors
// api.AuthMiddleware). A DB outage is not a bad token:
// answering 401 here would make the client treat a live,
// valid session as expired — the desktop client's doFetch
// 401 sink clears auth and deletes the stored credential for
// a session that was never revoked. Log it and report the
// failure as a server-side fault instead.
slog.ErrorContext(r.Context(), "admin: token resolution failed", "error", err)
writeErr(w, http.StatusServiceUnavailable, "SERVICE_UNAVAILABLE", "authentication service temporarily unavailable")
}
return
}
+66
View File
@@ -0,0 +1,66 @@
// Package admin whitebox test for OC-0225: adminAuthMiddleware must not
// report a transient DB error from auth.ResolveTokenHash as 401. A wrapped
// DB error is not "invalid or expired session" — treating it as one ejects
// an admin whose session was never revoked (see the finding for the desktop
// client's onUnauthorized -> clearAuth -> deleteCredential chain triggered by
// a stray 401).
package admin
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"testing"
"github.com/owncord/server/auth"
)
// TestAdminAuthMiddleware_DBErrorIsNotUnauthorized verifies that when
// ResolveTokenHash fails with a wrapped (non-sentinel) DB error — as happens
// when the underlying SQLite connection is unavailable — adminAuthMiddleware
// reports 503 SERVICE_UNAVAILABLE, not 401 UNAUTHORIZED. A 401 here is
// indistinguishable from a genuinely dead/unknown session and drives the
// desktop client to clear auth and delete the stored credential for a
// session that was never actually revoked.
func TestAdminAuthMiddleware_DBErrorIsNotUnauthorized(t *testing.T) {
database := openWhiteboxTestDB(t)
uid, err := database.CreateUser(context.Background(), "dberroruser", "$2a$12$x", 1)
if err != nil {
t.Fatalf("CreateUser: %v", err)
}
token := "db-error-token"
if _, err := database.CreateSession(context.Background(), uid, auth.HashToken(token), "test", "127.0.0.1"); err != nil {
t.Fatalf("CreateSession: %v", err)
}
// Close the DB so the very next query — GetSessionByTokenHash, called
// from inside ResolveTokenHash — fails with a wrapped, non-sentinel
// error (not sql.ErrNoRows, so not ErrTokenNotFound either).
if err := database.Close(); err != nil {
t.Fatalf("Close: %v", err)
}
handler := NewAdminAPI(database, "1.0.0", nil, nil, nil, nil, nil, nil, nil)
req := httptest.NewRequest(http.MethodGet, "/stats", nil)
req.Header.Set("Authorization", "Bearer "+token)
w := httptest.NewRecorder()
handler.ServeHTTP(w, req)
if w.Code == http.StatusUnauthorized {
t.Fatalf("status = %d (UNAUTHORIZED), want 503 (SERVICE_UNAVAILABLE) for a transient DB error; body: %s", w.Code, w.Body.String())
}
if w.Code != http.StatusServiceUnavailable {
t.Errorf("status = %d, want 503 (SERVICE_UNAVAILABLE); body: %s", w.Code, w.Body.String())
}
var resp map[string]string
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
t.Fatalf("unmarshal: %v", err)
}
if resp["error"] == "UNAUTHORIZED" {
t.Errorf("error = %q, must not be UNAUTHORIZED for a DB outage", resp["error"])
}
}
+25 -4
View File
@@ -72,7 +72,7 @@ var _ dmVoiceEvictor = (*ws.Hub)(nil)
func MountDMRoutes(r chi.Router, database *db.DB, svc *service.Services, broadcaster DMBroadcaster) {
r.Route("/api/v1/dms", func(r chi.Router) {
r.Use(AuthMiddleware(database))
r.Post("/", handleCreateDM(svc))
r.Post("/", handleCreateDM(svc, broadcaster))
r.Post("/group", handleCreateGroupDM(svc, broadcaster))
r.Get("/", handleListDMs(svc))
r.Patch("/{channelId}", handleRenameGroupDM(svc, broadcaster))
@@ -117,7 +117,7 @@ type listDMsResponse struct {
}
// handleCreateDM creates or retrieves a DM channel with a recipient.
func handleCreateDM(svc *service.Services) http.HandlerFunc {
func handleCreateDM(svc *service.Services, broadcaster DMBroadcaster) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
user, ok := r.Context().Value(UserKey).(*db.User)
if !ok || user == nil {
@@ -141,6 +141,19 @@ func handleCreateDM(svc *service.Services) http.HandlerFunc {
return
}
// A brand-new 1:1 DM has dm_open_state pre-seeded for BOTH users by
// GetOrCreateDMChannel (db/dm_queries.go), so the recipient's first
// OpenDM call — fired later from the sender's first message — finds
// the row already present and reports opened=false. Without this,
// nothing ever tells the recipient the DM exists: no live event, and
// no visibility-watermark bump for a warm reconnect either. Only the
// creation path needs this — CreateDM re-opening an existing DM for
// the caller only touches the caller's own dm_open_state row, which
// the caller obviously already knows about.
if result.Created {
broadcastDMOpen(r.Context(), svc, broadcaster, result.Channel.ID, []int64{result.Recipient.ID})
}
avatarStr := ""
if result.Recipient.Avatar != nil {
avatarStr = *result.Recipient.Avatar
@@ -399,6 +412,14 @@ func handleBlockUser(svc *service.Services, broadcaster DMBroadcaster) http.Hand
return
}
// The block has already committed at this point, so the rest of this
// handler must survive the caller's request context being cancelled
// right after that commit (client disconnect mid-handler) — same
// reasoning as handleRenameGroupDM's own bgCtx. Without this, a
// canceled request context makes the shared-DM lookup below fail and
// get skipped, silently defeating the eviction it gates.
bgCtx := context.WithoutCancel(r.Context())
// Revocation must evict a live session, not merely block the next
// join (the same invariant the voice sweep states): without this, a
// blocked user already in the pair's 1:1 DM voice call stays in it
@@ -407,11 +428,11 @@ func handleBlockUser(svc *service.Services, broadcaster DMBroadcaster) http.Hand
// controls. Group DM calls are deliberately untouched, matching
// requireDMNotBlocked's group exemption.
if ve, evictable := broadcaster.(dmVoiceEvictor); evictable {
if chID, exists, err := svc.DMs.SharedOneToOneDM(r.Context(), user.ID, targetID); err != nil {
if chID, exists, err := svc.DMs.SharedOneToOneDM(bgCtx, user.ID, targetID); err != nil {
slog.Warn("block: shared-DM lookup for voice eviction failed",
"blocker_id", user.ID, "target_id", targetID, "err", err)
} else if exists {
ve.DisconnectFromVoiceInChannel(context.WithoutCancel(r.Context()), targetID, chID)
ve.DisconnectFromVoiceInChannel(bgCtx, targetID, chID)
}
}
@@ -0,0 +1,88 @@
package api_test
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"testing"
"github.com/go-chi/chi/v5"
"github.com/owncord/server/api"
"github.com/owncord/server/auth"
"github.com/owncord/server/db"
"github.com/owncord/server/service"
)
// cancelAfterBlockStore wraps a real in-memory *db.DB and cancels an
// externally supplied context the instant BlockUser's write commits,
// simulating a client disconnect landing between the block commit and the
// post-commit voice-eviction gate (OC-0198). FindDMChannelIDBetween is
// overridden to fail fast on an already-canceled context, mirroring the
// context.Canceled a real sql query would surface in that window.
type cancelAfterBlockStore struct {
*db.DB
cancel context.CancelFunc
}
func (s *cancelAfterBlockStore) BlockUser(ctx context.Context, blockerID, blockedID int64) error {
err := s.DB.BlockUser(ctx, blockerID, blockedID)
if err == nil {
s.cancel()
}
return err
}
func (s *cancelAfterBlockStore) FindDMChannelIDBetween(ctx context.Context, user1ID, user2ID int64) (int64, bool, error) {
if err := ctx.Err(); err != nil {
return 0, false, err
}
return s.DB.FindDMChannelIDBetween(ctx, user1ID, user2ID)
}
// TestBlockUser_EvictsVoiceEvenIfRequestContextCanceledAfterCommit pins
// OC-0198: BlockUser has already committed once the store call returns, so a
// client disconnect that cancels the request context right after must not
// suppress the post-commit voice eviction. The shared-DM lookup gating that
// eviction has to run on a context detached from the request — the same way
// the eviction call itself already does — or the blocked user stays in the
// blocker's live 1:1 DM call forever.
func TestBlockUser_EvictsVoiceEvenIfRequestContextCanceledAfterCommit(t *testing.T) {
database := newDMTestDB(t)
bc := &watermarkVoiceBroadcaster{mockBroadcaster: &mockBroadcaster{}}
alice := dmCreateToken(t, database, "alice", 4)
dmCreateToken(t, database, "bob", 4)
setupRouter := buildDMRouter(database, bc)
rr := dmPost(t, setupRouter, "/api/v1/dms", alice, map[string]any{"recipient_id": 2})
var created struct {
ChannelID int64 `json:"channel_id"`
}
if err := json.Unmarshal(rr.Body.Bytes(), &created); err != nil {
t.Fatalf("decode create-dm response %q: %v", rr.Body.String(), err)
}
bc.evictCalls = nil
ctx, cancel := context.WithCancel(context.Background())
store := &cancelAfterBlockStore{DB: database, cancel: cancel}
svc := service.New(store, auth.NewRateLimiter())
r := chi.NewRouter()
api.MountDMRoutes(r, database, svc, bc)
req := httptest.NewRequest(http.MethodPut, "/api/v1/blocks/2", nil)
req.Header.Set("Authorization", "Bearer "+alice)
req.RemoteAddr = "127.0.0.1:9999"
req = req.WithContext(ctx)
blockRR := httptest.NewRecorder()
r.ServeHTTP(blockRR, req)
if blockRR.Code != http.StatusOK {
t.Fatalf("block: %d %s", blockRR.Code, blockRR.Body.String())
}
if len(bc.evictCalls) != 1 || bc.evictCalls[0].userID != 2 || bc.evictCalls[0].channelID != created.ChannelID {
t.Fatalf("DisconnectFromVoiceInChannel calls = %+v, want exactly one for user=2 channel=%d even though "+
"the request context was canceled right after the block commit", bc.evictCalls, created.ChannelID)
}
}
@@ -0,0 +1,72 @@
package api_test
import (
"bytes"
"context"
"encoding/json"
"net/http"
"testing"
)
// TestCreateDM_Success_NotifiesRecipient pins OC-0199: a REST-created 1:1 DM
// must tell the recipient about it immediately (a dm_channel_open event),
// mirroring what handleCreateGroupDM already does via broadcastDMOpen.
//
// Without this, GetOrCreateDMChannel pre-opens dm_open_state for BOTH users
// at creation time, so the recipient's OpenDM call on the first message
// later finds the row already present (INSERT OR IGNORE affects 0 rows) and
// never reports "opened" either — leaving the recipient with no live event
// and no visibility-watermark bump to pick the DM up on a warm reconnect.
func TestCreateDM_Success_NotifiesRecipient(t *testing.T) {
database := newDMTestDB(t)
broadcaster := &mockBroadcaster{}
router := buildDMRouter(database, broadcaster)
tokenAlice := dmCreateToken(t, database, "notify_alice", 4)
_ = dmCreateToken(t, database, "notify_bob", 4)
bob, err := database.GetUserByUsername(context.Background(), "notify_bob")
if err != nil || bob == nil {
t.Fatalf("lookup bob: %v", err)
}
rr := dmPost(t, router, "/api/v1/dms", tokenAlice, map[string]any{
"recipient_id": bob.ID,
})
if rr.Code != http.StatusCreated {
t.Fatalf("CreateDM: status = %d, want 201; body = %s", rr.Code, rr.Body.String())
}
var gotOpenForBob bool
for _, m := range broadcaster.sent {
if m.UserID != bob.ID {
continue
}
var payload struct {
Type string `json:"type"`
}
if jsonErr := json.Unmarshal(m.Msg, &payload); jsonErr != nil {
continue
}
if payload.Type == "dm_channel_open" {
gotOpenForBob = true
}
}
if !gotOpenForBob {
t.Errorf("CreateDM: recipient %d never got a dm_channel_open broadcast; sent = %v",
bob.ID, dumpSent(broadcaster.sent))
}
}
func dumpSent(sent []mockBroadcastMsg) string {
var b bytes.Buffer
for _, m := range sent {
b.WriteString(m.String())
b.WriteByte('\n')
}
return b.String()
}
// String renders a mockBroadcastMsg for test failure output.
func (m mockBroadcastMsg) String() string {
return string(m.Msg)
}
+16
View File
@@ -67,6 +67,22 @@ CREATE TABLE IF NOT EXISTS sessions (
);
CREATE INDEX IF NOT EXISTS idx_sessions_token ON sessions(token);
-- AuthMiddleware falls through to an API-token lookup whenever a bearer
-- token matches no session (auth.ResolveTokenHash), so this table must exist
-- even in DM-only fixtures — otherwise an ordinary "no such session" lookup
-- for a garbage/unknown token hits GetActiveAPIToken and fails with a real
-- "no such table" SQL error instead of the intended not-found sentinel.
CREATE TABLE IF NOT EXISTS api_tokens (
id INTEGER PRIMARY KEY AUTOINCREMENT,
user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
token_hash TEXT NOT NULL UNIQUE,
label TEXT NOT NULL DEFAULT '',
created_at TEXT NOT NULL DEFAULT (datetime('now')),
last_used_at TEXT,
expires_at TEXT,
revoked_at TEXT
);
CREATE TABLE IF NOT EXISTS channels (
id INTEGER PRIMARY KEY AUTOINCREMENT,
name TEXT NOT NULL,
+13 -6
View File
@@ -114,17 +114,24 @@ func AuthMiddleware(database *db.DB) func(http.Handler) http.Handler {
Message: "role not found",
})
return
case err != nil:
// ErrTokenNotFound or a wrapped DB error. A DB outage is not a bad
// token — log it so it's distinguishable from ordinary 401s.
if !errors.Is(err, auth.ErrTokenNotFound) {
slog.ErrorContext(r.Context(), "auth: token resolution failed", "error", err)
}
case errors.Is(err, auth.ErrTokenNotFound):
writeJSON(w, http.StatusUnauthorized, errorResponse{
Error: "UNAUTHORIZED",
Message: "invalid or expired session",
})
return
case err != nil:
// A wrapped DB error, not one of the sentinels above. A DB outage
// is not a bad token: answering 401 here would make the client
// treat a live, valid session as expired — it clears auth,
// disconnects the WS, and deletes the stored credential. Log it
// and report the failure as a server-side fault instead.
slog.ErrorContext(r.Context(), "auth: token resolution failed", "error", err)
writeJSON(w, http.StatusServiceUnavailable, errorResponse{
Error: "SERVICE_UNAVAILABLE",
Message: "authentication service temporarily unavailable",
})
return
}
// Reject effectively-banned users before any further processing.
+37
View File
@@ -299,6 +299,43 @@ func TestAuthMiddleware_DanglingRoleUnauthorized(t *testing.T) {
}
}
// TestAuthMiddleware_DBErrorIsNotUnauthorized pins OC-0202: a transient DB
// read error while resolving the bearer token (auth.ResolveTokenHash returns
// it WRAPPED, never as a sentinel) must not be reported as 401 UNAUTHORIZED.
// The desktop client treats every 401 as "session expired": it clears auth,
// disconnects the WS, and deletes the stored OS-keyring credential. A DB
// outage is not a bad token, so it must surface as a server-side failure
// (503) instead of tearing down a perfectly valid session.
func TestAuthMiddleware_DBErrorIsNotUnauthorized(t *testing.T) {
database := newAPITestDB(t)
uid, _ := database.CreateUser(context.Background(), "erin", "hash", 4)
token, _ := auth.GenerateToken()
hash := auth.HashToken(token)
_, _ = database.CreateSession(context.Background(), uid, hash, "test", "127.0.0.1")
h := api.AuthMiddleware(database)(http.HandlerFunc(ok))
// Close the underlying DB so the next GetSessionByTokenHash call fails
// with a wrapped "database is closed" error rather than sql.ErrNoRows —
// standing in for a transient outage (locked DB, disk I/O error, a
// restore swapping the file underneath the running server).
if err := database.Close(); err != nil {
t.Fatalf("Close: %v", err)
}
req := withBearer(httptest.NewRequest(http.MethodGet, "/", nil), token)
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
if rr.Code == http.StatusUnauthorized {
t.Errorf("AuthMiddleware DB error status = %d, want non-401 (503)", rr.Code)
}
if rr.Code != http.StatusServiceUnavailable {
t.Errorf("AuthMiddleware DB error status = %d, want 503", rr.Code)
}
}
// ─── RequirePermission tests ──────────────────────────────────────────────────
func TestRequirePermission_Allowed(t *testing.T) {
+131 -84
View File
@@ -31,8 +31,10 @@ type updateProfileRequest struct {
Avatar *string `json:"avatar"`
IdentityPublicKey *string `json:"identity_public_key"`
// DisplayName and About are omitted = unchanged, "" = cleared. Both are
// sanitized and length-checked in UserService, which is also the path a
// non-REST caller would take.
// length-checked in UserService, which is also the path a non-REST
// caller would take; DisplayName is additionally sanitized in this
// handler (before validateDisplayName runs — see the OC-0197 comment at
// the call site) and UserService's own sanitize of it is then a no-op.
DisplayName *string `json:"display_name"`
About *string `json:"about"`
}
@@ -158,6 +160,131 @@ var allowedAvatarMIME = map[string]bool{
// ─── Handlers ────────────────────────────────────────────────────────────────
// handleUpdateProfile processes PATCH /api/v1/users/me.
// parseUpdateProfileRequest decodes the PATCH /users/me body and applies the
// bound-then-sanitize-then-validate pass to every field, in the same order the
// register path canonicalizes them. On any failure it writes the error response
// and returns ok=false, and the caller must return without writing anything
// further. Split out of handleUpdateProfile only to keep that handler under the
// funlen limit; the field logic is unchanged.
func parseUpdateProfileRequest(w http.ResponseWriter, r *http.Request) (updateProfileRequest, bool) {
var req updateProfileRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: "malformed request body",
})
return req, false
}
// 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 req, false
}
// 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 &#39;
// and login (which never re-escapes) would look the account up
// under a name that no longer matches. See service.SanitizeText's
// doc comment and the register path (auth_handler.go), which
// already canonicalizes the same way.
req.Username = strings.TrimSpace(service.SanitizeText(req.Username))
if req.Username == "" {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: "username is required",
})
return req, false
}
if err := auth.ValidateUsername(req.Username); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return req, false
}
// OC-0192: bound the raw field before it reaches the fixpoint
// sanitizer below, same reasoning as the username bound above —
// sanitizeToFixpoint's cost is quadratic in input length. Unlike
// username, an oversized avatar was previously only caught *after*
// sanitizing, by validateAvatarURL's maxAvatarURLLen check.
if req.Avatar != nil && len(*req.Avatar) > maxAvatarURLLen*4 {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: "avatar URL is too long",
})
return req, false
}
// 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 "&amp;" and be persisted (and served) broken. Same reasoning as
// the username path above.
if req.Avatar != nil {
trimmed := strings.TrimSpace(service.SanitizeText(*req.Avatar))
if err := validateAvatarURL(trimmed); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return req, false
}
req.Avatar = &trimmed
}
// display_name gets the same username-shaped scrutiny beyond length:
// it is rendered wherever a username is, so control characters and
// bidi overrides are exactly as unwelcome here. Length and the
// empty-clears-it rule still live in UserService, but sanitizing has
// to happen *before* validateDisplayName, not after: OC-0197 found
// that validating the raw JSON string let an HTML-entity-encoded
// control or bidi character (e.g. "&#x202e;") pass this check as
// harmless ASCII, only to be turned into the real character
// afterwards by UserService.UpdateProfile's cleanText call — the
// same sanitize-then-validate order the username path above already
// uses. OC-0192's raw-byte bound applies here too, now that
// sanitizing happens in this handler (UserService.UpdateProfile
// still bounds DisplayName/About the same way before its own
// cleanText calls, for any non-REST caller; cleanText's fixpoint
// output is stable, so that re-sanitize is a no-op here).
if req.DisplayName != nil {
if len(*req.DisplayName) > service.MaxDisplayNameLen*4 {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: "display_name is too long",
})
return req, false
}
trimmed := strings.TrimSpace(service.SanitizeText(*req.DisplayName))
if err := validateDisplayName(trimmed); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return req, false
}
req.DisplayName = &trimmed
}
// Validate the identity key before any write so the request is
// all-or-nothing.
if req.IdentityPublicKey != nil {
trimmed := strings.TrimSpace(*req.IdentityPublicKey)
if err := validateIdentityKey(trimmed); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return req, false
}
req.IdentityPublicKey = &trimmed
}
return req, true
}
func handleUpdateProfile(svc *service.Services, broadcaster ProfileBroadcaster) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
user, ok := r.Context().Value(UserKey).(*db.User)
@@ -168,91 +295,11 @@ func handleUpdateProfile(svc *service.Services, broadcaster ProfileBroadcaster)
return
}
var req updateProfileRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: "malformed request body",
})
req, ok := parseUpdateProfileRequest(w, r)
if !ok {
return
}
// 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 &#39;
// and login (which never re-escapes) would look the account up
// under a name that no longer matches. See service.SanitizeText's
// doc comment and the register path (auth_handler.go), which
// already canonicalizes the same way.
req.Username = strings.TrimSpace(service.SanitizeText(req.Username))
if req.Username == "" {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: "username is required",
})
return
}
if err := auth.ValidateUsername(req.Username); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return
}
// 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 "&amp;" and be persisted (and served) broken. Same reasoning as
// the username path above.
if req.Avatar != nil {
trimmed := strings.TrimSpace(service.SanitizeText(*req.Avatar))
if err := validateAvatarURL(trimmed); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return
}
req.Avatar = &trimmed
}
// display_name gets the same username-shaped scrutiny beyond length:
// it is rendered wherever a username is, so control characters and
// bidi overrides are exactly as unwelcome here. Length, sanitization
// and the empty-clears-it rule live in UserService.
if req.DisplayName != nil {
if err := validateDisplayName(*req.DisplayName); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return
}
}
// Validate the identity key before any write so the request is
// all-or-nothing.
if req.IdentityPublicKey != nil {
trimmed := strings.TrimSpace(*req.IdentityPublicKey)
if err := validateIdentityKey(trimmed); err != nil {
writeJSON(w, http.StatusBadRequest, errorResponse{
Error: "INVALID_INPUT", Message: err.Error(),
})
return
}
req.IdentityPublicKey = &trimmed
}
updated, err := svc.Users.UpdateProfile(r.Context(), user.ID, service.ProfilePatch{
Username: req.Username,
Avatar: req.Avatar,
+75
View File
@@ -201,6 +201,81 @@ func TestUpdateProfile_OversizedUsernameRejectedBeforeSanitizing(t *testing.T) {
}
}
// OC-0192: same story as OC-0151 above, but for the avatar field — the
// service.SanitizeText call in the avatar branch has no byte-length guard at
// all, unlike the username field just above it. validateAvatarURL's
// maxAvatarURLLen check never gets a chance to reject a huge payload cheaply,
// because the fixpoint sanitizer already spent its (quadratic) cost on it
// first. The fix must reject an oversized avatar on a cheap byte-length
// check before sanitizing, so the rejection is near-instant regardless of
// payload size.
func TestUpdateProfile_OversizedAvatarRejectedBeforeSanitizing(t *testing.T) {
database := newAuthTestDB(t)
router := buildProfileRouter(database)
token := profileCreateToken(t, database, "avatarvictim", 4)
// Adversarial nested-entity payload (16 KB) — see service.sanitizeToFixpoint's
// doc comment for why this shape is quadratic to sanitize.
hugeAvatar := "&" + strings.Repeat("amp;", 4000) + "lt;"
start := time.Now()
rr := patchJSON(t, router, "/api/v1/users/me", token, map[string]string{
"username": "avatarvictim",
"avatar": hugeAvatar,
})
elapsed := time.Since(start)
if rr.Code != http.StatusBadRequest {
t.Errorf("UpdateProfile oversized avatar status = %d, want 400; body = %s", rr.Code, rr.Body.String())
}
// See TestUpdateProfile_OversizedUsernameRejectedBeforeSanitizing for the
// rationale behind this bound: a guard that runs before sanitizing
// rejects in well under a millisecond, while the pre-fix code spends over
// 150ms in sanitizeToFixpoint on this payload before validateAvatarURL's
// length check ever runs.
if elapsed > 150*time.Millisecond {
t.Errorf("UpdateProfile oversized avatar took %v, want well under 150ms (raw field must be bounded before sanitizing, not after)", elapsed)
}
}
// OC-0197: display_name is validated (validateDisplayName) against the raw
// JSON string, before the fixpoint sanitizer's outer html.UnescapeString
// ever runs (that happens later, inside UserService.UpdateProfile's
// cleanText call). So an entity-encoded control or bidi character like
// "&#x202e;" sails through validateDisplayName as harmless ASCII, and is only
// turned into the real U+202E RIGHT-TO-LEFT OVERRIDE character afterwards,
// on its way into storage. TestUpdateProfile_RejectsBadDisplayName
// (avatar_handler_test.go) shows the literal character is correctly
// rejected; this is the entity-encoded bypass of that same guard — the fix
// is to sanitize display_name before validating it, the same order the
// username field above already uses.
func TestUpdateProfile_RejectsEntityEncodedBidiOverrideInDisplayName(t *testing.T) {
database := newAuthTestDB(t)
router := buildProfileRouter(database)
token := profileCreateToken(t, database, "dnentity", 4)
rr := patchJSON(t, router, "/api/v1/users/me", token, map[string]string{
"username": "dnentity",
"display_name": "ada&#x202e;gnp.exe",
})
if rr.Code != http.StatusBadRequest {
t.Errorf("entity-encoded bidi override display_name status = %d, want 400; body = %s", rr.Code, rr.Body.String())
}
// Regardless of what the handler answered, the stored row must never end
// up holding a real bidi override character smuggled in via the entity
// encoding — that is the actual harm (it renders wherever the username
// does, in every connected client, once broadcast).
u, err := database.GetUserByUsername(context.Background(), "dnentity")
if err != nil || u == nil {
t.Fatalf("GetUserByUsername: %v, %v", u, err)
}
if u.DisplayName != nil && strings.ContainsRune(*u.DisplayName, '\u202e') {
t.Errorf("stored display_name = %q, contains a real U+202E bidi override smuggled past validateDisplayName via HTML entity", *u.DisplayName)
}
}
// 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
+16
View File
@@ -81,6 +81,22 @@ CREATE TABLE IF NOT EXISTS sessions (
);
CREATE INDEX IF NOT EXISTS idx_sessions_token ON sessions(token);
-- AuthMiddleware falls through to an API-token lookup whenever a bearer
-- token matches no session (auth.ResolveTokenHash), so this table must exist
-- even in upload-only fixtures — otherwise an ordinary "no such session"
-- lookup for a garbage/unknown token hits GetActiveAPIToken and fails with a
-- real "no such table" SQL error instead of the intended not-found sentinel.
CREATE TABLE IF NOT EXISTS api_tokens (
id INTEGER PRIMARY KEY AUTOINCREMENT,
user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
token_hash TEXT NOT NULL UNIQUE,
label TEXT NOT NULL DEFAULT '',
created_at TEXT NOT NULL DEFAULT (datetime('now')),
last_used_at TEXT,
expires_at TEXT,
revoked_at TEXT
);
CREATE TABLE IF NOT EXISTS channels (
id INTEGER PRIMARY KEY AUTOINCREMENT,
name TEXT NOT NULL,
+7 -3
View File
@@ -198,9 +198,13 @@ func deleteAccountAdminGuard(ctx context.Context, tx *sql.Tx, userID int64) erro
args = append(args, userID)
var adminCount int
if err := tx.QueryRowContext(ctx,
fmt.Sprintf(`SELECT COUNT(*) FROM users WHERE role_id IN (%s) AND id != ? AND banned = 0`,
strings.Join(placeholders, ",")),
// notBannedClause is appended outside the Sprintf format string
// (rather than joined into it) because it contains strftime
// verbs like %Y and %H that fmt.Sprintf would otherwise try to
// parse as its own format directives.
query := fmt.Sprintf(`SELECT COUNT(*) FROM users WHERE role_id IN (%s) AND id != ? AND `,
strings.Join(placeholders, ",")) + notBannedClause
if err := tx.QueryRowContext(ctx, query,
args...,
).Scan(&adminCount); err != nil {
return fmt.Errorf("DeleteAccount count admins: %w", err)
+27
View File
@@ -48,6 +48,33 @@ func TestDeleteAccount_AllowedWhenOtherAdminExists(t *testing.T) {
}
}
// TestDeleteAccount_AllowedWhenOtherAdminHasLapsedTempBan locks the guard's
// "is there another usable admin left" count against the same lapsed-ban
// split anonymiseUser and notBannedClause document elsewhere: an admin whose
// temporary ban has expired (banned=1, ban_expires in the past) is fully
// functional per auth.IsEffectivelyBanned, so the raw `banned = 0` filter
// must not make the guard blind to them.
func TestDeleteAccount_AllowedWhenOtherAdminHasLapsedTempBan(t *testing.T) {
database := openMigratedMemory(t)
admin1 := seedUser(t, database, "admin1")
admin2 := seedUser(t, database, "admin2")
setRole(t, database, admin1, 2) // Admin
setRole(t, database, admin2, 2) // Admin
// admin2's temp ban has lapsed: banned stays 1 but ban_expires is in the
// past, so admin2 logs in and administers normally.
if _, err := database.ExecContext(context.Background(),
`UPDATE users SET banned = 1, ban_expires = '2020-01-01 00:00:00' WHERE id = ?`, admin2,
); err != nil {
t.Fatalf("set lapsed temp ban: %v", err)
}
err := database.DeleteAccount(context.Background(), admin1)
if err != nil {
t.Fatalf("DeleteAccount with a lapsed-temp-ban admin present: %v", err)
}
}
func TestDeleteAccount_AdminAllowedWhenOwnerExists(t *testing.T) {
database := openMigratedMemory(t)
ownerID := seedUser(t, database, "owner")
+98 -20
View File
@@ -427,13 +427,22 @@ func runClosePlugins(registry *plugin.Registry) {
// runStartEventPersistence starts the event persister and pruner, returning
// both as (nil, nil) when event persistence is disabled. Extracted from run.
//
// seedHubReplayState runs unconditionally (whenever hub is non-nil), NOT
// gated on cfg.EventPersistence.Enabled: it seeds the hub's seq counter from
// a persisted floor even in ring-buffer-only mode, which is what closes
// OC-0210 — see its doc comment.
func runStartEventPersistence(bgCtx context.Context, log *slog.Logger, cfg *config.Config, hub *ws.Hub, database *db.DB) (*ws.EventPersister, <-chan struct{}) {
if !cfg.EventPersistence.Enabled || hub == nil {
if hub == nil {
return nil, nil
}
seedHubReplayState(bgCtx, hub, database, log)
if !cfg.EventPersistence.Enabled {
return nil, nil
}
persister := ws.NewEventPersister(
database,
4096,
@@ -813,29 +822,65 @@ func loadPinnedCert(path string) []byte {
return block.Bytes
}
// seedHubReplayState restores the hub's monotonic seq counter from the
// persisted MAX(events.seq) so wrapped-payload seqs stay monotonic across
// restarts. Without this, the events table accumulates rows whose payload
// seqs reset to 1 after every restart, breaking the reconnect "events since
// last_seq" contract.
// wsSeqFloorSettingKey is the generic settings-table key (see db.GetSetting /
// db.SetSetting) seedHubSeqFloor persists its reserved floor under.
const wsSeqFloorSettingKey = "ws_seq_floor"
// wsSeqFloorReserve is the block seedHubSeqFloor reserves above the persisted
// floor on every single boot (OC-0210). It only has to exceed the number of
// hub-sequenced broadcasts any one boot could plausibly emit before its own
// next restart — comfortably true at 1e9 for a self-hosted chat server — so
// this leaves an enormous safety margin while uint64's range still allows
// billions of restarts before the floor could ever wrap.
const wsSeqFloorReserve = 1_000_000_000
// seedHubReplayState seeds the hub's monotonic seq counter at startup from
// two independent, composable sources — both go through hub.SeedSeq, which
// only ever moves h.seq forward (CAS-max), so it doesn't matter which of the
// two runs first or whether either is available:
//
// It also forces every client resuming from at or before that restored seq
// onto the full-ready path for this boot. h.seq is persisted and restored
// here, but the paired watermark that tells a resuming client whether a
// channel-visibility change happened since its last_seq
// (visibilityChangeSeq) is in-memory only and always starts at 0 on a fresh
// process — see ws/hub_events.go's mustFullResync. Channel-visibility
// changes made to an offline client (RefreshChannelVisibility,
// revokeUnreadableChannels) are sent as targeted, unsequenced messages that
// are never written to the events table, so replay can never recover them.
// Without the MarkVisibilityChanged call below, a client resuming with
// last_seq at or before the pre-restart max sails straight through
// mustFullResync's zeroed watermark and can silently miss a visibility
// change it should have converged on.
// 1. seedHubSeqFloor (below) reserves and persists a fresh block of seq
// space on every boot, regardless of whether event persistence is
// enabled. This is what closes OC-0210: previously this function did
// nothing at all when event_persistence.enabled is false (the
// documented "ring-buffer-only behaviour", config.go's
// EventPersistenceConfig.Enabled), so every boot's h.seq — and
// therefore its ring buffer's first entries — started back at 0/1. A
// client reconnecting with a last_seq remembered from a PRIOR boot
// could then coincidentally land inside the new boot's own live ring
// window: EventRingBuffer.EventsSinceFiltered has no way to tell that
// watermark apart from a legitimate one from this boot, and would
// silently serve a partial cross-epoch replay as if it were an
// ordinary resume. Seeding a floor far above anything a single boot
// could reach guarantees every previous boot's real seq values now sit
// below the new ring buffer's oldest entry, so a stale last_seq is
// correctly rejected by the pre-existing "afterSeq <= oldestSeq" guard
// in ringbuffer.go and falls through to a full ready instead
// (serve.go's handleReconnect, the `events == nil` branch) — the same
// path any other unrecoverable resume already takes, with no protocol
// change required.
// 2. When event persistence is enabled and the events table has history,
// MAX(events.seq) is exact (not a heuristic reserve) and naturally
// wins if it is the higher of the two. This branch is also what forces
// the paired visibilityChangeSeq watermark forward via
// MarkVisibilityChanged: h.seq is restored here, but the watermark
// that tells a resuming client whether a channel-visibility change
// happened since its last_seq (visibilityChangeSeq) is in-memory only
// and always starts at 0 on a fresh process — see
// ws/hub_events.go's mustFullResync. Channel-visibility changes made to
// an offline client (RefreshChannelVisibility, revokeUnreadableChannels)
// are sent as targeted, unsequenced messages that are never written to
// the events table, so replay can never recover them. Without the
// MarkVisibilityChanged call below, a client resuming with last_seq at
// or before the pre-restart max would sail straight through
// mustFullResync's zeroed watermark and could silently miss a
// visibility change it should have converged on.
func seedHubReplayState(ctx context.Context, hub *ws.Hub, database *db.DB, log *slog.Logger) {
seedHubSeqFloor(ctx, hub, database, log)
maxSeq, seedErr := database.GetMaxEventSeq(ctx)
if seedErr != nil {
log.Warn("event persistence: failed to read MAX(events.seq); starting hub seq from 0", "error", seedErr)
log.Warn("event persistence: failed to read MAX(events.seq); hub seq still advanced from the persisted floor for this boot", "error", seedErr)
return
}
if maxSeq <= 0 {
@@ -846,6 +891,39 @@ func seedHubReplayState(ctx context.Context, hub *ws.Hub, database *db.DB, log *
hub.MarkVisibilityChanged()
}
// seedHubSeqFloor reserves and persists a fresh block of the hub's sequence
// space on every boot, independent of event persistence (OC-0210) — see
// seedHubReplayState's doc for why this is what actually closes the bug. A
// read or write failure against the settings table is logged and skipped
// rather than fatal: it leaves this one boot with the pre-fix exposure
// (plain Phase A ring-buffer behaviour) instead of blocking startup over a
// heuristic safety net.
func seedHubSeqFloor(ctx context.Context, hub *ws.Hub, database *db.DB, log *slog.Logger) {
var floor uint64
raw, err := database.GetSetting(ctx, wsSeqFloorSettingKey)
switch {
case err == nil:
parsed, perr := strconv.ParseUint(raw, 10, 64)
if perr != nil {
log.Warn("event persistence: stored ws seq floor is not a valid uint64, resetting to 0", "value", raw, "error", perr)
break
}
floor = parsed
case errors.Is(err, db.ErrNotFound):
// No prior boot has ever reserved a floor — start from 0.
default:
log.Warn("event persistence: failed to read persisted ws seq floor; hub seq not advanced this boot", "error", err)
return
}
newFloor := floor + wsSeqFloorReserve
if err := database.SetSetting(ctx, wsSeqFloorSettingKey, strconv.FormatUint(newFloor, 10)); err != nil {
log.Warn("event persistence: failed to persist advanced ws seq floor; hub seq not advanced this boot", "error", err)
return
}
hub.SeedSeq(newFloor)
}
// printBanner writes the startup banner to stderr (so it doesn't mix with
// the structured log output on stdout).
func printBanner(cfg *config.Config, ver string, tls bool) {
+171
View File
@@ -16,6 +16,7 @@ import (
"github.com/owncord/server/admin"
"github.com/owncord/server/auth"
"github.com/owncord/server/config"
"github.com/owncord/server/db"
"github.com/owncord/server/ws"
)
@@ -166,3 +167,173 @@ func TestSeedHubReplayState_ForcesFullResyncForOfflineClient(t *testing.T) {
bufTier, dbTier, fullTier)
}
}
// waitForFirstRingBufferEntry polls hub's ring buffer until it holds at
// least one entry and returns that entry's seq, or fails the test after a
// timeout. BroadcastToAll enqueues onto the hub's dispatch channel and
// returns before a seq is actually assigned, so tests that need to know a
// real assigned seq must synchronize on this instead of assuming one.
func waitForFirstRingBufferEntry(t *testing.T, hub *ws.Hub) uint64 {
t.Helper()
deadline := time.Now().Add(5 * time.Second)
for {
if oldest := hub.ReplayBuffer().OldestSeq(); oldest != 0 {
return oldest
}
if time.Now().After(deadline) {
t.Fatalf("timed out waiting for the first ring buffer entry to land")
}
time.Sleep(time.Millisecond)
}
}
// waitForRingBufferNewestAtLeast polls hub's ring buffer until its newest
// entry's seq is >= target, or fails the test after a timeout. See
// waitForFirstRingBufferEntry on why this can't be a fixed sleep.
func waitForRingBufferNewestAtLeast(t *testing.T, hub *ws.Hub, target uint64) {
t.Helper()
deadline := time.Now().Add(5 * time.Second)
for {
if newest := hub.ReplayBuffer().NewestSeq(); newest >= target {
return
}
if time.Now().After(deadline) {
t.Fatalf("timed out waiting for hub ring buffer newest seq to reach >= %d (currently %d)",
target, hub.ReplayBuffer().NewestSeq())
}
time.Sleep(time.Millisecond)
}
}
// TestRunStartEventPersistence_DisabledMode_StaleLastSeqForcesFullResync pins
// OC-0210: with event_persistence.enabled=false ("ring-buffer-only
// behaviour", config.go's EventPersistenceConfig.Enabled doc), every boot's
// h.seq previously started at 0 with an empty ring buffer, with nothing to
// distinguish this boot's own watermarks from a PRIOR boot's. A reconnecting
// client carrying a last_seq from a prior process's epoch was checked only
// against whatever the new epoch's ring buffer happened to hold; if the new
// epoch's traffic (e.g. other clients reconnecting first) had pushed seq past
// that stale value, EventsSinceFiltered reported it as an ordinary in-window
// replay instead of refusing it, silently handing back a different epoch's
// events as if they were a contiguous resume.
//
// This simulates exactly the repro: hub "A" (a prior boot) runs with
// persistence disabled and a client observes 40 broadcasts go by (its
// last_seq is whatever the 40th one's seq turns out to be — captured
// dynamically here rather than hardcoded, since the fix changes what that
// number actually is). Hub A is then stopped (restart) and a fresh hub "B" is
// booted with the same disabled config; other clients' traffic pushes hub B's
// own (unrelated) epoch's seq past that same watermark, then the client
// reconnects against hub B with its old last_seq — a watermark that has never
// existed in hub B's epoch. That resume must be forced onto the full-ready
// path; before the fix it silently resolves via the ordinary buffer tier
// instead.
func TestRunStartEventPersistence_DisabledMode_StaleLastSeqForcesFullResync(t *testing.T) {
database, err := db.Open(":memory:")
if err != nil {
t.Fatalf("db.Open: %v", err)
}
defer database.Close() //nolint:errcheck
if err := db.Migrate(database); err != nil {
t.Fatalf("db.Migrate: %v", err)
}
ctx := context.Background()
log := slog.New(slog.NewTextHandler(io.Discard, nil))
cfg := &config.Config{EventPersistence: config.EventPersistenceConfig{Enabled: false}}
userID, err := database.CreateUser(ctx, "oc-0210-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(ctx, userID, auth.HashToken(token), "test", "127.0.0.1"); err != nil {
t.Fatalf("CreateSession: %v", err)
}
limiter := auth.NewRateLimiter()
// --- Prior boot: hub A runs with persistence disabled. A client's
// last_seq ends up as whatever the 40th broadcast's real seq turns out to
// be — captured dynamically so this test holds regardless of what value
// scheme is in effect (raw 1..N pre-fix, or a seeded floor post-fix). ---
hubOld := ws.NewHub(database, limiter, nil)
go hubOld.Run()
if persister, prunerDone := runStartEventPersistence(ctx, log, cfg, hubOld, database); persister != nil || prunerDone != nil {
t.Fatalf("runStartEventPersistence with Enabled=false: want (nil, nil), got (%v, %v)", persister, prunerDone)
}
for range 40 {
hubOld.BroadcastToAll([]byte(`{"type":"broadcast"}`))
}
oldFirstSeq := waitForFirstRingBufferEntry(t, hubOld)
staleLastSeq := oldFirstSeq + 39 // the 40th broadcast's seq: what a real client's lastSeq tracker would hold
waitForRingBufferNewestAtLeast(t, hubOld, staleLastSeq)
hubOld.Stop()
// --- Restart: hub B is a brand-new process-equivalent hub, same disabled
// config, same (in-memory but never touched by persistence) database. ---
hubNew := ws.NewHub(database, limiter, nil)
go hubNew.Run()
defer hubNew.Stop()
if persister, prunerDone := runStartEventPersistence(ctx, log, cfg, hubNew, database); persister != nil || prunerDone != nil {
t.Fatalf("runStartEventPersistence with Enabled=false: want (nil, nil), got (%v, %v)", persister, prunerDone)
}
// Other clients reconnect first and push hub B's own new epoch forward by
// 60 broadcasts — enough to overtake staleLastSeq pre-fix (repro's 1..60
// window covering 40) and trivially so post-fix (the seeded floor alone
// already exceeds it).
for range 60 {
hubNew.BroadcastToAll([]byte(`{"type":"broadcast"}`))
}
newFirstSeq := waitForFirstRingBufferEntry(t, hubNew)
newTargetSeq := newFirstSeq + 59
waitForRingBufferNewestAtLeast(t, hubNew, newTargetSeq)
if newTargetSeq <= staleLastSeq {
t.Fatalf("test setup invariant broken: hub B's epoch (reached %d) never overtook the stale watermark (%d)", newTargetSeq, staleLastSeq)
}
handler := ws.ServeWS(hubNew, database, []string{"*"}, 0)
srv := httptest.NewServer(handler)
defer srv.Close()
wsURL := "ws" + strings.TrimPrefix(srv.URL, "http")
dialCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
defer cancel()
conn, dialResp, dialErr := websocket.Dial(dialCtx, wsURL, nil)
if dialResp != nil && dialResp.Body != nil {
_ = dialResp.Body.Close()
}
if dialErr != nil {
t.Fatalf("websocket.Dial: %v", dialErr)
}
defer func() { _ = conn.Close(websocket.StatusNormalClosure, "") }()
// staleLastSeq is a watermark from hub A's epoch. It sits inside hub B's
// own live ring window, but nothing about it describes hub B's history —
// it must not be served by ordinary replay.
authMsg := map[string]any{
"type": "auth",
"payload": map[string]any{
"token": token,
"last_seq": staleLastSeq,
},
}
raw, _ := json.Marshal(authMsg)
if err := conn.Write(dialCtx, websocket.MessageText, raw); err != nil {
t.Fatalf("write auth: %v", err)
}
if _, _, err := conn.Read(dialCtx); err != nil {
t.Fatalf("read handshake response: %v", err)
}
bufTier, dbTier, fullTier := hubNew.ReconnectTierStats()
if fullTier != 1 {
t.Fatalf("reconnect tiers (buffer=%d db=%d full=%d): want full=1 — a last_seq from a prior epoch must never be served by ring-buffer replay in ring-buffer-only mode, since the server has no way to tell it apart from an in-epoch watermark",
bufTier, dbTier, fullTier)
}
}
+8 -4
View File
@@ -5,7 +5,6 @@ import (
"fmt"
"log/slog"
"time"
"unicode/utf8"
"github.com/owncord/server/auth"
"github.com/owncord/server/db"
@@ -179,9 +178,14 @@ func (s *ChannelService) HandlePresenceUpdate(ctx context.Context, userID int64,
var cleaned *string
if customStatus != nil {
text := cleanText(*customStatus)
if utf8.RuneCountInString(text) > MaxCustomStatusLen {
return nil, fmt.Errorf("%w: custom_status must be at most %d characters", ErrBadRequest, MaxCustomStatusLen)
// OC-0195: bound the raw bytes before cleanText (sanitizeToFixpoint)
// runs — see cleanTextBounded's doc comment (user.go). This path is
// reachable over the WS presence_update frame, whose read limit is
// config.MaxMessageBytes (1 MiB), far larger than any REST body that
// reaches the equivalent guard on SetCustomStatus/UpdateProfile.
text, err := cleanTextBounded(*customStatus, MaxCustomStatusLen, "custom_status")
if err != nil {
return nil, err
}
cleaned = nullable(text)
}
+10 -7
View File
@@ -5,7 +5,6 @@ import (
"fmt"
"log/slog"
"time"
"unicode/utf8"
"github.com/owncord/server/auth"
"github.com/owncord/server/db"
@@ -231,9 +230,11 @@ func (s *DMService) CreateGroupDM(ctx context.Context, userID int64, recipientID
return nil, fmt.Errorf("%w: a group DM holds at most %d users", ErrBadRequest, db.MaxGroupDMParticipants)
}
cleanName := cleanText(name)
if utf8.RuneCountInString(cleanName) > MaxGroupDMNameLen {
return nil, fmt.Errorf("%w: name must be at most %d characters", ErrBadRequest, MaxGroupDMNameLen)
// OC-0195 sibling: bound the raw bytes before cleanText (sanitizeToFixpoint)
// runs — see cleanTextBounded's doc comment (user.go).
cleanName, err := cleanTextBounded(name, MaxGroupDMNameLen, "name")
if err != nil {
return nil, err
}
for _, rid := range unique {
@@ -320,9 +321,11 @@ func (s *DMService) RenameGroupDM(ctx context.Context, userID, channelID int64,
return nil, fmt.Errorf("%w: only group DMs can be named", ErrBadRequest)
}
cleanName := cleanText(name)
if utf8.RuneCountInString(cleanName) > MaxGroupDMNameLen {
return nil, fmt.Errorf("%w: name must be at most %d characters", ErrBadRequest, MaxGroupDMNameLen)
// OC-0195 sibling: bound the raw bytes before cleanText (sanitizeToFixpoint)
// runs — see cleanTextBounded's doc comment (user.go).
cleanName, err := cleanTextBounded(name, MaxGroupDMNameLen, "name")
if err != nil {
return nil, err
}
if err := s.st.SetDMChannelName(ctx, channelID, cleanName); err != nil {
+65
View File
@@ -3,7 +3,9 @@ package service
import (
"context"
"errors"
"strings"
"testing"
"time"
"github.com/owncord/server/db"
)
@@ -62,6 +64,69 @@ func TestDMService_CreateGroupDM_RefusesBannedRecipient(t *testing.T) {
}
}
// OC-0194: same defect as OC-0192/OC-0195 (see
// TestUpdateProfile_OversizedDisplayNameAndAboutRejectedBeforeSanitizing and
// TestHandlePresenceUpdate_OversizedCustomStatusRejectedBeforeSanitizing) but
// reached via CreateGroupDM. /api/v1/dms carries no rate limiter, and
// CreateGroupDM runs cleanText(name) *before* the recipient-existence/ban/
// block checks, so an adversarial nested-entity name pays the full quadratic
// sanitizeToFixpoint cost even for a request that is going to 404 on its
// recipients. The raw-byte guard must reject on cheap byte length alone.
func TestDMService_CreateGroupDM_OversizedNameRejectedBeforeSanitizing(t *testing.T) {
database := newTestDB(t)
seedUser(t, database, &db.User{ID: 1, Username: "alice"})
svc := NewDMService(database)
// Adversarial nested-entity payload (16 KB) — see sanitizeToFixpoint's
// doc comment (message.go) for why this shape is quadratic to sanitize.
huge := "&" + strings.Repeat("amp;", 4000) + "lt;"
start := time.Now()
// Recipients 999998/999999 do not exist. The guard must fire before the
// per-recipient GetUserByID/ban checks reach the database, matching the
// order CreateGroupDM actually runs them in.
_, err := svc.CreateGroupDM(context.Background(), 1, []int64{999998, 999999}, huge)
elapsed := time.Since(start)
if !errors.Is(err, ErrBadRequest) {
t.Errorf("CreateGroupDM with oversized name err = %v, want ErrBadRequest", err)
}
// A guard that runs before sanitizing rejects in well under a
// millisecond; the pre-fix code spends well over 150ms in
// sanitizeToFixpoint on this payload before the rune-count check ever
// runs. 150ms gives generous margin over noise while staying far below
// the unguarded cost.
if elapsed > 150*time.Millisecond {
t.Errorf("CreateGroupDM with oversized name took %v, want well under 150ms (raw field must be bounded before sanitizing)", elapsed)
}
}
// OC-0194 sibling: RenameGroupDM runs the identical cleanText(name) call and
// must be bounded the same way as CreateGroupDM.
func TestDMService_RenameGroupDM_OversizedNameRejectedBeforeSanitizing(t *testing.T) {
database := newTestDB(t)
seedUser(t, database, &db.User{ID: 1, Username: "alice"})
seedUser(t, database, &db.User{ID: 2, Username: "bob"})
seedUser(t, database, &db.User{ID: 3, Username: "carol"})
svc := NewDMService(database)
created, err := svc.CreateGroupDM(context.Background(), 1, []int64{2, 3}, "")
if err != nil {
t.Fatalf("setup CreateGroupDM: %v", err)
}
huge := "&" + strings.Repeat("amp;", 4000) + "lt;"
start := time.Now()
_, err = svc.RenameGroupDM(context.Background(), 1, created.Channel.ID, huge)
elapsed := time.Since(start)
if !errors.Is(err, ErrBadRequest) {
t.Errorf("RenameGroupDM with oversized name err = %v, want ErrBadRequest", err)
}
if elapsed > 150*time.Millisecond {
t.Errorf("RenameGroupDM with oversized name took %v, want well under 150ms (raw field must be bounded before sanitizing)", elapsed)
}
}
// cancelAfterCreateGroupDMStore wraps a real *db.DB and cancels a context the
// instant CreateGroupDMChannel returns successfully — simulating a client
// disconnect that lands exactly in the gap between the channel's commit and
+10 -1
View File
@@ -176,7 +176,16 @@ func (s *MessageService) applyMentionCounts(ctx context.Context, channelID, msgI
// here and a literal comparison would ping them with @here — the one
// thing "appear offline" is meant to stop. Collapsing first makes
// @here agree with what everyone else can see of that reader.
if set.HereOnly && db.BroadcastStatus(r.Status) == db.StatusOffline {
//
// That column check alone is not enough: users.status keeps a
// *chosen* idle/dnd across a disconnect by design
// (MarkUserDisconnected only ever rewrites "online" -> "offline"),
// so a signed-out reader whose last status was idle/dnd would still
// read as non-offline here. s.online (nil-safe) applies the read
// path's "no live connection is offline, whatever the row says"
// rule (ws/serve_ready.go presentableMembers) to close that gap.
if set.HereOnly && (db.BroadcastStatus(r.Status) == db.StatusOffline ||
(s.online != nil && !s.online(r.UserID))) {
continue
}
recipients[r.UserID] = struct{}{}
+31
View File
@@ -301,6 +301,37 @@ func TestSendMessage_HereSkipsInvisibleUsers(t *testing.T) {
}
}
// TestSendMessage_HereSkipsDisconnectedIdleDndUsers locks OC-0223: @here must
// treat a reader with no live connection as offline even when their stored
// status is idle/dnd, matching the read path's "no live connection is
// offline, whatever the row says" rule (ws/serve_ready.go presentableMembers).
// MarkUserDisconnected only ever rewrites "online" -> "offline" — an idle/dnd
// choice survives the disconnect by design, so a bare
// db.BroadcastStatus(r.Status) == db.StatusOffline test can never catch a
// disconnected idle/dnd reader without also consulting live connection state.
func TestSendMessage_HereSkipsDisconnectedIdleDndUsers(t *testing.T) {
svc, _, database := newMentionFixture(t)
// bob's last chosen status was "dnd" before disconnecting (mirrors what
// MarkUserDisconnected leaves behind for a non-"online" status).
if err := database.UpdateUserStatus(context.Background(), 2, db.StatusDND); err != nil {
t.Fatalf("UpdateUserStatus(dnd): %v", err)
}
// bob has no live connection.
svc.SetOnlineChecker(func(userID int64) bool { return userID != 2 })
sendAs(t, svc, 4, "@here quick question")
if got := mentionCount(t, database, 2); got != 0 {
t.Errorf("disconnected dnd bob mention_count = %d, want 0", got)
}
// A plain @everyone still reaches them: only @here narrows on presence.
sendAs(t, svc, 4, "@everyone meeting now")
if got := mentionCount(t, database, 2); got != 1 {
t.Errorf("disconnected dnd bob @everyone mention_count = %d, want 1", got)
}
}
// TestSendMessage_EveryoneSkipsUsersWithoutRead locks that the @everyone
// fan-out honors per-channel denies, not just the base role mask.
func TestSendMessage_EveryoneSkipsUsersWithoutRead(t *testing.T) {
+20
View File
@@ -133,6 +133,26 @@ type MessageService struct {
// tests swap it for an inline runner via RunBackgroundInlineForTest so they
// can read the counts deterministically right after a send.
bg func(fn func())
// online reports whether userID currently holds a live connection. It is
// wired by the ws layer (Hub.IsUserConnected) after both are constructed,
// so @here can apply the same "no live connection is offline, whatever the
// row stores" rule the read path uses (ws/serve_ready.go
// presentableMembers) instead of trusting users.status alone — that column
// keeps a *chosen* idle/dnd/invisible across a disconnect by design
// (MarkUserDisconnected only ever rewrites "online" -> "offline"), so a
// disconnected idle/dnd reader would otherwise still collect an @here
// badge. nil (the zero value, e.g. in tests and any caller with no hub)
// means "no live-connection information available" and applies no extra
// narrowing, preserving prior behavior.
online func(userID int64) bool
}
// SetOnlineChecker wires the live-connection predicate @here's offline
// narrowing consults in addition to users.status. Passing nil clears it. Safe
// to call once at startup (the ws layer, after constructing both the Hub and
// the Services) or from a test.
func (s *MessageService) SetOnlineChecker(online func(userID int64) bool) {
s.online = online
}
// NewMessageService creates a MessageService.
+81
View File
@@ -209,6 +209,49 @@ func TestUpdateProfile_RejectsOverlongFields(t *testing.T) {
}
}
// OC-0192: UpdateProfile is the one function every transport (the REST
// handler, and any future non-REST caller — see ProfilePatch's doc comment)
// goes through, so the raw-length bound belongs here, not only in the
// handler. cleanText (sanitizeToFixpoint) is quadratic in input length, and
// nothing bounds DisplayName/About before line 140/143 run it — the rune-
// count checks there run cleanText's full (expensive) output before ever
// looking at how long it is. A caller that hands UpdateProfile an
// adversarial nested-entity payload must be rejected on a cheap byte-length
// check, not after the fixpoint sanitizer has already paid its cost on it.
func TestUpdateProfile_OversizedDisplayNameAndAboutRejectedBeforeSanitizing(t *testing.T) {
svc, _ := newUserSvc(t)
ctx := context.Background()
// Adversarial nested-entity payload (16 KB) — see sanitizeToFixpoint's
// doc comment (message.go) for why this shape is quadratic to sanitize.
huge := "&" + strings.Repeat("amp;", 4000) + "lt;"
start := time.Now()
_, err := svc.UpdateProfile(ctx, 1, ProfilePatch{Username: "ada", DisplayName: &huge})
elapsed := time.Since(start)
if !errors.Is(err, ErrBadRequest) {
t.Errorf("oversized display_name err = %v, want ErrBadRequest", err)
}
// A guard that runs before sanitizing rejects in well under a
// millisecond; the pre-fix code spends well over 150ms in
// sanitizeToFixpoint on this payload before the rune-count check ever
// runs. 150ms gives generous margin over noise while staying far below
// the unguarded cost.
if elapsed > 150*time.Millisecond {
t.Errorf("oversized display_name took %v, want well under 150ms (raw field must be bounded before sanitizing)", elapsed)
}
start = time.Now()
_, err = svc.UpdateProfile(ctx, 1, ProfilePatch{Username: "ada", About: &huge})
elapsed = time.Since(start)
if !errors.Is(err, ErrBadRequest) {
t.Errorf("oversized about err = %v, want ErrBadRequest", err)
}
if elapsed > 150*time.Millisecond {
t.Errorf("oversized about took %v, want well under 150ms (raw field must be bounded before sanitizing)", elapsed)
}
}
func TestSetCustomStatus_RoundTripClearAndBound(t *testing.T) {
svc, database := newUserSvc(t)
ctx := context.Background()
@@ -293,6 +336,44 @@ func TestHandlePresenceUpdate_AcceptsInvisibleAndCarriesCustomStatus(t *testing.
}
}
// OC-0195: same defect as OC-0192 (TestUpdateProfile_OversizedDisplayNameAndAboutRejectedBeforeSanitizing)
// but reached over presence_update instead of PATCH /users/me. HandlePresenceUpdate
// applied MaxCustomStatusLen to cleanText's *output*, so an adversarial
// nested-entity payload paid the full quadratic sanitizeToFixpoint cost before
// ever being measured. The WS read limit (config.MaxMessageBytes, 1 MiB) admits
// a payload here far larger than PATCH /users/me's body ever could, and this
// runs on the connection's own readPump goroutine.
func TestHandlePresenceUpdate_OversizedCustomStatusRejectedBeforeSanitizing(t *testing.T) {
database := newTestDB(t)
seedUser(t, database, &db.User{ID: 1, Username: "ada", PasswordHash: "h"})
svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database)))
ctx := context.Background()
// Adversarial nested-entity payload (16 KB) — see sanitizeToFixpoint's
// doc comment (message.go) for why this shape is quadratic to sanitize.
huge := "&" + strings.Repeat("amp;", 4000) + "lt;"
start := time.Now()
_, err := svc.HandlePresenceUpdate(ctx, 1, db.StatusOnline, &huge, nil)
elapsed := time.Since(start)
if !errors.Is(err, ErrBadRequest) {
t.Errorf("oversized custom_status err = %v, want ErrBadRequest", err)
}
// A guard that runs before sanitizing rejects in well under a
// millisecond; the pre-fix code spends well over 150ms in
// sanitizeToFixpoint on this payload before the rune-count check ever
// runs. 150ms gives generous margin over noise while staying far below
// the unguarded cost.
if elapsed > 150*time.Millisecond {
t.Errorf("oversized custom_status took %v, want well under 150ms (raw field must be bounded before sanitizing)", elapsed)
}
// The rejected call must not have committed the status either.
u, _ := database.GetUserByID(ctx, 1)
if u.Status == db.StatusOnline {
t.Error("a rejected presence_update must not commit the status")
}
}
func TestHandlePresenceUpdate_RejectsUnknownStatusAndOverlongText(t *testing.T) {
database := newTestDB(t)
seedUser(t, database, &db.User{ID: 1, Username: "ada", PasswordHash: "h"})
+47 -3
View File
@@ -114,6 +114,33 @@ func cleanText(v string) string {
return strings.TrimSpace(sanitizeToFixpoint(v))
}
// cleanTextBounded is cleanText plus the raw-byte guard OC-0192 established
// for UpdateProfile's DisplayName/About fields, generalized for every other
// free-text field that runs through cleanText: SetCustomStatus,
// HandlePresenceUpdate's custom_status, and group DM names (OC-0195).
//
// cleanText's sanitizeToFixpoint pass is quadratic in input length, so a
// bound applied only to its *output* (a plain rune-count check on the
// cleaned string) still lets an adversarial nested-entity payload pay the
// full sanitize cost first — it can even sanitize down to something well
// under maxRunes and be silently accepted, having spent seconds of CPU to
// get there. The byte-length pre-check runs before cleanText ever does, on
// the untouched input, so the cost of rejecting an oversized value is
// O(len(v)) instead of the sanitizer's cost. *4 is deliberately looser than
// maxRunes — it exists only to keep the sanitizer from ever seeing a
// pathological payload, not to duplicate the real (rune-count) bound, which
// still runs afterward on the cleaned, trimmed value.
func cleanTextBounded(v string, maxRunes int, fieldName string) (string, error) {
if len(v) > maxRunes*4 {
return "", fmt.Errorf("%w: %s must be at most %d characters", ErrBadRequest, fieldName, maxRunes)
}
cleaned := cleanText(v)
if utf8.RuneCountInString(cleaned) > maxRunes {
return "", fmt.Errorf("%w: %s must be at most %d characters", ErrBadRequest, fieldName, maxRunes)
}
return cleaned, nil
}
// resolveOptional picks the column value for one nullable text field: the
// sanitized patch when it was supplied, the existing row otherwise.
func resolveOptional(patch *string, existing *string) *string {
@@ -137,6 +164,23 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro
span.End()
}()
// OC-0192: bound the raw bytes before either reaches cleanText
// (sanitizeToFixpoint) below — its cost is quadratic in input length,
// and an adversarial nested-entity payload can sanitize down to
// something well under the rune-count bound while still costing seconds
// of CPU to get there, so the rune-count check alone never rejects it
// early. This is the same cheap byte-length pre-check the handler uses
// for username/avatar (profile_handler.go); *4 still admits any
// legitimate UTF-8 value at the rune bound. UpdateProfile is the one
// function every transport reaches (see ProfilePatch's doc comment), so
// the guard belongs here rather than only in the REST handler.
if patch.DisplayName != nil && len(*patch.DisplayName) > MaxDisplayNameLen*4 {
return nil, fmt.Errorf("%w: display_name must be at most %d characters", ErrBadRequest, MaxDisplayNameLen)
}
if patch.About != nil && len(*patch.About) > MaxAboutLen*4 {
return nil, fmt.Errorf("%w: about must be at most %d characters", ErrBadRequest, MaxAboutLen)
}
if patch.DisplayName != nil && utf8.RuneCountInString(cleanText(*patch.DisplayName)) > MaxDisplayNameLen {
return nil, fmt.Errorf("%w: display_name must be at most %d characters", ErrBadRequest, MaxDisplayNameLen)
}
@@ -199,9 +243,9 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro
// value persists across reconnects and is cleared explicitly on logout, which
// is why it is stored rather than held on the connection.
func (s *UserService) SetCustomStatus(ctx context.Context, userID int64, text string) error {
cleaned := cleanText(text)
if utf8.RuneCountInString(cleaned) > MaxCustomStatusLen {
return fmt.Errorf("%w: custom_status must be at most %d characters", ErrBadRequest, MaxCustomStatusLen)
cleaned, err := cleanTextBounded(text, MaxCustomStatusLen, "custom_status")
if err != nil {
return err
}
if err := s.st.UpdateUserCustomStatus(ctx, userID, nullable(cleaned)); err != nil {
return fmt.Errorf("%w: failed to update custom status: %v", ErrInternal, err)
+12 -1
View File
@@ -119,7 +119,18 @@ func (h *Hub) handleMessageSessionRecheck(c *Client) bool {
if shouldCheck && c.tokenHash != "" {
result, dbErr := h.db.GetSessionWithBanStatus(c.ctx, c.tokenHash)
if dbErr != nil || result == nil || auth.IsSessionExpired(result.ExpiresAt) {
if dbErr != nil {
// A failed read says nothing about this session's validity —
// kicking the client on a transient DB error (SQLITE_BUSY, an
// I/O error, a maintenance window) would be a false positive.
// Skip this recheck; the next one retries, and
// sweepRevokedSessions remains the time-based backstop for
// idle connections. Matches sweepRevokedSessions's identical
// rule for a failed batch lookup (hub_sweep.go).
slog.Warn("ws session recheck: lookup failed, skipping", "user_id", c.userID, "err", dbErr)
return false
}
if result == nil || auth.IsSessionExpired(result.ExpiresAt) {
slog.Info("ws session expired, closing connection", "user_id", c.userID)
h.kickClient(c)
return true
+9 -1
View File
@@ -3,6 +3,7 @@ package ws
import (
"context"
"errors"
"log/slog"
"github.com/owncord/server/db"
"github.com/owncord/server/service"
@@ -182,6 +183,13 @@ func serviceErrorToResult(err error) Result {
case errors.Is(err, service.ErrConflict):
return Result{Error: ClientError{Code: ErrCodeConflict, Message: err.Error()}}
default:
return Result{Error: ClientError{Code: ErrCodeInternal, Message: err.Error()}}
// Internal errors (service.ErrInternal wrappers embed the underlying
// driver error via %v) must not reach the client verbatim, matching
// the REST twin writeServiceError (Server/api/channel_handler.go) and
// every other ErrCodeInternal site in this package. Log server-side
// since this is the only ErrCodeInternal path whose caller
// (handlers.go) skips its own logging for ClientError results.
slog.Error("ws service internal error", "err", err)
return Result{Error: ClientError{Code: ErrCodeInternal, Message: "internal error"}}
}
}
+5
View File
@@ -179,6 +179,11 @@ func NewHub(database *db.DB, limiter *auth.RateLimiter, svc *service.Services) *
callDeps.DMSvc = svc.DMs
h.messageSvc = svc.Messages
h.perms = svc.Permissions
// So @here's offline narrowing can tell a disconnected idle/dnd reader
// (users.status keeps their last *chosen* value across a disconnect)
// from one who is actually still connected — the same live-connection
// rule presentableMembers applies to the members array.
svc.Messages.SetOnlineChecker(h.IsUserConnected)
}
registerChatHandlers(reg, chatDeps)
@@ -0,0 +1,61 @@
package ws
// Internal test for OC-0211: handleMessageSessionRecheck must not treat a
// transient DB error the same as a genuinely revoked/expired session. The
// sibling sweep in hub_sweep.go (sweepRevokedSessions) already documents and
// implements the correct rule for the identical failure: "a failed batch
// lookup says nothing about any individual session — kicking everyone on a
// transient DB error would be a mass disconnect. Skip this sweep; the next
// tick retries." handleMessageSessionRecheck disagreed, kicking the client on
// dbErr != nil exactly like a deleted/expired session.
import (
"context"
"testing"
"github.com/owncord/server/auth"
)
func TestHandleMessageSessionRecheck_TransientDBErrorDoesNotKick(t *testing.T) {
database := newHarvestVoiceDB(t)
uid := seedHarvestVoiceUser(t, database, "recheck-dberr")
tokenHash := "tok-recheck-dberr"
if _, err := database.CreateSession(context.Background(), uid, tokenHash, "test-device", "127.0.0.1"); err != nil {
t.Fatalf("CreateSession: %v", err)
}
h := NewHub(database, auth.NewRateLimiter(), nil)
c := NewTestClient(h, uid, make(chan []byte, 8))
c.tokenHash = tokenHash
// Put the client one message away from the periodic recheck boundary, so
// the very next call to handleMessageSessionRecheck triggers the DB read.
c.msgCount = SessionCheckInterval - 1
h.clients[uid] = c
// Force GetSessionWithBanStatus to fail with a genuine DB error (not
// sql.ErrNoRows, which the production code already treats as "session
// gone" — that path is not in question here). Closing the underlying
// connection pool reproduces the same dbErr != nil branch a transient
// SQLITE_BUSY, I/O error, or maintenance window would.
if err := database.Close(); err != nil {
t.Fatalf("database.Close: %v", err)
}
closed := h.handleMessageSessionRecheck(c)
if closed {
t.Fatalf("handleMessageSessionRecheck reported the connection closed on a transient DB lookup error; " +
"a failed read is not evidence the session is invalid (compare sweepRevokedSessions, which skips on the same failure)")
}
h.mu.RLock()
_, stillConnected := h.clients[uid]
h.mu.RUnlock()
if !stillConnected {
t.Fatalf("client was removed from h.clients on a transient DB lookup error during session recheck")
}
if c.isSendClosed() {
t.Fatalf("client's send channels were closed on a transient DB lookup error during session recheck")
}
}
@@ -0,0 +1,100 @@
package ws
// oc_0219_voice_join_rollback_unsubscribe_test.go — regression test for
// finding OC-0219.
//
// rollbackVoiceJoin clears the client's voice channel ID but never drops its
// VoiceTopic subscription. voiceJoinComplete subscribes the joiner to
// VoiceTopic(channelID) (voice_join.go) BEFORE it reads back the channel's
// existing participants via GetChannelVoiceStates; when that read fails, the
// handler calls rollbackVoiceJoin to undo the join. Every other path that
// takes a client out of voice while its WS stays up (clearVoiceAndUnsubscribe
// in voice_leave.go, and its callers) also drops the VoiceTopic subscription
// — rollbackVoiceJoin is the only one that does not. A socket left subscribed
// after a failed join keeps receiving that room's voice_e2ee_announce relays
// (which carry no channel_id to filter on) for the rest of the connection,
// polluting whatever voice session the client joins next.
//
// This reuses voiceJoinPostTokenRaceHook (test-only plumbing shared with
// OC-0008 and OC-0172) to fault-inject a GetChannelVoiceStates failure inside
// voiceJoinComplete, landing strictly after h.pubsub.Subscribe has already
// run for this join.
import (
"context"
"encoding/json"
"testing"
"time"
"github.com/owncord/server/auth"
"github.com/owncord/server/config"
"github.com/owncord/server/db"
)
// TestVoiceJoin_GetChannelVoiceStatesError_UnsubscribesVoiceTopic pins
// OC-0219: rollbackVoiceJoin must drop the client's VoiceTopic subscription,
// not just its in-memory voiceChID, so a socket that failed mid-join stops
// receiving that room's E2EE relays.
func TestVoiceJoin_GetChannelVoiceStatesError_UnsubscribesVoiceTopic(t *testing.T) {
database := newHarvestVoiceDB(t)
uid := seedHarvestVoiceUser(t, database, "join-0219-victim")
chID := mustCreateVoiceChannel(t, database, "voice-join-0219")
lk, err := NewLiveKitClient(&config.VoiceConfig{
LiveKitAPIKey: "test-api-key-0219",
LiveKitAPISecret: "test-api-secret-0219-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-0219-victim"}
h.mu.Lock()
h.clients[uid] = c
h.mu.Unlock()
// Fault-inject the GetChannelVoiceStates call inside voiceJoinComplete —
// same technique as the OC-0172 regression test. This hook fires after
// GenerateToken succeeds and strictly before voiceJoinComplete's
// h.pubsub.Subscribe call runs, so by the time GetChannelVoiceStates
// executes the client is already subscribed to VoiceTopic(chID).
var hookRan bool
voiceJoinPostTokenRaceHook = func(client *Client) {
hookRan = true
if _, err := database.ExecContext(context.Background(), `ALTER TABLE users RENAME TO users_bak_0219`); 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")
}
drainChan(send, 200*time.Millisecond)
// Sanity: the in-memory voiceChID was rolled back (OC-0172 already pins
// this half of the cleanup).
if gotCh := c.getVoiceChID(); gotCh != 0 {
t.Fatalf("client voiceChID = %d after GetChannelVoiceStates failed mid-join, want 0 (rolled back)", gotCh)
}
// The bug: rollbackVoiceJoin must also drop the VoiceTopic subscription
// that voiceJoinComplete already established. Left in place, this socket
// keeps receiving voice_e2ee_announce relays for chID indefinitely.
topic := VoiceTopic(chID)
for _, tp := range h.pubsub.TopicsForClient(uid) {
if tp == topic {
t.Fatalf("client is still subscribed to %q after rollbackVoiceJoin — E2EE relays for this channel will keep reaching a socket that never finished joining it", topic)
}
}
}
@@ -0,0 +1,145 @@
package ws
// oc_0222_reconnect_status_order_test.go — regression test for OC-0222.
//
// handleReconnect wrote the resume handshake's auth_ok (reconnectWriteReplay,
// which reads c.user.Status) BEFORE calling applyConnectStatus, which is what
// settles c.user.Status via db.ConnectStatus(saved) and persists it. So a
// resumed auth_ok always carried the disconnect-time status rather than the
// status the session is about to come online as.
//
// Concretely: MarkUserDisconnected rewrites a plain "online" user to
// "offline" on socket loss. On a fast reconnect (still covered by the ring
// buffer, so the buffer-tier replay path is taken) the resumed auth_ok's
// payload.user.status must reflect db.ConnectStatus("offline") == "online" —
// matching what applyConnectStatus is about to write and broadcast — not the
// raw "offline" row value read moments earlier by refreshUserSnapshot.
//
// handleFreshConnect already gets this right: it calls applyConnectStatus
// before building auth_ok (serve.go, handleFreshConnect). This test locks the
// same ordering for the resume path.
import (
"context"
"encoding/json"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/coder/websocket"
"github.com/owncord/server/auth"
"github.com/owncord/server/db"
)
func TestReconnect_AuthOKReflectsSettledStatus_NotDisconnectTimeStatus(t *testing.T) {
database := newTeardownTestDB(t)
ctx := context.Background()
userID, err := database.CreateUser(ctx, "resume-status-user", "hash", 1)
if err != nil {
t.Fatalf("CreateUser: %v", err)
}
// Establish the user as a plain "online" session, then simulate the
// socket loss that precedes every reconnect: MarkUserDisconnected only
// rewrites a plain "online" row to "offline" (idle/dnd/invisible survive
// untouched), so this is the ordinary case, not a contrived one.
if err := database.UpdateUserStatus(ctx, userID, db.StatusOnline); err != nil {
t.Fatalf("UpdateUserStatus(online): %v", err)
}
if err := database.MarkUserDisconnected(ctx, userID); err != nil {
t.Fatalf("MarkUserDisconnected: %v", err)
}
pre, err := database.GetUserByID(ctx, userID)
if err != nil || pre == nil {
t.Fatalf("GetUserByID (precondition): %v", err)
}
if pre.Status != db.StatusOffline {
t.Fatalf("precondition: expected status=offline after MarkUserDisconnected, got %q", pre.Status)
}
token, err := auth.GenerateToken()
if err != nil {
t.Fatalf("GenerateToken: %v", err)
}
if _, err := database.CreateSession(ctx, userID, auth.HashToken(token), "test", "127.0.0.1"); err != nil {
t.Fatalf("CreateSession: %v", err)
}
hub := NewHub(database, auth.NewRateLimiter(), nil)
go hub.Run()
defer hub.Stop()
// Global (channel_id 0) frames bracketing last_seq=99, so the resume takes
// the buffer tier rather than falling through to a full ready (which also
// sends auth_ok, but via handleFreshConnect's already-correct ordering —
// asserting on that path would not exercise the bug).
rb := hub.ReplayBuffer()
rb.Push(98, 0, []byte(`{"seq":98,"type":"presence","payload":{}}`))
rb.Push(99, 0, []byte(`{"seq":99,"type":"presence","payload":{}}`))
rb.Push(100, 0, []byte(`{"seq":100,"type":"presence","payload":{}}`))
srv := httptest.NewServer(ServeWS(hub, database, []string{"*"}, 0))
defer srv.Close()
dialCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
defer cancel()
conn, dialResp, dialErr := websocket.Dial(dialCtx, "ws"+strings.TrimPrefix(srv.URL, "http"), nil)
if dialResp != nil && dialResp.Body != nil {
_ = dialResp.Body.Close()
}
if dialErr != nil {
t.Fatalf("websocket.Dial: %v", dialErr)
}
defer func() { _ = conn.Close(websocket.StatusNormalClosure, "") }()
raw, _ := json.Marshal(map[string]any{
"type": "auth",
"payload": map[string]any{
"token": token,
"last_seq": uint64(99),
},
})
if err := conn.Write(dialCtx, websocket.MessageText, raw); err != nil {
t.Fatalf("write auth: %v", err)
}
readCtx, readCancel := context.WithTimeout(ctx, 5*time.Second)
defer readCancel()
_, msg, err := conn.Read(readCtx)
if err != nil {
t.Fatalf("read handshake response: %v", err)
}
var parsed map[string]any
if err := json.Unmarshal(msg, &parsed); err != nil {
t.Fatalf("unmarshal handshake response: %v; raw=%s", err, msg)
}
if parsed["type"] != MsgTypeAuthOK {
t.Fatalf("expected auth_ok (buffer-tier resume), got %v; raw=%s", parsed["type"], msg)
}
payload, _ := parsed["payload"].(map[string]any)
if payload["replay_source"] != "buffer" {
t.Fatalf("expected replay_source=buffer (so this exercises handleReconnect, not the fresh-connect fallback), got %v", payload["replay_source"])
}
userField, _ := payload["user"].(map[string]any)
gotStatus, _ := userField["status"].(string)
if gotStatus != db.StatusOnline {
t.Fatalf("resumed auth_ok payload.user.status = %q, want %q (db.ConnectStatus of the pre-reconnect \"offline\" row) — "+
"the resumed auth_ok must carry the status the session is settling on, not the stale disconnect-time row value",
gotStatus, db.StatusOnline)
}
// The persisted row must agree with what auth_ok claimed — applyConnectStatus
// must have actually run and been visible before/at the point auth_ok was
// built, not merely be about to run after the client already parsed the
// (wrong) value.
post, err := database.GetUserByID(ctx, userID)
if err != nil || post == nil {
t.Fatalf("GetUserByID (postcondition): %v", err)
}
if post.Status != db.StatusOnline {
t.Fatalf("persisted status after reconnect = %q, want %q", post.Status, db.StatusOnline)
}
}
@@ -0,0 +1,64 @@
package ws
// Internal test for OC-0237: serviceErrorToResult's default branch (the one
// hit for service.ErrInternal, since ErrInternal has no dedicated case above
// it) put err.Error() straight into the ClientError sent to the requesting
// client, and never logged anything server-side. Service-layer ErrInternal
// wrappers embed the underlying driver error via %v (see Server/service/dm.go),
// so this leaked internal query names and driver state to an ordinary member,
// while producing zero server-side log output — handlers.go only logs when
// result.Error is NOT a ClientError. The REST twin, writeServiceError in
// Server/api/channel_handler.go, does the opposite: it logs the error and
// replies with the fixed string "an internal error occurred".
import (
"bytes"
"errors"
"fmt"
"log/slog"
"strings"
"testing"
"github.com/owncord/server/service"
)
func TestServiceErrorToResult_InternalErrorDoesNotLeakAndIsLogged(t *testing.T) {
prev := slog.Default()
var buf bytes.Buffer
slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})))
t.Cleanup(func() { slog.SetDefault(prev) })
// Mirrors Server/service/dm.go:149 — a real ErrInternal wrapper embedding
// driver error text via %v, exactly what handlers_call.go's RingTargets
// call produces when GetDMParticipantIDs fails.
driverErr := errors.New("GetDMParticipantIDs: database is locked")
svcErr := fmt.Errorf("%w: failed to read DM participants: %v", service.ErrInternal, driverErr)
result := serviceErrorToResult(svcErr)
ce, ok := result.Error.(ClientError)
if !ok {
t.Fatalf("serviceErrorToResult(ErrInternal wrapper) did not return a ClientError, got %T", result.Error)
}
if ce.Code != ErrCodeInternal {
t.Fatalf("ClientError.Code = %q, want %q", ce.Code, ErrCodeInternal)
}
// The client-facing message must not leak driver/query internals — it
// must match every other ErrCodeInternal site in this package, which all
// use a fixed string (deps.go, registry.go, voice_controls.go, serve.go).
if strings.Contains(ce.Message, "database is locked") || strings.Contains(ce.Message, "GetDMParticipantIDs") {
t.Fatalf("ClientError.Message leaked internal error detail to the client: %q", ce.Message)
}
if ce.Message == svcErr.Error() {
t.Fatalf("ClientError.Message is the raw wrapped service error verbatim: %q", ce.Message)
}
// Unlike the REST path (writeServiceError), and unlike this same handler
// path for every other error class, nothing was ever written to the
// server log for an internal error — the operator had no record the
// failure happened at all.
if !strings.Contains(buf.String(), "database is locked") {
t.Fatalf("serviceErrorToResult did not log the internal error server-side; log output: %q", buf.String())
}
}
+10
View File
@@ -133,3 +133,13 @@ func (rb *EventRingBuffer) OldestSeq() uint64 {
oldestIdx := (rb.pos - rb.count + rb.size) % rb.size
return rb.entries[oldestIdx].seq
}
// NewestSeq returns the highest sequence number in the buffer, or 0 if empty.
func (rb *EventRingBuffer) NewestSeq() uint64 {
rb.mu.RLock()
defer rb.mu.RUnlock()
if rb.count == 0 {
return 0
}
return rb.newestSeqLocked()
}
+9 -2
View File
@@ -277,6 +277,15 @@ func (h *Hub) handleReconnect(
events = append(events, h.liveVoiceEventsSince(ctx, lastSeq, liveVoiceChID)...)
}
// Settle the session's status BEFORE the auth_ok write below, mirroring
// handleFreshConnect's ordering: reconnectWriteReplay reads c.user.Status
// to build auth_ok, so if this ran after that write the resumed client
// would be told its disconnect-time status (routinely "offline", since
// MarkUserDisconnected just rewrote it) instead of the status it is about
// to come online as and broadcast (OC-0222). Skips member_join — the user
// was already known.
applyConnectStatus(ctx, database, c)
if !h.reconnectWriteReplay(ctx, conn, c, lastSeq, events, replaySource) {
// startPumps=false: the teardown inside reconnectWriteReplay already ran
// in full. Starting readPump on this closed conn would hit an immediate
@@ -285,8 +294,6 @@ func (h *Hub) handleReconnect(
return true, false
}
// Update presence but skip member_join — user was already known.
applyConnectStatus(ctx, database, c)
h.announceConnectPresence(c)
return true, true
+16 -10
View File
@@ -58,13 +58,17 @@ func authenticateConn(parent context.Context, conn *websocket.Conn, database *db
hash := auth.HashToken(p.Token)
sess, err := database.GetSessionByTokenHash(ctx, hash)
if err != nil || sess == nil {
if err != nil {
// DB outage, not a bad token — send a non-terminal error frame so the
// client's normal backoff/reconnect logic retries instead of treating
// this like a genuinely invalid session (buildAuthError is defined as
// non-recoverable on the wire: the client stops reconnecting and
// clears its stored credentials on that frame).
_ = conn.Write(ctx, websocket.MessageText, buildErrorMsg(ErrCodeInternal, "temporary failure, please retry"))
return nil, "", resumeHint{}, fmt.Errorf("auth: session lookup failed: %w", err)
}
if sess == nil {
_ = conn.Write(ctx, websocket.MessageText, buildAuthError("invalid token"))
if err != nil {
// DB outage, not a bad token — carry the cause so the caller's log
// distinguishes it from an ordinary invalid-token rejection.
return nil, "", resumeHint{}, fmt.Errorf("auth: session lookup failed: %w", err)
}
return nil, "", resumeHint{}, fmt.Errorf("auth: invalid session")
}
@@ -74,11 +78,13 @@ func authenticateConn(parent context.Context, conn *websocket.Conn, database *db
}
user, err := database.GetUserByID(ctx, sess.UserID)
if err != nil || user == nil {
if err != nil {
// Same DB-outage-vs-bad-credential distinction as above.
_ = conn.Write(ctx, websocket.MessageText, buildErrorMsg(ErrCodeInternal, "temporary failure, please retry"))
return nil, "", resumeHint{}, fmt.Errorf("auth: user lookup failed: %w", err)
}
if user == nil {
_ = conn.Write(ctx, websocket.MessageText, buildAuthError("user not found"))
if err != nil {
return nil, "", resumeHint{}, fmt.Errorf("auth: user lookup failed: %w", err)
}
return nil, "", resumeHint{}, fmt.Errorf("auth: user not found")
}
+10 -1
View File
@@ -640,7 +640,16 @@ func handleVoiceTokenRefreshV2(ctx context.Context, cmd Command, info ClientInfo
// the row back far enough to learn it), the row is re-read here and the
// delete is skipped unless it still names channelID.
func (h *Hub) rollbackVoiceJoin(ctx context.Context, c *Client, channelID int64, joinedAt string, broadcast bool) {
c.clearVoiceChID()
// OC-0219: use clearVoiceAndUnsubscribe (not the bare clearVoiceChID) so a
// join that already reached voiceJoinComplete's h.pubsub.Subscribe call
// drops its VoiceTopic subscription along with its in-memory voiceChID —
// exactly like every other path that takes a client out of voice while its
// WS stays up (see clearVoiceAndUnsubscribe's doc comment in
// voice_leave.go). Safe for the two earlier call sites too:
// Unsubscribe is a documented no-op when the client was never subscribed
// to that topic (pubsub.go), which is the case whenever this fires before
// voiceJoinComplete's Subscribe has run.
h.clearVoiceAndUnsubscribe(c)
// The client's voice state is now set before token generation (BUG-088),
// so a concurrent join/leave in the same channel can have elected this
// half-joined client key holder. Re-run the election after taking it back
+83
View File
@@ -281,6 +281,89 @@ func TestAuthenticateConn_InvalidToken_ReceivesAuthError(t *testing.T) {
}
}
// TestAuthenticateConn_SessionLookupDBError_NotTerminal verifies OC-0196: a
// transient DB error while looking up the session (GetSessionByTokenHash
// returning a genuine error rather than sql.ErrNoRows) must NOT be reported
// as the terminal auth_error frame. The client treats auth_error as
// non-recoverable — it stops reconnecting and clears the user's stored
// credentials (see Client/tauri-client/src/lib/ws.ts and dispatcher.ts) — so
// collapsing "DB unreachable" into "bad token" force-logs-out every client
// that reconnects during a sub-second SQLite hiccup even though its session
// row is perfectly valid. A DB error must surface as a non-terminal error
// frame instead, so the client's normal backoff/reconnect logic retries.
func TestAuthenticateConn_SessionLookupDBError_NotTerminal(t *testing.T) {
database := openServeTestDB(t)
limiter := auth.NewRateLimiter()
hub := ws.NewHub(database, limiter, nil)
go hub.Run()
defer hub.Stop()
userID, err := database.CreateUser(context.Background(), "db-hiccup-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)
srv := httptest.NewServer(handler)
defer srv.Close()
wsURL := "ws" + strings.TrimPrefix(srv.URL, "http")
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
conn, resp, err := websocket.Dial(ctx, wsURL, nil)
if resp != nil && resp.Body != nil {
defer resp.Body.Close()
}
if err != nil {
t.Fatalf("websocket.Dial: %v", err)
}
defer func() { _ = conn.Close(websocket.StatusNormalClosure, "") }()
// Simulate a transient DB outage AFTER the session/token above were
// written successfully: close the database so the next query
// (GetSessionByTokenHash, made when the auth frame below is processed)
// returns a genuine driver error instead of (nil, nil). The session row
// itself remains logically valid — this models momentary SQLite reader
// contention (WAL checkpoint, backup, busy_timeout), not a bad token.
if err := database.Close(); err != nil {
t.Fatalf("database.Close: %v", err)
}
authMsg := map[string]any{
"type": "auth",
"payload": map[string]string{"token": token},
}
raw, _ := json.Marshal(authMsg)
if err := conn.Write(ctx, websocket.MessageText, raw); err != nil {
t.Fatalf("write: %v", err)
}
_, respRaw, readErr := conn.Read(ctx)
if readErr != nil {
t.Fatalf("read: %v", readErr)
}
var msg map[string]any
if err := json.Unmarshal(respRaw, &msg); err != nil {
t.Fatalf("unmarshal response: %v", err)
}
if msg["type"] == ws.MsgTypeAuthError {
t.Errorf("got terminal %q frame for a transient DB error — the client "+
"treats this as non-recoverable and clears stored credentials; a DB "+
"hiccup must surface as a retryable error instead", ws.MsgTypeAuthError)
}
if msg["type"] != ws.MsgTypeError {
t.Errorf("response type = %q, want %q (non-terminal error frame)", msg["type"], ws.MsgTypeError)
}
}
// TestServeWS_ValidAuth_FullHandshake verifies the complete happy path:
// valid token → auth_ok + ready received, client counted in hub.
func TestServeWS_ValidAuth_FullHandshake(t *testing.T) {