bookhoard/cmd/server/tests/ANALYSIS.md

# 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.