chore(tests): remove temporary test analysis file

- Remove cmd/server/tests/ANYSIS.md (temporary investigation file) - No longer needed after test fixes completed
2026-02-06 22:00:12 -05:00
parent 4b68ef8abd
commit 73678a1df0
1 changed files with 0 additions and 683 deletions
@@ -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
 <div class="error">message</div>
 ```
 **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, `<div class="text-red-500">`+err.Error()+`</div>`)
 }
 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(`<div class="text-red-500">%s</div>`, 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.