panel: scope the send log to a domain admin's own domains

The Deliveries list narrowed the journal only when exactly one domain was assigned, so an administrator with none or with two or more read every domain's rows. The domain scope is now an IN constraint the store query carries, a filter that states no scope matches nothing, and the domain/app query parameters are validated against the principal before the query runs.

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

CHANGELOG.md

@@ -5,6 +5,18 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
 
 ## [Unreleased]
 
+### Fixed
+
+- security (panel): the Deliveries list is scoped to a domain administrator's
+  assigned domains for every number of assignments. Previously the send log was
+  narrowed only when exactly one domain was assigned, so an administrator with
+  none or with two or more read every domain's rows (sender, recipient, subject)
+  on the list and its polled fragment. The domain scope is now an `IN`
+  constraint carried by the store query — a filter that states no scope returns
+  nothing — and the `domain` and `app` query parameters are checked against the
+  principal's own domains and applications before the query runs, so a
+  hand-written URL cannot widen the scope. Global administrators are unaffected.
+
 ### Changed
 
 - docs: full-tree review plan ([docs/plans/code-review.md](docs/plans/code-review.md))

docs/plans/code-review.md

@@ -485,14 +485,16 @@ change. After Go changes: `go build`, `go vet`, `go test ./...`.
 **Model: Opus.** Tests in the same change. **Fable** on the diff after it
 lands (reviewer ≠ author).
 
-- [ ] Extend `SendLogFilter` so a non-empty domain list is an `IN` constraint.
-      Empty list for a non-global user → zero rows, not “all”.
-- [ ] `sendLogData`: for `!p.IsGlobal()`, always constrain to assigned domain
+- [x] Extend `SendLogFilter` so a non-empty domain list is an `IN` constraint.
+      Empty list for a non-global user → zero rows, not “all”. Done as
+      `Domains` + `AllDomains`: the zero value matches nothing, so a caller
+      that states no scope cannot read the journal.
+- [x] `sendLogData`: for `!p.IsGlobal()`, always constrain to assigned domain
       names; validate `AppLogin` against the user’s apps **before** query.
-- [ ] Tests: domain-admin with 0, 1, and 2 assigned domains; unfiltered list;
+- [x] Tests: domain-admin with 0, 1, and 2 assigned domains; unfiltered list;
       forged `?domain=` and `?app=`; detail page still 404s on a foreign id
       (already true — keep a regression test).
