From b81ff9f135c3e6c91ba02d9ec0455f58c2638c1b Mon Sep 17 00:00:00 2001 From: Mix Date: Sat, 17 Oct 2026 21:52:06 +0300 Subject: [PATCH] domains: a domain's DMARC reports are deleted with it (audit A5/W2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security audit 2026-10-17, finding A5/W2 (low), stage 1; the mockup is 7effc98. dmarc_reports belong to a domain by name, outlive the domain (no FK), and a domain deleted and added again handed its old reports to whoever it was assigned to then - the logs were moved to ids in 2.5.0 for exactly this case, reports were not. The cheap form of the plan: store.DeleteDomain deletes the domain's reports (their records by the table's cascade) in the same transaction as the domain. The confirmation page says so when the domain has reports ("delete the 12 DMARC reports received for it;"), counted by the new store.CountDMARCReports; NewOutDomainDelete takes the count. Tests: TestDeletingADomainDeletesItsReports (the domain added again has none, another domain keeps its report and record, a second delete is ErrDomainNotFound); TestOutDomainDeleteNamesWhatGoesWithIt with 12, 1 and 0 reports. Docs: security.md § Roles; CHANGELOG; plan. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 ++ docs/security-audit-2026-10-17.md | 5 +-- docs/security.md | 2 +- internal/store/dmarc.go | 10 ++++++ internal/store/dmarc_test.go | 39 +++++++++++++++++++++++ internal/store/domains.go | 19 ++++++++++- internal/web/handlers/handlers_domains.go | 8 ++++- internal/web/view/fixtures_test.go | 2 +- internal/web/view/outbound.go | 11 +++++-- internal/web/view/outbound_test.go | 12 ++++--- 10 files changed, 98 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c74a6ad..1920354 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version reports into, under any domain, or another domain's hosted one. Only the hosted address of a domain that chose it is a mailbox now, and a report must arrive at the address tagged with its domain. +- **A domain's DMARC reports are deleted with it** (audit A5/W2): reports + belong to a domain by name, and a domain deleted and added again showed + its old reports to whoever it was assigned to then. - **A stranger's recipient no longer writes an Inbound › Log row** (audit M2): Postfix quotes a refused recipient in its reply with the quotes removed, so `RCPT TO:<"; from= to= …"@domain>` spelled a second diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index 51f22e8..19f5764 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -745,9 +745,10 @@ the argument. 2026-10-17:* the live registry holds two hosted addresses and no custom one on the hostname; TestCustomReportAddressOnTheHostnameIsRefused, the untagged cases in TestIngestMessageRejectsTagMismatch. -- [ ] **A5/W2.** Delete a domain's reports in `domain.Service.Delete` (the +- [x] **A5/W2.** Delete a domain's reports in `domain.Service.Delete` (the cheap form); security.md § Roles says reports go with the domain. — - **Sonnet**, 0.5 h. (The `domain_id` form is better and 2 h; take it if + **Sonnet**, 0.5 h. *Done 2026-10-17:* in `store.DeleteDomain`, one + transaction with the domain; TestDeletingADomainDeletesItsReports. (The `domain_id` form is better and 2 h; take it if stage 1 is split across two sittings.) - [ ] **W3.** A mutex around `Resync`'s read + write + reload. — **Sonnet**, 0.5 h. diff --git a/docs/security.md b/docs/security.md index 3c4f01b..15e8215 100644 --- a/docs/security.md +++ b/docs/security.md @@ -193,7 +193,7 @@ assigned outbound domains and inbound domains **separately**, each list with | Overview, Server (Health, System log, Backup, Users, Settings, kit page) | yes | no — 404 | | Outbound queue | yes | no — 404 (a queue is not filtered by domain) | | Outbound: domain page, applications, Domain settings (report address, rate limit, export), log | every domain | assigned outbound domains | -| Outbound › DMARC reports | every report, with the server's ingest statistics | reports of assigned domains, without the ingest box | +| Outbound › DMARC reports | every report, with the server's ingest statistics | reports of assigned domains, without the ingest box (a domain's reports are deleted with it, so a domain added again under the same name starts with none) | | Inbound: domain page, upstream, recipients | every inbound domain | assigned inbound domains | | Inbound › Log | every row, including those of a domain deleted since | rows of assigned inbound domains (by id: not those of an earlier domain of the same name), without the links into the filter lists | | Inbound › Filter lists; the inbound log's retention | yes | no — 404 | diff --git a/internal/store/dmarc.go b/internal/store/dmarc.go index ea5df87..192e119 100644 --- a/internal/store/dmarc.go +++ b/internal/store/dmarc.go @@ -161,6 +161,16 @@ func (s *Store) PruneDMARCReports(domain string) error { return nil } +// CountDMARCReports is how many reports a domain has: what deleting it +// deletes with it (DeleteDomain). +func (s *Store) CountDMARCReports(domain string) (int, error) { + var n int + if err := s.db.QueryRow(`SELECT COUNT(*) FROM dmarc_reports WHERE domain = ?`, domain).Scan(&n); err != nil { + return 0, fmt.Errorf("count dmarc reports of %s: %w", domain, err) + } + return n, nil +} + // IncrDMARCParseFailures bumps the failure counter in settings. func (s *Store) IncrDMARCParseFailures() error { raw, err := s.GetSetting(DMARCParseFailuresKey) diff --git a/internal/store/dmarc_test.go b/internal/store/dmarc_test.go index ce2654a..36c7c51 100644 --- a/internal/store/dmarc_test.go +++ b/internal/store/dmarc_test.go @@ -106,3 +106,42 @@ func TestListDMARCReportsEmptyListIsNoRows(t *testing.T) { t.Errorf("all domains with a list = %d report(s), %v; want both (AllDomains wins)", len(list), err) } } + +// A domain's DMARC reports go when the domain does, records included, and +// another domain's stay: a domain added again under the old name starts with +// none (security audit 2026-10-17, A5/W2). +func TestDeletingADomainDeletesItsReports(t *testing.T) { + st := openTestStore(t) + gone, _ := st.AddDomain("gone.example", "sel") + if _, err := st.AddDomain("kept.example", "sel"); err != nil { + t.Fatal(err) + } + now := time.Now().UTC().Truncate(time.Second) + for _, d := range []string{"gone.example", "kept.example"} { + if _, err := st.InsertDMARCReport(DMARCReport{ + Domain: d, Reporter: "google.com", ReportID: "r-" + d, PeriodBegin: now.Add(-24 * time.Hour), PeriodEnd: now, ReceivedAt: now, + Records: []DMARCReportRecord{{SourceIP: "203.0.113.1", Count: 1, SPFResult: "pass", DKIMResult: "pass"}}, + }); err != nil { + t.Fatal(err) + } + } + if err := st.DeleteDomain(gone.ID); err != nil { + t.Fatal(err) + } + if _, err := st.AddDomain("gone.example", "sel"); err != nil { + t.Fatal(err) + } + if list, err := st.ListDMARCReports(DMARCReportScope{Domains: []string{"gone.example"}}, 10); err != nil || len(list) != 0 { + t.Errorf("the domain added again has %d old reports (%v)", len(list), err) + } + if list, err := st.ListDMARCReports(DMARCReportScope{Domains: []string{"kept.example"}}, 10); err != nil || len(list) != 1 { + t.Errorf("the other domain has %d reports (%v), want 1", len(list), err) + } + var records int + if err := st.db.QueryRow("SELECT COUNT(*) FROM dmarc_report_records").Scan(&records); err != nil || records != 1 { + t.Errorf("%d report records left (%v), want the kept domain's one", records, err) + } + if err := st.DeleteDomain(gone.ID); err != ErrDomainNotFound { + t.Errorf("deleting it again = %v, want ErrDomainNotFound", err) + } +} diff --git a/internal/store/domains.go b/internal/store/domains.go index aa32268..f99b910 100644 --- a/internal/store/domains.go +++ b/internal/store/domains.go @@ -132,8 +132,22 @@ func (s *Store) GetDomain(id int64) (Domain, error) { // foreign_keys ON, which Open's DSN sets on every one // (https://sqlite.org/foreignkeys.html § 2, § 4.3); the rows the cascade // removes are not in RowsAffected (https://sqlite.org/c3ref/changes.html). +// +// The domain's DMARC reports go with it, in the same transaction (their +// records by the table's cascade): reports belong to a domain by name, and a +// domain deleted and added again - by anyone, for anyone - must not hand its +// old reports to whoever it is assigned to then (security audit 2026-10-17, +// A5/W2). The logs keep their rows, by id, for the global role. func (s *Store) DeleteDomain(id int64) error { - res, err := s.db.Exec("DELETE FROM domains WHERE id = ?", id) + tx, err := s.db.Begin() + if err != nil { + return fmt.Errorf("delete domain: begin: %w", err) + } + defer tx.Rollback() //nolint:errcheck // a no-op after Commit + if _, err := tx.Exec(`DELETE FROM dmarc_reports WHERE domain = (SELECT name FROM domains WHERE id = ?)`, id); err != nil { + return fmt.Errorf("delete domain: its DMARC reports: %w", err) + } + res, err := tx.Exec("DELETE FROM domains WHERE id = ?", id) if err != nil { return fmt.Errorf("delete domain: %w", err) } @@ -144,6 +158,9 @@ func (s *Store) DeleteDomain(id int64) error { if n == 0 { return ErrDomainNotFound } + if err := tx.Commit(); err != nil { + return fmt.Errorf("delete domain: commit: %w", err) + } return nil } diff --git a/internal/web/handlers/handlers_domains.go b/internal/web/handlers/handlers_domains.go index 1c583a3..fefcf4e 100644 --- a/internal/web/handlers/handlers_domains.go +++ b/internal/web/handlers/handlers_domains.go @@ -136,7 +136,13 @@ func (h *Handlers) HandleDeleteConfirm(w http.ResponseWriter, r *http.Request) { for i, a := range apps { logins[i] = a.Login } - h.view.Render(w, http.StatusOK, "out-domain-delete", view.NewOutDomainDelete(h.shellMeta(r), d.ID, d.Name, logins)) + reports, err := h.store.CountDMARCReports(d.Name) + if err != nil { + logf("panel: delete domain %d: %v", d.ID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + h.view.Render(w, http.StatusOK, "out-domain-delete", view.NewOutDomainDelete(h.shellMeta(r), d.ID, d.Name, logins, reports)) } // HandleDeleteDomain performs the deletion and returns to the domain list. diff --git a/internal/web/view/fixtures_test.go b/internal/web/view/fixtures_test.go index 6cd8184..2e52732 100644 --- a/internal/web/view/fixtures_test.go +++ b/internal/web/view/fixtures_test.go @@ -260,7 +260,7 @@ func outAppCreatedFixture() *OutAppCreated { // outDomainDeleteFixture is the mockup's confirmation: the domain has the two // applications that go with it. func outDomainDeleteFixture() *OutDomainDelete { - return NewOutDomainDelete(admin(), 1, "example.org", []string{"prod-server", "alerts"}) + return NewOutDomainDelete(admin(), 1, "example.org", []string{"prod-server", "alerts"}, 12) } // outLogFixture is the mockup's Outbound log: six messages of three domains on diff --git a/internal/web/view/outbound.go b/internal/web/view/outbound.go index ce6c822..1808e15 100644 --- a/internal/web/view/outbound.go +++ b/internal/web/view/outbound.go @@ -768,8 +768,9 @@ type OutDomainDelete struct { } // NewOutDomainDelete builds the page. logins are the applications the domain -// has: they are named, because they go with it. -func NewOutDomainDelete(m Meta, id int64, name string, logins []string) *OutDomainDelete { +// has: they are named, because they go with it; reports is how many DMARC +// reports go with it (security audit 2026-10-17, A5/W2), not said when none. +func NewOutDomainDelete(m Meta, id int64, name string, logins []string, reports int) *OutDomainDelete { m.Title, m.Section, m.Page = "Delete "+name, "outbound", "domains" href := DomainHref(id) p := &OutDomainDelete{ @@ -797,6 +798,12 @@ func NewOutDomainDelete(m Meta, id int64, name string, logins []string) *OutDoma item = append(item, Inline{Text: ") with their credentials and sender bindings;"}) p.Consequences = append(p.Consequences, item) } + switch { + case reports == 1: + p.Consequences = append(p.Consequences, Rich("delete the ", Strong("DMARC report"), " received for it;")) + case reports > 1: + p.Consequences = append(p.Consequences, Rich("delete the ", Strong(strconv.Itoa(reports)+" DMARC reports"), " received for it;")) + } p.Consequences = append(p.Consequences, Rich("stop signing mail as the domain.")) return p } diff --git a/internal/web/view/outbound_test.go b/internal/web/view/outbound_test.go index b64ea46..60aa51d 100644 --- a/internal/web/view/outbound_test.go +++ b/internal/web/view/outbound_test.go @@ -435,18 +435,20 @@ func TestOutDomainDeleteNamesWhatGoesWithIt(t *testing.T) { `

Delete example.org

`, `
`, `

This cannot be undone

`, `Deleting example.org will:`, `permanently delete its DKIM signing key;`, `delete both applications (prod-server, alerts) with their credentials and sender bindings;`, + `delete the 12 DMARC reports received for it;`, `stop signing mail as the domain.`, `Inbound domains are not touched.`, `
`, `class="button is-danger"`, `Delete example.org`, `href="/outbound/domains/1">Keep it`, `

Moving it instead?

`, `Export the domain`, ) - // No applications: nothing to say about them. - out = renderSignedIn(t, "out-domain-delete", NewOutDomainDelete(admin(), 1, "example.org", nil)) - pageLacks(t, "Delete", out, `applications`, `credentials`) + // No applications and no reports: nothing to say about them. + out = renderSignedIn(t, "out-domain-delete", NewOutDomainDelete(admin(), 1, "example.org", nil, 0)) + pageLacks(t, "Delete", out, `applications`, `credentials`, `received for it`) pageHas(t, "Delete", out, `permanently delete its DKIM signing key;`, `stop signing mail as the domain.`) - out = renderSignedIn(t, "out-domain-delete", NewOutDomainDelete(admin(), 1, "example.org", []string{"only"})) - pageHas(t, "Delete", out, `its application (only)`) + out = renderSignedIn(t, "out-domain-delete", NewOutDomainDelete(admin(), 1, "example.org", []string{"only"}, 1)) + pageHas(t, "Delete", out, `its application (only)`, + `delete the DMARC report received for it;`) } // ---- Helpers