diff options
Diffstat (limited to 'CODEBASE_ANALYSIS.md')
| -rw-r--r-- | CODEBASE_ANALYSIS.md | 403 |
1 files changed, 403 insertions, 0 deletions
diff --git a/CODEBASE_ANALYSIS.md b/CODEBASE_ANALYSIS.md new file mode 100644 index 0000000..6437950 --- /dev/null +++ b/CODEBASE_ANALYSIS.md @@ -0,0 +1,403 @@ +# 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 |