-- [ ] Comment the invariant next to `sendLogData` (the comment that was
+- [x] Comment the invariant next to `sendLogData` (the comment that was
       missing).
 
 **Done when:** a domain-admin cannot read another domain’s send-log rows via

internal/store/sendlog.go

@@ -178,15 +178,29 @@ func (s *Store) GetSendLog(id int64) (SendLogRow, error) {
 	return row, nil
 }
 
-// SendLogFilter narrows QuerySendLog/CountSendLog by domain and/or
-// application login. An empty field matches everything.
+// SendLogFilter narrows QuerySendLog/CountSendLog. It carries two kinds of
+// narrowing, and they behave in opposite ways on purpose.
+//
+// Domain and AppLogin are the operator's own filters, chosen in the UI: an
+// empty field matches everything.
+//
+// Domains and AllDomains are the authorization scope, which no query parameter
+// may widen. Domains is the exhaustive set of domain names the caller is
+// entitled to read, applied as an IN constraint; AllDomains lifts that
+// restriction and is the only way to read the whole journal. A zero-valued
+// filter therefore matches *no* rows: a caller that forgets to state a scope
+// gets an empty log rather than every tenant's mail, which is the failure mode
+// this struct exists to make impossible.
 type SendLogFilter struct {
 	Domain     string
 	AppLogin   string
+	Domains    []string
+	AllDomains bool
 }
 
 // QuerySendLog returns send-log rows matching filter, newest first, for the
 // monitoring screen's server-side pagination (product.md's send-log view).
+// The filter's authorization scope is mandatory: see SendLogFilter.
 func (s *Store) QuerySendLog(filter SendLogFilter, limit, offset int) ([]SendLogRow, error) {
 	where, args := sendLogWhere(filter)
 	args = append(args, limit, offset)
@@ -233,6 +247,20 @@ func (s *Store) CountSendLog(filter SendLogFilter) (int64, error) {
 func sendLogWhere(f SendLogFilter) (string, []any) {
 	var clauses []string
 	var args []any
+	if !f.AllDomains {
+		// No scope is not "no restriction": a domain administrator whose last
+		// assignment was deleted owns nothing in the journal and must see
+		// nothing, and the same clause catches a caller that never set a scope.
+		if len(f.Domains) == 0 {
+			return " WHERE 1 = 0", nil
+		}
+		marks := make([]string, len(f.Domains))
+		for i, name := range f.Domains {
+			marks[i] = "?"
+			args = append(args, name)
+		}
+		clauses = append(clauses, "domain IN ("+strings.Join(marks, ", ")+")")
+	}
 	if f.Domain != "" {
 		clauses = append(clauses, "domain = ?")
 		args = append(args, f.Domain)

internal/store/sendlog_test.go

@@ -1,6 +1,8 @@
 package store
 
 import (
+	"sort"
+	"strings"
 	"testing"
 	"time"
 )
@@ -162,6 +164,61 @@ func TestUpdateStatusNoMatch(t *testing.T) {
 	}
 }
 
+// The journal is read by principals who are only entitled to part of it, so
+// the scope is part of the query rather than something the caller remembers to
+// apply afterwards. A filter that states no scope is a caller that has not
+// decided who is asking, and the safe answer to that is nothing.
+func TestSendLogScopeIsMandatory(t *testing.T) {
+	st := openTestStore(t)
+	for _, domain := range []string{"first.example.ru", "second.example.ru"} {
+		if err := st.InsertQueued(SendLogEntry{
+			QueueID: "Q-" + domain, Domain: domain, AppLogin: "app-" + domain,
+			From: "noreply@" + domain, To: "public@example.net", Subject: domain,
+		}); err != nil {
+			t.Fatalf("InsertQueued: %v", err)
+		}
+	}
+
+	for name, tc := range map[string]struct {
+		filter SendLogFilter
+		want   []string
+	}{
+		"no scope":           {SendLogFilter{}, nil},
+		"empty scope":        {SendLogFilter{Domains: []string{}}, nil},
+		"all domains":        {SendLogFilter{AllDomains: true}, []string{"first.example.ru", "second.example.ru"}},
+		"one domain":         {SendLogFilter{Domains: []string{"first.example.ru"}}, []string{"first.example.ru"}},
+		"two domains":        {SendLogFilter{Domains: []string{"first.example.ru", "second.example.ru"}}, []string{"first.example.ru", "second.example.ru"}},
+		"unknown domain":     {SendLogFilter{Domains: []string{"third.example.ru"}}, nil},
+		"filter within":      {SendLogFilter{Domain: "first.example.ru", Domains: []string{"first.example.ru", "second.example.ru"}}, []string{"first.example.ru"}},
+		"filter outside":     {SendLogFilter{Domain: "second.example.ru", Domains: []string{"first.example.ru"}}, nil},
+		"app filter outside": {SendLogFilter{AppLogin: "app-second.example.ru", Domains: []string{"first.example.ru"}}, nil},
+	} {
+		rows, err := st.QuerySendLog(tc.filter, 50, 0)
+		if err != nil {
+			t.Fatalf("%s: QuerySendLog: %v", name, err)
+		}
+		var got []string
+		for _, r := range rows {
+			got = append(got, r.Domain)
+		}
+		// Which rows came back is the question here; the page's own order is
+		// newest-first and is tested where it matters.
+		sort.Strings(got)
+		if strings.Join(got, ",") != strings.Join(tc.want, ",") {
+			t.Errorf("%s: rows for %v, want %v", name, got, tc.want)
+		}
+		// The count drives pagination, so it has to agree with the page or the
+		// UI advertises pages of rows the reader is not allowed to see.
+		n, err := st.CountSendLog(tc.filter)
+		if err != nil {
+			t.Fatalf("%s: CountSendLog: %v", name, err)
+		}
+		if int(n) != len(tc.want) {
+			t.Errorf("%s: count %d, want %d", name, n, len(tc.want))
+		}
+	}
+}
+
 func TestDeleteSendLogBefore(t *testing.T) {
 	st := openTestStore(t)
 

internal/web/handlers/handlers_monitor.go

@@ -299,16 +299,22 @@ func deliveriesBackURL(r *http.Request) string {
 // sendLogData reads the domain/app filters and page number off the query
 // string, queries the store, and assembles everything the template needs
 // (filter dropdown options plus the current selection, rows, and pagination).
+//
+// The invariant this function owes the journal: a principal who is not global
+// only ever reads rows for the domains assigned to them. That scope is stated
+// to the store as SendLogFilter.Domains and holds for every number of
+// assignments, including none — a domain administrator whose last domain was
+// deleted gets an empty log, not the whole one. The query parameters are
+// filters *within* that scope and can only narrow it: both are checked against
+// the assigned domains and their applications before the query runs, because a
+// dropdown that offers only permitted values is a courtesy to the browser, not
+// a check on the request.
 func (h *Handlers) sendLogData(r *http.Request) (map[string]any, error) {
 	p, ok := h.principal(r)
 	if !ok {
 		return nil, errors.New("no principal")
 	}
 	q := r.URL.Query()
-	filter := store.SendLogFilter{
-		Domain:   q.Get("domain"),
-		AppLogin: q.Get("app"),
-	}
 
 	assigned, err := h.assignedDomains(p)
 	if err != nil {
@@ -316,31 +322,6 @@ func (h *Handlers) sendLogData(r *http.Request) (map[string]any, error) {
 	}
 	allowedNames := domainNameSet(assigned)
 
-	if !p.IsGlobal() {
-		if filter.Domain != "" && !allowedNames[filter.Domain] {
-			filter.Domain = ""
-		}
-		if filter.Domain == "" && len(assigned) == 1 {
-			filter.Domain = assigned[0].Name
-		}
-	}
-
-	page := parsePage(q.Get("p"))
-
-	total, err := h.store.CountSendLog(filter)
-	if err != nil {
-		return nil, err
-	}
-	rows, err := h.store.QuerySendLog(filter, sendLogPageSize, (page-1)*sendLogPageSize)
-	if err != nil {
-		return nil, err
-	}
-	view := make([]sendLogRow, len(rows))
-	for i := range rows {
-		rows[i].Subject = mailhdr.DecodeSubject(rows[i].Subject)
-		view[i] = sendLogRow{SendLogRow: rows[i], Level: deliveryLevel(rows[i].Status)}
-	}
-
 	domainNames := make([]string, 0, len(assigned))
 	for _, d := range assigned {
 		domainNames = append(domainNames, d.Name)
@@ -362,9 +343,38 @@ func (h *Handlers) sendLogData(r *http.Request) (map[string]any, error) {
 	}
 	sort.Strings(logins)
 
-	if !p.IsGlobal() && filter.AppLogin != "" && !loginSet[filter.AppLogin] {
+	filter := store.SendLogFilter{
+		Domain:   q.Get("domain"),
+		AppLogin: q.Get("app"),
+		// A global administrator reads the whole journal, including rows left
+		// behind by a domain that has since been deleted.
+		Domains:    domainNames,
+		AllDomains: p.IsGlobal(),
+	}
+	if !p.IsGlobal() {
+		if filter.Domain != "" && !allowedNames[filter.Domain] {
+			filter.Domain = ""
+		}
+		if filter.AppLogin != "" && !loginSet[filter.AppLogin] {
 			filter.AppLogin = ""
 		}
+	}
+
+	page := parsePage(q.Get("p"))
+
+	total, err := h.store.CountSendLog(filter)
+	if err != nil {
+		return nil, err
+	}
+	rows, err := h.store.QuerySendLog(filter, sendLogPageSize, (page-1)*sendLogPageSize)
+	if err != nil {
+		return nil, err
+	}
+	view := make([]sendLogRow, len(rows))
+	for i := range rows {
+		rows[i].Subject = mailhdr.DecodeSubject(rows[i].Subject)
+		view[i] = sendLogRow{SendLogRow: rows[i], Level: deliveryLevel(rows[i].Status)}
+	}
 
 	lastPage := 1
 	if total > 0 {

internal/web/handlers/handlers_monitor_test.go

@@ -129,7 +129,7 @@ func TestDeliveryPageMarksAQueuedMessageAsStillWaiting(t *testing.T) {
 	}); err != nil {
 		t.Fatalf("insert: %v", err)
 	}
-	rows, err := h.store.QuerySendLog(store.SendLogFilter{}, 1, 0)
+	rows, err := h.store.QuerySendLog(store.SendLogFilter{AllDomains: true}, 1, 0)
 	if err != nil || len(rows) != 1 {
 		t.Fatalf("query: %v (%d rows)", err, len(rows))
 	}
@@ -215,6 +215,143 @@ func TestDeliveryPageNotFound(t *testing.T) {
 	}
 }
 
+// A domain administrator reads the journal of the domains assigned to them and
+// nothing else. The list used to be scoped only when exactly one domain was
+// assigned, which meant two assignments read as none at all.
+func TestSendLogScopedToAssignedDomains(t *testing.T) {
+	h, domains := serverWithTwoDomains(t)
+
+	for name, tc := range map[string]struct {
+		principal auth.Principal
+		want      []string
+		unwanted  []string
+	}{
+		"global sees both": {
+			globalPrincipal, []string{"First message", "Second message"}, nil,
+		},
+		"one assigned domain": {
+			domainAdmin(domains["first.example.ru"].ID),
+			[]string{"First message"}, []string{"Second message", "second-app"},
+		},
+		"two assigned domains": {
+			domainAdmin(domains["first.example.ru"].ID, domains["second.example.ru"].ID),
+			[]string{"First message", "Second message"}, nil,
+		},
+		// Every assigned domain deleted cascades the assignments away. That
+		// leaves a principal entitled to nothing, which is an empty log — the
+		// case that used to hand over the whole journal.
+		"no assigned domains": {
+			domainAdmin(), []string{"No messages logged yet."},
+			[]string{"First message", "Second message"},
+		},
+	} {
+		for view, handler := range map[string]http.HandlerFunc{
+			"page":     h.HandleDeliveries,
+			"fragment": h.HandleDeliveriesRows,
+		} {
+			out := getBodyAs(t, handler, "/deliveries", tc.principal)
+			for _, want := range tc.want {
+				if !strings.Contains(out, want) {
+					t.Errorf("%s (%s): missing %q:\n%s", name, view, want, out)
+				}
+			}
+			for _, unwanted := range tc.unwanted {
+				if strings.Contains(out, unwanted) {
+					t.Errorf("%s (%s): leaks %q:\n%s", name, view, unwanted, out)
+				}
+			}
+		}
+	}
+}
+
+// The filter dropdowns offer only permitted values, so a leak through them can
+// only come from a hand-written URL — which is exactly why the values are
+// checked against the principal's own domains and applications rather than
+// trusted for having been rendered by us.
+func TestSendLogIgnoresForgedFilters(t *testing.T) {
+	h, domains := serverWithTwoDomains(t)
+	p := domainAdmin(domains["first.example.ru"].ID)
+
+	for _, target := range []string{
+		"/deliveries?domain=second.example.ru",
+		"/deliveries?app=second-app",
+		"/deliveries?domain=second.example.ru&app=second-app",
+	} {
+		out := getBodyAs(t, h.HandleDeliveries, target, p)
+		if strings.Contains(out, "Second message") {
+			t.Errorf("GET %s leaks another domain's journal:\n%s", target, out)
+		}
+		if !strings.Contains(out, "First message") {
+			t.Errorf("GET %s hid the principal's own journal:\n%s", target, out)
+		}
+	}
+}
+
+// The detail page has always checked membership; keep it checked, because the
+// list and the page are two ways to the same row.
+func TestDeliveryPageForeignDomainNotFound(t *testing.T) {
+	h, domains := serverWithTwoDomains(t)
+	rows, err := h.store.QuerySendLog(store.SendLogFilter{Domain: "second.example.ru", AllDomains: true}, 1, 0)
+	if err != nil || len(rows) != 1 {
+		t.Fatalf("query: %v (%d rows)", err, len(rows))
+	}
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/deliveries/"+itoa(rows[0].ID), nil)
+	req.SetPathValue("id", itoa(rows[0].ID))
+	req = auth.RequestWithPrincipal(req, domainAdmin(domains["first.example.ru"].ID))
+	h.HandleDelivery(rec, req)
+	if rec.Code != http.StatusNotFound {
+		t.Errorf("delivery page for a foreign domain = %d, want 404", rec.Code)
+	}
+}
+
+// serverWithTwoDomains builds a panel over a store holding two domains, one
+// application and one delivered message each, so a scoping test can tell "my
+// rows" from "every row" by reading the page.
+func serverWithTwoDomains(t *testing.T) (*Handlers, map[string]store.Domain) {
+	t.Helper()
+	st, err := store.Open(filepath.Join(t.TempDir(), "test.db"))
+	if err != nil {
+		t.Fatalf("open store: %v", err)
+	}
+	t.Cleanup(func() { _ = st.Close() })
+
+	domains := make(map[string]store.Domain, 2)
+	for _, d := range []struct{ name, app, subject string }{
+		{"first.example.ru", "first-app", "First message"},
+		{"second.example.ru", "second-app", "Second message"},
+	} {
+		dom, err := st.AddDomain(d.name, "mail")
+		if err != nil {
+			t.Fatalf("add domain %s: %v", d.name, err)
+		}
+		if _, err := st.AddApplication(dom.ID, d.app, store.AddressModeWildcard, nil); err != nil {
+			t.Fatalf("add application %s: %v", d.app, err)
+		}
+		if err := st.InsertQueued(store.SendLogEntry{
+			QueueID: "Q" + d.app, Domain: d.name, AppLogin: d.app,
+			From: "noreply@" + d.name, To: "public@example.net", Subject: d.subject,
+		}); err != nil {
+			t.Fatalf("insert %s: %v", d.subject, err)
+		}
+		domains[d.name] = dom
+	}
+
+	return &Handlers{store: st, view: mustView(t), cfg: Config{Version: "test"}}, domains
+}
+
+var globalPrincipal = auth.Principal{ID: 1, Username: "admin", Role: auth.RoleGlobal}
+
+func domainAdmin(domainIDs ...int64) auth.Principal {
+	return auth.Principal{
+		ID:       2,
+		Username: "domain-admin",
+		Role:     auth.RoleDomainAdmin,
+		Domains:  domainIDs,
+	}
+}
+
 // serverWithDelivery builds a panel over a store holding one delivery, written
 // the way the journal-milter wrote them before it decoded subjects itself.
 func serverWithDelivery(t *testing.T) (*Handlers, store.SendLogRow) {
@@ -238,7 +375,7 @@ func serverWithDelivery(t *testing.T) (*Handlers, store.SendLogRow) {
 	if _, err := st.UpdateStatus("4A1B2C3D", "public@example.ru", store.StatusSent); err != nil {
 		t.Fatalf("update status: %v", err)
 	}
-	rows, err := st.QuerySendLog(store.SendLogFilter{}, 1, 0)
+	rows, err := st.QuerySendLog(store.SendLogFilter{AllDomains: true}, 1, 0)
 	if err != nil || len(rows) != 1 {
 		t.Fatalf("query: %v (%d rows)", err, len(rows))
 	}
@@ -246,18 +383,20 @@ func serverWithDelivery(t *testing.T) (*Handlers, store.SendLogRow) {
 	return &Handlers{store: st, view: mustView(t), cfg: Config{Version: "test"}}, rows[0]
 }
 
-// getBody runs one handler over a GET and returns the page it wrote, failing
-// the test on any non-200. The path's {id} is bound by hand because these calls
-// bypass the router that would otherwise fill it in.
+// getBody runs one handler over a GET as the global administrator.
 func getBody(t *testing.T, h http.HandlerFunc, target string) string {
+	t.Helper()
+	return getBodyAs(t, h, target, globalPrincipal)
+}
+
+// getBodyAs runs one handler over a GET as the given principal and returns the
+// page it wrote, failing the test on any non-200. The path's {id} is bound by
+// hand because these calls bypass the router that would otherwise fill it in.
+func getBodyAs(t *testing.T, h http.HandlerFunc, target string, p auth.Principal) string {
 	t.Helper()
 	rec := httptest.NewRecorder()
 	req := httptest.NewRequest(http.MethodGet, target, nil)
-	req = auth.RequestWithPrincipal(req, auth.Principal{
-		ID:       1,
-		Username: "admin",
-		Role:     auth.RoleGlobal,
-	})
+	req = auth.RequestWithPrincipal(req, p)
 	if rest, ok := strings.CutPrefix(req.URL.Path, "/deliveries/"); ok && rest != "rows" {
 		req.SetPathValue("id", rest)
 	}