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