Address Gadfly review on #4: TOCTOU, sliding cookie, CSRF, argon2

Fixes from the PR #23 adversarial review (graded 35 real / 1 false positive):

Security / correctness
- Race-free registration: is_admin and the registration gate are now
  computed atomically inside a single INSERT...SELECT, so concurrent
  first registrations can't both become admin or bypass closed
  registration (fixed the whole TOCTOU cluster).
- Sliding session now reaches the browser: ResolveSession returns the
  current expiry and requireAuth re-sets the cookie, so active users
  aren't logged out 30 days after login regardless of activity.
- Login CSRF: csrfGuard rejects state-changing requests whose Origin
  doesn't match PANSY_BASE_URL (no-op when unset, so the dev proxy is
  unaffected). SameSite=Lax alone didn't cover this.
- argon2id tuned to RFC 9106's second recommended profile (t=3).
- Timing equalizer can't fail open: the dummy hash is derived
  deterministically (fixed salt, no RNG) so it's always present.
- Password length (<=1024) enforced in the service for both register
  and login, not just HTTP binding tags; login rejects over-long input
  before spending argon2 work.

Error handling / robustness
- Login logs a malformed stored hash instead of silently treating it as
  a wrong password.
- Best-effort session writes (Touch/Delete during renewal, expiry, and
  corrupt-expiry cleanup) now log on failure.
- index sessions.expires_at via new migration 0002 (0001 is immutable).

Maintainability
- Extract startSessionAndRespond and abortUnauthenticated; make
  writeServiceError a free function; consistent error handling in
  decodeHash; doc/comment fixes.

Tests: over-long password, CSRF guard (cross-origin/same-origin/dev
no-op), and cookie refresh on authenticated requests; migration-version
assertions bumped to 2.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi

internal/service/auth.go

@@ -3,12 +3,17 @@ package service
 import (
 	"context"
 	"errors"
+	"log/slog"
 	"strings"
 	"time"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
 )
 
+// maxPasswordLen caps accepted password length. It bounds argon2 input and
+// request work, and sits far above any real password.
+const maxPasswordLen = 1024
+
 // RegisterInput is the payload for local self-service signup.
 type RegisterInput struct {
 	Email       string
@@ -46,27 +51,22 @@ func (s *Service) Register(ctx context.Context, in RegisterInput) (*domain.User,
 
 	email := normalizeEmail(in.Email)
 	displayName := strings.TrimSpace(in.DisplayName)
-	if email == "" || displayName == "" || in.Password == "" {
+	if email == "" || displayName == "" || in.Password == "" || len(in.Password) > maxPasswordLen {
 		return nil, domain.ErrInvalidInput
 	}
 
-	count, err := s.store.CountUsers(ctx)
-	if err != nil {
-		return nil, err
-	}
-	// Closed registration still allows the very first account so a locked-down
-	// instance can be bootstrapped without editing config.
-	if count > 0 && !s.cfg.RegistrationOpen() {
-		return nil, domain.ErrRegistrationClosed
-	}
-
-	// Friendly duplicate check. The UNIQUE index is the real guard against a
-	// race; that path surfaces as a generic insert error (500), acceptable at
-	// household scale.
-	if _, err := s.store.GetUserByEmail(ctx, email); err == nil {
-		return nil, domain.ErrEmailTaken
-	} else if !errors.Is(err, domain.ErrNotFound) {
-		return nil, err
+	// Cheap best-effort gate so a closed instance doesn't burn argon2 work on
+	// signups it will reject anyway. The authoritative, race-free gate — plus
+	// atomic first-user-is-admin assignment and duplicate-email detection — lives
+	// in store.CreateUser's single INSERT.
+	if !s.cfg.RegistrationOpen() {
+		n, err := s.store.CountUsers(ctx)
+		if err != nil {
+			return nil, err
+		}
+		if n > 0 {
+			return nil, domain.ErrRegistrationClosed
+		}
 	}
 
 	hash, err := hashPassword(in.Password)
@@ -78,8 +78,7 @@ func (s *Service) Register(ctx context.Context, in RegisterInput) (*domain.User,
 		Email:        email,
 		DisplayName:  displayName,
 		PasswordHash: &hash,
-		IsAdmin:      count == 0,
-	})
+	}, s.cfg.RegistrationOpen())
 }
 
 // Login verifies an email/password pair and returns the user. Unknown-email and
