path: root/CODEBASE_ANALYSIS.md

# Codebase Analysis & Improvement Plan

## Executive Summary

Comprehensive analysis of the task-dashboard codebase covering error handling, security, code quality, and concurrency. **38+ issues identified** across 4 categories with prioritized remediation roadmap.

**Overall Risk Level: MEDIUM-HIGH**
- Security: Good foundation but critical gaps (rate limiting, XSS, default credentials)
- Resilience: Partial failure handling masks issues; missing circuit breakers
- Code Quality: Significant duplication and complexity; needs refactoring
- Concurrency: One critical race condition; otherwise well-designed

---

## Prioritized Issues by Severity

### CRITICAL (Fix Immediately)

| # | Issue | Location | Category | Impact |
|---|-------|----------|----------|--------|
| C1 | Default password fallback | `main.go:62-67` | Security | Full system compromise |
| C2 | XSS in HTML generation | `handlers.go:795,920` | Security | Session hijacking |
| C3 | Data race in GetBoardsWithCards | `trello.go:182-197` | Concurrency | Data corruption |
| C4 | Missing rate limiting | All endpoints | Security | Brute force attacks |
| C5 | API keys in query params (Trello) | `trello.go:33-38` | Security | Credential exposure in logs |

### HIGH (Fix Within 1 Week)

| # | Issue | Location | Category | Impact |
|---|-------|----------|----------|--------|
| H1 | Missing security headers | `main.go` | Security | XSS, clickjacking |
| H2 | Insecure session cookies in debug | `main.go:49` | Security | Session hijacking |
| H3 | HandleTabTasks complexity (95 lines) | `handlers.go:959-1051` | Quality | Maintainability |
| H4 | Duplicate sorting logic | Multiple files | Quality | Bug risk from inconsistency |
| H5 | Errors swallowed in concurrent ops | `handlers.go:213-240` | Resilience | Hidden failures |
| H6 | JSON marshal errors ignored | `sqlite.go:174,218` | Resilience | Silent data loss |
| H7 | No timeout on Google Calendar init | `main.go:97` | Resilience | Startup hangs |
| H8 | Timing attack on CSRF comparison | `middleware.go:85` | Security | Token compromise |

### MEDIUM (Fix Within 1 Month)

| # | Issue | Location | Category | Impact |
|---|-------|----------|----------|--------|
| M1 | Magic numbers throughout | Multiple | Quality | Maintainability |
| M2 | aggregateShoppingLists (125 lines) | `handlers.go:1284-1408` | Quality | Hard to test |
| M3 | Repetitive error handling pattern | 50+ locations | Quality | Duplication |
| M4 | Mixed HTML in Go code | `handlers.go:876-888` | Quality | Maintainability |
| M5 | Cache fallback hides real problems | `handlers.go:350-355` | Resilience | Masked failures |
| M6 | No circuit breaker pattern | API clients | Resilience | Cascading failures |
| M7 | Session lifetime too long (24h) | `main.go:47` | Security | Extended exposure |
| M8 | Error messages leak DB structure | Various | Security | Information disclosure |
| M9 | Debug logging in production code | Multiple API files | Quality | Performance, security |
| M10 | Input validation missing | Multiple handlers | Security | DoS via large inputs |

### LOW (Backlog)

| # | Issue | Location | Category | Impact |
|---|-------|----------|----------|--------|
| L1 | Inconsistent naming conventions | Throughout | Quality | Readability |
| L2 | Verbose defer patterns | `sqlite.go` | Quality | Noise |
| L3 | Comments stating the obvious | Throughout | Quality | Noise |
| L4 | Unused stub methods | `plantoeat.go:189-196` | Quality | Confusion |
| L5 | Bug/UserShoppingItem in wrong package | `sqlite.go` | Quality | Inconsistency |

---

## Detailed Findings by Category

### 1. Security Vulnerabilities

#### Critical: Default Password Fallback (C1)
```go
// main.go:62-67
if defaultPass == "" {
    if !cfg.Debug {
        log.Fatal("CRITICAL: DEFAULT_PASS must be set...")
    }
    defaultPass = "changeme"  // DANGEROUS
}
```
**Fix:** Remove fallback entirely. Require password in ALL environments.

#### Critical: XSS in HTML Generation (C2)
```go
// handlers.go:795
_, _ = fmt.Fprintf(w, `<option value="%s">%s</option>`, list.ID, list.Name)
// list.Name is NOT escaped!
```
**Fix:** Use `template.HTMLEscapeString()` for all user-controlled values.

#### Critical: Missing Rate Limiting (C4)
No protection against brute force on `/login` or API endpoints.
**Fix:** Add rate limiting middleware (5 req/15min on auth endpoints).

