Compare commits

...

3 Commits

Author SHA1 Message Date
mix d9ab7bfa45 docs: retire the completed full-tree review plan
test / test (push) Has been cancelled
Every phase (P0-P7) is closed, so the plan file goes per its own exit criteria; history lives in git and the CHANGELOG. The roadmap's recommended order returns to queue-retries and then inbound-relay, and the order notes in both plans stop pointing at the deleted file, as does the 1.2.5 changelog entry (same treatment as logrotate-mode.md).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-14 15:23:52 +03:00
mix 18bfd148ec docs: close P7 — security review of the P0–P1 diff, no findings
The Fable review of the send-log authorization, tryAdmit, session create and app-delete ordering against security.md found no further issues: every send-log query path states its scope and an empty scope matches nothing, rejected rows cannot consume rate-limit budget, the milter's nil-registry fail-open is test-only, and each failure residue of the reordered delete fails safe. Outcome recorded in security.md's header and the plan; nothing added to the accepted risks.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-14 15:02:07 +03:00
mix e70ba9046e fix(panel): resync mail-path maps once after restore
When CheckRestore accepts a backup manifest, the panel re-derives OpenDKIM tables and the Postfix sender map from SQLite on that first boot and reloads both daemons, so archive/database drift is healed before mail flows.

Co-authored-by: Cursor <cursoragent@cursor.com>
2026-08-14 14:58:08 +03:00
13 changed files with 224 additions and 737 deletions
+26 -3
View File
@@ -19,6 +19,16 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
Explanation only; no attempt counter and no panel knobs. Not yet Explanation only; no attempt counter and no panel knobs. Not yet
implemented. implemented.
### Security
- The independent security review of the send-log authorization and
fail-closed fixes below (code-review plan § P7; reviewer model ≠ author
model) found no further issues: the domain scope holds on every query path,
a rate-limit refusal cannot consume window budget, and each failure residue
of the reordered application delete fails safe. Nothing was added to
[docs/security.md](docs/security.md) § Accepted risks; the review is
recorded in that file's header.
### Fixed ### Fixed
- test (e2e): send-log status scrapers follow the badge markup in - test (e2e): send-log status scrapers follow the badge markup in
@@ -66,6 +76,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
### Changed ### Changed
- docs: the 2026-08-13 full-tree review plan is complete — every phase (P0P7)
is closed — and `docs/plans/code-review.md` is deleted per its own exit
criteria (history in git and in this file). The
[roadmap](docs/roadmap.md)'s recommended order returns to **queue-retries**
and then **inbound-relay**.
- licence: [NOTICE](NOTICE) tells modifiers to update `SourceURL` in - licence: [NOTICE](NOTICE) tells modifiers to update `SourceURL` in
`internal/legal/legal.go` (the value the panel footer actually injects), not `internal/legal/legal.go` (the value the panel footer actually injects), not
`layout.html`. `release.yml` refuses a `workflow_dispatch` whose version is `layout.html`. `release.yml` refuses a `workflow_dispatch` whose version is
@@ -128,7 +144,8 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
prompts existed — and why that is acceptable (the prompt is a mis-click prompts existed — and why that is acceptable (the prompt is a mis-click
guard, not an authorization boundary). guard, not an authorization boundary).
- docs: full-tree review plan ([docs/plans/code-review.md](docs/plans/code-review.md)) - docs: full-tree review plan (`docs/plans/code-review.md`, since completed
and deleted — history in git)
— architecture, quality, GUI, tests, licence; P0 is domain-admin send-log — architecture, quality, GUI, tests, licence; P0 is domain-admin send-log
authorization. Roadmap queues that plan ahead of inbound-relay and records authorization. Roadmap queues that plan ahead of inbound-relay and records
`schema-squash` (replace the 1.x SQLite migration chain with a 2.x baseline; `schema-squash` (replace the 1.x SQLite migration chain with a 2.x baseline;
@@ -162,8 +179,14 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
expired buckets on a timer and cap the map at 4096 keys; the five expired buckets on a timer and cap the map at 4096 keys; the five
show/hide field helpers in `panel.js` are one rule table; DMARC copy no show/hide field helpers in `panel.js` are one rule table; DMARC copy no
longer promises in-panel report reception in a future release — SelfPost longer promises in-panel report reception in a future release — SelfPost
does not receive inbound mail. No optional post-restore map `Resync` (the does not receive inbound mail.
backup package comment fix in P2 is enough).
- panel (restore): after a backup is extracted and the version guard passes,
the panel runs one mail-path Resync on the first boot — OpenDKIM's tables
and Postfix's sender map are re-derived from SQLite and the daemons are
reloaded, so drift between the archive and the database is healed before
mail flows. Later starts skip that step; the Status page Reload button runs
the same Resync on demand.
## [1.2.5] - 2026-08-13 ## [1.2.5] - 2026-08-13
+48 -7
View File
@@ -3,6 +3,7 @@ package main
import ( import (
"context" "context"
"errors" "errors"
"fmt"
"log" "log"
"net/http" "net/http"
"time" "time"
@@ -15,17 +16,57 @@ import (
"github.com/mixeme/selfpost/internal/web" "github.com/mixeme/selfpost/internal/web"
) )
// mailStack is the panel's domain and application services plus the on-disk
// mail-path adapters they write through.
type mailStack struct {
Domains *domain.Service
Apps *app.Service
pf *postfix.Postfix
odk *domain.OpenDKIM
}
func newMailStack(cfg config, st *store.Store) *mailStack {
pf := postfix.New(cfg.postfixDir)
odk := domain.NewOpenDKIM(cfg.opendkimDir)
apps := app.NewService(st, app.NewSASLDB(cfg.saslDBPath, cfg.saslRealm), pf)
domains := domain.NewService(st, odk, apps, cfg.dkimSelectorDef)
return &mailStack{Domains: domains, Apps: apps, pf: pf, odk: odk}
}
// Resync rebuilds OpenDKIM's tables and Postfix's sender map from SQLite and
// reloads both daemons — the same work as the Status page's Reload button.
func (m *mailStack) Resync() error {
if err := m.Domains.Resync(); err != nil {
return fmt.Errorf("opendkim resync: %w", err)
}
if err := m.Apps.Resync(); err != nil {
return fmt.Errorf("postfix resync: %w", err)
}
return nil
}
func (m *mailStack) skipReloadForTest() {
m.pf.SetReloadHook(func() error { return nil })
m.odk.SetReloadHook(func() error { return nil })
}
// resyncAfterRestore runs one mail-path Resync on the first boot after a
// backup restore. testNoReload skips the supervisord reload step so restore
// tests can verify file regeneration without a running mail stack.
func resyncAfterRestore(cfg config, st *store.Store, testNoReload bool) error {
ms := newMailStack(cfg, st)
if testNoReload {
ms.skipReloadForTest()
}
return ms.Resync()
}
// newPanel wires the panel's services over the shared database handle and // newPanel wires the panel's services over the shared database handle and
// builds the HTTP application from cfg. It is the composition of the panel as // builds the HTTP application from cfg. It is the composition of the panel as
// the environment describes it, with nothing bound to a port yet. // the environment describes it, with nothing bound to a port yet.
func newPanel(cfg config, st *store.Store) (*web.Server, error) { func newPanel(cfg config, st *store.Store) (*web.Server, error) {
// Applications own the SASL accounts and the Postfix sender map; the domain ms := newMailStack(cfg, st)
// service delegates to them when a domain (and its applications) is deleted. return web.New(st, ms.Domains, ms.Apps, web.Config{
pf := postfix.New(cfg.postfixDir)
apps := app.NewService(st, app.NewSASLDB(cfg.saslDBPath, cfg.saslRealm), pf)
domains := domain.NewService(st, domain.NewOpenDKIM(cfg.opendkimDir), apps, cfg.dkimSelectorDef)
return web.New(st, domains, apps, web.Config{
Hostname: cfg.hostname, Hostname: cfg.hostname,
CookieSecure: cfg.cookieSecure, CookieSecure: cfg.cookieSecure,
SubmissionEnabled: cfg.submissionEnabled, SubmissionEnabled: cfg.submissionEnabled,
+9 -1
View File
@@ -222,7 +222,8 @@ func run() error {
// touch the database, so schema/format skew between versions cannot corrupt // touch the database, so schema/format skew between versions cannot corrupt
// the restored state. A match consumes the manifest; its absence is the // the restored state. A match consumes the manifest; its absence is the
// normal (non-restore) case. // normal (non-restore) case.
if err := backup.CheckRestore(cfg.manifestPath, buildinfo.Version); err != nil { restored, err := backup.CheckRestore(cfg.manifestPath, buildinfo.Version)
if err != nil {
return err return err
} }
@@ -235,6 +236,13 @@ func run() error {
} }
defer st.Close() defer st.Close()
if restored {
log.Printf("restore manifest accepted; regenerating mail-path maps from SQLite")
if err := resyncAfterRestore(cfg, st, false); err != nil {
return err
}
}
var wg sync.WaitGroup var wg sync.WaitGroup
errc := make(chan error, 3) errc := make(chan error, 3)
+69 -9
View File
@@ -4,6 +4,7 @@ import (
"archive/tar" "archive/tar"
"bytes" "bytes"
"compress/gzip" "compress/gzip"
"encoding/json"
"io" "io"
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
@@ -74,13 +75,12 @@ func TestPanelBootsOnADataDirectoryRestoredFromItsOwnBackup(t *testing.T) {
} }
} }
// The daemons read their own state from the archive rather than from // The archive carries the daemon files, and the first boot after restore
// SQLite, so the files have to land where the panel's configuration says // re-derives the maps from SQLite so they stay aligned with the database.
// they are — that is the whole reason restore needs no regeneration step.
for path, want := range map[string]string{ for path, want := range map[string]string{
filepath.Join("opendkim", "keys", restoreDomain, "selfpost.private"): "PRIVATE KEY", filepath.Join("opendkim", "keys", restoreDomain, "selfpost.private"): "PRIVATE KEY",
filepath.Join("sasl", "sasldb2"): "SASLDB", filepath.Join("sasl", "sasldb2"): "SASLDB",
filepath.Join("postfix", "sender_login_maps"): "@" + restoreDomain + " shop", filepath.Join("postfix", "sender_login_maps"): "@" + restoreDomain + " shop\n",
} { } {
got, err := os.ReadFile(filepath.Join(r.dataDir, path)) got, err := os.ReadFile(filepath.Join(r.dataDir, path))
if err != nil { if err != nil {
@@ -135,6 +135,57 @@ func TestAnEncryptedBackupRestoresTheSameWay(t *testing.T) {
} }
} }
// A restore boot runs one Resync from SQLite. If the archive's Postfix map
// drifted from the database, that step puts it back before mail flows.
func TestResyncAfterRestoreHealsDriftedMaps(t *testing.T) {
dataDir := seedPanelData(t)
cfg := panelConfig(t, dataDir)
mapPath := filepath.Join(dataDir, "postfix", "sender_login_maps")
if err := os.WriteFile(mapPath, []byte("stale map\n"), 0o640); err != nil {
t.Fatalf("write stale map: %v", err)
}
manifest, err := json.Marshal(backup.Manifest{
Format: backup.FormatFull,
Version: buildinfo.Version,
CreatedAt: "2026-08-14T00:00:00Z",
})
if err != nil {
t.Fatalf("marshal manifest: %v", err)
}
if err := os.WriteFile(cfg.manifestPath, manifest, 0o644); err != nil {
t.Fatalf("write manifest: %v", err)
}
restored, err := backup.CheckRestore(cfg.manifestPath, buildinfo.Version)
if err != nil {
t.Fatalf("CheckRestore: %v", err)
}
if !restored {
t.Fatal("CheckRestore did not report a restore")
}
st, err := store.Open(cfg.dbPath)
if err != nil {
t.Fatalf("open store: %v", err)
}
defer st.Close()
if err := resyncAfterRestore(cfg, st, true); err != nil {
t.Fatalf("resync after restore: %v", err)
}
got, err := os.ReadFile(mapPath)
if err != nil {
t.Fatalf("read sender map: %v", err)
}
want := "@" + restoreDomain + " shop\n"
if string(got) != want {
t.Errorf("sender map = %q, want %q", got, want)
}
}
// The version guard is what stops a restore from being silently corrupted by // The version guard is what stops a restore from being silently corrupted by
// schema skew, and it runs before anything opens the database. The manifest // schema skew, and it runs before anything opens the database. The manifest
// stays put on a mismatch: the operator's next move is to start the image the // stays put on a mismatch: the operator's next move is to start the image the
@@ -153,7 +204,7 @@ func TestPanelRefusesADataDirectoryRestoredFromAnotherVersion(t *testing.T) {
extract(t, archive.Bytes(), target) extract(t, archive.Bytes(), target)
cfg := panelConfig(t, target) cfg := panelConfig(t, target)
err := backup.CheckRestore(cfg.manifestPath, buildinfo.Version) _, err := backup.CheckRestore(cfg.manifestPath, buildinfo.Version)
if err == nil { if err == nil {
t.Fatal("the panel booted on a data directory left by another version") t.Fatal("the panel booted on a data directory left by another version")
} }
@@ -206,7 +257,7 @@ func seedPanelData(t *testing.T) string {
for path, content := range map[string]string{ for path, content := range map[string]string{
filepath.Join("opendkim", "keys", restoreDomain, "selfpost.private"): "PRIVATE KEY", filepath.Join("opendkim", "keys", restoreDomain, "selfpost.private"): "PRIVATE KEY",
filepath.Join("sasl", "sasldb2"): "SASLDB", filepath.Join("sasl", "sasldb2"): "SASLDB",
filepath.Join("postfix", "sender_login_maps"): "@" + restoreDomain + " shop", filepath.Join("postfix", "sender_login_maps"): "@" + restoreDomain + " shop\n",
filepath.Join("log", "mail.log"): "postfix/smtp[1]: 4A1B2C3D: status=sent", filepath.Join("log", "mail.log"): "postfix/smtp[1]: 4A1B2C3D: status=sent",
} { } {
full := filepath.Join(dataDir, path) full := filepath.Join(dataDir, path)
@@ -241,11 +292,14 @@ func bootPanel(t *testing.T, dataDir string) http.Handler {
t.Helper() t.Helper()
cfg := panelConfig(t, dataDir) cfg := panelConfig(t, dataDir)
if err := backup.CheckRestore(cfg.manifestPath, buildinfo.Version); err != nil { restored, err := backup.CheckRestore(cfg.manifestPath, buildinfo.Version)
if err != nil {
t.Fatalf("the panel refused to start on %s: %v", dataDir, err) t.Fatalf("the panel refused to start on %s: %v", dataDir, err)
} }
if _, err := os.Stat(cfg.manifestPath); err == nil { if restored {
t.Errorf("the restore manifest was not consumed, so the next start is gated by it too") if _, err := os.Stat(cfg.manifestPath); err == nil {
t.Errorf("the restore manifest was not consumed, so the next start is gated by it too")
}
} }
st, err := store.Open(cfg.dbPath) st, err := store.Open(cfg.dbPath)
@@ -254,6 +308,12 @@ func bootPanel(t *testing.T, dataDir string) http.Handler {
} }
t.Cleanup(func() { _ = st.Close() }) t.Cleanup(func() { _ = st.Close() })
if restored {
if err := resyncAfterRestore(cfg, st, true); err != nil {
t.Fatalf("resync after restore: %v", err)
}
}
panel, err := newPanel(cfg, st) panel, err := newPanel(cfg, st)
if err != nil { if err != nil {
t.Fatalf("build the panel: %v", err) t.Fatalf("build the panel: %v", err)
-661
View File
@@ -1,661 +0,0 @@
# Plan: code-review (full-tree review follow-ups)
**Status:** agreed
**Date:** 2026-08-13
**Reviewer:** Cursor Grok 4.6 (whole-tree authorship review). This is **not**
the Fable pre-release security audit in [development.md](../development.md) §
Model routing; a Fable pass is a later step on the P0 diff.
**Version:** patch for defects; docs/UI follow-ups have no schema.
**Order:** **P0 before inbound-relay.** P0 is a shipped RBAC hole, not a
feature. Remaining phases after P0, or interleaved with inbound-relay by
agreement.
---
## Goal
Record the 2026-08-13 full-tree review (architecture, complexity, quality,
docs, maintainability, logic, refactor, licence, legacy, stubs, GUI, disputed
decisions, edge cases, tests, duplication) and a phased implementation
checklist with a recommended model per step, using the routing table in
[development.md](../development.md).
---
## Verdict
SelfPost is a compact, well-bounded 1.x product: one image, one SQLite file,
thin `domain`/`app` services for multi-store writes, a fail-open journal
milter with a Postfix level-1 backstop, and documentation that is unusually
honest about accepted risks. Complexity matches the scale (~10.7k production
Go lines, ~5.8k unit-test lines, ~1.2k HTML, 749 CSS, 281 JS). Comments
explain decisions rather than restating code.
The outstanding defect is **send-log authorization for domain administrators**
(confidentiality). After that, the work is tightening a few fail-open paths,
catching docs that froze at “single administrator”, small GUI bugs from the
1.2.x layout pass, and filling test gaps around auth/RBAC. Do **not** use this
review as a licence to rewrite layers, squash migrations in 1.x, or add CSRF tokens
without revisiting the ADR.
---
## How to read this file
Findings are grouped by the sixteen review questions. Each finding has a
severity (**H**igh / **M**edium / **L**ow / **I**nfo). The implementation
checklist at the end is the work queue; it names the model for each step.
**Models** (from [development.md](../development.md)):
| Kind of work | Model |
|---|---|
| Security, infra, mail path, permissions, open-relay risk | **Opus** |
| UI / JS / CSS, templates, documentation (English) | **Sonnet** |
| Trivial mechanics: retarget links, grep, compose bump, comment fixes | **Haiku** |
| Security **review** of a diff (not authorship) | **Fable** |
Reviewers must not be the author of the code under review.
---
## 1. Architecture / structure
**Proportionate.** Composition root in `cmd/panel` (HTTP + journal-milter +
log-tailer, one `*store.Store`). `internal/domain` and `internal/app` own
multi-store writes and rollback. Adapters (`postfix`, `milter`, `logtail`,
`dnscheck`, `health`, `backup`, `secretfile`) are the only infrastructure-aware
code. Interfaces exist where they break import cycles or enable fakes
(`domain.Applications`, `app.SenderMaps`, `milter.Store`, `logtail.StatusStore`)
— not as a DI framework.
Handlers may call `store` directly for single-table reads (documented in
[architecture.md](../architecture.md) § Code layers). That is followed for
sessions, send-log queries, users, and DMARC settings. It is not a layering
violation; it is an incomplete service boundary that will hurt if those
surfaces keep growing.
No circular Go imports. `MaxOpenConns(1)` on SQLite is an intentional
single-connection trade-off for the three in-process roles.
| Sev | Finding |
|---|---|
| **M** | Users, send-log listing, and global DMARC settings have no service; handlers talk to `store`. Fine at current size; do not invent a service until a second writer appears. |
| **L** | `auth.RequireGlobal` (`internal/web/auth/middleware.go`) is unused; handlers duplicate `requireGlobal`. Either wire the middleware on `/users`, `/backup`, `/status`, `/mail-queue`, `/system-log` or delete the unused helper. |
| **L** | `HandleAccount` / `handlers_account.go` still use the pre-1.2.3 “account” name while the route is `/settings`. |
| **I** | Package comment on `internal/store` still says “the administrator account” after migration `0005` replaced `admin` with `users`. |
**Do not:** introduce a repository layer, split the panel binary, or move
SQLite behind an interface “for testability” — the existing fakes are enough.
---
## 2. Complexity vs project scale
The code is **not over-engineered**. A few files are large because the problem
is large, not because of unused abstraction:
| File | ~Lines | Note |
|---|---|---|
| `internal/health/machine.go` | 622 | Cohesive `/proc` sampler |
| `internal/logtail/logtail.go` | 528 | Follow + rotate + reconcile + retention |
| `internal/web/handlers/handlers_monitor.go` | 481 | Send-log UI + authz (this is where P0 lives) |
| `internal/web/view/templates/domain_detail.html` | 476 | DNS + apps + limits + export; composition debt |
| `internal/secretfile/secretfile.go` | ~423 | Isolated crypto envelope |
| `internal/web/view/static/panel.css` | 749 | Tokens + layout; comment-heavy by design |
Comments are long and mostly load-bearing (threat, fail-open, why not the
obvious alternative). The cost is scanability: some files are 3040% prose.
That matches the projects disclosed AI-authorship style
([development.md](../development.md) § Authorship). Do not strip comments in
the name of “cleanup”. Update the stale ones (see §4).
---
## 3. Code quality
Naming matches the docs (`domain` / `application`, level-1 / level-2). Errors
on the mail path log-and-continue (intentional fail-open). Panel paths log and
return 4xx/5xx. `crypto/rand` failure panics in `auth/token.go` — acceptable.
Context is used for process lifetime and DNS timeouts, not for SQLite (correct
with one connection). Dashboard DNS checks write distinct `rows[i]` from
goroutines; Go 1.22+ loop semantics make that safe (`go.mod` is 1.26).
Magic numbers are mostly named (`reservationTTL`, `renewThreshold`, CSP/HSTS).
Env defaults live in `loadConfig`.
| Sev | Finding |
|---|---|
| **M** | `sessionStore.Create` logs a DB error and still returns the token (`internal/web/auth/session.go`). Login sets the cookie; the next request bounces to `/login`. Fail closed: no cookie, error page. |
| **M** | `app.Service.Delete` removes the registry row **before** SASL / rate-limit cleanup. SASL failure → orphaned `sasldb2` account that can still authenticate. Domain delete does SASL first (`domain/service.go`). Align app delete with that order (or compensate: restore the row on SASL failure). |
| **L** | Login/setup `rateLimiter` sweeps expired buckets only when creating a **new** key. Many unique IPs grow the map until restart. Cap the map or sweep on a timer. |
| **L** | `parseTrustedProxies` skips invalid CIDRs instead of refusing to start. Silent misconfiguration of `TRUSTED_PROXY_CIDR`. |
---
## 4. Documentation completeness vs code; comments
Docs are a first-class artefact (env regression test, architecture as-built,
security accepted-risks). The drift is concentrated where **domain-admin
shipped in 1.2.0** and several files still argue “single-user”.
| Sev | Finding |
|---|---|
| **M** | [guide.md](../guide.md) Operations never mentions **Users** (`/users`) or the domain-admin role. Architecture and product do. An operator reading only the guide does not know the panel is multi-user. |
| **M** | [security.md](../security.md) CSRF ADR still says the panel is single-user and “revisit if multi-user”. Multi-user shipped. The origin-check decision can stand; the **rationale and revisit trigger must be rewritten**. |
| **M** | [security.md](../security.md) says passwords are “bcrypt (or argon2)”. Code is bcrypt only. |
| **M** | `internal/backup` package comment claims the panel **regenerates** Postfix/OpenDKIM maps from SQLite on every start after restore. Startup only runs `CheckRestore` (`cmd/panel/main.go`). Maps/keys are **in** the tarball. Heal path is the Status **Reload** button. |
| **L** | Architecture route table omits `/license` and the `/account``/settings` 308. |
| **L** | Guide Settings section: “change the administrator username and/or password” — global Settings also has the default DMARC `rua=` address. |
| **L** | Guide does not warn that restoring an **older** backup can resurrect sessions (architecture does). |
| **L** | Guide rate-limiting section does not stress that level 2 is **fail-open** (store error or missing client IP → mail continues; level 1 is the backstop). |
| **L** | [roadmap.md](../roadmap.md) and [plans/dmarc-reports.md](dmarc-reports.md) still say `admin.dmarc_report_email` after `0005` moved it to `settings`. |
| **L** | `setupManager` comments still say “admin row”; the fact is `users` / `UserExists()`. “Plan B.1 / C.4” comments are opaque to outsiders; keep them, they are history, not errors. |
| **I** | E2e coverage summary in development.md omits logrotate and supervisor-process checks that actually run. |
| **I** | [plans/logrotate-mode.md](logrotate-mode.md) is **done** but still in `docs/plans/` (active-plans directory). History belongs in git / CHANGELOG. |
Comments in production code are generally **high quality**. Missing comments
are on domain-admin authorization policy in `sendLogData` (the P0 hole has no
comment stating the intended invariant) and on `rateLimiter` memory bounds.
---
## 5. Human readability and maintainability
A new maintainer can follow the tree from [architecture.md](../architecture.md)
into `cmd/panel``internal/web/web.go` → services. Tests document *why*
(milter in-flight, queue-id anchoring, CSRF matrix).
Friction:
- `domain_detail.html` is the hardest HTML file to edit (repeated DNS
host/type/value blocks, checkbox “Edit” panels).
- `panel.css` structure-tied selectors (`.muted + form > select:first-of-type`)
will break on a copy change.
- Dual `CurrentUser` + `Principal` is redundant but works (`withPrincipal`
sets both).
- `assignedDomains` loads **all** domains then filters in Go, while
`store.listUserDomainNames` already exists and is unused by handlers.
None of this blocks maintenance at current size. Prefer small extractions
(DNS partial, `tryAdmit`) over a layer rewrite.
---
## 6. Logical errors
### H — Domain-admin send-log list leaks other domains
Detail page checks membership (`HandleDelivery`). The **list** does not.
`sendLogData` in `internal/web/handlers/handlers_monitor.go`:
- Empty `SendLogFilter.Domain` means “all rows” (`internal/store/sendlog.go`).
- For a non-global user, a disallowed `?domain=` is cleared to `""`. The
assigned domain is filled in **only when there is exactly one**.
- A domain-admin with **0 or ≥2** assigned domains and no (or a forged)
domain filter therefore sees **every** send-log row (From, To, Subject).
- `?app=` is applied to SQL **before** it is checked against the users
application logins. The allowlist only updates the templates selected
filter. Forged `?app=<foreign-login>` with an empty domain filter returns
that applications rows.
The deliveries tables domain dropdown still lists only assigned domains, so
the leak is silent.
**Invariant to implement:** a non-global principals `QuerySendLog` /
`CountSendLog` are always constrained to assigned domain names; if that set is
empty, the result is empty. Validate `AppLogin` against the allowlist
**before** the query.
### M — Level-2 check/reserve race
`enforceLimit` calls `flight.count` then `flight.reserve` under **separate**
mutex acquisitions (`internal/milter/ratelimit.go`, `inflight.go`). Two MAIL
FROM handlers can both observe `n == max-1` and both reserve. In-flight
tracking closes the *stored-count* race (and
`TestRateLimitCountsInFlightMessages` covers the **sequential** case). It does
not close parallel check-then-act. Severity is tempered by fail-open and
Postfix level-1. Fix: one `tryAdmit(key, since, max)` under the inflight
mutex.
### M — Session create fail-open
See §3. Not a stolen-session bug (hash never lands in the DB); it is a
logged-in-looking cookie that cannot be looked up.
### M — App delete ordering
See §3. Orphaned SASL is a mail-path consistency bug.
### L — Domain export `Version` ignored on import
`internal/domain/transfer.go` stamps `buildinfo.Version`; import checks format
only. Lower risk than full-backup `CheckRestore`; still a cross-version footgun.
### I — Journal milter fail-open; origin CSRF fail-open; queue-reconcile
`bounced`
Documented accepted risks in [security.md](../security.md). Not defects.
Revisit the CSRF ADRs *framing* (multi-user), not necessarily the mechanism.
---
## 7. Refactoring and optimisation
Worth doing, in order:
1. `tryAdmit` (correctness, not speed).
2. `SendLogFilter` domain IN-list (correctness).
3. DNS field partial + settings credentials partial (drift).
4. One helper for the five `panel.js` show/hide field pairs.
5. `assignedDomains` via SQL for the current user (clarity, not performance).
Not worth doing now:
- Service layer for users / send-log.
- Replacing SQLite, HTMX, or the single-container model.
- Squashing migrations `0001``0005`.
- CSRF tokens (see §12).
- Rewriting `machine.go` or `logtail.go` for size.
---
## 8. Licence (AGPL-3.0)
Packaging is largely correct: root `LICENSE` ≡ embedded `internal/legal/LICENSE`
(test), unauthenticated `/license`, footer copyright + Source + “No warranty”
on login/setup, image copies `LICENSE`/`NOTICE`, Go deps are BSD-family, htmx
is 0BSD. Network-use §13 is stated in `NOTICE` and the README.
| Sev | Finding |
|---|---|
| **M** | IBM Plex WOFF2 files are shipped without the SIL OFL 1.1 text. OFL requires the licence to travel with the font. Add `OFL.txt` next to the fonts (and mention the path in `NOTICE`). |
| **M** | `NOTICE` tells modifiers to change the Source URL in `layout.html`. The URL is `legal.SourceURL` in `internal/legal/legal.go`, injected by `view.go`. |
| **L** | `/license` serves LICENSE only, not NOTICE. Optional: serve NOTICE at `/notice` or append attributions. |
| **L** | SPDX headers only on `cmd/panel` and `cmd/selfpost-backup`. AGPL does not require per-file SPDX; either add them everywhere or drop the two so the convention is consistent. |
| **I** | Debian package licences are pointed at packages.debian.org rather than a pinned list — normal for an image that installs from bookworm. |
No AGPL-incompatible Go dependency found in `go.mod`.
---
## 9. Legacy code and migrations
| Migration | Role | Removal |
|---|---|---|
| `0001_init.sql` | Core schema (including historical `admin`) | Keep for all **1.x** (`PRAGMA user_version` chain) |
| `0002_sessions.sql` | DB sessions | Keep for 1.x |
| `0003_logtail_state.sql` | Tailer offset | Keep for 1.x |
| `0004_dmarc_report_email.sql` | DMARC columns on `admin` | Keep for 1.x; `0005` moves the data |
| `0005_panel_users.sql` | `users` / `user_domains`; `DROP TABLE admin` | Keep for 1.x |
Squash is deferred to **2.x** — [roadmap.md](../roadmap.md) `schema-squash`.
Until then do not delete, rename, or reorder these files. Document the 1.x
rule in architecture § Persistence (one sentence).
Compat shims to keep until a major:
- `GET/POST /account` → 308 `/settings`.
- Domain rate-limit rows may still have an unused IP list column; enforcement
ignores it.
`sessions.username` is a string, not a `user_id` FK. Renames update the column;
a missed rename would orphan sessions. Acceptable; a FK would be a 1.x
migration if usernames become mutable in more places.
**Delete** `docs/plans/logrotate-mode.md` once this review is the active plan
(status `done`; history is git / CHANGELOG `[1.2.3]`).
---
## 10. Stubs and claimed-but-unimplemented behaviour
| Item | Status |
|---|---|
| Inbound relay | Agreed plan, **no code stubs**, no `INBOUND_RELAY_*` env. Correct. |
| DMARC report **ingestion** | Candidate. UI copy already promises “a future release will be able to receive reports in the panel”. Settings `rua=` and DNS guidance **are** implemented. |
| `panel-docs` | Candidate. Status blurbs were removed in 1.2.2 in favour of this item. |
| `CONTRIBUTING.md` | Candidate, file absent. Matches roadmap. |
| CSRF tokens | Explicitly not implemented (ADR). |
| `auth.RequireGlobal` | Dead helper, not a feature stub. |
The DMARC “future release” sentence is the only user-visible promise of
unimplemented behaviour. Soften it to “SelfPost does not receive inbound mail”
or keep it and treat `dmarc-reports` as the fulfilment — product call, Sonnet
copy.
---
## 11. GUI: hacks and layout composition
The panel is CSP-strict (no inline script/style; `TestNoTemplateUsesInlineScriptOrStyle`).
No `!important`. Progressive enhancement is real (pages work without JS).
Adaptive polling in `panel.js` is a **documented** workaround: HTMX
`hx-trigger="every Ns [expr]"` uses `new Function`, which CSP would break.
| Sev | Finding |
|---|---|
| **M** | `RateLimitErr` uses `class="flash error"`. `.flash` is the **success** surface; `.error` only recolors text. There is no `.flash.error` rule. Validation failures look like success (red text on green). `domain_detail.html` + `panel.css`. |
| **M** | User **Delete** has no `data-confirm` and no confirm page. App delete / regen / rate-limit clear do; domain delete has `domain_delete.html`. One mis-click removes a panel user. |
| **M** | `domain_detail.html` repeats Host/Type/Value/`code-row` for DNS status **and** publishable records. Extract a partial (same pattern as `encrypt_fields.html`). |
| **M** | `settings.html` duplicates the credentials form (global split vs domain-admin narrow card). Drift already visible in the muted help text. |
| **L** | Adaptive polling: `outerHTML` swap every 5 s can steal clicks / focus; poll failures retry silently. Consider `aria-live="polite"` and a visible retry/error. Do not switch back to `hx-trigger="every"` under this CSP. |
| **L** | Checkbox-driven Edit panels instead of `<details>` (commented in the template). Works without JS; no `aria-expanded`. |
| **L** | Five near-identical show/hide helpers in `panel.js`. Encrypt/import fields can flash visible before `DOMContentLoaded`. |
| **L** | `hx-get` query params in `deliveries_rows.html` are not `urlquery`-encoded. Safe while domain/app charset is locked down. |
| **L** | Applications on a domain page are unpaginated. Fine until an operator has dozens of apps. |
| **L** | `<label>` used as a heading on DNS/status readouts (no `for`). |
| **I** | `{{define "wide"}}` override and `main:has(> .card.narrow)` are non-obvious but tested. Keep; do not “simplify” into per-page CSS files. |
`data-confirm` is skipped when JS is off (documented in `panel.js` only).
Domain delete already uses a real page; user delete should follow that
pattern or at least get `data-confirm`.
---
## 12. Weakly documented disputed decisions
These are real choices. Several are in [security.md](../security.md); the
problem is **stale framing** after domain-admin, not silence.
| Decision | Where | Gap |
|---|---|---|
| CSRF via origin / `Sec-Fetch-Site`; no tokens; POST with neither header allowed | security.md ADR | Still argued as “single-user”. Revisit trigger already fired. **Rewrite the ADR**; implementing tokens is a separate product call. |
| Journal-milter fail-open | architecture, milter comments | Guide rate-limit section should say L2 is best-effort. |
| Unencrypted backup/export by default | security.md | OK. Domain-admin can **export working SASL passwords** for assigned domains (`HandleExportDomain` uses `lookupDomain`). Guide/security should say so. |
| Queue reconcile marks lost lines `bounced` | security.md | OK. |
| Sliding session, no absolute cap; HTMX GET does not renew | architecture, guide | OK. |
| Restore can resurrect sessions from an older backup | architecture | Missing from the operator guide. |
| L2 skipped when client IP is unknown | milter + unit test | Not in the guide. |
| Backup encryption optional | security.md | OK. |
| Supervisord socket `0770` so the panel can `postfix reload` | supervisord.conf | Compromised panel ≈ mail-stack control. Documented as intentional; keep. |
| `workflow_dispatch` on `release.yml` derives version from `GITHUB_REF_NAME` | `.github/workflows/release.yml` | A manual run from `main` can publish a non-semver tag. Guard: only `vX.Y.Z` or an explicit version input. |
---
## 13. Edge cases
Covered above: 0 / 1 / ≥2 assigned domains on the send log; forged
`domain`/`app` query params; empty allowlist must not mean “all”.
Others:
- **Last global administrator** cannot be demoted/deleted (UI + server). Good.
- **Domain-admin with no domains** (all assigned domains deleted →
`user_domains` cascade): today they see the full send log (P0). After the
fix they should see an empty log, not an error.
- **Missing `mail.log`** after rotation: treated as empty, not an error
(tested). Good.
- **Backup download after headers committed**: truncated file possible
(streaming trade-off). Encrypted domain export is sealed in memory first.
Acceptable; do not buffer full backups.
- **`parsePage`**: huge `p` yields a large offset and an empty page, not a
500. Fine.
- **Concurrent domain DNS on the dashboard**: safe under Go 1.22+.
- **Import domain** is global-only; **export** is any principal who can
access the domain. Intentional once documented.
---
## 14. Tests
**Strengths.** Milter L2 + in-flight, DNS grading, logtail follow/rotate/
reconcile, secretfile tamper, SASL argv hygiene (`--` before login), template
CSP/nav/legal footer, env-key ↔ guide regression, e2e mail path (AUTH, DKIM,
queued→sent, L1/L2, fail-open, hostname gate, session vs restart). Test
comments are better than average.
**Documented?** How to run tests: [development.md](../development.md) §
Testing. There is no e2e README (package comment in `test/e2e/main_test.go`
is the stand-in). Individual tests are not inventoried in docs — that is
fine; the e2e **summary** should mention logrotate.
**Gaps (high value):**
| Area | Gap |
|---|---|
| RBAC | **No** tests for `authz.go`, `CanAccessDomain`, domain-admin send-log scoping, `/users` 404 for domain-admin, backup 404. This is why P0 shipped. |
| Auth HTTP | No `HandleLogin` / `HandleSetup` tests (TTL, constant-time, setup complete → 404). No tests for `auth/ratelimit.go` `Allow`. |
| Sessions store | No `store/sessions*_test.go` (covered only via `auth_test` wrappers). |
| Backup as operator path | Create + `CheckRestore` unit-tested; **no** extract-onto-`/data`-and-boot test; panel `HandleBackup` POST untested. |
| Handlers | No tests for users CRUD, domain add/delete, account POST, DNS recheck endpoints. |
| `postfix.Queue` | Parser only; exec path untested (e2e does not open Mail queue). |
**Weak / low-value (keep, do not grow this style):**
- `TestDecryptErrorMessage` — substring mapping.
- `TestBackupPageOffersEncryption``strings.Contains` over HTML.
- Many `templates_test.go` cases — structural guards (CSP, nav). Valuable as
guards, not as behaviour tests.
- E2e `testNoAuthRejected` vs `testForeignRelayRejected` — nearly the same
unauthenticated send.
**E2e hygiene:**
- Fatal string in `testLevel1RateLimit` says `RATE_LIMIT_MESSAGES_PER_IP=5`;
override is `50` (`test/e2e/negative_test.go` vs `compose.override.yml`).
- `coredns/coredns:latest` is unpinned.
- `TestImageBuildPreservesLogrotateMode` chmods the source conf then rebuilds
— can race a dirty tree.
Do not add snapshot tests of entire pages. Add **authorization** tests that
would have caught P0.
---
## 15. Duplication and local patches
| Local patch | Systemic fix |
|---|---|
| Send-log domain/app allowlist after/around the query | Store filter: `Domains []string` required for non-global; validate app login first |
| `assignedDomains` loads all domains | Use `listUserDomainNames` / `ListDomainsForUser` |
| `requireGlobal` on each handler | Optional: `auth.RequireGlobal` on those muxes |
| Five JS field-sync helpers | One `data-show-when` helper |
| DNS host/type/value markup × many | Template partial |
| Settings credentials form × 2 | Partial |
| `web/validate` vs `app/validate` | Keep separate (different alphabets); do not merge |
The send-log allowlist is the textbook “local patch instead of a store
invariant”.
---
## 16. Other improvements
- Pin CoreDNS in e2e.
- Guard `release.yml` `workflow_dispatch` versioning (**Opus**, infra).
- Optional: `Resync` once after a successful `CheckRestore` (heal drifted
maps). Small, mail-path, **Opus**. Not required if the tarball is the
restore story — but then **fix the backup package comment**.
- Optional: serve `NOTICE` next to `/license`.
- Do not start inbound-relay until P0 is closed.
---
## Implementation checklist
Work top to bottom. Commit per phase (or per coherent sub-step) when asked.
Update [CHANGELOG.md](../../CHANGELOG.md) `[Unreleased]` with each user-visible
change. After Go changes: `go build`, `go vet`, `go test ./...`.
### P0 — Domain-admin send-log authorization (defect)
**Model: Opus.** Tests in the same change. **Fable** on the diff after it
lands (reviewer ≠ author).
- [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 users apps **before** query.
- [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).
- [x] Comment the invariant next to `sendLogData` (the comment that was
missing).
**Done when:** a domain-admin cannot read another domains send-log rows via
the list, the fragment, or query parameters. `go test ./...` green.
### P1 — Fail-closed consistency (mail path / auth)
**Model: Opus.**
- [x] `inflight.tryAdmit` (count + reserve under one lock). Extend milter
tests with overlapping `MailFrom` (true concurrency, not sequential).
Two tests: concurrent `MailFrom` sessions gated so they all read the
stored count before anyone reserves (exactly one admitted), and a
saturation test on `tryAdmit` that overshoots the ceiling whenever count
and reserve are separate critical sections.
- [x] `sessionStore.Create` returns an error; login does not set a cookie on
failure.
- [x] `app.Service.Delete`: SASL (and rate-limit row) before or compensating
with the registry row; match domain-delete ordering. Test the failure
path with a fake SASL that errors.
**Done when:** unit tests cover the race and the two fail-closed paths.
### P2 — Security/operator docs that are wrong today
**Model: Sonnet** (English docs). No code behaviour change except copy.
- [x] Rewrite the CSRF ADR in [security.md](../security.md) for a panel that
already has global + domain-admin. Keep the origin-check mechanism
unless a new decision says otherwise. New revisit trigger (e.g. untrusted
domain-admins, or a requirement that does not depend on browser
headers).
- [x] Drop “or argon2” unless argon2 is implemented.
- [x] [guide.md](../guide.md): Users / roles; Settings DMARC field; L2
fail-open; restore can resurrect sessions; domain-admin can export
working SASL passwords for assigned domains.
- [x] Architecture route table: `/license`, `/account``/settings`.
- [x] Fix `internal/backup` package comment (restore = extract tarball +
`CheckRestore`; maps come from the archive; Reload heals drift).
- [x] `admin.dmarc_report_email``settings` in roadmap + dmarc-reports plan.
- [x] development.md e2e summary: logrotate + process checks.
- [x] `setupManager` / `store` package comments: `users`, not `admin` row.
**Done when:** an operator who reads only the guide knows the panel has two
roles, and security.md no longer calls the panel single-user.
### P3 — GUI defects from the 1.2.x layout pass
**Model: Sonnet.**
- [x] `.flash.error` (or stop using `.flash` for `RateLimitErr`) — danger
surface, not success.
- [x] User delete: `data-confirm` at minimum; prefer a confirm page like
domain delete. Done as a confirm page (`GET/POST /users/{uid}/delete`),
matching `domain_delete.html`.
- [x] DNS field partial; settings credentials partial.
- [x] Optional: `urlquery` on deliveries fragment params; `aria-live` on
polled regions; confirm-without-JS note next to the CSRF accepted risks.
**Done when:** a rate-limit validation error is visually an error; user delete
cannot be a single unmarked click.
### P4 — Tests and e2e hygiene
**Model: Opus** for auth/RBAC/limiter tests; **Haiku** for the L1 fatal-string
typo; **Sonnet** if e2e docs need a paragraph.
- [x] `auth/ratelimit.go` unit tests (window, lockout, sweep). Also the
per-key scope: one locked-out address must not lock out the others.
- [x] Login/setup handler tests (happy path + lockout + setup expiry). The
lockout test also states that a correct password does not bypass it, and
that the two refusals are byte-identical (no username enumeration).
- [x] Domain-admin 404 on `/users`, `/backup`, `/mail-queue`, `/system-log`,
`/status` — as a table of every global-only route (`internal/web/handlers/authz_test.go`),
including the write routes, plus the same 404 for a request with no
principal and a positive control so the table cannot pass on a handler
that always 404s.
- [x] Fix e2e L1 fatal string (`50`, not `5`).
- [x] Pin `coredns` image: tag `1.14.6`, not a digest — the tag is a multi-arch
manifest and the stand has to come up on arm64 developer machines.
- [x] Optional: backup extract + `CheckRestore` + panel boot. Done as an
in-process integration test (`cmd/panel/restore_test.go`) rather than
e2e, so it runs in `go test ./...`: the archive is downloaded from a
running panel through `POST /backup` (which closes the “`HandleBackup`
POST untested” gap in §14 as well), unpacked the way `tar -xzf` unpacks
it, and a second panel is booted on the result through run()'s own
startup order. Also covers the encrypted download, the version-mismatch
refusal, that a restore does not reopen the setup link, and that sessions
travel in the archive. `serveHTTP` was split so the composition it
performs (`newPanel`) can be booted without binding a port.
**Done when:** P0 cannot regress without a red test; e2e L1 message matches
the override.
### P5 — Licence and release infra
**Model: Sonnet** for OFL/NOTICE prose; **Opus** for `release.yml`; **Haiku**
for SPDX consistency and deleting the done logrotate plan.
- [x] Add SIL OFL 1.1 text beside the Plex WOFF2 files; point `NOTICE` at it.
IBM Plex `LICENSE.txt` as `internal/web/view/static/OFL.txt` (copyright
+ OFL 1.1). Copied into the image at `/usr/share/doc/selfpost/OFL.txt`;
served at `/static/OFL.txt`.
- [x] `NOTICE` Source URL instructions → `internal/legal/legal.go`.
- [x] `release.yml`: `workflow_dispatch` must not publish `main` as a version
(require `vX.Y.Z` or an explicit `version` input that matches SemVer).
- [x] Delete [plans/logrotate-mode.md](logrotate-mode.md) (done; git keeps it).
- [x] Decide SPDX-everywhere vs SPDX-nowhere; do not leave two files special
without a one-line note in development.md. SPDX-nowhere: dropped the
two `cmd/` headers; development.md § External libraries records that
AGPL-3.0 does not require per-file SPDX.
**Done when:** OFL travels with the fonts; a dispatch from `main` cannot tag
`ghcr.io/...:main`.
### P6 — Optional cleanup (do not start until P0P3 are done)
**Model: Sonnet** unless noted.
- [x] Use `auth.RequireGlobal` or delete it (**Haiku** if delete). Deleted.
- [x] Rename `handlers_account.go` / `HandleAccount` to settings (**Haiku**).
- [x] `ListDomainsForUser` instead of load-all-and-filter.
- [x] Cap or periodically sweep the login limiter map (**Opus**, small).
- [x] Collapse `panel.js` field-sync helpers.
- [x] Soften or keep DMARC “future release” UI copy (product call). Softened.
- [ ] Optional startup `Resync` after restore (**Opus**). Dropped — P2 backup
comment fix is sufficient.
### P7 — Security review of the P0P1 diff
**Model: Fable.** Not authorship.
- [ ] Review the send-log authz change, `tryAdmit`, session create, and app
delete ordering against [security.md](../security.md). Close each finding
with a fix or an accepted-risk entry.
---
## What not to do
- Do not squash SQLite migrations in **1.x** (see roadmap `schema-squash` for 2.x).
- Do not implement inbound-relay, DMARC ingestion, or in-panel docs as part of
this plan.
- Do not add CSRF tokens in the same breath as rewriting the ADR. Tokens are a
new decision.
- Do not “simplify” comments that record threat models.
- Do not introduce a general service/repository layer for users.
---
## Done when (this plan)
1. P0 is shipped and covered by tests.
2. P2 has removed “single-user” from the CSRF ADR and documented Users in the
operator guide.
3. P3 flash/delete bugs are gone.
4. P7 has run on the P0P1 diff.
5. This files remaining boxes are either checked or explicitly dropped in
[roadmap.md](../roadmap.md) with a reason.
6. [CHANGELOG.md](../../CHANGELOG.md) `[Unreleased]` lists the user-visible
items (authz, docs, GUI).
After that, delete this plan (history in git) and return the recommended
order on the roadmap to inbound-relay.
+3 -2
View File
@@ -3,8 +3,9 @@
**Status:** agreed **Status:** agreed
**Version:** target bump **1.x** MINOR; **`2.x` possible** — to be settled once **Version:** target bump **1.x** MINOR; **`2.x` possible** — to be settled once
the implementation lands (do not fix a major in advance). the implementation lands (do not fix a major in advance).
**Order:** after [code-review.md](code-review.md) P0 (and preferably P0P3); **Order:** the 2026-08-13 full-tree review follow-ups are closed (CHANGELOG
this remains the largest remaining 1.x+ **feature**. The domain-admin role `[Unreleased]`), so this is next up as the largest remaining 1.x+ **feature**.
The domain-admin role
and `internal/web` split have shipped — see [CHANGELOG](../../CHANGELOG.md) and `internal/web` split have shipped — see [CHANGELOG](../../CHANGELOG.md)
`[1.2.0]`. `[1.2.0]`.
+1 -2
View File
@@ -3,8 +3,7 @@
**Status:** agreed **Status:** agreed
**Date:** 2026-08-13 **Date:** 2026-08-13
**Version:** patch; no schema, no configuration surface. **Version:** patch; no schema, no configuration surface.
**Order:** small panel item; does not wait on inbound-relay. May follow or **Order:** small panel item; does not wait on inbound-relay.
interleave with remaining [code-review.md](code-review.md) work after P0.
--- ---
+6 -28
View File
@@ -28,7 +28,6 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md).
| ID | Topic | Status | Plan | | ID | Topic | Status | Plan |
|---|---|---|---| |---|---|---|---|
| code-review | Full-tree review follow-ups (authz, docs, GUI, tests) | **agreed** | [plans/code-review.md](plans/code-review.md) |
| queue-retries | Postfix retry policy in the panel (queue lifetime, backoff) | **agreed** | [plans/queue-retries.md](plans/queue-retries.md) | | queue-retries | Postfix retry policy in the panel (queue lifetime, backoff) | **agreed** | [plans/queue-retries.md](plans/queue-retries.md) |
| inbound-relay | Inbound relay (backup-MX / forwarding) | **agreed** | [plans/inbound-relay.md](plans/inbound-relay.md) | | inbound-relay | Inbound relay (backup-MX / forwarding) | **agreed** | [plans/inbound-relay.md](plans/inbound-relay.md) |
| contributing | `CONTRIBUTING.md` | candidate | — | | contributing | `CONTRIBUTING.md` | candidate | — |
@@ -36,39 +35,18 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md).
| panel-docs | In-panel operator documentation | candidate | — | | panel-docs | In-panel operator documentation | candidate | — |
| schema-squash | Squash SQLite migrations into a 2.x baseline | **2.x** | — | | schema-squash | Squash SQLite migrations into a 2.x baseline | **2.x** | — |
**Recommended order** (not binding): **code-review P0** first (shipped **Recommended order** (not binding): **queue-retries** is a small panel item
domain-admin send-log leak — a defect, not a feature), then the rest of that that can land first or in parallel; the next feature is **inbound-relay**.
plan as listed; **queue-retries** is a small panel item that can land in The 2026-08-13 full-tree review follow-ups (send-log authorization,
parallel after P0; then **inbound-relay**. Candidates need explicit agreement fail-closed paths, docs, GUI, tests, licence) are closed — history in
before they join the queue. [CHANGELOG.md](../CHANGELOG.md) `[Unreleased]` and git. Candidates need
explicit agreement before they join the queue.
After a context reset, pick an item marked `agreed` or `in progress`, then work After a context reset, pick an item marked `agreed` or `in progress`, then work
the checklist in its linked plan. the checklist in its linked plan.
--- ---
## code-review
**Goal:** close the 2026-08-13 full-tree review: domain-admin send-log
authorization, a few fail-closed paths, docs that still say “single-user”,
GUI flash/delete bugs, test gaps, licence/release hygiene.
**Boundary:** defects and docs/UI follow-ups inside the current 1.x product.
Not inbound-relay, not DMARC ingestion, not a layer rewrite.
**Done when:** see the criteria in
[plans/code-review.md](plans/code-review.md).
**Progress:** P0P5 done. Remaining: P6 (optional cleanup), P7 (Fable review
of the P0P1 diff).
**Dependencies / risks:** P0 is confidentiality between panel roles; it
jumps the feature queue. Implementation models are in the plan (Opus / Sonnet
/ Haiku / Fable per [development.md](development.md)).
**Version:** patch.
---
## queue-retries ## queue-retries
**Goal:** show on Mail queue and on a delivery's history how this Postfix **Goal:** show on Mail queue and on a delivery's history how this Postfix
+6 -1
View File
@@ -5,7 +5,12 @@ meet; the full v1.0 audit passed. The pre-release review (plan § D, 2026-08-06)
covered the whole diff from the v1.0 audit (Phase 11) to HEAD and the checklist covered the whole diff from the v1.0 audit (Phase 11) to HEAD and the checklist
in full: no exploitable findings; one defence-in-depth change — `--` before the in full: no exploitable findings; one defence-in-depth change — `--` before the
login in the `saslpasswd2` argv login in the `saslpasswd2` argv
([internal/app/sasl.go](../internal/app/sasl.go)). (2) **Accepted risks** ([internal/app/sasl.go](../internal/app/sasl.go)). The 2026-08-14 review
(code-review plan § P7, Fable; reviewer ≠ author) covered the P0P1 diff of the
2026-08-13 full-tree review against this document — send-log authorization for
domain administrators, the atomic level-2 admit (`tryAdmit`), fail-closed
session creation, and application-delete ordering: no findings, and nothing
needed adding to the accepted risks. (2) **Accepted risks**
deliberate departures beyond the mandatory, recorded so the decision is not deliberate departures beyond the mandatory, recorded so the decision is not
lost. lost.
+21 -19
View File
@@ -9,15 +9,16 @@
// Restore is not a separate code path in the panel: a backup is extracted into // Restore is not a separate code path in the panel: a backup is extracted into
// the /data bind mount before first start, and the archive already carries // the /data bind mount before first start, and the archive already carries
// everything the mail path needs — DKIM keys, sasldb2, and Postfix's sender // everything the mail path needs — DKIM keys, sasldb2, and Postfix's sender
// map — so nothing needs to be regenerated from SQLite for the daemons to // map — so the daemons can start on the extracted files alone. The
// start correctly. The only restore-specific step the panel runs is // restore-specific steps the panel runs are CheckRestore, which refuses to
// CheckRestore, which refuses to boot if the manifest's version does not match // boot if the manifest's version does not match the running binary so
// the running binary, so schema/format skew between versions cannot silently // schema/format skew between versions cannot silently corrupt state
// corrupt state (architecture.md § Persistence). If a daemon's on-disk state // (architecture.md § Persistence), and a one-time Resync of OpenDKIM's tables
// ever drifts from what SQLite records — for example after a manual edit // and the Postfix sender map from SQLite on that first boot, so any drift
// under /data — the Status page's "Reload configuration" button re-derives // between the archive and the database is healed before mail flows. If
// OpenDKIM's tables and the Postfix sender map from the database; that is a // on-disk state drifts again later — for example after a manual edit under
// deliberate, operator-triggered heal, not something restore does on its own. // /data — the Status page's "Reload configuration" button runs the same
// Resync on demand.
package backup package backup
import ( import (
@@ -269,33 +270,34 @@ func snapshotDB(dbPath string) (path string, cleanup func(), err error) {
// directory), its version must match binaryVersion or the panel refuses to // directory), its version must match binaryVersion or the panel refuses to
// start, telling the operator which image tag to use. On a match the manifest // start, telling the operator which image tag to use. On a match the manifest
// is consumed (deleted) so it guards only the first boot after a restore and // is consumed (deleted) so it guards only the first boot after a restore and
// never blocks a later in-place image upgrade. Absence of the manifest is the // never blocks a later in-place image upgrade, and restored is true so the
// normal case and returns nil. // caller can heal drifted daemon maps once. Absence of the manifest is the
func CheckRestore(manifestPath, binaryVersion string) error { // normal case and returns restored == false with a nil error.
func CheckRestore(manifestPath, binaryVersion string) (restored bool, err error) {
data, err := os.ReadFile(manifestPath) data, err := os.ReadFile(manifestPath)
if os.IsNotExist(err) { if os.IsNotExist(err) {
return nil // ordinary start, not a restore return false, nil // ordinary start, not a restore
} }
if err != nil { if err != nil {
return fmt.Errorf("backup: read restore manifest: %w", err) return false, fmt.Errorf("backup: read restore manifest: %w", err)
} }
var m Manifest var m Manifest
if err := json.Unmarshal(data, &m); err != nil { if err := json.Unmarshal(data, &m); err != nil {
return fmt.Errorf("backup: restore manifest %s is not valid JSON: %w", manifestPath, err) return false, fmt.Errorf("backup: restore manifest %s is not valid JSON: %w", manifestPath, err)
} }
if m.Format != FormatFull { if m.Format != FormatFull {
return fmt.Errorf("backup: %s is not a SelfPost full backup manifest (format %q)", manifestPath, m.Format) return false, fmt.Errorf("backup: %s is not a SelfPost full backup manifest (format %q)", manifestPath, m.Format)
} }
if m.Version != binaryVersion { if m.Version != binaryVersion {
return fmt.Errorf( return false, fmt.Errorf(
"backup: this backup was created by SelfPost %s but this image is %s — restore into the matching image (selfpost:%s)", "backup: this backup was created by SelfPost %s but this image is %s — restore into the matching image (selfpost:%s)",
m.Version, binaryVersion, m.Version) m.Version, binaryVersion, m.Version)
} }
// Version matches: consume the manifest so subsequent normal starts (and // Version matches: consume the manifest so subsequent normal starts (and
// in-place upgrades) are not gated by it. // in-place upgrades) are not gated by it.
if err := os.Remove(manifestPath); err != nil { if err := os.Remove(manifestPath); err != nil {
return fmt.Errorf("backup: consume restore manifest: %w", err) return false, fmt.Errorf("backup: consume restore manifest: %w", err)
} }
return nil return true, nil
} }
+19 -4
View File
@@ -156,17 +156,25 @@ func writeManifest(t *testing.T, dir, format, version string) string {
} }
func TestCheckRestoreNoManifestIsNormalStart(t *testing.T) { func TestCheckRestoreNoManifestIsNormalStart(t *testing.T) {
if err := CheckRestore(filepath.Join(t.TempDir(), "manifest.json"), "1.0.0"); err != nil { restored, err := CheckRestore(filepath.Join(t.TempDir(), "manifest.json"), "1.0.0")
if err != nil {
t.Errorf("CheckRestore with no manifest = %v, want nil", err) t.Errorf("CheckRestore with no manifest = %v, want nil", err)
} }
if restored {
t.Error("CheckRestore with no manifest reported a restore")
}
} }
func TestCheckRestoreMatchConsumesManifest(t *testing.T) { func TestCheckRestoreMatchConsumesManifest(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()
path := writeManifest(t, dir, FormatFull, "1.0.0") path := writeManifest(t, dir, FormatFull, "1.0.0")
if err := CheckRestore(path, "1.0.0"); err != nil { restored, err := CheckRestore(path, "1.0.0")
if err != nil {
t.Fatalf("CheckRestore matching = %v, want nil", err) t.Fatalf("CheckRestore matching = %v, want nil", err)
} }
if !restored {
t.Fatal("CheckRestore matching did not report a restore")
}
if _, err := os.Stat(path); !os.IsNotExist(err) { if _, err := os.Stat(path); !os.IsNotExist(err) {
t.Errorf("manifest should be consumed after a matching restore, stat err = %v", err) t.Errorf("manifest should be consumed after a matching restore, stat err = %v", err)
} }
@@ -175,10 +183,13 @@ func TestCheckRestoreMatchConsumesManifest(t *testing.T) {
func TestCheckRestoreVersionMismatchRefusesAndKeeps(t *testing.T) { func TestCheckRestoreVersionMismatchRefusesAndKeeps(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()
path := writeManifest(t, dir, FormatFull, "1.0.0") path := writeManifest(t, dir, FormatFull, "1.0.0")
err := CheckRestore(path, "2.0.0") restored, err := CheckRestore(path, "2.0.0")
if err == nil { if err == nil {
t.Fatal("CheckRestore mismatch = nil, want error") t.Fatal("CheckRestore mismatch = nil, want error")
} }
if restored {
t.Error("CheckRestore mismatch reported a restore")
}
if !strings.Contains(err.Error(), "1.0.0") || !strings.Contains(err.Error(), "2.0.0") { if !strings.Contains(err.Error(), "1.0.0") || !strings.Contains(err.Error(), "2.0.0") {
t.Errorf("error should name both versions: %v", err) t.Errorf("error should name both versions: %v", err)
} }
@@ -190,7 +201,11 @@ func TestCheckRestoreVersionMismatchRefusesAndKeeps(t *testing.T) {
func TestCheckRestoreWrongFormatRejected(t *testing.T) { func TestCheckRestoreWrongFormatRejected(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()
path := writeManifest(t, dir, "something-else", "1.0.0") path := writeManifest(t, dir, "something-else", "1.0.0")
if err := CheckRestore(path, "1.0.0"); err == nil { restored, err := CheckRestore(path, "1.0.0")
if err == nil {
t.Error("CheckRestore accepted a non-backup manifest") t.Error("CheckRestore accepted a non-backup manifest")
} }
if restored {
t.Error("CheckRestore wrong format reported a restore")
}
} }
+8
View File
@@ -167,6 +167,14 @@ func (o *OpenDKIM) Reload() error {
return o.reload() return o.reload()
} }
// SetReloadHook replaces how configuration is applied after a rebuild. Tests
// that cannot reach supervisord use this to verify file regeneration alone.
func (o *OpenDKIM) SetReloadHook(fn func() error) {
if fn != nil {
o.reload = fn
}
}
// renderTables builds the KeyTable and SigningTable byte contents for a domain // renderTables builds the KeyTable and SigningTable byte contents for a domain
// set, sorted by name so the output is deterministic. Every domain is // set, sorted by name so the output is deterministic. Every domain is
// re-checked for shell/config-injection safety before being written // re-checked for shell/config-injection safety before being written
+8
View File
@@ -75,6 +75,14 @@ func (p *Postfix) Reload() error {
return p.reload() return p.reload()
} }
// SetReloadHook replaces how configuration is applied after a rebuild. Tests
// that cannot reach supervisord use this to verify file regeneration alone.
func (p *Postfix) SetReloadHook(fn func() error) {
if fn != nil {
p.reload = fn
}
}
// renderSenderLoginMaps builds the sender_login_maps file contents. Keys are // renderSenderLoginMaps builds the sender_login_maps file contents. Keys are
// sorted for deterministic output and the logins under each key are sorted and // sorted for deterministic output and the logins under each key are sorted and
// de-duplicated. Every address and login is re-checked for injection safety // de-duplicated. Every address and login is re-checked for injection safety