From 2caedc172b8bc500840e72aa1284250accf58819 Mon Sep 17 00:00:00 2001 From: hobokenchicken Date: Tue, 30 Jun 2026 13:29:34 -0400 Subject: [PATCH] fix: resolve 7 audit issues - webhook tokens, cookie security, avatar save, TUI fallback, dead code, valkey default --- ISSUES.md | 41 +++++++++------------ cmd/tui/auth.go | 17 ++++++--- internal/auth/cookie.go | 4 +-- internal/auth/handlers.go | 5 +-- internal/config/config.go | 2 +- internal/db/db.go | 3 ++ internal/message/handlers.go | 6 ---- internal/permissions/overrides.go | 60 ------------------------------- internal/webhook/handlers.go | 6 ++-- 9 files changed, 41 insertions(+), 103 deletions(-) diff --git a/ISSUES.md b/ISSUES.md index 23acafa..7825490 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -6,11 +6,9 @@ This document contains a comprehensive audit of the dumpsterChat repository, hig ## 1. Security Vulnerabilities & Bypasses -### 🚨 Plaintext Webhook Tokens Stored in Database (Task 8 Regression/Incomplete) +### 🚨 Plaintext Webhook Tokens Stored in Database (Task 8 Regression/Incomplete) ✅ RESOLVED * **Location:** [internal/db/db.go](file:///opt/dumpsterChat/internal/db/db.go#L171-L191), [internal/webhook/handlers.go](file:///opt/dumpsterChat/internal/webhook/handlers.go#L145-L151) -* **Description:** While a `token_hash` column and SHA-256 validation were added to execute webhooks securely, the database migrations did not drop the plaintext `token` column. Furthermore, when webhooks are created, the handler still inserts the plaintext token directly into the `token` column alongside `token_hash`. -* **Impact:** If the database is compromised, an attacker can retrieve all webhook tokens in plaintext, bypassing the security gains of hashing. -* **Remediation:** Drop the `token` column from the database schema and modify the `INSERT` query in `Create` webhook handler to only store `token_hash`. +* **Fix:** DROP COLUMN migration added, INSERT no longer stores plaintext `token`. ### 🚨 Message and Channel Access Bypasses Role Permissions & Overrides * **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go#L671-L692), [internal/channel/handlers.go](file:///opt/dumpsterChat/internal/channel/handlers.go#L165-L187) @@ -18,45 +16,41 @@ This document contains a comprehensive audit of the dumpsterChat repository, hig * **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 +### ⚠️ WebAuthn Session Cookie SameSite Laxity ✅ RESOLVED * **Location:** [internal/auth/cookie.go](file:///opt/dumpsterChat/internal/auth/cookie.go#L20) -* **Description:** The session cookie's `SameSite` attribute is set to `http.SameSiteDefaultMode` (which modern browsers default to `SameSite=Lax`). For session tokens, `StrictMode` or explicit `LaxMode` is highly recommended to protect against CSRF. -* **Remediation:** Change `SameSite` to `http.SameSiteStrictMode` or `http.SameSiteLaxMode` explicitly. +* **Fix:** Changed to `http.SameSiteStrictMode`. -### ⚠️ WebAuthn Session Cookie Secure Flag Relies on Env Var +### ⚠️ WebAuthn Session Cookie Secure Flag Relies on Env Var ✅ RESOLVED * **Location:** [internal/auth/cookie.go](file:///opt/dumpsterChat/internal/auth/cookie.go#L12-L19) -* **Description:** The `Secure` flag on the session cookie is determined by `os.Getenv("COOKIE_SECURE") == "true"`. If this environment variable is misconfigured or omitted in production, the session cookie will be sent over unencrypted HTTP connections. -* **Remediation:** Default `secure` to `true` or check if the incoming request is served over HTTPS to automatically enable/disable it. +* **Fix:** Default `secure` to `true` unless `COOKIE_SECURE=false` explicitly set. --- ## 2. Functional Bugs & Defects -### 🐛 User Avatar Cannot Be Changed -* **Location:** [internal/auth/handlers.go](file:///opt/dumpsterChat/internal/auth/handlers.go#L212-L225), [internal/upload/handlers.go](file:///opt/dumpsterChat/internal/upload/handlers.go#L121-L123), [web/src/components/UserSettings.tsx](file:///opt/dumpsterChat/web/src/components/UserSettings.tsx#L73-L84) -* **Description:** The file upload endpoint `/api/v1/upload` uploads a file and returns its URL, but does not associate it with the user. The `UpdateProfile` handler (`PATCH /auth/me`) does not accept an avatar field in its payload, meaning there is no way to update the user's avatar database column. The frontend settings page allows uploading a new avatar, but since it is never saved to the profile, it reverts to the Gravatar on the next load. -* **Remediation:** Add `AvatarURL` to the `updateProfileRequest` payload and update the database query in `UpdateProfile` to set the `avatar` column. +### 🐛 User Avatar Cannot Be Changed ✅ RESOLVED +* **Location:** [internal/auth/handlers.go](file:///opt/dumpsterChat/internal/auth/handlers.go#L212-L225) +* **Fix:** Added `AvatarURL` to `updateProfileRequest`, wired into SQL UPDATE for `avatar` column. -### 🐛 TUI Client Auth Failure Fallback Bug +### 🐛 TUI Client Auth Failure Fallback Bug ✅ RESOLVED * **Location:** [cmd/tui/auth.go](file:///opt/dumpsterChat/cmd/tui/auth.go#L28) -* **Description:** If `term.ReadPassword` fails during TUI login, the fallback code runs `fmt.Scanln(&email)` instead of reading the password, which overwrites the user's email with whatever password they type. -* **Remediation:** Change `fmt.Scanln(&email)` to read into a password variable (e.g. `var password string; fmt.Scanln(&password)`). +* **Fix:** Fallback reads into password variable instead of overwriting email. --- ## 3. Dead Code -### 🗑️ Unused `ComputeChannelPermissions` +### 🗑️ Unused `ComputeChannelPermissions` ✅ RESOLVED * **Location:** [internal/permissions/overrides.go](file:///opt/dumpsterChat/internal/permissions/overrides.go#L10) -* **Description:** The helper method `ComputeChannelPermissions` (which properly layers channel-level overrides on top of server-wide role permissions) is never called anywhere in the active codebase. +* **Fix:** Removed the unused function. ### 🗑️ Duplicated Permission Verification * **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go#L136-L164) * **Description:** The `checkPermission` method in `message.Handler` duplicates role checking queries instead of using the `permissions.Checker` struct. -### 🗑️ Obsolete Handler Type +### 🗑️ Obsolete Handler Type ✅ RESOLVED * **Location:** [internal/message/handlers.go](file:///opt/dumpsterChat/internal/message/handlers.go#L31-L35) -* **Description:** `Handler_old` is declared but never instantiated or used. +* **Fix:** Removed `Handler_old`. --- @@ -66,7 +60,6 @@ This document contains a comprehensive audit of the dumpsterChat repository, hig * **Description:** There are zero unit or integration tests in the Go backend or React frontend (`go test ./...` runs but finds no test files). * **Remediation:** Introduce tests for critical components such as permissions checking, message parsing, and session stores. -### 🔑 Hardcoded Valkey Password Placeholder +### 🔑 Hardcoded Valkey Password Placeholder ✅ RESOLVED * **Location:** [internal/config/config.go](file:///opt/dumpsterChat/internal/config/config.go#L111) -* **Description:** The default fallback `VALKEY_URL` contains a masked password placeholder (`redis://localhost:***@example.com`), which is invalid. -* **Remediation:** Default fallback to a simple local endpoint without authentication (e.g., `redis://localhost:6379`). +* **Fix:** Default fallback set to `redis://localhost:6379`. diff --git a/cmd/tui/auth.go b/cmd/tui/auth.go index 761e089..bd1609d 100644 --- a/cmd/tui/auth.go +++ b/cmd/tui/auth.go @@ -21,16 +21,23 @@ func RunAuth(api *APIClient) (string, error) { email, _ := reader.ReadString('\n') email = strings.TrimSpace(email) + var password string + fmt.Print(" Password: ") pwBytes, err := term.ReadPassword(int(syscall.Stdin)) if err != nil { // Fallback to plain read - fmt.Scanln(&email) - return "", fmt.Errorf("read password: %w", err) + var pwLine string + fmt.Scanln(&pwLine) + password = strings.TrimSpace(pwLine) + fmt.Println() + if password == "" { + return "", fmt.Errorf("read password: %w", err) + } + } else { + fmt.Println() // newline after password + password = strings.TrimSpace(string(pwBytes)) } - fmt.Println() // newline after password - - password := strings.TrimSpace(string(pwBytes)) if email == "" || password == "" { return "", fmt.Errorf("email and password are required") diff --git a/internal/auth/cookie.go b/internal/auth/cookie.go index 14fdfa5..d6bd3c0 100644 --- a/internal/auth/cookie.go +++ b/internal/auth/cookie.go @@ -9,7 +9,7 @@ import ( // SetSessionCookie writes the session cookie with secure defaults. // Shared by password login, registration, and WebAuthn login. func SetSessionCookie(w http.ResponseWriter, cookieName, token string, duration time.Duration) { - secure := os.Getenv("COOKIE_SECURE") == "true" + secure := os.Getenv("COOKIE_SECURE") != "false" http.SetCookie(w, &http.Cookie{ Name: cookieName, @@ -17,7 +17,7 @@ func SetSessionCookie(w http.ResponseWriter, cookieName, token string, duration Path: "/", HttpOnly: true, Secure: secure, - SameSite: http.SameSiteDefaultMode, + SameSite: http.SameSiteStrictMode, MaxAge: int(duration.Seconds()), }) } diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index 3e52f22..ba53c2b 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -210,6 +210,7 @@ type loginRequest struct { } type updateProfileRequest struct { + AvatarURL string `json:"avatar_url"` DisplayName string `json:"display_name"` Bio string `json:"bio"` AccentColor string `json:"accent_color"` @@ -482,9 +483,9 @@ func (h *Handler) UpdateProfile(w http.ResponseWriter, r *http.Request) { _, err := h.db.ExecContext(r.Context(), ` UPDATE users SET display_name = $2, bio = $3, accent_color = $4, status_text = $5, status = COALESCE(NULLIF($6, ''), status), - banner_url = NULLIF($7, ''), tagline = NULLIF($8, ''), pronouns = NULLIF($9, ''), social_links = $10 + banner_url = NULLIF($7, ''), tagline = NULLIF($8, ''), pronouns = NULLIF($9, ''), social_links = $10, avatar = COALESCE(NULLIF($11, ''), avatar) WHERE id = $1 - `, userID, req.DisplayName, req.Bio, req.AccentColor, req.StatusText, req.Status, req.BannerURL, req.Tagline, req.Pronouns, socialLinksJSON) + `, userID, req.DisplayName, req.Bio, req.AccentColor, req.StatusText, req.Status, req.BannerURL, req.Tagline, req.Pronouns, socialLinksJSON, req.AvatarURL) if err != nil { http.Error(w, `{"error":"update failed"}`, http.StatusInternalServerError) return diff --git a/internal/config/config.go b/internal/config/config.go index 28c21c3..8e2894d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -108,7 +108,7 @@ func Load() *Config { SSLMode: getEnv("POSTGRES_SSLMODE", "require"), }, Valkey: ValkeyConfig{ - URL: getEnv("VALKEY_URL", "redis://localhost:***@example.com"), + URL: getEnv("VALKEY_URL", "redis://localhost:6379"), }, MinIO: MinIOConfig{ Endpoint: getEnv("MINIO_ENDPOINT", ""), diff --git a/internal/db/db.go b/internal/db/db.go index 7448a9e..2814eb4 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -189,6 +189,9 @@ CREATE INDEX IF NOT EXISTS idx_webhooks_token ON webhooks(token); ALTER TABLE webhooks ADD COLUMN IF NOT EXISTS token_hash VARCHAR(64); CREATE INDEX IF NOT EXISTS idx_webhooks_token_hash ON webhooks(token_hash); +-- Drop plaintext token column — token_hash is the only stored credential +ALTER TABLE webhooks DROP COLUMN IF EXISTS token; + -- Ensure reply_to column exists on messages (added after initial table creation) ALTER TABLE messages ADD COLUMN IF NOT EXISTS reply_to UUID REFERENCES messages(id) ON DELETE SET NULL; diff --git a/internal/message/handlers.go b/internal/message/handlers.go index 8ff8b85..1929568 100644 --- a/internal/message/handlers.go +++ b/internal/message/handlers.go @@ -28,12 +28,6 @@ type Handler struct { logger *slog.Logger } -type Handler_old struct { - db *sql.DB - hub *gateway.Hub - mentions *MentionHandler -} - func NewHandler(db *sql.DB, hub *gateway.Hub, pushHandler *push.Handler, logger *slog.Logger) *Handler { return &Handler{ db: db, diff --git a/internal/permissions/overrides.go b/internal/permissions/overrides.go index 528e1ee..e116bff 100644 --- a/internal/permissions/overrides.go +++ b/internal/permissions/overrides.go @@ -5,66 +5,6 @@ import ( "database/sql" ) -// ComputeChannelPermissions returns effective permissions for a user in a specific channel, -// layering channel overrides on top of server-wide role permissions. -func (c *Checker) ComputeChannelPermissions(ctx context.Context, serverID, channelID, userID string) (int64, error) { - base, err := c.GetUserPermissions(ctx, serverID, userID) - if err != nil { - return 0, err - } - if Has(base, ADMINISTRATOR) { - return base, nil - } - - // Everyone role override applies first. - var everyoneID string - _ = c.db.QueryRowContext(ctx, `SELECT id FROM roles WHERE server_id = $1 AND is_default = TRUE LIMIT 1`, serverID).Scan(&everyoneID) - if everyoneID != "" { - allow, deny, err := c.getOverride(ctx, channelID, "role", everyoneID) - if err != nil { - return 0, err - } - base |= allow - base &^= deny - } - - // Member-specific role overrides. - rows, err := c.db.QueryContext(ctx, ` - SELECT r.id 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 0, err - } - defer rows.Close() - for rows.Next() { - var roleID string - if err := rows.Scan(&roleID); err != nil { - return 0, err - } - allow, deny, err := c.getOverride(ctx, channelID, "role", roleID) - if err != nil { - return 0, err - } - base |= allow - base &^= deny - } - if err := rows.Err(); err != nil { - return 0, err - } - - // User-specific override wins last. - allow, deny, err := c.getOverride(ctx, channelID, "user", userID) - if err != nil { - return 0, err - } - base |= allow - base &^= deny - - return base, nil -} - func (c *Checker) getOverride(ctx context.Context, channelID, targetType, targetID string) (allow, deny int64, err error) { err = c.db.QueryRowContext(ctx, ` SELECT allow_bitflags, deny_bitflags FROM channel_overrides diff --git a/internal/webhook/handlers.go b/internal/webhook/handlers.go index fc643f6..2a51a91 100644 --- a/internal/webhook/handlers.go +++ b/internal/webhook/handlers.go @@ -143,10 +143,10 @@ func (h *Handler) Create(w http.ResponseWriter, r *http.Request) { var avatar sql.NullString var createdAt sql.NullString err = h.db.QueryRowContext(r.Context(), ` - INSERT INTO webhooks (channel_id, name, token, token_hash, avatar, created_by) - VALUES ($1, $2, $3, $4, $5, $6) + INSERT INTO webhooks (channel_id, name, token_hash, avatar, created_by) + VALUES ($1, $2, $3, $4, $5) RETURNING id, channel_id, name, avatar, created_by, created_at::text - `, channelID, req.Name, token, tokenHash, req.Avatar, userID).Scan( + `, channelID, req.Name, tokenHash, req.Avatar, userID).Scan( &wh.ID, &wh.ChannelID, &wh.Name, &avatar, &wh.CreatedBy, &createdAt, ) if err != nil {