#### High: Missing Security Headers (H1)
```go
// Add to main.go after line 115:
r.Use(func(next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("X-Content-Type-Options", "nosniff")
        w.Header().Set("X-Frame-Options", "DENY")
        w.Header().Set("Content-Security-Policy", "default-src 'self'...")
        next.ServeHTTP(w, r)
    })
})
```

### 2. Concurrency Issues

#### Critical: Data Race in GetBoardsWithCards (C3)
```go
// trello.go:182-197 - Multiple goroutines write to boards[i] without sync
boards[i].Cards = cards  // RACE CONDITION
boards[i].Lists = lists  // RACE CONDITION
```
**Fix:** Add mutex protection:
```go
var mu sync.Mutex
// In goroutine:
mu.Lock()
boards[i].Cards = cards
boards[i].Lists = lists
mu.Unlock()
```

### 3. Error Handling & Resilience

#### High: Ignored Errors (H6)
```go
// sqlite.go:174 - Marshal error silently dropped
labelsJSON, _ := json.Marshal(task.Labels)

// http.go:101 - Error response body read failure ignored
body, _ := io.ReadAll(resp.Body)
```
**Fix:** Handle all errors, at minimum log them.

#### Medium: Cache Fallback Masks Problems (M5)
```go
// handlers.go:350-355
if err != nil {
    // Falls back to cache with NO indication data is stale
    return h.store.GetTasks()
}
```
**Fix:** Add staleness indicator to response; log API failures prominently.

### 4. Code Quality

#### High: Complex Functions (H3, M2)

| Function | Lines | Responsibilities |
|----------|-------|------------------|
| `HandleTabTasks` | 95 | Fetch, convert, sort, filter, render |
| `aggregateShoppingLists` | 125 | 3 data sources, map building, sorting |
| `HandleTabPlanning` | 133 | Data fetch, business logic, time comparison, render |

**Fix:** Extract into smaller functions with single responsibilities.

#### High: Duplicate Sorting Logic (H4)
Three separate implementations of date/priority sorting:
- `sortTasksByUrgency` (handlers.go:276-291)
- `filterAndSortTrelloTasks` (handlers.go:294-320)
- Inline sort in HandleTabTasks (handlers.go:1008-1023)

**Fix:** Create centralized `models/sorting.go` with reusable comparators.

#### Medium: Magic Numbers (M1)
```go
sem := make(chan struct{}, 5)  // What's 5?
time.Date(..., 8, 0, 0, 0, ...)  // Breakfast at 8am?
Timeout: 15 * time.Second  // Why 15?
```
**Fix:** Extract to named constants in `internal/config/constants.go`.

---

## Refactoring Plan

### Phase 1: Critical Security (Days 1-2)

**Goal:** Eliminate critical security vulnerabilities.

1. **Remove default password** (C1)
   - File: `cmd/dashboard/main.go:62-67`
   - Change: Always require `DEFAULT_PASS`, remove "changeme" fallback

2. **Fix XSS vulnerabilities** (C2)
   - File: `internal/handlers/handlers.go:795,920`
   - Change: Wrap all user data with `template.HTMLEscapeString()`

3. **Add rate limiting** (C4)
   - File: `cmd/dashboard/main.go`
   - Add: Rate limiting middleware on `/login` (5 req/15min/IP)
   - Package: `github.com/ulule/limiter/v3`

4. **Add security headers** (H1)
   - File: `cmd/dashboard/main.go`
   - Add: Security headers middleware (CSP, X-Frame-Options, etc.)

### Phase 2: Critical Concurrency (Day 3)

**Goal:** Fix data race conditions.

1. **Fix GetBoardsWithCards race** (C3)
   - File: `internal/api/trello.go:165-236`
   - Change: Add mutex for board slice element writes

```go
// Add after line 166:
var mu sync.Mutex

// Wrap writes at lines 189 and 196:
mu.Lock()
boards[i].Cards = cards
boards[i].Lists = lists
mu.Unlock()
```

2. **Add race detector to CI**
   - Add `go test -race ./...` to build pipeline

### Phase 3: Error Handling (Days 4-5)

**Goal:** Improve resilience and observability.

1. **Handle ignored errors** (H6)
   - Files: `sqlite.go:174,218`, `http.go:101,132`
   - Change: Log errors instead of ignoring with `_`

2. **Add context timeout to Google Calendar init** (H7)
   - File: `cmd/dashboard/main.go:97`
   - Change: Use `context.WithTimeout(context.Background(), 30*time.Second)`

3. **Fix CSRF timing attack** (H8)
   - File: `internal/auth/middleware.go:85`
   - Change: Use `subtle.ConstantTimeCompare()` for token comparison

4. **Add staleness indicator to cache fallback** (M5)
   - File: `internal/handlers/handlers.go`
   - Change: Return `isStale` flag when serving cached data

