Address production-readiness review: clearer errors, safer votes, and ops hardening.

Distinguish auth/lookup failures, make votes idempotent on visible questions, bound shutdown, page admin users, LRU throttle, trusted-proxy CIDRs, avatar cleanup, versioned migrations, and session cleanup logging.

internal/web/admin.go

@@ -3,6 +3,8 @@ package web
 import (
 	"errors"
 	"net/http"
+	"net/url"
+	"strings"
 
 	"github.com/go-chi/chi/v5"
 
@@ -11,8 +13,11 @@ import (
 
 type adminUsersPage struct {
 	page
-	Users []store.User
-	Error string
+	Users      []store.User
+	Error      string
+	Search     string
+	NextCursor string
+	HasMore    bool
 }
 
 func (s *Server) requireAdmin(w http.ResponseWriter, r *http.Request) *store.User {
@@ -28,14 +33,35 @@ func (s *Server) handleAdminUsers(w http.ResponseWriter, r *http.Request) {
 	if s.requireAdmin(w, r) == nil {
 		return
 	}
-	users, err := s.store.ListUsers(r.Context())
+	search := strings.TrimSpace(r.URL.Query().Get("q"))
+	cursorCreated := r.URL.Query().Get("cursor_created")
+	cursorID := r.URL.Query().Get("cursor_id")
+	users, nextCreated, nextID, err := s.store.ListUsers(r.Context(), store.ListUsersQuery{
+		Search:        search,
+		CursorCreated: cursorCreated,
+		CursorID:      cursorID,
+		Limit:         store.AdminUsersLimit,
+	})
 	if err != nil {
 		http.Error(w, "could not load users", http.StatusInternalServerError)
 		return
 	}
+	next := ""
+	if nextCreated != "" {
+		v := url.Values{}
+		if search != "" {
+			v.Set("q", search)
+		}
+		v.Set("cursor_created", nextCreated)
+		v.Set("cursor_id", nextID)
+		next = "/admin/users?" + v.Encode()
+	}
 	s.exec(w, "admin-users", adminUsersPage{
-		page:  s.basePage(r, "Users"),
-		Users: users,
+		page:       s.basePage(r, "Users"),
+		Users:      users,
+		Search:     search,
+		NextCursor: next,
+		HasMore:    next != "",
 	})
 }
 
@@ -50,7 +76,7 @@ func (s *Server) handleAdminSetRole(w http.ResponseWriter, r *http.Request) {
 	role := store.Role(r.PostFormValue("role"))
 	err := s.store.SetUserRole(r.Context(), id, role)
 	if errors.Is(err, store.ErrLastAdmin) {
-		users, listErr := s.store.ListUsers(r.Context())
+		users, _, _, listErr := s.store.ListUsers(r.Context(), store.ListUsersQuery{Limit: store.AdminUsersLimit})
 		if listErr != nil {
 			http.Error(w, "could not demote last admin", http.StatusBadRequest)
 			return

internal/web/auth.go

@@ -2,6 +2,9 @@ package web
 
 import (
 	"crypto/subtle"
+	"database/sql"
+	"errors"
+	"log"
 	"net/http"
 	"net/url"
 	"regexp"
@@ -79,8 +82,16 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) {
 
 	u, err := s.store.UserByUsername(r.Context(), username)
 	hash := loginDummyHash
-	if err == nil {
+	switch {
+	case err == nil:
 		hash = []byte(u.PasswordHash)
+	case errors.Is(err, sql.ErrNoRows):
+		// unknown user — still bcrypt against dummy hash
+	default:
+		log.Printf("login lookup: %v", err)
+		_ = bcrypt.CompareHashAndPassword(loginDummyHash, []byte(password))
+		http.Error(w, "service temporarily unavailable", http.StatusServiceUnavailable)
+		return
 	}
 	if err != nil || bcrypt.CompareHashAndPassword(hash, []byte(password)) != nil {
 		s.loginFail.record(loginFailKey(ip, userKey))
@@ -138,7 +149,7 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) {
 	}
 	role := store.RoleUser
 	if setupSecretMatches(s.cfg.AdminSetupSecret, setupSecret) {
-		role = store.RoleAdmin // store downgrades if an admin already exists
+		role = store.RoleAdmin
 	}
 	u := &store.User{
 		Username:     username,
@@ -146,12 +157,19 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) {
 		Role:         role,
 	}
 	if err := s.store.CreateUser(r.Context(), u); err != nil {
-		p.Error = "That username is taken."
-		s.exec(w, "register", p)
+		if errors.Is(err, store.ErrDuplicateUsername) {
+			p.Error = "That username is taken."
+			s.exec(w, "register", p)
+			return
+		}
+		log.Printf("register create: %v", err)
+		http.Error(w, "could not create account", http.StatusInternalServerError)
 		return
 	}
 	if err := s.sessions.RenewToken(r.Context()); err != nil {
-		http.Error(w, "could not start session", http.StatusInternalServerError)
+		log.Printf("register session: %v", err)
+		s.sessions.Put(r.Context(), "flash", "Account created — please sign in.")
+		http.Redirect(w, r, "/login", http.StatusSeeOther)
 		return
 	}
 	s.sessions.Put(r.Context(), "user_id", u.ID)

internal/web/profile.go

@@ -11,7 +11,6 @@ import (
 	"path"
 	"strings"
 
-	"github.com/google/uuid"
 	"golang.org/x/image/draw"
 	_ "golang.org/x/image/webp"
 
@@ -63,6 +62,7 @@ func (s *Server) handleProfile(w http.ResponseWriter, r *http.Request) {
 	}
 
 	avatarURL := ""
+	avatarKey := ""
 	file, hdr, err := r.FormFile("avatar")
 	if err == nil {
 		defer file.Close()
@@ -79,9 +79,10 @@ func (s *Server) handleProfile(w http.ResponseWriter, r *http.Request) {
 			s.renderProfile(w, r, u, "Avatar must be a JPEG, PNG, or WebP image.", state)
 			return
 		}
-		key := path.Join("avatars", u.ID, uuid.NewString()+ext)
+		prevURL := u.AvatarURL
+		avatarKey = path.Join("avatars", u.ID, "avatar"+ext)
 		url, upErr := s.cfg.Blob.Upload(r.Context(), blob.FileUpload{
-			Key:         key,
+			Key:         avatarKey,
 			Body:        bytes.NewReader(body),
 			ContentType: contentType,
 			Size:        int64(len(body)),
@@ -91,15 +92,25 @@ func (s *Server) handleProfile(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 		avatarURL = url
+		u.State = state
+		u.AvatarURL = avatarURL
+		if err := s.store.SaveUserProfile(r.Context(), u); err != nil {
+			_ = s.cfg.Blob.Delete(r.Context(), avatarKey)
+			http.Error(w, "could not save profile", http.StatusInternalServerError)
+			return
+		}
+		if oldKey := avatarObjectKey(prevURL, u.ID); oldKey != "" && oldKey != avatarKey {
+			_ = s.cfg.Blob.Delete(r.Context(), oldKey)
+		}
+		s.sessions.Put(r.Context(), "flash", "Profile saved.")
+		http.Redirect(w, r, "/profile", http.StatusSeeOther)
+		return
 	} else if err != http.ErrMissingFile {
 		s.renderProfile(w, r, u, "Could not read avatar file.", state)
 		return
 	}
 
 	u.State = state
-	if avatarURL != "" {
-		u.AvatarURL = avatarURL
-	}
 	if err := s.store.SaveUserProfile(r.Context(), u); err != nil {
 		http.Error(w, "could not save profile", http.StatusInternalServerError)
 		return
@@ -108,6 +119,19 @@ func (s *Server) handleProfile(w http.ResponseWriter, r *http.Request) {
 	http.Redirect(w, r, "/profile", http.StatusSeeOther)
 }
 
+func avatarObjectKey(publicURL, userID string) string {
+	marker := "/avatars/" + userID + "/"
+	i := strings.Index(publicURL, marker)
+	if i < 0 {
+		return ""
+	}
+	rest := publicURL[i+1:] // avatars/...
+	if q := strings.IndexAny(rest, "?#"); q >= 0 {
+		rest = rest[:q]
+	}
+	return rest
+}
+
 // prepareAvatar reads at most maxBytes, sniffs/decodes the image, resizes to a
 // small avatar, and re-encodes so only bounded valid image bytes are stored.
 func prepareAvatar(r io.Reader, maxBytes int64) (body []byte, ext, contentType string, err error) {

internal/web/server.go

@@ -3,11 +3,14 @@ package web
 import (
 	"context"
 	"crypto/rand"
+	"database/sql"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"html/template"
 	"io/fs"
 	"log"
+	"net"
 	"net/http"
 	"net/url"
 	"strings"
@@ -28,9 +31,9 @@ type Config struct {
 	// posts the matching setup_secret. It is ignored once any admin exists.
 	AdminSetupSecret string
 	SecureCookie     bool
-	// TrustProxy enables X-Forwarded-For / RealIP only behind a known proxy.
-	TrustProxy bool
-	Blob       blob.Uploader
+	// TrustedProxies are CIDRs allowed to set X-Forwarded-For (direct peer).
+	TrustedProxies []*net.IPNet
+	Blob           blob.Uploader
 }
 
 type Server struct {
@@ -145,7 +148,7 @@ func New(st store.Store, sessionStore scs.Store, templateFS fs.FS, staticFS fs.F
 func (s *Server) Handler() http.Handler {
 	r := chi.NewRouter()
 	r.Use(middleware.RequestID)
-	if s.cfg.TrustProxy {
+	if len(s.cfg.TrustedProxies) > 0 {
 		r.Use(middleware.RealIP)
 	}
 	r.Use(middleware.Logger)
@@ -356,7 +359,17 @@ func (s *Server) handleQuestion(w http.ResponseWriter, r *http.Request) {
 	}
 	var ans *store.Answer
 	if q.Answered {
-		ans, _ = s.store.GetAnswer(r.Context(), q.ID)
+		ans, err = s.store.GetAnswer(r.Context(), q.ID)
+		if err != nil {
+			if errors.Is(err, sql.ErrNoRows) {
+				log.Printf("question %s marked answered but answer missing", q.ID)
+				http.Error(w, "answer unavailable", http.StatusInternalServerError)
+				return
+			}
+			log.Printf("get answer %s: %v", q.ID, err)
+			http.Error(w, "could not load answer", http.StatusInternalServerError)
+			return
+		}
 	}
 	s.exec(w, "question", questionPage{
 		page:     s.basePage(r, q.Title),
@@ -385,11 +398,17 @@ func (s *Server) handleVote(w http.ResponseWriter, r *http.Request) {
 		value = 1
 	case "-1":
 		value = -1
+	case "0":
+		value = 0
 	default:
 		http.Error(w, "invalid vote", http.StatusBadRequest)
 		return
 	}
 	if err := s.store.Vote(r.Context(), u.ID, id, value); err != nil {
+		if errors.Is(err, store.ErrHiddenOrMissing) {
+			http.Error(w, "not found", http.StatusNotFound)
+			return
+		}
 		http.Error(w, "could not vote", http.StatusInternalServerError)
 		return
 	}

internal/web/server_test.go

@@ -332,6 +332,8 @@ func (f *fakeBlob) Upload(_ context.Context, obj blob.FileUpload) (string, error
 	return "https://cdn.example.com/" + obj.Key, nil
 }
 
+func (f *fakeBlob) Delete(_ context.Context, _ string) error { return nil }
+
 func TestProfilePageAndState(t *testing.T) {
 	srv, mem := newTestServer(t, Config{})
 	h := srv.Handler()

internal/web/throttle.go

@@ -1,22 +1,33 @@
 package web
 
 import (
+	"container/list"
+	"log"
 	"net"
 	"net/http"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 )
 
 const defaultThrottleMaxKeys = 10_000
 
-// throttle is a sliding-window rate limiter with expired-key eviction and a cap.
+// throttle is a sliding-window rate limiter with LRU eviction at capacity.
 type throttle struct {
 	mu      sync.Mutex
-	hits    map[string][]time.Time
+	hits    map[string]*throttleEntry
+	lru     *list.List // front = most recently used
 	limit   int
 	window  time.Duration
 	maxKeys int
+	rejects atomic.Uint64
+	lastLog time.Time
+}
+
+type throttleEntry struct {
+	times []time.Time
+	el    *list.Element
 }
 
 func newThrottle(limit int, window time.Duration, maxKeys int) *throttle {
@@ -24,7 +35,8 @@ func newThrottle(limit int, window time.Duration, maxKeys int) *throttle {
 		maxKeys = defaultThrottleMaxKeys
 	}
 	return &throttle{
-		hits:    map[string][]time.Time{},
+		hits:    map[string]*throttleEntry{},
+		lru:     list.New(),
 		limit:   limit,
 		window:  window,
 		maxKeys: maxKeys,
@@ -38,45 +50,68 @@ func (t *throttle) allow(key string) bool {
 	t.mu.Lock()
 	defer t.mu.Unlock()
 	now := time.Now()
-	t.evictExpiredLocked(now)
+	cutoff := now.Add(-t.window)
 
-	xs := pruneTimes(t.hits[key], now.Add(-t.window))
-	if len(xs) >= t.limit {
-		if len(xs) == 0 {
-			delete(t.hits, key)
-		} else {
-			t.hits[key] = xs
+	ent, ok := t.hits[key]
+	if ok {
+		ent.times = pruneTimes(ent.times, cutoff)
+		if len(ent.times) == 0 {
+			t.removeLocked(key, ent)
+			ok = false
 		}
-		return false
 	}
-	if _, exists := t.hits[key]; !exists && len(t.hits) >= t.maxKeys {
-		t.evictExpiredLocked(now)
-		if len(t.hits) >= t.maxKeys {
+	if ok {
+		if len(ent.times) >= t.limit {
+			t.touchLocked(key, ent)
 			return false
 		}
+		ent.times = append(ent.times, now)
+		t.touchLocked(key, ent)
+		return true
 	}
-	t.hits[key] = append(xs, now)
+
+	// New key: make room via LRU if needed.
+	for len(t.hits) >= t.maxKeys {
+		oldest := t.lru.Back()
+		if oldest == nil {
+			break
+		}
+		oldKey := oldest.Value.(string)
+		t.removeLocked(oldKey, t.hits[oldKey])
+		n := t.rejects.Add(1)
+		if time.Since(t.lastLog) > time.Minute {
+			log.Printf("throttle: LRU evicted key at capacity=%d rejects=%d", t.maxKeys, n)
+			t.lastLog = now
+		}
+	}
+	ent = &throttleEntry{times: []time.Time{now}}
+	ent.el = t.lru.PushFront(key)
+	t.hits[key] = ent
 	return true
 }
 
+func (t *throttle) touchLocked(key string, ent *throttleEntry) {
+	if ent.el != nil {
+		t.lru.MoveToFront(ent.el)
+	}
+}
+
+func (t *throttle) removeLocked(key string, ent *throttleEntry) {
+	if ent == nil {
+		return
+	}
+	if ent.el != nil {
+		t.lru.Remove(ent.el)
+	}
+	delete(t.hits, key)
+}
+
 func (t *throttle) lenKeys() int {
 	t.mu.Lock()
 	defer t.mu.Unlock()
 	return len(t.hits)
 }
 
-func (t *throttle) evictExpiredLocked(now time.Time) {
-	cutoff := now.Add(-t.window)
-	for k, xs := range t.hits {
-		xs = pruneTimes(xs, cutoff)
-		if len(xs) == 0 {
-			delete(t.hits, k)
-		} else {
-			t.hits[k] = xs
-		}
-	}
-}
-
 func pruneTimes(xs []time.Time, cutoff time.Time) []time.Time {
 	n := 0
 	for _, ts := range xs {
@@ -137,7 +172,16 @@ func (f *failureTracker) record(key string) {
 	f.evictExpiredLocked(now)
 	st := f.fails[key]
 	if st.count == 0 && len(f.fails) >= f.maxKeys {
-		return
+		// Drop an arbitrary expired-or-oldest entry.
+		for k, v := range f.fails {
+			if now.Sub(v.last) > f.window/2 {
+				delete(f.fails, k)
+				break
+			}
+		}
+		if len(f.fails) >= f.maxKeys {
+			return
+		}
 	}
 	st.count++
 	st.last = now
@@ -184,18 +228,58 @@ func progressiveDelay(failCount int) time.Duration {
 }
 
 func (s *Server) clientIP(r *http.Request) string {
-	if s.cfg.TrustProxy {
-		if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
-			return strings.TrimSpace(strings.Split(xff, ",")[0])
-		}
-	}
 	host, _, err := net.SplitHostPort(r.RemoteAddr)
 	if err != nil {
-		return r.RemoteAddr
+		host = r.RemoteAddr
+	}
+	peer := net.ParseIP(host)
+	if peer == nil || !ipInNets(peer, s.cfg.TrustedProxies) {
+		return host
+	}
+	xff := r.Header.Get("X-Forwarded-For")
+	if xff == "" {
+		return host
+	}
+	parts := strings.Split(xff, ",")
+	// Walk right-to-left; skip trusted hops; first untrusted is the client.
+	for i := len(parts) - 1; i >= 0; i-- {
+		p := net.ParseIP(strings.TrimSpace(parts[i]))
+		if p == nil {
+			continue
+		}
+		if !ipInNets(p, s.cfg.TrustedProxies) {
+			return p.String()
+		}
 	}
 	return host
 }
 
+func ipInNets(ip net.IP, nets []*net.IPNet) bool {
+	for _, n := range nets {
+		if n.Contains(ip) {
+			return true
+		}
+	}
+	return false
+}
+
+func parseCIDRs(raw string) []*net.IPNet {
+	var out []*net.IPNet
+	for _, part := range strings.Split(raw, ",") {
+		part = strings.TrimSpace(part)
+		if part == "" {
+			continue
+		}
+		_, n, err := net.ParseCIDR(part)
+		if err != nil {
+			log.Printf("trusted proxy CIDR ignored %q: %v", part, err)
+			continue
+		}
+		out = append(out, n)
+	}
+	return out
+}
+
 func authTooMany(w http.ResponseWriter) {
 	http.Error(w, "too many attempts; try again later", http.StatusTooManyRequests)
 }

internal/web/throttle_test.go

@@ -1,6 +1,7 @@
 package web
 
 import (
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"sync"
@@ -33,8 +34,8 @@ func TestThrottleMaxKeys(t *testing.T) {
 	if !th.allow("one") || !th.allow("two") {
 		t.Fatal("first keys should pass")
 	}
-	if th.allow("three") {
-		t.Fatal("over maxKeys should reject new key")
+	if !th.allow("three") {
+		t.Fatal("over maxKeys should LRU-evict and accept new key")
 	}
 	if th.lenKeys() != 2 {
 		t.Fatalf("keys=%d want 2", th.lenKeys())
@@ -90,7 +91,11 @@ func TestFailureTrackerEvictsExpired(t *testing.T) {
 }
 
 func TestClientIPTrustProxy(t *testing.T) {
-	srv := &Server{cfg: Config{TrustProxy: true}}
+	_, proxyNet, err := net.ParseCIDR("10.0.0.0/8")
+	if err != nil {
+		t.Fatal(err)
+	}
+	srv := &Server{cfg: Config{TrustedProxies: []*net.IPNet{proxyNet}}}
 	req := httptest.NewRequest(http.MethodGet, "/", nil)
 	req.RemoteAddr = "10.0.0.1:1234"
 	req.Header.Set("X-Forwarded-For", "203.0.113.9, 10.0.0.1")
@@ -98,9 +103,9 @@ func TestClientIPTrustProxy(t *testing.T) {
 		t.Fatalf("trusted xff got %q", got)
 	}
 
-	srv.cfg.TrustProxy = false
-	if got := srv.clientIP(req); got != "10.0.0.1" {
-		t.Fatalf("untrusted should use RemoteAddr host, got %q", got)
+	req.RemoteAddr = "203.0.113.50:9"
+	if got := srv.clientIP(req); got != "203.0.113.50" {
+		t.Fatalf("untrusted peer should ignore xff, got %q", got)
 	}
 }