fix: address remaining code review findings (C-3, H-5, H-6, M-1 through M-16)

- 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
This commit is contained in:
jevb
2026-03-31 19:00:17 +02:00
parent 694007d5a4
commit 28f33644de
15 changed files with 166 additions and 95 deletions
+31 -21
View File
@@ -127,7 +127,7 @@ func handleRegister(database *db.DB) http.HandlerFunc {
registrationOpen, err := isRegistrationOpen(database)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to load registration policy",
})
return
@@ -143,7 +143,7 @@ func handleRegister(database *db.DB) http.HandlerFunc {
require2FA, err := isRequire2FAEnabled(database)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to load registration policy",
})
return
@@ -199,7 +199,7 @@ func handleRegister(database *db.DB) http.HandlerFunc {
hash, err := auth.HashPassword(req.Password)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to process registration",
})
return
@@ -218,7 +218,7 @@ func handleRegister(database *db.DB) http.HandlerFunc {
} else {
slog.Error("CreateUserWithInvite failed", "err", err, "username", req.Username)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "registration failed — please try again",
})
}
@@ -234,16 +234,16 @@ func handleRegister(database *db.DB) http.HandlerFunc {
token, err := auth.GenerateToken()
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to create session",
})
return
}
device := r.Header.Get("User-Agent")
device := truncateDevice(r.Header.Get("User-Agent"))
if _, err := database.CreateSession(uid, auth.HashToken(token), device, ip); err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to create session",
})
return
@@ -253,7 +253,7 @@ func handleRegister(database *db.DB) http.HandlerFunc {
if err != nil || user == nil {
slog.Error("failed to fetch user after registration", "user_id", uid, "error", err)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "registration succeeded but user fetch failed",
})
return
@@ -315,7 +315,7 @@ func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth.
// non-nil error here is a genuine DB failure.
slog.Error("login: GetUserByUsername failed", "err", err, "ip", ip)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "login temporarily unavailable",
})
return
@@ -352,16 +352,16 @@ func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth.
require2FA, err := isRequire2FAEnabled(database)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to load authentication policy",
})
return
}
if user.TOTPSecret != nil {
partialToken, err := partialStore.Issue(user.ID, r.Header.Get("User-Agent"), ip)
partialToken, err := partialStore.Issue(user.ID, truncateDevice(r.Header.Get("User-Agent")), ip)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to start two-factor challenge",
})
return
@@ -381,10 +381,10 @@ func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth.
}
// Issue session.
token, err := issueSession(database, user.ID, r.Header.Get("User-Agent"), ip)
token, err := issueSession(database, user.ID, truncateDevice(r.Header.Get("User-Agent")), ip)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to create session",
})
return
@@ -463,7 +463,7 @@ func handleVerifyTOTP(database *db.DB, partialStore *auth.PartialAuthStore) http
token, err := issueSession(database, user.ID, challenge.Device, challenge.IP)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to create session",
})
return
@@ -507,7 +507,7 @@ func handleEnableTOTP(pendingStore *auth.PendingTOTPStore) http.HandlerFunc {
secret, err := auth.GenerateTOTPSecret()
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to generate two-factor secret",
})
return
@@ -567,7 +567,7 @@ func handleConfirmTOTP(database *db.DB, pendingStore *auth.PendingTOTPStore) htt
if err := database.UpdateUserTOTPSecret(user.ID, &secret); err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to enable two-factor authentication",
})
return
@@ -607,7 +607,7 @@ func handleDisableTOTP(database *db.DB, pendingStore *auth.PendingTOTPStore) htt
require2FA, err := isRequire2FAEnabled(database)
if err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to load authentication policy",
})
return
@@ -623,7 +623,7 @@ func handleDisableTOTP(database *db.DB, pendingStore *auth.PendingTOTPStore) htt
pendingStore.Delete(user.ID)
if err := database.UpdateUserTOTPSecret(user.ID, nil); err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to disable two-factor authentication",
})
return
@@ -646,7 +646,7 @@ func handleLogout(database *db.DB) http.HandlerFunc {
if err := database.DeleteSession(sess.TokenHash); err != nil {
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to logout",
})
return
@@ -744,7 +744,7 @@ func handleDeleteAccount(database *db.DB, limiter *auth.RateLimiter) http.Handle
}
slog.Error("DeleteAccount failed", "err", err, "user_id", user.ID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to delete account",
})
return
@@ -777,6 +777,16 @@ func toUserResponse(u *db.User) *userResponse {
return resp
}
// truncateDevice truncates the User-Agent to prevent oversized session records.
const maxDeviceLen = 512
func truncateDevice(ua string) string {
if len(ua) > maxDeviceLen {
return ua[:maxDeviceLen]
}
return ua
}
func issueSession(database *db.DB, userID int64, device, ip string) (string, error) {
token, err := auth.GenerateToken()
if err != nil {
+12 -12
View File
@@ -105,7 +105,7 @@ func handleListChannels(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleListChannels ListChannels", "err", err)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to list channels",
})
return
@@ -119,7 +119,7 @@ func handleListChannels(database *db.DB) http.HandlerFunc {
if oErr != nil {
slog.Error("handleListChannels GetAllChannelPermissionsForRole", "err", oErr)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to fetch channel permissions",
})
return
@@ -153,7 +153,7 @@ func handleGetMessages(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleGetMessages GetChannel", "err", err, "channel_id", channelID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to look up channel",
})
return
@@ -236,7 +236,7 @@ func handleGetMessages(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleGetMessages GetMessagesForAPI", "err", err, "channel_id", channelID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to fetch messages",
})
return
@@ -309,7 +309,7 @@ func handleSearch(database *db.DB) http.HandlerFunc {
}
slog.Error("handleSearch SearchMessages", "err", err, "query", q)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "search failed",
})
return
@@ -325,7 +325,7 @@ func handleSearch(database *db.DB) http.HandlerFunc {
if oErr != nil {
slog.Error("handleSearch GetAllChannelPermissionsForRole", "err", oErr)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "search failed",
})
return
@@ -345,7 +345,7 @@ func handleSearch(database *db.DB) http.HandlerFunc {
if ctErr != nil {
slog.Error("handleSearch GetChannelTypes", "err", ctErr)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "search failed",
})
return
@@ -397,7 +397,7 @@ func handleGetPins(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleGetPins GetChannel", "err", err, "channel_id", channelID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to look up channel",
})
return
@@ -450,7 +450,7 @@ func handleGetPins(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleGetPins GetPinnedMessages", "err", err, "channel_id", channelID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to fetch pinned messages",
})
return
@@ -486,7 +486,7 @@ func handleSetPinned(database *db.DB, pinned bool) http.HandlerFunc {
if chErr != nil {
slog.Error("handleSetPinned GetChannel", "err", chErr, "channel_id", channelID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to look up channel",
})
return
@@ -534,7 +534,7 @@ func handleSetPinned(database *db.DB, pinned bool) http.HandlerFunc {
if err != nil {
slog.Error("handleSetPinned GetMessage", "err", err, "action", action, "message_id", messageID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to look up message",
})
return
@@ -550,7 +550,7 @@ func handleSetPinned(database *db.DB, pinned bool) http.HandlerFunc {
if err := database.SetMessagePinned(messageID, pinned); err != nil {
slog.Error("handleSetPinned SetMessagePinned", "err", err, "action", action, "message_id", messageID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to " + action + " message",
})
return
+5 -5
View File
@@ -88,7 +88,7 @@ func handleCreateDM(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleCreateDM GetUserByID", "err", err, "recipient_id", req.RecipientID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to look up recipient",
})
return
@@ -107,7 +107,7 @@ func handleCreateDM(database *db.DB) http.HandlerFunc {
slog.Error("handleCreateDM GetOrCreateDMChannel", "err", err,
"user_id", user.ID, "recipient_id", req.RecipientID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to create DM channel",
})
return
@@ -154,7 +154,7 @@ func handleListDMs(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleListDMs GetUserDMChannels", "err", err, "user_id", user.ID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to list DM channels",
})
return
@@ -187,7 +187,7 @@ func handleCloseDM(database *db.DB, broadcaster DMBroadcaster) http.HandlerFunc
slog.Error("handleCloseDM IsDMParticipant", "err", err,
"user_id", user.ID, "channel_id", channelID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to verify DM participation",
})
return
@@ -204,7 +204,7 @@ func handleCloseDM(database *db.DB, broadcaster DMBroadcaster) http.HandlerFunc
slog.Error("handleCloseDM CloseDM", "err", err,
"user_id", user.ID, "channel_id", channelID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "INTERNAL",
Error: "INTERNAL_ERROR",
Message: "failed to close DM",
})
return
+5 -5
View File
@@ -78,7 +78,7 @@ func handleCreateInvite(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleCreateInvite CreateInvite", "err", err, "user_id", user.ID)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to create invite",
})
return
@@ -88,7 +88,7 @@ func handleCreateInvite(database *db.DB) http.HandlerFunc {
if err != nil || inv == nil {
slog.Error("handleCreateInvite GetInvite", "err", err, "code", code)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to retrieve invite",
})
return
@@ -105,7 +105,7 @@ func handleListInvites(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleListInvites ListInvites", "err", err)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to list invites",
})
return
@@ -128,7 +128,7 @@ func handleRevokeInvite(database *db.DB) http.HandlerFunc {
if err != nil {
slog.Error("handleRevokeInvite GetInvite", "err", err, "code", code)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to look up invite",
})
return
@@ -144,7 +144,7 @@ func handleRevokeInvite(database *db.DB) http.HandlerFunc {
if err := database.RevokeInvite(code); err != nil {
slog.Error("handleRevokeInvite RevokeInvite", "err", err, "code", code)
writeJSON(w, http.StatusInternalServerError, errorResponse{
Error: "SERVER_ERROR",
Error: "INTERNAL_ERROR",
Message: "failed to revoke invite",
})
return
+12 -8
View File
@@ -324,16 +324,20 @@ func MaxBodySize(maxBytes int64) func(http.Handler) http.Handler {
}
}
// MaxBodySizeUnless is like MaxBodySize but skips the limit for specific paths.
// Exempted paths apply their own limit via route-scoped middleware.
func MaxBodySizeUnless(maxBytes int64, exemptPaths ...string) func(http.Handler) http.Handler {
exempt := make(map[string]bool, len(exemptPaths))
for _, p := range exemptPaths {
exempt[p] = true
}
// MaxBodySizeUnless is like MaxBodySize but skips the limit for paths that
// match any of the given prefixes. Exempted paths apply their own limit via
// route-scoped middleware.
func MaxBodySizeUnless(maxBytes int64, exemptPrefixes ...string) func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if !exempt[r.URL.Path] {
exempt := false
for _, prefix := range exemptPrefixes {
if strings.HasPrefix(r.URL.Path, prefix) {
exempt = true
break
}
}
if !exempt {
r.Body = http.MaxBytesReader(w, r.Body, maxBytes)
}
next.ServeHTTP(w, r)
+3 -1
View File
@@ -308,5 +308,7 @@ func requestLogger(next http.Handler) http.Handler {
func writeJSON(w http.ResponseWriter, status int, v any) {
w.Header().Set("Content-Type", "application/json; charset=utf-8")
w.WriteHeader(status)
_ = json.NewEncoder(w).Encode(v)
if err := json.NewEncoder(w).Encode(v); err != nil {
slog.Error("writeJSON: failed to encode response", "error", err)
}
}
+31 -4
View File
@@ -1,14 +1,17 @@
package api
import (
"errors"
"fmt"
"image"
_ "image/gif"
_ "image/jpeg"
_ "image/png"
"io"
"log/slog"
"mime"
"net/http"
"path/filepath"
"strings"
"time"
@@ -29,6 +32,29 @@ type uploadResponse struct {
Height *int `json:"height,omitempty"`
}
// sanitizeUploadFilename cleans an upload filename: strips control characters,
// removes path separators, and truncates to a safe length.
func sanitizeUploadFilename(name string) string {
// Strip path components — use only the base name.
name = filepath.Base(name)
// Remove control characters.
var sb strings.Builder
for _, r := range name {
if r >= 32 && r != 127 { // exclude control chars and DEL
sb.WriteRune(r)
}
}
name = strings.TrimSpace(sb.String())
// Truncate to 255 characters (filesystem limit).
if len(name) > 255 {
name = name[:255]
}
if name == "" || name == "." || name == ".." {
name = "unnamed"
}
return name
}
// MountUploadRoutes registers upload and file-serving endpoints.
// allowedOrigins controls the Access-Control-Allow-Origin header on served files.
func MountUploadRoutes(r chi.Router, database *db.DB, store *storage.Storage, allowedOrigins []string) {
@@ -68,7 +94,7 @@ func handleUpload(database *db.DB, store *storage.Storage) http.HandlerFunc {
// Detect MIME type from actual file bytes (never trust client header).
var sniffBuf [512]byte
n, readErr := file.Read(sniffBuf[:])
if readErr != nil && readErr.Error() != "EOF" && readErr.Error() != "unexpected EOF" {
if readErr != nil && !errors.Is(readErr, io.EOF) && !errors.Is(readErr, io.ErrUnexpectedEOF) {
writeJSON(w, http.StatusBadRequest, map[string]string{
"error": "BAD_REQUEST",
"message": "failed to read uploaded file",
@@ -114,7 +140,8 @@ func handleUpload(database *db.DB, store *storage.Storage) http.HandlerFunc {
}
// Insert attachment record in DB (unlinked — message_id is NULL).
if err := database.CreateAttachment(fileID, header.Filename, fileID, mime, header.Size, width, height); err != nil {
safeFilename := sanitizeUploadFilename(header.Filename)
if err := database.CreateAttachment(fileID, safeFilename, fileID, mime, header.Size, width, height); err != nil {
// Clean up stored file on DB failure.
_ = store.Delete(fileID)
slog.Error("failed to create attachment record", "error", err)
@@ -125,11 +152,11 @@ func handleUpload(database *db.DB, store *storage.Storage) http.HandlerFunc {
return
}
slog.Info("file uploaded", "id", fileID, "filename", header.Filename, "size", header.Size, "mime", mime)
slog.Info("file uploaded", "id", fileID, "filename", safeFilename, "size", header.Size, "mime", mime)
writeJSON(w, http.StatusCreated, uploadResponse{
ID: fileID,
Filename: header.Filename,
Filename: safeFilename,
Size: header.Size,
Mime: mime,
URL: "/api/v1/files/" + fileID,