### Phase 4: Code Quality - Constants (Day 6)

**Goal:** Eliminate magic numbers.

1. **Create constants file**
   - New file: `internal/config/constants.go`

```go
package config

import "time"

const (
    // Concurrency
    MaxConcurrentTrelloRequests = 5

    // Timeouts
    HTTPClientTimeout     = 15 * time.Second
    GoogleCalendarTimeout = 30 * time.Second
    GracefulShutdownTimeout = 10 * time.Second

    // Meal times
    BreakfastHour = 8
    LunchHour     = 12
    DinnerHour    = 19

    // Database
    SQLiteMaxOpenConns   = 5
    SQLiteMaxIdleConns   = 2
    SQLiteConnMaxLifetime = time.Hour

    // Session
    SessionLifetime = 24 * time.Hour
)
```

2. **Update all references** to use constants

### Phase 5: Code Quality - Extract Functions (Days 7-9)

**Goal:** Reduce function complexity.

1. **Refactor HandleTabTasks** (H3)
   - Extract: `buildUnifiedAtomList()`, `partitionAtomsByTime()`, `sortAtomsByUrgency()`

2. **Refactor aggregateShoppingLists** (M2)
   - Extract: `addTrelloShoppingItems()`, `addPlanToEatItems()`, `addUserShoppingItems()`, `convertStoreMapToSlice()`

3. **Consolidate sorting logic** (H4)
   - New file: `internal/models/sorting.go`
   - Functions: `SortTasksByUrgency()`, `SortAtomsByUrgency()`, `CompareByDueDate()`

### Phase 6: Code Quality - Deduplication (Days 10-11)

**Goal:** Reduce repetitive patterns.

1. **Create form parsing helper** (M3)
   - File: `internal/handlers/helpers.go`

```go
func parseFormOr400(w http.ResponseWriter, r *http.Request) bool {
    if err := r.ParseForm(); err != nil {
        JSONError(w, http.StatusBadRequest, "Failed to parse form", err)
        return false
    }
    return true
}
```

2. **Replace inline HTML with templates** (M4)
   - Create: `web/templates/partials/task-detail.html`
   - Create: `web/templates/partials/option-list.html`

3. **Implement structured logging** (M9)
   - Replace `log.Printf("DEBUG ...")` with `slog` package
   - Add log levels (Debug, Info, Warn, Error)

### Phase 7: Additional Security Hardening (Day 12)

**Goal:** Defense in depth.

1. **Session security improvements** (M7)
   - Reduce lifetime to 4 hours
   - Change SameSite to Strict
   - Ensure HttpOnly is set

2. **Input validation** (M10)
   - Add max length checks on all form inputs
   - Validate IDs match expected patterns

3. **Sanitize error messages** (M8)
   - Create internal error types
   - Return generic messages to users, log details server-side

---

## Verification Checklist

After each phase:
- [ ] `go build ./...` passes
- [ ] `go test ./...` passes
- [ ] `go test -race ./...` passes (Phase 2+)
- [ ] Manual smoke test of affected features
- [ ] Security headers verified (Phase 1) via browser dev tools

---

## Implementation Order Summary

```
Week 1:
├── Phase 1: Critical Security (2 days)
├── Phase 2: Critical Concurrency (1 day)
└── Phase 3: Error Handling (2 days)

Week 2:
├── Phase 4: Constants (1 day)
├── Phase 5: Extract Functions (3 days)
└── Phase 6: Deduplication (2 days)

Week 3:
└── Phase 7: Security Hardening (1 day)
```

---

## Metrics to Track

| Metric | Current | Target |
|--------|---------|--------|
| Critical issues | 5 | 0 |
| High issues | 8 | 0 |
| Test coverage | ~60% | 80% |
| Max function lines | 133 | 50 |
| `go vet` warnings | ? | 0 |
| `staticcheck` warnings | ? | 0 |

---

## Files Modified by Phase

| Phase | Files |
|-------|-------|
| 1 | main.go, handlers.go |
| 2 | trello.go |
| 3 | sqlite.go, http.go, main.go, middleware.go, handlers.go |
| 4 | NEW: config/constants.go, + 8 files updated |
| 5 | handlers.go, NEW: models/sorting.go |
| 6 | NEW: handlers/helpers.go, NEW: 2 templates |
| 7 | main.go, handlers.go |

---

## Long-term Recommendations (Post-Refactor)

1. **Service Layer**: Extract business logic from handlers into services
2. **Circuit Breaker**: Add resilience4j-style circuit breakers for API clients
3. **Request Coalescing**: Deduplicate concurrent identical requests
4. **Secrets Management**: Move from env vars to HashiCorp Vault or similar
5. **Automated Security Scanning**: Add gosec to CI pipeline
6. **Load Testing**: Verify concurrent request handling under load