panel,mail: fail closed on the rate-limit race, session create and app delete

The level-2 limiter counted stored plus in-flight messages and reserved its own slot in two critical sections, so SMTP sessions that overlapped could each take the last free slot; tryAdmit now does both under one lock. A session that cannot be written no longer yields a cookie the browser would carry while every request bounced to /login. Deleting an application clears its SASL account before its registry row, matching domain delete, so a saslpasswd2 failure leaves a retryable application rather than an account that still authenticates.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

CHANGELOG.md

@@ -24,6 +24,26 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
   principal's own domains and applications before the query runs, so a
   hand-written URL cannot widen the scope. Global administrators are unaffected.
 
+- mail (level-2 rate limit): the ceiling is no longer overshot by messages that
+  arrive at the same instant. The milter counted the stored and in-flight
+  messages and reserved its own slot in two separate steps, so several SMTP
+  sessions could pass the same check before any of them had reserved. Counting
+  and reserving now happen as one operation, and the ceiling is handed out
+  exactly as many times as configured. Postfix's level-1 limit remains the
+  backstop and the level-2 check stays fail-open on store errors.
+
+- panel (sign-in): a session that cannot be written to the database no longer
+  produces a session cookie. The login used to log the failure, set the cookie
+  and redirect to the dashboard, leaving the browser looking signed in while
+  every request bounced back to `/login`; it now fails closed with an error on
+  the sign-in page.
+
+- panel (applications): deleting an application removes its SASL credentials
+  before its registry row. If `saslpasswd2` fails, the application stays listed
+  and the delete can be retried, instead of leaving a hidden account that could
+  still authenticate to Postfix. This matches the order domain deletion already
+  used.
+
 ### Changed
 
 - ci: gofmt on eight files that failed the formatting workflow check (panel

docs/plans/code-review.md

@@ -504,11 +504,15 @@ the list, the fragment, or query parameters. `go test ./...` green.
 
 **Model: Opus.**
 
-- [ ] `inflight.tryAdmit` (count + reserve under one lock). Extend milter
+- [x] `inflight.tryAdmit` (count + reserve under one lock). Extend milter
       tests with overlapping `MailFrom` (true concurrency, not sequential).
-- [ ] `sessionStore.Create` returns an error; login does not set a cookie on
+      Two tests: concurrent `MailFrom` sessions gated so they all read the
+      stored count before anyone reserves (exactly one admitted), and a
+      saturation test on `tryAdmit` that overshoots the ceiling whenever count
+      and reserve are separate critical sections.
+- [x] `sessionStore.Create` returns an error; login does not set a cookie on
       failure.
-- [ ] `app.Service.Delete`: SASL (and rate-limit row) before or compensating
+- [x] `app.Service.Delete`: SASL (and rate-limit row) before or compensating
       with the registry row; match domain-delete ordering. Test the failure
       path with a fake SASL that errors.
 

internal/app/service.go

@@ -166,14 +166,24 @@ func (s *Service) RegeneratePassword(id int64) (string, error) {
 // Delete removes an application: its SASL account, its registry row (and address
 // rows via cascade) and its sender-map bindings, then reloads Postfix (spec
 // 7.2.8). The domain and other applications are untouched.
+//
+// The order matches domain deletion: the SASL account goes first, while the
+// login is still in the registry. Dropping the row first would, on a
+// saslpasswd2 failure, leave an account that can still authenticate to Postfix
+// but that the panel no longer knows about — an orphan no operator can see or
+// remove. Failing before the row is deleted is recoverable: the application is
+// still listed and the delete can be retried.
 func (s *Service) Delete(id int64) error {
-	a, err := s.store.DeleteApplication(id)
+	a, err := s.store.GetApplication(id)
 	if err != nil {
 		return err
 	}
 	if err := s.sasl.Delete(a.Login); err != nil {
 		return err
 	}
+	if _, err := s.store.DeleteApplication(id); err != nil {
+		return err
+	}
 	// Drop the application's level-2 limit, if any (guide § Rate limiting);
 	// rate_limits has no cascade of its own.
 	if err := s.store.DeleteRateLimit(store.RateLimitScopeApp, id); err != nil {

internal/app/service_test.go

@@ -181,6 +181,37 @@ func TestServiceDelete(t *testing.T) {
 	}
 }
 
+// If sasldb2 cannot be updated the application must stay in the registry: an
+// account that still authenticates but has no panel row is invisible to the
+// operator and cannot be deleted again.
+func TestServiceDeleteKeepsRowWhenSASLFails(t *testing.T) {
+	svc, st, rec, _ := newServiceHarness(t)
+	d := addDomain(t, st, "example.com")
+	a, _, err := svc.Create(d.ID, "app1", store.AddressModeWildcard, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	rec.failNext = true // saslpasswd2 -d fails
+	if err := svc.Delete(a.ID); err == nil {
+		t.Fatal("Delete reported success although the SASL account was not removed")
+	}
+	apps, _ := st.ListApplicationsByDomain(d.ID)
+	if len(apps) != 1 {
+		t.Fatalf("registry row dropped while the SASL account can still authenticate: %+v", apps)
+	}
+	if _, ok := rec.set["app1"]; !ok {
+		t.Fatal("SASL account gone despite the failure — the harness no longer proves the ordering")
+	}
+	// The delete is retryable now that the row is still there.
+	if err := svc.Delete(a.ID); err != nil {
+		t.Fatalf("retried Delete: %v", err)
+	}
+	if _, ok := rec.set["app1"]; ok {
+		t.Error("SASL account not deleted on retry")
+	}
+}
+
 func TestServiceUpdateMode(t *testing.T) {
 	svc, st, _, maps := newServiceHarness(t)
 	d := addDomain(t, st, "example.com")

internal/milter/inflight.go

@@ -36,6 +36,29 @@ type inflight struct {
 	m  map[string]map[*reservation]struct{}
 }
 
+// tryAdmit decides whether one more message may be sent for key and, if so,
+// claims its slot — both under a single lock. Counting and reserving in two
+// separate critical sections would let two SMTP sessions that reach MAIL FROM
+// at the same moment observe the same total and each take the last free slot,
+// which is exactly the overshoot the in-flight registry exists to prevent.
+//
+// stored is the count the send log already holds for the limit's window and max
+// is the ceiling; the caller supplies both because only it can query the store.
+// The returned total is what was measured, for the refusal log line.
+func (f *inflight) tryAdmit(key string, since time.Time, stored, max int64) (*reservation, int64, bool) {
+	if f == nil {
+		return nil, stored, stored < max // no in-flight accounting (tests)
+	}
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	total := stored + f.countLocked(key, since)
+	if total >= max {
+		return nil, total, false
+	}
+	return f.reserveLocked(key), total, true
+}
+
 // count returns how many reservations for key were taken within the limit's
 // window (at or after since), pruning any that outlived reservationTTL.
 func (f *inflight) count(key string, since time.Time) int64 {
@@ -45,6 +68,11 @@ func (f *inflight) count(key string, since time.Time) int64 {
 	f.mu.Lock()
 	defer f.mu.Unlock()
 
+	return f.countLocked(key, since)
+}
+
+// countLocked is count's body; the caller holds mu.
+func (f *inflight) countLocked(key string, since time.Time) int64 {
 	set := f.m[key]
 	cutoff := time.Now().Add(-reservationTTL)
 	var n int64
@@ -63,14 +91,8 @@ func (f *inflight) count(key string, since time.Time) int64 {
 	return n
 }
 
-// reserve claims a slot for key until the message is recorded or released.
-func (f *inflight) reserve(key string) *reservation {
-	if f == nil {
-		return nil
-	}
-	f.mu.Lock()
-	defer f.mu.Unlock()
-
+// reserveLocked is reserve's body; the caller holds mu.
+func (f *inflight) reserveLocked(key string) *reservation {
 	if f.m == nil {
 		f.m = make(map[string]map[*reservation]struct{})
 	}

internal/milter/milter_test.go

@@ -3,6 +3,7 @@ package milter
 import (
 	"errors"
 	"net"
+	"sync"
 	"testing"
 	"time"
 
@@ -15,7 +16,10 @@ import (
 // swallows recorder errors and still accepts the message. By default it reports
 // no configured rate limit, so the level-2 check is inert unless a test sets
 // limits (see fakeRecorder fields).
+// mu guards the recorded slices so several sessions may drive one recorder
+// concurrently, as they do in the real server.
 type fakeRecorder struct {
+	mu       sync.Mutex
 	entries  []store.SendLogEntry
 	rejected []store.SendLogEntry
 	fail     bool
@@ -27,17 +31,25 @@ type fakeRecorder struct {
 	counts    map[string]int64
 	lookupErr error
 	countErr  error
+
+	// onCount, if set, runs inside CountMessages. It lets a test hold every
+	// racing session at the store lookup until they can all proceed together.
+	onCount func()
 }
 
 func (f *fakeRecorder) InsertQueued(e store.SendLogEntry) error {
 	if f.fail {
 		return errors.New("boom")
 	}
+	f.mu.Lock()
+	defer f.mu.Unlock()
 	f.entries = append(f.entries, e)
 	return nil
 }
 
 func (f *fakeRecorder) InsertRejected(e store.SendLogEntry) error {
+	f.mu.Lock()
+	defer f.mu.Unlock()
 	f.rejected = append(f.rejected, e)
 	return nil
 }
@@ -57,6 +69,9 @@ func (f *fakeRecorder) CountMessages(scope, ref string, _ time.Time) (int64, err
 	if f.countErr != nil {
 		return 0, f.countErr
 	}
+	if f.onCount != nil {
+		f.onCount()
+	}
 	return f.counts[scope+"|"+ref], nil
 }
 
@@ -415,6 +430,58 @@ func TestRateLimitCountsInFlightMessages(t *testing.T) {
 	}
 }
 
+// The case above is sequential: the second session reads the stored count after
+// the first has already reserved. Here every session reads it first — the gate
+// holds them all inside the lookup — which is what concurrent SMTP connections
+// actually do. However many then race for the single free slot, exactly one may
+// pass. (TestTryAdmitHandsOutEachSlotOnce is the test that fails when counting
+// and reserving are not one step; this one pins the session-level behaviour.)
+func TestRateLimitAdmitsOnlyOneRacingSession(t *testing.T) {
+	rec := limitedRecorder(4) // one below the ceiling of 5
+	fl := &inflight{}
+	gate := make(chan struct{})
+
+	// Every session is held inside the stored-count lookup until all of them
+	// have read it, which is the state the race needs: none of them can see
+	// another's reservation, because none has been taken yet.
+	const racers = 32
+	var atCount, done sync.WaitGroup
+	atCount.Add(racers)
+	go func() { atCount.Wait(); close(gate) }()
+	rec.onCount = func() { atCount.Done(); <-gate }
+
+	responses := make([]milter.Response, racers)
+	for i := range racers {
+		done.Add(1)
+		go func() {
+			defer done.Done()
+			// Connect is skipped so every goroutine starts from the same point;
+			// the client IP is what Connect would have captured.
+			s := &session{rec: rec, flight: fl, clientIP: limitIP}
+			resp, err := s.MailFrom("a@example.com", mods(map[string]string{"auth_authen": "app1"}))
+			if err != nil {
+				resp = nil // reported as a missing Continue below
+			}
+			responses[i] = resp
+		}()
+	}
+	done.Wait()
+
+	admitted := 0
+	for _, resp := range responses {
+		if resp == milter.RespContinue {
+			admitted++
+		}
+	}
+	if admitted != 1 {
+		t.Fatalf("%d of %d racing sessions admitted, want exactly 1 — the last slot was handed out twice",
+			admitted, racers)
+	}
+	if n := fl.count(store.RateLimitScopeDomain+"|example.com", time.Now().Add(-time.Hour)); n != 1 {
+		t.Fatalf("in-flight reservations = %d, want 1", n)
+	}
+}
+
 // Once the message is recorded the stored count sees it, so its reservation
 // must be given back — otherwise it would be counted twice and the ceiling
 // would drift closed.
@@ -477,12 +544,55 @@ func TestRefusalDoesNotLeaveDomainReservation(t *testing.T) {
 	}
 }
 
+// The ceiling is handed out exactly max times however the sessions interleave.
+// Counting and reserving in two critical sections passes the sequential tests
+// above and still overshoots here, because between one session's count and its
+// reservation any number of others can pass the same check.
+func TestTryAdmitHandsOutEachSlotOnce(t *testing.T) {
+	const (
+		max     = 500
+		workers = 8
+	)
+	fl := &inflight{}
+	since := time.Now().Add(-time.Hour)
+	start := make(chan struct{})
+	admitted := make([]int, workers)
+	var wg sync.WaitGroup
+	for i := range workers {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			<-start
+			for {
+				_, _, ok := fl.tryAdmit("domain|example.com", since, 0, max)
+				if !ok {
+					return
+				}
+				admitted[i]++
+			}
+		}()
+	}
+	close(start)
+	wg.Wait()
+
+	total := 0
+	for _, n := range admitted {
+		total += n
+	}
+	if total != max {
+		t.Fatalf("admitted %d messages under a ceiling of %d", total, max)
+	}
+}
+
 // The in-flight count only covers the limit's own window: a reservation older
 // than it (a session stuck mid-DATA for longer than the window) must not be
 // counted against a window it no longer belongs to.
 func TestInflightIgnoresReservationsOutsideWindow(t *testing.T) {
 	fl := &inflight{}
-	r := fl.reserve("domain|example.com")
+	r, _, ok := fl.tryAdmit("domain|example.com", time.Now().Add(-time.Hour), 0, 1)
+	if !ok {
+		t.Fatal("tryAdmit refused the first message under a ceiling of 1")
+	}
 	r.at = time.Now().Add(-time.Minute)
 
 	if n := fl.count("domain|example.com", time.Now().Add(-time.Hour)); n != 1 {

internal/milter/ratelimit.go

@@ -54,22 +54,23 @@ func (s *session) overLimit() bool {
 }
 
 // enforceLimit counts recent messages for scope/ref and refuses when at or
-// above the ceiling. On admit it reserves an in-flight slot on the session.
+// above the ceiling. The stored count and the in-flight slots are weighed and
+// the admitted message's own slot is taken in one atomic step (tryAdmit), so
+// two sessions racing at MAIL FROM cannot both claim the last free slot.
 func (s *session) enforceLimit(scope, ref string, rl store.RateLimit) bool {
 	since := time.Now().Add(-time.Duration(rl.WindowSeconds) * time.Second)
-	n, err := s.rec.CountMessages(scope, ref, since)
+	stored, err := s.rec.CountMessages(scope, ref, since)
 	if err != nil {
 		log.Printf("journal-milter: rate-limit count %s %q: %v (fail-open)", scope, ref, err)
 		return false
 	}
-	key := scope + "|" + ref
-	n += s.flight.count(key, since)
-	if n >= int64(rl.MaxMessages) {
+	r, n, ok := s.flight.tryAdmit(scope+"|"+ref, since, stored, int64(rl.MaxMessages))
+	if !ok {
 		log.Printf("journal-milter: %s %q over limit: %d/%d in %ds from %s — refusing 4xx",
 			scope, ref, n, rl.MaxMessages, rl.WindowSeconds, s.clientIP)
 		return true
 	}
-	s.reserved = append(s.reserved, s.flight.reserve(key))
+	s.reserved = append(s.reserved, r)
 	return false
 }
 

internal/web/auth/auth_test.go

@@ -1,13 +1,17 @@
 package auth
 
 import (
+	"database/sql"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"path/filepath"
 	"strings"
 	"testing"
 	"time"
 
+	"golang.org/x/crypto/bcrypt"
+
 	"github.com/mixeme/selfpost/internal/store"
 	"github.com/mixeme/selfpost/internal/web/view"
 )
@@ -22,6 +26,15 @@ func newTestSessionStore(t *testing.T) *sessionStore {
 	return newSessionStore(st, 7*24*time.Hour)
 }
 
+func mustCreate(t *testing.T, s *sessionStore, username string) string {
+	t.Helper()
+	token, err := s.Create(username)
+	if err != nil {
+		t.Fatalf("create session: %v", err)
+	}
+	return token
+}
+
 func mustView(t *testing.T) *view.Engine {
 	t.Helper()
 	v, err := view.New("test")
@@ -86,7 +99,7 @@ func TestSessionTokenIgnoresTheOtherName(t *testing.T) {
 
 func TestRequireAuthRejectsDuplicateCookies(t *testing.T) {
 	m := testModule(t, false)
-	token := m.sessions.Create("admin")
+	token := mustCreate(t, m.sessions, "admin")
 
 	reached := false
 	h := m.RequireAuth(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true }))
@@ -107,7 +120,7 @@ func TestRequireAuthRejectsDuplicateCookies(t *testing.T) {
 
 func TestLogoutClearsBothCookieNames(t *testing.T) {
 	m := testModule(t, true)
-	token := m.sessions.Create("admin")
+	token := mustCreate(t, m.sessions, "admin")
 
 	r := httptest.NewRequest(http.MethodPost, "http://panel.example.com/logout", nil)
 	r.Host = "panel.example.com"
@@ -132,9 +145,63 @@ func TestLogoutClearsBothCookieNames(t *testing.T) {
 	}
 }
 
+// A session that could not be stored must not turn into a cookie: the browser
+// would look signed in, and every request it made would be bounced to /login
+// with no explanation. Only the sessions table is broken here, so the request
+// gets past the user lookup and password check and fails exactly where the
+// session is written.
+func TestLoginSetsNoCookieWhenTheSessionCannotBeStored(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "test.db")
+	st, err := store.Open(path)
+	if err != nil {
+		t.Fatalf("open store: %v", err)
+	}
+	t.Cleanup(func() { st.Close() })
+	hash, err := bcrypt.GenerateFromPassword([]byte("correct-horse-battery"), bcrypt.MinCost)
+	if err != nil {
+		t.Fatalf("hash password: %v", err)
+	}
+	if err := st.CreateGlobalUser("admin", string(hash)); err != nil {
+		t.Fatalf("create user: %v", err)
+	}
+	dropSessionsTable(t, path)
+
+	m := New(st, Config{}, mustView(t), "")
+	form := url.Values{"username": {"admin"}, "password": {"correct-horse-battery"}}
+	r := httptest.NewRequest(http.MethodPost, "http://panel.example.com/login",
+		strings.NewReader(form.Encode()))
+	r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	rec := httptest.NewRecorder()
+	m.HandleLogin(rec, r)
+
+	if got := rec.Header().Values("Set-Cookie"); len(got) != 0 {
+		t.Errorf("a session cookie was issued for a session that was never stored: %v", got)
+	}
+	if rec.Code != http.StatusInternalServerError {
+		t.Errorf("status = %d, want 500 (the login failed)", rec.Code)
+	}
+	if loc := rec.Header().Get("Location"); loc != "" {
+		t.Errorf("the browser was sent to %q as if it were signed in", loc)
+	}
+}
+
+// dropSessionsTable breaks session persistence while leaving the rest of the
+// schema usable. The SQLite driver is registered by internal/store.
+func dropSessionsTable(t *testing.T, path string) {
+	t.Helper()
+	db, err := sql.Open("sqlite", "file:"+path+"?_pragma=busy_timeout(5000)")
+	if err != nil {
+		t.Fatalf("open database directly: %v", err)
+	}
+	defer db.Close()
+	if _, err := db.Exec("DROP TABLE sessions"); err != nil {
+		t.Fatalf("drop sessions table: %v", err)
+	}
+}
+
 func TestSessionRename(t *testing.T) {
 	s := newTestSessionStore(t)
-	token := s.Create("admin")
+	token := mustCreate(t, s, "admin")
 
 	s.Rename(token, "operator")
 
@@ -149,8 +216,8 @@ func TestSessionRename(t *testing.T) {
 
 func TestSessionDestroyOthers(t *testing.T) {
 	s := newTestSessionStore(t)
-	keep := s.Create("admin")
-	other := s.Create("admin")
+	keep := mustCreate(t, s, "admin")
+	other := mustCreate(t, s, "admin")
 
 	s.DestroyOthers(keep)
 
@@ -165,7 +232,7 @@ func TestSessionDestroyOthers(t *testing.T) {
 func TestSessionLookupRejectsExpired(t *testing.T) {
 	s := newTestSessionStore(t)
 	s.idle = -time.Minute
-	token := s.Create("admin")
+	token := mustCreate(t, s, "admin")
 
 	if _, ok := s.Lookup(token); ok {
 		t.Fatal("expired session was accepted")
@@ -174,7 +241,7 @@ func TestSessionLookupRejectsExpired(t *testing.T) {
 
 func TestSessionTouchThrottled(t *testing.T) {
 	s := newTestSessionStore(t)
-	token := s.Create("admin")
+	token := mustCreate(t, s, "admin")
 
 	if s.Touch(token) {
 		t.Fatal("touch renewed a session created moments ago")

internal/web/auth/handlers.go

@@ -120,7 +120,12 @@ func (m *Module) submitLogin(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	token := m.sessions.Create(user.Username)
+	token, err := m.sessions.Create(user.Username)
+	if err != nil {
+		logf("panel: login: create session failed: %v", err)
+		m.renderLogin(w, http.StatusInternalServerError, "Internal error. Please try again.")
+		return
+	}
 	m.setSessionCookie(w, token)
 	http.Redirect(w, r, "/", http.StatusSeeOther)
 }

internal/web/auth/session.go

@@ -41,17 +41,22 @@ func hashToken(token string) string {
 	return hex.EncodeToString(sum[:])
 }
 
-// Create issues a new session for username and returns its token.
-func (s *sessionStore) Create(username string) string {
+// Create issues a new session for username and returns its token. It fails
+// closed: if the row cannot be written the caller gets an error and must not
+// hand out a cookie, because a token that is not in the database looks like a
+// signed-in browser while every request it makes bounces back to /login.
+func (s *sessionStore) Create(username string) (string, error) {
 	token := randomToken(32)
 	now := time.Now()
 	if err := s.store.CreateSession(hashToken(token), username, now.Add(s.idle)); err != nil {
-		logf("panel: session: create failed: %v", err)
+		return "", err
 	}
+	// Pruning is housekeeping: the new session is already valid, so a failure
+	// here is logged and does not fail the login.
 	if _, err := s.store.DeleteExpiredSessions(now); err != nil {
 		logf("panel: session: prune expired failed: %v", err)
 	}
-	return token
+	return token, nil
 }
 
 // Lookup returns the session username for a token if it exists and is