From 59513ab75e798faccf0fa6536ee83e034a8b1a25 Mon Sep 17 00:00:00 2001 From: codegirl-007 Date: Sat, 22 Aug 2026 11:47:42 -0700 Subject: [PATCH] Harden auth: setup secret, throttling, session destroy, secure cookies. Replace username-based admin bootstrap with a one-time setup secret, rate-limit login/register, equalize login bcrypt timing, cap passwords at 72 bytes, destroy sessions on logout, and require Secure cookies when PORT is set. --- .env.example | 11 +++-- cmd/server/main.go | 20 +++++++-- internal/web/auth.go | 74 +++++++++++++++++++++++++++------ internal/web/auth_test.go | 57 +++++++++++++++++++++++++ internal/web/server.go | 45 +++++++++++++------- internal/web/server_test.go | 33 ++++++++++----- internal/web/throttle.go | 83 +++++++++++++++++++++++++++++++++++++ templates/register.html | 7 +++- todo.md | 7 ++-- 9 files changed, 287 insertions(+), 50 deletions(-) create mode 100644 internal/web/auth_test.go create mode 100644 internal/web/throttle.go diff --git a/.env.example b/.env.example index ec4d4f2..475bedf 100644 --- a/.env.example +++ b/.env.example @@ -5,11 +5,14 @@ LISTEN=:8080 DATABASE_URL=postgresql://user:password@host.example.com:5432/postgres?sslmode=verify-full # Required for integration tests (do not point at the runtime DATABASE_URL). # TEST_DATABASE_URL=postgresql://user:password@host.example.com:5432/plumber_test?sslmode=verify-full -# Optional: first matching registrant becomes admin only if no admin exists yet. -# Later promote/demote via /admin/users (admins only). -ADMIN_USERNAME=yourusername -# Set to 1 when serving over HTTPS +# One-time first-admin bootstrap: registrant must also POST setup_secret matching this value, +# and only while no admin exists yet. Leave unset after bootstrap. Prefer a long random string. +# ADMIN_SETUP_SECRET= +# When PORT is set (App Platform), cookies are Secure by default; SECURE_COOKIE=0 is rejected. +# Locally, set to 1 when serving over HTTPS: SECURE_COOKIE=0 +# Set to 1 only behind a trusted reverse proxy that sets X-Forwarded-For. +# TRUST_PROXY=0 # DigitalOcean Spaces (profile avatars). Leave unset to disable uploads. # SPACES_KEY= # SPACES_SECRET= diff --git a/cmd/server/main.go b/cmd/server/main.go index b73377e..ee3a8ba 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -54,9 +54,10 @@ func openDB() (*sql.DB, *store.SessionStore) { func newHandler(db *sql.DB, sessions *store.SessionStore, uploader blob.Uploader) http.Handler { srv, err := web.New(store.NewPostgres(db), sessions.Store(), plumber.TemplateFS, plumber.StaticFS, web.Config{ - AdminUsername: os.Getenv("ADMIN_USERNAME"), - SecureCookie: os.Getenv("SECURE_COOKIE") == "1", - Blob: uploader, + AdminSetupSecret: strings.TrimSpace(os.Getenv("ADMIN_SETUP_SECRET")), + SecureCookie: secureCookieFromEnv(), + TrustProxy: os.Getenv("TRUST_PROXY") == "1", + Blob: uploader, }) if err != nil { log.Fatalf("server: %v", err) @@ -64,6 +65,19 @@ func newHandler(db *sql.DB, sessions *store.SessionStore, uploader blob.Uploader return srv.Handler() } +// secureCookieFromEnv defaults to secure when PORT is set (PaaS/production) +// and refuses an explicit disable in that environment. +func secureCookieFromEnv() bool { + v := strings.TrimSpace(os.Getenv("SECURE_COOKIE")) + if strings.TrimSpace(os.Getenv("PORT")) != "" { + if v == "0" { + log.Fatal("SECURE_COOKIE=0 is not allowed when PORT is set") + } + return true + } + return v == "1" +} + func run(httpSrv *http.Server) { errCh := make(chan error, 1) go func() { diff --git a/internal/web/auth.go b/internal/web/auth.go index 683b537..1b5c9e9 100644 --- a/internal/web/auth.go +++ b/internal/web/auth.go @@ -1,6 +1,7 @@ package web import ( + "crypto/subtle" "net/http" "net/url" "regexp" @@ -14,6 +15,23 @@ import ( var usernameRe = regexp.MustCompile(`^[a-zA-Z0-9_]{3,20}$`) +const ( + minPasswordRunes = 8 + maxPasswordBytes = 72 // bcrypt truncation limit +) + +// loginDummyHash is compared when the username is unknown so login timing +// does not reveal whether an account exists (same bcrypt cost as real hashes). +var loginDummyHash = mustBcrypt("timing-dummy-not-a-real-password") + +func mustBcrypt(s string) []byte { + h, err := bcrypt.GenerateFromPassword([]byte(s), bcrypt.DefaultCost) + if err != nil { + panic(err) + } + return h +} + func safeNext(raw string) string { if raw == "" { return "/" @@ -25,6 +43,16 @@ func safeNext(raw string) string { return u.RequestURI() } +func passwordValid(password string) (ok bool, msg string) { + if utf8.RuneCountInString(password) < minPasswordRunes { + return false, "Password must be at least 8 characters." + } + if len(password) > maxPasswordBytes { + return false, "Password must be at most 72 bytes." + } + return true, "" +} + func (s *Server) handleLoginForm(w http.ResponseWriter, r *http.Request) { if currentUser(r) != nil { http.Redirect(w, r, safeNext(r.URL.Query().Get("next")), http.StatusSeeOther) @@ -43,8 +71,16 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { username := strings.TrimSpace(r.PostFormValue("username")) password := r.PostFormValue("password") next := safeNext(r.PostFormValue("next")) + if !s.allowLoginAttempt(w, r, store.NormalizeUsername(username)) { + return + } + u, err := s.store.UserByUsername(r.Context(), username) - if err != nil || bcrypt.CompareHashAndPassword([]byte(u.PasswordHash), []byte(password)) != nil { + hash := loginDummyHash + if err == nil { + hash = []byte(u.PasswordHash) + } + if err != nil || bcrypt.CompareHashAndPassword(hash, []byte(password)) != nil { w.WriteHeader(http.StatusUnauthorized) s.exec(w, "login", authPage{ page: s.basePage(r, "Sign in"), @@ -74,16 +110,20 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { if !s.requireCSRF(w, r) { return } + if !s.allowRegisterAttempt(w, r) { + return + } username := strings.TrimSpace(r.PostFormValue("username")) password := r.PostFormValue("password") + setupSecret := r.PostFormValue("setup_secret") p := authPage{page: s.basePage(r, "Create account"), Username: username} if !usernameRe.MatchString(username) { p.Error = "Username must be 3–20 letters, numbers, or underscores." s.exec(w, "register", p) return } - if utf8.RuneCountInString(password) < 8 { - p.Error = "Password must be at least 8 characters." + if ok, msg := passwordValid(password); !ok { + p.Error = msg s.exec(w, "register", p) return } @@ -93,15 +133,8 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { return } role := store.RoleUser - if s.cfg.AdminUsername != "" && store.NormalizeUsername(username) == store.NormalizeUsername(s.cfg.AdminUsername) { - n, err := s.store.CountAdmins(r.Context()) - if err != nil { - http.Error(w, "could not create account", http.StatusInternalServerError) - return - } - if n == 0 { - role = store.RoleAdmin - } + if s.consumeAdminSetup(r, setupSecret) { + role = store.RoleAdmin } u := &store.User{ Username: username, @@ -121,6 +154,23 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/", http.StatusSeeOther) } +// consumeAdminSetup grants first-admin when a strong one-time setup secret matches +// and no admin exists yet. Username alone is never enough. +func (s *Server) consumeAdminSetup(r *http.Request, provided string) bool { + want := s.cfg.AdminSetupSecret + if want == "" || provided == "" { + return false + } + if subtle.ConstantTimeCompare([]byte(provided), []byte(want)) != 1 { + return false + } + n, err := s.store.CountAdmins(r.Context()) + if err != nil || n > 0 { + return false + } + return true +} + func (s *Server) handleAuthPrompt(w http.ResponseWriter, r *http.Request) { s.exec(w, "signin-prompt", s.basePage(r, "")) } diff --git a/internal/web/auth_test.go b/internal/web/auth_test.go new file mode 100644 index 0000000..83a11cd --- /dev/null +++ b/internal/web/auth_test.go @@ -0,0 +1,57 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestPasswordMaxBytes(t *testing.T) { + if ok, _ := passwordValid(strings.Repeat("a", 8)); !ok { + t.Fatal("8 ascii runes should pass") + } + if ok, msg := passwordValid(strings.Repeat("a", 73)); ok || !strings.Contains(msg, "72") { + t.Fatalf("73 bytes should fail: ok=%v msg=%q", ok, msg) + } +} + +func TestLogoutDestroysSession(t *testing.T) { + srv, _ := newTestServer(t, Config{}) + h := srv.Handler() + name := uniq("out") + cookies := registerUser(t, h, name, "hunter22") + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/profile", nil) + for _, c := range cookies { + req.AddCookie(c) + } + h.ServeHTTP(rec, req) + if rec.Code != 200 { + t.Fatalf("profile before logout %d", rec.Code) + } + csrf := csrfFrom(rec.Body.String()) + form := strings.NewReader("_csrf=" + csrf) + req = httptest.NewRequest(http.MethodPost, "/logout", form) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + for _, c := range cookies { + req.AddCookie(c) + } + rec = httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusSeeOther { + t.Fatalf("logout %d", rec.Code) + } + postLogout := mergeCookies(cookies, rec.Result().Cookies()) + + req = httptest.NewRequest(http.MethodGet, "/profile", nil) + for _, c := range postLogout { + req.AddCookie(c) + } + rec = httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusSeeOther { + t.Fatalf("profile after logout should redirect, got %d", rec.Code) + } +} diff --git a/internal/web/server.go b/internal/web/server.go index 4be3153..eee10be 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -24,17 +24,24 @@ import ( ) type Config struct { - AdminUsername string - SecureCookie bool - Blob blob.Uploader + // AdminSetupSecret, when set, can promote the first registrant who also + // 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 } type Server struct { - store store.Store - sessions *scs.SessionManager - tmpl *template.Template - cfg Config - static http.Handler + store store.Store + sessions *scs.SessionManager + tmpl *template.Template + cfg Config + static http.Handler + loginIP *throttle + loginUser *throttle + registerIP *throttle } type page struct { @@ -124,18 +131,23 @@ func New(st store.Store, sessionStore scs.Store, templateFS fs.FS, staticFS fs.F } return &Server{ - store: st, - sessions: sessions, - tmpl: tmpl, - cfg: cfg, - static: http.StripPrefix("/static/", http.FileServer(http.FS(sub))), + store: st, + sessions: sessions, + tmpl: tmpl, + cfg: cfg, + static: http.StripPrefix("/static/", http.FileServer(http.FS(sub))), + loginIP: newThrottle(20, 15*time.Minute), + loginUser: newThrottle(10, 15*time.Minute), + registerIP: newThrottle(10, 15*time.Minute), }, nil } func (s *Server) Handler() http.Handler { r := chi.NewRouter() r.Use(middleware.RequestID) - r.Use(middleware.RealIP) + if s.cfg.TrustProxy { + r.Use(middleware.RealIP) + } r.Use(middleware.Logger) r.Use(middleware.Recoverer) r.Use(func(next http.Handler) http.Handler { @@ -507,7 +519,10 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) { if !s.requireCSRF(w, r) { return } - s.sessions.Remove(r.Context(), "user_id") + if err := s.sessions.Destroy(r.Context()); err != nil { + http.Error(w, "could not sign out", http.StatusInternalServerError) + return + } http.Redirect(w, r, "/", http.StatusSeeOther) } diff --git a/internal/web/server_test.go b/internal/web/server_test.go index c7a0da4..69fb926 100644 --- a/internal/web/server_test.go +++ b/internal/web/server_test.go @@ -105,15 +105,18 @@ func loginUser(t *testing.T, h http.Handler, username, password string) []*http. return post } -func registerUser(t *testing.T, h http.Handler, username, password string) []*http.Cookie { +func registerUser(t *testing.T, h http.Handler, username, password string, setupSecret ...string) []*http.Cookie { t.Helper() rec := httptest.NewRecorder() h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/register", nil)) pre := rec.Result().Cookies() preToken := sessionValue(pre) csrf := csrfFrom(rec.Body.String()) - form := strings.NewReader("_csrf=" + csrf + "&username=" + username + "&password=" + password) - req := httptest.NewRequest(http.MethodPost, "/register", form) + form := "_csrf=" + csrf + "&username=" + username + "&password=" + password + if len(setupSecret) > 0 && setupSecret[0] != "" { + form += "&setup_secret=" + setupSecret[0] + } + req := httptest.NewRequest(http.MethodPost, "/register", strings.NewReader(form)) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") for _, c := range pre { req.AddCookie(c) @@ -179,25 +182,35 @@ func TestRegisterLoginAsk(t *testing.T) { } } -func TestAdminSeedOnlyWhenNoAdmins(t *testing.T) { +func TestAdminSetupSecretOnlyWhenNoAdmins(t *testing.T) { mem := store.NewMemory() + secret := "one-time-admin-setup-secret" adminName := uniq("seed") - srv := newTestServerStore(t, mem, Config{AdminUsername: adminName}) + srv := newTestServerStore(t, mem, Config{AdminSetupSecret: secret}) h := srv.Handler() - registerUser(t, h, adminName, "hunter22") + + plain := uniq("plain") + registerUser(t, h, plain, "hunter22") + uPlain, err := mem.UserByUsername(context.Background(), plain) + if err != nil || uPlain.Admin() { + t.Fatalf("register without setup secret must stay user: %+v %v", uPlain, err) + } + + registerUser(t, h, adminName, "hunter22", secret) u, err := mem.UserByUsername(context.Background(), adminName) if err != nil || !u.Admin() { - t.Fatalf("first matching registrant should be admin: %+v %v", u, err) + t.Fatalf("setup secret registrant should be admin: %+v %v", u, err) } + later := uniq("later") - srv2 := newTestServerStore(t, mem, Config{AdminUsername: later}) - registerUser(t, srv2.Handler(), later, "hunter22") + srv2 := newTestServerStore(t, mem, Config{AdminSetupSecret: secret}) + registerUser(t, srv2.Handler(), later, "hunter22", secret) u2, err := mem.UserByUsername(context.Background(), later) if err != nil { t.Fatal(err) } if u2.Admin() { - t.Fatal("later admin username must stay user when an admin already exists") + t.Fatal("setup secret must not grant admin once an admin already exists") } } diff --git a/internal/web/throttle.go b/internal/web/throttle.go new file mode 100644 index 0000000..73f31c0 --- /dev/null +++ b/internal/web/throttle.go @@ -0,0 +1,83 @@ +package web + +import ( + "net" + "net/http" + "strings" + "sync" + "time" +) + +// throttle is a simple sliding-window rate limiter for auth endpoints. +type throttle struct { + mu sync.Mutex + hits map[string][]time.Time + limit int + window time.Duration +} + +func newThrottle(limit int, window time.Duration) *throttle { + return &throttle{ + hits: map[string][]time.Time{}, + limit: limit, + window: window, + } +} + +func (t *throttle) allow(key string) bool { + if t == nil || key == "" { + return true + } + t.mu.Lock() + defer t.mu.Unlock() + now := time.Now() + cutoff := now.Add(-t.window) + xs := t.hits[key] + n := 0 + for _, ts := range xs { + if ts.After(cutoff) { + xs[n] = ts + n++ + } + } + xs = xs[:n] + if len(xs) >= t.limit { + t.hits[key] = xs + return false + } + t.hits[key] = append(xs, now) + return true +} + +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 + } + return host +} + +func authTooMany(w http.ResponseWriter) { + http.Error(w, "too many attempts; try again later", http.StatusTooManyRequests) +} + +func (s *Server) allowLoginAttempt(w http.ResponseWriter, r *http.Request, usernameKey string) bool { + if !s.loginIP.allow(s.clientIP(r)) || !s.loginUser.allow(usernameKey) { + authTooMany(w) + return false + } + return true +} + +func (s *Server) allowRegisterAttempt(w http.ResponseWriter, r *http.Request) bool { + if !s.registerIP.allow(s.clientIP(r)) { + authTooMany(w) + return false + } + return true +} diff --git a/templates/register.html b/templates/register.html index 1e2c273..432483d 100644 --- a/templates/register.html +++ b/templates/register.html @@ -10,8 +10,11 @@

