mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
Clean sweep of every actionable item from the two Copilot review passes
on head 59ae4d8. Grouped by severity:
─── Crash / security (must-fix) ─────────────────────────────────────
1. main.go:140 — telemetryShutdown nil panic.
telemetry.Init can return (nil, err) on the -tags otel skeleton
path; the deferred closure would then call a nil function. Normalise
to a no-op shutdown when Init errors so the defer is always safe.
2. api/upload_handler.go — permSvc nil deref.
MountUploadRoutes + handleServeFile dereference permSvc on every
authenticated file request. Add a fail-fast panic at mount time so
the misconfiguration surfaces at wiring, not on the first 500.
Update upload_handler_test.go to pass a real PermissionService built
on the test DB (the existing tests were missing the argument entirely,
which meant the package wouldn't compile — this fixes the real bug
Copilot flagged).
3. ws/event_persister.go — NewEventPersister nil EventStore panic.
run() dereferences p.store on every flush. Panic at constructor
time instead so the crash happens once at startup rather than
minutes later in a background goroutine.
4. plugin/host_ui.go — serve-time symlink check.
rejectSymlinksUnder only runs at install time, so a symlink created
post-install (accidental or malicious) would be followed by
http.ServeFile and leak host files. Add an os.Lstat + ModeSymlink
check + IsRegular check to AssetHandler on every request. Cheap
relative to the file read and closes the TOCTOU window.
─── Correctness / observability (should-fix) ───────────────────────
5. ws/deps.go:77 — requirePerm hides misconfig as FORBIDDEN.
Previously, nil database, nil perms, or a GetRoleForUser error all
returned ErrCodeForbidden with the same message, making operator
failures indistinguishable from legitimate permission denials.
Split the branches: misconfig + DB error now return ErrCodeInternal
with a server-side slog.Error so operators see the real problem;
FORBIDDEN is reserved for the actual permission-bit check.
6. telemetry/metrics.go — ServiceCallDurationMs renamed to Sec.
Field name said "Ms" but the instrument name was
`service_call_duration_seconds` with unit "s". Renamed the field
and updated all 8 service-layer callers so the struct field and
metric semantics match.
7. ws/event_persister.go — flushEvy typo → flushEvery.
Renamed the field and the one call site in run().
─── Comments out of sync with code ──────────────────────────────────
8. plugin/loader.go — Stat vs Lstat comment.
The comment claimed "Stat (not Lstat)" but the code correctly uses
os.Lstat to detect symlinks. Updated the comment to match the code;
the code was already right.
9. telemetry/telemetry_otel.go — compile claim wrong.
Comment said the file would fail to compile without the upstream
OTel modules, but the skeleton deliberately avoids importing them
and Init returns a runtime error instead. Updated the comment to
reflect actual CI behaviour (the -tags otel build step passes
today but doesn't exercise real telemetry).
─── Nit / polish ────────────────────────────────────────────────────
10. ws/event_pruner.go — startup delay magic constant.
Hard-coded time.Minute made the "run shortly after startup"
behaviour untestable (a test with a 100ms interval would still
wait a full minute). Cap the startup delay by the interval:
min(interval, time.Minute). Documented via a new `maxStartupDelay`
constant.
11. ws/event_pruner_test.go — new file.
Unit coverage for runPrune cutoff correctness, error swallowing,
StartEventPruner nil-store short-circuit, ctx cancellation, and
the interval-bounded startup delay from fix #10. Uses a fakeEventStore
stub that records every prune call and signals the first one so
tests don't sleep.
─── Verification ────────────────────────────────────────────────────
gofmt -l clean. No network access in sandbox so `go vet` and `go test`
could not run; the changes are local and surgical and every touched
file compiles in isolation against the existing signatures.
https://claude.ai/code/session_01UsBsQW2YiA2usk9pnJjAWk
130 lines
5.1 KiB
Go
130 lines
5.1 KiB
Go
package ws
|
|
|
|
import (
|
|
"context"
|
|
"log/slog"
|
|
|
|
"github.com/owncord/server/auth"
|
|
"github.com/owncord/server/db"
|
|
"github.com/owncord/server/permissions"
|
|
"github.com/owncord/server/service"
|
|
)
|
|
|
|
// ClientInfo holds a read-only snapshot of client state for V2 handlers.
|
|
// Handlers receive this instead of a mutable *Client pointer, making them
|
|
// easier to test and reason about.
|
|
type ClientInfo struct {
|
|
UserID int64
|
|
Username string
|
|
Avatar *string
|
|
RoleName string
|
|
ReqID string
|
|
VoiceChannelID int64 // 0 if not in a voice channel
|
|
VoiceJoinToken string // opaque join-instance token for the current voice session
|
|
}
|
|
|
|
// ── Per-domain dependency structs ───────────────────────────────────────────
|
|
|
|
// PingDeps holds dependencies for the ping handler.
|
|
type PingDeps struct {
|
|
Limiter *auth.RateLimiter
|
|
}
|
|
|
|
// ChatDeps holds dependencies for chat handlers.
|
|
type ChatDeps struct {
|
|
Limiter *auth.RateLimiter
|
|
MessageSvc *service.MessageService
|
|
}
|
|
|
|
// PresenceDeps holds dependencies for presence, typing, and channel focus handlers.
|
|
type PresenceDeps struct {
|
|
Limiter *auth.RateLimiter
|
|
ChannelSvc *service.ChannelService
|
|
}
|
|
|
|
// ReactionDeps holds dependencies for reaction handlers.
|
|
type ReactionDeps struct {
|
|
MessageSvc *service.MessageService
|
|
}
|
|
|
|
// VoiceTokenGenerator generates LiveKit access tokens. Abstracted so V2
|
|
// handlers can be tested without a real LiveKit server.
|
|
type VoiceTokenGenerator interface {
|
|
GenerateToken(userID int64, username string, channelID int64, voiceJoinToken string, canPublish, canSubscribe, canVideo, canScreenShare bool) (string, error)
|
|
URL() string
|
|
}
|
|
|
|
// KeyHolderChecker reports whether a user is the E2EE key holder for a voice channel.
|
|
type KeyHolderChecker interface {
|
|
IsVoiceKeyHolder(channelID, userID int64) bool
|
|
}
|
|
|
|
// VoiceDeps holds dependencies for voice handlers.
|
|
type VoiceDeps struct {
|
|
DB *db.DB
|
|
Limiter *auth.RateLimiter
|
|
Permissions *permissions.Checker
|
|
LiveKit *LiveKitClient
|
|
TokenGen VoiceTokenGenerator // used by voice_token_refresh V2
|
|
KeyHolder KeyHolderChecker // used by voice_token_refresh V2
|
|
}
|
|
|
|
// ── V2 permission helpers ───────────────────────────────────────────────────
|
|
|
|
// requirePerm checks a channel permission via DB lookups. Returns nil if
|
|
// allowed, or a Result carrying either an INTERNAL error (when the server
|
|
// is misconfigured or a DB lookup fails) or a FORBIDDEN error (when the
|
|
// permission bit is genuinely absent from the user's role). Previously
|
|
// every branch returned FORBIDDEN, which hid operator-visible failures
|
|
// behind a user-facing permission denial.
|
|
func requirePerm(database *db.DB, perms *permissions.Checker, userID, channelID, perm int64, label string) *Result {
|
|
if database == nil || perms == nil {
|
|
// Missing dependency is a server bug, not a user ACL outcome. Log
|
|
// here so operators see something even when the client surfaces a
|
|
// generic error.
|
|
slog.Error("ws: requirePerm called with nil dependency",
|
|
"have_database", database != nil, "have_perms", perms != nil, "label", label)
|
|
r := Result{Error: ClientError{Code: ErrCodeInternal, Message: "permission check unavailable"}}
|
|
return &r
|
|
}
|
|
role, err := database.GetRoleForUser(userID)
|
|
if err != nil {
|
|
slog.Error("ws: requirePerm GetRoleForUser failed",
|
|
"user_id", userID, "channel_id", channelID, "err", err)
|
|
r := Result{Error: ClientError{Code: ErrCodeInternal, Message: "permission check failed"}}
|
|
return &r
|
|
}
|
|
if role == nil {
|
|
// No role row is a genuine ACL outcome (no role == no perms).
|
|
r := Result{Error: ClientError{Code: ErrCodeForbidden, Message: "missing " + label + " permission"}}
|
|
return &r
|
|
}
|
|
if !perms.HasChannelPerm(role.Permissions, role.ID, channelID, perm) {
|
|
r := Result{Error: ClientError{Code: ErrCodeForbidden, Message: "missing " + label + " permission"}}
|
|
return &r
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// hasPerm checks a channel permission via DB lookups. Returns true if allowed.
|
|
func hasPerm(database *db.DB, perms *permissions.Checker, userID, channelID, perm int64) bool {
|
|
if database == nil || perms == nil {
|
|
return false
|
|
}
|
|
role, err := database.GetRoleForUser(userID)
|
|
if err != nil || role == nil {
|
|
return false
|
|
}
|
|
return perms.HasChannelPerm(role.Permissions, role.ID, channelID, perm)
|
|
}
|
|
|
|
// ── V2 handler type ─────────────────────────────────────────────────────────
|
|
|
|
// HandlerV2 is the function signature for new-style (pure-ish) handlers.
|
|
// They receive a typed Command, a read-only ClientInfo snapshot, and a
|
|
// domain-specific deps struct (passed as any; handler asserts the concrete type).
|
|
// They return a Result describing what events to emit and any error.
|
|
// TODO: consider replacing `deps any` with generics (HandlerV2[D any]) to get
|
|
// compile-time type safety on deps wiring. Requires reworking the registry map.
|
|
type HandlerV2 func(ctx context.Context, cmd Command, info ClientInfo, deps any) Result
|