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
This commit is contained in:
@@ -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)
|
* **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.
|
* **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
|
### 🚨 Message and Channel Access Bypasses Role Permissions & Overrides ✅ RESOLVED
|
||||||
* **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)
|
* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go), [internal/channel/handlers.go](file:///opt/dumpsterChat/internal/channel/handlers.go)
|
||||||
* **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.
|
* **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.
|
||||||
* **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.
|
|
||||||
|
|
||||||
### ⚠️ WebAuthn Session Cookie SameSite Laxity ✅ RESOLVED
|
### ⚠️ WebAuthn Session Cookie SameSite Laxity ✅ RESOLVED
|
||||||
* **Location:** [internal/auth/cookie.go](file:///opt/dumpsterChat/internal/auth/cookie.go#L17)
|
* **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
|
## 2. Functional Bugs & Defects
|
||||||
|
|
||||||
### 🐛 User Avatar Cannot Be Changed (Backend fixed, Frontend still broken) ⚠️ PARTIALLY RESOLVED
|
### 🐛 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#L73-L84)
|
* **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)
|
||||||
* **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.
|
* **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.
|
||||||
* **Remediation:** Update `handleAvatarUpload` in `UserSettings.tsx` to read the returned JSON URL and send a PATCH request via `updateProfile` containing the `avatar_url`.
|
|
||||||
|
|
||||||
### 🐛 TUI Client Auth Failure Fallback Bug ✅ RESOLVED
|
### 🐛 TUI Client Auth Failure Fallback Bug ✅ RESOLVED
|
||||||
* **Location:** [cmd/tui/auth.go](file:///opt/dumpsterChat/cmd/tui/auth.go#L21-L36)
|
* **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)
|
* **Location:** [internal/permissions/overrides.go](file:///opt/dumpsterChat/internal/permissions/overrides.go)
|
||||||
* **Fix:** Unused function removed from the package.
|
* **Fix:** Unused function removed from the package.
|
||||||
|
|
||||||
### 🗑️ Duplicated Permission Verification
|
### 🗑️ Duplicated Permission Verification ✅ RESOLVED
|
||||||
* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go#L129-L158)
|
* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go)
|
||||||
* **Description:** The `checkPermission` method in `message.Handler` duplicates role checking queries instead of using the `permissions.Checker` struct.
|
* **Fix:** Removed the dead `checkPermission` method. Callers now use the `Checker` from the `permissions` package and its new `CheckChannelPermission` method.
|
||||||
|
|
||||||
### 🗑️ Obsolete Handler Type ✅ RESOLVED
|
### 🗑️ Obsolete Handler Type ✅ RESOLVED
|
||||||
* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go)
|
* **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go)
|
||||||
|
|||||||
+1
-1
@@ -217,7 +217,7 @@ func main() {
|
|||||||
|
|
||||||
// Messages (under channels)
|
// Messages (under channels)
|
||||||
r.Route("/channels/{channelID}/messages", func(r chi.Router) {
|
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
|
// Per-channel notification settings
|
||||||
|
|||||||
@@ -211,8 +211,20 @@ func (h *Handler) List(w http.ResponseWriter, r *http.Request) {
|
|||||||
return
|
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")
|
w.Header().Set("Content-Type", "application/json")
|
||||||
json.NewEncoder(w).Encode(channels)
|
json.NewEncoder(w).Encode(filtered)
|
||||||
}
|
}
|
||||||
|
|
||||||
// @Summary Get a channel
|
// @Summary Get a channel
|
||||||
|
|||||||
@@ -26,15 +26,17 @@ type Handler struct {
|
|||||||
mentions *MentionHandler
|
mentions *MentionHandler
|
||||||
pushHandler *push.Handler
|
pushHandler *push.Handler
|
||||||
logger *slog.Logger
|
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{
|
return &Handler{
|
||||||
db: db,
|
db: db,
|
||||||
hub: hub,
|
hub: hub,
|
||||||
mentions: NewMentionHandler(db, pushHandler, logger),
|
mentions: NewMentionHandler(db, pushHandler, logger),
|
||||||
pushHandler: pushHandler,
|
pushHandler: pushHandler,
|
||||||
logger: logger,
|
logger: logger,
|
||||||
|
checker: checker,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -59,14 +61,12 @@ type bulkDeleteResponse struct {
|
|||||||
// and messages must be within the last 14 days.
|
// and messages must be within the last 14 days.
|
||||||
func (h *Handler) BulkDelete(w http.ResponseWriter, r *http.Request) {
|
func (h *Handler) BulkDelete(w http.ResponseWriter, r *http.Request) {
|
||||||
channelID := chi.URLParam(r, "channelID")
|
channelID := chi.URLParam(r, "channelID")
|
||||||
userID, serverID, ok := h.requireChannelAccess(w, r, channelID)
|
userID, serverID, ok := h.requireChannelAccess(w, r, channelID, 0)
|
||||||
if !ok {
|
if !ok {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
// ponytail: reuse existing auth structure; requireChannelAccess already verifies membership.
|
allowed, err := h.checker.CheckPermission(r.Context(), serverID, userID, permissions.MANAGE_MESSAGES)
|
||||||
// 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)
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
http.Error(w, `{"error":"server error"}`, http.StatusInternalServerError)
|
http.Error(w, `{"error":"server error"}`, http.StatusInternalServerError)
|
||||||
return
|
return
|
||||||
@@ -126,37 +126,6 @@ func (h *Handler) BulkDelete(w http.ResponseWriter, r *http.Request) {
|
|||||||
json.NewEncoder(w).Encode(bulkDeleteResponse{Deleted: int(deleted)})
|
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 {
|
type embedResponse struct {
|
||||||
ID string `json:"id"`
|
ID string `json:"id"`
|
||||||
URL string `json:"url"`
|
URL string `json:"url"`
|
||||||
@@ -208,7 +177,7 @@ type createMessageRequest struct {
|
|||||||
// @Router /channels/{channelID}/messages [post]
|
// @Router /channels/{channelID}/messages [post]
|
||||||
func (h *Handler) Create(w http.ResponseWriter, r *http.Request) {
|
func (h *Handler) Create(w http.ResponseWriter, r *http.Request) {
|
||||||
channelID := chi.URLParam(r, "channelID")
|
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 {
|
if !ok {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -494,7 +463,7 @@ func (h *Handler) Delete(w http.ResponseWriter, r *http.Request) {
|
|||||||
// @Router /channels/{channelID}/messages [get]
|
// @Router /channels/{channelID}/messages [get]
|
||||||
func (h *Handler) List(w http.ResponseWriter, r *http.Request) {
|
func (h *Handler) List(w http.ResponseWriter, r *http.Request) {
|
||||||
channelID := chi.URLParam(r, "channelID")
|
channelID := chi.URLParam(r, "channelID")
|
||||||
userID, _, ok := h.requireChannelAccess(w, r, channelID)
|
userID, _, ok := h.requireChannelAccess(w, r, channelID, permissions.VIEW_CHANNEL)
|
||||||
if !ok {
|
if !ok {
|
||||||
return
|
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.
|
// Search searches messages in a channel using PostgreSQL full-text search.
|
||||||
func (h *Handler) Search(w http.ResponseWriter, r *http.Request) {
|
func (h *Handler) Search(w http.ResponseWriter, r *http.Request) {
|
||||||
channelID := chi.URLParam(r, "channelID")
|
channelID := chi.URLParam(r, "channelID")
|
||||||
_, _, ok := h.requireChannelAccess(w, r, channelID)
|
_, _, ok := h.requireChannelAccess(w, r, channelID, permissions.VIEW_CHANNEL)
|
||||||
if !ok {
|
if !ok {
|
||||||
return
|
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.
|
// requireChannelAccess checks if user is authenticated and is a member of the channel's server.
|
||||||
// Returns (userID, serverID, ok).
|
// Returns (userID, serverID, ok). If required is non-zero, also checks the permission against
|
||||||
func (h *Handler) requireChannelAccess(w http.ResponseWriter, r *http.Request, channelID string) (string, string, bool) {
|
// 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())
|
userID, ok := middleware.UserIDFromContext(r.Context())
|
||||||
if !ok {
|
if !ok {
|
||||||
http.Error(w, `{"error":"unauthorized"}`, http.StatusUnauthorized)
|
http.Error(w, `{"error":"unauthorized"}`, http.StatusUnauthorized)
|
||||||
@@ -682,5 +652,19 @@ func (h *Handler) requireChannelAccess(w http.ResponseWriter, r *http.Request, c
|
|||||||
return "", "", false
|
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
|
return userID, serverID, true
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -80,3 +80,63 @@ func (c *Checker) CheckPermission(ctx context.Context, serverID string, userID s
|
|||||||
|
|
||||||
return Has(perms, required), nil
|
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
|
||||||
|
}
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ import { useAuthStore } from '../stores/auth.ts';
|
|||||||
import { usePushStore } from '../stores/push.ts';
|
import { usePushStore } from '../stores/push.ts';
|
||||||
|
|
||||||
export function UserSettings() {
|
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 { isSupported, isSubscribed, subscribe, unsubscribe, checkSubscription } = usePushStore();
|
||||||
const navigate = useNavigate();
|
const navigate = useNavigate();
|
||||||
|
|
||||||
@@ -81,7 +81,8 @@ export function UserSettings() {
|
|||||||
throw new Error(data?.message || data?.error || `Upload failed: ${response.status}`);
|
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) {
|
} catch (err) {
|
||||||
setUploadError(err instanceof Error ? err.message : 'Upload failed');
|
setUploadError(err instanceof Error ? err.message : 'Upload failed');
|
||||||
} finally {
|
} finally {
|
||||||
|
|||||||
@@ -31,6 +31,7 @@ export type BlockedUser = {
|
|||||||
};
|
};
|
||||||
|
|
||||||
export interface UpdateProfilePayload {
|
export interface UpdateProfilePayload {
|
||||||
|
avatar_url?: string;
|
||||||
display_name?: string;
|
display_name?: string;
|
||||||
bio?: string;
|
bio?: string;
|
||||||
accent_color?: string;
|
accent_color?: string;
|
||||||
|
|||||||
Reference in New Issue
Block a user