diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f8d27e..495f58a 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/docs/plans/code-review.md b/docs/plans/code-review.md index 0ad7a07..b9a7685 100644 --- a/docs/plans/code-review.md +++ b/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 diff --git a/internal/store/sendlog.go b/internal/store/sendlog.go index 6bb7e33..6c0a3f9 100644 --- a/internal/store/sendlog.go +++ b/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 + 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) diff --git a/internal/store/sendlog_test.go b/internal/store/sendlog_test.go index 0512ab2..05e4456 100644 --- a/internal/store/sendlog_test.go +++ b/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) diff --git a/internal/web/handlers/handlers_monitor.go b/internal/web/handlers/handlers_monitor.go index 57a0b4c..45aa2e3 100644 --- a/internal/web/handlers/handlers_monitor.go +++ b/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,8 +343,37 @@ func (h *Handlers) sendLogData(r *http.Request) (map[string]any, error) { } sort.Strings(logins) - if !p.IsGlobal() && filter.AppLogin != "" && !loginSet[filter.AppLogin] { - 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 diff --git a/internal/web/handlers/handlers_monitor_test.go b/internal/web/handlers/handlers_monitor_test.go index 2efeae5..748bbbe 100644 --- a/internal/web/handlers/handlers_monitor_test.go +++ b/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) }