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:

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

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

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:

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

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

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

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

And update createSession:

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

type Server struct {
    // ... existing fields ...
    done chan struct{}
}

Initialize it in NewServer:

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

Rewrite cleanupSessions:

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

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:

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

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:

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