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:

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:

Email string `form:"email" json:"email" validate:"required,email,excludes=@"`

Or add custom validator in Register function:

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:

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:

Username string `form:"username" json:"username" validate:"required,min=3,max=50,trim"`

And add explicit check:

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:

Password string `form:"password" json:"password" validate:"required,min=6"`

Problem: Only checks length, not complexity. "aaaaaa" is valid.

Fix Required:

Password string `form:"password" json:"password" validate:"required,min=8,containsany=ABCDEFGHIJKLMNOPQRSTUVWXYZ,containsany=abcdefghijklmnopqrstuvwxyz,containsany=0123456789,containsany=!@#$%^&*"

Or add custom validation:

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:

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:

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:

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:

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:

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:

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:

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:

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:

ScanFrequencyMinutes int32 `json:"scan_frequency_minutes" validate:"required,min=15,max=1440"`

Problem: 15 minutes = 4 scans/hour, excessive database load

Fix Required:

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:

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:

CurrentPage int32 `json:"current_page" validate:"required,min=0"`
TotalPages  int32 `json:"total_pages" validate:"omitempty,min=0"`

And add runtime check:

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:

Rating int32 `json:"rating" validate:"required,min=1,max=10"`

Problem: Actually correct, but verify database constraint matches

Fix Required: Add database constraint:

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:

// 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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

"exp": time.Now().Add(24 * time.Hour).Unix(),

Problem: 24 hour tokens with no refresh mechanism = poor security

Fix Required:

"exp": time.Now().Add(1 * time.Hour).Unix(), // Short-lived access token

And implement refresh tokens:

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:

folder, err := h.libraryService.AddLibraryFolder(
    c.Request().Context(),
    libraryID,
    req.FolderPath,
)

Problem: Doesn't verify path exists or is readable before saving

Fix Required:

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