summaryrefslogtreecommitdiff
path: root/CODEBASE_ANALYSIS.md
diff options
context:
space:
mode:
authorPeter Stone <thepeterstone@gmail.com>2026-01-26 16:49:44 -1000
committerPeter Stone <thepeterstone@gmail.com>2026-01-26 16:49:44 -1000
commit42a4e32daca13b518e64e5821080ff3d6adf0e39 (patch)
tree639c790e25b961ecf51ab6ea75206bc3432f1548 /CODEBASE_ANALYSIS.md
parent8de1b5cb8915ed9a6e32566431d05fafafeb338d (diff)
Use configured timezone throughout codebase
- Add config/timezone.go with timezone utilities: - SetDisplayTimezone(), GetDisplayTimezone() - Now(), Today() - current time/date in display TZ - ParseDateInDisplayTZ(), ToDisplayTZ() - parsing helpers - Initialize timezone at startup in main.go - Update all datetime logic to use configured timezone: - handlers/handlers.go - all time.Now() calls - handlers/timeline.go - date parsing - handlers/timeline_logic.go - now calculation - models/atom.go - ComputeUIFields() - models/timeline.go - ComputeDaySection() - api/plantoeat.go - meal date parsing - api/todoist.go - due date parsing - api/trello.go - due date parsing This ensures all dates/times display correctly regardless of server timezone setting. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Diffstat (limited to 'CODEBASE_ANALYSIS.md')
-rw-r--r--CODEBASE_ANALYSIS.md403
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