Security audit — fix 17 vulnerabilities across 11 files

Critical:
- Timing attack on API key comparison → constant-time compare
- Timing attack on Torznab API key → constant-time compare

High:
- Permissive CORS wildcard with cookie auth → origin validation
- Missing security headers → X-Content-Type-Options, X-Frame-Options, XSS, Referrer
- Unbounded request body → 1MB limit middleware
- OPDS file serving path traversal → directory whitelist + symlink resolution
- SSRF via connection test endpoints → block metadata/link-local addresses

Medium:
- Password length uncapped at bcrypt 72-byte limit → enforce 6-72 chars
- Error messages leak internal details → generic responses
- Unbounded HTTP response reads → 2MB limit on external responses
- Settings/test endpoints not admin-restricted → require admin role
- CSV import goroutines use expired request context → background context
- Rate limiter XFF spoofing → use last hop, not first
- Rate limiter memory leak → periodic bucket cleanup
- Session store memory leak → periodic expired session cleanup
- OIDC state nonce memory leak → periodic cleanup

Low:
- Upload response leaks internal file paths → removed
- Health checks use http.DefaultClient without timeout → 10s timeout

Author: JeremiahM37

internal/api/admin.go

@@ -236,16 +236,18 @@ func (s *Server) handleAdminBulkCancel(w http.ResponseWriter, r *http.Request) {
 func (s *Server) handleAdminHealth(w http.ResponseWriter, _ *http.Request) {
 	checks := make([]map[string]interface{}, 0)
 
+	healthClient := &http.Client{Timeout: 10 * time.Second}
+
 	// Prowlarr.
 	if s.cfg.HasProwlarr() {
 		status := "ok"
 		detail := ""
 		req, _ := http.NewRequest("GET", s.cfg.ProwlarrURL+"/api/v1/health", nil)
 		req.Header.Set("X-Api-Key", s.cfg.ProwlarrAPIKey)
-		resp, err := http.DefaultClient.Do(req)
+		resp, err := healthClient.Do(req)
 		if err != nil {
 			status = "error"
-			detail = err.Error()
+			detail = "Connection failed"
 		} else {
 			resp.Body.Close()
 			if resp.StatusCode != 200 {
@@ -302,10 +304,10 @@ func (s *Server) handleAdminHealth(w http.ResponseWriter, _ *http.Request) {
 		detail := ""
 		req, _ := http.NewRequest("GET", s.cfg.ABSURL+"/api/libraries", nil)
 		req.Header.Set("Authorization", "Bearer "+s.cfg.ABSToken)
-		resp, err := http.DefaultClient.Do(req)
+		resp, err := healthClient.Do(req)
 		if err != nil {
 			status = "error"
-			detail = err.Error()
+			detail = "Connection failed"
 		} else {
 			resp.Body.Close()
 			if resp.StatusCode != 200 {
@@ -324,10 +326,10 @@ func (s *Server) handleAdminHealth(w http.ResponseWriter, _ *http.Request) {
 	if s.cfg.HasKavita() {
 		status := "ok"
 		detail := ""
-		resp, err := http.Get(s.cfg.KavitaURL + "/api/health")
+		resp, err := healthClient.Get(s.cfg.KavitaURL + "/api/health")
 		if err != nil {
 			status = "error"
-			detail = err.Error()
+			detail = "Connection failed"
 		} else {
 			resp.Body.Close()
 			if resp.StatusCode != 200 {
@@ -346,10 +348,10 @@ func (s *Server) handleAdminHealth(w http.ResponseWriter, _ *http.Request) {
 	if s.cfg.HasCalibre() && s.cfg.CalibreURL != "" {
 		status := "ok"
 		detail := ""
-		resp, err := http.Get(s.cfg.CalibreURL)
+		resp, err := healthClient.Get(s.cfg.CalibreURL)
 		if err != nil {
 			status = "error"
-			detail = err.Error()
+			detail = "Connection failed"
 		} else {
 			resp.Body.Close()
 			if resp.StatusCode >= 400 {

internal/api/auth.go

@@ -4,9 +4,9 @@ import (
 	"context"
 	"crypto/rand"
 	"crypto/sha256"
+	"crypto/subtle"
 	"encoding/hex"
 	"encoding/json"
-	"fmt"
 	"log/slog"
 	"net/http"
 	"strconv"
@@ -51,10 +51,32 @@ type SessionStore struct {
 
 // NewSessionStore creates a new session store.
 func NewSessionStore() *SessionStore {
-	return &SessionStore{
+	s := &SessionStore{
 		sessions:    make(map[string]*SessionData),
 		pendingTOTP: make(map[string]*PendingTOTP),
 	}
+
+	// Periodically clean up expired sessions and pending TOTP tokens.
+	go func() {
+		ticker := time.NewTicker(10 * time.Minute)
+		for range ticker.C {
+			now := time.Now()
+			s.mu.Lock()
+			for token, data := range s.sessions {
+				if now.After(data.Expiry) {
+					delete(s.sessions, token)
+				}
+			}
+			for token, pending := range s.pendingTOTP {
+				if now.After(pending.Expiry) {
+					delete(s.pendingTOTP, token)
+				}
+			}
+			s.mu.Unlock()
+		}
+	}()
+
+	return s
 }
 
 // Create generates a new session token for a user, valid for 24 hours.
@@ -204,7 +226,7 @@ func authMiddleware(cfg *config.Config, database *db.DB, sessions *SessionStore,
 			if apiKey == "" {
 				apiKey = r.URL.Query().Get("apikey")
 			}
-			if apiKey == cfg.APIKey {
+			if subtle.ConstantTimeCompare([]byte(apiKey), []byte(cfg.APIKey)) == 1 {
 				// API key users get admin-level access.
 				ctx := context.WithValue(r.Context(), ctxUserRole, "admin")
 				ctx = context.WithValue(ctx, ctxUsername, "api")
@@ -490,10 +512,17 @@ func handleRegister(database *db.DB, sessions *SessionStore) http.HandlerFunc {
 			return
 		}
 
-		if len(req.Username) < 3 || len(req.Password) < 6 {
+		if len(req.Username) < 3 || len(req.Username) > 64 {
+			writeJSON(w, http.StatusBadRequest, map[string]interface{}{
+				"success": false,
+				"error":   "Username must be 3-64 characters",
+			})
+			return
+		}
+		if len(req.Password) < 6 || len(req.Password) > 72 {
 			writeJSON(w, http.StatusBadRequest, map[string]interface{}{
 				"success": false,
-				"error":   "Username must be at least 3 characters, password at least 6",
+				"error":   "Password must be 6-72 characters",
 			})
 			return
 		}
@@ -737,9 +766,10 @@ func handleDeleteUser(database *db.DB) http.HandlerFunc {
 		}
 
 		if err := database.DeleteUser(id); err != nil {
+			slog.Error("failed to delete user", "id", id, "error", err)
 			writeJSON(w, http.StatusNotFound, map[string]interface{}{
 				"success": false,
-				"error":   fmt.Sprintf("Failed to delete user: %s", err.Error()),
+				"error":   "Failed to delete user",
 			})
 			return
 		}

internal/api/csv.go

@@ -1,11 +1,13 @@
 package api
 
 import (
+	"context"
 	"encoding/csv"
 	"io"
 	"log/slog"
 	"net/http"
 	"strings"
+	"time"
 )
 
 func (s *Server) handleCSVImport(w http.ResponseWriter, r *http.Request) {
@@ -93,6 +95,7 @@ func (s *Server) handleCSVImport(w http.ResponseWriter, r *http.Request) {
 		}
 
 		// Search all sources and queue best result.
+		// Use background context since the goroutine outlives the HTTP request.
 		go func(title, author, mediaType string) {
 			tab := "main"
 			if mediaType == "audiobook" {
@@ -106,7 +109,9 @@ func (s *Server) handleCSVImport(w http.ResponseWriter, r *http.Request) {
 				query = title + " " + author
 			}
 
-			results, _ := s.searchMgr.Search(r.Context(), tab, query)
+			ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
+			defer cancel()
+			results, _ := s.searchMgr.Search(ctx, tab, query)
 			if len(results) == 0 {
 				slog.Warn("CSV import: no results", "title", title)
 				return

internal/api/oidc.go

@@ -63,7 +63,7 @@ func NewOIDCHandler(cfg *config.Config, database *db.DB, sessions *SessionStore)
 
 	slog.Info("OIDC provider initialized", "issuer", cfg.OIDCIssuer, "provider_name", cfg.OIDCProviderName)
 
-	return &OIDCHandler{
+	h := &OIDCHandler{
 		cfg:      cfg,
 		db:       database,
 		sessions: sessions,
@@ -72,6 +72,23 @@ func NewOIDCHandler(cfg *config.Config, database *db.DB, sessions *SessionStore)
 		oauth2:   oauth2Cfg,
 		states:   make(map[string]time.Time),
 	}
+
+	// Periodically clean up expired OIDC state nonces.
+	go func() {
+		ticker := time.NewTicker(5 * time.Minute)
+		for range ticker.C {
+			h.mu.Lock()
+			now := time.Now()
+			for state, expiry := range h.states {
+				if now.After(expiry) {
+					delete(h.states, state)
+				}
+			}
+			h.mu.Unlock()
+		}
+	}()
+
+	return h
 }
 
 // generateState creates a random state string for CSRF protection.

internal/api/opds.go

@@ -238,21 +238,49 @@ func (s *Server) handleOPDSDownload(w http.ResponseWriter, r *http.Request) {
 				http.Error(w, "File not found on disk", http.StatusNotFound)
 				return
 			}
-			if _, err := os.Stat(item.FilePath); os.IsNotExist(err) {
+
+			// Resolve the real path to prevent path traversal via symlinks or DB tampering.
+			realPath, err := filepath.EvalSymlinks(item.FilePath)
+			if err != nil {
+				http.Error(w, "File not found on disk", http.StatusNotFound)
+				return
+			}
+
+			// Validate the file is within an allowed directory.
+			allowed := false
+			for _, dir := range []string{s.cfg.EbookDir, s.cfg.AudiobookDir, s.cfg.MangaDir, s.cfg.IncomingDir, s.cfg.MangaIncomingDir} {
+				if dir == "" {
+					continue
+				}
+				absDir, err := filepath.Abs(dir)
+				if err != nil {
+					continue
+				}
+				if strings.HasPrefix(realPath, absDir+string(filepath.Separator)) || realPath == absDir {
+					allowed = true
+					break
+				}
+			}
+			if !allowed {
+				http.Error(w, "Access denied", http.StatusForbidden)
+				return
+			}
+
+			if _, err := os.Stat(realPath); os.IsNotExist(err) {
 				http.Error(w, "File not found on disk", http.StatusNotFound)
 				return
 			}
 
-			fmtStr := strings.ToLower(strings.TrimPrefix(filepath.Ext(item.FilePath), "."))
+			fmtStr := strings.ToLower(strings.TrimPrefix(filepath.Ext(realPath), "."))
 			mime := formatMIMEs[fmtStr]
 			if mime == "" {
 				mime = "application/octet-stream"
 			}
 
 			w.Header().Set("Content-Type", mime)
 			w.Header().Set("Content-Disposition",
-				fmt.Sprintf("attachment; filename=\"%s\"", filepath.Base(item.FilePath)))
-			http.ServeFile(w, r, item.FilePath)
+				fmt.Sprintf("attachment; filename=%q", filepath.Base(realPath)))
+			http.ServeFile(w, r, realPath)
 			return
 		}
 	}

internal/api/ratelimit.go

@@ -27,11 +27,34 @@ type rateLimitBucket struct {
 
 // NewRateLimiter creates a rate limiter with the given window and rules.
 func NewRateLimiter(windowSec int, rules map[string]int) *RateLimiter {
-	return &RateLimiter{
+	rl := &RateLimiter{
 		windowSec: windowSec,
 		rules:     rules,
 		buckets:   make(map[rateLimitKey]*rateLimitBucket),
 	}
+	// Periodically clean up stale buckets to prevent unbounded memory growth.
+	go func() {
+		ticker := time.NewTicker(5 * time.Minute)
+		for range ticker.C {
+			rl.cleanup()
+		}
+	}()
+	return rl
+}
+
+// cleanup removes expired buckets.
+func (rl *RateLimiter) cleanup() {
+	now := time.Now()
+	cutoff := now.Add(-time.Duration(rl.windowSec) * time.Second)
+
+	rl.mu.Lock()
+	defer rl.mu.Unlock()
+
+	for key, bucket := range rl.buckets {
+		if len(bucket.timestamps) == 0 || bucket.timestamps[len(bucket.timestamps)-1].Before(cutoff) {
+			delete(rl.buckets, key)
+		}
+	}
 }
 
 func (rl *RateLimiter) ruleForPath(path string) string {
@@ -116,9 +139,14 @@ func RateLimitMiddleware(rl *RateLimiter, next http.Handler) http.Handler {
 			return
 		}
 
-		identity := r.Header.Get("X-Forwarded-For")
-		if identity == "" {
-			identity = r.RemoteAddr
+		// Use RemoteAddr as primary identity. X-Forwarded-For is trivially
+		// spoofable unless behind a trusted reverse proxy. If behind a proxy,
+		// take only the rightmost (last hop before our server) IP from XFF.
+		identity := r.RemoteAddr
+		if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
+			parts := strings.Split(xff, ",")
+			// Use the last entry (closest proxy hop) -- less spoofable than first.
+			identity = strings.TrimSpace(parts[len(parts)-1])
 		}
 
 		allowed, retryAfter, rule, limit := rl.Check(identity, r.URL.Path)