Admin portal auth

[victor-cuevas]

No description provided.

[sergiopalacio]

Unbounded memory growth. prune(ip) is only called from Allow() (line 36) and only for that one IP. Record() here never prunes, and Reset() only clears on successful login — which attackers won't hit. An IP that records a failure and never returns (distributed brute force, scanners, NAT churn) keeps its entry in attempts forever. No size cap either.

Suggest a Sweep() that walks the whole map, plus a background goroutine started from runServer alongside cleanupExpiredSessions:

func (rl *RateLimiter) Sweep() {
    rl.mu.Lock()
    defer rl.mu.Unlock()
    cutoff := rl.now().Add(-rl.window)
    for ip, attempts := range rl.attempts {
        i := 0
        for i < len(attempts) && attempts[i].Before(cutoff) { i++ }
        if i == len(attempts) {
            delete(rl.attempts, ip)
        } else if i > 0 {
            rl.attempts[ip] = attempts[i:]
        }
    }
}

[sergiopalacio]

Rate-limit key is the raw TCP peer address. Today clientIP returns r.RemoteAddr, which behind a reverse proxy / LB is the proxy's IP on every request — 5 failed logins from any attacker lock out every user until the window expires.

Two ways to fix, depending on deployment assumptions:

Option A — always direct-to-internet. Document the assumption and explicitly reject X-Forwarded-For (so an attacker can't spoof the header to escape the per-IP counter):

// Admin portal is deployed direct-to-internet; X-Forwarded-For from clients
// is attacker-controlled and must be ignored for rate-limiting.
func clientIP(r *http.Request) string {
    host, _, err := net.SplitHostPort(r.RemoteAddr)
    if err != nil {
        return r.RemoteAddr
    }
    return host
}

Option B — can sit behind a trusted proxy. Add a config knob (server.trusted_proxies: ["10.0.0.0/8", ...]) and only trust X-Forwarded-For when the direct peer is on that list. Walk the header right-to-left, first untrusted IP wins:

func clientIP(r *http.Request, trusted []netip.Prefix) string {
    host, _, err := net.SplitHostPort(r.RemoteAddr)
    if err != nil {
        host = r.RemoteAddr
    }
    peer, err := netip.ParseAddr(host)
    if err != nil || !inTrusted(peer, trusted) {
        return host // untrusted peer — ignore XFF
    }
    parts := strings.Split(r.Header.Get("X-Forwarded-For"), ",")
    for i := len(parts) - 1; i >= 0; i-- {
        ip, err := netip.ParseAddr(strings.TrimSpace(parts[i]))
        if err == nil && !inTrusted(ip, trusted) {
            return ip.String()
        }
    }
    return host
}

func inTrusted(ip netip.Addr, prefixes []netip.Prefix) bool {
    for _, p := range prefixes {
        if p.Contains(ip) {
            return true
        }
    }
    return false
}

Option A is simpler; Option B is required if the portal ever ends up behind an ingress.

[sergiopalacio]

Default is insecure. SecureCookies zero-value is false, so any deployment that forgets to set the flag ships auth cookies over plaintext. Prefer defaulting to true and making explicit opt-out the dev path or auto-detect based on Server.Addr being localhost.

[sergiopalacio]

Field name is misleading — this holds the SHA-256 hash, not the raw token. GetSessionByToken (line 127) scans the token column — which is written as hashToken(token) on line 114 — into sess.Token. Meanwhile LoginResult.SessionToken in auth/auth.go:63 holds the raw token. A future maintainer mixing up sess.Token and loginResult.SessionToken could trivially create an auth bypass. Suggest renaming to TokenHash. The test at user_test.go even works around the confusion with a comment: "Token is stored as a SHA-256 hash, so it won't match the raw value."

[sergiopalacio]

SameSite mode is inconsistent with the session cookie. Session cookie uses SameSiteLaxMode (line 176), CSRF cookie uses SameSiteStrictMode here. With Lax on the session, a top-level navigation carries the session but not the CSRF cookie. For unsafe methods both are required (fine), but a GET /api/me on such a navigation succeeds with no CSRF attestation at all. Pick one — both Strict is the safer default; both Lax is acceptable if you rely on the CSRF header check for all state changes. Same mismatch exists in clearCookies (:205, :214).

[sergiopalacio]

svc.CreateUser error is discarded in 8 places in this file (:35, :86, :115, :144, :172, :193, :214, :251). If the test DB fails to create the user, the test proceeds with confusing downstream failures instead of a clear t.Fatalf. admin/auth/auth_test.go already has a createTestUser helper that does this properly — use it here, or inline if err := svc.CreateUser(...); err != nil { t.Fatalf(...) }.

[sergiopalacio]

No account-level lockout. Rate limiting is only per-IP (see RateLimiter in auth/ratelimit.go). An attacker with N distinct IPs still gets N × 5 guesses/minute against a single username. For an admin portal with presumably few users, a per-username failure counter (with a longer lockout window) is a reasonable additional layer. Not blocking, but worth tracking.

[sergiopalacio]

bcrypt.GenerateFromPassword(DefaultCost) runs at package init — every binary invocation, including the short-lived create-user and reset-password CLI commands, pays ~50–100ms of bcrypt startup even when it never hits the user-not-found branch. Defer it:

var dummyHash = sync.OnceValue(func() []byte {
    h, _ := bcrypt.GenerateFromPassword([]byte("dummy-password-for-timing"), bcrypt.DefaultCost)
    return h
})

And use dummyHash() at the call site on line 135.

[sergiopalacio]

Password bytes are never zeroed. term.ReadPassword returns a []byte; converting it with string(password) copies the bytes into an immutable string that lives until GC, and the original slice is also never cleared. For a short-lived CLI this is a minor hygiene issue, but the project already lists memguard as a dep in the core module for exactly this purpose. At minimum, for i := range password { password[i] = 0 } after the bcrypt hash is computed would reduce the exposure window.

[sergiopalacio]

Hard navigation instead of SPA routing. window.location.href = '/login' drops SPA state (scroll position, any in-flight router transitions, the redirect query param that LoginView.vue relies on to bounce back after login). Use the router instead:

const { default: router } = await import('../router/index.js');
router.push({ name: 'login', query: { redirect: window.location.pathname } });

Preserves the post-login redirect UX that the current LoginView already supports.