From ff59d9f79965fc3ddc4d8d7cfdbde87fe5e8b28d Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Sat, 4 Apr 2026 12:40:37 +0300 Subject: [PATCH] fix: security hardening for middleware, crypto, and backup handlers - Remove CORS origin reflection (SEC-C1 CRITICAL) - Add Content-Security-Policy header (SEC-H2) - Fix rate limiter memory leak with periodic stale IP cleanup (SEC-H5) - Enforce minimum 32-char ENCRYPTION_KEY (SEC-H4) - Validate backup type against allowlist (SEC-M6) - Fix backup download path traversal with path containment check (SEC-C2 CRITICAL) --- internal/api/backups.go | 26 ++++++++++++++++++-- internal/api/middleware.go | 49 ++++++++++++++++++++++++++------------ internal/backup/engine.go | 8 +++++++ internal/crypto/crypto.go | 3 +++ 4 files changed, 69 insertions(+), 17 deletions(-) diff --git a/internal/api/backups.go b/internal/api/backups.go index 88080eb..95a2e3e 100644 --- a/internal/api/backups.go +++ b/internal/api/backups.go @@ -6,6 +6,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "time" "github.com/alexei/docker-watcher/internal/store" @@ -69,14 +70,35 @@ func (s *Server) downloadBackup(w http.ResponseWriter, r *http.Request) { } filePath := s.backupEngine.FilePath(backup) - if _, err := os.Stat(filePath); err != nil { + + // Validate the resolved path stays within the backup directory to prevent path traversal. + absPath, err := filepath.Abs(filePath) + if err != nil { + respondError(w, http.StatusInternalServerError, "failed to resolve backup path") + return + } + absBackupDir, _ := filepath.Abs(s.backupEngine.BackupDir()) + if !strings.HasPrefix(absPath, absBackupDir+string(filepath.Separator)) { + respondError(w, http.StatusForbidden, "access denied") + return + } + + f, err := os.Open(absPath) + if err != nil { respondError(w, http.StatusNotFound, "backup file not found on disk") return } + defer f.Close() + + stat, err := f.Stat() + if err != nil { + respondError(w, http.StatusInternalServerError, "failed to read backup file") + return + } w.Header().Set("Content-Type", "application/octet-stream") w.Header().Set("Content-Disposition", "attachment; filename=\""+filepath.Base(backup.Filename)+"\"") - http.ServeFile(w, r, filePath) + http.ServeContent(w, r, filepath.Base(backup.Filename), stat.ModTime(), f) } // deleteBackup handles DELETE /api/backups/{id}. diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 281a1f9..e2692c6 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -45,23 +45,20 @@ func securityHeaders(next http.Handler) http.Handler { w.Header().Set("X-Content-Type-Options", "nosniff") w.Header().Set("X-Frame-Options", "DENY") w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin") + w.Header().Set("Content-Security-Policy", "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self'; font-src 'self'") next.ServeHTTP(w, r) }) } -// cors is an HTTP middleware that restricts CORS to same-origin requests. -// The frontend is served from the same origin, so no wildcard is needed. +// cors is an HTTP middleware that handles CORS for same-origin requests. +// The frontend is served from the same origin, so cross-origin requests are not expected. func cors(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - origin := r.Header.Get("Origin") - if origin != "" { - // Only allow the same origin (frontend is served from the same host). - w.Header().Set("Access-Control-Allow-Origin", origin) - w.Header().Set("Vary", "Origin") - w.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") - w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization") - w.Header().Set("Access-Control-Allow-Credentials", "true") - } + // The frontend is served from the same origin, so cross-origin + // requests are not expected. We do NOT reflect the Origin header + // back, as that would allow any website to make credentialed requests. + // If cross-origin support is needed in the future, maintain an + // explicit allowlist of trusted origins here. if r.Method == http.MethodOptions { w.WriteHeader(http.StatusNoContent) @@ -84,12 +81,16 @@ func limitBody(next http.Handler) http.Handler { // rateLimiter provides per-IP rate limiting for login endpoints. type rateLimiter struct { - mu sync.Mutex - attempts map[string][]time.Time + mu sync.Mutex + attempts map[string][]time.Time + lastCleanup time.Time } func newRateLimiter() *rateLimiter { - return &rateLimiter{attempts: make(map[string][]time.Time)} + return &rateLimiter{ + attempts: make(map[string][]time.Time), + lastCleanup: time.Now(), + } } // allow checks if the IP is allowed to make another request. @@ -101,7 +102,25 @@ func (rl *rateLimiter) allow(ip string) bool { now := time.Now() window := now.Add(-1 * time.Minute) - // Clean old entries. + // Periodically clean all stale IPs to prevent memory leak. + if now.Sub(rl.lastCleanup) > 5*time.Minute { + for k, times := range rl.attempts { + filtered := times[:0] + for _, t := range times { + if t.After(window) { + filtered = append(filtered, t) + } + } + if len(filtered) == 0 { + delete(rl.attempts, k) + } else { + rl.attempts[k] = filtered + } + } + rl.lastCleanup = now + } + + // Clean old entries for this IP. filtered := rl.attempts[ip][:0] for _, t := range rl.attempts[ip] { if t.After(window) { diff --git a/internal/backup/engine.go b/internal/backup/engine.go index 3421748..edacbdc 100644 --- a/internal/backup/engine.go +++ b/internal/backup/engine.go @@ -40,6 +40,14 @@ func (e *Engine) BackupDir() string { // CreateBackup creates a new database backup using VACUUM INTO. // Returns the backup metadata record. func (e *Engine) CreateBackup(backupType string) (store.Backup, error) { + // Validate backup type to prevent path traversal via filename. + switch backupType { + case "manual", "auto", "pre-restore": + // valid + default: + return store.Backup{}, fmt.Errorf("invalid backup type: %q", backupType) + } + e.mu.Lock() defer e.mu.Unlock() diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index ac8ccb4..5d61e1f 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -28,6 +28,9 @@ func KeyFromEnv() ([32]byte, error) { if raw == "" { return [32]byte{}, ErrNoKey } + if len(raw) < 32 { + return [32]byte{}, fmt.Errorf("ENCRYPTION_KEY must be at least 32 characters long (got %d)", len(raw)) + } return DeriveKey(raw), nil }