From 215f931311774c6edbc4a29491d051aa3d7a3c47 Mon Sep 17 00:00:00 2001 From: hobokenchicken Date: Tue, 30 Jun 2026 13:47:08 -0400 Subject: [PATCH] fix: wire permission checks into message/channel handlers - Add CheckChannelPermission to permissions.Checker (layers channel overrides on top of role permissions) - Refactor requireChannelAccess to accept required permission bitmap - Message Create checks SEND_MESSAGES; List/Search check VIEW_CHANNEL - BulkDelete uses Checker.CheckPermission for MANAGE_MESSAGES - Channel handler List filters out channels user cannot view - Remove dead checkPermission method from message handler - Fix frontend avatar upload: parse response URL, call updateProfile --- ISSUES.md | 21 ++++----- cmd/server/main.go | 2 +- internal/channel/handlers.go | 14 +++++- internal/message/handlers.go | 66 +++++++++++------------------ internal/permissions/checker.go | 60 ++++++++++++++++++++++++++ web/src/components/UserSettings.tsx | 5 ++- web/src/stores/auth.ts | 1 + 7 files changed, 112 insertions(+), 57 deletions(-) diff --git a/ISSUES.md b/ISSUES.md index 58f2d01..0129307 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -10,11 +10,9 @@ This document contains a comprehensive audit of the dumpsterChat repository, hig * **Location:** [internal/db/db.go](file:///opt/dumpsterChat/internal/db/db.go#L189), [internal/webhook/handlers.go](file:///opt/dumpsterChat/internal/webhook/handlers.go#L143-L151) * **Fix:** Plaintext token column has been dropped via DDL migration (`ALTER TABLE webhooks DROP COLUMN IF EXISTS token;`), and webhook creation no longer stores plaintext tokens in the database. -### 🚨 Message and Channel Access Bypasses Role Permissions & Overrides -* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go#L663-L686), [internal/channel/handlers.go](file:///opt/dumpsterChat/internal/channel/handlers.go#L165-L187) -* **Description:** Access to channel reading, message listing, and message posting is checked solely via server membership (`members` table). Role permissions (like `VIEW_CHANNEL` or `SEND_MESSAGES`) and channel-level overrides (`channel_overrides` table) are completely ignored. -* **Impact:** Any user who is a member of a server can read, search, and post messages in *any* channel on that server (including private, forum, calendar, or documentation channels), completely bypassing the permissions configuration system. -* **Remediation:** Update `requireChannelAccess` in `message.Handler` to use `permissions.Checker` to verify channel permission overrides and server roles. +### 🚨 Message and Channel Access Bypasses Role Permissions & Overrides ✅ RESOLVED +* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go), [internal/channel/handlers.go](file:///opt/dumpsterChat/internal/channel/handlers.go) +* **Fix:** Added `CheckChannelPermission` method to `permissions.Checker` that layers channel overrides on top of role permissions. Updated `requireChannelAccess` to accept a required permission bitmap — message Create checks `SEND_MESSAGES`, List/Search check `VIEW_CHANNEL`, BulkDelete uses `CheckPermission` for `MANAGE_MESSAGES`. Channel handler List now filters out channels the user cannot view. ### ⚠️ WebAuthn Session Cookie SameSite Laxity ✅ RESOLVED * **Location:** [internal/auth/cookie.go](file:///opt/dumpsterChat/internal/auth/cookie.go#L17) @@ -28,10 +26,9 @@ This document contains a comprehensive audit of the dumpsterChat repository, hig ## 2. Functional Bugs & Defects -### 🐛 User Avatar Cannot Be Changed (Backend fixed, Frontend still broken) ⚠️ PARTIALLY RESOLVED -* **Location:** [internal/auth/handlers.go](file:///opt/dumpsterChat/internal/auth/handlers.go#L210-L220), [web/src/components/UserSettings.tsx](file:///opt/dumpsterChat/web/src/components/UserSettings.tsx#L73-L84) -* **Status:** The backend has been successfully updated to accept `AvatarURL` in the PATCH request. However, the frontend avatar upload code is still broken: it uploads the file to `/api/v1/upload` but does not parse the response URL, nor does it call the profile update endpoint to save the returned URL. -* **Remediation:** Update `handleAvatarUpload` in `UserSettings.tsx` to read the returned JSON URL and send a PATCH request via `updateProfile` containing the `avatar_url`. +### 🐛 User Avatar Cannot Be Changed ✅ RESOLVED +* **Location:** [internal/auth/handlers.go](file:///opt/dumpsterChat/internal/auth/handlers.go#L210-L220), [web/src/components/UserSettings.tsx](file:///opt/dumpsterChat/web/src/components/UserSettings.tsx) +* **Fix:** Backend accepts `avatar_url` in PATCH profile. Frontend upload now parses the returned URL and calls `updateProfile` to save it. `UpdateProfilePayload` type also gained the `avatar_url` field. ### 🐛 TUI Client Auth Failure Fallback Bug ✅ RESOLVED * **Location:** [cmd/tui/auth.go](file:///opt/dumpsterChat/cmd/tui/auth.go#L21-L36) @@ -45,9 +42,9 @@ This document contains a comprehensive audit of the dumpsterChat repository, hig * **Location:** [internal/permissions/overrides.go](file:///opt/dumpsterChat/internal/permissions/overrides.go) * **Fix:** Unused function removed from the package. -### 🗑️ Duplicated Permission Verification -* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go#L129-L158) -* **Description:** The `checkPermission` method in `message.Handler` duplicates role checking queries instead of using the `permissions.Checker` struct. +### 🗑️ Duplicated Permission Verification ✅ RESOLVED +* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go) +* **Fix:** Removed the dead `checkPermission` method. Callers now use the `Checker` from the `permissions` package and its new `CheckChannelPermission` method. ### 🗑️ Obsolete Handler Type ✅ RESOLVED * **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go) diff --git a/cmd/server/main.go b/cmd/server/main.go index 2af8985..35eacb6 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -217,7 +217,7 @@ func main() { // Messages (under channels) r.Route("/channels/{channelID}/messages", func(r chi.Router) { - message.NewHandler(database.DB, hub, pushHandler, logger).RegisterRoutes(r) + message.NewHandler(database.DB, hub, pushHandler, logger, permissionsChecker).RegisterRoutes(r) }) // Per-channel notification settings diff --git a/internal/channel/handlers.go b/internal/channel/handlers.go index 16cc558..fe89ef6 100644 --- a/internal/channel/handlers.go +++ b/internal/channel/handlers.go @@ -211,8 +211,20 @@ func (h *Handler) List(w http.ResponseWriter, r *http.Request) { return } + // Filter out channels the user doesn't have VIEW_CHANNEL permission for. + filtered := make([]channelResponse, 0, len(channels)) + for _, ch := range channels { + allowed, checkErr := h.checker.CheckChannelPermission(r.Context(), serverID, userID, ch.ID, permissions.VIEW_CHANNEL) + if checkErr != nil { + continue + } + if allowed { + filtered = append(filtered, ch) + } + } + w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(channels) + json.NewEncoder(w).Encode(filtered) } // @Summary Get a channel diff --git a/internal/message/handlers.go b/internal/message/handlers.go index 1929568..2f785b8 100644 --- a/internal/message/handlers.go +++ b/internal/message/handlers.go @@ -26,15 +26,17 @@ type Handler struct { mentions *MentionHandler pushHandler *push.Handler logger *slog.Logger + checker *permissions.Checker } -func NewHandler(db *sql.DB, hub *gateway.Hub, pushHandler *push.Handler, logger *slog.Logger) *Handler { +func NewHandler(db *sql.DB, hub *gateway.Hub, pushHandler *push.Handler, logger *slog.Logger, checker *permissions.Checker) *Handler { return &Handler{ db: db, hub: hub, mentions: NewMentionHandler(db, pushHandler, logger), pushHandler: pushHandler, logger: logger, + checker: checker, } } @@ -59,14 +61,12 @@ type bulkDeleteResponse struct { // and messages must be within the last 14 days. func (h *Handler) BulkDelete(w http.ResponseWriter, r *http.Request) { channelID := chi.URLParam(r, "channelID") - userID, serverID, ok := h.requireChannelAccess(w, r, channelID) + userID, serverID, ok := h.requireChannelAccess(w, r, channelID, 0) if !ok { return } - // ponytail: reuse existing auth structure; requireChannelAccess already verifies membership. - // Permission check uses the server's permission checker via the caller's middleware chain. - allowed, err := h.checkPermission(r.Context(), serverID, userID, permissions.MANAGE_MESSAGES) + allowed, err := h.checker.CheckPermission(r.Context(), serverID, userID, permissions.MANAGE_MESSAGES) if err != nil { http.Error(w, `{"error":"server error"}`, http.StatusInternalServerError) return @@ -126,37 +126,6 @@ func (h *Handler) BulkDelete(w http.ResponseWriter, r *http.Request) { json.NewEncoder(w).Encode(bulkDeleteResponse{Deleted: int(deleted)}) } -// checkPermission is a helper wrapper around a permission checker lookup. -func (h *Handler) checkPermission(ctx context.Context, serverID, userID string, perm int64) (bool, error) { - // ponytail: keep it minimal; caller already has serverID. If no checker is wired, fall back to true only for admins. - rows, err := h.db.QueryContext(ctx, ` - SELECT r.permissions FROM roles r - INNER JOIN member_roles mr ON mr.role_id = r.id - WHERE mr.user_id = $1 AND mr.server_id = $2 - `, userID, serverID) - if err != nil { - return false, err - } - defer rows.Close() - var effective int64 - for rows.Next() { - var p int64 - if err := rows.Scan(&p); err != nil { - return false, err - } - effective |= p - } - if err := rows.Err(); err != nil { - return false, err - } - var everyone int64 - _ = h.db.QueryRowContext(ctx, ` - SELECT permissions FROM roles WHERE server_id = $1 AND is_default = TRUE LIMIT 1 - `, serverID).Scan(&everyone) - effective |= everyone - return permissions.Has(effective, permissions.ADMINISTRATOR) || permissions.Has(effective, perm), nil -} - type embedResponse struct { ID string `json:"id"` URL string `json:"url"` @@ -208,7 +177,7 @@ type createMessageRequest struct { // @Router /channels/{channelID}/messages [post] func (h *Handler) Create(w http.ResponseWriter, r *http.Request) { channelID := chi.URLParam(r, "channelID") - userID, serverID, ok := h.requireChannelAccess(w, r, channelID) + userID, serverID, ok := h.requireChannelAccess(w, r, channelID, permissions.SEND_MESSAGES) if !ok { return } @@ -494,7 +463,7 @@ func (h *Handler) Delete(w http.ResponseWriter, r *http.Request) { // @Router /channels/{channelID}/messages [get] func (h *Handler) List(w http.ResponseWriter, r *http.Request) { channelID := chi.URLParam(r, "channelID") - userID, _, ok := h.requireChannelAccess(w, r, channelID) + userID, _, ok := h.requireChannelAccess(w, r, channelID, permissions.VIEW_CHANNEL) if !ok { return } @@ -599,7 +568,7 @@ func (h *Handler) attachEmbeds(ctx context.Context, messages []messageResponse) // Search searches messages in a channel using PostgreSQL full-text search. func (h *Handler) Search(w http.ResponseWriter, r *http.Request) { channelID := chi.URLParam(r, "channelID") - _, _, ok := h.requireChannelAccess(w, r, channelID) + _, _, ok := h.requireChannelAccess(w, r, channelID, permissions.VIEW_CHANNEL) if !ok { return } @@ -661,8 +630,9 @@ func (h *Handler) Search(w http.ResponseWriter, r *http.Request) { } // requireChannelAccess checks if user is authenticated and is a member of the channel's server. -// Returns (userID, serverID, ok). -func (h *Handler) requireChannelAccess(w http.ResponseWriter, r *http.Request, channelID string) (string, string, bool) { +// Returns (userID, serverID, ok). If required is non-zero, also checks the permission against +// channel-level overrides. +func (h *Handler) requireChannelAccess(w http.ResponseWriter, r *http.Request, channelID string, required int64) (string, string, bool) { userID, ok := middleware.UserIDFromContext(r.Context()) if !ok { http.Error(w, `{"error":"unauthorized"}`, http.StatusUnauthorized) @@ -682,5 +652,19 @@ func (h *Handler) requireChannelAccess(w http.ResponseWriter, r *http.Request, c return "", "", false } + // Check channel-specific permission if a bitmap was provided. + if required != 0 { + allowed, checkErr := h.checker.CheckChannelPermission(r.Context(), serverID, userID, channelID, required) + if checkErr != nil { + h.logger.Error("permission check failed", "error", checkErr) + http.Error(w, `{"error":"server error"}`, http.StatusInternalServerError) + return "", "", false + } + if !allowed { + http.Error(w, `{"error":"missing permissions"}`, http.StatusForbidden) + return "", "", false + } + } + return userID, serverID, true } diff --git a/internal/permissions/checker.go b/internal/permissions/checker.go index 3c2aad5..7b42d44 100644 --- a/internal/permissions/checker.go +++ b/internal/permissions/checker.go @@ -80,3 +80,63 @@ func (c *Checker) CheckPermission(ctx context.Context, serverID string, userID s return Has(perms, required), nil } + +// CheckChannelPermission reports whether the user has the required permission bits +// for a specific channel. It layers channel-level overrides on top of server role +// permissions. ADMINISTRATOR bypasses all checks. +func (c *Checker) CheckChannelPermission(ctx context.Context, serverID, userID, channelID string, required int64) (bool, error) { + perms, err := c.GetUserPermissions(ctx, serverID, userID) + if err != nil { + return false, err + } + + // Administrator bypasses everything. + if Has(perms, ADMINISTRATOR) { + return true, nil + } + + // Layer channel overrides: apply deny first (subtract), then allow (add). + // Role-based overrides from all user roles, then user-specific override. + rows, err := c.db.QueryContext(ctx, ` + SELECT co.allow_bitflags, co.deny_bitflags + FROM channel_overrides co + WHERE co.channel_id = $1 AND co.target_type = 'role' + AND co.target_id IN ( + SELECT mr.role_id FROM member_roles mr WHERE mr.user_id = $2 AND mr.server_id = $3 + ) + `, channelID, userID, serverID) + if err != nil { + return false, err + } + + var allow, deny int64 + if rows != nil { + for rows.Next() { + var a, d int64 + if err := rows.Scan(&a, &d); err != nil { + rows.Close() + return false, err + } + allow |= a + deny |= d + } + rows.Close() + } + + // User-specific override. + var ua, ud int64 + err = c.db.QueryRowContext(ctx, ` + SELECT allow_bitflags, deny_bitflags FROM channel_overrides + WHERE channel_id = $1 AND target_type = 'user' AND target_id = $2 + `, channelID, userID).Scan(&ua, &ud) + if err != nil && err != sql.ErrNoRows { + return false, err + } + allow |= ua + deny |= ud + + // Apply deny first, then allow. + perms = (perms &^ deny) | allow + + return Has(perms, required), nil +} diff --git a/web/src/components/UserSettings.tsx b/web/src/components/UserSettings.tsx index 1453ea5..0ba9e1a 100644 --- a/web/src/components/UserSettings.tsx +++ b/web/src/components/UserSettings.tsx @@ -4,7 +4,7 @@ import { useAuthStore } from '../stores/auth.ts'; import { usePushStore } from '../stores/push.ts'; export function UserSettings() { - const { user, updateProfile, changePassword, isLoading, error, clearError, fetchMe } = useAuthStore(); + const { user, updateProfile, changePassword, isLoading, error, clearError } = useAuthStore(); const { isSupported, isSubscribed, subscribe, unsubscribe, checkSubscription } = usePushStore(); const navigate = useNavigate(); @@ -81,7 +81,8 @@ export function UserSettings() { throw new Error(data?.message || data?.error || `Upload failed: ${response.status}`); } - await fetchMe(); + const uploadData = await response.json(); + await updateProfile({ avatar_url: uploadData.url }); } catch (err) { setUploadError(err instanceof Error ? err.message : 'Upload failed'); } finally { diff --git a/web/src/stores/auth.ts b/web/src/stores/auth.ts index f436d8b..5830549 100644 --- a/web/src/stores/auth.ts +++ b/web/src/stores/auth.ts @@ -31,6 +31,7 @@ export type BlockedUser = { }; export interface UpdateProfilePayload { + avatar_url?: string; display_name?: string; bio?: string; accent_color?: string;