@@ -89,6 +88,10 @@ func (s *Service) Login(ctx context.Context, email, password string) (*domain.Us
 	if !s.cfg.LocalAuth {
 		return nil, domain.ErrLocalAuthDisabled
 	}
+	if len(password) > maxPasswordLen {
+		// No stored password is this long; reject without spending argon2 work.
+		return nil, domain.ErrInvalidCredentials
+	}
 
 	u, err := s.store.GetUserByEmail(ctx, normalizeEmail(email))
 	switch {
@@ -105,7 +108,13 @@ func (s *Service) Login(ctx context.Context, email, password string) (*domain.Us
 	}
 
 	ok, err := verifyPassword(*u.PasswordHash, password)
-	if err != nil || !ok {
+	if err != nil {
+		// A stored hash we can't parse is a data problem, not a wrong password;
+		// surface it so a corrupt row doesn't silently lock a user out unnoticed.
+		slog.Error("service: malformed stored password hash", "user_id", u.ID, "error", err)
+		return nil, domain.ErrInvalidCredentials
+	}
+	if !ok {
 		return nil, domain.ErrInvalidCredentials
 	}
 	return u, nil
@@ -129,38 +138,53 @@ func (s *Service) CreateSession(ctx context.Context, userID int64) (token string
 	return raw, exp, nil
 }
 
-// ResolveSession validates a raw bearer token and returns its user. An expired
-// session is deleted and treated as absent (domain.ErrNotFound). A still-valid
-// session has its expiry slid forward, but only when that moves it by more than
-// an hour, so a busy client doesn't write on every request.
-func (s *Service) ResolveSession(ctx context.Context, rawToken string) (*domain.User, error) {
+// ResolveSession validates a raw bearer token and returns its user and the
+// session's current expiry (so the caller can slide the client cookie in
+// lockstep with the server). An expired session is deleted and treated as absent
+// (domain.ErrNotFound). A still-valid session has its expiry slid forward, but
+// only when that moves it by more than an hour, so a busy client doesn't write
+// on every request.
+func (s *Service) ResolveSession(ctx context.Context, rawToken string) (*domain.User, time.Time, error) {
 	if rawToken == "" {
-		return nil, domain.ErrNotFound
+		return nil, time.Time{}, domain.ErrNotFound
 	}
 	hash := hashToken(rawToken)
 	sess, err := s.store.GetSession(ctx, hash)
 	if err != nil {
-		return nil, err
+		return nil, time.Time{}, err
 	}
 
 	exp, err := parseTime(sess.ExpiresAt)
 	if err != nil {
 		// A corrupt expiry means we can't trust the session; drop it.
-		_ = s.store.DeleteSession(ctx, hash)
-		return nil, domain.ErrNotFound
+		if delErr := s.store.DeleteSession(ctx, hash); delErr != nil {
+			slog.Warn("service: deleting session with corrupt expiry", "error", delErr)
+		}
+		return nil, time.Time{}, domain.ErrNotFound
 	}
 
 	now := s.now()
 	if !now.Before(exp) {
-		_ = s.store.DeleteSession(ctx, hash)
-		return nil, domain.ErrNotFound
+		if delErr := s.store.DeleteSession(ctx, hash); delErr != nil {
+			slog.Warn("service: deleting expired session", "error", delErr)
+		}
+		return nil, time.Time{}, domain.ErrNotFound
 	}
 
 	if newExp := now.Add(sessionTTL); newExp.Sub(exp) > time.Hour {
-		_ = s.store.TouchSession(ctx, hash, formatTime(newExp))
+		if err := s.store.TouchSession(ctx, hash, formatTime(newExp)); err != nil {
+			// Non-fatal: the session is still valid at its current expiry.
+			slog.Warn("service: sliding session expiry", "error", err)
+		} else {
+			exp = newExp
+		}
 	}
 
-	return s.store.GetUserByID(ctx, sess.UserID)
+	user, err := s.store.GetUserByID(ctx, sess.UserID)
+	if err != nil {
+		return nil, time.Time{}, err
+	}
+	return user, exp, nil
 }
 
 // Logout deletes the session behind a raw bearer token. It is idempotent.

internal/service/auth_test.go

@@ -3,6 +3,7 @@ package service
 import (
 	"context"
 	"errors"
+	"strings"
 	"testing"
 	"time"
 
@@ -98,6 +99,20 @@ func TestRegistrationClosedAllowsBootstrapThenBlocks(t *testing.T) {
 	}
 }
 
+func TestRejectsOverlongPassword(t *testing.T) {
+	s := newTestService(t, openConfig())
+	long := strings.Repeat("a", maxPasswordLen+1)
+
+	if _, err := s.Register(context.Background(), RegisterInput{Email: "[email protected]", DisplayName: "A", Password: long}); !errors.Is(err, domain.ErrInvalidInput) {
+		t.Errorf("register overlong err = %v, want ErrInvalidInput", err)
+	}
+
+	mustRegister(t, s, "[email protected]", "Bob", "password123")
+	if _, err := s.Login(context.Background(), "[email protected]", long); !errors.Is(err, domain.ErrInvalidCredentials) {
+		t.Errorf("login overlong err = %v, want ErrInvalidCredentials", err)
+	}
+}
+
 func TestLocalAuthDisabledRejectsRegisterAndLogin(t *testing.T) {
 	cfg := openConfig()
 	cfg.LocalAuth = false
@@ -139,7 +154,7 @@ func TestLoginRejectsOIDCOnlyUser(t *testing.T) {
 	iss, sub := "https://idp.example", "subject-1"
 	if _, err := s.store.CreateUser(context.Background(), &domain.User{
 		Email: "[email protected]", DisplayName: "O", OIDCIssuer: &iss, OIDCSubject: &sub,
-	}); err != nil {
+	}, true); err != nil {
 		t.Fatalf("seed oidc user: %v", err)
 	}
 	if _, err := s.Login(context.Background(), "[email protected]", "anything"); !errors.Is(err, domain.ErrInvalidCredentials) {
@@ -156,29 +171,32 @@ func TestSessionLifecycle(t *testing.T) {
 		t.Fatalf("CreateSession: %v", err)
 	}
 
-	got, err := s.ResolveSession(context.Background(), token)
+	got, exp, err := s.ResolveSession(context.Background(), token)
 	if err != nil {
 		t.Fatalf("ResolveSession: %v", err)
 	}
 	if got.ID != u.ID {
 		t.Errorf("resolved user %d, want %d", got.ID, u.ID)
 	}
+	if !exp.After(s.now()) {
+		t.Errorf("resolved expiry %v is not in the future", exp)
+	}
 
 	// Logout invalidates it.
 	if err := s.Logout(context.Background(), token); err != nil {
 		t.Fatalf("Logout: %v", err)
 	}
-	if _, err := s.ResolveSession(context.Background(), token); !errors.Is(err, domain.ErrNotFound) {
+	if _, _, err := s.ResolveSession(context.Background(), token); !errors.Is(err, domain.ErrNotFound) {
 		t.Errorf("resolve after logout err = %v, want ErrNotFound", err)
 	}
 }
 
 func TestResolveSessionRejectsGarbageToken(t *testing.T) {
 	s := newTestService(t, openConfig())
-	if _, err := s.ResolveSession(context.Background(), "not-a-real-token"); !errors.Is(err, domain.ErrNotFound) {
+	if _, _, err := s.ResolveSession(context.Background(), "not-a-real-token"); !errors.Is(err, domain.ErrNotFound) {
 		t.Errorf("garbage token err = %v, want ErrNotFound", err)
 	}
-	if _, err := s.ResolveSession(context.Background(), ""); !errors.Is(err, domain.ErrNotFound) {
+	if _, _, err := s.ResolveSession(context.Background(), ""); !errors.Is(err, domain.ErrNotFound) {
 		t.Errorf("empty token err = %v, want ErrNotFound", err)
 	}
 }
@@ -197,7 +215,7 @@ func TestSessionExpiryAndLazyDeletion(t *testing.T) {
 
 	// Jump past the 30-day TTL: the session must be treated as gone...
 	s.now = func() time.Time { return base.Add(sessionTTL + time.Hour) }
-	if _, err := s.ResolveSession(context.Background(), token); !errors.Is(err, domain.ErrNotFound) {
+	if _, _, err := s.ResolveSession(context.Background(), token); !errors.Is(err, domain.ErrNotFound) {
 		t.Fatalf("expired resolve err = %v, want ErrNotFound", err)
 	}
 	// ...and lazily deleted from the store.
@@ -219,9 +237,13 @@ func TestSessionSlidingRenewal(t *testing.T) {
 
 	// Use it 10 days later; expiry should slide forward.
 	s.now = func() time.Time { return base.Add(10 * 24 * time.Hour) }
-	if _, err := s.ResolveSession(context.Background(), token); err != nil {
+	_, resolvedExp, err := s.ResolveSession(context.Background(), token)
+	if err != nil {
 		t.Fatalf("ResolveSession: %v", err)
 	}
+	if !resolvedExp.After(firstExp) {
+		t.Errorf("returned expiry did not slide: %v not after %v", resolvedExp, firstExp)
+	}
 	sess, err := s.store.GetSession(context.Background(), hashToken(token))
 	if err != nil {
 		t.Fatalf("GetSession: %v", err)

internal/service/password.go

@@ -11,13 +11,13 @@ import (
 	"golang.org/x/crypto/argon2"
 )
 
-// argon2id parameters. ~64 MiB memory / 1 pass / 4 lanes is the interactive
-// profile recommended by the argon2 authors and is comfortable on a
-// self-hosted box. They are encoded into every stored hash, so raising them
-// later leaves old hashes verifiable.
+// argon2id parameters: RFC 9106's second recommended profile (m=64 MiB, t=3,
+// p=4) — the memory-constrained option, appropriate for a self-hosted box where
+// the 2 GiB first profile is too heavy. They are encoded into every stored hash,
+// so raising them later leaves old hashes verifiable.
 const (
 	argonMemKiB  = 64 * 1024 // 64 MiB
-	argonTime    = 1
+	argonTime    = 3
 	argonThreads = 4
 	argonKeyLen  = 32
 	argonSaltLen = 16
@@ -25,12 +25,27 @@ const (
 
 // errBadHash marks a stored hash string that could not be parsed — a data or
 // programming error, not a wrong password. Callers treat it as an auth failure
-// but should log it.
+// and log it (Login does).
 var errBadHash = errors.New("service: malformed password hash")
 
 // b64 is the padding-free base64 used inside the PHC-style hash string.
 var b64 = base64.RawStdEncoding
 
+// timingHash is a valid argon2id hash used to equalize login response time when
+// an email is unknown (see Service.dummyHash). It uses a fixed salt rather than
+// crypto/rand so it can never fail to be produced at startup — an all-important
+// property, since a missing timing hash would silently re-open account
+// enumeration. It guards nothing, so a static salt is safe; deriving it from the
+// live argon parameters keeps its cost matched to a real verify.
+func timingHash() string {
+	salt := []byte("pansy-timing-eq!") // exactly argonSaltLen (16) bytes
+	key := argon2.IDKey([]byte("x"), salt, argonTime, argonMemKiB, argonThreads, argonKeyLen)
+	return fmt.Sprintf("$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s",
+		argon2.Version, argonMemKiB, argonTime, argonThreads,
+		b64.EncodeToString(salt), b64.EncodeToString(key),
+	)
+}
+
 // hashPassword returns a self-describing argon2id hash in the PHC string format
 // "$argon2id$v=19$m=...,t=...,p=...$salt$hash" (all base64, no padding).
 func hashPassword(password string) (string, error) {
@@ -57,26 +72,31 @@ func verifyPassword(encoded, password string) (bool, error) {
 }
 
 // decodeHash parses a PHC-format argon2id string into its parameters, salt, and
-// derived key.
+// derived key. Any structural problem returns errBadHash (with zero values) so
+// the caller never has to distinguish parse failures.
 func decodeHash(encoded string) (mem, t uint32, threads uint8, salt, key []byte, err error) {
-	parts := strings.Split(encoded, "$")
-	// ["", "argon2id", "v=19", "m=..,t=..,p=..", "<salt>", "<hash>"]
-	if len(parts) != 6 || parts[1] != "argon2id" {
+	fail := func() (uint32, uint32, uint8, []byte, []byte, error) {
 		return 0, 0, 0, nil, nil, errBadHash
 	}
 
-	var version int
-	if _, err := fmt.Sscanf(parts[2], "v=%d", &version); err != nil || version != argon2.Version {
-		return 0, 0, 0, nil, nil, errBadHash
+	parts := strings.Split(encoded, "$")
+	// ["", "argon2id", "v=19", "m=..,t=..,p=..", "<salt>", "<hash>"]
+	if len(parts) != 6 || parts[1] != "argon2id" {
+		return fail()
 	}
-	if _, err := fmt.Sscanf(parts[3], "m=%d,t=%d,p=%d", &mem, &t, &threads); err != nil {
-		return 0, 0, 0, nil, nil, errBadHash
+
+	var version int
+	if _, e := fmt.Sscanf(parts[2], "v=%d", &version); e != nil || version != argon2.Version {
+		return fail()
+	}
+	if _, e := fmt.Sscanf(parts[3], "m=%d,t=%d,p=%d", &mem, &t, &threads); e != nil {
+		return fail()
 	}
 	if salt, err = b64.DecodeString(parts[4]); err != nil {
-		return 0, 0, 0, nil, nil, errBadHash
+		return fail()
 	}
 	if key, err = b64.DecodeString(parts[5]); err != nil || len(key) == 0 {
-		return 0, 0, 0, nil, nil, errBadHash
+		return fail()
 	}
 	return mem, t, threads, salt, key, nil
 }

internal/service/password_test.go

@@ -45,7 +45,7 @@ func TestVerifyPasswordRejectsMalformedHash(t *testing.T) {
 		"not-a-hash",
 		"$argon2id$v=19$m=65536,t=1,p=4$onlyfourparts",
 		"$argon2i$v=19$m=65536,t=1,p=4$c2FsdA$aGFzaA", // wrong variant
-		"$argon2id$v=1$m=65536,t=1,p=4$c2FsdA$aGFzaA",  // wrong version
+		"$argon2id$v=1$m=65536,t=1,p=4$c2FsdA$aGFzaA", // wrong version
 	} {
 		if _, err := verifyPassword(bad, "whatever"); err == nil {
 			t.Errorf("verifyPassword(%q) err = nil, want errBadHash", bad)

internal/service/service.go

@@ -1,9 +1,11 @@
-// Package service is pansy's business-logic seam: every operation is a method on
-// *Service taking (ctx, actor, args), and all permission checks and invariants
-// live here rather than in the HTTP handlers. REST handlers (internal/api) and,
-// later, agent tools (internal/agent) are thin adapters over these methods, so
-// both inherit the same rules. This file holds the shared plumbing; feature
-// methods live alongside it (auth.go, and gardens/objects/… in later issues).
+// Package service is pansy's business-logic seam: all permission checks and
+// invariants live here rather than in the HTTP handlers. Resource operations
+// take (ctx, actor, args) so every rule is enforced regardless of caller; the
+// auth operations here are the exception — they establish the actor, so they
+// take credentials rather than one. REST handlers (internal/api) and, later,
+// agent tools (internal/agent) are thin adapters over these methods, so both
+// inherit the same rules. This file holds the shared plumbing; feature methods
+// live alongside it (auth.go, and gardens/objects/… in later issues).
 package service
 
 import (
@@ -12,7 +14,6 @@ import (
 	"encoding/base64"
 	"encoding/hex"
 	"fmt"
-	"log/slog"
 	"time"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
@@ -33,23 +34,21 @@ type Service struct {
 	cfg   *config.Config
 	// now is the clock, injectable so tests can advance time (session expiry).
 	now func() time.Time
-	// dummyHash is a valid argon2id hash of a throwaway password. Login verifies
-	// against it when an email is unknown so the response time doesn't reveal
-	// whether an account exists.
+	// dummyHash is a valid argon2id hash Login verifies against when an email is
+	// unknown, so response time doesn't reveal whether an account exists. It is
+	// produced by timingHash (fixed salt, no RNG) so it is always present — an
+	// empty one would silently re-open account enumeration.
 	dummyHash string
 }
 
-// New constructs a Service. It precomputes a dummy password hash used to
-// equalize login timing; if that fails (it shouldn't), login still works but
-// loses the timing defense.
+// New constructs a Service.
 func New(st *store.DB, cfg *config.Config) *Service {
-	s := &Service{store: st, cfg: cfg, now: time.Now}
-	if h, err := hashPassword("pansy-timing-equalizer-not-a-real-password"); err != nil {
-		slog.Warn("service: could not precompute login timing hash", "error", err)
-	} else {
-		s.dummyHash = h
+	return &Service{
+		store:     st,
+		cfg:       cfg,
+		now:       time.Now,
+		dummyHash: timingHash(),
 	}
-	return s
 }
 
 // formatTime renders a time as pansy's canonical UTC string.