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
- upload_handler.go: uses PermissionService.HasChannelPerm instead of
the deleted hasChannelPermREST helper
- profile_handler.go: delegates to UserService for profile updates,
password changes, session listing, and session revocation
- Remove hasChannelPermREST from channel_handler.go (no longer needed)
- UserService.UpdateProfile now returns ErrConflict on duplicate username
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
Addresses 14 findings from the security audit across all severity levels:
CRITICAL:
- C-1: Add user blocking system (migration, DB queries, REST API, WS DM
send check) to prevent harassment via unconsented DMs
- C-2: Remove server version from unauthenticated /health and /info endpoints
to prevent fingerprinting
HIGH:
- H-1: Remove dangerous-settings feature from tauri-plugin-http
- H-3: Default allowSelfSigned to false in API client (was hardcoded true)
- H-4: Cap invite expiration to 30 days (720 hours)
- H-5: Add 256KB message size limit to LiveKit WS proxy (prevents OOM)
- H-6: Cap concurrent sessions to 25 per user (evicts oldest on overflow)
- H-8: Restrict /diagnostics/connectivity to ADMINISTRATOR role
MEDIUM:
- M-2: Deny access to legacy NULL-uploader unlinked attachments
- M-4: Log warnings on TOTP plaintext decryption fallback paths
- M-8: Remove acceptInvalidCerts from OG preview fetches
- M-10: Expand file upload blocklist (Java .class, OLE2, WASM, .lnk)
- M-12: Add LIMIT to ListInvites (200) and ListMembers (1000)
- M-14: Add CHECK constraint trigger on channels.type (text/voice/dm)
https://claude.ai/code/session_01KKo3RwjdmcNzkgXNfUkgNT
Security audit across all 11 sections (AUTH-001 through DATA-001) found
0 critical, 1 high, 7 medium, 15 low issues. This commit addresses:
- Add json:"-" to User.PasswordHash, User.TOTPSecret, Session.TokenHash
to prevent accidental serialization of sensitive fields (M7)
- Add X-Content-Type-Options: nosniff to file serve responses (M5)
- Apply owner-only guard to backup list endpoint for consistency (M6)
- Persist rate-limit lockouts to SQLite so they survive restarts (M2)
- Normalize DM non-participant responses to 404 to prevent oracle (L3)
- Add explicit per-entry expiry check in partial auth Lookup/Consume (L1)
- Truncate unknown WS message type to 64 chars before echo (L6)
- Rate-limit ping handler to 2/sec per user (L7)
- Replace raw error strings in update handlers with generic messages (L15)
- Update 4 tests to match new 404 behavior for DM non-participant
BUG-112: clientIPWithProxies now validates extracted X-Real-IP and
X-Forwarded-For values with net.ParseIP. Non-IP strings are rejected,
falling back to RemoteAddr. Prevents attackers from choosing arbitrary
rate-limit bucket keys via header injection.
BUG-118: Files with MIME types that could execute active content
(HTML, SVG, XML, PDF) are now served with Content-Disposition: attachment
instead of inline, preventing content hosting under the OwnCord origin.
Upload endpoint now enforces 10 uploads/min per user via the existing
RateLimiter. Previously only body size was capped (100 MiB) with no
per-user throttle, allowing authenticated users to exhaust disk with
repeated uploads.
Private attachments were accessible without authentication if the UUID
was known. Added AuthMiddleware to the GET /api/v1/files/{id} route,
uploader_id tracking on uploads, and channel-level permission checks
(guild READ_MESSAGES, DM participant, admin bypass) in handleServeFile.
Migration 010 adds uploader_id column to attachments table.
8 new access-control tests covering all authorization paths.
Create Server/api/constants.go (26 constants) and Server/auth/constants.go
(6 constants) for rate limits, timeouts, size limits, and token generation.
Replace all inline magic numbers with descriptive names across 9 source files.
No behavior changes — same values, just named for contributor readability.
- C-3: inject setupLimiter into NewAdminAPI instead of package-level global
- H-5: generateRandomKey returns error instead of panicking
- H-6: replace init() bcrypt with sync.Once lazy initialization
- M-2: remove unsafe-inline from admin CSP
- M-3: sanitize upload filenames (strip control chars, truncate to 255)
- M-5: truncate User-Agent to 512 bytes before storing as device
- M-10: MaxBodySizeUnless uses prefix matching instead of exact path
- M-12: wrap seedExistingDatabase in a single transaction
- M-13: use errors.Is for EOF check in upload handler
- M-14: log writeJSON encoding errors instead of discarding
- M-16: standardize error codes to INTERNAL_ERROR across all handlers
- C-3: inject setupLimiter into NewAdminAPI instead of package-level global
- H-5: generateRandomKey returns error instead of panicking
- H-6: replace init() bcrypt with sync.Once lazy initialization
- M-2: remove unsafe-inline from admin CSP script-src and style-src
- M-3: sanitize upload filenames (strip control chars, truncate to 255)
- M-5: truncate User-Agent to 512 bytes before storing as device
- M-10: MaxBodySizeUnless uses prefix matching instead of exact path
- M-12: wrap seedExistingDatabase in a single transaction
- M-13: use errors.Is for EOF check in upload handler
- M-14: log writeJSON encoding errors instead of discarding
- M-16: standardize error codes to INTERNAL_ERROR across all handlers
Server:
- Extract image width/height on upload via image.DecodeConfig (header-only)
- Add width/height columns to attachments table (migration 007)
- Include dimensions in AttachmentInfo JSON (optional, backward-compatible)
Client:
- Reserve exact space for images using server-provided dimensions
- Fallback min-height: 200px for external/old images, cleared on load
- Remove premeasureAll() — was caching wrong heights for unloaded images
- Smart per-type height estimates: 32px dividers, 42/72px text, +220px
per image attachment, +320px for YouTube embeds
- Batch ResizeObserver corrections to single RAF with anchor-based scroll
preservation (topmost visible item stays in place)
- Fenwick tree for O(log n) offset lookups (replaces O(n) linear scans)
- CSS contain: layout style on .msg-image to isolate layout shift
- CRIT-2: Prevent HTTP header injection in Content-Disposition via mime.FormatMediaType
- HIGH-1: Call hub.GracefulStop() on server shutdown to clean up WebSocket/voice state
- HIGH-2: Remove double send on serveErr channel that caused goroutine leak
- HIGH-3: Handle GetUserByID error after registration to prevent nil panic
- HIGH-4: Return nil,nil on sql.ErrNoRows in GetAttachmentByID (matching codebase convention)
- HIGH-5: Add voiceDone channel to bound RTP goroutine lifetime on voice teardown
- Fix handleVoiceICE to return VOICE_ERROR when no PC (consistent with other voice handlers)
- Update tests to match corrected behavior
Server:
- Add POST /api/v1/uploads and GET /api/v1/files/{id} endpoints
- Add CreateAttachment DB method for file upload records
- Allow empty message content when attachments are present
- CORS headers on file serving for WebView2 compatibility
Client — File uploads & attachments:
- Clipboard paste (Ctrl+V) and attach button (+) for file uploads
- Preview bar above input with thumbnail, spinner, and remove button
- Images fetched via Tauri HTTP plugin as base64 data URIs (bypasses
self-signed cert rejection in WebView2)
- Three-layer image cache: memory → IndexedDB → network
- In-flight deduplication prevents duplicate concurrent fetches
- Image lightbox with click-to-zoom, scroll wheel zoom, pan, keyboard shortcuts
Client — URL previews & embeds:
- URLs in messages rendered as clickable links
- YouTube embeds with thumbnail, play button, video title via oEmbed API
- Generic link previews with OG metadata (title, description, image)
- Fetched via Tauri HTTP plugin with Facebook crawler User-Agent
- YouTube title cache and OG metadata cache prevent re-fetch on re-render
- Links open in default browser via tauri-plugin-opener
Client — Voice & audio fixes:
- Mute uses replaceTrack(null) for reliable RTP-level muting in WebView2
- Deafen also mutes mic; undeafen/unmute unmutes both
- Muted users show crossed mic icon, deafened show crossed mic + headphone
- Re-apply mute state after input device switch
Client — UX improvements:
- Disable browser context menu globally (only custom menus show)
- Emoji search now matches by keyword names (smile, heart, fire, etc.)
- Emoji picker closes on click outside
- User bar status text moved below username
- Messages sorted chronologically (oldest first, newest at bottom)
- Scroll to bottom on initial load with deferred retries for layout shifts
- Image attachments constrained to 400x350px with click-to-lightbox
Claude