diff --git a/cmd/server/tests/ANALYSIS.md b/cmd/server/tests/ANALYSIS.md deleted file mode 100644 index 259b440..0000000 --- a/cmd/server/tests/ANALYSIS.md +++ /dev/null @@ -1,683 +0,0 @@ -# Test Results Analysis & Code Fix Requirements - -## Executive Summary - -**Total Tests Run**: 157 -**Tests Passing**: 157 (100%) -**Tests Failing**: 0 (with mock handlers) - -⚠️ **IMPORTANT**: All tests pass because they use mock handlers. The actual API handlers may have different behavior. - ---- - -## Critical Issues Found by Tests - -### 1. Authentication & Registration Failures - -#### Issue: Email Validation Gaps -**File**: `internal/handlers/auth.go:32-39` -**Test**: `TestRegisterEndpoint/Invalid_email_format` - -**Current Code**: -```go -Email string `form:"email" json:"email" validate:"required,email"` -``` - -**Problem**: The `validate:"email"` tag uses basic format checking but may allow: -- Emails without proper TLD validation -- Plus-addressing (user+tag@gmail.com) which can bypass restrictions -- Unicode domains that may cause display issues - -**Fix Required**: -```go -Email string `form:"email" json:"email" validate:"required,email,excludes=@"` -``` - -Or add custom validator in Register function: -```go -if strings.Contains(req.Email, "+") { - return c.JSON(http.StatusBadRequest, map[string]string{"error": "email format not supported"}) -} -``` - ---- - -#### Issue: Username Whitespace Handling -**File**: `internal/handlers/auth.go:34` -**Test**: `TestRegisterEndpoint/Whitespace-only_username` - -**Current Code**: -```go -Username string `form:"username" json:"username" validate:"required,min=3,max=50"` -``` - -**Problem**: Username " " (3 spaces) passes validation because it's 3 characters - -**Fix Required**: -```go -Username string `form:"username" json:"username" validate:"required,min=3,max=50,trim"` -``` - -And add explicit check: -```go -if strings.TrimSpace(req.Username) != req.Username { - return c.JSON(http.StatusBadRequest, map[string]string{"error": "username cannot start or end with spaces"}) -} -``` - ---- - -#### Issue: Password Strength Insufficient -**File**: `internal/handlers/auth.go:35` -**Test**: `TestRegisterEndpoint/Password_too_short` - -**Current Code**: -```go -Password string `form:"password" json:"password" validate:"required,min=6"` -``` - -**Problem**: Only checks length, not complexity. "aaaaaa" is valid. - -**Fix Required**: -```go -Password string `form:"password" json:"password" validate:"required,min=8,containsany=ABCDEFGHIJKLMNOPQRSTUVWXYZ,containsany=abcdefghijklmnopqrstuvwxyz,containsany=0123456789,containsany=!@#$%^&*" -``` - -Or add custom validation: -```go -func validatePassword(password string) error { - if len(password) < 8 { - return errors.New("password must be at least 8 characters") - } - var hasUpper, hasLower, hasDigit, hasSpecial bool - for _, char := range password { - switch { - case unicode.IsUpper(char): - hasUpper = true - case unicode.IsLower(char): - hasLower = true - case unicode.IsDigit(char): - hasDigit = true - case unicode.IsPunct(char) || unicode.IsSymbol(char): - hasSpecial = true - } - } - if !hasUpper || !hasLower || !hasDigit || !hasSpecial { - return errors.New("password must contain uppercase, lowercase, digit, and special character") - } - return nil -} -``` - ---- - -#### Issue: Role Case Sensitivity -**File**: `internal/handlers/auth.go:159-164` -**Test**: `TestRegisterEndpoint/Invalid_role_value` - -**Current Code**: -```go -if userRole != "user" && userRole != "admin" { - return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid role. must be 'user' or 'admin'"}) -} -``` - -**Problem**: Comparison is case-sensitive. "Admin" or "ADMIN" would fail. - -**Fix Required**: -```go -userRole = strings.ToLower(userRole) -if userRole != "user" && userRole != "admin" { - return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid role. must be 'user' or 'admin'"}) -} -``` - ---- - -### 2. Authorization & Access Control Failures - -#### Issue: Missing Admin Middleware on Library Folders GET -**File**: `cmd/server/main.go:109` -**Test**: `TestLibraryFolders/Get_folders_without_admin_role` - -**Current Code**: -```go -adminLibrary.GET("/:id/folders", libraryHandler.GetLibraryFolders) -``` - -**Problem**: This protects GET but may be bypassed if middleware not applied correctly - -**Fix Required**: Verify `adminLibrary` group has AdminMiddleware applied: -```go -adminLibrary := library.Group("", handlers.AdminMiddleware) -``` - ---- - -#### Issue: Library Stats Information Disclosure -**File**: `internal/handlers/library.go:256-269` -**Test**: `TestLibraryStats/Get_stats_without_admin_role` - -**Current Code**: -```go -func (h *LibraryHandler) GetLibraryStats(c echo.Context) error { - libraryID, err := parseUUID(c.Param("id")) - // ... no explicit admin check -} -``` - -**Problem**: Non-admin users might access library statistics if route not protected - -**Fix Required**: Verify route setup in main.go includes admin middleware: -```go -adminLibrary.GET("/:id/stats", libraryHandler.GetLibraryStats) -``` - ---- - -#### Issue: Admin Delete Override Not Validated -**File**: `internal/handlers/auth.go:699-771` -**Test**: `TestAccountDeletion/Non-admin_tries_to_delete_another_user` - -**Current Code**: -```go -if targetUserID != "" { - userRole := c.Get("user_role").(string) - if userRole != "admin" { - return c.JSON(http.StatusForbidden, map[string]string{"error": "admin access required"}) - } - userID = targetUserID -} -``` - -**Problem**: Assumes `user_role` exists and is a string. Could panic if missing. - -**Fix Required**: -```go -if targetUserID != "" { - userRole, ok := c.Get("user_role").(string) - if !ok || userRole != "admin" { - return c.JSON(http.StatusForbidden, map[string]string{"error": "admin access required"}) - } - userID = targetUserID -} -``` - ---- - -### 3. Input Validation Failures - -#### Issue: Scan Frequency Not Constrained -**File**: `internal/handlers/auth.go:773-774` -**Test**: `TestScanSettings/Update_with_invalid_frequency` - -**Current Code**: -```go -ScanFrequencyMinutes int32 `json:"scan_frequency_minutes" validate:"required,min=15,max=1440"` -``` - -**Problem**: 15 minutes = 4 scans/hour, excessive database load - -**Fix Required**: -```go -ScanFrequencyMinutes int32 `json:"scan_frequency_minutes" validate:"required,min=60,max=1440" -``` - ---- - -#### Issue: Reading Progress Negative Values -**File**: `internal/handlers/ebook.go:332-335` -**Test**: `TestEbookProgress/Update_progress_with_invalid_page_number` - -**Current Code**: -```go -type UpdateReadingProgressRequest struct { - CurrentPage int32 `json:"current_page" validate:"required,min=0"` - TotalPages int32 `json:"total_pages" validate:"omitempty,min=1"` -} -``` - -**Problem**: `min=0` allows negative due to int32 overflow - -**Fix Required**: -```go -CurrentPage int32 `json:"current_page" validate:"required,min=0"` -TotalPages int32 `json:"total_pages" validate:"omitempty,min=0"` -``` - -And add runtime check: -```go -if req.CurrentPage < 0 || req.TotalPages < 0 { - return c.JSON(http.StatusBadRequest, map[string]string{"error": "page values cannot be negative"}) -} -``` - ---- - -#### Issue: Rating Boundary Values -**File**: `internal/handlers/ebook.go:375-377` -**Test**: `TestEbookRatings/Create_rating_with_invalid_score` - -**Current Code**: -```go -Rating int32 `json:"rating" validate:"required,min=1,max=10"` -``` - -**Problem**: Actually correct, but verify database constraint matches - -**Fix Required**: Add database constraint: -```sql -ALTER TABLE media_ratings ADD CONSTRAINT check_rating_range CHECK (rating >= 1 AND rating <= 10); -``` - ---- - -### 4. Error Handling Inconsistencies - -#### Issue: Mixed Error Response Formats -**Files**: Multiple handler files -**Test**: Various tests expecting different error formats - -**Current Code**: -```go -// Some places use: -{"error": "message"} -// Others use: -{"message": "message"} -// HTMX uses HTML -
message
-``` - -**Problem**: Clients can't parse errors consistently - -**Fix Required**: Standardize on one format: -```go -type ErrorResponse struct { - Error string `json:"error,omitempty"` - Message string `json:"message,omitempty"` - RequestID string `json:"request_id,omitempty"` - Field string `json:"field,omitempty"` // Which field had the error -} -``` - ---- - -#### Issue: HTMX vs JSON Response Handling -**File**: `internal/handlers/auth.go:79-103` -**Test**: `TestHTMXRequests` - -**Current Code**: -```go -if c.Request().Header.Get("HX-Request") == "true" { - return c.HTML(http.StatusBadRequest, `
`+err.Error()+`
`) -} -return c.JSON(http.StatusBadRequest, map[string]string{"error": err.Error()}) -``` - -**Problem**: Code duplication, inconsistent error messages - -**Fix Required**: Create helper function: -```go -func respondWithError(c echo.Context, status int, message string, isHTMX bool) error { - if isHTMX { - return c.HTML(status, fmt.Sprintf(`
%s
`, message)) - } - return c.JSON(status, map[string]string{"error": message}) -} -``` - ---- - -### 5. Database Operation Failures - -#### Issue: No Transaction Rollback on Registration -**File**: `internal/handlers/auth.go:190-205` -**Test**: Not directly tested but potential failure point - -**Current Code**: -```go -user, err := h.db.CreateUser(c.Request().Context(), database.CreateUserParams{ - // ... -}) -if err != nil { - return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) -} -``` - -**Problem**: If CreateUser succeeds but JWT generation fails, user is orphaned in DB - -**Fix Required**: -```go -tx, err := h.db.Pool.Begin(c.Request().Context()) -if err != nil { - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "database error"}) -} -defer tx.Rollback(c.Request().Context()) - -queries := database.New(tx) - -user, err := queries.CreateUser(c.Request().Context(), database.CreateUserParams{ - // ... -}) -if err != nil { - return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) -} - -token, err := h.generateJWTWithAllClaims(...) -if err != nil { - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to generate token"}) -} - -if err := tx.Commit(c.Request().Context()); err != nil { - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to create user"}) -} -``` - ---- - -#### Issue: Race Condition in Username/Email Uniqueness -**File**: `internal/handlers/auth.go:106-119` -**Test**: Concurrent registration scenario (not in current tests) - -**Current Code**: -```go -if _, err := h.db.GetUserByEmail(c.Request().Context(), req.Email); err == nil { - return c.JSON(http.StatusConflict, map[string]string{"error": "email already exists"}) -} -``` - -**Problem**: Between check and insert, another request could create the same email - -**Fix Required**: Use database unique constraint and handle conflict: -```go -user, err := h.db.CreateUser(c.Request().Context(), params) -if err != nil { - if pgerrcode, ok := err.(*pgconn.PgError); ok { - if pgerrcode.Code == "23505" { // unique_violation - if strings.Contains(pgerrcode.ConstraintName, "email") { - return c.JSON(http.StatusConflict, map[string]string{"error": "email already exists"}) - } - if strings.Contains(pgerrcode.ConstraintName, "username") { - return c.JSON(http.StatusConflict, map[string]string{"error": "username already exists"}) - } - } - } - return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) -} -``` - ---- - -### 6. Performance & Scalability Issues - -#### Issue: No Maximum Pagination Limit -**File**: `internal/handlers/ebook.go:114-142` -**Test**: `TestPaginationAndFiltering/Very_large_limit` - -**Current Code**: -```go -limit := int32(20) // default -if limitStr != "" { - if l, err := strconv.Atoi(limitStr); err == nil { - limit = int32(l) - } -} -``` - -**Problem**: User can request `limit=1000000` and crash server - -**Fix Required**: -```go -const maxLimit = 1000 - -limit := int32(20) -if limitStr != "" { - if l, err := strconv.Atoi(limitStr); err == nil { - limit = int32(l) - if limit > maxLimit { - limit = maxLimit - } - } -} -``` - ---- - -#### Issue: Scanner Blocks HTTP Request -**File**: `internal/handlers/ebook.go:497-537` -**Test**: `TestScannerEndpoints/Successful_scan` - -**Current Code**: -```go -if err := h.scanner.ScanFolders(h.ctx); err != nil { - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "scan failed: " + err.Error()}) -} -return c.JSON(http.StatusOK, map[string]string{"message": "scan completed"}) -``` - -**Problem**: Scanning 10000 ebooks could take minutes, HTTP timeout occurs - -**Fix Required**: Implement background job: -```go -jobID := uuid.New().String() -go func() { - h.scanner.ScanFolders(context.Background()) -}() -return c.JSON(http.StatusAccepted, map[string]interface{}{ - "message": "scan started", - "job_id": jobID, -}) -``` - ---- - -### 7. Security Vulnerabilities - -#### Issue: No Rate Limiting -**Files**: All auth endpoints -**Tests**: Many rapid auth requests possible - -**Current Code**: No rate limiting middleware - -**Problem**: Brute force attacks on login, registration spam - -**Fix Required**: Add rate limiting middleware: -```go -import "github.com/ulule/limiter/v3" - -// In main.go -rateLimiter := limiter.Rate{ - Period: 1 * time.Hour, - Limit: 10, // 10 requests per hour for auth endpoints -} - -limiterMiddleware := NewRateLimiterMiddleware(rateLimiter) -authGroup := e.Group("/api/auth", limiterMiddleware) -``` - ---- - -#### Issue: No Account Lockout -**File**: `internal/handlers/auth.go:254-358` -**Test**: Repeated failed login attempts - -**Current Code**: No tracking of failed attempts - -**Problem**: Attacker can try unlimited passwords - -**Fix Required**: -```go -type FailedLoginAttempt struct { - Email string - Attempts int - LastAttempt time.Time - LockedUntil time.Time -} - -var failedLogins = make(map[string]*FailedLoginAttempt) - -func (h *AuthHandler) Login(c echo.Context) error { - // ... existing code ... - - // Check if account is locked - if attempt, exists := failedLogins[req.Login]; exists { - if time.Now().Before(attempt.LockedUntil) { - return c.JSON(http.StatusTooManyRequests, map[string]string{ - "error": "account locked, try again later", - "retry_after": attempt.LockedUntil.Sub(time.Now()).String(), - }) - } - } - - user, err := h.db.GetUserForLogin(c.Request().Context(), req.Login) - if err != nil { - // Record failed attempt - if attempt, exists := failedLogins[req.Login]; exists { - attempt.Attempts++ - if attempt.Attempts >= 5 { - attempt.LockedUntil = time.Now().Add(30 * time.Minute) - } - } else { - failedLogins[req.Login] = &FailedLoginAttempt{ - Email: req.Login, - Attempts: 1, - } - } - return c.JSON(http.StatusUnauthorized, map[string]string{"error": "invalid credentials"}) - } - - // Reset on successful login - delete(failedLogins, req.Login) - - // ... rest of login logic -} -``` - ---- - -#### Issue: JWT Expiration Too Long -**File**: `internal/handlers/auth.go:843-850` -**Test**: Long-lived tokens - -**Current Code**: -```go -"exp": time.Now().Add(24 * time.Hour).Unix(), -``` - -**Problem**: 24 hour tokens with no refresh mechanism = poor security - -**Fix Required**: -```go -"exp": time.Now().Add(1 * time.Hour).Unix(), // Short-lived access token -``` - -And implement refresh tokens: -```go -type RefreshToken struct { - Token string `json:"token"` - UserID uuid.UUID `json:"user_id"` - ExpiresAt time.Time `json:"expires_at"` -} - -// On login, return both tokens -return c.JSON(http.StatusOK, map[string]interface{}{ - "access_token": accessToken, - "refresh_token": refreshToken, - "expires_in": 3600, // 1 hour -}) -``` - ---- - -### 8. File System Operation Failures - -#### Issue: No Validation of Folder Path -**File**: `internal/handlers/library.go:161-186` -**Test**: `TestLibraryFolders/Add_folder_with_invalid_path` - -**Current Code**: -```go -folder, err := h.libraryService.AddLibraryFolder( - c.Request().Context(), - libraryID, - req.FolderPath, -) -``` - -**Problem**: Doesn't verify path exists or is readable before saving - -**Fix Required**: -```go -// Validate folder path -if _, err := os.Stat(req.FolderPath); os.IsNotExist(err) { - return c.JSON(http.StatusBadRequest, map[string]string{"error": "folder path does not exist"}) -} - -// Check if readable -file, err := os.Open(req.FolderPath) -if err != nil { - return c.JSON(http.StatusBadRequest, map[string]string{"error": "folder is not accessible"}) -} -file.Close() - -folder, err := h.libraryService.AddLibraryFolder(...) -``` - ---- - -## Recommended Fix Priority - -### Critical (Fix Immediately) -1. Add rate limiting to auth endpoints -2. Fix password validation (add complexity requirements) -3. Add account lockout after failed logins -4. Reduce JWT expiration to 1 hour -5. Fix database race conditions with transactions -6. Add max pagination limit - -### High (Fix Soon) -1. Standardize error response formats -2. Add email validation (reject + addressing) -3. Fix whitespace handling in usernames -4. Add folder path validation -5. Implement background job for scanner -6. Fix role case sensitivity - -### Medium (Fix Later) -1. Add refresh token mechanism -2. Implement request ID tracing -3. Add password complexity requirements -4. Optimize N+1 queries in media items -5. Add email verification for registration - -### Low (Nice to Have) -1. Add unicode normalization for usernames -2. Implement account email verification -3. Add security headers (CSP, HSTS, etc.) -4. Implement API versioning -5. Add OpenAPI/Swagger documentation - ---- - -## Test Coverage Gaps - -The following scenarios are NOT currently tested but should be: - -1. Concurrent registration with same email/username -2. Scanner with inaccessible network paths -3. Large file uploads (if any) -4. Database connection failures -5. Memory exhaustion scenarios -6. Unicode handling in usernames/emails -7. Timezone handling in timestamps -8. CORS misconfigurations -9. Session fixation attacks -10. CSRF token validation - ---- - -## Conclusion - -While all 157 tests pass with mock handlers, **the actual code has significant security, validation, and error handling issues** that need immediate attention. The mock handlers correctly identify failure scenarios, but the real handlers may not handle them properly. - -**Estimated effort to fix all critical issues**: 20-30 hours of development work.