deploy-felhom-compose/TASK.md

# TASK.md — Code Review Bugfixes (v0.6.1)

## Overview

Fix bugs and logic issues identified during the v0.6.0 code review. All changes are in the `controller/` subtree.
No new features — only correctness, safety, and quality fixes.

After all fixes: bump version to **v0.6.1**, commit, build, push, deploy, verify.

---

## Fix 1: `http.NotFound(w, nil)` — pass request, not nil

**Files:** `internal/web/handlers.go`

**Problem:** Two handlers discard the `*http.Request` parameter as `_`, then call `http.NotFound(w, nil)`. While Go's current stdlib doesn't dereference the request in `NotFound`, this is incorrect and will break if middleware wraps it.

**Changes:**

In `deployHandler`, change signature and call:
```go
// BEFORE:
func (s *Server) deployHandler(w http.ResponseWriter, _ *http.Request, name string) {
    ...
    if err != nil {
        http.NotFound(w, nil)

// AFTER:
func (s *Server) deployHandler(w http.ResponseWriter, r *http.Request, name string) {
    ...
    if err != nil {
        http.NotFound(w, r)
```

In `appDetailHandler`, same fix:
```go
// BEFORE:
func (s *Server) appDetailHandler(w http.ResponseWriter, _ *http.Request, slug string) {
    ...
    if found == nil {
        http.NotFound(w, nil)

// AFTER:
func (s *Server) appDetailHandler(w http.ResponseWriter, r *http.Request, slug string) {
    ...
    if found == nil {
        http.NotFound(w, r)
```

**Verify:** Grep for `NotFound(w, nil)` — should return 0 results after fix.

---

## Fix 2: Dashboard running/stopped counts don't match displayed stacks

**File:** `internal/web/handlers.go`, `dashboardHandler`

**Problem:** The `running`/`stopped` stat counters iterate over ALL stacks (including non-deployed ones), but the dashboard only displays deployed + protected stacks. The numbers don't match what the user sees.

**Fix:** Compute the counts from the same filtered set (`deployedStacks`), not from `stackList`. Move the filter loop first, then count from the filtered result.

```go
func (s *Server) dashboardHandler(w http.ResponseWriter, _ *http.Request) {
    stackList := s.stackMgr.GetStacks()

    // Filter to deployed + protected stacks first
    var deployedStacks []stacks.Stack
    for _, st := range stackList {
        if st.Deployed || st.Protected {
            deployedStacks = append(deployedStacks, st)
        }
    }

    // Count from the DISPLAYED set only
    running, stopped := 0, 0
    for _, st := range deployedStacks {
        switch st.State {
        case stacks.StateRunning, stacks.StateStarting, stacks.StateUnhealthy, stacks.StateRestarting:
            running++
        case stacks.StateStopped, stacks.StateExited:
            stopped++
        }
    }

    // ... rest unchanged, but use deployedStacks for display ...
    data["Stacks"] = deployedStacks
    data["RunningCount"] = running
    data["StoppedCount"] = stopped
    data["TotalCount"] = len(stackList)  // keep this as total catalog size
```

**Verify:** Deploy, open dashboard. Count of green + red badges on cards should match the stat numbers.

---

## Fix 3: `Secure: true` cookie blocks HTTP login

**File:** `internal/web/auth.go`, `handleLogin`

**Problem:** The session cookie has `Secure: true` hardcoded. When accessing via plain HTTP (e.g., `http://192.168.0.162:8080` during local setup), the browser silently refuses to send the cookie back, making login impossible with no visible error.

**Fix:** Set `Secure` dynamically based on the incoming request:

```go
// BEFORE:
http.SetCookie(w, &http.Cookie{
    Name:     sessionCookieName,
    Value:    token,
    Path:     "/",
    MaxAge:   int(sessionMaxAge.Seconds()),
    HttpOnly: true,
    SameSite: http.SameSiteStrictMode,
    Secure:   true,
})

// AFTER:
isSecure := r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"
http.SetCookie(w, &http.Cookie{
    Name:     sessionCookieName,
    Value:    token,
    Path:     "/",
    MaxAge:   int(sessionMaxAge.Seconds()),
    HttpOnly: true,
    SameSite: http.SameSiteLaxMode,  // Lax needed: Strict can break redirects through CF tunnel
    Secure:   isSecure,
})
```

