diff --git a/CHANGELOG.md b/CHANGELOG.md index e1660b9..939fd7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,14 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version with nothing assigned. Saving and creating a user are now one transaction each, the e-mail included (audit 2026-10-12, M9). +- **A domain import is checked like the forms.** The import trusted the file + for the report address, the rate limits and the client addresses: a limit + with a window of no seconds counted nothing, and an address that was not + one made the application refuse every client without a word. The file is + now held to the forms' rules before anything is created, and refused with + the form's own message, naming the field and the application (audit + 2026-10-12, M10). + ### Added - **The delivery log measures its own guesses** (migration `0007`). A table diff --git a/docs/audit-2026-10-12.md b/docs/audit-2026-10-12.md index 167f165..c0bd8ac 100644 --- a/docs/audit-2026-10-12.md +++ b/docs/audit-2026-10-12.md @@ -669,7 +669,7 @@ has already run is never edited — the next file is added; the published - [x] **`submitUserUpdate` in one transaction** (store method that takes the whole change). The users pages get direct handler tests while they are rewired - a row of review-2026-08-followups, placed here 2026-10-14. — **Opus** -- [ ] **Import validates like the form**: route `DMARCRua`, IPs, limit mode, +- [x] **Import validates like the form**: route `DMARCRua`, IPs, limit mode, window and count through the same parsers; refuse the file with the validator's text. Tests per field. — **Opus** - [ ] **Send-log rows belong to a domain by id** (L12; owner, 2026-10-12). diff --git a/docs/roadmap.md b/docs/roadmap.md index c6901ce..351d8a0 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -42,7 +42,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) | | panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) | -| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`; next stages 1+2 → `2.5.0`) | 17/62 | [audit-2026-10-12.md](audit-2026-10-12.md) | +| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`; next stages 1+2 → `2.5.0`) | 18/62 | [audit-2026-10-12.md](audit-2026-10-12.md) | **Recommended order** (not binding; owner, 2026-10-09: feature order follows development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and diff --git a/internal/web/handlers/handlers_backup.go b/internal/web/handlers/handlers_backup.go index 783606d..1d180a6 100644 --- a/internal/web/handlers/handlers_backup.go +++ b/internal/web/handlers/handlers_backup.go @@ -277,6 +277,11 @@ func (h *Handlers) HandleImportDomain(w http.ResponseWriter, r *http.Request) { return } + if msg := h.checkDomainImport(&exp); msg != "" { + h.renderBackupPage(w, r, http.StatusBadRequest, msg) + return + } + d, err := h.domains.Import(exp) if err != nil { logf("panel: import domain %q: %v", exp.Domain, err) @@ -365,3 +370,67 @@ func importErrorMessage(err error) (int, string) { return http.StatusBadRequest, "Could not import the domain: " + err.Error() } } + +// checkDomainImport holds an export file to the rules the forms hold the same +// settings to - the report address, the rate limits, the client addresses - +// before anything is created, and says what is wrong in the validator's own +// words; "" when nothing is. The file is untrusted input: a window of no +// seconds made a limit that counted nothing, and an address that is not one +// made an application refuse every client without a word at import (audit +// 2026-10-12, M10). Client addresses are written back as the form would store +// them. The login, the addresses an application may send as and the password +// are checked where the application is created (app.ImportApplication). +func (h *Handlers) checkDomainImport(exp *domain.DomainExport) string { + invalid := func(what string, err error) string { + return "Invalid " + what + " in export file: " + err.Error() + } + if exp.DMARCRua != "" { + if err := validate.Email(exp.DMARCRua); err != nil { + return invalid("report address", err) + } + } + limit := func(rl *domain.RateLimitExport) error { + if rl == nil { + return nil + } + mode, err := parseRateLimitMode(rl.Mode) + if err != nil { + return err + } + if mode == store.RateLimitModeAuto { + if rl.AutoMultiplier == 0 { + return nil // as an empty field on the form: the default + } + return checkAutoMultiplier(rl.AutoMultiplier) + } + if rl.MaxMessages <= 0 && rl.WindowSeconds <= 0 { + return nil // no limit of its own (an export of client addresses only) + } + return checkManualLimit(rl.MaxMessages, rl.WindowSeconds, h.l1Messages()) + } + if err := limit(exp.RateLimit); err != nil { + return invalid("rate limit for the domain", err) + } + for i := range exp.Applications { + a := &exp.Applications[i] + if err := limit(a.RateLimit); err != nil { + return invalid("rate limit for "+a.Login, err) + } + ips, err := parseIPList(strings.Join(a.AuthAllowedIPs, ",")) + if err != nil { + return invalid("client addresses for "+a.Login, err) + } + a.AuthAllowedIPs = ips + if a.RateLimit != nil && len(a.RateLimit.AllowedIPs) > 0 { + legacy, err := parseIPList(strings.Join(a.RateLimit.AllowedIPs, ",")) + if err != nil { + return invalid("client addresses for "+a.Login, err) + } + a.RateLimit.AllowedIPs = legacy + } + if a.AuthIPRestrict && len(a.AuthAllowedIPs) == 0 && (a.RateLimit == nil || len(a.RateLimit.AllowedIPs) == 0) { + return invalid("client addresses for "+a.Login, errors.New("enter at least one client IP when the allow-list is enabled")) + } + } + return "" +} diff --git a/internal/web/handlers/handlers_import_test.go b/internal/web/handlers/handlers_import_test.go new file mode 100644 index 0000000..8789e54 --- /dev/null +++ b/internal/web/handlers/handlers_import_test.go @@ -0,0 +1,102 @@ +package handlers + +import ( + "bytes" + "encoding/json" + "mime/multipart" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/mixeme/selfpost/internal/domain" + "github.com/mixeme/selfpost/internal/store" + "github.com/mixeme/selfpost/internal/web/auth" +) + +// A domain export is held to the rules the forms hold the same settings to, +// field by field, and refused in the validator's words (audit 2026-10-12, +// M10). +func TestDomainImportIsCheckedLikeTheForms(t *testing.T) { + h, _ := inboundHandlers(t) + good := func() domain.DomainExport { + return domain.DomainExport{ + Domain: "example.com", + DMARCRua: "reports@example.com", + RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeManual, MaxMessages: 10, WindowSeconds: 3600}, + Applications: []domain.AppExport{{ + Login: "app1", AddressMode: store.AddressModeWildcard, Password: "imported-password", + AuthIPRestrict: true, AuthAllowedIPs: []string{"192.0.2.1", " 2001:DB8::1 "}, + RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeAuto, AutoMultiplier: 2}, + }}, + } + } + exp := good() + if msg := h.checkDomainImport(&exp); msg != "" { + t.Fatalf("a good export refused: %s", msg) + } + if got := strings.Join(exp.Applications[0].AuthAllowedIPs, " "); got != "192.0.2.1 2001:db8::1" { + t.Errorf("client addresses stored as %q, want as the form stores them", got) + } + + for _, c := range []struct { + name string + break_ func(*domain.DomainExport) + want string + }{ + {"report address", func(e *domain.DomainExport) { e.DMARCRua = "not an address" }, + "Invalid report address in export file: "}, + {"no window", func(e *domain.DomainExport) { e.RateLimit.WindowSeconds = 0 }, + "Invalid rate limit for the domain in export file: enter a time window greater than zero seconds"}, + {"no messages", func(e *domain.DomainExport) { e.RateLimit.MaxMessages = 0 }, + "Invalid rate limit for the domain in export file: enter a message limit greater than zero"}, + {"over the backstop", func(e *domain.DomainExport) { e.RateLimit.MaxMessages = 1 << 20 }, + "Invalid rate limit for the domain in export file: message limit cannot exceed the level-1 backstop"}, + {"mode", func(e *domain.DomainExport) { e.RateLimit.Mode = "sometimes" }, + "Invalid rate limit for the domain in export file: choose manual or auto mode"}, + {"multiplier", func(e *domain.DomainExport) { e.Applications[0].RateLimit.AutoMultiplier = 1000 }, + "Invalid rate limit for app1 in export file: multiplier must be between"}, + {"client address", func(e *domain.DomainExport) { e.Applications[0].AuthAllowedIPs = []string{"not-an-ip"} }, + `Invalid client addresses for app1 in export file: "not-an-ip" is not a valid IP address`}, + {"allow-list on, empty", func(e *domain.DomainExport) { e.Applications[0].AuthAllowedIPs = nil }, + "Invalid client addresses for app1 in export file: enter at least one client IP when the allow-list is enabled"}, + {"legacy client address", func(e *domain.DomainExport) { + e.Applications[0].AuthAllowedIPs = nil + e.Applications[0].RateLimit = &domain.RateLimitExport{AllowedIPs: []string{"300.1.1.1"}} + }, `Invalid client addresses for app1 in export file: "300.1.1.1" is not a valid IP address`}, + } { + t.Run(c.name, func(t *testing.T) { + exp := good() + c.break_(&exp) + if msg := h.checkDomainImport(&exp); !strings.HasPrefix(msg, c.want) { + t.Fatalf("got %q, want it to start with %q", msg, c.want) + } + }) + } +} + +// The check runs before anything is created: a refused file leaves no domain. +func TestDomainImportRefusedCreatesNothing(t *testing.T) { + h, st := inboundHandlers(t) + root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + raw, _ := json.Marshal(domain.DomainExport{ + Format: domain.FormatDomainExport, Domain: "example.com", DKIMSelector: "mail", + RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeManual, MaxMessages: 10, WindowSeconds: 0}, + }) + var body bytes.Buffer + mw := multipart.NewWriter(&body) + fw, _ := mw.CreateFormFile("file", "example.com.json") + fw.Write(raw) + mw.Close() + req := httptest.NewRequest("POST", "/server/backup/import", &body) + req.Header.Set("Content-Type", mw.FormDataContentType()) + req = auth.RequestWithPrincipal(req, root) + rec := httptest.NewRecorder() + h.HandleImportDomain(rec, req) + if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "enter a time window greater than zero seconds") { + t.Fatalf("import of a limit with no window = %d:\n%s", rec.Code, rec.Body.String()) + } + if domains, _ := st.ListDomains(); len(domains) != 0 { + t.Fatalf("the refused import created %v", domains) + } +} diff --git a/internal/web/handlers/handlers_ratelimit.go b/internal/web/handlers/handlers_ratelimit.go index cedf3c4..7b3e897 100644 --- a/internal/web/handlers/handlers_ratelimit.go +++ b/internal/web/handlers/handlers_ratelimit.go @@ -45,10 +45,31 @@ func parseAutoMultiplier(raw string) (float64, error) { if err != nil { return 0, fmt.Errorf("enter a valid multiplier (%.1f–%.1f)", store.MinAutoMultiplier, store.MaxAutoMultiplier) } + return v, checkAutoMultiplier(v) +} + +// checkAutoMultiplier is the rule for an auto limit's multiplier, shared by +// the form and a domain import. +func checkAutoMultiplier(v float64) error { if v < store.MinAutoMultiplier || v > store.MaxAutoMultiplier { - return 0, fmt.Errorf("multiplier must be between %.1f and %.1f", store.MinAutoMultiplier, store.MaxAutoMultiplier) + return fmt.Errorf("multiplier must be between %.1f and %.1f", store.MinAutoMultiplier, store.MaxAutoMultiplier) } - return v, nil + return nil +} + +// checkManualLimit is the rule for a manual limit, shared by the form and a +// domain import: a window of no seconds would count nothing. +func checkManualLimit(maxMessages, windowSeconds, l1Max int) error { + if maxMessages <= 0 { + return fmt.Errorf("enter a message limit greater than zero") + } + if maxMessages > l1Max { + return fmt.Errorf("message limit cannot exceed the level-1 backstop (%d)", l1Max) + } + if windowSeconds <= 0 { + return fmt.Errorf("enter a time window greater than zero seconds") + } + return nil } func parseRateLimitMode(raw string) (string, error) { @@ -86,15 +107,15 @@ func parseRateLimitForm(r *http.Request, l1Max int) (rateLimitInput, error) { return rateLimitInput{clear: true}, nil } maxMessages, err := parsePositiveInt(rawMax, 0) - if err != nil || maxMessages <= 0 { - return rateLimitInput{}, fmt.Errorf("enter a message limit greater than zero") - } - if maxMessages > l1Max { - return rateLimitInput{}, fmt.Errorf("message limit cannot exceed the level-1 backstop (%d)", l1Max) + if err != nil { + maxMessages = 0 } windowSeconds, err := parsePositiveInt(r.PostFormValue("window_seconds"), defaultRateLimitWindowSeconds) - if err != nil || windowSeconds <= 0 { - return rateLimitInput{}, fmt.Errorf("enter a time window greater than zero seconds") + if err != nil { + windowSeconds = 0 + } + if err := checkManualLimit(maxMessages, windowSeconds, l1Max); err != nil { + return rateLimitInput{}, err } return rateLimitInput{mode: store.RateLimitModeManual, maxMessages: maxMessages, windowSeconds: windowSeconds}, nil }