3–20 letters, numbers, or underscores.

- -

At least 8 characters.

+ +

At least 8 characters (max 72 bytes).

+ + +

Only needed once to create the first admin. Leave blank otherwise.

Already have an account? Sign in

diff --git a/todo.md b/todo.md index b78651e..c509604 100644 --- a/todo.md +++ b/todo.md @@ -7,11 +7,12 @@ From the project review. Priority order within each section. - [x] **Persist sessions** — Custom sqlc-backed `SessionStore` (scs API kept; no `postgresstore`). - [x] **Drop Dockerfile** — DigitalOcean App Platform buildpack from `go.mod`. - [x] **Rune-safe truncation** — Form fields truncate by runes. -- [x] **Admin bootstrap** — `ADMIN_USERNAME` seeds first admin only when none exist; `/admin/users` for promote/demote. +- [x] **Admin bootstrap** — One-time `ADMIN_SETUP_SECRET` on register (not username alone); `/admin/users` for promote/demote. - [x] **Graceful shutdown** — Signal-aware `http.Server.Shutdown` with timeouts. - [x] **Handler tests** — Vote HTMX, answer/hide, CSRF, session rotation via in-memory `Store` (no Postgres for web suite). - [x] **App Platform listen port** — Prefers `PORT`, then `LISTEN`, then `:8080`. - [x] **Prod DB = PlanetScale Postgres** — Required `DATABASE_URL`; DSN cleanup for PlanetScale/libpq-only params. +- [x] **Auth hardening** — Rate limits, timing-safe login, password ≤72 bytes, logout destroys session, Secure cookies required when `PORT` is set. ## Docs & ops @@ -20,12 +21,10 @@ From the project review. Priority order within each section. ## Smaller / later -- [ ] Rate-limit login/register (bcrypt helps; still open to brute-force). - [ ] Cursor pagination UI when hunt/profile lists hit their row limits. - [ ] Optional Postgres integration tests (`TEST_DATABASE_URL`) for sqlc SessionStore / advisory locks. ## Suggested order of attack -1. Short README (run, env, admin, App Platform + PlanetScale). +1. Short README (run, env, admin setup secret, App Platform + PlanetScale). 2. Migrations plan before the next schema change. -3. Rate-limit auth endpoints.