Note: Also change `SameSiteStrictMode` → `SameSiteLaxMode`. Strict mode can cause issues when users arrive via Cloudflare Tunnel redirects (the cookie won't be sent on the first navigation from an external link).

**Verify:** Access `http://192.168.0.162:8080` in browser, log in — should work. Also verify HTTPS login still works via `https://vezerlo.demo-felhom.eu`.

---

## Fix 4: Remove misleading `subtle.ConstantTimeCompare` in session check

**File:** `internal/web/auth.go`, `isValidSession`

**Problem:** The map lookup `s.sessions[token]` already reveals the token via timing. The subsequent `ConstantTimeCompare` compares the token to itself (it was just fetched by that key), so it always returns 1 and adds no security. It's misleading to keep it.

**Fix:** Simplify:

```go
// BEFORE:
func (s *Server) isValidSession(token string) bool {
    s.sessionsMu.RLock()
    defer s.sessionsMu.RUnlock()
    sess, ok := s.sessions[token]
    if !ok || time.Now().After(sess.expiresAt) {
        return false
    }
    return subtle.ConstantTimeCompare([]byte(sess.token), []byte(token)) == 1
}

// AFTER:
func (s *Server) isValidSession(token string) bool {
    s.sessionsMu.RLock()
    defer s.sessionsMu.RUnlock()
    sess, ok := s.sessions[token]
    return ok && time.Now().Before(sess.expiresAt)
}
```

Also: the `token` field in the `session` struct is now unused (it duplicates the map key). Remove it:

```go
// BEFORE:
type session struct {
    token     string
    expiresAt time.Time
}

// AFTER:
type session struct {
    expiresAt time.Time
}
```

And update `createSession`:
```go
// BEFORE:
s.sessions[token] = &session{token: token, expiresAt: time.Now().Add(sessionMaxAge)}

// AFTER:
s.sessions[token] = &session{expiresAt: time.Now().Add(sessionMaxAge)}
```

After these changes, remove the `"crypto/subtle"` import if no longer used.

**Verify:** Log in, navigate around — session should work. Log out — should redirect to login.

---

## Fix 5: `cleanupSessions` goroutine leak

**File:** `internal/web/auth.go`

**Problem:** `time.Tick()` creates a ticker that can never be GC'd. The goroutine runs forever, even during shutdown.

**Fix:** This one is lower priority since the controller runs as a long-lived process, but the fix is simple. Since we don't currently pass a context to `NewServer`, use a `done` channel on the server:

Add a `done` channel to the Server struct:
```go
type Server struct {
    // ... existing fields ...
    done chan struct{}
}
```

Initialize it in `NewServer`:
```go
func NewServer(...) *Server {
    s := &Server{
        // ... existing ...
        done: make(chan struct{}),
    }
    s.loadTemplates()
    go s.cleanupSessions()
    return s
}
```

Rewrite `cleanupSessions`:
```go
func (s *Server) cleanupSessions() {
    ticker := time.NewTicker(15 * time.Minute)
    defer ticker.Stop()
    for {
        select {
        case <-s.done:
            return
        case <-ticker.C:
            s.sessionsMu.Lock()
            now := time.Now()
            for t, sess := range s.sessions {
                if now.After(sess.expiresAt) {
                    delete(s.sessions, t)
                }
            }
            s.sessionsMu.Unlock()
        }
    }
}
```

Add a `Close` method (called from main during shutdown, optional for now):
```go
func (s *Server) Close() {
    close(s.done)
}
```

**Verify:** Build succeeds, controller starts without errors.

---

## Fix 6: Add `http.MaxBytesReader` to API POST endpoints

**File:** `internal/api/router.go`

**Problem:** `json.NewDecoder(req.Body).Decode(&body)` has no size limit. A malicious or accidental large POST could exhaust memory.

**Fix:** Add a helper and use it in all handlers that decode JSON bodies (`deployStack`, `updateOptionalConfig`, `deleteStack`):

Add helper at the bottom of `router.go`:
```go
// limitBody wraps the request body with a size limit (default 1MB).
func limitBody(w http.ResponseWriter, req *http.Request) {
    req.Body = http.MaxBytesReader(w, req.Body, 1<<20) // 1MB
}
```

Then at the start of each handler that reads the body:
```go
func (r *Router) deployStack(w http.ResponseWriter, req *http.Request, name string) {
    limitBody(w, req)
    // ... existing json decode ...
}

func (r *Router) updateOptionalConfig(w http.ResponseWriter, req *http.Request, name string) {
    limitBody(w, req)
    // ...
}

func (r *Router) deleteStack(w http.ResponseWriter, req *http.Request, name string) {
    limitBody(w, req)
    // ...
}
```

**Verify:** Build succeeds. Normal deploy/config/delete still works (payloads are tiny).

---

## Fix 7: Cache `time.LoadLocation` in template funcmap

**File:** `internal/web/funcmap.go`

**Problem:** At least 5 template functions call `time.LoadLocation("Europe/Budapest")` on every render. While Go caches internally, it still acquires a mutex each time.

**Fix:** Load once at the top of `templateFuncMap` and capture in the closures:

```go
func (s *Server) templateFuncMap() template.FuncMap {
    loc, err := time.LoadLocation("Europe/Budapest")
    if err != nil {
        loc = time.UTC
    }

    return template.FuncMap{
        // ... in every function that currently calls time.LoadLocation,
        // replace with the captured `loc` variable.
        // Remove the per-function `loc, _ := time.LoadLocation(...)` lines.
        // Example:
        "timeAgo": func(t time.Time) string {
            if t.IsZero() { return "–" }
            now := time.Now().In(loc)
            d := now.Sub(t.In(loc))
            // ... rest unchanged ...
        },
        // Apply same pattern to: fmtTime, fmtTimeShort, nextRunLabel, nextPruneLabel
    }
}
```

There are 5 functions that need this change: `timeAgo`, `fmtTime`, `fmtTimeShort`, `nextRunLabel`, `nextPruneLabel`.

**Verify:** Build succeeds. Dashboard/backup page timestamps still display correctly in Budapest time.

---

## Post-fix checklist

1. `grep -rn 'NotFound(w, nil)' internal/` → 0 results
2. `grep -rn 'subtle.ConstantTimeCompare' internal/` → 0 results (unless used elsewhere)
3. `grep -rn 'time.Tick(' internal/` → 0 results
4. `grep -rn 'Secure:.*true' internal/web/auth.go` → 0 results (now dynamic)
5. Build: `go build ./cmd/controller/` succeeds with no errors
6. `go vet ./...` passes
7. Version bump in build to v0.6.1
8. Commit, push, build, deploy, verify on demo-felhom.eu